[v2,06/10] mmc: am654_sdhci: Implement workaround for card detect

The 4 bit MMC controllers have an internal debounce for the SDCD line
with a debounce delay of 1 second. Therefore, after clocks to the IP are
enabled, software has to wait for this time before it can power on the
controller.

Add an init() callback which polls on sdcd for a maximum of 2 seconds
before switching on power to the controller or (in the case of no card)
returning a ENOMEDIUM. This pushes the 1 second wait time to when the
card is actually needed rather than at every probe() making sure that
users who don't insert an SD card in the slot don't have to wait such a
long time.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
---
 drivers/mmc/am654_sdhci.c | 35 ++++++++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

On Fri, Jan 24, 2020 at 12:52 PM Faiz Abbas <faiz_abbas@ti.com> wrote:
>
> The 4 bit MMC controllers have an internal debounce for the SDCD line
> with a debounce delay of 1 second. Therefore, after clocks to the IP are
> enabled, software has to wait for this time before it can power on the
> controller.
>
> Add an init() callback which polls on sdcd for a maximum of 2 seconds
> before switching on power to the controller or (in the case of no card)
> returning a ENOMEDIUM. This pushes the 1 second wait time to when the
> card is actually needed rather than at every probe() making sure that
> users who don't insert an SD card in the slot don't have to wait such a
> long time.
>
> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> ---
>  drivers/mmc/am654_sdhci.c | 35 ++++++++++++++++++++++++++++++++---
>  1 file changed, 32 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/am654_sdhci.c b/drivers/mmc/am654_sdhci.c
> index ff0a81eaab..ccae3fea31 100644
> --- a/drivers/mmc/am654_sdhci.c
> +++ b/drivers/mmc/am654_sdhci.c
> @@ -254,7 +254,7 @@ const struct am654_driver_data j721e_4bit_drv_data = {
>         .flags = IOMUX_PRESENT,
>  };
>
> -int am654_sdhci_init(struct am654_sdhci_plat *plat)
> +int am654_sdhci_init_phy(struct am654_sdhci_plat *plat)
>  {
>         u32 ctl_cfg_2 = 0;
>         u32 mask, val;
> @@ -331,8 +331,37 @@ static int sdhci_am654_get_otap_delay(struct udevice *dev,
>         return 0;
>  }
>
> +#define MAX_SDCD_DEBOUNCE_TIME 2000
> +int am654_sdhci_init(struct udevice *dev)
> +{
> +       struct am654_sdhci_plat *plat = dev_get_platdata(dev);
> +       struct mmc *mmc = mmc_get_mmc_dev(dev);
> +       struct sdhci_host *host = mmc->priv;
> +       unsigned long start;
> +       int val;
> +
> +       /*
> +        * The controller takes about 1 second to debounce the card detect line
> +        * and doesn't let us power on until that time is up. Instead of waiting
> +        * for 1 second at every stage, poll on the CARD_PRESENT bit upto a
> +        * maximum of 2 seconds to be safe..
> +        */
> +       start = get_timer(0);
> +       do {
> +               if (get_timer(start) > MAX_SDCD_DEBOUNCE_TIME)
> +                       return -ENOMEDIUM;
> +
> +               val = mmc_getcd(host->mmc);
> +       } while (!val);
> +
> +       am654_sdhci_init_phy(plat);
> +
> +       return sdhci_init(mmc);
> +}
> +
>  static int am654_sdhci_probe(struct udevice *dev)
>  {
> +       struct dm_mmc_ops *ops = mmc_get_ops(dev);
>         struct am654_driver_data *drv_data =
>                         (struct am654_driver_data *)dev_get_driver_data(dev);
>         struct am654_sdhci_plat *plat = dev_get_platdata(dev);
> @@ -373,9 +402,9 @@ static int am654_sdhci_probe(struct udevice *dev)
>
>         regmap_init_mem_index(dev_ofnode(dev), &plat->base, 1);
>
> -       am654_sdhci_init(plat);
> +       ops->init = am654_sdhci_init;

Is this a valid approach? I mean, many drivers create their 'ops' const like
this:
   static const struct ram_ops altera_gen5_sdram_ops (...)

That would mean you write to a const region. I know the U-Boot sources make
this easy for you by providing the ops non-const via mmc_get_ops, but I still
think this is not good.

Regards,
Simon

>
> -       return sdhci_probe(dev);
> +       return 0;
>  }
>
>  static int am654_sdhci_ofdata_to_platdata(struct udevice *dev)
> --
> 2.19.2
>

Simon,

On 29/01/20 7:48 pm, Simon Goldschmidt wrote:
> On Fri, Jan 24, 2020 at 12:52 PM Faiz Abbas <faiz_abbas@ti.com> wrote:
>>
>> The 4 bit MMC controllers have an internal debounce for the SDCD line
>> with a debounce delay of 1 second. Therefore, after clocks to the IP are
>> enabled, software has to wait for this time before it can power on the
>> controller.
>>
>> Add an init() callback which polls on sdcd for a maximum of 2 seconds
>> before switching on power to the controller or (in the case of no card)
>> returning a ENOMEDIUM. This pushes the 1 second wait time to when the
>> card is actually needed rather than at every probe() making sure that
>> users who don't insert an SD card in the slot don't have to wait such a
>> long time.
>>
>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>> ---
>>  drivers/mmc/am654_sdhci.c | 35 ++++++++++++++++++++++++++++++++---
>>  1 file changed, 32 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mmc/am654_sdhci.c b/drivers/mmc/am654_sdhci.c
>> index ff0a81eaab..ccae3fea31 100644
>> --- a/drivers/mmc/am654_sdhci.c
>> +++ b/drivers/mmc/am654_sdhci.c
>> @@ -254,7 +254,7 @@ const struct am654_driver_data j721e_4bit_drv_data = {
>>         .flags = IOMUX_PRESENT,
>>  };
>>
>> -int am654_sdhci_init(struct am654_sdhci_plat *plat)
>> +int am654_sdhci_init_phy(struct am654_sdhci_plat *plat)
>>  {
>>         u32 ctl_cfg_2 = 0;
>>         u32 mask, val;
>> @@ -331,8 +331,37 @@ static int sdhci_am654_get_otap_delay(struct udevice *dev,
>>         return 0;
>>  }
>>
>> +#define MAX_SDCD_DEBOUNCE_TIME 2000
>> +int am654_sdhci_init(struct udevice *dev)
>> +{
>> +       struct am654_sdhci_plat *plat = dev_get_platdata(dev);
>> +       struct mmc *mmc = mmc_get_mmc_dev(dev);
>> +       struct sdhci_host *host = mmc->priv;
>> +       unsigned long start;
>> +       int val;
>> +
>> +       /*
>> +        * The controller takes about 1 second to debounce the card detect line
>> +        * and doesn't let us power on until that time is up. Instead of waiting
>> +        * for 1 second at every stage, poll on the CARD_PRESENT bit upto a
>> +        * maximum of 2 seconds to be safe..
>> +        */
>> +       start = get_timer(0);
>> +       do {
>> +               if (get_timer(start) > MAX_SDCD_DEBOUNCE_TIME)
>> +                       return -ENOMEDIUM;
>> +
>> +               val = mmc_getcd(host->mmc);
>> +       } while (!val);
>> +
>> +       am654_sdhci_init_phy(plat);
>> +
>> +       return sdhci_init(mmc);
>> +}
>> +
>>  static int am654_sdhci_probe(struct udevice *dev)
>>  {
>> +       struct dm_mmc_ops *ops = mmc_get_ops(dev);
>>         struct am654_driver_data *drv_data =
>>                         (struct am654_driver_data *)dev_get_driver_data(dev);
>>         struct am654_sdhci_plat *plat = dev_get_platdata(dev);
>> @@ -373,9 +402,9 @@ static int am654_sdhci_probe(struct udevice *dev)
>>
>>         regmap_init_mem_index(dev_ofnode(dev), &plat->base, 1);
>>
>> -       am654_sdhci_init(plat);
>> +       ops->init = am654_sdhci_init;
> 
> Is this a valid approach? I mean, many drivers create their 'ops' const like
> this:
>    static const struct ram_ops altera_gen5_sdram_ops (...)
> 
> That would mean you write to a const region. I know the U-Boot sources make
> this easy for you by providing the ops non-const via mmc_get_ops, but I still
> think this is not good.
> 

Sorry I missed this earlier.

I do see other drivers following this approach (see
drivers/mmc/sdhci-cadence.c). The issue is that I then have to export
every API in drivers/mmc/sdhci.c:694

const struct dm_mmc_ops sdhci_ops = {
        .send_cmd       = sdhci_send_command,
        .set_ios        = sdhci_set_ios,
        .get_cd         = sdhci_get_cd,
#ifdef MMC_SUPPORTS_TUNING
        .execute_tuning = sdhci_execute_tuning,
#endif
};

and create a duplicate structure in my platform driver with an extra .init

OR

I have to create one more wrapper sdhci_pltaform_init() API in the sdhci
driver that just calls a platform init function inside it. So its

include/sdhci.h:

struct sdhci_ops {
..
+	int (*platform_init)()

and then drivers/mmc/sdhci.c:


+dm_sdhci_platform_init()
+{
+...
+	host->ops->platform_init();
+}

const struct dm_mmc_ops sdhci_ops  = {
...
+	.init		= dm_sdhci_platform_init,
};


Both of these approaches are too much boiler plate code for nothing so
its just simpler to use my approach itself.

Anyways, please comment here or in my next version if you have a better
idea.

Thanks,
Faiz

+Simon Glass for the xxx_get_ops() functions in DM

On Mon, Feb 10, 2020 at 10:46 AM Faiz Abbas <faiz_abbas@ti.com> wrote:
>
> Simon,
>
> On 29/01/20 7:48 pm, Simon Goldschmidt wrote:
> > On Fri, Jan 24, 2020 at 12:52 PM Faiz Abbas <faiz_abbas@ti.com> wrote:
> >>
> >> The 4 bit MMC controllers have an internal debounce for the SDCD line
> >> with a debounce delay of 1 second. Therefore, after clocks to the IP are
> >> enabled, software has to wait for this time before it can power on the
> >> controller.
> >>
> >> Add an init() callback which polls on sdcd for a maximum of 2 seconds
> >> before switching on power to the controller or (in the case of no card)
> >> returning a ENOMEDIUM. This pushes the 1 second wait time to when the
> >> card is actually needed rather than at every probe() making sure that
> >> users who don't insert an SD card in the slot don't have to wait such a
> >> long time.
> >>
> >> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> >> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> >> ---
> >>  drivers/mmc/am654_sdhci.c | 35 ++++++++++++++++++++++++++++++++---
> >>  1 file changed, 32 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/mmc/am654_sdhci.c b/drivers/mmc/am654_sdhci.c
> >> index ff0a81eaab..ccae3fea31 100644
> >> --- a/drivers/mmc/am654_sdhci.c
> >> +++ b/drivers/mmc/am654_sdhci.c
> >> @@ -254,7 +254,7 @@ const struct am654_driver_data j721e_4bit_drv_data = {
> >>         .flags = IOMUX_PRESENT,
> >>  };
> >>
> >> -int am654_sdhci_init(struct am654_sdhci_plat *plat)
> >> +int am654_sdhci_init_phy(struct am654_sdhci_plat *plat)
> >>  {
> >>         u32 ctl_cfg_2 = 0;
> >>         u32 mask, val;
> >> @@ -331,8 +331,37 @@ static int sdhci_am654_get_otap_delay(struct udevice *dev,
> >>         return 0;
> >>  }
> >>
> >> +#define MAX_SDCD_DEBOUNCE_TIME 2000
> >> +int am654_sdhci_init(struct udevice *dev)
> >> +{
> >> +       struct am654_sdhci_plat *plat = dev_get_platdata(dev);
> >> +       struct mmc *mmc = mmc_get_mmc_dev(dev);
> >> +       struct sdhci_host *host = mmc->priv;
> >> +       unsigned long start;
> >> +       int val;
> >> +
> >> +       /*
> >> +        * The controller takes about 1 second to debounce the card detect line
> >> +        * and doesn't let us power on until that time is up. Instead of waiting
> >> +        * for 1 second at every stage, poll on the CARD_PRESENT bit upto a
> >> +        * maximum of 2 seconds to be safe..
> >> +        */
> >> +       start = get_timer(0);
> >> +       do {
> >> +               if (get_timer(start) > MAX_SDCD_DEBOUNCE_TIME)
> >> +                       return -ENOMEDIUM;
> >> +
> >> +               val = mmc_getcd(host->mmc);
> >> +       } while (!val);
> >> +
> >> +       am654_sdhci_init_phy(plat);
> >> +
> >> +       return sdhci_init(mmc);
> >> +}
> >> +
> >>  static int am654_sdhci_probe(struct udevice *dev)
> >>  {
> >> +       struct dm_mmc_ops *ops = mmc_get_ops(dev);
> >>         struct am654_driver_data *drv_data =
> >>                         (struct am654_driver_data *)dev_get_driver_data(dev);
> >>         struct am654_sdhci_plat *plat = dev_get_platdata(dev);
> >> @@ -373,9 +402,9 @@ static int am654_sdhci_probe(struct udevice *dev)
> >>
> >>         regmap_init_mem_index(dev_ofnode(dev), &plat->base, 1);
> >>
> >> -       am654_sdhci_init(plat);
> >> +       ops->init = am654_sdhci_init;
> >
> > Is this a valid approach? I mean, many drivers create their 'ops' const like
> > this:
> >    static const struct ram_ops altera_gen5_sdram_ops (...)
> >
> > That would mean you write to a const region. I know the U-Boot sources make
> > this easy for you by providing the ops non-const via mmc_get_ops, but I still
> > think this is not good.
> >
>
> Sorry I missed this earlier.
>
> I do see other drivers following this approach (see
> drivers/mmc/sdhci-cadence.c). The issue is that I then have to export
> every API in drivers/mmc/sdhci.c:694
>
> const struct dm_mmc_ops sdhci_ops = {
>         .send_cmd       = sdhci_send_command,
>         .set_ios        = sdhci_set_ios,
>         .get_cd         = sdhci_get_cd,
> #ifdef MMC_SUPPORTS_TUNING
>         .execute_tuning = sdhci_execute_tuning,
> #endif
> };
>
> and create a duplicate structure in my platform driver with an extra .init
>
> OR
>
> I have to create one more wrapper sdhci_pltaform_init() API in the sdhci
> driver that just calls a platform init function inside it. So its
>
> include/sdhci.h:
>
> struct sdhci_ops {
> ..
> +       int (*platform_init)()
>
> and then drivers/mmc/sdhci.c:
>
>
> +dm_sdhci_platform_init()
> +{
> +...
> +       host->ops->platform_init();
> +}
>
> const struct dm_mmc_ops sdhci_ops  = {
> ...
> +       .init           = dm_sdhci_platform_init,
> };
>
>
> Both of these approaches are too much boiler plate code for nothing so
> its just simpler to use my approach itself.
>
> Anyways, please comment here or in my next version if you have a better
> idea.

Honestly, I don't know the mmc code well enough to give you an alternative.

However, I do know that most drivers define 'ops' as const, so to prevent
writing to const memory, mmc_get_ops() and other similar accessors should return
a const pointer. I know that they don't currently, but strictly speaking, this
is a bug.

Your adding this code here makes it harder to fix that bug and change
mmc_get_ops() to return a const pointer.

Regards,
Simon

Hi,

On Mon, 10 Feb 2020 at 04:26, Simon Goldschmidt
<simon.k.r.goldschmidt@gmail.com> wrote:
>
> +Simon Glass for the xxx_get_ops() functions in DM
>
> On Mon, Feb 10, 2020 at 10:46 AM Faiz Abbas <faiz_abbas@ti.com> wrote:
> >
> > Simon,
> >
> > On 29/01/20 7:48 pm, Simon Goldschmidt wrote:
> > > On Fri, Jan 24, 2020 at 12:52 PM Faiz Abbas <faiz_abbas@ti.com> wrote:
> > >>
> > >> The 4 bit MMC controllers have an internal debounce for the SDCD line
> > >> with a debounce delay of 1 second. Therefore, after clocks to the IP are
> > >> enabled, software has to wait for this time before it can power on the
> > >> controller.
> > >>
> > >> Add an init() callback which polls on sdcd for a maximum of 2 seconds
> > >> before switching on power to the controller or (in the case of no card)
> > >> returning a ENOMEDIUM. This pushes the 1 second wait time to when the
> > >> card is actually needed rather than at every probe() making sure that
> > >> users who don't insert an SD card in the slot don't have to wait such a
> > >> long time.
> > >>
> > >> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> > >> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> > >> ---
> > >>  drivers/mmc/am654_sdhci.c | 35 ++++++++++++++++++++++++++++++++---
> > >>  1 file changed, 32 insertions(+), 3 deletions(-)
> > >>
> > >> diff --git a/drivers/mmc/am654_sdhci.c b/drivers/mmc/am654_sdhci.c
> > >> index ff0a81eaab..ccae3fea31 100644
> > >> --- a/drivers/mmc/am654_sdhci.c
> > >> +++ b/drivers/mmc/am654_sdhci.c
> > >> @@ -254,7 +254,7 @@ const struct am654_driver_data j721e_4bit_drv_data = {
> > >>         .flags = IOMUX_PRESENT,
> > >>  };
> > >>
> > >> -int am654_sdhci_init(struct am654_sdhci_plat *plat)
> > >> +int am654_sdhci_init_phy(struct am654_sdhci_plat *plat)
> > >>  {
> > >>         u32 ctl_cfg_2 = 0;
> > >>         u32 mask, val;
> > >> @@ -331,8 +331,37 @@ static int sdhci_am654_get_otap_delay(struct udevice *dev,
> > >>         return 0;
> > >>  }
> > >>
> > >> +#define MAX_SDCD_DEBOUNCE_TIME 2000
> > >> +int am654_sdhci_init(struct udevice *dev)
> > >> +{
> > >> +       struct am654_sdhci_plat *plat = dev_get_platdata(dev);
> > >> +       struct mmc *mmc = mmc_get_mmc_dev(dev);
> > >> +       struct sdhci_host *host = mmc->priv;
> > >> +       unsigned long start;
> > >> +       int val;
> > >> +
> > >> +       /*
> > >> +        * The controller takes about 1 second to debounce the card detect line
> > >> +        * and doesn't let us power on until that time is up. Instead of waiting
> > >> +        * for 1 second at every stage, poll on the CARD_PRESENT bit upto a
> > >> +        * maximum of 2 seconds to be safe..
> > >> +        */
> > >> +       start = get_timer(0);
> > >> +       do {
> > >> +               if (get_timer(start) > MAX_SDCD_DEBOUNCE_TIME)
> > >> +                       return -ENOMEDIUM;
> > >> +
> > >> +               val = mmc_getcd(host->mmc);
> > >> +       } while (!val);
> > >> +
> > >> +       am654_sdhci_init_phy(plat);
> > >> +
> > >> +       return sdhci_init(mmc);
> > >> +}
> > >> +
> > >>  static int am654_sdhci_probe(struct udevice *dev)
> > >>  {
> > >> +       struct dm_mmc_ops *ops = mmc_get_ops(dev);
> > >>         struct am654_driver_data *drv_data =
> > >>                         (struct am654_driver_data *)dev_get_driver_data(dev);
> > >>         struct am654_sdhci_plat *plat = dev_get_platdata(dev);
> > >> @@ -373,9 +402,9 @@ static int am654_sdhci_probe(struct udevice *dev)
> > >>
> > >>         regmap_init_mem_index(dev_ofnode(dev), &plat->base, 1);
> > >>
> > >> -       am654_sdhci_init(plat);
> > >> +       ops->init = am654_sdhci_init;
> > >
> > > Is this a valid approach? I mean, many drivers create their 'ops' const like
> > > this:
> > >    static const struct ram_ops altera_gen5_sdram_ops (...)
> > >
> > > That would mean you write to a const region. I know the U-Boot sources make
> > > this easy for you by providing the ops non-const via mmc_get_ops, but I still
> > > think this is not good.
> > >
> >
> > Sorry I missed this earlier.
> >
> > I do see other drivers following this approach (see
> > drivers/mmc/sdhci-cadence.c). The issue is that I then have to export
> > every API in drivers/mmc/sdhci.c:694
> >
> > const struct dm_mmc_ops sdhci_ops = {
> >         .send_cmd       = sdhci_send_command,
> >         .set_ios        = sdhci_set_ios,
> >         .get_cd         = sdhci_get_cd,
> > #ifdef MMC_SUPPORTS_TUNING
> >         .execute_tuning = sdhci_execute_tuning,
> > #endif
> > };
> >
> > and create a duplicate structure in my platform driver with an extra .init
> >
> > OR
> >
> > I have to create one more wrapper sdhci_pltaform_init() API in the sdhci
> > driver that just calls a platform init function inside it. So its
> >
> > include/sdhci.h:
> >
> > struct sdhci_ops {
> > ..
> > +       int (*platform_init)()
> >
> > and then drivers/mmc/sdhci.c:
> >
> >
> > +dm_sdhci_platform_init()
> > +{
> > +...
> > +       host->ops->platform_init();
> > +}
> >
> > const struct dm_mmc_ops sdhci_ops  = {
> > ...
> > +       .init           = dm_sdhci_platform_init,
> > };
> >
> >
> > Both of these approaches are too much boiler plate code for nothing so
> > its just simpler to use my approach itself.
> >
> > Anyways, please comment here or in my next version if you have a better
> > idea.
>
> Honestly, I don't know the mmc code well enough to give you an alternative.
>
> However, I do know that most drivers define 'ops' as const, so to prevent
> writing to const memory, mmc_get_ops() and other similar accessors should return
> a const pointer. I know that they don't currently, but strictly speaking, this
> is a bug.
>
> Your adding this code here makes it harder to fix that bug and change
> mmc_get_ops() to return a const pointer.

I really don't see why we want mmc_get_ops(). Can we just add the
operation to the existing MMC operations struct? What is the
disadvantage there?

In general with DM we should avoid callbacks and weak functions since
they bypass DM.

Regards,
Simon