| Message ID | 20211111115427.8228-2-biju.das.jz@bp.renesas.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/3] clk: renesas: rzg2l: Add support for watchdog reset selection | expand |
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, 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, > > 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
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(+)