diff mbox series

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

Message ID 20210307170742.70949-3-paul@crapouillou.net (mailing list archive)
State Changes Requested, archived
Headers show
Series mmc: jz4740: Support PLL frequency changes | expand

Commit Message

Paul Cercueil March 7, 2021, 5:07 p.m. UTC 
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(-)

Comments

kernel test robot March 8, 2021, 3:58 a.m. UTC | #1 
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
Paul Cercueil March 8, 2021, 2:46 p.m. UTC | #2 
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
>
diff mbox series

Patch

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);