Re: [linux-sunxi] Re: [PATCH v4 3/3] clk: sunxi-ng: support R40 SoC

[Chen-Yu Tsai]

On Tue, Aug 15, 2017 at 12:53 AM,  <icenowy@xxxxxxx> wrote:
> å 2017-08-15 00:12ïChen-Yu Tsai åéï
>>
>> )On Sat, Aug 12, 2017 at 8:43 PM, Icenowy Zheng <icenowy@xxxxxxx> wrote:
>>>
>>> Allwinner R40 SoC have a clock controller module in the style of the
>>> SoCs beyond sun6i, however, it's more rich and complex.
>>>
>>> Add support for it.
>>>
>>> Signed-off-by: Icenowy Zheng <icenowy@xxxxxxx>
>>> ---
>>> Changes in v4:
>>> - Removed usb-ohci-12M mux clocks.
>>> - Removed unused (and not in user manual) adda-4x clock.
>>> - Implemented proper SATA PLL system.
>>> - Renamed MP (Mixed Processor) clock names to drop the extra "DE_".
>>> - Renamed TCONs' clock names to "tcon-lcdX" or "tcon-tvX".
>>> - Added missing RST_DRAM.
>>> - Several clock post/pre-dividers and constraints fixes.
>>> Changes in v3:
>>> - Rebased on current linux-next.
>>> Changes in v2:
>>> - Fixes according to the SoC's user manual.
>>>
>>>  drivers/clk/sunxi-ng/Kconfig              |    5 +
>>>  drivers/clk/sunxi-ng/Makefile             |    1 +
>>>  drivers/clk/sunxi-ng/ccu-sun8i-r40.c      | 1240
>>> +++++++++++++++++++++++++++++
>>>  drivers/clk/sunxi-ng/ccu-sun8i-r40.h      |   69 ++
>>>  include/dt-bindings/clock/sun8i-r40-ccu.h |  187 +++++
>>>  include/dt-bindings/reset/sun8i-r40-ccu.h |  130 +++
>>>  6 files changed, 1632 insertions(+)
>>>  create mode 100644 drivers/clk/sunxi-ng/ccu-sun8i-r40.c
>>>  create mode 100644 drivers/clk/sunxi-ng/ccu-sun8i-r40.h
>>>  create mode 100644 include/dt-bindings/clock/sun8i-r40-ccu.h
>>>  create mode 100644 include/dt-bindings/reset/sun8i-r40-ccu.h
>>>
>>> diff --git a/drivers/clk/sunxi-ng/Kconfig b/drivers/clk/sunxi-ng/Kconfig
>>> index 7342928c35cd..7a360737fe3c 100644
>>> --- a/drivers/clk/sunxi-ng/Kconfig
>>> +++ b/drivers/clk/sunxi-ng/Kconfig
>>> @@ -48,6 +48,11 @@ config SUN8I_V3S_CCU
>>>  config SUN8I_DE2_CCU
>>>         bool "Support for the Allwinner SoCs DE2 CCU"
>>>
>>> +config SUN8I_R40_CCU
>>> +       bool "Support for the Allwinner R40 CCU"
>>> +       default MACH_SUN8I
>>> +       depends on MACH_SUN8I || COMPILE_TEST
>>> +
>>>  config SUN9I_A80_CCU
>>>         bool "Support for the Allwinner A80 CCU"
>>>         default MACH_SUN9I
>>> diff --git a/drivers/clk/sunxi-ng/Makefile
>>> b/drivers/clk/sunxi-ng/Makefile
>>> index 45a5910379a5..b1267fe68a8f 100644
>>> --- a/drivers/clk/sunxi-ng/Makefile
>>> +++ b/drivers/clk/sunxi-ng/Makefile
>>> @@ -29,6 +29,7 @@ obj-$(CONFIG_SUN8I_H3_CCU)    += ccu-sun8i-h3.o
>>>  obj-$(CONFIG_SUN8I_V3S_CCU)    += ccu-sun8i-v3s.o
>>>  obj-$(CONFIG_SUN8I_DE2_CCU)    += ccu-sun8i-de2.o
>>>  obj-$(CONFIG_SUN8I_R_CCU)      += ccu-sun8i-r.o
>>> +obj-$(CONFIG_SUN8I_R40_CCU)    += ccu-sun8i-r40.o
>>>  obj-$(CONFIG_SUN9I_A80_CCU)    += ccu-sun9i-a80.o
>>>  obj-$(CONFIG_SUN9I_A80_CCU)    += ccu-sun9i-a80-de.o
>>>  obj-$(CONFIG_SUN9I_A80_CCU)    += ccu-sun9i-a80-usb.o
>>> diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-r40.c
>>> b/drivers/clk/sunxi-ng/ccu-sun8i-r40.c
>>> new file mode 100644
>>> index 000000000000..9dc6ad419f9a
>>> --- /dev/null
>>> +++ b/drivers/clk/sunxi-ng/ccu-sun8i-r40.c
>>
>>
>> [...]
>>
>>> +/*
>>> + * The MIPI PLL has 2 modes: "MIPI" and "HDMI".
>>> + *
>>> + * The MIPI mode is a standard NKM-style clock. The HDMI mode is an
>>> + * integer / fractional clock with switchable multipliers and dividers.
>>> + * This is not supported here. We hardcode the PLL to MIPI mode.
>>> + */
>>> +#define SUN8I_R40_PLL_MIPI_REG 0x040
>>> +
>>> +static const char * const pll_mipi_parents[] = { "pll-video0" };
>>> +static SUNXI_CCU_NKM_WITH_MUX_GATE_LOCK(pll_mipi_clk, "pll-mipi",
>>> +                                       pll_mipi_parents, 0x040,
>>> +                                       8, 4,   /* N */
>>> +                                       4, 2,   /* K */
>>> +                                       0, 4,   /* M */
>>> +                                       21, 1,  /* mux */
>>> +                                       BIT(31) | BIT(23) | BIT(22), /*
>>> gate */
>>> +                                       BIT(28),        /* lock */
>>> +                                       CLK_SET_RATE_UNGATE);
>>
>>
>> K has a minimum of 2 here.
>
>
> I think it's not mentioned on the user manual...
>
> It says "The range is from 1 to 4."

