From: Claudiu Beznea <[email protected]>
Refactor SD MUX driver to be able to reuse the same code on RZ/G3S.
RZ/G2{L, UL} has a limitation with regards to switching the clock source
for SD MUX (MUX clock source has to be switched to 266MHz before switching
b/w 533MHz and 400MHz). This limitation has been introduced as a clock
notifier that is registered on platform based initialization data thus the
SD MUX code could be reused on RZ/G3S.
As both RZ/G2{L, UL} and RZ/G3S has specific bits in specific registers
to check if the clock switching has been done, this configuration (register
offset, register bits and bits width) is now passed though
struct cpg_core_clk::sconf (status configuration) from platform specific
initialization code.
Along with struct cpg_core_clk::sconf the mux table indexes is also
passed from platform specific initialization code.
Signed-off-by: Claudiu Beznea <[email protected]>
---
drivers/clk/renesas/r9a07g043-cpg.c | 12 +-
drivers/clk/renesas/r9a07g044-cpg.c | 12 +-
drivers/clk/renesas/rzg2l-cpg.c | 174 +++++++++++++++++++---------
drivers/clk/renesas/rzg2l-cpg.h | 17 ++-
4 files changed, 154 insertions(+), 61 deletions(-)
diff --git a/drivers/clk/renesas/r9a07g043-cpg.c b/drivers/clk/renesas/r9a07g043-cpg.c
index e87cbb54a640..791a38f1d2ec 100644
--- a/drivers/clk/renesas/r9a07g043-cpg.c
+++ b/drivers/clk/renesas/r9a07g043-cpg.c
@@ -21,6 +21,10 @@
#define G2UL_SEL_SDHI0 SEL_PLL_PACK(G2UL_CPG_PL2SDHI_DSEL, 0, 2)
#define G2UL_SEL_SDHI1 SEL_PLL_PACK(G2UL_CPG_PL2SDHI_DSEL, 4, 2)
+/* Clock status configuration. */
+#define G2UL_SEL_SDHI0_STS SEL_PLL_PACK(CPG_CLKSTATUS, 28, 1)
+#define G2UL_SEL_SDHI1_STS SEL_PLL_PACK(CPG_CLKSTATUS, 29, 1)
+
enum clk_ids {
/* Core Clock Outputs exported to DT */
LAST_DT_CORE_CLK = R9A07G043_CLK_P0_DIV2,
@@ -85,6 +89,8 @@ static const char * const sel_pll3_3[] = { ".pll3_533", ".pll3_400" };
static const char * const sel_pll6_2[] = { ".pll6_250", ".pll5_250" };
static const char * const sel_shdi[] = { ".clk_533", ".clk_400", ".clk_266" };
+static const u32 mtable_sdhi[] = {1, 2, 3};
+
static const struct cpg_core_clk r9a07g043_core_clks[] __initconst = {
/* External Clock Inputs */
DEF_INPUT("extal", CLK_EXTAL),
@@ -130,8 +136,10 @@ static const struct cpg_core_clk r9a07g043_core_clks[] __initconst = {
DEF_MUX("HP", R9A07G043_CLK_HP, SEL_PLL6_2, sel_pll6_2),
DEF_FIXED("SPI0", R9A07G043_CLK_SPI0, CLK_DIV_PLL3_C, 1, 2),
DEF_FIXED("SPI1", R9A07G043_CLK_SPI1, CLK_DIV_PLL3_C, 1, 4),
- DEF_SD_MUX("SD0", R9A07G043_CLK_SD0, G2UL_SEL_SDHI0, sel_shdi),
- DEF_SD_MUX("SD1", R9A07G043_CLK_SD1, G2UL_SEL_SDHI1, sel_shdi),
+ DEF_SD_MUX("SD0", R9A07G043_CLK_SD0, G2UL_SEL_SDHI0, G2UL_SEL_SDHI0_STS, sel_shdi,
+ mtable_sdhi, 0, SD_MUX_NOTIF),
+ DEF_SD_MUX("SD1", R9A07G043_CLK_SD1, G2UL_SEL_SDHI1, G2UL_SEL_SDHI0_STS, sel_shdi,
+ mtable_sdhi, 0, SD_MUX_NOTIF),
DEF_FIXED("SD0_DIV4", CLK_SD0_DIV4, R9A07G043_CLK_SD0, 1, 4),
DEF_FIXED("SD1_DIV4", CLK_SD1_DIV4, R9A07G043_CLK_SD1, 1, 4),
};
diff --git a/drivers/clk/renesas/r9a07g044-cpg.c b/drivers/clk/renesas/r9a07g044-cpg.c
index 8911f6053a9f..ad9059116603 100644
--- a/drivers/clk/renesas/r9a07g044-cpg.c
+++ b/drivers/clk/renesas/r9a07g044-cpg.c
@@ -22,6 +22,10 @@
#define G2L_SEL_SDHI0 SEL_PLL_PACK(G2L_CPG_PL2SDHI_DSEL, 0, 2)
#define G2L_SEL_SDHI1 SEL_PLL_PACK(G2L_CPG_PL2SDHI_DSEL, 4, 2)
+/* Clock status configuration. */
+#define G2L_SEL_SDHI0_STS SEL_PLL_PACK(CPG_CLKSTATUS, 28, 1)
+#define G2L_SEL_SDHI1_STS SEL_PLL_PACK(CPG_CLKSTATUS, 29, 1)
+
enum clk_ids {
/* Core Clock Outputs exported to DT */
LAST_DT_CORE_CLK = R9A07G054_CLK_DRP_A,
@@ -105,6 +109,8 @@ static const char * const sel_pll6_2[] = { ".pll6_250", ".pll5_250" };
static const char * const sel_shdi[] = { ".clk_533", ".clk_400", ".clk_266" };
static const char * const sel_gpu2[] = { ".pll6", ".pll3_div2_2" };
+static const u32 mtable_sdhi[] = {1, 2, 3};
+
static const struct {
struct cpg_core_clk common[56];
#ifdef CONFIG_CLK_R9A07G054
@@ -170,8 +176,10 @@ static const struct {
DEF_MUX("HP", R9A07G044_CLK_HP, SEL_PLL6_2, sel_pll6_2),
DEF_FIXED("SPI0", R9A07G044_CLK_SPI0, CLK_DIV_PLL3_C, 1, 2),
DEF_FIXED("SPI1", R9A07G044_CLK_SPI1, CLK_DIV_PLL3_C, 1, 4),
- DEF_SD_MUX("SD0", R9A07G044_CLK_SD0, G2L_SEL_SDHI0, sel_shdi),
- DEF_SD_MUX("SD1", R9A07G044_CLK_SD1, G2L_SEL_SDHI1, sel_shdi),
+ DEF_SD_MUX("SD0", R9A07G044_CLK_SD0, G2L_SEL_SDHI0, G2L_SEL_SDHI0_STS, sel_shdi,
+ mtable_sdhi, 0, SD_MUX_NOTIF),
+ DEF_SD_MUX("SD1", R9A07G044_CLK_SD1, G2L_SEL_SDHI1, G2L_SEL_SDHI0_STS, sel_shdi,
+ mtable_sdhi, 0, SD_MUX_NOTIF),
DEF_FIXED("SD0_DIV4", CLK_SD0_DIV4, R9A07G044_CLK_SD0, 1, 4),
DEF_FIXED("SD1_DIV4", CLK_SD1_DIV4, R9A07G044_CLK_SD1, 1, 4),
DEF_DIV("G", R9A07G044_CLK_G, CLK_SEL_GPU2, DIVGPU, dtable_1_8),
diff --git a/drivers/clk/renesas/rzg2l-cpg.c b/drivers/clk/renesas/rzg2l-cpg.c
index 120bc8d51691..dd9229f0be7d 100644
--- a/drivers/clk/renesas/rzg2l-cpg.c
+++ b/drivers/clk/renesas/rzg2l-cpg.c
@@ -62,25 +62,29 @@
* struct clk_hw_data - clock hardware data
* @hw: clock hw
* @conf: clock configuration (register offset, shift, width)
+ * @sconf: clock status configuration (register offset, shift, width)
* @priv: CPG private data structure
*/
struct clk_hw_data {
struct clk_hw hw;
u32 conf;
+ u32 sconf;
struct rzg2l_cpg_priv *priv;
};
#define to_clk_hw_data(_hw) container_of(_hw, struct clk_hw_data, hw)
/**
- * struct sd_hw_data - SD clock hardware data
+ * struct sd_mux_hw_data - SD MUX clock hardware data
* @hw_data: clock hw data
+ * @mtable: clock mux table
*/
-struct sd_hw_data {
+struct sd_mux_hw_data {
struct clk_hw_data hw_data;
+ const u32 *mtable;
};
-#define to_sd_hw_data(_hw) container_of(_hw, struct sd_hw_data, hw_data)
+#define to_sd_mux_hw_data(_hw) container_of(_hw, struct sd_mux_hw_data, hw_data)
struct rzg2l_pll5_param {
u32 pl5_fracin;
@@ -137,6 +141,77 @@ static void rzg2l_cpg_del_clk_provider(void *data)
of_clk_del_provider(data);
}
+/* Must be called in atomic context. */
+static int rzg2l_cpg_wait_clk_update_done(void __iomem *base, u32 conf)
+{
+ u32 bitmask = GENMASK(GET_WIDTH(conf) - 1, 0) << GET_SHIFT(conf);
+ u32 off = GET_REG_OFFSET(conf);
+ u32 val;
+
+ return readl_poll_timeout_atomic(base + off, val, !(val & bitmask), 100, 20000);
+}
+
+int rzg2l_cpg_sd_mux_clk_notifier(struct notifier_block *nb, unsigned long event,
+ void *data)
+{
+ struct clk_notifier_data *cnd = data;
+ struct clk_hw *hw = __clk_get_hw(cnd->clk);
+ struct clk_hw_data *clk_hw_data = to_clk_hw_data(hw);
+ struct rzg2l_cpg_priv *priv = clk_hw_data->priv;
+ u32 off = GET_REG_OFFSET(clk_hw_data->conf);
+ u32 shift = GET_SHIFT(clk_hw_data->conf);
+ const u32 clk_src_266 = 3;
+ unsigned long flags;
+ u32 bitmask;
+ int ret;
+
+ if (event != PRE_RATE_CHANGE || (cnd->new_rate / MEGA == 266))
+ return 0;
+
+ spin_lock_irqsave(&priv->rmw_lock, flags);
+
+ /*
+ * As per the HW manual, we should not directly switch from 533 MHz to
+ * 400 MHz and vice versa. To change the setting from 2’b01 (533 MHz)
+ * to 2’b10 (400 MHz) or vice versa, Switch to 2’b11 (266 MHz) first,
+ * and then switch to the target setting (2’b01 (533 MHz) or 2’b10
+ * (400 MHz)).
+ * Setting a value of '0' to the SEL_SDHI0_SET or SEL_SDHI1_SET clock
+ * switching register is prohibited.
+ * The clock mux has 3 input clocks(533 MHz, 400 MHz, and 266 MHz), and
+ * the index to value mapping is done by adding 1 to the index.
+ */
+ bitmask = (GENMASK(GET_WIDTH(clk_hw_data->conf) - 1, 0) << shift) << 16;
+ writel(bitmask | (clk_src_266 << shift), priv->base + off);
+
+ /* Wait for the update done. */
+ ret = rzg2l_cpg_wait_clk_update_done(priv->base, clk_hw_data->sconf);
+
+ spin_unlock_irqrestore(&priv->rmw_lock, flags);
+
+ if (ret)
+ dev_err(priv->dev, "failed to switch to safe clk source\n");
+
+ return ret;
+}
+
+static int rzg2l_register_notifier(struct clk_hw *hw, const struct cpg_core_clk *core,
+ struct rzg2l_cpg_priv *priv)
+{
+ struct notifier_block *nb;
+
+ if (!core->notifier)
+ return 0;
+
+ nb = devm_kzalloc(priv->dev, sizeof(*nb), GFP_KERNEL);
+ if (!nb)
+ return -ENOMEM;
+
+ nb->notifier_call = core->notifier;
+
+ return clk_notifier_register(hw->clk, nb);
+}
+
static struct clk * __init
rzg2l_cpg_div_clk_register(const struct cpg_core_clk *core,
struct clk **clks,
@@ -197,72 +272,54 @@ rzg2l_cpg_mux_clk_register(const struct cpg_core_clk *core,
return clk_hw->clk;
}
-static int rzg2l_cpg_sd_clk_mux_set_parent(struct clk_hw *hw, u8 index)
+static u8 rzg2l_cpg_sd_mux_clk_get_parent(struct clk_hw *hw)
+{
+ struct clk_hw_data *clk_hw_data = to_clk_hw_data(hw);
+ struct sd_mux_hw_data *sd_mux_hw_data = to_sd_mux_hw_data(clk_hw_data);
+ struct rzg2l_cpg_priv *priv = clk_hw_data->priv;
+ u32 val;
+
+ val = readl(priv->base + GET_REG_OFFSET(clk_hw_data->conf));
+ val >>= GET_SHIFT(clk_hw_data->conf);
+ val &= GENMASK(GET_WIDTH(clk_hw_data->conf) - 1, 0);
+
+ return clk_mux_val_to_index(hw, sd_mux_hw_data->mtable, CLK_MUX_ROUND_CLOSEST, val);
+}
+
+static int rzg2l_cpg_sd_mux_clk_set_parent(struct clk_hw *hw, u8 index)
{
struct clk_hw_data *clk_hw_data = to_clk_hw_data(hw);
+ struct sd_mux_hw_data *sd_mux_hw_data = to_sd_mux_hw_data(clk_hw_data);
struct rzg2l_cpg_priv *priv = clk_hw_data->priv;
u32 off = GET_REG_OFFSET(clk_hw_data->conf);
u32 shift = GET_SHIFT(clk_hw_data->conf);
- const u32 clk_src_266 = 2;
- u32 msk, val, bitmask;
unsigned long flags;
+ u32 bitmask, val;
int ret;
- /*
- * As per the HW manual, we should not directly switch from 533 MHz to
- * 400 MHz and vice versa. To change the setting from 2’b01 (533 MHz)
- * to 2’b10 (400 MHz) or vice versa, Switch to 2’b11 (266 MHz) first,
- * and then switch to the target setting (2’b01 (533 MHz) or 2’b10
- * (400 MHz)).
- * Setting a value of '0' to the SEL_SDHI0_SET or SEL_SDHI1_SET clock
- * switching register is prohibited.
- * The clock mux has 3 input clocks(533 MHz, 400 MHz, and 266 MHz), and
- * the index to value mapping is done by adding 1 to the index.
- */
+ val = clk_mux_index_to_val(sd_mux_hw_data->mtable, CLK_MUX_ROUND_CLOSEST, index);
+
bitmask = (GENMASK(GET_WIDTH(clk_hw_data->conf) - 1, 0) << shift) << 16;
+
spin_lock_irqsave(&priv->rmw_lock, flags);
- if (index != clk_src_266) {
- writel(bitmask | ((clk_src_266 + 1) << shift), priv->base + off);
- msk = off ? CPG_CLKSTATUS_SELSDHI1_STS : CPG_CLKSTATUS_SELSDHI0_STS;
+ writel(bitmask | (val << shift), priv->base + off);
- ret = readl_poll_timeout_atomic(priv->base + CPG_CLKSTATUS, val,
- !(val & msk), 100,
- CPG_SDHI_CLK_SWITCH_STATUS_TIMEOUT_US);
- if (ret)
- goto unlock;
- }
+ /* Wait for the update done. */
+ ret = rzg2l_cpg_wait_clk_update_done(priv->base, clk_hw_data->sconf);
- writel(bitmask | ((index + 1) << shift), priv->base + off);
-
- ret = readl_poll_timeout_atomic(priv->base + CPG_CLKSTATUS, val,
- !(val & msk), 100,
- CPG_SDHI_CLK_SWITCH_STATUS_TIMEOUT_US);
-unlock:
spin_unlock_irqrestore(&priv->rmw_lock, flags);
if (ret)
- dev_err(priv->dev, "failed to switch clk source\n");
+ dev_err(priv->dev, "Failed to switch parent\n");
return ret;
}
-static u8 rzg2l_cpg_sd_clk_mux_get_parent(struct clk_hw *hw)
-{
- struct clk_hw_data *clk_hw_data = to_clk_hw_data(hw);
- struct rzg2l_cpg_priv *priv = clk_hw_data->priv;
- u32 val = readl(priv->base + GET_REG_OFFSET(clk_hw_data->conf));
-
- val >>= GET_SHIFT(clk_hw_data->conf);
- val &= GENMASK(GET_WIDTH(clk_hw_data->conf) - 1, 0);
-
- return val ? --val : val;
-}
-
static const struct clk_ops rzg2l_cpg_sd_clk_mux_ops = {
.determine_rate = __clk_mux_determine_rate_closest,
- .set_parent = rzg2l_cpg_sd_clk_mux_set_parent,
- .get_parent = rzg2l_cpg_sd_clk_mux_get_parent,
+ .set_parent = rzg2l_cpg_sd_mux_clk_set_parent,
+ .get_parent = rzg2l_cpg_sd_mux_clk_get_parent,
};
static struct clk * __init
@@ -270,31 +327,40 @@ rzg2l_cpg_sd_mux_clk_register(const struct cpg_core_clk *core,
void __iomem *base,
struct rzg2l_cpg_priv *priv)
{
- struct sd_hw_data *sd_hw_data;
+ struct sd_mux_hw_data *sd_mux_hw_data;
struct clk_init_data init;
struct clk_hw *clk_hw;
int ret;
- sd_hw_data = devm_kzalloc(priv->dev, sizeof(*sd_hw_data), GFP_KERNEL);
- if (!sd_hw_data)
+ sd_mux_hw_data = devm_kzalloc(priv->dev, sizeof(*sd_mux_hw_data), GFP_KERNEL);
+ if (!sd_mux_hw_data)
return ERR_PTR(-ENOMEM);
- sd_hw_data->hw_data.priv = priv;
- sd_hw_data->hw_data.conf = core->conf;
+ sd_mux_hw_data->hw_data.priv = priv;
+ sd_mux_hw_data->hw_data.conf = core->conf;
+ sd_mux_hw_data->hw_data.sconf = core->sconf;
+ sd_mux_hw_data->mtable = core->mtable;
init.name = core->name;
init.ops = &rzg2l_cpg_sd_clk_mux_ops;
- init.flags = 0;
+ init.flags = core->flag;
init.num_parents = core->num_parents;
init.parent_names = core->parent_names;
- clk_hw = &sd_hw_data->hw_data.hw;
+ clk_hw = &sd_mux_hw_data->hw_data.hw;
clk_hw->init = &init;
ret = devm_clk_hw_register(priv->dev, clk_hw);
if (ret)
return ERR_PTR(ret);
+ ret = rzg2l_register_notifier(clk_hw, core, priv);
+ if (ret) {
+ dev_err(priv->dev, "Failed to register notifier for %s\n",
+ core->name);
+ return ERR_PTR(ret);
+ }
+
return clk_hw->clk;
}
diff --git a/drivers/clk/renesas/rzg2l-cpg.h b/drivers/clk/renesas/rzg2l-cpg.h
index 99a82567d1f8..140b6b04a091 100644
--- a/drivers/clk/renesas/rzg2l-cpg.h
+++ b/drivers/clk/renesas/rzg2l-cpg.h
@@ -9,6 +9,8 @@
#ifndef __RENESAS_RZG2L_CPG_H__
#define __RENESAS_RZG2L_CPG_H__
+#include <linux/notifier.h>
+
#define CPG_SIPLL5_STBY (0x140)
#define CPG_SIPLL5_CLK1 (0x144)
#define CPG_SIPLL5_CLK3 (0x14C)
@@ -86,8 +88,11 @@ struct cpg_core_clk {
unsigned int mult;
unsigned int type;
unsigned int conf;
+ unsigned int sconf;
const struct clk_div_table *dtable;
+ const u32 *mtable;
const char * const *parent_names;
+ notifier_fn_t notifier;
u32 flag;
u32 mux_flags;
int num_parents;
@@ -150,10 +155,11 @@ enum clk_types {
.parent_names = _parent_names, \
.num_parents = ARRAY_SIZE(_parent_names), \
.mux_flags = CLK_MUX_READ_ONLY)
-#define DEF_SD_MUX(_name, _id, _conf, _parent_names) \
- DEF_TYPE(_name, _id, CLK_TYPE_SD_MUX, .conf = _conf, \
+#define DEF_SD_MUX(_name, _id, _conf, _sconf, _parent_names, _mtable, _clk_flags, _notifier) \
+ DEF_TYPE(_name, _id, CLK_TYPE_SD_MUX, .conf = _conf, .sconf = _sconf, \
.parent_names = _parent_names, \
- .num_parents = ARRAY_SIZE(_parent_names))
+ .num_parents = ARRAY_SIZE(_parent_names), \
+ .mtable = _mtable, .flag = _clk_flags, .notifier = _notifier)
#define DEF_PLL5_FOUTPOSTDIV(_name, _id, _parent) \
DEF_TYPE(_name, _id, CLK_TYPE_SIPLL5, .parent = _parent)
#define DEF_PLL5_4_MUX(_name, _id, _conf, _parent_names) \
@@ -272,4 +278,9 @@ extern const struct rzg2l_cpg_info r9a07g044_cpg_info;
extern const struct rzg2l_cpg_info r9a07g054_cpg_info;
extern const struct rzg2l_cpg_info r9a09g011_cpg_info;
+int rzg2l_cpg_sd_mux_clk_notifier(struct notifier_block *nb, unsigned long event, void *data);
+
+/* Macros to be used in platform specific initialization code. */
+#define SD_MUX_NOTIF (&rzg2l_cpg_sd_mux_clk_notifier)
+
#endif
--
2.39.2
Hi Claudiu,
Thanks for your patch!
On Tue, Sep 12, 2023 at 6:52 AM Claudiu <[email protected]> wrote:
> From: Claudiu Beznea <[email protected]>
>
> Refactor SD MUX driver to be able to reuse the same code on RZ/G3S.
> RZ/G2{L, UL} has a limitation with regards to switching the clock source
> for SD MUX (MUX clock source has to be switched to 266MHz before switching
> b/w 533MHz and 400MHz). This limitation has been introduced as a clock
> notifier that is registered on platform based initialization data thus the
> SD MUX code could be reused on RZ/G3S.
>
> As both RZ/G2{L, UL} and RZ/G3S has specific bits in specific registers
> to check if the clock switching has been done, this configuration (register
> offset, register bits and bits width) is now passed though
> struct cpg_core_clk::sconf (status configuration) from platform specific
> initialization code.
>
> Along with struct cpg_core_clk::sconf the mux table indexes is also
indices are
> passed from platform specific initialization code.
Please also mention the passing of the mux flags, which is added so
you can pass CLK_SET_PARENT_GATE for G3S_SEL_PLL4 later.
> Signed-off-by: Claudiu Beznea <[email protected]>
> --- a/drivers/clk/renesas/r9a07g043-cpg.c
> +++ b/drivers/clk/renesas/r9a07g043-cpg.c
> @@ -21,6 +21,10 @@
> #define G2UL_SEL_SDHI0 SEL_PLL_PACK(G2UL_CPG_PL2SDHI_DSEL, 0, 2)
> #define G2UL_SEL_SDHI1 SEL_PLL_PACK(G2UL_CPG_PL2SDHI_DSEL, 4, 2)
>
> +/* Clock status configuration. */
> +#define G2UL_SEL_SDHI0_STS SEL_PLL_PACK(CPG_CLKSTATUS, 28, 1)
> +#define G2UL_SEL_SDHI1_STS SEL_PLL_PACK(CPG_CLKSTATUS, 29, 1)
Just like in [PATCH 17/37], there is no real need for the "G2UL_"-prefix.
> +
> enum clk_ids {
> /* Core Clock Outputs exported to DT */
> LAST_DT_CORE_CLK = R9A07G043_CLK_P0_DIV2,
> @@ -85,6 +89,8 @@ static const char * const sel_pll3_3[] = { ".pll3_533", ".pll3_400" };
> static const char * const sel_pll6_2[] = { ".pll6_250", ".pll5_250" };
> static const char * const sel_shdi[] = { ".clk_533", ".clk_400", ".clk_266" };
>
> +static const u32 mtable_sdhi[] = {1, 2, 3};
{ 1, 2, 3 };
(everywhere)
> +
> static const struct cpg_core_clk r9a07g043_core_clks[] __initconst = {
> /* External Clock Inputs */
> DEF_INPUT("extal", CLK_EXTAL),
> @@ -137,6 +141,77 @@ static void rzg2l_cpg_del_clk_provider(void *data)
> of_clk_del_provider(data);
> }
>
> +/* Must be called in atomic context. */
> +static int rzg2l_cpg_wait_clk_update_done(void __iomem *base, u32 conf)
> +{
> + u32 bitmask = GENMASK(GET_WIDTH(conf) - 1, 0) << GET_SHIFT(conf);
> + u32 off = GET_REG_OFFSET(conf);
> + u32 val;
> +
> + return readl_poll_timeout_atomic(base + off, val, !(val & bitmask), 100, 20000);
> +}
> +
> +int rzg2l_cpg_sd_mux_clk_notifier(struct notifier_block *nb, unsigned long event,
> + void *data)
> +{
> + struct clk_notifier_data *cnd = data;
> + struct clk_hw *hw = __clk_get_hw(cnd->clk);
> + struct clk_hw_data *clk_hw_data = to_clk_hw_data(hw);
> + struct rzg2l_cpg_priv *priv = clk_hw_data->priv;
> + u32 off = GET_REG_OFFSET(clk_hw_data->conf);
> + u32 shift = GET_SHIFT(clk_hw_data->conf);
> + const u32 clk_src_266 = 3;
> + unsigned long flags;
> + u32 bitmask;
> + int ret;
> +
> + if (event != PRE_RATE_CHANGE || (cnd->new_rate / MEGA == 266))
> + return 0;
> +
> + spin_lock_irqsave(&priv->rmw_lock, flags);
> +
> + /*
> + * As per the HW manual, we should not directly switch from 533 MHz to
> + * 400 MHz and vice versa. To change the setting from 2’b01 (533 MHz)
> + * to 2’b10 (400 MHz) or vice versa, Switch to 2’b11 (266 MHz) first,
> + * and then switch to the target setting (2’b01 (533 MHz) or 2’b10
> + * (400 MHz)).
> + * Setting a value of '0' to the SEL_SDHI0_SET or SEL_SDHI1_SET clock
> + * switching register is prohibited.
> + * The clock mux has 3 input clocks(533 MHz, 400 MHz, and 266 MHz), and
> + * the index to value mapping is done by adding 1 to the index.
> + */
> + bitmask = (GENMASK(GET_WIDTH(clk_hw_data->conf) - 1, 0) << shift) << 16;
> + writel(bitmask | (clk_src_266 << shift), priv->base + off);
> +
> + /* Wait for the update done. */
> + ret = rzg2l_cpg_wait_clk_update_done(priv->base, clk_hw_data->sconf);
> +
> + spin_unlock_irqrestore(&priv->rmw_lock, flags);
> +
> + if (ret)
> + dev_err(priv->dev, "failed to switch to safe clk source\n");
> +
> + return ret;
> +}
> +
> +static int rzg2l_register_notifier(struct clk_hw *hw, const struct cpg_core_clk *core,
> + struct rzg2l_cpg_priv *priv)
> +{
> + struct notifier_block *nb;
> +
> + if (!core->notifier)
> + return 0;
> +
> + nb = devm_kzalloc(priv->dev, sizeof(*nb), GFP_KERNEL);
> + if (!nb)
> + return -ENOMEM;
> +
> + nb->notifier_call = core->notifier;
> +
> + return clk_notifier_register(hw->clk, nb);
> +}
I am not sure a notifier is the best solution. Basically on RZ/G2L,
when changing the parent clock, you need to switch to a fixed
intermediate parent first.
What about just replacing the fixed clk_src_266 in the old
rzg2l_cpg_sd_mux_clk_set_parent() by a (signed) integer in
sd_mux_hw_data (specified in DEF_SD_MUX()), representing the index
of the intermediate clock?
-1 would mean an intermediate parent is not needed.
> +
> static struct clk * __init
> rzg2l_cpg_div_clk_register(const struct cpg_core_clk *core,
> struct clk **clks,
> @@ -197,72 +272,54 @@ rzg2l_cpg_mux_clk_register(const struct cpg_core_clk *core,
> return clk_hw->clk;
> }
>
> -static int rzg2l_cpg_sd_clk_mux_set_parent(struct clk_hw *hw, u8 index)
> +static u8 rzg2l_cpg_sd_mux_clk_get_parent(struct clk_hw *hw)
> +{
> + struct clk_hw_data *clk_hw_data = to_clk_hw_data(hw);
> + struct sd_mux_hw_data *sd_mux_hw_data = to_sd_mux_hw_data(clk_hw_data);
> + struct rzg2l_cpg_priv *priv = clk_hw_data->priv;
> + u32 val;
> +
> + val = readl(priv->base + GET_REG_OFFSET(clk_hw_data->conf));
> + val >>= GET_SHIFT(clk_hw_data->conf);
> + val &= GENMASK(GET_WIDTH(clk_hw_data->conf) - 1, 0);
> +
> + return clk_mux_val_to_index(hw, sd_mux_hw_data->mtable, CLK_MUX_ROUND_CLOSEST, val);
> +}
> +
> +static int rzg2l_cpg_sd_mux_clk_set_parent(struct clk_hw *hw, u8 index)
> {
> struct clk_hw_data *clk_hw_data = to_clk_hw_data(hw);
> + struct sd_mux_hw_data *sd_mux_hw_data = to_sd_mux_hw_data(clk_hw_data);
> struct rzg2l_cpg_priv *priv = clk_hw_data->priv;
> u32 off = GET_REG_OFFSET(clk_hw_data->conf);
> u32 shift = GET_SHIFT(clk_hw_data->conf);
> - const u32 clk_src_266 = 2;
> - u32 msk, val, bitmask;
> unsigned long flags;
> + u32 bitmask, val;
> int ret;
>
> - /*
> - * As per the HW manual, we should not directly switch from 533 MHz to
> - * 400 MHz and vice versa. To change the setting from 2’b01 (533 MHz)
> - * to 2’b10 (400 MHz) or vice versa, Switch to 2’b11 (266 MHz) first,
> - * and then switch to the target setting (2’b01 (533 MHz) or 2’b10
> - * (400 MHz)).
> - * Setting a value of '0' to the SEL_SDHI0_SET or SEL_SDHI1_SET clock
> - * switching register is prohibited.
> - * The clock mux has 3 input clocks(533 MHz, 400 MHz, and 266 MHz), and
> - * the index to value mapping is done by adding 1 to the index.
> - */
> + val = clk_mux_index_to_val(sd_mux_hw_data->mtable, CLK_MUX_ROUND_CLOSEST, index);
> +
> bitmask = (GENMASK(GET_WIDTH(clk_hw_data->conf) - 1, 0) << shift) << 16;
> +
> spin_lock_irqsave(&priv->rmw_lock, flags);
> - if (index != clk_src_266) {
> - writel(bitmask | ((clk_src_266 + 1) << shift), priv->base + off);
>
> - msk = off ? CPG_CLKSTATUS_SELSDHI1_STS : CPG_CLKSTATUS_SELSDHI0_STS;
> + writel(bitmask | (val << shift), priv->base + off);
>
> - ret = readl_poll_timeout_atomic(priv->base + CPG_CLKSTATUS, val,
> - !(val & msk), 100,
> - CPG_SDHI_CLK_SWITCH_STATUS_TIMEOUT_US);
> - if (ret)
> - goto unlock;
> - }
> + /* Wait for the update done. */
> + ret = rzg2l_cpg_wait_clk_update_done(priv->base, clk_hw_data->sconf);
>
> - writel(bitmask | ((index + 1) << shift), priv->base + off);
> -
> - ret = readl_poll_timeout_atomic(priv->base + CPG_CLKSTATUS, val,
> - !(val & msk), 100,
> - CPG_SDHI_CLK_SWITCH_STATUS_TIMEOUT_US);
> -unlock:
> spin_unlock_irqrestore(&priv->rmw_lock, flags);
>
> if (ret)
> - dev_err(priv->dev, "failed to switch clk source\n");
> + dev_err(priv->dev, "Failed to switch parent\n");
>
> return ret;
> }
>
> -static u8 rzg2l_cpg_sd_clk_mux_get_parent(struct clk_hw *hw)
> -{
> - struct clk_hw_data *clk_hw_data = to_clk_hw_data(hw);
> - struct rzg2l_cpg_priv *priv = clk_hw_data->priv;
> - u32 val = readl(priv->base + GET_REG_OFFSET(clk_hw_data->conf));
> -
> - val >>= GET_SHIFT(clk_hw_data->conf);
> - val &= GENMASK(GET_WIDTH(clk_hw_data->conf) - 1, 0);
> -
> - return val ? --val : val;
> -}
This would be easier to review if you kept the order and name of the
.[gs]et_parent() callbacks.
> -
> static const struct clk_ops rzg2l_cpg_sd_clk_mux_ops = {
> .determine_rate = __clk_mux_determine_rate_closest,
> - .set_parent = rzg2l_cpg_sd_clk_mux_set_parent,
> - .get_parent = rzg2l_cpg_sd_clk_mux_get_parent,
> + .set_parent = rzg2l_cpg_sd_mux_clk_set_parent,
> + .get_parent = rzg2l_cpg_sd_mux_clk_get_parent,
> };
> --- a/drivers/clk/renesas/rzg2l-cpg.h
> +++ b/drivers/clk/renesas/rzg2l-cpg.h
> @@ -86,8 +88,11 @@ struct cpg_core_clk {
> unsigned int mult;
> unsigned int type;
> unsigned int conf;
> + unsigned int sconf;
> const struct clk_div_table *dtable;
> + const u32 *mtable;
> const char * const *parent_names;
> + notifier_fn_t notifier;
FTR, this is growing each core clock entry by 24 bytes (on arm64).
We really should start using unions, but that is a bigger overhaul...
> u32 flag;
> u32 mux_flags;
> int num_parents;
> @@ -272,4 +278,9 @@ extern const struct rzg2l_cpg_info r9a07g044_cpg_info;
> extern const struct rzg2l_cpg_info r9a07g054_cpg_info;
> extern const struct rzg2l_cpg_info r9a09g011_cpg_info;
>
> +int rzg2l_cpg_sd_mux_clk_notifier(struct notifier_block *nb, unsigned long event, void *data);
> +
> +/* Macros to be used in platform specific initialization code. */
> +#define SD_MUX_NOTIF (&rzg2l_cpg_sd_mux_clk_notifier)
Any specific reason you are adding this macro?
What is wrong with using &rzg2l_cpg_sd_mux_clk_notifier directly?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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 Claudiu,
On Fri, Sep 15, 2023 at 9:30 AM claudiu beznea <[email protected]> wrote:
> On 14.09.2023 18:18, Geert Uytterhoeven wrote:
> > On Tue, Sep 12, 2023 at 6:52 AM Claudiu <[email protected]> wrote:
> >> From: Claudiu Beznea <[email protected]>
> >>
> >> Refactor SD MUX driver to be able to reuse the same code on RZ/G3S.
> >> RZ/G2{L, UL} has a limitation with regards to switching the clock source
> >> for SD MUX (MUX clock source has to be switched to 266MHz before switching
> >> b/w 533MHz and 400MHz). This limitation has been introduced as a clock
> >> notifier that is registered on platform based initialization data thus the
> >> SD MUX code could be reused on RZ/G3S.
> >>
> >> As both RZ/G2{L, UL} and RZ/G3S has specific bits in specific registers
> >> to check if the clock switching has been done, this configuration (register
> >> offset, register bits and bits width) is now passed though
> >> struct cpg_core_clk::sconf (status configuration) from platform specific
> >> initialization code.
> >>
> >> Along with struct cpg_core_clk::sconf the mux table indexes is also
> >
> > indices are
> >
> >> passed from platform specific initialization code.
> >
> > Please also mention the passing of the mux flags, which is added so
> > you can pass CLK_SET_PARENT_GATE for G3S_SEL_PLL4 later.
>
> Ok.
>
> >
> >> Signed-off-by: Claudiu Beznea <[email protected]>
> >
> >> --- a/drivers/clk/renesas/r9a07g043-cpg.c
> >> +++ b/drivers/clk/renesas/r9a07g043-cpg.c
> >> @@ -21,6 +21,10 @@
> >> #define G2UL_SEL_SDHI0 SEL_PLL_PACK(G2UL_CPG_PL2SDHI_DSEL, 0, 2)
> >> #define G2UL_SEL_SDHI1 SEL_PLL_PACK(G2UL_CPG_PL2SDHI_DSEL, 4, 2)
> >>
> >> +/* Clock status configuration. */
> >> +#define G2UL_SEL_SDHI0_STS SEL_PLL_PACK(CPG_CLKSTATUS, 28, 1)
> >> +#define G2UL_SEL_SDHI1_STS SEL_PLL_PACK(CPG_CLKSTATUS, 29, 1)
> >
> > Just like in [PATCH 17/37], there is no real need for the "G2UL_"-prefix.
>
> Ok, I ususlly tend to guard everything with a proper namespace.
Sure, in many cases, that makes good sense.
In this case, not having the prefix makes it easier to compare clock tables:
soc-dts-diff -b drivers/clk/renesas/r9a07g04[34]-cpg.c
(soc-dts-diff ignores the SoC part number, and can be found at
https://github.com/geertu/linux-scripts)
> >> +int rzg2l_cpg_sd_mux_clk_notifier(struct notifier_block *nb, unsigned long event,
> >> + void *data)
> >> +{
> >> + struct clk_notifier_data *cnd = data;
> >> + struct clk_hw *hw = __clk_get_hw(cnd->clk);
> >> + struct clk_hw_data *clk_hw_data = to_clk_hw_data(hw);
> >> + struct rzg2l_cpg_priv *priv = clk_hw_data->priv;
> >> + u32 off = GET_REG_OFFSET(clk_hw_data->conf);
> >> + u32 shift = GET_SHIFT(clk_hw_data->conf);
> >> + const u32 clk_src_266 = 3;
> >> + unsigned long flags;
> >> + u32 bitmask;
> >> + int ret;
> >> +
> >> + if (event != PRE_RATE_CHANGE || (cnd->new_rate / MEGA == 266))
> >> + return 0;
> >> +
> >> + spin_lock_irqsave(&priv->rmw_lock, flags);
> >> +
> >> + /*
> >> + * As per the HW manual, we should not directly switch from 533 MHz to
> >> + * 400 MHz and vice versa. To change the setting from 2’b01 (533 MHz)
> >> + * to 2’b10 (400 MHz) or vice versa, Switch to 2’b11 (266 MHz) first,
> >> + * and then switch to the target setting (2’b01 (533 MHz) or 2’b10
> >> + * (400 MHz)).
> >> + * Setting a value of '0' to the SEL_SDHI0_SET or SEL_SDHI1_SET clock
> >> + * switching register is prohibited.
> >> + * The clock mux has 3 input clocks(533 MHz, 400 MHz, and 266 MHz), and
> >> + * the index to value mapping is done by adding 1 to the index.
> >> + */
> >> + bitmask = (GENMASK(GET_WIDTH(clk_hw_data->conf) - 1, 0) << shift) << 16;
> >> + writel(bitmask | (clk_src_266 << shift), priv->base + off);
> >> +
> >> + /* Wait for the update done. */
> >> + ret = rzg2l_cpg_wait_clk_update_done(priv->base, clk_hw_data->sconf);
> >> +
> >> + spin_unlock_irqrestore(&priv->rmw_lock, flags);
> >> +
> >> + if (ret)
> >> + dev_err(priv->dev, "failed to switch to safe clk source\n");
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static int rzg2l_register_notifier(struct clk_hw *hw, const struct cpg_core_clk *core,
> >> + struct rzg2l_cpg_priv *priv)
> >> +{
> >> + struct notifier_block *nb;
> >> +
> >> + if (!core->notifier)
> >> + return 0;
> >> +
> >> + nb = devm_kzalloc(priv->dev, sizeof(*nb), GFP_KERNEL);
> >> + if (!nb)
> >> + return -ENOMEM;
> >> +
> >> + nb->notifier_call = core->notifier;
> >> +
> >> + return clk_notifier_register(hw->clk, nb);
> >> +}
> >
> > I am not sure a notifier is the best solution. Basically on RZ/G2L,
> > when changing the parent clock, you need to switch to a fixed
> > intermediate parent first.
> > What about just replacing the fixed clk_src_266 in the old
> > rzg2l_cpg_sd_mux_clk_set_parent() by a (signed) integer in
> > sd_mux_hw_data (specified in DEF_SD_MUX()), representing the index
> > of the intermediate clock?
> > -1 would mean an intermediate parent is not needed.
>
> That should work too but .set_rate() will be bulky for both mux and div.
>
> The idea was to have the .set_rate() common to the mux and the platform
> specificities implemented as notifiers and only the needed platforms to
> instantiate the notifier. And the same approach to be used by the divider
> (patch "[PATCH 19/37] clk: renesas: rzg2l: add a divider clock for RZ/G3S")
>
> With this it looked to me that the final code is more compact .set_rate
> being simple and platform specificities being implemented in notifier
> (valid for both MUX and DIV). The infrastructure is already there for
> notifier to be called before .set_rate().
TBH, I am not that familiar with clock notifiers, so I could use some
guidance from the clock maintainers.
Mike/Stephen: Are clock notifiers the right approach, here and in
[PATCH 19.37]?
> >> --- a/drivers/clk/renesas/rzg2l-cpg.h
> >> +++ b/drivers/clk/renesas/rzg2l-cpg.h
> >> @@ -272,4 +278,9 @@ extern const struct rzg2l_cpg_info r9a07g044_cpg_info;
> >> extern const struct rzg2l_cpg_info r9a07g054_cpg_info;
> >> extern const struct rzg2l_cpg_info r9a09g011_cpg_info;
> >>
> >> +int rzg2l_cpg_sd_mux_clk_notifier(struct notifier_block *nb, unsigned long event, void *data);
> >> +
> >> +/* Macros to be used in platform specific initialization code. */
> >> +#define SD_MUX_NOTIF (&rzg2l_cpg_sd_mux_clk_notifier)
> >
> > Any specific reason you are adding this macro?
>
> It looked to me like a better name to be used in platform specific drivers.
>
> > What is wrong with using &rzg2l_cpg_sd_mux_clk_notifier directly?
>
> Nothing, just that it is a longer than SD_MUX_NOTIF.
It adds another level of indirection for the casual reviewer, and needs
replacement when an SoC arrives that needs a different SD mux notifier.
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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,
On 14.09.2023 18:18, Geert Uytterhoeven wrote:
> Hi Claudiu,
>
> Thanks for your patch!
>
> On Tue, Sep 12, 2023 at 6:52 AM Claudiu <[email protected]> wrote:
>> From: Claudiu Beznea <[email protected]>
>>
>> Refactor SD MUX driver to be able to reuse the same code on RZ/G3S.
>> RZ/G2{L, UL} has a limitation with regards to switching the clock source
>> for SD MUX (MUX clock source has to be switched to 266MHz before switching
>> b/w 533MHz and 400MHz). This limitation has been introduced as a clock
>> notifier that is registered on platform based initialization data thus the
>> SD MUX code could be reused on RZ/G3S.
>>
>> As both RZ/G2{L, UL} and RZ/G3S has specific bits in specific registers
>> to check if the clock switching has been done, this configuration (register
>> offset, register bits and bits width) is now passed though
>> struct cpg_core_clk::sconf (status configuration) from platform specific
>> initialization code.
>>
>> Along with struct cpg_core_clk::sconf the mux table indexes is also
>
> indices are
>
>> passed from platform specific initialization code.
>
> Please also mention the passing of the mux flags, which is added so
> you can pass CLK_SET_PARENT_GATE for G3S_SEL_PLL4 later.
Ok.
>
>> Signed-off-by: Claudiu Beznea <[email protected]>
>
>> --- a/drivers/clk/renesas/r9a07g043-cpg.c
>> +++ b/drivers/clk/renesas/r9a07g043-cpg.c
>> @@ -21,6 +21,10 @@
>> #define G2UL_SEL_SDHI0 SEL_PLL_PACK(G2UL_CPG_PL2SDHI_DSEL, 0, 2)
>> #define G2UL_SEL_SDHI1 SEL_PLL_PACK(G2UL_CPG_PL2SDHI_DSEL, 4, 2)
>>
>> +/* Clock status configuration. */
>> +#define G2UL_SEL_SDHI0_STS SEL_PLL_PACK(CPG_CLKSTATUS, 28, 1)
>> +#define G2UL_SEL_SDHI1_STS SEL_PLL_PACK(CPG_CLKSTATUS, 29, 1)
>
> Just like in [PATCH 17/37], there is no real need for the "G2UL_"-prefix.
Ok, I ususlly tend to guard everything with a proper namespace.
>
>> +
>> enum clk_ids {
>> /* Core Clock Outputs exported to DT */
>> LAST_DT_CORE_CLK = R9A07G043_CLK_P0_DIV2,
>> @@ -85,6 +89,8 @@ static const char * const sel_pll3_3[] = { ".pll3_533", ".pll3_400" };
>> static const char * const sel_pll6_2[] = { ".pll6_250", ".pll5_250" };
>> static const char * const sel_shdi[] = { ".clk_533", ".clk_400", ".clk_266" };
>>
>> +static const u32 mtable_sdhi[] = {1, 2, 3};
>
> { 1, 2, 3 };
>
> (everywhere)
ok
>
>> +
>> static const struct cpg_core_clk r9a07g043_core_clks[] __initconst = {
>> /* External Clock Inputs */
>> DEF_INPUT("extal", CLK_EXTAL),
>
>> @@ -137,6 +141,77 @@ static void rzg2l_cpg_del_clk_provider(void *data)
>> of_clk_del_provider(data);
>> }
>>
>> +/* Must be called in atomic context. */
>> +static int rzg2l_cpg_wait_clk_update_done(void __iomem *base, u32 conf)
>> +{
>> + u32 bitmask = GENMASK(GET_WIDTH(conf) - 1, 0) << GET_SHIFT(conf);
>> + u32 off = GET_REG_OFFSET(conf);
>> + u32 val;
>> +
>> + return readl_poll_timeout_atomic(base + off, val, !(val & bitmask), 100, 20000);
>> +}
>> +
>> +int rzg2l_cpg_sd_mux_clk_notifier(struct notifier_block *nb, unsigned long event,
>> + void *data)
>> +{
>> + struct clk_notifier_data *cnd = data;
>> + struct clk_hw *hw = __clk_get_hw(cnd->clk);
>> + struct clk_hw_data *clk_hw_data = to_clk_hw_data(hw);
>> + struct rzg2l_cpg_priv *priv = clk_hw_data->priv;
>> + u32 off = GET_REG_OFFSET(clk_hw_data->conf);
>> + u32 shift = GET_SHIFT(clk_hw_data->conf);
>> + const u32 clk_src_266 = 3;
>> + unsigned long flags;
>> + u32 bitmask;
>> + int ret;
>> +
>> + if (event != PRE_RATE_CHANGE || (cnd->new_rate / MEGA == 266))
>> + return 0;
>> +
>> + spin_lock_irqsave(&priv->rmw_lock, flags);
>> +
>> + /*
>> + * As per the HW manual, we should not directly switch from 533 MHz to
>> + * 400 MHz and vice versa. To change the setting from 2’b01 (533 MHz)
>> + * to 2’b10 (400 MHz) or vice versa, Switch to 2’b11 (266 MHz) first,
>> + * and then switch to the target setting (2’b01 (533 MHz) or 2’b10
>> + * (400 MHz)).
>> + * Setting a value of '0' to the SEL_SDHI0_SET or SEL_SDHI1_SET clock
>> + * switching register is prohibited.
>> + * The clock mux has 3 input clocks(533 MHz, 400 MHz, and 266 MHz), and
>> + * the index to value mapping is done by adding 1 to the index.
>> + */
>> + bitmask = (GENMASK(GET_WIDTH(clk_hw_data->conf) - 1, 0) << shift) << 16;
>> + writel(bitmask | (clk_src_266 << shift), priv->base + off);
>> +
>> + /* Wait for the update done. */
>> + ret = rzg2l_cpg_wait_clk_update_done(priv->base, clk_hw_data->sconf);
>> +
>> + spin_unlock_irqrestore(&priv->rmw_lock, flags);
>> +
>> + if (ret)
>> + dev_err(priv->dev, "failed to switch to safe clk source\n");
>> +
>> + return ret;
>> +}
>> +
>> +static int rzg2l_register_notifier(struct clk_hw *hw, const struct cpg_core_clk *core,
>> + struct rzg2l_cpg_priv *priv)
>> +{
>> + struct notifier_block *nb;
>> +
>> + if (!core->notifier)
>> + return 0;
>> +
>> + nb = devm_kzalloc(priv->dev, sizeof(*nb), GFP_KERNEL);
>> + if (!nb)
>> + return -ENOMEM;
>> +
>> + nb->notifier_call = core->notifier;
>> +
>> + return clk_notifier_register(hw->clk, nb);
>> +}
>
> I am not sure a notifier is the best solution. Basically on RZ/G2L,
> when changing the parent clock, you need to switch to a fixed
> intermediate parent first.
> What about just replacing the fixed clk_src_266 in the old
> rzg2l_cpg_sd_mux_clk_set_parent() by a (signed) integer in
> sd_mux_hw_data (specified in DEF_SD_MUX()), representing the index
> of the intermediate clock?
> -1 would mean an intermediate parent is not needed.
That should work too but .set_rate() will be bulky for both mux and div.
The idea was to have the .set_rate() common to the mux and the platform
specificities implemented as notifiers and only the needed platforms to
instantiate the notifier. And the same approach to be used by the divider
(patch "[PATCH 19/37] clk: renesas: rzg2l: add a divider clock for RZ/G3S")
With this it looked to me that the final code is more compact .set_rate
being simple and platform specificities being implemented in notifier
(valid for both MUX and DIV). The infrastructure is already there for
notifier to be called before .set_rate().
>
>> +
>> static struct clk * __init
>> rzg2l_cpg_div_clk_register(const struct cpg_core_clk *core,
>> struct clk **clks,
>> @@ -197,72 +272,54 @@ rzg2l_cpg_mux_clk_register(const struct cpg_core_clk *core,
>> return clk_hw->clk;
>> }
>>
>> -static int rzg2l_cpg_sd_clk_mux_set_parent(struct clk_hw *hw, u8 index)
>> +static u8 rzg2l_cpg_sd_mux_clk_get_parent(struct clk_hw *hw)
>> +{
>> + struct clk_hw_data *clk_hw_data = to_clk_hw_data(hw);
>> + struct sd_mux_hw_data *sd_mux_hw_data = to_sd_mux_hw_data(clk_hw_data);
>> + struct rzg2l_cpg_priv *priv = clk_hw_data->priv;
>> + u32 val;
>> +
>> + val = readl(priv->base + GET_REG_OFFSET(clk_hw_data->conf));
>> + val >>= GET_SHIFT(clk_hw_data->conf);
>> + val &= GENMASK(GET_WIDTH(clk_hw_data->conf) - 1, 0);
>> +
>> + return clk_mux_val_to_index(hw, sd_mux_hw_data->mtable, CLK_MUX_ROUND_CLOSEST, val);
>> +}
>> +
>> +static int rzg2l_cpg_sd_mux_clk_set_parent(struct clk_hw *hw, u8 index)
>> {
>> struct clk_hw_data *clk_hw_data = to_clk_hw_data(hw);
>> + struct sd_mux_hw_data *sd_mux_hw_data = to_sd_mux_hw_data(clk_hw_data);
>> struct rzg2l_cpg_priv *priv = clk_hw_data->priv;
>> u32 off = GET_REG_OFFSET(clk_hw_data->conf);
>> u32 shift = GET_SHIFT(clk_hw_data->conf);
>> - const u32 clk_src_266 = 2;
>> - u32 msk, val, bitmask;
>> unsigned long flags;
>> + u32 bitmask, val;
>> int ret;
>>
>> - /*
>> - * As per the HW manual, we should not directly switch from 533 MHz to
>> - * 400 MHz and vice versa. To change the setting from 2’b01 (533 MHz)
>> - * to 2’b10 (400 MHz) or vice versa, Switch to 2’b11 (266 MHz) first,
>> - * and then switch to the target setting (2’b01 (533 MHz) or 2’b10
>> - * (400 MHz)).
>> - * Setting a value of '0' to the SEL_SDHI0_SET or SEL_SDHI1_SET clock
>> - * switching register is prohibited.
>> - * The clock mux has 3 input clocks(533 MHz, 400 MHz, and 266 MHz), and
>> - * the index to value mapping is done by adding 1 to the index.
>> - */
>> + val = clk_mux_index_to_val(sd_mux_hw_data->mtable, CLK_MUX_ROUND_CLOSEST, index);
>> +
>> bitmask = (GENMASK(GET_WIDTH(clk_hw_data->conf) - 1, 0) << shift) << 16;
>> +
>> spin_lock_irqsave(&priv->rmw_lock, flags);
>> - if (index != clk_src_266) {
>> - writel(bitmask | ((clk_src_266 + 1) << shift), priv->base + off);
>>
>> - msk = off ? CPG_CLKSTATUS_SELSDHI1_STS : CPG_CLKSTATUS_SELSDHI0_STS;
>> + writel(bitmask | (val << shift), priv->base + off);
>>
>> - ret = readl_poll_timeout_atomic(priv->base + CPG_CLKSTATUS, val,
>> - !(val & msk), 100,
>> - CPG_SDHI_CLK_SWITCH_STATUS_TIMEOUT_US);
>> - if (ret)
>> - goto unlock;
>> - }
>> + /* Wait for the update done. */
>> + ret = rzg2l_cpg_wait_clk_update_done(priv->base, clk_hw_data->sconf);
>>
>> - writel(bitmask | ((index + 1) << shift), priv->base + off);
>> -
>> - ret = readl_poll_timeout_atomic(priv->base + CPG_CLKSTATUS, val,
>> - !(val & msk), 100,
>> - CPG_SDHI_CLK_SWITCH_STATUS_TIMEOUT_US);
>> -unlock:
>> spin_unlock_irqrestore(&priv->rmw_lock, flags);
>>
>> if (ret)
>> - dev_err(priv->dev, "failed to switch clk source\n");
>> + dev_err(priv->dev, "Failed to switch parent\n");
>>
>> return ret;
>> }
>>
>> -static u8 rzg2l_cpg_sd_clk_mux_get_parent(struct clk_hw *hw)
>> -{
>> - struct clk_hw_data *clk_hw_data = to_clk_hw_data(hw);
>> - struct rzg2l_cpg_priv *priv = clk_hw_data->priv;
>> - u32 val = readl(priv->base + GET_REG_OFFSET(clk_hw_data->conf));
>> -
>> - val >>= GET_SHIFT(clk_hw_data->conf);
>> - val &= GENMASK(GET_WIDTH(clk_hw_data->conf) - 1, 0);
>> -
>> - return val ? --val : val;
>> -}
>
> This would be easier to review if you kept the order and name of the
> .[gs]et_parent() callbacks.
Appologies about that, I'll try to adapt it in the next version.
>
>> -
>> static const struct clk_ops rzg2l_cpg_sd_clk_mux_ops = {
>> .determine_rate = __clk_mux_determine_rate_closest,
>> - .set_parent = rzg2l_cpg_sd_clk_mux_set_parent,
>> - .get_parent = rzg2l_cpg_sd_clk_mux_get_parent,
>> + .set_parent = rzg2l_cpg_sd_mux_clk_set_parent,
>> + .get_parent = rzg2l_cpg_sd_mux_clk_get_parent,
>> };
>
>> --- a/drivers/clk/renesas/rzg2l-cpg.h
>> +++ b/drivers/clk/renesas/rzg2l-cpg.h
>
>> @@ -86,8 +88,11 @@ struct cpg_core_clk {
>> unsigned int mult;
>> unsigned int type;
>> unsigned int conf;
>> + unsigned int sconf;
>> const struct clk_div_table *dtable;
>> + const u32 *mtable;
>> const char * const *parent_names;
>> + notifier_fn_t notifier;
>
> FTR, this is growing each core clock entry by 24 bytes (on arm64).
> We really should start using unions, but that is a bigger overhaul...
>
>> u32 flag;
>> u32 mux_flags;
>> int num_parents;
>
>> @@ -272,4 +278,9 @@ extern const struct rzg2l_cpg_info r9a07g044_cpg_info;
>> extern const struct rzg2l_cpg_info r9a07g054_cpg_info;
>> extern const struct rzg2l_cpg_info r9a09g011_cpg_info;
>>
>> +int rzg2l_cpg_sd_mux_clk_notifier(struct notifier_block *nb, unsigned long event, void *data);
>> +
>> +/* Macros to be used in platform specific initialization code. */
>> +#define SD_MUX_NOTIF (&rzg2l_cpg_sd_mux_clk_notifier)
>
> Any specific reason you are adding this macro?
It looked to me like a better name to be used in platform specific drivers.
> What is wrong with using &rzg2l_cpg_sd_mux_clk_notifier directly?
Nothing, just that it is a longer than SD_MUX_NOTIF.
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> 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