mmc: sunxi: fix unusuable eMMC on some H6 boards by disabling DDR
diff mbox series

Message ID 20190825150558.15173-1-alejandro.gonzalez.correo@gmail.com
State New
Headers show
Series
  • mmc: sunxi: fix unusuable eMMC on some H6 boards by disabling DDR
Related show

Commit Message

Alejandro González Aug. 25, 2019, 3:05 p.m. UTC
Some Allwinner H6 boards have timing problems when dealing with
DDR-capable eMMC cards. These boards include the Pine H64 and Tanix TX6.

These timing problems result in out of sync communication between the
driver and the eMMC, which renders the memory unsuable for every
operation but some basic commmands, like reading the status register.

The cause of these timing problems is not yet well known, but they go
away by disabling DDR mode operation in the driver. Like on some H5
boards, it might be that the traces are not precise enough to support
these speeds. However, Jernej Skrabec compared the BSP driver with this
driver, and found that the BSP driver configures pinctrl to operate at
1.8 V when entering DDR mode (although 3.3 V operation is supported), while
the mainline kernel lacks any mechanism to switch voltages dynamically.
Finally, other possible cause might be some timing parameter that is
different on the H6 with respect to other SoCs.

Therefore, as this fix works reliably, the kernel lacks the required
dynamic pinctrl control for now and a slow eMMC is better than a not
working eMMC, just disable DDR operation for now on H6-compatible
devices.

Signed-off-by: Alejandro González <alejandro.gonzalez.correo@gmail.com>
---
 drivers/mmc/host/sunxi-mmc.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Ulf Hansson Aug. 27, 2019, 1:24 p.m. UTC | #1
On Sun, 25 Aug 2019 at 17:06, Alejandro González
<alejandro.gonzalez.correo@gmail.com> wrote:
>
> Some Allwinner H6 boards have timing problems when dealing with
> DDR-capable eMMC cards. These boards include the Pine H64 and Tanix TX6.
>
> These timing problems result in out of sync communication between the
> driver and the eMMC, which renders the memory unsuable for every
> operation but some basic commmands, like reading the status register.
>
> The cause of these timing problems is not yet well known, but they go
> away by disabling DDR mode operation in the driver. Like on some H5
> boards, it might be that the traces are not precise enough to support
> these speeds. However, Jernej Skrabec compared the BSP driver with this
> driver, and found that the BSP driver configures pinctrl to operate at
> 1.8 V when entering DDR mode (although 3.3 V operation is supported), while
> the mainline kernel lacks any mechanism to switch voltages dynamically.
> Finally, other possible cause might be some timing parameter that is
> different on the H6 with respect to other SoCs.
>
> Therefore, as this fix works reliably, the kernel lacks the required
> dynamic pinctrl control for now and a slow eMMC is better than a not
> working eMMC, just disable DDR operation for now on H6-compatible
> devices.
>
> Signed-off-by: Alejandro González <alejandro.gonzalez.correo@gmail.com>

Assuming this should go stable as well? Perhaps you can find a
relevant commit that we can put as a fixes tag as well?

Kind regards
Uffe

> ---
>  drivers/mmc/host/sunxi-mmc.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> index d577a6b0ceae..dac57d76d009 100644
> --- a/drivers/mmc/host/sunxi-mmc.c
> +++ b/drivers/mmc/host/sunxi-mmc.c
> @@ -1395,14 +1395,17 @@ static int sunxi_mmc_probe(struct platform_device *pdev)
>
>         /*
>          * Some H5 devices do not have signal traces precise enough to
> -        * use HS DDR mode for their eMMC chips.
> +        * use HS DDR mode for their eMMC chips. Other H6 devices operate
> +        * unreliably on HS DDR mode, too.
>          *
>          * We still enable HS DDR modes for all the other controller
> -        * variants that support them.
> +        * variants that support them properly.
>          */
>         if ((host->cfg->clk_delays || host->use_new_timings) &&
>             !of_device_is_compatible(pdev->dev.of_node,
> -                                    "allwinner,sun50i-h5-emmc"))
> +                                    "allwinner,sun50i-h5-emmc") &&
> +           !of_device_is_compatible(pdev->dev.of_node,
> +                                    "allwinner,sun50i-h6-emmc"))
>                 mmc->caps      |= MMC_CAP_1_8V_DDR | MMC_CAP_3_3V_DDR;
>
>         ret = mmc_of_parse(mmc);
> --
> 2.20.1
>
Maxime Ripard Aug. 27, 2019, 1:32 p.m. UTC | #2
On Sun, Aug 25, 2019 at 05:05:58PM +0200, Alejandro González wrote:
> Some Allwinner H6 boards have timing problems when dealing with
> DDR-capable eMMC cards. These boards include the Pine H64 and Tanix TX6.
>
> These timing problems result in out of sync communication between the
> driver and the eMMC, which renders the memory unsuable for every
> operation but some basic commmands, like reading the status register.
>
> The cause of these timing problems is not yet well known, but they go
> away by disabling DDR mode operation in the driver. Like on some H5
> boards, it might be that the traces are not precise enough to support
> these speeds. However, Jernej Skrabec compared the BSP driver with this
> driver, and found that the BSP driver configures pinctrl to operate at
> 1.8 V when entering DDR mode (although 3.3 V operation is supported), while
> the mainline kernel lacks any mechanism to switch voltages dynamically.
> Finally, other possible cause might be some timing parameter that is
> different on the H6 with respect to other SoCs.

