RE: [RFC 5/8] remoteproc: add davinci implementation
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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC 5/8] remoteproc: add davinci implementation
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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC 5/8] remoteproc: add davinci implementation
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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC 5/8] remoteproc: add davinci implementation
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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC 0/8] Introducing a generic AMP/IPC framework
From: Ohad Ben-Cohen Sent: Saturday, June 25, 2011 6:12 PM Hi Stephen, On Fri, Jun 24, 2011 at 11:16 PM, Stephen Boyd sb...@codeaurora.org wrote: Instead of devising a new firmware format, we decided to just stick with elf and parse the headers in the kernel because we needed them for authentication anyway. Is this reason enough to move to an ELF format instead? I think that consolidation of code is enough reason to make an effort. I know that our firmware format was chosen for simplicity, but I'm not sure if we have the tools yet to build standard ELF files for the remote processors (IIRC it's in the works though). I'll let Mark comment this one. Yes, we are converting from standard ELF to the simple format. I've used the GNU binutils to work with our ELF files. There were a few motivations: 1. Concern about complexity of parsing ELF files in kernel; however, the PIL implementation looks pretty clean to me. 2. We added a special section (resource table) that is interpreted as part of the loading process. The tool that generates our simple format just recognizes a named section (.resource_table), so perhaps that could be done with the PIL ELF loader. 3. Smaller firmware file sizes. Our ELF files are large relative to the payload, but this might be addressed by a better ELF strip utility. Another difference is inter-processor dependencies. For example, on msm8660 the modem can't boot until the dsp has been booted. I suppose we could hide this detail in the platform specific get() implementation by calling rproc_get() on the dependent processor (hopefully no locking issues arise). I'd rather have it built into the core though as it isn't really specific to the hardware. No problems, I'm sure we can solve this one easily. If we can resolve these differences I think we can easily support remote processor boot on MSM via remoteproc. That'd be very cool, I sure do hope we can work together. Yes, I hope we can merge our efforts on PIL and remoteproc since they seem quite close in function and design. Mark -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC 1/8] drivers: add generic remoteproc framework
From: Grant Likely Sent: Monday, June 27, 2011 1:50 PM Grant, thanks for the feedback. I'll try to answer one of your questions below and leave the rest for Ohad. Mark +Every remoteproc implementation must provide these handlers: + +struct rproc_ops { + int (*start)(struct rproc *rproc, u64 bootaddr); + int (*stop)(struct rproc *rproc); +}; + +The -start() handler takes a rproc handle and an optional bootaddr argument, +and should power on the device and boot it (using the bootaddr argument +if the hardware requires one). Naive question: Why is bootaddr an argument? Wouldn't rproc drivers keep track of the boot address in their driver private data? Our AMPs (remote processors) have a variety of boot mechanisms that vary across the different SoCs (yes, TI likes HW diversity). In some cases, the boot address is more like an entry point and that comes from the firmware, so it is not a static attribute of a driver. Correct me if I misunderstood your question. Mark -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC 1/8] drivers: add generic remoteproc framework
From: Grant Likely Sent: Monday, June 27, 2011 3:24 PM Our AMPs (remote processors) have a variety of boot mechanisms that vary across the different SoCs (yes, TI likes HW diversity). In some cases, the boot address is more like an entry point and that comes from the firmware, so it is not a static attribute of a driver. Correct me if I misunderstood your question. More to the point, I would expect the boot_address to be a property of the rproc instance because it represents the configuration of the remote processor. It seems odd that the caller of -start would know better than the rproc driver about the entry point of the processor. g. Yes, in many cases the boot_address will be defined by the HW. However, we have processors that are soft - the boot_address comes from the particular firmware being loaded and can (will) be different with each firmware image. We factored out the firmware loader to be device-independent (in remoteproc.c) so it's not repeated in each device-specific implementation like omap_remoteproc.c and davinci_remoteproc.c. In the cases where the HW dictates what happens, the start() method should just ignore the boot_address. Mark -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC 0/8] Introducing a generic AMP/IPC framework
From: Michael Williamson Sent: Thursday, June 23, 2011 5:23 AM ... I'd like to kick the tires on this with a da850 based platform (MityDSP-L138). Any chance you might be able to share the stuff you did on the remote side (DSP/BIOS adaptations for rpmsg, utils for ELF or COFF conversion to firmware format, etc.) for the DSP side of your tests done with the hawkboard? We have only implemented the remoteproc (load, start, stop) part for OMAPL138 so far, i.e., not the rpmsg part (communications). I am not sure when we will get to the rpmsg part. We will be publishing a Git tree soon with the remote processor code under a BSD license. This code works with our TI RTOS, SYS/BIOS (also BSD), on both M3 and DSP CPUs. This will include the utility to generate the RPRC firmware format from an ELF file. Note that the Linux side does not have any explicit binding or dependency on the runtime environment on the remote processor, so alternate RTOS or bare-metal implementations could also be done. We will post a follow-up to the list with a URL soon. It looks like, at least for the da850, this subsumes or obsoletes DSPLINK in order to drive a more general purpose architecture (which looks great, so far, BTW). Is that the intent? First, we are not abandoning DSPLINK. We have many users of this, even though it is out-of-tree, and we will continue to support it. That said, we do intend to make this new design the basis for DSPLINK-like functionality. It's designed to be done the right way for Linux (and we are looking for feedback to make it better). It is also designed to be more scalable and extensible in userspace. With a solid kernel foundation, we can provide lots of functionality in userspace, or users can implement their own custom solutions. One of the key things to do is map our existing DSPLINK APIs, like MessageQ, to the new rpmsg transport. Mark -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC 5/8] remoteproc: add davinci implementation
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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC 0/8] Introducing a generic AMP/IPC framework
From: Arnd Bergmann Sent: Thursday, June 23, 2011 11:47 AM Subject: Re: [RFC 0/8] Introducing a generic AMP/IPC framework On Thursday 23 June 2011 18:27:10 Grosen, Mark wrote: First, we are not abandoning DSPLINK. We have many users of this, even though it is out-of-tree, and we will continue to support it. That said, we do intend to make this new design the basis for DSPLINK-like functionality. It's designed to be done the right way for Linux (and we are looking for feedback to make it better). It is also designed to be more scalable and extensible in userspace. With a solid kernel foundation, we can provide lots of functionality in userspace, or users can implement their own custom solutions. One of the key things to do is map our existing DSPLINK APIs, like MessageQ, to the new rpmsg transport. Sounds all good. What about the PRUSS code? Does that fit into the new model as well? Arnd Arnd, Yes, I have been following some of the PRUSS discussion. I think the remoteproc driver could be used to manage the basic load/start/stop of the PRUSS processor. I am not sure if the virio/rpmsg part would be a good fit. The PRUSS processor is pretty limited, so the generality of virtio might be too much to fit and too much overhead. However, one of the good things about remoteproc currently is that it is standalone, so other transports could use it via the rproc_get/put methods. Mark -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] TI816X: Add minimal hwmod data
From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-ow...@vger.kernel.org] On Behalf Of Paul Walmsley Sent: Thursday, April 28, 2011 3:35 PM to review these patches, I'll need a copy of the TRM. Could you point me to it or send me one, please? http://focus.ti.com/general/docs/litabsmultiplefilelist.tsp?literatureNumber=sprugx8 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html