Skip to contentSkip to conversation

usb: ehci: mx6: Add support for i.MX6SL

PATCH New Otavio Salvador 8 participants 37 messages Jun 16, 2014 – Aug 18, 2014

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

PATCH 1/6 usb: ehci: mx6: Add support for i.MX6SL

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.

---

 drivers/usb/host/ehci-mx6.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
drivers/usb/host/ehci-mx6.c +8 -2
index c0a557b..5ba1c5e 100644
@@ -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)
Signature · 1 line
2.0.0
#1 lore raw

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

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

---

 arch/arm/include/asm/arch-mx6/mx6sl_pins.h | 5 +++++
 1 file changed, 5 insertions(+)
arch/arm/include/asm/arch-mx6/mx6sl_pins.h +5 -0
index 045ccc4..3ffd176 100644
@@ -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),
 
Signature · 1 line
2.0.0
#2 lore raw

PATCH 3/6 mmc: fsl_esdhc: Add CMD11 support to switch to 1.8V

In reply to Otavio Salvador

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

---

 drivers/mmc/fsl_esdhc.c | 30 +++++++++++++++++++++++-------
 include/fsl_esdhc.h     |  2 ++
 include/mmc.h           |  1 +
 3 files changed, 26 insertions(+), 7 deletions(-)
drivers/mmc/fsl_esdhc.c +23 -7
index 5541613..c75b38f 100644
@@ -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);
include/fsl_esdhc.h +2 -0
index 9814964..8876224 100644
@@ -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;
include/mmc.h +1 -0
index a3a100b..1b354ff 100644
@@ -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
Signature · 1 line
2.0.0
#3 lore raw

PATCH 3/6 mmc: fsl_esdhc: Add CMD11 support to switch to 1.8V

Quoted 46 lines · Otavio Salvador

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

---

 drivers/mmc/fsl_esdhc.c | 30 +++++++++++++++++++++++-------
 include/fsl_esdhc.h     |  2 ++
 include/mmc.h           |  1 +
 3 files changed, 26 insertions(+), 7 deletions(-)
drivers/mmc/fsl_esdhc.c +9 -7
index 5541613..c75b38f 100644
@@ -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;
        }

drivers/mmc/fsl_esdhc.c esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) · line 347

Quoted 10 lines
drivers/mmc/fsl_esdhc.c +9 -0
 
