RE: [PATCH] mmc: rtsx: usb add 74 clocks in poweron flow

[Ricky WU]

> >
> > > >
> > > > 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.
> 

I think we got a little mixed up...
I think I need to separate this patch into 2 part
1) for 74 clocks issue 
2) add delay 100ms before rtsx power on

I think 1) is no problem for you? It is for SD spec and some cards check this. 
2) add 100ms before power on it is to make sure the last time power off did the discharge clean,
Because during the card recognition process, the card power will be toggled in 1ms，
and we found 1ms is not enough for discharge

if (1) and (2) are ok for you I will separate it In V2 1/2 and 2/2
or two unrelated patches (1) for 74 clock issue, (2) for make sure the voltage is below 0.5V before power on
which one you think is good?
 
Ricky

> 
> >
> > > >
> > > > Signed-off-by: Ricky Wu <ricky_wu@xxxxxxxxxxx>
> > > > ---
> > > >  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