lkml.org 
[lkml]   [2018]   [Jan]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v5 11/44] clk: davinci: Add platform information for TI DA830 PSC
    From
    Date
    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@lechnology.com>
    > ---
    > 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@lechnology.com>
    > + */
    > +
    > +#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

    \
     
     \ /
      Last update: 2018-01-16 14:41    [W:4.322 / U:0.544 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site