diff mbox series

[<PATCH,v1>,7/9] mmc: core: Skip frequency retries for SDCC slots

Message ID 68b936a67e67cd25597915cbd76b73ed046d1ce4.1576540908.git.nguyenb@codeaurora.org (mailing list archive)
State New, archived
Headers show
Series SD card bug fixes | expand

Commit Message

Bao D. Nguyen Dec. 17, 2019, 2:50 a.m. UTC 
From: Sujith Reddy Thumma <sthumma@codeaurora.org>

Qualcomm SDCC controller supports minimum SD clock frequency
which is required for card initialization. This information is
exported through platform data for each SDCC controller. There is
no need of retrying higher frequencies than the minimum supported
by controller for Qualcomm chipsets which inturn add delay in
detection process if there is no card during suspend/resume cycles.
Hence, skip multiple frequency retries.

Signed-off-by: Sujith Reddy Thumma <sthumma@codeaurora.org>
Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
Signed-off-by: Xiaonian Wang <xiaonian@codeaurora.org>
Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
---
 drivers/mmc/core/core.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

Comments

Greg Kroah-Hartman Dec. 18, 2019, 8:34 a.m. UTC | #1 
On Mon, Dec 16, 2019 at 06:50:40PM -0800, Bao D. Nguyen wrote:
> From: Sujith Reddy Thumma <sthumma@codeaurora.org>
> 
> Qualcomm SDCC controller supports minimum SD clock frequency
> which is required for card initialization. This information is
> exported through platform data for each SDCC controller. There is
> no need of retrying higher frequencies than the minimum supported
> by controller for Qualcomm chipsets which inturn add delay in
> detection process if there is no card during suspend/resume cycles.
> Hence, skip multiple frequency retries.
> 
> Signed-off-by: Sujith Reddy Thumma <sthumma@codeaurora.org>
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> Signed-off-by: Xiaonian Wang <xiaonian@codeaurora.org>
> Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
> ---
>  drivers/mmc/core/core.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 1e37f78..38b0cec 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -2281,7 +2281,6 @@ void mmc_rescan(struct work_struct *work)
>  {
>  	struct mmc_host *host =
>  		container_of(work, struct mmc_host, detect.work);
> -	int i;
>  
>  	if (host->rescan_disable)
>  		return;
> @@ -2332,13 +2331,7 @@ void mmc_rescan(struct work_struct *work)
>  		mmc_release_host(host);
>  		goto out;
>  	}
> -
> -	for (i = 0; i < ARRAY_SIZE(freqs); i++) {
> -		if (!mmc_rescan_try_freq(host, max(freqs[i], host->f_min)))
> -			break;
> -		if (freqs[i] <= host->f_min)
> -			break;
> -	}
> +	mmc_rescan_try_freq(host, host->f_min);

What about for non-qualcomm controllers?  Did this just break their
functionality?

thanks,

greg k-h
Ulf Hansson Dec. 18, 2019, 11:48 a.m. UTC | #2 
On Wed, 18 Dec 2019 at 09:34, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Dec 16, 2019 at 06:50:40PM -0800, Bao D. Nguyen wrote:
> > From: Sujith Reddy Thumma <sthumma@codeaurora.org>
> >
> > Qualcomm SDCC controller supports minimum SD clock frequency
> > which is required for card initialization. This information is
> > exported through platform data for each SDCC controller. There is
> > no need of retrying higher frequencies than the minimum supported
> > by controller for Qualcomm chipsets which inturn add delay in
> > detection process if there is no card during suspend/resume cycles.
> > Hence, skip multiple frequency retries.
> >
> > Signed-off-by: Sujith Reddy Thumma <sthumma@codeaurora.org>
> > Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> > Signed-off-by: Xiaonian Wang <xiaonian@codeaurora.org>
> > Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
> > ---
> >  drivers/mmc/core/core.c | 9 +--------
> >  1 file changed, 1 insertion(+), 8 deletions(-)
> >
> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > index 1e37f78..38b0cec 100644
> > --- a/drivers/mmc/core/core.c
> > +++ b/drivers/mmc/core/core.c
> > @@ -2281,7 +2281,6 @@ void mmc_rescan(struct work_struct *work)
> >  {
> >       struct mmc_host *host =
> >               container_of(work, struct mmc_host, detect.work);
> > -     int i;
> >
> >       if (host->rescan_disable)
> >               return;
> > @@ -2332,13 +2331,7 @@ void mmc_rescan(struct work_struct *work)
> >               mmc_release_host(host);
> >               goto out;
> >       }
> > -
> > -     for (i = 0; i < ARRAY_SIZE(freqs); i++) {
> > -             if (!mmc_rescan_try_freq(host, max(freqs[i], host->f_min)))
> > -                     break;
> > -             if (freqs[i] <= host->f_min)
> > -                     break;
> > -     }
> > +     mmc_rescan_try_freq(host, host->f_min);
>
> What about for non-qualcomm controllers?  Did this just break their
> functionality?

