Hi Sekhar,
Thanks for the detailed review.
[...]
> It will be nice to point out which of the existing platforms
> were tested to be working after this series was applied
> (with some indication of the type of testing done).
Will do in the next rev.
[...]
> > +struct platform_device davinci_wdt_device = {
> > + .name = "watchdog",
> > + .id = -1,
> > + .num_resources = ARRAY_SIZE(wdt_resources),
> > + .resource = wdt_resources,
> > +};
>
> IMO, a better way to overcome the 'no watchdog timer'
> limitation would be to make the watchdog reset code
> not use the wdt platform data directly, but instead
> 'search' for watchdog device using bus_for_each_dev()
> iterator on platform bus.
>
> In your case, the watchdog device wouldn't be found
> and the reset function should exist gracefully.
Agreed. In addition to the platform_device search, I will also need to
put in an override-able reset hook for tnetv107x watchdog reset to plug
in into.
[...]
> > +void __init tnetv107x_serial_init(struct plat_serial8250_port* ports)
> > +{
> > + int i;
> > + char name[16];
> > + struct clk *uart_clk;
> > +
> > + for (i = 0; ports[i].flags; i++) {
> > + sprintf(name, "uart%d", i);
> > + uart_clk = clk_get(NULL, name);
> > + if (IS_ERR(uart_clk))
> > + printk(KERN_ERR "%s:%d: failed to get UART%d clock\n",
> > + __func__, __LINE__, i);
> > + else {
> > + clk_enable(uart_clk);
> > + ports[i].uartclk = clk_get_rate(uart_clk);
> > + }
> > + }
> > +}
>
> An explanation on why davinci_serial_init() is no good for
> this SoC would be good.
Will include comments. There are a couple of problems that prevent
davinci_serial_init() from being reused as is.
First, that function uses IO_ADDRESS() - a scheme that will not work (as
it stands) on tnetv107x.
Second, davinci_serial_init() goes around modifying PWREMU - a register
that doesnt exist on tnetv107x.
[...]
> No need of the 'clk_' prefix for all the LPSC
> clock names.
Ok. Will modify.
[...]
> > + * TNETV107X platforms do not use the static mappings from Davinci
> > + * IO_PHYS/IO_VIRT. This SOC's interesting MMRs are at different addresses,
> > + * and changing IO_PHYS would break away from existing Davinci SOCs.
>
> So why not redefine the IO_PHYS for this SoC? I believe you are not able
> to build a single image for TNETV107x and DaVinci anyway.
>
> That should also avoid the EDMA issue you highlight below.
Although tnetv107x and davinci currently cannot be built into a single
image, I wanted to keep that breakage to a minimum. At some point in
the future, if the underlying ARM arch code supports combined v6+v5, we
wouldn't need to go around fixing things all over the place.
[...]
> > +static void __iomem *fixed_ioremap(unsigned long p, size_t size)
> > +{
> > + struct map_desc *map = tnetv107x_soc_info.io_desc;
> > + int i;
> > +
> > + for (i = 0; i < tnetv107x_soc_info.io_desc_num; i++) {
> > + unsigned long iophys = __pfn_to_phys(map[i].pfn);
> > + unsigned long iosize = map[i].length;
> > +
> > + if (p >= iophys && (p + size) <= (iophys + iosize))
> > + return IOMEM(map[i].virtual + p - iophys);
> > + }
> > +
> > + panic("attempt to map invalid physical range %lx-%lx\n",
> > + p, p + size);
> > +}
>
> This sounds like something that should be useful on
> all DaVinci SoCs. Why not make davinci_ioremap to do
> exactly this?
Good point. davinci_ioremap() can do this based on the iotable info it
pulls out of davinci_soc_info. The only hitch here is that da8xx seems
to ioremap() before davinci_common_init(). Let me propose a patch for
this (separately) and take it from there.
[...]
> > +static unsigned long clk_sspll_recalc(struct clk *clk)
> > +{
> > + int pll;
> > + unsigned long mult = 0, prediv = 1, postdiv = 1;
> > + unsigned long ref = OSC_FREQ_ONCHIP, ret;
> > + u32 tmp;
> > +
> > + if (WARN_ON(!clk->pll_data))
> > + return clk->rate;
> > +
> > + pll = clk->pll_data->num;
> > +
> > + tmp = __raw_readl(&clk_ctrl_regs->pll_bypass);
> > + if (!(tmp & bypass_mask[pll])) {
> > + mult = __raw_readl(&sspll_regs[pll]->mult_factor);
> > + prediv = __raw_readl(&sspll_regs[pll]->pre_div) + 1;
> > + postdiv = __raw_readl(&sspll_regs[pll]->post_div) + 1;
> > + }
> > +
> > + tmp = __raw_readl(pllctl_regs[pll] + PLLCTL);
> > + if (tmp & PLLCTL_CLKMODE)
> > + ref = pll_ext_freq[pll];
> > +
> > + clk->pll_data->input_rate = ref;
> > +
> > + tmp = __raw_readl(pllctl_regs[pll] + PLLCTL);
> > + if (!(tmp & PLLCTL_PLLEN))
> > + return ref;
> > +
> > + ret = ref;
> > + if (mult)
> > + ret += ((unsigned long long)ref * mult) / 256;
> > +
> > + ret /= (prediv * postdiv);
> > +
> > + return ret;
> > +}
>
> How about moving this to clock.c and using a flag in clk
> structure to determine whether to use clk_pllclk_recalc()
> or clk_sspll_recalc()?
For now, it seems the sspll wrapper is specific to tnetv107x. That
said, I would rather keep this code private to tnetv107x (for now) and
promote to clock.c if/when other socs with ssplls turn up.
[...]
> > +static unsigned long clk_div_recalc(struct clk *clk)
> > +{
> > + struct pll_data *pll;
> > + unsigned long rate = clk->rate;
> > + unsigned long div;
> > +
> > + if (WARN_ON(!clk->parent))
> > + return rate;
> > +
> > + rate = clk->parent->rate;
> > +
> > + /* Otherwise, the parent must be a PLL */
> > + if (WARN_ON(!clk->parent->pll_data))
> > + return rate;
> > +
> > + pll = clk->parent->pll_data;
> > +
> > + if (!clk->div_reg)
> > + return rate;
> > +
> > + div = __raw_readl(pllctl_regs[pll->num] + clk->div_reg);
> > + if (div & PLLDIV_EN) {
> > + div &= div_mask[pll->num];
> > + rate /= (div + 1);
> > + }
> > +
> > + return rate;
> > +}
>
> Is the div_mask the only reason you could not use clock.c:clk_sysclk_recalc()
> here? How about making the mask a part of PLL data and using PLLDIV_RATIO_MASK
> if no mask is initialized (zero)?
Agreed. Will do so.
> > +static unsigned long clk_null_recalc(struct clk *clk)
> > +{
> > + BUG_ON(!clk->parent);
> > + return clk->parent->rate;
> > +}
>
> There is already a function in clock.c which does exactly this
> (clk_leafclk_recalc).
Agreed. Will reuse.
Thanks
- Cyril.
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
