[PATCH] PCI: designware: Make dw_pcie_rd_own_conf(), etc., static

[PATCH] PCI: designware: Make dw_pcie_rd_own_conf(), etc., static

[Bjorn Helgaas]

The following variables and functions are used only in pcie-designware.c,
so make them static:

  global_io_offset
  dw_pcie_rd_own_conf()
  dw_pcie_wr_own_conf()
  dw_pcie_setup()
  dw_pcie_scan_bus()
  dw_pcie_map_irq()

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/host/pcie-designware.c |   16 ++++++++--------
 drivers/pci/host/pcie-designware.h |    7 -------
 2 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index c10e9ac..900e875 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -64,7 +64,7 @@
 
 static struct hw_pci dw_pci;
 
-unsigned long global_io_offset;
+static unsigned long global_io_offset;
 
 static inline struct pcie_port *sys_to_pcie(struct pci_sys_data *sys)
 {
@@ -115,8 +115,8 @@ static inline void dw_pcie_writel_rc(struct pcie_port *pp, u32 val, u32 reg)
 		writel(val, pp->dbi_base + reg);
 }
 
-int dw_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
-				u32 *val)
+static int dw_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
+			       u32 *val)
 {
 	int ret;
 
@@ -128,8 +128,8 @@ int dw_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
 	return ret;
 }
 
-int dw_pcie_wr_own_conf(struct pcie_port *pp, int where, int size,
-				u32 val)
+static int dw_pcie_wr_own_conf(struct pcie_port *pp, int where, int size,
+			       u32 val)
 {
 	int ret;
 
@@ -438,7 +438,7 @@ static struct pci_ops dw_pcie_ops = {
 	.write = dw_pcie_wr_conf,
 };
 
-int dw_pcie_setup(int nr, struct pci_sys_data *sys)
+static int dw_pcie_setup(int nr, struct pci_sys_data *sys)
 {
 	struct pcie_port *pp;
 
@@ -461,7 +461,7 @@ int dw_pcie_setup(int nr, struct pci_sys_data *sys)
 	return 1;
 }
 
-struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys)
+static struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys)
 {
 	struct pci_bus *bus;
 	struct pcie_port *pp = sys_to_pcie(sys);
@@ -478,7 +478,7 @@ struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys)
 	return bus;
 }
 
-int dw_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
+static int dw_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
 {
 	struct pcie_port *pp = sys_to_pcie(dev->bus->sysdata);
 
diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
index 133820f..401b154 100644
--- a/drivers/pci/host/pcie-designware.h
+++ b/drivers/pci/host/pcie-designware.h
@@ -51,15 +51,8 @@ struct pcie_host_ops {
 	void (*host_init)(struct pcie_port *pp);
 };
 
-extern unsigned long global_io_offset;
-
 int cfg_read(void __iomem *addr, int where, int size, u32 *val);
 int cfg_write(void __iomem *addr, int where, int size, u32 val);
-int dw_pcie_wr_own_conf(struct pcie_port *pp, int where, int size, u32 val);
-int dw_pcie_rd_own_conf(struct pcie_port *pp, int where, int size, u32 *val);
 int dw_pcie_link_up(struct pcie_port *pp);
 void dw_pcie_setup_rc(struct pcie_port *pp);
 int dw_pcie_host_init(struct pcie_port *pp);
-int dw_pcie_setup(int nr, struct pci_sys_data *sys);
-struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys);
-int dw_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin);

Re: [PATCH] PCI: designware: Make dw_pcie_rd_own_conf(), etc., static

[Bjorn Helgaas]

On Mon, Oct 7, 2013 at 6:04 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> The following variables and functions are used only in pcie-designware.c,
> so make them static:
...
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index c10e9ac..900e875 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -64,7 +64,7 @@
>
>  static struct hw_pci dw_pci;
>
> -unsigned long global_io_offset;
> +static unsigned long global_io_offset;

While you're looking at this, I think "cfg_read()" and "cfg_write()"
are too generic to be global symbols.  I don't know if it would make
sense to rename them "dw_cfg_read()", pass around pointers in a
structure or what.

Bjorn

Re: [PATCH] PCI: designware: Make dw_pcie_rd_own_conf(), etc., static

[Pratyush Anand]