Yes it does, obviously.

Greg, thanks for providing some valuable feedback for Bao for a couple
of the patches in this series.

I have also browsed through the series, but stopped providing feedback
after patch1, when I realized that these are all just downstream
vendor specific hacks.

Sure, I guess most of the patches can be reworked as upstreamable
solutions, but rather than looking at them all at once, please post
them separately. Additionally, in general I would appreciate more
details in the changelogs to fully understand the problems your are
solving.

Kind regards
Uffe
Greg Kroah-Hartman Dec. 18, 2019, 12:04 p.m. UTC | #3 
On Wed, Dec 18, 2019 at 12:48:20PM +0100, Ulf Hansson wrote:
> On Wed, 18 Dec 2019 at 09:34, Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Mon, Dec 16, 2019 at 06:50:40PM -0800, Bao D. Nguyen wrote:
> > > From: Sujith Reddy Thumma <sthumma@codeaurora.org>
> > >
> > > Qualcomm SDCC controller supports minimum SD clock frequency
> > > which is required for card initialization. This information is
> > > exported through platform data for each SDCC controller. There is
> > > no need of retrying higher frequencies than the minimum supported
> > > by controller for Qualcomm chipsets which inturn add delay in
> > > detection process if there is no card during suspend/resume cycles.
> > > Hence, skip multiple frequency retries.
> > >
> > > Signed-off-by: Sujith Reddy Thumma <sthumma@codeaurora.org>
> > > Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> > > Signed-off-by: Xiaonian Wang <xiaonian@codeaurora.org>
> > > Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
> > > ---
> > >  drivers/mmc/core/core.c | 9 +--------
> > >  1 file changed, 1 insertion(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > > index 1e37f78..38b0cec 100644
> > > --- a/drivers/mmc/core/core.c
> > > +++ b/drivers/mmc/core/core.c
> > > @@ -2281,7 +2281,6 @@ void mmc_rescan(struct work_struct *work)
> > >  {
> > >       struct mmc_host *host =
> > >               container_of(work, struct mmc_host, detect.work);
> > > -     int i;
> > >
> > >       if (host->rescan_disable)
> > >               return;
> > > @@ -2332,13 +2331,7 @@ void mmc_rescan(struct work_struct *work)
> > >               mmc_release_host(host);
> > >               goto out;
> > >       }
> > > -
> > > -     for (i = 0; i < ARRAY_SIZE(freqs); i++) {
> > > -             if (!mmc_rescan_try_freq(host, max(freqs[i], host->f_min)))
> > > -                     break;
> > > -             if (freqs[i] <= host->f_min)
> > > -                     break;
> > > -     }
> > > +     mmc_rescan_try_freq(host, host->f_min);
> >
> > What about for non-qualcomm controllers?  Did this just break their
> > functionality?
> 
> Yes it does, obviously.
> 
> Greg, thanks for providing some valuable feedback for Bao for a couple
> of the patches in this series.
> 
> I have also browsed through the series, but stopped providing feedback
> after patch1, when I realized that these are all just downstream
> vendor specific hacks.

That's all kernel drivers are, vendor-specific quirks/hacks around
broken hardware :)

Splitting this out into logical fixes, like some of these are here, is
great.  But breaking non-qualcomm hardware like this patch would do, is
obviously not ok.

> Sure, I guess most of the patches can be reworked as upstreamable
> solutions,

They have to be upstreamable, you don't want these in random vendor
trees as they go no where and atrophy and break users.  We want them in
our main tree for everyone to use for the obvious reason that they are
needed to get real hardware working.

thanks,

