[U-Boot] [PATCH 1/6] usb: ehci: mx6: Add support for i.MX6SL

[U-Boot] [PATCH 1/6] usb: ehci: mx6: Add support for i.MX6SL

[Otavio Salvador]

The i.MX6SL has a different base address for the controller. This
patch adapts the driver to support the different base address for this
case.

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
---

 drivers/usb/host/ehci-mx6.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
index c0a557b..5ba1c5e 100644
--- a/drivers/usb/host/ehci-mx6.c
+++ b/drivers/usb/host/ehci-mx6.c
@@ -53,6 +53,12 @@
 #define UCMD_RUN_STOP           (1 << 0) /* controller run/stop */
 #define UCMD_RESET		(1 << 1) /* controller reset */
 
+#ifdef CONFIG_MX6SL
+#define USB_BASE_ADDR		USBO2H_USB_BASE_ADDR
+#else
+#define USB_BASE_ADDR		USBOH3_USB_BASE_ADDR
+#endif
+
 static const unsigned phy_bases[] = {
 	USB_PHY0_BASE_ADDR,
 	USB_PHY1_BASE_ADDR,
@@ -174,7 +180,7 @@ struct usbnc_regs {
 
 static void usb_oc_config(int index)
 {
-	struct usbnc_regs *usbnc = (struct usbnc_regs *)(USBOH3_USB_BASE_ADDR +
+	struct usbnc_regs *usbnc = (struct usbnc_regs *)(USB_BASE_ADDR +
 			USB_OTHERREGS_OFFSET);
 	void __iomem *ctrl = (void __iomem *)(&usbnc->ctrl[index]);
 	u32 val;
@@ -207,7 +213,7 @@ int ehci_hcd_init(int index, enum usb_init_type init,
 		struct ehci_hccr **hccr, struct ehci_hcor **hcor)
 {
 	enum usb_init_type type;
-	struct usb_ehci *ehci = (struct usb_ehci *)(USBOH3_USB_BASE_ADDR +
+	struct usb_ehci *ehci = (struct usb_ehci *)(USB_BASE_ADDR +
 		(0x200 * index));
 
 	if (index > 3)
-- 
2.0.0

[U-Boot] [PATCH 2/6] imx: mx6sl: Extend USDHC SD2 pins to support 8-wire use

[Otavio Salvador]

This adds the DATA[4-7] and RST pin definitions.

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
---

 arch/arm/include/asm/arch-mx6/mx6sl_pins.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/include/asm/arch-mx6/mx6sl_pins.h b/arch/arm/include/asm/arch-mx6/mx6sl_pins.h
index 045ccc4..3ffd176 100644
--- a/arch/arm/include/asm/arch-mx6/mx6sl_pins.h
+++ b/arch/arm/include/asm/arch-mx6/mx6sl_pins.h
@@ -20,6 +20,11 @@ enum {
 	MX6_PAD_SD2_DAT1__USDHC2_DAT1				= IOMUX_PAD(0x0568, 0x0260, 0, 0x0000, 0, 0),
 	MX6_PAD_SD2_DAT2__USDHC2_DAT2				= IOMUX_PAD(0x056C, 0x0264, 0, 0x0000, 0, 0),
 	MX6_PAD_SD2_DAT3__USDHC2_DAT3				= IOMUX_PAD(0x0570, 0x0268, 0, 0x0000, 0, 0),
+	MX6_PAD_SD2_DAT4__USDHC2_DAT4				= IOMUX_PAD(0X0574, 0X026C, 0, 0X0000, 0, 0),
+	MX6_PAD_SD2_DAT5__USDHC2_DAT5				= IOMUX_PAD(0X0578, 0X0270, 0, 0X0000, 0, 0),
+	MX6_PAD_SD2_DAT6__USDHC2_DAT6				= IOMUX_PAD(0X057C, 0X0274, 0, 0X0000, 0, 0),
+	MX6_PAD_SD2_DAT7__USDHC2_DAT7				= IOMUX_PAD(0X0580, 0X0278, 0, 0X0000, 0, 0),
+	MX6_PAD_SD2_RST__USDHC2_RST				= IOMUX_PAD(0x0584, 0x027C, 0, 0x0000, 0, 0),
 	MX6_PAD_UART1_RXD__UART1_RXD				= IOMUX_PAD(0x05A0, 0x0298, 0, 0x07FC, 0, 0),
 	MX6_PAD_UART1_TXD__UART1_TXD				= IOMUX_PAD(0x05A4, 0x029C, 0, 0x0000, 0, 0),
 
-- 
2.0.0

[U-Boot] [PATCH 3/6] mmc: fsl_esdhc: Add CMD11 support to switch to 1.8V

[Otavio Salvador]

This adds support to switch to 1.8V in case CMD11 succeeds.

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
---

 drivers/mmc/fsl_esdhc.c | 30 +++++++++++++++++++++++-------
 include/fsl_esdhc.h     |  2 ++
 include/mmc.h           |  1 +
 3 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
index 5541613..c75b38f 100644
--- a/drivers/mmc/fsl_esdhc.c
+++ b/drivers/mmc/fsl_esdhc.c
@@ -47,19 +47,21 @@ struct fsl_esdhc {
 	uint    fevt;		/* Force event register */
 	uint    admaes;		/* ADMA error status register */
 	uint    adsaddr;	/* ADMA system address register */
-	char    reserved2[160];	/* reserved */
+	char    reserved2[100];	/* reserved */
+	uint    vendorspec;	/* Vendor Specific register */
+	char    reserved3[59];	/* reserved */
 	uint    hostver;	/* Host controller version register */
-	char    reserved3[4];	/* reserved */
-	uint    dmaerraddr;	/* DMA error address register */
 	char    reserved4[4];	/* reserved */
-	uint    dmaerrattr;	/* DMA error attribute register */
+	uint    dmaerraddr;	/* DMA error address register */
 	char    reserved5[4];	/* reserved */
+	uint    dmaerrattr;	/* DMA error attribute register */
+	char    reserved6[4];	/* reserved */
 	uint    hostcapblt2;	/* Host controller capabilities register 2 */
-	char    reserved6[8];	/* reserved */
+	char    reserved7[8];	/* reserved */
 	uint    tcr;		/* Tuning control register */
-	char    reserved7[28];	/* reserved */
+	char    reserved8[28];	/* reserved */
 	uint    sddirctl;	/* SD direction control register */
-	char    reserved8[712];	/* reserved */
+	char    reserved9[712];	/* reserved */
 	uint    scr;		/* eSDHC control register */
 };
 
@@ -334,6 +336,15 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)
 		goto out;
 	}
 
+	/* Switch voltage to 1.8V if CMD11 succeeded */
+	if (cmd->cmdidx == SD_CMD_SWITCH_UHS18V) {
+		esdhc_setbits32(&regs->vendorspec, ESDHC_VENDORSPEC_VSELECT);
+
+		printf("Run CMD11 1.8V switch\n");
+		/* Sleep for 5 ms - max time for card to switch to 1.8V */
+		udelay(5000);
+	}
+
 	/* Workaround for ESDHC errata ENGcm03648 */
 	if (!data && (cmd->resp_type & MMC_RSP_BUSY)) {
 		int timeout = 2500;
@@ -406,6 +417,11 @@ out:
 			while ((esdhc_read32(&regs->sysctl) & SYSCTL_RSTD))
 				;
 		}
+
+		/* If this was CMD11, then notify that power cycle is needed */
+		if (cmd->cmdidx == SD_CMD_SWITCH_UHS18V)
+			printf("CMD11 to switch to 1.8V mode failed."
+				"Card requires power cycle\n");
 	}
 
 	esdhc_write32(&regs->irqstat, -1);
diff --git a/include/fsl_esdhc.h b/include/fsl_esdhc.h
index 9814964..8876224 100644
--- a/include/fsl_esdhc.h
+++ b/include/fsl_esdhc.h
@@ -154,6 +154,8 @@
 #define ESDHC_HOSTCAPBLT_DMAS	0x00400000
 #define ESDHC_HOSTCAPBLT_HSS	0x00200000
 
+#define ESDHC_VENDORSPEC_VSELECT 0x00000002 /* Use 1.8V */
+
 struct fsl_esdhc_cfg {
 	u32	esdhc_base;
 	u32	sdhc_clk;
diff --git a/include/mmc.h b/include/mmc.h
index a3a100b..1b354ff 100644
--- a/include/mmc.h
+++ b/include/mmc.h
@@ -89,6 +89,7 @@
 #define SD_CMD_SEND_RELATIVE_ADDR	3
 #define SD_CMD_SWITCH_FUNC		6
 #define SD_CMD_SEND_IF_COND		8
+#define SD_CMD_SWITCH_UHS18V		11
 
 #define SD_CMD_APP_SET_BUS_WIDTH	6
 #define SD_CMD_ERASE_WR_BLK_START	32
-- 
2.0.0

[U-Boot] [PATCH 3/6] mmc: fsl_esdhc: Add CMD11 support to switch to 1.8V

[Andy Fleming]

On Sun, Jun 15, 2014 at 7:46 PM, Otavio Salvador
<otavio@ossystems.com.br> wrote:
> This adds support to switch to 1.8V in case CMD11 succeeds.
>
> Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
> ---
>
>  drivers/mmc/fsl_esdhc.c | 30 +++++++++++++++++++++++-------
>  include/fsl_esdhc.h     |  2 ++
>  include/mmc.h           |  1 +
>  3 files changed, 26 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
> index 5541613..c75b38f 100644
> --- a/drivers/mmc/fsl_esdhc.c
> +++ b/drivers/mmc/fsl_esdhc.c
> @@ -47,19 +47,21 @@ struct fsl_esdhc {
>         uint    fevt;           /* Force event register */
>         uint    admaes;         /* ADMA error status register */
>         uint    adsaddr;        /* ADMA system address register */
> -       char    reserved2[160]; /* reserved */
> +       char    reserved2[100]; /* reserved */
> +       uint    vendorspec;     /* Vendor Specific register */
> +       char    reserved3[59];  /* reserved */
>         uint    hostver;        /* Host controller version register */
> -       char    reserved3[4];   /* reserved */
> -       uint    dmaerraddr;     /* DMA error address register */
>         char    reserved4[4];   /* reserved */
> -       uint    dmaerrattr;     /* DMA error attribute register */
> +       uint    dmaerraddr;     /* DMA error address register */
>         char    reserved5[4];   /* reserved */
> +       uint    dmaerrattr;     /* DMA error attribute register */
> +       char    reserved6[4];   /* reserved */
>         uint    hostcapblt2;    /* Host controller capabilities register 2 */
> -       char    reserved6[8];   /* reserved */
> +       char    reserved7[8];   /* reserved */
>         uint    tcr;            /* Tuning control register */
> -       char    reserved7[28];  /* reserved */
> +       char    reserved8[28];  /* reserved */
>         uint    sddirctl;       /* SD direction control register */
> -       char    reserved8[712]; /* reserved */
> +       char    reserved9[712]; /* reserved */
>         uint    scr;            /* eSDHC control register */
>  };
>
> @@ -334,6 +336,15 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)
>                 goto out;
>         }
>
> +       /* Switch voltage to 1.8V if CMD11 succeeded */
> +       if (cmd->cmdidx == SD_CMD_SWITCH_UHS18V) {
> +               esdhc_setbits32(&regs->vendorspec, ESDHC_VENDORSPEC_VSELECT);
> +
> +               printf("Run CMD11 1.8V switch\n");
> +               /* Sleep for 5 ms - max time for card to switch to 1.8V */
> +               udelay(5000);
> +       }
> +


This seems like the wrong place to put this sort of change. send_cmd
is meant to deal with sending a command and dealing with any errors.
Voltage changes should probably be handled by set_ios. It looks like
Linux actually implements a separate op for voltage change, but the
operation should be:

send CMD11
deactivate clock
change voltage
wait 5ms
reactivate clock
wait 1ms
check for errors


Andy

[U-Boot] [PATCH 4/6] mmc: fsl_esdhc: Add support to force VSELECT set

[Otavio Salvador]

There are board were we cannot do voltage negotiation but want to set
the VSELECT bit forcely to ensure it to work at 1.8V.

This commit adds CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT flag for this use.

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
---

 drivers/mmc/fsl_esdhc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
index c75b38f..b3870e2 100644
--- a/drivers/mmc/fsl_esdhc.c
+++ b/drivers/mmc/fsl_esdhc.c
@@ -517,6 +517,10 @@ static int esdhc_init(struct mmc *mmc)
 	/* Set timout to the maximum value */
 	esdhc_clrsetbits32(&regs->sysctl, SYSCTL_TIMEOUT_MASK, 14 << 16);
 
+#ifdef CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT
+	esdhc_setbits32(&regs->vendorspec, ESDHC_VENDORSPEC_VSELECT);
+#endif
+
 	return 0;
 }
 
-- 
2.0.0

[U-Boot] [PATCH 4/6] mmc: fsl_esdhc: Add support to force VSELECT set

[Marek Vasut]

On Monday, June 16, 2014 at 02:46:51 AM, Otavio Salvador wrote:
> There are board were

Please fix your English and send a patch, thanks :)

> we cannot do voltage negotiation but want to set
> the VSELECT bit forcely to ensure it to work at 1.8V.
> 
> This commit adds CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT flag for this use.
> 
> Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
> ---
> 
>  drivers/mmc/fsl_esdhc.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
> index c75b38f..b3870e2 100644
> --- a/drivers/mmc/fsl_esdhc.c
> +++ b/drivers/mmc/fsl_esdhc.c
> @@ -517,6 +517,10 @@ static int esdhc_init(struct mmc *mmc)
>  	/* Set timout to the maximum value */
>  	esdhc_clrsetbits32(&regs->sysctl, SYSCTL_TIMEOUT_MASK, 14 << 16);
> 
> +#ifdef CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT
> +	esdhc_setbits32(&regs->vendorspec, ESDHC_VENDORSPEC_VSELECT);
> +#endif

Documentation is missing.

Best regards,
Marek Vasut

[U-Boot] [PATCH 4/6] mmc: fsl_esdhc: Add support to force VSELECT set

[Otavio Salvador]

On Sun, Jun 15, 2014 at 9:51 PM, Marek Vasut <marex@denx.de> wrote:
> On Monday, June 16, 2014 at 02:46:51 AM, Otavio Salvador wrote:
>> There are board were
>
> Please fix your English and send a patch, thanks :)

