[U-Boot] [PATCH 4/4] imx: mx6: Set Pfuze mode to decrease power number for DSM
Li Ye-B37916
b37916 at freescale.com
Wed Nov 5 10:34:50 CET 2014
Hi Przemyslaw,
On 11/4/2014 11:57 PM, Przemyslaw Marczak wrote:
> Hello Ye Li,
>
> On 09/10/2014 11:08 AM, Ye.Li wrote:
>> Set all switches APS mode in normal and PFM mode in standby. So when
>> mx6 entering DSM mode, the power number can be decreased. There is
>> no impact for mx6 in run mode.
>>
>> Changes for boards:
>> -mx6 sabreauto
>> -mx6 sabresd
>> -mx6slevk
>> -mx6sxsabresd
>>
>> Signed-off-by: Ye.Li <B37916 at freescale.com>
>> ---
>> board/freescale/mx6qsabreauto/mx6qsabreauto.c | 36 +++++++++++++++++++++++++
>> board/freescale/mx6sabresd/mx6sabresd.c | 36 +++++++++++++++++++++++++
>> board/freescale/mx6slevk/mx6slevk.c | 36 +++++++++++++++++++++++++
>> board/freescale/mx6sxsabresd/mx6sxsabresd.c | 36 +++++++++++++++++++++++++
>> 4 files changed, 144 insertions(+), 0 deletions(-)
>>
>> diff --git a/board/freescale/mx6qsabreauto/mx6qsabreauto.c b/board/freescale/mx6qsabreauto/mx6qsabreauto.c
>> index 76b024b..9e79915 100644
>> --- a/board/freescale/mx6qsabreauto/mx6qsabreauto.c
>> +++ b/board/freescale/mx6qsabreauto/mx6qsabreauto.c
>> @@ -263,6 +263,37 @@ int board_init(void)
>> return 0;
>> }
>>
>> +/* set all switches APS in normal and PFM mode in standby */
>
> I think that the "chip" is quite misleading.
> Just use chipid and check the real value instead of using it as bool.
> And the PF100 DEVICEID should be defined in pf100 header.
>
Accept. Will change it in next version.
>> +static int pfuze_setup_mode(struct pmic *p, int chip)
>> +{
>> + unsigned char offset, i, switch_num, value;
>> +
>> + if (!chip) {
>> + /* pfuze100 */
>> + switch_num = 6;
>> + offset = 0x31;
>> + } else {
>> + /* pfuze200 */
>> + switch_num = 4;
>> + offset = 0x38;
>> + }
>> +
>> + value = 0xc;
>
> If you change "magic" values (e.g. 0xc) with proper defines in pmic header, then it could be reused in the future and the code will be more readable.
>
Ok.
>> + if (pmic_reg_write(p, 0x23, value)) {
>> + printf("Set SW1AB mode error!\n");
>> + return -1;
>> + }
>> +
>> + for (i = 0; i < switch_num - 1; i++) {
>> + if (pmic_reg_write(p, offset + i * 7, value)) {
>> + printf("Set switch%x mode error!\n", offset);
>> + return -1;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>
> The function above is duplicated few times in each board code.
> Why not make it common and just one?
>
I already have made it as a common function in v4. The patch you reviewed seems not the latest one. I will address your other comments and send out v5.
>> +
>> static int pfuze_init(void)
>> {
>> struct pmic *p;
>> @@ -281,6 +312,11 @@ static int pfuze_init(void)
>> pmic_reg_read(p, PFUZE100_DEVICEID, ®);
>> printf("PMIC: PFUZE100 ID=0x%02x\n", reg);
>>
>> + if (pfuze_setup_mode(p, (reg & 0xf))) {
>> + printf("setup pfuze mode error!\n");
>> + return -1;
>> + }
>> +
>> /* Set SW1AB stanby volage to 0.975V */
>> pmic_reg_read(p, PFUZE100_SW1ABSTBY, ®);
>> reg &= ~0x3f;
>> diff --git a/board/freescale/mx6sabresd/mx6sabresd.c b/board/freescale/mx6sabresd/mx6sabresd.c
>> index 72d6562..810fe13 100644
>> --- a/board/freescale/mx6sabresd/mx6sabresd.c
>> +++ b/board/freescale/mx6sabresd/mx6sabresd.c
>> @@ -456,6 +456,37 @@ int board_init(void)
>> return 0;
>> }
>>
>> +/* set all switches APS in normal and PFM mode in standby */
>> +static int pfuze_setup_mode(struct pmic *p, int chip)
>> +{
>> + unsigned char offset, i, switch_num, value;
>> +
>> + if (!chip) {
>> + /* pfuze100 */
>> + switch_num = 6;
>> + offset = 0x31;
>> + } else {
>> + /* pfuze200 */
>> + switch_num = 4;
>> + offset = 0x38;
>> + }
>> +
>> + value = 0xc;
>> + if (pmic_reg_write(p, 0x23, value)) {
>> + printf("Set SW1AB mode error!\n");
>> + return -1;
>> + }
>> +
>> + for (i = 0; i < switch_num - 1; i++) {
>> + if (pmic_reg_write(p, offset + i * 7, value)) {
>> + printf("Set switch%x mode error!\n", offset);
>> + return -1;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static int pfuze_init(void)
>> {
>> struct pmic *p;
>> @@ -475,6 +506,11 @@ static int pfuze_init(void)
>> printf("PMIC: PFUZE%s ID=0x%02x\n",
>> ((reg & 0xf) == 0) ? "100" : "200", reg);
>>
>> + if (pfuze_setup_mode(p, (reg & 0xf))) {
>> + printf("setup pfuze mode error!\n");
>> + return -1;
>> + }
>> +
>> /* Increase VGEN3 from 2.5 to 2.8V */
>> pmic_reg_read(p, PFUZE100_VGEN3VOL, ®);
>> reg &= ~0xf;
>> diff --git a/board/freescale/mx6slevk/mx6slevk.c b/board/freescale/mx6slevk/mx6slevk.c
>> index 8b6a79c..fe5e37d 100644
>> --- a/board/freescale/mx6slevk/mx6slevk.c
>> +++ b/board/freescale/mx6slevk/mx6slevk.c
>> @@ -195,6 +195,37 @@ int board_init(void)
>> return 0;
>> }
>>
>> +/* set all switches APS in normal and PFM mode in standby */
>> +static int pfuze_setup_mode(struct pmic *p, int chip)
>> +{
>> + unsigned char offset, i, switch_num, value;
>> +
>> + if (!chip) {
>> + /* pfuze100 */
>> + switch_num = 6;
>> + offset = 0x31;
>> + } else {
>> + /* pfuze200 */
>> + switch_num = 4;
>> + offset = 0x38;
>> + }
>> +
>> + value = 0xc;
>> + if (pmic_reg_write(p, 0x23, value)) {
>> + printf("Set SW1AB mode error!\n");
>> + return -1;
>> + }
>> +
>> + for (i = 0; i < switch_num - 1; i++) {
>> + if (pmic_reg_write(p, offset + i * 7, value)) {
>> + printf("Set switch%x mode error!\n", offset);
>> + return -1;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static int pfuze_init(void)
>> {
>> struct pmic *p;
>> @@ -214,6 +245,11 @@ static int pfuze_init(void)
>> printf("PMIC: PFUZE%s ID=0x%02x\n",
>> ((reg & 0xf) == 0) ? "100" : "200", reg);
>>
>> + if (pfuze_setup_mode(p, (reg & 0xf))) {
>> + printf("setup pfuze mode error!\n");
>> + return -1;
>> + }
>> +
>> /* Set SW1AB stanby volage to 0.975V */
>> pmic_reg_read(p, PFUZE100_SW1ABSTBY, ®);
>> reg &= ~0x3f;
>> diff --git a/board/freescale/mx6sxsabresd/mx6sxsabresd.c b/board/freescale/mx6sxsabresd/mx6sxsabresd.c
>> index 80d2d99..d6a33cd 100644
>> --- a/board/freescale/mx6sxsabresd/mx6sxsabresd.c
>> +++ b/board/freescale/mx6sxsabresd/mx6sxsabresd.c
>> @@ -170,6 +170,37 @@ struct i2c_pads_info i2c_pad_info1 = {
>> },
>> };
>>
>> +/* set all switches APS in normal and PFM mode in standby */
>> +static int pfuze_setup_mode(struct pmic *p, int chip)
>> +{
>> + unsigned char offset, i, switch_num, value;
>> +
>> + if (!chip) {
>> + /* pfuze100 */
>> + switch_num = 6;
>> + offset = 0x31;
>> + } else {
>> + /* pfuze200 */
>> + switch_num = 4;
>> + offset = 0x38;
>> + }
>> +
>> + value = 0xc;
>> + if (pmic_reg_write(p, 0x23, value)) {
>> + printf("Set SW1AB mode error!\n");
>> + return -1;
>> + }
>> +
>> + for (i = 0; i < switch_num - 1; i++) {
>> + if (pmic_reg_write(p, offset + i * 7, value)) {
>> + printf("Set switch%x mode error!\n", offset);
>> + return -1;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static int pfuze_init(void)
>> {
>> struct pmic *p;
>> @@ -188,6 +219,11 @@ static int pfuze_init(void)
>> pmic_reg_read(p, PFUZE100_DEVICEID, ®);
>> printf("PMIC: PFUZE100 ID=0x%02x\n", reg);
>>
>> + if (pfuze_setup_mode(p, (reg & 0xf))) {
>> + printf("setup pfuze mode error!\n");
>> + return -1;
>> + }
>> +
>> /* Set SW1AB standby voltage to 0.975V */
>> pmic_reg_read(p, PFUZE100_SW1ABSTBY, ®);
>> reg &= ~0x3f;
>>
>
> And as Fabio wrote, please return real error values or use errno instead of "-1".
>
Have fixed it in v4 patch.
> Best regards,
Best regards,
Ye Li
More information about the U-Boot
mailing list