| Message ID | 20171206100010.13892-1-yangbo.lu@nxp.com |
|---|---|
| State | Superseded |
| Delegated to: | York Sun |
| Headers | show |
| Series | [U-Boot,v3] armv8: ls1012a: enable/disable eSDHC1 through hwconfig for new RDB | expand |
On 12/06/2017 02:19 AM, Yangbo Lu wrote: > For LS1012ARDB RevD and later versions, the I2C reading for DIP > switch setting had been no longer reliable since the board was > reworked. This patch is to add hwconfig support to enable/disable I think this message is not accurate. How about saying "I2C reading for DIP switch setting is not reliable for LS1012ARDB rev D and later revisions."? > eSDHC1 manually for these boards. Also let kernel decide status > of esdhc0. I see you dropped fixing up esdhc0. What do you mean "let kernel decide"? How? > > Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com> > --- > Changes for v2: > - Just used hwconfig() instead of getenv() and hwconfig_f(). > Changes for v3: > - only applied hwconfig support for RevD and later versions. > --- > board/freescale/ls1012ardb/ls1012ardb.c | 51 +++++++++++++++++++++------------ > include/configs/ls1012ardb.h | 4 +++ > 2 files changed, 36 insertions(+), 19 deletions(-) > > diff --git a/board/freescale/ls1012ardb/ls1012ardb.c b/board/freescale/ls1012ardb/ls1012ardb.c > index c6c1c71202..b73c7d6639 100644 > --- a/board/freescale/ls1012ardb/ls1012ardb.c > +++ b/board/freescale/ls1012ardb/ls1012ardb.c > @@ -132,39 +132,52 @@ int board_init(void) > > int esdhc_status_fixup(void *blob, const char *compat) > { > - char esdhc0_path[] = "/soc/esdhc@1560000"; > char esdhc1_path[] = "/soc/esdhc@1580000"; > + u8 in1; I think you can reuse "io". Not a big deal though. > u8 io = 0; > u8 mux_sdhc2; > - > - do_fixup_by_path(blob, esdhc0_path, "status", "okay", > - sizeof("okay"), 1); > + bool sdhc2_en = false; > > i2c_set_bus_num(0); > > - /* > - * The I2C IO-expander for mux select is used to control the muxing > - * of various onboard interfaces. > - * > - * IO1[3:2] indicates SDHC2 interface demultiplexer select lines. > - * 00 - SDIO wifi > - * 01 - GPIO (to Arduino) > - * 10 - eMMC Memory > - * 11 - SPI > - */ > - if (i2c_read(I2C_MUX_IO1_ADDR, 0, 1, &io, 1) < 0) { > + if (i2c_read(I2C_MUX_IO1_ADDR, 1, 1, &in1, 1) < 0) { It would be helpful to use a named macro instead of "1" here as the register address. It is not obvious that you are reading a revision number. You can also put a comment. You said I2C reading the DIP is not reliable. Is this reading reliable? > printf("Error reading i2c boot information!\n"); > - return 0; /* Don't want to hang() on this error */ > + return 0; What does I2C failure mean here? Returning 0 will cause the code keep going in fdt_fixup_esdhc(). > + } > + > + if ((in1 & (~__SW_REV_MASK)) <= __SW_REV_D) { > + if (hwconfig("esdhc1")) Please double check. I remember in the past if hwconfig is not enabled, any check returns true. > + sdhc2_en = true; > + } else { > + /* > + * The I2C IO-expander for mux select is used to control > + * the muxing of various onboard interfaces. > + * > + * IO1[3:2] indicates SDHC2 interface demultiplexer > + * select lines. > + * 00 - SDIO wifi > + * 01 - GPIO (to Arduino) > + * 10 - eMMC Memory > + * 11 - SPI > + */ > + if (i2c_read(I2C_MUX_IO1_ADDR, 0, 1, &io, 1) < 0) { > + printf("Error reading i2c boot information!\n"); > + return 0; /* Don't want to hang() on this error */ > + } > + > + mux_sdhc2 = (io & 0x0c) >> 2; > + /* Enable SDHC2 only when use SDIO wifi and eMMC */ > + if (mux_sdhc2 == 2 || mux_sdhc2 == 0) > + sdhc2_en = true; > } > > - mux_sdhc2 = (io & 0x0c) >> 2; > - /* Enable SDHC2 only when use SDIO wifi and eMMC */ > - if (mux_sdhc2 == 2 || mux_sdhc2 == 0) > + if (sdhc2_en) > do_fixup_by_path(blob, esdhc1_path, "status", "okay", > sizeof("okay"), 1); > else > do_fixup_by_path(blob, esdhc1_path, "status", "disabled", > sizeof("disabled"), 1); > + > return 0; > } > > diff --git a/include/configs/ls1012ardb.h b/include/configs/ls1012ardb.h > index 794117062f..d5384e6027 100644 > --- a/include/configs/ls1012ardb.h > +++ b/include/configs/ls1012ardb.h > @@ -32,6 +32,10 @@ > #define __SW_REV_MASK 0x07 > #define __SW_REV_A 0xF8 > #define __SW_REV_B 0xF0 > +#define __SW_REV_C 0xE8 > +#define __SW_REV_C1 0xE0 > +#define __SW_REV_C2 0xD8 > +#define __SW_REV_D 0xD0 I don't have strong opinion about these macros. I would not use underscore myself. I guess I missed them at the first place. York
Hi York, > -----Original Message----- > From: York Sun > Sent: 2017年12月7日 3:08 > To: Y.b. Lu <yangbo.lu@nxp.com>; u-boot@lists.denx.de > Subject: Re: [v3] armv8: ls1012a: enable/disable eSDHC1 through hwconfig for > new RDB > > On 12/06/2017 02:19 AM, Yangbo Lu wrote: > > For LS1012ARDB RevD and later versions, the I2C reading for DIP switch > > setting had been no longer reliable since the board was reworked. This > > patch is to add hwconfig support to enable/disable > > I think this message is not accurate. How about saying "I2C reading for DIP > switch setting is not reliable for LS1012ARDB rev D and later revisions."? > [Y.b. Lu] Thanks. This is better. > > eSDHC1 manually for these boards. Also let kernel decide status of > > esdhc0. > > I see you dropped fixing up esdhc0. What do you mean "let kernel decide"? > How? [Y.b. Lu] I mean unless to avoid some issue, we don’t need to do any fix-up for 'status'. Let me rephrase it as "Also drop 'status' fix-up for esdhc0 and leave it as it is. It shouldn’t be always fixed up with 'okay'. ". > > > > > Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com> > > --- > > Changes for v2: > > - Just used hwconfig() instead of getenv() and hwconfig_f(). > > Changes for v3: > > - only applied hwconfig support for RevD and later versions. > > --- > > board/freescale/ls1012ardb/ls1012ardb.c | 51 > +++++++++++++++++++++------------ > > include/configs/ls1012ardb.h | 4 +++ > > 2 files changed, 36 insertions(+), 19 deletions(-) > > > > diff --git a/board/freescale/ls1012ardb/ls1012ardb.c > > b/board/freescale/ls1012ardb/ls1012ardb.c > > index c6c1c71202..b73c7d6639 100644 > > --- a/board/freescale/ls1012ardb/ls1012ardb.c > > +++ b/board/freescale/ls1012ardb/ls1012ardb.c > > @@ -132,39 +132,52 @@ int board_init(void) > > > > int esdhc_status_fixup(void *blob, const char *compat) { > > - char esdhc0_path[] = "/soc/esdhc@1560000"; > > char esdhc1_path[] = "/soc/esdhc@1580000"; > > + u8 in1; > > I think you can reuse "io". Not a big deal though. [Y.b. Lu] Ok. Will do that. > > > u8 io = 0; > > u8 mux_sdhc2; > > - > > - do_fixup_by_path(blob, esdhc0_path, "status", "okay", > > - sizeof("okay"), 1); > > + bool sdhc2_en = false; > > > > i2c_set_bus_num(0); > > > > - /* > > - * The I2C IO-expander for mux select is used to control the muxing > > - * of various onboard interfaces. > > - * > > - * IO1[3:2] indicates SDHC2 interface demultiplexer select lines. > > - * 00 - SDIO wifi > > - * 01 - GPIO (to Arduino) > > - * 10 - eMMC Memory > > - * 11 - SPI > > - */ > > - if (i2c_read(I2C_MUX_IO1_ADDR, 0, 1, &io, 1) < 0) { > > + if (i2c_read(I2C_MUX_IO1_ADDR, 1, 1, &in1, 1) < 0) { > > It would be helpful to use a named macro instead of "1" here as the register > address. It is not obvious that you are reading a revision number. You can also > put a comment. [Y.b. Lu] Let me put a comment here for this patch. I think it's proper to replace all the numbers with macro in this file in another patch. > > You said I2C reading the DIP is not reliable. Is this reading reliable? [Y.b. Lu] This is a question... Although I had verified this patch on RevE(the value read is correct), let me confirm with Kinjalk before sending new version patch. > > > printf("Error reading i2c boot information!\n"); > > - return 0; /* Don't want to hang() on this error */ > > + return 0; > > What does I2C failure mean here? Returning 0 will cause the code keep going in > fdt_fixup_esdhc(). [Y.b. Lu] I don’t know the possibility of I2C failure. If the failure happens, just print error message, leave 'status' as it is, and return 0 because the other fix-ups in fdt_fixup_esdhc() are still needed like clock-frequency. > > > + } > > + > > + if ((in1 & (~__SW_REV_MASK)) <= __SW_REV_D) { > > + if (hwconfig("esdhc1")) > > Please double check. I remember in the past if hwconfig is not enabled, any > check returns true. [Y.b. Lu] You're right. It's -ENOSYS. But what should I do here ? > > > + sdhc2_en = true; > > + } else { > > + /* > > + * The I2C IO-expander for mux select is used to control > > + * the muxing of various onboard interfaces. > > + * > > + * IO1[3:2] indicates SDHC2 interface demultiplexer > > + * select lines. > > + * 00 - SDIO wifi > > + * 01 - GPIO (to Arduino) > > + * 10 - eMMC Memory > > + * 11 - SPI > > + */ > > + if (i2c_read(I2C_MUX_IO1_ADDR, 0, 1, &io, 1) < 0) { > > + printf("Error reading i2c boot information!\n"); > > + return 0; /* Don't want to hang() on this error */ > > + } > > + > > + mux_sdhc2 = (io & 0x0c) >> 2; > > + /* Enable SDHC2 only when use SDIO wifi and eMMC */ > > + if (mux_sdhc2 == 2 || mux_sdhc2 == 0) > > + sdhc2_en = true; > > } > > > > - mux_sdhc2 = (io & 0x0c) >> 2; > > - /* Enable SDHC2 only when use SDIO wifi and eMMC */ > > - if (mux_sdhc2 == 2 || mux_sdhc2 == 0) > > + if (sdhc2_en) > > do_fixup_by_path(blob, esdhc1_path, "status", "okay", > > sizeof("okay"), 1); > > else > > do_fixup_by_path(blob, esdhc1_path, "status", "disabled", > > sizeof("disabled"), 1); > > + > > return 0; > > } > > > > diff --git a/include/configs/ls1012ardb.h > > b/include/configs/ls1012ardb.h index 794117062f..d5384e6027 100644 > > --- a/include/configs/ls1012ardb.h > > +++ b/include/configs/ls1012ardb.h > > @@ -32,6 +32,10 @@ > > #define __SW_REV_MASK 0x07 > > #define __SW_REV_A 0xF8 > > #define __SW_REV_B 0xF0 > > +#define __SW_REV_C 0xE8 > > +#define __SW_REV_C1 0xE0 > > +#define __SW_REV_C2 0xD8 > > +#define __SW_REV_D 0xD0 > > I don't have strong opinion about these macros. I would not use underscore > myself. I guess I missed them at the first place. [Y.b. Lu] It's confusing why define macro like this... And the __SW_BOOT_EMU is wrong, it should be 0x02, not 0x10. > > York
On 12/07/2017 12:10 AM, Y.b. Lu wrote: <snip> > >> >>> printf("Error reading i2c boot information!\n"); >>> - return 0; /* Don't want to hang() on this error */ >>> + return 0; >> >> What does I2C failure mean here? Returning 0 will cause the code keep going in >> fdt_fixup_esdhc(). > > [Y.b. Lu] I don’t know the possibility of I2C failure. If the failure happens, just print error message, leave 'status' as it is, and return 0 because the other fix-ups in fdt_fixup_esdhc() are still needed like clock-frequency. You can move on if it makes sense to do so. > >> >>> + } >>> + >>> + if ((in1 & (~__SW_REV_MASK)) <= __SW_REV_D) { >>> + if (hwconfig("esdhc1")) >> >> Please double check. I remember in the past if hwconfig is not enabled, any >> check returns true. > > [Y.b. Lu] You're right. It's -ENOSYS. But what should I do here ? It depends on which one is safe. If you want to presume esdhc1 is available, you can keep current code. If you'd rather not to enable it, add a check of #ifdef. > >> >>> + sdhc2_en = true; >>> + } else { >>> + /* >>> + * The I2C IO-expander for mux select is used to control >>> + * the muxing of various onboard interfaces. >>> + * >>> + * IO1[3:2] indicates SDHC2 interface demultiplexer >>> + * select lines. >>> + * 00 - SDIO wifi >>> + * 01 - GPIO (to Arduino) >>> + * 10 - eMMC Memory >>> + * 11 - SPI >>> + */ >>> + if (i2c_read(I2C_MUX_IO1_ADDR, 0, 1, &io, 1) < 0) { >>> + printf("Error reading i2c boot information!\n"); >>> + return 0; /* Don't want to hang() on this error */ >>> + } >>> + >>> + mux_sdhc2 = (io & 0x0c) >> 2; >>> + /* Enable SDHC2 only when use SDIO wifi and eMMC */ >>> + if (mux_sdhc2 == 2 || mux_sdhc2 == 0) >>> + sdhc2_en = true; >>> } >>> >>> - mux_sdhc2 = (io & 0x0c) >> 2; >>> - /* Enable SDHC2 only when use SDIO wifi and eMMC */ >>> - if (mux_sdhc2 == 2 || mux_sdhc2 == 0) >>> + if (sdhc2_en) >>> do_fixup_by_path(blob, esdhc1_path, "status", "okay", >>> sizeof("okay"), 1); >>> else >>> do_fixup_by_path(blob, esdhc1_path, "status", "disabled", >>> sizeof("disabled"), 1); >>> + >>> return 0; >>> } >>> >>> diff --git a/include/configs/ls1012ardb.h >>> b/include/configs/ls1012ardb.h index 794117062f..d5384e6027 100644 >>> --- a/include/configs/ls1012ardb.h >>> +++ b/include/configs/ls1012ardb.h >>> @@ -32,6 +32,10 @@ >>> #define __SW_REV_MASK 0x07 >>> #define __SW_REV_A 0xF8 >>> #define __SW_REV_B 0xF0 >>> +#define __SW_REV_C 0xE8 >>> +#define __SW_REV_C1 0xE0 >>> +#define __SW_REV_C2 0xD8 >>> +#define __SW_REV_D 0xD0 >> >> I don't have strong opinion about these macros. I would not use underscore >> myself. I guess I missed them at the first place. > > [Y.b. Lu] It's confusing why define macro like this... > And the __SW_BOOT_EMU is wrong, it should be 0x02, not 0x10. Add another patch before this one to clean them up, please. York
Sent out v4 patch-set. Please help to review. Thanks a lot :) > -----Original Message----- > From: York Sun > Sent: 2017年12月8日 1:34 > To: Y.b. Lu <yangbo.lu@nxp.com>; u-boot@lists.denx.de > Subject: Re: [v3] armv8: ls1012a: enable/disable eSDHC1 through hwconfig for > new RDB > > On 12/07/2017 12:10 AM, Y.b. Lu wrote: > > <snip> > > > > >> > >>> printf("Error reading i2c boot information!\n"); > >>> - return 0; /* Don't want to hang() on this error */ > >>> + return 0; > >> > >> What does I2C failure mean here? Returning 0 will cause the code keep > >> going in fdt_fixup_esdhc(). > > > > [Y.b. Lu] I don’t know the possibility of I2C failure. If the failure happens, just > print error message, leave 'status' as it is, and return 0 because the other > fix-ups in fdt_fixup_esdhc() are still needed like clock-frequency. > > You can move on if it makes sense to do so. > > > > >> > >>> + } > >>> + > >>> + if ((in1 & (~__SW_REV_MASK)) <= __SW_REV_D) { > >>> + if (hwconfig("esdhc1")) > >> > >> Please double check. I remember in the past if hwconfig is not > >> enabled, any check returns true. > > > > [Y.b. Lu] You're right. It's -ENOSYS. But what should I do here ? > > It depends on which one is safe. If you want to presume esdhc1 is available, > you can keep current code. If you'd rather not to enable it, add a check of > #ifdef. > > > > >> > >>> + sdhc2_en = true; > >>> + } else { > >>> + /* > >>> + * The I2C IO-expander for mux select is used to control > >>> + * the muxing of various onboard interfaces. > >>> + * > >>> + * IO1[3:2] indicates SDHC2 interface demultiplexer > >>> + * select lines. > >>> + * 00 - SDIO wifi > >>> + * 01 - GPIO (to Arduino) > >>> + * 10 - eMMC Memory > >>> + * 11 - SPI > >>> + */ > >>> + if (i2c_read(I2C_MUX_IO1_ADDR, 0, 1, &io, 1) < 0) { > >>> + printf("Error reading i2c boot information!\n"); > >>> + return 0; /* Don't want to hang() on this error */ > >>> + } > >>> + > >>> + mux_sdhc2 = (io & 0x0c) >> 2; > >>> + /* Enable SDHC2 only when use SDIO wifi and eMMC */ > >>> + if (mux_sdhc2 == 2 || mux_sdhc2 == 0) > >>> + sdhc2_en = true; > >>> } > >>> > >>> - mux_sdhc2 = (io & 0x0c) >> 2; > >>> - /* Enable SDHC2 only when use SDIO wifi and eMMC */ > >>> - if (mux_sdhc2 == 2 || mux_sdhc2 == 0) > >>> + if (sdhc2_en) > >>> do_fixup_by_path(blob, esdhc1_path, "status", "okay", > >>> sizeof("okay"), 1); > >>> else > >>> do_fixup_by_path(blob, esdhc1_path, "status", "disabled", > >>> sizeof("disabled"), 1); > >>> + > >>> return 0; > >>> } > >>> > >>> diff --git a/include/configs/ls1012ardb.h > >>> b/include/configs/ls1012ardb.h index 794117062f..d5384e6027 100644 > >>> --- a/include/configs/ls1012ardb.h > >>> +++ b/include/configs/ls1012ardb.h > >>> @@ -32,6 +32,10 @@ > >>> #define __SW_REV_MASK 0x07 > >>> #define __SW_REV_A 0xF8 > >>> #define __SW_REV_B 0xF0 > >>> +#define __SW_REV_C 0xE8 > >>> +#define __SW_REV_C1 0xE0 > >>> +#define __SW_REV_C2 0xD8 > >>> +#define __SW_REV_D 0xD0 > >> > >> I don't have strong opinion about these macros. I would not use > >> underscore myself. I guess I missed them at the first place. > > > > [Y.b. Lu] It's confusing why define macro like this... > > And the __SW_BOOT_EMU is wrong, it should be 0x02, not 0x10. > > Add another patch before this one to clean them up, please. > > York
diff --git a/board/freescale/ls1012ardb/ls1012ardb.c b/board/freescale/ls1012ardb/ls1012ardb.c index c6c1c71202..b73c7d6639 100644 --- a/board/freescale/ls1012ardb/ls1012ardb.c +++ b/board/freescale/ls1012ardb/ls1012ardb.c @@ -132,39 +132,52 @@ int board_init(void) int esdhc_status_fixup(void *blob, const char *compat) { - char esdhc0_path[] = "/soc/esdhc@1560000"; char esdhc1_path[] = "/soc/esdhc@1580000"; + u8 in1; u8 io = 0; u8 mux_sdhc2; - - do_fixup_by_path(blob, esdhc0_path, "status", "okay", - sizeof("okay"), 1); + bool sdhc2_en = false; i2c_set_bus_num(0); - /* - * The I2C IO-expander for mux select is used to control the muxing - * of various onboard interfaces. - * - * IO1[3:2] indicates SDHC2 interface demultiplexer select lines. - * 00 - SDIO wifi - * 01 - GPIO (to Arduino) - * 10 - eMMC Memory - * 11 - SPI - */ - if (i2c_read(I2C_MUX_IO1_ADDR, 0, 1, &io, 1) < 0) { + if (i2c_read(I2C_MUX_IO1_ADDR, 1, 1, &in1, 1) < 0) { printf("Error reading i2c boot information!\n"); - return 0; /* Don't want to hang() on this error */ + return 0; + } + + if ((in1 & (~__SW_REV_MASK)) <= __SW_REV_D) { + if (hwconfig("esdhc1")) + sdhc2_en = true; + } else { + /* + * The I2C IO-expander for mux select is used to control + * the muxing of various onboard interfaces. + * + * IO1[3:2] indicates SDHC2 interface demultiplexer + * select lines. + * 00 - SDIO wifi + * 01 - GPIO (to Arduino) + * 10 - eMMC Memory + * 11 - SPI + */ + if (i2c_read(I2C_MUX_IO1_ADDR, 0, 1, &io, 1) < 0) { + printf("Error reading i2c boot information!\n"); + return 0; /* Don't want to hang() on this error */ + } + + mux_sdhc2 = (io & 0x0c) >> 2; + /* Enable SDHC2 only when use SDIO wifi and eMMC */ + if (mux_sdhc2 == 2 || mux_sdhc2 == 0) + sdhc2_en = true; } - mux_sdhc2 = (io & 0x0c) >> 2; - /* Enable SDHC2 only when use SDIO wifi and eMMC */ - if (mux_sdhc2 == 2 || mux_sdhc2 == 0) + if (sdhc2_en) do_fixup_by_path(blob, esdhc1_path, "status", "okay", sizeof("okay"), 1); else do_fixup_by_path(blob, esdhc1_path, "status", "disabled", sizeof("disabled"), 1); + return 0; } diff --git a/include/configs/ls1012ardb.h b/include/configs/ls1012ardb.h index 794117062f..d5384e6027 100644 --- a/include/configs/ls1012ardb.h +++ b/include/configs/ls1012ardb.h @@ -32,6 +32,10 @@ #define __SW_REV_MASK 0x07 #define __SW_REV_A 0xF8 #define __SW_REV_B 0xF0 +#define __SW_REV_C 0xE8 +#define __SW_REV_C1 0xE0 +#define __SW_REV_C2 0xD8 +#define __SW_REV_D 0xD0 /* MMC */ #ifdef CONFIG_MMC
For LS1012ARDB RevD and later versions, the I2C reading for DIP switch setting had been no longer reliable since the board was reworked. This patch is to add hwconfig support to enable/disable eSDHC1 manually for these boards. Also let kernel decide status of esdhc0. Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com> --- Changes for v2: - Just used hwconfig() instead of getenv() and hwconfig_f(). Changes for v3: - only applied hwconfig support for RevD and later versions. --- board/freescale/ls1012ardb/ls1012ardb.c | 51 +++++++++++++++++++++------------ include/configs/ls1012ardb.h | 4 +++ 2 files changed, 36 insertions(+), 19 deletions(-)