Re: [RFC PATCH 1/2] PCI: designware: Add ARM64 PCI support

[Jingoo Han <jg1.han@samsung.com>]

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