On Tue, Feb 28, 2012 at 07:39:24AM +0000, Arnd Bergmann wrote:
> On Monday 27 February 2012, Jason Cooper wrote:
> > On the Globalscale Dreamplug (Marvell Kirkwood Development Platform),
> > 2MB of NOR flash are used to hold the bootloader, bootloader
> > environment, and devicetree blob. It is connected via spi.
> >
> > Signed-off-by: Jason Cooper <[email protected]>
> > ---
> > Notes:
> > - checkpatch clean.
> > - compiles, boots, registers partitions correctly.
>
> Nice, it seem that it was simpler than I first thought. I think you should
> lose some of the #ifdef statements, especially those where no extra
> object code gets added because the of_* function calls compile to
> empty inline functions.
Yes, I wasn't happy with those #ifdef's. But I wanted to get something
working early so the review process could start earlier. I don't like
polishing something for a week only to find out I did something wrong at
the beginning.
> More importantly, you should not treat CONFIG_OF as an exclusive option:
> It should be possible to build a kernel that has CONFIG_OF enabled but
> still comes with legacy board files that work like before.
Understood.
> > @@ -524,6 +528,13 @@ static int __init kirkwood_clock_gate(void)
> > } else /* keep this bit set for devices that don't have PCIe1 */
> > kirkwood_clk_ctrl |= CGC_PEX1;
> >
> > +#ifdef CONFIG_OF
> > + dp = of_find_node_by_path("/");
> > + if (of_device_is_available(of_find_compatible_node(dp, NULL,
> > + "marvell,orion-spi")))
> > + kirkwood_clk_ctrl |= CGC_RUNIT;
> > +#endif
> > +
> > /* Now gate clock the required units */
> > writel(kirkwood_clk_ctrl, CLOCK_GATING_CTRL);
> > printk(KERN_DEBUG " after: 0x%08x\n", readl(CLOCK_GATING_CTRL));
>
> This looks like it could be improved by only enabling the clock
> if we actually start using the device from the spi driver, in its
> probe function. Not sure if that's worth it.
Yes, That is the best solution, I'm trying to work around the fact that
when using fdt, kirkwood_spi_init() never gets called (CGC_RUNIT not
set), and then, orion_spi_init() (plat-orion/common.c) isn't executed,
so the plat data isn't populated.
As long as kirkwood_clock_gate() is called in late init, I'll need the
above code to prevent turning off the spi's clock.
> > @@ -101,10 +102,24 @@ static int orion_spi_baudrate_set(struct spi_device
> > *spi, unsigned int speed)
> > u32 prescale;
> > u32 reg;
> > struct orion_spi *orion_spi;
> > +#ifdef CONFIG_OF
> > + const __be32 *prop;
> > + int len;
> > +#endif
> >
> > orion_spi = spi_master_get_devdata(spi->master);
> >
> > +#ifdef CONFIG_OF
> > + prop = of_get_property(spi->master->dev.of_node, "clock-frequency",
> > + &len);
> > + if (!prop || len < sizeof(*prop)) {
> > + pr_debug("fdt missing 'clock-frequency' property.\n");
> > + return -EINVAL;
> > + }
> > + tclk_hz = be32_to_cpup(prop);
> > +#else
> > tclk_hz = orion_spi->spi_info->tclk;
> > +#endif
>
> of_get_property returns NULL when CONFIG_OF is disabled, so you can turn
> this all into a run-time logic:
>
> if (orion_spi->spi_info)
> tclk_hz = orion_spi->spi_info->tclk;
>
> of_property_read_u32(spi->master->dev.of_node,
> "clock-frequency", &tclk_hz);
>
> if (!tclk_hz) {
> dev_error(&spi->dev, "cannot set clock rate\n");
> return -EINVAL;
> }
Much nicer, thanks.
> > /*
> > * the supported rates are: 4,6,8...30
> > @@ -360,7 +375,11 @@ static int orion_spi_setup(struct spi_device *spi)
> > orion_spi = spi_master_get_devdata(spi->master);
> >
> > /* Fix ac timing if required. */
> > +#ifdef CONFIG_OF
> > + if (of_find_property(spi->master->dev.of_node, "spi-clock-fix", NULL))
> > +#else
> > if (orion_spi->spi_info->enable_clock_fix)
> > +#endif
> > orion_spi_setbits(orion_spi, ORION_SPI_IF_CONFIG_REG,
> > (1 << 14));
>
> Same thing here:
>
> if (of_find_property(spi->master->dev.of_node, "spi-clock-fix", NULL) ||
> (orion_spi->spi_info && orion_spi->spi_info->enable_clock_fix))
Also, should this be "mv,spi-clock-fix" like I've seen for some ti
custom dt bindings?
> > +#ifdef CONFIG_OF
> > + orion_spi_wq = create_singlethread_workqueue(
> > + orion_spi_driver.driver.name);
> > + if (orion_spi_wq == NULL)
> > + return -ENOMEM;
> > +#endif
>
> This seems wrong: why do you have to create the workqueue again here?
Gah! Originally, I was trying to mirror spi-tegra.c, which uses
module_platform_driver(). So, I was moving code out of orion_spi_init()
into orion_spi_probe() and setting .probe = orion_spi_probe(). I forgot
to undo this when I backed away from that approach (to get it working
first).
Should I go ahead and convert it to module_platform_driver()?
> > +#ifdef CONFIG_OF
> > + prop = of_get_property(master->dev.of_node, "clock-frequency", &len);
> > + if (!prop || len < sizeof(*prop)) {
> > + pr_debug("fdt missing 'clock-frequency' property.\n");
> > + status = -EINVAL;
> > + goto out;
> > + }
> > + tclk = be32_to_cpup(prop);
> > +
> > + spi->max_speed = DIV_ROUND_UP(tclk, 4);
> > + spi->min_speed = DIV_ROUND_UP(tclk, 30);
> > +#else
> > spi->spi_info = spi_info;
> >
> > spi->max_speed = DIV_ROUND_UP(spi_info->tclk, 4);
> > spi->min_speed = DIV_ROUND_UP(spi_info->tclk, 30);
> > +#endif
>
> Same code as above? Just find the clock frequency once and store it in
> spi->tclk.
Do you mean spi_info->tclk? If so, spi_info is NULL when using device
tree because orion_spi_init() in plat-orion/common.c never gets called,
so the platform data isn't set.
What I can do is set tclk as you suggested, above:
if (orion_spi->spi_info)
tclk = orion_spi->spi_info->tclk;
of_property_read_u32(spi->master->dev.of_node,
"clock-frequency", &tclk);
if (!tclk) {
dev_error(&spi->dev, "cannot set clock rate\n");
return -EINVAL;
}
Thanks for the review and pointers, much appreciated!
Jason.
_______________________________________________
devicetree-discuss mailing list
[email protected]
https://lists.ozlabs.org/listinfo/devicetree-discuss
