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

[Peng Fan <b51431@freescale.com>]

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

--