On 1/24/20 8:52 PM, Faiz Abbas wrote:

Add a saved_clock member to struct mmc to store the previous clock speed in the clock needs to be stopped for some time.

I think that it doesn't need to add saved_clock for mmc member. This is for only yours. Does it impossible to add saved_clock in platdata?

And i'm not sure but mmc->tran_speed should be kept previous speed. I will check it

Best Regards, Jaehoon Chung

Signed-off-by: Faiz Abbas faiz_abbas@ti.com Signed-off-by: Lokesh Vutla lokeshvutla@ti.com

---

include/mmc.h | 1 + 1 file changed, 1 insertion(+)

diff --git a/include/mmc.h b/include/mmc.h index b5cb514f57..2f21dbf1b7 100644 --- a/include/mmc.h +++ b/include/mmc.h @@ -602,6 +602,7 @@ struct mmc { bool clk_disable; /* true if the clock can be turned off */ uint bus_width; uint clock;

• uint saved_clock; enum mmc_voltage signal_voltage; uint card_caps; uint host_caps;

On Fri, Jan 24, 2020 at 12:52 PM Faiz Abbas faiz_abbas@ti.com wrote:

Add an init() API for platform specific init() operations.

Could you describe why this cannot be done in the probe callback? It's not easily visible as the function you changed (mmc_get_op_cond) doesn't even have a comment to describe what it does...

In general, I think commit messages could be more detailed than one line. If only to make it easier in the future to recap why things have been done.

Signed-off-by: Faiz Abbas faiz_abbas@ti.com Signed-off-by: Lokesh Vutla lokeshvutla@ti.com

---

drivers/mmc/mmc.c | 13 ++++++------- include/mmc.h | 7 +++++++ 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index d43983d4a6..50df8c8626 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -2787,14 +2787,13 @@ int mmc_get_op_cond(struct mmc *mmc) } if (err) return err;

#if CONFIG_IS_ENABLED(DM_MMC)

•   /* The device has already been probed ready for use */
  

-#else

•   /* made sure it's not NULL earlier */
  

•   err = mmc->cfg->ops->init(mmc);
  

•   if (err)
  

•           return err;
  

You're removing the init code for non-DM MMC here and did not describe it in the commit message. Is this change intended at all?

Regards, Simon

•   struct dm_mmc_ops *ops = mmc_get_ops(mmc->dev);
  

•   if (ops->init) {
  

•           err = ops->init(mmc->dev);
  

•           if (err)
  

•                   return err;
  

•   }
  

#endif mmc->ddr_mode = 0;

diff --git a/include/mmc.h b/include/mmc.h index 2f21dbf1b7..6aef125f25 100644 --- a/include/mmc.h +++ b/include/mmc.h @@ -406,6 +406,13 @@ struct mmc;

#if CONFIG_IS_ENABLED(DM_MMC) struct dm_mmc_ops {

•   /**
  

•    * init() - platform specific initialization for the host controller
  

•    *
  

•    * @dev:        Device to init
  

•    * @return 0 if Ok, -ve if error
  

•    */
  

•   int (*init)(struct udevice *dev);
    /**
     * send_cmd() - Send a command to the MMC device
     *
  

-- 2.19.2

On 1/24/20 8:52 PM, Faiz Abbas wrote:

Expose sdhci_init() as non-static.

Does it need to change to non-static?

Best Regards, Jaehoon Chung

Signed-off-by: Faiz Abbas faiz_abbas@ti.com Signed-off-by: Lokesh Vutla lokeshvutla@ti.com

---

drivers/mmc/sdhci.c | 2 +- include/sdhci.h | 1 + 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c index 01fa5a9d4d..4fce85a0ea 100644 --- a/drivers/mmc/sdhci.c +++ b/drivers/mmc/sdhci.c @@ -618,7 +618,7 @@ static int sdhci_set_ios(struct mmc *mmc) return 0; }

-static int sdhci_init(struct mmc *mmc) +int sdhci_init(struct mmc *mmc) { struct sdhci_host *host = mmc->priv; #if CONFIG_IS_ENABLED(DM_MMC) && CONFIG_IS_ENABLED(DM_GPIO) diff --git a/include/sdhci.h b/include/sdhci.h index 01addb7a60..0830e05d53 100644 --- a/include/sdhci.h +++ b/include/sdhci.h @@ -487,6 +487,7 @@ void sdhci_set_uhs_timing(struct sdhci_host *host); #ifdef CONFIG_DM_MMC /* Export the operations to drivers */ int sdhci_probe(struct udevice *dev); +int sdhci_init(struct mmc *mmc); int sdhci_set_clock(struct mmc *mmc, unsigned int clock); extern const struct dm_mmc_ops sdhci_ops; #else

