[U-Boot] [PATCH 1/3] imx-common: fix iomux settings

[U-Boot] [PATCH 1/3] imx-common: fix iomux settings

[Peng Fan]

When setting iomux for a pin mux, there is no need to check mux_ctrl_ofs.
Also If still checking mux_ctrl_ofs, we have no chance to set iomux
for i.MX7D IOMUXC_LPSR_SW_MUX_CTL_PAD_GPIO1_IO00, because the mux_ctrl_ofs
for this register is 0.

Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
Cc: Stefano Babic <sbabic@denx.de>
Cc: Fabio Estevam <fabio.estevam@freescale.com>
---
 arch/arm/imx-common/iomux-v3.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm/imx-common/iomux-v3.c b/arch/arm/imx-common/iomux-v3.c
index b4f481f..9b9cf58 100644
--- a/arch/arm/imx-common/iomux-v3.c
+++ b/arch/arm/imx-common/iomux-v3.c
@@ -53,8 +53,7 @@ void imx_iomux_v3_setup_pad(iomux_v3_cfg_t pad)
 	}
 #endif
 
-	if (mux_ctrl_ofs)
-		__raw_writel(mux_mode, base + mux_ctrl_ofs);
+	__raw_writel(mux_mode, base + mux_ctrl_ofs);
 
 	if (sel_input_ofs)
 		__raw_writel(sel_input, base + sel_input_ofs);
-- 
1.8.4

[U-Boot] [PATCH 2/3] imx: wdog: correct wcr register settings

[Peng Fan]

We should not simple use "writew(WCR_WDE, &wdog->wcr)" to set
wcr, since this will override bits set before reset_cpu.

Use clrsetbits_le32 instead of writew to fix this issue.

Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
Cc: Stefano Babic <sbabic@denx.de>
Cc: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Sebastian Siewior <bigeasy@linutronix.de>
---
 drivers/watchdog/imx_watchdog.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/imx_watchdog.c b/drivers/watchdog/imx_watchdog.c
index 1d18d4b..9a77a54 100644
--- a/drivers/watchdog/imx_watchdog.c
+++ b/drivers/watchdog/imx_watchdog.c
@@ -55,7 +55,8 @@ void reset_cpu(ulong addr)
 {
 	struct watchdog_regs *wdog = (struct watchdog_regs *)WDOG1_BASE_ADDR;
 
-	writew(WCR_WDE, &wdog->wcr);
+	clrsetbits_le16(&wdog->wcr, 0, WCR_WDE);
+
 	writew(0x5555, &wdog->wsr);
 	writew(0xaaaa, &wdog->wsr);	/* load minimum 1/2 second timeout */
 	while (1) {
-- 
1.8.4

[U-Boot] [PATCH 2/3] imx: wdog: correct wcr register settings

[Fabio Estevam]

On Mon, Sep 14, 2015 at 2:34 AM, Peng Fan <Peng.Fan@freescale.com> wrote:
> We should not simple use "writew(WCR_WDE, &wdog->wcr)" to set
> wcr, since this will override bits set before reset_cpu.
>
> Use clrsetbits_le32 instead of writew to fix this issue.
>
> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
> Cc: Stefano Babic <sbabic@denx.de>
> Cc: Fabio Estevam <fabio.estevam@freescale.com>
> Cc: Sebastian Siewior <bigeasy@linutronix.de>

Tested-by: Fabio Estevam <fabio.estevam@freescale.com>

[U-Boot] [PATCH 2/3] imx: wdog: correct wcr register settings

[Fabio Estevam]

On Mon, Sep 14, 2015 at 2:34 AM, Peng Fan <Peng.Fan@freescale.com> wrote:
> We should not simple use "writew(WCR_WDE, &wdog->wcr)" to set
> wcr, since this will override bits set before reset_cpu.
>
> Use clrsetbits_le32 instead of writew to fix this issue.

There is a typo here: it should be clrsetbits_le16.

[U-Boot] [PATCH 2/3] imx: wdog: correct wcr register settings

[Peng Fan]

On Mon, Sep 14, 2015 at 09:11:30AM -0300, Fabio Estevam wrote:
>On Mon, Sep 14, 2015 at 2:34 AM, Peng Fan <Peng.Fan@freescale.com> wrote:
>> We should not simple use "writew(WCR_WDE, &wdog->wcr)" to set
>> wcr, since this will override bits set before reset_cpu.
>>
>> Use clrsetbits_le32 instead of writew to fix this issue.
>
>There is a typo here: it should be clrsetbits_le16.

Fabio, thanks for pointing this out.

Stefano, can you help fix this when you plan to apply this patch?
Or do you need to send v2 to fix this commit log typo?

Thanks,
Peng.
-- 

[U-Boot] [PATCH 2/3] imx: wdog: correct wcr register settings

[Stefano Babic]

On 14/09/2015 14:11, Fabio Estevam wrote:
> On Mon, Sep 14, 2015 at 2:34 AM, Peng Fan <Peng.Fan@freescale.com> wrote:
>> We should not simple use "writew(WCR_WDE, &wdog->wcr)" to set
>> wcr, since this will override bits set before reset_cpu.
>>
>> Use clrsetbits_le32 instead of writew to fix this issue.
> 
> There is a typo here: it should be clrsetbits_le16.
> 


Corrected by applying, thanks to point it out.

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH 3/3] imx: mx7dsabresd set wdog SRS bit

[Peng Fan]

We use trigger pmic reset to reset the board, so set bit SRS to
disable internal WDOG_RESET_B_DEB to make reset stable.

Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
Cc: Stefano Babic <sbabic@denx.de>
Cc: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Adrian Alonso <aalonso@freescale.com>
---
 board/freescale/mx7dsabresd/mx7dsabresd.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/board/freescale/mx7dsabresd/mx7dsabresd.c b/board/freescale/mx7dsabresd/mx7dsabresd.c
index d163bee..6d88573 100644
--- a/board/freescale/mx7dsabresd/mx7dsabresd.c
+++ b/board/freescale/mx7dsabresd/mx7dsabresd.c
@@ -499,6 +499,8 @@ int power_init_board(void)
 
 int board_late_init(void)
 {
+	struct wdog_regs *wdog = (struct wdog_regs *)WDOG1_BASE_ADDR;
+
 #ifdef CONFIG_CMD_BMODE
 	add_board_boot_modes(board_boot_modes);
 #endif
@@ -509,7 +511,13 @@ int board_late_init(void)
 
 	imx_iomux_v3_setup_multiple_pads(wdog_pads, ARRAY_SIZE(wdog_pads));
 
-	set_wdog_reset((struct wdog_regs *)WDOG1_BASE_ADDR);
+	set_wdog_reset(wdog);
+
+	/*
+	 * Do not assert internal WDOG_RESET_B_DEB(controlled by bit 4),
+	 * since we use PMIC_PWRON to reset the board.
+	 */
+	clrsetbits_le16(&wdog->wcr, 0, 0x10);
 
 	return 0;
 }
-- 
1.8.4

[U-Boot] [PATCH 3/3] imx: mx7dsabresd set wdog SRS bit

[Fabio Estevam]

On Mon, Sep 14, 2015 at 2:34 AM, Peng Fan <Peng.Fan@freescale.com> wrote:
> We use trigger pmic reset to reset the board, so set bit SRS to
> disable internal WDOG_RESET_B_DEB to make reset stable.
>
> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
> Cc: Stefano Babic <sbabic@denx.de>
> Cc: Fabio Estevam <fabio.estevam@freescale.com>
> Cc: Adrian Alonso <aalonso@freescale.com>

Tested-by: Fabio Estevam <fabio.estevam@freescale.com>

[U-Boot] [PATCH 1/3] imx-common: fix iomux settings

[Fabio Estevam]

On Mon, Sep 14, 2015 at 2:34 AM, Peng Fan <Peng.Fan@freescale.com> wrote:
> When setting iomux for a pin mux, there is no need to check mux_ctrl_ofs.
> Also If still checking mux_ctrl_ofs, we have no chance to set iomux
> for i.MX7D IOMUXC_LPSR_SW_MUX_CTL_PAD_GPIO1_IO00, because the mux_ctrl_ofs
> for this register is 0.
>
> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
> Cc: Stefano Babic <sbabic@denx.de>
> Cc: Fabio Estevam <fabio.estevam@freescale.com>

Tested-by: Fabio Estevam <fabio.estevam@freescale.com>

[U-Boot] [PATCH 1/3] imx-common: fix iomux settings

[Stefano Babic]

On 14/09/2015 07:34, Peng Fan wrote:
> When setting iomux for a pin mux, there is no need to check mux_ctrl_ofs.
> Also If still checking mux_ctrl_ofs, we have no chance to set iomux
> for i.MX7D IOMUXC_LPSR_SW_MUX_CTL_PAD_GPIO1_IO00, because the mux_ctrl_ofs
> for this register is 0.
> 
> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
> Cc: Stefano Babic <sbabic@denx.de>
> Cc: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>  arch/arm/imx-common/iomux-v3.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/arm/imx-common/iomux-v3.c b/arch/arm/imx-common/iomux-v3.c
> index b4f481f..9b9cf58 100644
> --- a/arch/arm/imx-common/iomux-v3.c
> +++ b/arch/arm/imx-common/iomux-v3.c
> @@ -53,8 +53,7 @@ void imx_iomux_v3_setup_pad(iomux_v3_cfg_t pad)
>  	}
>  #endif
>  
> -	if (mux_ctrl_ofs)
> -		__raw_writel(mux_mode, base + mux_ctrl_ofs);
> +	__raw_writel(mux_mode, base + mux_ctrl_ofs);
>  
>  	if (sel_input_ofs)
>  		__raw_writel(sel_input, base + sel_input_ofs);
> 