+       /* 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

#4 lore raw

PATCH 4/6 mmc: fsl_esdhc: Add support to force VSELECT set

In reply to 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.

---

 drivers/mmc/fsl_esdhc.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/mmc/fsl_esdhc.c +4 -0
index c75b38f..b3870e2 100644
@@ -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;
 }
 
Signature · 1 line
2.0.0
#5 lore raw

PATCH 4/6 mmc: fsl_esdhc: Add support to force VSELECT set

Quoted 1 line · Otavio Salvador

There are board were

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

Quoted 16 lines

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.

---

 drivers/mmc/fsl_esdhc.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/mmc/fsl_esdhc.c +0 -0
index c75b38f..b3870e2 100644

drivers/mmc/fsl_esdhc.c static int esdhc_init(struct mmc *mmc) · line 522

Quoted 6 lines
drivers/mmc/fsl_esdhc.c +3 -0
@@ -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

#6 lore raw

PATCH 4/6 mmc: fsl_esdhc: Add support to force VSELECT set

Quoted 4 lines · Marek Vasut
Quoted 1 line · Otavio Salvador

There are board were

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

I fixed the commit log, thanks.

...

Quoted 5 lines

drivers/mmc/fsl_esdhc.c static int esdhc_init(struct mmc *mmc) · line 522

Quoted 3 lines
drivers/mmc/fsl_esdhc.c +3 -0
+#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.

Signature · 3 lines
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
#7 lore raw

PATCH 4/6 mmc: fsl_esdhc: Add support to force VSELECT set

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

[...]

Quoted 8 lines
Quoted 5 lines

drivers/mmc/fsl_esdhc.c static int esdhc_init(struct mmc *mmc) · line 522

Quoted 3 lines
drivers/mmc/fsl_esdhc.c +3 -0
+#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

#8 lore raw

PATCH 4/6 mmc: fsl_esdhc: Add support to force VSELECT set

Quoted 10 lines · Marek Vasut

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

[...]

Quoted 6 lines
Quoted 5 lines

drivers/mmc/fsl_esdhc.c static int esdhc_init(struct mmc *mmc) · line 522

Quoted 3 lines
drivers/mmc/fsl_esdhc.c +3 -0
+#ifdef CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT
+     esdhc_setbits32(&regs->vendorspec, ESDHC_VENDORSPEC_VSELECT);
+#endif

Documentation is missing.

Quoted 6 lines
Quoted 2 lines

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.

Signature · 3 lines
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
#9 lore raw

PATCH 4/6 mmc: fsl_esdhc: Add support to force VSELECT set

Quoted 14 lines · Otavio Salvador
Quoted 13 lines · Marek Vasut

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

[...]

Quoted 8 lines
Quoted 5 lines

drivers/mmc/fsl_esdhc.c static int esdhc_init(struct mmc *mmc) · line 522

Quoted 3 lines
drivers/mmc/fsl_esdhc.c +3 -0
+#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.

Quoted 6 lines
Quoted 4 lines

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

#10 lore raw

PATCH 4/6 mmc: fsl_esdhc: Add support to force VSELECT set

Quoted 15 lines · Marek Vasut
Quoted 14 lines · Otavio Salvador
Quoted 13 lines · Marek Vasut

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

[...]

Quoted 8 lines
Quoted 5 lines

drivers/mmc/fsl_esdhc.c static int esdhc_init(struct mmc *mmc) · line 522

Quoted 3 lines
drivers/mmc/fsl_esdhc.c +3 -0
+#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.

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

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.

Signature · 3 lines
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
#11 lore raw

PATCH 4/6 mmc: fsl_esdhc: Add support to force VSELECT set

Hi Otavio,

Quoted 21 lines · Otavio Salvador
Quoted 20 lines · Marek Vasut
Quoted 19 lines · Otavio Salvador
Quoted 17 lines · Marek Vasut

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

[...]

Quoted 8 lines
Quoted 5 lines

Review

Quoted 3 lines
(quoted diff) +3 -0
+#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 ?

Quoted 7 lines
Quoted 3 lines
Quoted 1 line

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).

Signature · 2 lines
Regards,
Igor.
#12 lore raw

PATCH 4/6 mmc: fsl_esdhc: Add support to force VSELECT set

Quoted 28 lines · Igor Grinberg

Hi Otavio,

Quoted 25 lines · Otavio Salvador
Quoted 23 lines · Marek Vasut
Quoted 14 lines · Otavio Salvador
Quoted 13 lines · Marek Vasut

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

[...]

Quoted 8 lines
Quoted 5 lines

Review

Quoted 3 lines
(quoted diff) +3 -0
+#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.

Quoted 6 lines
Quoted 4 lines

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.

Quoted 8 lines
Quoted 3 lines

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?

Signature · 3 lines
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
#13 lore raw

PATCH 4/6 mmc: fsl_esdhc: Add support to force VSELECT set

Quoted 1 line · Otavio Salvador

On Mon, Jun 16, 2014 at 4:03 AM, Igor Grinberg <[email protected]>

Quoted 38 lines · wrote:
Quoted 30 lines

Hi Otavio,

Quoted 27 lines · Otavio Salvador
Quoted 15 lines · Marek Vasut
Quoted 14 lines · Otavio Salvador
Quoted 13 lines · Marek Vasut

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

[...]

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
(quoted diff) +2 -0
+#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.

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

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?

Quoted 7 lines
Quoted 1 line

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).

Quoted 3 lines

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

#14 lore raw

PATCH 4/6 mmc: fsl_esdhc: Add support to force VSELECT set

Hi Marek

Il 17/giu/2014 08:03 "Marek Vasut" <[email protected]> ha scritto:

Quoted 10 lines · Otavio Salvador
Quoted 1 line

On Mon, Jun 16, 2014 at 4:03 AM, Igor Grinberg <[email protected]>

Quoted 6 lines · wrote:
Quoted 6 lines

Hi Otavio,

Quoted 3 lines · Otavio Salvador
Quoted 2 lines · Marek Vasut
Quoted 1 line · Otavio Salvador

On Sun, Jun 15, 2014 at 10:27 PM, Marek Vasut <[email protected]>

Quoted 11 lines · wrote:
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines · Otavio Salvador

[...]

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
(quoted diff) +2 -0
+#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

Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
Quoted 8 lines
Quoted 6 lines
Quoted 1 line

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.

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 3 lines

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

Quoted 7 lines
Quoted 5 lines
Quoted 1 line

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

Quoted 18 lines

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

Quoted 2 lines

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

Quoted 1 line

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

Quoted 9 lines

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

#15 lore raw

PATCH 4/6 mmc: fsl_esdhc: Add support to force VSELECT set

Quoted 83 lines · Michael Trimarchi

Hi Marek

Il 17/giu/2014 08:03 "Marek Vasut" <[email protected]> ha scritto:

Quoted 11 lines · Otavio Salvador
Quoted 2 lines

On Mon, Jun 16, 2014 at 4:03 AM, Igor Grinberg
<[email protected]>

Quoted 6 lines · wrote:
Quoted 6 lines

Hi Otavio,

Quoted 3 lines · Otavio Salvador
Quoted 2 lines · Marek Vasut
Quoted 1 line · Otavio Salvador

On Sun, Jun 15, 2014 at 10:27 PM, Marek Vasut <[email protected]>

Quoted 11 lines · wrote:
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines · Otavio Salvador

[...]

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
(quoted diff) +2 -0
+#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

Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
Quoted 8 lines
Quoted 6 lines
Quoted 1 line

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.

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 3 lines

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

Quoted 7 lines
Quoted 5 lines
Quoted 1 line

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

Quoted 18 lines

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

Quoted 2 lines

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.

Quoted 8 lines

This

Quoted 1 line

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).

#16 lore raw

PATCH 4/6 mmc: fsl_esdhc: Add support to force VSELECT set

Hi

Quoted 88 lines · Marek Vasut
Quoted 87 lines · Michael Trimarchi

Hi Marek

Il 17/giu/2014 08:03 "Marek Vasut" <[email protected]> ha scritto:

Quoted 11 lines · Otavio Salvador
Quoted 2 lines

On Mon, Jun 16, 2014 at 4:03 AM, Igor Grinberg
<[email protected]>

Quoted 6 lines · wrote:
Quoted 6 lines

Hi Otavio,

Quoted 3 lines · Otavio Salvador
Quoted 2 lines · Marek Vasut
Quoted 1 line · Otavio Salvador

On Sun, Jun 15, 2014 at 10:27 PM, Marek Vasut <[email protected]>

Quoted 11 lines · wrote:
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines · Otavio Salvador

[...]

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
(quoted diff) +2 -0
+#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

Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
Quoted 8 lines
Quoted 6 lines
Quoted 1 line

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.

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 3 lines

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

Quoted 7 lines
Quoted 5 lines
Quoted 1 line

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

Quoted 18 lines

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

Quoted 2 lines

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

Quoted 1 line

makes the driver completely unmaintainable. I would like to see them

Quoted 8 lines
Quoted 4 lines

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

#17 lore raw

PATCH 4/6 mmc: fsl_esdhc: Add support to force VSELECT set

In reply to Otavio Salvador

Hi Otavio,

Quoted 33 lines · Otavio Salvador
Quoted 32 lines · Igor Grinberg

Hi Otavio,

Quoted 28 lines · Otavio Salvador
Quoted 15 lines · Marek Vasut
Quoted 14 lines · Otavio Salvador
Quoted 13 lines · Marek Vasut

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

[...]

Quoted 8 lines
Quoted 5 lines

Review

Quoted 3 lines
(quoted diff) +3 -0
+#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.

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

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.

Quoted 8 lines
Quoted 4 lines

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.

Signature · 2 lines
Regards,
Igor.
#18 lore raw

PATCH 4/6 mmc: fsl_esdhc: Add support to force VSELECT set

In reply to Otavio Salvador

Hi Otavio,

Quoted 16 lines · 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.

---

 drivers/mmc/fsl_esdhc.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/mmc/fsl_esdhc.c +0 -0
index c75b38f..b3870e2 100644

drivers/mmc/fsl_esdhc.c static int esdhc_init(struct mmc *mmc) · line 522

Quoted 6 lines
drivers/mmc/fsl_esdhc.c +3 -0
@@ -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

Signature · 5 lines
=====================================================================
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
=====================================================================
#19 lore raw

PATCH 4/6 mmc: fsl_esdhc: Add support to force VSELECT set

Quoted 20 lines · Stefano Babic
Quoted 19 lines · 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.

---

 drivers/mmc/fsl_esdhc.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/mmc/fsl_esdhc.c +0 -0
index c75b38f..b3870e2 100644
@@ -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);
 
Quoted 8 lines

drivers/mmc/fsl_esdhc.c static int esdhc_init(struct mmc *mmc) · line 522

Quoted 3 lines
drivers/mmc/fsl_esdhc.c +3 -0
+#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.

Signature · 3 lines
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
#20 lore raw

PATCH 4/6 mmc: fsl_esdhc: Add support to force VSELECT set

Hi Stefano

Quoted 30 lines · Otavio Salvador
Quoted 20 lines · Stefano Babic
Quoted 19 lines · 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.

---

 drivers/mmc/fsl_esdhc.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/mmc/fsl_esdhc.c +0 -0
index c75b38f..b3870e2 100644
@@ -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);
 
Quoted 8 lines

drivers/mmc/fsl_esdhc.c static int esdhc_init(struct mmc *mmc) · line 522

Quoted 3 lines
drivers/mmc/fsl_esdhc.c +3 -0
+#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 ?

Quoted 2 lines

I will try to cook something in this direction.

Well I already said some emails ago ;).

Michael

Quoted 8 lines

--
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

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

PATCH 4/6 mmc: fsl_esdhc: Add support to force VSELECT set

Hi Michael,

Quoted 9 lines · Michael Trimarchi
Quoted 7 lines
Quoted 4 lines

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

Signature · 5 lines
=====================================================================
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
=====================================================================
#22 lore raw

PATCH 5/6 Print ramdisk load address if it is in use

In reply to Otavio Salvador

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

---

 common/image.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
common/image.c +3 -2
index fa4864d..84e115f 100644
@@ -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;
 
Signature · 1 line
2.0.0
#23 lore raw

PATCH 5/6 Print ramdisk load address if it is in use

Dear Otavio Salvador,

Quoted 2 lines · In message <[email protected]> you…

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

Agreed.

common/image.c int boot_ramdisk_high(struct lmb *lmb, ulong rd_data, ulong rd_len, · line 1058

Quoted 9 lines
common/image.c +3 -2
+
+		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

Signature · 4 lines
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
#24 lore raw

PATCH 5/6 Print ramdisk load address if it is in use

Quoted 18 lines · Wolfgang Denk

Dear Otavio Salvador,

Quoted 2 lines · In message <[email protected]> you…

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

Agreed.

common/image.c int boot_ramdisk_high(struct lmb *lmb, ulong rd_data, ulong rd_len, · line 1058

Quoted 9 lines
common/image.c +3 -2
+
+             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);
Quoted 8 lines

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.

Signature · 3 lines
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
#25 lore raw

PATCH 6/6 warp: Add initial WaRP Board support

In reply to Otavio Salvador

---

 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
board/warp/Makefile new file +8 -0
new file mode 100644
index 0000000..c555f87
@@ -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
board/warp/warp.c new file +111 -0

111 lines not on this page · raw message

boards.cfg +1 -0
index fd4324d..435edf3 100644
@@ -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 <[email protected]>
 Active  arm         armv7          mx6         -               wandboard           wandboard_quad                        wandboard:IMX_CONFIG=board/boundary/nitrogen6x/nitrogen6q2g.cfg,MX6Q,DDR_MB=2048                                                  Fabio Estevam <[email protected]>
 Active  arm         armv7          mx6         -               wandboard           wandboard_solo                        wandboard:IMX_CONFIG=board/boundary/nitrogen6x/nitrogen6s.cfg,MX6S,DDR_MB=512                                                     Fabio Estevam <[email protected]>
+Active  arm         armv7          mx6         -               warp                warp                                  warp:IMX_CONFIG=board/freescale/mx6slevk/imximage.cfg,MX6SL                                                                       Otavio Salvador <[email protected]>
 Active  arm         armv7          mx6         barco           titanium            titanium                              titanium:IMX_CONFIG=board/barco/titanium/imximage.cfg                                                                             Stefan Roese <[email protected]>
 Active  arm         armv7          mx6         boundary        nitrogen6x          mx6qsabrelite                         nitrogen6x:IMX_CONFIG=board/boundary/nitrogen6x/nitrogen6q.cfg,MX6Q,DDR_MB=1024,SABRELITE                                         Eric Nelson <[email protected]>
 Active  arm         armv7          mx6         boundary        nitrogen6x          nitrogen6dl                           nitrogen6x:IMX_CONFIG=board/boundary/nitrogen6x/nitrogen6dl.cfg,MX6DL,DDR_MB=1024                                                 Eric Nelson <[email protected]>
include/configs/warp.h new file +198 -0

198 lines not on this page · raw message

Signature · 1 line
2.0.0
#26 lore raw

PATCH 6/6 warp: Add initial WaRP Board support

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

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

Quoted 7 lines

---

 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

#27 lore raw

PATCH 6/6 warp: Add initial WaRP Board support

In reply to Otavio Salvador

Hi Otavio,

Quoted 11 lines · Otavio Salvador

---

 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?

#28 lore raw

PATCH 6/6 warp: Add initial WaRP Board support

Quoted 16 lines · Fabio Estevam

Hi Otavio,

Quoted 11 lines · Otavio Salvador

---

 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
Quoted 1 line

Do you have to submit v2 of this series?

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

#29 lore raw

PATCH 6/6 warp: Add initial WaRP Board support

Quoted 17 lines · Fabio Estevam
Quoted 16 lines · Fabio Estevam

Hi Otavio,

Quoted 11 lines · Otavio Salvador

---

 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
Quoted 3 lines
Quoted 1 line

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.

Signature · 3 lines
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
#30 lore raw

PATCH 1/6 usb: ehci: mx6: Add support for i.MX6SL

In reply to Otavio Salvador
Quoted 15 lines · 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.

---

 drivers/usb/host/ehci-mx6.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
drivers/usb/host/ehci-mx6.c +0 -0
index c0a557b..5ba1c5e 100644

drivers/usb/host/ehci-mx6.c line 60

Quoted 8 lines
drivers/usb/host/ehci-mx6.c +5 -0
@@ -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

#31 lore raw

PATCH 1/6 usb: ehci: mx6: Add support for i.MX6SL

Quoted 6 lines · Marek Vasut
Quoted 5 lines · 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.

...

Quoted 1 line

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.

Signature · 3 lines
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
#32 lore raw

PATCH 1/6 usb: ehci: mx6: Add support for i.MX6SL

Quoted 8 lines · Otavio Salvador
Quoted 6 lines · Marek Vasut
Quoted 5 lines · 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.

Quoted 7 lines

...

Quoted 2 lines

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

#33 lore raw

PATCH 1/6 usb: ehci: mx6: Add support for i.MX6SL

In reply to Otavio Salvador

Hi Otavio,

Quoted 15 lines · 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.

---

 drivers/usb/host/ehci-mx6.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
drivers/usb/host/ehci-mx6.c +0 -0
index c0a557b..5ba1c5e 100644

drivers/usb/host/ehci-mx6.c line 61

Quoted 9 lines
drivers/usb/host/ehci-mx6.c +6 -0
@@ -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.

Quoted 22 lines
drivers/usb/host/ehci-mx6.c +2 -2
 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)
Signature · 2 lines
Regards,
Igor.
#34 lore raw

PATCH 1/6 usb: ehci: mx6: Add support for i.MX6SL

Quoted 21 lines · Igor Grinberg

Hi Otavio,

Quoted 18 lines · 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.

---

 drivers/usb/host/ehci-mx6.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
drivers/usb/host/ehci-mx6.c +0 -0
index c0a557b..5ba1c5e 100644
@@ -53,6 +53,12 @@
 #define UCMD_RUN_STOP           (1 << 0) /* controller run/stop */
 #define UCMD_RESET           (1 << 1) /* controller reset */
 
