Re: [PATCH 1/2] ppc/pnv: Add models for POWER9 PHB4 PCIe Host bridge

[Oliver O'Halloran]

On Wed, Jan 29, 2020 at 2:09 PM David Gibson
<address@hidden> wrote:
>
> On Mon, Jan 27, 2020 at 03:45:05PM +0100, Cédric Le Goater wrote:
> > From: Benjamin Herrenschmidt <address@hidden>
> >

*snip*

> > +
> > +/*
> > + * The CONFIG_DATA register expects little endian accesses, but as the
> > + * region is big endian, we have to swap the value.
> > + */
> > +static void pnv_phb4_config_write(PnvPHB4 *phb, unsigned off,
> > +                                  unsigned size, uint64_t val)
> > +{
> > +    uint32_t cfg_addr, limit;
> > +    PCIDevice *pdev;
> > +
> > +    pdev = pnv_phb4_find_cfg_dev(phb);
> > +    if (!pdev) {
> > +        return;
> > +    }
> > +    cfg_addr = (phb->regs[PHB_CONFIG_ADDRESS >> 3] >> 32) & 0xffc;
> > +    cfg_addr |= off;
> > +    limit = pci_config_size(pdev);
> > +    if (limit <= cfg_addr) {
> > +        /*
> > +         * conventional pci device can be behind pcie-to-pci bridge.
> > +         * 256 <= addr < 4K has no effects.
> > +         */
> > +        return;
> > +    }
> > +    switch (size) {
> > +    case 1:
> > +        break;
> > +    case 2:
> > +        val = bswap16(val);
>
> I'm a little confused by these byteswaps.  As I see below the device
> is set to big endian, so the values passed in here should already be
> in host-native endian.  Why do you need the swap?  Are some of the
> registers in the bank BE and some LE?

All the registers are BE except for CONFIG_DATA, which isn't actually
a register. It's really a window into the config space of the device
specified in CONFIG_ADDR so it doesn't do any byte-swapping.

> > +        break;
> > +    case 4:
> > +        val = bswap32(val);
> > +        break;
> > +    default:
> > +        g_assert_not_reached();
> > +    }
> > +    pci_host_config_write_common(pdev, cfg_addr, limit, val, size);
> > +}