diff mbox series

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

Message ID 1be44121-2a6e-e97b-c47b-332735f5c952@samsung.com
State New
Headers show
Series Add Support for eMMC boot in AM65x and J721e | expand

Commit Message

Jaehoon Chung Feb. 17, 2020, 11:24 p.m. UTC 
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 at 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() .. }



> 
> Best Regards,
> Jaehoon Chung
> 
>>
>> Thanks,
>> Faiz
>>
>>
> 
> 
>

Comments

Faiz Abbas Feb. 18, 2020, 7:51 a.m. UTC | #1 
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.

Thanks,
Faiz
Jaehoon Chung Feb. 18, 2020, 8:20 a.m. UTC | #2 
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
> 
> 
>
diff mbox series

Patch

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)