[PATCH 1/3] mmc: sunxi: Disable HS-DDR mode for H5 eMMC controller by default

[PATCH 1/3] mmc: sunxi: Disable HS-DDR mode for H5 eMMC controller by default

[Chen-Yu Tsai]

Some H5 boards seem to not have proper trace lengths for eMMC to be able
to use the default setting for the delay chains under HS-DDR mode. These
include the Bananapi M2+ H5 and NanoPi NEO Core2. However the Libre
Computer ALL-H3-CC-H5 works just fine.

For the H5 (at least for now), default to not enabling HS-DDR modes in
the driver, and expect the device tree to signal HS-DDR capability on
boards that work.

Reported-by: Chris Blake <chrisrblake93@gmail.com>
Fixes: 07bafc1e3536 ("mmc: sunxi: Use new timing mode for A64 eMMC controller")
Cc: <stable@vger.kernel.org>
Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/mmc/host/sunxi-mmc.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index 279e326e397e..7415af8c8ff6 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -1399,7 +1399,16 @@ static int sunxi_mmc_probe(struct platform_device *pdev)
 	mmc->caps	       |= MMC_CAP_MMC_HIGHSPEED | MMC_CAP_SD_HIGHSPEED |
 				  MMC_CAP_ERASE | MMC_CAP_SDIO_IRQ;
 
-	if (host->cfg->clk_delays || host->use_new_timings)
+	/*
+	 * Some H5 devices do not have signal traces precise enough to
+	 * use HS DDR mode for their eMMC chips.
+	 *
+	 * We still enable HS DDR modes for all the other controller
+	 * variants that support them.
+	 */
+	if ((host->cfg->clk_delays || host->use_new_timings) &&
+	    !of_device_is_compatible(pdev->dev.of_node,
+				     "allwinner,sun50i-h5-emmc"))
 		mmc->caps      |= MMC_CAP_1_8V_DDR | MMC_CAP_3_3V_DDR;
 
 	ret = mmc_of_parse(mmc);
-- 
2.20.1

Re: [PATCH 1/3] mmc: sunxi: Disable HS-DDR mode for H5 eMMC controller by default

[Maxime Ripard]

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

On Sun, Feb 03, 2019 at 11:56:26PM +0800, Chen-Yu Tsai wrote:
> Some H5 boards seem to not have proper trace lengths for eMMC to be able
> to use the default setting for the delay chains under HS-DDR mode. These
> include the Bananapi M2+ H5 and NanoPi NEO Core2. However the Libre
> Computer ALL-H3-CC-H5 works just fine.
> 
> For the H5 (at least for now), default to not enabling HS-DDR modes in
> the driver, and expect the device tree to signal HS-DDR capability on
> boards that work.
> 
> Reported-by: Chris Blake <chrisrblake93@gmail.com>
> Fixes: 07bafc1e3536 ("mmc: sunxi: Use new timing mode for A64 eMMC controller")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

Acked-by: Maxime Ripard <maxime.ripard@bootlin.com>

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH 2/3] mmc: sunxi: Filter out unsupported modes declared in the device tree

[Chen-Yu Tsai]

The MMC device tree bindings include properties used to signal various
signalling speed modes. Until now the sunxi driver was accepting them
without any further filtering, while the sunxi device trees were not
actually using them.

Since some of the H5 boards can not run at higher speed modes stably,
we are resorting to declaring the higher speed modes per-board.

Regardless, having boards declare modes and blindly following them,
even without proper support in the driver, is generally a bad thing.

Filter out all unsupported modes from the capabilities mask after
the device tree properties have been parsed.

Cc: <stable@vger.kernel.org>
Signed-off-by: Chen-Yu Tsai <wens@csie.org>

---

This should be backported to stable kernels in case people try to run
new device trees (that declare newly supported modes) with old kernels.
---
 drivers/mmc/host/sunxi-mmc.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index 7415af8c8ff6..a01433012db0 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -1415,6 +1415,22 @@ static int sunxi_mmc_probe(struct platform_device *pdev)
 	if (ret)
 		goto error_free_dma;
 
