diff mbox

[U-Boot] nitrogen6x: Fix the PAD settings for the ECSPI chipselect

Message ID 1397249033-11124-1-git-send-email-festevam@gmail.com
State Accepted
Delegated to: Stefano Babic
Headers show

Commit Message

Fabio Estevam April 11, 2014, 8:43 p.m. UTC 
From: Fabio Estevam <fabio.estevam@freescale.com>

ECSPI chipselect (MX6_PAD_EIM_D19__GPIO3_IO19) is used with GPIO functionality,
so it does not make sense to set its pad as SPI pin.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
Eric,

This is untested.

 board/boundary/nitrogen6x/nitrogen6x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Eric Nelson April 12, 2014, 3:04 p.m. UTC | #1 
Thanks Fabio,

On 04/11/2014 01:43 PM, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
>
> ECSPI chipselect (MX6_PAD_EIM_D19__GPIO3_IO19) is used with GPIO functionality,
> so it does not make sense to set its pad as SPI pin.
>

You're right, although most of the SPI pad settings won't have
any effect as soon as the pad is configured as a GPIO output:
	http://git.denx.de/?p=u-boot.git;a=blob;f=board/boundary/nitrogen6x/nitrogen6x.c;h=d9c05b07bfae83466513735cbec95c7f949de7a5;hb=HEAD#l47

> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
> Eric,
>
> This is untested.
>
>   board/boundary/nitrogen6x/nitrogen6x.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/board/boundary/nitrogen6x/nitrogen6x.c b/board/boundary/nitrogen6x/nitrogen6x.c
> index d9c05b0..f2492e4 100644
> --- a/board/boundary/nitrogen6x/nitrogen6x.c
> +++ b/board/boundary/nitrogen6x/nitrogen6x.c
> @@ -331,7 +331,7 @@ int board_mmc_init(bd_t *bis)
>   #ifdef CONFIG_MXC_SPI
>   iomux_v3_cfg_t const ecspi1_pads[] = {
>   	/* SS1 */
> -	MX6_PAD_EIM_D19__GPIO3_IO19   | MUX_PAD_CTRL(SPI_PAD_CTRL),
> +	MX6_PAD_EIM_D19__GPIO3_IO19  | MUX_PAD_CTRL(NO_PAD_CTRL),
>   	MX6_PAD_EIM_D17__ECSPI1_MISO | MUX_PAD_CTRL(SPI_PAD_CTRL),
>   	MX6_PAD_EIM_D18__ECSPI1_MOSI | MUX_PAD_CTRL(SPI_PAD_CTRL),
>   	MX6_PAD_EIM_D16__ECSPI1_SCLK | MUX_PAD_CTRL(SPI_PAD_CTRL),
>

I'll test this out ASAP, but I have to ask: what made you
look at this pin's settings.

It seems that the only bit which could affect things is dropping
the drive strength setting, and 40 ohms is the power-on default.

Regards,


Eric
Fabio Estevam April 12, 2014, 3:32 p.m. UTC | #2 
Hi Eric,

On Sat, Apr 12, 2014 at 12:04 PM, Eric Nelson
<eric.nelson@boundarydevices.com> wrote:

> I'll test this out ASAP, but I have to ask: what made you
> look at this pin's settings.

I was debugging a SPI issue on mx6sl evk, so that's why I looked at it
for comparison.

Regards,

Fabio Estevam
Eric Nelson April 12, 2014, 10:54 p.m. UTC | #3 
Hi Fabio,

On 04/11/2014 01:43 PM, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
>
> ECSPI chipselect (MX6_PAD_EIM_D19__GPIO3_IO19) is used with GPIO functionality,
> so it does not make sense to set its pad as SPI pin.
>
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
> Eric,
>
> This is untested.
>
>   board/boundary/nitrogen6x/nitrogen6x.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/board/boundary/nitrogen6x/nitrogen6x.c b/board/boundary/nitrogen6x/nitrogen6x.c
> index d9c05b0..f2492e4 100644
> --- a/board/boundary/nitrogen6x/nitrogen6x.c
> +++ b/board/boundary/nitrogen6x/nitrogen6x.c
> @@ -331,7 +331,7 @@ int board_mmc_init(bd_t *bis)
>   #ifdef CONFIG_MXC_SPI
>   iomux_v3_cfg_t const ecspi1_pads[] = {
>   	/* SS1 */
> -	MX6_PAD_EIM_D19__GPIO3_IO19   | MUX_PAD_CTRL(SPI_PAD_CTRL),
> +	MX6_PAD_EIM_D19__GPIO3_IO19  | MUX_PAD_CTRL(NO_PAD_CTRL),
>   	MX6_PAD_EIM_D17__ECSPI1_MISO | MUX_PAD_CTRL(SPI_PAD_CTRL),
>   	MX6_PAD_EIM_D18__ECSPI1_MOSI | MUX_PAD_CTRL(SPI_PAD_CTRL),
>   	MX6_PAD_EIM_D16__ECSPI1_SCLK | MUX_PAD_CTRL(SPI_PAD_CTRL),
>

Tested-by: Eric Nelson <eric.nelson@boundarydevices.com>
Acked-by: Eric Nelson <eric.nelson@boundarydevices.com>
Troy Kisky April 13, 2014, 10:08 p.m. UTC | #4 
On 4/12/2014 3:54 PM, Eric Nelson wrote:
> Hi Fabio,
> 
> On 04/11/2014 01:43 PM, Fabio Estevam wrote:
>> From: Fabio Estevam <fabio.estevam@freescale.com>
>>
>> ECSPI chipselect (MX6_PAD_EIM_D19__GPIO3_IO19) is used with GPIO functionality,
>> so it does not make sense to set its pad as SPI pin.
>>
>> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
>> ---
>> Eric,
>>
>> This is untested.
>>
>>   board/boundary/nitrogen6x/nitrogen6x.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/board/boundary/nitrogen6x/nitrogen6x.c b/board/boundary/nitrogen6x/nitrogen6x.c
>> index d9c05b0..f2492e4 100644
>> --- a/board/boundary/nitrogen6x/nitrogen6x.c
>> +++ b/board/boundary/nitrogen6x/nitrogen6x.c
>> @@ -331,7 +331,7 @@ int board_mmc_init(bd_t *bis)
>>   #ifdef CONFIG_MXC_SPI
>>   iomux_v3_cfg_t const ecspi1_pads[] = {
>>       /* SS1 */
>> -    MX6_PAD_EIM_D19__GPIO3_IO19   | MUX_PAD_CTRL(SPI_PAD_CTRL),
>> +    MX6_PAD_EIM_D19__GPIO3_IO19  | MUX_PAD_CTRL(NO_PAD_CTRL),
>>       MX6_PAD_EIM_D17__ECSPI1_MISO | MUX_PAD_CTRL(SPI_PAD_CTRL),
>>       MX6_PAD_EIM_D18__ECSPI1_MOSI | MUX_PAD_CTRL(SPI_PAD_CTRL),
>>       MX6_PAD_EIM_D16__ECSPI1_SCLK | MUX_PAD_CTRL(SPI_PAD_CTRL),
>>
> 
> Tested-by: Eric Nelson <eric.nelson@boundarydevices.com>
> Acked-by: Eric Nelson <eric.nelson@boundarydevices.com>

NAK. Please don't use NO_PAD_CTRL. What is wrong with
SPI_PAD_CTRL. Your commit message doesn't say.
It is an SPI pin (even if used as a GPIO,) so
why doesn't it make sense.

I think your fixing a problem that doesn't exist.

Troy
Fabio Estevam April 13, 2014, 11:01 p.m. UTC | #5 
On Sun, Apr 13, 2014 at 7:08 PM, Troy Kisky
<troy.kisky@boundarydevices.com> wrote:

> NAK. Please don't use NO_PAD_CTRL. What is wrong with
> SPI_PAD_CTRL. Your commit message doesn't say.
> It is an SPI pin (even if used as a GPIO,) so
> why doesn't it make sense.

SPI_PAD_CTRL should be used by the pads that have SPI functionality.

This is not the case for the MX6_PAD_EIM_D19__GPIO3_IO19, which is a
GPIO, so why SPI_PAD_CTRL?

If we follow your argument then the enet_pads1 array is incorrect and
we should change all of them to ENET_PAD_CTRL instead.

iomux_v3_cfg_t const enet_pads1[] = {
    MX6_PAD_ENET_MDIO__ENET_MDIO        | MUX_PAD_CTRL(ENET_PAD_CTRL),
    MX6_PAD_ENET_MDC__ENET_MDC        | MUX_PAD_CTRL(ENET_PAD_CTRL),
    MX6_PAD_RGMII_TXC__RGMII_TXC    | MUX_PAD_CTRL(ENET_PAD_CTRL),
    MX6_PAD_RGMII_TD0__RGMII_TD0    | MUX_PAD_CTRL(ENET_PAD_CTRL),
    MX6_PAD_RGMII_TD1__RGMII_TD1    | MUX_PAD_CTRL(ENET_PAD_CTRL),
    MX6_PAD_RGMII_TD2__RGMII_TD2    | MUX_PAD_CTRL(ENET_PAD_CTRL),
    MX6_PAD_RGMII_TD3__RGMII_TD3    | MUX_PAD_CTRL(ENET_PAD_CTRL),
    MX6_PAD_RGMII_TX_CTL__RGMII_TX_CTL    | MUX_PAD_CTRL(ENET_PAD_CTRL),
    MX6_PAD_ENET_REF_CLK__ENET_TX_CLK    | MUX_PAD_CTRL(ENET_PAD_CTRL),
    /* pin 35 - 1 (PHY_AD2) on reset */
    MX6_PAD_RGMII_RXC__GPIO6_IO30        | MUX_PAD_CTRL(NO_PAD_CTRL),
    /* pin 32 - 1 - (MODE0) all */
    MX6_PAD_RGMII_RD0__GPIO6_IO25        | MUX_PAD_CTRL(NO_PAD_CTRL),
    /* pin 31 - 1 - (MODE1) all */
    MX6_PAD_RGMII_RD1__GPIO6_IO27        | MUX_PAD_CTRL(NO_PAD_CTRL),
    /* pin 28 - 1 - (MODE2) all */
    MX6_PAD_RGMII_RD2__GPIO6_IO28        | MUX_PAD_CTRL(NO_PAD_CTRL),
    /* pin 27 - 1 - (MODE3) all */
    MX6_PAD_RGMII_RD3__GPIO6_IO29        | MUX_PAD_CTRL(NO_PAD_CTRL),
    /* pin 33 - 1 - (CLK125_EN) 125Mhz clockout enabled */
    MX6_PAD_RGMII_RX_CTL__GPIO6_IO24    | MUX_PAD_CTRL(NO_PAD_CTRL),
    /* pin 42 PHY nRST */
    MX6_PAD_EIM_D23__GPIO3_IO23        | MUX_PAD_CTRL(NO_PAD_CTRL),
    MX6_PAD_ENET_RXD0__GPIO1_IO27        | MUX_PAD_CTRL(NO_PAD_CTRL),
};
Troy Kisky April 13, 2014, 11:21 p.m. UTC | #6 
On 4/13/2014 4:01 PM, Fabio Estevam wrote:
> On Sun, Apr 13, 2014 at 7:08 PM, Troy Kisky
> <troy.kisky@boundarydevices.com> wrote:
> 
>> NAK. Please don't use NO_PAD_CTRL. What is wrong with
>> SPI_PAD_CTRL. Your commit message doesn't say.
>> It is an SPI pin (even if used as a GPIO,) so
>> why doesn't it make sense.
> 
> SPI_PAD_CTRL should be used by the pads that have SPI functionality.
> 
> This is not the case for the MX6_PAD_EIM_D19__GPIO3_IO19, which is a
> GPIO, so why SPI_PAD_CTRL?
> 
> If we follow your argument then the enet_pads1 array is incorrect and
> we should change all of them to ENET_PAD_CTRL instead.

I would ack that patch. I do believe that all NO_PAD_CTRL should
be replaced with whatever the register actually contains currently.
A "nop" patch that just makes things explicit.

Would you have a problem with that patch?

For your particular example of enet, I see no reason that the
pad settings should change when switching the mux from ENET to
gpio.


Btw, I do appreciate your looking at this board file.
Sorry, if I sounded rude.

Troy
Stefano Babic April 28, 2014, 12:04 p.m. UTC | #7 
On 11/04/2014 22:43, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> ECSPI chipselect (MX6_PAD_EIM_D19__GPIO3_IO19) is used with GPIO functionality,
> so it does not make sense to set its pad as SPI pin.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---

Near Eric's Acked and Tested-by, I see Troy agree on this patch, too.

Applied to u-boot-imx, thanks !

Best regards,
Stefano Babic
diff mbox

Patch

diff --git a/board/boundary/nitrogen6x/nitrogen6x.c b/board/boundary/nitrogen6x/nitrogen6x.c
index d9c05b0..f2492e4 100644
--- a/board/boundary/nitrogen6x/nitrogen6x.c
+++ b/board/boundary/nitrogen6x/nitrogen6x.c
@@ -331,7 +331,7 @@  int board_mmc_init(bd_t *bis)
 #ifdef CONFIG_MXC_SPI
 iomux_v3_cfg_t const ecspi1_pads[] = {
 	/* SS1 */
-	MX6_PAD_EIM_D19__GPIO3_IO19   | MUX_PAD_CTRL(SPI_PAD_CTRL),
+	MX6_PAD_EIM_D19__GPIO3_IO19  | MUX_PAD_CTRL(NO_PAD_CTRL),
 	MX6_PAD_EIM_D17__ECSPI1_MISO | MUX_PAD_CTRL(SPI_PAD_CTRL),
 	MX6_PAD_EIM_D18__ECSPI1_MOSI | MUX_PAD_CTRL(SPI_PAD_CTRL),
 	MX6_PAD_EIM_D16__ECSPI1_SCLK | MUX_PAD_CTRL(SPI_PAD_CTRL),