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 Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source