Applied (whole series) to u-boot-imx, thanks !

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH 1/3] imx-common: fix iomux settings

[Benoît Thébaudeau]

Hi Stefano, Peng, Fabio, all,

Sorry for seeing this only now, but...

On Sun, Sep 20, 2015 at 9:43 AM, Stefano Babic <sbabic@denx.de> wrote:
>
>
> On 14/09/2015 07:34, Peng Fan wrote:
>> When setting iomux for a pin mux, there is no need to check mux_ctrl_ofs.

This assumption is wrong. This check was there for a reason. Some i.MX
SoCs have some registers controlling pads but not muxes, either for a
single pin or for groups of pins:
http://git.denx.de/?p=u-boot/u-boot-imx.git;a=blob;f=arch/arm/include/asm/arch-mx25/iomux-mx25.h;h=220cf4ef2e94aa69482557852ed0cc0690a79cec;hb=HEAD
http://git.denx.de/?p=u-boot/u-boot-imx.git;a=blob;f=arch/arm/include/asm/arch-mx35/iomux-mx35.h;h=5898b46f4720088b18882e21d0d2424fff987ab5;hb=HEAD
http://git.denx.de/?p=u-boot/u-boot-imx.git;a=blob;f=arch/arm/include/asm/arch-mx5/iomux-mx51.h;h=b7b169505f91c4a213be59efca47e8a5aed770e7;hb=HEAD

I have not checked whether these cases are currently used in-tree by
U-Boot, but they have to be possible anyway in order to support these
SoCs.

>> Also If still checking mux_ctrl_ofs, we have no chance to set iomux
>> for i.MX7D IOMUXC_LPSR_SW_MUX_CTL_PAD_GPIO1_IO00, because the mux_ctrl_ofs
>> for this register is 0.

The need is clear, but then the test mechanism should be changed, not
removed. You could find a free bit in mux_ctrl_ofs or in mux_mode or
elsewhere in IOMUX_PAD (e.g. bit 63, which is currently reserved),
something like NO_PAD_CTRL, or create a reserved value other than
__NA_ for mux_ctrl_ofs/mux_mode.

>> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
>> Cc: Stefano Babic <sbabic@denx.de>
>> Cc: Fabio Estevam <fabio.estevam@freescale.com>
>> ---
>>  arch/arm/imx-common/iomux-v3.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/imx-common/iomux-v3.c b/arch/arm/imx-common/iomux-v3.c
>> index b4f481f..9b9cf58 100644
>> --- a/arch/arm/imx-common/iomux-v3.c
>> +++ b/arch/arm/imx-common/iomux-v3.c
>> @@ -53,8 +53,7 @@ void imx_iomux_v3_setup_pad(iomux_v3_cfg_t pad)
>>       }
>>  #endif
>>
>> -     if (mux_ctrl_ofs)
>> -             __raw_writel(mux_mode, base + mux_ctrl_ofs);
>> +     __raw_writel(mux_mode, base + mux_ctrl_ofs);
>>
>>       if (sel_input_ofs)
>>               __raw_writel(sel_input, base + sel_input_ofs);
>>
>
> Applied (whole series) to u-boot-imx, thanks !

Please fix.

Best regards,
Beno?t

[U-Boot] [PATCH 1/3] imx-common: fix iomux settings

[Peng Fan]

On Sun, Sep 20, 2015 at 01:33:20PM +0200, Beno?t Th?baudeau wrote:
>Hi Stefano, Peng, Fabio, all,
>
>Sorry for seeing this only now, but...
>
>On Sun, Sep 20, 2015 at 9:43 AM, Stefano Babic <sbabic@denx.de> wrote:
>>
>>
>> On 14/09/2015 07:34, Peng Fan wrote:
>>> When setting iomux for a pin mux, there is no need to check mux_ctrl_ofs.
>
>This assumption is wrong. This check was there for a reason. Some i.MX
>SoCs have some registers controlling pads but not muxes, either for a
>single pin or for groups of pins:
>http://git.denx.de/?p=u-boot/u-boot-imx.git;a=blob;f=arch/arm/include/asm/arch-mx25/iomux-mx25.h;h=220cf4ef2e94aa69482557852ed0cc0690a79cec;hb=HEAD
>http://git.denx.de/?p=u-boot/u-boot-imx.git;a=blob;f=arch/arm/include/asm/arch-mx35/iomux-mx35.h;h=5898b46f4720088b18882e21d0d2424fff987ab5;hb=HEAD
>http://git.denx.de/?p=u-boot/u-boot-imx.git;a=blob;f=arch/arm/include/asm/arch-mx5/iomux-mx51.h;h=b7b169505f91c4a213be59efca47e8a5aed770e7;hb=HEAD
>
>I have not checked whether these cases are currently used in-tree by
>U-Boot, but they have to be possible anyway in order to support these
>SoCs.

Beno?t,

