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

[Ashok Reddy Soma <ashokred@xilinx.com>]

Hi Jaehoon,

Thanks for the review.

> -----Original Message-----
> From: Jaehoon Chung <jh80.chung@samsung.com>
> Sent: Monday, July 26, 2021 3:18 AM
> To: Ashok Reddy Soma <ashokred@xilinx.com>; u-boot@lists.denx.de
> Cc: peng.fan@nxp.com; faiz_abbas@ti.com; sjg@chromium.org;
> michael@walle.cc; git <git@xilinx.com>; monstr@monstr.eu;
> somaashokreddy@gmail.com; T Karthik Reddy <tkarthik@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@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@xilinx.com>
> > Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@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 ?

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);
> >  };
> >
> >