+Simon Glass for the xxx_get_ops() functions in DM
On Mon, Feb 10, 2020 at 10:46 AM Faiz Abbas <faiz_ab...@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_ab...@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_ab...@ti.com>
> >> Signed-off-by: Lokesh Vutla <lokeshvu...@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
