[v4,1/9] PCI: host: rcar: Add Renesas R-Car PCIe driver

This PCIe Host driver currently does not support MSI, so cards
fall back to INTx interrupts.

Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
---
v4:
 - Use runtime PM properly

v3:
 - Add DT support
 - Use 'of_irq_parse_and_map_pci' for '.map_irq'
 - Use pm ops to enable clocks
 - Fix checkpatch errors
 - Use subsys_initcall to overcome issues with port bus driver
 - Adjust Kconfig to match other R-Car drivers

v2:
 - Use msleep instead of udelay when waiting for the link
 - Use pm_runtime
 - Removed unused definition
 - Also replaced call to devm_request_and_ioremap with devm_ioremap_resource
   and fixed a bug with this when reporting errors.
---
 drivers/pci/host/Kconfig     |   6 +
 drivers/pci/host/Makefile    |   1 +
 drivers/pci/host/pcie-rcar.c | 601 +++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/host/pcie-rcar.h |  82 ++++++
 4 files changed, 690 insertions(+)
 create mode 100644 drivers/pci/host/pcie-rcar.c
 create mode 100644 drivers/pci/host/pcie-rcar.h

On Friday 21 March 2014 10:32:40 Phil Edworthy wrote:
> +static const struct of_device_id rcar_pcie_of_match[] = {
> +	{ .compatible = "renesas,r8a7779-pcie", .data = (void *)RCAR_H1 },
> +	{ .compatible = "renesas,r8a7790-pcie", .data = (void *)RCAR_GENERIC },
> +	{ .compatible = "renesas,r8a7791-pcie", .data = (void *)RCAR_GENERIC },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, rcar_pcie_of_match);

The only difference between RCAR_H1 and RCAR_GENERIC seems to be the
use of the rcar_pcie_phy_init_rcar_h1 vs rcar_pcie_hw_init
functions. I think this would be expressed in a nicer way doing

static const struct of_device_id rcar_pcie_of_match[] = {
	{ .compatible = "renesas,r8a7779-pcie", .data =  rcar_pcie_phy_init_rcar_h1},
	{ .compatible = "renesas,r8a7790-pcie", .data =  rcar_pcie_hw_init},
	{ .compatible = "renesas,r8a7791-pcie", .data =  rcar_pcie_hw_init},
	{},
};

If you need other differences in the future, you can extend this
to use a pointer to a struct containing the init function pointer.

> +static int rcar_pcie_setup_window(int win, struct resource *res,
> +					struct rcar_pcie *pcie)
> +{
> +	/* Setup PCIe address space mappings for each resource */
> +	resource_size_t size;
> +	u32 mask;
> +
> +	pci_write_reg(pcie, 0x00000000, PCIEPTCTLR(win));
> +
> +	/*
> +	 * The PAMR mask is calculated in units of 128Bytes, which
> +	 * keeps things pretty simple.
> +	 */
> +	size = resource_size(res);
> +	mask = (roundup_pow_of_two(size) / SZ_128) - 1;
> +	pci_write_reg(pcie, mask << 7, PCIEPAMR(win));
> +
> +	pci_write_reg(pcie, upper_32_bits(res->start), PCIEPARH(win));
> +	pci_write_reg(pcie, lower_32_bits(res->start), PCIEPARL(win));
> +
> +	/* First resource is for IO */
> +	mask = PAR_ENABLE;
> +	if (res->flags & IORESOURCE_IO)
> +		mask |= IO_SPACE;
> +
> +	pci_write_reg(pcie, mask, PCIEPTCTLR(win));
> +
> +	return 0;
> +}

Would it be possible to have this done in the boot loader so the kernel
doesn't need to be bothered with it?