Hi Simon,

On 1/29/20 11:16 PM, Simon Goldschmidt wrote:

On Wed, Jan 29, 2020 at 12:00 AM Jaehoon Chung jh80.chung@samsung.com wrote:

...

On 1/24/20 8:52 PM, Faiz Abbas wrote:

...

Expose sdhci_init() as non-static.

Does it need to change to non-static?

And even if it needs to, can we have a reason *why* in the commit message?

When i read entire your series, it seems that doesn't need to change to non-static. All of change that touched MMC code are only for your board. I don't know Peng's opinion, but it's not my prefer.

Best Regards, Jaehoon Chung

Thanks, Simon

...

Best Regards, Jaehoon Chung

...

Signed-off-by: Faiz Abbas faiz_abbas@ti.com Signed-off-by: Lokesh Vutla lokeshvutla@ti.com

---

drivers/mmc/sdhci.c | 2 +- include/sdhci.h | 1 + 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c index 01fa5a9d4d..4fce85a0ea 100644 --- a/drivers/mmc/sdhci.c +++ b/drivers/mmc/sdhci.c @@ -618,7 +618,7 @@ static int sdhci_set_ios(struct mmc *mmc) return 0; }

-static int sdhci_init(struct mmc *mmc) +int sdhci_init(struct mmc *mmc) { struct sdhci_host *host = mmc->priv; #if CONFIG_IS_ENABLED(DM_MMC) && CONFIG_IS_ENABLED(DM_GPIO) diff --git a/include/sdhci.h b/include/sdhci.h index 01addb7a60..0830e05d53 100644 --- a/include/sdhci.h +++ b/include/sdhci.h @@ -487,6 +487,7 @@ void sdhci_set_uhs_timing(struct sdhci_host *host); #ifdef CONFIG_DM_MMC /* Export the operations to drivers */ int sdhci_probe(struct udevice *dev); +int sdhci_init(struct mmc *mmc); int sdhci_set_clock(struct mmc *mmc, unsigned int clock); extern const struct dm_mmc_ops sdhci_ops; #else

Hi Faiz,

On 2/18/20 7:35 AM, Jaehoon Chung wrote:

Hi Faiz,

On 2/17/20 9:42 PM, Faiz Abbas wrote:

...

Jaehoon,

On 17/02/20 5:47 pm, Jaehoon Chung wrote:

...

Hi,

On 2/5/20 4:33 PM, Faiz Abbas wrote:

...

Hi Peng,

On 05/02/20 12:58 pm, Peng Fan wrote:

...

...

Subject: Re: [PATCH v2 04/10] mmc: sdhci: Expose sdhci_init() as non-static

Hi,

On 31/01/20 3:55 am, Simon Goldschmidt wrote: > Am 30.01.2020 um 23:21 schrieb Jaehoon Chung: >> Hi Simon, >> >> On 1/29/20 11:16 PM, Simon Goldschmidt wrote: >>> On Wed, Jan 29, 2020 at 12:00 AM Jaehoon Chung >>> jh80.chung@samsung.com wrote: >>>> >>>> On 1/24/20 8:52 PM, Faiz Abbas wrote: >>>>> Expose sdhci_init() as non-static. >>>> >>>> Does it need to change to non-static? >>> >>> And even if it needs to, can we have a reason *why* in the commit >>> message? >> >> When i read entire your series, it seems that doesn't need to change >> to non-static. >> All of change that touched MMC code are only for your board. >> I don't know Peng's opinion, but it's not my prefer. > > +1! > > We need to keep the core code clean of such hacks in order to keep the > size small for constrained targets! >

Peng can you comment on this?

Just one more question, does kernel has same issue, how resolved? Could U-Boot follow similar approach?

Kernel has interrupts enabled. So software just has to wait for the card detect interrupt to arrive rather than poll on anything.

...

Actually I am fine with platform specific approach , considering your platform issue.

But since Simon and Jaehoon has concerns, not sure whether Simon and Jaehoon have good ideas.

Ok. Lets see if they have some better ideas.

Well, Your code needs to call am654_sdhci_init() before sdhci_init(), right?

ops->init() -> am654_sdhci_init -> sdhci_init(). If am654_sdhci_init is called for preset, how about adding pre_init() or other ops callback. (pre_init is just example.)

ops->pre_init = am654_sdhci_init; or ops->preset = am654_sdhci_preset;

In mmc.

ops->preset or ops->pre_init -> ops->init

How about adding plotform specific init callback? Then someone can also use it for platform specific things in future.

So we basically want an init() callback even in sdhci.c.

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

Yes, I checked wrong sequence. Sorry.

When i have checked, some functions are needs.

...

include/sdhci.h:

struct sdhci_ops { ..

•   int (*platform_init)()
  
  

and then drivers/mmc/sdhci.c:

+sdhci_platform_init() +{ +...

•   host->ops->platform_init();
  

+}

const struct dm_mmc_ops sdhci_ops = { ...

•   .init           = sdhci_platform_init,
  

};

Right?

Right. but not init. Just platform_init?

Well, if you want, i will make patch about callback function.

How about below codes? Then you can just add am654_sdhci_deferred_probe { ... return sdhci_probe() .. }

diff --git a/drivers/mmc/mmc-uclass.c b/drivers/mmc/mmc-uclass.c index 0b90a97650..c75892a72c 100644 --- a/drivers/mmc/mmc-uclass.c +++ b/drivers/mmc/mmc-uclass.c @@ -138,6 +138,21 @@ int mmc_host_power_cycle(struct mmc *mmc) return dm_mmc_host_power_cycle(mmc->dev); }

+int dm_mmc_deferred_probe(struct udevice *dev) +{ + struct dm_mmc_ops *ops = mmc_get_ops(dev); + + if (ops->deferred_probe) + return ops->deferred_probe(dev); + + return 0; +} + +int mmc_deferred_probe(struct mmc *mmc) +{ + return dm_mmc_deferred_probe(mmc->dev); +} + int mmc_of_parse(struct udevice *dev, struct mmc_config *cfg) { int val; diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index d43983d4a6..9eae538af4 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -2790,6 +2790,7 @@ int mmc_get_op_cond(struct mmc *mmc)

#if CONFIG_IS_ENABLED(DM_MMC) /* The device has already been probed ready for use */ + mmc_deferred_probe(mmc); #else /* made sure it's not NULL earlier */ err = mmc->cfg->ops->init(mmc); diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c index 01fa5a9d4d..9ff37b888b 100644 --- a/drivers/mmc/sdhci.c +++ b/drivers/mmc/sdhci.c @@ -661,6 +661,20 @@ int sdhci_probe(struct udevice *dev) return sdhci_init(mmc); }

+static int sdhci_deferred_probe(struct udevice *dev) +{ + int err; + struct mmc *mmc = mmc_get_mmc_dev(dev); + struct sdhci_host *host = mmc->priv; + + if (host->ops && host->ops->deferred_probe) { + err = host->ops->deferred_probe(host); + if (err) + return err; + } + return 0; +} + static int sdhci_get_cd(struct udevice *dev) { struct mmc *mmc = mmc_get_mmc_dev(dev); @@ -695,6 +709,7 @@ const struct dm_mmc_ops sdhci_ops = { .send_cmd = sdhci_send_command, .set_ios = sdhci_set_ios, .get_cd = sdhci_get_cd, + .deferred_probe = sdhci_deferred_probe, #ifdef MMC_SUPPORTS_TUNING .execute_tuning = sdhci_execute_tuning, #endif diff --git a/include/mmc.h b/include/mmc.h index b5cb514f57..b362b7f4c7 100644 --- a/include/mmc.h +++ b/include/mmc.h @@ -477,6 +477,8 @@ struct dm_mmc_ops { * @return 0 if not present, 1 if present, -ve on error */ int (*host_power_cycle)(struct udevice *dev); + + int (*deferred_probe)(struct udevice *dev); };

#define mmc_get_ops(dev) ((struct dm_mmc_ops *)(dev)->driver->ops) @@ -489,6 +491,7 @@ int dm_mmc_get_wp(struct udevice *dev); int dm_mmc_execute_tuning(struct udevice *dev, uint opcode); int dm_mmc_wait_dat0(struct udevice *dev, int state, int timeout_us); int dm_mmc_host_power_cycle(struct udevice *dev); +int dm_mmc_deferred_probe(struct udevice *dev);

/* Transition functions for compatibility */ int mmc_set_ios(struct mmc *mmc); @@ -498,6 +501,7 @@ int mmc_execute_tuning(struct mmc *mmc, uint opcode); int mmc_wait_dat0(struct mmc *mmc, int state, int timeout_us); int mmc_set_enhanced_strobe(struct mmc *mmc); int mmc_host_power_cycle(struct mmc *mmc); +int mmc_deferred_probe(struct mmc *mmc);

#else struct mmc_ops { diff --git a/include/sdhci.h b/include/sdhci.h index 01addb7a60..1276f43935 100644 --- a/include/sdhci.h +++ b/include/sdhci.h @@ -267,6 +267,7 @@ struct sdhci_ops { void (*set_clock)(struct sdhci_host *host, u32 div); int (*platform_execute_tuning)(struct mmc *host, u8 opcode); void (*set_delay)(struct sdhci_host *host); + int (*deferred_probe)(struct sdhci_host *host); };

#if CONFIG_IS_ENABLED(MMC_SDHCI_ADMA)

Best Regards, Jaehoon Chung

...

Thanks, Faiz

On 2/18/20 4:51 PM, Faiz Abbas wrote:

Jaehoon,

On 18/02/20 4:54 am, Jaehoon Chung wrote:

...

Hi Faiz,

On 2/18/20 7:35 AM, Jaehoon Chung wrote:

...

Hi Faiz,

On 2/17/20 9:42 PM, Faiz Abbas wrote:

...

Jaehoon,

On 17/02/20 5:47 pm, Jaehoon Chung wrote:

...

Hi,

On 2/5/20 4:33 PM, Faiz Abbas wrote:

...

Hi Peng,

On 05/02/20 12:58 pm, Peng Fan wrote: >> Subject: Re: [PATCH v2 04/10] mmc: sdhci: Expose sdhci_init() as non-static >>

...

...

...

Well, if you want, i will make patch about callback function.

How about below codes? Then you can just add am654_sdhci_deferred_probe { ... return sdhci_probe() .. }

diff --git a/drivers/mmc/mmc-uclass.c b/drivers/mmc/mmc-uclass.c index 0b90a97650..c75892a72c 100644 --- a/drivers/mmc/mmc-uclass.c +++ b/drivers/mmc/mmc-uclass.c @@ -138,6 +138,21 @@ int mmc_host_power_cycle(struct mmc *mmc) return dm_mmc_host_power_cycle(mmc->dev); }

+int dm_mmc_deferred_probe(struct udevice *dev) +{

• struct dm_mmc_ops *ops = mmc_get_ops(dev);
• if (ops->deferred_probe)

• return ops->deferred_probe(dev);
  

• return 0;

+}

+int mmc_deferred_probe(struct mmc *mmc) +{

• return dm_mmc_deferred_probe(mmc->dev);

+}

int mmc_of_parse(struct udevice *dev, struct mmc_config *cfg) { int val; diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index d43983d4a6..9eae538af4 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -2790,6 +2790,7 @@ int mmc_get_op_cond(struct mmc *mmc)

#if CONFIG_IS_ENABLED(DM_MMC) /* The device has already been probed ready for use */

• mmc_deferred_probe(mmc);

#else /* made sure it's not NULL earlier */ err = mmc->cfg->ops->init(mmc); diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c index 01fa5a9d4d..9ff37b888b 100644 --- a/drivers/mmc/sdhci.c +++ b/drivers/mmc/sdhci.c @@ -661,6 +661,20 @@ int sdhci_probe(struct udevice *dev) return sdhci_init(mmc); }

+static int sdhci_deferred_probe(struct udevice *dev) +{

• int err;
• struct mmc *mmc = mmc_get_mmc_dev(dev);
• struct sdhci_host *host = mmc->priv;
• if (host->ops && host->ops->deferred_probe) {

• err = host->ops->deferred_probe(host);
  

• if (err)
  

• 	return err;
  

• }
• return 0;

+}

static int sdhci_get_cd(struct udevice *dev) { struct mmc *mmc = mmc_get_mmc_dev(dev); @@ -695,6 +709,7 @@ const struct dm_mmc_ops sdhci_ops = { .send_cmd = sdhci_send_command, .set_ios = sdhci_set_ios, .get_cd = sdhci_get_cd,

• .deferred_probe = sdhci_deferred_probe,

#ifdef MMC_SUPPORTS_TUNING .execute_tuning = sdhci_execute_tuning, #endif diff --git a/include/mmc.h b/include/mmc.h index b5cb514f57..b362b7f4c7 100644 --- a/include/mmc.h +++ b/include/mmc.h @@ -477,6 +477,8 @@ struct dm_mmc_ops { * @return 0 if not present, 1 if present, -ve on error */ int (*host_power_cycle)(struct udevice *dev);

• int (*deferred_probe)(struct udevice *dev);

};

#define mmc_get_ops(dev) ((struct dm_mmc_ops *)(dev)->driver->ops) @@ -489,6 +491,7 @@ int dm_mmc_get_wp(struct udevice *dev); int dm_mmc_execute_tuning(struct udevice *dev, uint opcode); int dm_mmc_wait_dat0(struct udevice *dev, int state, int timeout_us); int dm_mmc_host_power_cycle(struct udevice *dev); +int dm_mmc_deferred_probe(struct udevice *dev);

/* Transition functions for compatibility */ int mmc_set_ios(struct mmc *mmc); @@ -498,6 +501,7 @@ int mmc_execute_tuning(struct mmc *mmc, uint opcode); int mmc_wait_dat0(struct mmc *mmc, int state, int timeout_us); int mmc_set_enhanced_strobe(struct mmc *mmc); int mmc_host_power_cycle(struct mmc *mmc); +int mmc_deferred_probe(struct mmc *mmc);

#else struct mmc_ops { diff --git a/include/sdhci.h b/include/sdhci.h index 01addb7a60..1276f43935 100644 --- a/include/sdhci.h +++ b/include/sdhci.h @@ -267,6 +267,7 @@ struct sdhci_ops { void (*set_clock)(struct sdhci_host *host, u32 div); int (*platform_execute_tuning)(struct mmc *host, u8 opcode); void (*set_delay)(struct sdhci_host *host);

• int (*deferred_probe)(struct sdhci_host *host);

};

#if CONFIG_IS_ENABLED(MMC_SDHCI_ADMA)

...

This does look like the cleanest solution. Will add in a v3.

Above code is for just concept. Maybe you need to add/modify code.

Best Regards, Jaehoon Chung

Thanks, Faiz

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

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