[PATCH 0/2] mmc: jz4740: Support PLL frequency changes

[PATCH 0/2] mmc: jz4740: Support PLL frequency changes

[Paul Cercueil]

Hi,

This set of two patches enables the MMC driver to cope with the main PLL
updating its rate, typically when the CPU frequency is being updated.

The first patch introduces clk_get_first_to_set_rate(), which will allow
the MMC driver to get a pointer to the clock that will effectively be
modified when calling clk_set_rate(); this is required to avoid a
chicken-and-egg situation with the clock notifier.

If accepted, this function will be reused in a few more drivers which
need to perform the same operation.

The patch to the MMC driver adds a atomic/mutex couple so that the
frequency change will happen when we know that the controller is not in
use.

Cheers,
-Paul

Paul Cercueil (2):
  clk: Add clk_get_first_to_set_rate
  mmc: jz4740: Add support for monitoring PLL clock rate changes

 drivers/clk/clk.c             |  9 +++++
 drivers/mmc/host/jz4740_mmc.c | 70 ++++++++++++++++++++++++++++++++++-
 include/linux/clk.h           | 16 ++++++++
 3 files changed, 94 insertions(+), 1 deletion(-)

-- 
2.30.1

[PATCH 1/2] clk: Add clk_get_first_to_set_rate

[Paul Cercueil]

The purpose of this function is to be used along with the notifier
mechanism.

When a parent clock can see its rate externally changed at any moment,
and a driver needs a specific clock rate to function, it can register a
notifier on the parent clock, and call clk_set_rate() on the base clock
to adjust its frequency according to the new parent clock.

This works fine, until the base clock has the CLK_SET_RATE_PARENT flag
set. In that case, calling clk_set_rate() on the base clock will call
clk_set_rate() on the parent clock, which will trigger the notifier
again, and we're in a loop.

For that reason, we need to register the notifier on the parent clock of
the first ancestor of the base clock that will effectively modify its
rate when clk_set_rate() is called, which we can now obtain with
clk_get_first_to_set_rate().

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/clk/clk.c   |  9 +++++++++
 include/linux/clk.h | 16 ++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 5052541a0986..3fd75e283482 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2450,6 +2450,15 @@ struct clk *clk_get_parent(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_get_parent);
 
