diff mbox

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

Message ID 1472644094-82731-2-git-send-email-liudongdong3@huawei.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Dongdong Liu Aug. 31, 2016, 11:48 a.m. UTC 
re-architect the Hip05/Hip06 host controllers driver to prepare
for the ACPI based driver.
The common functions used also by the ACPI driver have been grouped
into a new "common" file.

Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
---
 MAINTAINERS                         |   2 +
 drivers/pci/host/Makefile           |   2 +-
 drivers/pci/host/pcie-hisi-common.c |  66 ++++++++++++++++++++++
 drivers/pci/host/pcie-hisi.c        | 110 ++++++++++--------------------------
 drivers/pci/host/pcie-hisi.h        |  23 ++++++++
 5 files changed, 123 insertions(+), 80 deletions(-)
 create mode 100644 drivers/pci/host/pcie-hisi-common.c
 create mode 100644 drivers/pci/host/pcie-hisi.h

Comments

Arnd Bergmann Aug. 31, 2016, 11:45 a.m. UTC | #1 
On Wednesday, August 31, 2016 7:48:12 PM CEST Dongdong Liu wrote:
> +
> +/* HipXX PCIe host only supports 32-bit config access */
> +int hisi_pcie_common_cfg_read(void __iomem *reg_base, int where, int size,
> +                             u32 *val)
> +{
> +       u32 reg;
> +       u32 reg_val;
> +       void *walker = &reg_val;
> +
> +       walker += (where & 0x3);
> +       reg = where & ~0x3;
> +       reg_val = readl(reg_base + reg);
> +
> +       if (size == 1)
> +               *val = *(u8 __force *) walker;
> +       else if (size == 2)
> +               *val = *(u16 __force *) walker;

What is the __force for?

> +       else if (size == 4)
> +               *val = reg_val;
> +       else
> +               return PCIBIOS_BAD_REGISTER_NUMBER;
> +
> +       return PCIBIOS_SUCCESSFUL;

It looks like you are reimplementing pci_generic_config_read32/pci_generic_config_write32
read here, better use them directly.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dongdong Liu Sept. 1, 2016, 2:05 a.m. UTC | #2 
在 2016/8/31 19:45, Arnd Bergmann 写道:
> On Wednesday, August 31, 2016 7:48:12 PM CEST Dongdong Liu wrote:
>> +
>> +/* HipXX PCIe host only supports 32-bit config access */
>> +int hisi_pcie_common_cfg_read(void __iomem *reg_base, int where, int size,
>> +                             u32 *val)
>> +{
>> +       u32 reg;
>> +       u32 reg_val;
>> +       void *walker = &reg_val;
>> +
>> +       walker += (where & 0x3);
>> +       reg = where & ~0x3;
>> +       reg_val = readl(reg_base + reg);
>> +
>> +       if (size == 1)
>> +               *val = *(u8 __force *) walker;
>> +       else if (size == 2)
>> +               *val = *(u16 __force *) walker;
>
> What is the __force for?

Hi Arnd, thanks for replying.

__force is used to, well, force a conversion, like casting from or to a bitwise type, else the Sparse checker will throw a warning.

>
>> +       else if (size == 4)
>> +               *val = reg_val;
>> +       else
>> +               return PCIBIOS_BAD_REGISTER_NUMBER;
>> +
>> +       return PCIBIOS_SUCCESSFUL;
>
> It looks like you are reimplementing pci_generic_config_read32/pci_generic_config_write32
> read here, better use them directly.
>

For our host bridge, access RC and EP config space are not the same way.
Our host bridge is non ECAM only for the RC bus config space;
for any other bus underneath the root bus we support ECAM access.

hisi_pcie_common_cfg_read is used to read RC config space, only supports 32-bit config access.
hisi_pcie_common_cfg_read/hisi_pcie_common_cfg_write may change as below will be better.

/* HipXX PCIe host only supports 32-bit config access */
int hisi_pcie_common_cfg_read(void __iomem *reg_base, int where, int size,
			      u32 *val)
{
	void __iomem *addr;

	addr = reg_base + (where & ~0x3);
	*val = readl(addr);

	if (size <= 2)
		*val = (*val >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);

	return PCIBIOS_SUCCESSFUL;
}

/* HipXX PCIe host only supports 32-bit config access */
int hisi_pcie_common_cfg_write(void __iomem *reg_base, int where, int  size,
				u32 val)
{
	void __iomem *addr;
	u32 mask, tmp;

	addr = reg_base + (where & ~0x3);
	if (size == 4) {
		writel(val, addr);
		return PCIBIOS_SUCCESSFUL;
	} else {
		mask = ~(((1 << (size * 8)) - 1) << ((where & 0x3) * 8));
	}

	tmp = readl(addr) & mask;
	tmp |= val << ((where & 0x3) * 8);
	writel(tmp, addr);

	return PCIBIOS_SUCCESSFUL;
}


Thanks
Dongdong

> 	Arnd
>
> .
>

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Sept. 1, 2016, 7:41 a.m. UTC | #3 
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:
> >> +
> >> +/* HipXX PCIe host only supports 32-bit config access */
> >> +int hisi_pcie_common_cfg_read(void __iomem *reg_base, int where, int size,
> >> +                             u32 *val)
> >> +{
> >> +       u32 reg;
> >> +       u32 reg_val;
> >> +       void *walker = &reg_val;
> >> +
> >> +       walker += (where & 0x3);
> >> +       reg = where & ~0x3;
> >> +       reg_val = readl(reg_base + reg);
> >> +
> >> +       if (size == 1)
> >> +               *val = *(u8 __force *) walker;
> >> +       else if (size == 2)
> >> +               *val = *(u16 __force *) walker;
> >
> > What is the __force for?
> 
> Hi Arnd, thanks for replying.
> 
> __force is used to, well, force a conversion, like casting from or to a bitwise type, else the Sparse checker will throw a warning.

I know what it's for in general, but in this case there is no __bitwise
or __iomem or any other annotation on either side of the assignment.

> >
> >> +       else if (size == 4)
> >> +               *val = reg_val;
> >> +       else
> >> +               return PCIBIOS_BAD_REGISTER_NUMBER;
> >> +
> >> +       return PCIBIOS_SUCCESSFUL;
> >
> > It looks like you are reimplementing pci_generic_config_read32/pci_generic_config_write32
> > read here, better use them directly.
> >
> 
> For our host bridge, access RC and EP config space are not the same way.
> Our host bridge is non ECAM only for the RC bus config space;
> for any other bus underneath the root bus we support ECAM access.
> 
> hisi_pcie_common_cfg_read is used to read RC config space, only supports 32-bit config access.
> hisi_pcie_common_cfg_read/hisi_pcie_common_cfg_write may change as below will be better.
> 
> /* HipXX PCIe host only supports 32-bit config access */
> int hisi_pcie_common_cfg_read(void __iomem *reg_base, int where, int size,
> 			      u32 *val)
> {
> 	void __iomem *addr;
> 
> 	addr = reg_base + (where & ~0x3);
> 	*val = readl(addr);
> 
> 	if (size <= 2)
> 		*val = (*val >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
> 
> 	return PCIBIOS_SUCCESSFUL;
> }

My point was: why not call pci_generic_config_read32() when accessing
the RC config space instead of duplicating the code from it?

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dongdong Liu Sept. 1, 2016, 12:44 p.m. UTC | #4 
Hi Arnd

在 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:
>>>> +
>>>> +/* HipXX PCIe host only supports 32-bit config access */
>>>> +int hisi_pcie_common_cfg_read(void __iomem *reg_base, int where, int size,
>>>> +                             u32 *val)
>>>> +{
>>>> +       u32 reg;
>>>> +       u32 reg_val;
>>>> +       void *walker = &reg_val;
>>>> +
>>>> +       walker += (where & 0x3);
>>>> +       reg = where & ~0x3;
>>>> +       reg_val = readl(reg_base + reg);
>>>> +
>>>> +       if (size == 1)
>>>> +               *val = *(u8 __force *) walker;
>>>> +       else if (size == 2)
>>>> +               *val = *(u16 __force *) walker;
>>>
>>> What is the __force for?
>>
>> Hi Arnd, thanks for replying.
>>
>> __force is used to, well, force a conversion, like casting from or to a bitwise type, else the Sparse checker will throw a warning.
>
> I know what it's for in general, but in this case there is no __bitwise
> or __iomem or any other annotation on either side of the assignment.

Thanks for you point that.

>
>>>
>>>> +       else if (size == 4)
>>>> +               *val = reg_val;
>>>> +       else
>>>> +               return PCIBIOS_BAD_REGISTER_NUMBER;
>>>> +
>>>> +       return PCIBIOS_SUCCESSFUL;
>>>
>>> It looks like you are reimplementing pci_generic_config_read32/pci_generic_config_write32
>>> read here, better use them directly.
>>>
>>
>> For our host bridge, access RC and EP config space are not the same way.
>> Our host bridge is non ECAM only for the RC bus config space;
>> for any other bus underneath the root bus we support ECAM access.
>>
>> hisi_pcie_common_cfg_read is used to read RC config space, only supports 32-bit config access.
>> hisi_pcie_common_cfg_read/hisi_pcie_common_cfg_write may change as below will be better.
>>
>> /* HipXX PCIe host only supports 32-bit config access */
>> int hisi_pcie_common_cfg_read(void __iomem *reg_base, int where, int size,
>> 			      u32 *val)
>> {
>> 	void __iomem *addr;
>>
>> 	addr = reg_base + (where & ~0x3);
>> 	*val = readl(addr);
>>
>> 	if (size <= 2)
>> 		*val = (*val >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
>>
>> 	return PCIBIOS_SUCCESSFUL;
>> }
>
> My point was: why not call pci_generic_config_read32() when accessing
> the RC config space instead of duplicating the code from it?

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);
}


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,
};

Thanks
Dongdong
>
> 	Arnd
>
> .
>

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Sept. 1, 2016, 2:02 p.m. UTC | #5 
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.

	ARnd
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dongdong Liu Sept. 2, 2016, 2:02 a.m. UTC | #6 
Hi Arnd

在 2016/9/1 22:02, Arnd Bergmann 写道:
>
>> 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.

I agree, many thanks.

Thanks
Dongdong
>
> 	ARnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> .
>

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Gabriele Paoloni Sept. 20, 2016, 9:45 a.m. UTC | #7 
Hi Arnd, thanks for your comments

> -----Original Message-----

> From: Arnd Bergmann [mailto:arnd@arndb.de]

> Sent: 01 September 2016 15:02

> To: liudongdong (C)

> Cc: helgaas@kernel.org; rafael@kernel.org; Lorenzo.Pieralisi@arm.com;

> tn@semihalf.com; Wangzhou (B); pratyush.anand@gmail.com; linux-

> pci@vger.kernel.org; linux-acpi@vger.kernel.org; linux-

> kernel@vger.kernel.org; jcm@redhat.com; Gabriele Paoloni; Chenxin

> (Charles); hanjun.guo@linaro.org; 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
Arnd Bergmann Sept. 20, 2016, 1:22 p.m. UTC | #8 
On Tuesday, September 20, 2016 9:45:13 AM CEST Gabriele Paoloni wrote:
> > 
> > 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... 

It's clearly a question of perspective: The way I see it, this
is not a driver at all, but a quirk for the acpi-pci driver,
and having hardware specific code shared between the two
complicates things.

In both cases (xgene and hisi), only a very small portion of
PCI host driver is needed as for the ACPI quirk, while all
of the hardware specific setup can be left to the firmware
here.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 20bb1d0..cdc1bba 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9081,7 +9081,9 @@  M:	Gabriele Paoloni <gabriele.paoloni@huawei.com>
 L:	linux-pci@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/pci/hisilicon-pcie.txt
+F:	drivers/pci/host/pcie-hisi.h
 F:	drivers/pci/host/pcie-hisi.c
+F:	drivers/pci/host/pcie-hisi-common.c
 
 PCIE DRIVER FOR QUALCOMM MSM
 M:     Stanimir Varbanov <svarbanov@mm-sol.com>
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 500cf78..02b498d 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -25,7 +25,7 @@  obj-$(CONFIG_PCIE_IPROC_PLATFORM) += pcie-iproc-platform.o
 obj-$(CONFIG_PCIE_IPROC_BCMA) += pcie-iproc-bcma.o
 obj-$(CONFIG_PCIE_ALTERA) += pcie-altera.o
 obj-$(CONFIG_PCIE_ALTERA_MSI) += pcie-altera-msi.o
-obj-$(CONFIG_PCI_HISI) += pcie-hisi.o
+obj-$(CONFIG_PCI_HISI) += pcie-hisi.o pcie-hisi-common.o
 obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
 obj-$(CONFIG_PCI_HOST_THUNDER_ECAM) += pci-thunder-ecam.o
 obj-$(CONFIG_PCI_HOST_THUNDER_PEM) += pci-thunder-pem.o
diff --git a/drivers/pci/host/pcie-hisi-common.c b/drivers/pci/host/pcie-hisi-common.c
new file mode 100644
index 0000000..5a5f269
--- /dev/null
+++ b/drivers/pci/host/pcie-hisi-common.c
@@ -0,0 +1,66 @@ 
+/*
+ * PCIe host controller common functions for HiSilicon SoCs
+ *
+ * Copyright (C) 2015 HiSilicon Co., Ltd. http://www.hisilicon.com
+ *
+ * Author: Zhou Wang <wangzhou1@hisilicon.com>
+ *         Dacai Zhu <zhudacai@hisilicon.com>
+ *         Gabriele Paoloni <gabriele.paoloni@huawei.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/pci.h>
+#include "pcie-hisi.h"
+
+/* HipXX PCIe host only supports 32-bit config access */
+int hisi_pcie_common_cfg_read(void __iomem *reg_base, int where, int size,
+			      u32 *val)
+{
+	u32 reg;
+	u32 reg_val;
+	void *walker = &reg_val;
+
+	walker += (where & 0x3);
+	reg = where & ~0x3;
+	reg_val = readl(reg_base + reg);
+
+	if (size == 1)
+		*val = *(u8 __force *) walker;
+	else if (size == 2)
+		*val = *(u16 __force *) walker;
+	else if (size == 4)
+		*val = reg_val;
+	else
+		return PCIBIOS_BAD_REGISTER_NUMBER;
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
+/* HipXX PCIe host only supports 32-bit config access */
+int hisi_pcie_common_cfg_write(void __iomem *reg_base, int where, int  size,
+				u32 val)
+{
+	u32 reg_val;
+	u32 reg;
+	void *walker = &reg_val;
+
+	walker += (where & 0x3);
+	reg = where & ~0x3;
+	if (size == 4)
+		writel(val, reg_base + reg);
+	else if (size == 2) {
+		reg_val = readl(reg_base + reg);
+		*(u16 __force *) walker = val;
+		writel(reg_val, reg_base + reg);
+	} else if (size == 1) {
+		reg_val = readl(reg_base + reg);
+		*(u8 __force *) walker = val;
+		writel(reg_val, reg_base + reg);
+	} else
+		return PCIBIOS_BAD_REGISTER_NUMBER;
+
+	return PCIBIOS_SUCCESSFUL;
+}
diff --git a/drivers/pci/host/pcie-hisi.c b/drivers/pci/host/pcie-hisi.c
index 7ee9dfc..23d74e9 100644
--- a/drivers/pci/host/pcie-hisi.c
+++ b/drivers/pci/host/pcie-hisi.c
@@ -21,6 +21,7 @@ 
 #include <linux/regmap.h>
 
 #include "pcie-designware.h"
+#include "pcie-hisi.h"
 
 #define PCIE_LTSSM_LINKUP_STATE				0x11
 #define PCIE_LTSSM_STATE_MASK				0x3F
@@ -30,12 +31,6 @@ 
 
 #define to_hisi_pcie(x)	container_of(x, struct hisi_pcie, pp)
 
-struct hisi_pcie;
-
-struct pcie_soc_ops {
-	int (*hisi_pcie_link_up)(struct hisi_pcie *pcie);
-};
-
 struct hisi_pcie {
 	struct regmap *subctrl;
 	void __iomem *reg_base;
@@ -44,87 +39,24 @@  struct hisi_pcie {
 	struct pcie_soc_ops *soc_ops;
 };
 
-static inline void hisi_pcie_apb_writel(struct hisi_pcie *pcie,
-					u32 val, u32 reg)
-{
-	writel(val, pcie->reg_base + reg);
-}
-
-static inline u32 hisi_pcie_apb_readl(struct hisi_pcie *pcie, u32 reg)
-{
-	return readl(pcie->reg_base + reg);
-}
+struct pcie_soc_ops {
+	int (*hisi_pcie_link_up)(struct hisi_pcie *pcie);
+};
 
-/* HipXX PCIe host only supports 32-bit config access */
-static int hisi_pcie_cfg_read(struct pcie_port *pp, int where, int size,
-			      u32 *val)
+static inline int hisi_pcie_cfg_read(struct pcie_port *pp, int where,
+		int size, u32 *val)
 {
-	u32 reg;
-	u32 reg_val;
 	struct hisi_pcie *pcie = to_hisi_pcie(pp);
-	void *walker = &reg_val;
-
-	walker += (where & 0x3);
-	reg = where & ~0x3;
-	reg_val = hisi_pcie_apb_readl(pcie, reg);
-
-	if (size == 1)
-		*val = *(u8 __force *) walker;
-	else if (size == 2)
-		*val = *(u16 __force *) walker;
-	else if (size == 4)
-		*val = reg_val;
-	else
-		return PCIBIOS_BAD_REGISTER_NUMBER;
-
-	return PCIBIOS_SUCCESSFUL;
-}
 
-/* HipXX PCIe host only supports 32-bit config access */
-static int hisi_pcie_cfg_write(struct pcie_port *pp, int where, int  size,
-				u32 val)
-{
-	u32 reg_val;
-	u32 reg;
-	struct hisi_pcie *pcie = to_hisi_pcie(pp);
-	void *walker = &reg_val;
-
-	walker += (where & 0x3);
-	reg = where & ~0x3;
-	if (size == 4)
-		hisi_pcie_apb_writel(pcie, val, reg);
-	else if (size == 2) {
-		reg_val = hisi_pcie_apb_readl(pcie, reg);
-		*(u16 __force *) walker = val;
-		hisi_pcie_apb_writel(pcie, reg_val, reg);
-	} else if (size == 1) {
-		reg_val = hisi_pcie_apb_readl(pcie, reg);
-		*(u8 __force *) walker = val;
-		hisi_pcie_apb_writel(pcie, reg_val, reg);
-	} else
-		return PCIBIOS_BAD_REGISTER_NUMBER;
-
-	return PCIBIOS_SUCCESSFUL;
+	return hisi_pcie_common_cfg_read(pcie->reg_base, where, size, val);
 }
 
-static int hisi_pcie_link_up_hip05(struct hisi_pcie *hisi_pcie)
+static inline int hisi_pcie_cfg_write(struct pcie_port *pp, int where,
+		int size, u32 val)
 {
-	u32 val;
-
-	regmap_read(hisi_pcie->subctrl, PCIE_SUBCTRL_SYS_STATE4_REG +
-		    0x100 * hisi_pcie->port_id, &val);
-
-	return ((val & PCIE_LTSSM_STATE_MASK) == PCIE_LTSSM_LINKUP_STATE);
-}
-
-static int hisi_pcie_link_up_hip06(struct hisi_pcie *hisi_pcie)
-{
-	u32 val;
-
-	val = hisi_pcie_apb_readl(hisi_pcie, PCIE_HIP06_CTRL_OFF +
-			PCIE_SYS_STATE4);
+	struct hisi_pcie *pcie = to_hisi_pcie(pp);
 
-	return ((val & PCIE_LTSSM_STATE_MASK) == PCIE_LTSSM_LINKUP_STATE);
+	return hisi_pcie_common_cfg_write(pcie->reg_base, where, size, val);
 }
 
 static int hisi_pcie_link_up(struct pcie_port *pp)
@@ -215,6 +147,26 @@  static int hisi_pcie_probe(struct platform_device *pdev)
 	return 0;
 }
 
+static int hisi_pcie_link_up_hip05(struct hisi_pcie *hisi_pcie)
+{
+	u32 val;
+
+	regmap_read(hisi_pcie->subctrl, PCIE_SUBCTRL_SYS_STATE4_REG +
+		    0x100 * hisi_pcie->port_id, &val);
+
+	return ((val & PCIE_LTSSM_STATE_MASK) == PCIE_LTSSM_LINKUP_STATE);
+}
+
+static int hisi_pcie_link_up_hip06(struct hisi_pcie *hisi_pcie)
+{
+	u32 val;
+
+	val = readl(hisi_pcie->reg_base + PCIE_HIP06_CTRL_OFF +
+			PCIE_SYS_STATE4);
+
+	return ((val & PCIE_LTSSM_STATE_MASK) == PCIE_LTSSM_LINKUP_STATE);
+}
+
 static struct pcie_soc_ops hip05_ops = {
 		&hisi_pcie_link_up_hip05
 };
diff --git a/drivers/pci/host/pcie-hisi.h b/drivers/pci/host/pcie-hisi.h
new file mode 100644
index 0000000..44fc680
--- /dev/null
+++ b/drivers/pci/host/pcie-hisi.h
@@ -0,0 +1,23 @@ 
+/*
+ * PCIe host controller driver for HiSilicon SoCs
+ *
+ * Copyright (C) 2015 HiSilicon Co., Ltd. http://www.hisilicon.com
+ *
+ * Author: Zhou Wang <wangzhou1@hisilicon.com>
+ *         Dacai Zhu <zhudacai@hisilicon.com>
+ *         Gabriele Paoloni <gabriele.paoloni@huawei.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#ifndef PCIE_HISI_H_
+#define PCIE_HISI_H_
+
+
+int hisi_pcie_common_cfg_read(void __iomem *reg_base, int where, int size,
+			      u32 *val);
+int hisi_pcie_common_cfg_write(void __iomem *reg_base, int where, int size,
+			       u32 val);
+
+#endif /* PCIE_HISI_H_ */