Re: [PATCH v5 11/44] clk: davinci: Add platform information for TI DA830 PSC
From: Sekhar Nori
Date: Tue Jan 16 2018 - 08:40:13 EST
On Monday 08 January 2018 07:47 AM, David Lechner wrote:
> This adds platform-specific declarations for the PSC clocks on TI DA830/
> OMAP-L137/AM17XX SoCs.
>
> Signed-off-by: David Lechner <david@xxxxxxxxxxxxxx>
> ---
> drivers/clk/davinci/Makefile | 1 +
> drivers/clk/davinci/psc-da830.c | 96 +++++++++++++++++++++++++++++++++++++++++
> include/linux/clk/davinci.h | 2 +
> 3 files changed, 99 insertions(+)
> create mode 100644 drivers/clk/davinci/psc-da830.c
>
> diff --git a/drivers/clk/davinci/Makefile b/drivers/clk/davinci/Makefile
> index cd1bf2c..fb14c8c 100644
> --- a/drivers/clk/davinci/Makefile
> +++ b/drivers/clk/davinci/Makefile
> @@ -10,4 +10,5 @@ obj-$(CONFIG_ARCH_DAVINCI_DM644x) += pll-dm644x.o
> obj-$(CONFIG_ARCH_DAVINCI_DM646x) += pll-dm646x.o
>
> obj-y += psc.o
> +obj-$(CONFIG_ARCH_DAVINCI_DA830) += psc-da830.o
> endif
> diff --git a/drivers/clk/davinci/psc-da830.c b/drivers/clk/davinci/psc-da830.c
> new file mode 100644
> index 0000000..193b08f
> --- /dev/null
> +++ b/drivers/clk/davinci/psc-da830.c
> @@ -0,0 +1,96 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * PSC clock descriptions for TI DA830/OMAP-L137/AM17XX
> + *
> + * Copyright (C) 2017 David Lechner <david@xxxxxxxxxxxxxx>
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/clkdev.h>
> +#include <linux/init.h>
> +#include <linux/types.h>
> +
> +#include "psc.h"
> +
> +static const struct davinci_psc_clk_info da830_psc0_info[] __initconst = {
> + LPSC(0, 0, tpcc, pll0_sysclk2, LPSC_ALWAYS_ENABLED),
> + LPSC(1, 0, tptc0, pll0_sysclk2, LPSC_ALWAYS_ENABLED),
> + LPSC(2, 0, tptc1, pll0_sysclk2, LPSC_ALWAYS_ENABLED),
> + LPSC(3, 0, aemif, pll0_sysclk3, LPSC_ALWAYS_ENABLED),
> + LPSC(4, 0, spi0, pll0_sysclk2, 0),
> + LPSC(5, 0, mmcsd, pll0_sysclk2, 0),
> + LPSC(6, 0, aintc, pll0_sysclk4, LPSC_ALWAYS_ENABLED),
> + LPSC(7, 0, arm_rom, pll0_sysclk2, LPSC_ALWAYS_ENABLED),
> + LPSC(8, 0, secu_mgr, pll0_sysclk4, LPSC_ALWAYS_ENABLED),
> + LPSC(9, 0, uart0, pll0_sysclk2, 0),
> + LPSC(10, 0, scr0_ss, pll0_sysclk2, LPSC_ALWAYS_ENABLED),
> + LPSC(11, 0, scr1_ss, pll0_sysclk2, LPSC_ALWAYS_ENABLED),
> + LPSC(12, 0, scr2_ss, pll0_sysclk2, LPSC_ALWAYS_ENABLED),
> + LPSC(13, 0, dmax, pll0_sysclk2, LPSC_ALWAYS_ENABLED),
pruss is better (I know the name is coming from existing code).
> + LPSC(14, 0, arm, pll0_sysclk6, LPSC_ALWAYS_ENABLED),
This is LPSC 15 which controls DSP too. But its missing from existing
code. Not sure why. Probably a note for future. For now okay with
ignoring it.
> + { }
> +};
Tables like these are much easier to parse if columns are spaced using a
tab.
> +
> +static const struct davinci_psc_clk_info da830_psc1_info[] __initconst = {
> + LPSC(1, 0, usb0, pll0_sysclk2, 0),
> + LPSC(2, 0, usb1, pll0_sysclk4, 0),
> + LPSC(3, 0, gpio, pll0_sysclk4, 0),
There is LPSC 4 controlling UHPI. Again, lets ignore for now.
> + LPSC(5, 0, emac, pll0_sysclk4, 0),
> + LPSC(6, 0, emif3, pll0_sysclk5, LPSC_ALWAYS_ENABLED),
> + LPSC(7, 0, mcasp0, pll0_sysclk2, 0),
> + LPSC(8, 0, mcasp1, pll0_sysclk2, 0),
> + LPSC(9, 0, mcasp2, pll0_sysclk2, 0),
> + LPSC(10, 0, spi1, pll0_sysclk2, 0),
> + LPSC(11, 0, i2c1, pll0_sysclk4, 0),
> + LPSC(12, 0, uart1, pll0_sysclk2, 0),
> + LPSC(13, 0, uart2, pll0_sysclk2, 0),
> + LPSC(16, 0, lcdc, pll0_sysclk2, 0),
> + LPSC(17, 0, pwm, pll0_sysclk2, 0),
> + LPSC(20, 0, ecap, pll0_sysclk2, 0),
> + LPSC(21, 0, eqep, pll0_sysclk2, 0),
> + { }
> +};
> +
> +void __init da830_psc_clk_init(void __iomem *psc0, void __iomem *psc1)
> +{
> + struct clk_onecell_data *clk_data;
> +
> + clk_data = davinci_psc_register_clocks(psc0, da830_psc0_info, 16);
> + if (!clk_data)
> + return;
> +
> + clk_register_clkdev(clk_data->clks[4], NULL, "spi_davinci.0");
> + clk_register_clkdev(clk_data->clks[5], NULL, "da830-mmc.0");
> + clk_register_clkdev(clk_data->clks[9], NULL, "serial8250.0");
> + clk_register_clkdev(clk_data->clks[14], "arm", NULL);
> +
> + clk_free_onecell_data(clk_data);
> +
> + clk_data = davinci_psc_register_clocks(psc1, da830_psc1_info, 32);
> + if (!clk_data)
> + return;
> +
> + clk_register_clkdev(clk_data->clks[1], NULL, "musb-da8xx");
> + clk_register_clkdev(clk_data->clks[1], NULL, "cppi41-dmaengine");
> + clk_register_clkdev(clk_data->clks[2], NULL, "ohci-da8xx");
> + clk_register_clkdev(clk_data->clks[3], "gpio", NULL);
This is pretty bad (and no fault of yours) - having a con_id but no
device name. Can you please make a pre-series which passes NULL con_id
in gpio-davinci.c?
> + clk_register_clkdev(clk_data->clks[5], NULL, "davinci_emac.1");
> + clk_register_clkdev(clk_data->clks[5], "fck", "davinci_mdio.0");
> + clk_register_clkdev(clk_data->clks[7], NULL, "davinci-mcasp.0");
> + clk_register_clkdev(clk_data->clks[8], NULL, "davinci-mcasp.1");
> + clk_register_clkdev(clk_data->clks[9], NULL, "davinci-mcasp.2");
> + clk_register_clkdev(clk_data->clks[10], NULL, "spi_davinci.1");
> + clk_register_clkdev(clk_data->clks[11], NULL, "i2c_davinci.2");
> + clk_register_clkdev(clk_data->clks[12], NULL, "serial8250.1");
> + clk_register_clkdev(clk_data->clks[13], NULL, "serial8250.2");
> + clk_register_clkdev(clk_data->clks[16], "fck", "da8xx_lcdc.0");
> + clk_register_clkdev(clk_data->clks[17], "fck", "ehrpwm.0");
> + clk_register_clkdev(clk_data->clks[17], "fck", "ehrpwm.1");
> + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.0");
> + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.1");
> + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.2");
> + clk_register_clkdev(clk_data->clks[21], NULL, "eqep.0");
> + clk_register_clkdev(clk_data->clks[21], NULL, "eqep.1");
This is going to be very difficult to audit for mistakes. How do you
feel about adding the con_id and dev_id to davinci_psc_clk_info[] so
they can be initialized as part of a single static table? And then here
you go over the table looking for non-NULL con_id/dev_id to call
clk_register_clkdev()?
I am guessing you did not take that route because the DT path does not
need those. But still, I think that will be much less error prone.
Thanks,
Sekhar