Quoted 11 lines

drivers/usb/host/ehci-mx6.c line 61

Quoted 6 lines
drivers/usb/host/ehci-mx6.c +6 -0
+#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.

Signature · 3 lines
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
#35 lore raw

PATCH 1/6 usb: ehci: mx6: Add support for i.MX6SL

In reply to Otavio Salvador

Hi Otavio,

Quoted 15 lines · 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.

---

 drivers/usb/host/ehci-mx6.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
drivers/usb/host/ehci-mx6.c +0 -0
index c0a557b..5ba1c5e 100644

drivers/usb/host/ehci-mx6.c line 61

Quoted 9 lines
drivers/usb/host/ehci-mx6.c +6 -0
@@ -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.

Quoted 14 lines
drivers/usb/host/ehci-mx6.c +1 -1
 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;

drivers/usb/host/ehci-mx6.c int ehci_hcd_init(int index, enum usb_init_type init, · line 219

Quoted 8 lines
drivers/usb/host/ehci-mx6.c +1 -1
@@ -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

Signature · 5 lines
=====================================================================
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
=====================================================================
#36 lore raw

PATCH 1/6 usb: ehci: mx6: Add support for i.MX6SL

Quoted 21 lines · Stefano Babic

Hi Otavio,

Quoted 18 lines · 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.

---

 drivers/usb/host/ehci-mx6.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
drivers/usb/host/ehci-mx6.c +0 -0
index c0a557b..5ba1c5e 100644
@@ -53,6 +53,12 @@
 #define UCMD_RUN_STOP           (1 << 0) /* controller run/stop */
 #define UCMD_RESET           (1 << 1) /* controller reset */
 
Quoted 11 lines

drivers/usb/host/ehci-mx6.c line 61

Quoted 6 lines
drivers/usb/host/ehci-mx6.c +6 -0
+#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.

Signature · 3 lines
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