Re: [PATCH] PCI: aardvark: fix big endian support

[Thomas Petazzoni <thomas.petazzoni@bootlin.com>]

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@semihalf.com> 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@semihalf.com>
> ---
>  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