| Message ID | 1308640714-17961-6-git-send-email-ohad@wizery.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
Hello. Ohad Ben-Cohen wrote: > From: Mark Grosen <mgrosen@ti.com> > Add remoteproc implementation for da850, so we can boot its DSP. > Signed-off-by: Mark Grosen <mgrosen@ti.com> > Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com> > --- > arch/arm/mach-davinci/include/mach/remoteproc.h | 28 +++++ > drivers/remoteproc/Kconfig | 16 +++ > drivers/remoteproc/Makefile | 1 + > drivers/remoteproc/davinci_remoteproc.c | 147 +++++++++++++++++++++++ > 4 files changed, 192 insertions(+), 0 deletions(-) > create mode 100644 arch/arm/mach-davinci/include/mach/remoteproc.h > create mode 100644 drivers/remoteproc/davinci_remoteproc.c > diff --git a/arch/arm/mach-davinci/include/mach/remoteproc.h b/arch/arm/mach-davinci/include/mach/remoteproc.h > new file mode 100644 > index 0000000..af6e88c > --- /dev/null > +++ b/arch/arm/mach-davinci/include/mach/remoteproc.h > @@ -0,0 +1,28 @@ > +/* > + * Remote Processor > + * > + * Copyright (C) 2011 Texas Instruments, Inc. > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License > + * version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#ifndef _DAVINCI_REMOTEPROC_H > +#define _DAVINCI_REMOTEPROC_H > + > +#include <linux/remoteproc.h> > + > +struct davinci_rproc_pdata { > + struct rproc_ops *ops; > + char *name; > + char *clk_name; > + char *firmware; > +}; > + > +#endif /* _DAVINCI_REMOTEPROC_H */ > diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig > index 88421bd..1e594b5 100644 > --- a/drivers/remoteproc/Kconfig > +++ b/drivers/remoteproc/Kconfig > @@ -26,3 +26,19 @@ config OMAP_REMOTE_PROC > > It's safe to say n here if you're not interested in multimedia > offloading or just want a bare minimum kernel. > + > +config DAVINCI_REMOTE_PROC > + tristate "Davinci remoteproc support" > + depends on ARCH_DAVINCI_DA850 It should work on DA830 as well, but not on real DaVinci, so the name is misleading... [...] > diff --git a/drivers/remoteproc/davinci_remoteproc.c b/drivers/remoteproc/davinci_remoteproc.c > new file mode 100644 > index 0000000..0e26fe9 > --- /dev/null > +++ b/drivers/remoteproc/davinci_remoteproc.c > @@ -0,0 +1,147 @@ > +/* > + * Remote processor machine-specific module for Davinci > + * > + * Copyright (C) 2011 Texas Instruments, Inc. > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License > + * version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#define pr_fmt(fmt) "%s: " fmt, __func__ > + > +#include <linux/kernel.h> > +#include <linux/err.h> > +#include <linux/printk.h> > +#include <linux/bitops.h> > +#include <linux/platform_device.h> > +#include <linux/remoteproc.h> > +#include <linux/clk.h> > +#include <linux/io.h> > + > +#include <mach/da8xx.h> > +#include <mach/cputype.h> > +#include <mach/psc.h> > +#include <mach/remoteproc.h> > + > +/* > + * Technical Reference: > + * OMAP-L138 Applications Processor System Reference Guide > + * http://www.ti.com/litv/pdf/sprugm7d > + */ > + > +/* local reset bit (0 is asserted) in MDCTL15 register (section 9.6.18) */ > +#define LRST BIT(8) Perhaps this should be named nLRST or something if the sense is inverted? > +/* next state bits in MDCTL15 register (section 9.6.18) */ > +#define NEXT_ENABLED 0x3 Isn't this already declared in <mach/psc.h> as PSC_STATE_ENABLED? > +/* register for DSP boot address in SYSCFG0 module (section 11.5.6) */ > +#define HOST1CFG 0x44 Worth declaring in <mach/da8xx.h> instead... > +static inline int davinci_rproc_start(struct rproc *rproc, u64 bootaddr) > +{ > + struct device *dev = rproc->dev; > + struct davinci_rproc_pdata *pdata = dev->platform_data; > + struct davinci_soc_info *soc_info = &davinci_soc_info; > + void __iomem *psc_base; > + struct clk *dsp_clk; > + > + /* hw requires the start (boot) address be on 1KB boundary */ > + if (bootaddr & 0x3ff) { > + dev_err(dev, "invalid boot address: must be aligned to 1KB\n"); > + return -EINVAL; > + } > + > + dsp_clk = clk_get(dev, pdata->clk_name); We could match using clkdev functionality, but the clock entry would need to be changed then... > + if (IS_ERR_OR_NULL(dsp_clk)) { > + dev_err(dev, "clk_get error: %ld\n", PTR_ERR(dsp_clk)); > + return PTR_ERR(dsp_clk); > + } > + > + clk_enable(dsp_clk); This seems rather senseless activity as on DA8xx the DSP core boots the ARM core, so the DSP clock will be already enabled. > + rproc->priv = dsp_clk; > + > + psc_base = ioremap(soc_info->psc_bases[0], SZ_4K); > + > + /* insure local reset is asserted before writing start address */ > + __raw_writel(NEXT_ENABLED, psc_base + MDCTL + 4 * DA8XX_LPSC0_GEM); > + > + __raw_writel(bootaddr, DA8XX_SYSCFG0_VIRT(HOST1CFG)); DA8XX_SYSCFG0_VIRT() is not supposed to be used outside mach-davinci. The variable it refers is not exported, so driver module won't work. > + /* de-assert local reset to start the dsp running */ > + __raw_writel(LRST | NEXT_ENABLED, psc_base + MDCTL + > + 4 * DA8XX_LPSC0_GEM); > + > + iounmap(psc_base); > + > + return 0; > +} > + > +static inline int davinci_rproc_stop(struct rproc *rproc) > +{ > + struct davinci_soc_info *soc_info = &davinci_soc_info; > + void __iomem *psc_base; > + struct clk *dsp_clk = rproc->priv; > + > + psc_base = ioremap(soc_info->psc_bases[0], SZ_4K); > + > + /* halt the dsp by asserting local reset */ > + __raw_writel(NEXT_ENABLED, psc_base + MDCTL + 4 * DA8XX_LPSC0_GEM); > + > + clk_disable(dsp_clk); > + clk_put(dsp_clk); > + > + iounmap(psc_base); > + > + return 0; > +} All this is DA8xx specific code which won't fly on real DaVincis, so I suggest that you rename the file to da8xx_remoteproc.c for clarity; and rename the patch as well... WBR, Sergei
> From: Sergei Shtylyov > Sent: Thursday, June 23, 2011 8:28 AM > Subject: Re: [RFC 5/8] remoteproc: add davinci implementation > > Hello. Sergei, thanks for the feedback. Comments below. Mark > > It should work on DA830 as well, but not on real DaVinci, so the > name is misleading... Yes, we debated calling it da8xx, but felt that with minor changes it could accomodate the other SoCs in the davinci family. However, it may be better to start with just the da8xx/omapl13x parts and then rename if we add the others. > [...] > > + > > +/* > > + * Technical Reference: > > + * OMAP-L138 Applications Processor System Reference Guide > > + * http://www.ti.com/litv/pdf/sprugm7d > > + */ > > + > > +/* local reset bit (0 is asserted) in MDCTL15 register (section > 9.6.18) */ > > +#define LRST BIT(8) > > Perhaps this should be named nLRST or something if the sense is inverted? If there is an established naming convention for this, I'll adopt it. > > > +/* next state bits in MDCTL15 register (section 9.6.18) */ > > +#define NEXT_ENABLED 0x3 > > Isn't this already declared in <mach/psc.h> as PSC_STATE_ENABLED? Yes, thanks, I missed it. > > +/* register for DSP boot address in SYSCFG0 module (section 11.5.6) > */ > > +#define HOST1CFG 0x44 > > Worth declaring in <mach/da8xx.h> instead... Possibly - since it is only used for the DSP, I thought it would be better to keep local to this implementation. I'll adopt whichever approach is the convention. > > +static inline int davinci_rproc_start(struct rproc *rproc, u64 > bootaddr) > > +{ > > + struct device *dev = rproc->dev; > > + struct davinci_rproc_pdata *pdata = dev->platform_data; > > + struct davinci_soc_info *soc_info = &davinci_soc_info; > > + void __iomem *psc_base; > > + struct clk *dsp_clk; > > + > > + /* hw requires the start (boot) address be on 1KB boundary */ > > + if (bootaddr & 0x3ff) { > > + dev_err(dev, "invalid boot address: must be aligned to > 1KB\n"); > > + return -EINVAL; > > + } > > + > > + dsp_clk = clk_get(dev, pdata->clk_name); > > We could match using clkdev functionality, but the clock entry > would need to be changed then... I followed the existing pattern I saw in other drivers. If there is a new, better way, please point me to an example. > > > + if (IS_ERR_OR_NULL(dsp_clk)) { > > + dev_err(dev, "clk_get error: %ld\n", PTR_ERR(dsp_clk)); > > + return PTR_ERR(dsp_clk); > > + } > > + > > + clk_enable(dsp_clk); > > This seems rather senseless activity as on DA8xx the DSP core > boots the ARM core, so the DSP clock will be already enabled. I think it is needed. It's true that the DSP initiates the boot, but then it is reset and the clock disabled. See Section 13.2 of http://focus.ti.com/lit/ug/sprugm7e/sprugm7e.pdf: 13.2 DSP Wake Up Following deassertion of device reset, the DSP intializes the ARM296 so that it can execute the ARM ROM bootloader. Upon successful wake up, the ARM places the DSP in a reset and clock gated (SwRstDisable) state that is controlled by the LPSC and the SYSCFG modules. Besides, the boot loader could have disabled it to save power. The ARM and DSP are clocked independently, so I think it's best to use clock management. > > + rproc->priv = dsp_clk; > > + > > + psc_base = ioremap(soc_info->psc_bases[0], SZ_4K); > > + > > + /* insure local reset is asserted before writing start address */ > > + __raw_writel(NEXT_ENABLED, psc_base + MDCTL + 4 * > DA8XX_LPSC0_GEM); > > + > > + __raw_writel(bootaddr, DA8XX_SYSCFG0_VIRT(HOST1CFG)); > > DA8XX_SYSCFG0_VIRT() is not supposed to be used outside mach-davinci. The > variable it refers is not exported, so driver module won't work. Ooops, I clearly did not build this as a module. Suggestion how to fix this? > > + /* de-assert local reset to start the dsp running */ > > + __raw_writel(LRST | NEXT_ENABLED, psc_base + MDCTL + > > + 4 * DA8XX_LPSC0_GEM); > > + > > + iounmap(psc_base); > > + > > + return 0; > > +} > > + > > +static inline int davinci_rproc_stop(struct rproc *rproc) > > +{ > > + struct davinci_soc_info *soc_info = &davinci_soc_info; > > + void __iomem *psc_base; > > + struct clk *dsp_clk = rproc->priv; > > + > > + psc_base = ioremap(soc_info->psc_bases[0], SZ_4K); > > + > > + /* halt the dsp by asserting local reset */ > > + __raw_writel(NEXT_ENABLED, psc_base + MDCTL + 4 * > DA8XX_LPSC0_GEM); > > + > > + clk_disable(dsp_clk); > > + clk_put(dsp_clk); > > + > > + iounmap(psc_base); > > + > > + return 0; > > +} > > All this is DA8xx specific code which won't fly on real DaVincis, so I > suggest that you rename the file to da8xx_remoteproc.c for clarity; and > rename the patch as well... This is probably the right thing to do ... Mark
Hello. Grosen, Mark wrote: >> It should work on DA830 as well, So please make it dependent on ARCH_DAVINCI_DA8XX. >> but not on real DaVinci, so the name is misleading... > Yes, we debated calling it da8xx, but felt that with minor changes it could > accomodate the other SoCs in the davinci family. I don't think it's a good idea. Using cpu_is_*() is drivers is bad. Using #ifdef's is not an option either. > However, it may be better > to start with just the da8xx/omapl13x parts and then rename if we add the > others. >> [...] >>> + >>> +/* >>> + * Technical Reference: >>> + * OMAP-L138 Applications Processor System Reference Guide >>> + * http://www.ti.com/litv/pdf/sprugm7d >>> + */ >>> + >>> +/* local reset bit (0 is asserted) in MDCTL15 register (section >> 9.6.18) */ >>> +#define LRST BIT(8) >> Perhaps this should be named nLRST or something if the sense is inverted? > If there is an established naming convention for this, I'll adopt it. Looking into my old PSC manual (can't get the recent documentation from TI's site right now), the bit is called LRSTz. It's worth moving this #define into <mach/psc.h> as well. >>> +/* register for DSP boot address in SYSCFG0 module (section 11.5.6) >> */ >>> +#define HOST1CFG 0x44 >> Worth declaring in <mach/da8xx.h> instead... > Possibly - since it is only used for the DSP, I thought it would be better > to keep local to this implementation. I'll adopt whichever approach is the > convention. Well, the general approach is to keep the #define's where they are used, so maybe we should keep this #define here. >>> +static inline int davinci_rproc_start(struct rproc *rproc, u64 >> bootaddr) >>> +{ >>> + struct device *dev = rproc->dev; >>> + struct davinci_rproc_pdata *pdata = dev->platform_data; >>> + struct davinci_soc_info *soc_info = &davinci_soc_info; >>> + void __iomem *psc_base; >>> + struct clk *dsp_clk; >>> + >>> + /* hw requires the start (boot) address be on 1KB boundary */ >>> + if (bootaddr & 0x3ff) { >>> + dev_err(dev, "invalid boot address: must be aligned to >> 1KB\n"); >>> + return -EINVAL; >>> + } >>> + >>> + dsp_clk = clk_get(dev, pdata->clk_name); >> We could match using clkdev functionality, but the clock entry >> would need to be changed then... > I followed the existing pattern I saw in other drivers. Probably MUSB? We're trying to move away from passing the clock name to thge drivers, using match by device instead. > If there is a new, better way, please point me to an example. Look at the da850_clks[] in mach-davinci/da850.c: if the device name is specified as a first argument to CLK() macro (instead of clock name the second argument), the matching is done by device, so you don't need to specify the clock name to clk_get(), passing NULL instead. >>> + if (IS_ERR_OR_NULL(dsp_clk)) { >>> + dev_err(dev, "clk_get error: %ld\n", PTR_ERR(dsp_clk)); >>> + return PTR_ERR(dsp_clk); >>> + } >>> + >>> + clk_enable(dsp_clk); >> This seems rather senseless activity as on DA8xx the DSP core >> boots the ARM core, so the DSP clock will be already enabled. > I think it is needed. It's true that the DSP initiates the boot, but then it is > reset and the clock disabled. See Section 13.2 of Hm, didn't know that. Contrarywise, we had to work around the races between ARM and DSP cores on accessing locked SYSCFG registers -- in kernel. So I was under impression that DSP code continues running some stuff of its own. > http://focus.ti.com/lit/ug/sprugm7e/sprugm7e.pdf: > 13.2 DSP Wake Up > Following deassertion of device reset, the DSP intializes the ARM296 so that > it can execute the ARM ROM bootloader. Upon successful wake up, the ARM > places the DSP in a reset and clock gated (SwRstDisable) state that is > controlled by the LPSC and the SYSCFG modules. > Besides, the boot loader could have disabled it to save power. The ARM and > DSP are clocked independently, so I think it's best to use clock management. OK, agreed. >>> + rproc->priv = dsp_clk; >>> + >>> + psc_base = ioremap(soc_info->psc_bases[0], SZ_4K); >>> + >>> + /* insure local reset is asserted before writing start address */ >>> + __raw_writel(NEXT_ENABLED, psc_base + MDCTL + 4 * >> DA8XX_LPSC0_GEM); >>> + >>> + __raw_writel(bootaddr, DA8XX_SYSCFG0_VIRT(HOST1CFG)); >> DA8XX_SYSCFG0_VIRT() is not supposed to be used outside mach-davinci. The >> variable it refers is not exported, so driver module won't work. > Ooops, I clearly did not build this as a module. Suggestion how to fix this? Using the normal ioremap() of SYSCFG0 space, I suppose. > Mark WBR, Sergei
Hi Mark, On Fri, Jun 24, 2011 at 20:43:50, Sergei Shtylyov wrote: > >>> + rproc->priv = dsp_clk; > >>> + > >>> + psc_base = ioremap(soc_info->psc_bases[0], SZ_4K); > >>> + > >>> + /* insure local reset is asserted before writing start address */ > >>> + __raw_writel(NEXT_ENABLED, psc_base + MDCTL + 4 * > >> DA8XX_LPSC0_GEM); > >>> + > >>> + __raw_writel(bootaddr, DA8XX_SYSCFG0_VIRT(HOST1CFG)); > > >> DA8XX_SYSCFG0_VIRT() is not supposed to be used outside mach-davinci. The > >> variable it refers is not exported, so driver module won't work. > > > Ooops, I clearly did not build this as a module. Suggestion how to fix this? > > Using the normal ioremap() of SYSCFG0 space, I suppose. Since procedure to set the boot address varies across DaVinci platforms, you could have a callback populated in platform data which will be implemented differently for original DaVinci and DA8xx devices. Also, all PSC accesses are better off going through clock framework to ensure proper locking and modularity. To assert/de-assert local reset when enabling or disabling PSC, you could use a flag in the clock structure to indicate the need for this. This way, if there is any other module needing a local reset, it can just define the same flag. Similarly, if the DSP does not need a local reset on a particular platform, that platform can simply skip the flag. This can be done in a manner similar to how the support for a forced transition PSC was added here: https://patchwork.kernel.org/patch/662941/ Thanks, Sekhar
> From: Sergei Shtylyov > Sent: Friday, June 24, 2011 8:14 AM > > Grosen, Mark wrote: > > >> It should work on DA830 as well, > > So please make it dependent on ARCH_DAVINCI_DA8XX. > > >> but not on real DaVinci, so the name is misleading... > > > Yes, we debated calling it da8xx, but felt that with minor changes it could > > accomodate the other SoCs in the davinci family. > > I don't think it's a good idea. Using cpu_is_*() is drivers is bad. Using > #ifdef's is not an option either. > > > However, it may be better > > to start with just the da8xx/omapl13x parts and then rename if we add the > > others. Sergei, I'll respond more to this in a response to Sekhar's ideas. We may be able to make this work generically for davinci w/o idef's. > Looking into my old PSC manual (can't get the recent documentation from TI's > site right now), the bit is called LRSTz. > It's worth moving this #define into <mach/psc.h> as well. Ok, I agree we should try to match the HW names as much as possible > >>> +/* register for DSP boot address in SYSCFG0 module (section 11.5.6) */ > >>> +#define HOST1CFG 0x44 > > >> Worth declaring in <mach/da8xx.h> instead... > > > Possibly - since it is only used for the DSP, I thought it would be better > > to keep local to this implementation. I'll adopt whichever approach is the > > convention. > > Well, the general approach is to keep the #define's where they are > used, so maybe we should keep this #define here. Will review as part of the general cleanup. > > >>> +static inline int davinci_rproc_start(struct rproc *rproc, u64 > >> bootaddr) > >>> +{ > >>> + struct device *dev = rproc->dev; > >>> + struct davinci_rproc_pdata *pdata = dev->platform_data; > >>> + struct davinci_soc_info *soc_info = &davinci_soc_info; > >>> + void __iomem *psc_base; > >>> + struct clk *dsp_clk; > >>> + > >>> + /* hw requires the start (boot) address be on 1KB boundary */ > >>> + if (bootaddr & 0x3ff) { > >>> + dev_err(dev, "invalid boot address: must be aligned to > >> 1KB\n"); > >>> + return -EINVAL; > >>> + } > >>> + > >>> + dsp_clk = clk_get(dev, pdata->clk_name); > > >> We could match using clkdev functionality, but the clock entry > >> would need to be changed then... > > > I followed the existing pattern I saw in other drivers. > > Probably MUSB? We're trying to move away from passing the clock name to thge > drivers, using match by device instead. > > > If there is a new, better way, please point me to an example. > > Look at the da850_clks[] in mach-davinci/da850.c: if the device name is > specified as a first argument to CLK() macro (instead of clock name the second > argument), the matching is done by device, so you don't need to specify the > clock name to clk_get(), passing NULL instead. Thanks, I'll look at this and ask for Sekhar and Kevin's preferences. Mark
> From: Nori, Sekhar > Sent: Friday, June 24, 2011 8:44 AM > > Hi Mark, Sekhar, thanks for your feedback and ideas. Comments below. Mark > Since procedure to set the boot address varies across DaVinci > platforms, you could have a callback populated in platform data > which will be implemented differently for original DaVinci and > DA8xx devices. I looked at DM6467 and it's the same as OMAPL13x, except at a different address. Rather than a callback, it could be just an address in the platform data. > > Also, all PSC accesses are better off going through clock > framework to ensure proper locking and modularity. > > To assert/de-assert local reset when enabling or disabling PSC, > you could use a flag in the clock structure to indicate the need > for this. This way, if there is any other module needing a local > reset, it can just define the same flag. Similarly, if the DSP > does not need a local reset on a particular platform, that > platform can simply skip the flag. > > This can be done in a manner similar to how the support for > a forced transition PSC was added here: > > https://patchwork.kernel.org/patch/662941/ Yes, I like this idea - much cleaner. For example, the start() method becomes (pseudo-code): start() { /* bootaddrreg derived from platform data */ bootaddrreg = boot_address; clk_enable(); } Referring to your patch above, would it be better to just pass the flags into the davinci_psc_config() function rather than breaking out more arguments for each flag? Mark
Hi Mark, On Mon, Jun 27, 2011 at 23:50:25, Grosen, Mark wrote: > > >>> +static inline int davinci_rproc_start(struct rproc *rproc, u64 > > >> bootaddr) > > >>> +{ > > >>> + struct device *dev = rproc->dev; > > >>> + struct davinci_rproc_pdata *pdata = dev->platform_data; > > >>> + struct davinci_soc_info *soc_info = &davinci_soc_info; > > >>> + void __iomem *psc_base; > > >>> + struct clk *dsp_clk; > > >>> + > > >>> + /* hw requires the start (boot) address be on 1KB boundary */ > > >>> + if (bootaddr & 0x3ff) { > > >>> + dev_err(dev, "invalid boot address: must be aligned to > > >> 1KB\n"); > > >>> + return -EINVAL; > > >>> + } > > >>> + > > >>> + dsp_clk = clk_get(dev, pdata->clk_name); > > > > >> We could match using clkdev functionality, but the clock entry > > >> would need to be changed then... > > > > > I followed the existing pattern I saw in other drivers. > > > > Probably MUSB? We're trying to move away from passing the clock name to thge > > drivers, using match by device instead. > > > > > If there is a new, better way, please point me to an example. > > > > Look at the da850_clks[] in mach-davinci/da850.c: if the device name is > > specified as a first argument to CLK() macro (instead of clock name the second > > argument), the matching is done by device, so you don't need to specify the > > clock name to clk_get(), passing NULL instead. > > Thanks, I'll look at this and ask for Sekhar and Kevin's preferences. The second argument is not a (SoC specific) clock name, it is the "connection id" which is specific to the IP (not to the SoC). For example, the EMAC IP may want to deal with interface clock, functional clock and mii clock. So, it passes a connection identifier to tell the clock framework which particular clock it is interested in. The names of these connection identifiers will not change with SoC as they are IP property. So, it will not be right to get this through platform data. If there is no connection identifier in your case, you can just pass the second argument as NULL. The clock matching will happen using device name (which should remain same across SoCs too). Of course, this means that any incorrectly defined clock lookup structures for dsp clock in platform code needs to be corrected. Thanks, Sekhar
Hi Mark, On Tue, Jun 28, 2011 at 00:01:41, Grosen, Mark wrote: > > From: Nori, Sekhar > > Sent: Friday, June 24, 2011 8:44 AM > > To assert/de-assert local reset when enabling or disabling PSC, > > you could use a flag in the clock structure to indicate the need > > for this. This way, if there is any other module needing a local > > reset, it can just define the same flag. Similarly, if the DSP > > does not need a local reset on a particular platform, that > > platform can simply skip the flag. > > > > This can be done in a manner similar to how the support for > > a forced transition PSC was added here: > > > > https://patchwork.kernel.org/patch/662941/ > > Yes, I like this idea - much cleaner. For example, the start() method > becomes (pseudo-code): > > start() > { > /* bootaddrreg derived from platform data */ > bootaddrreg = boot_address; > > clk_enable(); > } > > Referring to your patch above, would it be better to just pass > the flags into the davinci_psc_config() function rather than breaking out more > arguments for each flag? I did dwell on this quite a bit while writing that patch, but finally decided on passing an argument instead since I was not sure if there are going to be more modifiers required. Now that you have the need for one more flag, I think we should go ahead and pass the flags directly to davinci_psc_config(). My original patch is not upstream yet. I will re-spin it and you can build on top of it. Thanks, Sekhar
Hi Mark, On Tue, Jun 28, 2011 at 00:01:41, Grosen, Mark wrote: > > From: Nori, Sekhar > > Sent: Friday, June 24, 2011 8:44 AM > > > > Hi Mark, > > Sekhar, thanks for your feedback and ideas. Comments below. > > Mark > > > Since procedure to set the boot address varies across DaVinci > > platforms, you could have a callback populated in platform data > > which will be implemented differently for original DaVinci and > > DA8xx devices. > > I looked at DM6467 and it's the same as OMAPL13x, except at a different > address. Rather than a callback, it could be just an address in the > platform data. Sounds okay as long as _all_ the DaVinci devices have the same bit to be set. Plus, I hope there are no other users of the register so that there is no race with other platform code using the same register. Thanks, Sekhar
> From: Nori, Sekhar > Sent: Monday, July 04, 2011 10:30 PM ... > > > https://patchwork.kernel.org/patch/662941/ > > > > Yes, I like this idea - much cleaner. For example, the start() method > > becomes (pseudo-code): > > > > start() > > { > > /* bootaddrreg derived from platform data */ > > bootaddrreg = boot_address; > > > > clk_enable(); > > } > > > > Referring to your patch above, would it be better to just pass > > the flags into the davinci_psc_config() function rather than breaking > out more > > arguments for each flag? > > I did dwell on this quite a bit while writing that > patch, but finally decided on passing an argument > instead since I was not sure if there are going > to be more modifiers required. > > Now that you have the need for one more flag, I > think we should go ahead and pass the flags directly > to davinci_psc_config(). My original patch is not > upstream yet. I will re-spin it and you can build > on top of it. Thanks, Sekhar, this sounds good. I'll look for your patch and utilize it in the next rev of this patch. Mark
> From: Nori, Sekhar > Sent: Monday, July 04, 2011 10:35 PM > To: Grosen, Mark; Sergei Shtylyov ... > > > Since procedure to set the boot address varies across DaVinci > > > platforms, you could have a callback populated in platform data > > > which will be implemented differently for original DaVinci and > > > DA8xx devices. > > > > I looked at DM6467 and it's the same as OMAPL13x, except at a > different > > address. Rather than a callback, it could be just an address in the > > platform data. > > Sounds okay as long as _all_ the DaVinci devices have the same > bit to be set. Plus, I hope there are no other users of the > register so that there is no race with other platform code using > the same register. Sekhar, The register is a dedicated 32-bit register that holds the start/boot address for the DSP, so no other platform code should be using it. Once the LRST is de-asserted (via the PSC code enhancement), the DSP starts execution at the address in this register. Thanks, Mark
Hi Mark, On Tue, Jul 05, 2011 at 22:24:21, Grosen, Mark wrote: > > From: Nori, Sekhar > > Sent: Monday, July 04, 2011 10:35 PM > > To: Grosen, Mark; Sergei Shtylyov > > ... > > > > Since procedure to set the boot address varies across DaVinci > > > > platforms, you could have a callback populated in platform data > > > > which will be implemented differently for original DaVinci and > > > > DA8xx devices. > > > > > > I looked at DM6467 and it's the same as OMAPL13x, except at a > > different > > > address. Rather than a callback, it could be just an address in the > > > platform data. > > > > Sounds okay as long as _all_ the DaVinci devices have the same > > bit to be set. Plus, I hope there are no other users of the > > register so that there is no race with other platform code using > > the same register. > > Sekhar, > > The register is a dedicated 32-bit register that holds the start/boot > address for the DSP, so no other platform code should be using it. Once > the LRST is de-asserted (via the PSC code enhancement), the DSP starts > execution at the address in this register. Okay. I had misunderstood this as a bit which is used to reset the DSP. Thanks for clarifying. Regards, Sekhar
diff --git a/arch/arm/mach-davinci/include/mach/remoteproc.h b/arch/arm/mach-davinci/include/mach/remoteproc.h new file mode 100644 index 0000000..af6e88c --- /dev/null +++ b/arch/arm/mach-davinci/include/mach/remoteproc.h @@ -0,0 +1,28 @@ +/* + * Remote Processor + * + * Copyright (C) 2011 Texas Instruments, Inc. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef _DAVINCI_REMOTEPROC_H +#define _DAVINCI_REMOTEPROC_H + +#include <linux/remoteproc.h> + +struct davinci_rproc_pdata { + struct rproc_ops *ops; + char *name; + char *clk_name; + char *firmware; +}; + +#endif /* _DAVINCI_REMOTEPROC_H */ diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig index 88421bd..1e594b5 100644 --- a/drivers/remoteproc/Kconfig +++ b/drivers/remoteproc/Kconfig @@ -26,3 +26,19 @@ config OMAP_REMOTE_PROC It's safe to say n here if you're not interested in multimedia offloading or just want a bare minimum kernel. + +config DAVINCI_REMOTE_PROC + tristate "Davinci remoteproc support" + depends on ARCH_DAVINCI_DA850 + select REMOTE_PROC + default y + help + Say y here to support Davinci's DSP remote processor via the remote + processor framework (currently only da850 is supported). + + Usually you want to say y here, in order to enable multimedia + use-cases to run on your platform (multimedia codecs are + offloaded to remote DSP processors using this framework). + + It's safe to say n here if you're not interested in multimedia + offloading or just want a bare minimum kernel. diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile index 0198b1d..e83dac9 100644 --- a/drivers/remoteproc/Makefile +++ b/drivers/remoteproc/Makefile @@ -4,3 +4,4 @@ obj-$(CONFIG_REMOTE_PROC) += remoteproc.o obj-$(CONFIG_OMAP_REMOTE_PROC) += omap_remoteproc.o +obj-$(CONFIG_DAVINCI_REMOTE_PROC) += davinci_remoteproc.o diff --git a/drivers/remoteproc/davinci_remoteproc.c b/drivers/remoteproc/davinci_remoteproc.c new file mode 100644 index 0000000..0e26fe9 --- /dev/null +++ b/drivers/remoteproc/davinci_remoteproc.c @@ -0,0 +1,147 @@ +/* + * Remote processor machine-specific module for Davinci + * + * Copyright (C) 2011 Texas Instruments, Inc. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#define pr_fmt(fmt) "%s: " fmt, __func__ + +#include <linux/kernel.h> +#include <linux/err.h> +#include <linux/printk.h> +#include <linux/bitops.h> +#include <linux/platform_device.h> +#include <linux/remoteproc.h> +#include <linux/clk.h> +#include <linux/io.h> + +#include <mach/da8xx.h> +#include <mach/cputype.h> +#include <mach/psc.h> +#include <mach/remoteproc.h> + +/* + * Technical Reference: + * OMAP-L138 Applications Processor System Reference Guide + * http://www.ti.com/litv/pdf/sprugm7d + */ + +/* local reset bit (0 is asserted) in MDCTL15 register (section 9.6.18) */ +#define LRST BIT(8) + +/* next state bits in MDCTL15 register (section 9.6.18) */ +#define NEXT_ENABLED 0x3 + +/* register for DSP boot address in SYSCFG0 module (section 11.5.6) */ +#define HOST1CFG 0x44 + +static inline int davinci_rproc_start(struct rproc *rproc, u64 bootaddr) +{ + struct device *dev = rproc->dev; + struct davinci_rproc_pdata *pdata = dev->platform_data; + struct davinci_soc_info *soc_info = &davinci_soc_info; + void __iomem *psc_base; + struct clk *dsp_clk; + + /* hw requires the start (boot) address be on 1KB boundary */ + if (bootaddr & 0x3ff) { + dev_err(dev, "invalid boot address: must be aligned to 1KB\n"); + return -EINVAL; + } + + dsp_clk = clk_get(dev, pdata->clk_name); + if (IS_ERR_OR_NULL(dsp_clk)) { + dev_err(dev, "clk_get error: %ld\n", PTR_ERR(dsp_clk)); + return PTR_ERR(dsp_clk); + } + + clk_enable(dsp_clk); + rproc->priv = dsp_clk; + + psc_base = ioremap(soc_info->psc_bases[0], SZ_4K); + + /* insure local reset is asserted before writing start address */ + __raw_writel(NEXT_ENABLED, psc_base + MDCTL + 4 * DA8XX_LPSC0_GEM); + + __raw_writel(bootaddr, DA8XX_SYSCFG0_VIRT(HOST1CFG)); + + /* de-assert local reset to start the dsp running */ + __raw_writel(LRST | NEXT_ENABLED, psc_base + MDCTL + + 4 * DA8XX_LPSC0_GEM); + + iounmap(psc_base); + + return 0; +} + +static inline int davinci_rproc_stop(struct rproc *rproc) +{ + struct davinci_soc_info *soc_info = &davinci_soc_info; + void __iomem *psc_base; + struct clk *dsp_clk = rproc->priv; + + psc_base = ioremap(soc_info->psc_bases[0], SZ_4K); + + /* halt the dsp by asserting local reset */ + __raw_writel(NEXT_ENABLED, psc_base + MDCTL + 4 * DA8XX_LPSC0_GEM); + + clk_disable(dsp_clk); + clk_put(dsp_clk); + + iounmap(psc_base); + + return 0; +} + +static struct rproc_ops davinci_rproc_ops = { + .start = davinci_rproc_start, + .stop = davinci_rproc_stop, +}; + +static int davinci_rproc_probe(struct platform_device *pdev) +{ + struct davinci_rproc_pdata *pdata = pdev->dev.platform_data; + + return rproc_register(&pdev->dev, pdata->name, &davinci_rproc_ops, + pdata->firmware, NULL, THIS_MODULE); +} + +static int __devexit davinci_rproc_remove(struct platform_device *pdev) +{ + struct davinci_rproc_pdata *pdata = pdev->dev.platform_data; + + return rproc_unregister(pdata->name); +} + +static struct platform_driver davinci_rproc_driver = { + .probe = davinci_rproc_probe, + .remove = __devexit_p(davinci_rproc_remove), + .driver = { + .name = "davinci-rproc", + .owner = THIS_MODULE, + }, +}; + +static int __init davinci_rproc_init(void) +{ + return platform_driver_register(&davinci_rproc_driver); +} +module_init(davinci_rproc_init); + +static void __exit davinci_rproc_exit(void) +{ + platform_driver_unregister(&davinci_rproc_driver); +} +module_exit(davinci_rproc_exit); + +MODULE_LICENSE("GPL v2"); +MODULE_DESCRIPTION("Davinci Remote Processor control driver");