On Wed, Jan 29, 2020 at 2:09 PM David Gibson
<da...@gibson.dropbear.id.au> wrote:
>
> On Mon, Jan 27, 2020 at 03:45:05PM +0100, Cédric Le Goater wrote:
> > From: Benjamin Herrenschmidt <b...@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.
> > + break;
> > + case 4:
> > + val = bswap32(val);
> > + break;
> > + default:
> > + g_assert_not_reached();
> > + }
> > + pci_host_config_write_common(pdev, cfg_addr, limit, val, size);
> > +}