+	/*
+	 * If we don't support delay chains in the SoC, we can't use any
+	 * of the DDR speed modes. Mask them out in case the device
+	 * tree specifies the properties for them, which gets added to
+	 * the caps by mmc_of_parse() above.
+	 */
+	if (!(host->cfg->clk_delays || host->use_new_timings))
+		mmc->caps &= ~(MMC_CAP_3_3V_DDR | MMC_CAP_1_8V_DDR |
+			       MMC_CAP_1_2V_DDR);
+
+	/* TODO: UHS modes untested due to lack of supporting boards */
+	mmc->caps &= ~MMC_CAP_UHS;
+
+	/* TODO: This driver doesn't support HS200 and HS400 modes yet */
+	mmc->caps2 &= ~(MMC_CAP2_HS200 | MMC_CAP2_HS400);
+
 	ret = sunxi_mmc_init_host(host);
 	if (ret)
 		goto error_free_dma;
-- 
2.20.1

Re: [PATCH 2/3] mmc: sunxi: Filter out unsupported modes declared in the device tree

[Maxime Ripard]

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

On Sun, Feb 03, 2019 at 11:56:27PM +0800, Chen-Yu Tsai wrote:
> The MMC device tree bindings include properties used to signal various
> signalling speed modes. Until now the sunxi driver was accepting them
> without any further filtering, while the sunxi device trees were not
> actually using them.
> 
> Since some of the H5 boards can not run at higher speed modes stably,
> we are resorting to declaring the higher speed modes per-board.
> 
> Regardless, having boards declare modes and blindly following them,
> even without proper support in the driver, is generally a bad thing.
> 
> Filter out all unsupported modes from the capabilities mask after
> the device tree properties have been parsed.
> 
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> 
> ---
> 
> This should be backported to stable kernels in case people try to run
> new device trees (that declare newly supported modes) with old kernels.
> ---
>  drivers/mmc/host/sunxi-mmc.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> index 7415af8c8ff6..a01433012db0 100644
> --- a/drivers/mmc/host/sunxi-mmc.c
> +++ b/drivers/mmc/host/sunxi-mmc.c
> @@ -1415,6 +1415,22 @@ static int sunxi_mmc_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto error_free_dma;
>  
> +	/*
> +	 * If we don't support delay chains in the SoC, we can't use any
> +	 * of the DDR speed modes. Mask them out in case the device
> +	 * tree specifies the properties for them, which gets added to
> +	 * the caps by mmc_of_parse() above.
> +	 */
> +	if (!(host->cfg->clk_delays || host->use_new_timings))
> +		mmc->caps &= ~(MMC_CAP_3_3V_DDR | MMC_CAP_1_8V_DDR |
> +			       MMC_CAP_1_2V_DDR);
> +
> +	/* TODO: UHS modes untested due to lack of supporting boards */
> +	mmc->caps &= ~MMC_CAP_UHS;

I've tested up to SDR104 and it works on the A64 at least

> +	/* TODO: This driver doesn't support HS200 and HS400 modes yet */
> +	mmc->caps2 &= ~(MMC_CAP2_HS200 | MMC_CAP2_HS400);

And HS200 works too.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/3] mmc: sunxi: Filter out unsupported modes declared in the device tree

[Chen-Yu Tsai]

