Hi Geert,
Thanks for the feedback.
> Subject: Re: [PATCH v2 1/3] clk: renesas: rzg2l: Add support for watchdog
> reset selection
>
> Hi Biju,
>
> Thanks for your patch!
>
> On Thu, Nov 11, 2021 at 12:54 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> wrote:
> > This patch adds support for watchdog reset selection.
>
> Please explain what this patch really does, and why it is needed, instead
> of repeating the one-line summary.
OK will do. CPG IP block contains 2 WDT registers. 1) WDT reset selector and
2) WDT Overflow system Register.
Former one is used to mask reset requests from WDT and the later
One used to check the WDTOVF status and clearing.
CPG IP and WDT IP are in different address space. The operation involving
WDT reset selector, is a configuration operation, which we can do
in Clock driver without any direct call from WDT driver to CPG.
Where as Operation involving WDT Overflow system Register, needs to
be exported to WDT driver, as it needs WDTOVF status, so that It can support
WDIOF_CARDRESET option.
>
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
>
> > --- a/drivers/clk/renesas/r9a07g044-cpg.c
> > +++ b/drivers/clk/renesas/r9a07g044-cpg.c
>
> > @@ -295,7 +296,28 @@ static const unsigned int r9a07g044_crit_mod_clks[]
> __initconst = {
> > MOD_CLK_BASE + R9A07G044_DMAC_ACLK, };
> >
> > +#define CPG_WDTRST_SEL 0xb14
> > +#define CPG_WDTRST_SEL_WDTRSTSEL(n) BIT(n)
> > +
> > +#define CPG_WDTRST_SEL_WDTRST (CPG_WDTRST_SEL_WDTRSTSEL(0) | \
> > + CPG_WDTRST_SEL_WDTRSTSEL(1) | \
> > + CPG_WDTRST_SEL_WDTRSTSEL(2))
>
> You might as well use BIT() directly. Or GENMASK().
OK.
>
> > +
> > +int r9a07g044_wdt_rst_setect(void __iomem *base) {
> > + writel((CPG_WDTRST_SEL_WDTRST << 16) | CPG_WDTRST_SEL_WDTRST,
> > + base + CPG_WDTRST_SEL);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct rzg2l_cpg_soc_operations r9a07g044_cpg_ops = {
> > + .wdt_rst_setect = r9a07g044_wdt_rst_setect,
>
> As you use a function pointer, I assume different SoCs need different
> handling, and you can't just store e.g. a bitmask of bits to set in info?
Currently I am not sure about RZ/G2UL, since it has single Core.
That is the reason function pointer Introduced. For RZ/G2{L,LC} it is same.
I will drop function pointer and store it in bitmask of bits.
>
>
> > +};
> > +
> > const struct rzg2l_cpg_info r9a07g044_cpg_info = {
> > + .ops = &r9a07g044_cpg_ops,
> > +
> > /* Core Clocks */
> > .core_clks = r9a07g044_core_clks,
> > .num_core_clks = ARRAY_SIZE(r9a07g044_core_clks), diff --git
> > a/drivers/clk/renesas/rzg2l-cpg.c b/drivers/clk/renesas/rzg2l-cpg.c
> > index a77cb47b75e7..f9dfee14a33e 100644
> > --- a/drivers/clk/renesas/rzg2l-cpg.c
> > +++ b/drivers/clk/renesas/rzg2l-cpg.c
> > @@ -932,6 +932,12 @@ static int __init rzg2l_cpg_probe(struct
> platform_device *pdev)
> > if (error)
> > return error;
> >
> > + if (info->ops && info->ops->wdt_rst_setect) {
> > + error = info->ops->wdt_rst_setect(priv->base);
> > + if (error)
> > + return error;
> > + }
> > +
> > return 0;
> > }
> >
> > diff --git a/drivers/clk/renesas/rzg2l-cpg.h
> > b/drivers/clk/renesas/rzg2l-cpg.h index 484c7cee2629..e1b1497002ed
> > 100644
> > --- a/drivers/clk/renesas/rzg2l-cpg.h
> > +++ b/drivers/clk/renesas/rzg2l-cpg.h
> > @@ -156,9 +156,20 @@ struct rzg2l_reset {
> > .bit = (_bit) \
> > }
> >
> > +/**
> > + * struct rzg2l_cpg_soc_operations - SoC-specific CPG Operations
> > + *
> > + * @wdt_rst_setect: WDT reset selection */ struct
> > +rzg2l_cpg_soc_operations {
> > + int (*wdt_rst_setect)(void __iomem *base); /* Platform
> > +specific WDT reset selection */
>
> Do you plan to add more operations?
As mentioned above, I am dropping function pointer and going with bitmask.
Another Operation to be added in future is to export WDTOVF status to WDT driver.
So that the Watchdog driver will get last boot status from CPG IP.
But currently, I guess we can do this only with exported API??
Do you have any other suggestion apart from this? Please let me know.
Regards,
Biju
