[RFC] mmc: fsl_esdhc_imx: Use esdhc_soc_data flags to set host caps

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[RFC] mmc: fsl_esdhc_imx: Use esdhc_soc_data flags to set host caps

Adam Ford
The Linux driver automatically can detect and enable UHS, HS200, HS400
and HS400_ES automatically without extra flags being placed into the
device tree.

Right now, for U-Boot to use UHS, HS200 or HS400, the extra flags are
needed.  Let's go through the esdhc_soc_data flags and enable the
host caps where applicable.

Suggested-by: Fabio Estevam <[hidden email]>
Signed-off-by: Adam Ford <[hidden email]>
---
I am not an expert on the SD/MMC standards, but I used the Linux
driver as a model, and made the assumption that the USDHC flag needs
to be set in order to use the extra speeds.

diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c
index e5409ade1b..3f1774551a 100644
--- a/drivers/mmc/fsl_esdhc_imx.c
+++ b/drivers/mmc/fsl_esdhc_imx.c
@@ -1293,8 +1293,30 @@ static int fsl_esdhc_init(struct fsl_esdhc_priv *priv,
  val |= ESDHC_TUNING_CMD_CRC_CHECK_DISABLE;
  esdhc_write32(&regs->tuning_ctrl, val);
  }
- }
 
+ if (CONFIG_IS_ENABLED(MMC_UHS_SUPPORT)) {
+ cfg->host_caps |= MMC_CAP(UHS_SDR12);
+ cfg->host_caps |= MMC_CAP(UHS_SDR25);
+ cfg->host_caps |= MMC_CAP(UHS_SDR50);
+ cfg->host_caps |= MMC_CAP(UHS_SDR104);
+ cfg->host_caps |= MMC_CAP(UHS_DDR50);
+ }
+
+ if (CONFIG_IS_ENABLED(MMC_HS200_SUPPORT)) {
+ if (priv->flags & ESDHC_FLAG_HS200)
+ cfg->host_caps |= MMC_CAP(MMC_HS_200);
+ }
+
+ if (CONFIG_IS_ENABLED(MMC_HS400_SUPPORT)) {
+ if (priv->flags & ESDHC_FLAG_HS400)
+ cfg->host_caps |= MMC_CAP(MMC_HS_400);
+ }
+
+ if (CONFIG_IS_ENABLED(MMC_HS400_ES_SUPPORT)) {
+ if (priv->flags & ESDHC_FLAG_HS400_ES)
+ cfg->host_caps |= MMC_CAP(MMC_HS_400_ES);
+ }
+ }
  return 0;
 }
 
--
2.25.1

Reply | Threaded
Open this post in threaded view
|

Re: [RFC] mmc: fsl_esdhc_imx: Use esdhc_soc_data flags to set host caps

Jaehoon Chung
Hi Adam,

On 12/31/20 2:39 AM, Adam Ford wrote:

> The Linux driver automatically can detect and enable UHS, HS200, HS400
> and HS400_ES automatically without extra flags being placed into the
> device tree.
>
> Right now, for U-Boot to use UHS, HS200 or HS400, the extra flags are
> needed.  Let's go through the esdhc_soc_data flags and enable the
> host caps where applicable.
>
> Suggested-by: Fabio Estevam <[hidden email]>
> Signed-off-by: Adam Ford <[hidden email]>
> ---
> I am not an expert on the SD/MMC standards, but I used the Linux
> driver as a model, and made the assumption that the USDHC flag needs
> to be set in order to use the extra speeds.

Looks good to me. If it can't parse property from device-tree, it needs to set host_caps as proper mode.
Does it work fine about UHS mode? AFAIK, there was an issue about not detecting host capabilities.

>
> diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c
> index e5409ade1b..3f1774551a 100644
> --- a/drivers/mmc/fsl_esdhc_imx.c
> +++ b/drivers/mmc/fsl_esdhc_imx.c
> @@ -1293,8 +1293,30 @@ static int fsl_esdhc_init(struct fsl_esdhc_priv *priv,
>   val |= ESDHC_TUNING_CMD_CRC_CHECK_DISABLE;
>   esdhc_write32(&regs->tuning_ctrl, val);
>   }
> - }
>  
> + if (CONFIG_IS_ENABLED(MMC_UHS_SUPPORT)) {
> + cfg->host_caps |= MMC_CAP(UHS_SDR12);
> + cfg->host_caps |= MMC_CAP(UHS_SDR25);
> + cfg->host_caps |= MMC_CAP(UHS_SDR50);
> + cfg->host_caps |= MMC_CAP(UHS_SDR104);
> + cfg->host_caps |= MMC_CAP(UHS_DDR50);

If it needs to set all capabilities, then you can use UHS_CAPS instead of them.

> + }
> +
> + if (CONFIG_IS_ENABLED(MMC_HS200_SUPPORT)) {
> + if (priv->flags & ESDHC_FLAG_HS200)
> + cfg->host_caps |= MMC_CAP(MMC_HS_200);
> + }
> +
> + if (CONFIG_IS_ENABLED(MMC_HS400_SUPPORT)) {
> + if (priv->flags & ESDHC_FLAG_HS400)
> + cfg->host_caps |= MMC_CAP(MMC_HS_400);
> + }
> +
> + if (CONFIG_IS_ENABLED(MMC_HS400_ES_SUPPORT)) {
> + if (priv->flags & ESDHC_FLAG_HS400_ES)
> + cfg->host_caps |= MMC_CAP(MMC_HS_400_ES);
> + }
> + }
>   return 0;
>  }
>  
>

