Patchwork [RFC,1/2] PCI: designware: Add ARM64 PCI support

login
register
mail settings
Submitter Jingoo Han
Date April 16, 2014, 4:42 a.m.
Message ID <000901cf592e$563bc8c0$02b35a40$%han@samsung.com>
Download mbox | patch
Permalink /patch/339427/
State Not Applicable
Headers show

Comments

Jingoo Han - April 16, 2014, 4:42 a.m.
Add ARM64 PCI support for Synopsys designware PCIe, by using
pcie arm64 arch support and creating generic pcie bridge from
device tree.

Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
 drivers/pci/host/pcie-designware.c |   32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)
Liviu Dudau - April 16, 2014, 4:57 p.m.
Jingoo,

Thanks for taking a stab at trying to convert a host bridge
driver to use the new generic host bridge code.

I do however have concerns on the direction you took. You have split
your driver in two, depending on whether it was CONFIG_ARM or CONFIG_ARM64,
even if (with my series) it should be no reason why the host bridge
driver should not work on other architectures as well once they are
converted.

Also, some of the functions that you use have identical names but different
signatures depending on what arch you have selected. This is really bad
in my books!

What about creating functions that use my series directly if CONFIG_ARM64 is
defined (or any CONFIG_ you want to create for your driver that you select
from CONFIG_ARM64) and otherwise implement the CONFIG_ARM version? That
way your driver will call only one API without any #ifdef and when arm code
gets converted you drop your adaptation functions. Or (better yet), have a
stab at converting bios32 (Rob Herring has already provided some hints on
how to do it for arch/arm).

To give an example on how things are not going well in your version (not obvious
from your patch, but you can see it once you apply it): dw_pcie_host_init()
will still carry the handcoded version of DT parsing and that is not guarded
against CONFIG_ARM64 being defined, where the parsing will happen again
when you call of_create_pci_host_bridge().

Speaking of the handcoded DT parsing of resources: you are using restype == 0
as a way of selecting config space, *and then split the range size into two
halves*. From what Jason Gunthorpe and Arnd were saying, config ranges in the DT
tree should only be used for ECAM space, so no split is allowed.

Arnd, are you allowing this non-standard use to creep in the bindings?

Best regards,
Liviu