I fixed the commit log, thanks.

...
>> +#ifdef CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT
>> +     esdhc_setbits32(&regs->vendorspec, ESDHC_VENDORSPEC_VSELECT);
>> +#endif
>
> Documentation is missing.

There is no FSL ESDHC README file so that's why I didn't include it anywhere.

-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

[U-Boot] [PATCH 4/6] mmc: fsl_esdhc: Add support to force VSELECT set

[Marek Vasut]

On Monday, June 16, 2014 at 03:22:22 AM, Otavio Salvador wrote:

[...]

> >> +#ifdef CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT
> >> +     esdhc_setbits32(&regs->vendorspec, ESDHC_VENDORSPEC_VSELECT);
> >> +#endif
> > 
> > Documentation is missing.
> 
> There is no FSL ESDHC README file so that's why I didn't include it
> anywhere.

I'm at loss for words here, really...

I think you know what needs to be done (hint: write the documentation), right ?

Best regards,
Marek Vasut

[U-Boot] [PATCH 4/6] mmc: fsl_esdhc: Add support to force VSELECT set

[Otavio Salvador]

On Sun, Jun 15, 2014 at 10:27 PM, Marek Vasut <marex@denx.de> wrote:
> On Monday, June 16, 2014 at 03:22:22 AM, Otavio Salvador wrote:
>
> [...]
>
>> >> +#ifdef CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT
>> >> +     esdhc_setbits32(&regs->vendorspec, ESDHC_VENDORSPEC_VSELECT);
>> >> +#endif
>> >
>> > Documentation is missing.
>>
>> There is no FSL ESDHC README file so that's why I didn't include it
>> anywhere.
>
> I'm at loss for words here, really...
>
> I think you know what needs to be done (hint: write the documentation), right ?

I won't write the full documentation for it. I am sorry.

-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

[U-Boot] [PATCH 4/6] mmc: fsl_esdhc: Add support to force VSELECT set

[Marek Vasut]

On Monday, June 16, 2014 at 03:39:08 AM, Otavio Salvador wrote:
> On Sun, Jun 15, 2014 at 10:27 PM, Marek Vasut <marex@denx.de> wrote:
> > On Monday, June 16, 2014 at 03:22:22 AM, Otavio Salvador wrote:
> > 
> > [...]
> > 
> >> >> +#ifdef CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT
> >> >> +     esdhc_setbits32(&regs->vendorspec, ESDHC_VENDORSPEC_VSELECT);
> >> >> +#endif
> >> > 
> >> > Documentation is missing.
> >> 
> >> There is no FSL ESDHC README file so that's why I didn't include it
> >> anywhere.
> > 
> > I'm at loss for words here, really...
> > 
> > I think you know what needs to be done (hint: write the documentation),
> > right ?
> 
> I won't write the full documentation for it. I am sorry.

Undocumented configuration option is not acceptable, period.

Best regards,
Marek Vasut

[U-Boot] [PATCH 4/6] mmc: fsl_esdhc: Add support to force VSELECT set

[Otavio Salvador]

On Sun, Jun 15, 2014 at 11:03 PM, Marek Vasut <marex@denx.de> wrote:
> On Monday, June 16, 2014 at 03:39:08 AM, Otavio Salvador wrote:
>> On Sun, Jun 15, 2014 at 10:27 PM, Marek Vasut <marex@denx.de> wrote:
>> > On Monday, June 16, 2014 at 03:22:22 AM, Otavio Salvador wrote:
>> >
>> > [...]
>> >
>> >> >> +#ifdef CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT
>> >> >> +     esdhc_setbits32(&regs->vendorspec, ESDHC_VENDORSPEC_VSELECT);
>> >> >> +#endif
>> >> >
>> >> > Documentation is missing.
>> >>
>> >> There is no FSL ESDHC README file so that's why I didn't include it
>> >> anywhere.
>> >
>> > I'm at loss for words here, really...
>> >
>> > I think you know what needs to be done (hint: write the documentation),
>> > right ?
>>
>> I won't write the full documentation for it. I am sorry.
>
> Undocumented configuration option is not acceptable, period.

Who accepted the driver in the first version, without Doc?

I am not in the position to write the full doc.

-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

[U-Boot] [PATCH 4/6] mmc: fsl_esdhc: Add support to force VSELECT set

[Igor Grinberg]

Hi Otavio,

On 06/16/14 05:24, Otavio Salvador wrote:
> On Sun, Jun 15, 2014 at 11:03 PM, Marek Vasut <marex@denx.de> wrote:
>> On Monday, June 16, 2014 at 03:39:08 AM, Otavio Salvador wrote:
>>> On Sun, Jun 15, 2014 at 10:27 PM, Marek Vasut <marex@denx.de> wrote:
>>>> On Monday, June 16, 2014 at 03:22:22 AM, Otavio Salvador wrote:
>>>>
>>>> [...]
>>>>
>>>>>>> +#ifdef CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT
>>>>>>> +     esdhc_setbits32(&regs->vendorspec, ESDHC_VENDORSPEC_VSELECT);
>>>>>>> +#endif
>>>>>>
>>>>>> Documentation is missing.
>>>>>
>>>>> There is no FSL ESDHC README file so that's why I didn't include it
>>>>> anywhere.
>>>>
>>>> I'm at loss for words here, really...
>>>>
>>>> I think you know what needs to be done (hint: write the documentation),
>>>> right ?
>>>
>>> I won't write the full documentation for it. I am sorry.
>>
>> Undocumented configuration option is not acceptable, period.
> 
> Who accepted the driver in the first version, without Doc?
> 
> I am not in the position to write the full doc.

I think there is a misunderstanding here...
I think Marek does not want to say that you need to write the full
documentation for the driver, but only document the CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT configuration option (what does it
do when you define it and why should one define it).


-- 
Regards,
Igor.

[U-Boot] [PATCH 4/6] mmc: fsl_esdhc: Add support to force VSELECT set

[Otavio Salvador]

On Mon, Jun 16, 2014 at 4:03 AM, Igor Grinberg <grinberg@compulab.co.il> wrote:
> Hi Otavio,
>
> On 06/16/14 05:24, Otavio Salvador wrote:
>> On Sun, Jun 15, 2014 at 11:03 PM, Marek Vasut <marex@denx.de> wrote:
>>> On Monday, June 16, 2014 at 03:39:08 AM, Otavio Salvador wrote:
>>>> On Sun, Jun 15, 2014 at 10:27 PM, Marek Vasut <marex@denx.de> wrote:
>>>>> On Monday, June 16, 2014 at 03:22:22 AM, Otavio Salvador wrote:
>>>>>
>>>>> [...]
>>>>>
>>>>>>>> +#ifdef CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT
>>>>>>>> +     esdhc_setbits32(&regs->vendorspec, ESDHC_VENDORSPEC_VSELECT);
>>>>>>>> +#endif
>>>>>>>
>>>>>>> Documentation is missing.
>>>>>>
>>>>>> There is no FSL ESDHC README file so that's why I didn't include it
>>>>>> anywhere.
>>>>>
>>>>> I'm at loss for words here, really...
>>>>>
>>>>> I think you know what needs to be done (hint: write the documentation),
>>>>> right ?
>>>>
>>>> I won't write the full documentation for it. I am sorry.
>>>
>>> Undocumented configuration option is not acceptable, period.
>>
>> Who accepted the driver in the first version, without Doc?
>>
>> I am not in the position to write the full doc.
>
> I think there is a misunderstanding here...
> I think Marek does not want to say that you need to write the full
> documentation for the driver, but only document the CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT configuration option (what does it
> do when you define it and why should one define it).

Great but where if it does not exist?

should I make a README.fsl-esdhc and include just it?

-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

[U-Boot] [PATCH 4/6] mmc: fsl_esdhc: Add support to force VSELECT set

[Marek Vasut]

On Monday, June 16, 2014 at 01:48:11 PM, Otavio Salvador wrote:
> On Mon, Jun 16, 2014 at 4:03 AM, Igor Grinberg <grinberg@compulab.co.il> 
wrote:
> > Hi Otavio,
> > 
> > On 06/16/14 05:24, Otavio Salvador wrote:
> >> On Sun, Jun 15, 2014 at 11:03 PM, Marek Vasut <marex@denx.de> wrote:
> >>> On Monday, June 16, 2014 at 03:39:08 AM, Otavio Salvador wrote:
> >>>> On Sun, Jun 15, 2014 at 10:27 PM, Marek Vasut <marex@denx.de> wrote:
> >>>>> On Monday, June 16, 2014 at 03:22:22 AM, Otavio Salvador wrote:
> >>>>> 
> >>>>> [...]
> >>>>> 
> >>>>>>>> +#ifdef CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT
> >>>>>>>> +     esdhc_setbits32(&regs->vendorspec,
> >>>>>>>> ESDHC_VENDORSPEC_VSELECT); +#endif
> >>>>>>> 
> >>>>>>> Documentation is missing.
> >>>>>> 
> >>>>>> There is no FSL ESDHC README file so that's why I didn't include it
> >>>>>> anywhere.
> >>>>> 
> >>>>> I'm at loss for words here, really...
> >>>>> 
> >>>>> I think you know what needs to be done (hint: write the
> >>>>> documentation), right ?
> >>>> 
> >>>> I won't write the full documentation for it. I am sorry.
> >>> 
> >>> Undocumented configuration option is not acceptable, period.
> >> 
> >> Who accepted the driver in the first version, without Doc?
> >> 
> >> I am not in the position to write the full doc.
> > 
> > I think there is a misunderstanding here...
> > I think Marek does not want to say that you need to write the full
> > documentation for the driver, but only document the
> > CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT configuration option (what does it do
> > when you define it and why should one define it).
> 
> Great but where if it does not exist?
> 
> should I make a README.fsl-esdhc and include just it?

I briefly looked over the options in the driver, prefixed with hash are the ones 
which are not driver specific:

CONFIG_ESDHC_DETECT_8_BIT_QUIRK
CONFIG_ESDHC_DETECT_QUIRK
CONFIG_FSL_ESDHC_PIN_MUX
CONFIG_FSL_USDHC
#CONFIG_MX53
#CONFIG_OF_LIBFDT
CONFIG_SYS_FSL_ERRATUM_ESDHC111
CONFIG_SYS_FSL_ERRATUM_ESDHC135
CONFIG_SYS_FSL_ERRATUM_ESDHC_A001
CONFIG_SYS_FSL_ESDHC_ADDR
CONFIG_SYS_FSL_ESDHC_USE_PIO
CONFIG_SYS_FSL_MMC_HAS_CAPBLT_VS33
#CONFIG_SYS_MMC_MAX_BLK_COUNT
#CONFIG_SYS_SD_VOLTAGE
#CONFIG_T4240QDS

It looks like completely ad-hoc addition of new and new config options as 
whoever seen fit to me, both from their names and git log of the driver. This 
makes the driver completely unmaintainable. I would like to see them documented 
before any new config options are added, so please instead of fighting the 
mailing list, spend 10 minutes of your time and document them.

Best regards,
Marek Vasut

[U-Boot] [PATCH 4/6] mmc: fsl_esdhc: Add support to force VSELECT set

[Michael Trimarchi]

Hi Marek

Il 17/giu/2014 08:03 "Marek Vasut" <marex@denx.de> ha scritto:
>
> On Monday, June 16, 2014 at 01:48:11 PM, Otavio Salvador wrote:
> > On Mon, Jun 16, 2014 at 4:03 AM, Igor Grinberg <grinberg@compulab.co.il>
> wrote:
> > > Hi Otavio,
> > >
> > > On 06/16/14 05:24, Otavio Salvador wrote:
> > >> On Sun, Jun 15, 2014 at 11:03 PM, Marek Vasut <marex@denx.de> wrote:
> > >>> On Monday, June 16, 2014 at 03:39:08 AM, Otavio Salvador wrote:
> > >>>> On Sun, Jun 15, 2014 at 10:27 PM, Marek Vasut <marex@denx.de>
wrote:
> > >>>>> On Monday, June 16, 2014 at 03:22:22 AM, Otavio Salvador wrote:
> > >>>>>
> > >>>>> [...]
> > >>>>>
> > >>>>>>>> +#ifdef CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT
> > >>>>>>>> +     esdhc_setbits32(&regs->vendorspec,
> > >>>>>>>> ESDHC_VENDORSPEC_VSELECT); +#endif
> > >>>>>>>
> > >>>>>>> Documentation is missing.
> > >>>>>>
> > >>>>>> There is no FSL ESDHC README file so that's why I didn't include
it
> > >>>>>> anywhere.
> > >>>>>
> > >>>>> I'm at loss for words here, really...
> > >>>>>
> > >>>>> I think you know what needs to be done (hint: write the
> > >>>>> documentation), right ?
> > >>>>
> > >>>> I won't write the full documentation for it. I am sorry.
> > >>>
> > >>> Undocumented configuration option is not acceptable, period.
> > >>
> > >> Who accepted the driver in the first version, without Doc?
> > >>
> > >> I am not in the position to write the full doc.
> > >
> > > I think there is a misunderstanding here...
> > > I think Marek does not want to say that you need to write the full
> > > documentation for the driver, but only document the
> > > CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT configuration option (what does it
do
> > > when you define it and why should one define it).
> >
> > Great but where if it does not exist?
> >
> > should I make a README.fsl-esdhc and include just it?
>
> I briefly looked over the options in the driver, prefixed with hash are
the ones
> which are not driver specific:
>
> CONFIG_ESDHC_DETECT_8_BIT_QUIRK
> CONFIG_ESDHC_DETECT_QUIRK
> CONFIG_FSL_ESDHC_PIN_MUX
> CONFIG_FSL_USDHC
> #CONFIG_MX53
> #CONFIG_OF_LIBFDT
> CONFIG_SYS_FSL_ERRATUM_ESDHC111
> CONFIG_SYS_FSL_ERRATUM_ESDHC135
> CONFIG_SYS_FSL_ERRATUM_ESDHC_A001
> CONFIG_SYS_FSL_ESDHC_ADDR
> CONFIG_SYS_FSL_ESDHC_USE_PIO
> CONFIG_SYS_FSL_MMC_HAS_CAPBLT_VS33
> #CONFIG_SYS_MMC_MAX_BLK_COUNT
> #CONFIG_SYS_SD_VOLTAGE
> #CONFIG_T4240QDS
>
> It looks like completely ad-hoc addition of new and new config options as
> whoever seen fit to me, both from their names and git log of the driver.
This
> makes the driver completely unmaintainable. I would like to see them
documented

Do you think that could be a better option to use flags and runtime detect
instead having hundred of define?

Michael

> before any new config options are added, so please instead of fighting the
> mailing list, spend 10 minutes of your time and document them.
>
> Best regards,
> Marek Vasut
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 4/6] mmc: fsl_esdhc: Add support to force VSELECT set

[Marek Vasut]

On Tuesday, June 17, 2014 at 08:06:38 AM, Michael Trimarchi wrote:
> Hi Marek
> 
> Il 17/giu/2014 08:03 "Marek Vasut" <marex@denx.de> ha scritto:
> > On Monday, June 16, 2014 at 01:48:11 PM, Otavio Salvador wrote:
> > > On Mon, Jun 16, 2014 at 4:03 AM, Igor Grinberg
> > > <grinberg@compulab.co.il>
> > 
> > wrote:
> > > > Hi Otavio,
> > > > 
> > > > On 06/16/14 05:24, Otavio Salvador wrote:
> > > >> On Sun, Jun 15, 2014 at 11:03 PM, Marek Vasut <marex@denx.de> wrote:
> > > >>> On Monday, June 16, 2014 at 03:39:08 AM, Otavio Salvador wrote:
> > > >>>> On Sun, Jun 15, 2014 at 10:27 PM, Marek Vasut <marex@denx.de>
> 
> wrote:
> > > >>>>> On Monday, June 16, 2014 at 03:22:22 AM, Otavio Salvador wrote:
> > > >>>>> 
> > > >>>>> [...]
> > > >>>>> 
> > > >>>>>>>> +#ifdef CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT
> > > >>>>>>>> +     esdhc_setbits32(&regs->vendorspec,
> > > >>>>>>>> ESDHC_VENDORSPEC_VSELECT); +#endif
> > > >>>>>>> 
> > > >>>>>>> Documentation is missing.
> > > >>>>>> 
> > > >>>>>> There is no FSL ESDHC README file so that's why I didn't include
> 
> it
> 
> > > >>>>>> anywhere.
> > > >>>>> 
> > > >>>>> I'm at loss for words here, really...
> > > >>>>> 
> > > >>>>> I think you know what needs to be done (hint: write the
> > > >>>>> documentation), right ?
> > > >>>> 
> > > >>>> I won't write the full documentation for it. I am sorry.
> > > >>> 
> > > >>> Undocumented configuration option is not acceptable, period.
> > > >> 
> > > >> Who accepted the driver in the first version, without Doc?
> > > >> 
> > > >> I am not in the position to write the full doc.
> > > > 
> > > > I think there is a misunderstanding here...
> > > > I think Marek does not want to say that you need to write the full
> > > > documentation for the driver, but only document the
> > > > CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT configuration option (what does it
> 
> do
> 
> > > > when you define it and why should one define it).
> > > 
> > > Great but where if it does not exist?
> > > 
> > > should I make a README.fsl-esdhc and include just it?
> > 
> > I briefly looked over the options in the driver, prefixed with hash are
> 
> the ones
> 
> > which are not driver specific:
> > 
> > CONFIG_ESDHC_DETECT_8_BIT_QUIRK
> > CONFIG_ESDHC_DETECT_QUIRK
> > CONFIG_FSL_ESDHC_PIN_MUX
> > CONFIG_FSL_USDHC
> > #CONFIG_MX53
> > #CONFIG_OF_LIBFDT
> > CONFIG_SYS_FSL_ERRATUM_ESDHC111
> > CONFIG_SYS_FSL_ERRATUM_ESDHC135
> > CONFIG_SYS_FSL_ERRATUM_ESDHC_A001
> > CONFIG_SYS_FSL_ESDHC_ADDR
> > CONFIG_SYS_FSL_ESDHC_USE_PIO
> > CONFIG_SYS_FSL_MMC_HAS_CAPBLT_VS33
> > #CONFIG_SYS_MMC_MAX_BLK_COUNT
> > #CONFIG_SYS_SD_VOLTAGE
> > #CONFIG_T4240QDS
> > 
> > It looks like completely ad-hoc addition of new and new config options as
> > whoever seen fit to me, both from their names and git log of the driver.
> 
> This
> 
> > makes the driver completely unmaintainable. I would like to see them
> 
> documented
> 
> Do you think that could be a better option to use flags and runtime detect
> instead having hundred of define?

Eventually, when we transition to DT, this will be indeed needed anyway. But 
before that, we need to understand what those flags do (which we don't because 
previous patches neglected adding proper documentation).

[U-Boot] [PATCH 4/6] mmc: fsl_esdhc: Add support to force VSELECT set

[Michael Trimarchi]

Hi

On Tue, Jun 17, 2014 at 5:49 PM, Marek Vasut <marex@denx.de> wrote:
> On Tuesday, June 17, 2014 at 08:06:38 AM, Michael Trimarchi wrote:
>> Hi Marek
>>
>> Il 17/giu/2014 08:03 "Marek Vasut" <marex@denx.de> ha scritto:
>> > On Monday, June 16, 2014 at 01:48:11 PM, Otavio Salvador wrote:
>> > > On Mon, Jun 16, 2014 at 4:03 AM, Igor Grinberg
>> > > <grinberg@compulab.co.il>
>> >
>> > wrote:
>> > > > Hi Otavio,
>> > > >
>> > > > On 06/16/14 05:24, Otavio Salvador wrote:
>> > > >> On Sun, Jun 15, 2014 at 11:03 PM, Marek Vasut <marex@denx.de> wrote:
>> > > >>> On Monday, June 16, 2014 at 03:39:08 AM, Otavio Salvador wrote:
>> > > >>>> On Sun, Jun 15, 2014 at 10:27 PM, Marek Vasut <marex@denx.de>
>>
>> wrote:
>> > > >>>>> On Monday, June 16, 2014 at 03:22:22 AM, Otavio Salvador wrote:
>> > > >>>>>
>> > > >>>>> [...]
>> > > >>>>>
>> > > >>>>>>>> +#ifdef CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT
>> > > >>>>>>>> +     esdhc_setbits32(&regs->vendorspec,
>> > > >>>>>>>> ESDHC_VENDORSPEC_VSELECT); +#endif
>> > > >>>>>>>
>> > > >>>>>>> Documentation is missing.
>> > > >>>>>>
>> > > >>>>>> There is no FSL ESDHC README file so that's why I didn't include
>>
>> it
>>
>> > > >>>>>> anywhere.
>> > > >>>>>
>> > > >>>>> I'm at loss for words here, really...
>> > > >>>>>
>> > > >>>>> I think you know what needs to be done (hint: write the
>> > > >>>>> documentation), right ?
>> > > >>>>
>> > > >>>> I won't write the full documentation for it. I am sorry.
>> > > >>>
>> > > >>> Undocumented configuration option is not acceptable, period.
>> > > >>
>> > > >> Who accepted the driver in the first version, without Doc?
>> > > >>
>> > > >> I am not in the position to write the full doc.
>> > > >
>> > > > I think there is a misunderstanding here...
>> > > > I think Marek does not want to say that you need to write the full
>> > > > documentation for the driver, but only document the
>> > > > CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT configuration option (what does it
>>
>> do
>>
>> > > > when you define it and why should one define it).
>> > >
>> > > Great but where if it does not exist?
>> > >
>> > > should I make a README.fsl-esdhc and include just it?
>> >
>> > I briefly looked over the options in the driver, prefixed with hash are
>>
>> the ones
>>
>> > which are not driver specific:
>> >
>> > CONFIG_ESDHC_DETECT_8_BIT_QUIRK
>> > CONFIG_ESDHC_DETECT_QUIRK
>> > CONFIG_FSL_ESDHC_PIN_MUX
>> > CONFIG_FSL_USDHC
>> > #CONFIG_MX53
>> > #CONFIG_OF_LIBFDT
>> > CONFIG_SYS_FSL_ERRATUM_ESDHC111
>> > CONFIG_SYS_FSL_ERRATUM_ESDHC135
>> > CONFIG_SYS_FSL_ERRATUM_ESDHC_A001
>> > CONFIG_SYS_FSL_ESDHC_ADDR
>> > CONFIG_SYS_FSL_ESDHC_USE_PIO
>> > CONFIG_SYS_FSL_MMC_HAS_CAPBLT_VS33
>> > #CONFIG_SYS_MMC_MAX_BLK_COUNT
>> > #CONFIG_SYS_SD_VOLTAGE
>> > #CONFIG_T4240QDS
>> >
>> > It looks like completely ad-hoc addition of new and new config options as
>> > whoever seen fit to me, both from their names and git log of the driver.
>>
>> This
>>
>> > makes the driver completely unmaintainable. I would like to see them
>>
>> documented
>>
>> Do you think that could be a better option to use flags and runtime detect
>> instead having hundred of define?
>
> Eventually, when we transition to DT, this will be indeed needed anyway. But
> before that, we need to understand what those flags do (which we don't because
> previous patches neglected adding proper documentation).

I suggest as Stefano said to add this option/flags now in preparation
of DT transition
and start to document flags as you said.

Michael

[U-Boot] [PATCH 4/6] mmc: fsl_esdhc: Add support to force VSELECT set

[Igor Grinberg]

Hi Otavio,

On 06/16/14 14:48, Otavio Salvador wrote:
> On Mon, Jun 16, 2014 at 4:03 AM, Igor Grinberg <grinberg@compulab.co.il> wrote:
>> Hi Otavio,
>>
>> On 06/16/14 05:24, Otavio Salvador wrote:
>>> On Sun, Jun 15, 2014 at 11:03 PM, Marek Vasut <marex@denx.de> wrote:
>>>> On Monday, June 16, 2014 at 03:39:08 AM, Otavio Salvador wrote:
>>>>> On Sun, Jun 15, 2014 at 10:27 PM, Marek Vasut <marex@denx.de> wrote:
>>>>>> On Monday, June 16, 2014 at 03:22:22 AM, Otavio Salvador wrote:
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>>>> +#ifdef CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT
>>>>>>>>> +     esdhc_setbits32(&regs->vendorspec, ESDHC_VENDORSPEC_VSELECT);
>>>>>>>>> +#endif
>>>>>>>>
>>>>>>>> Documentation is missing.
>>>>>>>
>>>>>>> There is no FSL ESDHC README file so that's why I didn't include it
>>>>>>> anywhere.
>>>>>>
>>>>>> I'm at loss for words here, really...
>>>>>>
>>>>>> I think you know what needs to be done (hint: write the documentation),
>>>>>> right ?
>>>>>
>>>>> I won't write the full documentation for it. I am sorry.
>>>>
>>>> Undocumented configuration option is not acceptable, period.
>>>
>>> Who accepted the driver in the first version, without Doc?
>>>
>>> I am not in the position to write the full doc.
>>
>> I think there is a misunderstanding here...
>> I think Marek does not want to say that you need to write the full
>> documentation for the driver, but only document the CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT configuration option (what does it
>> do when you define it and why should one define it).
> 
> Great but where if it does not exist?
> 
> should I make a README.fsl-esdhc and include just it?

Hmmm... May be.
I would make a decision and start something, then send an RFC.
Writing some words should not be hard especially for configuration
options that you introduce yourself, but if you find some stuff
currently hard or time consuming for some reason, a "TODO: ..."
might be an acceptable compromise.


-- 
Regards,
Igor.

[U-Boot] [PATCH 4/6] mmc: fsl_esdhc: Add support to force VSELECT set

[Stefano Babic]

Hi Otavio,

On 16/06/2014 02:46, Otavio Salvador wrote:
> There are board were we cannot do voltage negotiation but want to set
> the VSELECT bit forcely to ensure it to work at 1.8V.
> 
> This commit adds CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT flag for this use.
> 
> Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
> ---
> 
>  drivers/mmc/fsl_esdhc.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
> index c75b38f..b3870e2 100644
> --- a/drivers/mmc/fsl_esdhc.c
> +++ b/drivers/mmc/fsl_esdhc.c
> @@ -517,6 +517,10 @@ static int esdhc_init(struct mmc *mmc)
>  	/* Set timout to the maximum value */
>  	esdhc_clrsetbits32(&regs->sysctl, SYSCTL_TIMEOUT_MASK, 14 << 16);
>  
> +#ifdef CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT
> +	esdhc_setbits32(&regs->vendorspec, ESDHC_VENDORSPEC_VSELECT);
> +#endif

Instead of adding a new compiler switch that should be documented (I
have already read Marek's comments), what do you think to extend struct
fsl_esdhc_cfg, putting for exmaple an "options" field with this kind of
specialization ?

I see also a CONFIG_SYS_FSL_ESDHC_USE_PIO that slipped into mainline
(you are both right : Otavio is not the first to add such kind of
configuration switches that are still undocumented, but according to
rules should be documented as Marek said).

I would suggest to get rid as much as possible with these CONFIG_
switches. If we have some specialization for this driver before calling
esdhc_init (better: fsl_esdhc_initialize), they are self explaining and
we do not need further documentation. What do you think ?

Best regards,
Stefano


-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH 4/6] mmc: fsl_esdhc: Add support to force VSELECT set

[Otavio Salvador]

On Tue, Jun 17, 2014 at 12:11 PM, Stefano Babic <sbabic@denx.de> wrote:
> On 16/06/2014 02:46, Otavio Salvador wrote:
>> There are board were we cannot do voltage negotiation but want to set
>> the VSELECT bit forcely to ensure it to work at 1.8V.
>>
>> This commit adds CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT flag for this use.
>>
>> Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
>> ---
>>
>>  drivers/mmc/fsl_esdhc.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
>> index c75b38f..b3870e2 100644
>> --- a/drivers/mmc/fsl_esdhc.c
>> +++ b/drivers/mmc/fsl_esdhc.c
>> @@ -517,6 +517,10 @@ static int esdhc_init(struct mmc *mmc)
>>       /* Set timout to the maximum value */
>>       esdhc_clrsetbits32(&regs->sysctl, SYSCTL_TIMEOUT_MASK, 14 << 16);
>>
>> +#ifdef CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT
>> +     esdhc_setbits32(&regs->vendorspec, ESDHC_VENDORSPEC_VSELECT);
>> +#endif
>
> Instead of adding a new compiler switch that should be documented (I
> have already read Marek's comments), what do you think to extend struct
> fsl_esdhc_cfg, putting for exmaple an "options" field with this kind of
> specialization ?

I will try to cook something in this direction.

-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

[U-Boot] [PATCH 4/6] mmc: fsl_esdhc: Add support to force VSELECT set

[Michael Trimarchi]

Hi Stefano

On Tue, Jun 17, 2014 at 5:12 PM, Otavio Salvador
<otavio@ossystems.com.br> wrote:
> On Tue, Jun 17, 2014 at 12:11 PM, Stefano Babic <sbabic@denx.de> wrote:
>> On 16/06/2014 02:46, Otavio Salvador wrote:
>>> There are board were we cannot do voltage negotiation but want to set
>>> the VSELECT bit forcely to ensure it to work at 1.8V.
>>>
>>> This commit adds CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT flag for this use.
>>>
>>> Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
>>> ---
>>>
>>>  drivers/mmc/fsl_esdhc.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
>>> index c75b38f..b3870e2 100644
>>> --- a/drivers/mmc/fsl_esdhc.c
>>> +++ b/drivers/mmc/fsl_esdhc.c
>>> @@ -517,6 +517,10 @@ static int esdhc_init(struct mmc *mmc)
>>>       /* Set timout to the maximum value */
>>>       esdhc_clrsetbits32(&regs->sysctl, SYSCTL_TIMEOUT_MASK, 14 << 16);
>>>
>>> +#ifdef CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT
>>> +     esdhc_setbits32(&regs->vendorspec, ESDHC_VENDORSPEC_VSELECT);
>>> +#endif
>>
>> Instead of adding a new compiler switch that should be documented (I
>> have already read Marek's comments), what do you think to extend struct
>> fsl_esdhc_cfg, putting for exmaple an "options" field with this kind of
>> specialization ?
>
> I will try to cook something in this direction.
>

Well I already said some emails ago ;).

Michael

> --
> Otavio Salvador                             O.S. Systems
> http://www.ossystems.com.br        http://code.ossystems.com.br
> Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot



-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

[U-Boot] [PATCH 4/6] mmc: fsl_esdhc: Add support to force VSELECT set

[Stefano Babic]

Hi Michael,

On 17/06/2014 17:14, Michael Trimarchi wrote:

>>> Instead of adding a new compiler switch that should be documented (I
>>> have already read Marek's comments), what do you think to extend struct
>>> fsl_esdhc_cfg, putting for exmaple an "options" field with this kind of
>>> specialization ?
>>
>> I will try to cook something in this direction.
>>
> 
> Well I already said some emails ago ;).

Nice I am not alone to think it in this way :-). Sorry, I have not read
your answer, I was dropped from the CC list and I missed that you have
already said the same.

Best regards,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH 5/6] Print ramdisk load address if it is in use

[Otavio Salvador]

When debugging initramfs failures it is quite useful to known where it
is being loaded from.

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
---

 common/image.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/common/image.c b/common/image.c
index fa4864d..84e115f 100644
--- a/common/image.c
+++ b/common/image.c
@@ -1050,12 +1050,13 @@ int boot_ramdisk_high(struct lmb *lmb, ulong rd_data, ulong rd_len,
 #endif
 			puts("OK\n");
 		}
+
+		printf("   ramdisk load start = 0x%08lx, ramdisk load end = 0x%08lx\n",
+			   *initrd_start, *initrd_end);
 	} else {
 		*initrd_start = 0;
 		*initrd_end = 0;
 	}
-	debug("   ramdisk load start = 0x%08lx, ramdisk load end = 0x%08lx\n",
-			*initrd_start, *initrd_end);
 
 	return 0;
 
-- 
2.0.0

[U-Boot] [PATCH 5/6] Print ramdisk load address if it is in use

[Wolfgang Denk]

Dear Otavio Salvador,

In message <1402879613-21362-5-git-send-email-otavio@ossystems.com.br> you wrote:
> When debugging initramfs failures it is quite useful to known where it
> is being loaded from.

Agreed.

> +
> +		printf("   ramdisk load start = 0x%08lx, ramdisk load end = 0x%08lx\n",
> +			   *initrd_start, *initrd_end);
>  	} else {
>  		*initrd_start = 0;
>  		*initrd_end = 0;
>  	}
> -	debug("   ramdisk load start = 0x%08lx, ramdisk load end = 0x%08lx\n",
> -			*initrd_start, *initrd_end);

As you write yourself, this is a debug feature.  There is no need to
pollute normal output with such information.  It just makes booting
slower.  So debug() is appropriate here.

And I don't see why you would NOT want to have the information in
the else case.

I don't think this change is a good idea.  It should not be applied.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The past is not dead. In fact, it's not even past.  - William Faulkner

[U-Boot] [PATCH 5/6] Print ramdisk load address if it is in use

[Otavio Salvador]

On Mon, Jun 16, 2014 at 1:38 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Otavio Salvador,
>
> In message <1402879613-21362-5-git-send-email-otavio@ossystems.com.br> you wrote:
>> When debugging initramfs failures it is quite useful to known where it
>> is being loaded from.
>
> Agreed.
>
>> +
>> +             printf("   ramdisk load start = 0x%08lx, ramdisk load end = 0x%08lx\n",
>> +                        *initrd_start, *initrd_end);
>>       } else {
>>               *initrd_start = 0;
>>               *initrd_end = 0;
>>       }
>> -     debug("   ramdisk load start = 0x%08lx, ramdisk load end = 0x%08lx\n",
>> -                     *initrd_start, *initrd_end);
>
> As you write yourself, this is a debug feature.  There is no need to
> pollute normal output with such information.  It just makes booting
> slower.  So debug() is appropriate here.
>
> And I don't see why you would NOT want to have the information in
> the else case.
>
> I don't think this change is a good idea.  It should not be applied.

Ok, no problem with me.

I will drop it for v2.

-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

[U-Boot] [PATCH 6/6] warp: Add initial WaRP Board support

[Otavio Salvador]

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
---

 board/warp/Makefile    |   8 ++
 board/warp/warp.c      | 111 +++++++++++++++++++++++++++
 boards.cfg             |   1 +
 include/configs/warp.h | 198 +++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 318 insertions(+)
 create mode 100644 board/warp/Makefile
 create mode 100644 board/warp/warp.c
 create mode 100644 include/configs/warp.h

diff --git a/board/warp/Makefile b/board/warp/Makefile
new file mode 100644
index 0000000..c555f87
--- /dev/null
+++ b/board/warp/Makefile
@@ -0,0 +1,8 @@
+# Copyright (C) 2014 O.S. Systems Software LTDA.
+# Copyright (C) 2014 Kynetics LLC.
+# Copyright (C) 2014 Revolution Robotics, Inc.
+#
+# SPDX-License-Identifier:	GPL-2.0+
+#
+
+obj-y  := warp.o
diff --git a/board/warp/warp.c b/board/warp/warp.c
new file mode 100644
index 0000000..f08707c
--- /dev/null
+++ b/board/warp/warp.c
@@ -0,0 +1,111 @@
+/*
+ * Copyright (C) 2014 O.S. Systems Software LTDA.
+ * Copyright (C) 2014 Kynetics LLC.
+ * Copyright (C) 2014 Revolution Robotics, Inc.
+ *
+ * Author: Otavio Salvador <otavio@ossystems.com.br>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <asm/arch/clock.h>
+#include <asm/arch/iomux.h>
+#include <asm/arch/imx-regs.h>
+#include <asm/arch/mx6-pins.h>
+#include <asm/arch/sys_proto.h>
+#include <asm/gpio.h>
+#include <asm/imx-common/iomux-v3.h>
+#include <asm/io.h>
+#include <linux/sizes.h>
+#include <common.h>
+#include <fsl_esdhc.h>
+#include <mmc.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+#define UART_PAD_CTRL  (PAD_CTL_PUS_100K_UP |			\
+	PAD_CTL_SPEED_MED | PAD_CTL_DSE_40ohm |			\
+	PAD_CTL_SRE_FAST  | PAD_CTL_HYS)
+
+#define USDHC_PAD_CTRL (PAD_CTL_PUS_22K_UP |			\
+	PAD_CTL_SPEED_LOW | PAD_CTL_DSE_80ohm |			\
+	PAD_CTL_SRE_FAST  | PAD_CTL_HYS |			\
+	PAD_CTL_LVE)
+
+int dram_init(void)
+{
+	gd->ram_size = get_ram_size((void *)PHYS_SDRAM, PHYS_SDRAM_SIZE);
+
+	return 0;
+}
+
+static void setup_iomux_uart(void)
+{
+	static iomux_v3_cfg_t const uart1_pads[] = {
+		MX6_PAD_UART1_TXD__UART1_TXD | MUX_PAD_CTRL(UART_PAD_CTRL),
+		MX6_PAD_UART1_RXD__UART1_RXD | MUX_PAD_CTRL(UART_PAD_CTRL),
+	};
+
+	imx_iomux_v3_setup_multiple_pads(uart1_pads, ARRAY_SIZE(uart1_pads));
+}
+
+static struct fsl_esdhc_cfg usdhc_cfg[1] = {
+	{USDHC2_BASE_ADDR},
+};
+
+int board_mmc_getcd(struct mmc *mmc)
+{
+	return 1;	/* Assume boot SD always present */
+}
+
+int board_mmc_init(bd_t *bis)
+{
+	static iomux_v3_cfg_t const usdhc2_pads[] = {
+		MX6_PAD_SD2_CLK__USDHC2_CLK | MUX_PAD_CTRL(USDHC_PAD_CTRL),
+		MX6_PAD_SD2_CMD__USDHC2_CMD | MUX_PAD_CTRL(USDHC_PAD_CTRL),
+		MX6_PAD_SD2_RST__USDHC2_RST | MUX_PAD_CTRL(USDHC_PAD_CTRL),
+		MX6_PAD_SD2_DAT0__USDHC2_DAT0 | MUX_PAD_CTRL(USDHC_PAD_CTRL),
+		MX6_PAD_SD2_DAT1__USDHC2_DAT1 | MUX_PAD_CTRL(USDHC_PAD_CTRL),
+		MX6_PAD_SD2_DAT2__USDHC2_DAT2 | MUX_PAD_CTRL(USDHC_PAD_CTRL),
+		MX6_PAD_SD2_DAT3__USDHC2_DAT3 | MUX_PAD_CTRL(USDHC_PAD_CTRL),
+		MX6_PAD_SD2_DAT4__USDHC2_DAT4 | MUX_PAD_CTRL(USDHC_PAD_CTRL),
+		MX6_PAD_SD2_DAT5__USDHC2_DAT5 | MUX_PAD_CTRL(USDHC_PAD_CTRL),
+		MX6_PAD_SD2_DAT6__USDHC2_DAT6 | MUX_PAD_CTRL(USDHC_PAD_CTRL),
+		MX6_PAD_SD2_DAT7__USDHC2_DAT7 | MUX_PAD_CTRL(USDHC_PAD_CTRL),
+	};
+
+	imx_iomux_v3_setup_multiple_pads(usdhc2_pads, ARRAY_SIZE(usdhc2_pads));
+
+	usdhc_cfg[0].sdhc_clk = mxc_get_clock(MXC_ESDHC2_CLK);
+	return fsl_esdhc_initialize(bis, &usdhc_cfg[0]);
+}
+
+int board_early_init_f(void)
+{
+	setup_iomux_uart();
+	return 0;
+}
+
+int board_init(void)
+{
+	/* address of boot parameters */
+	gd->bd->bi_boot_params = PHYS_SDRAM + 0x100;
+
+	return 0;
+}
+
+int board_late_init(void)
+{
+#ifdef CONFIG_HW_WATCHDOG
+	hw_watchdog_init();
+#endif
+
+	return 0;
+}
+
+int checkboard(void)
+{
+	puts("Board: WaRP Board\n");
+
+	return 0;
+}
diff --git a/boards.cfg b/boards.cfg
index fd4324d..435edf3 100644
--- a/boards.cfg
+++ b/boards.cfg
@@ -311,6 +311,7 @@ Active  arm         armv7          mx6         -               udoo
 Active  arm         armv7          mx6         -               wandboard           wandboard_dl                          wandboard:IMX_CONFIG=board/boundary/nitrogen6x/nitrogen6dl.cfg,MX6DL,DDR_MB=1024                                                  Fabio Estevam <fabio.estevam@freescale.com>
 Active  arm         armv7          mx6         -               wandboard           wandboard_quad                        wandboard:IMX_CONFIG=board/boundary/nitrogen6x/nitrogen6q2g.cfg,MX6Q,DDR_MB=2048                                                  Fabio Estevam <fabio.estevam@freescale.com>
 Active  arm         armv7          mx6         -               wandboard           wandboard_solo                        wandboard:IMX_CONFIG=board/boundary/nitrogen6x/nitrogen6s.cfg,MX6S,DDR_MB=512                                                     Fabio Estevam <fabio.estevam@freescale.com>
+Active  arm         armv7          mx6         -               warp                warp                                  warp:IMX_CONFIG=board/freescale/mx6slevk/imximage.cfg,MX6SL                                                                       Otavio Salvador <otavio@ossystems.com.br>
 Active  arm         armv7          mx6         barco           titanium            titanium                              titanium:IMX_CONFIG=board/barco/titanium/imximage.cfg                                                                             Stefan Roese <sr@denx.de>
 Active  arm         armv7          mx6         boundary        nitrogen6x          mx6qsabrelite                         nitrogen6x:IMX_CONFIG=board/boundary/nitrogen6x/nitrogen6q.cfg,MX6Q,DDR_MB=1024,SABRELITE                                         Eric Nelson <eric.nelson@boundarydevices.com>
 Active  arm         armv7          mx6         boundary        nitrogen6x          nitrogen6dl                           nitrogen6x:IMX_CONFIG=board/boundary/nitrogen6x/nitrogen6dl.cfg,MX6DL,DDR_MB=1024                                                 Eric Nelson <eric.nelson@boundarydevices.com>
diff --git a/include/configs/warp.h b/include/configs/warp.h
new file mode 100644
index 0000000..82036e4
--- /dev/null
+++ b/include/configs/warp.h
@@ -0,0 +1,198 @@
+/*
+ * Copyright (C) 2014 O.S. Systems Software LTDA.
+ * Copyright (C) 2014 Kynetics LLC.
+ * Copyright (C) 2014 Revolution Robotics, Inc.
+ *
+ * Author: Otavio Salvador <otavio@ossystems.com.br>
+ *
+ * Configuration settings for the WaRP Board
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#ifndef __CONFIG_H
+#define __CONFIG_H
+
+#include <asm/arch/imx-regs.h>
+#include <linux/sizes.h>
+#include "mx6_common.h"
+
+#define CONFIG_MX6
+#define CONFIG_DISPLAY_CPUINFO
+#define CONFIG_DISPLAY_BOARDINFO
+#define CONFIG_SYS_GENERIC_BOARD
+
+#define CONFIG_CMDLINE_TAG
+#define CONFIG_SETUP_MEMORY_TAGS
+#define CONFIG_INITRD_TAG
+#define CONFIG_REVISION_TAG
+
+/* Size of malloc() pool */
+#define CONFIG_SYS_MALLOC_LEN		(3 * SZ_1M)
+
+#define CONFIG_BOARD_EARLY_INIT_F
+#define CONFIG_BOARD_LATE_INIT
+#define CONFIG_MXC_GPIO
+
+#define CONFIG_MXC_UART
+#define CONFIG_MXC_UART_BASE		UART1_IPS_BASE_ADDR
+
+/* MMC Configs */
+#define CONFIG_FSL_ESDHC
+#define CONFIG_FSL_USDHC
+#define CONFIG_SYS_FSL_ESDHC_ADDR	0
+#define CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT
+
+#define CONFIG_MMC
+#define CONFIG_CMD_MMC
+#define CONFIG_GENERIC_MMC
+#define CONFIG_CMD_FAT
+#define CONFIG_DOS_PARTITION
+
+/* allow to overwrite serial and ethaddr */
+#define CONFIG_ENV_OVERWRITE
+#define CONFIG_CONS_INDEX		1
+#define CONFIG_BAUDRATE			115200
+
+/* FLASH and environment organization */
+#define CONFIG_SYS_NO_FLASH
+
+/* Command definition */
+#include <config_cmd_default.h>
+#undef CONFIG_CMD_NET
+#undef CONFIG_CMD_NFS
+
+#define CONFIG_BOOTDELAY		3
+
+#define CONFIG_LOADADDR			0x82000000
+#define CONFIG_SYS_TEXT_BASE		0x87800000
+
+/* Miscellaneous configurable options */
+#define CONFIG_SYS_LONGHELP
+#define CONFIG_SYS_HUSH_PARSER
+#define CONFIG_AUTO_COMPLETE
+#define CONFIG_SYS_CBSIZE		256
+
+/* Watchdog */
+#define CONFIG_HW_WATCHDOG
+#define CONFIG_IMX_WATCHDOG
+#define CONFIG_WATCHDOG_TIMEOUT_MSECS 30000 /* 30s */
+
+/* Print Buffer Size */
+#define CONFIG_SYS_PBSIZE (CONFIG_SYS_CBSIZE + sizeof(CONFIG_SYS_PROMPT) + 16)
+#define CONFIG_SYS_MAXARGS		16
+#define CONFIG_SYS_BARGSIZE CONFIG_SYS_CBSIZE
+
+#define CONFIG_SYS_MEMTEST_START	0x80000000
+#define CONFIG_SYS_MEMTEST_END		(CONFIG_SYS_MEMTEST_START + SZ_256M)
+
+#define CONFIG_SYS_LOAD_ADDR		CONFIG_LOADADDR
+
+#define CONFIG_CMDLINE_EDITING
+#define CONFIG_STACKSIZE		SZ_128K
+
+/* Physical Memory Map */
+#define CONFIG_NR_DRAM_BANKS		1
+#define PHYS_SDRAM			MMDC0_ARB_BASE_ADDR
+#define PHYS_SDRAM_SIZE			SZ_512M
+
+#define CONFIG_SYS_SDRAM_BASE		PHYS_SDRAM
+#define CONFIG_SYS_INIT_RAM_ADDR	IRAM_BASE_ADDR
+#define CONFIG_SYS_INIT_RAM_SIZE	IRAM_SIZE
+
+#define CONFIG_SYS_INIT_SP_OFFSET \
+	(CONFIG_SYS_INIT_RAM_SIZE - GENERATED_GBL_DATA_SIZE)
+#define CONFIG_SYS_INIT_SP_ADDR \
+	(CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_SP_OFFSET)
+
+#define CONFIG_ENV_OFFSET		(6 * SZ_64K)
+#define CONFIG_ENV_SIZE			SZ_8K
+#define CONFIG_ENV_IS_IN_MMC
+#define CONFIG_SYS_MMC_ENV_DEV		0
+
+/* VDD voltage 1.65 - 1.95 */
+#define CONFIG_SYS_SD_VOLTAGE		0x00000080
+
+#define CONFIG_OF_LIBFDT
+#define CONFIG_CMD_BOOTZ
+
+#ifndef CONFIG_SYS_DCACHE_OFF
+#define CONFIG_CMD_CACHE
+#endif
+
+#define CONFIG_EXTRA_ENV_SETTINGS \
+	"script=boot.scr\0" \
+	"image=zImage\0" \
+	"console=ttymxc0\0" \
+	"fdt_high=0xffffffff\0" \
+	"initrd_high=0xffffffff\0" \
+	"fdt_file=imx6sl-warp.dtb\0" \
+	"fdt_addr=0x88000000\0" \
+	"initrd_addr=0x83800000\0" \
+	"boot_fdt=try\0" \
+	"ip_dyn=yes\0" \
+	"mmcdev=0\0" \
+	"mmcpart=1\0" \
+	"mmcroot=/dev/mmcblk0p2 rootwait rw\0" \
+	"mmcargs=setenv bootargs console=${console},${baudrate} " \
+		"root=${mmcroot}\0" \
+	"loadbootscript=" \
+		"fatload mmc ${mmcdev}:${mmcpart} ${loadaddr} ${script};\0" \
+	"bootscript=echo Running bootscript from mmc ...; " \
+		"source\0" \
+	"loadimage=fatload mmc ${mmcdev}:${mmcpart} ${loadaddr} ${image}\0" \
+	"loadfdt=fatload mmc ${mmcdev}:${mmcpart} ${fdt_addr} ${fdt_file}\0" \
+	"mmcboot=echo Booting from mmc ...; " \
+		"run mmcargs; " \
+		"if test ${boot_fdt} = yes || test ${boot_fdt} = try; then " \
+			"if run loadfdt; then " \
+				"bootz ${loadaddr} - ${fdt_addr}; " \
+			"else " \
+				"if test ${boot_fdt} = try; then " \
+					"bootz; " \
+				"else " \
+					"echo WARN: Cannot load the DT; " \
+				"fi; " \
+			"fi; " \
+		"else " \
+			"bootz; " \
+		"fi;\0" \
+	"netargs=setenv bootargs console=${console},${baudrate} " \
+		"root=/dev/nfs " \
+	"ip=dhcp nfsroot=${serverip}:${nfsroot},v3,tcp\0" \
+		"netboot=echo Booting from net ...; " \
+		"run netargs; " \
+		"if test ${ip_dyn} = yes; then " \
+			"setenv get_cmd dhcp; " \
+		"else " \
+			"setenv get_cmd tftp; " \
+		"fi; " \
+		"${get_cmd} ${image}; " \
+		"if test ${boot_fdt} = yes || test ${boot_fdt} = try; then " \
+			"if ${get_cmd} ${fdt_addr} ${fdt_file}; then " \
+				"bootz ${loadaddr} - ${fdt_addr}; " \
+			"else " \
+				"if test ${boot_fdt} = try; then " \
+					"bootz; " \
+				"else " \
+					"echo WARN: Cannot load the DT; " \
+				"fi; " \
+			"fi; " \
+		"else " \
+			"bootz; " \
+		"fi;\0"
+
+#define CONFIG_BOOTCOMMAND \
+	   "mmc dev ${mmcdev};" \
+	   "mmc dev ${mmcdev}; if mmc rescan; then " \
+		   "if run loadbootscript; then " \
+			   "run bootscript; " \
+		   "else " \
+			   "if run loadimage; then " \
+				   "run mmcboot; " \
+			   "else run netboot; " \
+			   "fi; " \
+		   "fi; " \
+	   "else run netboot; fi"
+
+#endif				/* __CONFIG_H */
-- 
2.0.0

[U-Boot] [PATCH 6/6] warp: Add initial WaRP Board support

[Marek Vasut]

On Monday, June 16, 2014 at 02:46:53 AM, Otavio Salvador wrote:

Such commit message, I can't stop reading ...

> Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
> ---
> 
>  board/warp/Makefile    |   8 ++
>  board/warp/warp.c      | 111 +++++++++++++++++++++++++++
>  boards.cfg             |   1 +
>  include/configs/warp.h | 198

[...]

btw. this thing contains like gazilion-th copy of the same env ... that's 
starting to annoy me.

Best regards,
Marek Vasut

[U-Boot] [PATCH 6/6] warp: Add initial WaRP Board support

[Fabio Estevam]

Hi Otavio,

On Sun, Jun 15, 2014 at 9:46 PM, Otavio Salvador
<otavio@ossystems.com.br> wrote:
> Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
> ---
>
>  board/warp/Makefile    |   8 ++
>  board/warp/warp.c      | 111 +++++++++++++++++++++++++++
>  boards.cfg             |   1 +
>  include/configs/warp.h | 198 +++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 318 insertions(+)
>  create mode 100644 board/warp/Makefile
>  create mode 100644 board/warp/warp.c
>  create mode 100644 include/configs/warp.h

Do you have to submit v2 of this series?

[U-Boot] [PATCH 6/6] warp: Add initial WaRP Board support

[Fabio Estevam]

On Mon, Aug 18, 2014 at 2:56 PM, Fabio Estevam <festevam@gmail.com> wrote:
> Hi Otavio,
>
> On Sun, Jun 15, 2014 at 9:46 PM, Otavio Salvador
> <otavio@ossystems.com.br> wrote:
>> Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
>> ---
>>
>>  board/warp/Makefile    |   8 ++
>>  board/warp/warp.c      | 111 +++++++++++++++++++++++++++
>>  boards.cfg             |   1 +
>>  include/configs/warp.h | 198 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 318 insertions(+)
>>  create mode 100644 board/warp/Makefile
>>  create mode 100644 board/warp/warp.c
>>  create mode 100644 include/configs/warp.h
>
> Do you have to submit v2 of this series?

I meant "Do you have plans to submit v2 of this series?"

[U-Boot] [PATCH 6/6] warp: Add initial WaRP Board support

[Otavio Salvador]

On Mon, Aug 18, 2014 at 2:57 PM, Fabio Estevam <festevam@gmail.com> wrote:
> On Mon, Aug 18, 2014 at 2:56 PM, Fabio Estevam <festevam@gmail.com> wrote:
>> Hi Otavio,
>>
>> On Sun, Jun 15, 2014 at 9:46 PM, Otavio Salvador
>> <otavio@ossystems.com.br> wrote:
>>> Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
>>> ---
>>>
>>>  board/warp/Makefile    |   8 ++
>>>  board/warp/warp.c      | 111 +++++++++++++++++++++++++++
>>>  boards.cfg             |   1 +
>>>  include/configs/warp.h | 198 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>  4 files changed, 318 insertions(+)
>>>  create mode 100644 board/warp/Makefile
>>>  create mode 100644 board/warp/warp.c
>>>  create mode 100644 include/configs/warp.h
>>
>> Do you have to submit v2 of this series?
>
> I meant "Do you have plans to submit v2 of this series?"

Yes; working on it.

-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

[U-Boot] [PATCH 1/6] usb: ehci: mx6: Add support for i.MX6SL

[Marek Vasut]

On Monday, June 16, 2014 at 02:46:48 AM, Otavio Salvador wrote:
> The i.MX6SL has a different base address for the controller. This
> patch adapts the driver to support the different base address for this
> case.
> 
> Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
> ---
> 
>  drivers/usb/host/ehci-mx6.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
> index c0a557b..5ba1c5e 100644
> --- a/drivers/usb/host/ehci-mx6.c
> +++ b/drivers/usb/host/ehci-mx6.c
> @@ -53,6 +53,12 @@
>  #define UCMD_RUN_STOP           (1 << 0) /* controller run/stop */
>  #define UCMD_RESET		(1 << 1) /* controller reset */
> 
> +#ifdef CONFIG_MX6SL
> +#define USB_BASE_ADDR		USBO2H_USB_BASE_ADDR
> +#else
> +#define USB_BASE_ADDR		USBOH3_USB_BASE_ADDR
> +#endif

Can the CPU type be detected at runtime so we don't need this ugly ifdef ?

Best regards,
Marek Vasut

[U-Boot] [PATCH 1/6] usb: ehci: mx6: Add support for i.MX6SL

[Otavio Salvador]

On Sun, Jun 15, 2014 at 9:49 PM, Marek Vasut <marex@denx.de> wrote:
> On Monday, June 16, 2014 at 02:46:48 AM, Otavio Salvador wrote:
>> The i.MX6SL has a different base address for the controller. This
>> patch adapts the driver to support the different base address for this
>> case.
>>
>> Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
...
> Can the CPU type be detected at runtime so we don't need this ugly ifdef ?

The SoloLite is not pin-compatible so it is unable to be used in same binary.

-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

[U-Boot] [PATCH 1/6] usb: ehci: mx6: Add support for i.MX6SL

[Marek Vasut]

On Monday, June 16, 2014 at 03:11:03 AM, Otavio Salvador wrote:
> On Sun, Jun 15, 2014 at 9:49 PM, Marek Vasut <marex@denx.de> wrote:
> > On Monday, June 16, 2014 at 02:46:48 AM, Otavio Salvador wrote:
> >> The i.MX6SL has a different base address for the controller. This
> >> patch adapts the driver to support the different base address for this
> >> case.
> >> 
> >> Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
> 
> ...
> 
> > Can the CPU type be detected at runtime so we don't need this ugly ifdef
> > ?
> 
> The SoloLite is not pin-compatible so it is unable to be used in same
> binary.

You didn't answer my question ;)

Best regards,
Marek Vasut

[U-Boot] [PATCH 1/6] usb: ehci: mx6: Add support for i.MX6SL

[Igor Grinberg]

Hi Otavio,

On 06/16/14 03:46, Otavio Salvador wrote:
> The i.MX6SL has a different base address for the controller. This
> patch adapts the driver to support the different base address for this
> case.
> 
> Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
> ---
> 
>  drivers/usb/host/ehci-mx6.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
> index c0a557b..5ba1c5e 100644
> --- a/drivers/usb/host/ehci-mx6.c
> +++ b/drivers/usb/host/ehci-mx6.c
> @@ -53,6 +53,12 @@
>  #define UCMD_RUN_STOP           (1 << 0) /* controller run/stop */
>  #define UCMD_RESET		(1 << 1) /* controller reset */
>  
> +#ifdef CONFIG_MX6SL
> +#define USB_BASE_ADDR		USBO2H_USB_BASE_ADDR
> +#else
> +#define USB_BASE_ADDR		USBOH3_USB_BASE_ADDR
> +#endif
> +

Hmmm... Can't this be done dynamically?
I mean... you can check the SoC type in runtime, right?
And then substitute the correct address in a variable or so...
I would really prefer such kind of things done in runtime.

>  static const unsigned phy_bases[] = {
>  	USB_PHY0_BASE_ADDR,
>  	USB_PHY1_BASE_ADDR,
> @@ -174,7 +180,7 @@ struct usbnc_regs {
>  
>  static void usb_oc_config(int index)
>  {
> -	struct usbnc_regs *usbnc = (struct usbnc_regs *)(USBOH3_USB_BASE_ADDR +
> +	struct usbnc_regs *usbnc = (struct usbnc_regs *)(USB_BASE_ADDR +
>  			USB_OTHERREGS_OFFSET);
>  	void __iomem *ctrl = (void __iomem *)(&usbnc->ctrl[index]);
>  	u32 val;
> @@ -207,7 +213,7 @@ int ehci_hcd_init(int index, enum usb_init_type init,
>  		struct ehci_hccr **hccr, struct ehci_hcor **hcor)
>  {
>  	enum usb_init_type type;
> -	struct usb_ehci *ehci = (struct usb_ehci *)(USBOH3_USB_BASE_ADDR +
> +	struct usb_ehci *ehci = (struct usb_ehci *)(USB_BASE_ADDR +
>  		(0x200 * index));
>  
>  	if (index > 3)
> 

-- 
Regards,
Igor.

[U-Boot] [PATCH 1/6] usb: ehci: mx6: Add support for i.MX6SL

[Otavio Salvador]

On Mon, Jun 16, 2014 at 4:05 AM, Igor Grinberg <grinberg@compulab.co.il> wrote:
> Hi Otavio,
>
> On 06/16/14 03:46, Otavio Salvador wrote:
>> The i.MX6SL has a different base address for the controller. This
>> patch adapts the driver to support the different base address for this
>> case.
>>
>> Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
>> ---
>>
>>  drivers/usb/host/ehci-mx6.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
>> index c0a557b..5ba1c5e 100644
>> --- a/drivers/usb/host/ehci-mx6.c
>> +++ b/drivers/usb/host/ehci-mx6.c
>> @@ -53,6 +53,12 @@
>>  #define UCMD_RUN_STOP           (1 << 0) /* controller run/stop */
>>  #define UCMD_RESET           (1 << 1) /* controller reset */
>>
>> +#ifdef CONFIG_MX6SL
>> +#define USB_BASE_ADDR                USBO2H_USB_BASE_ADDR
>> +#else
>> +#define USB_BASE_ADDR                USBOH3_USB_BASE_ADDR
>> +#endif
>> +
>
> Hmmm... Can't this be done dynamically?
> I mean... you can check the SoC type in runtime, right?
> And then substitute the correct address in a variable or so...
> I would really prefer such kind of things done in runtime.

I will check about changing it for v2.

-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

[U-Boot] [PATCH 1/6] usb: ehci: mx6: Add support for i.MX6SL

[Stefano Babic]

Hi Otavio,

On 16/06/2014 02:46, Otavio Salvador wrote:
> The i.MX6SL has a different base address for the controller. This
> patch adapts the driver to support the different base address for this
> case.
> 
> Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
> ---
> 
>  drivers/usb/host/ehci-mx6.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
> index c0a557b..5ba1c5e 100644
> --- a/drivers/usb/host/ehci-mx6.c
> +++ b/drivers/usb/host/ehci-mx6.c
> @@ -53,6 +53,12 @@
>  #define UCMD_RUN_STOP           (1 << 0) /* controller run/stop */
>  #define UCMD_RESET		(1 << 1) /* controller reset */
>  
> +#ifdef CONFIG_MX6SL
> +#define USB_BASE_ADDR		USBO2H_USB_BASE_ADDR
> +#else
> +#define USB_BASE_ADDR		USBOH3_USB_BASE_ADDR
> +#endif
> +

What about using the is_cpu_type() function, recently added together
with SPL support ? I think we have reached, thank to Tim's patches, the
goal to have a single image for different Freescale's mx6 variations. It
is pity to go back and decide at compile time which SOC is running.

>  static const unsigned phy_bases[] = {
>  	USB_PHY0_BASE_ADDR,
>  	USB_PHY1_BASE_ADDR,
> @@ -174,7 +180,7 @@ struct usbnc_regs {
>  
>  static void usb_oc_config(int index)
>  {
> -	struct usbnc_regs *usbnc = (struct usbnc_regs *)(USBOH3_USB_BASE_ADDR +
> +	struct usbnc_regs *usbnc = (struct usbnc_regs *)(USB_BASE_ADDR +
>  			USB_OTHERREGS_OFFSET);
>  	void __iomem *ctrl = (void __iomem *)(&usbnc->ctrl[index]);
>  	u32 val;
> @@ -207,7 +213,7 @@ int ehci_hcd_init(int index, enum usb_init_type init,
>  		struct ehci_hccr **hccr, struct ehci_hcor **hcor)
>  {
>  	enum usb_init_type type;
> -	struct usb_ehci *ehci = (struct usb_ehci *)(USBOH3_USB_BASE_ADDR +
> +	struct usb_ehci *ehci = (struct usb_ehci *)(USB_BASE_ADDR +
>  		(0x200 * index));
>  
>  	if (index > 3)
> 

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH 1/6] usb: ehci: mx6: Add support for i.MX6SL

[Otavio Salvador]

On Tue, Jun 17, 2014 at 11:56 AM, Stefano Babic <sbabic@denx.de> wrote:
> Hi Otavio,
>
> On 16/06/2014 02:46, Otavio Salvador wrote:
>> The i.MX6SL has a different base address for the controller. This
>> patch adapts the driver to support the different base address for this
>> case.
>>
>> Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
>> ---
>>
>>  drivers/usb/host/ehci-mx6.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
>> index c0a557b..5ba1c5e 100644
>> --- a/drivers/usb/host/ehci-mx6.c
>> +++ b/drivers/usb/host/ehci-mx6.c
>> @@ -53,6 +53,12 @@
>>  #define UCMD_RUN_STOP           (1 << 0) /* controller run/stop */
>>  #define UCMD_RESET           (1 << 1) /* controller reset */
>>
>> +#ifdef CONFIG_MX6SL
>> +#define USB_BASE_ADDR                USBO2H_USB_BASE_ADDR
>> +#else
>> +#define USB_BASE_ADDR                USBOH3_USB_BASE_ADDR
>> +#endif
>> +
>
> What about using the is_cpu_type() function, recently added together
> with SPL support ? I think we have reached, thank to Tim's patches, the
> goal to have a single image for different Freescale's mx6 variations. It
> is pity to go back and decide at compile time which SOC is running.

Yes; I will do it for v2.

-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

[U-Boot] [PATCH 1/6] usb: ehci: mx6: Add support for i.MX6SL

[Nikolay Dimitrov]

Hi Igor,

My personal opinion is that unless you intend to run the binary on 
multiple IMX6 variants, there's no
need to do expensive checks in runtime, when you can do the same at 
compile-time. For me it's the
same as choosing puts() vs printf() - you know at compile time whether 
you need to print arguments
or not, so same with the USB controller base address - you know in 
advance that you target a specific
CPU variant and not the other ones.

To a certain extent I agree that it would be awesome to have the same 
code running on all IMX6
variants, but the run-time checks will increase somewhat the binary 
footprint and U-Boot community has
already gone to great efforts to remove unnecessary bloat. My personal 
assumption is that such generic
approach would be more tolerated in the Linux kernel.

Kind regards,
Nikolay


On 6/16/2014 1:00 PM, u-boot-request at lists.denx.de wrote:
> Message: 30 Date: Mon, 16 Jun 2014 10:05:08 +0300 From: Igor Grinberg 
> <grinberg@compulab.co.il> Subject: Re: [U-Boot] [PATCH 1/6] usb: ehci: 
> mx6: Add support for i.MX6SL To: Otavio Salvador 
> <otavio@ossystems.com.br>, U-Boot Mailing List <u-boot@lists.denx.de> 
> Cc: Marek Vasut <marex@denx.de>, Fabio Estevam 
> <fabio.estevam@freescale.com> Message-ID: 
> <539E9724.4020809@compulab.co.il> Content-Type: text/plain; 
> charset=ISO-8859-1 Hmmm... Can't this be done dynamically? I mean... 
> you can check the SoC type in runtime, right? And then substitute the 
> correct address in a variable or so... I would really prefer such kind 
> of things done in runtime.
> -- Regards, Igor.

[U-Boot] [PATCH 1/6] usb: ehci: mx6: Add support for i.MX6SL

[Marek Vasut]

On Monday, June 16, 2014 at 04:33:09 PM, Nikolay Dimitrov wrote:
> Hi Igor,

Please do not top-post.

> My personal opinion is that unless you intend to run the binary on
> multiple IMX6 variants, there's no
> need to do expensive checks in runtime, when you can do the same at
> compile-time.

With the upcoming DM implementation, that's what we plan. Moreover, the checks 
are not expensive, you just decide the address based on the CPU model and assign 
it into some variable. You then access the registers via that variable + offset, 
as usual.

> For me it's the
> same as choosing puts() vs printf() - you know at compile time whether
> you need to print arguments
> or not, so same with the USB controller base address - you know in
> advance that you target a specific
> CPU variant and not the other ones.
> 
> To a certain extent I agree that it would be awesome to have the same
> code running on all IMX6
> variants, but the run-time checks will increase somewhat the binary
> footprint and U-Boot community has
> already gone to great efforts to remove unnecessary bloat. My personal
> assumption is that such generic
> approach would be more tolerated in the Linux kernel.
> 
> Kind regards,
> Nikolay
> 
> On 6/16/2014 1:00 PM, u-boot-request at lists.denx.de wrote:
> > Message: 30 Date: Mon, 16 Jun 2014 10:05:08 +0300 From: Igor Grinberg
> > <grinberg@compulab.co.il> Subject: Re: [U-Boot] [PATCH 1/6] usb: ehci:
> > mx6: Add support for i.MX6SL To: Otavio Salvador
> > <otavio@ossystems.com.br>, U-Boot Mailing List <u-boot@lists.denx.de>
> > Cc: Marek Vasut <marex@denx.de>, Fabio Estevam
> > <fabio.estevam@freescale.com> Message-ID:
> > <539E9724.4020809@compulab.co.il> Content-Type: text/plain;
> > charset=ISO-8859-1 Hmmm... Can't this be done dynamically? I mean...
> > you can check the SoC type in runtime, right? And then substitute the
> > correct address in a variable or so... I would really prefer such kind
> > of things done in runtime.
> > -- Regards, Igor.

Best regards,
Marek Vasut

[U-Boot] [PATCH 1/6] usb: ehci: mx6: Add support for i.MX6SL

[Nikolay Dimitrov]

Hi Marek,

On 6/16/2014 5:53 PM, Marek Vasut wrote:
> Please do not top-post. 
Please excuse me - I was unable to find posting guidelines for the 
mailing list, so just posted by habit (last time I had to post to 
another list, they wanted me to top-post).

> With the upcoming DM implementation, that's what we plan. Moreover, 
> the checks are not expensive, you just decide the address based on the 
> CPU model and assign it into some variable. You then access the 
> registers via that variable + offset, as usual. 
I see. Would be great to see this compatibility coming.

> Best regards,
> Marek Vasut
Thanks for taking your time to reply.

Kind regards,
Nikolay

[U-Boot] [PATCH 1/6] usb: ehci: mx6: Add support for i.MX6SL

[Marek Vasut]

On Tuesday, June 17, 2014 at 06:21:05 PM, Nikolay Dimitrov wrote:
> Hi Marek,
> 
> On 6/16/2014 5:53 PM, Marek Vasut wrote:
> > Please do not top-post.
> 
> Please excuse me - I was unable to find posting guidelines for the
> mailing list, so just posted by habit (last time I had to post to
> another list, they wanted me to top-post).

Then follow RFC1855, quote:

"
    - If you are sending a reply to a message or a posting be sure you
      summarize the original at the top of the message, or include just
      enough text of the original to give a context.  This will make
      sure readers understand when they start to read your response.
"
;-)

Best regards,
Marek Vasut

[U-Boot] [PATCH 1/6] usb: ehci: mx6: Add support for i.MX6SL

[Igor Grinberg]

Hi Nikolay,

On 06/16/14 17:33, Nikolay Dimitrov wrote:
> Hi Igor,
> 
> My personal opinion is that unless you intend to run the binary on multiple IMX6 variants,

That is exactly what we do already (code is on the way) and IMO what
we should aim for.

> there's no
> need to do expensive checks in runtime, when you can do the same at compile-time.

Why do you think it is expensive? Any benchmarks?
Also, the solution is what Marek already said, in short:
do the check once and store the result for future use...

> For me it's the
> same as choosing puts() vs printf() - you know at compile time whether you need to print arguments
> or not, so same with the USB controller base address - you know in advance that you target a specific
> CPU variant and not the other ones.

For me it is just an artificial complication which prevents single binary for
i.MX6 based boards.
Don't get me wrong, I think that in your board code you can choose which
approach you want, whether it will be single or multi binary, but this
is i.MX6 (and possibly future i.MX*) USB code which can be used on many
i.MX6 boards.

> 
> To a certain extent I agree that it would be awesome to have the same code running on all IMX6
> variants, but the run-time checks will increase somewhat the binary footprint and U-Boot community has
> already gone to great efforts to remove unnecessary bloat.

Again, what are we talking about? A couple of bytes?

> My personal assumption is that such generic
> approach would be more tolerated in the Linux kernel.

For Linux kernel this is the only acceptable way now.


-- 
Regards,
Igor.

[U-Boot] [PATCH 1/6] usb: ehci: mx6: Add support for i.MX6SL

[Nikolay Dimitrov]

Hi Igor,

On 6/17/2014 9:26 AM, Igor Grinberg wrote:
> That is exactly what we do already (code is on the way) and IMO what 
> we should aim for.
I really didn't knew this in the beginning before seeing your answers, 
this would be definitely easier
to support.

> For me it is just an artificial complication which prevents single binary for
> i.MX6 based boards.
> Don't get me wrong, I think that in your board code you can choose which
> approach you want, whether it will be single or multi binary, but this
> is i.MX6 (and possibly future i.MX*) USB code which can be used on many
> i.MX6 boards.
I definitely like the idea, no argument on this.

> Again, what are we talking about? A couple of bytes? 
I think it will cost 22 additional bytes per check. Here's a rough example:

(Code that just assigns the address to the struct pointer -> 10 bytes)
     p = (mx6_usb_t*)0x55667788;
     8380:       f247 7388       movw    r3, #30600      ; 0x7788
     8384:       f2c5 5366       movt    r3, #21862      ; 0x5566
     8388:       60fb            str     r3, [r7, #12]


(Code that checks in runtime the CPU type -> 32 bytes)
     if (is_imx6q())
     8380:       f7ff ffee       bl      8360 <is_imx6q>
     8384:       4603            mov     r3, r0
     8386:       2b00            cmp     r3, #0
     8388:       d005            beq.n   8396 <main+0x26>
     {
         p = (mx6_usb_t*)0x11223344;
     838a:       f243 3344       movw    r3, #13124      ; 0x3344
     838e:       f2c1 1322       movt    r3, #4386       ; 0x1122
     8392:       60fb            str     r3, [r7, #12]
     8394:       e004            b.n     83a0 <main+0x30>
     }
     else
     {
         p = (mx6_usb_t*)0x55667788;
     8396:       f247 7388       movw    r3, #30600      ; 0x7788
     839a:       f2c5 5366       movt    r3, #21862      ; 0x5566
     839e:       60fb            str     r3, [r7, #12]
     }

If I assume that we have 20 sub-systems in U-Boot, and each needs to 
check the CPU type
in 10 places, this makes 4400 bytes difference, which is roughly the 
size of a moderately
small driver in U-Boot. I need to say that I'm in no way expert in ARM 
assembly, so please
feel free to point out any mistakes in my assumptions.

Please don't get me wrong - I don't want to argue at all. I was just 
wondering about this topic
and decided to check with you guys just to be sure.

Kind regards,
Nikolay

[U-Boot] [PATCH 1/6] usb: ehci: mx6: Add support for i.MX6SL

[Igor Grinberg]

Hi Nikolay,

On 06/17/14 20:49, Nikolay Dimitrov wrote:
> Hi Igor,
> 
> On 6/17/2014 9:26 AM, Igor Grinberg wrote:
>> That is exactly what we do already (code is on the way) and IMO what we should aim for.
> I really didn't knew this in the beginning before seeing your answers, this would be definitely easier
> to support.
> 
>> For me it is just an artificial complication which prevents single binary for
>> i.MX6 based boards.
>> Don't get me wrong, I think that in your board code you can choose which
>> approach you want, whether it will be single or multi binary, but this
>> is i.MX6 (and possibly future i.MX*) USB code which can be used on many
>> i.MX6 boards.
> I definitely like the idea, no argument on this.
> 
>> Again, what are we talking about? A couple of bytes? 
> I think it will cost 22 additional bytes per check. Here's a rough example:
> 
> (Code that just assigns the address to the struct pointer -> 10 bytes)
>     p = (mx6_usb_t*)0x55667788;
>     8380:       f247 7388       movw    r3, #30600      ; 0x7788
>     8384:       f2c5 5366       movt    r3, #21862      ; 0x5566
>     8388:       60fb            str     r3, [r7, #12]
> 
> 
> (Code that checks in runtime the CPU type -> 32 bytes)
>     if (is_imx6q())
>     8380:       f7ff ffee       bl      8360 <is_imx6q>
>     8384:       4603            mov     r3, r0
>     8386:       2b00            cmp     r3, #0
>     8388:       d005            beq.n   8396 <main+0x26>
>     {
>         p = (mx6_usb_t*)0x11223344;
>     838a:       f243 3344       movw    r3, #13124      ; 0x3344
>     838e:       f2c1 1322       movt    r3, #4386       ; 0x1122
>     8392:       60fb            str     r3, [r7, #12]
>     8394:       e004            b.n     83a0 <main+0x30>
>     }
>     else
>     {
>         p = (mx6_usb_t*)0x55667788;
>     8396:       f247 7388       movw    r3, #30600      ; 0x7788
>     839a:       f2c5 5366       movt    r3, #21862      ; 0x5566
>     839e:       60fb            str     r3, [r7, #12]
>     }
> 
> If I assume that we have 20 sub-systems in U-Boot, and each needs to check the CPU type
> in 10 places, this makes 4400 bytes difference, which is roughly the size of a moderately
> small driver in U-Boot.

This looks like a very rough estimation.
It also can be improved by various ways.

Anyway, I think that even 4K addition is an acceptable compromise for a
single binary.

> I need to say that I'm in no way expert in ARM assembly, so please
> feel free to point out any mistakes in my assumptions.
> 
> Please don't get me wrong - I don't want to argue at all. I was just wondering about this topic
> and decided to check with you guys just to be sure.

No problem.


-- 
Regards,
Igor.