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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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