On Mon, Feb 4, 2019 at 5:34 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Sun, Feb 03, 2019 at 11:56:27PM +0800, Chen-Yu Tsai wrote:
> > The MMC device tree bindings include properties used to signal various
> > signalling speed modes. Until now the sunxi driver was accepting them
> > without any further filtering, while the sunxi device trees were not
> > actually using them.
> >
> > Since some of the H5 boards can not run at higher speed modes stably,
> > we are resorting to declaring the higher speed modes per-board.
> >
> > Regardless, having boards declare modes and blindly following them,
> > even without proper support in the driver, is generally a bad thing.
> >
> > Filter out all unsupported modes from the capabilities mask after
> > the device tree properties have been parsed.
> >
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> >
> > ---
> >
> > This should be backported to stable kernels in case people try to run
> > new device trees (that declare newly supported modes) with old kernels.
> > ---
> >  drivers/mmc/host/sunxi-mmc.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> > index 7415af8c8ff6..a01433012db0 100644
> > --- a/drivers/mmc/host/sunxi-mmc.c
> > +++ b/drivers/mmc/host/sunxi-mmc.c
> > @@ -1415,6 +1415,22 @@ static int sunxi_mmc_probe(struct platform_device *pdev)
> >       if (ret)
> >               goto error_free_dma;
> >
> > +     /*
> > +      * If we don't support delay chains in the SoC, we can't use any
> > +      * of the DDR speed modes. Mask them out in case the device
> > +      * tree specifies the properties for them, which gets added to
> > +      * the caps by mmc_of_parse() above.
> > +      */
> > +     if (!(host->cfg->clk_delays || host->use_new_timings))
> > +             mmc->caps &= ~(MMC_CAP_3_3V_DDR | MMC_CAP_1_8V_DDR |
> > +                            MMC_CAP_1_2V_DDR);
> > +
> > +     /* TODO: UHS modes untested due to lack of supporting boards */
> > +     mmc->caps &= ~MMC_CAP_UHS;
>
> I've tested up to SDR104 and it works on the A64 at least

That's good to know. What board was this on? I had given up hope waiting
for a vendor to produce a board that could do proper voltage switching for
SD cards.

> > +     /* TODO: This driver doesn't support HS200 and HS400 modes yet */
> > +     mmc->caps2 &= ~(MMC_CAP2_HS200 | MMC_CAP2_HS400);
>
> And HS200 works too.

OK. I thought there was some special magic required in the driver. Maybe
that was for HS400 only? Again, what board was this on?

Thanks
ChenYu

Re: [PATCH 2/3] mmc: sunxi: Filter out unsupported modes declared in the device tree

[Maxime Ripard]

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

On Mon, Feb 04, 2019 at 06:16:24PM +0800, Chen-Yu Tsai wrote:
> On Mon, Feb 4, 2019 at 5:34 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> >
> > On Sun, Feb 03, 2019 at 11:56:27PM +0800, Chen-Yu Tsai wrote:
> > > The MMC device tree bindings include properties used to signal various
> > > signalling speed modes. Until now the sunxi driver was accepting them
> > > without any further filtering, while the sunxi device trees were not
> > > actually using them.
> > >
> > > Since some of the H5 boards can not run at higher speed modes stably,
> > > we are resorting to declaring the higher speed modes per-board.
> > >
> > > Regardless, having boards declare modes and blindly following them,
> > > even without proper support in the driver, is generally a bad thing.
> > >
> > > Filter out all unsupported modes from the capabilities mask after
> > > the device tree properties have been parsed.
> > >
> > > Cc: <stable@vger.kernel.org>
> > > Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> > >
> > > ---
> > >
> > > This should be backported to stable kernels in case people try to run
> > > new device trees (that declare newly supported modes) with old kernels.
> > > ---
> > >  drivers/mmc/host/sunxi-mmc.c | 16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > >
> > > diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> > > index 7415af8c8ff6..a01433012db0 100644
> > > --- a/drivers/mmc/host/sunxi-mmc.c
> > > +++ b/drivers/mmc/host/sunxi-mmc.c
> > > @@ -1415,6 +1415,22 @@ static int sunxi_mmc_probe(struct platform_device *pdev)
> > >       if (ret)
> > >               goto error_free_dma;
> > >
> > > +     /*
> > > +      * If we don't support delay chains in the SoC, we can't use any
> > > +      * of the DDR speed modes. Mask them out in case the device
> > > +      * tree specifies the properties for them, which gets added to
> > > +      * the caps by mmc_of_parse() above.
> > > +      */
> > > +     if (!(host->cfg->clk_delays || host->use_new_timings))
> > > +             mmc->caps &= ~(MMC_CAP_3_3V_DDR | MMC_CAP_1_8V_DDR |
> > > +                            MMC_CAP_1_2V_DDR);
> > > +
> > > +     /* TODO: UHS modes untested due to lack of supporting boards */
> > > +     mmc->caps &= ~MMC_CAP_UHS;
> >
> > I've tested up to SDR104 and it works on the A64 at least
> 
> That's good to know. What board was this on? I had given up hope waiting
> for a vendor to produce a board that could do proper voltage switching for
> SD cards.

