[PATCH v6 2/4] rockchip: rk3066: add clock driver for rk3066 soc

[Simon Glass]

Hi Paweł,

On Sun, 25 Jul 2021 at 08:15, Paweł Jarosz <paweljarosz3691 at gmail.com> wrote:
>
> Hi Simon,
>
>
> sorry for late response i was offline a bit
>
> W dniu 13.07.2021 o 22:17, Simon Glass pisze:
> > Hi Paweł,
> >
> > On Tue, 13 Jul 2021 at 12:59, Paweł Jarosz <paweljarosz3691 at gmail.com> wrote:
> >> Add clock driver for rk3066 platform.
> >>
> >> Signed-off-by: Paweł Jarosz <paweljarosz3691 at gmail.com>
> >> Acked-by: Philipp Tomsich <philipp.tomsich at vrull.eu>
> >> ---
> >>
> >> Changes since v1:
> >> - updated to shifted masks
> >> - moved clk init to tpl
> >>
> >> Changes since v2:
> >> - none
> >>
> >> Changes since v3:
> >> - none
> >>
> >> Changes since v4:
> >> - updated to current codebase
> >> - fixed compilation errors
> >>
> >> Changes since v5:
> >> - various style changes
> >> - added clk_enable/clk_disable support for nand and mmc clocks
> >> - updated maintainer email
> >> - renamed uint32_t to u32
> >> - used #if IS_ENABLED macro instead #ifdef
> >>
> >>
> >>
> >>  .../include/asm/arch-rockchip/cru_rk3066.h    | 203 +++++
> >>  drivers/clk/rockchip/Makefile                 |   1 +
> >>  drivers/clk/rockchip/clk_rk3066.c             | 704 ++++++++++++++++++
> >>  3 files changed, 908 insertions(+)
> >>  create mode 100644 arch/arm/include/asm/arch-rockchip/cru_rk3066.h
> >>  create mode 100644 drivers/clk/rockchip/clk_rk3066.c
> >>
> > [..]
> >
> >> +
> >> +static int rk3066_clk_of_to_plat(struct udevice *dev)
> >> +{
> >> +#if !CONFIG_IS_ENABLED(OF_PLATDATA)
> >> +       struct rk3066_clk_priv *priv = dev_get_priv(dev);
> >> +
> >> +       priv->cru = dev_read_addr_ptr(dev);
> >> +#endif
> >> +
> >> +       return 0;
> >> +}
> >> +
> >> +static int rk3066_clk_probe(struct udevice *dev)
> >> +{
> >> +       struct rk3066_clk_priv *priv = dev_get_priv(dev);
> >> +
> >> +       priv->grf = syscon_get_first_range(ROCKCHIP_SYSCON_GRF);
> >> +       if (IS_ERR(priv->grf))
> >> +               return PTR_ERR(priv->grf);
> >> +
> >> +#if IS_ENABLED(CONFIG_TPL_BUILD)
> > Do you need that? The line below should take care of it.
>
>
> Yep. Later rkclk_init and rkclk_configure_cpu should be only executed in TPL.

But CONFIG_IS_ENABLED(OF_PLATDATA) should be enough to check that,
right, since it is only true in TPL? You the TPL check seems to be
redundant.

>
>
> >> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
> >> +       struct rk3066_clk_plat *plat = dev_get_plat(dev);
> >> +
> >> +       priv->cru = map_sysmem(plat->dtd.reg[0], plat->dtd.reg[1]);
> >> +#endif
> >> +
> >> +       rkclk_init(priv->cru, priv->grf);
> >> +
> >> +       /* Init CPU frequency */
> >> +       rkclk_configure_cpu(priv->cru, priv->grf, APLL_SAFE_HZ);
> >> +#endif
> >> +
> >> +       return 0;
> >> +}
> >> +
> >> +static int rk3066_clk_bind(struct udevice *dev)
> >> +{
> >> +       int ret;
> >> +       struct udevice *sys_child;
> >> +       struct sysreset_reg *priv;
> >> +
> >> +       /* The reset driver does not have a device node, so bind it here */
> >> +       ret = device_bind_driver(dev, "rockchip_sysreset", "sysreset",
> >> +                                &sys_child);
> >> +       if (ret) {
> >> +               debug("Warning: No sysreset driver: ret=%d\n", ret);
> >> +       } else {
> >> +               priv = malloc(sizeof(struct sysreset_reg));
> >> +               priv->glb_srst_fst_value = offsetof(struct rk3066_cru,
> >> +                                                   cru_glb_srst_fst_value);
> >> +               priv->glb_srst_snd_value = offsetof(struct rk3066_cru,
> >> +                                                   cru_glb_srst_snd_value);
> >> +               dev_set_priv(sys_child, priv);
> >> +       }
> >> +
> >> +#if CONFIG_IS_ENABLED(RESET_ROCKCHIP)
> > Can you use if() instead of #if ?
>
>
> Yes, but what is the difference? Sorry ... I don't know what to ask google to get the answer.
>
> Is it in this case doing the same thing and just looking better?

We are avoiding the preprocessor these days in favour of compile-time
checks, since it increases build coverage and makes the code easier to
read (there are many other things here). Also patman should warn you
about this?

Regards,
Simon