Re: [PATCH 5/6] clk: actions: Add clock driver for S500 SoC

From: Manivannan Sadhasivam
Date: Mon Jan 14 2019 - 22:15:48 EST

Hi Stephen,

On Fri, Jan 11, 2019 at 03:56:11PM -0800, Stephen Boyd wrote:
> Quoting Manivannan Sadhasivam (2018-12-31 10:55:16)
> > diff --git a/drivers/clk/actions/owl-s500.c b/drivers/clk/actions/owl-s500.c
> > new file mode 100644
> > index 000000000000..93feea8d71e2
> > --- /dev/null
> > +++ b/drivers/clk/actions/owl-s500.c
> > @@ -0,0 +1,524 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +//
>
> Please make the reset of these the typical /* type of comments.
>

Okay. I will do separate patchset for converting S700 and S900.

> > +// Actions Semi Owl S500 SoC clock driver
> > +//
> > +// Copyright (c) 2014 Actions Semi Inc.
> > +// Author: David Liu <liuwei@xxxxxxxxxxxxxxxx>
> > +//
> > +// Copyright (c) 2018 Linaro Ltd.
> > +// Author: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> > +//
> > +// Copyright (c) 2018 LSI-TEC - Caninos Loucos
> > +// Author: Edgar Bernardi Righi <edgar.righi@xxxxxxxxxxxxx>
> > +
> > +#include <linux/clk-provider.h>
> > +#include <linux/platform_device.h>
>
> I'm amazed nothing else is required to be included.
>

Most of the generic includes are added in the local header files. But
yeah, we need to declare those here also. Will do it as an incremental
patchset.

> [..]
> > +#define CMU_NANDPLLDEBUG (0x00E4)
> > +#define CMU_DISPLAYPLLDEBUG (0x00E8)
> > +#define CMU_TVOUTPLLDEBUG (0x00EC)
> > +#define CMU_DEEPCOLORPLLDEBUG (0x00F4)
> > +#define CMU_AUDIOPLL_ETHPLLDEBUG (0x00F8)
> > +#define CMU_CVBSPLLDEBUG (0x00FC)
> > +
> > +#define OWL_S500_COREPLL_DELAY (150)
> > +#define OWL_S500_DDRPLL_DELAY (63)
> > +#define OWL_S500_DEVPLL_DELAY (28)
> > +#define OWL_S500_NANDPLL_DELAY (44)
> > +#define OWL_S500_DISPLAYPLL_DELAY (57)
> > +#define OWL_S500_ETHERNETPLL_DELAY (25)
> > +#define OWL_S500_AUDIOPLL_DELAY (100)
> > +
> > +static const struct clk_pll_table clk_audio_pll_table[] = {
> > + { 0, 45158400 }, { 1, 49152000 },
> > + { 0, 0 },
> > +};
> > +
> > +/* pll clocks */
> > +static OWL_PLL_NO_PARENT_DELAY(ethernet_pll_clk, "ethernet_pll_clk", CMU_ETHERNETPLL, 500000000, 0, 0, 0, 0, 0, OWL_S500_ETHERNETPLL_DELAY, NULL, CLK_IGNORE_UNUSED);
> > +static OWL_PLL_NO_PARENT_DELAY(core_pll_clk, "core_pll_clk", CMU_COREPLL, 12000000, 9, 0, 8, 4, 134, OWL_S500_COREPLL_DELAY, NULL, CLK_IGNORE_UNUSED);
> > +static OWL_PLL_NO_PARENT_DELAY(ddr_pll_clk, "ddr_pll_clk", CMU_DDRPLL, 12000000, 8, 0, 8, 1, 67, OWL_S500_DDRPLL_DELAY, NULL, CLK_IGNORE_UNUSED);
> > +static OWL_PLL_NO_PARENT_DELAY(nand_pll_clk, "nand_pll_clk", CMU_NANDPLL, 6000000, 8, 0, 7, 2, 86, OWL_S500_NANDPLL_DELAY, NULL, CLK_IGNORE_UNUSED);
> > +static OWL_PLL_NO_PARENT_DELAY(display_pll_clk, "display_pll_clk", CMU_DISPLAYPLL, 6000000, 8, 0, 8, 2, 126, OWL_S500_DISPLAYPLL_DELAY, NULL, CLK_IGNORE_UNUSED);
> > +static OWL_PLL_NO_PARENT_DELAY(dev_pll_clk, "dev_pll_clk", CMU_DEVPLL, 6000000, 8, 0, 7, 8, 126, OWL_S500_DEVPLL_DELAY, NULL, CLK_IGNORE_UNUSED);
> > +static OWL_PLL_NO_PARENT_DELAY(audio_pll_clk, "audio_pll_clk", CMU_AUDIOPLL, 0, 4, 0, 1, 0, 0, OWL_S500_AUDIOPLL_DELAY, clk_audio_pll_table, CLK_IGNORE_UNUSED);
> > +
> > +static const char *dev_clk_mux_p[] = { "hosc", "dev_pll_clk" };
>
> These can't be const char * const ?
>

