diff mbox series

[12/22] mmc: Enable signal voltage to be selected from mmc core

Message ID 1494613000-8156-13-git-send-email-jjhiblot@ti.com
State New
Headers show
Series None | expand

Commit Message

Jean-Jacques Hiblot May 12, 2017, 6:16 p.m. UTC 
From: Kishon Vijay Abraham I <kishon@ti.com>

Add a new function *mmc_set_signal_voltage* in mmc core
which can be used during mmc initialization to select the
signal voltage. Platform driver should use the set_ios
callback function to select the signal voltage.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
---
 drivers/mmc/mmc.c | 15 +++++++++++++++
 include/mmc.h     |  5 +++++
 2 files changed, 20 insertions(+)

Comments

Simon Glass May 15, 2017, 3:28 a.m. UTC | #1 
Hi Jen-Jacques,

On 12 May 2017 at 12:16, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
> From: Kishon Vijay Abraham I <kishon@ti.com>
>
> Add a new function *mmc_set_signal_voltage* in mmc core
> which can be used during mmc initialization to select the
> signal voltage. Platform driver should use the set_ios
> callback function to select the signal voltage.
>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> ---
>  drivers/mmc/mmc.c | 15 +++++++++++++++
>  include/mmc.h     |  5 +++++
>  2 files changed, 20 insertions(+)
>
> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
> index 2ae6f1c..10af81d 100644
> --- a/drivers/mmc/mmc.c
> +++ b/drivers/mmc/mmc.c
> @@ -29,6 +29,7 @@ static const unsigned int sd_au_size[] = {
>         SZ_4M / 512,    SZ_8M / 512,            (SZ_8M + SZ_4M) / 512,
>         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);
>
>  #if CONFIG_IS_ENABLED(MMC_TINY)
>  static struct mmc mmc_static;
> @@ -1247,6 +1248,12 @@ struct mode_width_tuning {
>         uint widths;
>  };
>
> +static int mmc_set_signal_voltage(struct mmc *mmc, uint signal_voltage)
> +{
> +       mmc->signal_voltage = signal_voltage;
> +       return mmc_set_ios(mmc);
> +}
> +
>  static const struct mode_width_tuning sd_modes_by_pref[] = {
>         {
>                 .mode = SD_HS,
> @@ -1935,6 +1942,14 @@ int mmc_start_init(struct mmc *mmc)
>                 return err;
>  #endif
>         mmc->ddr_mode = 0;
> +
> +       /* 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?

> +
>         mmc_set_bus_width(mmc, 1);
>         mmc_set_clock(mmc, 1);
>
> diff --git a/include/mmc.h b/include/mmc.h
> index 9f20eb4..89cb26c 100644
> --- a/include/mmc.h
> +++ b/include/mmc.h
> @@ -266,6 +266,10 @@
>  #define ENHNCD_SUPPORT         (0x2)
>  #define PART_ENH_ATTRIB                (0x1f)
>
> +#define MMC_SIGNAL_VOLTAGE_330 1
> +#define MMC_SIGNAL_VOLTAGE_180 2
> +#define MMC_SIGNAL_VOLTAGE_120 3
> +
>  /* Maximum block size for MMC */
>  #define MMC_MAX_BLOCK_LEN      512
>
> @@ -452,6 +456,7 @@ struct mmc {
>         int high_capacity;
>         uint bus_width;
>         uint clock;
> +       uint signal_voltage;

Comment. What does this value mean? What units is it? Millivolts or
something else?

>         uint card_caps;
>         uint ocr;
>         uint dsr;
> --
> 1.9.1
>

Regards,
Simon
Jean-Jacques Hiblot May 15, 2017, 2:18 p.m. UTC | #2 
On 15/05/2017 05:28, Simon Glass wrote:
> Hi Jen-Jacques,
>
> On 12 May 2017 at 12:16, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
>> From: Kishon Vijay Abraham I <kishon@ti.com>
>>
>> Add a new function *mmc_set_signal_voltage* in mmc core
>> which can be used during mmc initialization to select the
>> signal voltage. Platform driver should use the set_ios
>> callback function to select the signal voltage.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
>> ---
>>   drivers/mmc/mmc.c | 15 +++++++++++++++
>>   include/mmc.h     |  5 +++++
>>   2 files changed, 20 insertions(+)
>>
>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
>> index 2ae6f1c..10af81d 100644
>> --- a/drivers/mmc/mmc.c
>> +++ b/drivers/mmc/mmc.c
>> @@ -29,6 +29,7 @@ static const unsigned int sd_au_size[] = {
>>          SZ_4M / 512,    SZ_8M / 512,            (SZ_8M + SZ_4M) / 512,
>>          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);
>>
>>   #if CONFIG_IS_ENABLED(MMC_TINY)
>>   static struct mmc mmc_static;
>> @@ -1247,6 +1248,12 @@ struct mode_width_tuning {
>>          uint widths;
>>   };
>>
>> +static int mmc_set_signal_voltage(struct mmc *mmc, uint signal_voltage)
>> +{
>> +       mmc->signal_voltage = signal_voltage;
>> +       return mmc_set_ios(mmc);
>> +}
>> +
>>   static const struct mode_width_tuning sd_modes_by_pref[] = {
>>          {
>>                  .mode = SD_HS,
>> @@ -1935,6 +1942,14 @@ int mmc_start_init(struct mmc *mmc)
>>                  return err;
>>   #endif
>>          mmc->ddr_mode = 0;
>> +
>> +       /* 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?
Maybe we should. I don't know.
Setting signal voltage is optional (at least for most modes), some 
platforms don't support it. So I wouldn't exit on this kind of error. 
Those 2 calls to mmc_set_signal_voltage() are here merely to turn on 
regulators before doing the card init. If setting voltage is required 
and fails, the init process won't go very far.

>> +
>>          mmc_set_bus_width(mmc, 1);
>>          mmc_set_clock(mmc, 1);
>>
>> diff --git a/include/mmc.h b/include/mmc.h
>> index 9f20eb4..89cb26c 100644
>> --- a/include/mmc.h
>> +++ b/include/mmc.h
>> @@ -266,6 +266,10 @@
>>   #define ENHNCD_SUPPORT         (0x2)
>>   #define PART_ENH_ATTRIB                (0x1f)
>>
>> +#define MMC_SIGNAL_VOLTAGE_330 1
>> +#define MMC_SIGNAL_VOLTAGE_180 2
>> +#define MMC_SIGNAL_VOLTAGE_120 3
>> +
>>   /* Maximum block size for MMC */
>>   #define MMC_MAX_BLOCK_LEN      512
>>
>> @@ -452,6 +456,7 @@ struct mmc {
>>          int high_capacity;
>>          uint bus_width;
>>          uint clock;
>> +       uint signal_voltage;
> Comment. What does this value mean? What units is it? Millivolts or
> something else?
it's more an enum than a value with a physical meaning (like mV). I'll 
change this in the next version to be a real enum.
>
>>          uint card_caps;
>>          uint ocr;
>>          uint dsr;
>> --
>> 1.9.1
>>
> Regards,
> Simon
>
Simon Glass May 17, 2017, 1:38 a.m. UTC | #3 
Hi Jean-Jacques,

On 15 May 2017 at 08:18, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
>
>
> On 15/05/2017 05:28, Simon Glass wrote:
>>
>> Hi Jen-Jacques,
>>
>> On 12 May 2017 at 12:16, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
>>>
>>> From: Kishon Vijay Abraham I <kishon@ti.com>
>>>
>>> Add a new function *mmc_set_signal_voltage* in mmc core
>>> which can be used during mmc initialization to select the
>>> signal voltage. Platform driver should use the set_ios
>>> callback function to select the signal voltage.
>>>
>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
>>> ---
>>>   drivers/mmc/mmc.c | 15 +++++++++++++++
>>>   include/mmc.h     |  5 +++++
>>>   2 files changed, 20 insertions(+)
>>>
>>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
>>> index 2ae6f1c..10af81d 100644
>>> --- a/drivers/mmc/mmc.c
>>> +++ b/drivers/mmc/mmc.c
>>> @@ -29,6 +29,7 @@ static const unsigned int sd_au_size[] = {
>>>          SZ_4M / 512,    SZ_8M / 512,            (SZ_8M + SZ_4M) / 512,
>>>          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);
>>>
>>>   #if CONFIG_IS_ENABLED(MMC_TINY)
>>>   static struct mmc mmc_static;
>>> @@ -1247,6 +1248,12 @@ struct mode_width_tuning {
>>>          uint widths;
>>>   };
>>>
>>> +static int mmc_set_signal_voltage(struct mmc *mmc, uint signal_voltage)
>>> +{
>>> +       mmc->signal_voltage = signal_voltage;
>>> +       return mmc_set_ios(mmc);
>>> +}
>>> +
>>>   static const struct mode_width_tuning sd_modes_by_pref[] = {
>>>          {
>>>                  .mode = SD_HS,
>>> @@ -1935,6 +1942,14 @@ int mmc_start_init(struct mmc *mmc)
>>>                  return err;
>>>   #endif
>>>          mmc->ddr_mode = 0;
>>> +
>>> +       /* 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?
>
> Maybe we should. I don't know.
> Setting signal voltage is optional (at least for most modes), some platforms
> don't support it. So I wouldn't exit on this kind of error. Those 2 calls to
> mmc_set_signal_voltage() are here merely to turn on regulators before doing
> the card init. If setting voltage is required and fails, the init process
> won't go very far.

If there is no regulator, then how about checking the error return
value, and if it is -ENODEV, continue. But any other error, you exit.

>
>>> +
>>>          mmc_set_bus_width(mmc, 1);
>>>          mmc_set_clock(mmc, 1);
>>>
>>> diff --git a/include/mmc.h b/include/mmc.h
>>> index 9f20eb4..89cb26c 100644
>>> --- a/include/mmc.h
>>> +++ b/include/mmc.h
>>> @@ -266,6 +266,10 @@
>>>   #define ENHNCD_SUPPORT         (0x2)
>>>   #define PART_ENH_ATTRIB                (0x1f)
>>>
>>> +#define MMC_SIGNAL_VOLTAGE_330 1
>>> +#define MMC_SIGNAL_VOLTAGE_180 2
>>> +#define MMC_SIGNAL_VOLTAGE_120 3
>>> +
>>>   /* Maximum block size for MMC */
>>>   #define MMC_MAX_BLOCK_LEN      512
>>>
>>> @@ -452,6 +456,7 @@ struct mmc {
>>>          int high_capacity;
>>>          uint bus_width;
>>>          uint clock;
>>> +       uint signal_voltage;
>>
>> Comment. What does this value mean? What units is it? Millivolts or
>> something else?
>
> it's more an enum than a value with a physical meaning (like mV). I'll
> change this in the next version to be a real enum.
>
>>
>>>          uint card_caps;
>>>          uint ocr;
>>>          uint dsr;
>>> --
>>> 1.9.1
>>

Regards,
Simon
Kishon Vijay Abraham I May 17, 2017, 10:35 a.m. UTC | #4 
Hi Simon,

On Monday 15 May 2017 08:58 AM, Simon Glass wrote:
> Hi Jen-Jacques,
> 
> On 12 May 2017 at 12:16, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
>> From: Kishon Vijay Abraham I <kishon@ti.com>
>>
>> Add a new function *mmc_set_signal_voltage* in mmc core
>> which can be used during mmc initialization to select the
>> signal voltage. Platform driver should use the set_ios
>> callback function to select the signal voltage.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
>> ---
>>  drivers/mmc/mmc.c | 15 +++++++++++++++
>>  include/mmc.h     |  5 +++++
>>  2 files changed, 20 insertions(+)
>>
>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
>> index 2ae6f1c..10af81d 100644
>> --- a/drivers/mmc/mmc.c
>> +++ b/drivers/mmc/mmc.c
>> @@ -29,6 +29,7 @@ static const unsigned int sd_au_size[] = {
>>         SZ_4M / 512,    SZ_8M / 512,            (SZ_8M + SZ_4M) / 512,
>>         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);
>>
>>  #if CONFIG_IS_ENABLED(MMC_TINY)
>>  static struct mmc mmc_static;
>> @@ -1247,6 +1248,12 @@ struct mode_width_tuning {
>>         uint widths;
>>  };
>>
>> +static int mmc_set_signal_voltage(struct mmc *mmc, uint signal_voltage)
>> +{
>> +       mmc->signal_voltage = signal_voltage;
>> +       return mmc_set_ios(mmc);
>> +}
>> +
>>  static const struct mode_width_tuning sd_modes_by_pref[] = {
>>         {
>>                 .mode = SD_HS,
>> @@ -1935,6 +1942,14 @@ int mmc_start_init(struct mmc *mmc)
>>                 return err;
>>  #endif
>>         mmc->ddr_mode = 0;
>> +
>> +       /* 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?

Since other API's here like mmc_set_bus_width, mmc_set_clock wasn't returning
error, I didn't return error from mmc_set_signal_voltage too. Thought returning
error can be added as a separate patch later for all the APIs used here.

Thanks
Kishon
diff mbox series

Patch

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index 2ae6f1c..10af81d 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -29,6 +29,7 @@  static const unsigned int sd_au_size[] = {
 	SZ_4M / 512,	SZ_8M / 512,		(SZ_8M + SZ_4M) / 512,
 	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);
 
 #if CONFIG_IS_ENABLED(MMC_TINY)
 static struct mmc mmc_static;
@@ -1247,6 +1248,12 @@  struct mode_width_tuning {
 	uint widths;
 };
 
+static int mmc_set_signal_voltage(struct mmc *mmc, uint signal_voltage)
+{
+	mmc->signal_voltage = signal_voltage;
+	return mmc_set_ios(mmc);
+}
+
 static const struct mode_width_tuning sd_modes_by_pref[] = {
 	{
 		.mode = SD_HS,
@@ -1935,6 +1942,14 @@  int mmc_start_init(struct mmc *mmc)
 		return err;
 #endif
 	mmc->ddr_mode = 0;
+
+	/* 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);
 
diff --git a/include/mmc.h b/include/mmc.h
index 9f20eb4..89cb26c 100644
--- a/include/mmc.h
+++ b/include/mmc.h
@@ -266,6 +266,10 @@ 
 #define ENHNCD_SUPPORT		(0x2)
 #define PART_ENH_ATTRIB		(0x1f)
 
+#define MMC_SIGNAL_VOLTAGE_330	1
+#define MMC_SIGNAL_VOLTAGE_180	2
+#define MMC_SIGNAL_VOLTAGE_120	3
+
 /* Maximum block size for MMC */
 #define MMC_MAX_BLOCK_LEN	512
 
@@ -452,6 +456,7 @@  struct mmc {
 	int high_capacity;
 	uint bus_width;
 	uint clock;
+	uint signal_voltage;
 	uint card_caps;
 	uint ocr;
 	uint dsr;