> +
> +static int rcar_pcie_setup(int nr, struct pci_sys_data *sys)
> +{
> +	struct rcar_pcie *pcie = sys_to_pcie(sys);
> +	struct resource *res;
> +	int i, ret;
> +
> +	pcie->root_bus_nr = sys->busnr;
> +
> +	/* Setup PCI resources */
> +	for (i = 0; i < PCI_MAX_RESOURCES; i++) {
> +
> +		res = &pcie->res[i];
> +		if (!res->flags)
> +			continue;
> +
> +		if (res->flags & IORESOURCE_IO) {
> +			/* Setup IO mapped memory accesses */
> +			ret = pci_ioremap_io(0, res->start);
> +			if (ret)
> +				return 1;
> +
> +			/* Correct addresses for remapped IO */
> +			res->end = res->end - res->start;
> +			res->start = 0;
> +		}
> +
> +		rcar_pcie_setup_window(i, res, pcie);
> +		pci_add_resource(&sys->resources, res);
> +	}

This assumes that the start of the I/O window is always at
bus address 0, and that you have only one PCI host in the system.
Are both guaranteed through the hardware design?

> +static void __init rcar_pcie_enable(struct rcar_pcie *pcie)
> +{
> +	struct platform_device *pdev = to_platform_device(pcie->dev);
> +	struct hw_pci hw;
> +
> +	memset(&hw, 0, sizeof(hw));
> +
> +	hw.nr_controllers = 1;
> +	hw.private_data   = (void **)&pcie;
> +	hw.setup          = rcar_pcie_setup,
> +	hw.map_irq        = of_irq_parse_and_map_pci,
> +	hw.ops		  = &rcar_pcie_ops,

You can write this slightly nicer doing

	struct hw_pci hw = {
		.nr_controllers = 1,
		.private_data = (void **)&pcie,
		...
	};
> +static int __init rcar_pcie_phy_init_rcar_h1(struct rcar_pcie *pcie)
> +{
> +	unsigned int timeout = 10;
> +
> +	/* Initialize the phy */
> +	phy_write_reg(pcie, 0, 0x42, 0x1, 0x0EC34191);
> +	phy_write_reg(pcie, 1, 0x42, 0x1, 0x0EC34180);
> +	phy_write_reg(pcie, 0, 0x43, 0x1, 0x00210188);
> +	phy_write_reg(pcie, 1, 0x43, 0x1, 0x00210188);
> +	phy_write_reg(pcie, 0, 0x44, 0x1, 0x015C0014);
> +	phy_write_reg(pcie, 1, 0x44, 0x1, 0x015C0014);
> +	phy_write_reg(pcie, 1, 0x4C, 0x1, 0x786174A0);

Have you considered moving this into a separate PHY driver?
There are probably good reasons either way, so I'm not asking you
to do it, just to think about what would be best for this driver.

It seems that you already have two different implementations
that only vary in how they access the PHY, so moving that
into a separate driver would keep the knowledge out of the
host driver. OTOH, the PHY registers look like they are part of
the pci host itself.

> +	/*
> +	 * For target transfers, setup a single 64-bit 4GB mapping at address
> +	 * 0. The hardware can only map memory that starts on a power of two
> +	 * boundary, and size is power of 2, so best to ignore it.
> +	 */
> +	pci_write_reg(pcie, 0, PCIEPRAR(0));
> +	pci_write_reg(pcie, 0, PCIELAR(0));
> +	pci_write_reg(pcie, 0xfffffff0UL | LAM_PREFETCH |
> +		LAM_64BIT | LAR_ENABLE, PCIELAMR(0));
> +	pci_write_reg(pcie, 0, PCIELAR(1));
> +	pci_write_reg(pcie, 0, PCIEPRAR(1));
> +	pci_write_reg(pcie, 0, PCIELAMR(1));

Is this the only reasonable configuration? If not, you might want to
do the same thing as the x-gene PCI driver that is not yet merged,
and parse the 'dma-ranges' property to determine what the mapping
on a particular machine should be.

Most importantly, why don't you use a mapping that includes RAM beyond
the first 4GB?

> +static int __init pcie_init(void)
> +{
> +	return platform_driver_probe(&rcar_pcie_driver, rcar_pcie_probe);
> +}
> +subsys_initcall(pcie_init);

Why so early?

Overall, this driver looks pretty good.

	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

Hi Arnd,

Thanks for the review.

> From: Arnd Bergmann <arnd@arndb.de>
> To: linux-arm-kernel@lists.infradead.org, 
> Cc: Phil Edworthy <phil.edworthy@renesas.com>, 
linux-pci@vger.kernel.org, 
> linux-sh@vger.kernel.org, Magnus Damm <magnus.damm@gmail.com>, Valentine 

> Barshak <valentine.barshak@cogentembedded.com>, Simon Horman 
> <horms@verge.net.au>, Bjorn Helgaas <bhelgaas@google.com>, Ben Dooks 
> <ben.dooks@codethink.co.uk>
> Date: 21/03/2014 11:04
> Subject: Re: [PATCH v4 1/9] PCI: host: rcar: Add Renesas R-Car PCIe 
driver
> 
> On Friday 21 March 2014 10:32:40 Phil Edworthy wrote:
> > +static const struct of_device_id rcar_pcie_of_match[] = {
> > +   { .compatible = "renesas,r8a7779-pcie", .data = (void *)RCAR_H1 },
> > +   { .compatible = "renesas,r8a7790-pcie", .data = (void 
*)RCAR_GENERIC },
> > +   { .compatible = "renesas,r8a7791-pcie", .data = (void 
*)RCAR_GENERIC },
> > +   {},
> > +};
> > +MODULE_DEVICE_TABLE(of, rcar_pcie_of_match);
> 
> The only difference between RCAR_H1 and RCAR_GENERIC seems to be the
> use of the rcar_pcie_phy_init_rcar_h1 vs rcar_pcie_hw_init
> functions. I think this would be expressed in a nicer way doing
> 
> static const struct of_device_id rcar_pcie_of_match[] = {
>    { .compatible = "renesas,r8a7779-pcie", .data = 
rcar_pcie_phy_init_rcar_h1},
>    { .compatible = "renesas,r8a7790-pcie", .data =  rcar_pcie_hw_init},
>    { .compatible = "renesas,r8a7791-pcie", .data =  rcar_pcie_hw_init},
>    {},
> };
> 
> If you need other differences in the future, you can extend this
> to use a pointer to a struct containing the init function pointer.
Ok, I understand what you mean, though the rcar_pcie_phy_init_rcar_h1 
function is an extra call for r8a7779 devices to setup the PHY, not an 
alternative to rcar_pcie_hw_init.


> > +static int rcar_pcie_setup_window(int win, struct resource *res,
> > +               struct rcar_pcie *pcie)
> > +{
> > +   /* Setup PCIe address space mappings for each resource */
> > +   resource_size_t size;
> > +   u32 mask;
> > +
> > +   pci_write_reg(pcie, 0x00000000, PCIEPTCTLR(win));
> > +
> > +   /*
> > +    * The PAMR mask is calculated in units of 128Bytes, which
> > +    * keeps things pretty simple.
> > +    */
> > +   size = resource_size(res);
> > +   mask = (roundup_pow_of_two(size) / SZ_128) - 1;
> > +   pci_write_reg(pcie, mask << 7, PCIEPAMR(win));
> > +
> > +   pci_write_reg(pcie, upper_32_bits(res->start), PCIEPARH(win));
> > +   pci_write_reg(pcie, lower_32_bits(res->start), PCIEPARL(win));
> > +
> > +   /* First resource is for IO */
> > +   mask = PAR_ENABLE;
> > +   if (res->flags & IORESOURCE_IO)
> > +      mask |= IO_SPACE;
> > +
> > +   pci_write_reg(pcie, mask, PCIEPTCTLR(win));
> > +
> > +   return 0;
> > +}
> 
> Would it be possible to have this done in the boot loader so the kernel
> doesn't need to be bothered with it?
Hmm, I don't like the idea of depending on settings made by a bootloader, 
when it may be that a different bootloader could be used, or no bootloader 
at all.


> > +
> > +static int rcar_pcie_setup(int nr, struct pci_sys_data *sys)
> > +{
> > +   struct rcar_pcie *pcie = sys_to_pcie(sys);
> > +   struct resource *res;
> > +   int i, ret;
> > +
> > +   pcie->root_bus_nr = sys->busnr;
> > +
> > +   /* Setup PCI resources */
> > +   for (i = 0; i < PCI_MAX_RESOURCES; i++) {
> > +
> > +      res = &pcie->res[i];
> > +      if (!res->flags)
> > +         continue;
> > +
> > +      if (res->flags & IORESOURCE_IO) {
> > +         /* Setup IO mapped memory accesses */
> > +         ret = pci_ioremap_io(0, res->start);
> > +         if (ret)
> > +            return 1;
> > +
> > +         /* Correct addresses for remapped IO */
> > +         res->end = res->end - res->start;
> > +         res->start = 0;
> > +      }
> > +
> > +      rcar_pcie_setup_window(i, res, pcie);
> > +      pci_add_resource(&sys->resources, res);
> > +   }
> 
> This assumes that the start of the I/O window is always at
> bus address 0, and that you have only one PCI host in the system.
> Are both guaranteed through the hardware design?
The I/O window can be anywhere I believe, so I guess I should pull the 
actual bus address from the DT.
All existing devices I have seen, have a single PCIe host.


> > +static void __init rcar_pcie_enable(struct rcar_pcie *pcie)
> > +{
> > +   struct platform_device *pdev = to_platform_device(pcie->dev);
> > +   struct hw_pci hw;
> > +
> > +   memset(&hw, 0, sizeof(hw));
> > +
> > +   hw.nr_controllers = 1;
> > +   hw.private_data   = (void **)&pcie;
> > +   hw.setup          = rcar_pcie_setup,
> > +   hw.map_irq        = of_irq_parse_and_map_pci,
> > +   hw.ops        = &rcar_pcie_ops,
> 
> You can write this slightly nicer doing
> 
>    struct hw_pci hw = {
>       .nr_controllers = 1,
>       .private_data = (void **)&pcie,
>       ...
>    };
Ok, will do.


> > +static int __init rcar_pcie_phy_init_rcar_h1(struct rcar_pcie *pcie)
> > +{
> > +   unsigned int timeout = 10;
> > +
> > +   /* Initialize the phy */
> > +   phy_write_reg(pcie, 0, 0x42, 0x1, 0x0EC34191);
> > +   phy_write_reg(pcie, 1, 0x42, 0x1, 0x0EC34180);
> > +   phy_write_reg(pcie, 0, 0x43, 0x1, 0x00210188);
> > +   phy_write_reg(pcie, 1, 0x43, 0x1, 0x00210188);
> > +   phy_write_reg(pcie, 0, 0x44, 0x1, 0x015C0014);
> > +   phy_write_reg(pcie, 1, 0x44, 0x1, 0x015C0014);
> > +   phy_write_reg(pcie, 1, 0x4C, 0x1, 0x786174A0);
> 
> Have you considered moving this into a separate PHY driver?
> There are probably good reasons either way, so I'm not asking you
> to do it, just to think about what would be best for this driver.
> 
> It seems that you already have two different implementations
> that only vary in how they access the PHY, so moving that
> into a separate driver would keep the knowledge out of the
> host driver. OTOH, the PHY registers look like they are part of
> the pci host itself.
The PHYs are separate entities, but access to them is via the PCIe 
controller's registers. In the general case, there shouldn't be any PHY 
setup to do, it's just the R-Car H1 devices that needs a little tweaking.


> > +   /*
> > +    * For target transfers, setup a single 64-bit 4GB mapping at 
address
> > +    * 0. The hardware can only map memory that starts on a power of 
two
> > +    * boundary, and size is power of 2, so best to ignore it.
> > +    */
> > +   pci_write_reg(pcie, 0, PCIEPRAR(0));
> > +   pci_write_reg(pcie, 0, PCIELAR(0));
> > +   pci_write_reg(pcie, 0xfffffff0UL | LAM_PREFETCH |
> > +      LAM_64BIT | LAR_ENABLE, PCIELAMR(0));
> > +   pci_write_reg(pcie, 0, PCIELAR(1));
> > +   pci_write_reg(pcie, 0, PCIEPRAR(1));
> > +   pci_write_reg(pcie, 0, PCIELAMR(1));
> 
> Is this the only reasonable configuration? If not, you might want to
> do the same thing as the x-gene PCI driver that is not yet merged,
> and parse the 'dma-ranges' property to determine what the mapping
> on a particular machine should be.
Based on the restrictions of these registers, it seemed like a sensible 
approach to open up the whole 32-bit address space. I'll have a look at 
the x-gene patches to see if that would be better.

> Most importantly, why don't you use a mapping that includes RAM beyond
> the first 4GB?
I had intended to send a patch later on to open this up to the first 12GB. 
Limitations of this block of hardware mean that we can do at most three 
4GB regions. The first 12GB would be ok for LPAE on existing devices as 
all DDR is mapped within that range, but obviously not ideal.


> > +static int __init pcie_init(void)
> > +{
> > +   return platform_driver_probe(&rcar_pcie_driver, rcar_pcie_probe);
> > +}
> > +subsys_initcall(pcie_init);
> 
> Why so early?
Good catch, copied from another PCIe driver!

> Overall, this driver looks pretty good.
> 
>    Arnd

Thanks
Phil
--
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

On 21/03/14 14:59, Phil.Edworthy@renesas.com wrote:
> Hi Arnd,
>
> Thanks for the review.
>
>
>>> +static int __init pcie_init(void)
>>> +{
>>> +   return platform_driver_probe(&rcar_pcie_driver, rcar_pcie_probe);
>>> +}
>>> +subsys_initcall(pcie_init);
>>
>> Why so early?
> Good catch, copied from another PCIe driver!

I think you can change it to use module_driver() instead of the
init_call.

On Friday 21 March 2014 13:59:35 Phil.Edworthy@renesas.com wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> > static const struct of_device_id rcar_pcie_of_match[] = {
> >    { .compatible = "renesas,r8a7779-pcie", .data = 
> rcar_pcie_phy_init_rcar_h1},
> >    { .compatible = "renesas,r8a7790-pcie", .data =  rcar_pcie_hw_init},
> >    { .compatible = "renesas,r8a7791-pcie", .data =  rcar_pcie_hw_init},
> >    {},
> > };
> > 
> > If you need other differences in the future, you can extend this
> > to use a pointer to a struct containing the init function pointer.
>
> Ok, I understand what you mean, though the rcar_pcie_phy_init_rcar_h1 
> function is an extra call for r8a7779 devices to setup the PHY, not an 
> alternative to rcar_pcie_hw_init.

I assumed that would be trivial to change, by calling rcar_pcie_hw_init
at the end of rcar_pcie_phy_init_rcar_h1 rather than the other way round.

> > > +static int rcar_pcie_setup_window(int win, struct resource *res,
> > > +               struct rcar_pcie *pcie)
> > > +{
> > > +   /* Setup PCIe address space mappings for each resource */
> > > +   resource_size_t size;
> > > +   u32 mask;
> > > +
> > > +   pci_write_reg(pcie, 0x00000000, PCIEPTCTLR(win));
> > > +
> > > +   /*
> > > +    * The PAMR mask is calculated in units of 128Bytes, which
> > > +    * keeps things pretty simple.
> > > +    */
> > > +   size = resource_size(res);
> > > +   mask = (roundup_pow_of_two(size) / SZ_128) - 1;
> > > +   pci_write_reg(pcie, mask << 7, PCIEPAMR(win));
> > > +
> > > +   pci_write_reg(pcie, upper_32_bits(res->start), PCIEPARH(win));
> > > +   pci_write_reg(pcie, lower_32_bits(res->start), PCIEPARL(win));
> > > +
> > > +   /* First resource is for IO */
> > > +   mask = PAR_ENABLE;
> > > +   if (res->flags & IORESOURCE_IO)
> > > +      mask |= IO_SPACE;
> > > +
> > > +   pci_write_reg(pcie, mask, PCIEPTCTLR(win));
> > > +
> > > +   return 0;
> > > +}
> > 
> > Would it be possible to have this done in the boot loader so the kernel
> > doesn't need to be bothered with it?
>
> Hmm, I don't like the idea of depending on settings made by a bootloader, 
> when it may be that a different bootloader could be used, or no bootloader 
> at all.

If you need to support the case without a boot loader, that would be
a good reason to leave the code in.

Otherwise, I think it's better to mandate early that the boot loader
sets up a lot of the basic stuff. As long as nobody has a kernel that
does the same setup, you also won't see any boot loaders that get it
wrong, because they never work. It's also easy enough to add back
if you later have to support a broken boot loader.

> > > +static int rcar_pcie_setup(int nr, struct pci_sys_data *sys)
> > > +{
> > > +   struct rcar_pcie *pcie = sys_to_pcie(sys);
> > > +   struct resource *res;
> > > +   int i, ret;
> > > +
> > > +   pcie->root_bus_nr = sys->busnr;
> > > +
> > > +   /* Setup PCI resources */
> > > +   for (i = 0; i < PCI_MAX_RESOURCES; i++) {
> > > +
> > > +      res = &pcie->res[i];
> > > +      if (!res->flags)
> > > +         continue;
> > > +
> > > +      if (res->flags & IORESOURCE_IO) {
> > > +         /* Setup IO mapped memory accesses */
> > > +         ret = pci_ioremap_io(0, res->start);
> > > +         if (ret)
> > > +            return 1;
> > > +
> > > +         /* Correct addresses for remapped IO */
> > > +         res->end = res->end - res->start;
> > > +         res->start = 0;
> > > +      }
> > > +
> > > +      rcar_pcie_setup_window(i, res, pcie);
> > > +      pci_add_resource(&sys->resources, res);
> > > +   }
> > 
> > This assumes that the start of the I/O window is always at
> > bus address 0, and that you have only one PCI host in the system.
> > Are both guaranteed through the hardware design?
>
> The I/O window can be anywhere I believe, so I guess I should pull the 
> actual bus address from the DT.

Ok. There is actually work on the way to simplify this a lot
so you don't have to do it in the driver, but it will probably
take a little more time, especially since Will Deacon is on an
extended vacation at the moment and he was driving things.

> All existing devices I have seen, have a single PCIe host.

The seems less obvious to rely on though, it's quite common
for newer hardware to have multiple blocks of whatever the first
generation has only one of ;-)

> > > +static int __init rcar_pcie_phy_init_rcar_h1(struct rcar_pcie *pcie)
> > > +{
> > > +   unsigned int timeout = 10;
> > > +
> > > +   /* Initialize the phy */
> > > +   phy_write_reg(pcie, 0, 0x42, 0x1, 0x0EC34191);
> > > +   phy_write_reg(pcie, 1, 0x42, 0x1, 0x0EC34180);
> > > +   phy_write_reg(pcie, 0, 0x43, 0x1, 0x00210188);
> > > +   phy_write_reg(pcie, 1, 0x43, 0x1, 0x00210188);
> > > +   phy_write_reg(pcie, 0, 0x44, 0x1, 0x015C0014);
> > > +   phy_write_reg(pcie, 1, 0x44, 0x1, 0x015C0014);
> > > +   phy_write_reg(pcie, 1, 0x4C, 0x1, 0x786174A0);
> > 
> > Have you considered moving this into a separate PHY driver?
> > There are probably good reasons either way, so I'm not asking you
> > to do it, just to think about what would be best for this driver.
> > 
> > It seems that you already have two different implementations
> > that only vary in how they access the PHY, so moving that
> > into a separate driver would keep the knowledge out of the
> > host driver. OTOH, the PHY registers look like they are part of
> > the pci host itself.
>
> The PHYs are separate entities, but access to them is via the PCIe 
> controller's registers. In the general case, there shouldn't be any PHY 
> setup to do, it's just the R-Car H1 devices that needs a little tweaking.

Ok.


> > Most importantly, why don't you use a mapping that includes RAM beyond
> > the first 4GB?
> I had intended to send a patch later on to open this up to the first 12GB. 
> Limitations of this block of hardware mean that we can do at most three 
> 4GB regions. The first 12GB would be ok for LPAE on existing devices as 
> all DDR is mapped within that range, but obviously not ideal.

Ok, in this case it sounds like you should definitely use the dma-ranges
property, since it's possibly that board designers put their ram in funny
places. You probably want to have a dma-ranges property with three 4GB
entries then and document the size limitation in the pci host binding,
rather than a single entry spanning all of RAM.

	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

On Fri, Mar 21, 2014 at 4:07 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 21 March 2014 13:59:35 Phil.Edworthy@renesas.com wrote:
>> > From: Arnd Bergmann <arnd@arndb.de>
>> > static const struct of_device_id rcar_pcie_of_match[] = {
>> >    { .compatible = "renesas,r8a7779-pcie", .data =
>> rcar_pcie_phy_init_rcar_h1},
>> >    { .compatible = "renesas,r8a7790-pcie", .data =  rcar_pcie_hw_init},
>> >    { .compatible = "renesas,r8a7791-pcie", .data =  rcar_pcie_hw_init},
>> >    {},
>> > };
>> >
>> > If you need other differences in the future, you can extend this
>> > to use a pointer to a struct containing the init function pointer.
>>
>> Ok, I understand what you mean, though the rcar_pcie_phy_init_rcar_h1
>> function is an extra call for r8a7779 devices to setup the PHY, not an
>> alternative to rcar_pcie_hw_init.
>
> I assumed that would be trivial to change, by calling rcar_pcie_hw_init
> at the end of rcar_pcie_phy_init_rcar_h1 rather than the other way round.

And this would allow to propagate the error code from
rcar_pcie_phy_init_rcar_h1(), as currently rcar_pcie_hw_init() ignores it,
and returns void.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
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