This should be a comment in the driver where this is disabled.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Alejandro González Aug. 28, 2019, 10:52 a.m. UTC | #3
El 27/8/19 a las 15:24, Ulf Hansson escribió:> Assuming this should go stable as well? Perhaps you can find a
> relevant commit that we can put as a fixes tag as well?
> 
> Kind regards
> Uffe

The most relevant commit I've found that is related to enabling DDR speeds
on H6 boards is this one: https://github.com/torvalds/linux/commit/07bafc1e3536a4e3c422dbd13341688b54f159bb .
But it doesn't address the H6 SoC specifically, so I doubted whether it would
be appropiate to mark this patch as fixing it, and opted to not do it. I don't
mind adding that tag if it's appropiate, though :-)

On the other hand, I'm not sure that I understood correctly what do you mean by
this patch going stable, but I might say the changes themselves are stable and work.
The only downside I can think of to them is that they are a kind of workaround that
reduces performance on H6 boards and/or eMMC not affected by this problem (are there
any?), unless device trees are changed.

El 27/8/19 a las 15:32, Maxime Ripard escribió:
> On Sun, Aug 25, 2019 at 05:05:58PM +0200, Alejandro González wrote:
>> Some Allwinner H6 boards have timing problems when dealing with
>> DDR-capable eMMC cards. These boards include the Pine H64 and Tanix TX6.
>>
>> These timing problems result in out of sync communication between the
>> driver and the eMMC, which renders the memory unsuable for every
>> operation but some basic commmands, like reading the status register.
>>
>> The cause of these timing problems is not yet well known, but they go
>> away by disabling DDR mode operation in the driver. Like on some H5
>> boards, it might be that the traces are not precise enough to support
>> these speeds. However, Jernej Skrabec compared the BSP driver with this
>> driver, and found that the BSP driver configures pinctrl to operate at
>> 1.8 V when entering DDR mode (although 3.3 V operation is supported), while
>> the mainline kernel lacks any mechanism to switch voltages dynamically.
>> Finally, other possible cause might be some timing parameter that is
>> different on the H6 with respect to other SoCs.
> 
> This should be a comment in the driver where this is disabled.
> 
> Maxime

Thank you for your review. I'll mention that briefly in a comment in the code for
the next revision of this patch.

Regards.
Linus Walleij Aug. 28, 2019, 12:52 p.m. UTC | #4
On Sun, Aug 25, 2019 at 5:06 PM Alejandro González
<alejandro.gonzalez.correo@gmail.com> wrote:

> Jernej Skrabec compared the BSP driver with this
> driver, and found that the BSP driver configures pinctrl to operate at
> 1.8 V when entering DDR mode (although 3.3 V operation is supported), while
> the mainline kernel lacks any mechanism to switch voltages dynamically.
(...)
> the kernel lacks the required
> dynamic pinctrl control for now

This is not a pin control thing, the I/O voltage level is usually
controlled by a regulator called VCCQ, if the selection of the
voltage rails is inside the pin control registers, see the solution
in drivers/pinctrl/sh-pfc/pfc-sh73a0.c where we simply provide
a regulator from inside the pinctrl driver to make things easy
for the MMC core. Do this thing!

If you don't have time to fix it up properly right now I would slap
in a big FIXME in the code so people know this needs
to be fixed properly.

Yours,
Linus Walleij
Chen-Yu Tsai Aug. 28, 2019, 1:29 p.m. UTC | #5
On Wed, Aug 28, 2019 at 8:52 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Sun, Aug 25, 2019 at 5:06 PM Alejandro González
> <alejandro.gonzalez.correo@gmail.com> wrote:
>
> > Jernej Skrabec compared the BSP driver with this
> > driver, and found that the BSP driver configures pinctrl to operate at
> > 1.8 V when entering DDR mode (although 3.3 V operation is supported), while
> > the mainline kernel lacks any mechanism to switch voltages dynamically.

AFAIK The Pine H64 does not have the ability to switch I/O voltages. It is
fixed to either 1.8V (the default based on the schematics) or 3.3V.

> (...)
> > the kernel lacks the required
> > dynamic pinctrl control for now
>
> This is not a pin control thing, the I/O voltage level is usually
> controlled by a regulator called VCCQ, if the selection of the
> voltage rails is inside the pin control registers, see the solution
> in drivers/pinctrl/sh-pfc/pfc-sh73a0.c where we simply provide
> a regulator from inside the pinctrl driver to make things easy
> for the MMC core. Do this thing!

Or if it's simply voltage trimming for input, the a80 pinctrl driver
has something similar. Basically it registers a notifier on the
voltage rail supplying a set of pins, and toggles the register
to match the external voltage provided.