My R40 user manual v1.0 says otherwise. Page 85: PLL_FACTOR_K
range is from 2 to 4.

>
>>
>>> +
>>> +static SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK(pll_de_clk, "pll-de",
>>> +                                       "osc24M", 0x0048,
>>> +                                       8, 7,           /* N */
>>> +                                       0, 4,           /* M */
>>> +                                       BIT(24),        /* frac enable */
>>> +                                       BIT(25),        /* frac select */
>>> +                                       270000000,      /* frac rate 0 */
>>> +                                       297000000,      /* frac rate 1 */
>>> +                                       BIT(31),        /* gate */
>>> +                                       BIT(28),        /* lock */
>>> +                                       0);
>>
>>
>> CLK_SET_RATE_UNGATE should be set for _all_ PLLs. The lock status bit
>> depends on the PLL running, and we check the lock status whenever set_rate
>> is done.
>
>
> Should we also do this on older SoCs?
>
> I think at least on H3 I met PLL-DE waiting for lock failure.

We probably should. But let us focus on this driver for now.

>
>>
>>> +
>>> +static SUNXI_CCU_NM_WITH_GATE_LOCK(pll_ddr1_clk, "pll-ddr1",
>>> +                                  "osc24M", 0x04c,
>>> +                                  8, 7,        /* N */
>>> +                                  0, 2,        /* M */
>>> +                                  BIT(31),     /* gate */
>>> +                                  BIT(28),     /* lock */
>>> +                                  CLK_SET_RATE_UNGATE);
>>
>>
>> Manual says 16 <= N <= 75.
>
>
> Maybe we need to implement more things for this constraint?
>
> But I think the frequency is not going to be changed when
> running...

Either you implement it now, or leave a note saying it has
said restrictions. I am in favor of implementing it now.
That said, some other PLLs have restrictions on N*K. That
we cannot implement at this time.

