Re: [PATCH v1 06/14] xen/arm: Add support for PCI ecam operations

[Stefano Stabellini <sstabellini@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4482 bytes --]

On Tue, 14 Sep 2021, Rahul Singh wrote:
> >> +        return NULL;
> >> +
> >> +    busn -= cfg->busn_start;
> >> +    base = cfg->win + (busn << cfg->ops->bus_shift);
> >> +
> >> +    return base + (PCI_DEVFN(sbdf_t.dev, sbdf_t.fn) << devfn_shift) + where;
> >> +}
> > 
> > I understand that the arm32 part is not implemented and not part of this
> > series, that's fine. However if the plan is that arm32 will dynamically
> > map each bus individually, then I imagine this function will have an
> > ioremap in the arm32 version. Which means that we also need an
> > unmap_bus call in struct pci_ops. I understand that pci_ecam_unmap_bus
> > would be a NOP today for arm64, but I think it makes sense to have it if
> > we want the API to be generic.
> 
> As per my understanding we don’t need pci_ecam_unmap_bus(..) as I don’t see any use case to unmap the 
> bus dynamically. We can add the support for per_bus_mapping for ARM32 once we implement arm32 part.
> Let me know your view on this.

In the patch titled "xen/arm: PCI host bridge discovery within XEN on
ARM" there is the following in-code comment:

* On 64-bit systems, we do a single ioremap for the whole config space
* since we have enough virtual address range available.  On 32-bit, we
* ioremap the config space for each bus individually.
*
* As of now only 64-bit is supported 32-bit is not supported.


So I take it that on arm32 we don't have enough virtual address range
available, therefore we cannot ioremap the whole range. Instead, we'll
have to ioremap the config space of each bus individually.

I assumed that the idea was to call ioremap and iounmap dynamically,
otherwise the total amount of virtual address range required would still
be the same. I also thought that the dynamic mapping function, the one
which would end up calling ioremap on arm32, would be pci_ecam_map_bus.
If so, then we are missing the corresponding unmapping function, the one
that would call iounmap on arm32 and do nothing on arm64, called before
returning from pci_generic_config_read and pci_generic_config_write.

As always, I am not asking for the arm32 implementation, but if we are
introducing internal APIs, it would be good that they are consistent.



> >> @@ -50,10 +51,41 @@ struct pci_host_bridge {
> >>     u8 bus_start;                    /* Bus start of this bridge. */
> >>     u8 bus_end;                      /* Bus end of this bridge. */
> >>     void *sysdata;                   /* Pointer to the config space window*/
> >> +    const struct pci_ops *ops;
> >> };
> >> 
> >> +struct pci_ops {
> >> +    void __iomem *(*map_bus)(struct pci_host_bridge *bridge, uint32_t sbdf,
> >> +                             uint32_t offset);
> >> +    int (*read)(struct pci_host_bridge *bridge, uint32_t sbdf,
> >> +                uint32_t reg, uint32_t len, uint32_t *value);
> >> +    int (*write)(struct pci_host_bridge *bridge, uint32_t sbdf,
> >> +                 uint32_t reg, uint32_t len, uint32_t value);
> >> +};
> >> +
> >> +/*
> >> + * struct to hold pci ops and bus shift of the config window
> >> + * for a PCI controller.
> >> + */
> >> +struct pci_ecam_ops {
> >> +    unsigned int            bus_shift;
> >> +    struct pci_ops          pci_ops;
> >> +    int (*init)(struct pci_config_window *);
> > 
> > Is this last member of the struct needed/used?
> 
> Yes It will be used by some vendor specific board( N1SDP ). Please check below.
> https://git.linaro.org/landing-teams/working/arm/n1sdp-pcie-quirk.git/commit/?id=04b7e76d0fe6481a803f58e54e008a1489d713a5

OK. I don't doubt that there might be bridge-specific initializations,
but we already have things like:

DT_DEVICE_START(pci_gen, "PCI HOST ZYNQMP", DEVICE_PCI)
.dt_match = gen_pci_dt_match,
.init = gen_pci_dt_init,
DT_DEVICE_END

The question is do we actually need two different vendor-specific init
functions? One is driven by device tree, e.g. ZynqMP is calling
gen_pci_dt_init. The other one is pci_ecam_ops->init, which is called
from the following chain:

DT_DEVICE_START -> gen_pci_dt_init -> pci_host_common_probe ->
gen_pci_init -> pci_generic_ecam_ops.init

What's the difference between gen_pci_dt_init and pci_ecam_ops.init in
terms of purpose?

I am happy to have pci_ecam_ops.init if it serves a different purpose
from DT_DEVICE_START.init. In that case we might want to add an in-code
comment so that an engineer would know where to add vendor-specific code
(whether to DT_DEVICE_START.init or to pci_ecam_ops.init).