Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932369Ab2JDNHi (ORCPT ); Thu, 4 Oct 2012 09:07:38 -0400 Received: from mail-ia0-f174.google.com ([209.85.210.174]:51501 "EHLO mail-ia0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932084Ab2JDNHh (ORCPT ); Thu, 4 Oct 2012 09:07:37 -0400 Date: Thu, 4 Oct 2012 09:08:06 -0400 From: Matt Porter To: Sekhar Nori Cc: Linux DaVinci Kernel List , Russell King , Greg Kroah-Hartman , Linux Kernel Mailing List , "Hans J. Koch" , Linux ARM Kernel List Subject: Re: [PATCH v3 5/6] ARM: davinci: Add support for PRUSS on DA850 Message-ID: <20121004130806.GG11149@beef> References: <1349276133-26408-1-git-send-email-mporter@ti.com> <1349276133-26408-6-git-send-email-mporter@ti.com> <506D7F95.2030401@ti.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <506D7F95.2030401@ti.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7244 Lines: 208 On Thu, Oct 04, 2012 at 05:52:45PM +0530, Sekhar Nori wrote: > On 10/3/2012 8:25 PM, Matt Porter wrote: > > Adds PRUSS clock, registers the L3RAM pool, and registers the > > platform device for uio_pruss on DA850. > > > > Signed-off-by: Matt Porter > > I am interested in knowing how this patch was tested. I'll add similar test information in the commit that I mention in my other reply. Basically, I use the examples in the PRU package downloadable from ti.com. There's a PRU_memAccessL3andDDR example which will fail miserably if the uio_pruss driver SRAM resource is not configured properly. With the complete series in place, it's able to complete execution successfully, communicating via the shared sram pool. > > --- > > arch/arm/mach-davinci/board-da850-evm.c | 12 +++++ > > arch/arm/mach-davinci/da850.c | 7 +++ > > arch/arm/mach-davinci/devices-da8xx.c | 66 ++++++++++++++++++++++++++++ > > arch/arm/mach-davinci/include/mach/da8xx.h | 2 + > > 4 files changed, 87 insertions(+) > > > > diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c > > index 1295e61..acc6e84 100644 > > --- a/arch/arm/mach-davinci/board-da850-evm.c > > +++ b/arch/arm/mach-davinci/board-da850-evm.c > > @@ -29,6 +29,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -42,6 +43,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > I know you did not introduce the mess, but can you include a patch to > fix the mixture of mach/ and linux/ includes here? mach/ includes should > follow the linux/ includes. Ok. Yeah, I agree it's a mess there. > > @@ -1253,6 +1255,10 @@ static __init int da850_wl12xx_init(void) > > > > #endif /* CONFIG_DA850_WL12XX */ > > > > +struct uio_pruss_pdata da8xx_pruss_uio_pdata = { > > + .pintc_base = 0x4000, > > +}; > > + > > #define DA850EVM_SATA_REFCLKPN_RATE (100 * 1000 * 1000) > > > > static __init void da850_evm_init(void) > > @@ -1339,6 +1345,12 @@ static __init void da850_evm_init(void) > > pr_warning("da850_evm_init: lcdcntl mux setup failed: %d\n", > > ret); > > > > + da8xx_pruss_uio_pdata.l3ram_pool = sram_get_gen_pool(); > > I wonder if this platform data should still be called l3ram pool. L3RAM > is OMAP terminology. May be just call it sram_pool? Also this patch > should follow 6/6 to prevent build breakage. I agree, I'll change to sram_pool. I went back and forth on this because I found even the userspace PRU tools had it called L3...I think it's wise just say sram and avoid confusion. I'll move the patch too. > > + ret = da8xx_register_pruss_uio(&da8xx_pruss_uio_pdata); > > + if (ret) > > + pr_warning("pruss_uio initialization failed: %d\n", > > + ret); > > + > > /* Handle board specific muxing for LCD here */ > > ret = davinci_cfg_reg_list(da850_evm_lcdc_pins); > > if (ret) > > diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c > > index d8d69de..7c01d31 100644 > > --- a/arch/arm/mach-davinci/da850.c > > +++ b/arch/arm/mach-davinci/da850.c > > Can you separate out board and SoC changes into different patches? Ok, yes. > > @@ -212,6 +212,12 @@ static struct clk tptc2_clk = { > > .flags = ALWAYS_ENABLED, > > }; > > > > +static struct clk pruss_clk = { > > + .name = "pruss", > > + .parent = &pll0_sysclk2, > > + .lpsc = DA8XX_LPSC0_PRUSS, > > +}; > > + > > static struct clk uart0_clk = { > > .name = "uart0", > > .parent = &pll0_sysclk2, > > @@ -378,6 +384,7 @@ static struct clk_lookup da850_clks[] = { > > CLK(NULL, "tptc1", &tptc1_clk), > > CLK(NULL, "tpcc1", &tpcc1_clk), > > CLK(NULL, "tptc2", &tptc2_clk), > > + CLK(NULL, "pruss", &pruss_clk), > > This is actually incorrect since we should use device name rather than > con_id for matching the clock. If there is just one clock that the > driver needs, connection id should be NULL. Looking at the driver now, > the clk_get() call seems to pass a valid device pointer. So, I wonder > how you are able to look up the clock even with a NULL device name. Hrm, I'll look into this. This part was a not looked at closely, just a quick copy/paste/modify and worked as tested with the PRU examples so I didn't look further. I'll look again let you know why it actually works. I'm guessing I got lucky here, and should be a simple fix. > > CLK(NULL, "uart0", &uart0_clk), > > CLK(NULL, "uart1", &uart1_clk), > > CLK(NULL, "uart2", &uart2_clk), > > diff --git a/arch/arm/mach-davinci/devices-da8xx.c b/arch/arm/mach-davinci/devices-da8xx.c > > index bd2f72b..7c2e0d2 100644 > > --- a/arch/arm/mach-davinci/devices-da8xx.c > > +++ b/arch/arm/mach-davinci/devices-da8xx.c > > @@ -518,6 +518,72 @@ void __init da8xx_register_mcasp(int id, struct snd_platform_data *pdata) > > } > > } > > > > +#define DA8XX_PRUSS_MEM_BASE 0x01C30000 > > In this file all base addresses are added at the top of the file. The > defines are sorted in ascending order of address. If that's broken, its > all my fault, but please don't add to the breakage when adding this entry :) Ok :) Will fix. > > + > > +static struct resource da8xx_pruss_resources[] = { > > + { > > + .start = DA8XX_PRUSS_MEM_BASE, > > + .end = DA8XX_PRUSS_MEM_BASE + 0xFFFF, > > + .flags = IORESOURCE_MEM, > > + }, > > + { > > + .start = IRQ_DA8XX_EVTOUT0, > > + .end = IRQ_DA8XX_EVTOUT0, > > + .flags = IORESOURCE_IRQ, > > + }, > > + { > > + .start = IRQ_DA8XX_EVTOUT1, > > + .end = IRQ_DA8XX_EVTOUT1, > > + .flags = IORESOURCE_IRQ, > > + }, > > + { > > + .start = IRQ_DA8XX_EVTOUT2, > > + .end = IRQ_DA8XX_EVTOUT2, > > + .flags = IORESOURCE_IRQ, > > + }, > > + { > > + .start = IRQ_DA8XX_EVTOUT3, > > + .end = IRQ_DA8XX_EVTOUT3, > > + .flags = IORESOURCE_IRQ, > > + }, > > + { > > + .start = IRQ_DA8XX_EVTOUT4, > > + .end = IRQ_DA8XX_EVTOUT4, > > + .flags = IORESOURCE_IRQ, > > + }, > > + { > > + .start = IRQ_DA8XX_EVTOUT5, > > + .end = IRQ_DA8XX_EVTOUT5, > > + .flags = IORESOURCE_IRQ, > > + }, > > + { > > + .start = IRQ_DA8XX_EVTOUT6, > > + .end = IRQ_DA8XX_EVTOUT6, > > + .flags = IORESOURCE_IRQ, > > + }, > > + { > > + .start = IRQ_DA8XX_EVTOUT7, > > + .end = IRQ_DA8XX_EVTOUT7, > > + .flags = IORESOURCE_IRQ, > > + }, > > +}; > > + > > +static struct platform_device da8xx_pruss_uio_dev = { > > + .name = "pruss_uio", > > + .id = -1, > > + .num_resources = ARRAY_SIZE(da8xx_pruss_resources), > > + .resource = da8xx_pruss_resources, > > + .dev = { > > + .coherent_dma_mask = 0xffffffff, > > DMA_BIT_MASK(32)? Yeah, more copy/paste/modify stuff. I'll fix that. -Matt -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/