Thanks for pointing this out.
You mean piece of code like this, right?
509         MX25_PAD_CTL_GRP_DVS_MISC               = IOMUX_PAD(0x418, 0x000, 0, 0, 0, NO_PAD_CTRL),
510         MX25_PAD_CTL_GRP_DSE_FEC                = IOMUX_PAD(0x41c, 0x000, 0, 0, 0, NO_PAD_CTRL),
511         MX25_PAD_CTL_GRP_DVS_JTAG               = IOMUX_PAD(0x420, 0x000, 0, 0, 0, NO_PAD_CTRL),
512         MX25_PAD_CTL_GRP_DSE_NFC                = IOMUX_PAD(0x424, 0x000, 0, 0, 0, NO_PAD_CTRL),
513         MX25_PAD_CTL_GRP_DSE_CSI                = IOMUX_PAD(0x428, 0x000, 0, 0, 0, NO_PAD_CTRL),
514         MX25_PAD_CTL_GRP_DSE_WEIM               = IOMUX_PAD(0x42c, 0x000, 0, 0, 0, NO_PAD_CTRL),
515         MX25_PAD_CTL_GRP_DSE_DDR                = IOMUX_PAD(0x430, 0x000, 0, 0, 0, NO_PAD_CTRL),
516         MX25_PAD_CTL_GRP_DVS_CRM                = IOMUX_PAD(0x434, 0x000, 0, 0, 0, NO_PAD_CTRL),
517         MX25_PAD_CTL_GRP_DSE_KPP                = IOMUX_PAD(0x438, 0x000, 0, 0, 0, NO_PAD_CTRL),
518         MX25_PAD_CTL_GRP_DSE_SDHC1              = IOMUX_PAD(0x43c, 0x000, 0, 0, 0, NO_PAD_CTRL),
519         MX25_PAD_CTL_GRP_DSE_LCD                = IOMUX_PAD(0x440, 0x000, 0, 0, 0, NO_PAD_CTRL),
520         MX25_PAD_CTL_GRP_DSE_UART               = IOMUX_PAD(0x444, 0x000, 0, 0, 0, NO_PAD_CTRL),
521         MX25_PAD_CTL_GRP_DVS_NFC                = IOMUX_PAD(0x448, 0x000, 0, 0, 0, NO_PAD_CTRL),
522         MX25_PAD_CTL_GRP_DVS_CSI                = IOMUX_PAD(0x44c, 0x000, 0, 0, 0, NO_PAD_CTRL),
523         MX25_PAD_CTL_GRP_DSE_CSPI1              = IOMUX_PAD(0x450, 0x000, 0, 0, 0, NO_PAD_CTRL),
524         MX25_PAD_CTL_GRP_DDRTYPE                = IOMUX_PAD(0x454, 0x000, 0, 0, 0, NO_PAD_CTRL),
525         MX25_PAD_CTL_GRP_DVS_SDHC1              = IOMUX_PAD(0x458, 0x000, 0, 0, 0, NO_PAD_CTRL),
526         MX25_PAD_CTL_GRP_DVS_LCD                = IOMUX_PAD(0x45c, 0x000, 0, 0, 0, NO_PAD_CTRL)

My bad. I only took i.mx6/7 into consideration when working this patch.
>
>>> Also If still checking mux_ctrl_ofs, we have no chance to set iomux
>>> for i.MX7D IOMUXC_LPSR_SW_MUX_CTL_PAD_GPIO1_IO00, because the mux_ctrl_ofs
>>> for this register is 0.
>
>The need is clear, but then the test mechanism should be changed, not
>removed. You could find a free bit in mux_ctrl_ofs or in mux_mode or
>elsewhere in IOMUX_PAD (e.g. bit 63, which is currently reserved),
>something like NO_PAD_CTRL, or create a reserved value other than
>__NA_ for mux_ctrl_ofs/mux_mode.

Stefano,

There is '#define NO_PAD_CTRL             (1 << 17)' now,
we can add'NO_MUX_CTRL' and 'NO_SEL_CTRL(select input)', but need to check
whether the __NA__ pads are used or not now.
also need a big change for the layout and related macro definition:
39  * MUX_CTRL_OFS:            0..11 (12)
40  * PAD_CTRL_OFS:           12..23 (12)
41  * SEL_INPUT_OFS:          24..35 (12)
42  * MUX_MODE + SION:        36..40  (5)
43  * PAD_CTRL + NO_PAD_CTRL: 41..58 (18)
44  * SEL_INP:                59..62  (4)
45  * reserved:                 63    (1)

Can we just use the following way, since only i.mx7 has the requirement of
mux_ctrl_ofs maybe at 0. 
if (is_soc_type(MX7)) {
	__raw_writel(mux_mode, base + mux_ctrl_ofs);
} else {
	if (mux_ctrl_ofs)
        __raw_writel(mux_mode, base + mux_ctrl_ofs);
}
I prefer this simple way for now, since we are@RC2 now. Later we can
refactor the code using the way to provide macros NO_MUX_CTRL or NO_SEL_CTRL.
What do you think?

Regards,
Peng.

>
>>> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
>>> Cc: Stefano Babic <sbabic@denx.de>
>>> Cc: Fabio Estevam <fabio.estevam@freescale.com>
>>> ---
>>>  arch/arm/imx-common/iomux-v3.c | 3 +--
>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm/imx-common/iomux-v3.c b/arch/arm/imx-common/iomux-v3.c
>>> index b4f481f..9b9cf58 100644
>>> --- a/arch/arm/imx-common/iomux-v3.c
>>> +++ b/arch/arm/imx-common/iomux-v3.c
>>> @@ -53,8 +53,7 @@ void imx_iomux_v3_setup_pad(iomux_v3_cfg_t pad)
>>>       }
>>>  #endif
>>>
>>> -     if (mux_ctrl_ofs)
>>> -             __raw_writel(mux_mode, base + mux_ctrl_ofs);
>>> +     __raw_writel(mux_mode, base + mux_ctrl_ofs);
>>>
>>>       if (sel_input_ofs)
>>>               __raw_writel(sel_input, base + sel_input_ofs);
>>>
>>
>> Applied (whole series) to u-boot-imx, thanks !
>
>Please fix.
>
>Best regards,
>Beno?t

-- 

[U-Boot] [PATCH 1/3] imx-common: fix iomux settings

[Stefano Babic]

Hi Peng,

On 20/09/2015 15:02, Peng Fan wrote:
> On Sun, Sep 20, 2015 at 01:33:20PM +0200, Beno?t Th?baudeau wrote:
>> Hi Stefano, Peng, Fabio, all,
>>
>> Sorry for seeing this only now, but...
>>
>> On Sun, Sep 20, 2015 at 9:43 AM, Stefano Babic <sbabic@denx.de> wrote:
>>>
>>>
>>> On 14/09/2015 07:34, Peng Fan wrote:
>>>> When setting iomux for a pin mux, there is no need to check mux_ctrl_ofs.
>>
>> This assumption is wrong. This check was there for a reason. Some i.MX
>> SoCs have some registers controlling pads but not muxes, either for a
>> single pin or for groups of pins:
>> http://git.denx.de/?p=u-boot/u-boot-imx.git;a=blob;f=arch/arm/include/asm/arch-mx25/iomux-mx25.h;h=220cf4ef2e94aa69482557852ed0cc0690a79cec;hb=HEAD
>> http://git.denx.de/?p=u-boot/u-boot-imx.git;a=blob;f=arch/arm/include/asm/arch-mx35/iomux-mx35.h;h=5898b46f4720088b18882e21d0d2424fff987ab5;hb=HEAD
>> http://git.denx.de/?p=u-boot/u-boot-imx.git;a=blob;f=arch/arm/include/asm/arch-mx5/iomux-mx51.h;h=b7b169505f91c4a213be59efca47e8a5aed770e7;hb=HEAD
>>
>> I have not checked whether these cases are currently used in-tree by
>> U-Boot, but they have to be possible anyway in order to support these
>> SoCs.
> 
> Beno?t,
> 
> Thanks for pointing this out.
> You mean piece of code like this, right?
> 509         MX25_PAD_CTL_GRP_DVS_MISC               = IOMUX_PAD(0x418, 0x000, 0, 0, 0, NO_PAD_CTRL),
> 510         MX25_PAD_CTL_GRP_DSE_FEC                = IOMUX_PAD(0x41c, 0x000, 0, 0, 0, NO_PAD_CTRL),
> 511         MX25_PAD_CTL_GRP_DVS_JTAG               = IOMUX_PAD(0x420, 0x000, 0, 0, 0, NO_PAD_CTRL),
> 512         MX25_PAD_CTL_GRP_DSE_NFC                = IOMUX_PAD(0x424, 0x000, 0, 0, 0, NO_PAD_CTRL),
> 513         MX25_PAD_CTL_GRP_DSE_CSI                = IOMUX_PAD(0x428, 0x000, 0, 0, 0, NO_PAD_CTRL),
> 514         MX25_PAD_CTL_GRP_DSE_WEIM               = IOMUX_PAD(0x42c, 0x000, 0, 0, 0, NO_PAD_CTRL),
> 515         MX25_PAD_CTL_GRP_DSE_DDR                = IOMUX_PAD(0x430, 0x000, 0, 0, 0, NO_PAD_CTRL),
> 516         MX25_PAD_CTL_GRP_DVS_CRM                = IOMUX_PAD(0x434, 0x000, 0, 0, 0, NO_PAD_CTRL),
> 517         MX25_PAD_CTL_GRP_DSE_KPP                = IOMUX_PAD(0x438, 0x000, 0, 0, 0, NO_PAD_CTRL),
> 518         MX25_PAD_CTL_GRP_DSE_SDHC1              = IOMUX_PAD(0x43c, 0x000, 0, 0, 0, NO_PAD_CTRL),
> 519         MX25_PAD_CTL_GRP_DSE_LCD                = IOMUX_PAD(0x440, 0x000, 0, 0, 0, NO_PAD_CTRL),
> 520         MX25_PAD_CTL_GRP_DSE_UART               = IOMUX_PAD(0x444, 0x000, 0, 0, 0, NO_PAD_CTRL),
> 521         MX25_PAD_CTL_GRP_DVS_NFC                = IOMUX_PAD(0x448, 0x000, 0, 0, 0, NO_PAD_CTRL),
> 522         MX25_PAD_CTL_GRP_DVS_CSI                = IOMUX_PAD(0x44c, 0x000, 0, 0, 0, NO_PAD_CTRL),
> 523         MX25_PAD_CTL_GRP_DSE_CSPI1              = IOMUX_PAD(0x450, 0x000, 0, 0, 0, NO_PAD_CTRL),
> 524         MX25_PAD_CTL_GRP_DDRTYPE                = IOMUX_PAD(0x454, 0x000, 0, 0, 0, NO_PAD_CTRL),
> 525         MX25_PAD_CTL_GRP_DVS_SDHC1              = IOMUX_PAD(0x458, 0x000, 0, 0, 0, NO_PAD_CTRL),
> 526         MX25_PAD_CTL_GRP_DVS_LCD                = IOMUX_PAD(0x45c, 0x000, 0, 0, 0, NO_PAD_CTRL)
> 
> My bad. I only took i.mx6/7 into consideration when working this patch.
>>
>>>> Also If still checking mux_ctrl_ofs, we have no chance to set iomux
>>>> for i.MX7D IOMUXC_LPSR_SW_MUX_CTL_PAD_GPIO1_IO00, because the mux_ctrl_ofs
>>>> for this register is 0.
>>
>> The need is clear, but then the test mechanism should be changed, not
>> removed. You could find a free bit in mux_ctrl_ofs or in mux_mode or
>> elsewhere in IOMUX_PAD (e.g. bit 63, which is currently reserved),
>> something like NO_PAD_CTRL, or create a reserved value other than
>> __NA_ for mux_ctrl_ofs/mux_mode.
> 
> Stefano,
> 
> There is '#define NO_PAD_CTRL             (1 << 17)' now,
> we can add'NO_MUX_CTRL' and 'NO_SEL_CTRL(select input)', but need to check
> whether the __NA__ pads are used or not now.
> also need a big change for the layout and related macro definition:

Right

> 39  * MUX_CTRL_OFS:            0..11 (12)
> 40  * PAD_CTRL_OFS:           12..23 (12)
> 41  * SEL_INPUT_OFS:          24..35 (12)
> 42  * MUX_MODE + SION:        36..40  (5)
> 43  * PAD_CTRL + NO_PAD_CTRL: 41..58 (18)
> 44  * SEL_INP:                59..62  (4)
> 45  * reserved:                 63    (1)
> 
> Can we just use the following way, since only i.mx7 has the requirement of
> mux_ctrl_ofs maybe at 0. 
> if (is_soc_type(MX7)) {
> 	__raw_writel(mux_mode, base + mux_ctrl_ofs);
> } else {
> 	if (mux_ctrl_ofs)
>         __raw_writel(mux_mode, base + mux_ctrl_ofs);
> }
> I prefer this simple way for now, since we are at RC2 now. Later we can
> refactor the code using the way to provide macros NO_MUX_CTRL or NO_SEL_CTRL.
> What do you think?

Yes, it is ok for now, green light on my side.

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH 1/3] imx-common: fix iomux settings

[Benoît Thébaudeau]

Hi Peng,

On Sun, Sep 20, 2015 at 3:02 PM, Peng Fan <b51431@freescale.com> wrote:
> On Sun, Sep 20, 2015 at 01:33:20PM +0200, Beno?t Th?baudeau wrote:
>>Hi Stefano, Peng, Fabio, all,
>>
>>Sorry for seeing this only now, but...
>>
>>On Sun, Sep 20, 2015 at 9:43 AM, Stefano Babic <sbabic@denx.de> wrote:
>>>
>>>
>>> On 14/09/2015 07:34, Peng Fan wrote:
>>>> When setting iomux for a pin mux, there is no need to check mux_ctrl_ofs.
>>
>>This assumption is wrong. This check was there for a reason. Some i.MX
>>SoCs have some registers controlling pads but not muxes, either for a
>>single pin or for groups of pins:
>>http://git.denx.de/?p=u-boot/u-boot-imx.git;a=blob;f=arch/arm/include/asm/arch-mx25/iomux-mx25.h;h=220cf4ef2e94aa69482557852ed0cc0690a79cec;hb=HEAD
>>http://git.denx.de/?p=u-boot/u-boot-imx.git;a=blob;f=arch/arm/include/asm/arch-mx35/iomux-mx35.h;h=5898b46f4720088b18882e21d0d2424fff987ab5;hb=HEAD
>>http://git.denx.de/?p=u-boot/u-boot-imx.git;a=blob;f=arch/arm/include/asm/arch-mx5/iomux-mx51.h;h=b7b169505f91c4a213be59efca47e8a5aed770e7;hb=HEAD
>>
>>I have not checked whether these cases are currently used in-tree by
>>U-Boot, but they have to be possible anyway in order to support these
>>SoCs.
>
> Beno?t,
>
> Thanks for pointing this out.
> You mean piece of code like this, right?
> 509         MX25_PAD_CTL_GRP_DVS_MISC               = IOMUX_PAD(0x418, 0x000, 0, 0, 0, NO_PAD_CTRL),
> 510         MX25_PAD_CTL_GRP_DSE_FEC                = IOMUX_PAD(0x41c, 0x000, 0, 0, 0, NO_PAD_CTRL),
> 511         MX25_PAD_CTL_GRP_DVS_JTAG               = IOMUX_PAD(0x420, 0x000, 0, 0, 0, NO_PAD_CTRL),
> 512         MX25_PAD_CTL_GRP_DSE_NFC                = IOMUX_PAD(0x424, 0x000, 0, 0, 0, NO_PAD_CTRL),
> 513         MX25_PAD_CTL_GRP_DSE_CSI                = IOMUX_PAD(0x428, 0x000, 0, 0, 0, NO_PAD_CTRL),
> 514         MX25_PAD_CTL_GRP_DSE_WEIM               = IOMUX_PAD(0x42c, 0x000, 0, 0, 0, NO_PAD_CTRL),
> 515         MX25_PAD_CTL_GRP_DSE_DDR                = IOMUX_PAD(0x430, 0x000, 0, 0, 0, NO_PAD_CTRL),
> 516         MX25_PAD_CTL_GRP_DVS_CRM                = IOMUX_PAD(0x434, 0x000, 0, 0, 0, NO_PAD_CTRL),
> 517         MX25_PAD_CTL_GRP_DSE_KPP                = IOMUX_PAD(0x438, 0x000, 0, 0, 0, NO_PAD_CTRL),
> 518         MX25_PAD_CTL_GRP_DSE_SDHC1              = IOMUX_PAD(0x43c, 0x000, 0, 0, 0, NO_PAD_CTRL),
> 519         MX25_PAD_CTL_GRP_DSE_LCD                = IOMUX_PAD(0x440, 0x000, 0, 0, 0, NO_PAD_CTRL),
> 520         MX25_PAD_CTL_GRP_DSE_UART               = IOMUX_PAD(0x444, 0x000, 0, 0, 0, NO_PAD_CTRL),
> 521         MX25_PAD_CTL_GRP_DVS_NFC                = IOMUX_PAD(0x448, 0x000, 0, 0, 0, NO_PAD_CTRL),
> 522         MX25_PAD_CTL_GRP_DVS_CSI                = IOMUX_PAD(0x44c, 0x000, 0, 0, 0, NO_PAD_CTRL),
> 523         MX25_PAD_CTL_GRP_DSE_CSPI1              = IOMUX_PAD(0x450, 0x000, 0, 0, 0, NO_PAD_CTRL),
> 524         MX25_PAD_CTL_GRP_DDRTYPE                = IOMUX_PAD(0x454, 0x000, 0, 0, 0, NO_PAD_CTRL),
> 525         MX25_PAD_CTL_GRP_DVS_SDHC1              = IOMUX_PAD(0x458, 0x000, 0, 0, 0, NO_PAD_CTRL),
> 526         MX25_PAD_CTL_GRP_DVS_LCD                = IOMUX_PAD(0x45c, 0x000, 0, 0, 0, NO_PAD_CTRL)

Correct.

> My bad. I only took i.mx6/7 into consideration when working this patch.
>>
>>>> Also If still checking mux_ctrl_ofs, we have no chance to set iomux
>>>> for i.MX7D IOMUXC_LPSR_SW_MUX_CTL_PAD_GPIO1_IO00, because the mux_ctrl_ofs
>>>> for this register is 0.
>>
>>The need is clear, but then the test mechanism should be changed, not
>>removed. You could find a free bit in mux_ctrl_ofs or in mux_mode or
>>elsewhere in IOMUX_PAD (e.g. bit 63, which is currently reserved),
>>something like NO_PAD_CTRL, or create a reserved value other than
>>__NA_ for mux_ctrl_ofs/mux_mode.
>
> Stefano,
>
> There is '#define NO_PAD_CTRL             (1 << 17)' now,
> we can add'NO_MUX_CTRL' and 'NO_SEL_CTRL(select input)', but need to check
> whether the __NA__ pads are used or not now.
> also need a big change for the layout and related macro definition:
> 39  * MUX_CTRL_OFS:            0..11 (12)
> 40  * PAD_CTRL_OFS:           12..23 (12)
> 41  * SEL_INPUT_OFS:          24..35 (12)
> 42  * MUX_MODE + SION:        36..40  (5)
> 43  * PAD_CTRL + NO_PAD_CTRL: 41..58 (18)
> 44  * SEL_INP:                59..62  (4)
> 45  * reserved:                 63    (1)
>
> Can we just use the following way, since only i.mx7 has the requirement of
> mux_ctrl_ofs maybe at 0.
> if (is_soc_type(MX7)) {
>         __raw_writel(mux_mode, base + mux_ctrl_ofs);
> } else {
>         if (mux_ctrl_ofs)
>         __raw_writel(mux_mode, base + mux_ctrl_ofs);
> }
> I prefer this simple way for now, since we are at RC2 now. Later we can
> refactor the code using the way to provide macros NO_MUX_CTRL or NO_SEL_CTRL.
> What do you think?

Maybe, but instead of NO_MUX_CTRL and the like we could also just
define __NA_ to (-1) instead of 0 and mask the passed values
appropriately in IOMUX_PAD(). This should be done for all types of
offsets, and __NA_ should be used everywhere instead of raw 0x000
values. -1 is guaranteed not to be needed by any SoC because of the
word alignment requirement for valid offsets. That would keep the
changes small.

The NO_PAD_CTRL case is acutally a bit different because it means that
you don't want to set the pad value, even if there is a pad control
register offset.

Best regards,
Beno?t

[U-Boot] [PATCH 1/3] imx-common: fix iomux settings

[Peng Fan]

On Sun, Sep 20, 2015 at 05:02:58PM +0200, Beno?t Th?baudeau wrote:
>Hi Peng,
>
>On Sun, Sep 20, 2015 at 3:02 PM, Peng Fan <b51431@freescale.com> wrote:
>> On Sun, Sep 20, 2015 at 01:33:20PM +0200, Beno?t Th?baudeau wrote:
>>>Hi Stefano, Peng, Fabio, all,
>>>
>>>Sorry for seeing this only now, but...
>>>
>>>On Sun, Sep 20, 2015 at 9:43 AM, Stefano Babic <sbabic@denx.de> wrote:
>>>>
>>>>
>>>> On 14/09/2015 07:34, Peng Fan wrote:
>>>>> When setting iomux for a pin mux, there is no need to check mux_ctrl_ofs.
>>>
>>>This assumption is wrong. This check was there for a reason. Some i.MX
>>>SoCs have some registers controlling pads but not muxes, either for a
>>>single pin or for groups of pins:
>>>http://git.denx.de/?p=u-boot/u-boot-imx.git;a=blob;f=arch/arm/include/asm/arch-mx25/iomux-mx25.h;h=220cf4ef2e94aa69482557852ed0cc0690a79cec;hb=HEAD
>>>http://git.denx.de/?p=u-boot/u-boot-imx.git;a=blob;f=arch/arm/include/asm/arch-mx35/iomux-mx35.h;h=5898b46f4720088b18882e21d0d2424fff987ab5;hb=HEAD
>>>http://git.denx.de/?p=u-boot/u-boot-imx.git;a=blob;f=arch/arm/include/asm/arch-mx5/iomux-mx51.h;h=b7b169505f91c4a213be59efca47e8a5aed770e7;hb=HEAD
>>>
>>>I have not checked whether these cases are currently used in-tree by
>>>U-Boot, but they have to be possible anyway in order to support these
>>>SoCs.
>>
>> Beno?t,
>>
>> Thanks for pointing this out.
>> You mean piece of code like this, right?
>> 509         MX25_PAD_CTL_GRP_DVS_MISC               = IOMUX_PAD(0x418, 0x000, 0, 0, 0, NO_PAD_CTRL),
>> 510         MX25_PAD_CTL_GRP_DSE_FEC                = IOMUX_PAD(0x41c, 0x000, 0, 0, 0, NO_PAD_CTRL),
>> 511         MX25_PAD_CTL_GRP_DVS_JTAG               = IOMUX_PAD(0x420, 0x000, 0, 0, 0, NO_PAD_CTRL),
>> 512         MX25_PAD_CTL_GRP_DSE_NFC                = IOMUX_PAD(0x424, 0x000, 0, 0, 0, NO_PAD_CTRL),
>> 513         MX25_PAD_CTL_GRP_DSE_CSI                = IOMUX_PAD(0x428, 0x000, 0, 0, 0, NO_PAD_CTRL),
>> 514         MX25_PAD_CTL_GRP_DSE_WEIM               = IOMUX_PAD(0x42c, 0x000, 0, 0, 0, NO_PAD_CTRL),
>> 515         MX25_PAD_CTL_GRP_DSE_DDR                = IOMUX_PAD(0x430, 0x000, 0, 0, 0, NO_PAD_CTRL),
>> 516         MX25_PAD_CTL_GRP_DVS_CRM                = IOMUX_PAD(0x434, 0x000, 0, 0, 0, NO_PAD_CTRL),
>> 517         MX25_PAD_CTL_GRP_DSE_KPP                = IOMUX_PAD(0x438, 0x000, 0, 0, 0, NO_PAD_CTRL),
>> 518         MX25_PAD_CTL_GRP_DSE_SDHC1              = IOMUX_PAD(0x43c, 0x000, 0, 0, 0, NO_PAD_CTRL),
>> 519         MX25_PAD_CTL_GRP_DSE_LCD                = IOMUX_PAD(0x440, 0x000, 0, 0, 0, NO_PAD_CTRL),
>> 520         MX25_PAD_CTL_GRP_DSE_UART               = IOMUX_PAD(0x444, 0x000, 0, 0, 0, NO_PAD_CTRL),
>> 521         MX25_PAD_CTL_GRP_DVS_NFC                = IOMUX_PAD(0x448, 0x000, 0, 0, 0, NO_PAD_CTRL),
>> 522         MX25_PAD_CTL_GRP_DVS_CSI                = IOMUX_PAD(0x44c, 0x000, 0, 0, 0, NO_PAD_CTRL),
>> 523         MX25_PAD_CTL_GRP_DSE_CSPI1              = IOMUX_PAD(0x450, 0x000, 0, 0, 0, NO_PAD_CTRL),
>> 524         MX25_PAD_CTL_GRP_DDRTYPE                = IOMUX_PAD(0x454, 0x000, 0, 0, 0, NO_PAD_CTRL),
>> 525         MX25_PAD_CTL_GRP_DVS_SDHC1              = IOMUX_PAD(0x458, 0x000, 0, 0, 0, NO_PAD_CTRL),
>> 526         MX25_PAD_CTL_GRP_DVS_LCD                = IOMUX_PAD(0x45c, 0x000, 0, 0, 0, NO_PAD_CTRL)
>
>Correct.
>
>> My bad. I only took i.mx6/7 into consideration when working this patch.
>>>
>>>>> Also If still checking mux_ctrl_ofs, we have no chance to set iomux
>>>>> for i.MX7D IOMUXC_LPSR_SW_MUX_CTL_PAD_GPIO1_IO00, because the mux_ctrl_ofs
>>>>> for this register is 0.
>>>
>>>The need is clear, but then the test mechanism should be changed, not
>>>removed. You could find a free bit in mux_ctrl_ofs or in mux_mode or
>>>elsewhere in IOMUX_PAD (e.g. bit 63, which is currently reserved),
>>>something like NO_PAD_CTRL, or create a reserved value other than
>>>__NA_ for mux_ctrl_ofs/mux_mode.
>>
>> Stefano,
>>
>> There is '#define NO_PAD_CTRL             (1 << 17)' now,
>> we can add'NO_MUX_CTRL' and 'NO_SEL_CTRL(select input)', but need to check
>> whether the __NA__ pads are used or not now.
>> also need a big change for the layout and related macro definition:
>> 39  * MUX_CTRL_OFS:            0..11 (12)
>> 40  * PAD_CTRL_OFS:           12..23 (12)
>> 41  * SEL_INPUT_OFS:          24..35 (12)
>> 42  * MUX_MODE + SION:        36..40  (5)
>> 43  * PAD_CTRL + NO_PAD_CTRL: 41..58 (18)
>> 44  * SEL_INP:                59..62  (4)
>> 45  * reserved:                 63    (1)
>>
>> Can we just use the following way, since only i.mx7 has the requirement of
>> mux_ctrl_ofs maybe at 0.
>> if (is_soc_type(MX7)) {
>>         __raw_writel(mux_mode, base + mux_ctrl_ofs);
>> } else {
>>         if (mux_ctrl_ofs)
>>         __raw_writel(mux_mode, base + mux_ctrl_ofs);
>> }
>> I prefer this simple way for now, since we are at RC2 now. Later we can
>> refactor the code using the way to provide macros NO_MUX_CTRL or NO_SEL_CTRL.
>> What do you think?
>
>Maybe, but instead of NO_MUX_CTRL and the like we could also just
>define __NA_ to (-1) instead of 0 and mask the passed values
>appropriately in IOMUX_PAD(). This should be done for all types of
>offsets, and __NA_ should be used everywhere instead of raw 0x000
>values. -1 is guaranteed not to be needed by any SoC because of the
>word alignment requirement for valid offsets. That would keep the
>changes small.

We can not just simple use __NA_ with value -1.
see 
70 #define IOMUX_PAD(pad_ctrl_ofs, mux_ctrl_ofs, mux_mode, sel_input_ofs,  \
71                 sel_input, pad_ctrl)                                    \
72         (((iomux_v3_cfg_t)(mux_ctrl_ofs) << MUX_CTRL_OFS_SHIFT)     |   \
73         ((iomux_v3_cfg_t)(mux_mode)      << MUX_MODE_SHIFT)         |   \
74         ((iomux_v3_cfg_t)(pad_ctrl_ofs)  << MUX_PAD_CTRL_OFS_SHIFT) |   \
75         ((iomux_v3_cfg_t)(pad_ctrl)      << MUX_PAD_CTRL_SHIFT)     |   \
76         ((iomux_v3_cfg_t)(sel_input_ofs) << MUX_SEL_INPUT_OFS_SHIFT)|   \
77         ((iomux_v3_cfg_t)(sel_input)     << MUX_SEL_INPUT_SHIFT))

iomux_v3_cfg_t(mux_ctrl_ofs) << MUX_CTRL_OFS_SHIFT should be changed to 
`iomux_v3_cfg_t((mux_ctrl_ofs) << MUX_CTRL_OFS_SHIFT) & MUX_CTRL_OFS_MASK`,
in iomux-v3.c, need to test if (!(mux_ctrl_ofs & 1)) {xxxxx}.

I am not sure whether this will incur unexpected things or not, also
the IOMUX_PAD with 0, but not __NA_ need to change to use __NA_.

So I prefer to use is_soc_type(MXC_CPU_MX7) for now.

Regards,
Peng.

>
>The NO_PAD_CTRL case is acutally a bit different because it means that
>you don't want to set the pad value, even if there is a pad control
>register offset.
>
>Best regards,
>Beno?t

-- 

[U-Boot] [PATCH 1/3] imx-common: fix iomux settings

[Benoît Thébaudeau]

Hi Peng,

On Mon, Sep 21, 2015 at 3:05 AM, Peng Fan <b51431@freescale.com> wrote:
> On Sun, Sep 20, 2015 at 05:02:58PM +0200, Beno?t Th?baudeau wrote:
>>On Sun, Sep 20, 2015 at 3:02 PM, Peng Fan <b51431@freescale.com> wrote:
>>> On Sun, Sep 20, 2015 at 01:33:20PM +0200, Beno?t Th?baudeau wrote:
>>>>>> Also If still checking mux_ctrl_ofs, we have no chance to set iomux
>>>>>> for i.MX7D IOMUXC_LPSR_SW_MUX_CTL_PAD_GPIO1_IO00, because the mux_ctrl_ofs
>>>>>> for this register is 0.
>>>>
>>>>The need is clear, but then the test mechanism should be changed, not
>>>>removed. You could find a free bit in mux_ctrl_ofs or in mux_mode or
>>>>elsewhere in IOMUX_PAD (e.g. bit 63, which is currently reserved),
>>>>something like NO_PAD_CTRL, or create a reserved value other than
>>>>__NA_ for mux_ctrl_ofs/mux_mode.
>>>
>>> Stefano,
>>>
>>> There is '#define NO_PAD_CTRL             (1 << 17)' now,
>>> we can add'NO_MUX_CTRL' and 'NO_SEL_CTRL(select input)', but need to check
>>> whether the __NA__ pads are used or not now.
>>> also need a big change for the layout and related macro definition:
>>> 39  * MUX_CTRL_OFS:            0..11 (12)
>>> 40  * PAD_CTRL_OFS:           12..23 (12)
>>> 41  * SEL_INPUT_OFS:          24..35 (12)
>>> 42  * MUX_MODE + SION:        36..40  (5)
>>> 43  * PAD_CTRL + NO_PAD_CTRL: 41..58 (18)
>>> 44  * SEL_INP:                59..62  (4)
>>> 45  * reserved:                 63    (1)
>>>
>>> Can we just use the following way, since only i.mx7 has the requirement of
>>> mux_ctrl_ofs maybe at 0.
>>> if (is_soc_type(MX7)) {
>>>         __raw_writel(mux_mode, base + mux_ctrl_ofs);
>>> } else {
>>>         if (mux_ctrl_ofs)
>>>         __raw_writel(mux_mode, base + mux_ctrl_ofs);
>>> }
>>> I prefer this simple way for now, since we are at RC2 now. Later we can
>>> refactor the code using the way to provide macros NO_MUX_CTRL or NO_SEL_CTRL.
>>> What do you think?
>>
>>Maybe, but instead of NO_MUX_CTRL and the like we could also just
>>define __NA_ to (-1) instead of 0 and mask the passed values
>>appropriately in IOMUX_PAD(). This should be done for all types of
>>offsets, and __NA_ should be used everywhere instead of raw 0x000
>>values. -1 is guaranteed not to be needed by any SoC because of the
>>word alignment requirement for valid offsets. That would keep the
>>changes small.
>
> We can not just simple use __NA_ with value -1.
> see
> 70 #define IOMUX_PAD(pad_ctrl_ofs, mux_ctrl_ofs, mux_mode, sel_input_ofs,  \
> 71                 sel_input, pad_ctrl)                                    \
> 72         (((iomux_v3_cfg_t)(mux_ctrl_ofs) << MUX_CTRL_OFS_SHIFT)     |   \
> 73         ((iomux_v3_cfg_t)(mux_mode)      << MUX_MODE_SHIFT)         |   \
> 74         ((iomux_v3_cfg_t)(pad_ctrl_ofs)  << MUX_PAD_CTRL_OFS_SHIFT) |   \
> 75         ((iomux_v3_cfg_t)(pad_ctrl)      << MUX_PAD_CTRL_SHIFT)     |   \
> 76         ((iomux_v3_cfg_t)(sel_input_ofs) << MUX_SEL_INPUT_OFS_SHIFT)|   \
> 77         ((iomux_v3_cfg_t)(sel_input)     << MUX_SEL_INPUT_SHIFT))
>
> iomux_v3_cfg_t(mux_ctrl_ofs) << MUX_CTRL_OFS_SHIFT should be changed to
> `iomux_v3_cfg_t((mux_ctrl_ofs) << MUX_CTRL_OFS_SHIFT) & MUX_CTRL_OFS_MASK`,

Yes. That's why I said "mask the passed values appropriately in IOMUX_PAD()".

> in iomux-v3.c, need to test if (!(mux_ctrl_ofs & 1)) {xxxxx}.

Yes, of course.

> I am not sure whether this will incur unexpected things or not,

There's no reason.

> also
> the IOMUX_PAD with 0, but not __NA_ need to change to use __NA_.

And also, as I said, do the same for pad_ctrl_ofs and sel_input_ofs in
order to be consistent, and replace definitions like:
         MX25_PAD_CTL_GRP_DVS_MISC               = IOMUX_PAD(0x418,
0x000, 0, 0, 0, NO_PAD_CTRL),
with:
         MX25_PAD_CTL_GRP_DVS_MISC               = IOMUX_PAD(0x418,
__NA_, 0, __NA_, 0, NO_PAD_CTRL),

> So I prefer to use is_soc_type(MXC_CPU_MX7) for now.

Yes, that's OK for now. I was suggesting that as a longterm approach.
This change would be simple, but many definitions would have to be
updated.

Best regards,
Beno?t

[U-Boot] [PATCH 1/3] imx-common: fix iomux settings

[Stefano Babic]

Hi Benoit, Peng,

On 21/09/2015 20:41, Beno?t Th?baudeau wrote:
> Hi Peng,
> 
> On Mon, Sep 21, 2015 at 3:05 AM, Peng Fan <b51431@freescale.com> wrote:
>> On Sun, Sep 20, 2015 at 05:02:58PM +0200, Beno?t Th?baudeau wrote:
>>> On Sun, Sep 20, 2015 at 3:02 PM, Peng Fan <b51431@freescale.com> wrote:
>>>> On Sun, Sep 20, 2015 at 01:33:20PM +0200, Beno?t Th?baudeau wrote:
>>>>>>> Also If still checking mux_ctrl_ofs, we have no chance to set iomux
>>>>>>> for i.MX7D IOMUXC_LPSR_SW_MUX_CTL_PAD_GPIO1_IO00, because the mux_ctrl_ofs
>>>>>>> for this register is 0.
>>>>>
>>>>> The need is clear, but then the test mechanism should be changed, not
>>>>> removed. You could find a free bit in mux_ctrl_ofs or in mux_mode or
>>>>> elsewhere in IOMUX_PAD (e.g. bit 63, which is currently reserved),
>>>>> something like NO_PAD_CTRL, or create a reserved value other than
>>>>> __NA_ for mux_ctrl_ofs/mux_mode.
>>>>
>>>> Stefano,
>>>>
>>>> There is '#define NO_PAD_CTRL             (1 << 17)' now,
>>>> we can add'NO_MUX_CTRL' and 'NO_SEL_CTRL(select input)', but need to check
>>>> whether the __NA__ pads are used or not now.
>>>> also need a big change for the layout and related macro definition:
>>>> 39  * MUX_CTRL_OFS:            0..11 (12)
>>>> 40  * PAD_CTRL_OFS:           12..23 (12)
>>>> 41  * SEL_INPUT_OFS:          24..35 (12)
>>>> 42  * MUX_MODE + SION:        36..40  (5)
>>>> 43  * PAD_CTRL + NO_PAD_CTRL: 41..58 (18)
>>>> 44  * SEL_INP:                59..62  (4)
>>>> 45  * reserved:                 63    (1)
>>>>
>>>> Can we just use the following way, since only i.mx7 has the requirement of
>>>> mux_ctrl_ofs maybe at 0.
>>>> if (is_soc_type(MX7)) {
>>>>         __raw_writel(mux_mode, base + mux_ctrl_ofs);
>>>> } else {
>>>>         if (mux_ctrl_ofs)
>>>>         __raw_writel(mux_mode, base + mux_ctrl_ofs);
>>>> }
>>>> I prefer this simple way for now, since we are at RC2 now. Later we can
>>>> refactor the code using the way to provide macros NO_MUX_CTRL or NO_SEL_CTRL.
>>>> What do you think?
>>>
>>> Maybe, but instead of NO_MUX_CTRL and the like we could also just
>>> define __NA_ to (-1) instead of 0 and mask the passed values
>>> appropriately in IOMUX_PAD(). This should be done for all types of
>>> offsets, and __NA_ should be used everywhere instead of raw 0x000
>>> values. -1 is guaranteed not to be needed by any SoC because of the
>>> word alignment requirement for valid offsets. That would keep the
>>> changes small.
>>
>> We can not just simple use __NA_ with value -1.
>> see
>> 70 #define IOMUX_PAD(pad_ctrl_ofs, mux_ctrl_ofs, mux_mode, sel_input_ofs,  \
>> 71                 sel_input, pad_ctrl)                                    \
>> 72         (((iomux_v3_cfg_t)(mux_ctrl_ofs) << MUX_CTRL_OFS_SHIFT)     |   \
>> 73         ((iomux_v3_cfg_t)(mux_mode)      << MUX_MODE_SHIFT)         |   \
>> 74         ((iomux_v3_cfg_t)(pad_ctrl_ofs)  << MUX_PAD_CTRL_OFS_SHIFT) |   \
>> 75         ((iomux_v3_cfg_t)(pad_ctrl)      << MUX_PAD_CTRL_SHIFT)     |   \
>> 76         ((iomux_v3_cfg_t)(sel_input_ofs) << MUX_SEL_INPUT_OFS_SHIFT)|   \
>> 77         ((iomux_v3_cfg_t)(sel_input)     << MUX_SEL_INPUT_SHIFT))
>>
>> iomux_v3_cfg_t(mux_ctrl_ofs) << MUX_CTRL_OFS_SHIFT should be changed to
>> `iomux_v3_cfg_t((mux_ctrl_ofs) << MUX_CTRL_OFS_SHIFT) & MUX_CTRL_OFS_MASK`,
> 
> Yes. That's why I said "mask the passed values appropriately in IOMUX_PAD()".
> 
>> in iomux-v3.c, need to test if (!(mux_ctrl_ofs & 1)) {xxxxx}.
> 
> Yes, of course.
> 
>> I am not sure whether this will incur unexpected things or not,
> 
> There's no reason.
> 
>> also
>> the IOMUX_PAD with 0, but not __NA_ need to change to use __NA_.
> 
> And also, as I said, do the same for pad_ctrl_ofs and sel_input_ofs in
> order to be consistent, and replace definitions like:
>          MX25_PAD_CTL_GRP_DVS_MISC               = IOMUX_PAD(0x418,
> 0x000, 0, 0, 0, NO_PAD_CTRL),
> with:
>          MX25_PAD_CTL_GRP_DVS_MISC               = IOMUX_PAD(0x418,
> __NA_, 0, __NA_, 0, NO_PAD_CTRL),
> 
>> So I prefer to use is_soc_type(MXC_CPU_MX7) for now.
> 
> Yes, that's OK for now. I was suggesting that as a longterm approach.

Agree.

> This change would be simple, but many definitions would have to be
> updated.

Yes - so let it after next release.

Best regards,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH 1/3] imx-common: fix iomux settings

[Peng Fan]

Hi Stefano, Benoit,

On Mon, Sep 21, 2015 at 10:13:42PM +0200, Stefano Babic wrote:
>Hi Benoit, Peng,
>
>On 21/09/2015 20:41, Beno?t Th?baudeau wrote:
>> Hi Peng,
>> 
>> On Mon, Sep 21, 2015 at 3:05 AM, Peng Fan <b51431@freescale.com> wrote:
>>> On Sun, Sep 20, 2015 at 05:02:58PM +0200, Beno?t Th?baudeau wrote:
>>>> On Sun, Sep 20, 2015 at 3:02 PM, Peng Fan <b51431@freescale.com> wrote:
>>>>> On Sun, Sep 20, 2015 at 01:33:20PM +0200, Beno?t Th?baudeau wrote:
>>>>>>>> Also If still checking mux_ctrl_ofs, we have no chance to set iomux
>>>>>>>> for i.MX7D IOMUXC_LPSR_SW_MUX_CTL_PAD_GPIO1_IO00, because the mux_ctrl_ofs
>>>>>>>> for this register is 0.
>>>>>>
>>>>>> The need is clear, but then the test mechanism should be changed, not
>>>>>> removed. You could find a free bit in mux_ctrl_ofs or in mux_mode or
>>>>>> elsewhere in IOMUX_PAD (e.g. bit 63, which is currently reserved),
>>>>>> something like NO_PAD_CTRL, or create a reserved value other than
>>>>>> __NA_ for mux_ctrl_ofs/mux_mode.
>>>>>
>>>>> Stefano,
>>>>>
>>>>> There is '#define NO_PAD_CTRL             (1 << 17)' now,
>>>>> we can add'NO_MUX_CTRL' and 'NO_SEL_CTRL(select input)', but need to check
>>>>> whether the __NA__ pads are used or not now.
>>>>> also need a big change for the layout and related macro definition:
>>>>> 39  * MUX_CTRL_OFS:            0..11 (12)
>>>>> 40  * PAD_CTRL_OFS:           12..23 (12)
>>>>> 41  * SEL_INPUT_OFS:          24..35 (12)
>>>>> 42  * MUX_MODE + SION:        36..40  (5)
>>>>> 43  * PAD_CTRL + NO_PAD_CTRL: 41..58 (18)
>>>>> 44  * SEL_INP:                59..62  (4)
>>>>> 45  * reserved:                 63    (1)
>>>>>
>>>>> Can we just use the following way, since only i.mx7 has the requirement of
>>>>> mux_ctrl_ofs maybe at 0.
>>>>> if (is_soc_type(MX7)) {
>>>>>         __raw_writel(mux_mode, base + mux_ctrl_ofs);
>>>>> } else {
>>>>>         if (mux_ctrl_ofs)
>>>>>         __raw_writel(mux_mode, base + mux_ctrl_ofs);
>>>>> }
>>>>> I prefer this simple way for now, since we are at RC2 now. Later we can
>>>>> refactor the code using the way to provide macros NO_MUX_CTRL or NO_SEL_CTRL.
>>>>> What do you think?
>>>>
>>>> Maybe, but instead of NO_MUX_CTRL and the like we could also just
>>>> define __NA_ to (-1) instead of 0 and mask the passed values
>>>> appropriately in IOMUX_PAD(). This should be done for all types of
>>>> offsets, and __NA_ should be used everywhere instead of raw 0x000
>>>> values. -1 is guaranteed not to be needed by any SoC because of the
>>>> word alignment requirement for valid offsets. That would keep the
>>>> changes small.
>>>
>>> We can not just simple use __NA_ with value -1.
>>> see
>>> 70 #define IOMUX_PAD(pad_ctrl_ofs, mux_ctrl_ofs, mux_mode, sel_input_ofs,  \
>>> 71                 sel_input, pad_ctrl)                                    \
>>> 72         (((iomux_v3_cfg_t)(mux_ctrl_ofs) << MUX_CTRL_OFS_SHIFT)     |   \
>>> 73         ((iomux_v3_cfg_t)(mux_mode)      << MUX_MODE_SHIFT)         |   \
>>> 74         ((iomux_v3_cfg_t)(pad_ctrl_ofs)  << MUX_PAD_CTRL_OFS_SHIFT) |   \
>>> 75         ((iomux_v3_cfg_t)(pad_ctrl)      << MUX_PAD_CTRL_SHIFT)     |   \
>>> 76         ((iomux_v3_cfg_t)(sel_input_ofs) << MUX_SEL_INPUT_OFS_SHIFT)|   \
>>> 77         ((iomux_v3_cfg_t)(sel_input)     << MUX_SEL_INPUT_SHIFT))
>>>
>>> iomux_v3_cfg_t(mux_ctrl_ofs) << MUX_CTRL_OFS_SHIFT should be changed to
>>> `iomux_v3_cfg_t((mux_ctrl_ofs) << MUX_CTRL_OFS_SHIFT) & MUX_CTRL_OFS_MASK`,
>> 
>> Yes. That's why I said "mask the passed values appropriately in IOMUX_PAD()".
>> 
>>> in iomux-v3.c, need to test if (!(mux_ctrl_ofs & 1)) {xxxxx}.
>> 
>> Yes, of course.
>> 
>>> I am not sure whether this will incur unexpected things or not,
>> 
>> There's no reason.
>> 
>>> also
>>> the IOMUX_PAD with 0, but not __NA_ need to change to use __NA_.
>> 
>> And also, as I said, do the same for pad_ctrl_ofs and sel_input_ofs in
>> order to be consistent, and replace definitions like:
>>          MX25_PAD_CTL_GRP_DVS_MISC               = IOMUX_PAD(0x418,
>> 0x000, 0, 0, 0, NO_PAD_CTRL),
>> with:
>>          MX25_PAD_CTL_GRP_DVS_MISC               = IOMUX_PAD(0x418,
>> __NA_, 0, __NA_, 0, NO_PAD_CTRL),
>> 
>>> So I prefer to use is_soc_type(MXC_CPU_MX7) for now.

I have sent out one patch: https://patchwork.ozlabs.org/patch/520204/

>> 
>> Yes, that's OK for now. I was suggesting that as a longterm approach.
>
>Agree.
>
>> This change would be simple, but many definitions would have to be
>> updated.
>
>Yes - so let it after next release.

I'll prepare the patch for next release.

Regards,
Peng.
>
>Best regards,
>Stefano
>
>-- 
>=====================================================================
>DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
>HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
>=====================================================================

--