Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1451488 > unrolled thread

Re: [PATCH v7 2/2] PCI: Rockchip: Add Rockchip PCIe controller support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v7 2/2] PCI: Rockchip: Add Rockchip PCIe controller support Paul Gortmaker <paul.gortmaker@windriver.com> - 2016-07-27 22:00 +0200
    Re: [PATCH v7 2/2] PCI: Rockchip: Add Rockchip PCIe controller  support Brian Norris <briannorris@chromium.org> - 2016-07-27 22:10 +0200
      Re: [PATCH v7 2/2] PCI: Rockchip: Add Rockchip PCIe controller  support Paul Gortmaker <paul.gortmaker@windriver.com> - 2016-07-28 00:00 +0200

#1451488 — Re: [PATCH v7 2/2] PCI: Rockchip: Add Rockchip PCIe controller support

On Wed, Jul 27, 2016 at 2:22 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> Hi Shawn,
>
> I have some relatively minor comments below.
>
> On Mon, Jul 18, 2016 at 08:42:13AM +0800, Shawn Lin wrote:
>> This patch adds Rockchip PCIe controller support found
>> on RK3399 Soc platform.
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>
>> ---
>>
>> Changes in v7:
>> - make it as a build-in driver

[...]

>> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
>> index c917e2f..4d714d2 100644
>> --- a/drivers/pci/host/Kconfig
>> +++ b/drivers/pci/host/Kconfig
>> @@ -264,4 +264,15 @@ config PCIE_ARTPEC6
>>         Say Y here to enable PCIe controller support on Axis ARTPEC-6
>>         SoCs.  This PCIe controller uses the DesignWare core.
>>
>> +config PCIE_ROCKCHIP
>> +     bool "Rockchip PCIe controller"
>> +     depends on ARM64 && ARCH_ROCKCHIP
>> +     depends on OF
>> +     depends on PCI_MSI_IRQ_DOMAIN
>> +     select MFD_SYSCON
>> +     help
>> +       Say Y here if you want internal PCI support on Rockchip SoC.
>> +       There is 1 internal PCIe port available to support GEN2 with
>> +       4 slots.
>> +
>>  endmenu
>> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
>> index 5bc0af2..695f716 100644
>> --- a/drivers/pci/host/Makefile
>> +++ b/drivers/pci/host/Makefile
>> @@ -30,3 +30,4 @@ obj-$(CONFIG_PCI_HOST_THUNDER_ECAM) += pci-thunder-ecam.o
>>  obj-$(CONFIG_PCI_HOST_THUNDER_PEM) += pci-thunder-pem.o
>>  obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
>>  obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
>> +obj-$(CONFIG_PCIE_ROCKCHIP) += pcie-rockchip.o
>> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
>> new file mode 100644
>> index 0000000..8a1fef1
>> --- /dev/null
>> +++ b/drivers/pci/host/pcie-rockchip.c
>> @@ -0,0 +1,1233 @@
>> +/*
>> + * Rockchip AXI PCIe host controller driver
>> + *
>> + * Copyright (c) 2016 Rockchip, Inc.
>> + *
>> + * Author: Shawn Lin <shawn.lin@rock-chips.com>
>> + *         Wenrui Li <wenrui.li@rock-chips.com>
>> + *
>> + * Bits taken from Synopsys Designware Host controller driver and
>> + * ARM PCI Host generic driver.
>> + *
>> + * This program is free software: you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation, either version 2 of the License, or
>> + * (at your option) any later version.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>> +#include <linux/irqchip/chained_irq.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/module.h>

[...]

>> +static struct platform_driver rockchip_pcie_driver = {
>> +     .driver = {
>> +             .name = "rockchip-pcie",
>> +             .of_match_table = rockchip_pcie_of_match,
>> +     },
>> +     .probe = rockchip_pcie_probe,
>> +
>> +};
>> +module_platform_driver(rockchip_pcie_driver);
>> +
>> +MODULE_AUTHOR("Rockchip Inc");
>> +MODULE_DESCRIPTION("Rockchip AXI PCIe driver");
>> +MODULE_LICENSE("GPL v2");
>
> Per your Kconfig, this driver cannot be a module, so remove these MODULE_*
> annotations.  This is to follow Paul Gortmaker's recent "Make explicitly
> non-modular" work.  Also change the include of <linux/module.h> to
> <linux/init.h>.

Thanks!  You beat me to it.    I didn't see any EXPORT_SYMBOL so it doesn't
look like we need export.h  added here.   Also, at the risk of stating
the obvious,
the module_platform_driver becomes builtin_platform_driver.

Paul.

[toc] | [next] | [standalone]

#1451489 — Re: [PATCH v7 2/2] PCI: Rockchip: Add Rockchip PCIe controller support

+ Guenter

Hi,

On Wed, Jul 27, 2016 at 03:56:36PM -0400, Paul Gortmaker wrote:
> On Wed, Jul 27, 2016 at 2:22 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Mon, Jul 18, 2016 at 08:42:13AM +0800, Shawn Lin wrote:
> >> This patch adds Rockchip PCIe controller support found
> >> on RK3399 Soc platform.
> >>
> >> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> >>
> >> ---
> >>
> >> Changes in v7:
> >> - make it as a build-in driver
> 
> [...]
> 

...

> [...]
> 
> >> +static struct platform_driver rockchip_pcie_driver = {
> >> +     .driver = {
> >> +             .name = "rockchip-pcie",
> >> +             .of_match_table = rockchip_pcie_of_match,
> >> +     },
> >> +     .probe = rockchip_pcie_probe,
> >> +
> >> +};
> >> +module_platform_driver(rockchip_pcie_driver);
> >> +
> >> +MODULE_AUTHOR("Rockchip Inc");
> >> +MODULE_DESCRIPTION("Rockchip AXI PCIe driver");
> >> +MODULE_LICENSE("GPL v2");
> >
> > Per your Kconfig, this driver cannot be a module, so remove these MODULE_*
> > annotations.  This is to follow Paul Gortmaker's recent "Make explicitly
> > non-modular" work.  Also change the include of <linux/module.h> to
> > <linux/init.h>.
> 
> Thanks!  You beat me to it.    I didn't see any EXPORT_SYMBOL so it doesn't
> look like we need export.h  added here.   Also, at the risk of stating
> the obvious,
> the module_platform_driver becomes builtin_platform_driver.

Just for reference (not to necessarily disagree here): we've kinda
ping-ponged on this one. I suggested it could be built as a module, so
Shawn added it, but apparently we're missing an export:

https://lkml.org/lkml/2016/7/8/704

so he reverted back. It's probably best to just leave this built-in and
resolve the comments Paul and Bjorn made, to avoid too much more
flip-flopping. If we still want to patch up the __weak
pci_remap_iospace(), then we can make this a module at a later time.

Regards,
Brian

[toc] | [prev] | [next] | [standalone]

#1451537 — Re: [PATCH v7 2/2] PCI: Rockchip: Add Rockchip PCIe controller support

[Re: [PATCH v7 2/2] PCI: Rockchip: Add Rockchip PCIe controller support] On 27/07/2016 (Wed 13:04) Brian Norris wrote:

> + Guenter
> 
> Hi,
> 
> On Wed, Jul 27, 2016 at 03:56:36PM -0400, Paul Gortmaker wrote:
> > On Wed, Jul 27, 2016 at 2:22 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Mon, Jul 18, 2016 at 08:42:13AM +0800, Shawn Lin wrote:
> > >> This patch adds Rockchip PCIe controller support found
> > >> on RK3399 Soc platform.
> > >>
> > >> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> > >>
> > >> ---
> > >>
> > >> Changes in v7:
> > >> - make it as a build-in driver
> > 
> > [...]
> > 
> 
> ...
> 
> > [...]
> > 
> > >> +static struct platform_driver rockchip_pcie_driver = {
> > >> +     .driver = {
> > >> +             .name = "rockchip-pcie",
> > >> +             .of_match_table = rockchip_pcie_of_match,
> > >> +     },
> > >> +     .probe = rockchip_pcie_probe,
> > >> +
> > >> +};
> > >> +module_platform_driver(rockchip_pcie_driver);
> > >> +
> > >> +MODULE_AUTHOR("Rockchip Inc");
> > >> +MODULE_DESCRIPTION("Rockchip AXI PCIe driver");
> > >> +MODULE_LICENSE("GPL v2");
> > >
> > > Per your Kconfig, this driver cannot be a module, so remove these MODULE_*
> > > annotations.  This is to follow Paul Gortmaker's recent "Make explicitly
> > > non-modular" work.  Also change the include of <linux/module.h> to
> > > <linux/init.h>.
> > 
> > Thanks!  You beat me to it.    I didn't see any EXPORT_SYMBOL so it doesn't
> > look like we need export.h  added here.   Also, at the risk of stating
> > the obvious,
> > the module_platform_driver becomes builtin_platform_driver.
> 
> Just for reference (not to necessarily disagree here): we've kinda
> ping-ponged on this one. I suggested it could be built as a module, so
> Shawn added it, but apparently we're missing an export:
> 
> https://lkml.org/lkml/2016/7/8/704
> 
> so he reverted back. It's probably best to just leave this built-in and
> resolve the comments Paul and Bjorn made, to avoid too much more
> flip-flopping. If we still want to patch up the __weak
> pci_remap_iospace(), then we can make this a module at a later time.

Agreed, and this was the general consensus we came to for other PCI code
as well.   It might be different if we were deleting hundreds of lines
of code, but we are just making a few simple changes to make it clear
that things aren't modular, and that ensures others don't continue to
copy the same inconsistency into their new driver(s).  It doesn't in any
way prevent a future conversion to tristate if that use case makes sense.

THanks,
Paul.
--

> 
> Regards,
> Brian

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web