On a Sootech SoM, that had an HS400 eMMC and an SDR104 Marvell WiFi
chip. I don't have that board anymore, and the website seems down now
though :/

> > > +     /* TODO: This driver doesn't support HS200 and HS400 modes yet */
> > > +     mmc->caps2 &= ~(MMC_CAP2_HS200 | MMC_CAP2_HS400);
> >
> > And HS200 works too.
> 
> OK. I thought there was some special magic required in the driver. Maybe
> that was for HS400 only? Again, what board was this on?

Yeah, that was for HS400 only

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/3] mmc: sunxi: Filter out unsupported modes declared in the device tree

[Chen-Yu Tsai]

On Mon, Feb 4, 2019 at 9:41 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Mon, Feb 04, 2019 at 06:16:24PM +0800, Chen-Yu Tsai wrote:
> > On Mon, Feb 4, 2019 at 5:34 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > >
> > > On Sun, Feb 03, 2019 at 11:56:27PM +0800, Chen-Yu Tsai wrote:
> > > > The MMC device tree bindings include properties used to signal various
> > > > signalling speed modes. Until now the sunxi driver was accepting them
> > > > without any further filtering, while the sunxi device trees were not
> > > > actually using them.
> > > >
> > > > Since some of the H5 boards can not run at higher speed modes stably,
> > > > we are resorting to declaring the higher speed modes per-board.
> > > >
> > > > Regardless, having boards declare modes and blindly following them,
> > > > even without proper support in the driver, is generally a bad thing.
> > > >
> > > > Filter out all unsupported modes from the capabilities mask after
> > > > the device tree properties have been parsed.
> > > >
> > > > Cc: <stable@vger.kernel.org>
> > > > Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> > > >
> > > > ---
> > > >
> > > > This should be backported to stable kernels in case people try to run
> > > > new device trees (that declare newly supported modes) with old kernels.
> > > > ---
> > > >  drivers/mmc/host/sunxi-mmc.c | 16 ++++++++++++++++
> > > >  1 file changed, 16 insertions(+)
> > > >
> > > > diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> > > > index 7415af8c8ff6..a01433012db0 100644
> > > > --- a/drivers/mmc/host/sunxi-mmc.c
> > > > +++ b/drivers/mmc/host/sunxi-mmc.c
> > > > @@ -1415,6 +1415,22 @@ static int sunxi_mmc_probe(struct platform_device *pdev)
> > > >       if (ret)
> > > >               goto error_free_dma;
> > > >
> > > > +     /*
> > > > +      * If we don't support delay chains in the SoC, we can't use any
> > > > +      * of the DDR speed modes. Mask them out in case the device
> > > > +      * tree specifies the properties for them, which gets added to
> > > > +      * the caps by mmc_of_parse() above.
> > > > +      */
> > > > +     if (!(host->cfg->clk_delays || host->use_new_timings))
> > > > +             mmc->caps &= ~(MMC_CAP_3_3V_DDR | MMC_CAP_1_8V_DDR |
> > > > +                            MMC_CAP_1_2V_DDR);
> > > > +
> > > > +     /* TODO: UHS modes untested due to lack of supporting boards */
> > > > +     mmc->caps &= ~MMC_CAP_UHS;
> > >
> > > I've tested up to SDR104 and it works on the A64 at least
> >
> > That's good to know. What board was this on? I had given up hope waiting
> > for a vendor to produce a board that could do proper voltage switching for
> > SD cards.
>
> On a Sootech SoM, that had an HS400 eMMC and an SDR104 Marvell WiFi
> chip. I don't have that board anymore, and the website seems down now
> though :/

Bummer. So no commercially available board still. :/

> > > > +     /* TODO: This driver doesn't support HS200 and HS400 modes yet */
> > > > +     mmc->caps2 &= ~(MMC_CAP2_HS200 | MMC_CAP2_HS400);
> > >
> > > And HS200 works too.
> >
> > OK. I thought there was some special magic required in the driver. Maybe
> > that was for HS400 only? Again, what board was this on?
>
> Yeah, that was for HS400 only

