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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2327 bytes --]

On Wed, Jan 29, 2020 at 02:54:19PM +1100, Oliver O'Halloran wrote:
> On Wed, Jan 29, 2020 at 2:09 PM David Gibson
> <david@gibson.dropbear.id.au> wrote:
> >
> > On Mon, Jan 27, 2020 at 03:45:05PM +0100, Cédric Le Goater wrote:
> > > From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > >
> 
> *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.

Ah, right, that makes sense.

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

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]