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
