| Message ID | 1494613000-8156-15-git-send-email-jjhiblot@ti.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Jaehoon Chung |
| Headers | show |
Hi, On 12 May 2017 at 12:16, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote: > mmc/sd specification requires vdd to be disabled for 1 ms > and then enabled again during power cycle. Add a > function in mmc core to perform power cycle and set > the io signal to it's initial state. > > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> > --- > drivers/mmc/mmc.c | 50 +++++++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 41 insertions(+), 9 deletions(-) > > diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c > index d40a22b..032260b 100644 > --- a/drivers/mmc/mmc.c > +++ b/drivers/mmc/mmc.c > @@ -30,6 +30,7 @@ static const unsigned int sd_au_size[] = { > SZ_16M / 512, (SZ_16M + SZ_8M) / 512, SZ_32M / 512, SZ_64M / 512, > }; > static int mmc_set_signal_voltage(struct mmc *mmc, uint signal_voltage); > +static void mmc_power_cycle(struct mmc *mmc); > > #if CONFIG_IS_ENABLED(MMC_TINY) > static struct mmc mmc_static; > @@ -1915,6 +1916,45 @@ static int mmc_power_init(struct mmc *mmc) > return 0; > } > > +static void mmc_set_initial_state(struct mmc *mmc) Function comment > +{ > + int err; > + > + /* First try to set 3.3V. If it fails set to 1.8V */ > + err = mmc_set_signal_voltage(mmc, MMC_SIGNAL_VOLTAGE_330); > + if (err != 0) > + err = mmc_set_signal_voltage(mmc, MMC_SIGNAL_VOLTAGE_180); > + if (err != 0) > + printf("failed to set signal voltage\n"); > + > + mmc_set_bus_width(mmc, 1); > + mmc_set_clock(mmc, 1); > + mmc_select_mode(mmc, MMC_LEGACY); > +} > + > +static void mmc_power_up(struct mmc *mmc) > +{ > + mmc_set_initial_state(mmc); > + mmc_set_vdd(mmc, true); > + udelay(10000); Eek. Please add a comment as to why > +} > + > +static void mmc_power_off(struct mmc *mmc) > +{ > + mmc_set_vdd(mmc, false); > +} > + > +static void mmc_power_cycle(struct mmc *mmc) > +{ > + mmc_power_off(mmc); > + /* > + * SD spec recommends at least 1ms of delay. Let's wait for 2ms > + * to be on the safer side. > + */ > + udelay(2000); > + mmc_power_up(mmc); > +} > + > int mmc_start_init(struct mmc *mmc) > { > bool no_card; > @@ -1952,16 +1992,8 @@ int mmc_start_init(struct mmc *mmc) > return err; > #endif > mmc->ddr_mode = 0; > - mmc_set_vdd(mmc, true); > - /* First try to set 3.3V. If it fails set to 1.8V */ > - err = mmc_set_signal_voltage(mmc, MMC_SIGNAL_VOLTAGE_330); > - if (err != 0) > - err = mmc_set_signal_voltage(mmc, MMC_SIGNAL_VOLTAGE_180); > - if (err != 0) > - printf("failed to set signal voltage\n"); Return error? Also please add some mmc: to your message. > > - mmc_set_bus_width(mmc, 1); > - mmc_set_clock(mmc, 1); > + mmc_power_cycle(mmc); > > /* Reset the Card */ > err = mmc_go_idle(mmc); > -- > 1.9.1 > Regards, Simon
On 15/05/2017 05:28, Simon Glass wrote: > Hi, > > On 12 May 2017 at 12:16, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote: >> mmc/sd specification requires vdd to be disabled for 1 ms >> and then enabled again during power cycle. Add a >> function in mmc core to perform power cycle and set >> the io signal to it's initial state. >> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> >> --- >> drivers/mmc/mmc.c | 50 +++++++++++++++++++++++++++++++++++++++++--------- >> 1 file changed, 41 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c >> index d40a22b..032260b 100644 >> --- a/drivers/mmc/mmc.c >> +++ b/drivers/mmc/mmc.c >> @@ -30,6 +30,7 @@ static const unsigned int sd_au_size[] = { >> SZ_16M / 512, (SZ_16M + SZ_8M) / 512, SZ_32M / 512, SZ_64M / 512, >> }; >> static int mmc_set_signal_voltage(struct mmc *mmc, uint signal_voltage); >> +static void mmc_power_cycle(struct mmc *mmc); >> >> #if CONFIG_IS_ENABLED(MMC_TINY) >> static struct mmc mmc_static; >> @@ -1915,6 +1916,45 @@ static int mmc_power_init(struct mmc *mmc) >> return 0; >> } >> >> +static void mmc_set_initial_state(struct mmc *mmc) > Function comment OK > >> +{ >> + int err; >> + >> + /* First try to set 3.3V. If it fails set to 1.8V */ >> + err = mmc_set_signal_voltage(mmc, MMC_SIGNAL_VOLTAGE_330); >> + if (err != 0) >> + err = mmc_set_signal_voltage(mmc, MMC_SIGNAL_VOLTAGE_180); >> + if (err != 0) >> + printf("failed to set signal voltage\n"); >> + >> + mmc_set_bus_width(mmc, 1); >> + mmc_set_clock(mmc, 1); >> + mmc_select_mode(mmc, MMC_LEGACY); >> +} >> + >> +static void mmc_power_up(struct mmc *mmc) >> +{ >> + mmc_set_initial_state(mmc); >> + mmc_set_vdd(mmc, true); >> + udelay(10000); > Eek. Please add a comment as to why This is to let the Vdd time to settle. I'll remove this for the next version. If a delay is required by the platform it can be handled by the host driver. > >> +} >> + >> +static void mmc_power_off(struct mmc *mmc) >> +{ >> + mmc_set_vdd(mmc, false); >> +} >> + >> +static void mmc_power_cycle(struct mmc *mmc) >> +{ >> + mmc_power_off(mmc); >> + /* >> + * SD spec recommends at least 1ms of delay. Let's wait for 2ms >> + * to be on the safer side. >> + */ >> + udelay(2000); >> + mmc_power_up(mmc); >> +} >> + >> int mmc_start_init(struct mmc *mmc) >> { >> bool no_card; >> @@ -1952,16 +1992,8 @@ int mmc_start_init(struct mmc *mmc) >> return err; >> #endif >> mmc->ddr_mode = 0; >> - mmc_set_vdd(mmc, true); >> - /* First try to set 3.3V. If it fails set to 1.8V */ >> - err = mmc_set_signal_voltage(mmc, MMC_SIGNAL_VOLTAGE_330); >> - if (err != 0) >> - err = mmc_set_signal_voltage(mmc, MMC_SIGNAL_VOLTAGE_180); >> - if (err != 0) >> - printf("failed to set signal voltage\n"); > Return error? Also please add some mmc: to your message. > >> - mmc_set_bus_width(mmc, 1); >> - mmc_set_clock(mmc, 1); >> + mmc_power_cycle(mmc); >> >> /* Reset the Card */ >> err = mmc_go_idle(mmc); >> -- >> 1.9.1 >> > Regards, > Simon >
On 05/13/2017 03:16 AM, Jean-Jacques Hiblot wrote: > mmc/sd specification requires vdd to be disabled for 1 ms > and then enabled again during power cycle. Add a > function in mmc core to perform power cycle and set > the io signal to it's initial state. > > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> > --- > drivers/mmc/mmc.c | 50 +++++++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 41 insertions(+), 9 deletions(-) > > diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c > index d40a22b..032260b 100644 > --- a/drivers/mmc/mmc.c > +++ b/drivers/mmc/mmc.c > @@ -30,6 +30,7 @@ static const unsigned int sd_au_size[] = { > SZ_16M / 512, (SZ_16M + SZ_8M) / 512, SZ_32M / 512, SZ_64M / 512, > }; > static int mmc_set_signal_voltage(struct mmc *mmc, uint signal_voltage); > +static void mmc_power_cycle(struct mmc *mmc); > > #if CONFIG_IS_ENABLED(MMC_TINY) > static struct mmc mmc_static; > @@ -1915,6 +1916,45 @@ static int mmc_power_init(struct mmc *mmc) > return 0; > } > > +static void mmc_set_initial_state(struct mmc *mmc) > +{ > + int err; > + > + /* First try to set 3.3V. If it fails set to 1.8V */ > + err = mmc_set_signal_voltage(mmc, MMC_SIGNAL_VOLTAGE_330); > + if (err != 0) > + err = mmc_set_signal_voltage(mmc, MMC_SIGNAL_VOLTAGE_180); > + if (err != 0) > + printf("failed to set signal voltage\n"); > + > + mmc_set_bus_width(mmc, 1); > + mmc_set_clock(mmc, 1); > + mmc_select_mode(mmc, MMC_LEGACY); > +} > + > +static void mmc_power_up(struct mmc *mmc) > +{ > + mmc_set_initial_state(mmc); > + mmc_set_vdd(mmc, true); mmc_set_vdd has the return value..but there are no usage anywhere.. if this is correct, mmc_set_vdd can be *void* type. > + udelay(10000); > +} > + > +static void mmc_power_off(struct mmc *mmc) > +{ > + mmc_set_vdd(mmc, false); > +} > + > +static void mmc_power_cycle(struct mmc *mmc) > +{ > + mmc_power_off(mmc); > + /* > + * SD spec recommends at least 1ms of delay. Let's wait for 2ms > + * to be on the safer side. > + */ Could you put the SD spec version and about which part...? I didn't find the 2ms so..i want to see the spec about 2ms. > + udelay(2000); > + mmc_power_up(mmc); > +} > + > int mmc_start_init(struct mmc *mmc) > { > bool no_card; > @@ -1952,16 +1992,8 @@ int mmc_start_init(struct mmc *mmc) > return err; > #endif > mmc->ddr_mode = 0; > - mmc_set_vdd(mmc, true); > - /* First try to set 3.3V. If it fails set to 1.8V */ > - err = mmc_set_signal_voltage(mmc, MMC_SIGNAL_VOLTAGE_330); > - if (err != 0) > - err = mmc_set_signal_voltage(mmc, MMC_SIGNAL_VOLTAGE_180); > - if (err != 0) > - printf("failed to set signal voltage\n"); > > - mmc_set_bus_width(mmc, 1); > - mmc_set_clock(mmc, 1); > + mmc_power_cycle(mmc); > > /* Reset the Card */ > err = mmc_go_idle(mmc); >
On 25/05/2017 14:35, Jaehoon Chung wrote: > On 05/13/2017 03:16 AM, Jean-Jacques Hiblot wrote: >> mmc/sd specification requires vdd to be disabled for 1 ms >> and then enabled again during power cycle. Add a >> function in mmc core to perform power cycle and set >> the io signal to it's initial state. >> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> >> --- >> drivers/mmc/mmc.c | 50 +++++++++++++++++++++++++++++++++++++++++--------- >> 1 file changed, 41 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c >> index d40a22b..032260b 100644 >> --- a/drivers/mmc/mmc.c >> +++ b/drivers/mmc/mmc.c >> @@ -30,6 +30,7 @@ static const unsigned int sd_au_size[] = { >> SZ_16M / 512, (SZ_16M + SZ_8M) / 512, SZ_32M / 512, SZ_64M / 512, >> }; >> static int mmc_set_signal_voltage(struct mmc *mmc, uint signal_voltage); >> +static void mmc_power_cycle(struct mmc *mmc); >> >> #if CONFIG_IS_ENABLED(MMC_TINY) >> static struct mmc mmc_static; >> @@ -1915,6 +1916,45 @@ static int mmc_power_init(struct mmc *mmc) >> return 0; >> } >> >> +static void mmc_set_initial_state(struct mmc *mmc) >> +{ >> + int err; >> + >> + /* First try to set 3.3V. If it fails set to 1.8V */ >> + err = mmc_set_signal_voltage(mmc, MMC_SIGNAL_VOLTAGE_330); >> + if (err != 0) >> + err = mmc_set_signal_voltage(mmc, MMC_SIGNAL_VOLTAGE_180); >> + if (err != 0) >> + printf("failed to set signal voltage\n"); >> + >> + mmc_set_bus_width(mmc, 1); >> + mmc_set_clock(mmc, 1); >> + mmc_select_mode(mmc, MMC_LEGACY); >> +} >> + >> +static void mmc_power_up(struct mmc *mmc) >> +{ >> + mmc_set_initial_state(mmc); >> + mmc_set_vdd(mmc, true); > mmc_set_vdd has the return value..but there are no usage anywhere.. > if this is correct, mmc_set_vdd can be *void* type. OK. i'll change it in v2. > >> + udelay(10000); >> +} >> + >> +static void mmc_power_off(struct mmc *mmc) >> +{ >> + mmc_set_vdd(mmc, false); >> +} >> + >> +static void mmc_power_cycle(struct mmc *mmc) >> +{ >> + mmc_power_off(mmc); >> + /* >> + * SD spec recommends at least 1ms of delay. Let's wait for 2ms >> + * to be on the safer side. >> + */ > Could you put the SD spec version and about which part...? > I didn't find the 2ms so..i want to see the spec about 2ms. I need to figure this out. I forgot to mention it, but all this code is based on the work of Kishon Vijay Abraham who did the hs200 support for the ti u-boot tree. Kishon, can you point to the relevant part of the spec ? > >> + udelay(2000); >> + mmc_power_up(mmc); >> +} >> + >> int mmc_start_init(struct mmc *mmc) >> { >> bool no_card; >> @@ -1952,16 +1992,8 @@ int mmc_start_init(struct mmc *mmc) >> return err; >> #endif >> mmc->ddr_mode = 0; >> - mmc_set_vdd(mmc, true); >> - /* First try to set 3.3V. If it fails set to 1.8V */ >> - err = mmc_set_signal_voltage(mmc, MMC_SIGNAL_VOLTAGE_330); >> - if (err != 0) >> - err = mmc_set_signal_voltage(mmc, MMC_SIGNAL_VOLTAGE_180); >> - if (err != 0) >> - printf("failed to set signal voltage\n"); >> >> - mmc_set_bus_width(mmc, 1); >> - mmc_set_clock(mmc, 1); >> + mmc_power_cycle(mmc); >> >> /* Reset the Card */ >> err = mmc_go_idle(mmc); >> >
Hi Jaehoon, On 25/05/2017 14:35, Jaehoon Chung wrote: > On 05/13/2017 03:16 AM, Jean-Jacques Hiblot wrote: >> mmc/sd specification requires vdd to be disabled for 1 ms >> and then enabled again during power cycle. Add a >> function in mmc core to perform power cycle and set >> the io signal to it's initial state. >> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> >> --- >> drivers/mmc/mmc.c | 50 +++++++++++++++++++++++++++++++++++++++++--------- >> 1 file changed, 41 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c >> index d40a22b..032260b 100644 >> --- a/drivers/mmc/mmc.c >> +++ b/drivers/mmc/mmc.c >> @@ -30,6 +30,7 @@ static const unsigned int sd_au_size[] = { >> SZ_16M / 512, (SZ_16M + SZ_8M) / 512, SZ_32M / 512, SZ_64M / 512, >> }; >> static int mmc_set_signal_voltage(struct mmc *mmc, uint signal_voltage); >> +static void mmc_power_cycle(struct mmc *mmc); >> >> #if CONFIG_IS_ENABLED(MMC_TINY) >> static struct mmc mmc_static; >> @@ -1915,6 +1916,45 @@ static int mmc_power_init(struct mmc *mmc) >> return 0; >> } >> >> +static void mmc_set_initial_state(struct mmc *mmc) >> +{ >> + int err; >> + >> + /* First try to set 3.3V. If it fails set to 1.8V */ >> + err = mmc_set_signal_voltage(mmc, MMC_SIGNAL_VOLTAGE_330); >> + if (err != 0) >> + err = mmc_set_signal_voltage(mmc, MMC_SIGNAL_VOLTAGE_180); >> + if (err != 0) >> + printf("failed to set signal voltage\n"); >> + >> + mmc_set_bus_width(mmc, 1); >> + mmc_set_clock(mmc, 1); >> + mmc_select_mode(mmc, MMC_LEGACY); >> +} >> + >> +static void mmc_power_up(struct mmc *mmc) >> +{ >> + mmc_set_initial_state(mmc); >> + mmc_set_vdd(mmc, true); > mmc_set_vdd has the return value..but there are no usage anywhere.. > if this is correct, mmc_set_vdd can be *void* type. > >> + udelay(10000); >> +} >> + >> +static void mmc_power_off(struct mmc *mmc) >> +{ >> + mmc_set_vdd(mmc, false); >> +} >> + >> +static void mmc_power_cycle(struct mmc *mmc) >> +{ >> + mmc_power_off(mmc); >> + /* >> + * SD spec recommends at least 1ms of delay. Let's wait for 2ms >> + * to be on the safer side. >> + */ > Could you put the SD spec version and about which part...? > I didn't find the 2ms so..i want to see the spec about 2ms. The requirement for 1ms is found in the simplified specifications "Part1_Physical_Layer_Simplified_Specification_Ver6.00.pdf": [...] 6.4.1.5 Power Down and Power Cycle When the host shuts down the power, the card VDD shall be lowered to less than 0.5Volt for a minimum period of 1ms. [....] 2ms is just to be on the safer side. JJ > >> + udelay(2000); >> + mmc_power_up(mmc); >> +} >> + >> int mmc_start_init(struct mmc *mmc) >> { >> bool no_card; >> @@ -1952,16 +1992,8 @@ int mmc_start_init(struct mmc *mmc) >> return err; >> #endif >> mmc->ddr_mode = 0; >> - mmc_set_vdd(mmc, true); >> - /* First try to set 3.3V. If it fails set to 1.8V */ >> - err = mmc_set_signal_voltage(mmc, MMC_SIGNAL_VOLTAGE_330); >> - if (err != 0) >> - err = mmc_set_signal_voltage(mmc, MMC_SIGNAL_VOLTAGE_180); >> - if (err != 0) >> - printf("failed to set signal voltage\n"); >> >> - mmc_set_bus_width(mmc, 1); >> - mmc_set_clock(mmc, 1); >> + mmc_power_cycle(mmc); >> >> /* Reset the Card */ >> err = mmc_go_idle(mmc); >> >
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index d40a22b..032260b 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -30,6 +30,7 @@ static const unsigned int sd_au_size[] = { SZ_16M / 512, (SZ_16M + SZ_8M) / 512, SZ_32M / 512, SZ_64M / 512, }; static int mmc_set_signal_voltage(struct mmc *mmc, uint signal_voltage); +static void mmc_power_cycle(struct mmc *mmc); #if CONFIG_IS_ENABLED(MMC_TINY) static struct mmc mmc_static; @@ -1915,6 +1916,45 @@ static int mmc_power_init(struct mmc *mmc) return 0; } +static void mmc_set_initial_state(struct mmc *mmc) +{ + int err; + + /* First try to set 3.3V. If it fails set to 1.8V */ + err = mmc_set_signal_voltage(mmc, MMC_SIGNAL_VOLTAGE_330); + if (err != 0) + err = mmc_set_signal_voltage(mmc, MMC_SIGNAL_VOLTAGE_180); + if (err != 0) + printf("failed to set signal voltage\n"); + + mmc_set_bus_width(mmc, 1); + mmc_set_clock(mmc, 1); + mmc_select_mode(mmc, MMC_LEGACY); +} + +static void mmc_power_up(struct mmc *mmc) +{ + mmc_set_initial_state(mmc); + mmc_set_vdd(mmc, true); + udelay(10000); +} + +static void mmc_power_off(struct mmc *mmc) +{ + mmc_set_vdd(mmc, false); +} + +static void mmc_power_cycle(struct mmc *mmc) +{ + mmc_power_off(mmc); + /* + * SD spec recommends at least 1ms of delay. Let's wait for 2ms + * to be on the safer side. + */ + udelay(2000); + mmc_power_up(mmc); +} + int mmc_start_init(struct mmc *mmc) { bool no_card; @@ -1952,16 +1992,8 @@ int mmc_start_init(struct mmc *mmc) return err; #endif mmc->ddr_mode = 0; - mmc_set_vdd(mmc, true); - /* First try to set 3.3V. If it fails set to 1.8V */ - err = mmc_set_signal_voltage(mmc, MMC_SIGNAL_VOLTAGE_330); - if (err != 0) - err = mmc_set_signal_voltage(mmc, MMC_SIGNAL_VOLTAGE_180); - if (err != 0) - printf("failed to set signal voltage\n"); - mmc_set_bus_width(mmc, 1); - mmc_set_clock(mmc, 1); + mmc_power_cycle(mmc); /* Reset the Card */ err = mmc_go_idle(mmc);