Reply | Threaded
Open this post in threaded view
|

Re: [RFC] mmc: fsl_esdhc_imx: Use esdhc_soc_data flags to set host caps

Adam Ford
On Tue, Jan 5, 2021 at 4:07 PM Jaehoon Chung <[hidden email]> wrote:
>
> Hi Adam,
>
+CC: Tim Harvey,

> On 12/31/20 2:39 AM, Adam Ford wrote:
> > The Linux driver automatically can detect and enable UHS, HS200, HS400
> > and HS400_ES automatically without extra flags being placed into the
> > device tree.
> >
> > Right now, for U-Boot to use UHS, HS200 or HS400, the extra flags are
> > needed.  Let's go through the esdhc_soc_data flags and enable the
> > host caps where applicable.
> >
> > Suggested-by: Fabio Estevam <[hidden email]>
> > Signed-off-by: Adam Ford <[hidden email]>
> > ---
> > I am not an expert on the SD/MMC standards, but I used the Linux
> > driver as a model, and made the assumption that the USDHC flag needs
> > to be set in order to use the extra speeds.
>
> Looks good to me. If it can't parse property from device-tree, it needs to set host_caps as proper mode.
> Does it work fine about UHS mode? AFAIK, there was an issue about not detecting host capabilities.

From the troubleshooting I've done, without this patch the host didn't
show it was capable of UHS, but with this patch, the card doesn't show
UHS, so I think there is something wrong still, but this at least gets
us past it being a host caps issue.
I was able to get it working with HS400ES.

If you want me to hold off until we get UHS working, I can hold off.
I know Tim was having issues as well.

I was able to confirm the UHS SDR104 works on the Renesas RZ/G2H board
that I have using the same care, so I think the issue may be unique to
the esdhc driver.

adam


>
> >
> > diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c
> > index e5409ade1b..3f1774551a 100644
> > --- a/drivers/mmc/fsl_esdhc_imx.c
> > +++ b/drivers/mmc/fsl_esdhc_imx.c
> > @@ -1293,8 +1293,30 @@ static int fsl_esdhc_init(struct fsl_esdhc_priv *priv,
> >                       val |= ESDHC_TUNING_CMD_CRC_CHECK_DISABLE;
> >                       esdhc_write32(&regs->tuning_ctrl, val);
> >               }
> > -     }
> >
> > +             if (CONFIG_IS_ENABLED(MMC_UHS_SUPPORT)) {
> > +                     cfg->host_caps |= MMC_CAP(UHS_SDR12);
> > +                     cfg->host_caps |= MMC_CAP(UHS_SDR25);
> > +                     cfg->host_caps |= MMC_CAP(UHS_SDR50);
> > +                     cfg->host_caps |= MMC_CAP(UHS_SDR104);
> > +                     cfg->host_caps |= MMC_CAP(UHS_DDR50);
>
> If it needs to set all capabilities, then you can use UHS_CAPS instead of them.
>
> > +             }
> > +
> > +             if (CONFIG_IS_ENABLED(MMC_HS200_SUPPORT)) {
> > +                     if (priv->flags & ESDHC_FLAG_HS200)
> > +                             cfg->host_caps |= MMC_CAP(MMC_HS_200);
> > +             }
> > +
> > +             if (CONFIG_IS_ENABLED(MMC_HS400_SUPPORT)) {
> > +                     if (priv->flags & ESDHC_FLAG_HS400)
> > +                             cfg->host_caps |= MMC_CAP(MMC_HS_400);
> > +             }
> > +
> > +             if (CONFIG_IS_ENABLED(MMC_HS400_ES_SUPPORT)) {
> > +                     if (priv->flags & ESDHC_FLAG_HS400_ES)
> > +                             cfg->host_caps |= MMC_CAP(MMC_HS_400_ES);
> > +             }
> > +     }
> >       return 0;
> >  }
> >
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: [RFC] mmc: fsl_esdhc_imx: Use esdhc_soc_data flags to set host caps

Tim Harvey
On Tue, Jan 5, 2021 at 2:20 PM Adam Ford <[hidden email]> wrote:

>
> On Tue, Jan 5, 2021 at 4:07 PM Jaehoon Chung <[hidden email]> wrote:
> >
> > Hi Adam,
> >
> +CC: Tim Harvey,
>
> > On 12/31/20 2:39 AM, Adam Ford wrote:
> > > The Linux driver automatically can detect and enable UHS, HS200, HS400
> > > and HS400_ES automatically without extra flags being placed into the
> > > device tree.
> > >
> > > Right now, for U-Boot to use UHS, HS200 or HS400, the extra flags are
> > > needed.  Let's go through the esdhc_soc_data flags and enable the
> > > host caps where applicable.
> > >
> > > Suggested-by: Fabio Estevam <[hidden email]>
> > > Signed-off-by: Adam Ford <[hidden email]>
> > > ---
> > > I am not an expert on the SD/MMC standards, but I used the Linux
> > > driver as a model, and made the assumption that the USDHC flag needs
> > > to be set in order to use the extra speeds.
> >
> > Looks good to me. If it can't parse property from device-tree, it needs to set host_caps as proper mode.
> > Does it work fine about UHS mode? AFAIK, there was an issue about not detecting host capabilities.
>
> From the troubleshooting I've done, without this patch the host didn't
> show it was capable of UHS, but with this patch, the card doesn't show
> UHS, so I think there is something wrong still, but this at least gets
> us past it being a host caps issue.
> I was able to get it working with HS400ES.
>
> If you want me to hold off until we get UHS working, I can hold off.
> I know Tim was having issues as well.
>
> I was able to confirm the UHS SDR104 works on the Renesas RZ/G2H board
> that I have using the same care, so I think the issue may be unique to
> the esdhc driver.
>

I'm all for this patch and getting rid of the custom u-boot props that
were added to enable UHS.

I don't think there is any need to wait as this patch does not cause
any issue with microSD, just exposes something and if you hit the
issue we are hitting with the microSD failing to acknowledge the
voltage switch at least it falls back to legacy mode so it doesn't
hurt to have this in place, esp given the benefit of enabling
HS200/HS400/HS400ES. I could use some help understanding the microSD
failing to acknowledge the voltage switch as I'm completely out of
ideas.

The IMX6 SDHC supports UHS as well but it isn't enabled by this patch
- have you tried enabling UHS for IMX6?

Best regards,

Tim
Reply | Threaded
Open this post in threaded view
|

Re: [RFC] mmc: fsl_esdhc_imx: Use esdhc_soc_data flags to set host caps

Adam Ford
On Wed, Jan 6, 2021 at 7:06 PM Tim Harvey <[hidden email]> wrote:

>
> On Tue, Jan 5, 2021 at 2:20 PM Adam Ford <[hidden email]> wrote:
> >
> > On Tue, Jan 5, 2021 at 4:07 PM Jaehoon Chung <[hidden email]> wrote:
> > >
> > > Hi Adam,
> > >
> > +CC: Tim Harvey,
> >
> > > On 12/31/20 2:39 AM, Adam Ford wrote:
> > > > The Linux driver automatically can detect and enable UHS, HS200, HS400
> > > > and HS400_ES automatically without extra flags being placed into the
> > > > device tree.
> > > >
> > > > Right now, for U-Boot to use UHS, HS200 or HS400, the extra flags are
> > > > needed.  Let's go through the esdhc_soc_data flags and enable the
> > > > host caps where applicable.
> > > >
> > > > Suggested-by: Fabio Estevam <[hidden email]>
> > > > Signed-off-by: Adam Ford <[hidden email]>
> > > > ---
> > > > I am not an expert on the SD/MMC standards, but I used the Linux
> > > > driver as a model, and made the assumption that the USDHC flag needs
> > > > to be set in order to use the extra speeds.
> > >
> > > Looks good to me. If it can't parse property from device-tree, it needs to set host_caps as proper mode.
> > > Does it work fine about UHS mode? AFAIK, there was an issue about not detecting host capabilities.
> >
> > From the troubleshooting I've done, without this patch the host didn't
> > show it was capable of UHS, but with this patch, the card doesn't show
> > UHS, so I think there is something wrong still, but this at least gets
> > us past it being a host caps issue.
> > I was able to get it working with HS400ES.
> >
> > If you want me to hold off until we get UHS working, I can hold off.
> > I know Tim was having issues as well.
> >
> > I was able to confirm the UHS SDR104 works on the Renesas RZ/G2H board
> > that I have using the same care, so I think the issue may be unique to
> > the esdhc driver.
> >
>
> I'm all for this patch and getting rid of the custom u-boot props that
> were added to enable UHS.
>
> I don't think there is any need to wait as this patch does not cause
> any issue with microSD, just exposes something and if you hit the
> issue we are hitting with the microSD failing to acknowledge the
> voltage switch at least it falls back to legacy mode so it doesn't
> hurt to have this in place, esp given the benefit of enabling
> HS200/HS400/HS400ES. I could use some help understanding the microSD
> failing to acknowledge the voltage switch as I'm completely out of
> ideas.
>
> The IMX6 SDHC supports UHS as well but it isn't enabled by this patch
> - have you tried enabling UHS for IMX6?

I have an i.MX6Q laying around somewhere, but I haven't tested it.  I
would expect the patch to enable the settings based on the host caps.
If they're not set in the host flag, I think a separate patch would be
appropriate to update the i.MX6 flags regardless of whether or not
this patch gets accepted. There are so many variants of i.MX6, I am
reluctant to do it for fear of breaking something.

adam
>
> Best regards,
>
> Tim