[U-Boot] [PATCH V3] imx: mx25: Remove SION bit in all pin-mux that are safe

Fri Jan 26 09:38:08 UTC 2018

Hi Michael,

On 26/01/2018 at 08:51, Michael Trimarchi wrote:
> On Thu, Jan 25, 2018 at 10:48:42PM +0100, Benoît Thébaudeau wrote:
>> On Thu, Jan 25, 2018 at 2:06 PM, Michael Trimarchi
>> <michael at amarulasolutions.com> wrote:
>>> SION bit should be used in the situation that we need
>>> to read back the value of a pin and should not be set by
>>> default macro.
>>>
>>> We get some malfunction as raised by following thread
>>>
>>> https://www.spinics.net/lists/linux-usb/msg162574.html
>>>
>>> As reported by this application note:
>>> https://www.nxp.com/docs/en/application-note/AN5078.pdf
>>>
>>> The software input on (SION) bit is an option to force an input
>>> path to be active regardless of the value driven by the
>>> corresponding module. It is used when the nature direction
>>> of a pin depending on selected alternative function is an output,
>>> but it is needed to read the real logic value on a pin.
>>>
>>> The SION bit can be used in:
>>> • Loopback: the module of a selected alternative function drives
>>> the pad and also receives the pad value as an input
>>> • GPIO capture: the module of a selected alternative function
>>> drives the pin and the value is captured by the GPIO
>>>
>>> SION bit is not necessary when the pin is configured as a peripheral
>>> apart specific silicon bug. If an application needs to have this
>>> set, this should be done in board file or in dts file
>>>
>>> Signed-off-by: Michael Trimarchi <michael at amarulasolutions.com>
>>
>> [...]
>>
>> I have now tested the following peripherals on i.MX25 without any SION
>> bit set: AUDMUX/SSI, eSDHC, FEC (except PHY access for which MDIO
>> might have an issue without SION; Ethernet works but I have not
>> checked whether this is possible at least partially without access to
>> the PHY registers), GPIO, I2C, SLCDC, UART, and USB. Everything worked
>> fine except the lack of SION for eSDHC CMD, which is true for all
>> eSDHC instances (and not only for eSDHC1, so Linux's
>> arch/arm/boot/dts/imx25-pinfunc.h should be fixed). Therefore, SION is
>> mandatory for all the eSDHC CMD functions in
> 
> I have checked and u-boot does not have this pin-muxing. I think that this
> patch should go as it is and a separate patch like this one following should
> be applied after that. What do you think?

Your patch is just fine as is. We do not need to add support for eSDHC2 in
U-Boot unless some mainline board needs it. But eSDHC2 CMD should have SION set
by default in Linux.

>> arch/arm/include/asm/arch-mx25/iomux-mx25.h, but it can be removed for
>> everything else (I still have to carry out one more test to confirm
>> this for FEC MDIO), and moved to the board files if necessary (but I
>> don't think that any mainline board code is doing something fancy
>> enough to need it).
>>
>> If the bus-status detection example in the documentation of SION in
>> the reference manual is useful, it means that SION is probably
>> required for all the signals requiring simultaneous output and input
>> (such as I²C for device clock stretching or multi-master bus
>> arbitration, except if the IP toggles between input and output low at
>> each clock edge rather than between open-drain output high and output
>> low), because there are no automatic SION signals between the
>> peripherals and the pads but only direction signals that can request a
>> single direction at a time. For bidirectional signals that do not
>> require simultaneous output and input because they work in turns (such
>> as FEC MDIO), SION can be required or not depending on whether the IP
>> toggles the direction signal for each turn or always expects an input
>> feedback while driving an open-drain output high. Even if SION is
>> required for the I²C example mentioned above (which is unlikely as
>> basic I²C transfers work fine and clock stretching detection is
>> automatic and would always need the input state), the need for these
>> advanced I²C features can be considered board-specific, so SION would
>> still not be required in iomux-mx25.h.
>>
>> In the end, for this patch, apart from the pending test for FEC MDIO:
>> Reviewed-by: Benoît Thébaudeau <benoit.thebaudeau.dev at gmail.com>
>>
>> Best regards,
>> Benoît
> 
>>From d6b5bf4f1b6a0bcbcfd6ff2597fd2a7b3884cb3b Mon Sep 17 00:00:00 2001
> From: Michael Trimarchi <michael at amarulasolutions.com>
> Date: Fri, 26 Jan 2018 08:42:54 +0100
> Subject: [PATCH] imx: mx25: Add pinmux definition for ESDHC2
> 
> Add pin mux of second esdhc interface. Set SION bin on the CMD
> muxing in order to avoid sdcard fail detection
> 
> Signed-off-by: Michael Trimarchi <michael at amarulasolutions.com>
> ---
>  arch/arm/include/asm/arch-mx25/iomux-mx25.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm/include/asm/arch-mx25/iomux-mx25.h b/arch/arm/include/asm/arch-mx25/iomux-mx25.h
> index 437f122..1b38d16 100644
> --- a/arch/arm/include/asm/arch-mx25/iomux-mx25.h
> +++ b/arch/arm/include/asm/arch-mx25/iomux-mx25.h
> @@ -225,15 +225,18 @@ enum {
>  
>  	MX25_PAD_LD8__LD8			= IOMUX_PAD(0x2e0, 0x0e8, 0x00, 0, 0, PAD_CTL_SRE_FAST),
>  	MX25_PAD_LD8__FEC_TX_ERR		= IOMUX_PAD(0x2e0, 0x0e8, 0x05, 0, 0, NO_PAD_CTRL),
> +	MX25_PAD_LD8__SDHC2_CMD			= IOMUX_PAD(0x2e0, 0x0e8, 0x06, 0x4e0, 0, NO_PAD_CTRL),

SD CMD, so SION is required here.

>  
>  	MX25_PAD_LD9__LD9			= IOMUX_PAD(0x2e4, 0x0ec, 0x00, 0, 0, PAD_CTL_SRE_FAST),
>  	MX25_PAD_LD9__FEC_COL			= IOMUX_PAD(0x2e4, 0x0ec, 0x05, 0x504, 1, NO_PAD_CTRL),
> +	MX25_PAD_LD9__SDHC2_CLK			= IOMUX_PAD(0x2e4, 0x0ec, 0x06, 0x4dc, 0, NO_PAD_CTRL),
>  
>  	MX25_PAD_LD10__LD10			= IOMUX_PAD(0x2e8, 0x0f0, 0x00, 0, 0, PAD_CTL_SRE_FAST),
>  	MX25_PAD_LD10__FEC_RX_ER		= IOMUX_PAD(0x2e8, 0x0f0, 0x05, 0x518, 1, NO_PAD_CTRL),
>  
>  	MX25_PAD_LD11__LD11			= IOMUX_PAD(0x2ec, 0x0f4, 0x00, 0, 0, PAD_CTL_SRE_FAST),
>  	MX25_PAD_LD11__FEC_RDATA2		= IOMUX_PAD(0x2ec, 0x0f4, 0x05, 0x50c, 1, NO_PAD_CTRL),
> +	MX25_PAD_LD11__SDHC2_DAT1		= IOMUX_PAD(0x2ec, 0x0f4, 0x06, 0x4e8, 0, NO_PAD_CTRL),
>  
>  	MX25_PAD_LD12__LD12			= IOMUX_PAD(0x2f0, 0x0f8, 0x00, 0, 0, PAD_CTL_SRE_FAST),
>  	MX25_PAD_LD12__FEC_RDATA3		= IOMUX_PAD(0x2f0, 0x0f8, 0x05, 0x510, 1, NO_PAD_CTRL),
> @@ -287,8 +290,10 @@ enum {
>  
>  	MX25_PAD_CSI_D6__CSI_D6			= IOMUX_PAD(0x328, 0x130, 0x00, 0, 0, NO_PAD_CTRL),
>  	MX25_PAD_CSI_D6__GPIO_1_31		= IOMUX_PAD(0x328, 0x130, 0x05, 0, 0, NO_PAD_CTRL),
> +	MX25_PAD_CSI_D6__SDHC2_CMD		= IOMUX_PAD(0x328, 0x130, 0x12, 0x4e0, 1, NO_PAD_CTRL),
>  
>  	MX25_PAD_CSI_D7__CSI_D7			= IOMUX_PAD(0x32c, 0x134, 0x00, 0, 0, NO_PAD_CTRL),
> +	MX25_PAD_CSI_D7__SDHC2_DAT_CLK		= IOMUX_PAD(0x32C, 0x134, 0x02, 0x4dc, 1, NO_PAD_CTRL),
>  	MX25_PAD_CSI_D7__GPIO_1_6		= IOMUX_PAD(0x32c, 0x134, 0x05, 0, 0, NO_PAD_CTRL),
>  
>  	MX25_PAD_CSI_D8__CSI_D8			= IOMUX_PAD(0x330, 0x138, 0x00, 0, 0, NO_PAD_CTRL),
> @@ -298,15 +303,18 @@ enum {
>  	MX25_PAD_CSI_D9__GPIO_4_21		= IOMUX_PAD(0x334, 0x13c, 0x05, 0, 0, NO_PAD_CTRL),
>  
>  	MX25_PAD_CSI_MCLK__CSI_MCLK		= IOMUX_PAD(0x338, 0x140, 0x00, 0, 0, NO_PAD_CTRL),
> +	MX25_PAD_CSI_MCLK__SDHC2_DAT0		= IOMUX_PAD(0x338, 0x140, 0x02, 0x4e4, 1, NO_PAD_CTRL),
>  	MX25_PAD_CSI_MCLK__GPIO_1_8		= IOMUX_PAD(0x338, 0x140, 0x05, 0, 0, NO_PAD_CTRL),
>  
>  	MX25_PAD_CSI_VSYNC__CSI_VSYNC		= IOMUX_PAD(0x33c, 0x144, 0x00, 0, 0, NO_PAD_CTRL),
>  	MX25_PAD_CSI_VSYNC__GPIO_1_9		= IOMUX_PAD(0x33c, 0x144, 0x05, 0, 0, NO_PAD_CTRL),
>  
>  	MX25_PAD_CSI_HSYNC__CSI_HSYNC		= IOMUX_PAD(0x340, 0x148, 0x00, 0, 0, NO_PAD_CTRL),
> +	MX25_PAD_CSI_HSYNC__SDHC2_DAT2		= IOMUX_PAD(0x340, 0x148, 0x02, 0x4ec, 1, NO_PAD_CTRL),
>  	MX25_PAD_CSI_HSYNC__GPIO_1_10		= IOMUX_PAD(0x340, 0x148, 0x05, 0, 0, NO_PAD_CTRL),
>  
>  	MX25_PAD_CSI_PIXCLK__CSI_PIXCLK		= IOMUX_PAD(0x344, 0x14c, 0x00, 0, 0, NO_PAD_CTRL),
> +	MX25_PAD_CSI_PIXCLK__SDHC2_DAT3		= IOMUX_PAD(0x344, 0x14c, 0x02, 0x4f0, 1, NO_PAD_CTRL),
>  	MX25_PAD_CSI_PIXCLK__GPIO_1_11		= IOMUX_PAD(0x344, 0x14c, 0x05, 0, 0, NO_PAD_CTRL),
>  
>  	MX25_PAD_I2C1_CLK__I2C1_CLK		= IOMUX_PAD(0x348, 0x150, 0x00, 0, 0, NO_PAD_CTRL),
> 

Best regards,
Benoît