On Tue, Oct 08, 2013 at 08:07:24AM +0800, Bjorn Helgaas wrote:
> On Mon, Oct 7, 2013 at 6:04 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > The following variables and functions are used only in pcie-designware.c,
> > so make them static:
> ...
> > diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> > index c10e9ac..900e875 100644
> > --- a/drivers/pci/host/pcie-designware.c
> > +++ b/drivers/pci/host/pcie-designware.c
> > @@ -64,7 +64,7 @@
> >
> >  static struct hw_pci dw_pci;
> >
> > -unsigned long global_io_offset;
> > +static unsigned long global_io_offset;
> 
> While you're looking at this, I think "cfg_read()" and "cfg_write()"
> are too generic to be global symbols.  I don't know if it would make
> sense to rename them "dw_cfg_read()", pass around pointers in a
> structure or what.

In fact cfg_read, cfg_write & sys_to_pcie should go to common library,
as you will find similar function in other files too.

Regards
Pratyush

> 
> Bjorn
> --
> 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

Re: [PATCH] PCI: designware: Make dw_pcie_rd_own_conf(), etc., static

[Jingoo Han]

On Tuesday, October 08, 2013 3:07 PM, Pratyush Anand wrote:
> On Tue, Oct 08, 2013 at 08:07:24AM +0800, Bjorn Helgaas wrote:
> > On Mon, Oct 7, 2013 at 6:04 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > > The following variables and functions are used only in pcie-designware.c,
> > > so make them static:
> > ...
> > > diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> > > index c10e9ac..900e875 100644
> > > --- a/drivers/pci/host/pcie-designware.c
> > > +++ b/drivers/pci/host/pcie-designware.c
> > > @@ -64,7 +64,7 @@
> > >
> > >  static struct hw_pci dw_pci;
> > >
> > > -unsigned long global_io_offset;
> > > +static unsigned long global_io_offset;
> >
> > While you're looking at this, I think "cfg_read()" and "cfg_write()"
> > are too generic to be global symbols.  I don't know if it would make
> > sense to rename them "dw_cfg_read()", pass around pointers in a
> > structure or what.
> 
> In fact cfg_read, cfg_write & sys_to_pcie should go to common library,
> as you will find similar function in other files too.
> 

(+CC each PCIe host controller maintainer)

Yes, you're right. I think so, too. :-)

cfg_read, cfg_write & sys_to_pcie can go to common library.
I looked at pcie-designware.c, pci-tegra.c, & pci-mvebu.c.
These drivers can use common cfg_read(), cfg_write(), and sys_to_pcie().

For example, common cfg_read() can be used as below:
If there is no objection, I will send the patch.
However, I cannot decide which file is a good place for the common
cfg_read(), cfg_write(), and sys_to_pcie().

int cfg_read(void __iomem *addr, int where, int size, u32 *val)
{
        *val = readl(addr);

        if (size == 1)
                *val = (*val >> (8 * (where & 3))) & 0xff;
        else if (size == 2)
                *val = (*val >> (8 * (where & 3))) & 0xffff;
        else if (size != 4)
                return PCIBIOS_BAD_REGISTER_NUMBER;

        return PCIBIOS_SUCCESSFUL;
}

./drivers/pci/host/pci-mvebu.c
@@ -243,14 +243,7 @@ static int mvebu_pcie_hw_rd_conf(struct mvebu_pcie_port *port,
        writel(PCIE_CONF_ADDR(bus->number, devfn, where),
               port->base + PCIE_CONF_ADDR_OFF);

-       *val = readl(port->base + PCIE_CONF_DATA_OFF);
-
-       if (size == 1)
-               *val = (*val >> (8 * (where & 3))) & 0xff;
-       else if (size == 2)
-               *val = (*val >> (8 * (where & 3))) & 0xffff;
-
-       return PCIBIOS_SUCCESSFUL;
+       return cfg_read(port->base + PCIE_CONF_DATA_OFF, where, size, val);
 }


./drivers/pci/host/pci-tegra.c
@@ -462,14 +462,7 @@ static int tegra_pcie_read_conf(struct pci_bus *bus, unsigned int devfn,
                return PCIBIOS_DEVICE_NOT_FOUND;
        }

-       *value = readl(addr);
-
-       if (size == 1)
-               *value = (*value >> (8 * (where & 3))) & 0xff;
-       else if (size == 2)
-               *value = (*value >> (8 * (where & 3))) & 0xffff;
-
-       return PCIBIOS_SUCCESSFUL;
+       return cfg_read(addr, where, size, val);
 }

Best regards,
Jingoo Han

Re: [PATCH] PCI: designware: Make dw_pcie_rd_own_conf(), etc., static