On Wed, Apr 16, 2014 at 05:42:56AM +0100, Jingoo Han wrote:
> Add ARM64 PCI support for Synopsys designware PCIe, by using
> pcie arm64 arch support and creating generic pcie bridge from
> device tree.
> 
> Signed-off-by: Jingoo Han <jg1.han@samsung.com>
> ---
>  drivers/pci/host/pcie-designware.c |   32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index 6d23d8c..fac0440 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -65,14 +65,27 @@
>  #define PCIE_ATU_FUNC(x)		(((x) & 0x7) << 16)
>  #define PCIE_ATU_UPPER_TARGET		0x91C
>  
> +#ifdef CONFIG_ARM
>  static struct hw_pci dw_pci;
> +#endif
>  
>  static unsigned long global_io_offset;
>  
> +#ifdef CONFIG_ARM
>  static inline struct pcie_port *sys_to_pcie(struct pci_sys_data *sys)
>  {
>  	return sys->private_data;
>  }
> +#endif
> +
> +#ifdef CONFIG_ARM64
> +static inline struct pcie_port *sys_to_pcie(struct pcie_port *pp)
> +{
> +	return pp;
> +}
> +
> +static struct pci_ops dw_pcie_ops;
> +#endif
>  
>  int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val)
>  {
> @@ -381,7 +394,9 @@ static int dw_pcie_msi_map(struct irq_domain *domain, unsigned int irq,
>  {
>  	irq_set_chip_and_handler(irq, &dw_msi_irq_chip, handle_simple_irq);
>  	irq_set_chip_data(irq, domain->host_data);
> +#ifdef CONFIG_ARM
>  	set_irq_flags(irq, IRQF_VALID);
> +#endif
>  
>  	return 0;
>  }
> @@ -397,6 +412,10 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
>  	struct of_pci_range_parser parser;
>  	u32 val;
>  	int i;
> +#ifdef CONFIG_ARM64
> +	struct pci_host_bridge *bridge;
> +	resource_size_t lastbus;
> +#endif
>  
>  	if (of_pci_range_parser_init(&parser, np)) {
>  		dev_err(pp->dev, "missing ranges property\n");
> @@ -489,6 +508,7 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
>  	val |= PORT_LOGIC_SPEED_CHANGE;
>  	dw_pcie_wr_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, val);
>  
> +#ifdef CONFIG_ARM
>  	dw_pci.nr_controllers = 1;
>  	dw_pci.private_data = (void **)&pp;
>  
> @@ -497,6 +517,16 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
>  #ifdef CONFIG_PCI_DOMAINS
>  	dw_pci.domain++;
>  #endif
> +#endif
> +
> +#ifdef CONFIG_ARM64
> +	bridge = of_create_pci_host_bridge(pp->dev, &dw_pcie_ops, pp);
> +	if (IS_ERR_OR_NULL(bridge))
> +		return PTR_ERR(bridge);
> +
> +	lastbus = pci_rescan_bus(bridge->bus);
> +	pci_bus_update_busn_res_end(bridge->bus, lastbus);
> +#endif
>  
>  	return 0;
>  }
> @@ -695,6 +725,7 @@ static struct pci_ops dw_pcie_ops = {
>  	.write = dw_pcie_wr_conf,
>  };
>  
> +#ifdef CONFIG_ARM
>  static int dw_pcie_setup(int nr, struct pci_sys_data *sys)
>  {
>  	struct pcie_port *pp;
> @@ -758,6 +789,7 @@ static struct hw_pci dw_pci = {
>  	.map_irq	= dw_pcie_map_irq,
>  	.add_bus	= dw_pcie_add_bus,
>  };
> +#endif /* CONFIG_ARM */
>  
>  void dw_pcie_setup_rc(struct pcie_port *pp)
>  {
> -- 
> 1.7.10.4
> 
> 
>
Arnd Bergmann - April 16, 2014, 6:26 p.m.
On Wednesday 16 April 2014 17:57:24 Liviu Dudau wrote:
> Jingoo,
> 
> Thanks for taking a stab at trying to convert a host bridge
> driver to use the new generic host bridge code.
> 
> I do however have concerns on the direction you took. You have split
> your driver in two, depending on whether it was CONFIG_ARM or CONFIG_ARM64,
> even if (with my series) it should be no reason why the host bridge
> driver should not work on other architectures as well once they are
> converted.

Right.

> Also, some of the functions that you use have identical names but different
> signatures depending on what arch you have selected. This is really bad
> in my books!

It's only the sys_to_pcie() function, right?

You can probably simplify that to take a void pointer and have only one line
difference.

> What about creating functions that use my series directly if CONFIG_ARM64 is
> defined (or any CONFIG_ you want to create for your driver that you select
> from CONFIG_ARM64) and otherwise implement the CONFIG_ARM version? That
> way your driver will call only one API without any #ifdef and when arm code
> gets converted you drop your adaptation functions. Or (better yet), have a
> stab at converting bios32 (Rob Herring has already provided some hints on
> how to do it for arch/arm).

That would of course be best.

> To give an example on how things are not going well in your version (not obvious
> from your patch, but you can see it once you apply it): dw_pcie_host_init()
> will still carry the handcoded version of DT parsing and that is not guarded
> against CONFIG_ARM64 being defined, where the parsing will happen again
> when you call of_create_pci_host_bridge().

How about making the generic DT parsing code from of_create_pci_host_bridge()
an exported function that can be called by drivers that don't use
of_create_pci_host_bridge?

> Speaking of the handcoded DT parsing of resources: you are using restype == 0
> as a way of selecting config space, *and then split the range size into two
> halves*. From what Jason Gunthorpe and Arnd were saying, config ranges in the DT
> tree should only be used for ECAM space, so no split is allowed.
> 
> Arnd, are you allowing this non-standard use to creep in the bindings?

I fear it's too late to change that now. In retrospect we probably shoulnd't
have defined the binding like that.

Overall, my impression of the patch is that it should be possible to do
the same with much fewer #ifdefs by first rearranging the code in one patch
and then doing another patch on top to add the generic arm64 support.

> > diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> > index 6d23d8c..fac0440 100644
> > --- a/drivers/pci/host/pcie-designware.c
> > +++ b/drivers/pci/host/pcie-designware.c
> > @@ -65,14 +65,27 @@
> >  #define PCIE_ATU_FUNC(x)		(((x) & 0x7) << 16)
> >  #define PCIE_ATU_UPPER_TARGET		0x91C
> >  
> > +#ifdef CONFIG_ARM
> >  static struct hw_pci dw_pci;
> > +#endif
> >  
> >  static unsigned long global_io_offset;
> >  
> > +#ifdef CONFIG_ARM
> >  static inline struct pcie_port *sys_to_pcie(struct pci_sys_data *sys)
> >  {
> >  	return sys->private_data;
> >  }
> > +#endif
> > +
> > +#ifdef CONFIG_ARM64
> > +static inline struct pcie_port *sys_to_pcie(struct pcie_port *pp)
> > +{
> > +	return pp;
> > +}
> > +
> > +static struct pci_ops dw_pcie_ops;
> > +#endif
> >  
> >  int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val)
> >  {
> > @@ -381,7 +394,9 @@ static int dw_pcie_msi_map(struct irq_domain *domain, unsigned int irq,
> >  {
> >  	irq_set_chip_and_handler(irq, &dw_msi_irq_chip, handle_simple_irq);
> >  	irq_set_chip_data(irq, domain->host_data);
> > +#ifdef CONFIG_ARM
> >  	set_irq_flags(irq, IRQF_VALID);
> > +#endif
> >  
> >  	return 0;
> >  }
> > @@ -397,6 +412,10 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
> >  	struct of_pci_range_parser parser;
> >  	u32 val;
> >  	int i;
> > +#ifdef CONFIG_ARM64
> > +	struct pci_host_bridge *bridge;
> > +	resource_size_t lastbus;
> > +#endif
> >  
> >  	if (of_pci_range_parser_init(&parser, np)) {
> >  		dev_err(pp->dev, "missing ranges property\n");
> > @@ -489,6 +508,7 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
> >  	val |= PORT_LOGIC_SPEED_CHANGE;
> >  	dw_pcie_wr_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, val);
> >  
> > +#ifdef CONFIG_ARM
> >  	dw_pci.nr_controllers = 1;
> >  	dw_pci.private_data = (void **)&pp;
> >  
> > @@ -497,6 +517,16 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
> >  #ifdef CONFIG_PCI_DOMAINS
> >  	dw_pci.domain++;
> >  #endif
> > +#endif
> > +
> > +#ifdef CONFIG_ARM64
> > +	bridge = of_create_pci_host_bridge(pp->dev, &dw_pcie_ops, pp);
> > +	if (IS_ERR_OR_NULL(bridge))
> > +		return PTR_ERR(bridge);
> > +
> > +	lastbus = pci_rescan_bus(bridge->bus);
> > +	pci_bus_update_busn_res_end(bridge->bus, lastbus);
> > +#endif
> >  
> >  	return 0;
> >  }
> > @@ -695,6 +725,7 @@ static struct pci_ops dw_pcie_ops = {
> >  	.write = dw_pcie_wr_conf,
> >  };
> >  
> > +#ifdef CONFIG_ARM
> >  static int dw_pcie_setup(int nr, struct pci_sys_data *sys)
> >  {
> >  	struct pcie_port *pp;
> > @@ -758,6 +789,7 @@ static struct hw_pci dw_pci = {
> >  	.map_irq	= dw_pcie_map_irq,
> >  	.add_bus	= dw_pcie_add_bus,
> >  };
> > +#endif /* CONFIG_ARM */
> >  
> >  void dw_pcie_setup_rc(struct pcie_port *pp)



--
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
Jingoo Han - April 21, 2014, 1:54 a.m.
On Thursday, April 17, 2014 3:26 AM, Arnd Bergmann wrote:

(+cc Mohit KUMAR, Pratyush Anand, Marek Vasut, Richard Zhu,
      Kishon Vijay Abraham I, Byungho An)
> 
> On Wednesday 16 April 2014 17:57:24 Liviu Dudau wrote:
> > Jingoo,
> >
> > Thanks for taking a stab at trying to convert a host bridge
> > driver to use the new generic host bridge code.
> >
> > I do however have concerns on the direction you took. You have split
> > your driver in two, depending on whether it was CONFIG_ARM or CONFIG_ARM64,
> > even if (with my series) it should be no reason why the host bridge
> > driver should not work on other architectures as well once they are
> > converted.
> 
> Right.
> 
> > Also, some of the functions that you use have identical names but different
> > signatures depending on what arch you have selected. This is really bad
> > in my books!
> 
> It's only the sys_to_pcie() function, right?
> 
> You can probably simplify that to take a void pointer and have only one line
> difference.

Do you mean the following?
Would you give me more detailed advice?

static inline struct pcie_port *sys_to_pcie(void *sys)
{
	struct pcie_port *pp

#ifdef CONFIG_ARM
	pp = ((struct pci_sys_data *)sys)->private_data;
#else
	pp = (struct pcie_port *)sys;
#endif
	return pp;
}

> 
> > What about creating functions that use my series directly if CONFIG_ARM64 is
> > defined (or any CONFIG_ you want to create for your driver that you select
> > from CONFIG_ARM64) and otherwise implement the CONFIG_ARM version? That
> > way your driver will call only one API without any #ifdef and when arm code
> > gets converted you drop your adaptation functions. Or (better yet), have a
> > stab at converting bios32 (Rob Herring has already provided some hints on
> > how to do it for arch/arm).

To: Liviu Dudau

Sorry, but I will not implement this.
At first, you had to think the compatibility with ARM32 PCIe.
Why do you want other engineers to take this load?

> 
> That would of course be best.
> 
> > To give an example on how things are not going well in your version (not obvious
> > from your patch, but you can see it once you apply it): dw_pcie_host_init()
> > will still carry the handcoded version of DT parsing and that is not guarded
> > against CONFIG_ARM64 being defined, where the parsing will happen again
> > when you call of_create_pci_host_bridge().
> 
> How about making the generic DT parsing code from of_create_pci_host_bridge()
> an exported function that can be called by drivers that don't use
> of_create_pci_host_bridge?

Do you mean that of_create_pci_host_bridge() should be used for both ARM32
and ARM64 cases?

> 
> > Speaking of the handcoded DT parsing of resources: you are using restype == 0
> > as a way of selecting config space, *and then split the range size into two
> > halves*. From what Jason Gunthorpe and Arnd were saying, config ranges in the DT
> > tree should only be used for ECAM space, so no split is allowed.
> >
> > Arnd, are you allowing this non-standard use to creep in the bindings?
> 
> I fear it's too late to change that now. In retrospect we probably shoulnd't
> have defined the binding like that.
> 
> Overall, my impression of the patch is that it should be possible to do
> the same with much fewer #ifdefs by first rearranging the code in one patch
> and then doing another patch on top to add the generic arm64 support.

I will try to reduce #ifdefs as possible.

Best regards,
Jingoo Han

> 
> > > diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> > > index 6d23d8c..fac0440 100644
> > > --- a/drivers/pci/host/pcie-designware.c
> > > +++ b/drivers/pci/host/pcie-designware.c
> > > @@ -65,14 +65,27 @@
> > >  #define PCIE_ATU_FUNC(x)		(((x) & 0x7) << 16)
> > >  #define PCIE_ATU_UPPER_TARGET		0x91C
> > >
> > > +#ifdef CONFIG_ARM
> > >  static struct hw_pci dw_pci;
> > > +#endif
> > >
> > >  static unsigned long global_io_offset;
> > >
> > > +#ifdef CONFIG_ARM
> > >  static inline struct pcie_port *sys_to_pcie(struct pci_sys_data *sys)
> > >  {
> > >  	return sys->private_data;
> > >  }
> > > +#endif
> > > +
> > > +#ifdef CONFIG_ARM64
> > > +static inline struct pcie_port *sys_to_pcie(struct pcie_port *pp)
> > > +{
> > > +	return pp;
> > > +}
> > > +
> > > +static struct pci_ops dw_pcie_ops;
> > > +#endif
> > >
> > >  int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val)
> > >  {
> > > @@ -381,7 +394,9 @@ static int dw_pcie_msi_map(struct irq_domain *domain, unsigned int irq,
> > >  {
> > >  	irq_set_chip_and_handler(irq, &dw_msi_irq_chip, handle_simple_irq);
> > >  	irq_set_chip_data(irq, domain->host_data);
> > > +#ifdef CONFIG_ARM
> > >  	set_irq_flags(irq, IRQF_VALID);
> > > +#endif
> > >
> > >  	return 0;
> > >  }
> > > @@ -397,6 +412,10 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
> > >  	struct of_pci_range_parser parser;
> > >  	u32 val;
> > >  	int i;
> > > +#ifdef CONFIG_ARM64
> > > +	struct pci_host_bridge *bridge;
> > > +	resource_size_t lastbus;
> > > +#endif
> > >
> > >  	if (of_pci_range_parser_init(&parser, np)) {
> > >  		dev_err(pp->dev, "missing ranges property\n");
> > > @@ -489,6 +508,7 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
> > >  	val |= PORT_LOGIC_SPEED_CHANGE;
> > >  	dw_pcie_wr_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, val);
> > >
> > > +#ifdef CONFIG_ARM
> > >  	dw_pci.nr_controllers = 1;
> > >  	dw_pci.private_data = (void **)&pp;
> > >
> > > @@ -497,6 +517,16 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
> > >  #ifdef CONFIG_PCI_DOMAINS
> > >  	dw_pci.domain++;
> > >  #endif
> > > +#endif
> > > +
> > > +#ifdef CONFIG_ARM64
> > > +	bridge = of_create_pci_host_bridge(pp->dev, &dw_pcie_ops, pp);
> > > +	if (IS_ERR_OR_NULL(bridge))
> > > +		return PTR_ERR(bridge);
> > > +
> > > +	lastbus = pci_rescan_bus(bridge->bus);
> > > +	pci_bus_update_busn_res_end(bridge->bus, lastbus);
> > > +#endif
> > >
> > >  	return 0;
> > >  }
> > > @@ -695,6 +725,7 @@ static struct pci_ops dw_pcie_ops = {
> > >  	.write = dw_pcie_wr_conf,
> > >  };
> > >
> > > +#ifdef CONFIG_ARM
> > >  static int dw_pcie_setup(int nr, struct pci_sys_data *sys)
> > >  {
> > >  	struct pcie_port *pp;
> > > @@ -758,6 +789,7 @@ static struct hw_pci dw_pci = {
> > >  	.map_irq	= dw_pcie_map_irq,
> > >  	.add_bus	= dw_pcie_add_bus,
> > >  };
> > > +#endif /* CONFIG_ARM */
> > >
> > >  void dw_pcie_setup_rc(struct pcie_port *pp)
> 


--
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
Jingoo Han - April 21, 2014, 9:58 a.m.
On Monday, April 21, 2014 10:54 AM, Jingoo Han wrote:
> On Thursday, April 17, 2014 3:26 AM, Arnd Bergmann wrote:
> > On Wednesday 16 April 2014 17:57:24 Liviu Dudau wrote:
> > > Jingoo,
> > >
> > > Thanks for taking a stab at trying to convert a host bridge
> > > driver to use the new generic host bridge code.
> > >
> > > I do however have concerns on the direction you took. You have split
> > > your driver in two, depending on whether it was CONFIG_ARM or CONFIG_ARM64,
> > > even if (with my series) it should be no reason why the host bridge
> > > driver should not work on other architectures as well once they are
> > > converted.
> >
> > Right.
> >
> > > Also, some of the functions that you use have identical names but different
> > > signatures depending on what arch you have selected. This is really bad
> > > in my books!

[.....]

> > > What about creating functions that use my series directly if CONFIG_ARM64 is
> > > defined (or any CONFIG_ you want to create for your driver that you select
> > > from CONFIG_ARM64) and otherwise implement the CONFIG_ARM version? That
> > > way your driver will call only one API without any #ifdef and when arm code
> > > gets converted you drop your adaptation functions. Or (better yet), have a
> > > stab at converting bios32 (Rob Herring has already provided some hints on
> > > how to do it for arch/arm).
> 
> To: Liviu Dudau
> 
> Sorry, but I will not implement this.
> At first, you had to think the compatibility with ARM32 PCIe.
> Why do you want other engineers to take this load?

(+cc Rob Herring)

Um, I am looking at Rob Herring's patchset for Versatile PCI. [1]
Then, do you mean the following?

1. Add Rob Herring's patch converting bios32. [2]
2. Reference Rob Herring's patch in order to know how to
    handle "of_create_pci_host_bridge()" directly in ARM32. [3]
3. Use of_create_pci_host_bridge() for the designware PCIe
    driver in ARM32.
4. Also, use of_create_pci_host_bridge() for the designware PCIe
    driver in "ARM64".


[1] http://www.spinics.net/lists/linux-pci/msg30084.html
[2] http://www.spinics.net/lists/linux-pci/msg30083.html
[3] http://www.spinics.net/lists/linux-pci/msg30086.html

Best regards,
Jingoo Han

> 
> >
> > That would of course be best.
> >

[.....]

--
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
Liviu Dudau - April 22, 2014, 12:54 p.m.
On Wed, Apr 16, 2014 at 07:26:15PM +0100, Arnd Bergmann wrote:
> On Wednesday 16 April 2014 17:57:24 Liviu Dudau wrote:
> > Jingoo,
> > 
> > Thanks for taking a stab at trying to convert a host bridge
> > driver to use the new generic host bridge code.
> > 
> > I do however have concerns on the direction you took. You have split
> > your driver in two, depending on whether it was CONFIG_ARM or CONFIG_ARM64,
> > even if (with my series) it should be no reason why the host bridge
> > driver should not work on other architectures as well once they are
> > converted.
> 
> Right.
> 
> > Also, some of the functions that you use have identical names but different
> > signatures depending on what arch you have selected. This is really bad
> > in my books!
> 
> It's only the sys_to_pcie() function, right?

Right.

> 
> You can probably simplify that to take a void pointer and have only one line
> difference.
> 
> > What about creating functions that use my series directly if CONFIG_ARM64 is
> > defined (or any CONFIG_ you want to create for your driver that you select
> > from CONFIG_ARM64) and otherwise implement the CONFIG_ARM version? That
> > way your driver will call only one API without any #ifdef and when arm code
> > gets converted you drop your adaptation functions. Or (better yet), have a
> > stab at converting bios32 (Rob Herring has already provided some hints on
> > how to do it for arch/arm).
> 
> That would of course be best.
> 
> > To give an example on how things are not going well in your version (not obvious
> > from your patch, but you can see it once you apply it): dw_pcie_host_init()
> > will still carry the handcoded version of DT parsing and that is not guarded
> > against CONFIG_ARM64 being defined, where the parsing will happen again
> > when you call of_create_pci_host_bridge().
> 
> How about making the generic DT parsing code from of_create_pci_host_bridge()
> an exported function that can be called by drivers that don't use
> of_create_pci_host_bridge?

Yes, I guess that should ease the transition to the new code even if the function
should not be needed for the users of the framework.

> 
> > Speaking of the handcoded DT parsing of resources: you are using restype == 0
> > as a way of selecting config space, *and then split the range size into two
> > halves*. From what Jason Gunthorpe and Arnd were saying, config ranges in the DT
> > tree should only be used for ECAM space, so no split is allowed.
> > 
> > Arnd, are you allowing this non-standard use to creep in the bindings?
> 
> I fear it's too late to change that now. In retrospect we probably shoulnd't
> have defined the binding like that.

OK, but I'm going to NAK the driver using the same tricks for arm64. Jingoo will
need to come up with an upgrade path where he has the config space described in
the reg property and parses that if CONFIG_ARM64 is enabled.

> 
> Overall, my impression of the patch is that it should be possible to do
> the same with much fewer #ifdefs by first rearranging the code in one patch
> and then doing another patch on top to add the generic arm64 support.

Agree.

Best regards,
Liviu

> 
> > > diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> > > index 6d23d8c..fac0440 100644
> > > --- a/drivers/pci/host/pcie-designware.c
> > > +++ b/drivers/pci/host/pcie-designware.c
> > > @@ -65,14 +65,27 @@
> > >  #define PCIE_ATU_FUNC(x)		(((x) & 0x7) << 16)
> > >  #define PCIE_ATU_UPPER_TARGET		0x91C
> > >  
> > > +#ifdef CONFIG_ARM
> > >  static struct hw_pci dw_pci;
> > > +#endif
> > >  
> > >  static unsigned long global_io_offset;
> > >  
> > > +#ifdef CONFIG_ARM
> > >  static inline struct pcie_port *sys_to_pcie(struct pci_sys_data *sys)
> > >  {
> > >  	return sys->private_data;
> > >  }
> > > +#endif
> > > +
> > > +#ifdef CONFIG_ARM64
> > > +static inline struct pcie_port *sys_to_pcie(struct pcie_port *pp)
> > > +{
> > > +	return pp;
> > > +}
> > > +
> > > +static struct pci_ops dw_pcie_ops;
> > > +#endif
> > >  
> > >  int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val)
> > >  {
> > > @@ -381,7 +394,9 @@ static int dw_pcie_msi_map(struct irq_domain *domain, unsigned int irq,
> > >  {
> > >  	irq_set_chip_and_handler(irq, &dw_msi_irq_chip, handle_simple_irq);
> > >  	irq_set_chip_data(irq, domain->host_data);
> > > +#ifdef CONFIG_ARM
> > >  	set_irq_flags(irq, IRQF_VALID);
> > > +#endif
> > >  
> > >  	return 0;
> > >  }
> > > @@ -397,6 +412,10 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
> > >  	struct of_pci_range_parser parser;
> > >  	u32 val;
> > >  	int i;
> > > +#ifdef CONFIG_ARM64
> > > +	struct pci_host_bridge *bridge;
> > > +	resource_size_t lastbus;
> > > +#endif
> > >  
> > >  	if (of_pci_range_parser_init(&parser, np)) {
> > >  		dev_err(pp->dev, "missing ranges property\n");
> > > @@ -489,6 +508,7 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
> > >  	val |= PORT_LOGIC_SPEED_CHANGE;
> > >  	dw_pcie_wr_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, val);
> > >  
> > > +#ifdef CONFIG_ARM
> > >  	dw_pci.nr_controllers = 1;
> > >  	dw_pci.private_data = (void **)&pp;
> > >  
> > > @@ -497,6 +517,16 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
> > >  #ifdef CONFIG_PCI_DOMAINS
> > >  	dw_pci.domain++;
> > >  #endif
> > > +#endif
> > > +
> > > +#ifdef CONFIG_ARM64
> > > +	bridge = of_create_pci_host_bridge(pp->dev, &dw_pcie_ops, pp);
> > > +	if (IS_ERR_OR_NULL(bridge))
> > > +		return PTR_ERR(bridge);
> > > +
> > > +	lastbus = pci_rescan_bus(bridge->bus);
> > > +	pci_bus_update_busn_res_end(bridge->bus, lastbus);
> > > +#endif
> > >  
> > >  	return 0;
> > >  }
> > > @@ -695,6 +725,7 @@ static struct pci_ops dw_pcie_ops = {
> > >  	.write = dw_pcie_wr_conf,
> > >  };
> > >  
> > > +#ifdef CONFIG_ARM
> > >  static int dw_pcie_setup(int nr, struct pci_sys_data *sys)
> > >  {
> > >  	struct pcie_port *pp;
> > > @@ -758,6 +789,7 @@ static struct hw_pci dw_pci = {
> > >  	.map_irq	= dw_pcie_map_irq,
> > >  	.add_bus	= dw_pcie_add_bus,
> > >  };
> > > +#endif /* CONFIG_ARM */
> > >  
> > >  void dw_pcie_setup_rc(struct pcie_port *pp)
> 
> 
> 
> 
>
Liviu Dudau - April 22, 2014, 12:59 p.m.
On Mon, Apr 21, 2014 at 02:54:12AM +0100, Jingoo Han wrote:
> On Thursday, April 17, 2014 3:26 AM, Arnd Bergmann wrote:
> 
> (+cc Mohit KUMAR, Pratyush Anand, Marek Vasut, Richard Zhu,
>       Kishon Vijay Abraham I, Byungho An)
> > 
> > On Wednesday 16 April 2014 17:57:24 Liviu Dudau wrote:
> > > Jingoo,
> > >
> > > Thanks for taking a stab at trying to convert a host bridge
> > > driver to use the new generic host bridge code.
> > >
> > > I do however have concerns on the direction you took. You have split
> > > your driver in two, depending on whether it was CONFIG_ARM or CONFIG_ARM64,
> > > even if (with my series) it should be no reason why the host bridge
> > > driver should not work on other architectures as well once they are
> > > converted.
> > 
> > Right.
> > 
> > > Also, some of the functions that you use have identical names but different
> > > signatures depending on what arch you have selected. This is really bad
> > > in my books!
> > 
> > It's only the sys_to_pcie() function, right?
> > 
> > You can probably simplify that to take a void pointer and have only one line
> > difference.
> 
> Do you mean the following?
> Would you give me more detailed advice?
> 
> static inline struct pcie_port *sys_to_pcie(void *sys)
> {
> 	struct pcie_port *pp
> 
> #ifdef CONFIG_ARM
> 	pp = ((struct pci_sys_data *)sys)->private_data;
> #else
> 	pp = (struct pcie_port *)sys;
> #endif
> 	return pp;
> }

Yes, that would be better.

> 
> > 
> > > What about creating functions that use my series directly if CONFIG_ARM64 is
> > > defined (or any CONFIG_ you want to create for your driver that you select
> > > from CONFIG_ARM64) and otherwise implement the CONFIG_ARM version? That
> > > way your driver will call only one API without any #ifdef and when arm code
> > > gets converted you drop your adaptation functions. Or (better yet), have a
> > > stab at converting bios32 (Rob Herring has already provided some hints on
> > > how to do it for arch/arm).
> 
> To: Liviu Dudau
> 
> Sorry, but I will not implement this.
> At first, you had to think the compatibility with ARM32 PCIe.

I did. I've also looked at the other implementations and have realised how unique
and baroque the arm32 version is. If I would've implemented that version for arm64
(Will Deacon had a stab internally) I would have to remove all the quirks from
the arch code and all that is left is something very similar to what you currently
have. But that would've made that code only usable by ARM platforms and no one
else. Why being so narrow?

> Why do you want other engineers to take this load?

I don't want to, but you are the first scratching that itch. This is your path to
glory!

> 
> > 
> > That would of course be best.
> > 
> > > To give an example on how things are not going well in your version (not obvious
> > > from your patch, but you can see it once you apply it): dw_pcie_host_init()
> > > will still carry the handcoded version of DT parsing and that is not guarded
> > > against CONFIG_ARM64 being defined, where the parsing will happen again
> > > when you call of_create_pci_host_bridge().
> > 
> > How about making the generic DT parsing code from of_create_pci_host_bridge()
> > an exported function that can be called by drivers that don't use
> > of_create_pci_host_bridge?
> 
> Do you mean that of_create_pci_host_bridge() should be used for both ARM32
> and ARM64 cases?

Yes, ultimately. 

> 
> > 
> > > Speaking of the handcoded DT parsing of resources: you are using restype == 0
> > > as a way of selecting config space, *and then split the range size into two
> > > halves*. From what Jason Gunthorpe and Arnd were saying, config ranges in the DT
> > > tree should only be used for ECAM space, so no split is allowed.
> > >
> > > Arnd, are you allowing this non-standard use to creep in the bindings?
> > 
> > I fear it's too late to change that now. In retrospect we probably shoulnd't
> > have defined the binding like that.
> > 
> > Overall, my impression of the patch is that it should be possible to do
> > the same with much fewer #ifdefs by first rearranging the code in one patch
> > and then doing another patch on top to add the generic arm64 support.
> 
> I will try to reduce #ifdefs as possible.

Thanks!

Best regards,
Liviu

> 
> Best regards,
> Jingoo Han
> 
> > 
> > > > diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> > > > index 6d23d8c..fac0440 100644
> > > > --- a/drivers/pci/host/pcie-designware.c
> > > > +++ b/drivers/pci/host/pcie-designware.c
> > > > @@ -65,14 +65,27 @@
> > > >  #define PCIE_ATU_FUNC(x)		(((x) & 0x7) << 16)
> > > >  #define PCIE_ATU_UPPER_TARGET		0x91C
> > > >
> > > > +#ifdef CONFIG_ARM
> > > >  static struct hw_pci dw_pci;
> > > > +#endif
> > > >
> > > >  static unsigned long global_io_offset;
> > > >
> > > > +#ifdef CONFIG_ARM
> > > >  static inline struct pcie_port *sys_to_pcie(struct pci_sys_data *sys)
> > > >  {
> > > >  	return sys->private_data;
> > > >  }
> > > > +#endif
> > > > +
> > > > +#ifdef CONFIG_ARM64
> > > > +static inline struct pcie_port *sys_to_pcie(struct pcie_port *pp)
> > > > +{
> > > > +	return pp;
> > > > +}
> > > > +
> > > > +static struct pci_ops dw_pcie_ops;
> > > > +#endif
> > > >
> > > >  int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val)
> > > >  {
> > > > @@ -381,7 +394,9 @@ static int dw_pcie_msi_map(struct irq_domain *domain, unsigned int irq,
> > > >  {
> > > >  	irq_set_chip_and_handler(irq, &dw_msi_irq_chip, handle_simple_irq);
> > > >  	irq_set_chip_data(irq, domain->host_data);
> > > > +#ifdef CONFIG_ARM
> > > >  	set_irq_flags(irq, IRQF_VALID);
> > > > +#endif
> > > >
> > > >  	return 0;
> > > >  }
> > > > @@ -397,6 +412,10 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
> > > >  	struct of_pci_range_parser parser;
> > > >  	u32 val;
> > > >  	int i;
> > > > +#ifdef CONFIG_ARM64
> > > > +	struct pci_host_bridge *bridge;
> > > > +	resource_size_t lastbus;
> > > > +#endif
> > > >
> > > >  	if (of_pci_range_parser_init(&parser, np)) {
> > > >  		dev_err(pp->dev, "missing ranges property\n");
> > > > @@ -489,6 +508,7 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
> > > >  	val |= PORT_LOGIC_SPEED_CHANGE;
> > > >  	dw_pcie_wr_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, val);
> > > >
> > > > +#ifdef CONFIG_ARM
> > > >  	dw_pci.nr_controllers = 1;
> > > >  	dw_pci.private_data = (void **)&pp;
> > > >
> > > > @@ -497,6 +517,16 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
> > > >  #ifdef CONFIG_PCI_DOMAINS
> > > >  	dw_pci.domain++;
> > > >  #endif
> > > > +#endif
> > > > +
> > > > +#ifdef CONFIG_ARM64
> > > > +	bridge = of_create_pci_host_bridge(pp->dev, &dw_pcie_ops, pp);
> > > > +	if (IS_ERR_OR_NULL(bridge))
> > > > +		return PTR_ERR(bridge);
> > > > +
> > > > +	lastbus = pci_rescan_bus(bridge->bus);
> > > > +	pci_bus_update_busn_res_end(bridge->bus, lastbus);
> > > > +#endif
> > > >
> > > >  	return 0;
> > > >  }
> > > > @@ -695,6 +725,7 @@ static struct pci_ops dw_pcie_ops = {
> > > >  	.write = dw_pcie_wr_conf,
> > > >  };
> > > >
> > > > +#ifdef CONFIG_ARM
> > > >  static int dw_pcie_setup(int nr, struct pci_sys_data *sys)
> > > >  {
> > > >  	struct pcie_port *pp;
> > > > @@ -758,6 +789,7 @@ static struct hw_pci dw_pci = {
> > > >  	.map_irq	= dw_pcie_map_irq,
> > > >  	.add_bus	= dw_pcie_add_bus,
> > > >  };
> > > > +#endif /* CONFIG_ARM */
> > > >
> > > >  void dw_pcie_setup_rc(struct pcie_port *pp)
> > 
> 
> 
>
Liviu Dudau - April 22, 2014, 1:01 p.m.
On Mon, Apr 21, 2014 at 10:58:52AM +0100, Jingoo Han wrote:
> On Monday, April 21, 2014 10:54 AM, Jingoo Han wrote:
> > On Thursday, April 17, 2014 3:26 AM, Arnd Bergmann wrote:
> > > On Wednesday 16 April 2014 17:57:24 Liviu Dudau wrote:
> > > > Jingoo,
> > > >
> > > > Thanks for taking a stab at trying to convert a host bridge
> > > > driver to use the new generic host bridge code.
> > > >
> > > > I do however have concerns on the direction you took. You have split
> > > > your driver in two, depending on whether it was CONFIG_ARM or CONFIG_ARM64,
> > > > even if (with my series) it should be no reason why the host bridge
> > > > driver should not work on other architectures as well once they are
> > > > converted.
> > >
> > > Right.
> > >
> > > > Also, some of the functions that you use have identical names but different
> > > > signatures depending on what arch you have selected. This is really bad
> > > > in my books!
> 
> [.....]
> 
> > > > What about creating functions that use my series directly if CONFIG_ARM64 is
> > > > defined (or any CONFIG_ you want to create for your driver that you select
> > > > from CONFIG_ARM64) and otherwise implement the CONFIG_ARM version? That
> > > > way your driver will call only one API without any #ifdef and when arm code
> > > > gets converted you drop your adaptation functions. Or (better yet), have a
> > > > stab at converting bios32 (Rob Herring has already provided some hints on
> > > > how to do it for arch/arm).
> > 
> > To: Liviu Dudau
> > 
> > Sorry, but I will not implement this.
> > At first, you had to think the compatibility with ARM32 PCIe.
> > Why do you want other engineers to take this load?
> 
> (+cc Rob Herring)
> 
> Um, I am looking at Rob Herring's patchset for Versatile PCI. [1]
> Then, do you mean the following?
> 
> 1. Add Rob Herring's patch converting bios32. [2]
> 2. Reference Rob Herring's patch in order to know how to
>     handle "of_create_pci_host_bridge()" directly in ARM32. [3]
> 3. Use of_create_pci_host_bridge() for the designware PCIe
>     driver in ARM32.
> 4. Also, use of_create_pci_host_bridge() for the designware PCIe
>     driver in "ARM64".
> 

Sounds like a good plan. 3 and 4 should be one and the same in my opinion, but
there might be more things in designware driver that I am missing right now.

Best regards,
Liviu

> 
> [1] http://www.spinics.net/lists/linux-pci/msg30084.html
> [2] http://www.spinics.net/lists/linux-pci/msg30083.html
> [3] http://www.spinics.net/lists/linux-pci/msg30086.html
> 
> Best regards,
> Jingoo Han
> 
> > 
> > >
> > > That would of course be best.
> > >
> 
> [.....]
> 
>
Rob Herring - April 22, 2014, 3:39 p.m.
On Mon, Apr 21, 2014 at 4:58 AM, Jingoo Han <jg1.han@samsung.com> wrote:
> On Monday, April 21, 2014 10:54 AM, Jingoo Han wrote:
>> On Thursday, April 17, 2014 3:26 AM, Arnd Bergmann wrote:
>> > On Wednesday 16 April 2014 17:57:24 Liviu Dudau wrote:
>> > > Jingoo,
>> > >
>> > > Thanks for taking a stab at trying to convert a host bridge
>> > > driver to use the new generic host bridge code.
>> > >
>> > > I do however have concerns on the direction you took. You have split
>> > > your driver in two, depending on whether it was CONFIG_ARM or CONFIG_ARM64,
>> > > even if (with my series) it should be no reason why the host bridge
>> > > driver should not work on other architectures as well once they are
>> > > converted.
>> >
>> > Right.
>> >
>> > > Also, some of the functions that you use have identical names but different
>> > > signatures depending on what arch you have selected. This is really bad
>> > > in my books!
>
> [.....]
>
>> > > What about creating functions that use my series directly if CONFIG_ARM64 is
>> > > defined (or any CONFIG_ you want to create for your driver that you select
>> > > from CONFIG_ARM64) and otherwise implement the CONFIG_ARM version? That
>> > > way your driver will call only one API without any #ifdef and when arm code
>> > > gets converted you drop your adaptation functions. Or (better yet), have a
>> > > stab at converting bios32 (Rob Herring has already provided some hints on
>> > > how to do it for arch/arm).
>>
>> To: Liviu Dudau
>>
>> Sorry, but I will not implement this.

Then you will not get what you want upstream.

>> At first, you had to think the compatibility with ARM32 PCIe.
>> Why do you want other engineers to take this load?

That is how the process works.

> (+cc Rob Herring)
>
> Um, I am looking at Rob Herring's patchset for Versatile PCI. [1]
> Then, do you mean the following?
>
> 1. Add Rob Herring's patch converting bios32. [2]

I would expect all or most of this patch to go away in Liviu's next
version. It does at least show we don't have to fix all of ARM PCI
support to use of_create_pci_host_bridge on ARM.

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

Patch

diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index 6d23d8c..fac0440 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -65,14 +65,27 @@ 
 #define PCIE_ATU_FUNC(x)		(((x) & 0x7) << 16)
 #define PCIE_ATU_UPPER_TARGET		0x91C
 
+#ifdef CONFIG_ARM
 static struct hw_pci dw_pci;
+#endif
 
 static unsigned long global_io_offset;
 
+#ifdef CONFIG_ARM
 static inline struct pcie_port *sys_to_pcie(struct pci_sys_data *sys)
 {
 	return sys->private_data;
 }
+#endif
+
+#ifdef CONFIG_ARM64
+static inline struct pcie_port *sys_to_pcie(struct pcie_port *pp)
+{
+	return pp;
+}
+
+static struct pci_ops dw_pcie_ops;
+#endif
 
 int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val)
 {
@@ -381,7 +394,9 @@  static int dw_pcie_msi_map(struct irq_domain *domain, unsigned int irq,
 {
 	irq_set_chip_and_handler(irq, &dw_msi_irq_chip, handle_simple_irq);
 	irq_set_chip_data(irq, domain->host_data);
+#ifdef CONFIG_ARM
 	set_irq_flags(irq, IRQF_VALID);
+#endif
 
 	return 0;
 }
@@ -397,6 +412,10 @@  int __init dw_pcie_host_init(struct pcie_port *pp)
 	struct of_pci_range_parser parser;
 	u32 val;
 	int i;
+#ifdef CONFIG_ARM64
+	struct pci_host_bridge *bridge;
+	resource_size_t lastbus;
+#endif
 
 	if (of_pci_range_parser_init(&parser, np)) {
 		dev_err(pp->dev, "missing ranges property\n");
@@ -489,6 +508,7 @@  int __init dw_pcie_host_init(struct pcie_port *pp)
 	val |= PORT_LOGIC_SPEED_CHANGE;
 	dw_pcie_wr_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, val);
 
+#ifdef CONFIG_ARM
 	dw_pci.nr_controllers = 1;
 	dw_pci.private_data = (void **)&pp;
 
@@ -497,6 +517,16 @@  int __init dw_pcie_host_init(struct pcie_port *pp)
 #ifdef CONFIG_PCI_DOMAINS
 	dw_pci.domain++;
 #endif
+#endif
+
+#ifdef CONFIG_ARM64
+	bridge = of_create_pci_host_bridge(pp->dev, &dw_pcie_ops, pp);
+	if (IS_ERR_OR_NULL(bridge))
+		return PTR_ERR(bridge);
+
+	lastbus = pci_rescan_bus(bridge->bus);
+	pci_bus_update_busn_res_end(bridge->bus, lastbus);
+#endif
 
 	return 0;
 }
@@ -695,6 +725,7 @@  static struct pci_ops dw_pcie_ops = {
 	.write = dw_pcie_wr_conf,
 };
 
+#ifdef CONFIG_ARM
 static int dw_pcie_setup(int nr, struct pci_sys_data *sys)
 {
 	struct pcie_port *pp;
@@ -758,6 +789,7 @@  static struct hw_pci dw_pci = {
 	.map_irq	= dw_pcie_map_irq,
 	.add_bus	= dw_pcie_add_bus,
 };
+#endif /* CONFIG_ARM */
 
 void dw_pcie_setup_rc(struct pcie_port *pp)
 {