diff mbox series

[v4,1/2] clk: rockchip: rk3288: Handle clock tree for rk3288w

Message ID 20200602080644.11333-2-mylene.josserand@collabora.com (mailing list archive)
State Mainlined
Commit 1627f683636df70fb25358b0a7b39a24e8fce5bf
Headers show
Series ARM: Add Rockchip rk3288w support | expand

Commit Message

Mylene Josserand June 2, 2020, 8:06 a.m. UTC
The revision rk3288w has a different clock tree about "hclk_vio"
clock, according to the BSP kernel code.

This patch handles this difference by detecting which device-tree
we are using. If it is a "rockchip,rk3288-cru", let's register
the clock tree as it was before. If the device-tree node is
"rockchip,rk3288w-cru", we will apply the difference with this
version of this SoC.

Noticed that this new device-tree compatible must be handled in
bootloader such as u-boot.

Signed-off-by: Mylène Josserand <mylene.josserand@collabora.com>
---
 drivers/clk/rockchip/clk-rk3288.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

Comments

Jagan Teki June 29, 2020, 7:11 p.m. UTC | #1
On Tue, Jun 2, 2020 at 1:37 PM Mylène Josserand
<mylene.josserand@collabora.com> wrote:
>
> The revision rk3288w has a different clock tree about "hclk_vio"
> clock, according to the BSP kernel code.
>
> This patch handles this difference by detecting which device-tree
> we are using. If it is a "rockchip,rk3288-cru", let's register
> the clock tree as it was before. If the device-tree node is
> "rockchip,rk3288w-cru", we will apply the difference with this
> version of this SoC.
>
> Noticed that this new device-tree compatible must be handled in
> bootloader such as u-boot.
>
> Signed-off-by: Mylène Josserand <mylene.josserand@collabora.com>
> ---
>  drivers/clk/rockchip/clk-rk3288.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/rockchip/clk-rk3288.c b/drivers/clk/rockchip/clk-rk3288.c
> index cc2a177bbdbf..204976e2d0cb 100644
> --- a/drivers/clk/rockchip/clk-rk3288.c
> +++ b/drivers/clk/rockchip/clk-rk3288.c
> @@ -425,8 +425,6 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = {
>         COMPOSITE(0, "aclk_vio0", mux_pll_src_cpll_gpll_usb480m_p, CLK_IGNORE_UNUSED,
>                         RK3288_CLKSEL_CON(31), 6, 2, MFLAGS, 0, 5, DFLAGS,
>                         RK3288_CLKGATE_CON(3), 0, GFLAGS),
> -       DIV(0, "hclk_vio", "aclk_vio0", 0,
> -                       RK3288_CLKSEL_CON(28), 8, 5, DFLAGS),
>         COMPOSITE(0, "aclk_vio1", mux_pll_src_cpll_gpll_usb480m_p, CLK_IGNORE_UNUSED,
>                         RK3288_CLKSEL_CON(31), 14, 2, MFLAGS, 8, 5, DFLAGS,
>                         RK3288_CLKGATE_CON(3), 2, GFLAGS),
> @@ -819,6 +817,16 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = {
>         INVERTER(0, "pclk_isp", "pclk_isp_in", RK3288_CLKSEL_CON(29), 3, IFLAGS),
>  };
>
> +static struct rockchip_clk_branch rk3288w_hclkvio_branch[] __initdata = {
> +       DIV(0, "hclk_vio", "aclk_vio1", 0,
> +                       RK3288_CLKSEL_CON(28), 8, 5, DFLAGS),
> +};
> +
> +static struct rockchip_clk_branch rk3288_hclkvio_branch[] __initdata = {
> +       DIV(0, "hclk_vio", "aclk_vio0", 0,
> +                       RK3288_CLKSEL_CON(28), 8, 5, DFLAGS),
> +};
> +
>  static const char *const rk3288_critical_clocks[] __initconst = {
>         "aclk_cpu",
>         "aclk_peri",
> @@ -936,6 +944,14 @@ static void __init rk3288_clk_init(struct device_node *np)
>                                    RK3288_GRF_SOC_STATUS1);
>         rockchip_clk_register_branches(ctx, rk3288_clk_branches,
>                                   ARRAY_SIZE(rk3288_clk_branches));
> +
> +       if (of_device_is_compatible(np, "rockchip,rk3288w-cru"))
> +               rockchip_clk_register_branches(ctx, rk3288w_hclkvio_branch,
> +                                              ARRAY_SIZE(rk3288w_hclkvio_branch));
> +       else
> +               rockchip_clk_register_branches(ctx, rk3288_hclkvio_branch,
> +                                              ARRAY_SIZE(rk3288_hclkvio_branch));
> +

Sorry for the late query on this. I am a bit unclear about this
compatible change, does Linux expect to replace rockchip,rk3288-cru
with rockchip,rk3288w-cru in bootloader if the chip is RK3288w? or
append the existing cru compatible node with rockchip,rk3288w-cru?
because replace new cru node make clock never probe since the
CLK_OF_DECLARE checking rockchip,rk3288-cru

Jagan.
Heiko Stübner July 3, 2020, 2:11 p.m. UTC | #2
Hi Jagan,

Am Montag, 29. Juni 2020, 21:11:03 CEST schrieb Jagan Teki:
> On Tue, Jun 2, 2020 at 1:37 PM Mylène Josserand
> <mylene.josserand@collabora.com> wrote:
> >
> > The revision rk3288w has a different clock tree about "hclk_vio"
> > clock, according to the BSP kernel code.
> >
> > This patch handles this difference by detecting which device-tree
> > we are using. If it is a "rockchip,rk3288-cru", let's register
> > the clock tree as it was before. If the device-tree node is
> > "rockchip,rk3288w-cru", we will apply the difference with this
> > version of this SoC.
> >
> > Noticed that this new device-tree compatible must be handled in
> > bootloader such as u-boot.
> >
> > Signed-off-by: Mylène Josserand <mylene.josserand@collabora.com>
> > ---
> >  drivers/clk/rockchip/clk-rk3288.c | 20 ++++++++++++++++++--
> >  1 file changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/clk/rockchip/clk-rk3288.c b/drivers/clk/rockchip/clk-rk3288.c
> > index cc2a177bbdbf..204976e2d0cb 100644
> > --- a/drivers/clk/rockchip/clk-rk3288.c
> > +++ b/drivers/clk/rockchip/clk-rk3288.c
> > @@ -425,8 +425,6 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = {
> >         COMPOSITE(0, "aclk_vio0", mux_pll_src_cpll_gpll_usb480m_p, CLK_IGNORE_UNUSED,
> >                         RK3288_CLKSEL_CON(31), 6, 2, MFLAGS, 0, 5, DFLAGS,
> >                         RK3288_CLKGATE_CON(3), 0, GFLAGS),
> > -       DIV(0, "hclk_vio", "aclk_vio0", 0,
> > -                       RK3288_CLKSEL_CON(28), 8, 5, DFLAGS),
> >         COMPOSITE(0, "aclk_vio1", mux_pll_src_cpll_gpll_usb480m_p, CLK_IGNORE_UNUSED,
> >                         RK3288_CLKSEL_CON(31), 14, 2, MFLAGS, 8, 5, DFLAGS,
> >                         RK3288_CLKGATE_CON(3), 2, GFLAGS),
> > @@ -819,6 +817,16 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = {
> >         INVERTER(0, "pclk_isp", "pclk_isp_in", RK3288_CLKSEL_CON(29), 3, IFLAGS),
> >  };
> >
> > +static struct rockchip_clk_branch rk3288w_hclkvio_branch[] __initdata = {
> > +       DIV(0, "hclk_vio", "aclk_vio1", 0,
> > +                       RK3288_CLKSEL_CON(28), 8, 5, DFLAGS),
> > +};
> > +
> > +static struct rockchip_clk_branch rk3288_hclkvio_branch[] __initdata = {
> > +       DIV(0, "hclk_vio", "aclk_vio0", 0,
> > +                       RK3288_CLKSEL_CON(28), 8, 5, DFLAGS),
> > +};
> > +
> >  static const char *const rk3288_critical_clocks[] __initconst = {
> >         "aclk_cpu",
> >         "aclk_peri",
> > @@ -936,6 +944,14 @@ static void __init rk3288_clk_init(struct device_node *np)
> >                                    RK3288_GRF_SOC_STATUS1);
> >         rockchip_clk_register_branches(ctx, rk3288_clk_branches,
> >                                   ARRAY_SIZE(rk3288_clk_branches));
> > +
> > +       if (of_device_is_compatible(np, "rockchip,rk3288w-cru"))
> > +               rockchip_clk_register_branches(ctx, rk3288w_hclkvio_branch,
> > +                                              ARRAY_SIZE(rk3288w_hclkvio_branch));
> > +       else
> > +               rockchip_clk_register_branches(ctx, rk3288_hclkvio_branch,
> > +                                              ARRAY_SIZE(rk3288_hclkvio_branch));
> > +
> 
> Sorry for the late query on this. I am a bit unclear about this
> compatible change, does Linux expect to replace rockchip,rk3288-cru
> with rockchip,rk3288w-cru in bootloader if the chip is RK3288w? or
> append the existing cru compatible node with rockchip,rk3288w-cru?
> because replace new cru node make clock never probe since the
> CLK_OF_DECLARE checking rockchip,rk3288-cru

I guess right now we'd expect "rockchip,rk3288w-cru", "rockchip,rk3288-cru",

Thinking again about this, I'm wondering if we should switch to having
only one per variant ... like on the two rk3188 variants,
so declaring separate rk3288-cru and rk3288w-cru of-clks with shared
common code.
Jagan Teki July 3, 2020, 2:23 p.m. UTC | #3
On Fri, Jul 3, 2020 at 7:41 PM Heiko Stuebner <heiko@sntech.de> wrote:
>
> Hi Jagan,
>
> Am Montag, 29. Juni 2020, 21:11:03 CEST schrieb Jagan Teki:
> > On Tue, Jun 2, 2020 at 1:37 PM Mylène Josserand
> > <mylene.josserand@collabora.com> wrote:
> > >
> > > The revision rk3288w has a different clock tree about "hclk_vio"
> > > clock, according to the BSP kernel code.
> > >
> > > This patch handles this difference by detecting which device-tree
> > > we are using. If it is a "rockchip,rk3288-cru", let's register
> > > the clock tree as it was before. If the device-tree node is
> > > "rockchip,rk3288w-cru", we will apply the difference with this
> > > version of this SoC.
> > >
> > > Noticed that this new device-tree compatible must be handled in
> > > bootloader such as u-boot.
> > >
> > > Signed-off-by: Mylène Josserand <mylene.josserand@collabora.com>
> > > ---
> > >  drivers/clk/rockchip/clk-rk3288.c | 20 ++++++++++++++++++--
> > >  1 file changed, 18 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/clk/rockchip/clk-rk3288.c b/drivers/clk/rockchip/clk-rk3288.c
> > > index cc2a177bbdbf..204976e2d0cb 100644
> > > --- a/drivers/clk/rockchip/clk-rk3288.c
> > > +++ b/drivers/clk/rockchip/clk-rk3288.c
> > > @@ -425,8 +425,6 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = {
> > >         COMPOSITE(0, "aclk_vio0", mux_pll_src_cpll_gpll_usb480m_p, CLK_IGNORE_UNUSED,
> > >                         RK3288_CLKSEL_CON(31), 6, 2, MFLAGS, 0, 5, DFLAGS,
> > >                         RK3288_CLKGATE_CON(3), 0, GFLAGS),
> > > -       DIV(0, "hclk_vio", "aclk_vio0", 0,
> > > -                       RK3288_CLKSEL_CON(28), 8, 5, DFLAGS),
> > >         COMPOSITE(0, "aclk_vio1", mux_pll_src_cpll_gpll_usb480m_p, CLK_IGNORE_UNUSED,
> > >                         RK3288_CLKSEL_CON(31), 14, 2, MFLAGS, 8, 5, DFLAGS,
> > >                         RK3288_CLKGATE_CON(3), 2, GFLAGS),
> > > @@ -819,6 +817,16 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = {
> > >         INVERTER(0, "pclk_isp", "pclk_isp_in", RK3288_CLKSEL_CON(29), 3, IFLAGS),
> > >  };
> > >
> > > +static struct rockchip_clk_branch rk3288w_hclkvio_branch[] __initdata = {
> > > +       DIV(0, "hclk_vio", "aclk_vio1", 0,
> > > +                       RK3288_CLKSEL_CON(28), 8, 5, DFLAGS),
> > > +};
> > > +
> > > +static struct rockchip_clk_branch rk3288_hclkvio_branch[] __initdata = {
> > > +       DIV(0, "hclk_vio", "aclk_vio0", 0,
> > > +                       RK3288_CLKSEL_CON(28), 8, 5, DFLAGS),
> > > +};
> > > +
> > >  static const char *const rk3288_critical_clocks[] __initconst = {
> > >         "aclk_cpu",
> > >         "aclk_peri",
> > > @@ -936,6 +944,14 @@ static void __init rk3288_clk_init(struct device_node *np)
> > >                                    RK3288_GRF_SOC_STATUS1);
> > >         rockchip_clk_register_branches(ctx, rk3288_clk_branches,
> > >                                   ARRAY_SIZE(rk3288_clk_branches));
> > > +
> > > +       if (of_device_is_compatible(np, "rockchip,rk3288w-cru"))
> > > +               rockchip_clk_register_branches(ctx, rk3288w_hclkvio_branch,
> > > +                                              ARRAY_SIZE(rk3288w_hclkvio_branch));
> > > +       else
> > > +               rockchip_clk_register_branches(ctx, rk3288_hclkvio_branch,
> > > +                                              ARRAY_SIZE(rk3288_hclkvio_branch));
> > > +
> >
> > Sorry for the late query on this. I am a bit unclear about this
> > compatible change, does Linux expect to replace rockchip,rk3288-cru
> > with rockchip,rk3288w-cru in bootloader if the chip is RK3288w? or
> > append the existing cru compatible node with rockchip,rk3288w-cru?
> > because replace new cru node make clock never probe since the
> > CLK_OF_DECLARE checking rockchip,rk3288-cru
>
> I guess right now we'd expect "rockchip,rk3288w-cru", "rockchip,rk3288-cru",
>
> Thinking again about this, I'm wondering if we should switch to having
> only one per variant ... like on the two rk3188 variants,
> so declaring separate rk3288-cru and rk3288w-cru of-clks with shared
> common code.

What if can check the root compatible instead cru compatible for revision W like

-  if (of_device_is_compatible(np, "rockchip,rk3288w-cru"))
+ if (of_device_is_compatible(np, "rockchip,rk3288w"))

This way we can have a single compatible update at bootloader that
makes Linux adjust revision W chips code.

Doesn't it make sense?

Jagan.
Heiko Stübner July 3, 2020, 2:48 p.m. UTC | #4
Am Freitag, 3. Juli 2020, 16:23:27 CEST schrieb Jagan Teki:
> On Fri, Jul 3, 2020 at 7:41 PM Heiko Stuebner <heiko@sntech.de> wrote:
> >
> > Hi Jagan,
> >
> > Am Montag, 29. Juni 2020, 21:11:03 CEST schrieb Jagan Teki:
> > > On Tue, Jun 2, 2020 at 1:37 PM Mylène Josserand
> > > <mylene.josserand@collabora.com> wrote:
> > > >
> > > > The revision rk3288w has a different clock tree about "hclk_vio"
> > > > clock, according to the BSP kernel code.
> > > >
> > > > This patch handles this difference by detecting which device-tree
> > > > we are using. If it is a "rockchip,rk3288-cru", let's register
> > > > the clock tree as it was before. If the device-tree node is
> > > > "rockchip,rk3288w-cru", we will apply the difference with this
> > > > version of this SoC.
> > > >
> > > > Noticed that this new device-tree compatible must be handled in
> > > > bootloader such as u-boot.
> > > >
> > > > Signed-off-by: Mylène Josserand <mylene.josserand@collabora.com>
> > > > ---
> > > >  drivers/clk/rockchip/clk-rk3288.c | 20 ++++++++++++++++++--
> > > >  1 file changed, 18 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/clk/rockchip/clk-rk3288.c b/drivers/clk/rockchip/clk-rk3288.c
> > > > index cc2a177bbdbf..204976e2d0cb 100644
> > > > --- a/drivers/clk/rockchip/clk-rk3288.c
> > > > +++ b/drivers/clk/rockchip/clk-rk3288.c
> > > > @@ -425,8 +425,6 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = {
> > > >         COMPOSITE(0, "aclk_vio0", mux_pll_src_cpll_gpll_usb480m_p, CLK_IGNORE_UNUSED,
> > > >                         RK3288_CLKSEL_CON(31), 6, 2, MFLAGS, 0, 5, DFLAGS,
> > > >                         RK3288_CLKGATE_CON(3), 0, GFLAGS),
> > > > -       DIV(0, "hclk_vio", "aclk_vio0", 0,
> > > > -                       RK3288_CLKSEL_CON(28), 8, 5, DFLAGS),
> > > >         COMPOSITE(0, "aclk_vio1", mux_pll_src_cpll_gpll_usb480m_p, CLK_IGNORE_UNUSED,
> > > >                         RK3288_CLKSEL_CON(31), 14, 2, MFLAGS, 8, 5, DFLAGS,
> > > >                         RK3288_CLKGATE_CON(3), 2, GFLAGS),
> > > > @@ -819,6 +817,16 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = {
> > > >         INVERTER(0, "pclk_isp", "pclk_isp_in", RK3288_CLKSEL_CON(29), 3, IFLAGS),
> > > >  };
> > > >
> > > > +static struct rockchip_clk_branch rk3288w_hclkvio_branch[] __initdata = {
> > > > +       DIV(0, "hclk_vio", "aclk_vio1", 0,
> > > > +                       RK3288_CLKSEL_CON(28), 8, 5, DFLAGS),
> > > > +};
> > > > +
> > > > +static struct rockchip_clk_branch rk3288_hclkvio_branch[] __initdata = {
> > > > +       DIV(0, "hclk_vio", "aclk_vio0", 0,
> > > > +                       RK3288_CLKSEL_CON(28), 8, 5, DFLAGS),
> > > > +};
> > > > +
> > > >  static const char *const rk3288_critical_clocks[] __initconst = {
> > > >         "aclk_cpu",
> > > >         "aclk_peri",
> > > > @@ -936,6 +944,14 @@ static void __init rk3288_clk_init(struct device_node *np)
> > > >                                    RK3288_GRF_SOC_STATUS1);
> > > >         rockchip_clk_register_branches(ctx, rk3288_clk_branches,
> > > >                                   ARRAY_SIZE(rk3288_clk_branches));
> > > > +
> > > > +       if (of_device_is_compatible(np, "rockchip,rk3288w-cru"))
> > > > +               rockchip_clk_register_branches(ctx, rk3288w_hclkvio_branch,
> > > > +                                              ARRAY_SIZE(rk3288w_hclkvio_branch));
> > > > +       else
> > > > +               rockchip_clk_register_branches(ctx, rk3288_hclkvio_branch,
> > > > +                                              ARRAY_SIZE(rk3288_hclkvio_branch));
> > > > +
> > >
> > > Sorry for the late query on this. I am a bit unclear about this
> > > compatible change, does Linux expect to replace rockchip,rk3288-cru
> > > with rockchip,rk3288w-cru in bootloader if the chip is RK3288w? or
> > > append the existing cru compatible node with rockchip,rk3288w-cru?
> > > because replace new cru node make clock never probe since the
> > > CLK_OF_DECLARE checking rockchip,rk3288-cru
> >
> > I guess right now we'd expect "rockchip,rk3288w-cru", "rockchip,rk3288-cru",
> >
> > Thinking again about this, I'm wondering if we should switch to having
> > only one per variant ... like on the two rk3188 variants,
> > so declaring separate rk3288-cru and rk3288w-cru of-clks with shared
> > common code.
> 
> What if can check the root compatible instead cru compatible for revision W like
> 
> -  if (of_device_is_compatible(np, "rockchip,rk3288w-cru"))
> + if (of_device_is_compatible(np, "rockchip,rk3288w"))

you'd need to check against the root compatible.

> 
> This way we can have a single compatible update at bootloader that
> makes Linux adjust revision W chips code.
> 
> Doesn't it make sense?

The compatible describes the block and the rk3288w's cru isn't the same as
the rk3288's ... as the clock routing is different, so it should have a
different compatible value, I think.

As the DT sis supposed to be a _generic_ description of the hardware,
we don't want to cement hacks to other implementations would need to copy.


Heiko
Ezequiel Garcia July 3, 2020, 3:02 p.m. UTC | #5
On Fri, 2020-07-03 at 16:11 +0200, Heiko Stuebner wrote:
> Hi Jagan,
> 
> Am Montag, 29. Juni 2020, 21:11:03 CEST schrieb Jagan Teki:
> > On Tue, Jun 2, 2020 at 1:37 PM Mylène Josserand
> > <mylene.josserand@collabora.com> wrote:
> > > The revision rk3288w has a different clock tree about "hclk_vio"
> > > clock, according to the BSP kernel code.
> > > 
> > > This patch handles this difference by detecting which device-tree
> > > we are using. If it is a "rockchip,rk3288-cru", let's register
> > > the clock tree as it was before. If the device-tree node is
> > > "rockchip,rk3288w-cru", we will apply the difference with this
> > > version of this SoC.
> > > 
> > > Noticed that this new device-tree compatible must be handled in
> > > bootloader such as u-boot.
> > > 
> > > Signed-off-by: Mylène Josserand <mylene.josserand@collabora.com>
> > > ---
> > >  drivers/clk/rockchip/clk-rk3288.c | 20 ++++++++++++++++++--
> > >  1 file changed, 18 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/clk/rockchip/clk-rk3288.c b/drivers/clk/rockchip/clk-rk3288.c
> > > index cc2a177bbdbf..204976e2d0cb 100644
> > > --- a/drivers/clk/rockchip/clk-rk3288.c
> > > +++ b/drivers/clk/rockchip/clk-rk3288.c
> > > @@ -425,8 +425,6 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = {
> > >         COMPOSITE(0, "aclk_vio0", mux_pll_src_cpll_gpll_usb480m_p, CLK_IGNORE_UNUSED,
> > >                         RK3288_CLKSEL_CON(31), 6, 2, MFLAGS, 0, 5, DFLAGS,
> > >                         RK3288_CLKGATE_CON(3), 0, GFLAGS),
> > > -       DIV(0, "hclk_vio", "aclk_vio0", 0,
> > > -                       RK3288_CLKSEL_CON(28), 8, 5, DFLAGS),
> > >         COMPOSITE(0, "aclk_vio1", mux_pll_src_cpll_gpll_usb480m_p, CLK_IGNORE_UNUSED,
> > >                         RK3288_CLKSEL_CON(31), 14, 2, MFLAGS, 8, 5, DFLAGS,
> > >                         RK3288_CLKGATE_CON(3), 2, GFLAGS),
> > > @@ -819,6 +817,16 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = {
> > >         INVERTER(0, "pclk_isp", "pclk_isp_in", RK3288_CLKSEL_CON(29), 3, IFLAGS),
> > >  };
> > > 
> > > +static struct rockchip_clk_branch rk3288w_hclkvio_branch[] __initdata = {
> > > +       DIV(0, "hclk_vio", "aclk_vio1", 0,
> > > +                       RK3288_CLKSEL_CON(28), 8, 5, DFLAGS),
> > > +};
> > > +
> > > +static struct rockchip_clk_branch rk3288_hclkvio_branch[] __initdata = {
> > > +       DIV(0, "hclk_vio", "aclk_vio0", 0,
> > > +                       RK3288_CLKSEL_CON(28), 8, 5, DFLAGS),
> > > +};
> > > +
> > >  static const char *const rk3288_critical_clocks[] __initconst = {
> > >         "aclk_cpu",
> > >         "aclk_peri",
> > > @@ -936,6 +944,14 @@ static void __init rk3288_clk_init(struct device_node *np)
> > >                                    RK3288_GRF_SOC_STATUS1);
> > >         rockchip_clk_register_branches(ctx, rk3288_clk_branches,
> > >                                   ARRAY_SIZE(rk3288_clk_branches));
> > > +
> > > +       if (of_device_is_compatible(np, "rockchip,rk3288w-cru"))
> > > +               rockchip_clk_register_branches(ctx, rk3288w_hclkvio_branch,
> > > +                                              ARRAY_SIZE(rk3288w_hclkvio_branch));
> > > +       else
> > > +               rockchip_clk_register_branches(ctx, rk3288_hclkvio_branch,
> > > +                                              ARRAY_SIZE(rk3288_hclkvio_branch));
> > > +
> > 
> > Sorry for the late query on this. I am a bit unclear about this
> > compatible change, does Linux expect to replace rockchip,rk3288-cru
> > with rockchip,rk3288w-cru in bootloader if the chip is RK3288w? or
> > append the existing cru compatible node with rockchip,rk3288w-cru?
> > because replace new cru node make clock never probe since the
> > CLK_OF_DECLARE checking rockchip,rk3288-cru
> 
> I guess right now we'd expect "rockchip,rk3288w-cru", "rockchip,rk3288-cru",
> 
> Thinking again about this, I'm wondering if we should switch to having
> only one per variant ... like on the two rk3188 variants,
> so declaring separate rk3288-cru and rk3288w-cru of-clks with shared
> common code.
> 

If we want to take this route (which I think makes sense), we should
do that sooner than later, so we don't release two different implementations
with two different requirements.

This change should be quite simple, no?

Thanks,
Ezequiel
Heiko Stübner July 3, 2020, 3:05 p.m. UTC | #6
Am Freitag, 3. Juli 2020, 17:02:52 CEST schrieb Ezequiel Garcia:
> On Fri, 2020-07-03 at 16:11 +0200, Heiko Stuebner wrote:
> > Hi Jagan,
> > 
> > Am Montag, 29. Juni 2020, 21:11:03 CEST schrieb Jagan Teki:
> > > On Tue, Jun 2, 2020 at 1:37 PM Mylène Josserand
> > > <mylene.josserand@collabora.com> wrote:
> > > > The revision rk3288w has a different clock tree about "hclk_vio"
> > > > clock, according to the BSP kernel code.
> > > > 
> > > > This patch handles this difference by detecting which device-tree
> > > > we are using. If it is a "rockchip,rk3288-cru", let's register
> > > > the clock tree as it was before. If the device-tree node is
> > > > "rockchip,rk3288w-cru", we will apply the difference with this
> > > > version of this SoC.
> > > > 
> > > > Noticed that this new device-tree compatible must be handled in
> > > > bootloader such as u-boot.
> > > > 
> > > > Signed-off-by: Mylène Josserand <mylene.josserand@collabora.com>
> > > > ---
> > > >  drivers/clk/rockchip/clk-rk3288.c | 20 ++++++++++++++++++--
> > > >  1 file changed, 18 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/clk/rockchip/clk-rk3288.c b/drivers/clk/rockchip/clk-rk3288.c
> > > > index cc2a177bbdbf..204976e2d0cb 100644
> > > > --- a/drivers/clk/rockchip/clk-rk3288.c
> > > > +++ b/drivers/clk/rockchip/clk-rk3288.c
> > > > @@ -425,8 +425,6 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = {
> > > >         COMPOSITE(0, "aclk_vio0", mux_pll_src_cpll_gpll_usb480m_p, CLK_IGNORE_UNUSED,
> > > >                         RK3288_CLKSEL_CON(31), 6, 2, MFLAGS, 0, 5, DFLAGS,
> > > >                         RK3288_CLKGATE_CON(3), 0, GFLAGS),
> > > > -       DIV(0, "hclk_vio", "aclk_vio0", 0,
> > > > -                       RK3288_CLKSEL_CON(28), 8, 5, DFLAGS),
> > > >         COMPOSITE(0, "aclk_vio1", mux_pll_src_cpll_gpll_usb480m_p, CLK_IGNORE_UNUSED,
> > > >                         RK3288_CLKSEL_CON(31), 14, 2, MFLAGS, 8, 5, DFLAGS,
> > > >                         RK3288_CLKGATE_CON(3), 2, GFLAGS),
> > > > @@ -819,6 +817,16 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = {
> > > >         INVERTER(0, "pclk_isp", "pclk_isp_in", RK3288_CLKSEL_CON(29), 3, IFLAGS),
> > > >  };
> > > > 
> > > > +static struct rockchip_clk_branch rk3288w_hclkvio_branch[] __initdata = {
> > > > +       DIV(0, "hclk_vio", "aclk_vio1", 0,
> > > > +                       RK3288_CLKSEL_CON(28), 8, 5, DFLAGS),
> > > > +};
> > > > +
> > > > +static struct rockchip_clk_branch rk3288_hclkvio_branch[] __initdata = {
> > > > +       DIV(0, "hclk_vio", "aclk_vio0", 0,
> > > > +                       RK3288_CLKSEL_CON(28), 8, 5, DFLAGS),
> > > > +};
> > > > +
> > > >  static const char *const rk3288_critical_clocks[] __initconst = {
> > > >         "aclk_cpu",
> > > >         "aclk_peri",
> > > > @@ -936,6 +944,14 @@ static void __init rk3288_clk_init(struct device_node *np)
> > > >                                    RK3288_GRF_SOC_STATUS1);
> > > >         rockchip_clk_register_branches(ctx, rk3288_clk_branches,
> > > >                                   ARRAY_SIZE(rk3288_clk_branches));
> > > > +
> > > > +       if (of_device_is_compatible(np, "rockchip,rk3288w-cru"))
> > > > +               rockchip_clk_register_branches(ctx, rk3288w_hclkvio_branch,
> > > > +                                              ARRAY_SIZE(rk3288w_hclkvio_branch));
> > > > +       else
> > > > +               rockchip_clk_register_branches(ctx, rk3288_hclkvio_branch,
> > > > +                                              ARRAY_SIZE(rk3288_hclkvio_branch));
> > > > +
> > > 
> > > Sorry for the late query on this. I am a bit unclear about this
> > > compatible change, does Linux expect to replace rockchip,rk3288-cru
> > > with rockchip,rk3288w-cru in bootloader if the chip is RK3288w? or
> > > append the existing cru compatible node with rockchip,rk3288w-cru?
> > > because replace new cru node make clock never probe since the
> > > CLK_OF_DECLARE checking rockchip,rk3288-cru
> > 
> > I guess right now we'd expect "rockchip,rk3288w-cru", "rockchip,rk3288-cru",
> > 
> > Thinking again about this, I'm wondering if we should switch to having
> > only one per variant ... like on the two rk3188 variants,
> > so declaring separate rk3288-cru and rk3288w-cru of-clks with shared
> > common code.
> > 
> 
> If we want to take this route (which I think makes sense), we should
> do that sooner than later, so we don't release two different implementations
> with two different requirements.
> 
> This change should be quite simple, no?

the underlying change is queued for 5.9, but yeah I am currently testing
exactly such a patch ;-)
Especially as when reading the binding addition it states
rk3288w-cru _or_ rk3288-cru for the compatible.


Heiko
Robin Murphy July 3, 2020, 3:43 p.m. UTC | #7
On 2020-07-03 15:48, Heiko Stuebner wrote:
> Am Freitag, 3. Juli 2020, 16:23:27 CEST schrieb Jagan Teki:
>> On Fri, Jul 3, 2020 at 7:41 PM Heiko Stuebner <heiko@sntech.de> wrote:
>>>
>>> Hi Jagan,
>>>
>>> Am Montag, 29. Juni 2020, 21:11:03 CEST schrieb Jagan Teki:
>>>> On Tue, Jun 2, 2020 at 1:37 PM Mylène Josserand
>>>> <mylene.josserand@collabora.com> wrote:
>>>>>
>>>>> The revision rk3288w has a different clock tree about "hclk_vio"
>>>>> clock, according to the BSP kernel code.
>>>>>
>>>>> This patch handles this difference by detecting which device-tree
>>>>> we are using. If it is a "rockchip,rk3288-cru", let's register
>>>>> the clock tree as it was before. If the device-tree node is
>>>>> "rockchip,rk3288w-cru", we will apply the difference with this
>>>>> version of this SoC.
>>>>>
>>>>> Noticed that this new device-tree compatible must be handled in
>>>>> bootloader such as u-boot.
>>>>>
>>>>> Signed-off-by: Mylène Josserand <mylene.josserand@collabora.com>
>>>>> ---
>>>>>   drivers/clk/rockchip/clk-rk3288.c | 20 ++++++++++++++++++--
>>>>>   1 file changed, 18 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/clk/rockchip/clk-rk3288.c b/drivers/clk/rockchip/clk-rk3288.c
>>>>> index cc2a177bbdbf..204976e2d0cb 100644
>>>>> --- a/drivers/clk/rockchip/clk-rk3288.c
>>>>> +++ b/drivers/clk/rockchip/clk-rk3288.c
>>>>> @@ -425,8 +425,6 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = {
>>>>>          COMPOSITE(0, "aclk_vio0", mux_pll_src_cpll_gpll_usb480m_p, CLK_IGNORE_UNUSED,
>>>>>                          RK3288_CLKSEL_CON(31), 6, 2, MFLAGS, 0, 5, DFLAGS,
>>>>>                          RK3288_CLKGATE_CON(3), 0, GFLAGS),
>>>>> -       DIV(0, "hclk_vio", "aclk_vio0", 0,
>>>>> -                       RK3288_CLKSEL_CON(28), 8, 5, DFLAGS),
>>>>>          COMPOSITE(0, "aclk_vio1", mux_pll_src_cpll_gpll_usb480m_p, CLK_IGNORE_UNUSED,
>>>>>                          RK3288_CLKSEL_CON(31), 14, 2, MFLAGS, 8, 5, DFLAGS,
>>>>>                          RK3288_CLKGATE_CON(3), 2, GFLAGS),
>>>>> @@ -819,6 +817,16 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = {
>>>>>          INVERTER(0, "pclk_isp", "pclk_isp_in", RK3288_CLKSEL_CON(29), 3, IFLAGS),
>>>>>   };
>>>>>
>>>>> +static struct rockchip_clk_branch rk3288w_hclkvio_branch[] __initdata = {
>>>>> +       DIV(0, "hclk_vio", "aclk_vio1", 0,
>>>>> +                       RK3288_CLKSEL_CON(28), 8, 5, DFLAGS),
>>>>> +};
>>>>> +
>>>>> +static struct rockchip_clk_branch rk3288_hclkvio_branch[] __initdata = {
>>>>> +       DIV(0, "hclk_vio", "aclk_vio0", 0,
>>>>> +                       RK3288_CLKSEL_CON(28), 8, 5, DFLAGS),
>>>>> +};
>>>>> +
>>>>>   static const char *const rk3288_critical_clocks[] __initconst = {
>>>>>          "aclk_cpu",
>>>>>          "aclk_peri",
>>>>> @@ -936,6 +944,14 @@ static void __init rk3288_clk_init(struct device_node *np)
>>>>>                                     RK3288_GRF_SOC_STATUS1);
>>>>>          rockchip_clk_register_branches(ctx, rk3288_clk_branches,
>>>>>                                    ARRAY_SIZE(rk3288_clk_branches));
>>>>> +
>>>>> +       if (of_device_is_compatible(np, "rockchip,rk3288w-cru"))
>>>>> +               rockchip_clk_register_branches(ctx, rk3288w_hclkvio_branch,
>>>>> +                                              ARRAY_SIZE(rk3288w_hclkvio_branch));
>>>>> +       else
>>>>> +               rockchip_clk_register_branches(ctx, rk3288_hclkvio_branch,
>>>>> +                                              ARRAY_SIZE(rk3288_hclkvio_branch));
>>>>> +
>>>>
>>>> Sorry for the late query on this. I am a bit unclear about this
>>>> compatible change, does Linux expect to replace rockchip,rk3288-cru
>>>> with rockchip,rk3288w-cru in bootloader if the chip is RK3288w? or
>>>> append the existing cru compatible node with rockchip,rk3288w-cru?
>>>> because replace new cru node make clock never probe since the
>>>> CLK_OF_DECLARE checking rockchip,rk3288-cru
>>>
>>> I guess right now we'd expect "rockchip,rk3288w-cru", "rockchip,rk3288-cru",
>>>
>>> Thinking again about this, I'm wondering if we should switch to having
>>> only one per variant ... like on the two rk3188 variants,
>>> so declaring separate rk3288-cru and rk3288w-cru of-clks with shared
>>> common code.
>>
>> What if can check the root compatible instead cru compatible for revision W like
>>
>> -  if (of_device_is_compatible(np, "rockchip,rk3288w-cru"))
>> + if (of_device_is_compatible(np, "rockchip,rk3288w"))
> 
> you'd need to check against the root compatible.
> 
>>
>> This way we can have a single compatible update at bootloader that
>> makes Linux adjust revision W chips code.
>>
>> Doesn't it make sense?
> 
> The compatible describes the block and the rk3288w's cru isn't the same as
> the rk3288's ... as the clock routing is different, so it should have a
> different compatible value, I think.

Right, if two devices behave differently in a way that one behaviour is 
not a strict superset of the other, and there's no way to tell which is 
which by simply looking at the device itself, then by definition they 
are not compatible.

> As the DT sis supposed to be a _generic_ description of the hardware,
> we don't want to cement hacks to other implementations would need to copy.

Indeed it's a pain in the bum for driver developers to be given a new 
thing that's 99.9% compatible with the old thing but the one tiny 
difference fundamentally breaks it, and the temptation to look for an 
easy way out is strong, but c'est la vie ;)

The question I always bear in mind for cases like these is this: If a 
kernel without the driver patch runs on the new system with the expected 
new DTB, will it:

A) work entirely correctly
B) ignore the device (with or without an error), but the rest of the 
system still works fine
C) ignore the device and fail to boot at all (with or without an error) 
because it's critical to operation of the rest of the system
D) probe the device and superficially appear to be OK but actually be 
broken in weird and subtle ways

A and B are the ideal answers. C is a little unfortunate, but acceptable 
if neither A nor B is possible. D is a sign that you're doing something 
wrong, unless there are *very* specific circumstances that might justify 
it (e.g. the SoC is exclusively deployed in embedded devices with a 
controlled update mechanism such that users could never introduce this 
mismatch in the first place).

Robin.
diff mbox series

Patch

diff --git a/drivers/clk/rockchip/clk-rk3288.c b/drivers/clk/rockchip/clk-rk3288.c
index cc2a177bbdbf..204976e2d0cb 100644
--- a/drivers/clk/rockchip/clk-rk3288.c
+++ b/drivers/clk/rockchip/clk-rk3288.c
@@ -425,8 +425,6 @@  static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = {
 	COMPOSITE(0, "aclk_vio0", mux_pll_src_cpll_gpll_usb480m_p, CLK_IGNORE_UNUSED,
 			RK3288_CLKSEL_CON(31), 6, 2, MFLAGS, 0, 5, DFLAGS,
 			RK3288_CLKGATE_CON(3), 0, GFLAGS),
-	DIV(0, "hclk_vio", "aclk_vio0", 0,
-			RK3288_CLKSEL_CON(28), 8, 5, DFLAGS),
 	COMPOSITE(0, "aclk_vio1", mux_pll_src_cpll_gpll_usb480m_p, CLK_IGNORE_UNUSED,
 			RK3288_CLKSEL_CON(31), 14, 2, MFLAGS, 8, 5, DFLAGS,
 			RK3288_CLKGATE_CON(3), 2, GFLAGS),
@@ -819,6 +817,16 @@  static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = {
 	INVERTER(0, "pclk_isp", "pclk_isp_in", RK3288_CLKSEL_CON(29), 3, IFLAGS),
 };
 
+static struct rockchip_clk_branch rk3288w_hclkvio_branch[] __initdata = {
+	DIV(0, "hclk_vio", "aclk_vio1", 0,
+			RK3288_CLKSEL_CON(28), 8, 5, DFLAGS),
+};
+
+static struct rockchip_clk_branch rk3288_hclkvio_branch[] __initdata = {
+	DIV(0, "hclk_vio", "aclk_vio0", 0,
+			RK3288_CLKSEL_CON(28), 8, 5, DFLAGS),
+};
+
 static const char *const rk3288_critical_clocks[] __initconst = {
 	"aclk_cpu",
 	"aclk_peri",
@@ -936,6 +944,14 @@  static void __init rk3288_clk_init(struct device_node *np)
 				   RK3288_GRF_SOC_STATUS1);
 	rockchip_clk_register_branches(ctx, rk3288_clk_branches,
 				  ARRAY_SIZE(rk3288_clk_branches));
+
+	if (of_device_is_compatible(np, "rockchip,rk3288w-cru"))
+		rockchip_clk_register_branches(ctx, rk3288w_hclkvio_branch,
+					       ARRAY_SIZE(rk3288w_hclkvio_branch));
+	else
+		rockchip_clk_register_branches(ctx, rk3288_hclkvio_branch,
+					       ARRAY_SIZE(rk3288_hclkvio_branch));
+
 	rockchip_clk_protect_critical(rk3288_critical_clocks,
 				      ARRAY_SIZE(rk3288_critical_clocks));