On Tue, 20 Apr 2021 20:59:57 +0200
Andrew Jones <drjones@xxxxxxxxxx> wrote:
Hi,
> Don't assume the physical addresses used with PCI have already been
> identity mapped.
>
> Reviewed-by: Nikos Nikoleris <nikos.nikoleris@xxxxxxx>
> Signed-off-by: Andrew Jones <drjones@xxxxxxxxxx>
> ---
> lib/pci-host-generic.c | 5 ++---
> lib/pci-host-generic.h | 4 ++--
> lib/pci-testdev.c | 4 ++++
> 3 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
> index 818150dc0a66..de93b8feac39 100644
> --- a/lib/pci-host-generic.c
> +++ b/lib/pci-host-generic.c
> @@ -122,7 +122,7 @@ static struct pci_host_bridge *pci_dt_probe(void)
> sizeof(host->addr_space[0]) * nr_addr_spaces);
> assert(host != NULL);
>
> - host->start = base.addr;
> + host->start = ioremap(base.addr, base.size);
> host->size = base.size;
> host->bus = bus;
> host->bus_max = bus_max;
> @@ -279,8 +279,7 @@ phys_addr_t pci_host_bridge_get_paddr(u64 pci_addr)
>
> static void __iomem *pci_get_dev_conf(struct pci_host_bridge *host, int devfn)
> {
> - return (void __iomem *)(unsigned long)
> - host->start + (devfn << PCI_ECAM_DEVFN_SHIFT);
> + return (void __iomem *)host->start + (devfn << PCI_ECAM_DEVFN_SHIFT);
But host->start's type is now exactly "void __iomem *", so why the cast?
And are we OK with doing pointer arithmetic on a void pointer?
> }
>
> u8 pci_config_readb(pcidevaddr_t dev, u8 off)
> diff --git a/lib/pci-host-generic.h b/lib/pci-host-generic.h
> index fd30e7c74ed8..0ffe6380ec8f 100644
> --- a/lib/pci-host-generic.h
> +++ b/lib/pci-host-generic.h
> @@ -18,8 +18,8 @@ struct pci_addr_space {
> };
>
> struct pci_host_bridge {
> - phys_addr_t start;
> - phys_addr_t size;
> + void __iomem *start;
> + size_t size;
> int bus;
> int bus_max;
> int nr_addr_spaces;
> diff --git a/lib/pci-testdev.c b/lib/pci-testdev.c
> index 039bb44781c1..4f2e5663b2d6 100644
> --- a/lib/pci-testdev.c
> +++ b/lib/pci-testdev.c
> @@ -185,7 +185,11 @@ int pci_testdev(void)
> mem = ioremap(addr, PAGE_SIZE);
>
> addr = pci_bar_get_addr(&pci_dev, 1);
> +#if defined(__i386__) || defined(__x86_64__)
> io = (void *)(unsigned long)addr;
> +#else
> + io = ioremap(addr, PAGE_SIZE);
> +#endif
I am bit puzzled: For anything but x86 ioremap() is implemented like the
first statement, so why do we differentiate here? Shouldn't either one
of the statements be fine, for all architectures?
Cheers,
Andre
>
> nr_tests += pci_testdev_all(mem, &pci_testdev_mem_ops);
> nr_tests += pci_testdev_all(io, &pci_testdev_io_ops);
