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

[Jaehoon Chung]

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 at samsung.com>
>>>> Signed-off-by: Kyungmin Park <kyungmin.park at 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
>