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); > > +}