| Message ID | 3e9a2f384fd831fb2639ad99ec43ce0f019ac17c.1529333762.git.gustavo.pimentel@synopsys.com |
|---|---|
| State | New |
| Headers | show |
| Series | ARC: Add PCIe support for ARC HSDK platform | expand |
Hi Gustavo, On Mon, 2018-06-18 at 15:59 +0100, Gustavo Pimentel wrote: > Add PCI support to the ARC HSDK platform allowing to use the generic PCI > setup functions. This is the first logically independent change, so put it in a separate patch. > Add GPIO interrupt configuration function on ARC HSDK platform and > configures it to PCI support. That's the second change which is not directly tied to the first one so please put it in a separate patch. Than as compared to the first [very simple and obvious] change this one warrants more verbose justification. 1. IMHO it worth adding more info to commit message explaining what problem you're solving and why you do it in this particular way. In case of HSDK we have intermediate INTC in for of DW APB GPIO controller which is used as a de-bounce logic for interrupt wires that come from outside the board. We cannot use existing "irq-dw-apb-ictl" driver here because all input lines are routed to corresponding output lines but not muxed into one line (this is configured in RTL and we cannot change this in software). But even if we add such a feature to "irq-dw-apb-ictl" driver that won't benefit us as higher-level INTC (in case of HSDK it is IDU) anyways has per-input control so adding fully-controller intermediate INTC will only bring some overhead on interrupt processing but no other benefits. Thus we just do one-time configuration of DW APP GPIO controller and forget about it. > Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com> > --- > arch/arc/plat-hsdk/Kconfig | 1 + > arch/arc/plat-hsdk/platform.c | 41 +++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 42 insertions(+) > > diff --git a/arch/arc/plat-hsdk/Kconfig b/arch/arc/plat-hsdk/Kconfig > index 19ab3cf..556bc5e 100644 > --- a/arch/arc/plat-hsdk/Kconfig > +++ b/arch/arc/plat-hsdk/Kconfig > @@ -9,3 +9,4 @@ menuconfig ARC_SOC_HSDK > bool "ARC HS Development Kit SOC" > select CLK_HSDK > select RESET_HSDK > + select MIGHT_HAVE_PCI > diff --git a/arch/arc/plat-hsdk/platform.c b/arch/arc/plat-hsdk/platform.c > index 2958aed..31adda7 100644 > --- a/arch/arc/plat-hsdk/platform.c > +++ b/arch/arc/plat-hsdk/platform.c > @@ -42,6 +42,45 @@ static void __init hsdk_init_per_cpu(unsigned int cpu) > #define SDIO_UHS_REG_EXT (SDIO_BASE + 0x108) > #define SDIO_UHS_REG_EXT_DIV_2 (2 << 30) > > +#define HSDK_GPIO_INTC (ARC_PERIPHERAL_BASE + 0x3000) > +#define GPIO_INTEN (HSDK_GPIO_INTC + 0x30) > +#define GPIO_INTMASK (HSDK_GPIO_INTC + 0x34) > +#define GPIO_INTTYPE_LEVEL (HSDK_GPIO_INTC + 0x38) > +#define GPIO_INT_POLARITY (HSDK_GPIO_INTC + 0x3c) > + > +#define GPIO_BLUETOOTH_INT 0x00000001 > +#define GPIO_HAPS_INT 0x00000004 > +#define GPIO_AUDIO_INT 0x00000008 > +/* PMOD_A header */ > +#define GPIO_PIN_08_INT 0x00000100 > +#define GPIO_PIN_09_INT 0x00000200 > +#define GPIO_PIN_10_INT 0x00000400 > +#define GPIO_PIN_11_INT 0x00000800 > +/* PMOD_B header */ > +#define GPIO_PIN_12_INT 0x00001000 > +#define GPIO_PIN_13_INT 0x00002000 > +#define GPIO_PIN_14_INT 0x00004000 > +#define GPIO_PIN_15_INT 0x00008000 > +/* PMOD_C header */ > +#define GPIO_PIN_16_INT 0x00010000 > +#define GPIO_PIN_17_INT 0x00020000 > +#define GPIO_PIN_18_INT 0x00040000 > +#define GPIO_PIN_19_INT 0x00080000 > +#define GPIO_PIN_20_INT 0x00100000 > +#define GPIO_PIN_21_INT 0x00200000 > +#define GPIO_PIN_22_INT 0x00400000 > +#define GPIO_PIN_23_INT 0x00800000 Could you please run your patch through checkpatch.pl? Here I guss newline is missing. Also why adding those many defines if they are not really used anywhere? Instead maybe just add more informative pseudo-graphics as we have in axs10x platform? See https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arc/plat-axs10x/axs10x.c > +static void __init hsdk_enable_gpio_intc_wire(void) > +{ > + u32 val = GPIO_HAPS_INT; > + > + iowrite32(0xffffffff, (void __iomem *) GPIO_INTMASK); > + iowrite32(~val, (void __iomem *) GPIO_INTMASK); > + iowrite32(0x00000000, (void __iomem *) GPIO_INTTYPE_LEVEL); > + iowrite32(0xffffffff, (void __iomem *) GPIO_INT_POLARITY); > + iowrite32(val, (void __iomem *) GPIO_INTEN); > +} I would suggest to have a map of really used lines and enable all of them instead of adding one-by-one occasionally. -Alexey
On 06/18/2018 08:20 AM, Alexey Brodkin wrote: >> +static void __init hsdk_enable_gpio_intc_wire(void) >> +{ >> + u32 val = GPIO_HAPS_INT; >> + >> + iowrite32(0xffffffff, (void __iomem *) GPIO_INTMASK); >> + iowrite32(~val, (void __iomem *) GPIO_INTMASK); >> + iowrite32(0x00000000, (void __iomem *) GPIO_INTTYPE_LEVEL); >> + iowrite32(0xffffffff, (void __iomem *) GPIO_INT_POLARITY); >> + iowrite32(val, (void __iomem *) GPIO_INTEN); >> +} > I would suggest to have a map of really used lines and enable all of them > instead of adding one-by-one occasionally. More importantly, adding any code in this file is an absolute abomination and only desired if this is a platform specific hack which can't be added in the generic driver and/or specified via the Device Tree. Here it seems like we are enabling some gpio lines which likely could be done via the gpio driver paths ? -Vineet
Hi Vineet, > -----Original Message----- > From: Vineet Gupta > Sent: Monday, June 18, 2018 9:11 PM > To: Alexey Brodkin <abrodkin@synopsys.com>; gustavo.pimentel@synopsys.com > Cc: robh@kernel.org; linux-kernel@vger.kernel.org; Eugeniy.Paltsev@synopsys.com; sboyd@codeaurora.org; linux-snps- > arc@lists.infradead.org > Subject: Re: [PATCH] ARC: Add PCIe support for ARC HSDK platform > > On 06/18/2018 08:20 AM, Alexey Brodkin wrote: > >> +static void __init hsdk_enable_gpio_intc_wire(void) > >> +{ > >> + u32 val = GPIO_HAPS_INT; > >> + > >> + iowrite32(0xffffffff, (void __iomem *) GPIO_INTMASK); > >> + iowrite32(~val, (void __iomem *) GPIO_INTMASK); > >> + iowrite32(0x00000000, (void __iomem *) GPIO_INTTYPE_LEVEL); > >> + iowrite32(0xffffffff, (void __iomem *) GPIO_INT_POLARITY); > >> + iowrite32(val, (void __iomem *) GPIO_INTEN); > >> +} > > I would suggest to have a map of really used lines and enable all of them > > instead of adding one-by-one occasionally. > > More importantly, adding any code in this file is an absolute abomination and only > desired if this is a platform specific hack which can't be added in the generic > driver and/or specified via the Device Tree. Here it seems like we are enabling > some gpio lines which likely could be done via the gpio driver paths ? See my comment much higher - what we're doing here is pretty much the same as With AXS10x - we implement wires which connect input interrupt lines to IDU. Thus I'm not sure if it worth messing with DW APB GPIO INTC driver which as I mentioned As well won't work as it is and we'll need to patch it to support multiple outputs mode. -Alexey
diff --git a/arch/arc/plat-hsdk/Kconfig b/arch/arc/plat-hsdk/Kconfig index 19ab3cf..556bc5e 100644 --- a/arch/arc/plat-hsdk/Kconfig +++ b/arch/arc/plat-hsdk/Kconfig @@ -9,3 +9,4 @@ menuconfig ARC_SOC_HSDK bool "ARC HS Development Kit SOC" select CLK_HSDK select RESET_HSDK + select MIGHT_HAVE_PCI diff --git a/arch/arc/plat-hsdk/platform.c b/arch/arc/plat-hsdk/platform.c index 2958aed..31adda7 100644 --- a/arch/arc/plat-hsdk/platform.c +++ b/arch/arc/plat-hsdk/platform.c @@ -42,6 +42,45 @@ static void __init hsdk_init_per_cpu(unsigned int cpu) #define SDIO_UHS_REG_EXT (SDIO_BASE + 0x108) #define SDIO_UHS_REG_EXT_DIV_2 (2 << 30) +#define HSDK_GPIO_INTC (ARC_PERIPHERAL_BASE + 0x3000) +#define GPIO_INTEN (HSDK_GPIO_INTC + 0x30) +#define GPIO_INTMASK (HSDK_GPIO_INTC + 0x34) +#define GPIO_INTTYPE_LEVEL (HSDK_GPIO_INTC + 0x38) +#define GPIO_INT_POLARITY (HSDK_GPIO_INTC + 0x3c) + +#define GPIO_BLUETOOTH_INT 0x00000001 +#define GPIO_HAPS_INT 0x00000004 +#define GPIO_AUDIO_INT 0x00000008 +/* PMOD_A header */ +#define GPIO_PIN_08_INT 0x00000100 +#define GPIO_PIN_09_INT 0x00000200 +#define GPIO_PIN_10_INT 0x00000400 +#define GPIO_PIN_11_INT 0x00000800 +/* PMOD_B header */ +#define GPIO_PIN_12_INT 0x00001000 +#define GPIO_PIN_13_INT 0x00002000 +#define GPIO_PIN_14_INT 0x00004000 +#define GPIO_PIN_15_INT 0x00008000 +/* PMOD_C header */ +#define GPIO_PIN_16_INT 0x00010000 +#define GPIO_PIN_17_INT 0x00020000 +#define GPIO_PIN_18_INT 0x00040000 +#define GPIO_PIN_19_INT 0x00080000 +#define GPIO_PIN_20_INT 0x00100000 +#define GPIO_PIN_21_INT 0x00200000 +#define GPIO_PIN_22_INT 0x00400000 +#define GPIO_PIN_23_INT 0x00800000 +static void __init hsdk_enable_gpio_intc_wire(void) +{ + u32 val = GPIO_HAPS_INT; + + iowrite32(0xffffffff, (void __iomem *) GPIO_INTMASK); + iowrite32(~val, (void __iomem *) GPIO_INTMASK); + iowrite32(0x00000000, (void __iomem *) GPIO_INTTYPE_LEVEL); + iowrite32(0xffffffff, (void __iomem *) GPIO_INT_POLARITY); + iowrite32(val, (void __iomem *) GPIO_INTEN); +} + static void __init hsdk_init_early(void) { /* @@ -62,6 +101,8 @@ static void __init hsdk_init_early(void) * minimum possible div-by-2. */ iowrite32(SDIO_UHS_REG_EXT_DIV_2, (void __iomem *) SDIO_UHS_REG_EXT); + + sdk_enable_gpio_intc_wire(); } static const char *hsdk_compat[] __initconst = {
Add PCI support to the ARC HSDK platform allowing to use the generic PCI setup functions. Add GPIO interrupt configuration function on ARC HSDK platform and configures it to PCI support. Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com> --- arch/arc/plat-hsdk/Kconfig | 1 + arch/arc/plat-hsdk/platform.c | 41 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+)