RE: [RFC PATCH V2 1/3] PCI: hisi: re-architect Hip05/Hip06 controllers driver to preapare for ACPI

[Gabriele Paoloni]

Hi Arnd, thanks for your comments

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@xxxxxxxx]
> Sent: 01 September 2016 15:02
> To: liudongdong (C)
> Cc: helgaas@xxxxxxxxxx; rafael@xxxxxxxxxx; Lorenzo.Pieralisi@xxxxxxx;
> tn@xxxxxxxxxxxx; Wangzhou (B); pratyush.anand@xxxxxxxxx; linux-
> pci@xxxxxxxxxxxxxxx; linux-acpi@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; jcm@xxxxxxxxxx; Gabriele Paoloni; Chenxin
> (Charles); hanjun.guo@xxxxxxxxxx; Linuxarm
> Subject: Re: [RFC PATCH V2 1/3] PCI: hisi: re-architect Hip05/Hip06
> controllers driver to preapare for ACPI
> 
> On Thursday, September 1, 2016 8:44:49 PM CEST Dongdong Liu wrote:
> > å 2016/9/1 15:41, Arnd Bergmann åé:
> > > On Thursday, September 1, 2016 10:05:29 AM CEST Dongdong Liu wrote:
> > >> å 2016/8/31 19:45, Arnd Bergmann åé:
> > >>> On Wednesday, August 31, 2016 7:48:12 PM CEST Dongdong Liu wrote:
> > I know your point.
> >
> > 1. For our host bridge , ".map_bus = pci_ecam_map_bus" is only
> suitable for
> > accessing the EP config space.
> > pci_generic_config_read32() need to call "addr = bus->ops-
> >map_bus(bus, devfn, where & ~0x3);",
> >
> > drivers/pci/host/pcie-hisi-acpi.c
> > static struct pci_ops hisi_pcie_ops = {
> > 	.map_bus = pci_ecam_map_bus,
> > 	.read = hisi_pcie_acpi_rd_conf,
> > 	.write = hisi_pcie_acpi_wr_conf,
> > };
> >
> > Yes, we can change ".map_bus = pci_ecam_map_bus" to ".map_bus =
> hisi_pci_map_bus", and implentment hisi_pci_map_bus as below,
> > then we will not need to call hisi_pcie_common_cfg_read().
> >
> > void __iomem *hisi_pci_map_bus(struct pci_bus *bus, unsigned int
> devfn, int where)
> > {
> > 	struct pci_config_window *cfg = bus->sysdata;
> > 	void __iomem *reg_base = cfg->priv;
> >
> > 	/* for RC config access*/
> > 	if (bus->number == cfg->busr.start)
> > 		return reg_base + (where & ~0x3);
> > 	else
> > 		/* for EP config access */
> > 		return pci_ecam_map_bus(bus, devfn, where);
> > }
> >
> > and hisi_pcie_acpi_rd_conf() need to change as below.
> > static int hisi_pcie_acpi_rd_conf(struct pci_bus *bus, u32 devfn, int
> where,
> > 				  int size, u32 *val)
> > {
> > 	struct pci_config_window *cfg = bus->sysdata;
> >
> > 	if (hisi_pcie_acpi_valid_config(cfg, bus, PCI_SLOT(devfn)) == 0)
> > 		return PCIBIOS_DEVICE_NOT_FOUND;
> >
> > 	/* access RC config space */
> > 	if (bus->number == cfg->busr.start)
> > 		return pci_generic_config_read32(bus, devfn, where, size,
> val);
> >
> > 	/* access EP config space */
> > 	return pci_generic_config_read(bus, devfn, where, size, val);
> > }
> 
> Right, this is what I had in mind.
> 
> 
> > 2. We need to backward compatible with the old dt way config access
> as below code,
> > so we have to call hisi_pcie_common_cfg_read() when accessing the RC
> config space.
> > For this, we have to call hisi_pcie_common_cfg_read().
> >
> > drivers/pci/host/pcie-hisi.c
> > static inline int hisi_pcie_cfg_read(struct pcie_port *pp, int where,
> > 		int size, u32 *val)
> > {
> > 	struct hisi_pcie *pcie = to_hisi_pcie(pp);
> >
> > 	return hisi_pcie_common_cfg_read(pcie->reg_base, where, size,
> val);
> > }
> >
> > static struct pcie_host_ops hisi_pcie_host_ops = {
> > 	.rd_own_conf = hisi_pcie_cfg_read,
> > 	.wr_own_conf = hisi_pcie_cfg_write,
> > 	.link_up = hisi_pcie_link_up,
> > };
> 
> I think this would be easier if you separate the ACPI code from the
> DT code and not try to have a common file used for both.
> 
> Sharing the config space accessors really isn't worth it when both
> variants are fairly simple to do, but they don't fit in a common
> model because one is called from the ACPI quirks and the other
> is called from the dw-pcie driver with completely different calling
> conventions.

Not sure about this...
From my perspective having the shared code would make clear that
the two drivers (ACPI and DT) are kind of related...

For example see the reply from Bjorn to the xgene driver:
https://lkml.org/lkml/2016/9/19/749

I know in our case the duplication isn't much but as I said
I am a bit reluctant to rework this... 

Thx

Gab

> 
> 	ARnd