Re: [U-Boot] [PATCH v4 0/4] configs: rk3288: Tinker Board SPL file must fit into 32 KiB

[Benoît Thébaudeau]

Hi Heinrich,

On Wed, Apr 3, 2019 at 8:20 AM Heinrich Schuchardt  wrote:
>
> On 4/3/19 2:59 AM, Kever Yang wrote:
> > Hi Heinrich,
> >
> >
> > On 04/03/2019 01:19 AM, Heinrich Schuchardt wrote:
> >> The SPL image for the Tinker Board has to fit into 32 KiB. This includes
> >> up to 2 KiB for the file header.
> >
> > 32KB is the limit of SPL size, not SPL image size, does not include 2KB
> > header.
> >>
> >> A new configuration variable CONFIG_SPL_SIZE_LIMIT is introduced to define
> >> the board specific limit.
> >
> > There is already CONFIG_SPL_MAX_SIZE for the SPL size limit, isn't it?
> > I don't understand  why we need new variable.
>
> SPL_MAX_SIZE is used to set CONFIG_SPL_PAD_TO if CONFIG_SPL_PAD_TO is
> not defined. If CONFIG_SPL_PAD_TO is defined it specifies the maximum
> value of CONFIG_SPL_PAD_TO.

If CONFIG_SPL_PAD_TO is defined and non-zero, CONFIG_SPL_MAX_SIZE
specifies the _minimum_ value of CONFIG_SPL_PAD_TO.

> So in case CONFIG_SPL_PAD_TO is not defined
> SPL_MAX_SIZE does not specify a maximum size but a minimum one.

CONFIG_SPL_MAX_SIZE specifies a minimum value for CONFIG_SPL_PAD_TO,
but a maximum size for the SPL.

> It is unclear to me why Benoît in 6113d3f27ca ("Makefile: Change
> CONFIG_SPL_PAD_TO to image offset") chose this design. Entangling the
> variables in this way does not provide a clean design.

Everything is explained in this thread:
https://lists.denx.de/pipermail/u-boot/2013-February/146851.html

These two variables coexisted before this change. They have different
purposes, but they are related, so they cannot take any values
independently of each other.

The purpose of CONFIG_SPL_MAX_SIZE is not to define default or minimum
values for CONFIG_SPL_PAD_TO, but only what its name says, i.e. to
define the maximum SPL size.

After the SPL, there may be a gap, then a payload appended to the
image. The purpose of CONFIG_SPL_PAD_TO is to define the image offset
of the payload, if any. CONFIG_SPL_PAD_TO can be 0 if the payload can
always be appended to the SPL without any gap.

Therefore, the SPL must fit inside CONFIG_SPL_MAX_SIZE, and
CONFIG_SPL_MAX_SIZE must fit inside CONFIG_SPL_PAD_TO if the latter is
non-zero.

[...]

Best regards,
Benoît

On Wed, Apr 3, 2019 at 8:20 AM Heinrich Schuchardt  wrote:
>
> On 4/3/19 2:59 AM, Kever Yang wrote:
> > Hi Heinrich,
> >
> >
> > On 04/03/2019 01:19 AM, Heinrich Schuchardt wrote:
> >> The SPL image for the Tinker Board has to fit into 32 KiB. This includes
> >> up to 2 KiB for the file header.
> >
> > 32KB is the limit of SPL size, not SPL image size, does not include 2KB
> > header.
> >>
> >> A new configuration variable CONFIG_SPL_SIZE_LIMIT is introduced to define
> >> the board specific limit.
> >
> > There is already CONFIG_SPL_MAX_SIZE for the SPL size limit, isn't it?
> > I don't understand  why we need new variable.
>
> SPL_MAX_SIZE is used to set CONFIG_SPL_PAD_TO if CONFIG_SPL_PAD_TO is
> not defined. If CONFIG_SPL_PAD_TO is defined it specifies the maximum
> value of CONFIG_SPL_PAD_TO. So in case CONFIG_SPL_PAD_TO is not defined
> SPL_MAX_SIZE does not specify a maximum size but a minimum one.
>
> It is unclear to me why Benoît in 6113d3f27ca ("Makefile: Change
> CONFIG_SPL_PAD_TO to image offset") chose this design. Entangling the
> variables in this way does not provide a clean design.
>
> The check
>
> #if defined(IMAGE_MAX_SIZE)
> ASSERT(__image_copy_end - __image_copy_start < (IMAGE_MAX_SIZE), \
> "SPL image too big");
> #endif
>
> in arch/arm/cpu/u-boot-spl.lds neither considers the size of the device
> tree nor of the BSS section. So at least for Rockchip SOCs it is
> checking a measure that is not directly related to the image size
> created by `mkimage -T rksd`.
>
> Best regards
>
> Heinrich
>
> >
> > Thanks,
> > - Kever
> >> A common Makefile function is used for this test and the test against
> >> CONFIG_BOARD_SIZE_LIMIT.
> >>
> >> Move the board size check from arch/arm/mach-imx/Makefile to Makefile.
> >>
> >> v4:
> >>  use a common function for all size checks in the Makefiles
> >>
> >> Heinrich Schuchardt (4):
> >>   Makefile: reusable function for BOARD_SIZE_CHECK
> >>   imx: move BOARD_SIZE_CHECK to main Makefile
> >>   configs: define CONFIG_SPL_SIZE_LIMIT
> >>   configs: rk3288: Tinker Board SPL file must fit into 32 KiB
> >>
> >>  Kconfig |  8 
> >>  Makefile| 33 +++--
> >>  arch/arm/mach-imx/Makefile  | 16 
> >>  configs/tinker-rk3288_defconfig |  1 +
> >>  4 files changed, 32 insertions(+), 26 deletions(-)
> >>
> >> --
> >> 2.20.1
> ___
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 37/41] mmc: fsl_esdhc: fix sd/mmc ddr mode clock setting issue

[Benoît Thébaudeau]

Dear Peng Fan,

Some other remarks.

On Tue, May 29, 2018 at 3:45 AM, Peng Fan  wrote:
>
>
>> -Original Message-----
>> From: Benoît Thébaudeau [mailto:benoit.thebaudeau@gmail.com]
>> Sent: 2018年5月29日 6:32
>> To: Peng Fan 
>> Cc: sba...@denx.de; Fabio Estevam ; U-Boot
>> ; Bough Chen 
>> Subject: Re: [U-Boot] [PATCH 37/41] mmc: fsl_esdhc: fix sd/mmc ddr mode clock
>> setting issue
>>
>> Dear Peng Fan,
>>
>> On Mon, May 28, 2018 at 2:25 PM, Peng Fan  wrote:
>> > From: Ye Li 
>> >
>> > When sd/mmc work at DDR mode, like HS400/HS400ES/DDR52/DDR50 mode,
>> the
>> > actual clock rate is just half of the expected clock.
>> >
>> > This patch set the DDR_EN bit first for DDR mode, hardware divide the
>> > usdhc clock automatically, then follow the original sdr clock setting
>> > method.
>> >
>> > Signed-off-by: Haibo Chen 
>> > Signed-off-by: Ye Li 
>> > ---
>> >  drivers/mmc/fsl_esdhc.c | 24 +++-
>> >  1 file changed, 19 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c index
>> > 1b062ff06d..bf4ae74847 100644
>> > --- a/drivers/mmc/fsl_esdhc.c
>> > +++ b/drivers/mmc/fsl_esdhc.c
>> > @@ -583,18 +583,32 @@ static void set_sysctl(struct fsl_esdhc_priv
>> > *priv, struct mmc *mmc, uint clock)  #else
>> > int pre_div = 2;
>> >  #endif
>> > -   int ddr_pre_div = mmc->ddr_mode ? 2 : 1;
>> > int sdhc_clk = priv->sdhc_clk;
>> > uint clk;
>> >
>> > +   /*
>> > +* For ddr mode, usdhc need to enable DDR mode first, after select
>> > +* this DDR mode, usdhc will automatically divide the usdhc clock
>> > +*/
>> > +   if (mmc->ddr_mode) {
>> > +   writel(readl(>mixctrl) | MIX_CTRL_DDREN,
>> >mixctrl);

Isn't this redundant with what is done in esdhc_set_timing()?
According to the reference manual, the prescalers might have to be set
after DDREN (as done here), so perhaps DDREN does not have to be set
too in esdhc_set_timing().

>> > +   sdhc_clk >>= 1;
>> > +   }
>> > +
>> > if (clock < mmc->cfg->f_min)
>> > clock = mmc->cfg->f_min;
>> >
>> > -   while (sdhc_clk / (16 * pre_div * ddr_pre_div) > clock && pre_div <
>> 256)
>> > -   pre_div *= 2;
>> > +   if ((sdhc_clk / 16) > clock) {
>> > +   for (; pre_div < 256; pre_div *= 2)
>> > +   if ((sdhc_clk / pre_div) <= (clock * 16))
>> > +   break;
>> > +   } else {
>> > +   pre_div = 1;
>>
>> This value is not always available for this divider. See the conditions in 
>> the
>> initialization of this variable giving its minimum value.
>
> The else {pre_div = 1;} could be removed then.

And the if conditioning the for loop becomes useless. In the end, the
new loop would be equivalent to the previous while loop.

>>
>> > +   }
>> >
>> > -   while (sdhc_clk / (div * pre_div * ddr_pre_div) > clock && div < 
>> > 16)
>> > -   div++;
>> > +   for (div = 1; div <= 16; div++)

div is already initialized in its definition, so only one of these
initializations can be kept.

>> > +   if ((sdhc_clk / (div * pre_div)) <= clock)
>> > +   break;

The only difference with the previous while loop is the "<= 16", which
is correct, but maybe the change could have been limited to that.

>> >
>> > pre_div >>= 1;
>> > div -= 1;

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 37/41] mmc: fsl_esdhc: fix sd/mmc ddr mode clock setting issue

[Benoît Thébaudeau]

Dear Peng Fan,

On Mon, May 28, 2018 at 2:25 PM, Peng Fan  wrote:
> From: Ye Li 
>
> When sd/mmc work at DDR mode, like HS400/HS400ES/DDR52/DDR50 mode,
> the actual clock rate is just half of the expected clock.
>
> This patch set the DDR_EN bit first for DDR mode, hardware divide
> the usdhc clock automatically, then follow the original sdr clock
> setting method.
>
> Signed-off-by: Haibo Chen 
> Signed-off-by: Ye Li 
> ---
>  drivers/mmc/fsl_esdhc.c | 24 +++-
>  1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
> index 1b062ff06d..bf4ae74847 100644
> --- a/drivers/mmc/fsl_esdhc.c
> +++ b/drivers/mmc/fsl_esdhc.c
> @@ -583,18 +583,32 @@ static void set_sysctl(struct fsl_esdhc_priv *priv, 
> struct mmc *mmc, uint clock)
>  #else
> int pre_div = 2;
>  #endif
> -   int ddr_pre_div = mmc->ddr_mode ? 2 : 1;
> int sdhc_clk = priv->sdhc_clk;
> uint clk;
>
> +   /*
> +* For ddr mode, usdhc need to enable DDR mode first, after select
> +* this DDR mode, usdhc will automatically divide the usdhc clock
> +*/
> +   if (mmc->ddr_mode) {
> +   writel(readl(>mixctrl) | MIX_CTRL_DDREN, 
> >mixctrl);
> +   sdhc_clk >>= 1;
> +   }
> +
> if (clock < mmc->cfg->f_min)
> clock = mmc->cfg->f_min;
>
> -   while (sdhc_clk / (16 * pre_div * ddr_pre_div) > clock && pre_div < 
> 256)
> -   pre_div *= 2;
> +   if ((sdhc_clk / 16) > clock) {
> +   for (; pre_div < 256; pre_div *= 2)
> +   if ((sdhc_clk / pre_div) <= (clock * 16))
> +   break;
> +   } else {
> +   pre_div = 1;

This value is not always available for this divider. See the
conditions in the initialization of this variable giving its minimum
value.

> +   }
>
> -   while (sdhc_clk / (div * pre_div * ddr_pre_div) > clock && div < 16)
> -   div++;
> +   for (div = 1; div <= 16; div++)
> +   if ((sdhc_clk / (div * pre_div)) <= clock)
> +   break;
>
> pre_div >>= 1;
> div -= 1;

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH V3] imx: mx25: Remove SION bit in all pin-mux that are safe

[Benoît Thébaudeau]

On 25/01/2018 at 22:48, Benoît Thébaudeau wrote:
> On Thu, Jan 25, 2018 at 2:06 PM, Michael Trimarchi
> <mich...@amarulasolutions.com> wrote:
>> SION bit should be used in the situation that we need
>> to read back the value of a pin and should not be set by
>> default macro.
>>
>> We get some malfunction as raised by following thread
>>
>> https://www.spinics.net/lists/linux-usb/msg162574.html
>>
>> As reported by this application note:
>> https://www.nxp.com/docs/en/application-note/AN5078.pdf
>>
>> The software input on (SION) bit is an option to force an input
>> path to be active regardless of the value driven by the
>> corresponding module. It is used when the nature direction
>> of a pin depending on selected alternative function is an output,
>> but it is needed to read the real logic value on a pin.
>>
>> The SION bit can be used in:
>> • Loopback: the module of a selected alternative function drives
>> the pad and also receives the pad value as an input
>> • GPIO capture: the module of a selected alternative function
>> drives the pin and the value is captured by the GPIO
>>
>> SION bit is not necessary when the pin is configured as a peripheral
>> apart specific silicon bug. If an application needs to have this
>> set, this should be done in board file or in dts file
>>
>> Signed-off-by: Michael Trimarchi <mich...@amarulasolutions.com>
> 
> [...]
> 
> I have now tested the following peripherals on i.MX25 without any SION
> bit set: AUDMUX/SSI, eSDHC, FEC (except PHY access for which MDIO
> might have an issue without SION; Ethernet works but I have not
> checked whether this is possible at least partially without access to
> the PHY registers), GPIO, I2C, SLCDC, UART, and USB. Everything worked
> fine except the lack of SION for eSDHC CMD, which is true for all
> eSDHC instances (and not only for eSDHC1, so Linux's
> arch/arm/boot/dts/imx25-pinfunc.h should be fixed). Therefore, SION is
> mandatory for all the eSDHC CMD functions in
> arch/arm/include/asm/arch-mx25/iomux-mx25.h, but it can be removed for
> everything else (I still have to carry out one more test to confirm
> this for FEC MDIO), and moved to the board files if necessary (but I
> don't think that any mainline board code is doing something fancy
> enough to need it).

I confirm that FEC RMII register access works fine without setting SION for
MDIO.

Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH V3] imx: mx25: Remove SION bit in all pin-mux that are safe

[Benoît Thébaudeau]

Hi Michael,

On 26/01/2018 at 08:51, Michael Trimarchi wrote:
> On Thu, Jan 25, 2018 at 10:48:42PM +0100, Benoît Thébaudeau wrote:
>> On Thu, Jan 25, 2018 at 2:06 PM, Michael Trimarchi
>> <mich...@amarulasolutions.com> wrote:
>>> SION bit should be used in the situation that we need
>>> to read back the value of a pin and should not be set by
>>> default macro.
>>>
>>> We get some malfunction as raised by following thread
>>>
>>> https://www.spinics.net/lists/linux-usb/msg162574.html
>>>
>>> As reported by this application note:
>>> https://www.nxp.com/docs/en/application-note/AN5078.pdf
>>>
>>> The software input on (SION) bit is an option to force an input
>>> path to be active regardless of the value driven by the
>>> corresponding module. It is used when the nature direction
>>> of a pin depending on selected alternative function is an output,
>>> but it is needed to read the real logic value on a pin.
>>>
>>> The SION bit can be used in:
>>> • Loopback: the module of a selected alternative function drives
>>> the pad and also receives the pad value as an input
>>> • GPIO capture: the module of a selected alternative function
>>> drives the pin and the value is captured by the GPIO
>>>
>>> SION bit is not necessary when the pin is configured as a peripheral
>>> apart specific silicon bug. If an application needs to have this
>>> set, this should be done in board file or in dts file
>>>
>>> Signed-off-by: Michael Trimarchi <mich...@amarulasolutions.com>
>>
>> [...]
>>
>> I have now tested the following peripherals on i.MX25 without any SION
>> bit set: AUDMUX/SSI, eSDHC, FEC (except PHY access for which MDIO
>> might have an issue without SION; Ethernet works but I have not
>> checked whether this is possible at least partially without access to
>> the PHY registers), GPIO, I2C, SLCDC, UART, and USB. Everything worked
>> fine except the lack of SION for eSDHC CMD, which is true for all
>> eSDHC instances (and not only for eSDHC1, so Linux's
>> arch/arm/boot/dts/imx25-pinfunc.h should be fixed). Therefore, SION is
>> mandatory for all the eSDHC CMD functions in
> 
> I have checked and u-boot does not have this pin-muxing. I think that this
> patch should go as it is and a separate patch like this one following should
> be applied after that. What do you think?

Your patch is just fine as is. We do not need to add support for eSDHC2 in
U-Boot unless some mainline board needs it. But eSDHC2 CMD should have SION set
by default in Linux.

>> arch/arm/include/asm/arch-mx25/iomux-mx25.h, but it can be removed for
>> everything else (I still have to carry out one more test to confirm
>> this for FEC MDIO), and moved to the board files if necessary (but I
>> don't think that any mainline board code is doing something fancy
>> enough to need it).
>>
>> If the bus-status detection example in the documentation of SION in
>> the reference manual is useful, it means that SION is probably
>> required for all the signals requiring simultaneous output and input
>> (such as I²C for device clock stretching or multi-master bus
>> arbitration, except if the IP toggles between input and output low at
>> each clock edge rather than between open-drain output high and output
>> low), because there are no automatic SION signals between the
>> peripherals and the pads but only direction signals that can request a
>> single direction at a time. For bidirectional signals that do not
>> require simultaneous output and input because they work in turns (such
>> as FEC MDIO), SION can be required or not depending on whether the IP
>> toggles the direction signal for each turn or always expects an input
>> feedback while driving an open-drain output high. Even if SION is
>> required for the I²C example mentioned above (which is unlikely as
>> basic I²C transfers work fine and clock stretching detection is
>> automatic and would always need the input state), the need for these
>> advanced I²C features can be considered board-specific, so SION would
>> still not be required in iomux-mx25.h.
>>
>> In the end, for this patch, apart from the pending test for FEC MDIO:
>> Reviewed-by: Benoît Thébaudeau <benoit.thebaudeau@gmail.com>
>>
>> Best regards,
>> Benoît
> 
>>From d6b5bf4f1b6a0bcbcfd6ff2597fd2a7b3884cb3b Mon Sep 17 00:00:00 2001
> From: Michael Trimarchi <mich...@amarulasolutions.com>
> Date: Fri, 26 Jan 2018 08:42:54 +0100
> Subject: [PATCH] imx: mx25: Add pinmux def

Re: [U-Boot] [PATCH V3] imx: mx25: Remove SION bit in all pin-mux that are safe

[Benoît Thébaudeau]

Hi Michael,

On Thu, Jan 25, 2018 at 2:06 PM, Michael Trimarchi
<mich...@amarulasolutions.com> wrote:
> SION bit should be used in the situation that we need
> to read back the value of a pin and should not be set by
> default macro.
>
> We get some malfunction as raised by following thread
>
> https://www.spinics.net/lists/linux-usb/msg162574.html
>
> As reported by this application note:
> https://www.nxp.com/docs/en/application-note/AN5078.pdf
>
> The software input on (SION) bit is an option to force an input
> path to be active regardless of the value driven by the
> corresponding module. It is used when the nature direction
> of a pin depending on selected alternative function is an output,
> but it is needed to read the real logic value on a pin.
>
> The SION bit can be used in:
> • Loopback: the module of a selected alternative function drives
> the pad and also receives the pad value as an input
> • GPIO capture: the module of a selected alternative function
> drives the pin and the value is captured by the GPIO
>
> SION bit is not necessary when the pin is configured as a peripheral
> apart specific silicon bug. If an application needs to have this
> set, this should be done in board file or in dts file
>
> Signed-off-by: Michael Trimarchi <mich...@amarulasolutions.com>

[...]

I have now tested the following peripherals on i.MX25 without any SION
bit set: AUDMUX/SSI, eSDHC, FEC (except PHY access for which MDIO
might have an issue without SION; Ethernet works but I have not
checked whether this is possible at least partially without access to
the PHY registers), GPIO, I2C, SLCDC, UART, and USB. Everything worked
fine except the lack of SION for eSDHC CMD, which is true for all
eSDHC instances (and not only for eSDHC1, so Linux's
arch/arm/boot/dts/imx25-pinfunc.h should be fixed). Therefore, SION is
mandatory for all the eSDHC CMD functions in
arch/arm/include/asm/arch-mx25/iomux-mx25.h, but it can be removed for
everything else (I still have to carry out one more test to confirm
this for FEC MDIO), and moved to the board files if necessary (but I
don't think that any mainline board code is doing something fancy
enough to need it).

If the bus-status detection example in the documentation of SION in
the reference manual is useful, it means that SION is probably
required for all the signals requiring simultaneous output and input
(such as I²C for device clock stretching or multi-master bus
arbitration, except if the IP toggles between input and output low at
each clock edge rather than between open-drain output high and output
low), because there are no automatic SION signals between the
peripherals and the pads but only direction signals that can request a
single direction at a time. For bidirectional signals that do not
require simultaneous output and input because they work in turns (such
as FEC MDIO), SION can be required or not depending on whether the IP
toggles the direction signal for each turn or always expects an input
feedback while driving an open-drain output high. Even if SION is
required for the I²C example mentioned above (which is unlikely as
basic I²C transfers work fine and clock stretching detection is
automatic and would always need the input state), the need for these
advanced I²C features can be considered board-specific, so SION would
still not be required in iomux-mx25.h.

In the end, for this patch, apart from the pending test for FEC MDIO:
Reviewed-by: Benoît Thébaudeau <benoit.thebaudeau@gmail.com>

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] imx: mx25: Remove SION bit in all pin-mux

[Benoît Thébaudeau]

On 25/01/2018 at 11:17, Michael Nazzareno Trimarchi wrote:
> On Thu, Jan 25, 2018 at 11:02 AM, Benoît Thébaudeau <ben...@wsystem.com> 
> wrote:
>> You said that setting SION only for a UART is enough to trigger your USB 
>> issue.
>> Of course, there is no reason to set SION by default for a UART, but I was
>> thinking about a possible link between UART and USB, as this behavior is very
>> strange. Which USB host port are use using with the problematic pen drive, 
>> and
>> with which PHY (SoC-internal/external, bus)? Have you checked that this port 
>> is
>> properly configured for this PHY and PWR/OC (on/off + polarity) in both 
>> U-Boot
>> and Linux? For instance, if this port is configured to use OC but no OC 
>> signal
> 
> Nothing of above is connected. Pen drive is in the linux thread
> described in the commit
> message. All the usb stack is fully functional with a lot of pen
> drive. OC, PWR are not
> managed on USB/serial ehci port so they are not involved and I can
> check. I have posted
> some patches on linux mailing list to fix minor stuff. Let's say any
> permutation bit on phy
> configuration does not solve the problem. It's not a problem of usb
> suspend etc. I think that
> approch should be:
> 
> - align the pin mux mx25 file to the other architecture. So drop all
> the sion bit expect for the one
> where we know that silicon is buggy
> - apply SION when is needed in the board that are already in uboot
> - send patches on pin mux for board that are in mainline and we can test
> 
> On my side. I will restrict the change on my board to full isolate the
> configuration. Force the reset of SION
> bit anyway in linux if this solve.
> 
> Agree?

Looks good to me.

Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] imx: mx25: Remove SION bit in all pin-mux

[Benoît Thébaudeau]

Hi Michael,

On 25/01/2018 at 06:47, Michael Nazzareno Trimarchi wrote:
> On 25 Jan. 2018 12:07 am, "Fabio Estevam"  > wrote:
> 
> Hi Michael,
> 
> On Wed, Jan 24, 2018 at 3:46 PM, Michael Nazzareno Trimarchi
> > 
> wrote:
> 
> > This is exactly my initial propose. Can we give a try and manage on 
> board level?
> 
> The kernel should not rely on the IOMUX setting done by the bootloader.
> 
> Do you use 0x8000 in your dts IOMUX configuration by any chance?
> 
> 0x8000 means that the kernel will not do IOMUX configuration and
> will use the IOMUX value that comes from the bootloader.
> 
> 
> Yes but those should not be even wrong. We can not be sure if the state 
> machine of any logic as already corrupted. Remember that we have already this 
> problem with the clock in general that most of the time are already enabled 
> and so logic can be up.
> 
> 
> It seems you can fix your USB problem by not using the IOMUX value
> from the bootloader and just use the good IOMUX (without SION)
> explicitly in your dts.
> 
> Does it fix the problem?
> 
> 
> I think that the way to fix in a specific case could be more then one. I will 
> do the best on my side but I will include to not touch iomux without any 
> reason. I already point out that just with few pins configured like console I 
> get the problem . I can check two extra gpio too. 
> 
> To be clear, my board was "working". We are talking about a product in the 
> field since years with one minimal USB mulfuction . Other boards can have the 
> same problem but just not rise in the field. If the host port is direct 
> connected to the pen drive without an hub the USB reset can most of the time 
> recover the connection.

I agree with Fabio: Linux should not rely on the pad configurations performed by
U-Boot. But as you say, U-Boot should work fine itself too. Have you tested the
problematic USB pen drive with U-Boot?

Besides your USB issue, in order to optimize power consumption, iomux-mx25.h
should not set SION by default, except for the pad functions that can in no way
work without it (still to be identified/tested). For the other use cases, the
board files can set SION themselves, thanks to a NEW_PAD_CTRL()-like mechanism
(apparently yet to be introduced into U-Boot). The changes introduced here
should not break anything for the current in-tree boards.

You said that setting SION only for a UART is enough to trigger your USB issue.
Of course, there is no reason to set SION by default for a UART, but I was
thinking about a possible link between UART and USB, as this behavior is very
strange. Which USB host port are use using with the problematic pen drive, and
with which PHY (SoC-internal/external, bus)? Have you checked that this port is
properly configured for this PHY and PWR/OC (on/off + polarity) in both U-Boot
and Linux? For instance, if this port is configured to use OC but no OC signal
is actually wired, this can probably do weird things.

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] imx: mx25: Remove SION bit in all pin-mux

[Benoît Thébaudeau]

Hi Fabio,

On 24/01/2018 at 18:21, Fabio Estevam wrote:
> On Wed, Jan 24, 2018 at 3:12 PM, Michael Nazzareno Trimarchi
>  wrote:
> 
>> From the datasheet I have:
>>
>> Software Input On Field. Force the selected mux mode Input path no
>> matter of MUX_MODE functionality.
>> 1: Force input path of pad A14.
>> 0: Input Path is determined by functionality of the selected mux mode
>> (regular).
>>
>> So i think that in case of some peripheral this is not relevant but I
>> need confirmation
> 
> Looking at arch/arm/boot/dts/imx25-pinfunc.h I see that there is only
> one pin that sets the SION bit in the common pin definition:
> 
> /*
>  * Removing the SION bit from MX25_PAD_SD1_CMD__SD1_CMD breaks detecting an SD
>  * card. According to the i.MX25 reference manual (e.g. Figure 23-2 in IMX25RM
>  * Rev. 2 from 01/2011) this pin is bidirectional. So it seems to be a silicon
>  * bug that configuring the SD1_CMD function doesn't enable the input path for
>  * this pin.
>  * This might have side effects for other hardware units that are connected to
>  * that pin and use the respective function as input.
>  */
> #define MX25_PAD_SD1_CMD__SD1_CMD0x190 0x388 0x000 0x10 0x000

In mainline Linux, SION is also set by some DTS files (MX25_PAD_* 0x4000
flag) for FEC MDIO and SD CMD/CLK/DATAn.

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] imx: mx25: Remove SION bit in all pin-mux

[Benoît Thébaudeau]

On 24/01/2018 at 18:12, Michael Nazzareno Trimarchi wrote:
> On Wed, Jan 24, 2018 at 6:05 PM, Benoît Thébaudeau <ben...@wsystem.com> wrote:
>> Hi Michael,
>>
>> On 24/01/2018 at 17:46, Michael Nazzareno Trimarchi wrote:
>>> On Wed, Jan 24, 2018 at 5:38 PM, Fabio Estevam <feste...@gmail.com> wrote:
>>>> Hi Michael,
>>>>
>>>> [Removed usb list as this is off-topic for them]
>>>>
>>>> On Wed, Jan 24, 2018 at 2:31 PM, Michael Nazzareno Trimarchi
>>>> <mich...@amarulasolutions.com> wrote:
>>>>
>>>>> Can you check where SION bit is mandatory for mx25? I have on IMX51
>>>>> some PINMUX where sion is enabled.
>>>>> I can clean up a bit the patch to just minimize the change. We have for 
>>>>> example:
>>>>
>>>> I think it depends on the pin function per board design, no?
>>>>
>>>
>>> I don't have a pdk and it's difficult to find them so I need to check. 
>>> Anyway
>>> some revision of the firmware on this pendrive are working but the latest 
>>> one
>>> is not working. I have a lot of pen drives and all of them are fully 
>>> working.
>>>
>>>> Benoît brings a good point: what is the exact pin that causes the USB
>>>> failure on your case?
>>>
>>> I have tested only with uart and already it does not let it work
>>> properly. So I can try
>>> to limit in my boot case but I don't think that having SION bit
>>> pre-defined it's valid
>>> at all. We have a limited number of pin mux with SION enable on any nxp
>>> architecture right now for every architecture.
>>>
>>> * Benoit * can you test them? I will provide a better changeset let
>>> sdcard and some ethernet pin
>>> in the right configuation (even ethernet was working without SION enable)
>>
>> Not sure I can find time for that. Perhaps in a few days.
>>
> 
>>From the datasheet I have:
> 
> Software Input On Field. Force the selected mux mode Input path no
> matter of MUX_MODE functionality.
> 1: Force input path of pad A14.
> 0: Input Path is determined by functionality of the selected mux mode
> (regular).
> 
> So i think that in case of some peripheral this is not relevant but I
> need confirmation

I think I remember people on the U-Boot mailing list having had issues after
having dropped SION for some bidirectional alternate functions on some i.MXs
(maybe I²C SDA/SCL on i.MX51, which is very close to i.MX25). It is not possible
to assume that all peripherals drive the input circuitry properly without SION
for bidirectional signals.

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] imx: mx25: Remove SION bit in all pin-mux