[Thomas Petazzoni]

Dear Jingoo Han,

On Wed, 09 Oct 2013 10:33:30 +0900, Jingoo Han wrote:

> ./drivers/pci/host/pci-mvebu.c
> @@ -243,14 +243,7 @@ static int mvebu_pcie_hw_rd_conf(struct mvebu_pcie_port *port,
>         writel(PCIE_CONF_ADDR(bus->number, devfn, where),
>                port->base + PCIE_CONF_ADDR_OFF);
> 
> -       *val = readl(port->base + PCIE_CONF_DATA_OFF);
> -
> -       if (size == 1)
> -               *val = (*val >> (8 * (where & 3))) & 0xff;
> -       else if (size == 2)
> -               *val = (*val >> (8 * (where & 3))) & 0xffff;
> -
> -       return PCIBIOS_SUCCESSFUL;
> +       return cfg_read(port->base + PCIE_CONF_DATA_OFF, where, size, val);
>  }

The cfg_read() name looks too generic to me to be globally exported in
the kernel, but provided a more specific name is used, I'm fine with
using that in the PCIe mvebu driver.

Thanks!

Thomas
-- 
Thomas Petazzoni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH] PCI: designware: Make dw_pcie_rd_own_conf(), etc., static

[Jingoo Han]

On Tuesday, October 08, 2013 9:05 AM, Bjorn Helgaas wrote:
> 
> The following variables and functions are used only in pcie-designware.c,
> so make them static:
> 
>   global_io_offset
>   dw_pcie_rd_own_conf()
>   dw_pcie_wr_own_conf()
>   dw_pcie_setup()
>   dw_pcie_scan_bus()
>   dw_pcie_map_irq()
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Acked-by: Jingoo Han <jg1.han@samsung.com>

It looks good.
Also I tested this patch on Exynos5440. It works properly.
Thank you for sending the patch.

Best regards,
Jingoo Han

> ---
>  drivers/pci/host/pcie-designware.c |   16 ++++++++--------
>  drivers/pci/host/pcie-designware.h |    7 -------
>  2 files changed, 8 insertions(+), 15 deletions(-)

Re: [PATCH] PCI: designware: Make dw_pcie_rd_own_conf(), etc., static

[Bjorn Helgaas]

On Wed, Oct 9, 2013 at 1:50 AM, Jingoo Han <jg1.han@samsung.com> wrote:
> On Tuesday, October 08, 2013 9:05 AM, Bjorn Helgaas wrote:
>>
>> The following variables and functions are used only in pcie-designware.c,
>> so make them static:
>>
>>   global_io_offset
>>   dw_pcie_rd_own_conf()
>>   dw_pcie_wr_own_conf()
>>   dw_pcie_setup()
>>   dw_pcie_scan_bus()
>>   dw_pcie_map_irq()
>>
>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>
> Acked-by: Jingoo Han <jg1.han@samsung.com>
>
> It looks good.
> Also I tested this patch on Exynos5440. It works properly.
> Thank you for sending the patch.

I applied this to my pci/host-designware branch for v3.13.  Thanks!

>> ---
>>  drivers/pci/host/pcie-designware.c |   16 ++++++++--------
>>  drivers/pci/host/pcie-designware.h |    7 -------
>>  2 files changed, 8 insertions(+), 15 deletions(-)
>

Re: [PATCH] PCI: designware: Make dw_pcie_rd_own_conf(), etc., static

[Bjorn Helgaas]

On Wed, Oct 9, 2013 at 9:00 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Wed, Oct 9, 2013 at 1:50 AM, Jingoo Han <jg1.han@samsung.com> wrote:
>> On Tuesday, October 08, 2013 9:05 AM, Bjorn Helgaas wrote:
>>>
>>> The following variables and functions are used only in pcie-designware.c,
>>> so make them static:
>>>
>>>   global_io_offset
>>>   dw_pcie_rd_own_conf()
>>>   dw_pcie_wr_own_conf()
>>>   dw_pcie_setup()
>>>   dw_pcie_scan_bus()
>>>   dw_pcie_map_irq()
>>>
>>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>
>> Acked-by: Jingoo Han <jg1.han@samsung.com>
>>
>> It looks good.
>> Also I tested this patch on Exynos5440. It works properly.
>> Thank you for sending the patch.
>
> I applied this to my pci/host-designware branch for v3.13.  Thanks!

Correction: I moved this to pci/host-exynos because it's tangled up
with the MSI support there.