+struct clk *clk_get_first_to_set_rate(struct clk *clk)
+{
+	while (clk && (clk->core->flags & CLK_SET_RATE_PARENT))
+		clk = clk_get_parent(clk);
+
+	return clk;
+}
+
+
 static struct clk_core *__clk_init_parent(struct clk_core *core)
 {
 	u8 index = 0;
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 266e8de3cb51..f0ea6ac6aa39 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -766,6 +766,17 @@ struct clk *clk_get_parent(struct clk *clk);
  */
 struct clk *clk_get_sys(const char *dev_id, const char *con_id);
 
+/**
+ * clk_get_first_to_set_rate - get a pointer to the clock that will
+ *   effectively modify its rate when clk_set_rate(clk) is called
+ *   (might be clk itself, or any ancestor)
+ * @clk: clock source
+ *
+ * Returns struct clk corresponding to the matched clock source, or
+ * NULL on error.
+ */
+struct clk *clk_get_first_to_set_rate(struct clk *clk);
+
 /**
  * clk_save_context - save clock context for poweroff
  *
@@ -928,6 +939,11 @@ static inline struct clk *clk_get_parent(struct clk *clk)
 	return NULL;
 }
 
+static inline struct clk *clk_get_first_to_set_rate(struct clk *clk)
+{
+	return NULL;
+}
+
 static inline struct clk *clk_get_sys(const char *dev_id, const char *con_id)
 {
 	return NULL;
-- 
2.30.1

Re: [PATCH 1/2] clk: Add clk_get_first_to_set_rate

[Stephen Boyd]

Quoting Paul Cercueil (2021-03-07 09:07:41)
> The purpose of this function is to be used along with the notifier
> mechanism.
> 
> When a parent clock can see its rate externally changed at any moment,
> and a driver needs a specific clock rate to function, it can register a
> notifier on the parent clock, and call clk_set_rate() on the base clock
> to adjust its frequency according to the new parent clock.

Can the driver use the rate locking mechanism to get a certain rate
instead of registering for notifiers and trying to react to changes?

> 
> This works fine, until the base clock has the CLK_SET_RATE_PARENT flag
> set. In that case, calling clk_set_rate() on the base clock will call
> clk_set_rate() on the parent clock, which will trigger the notifier
> again, and we're in a loop.
> 
> For that reason, we need to register the notifier on the parent clock of
> the first ancestor of the base clock that will effectively modify its
> rate when clk_set_rate() is called, which we can now obtain with
> clk_get_first_to_set_rate().
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

Re: [PATCH 1/2] clk: Add clk_get_first_to_set_rate

[Paul Cercueil]

Hi Stephen,


Le sam. 13 mars 2021 à 14:28, Stephen Boyd <sboyd@kernel.org> a écrit 
:
> Quoting Paul Cercueil (2021-03-07 09:07:41)
>>  The purpose of this function is to be used along with the notifier
>>  mechanism.
>> 
>>  When a parent clock can see its rate externally changed at any 
>> moment,
>>  and a driver needs a specific clock rate to function, it can 
>> register a
>>  notifier on the parent clock, and call clk_set_rate() on the base 
>> clock
>>  to adjust its frequency according to the new parent clock.
> 
> Can the driver use the rate locking mechanism to get a certain rate
> instead of registering for notifiers and trying to react to changes?

You mean with clk_rate_exclusive_get()? That sounds like a good idea, 
but what would happen when a different driver calls the non-exclusive 
clk_set_rate() on this clock (or the parent), would it return -EBUSY, 
lock on a mutex? ...

Cheers,
-Paul

> 
>> 
>>  This works fine, until the base clock has the CLK_SET_RATE_PARENT 
>> flag
>>  set. In that case, calling clk_set_rate() on the base clock will 
>> call
>>  clk_set_rate() on the parent clock, which will trigger the notifier
>>  again, and we're in a loop.
>> 
>>  For that reason, we need to register the notifier on the parent 
>> clock of
>>  the first ancestor of the base clock that will effectively modify 
>> its
>>  rate when clk_set_rate() is called, which we can now obtain with
>>  clk_get_first_to_set_rate().
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>

[PATCH 2/2] mmc: jz4740: Add support for monitoring PLL clock rate changes

[Paul Cercueil]

The main PLL can have its rate changed at any moment. To keep the MMC
clock running at a rate that fits the specifications, we need to
recompute the MMC clock rate every time the PLL rate changes.

Use a mutex to ensure that the MMC is idle before performing the PLL and
MMC rate changes.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/mmc/host/jz4740_mmc.c | 70 ++++++++++++++++++++++++++++++++++-
 1 file changed, 69 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/jz4740_mmc.c b/drivers/mmc/host/jz4740_mmc.c
index b3c636edbb46..1197b8c6b6ed 100644
--- a/drivers/mmc/host/jz4740_mmc.c
+++ b/drivers/mmc/host/jz4740_mmc.c
@@ -18,6 +18,7 @@
 #include <linux/mmc/host.h>
 #include <linux/mmc/slot-gpio.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/of_device.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
@@ -149,6 +150,10 @@ struct jz4740_mmc_host {
 	struct platform_device *pdev;
 	struct clk *clk;
 
+	atomic_t clk_mutex_count;
+	struct mutex clk_mutex;
+	struct notifier_block clock_nb;
+
 	enum jz4740_mmc_version version;
 
 	int irq;
@@ -338,6 +343,9 @@ static void jz4740_mmc_pre_request(struct mmc_host *mmc,
 	struct jz4740_mmc_host *host = mmc_priv(mmc);
 	struct mmc_data *data = mrq->data;
 
+	if (atomic_inc_and_test(&host->clk_mutex_count))
+		mutex_lock(&host->clk_mutex);
+
 	if (!host->use_dma)
 		return;
 
@@ -353,6 +361,9 @@ static void jz4740_mmc_post_request(struct mmc_host *mmc,
 	struct jz4740_mmc_host *host = mmc_priv(mmc);
 	struct mmc_data *data = mrq->data;
 
+	if (atomic_dec_return(&host->clk_mutex_count) == -1)
+		mutex_unlock(&host->clk_mutex);
+
 	if (data && data->host_cookie != COOKIE_UNMAPPED)
 		jz4740_mmc_dma_unmap(host, data);
 
@@ -955,6 +966,48 @@ static const struct mmc_host_ops jz4740_mmc_ops = {
 	.enable_sdio_irq = jz4740_mmc_enable_sdio_irq,
 };
 
+static inline struct jz4740_mmc_host *
+jz4740_mmc_nb_get_priv(struct notifier_block *nb)
+{
+	return container_of(nb, struct jz4740_mmc_host, clock_nb);
+}
+
+static struct clk *jz4740_mmc_get_parent_clk(struct clk *clk)
+{
+	/*
+	 * Return the first clock above the one that will effectively modify
+	 * its rate when clk_set_rate(clk) is called.
+	 */
+	clk = clk_get_first_to_set_rate(clk);
+
+	return clk_get_parent(clk);
+}
+
+static int jz4740_mmc_update_clk(struct notifier_block *nb,
+				 unsigned long action,
+				 void *data)
+{
+	struct jz4740_mmc_host *host = jz4740_mmc_nb_get_priv(nb);
+
+	/*
+	 * PLL may have changed its frequency; our clock may be running above
+	 * spec. Wait until MMC is idle (using host->clk_mutex) before changing
+	 * the PLL clock, and after it's done, reset our clock rate.
+	 */
+
+	switch (action) {
+	case PRE_RATE_CHANGE:
+		mutex_lock(&host->clk_mutex);
+		break;
+	default:
+		clk_set_rate(host->clk, host->mmc->f_max);
+		mutex_unlock(&host->clk_mutex);
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
 static const struct of_device_id jz4740_mmc_of_match[] = {
 	{ .compatible = "ingenic,jz4740-mmc", .data = (void *) JZ_MMC_JZ4740 },
 	{ .compatible = "ingenic,jz4725b-mmc", .data = (void *)JZ_MMC_JZ4725B },
@@ -971,6 +1024,7 @@ static int jz4740_mmc_probe(struct platform_device* pdev)
 	struct mmc_host *mmc;
 	struct jz4740_mmc_host *host;
 	const struct of_device_id *match;
+	struct clk *parent_clk;
 
 	mmc = mmc_alloc_host(sizeof(struct jz4740_mmc_host), &pdev->dev);
 	if (!mmc) {
@@ -1058,12 +1112,24 @@ static int jz4740_mmc_probe(struct platform_device* pdev)
 		goto err_free_irq;
 	host->use_dma = !ret;
 
+	atomic_set(&host->clk_mutex_count, -1);
+	mutex_init(&host->clk_mutex);
+	host->clock_nb.notifier_call = jz4740_mmc_update_clk;
+
+	parent_clk = jz4740_mmc_get_parent_clk(host->clk);
+
+	ret = clk_notifier_register(parent_clk, &host->clock_nb);
+	if (ret) {
+		dev_err(&pdev->dev, "Unable to register clock notifier\n");
+		goto err_release_dma;
+	}
+
 	platform_set_drvdata(pdev, host);
 	ret = mmc_add_host(mmc);
 
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to add mmc host: %d\n", ret);
-		goto err_release_dma;
+		goto err_unregister_clk_notifier;
 	}
 	dev_info(&pdev->dev, "Ingenic SD/MMC card driver registered\n");
 
@@ -1074,6 +1140,8 @@ static int jz4740_mmc_probe(struct platform_device* pdev)
 
 	return 0;
 
+err_unregister_clk_notifier:
+	clk_notifier_unregister(parent_clk, &host->clock_nb);
 err_release_dma:
 	if (host->use_dma)
 		jz4740_mmc_release_dma_channels(host);
-- 
2.30.1

Re: [PATCH 2/2] mmc: jz4740: Add support for monitoring PLL clock rate changes

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1642 bytes --]

Hi Paul,

I love your patch! Yet something to improve:

[auto build test ERROR on clk/clk-next]
[also build test ERROR on linus/master v5.12-rc2]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Paul-Cercueil/mmc-jz4740-Support-PLL-frequency-changes/20210308-010934
base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
config: mips-allmodconfig (attached as .config)
compiler: mips-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/a639c4eca022073332cb4ebbdf280dc90e438a2a
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Paul-Cercueil/mmc-jz4740-Support-PLL-frequency-changes/20210308-010934
        git checkout a639c4eca022073332cb4ebbdf280dc90e438a2a
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=mips 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "clk_get_first_to_set_rate" [drivers/mmc/host/jz4740_mmc.ko] undefined!

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 69857 bytes --]

Re: [PATCH 2/2] mmc: jz4740: Add support for monitoring PLL clock rate changes

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1678 bytes --]

