[U-Boot] [PATCH 1/3] imx-common: fix iomux settings
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);
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) {
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
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.
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. --
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
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; }
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
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
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
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/ar... http://git.denx.de/?p=u-boot/u-boot-imx.git;a=blob;f=arch/arm/include/asm/ar... http://git.denx.de/?p=u-boot/u-boot-imx.git;a=blob;f=arch/arm/include/asm/ar...
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
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/ar... http://git.denx.de/?p=u-boot/u-boot-imx.git;a=blob;f=arch/arm/include/asm/ar... http://git.denx.de/?p=u-boot/u-boot-imx.git;a=blob;f=arch/arm/include/asm/ar...
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 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?
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
--
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/ar... http://git.denx.de/?p=u-boot/u-boot-imx.git;a=blob;f=arch/arm/include/asm/ar... http://git.denx.de/?p=u-boot/u-boot-imx.git;a=blob;f=arch/arm/include/asm/ar...
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
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/ar... http://git.denx.de/?p=u-boot/u-boot-imx.git;a=blob;f=arch/arm/include/asm/ar... http://git.denx.de/?p=u-boot/u-boot-imx.git;a=blob;f=arch/arm/include/asm/ar...
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
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/ar... http://git.denx.de/?p=u-boot/u-boot-imx.git;a=blob;f=arch/arm/include/asm/ar... http://git.denx.de/?p=u-boot/u-boot-imx.git;a=blob;f=arch/arm/include/asm/ar...
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
--
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
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
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@denx.de =====================================================================
--
participants (5)
-
Benoît Thébaudeau -
Fabio Estevam -
Peng Fan -
Peng Fan -
Stefano Babic