Re: [PATCH v3 5/6] ARM: davinci: Add support for PRUSS on DA850

[Matt Porter]

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 <mporter@xxxxxx>
> 
> 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 <linux/regulator/machine.h>
> >  #include <linux/regulator/tps6507x.h>
> >  #include <linux/input/tps6507x-ts.h>
> > +#include <linux/platform_data/uio_pruss.h>
> >  #include <linux/spi/spi.h>
> >  #include <linux/spi/flash.h>
> >  #include <linux/delay.h>
> > @@ -42,6 +43,7 @@
> >  #include <mach/da8xx.h>
> >  #include <linux/platform_data/mtd-davinci.h>
> >  #include <mach/mux.h>
> > +#include <mach/sram.h>
> >  #include <linux/platform_data/mtd-davinci-aemif.h>
> >  #include <linux/platform_data/spi-davinci.h>
> 
> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/