Unfortunately the user manual is quite vague on what it actually is.

> If you don't have time to fix it up properly right now I would slap
> in a big FIXME in the code so people know this needs
> to be fixed properly.

+1

ChenYu
Maxime Ripard Aug. 28, 2019, 1:43 p.m. UTC | #6
On Wed, Aug 28, 2019 at 09:29:32PM +0800, Chen-Yu Tsai wrote:
> On Wed, Aug 28, 2019 at 8:52 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> >
> > On Sun, Aug 25, 2019 at 5:06 PM Alejandro González
> > <alejandro.gonzalez.correo@gmail.com> wrote:
> >
> > > Jernej Skrabec compared the BSP driver with this
> > > driver, and found that the BSP driver configures pinctrl to operate at
> > > 1.8 V when entering DDR mode (although 3.3 V operation is supported), while
> > > the mainline kernel lacks any mechanism to switch voltages dynamically.
>
> AFAIK The Pine H64 does not have the ability to switch I/O voltages. It is
> fixed to either 1.8V (the default based on the schematics) or 3.3V.

Should that be handled at the board level then maybe?

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Chen-Yu Tsai Aug. 28, 2019, 1:46 p.m. UTC | #7
On Wed, Aug 28, 2019 at 9:43 PM Maxime Ripard <mripard@kernel.org> wrote:
>
> On Wed, Aug 28, 2019 at 09:29:32PM +0800, Chen-Yu Tsai wrote:
> > On Wed, Aug 28, 2019 at 8:52 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> > >
> > > On Sun, Aug 25, 2019 at 5:06 PM Alejandro González
> > > <alejandro.gonzalez.correo@gmail.com> wrote:
> > >
> > > > Jernej Skrabec compared the BSP driver with this
> > > > driver, and found that the BSP driver configures pinctrl to operate at
> > > > 1.8 V when entering DDR mode (although 3.3 V operation is supported), while
> > > > the mainline kernel lacks any mechanism to switch voltages dynamically.
> >
> > AFAIK The Pine H64 does not have the ability to switch I/O voltages. It is
> > fixed to either 1.8V (the default based on the schematics) or 3.3V.
>
> Should that be handled at the board level then maybe?

Yeah. You'd specify which one using vqmmc-supply in the mmc node and
vcc-pc-supply
in the pinctrl node.

ChenYu
Ulf Hansson Aug. 29, 2019, 11:37 a.m. UTC | #8
On Wed, 28 Aug 2019 at 12:52, Alejandro González
<alejandro.gonzalez.correo@gmail.com> wrote:
>
> El 27/8/19 a las 15:24, Ulf Hansson escribió:> Assuming this should go stable as well? Perhaps you can find a
> > relevant commit that we can put as a fixes tag as well?
> >
> > Kind regards
> > Uffe
>
> The most relevant commit I've found that is related to enabling DDR speeds
> on H6 boards is this one: https://github.com/torvalds/linux/commit/07bafc1e3536a4e3c422dbd13341688b54f159bb .
> But it doesn't address the H6 SoC specifically, so I doubted whether it would
> be appropiate to mark this patch as fixing it, and opted to not do it. I don't
> mind adding that tag if it's appropiate, though :-)

Hard to say what makes sense here, but how about picking this below instead?

Fixes: 0a23f1ad88fc ("dt-binding: mmc: sunxi: add H6 compatible (with
A64 fallback)")

>
> On the other hand, I'm not sure that I understood correctly what do you mean by
> this patch going stable, but I might say the changes themselves are stable and work.
> The only downside I can think of to them is that they are a kind of workaround that
> reduces performance on H6 boards and/or eMMC not affected by this problem (are there
> any?), unless device trees are changed.

Adding a stable tag and a fixes tag for the commit, makes maintainers
of stable kernels to try to backport this commit and fix the problem
for "older" kernels.

Kind regards
Uffe

Patch
diff mbox series

diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index d577a6b0ceae..dac57d76d009 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -1395,14 +1395,17 @@  static int sunxi_mmc_probe(struct platform_device *pdev)
 
 	/*
 	 * Some H5 devices do not have signal traces precise enough to
-	 * use HS DDR mode for their eMMC chips.
+	 * use HS DDR mode for their eMMC chips. Other H6 devices operate
+	 * unreliably on HS DDR mode, too.
 	 *
 	 * We still enable HS DDR modes for all the other controller
-	 * variants that support them.
+	 * variants that support them properly.
 	 */
 	if ((host->cfg->clk_delays || host->use_new_timings) &&
 	    !of_device_is_compatible(pdev->dev.of_node,
-				     "allwinner,sun50i-h5-emmc"))
+				     "allwinner,sun50i-h5-emmc") &&
+	    !of_device_is_compatible(pdev->dev.of_node,
+				     "allwinner,sun50i-h6-emmc"))
 		mmc->caps      |= MMC_CAP_1_8V_DDR | MMC_CAP_3_3V_DDR;
 
 	ret = mmc_of_parse(mmc);