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

[Simon Goldschmidt]

+Simon Glass for the xxx_get_ops() functions in DM

On Mon, Feb 10, 2020 at 10:46 AM Faiz Abbas <faiz_abbas at 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 at 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 at ti.com>
> >> Signed-off-by: Lokesh Vutla <lokeshvutla at 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