| Message ID | 20211111115427.8228-2-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v2,1/3] clk: renesas: rzg2l: Add support for watchdog reset selection | expand |
Hi Biju, Thank you for the patch. > -----Original Message----- > From: Biju Das <biju.das.jz@bp.renesas.com> > Sent: 11 November 2021 11:54 > To: Michael Turquette <mturquette@baylibre.com>; Stephen Boyd <sboyd@kernel.org> > Cc: Biju Das <biju.das.jz@bp.renesas.com>; Geert Uytterhoeven <geert+renesas@glider.be>; linux- > renesas-soc@vger.kernel.org; linux-watchdog@vger.kernel.org; linux-clk@vger.kernel.org; Chris Paterson > <Chris.Paterson2@renesas.com>; Biju Das <biju.das@bp.renesas.com>; Prabhakar Mahadev Lad > <prabhakar.mahadev-lad.rj@bp.renesas.com> > Subject: [PATCH v2 1/3] clk: renesas: rzg2l: Add support for watchdog reset selection > > This patch adds support for watchdog reset selection. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > V1->V2: > * No Change > RFC->V1: > * No change > --- > drivers/clk/renesas/r9a07g044-cpg.c | 22 ++++++++++++++++++++++ > drivers/clk/renesas/rzg2l-cpg.c | 6 ++++++ > drivers/clk/renesas/rzg2l-cpg.h | 14 ++++++++++++++ > 3 files changed, 42 insertions(+) > > diff --git a/drivers/clk/renesas/r9a07g044-cpg.c b/drivers/clk/renesas/r9a07g044-cpg.c > index 91643b4e1c9c..0bbdc8bd6235 100644 > --- a/drivers/clk/renesas/r9a07g044-cpg.c > +++ b/drivers/clk/renesas/r9a07g044-cpg.c > @@ -8,6 +8,7 @@ > #include <linux/clk-provider.h> > #include <linux/device.h> > #include <linux/init.h> > +#include <linux/io.h> > #include <linux/kernel.h> > > #include <dt-bindings/clock/r9a07g044-cpg.h> > @@ -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)) > + > +int r9a07g044_wdt_rst_setect(void __iomem *base) > +{ Can be static. Cheers, Prabhakar > + 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, > +}; > + > 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 */ > +}; > + > /** > * struct rzg2l_cpg_info - SoC-specific CPG Description > * > + * @ops: SoC-specific CPG Operations > + * > * @core_clks: Array of Core Clock definitions > * @num_core_clks: Number of entries in core_clks[] > * @last_dt_core_clk: ID of the last Core Clock exported to DT > @@ -176,6 +187,9 @@ struct rzg2l_reset { > * @num_crit_mod_clks: Number of entries in crit_mod_clks[] > */ > struct rzg2l_cpg_info { > + /* CPG Operations */ > + const struct rzg2l_cpg_soc_operations *ops; > + > /* Core Clocks */ > const struct cpg_core_clk *core_clks; > unsigned int num_core_clks; > -- > 2.17.1
Hi Prabhakar, Thanks for the feedback > Subject: RE: [PATCH v2 1/3] clk: renesas: rzg2l: Add support for watchdog > reset selection > > Hi Biju, > > Thank you for the patch. > > > -----Original Message----- > > From: Biju Das <biju.das.jz@bp.renesas.com> > > Sent: 11 November 2021 11:54 > > To: Michael Turquette <mturquette@baylibre.com>; Stephen Boyd > > <sboyd@kernel.org> > > Cc: Biju Das <biju.das.jz@bp.renesas.com>; Geert Uytterhoeven > > <geert+renesas@glider.be>; linux- renesas-soc@vger.kernel.org; > > linux-watchdog@vger.kernel.org; linux-clk@vger.kernel.org; Chris > > Paterson <Chris.Paterson2@renesas.com>; Biju Das > > <biju.das@bp.renesas.com>; Prabhakar Mahadev Lad > > <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Subject: [PATCH v2 1/3] clk: renesas: rzg2l: Add support for watchdog > > reset selection > > > > This patch adds support for watchdog reset selection. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > --- > > V1->V2: > > * No Change > > RFC->V1: > > * No change > > --- > > drivers/clk/renesas/r9a07g044-cpg.c | 22 ++++++++++++++++++++++ > > drivers/clk/renesas/rzg2l-cpg.c | 6 ++++++ > > drivers/clk/renesas/rzg2l-cpg.h | 14 ++++++++++++++ > > 3 files changed, 42 insertions(+) > > > > diff --git a/drivers/clk/renesas/r9a07g044-cpg.c > > b/drivers/clk/renesas/r9a07g044-cpg.c > > index 91643b4e1c9c..0bbdc8bd6235 100644 > > --- a/drivers/clk/renesas/r9a07g044-cpg.c > > +++ b/drivers/clk/renesas/r9a07g044-cpg.c > > @@ -8,6 +8,7 @@ > > #include <linux/clk-provider.h> > > #include <linux/device.h> > > #include <linux/init.h> > > +#include <linux/io.h> > > #include <linux/kernel.h> > > > > #include <dt-bindings/clock/r9a07g044-cpg.h> > > @@ -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)) > > + > > +int r9a07g044_wdt_rst_setect(void __iomem *base) { > Can be static. OK. Will change to static on the next version. Geert, On the, next version I am planning to introduce the below code for Reset selection based on device availability, instead of selecting all the channels. Is it the right way to do ? please let me know. node = of_find_node_by_name (NULL, NULL, "watchdog@12800800"); if (node && of_device_is_available(node) { // set reset selection for that channel of_node_put(node); } node = of_find_node_by_name (NULL, NULL, "watchdog@12800c00"); if (node && of_device_is_available(node) { // set reset selection for that channel of_node_put(node); } node = of_find_node_by_name (NULL, NULL, "watchdog@12800400"); if (node && of_device_is_available(node) { // set reset selection for that channel of_node_put(node); } Regards, Biju > > Cheers, > Prabhakar > > > + 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, }; > > + > > 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 */ }; > > + > > /** > > * struct rzg2l_cpg_info - SoC-specific CPG Description > > * > > + * @ops: SoC-specific CPG Operations > > + * > > * @core_clks: Array of Core Clock definitions > > * @num_core_clks: Number of entries in core_clks[] > > * @last_dt_core_clk: ID of the last Core Clock exported to DT @@ > > -176,6 +187,9 @@ struct rzg2l_reset { > > * @num_crit_mod_clks: Number of entries in crit_mod_clks[] > > */ > > struct rzg2l_cpg_info { > > + /* CPG Operations */ > > + const struct rzg2l_cpg_soc_operations *ops; > > + > > /* Core Clocks */ > > const struct cpg_core_clk *core_clks; > > unsigned int num_core_clks; > > -- > > 2.17.1
Hi Biju, Thanks for your patch! On Thu, Nov 11, 2021 at 12:54 PM Biju Das <biju.das.jz@bp.renesas.com> 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. > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- 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(). > + > +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? > +}; > + > 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? > +}; > + > /** > * struct rzg2l_cpg_info - SoC-specific CPG Description > * > + * @ops: SoC-specific CPG Operations > + * > * @core_clks: Array of Core Clock definitions > * @num_core_clks: Number of entries in core_clks[] > * @last_dt_core_clk: ID of the last Core Clock exported to DT > @@ -176,6 +187,9 @@ struct rzg2l_reset { > * @num_crit_mod_clks: Number of entries in crit_mod_clks[] > */ > struct rzg2l_cpg_info { > + /* CPG Operations */ > + const struct rzg2l_cpg_soc_operations *ops; > + > /* Core Clocks */ > const struct cpg_core_clk *core_clks; > unsigned int num_core_clks; Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Biju, On Wed, Nov 17, 2021 at 9:21 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > On the, next version I am planning to introduce the below code for > Reset selection based on device availability, instead of selecting > all the channels. Is it the right way to do ? please let me know. > > node = of_find_node_by_name (NULL, NULL, "watchdog@12800800"); > if (node && of_device_is_available(node) { > // set reset selection for that channel > of_node_put(node); > } > > node = of_find_node_by_name (NULL, NULL, "watchdog@12800c00"); > if (node && of_device_is_available(node) { > // set reset selection for that channel > of_node_put(node); > } > > node = of_find_node_by_name (NULL, NULL, "watchdog@12800400"); > if (node && of_device_is_available(node) { > // set reset selection for that channel > of_node_put(node); > } Matching on node names is very fragile. And what if the watchdog node is enabled in DT, but the watchdog driver is not available? Moreover, this looks like it should not be controlled from the clock driver, but from the watchdog driver instead. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
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@bp.renesas.com> > 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@bp.renesas.com> > > > --- 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
Hi Geert, Thanks for the feedback. > Subject: Re: [PATCH v2 1/3] clk: renesas: rzg2l: Add support for watchdog > reset selection > > Hi Biju, > > On Wed, Nov 17, 2021 at 9:21 AM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > On the, next version I am planning to introduce the below code for > > Reset selection based on device availability, instead of selecting all > > the channels. Is it the right way to do ? please let me know. > > > > node = of_find_node_by_name (NULL, NULL, "watchdog@12800800"); if > > (node && of_device_is_available(node) { > > // set reset selection for that channel > > of_node_put(node); > > } > > > > node = of_find_node_by_name (NULL, NULL, "watchdog@12800c00"); if > > (node && of_device_is_available(node) { > > // set reset selection for that channel > > of_node_put(node); > > } > > > > node = of_find_node_by_name (NULL, NULL, "watchdog@12800400"); if > > (node && of_device_is_available(node) { > > // set reset selection for that channel > > of_node_put(node); > > } > > Matching on node names is very fragile. Agreed. And what if the watchdog node is > enabled in DT, but the watchdog driver is not available? We will just configure, but since there is no watch driver available. I guess nothing will happen. > Moreover, this looks like it should not be controlled from the clock > driver, but from the watchdog driver instead. I have referred configure option from reset driver for R-Car, where WDT is configured in reset block as similar register is located in reset block rather the watchdog driver. May be I should not use Matching on node names, rather use bitmask of bits as you suggested. Please share your views. Regards, Biju
Hi Biju, On Thu, Nov 18, 2021 at 5:01 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > Subject: Re: [PATCH v2 1/3] clk: renesas: rzg2l: Add support for watchdog > > reset selection > > On Wed, Nov 17, 2021 at 9:21 AM Biju Das <biju.das.jz@bp.renesas.com> > > wrote: > > > On the, next version I am planning to introduce the below code for > > > Reset selection based on device availability, instead of selecting all > > > the channels. Is it the right way to do ? please let me know. > > > > > > node = of_find_node_by_name (NULL, NULL, "watchdog@12800800"); if > > > (node && of_device_is_available(node) { > > > // set reset selection for that channel > > > of_node_put(node); > > > } > > > > > > node = of_find_node_by_name (NULL, NULL, "watchdog@12800c00"); if > > > (node && of_device_is_available(node) { > > > // set reset selection for that channel > > > of_node_put(node); > > > } > > > > > > node = of_find_node_by_name (NULL, NULL, "watchdog@12800400"); if > > > (node && of_device_is_available(node) { > > > // set reset selection for that channel > > > of_node_put(node); > > > } > > > > Matching on node names is very fragile. > > Agreed. > > And what if the watchdog node is > > enabled in DT, but the watchdog driver is not available? > We will just configure, but since there is no watch driver available. > I guess nothing will happen. > > > Moreover, this looks like it should not be controlled from the clock > > driver, but from the watchdog driver instead. > > I have referred configure option from reset driver for R-Car, where WDT is configured > in reset block as similar register is located in reset block rather the watchdog driver. > > May be I should not use Matching on node names, rather use bitmask of bits as you suggested. > > Please share your views. On R-Car Gen2 and RZ/G1 SoCs, this is indeed configured by the rcar-rst driver, because the support was added later. Initial R-Car Gen2 revisions had hardware quirks preventing proper operation. On R-Car Gen3 and other RZ/G2 SoCs, this is configured by the boot loader, and it would be great if new SoCs (R-Car V3U, R-Car S4-8, and RZ/G2L) would handle this the same way. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, > Subject: Re: [PATCH v2 1/3] clk: renesas: rzg2l: Add support for watchdog > reset selection > > Hi Biju, > > On Thu, Nov 18, 2021 at 5:01 PM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > > Subject: Re: [PATCH v2 1/3] clk: renesas: rzg2l: Add support for > > > watchdog reset selection On Wed, Nov 17, 2021 at 9:21 AM Biju Das > > > <biju.das.jz@bp.renesas.com> > > > wrote: > > > > On the, next version I am planning to introduce the below code for > > > > Reset selection based on device availability, instead of selecting > > > > all the channels. Is it the right way to do ? please let me know. > > > > > > > > node = of_find_node_by_name (NULL, NULL, "watchdog@12800800"); if > > > > (node && of_device_is_available(node) { > > > > // set reset selection for that channel > > > > of_node_put(node); > > > > } > > > > > > > > node = of_find_node_by_name (NULL, NULL, "watchdog@12800c00"); if > > > > (node && of_device_is_available(node) { > > > > // set reset selection for that channel > > > > of_node_put(node); > > > > } > > > > > > > > node = of_find_node_by_name (NULL, NULL, "watchdog@12800400"); if > > > > (node && of_device_is_available(node) { > > > > // set reset selection for that channel > > > > of_node_put(node); > > > > } > > > > > > Matching on node names is very fragile. > > > > Agreed. > > > > And what if the watchdog node is > > > enabled in DT, but the watchdog driver is not available? > > We will just configure, but since there is no watch driver available. > > I guess nothing will happen. > > > > > Moreover, this looks like it should not be controlled from the clock > > > driver, but from the watchdog driver instead. > > > > I have referred configure option from reset driver for R-Car, where > > WDT is configured in reset block as similar register is located in reset > block rather the watchdog driver. > > > > May be I should not use Matching on node names, rather use bitmask of > bits as you suggested. > > > > Please share your views. > > On R-Car Gen2 and RZ/G1 SoCs, this is indeed configured by the rcar-rst > driver, because the support was added later. Initial R-Car > Gen2 revisions had hardware quirks preventing proper operation. > On R-Car Gen3 and other RZ/G2 SoCs, this is configured by the boot loader, > and it would be great if new SoCs (R-Car V3U, R-Car S4-8, and RZ/G2L) > would handle this the same way. Agreed, will move to bootloader. On R-Car it is done in TF-A(bl2_plat_setup). Will do the same for RZ/G2L as well. Regards, Biju
diff --git a/drivers/clk/renesas/r9a07g044-cpg.c b/drivers/clk/renesas/r9a07g044-cpg.c index 91643b4e1c9c..0bbdc8bd6235 100644 --- a/drivers/clk/renesas/r9a07g044-cpg.c +++ b/drivers/clk/renesas/r9a07g044-cpg.c @@ -8,6 +8,7 @@ #include <linux/clk-provider.h> #include <linux/device.h> #include <linux/init.h> +#include <linux/io.h> #include <linux/kernel.h> #include <dt-bindings/clock/r9a07g044-cpg.h> @@ -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)) + +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, +}; + 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 */ +}; + /** * struct rzg2l_cpg_info - SoC-specific CPG Description * + * @ops: SoC-specific CPG Operations + * * @core_clks: Array of Core Clock definitions * @num_core_clks: Number of entries in core_clks[] * @last_dt_core_clk: ID of the last Core Clock exported to DT @@ -176,6 +187,9 @@ struct rzg2l_reset { * @num_crit_mod_clks: Number of entries in crit_mod_clks[] */ struct rzg2l_cpg_info { + /* CPG Operations */ + const struct rzg2l_cpg_soc_operations *ops; + /* Core Clocks */ const struct cpg_core_clk *core_clks; unsigned int num_core_clks;
This patch adds support for watchdog reset selection. Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- V1->V2: * No Change RFC->V1: * No change --- drivers/clk/renesas/r9a07g044-cpg.c | 22 ++++++++++++++++++++++ drivers/clk/renesas/rzg2l-cpg.c | 6 ++++++ drivers/clk/renesas/rzg2l-cpg.h | 14 ++++++++++++++ 3 files changed, 42 insertions(+)