Patchwork [U-Boot] arm: trats: add the sd-card power control function.

login
register
mail settings
Submitter Jaehoon Chung
Date Oct. 31, 2013, 1:37 p.m.
Message ID <52725D29.4080106@samsung.com>
Download mbox | patch
Permalink /patch/287493/
State Changes Requested
Delegated to: Minkyu Kang
Headers show

Comments

Jaehoon Chung - Oct. 31, 2013, 1:37 p.m.
To use the sd-card, VTF_2.8V will be enabled.
Before this patch, VTF_2.8V is always disabled.(card can't initialize.)
When card is detected, SD-card power will enable with sd_ldo_control().

Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 board/samsung/trats/trats.c |   38 ++++++++++++++++++++++++++++++++------
 1 file changed, 32 insertions(+), 6 deletions(-)
Ɓukasz Majewski - Oct. 31, 2013, 5:03 p.m.
Hi Jaehoon,

> To use the sd-card, VTF_2.8V will be enabled.
> Before this patch, VTF_2.8V is always disabled.(card can't
> initialize.) When card is detected, SD-card power will enable with
> sd_ldo_control().
> 
> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  board/samsung/trats/trats.c |   38
> ++++++++++++++++++++++++++++++++------ 1 file changed, 32
> insertions(+), 6 deletions(-)
> 
> diff --git a/board/samsung/trats/trats.c b/board/samsung/trats/trats.c
> index 7f61d17..9b6dd12 100644
> --- a/board/samsung/trats/trats.c
> +++ b/board/samsung/trats/trats.c
> @@ -408,11 +408,31 @@ int checkboard(void)
>  #endif
>  
>  #ifdef CONFIG_GENERIC_MMC
> +static int sd_ldo_control(int on)
> +{
> +	struct pmic *p = pmic_get("MAX8997_PMIC");
> +	u32 val = 0;
> +	int ret = 0;
> +	if (pmic_probe(p))
> +		return -1;

Maybe -1 -> -ENODEV

> +
> +	/* LDO17 VTF: 2.8V */
> +	val = max8997_reg_ldo(2800000) | (on? EN_LDO : DIS_LDO);
                                          ^^^ on ?
> +	ret |= pmic_reg_write(p, MAX8997_REG_LDO17CTRL, val);
	^^^^^ ret = 
> +
> +	if (ret) {
> +		puts("MAX8997 SD setting error!\n");
		
Please use error() instead of puts()

> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
>  int board_mmc_init(bd_t *bis)
>  {
>  	struct exynos4_gpio_part2 *gpio =
>  		(struct exynos4_gpio_part2
> *)samsung_get_base_gpio_part2();
> -	int err;
> +	int err, err_sd = 0;

I think that the err_sd is not needed here. Please use the err as is
was at the original code.

>  
>  	/* eMMC_EN: SD_0_CDn: GPK0[2] Output High */
>  	s5p_gpio_direction_output(&gpio->k0, 2, 1);
> @@ -438,14 +458,20 @@ int board_mmc_init(bd_t *bis)
>  	 * GPX3[4] T-flash detect pin
>  	 */
>  	if (!s5p_gpio_get_value(&gpio->x3, 4)) {
> -		err = exynos_pinmux_config(PERIPH_ID_SDMMC2,
> PINMUX_FLAG_NONE);
> -		if (err)
> +		err_sd = exynos_pinmux_config(PERIPH_ID_SDMMC2,
> PINMUX_FLAG_NONE);
> +		if (err_sd)
>  			debug("SDMMC2 not configured\n");
> -		else
> -			err = s5p_mmc_init(2, 4);
> +		else {
> +			if (!sd_ldo_control(1)) {
> +				err_sd = s5p_mmc_init(2, 4);
> +				if (err_sd) {
> +					sd_ldo_control(0);
> +				}
				Parenthesis with this if aren't needed.
> +			}
> +		}
>  	}
>  
> -	return err;
> +	return err & err_sd;
>  }
>  #endif
>
Jaehoon Chung - Nov. 1, 2013, 1:02 a.m.
Hi Lukasz,

Thanks for your comments.

On 11/01/2013 02:03 AM, Lukasz Majewski wrote:
> Hi Jaehoon,
> 
>> To use the sd-card, VTF_2.8V will be enabled.
>> Before this patch, VTF_2.8V is always disabled.(card can't
>> initialize.) When card is detected, SD-card power will enable with
>> sd_ldo_control().
>>
>> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>>  board/samsung/trats/trats.c |   38
>> ++++++++++++++++++++++++++++++++------ 1 file changed, 32
>> insertions(+), 6 deletions(-)
>>
>> diff --git a/board/samsung/trats/trats.c b/board/samsung/trats/trats.c
>> index 7f61d17..9b6dd12 100644
>> --- a/board/samsung/trats/trats.c
>> +++ b/board/samsung/trats/trats.c
>> @@ -408,11 +408,31 @@ int checkboard(void)
>>  #endif
>>  
>>  #ifdef CONFIG_GENERIC_MMC
>> +static int sd_ldo_control(int on)
>> +{
>> +	struct pmic *p = pmic_get("MAX8997_PMIC");
>> +	u32 val = 0;
>> +	int ret = 0;
>> +	if (pmic_probe(p))
>> +		return -1;
> 
> Maybe -1 -> -ENODEV
Will fix..
> 
>> +
>> +	/* LDO17 VTF: 2.8V */
>> +	val = max8997_reg_ldo(2800000) | (on? EN_LDO : DIS_LDO);
>                                           ^^^ on ?
Will fix
>> +	ret |= pmic_reg_write(p, MAX8997_REG_LDO17CTRL, val);
> 	^^^^^ ret = 
Will fix.
>> +
>> +	if (ret) {
>> +		puts("MAX8997 SD setting error!\n");
> 		
> Please use error() instead of puts()
Using error().
> 
>> +		return -1;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  int board_mmc_init(bd_t *bis)
>>  {
>>  	struct exynos4_gpio_part2 *gpio =
>>  		(struct exynos4_gpio_part2
>> *)samsung_get_base_gpio_part2();
>> -	int err;
>> +	int err, err_sd = 0;
> 
> I think that the err_sd is not needed here. Please use the err as is
> was at the original code.
As like the goni, I think sd_err needs herre.
We need to consider the cases when eMMC is intiailzed and SD-card is failed.
If SD-card is failed, then it's always retunred "Fail", although eMMC is initialized.
how about?
After replying your, i will send the fixing patch.

Best Regards,
Jaehoon Chung
> 
>>  
>>  	/* eMMC_EN: SD_0_CDn: GPK0[2] Output High */
>>  	s5p_gpio_direction_output(&gpio->k0, 2, 1);
>> @@ -438,14 +458,20 @@ int board_mmc_init(bd_t *bis)
>>  	 * GPX3[4] T-flash detect pin
>>  	 */
>>  	if (!s5p_gpio_get_value(&gpio->x3, 4)) {
>> -		err = exynos_pinmux_config(PERIPH_ID_SDMMC2,
>> PINMUX_FLAG_NONE);
>> -		if (err)
>> +		err_sd = exynos_pinmux_config(PERIPH_ID_SDMMC2,
>> PINMUX_FLAG_NONE);
>> +		if (err_sd)
>>  			debug("SDMMC2 not configured\n");
>> -		else
>> -			err = s5p_mmc_init(2, 4);
>> +		else {
>> +			if (!sd_ldo_control(1)) {
>> +				err_sd = s5p_mmc_init(2, 4);
>> +				if (err_sd) {
>> +					sd_ldo_control(0);
>> +				}
> 				Parenthesis with this if aren't needed.
>> +			}
>> +		}
>>  	}
>>  
>> -	return err;
>> +	return err & err_sd;
>>  }
>>  #endif
>>  
> 
> 
>
Lukasz Majewski - Nov. 3, 2013, 10:24 p.m.
Hi Jaehoon,

> Hi Lukasz,
> 
> Thanks for your comments.
> 
> On 11/01/2013 02:03 AM, Lukasz Majewski wrote:
> > Hi Jaehoon,
> > 
> >> To use the sd-card, VTF_2.8V will be enabled.
> >> Before this patch, VTF_2.8V is always disabled.(card can't
> >> initialize.) When card is detected, SD-card power will enable with
> >> sd_ldo_control().
> >>
> >> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> >> ---
> >>  board/samsung/trats/trats.c |   38
> >> ++++++++++++++++++++++++++++++++------ 1 file changed, 32
> >> insertions(+), 6 deletions(-)
> >>
> >> diff --git a/board/samsung/trats/trats.c
> >> b/board/samsung/trats/trats.c index 7f61d17..9b6dd12 100644
> >> --- a/board/samsung/trats/trats.c
> >> +++ b/board/samsung/trats/trats.c
> >> @@ -408,11 +408,31 @@ int checkboard(void)
> >>  #endif
> >>  
> >>  #ifdef CONFIG_GENERIC_MMC
> >> +static int sd_ldo_control(int on)
> >> +{
> >> +	struct pmic *p = pmic_get("MAX8997_PMIC");
> >> +	u32 val = 0;
> >> +	int ret = 0;
> >> +	if (pmic_probe(p))
> >> +		return -1;
> > 
> > Maybe -1 -> -ENODEV
> Will fix..
> > 
> >> +
> >> +	/* LDO17 VTF: 2.8V */
> >> +	val = max8997_reg_ldo(2800000) | (on? EN_LDO : DIS_LDO);
> >                                           ^^^ on ?
> Will fix
> >> +	ret |= pmic_reg_write(p, MAX8997_REG_LDO17CTRL, val);
> > 	^^^^^ ret = 
> Will fix.
> >> +
> >> +	if (ret) {
> >> +		puts("MAX8997 SD setting error!\n");
> > 		
> > Please use error() instead of puts()
> Using error().
> > 
> >> +		return -1;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  int board_mmc_init(bd_t *bis)
> >>  {
> >>  	struct exynos4_gpio_part2 *gpio =
> >>  		(struct exynos4_gpio_part2
> >> *)samsung_get_base_gpio_part2();
> >> -	int err;
> >> +	int err, err_sd = 0;
> > 
> > I think that the err_sd is not needed here. Please use the err as is
> > was at the original code.
> As like the goni, I think sd_err needs herre.
> We need to consider the cases when eMMC is intiailzed and SD-card is
> failed. If SD-card is failed, then it's always retunred "Fail",
> although eMMC is initialized. how about?

I think that it is perfectly correct when we insert SD card (which is
signaled by change of card detect pin - X3[4]) and when we fail to
initialize it to display the error message and pass the error code out
of the function.

Then user can read the error cause from the console and perform hard
reset with SD card removed.


> After replying your, i will send the fixing patch.
> 
> Best Regards,
> Jaehoon Chung
> > 
> >>  
> >>  	/* eMMC_EN: SD_0_CDn: GPK0[2] Output High */
> >>  	s5p_gpio_direction_output(&gpio->k0, 2, 1);
> >> @@ -438,14 +458,20 @@ int board_mmc_init(bd_t *bis)
> >>  	 * GPX3[4] T-flash detect pin
> >>  	 */
> >>  	if (!s5p_gpio_get_value(&gpio->x3, 4)) {
> >> -		err = exynos_pinmux_config(PERIPH_ID_SDMMC2,
> >> PINMUX_FLAG_NONE);
> >> -		if (err)
> >> +		err_sd = exynos_pinmux_config(PERIPH_ID_SDMMC2,
> >> PINMUX_FLAG_NONE);
> >> +		if (err_sd)
> >>  			debug("SDMMC2 not configured\n");
> >> -		else
> >> -			err = s5p_mmc_init(2, 4);
> >> +		else {
> >> +			if (!sd_ldo_control(1)) {
> >> +				err_sd = s5p_mmc_init(2, 4);
> >> +				if (err_sd) {
> >> +					sd_ldo_control(0);
> >> +				}
> > 				Parenthesis with this if aren't
> > needed.
> >> +			}
> >> +		}
> >>  	}
> >>  
> >> -	return err;
> >> +	return err & err_sd;

To be honest I'm frowned upon when errors are ANDed. In my opinion they
shall be ORed together.


So I think that for the trats.c we only need to do the following:

 	if (!s5p_gpio_get_value(&gpio->x3, 4)) {
	err |= exynos_pinmux_config(PERIPH_ID_SDMMC2, PINMUX_FLAG_NONE);
		if (err) {
 			debug("SDMMC2 not configured\n");
		} else {
			if (!sd_ldo_control(1)) {
				err |= s5p_mmc_init(2, 4);
				if (err) {
					sd_ldo_control(0);
					error("SD catd init failed\
					- remove it\n");
				}
			}
		}
 	}
 
	return err;

Please consider following scenarios:

1. eMMC OK, card present and broken -> print informative error message
(to recover user will remove the card)

2. eMMC OK + card inserted and init OK = no problem 

3. eMMC OK + card not inserted = no problem

4. eMMC initialization broken + card inserted and init OK - we will not
boot the device from eMMC, so we will not be able to use SD card at
u-boot. 

Please correct me if I'm wrong and/or I've overlooked some useful
scenario for you.

Best regards,
Lukasz Majewski
Jaehoon Chung - Nov. 4, 2013, 1:28 a.m.
Hi, Lukasz,

On 11/04/2013 07:24 AM, Lukasz Majewski wrote:
> Hi Jaehoon,
> 
>> Hi Lukasz,
>>
>> Thanks for your comments.
>>
>> On 11/01/2013 02:03 AM, Lukasz Majewski wrote:
>>> Hi Jaehoon,
>>>
>>>> To use the sd-card, VTF_2.8V will be enabled.
>>>> Before this patch, VTF_2.8V is always disabled.(card can't
>>>> initialize.) When card is detected, SD-card power will enable with
>>>> sd_ldo_control().
>>>>
>>>> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
>>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>>> ---
>>>>  board/samsung/trats/trats.c |   38
>>>> ++++++++++++++++++++++++++++++++------ 1 file changed, 32
>>>> insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/board/samsung/trats/trats.c
>>>> b/board/samsung/trats/trats.c index 7f61d17..9b6dd12 100644
>>>> --- a/board/samsung/trats/trats.c
>>>> +++ b/board/samsung/trats/trats.c
>>>> @@ -408,11 +408,31 @@ int checkboard(void)
>>>>  #endif
>>>>  
>>>>  #ifdef CONFIG_GENERIC_MMC
>>>> +static int sd_ldo_control(int on)
>>>> +{
>>>> +	struct pmic *p = pmic_get("MAX8997_PMIC");
>>>> +	u32 val = 0;
>>>> +	int ret = 0;
>>>> +	if (pmic_probe(p))
>>>> +		return -1;
>>>
>>> Maybe -1 -> -ENODEV
>> Will fix..
>>>
>>>> +
>>>> +	/* LDO17 VTF: 2.8V */
>>>> +	val = max8997_reg_ldo(2800000) | (on? EN_LDO : DIS_LDO);
>>>                                           ^^^ on ?
>> Will fix
>>>> +	ret |= pmic_reg_write(p, MAX8997_REG_LDO17CTRL, val);
>>> 	^^^^^ ret = 
>> Will fix.
>>>> +
>>>> +	if (ret) {
>>>> +		puts("MAX8997 SD setting error!\n");
>>> 		
>>> Please use error() instead of puts()
>> Using error().
>>>
>>>> +		return -1;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>>  int board_mmc_init(bd_t *bis)
>>>>  {
>>>>  	struct exynos4_gpio_part2 *gpio =
>>>>  		(struct exynos4_gpio_part2
>>>> *)samsung_get_base_gpio_part2();
>>>> -	int err;
>>>> +	int err, err_sd = 0;
>>>
>>> I think that the err_sd is not needed here. Please use the err as is
>>> was at the original code.
>> As like the goni, I think sd_err needs herre.
>> We need to consider the cases when eMMC is intiailzed and SD-card is
>> failed. If SD-card is failed, then it's always retunred "Fail",
>> although eMMC is initialized. how about?
> 
> I think that it is perfectly correct when we insert SD card (which is
> signaled by change of card detect pin - X3[4]) and when we fail to
> initialize it to display the error message and pass the error code out
> of the function.
> 
> Then user can read the error cause from the console and perform hard
> reset with SD card removed.
> 
> 
>> After replying your, i will send the fixing patch.
>>
>> Best Regards,
>> Jaehoon Chung
>>>
>>>>  
>>>>  	/* eMMC_EN: SD_0_CDn: GPK0[2] Output High */
>>>>  	s5p_gpio_direction_output(&gpio->k0, 2, 1);
>>>> @@ -438,14 +458,20 @@ int board_mmc_init(bd_t *bis)
>>>>  	 * GPX3[4] T-flash detect pin
>>>>  	 */
>>>>  	if (!s5p_gpio_get_value(&gpio->x3, 4)) {
>>>> -		err = exynos_pinmux_config(PERIPH_ID_SDMMC2,
>>>> PINMUX_FLAG_NONE);
>>>> -		if (err)
>>>> +		err_sd = exynos_pinmux_config(PERIPH_ID_SDMMC2,
>>>> PINMUX_FLAG_NONE);
>>>> +		if (err_sd)
>>>>  			debug("SDMMC2 not configured\n");
>>>> -		else
>>>> -			err = s5p_mmc_init(2, 4);
>>>> +		else {
>>>> +			if (!sd_ldo_control(1)) {
>>>> +				err_sd = s5p_mmc_init(2, 4);
>>>> +				if (err_sd) {
>>>> +					sd_ldo_control(0);
>>>> +				}
>>> 				Parenthesis with this if aren't
>>> needed.
>>>> +			}
>>>> +		}
>>>>  	}
>>>>  
>>>> -	return err;
>>>> +	return err & err_sd;
> 
> To be honest I'm frowned upon when errors are ANDed. In my opinion they
> shall be ORed together.
> 
> 
> So I think that for the trats.c we only need to do the following:
> 
>  	if (!s5p_gpio_get_value(&gpio->x3, 4)) {
> 	err |= exynos_pinmux_config(PERIPH_ID_SDMMC2, PINMUX_FLAG_NONE);
> 		if (err) {
>  			debug("SDMMC2 not configured\n");
> 		} else {
> 			if (!sd_ldo_control(1)) {
> 				err |= s5p_mmc_init(2, 4);
> 				if (err) {
> 					sd_ldo_control(0);
> 					error("SD catd init failed\
> 					- remove it\n");
> 				}
> 			}
> 		}
>  	}
>  
> 	return err;
> 
> Please consider following scenarios:
> 
> 1. eMMC OK, card present and broken -> print informative error message
> (to recover user will remove the card)
> 
> 2. eMMC OK + card inserted and init OK = no problem 
> 
> 3. eMMC OK + card not inserted = no problem
> 
> 4. eMMC initialization broken + card inserted and init OK - we will not
> boot the device from eMMC, so we will not be able to use SD card at
> u-boot. 
> 
> Please correct me if I'm wrong and/or I've overlooked some useful
> scenario for you.
As your opinion, I will test and rework the patch.

Best Regards,
Jaehoon Chung
> 
> Best regards,
> Lukasz Majewski
>

Patch

diff --git a/board/samsung/trats/trats.c b/board/samsung/trats/trats.c
index 7f61d17..9b6dd12 100644
--- a/board/samsung/trats/trats.c
+++ b/board/samsung/trats/trats.c
@@ -408,11 +408,31 @@  int checkboard(void)
 #endif
 
 #ifdef CONFIG_GENERIC_MMC
+static int sd_ldo_control(int on)
+{
+	struct pmic *p = pmic_get("MAX8997_PMIC");
+	u32 val = 0;
+	int ret = 0;
+	if (pmic_probe(p))
+		return -1;
+
+	/* LDO17 VTF: 2.8V */
+	val = max8997_reg_ldo(2800000) | (on? EN_LDO : DIS_LDO);
+	ret |= pmic_reg_write(p, MAX8997_REG_LDO17CTRL, val);
+
+	if (ret) {
+		puts("MAX8997 SD setting error!\n");
+		return -1;
+	}
+
+	return 0;
+}
+
 int board_mmc_init(bd_t *bis)
 {
 	struct exynos4_gpio_part2 *gpio =
 		(struct exynos4_gpio_part2 *)samsung_get_base_gpio_part2();
-	int err;
+	int err, err_sd = 0;
 
 	/* eMMC_EN: SD_0_CDn: GPK0[2] Output High */
 	s5p_gpio_direction_output(&gpio->k0, 2, 1);
@@ -438,14 +458,20 @@  int board_mmc_init(bd_t *bis)
 	 * GPX3[4] T-flash detect pin
 	 */
 	if (!s5p_gpio_get_value(&gpio->x3, 4)) {
-		err = exynos_pinmux_config(PERIPH_ID_SDMMC2, PINMUX_FLAG_NONE);
-		if (err)
+		err_sd = exynos_pinmux_config(PERIPH_ID_SDMMC2, PINMUX_FLAG_NONE);
+		if (err_sd)
 			debug("SDMMC2 not configured\n");
-		else
-			err = s5p_mmc_init(2, 4);
+		else {
+			if (!sd_ldo_control(1)) {
+				err_sd = s5p_mmc_init(2, 4);
+				if (err_sd) {
+					sd_ldo_control(0);
+				}
+			}
+		}
 	}
 
-	return err;
+	return err & err_sd;
 }
 #endif