[PATCH v2 0/4] mmc: sdhci-of-aspeed: Fixes for AST2600 eMMC support

[PATCH v2 0/4] mmc: sdhci-of-aspeed: Fixes for AST2600 eMMC support

[Andrew Jeffery]

Hello,

I've added a couple of patches since v1 of this series. The horizon has
broadened slightly with a fix for SPARC builds as well in patch 1/4. Ulf
suggested a minor cleanup on v1 with respect to handling of the current clock
value, so that's now patch 2/4. Patches 3/4 and 4/4 are as they were in v1.

The v1 series can be found here:

https://patchwork.ozlabs.org/cover/1155757/

Please review!

Andrew

Andrew Jeffery (4):
  mmc: sdhci-of-aspeed: Fix link failure for SPARC
  mmc: sdhci-of-aspeed: Drop redundant assignment to host->clock
  mmc: sdhci-of-aspeed: Uphold clocks-on post-condition of set_clock()
  mmc: sdhci-of-aspeed: Allow max-frequency limitation of SDCLK

 drivers/mmc/host/sdhci-of-aspeed.c | 62 ++++++++++++++++++++----------
 1 file changed, 42 insertions(+), 20 deletions(-)

-- 
2.20.1

[PATCH v2 1/4] mmc: sdhci-of-aspeed: Fix link failure for SPARC

[Andrew Jeffery]

Resolves the following build error reported by the 0-day bot:

    ERROR: "of_platform_device_create" [drivers/mmc/host/sdhci-of-aspeed.ko] undefined!

SPARC does not set CONFIG_OF_ADDRESS so the symbol is missing. Guard the
callsite to maintain build coverage for the rest of the driver.

Reported-by: kbuild test robot <lkp@intel.com>
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/mmc/host/sdhci-of-aspeed.c | 38 ++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 13 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
index d5acb5afc50f..96ca494752c5 100644
--- a/drivers/mmc/host/sdhci-of-aspeed.c
+++ b/drivers/mmc/host/sdhci-of-aspeed.c
@@ -224,10 +224,30 @@ static struct platform_driver aspeed_sdhci_driver = {
 	.remove		= aspeed_sdhci_remove,
 };
 
