diff mbox

Warnings for invalid VDD (sdhci-s3c)

Message ID 56F3E9EB.3090207@intel.com
State New
Headers show

Commit Message

Adrian Hunter March 24, 2016, 1:21 p.m. UTC
On 24/03/16 15:11, Adrian Hunter wrote:
> On 24/03/16 10:42, Krzysztof Kozlowski wrote:
>> On 24.03.2016 17:24, Jisheng Zhang wrote:
>>> Hi,
>>>
>>> On Thu, 24 Mar 2016 17:09:27 +0900 Jaehoon Chung wrote:
>>>
>>>> Hi,
>>>>
>>>> On 03/24/2016 04:58 PM, Jisheng Zhang wrote:
>>>>> Hi,
>>>>>
>>>>> On Thu, 24 Mar 2016 16:28:56 +0900 Krzysztof Kozlowski wrote:
>>>>>   
>>>>>> Hi,
>>>>>>
>>>>>> After 918f4cbd4340 ("mmc: sdhci: restore behavior when setting VDD via
>>>>>> external regulator") On Trats2 board I see warnings for invalid VDD
>>>>>> value (2.8V):
>>>>>>
>>>>>> [    3.119656] ------------[ cut here ]------------
>>>>>> [    3.119666] WARNING: CPU: 3 PID: 90 at
>>>>>> ../drivers/mmc/host/sdhci.c:1234 sdhci_do_set_ios+0x4cc/0x5e0
>>>>>> [    3.119669] mmc0: Invalid vdd 0x10  
>>>>>
>>>>> Per my understanding, the wrong vdd indicates a wrong ocr, what's the voltage of
>>>>> this host's vmmc regulator?  
>>>>
>>>> As i know, it's fixed-voltage with gpio on trats2. It's 2.8V.
>>>> I didn't check this entirely..need to check ocr value.
>>>>
>>>
>>> I may know the reason. the vmmc is 2.8v, then mmc_regulator_get_supply() convert
>>> the value to a ocr as 0x10. The key here is that the 2.8v is invalid in SDHCI
>>> case and isn't accepted by current sdhci driver.
>>
>> Yeah, I already wrote that. It is the part of the warning and my email.
>> Our regulator is fixed at 2.8 which is 0x10. :)
>>
>>> I dunno the elegant solution to handle this case, let's wait for sdhci maintainers
>>> idea.
>>
>> Hmm...
> 
> I haven't tested it, but what about this:

And now with checkpatch complaints fixed:

From: Adrian Hunter <adrian.hunter@intel.com>
Date: Thu, 24 Mar 2016 14:29:24 +0200
Subject: [PATCH V2] mmc: sdhci: Fix regression setting power on Trats2 board

Several commits relating to setting power have been introducing
problems by putting driver-specific rules into generic SDHCI code.

