[V3,7/7] mmc: core: Support UHS-II card access

From: Jason Lai <jason.lai@genesyslogic.com.tw>

Embed UHS-II access functionality into the MMC request processing flow.

Signed-off-by: Jason Lai <jason.lai@genesyslogic.com.tw>
---
 drivers/mmc/core/core.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

On Tue, 22 Feb 2022 at 04:40, Jason Lai <jasonlai.genesyslogic@gmail.com> wrote:
>
> From: Jason Lai <jason.lai@genesyslogic.com.tw>
>
> Embed UHS-II access functionality into the MMC request processing flow.
>
> Signed-off-by: Jason Lai <jason.lai@genesyslogic.com.tw>
> ---
>  drivers/mmc/core/core.c | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index fc3d8d61a97c..d2dcaa64bf27 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -31,6 +31,7 @@
>  #include <linux/mmc/mmc.h>
>  #include <linux/mmc/sd.h>
>  #include <linux/mmc/slot-gpio.h>
> +#include <linux/mmc/sd_uhs2.h>
>
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/mmc.h>
> @@ -42,6 +43,7 @@
>  #include "host.h"
>  #include "sdio_bus.h"
>  #include "pwrseq.h"
> +#include "sd_uhs2.h"
>
>  #include "mmc_ops.h"
>  #include "sd_ops.h"
> @@ -335,6 +337,8 @@ static int mmc_mrq_prep(struct mmc_host *host, struct mmc_request *mrq)
>
>  int mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
>  {
> +       struct uhs2_command uhs2_cmd;
> +       u32 payload[4]; /* for maximum size */
>         int err;
>
>         init_completion(&mrq->cmd_completion);
> @@ -352,6 +356,13 @@ int mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
>         if (err)
>                 return err;
>
> +       if (host->uhs2_caps.flags & MMC_UHS2_SUPPORT &&
> +           host->uhs2_caps.flags & MMC_UHS2_INITIALIZED) {
> +               uhs2_cmd.payload = payload;
> +               mrq->cmd->uhs2_cmd = &uhs2_cmd;
> +               sd_uhs2_prepare_cmd(host, mrq);
> +       }
> +
>         led_trigger_event(host->led, LED_FULL);
>         __mmc_start_request(host, mrq);
>
> @@ -431,6 +442,8 @@ EXPORT_SYMBOL(mmc_wait_for_req_done);
>   */
>  int mmc_cqe_start_req(struct mmc_host *host, struct mmc_request *mrq)
>  {
> +       struct uhs2_command uhs2_cmd;
> +       u32 payload[4]; /* for maximum size */
>         int err;
>
>         /*
> @@ -451,6 +464,13 @@ int mmc_cqe_start_req(struct mmc_host *host, struct mmc_request *mrq)
>         if (err)
>                 goto out_err;
>
> +       if (host->uhs2_caps.flags & MMC_UHS2_SUPPORT &&
> +           host->uhs2_caps.flags & MMC_UHS2_INITIALIZED) {
> +               uhs2_cmd.payload = payload;
> +               mrq->cmd->uhs2_cmd = &uhs2_cmd;
> +               sd_uhs2_prepare_cmd(host, mrq);
> +       }
> +
>         err = host->cqe_ops->cqe_request(host, mrq);
>         if (err)
>                 goto out_err;
> @@ -899,8 +919,10 @@ static inline void mmc_set_ios(struct mmc_host *host)
>   */
>  void mmc_set_chip_select(struct mmc_host *host, int mode)
>  {
> -       host->ios.chip_select = mode;
> -       mmc_set_ios(host);
> +       if (!(host->uhs2_caps.flags & MMC_UHS2_INITIALIZED)) {
> +               host->ios.chip_select = mode;
> +               mmc_set_ios(host);
> +       }

As I stated for patch6, rather than having these hacks spread out into
the legacy code, we must not allow the ->set_ios() ops to be invoked
when UHS-II is being used/initialized.

>  }
>

Moreover, I think $subject patch should be squashed into patch6. There
is really no reason to have them split up like this. Or is there?

I have now walked through the series - and it seems like there are a
few more pieces that we need to sort out, but it's certainly getting
better and better.

Kind regards
Uffe

Hi Uffe,

On Thu, Mar 24, 2022 at 12:23 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Tue, 22 Feb 2022 at 04:40, Jason Lai <jasonlai.genesyslogic@gmail.com> wrote:
> >
> > From: Jason Lai <jason.lai@genesyslogic.com.tw>
> >
> > Embed UHS-II access functionality into the MMC request processing flow.
> >
> > Signed-off-by: Jason Lai <jason.lai@genesyslogic.com.tw>
> > ---
> >  drivers/mmc/core/core.c | 26 ++++++++++++++++++++++++--
> >  1 file changed, 24 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > index fc3d8d61a97c..d2dcaa64bf27 100644
> > --- a/drivers/mmc/core/core.c
> > +++ b/drivers/mmc/core/core.c
> > @@ -31,6 +31,7 @@
> >  #include <linux/mmc/mmc.h>
> >  #include <linux/mmc/sd.h>
> >  #include <linux/mmc/slot-gpio.h>
> > +#include <linux/mmc/sd_uhs2.h>
> >
> >  #define CREATE_TRACE_POINTS
> >  #include <trace/events/mmc.h>
> > @@ -42,6 +43,7 @@
> >  #include "host.h"
> >  #include "sdio_bus.h"
> >  #include "pwrseq.h"
> > +#include "sd_uhs2.h"
> >
> >  #include "mmc_ops.h"
> >  #include "sd_ops.h"
> > @@ -335,6 +337,8 @@ static int mmc_mrq_prep(struct mmc_host *host, struct mmc_request *mrq)
> >
> >  int mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
> >  {
> > +       struct uhs2_command uhs2_cmd;
> > +       u32 payload[4]; /* for maximum size */
> >         int err;
> >
> >         init_completion(&mrq->cmd_completion);
> > @@ -352,6 +356,13 @@ int mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
> >         if (err)
> >                 return err;
> >
> > +       if (host->uhs2_caps.flags & MMC_UHS2_SUPPORT &&
> > +           host->uhs2_caps.flags & MMC_UHS2_INITIALIZED) {
> > +               uhs2_cmd.payload = payload;
> > +               mrq->cmd->uhs2_cmd = &uhs2_cmd;
> > +               sd_uhs2_prepare_cmd(host, mrq);
> > +       }
> > +
> >         led_trigger_event(host->led, LED_FULL);
> >         __mmc_start_request(host, mrq);
> >
> > @@ -431,6 +442,8 @@ EXPORT_SYMBOL(mmc_wait_for_req_done);
> >   */
> >  int mmc_cqe_start_req(struct mmc_host *host, struct mmc_request *mrq)
> >  {
> > +       struct uhs2_command uhs2_cmd;
> > +       u32 payload[4]; /* for maximum size */
> >         int err;
> >
> >         /*
> > @@ -451,6 +464,13 @@ int mmc_cqe_start_req(struct mmc_host *host, struct mmc_request *mrq)
> >         if (err)
> >                 goto out_err;
> >
> > +       if (host->uhs2_caps.flags & MMC_UHS2_SUPPORT &&
> > +           host->uhs2_caps.flags & MMC_UHS2_INITIALIZED) {
> > +               uhs2_cmd.payload = payload;
> > +               mrq->cmd->uhs2_cmd = &uhs2_cmd;
> > +               sd_uhs2_prepare_cmd(host, mrq);
> > +       }
> > +
> >         err = host->cqe_ops->cqe_request(host, mrq);
> >         if (err)
> >                 goto out_err;
> > @@ -899,8 +919,10 @@ static inline void mmc_set_ios(struct mmc_host *host)
> >   */
> >  void mmc_set_chip_select(struct mmc_host *host, int mode)
> >  {
> > -       host->ios.chip_select = mode;
> > -       mmc_set_ios(host);
> > +       if (!(host->uhs2_caps.flags & MMC_UHS2_INITIALIZED)) {
> > +               host->ios.chip_select = mode;
> > +               mmc_set_ios(host);
> > +       }
>
> As I stated for patch6, rather than having these hacks spread out into
> the legacy code, we must not allow the ->set_ios() ops to be invoked
> when UHS-II is being used/initialized.
>

After checking sd_uhs2.c, mmc_set_chip_select() is called in mmc_go_idle()
and mmc_go_idle() is called in sd_uhs2_legacy_init(). Hence, I issue CMD0
directly in sd_uhs2_legacy_init().

> >  }
> >
>
> Moreover, I think $subject patch should be squashed into patch6. There
> is really no reason to have them split up like this. Or is there?
>

OK.

Kind regards,
Jason


> I have now walked through the series - and it seems like there are a
> few more pieces that we need to sort out, but it's certainly getting
> better and better.
>
> Kind regards
> Uffe

On Thu, 7 Apr 2022 at 13:00, Lai Jason <jasonlai.genesyslogic@gmail.com> wrote:
>
> Hi Uffe,
>
> On Thu, Mar 24, 2022 at 12:23 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Tue, 22 Feb 2022 at 04:40, Jason Lai <jasonlai.genesyslogic@gmail.com> wrote:
> > >
> > > From: Jason Lai <jason.lai@genesyslogic.com.tw>
> > >
> > > Embed UHS-II access functionality into the MMC request processing flow.
> > >
> > > Signed-off-by: Jason Lai <jason.lai@genesyslogic.com.tw>
> > > ---
> > >  drivers/mmc/core/core.c | 26 ++++++++++++++++++++++++--
> > >  1 file changed, 24 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > > index fc3d8d61a97c..d2dcaa64bf27 100644
> > > --- a/drivers/mmc/core/core.c
> > > +++ b/drivers/mmc/core/core.c
> > > @@ -31,6 +31,7 @@
> > >  #include <linux/mmc/mmc.h>
> > >  #include <linux/mmc/sd.h>
> > >  #include <linux/mmc/slot-gpio.h>
> > > +#include <linux/mmc/sd_uhs2.h>
> > >
> > >  #define CREATE_TRACE_POINTS
> > >  #include <trace/events/mmc.h>
> > > @@ -42,6 +43,7 @@
> > >  #include "host.h"
> > >  #include "sdio_bus.h"
> > >  #include "pwrseq.h"
> > > +#include "sd_uhs2.h"
> > >
> > >  #include "mmc_ops.h"
> > >  #include "sd_ops.h"
> > > @@ -335,6 +337,8 @@ static int mmc_mrq_prep(struct mmc_host *host, struct mmc_request *mrq)
> > >
> > >  int mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
> > >  {
> > > +       struct uhs2_command uhs2_cmd;
> > > +       u32 payload[4]; /* for maximum size */
> > >         int err;
> > >
> > >         init_completion(&mrq->cmd_completion);
> > > @@ -352,6 +356,13 @@ int mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
> > >         if (err)
> > >                 return err;
> > >
> > > +       if (host->uhs2_caps.flags & MMC_UHS2_SUPPORT &&
> > > +           host->uhs2_caps.flags & MMC_UHS2_INITIALIZED) {
> > > +               uhs2_cmd.payload = payload;
> > > +               mrq->cmd->uhs2_cmd = &uhs2_cmd;
> > > +               sd_uhs2_prepare_cmd(host, mrq);
> > > +       }
> > > +
> > >         led_trigger_event(host->led, LED_FULL);
> > >         __mmc_start_request(host, mrq);
> > >
> > > @@ -431,6 +442,8 @@ EXPORT_SYMBOL(mmc_wait_for_req_done);
> > >   */
> > >  int mmc_cqe_start_req(struct mmc_host *host, struct mmc_request *mrq)
> > >  {
> > > +       struct uhs2_command uhs2_cmd;
> > > +       u32 payload[4]; /* for maximum size */
> > >         int err;
> > >
> > >         /*
> > > @@ -451,6 +464,13 @@ int mmc_cqe_start_req(struct mmc_host *host, struct mmc_request *mrq)
> > >         if (err)
> > >                 goto out_err;
> > >
> > > +       if (host->uhs2_caps.flags & MMC_UHS2_SUPPORT &&
> > > +           host->uhs2_caps.flags & MMC_UHS2_INITIALIZED) {
> > > +               uhs2_cmd.payload = payload;
> > > +               mrq->cmd->uhs2_cmd = &uhs2_cmd;
> > > +               sd_uhs2_prepare_cmd(host, mrq);
> > > +       }
> > > +
> > >         err = host->cqe_ops->cqe_request(host, mrq);
> > >         if (err)
> > >                 goto out_err;
> > > @@ -899,8 +919,10 @@ static inline void mmc_set_ios(struct mmc_host *host)
> > >   */
> > >  void mmc_set_chip_select(struct mmc_host *host, int mode)
> > >  {
> > > -       host->ios.chip_select = mode;
> > > -       mmc_set_ios(host);
> > > +       if (!(host->uhs2_caps.flags & MMC_UHS2_INITIALIZED)) {
> > > +               host->ios.chip_select = mode;
> > > +               mmc_set_ios(host);
> > > +       }
> >
> > As I stated for patch6, rather than having these hacks spread out into
> > the legacy code, we must not allow the ->set_ios() ops to be invoked
> > when UHS-II is being used/initialized.
> >
>
> After checking sd_uhs2.c, mmc_set_chip_select() is called in mmc_go_idle()
> and mmc_go_idle() is called in sd_uhs2_legacy_init(). Hence, I issue CMD0
> directly in sd_uhs2_legacy_init().

That works as a simple solution. But an even better option would be to
re-factor mmc_go_idle(), which then can avoid the open coding. Along
the lines like this:

1) Add a new function, _mmc_go_idle(struct mmc_host *host, bool
chip_select) - which is in principle the same as mmc_go_idle() as
today.
2) From mmc_go_idle() call _mmc_go_idle(host, true).
3) From uhs2 specific code, call  _mmc_go_idle(host, false).

[...]

Kind regards
Uffe