OK. So would unblocking UHS and HS200, but not enabling them by default,
which is essentially the original behavior, work for you?

If so, I'll respin a version tonight.

ChenYu

Re: [PATCH 2/3] mmc: sunxi: Filter out unsupported modes declared in the device tree

[Maxime Ripard]

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

On Tue, Feb 05, 2019 at 04:42:53PM +0800, Chen-Yu Tsai wrote:
> On Mon, Feb 4, 2019 at 9:41 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> >
> > On Mon, Feb 04, 2019 at 06:16:24PM +0800, Chen-Yu Tsai wrote:
> > > On Mon, Feb 4, 2019 at 5:34 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > >
> > > > On Sun, Feb 03, 2019 at 11:56:27PM +0800, Chen-Yu Tsai wrote:
> > > > > The MMC device tree bindings include properties used to signal various
> > > > > signalling speed modes. Until now the sunxi driver was accepting them
> > > > > without any further filtering, while the sunxi device trees were not
> > > > > actually using them.
> > > > >
> > > > > Since some of the H5 boards can not run at higher speed modes stably,
> > > > > we are resorting to declaring the higher speed modes per-board.
> > > > >
> > > > > Regardless, having boards declare modes and blindly following them,
> > > > > even without proper support in the driver, is generally a bad thing.
> > > > >
> > > > > Filter out all unsupported modes from the capabilities mask after
> > > > > the device tree properties have been parsed.
> > > > >
> > > > > Cc: <stable@vger.kernel.org>
> > > > > Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> > > > >
> > > > > ---
> > > > >
> > > > > This should be backported to stable kernels in case people try to run
> > > > > new device trees (that declare newly supported modes) with old kernels.
> > > > > ---
> > > > >  drivers/mmc/host/sunxi-mmc.c | 16 ++++++++++++++++
> > > > >  1 file changed, 16 insertions(+)
> > > > >
> > > > > diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> > > > > index 7415af8c8ff6..a01433012db0 100644
> > > > > --- a/drivers/mmc/host/sunxi-mmc.c
> > > > > +++ b/drivers/mmc/host/sunxi-mmc.c
> > > > > @@ -1415,6 +1415,22 @@ static int sunxi_mmc_probe(struct platform_device *pdev)
> > > > >       if (ret)
> > > > >               goto error_free_dma;
> > > > >
> > > > > +     /*
> > > > > +      * If we don't support delay chains in the SoC, we can't use any
> > > > > +      * of the DDR speed modes. Mask them out in case the device
> > > > > +      * tree specifies the properties for them, which gets added to
> > > > > +      * the caps by mmc_of_parse() above.
> > > > > +      */
> > > > > +     if (!(host->cfg->clk_delays || host->use_new_timings))
> > > > > +             mmc->caps &= ~(MMC_CAP_3_3V_DDR | MMC_CAP_1_8V_DDR |
> > > > > +                            MMC_CAP_1_2V_DDR);
> > > > > +
> > > > > +     /* TODO: UHS modes untested due to lack of supporting boards */
> > > > > +     mmc->caps &= ~MMC_CAP_UHS;
> > > >
> > > > I've tested up to SDR104 and it works on the A64 at least
> > >
> > > That's good to know. What board was this on? I had given up hope waiting
> > > for a vendor to produce a board that could do proper voltage switching for
> > > SD cards.
> >
> > On a Sootech SoM, that had an HS400 eMMC and an SDR104 Marvell WiFi
> > chip. I don't have that board anymore, and the website seems down now
> > though :/
> 
> Bummer. So no commercially available board still. :/
> 
> > > > > +     /* TODO: This driver doesn't support HS200 and HS400 modes yet */
> > > > > +     mmc->caps2 &= ~(MMC_CAP2_HS200 | MMC_CAP2_HS400);
> > > >
> > > > And HS200 works too.
> > >
> > > OK. I thought there was some special magic required in the driver. Maybe
> > > that was for HS400 only? Again, what board was this on?
> >
> > Yeah, that was for HS400 only
> 
> OK. So would unblocking UHS and HS200, but not enabling them by default,
> which is essentially the original behavior, work for you?

Yep, that's perfect

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]