Re: [PATCH v5] PCI: Unify ECAM constants in native PCI Express drivers

["Krzysztof Wilczyński" <kw@linux.com>]

[+CC David for visibility]

Hi Bjorn,

Thank you for the review!

On 20-11-28 12:35:16, Bjorn Helgaas wrote:
[...]
> It's ironic that we don't use PCIE_ECAM_OFFSET in drivers/pci/ecam.c.
> We could do something like this, which would also let us drop
> .bus_shift completely in all the conforming implementations.  It also
> closes the hole that we didn't limit "where" to 4K for
> pci_ecam_map_bus() users.
> 
>   if (per_bus_mapping) {
>     base = cfg->winp[busn];
>     busn = 0;
>   } else {
>     base = cfg->win;
>   }
> 
>   if (cfg->ops->bus_shift) {
>     u32 bus_offset = (busn & 0xff) << cfg->ops->bus_shift;
>     u32 devfn_offset = (devfn & 0xff) << (cfg->ops->bus_shift - 8);
> 
>     where &= 0xfff;
> 
>     return base + (bus_offset | devfn_offset | where);
>   }
> 
>   return base + PCIE_ECAM_OFFSET(busn, devfn, where);
[...]

Thank you for suggesting this!  I sent v6 recently that includes this.

> >  static void __iomem *ppc4xx_pciex_get_config_base(struct ppc4xx_pciex_port *port,
> >  						  struct pci_bus *bus,
> > -						  unsigned int devfn)
> > +						  unsigned int devfn,
> > +						  int offset)
> 
> The interface change (to add "offset") could be a preparatory patch by
> itself.
> 
> But I'm actually not sure it's worth even touching this file.  This is
> the only place outside drivers/pci that includes linux/pci-ecam.h.  I
> think I might rather put PCIE_ECAM_OFFSET() and related things in
> drivers/pci/pci.h and keep it all inside drivers/pci.

Makes sense to drop it.  We can always introduce chances on PPC 4xx
platform in the future if we ever want it to leverage all the new macros
and constants.

These changes are not included in v6.

> >  static const struct pci_ecam_ops pci_thunder_pem_ops = {
> > -	.bus_shift	= 24,
> > +	.bus_shift	= THUNDER_PCIE_ECAM_BUS_SHIFT,
> >  	.init		= thunder_pem_platform_init,
> >  	.pci_ops	= {
> >  		.map_bus	= pci_ecam_map_bus,
> 
> This could be split to its own patch, no big deal either way.

Done.  v6 is now a series that includes this as a separate patch.

> >  const struct pci_ecam_ops xgene_v2_pcie_ecam_ops = {
> > -	.bus_shift	= 16,
> >  	.init		= xgene_v2_pcie_ecam_init,
> >  	.pci_ops	= {
> >  		.map_bus	= xgene_pcie_map_bus,
> 
> Thanks for mentioning this change in the cover letter.  It could also
> be split off to a preparatory patch, since it's not related to
> PCIE_ECAM_OFFSET(), which is the main point of this patch.

Done.
 
> >  static void __iomem *iproc_pcie_map_ep_cfg_reg(struct iproc_pcie *pcie,
> >  					       unsigned int busno,
> > -					       unsigned int slot,
> > -					       unsigned int fn,
> > +					       unsigned int devfn,
> 
> This interface change *could* be a separate preparatory patch, too,
> but I'm starting to feel even more OCD than usual :)

Done.  It's a separate patch in v6, although I kept it together with the
change to introduce the PCIE_ECAM_OFFSET() macro since I was retiring the
use of PCI_SLOT() and PCI_FUNC() macros.

> > @@ -94,7 +95,7 @@ struct vmd_dev {
> >  	struct pci_dev		*dev;
> >  
> >  	spinlock_t		cfg_lock;
> > -	char __iomem		*cfgbar;
> > +	void __iomem		*cfgbar;
> 
> This type change might be worth pushing to a separate patch since the
> casting issues are not completely trivial.

Done.

The patch included in the series as part of v6 already got a review from
David Laight (thank you!) who suggests that this might not be a good
idea to do, and keeping existing type would be better.

Krzysztof