Ack.

> > +static const char *bisp_clk_mux_p[] = { "display_pll_clk", "dev_clk" };
> > +static const char *sensor_clk_mux_p[] = { "hosc", "bisp_clk" };
> > +static const char *sd_clk_mux_p[] = { "dev_clk", "nand_pll_clk" };
> > +static const char *pwm_clk_mux_p[] = { "losc", "hosc" };
> > +static const char *ahbprediv_clk_mux_p[] = { "dev_clk", "display_pll_clk", "nand_pll_clk", "ddr_pll_clk" };
> > +static const char *uart_clk_mux_p[] = { "hosc", "dev_pll_clk" };
> > +static const char *de_clk_mux_p[] = { "display_pll_clk", "dev_clk" };
> > +static const char *i2s_clk_mux_p[] = { "audio_pll_clk" };
> > +static const char *hde_clk_mux_p[] = { "dev_clk", "display_pll_clk", "nand_pll_clk", "ddr_pll_clk" };
> > +static const char *nand_clk_mux_p[] = { "nand_pll_clk", "display_pll_clk", "dev_clk", "ddr_pll_clk" };
> > +
> > +static struct clk_factor_table sd_factor_table[] = {
>
> Can these tables be const?
>

They can but sadly const will get discarded during assignments. I will
fix that in incremental patchset.

> > + /* bit0 ~ 4 */
> > + { 0, 1, 1 }, { 1, 1, 2 }, { 2, 1, 3 }, { 3, 1, 4 },
> > + { 4, 1, 5 }, { 5, 1, 6 }, { 6, 1, 7 }, { 7, 1, 8 },
> > + { 8, 1, 9 }, { 9, 1, 10 }, { 10, 1, 11 }, { 11, 1, 12 },
> > + { 12, 1, 13 }, { 13, 1, 14 }, { 14, 1, 15 }, { 15, 1, 16 },
> > + { 16, 1, 17 }, { 17, 1, 18 }, { 18, 1, 19 }, { 19, 1, 20 },
> > + { 20, 1, 21 }, { 21, 1, 22 }, { 22, 1, 23 }, { 23, 1, 24 },
> > + { 24, 1, 25 }, { 25, 1, 26 }, { 26, 1, 27 }, { 27, 1, 28 },
> > + { 28, 1, 29 }, { 29, 1, 30 }, { 30, 1, 31 }, { 31, 1, 32 },
> > +
> > + /* bit8: /128 */
> > + { 256, 1, 1 * 128 }, { 257, 1, 2 * 128 }, { 258, 1, 3 * 128 }, { 259, 1, 4 * 128 },
> > + { 260, 1, 5 * 128 }, { 261, 1, 6 * 128 }, { 262, 1, 7 * 128 }, { 263, 1, 8 * 128 },
> > + { 264, 1, 9 * 128 }, { 265, 1, 10 * 128 }, { 266, 1, 11 * 128 }, { 267, 1, 12 * 128 },
> > + { 268, 1, 13 * 128 }, { 269, 1, 14 * 128 }, { 270, 1, 15 * 128 }, { 271, 1, 16 * 128 },
> > + { 272, 1, 17 * 128 }, { 273, 1, 18 * 128 }, { 274, 1, 19 * 128 }, { 275, 1, 20 * 128 },
> > + { 276, 1, 21 * 128 }, { 277, 1, 22 * 128 }, { 278, 1, 23 * 128 }, { 279, 1, 24 * 128 },
> > + { 280, 1, 25 * 128 }, { 281, 1, 26 * 128 }, { 282, 1, 27 * 128 }, { 283, 1, 28 * 128 },
> > + { 284, 1, 29 * 128 }, { 285, 1, 30 * 128 }, { 286, 1, 31 * 128 }, { 287, 1, 32 * 128 },
> > + { 0, 0, 0 },
> > +};
> > +
> > +static struct clk_factor_table bisp_factor_table[] = {
> > + { 0, 1, 1 }, { 1, 1, 2 }, { 2, 1, 3 }, { 3, 1, 4 },
> > + { 4, 1, 5 }, { 5, 1, 6 }, { 6, 1, 7 }, { 7, 1, 8 },
> > + { 0, 0, 0 },
> > +};
> > +
> > +static struct clk_factor_table ahb_factor_table[] = {
> > + { 1, 1, 2 }, { 2, 1, 3 },
> > + { 0, 0, 0 },
> > +};
> > +
> > +static struct clk_div_table rmii_ref_div_table[] = {
> > + { 0, 4 }, { 1, 10 },
> > + { 0, 0 },
> > +};
> > +
> > +static struct clk_div_table i2s_div_table[] = {
> > + { 0, 1 }, { 1, 2 }, { 2, 3 }, { 3, 4 },
> > + { 4, 6 }, { 5, 8 }, { 6, 12 }, { 7, 16 },
> > + { 8, 24 },
> > + { 0, 0 },
> > +};
> > +
> > +static struct clk_div_table nand_div_table[] = {
> > + { 0, 1 }, { 1, 2 }, { 2, 4 }, { 3, 6 },
> > + { 4, 8 }, { 5, 10 }, { 6, 12 }, { 7, 14 },
> > + { 8, 16 }, { 9, 18 }, { 10, 20 }, { 11, 22 },
> > + { 0, 0 },
> > +};
> > +
> > +/* mux clock */
> > +static OWL_MUX(dev_clk, "dev_clk", dev_clk_mux_p, CMU_DEVPLL, 12, 1, CLK_SET_RATE_PARENT);
> > +static OWL_MUX(ahbprediv_clk, "ahbprediv_clk", ahbprediv_clk_mux_p, CMU_BUSCLK1, 8, 3, CLK_SET_RATE_PARENT);
> > +
> > +/* gate clocks */
> > +static OWL_GATE(spi0_clk, "spi0_clk", "ahb_clk", CMU_DEVCLKEN1, 10, 0, CLK_IGNORE_UNUSED);
> > +static OWL_GATE(spi1_clk, "spi1_clk", "ahb_clk", CMU_DEVCLKEN1, 11, 0, CLK_IGNORE_UNUSED);
> [...]
> > + [CLK_HDMI] = &hdmi_clk.common.hw,
> > + [CLK_VDE] = &vde_clk.common.hw,
> > + [CLK_VCE] = &vce_clk.common.hw,
> > + [CLK_SPDIF] = &spdif_clk.common.hw,
> > + [CLK_NAND] = &nand_clk.common.hw,
> > + [CLK_ECC] = &ecc_clk.common.hw,
> > + },
> > + .num = CLK_NR_CLKS,
> > +};
> > +
> > +static struct owl_clk_desc s500_clk_desc = {
>
> This can't be const?
>

Same as above.

Thanks,
Mani

> > + .clks = s500_clks,
> > + .num_clks = ARRAY_SIZE(s500_clks),
> > +
> > + .hw_clks = &s500_hw_clks,
> > +};
> > +