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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
