Hi York,
> -----Original Message-----
> From: York Sun
> Sent: 2017年12月7日 3:08
> To: Y.b. Lu <yangbo...@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...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