>
>>
>> [...]
>>
>>> +
>>> +static struct clk_div_table ths_div_table[] = {
>>> +       { .val = 0, .div = 1 },
>>> +       { .val = 1, .div = 2 },
>>> +       { .val = 2, .div = 4 },
>>> +       { .val = 3, .div = 8 },
>>> +};
>>> +static const char * const ths_parents[] = { "osc24M" };
>>> +static struct ccu_div ths_clk = {
>>> +       .enable = BIT(31),
>>> +       .div    = _SUNXI_CCU_DIV_TABLE(0, 2, ths_div_table),
>>
>>
>> You could use _SUNXI_CCU_DIV_FLAGS(0, 2, CLK_DIVIDER_POWER_OF_TWO)
>> instead of div_table.
>
>
> Thanks.
>
>>
>>> +       .mux    = _SUNXI_CCU_MUX(24, 2),
>>> +       .common = {
>>> +               .reg            = 0x074,
>>> +               .hw.init        = CLK_HW_INIT_PARENTS("ths",
>>> +                                                     ths_parents,
>>> +                                                     &ccu_div_ops,
>>> +                                                     0),
>>> +       },
>>> +};
>>
>>
>> [...]
>>
>>> +static const char * const keypad_parents[] = { "osc24M", "osc32k", };
>>
>>
>> Nit: you can drop the extra comma.
>
>
> Thanks.
>
>>
>> [...]
>>
>>> +static const char * const sata_parents[] = { "pll-sata-out", "sata-ext"
>>> };
>>> +static SUNXI_CCU_MUX_WITH_GATE(sata_clk, "sata", sata_parents,
>>> +                              0x0c8, 24, 1, BIT(31),
>>> CLK_SET_RATE_PARENT);
>>
>>
>> You don't have CLK_SET_RATE_PARENT for the parent, which is also a
>> mux+gate.
>> I'm not sure what you are expecting to happen here.
>
>
> Oh maybe I should add it there.

Adding it there may affect pll-periph0.

>>
>>> +
>>> +static SUNXI_CCU_GATE(usb_phy0_clk,    "usb-phy0",     "osc24M",
>>> +                     0x0cc, BIT(8), 0);
>>> +static SUNXI_CCU_GATE(usb_phy1_clk,    "usb-phy1",     "osc24M",
>>> +                     0x0cc, BIT(9), 0);
>>> +static SUNXI_CCU_GATE(usb_phy2_clk,    "usb-phy2",     "osc24M",
>>> +                     0x0cc, BIT(10), 0);
>>> +static SUNXI_CCU_GATE(usb_ohci0_clk,   "usb-ohci0",    "osc12M",
>>> +                     0x0cc, BIT(16), 0);
>>> +static SUNXI_CCU_GATE(usb_ohci1_clk,   "usb-ohci1",    "osc12M",
>>> +                     0x0cc, BIT(17), 0);
>>> +static SUNXI_CCU_GATE(usb_ohci2_clk,   "usb-ohci2",    "osc12M",
>>> +                     0x0cc, BIT(18), 0);
>>
>>
>> Maybe you should force the OHCI muxes to 0?
>
>
> This seems to be not done in A64 driver.
>
> P.S. I changed this block to make it similar to the A64 driver.

Then you should at least leave a note saying this is unimplemented
and we are depending on the hardware default.

>
>>
>> [...]
>>
>>> +static const char * const tcon_parents[] = { "pll-video0", "pll-video1",
>>> +                                            "pll-video0-2x",
>>> "pll-video1-2x",
>>> +                                            "pll-mipi" };
>>> +static SUNXI_CCU_MUX_WITH_GATE(tcon_lcd0_clk, "tcon-lcd0", tcon_parents,
>>> +                              0x110, 24, 3, BIT(31),
>>> CLK_SET_RATE_PARENT);
>>> +static SUNXI_CCU_MUX_WITH_GATE(tcon_lcd1_clk, "tcon-lcd1", tcon_parents,
>>> +                              0x114, 24, 3, BIT(31),
>>> CLK_SET_RATE_PARENT);
>>> +static SUNXI_CCU_M_WITH_MUX_GATE(tcon_tv0_clk, "tcon-tv0", tcon_parents,
>>> +                                0x118, 0, 4, 24, 3, BIT(31), 0);
>>> +static SUNXI_CCU_M_WITH_MUX_GATE(tcon_tv1_clk, "tcon-tv1", tcon_parents,
>>> +                                0x11c, 0, 4, 24, 3, BIT(31), 0);
>>
>>
>> You will likely need CLK_SET_RATE_PARENT for tcon-tv* as well.
>
>
> According to the experience on H3, the TCONs may be more complex.
>
> Leave 0 here until we have either HDMI driver or TVE driver.

OK.

>
>>
>> [...]
>>
>>> +static const char * const csi_mclk_parents[] = { "osc24M", "pll-video",
>>
>>
>> pll-video1?
>
>
> oh yes, thanks.
>
>>
>>> +                                                "pll-periph1" };
>>> +static SUNXI_CCU_M_WITH_MUX_GATE(csi1_mclk_clk, "csi1-mclk",
>>> csi_mclk_parents,
>>> +                                0x130, 0, 5, 8, 3, BIT(15), 0);
>>> +
>>> +static const char * const csi_sclk_parents[] = { "pll-periph0",
>>> "pll-periph1" };
>>> +static SUNXI_CCU_M_WITH_MUX_GATE(csi_sclk_clk, "csi-sclk",
>>> csi_sclk_parents,
>>> +                                0x134, 16, 4, 24, 3, BIT(31), 0);
>>> +
>>> +static SUNXI_CCU_M_WITH_MUX_GATE(csi0_mclk_clk, "csi0-mclk",
>>> csi_mclk_parents,
>>> +                                0x134, 0, 5, 8, 3, BIT(15), 0);
>>
>>
>> [...]
>>
>>> +
>>> +static SUNXI_CCU_M_WITH_GATE(ve_clk, "ve", "pll-ve",
>>> +                            0x13c, 16, 3, BIT(31), 0);
>>
>>
>> CLK_SET_RATE_PARENT?
>
>
> Seems ok.
>
>>
>>> +
>>> +static SUNXI_CCU_GATE(adda_clk,                "adda",
>>> "pll-audio",
>>> +                     0x140, BIT(31), CLK_SET_RATE_PARENT);
>>
>>
>> You call the bus clock "bus-codec", but here you call it "adda". Pick one
>> and
>> be consistent.
>
>
> Use codec?

Cool.

>
>>
>> [...]
>>
>>> +static const char * const hdmi_parents[] = { "pll-video0", "pll-video1"
>>> };
>>> +static SUNXI_CCU_M_WITH_MUX_GATE(hdmi_clk, "hdmi", hdmi_parents,
>>> +                                0x150, 0, 4, 24, 2, BIT(31), 0);
>>
>>
>> CLK_SET_RATE_PARENT?
>
>
> Wait for HDMI driver.

OK.

>
>>
>>
>> [...]
>>
>>> +static const char * const mbus_parents[] = { "osc24M", "pll-periph0-2x",
>>> +                                            "pll-ddr0" };
>>> +static SUNXI_CCU_MP_WITH_MUX_GATE(mbus_clk, "mbus", mbus_parents, 0x15c,
>>> +                                 0, 4,         /* M */
>>> +                                 16, 2,        /* P */
>>> +                                 24, 2,        /* mux */
>>> +                                 BIT(31),      /* gate */
>>> +                                 CLK_IS_CRITICAL);
>>
>>
>> The documentation for this one is fuzzy. It mentions the N (or P) factor,
>> but does not include it in the formula. Can you double check it against
>> the BSP and add a comment about it?
>
>
> ```
> SUNXI_CLK_PERIPH(mbus, MBUS_CFG, 24, 2, MBUS_CFG, 0, 4, 16, 2, 0, MBUS_CFG,
>                         MBUS_RST, 0, 0, 31, 31, 0, 0, &clk_lock, NULL, 0);
> ```
> I think the BSP driver used both factors, however, the driver may be also
> not accurate as I think maybe mbus is not going to be changed even in BSP
> kernel.

OK. Please add a comment saying that the doc is fuzzy and you're following
the BSP on this one.

>
>>
>>> +
>>> +static const char * const dsi_dphy_parents[] = { "pll-video0",
>>> "pll-video1",
>>> +                                                "pll-periph0" };
>>> +static SUNXI_CCU_M_WITH_MUX_GATE(dsi_dphy_clk, "dsi-dphy",
>>> dsi_dphy_parents,
>>> +                                0x168, 0, 4, 8, 2, BIT(15), 0);
>>
>>
>> Note: we might need CLK_SET_RATE_PARENT later on, but we will need to find
>> a way to keep pll-periph* fixed.
>>
>>> +
>>> +static SUNXI_CCU_M_WITH_MUX_GATE(tve0_clk, "tve0", tcon_parents,
>>> +                                0x180, 0, 4, 24, 3, BIT(31), 0);
>>> +static SUNXI_CCU_M_WITH_MUX_GATE(tve1_clk, "tve1", tcon_parents,
>>> +                                0x184, 0, 4, 24, 3, BIT(31), 0);
>>
>>
>> CLK_SET_RATE_PARENT?
>
>
> Wait for the real driver -- it may be more complex.

OK.