[Benoît Thébaudeau]

Hi Michael,

On 24/01/2018 at 17:46, Michael Nazzareno Trimarchi wrote:
> On Wed, Jan 24, 2018 at 5:38 PM, Fabio Estevam  wrote:
>> Hi Michael,
>>
>> [Removed usb list as this is off-topic for them]
>>
>> On Wed, Jan 24, 2018 at 2:31 PM, Michael Nazzareno Trimarchi
>>  wrote:
>>
>>> Can you check where SION bit is mandatory for mx25? I have on IMX51
>>> some PINMUX where sion is enabled.
>>> I can clean up a bit the patch to just minimize the change. We have for 
>>> example:
>>
>> I think it depends on the pin function per board design, no?
>>
> 
> I don't have a pdk and it's difficult to find them so I need to check. Anyway
> some revision of the firmware on this pendrive are working but the latest one
> is not working. I have a lot of pen drives and all of them are fully working.
> 
>> Benoît brings a good point: what is the exact pin that causes the USB
>> failure on your case?
> 
> I have tested only with uart and already it does not let it work
> properly. So I can try
> to limit in my boot case but I don't think that having SION bit
> pre-defined it's valid
> at all. We have a limited number of pin mux with SION enable on any nxp
> architecture right now for every architecture.
> 
> * Benoit * can you test them? I will provide a better changeset let
> sdcard and some ethernet pin
> in the right configuation (even ethernet was working without SION enable)

Not sure I can find time for that. Perhaps in a few days.

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] imx: mx25: Remove SION bit in all pin-mux

[Benoît Thébaudeau]

Hi,

On 24/01/2018 at 17:17, Michael Nazzareno Trimarchi wrote:
> On Wed, Jan 24, 2018 at 5:03 PM, Benoît Thébaudeau <ben...@wsystem.com> wrote:
>> On 24/01/2018 at 16:43, Michael Nazzareno Trimarchi wrote:
>>> On Wed, Jan 24, 2018 at 4:39 PM, Benoît Thébaudeau <ben...@wsystem.com> 
>>> wrote:
>>>> On 24/01/2018 at 16:34, Benoît Thébaudeau wrote:
>>>>> On 24/01/2018 at 16:26, Michael Nazzareno Trimarchi wrote:
>>>>>> On Wed, Jan 24, 2018 at 4:14 PM, Fabio Estevam <feste...@gmail.com> 
>>>>>> wrote:
>>>>>>> On Wed, Jan 24, 2018 at 12:56 PM, Michael Trimarchi
>>>>>>> <mich...@amarulasolutions.com> wrote:
>>>>>>>> SION bit should be used in the situation that we need
>>>>>>>> to read back the value of a pin and should be set by
>>>>>>>> default.
>>>>>
>>>>> You remove this bit because it should be set by default? This sentence is
>>>>> confusing.
>>>
>>> English is wrong ;)
>>>
>>> SION bit as a specific purpose to read back value that is set in
>>> output. You don't need
>>> and it's not set in any freescale board. If you need to set you need
>>> to add to your peripheral.
>>
>> Unless there is a NEW_PAD_CTRL()-like mechanism for SION, all these 
>> definitions
>> should be kept in iomux-mx25.h in order not to redefine the register offsets
>> everywhere. AFAIK, all the Freescale boards use the definitions from
>> iomux-mx25.h too.
>>
>>> The only case you need maybe is the data[0] of sdcard.
>>
>> And eSDHC CMD, and I²C probably too. Yet, you are also removing SION in these
>> cases. I have 3 i.M25-based boards working fine with SION. ;) Can you explain
>> the precise issue that you are trying to fix (which pin)?
>>
> 
> Let me summarize for you:
> - was having a board with linux 2.6.x and uboot from 2009 working fine
> on a usb pen driver (look on thread in linux-usb)
> - was having the same board with any version of linux from 3.18 to
> 4.15 and fail with this pen drive
> - check back all the changes from linux 2.6.x  to linux 4.15.x and
> compare every single register and all the usb code and was just
> confirm a better implementation of new kernel.
>   but with a result of a usb stuck on the host port
> - swap the boot-loader and having a working board
> - go in deep in boot-loader and compare everything
> - Understand the difference was the SION bit that was enable on all the mux
> 
> In general when a board start from reset it has default pin muxing.
> Each peripheral need to setup the pin muxing according to the real
> usage.
> SION by default is not the right way to do it. What is the concept of
> working board in your side? Just pass your testcase? Ok even this
> board
> was passing all test cases apart this usb pen drive. We was having in
> the field some customer with usb issue time to time and only this
> proof that somenthing was not real ok.

Thanks for these details. All the test cases should of course work. I'm just
trying to figure out the root cause of your issue, and maybe it's not SION
itself but a power issue triggered by SION, or maybe it's SION only for some
specific pads and not for all the pads, or something else. So what I'm saying is
that this change might be too large, and care should be taken.

Some Freescale boards actually do use SION. E.g., see
MX25_PAD_FEC_MDIO__FEC_MDIO in
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/imx25-pdk.dts?h=v4.15-rc9
 . SION is required to read back the actual state of a GPIO
in output mode. It is also required for some bidirectional alternate functions
(such as I²C SDA/SCL, SD CMD/DATn, etc.) on some i.MXs (not all i.MXs behave in
the same way for the same peripheral), but which ones are affected is not always
documented, so please double check and test all these cases.

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] imx: mx25: Remove SION bit in all pin-mux

[Benoît Thébaudeau]

On 24/01/2018 at 16:43, Michael Nazzareno Trimarchi wrote:
> On Wed, Jan 24, 2018 at 4:39 PM, Benoît Thébaudeau <ben...@wsystem.com> wrote:
>> On 24/01/2018 at 16:34, Benoît Thébaudeau wrote:
>>> On 24/01/2018 at 16:26, Michael Nazzareno Trimarchi wrote:
>>>> On Wed, Jan 24, 2018 at 4:14 PM, Fabio Estevam <feste...@gmail.com> wrote:
>>>>> On Wed, Jan 24, 2018 at 12:56 PM, Michael Trimarchi
>>>>> <mich...@amarulasolutions.com> wrote:
>>>>>> SION bit should be used in the situation that we need
>>>>>> to read back the value of a pin and should be set by
>>>>>> default.
>>>
>>> You remove this bit because it should be set by default? This sentence is
>>> confusing.
> 
> English is wrong ;)
> 
> SION bit as a specific purpose to read back value that is set in
> output. You don't need
> and it's not set in any freescale board. If you need to set you need
> to add to your peripheral.

Unless there is a NEW_PAD_CTRL()-like mechanism for SION, all these definitions
should be kept in iomux-mx25.h in order not to redefine the register offsets
everywhere. AFAIK, all the Freescale boards use the definitions from
iomux-mx25.h too.

> The only case you need maybe is the data[0] of sdcard.

And eSDHC CMD, and I²C probably too. Yet, you are also removing SION in these
cases. I have 3 i.M25-based boards working fine with SION. ;) Can you explain
the precise issue that you are trying to fix (which pin)?

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] imx: mx25: Remove SION bit in all pin-mux

[Benoît Thébaudeau]

On 24/01/2018 at 16:34, Benoît Thébaudeau wrote:
> On 24/01/2018 at 16:26, Michael Nazzareno Trimarchi wrote:
>> On Wed, Jan 24, 2018 at 4:14 PM, Fabio Estevam <feste...@gmail.com> wrote:
>>> On Wed, Jan 24, 2018 at 12:56 PM, Michael Trimarchi
>>> <mich...@amarulasolutions.com> wrote:
>>>> SION bit should be used in the situation that we need
>>>> to read back the value of a pin and should be set by
>>>> default.
> 
> You remove this bit because it should be set by default? This sentence is
> confusing.
> 
>>>> This can generate any kind of random malfunction
>>>> as described in this thread.
>>>>
>>>> According to this thread:
>>>> https://www.spinics.net/lists/linux-usb/msg162574.html
> 
> I can't find details about SION on a specific pin in this thread. Please
> elaborate.
> 
>>>> We consider this an early bug so all the boards running imx25
>>>> with a minimimal set of functionalities can be affected.
>>>>
>>>> As reported by this application note:
>>>> https://www.nxp.com/docs/en/application-note/AN5078.pdf
>>>>
>>>> The software input on (SION) bit is an option to force an input
>>>> path to be active regardless of the value driven by the
>>>> corresponding module. It is used when the nature direction
>>>> of a pin depending on selected alternative function is an output,
>>>> but it is needed to read the real logic value on a pin.
>>>>
>>>> The SION bit can be used in:
>>>> • Loopback: the module of a selected alternative function drives
>>>> the pad and also receives the pad value as an input
>>>> • GPIO capture: the module of a selected alternative function
>>>> drives the pin and the value is captured by the GPIO
>>>>
>>>> Signed-off-by: Michael Trimarchi <mich...@amarulasolutions.com>
>>>> ---
>>>> Refer-to:
>>>> MX25 USB timeout on ID 0951:1665 Kingston Technology Digital
>>>> DataTraveler SE9 64GB
>>>
>>> Glad you found a fix for the issue!
>>>
>>
>> The idea was to align to the other freescale architecture. I can
>> create two patches on it
>>
>> Michael
>>
>>>
>>>> ---
>>>>  arch/arm/include/asm/arch-mx25/iomux-mx25.h | 680 
>>>> ++--
>>>>  1 file changed, 340 insertions(+), 340 deletions(-)
>>>>
>>>> diff --git a/arch/arm/include/asm/arch-mx25/iomux-mx25.h 
>>>> b/arch/arm/include/asm/arch-mx25/iomux-mx25.h
>>>> index 5b2863e..2fcaf60 100644
>>>> --- a/arch/arm/include/asm/arch-mx25/iomux-mx25.h
>>>> +++ b/arch/arm/include/asm/arch-mx25/iomux-mx25.h
>>>> @@ -30,481 +30,481 @@
>>>>
>>>>  /* PADMUX
>>>> ALT INPSE PATH PADCTRL */
>>>>  enum {
>>>> -   MX25_PAD_A10__A10   = IOMUX_PAD(0x000, 0x008, 
>>>> 0x00, 0, 0, NO_PAD_CTRL),
>>>> -   MX25_PAD_A10__GPIO_4_0  = IOMUX_PAD(0x000, 0x008, 
>>>> 0x05, 0, 0, NO_PAD_CTRL),
>>>> +   MX25_PAD_A10__A10   = IOMUX_PAD(0x000, 0x008, 
>>>> 0, 0, 0, NO_PAD_CTRL),
>>>> +   MX25_PAD_A10__GPIO_4_0  = IOMUX_PAD(0x000, 0x008, 
>>>> 5, 0, 0, NO_PAD_CTRL),
>>>
>>> In many places in this patch you are only changing things like 0x00
>>> --> 0 or 0x05--> 5, which just makes it harder to review.
>>>
>>> Please send a new version that only removes the SION bit.
> 
> Please make sure that these changes are not breaking anything for any board
> (NEW_PAD_CTRL() can be used for these boards to restore SION if it really has 
> to
> be removed by default). SION is required in some cases, and it's unlikely to 
> be
> harmful, its main side effect being an increased power consumption.

Actually, NEW_PAD_CTRL() cannot act upon SION, so removing it by default is all
the more risky.

Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] imx: mx25: Remove SION bit in all pin-mux

[Benoît Thébaudeau]

Hi Michael,

On 24/01/2018 at 16:26, Michael Nazzareno Trimarchi wrote:
> On Wed, Jan 24, 2018 at 4:14 PM, Fabio Estevam  wrote:
>> On Wed, Jan 24, 2018 at 12:56 PM, Michael Trimarchi
>>  wrote:
>>> SION bit should be used in the situation that we need
>>> to read back the value of a pin and should be set by
>>> default.

You remove this bit because it should be set by default? This sentence is
confusing.

>>> This can generate any kind of random malfunction
>>> as described in this thread.
>>>
>>> According to this thread:
>>> https://www.spinics.net/lists/linux-usb/msg162574.html

I can't find details about SION on a specific pin in this thread. Please
elaborate.

>>> We consider this an early bug so all the boards running imx25
>>> with a minimimal set of functionalities can be affected.
>>>
>>> As reported by this application note:
>>> https://www.nxp.com/docs/en/application-note/AN5078.pdf
>>>
>>> The software input on (SION) bit is an option to force an input
>>> path to be active regardless of the value driven by the
>>> corresponding module. It is used when the nature direction
>>> of a pin depending on selected alternative function is an output,
>>> but it is needed to read the real logic value on a pin.
>>>
>>> The SION bit can be used in:
>>> • Loopback: the module of a selected alternative function drives
>>> the pad and also receives the pad value as an input
>>> • GPIO capture: the module of a selected alternative function
>>> drives the pin and the value is captured by the GPIO
>>>
>>> Signed-off-by: Michael Trimarchi 
>>> ---
>>> Refer-to:
>>> MX25 USB timeout on ID 0951:1665 Kingston Technology Digital
>>> DataTraveler SE9 64GB
>>
>> Glad you found a fix for the issue!
>>
> 
> The idea was to align to the other freescale architecture. I can
> create two patches on it
> 
> Michael
> 
>>
>>> ---
>>>  arch/arm/include/asm/arch-mx25/iomux-mx25.h | 680 
>>> ++--
>>>  1 file changed, 340 insertions(+), 340 deletions(-)
>>>
>>> diff --git a/arch/arm/include/asm/arch-mx25/iomux-mx25.h 
>>> b/arch/arm/include/asm/arch-mx25/iomux-mx25.h
>>> index 5b2863e..2fcaf60 100644
>>> --- a/arch/arm/include/asm/arch-mx25/iomux-mx25.h
>>> +++ b/arch/arm/include/asm/arch-mx25/iomux-mx25.h
>>> @@ -30,481 +30,481 @@
>>>
>>>  /* PADMUX
>>> ALT INPSE PATH PADCTRL */
>>>  enum {
>>> -   MX25_PAD_A10__A10   = IOMUX_PAD(0x000, 0x008, 
>>> 0x00, 0, 0, NO_PAD_CTRL),
>>> -   MX25_PAD_A10__GPIO_4_0  = IOMUX_PAD(0x000, 0x008, 
>>> 0x05, 0, 0, NO_PAD_CTRL),
>>> +   MX25_PAD_A10__A10   = IOMUX_PAD(0x000, 0x008, 
>>> 0, 0, 0, NO_PAD_CTRL),
>>> +   MX25_PAD_A10__GPIO_4_0  = IOMUX_PAD(0x000, 0x008, 
>>> 5, 0, 0, NO_PAD_CTRL),
>>
>> In many places in this patch you are only changing things like 0x00
>> --> 0 or 0x05--> 5, which just makes it harder to review.
>>
>> Please send a new version that only removes the SION bit.

Please make sure that these changes are not breaking anything for any board
(NEW_PAD_CTRL() can be used for these boards to restore SION if it really has to
be removed by default). SION is required in some cases, and it's unlikely to be
harmful, its main side effect being an increased power consumption.

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH v2] mmc: fsl_esdhc: Fix i.MX53 eSDHCv3 clock

[Benoît Thébaudeau]

Commit 4f425280fa71 ("mmc: fsl_esdhc: Allow all supported prescaler
values") made it possible to set SYSCTL.SDCLKFS to 0 in SDR mode on
i.MX, thus bypassing the SD clock frequency prescaler, in order to be
able to get higher SD clock frequencies in some contexts. However, that
commit missed the fact that this value is illegal on the eSDHCv3
instance of the i.MX53. This seems to be the only exception on i.MX,
this value being legal even for the eSDHCv2 instances of the i.MX53.

Fix this issue by changing the minimum prescaler value for the single
instance of the i.MX53 eSDHCv3 controller.

Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@gmail.com>
Reviewed-by: Fabio Estevam <fabio.este...@nxp.com>
---
Changes for v2:
 - Surround == with parentheses for clarity (suggested by Stefano).
---
 drivers/mmc/fsl_esdhc.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
index 499d622c6d..5b6042c22c 100644
--- a/drivers/mmc/fsl_esdhc.c
+++ b/drivers/mmc/fsl_esdhc.c
@@ -528,14 +528,19 @@ out:
 
 static void set_sysctl(struct fsl_esdhc_priv *priv, struct mmc *mmc, uint 
clock)
 {
+   struct fsl_esdhc *regs = priv->esdhc_regs;
int div = 1;
 #ifdef ARCH_MXC
+#ifdef CONFIG_MX53
+   /* For i.MX53 eSDHCv3, SYSCTL.SDCLKFS may not be set to 0. */
+   int pre_div = (regs == (struct fsl_esdhc *)MMC_SDHC3_BASE_ADDR) ? 2 : 1;
+#else
int pre_div = 1;
+#endif
 #else
int pre_div = 2;
 #endif
int ddr_pre_div = mmc->ddr_mode ? 2 : 1;
-   struct fsl_esdhc *regs = priv->esdhc_regs;
int sdhc_clk = priv->sdhc_clk;
uint clk;
 
-- 
2.14.1

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] mmc: fsl_esdhc: Fix i.MX53 eSDHCv3 clock

[Benoît Thébaudeau]

Hi Stefano,

On Mon, Jan 15, 2018 at 11:59 AM, Stefano Babic <sba...@denx.de> wrote:
> On 15/01/2018 00:46, Benoît Thébaudeau wrote:
>> + int pre_div = regs == (struct fsl_esdhc *)MMC_SDHC3_BASE_ADDR ? 2 : 1;
>
> It is surely a question of taste - but is it not cleared to surround the
> expression with parenthesis ? (regs ==  )

I can send a v2 for that if you want. Which of the following do you
prefer (personally, I'd say the last one)?

int pre_div = (regs == (struct fsl_esdhc *)MMC_SDHC3_BASE_ADDR ? 2 : 1);
int pre_div = (regs == (struct fsl_esdhc *)MMC_SDHC3_BASE_ADDR) ? 2 : 1;

Note that this patch can be tested with the 2nd MMC on i.MX53 Loco if
anyone has this board. Wladimir?

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH] mmc: fsl_esdhc: Fix i.MX53 eSDHCv3 clock

[Benoît Thébaudeau]

Commit 4f425280fa71 ("mmc: fsl_esdhc: Allow all supported prescaler
values") made it possible to set SYSCTL.SDCLKFS to 0 in SDR mode on
i.MX, thus bypassing the SD clock frequency prescaler, in order to be
able to get higher SD clock frequencies in some contexts. However, that
commit missed the fact that this value is illegal on the eSDHCv3
instance of the i.MX53. This seems to be the only exception on i.MX,
this value being legal even for the eSDHCv2 instances of the i.MX53.

Fix this issue by changing the minimum prescaler value for the single
instance of the i.MX53 eSDHCv3 controller.

Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@gmail.com>
---
 drivers/mmc/fsl_esdhc.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
index 499d622c6d..90425e8a30 100644
--- a/drivers/mmc/fsl_esdhc.c
+++ b/drivers/mmc/fsl_esdhc.c
@@ -528,14 +528,19 @@ out:
 
 static void set_sysctl(struct fsl_esdhc_priv *priv, struct mmc *mmc, uint 
clock)
 {
+   struct fsl_esdhc *regs = priv->esdhc_regs;
int div = 1;
 #ifdef ARCH_MXC
+#ifdef CONFIG_MX53
+   /* For i.MX53 eSDHCv3, SYSCTL.SDCLKFS may not be set to 0. */
+   int pre_div = regs == (struct fsl_esdhc *)MMC_SDHC3_BASE_ADDR ? 2 : 1;
+#else
int pre_div = 1;
+#endif
 #else
int pre_div = 2;
 #endif
int ddr_pre_div = mmc->ddr_mode ? 2 : 1;
-   struct fsl_esdhc *regs = priv->esdhc_regs;
int sdhc_clk = priv->sdhc_clk;
uint clk;
 
-- 
2.14.1

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v3 1/3] mx25: Move MX25 selection to Kconfig

[Benoît Thébaudeau]

Hi Stefano, Tom,

On Tue, Nov 7, 2017 at 10:10 AM, Stefano Babic  wrote:
> On 07/11/2017 02:22, Tom Rini wrote:
>> On Mon, Nov 06, 2017 at 09:44:40PM -0200, Fabio Estevam wrote:
>>> Hi Stefano and Tom,
>>>
>>> On Fri, Nov 3, 2017 at 1:40 PM, Fabio Estevam  wrote:
 From: Fabio Estevam 

 The motivation for moving MX25 selection to Kconfig is to be
 able to better handle MX25 specific errata, so that an errata option
 can be selected at SoC level instead of board level.

 This selection method also aligns with the way other i.MX SoCs are
 selected in U-Boot.

 Signed-off-by: Fabio Estevam 
 Acked-by: Sebastien Bourdelin 
>>>
>>> Could you please consider this series (bug fix) for 2017.11?
>>
>> Otavio brought this up to me as well today, I would be happy to see this
>> come in as a bugfix.  Thanks!
>>
>
> I merge into u-boot-imx and I'll send my PR - thanks !

I have seen other PRs going through, but nothing for this series.
Please do not forget.

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [RFC] mmc: fsl_esdhc: Do not set high speed mode on MX25 and MX51

[Benoît Thébaudeau]

Hi Fabio,

On Fri, Nov 3, 2017 at 4:17 PM, Fabio Estevam <feste...@gmail.com> wrote:
> On Sun, Oct 29, 2017 at 7:18 PM, Benoît Thébaudeau
> <benoit.thebaudeau@gmail.com> wrote:
>
>> Of course we can, but CONFIG_SYS_FSL_ERRATUM_ESDHC_A001 seems to be
>> the most optimal approach here, so I'll wait for the answer from your
>> design team before doing anything.
>
> I received the confirmation that ESDHC_A001 also affects mx25 and mx51
> and set a patch series to select this erratum on these SoCs.

Thanks. If NXP could update the corresponding errata sheets, that
would be great.

> Thanks a lot for your help in debugging this problem. Really appreciated!

You're welcome.

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v3 1/3] mx25: Move MX25 selection to Kconfig

[Benoît Thébaudeau]

On Fri, Nov 3, 2017 at 4:40 PM, Fabio Estevam <feste...@gmail.com> wrote:
> From: Fabio Estevam <fabio.este...@nxp.com>
>
> The motivation for moving MX25 selection to Kconfig is to be
> able to better handle MX25 specific errata, so that an errata option
> can be selected at SoC level instead of board level.
>
> This selection method also aligns with the way other i.MX SoCs are
> selected in U-Boot.
>
> Signed-off-by: Fabio Estevam <fabio.este...@nxp.com>
> Acked-by: Sebastien Bourdelin <sebastien.bourde...@savoirfairelinux.com>

[...]

For the series:
Reviewed-by: Benoît Thébaudeau <benoit.thebaudeau@gmail.com>
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [RFC] mmc: fsl_esdhc: Do not set high speed mode on MX25 and MX51

[Benoît Thébaudeau]

Hi Fabio,

On Tue, Oct 24, 2017 at 10:50 AM, Fabio Estevam <feste...@gmail.com> wrote:
> On Mon, Oct 23, 2017 at 8:45 PM, Benoît Thébaudeau
> <benoit.thebaudeau@gmail.com> wrote:
>
>> The issue is the timeout in esdhc_setup_data() on line 309. If I
>> revert e978a31b and fb823981, then everything works fine. However, the
>> latest calculation is correct, so reverting these commits is not the
>> fix. Indeed, for HS transfers:
>> mmc->tran_speed = 5000, fls(mmc->tran_speed/4) = 24
>> mmc->clock = 4800, fls(mmc->clock/2) = 25
>> I've tested with 26 and 27, and they work fine. Therefore,
>> SYSCTL.DTOCV is broken for 25 - 13 = 12 here, so the i.MX25 has an
>> undocumented erratum, which could just happen to be
>> CONFIG_SYS_FSL_ERRATUM_ESDHC_A001. If it's not exactly this erratum,
>> then a new workaround could be implemented for it, or
>> ESDHCI_QUIRK_BROKEN_TIMEOUT_VALUE could be used instead. Can you ask
>> your design team to confirm this?
>
> I can try to ask it internally when I am back to the office next week.
>
>> Shouldn't CONFIG_SYS_FSL_ERRATUM_ESDHC_A001 be changed into a quirk to
>> be defined in a SoC header file (such as
>> arch/arm/include/asm/arch-mx25/imx-regs.h) or in
>> drivers/mmc/fsl_esdhc.c if an affected SoC or IP version is detected
>> from the configuration, rather than keeping it as a configuration
>> setting to be defined in a board configuration file (such as
>> include/configs/mx25pdk.h)?
>>
>> Note that it would be possible to go further than fb823981 to optimize
>> this timeout because mmc->clock is also not always the actual SD clock
>> frequency resulting from set_sysctl(). However, timeout cases should
>> normally never occur, so optimizing them is a bit pointless.
>>
>> Note that mainline Linux always uses the maximum timeout regardless of
>> the SD clock frequency.
>
> Can we also do like this in U-Boot for now?

Of course we can, but CONFIG_SYS_FSL_ERRATUM_ESDHC_A001 seems to be
the most optimal approach here, so I'll wait for the answer from your
design team before doing anything.

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH] mmc: fsl_esdhc: Fix PIO timeout

[Benoît Thébaudeau]

The following error has been observed on i.MX25 with a high-speed SDSC
card:
Data Write Failed in PIO Mode.

It was caused by the timeout set on PRSSTAT.BWEN, which was triggered
because this bit takes 15 ms to be set after writing the first block to
DATPORT with this card. Without this timeout, all the blocks are
properly written.

This timeout was implemented by decrementing a variable, so it was
depending on the CPU frequency. Fix this issue by setting this timeout
to a long enough absolute duration (500 ms).

Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@gmail.com>
Cc: Stefano Babic <sba...@denx.de>
Cc: Fabio Estevam <fabio.este...@nxp.com>
Cc: Jaehoon Chung <jh80.ch...@samsung.com>
---
 drivers/mmc/fsl_esdhc.c | 26 +-
 include/fsl_esdhc.h |  2 +-
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
index cc188c4260..499d622c6d 100644
--- a/drivers/mmc/fsl_esdhc.c
+++ b/drivers/mmc/fsl_esdhc.c
@@ -171,20 +171,20 @@ static void esdhc_pio_read_write(struct fsl_esdhc_priv 
*priv,
uint databuf;
uint size;
uint irqstat;
-   uint timeout;
+   ulong start;
 
if (data->flags & MMC_DATA_READ) {
blocks = data->blocks;
buffer = data->dest;
while (blocks) {
-   timeout = PIO_TIMEOUT;
+   start = get_timer(0);
size = data->blocksize;
irqstat = esdhc_read32(>irqstat);
-   while (!(esdhc_read32(>prsstat) & PRSSTAT_BREN)
-   && --timeout);
-   if (timeout <= 0) {
-   printf("\nData Read Failed in PIO Mode.");
-   return;
+   while (!(esdhc_read32(>prsstat) & PRSSTAT_BREN)) {
+   if (get_timer(start) > PIO_TIMEOUT) {
+   printf("\nData Read Failed in PIO 
Mode.");
+   return;
+   }
}
while (size && (!(irqstat & IRQSTAT_TC))) {
udelay(100); /* Wait before last byte transfer 
complete */
@@ -200,14 +200,14 @@ static void esdhc_pio_read_write(struct fsl_esdhc_priv 
*priv,
blocks = data->blocks;
buffer = (char *)data->src;
while (blocks) {
-   timeout = PIO_TIMEOUT;
+   start = get_timer(0);
size = data->blocksize;
irqstat = esdhc_read32(>irqstat);
-   while (!(esdhc_read32(>prsstat) & PRSSTAT_BWEN)
-   && --timeout);
-   if (timeout <= 0) {
-   printf("\nData Write Failed in PIO Mode.");
-   return;
+   while (!(esdhc_read32(>prsstat) & PRSSTAT_BWEN)) {
+   if (get_timer(start) > PIO_TIMEOUT) {
+   printf("\nData Write Failed in PIO 
Mode.");
+   return;
+   }
}
while (size && (!(irqstat & IRQSTAT_TC))) {
udelay(100); /* Wait before last byte transfer 
complete */
diff --git a/include/fsl_esdhc.h b/include/fsl_esdhc.h
index 02b362d5e3..de1f5e7d9f 100644
--- a/include/fsl_esdhc.h
+++ b/include/fsl_esdhc.h
@@ -130,7 +130,7 @@
 #define XFERTYP_DMAEN  0x0001
 
 #define CINS_TIMEOUT   1000
-#define PIO_TIMEOUT10
+#define PIO_TIMEOUT500
 
 #define DSADDR 0x2e004
 
-- 
2.14.1

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [RFC] mmc: fsl_esdhc: Do not set high speed mode on MX25 and MX51

[Benoît Thébaudeau]

Hi Fabio,

On Sat, Oct 21, 2017 at 2:38 PM, Fabio Estevam <feste...@gmail.com> wrote:
> On Sat, Oct 21, 2017 at 10:34 AM, Benoît Thébaudeau
> <benoit.thebaudeau@gmail.com> wrote:
>
>> I already have a mainline version working at HS with changes only in
>> fsl_esdhc.c (apart from the port of my board). I still have to narrow
>> these changes down to the issue.
>
> Ok, great!

The issue is the timeout in esdhc_setup_data() on line 309. If I
revert e978a31b and fb823981, then everything works fine. However, the
latest calculation is correct, so reverting these commits is not the
fix. Indeed, for HS transfers:
mmc->tran_speed = 5000, fls(mmc->tran_speed/4) = 24
mmc->clock = 4800, fls(mmc->clock/2) = 25
I've tested with 26 and 27, and they work fine. Therefore,
SYSCTL.DTOCV is broken for 25 - 13 = 12 here, so the i.MX25 has an
undocumented erratum, which could just happen to be
CONFIG_SYS_FSL_ERRATUM_ESDHC_A001. If it's not exactly this erratum,
then a new workaround could be implemented for it, or
ESDHCI_QUIRK_BROKEN_TIMEOUT_VALUE could be used instead. Can you ask
your design team to confirm this?

Shouldn't CONFIG_SYS_FSL_ERRATUM_ESDHC_A001 be changed into a quirk to
be defined in a SoC header file (such as
arch/arm/include/asm/arch-mx25/imx-regs.h) or in
drivers/mmc/fsl_esdhc.c if an affected SoC or IP version is detected
from the configuration, rather than keeping it as a configuration
setting to be defined in a board configuration file (such as
include/configs/mx25pdk.h)?

Note that it would be possible to go further than fb823981 to optimize
this timeout because mmc->clock is also not always the actual SD clock
frequency resulting from set_sysctl(). However, timeout cases should
normally never occur, so optimizing them is a bit pointless.

Note that mainline Linux always uses the maximum timeout regardless of
the SD clock frequency.

I've also noticed that the PIO mode fails (even with the commits above
reverted) for write accesses (read accesses work fine) with "Data
Write Failed in PIO Mode.". I have not investigated this issue yet.
This is also related to a timeout, but the root cause could be
somewhere else, e.g. in the changes introduced in esdhc_setup_data()
for the PIO mode.

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [RFC] mmc: fsl_esdhc: Do not set high speed mode on MX25 and MX51

[Benoît Thébaudeau]

Hi Fabio,

On Fri, Oct 20, 2017 at 8:40 PM, Fabio Estevam <feste...@gmail.com> wrote:
> On Fri, Oct 20, 2017 at 10:40 AM, Benoît Thébaudeau <ben...@wsystem.com> 
> wrote:
>
>> With mainline U-Boot on my board, normal-speed SD cards work fine, but not HS
>> ones. Both types of cards work fine at 48 MHz with my custom and older 
>> U-Boot.
>
> Ok, great! What is the version of your old U-Boot?

2012.07. I know, _very_ old.

About the drive strength, the CSI pads used on my board for eSDHC2
have their drive strength set to nominal after POR, and I had to set
it to high in order to make HS work, whereas the SD1 pads used on
mx25pdk for eSDHC1 already have their default drive strength set to
high, so setting it to max as I previously suggested should not be
needed, unless the routing of the board requires it and there are also
errors in Linux that are just silently recovered.

>> The main difference seems to be the management of SD timeouts. I will try to
>> track the differences until I find the root cause. The test results in PIO 
>> mode
>> might also give some clues.
>
> Thanks for the investigation!

I already have a mainline version working at HS with changes only in
fsl_esdhc.c (apart from the port of my board). I still have to narrow
these changes down to the issue.

Is it mainline Linux that you have tested?

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [RFC] mmc: fsl_esdhc: Do not set high speed mode on MX25 and MX51

[Benoît Thébaudeau]

Hi Fabio,

On 19/10/2017 at 13:57, Fabio Estevam wrote:
> 
> I would be interested to see if you can get an SD card high speed to
> work with mainline U-Boot on your board.
> 
> On my tests I need to force it 25MHz operation to be able to use the SD card.

With mainline U-Boot on my board, normal-speed SD cards work fine, but not HS
ones. Both types of cards work fine at 48 MHz with my custom and older U-Boot.

The main difference seems to be the management of SD timeouts. I will try to
track the differences until I find the root cause. The test results in PIO mode
might also give some clues.

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [RFC] mmc: fsl_esdhc: Do not set high speed mode on MX25 and MX51

[Benoît Thébaudeau]

On 19/10/2017 at 14:52, Fabio Estevam wrote:
> On Thu, Oct 19, 2017 at 10:46 AM, Otavio Salvador
>  wrote:
> 
>> I think the original RFC is better as workaround as it solves the
>> issue for other boards. This does not mean we shouldn't fix the root
>> cause ...
> 
> Actually I don't know if this issue happens with other mx25/mx51 boards.
> 
> Benoit says his mx25 board can operate with high speed SD card in a
> modified version of U-Boot.
> I asked him if he could test his board with mainline U-Boot instead
> for comparison.

I will try to quickly run mainline U-Boot on my board as asked to see.

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [RFC] mmc: fsl_esdhc: Do not set high speed mode on MX25 and MX51

[Benoît Thébaudeau]

Hi Fabio,

On Wed, Oct 18, 2017 at 7:35 PM, Fabio Estevam <feste...@gmail.com> wrote:
> On Wed, Oct 18, 2017 at 2:56 PM, Benoît Thébaudeau <ben...@wsystem.com> wrote:
>
>> I can tell you what to use for imx25pdk if you give me the pads used by the
>> eSDHC instance in question here.
>
> mx25pdk uses the POR default IOMUX settings for sdhc1:

Can you try with this?

static const iomux_v3_cfg_t sdhc1_pads[] = {
NEW_PAD_CTRL(MX25_PAD_SD1_CMD__SD1_CMD, PAD_CTL_PUS_47K_UP | PAD_CTL_SRE_FAST),
NEW_PAD_CTRL(MX25_PAD_SD1_CLK__SD1_CLK, PAD_CTL_SRE_FAST),
NEW_PAD_CTRL(MX25_PAD_SD1_DATA0__SD1_DATA0, PAD_CTL_PUS_47K_UP |
PAD_CTL_SRE_FAST),
NEW_PAD_CTRL(MX25_PAD_SD1_DATA1__SD1_DATA1, PAD_CTL_PUS_47K_UP |
PAD_CTL_SRE_FAST),
NEW_PAD_CTRL(MX25_PAD_SD1_DATA2__SD1_DATA2, PAD_CTL_PUS_47K_UP |
PAD_CTL_SRE_FAST),
NEW_PAD_CTRL(MX25_PAD_SD1_DATA3__SD1_DATA3, PAD_CTL_PUS_47K_UP |
PAD_CTL_SRE_FAST),
NEW_PAD_CTRL(MX25_PAD_CTL_GRP_DSE_SDHC1, PAD_CTL_DSE_HIGH),
};

If you scope the SD clock during U-Boot HS SD transfers, do you get 48
MHz as expected?

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [RFC] mmc: fsl_esdhc: Do not set high speed mode on MX25 and MX51

[Benoît Thébaudeau]

Hi Fabio,

On 18/10/2017 at 18:40, Fabio Estevam wrote:
> On Wed, Oct 18, 2017 at 2:13 PM, Benoît Thébaudeau <ben...@wsystem.com> wrote:
>> You can try to set the drive strength of all the eSDHC pads to high or max 
>> (for
> 
> Looks like the eSDHC pins do not have drive strength control, only
> PKE, PUE, PUS and SRE fields.

They may also be in a pad group control register such as
MX25_PAD_CTL_GRP_DSE_CSI.

> Also, in the kernel I do not do pinctrl setting and just use the IOMUX
> values from U-Boot. In the kernel the SD card operation in high speed
> works fine.
> 
>> each pad or for the group of pads depending on the register layout for the 
>> pads
>> used on the board). This worked on my board (custom i.MX25-based, not 
>> imx25pdk).
> 
> Could you please share your custom eSDHC pinctrl settings in U-Boot?

I can tell you what to use for imx25pdk if you give me the pads used by the
eSDHC instance in question here.

> Also, I suppose you see "Tran Speed: 5000" when you run mmc info
> with a SD high speed card connected.

Indeed.

> In my case whenever 'mmc info' reports 50MHz I am not able to access
> the SD card (saveenv, for example) on both mx25pdk and mx51evk.

Note that I am using a modified version of U-Boot, so it may work thanks to that
for me, but the pad fixup was required anyway.

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [RFC] mmc: fsl_esdhc: Do not set high speed mode on MX25 and MX51

[Benoît Thébaudeau]

On 18/10/2017 at 18:13, Benoît Thébaudeau wrote:
> Hi Fabio,
> 
> On 18/10/2017 at 17:57, Fabio Estevam wrote:
>> Currently when a high speed SD card is connected on MX25 or MX51 boards
>> the following error happens:
>>
>> U-Boot 2017.11-rc2 (Oct 18 2017 - 13:49:26 -0200)
>>
>> CPU:   Freescale i.MX51 rev3.0 at 800 MHz
>> Reset cause: POR
>> Board: MX51EVK
>> DRAM:  512 MiB
>> MMC:   FSL_SDHC: 0, FSL_SDHC: 1
>> *** Warning - read failed, using default environment
>>
>> In:serial
>> Out:   serial
>> Err:   serial
>> Net:   FEC
>> Hit any key to stop autoboot:  0
>> => saveenv
>> Saving Environment to MMC...
>> Writing to MMC(0)... failed
>>
>> Workaround this issue by not setting the mmc high speed mode flags even
>> if the HOSTCAPBLT register reports that the SD card can operate at
>> high speed.
>>
>> Tested on imx51evk and imx25pdk boards.
>>
>> Signed-off-by: Fabio Estevam <fabio.este...@nxp.com>
>> ---
>> Kernel is capable of running the SD card at 50MHz clock, but U-Boot fails.
>>
>> I haven't had a chance to debug this further, but sending it as RFC in case
>> someone has some suggestions.
> 
> [...]
> 
> You could stress-test the SD read/write accesses from U-Boot. Does it occur 
> with
> all HS cards?
> 
> You can try to set the drive strength of all the eSDHC pads to high or max 
> (for
> each pad or for the group of pads depending on the register layout for the 
> pads
> used on the board). This worked on my board (custom i.MX25-based, not 
> imx25pdk).

And set SRE to fast too.

Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [RFC] mmc: fsl_esdhc: Do not set high speed mode on MX25 and MX51

[Benoît Thébaudeau]

Hi Fabio,

On 18/10/2017 at 17:57, Fabio Estevam wrote:
> Currently when a high speed SD card is connected on MX25 or MX51 boards
> the following error happens:
> 
> U-Boot 2017.11-rc2 (Oct 18 2017 - 13:49:26 -0200)
> 
> CPU:   Freescale i.MX51 rev3.0 at 800 MHz
> Reset cause: POR
> Board: MX51EVK
> DRAM:  512 MiB
> MMC:   FSL_SDHC: 0, FSL_SDHC: 1
> *** Warning - read failed, using default environment
> 
> In:serial
> Out:   serial
> Err:   serial
> Net:   FEC
> Hit any key to stop autoboot:  0
> => saveenv
> Saving Environment to MMC...
> Writing to MMC(0)... failed
> 
> Workaround this issue by not setting the mmc high speed mode flags even
> if the HOSTCAPBLT register reports that the SD card can operate at
> high speed.
> 
> Tested on imx51evk and imx25pdk boards.
> 
> Signed-off-by: Fabio Estevam 
> ---
> Kernel is capable of running the SD card at 50MHz clock, but U-Boot fails.
> 
> I haven't had a chance to debug this further, but sending it as RFC in case
> someone has some suggestions.

[...]

You could stress-test the SD read/write accesses from U-Boot. Does it occur with
all HS cards?

You can try to set the drive strength of all the eSDHC pads to high or max (for
each pad or for the group of pads depending on the register layout for the pads
used on the board). This worked on my board (custom i.MX25-based, not imx25pdk).

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2] usb: fix usb_stor_read/write on DM

[Benoît Thébaudeau]

On Sat, Jul 15, 2017 at 12:06 AM, Marek Vasut <ma...@denx.de> wrote:
> On 07/14/2017 11:46 PM, Benoît Thébaudeau wrote:
>> On Fri, Jul 14, 2017 at 1:50 PM, Marek Vasut <ma...@denx.de> wrote:
>>> On 07/14/2017 01:03 PM, Masahiro Yamada wrote:
>>>> 2017-07-14 19:07 GMT+09:00 Marek Vasut <ma...@denx.de>:
>>>>> On 07/14/2017 04:31 AM, Masahiro Yamada wrote:
>>>>>> Prior to DM, we could not enable different types of USB controllers
>>>>>> at the same time.  DM was supposed to loosen the limitation.  It is
>>>>>> true that we can compile drivers, but they do not work.
>>>>>>
>>>>>> For example, if EHCI is enabled, xHCI fails as follows:
>>>>>>
>>>>>>   => usb read 8200 0 2000
>>>>>>
>>>>>>   USB read: device 0 block # 0, count 8192 ... WARN halted endpoint, 
>>>>>> queueing URB anyway.
>>>>>>   Unexpected XHCI event TRB, skipping... (3fb54010 0001 1300 
>>>>>> 01008401)
>>>>>>   BUG: failure at drivers/usb/host/xhci-ring.c:489/abort_td()!
>>>>>>   BUG!
>>>>>>   ### ERROR ### Please RESET the board ###
>>>>>>
>>>>>> The cause of the error seems the following code:
>>>>>>
>>>>>>   #ifdef CONFIG_USB_EHCI_HCD
>>>>>>   /*
>>>>>>* The U-Boot EHCI driver can handle any transfer length as long as 
>>>>>> there is
>>>>>>* enough free heap space left, but the SCSI READ(10) and WRITE(10) 
>>>>>> commands are
>>>>>>* limited to 65535 blocks.
>>>>>>*/
>>>>>>   #define USB_MAX_XFER_BLK65535
>>>>>>   #else
>>>>>>   #define USB_MAX_XFER_BLK20
>>>>>>   #endif
>>>>>>
>>>>>> To fix the problem, choose the chunk size at run-time for CONFIG_BLK.
>>>>>
>>>>> What happens if CONFIG_BLK is not set ?
>>>>
>>>> USB_MAX_XFER_BLK is chosen.
>>>
>>> And can we fix that even for non-CONFIG_BLK ?
>>>
>>>>> Why is it 20 for XHCI anyway ?
>>>>
>>>> You are the maintainer.
>>>> (I hope) you have better knowledge with this.
>>>
>>> Heh, way to deflect the question. I seem to remember some discussion
>>> about the DMA (?) limitation on XHCI, but I'd have to dig through the ML
>>> archives myself.
>>>
>>>> Looks like the following commit was picked up by you.
>>>
>>> 5 years ago, way before DM was what it is today .
>>
>> And even way before the introduction of XHCI into U-Boot, which means
>> that this 20 was targeting OHCI or proprietary HCDs, not XHCI.
>> USB_MAX_READ_BLK was already set to 20 in the initial revision of
>> usb_storage.c. As I said in the commit message, this 20 was certainly
>> not optimal for these non-EHCI HCDs, but it restored the previous
>> (i.e. pre-5dd95cf) behavior for these HCDs instead of using the 5 * 4
>> KiB code, which was specific to ehci-hcd.c at that time. Without
>> knowing the rationale for the legacy 20 blocks, the safest approach
>> for non-EHCI HCDs was to use this value in order to avoid breaking a
>> platform or something. Looking at ohci-hcd.c, it limits the transfer
>> size to (N_URB_TD - 2) * 4 KiB, with N_URB_TD set to 48, so the
>> maximum number of transfers would depend on the MSC block size.
>> dwc2.c, isp116x-hcd.c, r8a66597-hcd.c, and sl811-hcd.c do not seem to
>> have any limit caused by these drivers. The limit with the current
>> XHCI code seems to be 64 * 64 KiB. So, nowadays, USB_MAX_XFER_BLK
>> could be set to 65535 for all HCDs but OHCI and XHCI, which require
>> specific rules depending on the MSC block size.
>
> For whatever reason, something tells me that setting the block size to
> 64k for XHCI broke things, but I cannot locate the thread. But there's
> something in the back of my head ...

Indeed: according to what I said above, USB_MAX_XFER_BLK cannot be set
to 65535 for XHCI. With an MSC block size of blksz = 512 bytes /
block, USB_MAX_XFER_BLK can be set to at most 1 segment *
(TRBS_PER_SEGMENT = 64 TRBs / segment) * (TRB_MAX_BUFF_SIZE = 65536
bytes / TRB) / blksz = 8192 blocks for XHCI. And for OHCI, the limit
is (N_URB_TD - 2 = 46 TDs) * (4096 bytes / TD) / blksz = 368 blocks.
The buffer alignment may also have to be taken into account to adjust
these values, which would require a USB_MAX_XFER_BLK(host_if, start,
blksz) macro or function. USB_MAX_XFER_BLK can however be set to 65535
regardless of blksz for all the other HCDs (i.e. EHCI, dwc2.c,
isp116x-hcd.c, r8a66597-hcd.c, and sl811-hcd.c).

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2] usb: fix usb_stor_read/write on DM

[Benoît Thébaudeau]

On Fri, Jul 14, 2017 at 1:50 PM, Marek Vasut <ma...@denx.de> wrote:
> On 07/14/2017 01:03 PM, Masahiro Yamada wrote:
>> 2017-07-14 19:07 GMT+09:00 Marek Vasut <ma...@denx.de>:
>>> On 07/14/2017 04:31 AM, Masahiro Yamada wrote:
>>>> Prior to DM, we could not enable different types of USB controllers
>>>> at the same time.  DM was supposed to loosen the limitation.  It is
>>>> true that we can compile drivers, but they do not work.
>>>>
>>>> For example, if EHCI is enabled, xHCI fails as follows:
>>>>
>>>>   => usb read 8200 0 2000
>>>>
>>>>   USB read: device 0 block # 0, count 8192 ... WARN halted endpoint, 
>>>> queueing URB anyway.
>>>>   Unexpected XHCI event TRB, skipping... (3fb54010 0001 1300 
>>>> 01008401)
>>>>   BUG: failure at drivers/usb/host/xhci-ring.c:489/abort_td()!
>>>>   BUG!
>>>>   ### ERROR ### Please RESET the board ###
>>>>
>>>> The cause of the error seems the following code:
>>>>
>>>>   #ifdef CONFIG_USB_EHCI_HCD
>>>>   /*
>>>>* The U-Boot EHCI driver can handle any transfer length as long as 
>>>> there is
>>>>* enough free heap space left, but the SCSI READ(10) and WRITE(10) 
>>>> commands are
>>>>* limited to 65535 blocks.
>>>>*/
>>>>   #define USB_MAX_XFER_BLK65535
>>>>   #else
>>>>   #define USB_MAX_XFER_BLK20
>>>>   #endif
>>>>
>>>> To fix the problem, choose the chunk size at run-time for CONFIG_BLK.
>>>
>>> What happens if CONFIG_BLK is not set ?
>>
>> USB_MAX_XFER_BLK is chosen.
>
> And can we fix that even for non-CONFIG_BLK ?
>
>>> Why is it 20 for XHCI anyway ?
>>
>> You are the maintainer.
>> (I hope) you have better knowledge with this.
>
> Heh, way to deflect the question. I seem to remember some discussion
> about the DMA (?) limitation on XHCI, but I'd have to dig through the ML
> archives myself.
>
>> Looks like the following commit was picked up by you.
>
> 5 years ago, way before DM was what it is today .

And even way before the introduction of XHCI into U-Boot, which means
that this 20 was targeting OHCI or proprietary HCDs, not XHCI.
USB_MAX_READ_BLK was already set to 20 in the initial revision of
usb_storage.c. As I said in the commit message, this 20 was certainly
not optimal for these non-EHCI HCDs, but it restored the previous
(i.e. pre-5dd95cf) behavior for these HCDs instead of using the 5 * 4
KiB code, which was specific to ehci-hcd.c at that time. Without
knowing the rationale for the legacy 20 blocks, the safest approach
for non-EHCI HCDs was to use this value in order to avoid breaking a
platform or something. Looking at ohci-hcd.c, it limits the transfer
size to (N_URB_TD - 2) * 4 KiB, with N_URB_TD set to 48, so the
maximum number of transfers would depend on the MSC block size.
dwc2.c, isp116x-hcd.c, r8a66597-hcd.c, and sl811-hcd.c do not seem to
have any limit caused by these drivers. The limit with the current
XHCI code seems to be 64 * 64 KiB. So, nowadays, USB_MAX_XFER_BLK
could be set to 65535 for all HCDs but OHCI and XHCI, which require
specific rules depending on the MSC block size.

>> commit cffcc503580907d733e694d505e12ff6ec6c679a
>> Author: Benoît Thébaudeau <benoit.thebaud...@advansee.com>
>> AuthorDate: Fri Aug 10 18:26:50 2012 +0200
>> Commit: Marek Vasut <ma...@denx.de>
>> CommitDate: Sat Sep 1 16:21:52 2012 +0200
>>
>> usb_storage: Restore non-EHCI support
>>
>> The commit 5dd95cf made the MSC driver EHCI-specific. This patch 
>> restores a
>> basic support of non-EHCI HCDs, like before that commit.
>>
>> The fallback transfer size is certainly not optimal, but at least
>> it should work
>> like before.
>>
>> Signed-off-by: Benoît Thébaudeau <benoit.thebaud...@advansee.com>
>> Cc: Marek Vasut <ma...@denx.de>
>> Cc: Ilya Yanok <ilya.ya...@cogentembedded.com>
>> Cc: Stefan Herbrechtsmeier <ste...@herbrechtsmeier.net>
>>
>> diff --git a/common/usb_storage.c b/common/usb_storage.c
>> index bdc306f587fd..0cd6399a3c42 100644
>> --- a/common/usb_storage.c
>> +++ b/common/usb_storage.c
>> @@ -155,11 +155,15 @@ struct us_data {
>> trans_cmnd  transport;  /* transport routine */
>>  };
>>
>> +#ifdef CONFIG_USB_EHCI
>>  /*
>>   * The U-Boot EHCI driver cannot handle more than 5 page aligned buffers
>>   * of 4096 bytes in a transfer without running itself out of qt_buffers
>>   */
>>  #define USB_MAX_XFER_BLK(start, blksz) (((4096 * 5) - (start % 4096)) / 
>> blksz)
>> +#else
>> +#define USB_MAX_XFER_BLK(start, blksz) 20
>> +#endif
>>
>>  static struct us_data usb_stor[USB_MAX_STOR_DEV];

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH 4/4] mx25pdk: Set the eSDHC PER clock to 48 MHz

[Benoît Thébaudeau]

The maximum SD clock frequency in High Speed mode is 50 MHz. This change
makes it possible to get 48 MHz from the USB PLL (240 MHz / 5 / 1)
instead of the previous 33.25 MHz from the AHB clock (133 MHz / 2 / 2).

Signed-off-by: Benoît Thébaudeau <ben...@wsystem.com>
---
 board/freescale/mx25pdk/mx25pdk.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/board/freescale/mx25pdk/mx25pdk.c 
b/board/freescale/mx25pdk/mx25pdk.c
index 788d3c3..cab769c 100644
--- a/board/freescale/mx25pdk/mx25pdk.c
+++ b/board/freescale/mx25pdk/mx25pdk.c
@@ -175,6 +175,12 @@ int board_mmc_init(bd_t *bis)
 
imx_iomux_v3_setup_multiple_pads(sdhc1_pads, ARRAY_SIZE(sdhc1_pads));
 
+   /*
+* Set the eSDHC1 PER clock to the maximum frequency lower than or equal
+* to 50 MHz that can be obtained, which requires to use UPLL as the
+* clock source. This actually gives 48 MHz.
+*/
+   imx_set_perclk(MXC_ESDHC1_CLK, true, 5000);
esdhc_cfg[0].sdhc_clk = mxc_get_clock(MXC_ESDHC1_CLK);
return fsl_esdhc_initialize(bis, _cfg[0]);
 }
-- 
2.7.4

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH 2/4] mx25: Fix imx_get_perclk()

[Benoît Thébaudeau]

imx_get_perclk() used the AHB clock as the clock source for all PER
clocks, but the USB PLL output can also be a PER clock source if the
corresponding PER CLK MUX bit is set in CCM.MCR.

Signed-off-by: Benoît Thébaudeau <ben...@wsystem.com>
---
 arch/arm/cpu/arm926ejs/mx25/generic.c | 11 ++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/arm/cpu/arm926ejs/mx25/generic.c 
b/arch/arm/cpu/arm926ejs/mx25/generic.c
index 0b1a8f4..f02cffb 100644
--- a/arch/arm/cpu/arm926ejs/mx25/generic.c
+++ b/arch/arm/cpu/arm926ejs/mx25/generic.c
@@ -58,6 +58,14 @@ static ulong imx_get_mpllclk(void)
return imx_decode_pll(readl(>mpctl), fref);
 }
 
+static ulong imx_get_upllclk(void)
+{
+   struct ccm_regs *ccm = (struct ccm_regs *)IMX_CCM_BASE;
+   ulong fref = MXC_HCLK;
+
+   return imx_decode_pll(readl(>upctl), fref);
+}
+
 static ulong imx_get_armclk(void)
 {
struct ccm_regs *ccm = (struct ccm_regs *)IMX_CCM_BASE;
@@ -95,7 +103,8 @@ static ulong imx_get_ipgclk(void)
 static ulong imx_get_perclk(int clk)
 {
struct ccm_regs *ccm = (struct ccm_regs *)IMX_CCM_BASE;
-   ulong fref = imx_get_ahbclk();
+   ulong fref = readl(>mcr) & (1 << clk) ? imx_get_upllclk() :
+imx_get_ahbclk();
ulong div;
 
div = readl(>pcdr[CCM_PERCLK_REG(clk)]);
-- 
2.7.4

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH 1/4] mmc: fsl_esdhc: Allow all supported prescaler values

[Benoît Thébaudeau]

On i.MX, SYSCTL.SDCLKFS may be set to 0 in order to make the SD clock
frequency prescaler divide by 1 in SDR mode. In DDR mode, the prescaler
can divide by up to 512. Allow both of these settings.

The maximum SD clock frequency in High Speed mode is 50 MHz. On i.MX25,
this change makes it possible to get 48 MHz from the USB PLL
(240 MHz / 5 / 1) instead of only 40 MHz from the USB PLL
(240 MHz / 3 / 2) or 33.25 MHz from the AHB clock (133 MHz / 2 / 2).

Signed-off-by: Benoît Thébaudeau <ben...@wsystem.com>
---
 drivers/mmc/fsl_esdhc.c | 23 ---
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
index f3c6358..ca72627 100644
--- a/drivers/mmc/fsl_esdhc.c
+++ b/drivers/mmc/fsl_esdhc.c
@@ -521,7 +521,13 @@ out:
 
 static void set_sysctl(struct mmc *mmc, uint clock)
 {
-   int div, pre_div;
+   int div = 1;
+#ifdef ARCH_MXC
+   int pre_div = 1;
+#else
+   int pre_div = 2;
+#endif
+   int ddr_pre_div = mmc->ddr_mode ? 2 : 1;
struct fsl_esdhc_priv *priv = mmc->priv;
struct fsl_esdhc *regs = priv->esdhc_regs;
int sdhc_clk = priv->sdhc_clk;
@@ -530,18 +536,13 @@ static void set_sysctl(struct mmc *mmc, uint clock)
if (clock < mmc->cfg->f_min)
clock = mmc->cfg->f_min;
 
-   if (sdhc_clk / 16 > clock) {
-   for (pre_div = 2; pre_div < 256; pre_div *= 2)
-   if ((sdhc_clk / pre_div) <= (clock * 16))
-   break;
-   } else
-   pre_div = 2;
+   while (sdhc_clk / (16 * pre_div * ddr_pre_div) > clock && pre_div < 256)
+   pre_div *= 2;
 
-   for (div = 1; div <= 16; div++)
-   if ((sdhc_clk / (div * pre_div)) <= clock)
-   break;
+   while (sdhc_clk / (div * pre_div * ddr_pre_div) > clock && div < 16)
+   div++;
 
-   pre_div >>= mmc->ddr_mode ? 2 : 1;
+   pre_div >>= 1;
div -= 1;
 
clk = (pre_div << 8) | (div << 4);
-- 
2.7.4

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH 3/4] mx25: Add function to set PER clocks

[Benoît Thébaudeau]

Introduce the imx_set_perclk() function to make it possible to set the
PER clocks.

Signed-off-by: Benoît Thébaudeau <ben...@wsystem.com>
---
 arch/arm/cpu/arm926ejs/mx25/generic.c  | 19 +++
 arch/arm/include/asm/arch-mx25/clock.h |  1 +
 2 files changed, 20 insertions(+)

diff --git a/arch/arm/cpu/arm926ejs/mx25/generic.c 
b/arch/arm/cpu/arm926ejs/mx25/generic.c
index f02cffb..5d9bc6c 100644
--- a/arch/arm/cpu/arm926ejs/mx25/generic.c
+++ b/arch/arm/cpu/arm926ejs/mx25/generic.c
@@ -113,6 +113,25 @@ static ulong imx_get_perclk(int clk)
return fref / div;
 }
 
+int imx_set_perclk(enum mxc_clock clk, bool from_upll, unsigned int freq)
+{
+   struct ccm_regs *ccm = (struct ccm_regs *)IMX_CCM_BASE;
+   ulong fref = from_upll ? imx_get_upllclk() : imx_get_ahbclk();
+   ulong div = (fref + freq - 1) / freq;
+
+   if (clk > MXC_UART_CLK || !div || --div > CCM_PERCLK_MASK)
+   return -EINVAL;
+
+   clrsetbits_le32(>pcdr[CCM_PERCLK_REG(clk)],
+   CCM_PERCLK_MASK << CCM_PERCLK_SHIFT(clk),
+   div << CCM_PERCLK_SHIFT(clk));
+   if (from_upll)
+   setbits_le32(>mcr, 1 << clk);
+   else
+   clrbits_le32(>mcr, 1 << clk);
+   return 0;
+}
+
 unsigned int mxc_get_clock(enum mxc_clock clk)
 {
if (clk >= MXC_CLK_NUM)
diff --git a/arch/arm/include/asm/arch-mx25/clock.h 
b/arch/arm/include/asm/arch-mx25/clock.h
index 9fdaa9d..7753caf 100644
--- a/arch/arm/include/asm/arch-mx25/clock.h
+++ b/arch/arm/include/asm/arch-mx25/clock.h
@@ -51,6 +51,7 @@ enum mxc_clock {
MXC_CLK_NUM
 };
 
+int imx_set_perclk(enum mxc_clock clk, bool from_upll, unsigned int freq);
 unsigned int mxc_get_clock(enum mxc_clock clk);
 
 #define imx_get_uartclk()  mxc_get_clock(MXC_UART_CLK)
-- 
2.7.4

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] ARM - cache and alignment

[Benoît Thébaudeau]

Hi,

On Tue, Jan 17, 2017 at 10:54 AM, Marek Vasut  wrote:
> On 01/17/2017 10:51 AM, Jean-Jacques Hiblot wrote:
>>
>>
>> On 17/01/2017 10:38, Marek Vasut wrote:
>>> On 01/17/2017 10:35 AM, Jean-Jacques Hiblot wrote:

 On 17/01/2017 10:15, Marek Vasut wrote:
> On 01/17/2017 10:08 AM, Jean-Jacques Hiblot wrote:
>> On 16/01/2017 20:33, Marek Vasut wrote:
>>> On 01/16/2017 08:16 PM, Jean-Jacques Hiblot wrote:
 On 16/01/2017 17:00, Marek Vasut wrote:
> On 01/16/2017 02:29 PM, Jean-Jacques Hiblot wrote:
>> Tom, Marek
> Hi,
>
>> At the moment, whenever an unaligned address is used in cache
>> operations
>> (invalidate_dcache_range, or flush_dcache_range), the whole
>> request is
>> discarded  for am926ejs. for armV7 or armV8 only the aligned
>> part is
>> maintained. This is probably what is causing the bug addressed in
>> 8133f43d1cd. There are a lot of unaligned buffers used in DMA
>> operations
>> and for all of them, we're possibly handling the cached partially
>> or not
>> at all. I've seen this when using the environment from a file
>> stored in
>> a FAT partition. commit 8133f43d1cd addresses this by using a
>> bounce
>> buffer at the FAT level but it's only one of many cases.
>>
>> I think we can do better with unaligned cache operations:
>>
>> * flush (writeback + invalidate): Suppose we use address p
>> which is
>> unaligned, flush_dcache_range() can do the writeback+invalidate
>> on the
>> whole range [p & ~(line_sz - 1); p + length | (line_sz - 1)].
>> There
>> should no problem with that since writeback can happen at any
>> point in
>> time.
>>
>> * invalidation
>>
>> It is a bit trickier. here is a pseudo-code:
>> invalidate_dcache_range(p,length)
>> {
>>  write_back_invalidate(first line)
>>  write_back_invalidate(last line)
>>  invalidate(all other lines)
>> }
>>
>> Here again this should work fine IF invalidate_dcache_range() is
>> called
>> BEFORE the DMA operation (again the writeback can happen at
>> time so
>> it's
>> valid do it here). Calling it only AFTER the operation, may
>> corrupt
>> the
>> data written by the DMA with old data from CPU. This how I used to
>> handle unaligned buffers in some other projects.
>>
>>
>> There is however one loophole: a data sitting in the first or the
>> last
>> line is accessed before the memory is updated by the DMA, then the
>> first/line will be corrupted. But it's not highly probable as this
>> data
>> would have to be used in parallel of the DMA (interrupt handling,
>> SMP?,
>> dma mgt related variable). So it's not perfect but it would
>> still be
>> better than we have today.
> Or just fix all the code which complains about unaligned buffers,
> done.
> That's the way to go without all the complications above.
 It's not that complex, but it's not perfect. We would need to
 keep the
 same warning as we have now, but it would make it work in more
 cases.
>>> The warning is there for that exact reason -- to inform you
>>> something's
>>> wrong.
>>>
 Tracking every possible unaligned buffer that gets invalidated is
 not a
 trivial job. Most of the time the buffer is allocated in a upper
 layer
 and passed down to a driver via layers like network stack, block
 layer
 etc.And in many cases, the warning will come and go depending on
 how the
 variable aligned on the stack or the heap.
>>> I didn't observe this much in fact. I usually see the buffers
>>> coming it
>>> aligned or being allocated in drivers. Also, I think that's why
>>> the RC
>>> cycle is there, so we can test the next release and fix these issues.
>> It's not commonly seen but I came across it some times.
>>
>> Here are two examples:
>>
>> Network:
>> U-Boot 2016.09-rc1-00087-gd40ff0a (Jul 27 2016 - 10:04:33 -0500)
>> CPU  : DRA752-HS ES2.0
>> Model: TI DRA742
>> [...]
>> Booting from network ...
>> cpsw Waiting for PHY auto negotiation to complete done
>> link up on port 0, speed 1000, full duplex
>> BOOTP broadcast 1
>> CACHE: Misaligned operation at range [dffecb40, dffecc96]
>> CACHE: Misaligned operation at range [dffed140, dffed17e]
>> BOOTP broadcast 2
>> [...]
>> File transfer via NFS from server 10.0.1.26; our IP address is
>> 128.247.83.128; sending 

Re: [U-Boot] DMA bounce buffer in FAT code

[Benoît Thébaudeau]

Dear Jean-Jacques,

On Tue, Jan 10, 2017 at 12:35 PM, Jean-Jacques Hiblot  wrote:
> commits 8133f43d and cc63b25e introduced buffer bouncing at the level of the
> FAT fs driver. I came across those because of printf() saying that the
> buffer is not aligned when I save my env in file on a FAT partition. Those
> messages are annoying because they lead to think that there's a problem when
> saving the environment. We could move them to debug() or at least indicate
> that's only a warning.

That would be fine. I would have a small preference for debug() here.

> However I think that removing the bounce buffer mechanism altogether is
> better option. The alignment constraint doesn't belong at this level and
> indeed there are controllers that do not require it at all (those not using
> DMA for once). It could be moved to the block level and activated only if
> the driver for the controller needs it. Or it could be left to the driver of
> the controller do implement it.

Indeed. This was my first idea at the time of these commits, but it
required too many changes as this was before the introduction of DM.
The best way to go nowadays is probably to move the bounce buffer
mechanism to the block driver, using it if and only if the underlying
storage driver has flagged somewhere that it needs it. Doing it at a
lower level would require to duplicate this mechanism, which would not
be great. Note that this mechanism may also be present in other FS
drivers.

> I'm willing to prepare the patches for the modifications if you think they
> make sense.

Thanks.

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 2/2] fs/fat: merge readwrite get_fatent_value() with readonly get_fatent()

[Benoît Thébaudeau]

Dear Stefan Brüns,

On Sun, Dec 11, 2016 at 3:32 AM, Stefan Brüns
<stefan.bru...@rwth-aachen.de> wrote:
> get_fatent_value(...) flushes changed FAT entries to disk when fetching
> the next FAT blocks, in every other aspect it is identical to
> get_fatent(...).
>
> Provide a stub implementation for flush_dirty_fat_buffer if
> CONFIG_FAT_WRITE is not set. Calling flush_dirty_fat_buffer during read
> only operation is fine as it checks if any buffers needs flushing.
>
> Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de>
> ---
>  fs/fat/fat.c   |  19 +
>  fs/fat/fat_write.c | 118 
> +++--
>  2 files changed, 24 insertions(+), 113 deletions(-)

[...]

> diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
> index 0be60ecd99..c05fc7f099 100644
> --- a/fs/fat/fat_write.c
> +++ b/fs/fat/fat_write.c
> @@ -143,114 +143,6 @@ static int flush_dirty_fat_buffer(fsdata *mydata)
>  }
>
>  /*
> - * Get the entry at index 'entry' in a FAT (12/16/32) table.
> - * On failure 0x00 is returned.
> - * When bufnum is changed, write back the previous fatbuf to the disk.
> - */
> -static __u32 get_fatent_value(fsdata *mydata, __u32 entry)
> -{

[...]

> -   debug("FAT%d: ret: %08x, entry: %08x, offset: %04x\n",
> -  mydata->fatsize, ret, entry, offset);

It might be worth adding this entry info to the same line in get_fatent().

> -
> -   return ret;
> -}

[...]

Apart from that:
Reviewed-by: Benoît Thébaudeau <benoit.thebaudeau@gmail.com>

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 1/2] fs/fat: Avoid corruption of sectors following the FAT

[Benoît Thébaudeau]

Dear Stefan Brüns,

On Sun, Dec 11, 2016 at 3:32 AM, Stefan Brüns
<stefan.bru...@rwth-aachen.de> wrote:
> From: Stefan Brüns <stefan.bru...@rwth-aachen.de>
>
> The FAT is read/flushed in segments of 6 (FATBUFBLOCKS) disk sectors. The
> last segment may be less than 6 sectors, cap the length.
>
> Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de>

[...]

There's a character encoding issue for the "ü" in your last name in
"From:". It is correct in your "Signed-off-by:". Apart from that:
Reviewed-by: Benoît Thébaudeau <benoit.thebaudeau@gmail.com>

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v1] fat: fatwrite: fix the command for FAT12

[Benoît Thébaudeau]

Dear Stefan Brüns,

On Sun, Dec 11, 2016 at 12:29 AM, Stefan Bruens
 wrote:
> On Freitag, 9. Dezember 2016 13:55:37 CET Philipp Skadorov wrote:
>> The u-boot command fatwrite empties FAT clusters from the beginning
>> till the end of the file.
>> Specifically for FAT12 it fails to detect the end of the file and goes
>> beyond the file bounds thus corrupting the file system.
>>
>> The users normally workaround this by re-formatting the partition as
>> FAT16/FAT32, like here:
>> https://github.com/FEDEVEL/openrex-uboot-v2015.10/issues/1
>>
>> The patch is to check file bounds by already-existing macro that
>> accounts for FAT12.
>> The command then works correctly for all types of FAT.
>>
>> Signed-off-by: Philipp Skadorov 
>> Cc:Donggeun Kim 
>> ---
>>  fs/fat/fat_write.c | 5 +
>>  1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
>> index 40a3860..e4f600e 100644
>> --- a/fs/fat/fat_write.c
>> +++ b/fs/fat/fat_write.c
>> @@ -670,16 +670,13 @@ static int clear_fatent(fsdata *mydata, __u32 entry)
>>  {
>>   __u32 fat_val;
>>
>> - while (1) {
>> + while (!CHECK_CLUST(entry, mydata->fatsize)) {
>>   fat_val = get_fatent_value(mydata, entry);
>>   if (fat_val != 0)
>>   set_fatent_value(mydata, entry, 0);
>>   else
>>   break;
>>
>> - if (fat_val == 0xfff || fat_val == 0x)
>> - break;
>> -
>>   entry = fat_val;
>>   }
>
> NAK.
>
> This corrupts the file system, as set_fatent_value(...) has:
>
> switch (mydata->fatsize) {
> case 32:
> bufnum = entry / FAT32BUFSIZE;
> offset = entry - bufnum * FAT32BUFSIZE;
> break;
> case 16:
> bufnum = entry / FAT16BUFSIZE;
> offset = entry - bufnum * FAT16BUFSIZE;
> break;
> default:
> /* Unsupported FAT size */
> return -1;
> }

So this patch can be kept, but it needs to be combined with a new one
in a series to fully fix fatwrite for FAT12.

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v1] fat: fatwrite: fix the command for FAT12

[Benoît Thébaudeau]

Dear Philipp Skadorov,

On Fri, Dec 9, 2016 at 7:55 PM, Philipp Skadorov
<philipp.skado...@savoirfairelinux.com> wrote:
> The u-boot command fatwrite empties FAT clusters from the beginning
> till the end of the file.
> Specifically for FAT12 it fails to detect the end of the file and goes
> beyond the file bounds thus corrupting the file system.
>
> The users normally workaround this by re-formatting the partition as
> FAT16/FAT32, like here:
> https://github.com/FEDEVEL/openrex-uboot-v2015.10/issues/1
>
> The patch is to check file bounds by already-existing macro that
> accounts for FAT12.
> The command then works correctly for all types of FAT.
>
> Signed-off-by: Philipp Skadorov <philipp.skado...@savoirfairelinux.com>
> Cc:Donggeun Kim <dg77@samsung.com>

[...]

Reviewed-by: Benoît Thébaudeau <benoit.thebaudeau@gmail.com>

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] cmd/nand.c: Remove unnecessarily repeated step

[Benoît Thébaudeau]

Dear Ahmed Samir Khalil,

On Mon, Oct 10, 2016 at 6:44 AM, Ahmed Samir Khalil
 wrote:
> Getting the current NAND device is already done once as part
>  of nand command. Therefore, repeating this step as part of
>  the sub-commands is unnecessary.
>
> Signed-off-by: Ahmed Samir Khalil 
> ---
>  cmd/nand.c | 6 --
>  1 file changed, 6 deletions(-)
>
> diff --git a/cmd/nand.c b/cmd/nand.c
> index ec7f1df..71ffe85 100644
> --- a/cmd/nand.c
> +++ b/cmd/nand.c
> @@ -528,8 +528,6 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, 
> char * const argv[])
> if (set_dev(dev))
> return 1;
>
> -   mtd = nand_info[dev];
> -
> memset(, 0, sizeof(opts));
> opts.offset = off;
> opts.length = size;
> @@ -597,8 +595,6 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, 
> char * const argv[])
> if (set_dev(dev))
> return 1;
>
> -   mtd = nand_info[dev];
> -
> if (argc > 4 && !str2long(argv[4], )) {
> printf("'%s' is not a number\n", argv[4]);
> return 1;
> @@ -626,8 +622,6 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, 
> char * const argv[])
> rwsize = size;
> }
>
> -   mtd = nand_info[dev];
> -
> if (!s || !strcmp(s, ".jffs2") ||
> !strcmp(s, ".e") || !strcmp(s, ".i")) {
> if (read)

Quickly looking at the code, these assignments of mtd directly follow
calls to mtd_arg_off_size(), which may alter dev, so they are required
and should not be removed. This is even mentioned in the comment block
above the initialization of mtd that you refer to: "The following
commands operate on the current device, _unless overridden by a
partition specifier_.".

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 1/4] fs/fat: Remove two statements without effect

[Benoît Thébaudeau]

Hi Stefan,

On Sun, Sep 11, 2016 at 10:51 PM, Stefan Brüns
<stefan.bru...@rwth-aachen.de> wrote:
> fatlength is a local variable which is no more used after the assignment.
> s_name is not used in the function, save the strncpy.
>
> Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de>

[...]

For the series:
Reviewed-by: Benoît Thébaudeau <benoit.thebaudeau@gmail.com>

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 2/2] fs/fat: Optimizes memory size with single global variable instead of 3

[Benoît Thébaudeau]

Hi,

On Fri, Sep 9, 2016 at 6:34 PM, Brüns, Stefan
<stefan.bru...@rwth-aachen.de> wrote:
> On Sonntag, 14. August 2016 00:57:38 CEST Benoît Thébaudeau wrote:
>> On Tue, Aug 2, 2016 at 9:35 PM, Benoît Thébaudeau
>>
>> <benoit.thebaudeau@gmail.com> wrote:
>> > On Tue, Aug 2, 2016 at 8:53 PM, Stephen Warren <swar...@wwwdotorg.org>
> wrote:
>> >> On 07/28/2016 12:11 AM, Tien Fong Chee wrote:
>> >>> Single 64KB get_contents_vfatname_block global variable would be used
>> >>> for
>> >>> all FAT implementation instead of allocating additional two global
>> >>> variables
>> >>> which are get_denfromdir_block and do_fat_read_at_block. This
>> >>> implementation
>> >>> can help in saving up 128KB memory space.
>> >>
>> >> The series,
>> >>
>> >> Tested-by: Stephen Warren <swar...@nvidia.com>
>> >> (via DFU's FAT reading/writing on various Tegra; likely primarily FAT
>> >> rather than VFAT though)
>> >>
>> >> Reviewed-by: Stephen Warren <swar...@nvidia.com>
>> >
>> > I suspect that reading a filename with VFAT entries crossing a cluster
>> > boundary in a FAT32 root directory will be broken by this series. I do
>> > not have time to test this and other corner cases right now though,
>> > but it will be possible in the next few weeks.
>>
>> I have tested VFAT long filenames with the current implementation on
>> Sandbox. It's completely broken:
>>  - There is a length limit somewhere between 111 and 120 characters,
>> instead of 256 characters. Beyond this limit, the files are invisible
>> with ls.
>
> That one is expected. U-Boot limits the extended name to 9 "slots", each 26
> bytes. As filenames are encoded as UTF-16/UCS-2, each ASCII character uses two
> bytes -> 9 * 26 / 2 = 117.

Indeed. I had only checked VFAT_MAXLEN_BYTES, not VFAT_MAXSEQ.

>>  - Some filenames are truncated or mixed up between files. I have
>> tested 111-character random filenames for 1000 empty files in the root
>> directory. Most filenames had the expected length, but a few were
>> shorter or longer.
>
> Where there any filenames with characters outside the ASCII range? There may
> have been some double en-/decoding of UTF-8 vs UTF-16.

No, only alnum for the affected files. There may have been one or two
filenames from previous tests outside the ASCII range, though, but I
don't think so.

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 2/2] fs/fat: Optimizes memory size with single global variable instead of 3

[Benoît Thébaudeau]

Hi,

On Tue, Aug 2, 2016 at 9:35 PM, Benoît Thébaudeau
<benoit.thebaudeau@gmail.com> wrote:
> On Tue, Aug 2, 2016 at 8:53 PM, Stephen Warren <swar...@wwwdotorg.org> wrote:
>> On 07/28/2016 12:11 AM, Tien Fong Chee wrote:
>>>
>>> Single 64KB get_contents_vfatname_block global variable would be used for
>>> all FAT implementation instead of allocating additional two global
>>> variables
>>> which are get_denfromdir_block and do_fat_read_at_block. This
>>> implementation
>>> can help in saving up 128KB memory space.
>>
>>
>> The series,
>>
>> Tested-by: Stephen Warren <swar...@nvidia.com>
>> (via DFU's FAT reading/writing on various Tegra; likely primarily FAT rather
>> than VFAT though)
>>
>> Reviewed-by: Stephen Warren <swar...@nvidia.com>
>
> I suspect that reading a filename with VFAT entries crossing a cluster
> boundary in a FAT32 root directory will be broken by this series. I do
> not have time to test this and other corner cases right now though,
> but it will be possible in the next few weeks.

I have tested VFAT long filenames with the current implementation on
Sandbox. It's completely broken:
 - There is a length limit somewhere between 111 and 120 characters,
instead of 256 characters. Beyond this limit, the files are invisible
with ls.
 - Some filenames are truncated or mixed up between files. I have
tested 111-character random filenames for 1000 empty files in the root
directory. Most filenames had the expected length, but a few were
shorter or longer.
 - If there are too many files in the root directory, ls hangs.

I am pretty sure that this series introduces some regressions, but
they seem to be in corner cases that cannot even be used or tested
because of other bugs, so this series might not make this
implementation much more broken than it currently is. It's risky,
though.

I've quickly looked into TianoCore EDK II. It is so deeply tied to the
EFI driver model and APIs that it would be a pain to port to U-Boot.
Its FAT module is not designed to be portable beyond EFI. Its build
system would complicate things too.

Stephen, according to what you say in test/fs/fat-noncontig-test.sh,
your solution to accelerate FatFs seems to be working, even if the
author is not interested in it, so maybe it would still be worth
maintaining locally in order to have a reliable FAT support, also with
a small memory footprint. barebox uses FatFs.

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 2/2] fs/fat: Optimizes memory size with single global variable instead of 3

[Benoît Thébaudeau]

Hi,

On Tue, Aug 2, 2016 at 8:53 PM, Stephen Warren  wrote:
> On 07/28/2016 12:11 AM, Tien Fong Chee wrote:
>>
>> Single 64KB get_contents_vfatname_block global variable would be used for
>> all FAT implementation instead of allocating additional two global
>> variables
>> which are get_denfromdir_block and do_fat_read_at_block. This
>> implementation
>> can help in saving up 128KB memory space.
>
>
> The series,
>
> Tested-by: Stephen Warren 
> (via DFU's FAT reading/writing on various Tegra; likely primarily FAT rather
> than VFAT though)
>
> Reviewed-by: Stephen Warren 

I suspect that reading a filename with VFAT entries crossing a cluster
boundary in a FAT32 root directory will be broken by this series. I do
not have time to test this and other corner cases right now though,
but it will be possible in the next few weeks.

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] fatwrite issue in sub-directories

[Benoît Thébaudeau]

Hi Nicolas,

On Wed, Jul 20, 2016 at 6:21 PM, Nicolas le bayon  wrote:
> Hi all,
>
> With a quite old u-boot release (October 2015 more or less), I has some
> different issues around fatwrite operations. I realigned a few things
> around FAT with latest release, and now I observe only one issue. Maybe
> this has been discussed here before, I searched but no success.
>
> I'd like to save a file on an external device (sd card or usb stick), but
> not on the root, in a sub-directory. I made the implementation with EXT4
> and it works fine. But no way (for the moment) with FAT.
>
> Here is the behaviour, let's imagine the device contains only a "mydir"
> directory.
>> fatls 
> mydir/
>> fatwrite   myfile.txt 
>> fatls 
> mydir/
> myfile.txt
>> fatwrite   mydir/myfile2.txt 
>> fatls 
>  mydir/
>  myfile.txt
>  mydir/myfile2.txt
> In fact, data is stored in a file strangely named with a '/' character.
>
> And when I put the device back on my Linux PC, I can see myfile.txt,
> nothing special in mydir directory, and no "mydir/myfile2.txt" file.
>
> Does someone have already observed this?

Yes. The issue is that do_fat_write() only supports filenames, and not
full paths, so whatever you give it ends up as a filename in the root
directory. There is no workaround.

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 2/2] fs/fat/fat: Optimizes memory size with single global variable instead of 3

[Benoît Thébaudeau]

Dear Tien Fong,

On Thu, Jul 14, 2016 at 12:48 PM, Tien Fong Chee <tfc...@altera.com> wrote:
> Dear Benoît,
>
> On Wed, 2016-07-13 at 12:56 +0200, Benoît Thébaudeau wrote:
>> Dear Tien Fong Chee,
>>
>> On Jul 13, 2016 at 11:01 AM, Tien Fong Chee wrote:
>> > Single 64KB get_contents_vfatname_block global variable would be
>> > used for
>> > all FAT implementation instead of allocating additional two global
>> > variables
>> > which are get_denfromdir_block and do_fat_read_at_block. This
>> > implementation
>> > can help in saving up 128KB memory space.
>> >
>> > Signed-off-by: Tien Fong Chee <tfc...@altera.com>
>> > Cc: Dinh Nguyen <dingu...@opensource.altera.com>
>> > Cc: Dinh Nguyen <dinh.li...@gmail.com>
>> > Cc: ChinLiang <cl...@altera.com>
>> > Cc: Vagrant Cascadian <vagr...@debian.org>
>> > Cc: Simon Glass <s...@chromium.org>
>> > Cc: Stephen Warren <swar...@nvidia.com>
>> > Cc: Benoît Thébaudeau <ben...@wsystem.com>
>> > ---
>> >  fs/fat/fat.c |6 ++
>> >  1 files changed, 2 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/fs/fat/fat.c b/fs/fat/fat.c
>> > index 826bd85..5d1afe6 100644
>> > --- a/fs/fat/fat.c
>> > +++ b/fs/fat/fat.c
>> > @@ -579,8 +579,7 @@ static __u8 mkcksum(const char name[8], const
>> > char ext[3])
>> >   * Get the directory entry associated with 'filename' from the
>> > directory
>> >   * starting at 'startsect'
>> >   */
>> > -__u8 get_dentfromdir_block[MAX_CLUSTSIZE]
>> > -   __aligned(ARCH_DMA_MINALIGN);
>> > +__u8 *get_dentfromdir_block = get_contents_vfatname_block;
>> >
>> >  static dir_entry *get_dentfromdir(fsdata *mydata, int startsect,
>> >   char *filename, dir_entry
>> > *retdent,
>> > @@ -811,8 +810,7 @@ exit:
>> > return ret;
>> >  }
>> >
>> > -__u8 do_fat_read_at_block[MAX_CLUSTSIZE]
>> > -   __aligned(ARCH_DMA_MINALIGN);
>> > +__u8 *do_fat_read_at_block = get_contents_vfatname_block;
>> >
>> >  int do_fat_read_at(const char *filename, loff_t pos, void *buffer,
>> >loff_t maxsize, int dols, int dogetsize, loff_t
>> > *size)
>>
>> This probably breaks at least fat_write.c, which uses:
>>   memcpy(get_dentfromdir_block, get_contents_vfatname_block,
>
> With this patch, this line code "memcpy(get_dentfromdir_block,
> get_contents_vfatname_block," is not required anymore since both
>  get_dentfromdir_block and get_contents_vfatname_block are sharing the
> same content and memory. So, this line code can be removed or leaving
> in there. However, there is only one place within fill_dir_slot_buffer
> function where it can corrupt the the global memory, and it is fixed by
> replacing with local buffer. This was sent out with another patch
> called "[PATCH 1/2] fs/fat/fatwrite: Local variable as buffer to store
> dir_slot entries". Overall, applying these two patches, a lot memory
> space can be saved and fitting into small OCRAM, but need to be very
> careful when modifying the code related to global memory.

I get the point, but I am a bit concerned because these changes make
the code even more fragile and hard to maintain than it currently is.
Perhaps it would be time to switch to FatFs as previously suggested.
Here is its memory usage:
http://elm-chan.org/fsw/ff/en/appnote.html#memory

I've not checked in detail all the possible code paths, but for
instance, if I look at get_vfatname(), it's called twice in fat.c,
once with cluster and retdent pointing to somewhere in
get_dentfromdir_block, and once with them pointing to somewhere in
do_fat_read_at_block (through other pointer variables), while
get_vfatname() may write to get_contents_vfatname_block.
get_vfatname() then uses the original data pointed to by slotptr >=
retdent. To make things worse, there is a memcpy (not memmove) from
realdent, which may point to somewhere in get_contents_vfatname_block,
to retdent. It's almost impossible for the involved buffer areas not
to overlap in that case since a whole cluster is read.

I also agree with Stephen's recommendations.

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 2/2] fs/fat/fat: Optimizes memory size with single global variable instead of 3

[Benoît Thébaudeau]

Dear Tien Fong Chee,

On Jul 13, 2016 at 11:01 AM, Tien Fong Chee wrote:
> Single 64KB get_contents_vfatname_block global variable would be used for
> all FAT implementation instead of allocating additional two global variables
> which are get_denfromdir_block and do_fat_read_at_block. This implementation
> can help in saving up 128KB memory space.
> 
> Signed-off-by: Tien Fong Chee <tfc...@altera.com>
> Cc: Dinh Nguyen <dingu...@opensource.altera.com>
> Cc: Dinh Nguyen <dinh.li...@gmail.com>
> Cc: ChinLiang <cl...@altera.com>
> Cc: Vagrant Cascadian <vagr...@debian.org>
> Cc: Simon Glass <s...@chromium.org>
> Cc: Stephen Warren <swar...@nvidia.com>
> Cc: Benoît Thébaudeau <ben...@wsystem.com>
> ---
>  fs/fat/fat.c |6 ++
>  1 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> index 826bd85..5d1afe6 100644
> --- a/fs/fat/fat.c
> +++ b/fs/fat/fat.c
> @@ -579,8 +579,7 @@ static __u8 mkcksum(const char name[8], const char ext[3])
>   * Get the directory entry associated with 'filename' from the directory
>   * starting at 'startsect'
>   */
> -__u8 get_dentfromdir_block[MAX_CLUSTSIZE]
> - __aligned(ARCH_DMA_MINALIGN);
> +__u8 *get_dentfromdir_block = get_contents_vfatname_block;
>  
>  static dir_entry *get_dentfromdir(fsdata *mydata, int startsect,
> char *filename, dir_entry *retdent,
> @@ -811,8 +810,7 @@ exit:
>   return ret;
>  }
>  
> -__u8 do_fat_read_at_block[MAX_CLUSTSIZE]
> - __aligned(ARCH_DMA_MINALIGN);
> +__u8 *do_fat_read_at_block = get_contents_vfatname_block;
>  
>  int do_fat_read_at(const char *filename, loff_t pos, void *buffer,
>  loff_t maxsize, int dols, int dogetsize, loff_t *size)

This probably breaks at least fat_write.c, which uses:
  memcpy(get_dentfromdir_block, get_contents_vfatname_block,

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v3 2/2] nand: extend nand torture

[Benoît Thébaudeau]

erasing bad blocks (UNSAFE)\n"
> diff --git a/doc/README.nand b/doc/README.nand
> index 96ffc48..4ecf9de 100644
> --- a/doc/README.nand
> +++ b/doc/README.nand
> @@ -307,7 +307,7 @@ Miscellaneous and testing commands:
>DANGEROUS!!! Factory set bad blocks will be lost. Use only
>to remove artificial bad blocks created with the "markbad" command.
>
> -  "torture offset"
> +  "torture offset [size]"
>Torture block to determine if it is still reliable.
>Enabled by the CONFIG_CMD_NAND_TORTURE configuration option.
>This command returns 0 if the block is still reliable, else 1.
> @@ -324,6 +324,10 @@ Miscellaneous and testing commands:
>    automate actions following a nand->write() error. This would e.g. be 
> required
>in order to program or update safely firmware to NAND, especially for the 
> UBI
>part of such firmware.
> +  Optionally, a second parameter size can be given to test multiple blocks 
> with
> +  one call. If size is not a multiple of the NAND's erase size, then the 
> block
> +  that contains offset + size will be tested in full. If used with size, this
> +  command returns 0 if all tested blocks have been found reliable, else 1.
>
>
>  NAND locking command (for chips with active LOCKPRE pin)
> --
> 2.5.5
>

Reviewed-by: Benoît Thébaudeau <benoit.thebaudeau@gmail.com>

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v2 2/2] nand: extend nand torture

[Benoît Thébaudeau]

Hi Max,

On Thu, Jun 9, 2016 at 3:19 PM, Max Krummenacher <max.oss...@gmail.com> wrote:
> Hi Benoît,
>
> 2016-06-09 1:41 GMT+02:00 Benoît Thébaudeau <benoit.thebaudeau@gmail.com>:
>> On Tue, Jun 7, 2016 at 1:46 PM, Max Krummenacher <max.oss...@gmail.com> 
>> wrote:
>>> diff --git a/cmd/nand.c b/cmd/nand.c
>>> index 583a18f..8ade5e2 100644
>>> --- a/cmd/nand.c
>>> +++ b/cmd/nand.c
[...]
>> +if (argc > 3 && !str2off(argv[3], )) {
>
> Here I prefer having that in 2 if() as the stuff tested is only loosely 
> related.

Usually, we keep things as compact as possible, which also limits the
number of indentation levels, but that's fine if you prefer otherwise.
I don't think it's a strong rule.

> I guess keeping it like this would also require parantheses around (argc > 3).

No: `>` has higher precedence than `&&`.

> Will revert to two if's in v3
>
>> +puts("Size is not a valid number\n");
>> +return 1;
>> +}
>>
>> -return ret == 0 ? 0 : 1;
>> +endoff = off + size;
>> +if (endoff > mtd->size) {
>> +puts("Arguments beyond end of NAND\n");
>> +return 1;
>> +}
>> +
>> +off = round_down(off, mtd->erasesize);
>> +endoff = round_up(endoff, mtd->erasesize);
>> +size = endoff - off;
>> +printf("\nNAND torture: device %d offset 0x%llx size 0x%llx "
>> +"(block size 0x%x)\n",
>
> patman.py/checkpatch.py warn here to keep quoted strings on one line
> even when the line length exceeds 80 characters.
> Will remove the line break / string concatenation for v3.

Normally, this rule is for grep-ability. Here, it's more complicated
with the '%' in-between, but it's still makes sense with a regular
expression, so OK.

>> +dev, off, size, mtd->erasesize);
>> +while (off < endoff) {
>> +ret = nand_torture(mtd, off);
>> +if (ret) {
>> +failed++;
>> +printf("  block at 0x%llx failed\n", off);
>> +} else {
>> +passed++;
>> +}
>> +off += mtd->erasesize;
>> +}
>> +printf(" Passed: %u, failed: %u\n", passed, failed);
>> +return failed != 0;
>>
>>> }
>>>  #endif
>
> Apart from above comments I merged your proposal.
>
>>>
>>> @@ -775,7 +792,8 @@ static char nand_help_text[] =
> ...
>>> diff --git a/doc/README.nand b/doc/README.nand
>>> index 96ffc48..5136f31 100644
>>> --- a/doc/README.nand
>>> +++ b/doc/README.nand
>>> @@ -307,7 +307,7 @@ Miscellaneous and testing commands:
>>>DANGEROUS!!! Factory set bad blocks will be lost. Use only
>>>to remove artificial bad blocks created with the "markbad" command.
>>>
>>> -  "torture offset"
>>> +  "torture offset [size]"
>>>Torture block to determine if it is still reliable.
>>>Enabled by the CONFIG_CMD_NAND_TORTURE configuration option.
>>>This command returns 0 if the block is still reliable, else 1.
>>> @@ -324,6 +324,10 @@ Miscellaneous and testing commands:
>>>automate actions following a nand->write() error. This would e.g. be 
>>> required
>>>in order to program or update safely firmware to NAND, especially for 
>>> the UBI
>>>part of such firmware.
>>> +  Optionally a second parameter size can be given to test multiple blocks 
>>> with
>>
>> "Optionally,"
>>
>>> +  one call. If size is not a multiple of the NAND's erasesize then the 
>>> block
>>
>> "erase size, then"
>>
>>> +  which contains offset + size will be tested in full. If used with size 
>>> this
>>
>> "that", not "which".
>> "size, this"
>>
>
> Agreed. Will add that to v3.

Thanks.

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v2 2/2] nand: extend nand torture

[Benoît Thébaudeau]

Hi Max,

On Tue, Jun 7, 2016 at 1:46 PM, Max Krummenacher  wrote:
> nand torture currently works on exactly one nand block which is specified
> by giving the byteoffset to the beginning of the block.
>
> Extend this by allowing for a second parameter specifying the byte size
> to be tested.
>
> e.g.
> ==> nand torture 100
>
> NAND torture: device 0 offset 0x100 size 0x2 (nand block size 0x2)
> passed 1, failed 0
>
> ==> nand torture 100 4
>
> NAND torture: device 0 offset 0x100 size 0x4 (nand block size 0x2)
> passed 2, failed 0
>
> Signed-off-by: Max Krummenacher 
>
> ---
>
> Changes in v2:
> - findings from Benoît:
>   - change interface to be offset/size
>   - change the output to include both 'size tested' and 'nand block size'
>   - updated doc/README.nand accordingly
>   - I did not implement the suggestion to move the code into the
> nand_torture() function. Likely one uses the extended functionality
> only during HW bringup interactively. If one would want to test
> multiple blocks from code one would also want to know the testresult
> of each individual block rather than only having a return parameter
> indicating a 'all good' or 'at least one block failed'.
>
>  cmd/nand.c  | 34 ++
>  doc/README.nand |  6 +-
>  2 files changed, 31 insertions(+), 9 deletions(-)
>
> diff --git a/cmd/nand.c b/cmd/nand.c
> index 583a18f..8ade5e2 100644
> --- a/cmd/nand.c
> +++ b/cmd/nand.c
> @@ -647,6 +647,8 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, 
> char * const argv[])
>
>  #ifdef CONFIG_CMD_NAND_TORTURE
> if (strcmp(cmd, "torture") == 0) {
> +   loff_t endoff;
> +   unsigned failed = 0, passed = 0;
> if (argc < 3)
> goto usage;
>
> @@ -654,13 +656,28 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int 
> argc, char * const argv[])
> puts("Offset is not a valid number\n");
> return 1;
> }
> -
> -   printf("\nNAND torture: device %d offset 0x%llx size 0x%x\n",
> -   dev, off, mtd->erasesize);
> -   ret = nand_torture(mtd, off);
> -   printf(" %s\n", ret ? "Failed" : "Passed");
> -
> -   return ret == 0 ? 0 : 1;
> +   size = mtd->erasesize;
> +   if (argc > 3)
> +   if (!str2off(argv[3], )) {
> +   puts("Size is not a valid number\n");
> +   return 1;
> +   }
> +   printf("\nNAND torture: device %d offset 0x%llx size 0x%llx 
> (nand block size 0x%x)\n",
> +  dev, off, size, mtd->erasesize);
> +
> +   endoff = off + size;
> +   while (off < endoff) {
> +   ret = nand_torture(mtd, off);
> +   if (ret) {
> +   failed++;
> +   printf(" off 0x%llx %s\n", off, "Failed");
> +   } else {
> +   passed++;
> +   }
> +   off += mtd->erasesize;
> +   }
> +   printf("passed %u, failed %u\n", passed, failed);
> +   return failed == 0 ? 0 : 1;

The given offset could also start anywhere, so it's better to
auto-align it like the size.

If the arguments extend beyond the end of the flash, then
nand_torture() will return an error at each iteration, so it's better
to break the loop or not to start it in this case.

It's better to print the range actually tortured than the arguments
from the user.

So what about the following?

@@ -647,6 +647,9 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int
argc, char * const argv[])

 #ifdef CONFIG_CMD_NAND_TORTURE
 if (strcmp(cmd, "torture") == 0) {
+loff_t endoff;
+unsigned int failed = 0, passed = 0;
+
 if (argc < 3)
 goto usage;

@@ -655,12 +658,36 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag,
int argc, char * const argv[])
 return 1;
 }

-printf("\nNAND torture: device %d offset 0x%llx size 0x%x\n",
-dev, off, mtd->erasesize);
-ret = nand_torture(mtd, off);
-printf(" %s\n", ret ? "Failed" : "Passed");
+size = mtd->erasesize;
+if (argc > 3 && !str2off(argv[3], )) {
+puts("Size is not a valid number\n");
+return 1;
+}

-return ret == 0 ? 0 : 1;
+endoff = off + size;
+if (endoff > mtd->size) {
+puts("Arguments beyond end of NAND\n");
+return 1;
+}
+
+off = round_down(off, mtd->erasesize);
+endoff = round_up(endoff, mtd->erasesize);
+size = endoff - off;
+printf("\nNAND torture: device %d offset 0x%llx size 

Re: [U-Boot] [PATCH 2/2] nand: extend nand torture

[Benoît Thébaudeau]

Hi Max,

On Tue, Jun 7, 2016 at 12:57 PM, Max Krummenacher <max.oss...@gmail.com> wrote:
> Hi Benoît,
>
> Thank you for your review.

You're welcome.

> I wanted to wait for Scott's patchseries to make it into master to
> allow for potential needed
> changes.

No problem.

> 2016-05-31 22:21 GMT+02:00 Benoît Thébaudeau 
> <benoit.thebaudeau@gmail.com>:
> ...
>>> Extend this by allowing for a second parameter specifying the byte offset
>>> to the last block to be tested.
>>>
>>
>> End offsets are always ambiguous because users can hesitate between
>> the offset of the first byte of the last block, the offset of the last
>> byte of the last block, and the offset of the first byte of the block
>> following the last one (if any). A byte size would probably be better
>> here, and it would also be more consistent with the other nand
>> commands.
>
> Ok. I will change the interface to use offset/size.

Good.

> ...
>>> NAND torture: device 0 offset 0x100 size 0x2
>>> passed 2, failed 0
>>>
>>
>> With more than one block to test, the printed size becomes ambiguous
>> here. It would be better to indicate that it is the erase size of the
>> block. The total test size could also be printed, either instead of
>> the erase size, or besides it.
>>
> Ok. I will change this to print test size and block size, e.g. like:
> NAND torture: device 0 offset 0x100 size 0x4 (nand block size 0x2)

Good.

> ...
>>> -   return ret == 0 ? 0 : 1;
>>> +   ret = nand_torture(nand, off);
>>> +   if (ret) {
> ...
>>
>> A size parameter could probably be added to nand_torture() instead of
>> handling the range in the command, so that the direct usages of
>> nand_torture() (in or out of tree) can also benefit from this
>> enhancement.
>
> I disagree here.
> Likely one uses the extended functionality only during HW bringup and
> only interactively. If one would want to test multiple blocks from code one
> would also want to know the testresult of each individual block rather
> than only having a return parameter indicating a 'all good' or
> 'at least one block failed'.
> Even here in the interactive 'nand torture' cmd the printf of a failed
> block in the loop
> is a usecase of this.

Makes sense. Agreed.

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 2/2] nand: extend nand torture

[Benoît Thébaudeau]

Dear Max Krummenacher,

On Mon, May 30, 2016 at 4:28 PM, Max Krummenacher  wrote:
> nand torture currently works on exactly one nand block which is specified
> by giving the byteoffset to the beginning of the block.
>
> Extend this by allowing for a second parameter specifying the byte offset
> to the last block to be tested.
>

End offsets are always ambiguous because users can hesitate between
the offset of the first byte of the last block, the offset of the last
byte of the last block, and the offset of the first byte of the block
following the last one (if any). A byte size would probably be better
here, and it would also be more consistent with the other nand
commands.

> e.g.
> ==> nand torture 100
>
> NAND torture: device 0 offset 0x100 size 0x2
> passed 1, failed 0
>
> ==> nand torture 100 104
>
> NAND torture: device 0 offset 0x100 size 0x2
> passed 2, failed 0
>

With more than one block to test, the printed size becomes ambiguous
here. It would be better to indicate that it is the erase size of the
block. The total test size could also be printed, either instead of
the erase size, or besides it.

> Signed-off-by: Max Krummenacher 
> ---
>
>  cmd/nand.c | 25 +
>  1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/cmd/nand.c b/cmd/nand.c
> index a6b67e2..615dbd5 100644
> --- a/cmd/nand.c
> +++ b/cmd/nand.c
> @@ -646,6 +646,8 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, 
> char * const argv[])
>
>  #ifdef CONFIG_CMD_NAND_TORTURE
> if (strcmp(cmd, "torture") == 0) {
> +   loff_t endoff;
> +   unsigned failed = 0, passed = 0;
> if (argc < 3)
> goto usage;
>
> @@ -653,13 +655,27 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int 
> argc, char * const argv[])
> puts("Offset is not a valid number\n");
> return 1;
> }
> -
> +   endoff = off + nand->erasesize;
> +   if (argc > 3)
> +   if (!str2off(argv[3], )) {
> +   puts("End is not a valid number\n");
> +   return 1;
> +   }
> printf("\nNAND torture: device %d offset 0x%llx size 0x%x\n",
> dev, off, nand->erasesize);
> -   ret = nand_torture(nand, off);
> -   printf(" %s\n", ret ? "Failed" : "Passed");
> +   while (off < endoff) {
>
> -   return ret == 0 ? 0 : 1;
> +   ret = nand_torture(nand, off);
> +   if (ret) {
> +   failed++;
> +   printf(" off 0x%llx %s\n", off, "Failed");
> +   } else {
> +   passed++;
> +   }
> +   off += nand->erasesize;
> +   }
> +   printf("passed %u, failed %u\n", passed, failed);
> +   return failed == 0 ? 0 : 1;
> }
>  #endif

A size parameter could probably be added to nand_torture() instead of
handling the range in the command, so that the direct usages of
nand_torture() (in or out of tree) can also benefit from this
enhancement.

>
> @@ -775,6 +791,7 @@ static char nand_help_text[] =
> "nand dump[.oob] off - dump page\n"
>  #ifdef CONFIG_CMD_NAND_TORTURE
> "nand torture off - torture block at offset\n"
> +   "nand torture start end - torture blocks from start to end offset\n"
>  #endif
> "nand scrub [-y] off size | scrub.part partition | scrub.chip\n"
> "really clean NAND erasing bad blocks (UNSAFE)\n"

doc/README.nand should also be updated accordingly.

> --
> 2.5.5

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 03/13] imx-common: use simpler runtime cpu dection macros

[Benoît Thébaudeau]

Hi Peng,

On Fri, May 20, 2016 at 10:41 AM, Peng Fan <van.free...@gmail.com> wrote:
> On Fri, May 20, 2016 at 01:02:10AM +0200, Benoît Thébaudeau wrote:
>>On Wed, May 18, 2016 at 9:53 AM, Peng Fan <van.free...@gmail.com> wrote:
>>> Use simpler runtime cpu dection macros.
>>>
>>> Signed-off-by: Peng Fan <van.free...@gmail.com>
>>> Cc: Stefano Babic <sba...@denx.de>
>>> Cc: Ulises Cardenas <ulises.carde...@freescale.com>
>>> Cc: Bhuvanchandra DV <bhuvanchandra...@toradex.com>
>>> Cc: "Benoît Thébaudeau" <benoit.thebaudeau@gmail.com>
>>> ---
>>>  arch/arm/imx-common/hab.c  | 43 
>>> +-
>>>  arch/arm/imx-common/init.c |  5 ++---
>>>  arch/arm/imx-common/iomux-v3.c |  2 +-
>>>  arch/arm/imx-common/sata.c |  2 +-
>>>  arch/arm/imx-common/timer.c| 11 +++
>>>  5 files changed, 20 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/arch/arm/imx-common/hab.c b/arch/arm/imx-common/hab.c
>>> index 8bbcc22..a980688 100644
>>> --- a/arch/arm/imx-common/hab.c
>>> +++ b/arch/arm/imx-common/hab.c
>>> @@ -17,60 +17,45 @@
>>>
>>>  #define hab_rvt_report_event_p \
>>>  (  \
>>> -   ((is_cpu_type(MXC_CPU_MX6Q) ||  \
>>> - is_cpu_type(MXC_CPU_MX6D)) && \
>>> - (soc_rev() >= CHIP_REV_1_5)) ?\
>>> +   (is_mx6dq() && (soc_rev() >= CHIP_REV_1_5)) ?   \
>>> ((hab_rvt_report_event_t *)HAB_RVT_REPORT_EVENT_NEW) :  \
>>> -   (is_cpu_type(MXC_CPU_MX6DL) &&  \
>>> -(soc_rev() >= CHIP_REV_1_2)) ? \
>>> +   (is_mx6sdl() && (soc_rev() >= CHIP_REV_1_2)) ?  \
>>
>>This change silently introduces the possibility of MXC_CPU_MX6SOLO. Is
>>this on purpose? If so, then it means that there was something
>
> Yeah. on purpose.
>
>>unrelated to this patch that was wrong in this code for
>>MXC_CPU_MX6SOLO, so this should be fixed in a separate patch before
>>this one. If not, then an is_mx6dl() macro should be introduced.
>
> 6solo and 6dl works the same. I do not plan to add a is_mx6dl here.
> I can refine the commit log to note this in V2.

Then you can indeed either mention this in the commit message, or add
a patch fixing 6solo support before this one.

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 03/13] imx-common: use simpler runtime cpu dection macros

[Benoît Thébaudeau]

Dear Peng Fan,

On Wed, May 18, 2016 at 9:53 AM, Peng Fan <van.free...@gmail.com> wrote:
> Use simpler runtime cpu dection macros.
>
> Signed-off-by: Peng Fan <van.free...@gmail.com>
> Cc: Stefano Babic <sba...@denx.de>
> Cc: Ulises Cardenas <ulises.carde...@freescale.com>
> Cc: Bhuvanchandra DV <bhuvanchandra...@toradex.com>
> Cc: "Benoît Thébaudeau" <benoit.thebaudeau@gmail.com>
> ---
>  arch/arm/imx-common/hab.c  | 43 
> +-
>  arch/arm/imx-common/init.c |  5 ++---
>  arch/arm/imx-common/iomux-v3.c |  2 +-
>  arch/arm/imx-common/sata.c |  2 +-
>  arch/arm/imx-common/timer.c| 11 +++
>  5 files changed, 20 insertions(+), 43 deletions(-)
>
> diff --git a/arch/arm/imx-common/hab.c b/arch/arm/imx-common/hab.c
> index 8bbcc22..a980688 100644
> --- a/arch/arm/imx-common/hab.c
> +++ b/arch/arm/imx-common/hab.c
> @@ -17,60 +17,45 @@
>
>  #define hab_rvt_report_event_p \
>  (  \
> -   ((is_cpu_type(MXC_CPU_MX6Q) ||  \
> - is_cpu_type(MXC_CPU_MX6D)) && \
> - (soc_rev() >= CHIP_REV_1_5)) ?\
> +   (is_mx6dq() && (soc_rev() >= CHIP_REV_1_5)) ?   \
> ((hab_rvt_report_event_t *)HAB_RVT_REPORT_EVENT_NEW) :  \
> -   (is_cpu_type(MXC_CPU_MX6DL) &&  \
> -(soc_rev() >= CHIP_REV_1_2)) ? \
> +   (is_mx6sdl() && (soc_rev() >= CHIP_REV_1_2)) ?  \

This change silently introduces the possibility of MXC_CPU_MX6SOLO. Is
this on purpose? If so, then it means that there was something
unrelated to this patch that was wrong in this code for
MXC_CPU_MX6SOLO, so this should be fixed in a separate patch before
this one. If not, then an is_mx6dl() macro should be introduced.

> ((hab_rvt_report_event_t *)HAB_RVT_REPORT_EVENT_NEW) :  \
> ((hab_rvt_report_event_t *)HAB_RVT_REPORT_EVENT)\
>  )
>
>  #define hab_rvt_report_status_p\
>  (  \
> -   ((is_cpu_type(MXC_CPU_MX6Q) ||  \
> - is_cpu_type(MXC_CPU_MX6D)) && \
> - (soc_rev() >= CHIP_REV_1_5)) ?\
> +   (is_mx6dq() && (soc_rev() >= CHIP_REV_1_5)) ?   \
> ((hab_rvt_report_status_t *)HAB_RVT_REPORT_STATUS_NEW) :\
> -   (is_cpu_type(MXC_CPU_MX6DL) &&  \
> -(soc_rev() >= CHIP_REV_1_2)) ? \
> +   (is_mx6sdl() && (soc_rev() >= CHIP_REV_1_2)) ?  \

Ditto.

> ((hab_rvt_report_status_t *)HAB_RVT_REPORT_STATUS_NEW) :\
> ((hab_rvt_report_status_t *)HAB_RVT_REPORT_STATUS)  \
>  )
>
>  #define hab_rvt_authenticate_image_p   \
>  (  \
> -   ((is_cpu_type(MXC_CPU_MX6Q) ||  \
> - is_cpu_type(MXC_CPU_MX6D)) && \
> - (soc_rev() >= CHIP_REV_1_5)) ?\
> +   (is_mx6dq() && (soc_rev() >= CHIP_REV_1_5)) ?   \
> ((hab_rvt_authenticate_image_t *)HAB_RVT_AUTHENTICATE_IMAGE_NEW) : \
> -   (is_cpu_type(MXC_CPU_MX6DL) &&  \
> -(soc_rev() >= CHIP_REV_1_2)) ? \
> +   (is_mx6sdl() && (soc_rev() >= CHIP_REV_1_2)) ?  \

Ditto.

> ((hab_rvt_authenticate_image_t *)HAB_RVT_AUTHENTICATE_IMAGE_NEW) : \
> ((hab_rvt_authenticate_image_t *)HAB_RVT_AUTHENTICATE_IMAGE)\
>  )
>
>  #define hab_rvt_entry_p\
>  (  \
> -   ((is_cpu_type(MXC_CPU_MX6Q) ||  \
> - is_cpu_type(MXC_CPU_MX6D)) && \
> - (soc_rev() >= CHIP_REV_1_5)) ?\
> +   (is_mx6dq() && (soc_rev() >= CHIP_REV_1_5)) ?   \
> ((hab_rvt_entry_t *)HAB_RVT_ENTRY_NEW) :\
> -   (is_cpu_type(MXC_CPU_MX6DL) &&  \
> -(soc_rev() >= CHIP_REV_1_2)) ? \
> +   (is_mx6sdl() && (soc_rev() >= CHIP_REV_1_2)) ?  \

Ditto.

> ((hab_rvt_entry_t *)HAB_RVT_ENTRY_NEW) :\
> ((hab_

Re: [U-Boot] Linker script u-boot.lds makes u-boot ELF not load with debugger

[Benoît Thébaudeau]

Hi Héctor,

On Wed, May 18, 2016 at 1:28 PM, Palacios, Hector
 wrote:
> I'm loading U-Boot to an i.MX6Q platform using ARM DSTREAM debugger and after 
> running the DDR initialization script it fails to load the u-boot ELF binary 
> complaining with:
>
> loadfile "/home/hpalacio/git/u-boot-denx/u-boot"
> ERROR(CMD16-TAD11-NAL18):
> ! Failed to load "u-boot"
> ! Failed to write 160 bytes to address S:0x00010034
> ! Bus error on memory operation.
> Target Message: Memory access caused precise abort.
> Debug Precise Abort Registers : DFSR = 0x1808, DFAR = 0x00010034
>
>
> If I revert the changes introduced to arch/arm/cpu/u-boot.lds in commit 
> 47ed5dd031d7d2c587e6afd386e79ccec1a1b7f7, then the u-boot ELF loads fine:
[...]
>
> Does anybody know if I should do anything special in my debugger 
> initialization script after commit 47ed5dd031d7d2c587e6afd386e79ccec1a1b7f7 
> to have it load U-Boot normally?

As explained in the commit message
(http://git.denx.de/?p=u-boot.git;a=commitdiff;h=47ed5dd031d7d2c587e6afd386e79ccec1a1b7f7),
this change was required in order to make diagnostic tools such as
readelf or objdump produce thorough output. A consequence is that some
generated ELF files now have some program headers (see
https://sourceware.org/binutils/docs/ld/PHDRS.html#PHDRS) that are
incompatible with debuggers such as yours because they order these
tools to write to non-writable addresses.

One solution is to program with the debugger u-boot.bin or another
binary objcopied from the original ELF file.

Another solution is not to use any debugger to copy the image to the
target RAM, but instead to program u-boot.imx to the boot device
(NAND, SD, etc.), in which case you have to instruct the debugger to
halt execution after reset. You will then even be able to see the ROM
bootloader load and boot the binary image from the boot device.
However, this solution may be a bit less convenient for frequent
target programming.

In the longer term, it would be possible to check the current ELF
program headers in order to fix the linker scripts for this use case,
but doing this in a scalable way for all the targets might prove
difficult and maybe not worth it. I think that being able to program a
binary file with debuggers is sufficient. Albert, what do you think?

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] Inconsistencies in commands regarding load_addr

[Benoît Thébaudeau]

Dear Wolfgang,

On 08/10/2015 23:29, Wolfgang Denk wrote:
> Dear Stephen,
> 
> In message <56167db6.3000...@wwwdotorg.org> you wrote:
>>
 What's the expected correct behavior here?
>>>
>>> After successful loading the data to memory, load_addr should be set
>>> correctly, for all commands.  In the error case, the value of
>>> load_addr is undefined.
>>
>> Is this documented anywhere? If not, I'm not convinced that there's a 
>> contract to be followed; it "just happens" that some filesystem-related 
>> commands work(ed) that way (and as Benoît pointed out, apparently some 
>> don't irrespective of the mentioned patch).
> 
> I'm afraid it's not documented, but it is what I would consider a sane
> and consistent behaviour.  If we intend to implement POLA [1] (and I
> very much think we should), this is how U-Boot should behave.
> 
> 
> [1] https://en.wikipedia.org/wiki/Principle_of_least_astonishment

I'm not certain that this would be the least astonishing behavior. When I read
the documentation, I rather expect the loadaddr environment variable to be used
whenever the address is omitted in a command invocation. Moreover, one may have
to read/load several data pieces before booting, and the last loaded piece would
not necessarily be the one containing the kernel to be booted. This should at
least be documented.

Another approach would be to compel users to pass an address for all commands.
Implicit behaviors are always dangerous, all the more if they are undocumented.
But of course, this would break some existing configurations.

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] Inconsistencies in commands regarding load_addr

[Benoît Thébaudeau]

Dear Wolfgang,

On 09/10/2015 15:18, Wolfgang Denk wrote:
> Regarding the "load address" topic, be careful, as there has always
> been a lot of confusion (due to unfortunate historic choice of names).
> There is the "load address" as part of the image formates (uImage, FIT
> image), which means the address where the image (OS code) gets loaded
> (or even uncompressed) _to_.  This is recorded in the image itself,
> and has nothing to do woth the "loadaddr" variable, which states where
> the image is located in system memory.

Indeed, but I was only referring to the load address below.

> A command, that _loads_ an image to memory, should either use the
> current setting of "loadaddr" (if no argument is given), of, if the
> argument is given, set "loadaddr" to that value, so that further
> commands can refer to that address by default.

Makes sense.

Currently, it's all mixed up between CONFIG_SYS_LOAD_ADDR, the loadaddr
environment variable and the load_addr global C variable.

The 1st issue is that loadaddr and load_addr currently diverge if the user
changes loadaddr or if commands change load_addr.

The 2nd issue is that some commands use the value of loadaddr as a default,
whereas others use load_addr. And if that fails, CONFIG_SYS_LOAD_ADDR is
sometimes used as a fallback value.

The 3rd issue is that some read/load commands set load_addr, but not all (e.g.:
mmc read, ext2load), which breaks the whole feature, but fixing this could break
existing configurations relying on the current behavior.

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 1/5] fs/fat/fat_write: Fix buffer alignments

[Benoît Thébaudeau]

Hi Tom,

On Mon, Sep 28, 2015 at 5:22 PM, Tom Rini <tr...@konsulko.com> wrote:
> On Mon, Sep 28, 2015 at 03:45:28PM +0200, Benoît Thébaudeau wrote:
>
>> set_cluster() was using a temporary buffer without enforcing its
>> alignment for DMA and cache. Moreover, it did not check the alignment of
>> the passed buffer, which can come directly from applicative code or from
>> the user.
>>
>> This could cause random data corruption, which has been observed on
>> i.MX25 writing to an SD card.
>>
>> Fix this by only passing ARCH_DMA_MINALIGN-aligned buffers to
>> disk_write(), which requires the introduction of a buffer bouncing
>> mechanism for the misaligned buffers passed to set_cluster().
>>
>> By the way, improve the handling of the corresponding return values from
>> disk_write():
>>  - print them with debug() in case of error,
>>  - consider that there is an error is disk_write() returns a smaller
>>block count than the requested one, not only if its return value is
>>negative.
>>
>> After this change, set_cluster() and get_cluster() are almost
>> symmetrical.
>>
>> Signed-off-by: Benoît Thébaudeau <ben...@wsystem.com>
>
> OK.  I know Stephen has a series to replace all of the FAT code for
> the next release once some performance issues are addressed.  But I'm
> inclined to take this series (after some reviews and so forth) for this
> release at least because this sounds like some bad bugs and more things
> are starting to rely on fatwrite functionality (for example, env saved
> as a file in FAT is getting common on community-style boards).

I agree, but there is not much review activity. Do you know anyone
else who might be interested in this and who should be Cc'ed?

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] Inconsistencies in commands regarding load_addr

[Benoît Thébaudeau]

Hi all,

I've just noticed that before the commit
045fa1e1142552799ad3203e9e0bc22a11e866ea, ext2load and ext4load were setting the
load_addr global variable, but not fatload. Since then, none of these commands
set load_addr (initially derived from the loadaddr environment variable).

ubifsload also does not set load_addr, but a quick grep shows that some other
filesystem commands set it, e.g. for zfs, jffs2, reiser or cramfs.

Also, some commands set it only on success, while some other commands set it
from the command line arguments unconditionally.

What's the expected correct behavior here?

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] Inconsistencies in commands regarding load_addr

[Benoît Thébaudeau]

On Tue, Oct 6, 2015 at 8:09 PM, Stephen Warren <swar...@wwwdotorg.org> wrote:
> On 10/06/2015 09:00 AM, Benoît Thébaudeau wrote:
>>
>> Hi all,
>>
>> I've just noticed that before the commit
>> 045fa1e1142552799ad3203e9e0bc22a11e866ea, ext2load and ext4load were
>> setting the
>> load_addr global variable, but not fatload. Since then, none of these
>> commands
>> set load_addr (initially derived from the loadaddr environment variable).
>
>
> Oh dear; I see that has indeed changed. Still, it's been 3 years so I
> imagine nobody was using the feature?

Probably.

>> ubifsload also does not set load_addr, but a quick grep shows that some
>> other
>> filesystem commands set it, e.g. for zfs, jffs2, reiser or cramfs.
>>
>> Also, some commands set it only on success, while some other commands set
>> it
>> from the command line arguments unconditionally.
>>
>> What's the expected correct behavior here?
>
>
> I'm not quite sure how useful the behaviour is; I'd tend towards not setting
> $load_addr. If some script wants it set, it can easily do it itself.
>
> Did you just notice this while reading code, or does this break some
> existing use-case? If the latter, it seems reasonable to add the
> previously-working feature back.

Actually, I was working on an older version of U-Boot based on
2012.07, and I got an issue using bootm without any arguments because
ext2load was unexpectedly setting load_addr (this was undocumented).
So I checked how things evolved in mainline in the meantime, and I
noticed the change. I prefer the new behavior, but still, not all
current commands do the same.

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 2/5] fs/fat/fat_write: Merge calls to set_cluster()

[Benoît Thébaudeau]

set_contents() had uselessly split calls to set_cluster(). Merge these
calls, which removes some cases of set_cluster() being called with a
size of zero.

Signed-off-by: Benoît Thébaudeau <ben...@wsystem.com>
---
 fs/fat/fat_write.c | 13 +
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
index d0d9df7..e08cf83 100644
--- a/fs/fat/fat_write.c
+++ b/fs/fat/fat_write.c
@@ -728,21 +728,10 @@ set_contents(fsdata *mydata, dir_entry *dentptr, __u8 
*buffer,
endclust = newclust;
actsize += bytesperclust;
}
-   /* actsize >= file size */
-   actsize -= bytesperclust;
-   /* set remaining clusters */
-   if (set_cluster(mydata, curclust, buffer, (int)actsize) != 0) {
-   debug("error: writing cluster\n");
-   return -1;
-   }
 
/* set remaining bytes */
-   *gotsize += actsize;
-   filesize -= actsize;
-   buffer += actsize;
actsize = filesize;
-
-   if (set_cluster(mydata, endclust, buffer, (int)actsize) != 0) {
+   if (set_cluster(mydata, curclust, buffer, (int)actsize) != 0) {
debug("error: writing cluster\n");
return -1;
}
-- 
2.1.4

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 4/5] fs/fat/fat_write: Factor out duplicate code

[Benoît Thébaudeau]

Signed-off-by: Benoît Thébaudeau <ben...@wsystem.com>
---
 fs/fat/fat_write.c | 72 +-
 1 file changed, 22 insertions(+), 50 deletions(-)

diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
index 2399844..2d032ee 100644
--- a/fs/fat/fat_write.c
+++ b/fs/fat/fat_write.c
@@ -1028,10 +1028,7 @@ static int do_fat_write(const char *filename, void 
*buffer, loff_t size,
if (retdent) {
/* Update file size and start_cluster in a directory entry */
retdent->size = cpu_to_le32(size);
-   start_cluster = FAT2CPU16(retdent->start);
-   if (mydata->fatsize == 32)
-   start_cluster |=
-   (FAT2CPU16(retdent->starthi) << 16);
+   start_cluster = START(retdent);
 
ret = check_overflow(mydata, start_cluster, size);
if (ret) {
@@ -1044,29 +1041,6 @@ static int do_fat_write(const char *filename, void 
*buffer, loff_t size,
printf("Error: clearing FAT entries\n");
goto exit;
}
-
-   ret = set_contents(mydata, retdent, buffer, size, actwrite);
-   if (ret < 0) {
-   printf("Error: writing contents\n");
-   goto exit;
-   }
-   debug("attempt to write 0x%llx bytes\n", *actwrite);
-
-   /* Flush fat buffer */
-   ret = flush_fat_buffer(mydata);
-   if (ret) {
-   printf("Error: flush fat buffer\n");
-   goto exit;
-   }
-
-   /* Write directory table to device */
-   ret = set_cluster(mydata, dir_curclust,
-   get_dentfromdir_block,
-   mydata->clust_size * mydata->sect_size);
-   if (ret) {
-   printf("Error: writing directory entry\n");
-   goto exit;
-   }
} else {
/* Set short name to set alias checksum field in dir_slot */
set_name(empty_dentptr, filename);
@@ -1088,31 +1062,29 @@ static int do_fat_write(const char *filename, void 
*buffer, loff_t size,
fill_dentry(mydata, empty_dentptr, filename,
start_cluster, size, 0x20);
 
-   ret = set_contents(mydata, empty_dentptr, buffer, size,
-  actwrite);
-   if (ret < 0) {
-   printf("Error: writing contents\n");
-   goto exit;
-   }
-   debug("attempt to write 0x%llx bytes\n", *actwrite);
-
-   /* Flush fat buffer */
-   ret = flush_fat_buffer(mydata);
-   if (ret) {
-   printf("Error: flush fat buffer\n");
-   goto exit;
-   }
-
-   /* Write directory table to device */
-   ret = set_cluster(mydata, dir_curclust,
-   get_dentfromdir_block,
-   mydata->clust_size * mydata->sect_size);
-   if (ret) {
-   printf("Error: writing directory entry\n");
-   goto exit;
-   }
+   retdent = empty_dentptr;
}
 
+   ret = set_contents(mydata, retdent, buffer, size, actwrite);
+   if (ret < 0) {
+   printf("Error: writing contents\n");
+   goto exit;
+   }
+   debug("attempt to write 0x%llx bytes\n", *actwrite);
+
+   /* Flush fat buffer */
+   ret = flush_fat_buffer(mydata);
+   if (ret) {
+   printf("Error: flush fat buffer\n");
+   goto exit;
+   }
+
+   /* Write directory table to device */
+   ret = set_cluster(mydata, dir_curclust, get_dentfromdir_block,
+   mydata->clust_size * mydata->sect_size);
+   if (ret)
+   printf("Error: writing directory entry\n");
+
 exit:
free(mydata->fatbuf);
return ret;
-- 
2.1.4

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 5/5] fs/fat/fat_write: Fix management of empty files

[Benoît Thébaudeau]

Overwriting an empty file not created by U-Boot did not work, and it
could even corrupt the FAT. Moreover, creating empty files or emptying
existing files allocated a cluster, which is not standard.

Fix this by always keeping empty files clusterless as specified by
Microsoft (the start cluster must be set to 0 in the directory entry in
that case), and by supporting overwriting such files.

Signed-off-by: Benoît Thébaudeau <ben...@wsystem.com>
---
 fs/fat/fat_write.c | 85 --
 1 file changed, 64 insertions(+), 21 deletions(-)

diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
index 2d032ee..af828d0 100644
--- a/fs/fat/fat_write.c
+++ b/fs/fat/fat_write.c
@@ -710,6 +710,14 @@ set_contents(fsdata *mydata, dir_entry *dentptr, __u8 
*buffer,
 
debug("%llu bytes\n", filesize);
 
+   if (!curclust) {
+   if (filesize) {
+   debug("error: nonempty clusterless file!\n");
+   return -1;
+   }
+   return 0;
+   }
+
actsize = bytesperclust;
endclust = curclust;
do {
@@ -765,15 +773,24 @@ getit:
 }
 
 /*
- * Fill dir_entry
+ * Set start cluster in directory entry
  */
-static void fill_dentry(fsdata *mydata, dir_entry *dentptr,
-   const char *filename, __u32 start_cluster, __u32 size, __u8 attr)
+static void set_start_cluster(const fsdata *mydata, dir_entry *dentptr,
+   __u32 start_cluster)
 {
if (mydata->fatsize == 32)
dentptr->starthi =
cpu_to_le16((start_cluster & 0x) >> 16);
dentptr->start = cpu_to_le16(start_cluster & 0x);
+}
+
+/*
+ * Fill dir_entry
+ */
+static void fill_dentry(fsdata *mydata, dir_entry *dentptr,
+   const char *filename, __u32 start_cluster, __u32 size, __u8 attr)
+{
+   set_start_cluster(mydata, dentptr, start_cluster);
dentptr->size = cpu_to_le32(size);
 
dentptr->attr = attr;
@@ -1030,32 +1047,58 @@ static int do_fat_write(const char *filename, void 
*buffer, loff_t size,
retdent->size = cpu_to_le32(size);
start_cluster = START(retdent);
 
-   ret = check_overflow(mydata, start_cluster, size);
-   if (ret) {
-   printf("Error: %llu overflow\n", size);
-   goto exit;
-   }
+   if (start_cluster) {
+   if (size) {
+   ret = check_overflow(mydata, start_cluster,
+   size);
+   if (ret) {
+   printf("Error: %llu overflow\n", size);
+   goto exit;
+   }
+   }
 
-   ret = clear_fatent(mydata, start_cluster);
-   if (ret) {
-   printf("Error: clearing FAT entries\n");
-   goto exit;
+   ret = clear_fatent(mydata, start_cluster);
+   if (ret) {
+   printf("Error: clearing FAT entries\n");
+   goto exit;
+   }
+
+   if (!size)
+   set_start_cluster(mydata, retdent, 0);
+   } else if (size) {
+   ret = start_cluster = find_empty_cluster(mydata);
+   if (ret < 0) {
+   printf("Error: finding empty cluster\n");
+   goto exit;
+   }
+
+   ret = check_overflow(mydata, start_cluster, size);
+   if (ret) {
+   printf("Error: %llu overflow\n", size);
+   goto exit;
+   }
+
+   set_start_cluster(mydata, retdent, start_cluster);
}
} else {
/* Set short name to set alias checksum field in dir_slot */
set_name(empty_dentptr, filename);
fill_dir_slot(mydata, _dentptr, filename);
 
-   ret = start_cluster = find_empty_cluster(mydata);
-   if (ret < 0) {
-   printf("Error: finding empty cluster\n");
-   goto exit;
-   }
+   if (size) {
+   ret = start_cluster = find_empty_cluster(mydata);
+   if (ret < 0) {
+   printf("Error: finding empty cluster\n");
+   goto exit;
+   }
 
-   ret = check_overflow(mydata, start_cluster, size);
-   if (ret

[U-Boot] [PATCH 1/5] fs/fat/fat_write: Fix buffer alignments

[Benoît Thébaudeau]

set_cluster() was using a temporary buffer without enforcing its
alignment for DMA and cache. Moreover, it did not check the alignment of
the passed buffer, which can come directly from applicative code or from
the user.

This could cause random data corruption, which has been observed on
i.MX25 writing to an SD card.

Fix this by only passing ARCH_DMA_MINALIGN-aligned buffers to
disk_write(), which requires the introduction of a buffer bouncing
mechanism for the misaligned buffers passed to set_cluster().

By the way, improve the handling of the corresponding return values from
disk_write():
 - print them with debug() in case of error,
 - consider that there is an error is disk_write() returns a smaller
   block count than the requested one, not only if its return value is
   negative.

After this change, set_cluster() and get_cluster() are almost
symmetrical.

Signed-off-by: Benoît Thébaudeau <ben...@wsystem.com>
---
 fs/fat/fat_write.c | 48 ++--
 1 file changed, 34 insertions(+), 14 deletions(-)

diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
index adb6940..d0d9df7 100644
--- a/fs/fat/fat_write.c
+++ b/fs/fat/fat_write.c
@@ -555,8 +555,9 @@ static int
 set_cluster(fsdata *mydata, __u32 clustnum, __u8 *buffer,
 unsigned long size)
 {
-   int idx = 0;
+   __u32 idx = 0;
__u32 startsect;
+   int ret;
 
if (clustnum > 0)
startsect = mydata->data_begin +
@@ -566,26 +567,45 @@ set_cluster(fsdata *mydata, __u32 clustnum, __u8 *buffer,
 
debug("clustnum: %d, startsect: %d\n", clustnum, startsect);
 
-   if ((size / mydata->sect_size) > 0) {
-   if (disk_write(startsect, size / mydata->sect_size, buffer) < 
0) {
-   debug("Error writing data\n");
+   if ((unsigned long)buffer & (ARCH_DMA_MINALIGN - 1)) {
+   ALLOC_CACHE_ALIGN_BUFFER(__u8, tmpbuf, mydata->sect_size);
+
+   printf("FAT: Misaligned buffer address (%p)\n", buffer);
+
+   while (size >= mydata->sect_size) {
+   memcpy(tmpbuf, buffer, mydata->sect_size);
+   ret = disk_write(startsect++, 1, tmpbuf);
+   if (ret != 1) {
+   debug("Error writing data (got %d)\n", ret);
+   return -1;
+   }
+
+   buffer += mydata->sect_size;
+   size -= mydata->sect_size;
+   }
+   } else if (size >= mydata->sect_size) {
+   idx = size / mydata->sect_size;
+   ret = disk_write(startsect, idx, buffer);
+   if (ret != idx) {
+   debug("Error writing data (got %d)\n", ret);
return -1;
}
+
+   startsect += idx;
+   idx *= mydata->sect_size;
+   buffer += idx;
+   size -= idx;
}
 
-   if (size % mydata->sect_size) {
-   __u8 tmpbuf[mydata->sect_size];
+   if (size) {
+   ALLOC_CACHE_ALIGN_BUFFER(__u8, tmpbuf, mydata->sect_size);
 
-   idx = size / mydata->sect_size;
-   buffer += idx * mydata->sect_size;
-   memcpy(tmpbuf, buffer, size % mydata->sect_size);
-
-   if (disk_write(startsect + idx, 1, tmpbuf) < 0) {
-   debug("Error writing data\n");
+   memcpy(tmpbuf, buffer, size);
+   ret = disk_write(startsect, 1, tmpbuf);
+   if (ret != 1) {
+   debug("Error writing data (got %d)\n", ret);
return -1;
}
-
-   return 0;
}
 
return 0;
-- 
2.1.4

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 3/5] fs/fat/fat_write: Fix curclust/newclust mix-up

[Benoît Thébaudeau]

curclust was used instead of newclust in the debug() calls and in one
CHECK_CLUST() call, which could skip a failure case.

Signed-off-by: Benoît Thébaudeau <ben...@wsystem.com>
---
 fs/fat/fat_write.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
index e08cf83..2399844 100644
--- a/fs/fat/fat_write.c
+++ b/fs/fat/fat_write.c
@@ -721,7 +721,7 @@ set_contents(fsdata *mydata, dir_entry *dentptr, __u8 
*buffer,
goto getit;
 
if (CHECK_CLUST(newclust, mydata->fatsize)) {
-   debug("curclust: 0x%x\n", newclust);
+   debug("newclust: 0x%x\n", newclust);
debug("Invalid FAT entry\n");
return 0;
}
@@ -754,8 +754,8 @@ getit:
filesize -= actsize;
buffer += actsize;
 
-   if (CHECK_CLUST(curclust, mydata->fatsize)) {
-   debug("curclust: 0x%x\n", curclust);
+   if (CHECK_CLUST(newclust, mydata->fatsize)) {
+   debug("newclust: 0x%x\n", newclust);
debug("Invalid FAT entry\n");
return 0;
}
-- 
2.1.4

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH V2] imx-common: consider mux_ctrl_ofs when setting mux_mode

[Benoît Thébaudeau]

Hi Peng,

On Wed, Sep 23, 2015 at 5:13 AM, Peng Fan <peng@freescale.com> wrote:
> Some i.MXes use __NA_ or 0 to avoid setting mux_mode, but the following patch
> only take i.MX6/7 into consideration.

Small typo that I missed to report in v1: "takes". This can probably
be fixed when applying.

>
> "c3c8a5748897b24f18618047804317167a531dd3 imx-common: fix iomux settings"
>
> Use is_soc_type(MXC_CPU_MX7) to avoid breaking other i.MXes when
> setting mux_mode.
>
> In this patch, switch to use "asm/imx-common/sys_proto.h" to avoid
> build break for "is_soc_type" for vf610 and mx25.
>
> Signed-off-by: Peng Fan <peng@freescale.com>
> Cc: Bhuvanchandra DV <bhuvanchandra...@toradex.com>
> Cc: Stefano Babic <sba...@denx.de>
> Cc: Fabio Estevam <fabio.este...@freescale.com>
> ---
>
> Changes v2:
>  refine commit msg, "add mx25".
>  use cleaner way to for setting mux code, following Benoît's suggestion.
>
>  arch/arm/imx-common/iomux-v3.c | 7 +++
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/imx-common/iomux-v3.c b/arch/arm/imx-common/iomux-v3.c
> index 9b9cf58..228d5f8 100644
> --- a/arch/arm/imx-common/iomux-v3.c
> +++ b/arch/arm/imx-common/iomux-v3.c
> @@ -11,10 +11,8 @@
>  #include 
>  #include 
>  #include 
> -#if !defined(CONFIG_MX25) && !defined(CONFIG_VF610)
> -#include 
> -#endif
>  #include 
> +#include 
>
>  static void *base = (void *)IOMUXC_BASE_ADDR;
>
> @@ -53,7 +51,8 @@ void imx_iomux_v3_setup_pad(iomux_v3_cfg_t pad)
> }
>  #endif
>
> -   __raw_writel(mux_mode, base + mux_ctrl_ofs);
> +   if (is_soc_type(MXC_SOC_MX7) || mux_ctrl_ofs)
> +   __raw_writel(mux_mode, base + mux_ctrl_ofs);
>
> if (sel_input_ofs)
> __raw_writel(sel_input, base + sel_input_ofs);
> --
> 1.8.4


Reviewed-by: Benoît Thébaudeau <benoit.thebaudeau@gmail.com>

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] imx-common: consider mux_ctrl_ofs when setting mux_mode

[Benoît Thébaudeau]

Hi Peng,

On Mon, Sep 21, 2015 at 11:08 AM, Peng Fan  wrote:
> Some i.MXes use __NA_ or 0 to avoid setting mux_mode, but the following patch
> only take i.MX6/7 into consideration.
>
> "c3c8a5748897b24f18618047804317167a531dd3 imx-common: fix iomux settings"
>
> Use is_soc_type(MXC_CPU_MX7) to avoid breaking other i.MXes when
> setting mux_mode.
>
> In this patch, switch to use "asm/imx-common/sys_proto.h" to avoid
> build break for "is_soc_type" for vf610.

And probably for i.MX25 too.

> Signed-off-by: Peng Fan 
> Cc: Bhuvanchandra DV 
> Cc: Stefano Babic 
> Cc: Fabio Estevam 
> ---
>  arch/arm/imx-common/iomux-v3.c | 10 ++
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/imx-common/iomux-v3.c b/arch/arm/imx-common/iomux-v3.c
> index 9b9cf58..81ec1d3 100644
> --- a/arch/arm/imx-common/iomux-v3.c
> +++ b/arch/arm/imx-common/iomux-v3.c
> @@ -11,10 +11,8 @@
>  #include 
>  #include 
>  #include 
> -#if !defined(CONFIG_MX25) && !defined(CONFIG_VF610)
> -#include 
> -#endif
>  #include 
> +#include 
>
>  static void *base = (void *)IOMUXC_BASE_ADDR;
>
> @@ -53,7 +51,11 @@ void imx_iomux_v3_setup_pad(iomux_v3_cfg_t pad)
> }
>  #endif
>
> -   __raw_writel(mux_mode, base + mux_ctrl_ofs);
> +   if (is_soc_type(MXC_SOC_MX7))
> +   __raw_writel(mux_mode, base + mux_ctrl_ofs);
> +   else
> +   if (mux_ctrl_ofs)
> +   __raw_writel(mux_mode, base + mux_ctrl_ofs);

Or, in order to avoid duplicating the line:
+   if (is_soc_type(MXC_SOC_MX7) || mux_ctrl_ofs)
+   __raw_writel(mux_mode, base + mux_ctrl_ofs);

>
> if (sel_input_ofs)
> __raw_writel(sel_input, base + sel_input_ofs);
> --
> 1.8.4

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 1/3] imx-common: fix iomux settings

[Benoît Thébaudeau]

Hi Peng,

On Mon, Sep 21, 2015 at 3:05 AM, Peng Fan <b51...@freescale.com> wrote:
> On Sun, Sep 20, 2015 at 05:02:58PM +0200, Benoît Thébaudeau wrote:
>>On Sun, Sep 20, 2015 at 3:02 PM, Peng Fan <b51...@freescale.com> wrote:
>>> On Sun, Sep 20, 2015 at 01:33:20PM +0200, Benoît Thébaudeau wrote:
>>>>>> Also If still checking mux_ctrl_ofs, we have no chance to set iomux
>>>>>> for i.MX7D IOMUXC_LPSR_SW_MUX_CTL_PAD_GPIO1_IO00, because the 
>>>>>> mux_ctrl_ofs
>>>>>> for this register is 0.
>>>>
>>>>The need is clear, but then the test mechanism should be changed, not
>>>>removed. You could find a free bit in mux_ctrl_ofs or in mux_mode or
>>>>elsewhere in IOMUX_PAD (e.g. bit 63, which is currently reserved),
>>>>something like NO_PAD_CTRL, or create a reserved value other than
>>>>__NA_ for mux_ctrl_ofs/mux_mode.
>>>
>>> Stefano,
>>>
>>> There is '#define NO_PAD_CTRL (1 << 17)' now,
>>> we can add'NO_MUX_CTRL' and 'NO_SEL_CTRL(select input)', but need to check
>>> whether the __NA__ pads are used or not now.
>>> also need a big change for the layout and related macro definition:
>>> 39  * MUX_CTRL_OFS:0..11 (12)
>>> 40  * PAD_CTRL_OFS:   12..23 (12)
>>> 41  * SEL_INPUT_OFS:  24..35 (12)
>>> 42  * MUX_MODE + SION:36..40  (5)
>>> 43  * PAD_CTRL + NO_PAD_CTRL: 41..58 (18)
>>> 44  * SEL_INP:59..62  (4)
>>> 45  * reserved: 63(1)
>>>
>>> Can we just use the following way, since only i.mx7 has the requirement of
>>> mux_ctrl_ofs maybe at 0.
>>> if (is_soc_type(MX7)) {
>>> __raw_writel(mux_mode, base + mux_ctrl_ofs);
>>> } else {
>>> if (mux_ctrl_ofs)
>>> __raw_writel(mux_mode, base + mux_ctrl_ofs);
>>> }
>>> I prefer this simple way for now, since we are at RC2 now. Later we can
>>> refactor the code using the way to provide macros NO_MUX_CTRL or 
>>> NO_SEL_CTRL.
>>> What do you think?
>>
>>Maybe, but instead of NO_MUX_CTRL and the like we could also just
>>define __NA_ to (-1) instead of 0 and mask the passed values
>>appropriately in IOMUX_PAD(). This should be done for all types of
>>offsets, and __NA_ should be used everywhere instead of raw 0x000
>>values. -1 is guaranteed not to be needed by any SoC because of the
>>word alignment requirement for valid offsets. That would keep the
>>changes small.
>
> We can not just simple use __NA_ with value -1.
> see
> 70 #define IOMUX_PAD(pad_ctrl_ofs, mux_ctrl_ofs, mux_mode, sel_input_ofs,  \
> 71 sel_input, pad_ctrl)\
> 72 (((iomux_v3_cfg_t)(mux_ctrl_ofs) << MUX_CTRL_OFS_SHIFT) |   \
> 73 ((iomux_v3_cfg_t)(mux_mode)  << MUX_MODE_SHIFT) |   \
> 74 ((iomux_v3_cfg_t)(pad_ctrl_ofs)  << MUX_PAD_CTRL_OFS_SHIFT) |   \
> 75 ((iomux_v3_cfg_t)(pad_ctrl)  << MUX_PAD_CTRL_SHIFT) |   \
> 76 ((iomux_v3_cfg_t)(sel_input_ofs) << MUX_SEL_INPUT_OFS_SHIFT)|   \
> 77 ((iomux_v3_cfg_t)(sel_input) << MUX_SEL_INPUT_SHIFT))
>
> iomux_v3_cfg_t(mux_ctrl_ofs) << MUX_CTRL_OFS_SHIFT should be changed to
> `iomux_v3_cfg_t((mux_ctrl_ofs) << MUX_CTRL_OFS_SHIFT) & MUX_CTRL_OFS_MASK`,

Yes. That's why I said "mask the passed values appropriately in IOMUX_PAD()".

> in iomux-v3.c, need to test if (!(mux_ctrl_ofs & 1)) {x}.

Yes, of course.

> I am not sure whether this will incur unexpected things or not,

There's no reason.

> also
> the IOMUX_PAD with 0, but not __NA_ need to change to use __NA_.

And also, as I said, do the same for pad_ctrl_ofs and sel_input_ofs in
order to be consistent, and replace definitions like:
 MX25_PAD_CTL_GRP_DVS_MISC   = IOMUX_PAD(0x418,
0x000, 0, 0, 0, NO_PAD_CTRL),
with:
 MX25_PAD_CTL_GRP_DVS_MISC   = IOMUX_PAD(0x418,
__NA_, 0, __NA_, 0, NO_PAD_CTRL),

> So I prefer to use is_soc_type(MXC_CPU_MX7) for now.

Yes, that's OK for now. I was suggesting that as a longterm approach.
This change would be simple, but many definitions would have to be
updated.

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 1/3] imx-common: fix iomux settings

[Benoît Thébaudeau]

Hi Peng,

On Sun, Sep 20, 2015 at 3:02 PM, Peng Fan <b51...@freescale.com> wrote:
> On Sun, Sep 20, 2015 at 01:33:20PM +0200, Benoît Thébaudeau wrote:
>>Hi Stefano, Peng, Fabio, all,
>>
>>Sorry for seeing this only now, but...
>>
>>On Sun, Sep 20, 2015 at 9:43 AM, Stefano Babic <sba...@denx.de> wrote:
>>>
>>>
>>> On 14/09/2015 07:34, Peng Fan wrote:
>>>> When setting iomux for a pin mux, there is no need to check mux_ctrl_ofs.
>>
>>This assumption is wrong. This check was there for a reason. Some i.MX
>>SoCs have some registers controlling pads but not muxes, either for a
>>single pin or for groups of pins:
>>http://git.denx.de/?p=u-boot/u-boot-imx.git;a=blob;f=arch/arm/include/asm/arch-mx25/iomux-mx25.h;h=220cf4ef2e94aa69482557852ed0cc0690a79cec;hb=HEAD
>>http://git.denx.de/?p=u-boot/u-boot-imx.git;a=blob;f=arch/arm/include/asm/arch-mx35/iomux-mx35.h;h=5898b46f4720088b18882e21d0d2424fff987ab5;hb=HEAD
>>http://git.denx.de/?p=u-boot/u-boot-imx.git;a=blob;f=arch/arm/include/asm/arch-mx5/iomux-mx51.h;h=b7b169505f91c4a213be59efca47e8a5aed770e7;hb=HEAD
>>
>>I have not checked whether these cases are currently used in-tree by
>>U-Boot, but they have to be possible anyway in order to support these
>>SoCs.
>
> Benoît,
>
> Thanks for pointing this out.
> You mean piece of code like this, right?
> 509 MX25_PAD_CTL_GRP_DVS_MISC   = IOMUX_PAD(0x418, 0x000, 
> 0, 0, 0, NO_PAD_CTRL),
> 510 MX25_PAD_CTL_GRP_DSE_FEC= IOMUX_PAD(0x41c, 0x000, 
> 0, 0, 0, NO_PAD_CTRL),
> 511 MX25_PAD_CTL_GRP_DVS_JTAG   = IOMUX_PAD(0x420, 0x000, 
> 0, 0, 0, NO_PAD_CTRL),
> 512 MX25_PAD_CTL_GRP_DSE_NFC= IOMUX_PAD(0x424, 0x000, 
> 0, 0, 0, NO_PAD_CTRL),
> 513 MX25_PAD_CTL_GRP_DSE_CSI= IOMUX_PAD(0x428, 0x000, 
> 0, 0, 0, NO_PAD_CTRL),
> 514 MX25_PAD_CTL_GRP_DSE_WEIM   = IOMUX_PAD(0x42c, 0x000, 
> 0, 0, 0, NO_PAD_CTRL),
> 515 MX25_PAD_CTL_GRP_DSE_DDR= IOMUX_PAD(0x430, 0x000, 
> 0, 0, 0, NO_PAD_CTRL),
> 516 MX25_PAD_CTL_GRP_DVS_CRM= IOMUX_PAD(0x434, 0x000, 
> 0, 0, 0, NO_PAD_CTRL),
> 517 MX25_PAD_CTL_GRP_DSE_KPP= IOMUX_PAD(0x438, 0x000, 
> 0, 0, 0, NO_PAD_CTRL),
> 518 MX25_PAD_CTL_GRP_DSE_SDHC1  = IOMUX_PAD(0x43c, 0x000, 
> 0, 0, 0, NO_PAD_CTRL),
> 519 MX25_PAD_CTL_GRP_DSE_LCD= IOMUX_PAD(0x440, 0x000, 
> 0, 0, 0, NO_PAD_CTRL),
> 520 MX25_PAD_CTL_GRP_DSE_UART   = IOMUX_PAD(0x444, 0x000, 
> 0, 0, 0, NO_PAD_CTRL),
> 521 MX25_PAD_CTL_GRP_DVS_NFC= IOMUX_PAD(0x448, 0x000, 
> 0, 0, 0, NO_PAD_CTRL),
> 522 MX25_PAD_CTL_GRP_DVS_CSI= IOMUX_PAD(0x44c, 0x000, 
> 0, 0, 0, NO_PAD_CTRL),
> 523 MX25_PAD_CTL_GRP_DSE_CSPI1  = IOMUX_PAD(0x450, 0x000, 
> 0, 0, 0, NO_PAD_CTRL),
> 524 MX25_PAD_CTL_GRP_DDRTYPE= IOMUX_PAD(0x454, 0x000, 
> 0, 0, 0, NO_PAD_CTRL),
> 525 MX25_PAD_CTL_GRP_DVS_SDHC1  = IOMUX_PAD(0x458, 0x000, 
> 0, 0, 0, NO_PAD_CTRL),
> 526 MX25_PAD_CTL_GRP_DVS_LCD= IOMUX_PAD(0x45c, 0x000, 
> 0, 0, 0, NO_PAD_CTRL)

Correct.

> My bad. I only took i.mx6/7 into consideration when working this patch.
>>
>>>> Also If still checking mux_ctrl_ofs, we have no chance to set iomux
>>>> for i.MX7D IOMUXC_LPSR_SW_MUX_CTL_PAD_GPIO1_IO00, because the mux_ctrl_ofs
>>>> for this register is 0.
>>
>>The need is clear, but then the test mechanism should be changed, not
>>removed. You could find a free bit in mux_ctrl_ofs or in mux_mode or
>>elsewhere in IOMUX_PAD (e.g. bit 63, which is currently reserved),
>>something like NO_PAD_CTRL, or create a reserved value other than
>>__NA_ for mux_ctrl_ofs/mux_mode.
>
> Stefano,
>
> There is '#define NO_PAD_CTRL (1 << 17)' now,
> we can add'NO_MUX_CTRL' and 'NO_SEL_CTRL(select input)', but need to check
> whether the __NA__ pads are used or not now.
> also need a big change for the layout and related macro definition:
> 39  * MUX_CTRL_OFS:0..11 (12)
> 40  * PAD_CTRL_OFS:   12..23 (12)
> 41  * SEL_INPUT_OFS:  24..35 (12)
> 42  * MUX_MODE + SION:36..40  (5)
> 43  * PAD_CTRL + NO_PAD_CTRL: 41..58 (18)
> 44  * SEL_INP:59..62  (4)
> 45  * reserved: 63(1)
>
> Can we just use the following way, since only i.mx7 has the requirement of
> mux_ctrl_ofs maybe at 0.
> if (is_soc_type(MX7)) {
> __raw_writel(mux_mode, base + mux_ctrl_ofs);
> } els

Re: [U-Boot] [PATCH 1/3] imx-common: fix iomux settings

[Benoît Thébaudeau]

Hi Stefano, Peng, Fabio, all,

Sorry for seeing this only now, but...

On Sun, Sep 20, 2015 at 9:43 AM, Stefano Babic  wrote:
>
>
> On 14/09/2015 07:34, Peng Fan wrote:
>> When setting iomux for a pin mux, there is no need to check mux_ctrl_ofs.

This assumption is wrong. This check was there for a reason. Some i.MX
SoCs have some registers controlling pads but not muxes, either for a
single pin or for groups of pins:
http://git.denx.de/?p=u-boot/u-boot-imx.git;a=blob;f=arch/arm/include/asm/arch-mx25/iomux-mx25.h;h=220cf4ef2e94aa69482557852ed0cc0690a79cec;hb=HEAD
http://git.denx.de/?p=u-boot/u-boot-imx.git;a=blob;f=arch/arm/include/asm/arch-mx35/iomux-mx35.h;h=5898b46f4720088b18882e21d0d2424fff987ab5;hb=HEAD
http://git.denx.de/?p=u-boot/u-boot-imx.git;a=blob;f=arch/arm/include/asm/arch-mx5/iomux-mx51.h;h=b7b169505f91c4a213be59efca47e8a5aed770e7;hb=HEAD

I have not checked whether these cases are currently used in-tree by
U-Boot, but they have to be possible anyway in order to support these
SoCs.

>> Also If still checking mux_ctrl_ofs, we have no chance to set iomux
>> for i.MX7D IOMUXC_LPSR_SW_MUX_CTL_PAD_GPIO1_IO00, because the mux_ctrl_ofs
>> for this register is 0.

The need is clear, but then the test mechanism should be changed, not
removed. You could find a free bit in mux_ctrl_ofs or in mux_mode or
elsewhere in IOMUX_PAD (e.g. bit 63, which is currently reserved),
something like NO_PAD_CTRL, or create a reserved value other than
__NA_ for mux_ctrl_ofs/mux_mode.

>> Signed-off-by: Peng Fan 
>> Cc: Stefano Babic 
>> Cc: Fabio Estevam 
>> ---
>>  arch/arm/imx-common/iomux-v3.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/imx-common/iomux-v3.c b/arch/arm/imx-common/iomux-v3.c
>> index b4f481f..9b9cf58 100644
>> --- a/arch/arm/imx-common/iomux-v3.c
>> +++ b/arch/arm/imx-common/iomux-v3.c
>> @@ -53,8 +53,7 @@ void imx_iomux_v3_setup_pad(iomux_v3_cfg_t pad)
>>   }
>>  #endif
>>
>> - if (mux_ctrl_ofs)
>> - __raw_writel(mux_mode, base + mux_ctrl_ofs);
>> + __raw_writel(mux_mode, base + mux_ctrl_ofs);
>>
>>   if (sel_input_ofs)
>>   __raw_writel(sel_input, base + sel_input_ofs);
>>
>
> Applied (whole series) to u-boot-imx, thanks !

Please fix.

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] BDI3000 U-Boot debugging questions (MX51/MX53)

[Benoît Thébaudeau]

Hi Dave,

On Thu, Feb 26, 2015 at 9:05 PM, DaveKucharczyk
david.kucharc...@gmail.com wrote:
 I would like to debug from the earliest possible point pre-relocation (for
 educational reasons). Couple questions

 In the Makefile, where do I place the following flags...
  -Os #-fomit-frame-pointer -g -fno-schedule-insns -fno-schedule-insns2

 I've added the flags in a few different spots, but I still can't break on
 cpu_init_f.
 Here's the output...
 (gdb) b cpu_init_f
 Function cpu_init_f not defined.
 Make breakpoint pending on future shared library load? (y or [n]) y

 I assume the flags didn't get set, specifically -g.

 To backup a little here is what I get when I do target remote...

 (gdb) target remote 169.254.21.13:2001
 Remote debugging using 169.254.21.13:2001
 warning: Unable to find dynamic linker breakpoint function.
 GDB will be unable to debug shared library initializers
 and track explicitly loaded dynamic code.
 0x in ?? ()
 (gdb)

 I assume those warning are related to my issue above?

You have to load the symbols from the unstripped U-Boot ELF file:
https://sourceware.org/gdb/current/onlinedocs/gdb/Files.html#Files

You can use 'symbol-file' first. The loaded symbols will match the PC
values only until relocation. Then, you can use 'add-symbol-file' to
specify the relocation address.

If needed, 'set sysroot' can be used for the shared libraries, but
this does not apply to U-Boot.

Note that very early after boot, the PC will be in the boot ROM, so
there won't be any matching symbols.

 Also, when I power on the target, u-boot just starts loading. How do I halt
 it? I tried to set a breakpoint at text base in the BDI, but it doesn't
 halt.

You must have a configuration file for your JTAG probe. In this file,
you can choose between hardware and software breakpoints, select the
default action (run, run for some time, halt, etc.), set breakpoints,
etc. With all these settings, you should be able to halt early. You
can also use the JTAG probe Telnet to issue a manual break when
needed. Avoid setting breakpoints directly onto exception vectors like
reset; sometimes it does not work well, so it is better to set it on a
known function executed a bit later.

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] U-Boot stuck after relocation attempt on MX51 board

[Benoît Thébaudeau]

Hi Dave,

On Thu, Feb 26, 2015 at 3:19 PM, DaveKucharczyk
david.kucharc...@gmail.com wrote:
 Benoît Thébaudeau-2 wrote
 Also, check the CONFIG_SYS_TEXT_BASE of your board. From your log, I'm
 wondering if it's not set too high, resulting in an overlap of the
 pre- and post-relocation addresses occupied by your binary in the
 256-MiB case.

 BINGO!!! Good catch Benoît, thank you. I changed it from 9FF8 to
 9F00 and now it boots.

 Looking at the original log...it relocated 90KB above text base. Not quite
 enough room...
 
 U-Boot code: 9FF8 - 9FFA6840  BSS: - 9FFD944C
 .
 Relocating to 9ff96000, new gd at 9fe55ed0, sp at 9fe55eb0
 --

You're welcome.

 On a side note, how involved would it be to convert to 2015.01? I've already
 ported 3 boards for 2014.07. Will everything I've done work perfectly fine
 with 2015.01?

You will probably have to make a few adjustments, but that should not
be too heavy. There have been many changes in the infrastructure of
U-Boot lately, mainly revolving around the integration of Kconfig,
Kbuild and the driver model. At worst, you should have to upgrade your
config / make / maintainers files, and maybe adjust your code for a
few changes in the driver APIs. To have an idea:
http://git.denx.de/cgi-bin/gitweb.cgi?p=u-boot.git;a=history;f=board/freescale/mx51evk;hpb=refs/tags/v2014.07;hb=refs/tags/v2015.01

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] U-Boot stuck after relocation attempt on MX51 board

[Benoît Thébaudeau]

Hi Albert,

On Thu, Feb 26, 2015 at 11:38 AM, Albert ARIBAUD
albert.u.b...@aribaud.net wrote:
 Hello Benoît,

 On Thu, 26 Feb 2015 00:56:00 +0100, Benoît Thébaudeau
 benoit.thebaudeau@gmail.com wrote:
 Dear Dave Kucharczyk,

 On Wed, Feb 25, 2015 at 11:08 PM, DaveKucharczyk
 david.kucharc...@gmail.com wrote:
  Fabio Estevam-2 wrote
  Also, you said that your 512MB board version works fine, but the 256MB
  fails.
 
  I suppose you are using two different binaries for each board, right?
  You can't have a single binary for the two boards, unless you use the
  SPL approach.
 
  Fabio, we use one binary. It has runtime memory discovery via gpio's
  (resistor reads). DRAM size is reported correctly from both boards. It just
  hangs on the 256MB board.
 
  We do not have SPL setup.

 You should try with 2015.01 as Fabio suggested.

 Also, check the CONFIG_SYS_TEXT_BASE of your board. From your log, I'm
 wondering if it's not set too high, resulting in an overlap of the
 pre- and post-relocation addresses occupied by your binary in the
 256-MiB case.

 /me wonders whether we should not add a test for this situation, with
 a conspicuous error message on the console stating that relocation will
 probably fail due to overlap.

Yes, that would be very helpful in such a case, which must occur very
often for new boards being ported to U-Boot. Besides that, it would be
great if CONFIG_SYS_TEXT_BASE could be automatically set to a sensible
value for most boards, from the configured internal/external RAM base
addresses.

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] U-Boot stuck after relocation attempt on MX51 board

[Benoît Thébaudeau]

.Hi Fabio,

On Wed, Feb 25, 2015 at 11:05 PM, Fabio Estevam feste...@gmail.com wrote:
 I have just tested top of tree U-boot and my mx53loco board boots fine.

That's because CONFIG_HAS_VBAR is set for ARMv7. There may be an
issue, though: according to Freescale, the TrustZone security
extensions are present on i.MX514/515/516/53x, but not on i.MX512/513.
According to ARM, the security extensions seem to always be
implemented on Cortex-A8, so it's not clear what Freescale means for
i.MX512/513, and if the non-secure VBAR is still available on these.
If not, 0x is the boot ROM, and 0x is reserved, just
like on i.MX25/27/31/35, so disabling CONFIG_HAS_VBAR for i.MX512/513
would not help, and your vector relocation patch would be required
here too. It would be nice if someone could test the latest U-Boot on
i.MX512/513.

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] U-Boot stuck after relocation attempt on MX51 board

[Benoît Thébaudeau]

Dear Dave Kucharczyk,

On Wed, Feb 25, 2015 at 3:19 AM, DaveKucharczyk
david.kucharc...@gmail.com wrote:
 I'm porting U-Boot for an MX51 based board.

 This is the boot sequence with debug on...

 U-Boot 2014.07-svn10 (Feb 24 2015 - 15:49:39)

 initcall: 9ff85820
 U-Boot code: 9FF8 - 9FFA6824  BSS: - 9FFD944C
 initcall: 9ff8118c
 CPU:   Freescale i.MX51 rev3.0 at 800 MHz
 initcall: 9ff858ac
 I2C:   ready
 initcall: 9ff85894
 DRAM:  initcall: 9ff81ff4
 initcall: 9ff85a04
 Monitor len: 0005944C
 Ram size: 1000
 Ram top: A000
 initcall: 9ff855b0
 initcall: 9ff857c8
 TLB table from 9fff to 9fff4000
 initcall: 9ff855c8
 initcall: 9ff8577c
 Reserving 357k for U-Boot at: 9ff96000
 initcall: 9ff85750
 Reserving 1280k for malloc() at: 9fe56000
 initcall: 9ff85850
 Reserving 88 Bytes for Board Info at: 9fe55fa8
 initcall: 9ff855d0
 initcall: 9ff8571c
 Reserving 216 Bytes for Global Data at: 9fe55ed0
 initcall: 9ff856b8
 initcall: 9ff855e4
 initcall: 9ff859ec
 initcall: 9ff85948

 RAM Configuration:
 Bank #0: 9000 256 MiB

 DRAM:  256 MiB
 initcall: 9ff8569c
 New Stack Pointer is: 9fe55eb0
 initcall: 9ff85618
 initcall: 9ff85648
 Relocation Offset is: 00016000
 Relocating to 9ff96000, new gd at 9fe55ed0, sp at 9fe55eb0

 And that's where it hangs and resets in a continuous loop.

 I confirmed that the entire init_sequence completed in board_f.c, but never
 makes it to board_r.c

 So...according to ./arch/arm/lib/crt0.S ...

 /* assembly code */

 board_init_f  --we make it out of here

 /* assembly code */   -- stuck somewhere in here

 relocate_code   -- stuck somewhere in here

 /* assembly code */   -- stuck somewhere in here

 board_init_r  --we never make it here

 I can't sprinkle any debug statements where it's stuck because it's all
 assembly. Before I break out the BDI tomorrow does anyone have any ideas?

 BTW it works on our 512MB board, but not the 256MB board. Thanks.

The following needs to be added for i.MX51 and i.MX53 too:
http://lists.denx.de/pipermail/u-boot/2015-February/205849.html

This should fix your issue.

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] U-Boot stuck after relocation attempt on MX51 board

[Benoît Thébaudeau]

Dear Dave Kucharczyk,

On Wed, Feb 25, 2015 at 11:08 PM, DaveKucharczyk
david.kucharc...@gmail.com wrote:
 Fabio Estevam-2 wrote
 Also, you said that your 512MB board version works fine, but the 256MB
 fails.

 I suppose you are using two different binaries for each board, right?
 You can't have a single binary for the two boards, unless you use the
 SPL approach.

 Fabio, we use one binary. It has runtime memory discovery via gpio's
 (resistor reads). DRAM size is reported correctly from both boards. It just
 hangs on the 256MB board.

 We do not have SPL setup.

You should try with 2015.01 as Fabio suggested.

Also, check the CONFIG_SYS_TEXT_BASE of your board. From your log, I'm
wondering if it's not set too high, resulting in an overlap of the
pre- and post-relocation addresses occupied by your binary in the
256-MiB case.

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] Unused video drivers

[Benoît Thébaudeau]

Dear Nikita Kiryanov, Fabio, all,

On Fri, Feb 13, 2015 at 1:49 PM, Nikita Kiryanov nik...@compulab.co.il wrote:
 I noticed we have a lot of unused video drivers, specifically:

 CONFIG_EXYNOS_PWM_BL
 CONFIG_L5F31188
 CONFIG_SED156X
 CONFIG_VIDEO_COREBOOT
 CONFIG_VIDEO_IMX25LCDC

Fabio, do you think that this one should be enabled on mx25pdk?

 CONFIG_VIDEO_SED13806
 CONFIG_VIDEO_SMI_LYNXEM
 CONFIG_AM335X_LCD

 Any objections to removing these drivers?

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] Nokia RX-51 (N900) board broken

[Benoît Thébaudeau]

Dear Pali Rohár,

On Sat, Jan 3, 2015 at 11:47 AM, Pali Rohár pali.ro...@gmail.com wrote:
 Hello,

 Nokia N900 board does not work anymore from master branch in
 qemu. I bisected first commit which broke it. It is:

 41623c91b09a0c865fab41acdaff30f060f29ad6
 arm: move exception handling out of start.S files

 Before this commit uboot on n900 in qemu working fine. Since this
 commit qemu crash with fatal error (see below). I bisected other
 two commits which changed error behaviour, but still uboot is not
 working...

 

 Range: 41623c91b09a0c865fab41acdaff30f060f29ad6 .. 
 5bfdcebf73f843b4a0968e87cff9ad6546358c8d

 Commit: 41623c91b09a0c865fab41acdaff30f060f29ad6
 Author: Albert ARIBAUD albert.u.b...@aribaud.net  2014-04-15 16:13:51
 Committer: Albert ARIBAUD albert.u.b...@aribaud.net  2014-05-15 16:24:53
 Message: arm: move exception handling out of start.S files

 Error: qemu crashed with error message:

 qemu: fatal: Trying to execute code outside RAM or ROM at 0x00014080
 R00= R01=07a3 R02=8100 R03=80500508
 R04=805003c8 R05=83e8 R06=80531db0 R07=804ffb98
 R08= R09=0800 R10=8140 R11=
 R12=8414 R13=402064b0 R14=8014 R15=00014080
 PSR=61db -ZC- A und32

 

 Range: 58f9e1ae6391a1fbb7ca024ae45e288aabb88807 .. 
 2e07c249a67e055db294818ff063d502d15db0f8

 Commit: 58f9e1ae6391a1fbb7ca024ae45e288aabb88807
 Author: Benoit Thebaudeau benoit.thebaudeau@gmail.com  2014-09-03 
 23:32:33
 Committer: Albert ARIBAUD albert.u.b...@aribaud.net  2014-09-11 18:04:34

 Error: uboot freezed without any error message in serial console and with 
 broken screen output

 

 Range: c57a642384df0dfaacc8e9c6c06d76b5aecb965d .. 
 b7b3b8c6a0bfc87047cb18a7abfa06fb6e9d0331

 Commit: c57a642384df0dfaacc8e9c6c06d76b5aecb965d
 Author: Georges Savoundararadj savou...@gmail.com  2014-10-28 23:16:10
 Committer: Tom Rini tr...@ti.com  2014-10-29 14:02:17
 Message: arm: make .vectors section allocatable

 Error: uboot printed error to serial console and rebooted machine:

 prefetch abort
 pc : []  lr : [8fdd3684]

Can you tell from the .map or .lst files what this 0x8fdd3684 address
corresponds to in the source code?

 sp : 8fcefe78  ip : 002a fp : 8fdfd96c
 r10: 8fdfda3c  r9 : 8fceff24 r8 : 8fdfd968
 r7 : 8140  r6 : 00ff r5 : 8420  r4 : 8420
 r3 : 40208000  r2 : 0004 r1 :   r0 : 002a
 Flags: nzcv  IRQs on  FIQs on  Mode USER_26
 Resetting CPU ...

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 1/2] mxc_ocotp: Do not disable the OCOTP clock after every access

[Benoît Thébaudeau]

Hi Fabio,

On Tue, Nov 25, 2014 at 4:11 PM, Fabio Estevam
fabio.este...@freescale.com wrote:
 Leave the OCOTP turned on, so that we subsequent access do not fail.

 After enabling the thermal driver on a mx6sxsabresd board:

 U-Boot 2015.01-rc1-18267-g99d4189-dirty (Nov 24 2014 - 12:59:01)

 CPU:   Freescale i.MX6SX rev1.0 at 792 MHz
 CPU:   Temperature 48 C
 Reset cause: POR
 Board: MX6SX SABRE SDB
 I2C:   ready
 DRAM:  1 GiB
 PMIC:  PFUZE100 ID=0x10
 MMC:   FSL_SDHC: 0, FSL_SDHC: 1, FSL_SDHC: 2
   00:01.0 - 16c3:abcd - Bridge device
01:00.0- 8086:08b1 - Network controller
 In:serial
 Out:   serial
 Err:   serial
 Net:
 (hang)

 As the thermal driver accesses the ocotp registers, its clock will be disabled
 afterwards.

 Then when the MAC address is read (also from ocotp registers) it will cause a
 hang.

 Do not disable the ocotp clock to prevent this problem.

 Signed-off-by: Fabio Estevam fabio.este...@freescale.com
 ---
  drivers/misc/mxc_ocotp.c | 2 --
  1 file changed, 2 deletions(-)

 diff --git a/drivers/misc/mxc_ocotp.c b/drivers/misc/mxc_ocotp.c
 index 3de1245..67f9429 100644
 --- a/drivers/misc/mxc_ocotp.c
 +++ b/drivers/misc/mxc_ocotp.c
 @@ -81,8 +81,6 @@ static int finish_access(struct ocotp_regs *regs, const 
 char *caller)
 err = !!(readl(regs-ctrl)  BM_CTRL_ERROR);
 clear_error(regs);

 -   enable_ocotp_clk(0);
 -
 if (err) {
 printf(mxc_ocotp %s(): Access protect error\n, caller);
 return -EIO;

That, or:
 - Make imx_get_mac_from_fuse() call enable_ocotp_clk(1) before
reading the fuses, then call enable_ocotp_clk(0).
 - Make enable_ocotp_clk() return the clock initial state and store it
in prepare_access(), then restore it in finish_access(). Same in
imx_get_mac_from_fuse().

Which of these 3 choices do you think would be the best?

Regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 1/2] mxc_ocotp: Do not disable the OCOTP clock after every access

[Benoît Thébaudeau]

Hi Fabio,

On Tue, Nov 25, 2014 at 8:56 PM, Fabio Estevam feste...@gmail.com wrote:
 On Tue, Nov 25, 2014 at 5:48 PM, Benoît Thébaudeau
 benoit.thebaudeau@gmail.com wrote:

 That, or:
  - Make imx_get_mac_from_fuse() call enable_ocotp_clk(1) before
 reading the fuses, then call enable_ocotp_clk(0).

 Yes, I started with this exact same approach as well. It works, but
 Nitin told me he also had similiar issues with hab.

 Other issue I see with such approach is that if people would try to
 read the ocotp registers manually in the U-boot prompt (via md.l
 command), then they will also get a hang.

If users access the OCOTP registers manually, they can also enable the
OCOTP clock in the corresponding register beforehand, even if this
complicates things.

  - Make enable_ocotp_clk() return the clock initial state and store it
 in prepare_access(), then restore it in finish_access(). Same in
 imx_get_mac_from_fuse().

 This would work as well, but with some more complexity. Still would
 cause the hang via manual readings.

 Which of these 3 choices do you think would be the best?

 I think the simplest one and the one that would be more general would
 be the one proposed by this patch.

The only possible issue that I see with leaving the OCOTP clock
enabled is the risk of inadvertently writing the fuses, either in
U-Boot or in the booted OS. That being said, Freescale advise against
leaving the fuses powered for production boards, in which cases there
is no such risk.

Reviewed-by: Benoît Thébaudeau benoit.thebaudeau@gmail.com

Regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v2] cmd_fuse: return CMD_RET_FAILURE on error

[Benoît Thébaudeau]

Dear Hector Palacios,

On Thu, Nov 20, 2014 at 9:27 AM, Hector Palacios
hector.palac...@digi.com wrote:
 Fuse drivers, like the mxs_ocotp.c, may return negative error codes but
 the commands are only allowed to return CMD_RET_* enum values to the
 shell, otherwise the following error appears:

 exit not allowed from main input shell.

 Signed-off-by: Hector Palacios hector.palac...@digi.com
 ---

 Changes for v2:
 Reword commit message.

  common/cmd_fuse.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/common/cmd_fuse.c b/common/cmd_fuse.c
 index abab9789b0df..d4bc0f6c94a1 100644
 --- a/common/cmd_fuse.c
 +++ b/common/cmd_fuse.c
 @@ -128,7 +128,7 @@ static int do_fuse(cmd_tbl_t *cmdtp, int flag, int argc, 
 char *const argv[])

  err:
 puts(ERROR\n);
 -   return ret;
 +   return CMD_RET_FAILURE;
  }

  U_BOOT_CMD(

Thanks.

Reviewed-by: Benoît Thébaudeau benoit.thebaudeau@gmail.com

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] cmd_fuse: return CMD_RET_FAILURE on error

[Benoît Thébaudeau]

Dear Hector Palacios,

On Wed, Nov 19, 2014 at 4:52 PM, Hector Palacios
hector.palac...@digi.com wrote:
 Fuse drivers, like the mxs_ocotp.c, may return negative error codes but
 the commands are not allowed to return negative error codes to the shell,

Correct, except -1 for CMD_RET_USAGE.

 otherwise the following error appears:

 exit not allowed from main input shell.

Not for any negative error.

The change is correct, but maybe the commit message should be reworded
to be more precise. You could perhaps just say that this error message
may appear because of unsupported return codes, and that only the
CMD_RET_* enum values should be used as command return values.


 Signed-off-by: Hector Palacios hector.palac...@digi.com
 ---
  common/cmd_fuse.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/common/cmd_fuse.c b/common/cmd_fuse.c
 index abab9789b0df..d4bc0f6c94a1 100644
 --- a/common/cmd_fuse.c
 +++ b/common/cmd_fuse.c
 @@ -128,7 +128,7 @@ static int do_fuse(cmd_tbl_t *cmdtp, int flag, int argc, 
 char *const argv[])

  err:
 puts(ERROR\n);
 -   return ret;
 +   return CMD_RET_FAILURE;

Correct. BTW, you could perhaps replace the 'return 0;' above 'err:'
with 'return CMD_RET_SUCCESS;' as a cosmetic change enforcing the use
of CMD_RET_* values.

  }

  U_BOOT_CMD(

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] SPL broken on i.mx31 platforms

[Benoît Thébaudeau]

Hi Magnus,

On Thu, Sep 4, 2014 at 9:12 PM, Magnus Lilja lilja.mag...@gmail.com wrote:
 On 3 September 2014 03:13, Tom Rini tr...@ti.com wrote:
 On Wed, Sep 03, 2014 at 02:53:17AM +0200, Benoît Thébaudeau wrote:
  IMHO, the 'b reset' and the 'nop nop nop' are two different issues, so
  Helmut should create a formal patch for the 'b reset' issue right now,
  which will fix mx31pdk (and maybe other boards) for the release. Then,
  once the 'nop nop nop' issue has been resolved for TT-01 (cache issue
  or something else), another formal patch should be created for this
  issue, unless it is purely out of tree.

 v2014.10 is getting closer with the release of -rc2. It would be much
 better to get mx31pdk fixed for this release. Helmut, can you send a
 patch for the 'b reset' issue? If not, do you agree that someone else
 (maybe the board maintainer: Magnus?) sends it with a 'Reported-by:
 Helmut Raiger helmut.rai...@hale.at'?

 Yes, please, I'd like to see this fixed for the release proper.

 I'll try to test and submit a match in a couple of days.

I have already submitted a patch for that yesterday. It's 'arm: Make
reset position-independent', and you were in Cc. So you just have to
test and to add your 'Tested-by'.

Thanks in advance.

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 2/2] arm: Fix _start for CONFIG_SYS_DV_NOR_BOOT_CFG

[Benoît Thébaudeau]

The boards using CONFIG_SYS_DV_NOR_BOOT_CFG (i.e. calimain,
da850evm_direct_nor and enbw_cmc) had the _start symbol defined after
the CONFIG_SYS_DV_NOR_BOOT_CFG word rather than before it in
arch/arm/lib/vectors.S. Because of that, if by lack of luck
'gd-mon_len = (ulong)__bss_end - (ulong)_start' (see setup_mon_len())
was a multiple of 4 kiB (see reserve_uboot()), then the last BSS word
overlapped the first word of the following reserved RAM area (or went
beyond the top of RAM without such an area) after relocation because
__image_copy_start did not match _start (see relocate_code()).

This was broken by commit 41623c9 'arm: move exception handling out of
start.S files', which defined _start twice (before and after the
CONFIG_SYS_DV_NOR_BOOT_CFG word), then by commit 0a26e1d 'arm: fix a
double-definition error of _start symbol', which kept the definition of
the _start symbol after the CONFIG_SYS_DV_NOR_BOOT_CFG word. This new
commit fixes this issue by restoring the original behavior, i.e. by
defining the _start symbol before the CONFIG_SYS_DV_NOR_BOOT_CFG word.

Signed-off-by: Benoît Thébaudeau benoit.thebaudeau@gmail.com
Cc: Albert Aribaud albert.u.b...@aribaud.net
Cc: Manfred Rudigier manfred.rudig...@omicron.at
Cc: Christian Riesch christian.rie...@omicron.at
Cc: Sudhakar Rajashekhara sudhakar@ti.com
Cc: Heiko Schocher h...@denx.de
---
 arch/arm/lib/vectors.S |3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/lib/vectors.S b/arch/arm/lib/vectors.S
index 843b18f..0cb87ce 100644
--- a/arch/arm/lib/vectors.S
+++ b/arch/arm/lib/vectors.S
@@ -45,11 +45,12 @@
  *
  */
 
+_start:
+
 #ifdef CONFIG_SYS_DV_NOR_BOOT_CFG
.word   CONFIG_SYS_DV_NOR_BOOT_CFG
 #endif
 
-_start:
b   reset
ldr pc, _undefined_instruction
ldr pc, _software_interrupt
-- 
1.7.10.4

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 1/2] arm: Make reset position-independent

[Benoît Thébaudeau]

Some boards, like mx31pdk and tx25, require the beginning of the SPL
code to be position-independent. For these two boards, this is because
they use the i.MX external NAND boot, which starts by executing the
first NAND Flash page from the NFC page buffer. The SPL then needs to
copy itself to its actual link address in order to free the NFC page
buffer and use it to load the non-SPL image from Flash before running
it. This means that the SPL runtime address differs from its link
address between the reset and the initial copy performed by
board_init_f(), so this part of the SPL binary must be
position-independent.

This requirement was broken by commit 41623c9 'arm: move exception
handling out of start.S files', which used an absolute address to branch
to the reset routine. This new commit restores the original behavior,
which just performed a relative branch. This fixes the boot of mx31pdk
and tx25.

Signed-off-by: Benoît Thébaudeau benoit.thebaudeau@gmail.com
Reported-by: Helmut Raiger helmut.rai...@hale.at
Cc: Albert Aribaud albert.u.b...@aribaud.net
Cc: Magnus Lilja lilja.mag...@gmail.com
Cc: John Rigby jcri...@gmail.com
---
 arch/arm/lib/vectors.S |3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm/lib/vectors.S b/arch/arm/lib/vectors.S
index 493f337..843b18f 100644
--- a/arch/arm/lib/vectors.S
+++ b/arch/arm/lib/vectors.S
@@ -50,7 +50,7 @@
 #endif
 
 _start:
-   ldr pc, _reset
+   b   reset
ldr pc, _undefined_instruction
ldr pc, _software_interrupt
ldr pc, _prefetch_abort
@@ -77,7 +77,6 @@ _start:
.globl  _irq
.globl  _fiq
 
-_reset:.word reset
 _undefined_instruction:.word undefined_instruction
 _software_interrupt:   .word software_interrupt
 _prefetch_abort:   .word prefetch_abort
-- 
1.7.10.4

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 1/2] arm: Make reset position-independent

[Benoît Thébaudeau]

On Wed, Sep 3, 2014 at 11:32 PM, Benoît Thébaudeau
benoit.thebaudeau@gmail.com wrote:
 Some boards, like mx31pdk and tx25, require the beginning of the SPL
 code to be position-independent. For these two boards, this is because
 they use the i.MX external NAND boot, which starts by executing the
 first NAND Flash page from the NFC page buffer. The SPL then needs to
 copy itself to its actual link address in order to free the NFC page
 buffer and use it to load the non-SPL image from Flash before running
 it. This means that the SPL runtime address differs from its link
 address between the reset and the initial copy performed by
 board_init_f(), so this part of the SPL binary must be
 position-independent.

 This requirement was broken by commit 41623c9 'arm: move exception
 handling out of start.S files', which used an absolute address to branch
 to the reset routine. This new commit restores the original behavior,
 which just performed a relative branch. This fixes the boot of mx31pdk
 and tx25.

 Signed-off-by: Benoît Thébaudeau benoit.thebaudeau@gmail.com
 Reported-by: Helmut Raiger helmut.rai...@hale.at
 Cc: Albert Aribaud albert.u.b...@aribaud.net
 Cc: Magnus Lilja lilja.mag...@gmail.com
 Cc: John Rigby jcri...@gmail.com
 ---
  arch/arm/lib/vectors.S |3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

 diff --git a/arch/arm/lib/vectors.S b/arch/arm/lib/vectors.S
 index 493f337..843b18f 100644
 --- a/arch/arm/lib/vectors.S
 +++ b/arch/arm/lib/vectors.S
 @@ -50,7 +50,7 @@
  #endif

  _start:
 -   ldr pc, _reset
 +   b   reset
 ldr pc, _undefined_instruction
 ldr pc, _software_interrupt
 ldr pc, _prefetch_abort
 @@ -77,7 +77,6 @@ _start:
 .globl  _irq
 .globl  _fiq

 -_reset:.word reset
  _undefined_instruction:.word undefined_instruction
  _software_interrupt:   .word software_interrupt
  _prefetch_abort:   .word prefetch_abort
 --
 1.7.10.4