Fix by adding a 'set_power' callback and restoring the default
behaviour prior to commit 918f4cbd4340 ("mmc: sdhci: restore
behavior when setting VDD via external regulator").  The desired
behaviour of that commit is gotten by having sdhci-pxav3 provide
its own set_power callback.

Reported-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Fixes: 918f4cbd4340 ("mmc: sdhci: restore behavior when setting VDD...)
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Cc: stable@vger.kernel.org # v4.5+
---
 drivers/mmc/host/sdhci-pxav3.c | 17 +++++++++++++++++
 drivers/mmc/host/sdhci.c       | 38 ++++++++++++++++++++++++++++++--------
 drivers/mmc/host/sdhci.h       |  4 ++++
 3 files changed, 51 insertions(+), 8 deletions(-)

Comments

Jisheng Zhang March 24, 2016, 1:45 p.m. UTC | #1
On Thu, 24 Mar 2016 15:21:47 +0200
Adrian Hunter <adrian.hunter@intel.com> wrote:

> On 24/03/16 15:11, Adrian Hunter wrote:
> > On 24/03/16 10:42, Krzysztof Kozlowski wrote:  
> >> On 24.03.2016 17:24, Jisheng Zhang wrote:  
> >>> Hi,
> >>>
> >>> On Thu, 24 Mar 2016 17:09:27 +0900 Jaehoon Chung wrote:
> >>>  
> >>>> Hi,
> >>>>
> >>>> On 03/24/2016 04:58 PM, Jisheng Zhang wrote:  
> >>>>> Hi,
> >>>>>
> >>>>> On Thu, 24 Mar 2016 16:28:56 +0900 Krzysztof Kozlowski wrote:
> >>>>>     
> >>>>>> Hi,
> >>>>>>
> >>>>>> After 918f4cbd4340 ("mmc: sdhci: restore behavior when setting VDD via
> >>>>>> external regulator") On Trats2 board I see warnings for invalid VDD
> >>>>>> value (2.8V):
> >>>>>>
> >>>>>> [    3.119656] ------------[ cut here ]------------
> >>>>>> [    3.119666] WARNING: CPU: 3 PID: 90 at
> >>>>>> ../drivers/mmc/host/sdhci.c:1234 sdhci_do_set_ios+0x4cc/0x5e0
> >>>>>> [    3.119669] mmc0: Invalid vdd 0x10    
> >>>>>
> >>>>> Per my understanding, the wrong vdd indicates a wrong ocr, what's the voltage of
> >>>>> this host's vmmc regulator?    
> >>>>
> >>>> As i know, it's fixed-voltage with gpio on trats2. It's 2.8V.
> >>>> I didn't check this entirely..need to check ocr value.
> >>>>  
> >>>
> >>> I may know the reason. the vmmc is 2.8v, then mmc_regulator_get_supply() convert
> >>> the value to a ocr as 0x10. The key here is that the 2.8v is invalid in SDHCI
> >>> case and isn't accepted by current sdhci driver.  
> >>
> >> Yeah, I already wrote that. It is the part of the warning and my email.
> >> Our regulator is fixed at 2.8 which is 0x10. :)
> >>  
> >>> I dunno the elegant solution to handle this case, let's wait for sdhci maintainers
> >>> idea.  
> >>
> >> Hmm...  
> > 
> > I haven't tested it, but what about this:  
> 
> And now with checkpatch complaints fixed:
> 
> From: Adrian Hunter <adrian.hunter@intel.com>
> Date: Thu, 24 Mar 2016 14:29:24 +0200
> Subject: [PATCH V2] mmc: sdhci: Fix regression setting power on Trats2 board
> 
> Several commits relating to setting power have been introducing
> problems by putting driver-specific rules into generic SDHCI code.
> 
> Fix by adding a 'set_power' callback and restoring the default
> behaviour prior to commit 918f4cbd4340 ("mmc: sdhci: restore
> behavior when setting VDD via external regulator").  The desired
> behaviour of that commit is gotten by having sdhci-pxav3 provide
> its own set_power callback.
> 
> Reported-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Fixes: 918f4cbd4340 ("mmc: sdhci: restore behavior when setting VDD...)
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>

Reviewed-by: Jisheng Zhang <jszhang@marvell.com>
Tested-by: Jisheng Zhang <jszhang@marvell.com>

The only problem is atmel's sdhc host also need similar own .set_power
Ludovic knows details.

Thanks,
Jisheng

> Cc: stable@vger.kernel.org # v4.5+
> ---
>  drivers/mmc/host/sdhci-pxav3.c | 17 +++++++++++++++++
>  drivers/mmc/host/sdhci.c       | 38 ++++++++++++++++++++++++++++++--------
>  drivers/mmc/host/sdhci.h       |  4 ++++
>  3 files changed, 51 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
> index f5edf9d3a18a..673d1e8446a5 100644
> --- a/drivers/mmc/host/sdhci-pxav3.c
> +++ b/drivers/mmc/host/sdhci-pxav3.c
> @@ -307,8 +307,25 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
>  		__func__, uhs, ctrl_2);
>  }
>  
> +static void pxav3_set_power(struct sdhci_host *host, unsigned char mode,
> +			    unsigned short vdd)
> +{
> +	struct mmc_host *mmc = host->mmc;
> +	u8 pwr = host->pwr;
> +
> +	sdhci_set_power(host, mode, vdd);
> +
> +	if (host->pwr == pwr)
> +		return;
> +
> +	spin_unlock_irq(&host->lock);
> +	mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
> +	spin_lock_irq(&host->lock);
> +}
> +
>  static const struct sdhci_ops pxav3_sdhci_ops = {
>  	.set_clock = sdhci_set_clock,
> +	.set_power = pxav3_set_power,
>  	.platform_send_init_74_clocks = pxav3_gen_init_74_clocks,
>  	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
>  	.set_bus_width = sdhci_set_bus_width,
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index add9fdfd1d8f..282257222121 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1269,10 +1269,24 @@ clock_set:
>  }
>  EXPORT_SYMBOL_GPL(sdhci_set_clock);
>  
> -static void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
> -			    unsigned short vdd)
> +static void sdhci_set_power_reg(struct sdhci_host *host, unsigned char mode,
> +				unsigned short vdd)
>  {
>  	struct mmc_host *mmc = host->mmc;
> +
> +	spin_unlock_irq(&host->lock);
> +	mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
> +	spin_lock_irq(&host->lock);
> +
> +	if (mode != MMC_POWER_OFF)
> +		sdhci_writeb(host, SDHCI_POWER_ON, SDHCI_POWER_CONTROL);
> +	else
> +		sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
> +}
> +
> +void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
> +		     unsigned short vdd)
> +{
>  	u8 pwr = 0;
>  
>  	if (mode != MMC_POWER_OFF) {
> @@ -1335,12 +1349,20 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
>  		if (host->quirks & SDHCI_QUIRK_DELAY_AFTER_POWER)
>  			mdelay(10);
>  	}
> +}
> +EXPORT_SYMBOL_GPL(sdhci_set_power);
>  
> -	if (!IS_ERR(mmc->supply.vmmc)) {
> -		spin_unlock_irq(&host->lock);
> -		mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
> -		spin_lock_irq(&host->lock);
> -	}
> +static void __sdhci_set_power(struct sdhci_host *host, unsigned char mode,
> +			      unsigned short vdd)
> +{
> +	struct mmc_host *mmc = host->mmc;
> +
> +	if (host->ops->set_power)
> +		host->ops->set_power(host, mode, vdd);
> +	else if (!IS_ERR(mmc->supply.vmmc))
> +		sdhci_set_power_reg(host, mode, vdd);
> +	else
> +		sdhci_set_power(host, mode, vdd);
>  }
>  
>  /*****************************************************************************\
> @@ -1490,7 +1512,7 @@ static void sdhci_do_set_ios(struct sdhci_host *host, struct mmc_ios *ios)
>  		}
>  	}
>  
> -	sdhci_set_power(host, ios->power_mode, ios->vdd);
> +	__sdhci_set_power(host, ios->power_mode, ios->vdd);
>  
>  	if (host->ops->platform_send_init_74_clocks)
>  		host->ops->platform_send_init_74_clocks(host, ios->power_mode);
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 0115e9907bf8..033d72b5bbd5 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -529,6 +529,8 @@ struct sdhci_ops {
>  #endif
>  
>  	void	(*set_clock)(struct sdhci_host *host, unsigned int clock);
> +	void	(*set_power)(struct sdhci_host *host, unsigned char mode,
> +			     unsigned short vdd);
>  
>  	int		(*enable_dma)(struct sdhci_host *host);
>  	unsigned int	(*get_max_clock)(struct sdhci_host *host);
> @@ -660,6 +662,8 @@ static inline bool sdhci_sdio_irq_enabled(struct sdhci_host *host)
>  }
>  
>  void sdhci_set_clock(struct sdhci_host *host, unsigned int clock);
> +void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
> +		     unsigned short vdd);
>  void sdhci_set_bus_width(struct sdhci_host *host, int width);
>  void sdhci_reset(struct sdhci_host *host, u8 mask);
>  void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing);

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ludovic Desroches March 24, 2016, 2:27 p.m. UTC | #2
On Thu, Mar 24, 2016 at 09:45:56PM +0800, Jisheng Zhang wrote:
> On Thu, 24 Mar 2016 15:21:47 +0200
> Adrian Hunter <adrian.hunter@intel.com> wrote:
> 
> > On 24/03/16 15:11, Adrian Hunter wrote:
> > > On 24/03/16 10:42, Krzysztof Kozlowski wrote:  
> > >> On 24.03.2016 17:24, Jisheng Zhang wrote:  
> > >>> Hi,
> > >>>
> > >>> On Thu, 24 Mar 2016 17:09:27 +0900 Jaehoon Chung wrote:
> > >>>  
> > >>>> Hi,
> > >>>>
> > >>>> On 03/24/2016 04:58 PM, Jisheng Zhang wrote:  
> > >>>>> Hi,
> > >>>>>
> > >>>>> On Thu, 24 Mar 2016 16:28:56 +0900 Krzysztof Kozlowski wrote:
> > >>>>>     
> > >>>>>> Hi,
> > >>>>>>
> > >>>>>> After 918f4cbd4340 ("mmc: sdhci: restore behavior when setting VDD via
> > >>>>>> external regulator") On Trats2 board I see warnings for invalid VDD
> > >>>>>> value (2.8V):
> > >>>>>>
> > >>>>>> [    3.119656] ------------[ cut here ]------------
> > >>>>>> [    3.119666] WARNING: CPU: 3 PID: 90 at
> > >>>>>> ../drivers/mmc/host/sdhci.c:1234 sdhci_do_set_ios+0x4cc/0x5e0
> > >>>>>> [    3.119669] mmc0: Invalid vdd 0x10    
> > >>>>>
> > >>>>> Per my understanding, the wrong vdd indicates a wrong ocr, what's the voltage of
> > >>>>> this host's vmmc regulator?    
> > >>>>
> > >>>> As i know, it's fixed-voltage with gpio on trats2. It's 2.8V.
> > >>>> I didn't check this entirely..need to check ocr value.
> > >>>>  
> > >>>
> > >>> I may know the reason. the vmmc is 2.8v, then mmc_regulator_get_supply() convert
> > >>> the value to a ocr as 0x10. The key here is that the 2.8v is invalid in SDHCI
> > >>> case and isn't accepted by current sdhci driver.  
> > >>
> > >> Yeah, I already wrote that. It is the part of the warning and my email.
> > >> Our regulator is fixed at 2.8 which is 0x10. :)
> > >>  
> > >>> I dunno the elegant solution to handle this case, let's wait for sdhci maintainers
> > >>> idea.  
> > >>
> > >> Hmm...  
> > > 
> > > I haven't tested it, but what about this:  
> > 
> > And now with checkpatch complaints fixed:
> > 
> > From: Adrian Hunter <adrian.hunter@intel.com>
> > Date: Thu, 24 Mar 2016 14:29:24 +0200
> > Subject: [PATCH V2] mmc: sdhci: Fix regression setting power on Trats2 board
> > 
> > Several commits relating to setting power have been introducing
> > problems by putting driver-specific rules into generic SDHCI code.
> > 
> > Fix by adding a 'set_power' callback and restoring the default
> > behaviour prior to commit 918f4cbd4340 ("mmc: sdhci: restore
> > behavior when setting VDD via external regulator").  The desired
> > behaviour of that commit is gotten by having sdhci-pxav3 provide
> > its own set_power callback.
> > 
> > Reported-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > Fixes: 918f4cbd4340 ("mmc: sdhci: restore behavior when setting VDD...)
> > Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> 
> Reviewed-by: Jisheng Zhang <jszhang@marvell.com>
> Tested-by: Jisheng Zhang <jszhang@marvell.com>
> 
> The only problem is atmel's sdhc host also need similar own .set_power
> Ludovic knows details.

It doesn't introduce regression on my side since I have removed
regulators from my device tree. If I introduce then back, I'll need my
own set_power callback.

I am not in favour of this patch only because I have the feeling that
sdhci_set_power_reg() is not conform to the specification.

Tested-by: Ludovic Desroches <ludovic.desroches@atmel.com>

> 
> Thanks,
> Jisheng
> 
> > Cc: stable@vger.kernel.org # v4.5+
> > ---
> >  drivers/mmc/host/sdhci-pxav3.c | 17 +++++++++++++++++
> >  drivers/mmc/host/sdhci.c       | 38 ++++++++++++++++++++++++++++++--------
> >  drivers/mmc/host/sdhci.h       |  4 ++++
> >  3 files changed, 51 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
> > index f5edf9d3a18a..673d1e8446a5 100644
> > --- a/drivers/mmc/host/sdhci-pxav3.c
> > +++ b/drivers/mmc/host/sdhci-pxav3.c
> > @@ -307,8 +307,25 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
> >  		__func__, uhs, ctrl_2);
> >  }
> >  
> > +static void pxav3_set_power(struct sdhci_host *host, unsigned char mode,
> > +			    unsigned short vdd)
> > +{
> > +	struct mmc_host *mmc = host->mmc;
> > +	u8 pwr = host->pwr;
> > +
> > +	sdhci_set_power(host, mode, vdd);
> > +
> > +	if (host->pwr == pwr)
> > +		return;
> > +
> > +	spin_unlock_irq(&host->lock);
> > +	mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
> > +	spin_lock_irq(&host->lock);
> > +}
> > +
> >  static const struct sdhci_ops pxav3_sdhci_ops = {
> >  	.set_clock = sdhci_set_clock,
> > +	.set_power = pxav3_set_power,
> >  	.platform_send_init_74_clocks = pxav3_gen_init_74_clocks,
> >  	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
> >  	.set_bus_width = sdhci_set_bus_width,
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > index add9fdfd1d8f..282257222121 100644
> > --- a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> > @@ -1269,10 +1269,24 @@ clock_set:
> >  }
> >  EXPORT_SYMBOL_GPL(sdhci_set_clock);
> >  
> > -static void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
> > -			    unsigned short vdd)
> > +static void sdhci_set_power_reg(struct sdhci_host *host, unsigned char mode,
> > +				unsigned short vdd)
> >  {
> >  	struct mmc_host *mmc = host->mmc;
> > +
> > +	spin_unlock_irq(&host->lock);
> > +	mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
> > +	spin_lock_irq(&host->lock);
> > +
> > +	if (mode != MMC_POWER_OFF)
> > +		sdhci_writeb(host, SDHCI_POWER_ON, SDHCI_POWER_CONTROL);
> > +	else
> > +		sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
> > +}
> > +
> > +void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
> > +		     unsigned short vdd)
> > +{
> >  	u8 pwr = 0;
> >  
> >  	if (mode != MMC_POWER_OFF) {
> > @@ -1335,12 +1349,20 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
> >  		if (host->quirks & SDHCI_QUIRK_DELAY_AFTER_POWER)
> >  			mdelay(10);
> >  	}
> > +}
> > +EXPORT_SYMBOL_GPL(sdhci_set_power);
> >  
> > -	if (!IS_ERR(mmc->supply.vmmc)) {
> > -		spin_unlock_irq(&host->lock);
> > -		mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
> > -		spin_lock_irq(&host->lock);
> > -	}
> > +static void __sdhci_set_power(struct sdhci_host *host, unsigned char mode,
> > +			      unsigned short vdd)
> > +{
> > +	struct mmc_host *mmc = host->mmc;
> > +
> > +	if (host->ops->set_power)
> > +		host->ops->set_power(host, mode, vdd);
> > +	else if (!IS_ERR(mmc->supply.vmmc))
> > +		sdhci_set_power_reg(host, mode, vdd);
> > +	else
> > +		sdhci_set_power(host, mode, vdd);
> >  }
> >  
> >  /*****************************************************************************\
> > @@ -1490,7 +1512,7 @@ static void sdhci_do_set_ios(struct sdhci_host *host, struct mmc_ios *ios)
> >  		}
> >  	}
> >  
> > -	sdhci_set_power(host, ios->power_mode, ios->vdd);
> > +	__sdhci_set_power(host, ios->power_mode, ios->vdd);
> >  
> >  	if (host->ops->platform_send_init_74_clocks)
> >  		host->ops->platform_send_init_74_clocks(host, ios->power_mode);
> > diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> > index 0115e9907bf8..033d72b5bbd5 100644
> > --- a/drivers/mmc/host/sdhci.h
> > +++ b/drivers/mmc/host/sdhci.h
> > @@ -529,6 +529,8 @@ struct sdhci_ops {
> >  #endif
> >  
> >  	void	(*set_clock)(struct sdhci_host *host, unsigned int clock);
> > +	void	(*set_power)(struct sdhci_host *host, unsigned char mode,
> > +			     unsigned short vdd);
> >  
> >  	int		(*enable_dma)(struct sdhci_host *host);
> >  	unsigned int	(*get_max_clock)(struct sdhci_host *host);
> > @@ -660,6 +662,8 @@ static inline bool sdhci_sdio_irq_enabled(struct sdhci_host *host)
> >  }
> >  
> >  void sdhci_set_clock(struct sdhci_host *host, unsigned int clock);
> > +void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
> > +		     unsigned short vdd);
> >  void sdhci_set_bus_width(struct sdhci_host *host, int width);
> >  void sdhci_reset(struct sdhci_host *host, u8 mask);
> >  void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing);
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Krzysztof Kozlowski March 28, 2016, 6:07 a.m. UTC | #3
On 24.03.2016 22:21, Adrian Hunter wrote:
> On 24/03/16 15:11, Adrian Hunter wrote:
>> On 24/03/16 10:42, Krzysztof Kozlowski wrote:
>>> On 24.03.2016 17:24, Jisheng Zhang wrote:
>>>> Hi,
>>>>
>>>> On Thu, 24 Mar 2016 17:09:27 +0900 Jaehoon Chung wrote:
>>>>
>>>>> Hi,
>>>>>
>>>>> On 03/24/2016 04:58 PM, Jisheng Zhang wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On Thu, 24 Mar 2016 16:28:56 +0900 Krzysztof Kozlowski wrote:
>>>>>>   
>>>>>>> Hi,
>>>>>>>
>>>>>>> After 918f4cbd4340 ("mmc: sdhci: restore behavior when setting VDD via
>>>>>>> external regulator") On Trats2 board I see warnings for invalid VDD
>>>>>>> value (2.8V):
>>>>>>>
>>>>>>> [    3.119656] ------------[ cut here ]------------
>>>>>>> [    3.119666] WARNING: CPU: 3 PID: 90 at
>>>>>>> ../drivers/mmc/host/sdhci.c:1234 sdhci_do_set_ios+0x4cc/0x5e0
>>>>>>> [    3.119669] mmc0: Invalid vdd 0x10  
>>>>>>
>>>>>> Per my understanding, the wrong vdd indicates a wrong ocr, what's the voltage of
>>>>>> this host's vmmc regulator?  
>>>>>
>>>>> As i know, it's fixed-voltage with gpio on trats2. It's 2.8V.
>>>>> I didn't check this entirely..need to check ocr value.
>>>>>
>>>>
>>>> I may know the reason. the vmmc is 2.8v, then mmc_regulator_get_supply() convert
>>>> the value to a ocr as 0x10. The key here is that the 2.8v is invalid in SDHCI
>>>> case and isn't accepted by current sdhci driver.
>>>
>>> Yeah, I already wrote that. It is the part of the warning and my email.
>>> Our regulator is fixed at 2.8 which is 0x10. :)
>>>
>>>> I dunno the elegant solution to handle this case, let's wait for sdhci maintainers
>>>> idea.
>>>
>>> Hmm...
>>
>> I haven't tested it, but what about this:
> 
> And now with checkpatch complaints fixed:
> 
> From: Adrian Hunter <adrian.hunter@intel.com>
> Date: Thu, 24 Mar 2016 14:29:24 +0200
> Subject: [PATCH V2] mmc: sdhci: Fix regression setting power on Trats2 board
> 
> Several commits relating to setting power have been introducing
> problems by putting driver-specific rules into generic SDHCI code.
> 
> Fix by adding a 'set_power' callback and restoring the default
> behaviour prior to commit 918f4cbd4340 ("mmc: sdhci: restore
> behavior when setting VDD via external regulator").  The desired

s/behavior/behaviour/

> behaviour of that commit is gotten by having sdhci-pxav3 provide
> its own set_power callback.
> 
> Reported-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Fixes: 918f4cbd4340 ("mmc: sdhci: restore behavior when setting VDD...)
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> Cc: stable@vger.kernel.org # v4.5+
> ---
>  drivers/mmc/host/sdhci-pxav3.c | 17 +++++++++++++++++
>  drivers/mmc/host/sdhci.c       | 38 ++++++++++++++++++++++++++++++--------
>  drivers/mmc/host/sdhci.h       |  4 ++++
>  3 files changed, 51 insertions(+), 8 deletions(-)
>

Works for me (sdhci-s3c, Exynos4412 on Trats2 board):
Tested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>


Best regards,
Krzysztof

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Adrian Hunter March 29, 2016, 9:39 a.m. UTC | #4
On 28/03/16 09:07, Krzysztof Kozlowski wrote:
> On 24.03.2016 22:21, Adrian Hunter wrote:
>> On 24/03/16 15:11, Adrian Hunter wrote:
>>> On 24/03/16 10:42, Krzysztof Kozlowski wrote:
>>>> On 24.03.2016 17:24, Jisheng Zhang wrote:
>>>>> Hi,
>>>>>
>>>>> On Thu, 24 Mar 2016 17:09:27 +0900 Jaehoon Chung wrote:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On 03/24/2016 04:58 PM, Jisheng Zhang wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On Thu, 24 Mar 2016 16:28:56 +0900 Krzysztof Kozlowski wrote:
>>>>>>>   
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> After 918f4cbd4340 ("mmc: sdhci: restore behavior when setting VDD via
>>>>>>>> external regulator") On Trats2 board I see warnings for invalid VDD
>>>>>>>> value (2.8V):
>>>>>>>>
>>>>>>>> [    3.119656] ------------[ cut here ]------------
>>>>>>>> [    3.119666] WARNING: CPU: 3 PID: 90 at
>>>>>>>> ../drivers/mmc/host/sdhci.c:1234 sdhci_do_set_ios+0x4cc/0x5e0
>>>>>>>> [    3.119669] mmc0: Invalid vdd 0x10  
>>>>>>>
>>>>>>> Per my understanding, the wrong vdd indicates a wrong ocr, what's the voltage of
>>>>>>> this host's vmmc regulator?  
>>>>>>
>>>>>> As i know, it's fixed-voltage with gpio on trats2. It's 2.8V.
>>>>>> I didn't check this entirely..need to check ocr value.
>>>>>>
>>>>>
>>>>> I may know the reason. the vmmc is 2.8v, then mmc_regulator_get_supply() convert
>>>>> the value to a ocr as 0x10. The key here is that the 2.8v is invalid in SDHCI
>>>>> case and isn't accepted by current sdhci driver.
>>>>
>>>> Yeah, I already wrote that. It is the part of the warning and my email.
>>>> Our regulator is fixed at 2.8 which is 0x10. :)
>>>>
>>>>> I dunno the elegant solution to handle this case, let's wait for sdhci maintainers
>>>>> idea.
>>>>
>>>> Hmm...
>>>
>>> I haven't tested it, but what about this:
>>
>> And now with checkpatch complaints fixed:
>>
>> From: Adrian Hunter <adrian.hunter@intel.com>
>> Date: Thu, 24 Mar 2016 14:29:24 +0200
>> Subject: [PATCH V2] mmc: sdhci: Fix regression setting power on Trats2 board
>>
>> Several commits relating to setting power have been introducing
>> problems by putting driver-specific rules into generic SDHCI code.
>>
>> Fix by adding a 'set_power' callback and restoring the default
>> behaviour prior to commit 918f4cbd4340 ("mmc: sdhci: restore
>> behavior when setting VDD via external regulator").  The desired
> 
> s/behavior/behaviour/

It seems checkpatch insists the commit message must be identical to the
original, so I am leaving it that way.

> 
>> behaviour of that commit is gotten by having sdhci-pxav3 provide
>> its own set_power callback.
>>
>> Reported-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>> Fixes: 918f4cbd4340 ("mmc: sdhci: restore behavior when setting VDD...)
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> Cc: stable@vger.kernel.org # v4.5+
>> ---
>>  drivers/mmc/host/sdhci-pxav3.c | 17 +++++++++++++++++
>>  drivers/mmc/host/sdhci.c       | 38 ++++++++++++++++++++++++++++++--------
>>  drivers/mmc/host/sdhci.h       |  4 ++++
>>  3 files changed, 51 insertions(+), 8 deletions(-)
>>
> 
> Works for me (sdhci-s3c, Exynos4412 on Trats2 board):
> Tested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

I made a V3 which has a couple of minor changes for pxav3_set_power()
and consequently have not added the Reviewed/Tested-by Jisheng Zhang.

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
index f5edf9d3a18a..673d1e8446a5 100644
--- a/drivers/mmc/host/sdhci-pxav3.c
+++ b/drivers/mmc/host/sdhci-pxav3.c
@@ -307,8 +307,25 @@  static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
 		__func__, uhs, ctrl_2);
 }
 
+static void pxav3_set_power(struct sdhci_host *host, unsigned char mode,
+			    unsigned short vdd)
+{
+	struct mmc_host *mmc = host->mmc;
+	u8 pwr = host->pwr;
+
+	sdhci_set_power(host, mode, vdd);
+
+	if (host->pwr == pwr)
+		return;
+
+	spin_unlock_irq(&host->lock);
+	mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
+	spin_lock_irq(&host->lock);
+}
+
 static const struct sdhci_ops pxav3_sdhci_ops = {
 	.set_clock = sdhci_set_clock,
+	.set_power = pxav3_set_power,
 	.platform_send_init_74_clocks = pxav3_gen_init_74_clocks,
 	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
 	.set_bus_width = sdhci_set_bus_width,
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index add9fdfd1d8f..282257222121 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1269,10 +1269,24 @@  clock_set:
 }
 EXPORT_SYMBOL_GPL(sdhci_set_clock);
 
-static void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
-			    unsigned short vdd)
+static void sdhci_set_power_reg(struct sdhci_host *host, unsigned char mode,
+				unsigned short vdd)
 {
 	struct mmc_host *mmc = host->mmc;
+
+	spin_unlock_irq(&host->lock);
+	mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
+	spin_lock_irq(&host->lock);
+
+	if (mode != MMC_POWER_OFF)
+		sdhci_writeb(host, SDHCI_POWER_ON, SDHCI_POWER_CONTROL);
+	else
+		sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
+}
+
+void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
+		     unsigned short vdd)
+{
 	u8 pwr = 0;
 
 	if (mode != MMC_POWER_OFF) {
@@ -1335,12 +1349,20 @@  static void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
 		if (host->quirks & SDHCI_QUIRK_DELAY_AFTER_POWER)
 			mdelay(10);
 	}
+}
+EXPORT_SYMBOL_GPL(sdhci_set_power);
 
-	if (!IS_ERR(mmc->supply.vmmc)) {
-		spin_unlock_irq(&host->lock);
-		mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
-		spin_lock_irq(&host->lock);
-	}
+static void __sdhci_set_power(struct sdhci_host *host, unsigned char mode,
+			      unsigned short vdd)
+{
+	struct mmc_host *mmc = host->mmc;
+
+	if (host->ops->set_power)
+		host->ops->set_power(host, mode, vdd);
+	else if (!IS_ERR(mmc->supply.vmmc))
+		sdhci_set_power_reg(host, mode, vdd);
+	else
+		sdhci_set_power(host, mode, vdd);
 }
 
 /*****************************************************************************\
@@ -1490,7 +1512,7 @@  static void sdhci_do_set_ios(struct sdhci_host *host, struct mmc_ios *ios)
 		}
 	}
 
-	sdhci_set_power(host, ios->power_mode, ios->vdd);
+	__sdhci_set_power(host, ios->power_mode, ios->vdd);
 
 	if (host->ops->platform_send_init_74_clocks)
 		host->ops->platform_send_init_74_clocks(host, ios->power_mode);
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 0115e9907bf8..033d72b5bbd5 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -529,6 +529,8 @@  struct sdhci_ops {
 #endif
 
 	void	(*set_clock)(struct sdhci_host *host, unsigned int clock);
+	void	(*set_power)(struct sdhci_host *host, unsigned char mode,
+			     unsigned short vdd);
 
 	int		(*enable_dma)(struct sdhci_host *host);
 	unsigned int	(*get_max_clock)(struct sdhci_host *host);
@@ -660,6 +662,8 @@  static inline bool sdhci_sdio_irq_enabled(struct sdhci_host *host)
 }
 
 void sdhci_set_clock(struct sdhci_host *host, unsigned int clock);
+void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
+		     unsigned short vdd);
 void sdhci_set_bus_width(struct sdhci_host *host, int width);
 void sdhci_reset(struct sdhci_host *host, u8 mask);
 void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing);