| Message ID | 1442208885-32387-1-git-send-email-Peng.Fan@freescale.com |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Stefano Babic |
| Headers | show |
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/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
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 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/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
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
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
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 >=====================================================================
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);
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(-)