-static int aspeed_sdc_probe(struct platform_device *pdev)
-
+static int aspeed_sdc_create_sdhcis(struct platform_device *pdev)
 {
+#if defined(CONFIG_OF_ADDRESS)
 	struct device_node *parent, *child;
+
+	parent = pdev->dev.of_node;
+
+	for_each_available_child_of_node(parent, child) {
+		struct platform_device *cpdev;
+
+		cpdev = of_platform_device_create(child, NULL, &pdev->dev);
+		if (!cpdev) {
+			of_node_put(child);
+			return -ENODEV;
+		}
+	}
+#endif
+
+	return 0;
+}
+
+static int aspeed_sdc_probe(struct platform_device *pdev)
+
+{
 	struct aspeed_sdc *sdc;
 	int ret;
 
@@ -256,17 +276,9 @@ static int aspeed_sdc_probe(struct platform_device *pdev)
 
 	dev_set_drvdata(&pdev->dev, sdc);
 
-	parent = pdev->dev.of_node;
-	for_each_available_child_of_node(parent, child) {
-		struct platform_device *cpdev;
-
-		cpdev = of_platform_device_create(child, NULL, &pdev->dev);
-		if (!cpdev) {
-			of_node_put(child);
-			ret = -ENODEV;
-			goto err_clk;
-		}
-	}
+	ret = aspeed_sdc_create_sdhcis(pdev);
+	if (ret)
+		goto err_clk;
 
 	return 0;
 
-- 
2.20.1

[PATCH v2 2/4] mmc: sdhci-of-aspeed: Drop redundant assignment to host->clock

[Andrew Jeffery]

host->clock is already managed by sdhci_set_ios().

Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/mmc/host/sdhci-of-aspeed.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
index 96ca494752c5..213b3dbd49ef 100644
--- a/drivers/mmc/host/sdhci-of-aspeed.c
+++ b/drivers/mmc/host/sdhci-of-aspeed.c
@@ -61,7 +61,7 @@ static void aspeed_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 	sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
 
 	if (clock == 0)
-		goto out;
+		return;
 
 	for (div = 1; div < 256; div *= 2) {
 		if ((host->max_clk / div) <= clock)
@@ -72,9 +72,6 @@ static void aspeed_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 	clk = div << SDHCI_DIVIDER_SHIFT;
 
 	sdhci_enable_clk(host, clk);
-
-out:
-	host->clock = clock;
 }
 
 static void aspeed_sdhci_set_bus_width(struct sdhci_host *host, int width)
-- 
2.20.1

[PATCH v2 3/4] mmc: sdhci-of-aspeed: Uphold clocks-on post-condition of set_clock()

[Andrew Jeffery]

The early-exit didn't seem to matter on the AST2500, but on the AST2600
the SD clock genuinely may not be running on entry to
aspeed_sdhci_set_clock(). Remove the early exit to ensure we always run
sdhci_enable_clk().

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/mmc/host/sdhci-of-aspeed.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
index 213b3dbd49ef..c31d74427c49 100644
--- a/drivers/mmc/host/sdhci-of-aspeed.c
+++ b/drivers/mmc/host/sdhci-of-aspeed.c
@@ -55,9 +55,6 @@ static void aspeed_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 	int div;
 	u16 clk;
 
-	if (clock == host->clock)
-		return;
-
 	sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
 
 	if (clock == 0)
-- 
2.20.1

[PATCH v2 4/4] mmc: sdhci-of-aspeed: Allow max-frequency limitation of SDCLK

[Andrew Jeffery]

Add a get_max_clock() handler to sdhci-of-aspeed to report f_max as the
maximum clock rate if it is set. This enables artificial limitation of
the bus speed via max-frequency in the devicetree for e.g. the AST2600
evaluation board where I was seeing errors at 200MHz.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/mmc/host/sdhci-of-aspeed.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
index c31d74427c49..a8a5341b526c 100644
--- a/drivers/mmc/host/sdhci-of-aspeed.c
+++ b/drivers/mmc/host/sdhci-of-aspeed.c
@@ -52,16 +52,24 @@ static void aspeed_sdc_configure_8bit_mode(struct aspeed_sdc *sdc,
 
 static void aspeed_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 {
+	struct sdhci_pltfm_host *pltfm_host;
+	unsigned long parent;
 	int div;
 	u16 clk;
 
+	pltfm_host = sdhci_priv(host);
+	parent = clk_get_rate(pltfm_host->clk);
+
 	sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
 
 	if (clock == 0)
 		return;
 
+	if (WARN_ON(clock > host->max_clk))
+		clock = host->max_clk;
+
 	for (div = 1; div < 256; div *= 2) {
-		if ((host->max_clk / div) <= clock)
+		if ((parent / div) <= clock)
 			break;
 	}
 	div >>= 1;
@@ -71,6 +79,14 @@ static void aspeed_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 	sdhci_enable_clk(host, clk);
 }
 
+static unsigned int aspeed_sdhci_get_max_clock(struct sdhci_host *host)
+{
+	if (host->mmc->f_max)
+		return host->mmc->f_max;
+
+	return sdhci_pltfm_clk_get_max_clock(host);
+}
+
 static void aspeed_sdhci_set_bus_width(struct sdhci_host *host, int width)
 {
 	struct sdhci_pltfm_host *pltfm_priv;
@@ -97,7 +113,7 @@ static void aspeed_sdhci_set_bus_width(struct sdhci_host *host, int width)
 
 static const struct sdhci_ops aspeed_sdhci_ops = {
 	.set_clock = aspeed_sdhci_set_clock,
-	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
+	.get_max_clock = aspeed_sdhci_get_max_clock,
 	.set_bus_width = aspeed_sdhci_set_bus_width,
 	.get_timeout_clock = sdhci_pltfm_clk_get_max_clock,
 	.reset = sdhci_reset,
-- 
2.20.1

Re: [PATCH v2 1/4] mmc: sdhci-of-aspeed: Fix link failure for SPARC

[Joel Stanley]

On Mon, 2 Sep 2019 at 03:58, Andrew Jeffery <andrew@aj.id.au> wrote:
>
> Resolves the following build error reported by the 0-day bot:
>
>     ERROR: "of_platform_device_create" [drivers/mmc/host/sdhci-of-aspeed.ko] undefined!
>
> SPARC does not set CONFIG_OF_ADDRESS so the symbol is missing. Guard the
> callsite to maintain build coverage for the rest of the driver.
>
> Reported-by: kbuild test robot <lkp@intel.com>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  drivers/mmc/host/sdhci-of-aspeed.c | 38 ++++++++++++++++++++----------
>  1 file changed, 25 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
> index d5acb5afc50f..96ca494752c5 100644
> --- a/drivers/mmc/host/sdhci-of-aspeed.c
> +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> @@ -224,10 +224,30 @@ static struct platform_driver aspeed_sdhci_driver = {
>         .remove         = aspeed_sdhci_remove,
>  };
>
> -static int aspeed_sdc_probe(struct platform_device *pdev)
> -
> +static int aspeed_sdc_create_sdhcis(struct platform_device *pdev)
>  {
> +#if defined(CONFIG_OF_ADDRESS)

This is going to be untested code forever, as no one will be running
on a chip with this hardware present but OF_ADDRESS disabled.

How about we make the driver depend on OF_ADDRESS instead?

Cheers,

Joel



>         struct device_node *parent, *child;
> +
> +       parent = pdev->dev.of_node;
> +
> +       for_each_available_child_of_node(parent, child) {
> +               struct platform_device *cpdev;
> +
> +               cpdev = of_platform_device_create(child, NULL, &pdev->dev);
> +               if (!cpdev) {
> +                       of_node_put(child);
> +                       return -ENODEV;
> +               }
> +       }
> +#endif
> +
> +       return 0;
> +}
> +
> +static int aspeed_sdc_probe(struct platform_device *pdev)
> +
> +{
>         struct aspeed_sdc *sdc;
>         int ret;
>
> @@ -256,17 +276,9 @@ static int aspeed_sdc_probe(struct platform_device *pdev)
>
>         dev_set_drvdata(&pdev->dev, sdc);
>
> -       parent = pdev->dev.of_node;
> -       for_each_available_child_of_node(parent, child) {
> -               struct platform_device *cpdev;
> -
> -               cpdev = of_platform_device_create(child, NULL, &pdev->dev);
> -               if (!cpdev) {
> -                       of_node_put(child);
> -                       ret = -ENODEV;
> -                       goto err_clk;
> -               }
> -       }
> +       ret = aspeed_sdc_create_sdhcis(pdev);
> +       if (ret)
> +               goto err_clk;
>
>         return 0;
>
> --
> 2.20.1
>

Re: [PATCH v2 1/4] mmc: sdhci-of-aspeed: Fix link failure for SPARC

[Andrew Jeffery]

On Mon, 2 Sep 2019, at 13:42, Joel Stanley wrote:
> On Mon, 2 Sep 2019 at 03:58, Andrew Jeffery <andrew@aj.id.au> wrote:
> >
> > Resolves the following build error reported by the 0-day bot:
> >
> >     ERROR: "of_platform_device_create" [drivers/mmc/host/sdhci-of-aspeed.ko] undefined!
> >
> > SPARC does not set CONFIG_OF_ADDRESS so the symbol is missing. Guard the
> > callsite to maintain build coverage for the rest of the driver.
> >
> > Reported-by: kbuild test robot <lkp@intel.com>
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> >  drivers/mmc/host/sdhci-of-aspeed.c | 38 ++++++++++++++++++++----------
> >  1 file changed, 25 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
> > index d5acb5afc50f..96ca494752c5 100644
> > --- a/drivers/mmc/host/sdhci-of-aspeed.c
> > +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> > @@ -224,10 +224,30 @@ static struct platform_driver aspeed_sdhci_driver = {
> >         .remove         = aspeed_sdhci_remove,
> >  };
> >
> > -static int aspeed_sdc_probe(struct platform_device *pdev)
> > -
> > +static int aspeed_sdc_create_sdhcis(struct platform_device *pdev)
> >  {
> > +#if defined(CONFIG_OF_ADDRESS)
> 
> This is going to be untested code forever, as no one will be running
> on a chip with this hardware present but OF_ADDRESS disabled.
> 
> How about we make the driver depend on OF_ADDRESS instead?

Testing is split into two pieces here: compile-time and run-time.
Clearly the run-time behaviour is going to be broken on configurations
without CONFIG_OF_ADDRESS (SPARC as mentioned), but I don't think
that means we shouldn't allow it to be compiled in that case
(e.g. CONFIG_COMPILE_TEST performs a similar role).

With respect to compile-time it's possible to compile either path as
demonstrated by the build failure report.

Having said that there's no reason we  couldn't do what you suggest,
just it wasn't the existing solution pattern for the problem (there are
several other drivers that suffered the same bug that were fixed in the
style of this patch). Either way works, it's all somewhat academic.
Your suggestion is more obvious in terms of correctness, but this
patch is basically just code motion (the only addition is the `#if`/
`#endif` lines over what was already there if we disregard the
function declaration/invocation). I'll change it if there are further
complaints and a reason to do a v3.

Andrew

Re: [PATCH v2 1/4] mmc: sdhci-of-aspeed: Fix link failure for SPARC

[Ulf Hansson]

On Mon, 2 Sep 2019 at 07:26, Andrew Jeffery <andrew@aj.id.au> wrote:
>
>
>
> On Mon, 2 Sep 2019, at 13:42, Joel Stanley wrote:
> > On Mon, 2 Sep 2019 at 03:58, Andrew Jeffery <andrew@aj.id.au> wrote:
> > >
> > > Resolves the following build error reported by the 0-day bot:
> > >
> > >     ERROR: "of_platform_device_create" [drivers/mmc/host/sdhci-of-aspeed.ko] undefined!
> > >
> > > SPARC does not set CONFIG_OF_ADDRESS so the symbol is missing. Guard the
> > > callsite to maintain build coverage for the rest of the driver.
> > >
> > > Reported-by: kbuild test robot <lkp@intel.com>
> > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > > ---
> > >  drivers/mmc/host/sdhci-of-aspeed.c | 38 ++++++++++++++++++++----------
> > >  1 file changed, 25 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
> > > index d5acb5afc50f..96ca494752c5 100644
> > > --- a/drivers/mmc/host/sdhci-of-aspeed.c
> > > +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> > > @@ -224,10 +224,30 @@ static struct platform_driver aspeed_sdhci_driver = {
> > >         .remove         = aspeed_sdhci_remove,
> > >  };
> > >
> > > -static int aspeed_sdc_probe(struct platform_device *pdev)
> > > -
> > > +static int aspeed_sdc_create_sdhcis(struct platform_device *pdev)
> > >  {
> > > +#if defined(CONFIG_OF_ADDRESS)
> >
> > This is going to be untested code forever, as no one will be running
> > on a chip with this hardware present but OF_ADDRESS disabled.
> >
> > How about we make the driver depend on OF_ADDRESS instead?
>
> Testing is split into two pieces here: compile-time and run-time.
> Clearly the run-time behaviour is going to be broken on configurations
> without CONFIG_OF_ADDRESS (SPARC as mentioned), but I don't think
> that means we shouldn't allow it to be compiled in that case
> (e.g. CONFIG_COMPILE_TEST performs a similar role).
>
> With respect to compile-time it's possible to compile either path as
> demonstrated by the build failure report.
>
> Having said that there's no reason we  couldn't do what you suggest,
> just it wasn't the existing solution pattern for the problem (there are
> several other drivers that suffered the same bug that were fixed in the
> style of this patch). Either way works, it's all somewhat academic.
> Your suggestion is more obvious in terms of correctness, but this
> patch is basically just code motion (the only addition is the `#if`/
> `#endif` lines over what was already there if we disregard the
> function declaration/invocation). I'll change it if there are further
> complaints and a reason to do a v3.

I am in favor of Joel's suggestion as I don't really like having
ifdefs bloating around in the driver (unless very good reasons).
Please re-spin a v3.

Another option is to implement stub function and to deal with error
codes, but that sounds more like a long term thingy, if even
applicable here.

Kind regards
Uffe

Re: [PATCH v2 0/4] mmc: sdhci-of-aspeed: Fixes for AST2600 eMMC support

[Ulf Hansson]

On Mon, 2 Sep 2019 at 05:58, Andrew Jeffery <andrew@aj.id.au> wrote:
>
> Hello,
>
> I've added a couple of patches since v1 of this series. The horizon has
> broadened slightly with a fix for SPARC builds as well in patch 1/4. Ulf
> suggested a minor cleanup on v1 with respect to handling of the current clock
> value, so that's now patch 2/4. Patches 3/4 and 4/4 are as they were in v1.

Applied patch 2->4 for next, thanks!

Kind regards
Uffe


>
> The v1 series can be found here:
>
> https://patchwork.ozlabs.org/cover/1155757/
>
> Please review!
>
> Andrew
>
> Andrew Jeffery (4):
>   mmc: sdhci-of-aspeed: Fix link failure for SPARC
>   mmc: sdhci-of-aspeed: Drop redundant assignment to host->clock
>   mmc: sdhci-of-aspeed: Uphold clocks-on post-condition of set_clock()
>   mmc: sdhci-of-aspeed: Allow max-frequency limitation of SDCLK
>
>  drivers/mmc/host/sdhci-of-aspeed.c | 62 ++++++++++++++++++++----------
>  1 file changed, 42 insertions(+), 20 deletions(-)
>
> --
> 2.20.1
>

Re: [PATCH v2 1/4] mmc: sdhci-of-aspeed: Fix link failure for SPARC

[Andrew Jeffery]

On Wed, 4 Sep 2019, at 00:18, Ulf Hansson wrote:
> On Mon, 2 Sep 2019 at 07:26, Andrew Jeffery <andrew@aj.id.au> wrote:
> >
> >
> >
> > On Mon, 2 Sep 2019, at 13:42, Joel Stanley wrote:
> > > On Mon, 2 Sep 2019 at 03:58, Andrew Jeffery <andrew@aj.id.au> wrote:
> > > >
> > > > Resolves the following build error reported by the 0-day bot:
> > > >
> > > >     ERROR: "of_platform_device_create" [drivers/mmc/host/sdhci-of-aspeed.ko] undefined!
> > > >
> > > > SPARC does not set CONFIG_OF_ADDRESS so the symbol is missing. Guard the
> > > > callsite to maintain build coverage for the rest of the driver.
> > > >
> > > > Reported-by: kbuild test robot <lkp@intel.com>
> > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > > > ---
> > > >  drivers/mmc/host/sdhci-of-aspeed.c | 38 ++++++++++++++++++++----------
> > > >  1 file changed, 25 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
> > > > index d5acb5afc50f..96ca494752c5 100644
> > > > --- a/drivers/mmc/host/sdhci-of-aspeed.c
> > > > +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> > > > @@ -224,10 +224,30 @@ static struct platform_driver aspeed_sdhci_driver = {
> > > >         .remove         = aspeed_sdhci_remove,
> > > >  };
> > > >
> > > > -static int aspeed_sdc_probe(struct platform_device *pdev)
> > > > -
> > > > +static int aspeed_sdc_create_sdhcis(struct platform_device *pdev)
> > > >  {
> > > > +#if defined(CONFIG_OF_ADDRESS)
> > >
> > > This is going to be untested code forever, as no one will be running
> > > on a chip with this hardware present but OF_ADDRESS disabled.
> > >
> > > How about we make the driver depend on OF_ADDRESS instead?
> >
> > Testing is split into two pieces here: compile-time and run-time.
> > Clearly the run-time behaviour is going to be broken on configurations
> > without CONFIG_OF_ADDRESS (SPARC as mentioned), but I don't think
> > that means we shouldn't allow it to be compiled in that case
> > (e.g. CONFIG_COMPILE_TEST performs a similar role).
> >
> > With respect to compile-time it's possible to compile either path as
> > demonstrated by the build failure report.
> >
> > Having said that there's no reason we  couldn't do what you suggest,
> > just it wasn't the existing solution pattern for the problem (there are
> > several other drivers that suffered the same bug that were fixed in the
> > style of this patch). Either way works, it's all somewhat academic.
> > Your suggestion is more obvious in terms of correctness, but this
> > patch is basically just code motion (the only addition is the `#if`/
> > `#endif` lines over what was already there if we disregard the
> > function declaration/invocation). I'll change it if there are further
> > complaints and a reason to do a v3.
> 
> I am in favor of Joel's suggestion as I don't really like having
> ifdefs bloating around in the driver (unless very good reasons).
> Please re-spin a v3.
> 
> Another option is to implement stub function and to deal with error
> codes, but that sounds more like a long term thingy, if even
> applicable here.

No worries then, will post a respin shortly.

Andrew