Magnus, can I have your 'Tested-by' for mx31pdk since you said you
have tested this?

Tom, Albert, can you build-test all ARM boards with this patch (that
would take eons on my ultra slow machine)? It would also be nice if
someone could runtime-test an ARM board other than mx31pdk and tx25
with this patch.

Thanks in advance.

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 1/2] arm: Make reset position-independent

[Benoît Thébaudeau]

On Wed, Sep 3, 2014 at 11:32 PM, Benoît Thébaudeau
benoit.thebaudeau@gmail.com wrote:
 Some boards, like mx31pdk and tx25, require the beginning of the SPL
 code to be position-independent. For these two boards, this is because
 they use the i.MX external NAND boot, which starts by executing the
 first NAND Flash page from the NFC page buffer. The SPL then needs to
 copy itself to its actual link address in order to free the NFC page
 buffer and use it to load the non-SPL image from Flash before running
 it. This means that the SPL runtime address differs from its link
 address between the reset and the initial copy performed by
 board_init_f(), so this part of the SPL binary must be
 position-independent.

 This requirement was broken by commit 41623c9 'arm: move exception
 handling out of start.S files', which used an absolute address to branch
 to the reset routine. This new commit restores the original behavior,
 which just performed a relative branch. This fixes the boot of mx31pdk
 and tx25.

 Signed-off-by: Benoît Thébaudeau benoit.thebaudeau@gmail.com
 Reported-by: Helmut Raiger helmut.rai...@hale.at
 Cc: Albert Aribaud albert.u.b...@aribaud.net
 Cc: Magnus Lilja lilja.mag...@gmail.com
 Cc: John Rigby jcri...@gmail.com
 ---
  arch/arm/lib/vectors.S |3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

 diff --git a/arch/arm/lib/vectors.S b/arch/arm/lib/vectors.S
 index 493f337..843b18f 100644
 --- a/arch/arm/lib/vectors.S
 +++ b/arch/arm/lib/vectors.S
 @@ -50,7 +50,7 @@
  #endif

  _start:
 -   ldr pc, _reset
 +   b   reset
 ldr pc, _undefined_instruction
 ldr pc, _software_interrupt
 ldr pc, _prefetch_abort
 @@ -77,7 +77,6 @@ _start:
 .globl  _irq
 .globl  _fiq

 -_reset:.word reset
  _undefined_instruction:.word undefined_instruction
  _software_interrupt:   .word software_interrupt
  _prefetch_abort:   .word prefetch_abort
 --
 1.7.10.4


Adding Helmut to Cc.

Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] SPL broken on i.mx31 platforms

[Benoît Thébaudeau]

Hi Helmut, all,

On Tue, Aug 19, 2014 at 10:55 PM, Benoît Thébaudeau
benoit.thebaudeau@gmail.com wrote:
 On Fri, Aug 15, 2014 at 7:45 PM, Magnus Lilja lilja.mag...@gmail.com wrote:
 On 13 August 2014 14:01, Helmut Raiger helmut.rai...@hale.at wrote:
 On 08/05/2014 02:32 PM, Magnus Lilja wrote:
 I would expect Helmut to create a formal patch then I can test that
 and add a Tested-by.

 The problem is it does not work with only the 'b reset' change on my
 platform.
 Should I provide a patch with the nops and the question marks around them?
 It still could be a toolchain difference, mine is pretty old:

 $ arm-angstrom-linux-gnueabi-gcc --version
 arm-angstrom-linux-gnueabi-gcc (GCC) 4.7.2
 Copyright (C) 2012 Free Software Foundation, Inc.

 When I objdump the elf file I can see the very same code in cpu_init_crit()
 as in start.S,
 whatever that might mean (objdump is from the same toolchain).

 I use an even older gcc so I don't think that's the problem. I use:
 arm-none-linux-gnueabi-gcc (Sourcery CodeBench Lite 2011.09-70) 4.6.1

 Not sure how you should proceed with the path.

 IMHO, the 'b reset' and the 'nop nop nop' are two different issues, so
 Helmut should create a formal patch for the 'b reset' issue right now,
 which will fix mx31pdk (and maybe other boards) for the release. Then,
 once the 'nop nop nop' issue has been resolved for TT-01 (cache issue
 or something else), another formal patch should be created for this
 issue, unless it is purely out of tree.

v2014.10 is getting closer with the release of -rc2. It would be much
better to get mx31pdk fixed for this release. Helmut, can you send a
patch for the 'b reset' issue? If not, do you agree that someone else
(maybe the board maintainer: Magnus?) sends it with a 'Reported-by:
Helmut Raiger helmut.rai...@hale.at'?

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH v2] arm: Add missing .vectors section to linker scripts

[Benoît Thébaudeau]

Commit 41623c9 'arm: move exception handling out of start.S files' missed some
linker scripts. Hence, some boards no longer had exception handling linked since
this commit. Restore the original behavior by adding the .vectors section to
these linker scripts.

Signed-off-by: Benoît Thébaudeau benoit.thebaudeau@gmail.com
Cc: Albert ARIBAUD albert.u.b...@aribaud.net

---

Changes in v2:
 - Reword the patch subject from 'arm: fix missing exception handling' to 'arm:
   Add missing .vectors section to linker scripts'.
---
 arch/arm/cpu/arm1136/u-boot-spl.lds| 1 +
 arch/arm/cpu/arm926ejs/mxs/u-boot-spl.lds  | 1 +
 arch/arm/cpu/armv7/am33xx/u-boot-spl.lds   | 1 +
 arch/arm/cpu/armv7/omap-common/u-boot-spl.lds  | 1 +
 arch/arm/cpu/armv7/socfpga/u-boot-spl.lds  | 1 +
 arch/arm/cpu/armv7/zynq/u-boot-spl.lds | 1 +
 arch/arm/cpu/at91-common/u-boot-spl.lds| 1 +
 board/Barix/ipam390/u-boot-spl-ipam390.lds | 1 +
 board/ait/cam_enc_4xx/u-boot-spl.lds   | 1 +
 board/cirrus/edb93xx/u-boot.lds| 1 +
 board/davinci/da8xxevm/u-boot-spl-da850evm.lds | 1 +
 board/davinci/da8xxevm/u-boot-spl-hawk.lds | 1 +
 board/samsung/common/exynos-uboot-spl.lds  | 1 +
 board/vpac270/u-boot-spl.lds   | 1 +
 14 files changed, 14 insertions(+)

diff --git a/arch/arm/cpu/arm1136/u-boot-spl.lds 
b/arch/arm/cpu/arm1136/u-boot-spl.lds
index 0299902..97e4a8b 100644
--- a/arch/arm/cpu/arm1136/u-boot-spl.lds
+++ b/arch/arm/cpu/arm1136/u-boot-spl.lds
@@ -22,6 +22,7 @@ SECTIONS
.text  :
{
__start = .;
+ *(.vectors)
  arch/arm/cpu/arm1136/start.o  (.text*)
  *(.text*)
} .sram
diff --git a/arch/arm/cpu/arm926ejs/mxs/u-boot-spl.lds 
b/arch/arm/cpu/arm926ejs/mxs/u-boot-spl.lds
index f4bf8ac..bf2ac13 100644
--- a/arch/arm/cpu/arm926ejs/mxs/u-boot-spl.lds
+++ b/arch/arm/cpu/arm926ejs/mxs/u-boot-spl.lds
@@ -21,6 +21,7 @@ SECTIONS
. = ALIGN(4);
.text   :
{
+   *(.vectors)
arch/arm/cpu/arm926ejs/mxs/start.o  (.text*)
*(.text*)
}
diff --git a/arch/arm/cpu/armv7/am33xx/u-boot-spl.lds 
b/arch/arm/cpu/armv7/am33xx/u-boot-spl.lds
index b1c28c9..07cf267 100644
--- a/arch/arm/cpu/armv7/am33xx/u-boot-spl.lds
+++ b/arch/arm/cpu/armv7/am33xx/u-boot-spl.lds
@@ -22,6 +22,7 @@ SECTIONS
.text  :
{
__start = .;
+   *(.vectors)
arch/arm/cpu/armv7/start.o  (.text)
*(.text*)
} .sram
diff --git a/arch/arm/cpu/armv7/omap-common/u-boot-spl.lds 
b/arch/arm/cpu/armv7/omap-common/u-boot-spl.lds
index 745603d..ccd0c83 100644
--- a/arch/arm/cpu/armv7/omap-common/u-boot-spl.lds
+++ b/arch/arm/cpu/armv7/omap-common/u-boot-spl.lds
@@ -22,6 +22,7 @@ SECTIONS
.text  :
{
__start = .;
+   *(.vectors)
arch/arm/cpu/armv7/start.o  (.text*)
*(.text*)
} .sram
diff --git a/arch/arm/cpu/armv7/socfpga/u-boot-spl.lds 
b/arch/arm/cpu/armv7/socfpga/u-boot-spl.lds
index 4282beb..db9bdad 100644
--- a/arch/arm/cpu/armv7/socfpga/u-boot-spl.lds
+++ b/arch/arm/cpu/armv7/socfpga/u-boot-spl.lds
@@ -16,6 +16,7 @@ SECTIONS
. = ALIGN(4);
.text   :
{
+   *(.vectors)
arch/arm/cpu/armv7/start.o  (.text*)
*(.text*)
} .sdram
diff --git a/arch/arm/cpu/armv7/zynq/u-boot-spl.lds 
b/arch/arm/cpu/armv7/zynq/u-boot-spl.lds
index 0c4501e..0f2f756 100644
--- a/arch/arm/cpu/armv7/zynq/u-boot-spl.lds
+++ b/arch/arm/cpu/armv7/zynq/u-boot-spl.lds
@@ -22,6 +22,7 @@ SECTIONS
.text :
{
__image_copy_start = .;
+   *(.vectors)
CPUDIR/start.o (.text*)
*(.text*)
}  .sram
diff --git a/arch/arm/cpu/at91-common/u-boot-spl.lds 
b/arch/arm/cpu/at91-common/u-boot-spl.lds
index 57ac1eb..eccca43 100644
--- a/arch/arm/cpu/at91-common/u-boot-spl.lds
+++ b/arch/arm/cpu/at91-common/u-boot-spl.lds
@@ -25,6 +25,7 @@ SECTIONS
.text  :
{
__start = .;
+   *(.vectors)
arch/arm/cpu/armv7/start.o  (.text*)
*(.text*)
} .sram
diff --git a/board/Barix/ipam390/u-boot-spl-ipam390.lds 
b/board/Barix/ipam390/u-boot-spl-ipam390.lds
index 8604696..5f290ec 100644
--- a/board/Barix/ipam390/u-boot-spl-ipam390.lds
+++ b/board/Barix/ipam390/u-boot-spl-ipam390.lds
@@ -22,6 +22,7 @@ SECTIONS
.text  :
{
__start = .;
+ *(.vectors)
  arch/arm/cpu/arm926ejs/start.o(.text*)
  *(.text*)
} .sram
diff --git a/board/ait/cam_enc_4xx/u-boot-spl.lds 
b/board/ait/cam_enc_4xx/u-boot-spl.lds
index c0d09ad..f5c19df 100644
--- a/board/ait/cam_enc_4xx/u-boot-spl.lds
+++ b/board/ait/cam_enc_4xx/u-boot-spl.lds
@@ -22,6 +22,7 @@ SECTIONS
.text

Re: [U-Boot] [PATCH] arm: fix missing exception handling

[Benoît Thébaudeau]

On Thu, Aug 21, 2014 at 8:36 AM, Christian Riesch
christian.rie...@omicron.at wrote:
 Benoît,

 On Wed, Aug 20, 2014 at 12:47 PM, Benoît Thébaudeau
 benoit.thebaudeau@gmail.com wrote:
 On Wed, Aug 20, 2014 at 9:21 AM, Christian Riesch
 christian.rie...@omicron.at wrote:
 On Tue, Aug 19, 2014 at 8:35 PM, Benoît Thébaudeau
 benoit.thebaudeau@gmail.com wrote:
 Commit 41623c9 'arm: move exception handling out of start.S files' missed 
 some
 linker scripts. Hence, some boards no longer had exception handling linked 
 since
 this commit. Restore the original behavior by adding the .vectors section 
 to
 these linker scripts.

 Signed-off-by: Benoît Thébaudeau benoit.thebaudeau@gmail.com
 Cc: Albert ARIBAUD albert.u.b...@aribaud.net

 Does this really _fix_ exception handling for all of these boards (as
 your message subject states) or just restore the state before commit
 41623c9? I believe exception handling has never worked for most of
 these boards, they don't use it. See the discussion in [1], in
 particular [2] and [3].

 Regards, Christian

 [1] http://marc.info/?t=14024710541r=1w=2
 [2] http://marc.info/?l=u-bootm=140309659906074w=2
 [3] http://marc.info/?l=u-bootm=140450616517545w=2

 The issue is that commit 41623c9 silently removed exception handling
 for many boards. Whether it was working or not for these boards before
 this commit is another issue. My patch only fixes _missing_ exception
 handling, not exception handling itself. I had discussed that with
 Albert here:
 http://lists.denx.de/pipermail/u-boot/2014-July/183073.html

 Thanks for the pointer to the discussion.

 What do you suggest?
  - Changing the wording? In which way?

 Yes, how about Add missing .vectors section to linker scripts?

Superseded by:
http://patchwork.ozlabs.org/patch/381971/

Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] mx6: Fix cacheline size

[Benoît Thébaudeau]

Hi Fabio,

On Thu, Aug 21, 2014 at 9:11 PM, Fabio Estevam feste...@gmail.com wrote:
 On Thu, Aug 21, 2014 at 2:14 PM, Marek Vasut ma...@denx.de wrote:
 On Thursday, August 21, 2014 at 07:10:02 PM, Fabio Estevam wrote:
 mx6 is an armv7 which has 64-byte cacheline size.

 Without this fix we are not able to get the FEC driver to work on mx6solox.

 64-byte cacheline is also used by the kernel on ARMv7, so fix it
 accordingly.

 It's not a kernel thing, it's architecture thing. Otherwise,

 Acked-by: Marek Vasut ma...@denx.de

 Actually the CortexA9 manual says:
 http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0388f/Caccifbd.html

 The cache line length is eight words.

 and the mx6q RM says:

 12.5.4.1 L1 features
 
 • Eight 32-bit words per cache line

 So the current CONFIG_SYS_CACHELINE_SIZE of 32 is correct for mx6.

Yes, it's always 32 bytes for Cortex-A9. But does mx6solox really have
a standard Cortex-A9 core like all the currently released i.MX6 SoCs
(which seems to be the case according to
http://lwn.net/Articles/598434/), or a Cortex-A9 implementation with
non-standard parameters tuned by Freescale, or even another core like
a Cortex-A8?

 In kernel we use 64-bytes of cache line for armv7 though:

 config ARM_L1_CACHE_SHIFT_6
 bool
 default y if CPU_V7
 help
   Setting ARM L1 cache line size to 64 Bytes.

This seems to be used only for alignments, and stuff aligned with 64
bytes is also aligned with 32 bytes, so this should not matter.
However, [1] reads the cache line size from the registers, and it does
not use this constant.

Regards,
Benoît

[1] 
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/mm/cache-v7.S?id=refs/tags/v3.17-rc1
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] arm: fix missing exception handling

[Benoît Thébaudeau]

Christian,

On Wed, Aug 20, 2014 at 9:21 AM, Christian Riesch
christian.rie...@omicron.at wrote:
 Benoît,

 On Tue, Aug 19, 2014 at 8:35 PM, Benoît Thébaudeau
 benoit.thebaudeau@gmail.com wrote:
 Commit 41623c9 'arm: move exception handling out of start.S files' missed 
 some
 linker scripts. Hence, some boards no longer had exception handling linked 
 since
 this commit. Restore the original behavior by adding the .vectors section to
 these linker scripts.

 Signed-off-by: Benoît Thébaudeau benoit.thebaudeau@gmail.com
 Cc: Albert ARIBAUD albert.u.b...@aribaud.net

 Does this really _fix_ exception handling for all of these boards (as
 your message subject states) or just restore the state before commit
 41623c9? I believe exception handling has never worked for most of
 these boards, they don't use it. See the discussion in [1], in
 particular [2] and [3].

 Regards, Christian

 [1] http://marc.info/?t=14024710541r=1w=2
 [2] http://marc.info/?l=u-bootm=140309659906074w=2
 [3] http://marc.info/?l=u-bootm=140450616517545w=2

The issue is that commit 41623c9 silently removed exception handling
for many boards. Whether it was working or not for these boards before
this commit is another issue. My patch only fixes _missing_ exception
handling, not exception handling itself. I had discussed that with
Albert here:
http://lists.denx.de/pipermail/u-boot/2014-July/183073.html

What do you suggest?
 - Changing the wording? In which way?
 - Dropping some boards from this patch? Which ones? I think that
there should be an explicit commit from the board maintainers doing
that, rather than just commit 41623c9.

Regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH] arm: fix missing exception handling

[Benoît Thébaudeau]

Commit 41623c9 'arm: move exception handling out of start.S files' missed some
linker scripts. Hence, some boards no longer had exception handling linked since
this commit. Restore the original behavior by adding the .vectors section to
these linker scripts.

Signed-off-by: Benoît Thébaudeau benoit.thebaudeau@gmail.com
Cc: Albert ARIBAUD albert.u.b...@aribaud.net
---
 arch/arm/cpu/arm1136/u-boot-spl.lds| 1 +
 arch/arm/cpu/arm926ejs/mxs/u-boot-spl.lds  | 1 +
 arch/arm/cpu/armv7/am33xx/u-boot-spl.lds   | 1 +
 arch/arm/cpu/armv7/omap-common/u-boot-spl.lds  | 1 +
 arch/arm/cpu/armv7/socfpga/u-boot-spl.lds  | 1 +
 arch/arm/cpu/armv7/zynq/u-boot-spl.lds | 1 +
 arch/arm/cpu/at91-common/u-boot-spl.lds| 1 +
 board/Barix/ipam390/u-boot-spl-ipam390.lds | 1 +
 board/ait/cam_enc_4xx/u-boot-spl.lds   | 1 +
 board/cirrus/edb93xx/u-boot.lds| 1 +
 board/davinci/da8xxevm/u-boot-spl-da850evm.lds | 1 +
 board/davinci/da8xxevm/u-boot-spl-hawk.lds | 1 +
 board/samsung/common/exynos-uboot-spl.lds  | 1 +
 board/vpac270/u-boot-spl.lds   | 1 +
 14 files changed, 14 insertions(+)

diff --git a/arch/arm/cpu/arm1136/u-boot-spl.lds 
b/arch/arm/cpu/arm1136/u-boot-spl.lds
index 0299902..97e4a8b 100644
--- a/arch/arm/cpu/arm1136/u-boot-spl.lds
+++ b/arch/arm/cpu/arm1136/u-boot-spl.lds
@@ -22,6 +22,7 @@ SECTIONS
.text  :
{
__start = .;
+ *(.vectors)
  arch/arm/cpu/arm1136/start.o  (.text*)
  *(.text*)
} .sram
diff --git a/arch/arm/cpu/arm926ejs/mxs/u-boot-spl.lds 
b/arch/arm/cpu/arm926ejs/mxs/u-boot-spl.lds
index f4bf8ac..bf2ac13 100644
--- a/arch/arm/cpu/arm926ejs/mxs/u-boot-spl.lds
+++ b/arch/arm/cpu/arm926ejs/mxs/u-boot-spl.lds
@@ -21,6 +21,7 @@ SECTIONS
. = ALIGN(4);
.text   :
{
+   *(.vectors)
arch/arm/cpu/arm926ejs/mxs/start.o  (.text*)
*(.text*)
}
diff --git a/arch/arm/cpu/armv7/am33xx/u-boot-spl.lds 
b/arch/arm/cpu/armv7/am33xx/u-boot-spl.lds
index b1c28c9..07cf267 100644
--- a/arch/arm/cpu/armv7/am33xx/u-boot-spl.lds
+++ b/arch/arm/cpu/armv7/am33xx/u-boot-spl.lds
@@ -22,6 +22,7 @@ SECTIONS
.text  :
{
__start = .;
+   *(.vectors)
arch/arm/cpu/armv7/start.o  (.text)
*(.text*)
} .sram
diff --git a/arch/arm/cpu/armv7/omap-common/u-boot-spl.lds 
b/arch/arm/cpu/armv7/omap-common/u-boot-spl.lds
index 745603d..ccd0c83 100644
--- a/arch/arm/cpu/armv7/omap-common/u-boot-spl.lds
+++ b/arch/arm/cpu/armv7/omap-common/u-boot-spl.lds
@@ -22,6 +22,7 @@ SECTIONS
.text  :
{
__start = .;
+   *(.vectors)
arch/arm/cpu/armv7/start.o  (.text*)
*(.text*)
} .sram
diff --git a/arch/arm/cpu/armv7/socfpga/u-boot-spl.lds 
b/arch/arm/cpu/armv7/socfpga/u-boot-spl.lds
index 4282beb..db9bdad 100644
--- a/arch/arm/cpu/armv7/socfpga/u-boot-spl.lds
+++ b/arch/arm/cpu/armv7/socfpga/u-boot-spl.lds
@@ -16,6 +16,7 @@ SECTIONS
. = ALIGN(4);
.text   :
{
+   *(.vectors)
arch/arm/cpu/armv7/start.o  (.text*)
*(.text*)
} .sdram
diff --git a/arch/arm/cpu/armv7/zynq/u-boot-spl.lds 
b/arch/arm/cpu/armv7/zynq/u-boot-spl.lds
index 0c4501e..0f2f756 100644
--- a/arch/arm/cpu/armv7/zynq/u-boot-spl.lds
+++ b/arch/arm/cpu/armv7/zynq/u-boot-spl.lds
@@ -22,6 +22,7 @@ SECTIONS
.text :
{
__image_copy_start = .;
+   *(.vectors)
CPUDIR/start.o (.text*)
*(.text*)
}  .sram
diff --git a/arch/arm/cpu/at91-common/u-boot-spl.lds 
b/arch/arm/cpu/at91-common/u-boot-spl.lds
index 57ac1eb..eccca43 100644
--- a/arch/arm/cpu/at91-common/u-boot-spl.lds
+++ b/arch/arm/cpu/at91-common/u-boot-spl.lds
@@ -25,6 +25,7 @@ SECTIONS
.text  :
{
__start = .;
+   *(.vectors)
arch/arm/cpu/armv7/start.o  (.text*)
*(.text*)
} .sram
diff --git a/board/Barix/ipam390/u-boot-spl-ipam390.lds 
b/board/Barix/ipam390/u-boot-spl-ipam390.lds
index 8604696..5f290ec 100644
--- a/board/Barix/ipam390/u-boot-spl-ipam390.lds
+++ b/board/Barix/ipam390/u-boot-spl-ipam390.lds
@@ -22,6 +22,7 @@ SECTIONS
.text  :
{
__start = .;
+ *(.vectors)
  arch/arm/cpu/arm926ejs/start.o(.text*)
  *(.text*)
} .sram
diff --git a/board/ait/cam_enc_4xx/u-boot-spl.lds 
b/board/ait/cam_enc_4xx/u-boot-spl.lds
index c0d09ad..f5c19df 100644
--- a/board/ait/cam_enc_4xx/u-boot-spl.lds
+++ b/board/ait/cam_enc_4xx/u-boot-spl.lds
@@ -22,6 +22,7 @@ SECTIONS
.text  :
{
__start = .;
+ *(.vectors)
  arch/arm/cpu/arm926ejs/start.o(.text*)
  *(.text*)
} .sram
diff --git

Re: [U-Boot] SPL broken on i.mx31 platforms

[Benoît Thébaudeau]

Hi Magnus, all,

On Fri, Aug 15, 2014 at 7:45 PM, Magnus Lilja lilja.mag...@gmail.com wrote:
 Hi

 On 13 August 2014 14:01, Helmut Raiger helmut.rai...@hale.at wrote:
 On 08/05/2014 02:32 PM, Magnus Lilja wrote:

 Hi Fabio,

 On 5 August 2014 14:28, Fabio Estevam feste...@gmail.com wrote:

 Hi Magnus,

 I would expect Helmut to create a formal patch then I can test that
 and add a Tested-by.

 The problem is it does not work with only the 'b reset' change on my
 platform.
 Should I provide a patch with the nops and the question marks around them?
 It still could be a toolchain difference, mine is pretty old:

 $ arm-angstrom-linux-gnueabi-gcc --version
 arm-angstrom-linux-gnueabi-gcc (GCC) 4.7.2
 Copyright (C) 2012 Free Software Foundation, Inc.

 When I objdump the elf file I can see the very same code in cpu_init_crit()
 as in start.S,
 whatever that might mean (objdump is from the same toolchain).

 I use an even older gcc so I don't think that's the problem. I use:
 arm-none-linux-gnueabi-gcc (Sourcery CodeBench Lite 2011.09-70) 4.6.1

 Not sure how you should proceed with the path.

IMHO, the 'b reset' and the 'nop nop nop' are two different issues, so
Helmut should create a formal patch for the 'b reset' issue right now,
which will fix mx31pdk (and maybe other boards) for the release. Then,
once the 'nop nop nop' issue has been resolved for TT-01 (cache issue
or something else), another formal patch should be created for this
issue, unless it is purely out of tree.

Albert, Fabio, what do you think?

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 0/8] zmx25: Add hardware support

[Benoît Thébaudeau]

Hi Thomas,

On Mon, Aug 4, 2014 at 7:59 AM, Thomas Diener die...@gmx.de wrote:
 Hi folks,
 do you have any comments or statements for this patches?

You already have received replies to 1, 3, 4 and 8 / 8 in May:
http://lists.denx.de/pipermail/u-boot/2014-May/179759.html
http://lists.denx.de/pipermail/u-boot/2014-May/179654.html
http://lists.denx.de/pipermail/u-boot/2014-May/180530.html
http://lists.denx.de/pipermail/u-boot/2014-May/179731.html

Best regards,
Benoît
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] SPL broken on i.mx31 platforms

[Benoît Thébaudeau]

Hi Albert,

On Fri, Jul 4, 2014 at 10:50 AM, Albert ARIBAUD
albert.u.b...@aribaud.net wrote:
 On Thu, 3 Jul 2014 22:58:56 +0200, Benoît Thébaudeau
 benoit.thebaudeau@gmail.com wrote:
 On Thu, Jul 3, 2014 at 3:35 PM, Albert ARIBAUD
 albert.u.b...@aribaud.net wrote:
  On Thu, 03 Jul 2014 10:19:39 +0200, Helmut Raiger
  helmut.rai...@hale.at wrote:
  On 07/03/2014 01:20 AM, Benoît Thébaudeau wrote:
   On Wed, Jul 2, 2014 at 9:04 AM, Helmut Raiger helmut.rai...@hale.at 
   wrote:
 the commit 41623c91 breaks the SPL on i.mx31 platforms.
   Here, you are talking about mx31pdk, right?
  Actually im talking TT-01, but it has no contributed NAND boot code 
  (which I
  was working on), but it should hit mx31pdk in the same way.
  This should answer Albert's question about the board.
 
  It does, thanks -- but I fail to see any SPL code built for TT-01.
  You're getting SPL issues with another target, right?

 Helmut seems to be working on a custom TT-01 variant (or just with a
 specific hardware configuration using dip switches, or jumpers, etc.)
 using the i.MX31 NAND internal boot rather than the mainline boot
 source.

   No, it will only be a minor change, I think, but I thought there might
   have been an additional intention behind the change to position
   dependent code. One could link the first part to 0xB800
   (the original position of the SPL when loaded by the IPL) and
   the part after the relocation to CONFIG_SPL_TEXT_BASE.
   Actually, the ROM bootloader first copies the first NAND page to
   0xB800. Then, the SPL placed here but linked at
   CONFIG_SPL_TEXT_BASE copies itself to CONFIG_SPL_TEXT_BASE in order
   to free the NFC buffer so that it can be used by the SPL. There is
   no relocation going on at this stage, but only a copy, and the SPL
   code size is limited to 2 kiB. Then, the SPL does its NAND load job
   towards CONFIG_SYS_TEXT_BASE and starts executing the non-SPL
   binary, which then relocates itself according to the heap size, etc.
 
  Ok, I think I'm getting it, but actually you don't need PIC (your code
  won't run at arbitrary locations), you need VMAs vs LMAs (your code
  will run partly at one location, partly at another, but will be loaded
  at only one of these locations).
 
  Therefore, we should be able to manage this in the linker script, by
  basically defining two output sections: the first one with a VMA and
  LMA equal to 0xB800 both, and which would contain the 'copier' code;
  and the second one with a VMA equal to CONFIG_SPL_TEXT_BASE (so that it
  links properly for running at that address) and a LMA equal to 0xB80
  (so that it gets lumped with the first section in the less-than-2K ELF
  file produced by the linker.
 
  (actually we'd have several output sections with VMA==LMA, but it
  does not affect the core of the idea.)
 
   Does it make sense to you?

 It makes sense to me. That should work, but it'd be better to avoid
 adding a custom linker script. A simple fix in vectors.S would be
 preferable if possible. Also, the __image_copy_start stuff should be
 taken care of with such a change.

 I do not intend to have this in a custom linker script; I want this to
 be in the common ARM SPL linker script (I hope it is what Helmut's
 TT-01 changes use) -- I also want to get rid of all custom linker
 scripts in the ARM par of U-Boot, but I could not get this done for
 2014.07.

Looks good.

 BTW, I see that you skipped arch/arm/cpu/arm1136/u-boot-spl.lds in
 commit 41623c91 (addition of *(.vectors)). Was it intentional?

 Not that I can remember: I did modify the arm1136 start.S, so te linker
 scripts should have followed.

OK. There seems to be the same issue for
board/ait/cam_enc_4xx/u-boot-spl.lds . All the ARM arch and ARM boards
linker scripts should probably be rechecked.

 It silently changes woodburn_sd because the fallback exception vectors no
 longer exist. This should not cause a build error because the _start
 symbol is duplicated in this linker script. The board may also boot
 correctly with this, but the default vectors can be useful in some
 cases, especially for debugging exceptions.

 Can you post a patch today? I'll pull it in a PR I'll do today before
 COB.

Sorry, I can't before next week at best.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot