Re: [PATCH] ARM: dts: imx25-pinfunc: Always set SION for SD CMD

[Uwe Kleine-König]

On Mon, Feb 05, 2018 at 04:10:20PM +0800, Shawn Guo wrote:
> On Sat, Jan 27, 2018 at 09:37:11PM +0100, Benoît Thébaudeau wrote:
> > On Sat, Jan 27, 2018 at 4:37 PM, Uwe Kleine-König
> >  wrote:
> > > On Sat, Jan 27, 2018 at 01:07:52AM +0100, Benoît Thébaudeau wrote:
> > >> The eSDHC does not work properly if the SION bit is not set for the
> > >> bidirectional CMD signal, whatever the eSDHC instance and the selected
> > >> pad. Therefore, setting SION is mandatory for all eSDHC CMD ports. Do
> > >> this for MX25_PAD_*__SD*_CMD in imx25-pinfunc.h in order to enforce this
> > >> behavior for all boards.
> > >>
> > >> This had already been done for eSDHC1, but not for eSDHC2. Also, define
> > >> MX25_PAD_FEC_MDC__SDHC2_CMD so that all the possible cases are covered
> > >> from now on.
> > >
> > > There is an inconsistency in the naming. The eSDHC1 CMD constants are
> > > named:
> > >
> > > MX25_PAD_SD1_CMD__SD1_CMD
> > >
> > > The reference calls this:
> > >
> > > CMD of instance: esdhc1.
> > >
> > > The register name is correct though. Not sure it's worth to fix this to
> > > use consistent naming (which would result in:
> > >
> > > MX25_PAD_SD1_CMD__ESDHC1_CMD
> > >
> > > which looks ugly, too).
> > 
> > Indeed. I had also noticed this. I can send a patch to apply before
> > this one. But that would break the out-of-tree DT files using these
> > definitions. Would that be OK?
> 
> That's OK, I would say.  It could be a reminder of that they should
> upstream their files.

I assume you saw v2 which introduced some defines to give those guys a
bit of time, which IMHO is still better. Then we can remove these compat
defines in (say) a year and so trigger this reminder.

Also one of the advantages of dt that was advertised when ARM was
converted to dt was, that upstreaming the dt isn't necessary any more. I
personally like this and still consider not upstreaming the dtb part of
a machine a sane approach.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/  |

Re: [PATCH] ARM: dts: imx25-pinfunc: Always set SION for SD CMD

[Uwe Kleine-König]

On Mon, Feb 05, 2018 at 04:10:20PM +0800, Shawn Guo wrote:
> On Sat, Jan 27, 2018 at 09:37:11PM +0100, Benoît Thébaudeau wrote:
> > On Sat, Jan 27, 2018 at 4:37 PM, Uwe Kleine-König
> >  wrote:
> > > On Sat, Jan 27, 2018 at 01:07:52AM +0100, Benoît Thébaudeau wrote:
> > >> The eSDHC does not work properly if the SION bit is not set for the
> > >> bidirectional CMD signal, whatever the eSDHC instance and the selected
> > >> pad. Therefore, setting SION is mandatory for all eSDHC CMD ports. Do
> > >> this for MX25_PAD_*__SD*_CMD in imx25-pinfunc.h in order to enforce this
> > >> behavior for all boards.
> > >>
> > >> This had already been done for eSDHC1, but not for eSDHC2. Also, define
> > >> MX25_PAD_FEC_MDC__SDHC2_CMD so that all the possible cases are covered
> > >> from now on.
> > >
> > > There is an inconsistency in the naming. The eSDHC1 CMD constants are
> > > named:
> > >
> > > MX25_PAD_SD1_CMD__SD1_CMD
> > >
> > > The reference calls this:
> > >
> > > CMD of instance: esdhc1.
> > >
> > > The register name is correct though. Not sure it's worth to fix this to
> > > use consistent naming (which would result in:
> > >
> > > MX25_PAD_SD1_CMD__ESDHC1_CMD
> > >
> > > which looks ugly, too).
> > 
> > Indeed. I had also noticed this. I can send a patch to apply before
> > this one. But that would break the out-of-tree DT files using these
> > definitions. Would that be OK?
> 
> That's OK, I would say.  It could be a reminder of that they should
> upstream their files.

I assume you saw v2 which introduced some defines to give those guys a
bit of time, which IMHO is still better. Then we can remove these compat
defines in (say) a year and so trigger this reminder.

Also one of the advantages of dt that was advertised when ARM was
converted to dt was, that upstreaming the dt isn't necessary any more. I
personally like this and still consider not upstreaming the dtb part of
a machine a sane approach.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/  |

Re: [PATCH] ARM: dts: imx25-pinfunc: Always set SION for SD CMD

[Shawn Guo]

On Sat, Jan 27, 2018 at 09:37:11PM +0100, Benoît Thébaudeau wrote:
> On Sat, Jan 27, 2018 at 4:37 PM, Uwe Kleine-König
>  wrote:
> > On Sat, Jan 27, 2018 at 01:07:52AM +0100, Benoît Thébaudeau wrote:
> >> The eSDHC does not work properly if the SION bit is not set for the
> >> bidirectional CMD signal, whatever the eSDHC instance and the selected
> >> pad. Therefore, setting SION is mandatory for all eSDHC CMD ports. Do
> >> this for MX25_PAD_*__SD*_CMD in imx25-pinfunc.h in order to enforce this
> >> behavior for all boards.
> >>
> >> This had already been done for eSDHC1, but not for eSDHC2. Also, define
> >> MX25_PAD_FEC_MDC__SDHC2_CMD so that all the possible cases are covered
> >> from now on.
> >
> > There is an inconsistency in the naming. The eSDHC1 CMD constants are
> > named:
> >
> > MX25_PAD_SD1_CMD__SD1_CMD
> >
> > The reference calls this:
> >
> > CMD of instance: esdhc1.
> >
> > The register name is correct though. Not sure it's worth to fix this to
> > use consistent naming (which would result in:
> >
> > MX25_PAD_SD1_CMD__ESDHC1_CMD
> >
> > which looks ugly, too).
> 
> Indeed. I had also noticed this. I can send a patch to apply before
> this one. But that would break the out-of-tree DT files using these
> definitions. Would that be OK?

That's OK, I would say.  It could be a reminder of that they should
upstream their files.

Shawn

Re: [PATCH] ARM: dts: imx25-pinfunc: Always set SION for SD CMD

[Shawn Guo]

On Sat, Jan 27, 2018 at 09:37:11PM +0100, Benoît Thébaudeau wrote:
> On Sat, Jan 27, 2018 at 4:37 PM, Uwe Kleine-König
>  wrote:
> > On Sat, Jan 27, 2018 at 01:07:52AM +0100, Benoît Thébaudeau wrote:
> >> The eSDHC does not work properly if the SION bit is not set for the
> >> bidirectional CMD signal, whatever the eSDHC instance and the selected
> >> pad. Therefore, setting SION is mandatory for all eSDHC CMD ports. Do
> >> this for MX25_PAD_*__SD*_CMD in imx25-pinfunc.h in order to enforce this
> >> behavior for all boards.
> >>
> >> This had already been done for eSDHC1, but not for eSDHC2. Also, define
> >> MX25_PAD_FEC_MDC__SDHC2_CMD so that all the possible cases are covered
> >> from now on.
> >
> > There is an inconsistency in the naming. The eSDHC1 CMD constants are
> > named:
> >
> > MX25_PAD_SD1_CMD__SD1_CMD
> >
> > The reference calls this:
> >
> > CMD of instance: esdhc1.
> >
> > The register name is correct though. Not sure it's worth to fix this to
> > use consistent naming (which would result in:
> >
> > MX25_PAD_SD1_CMD__ESDHC1_CMD
> >
> > which looks ugly, too).
> 
> Indeed. I had also noticed this. I can send a patch to apply before
> this one. But that would break the out-of-tree DT files using these
> definitions. Would that be OK?

That's OK, I would say.  It could be a reminder of that they should
upstream their files.

Shawn

Re: [PATCH] ARM: dts: imx25-pinfunc: Always set SION for SD CMD

[Uwe Kleine-König]

On Sat, Jan 27, 2018 at 09:37:11PM +0100, Benoît Thébaudeau wrote:
> On Sat, Jan 27, 2018 at 4:37 PM, Uwe Kleine-König
>  wrote:
> > On Sat, Jan 27, 2018 at 01:07:52AM +0100, Benoît Thébaudeau wrote:
> >> The eSDHC does not work properly if the SION bit is not set for the
> >> bidirectional CMD signal, whatever the eSDHC instance and the selected
> >> pad. Therefore, setting SION is mandatory for all eSDHC CMD ports. Do
> >> this for MX25_PAD_*__SD*_CMD in imx25-pinfunc.h in order to enforce this
> >> behavior for all boards.
> >>
> >> This had already been done for eSDHC1, but not for eSDHC2. Also, define
> >> MX25_PAD_FEC_MDC__SDHC2_CMD so that all the possible cases are covered
> >> from now on.
> >
> > There is an inconsistency in the naming. The eSDHC1 CMD constants are
> > named:
> >
> > MX25_PAD_SD1_CMD__SD1_CMD
> >
> > The reference calls this:
> >
> > CMD of instance: esdhc1.
> >
> > The register name is correct though. Not sure it's worth to fix this to
> > use consistent naming (which would result in:
> >
> > MX25_PAD_SD1_CMD__ESDHC1_CMD
> >
> > which looks ugly, too).
> 
> Indeed. I had also noticed this. I can send a patch to apply before
> this one. But that would break the out-of-tree DT files using these
> definitions. Would that be OK?

One possibility is to do something like this:

#define MX25_PAD_SD1_CMD__ESDHC1_CMDX Y Z ...

...

/*
 * compat defines for out-of-tree users. Yes, you should update when you
 * make use of one of them.
 */
#define MX25_PAD_SD1_CMD__SD1_CMD MX25_PAD_SD1_CMD__ESDHC1_CMD

This would also allow to fix the users in a separate step.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/  |

Re: [PATCH] ARM: dts: imx25-pinfunc: Always set SION for SD CMD

[Uwe Kleine-König]

On Sat, Jan 27, 2018 at 09:37:11PM +0100, Benoît Thébaudeau wrote:
> On Sat, Jan 27, 2018 at 4:37 PM, Uwe Kleine-König
>  wrote:
> > On Sat, Jan 27, 2018 at 01:07:52AM +0100, Benoît Thébaudeau wrote:
> >> The eSDHC does not work properly if the SION bit is not set for the
> >> bidirectional CMD signal, whatever the eSDHC instance and the selected
> >> pad. Therefore, setting SION is mandatory for all eSDHC CMD ports. Do
> >> this for MX25_PAD_*__SD*_CMD in imx25-pinfunc.h in order to enforce this
> >> behavior for all boards.
> >>
> >> This had already been done for eSDHC1, but not for eSDHC2. Also, define
> >> MX25_PAD_FEC_MDC__SDHC2_CMD so that all the possible cases are covered
> >> from now on.
> >
> > There is an inconsistency in the naming. The eSDHC1 CMD constants are
> > named:
> >
> > MX25_PAD_SD1_CMD__SD1_CMD
> >
> > The reference calls this:
> >
> > CMD of instance: esdhc1.
> >
> > The register name is correct though. Not sure it's worth to fix this to
> > use consistent naming (which would result in:
> >
> > MX25_PAD_SD1_CMD__ESDHC1_CMD
> >
> > which looks ugly, too).
> 
> Indeed. I had also noticed this. I can send a patch to apply before
> this one. But that would break the out-of-tree DT files using these
> definitions. Would that be OK?

One possibility is to do something like this:

#define MX25_PAD_SD1_CMD__ESDHC1_CMDX Y Z ...

...

/*
 * compat defines for out-of-tree users. Yes, you should update when you
 * make use of one of them.
 */
#define MX25_PAD_SD1_CMD__SD1_CMD MX25_PAD_SD1_CMD__ESDHC1_CMD

This would also allow to fix the users in a separate step.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/  |

Re: [PATCH] ARM: dts: imx25-pinfunc: Always set SION for SD CMD

[Benoît Thébaudeau]

On Sat, Jan 27, 2018 at 4:37 PM, Uwe Kleine-König
 wrote:
> On Sat, Jan 27, 2018 at 01:07:52AM +0100, Benoît Thébaudeau wrote:
>> The eSDHC does not work properly if the SION bit is not set for the
>> bidirectional CMD signal, whatever the eSDHC instance and the selected
>> pad. Therefore, setting SION is mandatory for all eSDHC CMD ports. Do
>> this for MX25_PAD_*__SD*_CMD in imx25-pinfunc.h in order to enforce this
>> behavior for all boards.
>>
>> This had already been done for eSDHC1, but not for eSDHC2. Also, define
>> MX25_PAD_FEC_MDC__SDHC2_CMD so that all the possible cases are covered
>> from now on.
>
> There is an inconsistency in the naming. The eSDHC1 CMD constants are
> named:
>
> MX25_PAD_SD1_CMD__SD1_CMD
>
> The reference calls this:
>
> CMD of instance: esdhc1.
>
> The register name is correct though. Not sure it's worth to fix this to
> use consistent naming (which would result in:
>
> MX25_PAD_SD1_CMD__ESDHC1_CMD
>
> which looks ugly, too).

Indeed. I had also noticed this. I can send a patch to apply before
this one. But that would break the out-of-tree DT files using these
definitions. Would that be OK?

>> Signed-off-by: Benoît Thébaudeau 
>> ---
>>  arch/arm/boot/dts/imx25-pinfunc.h | 8 ++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/imx25-pinfunc.h 
>> b/arch/arm/boot/dts/imx25-pinfunc.h
>> index 6c63dca1b9b8..6cc71f7e1baa 100644
>> --- a/arch/arm/boot/dts/imx25-pinfunc.h
>> +++ b/arch/arm/boot/dts/imx25-pinfunc.h
>> @@ -236,7 +236,8 @@
>>  #define MX25_PAD_LD8__LD80x0e8 0x2e0 0x000 0x00 0x000
>>  #define MX25_PAD_LD8__UART4_RXD  0x0e8 0x2e0 0x570 0x02 
>> 0x000
>>  #define MX25_PAD_LD8__FEC_TX_ERR 0x0e8 0x2e0 0x000 0x05 0x000
>> -#define MX25_PAD_LD8__SDHC2_CMD  0x0e8 0x2e0 0x4e0 0x06 
>> 0x000
>> +/* The eSDHC cannot work if SION is not set for the bidirectional CMD 
>> signal. */
>
> Maybe reference the more verbose comment for MX25_PAD_SD1_CMD__SD1_CMD
> here?

Good idea. That would avoid repeating the same thing everywhere while
giving a pointer to more details.

>> +#define MX25_PAD_LD8__SDHC2_CMD  0x0e8 0x2e0 0x4e0 0x16 
>> 0x000
>>
>>  #define MX25_PAD_LD9__LD90x0ec 0x2e4 0x000 0x00 0x000
>>  #define MX25_PAD_LD9__UART4_TXD  0x0ec 0x2e4 0x000 0x02 
>> 0x000
>> @@ -316,7 +317,8 @@
>>  #define MX25_PAD_CSI_D5__CSPI3_RDY   0x12c 0x324 0x000 0x07 0x000
>>
>>  #define MX25_PAD_CSI_D6__CSI_D6  0x130 0x328 0x000 0x00 
>> 0x000
>> -#define MX25_PAD_CSI_D6__SDHC2_CMD   0x130 0x328 0x4e0 0x02 0x001
>> +/* The eSDHC cannot work if SION is not set for the bidirectional CMD 
>> signal. */
>> +#define MX25_PAD_CSI_D6__SDHC2_CMD   0x130 0x328 0x4e0 0x12 0x001
>>  #define MX25_PAD_CSI_D6__SIM1_PD00x130 0x328 0x000 0x04 0x000
>>  #define MX25_PAD_CSI_D6__GPIO_1_31   0x130 0x328 0x000 0x05 0x000
>>
>> @@ -496,6 +498,8 @@
>>  #define MX25_PAD_KPP_COL3__GPIO_3_4  0x1c4 0x3bc 0x000 0x05 0x000
>>
>>  #define MX25_PAD_FEC_MDC__FEC_MDC0x1c8 0x3c0 0x000 0x00 0x000
>> +/* The eSDHC cannot work if SION is not set for the bidirectional CMD 
>> signal. */
>> +#define MX25_PAD_FEC_MDC__SDHC2_CMD  0x1c8 0x3c0 0x4e0 0x11 0x002
>>  #define MX25_PAD_FEC_MDC__AUD4_TXD   0x1c8 0x3c0 0x464 0x02 0x001
>>  #define MX25_PAD_FEC_MDC__GPIO_3_5   0x1c8 0x3c0 0x000 0x05 0x000
>
> I double checked the numbers, and these seem all to be correct.

Thanks for your review.

Best regards,
Benoît

Re: [PATCH] ARM: dts: imx25-pinfunc: Always set SION for SD CMD

[Benoît Thébaudeau]

On Sat, Jan 27, 2018 at 4:37 PM, Uwe Kleine-König
 wrote:
> On Sat, Jan 27, 2018 at 01:07:52AM +0100, Benoît Thébaudeau wrote:
>> The eSDHC does not work properly if the SION bit is not set for the
>> bidirectional CMD signal, whatever the eSDHC instance and the selected
>> pad. Therefore, setting SION is mandatory for all eSDHC CMD ports. Do
>> this for MX25_PAD_*__SD*_CMD in imx25-pinfunc.h in order to enforce this
>> behavior for all boards.
>>
>> This had already been done for eSDHC1, but not for eSDHC2. Also, define
>> MX25_PAD_FEC_MDC__SDHC2_CMD so that all the possible cases are covered
>> from now on.
>
> There is an inconsistency in the naming. The eSDHC1 CMD constants are
> named:
>
> MX25_PAD_SD1_CMD__SD1_CMD
>
> The reference calls this:
>
> CMD of instance: esdhc1.
>
> The register name is correct though. Not sure it's worth to fix this to
> use consistent naming (which would result in:
>
> MX25_PAD_SD1_CMD__ESDHC1_CMD
>
> which looks ugly, too).

Indeed. I had also noticed this. I can send a patch to apply before
this one. But that would break the out-of-tree DT files using these
definitions. Would that be OK?

>> Signed-off-by: Benoît Thébaudeau 
>> ---
>>  arch/arm/boot/dts/imx25-pinfunc.h | 8 ++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/imx25-pinfunc.h 
>> b/arch/arm/boot/dts/imx25-pinfunc.h
>> index 6c63dca1b9b8..6cc71f7e1baa 100644
>> --- a/arch/arm/boot/dts/imx25-pinfunc.h
>> +++ b/arch/arm/boot/dts/imx25-pinfunc.h
>> @@ -236,7 +236,8 @@
>>  #define MX25_PAD_LD8__LD80x0e8 0x2e0 0x000 0x00 0x000
>>  #define MX25_PAD_LD8__UART4_RXD  0x0e8 0x2e0 0x570 0x02 
>> 0x000
>>  #define MX25_PAD_LD8__FEC_TX_ERR 0x0e8 0x2e0 0x000 0x05 0x000
>> -#define MX25_PAD_LD8__SDHC2_CMD  0x0e8 0x2e0 0x4e0 0x06 
>> 0x000
>> +/* The eSDHC cannot work if SION is not set for the bidirectional CMD 
>> signal. */
>
> Maybe reference the more verbose comment for MX25_PAD_SD1_CMD__SD1_CMD
> here?

Good idea. That would avoid repeating the same thing everywhere while
giving a pointer to more details.

>> +#define MX25_PAD_LD8__SDHC2_CMD  0x0e8 0x2e0 0x4e0 0x16 
>> 0x000
>>
>>  #define MX25_PAD_LD9__LD90x0ec 0x2e4 0x000 0x00 0x000
>>  #define MX25_PAD_LD9__UART4_TXD  0x0ec 0x2e4 0x000 0x02 
>> 0x000
>> @@ -316,7 +317,8 @@
>>  #define MX25_PAD_CSI_D5__CSPI3_RDY   0x12c 0x324 0x000 0x07 0x000
>>
>>  #define MX25_PAD_CSI_D6__CSI_D6  0x130 0x328 0x000 0x00 
>> 0x000
>> -#define MX25_PAD_CSI_D6__SDHC2_CMD   0x130 0x328 0x4e0 0x02 0x001
>> +/* The eSDHC cannot work if SION is not set for the bidirectional CMD 
>> signal. */
>> +#define MX25_PAD_CSI_D6__SDHC2_CMD   0x130 0x328 0x4e0 0x12 0x001
>>  #define MX25_PAD_CSI_D6__SIM1_PD00x130 0x328 0x000 0x04 0x000
>>  #define MX25_PAD_CSI_D6__GPIO_1_31   0x130 0x328 0x000 0x05 0x000
>>
>> @@ -496,6 +498,8 @@
>>  #define MX25_PAD_KPP_COL3__GPIO_3_4  0x1c4 0x3bc 0x000 0x05 0x000
>>
>>  #define MX25_PAD_FEC_MDC__FEC_MDC0x1c8 0x3c0 0x000 0x00 0x000
>> +/* The eSDHC cannot work if SION is not set for the bidirectional CMD 
>> signal. */
>> +#define MX25_PAD_FEC_MDC__SDHC2_CMD  0x1c8 0x3c0 0x4e0 0x11 0x002
>>  #define MX25_PAD_FEC_MDC__AUD4_TXD   0x1c8 0x3c0 0x464 0x02 0x001
>>  #define MX25_PAD_FEC_MDC__GPIO_3_5   0x1c8 0x3c0 0x000 0x05 0x000
>
> I double checked the numbers, and these seem all to be correct.

Thanks for your review.

Best regards,
Benoît

Re: [PATCH] ARM: dts: imx25-pinfunc: Always set SION for SD CMD

[Uwe Kleine-König]

On Sat, Jan 27, 2018 at 01:07:52AM +0100, Benoît Thébaudeau wrote:
> The eSDHC does not work properly if the SION bit is not set for the
> bidirectional CMD signal, whatever the eSDHC instance and the selected
> pad. Therefore, setting SION is mandatory for all eSDHC CMD ports. Do
> this for MX25_PAD_*__SD*_CMD in imx25-pinfunc.h in order to enforce this
> behavior for all boards.
> 
> This had already been done for eSDHC1, but not for eSDHC2. Also, define
> MX25_PAD_FEC_MDC__SDHC2_CMD so that all the possible cases are covered
> from now on.

There is an inconsistency in the naming. The eSDHC1 CMD constants are
named:

MX25_PAD_SD1_CMD__SD1_CMD

The reference calls this:

CMD of instance: esdhc1.

The register name is correct though. Not sure it's worth to fix this to
use consistent naming (which would result in:

MX25_PAD_SD1_CMD__ESDHC1_CMD

which looks ugly, too).

> Signed-off-by: Benoît Thébaudeau 
> ---
>  arch/arm/boot/dts/imx25-pinfunc.h | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/imx25-pinfunc.h 
> b/arch/arm/boot/dts/imx25-pinfunc.h
> index 6c63dca1b9b8..6cc71f7e1baa 100644
> --- a/arch/arm/boot/dts/imx25-pinfunc.h
> +++ b/arch/arm/boot/dts/imx25-pinfunc.h
> @@ -236,7 +236,8 @@
>  #define MX25_PAD_LD8__LD80x0e8 0x2e0 0x000 0x00 0x000
>  #define MX25_PAD_LD8__UART4_RXD  0x0e8 0x2e0 0x570 0x02 
> 0x000
>  #define MX25_PAD_LD8__FEC_TX_ERR 0x0e8 0x2e0 0x000 0x05 0x000
> -#define MX25_PAD_LD8__SDHC2_CMD  0x0e8 0x2e0 0x4e0 0x06 
> 0x000
> +/* The eSDHC cannot work if SION is not set for the bidirectional CMD 
> signal. */

Maybe reference the more verbose comment for MX25_PAD_SD1_CMD__SD1_CMD
here?

> +#define MX25_PAD_LD8__SDHC2_CMD  0x0e8 0x2e0 0x4e0 0x16 
> 0x000
>  
>  #define MX25_PAD_LD9__LD90x0ec 0x2e4 0x000 0x00 0x000
>  #define MX25_PAD_LD9__UART4_TXD  0x0ec 0x2e4 0x000 0x02 
> 0x000
> @@ -316,7 +317,8 @@
>  #define MX25_PAD_CSI_D5__CSPI3_RDY   0x12c 0x324 0x000 0x07 0x000
>  
>  #define MX25_PAD_CSI_D6__CSI_D6  0x130 0x328 0x000 0x00 
> 0x000
> -#define MX25_PAD_CSI_D6__SDHC2_CMD   0x130 0x328 0x4e0 0x02 0x001
> +/* The eSDHC cannot work if SION is not set for the bidirectional CMD 
> signal. */
> +#define MX25_PAD_CSI_D6__SDHC2_CMD   0x130 0x328 0x4e0 0x12 0x001
>  #define MX25_PAD_CSI_D6__SIM1_PD00x130 0x328 0x000 0x04 0x000
>  #define MX25_PAD_CSI_D6__GPIO_1_31   0x130 0x328 0x000 0x05 0x000
>  
> @@ -496,6 +498,8 @@
>  #define MX25_PAD_KPP_COL3__GPIO_3_4  0x1c4 0x3bc 0x000 0x05 0x000
>  
>  #define MX25_PAD_FEC_MDC__FEC_MDC0x1c8 0x3c0 0x000 0x00 0x000
> +/* The eSDHC cannot work if SION is not set for the bidirectional CMD 
> signal. */
> +#define MX25_PAD_FEC_MDC__SDHC2_CMD  0x1c8 0x3c0 0x4e0 0x11 0x002
>  #define MX25_PAD_FEC_MDC__AUD4_TXD   0x1c8 0x3c0 0x464 0x02 0x001
>  #define MX25_PAD_FEC_MDC__GPIO_3_5   0x1c8 0x3c0 0x000 0x05 0x000

I double checked the numbers, and these seem all to be correct.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/  |

Re: [PATCH] ARM: dts: imx25-pinfunc: Always set SION for SD CMD

[Uwe Kleine-König]

On Sat, Jan 27, 2018 at 01:07:52AM +0100, Benoît Thébaudeau wrote:
> The eSDHC does not work properly if the SION bit is not set for the
> bidirectional CMD signal, whatever the eSDHC instance and the selected
> pad. Therefore, setting SION is mandatory for all eSDHC CMD ports. Do
> this for MX25_PAD_*__SD*_CMD in imx25-pinfunc.h in order to enforce this
> behavior for all boards.
> 
> This had already been done for eSDHC1, but not for eSDHC2. Also, define
> MX25_PAD_FEC_MDC__SDHC2_CMD so that all the possible cases are covered
> from now on.

There is an inconsistency in the naming. The eSDHC1 CMD constants are
named:

MX25_PAD_SD1_CMD__SD1_CMD

The reference calls this:

CMD of instance: esdhc1.

The register name is correct though. Not sure it's worth to fix this to
use consistent naming (which would result in:

MX25_PAD_SD1_CMD__ESDHC1_CMD

which looks ugly, too).

> Signed-off-by: Benoît Thébaudeau 
> ---
>  arch/arm/boot/dts/imx25-pinfunc.h | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/imx25-pinfunc.h 
> b/arch/arm/boot/dts/imx25-pinfunc.h
> index 6c63dca1b9b8..6cc71f7e1baa 100644
> --- a/arch/arm/boot/dts/imx25-pinfunc.h
> +++ b/arch/arm/boot/dts/imx25-pinfunc.h
> @@ -236,7 +236,8 @@
>  #define MX25_PAD_LD8__LD80x0e8 0x2e0 0x000 0x00 0x000
>  #define MX25_PAD_LD8__UART4_RXD  0x0e8 0x2e0 0x570 0x02 
> 0x000
>  #define MX25_PAD_LD8__FEC_TX_ERR 0x0e8 0x2e0 0x000 0x05 0x000
> -#define MX25_PAD_LD8__SDHC2_CMD  0x0e8 0x2e0 0x4e0 0x06 
> 0x000
> +/* The eSDHC cannot work if SION is not set for the bidirectional CMD 
> signal. */

Maybe reference the more verbose comment for MX25_PAD_SD1_CMD__SD1_CMD
here?

> +#define MX25_PAD_LD8__SDHC2_CMD  0x0e8 0x2e0 0x4e0 0x16 
> 0x000
>  
>  #define MX25_PAD_LD9__LD90x0ec 0x2e4 0x000 0x00 0x000
>  #define MX25_PAD_LD9__UART4_TXD  0x0ec 0x2e4 0x000 0x02 
> 0x000
> @@ -316,7 +317,8 @@
>  #define MX25_PAD_CSI_D5__CSPI3_RDY   0x12c 0x324 0x000 0x07 0x000
>  
>  #define MX25_PAD_CSI_D6__CSI_D6  0x130 0x328 0x000 0x00 
> 0x000
> -#define MX25_PAD_CSI_D6__SDHC2_CMD   0x130 0x328 0x4e0 0x02 0x001
> +/* The eSDHC cannot work if SION is not set for the bidirectional CMD 
> signal. */
> +#define MX25_PAD_CSI_D6__SDHC2_CMD   0x130 0x328 0x4e0 0x12 0x001
>  #define MX25_PAD_CSI_D6__SIM1_PD00x130 0x328 0x000 0x04 0x000
>  #define MX25_PAD_CSI_D6__GPIO_1_31   0x130 0x328 0x000 0x05 0x000
>  
> @@ -496,6 +498,8 @@
>  #define MX25_PAD_KPP_COL3__GPIO_3_4  0x1c4 0x3bc 0x000 0x05 0x000
>  
>  #define MX25_PAD_FEC_MDC__FEC_MDC0x1c8 0x3c0 0x000 0x00 0x000
> +/* The eSDHC cannot work if SION is not set for the bidirectional CMD 
> signal. */
> +#define MX25_PAD_FEC_MDC__SDHC2_CMD  0x1c8 0x3c0 0x4e0 0x11 0x002
>  #define MX25_PAD_FEC_MDC__AUD4_TXD   0x1c8 0x3c0 0x464 0x02 0x001
>  #define MX25_PAD_FEC_MDC__GPIO_3_5   0x1c8 0x3c0 0x000 0x05 0x000

I double checked the numbers, and these seem all to be correct.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/  |

Re: [PATCH] ARM: dts: imx25-pinfunc: Always set SION for SD CMD

[Fabio Estevam]

On Fri, Jan 26, 2018 at 10:07 PM, Benoît Thébaudeau
 wrote:
> The eSDHC does not work properly if the SION bit is not set for the
> bidirectional CMD signal, whatever the eSDHC instance and the selected
> pad. Therefore, setting SION is mandatory for all eSDHC CMD ports. Do
> this for MX25_PAD_*__SD*_CMD in imx25-pinfunc.h in order to enforce this
> behavior for all boards.
>
> This had already been done for eSDHC1, but not for eSDHC2. Also, define
> MX25_PAD_FEC_MDC__SDHC2_CMD so that all the possible cases are covered
> from now on.
>
> Signed-off-by: Benoît Thébaudeau 

Thanks for the fix:

Reviewed-by: Fabio Estevam 

Re: [PATCH] ARM: dts: imx25-pinfunc: Always set SION for SD CMD

[Fabio Estevam]

On Fri, Jan 26, 2018 at 10:07 PM, Benoît Thébaudeau
 wrote:
> The eSDHC does not work properly if the SION bit is not set for the
> bidirectional CMD signal, whatever the eSDHC instance and the selected
> pad. Therefore, setting SION is mandatory for all eSDHC CMD ports. Do
> this for MX25_PAD_*__SD*_CMD in imx25-pinfunc.h in order to enforce this
> behavior for all boards.
>
> This had already been done for eSDHC1, but not for eSDHC2. Also, define
> MX25_PAD_FEC_MDC__SDHC2_CMD so that all the possible cases are covered
> from now on.
>
> Signed-off-by: Benoît Thébaudeau 

Thanks for the fix:

Reviewed-by: Fabio Estevam