>
>>
>>> +
>>> +static const char * const tvd_parents[] = { "pll-video0", "pll-video1",
>>> +                                           "pll-video0-2x",
>>> "pll-video1-2x" };
>>> +static SUNXI_CCU_M_WITH_MUX_GATE(tvd0_clk, "tvd0", tvd_parents,
>>> +                                0x188, 0, 4, 24, 3, BIT(31), 0);
>>> +static SUNXI_CCU_M_WITH_MUX_GATE(tvd1_clk, "tvd1", tvd_parents,
>>> +                                0x18c, 0, 4, 24, 3, BIT(31), 0);
>>> +static SUNXI_CCU_M_WITH_MUX_GATE(tvd2_clk, "tvd2", tvd_parents,
>>> +                                0x190, 0, 4, 24, 3, BIT(31), 0);
>>> +static SUNXI_CCU_M_WITH_MUX_GATE(tvd3_clk, "tvd3", tvd_parents,
>>> +                                0x194, 0, 4, 24, 3, BIT(31), 0);
>>
>>
>> CLK_SET_RATE_PARENT?
>
>
> Not sure at all, maybe they should be changed when really doing TVD
> driver.

OK.

>
>>
>> [...]
>>
>> You are missing the SYS_32k mux at register 0x310. According to
>> the diagrams, this is the actual clock that supplies the LOSC
>> parent of various muxes in the CCU. It itself is a mux between
>> an internal 2M RC oscillator, and LOSC from the RTC module.
>>
>>> +
>>> +static struct ccu_reset_map sun8i_r40_ccu_resets[] = {
>>
>>
>> [...]
>>
>>> +       [RST_BUS_HDMI_SLOW]     =  { 0x2c4, BIT(10) },
>>> +       [RST_BUS_HDMI]          =  { 0x2c4, BIT(11) },
>>
>>
>> Manual says bit 10 is HDMI0, bit 11 is HDMI1. Looking at the clocks,
>> bit 10 would be RST_BUS_HDMI, while bit 11 would be RST_BUS_HDMI_SLOW.
>
>
> Let me just call them 0 and 1 in next version.

OK.

>
>
>>
>> [...]
>>
>>> +
>>> +static const struct sunxi_ccu_desc sun8i_r40_ccu_desc = {
>>> +       .ccu_clks       = sun8i_r40_ccu_clks,
>>> +       .num_ccu_clks   = ARRAY_SIZE(sun8i_r40_ccu_clks),
>>> +
>>> +       .hw_clks        = &sun8i_r40_hw_clks,
>>> +
>>> +       .resets         = sun8i_r40_ccu_resets,
>>> +       .num_resets     = ARRAY_SIZE(sun8i_r40_ccu_resets),
>>> +};
>>> +
>>> +static struct ccu_mux_nb sun8i_r40_cpu_nb = {
>>> +       .common         = &cpu_clk.common,
>>> +       .cm             = &cpu_clk.mux,
>>> +       .delay_us       = 1, /* > 8 clock cycles at 24 MHz */
>>> +       .bypass_index   = 1, /* index of 24 MHz oscillator */
>>> +};
>>> +
>>> +static void __init sun8i_r40_ccu_setup(struct device_node *node)
>>> +{
>>> +       void __iomem *reg;
>>> +       u32 val;
>>> +
>>> +       reg = of_io_request_and_map(node, 0, of_node_full_name(node));
>>> +       if (IS_ERR(reg)) {
>>> +               pr_err("%s: Could not map the clock registers\n",
>>> +                      of_node_full_name(node));
>>> +               return;
>>> +       }
>>> +
>>> +       /* Force the PLL-Audio-1x divider to 4 */
>>> +       val = readl(reg + SUN8I_R40_PLL_AUDIO_REG);
>>> +       val &= ~GENMASK(19, 16);
>>> +       writel(val | (3 << 16), reg + SUN8I_R40_PLL_AUDIO_REG);
>>
>>
>> You should also force pll-mipi to mipi mode. And the OHCI muxes.
>>
>>> +
>>> +       sunxi_ccu_probe(node, reg, &sun8i_r40_ccu_desc);
>>> +
>>> +       ccu_mux_notifier_register(pll_cpu_clk.common.hw.clk,
>>> +                                 &sun8i_r40_cpu_nb);
>>
>>
>> Do you also need the gate-then-ungate notifier?
>
>
> Still unknown...

OK.

Thanks
ChenYu