Re: [PATCH v4 3/6] clk: ralink: add clock driver for mt7621 SoC

[Sergio Paracuellos <sergio.paracuellos@gmail.com>]

On Thu, Dec 17, 2020 at 11:12 AM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Sergio Paracuellos (2020-12-17 01:54:18)
> >
> > On Thu, Dec 17, 2020 at 10:09 AM Stephen Boyd <sboyd@kernel.org> wrote:
> > >
> > > Quoting Sergio Paracuellos (2020-11-22 01:55:53)
> > > > diff --git a/drivers/clk/ralink/Makefile b/drivers/clk/ralink/Makefile
> > > > new file mode 100644
> > > > index 000000000000..cf6f9216379d
> > > > --- /dev/null
> > > > +++ b/drivers/clk/ralink/Makefile
> > > > @@ -0,0 +1,2 @@
> > > > +# SPDX-License-Identifier: GPL-2.0
> > > > +obj-$(CONFIG_CLK_MT7621) += clk-mt7621.o
> > > > diff --git a/drivers/clk/ralink/clk-mt7621.c b/drivers/clk/ralink/clk-mt7621.c
> > > > new file mode 100644
> > > > index 000000000000..4e929f13fe7c
> > > > --- /dev/null
> > > > +++ b/drivers/clk/ralink/clk-mt7621.c
> > > > @@ -0,0 +1,435 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Mediatek MT7621 Clock Driver
> > > > + * Author: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > > > + */
> > > > +
> > > > +#include <linux/bitops.h>
> > > > +#include <linux/clk-provider.h>
> > > > +#include <linux/mfd/syscon.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/slab.h>
> > > > +#include <linux/regmap.h>
> > > > +#include <asm/mach-ralink/ralink_regs.h>
> > >
> > > Is it possible to drop this include? Doing so would make this portable
> > > and compilable on more architectures so us cross compilers can check
> > > build stuff and make changes easily.
> >
> > No, this is not possible. This old arch makes some global functions
> > there to properly access different registers in the palmbus. It is not
> > also well documented so it is really difficult to make something
> > better with this.
> > This is needed to use 'rt_memc_r32'
> > (arch/mips/include/asm/mach-ralink/ralink_regs.h) for reading
> > MEMC_REG_CPU_PLL.
> >
> > This is a not documented register and is not in the syscon related
> > part and we need it to derive the clock frequency for the XTAL clock.
>
> Ok.
>
> > > > +static int mt7621_gate_ops_init(struct device_node *np,
> > > > +                                struct mt7621_gate *sclk)
> > > > +{
> > > > +       struct clk_init_data init = {
> > > > +               .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
> > >
> > > Why ignore unused? Are they CLK_IS_CRITICAL? Can they be enabled at
> > > driver probe instead of here? Or left out of the kernel entirely if they
> > > shouldn't be turned off?
> >
> > Because all the platform drivers are not changed to use this gates yet
> > and all gates are enabled by default (related registers are set to all
> > ones),  kernel disables all the stuff because they are not being
> > referenced, but yes, you are right, I think I can call
> > clk_prepare_enable for all of them at init time and avoid this
> > 'CLK_IGNORE_UNUSED' flag to don't break anything of the current other
> > upstream code.
>
> Does something crash if they're turned off? We have CLK_IS_CRITICAL for
> that. The CLK_IGNORE_UNUSED flag is sort of deprecated now.

Well, as drivers are not getting into account gates and not referenced
real hw bits are disabled by kernel because nobody requested them so
for example my uart gets down and cannot really see anything :). I
think call to 'clk_prepare_enable' should be enough since by default
all of them are setting up in registers, so call that will also
reference them...

>
> > > > +
> > > > +#define CLK_BASE(_name, _parent, _recalc) {                            \
> > > > +       .init = &(struct clk_init_data) {                               \
> > > > +               .name = _name,                                          \
> > > > +               .ops = &(const struct clk_ops) {                        \
> > > > +                       .recalc_rate = _recalc,                         \
> > > > +               },                                                      \
> > > > +               .parent_names = (const char *const[]) { _parent },      \
> > >
> > > Please use clk_parent_data instead
> >
> > parent can also be NULL here and num_parents zero, but I will search
> > what do you really mean with this 'clk_parent_data' :).
>
> Heh, 'git grep clk_parent_data -- drivers/clk/' should give some clues.

Thanks, will do!

>
> > > > +free_clk_prov:
> > > > +       kfree(clk_prov);
> > > > +}
> > > > +
> > > > +CLK_OF_DECLARE(mt7621_clk, "mediatek,mt7621-clk", mt7621_clk_init);
> > >
> > > Any reason to use this vs. a platform driver?
> >
> > We need clocks available in 'plat_time_init' before setting up the
> > timer for the GIC, so to maintain all the clock driver in a simple
> > file and using only one device tree node and no separate the gates
> > into another platform driver, I think this is the only way to go, but
> > please correct me if I am wrong.
>
> We can register the few clks like that early with
> CLK_OF_DECLARE_DRIVER() and then have a platform driver register the
> rest of the clks that aren't required early. This lets us hook into the
> driver framework better while still getting those few clks to turn on
> early enough for the timers.

I see. I will explore the way to do this as you are suggesting here, thanks!

Best regards,
    Sergio Paracuellos