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