linux-kernel - Re: [PATCH] mmc: rtsx: usb add 74 clocks in poweron flow

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAPDyKFogPwixHGdmUy_z_udrUpU36mi_9cqdo1bPdM88OL1Erw@mail.gmail.com>
Date: Thu, 10 Apr 2025 15:44:42 +0200
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Ricky WU <ricky_wu@...ltek.com>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, 
	"linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>, 
	"viro@...iv.linux.org.uk" <viro@...iv.linux.org.uk>
Subject: Re: [PATCH] mmc: rtsx: usb add 74 clocks in poweron flow

On Thu, 10 Apr 2025 at 08:37, Ricky WU <ricky_wu@...ltek.com> wrote:
>
> > >
> > > SD spec definition:
> > > "Host provides at least 74 Clocks before issuing first command"
> > > After 1ms for the voltage stable then start issuing the Clock signals
> > >
> > > add if statement to issue/stop the clock signal to card:
> > > The power state from POWER_OFF to POWER_UP issue the signal to card,
> > > POWER_UP to POWER_ON stop the signal
> > >
> > > add 100ms delay in power_on to make sure the power cycle complete
> >
> > Why 100ms? That sounds a lot to me?
> >
> Hi Ulf,
>
> This 100ms delay is to ensure the voltage is below 0.5V before power on during a power cycle,
> The delays in the mmc core is not sufficient for the rtsx usb device.
> Because during the card recognition process, the card power will be toggled once in 1ms.
> If the card power is not fully discharged within that 1ms before being turned on again,
> It may affect the card recognition

Okay, I see. So 1ms isn't sufficient for your case.

Is there a regulator described somewhere? Could this delay be part of
the regulator enable/disable instead?

>
> > Is this fixing a real problem or is just trying to better follow the spec?
> >
>
> We found some cards not be recognized if not issue this 74 clocks

That still doesn't explain why you picked exactly 100ms as the delay.
Assuming we are running at lowest initialization frequency for SD/eMMC
at 100kHz, then 74 clocks are:

74/100000 = 0,00074s.

Kind regards
Uffe

>
> > >
> > > Signed-off-by: Ricky Wu <ricky_wu@...ltek.com>
> > > ---
> > >  drivers/mmc/host/rtsx_usb_sdmmc.c | 28
> > +++++++++++++++++++++++++---
> > >  1 file changed, 25 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/mmc/host/rtsx_usb_sdmmc.c
> > > b/drivers/mmc/host/rtsx_usb_sdmmc.c
> > > index d229c2b83ea9..e5820b2bb380 100644
> > > --- a/drivers/mmc/host/rtsx_usb_sdmmc.c
> > > +++ b/drivers/mmc/host/rtsx_usb_sdmmc.c
> > > @@ -48,7 +48,7 @@ struct rtsx_usb_sdmmc {
> > >         bool                    ddr_mode;
> > >
> > >         unsigned char           power_mode;
> > > -
> > > +       unsigned char           prev_power_mode;
> > >  #ifdef RTSX_USB_USE_LEDS_CLASS
> > >         struct led_classdev     led;
> > >         char                    led_name[32];
> > > @@ -952,6 +952,8 @@ static int sd_power_on(struct rtsx_usb_sdmmc
> > *host)
> > >         struct rtsx_ucr *ucr = host->ucr;
> > >         int err;
> > >
> > > +       msleep(100);
> > > +
> > >         dev_dbg(sdmmc_dev(host), "%s\n", __func__);
> > >         rtsx_usb_init_cmd(ucr);
> > >         rtsx_usb_add_cmd(ucr, WRITE_REG_CMD, CARD_SELECT, 0x07,
> > > SD_MOD_SEL); @@ -1014,6 +1016,16 @@ static int
> > sd_set_power_mode(struct rtsx_usb_sdmmc *host,
> > >                 unsigned char power_mode)  {
> > >         int err;
> > > +       int power_mode_temp;
> > > +       struct rtsx_ucr *ucr = host->ucr;
> > > +
> > > +       power_mode_temp = power_mode;
> > > +
> > > +       if ((power_mode == MMC_POWER_ON) && (host->power_mode
> > == MMC_POWER_ON) &&
> > > +                       (host->prev_power_mode ==
> > MMC_POWER_UP)) {
> > > +               host->prev_power_mode = MMC_POWER_ON;
> > > +               rtsx_usb_write_register(ucr, SD_BUS_STAT,
> > SD_CLK_TOGGLE_EN, 0x00);
> > > +       }
> > >
> > >         if (power_mode != MMC_POWER_OFF)
> > >                 power_mode = MMC_POWER_ON; @@ -1029,9
> > +1041,18 @@
> > > static int sd_set_power_mode(struct rtsx_usb_sdmmc *host,
> > >                 err = sd_power_on(host);
> > >         }
> > >
> > > -       if (!err)
> > > -               host->power_mode = power_mode;
> > > +       if (!err) {
> > > +               if ((power_mode_temp == MMC_POWER_UP) &&
> > (host->power_mode == MMC_POWER_OFF)) {
> > > +                       host->prev_power_mode = MMC_POWER_UP;
> > > +                       rtsx_usb_write_register(ucr, SD_BUS_STAT,
> > SD_CLK_TOGGLE_EN,
> > > +                                       SD_CLK_TOGGLE_EN);
> > > +               }
> > > +
> > > +               if ((power_mode_temp == MMC_POWER_OFF) &&
> > (host->power_mode == MMC_POWER_ON))
> > > +                       host->prev_power_mode = MMC_POWER_OFF;
> > >
> > > +               host->power_mode = power_mode;
> > > +       }
> > >         return err;
> > >  }
> > >
> > > @@ -1316,6 +1337,7 @@ static void rtsx_usb_init_host(struct
> > rtsx_usb_sdmmc *host)
> > >         mmc->max_req_size = 524288;
> > >
> > >         host->power_mode = MMC_POWER_OFF;
> > > +       host->prev_power_mode = MMC_POWER_OFF;
> > >  }
> > >
> > >  static int rtsx_usb_sdmmc_drv_probe(struct platform_device *pdev)
> > > --
> > > 2.25.1
> > >
> >
> > Kind regards
> > Uffe