From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peng Fan Date: Mon, 21 Sep 2015 09:05:51 +0800 Subject: [U-Boot] [PATCH 1/3] imx-common: fix iomux settings In-Reply-To: References: <1442208885-32387-1-git-send-email-Peng.Fan@freescale.com> <55FE63A0.7060801@denx.de> <20150920130226.GA17124@shlinux2> Message-ID: <20150921010548.GA5788@shlinux2> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 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 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 --