Hi Thomas,
pon., 15 lip 2019 o 17:08 Thomas Petazzoni
<thomas.petazzoni@xxxxxxxxxxx> napisał(a):
>
>
> > - bridge->conf.vendor = advk_readl(pcie, PCIE_CORE_DEV_ID_REG) & 0xffff;
> > - bridge->conf.device = advk_readl(pcie, PCIE_CORE_DEV_ID_REG) >> 16;
> > + bridge->conf.vendor =
> > + cpu_to_le16(advk_readl(pcie, PCIE_CORE_DEV_ID_REG) & 0xffff);
> > + bridge->conf.device =
> > + cpu_to_le16(advk_readl(pcie, PCIE_CORE_DEV_ID_REG) >> 16);
> > bridge->conf.class_revision =
> > advk_readl(pcie, PCIE_CORE_DEV_REV_REG) & 0xff;
>
> So conf.vendor and conf.device and stored as little-endian in the
> emulated config address space, but conf.class_revision is stored in the
> CPU endianness ?
Thank you it seems it slip over - after my change I dump whole config
space in big and little endian and compere to be sure that there are
no more fields that Iam missing and everything seemed ok - so it is
probably '0' therefore the bug wasn't caught.
In bridge emulation the conversion is done correctly:
bridge->conf.class_revision |= cpu_to_le32(PCI_CLASS_BRIDGE_PCI << 16);
>
> >
> > @@ -489,8 +491,8 @@ static void advk_sw_pci_bridge_init(struct advk_pcie *pcie)
> > bridge->conf.iolimit = PCI_IO_RANGE_TYPE_32;
>
> >
> > /* Support 64 bits memory pref */
> > - bridge->conf.pref_mem_base = PCI_PREF_RANGE_TYPE_64;
> > - bridge->conf.pref_mem_limit = PCI_PREF_RANGE_TYPE_64;
> > + bridge->conf.pref_mem_base = cpu_to_le16(PCI_PREF_RANGE_TYPE_64);
> > + bridge->conf.pref_mem_limit = cpu_to_le16(PCI_PREF_RANGE_TYPE_64);
>
> Same here: why are conf.pref_mem_{base,limit} converted to LE, but not
> conf.iolimit ?
Here we are ok, since iobase and iolimit are 1byte wide.
>
>
> Also, the advk_pci_bridge_emul_pcie_conf_read() and
> advk_pci_bridge_emul_pcie_conf_write() return values that are in the
> CPU endianness.
>
> Am I missing something ?
Yes because we are mixing the 4byte accesses in
advk_pci_bridge_emul_pcie_conf_read/write with u16 and u8 accesses
when referring to structure fields directly.
E.g. please see what will happen when in BE e.g. device id and vendor
id are set via conf->vendor and conf->device and then read via
advk_pci_bridge_emul_pcie_conf_read which first read whole 32bit value
and then shift it. The same with other not u32 fields.
Before my changes PCIe didn't work in BE mode at all - I've tested it
on a3700. Nevertheless Russell advice about Sparse validation is
really good - it helps to detect some bugs which slip over - I will
send v2 soon.
Best regards,
Grzegorz