Hi Paul,

I love your patch! Yet something to improve:

[auto build test ERROR on clk/clk-next]
[also build test ERROR on linus/master v5.12-rc2]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Paul-Cercueil/mmc-jz4740-Support-PLL-frequency-changes/20210308-010934
base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
config: mips-allmodconfig (attached as .config)
compiler: mips-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/a639c4eca022073332cb4ebbdf280dc90e438a2a
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Paul-Cercueil/mmc-jz4740-Support-PLL-frequency-changes/20210308-010934
        git checkout a639c4eca022073332cb4ebbdf280dc90e438a2a
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=mips 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "clk_get_first_to_set_rate" [drivers/mmc/host/jz4740_mmc.ko] undefined!

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 69857 bytes --]

Re: [PATCH 2/2] mmc: jz4740: Add support for monitoring PLL clock rate changes

[Paul Cercueil]

Le dim. 7 mars 2021 à 17:07, Paul Cercueil <paul@crapouillou.net> a 
écrit :
> The main PLL can have its rate changed at any moment. To keep the MMC
> clock running at a rate that fits the specifications, we need to
> recompute the MMC clock rate every time the PLL rate changes.
> 
> Use a mutex to ensure that the MMC is idle before performing the PLL 
> and
> MMC rate changes.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/mmc/host/jz4740_mmc.c | 70 
> ++++++++++++++++++++++++++++++++++-
>  1 file changed, 69 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/jz4740_mmc.c 
> b/drivers/mmc/host/jz4740_mmc.c
> index b3c636edbb46..1197b8c6b6ed 100644
> --- a/drivers/mmc/host/jz4740_mmc.c
> +++ b/drivers/mmc/host/jz4740_mmc.c
> @@ -18,6 +18,7 @@
>  #include <linux/mmc/host.h>
>  #include <linux/mmc/slot-gpio.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/of_device.h>
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/platform_device.h>
> @@ -149,6 +150,10 @@ struct jz4740_mmc_host {
>  	struct platform_device *pdev;
>  	struct clk *clk;
> 
> +	atomic_t clk_mutex_count;
> +	struct mutex clk_mutex;
> +	struct notifier_block clock_nb;
> +
>  	enum jz4740_mmc_version version;
> 
>  	int irq;
> @@ -338,6 +343,9 @@ static void jz4740_mmc_pre_request(struct 
> mmc_host *mmc,
>  	struct jz4740_mmc_host *host = mmc_priv(mmc);
>  	struct mmc_data *data = mrq->data;
> 
> +	if (atomic_inc_and_test(&host->clk_mutex_count))
> +		mutex_lock(&host->clk_mutex);

There's an obvious race here, let me rewrite this using the proper 
locking mechanism.

-Paul

> +
>  	if (!host->use_dma)
>  		return;
> 
> @@ -353,6 +361,9 @@ static void jz4740_mmc_post_request(struct 
> mmc_host *mmc,
>  	struct jz4740_mmc_host *host = mmc_priv(mmc);
>  	struct mmc_data *data = mrq->data;
> 
> +	if (atomic_dec_return(&host->clk_mutex_count) == -1)
> +		mutex_unlock(&host->clk_mutex);
> +
>  	if (data && data->host_cookie != COOKIE_UNMAPPED)
>  		jz4740_mmc_dma_unmap(host, data);
> 
> @@ -955,6 +966,48 @@ static const struct mmc_host_ops jz4740_mmc_ops 
> = {
>  	.enable_sdio_irq = jz4740_mmc_enable_sdio_irq,
>  };
> 
> +static inline struct jz4740_mmc_host *
> +jz4740_mmc_nb_get_priv(struct notifier_block *nb)
> +{
> +	return container_of(nb, struct jz4740_mmc_host, clock_nb);
> +}
> +
> +static struct clk *jz4740_mmc_get_parent_clk(struct clk *clk)
> +{
> +	/*
> +	 * Return the first clock above the one that will effectively modify
> +	 * its rate when clk_set_rate(clk) is called.
> +	 */
> +	clk = clk_get_first_to_set_rate(clk);
> +
> +	return clk_get_parent(clk);
> +}
> +
> +static int jz4740_mmc_update_clk(struct notifier_block *nb,
> +				 unsigned long action,
> +				 void *data)
> +{
> +	struct jz4740_mmc_host *host = jz4740_mmc_nb_get_priv(nb);
> +
> +	/*
> +	 * PLL may have changed its frequency; our clock may be running 
> above
> +	 * spec. Wait until MMC is idle (using host->clk_mutex) before 
> changing
> +	 * the PLL clock, and after it's done, reset our clock rate.
> +	 */
> +
> +	switch (action) {
> +	case PRE_RATE_CHANGE:
> +		mutex_lock(&host->clk_mutex);
> +		break;
> +	default:
> +		clk_set_rate(host->clk, host->mmc->f_max);
> +		mutex_unlock(&host->clk_mutex);
> +		break;
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
>  static const struct of_device_id jz4740_mmc_of_match[] = {
>  	{ .compatible = "ingenic,jz4740-mmc", .data = (void *) 
> JZ_MMC_JZ4740 },
>  	{ .compatible = "ingenic,jz4725b-mmc", .data = (void 
> *)JZ_MMC_JZ4725B },
> @@ -971,6 +1024,7 @@ static int jz4740_mmc_probe(struct 
> platform_device* pdev)
>  	struct mmc_host *mmc;
>  	struct jz4740_mmc_host *host;
>  	const struct of_device_id *match;
> +	struct clk *parent_clk;
> 
>  	mmc = mmc_alloc_host(sizeof(struct jz4740_mmc_host), &pdev->dev);
>  	if (!mmc) {
> @@ -1058,12 +1112,24 @@ static int jz4740_mmc_probe(struct 
> platform_device* pdev)
>  		goto err_free_irq;
>  	host->use_dma = !ret;
> 
> +	atomic_set(&host->clk_mutex_count, -1);
> +	mutex_init(&host->clk_mutex);
> +	host->clock_nb.notifier_call = jz4740_mmc_update_clk;
> +
> +	parent_clk = jz4740_mmc_get_parent_clk(host->clk);
> +
> +	ret = clk_notifier_register(parent_clk, &host->clock_nb);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Unable to register clock notifier\n");
> +		goto err_release_dma;
> +	}
> +
>  	platform_set_drvdata(pdev, host);
>  	ret = mmc_add_host(mmc);
> 
>  	if (ret) {
>  		dev_err(&pdev->dev, "Failed to add mmc host: %d\n", ret);
> -		goto err_release_dma;
> +		goto err_unregister_clk_notifier;
>  	}
>  	dev_info(&pdev->dev, "Ingenic SD/MMC card driver registered\n");
> 
> @@ -1074,6 +1140,8 @@ static int jz4740_mmc_probe(struct 
> platform_device* pdev)
> 
>  	return 0;
> 
> +err_unregister_clk_notifier:
> +	clk_notifier_unregister(parent_clk, &host->clock_nb);
>  err_release_dma:
>  	if (host->use_dma)
>  		jz4740_mmc_release_dma_channels(host);
> --
> 2.30.1
>