Hello Grzegorz,
Thanks for this work. I indeed never tested this code on BE platforms,
and it is very possible that I overlooked endianness issues, so thanks
for having a look at this and proposing some patches. See some
questions/comments below.
On Mon, 15 Jul 2019 16:15:22 +0200
Grzegorz Jaszczyk <jaz@xxxxxxxxxxxx> wrote:
> Initialise every not-byte wide fields of emulated pci bridge config
> space with proper cpu_to_le* macro. This is required since the structure
> describing config space of emulated bridge assumes little-endian
> convention.
>
> Signed-off-by: Grzegorz Jaszczyk <jaz@xxxxxxxxxxxx>
> ---
> drivers/pci/controller/pci-aardvark.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index 134e030..06a12749 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -479,8 +479,10 @@ static void advk_sw_pci_bridge_init(struct advk_pcie *pcie)
> {
> struct pci_bridge_emul *bridge = &pcie->bridge;
>
> - 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 ?
>
> @@ -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 ?
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 ?
Thomas
--
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