greg k-h
Ulf Hansson Dec. 18, 2019, 1:12 p.m. UTC | #4 
On Wed, 18 Dec 2019 at 13:04, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Dec 18, 2019 at 12:48:20PM +0100, Ulf Hansson wrote:
> > On Wed, 18 Dec 2019 at 09:34, Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Mon, Dec 16, 2019 at 06:50:40PM -0800, Bao D. Nguyen wrote:
> > > > From: Sujith Reddy Thumma <sthumma@codeaurora.org>
> > > >
> > > > Qualcomm SDCC controller supports minimum SD clock frequency
> > > > which is required for card initialization. This information is
> > > > exported through platform data for each SDCC controller. There is
> > > > no need of retrying higher frequencies than the minimum supported
> > > > by controller for Qualcomm chipsets which inturn add delay in
> > > > detection process if there is no card during suspend/resume cycles.
> > > > Hence, skip multiple frequency retries.
> > > >
> > > > Signed-off-by: Sujith Reddy Thumma <sthumma@codeaurora.org>
> > > > Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> > > > Signed-off-by: Xiaonian Wang <xiaonian@codeaurora.org>
> > > > Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
> > > > ---
> > > >  drivers/mmc/core/core.c | 9 +--------
> > > >  1 file changed, 1 insertion(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > > > index 1e37f78..38b0cec 100644
> > > > --- a/drivers/mmc/core/core.c
> > > > +++ b/drivers/mmc/core/core.c
> > > > @@ -2281,7 +2281,6 @@ void mmc_rescan(struct work_struct *work)
> > > >  {
> > > >       struct mmc_host *host =
> > > >               container_of(work, struct mmc_host, detect.work);
> > > > -     int i;
> > > >
> > > >       if (host->rescan_disable)
> > > >               return;
> > > > @@ -2332,13 +2331,7 @@ void mmc_rescan(struct work_struct *work)
> > > >               mmc_release_host(host);
> > > >               goto out;
> > > >       }
> > > > -
> > > > -     for (i = 0; i < ARRAY_SIZE(freqs); i++) {
> > > > -             if (!mmc_rescan_try_freq(host, max(freqs[i], host->f_min)))
> > > > -                     break;
> > > > -             if (freqs[i] <= host->f_min)
> > > > -                     break;
> > > > -     }
> > > > +     mmc_rescan_try_freq(host, host->f_min);
> > >
> > > What about for non-qualcomm controllers?  Did this just break their
> > > functionality?
> >
> > Yes it does, obviously.
> >
> > Greg, thanks for providing some valuable feedback for Bao for a couple
> > of the patches in this series.
> >
> > I have also browsed through the series, but stopped providing feedback
> > after patch1, when I realized that these are all just downstream
> > vendor specific hacks.
>
> That's all kernel drivers are, vendor-specific quirks/hacks around
> broken hardware :)
>
> Splitting this out into logical fixes, like some of these are here, is
> great.  But breaking non-qualcomm hardware like this patch would do, is
> obviously not ok.
>
> > Sure, I guess most of the patches can be reworked as upstreamable
> > solutions,
>
> They have to be upstreamable, you don't want these in random vendor
> trees as they go no where and atrophy and break users.  We want them in
> our main tree for everyone to use for the obvious reason that they are
> needed to get real hardware working.

I fully agree and I am willing to help!

Just didn't want to waste my time reviewing all at once in great
detail - if it turns out that the submitter don't care to re-spin, as
that has happened before (not by Bao).

Kind regards
Uffe
diff mbox series

Patch

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 1e37f78..38b0cec 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2281,7 +2281,6 @@  void mmc_rescan(struct work_struct *work)
 {
 	struct mmc_host *host =
 		container_of(work, struct mmc_host, detect.work);
-	int i;
 
 	if (host->rescan_disable)
 		return;
@@ -2332,13 +2331,7 @@  void mmc_rescan(struct work_struct *work)
 		mmc_release_host(host);
 		goto out;
 	}
-
-	for (i = 0; i < ARRAY_SIZE(freqs); i++) {
-		if (!mmc_rescan_try_freq(host, max(freqs[i], host->f_min)))
-			break;
-		if (freqs[i] <= host->f_min)
-			break;
-	}
+	mmc_rescan_try_freq(host, host->f_min);
 	mmc_release_host(host);
 
  out: