[PATCH 1/7] mmc: sdhci: Return error in case of failure

[Jaehoon Chung]

Hi Ashok,

On 7/26/21 2:33 PM, Ashok Reddy Soma wrote:
> Hi Jaehoon,
> 
> Thanks for the review.
> 
>> -----Original Message-----
>> From: Jaehoon Chung <jh80.chung at samsung.com>
>> Sent: Monday, July 26, 2021 3:18 AM
>> To: Ashok Reddy Soma <ashokred at xilinx.com>; u-boot at lists.denx.de
>> Cc: peng.fan at nxp.com; faiz_abbas at ti.com; sjg at chromium.org;
>> michael at walle.cc; git <git at xilinx.com>; monstr at monstr.eu;
>> somaashokreddy at gmail.com; T Karthik Reddy <tkarthik at xilinx.com>
>> Subject: Re: [PATCH 1/7] mmc: sdhci: Return error in case of failure
>>
>> Hi Ashok,
>>
>> On 7/24/21 5:10 PM, Ashok Reddy Soma wrote:
>>> From: T Karthik Reddy <t.karthik.reddy at xilinx.com>
>>>
>>> set_delay() function is from sdhci host ops, which does not return any
>>> error due to void return type. Get return values from input and output
>>> set clock phase functions inside arasan_sdhci_set_tapdelay() and
>>> return the errors.
>>>
>>> Change return type to int for arasan_sdhci_set_tapdelay() and also for
>>> set_delay() in sdhci_ops structure.
>>
>> Could you separate the patch to sdhci and zync_sdhci part?
> Sure, i will split in to two patches.
>>
>>>
>>> Signed-off-by: T Karthik Reddy <t.karthik.reddy at xilinx.com>
>>> Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma at xilinx.com>
>>> ---
>>>
>>>  drivers/mmc/sdhci.c      |  8 ++++++--
>>>  drivers/mmc/zynq_sdhci.c | 21 ++++++++++++++++-----
>>>  include/sdhci.h          |  2 +-
>>>  3 files changed, 23 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c index
>>> d9ab6a0a83..f144602eec 100644
>>> --- a/drivers/mmc/sdhci.c
>>> +++ b/drivers/mmc/sdhci.c
>>> @@ -366,6 +366,7 @@ int sdhci_set_clock(struct mmc *mmc, unsigned int
>>> clock)  {
>>>  	struct sdhci_host *host = mmc->priv;
>>>  	unsigned int div, clk = 0, timeout;
>>> +	int ret;
>>>
>>>  	/* Wait max 20 ms */
>>>  	timeout = 200;
>>> @@ -386,8 +387,11 @@ int sdhci_set_clock(struct mmc *mmc, unsigned int
>> clock)
>>>  	if (clock == 0)
>>>  		return 0;
>>>
>>> -	if (host->ops && host->ops->set_delay)
>>> -		host->ops->set_delay(host);
>>> +	if (host->ops && host->ops->set_delay) {
>>> +		ret = host->ops->set_delay(host);
>>> +		if (ret)
>>> +			return ret;
>>
>> how about adding debug(). It's helpful to debug when it's failed.
> 
> Ok, I will add a debug print here.
> 
> Any comments for other patches in this series or shall I send V2 with these changes ?

I didn't see other patch yet. Sorry.
After checked other patch, I will reply ASAP.

Best Regards,
Jaehoon Chung

> 
> Thanks,
> Ashok
>>
>> Best Regards,
>> Jaehoon Chung
>>
>>> +	}
>>>
>>>  	if (SDHCI_GET_VERSION(host) >= SDHCI_SPEC_300) {
>>>  		/*
>>> diff --git a/drivers/mmc/zynq_sdhci.c b/drivers/mmc/zynq_sdhci.c index
>>> ba87ee8dd5..9fb3603c7e 100644
>>> --- a/drivers/mmc/zynq_sdhci.c
>>> +++ b/drivers/mmc/zynq_sdhci.c
>>> @@ -422,7 +422,7 @@ static int sdhci_versal_sampleclk_set_phase(struct
>> sdhci_host *host,
>>>  	return 0;
>>>  }
>>>
>>> -static void arasan_sdhci_set_tapdelay(struct sdhci_host *host)
>>> +static int arasan_sdhci_set_tapdelay(struct sdhci_host *host)
>>>  {
>>>  	struct arasan_sdhci_priv *priv = dev_get_priv(host->mmc->dev);
>>>  	struct arasan_sdhci_clk_data *clk_data = &priv->clk_data; @@ -431,18
>>> +431,29 @@ static void arasan_sdhci_set_tapdelay(struct sdhci_host *host)
>>>  	u8 timing = mode2timing[mmc->selected_mode];
>>>  	u32 iclk_phase = clk_data->clk_phase_in[timing];
>>>  	u32 oclk_phase = clk_data->clk_phase_out[timing];
>>> +	int ret;
>>>
>>>  	dev_dbg(dev, "%s, host:%s, mode:%d\n", __func__, host->name,
>>> timing);
>>>
>>>  	if (IS_ENABLED(CONFIG_ARCH_ZYNQMP) &&
>>>  	    device_is_compatible(dev, "xlnx,zynqmp-8.9a")) {
>>> -		sdhci_zynqmp_sampleclk_set_phase(host, iclk_phase);
>>> -		sdhci_zynqmp_sdcardclk_set_phase(host, oclk_phase);
>>> +		ret = sdhci_zynqmp_sampleclk_set_phase(host, iclk_phase);
>>> +		if (ret)
>>> +			return ret;
>>> +		ret = sdhci_zynqmp_sdcardclk_set_phase(host, oclk_phase);
>>> +		if (ret)
>>> +			return ret;
>>>  	} else if (IS_ENABLED(CONFIG_ARCH_VERSAL) &&
>>>  		   device_is_compatible(dev, "xlnx,versal-8.9a")) {
>>> -		sdhci_versal_sampleclk_set_phase(host, iclk_phase);
>>> -		sdhci_versal_sdcardclk_set_phase(host, oclk_phase);
>>> +		ret = sdhci_versal_sampleclk_set_phase(host, iclk_phase);
>>> +		if (ret)
>>> +			return ret;
>>> +		ret = sdhci_versal_sdcardclk_set_phase(host, oclk_phase);
>>> +		if (ret)
>>> +			return ret;
>>>  	}
>>> +
>>> +	return 0;
>>>  }
>>>
>>>  static void arasan_dt_read_clk_phase(struct udevice *dev, unsigned
>>> char timing, diff --git a/include/sdhci.h b/include/sdhci.h index
>>> 0ae9471ad7..44a0d84e5a 100644
>>> --- a/include/sdhci.h
>>> +++ b/include/sdhci.h
>>> @@ -268,7 +268,7 @@ struct sdhci_ops {
>>>  	int	(*set_ios_post)(struct sdhci_host *host);
>>>  	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 (*set_delay)(struct sdhci_host *host);
>>>  	int	(*deferred_probe)(struct sdhci_host *host);
>>>  };
>>>
>>>
>