Re: [U-Boot] [PATCH v4 0/7] am57xx: cl-som-am57x: fix usb

[Igor Grinberg]

On 05/05/17 17:17, Tom Rini wrote:
> On Thu, May 04, 2017 at 10:05:22AM +0300, Igor Grinberg wrote:
>> Hi Tom, Marek,
>>
>> On 04/25/17 22:27, Marek Vasut wrote:
>>> On 04/25/2017 03:09 AM, Tom Rini wrote:
>>>> On Sun, Apr 23, 2017 at 11:18:04AM +0300, Uri Mashiach wrote:
>>>>> Hello Tom,
>>>>>
>>>>> A gentle ping on this patch series.
>>>>>
>>>>> On 02/23/2017 03:39 PM, Uri Mashiach wrote:
>>>>>> Various USB related comits for the CL-SOM-AM57x module.
>>>>>>
>>>>>> ---
>>>>>> V1 -> V2: Replace commit "fix XHCI registers base address" with 
>>>>>> "reintroduce the CONFIG_AM57XX symbol".
>>>>>> V2 -> V3: * New commit "move CONFIG_DRA7XX to Kconfig"
>>>>>>  * Replace commit "reintroduce the CONFIG_AM57XX symbol" with 
>>>>>> "xHCI registers based on USB port index"
>>>>>> V3 -> V4: Update commit "move CONFIG_DRA7XX to Kconfig"
>>>>>>Update commit "xHCI registers based on USB port index"
>>>>>>
>>>>>> Uri Mashiach (7):
>>>>>>  arm: dra7xx: move CONFIG_DRA7XX to Kconfig
>>>>>>  arm: usb: dra7xx: xHCI registers based on USB port index
>>>>>>  usb: host: xhci-omap: fix double weak board_usb_init functions
>>>>>>  arm: am57xx: cl-som-am57x: invoke clock API to enable/disable clocks
>>>>>>  arm: am57xx: cl-som-am57x: fix USB scan
>>>>>>  arm: am57xx: cl-som-am57x: enable USB storage
>>>>>>  arm: am57xx: cl-som-am57x: enable USB commands
>>>>>>
>>>>>> arch/arm/mach-omap2/omap5/Kconfig  |  8 
>>>>>> board/compulab/cl-som-am57x/cl-som-am57x.c | 10 --
>>>>>> board/ti/am43xx/board.c|  4 ++--
>>>>>> board/ti/am57xx/board.c|  4 ++--
>>>>>> board/ti/dra7xx/evm.c  |  4 ++--
>>>>>> configs/cl-som-am57x_defconfig |  2 ++
>>>>>> configs/dra7xx_evm_defconfig   |  1 +
>>>>>> configs/dra7xx_hs_evm_defconfig|  1 +
>>>>>> drivers/usb/host/Kconfig   |  9 +
>>>>>> drivers/usb/host/xhci-omap.c   | 19 +--
>>>>>> include/configs/am57xx_evm.h   |  2 --
>>>>>> include/configs/cl-som-am57x.h |  6 ++
>>>>>> include/configs/dra7xx_evm.h   |  2 --
>>>>>> include/linux/usb/xhci-omap.h  |  6 --
>>>>>> scripts/config_whitelist.txt   |  1 -
>>>>>> 15 files changed, 50 insertions(+), 29 deletions(-)
>>>>
>>>> Marek?  Thanks!
>>>
>>> Tom?  Thanks!
>>
>> IIRC, it has been decided to take this patch series through the OMAP tree.
>> Contradictory to what I remember, I can see the patches have been delegated 
>> to Marek
>> in patchwork.
> 
> Yeah, I assigned it to Marek so I wouldn't grab it before I was sure he
> was happy with it.  I haven't (until now) taken it back.
> 
>> So, what's happening? Can we please go on with merging this series?
> 
> I'll pick it up after the current release is out, thanks!

Thanks!

-- 
Regards,
Igor.



signature.asc
Description: OpenPGP digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v4 0/7] am57xx: cl-som-am57x: fix usb

[Igor Grinberg]

Hi Tom, Marek,

On 04/25/17 22:27, Marek Vasut wrote:
> On 04/25/2017 03:09 AM, Tom Rini wrote:
>> On Sun, Apr 23, 2017 at 11:18:04AM +0300, Uri Mashiach wrote:
>>> Hello Tom,
>>>
>>> A gentle ping on this patch series.
>>>
>>> On 02/23/2017 03:39 PM, Uri Mashiach wrote:
 Various USB related comits for the CL-SOM-AM57x module.

 ---
 V1 -> V2: Replace commit "fix XHCI registers base address" with 
 "reintroduce the CONFIG_AM57XX symbol".
 V2 -> V3: * New commit "move CONFIG_DRA7XX to Kconfig"
  * Replace commit "reintroduce the CONFIG_AM57XX symbol" with 
 "xHCI registers based on USB port index"
 V3 -> V4: Update commit "move CONFIG_DRA7XX to Kconfig"
  Update commit "xHCI registers based on USB port index"

 Uri Mashiach (7):
  arm: dra7xx: move CONFIG_DRA7XX to Kconfig
  arm: usb: dra7xx: xHCI registers based on USB port index
  usb: host: xhci-omap: fix double weak board_usb_init functions
  arm: am57xx: cl-som-am57x: invoke clock API to enable/disable clocks
  arm: am57xx: cl-som-am57x: fix USB scan
  arm: am57xx: cl-som-am57x: enable USB storage
  arm: am57xx: cl-som-am57x: enable USB commands

 arch/arm/mach-omap2/omap5/Kconfig  |  8 
 board/compulab/cl-som-am57x/cl-som-am57x.c | 10 --
 board/ti/am43xx/board.c|  4 ++--
 board/ti/am57xx/board.c|  4 ++--
 board/ti/dra7xx/evm.c  |  4 ++--
 configs/cl-som-am57x_defconfig |  2 ++
 configs/dra7xx_evm_defconfig   |  1 +
 configs/dra7xx_hs_evm_defconfig|  1 +
 drivers/usb/host/Kconfig   |  9 +
 drivers/usb/host/xhci-omap.c   | 19 +--
 include/configs/am57xx_evm.h   |  2 --
 include/configs/cl-som-am57x.h |  6 ++
 include/configs/dra7xx_evm.h   |  2 --
 include/linux/usb/xhci-omap.h  |  6 --
 scripts/config_whitelist.txt   |  1 -
 15 files changed, 50 insertions(+), 29 deletions(-)
>>
>> Marek?  Thanks!
> 
> Tom?  Thanks!

IIRC, it has been decided to take this patch series through the OMAP tree.
Contradictory to what I remember, I can see the patches have been delegated to 
Marek
in patchwork.
So, what's happening? Can we please go on with merging this series?

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

Re: [U-Boot] [PATCH] fdt: allow address translation in case of SPL_OF_TRANSLATE

[Igor Grinberg]

Hi,

Please provide a commit message.

On 03/25/17 01:19, Vikas Manocha wrote:

Signed-off-by: Vikas Manocha 
---
 lib/fdtdec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index 81f47ef..a1c4d16 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -112,7 +112,7 @@ fdt_addr_t fdtdec_get_addr_size_fixed(const void *blob, int 
node,
return FDT_ADDR_T_NONE;
}

-#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_OF_LIBFDT)
+#if defined(CONFIG_SPL_OF_TRANSLATE) || defined(CONFIG_OF_LIBFDT)
if (translate)
addr = fdt_translate_address(blob, node, prop_addr);
else



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

Re: [U-Boot] [PATCH 00/16] RFC: Board init using driver model

[Igor Grinberg]

Hi Simon,

On 03/19/17 20:59, Simon Glass wrote:
> At present we have a lot of ad-hoc init functions related to boards, for
> example board_early_init_f(), board_misc_init_f() and dram_init().
> 
> There are used in different ways by different boards as useful hooks to
> do the required init and sequence it correctly. Some functions are always
> enabled but have a __weak default. Some are controlled by the existence
> of a CONFIG.
> 
> There are two main init sequences: board_init_f() (f for running from
> read-only flash) which runs before relocation and board_init_r() (r for
> relocated) which runs afterwards.
> 
> One problem with the current sequence is that it has a lot of
> arch-specific #ifdefs around various functions. There are also #ifdefs
> for various features. There has been quite a bit of discussion about how
> to tidy this up and at least one RFC series[1].
> 
> Now that we have driver model we can use this to deal with the init
> sequences. This approach has several advantages:
> 
> - We have a path to remove the #ifdefs
> - It is easy for multiple parts of the code to implement the same hook
> - We can track what is called and what is not
> - We don't need weak functions
> - We can eventually adjust the sequence to improve naming or to add new
> init phases
> - It provides a model for how we might deal with ft_board_setup() and
> friends

I haven't got this one yet from the patchset.
May be I need to look closer...

> 
> This series starts the process of replacing the pre-relocation init
> sequence with a driver-model solution. It defines a uclass, adds tests
> and converts sandbox and a few x86 boards over to use this new setup.
> 
> This series is not ready for use yet as the rest of the init sequence
> hooks need to be converted. But there is enough here to show the idea.
> 
> Comments welcome.

Overall, it sounds like a great idea!

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

Re: [U-Boot] [PATCH 04/16] dm: board: Add a uclass for init functions

[Igor Grinberg]

On 03/19/17 20:59, Simon Glass wrote:
> Add a uclass to handle board init. This allows drivers to be provided to
> perform the various phases of init. Functions are provided to call all
> devices that can handle a particular phase.
> 
> Signed-off-by: Simon Glass 
> ---
> 
>  common/Kconfig|  31 +++
>  common/init/Makefile  |   1 +
>  common/init/board-uclass.c| 108 
>  include/asm-generic/global_data.h |   5 ++
>  include/board.h   | 170 
> ++
>  include/dm/uclass-id.h|   1 +
>  6 files changed, 316 insertions(+)
>  create mode 100644 common/init/board-uclass.c
>  create mode 100644 include/board.h

[...]

> diff --git a/include/board.h b/include/board.h
> new file mode 100644
> index 00..0975f5ac12
> --- /dev/null
> +++ b/include/board.h

[...]

> +/* Operations for the board driver */
> +struct board_ops {
> + /**
> + * phase() - Execute a phase of board init
> + *
> +  * @dev:Device to use
> + * @phase:   Phase to execute
> + * @return 0 if done, -ENOSYS if not supported (which is often fine),
> + BOARD_PHASE_CLAIMED if this was handled and that processing
> + of this phase should stop (i.e. do not send it to other
> + devices), other error if something went wrong
> + */
> + int (*phase)(struct udevice *dev, enum board_phase_t phase);

That looks a bit tiny interface.
This will force all the boards to define switch to figure out what the
current phase is... This might cause a problem (probably #ifdefs) in the board
code as some code will be available in SPL and some not.

I would prefer a wider interface instead of a single entry point to any
kind of flexibility to the board driver.

> +
> + /**
> +  * get_desc() - Get a description string for a board
> +  *
> +  * @dev:Device to check (UCLASS_BOARD)
> +  * @buf:Buffer to place string
> +  * @size:   Size of string space
> +  * @return 0 if OK, -ENOSPC if buffer is too small, other -ve on error
> +  */
> + int (*get_desc)(struct udevice *dev, char *buf, int size);
> +};
> +
> +#define board_get_ops(dev)((struct board_ops *)(dev)->driver->ops)
> +

[...]


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

Re: [U-Boot] [PATCH v1] env_mmc: configure environment offsets via device tree

[Igor Grinberg]

Hi Simon,

On 03/12/17 22:21, Simon Glass wrote:
> Hi Igor,
> 
> On 5 March 2017 at 01:39, Igor Grinberg <grinb...@compulab.co.il> wrote:
>> Hi Simon,
>>
>> On 03/03/17 06:53, Simon Glass wrote:
>>> Hi Igor,
>>>
>>> On 22 February 2017 at 00:35, Igor Grinberg <grinb...@compulab.co.il> wrote:
>>>> Hi Philipp, Simon,
>>>>
>>>> On 02/22/17 05:59, Simon Glass wrote:
>>>>> Hi,
>>>>>
>>>>> On 20 February 2017 at 02:18, Dr. Philipp Tomsich
>>>>> <philipp.toms...@theobroma-systems.com> wrote:
>>>>>>
>>>>>> On 20 Feb 2017, at 08:22, Igor Grinberg <grinb...@compulab.co.il> wrote:
>>>>>>
>>>>>> That sounds too odd...
>>>>>> DT's purpose is to describe the h/w... and that does not look so...
>>>>>> We also, have a dt file name in the environment, so this creates will 
>>>>>> create
>>>>>> a chicken and an egg problem…
>>>>>>
>>>>>>
>>>>>> I don’t really follow… as far as I knew the DT name would have to come
>>>>>> from some other source anyway, as the device containing the env might
>>>>>> only be described through the device tree (e.g. mmc0).
>>>>>>
>>>>>>
>>>>>> Why? U-Boot can live pretty well w/o DT.
>>>>>>
>>>>>>
>>>>>> If U-Boot runs without DT, then nothing will/should change about how the
>>>>>> setting
>>>>>> is retrieved from CONFIG_ENV_OFFSET.
>>>>
>>>> ok.
>>>>
>>>>>>
>>>>>> The platform that motivated this change is ARCH_SUNXI, which does not use
>>>>>> per-board defines but aims to have one generic bootloader per-SoC.
>>>>
>>>> Well, after a ten year experience with boars and different SoC vendors,
>>>> I don't think it is possible...
>>>> Unless all boards are copies of their respective reference design...
>>>>
>>>>>>
>>>>>> I really don't think we should go that direction. DT is not meant to 
>>>>>> provide
>>>>>> a solution to all your problems...
>>>>>>
>>>>>>
>>>>>> I don’t see how this is different from other entries in chosen and 
>>>>>> config as
>>>>>> of today:
>>>>>> common/autoboot.c allows an override through /config/bootdelay
>>>>>> common/board_r.c uses /config/load-environment
>>>>>> common/cli.c can pull in /config/bootcmd
>>>>>> drivers/serial/serial-uclass.c uses /chosen/stdout-path
>>>>>>
>>>>>> In fact, it is the absence of this mechanism that is causing problems 
>>>>>> today:
>>>>>> CONFIG_ENV_OFFSET is not configurable through Kconfig, so we would
>>>>>> need board-specific defines (e.g. CONFIG_SUNXI_BOARD_LYNX) and
>>>>>> matching #ifdef primitives in a shared header (sunxi-common.h in our 
>>>>>> case).
>>>>>>
>>>>>>
>>>>>> Right. Exactly, I think we should move the CONFIG_ENV_OFFSET to Kconfig.
>>>>>> And that will solve the problem.
>>>>>>
>>>>>>
>>>>>> Doing this would still get into the way of architectures that want to 
>>>>>> build
>>>>>> a single
>>>>>> ‘universal’ bootloader for their SoC: the ENV_OFFSET may not be the same
>>>>>> across all board and vendor configurations. This can easily be handled 
>>>>>> with
>>>>>> the
>>>>>> (optional) prop in the DT, but not with the compile-time ENV_OFFSET.
>>>>
>>>> I think Kconfig would be enough for this, but please try your approach.
>>>> The 'universal' thing will probably work if you don't have too many boards 
>>>> and
>>>> they don't differ too much from each other...
>>>>
>>>>>>
>>>>>> If we decide to this, I’d at least like to introduce the function call to
>>>>>> (the weak
>>>>>> function) mmc_get_env_addr(…), so we can override this in the board code.
>>>>
>>>> That is how it works today:
>>>> include/environment.h:185:extern int mmc_get_en

Re: [U-Boot] [PATCH v1] env_mmc: configure environment offsets via device tree

[Igor Grinberg]

Hi Simon,

On 03/03/17 06:53, Simon Glass wrote:
> Hi Igor,
> 
> On 22 February 2017 at 00:35, Igor Grinberg <grinb...@compulab.co.il> wrote:
>> Hi Philipp, Simon,
>>
>> On 02/22/17 05:59, Simon Glass wrote:
>>> Hi,
>>>
>>> On 20 February 2017 at 02:18, Dr. Philipp Tomsich
>>> <philipp.toms...@theobroma-systems.com> wrote:
>>>>
>>>> On 20 Feb 2017, at 08:22, Igor Grinberg <grinb...@compulab.co.il> wrote:
>>>>
>>>> That sounds too odd...
>>>> DT's purpose is to describe the h/w... and that does not look so...
>>>> We also, have a dt file name in the environment, so this creates will 
>>>> create
>>>> a chicken and an egg problem…
>>>>
>>>>
>>>> I don’t really follow… as far as I knew the DT name would have to come
>>>> from some other source anyway, as the device containing the env might
>>>> only be described through the device tree (e.g. mmc0).
>>>>
>>>>
>>>> Why? U-Boot can live pretty well w/o DT.
>>>>
>>>>
>>>> If U-Boot runs without DT, then nothing will/should change about how the
>>>> setting
>>>> is retrieved from CONFIG_ENV_OFFSET.
>>
>> ok.
>>
>>>>
>>>> The platform that motivated this change is ARCH_SUNXI, which does not use
>>>> per-board defines but aims to have one generic bootloader per-SoC.
>>
>> Well, after a ten year experience with boars and different SoC vendors,
>> I don't think it is possible...
>> Unless all boards are copies of their respective reference design...
>>
>>>>
>>>> I really don't think we should go that direction. DT is not meant to 
>>>> provide
>>>> a solution to all your problems...
>>>>
>>>>
>>>> I don’t see how this is different from other entries in chosen and config 
>>>> as
>>>> of today:
>>>> common/autoboot.c allows an override through /config/bootdelay
>>>> common/board_r.c uses /config/load-environment
>>>> common/cli.c can pull in /config/bootcmd
>>>> drivers/serial/serial-uclass.c uses /chosen/stdout-path
>>>>
>>>> In fact, it is the absence of this mechanism that is causing problems 
>>>> today:
>>>> CONFIG_ENV_OFFSET is not configurable through Kconfig, so we would
>>>> need board-specific defines (e.g. CONFIG_SUNXI_BOARD_LYNX) and
>>>> matching #ifdef primitives in a shared header (sunxi-common.h in our case).
>>>>
>>>>
>>>> Right. Exactly, I think we should move the CONFIG_ENV_OFFSET to Kconfig.
>>>> And that will solve the problem.
>>>>
>>>>
>>>> Doing this would still get into the way of architectures that want to build
>>>> a single
>>>> ‘universal’ bootloader for their SoC: the ENV_OFFSET may not be the same
>>>> across all board and vendor configurations. This can easily be handled with
>>>> the
>>>> (optional) prop in the DT, but not with the compile-time ENV_OFFSET.
>>
>> I think Kconfig would be enough for this, but please try your approach.
>> The 'universal' thing will probably work if you don't have too many boards 
>> and
>> they don't differ too much from each other...
>>
>>>>
>>>> If we decide to this, I’d at least like to introduce the function call to
>>>> (the weak
>>>> function) mmc_get_env_addr(…), so we can override this in the board code.
>>
>> That is how it works today:
>> include/environment.h:185:extern int mmc_get_env_addr(struct mmc *mmc, int 
>> copy, u32 *env_addr);
>>
>>>>
>>>> So putting this in the DT is the best (and least intrusive) option
>>>> available.
>>>>
>>>>
>>>> Ok. I see your point now. Yet I think we should keep the DT to its purpose 
>>>> -
>>>> which
>>>> is to describe the h/w (and not the s/w placement layout).
>>>
>>> Well actually we put things like flash map in there and the
>>> environment position seems like a very good thing to have there.
>>
>> Well, I thought so too... But I had a discussion with kernel people
>> some time ago and they discourage this approach and would not like to
>> have the flash mapping in the DT.
> 
> I'm sorry to hear that, but it doesn't change the fact that it is very
> useful for dealing with hardware-specific diff

Re: [U-Boot] [PATCH] splash: Prevent splash_load_fs from writing to 0x0

[Igor Grinberg]

Hi Jonathan,

On 02/27/17 10:10, Jonathan Golder wrote:
> Hi Igor,
> 
>> I haven't looked at fs_read() yet, but from the above it seems that
>> a better approach would be to fix the fs_read()? Might there be use
>> cases when it is legitimate to pass NULL?
> 
> 
> Well, actually I have not dived into the depths of the fs_read ()
> deeper than realising that something went wrong.
> 
> Before posting this patch I greped for usages of fs_read() with NULL
> as last parameter and found only this occurrence. So I supposed that
> passing NULL for actread is not an intended use case of fs_read ().

Probably, yet this use case appeared and correct me if I'm wrong,
the splash source code does not really care of that parameter, and
it wants to use the fs_read().
So maybe the problem is in fs_read() API not covering a legitimate
use case?
Of course this patch will fix the problem by hiding it till next time.

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

Re: [U-Boot] [PATCH v4 3/7] usb: host: xhci-omap: fix double weak board_usb_init functions

[Igor Grinberg]

Cc Marex...

On 02/23/17 15:39, Uri Mashiach wrote:
> A weak version of the function board_usb_init is implemented in:
> common/usb.c
> drivers/usb/host/xhci-omap.c
> 
> To fix the double implementations:
> * Convert the board_usb_init function in drivers/usb/host/xhci-omap.c
>   normal (not weak).
> * The function board_usb_init in drivers/usb/host/xhci-omap.c calls to
>   the weak function omap_xhci_board_usb_init.
> * Rename board version of the function board_usb_init to
>   omap_xhci_board_usb_init.
>   Done only for boards that defines CONFIG_USB_XHCI_OMAP.
> 
> To achieve the same flexibility with the function board_usb_cleanup:
> * Add a normal (not weak) implementation of the function
>   board_usb_cleanup in drivers/usb/host/xhci-omap.c
> * The function board_usb_cleanup in drivers/usb/host/xhci-omap.c calls
>   to the weak function omap_xhci_board_usb_cleanup.
> * Rename board version of the function board_usb_cleanup to
>   omap_xhci_board_usb_cleanup.
>   Done only for boards that defines CONFIG_USB_XHCI_OMAP.
> 
> Cc: Lokesh Vutla 
> Signed-off-by: Uri Mashiach 
> Acked-by: Marek Vasut 
> Reviewed-by: Tom Rini 
> ---
> V1 -> V2: Use __weak instead of attribute block
> V2 -> V4: none
> 
>  board/compulab/cl-som-am57x/cl-som-am57x.c |  2 +-
>  board/ti/am43xx/board.c|  4 ++--
>  board/ti/am57xx/board.c|  4 ++--
>  board/ti/dra7xx/evm.c  |  4 ++--
>  drivers/usb/host/xhci-omap.c   | 17 +++--
>  5 files changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/board/compulab/cl-som-am57x/cl-som-am57x.c 
> b/board/compulab/cl-som-am57x/cl-som-am57x.c
> index bdd0a2b..fe1468f 100644
> --- a/board/compulab/cl-som-am57x/cl-som-am57x.c
> +++ b/board/compulab/cl-som-am57x/cl-som-am57x.c
> @@ -54,7 +54,7 @@ int board_mmc_init(bd_t *bis)
>  #endif /* CONFIG_GENERIC_MMC */
>  
>  #ifdef CONFIG_USB_XHCI_OMAP
> -int board_usb_init(int index, enum usb_init_type init)
> +int omap_xhci_board_usb_init(int index, enum usb_init_type init)
>  {
>   setbits_le32((*prcm)->cm_l3init_usb_otg_ss1_clkctrl,
>OTG_SS_CLKCTRL_MODULEMODE_HW | OPTFCLKEN_REFCLK960M);
> diff --git a/board/ti/am43xx/board.c b/board/ti/am43xx/board.c
> index 390cc16..2572029 100644
> --- a/board/ti/am43xx/board.c
> +++ b/board/ti/am43xx/board.c
> @@ -694,7 +694,7 @@ int usb_gadget_handle_interrupts(int index)
>  #endif /* CONFIG_USB_DWC3 */
>  
>  #if defined(CONFIG_USB_DWC3) || defined(CONFIG_USB_XHCI_OMAP)
> -int board_usb_init(int index, enum usb_init_type init)
> +int omap_xhci_board_usb_init(int index, enum usb_init_type init)
>  {
>   enable_usb_clocks(index);
>  #ifdef CONFIG_USB_DWC3
> @@ -725,7 +725,7 @@ int board_usb_init(int index, enum usb_init_type init)
>   return 0;
>  }
>  
> -int board_usb_cleanup(int index, enum usb_init_type init)
> +int omap_xhci_board_usb_cleanup(int index, enum usb_init_type init)
>  {
>  #ifdef CONFIG_USB_DWC3
>   switch (index) {
> diff --git a/board/ti/am57xx/board.c b/board/ti/am57xx/board.c
> index 1611514..4afa914 100644
> --- a/board/ti/am57xx/board.c
> +++ b/board/ti/am57xx/board.c
> @@ -618,7 +618,7 @@ int usb_gadget_handle_interrupts(int index)
>  #endif /* CONFIG_USB_DWC3 */
>  
>  #if defined(CONFIG_USB_DWC3) || defined(CONFIG_USB_XHCI_OMAP)
> -int board_usb_init(int index, enum usb_init_type init)
> +int omap_xhci_board_usb_init(int index, enum usb_init_type init)
>  {
>   enable_usb_clocks(index);
>   switch (index) {
> @@ -652,7 +652,7 @@ int board_usb_init(int index, enum usb_init_type init)
>   return 0;
>  }
>  
> -int board_usb_cleanup(int index, enum usb_init_type init)
> +int omap_xhci_board_usb_cleanup(int index, enum usb_init_type init)
>  {
>  #ifdef CONFIG_USB_DWC3
>   switch (index) {
> diff --git a/board/ti/dra7xx/evm.c b/board/ti/dra7xx/evm.c
> index bd1c809..65bce93 100644
> --- a/board/ti/dra7xx/evm.c
> +++ b/board/ti/dra7xx/evm.c
> @@ -727,7 +727,7 @@ static struct ti_usb_phy_device usb_phy2_device = {
>   .index = 1,
>  };
>  
> -int board_usb_init(int index, enum usb_init_type init)
> +int omap_xhci_board_usb_init(int index, enum usb_init_type init)
>  {
>   enable_usb_clocks(index);
>   switch (index) {
> @@ -764,7 +764,7 @@ int board_usb_init(int index, enum usb_init_type init)
>   return 0;
>  }
>  
> -int board_usb_cleanup(int index, enum usb_init_type init)
> +int omap_xhci_board_usb_cleanup(int index, enum usb_init_type init)
>  {
>   switch (index) {
>   case 0:
> diff --git a/drivers/usb/host/xhci-omap.c b/drivers/usb/host/xhci-omap.c
> index b881b19..a1b4f2f 100644
> --- a/drivers/usb/host/xhci-omap.c
> +++ b/drivers/usb/host/xhci-omap.c
> @@ -27,12 +27,25 @@ DECLARE_GLOBAL_DATA_PTR;
>  
>  static struct omap_xhci omap;
>  
> -__weak int __board_usb_init(int index, enum usb_init_type init)
> +__weak int 

Re: [U-Boot] [PATCH] splash: Prevent splash_load_fs from writing to 0x0

[Igor Grinberg]

Hi Jonathan,

On 02/24/17 18:46, Jonathan Golder wrote:
> Passing NULL to fs_read() for actread value results in hanging U-Boot
> at least on our ARM plattform (TI AM335x). Since fs_read() and
> following functions do not catch nullpointers, writing to 0x0 occurs.
> 
> Passing a local dummy var instead of NULL solves this issue.

I haven't looked at fs_read() yet, but from the above it seems
that a better approach would be to fix the fs_read()?
Might there be use cases when it is legitimate to pass NULL?

> 
> Signed-off-by: Jonathan Golder 
> Cc: Anatolij Gustschin 
> ---
>  common/splash_source.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/common/splash_source.c b/common/splash_source.c
> index a5eeb3f..d1647c8 100644
> --- a/common/splash_source.c
> +++ b/common/splash_source.c
> @@ -216,6 +216,7 @@ static int splash_load_fs(struct splash_location 
> *location, u32 bmp_load_addr)
>  {
>   int res = 0;
>   loff_t bmp_size;
> + loff_t actread;
>   char *splash_file;
>  
>   splash_file = getenv("splashfile");
> @@ -251,7 +252,7 @@ static int splash_load_fs(struct splash_location 
> *location, u32 bmp_load_addr)
>   }
>  
>   splash_select_fs_dev(location);
> - res = fs_read(splash_file, bmp_load_addr, 0, 0, NULL);
> + res = fs_read(splash_file, bmp_load_addr, 0, 0, );
>  
>  out:
>   if (location->ubivol != NULL)
> 

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

Re: [U-Boot] [PATCH v1] env_mmc: configure environment offsets via device tree

[Igor Grinberg]

Hi Philipp, Simon,

On 02/22/17 05:59, Simon Glass wrote:
> Hi,
> 
> On 20 February 2017 at 02:18, Dr. Philipp Tomsich
> <philipp.toms...@theobroma-systems.com> wrote:
>>
>> On 20 Feb 2017, at 08:22, Igor Grinberg <grinb...@compulab.co.il> wrote:
>>
>> That sounds too odd...
>> DT's purpose is to describe the h/w... and that does not look so...
>> We also, have a dt file name in the environment, so this creates will create
>> a chicken and an egg problem…
>>
>>
>> I don’t really follow… as far as I knew the DT name would have to come
>> from some other source anyway, as the device containing the env might
>> only be described through the device tree (e.g. mmc0).
>>
>>
>> Why? U-Boot can live pretty well w/o DT.
>>
>>
>> If U-Boot runs without DT, then nothing will/should change about how the
>> setting
>> is retrieved from CONFIG_ENV_OFFSET.

ok.

>>
>> The platform that motivated this change is ARCH_SUNXI, which does not use
>> per-board defines but aims to have one generic bootloader per-SoC.

Well, after a ten year experience with boars and different SoC vendors,
I don't think it is possible...
Unless all boards are copies of their respective reference design...

>>
>> I really don't think we should go that direction. DT is not meant to provide
>> a solution to all your problems...
>>
>>
>> I don’t see how this is different from other entries in chosen and config as
>> of today:
>> common/autoboot.c allows an override through /config/bootdelay
>> common/board_r.c uses /config/load-environment
>> common/cli.c can pull in /config/bootcmd
>> drivers/serial/serial-uclass.c uses /chosen/stdout-path
>>
>> In fact, it is the absence of this mechanism that is causing problems today:
>> CONFIG_ENV_OFFSET is not configurable through Kconfig, so we would
>> need board-specific defines (e.g. CONFIG_SUNXI_BOARD_LYNX) and
>> matching #ifdef primitives in a shared header (sunxi-common.h in our case).
>>
>>
>> Right. Exactly, I think we should move the CONFIG_ENV_OFFSET to Kconfig.
>> And that will solve the problem.
>>
>>
>> Doing this would still get into the way of architectures that want to build
>> a single
>> ‘universal’ bootloader for their SoC: the ENV_OFFSET may not be the same
>> across all board and vendor configurations. This can easily be handled with
>> the
>> (optional) prop in the DT, but not with the compile-time ENV_OFFSET.

I think Kconfig would be enough for this, but please try your approach.
The 'universal' thing will probably work if you don't have too many boards and
they don't differ too much from each other...

>>
>> If we decide to this, I’d at least like to introduce the function call to
>> (the weak
>> function) mmc_get_env_addr(…), so we can override this in the board code.

That is how it works today:
include/environment.h:185:extern int mmc_get_env_addr(struct mmc *mmc, int 
copy, u32 *env_addr);

>>
>> So putting this in the DT is the best (and least intrusive) option
>> available.
>>
>>
>> Ok. I see your point now. Yet I think we should keep the DT to its purpose -
>> which
>> is to describe the h/w (and not the s/w placement layout).
> 
> Well actually we put things like flash map in there and the
> environment position seems like a very good thing to have there.

Well, I thought so too... But I had a discussion with kernel people
some time ago and they discourage this approach and would not like to
have the flash mapping in the DT.

> So I
> like this patch. There is too compile-time configuration in U-Boot
> still...

Yeah, I know you like it ;-) but DT is not the place for it,
unless DT specs. guys decide to change its purpose and place
s/w configuration straps inside...

Although, U-Boot build process is not a pain at all, you might want
to design another abstraction layer for s/w configuration straps.
That way you will be able to keep the U-Boot core binary generic...
Is it worse the effort? I don't know. Currently, having the board
infrastructure, provides that layer and works fine.

> 
> In fact I wish we could support this for all environment types.
> Overall the environment drivers need some work to make them more
> similar...

Indeed, but not just that... It would be great to add a DM to env. drivers.
This will make it possible to use more than one environment driver
in runtime and make many people who struggle with it happy...


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

Re: [U-Boot] [PATCH v3 2/7] arm: usb: dra7xx: xHCI registers based on USB port index

[Igor Grinberg]

Hey Marek,

We haven't spoken for great while... I'm glad to hear from you!

On 02/19/17 19:39, Marek Vasut wrote:
> On 02/19/2017 05:26 PM, Igor Grinberg wrote:
>> Hi guys,
>>
>> On 02/19/17 17:15, Tom Rini wrote:
>>> On Sun, Feb 19, 2017 at 04:13:02PM +0100, Marek Vasut wrote:
>>>> On 02/19/2017 03:55 PM, Uri Mashiach wrote:
>>>>>
>>>>>
>>>>> On 02/19/2017 04:27 PM, Marek Vasut wrote:
>>>>>> On 02/19/2017 02:27 PM, Uri Mashiach wrote:
>>>>>>> Modify the determination of the base address of xHCI registers of DRA7XX
>>>>>>> targets.
>>>>>>> Before the commit: by the target.
>>>>>>> After the commit: by the USB port index.
>>>>>>>
>>>>>>> Cc: Lokesh Vutla <lokeshvu...@ti.com>
>>>>>>> Cc: Marek Vasut <ma...@denx.de>
>>>>>>> Signed-off-by: Uri Mashiach <uri.mashi...@compulab.co.il>
>>>>>>> ---
>>>>>>> V1 -> V2: Replace the commit "fix XHCI registers base address".
>>>>>>> V2 -> V3: Replace the commit "reintroduce the CONFIG_AM57XX symbol"
>>>>>>>
>>>>>>>  configs/dra7xx_evm_defconfig|  1 +
>>>>>>>  configs/dra7xx_hs_evm_defconfig |  1 +
>>>>>>>  drivers/usb/host/Kconfig| 16 
>>>>>>>  include/linux/usb/xhci-omap.h   |  6 --
>>>>>>>  4 files changed, 22 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/configs/dra7xx_evm_defconfig b/configs/dra7xx_evm_defconfig
>>>>>>> index 26b26cc..1f47f92 100644
>>>>>
>>>>> [...]
>>>>>
>>>>>>> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
>>>>>>> index 5129a57..440fbcf 100644
>>>>>>> --- a/drivers/usb/host/Kconfig
>>>>>>> +++ b/drivers/usb/host/Kconfig
>>>>>>> @@ -43,6 +43,22 @@ config USB_XHCI_ZYNQMP
>>>>>>>  help
>>>>>>>Enables support for the on-chip xHCI controller on Xilinx
>>>>>>> ZynqMP SoCs.
>>>>>>>
>>>>>>> +choice
>>>>>>> +prompt "DRA7XX xHCI USB index select"
>>>>>>> +depends on DRA7XX
>>>>>>> +
>>>>>>> +config USB_XHCI_DRA7XX_INDEX0
>>>>>>> +bool "USB0"
>>>>>>> +help
>>>>>>> +  DRA7XX xHCI USB0.
>>>>>>> +
>>>>>>> +config USB_XHCI_DRA7XX_INDEX1
>>>>>>> +bool "USB1"
>>>>>>> +help
>>>>>>> +  DRA7XX xHCI USB1.
>>>>>>
>>>>>> What is this all about ? Shouldn't this come from DT ? And what if I
>>>>>> want to use both XHCI ? This looks totally bogus ...
>>
>> Right, both XHCIs cannot be used with current driver and we do not have
>> the time to fix it by our own... may be TI has?
>> Remember, you've accepted the driver and following patches, right?
> 
> You mean this patch [1] which actually adds the board dependency ?
> Actually , no , I DID NOT. It's been going on for a while that my
> role as a USB custodian was actively circumvented and patches which
> should've gone through the USB tree just did not.

Ahh.. Sorry, thought you are collecting all the USB patches or at least
provide your Ack so patches can go through other trees..

> 
> So please, next time you start blaming someone, get your facts right.

Yeah, I haven't looked at it thoroughly... That's why the "right?" in the
end of my sentence. Sorry once again, I did not mean to offend you.

> 
> I already have almost zero motivation to maintain USB in my free time,
> being paid nothing for it, not ever hearing a single "thank you" and
> just spending time I could use otherwise. Being circumvented and only
> catching shit for problematic patches I did NOT even apply though, that
> is completely off-putting .

Ah... First of all thank you Marek! You've done a great job AFAIR.
I really mean it.
And it is a pity, things have gone this way.
May be this can still be fixed? Tom?

> 
> [1] https://patchwork.ozlabs.org/patch/700372/
> 
>> Regarding DT, do we have a DT as a requirement to run USB in U-Boot?
> 
> It is highly recommended.

Hmmm... I don't really like this appro

Re: [U-Boot] [PATCH v1] env_mmc: configure environment offsets via device tree

[Igor Grinberg]

Hi Philipp,

On 02/19/17 19:59, Dr. Philipp Tomsich wrote:
> Igor,
> 
>> On 19 Feb 2017, at 12:52, Igor Grinberg <grinb...@compulab.co.il> wrote:
>>
>> On 02/17/17 19:28, Philipp Tomsich wrote:
>>> This introduces the ability to override the environment offets from the
>>> device tree by setting the following nodes in '/config':
>>> 'u-boot,mmc-env-offset' - overrides CONFIG_ENV_OFFSET
>>> 'u-boot,mmc-env-offset-redundant'
>>> - overrides CONFIG_ENV_OFFSET_REDUND
>>>
>>> To keep with the previous logic, the CONFIG_* defines still need to
>>> be available and the statically defined values become the defaults,
>>> when the corresponding properties are not set in the device-tree.
>>
>> That sounds too odd...
>> DT's purpose is to describe the h/w... and that does not look so...
>> We also, have a dt file name in the environment, so this creates will create
>> a chicken and an egg problem…
> 
> I don’t really follow… as far as I knew the DT name would have to come
> from some other source anyway, as the device containing the env might
> only be described through the device tree (e.g. mmc0).

Why? U-Boot can live pretty well w/o DT.

> 
> U-Boot usually locates the FDT to use at a preconfigured address (or one
> configured in the early environment).

Or just by providing the address to bootz command...

> This will not be the environment we’d
> load from a MMC device configured via DM_MMC.
> 
> So there shouldn’t be a chicken & egg here.
> 
>> I really don't think we should go that direction. DT is not meant to provide
>> a solution to all your problems...
> 
> I don’t see how this is different from other entries in chosen and config as
> of today:
>   common/autoboot.c allows an override through /config/bootdelay
>   common/board_r.c uses /config/load-environment
>   common/cli.c can pull in /config/bootcmd
>   drivers/serial/serial-uclass.c uses /chosen/stdout-path
> 
> In fact, it is the absence of this mechanism that is causing problems today:
> CONFIG_ENV_OFFSET is not configurable through Kconfig, so we would
> need board-specific defines (e.g. CONFIG_SUNXI_BOARD_LYNX) and
> matching #ifdef primitives in a shared header (sunxi-common.h in our case).

Right. Exactly, I think we should move the CONFIG_ENV_OFFSET to Kconfig.
And that will solve the problem.

> 
> So putting this in the DT is the best (and least intrusive) option available.

Ok. I see your point now. Yet I think we should keep the DT to its purpose - 
which
is to describe the h/w (and not the s/w placement layout).


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

Re: [U-Boot] [PATCH v3 2/7] arm: usb: dra7xx: xHCI registers based on USB port index

[Igor Grinberg]

Hi guys,

On 02/19/17 17:15, Tom Rini wrote:
> On Sun, Feb 19, 2017 at 04:13:02PM +0100, Marek Vasut wrote:
>> On 02/19/2017 03:55 PM, Uri Mashiach wrote:
>>>
>>>
>>> On 02/19/2017 04:27 PM, Marek Vasut wrote:
 On 02/19/2017 02:27 PM, Uri Mashiach wrote:
> Modify the determination of the base address of xHCI registers of DRA7XX
> targets.
> Before the commit: by the target.
> After the commit: by the USB port index.
>
> Cc: Lokesh Vutla 
> Cc: Marek Vasut 
> Signed-off-by: Uri Mashiach 
> ---
> V1 -> V2: Replace the commit "fix XHCI registers base address".
> V2 -> V3: Replace the commit "reintroduce the CONFIG_AM57XX symbol"
>
>  configs/dra7xx_evm_defconfig|  1 +
>  configs/dra7xx_hs_evm_defconfig |  1 +
>  drivers/usb/host/Kconfig| 16 
>  include/linux/usb/xhci-omap.h   |  6 --
>  4 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/configs/dra7xx_evm_defconfig b/configs/dra7xx_evm_defconfig
> index 26b26cc..1f47f92 100644
>>>
>>> [...]
>>>
> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> index 5129a57..440fbcf 100644
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -43,6 +43,22 @@ config USB_XHCI_ZYNQMP
>  help
>Enables support for the on-chip xHCI controller on Xilinx
> ZynqMP SoCs.
>
> +choice
> +prompt "DRA7XX xHCI USB index select"
> +depends on DRA7XX
> +
> +config USB_XHCI_DRA7XX_INDEX0
> +bool "USB0"
> +help
> +  DRA7XX xHCI USB0.
> +
> +config USB_XHCI_DRA7XX_INDEX1
> +bool "USB1"
> +help
> +  DRA7XX xHCI USB1.

 What is this all about ? Shouldn't this come from DT ? And what if I
 want to use both XHCI ? This looks totally bogus ...

Right, both XHCIs cannot be used with current driver and we do not have
the time to fix it by our own... may be TI has?
Remember, you've accepted the driver and following patches, right?

Regarding DT, do we have a DT as a requirement to run USB in U-Boot?
I don't remember this happening and I think it shouldn't be a requirement.


>>>
>>> The support for both XHCI is currently missing.
>>> This could be a temporary solution until the DT solution.
>>> The current situation is worse - selecting USB0 or USB1 based on the
>>> target.
>>
>> So we're replacing it with equally bad solution ?

I don't think equally applies here...
This IS an improvement. Of course it is not like you would want it to be,
but still it is from a platform POV.

>> Hmmm , no.
>> The MW will open mid-march, there's about a month to fix this,
>> so please do.
> 
> Do note that the relevant driver here is not yet using DM_USB.

Yes, the driver should be fixed some day. We would really like to take
this task, but unfortunately, we cannot, at least not right now.
But we do need that USB working on our board and not only on TI EVMs...

Tom,
Should we fall back to v1 and have a worse solution for the base addresses?

-- 
Regards,
Igor.



signature.asc
Description: OpenPGP digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v3 2/7] arm: usb: dra7xx: xHCI registers based on USB port index

[Igor Grinberg]

On 02/19/17 17:52, Marek Vasut wrote:
> On 02/19/2017 04:15 PM, Tom Rini wrote:
>> On Sun, Feb 19, 2017 at 04:13:02PM +0100, Marek Vasut wrote:
>>> On 02/19/2017 03:55 PM, Uri Mashiach wrote:


 On 02/19/2017 04:27 PM, Marek Vasut wrote:
> On 02/19/2017 02:27 PM, Uri Mashiach wrote:
>> Modify the determination of the base address of xHCI registers of DRA7XX
>> targets.
>> Before the commit: by the target.
>> After the commit: by the USB port index.
>>
>> Cc: Lokesh Vutla 
>> Cc: Marek Vasut 
>> Signed-off-by: Uri Mashiach 
>> ---
>> V1 -> V2: Replace the commit "fix XHCI registers base address".
>> V2 -> V3: Replace the commit "reintroduce the CONFIG_AM57XX symbol"
>>
>>  configs/dra7xx_evm_defconfig|  1 +
>>  configs/dra7xx_hs_evm_defconfig |  1 +
>>  drivers/usb/host/Kconfig| 16 
>>  include/linux/usb/xhci-omap.h   |  6 --
>>  4 files changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/configs/dra7xx_evm_defconfig b/configs/dra7xx_evm_defconfig
>> index 26b26cc..1f47f92 100644

 [...]

>> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
>> index 5129a57..440fbcf 100644
>> --- a/drivers/usb/host/Kconfig
>> +++ b/drivers/usb/host/Kconfig
>> @@ -43,6 +43,22 @@ config USB_XHCI_ZYNQMP
>>  help
>>Enables support for the on-chip xHCI controller on Xilinx
>> ZynqMP SoCs.
>>
>> +choice
>> +prompt "DRA7XX xHCI USB index select"
>> +depends on DRA7XX
>> +
>> +config USB_XHCI_DRA7XX_INDEX0
>> +bool "USB0"
>> +help
>> +  DRA7XX xHCI USB0.
>> +
>> +config USB_XHCI_DRA7XX_INDEX1
>> +bool "USB1"
>> +help
>> +  DRA7XX xHCI USB1.
>
> What is this all about ? Shouldn't this come from DT ? And what if I
> want to use both XHCI ? This looks totally bogus ...
>

 The support for both XHCI is currently missing.
 This could be a temporary solution until the DT solution.
 The current situation is worse - selecting USB0 or USB1 based on the
 target.
>>>
>>> So we're replacing it with equally bad solution ? Hmmm , no.
>>> The MW will open mid-march, there's about a month to fix this,
>>> so please do.
>>
>> Do note that the relevant driver here is not yet using DM_USB.
>>
> We had multi-controller support before DM existed, no problem.
> But then, xhci does support DM, so fixing this should also not
> be a problem.

Right! We're talking on fixing the XHCI driver...
We can't afford it right now...

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

Re: [U-Boot] [PATCH v1] env_mmc: configure environment offsets via device tree

[Igor Grinberg]

On 02/17/17 19:28, Philipp Tomsich wrote:
> This introduces the ability to override the environment offets from the
> device tree by setting the following nodes in '/config':
>   'u-boot,mmc-env-offset' - overrides CONFIG_ENV_OFFSET
>   'u-boot,mmc-env-offset-redundant'
>   - overrides CONFIG_ENV_OFFSET_REDUND
> 
> To keep with the previous logic, the CONFIG_* defines still need to
> be available and the statically defined values become the defaults,
> when the corresponding properties are not set in the device-tree.

That sounds too odd...
DT's purpose is to describe the h/w... and that does not look so...
We also, have a dt file name in the environment, so this creates will create
a chicken and an egg problem...
I really don't think we should go that direction. DT is not meant to provide
a solution to all your problems...

> 
> Signed-off-by: Philipp Tomsich 
> ---
>  common/env_mmc.c | 31 +++
>  1 file changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/common/env_mmc.c b/common/env_mmc.c
> index 16f6a17..ef3dbd1 100644
> --- a/common/env_mmc.c
> +++ b/common/env_mmc.c
> @@ -1,24 +1,25 @@
>  /*
>   * (C) Copyright 2008-2011 Freescale Semiconductor, Inc.
>   *
>   * SPDX-License-Identifier:  GPL-2.0+
>   */
>  
>  /* #define DEBUG */
>  
>  #include 
>  
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
>  #include 
>  #include 
>  #include 
>  
>  #if defined(CONFIG_ENV_SIZE_REDUND) &&  \
>   (CONFIG_ENV_SIZE_REDUND != CONFIG_ENV_SIZE)
>  #error CONFIG_ENV_SIZE_REDUND should be the same as CONFIG_ENV_SIZE
>  #endif
>  
> @@ -36,21 +37,43 @@ DECLARE_GLOBAL_DATA_PTR;
>  #define CONFIG_ENV_OFFSET 0
>  #endif
>  
> -__weak int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr)
> +#ifdef CONFIG_OF_LIBFDT
> +static inline s64 mmc_offset(int copy)
>  {
> - s64 offset;
> + const char *propname = "u-boot,mmc-env-offset";
> + s64 defvalue = CONFIG_ENV_OFFSET;
>  
> - offset = CONFIG_ENV_OFFSET;
> -#ifdef CONFIG_ENV_OFFSET_REDUND
> +#if defined(CONFIG_ENV_OFFSET_REDUND)
> + if (copy) {
> + propname = "u-boot,mmc-env-offset-redundant";
> + defvalue = CONFIG_ENV_OFFSET_REDUND;
> + }
> +#endif
> +
> + return fdtdec_get_config_int(gd->fdt_blob, propname, defvalue);
> +}
> +#else
> +static inline s64 mmc_offset(int copy)
> +{
> + s64 offset = CONFIG_ENV_OFFSET;
> +
> +#if defined(CONFIG_ENV_OFFSET_REDUND)
>   if (copy)
>   offset = CONFIG_ENV_OFFSET_REDUND;
>  #endif
> + return offset;
> +}
> +#endif
> +
> +__weak int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr)
> +{
> + s64 offset = mmc_offset(copy);
>  
>   if (offset < 0)
>   offset += mmc->capacity;
>  
>   *env_addr = offset;
>  
>   return 0;
>  }
>  
> 

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

Re: [U-Boot] [u-boot PATCH v3 7/8] ARM: k2g: setup PRU ethernet MAC addresses

[Igor Grinberg]

Hi Tom,

On 02/07/17 20:28, Tom Rini wrote:
> On Tue, Feb 07, 2017 at 09:52:25AM +0200, Igor Grinberg wrote:
>> Hi Roger,
>>
>> On 02/06/17 11:36, Roger Quadros wrote:
>>> PRU ethernet MAC address range is present in the
>>> board EEPROM. Parse it and setup eth?addr
>>> environment variables.
>>>
>>> Signed-off-by: Roger Quadros <rog...@ti.com>
>>> Reviewed-by: Lokesh Vutla <lokeshvu...@ti.com>
>>> ---
>>>  board/ti/ks2_evm/board_k2g.c | 19 +++
>>>  1 file changed, 19 insertions(+)
>>>
>>> diff --git a/board/ti/ks2_evm/board_k2g.c b/board/ti/ks2_evm/board_k2g.c
>>> index 40edbaa..a738dd2 100644
>>> --- a/board/ti/ks2_evm/board_k2g.c
>>> +++ b/board/ti/ks2_evm/board_k2g.c
>>> @@ -12,6 +12,7 @@
>>>  #include 
>>>  #include 
>>>  #include "mux-k2g.h"
>>> +#include "../common/board_detect.h"
>>>  
>>>  #define SYS_CLK2400
>>>  
>>> @@ -149,6 +150,24 @@ int board_early_init_f(void)
>>>  }
>>>  #endif
>>>  
>>> +#ifdef CONFIG_BOARD_LATE_INIT
>>> +int board_late_init(void)
>>> +{
>>> +#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_TI_I2C_BOARD_DETECT)
>>> +   int rc;
>>> +
>>> +   rc = ti_i2c_eeprom_am_get(CONFIG_EEPROM_BUS_ADDRESS,
>>> +   CONFIG_EEPROM_CHIP_ADDRESS);
>>> +   if (rc)
>>> +   printf("ti_i2c_eeprom_init failed %d\n", rc);
>>> +
>>> +   board_ti_set_ethaddr(1);
>>
>> What if the MAC address has already been set in the environment?
>> AFAIR, the MAC address in the environment has a higher precedence
>> than others.
>> May be I missed this, but I don't remember any discussion about changing
>> this assumption.
>> So, if the assumption is still correct, you shouldn't change the MAC in the 
>> env.
> 
> This is true.  Can we perhaps come up with a helper function that's
> normally called to set the "eth?addr" to MAC if unset already, instead
> of having N instances of the same logic?

That sounds like a good plan.

-- 
Regards,
Igor.



signature.asc
Description: OpenPGP digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [u-boot PATCH v3 7/8] ARM: k2g: setup PRU ethernet MAC addresses

[Igor Grinberg]

Hi Roger,

On 02/08/17 10:51, Roger Quadros wrote:
> Hi Igor,
> 
> On 07/02/17 09:52, Igor Grinberg wrote:
>> Hi Roger,
>>
>> On 02/06/17 11:36, Roger Quadros wrote:
>>> PRU ethernet MAC address range is present in the
>>> board EEPROM. Parse it and setup eth?addr
>>> environment variables.
>>>
>>> Signed-off-by: Roger Quadros <rog...@ti.com>
>>> Reviewed-by: Lokesh Vutla <lokeshvu...@ti.com>
>>> ---
>>>  board/ti/ks2_evm/board_k2g.c | 19 +++
>>>  1 file changed, 19 insertions(+)
>>>
>>> diff --git a/board/ti/ks2_evm/board_k2g.c b/board/ti/ks2_evm/board_k2g.c
>>> index 40edbaa..a738dd2 100644
>>> --- a/board/ti/ks2_evm/board_k2g.c
>>> +++ b/board/ti/ks2_evm/board_k2g.c
>>> @@ -12,6 +12,7 @@
>>>  #include 
>>>  #include 
>>>  #include "mux-k2g.h"
>>> +#include "../common/board_detect.h"
>>>  
>>>  #define SYS_CLK2400
>>>  
>>> @@ -149,6 +150,24 @@ int board_early_init_f(void)
>>>  }
>>>  #endif
>>>  
>>> +#ifdef CONFIG_BOARD_LATE_INIT
>>> +int board_late_init(void)
>>> +{
>>> +#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_TI_I2C_BOARD_DETECT)
>>> +   int rc;
>>> +
>>> +   rc = ti_i2c_eeprom_am_get(CONFIG_EEPROM_BUS_ADDRESS,
>>> +   CONFIG_EEPROM_CHIP_ADDRESS);
>>> +   if (rc)
>>> +   printf("ti_i2c_eeprom_init failed %d\n", rc);
>>> +
>>> +   board_ti_set_ethaddr(1);
>>
>> What if the MAC address has already been set in the environment?
> 
> by whom?
> At least as of now nobody is setting ethadddr1 on the k2g-ice board.

Well, for example by user... and it is eth1addr.

> 
>> AFAIR, the MAC address in the environment has a higher precedence
>> than others.
>> May be I missed this, but I don't remember any discussion about changing
>> this assumption.
>> So, if the assumption is still correct, you shouldn't change the MAC in the 
>> env.
> 
> I agree with you. I saw Olliver's "Retrieve MAC address from EEPROM" series.
> However, that may not apply to TI boards yet because:
> -the SoC's ethernet devices MAC addresses are stored in the SoC registers.
> -the PRU ethernet MAC addresses which this patch is setting are stored in
> EEPROM but they are not used at u-boot at all.
> 
> I'm open to ideas if we can do what we're doing in a better way.

I think Tom's idea of a common function or may be a change to
eth_setenv_enetaddr_*() functions that will handle the precedence
is very sensible for such cases. 

>>
>>> +#endif
>>> +
>>> +   return 0;
>>> +}
>>> +#endif
>>> +
>>>  #ifdef CONFIG_SPL_BUILD
>>>  void spl_init_keystone_plls(void)
>>>  {
>>>
>>
> 

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

Re: [U-Boot] [u-boot PATCH v3 7/8] ARM: k2g: setup PRU ethernet MAC addresses

[Igor Grinberg]

Hi Roger,

On 02/06/17 11:36, Roger Quadros wrote:
> PRU ethernet MAC address range is present in the
> board EEPROM. Parse it and setup eth?addr
> environment variables.
> 
> Signed-off-by: Roger Quadros 
> Reviewed-by: Lokesh Vutla 
> ---
>  board/ti/ks2_evm/board_k2g.c | 19 +++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/board/ti/ks2_evm/board_k2g.c b/board/ti/ks2_evm/board_k2g.c
> index 40edbaa..a738dd2 100644
> --- a/board/ti/ks2_evm/board_k2g.c
> +++ b/board/ti/ks2_evm/board_k2g.c
> @@ -12,6 +12,7 @@
>  #include 
>  #include 
>  #include "mux-k2g.h"
> +#include "../common/board_detect.h"
>  
>  #define SYS_CLK  2400
>  
> @@ -149,6 +150,24 @@ int board_early_init_f(void)
>  }
>  #endif
>  
> +#ifdef CONFIG_BOARD_LATE_INIT
> +int board_late_init(void)
> +{
> +#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_TI_I2C_BOARD_DETECT)
> + int rc;
> +
> + rc = ti_i2c_eeprom_am_get(CONFIG_EEPROM_BUS_ADDRESS,
> + CONFIG_EEPROM_CHIP_ADDRESS);
> + if (rc)
> + printf("ti_i2c_eeprom_init failed %d\n", rc);
> +
> + board_ti_set_ethaddr(1);

What if the MAC address has already been set in the environment?
AFAIR, the MAC address in the environment has a higher precedence
than others.
May be I missed this, but I don't remember any discussion about changing
this assumption.
So, if the assumption is still correct, you shouldn't change the MAC in the env.

> +#endif
> +
> + return 0;
> +}
> +#endif
> +
>  #ifdef CONFIG_SPL_BUILD
>  void spl_init_keystone_plls(void)
>  {
> 

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

Re: [U-Boot] [PATCH 1/2] arm: Remove unofficial mach-type number uses

[Igor Grinberg]

Hi Tom,

On 01/24/17 23:31, Tom Rini wrote:
> We have a number of instances of platforms that define a MACH_TYPE_xxx
> value and number.  We are currently synced with the latest vales from
> the Linux Kernel and in turn if numbers are not used there it is because
> they were never officially used anywhere.  We drop all our instances of
> these numbers.

[...]

>  include/configs/cm_t335.h |  3 ---

[...]

> diff --git a/include/configs/cm_t335.h b/include/configs/cm_t335.h
> index 8d5f26a139c3..e37d96707979 100644
> --- a/include/configs/cm_t335.h
> +++ b/include/configs/cm_t335.h
> @@ -24,9 +24,6 @@
>  #undef CONFIG_MAX_RAM_BANK_SIZE
>  #define CONFIG_MAX_RAM_BANK_SIZE (512 << 20) /* 512MB */
>  
> -#define MACH_TYPE_CM_T3354586/* Until the next sync */
> -#define CONFIG_MACH_TYPE MACH_TYPE_CM_T335
> -
>  /* Clock Defines */
>  #define V_OSCK   2500  /* Clock output from 
> T2 */
>  #define V_SCLK   (V_OSCK)

Most of cm-t335 customers use non-DT Linux kernel 3.2 in production.
I'd really appreciate if we can keep the two above lines, so we or our customers
can just update/use the upstream U-Boot.
In fact, this is the only board that received Linux upstream DT-only support 
while
there were non-DT kernels released to customers.

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

Re: [U-Boot] [PATCH 1/2] arm: Remove unofficial mach-type number uses

[Igor Grinberg]

Hi guys,

On 01/25/17 03:34, Vladimir Zapolskiy wrote:
> On 01/25/2017 01:13 AM, Tom Rini wrote:
>> [re-sending without a URL in it]
>>
>> On Wed, Jan 25, 2017 at 12:28:47AM +0200, Vladimir Zapolskiy wrote:
>>> Hi Tom,
>>>
>>> On 01/24/2017 11:31 PM, Tom Rini wrote:
 We have a number of instances of platforms that define a MACH_TYPE_xxx
 value and number.  We are currently synced with the latest vales from
 the Linux Kernel and in turn if numbers are not used there it is because
 they were never officially used anywhere.  We drop all our instances of
 these numbers.
>>>
>>> [snip]

[...]

>> So I talked with rmk about this quickly and the reduced list, which is
>> what the kernel uses and I also wish to use is the reduced list of
>> MACH_TYPE_xxx and the short version is that the reduced list only
>> includes the MACH_TYPE_xxx values that were used in mainline rather than
>> just registered.  Perhaps instead of saying "officially used" I should
>> make it clearer that it means used in the upstream linux kernel?
>>
> 
> Right, just the usage of the list in the upstream Linux kernel is assumed.
> 
> I see a few more points for consideration.
> 
> 1) From Documentation/arm/README the list of machine type IDs is only
> usable for non-DT machines and combined DT/non-DT machines. I suppose it
> should be rather safe to remove mach types from U-Boot board files for
> all archs/boards with CONFIG_OF_LIBFDT build option selected, for example
> the DevKit3250 board falls into this category. By this criterion you may
> consider to split the change into two -- one change is safe, another
> change updates legacy or badly maintained boards.
> 
> 2) The reduced mach-types list is used by Linux, I'm not sure and it
> may happen that other OS kernels recognize the complete list. Also if
> by chance someone wants to update a U-Boot image and keep the old Linux
> kernel, there is a possibility that the old kernel won't boot.

cm-t335 is an example of such machine.
Most of our cm-t335 customers use non-DT kernel 3.2 in their production.
Removing cm-t335 mach type will prevent us to provide them with an
updated bootloader, or will force us to maintain an out of tree patch...

> 
> 3) From header info in the Russell's mach-types file, the reduced list
> enumerates machines found in vanilla or machines with information not
> edited for one year. Generally this is a soft enforcement to push machine
> support to the kernel officially (or better switch it to DT), because
> unlikely once correctly added information needs any further updates,
> and most of the entries are removed due to this reason.
> 
> Hypothetically it could be possible that a machine maintainer finds that
> after rebasing private changes on top of the latest Linux release, a board
> mach type is not recognized by the Linux kernel due to an expired and
> removed entry in mach-types, then s/he updates a record in Russell's DB
> to get the old machine type ID again. However a counterpart change in
> U-boot board config won't be synced automatically, and IMHO it should be
> stated that this problem is neglected, obviously because it causes
> maintenance burden for no gain.
> 
> That said, your change is good, it may produce a discontent in particular
> cases, but it won't be problematic to remove it by partial commit
> reverting and adding an explanatory comment into the code.
> 
> --
> With best wishes,
> Vladimir
> 

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

Re: [U-Boot] [PATCH] arm: layerscape: Enable UUID & GPT partition for NXP's ARM SoC

[Igor Grinberg]

On 01/19/17 19:31, york sun wrote:
> On 12/25/2016 10:45 PM, Prabhakar Kushwaha wrote:
>> Enable UUID and GPT partition support for NXP's ARM based SoCs
>> i.e. LS1012A, LS1021A, LS1043A, LS1046A and LS2080A.
>>
>> Also enable DOS partition for LS1012AFRDM boards.
>>
>> Signed-off-by: Prabhakar Kushwaha 
>> ---
> 
> Applied to fsl-qoriq master, awaiting upstream. Thanks.

Well, of course it is your call, but that's sad.
Why not request people to move things to Kconfig?

I would suggest removing these from scripts/config_whitelist.txt...


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

Re: [U-Boot] [PATCH v5 2/2] splash: add support for loading splash from a FIT image

[Igor Grinberg]

On 01/13/17 13:20, Tomas Melin wrote:
> Enable support for loading a splash image from within a FIT image.
> The image is assumed to be generated with mkimage -E flag to hold
> the data external to the FIT.
> 
> Signed-off-by: Tomas Melin <tomas.me...@vaisala.com>

Acked-by: Igor Grinberg <grinb...@compulab.co.il>

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

Re: [U-Boot] [PATCH] arm: layerscape: Enable UUID & GPT partition for NXP's ARM SoC

[Igor Grinberg]

On 01/05/17 18:25, york sun wrote:
> On 01/05/2017 01:51 AM, Igor Grinberg wrote:
>> On 01/04/17 23:41, york sun wrote:
>>> On 12/25/2016 11:29 PM, Igor Grinberg wrote:
>>>> Hi Prabhakar Kushwaha,
>>>>
>>>> On 12/26/16 08:45, Prabhakar Kushwaha wrote:
>>>>> Enable UUID and GPT partition support for NXP's ARM based SoCs
>>>>> i.e. LS1012A, LS1021A, LS1043A, LS1046A and LS2080A.
>>>>>
>>>>> Also enable DOS partition for LS1012AFRDM boards.
>>>>>
>>>>> Signed-off-by: Prabhakar Kushwaha <prabhakar.kushw...@nxp.com>
>>>>> ---
>>>>>  include/configs/ls1012afrdm.h | 5 +
>>>>>  include/configs/ls1012aqds.h  | 4 
>>>>>  include/configs/ls1012ardb.h  | 4 
>>>>>  include/configs/ls1021aiot.h  | 3 +++
>>>>>  include/configs/ls1021aqds.h  | 3 +++
>>>>>  include/configs/ls1021atwr.h  | 3 +++
>>>>>  include/configs/ls1043aqds.h  | 4 
>>>>>  include/configs/ls1043ardb.h  | 4 
>>>>>  include/configs/ls1046aqds.h  | 4 
>>>>>  include/configs/ls1046ardb.h  | 4 
>>>>>  include/configs/ls2080aqds.h  | 3 +++
>>>>>  include/configs/ls2080ardb.h  | 3 +++
>>>>
>>>> I'm not sure this is the expected way today...
>>>>
>>>>>  12 files changed, 44 insertions(+)
>>>>>
>>>>> diff --git a/include/configs/ls1012afrdm.h b/include/configs/ls1012afrdm.h
>>>>> index f6f88e8..94f7460 100644
>>>>> --- a/include/configs/ls1012afrdm.h
>>>>> +++ b/include/configs/ls1012afrdm.h
>>>>> @@ -42,6 +42,11 @@
>>>>>  #define CONFIG_SYS_USB_XHCI_MAX_ROOT_PORTS  2
>>>>>  #endif
>>>>>
>>>>> +#define CONFIG_DOS_PARTITION
>>>>> +#define CONFIG_PARTITION_UUIDS
>>>>> +#define CONFIG_EFI_PARTITION
>>>>> +#define CONFIG_CMD_GPT
>>>>> +
>>>>
>>>> These are the common (not board specific) things and I think today we 
>>>> expect
>>>> it to come through the Kconfig instead of the old config files.
>>>
>>> Igor,
>>>
>>> They are not totally common. The board has to support the device. I
>>> would recommend to use Kconfig as well if such config is in Kconfig.
>>> However it is not the case here.
>>
>> Right. They are not in Kconfig yet...
>> We are making efforts to move stuff to Kconfig.
>> Care to land a hand and move these ones to Kconfig?
>>
> 
> Can't promise. I have been doing just that for powerpc. Still have many 
> left.

Yeah, great job on those patches! Thanks!
I think it is still worth waiting instead of applying and moving afterwards...

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

Re: [U-Boot] [PATCH] arm: layerscape: Enable UUID & GPT partition for NXP's ARM SoC

[Igor Grinberg]

Hi Prabhakar Kushwaha,

On 12/26/16 08:45, Prabhakar Kushwaha wrote:
> Enable UUID and GPT partition support for NXP's ARM based SoCs
> i.e. LS1012A, LS1021A, LS1043A, LS1046A and LS2080A.
> 
> Also enable DOS partition for LS1012AFRDM boards.
> 
> Signed-off-by: Prabhakar Kushwaha 
> ---
>  include/configs/ls1012afrdm.h | 5 +
>  include/configs/ls1012aqds.h  | 4 
>  include/configs/ls1012ardb.h  | 4 
>  include/configs/ls1021aiot.h  | 3 +++
>  include/configs/ls1021aqds.h  | 3 +++
>  include/configs/ls1021atwr.h  | 3 +++
>  include/configs/ls1043aqds.h  | 4 
>  include/configs/ls1043ardb.h  | 4 
>  include/configs/ls1046aqds.h  | 4 
>  include/configs/ls1046ardb.h  | 4 
>  include/configs/ls2080aqds.h  | 3 +++
>  include/configs/ls2080ardb.h  | 3 +++

I'm not sure this is the expected way today...

>  12 files changed, 44 insertions(+)
> 
> diff --git a/include/configs/ls1012afrdm.h b/include/configs/ls1012afrdm.h
> index f6f88e8..94f7460 100644
> --- a/include/configs/ls1012afrdm.h
> +++ b/include/configs/ls1012afrdm.h
> @@ -42,6 +42,11 @@
>  #define CONFIG_SYS_USB_XHCI_MAX_ROOT_PORTS  2
>  #endif
>  
> +#define CONFIG_DOS_PARTITION
> +#define CONFIG_PARTITION_UUIDS
> +#define CONFIG_EFI_PARTITION
> +#define CONFIG_CMD_GPT
> +

These are the common (not board specific) things and I think today we expect
it to come through the Kconfig instead of the old config files.

[...]

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

Re: [U-Boot] [PATCH v4 2/2] splash: add support for loading splash from a FIT image

[Igor Grinberg]

Hi Tomas,

Sorry for the late response...

On 12/20/16 07:29, Tomas Melin wrote:
> Hi Igor, Simon,
> 
> On 12/15/2016 11:08 AM, Igor Grinberg wrote:
>> Hi Tomas,
>>
>> On 12/14/16 16:23, Tomas Melin wrote:
>>> Hi Simon, Igor,
>>>
>>> On 12/14/2016 02:53 PM, Igor Grinberg wrote:
>>>> On 12/13/16 22:29, Simon Glass wrote:
>>>>>>>>
>>>>>>>> I think two above debug() are very legitimate - no need to shout if no 
>>>>>>>> FIT image
>>>>>>>> or no splash in it...
>>>>>>>>
>>>>>>>>> + res = fit_image_get_data_offset(fit_header, node_offset,
>>>>>>>>> + _offset);
>>>>>>>>> + if (res < 0) {
>>>>>>>>> + debug("Could not find 'data-offset' property in FIT\n");
>>>>>>>>> + return res;
>>>>>>>>> + }
>>>>>>>>> +
>>>>>>>>> + res = fit_image_get_data_size(fit_header, node_offset, 
>>>>>>>>> _size);
>>>>>>>>> + if (res < 0) {
>>>>>>>>> + debug("Could not find 'data-size' property in FIT\n");
>>>>>>>>> + return res;
>>>>>>>>> + }
>>>>>>>>
>>>>>>>> Now regarding these two, I'm not sure.
>>>>>>>> Since we have found a valid FIT and also a node with a correct splash 
>>>>>>>> name,
>>>>>>>> probably the intent is that we show the splash, right?
>>>>>>>> But in the two above checks, we find inconsistencies that do not allow 
>>>>>>>> us to
>>>>>>>> show the splash - meaning the FIT is not actually good (am I right 
>>>>>>>> here?).
>>>>>>>> So may be we should report it to the 'user' and allow correcting the 
>>>>>>>> FIT?
>>>>>>>> Otherwise, it is impossible to debug the image w/o a debug version of 
>>>>>>>> U-Boot...
>>>>>>>> Do I make sense, or do I miss something?
>>>>>>>
>>>>>>> Yes that makes some sense, but the problem is that then you are
>>>>>>> including error messages always which would never happen in a working
>>>>>>> system (i.e. it just bloats the code).
>>>>>>
>>>>>> Unless, there a kind of corruption or a user mistake and then that same
>>>>>> user can't even understand what happened because we do not help him with
>>>>>> an error message.
>>>>>> You cannot know that these error messages will never happen...
>>>>>> This is a generic code which can be used by a wide variety of platforms -
>>>>>> I don't think you can foresee all the possible use cases.
>>>>>>
>>>>>> We are talking about several tens of bytes vs. usability.
>>>>>> If there is an error, it should be stated as such. It should not just
>>>>>> exit silently...
>>>>>
>>>>> I agree with that, there should definitely be an error printed. It
>>>>> should say something like 'Failed to load splash screen (err=-4)' or
>>>>> something like that. The error number should provide some clues and
>>>>> people can dig in.
>>>>
>>>> Great idea!
>>>
>>> splash_load_fit() currently fails silently but still reports the error in
>>> the return value. And this function is used so that board.c calls 
>>> splash_source_load()->splash_load_fit().
>>> The board function call will get notified of the error value as provided
>>> by the return value for splash_load_fit(). In our board implementation that 
>>> is 
>>> actually exactly how it is done, the board function call checks the return
>>> value and prints ("Failed to load splash screen image, error: %d\n", ret)
>>> in case there was and error.
>>>
>>> IMHO this is quite good behaviour as is, leaving it up to the implementation
>>> in the board.c if there should be a error message or not (and if it should 
>>> bloat the code with another printf or not).
>>
>> Well, yes this makes sense if you care to do the work in the board code.
>> Although, I would expect that sometime this code will be called from
>> a generic board independent place (e.g. init array, etc.).
> 
> I don't have a strong opinion, even if I do prefer the current version.
> Please let me know if patch this still needs changes. 

Well, I can see two courses this patch can take:
1) We merge it as-is and any additional board uses the splash_load_fit() will
   copy/paste the error handling from your board, or generalizes it and patches
   both boards.
2) We make a more generic approach (e.g. print error messages in the common 
code)
   and any new user (board or common call) will not need to handle the errors,
   but just use the call.

Either one is good with me.

Have a great holidays everyone!

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

Re: [U-Boot] [PATCH v4 2/2] splash: add support for loading splash from a FIT image

[Igor Grinberg]

Hi Tomas,

On 12/14/16 16:23, Tomas Melin wrote:
> Hi Simon, Igor,
> 
> On 12/14/2016 02:53 PM, Igor Grinberg wrote:
>> On 12/13/16 22:29, Simon Glass wrote:
>>>>>>
>>>>>> I think two above debug() are very legitimate - no need to shout if no 
>>>>>> FIT image
>>>>>> or no splash in it...
>>>>>>
>>>>>>> + res = fit_image_get_data_offset(fit_header, node_offset,
>>>>>>> + _offset);
>>>>>>> + if (res < 0) {
>>>>>>> + debug("Could not find 'data-offset' property in FIT\n");
>>>>>>> + return res;
>>>>>>> + }
>>>>>>> +
>>>>>>> + res = fit_image_get_data_size(fit_header, node_offset, 
>>>>>>> _size);
>>>>>>> + if (res < 0) {
>>>>>>> + debug("Could not find 'data-size' property in FIT\n");
>>>>>>> + return res;
>>>>>>> + }
>>>>>>
>>>>>> Now regarding these two, I'm not sure.
>>>>>> Since we have found a valid FIT and also a node with a correct splash 
>>>>>> name,
>>>>>> probably the intent is that we show the splash, right?
>>>>>> But in the two above checks, we find inconsistencies that do not allow 
>>>>>> us to
>>>>>> show the splash - meaning the FIT is not actually good (am I right 
>>>>>> here?).
>>>>>> So may be we should report it to the 'user' and allow correcting the FIT?
>>>>>> Otherwise, it is impossible to debug the image w/o a debug version of 
>>>>>> U-Boot...
>>>>>> Do I make sense, or do I miss something?
>>>>>
>>>>> Yes that makes some sense, but the problem is that then you are
>>>>> including error messages always which would never happen in a working
>>>>> system (i.e. it just bloats the code).
>>>>
>>>> Unless, there a kind of corruption or a user mistake and then that same
>>>> user can't even understand what happened because we do not help him with
>>>> an error message.
>>>> You cannot know that these error messages will never happen...
>>>> This is a generic code which can be used by a wide variety of platforms -
>>>> I don't think you can foresee all the possible use cases.
>>>>
>>>> We are talking about several tens of bytes vs. usability.
>>>> If there is an error, it should be stated as such. It should not just
>>>> exit silently...
>>>
>>> I agree with that, there should definitely be an error printed. It
>>> should say something like 'Failed to load splash screen (err=-4)' or
>>> something like that. The error number should provide some clues and
>>> people can dig in.
>>
>> Great idea!
> 
> splash_load_fit() currently fails silently but still reports the error in
> the return value. And this function is used so that board.c calls 
> splash_source_load()->splash_load_fit().
> The board function call will get notified of the error value as provided
> by the return value for splash_load_fit(). In our board implementation that 
> is 
> actually exactly how it is done, the board function call checks the return
> value and prints ("Failed to load splash screen image, error: %d\n", ret)
> in case there was and error.
> 
> IMHO this is quite good behaviour as is, leaving it up to the implementation
> in the board.c if there should be a error message or not (and if it should 
> bloat the code with another printf or not).

Well, yes this makes sense if you care to do the work in the board code.
Although, I would expect that sometime this code will be called from
a generic board independent place (e.g. init array, etc.).

> 
>>>>>
>>>>> So long as the error is reported (even if it is not a very specific
>>>>> error), people can add DEBUG and track it down.
>>>>
>>>> That depends who 'people' are? Devs? Users?
>>>
>>> Well in this case the user will never see the problem, unless someone
>>> has screwed up the splash screen image. Mostly I'm talking about devs.
>>>
>>> Better would be to have an expanded debug() system which lets you turn
>>> debugging on globally when hunting for a problem. That would be a nice
>>> project for someone...
>>
>> Yes, indeed that sounds like a nice project.
>> It would be great to be able to specify the debug verbosity on per build
>> basis (e.g. Kconfig).
>>
> 
> Indeed, that would be a great feature.
> 
> Regards,
> Tomas
> 

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

Re: [U-Boot] [PATCH v4 2/2] splash: add support for loading splash from a FIT image

[Igor Grinberg]

On 12/13/16 22:29, Simon Glass wrote:
> Hi Igor,
> 
> On 12 December 2016 at 01:37, Igor Grinberg <grinb...@compulab.co.il> wrote:
>> On 12/11/16 22:27, Simon Glass wrote:
>>> Hi Igor,
>>>
>>> On 11 December 2016 at 10:37, Igor Grinberg <grinb...@compulab.co.il> wrote:
>>>> Hi Tomas, Simon,
>>>>
>>>> Sorry, to break in that late...
>>>> I have a quick question below.
>>>>
>>>> On 12/05/16 09:36, Tomas Melin wrote:
>>>>> Enable support for loading a splash image from within a FIT image.
>>>>> The image is assumed to be generated with mkimage -E flag to hold
>>>>> the data external to the FIT.
>>>>>
>>>>> Signed-off-by: Tomas Melin <tomas.me...@vaisala.com>
>>>>
>>>> [...]
>>>>
>>>>> diff --git a/common/splash_source.c b/common/splash_source.c
>>>>> index 70d724f..94b46d3 100644
>>>>> --- a/common/splash_source.c
>>>>> +++ b/common/splash_source.c
>>>>
>>>> [...]
>>>>
>>>>> +#ifdef CONFIG_FIT
>>>>> +static int splash_load_fit(struct splash_location *location, u32 
>>>>> bmp_load_addr)
>>>>> +{
>>>>> + int res;
>>>>> + int node_offset;
>>>>> + int splash_offset;
>>>>> + int splash_size;
>>>>> + struct image_header *img_header;
>>>>> + const u32 *fit_header;
>>>>> + u32 fit_size;
>>>>> + const size_t header_size = sizeof(struct image_header);
>>>>> +
>>>>> + /* Read in image header */
>>>>> + res = splash_storage_read_raw(location, bmp_load_addr, header_size);
>>>>> + if (res < 0)
>>>>> + return res;
>>>>> +
>>>>> + img_header = (struct image_header *)bmp_load_addr;
>>>>> + fit_size = fdt_totalsize(img_header);
>>>>> +
>>>>> + /* Read in entire FIT */
>>>>> + fit_header = (const u32 *)(bmp_load_addr + header_size);
>>>>> + res = splash_storage_read_raw(location, (u32)fit_header, fit_size);
>>>>> + if (res < 0)
>>>>> + return res;
>>>>> +
>>>>> + res = fit_check_format(fit_header);
>>>>> + if (!res) {
>>>>> + debug("Could not find valid FIT image\n");
>>>>> + return -EINVAL;
>>>>> + }
>>>>> +
>>>>> + node_offset = fit_image_get_node(fit_header, location->name);
>>>>> + if (node_offset < 0) {
>>>>> + debug("Could not find splash image '%s' in FIT\n",
>>>>> +   location->name);
>>>>> + return -ENOENT;
>>>>> + }
>>>>> +
>>>>
>>>> I think two above debug() are very legitimate - no need to shout if no FIT 
>>>> image
>>>> or no splash in it...
>>>>
>>>>> + res = fit_image_get_data_offset(fit_header, node_offset,
>>>>> + _offset);
>>>>> + if (res < 0) {
>>>>> + debug("Could not find 'data-offset' property in FIT\n");
>>>>> + return res;
>>>>> + }
>>>>> +
>>>>> + res = fit_image_get_data_size(fit_header, node_offset, 
>>>>> _size);
>>>>> + if (res < 0) {
>>>>> + debug("Could not find 'data-size' property in FIT\n");
>>>>> + return res;
>>>>> + }
>>>>
>>>> Now regarding these two, I'm not sure.
>>>> Since we have found a valid FIT and also a node with a correct splash name,
>>>> probably the intent is that we show the splash, right?
>>>> But in the two above checks, we find inconsistencies that do not allow us 
>>>> to
>>>> show the splash - meaning the FIT is not actually good (am I right here?).
>>>> So may be we should report it to the 'user' and allow correcting the FIT?
>>>> Otherwise, it is impossible to debug the image w/o a debug version of 
>>>> U-Boot...
>>>> Do I make sense, or do I miss something?
>>>
>>> Yes that makes some sense, but the 

Re: [U-Boot] [PATCH 0/3] Add generic FDT memory bank decoding and gd initialization

[Igor Grinberg]

On 12/13/16 13:12, Michal Simek wrote:
> On 13.12.2016 09:56, Igor Grinberg wrote:
>> On 12/12/16 14:05, Mike Looijmans wrote:
>>> On 12-12-16 09:18, Michal Simek wrote:
>>>> On 12.12.2016 09:05, Igor Grinberg wrote:
>>>>> On 12/12/16 09:13, Nathan Rossi wrote:
>>>>>> On 12 December 2016 at 03:11, Igor Grinberg <grinb...@compulab.co.il> 
>>>>>> wrote:
>>>>>>> dropping the list for this one as the question seems to me irrelevant 
>>>>>>> to your patch set.
>>>>>>>
>>>>>>> On 12/11/16 18:47, Nathan Rossi wrote:
>>>>>>>> On 12 December 2016 at 01:08, Igor Grinberg <grinb...@compulab.co.il> 
>>>>>>>> wrote:
>>>>>>>>> Hi Nathan,
>>>>>>>>>
>>>>>>>>> On 12/11/16 15:58, Nathan Rossi wrote:
>>>>>>>>>> This series adds two functions for handling the memory bank decoding 
>>>>>>>>>> and
>>>>>>>>>> initialization of global data for use by boards in their dram_init 
>>>>>>>>>> and
>>>>>>>>>> dram_init_banksize functions.
>>>>>>>>>
>>>>>>>>> I might have missed some discussions on this meter,
>>>>>>>>> can you please provide the use cases for this?
>>>>>>>>> IMO, the bootloader's job is to initialize the DRAM, detect its size, 
>>>>>>>>> and pass
>>>>>>>>> the detected DRAM configuration on to an OS.
>>>>>>>>
>>>>>>>> Hi Igor,
>>>>>>>>
>>>>>>>> I do not think there have been any discussions on this (at least none
>>>>>>>> that I am aware of).
>>>>>>>>
>>>>>>>> Some boards (like Zynq and ZynqMP ones) are using
>>>>>>>> CONFIG_SYS_SDRAM_SIZE to define the amount of memory that is available
>>>>>>>> (since detection is not possible).
>>>>>>>
>>>>>>> May I ask why is detection not possible on these boards (may be SoCs)?
>>>>>>
>>>>>> That is correct, Zynq and ZynqMP are ARM SoCs. Which in the majority
>>>>>> of cases are used in boards which have fixed memory device setups
>>>>>> (without any SPD or equivalent).
>>>>>
>>>>> Ok. That is understood. Yet, it does not explain why the detection
>>>>> cannot be done.. For example, you know which memory device setups you
>>>>> _can_ have on the board, so the detection is just to figure out which
>>>>> one is assembled. We are doing this on i.MXes, OMAPs, PXAs, and others.
>>>>>
>>>>> I was working on many ARM boards w/o SPD and still we almost always 
>>>>> develop
>>>>> a detection mechanism which has some assumptions and knowledge of the 
>>>>> board
>>>>> but it is much better then using some static data like compiled in or a 
>>>>> dtb.
>>>>>
>>>>> Have you considered a detection mechanism at all?
>>>>
>>>> If you look at my previous email as you see that topic.nl is using this.
>>>>
>>>> But the question is if this will work with all cases or just for
>>>> particular configuration. All zynq/zynqmp boards requires initial
>>>> setting which is created in Xilinx design tools. Export for these uniq
>>>> configurations are in ps7_init* or psu_init* files where DDR
>>>> configuration is part of this.
>>>>
>>>> Devices contain various configurations for various memory types. Also
>>>> there is an option to add memory controller to programmable logic and
>>>> use it.
>>>
>>> And the static memory controller can probably also be used to drive SRAM...
>>>
>>> But you could combine the two. The devicetree could set up the area's to 
>>> search, and then a detection routine to check
>  what's really there. This has the added value of a quick test that the
> memory actually works before starting to use it.
>>
>> That's exactly the point I was trying to show.
> 
> No problem with this but for me this is generic configuration option.
> It means this should be covered by Kconfig to be selected for specific
> platform. This code should go to common folder not to board folder or
> arm-mach folder.

Well, if it is generic enough it really should.

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

Re: [U-Boot] [PATCH 0/3] Add generic FDT memory bank decoding and gd initialization

[Igor Grinberg]

On 12/13/16 13:07, Michal Simek wrote:
> On 13.12.2016 09:53, Igor Grinberg wrote:
>> On 12/12/16 13:56, Michal Simek wrote:
>>> On 12.12.2016 12:10, Igor Grinberg wrote:
>>>> On 12/12/16 11:03, Michal Simek wrote:
>>>>> On 12.12.2016 09:54, Igor Grinberg wrote:
>>>>>> On 12/12/16 10:27, Michal Simek wrote:
>>>>>>> On 12.12.2016 09:24, Igor Grinberg wrote:
>>>>>>>> On 12/12/16 10:02, Michal Simek wrote:
>>>>>>>>> On 12.12.2016 08:13, Nathan Rossi wrote:
>>>>>>>>>> On 12 December 2016 at 03:11, Igor Grinberg 
>>>>>>>>>> <grinb...@compulab.co.il> wrote:
>>>>>>>>>>> dropping the list for this one as the question seems to me 
>>>>>>>>>>> irrelevant to your patch set.
>>>>>>>>>>>
>>>>>>>>>>> On 12/11/16 18:47, Nathan Rossi wrote:
>>>>>>>>>>>> On 12 December 2016 at 01:08, Igor Grinberg 
>>>>>>>>>>>> <grinb...@compulab.co.il> wrote:
>>>>>>>>>>>>> Hi Nathan,
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 12/11/16 15:58, Nathan Rossi wrote:
>>>>>>>>>>>>>> This series adds two functions for handling the memory bank 
>>>>>>>>>>>>>> decoding and
>>>>>>>>>>>>>> initialization of global data for use by boards in their 
>>>>>>>>>>>>>> dram_init and
>>>>>>>>>>>>>> dram_init_banksize functions.
>>>>>>>>>>>>>
>>>>>>>>>>>>> I might have missed some discussions on this meter,
>>>>>>>>>>>>> can you please provide the use cases for this?
>>>>>>>>>>>>> IMO, the bootloader's job is to initialize the DRAM, detect its 
>>>>>>>>>>>>> size, and pass
>>>>>>>>>>>>> the detected DRAM configuration on to an OS.
>>>>>>>>>>>>
>>>>>>>>>>>> Hi Igor,
>>>>>>>>>>>>
>>>>>>>>>>>> I do not think there have been any discussions on this (at least 
>>>>>>>>>>>> none
>>>>>>>>>>>> that I am aware of).
>>>>>>>>>>>>
>>>>>>>>>>>> Some boards (like Zynq and ZynqMP ones) are using
>>>>>>>>>>>> CONFIG_SYS_SDRAM_SIZE to define the amount of memory that is 
>>>>>>>>>>>> available
>>>>>>>>>>>> (since detection is not possible).
>>>>>>>>>>>
>>>>>>>>>>> May I ask why is detection not possible on these boards (may be 
>>>>>>>>>>> SoCs)?
>>>>>>>>>>
>>>>>>>>>> That is correct, Zynq and ZynqMP are ARM SoCs. Which in the majority
>>>>>>>>>> of cases are used in boards which have fixed memory device setups
>>>>>>>>>> (without any SPD or equivalent).
>>>>>>>>>
>>>>>>>>> For example zcu102 zynqmp development board is using modules with SPD 
>>>>>>>>> on
>>>>>>>>> it but if you look at generic SPD support then you will find out that
>>>>>>>>> FSL(drivers/ddr/fsl) is one of the major platform which is using it 
>>>>>>>>> for
>>>>>>>>> ddr size detection.
>>>>>>>>> Anyway as Nathan wrote above the majority of boards with zynq/zynqmp
>>>>>>>>> devices need to be configured at build time or at run time by DTB.
>>>>>>>>>
>>>>>>>>> There is also topic.nl boards which contain ddr configuration the same
>>>>>>>>> for different ddr sizes and Mike sent this patch to get it work
>>>>>>>>> http://lists.denx.de/pipermail/u-boot/2016-November/273385.html
>>>>>>>>
>>>>>>>> That's exactly my point. I think Mike's patch does a way better job
&

Re: [U-Boot] [PATCH 0/3] Add generic FDT memory bank decoding and gd initialization

[Igor Grinberg]

On 12/12/16 14:05, Mike Looijmans wrote:
> On 12-12-16 09:18, Michal Simek wrote:
>> On 12.12.2016 09:05, Igor Grinberg wrote:
>>> On 12/12/16 09:13, Nathan Rossi wrote:
>>>> On 12 December 2016 at 03:11, Igor Grinberg <grinb...@compulab.co.il> 
>>>> wrote:
>>>>> dropping the list for this one as the question seems to me irrelevant to 
>>>>> your patch set.
>>>>>
>>>>> On 12/11/16 18:47, Nathan Rossi wrote:
>>>>>> On 12 December 2016 at 01:08, Igor Grinberg <grinb...@compulab.co.il> 
>>>>>> wrote:
>>>>>>> Hi Nathan,
>>>>>>>
>>>>>>> On 12/11/16 15:58, Nathan Rossi wrote:
>>>>>>>> This series adds two functions for handling the memory bank decoding 
>>>>>>>> and
>>>>>>>> initialization of global data for use by boards in their dram_init and
>>>>>>>> dram_init_banksize functions.
>>>>>>>
>>>>>>> I might have missed some discussions on this meter,
>>>>>>> can you please provide the use cases for this?
>>>>>>> IMO, the bootloader's job is to initialize the DRAM, detect its size, 
>>>>>>> and pass
>>>>>>> the detected DRAM configuration on to an OS.
>>>>>>
>>>>>> Hi Igor,
>>>>>>
>>>>>> I do not think there have been any discussions on this (at least none
>>>>>> that I am aware of).
>>>>>>
>>>>>> Some boards (like Zynq and ZynqMP ones) are using
>>>>>> CONFIG_SYS_SDRAM_SIZE to define the amount of memory that is available
>>>>>> (since detection is not possible).
>>>>>
>>>>> May I ask why is detection not possible on these boards (may be SoCs)?
>>>>
>>>> That is correct, Zynq and ZynqMP are ARM SoCs. Which in the majority
>>>> of cases are used in boards which have fixed memory device setups
>>>> (without any SPD or equivalent).
>>>
>>> Ok. That is understood. Yet, it does not explain why the detection
>>> cannot be done.. For example, you know which memory device setups you
>>> _can_ have on the board, so the detection is just to figure out which
>>> one is assembled. We are doing this on i.MXes, OMAPs, PXAs, and others.
>>>
>>> I was working on many ARM boards w/o SPD and still we almost always develop
>>> a detection mechanism which has some assumptions and knowledge of the board
>>> but it is much better then using some static data like compiled in or a dtb.
>>>
>>> Have you considered a detection mechanism at all?
>>
>> If you look at my previous email as you see that topic.nl is using this.
>>
>> But the question is if this will work with all cases or just for
>> particular configuration. All zynq/zynqmp boards requires initial
>> setting which is created in Xilinx design tools. Export for these uniq
>> configurations are in ps7_init* or psu_init* files where DDR
>> configuration is part of this.
>>
>> Devices contain various configurations for various memory types. Also
>> there is an option to add memory controller to programmable logic and
>> use it.
> 
> And the static memory controller can probably also be used to drive SRAM...
> 
> But you could combine the two. The devicetree could set up the area's to 
> search, and then a detection routine to check what's really there. This has 
> the added value of a quick test that the memory actually works before 
> starting to use it.

That's exactly the point I was trying to show.
Thanks Mike.


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

Re: [U-Boot] [PATCH 0/3] Add generic FDT memory bank decoding and gd initialization

[Igor Grinberg]

On 12/12/16 13:56, Michal Simek wrote:
> On 12.12.2016 12:10, Igor Grinberg wrote:
>> On 12/12/16 11:03, Michal Simek wrote:
>>> On 12.12.2016 09:54, Igor Grinberg wrote:
>>>> On 12/12/16 10:27, Michal Simek wrote:
>>>>> On 12.12.2016 09:24, Igor Grinberg wrote:
>>>>>> On 12/12/16 10:02, Michal Simek wrote:
>>>>>>> On 12.12.2016 08:13, Nathan Rossi wrote:
>>>>>>>> On 12 December 2016 at 03:11, Igor Grinberg <grinb...@compulab.co.il> 
>>>>>>>> wrote:
>>>>>>>>> dropping the list for this one as the question seems to me irrelevant 
>>>>>>>>> to your patch set.
>>>>>>>>>
>>>>>>>>> On 12/11/16 18:47, Nathan Rossi wrote:
>>>>>>>>>> On 12 December 2016 at 01:08, Igor Grinberg 
>>>>>>>>>> <grinb...@compulab.co.il> wrote:
>>>>>>>>>>> Hi Nathan,
>>>>>>>>>>>
>>>>>>>>>>> On 12/11/16 15:58, Nathan Rossi wrote:
>>>>>>>>>>>> This series adds two functions for handling the memory bank 
>>>>>>>>>>>> decoding and
>>>>>>>>>>>> initialization of global data for use by boards in their dram_init 
>>>>>>>>>>>> and
>>>>>>>>>>>> dram_init_banksize functions.
>>>>>>>>>>>
>>>>>>>>>>> I might have missed some discussions on this meter,
>>>>>>>>>>> can you please provide the use cases for this?
>>>>>>>>>>> IMO, the bootloader's job is to initialize the DRAM, detect its 
>>>>>>>>>>> size, and pass
>>>>>>>>>>> the detected DRAM configuration on to an OS.
>>>>>>>>>>
>>>>>>>>>> Hi Igor,
>>>>>>>>>>
>>>>>>>>>> I do not think there have been any discussions on this (at least none
>>>>>>>>>> that I am aware of).
>>>>>>>>>>
>>>>>>>>>> Some boards (like Zynq and ZynqMP ones) are using
>>>>>>>>>> CONFIG_SYS_SDRAM_SIZE to define the amount of memory that is 
>>>>>>>>>> available
>>>>>>>>>> (since detection is not possible).
>>>>>>>>>
>>>>>>>>> May I ask why is detection not possible on these boards (may be SoCs)?
>>>>>>>>
>>>>>>>> That is correct, Zynq and ZynqMP are ARM SoCs. Which in the majority
>>>>>>>> of cases are used in boards which have fixed memory device setups
>>>>>>>> (without any SPD or equivalent).
>>>>>>>
>>>>>>> For example zcu102 zynqmp development board is using modules with SPD on
>>>>>>> it but if you look at generic SPD support then you will find out that
>>>>>>> FSL(drivers/ddr/fsl) is one of the major platform which is using it for
>>>>>>> ddr size detection.
>>>>>>> Anyway as Nathan wrote above the majority of boards with zynq/zynqmp
>>>>>>> devices need to be configured at build time or at run time by DTB.
>>>>>>>
>>>>>>> There is also topic.nl boards which contain ddr configuration the same
>>>>>>> for different ddr sizes and Mike sent this patch to get it work
>>>>>>> http://lists.denx.de/pipermail/u-boot/2016-November/273385.html
>>>>>>
>>>>>> That's exactly my point. I think Mike's patch does a way better job
>>>>>> detecting at run time than any compiled in or a DT based pseudo 
>>>>>> detection...
>>>>>>
>>>>>>>
>>>>>>> Anyway in general there are some ways how to configure DDR size
>>>>>>> - build time setup (CONFIG_SYS_SDRAM*)
>>>>>>> - run time setup
>>>>>>> - ddr detection
>>>>>>> - via SPD (like FSL)
>>>>>>> - via algorithm (like topic.nl)
>>>>>>> - configuration
>>>>>>> - read DTB
>>>>>>>
>>>&

Re: [U-Boot] [PATCH 0/3] Add generic FDT memory bank decoding and gd initialization

[Igor Grinberg]

On 12/12/16 11:03, Michal Simek wrote:
> On 12.12.2016 09:54, Igor Grinberg wrote:
>> On 12/12/16 10:27, Michal Simek wrote:
>>> On 12.12.2016 09:24, Igor Grinberg wrote:
>>>> On 12/12/16 10:02, Michal Simek wrote:
>>>>> On 12.12.2016 08:13, Nathan Rossi wrote:
>>>>>> On 12 December 2016 at 03:11, Igor Grinberg <grinb...@compulab.co.il> 
>>>>>> wrote:
>>>>>>> dropping the list for this one as the question seems to me irrelevant 
>>>>>>> to your patch set.
>>>>>>>
>>>>>>> On 12/11/16 18:47, Nathan Rossi wrote:
>>>>>>>> On 12 December 2016 at 01:08, Igor Grinberg <grinb...@compulab.co.il> 
>>>>>>>> wrote:
>>>>>>>>> Hi Nathan,
>>>>>>>>>
>>>>>>>>> On 12/11/16 15:58, Nathan Rossi wrote:
>>>>>>>>>> This series adds two functions for handling the memory bank decoding 
>>>>>>>>>> and
>>>>>>>>>> initialization of global data for use by boards in their dram_init 
>>>>>>>>>> and
>>>>>>>>>> dram_init_banksize functions.
>>>>>>>>>
>>>>>>>>> I might have missed some discussions on this meter,
>>>>>>>>> can you please provide the use cases for this?
>>>>>>>>> IMO, the bootloader's job is to initialize the DRAM, detect its size, 
>>>>>>>>> and pass
>>>>>>>>> the detected DRAM configuration on to an OS.
>>>>>>>>
>>>>>>>> Hi Igor,
>>>>>>>>
>>>>>>>> I do not think there have been any discussions on this (at least none
>>>>>>>> that I am aware of).
>>>>>>>>
>>>>>>>> Some boards (like Zynq and ZynqMP ones) are using
>>>>>>>> CONFIG_SYS_SDRAM_SIZE to define the amount of memory that is available
>>>>>>>> (since detection is not possible).
>>>>>>>
>>>>>>> May I ask why is detection not possible on these boards (may be SoCs)?
>>>>>>
>>>>>> That is correct, Zynq and ZynqMP are ARM SoCs. Which in the majority
>>>>>> of cases are used in boards which have fixed memory device setups
>>>>>> (without any SPD or equivalent).
>>>>>
>>>>> For example zcu102 zynqmp development board is using modules with SPD on
>>>>> it but if you look at generic SPD support then you will find out that
>>>>> FSL(drivers/ddr/fsl) is one of the major platform which is using it for
>>>>> ddr size detection.
>>>>> Anyway as Nathan wrote above the majority of boards with zynq/zynqmp
>>>>> devices need to be configured at build time or at run time by DTB.
>>>>>
>>>>> There is also topic.nl boards which contain ddr configuration the same
>>>>> for different ddr sizes and Mike sent this patch to get it work
>>>>> http://lists.denx.de/pipermail/u-boot/2016-November/273385.html
>>>>
>>>> That's exactly my point. I think Mike's patch does a way better job
>>>> detecting at run time than any compiled in or a DT based pseudo 
>>>> detection...
>>>>
>>>>>
>>>>> Anyway in general there are some ways how to configure DDR size
>>>>> - build time setup (CONFIG_SYS_SDRAM*)
>>>>> - run time setup
>>>>>   - ddr detection
>>>>>   - via SPD (like FSL)
>>>>>   - via algorithm (like topic.nl)
>>>>>   - configuration
>>>>>   - read DTB
>>>>>
>>>>> Nathan's path is trying to address last run time DTB configuration
>>>>> method to be shared across platforms because similar stuff Uniphier is
>>>>> using too. And it doesn't make sense to copy that functions everywhere
>>>>> that's why this should go core parts.
>>>>
>>>> Yep. That's exactly what I thought. I see Nathan's patch set as an
>>>> improvement of the current situation anyway.
>>>>
>>>> Also I think Mike's patch does a much better job on this.
>>>>
>>>
>>> Just keep in your mind just in case that you know that your
>>> configuration supports it. If you don't have DDR connected to hard block
>>> and you use ddr to PL you don't even know where to look for DDR.
>>> And with arm64 it is pretty huge space.
>>
>> I see this as exactly the type of information that should be provided by
>> the board code or a dtb.
> 
> I tend to remove all board files for zynq/zynqmp targets. Will see how
> we can do it in future.

This is not really related to current thread...
I'm not sure how this can be done... We're in a bootloader world here...
It should be board specific by definition...
There should be board specific code (not static data) somewhere, and
I think we had agreed a year or so ago... on this meter.
I think if you go this way, we will end up having board specific middleware
all around... and lots of tools for changing the static data (e.g. dtbs).


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

Re: [U-Boot] [PATCH 0/3] Add generic FDT memory bank decoding and gd initialization

[Igor Grinberg]

On 12/12/16 10:27, Michal Simek wrote:
> On 12.12.2016 09:24, Igor Grinberg wrote:
>> On 12/12/16 10:02, Michal Simek wrote:
>>> On 12.12.2016 08:13, Nathan Rossi wrote:
>>>> On 12 December 2016 at 03:11, Igor Grinberg <grinb...@compulab.co.il> 
>>>> wrote:
>>>>> dropping the list for this one as the question seems to me irrelevant to 
>>>>> your patch set.
>>>>>
>>>>> On 12/11/16 18:47, Nathan Rossi wrote:
>>>>>> On 12 December 2016 at 01:08, Igor Grinberg <grinb...@compulab.co.il> 
>>>>>> wrote:
>>>>>>> Hi Nathan,
>>>>>>>
>>>>>>> On 12/11/16 15:58, Nathan Rossi wrote:
>>>>>>>> This series adds two functions for handling the memory bank decoding 
>>>>>>>> and
>>>>>>>> initialization of global data for use by boards in their dram_init and
>>>>>>>> dram_init_banksize functions.
>>>>>>>
>>>>>>> I might have missed some discussions on this meter,
>>>>>>> can you please provide the use cases for this?
>>>>>>> IMO, the bootloader's job is to initialize the DRAM, detect its size, 
>>>>>>> and pass
>>>>>>> the detected DRAM configuration on to an OS.
>>>>>>
>>>>>> Hi Igor,
>>>>>>
>>>>>> I do not think there have been any discussions on this (at least none
>>>>>> that I am aware of).
>>>>>>
>>>>>> Some boards (like Zynq and ZynqMP ones) are using
>>>>>> CONFIG_SYS_SDRAM_SIZE to define the amount of memory that is available
>>>>>> (since detection is not possible).
>>>>>
>>>>> May I ask why is detection not possible on these boards (may be SoCs)?
>>>>
>>>> That is correct, Zynq and ZynqMP are ARM SoCs. Which in the majority
>>>> of cases are used in boards which have fixed memory device setups
>>>> (without any SPD or equivalent).
>>>
>>> For example zcu102 zynqmp development board is using modules with SPD on
>>> it but if you look at generic SPD support then you will find out that
>>> FSL(drivers/ddr/fsl) is one of the major platform which is using it for
>>> ddr size detection.
>>> Anyway as Nathan wrote above the majority of boards with zynq/zynqmp
>>> devices need to be configured at build time or at run time by DTB.
>>>
>>> There is also topic.nl boards which contain ddr configuration the same
>>> for different ddr sizes and Mike sent this patch to get it work
>>> http://lists.denx.de/pipermail/u-boot/2016-November/273385.html
>>
>> That's exactly my point. I think Mike's patch does a way better job
>> detecting at run time than any compiled in or a DT based pseudo detection...
>>
>>>
>>> Anyway in general there are some ways how to configure DDR size
>>> - build time setup (CONFIG_SYS_SDRAM*)
>>> - run time setup
>>> - ddr detection
>>> - via SPD (like FSL)
>>> - via algorithm (like topic.nl)
>>> - configuration
>>> - read DTB
>>>
>>> Nathan's path is trying to address last run time DTB configuration
>>> method to be shared across platforms because similar stuff Uniphier is
>>> using too. And it doesn't make sense to copy that functions everywhere
>>> that's why this should go core parts.
>>
>> Yep. That's exactly what I thought. I see Nathan's patch set as an
>> improvement of the current situation anyway.
>>
>> Also I think Mike's patch does a much better job on this.
>>
> 
> Just keep in your mind just in case that you know that your
> configuration supports it. If you don't have DDR connected to hard block
> and you use ddr to PL you don't even know where to look for DDR.
> And with arm64 it is pretty huge space.

I see this as exactly the type of information that should be provided by
the board code or a dtb.

> It means we really have to provide ways how to do it and these patches
> are just generic way how to do it via reading DTB instead of code
> duplication (+ bug) which we have now.

No doubt, Nathan's patch improves the situation.

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

Re: [U-Boot] [PATCH 0/3] Add generic FDT memory bank decoding and gd initialization

[Igor Grinberg]

On 12/12/16 10:18, Michal Simek wrote:
> On 12.12.2016 09:05, Igor Grinberg wrote:
>> On 12/12/16 09:13, Nathan Rossi wrote:
>>> On 12 December 2016 at 03:11, Igor Grinberg <grinb...@compulab.co.il> wrote:
>>>> dropping the list for this one as the question seems to me irrelevant to 
>>>> your patch set.
>>>>
>>>> On 12/11/16 18:47, Nathan Rossi wrote:
>>>>> On 12 December 2016 at 01:08, Igor Grinberg <grinb...@compulab.co.il> 
>>>>> wrote:
>>>>>> Hi Nathan,
>>>>>>
>>>>>> On 12/11/16 15:58, Nathan Rossi wrote:
>>>>>>> This series adds two functions for handling the memory bank decoding and
>>>>>>> initialization of global data for use by boards in their dram_init and
>>>>>>> dram_init_banksize functions.
>>>>>>
>>>>>> I might have missed some discussions on this meter,
>>>>>> can you please provide the use cases for this?
>>>>>> IMO, the bootloader's job is to initialize the DRAM, detect its size, 
>>>>>> and pass
>>>>>> the detected DRAM configuration on to an OS.
>>>>>
>>>>> Hi Igor,
>>>>>
>>>>> I do not think there have been any discussions on this (at least none
>>>>> that I am aware of).
>>>>>
>>>>> Some boards (like Zynq and ZynqMP ones) are using
>>>>> CONFIG_SYS_SDRAM_SIZE to define the amount of memory that is available
>>>>> (since detection is not possible).
>>>>
>>>> May I ask why is detection not possible on these boards (may be SoCs)?
>>>
>>> That is correct, Zynq and ZynqMP are ARM SoCs. Which in the majority
>>> of cases are used in boards which have fixed memory device setups
>>> (without any SPD or equivalent).
>>
>> Ok. That is understood. Yet, it does not explain why the detection
>> cannot be done.. For example, you know which memory device setups you
>> _can_ have on the board, so the detection is just to figure out which
>> one is assembled. We are doing this on i.MXes, OMAPs, PXAs, and others.
>>
>> I was working on many ARM boards w/o SPD and still we almost always develop
>> a detection mechanism which has some assumptions and knowledge of the board
>> but it is much better then using some static data like compiled in or a dtb.
>>
>> Have you considered a detection mechanism at all?
> 
> If you look at my previous email as you see that topic.nl is using this.

No, I sent it before seeing the email and Mike's patches.

> 
> But the question is if this will work with all cases or just for
> particular configuration. All zynq/zynqmp boards requires initial
> setting which is created in Xilinx design tools. Export for these uniq
> configurations are in ps7_init* or psu_init* files where DDR
> configuration is part of this.

Well, I think the board can provide the configuration as the board maintainer
probably knows it. I can be with the vendor provided tools or some other ways.
Once the configuration is provided, a common code should be able to do
the detection.

> 
> Devices contain various configurations for various memory types. Also
> there is an option to add memory controller to programmable logic and
> use it.
> At the end of the day we won't be able to find out generic way for all
> zynq/zynqmp boards which will simply work everywhere.

That's sounds correct. You can abstract the configuration process
and each board should provide its configuration data.

> 
> Just a summary of this. If you have one line of products which you have
> tested and you know how they work then topic.nl solution is a way to go.
> But I don't think I want to risk to have this only one method for all
> zynq/zynqmp SoC.

I don't think you need to.

> 
> Maybe thinking a little bit to the future. U-Boot is using linked-lists
> more than before and we could provide all options as I described (and
> maybe more) call them in loop. Limit this via Kconfig etc.

Yep. All the above sounds sane to me.


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

Re: [U-Boot] [PATCH v4 2/2] splash: add support for loading splash from a FIT image

[Igor Grinberg]

On 12/11/16 22:27, Simon Glass wrote:
> Hi Igor,
> 
> On 11 December 2016 at 10:37, Igor Grinberg <grinb...@compulab.co.il> wrote:
>> Hi Tomas, Simon,
>>
>> Sorry, to break in that late...
>> I have a quick question below.
>>
>> On 12/05/16 09:36, Tomas Melin wrote:
>>> Enable support for loading a splash image from within a FIT image.
>>> The image is assumed to be generated with mkimage -E flag to hold
>>> the data external to the FIT.
>>>
>>> Signed-off-by: Tomas Melin <tomas.me...@vaisala.com>
>>
>> [...]
>>
>>> diff --git a/common/splash_source.c b/common/splash_source.c
>>> index 70d724f..94b46d3 100644
>>> --- a/common/splash_source.c
>>> +++ b/common/splash_source.c
>>
>> [...]
>>
>>> +#ifdef CONFIG_FIT
>>> +static int splash_load_fit(struct splash_location *location, u32 
>>> bmp_load_addr)
>>> +{
>>> + int res;
>>> + int node_offset;
>>> + int splash_offset;
>>> + int splash_size;
>>> + struct image_header *img_header;
>>> + const u32 *fit_header;
>>> + u32 fit_size;
>>> + const size_t header_size = sizeof(struct image_header);
>>> +
>>> + /* Read in image header */
>>> + res = splash_storage_read_raw(location, bmp_load_addr, header_size);
>>> + if (res < 0)
>>> + return res;
>>> +
>>> + img_header = (struct image_header *)bmp_load_addr;
>>> + fit_size = fdt_totalsize(img_header);
>>> +
>>> + /* Read in entire FIT */
>>> + fit_header = (const u32 *)(bmp_load_addr + header_size);
>>> + res = splash_storage_read_raw(location, (u32)fit_header, fit_size);
>>> + if (res < 0)
>>> + return res;
>>> +
>>> + res = fit_check_format(fit_header);
>>> + if (!res) {
>>> + debug("Could not find valid FIT image\n");
>>> + return -EINVAL;
>>> + }
>>> +
>>> + node_offset = fit_image_get_node(fit_header, location->name);
>>> + if (node_offset < 0) {
>>> + debug("Could not find splash image '%s' in FIT\n",
>>> +   location->name);
>>> + return -ENOENT;
>>> + }
>>> +
>>
>> I think two above debug() are very legitimate - no need to shout if no FIT 
>> image
>> or no splash in it...
>>
>>> + res = fit_image_get_data_offset(fit_header, node_offset,
>>> + _offset);
>>> + if (res < 0) {
>>> + debug("Could not find 'data-offset' property in FIT\n");
>>> + return res;
>>> + }
>>> +
>>> + res = fit_image_get_data_size(fit_header, node_offset, _size);
>>> + if (res < 0) {
>>> + debug("Could not find 'data-size' property in FIT\n");
>>> + return res;
>>> + }
>>
>> Now regarding these two, I'm not sure.
>> Since we have found a valid FIT and also a node with a correct splash name,
>> probably the intent is that we show the splash, right?
>> But in the two above checks, we find inconsistencies that do not allow us to
>> show the splash - meaning the FIT is not actually good (am I right here?).
>> So may be we should report it to the 'user' and allow correcting the FIT?
>> Otherwise, it is impossible to debug the image w/o a debug version of 
>> U-Boot...
>> Do I make sense, or do I miss something?
> 
> Yes that makes some sense, but the problem is that then you are
> including error messages always which would never happen in a working
> system (i.e. it just bloats the code).

Unless, there a kind of corruption or a user mistake and then that same
user can't even understand what happened because we do not help him with
an error message.
You cannot know that these error messages will never happen...
This is a generic code which can be used by a wide variety of platforms - 
I don't think you can foresee all the possible use cases.

We are talking about several tens of bytes vs. usability.
If there is an error, it should be stated as such. It should not just
exit silently...

I think that debug() is a good thing to use, but we shouldn't be obsessed
with it.

> 
> So long as the error is reported (even if it is not a very specific
> error), people can add DEBUG and track it down.

That depends who 'people' are? Devs? Users?


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

Re: [U-Boot] [PATCH 0/3] Add generic FDT memory bank decoding and gd initialization

[Igor Grinberg]

On 12/12/16 10:02, Michal Simek wrote:
> On 12.12.2016 08:13, Nathan Rossi wrote:
>> On 12 December 2016 at 03:11, Igor Grinberg <grinb...@compulab.co.il> wrote:
>>> dropping the list for this one as the question seems to me irrelevant to 
>>> your patch set.
>>>
>>> On 12/11/16 18:47, Nathan Rossi wrote:
>>>> On 12 December 2016 at 01:08, Igor Grinberg <grinb...@compulab.co.il> 
>>>> wrote:
>>>>> Hi Nathan,
>>>>>
>>>>> On 12/11/16 15:58, Nathan Rossi wrote:
>>>>>> This series adds two functions for handling the memory bank decoding and
>>>>>> initialization of global data for use by boards in their dram_init and
>>>>>> dram_init_banksize functions.
>>>>>
>>>>> I might have missed some discussions on this meter,
>>>>> can you please provide the use cases for this?
>>>>> IMO, the bootloader's job is to initialize the DRAM, detect its size, and 
>>>>> pass
>>>>> the detected DRAM configuration on to an OS.
>>>>
>>>> Hi Igor,
>>>>
>>>> I do not think there have been any discussions on this (at least none
>>>> that I am aware of).
>>>>
>>>> Some boards (like Zynq and ZynqMP ones) are using
>>>> CONFIG_SYS_SDRAM_SIZE to define the amount of memory that is available
>>>> (since detection is not possible).
>>>
>>> May I ask why is detection not possible on these boards (may be SoCs)?
>>
>> That is correct, Zynq and ZynqMP are ARM SoCs. Which in the majority
>> of cases are used in boards which have fixed memory device setups
>> (without any SPD or equivalent).
> 
> For example zcu102 zynqmp development board is using modules with SPD on
> it but if you look at generic SPD support then you will find out that
> FSL(drivers/ddr/fsl) is one of the major platform which is using it for
> ddr size detection.
> Anyway as Nathan wrote above the majority of boards with zynq/zynqmp
> devices need to be configured at build time or at run time by DTB.
> 
> There is also topic.nl boards which contain ddr configuration the same
> for different ddr sizes and Mike sent this patch to get it work
> http://lists.denx.de/pipermail/u-boot/2016-November/273385.html

That's exactly my point. I think Mike's patch does a way better job
detecting at run time than any compiled in or a DT based pseudo detection...

> 
> Anyway in general there are some ways how to configure DDR size
> - build time setup (CONFIG_SYS_SDRAM*)
> - run time setup
>   - ddr detection
>   - via SPD (like FSL)
>   - via algorithm (like topic.nl)
>   - configuration
>   - read DTB
> 
> Nathan's path is trying to address last run time DTB configuration
> method to be shared across platforms because similar stuff Uniphier is
> using too. And it doesn't make sense to copy that functions everywhere
> that's why this should go core parts.

Yep. That's exactly what I thought. I see Nathan's patch set as an
improvement of the current situation anyway.

Also I think Mike's patch does a much better job on this.

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

Re: [U-Boot] [PATCH] board: topic: Detect RAM size at boot

[Igor Grinberg]

On 11/22/16 16:54, Mike Looijmans wrote:
> On 22-11-16 12:00, Michal Simek wrote:
>> On 21.11.2016 09:30, Mike Looijmans wrote:
>>> Miami boards can have memory sizes of 256M, 512M or 1GB. To
>>> prevent requiring separate bootloaders for each variant, just
>>> detect the RAM size at boot time instead of relying on the
>>> devicetree information.
>>> 
>>> Signed-off-by: Mike Looijmans  --- 
>>> board/topic/zynq/board.c  | 39
>>> +++ 
>>> configs/topic_miami_defconfig |  1 + 
>>> configs/topic_miamiplus_defconfig |  1 + 3 files changed, 41
>>> insertions(+)
>>> 
>>> diff --git a/board/topic/zynq/board.c b/board/topic/zynq/board.c 
>>> index a95c9d1..8a5765e 100644 --- a/board/topic/zynq/board.c +++
>>> b/board/topic/zynq/board.c @@ -1 +1,40 @@ +/* + * (C) Copyright
>>> 2016 Topic Embedded Products + * + * SPDX-License-Identifier:
>>> GPL-2.0+ + */ + +/* + * Miami boards can have memory sizes of
>>> 256M, 512M or 1GB. To prevent needing + * separate bootloaders
>>> for each variant, just detect the RAM size at boot time + *
>>> instead of relying on the devicetree information. + */ +#define
>>> CONFIG_SYS_SDRAM_BASE0 +#define CONFIG_SYS_SDRAM_SIZE
>>> topic_get_sdram_size()
>> 
>> I am not happy with this but I see where you go.
> 
> Of the several options I tried to "overload" the memory init
> functions, this was basically the lesser evil...
> 
> Maybe it would be possible to make some changes to the generic Zynq
> board.c file to facilitate overlaying the code?
> 
> Note that the memory detection would work on any board, not just the
> Topic boards. I agree that devicetree is considered the best thing
> since frozen pizza, but for the Zynq it looks as though detection is
> much simpler than static configuration.
> 
>> 
>>> +#define CONFIG_SYS_SDRAM_SIZE_MAX 0x4000u + +static unsigned
>>> int topic_get_sdram_size(void); + #include
>>> "../../xilinx/zynq/board.c" + +#include  + +int
>>> ft_board_setup(void *blob, bd_t *bd) +{ +
>>> fdt_fixup_memory(blob, (u64)CONFIG_SYS_SDRAM_BASE,
>>> (u64)gd->ram_size); + +return 0; +}
>> 
>> This action is taken at arch_fixup_fdt(). Can you please confirm
>> that this is really needed? And it is not done there? That you
>> don't duplicate stuff here.
> 
> If I omitted this step, the Linux devicetree would not be adjusted
> (during bootm) and would still report its default. I needed both
> arch_fixup_fdt and this extra line to properly adjust the Linux
> devicetree for booting.
> 
> 
>> 
>>> + +void dram_init_banksize(void) +{ +gd->bd->bi_dram[0].start
>>> = CONFIG_SYS_SDRAM_BASE; +gd->bd->bi_dram[0].size =
>>> gd->ram_size; +}
>> 
>> This should be fine.
>> 
> 
> A note here, if you compile for Zynq with  CONFIG_SYS_SDRAM_BASE and
> CONFIG_SYS_SDRAM_SIZE set, there will be no implementation of
> 'dram_init_banksize' and the system will fail to boot.
> 
> It might be wise to put this snippet into the zynq/board.c in the
> "#else" clause. I'll send a separate patch for that if you agree.
> 
> 
>>> + +unsigned int topic_get_sdram_size(void) +{ +/* Detect and
>>> fix ram size */ +return get_ram_size((void
>>> *)CONFIG_SYS_SDRAM_BASE, +
>>> CONFIG_SYS_SDRAM_SIZE_MAX);
>> 
>> In the first look I didn't know that get_ram_size is u-boot
>> function for memsize detection.
> 
> Indeed, the name "get_ram_size" doesn't really match what it actually
> does, but the function's documentation is quite good.
> 
> I found "get_ram_size" and "ft_board_setup" while browsing the code
> of other boards to see how they handled dynamic configuration.

+1

That's how things works in U-Boot for years...

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

Re: [U-Boot] [PATCH 0/3] Add generic FDT memory bank decoding and gd initialization

[Igor Grinberg]

On 12/11/16 18:47, Nathan Rossi wrote:
> On 12 December 2016 at 01:08, Igor Grinberg <grinb...@compulab.co.il> wrote:
>> Hi Nathan,
>>
>> On 12/11/16 15:58, Nathan Rossi wrote:
>>> This series adds two functions for handling the memory bank decoding and
>>> initialization of global data for use by boards in their dram_init and
>>> dram_init_banksize functions.
>>
>> I might have missed some discussions on this meter,
>> can you please provide the use cases for this?
>> IMO, the bootloader's job is to initialize the DRAM, detect its size, and 
>> pass
>> the detected DRAM configuration on to an OS.
> 
> Hi Igor,
> 
> I do not think there have been any discussions on this (at least none
> that I am aware of).
> 
> Some boards (like Zynq and ZynqMP ones) are using
> CONFIG_SYS_SDRAM_SIZE to define the amount of memory that is available
> (since detection is not possible). However with the introduction of
> dtbs for some boards they are also capable of loading the size of
> memory from the embedded/appended dtb (instead of hardcoded). This
> allows for more of the board config to be loaded from the device tree
> instead of from include/configs/*.h. This however is up to the
> individual board to implement in its dram_init* functions.

Thanks for the explanation!
I assume that the key point is "detection is not possible" and therefore
we must rely on a user or a production process to place (append) the correct 
dtb.
Makes sense to me now and looks like an improvement to the current situation.

> 
> The first patch of the series is only adding some decoding helper
> functions to make this generic between the Zynq and ZynqMP boards as
> well as to allow for any other boards that may want to use the same
> mechanism to get the memory size from the fdt. There is no requirement
> for boards to use these functions.

Can you please next time place a similar explanation in at least the cover
letter. This way, the intent might be understood the first time ;-)
I would also like to see some parts of the above explanation in the functions
documentation (e.g. this allows to improve the DRAM configuration mechanics
on boards that cannot detect its DRAM size/config).

Thanks!

> 
> Regards,
> Nathan
> 
>>
>>>
>>> The series also changes the zynq and zynqmp board implementations to use
>>> these functions to resolve a issue with static variable use.
>>>
>>> Nathan Rossi (3):
>>>   fdt: add memory bank decoding functions for board setup
>>>   ARM: zynq: Replace board specific with generic memory bank decoding
>>>   ARM64: zynqmp: Replace board specific with generic memory bank
>>> decoding
>>>
>>>  board/xilinx/zynq/board.c| 112 
>>> ++-
>>>  board/xilinx/zynqmp/zynqmp.c | 112 
>>> ++-
>>>  include/fdtdec.h |  25 ++
>>>  lib/fdtdec.c |  54 +
>>>  4 files changed, 85 insertions(+), 218 deletions(-)
>>>
>>
>> --
>> Regards,
>> Igor.
> 

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

Re: [U-Boot] [PATCH v4 2/2] splash: add support for loading splash from a FIT image

[Igor Grinberg]

Hi Tomas, Simon,

Sorry, to break in that late...
I have a quick question below.

On 12/05/16 09:36, Tomas Melin wrote:
> Enable support for loading a splash image from within a FIT image.
> The image is assumed to be generated with mkimage -E flag to hold
> the data external to the FIT.
> 
> Signed-off-by: Tomas Melin 

[...]

> diff --git a/common/splash_source.c b/common/splash_source.c
> index 70d724f..94b46d3 100644
> --- a/common/splash_source.c
> +++ b/common/splash_source.c

[...]

> +#ifdef CONFIG_FIT
> +static int splash_load_fit(struct splash_location *location, u32 
> bmp_load_addr)
> +{
> + int res;
> + int node_offset;
> + int splash_offset;
> + int splash_size;
> + struct image_header *img_header;
> + const u32 *fit_header;
> + u32 fit_size;
> + const size_t header_size = sizeof(struct image_header);
> +
> + /* Read in image header */
> + res = splash_storage_read_raw(location, bmp_load_addr, header_size);
> + if (res < 0)
> + return res;
> +
> + img_header = (struct image_header *)bmp_load_addr;
> + fit_size = fdt_totalsize(img_header);
> +
> + /* Read in entire FIT */
> + fit_header = (const u32 *)(bmp_load_addr + header_size);
> + res = splash_storage_read_raw(location, (u32)fit_header, fit_size);
> + if (res < 0)
> + return res;
> +
> + res = fit_check_format(fit_header);
> + if (!res) {
> + debug("Could not find valid FIT image\n");
> + return -EINVAL;
> + }
> +
> + node_offset = fit_image_get_node(fit_header, location->name);
> + if (node_offset < 0) {
> + debug("Could not find splash image '%s' in FIT\n",
> +   location->name);
> + return -ENOENT;
> + }
> +

I think two above debug() are very legitimate - no need to shout if no FIT image
or no splash in it...

> + res = fit_image_get_data_offset(fit_header, node_offset,
> + _offset);
> + if (res < 0) {
> + debug("Could not find 'data-offset' property in FIT\n");
> + return res;
> + }
> +
> + res = fit_image_get_data_size(fit_header, node_offset, _size);
> + if (res < 0) {
> + debug("Could not find 'data-size' property in FIT\n");
> + return res;
> + }

Now regarding these two, I'm not sure.
Since we have found a valid FIT and also a node with a correct splash name,
probably the intent is that we show the splash, right?
But in the two above checks, we find inconsistencies that do not allow us to
show the splash - meaning the FIT is not actually good (am I right here?).
So may be we should report it to the 'user' and allow correcting the FIT?
Otherwise, it is impossible to debug the image w/o a debug version of U-Boot...
Do I make sense, or do I miss something?

> +
> + /* Align data offset to 4-byte boundrary */
> + fit_size = fdt_totalsize(fit_header);
> + fit_size = (fit_size + 3) & ~3;
> +
> + /* Read in the splash data */
> + location->offset = (location->offset + fit_size + splash_offset);
> + res = splash_storage_read_raw(location, bmp_load_addr , splash_size);
> + if (res < 0)
> + return res;
> +
> + return 0;
> +}
> +#endif /* CONFIG_FIT */
> +
>  /**
>   * splash_source_load - load splash image from a supported location.
>   *
> @@ -277,5 +344,9 @@ int splash_source_load(struct splash_location *locations, 
> uint size)
>   return splash_load_raw(splash_location, bmp_load_addr);
>   else if (splash_location->flags == SPLASH_STORAGE_FS)
>   return splash_load_fs(splash_location, bmp_load_addr);
> +#ifdef CONFIG_FIT
> + else if (splash_location->flags == SPLASH_STORAGE_FIT)
> + return splash_load_fit(splash_location, bmp_load_addr);
> +#endif
>   return -EINVAL;
>  }
> diff --git a/doc/README.splashprepare b/doc/README.splashprepare
> index 56c1bef..f1418de 100644
> --- a/doc/README.splashprepare
> +++ b/doc/README.splashprepare
> @@ -5,7 +5,7 @@ The splash_screen_prepare() function is a weak function 
> defined in
>  common/splash.c. It is called as part of the splash screen display
>  sequence. It gives the board an opportunity to prepare the splash
>  image data before it is processed and sent to the frame buffer by
> -U-Boot.  Define your own version to use this feature.
> +U-Boot. Define your own version to use this feature.
>  
>  CONFIG_SPLASH_SOURCE
>  
> @@ -20,7 +20,12 @@ splashsource works as follows:
>  - If splashsource is undefined, use the first splash location as default.
>  - If splashsource is set to an unsupported value, do not load a splash 
> screen.
>  
> -A splash source location can describe either storage with raw data, or 
> storage
> -formatted with a file system. In case of a filesystem, the splash screen 
> data is
> -loaded as a file. The name of the splash screen file can be controlled with 
> the
> 

Re: [U-Boot] [PATCH 0/3] Add generic FDT memory bank decoding and gd initialization

[Igor Grinberg]

Hi Nathan,

On 12/11/16 15:58, Nathan Rossi wrote:
> This series adds two functions for handling the memory bank decoding and
> initialization of global data for use by boards in their dram_init and
> dram_init_banksize functions.

I might have missed some discussions on this meter,
can you please provide the use cases for this?
IMO, the bootloader's job is to initialize the DRAM, detect its size, and pass
the detected DRAM configuration on to an OS.

> 
> The series also changes the zynq and zynqmp board implementations to use
> these functions to resolve a issue with static variable use.
> 
> Nathan Rossi (3):
>   fdt: add memory bank decoding functions for board setup
>   ARM: zynq: Replace board specific with generic memory bank decoding
>   ARM64: zynqmp: Replace board specific with generic memory bank
> decoding
> 
>  board/xilinx/zynq/board.c| 112 
> ++-
>  board/xilinx/zynqmp/zynqmp.c | 112 
> ++-
>  include/fdtdec.h |  25 ++
>  lib/fdtdec.c |  54 +
>  4 files changed, 85 insertions(+), 218 deletions(-)
> 

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

Re: [U-Boot] [PATCH 0/3] SPL: move CONFIG_SPL_*_LOAD to Kconfig

[Igor Grinberg]

Hi Tom,

On 12/06/16 16:32, Tom Rini wrote:
> On Tue, Dec 06, 2016 at 02:48:27PM +0100, Fabien Parent wrote:
> 
>> Moving to CONFIG_SPL_*_LOAD options to Kconfig offers several advantage:
>>  * simpler config headers
>>  * on some boards we can easily switch to another boot media without needing
>>to modify the config headers.
>>
>> This series fixes an issue in davinci where a wrong option was used in place
>> of a CONFIG_SPL_*_LOAD option, then move the options to Kconfig options, and
>> finally start using these Kconfig options for the OMAPL138-LCDK board.
>>
>> Fabien Parent (3):
>>   davinci: spl: use correct macro to select boot device
>>   SPL: create Kconfig options for CONFIG_SPL_*_LOAD
>>   davinci: omapl138_lcdk: use new CONFIG_SPL_*_LOAD Kconfig options
>>
>>  arch/arm/mach-davinci/spl.c |  2 +-
>>  common/spl/Kconfig  | 25 +
>>  configs/omapl138_lcdk_defconfig |  1 +
>>  include/configs/omapl138_lcdk.h |  2 --
>>  scripts/config_whitelist.txt|  3 ---
>>  5 files changed, 27 insertions(+), 6 deletions(-)
> 
> So, I think this shows that some of the SPL framework needs to be
> revisited for davinci.  First, lets make it clear what
> CONFIG_SPL_{SPI,NAND,MMC}_LOAD is doing.
> 
> CONFIG_SPL_MMC_LOAD is used to flag that on davinci we're loading U-Boot
> from MMC.  It's not set / used today but I assume it was working when I
> introduced all of this.
> 
> CONFIG_SPL_NAND_LOAD is used for two things.  First, it is used to flag
> that on davinci we're loading U-Boot from NAND.  Second, it is used to
> enable the non-SPL_FRAMEWORK NAND driver
> (drivers/mtd/nand/nand_spl_load.c).  This driver is not used on davinci.
> 
> CONFIG_SPL_SPI_LOAD is used for two things.  First, it is used to flag
> that on davinci we're loading U-Boot from SPI flash.  Second, it used
> globally to enable common/spl/spl_spi.c.
> 
> NAND boot is done here via CONFIG_SPL_NAND_SIMPLE which is the regular
> SPL framework based NAND driver (drivers/mtd/nand/nand_spl_simple.c).
> This also means that the patch to update CONFIG_SYS_NAND_U_BOOT_SIZE was
> not needed since we don't use that driver.
> 
> Now, I think that in retrospect
> arch/arm/mach-davinci/spl.c::spl_boot_device could be re-worked to key
> off of CONFIG_SPL_NAND_SIMPLE / CONFIG_SPL_SPI_SUPPORT / SPL_MMC_SUPPORT.
> 
> And a good but possibly complex series would be to consolidate the usage
> of SPL_SPI_SUPPORT, SPL_SPI_FLASH_SUPPORT and SPL_SPI_LOAD just in to
> SPL_SPI_SUPPORT.  I'll probably try and do this myself as there's a ton
> of build testing and size checking to make sure nothing odd breaks here
> to do.

Just a thought...
Are you sure you want to combine all three (spi, spi flash, and spi load)
under one define?
Won't be there any case for parsing some spi device (say eeprom) in the SPL,
but no spi flash to load U-Boot from?
It might be sensible to keep the "spi flash" and the "spi load" together, but
I think it might be more beneficial to keep the spi bus support apart.

-- 
Regards,
Igor.



signature.asc
Description: OpenPGP digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCHv4] Retrieve MAC address from EEPROM

[Igor Grinberg]

Added Nikita to Cc.

On 11/28/16 12:45, Olliver Schinagl wrote:
> On 28-11-16 10:13, Igor Grinberg wrote:
>> Hi Olliver,
>>
>> On 11/25/16 17:30, Olliver Schinagl wrote:
>>
>> [...]
>>
>>> The current idea of the eeprom layout, is to skip the first 8 bytes, so that
>>> other information can be stored there if needed, for example a header with 
>>> some
>>> magic to identify the EEPROM. Or equivalent purposes.
>>>
>>> After those 8 bytes the MAC address follows the first macaddress. The 
>>> macaddress
>>> is appended by a CRC8 byte and then padded to make for nice 8 bytes. 
>>> Following
>>> the first macaddress one can store a second, or a third etc etc mac address.
>>>
>>> The CRC8 is optional (via a define) but is strongly recommended to have. It
>>> helps preventing user error and more importantly, checks if the bytes read 
>>> are
>>> actually a user inserted address. E.g. only writing 1 macaddress into the 
>>> eeprom
>>> but trying to consume 2.
>> While reading the above, I'm wondering, have you considered the eeprom layout
>> feature that we have in: common/eeprom/... ?
> Last year, when starting this patch series, this did not really exist in so 
> far (iirc).
>>
>> The layout feature was actually designed for these tasks, but in a more 
>> generic
>> way then just Ethernet MAC address.
> I did see it and I was quite excited. I think a follow up patch should switch 
> over.

Sounds great!

> I did not yet use the new eeprom layout thing as I am hoping for Maxime's 
> patches to
> land first, where he makes the whole eeprom interfacing more generic.

That's interesting. I haven't been around for some time, are there any public 
patches
already? If so, can you please point where should I look at?

>>
>> What do you think?
> I'll have to look at the eeprom layout feature a little more in depth, the 
> one thing
> that was a little 'annoying' (from a short quick glance) was that the layout 
> was
> jumping around a bit (eth0, eth1, something else, eth2, eth3). But yes,
> I was quite intrigued herein.

Ohh.. you mean compulab layout? I see.
Compulab layout is already out there for 6 years in various devices.
It has gone through several versions, so to be backward compatible (and 
currently it is,
besides the legacy version), it had to do the "jumping" thing.
It is only last year Nikita has started to facilitate it a bit in upstream.
Anyway, the point is that eeprom layout in u-boot/common/eeprom/... should be 
generic
as a framework and any vendor/board can define their own layout in vendor/board 
location.
We are also going to extend the layout framework to provide more in-U-Boot APIs.

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

Re: [U-Boot] [PATCHv4] Retrieve MAC address from EEPROM

[Igor Grinberg]

Hi Olliver,

On 11/25/16 17:30, Olliver Schinagl wrote:

[...]

> 
> The current idea of the eeprom layout, is to skip the first 8 bytes, so that
> other information can be stored there if needed, for example a header with 
> some
> magic to identify the EEPROM. Or equivalent purposes.
> 
> After those 8 bytes the MAC address follows the first macaddress. The 
> macaddress
> is appended by a CRC8 byte and then padded to make for nice 8 bytes. Following
> the first macaddress one can store a second, or a third etc etc mac address.
> 
> The CRC8 is optional (via a define) but is strongly recommended to have. It
> helps preventing user error and more importantly, checks if the bytes read are
> actually a user inserted address. E.g. only writing 1 macaddress into the 
> eeprom
> but trying to consume 2.

While reading the above, I'm wondering, have you considered the eeprom layout
feature that we have in: common/eeprom/... ?

The layout feature was actually designed for these tasks, but in a more generic
way then just Ethernet MAC address.

What do you think?


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

Re: [U-Boot] [PATCH v2 4/4] ARM: configs: cm_fx6: add mtd support

[Igor Grinberg]

On 07/13/2016 12:37 AM, christopher.spinr...@rwth-aachen.de wrote:
> From: Christopher Spinrath <christopher.spinr...@rwth-aachen.de>
> 
> The cm-fx6 module has an on-board spi flash chip. Enable mtd support
> and the mtdparts command. Also define a default partitioning, add
> it to the default environment, and enable support to overwrite the
> partitioning defined in a device tree by it. Finally, probe for the
> chip on preboot to register the flash chip and, thus, establish the
> connection between the mtd environment settings and the actual device.
> 
> These changes move the effective default partitioning from the device
> tree shipped with the vendor kernels to U-Boot which becomes the single
> point of definition for the partitioning for all device tree based
> kernels (in particular, for the upstream Linux kernel which does not
> have a default partitioning defined in its device tree).
> 
> Signed-off-by: Christopher Spinrath <christopher.spinr...@rwth-aachen.de>

Acked-by: Igor Grinberg <grinb...@compulab.co.il>

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

Re: [U-Boot] [PATCH v2 3/4] ARM: board: cm_fx6: fixup mtd partitions in the fdt

[Igor Grinberg]

On 07/13/2016 12:37 AM, christopher.spinr...@rwth-aachen.de wrote:
> From: Christopher Spinrath <christopher.spinr...@rwth-aachen.de>
> 
> The cm-fx6 module has an on-board st,m25p compatible spi flash chip
> used for U-Boot (binary & environment). Overwrite the partitions in
> the device tree by the partition table provided in the mtdparts
> environment variable, if it is set.
> 
> This allows to specify a kernel independent partitioning in the
> environment and provides a convient way for the user to adapt the
> partition table.
> 
> Signed-off-by: Christopher Spinrath <christopher.spinr...@rwth-aachen.de>

Acked-by: Igor Grinberg <grinb...@compulab.co.il>

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

Re: [U-Boot] [PATCH v2 2/4] fdt_support: define stub for fdt_fixup_mtdparts

[Igor Grinberg]

On 07/13/2016 12:37 AM, christopher.spinr...@rwth-aachen.de wrote:
> From: Christopher Spinrath <christopher.spinr...@rwth-aachen.de>
> 
> Define an inline stub for fdt_fixup_mtdparts in the case that
> CONFIG_FDT_FIXUP_PARTITIONS is not defined. This avoids the need
> to guard every call to this function by a proper #ifdef in board
> files.
> 
> Signed-off-by: Christopher Spinrath <christopher.spinr...@rwth-aachen.de>

Thanks!

Reviewed-by: Igor Grinberg <grinb...@compulab.co.il>

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

Re: [U-Boot] [PATCH 3/3] ARM: configs: cm_fx6: add mtd support

[Igor Grinberg]

On 06/22/2016 10:27 PM, Christopher Spinrath wrote:
> Hi Igor,
> 
> On 06/22/2016 06:15 PM, Igor Grinberg wrote:
>> On 06/19/2016 06:44 PM, Christopher Spinrath wrote:
>>> The cm-fx6 module has an on-board spi flash chip. Enable mtd support
>>> and the mtdparts command. Also define a default partitioning, add
>>> it to the default environment, and enable support to overwrite the
>>> partitioning defined in a device tree by it.
>>>
>>> These changes move the effective default partitioning from the device
>>> tree shipped with the vendor kernels to u-boot which becomes the single
>>> point of definition for the partitioning for all device tree based
>>> kernels (in particular, for the upstream linux kernel which does not
>>> have a default partitioning defined in its device tree).
>>>
>>> Signed-off-by: Christopher Spinrath <christopher.spinr...@rwth-aachen.de>
>>> ---
>>>  include/configs/cm_fx6.h | 19 ++-
>>>  1 file changed, 18 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/configs/cm_fx6.h b/include/configs/cm_fx6.h
>>> index f054ca8..c839b03 100644
>>> --- a/include/configs/cm_fx6.h
>>> +++ b/include/configs/cm_fx6.h
>>
>> [...]
>>
>>> @@ -157,7 +174,7 @@
>>> "run setupnandboot;" \
>>> "run nandboot;"
>>>  
>>> -#define CONFIG_PREBOOT "usb start"
>>> +#define CONFIG_PREBOOT "usb start;sf probe"
>>
>> Probably, this is really needed.
>> Care to explain?
>>
> The sf probe command probes for the spi flash and registers (on success)
> the device as nor0. This device is used by mtdparts (cf. the mtdids
> variable; it maps the U-Boot name nor0 to the kernel name spi0.0) and
> the mtd fixup code in patch 2 (cf. the nodes array; it specifies the
> compatible of the flash chip of type NOR #0, i.e. nor0).
> 
> Without this all mtdparts commands will fail and the fixup code won't
> work because there is nor0 device.

Thanks for the explanation!
That sounds to me like this should go away once we have the DM in place
for spi flash and MTD (added Simon to Cc).
Meanwhile, may be a short notice in the commit message?

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

Re: [U-Boot] [PATCH 2/3] ARM: board: cm_fx6: fixup mtd partitions in the fdt

[Igor Grinberg]

Hi Christopher,

On 06/22/2016 10:21 PM, Christopher Spinrath wrote:
> Hi Igor,
> 
> On 06/22/2016 06:02 PM, Igor Grinberg wrote:
>> Hi Christopher,
>>
>> On 06/19/2016 06:44 PM, Christopher Spinrath wrote:
>>> The cm-fx6 module has an on-board st,m25p compatible spi flash chip
>>> used for u-boot (binary & environment). Overwrite the partitions in
>>> the device tree by the partition table provided in the mtdparts
>>> environment variable, if it is set.
>>>
>>> This allows to specify a kernel independent partitioning in the
>>> environment and provides a convient way for the user to adapt the
>>> partition table.
>>>
>>> Signed-off-by: Christopher Spinrath <christopher.spinr...@rwth-aachen.de>
>>> ---
>>>  board/compulab/cm_fx6/cm_fx6.c | 16 +++-
>>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/board/compulab/cm_fx6/cm_fx6.c b/board/compulab/cm_fx6/cm_fx6.c
>>> index 712057a..81a7ae2 100644
>>> --- a/board/compulab/cm_fx6/cm_fx6.c
>>> +++ b/board/compulab/cm_fx6/cm_fx6.c

[...]

>>> @@ -28,6 +29,7 @@
>>>  #include 
>>>  #include 
>>>  #include 
>>> +#include 
>>
>> Why is this needed?
>>
> The MTD_DEV_TYPE_NOR constant is defined in this header. I agree that it
> is a bit ugly but this header is used for the same purpose in other
> board files, too (e.g.board/pdm360ng/pdm360ng.c).

I see.
I don't feel right to request this in scope of these patches, but
if you can take care of that one (even in a follow up patch) - it would
be awesome.

[...]

>>> @@ -616,6 +626,10 @@ int ft_board_setup(void *blob, bd_t *bd)
>>>  NULL, 0, 1);
>>> }
>>>  
>>> +#ifdef CONFIG_FDT_FIXUP_PARTITIONS
>>> +   fdt_fixup_mtdparts(blob, nodes, ARRAY_SIZE(nodes));
>>> +#endif
>>
>> I really dislike the ifdeffery inside functions.
>> Care to introduce a stub for the !CONFIG_FDT_FIXUP_PARTITIONS case in
>> include/fdt_support.h for this one?
>>
> Sure. Is the header the correct place for this or should I add a #else
> case in the .c file?

IMO, the header file is better for stubbing things out.
It does not require you to add .c into compilation.
There are also already several examples inside this header.

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

Re: [U-Boot] [PATCH 2/3] ARM: board: cm_fx6: fixup mtd partitions in the fdt

[Igor Grinberg]

On 06/19/2016 06:44 PM, Christopher Spinrath wrote:
> The cm-fx6 module has an on-board st,m25p compatible spi flash chip
> used for u-boot (binary & environment). Overwrite the partitions in
> the device tree by the partition table provided in the mtdparts
> environment variable, if it is set.
> 
> This allows to specify a kernel independent partitioning in the
> environment and provides a convient way for the user to adapt the
> partition table.
> 
> Signed-off-by: Christopher Spinrath 
> ---
>  board/compulab/cm_fx6/cm_fx6.c | 16 +++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/board/compulab/cm_fx6/cm_fx6.c b/board/compulab/cm_fx6/cm_fx6.c
> index 712057a..81a7ae2 100644
> --- a/board/compulab/cm_fx6/cm_fx6.c
> +++ b/board/compulab/cm_fx6/cm_fx6.c

[...]

> +#ifdef CONFIG_FDT_FIXUP_PARTITIONS
> +struct node_info nodes[] = {
> + { "st,m25p",MTD_DEV_TYPE_NOR,   },

Nikita, is this enough for all flashes we assemble on cm-fx6?

> +};
> +#endif
> +

[...]

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

Re: [U-Boot] [PATCH 3/3] ARM: configs: cm_fx6: add mtd support

[Igor Grinberg]

On 06/19/2016 06:44 PM, Christopher Spinrath wrote:
> The cm-fx6 module has an on-board spi flash chip. Enable mtd support
> and the mtdparts command. Also define a default partitioning, add
> it to the default environment, and enable support to overwrite the
> partitioning defined in a device tree by it.
> 
> These changes move the effective default partitioning from the device
> tree shipped with the vendor kernels to u-boot which becomes the single
> point of definition for the partitioning for all device tree based
> kernels (in particular, for the upstream linux kernel which does not
> have a default partitioning defined in its device tree).
> 
> Signed-off-by: Christopher Spinrath 
> ---
>  include/configs/cm_fx6.h | 19 ++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/include/configs/cm_fx6.h b/include/configs/cm_fx6.h
> index f054ca8..c839b03 100644
> --- a/include/configs/cm_fx6.h
> +++ b/include/configs/cm_fx6.h

[...]

> @@ -157,7 +174,7 @@
>   "run setupnandboot;" \
>   "run nandboot;"
>  
> -#define CONFIG_PREBOOT   "usb start"
> +#define CONFIG_PREBOOT   "usb start;sf probe"

Probably, this is really needed.
Care to explain?

>  
>  /* SPI */
>  #define CONFIG_SPI
> 

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

Re: [U-Boot] [PATCH 2/3] ARM: board: cm_fx6: fixup mtd partitions in the fdt

[Igor Grinberg]

Hi Christopher,

On 06/19/2016 06:44 PM, Christopher Spinrath wrote:
> The cm-fx6 module has an on-board st,m25p compatible spi flash chip
> used for u-boot (binary & environment). Overwrite the partitions in
> the device tree by the partition table provided in the mtdparts
> environment variable, if it is set.
> 
> This allows to specify a kernel independent partitioning in the
> environment and provides a convient way for the user to adapt the
> partition table.
> 
> Signed-off-by: Christopher Spinrath 
> ---
>  board/compulab/cm_fx6/cm_fx6.c | 16 +++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/board/compulab/cm_fx6/cm_fx6.c b/board/compulab/cm_fx6/cm_fx6.c
> index 712057a..81a7ae2 100644
> --- a/board/compulab/cm_fx6/cm_fx6.c
> +++ b/board/compulab/cm_fx6/cm_fx6.c
> @@ -12,6 +12,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -28,6 +29,7 @@
>  #include 
>  #include 
>  #include 
> +#include 

Why is this needed?

>  #include "common.h"
>  #include "../common/eeprom.h"
>  #include "../common/common.h"
> @@ -581,6 +583,13 @@ int cm_fx6_setup_ecspi(void) { return 0; }
>  
>  #ifdef CONFIG_OF_BOARD_SETUP
>  #define USDHC3_PATH  "/soc/aips-bus@0210/usdhc@02198000/"
> +
> +#ifdef CONFIG_FDT_FIXUP_PARTITIONS
> +struct node_info nodes[] = {
> + { "st,m25p",MTD_DEV_TYPE_NOR,   },
> +};
> +#endif
> +
>  int ft_board_setup(void *blob, bd_t *bd)
>  {
>   u32 baseboard_rev;
> @@ -589,6 +598,8 @@ int ft_board_setup(void *blob, bd_t *bd)
>   char baseboard_name[16];
>   int err;
>  
> + fdt_shrink_to_minimum(blob); /* Make room for new properties */
> +
>   /* MAC addr */
>   if (eth_getenv_enetaddr("ethaddr", enetaddr)) {
>   fdt_find_and_setprop(blob,
> @@ -607,7 +618,6 @@ int ft_board_setup(void *blob, bd_t *bd)
>   return 0; /* Assume not an early revision SB-FX6m baseboard */
>  
>   if (!strncmp("SB-FX6m", baseboard_name, 7) && baseboard_rev <= 120) {
> - fdt_shrink_to_minimum(blob); /* Make room for new properties */
>   nodeoffset = fdt_path_offset(blob, USDHC3_PATH);
>   fdt_delprop(blob, nodeoffset, "cd-gpios");
>   fdt_find_and_setprop(blob, USDHC3_PATH, "broken-cd",
> @@ -616,6 +626,10 @@ int ft_board_setup(void *blob, bd_t *bd)
>NULL, 0, 1);
>   }
>  
> +#ifdef CONFIG_FDT_FIXUP_PARTITIONS
> + fdt_fixup_mtdparts(blob, nodes, ARRAY_SIZE(nodes));
> +#endif

I really dislike the ifdeffery inside functions.
Care to introduce a stub for the !CONFIG_FDT_FIXUP_PARTITIONS case in
include/fdt_support.h for this one?

> +
>   return 0;
>  }
>  #endif
> 

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

Re: [U-Boot] [PATCH 0/3] ARM: imx: enhance support for the cm-fx6 module

[Igor Grinberg]

Hi Christopher,

Thanks for doing this work.

On 06/19/2016 06:44 PM, Christopher Spinrath wrote:
> Hi,
> 
> this series aims at enhancing support for the cm-fx6 module. In
> particular, with respect to the upstream linux kernel.
> 
> The first patch improves the default environment. It is non-functional
> but makes it more convenient to adapt certain settings.
> 
> The later two patches add mtd partition support for the on-board spi
> flash chip. They pick up the discussion about specifying a default
> partitioning in the device tree from here [1]. In short: adding the
> default partitioning to the device tree was rejected by the linux/
> device tree community during mainlining large parts of the device tree.
> It was proposed to implement the partition/mtd handling in u-boot.
> On the other hand, it was argued that the flash chip becomes some
> kind of "black-box" since there will be no partition labeling (in
> particular, with old u-boot versions).
> 
> IMHO defining the mtd partitioning has the following (dis-)advantages:

You mean defining it in U-Boot instead of upstream DT.

> 
> Advantages:
> - It is easier for the user to change the partitioning (e.g. to use
>   the unsued 1MB free space).

I know it says reserved, but that is not exactly unused...
It is intended to hold a splash screen of up to 1MB size and may be other
things (like dtb, etc.).
By the time the partitioning layout was defined, it was still unclear
what requirements of that additional space will be.
So it was called reserved to provide a kind of warning to the users as
it might be used at some point.

> 
> - The flash ship is used entirely for u-boot.

Close, but not precise...
The spi flash chip is used for all boot purposes, so it might be beyond
U-Boot.

>   So it is quite natural
>   that u-boot manages it. Also, moving the partition table to it
>   allows us to change the layout in future versions of u-boot (almost
>   independently of the kernel - there are still non-device tree kernels).

Please don't change the layout as it will break backwards compatibility.
Also, there is not much you can change.

> 
> - U-Boot becomes the single point of definition for all device tree
>   kernels. Otherwise, each kernel (vendor vs. upstream + version)
>   would ship its own partitioning.

True.

>   Moreover, u-boot has to know
>   something about the partitioning, too, because it has to know where
>   the environment is saved.

It does, although the env address is hardcoded, but it should not be
changed.
The reason for providing a partition for it is purely for Linux to able
to change the environment variables.

> 
> Disadvantages:
> - Users of the upstream linux kernel have to use a recent u-boot
>   version to avoid the "black box" effect. A concrete impact is
>   that the update routine (described/proposed by CompuLab) does
>   not work out of the box with older u-boot versions.

True.

> 
> - Updating u-boot is something users might not want or miss to do.
> 
> However, I think nowadays it is ok to demand a recent u-boot in
> combination with the upstream kernel. The cm-fx6 wouldn't be
> the first board doing so. Hence, I propose these patches here.
> 
> Cheers,
> Christopher
> 
> [1] 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/434562.html
> 
> 

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

Re: [U-Boot] [PATCH 1/3] ARM: configs: cm_fx6: improve default environment

[Igor Grinberg]

On 06/19/2016 06:44 PM, Christopher Spinrath wrote:
> Currently, entire script segments have to be changed in the default
> environment to change the kernel image location or to append kernel
> cmdline parameters. In the later case this has to be changed for
> every possible boot device.
> 
> Introduce new variables for kernel image locations and boot device
> independent kernel parameters to make it easier to change these
> settings.
> 
> Signed-off-by: Christopher Spinrath <christopher.spinr...@rwth-aachen.de>

Reviewed-by: Igor Grinberg <grinb...@compulab.co.il>

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

Re: [U-Boot] [PATCH 3/6] autoboot: remove CONFIG_ZERO_BOOTDELAY_CHECK

[Igor Grinberg]

On 06/21/2016 08:32 AM, Masahiro Yamada wrote:
> As the help message of CONFIG_BOOTDELAY says, CONFIG_BOOTDELAY=-2
> means the autoboot with no delay, with no abort check even if
> CONFIG_ZERO_BOOTDELAY_CHECK is defined.
> 
> To sum up, the autoboot behaves as follows:
> 
>  [1] CONFIG_BOOTDELAY=0 && CONFIG_ZERO_BOOTDELAY_CHECK=y
> autoboot with no delay, but you can abort it by key input
> 
>  [2] CONFIG_BOOTDELAY=0 && CONFIG_ZERO_BOOTDELAY_CHECK=n
> autoboot with no delay, with no check for abort
> 
>  [3] CONFIG_BOOTDELAY=-1
> disable autoboot
> 
>  [4] CONFIG_BOOTDELAY=-2
> autoboot with no delay, with no check for abort
> 
> As you notice, [2] and [4] come to the same result, which means we
> do not need CONFIG_ZERO_BOOTDELAY_CHECK.  We can control all the
> cases only by CONFIG_BOOTDELAY, like this:
> 
>  [1] CONFIG_BOOTDELAY=0
> autoboot with no delay, but you can abort it by key input
> 
>  [2] CONFIG_BOOTDELAY=-1
> disable autoboot
> 
>  [3] CONFIG_BOOTDELAY=-2
> autoboot with no delay, with no check for abort
> 
> This commit converts the logic as follow:
>   CONFIG_BOOTDELAY=0 && CONFIG_ZERO_BOOTDELAY_CHECK=n
> --> CONFIG_BOOTDELAY=-2
> 
> Signed-off-by: Masahiro Yamada <yamada.masah...@socionext.com>

Thanks!

Acked-by: Igor Grinberg <grinb...@compulab.co.il>

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

Re: [U-Boot] [PATCH v2] ARM: board: cm-fx6: fix mmc for old revisions of utilite

[Igor Grinberg]

On 06/16/2016 03:02 PM, Christopher Spinrath wrote:
> Old revisions of Utilite (based on cm-fx6) do not have a dedicated
> card detect pin. But the card is removable by the user and card
> detection can be realized with polling (e.g. supported by Linux).
> 
> Add the broken-cd property to the mmc device tree instead of the
> non-removable property to make card detection possible if polling
> is supported.
> 
> Signed-off-by: Christopher Spinrath <christopher.spinr...@rwth-aachen.de>
> Acked-by: Nikita Kiryanov <nik...@compulab.co.il>

Acked-by: Igor Grinberg <grinb...@compulab.co.il>

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

Re: [U-Boot] [PATCH] ARM: board: cm-fx6: fix mmc for old revisions of utilite

[Igor Grinberg]

On 06/16/2016 02:21 PM, Christopher Spinrath wrote:
> Hi Igor,
> 
> On 06/16/2016 11:05 AM, Igor Grinberg wrote:
>> Hi Christopher,
>>
>> On 06/15/2016 06:38 PM, Christopher Spinrath wrote:
>>> Hi Nikita,

[...]

>>>> One nit-pick below:
>>>>
>>>>>
>>>>> Fixes: 41855186afd3 ("arm: mx6: cm-fx6: modify device tree for old 
>>>>> revisions of utilite")
>>>>
>>>> This isn't technically a fix; you're enabling new functionality. The
>>>> original behavior wasn't buggy, it just lacked the card detect feature.
>>>>
>>> Well, the card is clearly removable. So IMHO adding the non-removable
>>> property is wrong and this patch corrects/fixes it. But I'm fine either way.
>>
>> Just a little explanation...
>> Mechanically the card _is_ removable, but for revisions < 1.3, it will
>> result in errors on the bus as no removal event will be sent to the
>> subsystem. Moreover, if I'm not mistaken, you have the PRO model, right?
>> In PRO model, you have internal storage (e.g. SSD) which makes the SD card
>> an additional and sensibly removable device...
>> There are additional Utilite models which have the SD card as the only
>> storage device and those models have the rootfs on the SD card.
>> In such case, IMO, it is much more appropriate to state that it should be
>> non-removable.
>>
> With the broken-cd property the driver/subsystem knows that the card may
> have been removed and checks that if an (false positive) error occurs.
> 
> Indeed, I have the Pro model but even for the other models there are use
> cases where the card may be removed. For instance, you can use netboot
> (think of thin clients) or boot from usb storage. So IMO the broken-cd
> property is a better choice for all models.

Yeah, no problem - I'm not saying we should keep the original, just
explaining the rationale behind what was done and why we do not see it
as a fix, but rather as an improvement to cover more use cases.

Thanks for the patch!

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

Re: [U-Boot] [PATCH] ARM: board: cm-fx6: fix mmc for old revisions of utilite

[Igor Grinberg]

Hi Christopher,

On 06/15/2016 06:38 PM, Christopher Spinrath wrote:
> Hi Nikita,
> 
> On 06/15/2016 05:15 PM, Nikita Kiryanov wrote:
>> Hi CHristopher,
>>
>> On Wed, Jun 08, 2016 at 09:02:36PM +0200, Christopher Spinrath wrote:
>>> Old revisions of Utilite (based on cmfx6) do not have a dedicated
>>> card detect pin. But the card is removable by the user and card
>>> detection can be realized with polling (e.g. supported by Linux).
>>>
>>> Add the broken-cd property to the mmc device tree instead of the
>>> non-removable property to make card detection possible if polling
>>> is supported.
>>
>> Acked-by: Nikita Kiryanov 
> 
> How is this patch (and, in general, patches for Utilite/cm-fx6) supposed
> to be merged?

Well, you've done this almost correctly. You just need to add
Stefano Babic 
(who is the maintainer of imx) to "cc" or "to" list.
Added now.

> Due to get_maintainers your are (the only) maintainer
> related to the cm-fx6 board.

That is a separate discussion (e.g. how boards of a specific vendor, but
different architectures/platforms should be listed in the MAINTAINERS files).

> Do you want me to resend the patch (without
> the Fixes: tag)?
> 
>> One nit-pick below:
>>
>>>
>>> Fixes: 41855186afd3 ("arm: mx6: cm-fx6: modify device tree for old 
>>> revisions of utilite")
>>
>> This isn't technically a fix; you're enabling new functionality. The
>> original behavior wasn't buggy, it just lacked the card detect feature.
>>
> Well, the card is clearly removable. So IMHO adding the non-removable
> property is wrong and this patch corrects/fixes it. But I'm fine either way.

Just a little explanation...
Mechanically the card _is_ removable, but for revisions < 1.3, it will
result in errors on the bus as no removal event will be sent to the
subsystem. Moreover, if I'm not mistaken, you have the PRO model, right?
In PRO model, you have internal storage (e.g. SSD) which makes the SD card
an additional and sensibly removable device...
There are additional Utilite models which have the SD card as the only
storage device and those models have the rootfs on the SD card.
In such case, IMO, it is much more appropriate to state that it should be
non-removable.

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

Re: [U-Boot] [PATCH v3 01/12] common: bootdelay: move CONFIG_BOOTDELAY into a Kconfig option

[Igor Grinberg]

Hi Heiko,

On 06/07/2016 09:31 AM, Heiko Schocher wrote:
> move CONFIG_BOOTDELAY into a Kconfig option. Used for this
> purpose the moveconfig.py tool in tools.

That's great! Finally, someone did this long long patch!
It has been sitting in my queue for about a year... and I never
had the time to rework it.

> 
> Signed-off-by: Heiko Schocher <h...@denx.de>
> 
> Reviewed-by: Tom Rini <tr...@konsulko.com>

Acked-by: Igor Grinberg <grinb...@compulab.co.il>

Thanks!

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

Re: [U-Boot] [PATCH v3 09/12] mmc: omap_hsmmc: enable 8bit interface for eMMC for AM33xx

[Igor Grinberg]

Hi Heiko,

On 06/07/2016 09:31 AM, Heiko Schocher wrote:
> Enable 8bit interface on HSMMC2 for am33xx to support 8bit eMMC chips.
> 
> Signed-off-by: Heiko Schocher 
> Reviewed-by: Tom Rini 
> 
> ---
> 
> Changes in v3: None
> Changes in v2:
> - add Reviewed-by from Tom Rini
> 
>  drivers/mmc/omap_hsmmc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c
> index be34057..d007b56 100644
> --- a/drivers/mmc/omap_hsmmc.c
> +++ b/drivers/mmc/omap_hsmmc.c
> @@ -701,6 +701,7 @@ int omap_mmc_init(int dev_index, uint host_caps_mask, 
> uint f_max, int cd_gpio,
>   priv_data->base_addr = (struct hsmmc *)OMAP_HSMMC2_BASE;
>  #if (defined(CONFIG_OMAP44XX) || defined(CONFIG_OMAP54XX) || \
>   defined(CONFIG_DRA7XX) || defined(CONFIG_AM57XX) || \
> + defined(CONFIG_AM33XX) || \
>   defined(CONFIG_AM43XX) || defined(CONFIG_SOC_KEYSTONE)) && \
>   defined(CONFIG_HSMMC2_8BIT)

Don't you find the above habit terrible - adding more and more SoCs
to the ifdef...

Don't get me wrong, I'm not trying to prevent this patch - please merge it.
But I think, we should do something about the above (and I can see also other
cases in the same function).

>   /* Enable 8-bit interface for eMMC on OMAP4/5 or DRA7XX */
> 

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

Re: [U-Boot] [PATCH V2 01/12] Makefile: Include vendor common library in include search path

[Igor Grinberg]

Hi Nishanth,

On 11/13/15 07:43, Nishanth Menon wrote:
> When the vendor common libraries exists, then board should be able to
> reference headers located from a generic base, rather than having to
> do weird logic such as '#include "../common/xyz.h"'.
> 
> There are multiple options of implementation, the current strategy
> expects that:
> a) Vendor boards that need generic files will define build in:
>   board/$(VENDOR)/common
> b) Vendor boards that expose generic functions from (a) for usage
> from other board specific files will provide these headers in:
>   board/$(VENDOR)/common/include/board-common

I would agree with Simon - no need for additional /common/ in the path.

> c) Vendor board files that need these function services will refer
>   #include 
>   Where, xyz.h is an example header exposing generic vendor common
>   functions.
> 
> Cc: Simon Glass 
> Cc: Tom Rini 
> Cc: Masahiro Yamada 
> Cc: Daniel Schwierzeck 
> Cc: Michal Marek 
> Cc: Stefan Roese 
> Cc: Bin Meng 
> 
> Signed-off-by: Nishanth Menon 
> ---
>  Makefile | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Makefile b/Makefile
> index 61050adb13f5..e7a3e2b4de51 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -626,6 +626,9 @@ c_flags := $(KBUILD_CFLAGS) $(cpp_flags)
>  # U-Boot objectsorder is important (i.e. start must be first)
>  
>  HAVE_VENDOR_COMMON_LIB = $(if $(wildcard 
> $(srctree)/board/$(VENDOR)/common/Makefile),y,n)
> +# Include vendor headers - they should be in the following location.
> +# board/$(VENDOR)/common/include/board-common
> +UBOOTINCLUDE += $(if $(HAVE_VENDOR_COMMON_LIB:y=1), 
> -I$(srctree)/board/$(VENDOR)/common/include)
>  
>  libs-y += lib/
>  libs-$(HAVE_VENDOR_COMMON_LIB) += board/$(VENDOR)/common/
> 

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

Re: [U-Boot] [PATCH v2] omap3: cm-t3517: add board specific get_board_rev()

[Igor Grinberg]

On 11/10/15 15:17, Dmitry Lifshitz wrote:
> CM-T3517 has several HW revisions.
> Add board specific get_board_rev() callback to retrieve revision number.
> 
> Signed-off-by: Dmitry Lifshitz <lifsh...@compulab.co.il>

Reviewed-by: Igor Grinberg <grinb...@compulab.co.il>

> ---
> 
>  Igor, Tom
> 
>  I forgot to add a mailing list address in send-email.
>  Resending the patch.

you may want to use aliases to simplify the stuff, check out doc/git-mailrc.

> 
>  v2: Make proper use of cl_eeprom_get_board_rev() parameter.
> 
>  board/compulab/cm_t3517/cm_t3517.c | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/board/compulab/cm_t3517/cm_t3517.c 
> b/board/compulab/cm_t3517/cm_t3517.c
> index d1c74db..8aae248 100644
> --- a/board/compulab/cm_t3517/cm_t3517.c
> +++ b/board/compulab/cm_t3517/cm_t3517.c
> @@ -98,6 +98,15 @@ int board_init(void)
>   return 0;
>  }
>  
> +/*
> + * Routine: get_board_rev
> + * Description: read system revision
> + */
> +u32 get_board_rev(void)
> +{
> + return cl_eeprom_get_board_rev(CONFIG_SYS_I2C_EEPROM_BUS);
> +};
> +
>  int misc_init_r(void)
>  {
>   cl_print_pcb_info();
> 

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

Re: [U-Boot] [PATCH v4 3/7] ARM: omap-common: Add standard access for board description EEPROM

[Igor Grinberg]

On 11/06/15 02:39, Steve Kipisz wrote:
> From: Lokesh Vutla <lokeshvu...@ti.com>
> 
> Several TI EVMs have EEPROM that can contain board description information
> such as revision, DDR definition, serial number, etc. In just about all
> cases, these EEPROM are on the I2C bus and provides us the opportunity
> to centralize the generic operations involved.
> 
> The on-board EEPROM on the BeagleBone Black, BeagleBone, AM335x EVM,
> AM43x GP EVM, AM57xx-evm, BeagleBoard-X15 share the same format.
> However, DRA-7* EVMs, OMAP4SDP use a modified format.
> 
> We hence introduce logic which is generic between these platforms
> without enforcing any specific format. This allows the boards to use the
> relevant format for operations that they might choose.
> 
> This module will compile for all TI SoC based boards when I2C is enabled,
> even non-TI boards that do not have the EEPROM. If the functions are not
> used, they will not be linked in.
> 
> It is important to note that this logic is fundamental to the board
> configuration process such as DDR configuration which is needed in
> SPL, hence cannot be part of the standard u-boot driver model (which
> is available later in the process). Hence, to aid efficiency, the
> eeprom contents are copied over to SRAM scratchpad memory area at the
> first invocation to retrieve data.
> 
> The follow on patches introduce the use of this library for AM335x,
> AM437x, and AM57xx.
> 
> Signed-off-by: Lokesh Vutla <lokeshvu...@ti.com>
> Signed-off-by: Steve Kipisz <s-kipi...@ti.com>

Acked-by: Igor Grinberg <grinb...@compulab.co.il>

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

Re: [U-Boot] [PATCH v4 7/7] board: ti: AM57xx: Add detection logic for AM57xx-evm

[Igor Grinberg]

On 11/06/15 02:39, Steve Kipisz wrote:
> Current AM57xx evm supports both BeagleBoard-X15
> (http://beagleboard.org/x15) and AM57xx EVM
> (http://www.ti.com/tool/tmdxevm5728).
> 
> The AM572x EValuation Module(EVM) provides an affordable platform to
> quickly start evaluation of Sitara. ARM Cortex-A15 AM57x Processors
> (AM5728, AM5726, AM5718, AM5716) and accelerate development for HMI,
> machine vision, networking, medical imaging and many other industrial
> applications. This EVM is based on the same BeagleBoard-X15 Chassis
> and adds mPCIe, mSATA, LCD, touchscreen, Camera, push button and TI's
> wlink8 offering.
> 
> Since the EEPROM contents are compatible between the BeagleBoard-X15 and
> the AM57xx-evm, we add support for the detection logic to enable
> support for various user programmable scripting capability.
> 
> NOTE: U-boot configuration is currently a superset of AM57xx evm and
> BeagleBoard-X15 and no additional configuration tweaking is needed.
> 
> This change also sets up the stage for future support of TI AM57xx EVMs
> to the same base bootloader build.
> 
> Signed-off-by: Steve Kipisz <s-kipi...@ti.com>

Acked-by: Igor Grinberg <grinb...@compulab.co.il>

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

Re: [U-Boot] [PATCH v2 5/5] board: ti: AM57xx: Add detection logic for AM57xx-evm

[Igor Grinberg]

Hi Steve,

On 11/03/15 14:22, Steve Kipisz wrote:

[...]

> Signed-off-by: Steve Kipisz 
> ---
> v2 Based on:
>  master a6104737 ARM: at91: sama5: change the environment address to 
> 0x6000
> 
> Build testing: MAKEALL -s omap4 -s omap5 (no warnings/build errors)
>   Boot Testing:
>   am57xx_evm_nodt_config: http://pastebin.ubuntu.com/13039296/
>   beagle_x15_config: http://pastebin.ubuntu.com/13039331/
> 
> Changes in v2 (since v1):
>   - move the board detection code into the new routine
> do_board_detect
>   - eliminate board.h and move the ix_xxx into board.c
>   - redo commit message to be more clear
> 
> v1:  http://marc.info/?t=14460800792=1=2
>  http://marc.info/?t=14460800794=1=2
>   (mailing list squashed original submission)

[...]

> +#define is_x15() board_am_is("BBRDX15_")
> +#define is_am572x_evm()  board_am_is("AM572PM_")

I think board_is_* much more appropriate here...

> +
>  #ifdef CONFIG_DRIVER_TI_CPSW
>  #include 
>  #endif
> @@ -246,6 +249,54 @@ struct vcores_data beagle_x15_volts = {
>   .iva.pmic   = ,
>  };
>  
> +#ifdef CONFIG_SPL_BUILD
> +/* No env to setup for SPL */
> +static inline void setup_board_eeprom_env(void) { }
> +
> +/* Override function to read eeprom information */
> +void do_board_detect(void)
> +{
> + struct ti_am_eeprom *ep;
> + int rc;
> +
> + rc = ti_i2c_eeprom_am_get(CONFIG_EEPROM_BUS_ADDRESS,
> +   CONFIG_EEPROM_CHIP_ADDRESS, );
> + if (rc)
> + printf("ti_i2c_eeprom_init failed %d\n", rc);
> +}

Do you really need this in SPL?

> +
> +#else/* CONFIG_SPL_BUILD */
> +
> +static void setup_board_eeprom_env(void)
> +{
> + char *name = NULL;

How about:

char *name = "beagle_x15";

> + int rc;
> + struct ti_am_eeprom_printable p;
> +
> + rc = ti_i2c_eeprom_am_get_print(CONFIG_EEPROM_BUS_ADDRESS,
> + CONFIG_EEPROM_CHIP_ADDRESS, );
> + if (rc) {
> + printf("Invalid EEPROM data(@0x%p). Default to X15\n",
> +TI_AM_EEPROM_DATA);
> + goto invalid_eeprom;
> + }
> +
> + if (is_x15())
> + name = "beagle_x15";

This will not be needed if the above comment is implemented.

> + else if (is_am572x_evm())
> + name = "am57xx_evm";
> + else
> + printf("Unidentified board claims %s in eeprom header\n",
> +p.name);
> +
> +invalid_eeprom:
> + set_board_info_env(name, "beagle_x15", p.version, p.serial);

If the above comment is implemented, no more need for the
default_name parameter...

> +}
> +
> +/* Eeprom is alread read by SPL.. nothing more to do here.. */
> +
> +#endif   /* CONFIG_SPL_BUILD */

[...]


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

Re: [U-Boot] [PATCH v2 3/5] ARM: omap-common: Add standard access for board description EEPROM

[Igor Grinberg]

Hi Steve,

On 11/03/15 14:22, Steve Kipisz wrote:
> From: Lokesh Vutla 

[...]

> 
> Signed-off-by: Lokesh Vutla 
> Signed-off-by: Steve Kipisz 
> ---
> v2 Based on
>  master  a6104737  ARM: at91: sama5: change the environment address to 
> 0x6000
> 
> Changes in v2 (since v1)
>   - make the EEPROM code mor generic for TI EVMs
>   - rename structures/subroutines to ti_am_x
>   - add routines to access the EEPROM data
>   - redo commit message to be more clear
> 
> v1:   http://marc.info/?t=14460800791=1=2
>   (mailing list squashed original submission)
> 
>  arch/arm/cpu/armv7/omap-common/Makefile|   1 +
>  arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c | 148 
> +
>  arch/arm/include/asm/omap_common.h | 130 +-
>  3 files changed, 278 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c
> 
> diff --git a/arch/arm/cpu/armv7/omap-common/Makefile 
> b/arch/arm/cpu/armv7/omap-common/Makefile
> index 464a5d1d732a..53a9fdb81100 100644
> --- a/arch/arm/cpu/armv7/omap-common/Makefile
> +++ b/arch/arm/cpu/armv7/omap-common/Makefile
> @@ -15,6 +15,7 @@ obj-y   += clocks-common.o
>  obj-y+= emif-common.o
>  obj-y+= vc.o
>  obj-y+= abb.o
> +obj-$(CONFIG_I2C) += ti-i2c-eeprom.o

This makes this module compile on all TI SoC based boards enabling I2C.
AFAIU, this is a separate chip (not inside the SoC), so this module will
also compile on non-TI boards that do not have this EEPROM.
I think, it should be more fine grained (e.g. have its own symbol).

>  endif
>  
>  ifneq ($(CONFIG_OMAP54XX),)
> diff --git a/arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c 
> b/arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c
> new file mode 100644
> index ..f59ebbdb4ee8
> --- /dev/null
> +++ b/arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c

[...]

> +void __maybe_unused set_board_info_env(char *name, char *default_name,
> +char *revision, char *serial)

That looks really weird API to me...
You have name and default_name? Why would you need both...

> +{
> + char *unknown = "unknown";
> +
> + if (name)
> + setenv("board_name", name);
> + else
> + setenv("board_name", default_name);
> +
> + if (revision)
> + setenv("board_revision", revision);
> + else
> + setenv("board_revision", unknown);
> +
> + if (serial)
> + setenv("board_serial", serial);
> + else
> + setenv("board_serial", unknown);
> +}

[...]

> diff --git a/arch/arm/include/asm/omap_common.h 
> b/arch/arm/include/asm/omap_common.h
> index d773b0430ad4..a76c67a85d37 100644
> --- a/arch/arm/include/asm/omap_common.h
> +++ b/arch/arm/include/asm/omap_common.h

[...]

> +/**
> + * set_board_info_env() - Setup commonly used board information environment 
> vars
> + * @name:Name of the board
> + * @default_name: In case of empty string, what name to use?

That seems redundant.
The caller knows the board name, and if it does not, then it can place
an arbitrary name (like unknown) into name parameter.

> + * @revision:Revision of the board
> + * @serial:  Serial Number of the board
> + *
> + * In case of NULL revision or serial information "unknown" is setup.
> + * If name is NULL, default_name is used.
> + */
> +void set_board_info_env(char *name, char *default_name,
> + char *revision, char *serial);
> +
> +/**
> + * board_am_is() - Generic Board detection logic
> + * @name_tag:Tag used in eeprom for the board
> + *
> + * Return: false if board information does not match OR eeprom was'nt read.
> + *  true otherwise
> + */
> +static inline bool board_am_is(char *name_tag)
> +{
> + struct ti_am_eeprom *ep = TI_AM_EEPROM_DATA;
> +
> + if (ep->header != TI_EEPROM_HEADER_MAGIC)
> + return false;
> + return !strncmp(ep->name, name_tag, TI_EEPROM_HDR_NAME_LEN);
> +}

Why do you need to place non trivial function implementation inside the
header file?

> +
> +/**
> + * board_am_rev_is() - Compare board revision
> + * @rev_tag: Revision tag to check in eeprom
> + * @cmp_len: How many chars to compare?
> + *
> + * NOTE: revision information is often messed up (hence the str len match) :(
> + *
> + * Return: false if board information does not match OR eeprom was'nt read.
> + *  true otherwise
> + */
> +static inline bool board_am_rev_is(char *rev_tag, int cmp_len)
> +{
> + struct ti_am_eeprom *ep = TI_AM_EEPROM_DATA;
> + int l;
> +
> + if (ep->header != TI_EEPROM_HEADER_MAGIC)
> + return false;
> +
> + l = cmp_len > TI_EEPROM_HDR_REV_LEN ? TI_EEPROM_HDR_NAME_LEN : cmp_len;
> + return !strncmp(ep->version, rev_tag, l);
> +}

Same here.

> +#endif   /* __ASSEMBLY__ */
> +
>  #endif /* _OMAP_COMMON_H_ */
> 

-- 
Regards,
Igor.

Re: [U-Boot] [PATCH v2 5/5] board: ti: AM57xx: Add detection logic for AM57xx-evm

[Igor Grinberg]

On 11/03/15 17:09, Steven Kipisz wrote:
> On 11/03/2015 07:29 AM, Igor Grinberg wrote:
>> Hi Steve,
>>
>> On 11/03/15 14:22, Steve Kipisz wrote:
>>
>> [...]
>>
>>> Signed-off-by: Steve Kipisz <s-kipi...@ti.com>
>>> ---
>>> v2 Based on:
>>>   master a6104737 ARM: at91: sama5: change the environment address to 
>>> 0x6000
>>>
>>> Build testing: MAKEALL -s omap4 -s omap5 (no warnings/build errors)
>>> Boot Testing:
>>> am57xx_evm_nodt_config: http://pastebin.ubuntu.com/13039296/
>>> beagle_x15_config: http://pastebin.ubuntu.com/13039331/
>>>
>>> Changes in v2 (since v1):
>>> - move the board detection code into the new routine
>>>   do_board_detect
>>> - eliminate board.h and move the ix_xxx into board.c
>>> - redo commit message to be more clear
>>>
>>> v1:  http://marc.info/?t=14460800792=1=2
>>>   http://marc.info/?t=14460800794=1=2
>>> (mailing list squashed original submission)
>>
>> [...]
>>
>>> +#define is_x15()board_am_is("BBRDX15_")
>>> +#define is_am572x_evm()board_am_is("AM572PM_")
>>
>> I think board_is_* much more appropriate here...
>>
> Ok. so board_is_x15 and board_is_am572x_evm

Yep. Sounds better.

>>> +
>>>   #ifdef CONFIG_DRIVER_TI_CPSW
>>>   #include 
>>>   #endif
>>> @@ -246,6 +249,54 @@ struct vcores_data beagle_x15_volts = {
>>>   .iva.pmic= ,
>>>   };
>>>
>>> +#ifdef CONFIG_SPL_BUILD
>>> +/* No env to setup for SPL */
>>> +static inline void setup_board_eeprom_env(void) { }
>>> +
>>> +/* Override function to read eeprom information */
>>> +void do_board_detect(void)
>>> +{
>>> +struct ti_am_eeprom *ep;
>>> +int rc;
>>> +
>>> +rc = ti_i2c_eeprom_am_get(CONFIG_EEPROM_BUS_ADDRESS,
>>> +  CONFIG_EEPROM_CHIP_ADDRESS, );
>>> +if (rc)
>>> +printf("ti_i2c_eeprom_init failed %d\n", rc);
>>> +}
>>
>> Do you really need this in SPL?
> 
> Yes. We need to detect the board to determine DDR setup, pin mux, iodelay. 
> All of that needs to be done in SPL. X15 and EVM are the same, but more 
> boards will be added that have some differences.

Ok.

>>
>>> +
>>> +#else/* CONFIG_SPL_BUILD */
>>> +
>>> +static void setup_board_eeprom_env(void)
>>> +{
>>> +char *name = NULL;
>>
>> How about:
>>
>> char *name = "beagle_x15";
>>
>>> +int rc;
>>> +struct ti_am_eeprom_printable p;
>>> +
>>> +rc = ti_i2c_eeprom_am_get_print(CONFIG_EEPROM_BUS_ADDRESS,
>>> +CONFIG_EEPROM_CHIP_ADDRESS, );
>>> +if (rc) {
>>> +printf("Invalid EEPROM data(@0x%p). Default to X15\n",
>>> +   TI_AM_EEPROM_DATA);
>>> +goto invalid_eeprom;
>>> +}
>>> +
>>> +if (is_x15())
>>> +name = "beagle_x15";
>>
>> This will not be needed if the above comment is implemented.
>>
>>> +else if (is_am572x_evm())
>>> +name = "am57xx_evm";
>>> +else
>>> +printf("Unidentified board claims %s in eeprom header\n",
>>> +   p.name);
>>> +
>>> +invalid_eeprom:
>>> +set_board_info_env(name, "beagle_x15", p.version, p.serial);
>>
>> If the above comment is implemented, no more need for the
>> default_name parameter...
>>
> Ok, I'll look at that.
>>> +}
>>> +
>>> +/* Eeprom is alread read by SPL.. nothing more to do here.. */
>>> +
>>> +#endif/* CONFIG_SPL_BUILD */
>>
>> [...]
>>
>>
> Steve K.
> 

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

Re: [U-Boot] [PATCH v2 3/5] ARM: omap-common: Add standard access for board description EEPROM

[Igor Grinberg]

On 11/03/15 17:26, Nishanth Menon wrote:
> On 11/03/2015 09:13 AM, Steven Kipisz wrote:
>> On 11/03/2015 07:16 AM, Igor Grinberg wrote:
>>> Hi Steve,
>>>
>>> On 11/03/15 14:22, Steve Kipisz wrote:
>>>> From: Lokesh Vutla <lokeshvu...@ti.com>
>>>
>>> [...]
>>>
>>>>
>>>> Signed-off-by: Lokesh Vutla <lokeshvu...@ti.com>
>>>> Signed-off-by: Steve Kipisz <s-kipi...@ti.com>
>>>> ---
>>>> v2 Based on
>>>>   master  a6104737  ARM: at91: sama5: change the environment
>>>> address to 0x6000
>>>>
>>>> Changes in v2 (since v1)
>>>> - make the EEPROM code mor generic for TI EVMs
>>>> - rename structures/subroutines to ti_am_x
>>>> - add routines to access the EEPROM data
>>>> - redo commit message to be more clear
>>>>
>>>> v1:   http://marc.info/?t=14460800791=1=2
>>>> (mailing list squashed original submission)
>>>>
>>>>   arch/arm/cpu/armv7/omap-common/Makefile|   1 +
>>>>   arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c | 148
>>>> +
>>>>   arch/arm/include/asm/omap_common.h | 130
>>>> +-
>>>>   3 files changed, 278 insertions(+), 1 deletion(-)
>>>>   create mode 100644 arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c
>>>>
>>>> diff --git a/arch/arm/cpu/armv7/omap-common/Makefile
>>>> b/arch/arm/cpu/armv7/omap-common/Makefile
>>>> index 464a5d1d732a..53a9fdb81100 100644
>>>> --- a/arch/arm/cpu/armv7/omap-common/Makefile
>>>> +++ b/arch/arm/cpu/armv7/omap-common/Makefile
>>>> @@ -15,6 +15,7 @@ obj-y+= clocks-common.o
>>>>   obj-y+= emif-common.o
>>>>   obj-y+= vc.o
>>>>   obj-y+= abb.o
>>>> +obj-$(CONFIG_I2C) += ti-i2c-eeprom.o
>>>
>>> This makes this module compile on all TI SoC based boards enabling I2C.
>>> AFAIU, this is a separate chip (not inside the SoC), so this module will
>>> also compile on non-TI boards that do not have this EEPROM.
>>> I think, it should be more fine grained (e.g. have its own symbol).
>>>
>> Can you give a suggestion?
> 
> Are you sure this will be built into non-ti SoCs with I2C enabled if you
> are not using the function? I assume  __maybe_unused should take care of
> that, no - let the compiler do the gc anyways?
> 
> 
> It should have been documented on commit message as well.
> 
> 
>>>> + * @revision:Revision of the board
>>>> + * @serial:Serial Number of the board
>>>> + *
>>>> + * In case of NULL revision or serial information "unknown" is setup.
>>>> + * If name is NULL, default_name is used.
>>>> + */
>>>> +void set_board_info_env(char *name, char *default_name,
>>>> +char *revision, char *serial);
>>>> +
>>>> +/**
>>>> + * board_am_is() - Generic Board detection logic
>>>> + * @name_tag:Tag used in eeprom for the board
>>>> + *
>>>> + * Return: false if board information does not match OR eeprom
>>>> was'nt read.
>>>> + *   true otherwise
>>>> + */
>>>> +static inline bool board_am_is(char *name_tag)
>>>> +{
>>>> +struct ti_am_eeprom *ep = TI_AM_EEPROM_DATA;
>>>> +
>>>> +if (ep->header != TI_EEPROM_HEADER_MAGIC)
>>>> +return false;
>>>> +return !strncmp(ep->name, name_tag, TI_EEPROM_HDR_NAME_LEN);
>>>> +}
>>>
>>> Why do you need to place non trivial function implementation inside the
>>> header file?
>>>
>>>> +
>>>> +/**
>>>> + * board_am_rev_is() - Compare board revision
>>>> + * @rev_tag:Revision tag to check in eeprom
>>>> + * @cmp_len:How many chars to compare?
>>>> + *
>>>> + * NOTE: revision information is often messed up (hence the str len
>>>> match) :(
>>>> + *
>>>> + * Return: false if board information does not match OR eeprom
>>>> was'nt read.
>>>> + *   true otherwise
>>>> + */
>>>> +static inline bool board_am_rev_is(char *rev_tag, int cmp_len)
>>>> +{
>>>> +struct ti_am_eeprom *ep = TI_AM_EEPROM_DATA;
>>>> +int l;
>>>> +
>>>> +if (ep->header != TI_EEPROM_HEADER_MAGIC)
>>>> +return false;
>>>> +
>>>> +l = cmp_len > TI_EEPROM_HDR_REV_LEN ? TI_EEPROM_HDR_NAME_LEN :
>>>> cmp_len;
>>>> +return !strncmp(ep->version, rev_tag, l);
>>>> +}
>>>
>>> Same here.
>>>
>> I thought by making them static inline would save space.
> 
> I prefer that myself as well.

I'm not sure I understand what space will it save?
AFAIK, inline places the function code inside the the caller function
and thus spreads into each caller, no? It probably saves some branches,
but how does that save space?

Also, AFAIR, we try to not place code inside headers, unless the code
is a stub.

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

Re: [U-Boot] [PATCH v2 3/5] ARM: omap-common: Add standard access for board description EEPROM

[Igor Grinberg]

On 11/03/15 19:45, Nishanth Menon wrote:
> On 11/03/2015 11:02 AM, Igor Grinberg wrote:
> 
>>>>>> +
>>>>>> +/**
>>>>>> + * board_am_rev_is() - Compare board revision
>>>>>> + * @rev_tag:Revision tag to check in eeprom
>>>>>> + * @cmp_len:How many chars to compare?
>>>>>> + *
>>>>>> + * NOTE: revision information is often messed up (hence the str len
>>>>>> match) :(
>>>>>> + *
>>>>>> + * Return: false if board information does not match OR eeprom
>>>>>> was'nt read.
>>>>>> + *   true otherwise
>>>>>> + */
>>>>>> +static inline bool board_am_rev_is(char *rev_tag, int cmp_len)
>>>>>> +{
>>>>>> +struct ti_am_eeprom *ep = TI_AM_EEPROM_DATA;
>>>>>> +int l;
>>>>>> +
>>>>>> +if (ep->header != TI_EEPROM_HEADER_MAGIC)
>>>>>> +return false;
>>>>>> +
>>>>>> +l = cmp_len > TI_EEPROM_HDR_REV_LEN ? TI_EEPROM_HDR_NAME_LEN :
>>>>>> cmp_len;
>>>>>> +return !strncmp(ep->version, rev_tag, l);
>>>>>> +}
>>>>>
>>>>> Same here.
>>>>>
>>>> I thought by making them static inline would save space.
>>>
>>> I prefer that myself as well.
>>
>> I'm not sure I understand what space will it save?
>> AFAIK, inline places the function code inside the the caller function
>> and thus spreads into each caller, no? It probably saves some branches,
>> but how does that save space?
> 
> I dont think it saves space, but rather a function call overhead for
> trivial code as above.

I'm sorry, IMO the above code is not trivial enough... 
or just it is not trivial: several variables on the stack,
some logic which is not that straight forward for someone who is
not familiar with the code and defines that are mapped into an SRAM...
and we also have strings comparison in the end.
Come on... that is not trivial at all. Moreover it gives a very bad
example to any newcomer...

> 
>> Also, AFAIR, we try to not place code inside headers, unless the code
>> is a stub.
> 
> 
> That does not always make sense. here it is a straight forward
> comparison.. why hide it a function call deep when you can inline it?

Well, at least because header files are not meant to hold code.
Accepting this gives a bad example and reduces code quality...

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

Re: [U-Boot] [PATCH 3/5] splash_source: add support for filesystem formatted usb

[Igor Grinberg]

On 10/23/15 13:23, Nikita Kiryanov wrote:
> On Wed, Oct 21, 2015 at 04:01:46PM +0300, Igor Grinberg wrote:
>> On 08/30/15 11:42, Nikita Kiryanov wrote:
>>> Add support for loading splash image from USB drive formatted with a
>>> filesystem.
>>>
>>> Cc: Igor Grinberg <grinb...@compulab.co.il>
>>> Cc: Tom Rini <tr...@konsulko.com>
>>> Signed-off-by: Nikita Kiryanov <nik...@compulab.co.il>
>>
>> [...]
>>
>>> +#ifdef CONFIG_USB_STORAGE
>>> +   if (location->storage == SPLASH_STORAGE_USB) {
>>> +   usb_init();
>>> +   usb_stor_scan(1);
>>> +   }
>>> +#endif
>>
>> Can we use IS_ENABLED() stuff here instead?
> 
> IS_ENABLED() does not prevent compile errors, only closes off certain
> code paths.

Ok. Any other ways to not have that ifdefferry inside functions?


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

Re: [U-Boot] [PATCH 3/5] splash_source: add support for filesystem formatted usb

[Igor Grinberg]

On 08/30/15 11:42, Nikita Kiryanov wrote:
> Add support for loading splash image from USB drive formatted with a
> filesystem.
> 
> Cc: Igor Grinberg <grinb...@compulab.co.il>
> Cc: Tom Rini <tr...@konsulko.com>
> Signed-off-by: Nikita Kiryanov <nik...@compulab.co.il>

[...]

> +#ifdef CONFIG_USB_STORAGE
> + if (location->storage == SPLASH_STORAGE_USB) {
> + usb_init();
> + usb_stor_scan(1);
> + }
> +#endif

Can we use IS_ENABLED() stuff here instead?


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

Re: [U-Boot] [PATCH 1/5] splash_source: rename *_read() to *_read_raw()

[Igor Grinberg]

On 08/30/15 11:42, Nikita Kiryanov wrote:
> Rename raw read functions to *_read_raw() in preparation for supporting
> read_fs() feature.
> 
> Cc: Igor Grinberg <grinb...@compulab.co.il>
> Cc: Tom Rini <tr...@konsulko.com>
> Signed-off-by: Nikita Kiryanov <nik...@compulab.co.il>

Acked-by: Igor Grinberg <grinb...@compulab.co.il>


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

Re: [U-Boot] [PATCH 2/5] splash_source: add support for filesystem formatted mmc

[Igor Grinberg]

Hi Nikita,

On 08/30/15 11:42, Nikita Kiryanov wrote:
> Add support for loading splash image from an SD card formatted with
> a filesystem. Update boards to maintain original behavior where needed.
> 
> Cc: Igor Grinberg <grinb...@compulab.co.il>
> Cc: Tom Rini <tr...@konsulko.com>
> Signed-off-by: Nikita Kiryanov <nik...@compulab.co.il>
> ---

[...]

> diff --git a/common/splash_source.c b/common/splash_source.c
> index 4820c12..cb45c63 100644
> --- a/common/splash_source.c
> +++ b/common/splash_source.c

[...]

> +static int splash_load_fs(struct splash_location *location, u32 
> bmp_load_addr)
> +{
> + int res;
> + loff_t bmp_size;
> +
> + res = splash_select_fs_dev(location);
> + if (res)
> + return res;
> +
> + res = fs_size("splash.bmp", _size);

The splash.bmp can be a default file name (which is selected through
menuconfig), but I think, will it be better to make it a env variable?

> + if (res) {
> + printf("Error (%d): cannot determine file size\n", res);
> + return res;
> + }
> +
> + if (bmp_load_addr + bmp_size >= gd->start_addr_sp) {
> + printf("Error: splashimage address too high. Data overwrites 
> U-Boot and/or placed beyond DRAM boundaries.\n");
> + return -EFAULT;
> + }
> +
> + splash_select_fs_dev(location);
> + return fs_read("splash.bmp", bmp_load_addr, 0, 0, NULL);
> +}
> +

[...]

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

Re: [U-Boot] [PATCH 5/5] arm: mx6: cm-fx6: add splash locations to cm-fx6

[Igor Grinberg]

On 08/30/15 11:42, Nikita Kiryanov wrote:
> Add the following splash locations to cm-fx6:
> * filesystem formatted mmc
> * filesystem formatted usb
> * filesystem formatted sata
> 
> Cc: Igor Grinberg <grinb...@compulab.co.il>
> Cc: Tom Rini <tr...@konsulko.com>
> Signed-off-by: Nikita Kiryanov <nik...@compulab.co.il>

Acked-by: Igor Grinberg <grinb...@compulab.co.il>

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

Re: [U-Boot] [ANN] U-Boot v2015.10 released

[Igor Grinberg]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

On 10/20/15 18:31, Tom Rini wrote:
> On Tue, Oct 20, 2015 at 07:34:27AM +0200, Wolfgang Denk wrote:
>> Dear Tom,
>>
>> In message <2015102800.GF23893@bill-the-cat> you wrote:
>>>
>>> I've pushed v2015.10 out to the repository and tarballs should exist
>>> soon.
>>
>> It appears you did not push the tag, though?
> 
> Oops, fixed.

I've just pulled and nope, not there...


- -- 
Regards,
Igor.
-BEGIN PGP SIGNATURE-
Version: GnuPG v2

iQIcBAEBCAAGBQJWJmlpAAoJEBDE8YO64Efae4wP+gOCnXgEH5Rz+l3lm5cYE0W5
MGeNANvVQqVPFNatwstqMTKhxS+/gCKxzku/YYh2AblqbSlxF4+kEYtnCHye561A
ZOgddVCJrWrE2a+kkGvjwiK2xu+oLsvlSdSQUN7tCn2zSn8Vej6UVffbOGKcL6/Q
O4Bd3qCIBJG4i5Wj1qSUpa2G9mmj9LRq7KocG++lCw7ZKpE5vSdq5WN87y7/6X0m
/9jUHA5HQGA7KyhR3WOJ3ilIkRwOahrELS/GYeGNep8gYg6YVaGk/dPWBuKC/P+W
eZWDF9z3G+QzmCNCVKVPtgHBBOq88vYThJInYoZiC1MjXxvp1VgwC9m4Z9QUXn2f
olfV4MQ7K+AGgt0g0Ak2cBTdTYz5zqu3smktjyFt2G/lxNQiTT2wGq44HRBbfI+p
JfadBndjn6Xpk3enflmwmGHHXoo78Vp3SexSianuD6SHYG9Gq4UV2TfFm9FJgJxJ
S8ugeR6QI0131qNfi8krxv1Wkq+Bxqx4LJ8oobrOQczUXhqSQAQ9lRuf/kRw8LYi
IT2O+VI05ed/+Ri6jbZV3dsdJaIU7xsikf3VYkJCBhezDAjefcOrWCv5keTsyrWE
kk0Sb67Cec0NzZwCec88w8gxBYUKNgje/qZtMITdnIocf+07+MAPk2NV/3jqztt1
J5HgfXhHamEqxJB/kfw3
=GBX4
-END PGP SIGNATURE-
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 1/2] Kconfig: add CONFIG_BOOTDELAY

[Igor Grinberg]

Hi Simon,

On 10/09/15 12:36, Simon Glass wrote:
> Hi Igor,
> 
> On 8 October 2015 at 20:10, Igor Grinberg <grinb...@compulab.co.il> wrote:
>> Add CONFIG_BOOTDELAY to the Kconfig.
>> Default it to 3 seconds according to the majority of configs.
>>
>> Signed-off-by: Igor Grinberg <grinb...@compulab.co.il>
>> Cc: Simon Glass <s...@chromium.org>
>> Cc: Masahiro Yamada <yamada.masah...@socionext.com>
>> ---
>>  common/Kconfig | 10 ++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/common/Kconfig b/common/Kconfig
>> index d98eb19..e13d255 100644
>> --- a/common/Kconfig
>> +++ b/common/Kconfig
>> @@ -100,6 +100,16 @@ config AUTOBOOT_STOP_STR_SHA256
>>   string / password matches a values that is encypted via
>>   a SHA256 hash and saved in the environment.
>>
>> +config BOOTDELAY
>> +   int "Seconds to delay before autobooting"
>> +   default 3
>> +   help
>> + Delay before automatically booting the default image;
>> + set to -1 to disable autoboot.
>> + set to -2 to autoboot with no delay and not check for abort
>> + (even when CONFIG_ZERO_BOOTDELAY_CHECK is defined).
>> + See doc/README.autoboot for additional information.
>> +
>>  endmenu
>>
>>  comment "Commands"
>> --
>> 2.4.9
>>
> 
> It's great that you are taking on these difficult ones.

Thanks...

> 
> IMO we should split this config into a few parts:
> 
> - enabling the boot-delay feature
> - setting the boot-delay time
> - whether to allow abort
> - whether to allow abort even when boot delay is 0
> 
> The way it is written -2 sounds like a weird case that would be better
> merged with the last one above.

I completely agree with you. This is indeed the better way, AFAICS.
Although, I'm not sure I can work on this right now.
How about having it currently the way above and adjusting it later
(when one/me has the time to do it)?


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

Re: [U-Boot] [PATCH 1/2] Kconfig: add CONFIG_CMD_BOOTZ

[Igor Grinberg]

Hi Simon,

On 10/09/15 12:36, Simon Glass wrote:
> Hi Igor,
> 
> On 8 October 2015 at 19:48, Igor Grinberg <grinb...@compulab.co.il> wrote:
>> Add CONFIG_CMD_BOOTZ to the Kconfig.
>> Since the CONFIG_CMD_BOOTZ cannot live without the CONFIG_CMD_BOOTM,
>> make it select the CONFIG_CMD_BOOTM.
>>
>> Signed-off-by: Igor Grinberg <grinb...@compulab.co.il>
>> Cc: Masahiro Yamada <yamada.masah...@socionext.com>
>> Cc: Simon Glass <s...@chromium.org>
>> ---
>>  common/Kconfig | 6 ++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/common/Kconfig b/common/Kconfig
>> index 2c42b8e..d98eb19 100644
>> --- a/common/Kconfig
>> +++ b/common/Kconfig
>> @@ -148,6 +148,12 @@ config CMD_BOOTM
>> help
>>   Boot an application image from the memory.
>>
>> +config CMD_BOOTZ
>> +   bool "bootz"
>> +   select CMD_BOOTM
> 
> Do we need this, or can it be 'depends on CMD_BOOTM'?

Well, the way I see it is that today (with the multi-platform support
in the kernel) you do not need the bootm command (historically happened
that it represents the uImage) and you just want the bootz command.
If we make it "depend" on bootm, the bootz entry will not show up until
you enable the bootm command. That might be not trivial for the "regular"
user who just want to adjust the configuration and build a new binary...

Well of course it can be argued, but I really like how it works with
"select" and I see bootz as an independent command and technically it
happens that they share the same code for the most of it.

> 
>> +   help
>> + Boot a Linux kernel zImage from the memory.
>> +
> 
> Could you add a brief description of what a zImage is and how it gets
> created? It would be good to be a bit more descriptive in these help
> messages.

Yes that is a great idea, and of course it went through my head, but
the problem is that I'm completely out of time and I've seen the other
commands have that simple one liners, so I just did the same thing.
I will try to figure out something more descriptive, but I'm "overbooked"
with work (I've actually used my conferences time to prepare/send the
patches).

> 
>>  config CMD_GO
>> bool "go"
>> default y
>> --
>> 2.4.9
>>
> 
> Regards,
> Simon
> 

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

[U-Boot] [PATCH] ti: omap3: config: remove 1 from boolean define

[Igor Grinberg]

CONFIG_TWL4030_POWER is a boolean define variable. It is either defined
or not defined and should not have a value assigned to it.
Remove the value.

Signed-off-by: Igor Grinberg <grinb...@compulab.co.il>
---
 include/configs/ti_omap3_common.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/configs/ti_omap3_common.h 
b/include/configs/ti_omap3_common.h
index be231a5..e399a87 100644
--- a/include/configs/ti_omap3_common.h
+++ b/include/configs/ti_omap3_common.h
@@ -66,7 +66,7 @@
 #define CONFIG_SYS_MONITOR_LEN (256 << 10)
 
 /* TWL4030 */
-#define CONFIG_TWL4030_POWER   1
+#define CONFIG_TWL4030_POWER
 
 /* SPL */
 #define CONFIG_SPL_TEXT_BASE   0x40200800
-- 
2.4.9

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

[U-Boot] [PATCH 0/2] config: move CONFIG_BOOTDELAY to defconfig

[Igor Grinberg]

Move the CONFIG_BOOTDELAY to defconfig way.
The second patch is quite large. I'm not sure the mailing list will
accept it.
If it will not, then please tell me if can put it on a github
or any other way of delivery.

Igor Grinberg (2):
  Kconfig: add CONFIG_BOOTDELAY
  config: move CONFIG_BOOTDELAY to defconfig

 common/Kconfig | 10 ++
 configs/A10-OLinuXino-Lime_defconfig   |  1 +
 configs/A10s-OLinuXino-M_defconfig |  1 +
 configs/A13-OLinuXinoM_defconfig   |  1 +
 configs/A13-OLinuXino_defconfig|  1 +
 configs/A20-OLinuXino-Lime2_defconfig  |  1 +
 configs/A20-OLinuXino-Lime_defconfig   |  1 +
 configs/A20-OLinuXino_MICRO_defconfig  |  1 +
 configs/A20-Olimex-SOM-EVB_defconfig   |  1 +
 configs/Ainol_AW1_defconfig|  1 +
 configs/Ampe_A76_defconfig |  1 +
 configs/Auxtek-T003_defconfig  |  1 +
 configs/Auxtek-T004_defconfig  |  1 +
 configs/B4420QDS_NAND_defconfig|  1 +
 configs/B4420QDS_SPIFLASH_defconfig|  1 +
 configs/B4420QDS_defconfig |  1 +
 configs/B4860QDS_NAND_defconfig|  1 +
 configs/B4860QDS_SECURE_BOOT_defconfig |  1 +
 configs/B4860QDS_SPIFLASH_defconfig|  1 +
 configs/B4860QDS_SRIO_PCIE_BOOT_defconfig  |  1 +
 configs/B4860QDS_defconfig |  1 +
 configs/BSC9131RDB_NAND_SYSCLK100_defconfig|  1 +
 configs/BSC9131RDB_NAND_defconfig  |  1 +
 configs/BSC9131RDB_SPIFLASH_SYSCLK100_defconfig|  1 +
 configs/BSC9131RDB_SPIFLASH_defconfig  |  1 +
 configs/BSC9132QDS_NAND_DDRCLK100_SECURE_defconfig |  1 +
 configs/BSC9132QDS_NAND_DDRCLK100_defconfig|  1 +
 configs/BSC9132QDS_NAND_DDRCLK133_SECURE_defconfig |  1 +
 configs/BSC9132QDS_NAND_DDRCLK133_defconfig|  1 +
 configs/BSC9132QDS_NOR_DDRCLK100_SECURE_defconfig  |  1 +
 configs/BSC9132QDS_NOR_DDRCLK100_defconfig |  1 +
 configs/BSC9132QDS_NOR_DDRCLK133_SECURE_defconfig  |  1 +
 configs/BSC9132QDS_NOR_DDRCLK133_defconfig |  1 +
 configs/BSC9132QDS_SDCARD_DDRCLK100_SECURE_defconfig   |  1 +
 configs/BSC9132QDS_SDCARD_DDRCLK100_defconfig  |  1 +
 configs/BSC9132QDS_SDCARD_DDRCLK133_SECURE_defconfig   |  1 +
 configs/BSC9132QDS_SDCARD_DDRCLK133_defconfig  |  1 +
 configs/BSC9132QDS_SPIFLASH_DDRCLK100_SECURE_defconfig |  1 +
 configs/BSC9132QDS_SPIFLASH_DDRCLK100_defconfig|  1 +
 configs/BSC9132QDS_SPIFLASH_DDRCLK133_SECURE_defconfig |  1 +
 configs/BSC9132QDS_SPIFLASH_DDRCLK133_defconfig|  1 +
 configs/Bananapi_defconfig |  1 +
 configs/Bananapro_defconfig|  1 +
 configs/C29XPCIE_NOR_SECBOOT_defconfig |  1 +
 configs/C29XPCIE_SPIFLASH_SECBOOT_defconfig|  1 +
 configs/CSQ_CS908_defconfig|  1 +
 configs/Chuwi_V7_CW0825_defconfig  |  1 +
 configs/Colombus_defconfig |  1 +
 configs/Cubieboard2_defconfig  |  1 +
 configs/Cubieboard_defconfig   |  1 +
 configs/Cubietruck_defconfig   |  1 +
 configs/Et_q8_v1_6_defconfig   |  1 +
 configs/Hummingbird_A31_defconfig  |  1 +
 configs/Hyundai_A7HD_defconfig |  1 +
 configs/Ippo_q8h_v1_2_a33_1024x600_defconfig   |  1 +
 configs/Ippo_q8h_v1_2_defconfig|  1 +
 configs/Ippo_q8h_v5_defconfig  |  1 +
 configs/Linksprite_pcDuino3_Nano_defconfig |  1 +
 configs/Linksprite_pcDuino3_defconfig  |  1 +
 configs/Linksprite_pcDuino_defconfig   |  1 +
 configs/M5253DEMO_defconfig|  1 +
 configs/M5253EVBE_defconfig|  1 +
 configs/M5272C3_defconfig  |  1 +
 configs/M5275EVB_defconfig |  1 +
 configs/M5282EVB_defconfig |  1 +
 configs/M54418TWR_defconfig|  1 +
 configs/M54418TWR_nand_mii_defconfig   |  1 +
 configs/M54418TWR_nand_rmii_defconfig  |  1 +
 configs/M54418TWR_nand_rmii_lowfreq_defconfig  |  1 +
 configs/M54418TWR_serial_mii_defconfig |  1 +
 configs/M54418TWR_serial_rmii_defconfig|  1 +
 configs/MIP405T_defconfig  |  1 +
 configs/MIP405_defconfig   |  1 +
 configs/MK808C_defconfig   |  1 +
 configs

[U-Boot] [PATCH 1/2] Kconfig: add CONFIG_BOOTDELAY

[Igor Grinberg]

Add CONFIG_BOOTDELAY to the Kconfig.
Default it to 3 seconds according to the majority of configs.

Signed-off-by: Igor Grinberg <grinb...@compulab.co.il>
Cc: Simon Glass <s...@chromium.org>
Cc: Masahiro Yamada <yamada.masah...@socionext.com>
---
 common/Kconfig | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/common/Kconfig b/common/Kconfig
index d98eb19..e13d255 100644
--- a/common/Kconfig
+++ b/common/Kconfig
@@ -100,6 +100,16 @@ config AUTOBOOT_STOP_STR_SHA256
  string / password matches a values that is encypted via
  a SHA256 hash and saved in the environment.
 
+config BOOTDELAY
+   int "Seconds to delay before autobooting"
+   default 3
+   help
+ Delay before automatically booting the default image;
+ set to -1 to disable autoboot.
+ set to -2 to autoboot with no delay and not check for abort
+ (even when CONFIG_ZERO_BOOTDELAY_CHECK is defined).
+ See doc/README.autoboot for additional information.
+
 endmenu
 
 comment "Commands"
-- 
2.4.9

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

[U-Boot] [PATCH 0/2] config: move CONFIG_CMD_BOOTZ to defconfig

[Igor Grinberg]

I've split the original patchset into several smaller patchsets,
as I'm not sure how long will it take me to finish them all.
This one deals with the CONFIG_CMD_BOOTZ.

Move the CONFIG_CMD_BOOTZ to defconfig way.

Igor Grinberg (2):
  Kconfig: add CONFIG_CMD_BOOTZ
  config: move CONFIG_CMD_BOOTZ to defconfig

 common/Kconfig | 6 ++
 configs/A10-OLinuXino-Lime_defconfig   | 1 +
 configs/A10s-OLinuXino-M_defconfig | 1 +
 configs/A13-OLinuXinoM_defconfig   | 1 +
 configs/A13-OLinuXino_defconfig| 1 +
 configs/A20-OLinuXino-Lime2_defconfig  | 1 +
 configs/A20-OLinuXino-Lime_defconfig   | 1 +
 configs/A20-OLinuXino_MICRO_defconfig  | 1 +
 configs/A20-Olimex-SOM-EVB_defconfig   | 1 +
 configs/Ainol_AW1_defconfig| 1 +
 configs/Ampe_A76_defconfig | 1 +
 configs/Auxtek-T003_defconfig  | 1 +
 configs/Auxtek-T004_defconfig  | 1 +
 configs/Bananapi_defconfig | 1 +
 configs/Bananapro_defconfig| 1 +
 configs/CSQ_CS908_defconfig| 1 +
 configs/Chuwi_V7_CW0825_defconfig  | 1 +
 configs/Colombus_defconfig | 1 +
 configs/Cubieboard2_defconfig  | 1 +
 configs/Cubieboard_defconfig   | 1 +
 configs/Cubietruck_defconfig   | 1 +
 configs/Et_q8_v1_6_defconfig   | 1 +
 configs/Hummingbird_A31_defconfig  | 1 +
 configs/Hyundai_A7HD_defconfig | 1 +
 configs/Ippo_q8h_v1_2_a33_1024x600_defconfig   | 1 +
 configs/Ippo_q8h_v1_2_defconfig| 1 +
 configs/Ippo_q8h_v5_defconfig  | 1 +
 configs/Linksprite_pcDuino3_Nano_defconfig | 1 +
 configs/Linksprite_pcDuino3_defconfig  | 1 +
 configs/Linksprite_pcDuino_defconfig   | 1 +
 configs/MK808C_defconfig   | 1 +
 configs/MSI_Primo73_defconfig  | 1 +
 configs/MSI_Primo81_defconfig  | 1 +
 configs/Marsboard_A10_defconfig| 1 +
 configs/Mele_A1000G_quad_defconfig | 1 +
 configs/Mele_A1000_defconfig   | 1 +
 configs/Mele_I7_defconfig  | 1 +
 configs/Mele_M3_defconfig  | 1 +
 configs/Mele_M5_defconfig  | 1 +
 configs/Mele_M9_defconfig  | 1 +
 configs/Merrii_A80_Optimus_defconfig   | 1 +
 configs/Mini-X_defconfig   | 1 +
 configs/Orangepi_defconfig | 1 +
 configs/Orangepi_mini_defconfig| 1 +
 configs/Sinlinx_SinA33_defconfig   | 1 +
 configs/TZX-Q8-713B7_defconfig | 1 +
 configs/UTOO_P66_defconfig | 1 +
 configs/Wexler_TAB7200_defconfig   | 1 +
 configs/Wits_Pro_A20_DKT_defconfig | 1 +
 configs/Wobo_i5_defconfig  | 1 +
 configs/Yones_Toptech_BD1078_defconfig | 1 +
 configs/alt_defconfig  | 1 +
 configs/am335x_baltos_defconfig| 1 +
 configs/am335x_boneblack_defconfig | 1 +
 configs/am335x_boneblack_vboot_defconfig   | 1 +
 configs/am335x_evm_defconfig   | 1 +
 configs/am335x_evm_nor_defconfig   | 1 +
 configs/am335x_evm_norboot_defconfig   | 1 +
 configs/am335x_evm_spiboot_defconfig   | 1 +
 configs/am335x_evm_usbspl_defconfig| 1 +
 configs/am335x_gp_evm_defconfig| 1 +
 configs/am335x_igep0033_defconfig  | 1 +
 configs/am335x_sl50_defconfig  | 1 +
 configs/am43xx_evm_defconfig   | 1 +
 configs/am43xx_evm_ethboot_defconfig   | 1 +
 configs/am43xx_evm_qspiboot_defconfig  | 1 +
 configs/am43xx_evm_usbhost_boot_defconfig  | 1 +
 configs/apalis_t30_defconfig   | 1 +
 configs/aristainetos2_defconfig| 1 +
 configs/aristainetos2b_defconfig   | 1 +
 configs/aristainetos_defconfig | 1 +
 configs/armadillo-800eva_defconfig | 1 +
 configs/arndale_defconfig  | 1 +
 configs/at91rm9200ek_defconfig | 1 +
 configs/at91rm9200ek_ram_defconfig | 1 +
 configs/at91sam9260ek_dataflash_cs0_defconfig  | 1 +
 configs/at91sam9260ek_dataflash_cs1_defconfig  | 1 +
 configs/at91sam9260ek_nandflash_defconfig  | 1 +
 configs/at91sam9263ek_dataflash_cs0_defconfig  | 1 +
 configs/at91sam9263ek_dataflash_defconfig  | 1 +
 configs/at91sam9263ek_nandflash_defconfig  | 1 +
 configs/at91sam9263ek_norflash_boot_defconfig  | 1 +
 configs/at91sam9263ek_norflash_defconfig   | 1 +
 configs/at91sam9g20ek_2mmc_defconfig   | 1 +
 configs/at91sam9g20ek_2mmc_nandflash_defconfig | 1 +
 configs/at91sam9g20ek_dataflash_cs0_defconfig  | 1 +
 configs

[U-Boot] [PATCH 1/2] Kconfig: add CONFIG_CMD_BOOTZ

[Igor Grinberg]

Add CONFIG_CMD_BOOTZ to the Kconfig.
Since the CONFIG_CMD_BOOTZ cannot live without the CONFIG_CMD_BOOTM,
make it select the CONFIG_CMD_BOOTM.

Signed-off-by: Igor Grinberg <grinb...@compulab.co.il>
Cc: Masahiro Yamada <yamada.masah...@socionext.com>
Cc: Simon Glass <s...@chromium.org>
---
 common/Kconfig | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/common/Kconfig b/common/Kconfig
index 2c42b8e..d98eb19 100644
--- a/common/Kconfig
+++ b/common/Kconfig
@@ -148,6 +148,12 @@ config CMD_BOOTM
help
  Boot an application image from the memory.
 
+config CMD_BOOTZ
+   bool "bootz"
+   select CMD_BOOTM
+   help
+ Boot a Linux kernel zImage from the memory.
+
 config CMD_GO
bool "go"
default y
-- 
2.4.9

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

Re: [U-Boot] [PATCH] omap3: cm-t3517: add board specific get_board_rev()

[Igor Grinberg]

Hi Dima,

On 10/08/15 14:20, Dmitry Lifshitz wrote:
> CM-T3517 has several HW revisions.
> Add board specific get_board_rev() callback to retrieve revision number.
> 
> Signed-off-by: Dmitry Lifshitz 
> ---
>  board/compulab/cm_t3517/cm_t3517.c |9 +
>  1 files changed, 9 insertions(+), 0 deletions(-)
> 
> diff --git a/board/compulab/cm_t3517/cm_t3517.c 
> b/board/compulab/cm_t3517/cm_t3517.c
> index 03b2bad..bf7dec2 100644
> --- a/board/compulab/cm_t3517/cm_t3517.c
> +++ b/board/compulab/cm_t3517/cm_t3517.c
> @@ -98,6 +98,15 @@ int board_init(void)
>   return 0;
>  }
>  
> +/*
> + * Routine: get_board_rev
> + * Description: read system revision
> + */
> +u32 get_board_rev(void)
> +{
> + return cl_eeprom_get_board_rev();

will this compile on the recent U-Boot?

> +};
> +
>  int misc_init_r(void)
>  {
>   cl_print_pcb_info();
> 

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

Re: [U-Boot] [PATCH 04/10] Kconfig: add CONFIG_CMD_BOOTZ

[Igor Grinberg]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

On 09/29/15 20:11, Tom Rini wrote:
> On Tue, Sep 29, 2015 at 07:25:02PM +0300, Igor Grinberg wrote:
>> ping x2
>>
>> On 09/24/15 12:57, Igor Grinberg wrote:
>>> ping.
>>>
>>> On 08/26/15 17:54, Igor Grinberg wrote:
>>>> Add CONFIG_CMD_BOOTZ to the Kconfig.
>>>> Since the CONFIG_CMD_BOOTZ cannot live without the CONFIG_CMD_BOOTM,
>>>> make it select the CONFIG_CMD_BOOTM.
>>>>
>>>> Signed-off-by: Igor Grinberg <grinb...@compulab.co.il>
>>>> Cc: Masahiro Yamada <yamada.masah...@socionext.com>
>>>> Cc: Simon Glass <s...@chromium.org>
>>>> ---
>>>>  common/Kconfig | 6 ++
>>>>  1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/common/Kconfig b/common/Kconfig
>>>> index ecbf6cb..b7791bd 100644
>>>> --- a/common/Kconfig
>>>> +++ b/common/Kconfig
>>>> @@ -165,6 +165,12 @@ config CMD_BOOTM
>>>>help
>>>>  Boot an application image from the memory.
>>>>  
>>>> +config CMD_BOOTZ
>>>> +  bool "bootz"
>>>> +  select CMD_BOOTM
>>>> +  help
>>>> +Boot a Linux kernel zImage from the memory.
>>>> +
>>>>  config CMD_GO
>>>>bool "go"
>>>>default y
> 
> I figured you were tackling this later with moveconfig.py, no?

Aahh, yes that is true. Sorry about that.


- -- 
Regards,
Igor.
-BEGIN PGP SIGNATURE-
Version: GnuPG v2

iQIcBAEBCAAGBQJWC5S1AAoJEBDE8YO64Efal6QQAI1/cg7ftNL0Kcyvkp7QMBKy
+KyLmJZQ+dVLjATzXJWLScaSQp3AuHfJgvoEfPIALZMxZmyXPuAWroP/hoBgNJdA
koo6Mk2x0ckMfqR4VVk0xF7peIUq528R9b+OVMrXCnum2RwOyz5+4IvWuVVmzUKa
B4a+7YACjO74M33V+FhQLMP4NJwtmBA+2Jwb09kitR26uGV/nenYDxCtSVHClO2F
4vZWYMw36Ex0H2nZP3PY7U2mDIyEqBFuCgEgve/g0HIz0BIKbxP8viRq5zVQMLcg
edJpn2/9eRKLfCBcDuZxMXqtx7Ox3fGK22OanhE3IGGvQk3EWu0zUWlmmnagb5Ja
8VwR2MwVCtQj+hCAY76dqcD9Dzv5iH/vygPvnHnwTjCEDXGH1FyVuI5emIWFqa8S
RvYVQyB8qMImYAI60oFwrIeg5D848fIwhJ3Gon1ydkZCA/eWE1KDb0Q/V7OgWuZq
rJGPWtGpJWezuUIelU4GoN0QLKhPnuhREpa3Bgtb1JvN72r0LMlVEq15GHSn3w1a
IRlIGSMkajtRO2l9ljfi0L0FghLBfdcMCoW0bsKp3eEdux3WX3AQs4+26jfZVd4L
E9uC5Si5YYdE3R4TSkjEYTVsVwDaSzzQdf3q6HqAfc62lD0fXFYYUMYnN0pHdH5b
S0W0cgxgtCs3yTzXiQIE
=DhyG
-END PGP SIGNATURE-
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 05/10] ti: omap3: config: remove 1 from boolean define

[Igor Grinberg]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

On 09/29/15 20:12, Tom Rini wrote:
> On Tue, Sep 29, 2015 at 07:25:27PM +0300, Igor Grinberg wrote:
> 
>> ping
>>
>> On 09/24/15 12:56, Igor Grinberg wrote:
>>> Hi Tom,
>>>
>>> On 08/26/15 17:54, Igor Grinberg wrote:
>>>> CONFIG_TWL4030_POWER is a boolean define variable. It is either defined
>>>> or not defined and should not have a value assigned to it.
>>>> Remove the value.
>>>>
>>>> Signed-off-by: Igor Grinberg <grinb...@compulab.co.il>
>>>
>>> I think this one can also be applied, no?
>>>
>>>> ---
>>>>  include/configs/ti_omap3_common.h | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/configs/ti_omap3_common.h 
>>>> b/include/configs/ti_omap3_common.h
>>>> index be231a5..e399a87 100644
>>>> --- a/include/configs/ti_omap3_common.h
>>>> +++ b/include/configs/ti_omap3_common.h
>>>> @@ -66,7 +66,7 @@
>>>>  #define CONFIG_SYS_MONITOR_LEN(256 << 10)
>>>>  
>>>>  /* TWL4030 */
>>>> -#define CONFIG_TWL4030_POWER  1
>>>> +#define CONFIG_TWL4030_POWER
>>>>  
>>>>  /* SPL */
>>>>  #define CONFIG_SPL_TEXT_BASE  0x40200800
> 
> Reviewed-by: Tom Rini <tr...@konsulko.com>
> 
> And I figured you just re-post this as part of v2 as it's trivially
> correct but also a tiny thing in a bigger series.

Well, its relation is only by the affected file, so it will help me more
(less things to carry for v2) if you can apply it already.

Thanks!

- -- 
Regards,
Igor.
-BEGIN PGP SIGNATURE-
Version: GnuPG v2

iQIcBAEBCAAGBQJWC5U9AAoJEBDE8YO64EfaguIP/00wC4X6lL89yIlHpWM6qGZ9
p60KzPvbmfXoa86UyWNrmGBNKkP1i86ECNnRk8CRyzTWKt9f7o9RlceLWfqIYK0G
wkAEk724G+FpYAqgTgkVB/BBF/mimm9JbjdoA5Tx58MDaK3bP5zKQ4J4a1mReZa9
fW/remBNgOtV++QGQBZMzPUWowAc4YQNZ9ZlXN0ZhSqoGprVQAsuQdJybfIIyF+i
GSctjK77ObGpwf7NLS+ft8290f1eo/Jd+KMXFs7m3OHXo+yFvjq9FbET8DGYSy2N
pANLDbxmd3Mgc1PA4XZNnGt6BJ04vNlFhWUW9I2+FZTMldegyblUSc1Fug5mWqWa
DtxgE8YSlEjLbrrgEUlZ6zk6Xmfb5Nz9ndBiNUfduOt3YOUdnR5SJThF7bjq3UXz
LN+fO6K1HK/EY8jc9qMb7Jme4hx1/pHcy4Vzx7Zh/9m4u9mZkRXJnqITxcS2+hhs
H00v3XuhTfjNybgmrunvclPFlxvicb8P7BBTDKhmPzb2mV19g+AhrmmHjIbPhvVh
QBMv0mkQsG+KYqD9ruj+OLT3xMQ99skipX3uUUY3z90u6RGMjRWKvlSrLSmr56SE
AdmnZw6Al/WTLuvi66S3WE3jmjF0f5TKBPa6uIVLyxgdC9jDJZNA1iSDIAX9s5wZ
LDwX8bcLf32c7BPKBe1/
=M25l
-END PGP SIGNATURE-
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 05/10] ti: omap3: config: remove 1 from boolean define

[Igor Grinberg]

ping

On 09/24/15 12:56, Igor Grinberg wrote:
> Hi Tom,
> 
> On 08/26/15 17:54, Igor Grinberg wrote:
>> CONFIG_TWL4030_POWER is a boolean define variable. It is either defined
>> or not defined and should not have a value assigned to it.
>> Remove the value.
>>
>> Signed-off-by: Igor Grinberg <grinb...@compulab.co.il>
> 
> I think this one can also be applied, no?
> 
>> ---
>>  include/configs/ti_omap3_common.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/configs/ti_omap3_common.h 
>> b/include/configs/ti_omap3_common.h
>> index be231a5..e399a87 100644
>> --- a/include/configs/ti_omap3_common.h
>> +++ b/include/configs/ti_omap3_common.h
>> @@ -66,7 +66,7 @@
>>  #define CONFIG_SYS_MONITOR_LEN  (256 << 10)
>>  
>>  /* TWL4030 */
>> -#define CONFIG_TWL4030_POWER1
>> +#define CONFIG_TWL4030_POWER
>>  
>>  /* SPL */
>>  #define CONFIG_SPL_TEXT_BASE0x40200800
>>
> 

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

Re: [U-Boot] [PATCH 04/10] Kconfig: add CONFIG_CMD_BOOTZ

[Igor Grinberg]

ping x2

On 09/24/15 12:57, Igor Grinberg wrote:
> ping.
> 
> On 08/26/15 17:54, Igor Grinberg wrote:
>> Add CONFIG_CMD_BOOTZ to the Kconfig.
>> Since the CONFIG_CMD_BOOTZ cannot live without the CONFIG_CMD_BOOTM,
>> make it select the CONFIG_CMD_BOOTM.
>>
>> Signed-off-by: Igor Grinberg <grinb...@compulab.co.il>
>> Cc: Masahiro Yamada <yamada.masah...@socionext.com>
>> Cc: Simon Glass <s...@chromium.org>
>> ---
>>  common/Kconfig | 6 ++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/common/Kconfig b/common/Kconfig
>> index ecbf6cb..b7791bd 100644
>> --- a/common/Kconfig
>> +++ b/common/Kconfig
>> @@ -165,6 +165,12 @@ config CMD_BOOTM
>>  help
>>Boot an application image from the memory.
>>  
>> +config CMD_BOOTZ
>> +bool "bootz"
>> +select CMD_BOOTM
>> +help
>> +  Boot a Linux kernel zImage from the memory.
>> +
>>  config CMD_GO
>>  bool "go"
>>  default y
>>
> 

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

Re: [U-Boot] [PATCH 00/10] Kconfig and ti omap configs tidy up

[Igor Grinberg]

Hi Tom,

On 08/26/15 17:54, Igor Grinberg wrote:
> The patch set is aimed to transferring several config options
> from the old config.h configuration style to the new Kconfig
> framework and also move some of the existing (in Kconfig) ones
> from config.h style to the defconfig style in the ti-omap land.
> 
> It is based on the 7d31c6a (Merge git://git.denx.de/u-boot-pxa) commit
> upstream.
> 
> Igor Grinberg (10):
>   configs: remove remnants of CONFIG_SYS_NAND_QUIET_TEST
>   Kconfig: fix typo in CONFIG_FIT description

Can the above two be applied please?
I'm going to rework the below ones (once I've got some time...),
but above two can be applied.

Thanks!

>   Kconfig: add CONFIG_BOOTDELAY
>   Kconfig: add CONFIG_CMD_BOOTZ
>   ti: omap3: config: remove 1 from boolean define
>   ti: omap/am: move CONFIG_HUSH_PARSER to defconfig
>   ti: omap/am: move CONFIG_CMD_BOOTZ to defconfig
>   ti: omap/am: move CONFIG_CMD_NAND to defconfig
>   ti: omap/am: move CONFIG_CMD_I2C to defconfig
>   ti: omap/am: move CONFIG_BOOTDELAY to defconfig

[...]

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

Re: [U-Boot] [PATCH 05/10] ti: omap3: config: remove 1 from boolean define

[Igor Grinberg]

Hi Tom,

On 08/26/15 17:54, Igor Grinberg wrote:
> CONFIG_TWL4030_POWER is a boolean define variable. It is either defined
> or not defined and should not have a value assigned to it.
> Remove the value.
> 
> Signed-off-by: Igor Grinberg <grinb...@compulab.co.il>

I think this one can also be applied, no?

> ---
>  include/configs/ti_omap3_common.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/configs/ti_omap3_common.h 
> b/include/configs/ti_omap3_common.h
> index be231a5..e399a87 100644
> --- a/include/configs/ti_omap3_common.h
> +++ b/include/configs/ti_omap3_common.h
> @@ -66,7 +66,7 @@
>  #define CONFIG_SYS_MONITOR_LEN   (256 << 10)
>  
>  /* TWL4030 */
> -#define CONFIG_TWL4030_POWER 1
> +#define CONFIG_TWL4030_POWER
>  
>  /* SPL */
>  #define CONFIG_SPL_TEXT_BASE 0x40200800
> 

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

Re: [U-Boot] [PATCH 04/10] Kconfig: add CONFIG_CMD_BOOTZ

[Igor Grinberg]

ping.

On 08/26/15 17:54, Igor Grinberg wrote:
> Add CONFIG_CMD_BOOTZ to the Kconfig.
> Since the CONFIG_CMD_BOOTZ cannot live without the CONFIG_CMD_BOOTM,
> make it select the CONFIG_CMD_BOOTM.
> 
> Signed-off-by: Igor Grinberg <grinb...@compulab.co.il>
> Cc: Masahiro Yamada <yamada.masah...@socionext.com>
> Cc: Simon Glass <s...@chromium.org>
> ---
>  common/Kconfig | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/common/Kconfig b/common/Kconfig
> index ecbf6cb..b7791bd 100644
> --- a/common/Kconfig
> +++ b/common/Kconfig
> @@ -165,6 +165,12 @@ config CMD_BOOTM
>   help
> Boot an application image from the memory.
>  
> +config CMD_BOOTZ
> + bool "bootz"
> + select CMD_BOOTM
> + help
> +   Boot a Linux kernel zImage from the memory.
> +
>  config CMD_GO
>   bool "go"
>   default y
> 

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

Re: [U-Boot] [PATCH V2 0/4] cm-fx6 updates for Utilite

[Igor Grinberg]

Hi Stefano,

On 09/13/15 11:36, Stefano Babic wrote:
> 
> 
> On 06/09/2015 10:48, Nikita Kiryanov wrote:
>> This series provides a fix necessary for early models of Utilite, a miniature
>> desktop based on CM-FX6. It implements a dynamic modification to the device 
>> tree
>> that is necessary for mmc boot.
>>
>> Cc: Stefano Babic <sba...@denx.de>
>> Cc: Igor Grinberg <grinb...@compulab.co.il>
>>
>> Changes in V2:
>>  - New patch: compulab: eeprom: propagate error value in read_mac_addr()
>>  - s/BOARD_PRODUCT_NAME_*/PRODUCT_NAME_*
>>  - rewrote cl_eeprom_get_product_name() to take a buffer parameter and
>>added documentation
>>  - #define USDHC3_PATH instead of const variable usdhc3_path
>>  - Do not update device tree on eeprom read failures in a more explicit
>>way
>>
>> Nikita Kiryanov (4):
>>   compulab: eeprom: select i2c bus when querying for board rev
>>   compulab: eeprom: propagate error value in read_mac_addr()
>>   compulab: eeprom: add support for obtaining product name
>>   arm: mx6: cm-fx6: modify device tree for old revisions of utilite
>>
>>  board/compulab/cm_fx6/cm_fx6.c | 22 +-
>>  board/compulab/cm_t35/cm_t35.c |  2 +-
>>  board/compulab/common/eeprom.c | 40 
>>  board/compulab/common/eeprom.h | 10 --
>>  4 files changed, 66 insertions(+), 8 deletions(-)
>>
> 
> 
> Applied to u-boot-imx, thanks !

IMO, this was a bit early...
We haven't received any answer from Nikita yet.
I tend to think that he is preparing a newer version.
Nikita?

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

Re: [U-Boot] [PATCH v2 3/3] omap3: cm-t3517: change environment size

[Igor Grinberg]

On 09/09/15 11:25, Dmitry Lifshitz wrote:
> Mainline CM-T3517 U-Boot environment size differs from that one
> shipped with CM-T3517 boards.
> 
> Update environment size, to avoid backward compatibility issues.
> 
> Signed-off-by: Dmitry Lifshitz <lifsh...@compulab.co.il>

Acked-by: Igor Grinberg <grinb...@compulab.co.il>

> ---
> 
>  Changes in v2:
> 
> * Removed unjustified CONFIG_SYS_MALLOC_LEN change.
> 
>  include/configs/cm_t3517.h |2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/include/configs/cm_t3517.h b/include/configs/cm_t3517.h
> index 2d16799..fbc10dd 100644
> --- a/include/configs/cm_t3517.h
> +++ b/include/configs/cm_t3517.h
> @@ -66,7 +66,7 @@
>  /*
>   * Size of malloc() pool
>   */
> -#define CONFIG_ENV_SIZE  (16 << 10)  /* 16 KiB */
> +#define CONFIG_ENV_SIZE  (128 << 10) /* 128 KiB */
>  #define CONFIG_SYS_MALLOC_LEN(CONFIG_ENV_SIZE + (128 << 10))
>  
>  /*
> 

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

Re: [U-Boot] [PATCH 1/2] mx6: remove SYS_SOC from board Kconfig

[Igor Grinberg]

On 09/07/15 09:59, Peng Fan wrote:
> Remove duplicated SYS_SOC Kconfig entry from board Kconfig,
> because we have this entry in arch/arm/cpu/armv7/mx6/Kconfig.
> 
> Signed-off-by: Peng Fan <peng@freescale.com>
> Cc: Stefano Babic <sba...@denx.de>
> Cc: Heiko Schocher <h...@denx.de>
> Cc: Christian Gmeiner <christian.gmei...@gmail.com>
> Cc: Stefan Roese <s...@denx.de>
> Cc: Troy Kisky <troy.ki...@boundarydevices.com>
> Cc: Nikita Kiryanov <nik...@compulab.co.il>
> Cc: "Eric Bénard" <e...@eukrea.com>
> Cc: Fabio Estevam <fabio.este...@freescale.com>
> Cc: Tim Harvey <thar...@gateworks.com>
> Cc: Marek Vasut <ma...@denx.de>
> Cc: Markus Niebel <markus.nie...@tq-group.com>
> Cc: Otavio Salvador <ota...@ossystems.com.br>
> ---
>  board/aristainetos/Kconfig  | 9 -
>  board/bachmann/ot1200/Kconfig   | 3 ---
>  board/barco/platinum/Kconfig| 6 --
>  board/barco/titanium/Kconfig| 3 ---
>  board/boundary/nitrogen6x/Kconfig   | 3 ---
>  board/compulab/cm_fx6/Kconfig   | 3 ---

Acked-by: Igor Grinberg <grinb...@compulab.co.il>

[...]

> diff --git a/board/compulab/cm_fx6/Kconfig b/board/compulab/cm_fx6/Kconfig
> index 508c21f..59070c5 100644
> --- a/board/compulab/cm_fx6/Kconfig
> +++ b/board/compulab/cm_fx6/Kconfig
> @@ -6,9 +6,6 @@ config SYS_BOARD
>  config SYS_VENDOR
>   default "compulab"
>  
> -config SYS_SOC
> - default "mx6"
> -
>  config SYS_CONFIG_NAME
>   default "cm_fx6"
>  

[...]

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

Re: [U-Boot] [PATCH] omap3: cm-t3517: define CONFIG_MACH_TYPE

[Igor Grinberg]

On 09/09/15 11:27, Dmitry Lifshitz wrote:
> Define CONFIG_MACH_TYPE to allow non DT Linux boot.
> 
> Signed-off-by: Dmitry Lifshitz <lifsh...@compulab.co.il>

Acked-by: Igor Grinberg <grinb...@compulab.co.il>

> ---
>  include/configs/cm_t3517.h |2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/include/configs/cm_t3517.h b/include/configs/cm_t3517.h
> index fbc10dd..27c023c 100644
> --- a/include/configs/cm_t3517.h
> +++ b/include/configs/cm_t3517.h
> @@ -36,6 +36,8 @@
>  #include /* get chip and board defs */
>  #include 
>  
> +#define CONFIG_MACH_TYPEMACH_TYPE_CM_T3517
> +
>  /*
>   * Display CPU and Board information
>   */
> 

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

Re: [U-Boot] [PATCH 3/3] omap3: cm-t3517: change environment size

[Igor Grinberg]

Hi Dima,

On 09/08/15 09:50, Dmitry Lifshitz wrote:
> Mainline CM-T3517 U-Boot environment size differs from the that one
> shipped with CM-T3517 boards.

There is some strangeness in the above sentence...

> 
> Update environment size, to avoid backward compatability issues.

s/compatability/compatibility/

> 
> Signed-off-by: Dmitry Lifshitz 
> ---
>  include/configs/cm_t3517.h |4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/configs/cm_t3517.h b/include/configs/cm_t3517.h
> index 2d16799..36488d7 100644
> --- a/include/configs/cm_t3517.h
> +++ b/include/configs/cm_t3517.h
> @@ -66,8 +66,8 @@
>  /*
>   * Size of malloc() pool
>   */
> -#define CONFIG_ENV_SIZE  (16 << 10)  /* 16 KiB */
> -#define CONFIG_SYS_MALLOC_LEN(CONFIG_ENV_SIZE + (128 << 10))
> +#define CONFIG_ENV_SIZE  (128 << 10) /* 128 KiB */
> +#define CONFIG_SYS_MALLOC_LEN(CONFIG_ENV_SIZE + (512 << 10))

Why do you change CONFIG_SYS_MALLOC_LEN?
Isn't it not enough?
If so, I would expect to see some explanation in the commit message.


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

Re: [U-Boot] [PATCH 1/3] omap3: cm-t3517: enable 'netretry' and setup timeout

[Igor Grinberg]

On 09/08/15 09:50, Dmitry Lifshitz wrote:
> SBC-T3517 evaluation board has two Eth interfaces.
> Enable network retry of another interface if the default if failed
> or disconnected.
> 
> Add 'netretry=yes' in the default env. Setup relevant
> timeout values in the board config file.
> 
> Signed-off-by: Dmitry Lifshitz <lifsh...@compulab.co.il>

Acked-by: Igor Grinberg <grinb...@compulab.co.il>

> ---
>  include/configs/cm_t3517.h |3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/include/configs/cm_t3517.h b/include/configs/cm_t3517.h
> index a8d0b97..2d16799 100644
> --- a/include/configs/cm_t3517.h
> +++ b/include/configs/cm_t3517.h
> @@ -170,6 +170,7 @@
>   "loadaddr=0x8200\0" \
>   "baudrate=115200\0" \
>   "console=ttyO2,115200n8\0" \
> + "netretry=yes\0" \
>   "mpurate=auto\0" \
>   "vram=12M\0" \
>   "dvimode=1024x768MR-16@60\0" \
> @@ -275,6 +276,8 @@
>  #define CONFIG_SMC911X
>  #define CONFIG_SMC911X_32_BIT
>  #define CONFIG_SMC911X_BASE  (0x2C00 + (16 << 20))
> +#define CONFIG_ARP_TIMEOUT   200UL
> +#define CONFIG_NET_RETRY_COUNT   5
>  #endif /* CONFIG_CMD_NET */
>  
>  /* additions for new relocation code, must be added to all boards */
> 

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

Re: [U-Boot] [PATCH 2/3] omap3: cm-t3517: fix MMC1 pinmux

[Igor Grinberg]

On 09/08/15 09:50, Dmitry Lifshitz wrote:
> Fix MMC1 pinmux setup, thus enable SD/MMC card support with CM-T3517.
> 
> Signed-off-by: Dmitry Lifshitz <lifsh...@compulab.co.il>

Acked-by: Igor Grinberg <grinb...@compulab.co.il>

> ---
>  board/compulab/cm_t3517/mux.c |   12 ++--
>  1 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/board/compulab/cm_t3517/mux.c b/board/compulab/cm_t3517/mux.c
> index 88ce2cc..f31f19e 100644
> --- a/board/compulab/cm_t3517/mux.c
> +++ b/board/compulab/cm_t3517/mux.c
> @@ -121,12 +121,12 @@ void set_muxconf_regs(void)
>   MUX_VAL(CP(UART2_RX),   (IEN  | PTD | EN  | M4)); /*GPIO_147*/
>  
>   /* MMC1 */
> - MUX_VAL(CP(MMC1_CLK),   (IDIS | PTU | EN  | M0));
> - MUX_VAL(CP(MMC1_CMD),   (IEN  | PTU | EN  | M0));
> - MUX_VAL(CP(MMC1_DAT0),  (IEN  | PTU | EN  | M0));
> - MUX_VAL(CP(MMC1_DAT1),  (IEN  | PTU | EN  | M0));
> - MUX_VAL(CP(MMC1_DAT2),  (IEN  | PTU | EN  | M0));
> - MUX_VAL(CP(MMC1_DAT3),  (IEN  | PTU | EN  | M0));
> + MUX_VAL(CP(MMC1_CLK),   (IEN  | PTU | EN  | M0));
> + MUX_VAL(CP(MMC1_CMD),   (IEN  | PTU | DIS  | M0));
> + MUX_VAL(CP(MMC1_DAT0),  (IEN  | PTU | DIS  | M0));
> + MUX_VAL(CP(MMC1_DAT1),  (IEN  | PTU | DIS  | M0));
> + MUX_VAL(CP(MMC1_DAT2),  (IEN  | PTU | DIS  | M0));
> + MUX_VAL(CP(MMC1_DAT3),  (IEN  | PTU | DIS  | M0));
>  
>   /* DSS */
>   MUX_VAL(CP(DSS_PCLK),   (IDIS | PTD | DIS | M0));
> 

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

Re: [U-Boot] [PATCH V2 3/4] compulab: eeprom: add support for obtaining product name

[Igor Grinberg]

Hi Nikita,

On 09/06/15 11:48, Nikita Kiryanov wrote:
> Introduce cl_eeprom_get_product_name() for obtaining product name
> from the eeprom.
> 
> Cc: Stefano Babic <sba...@denx.de>
> Cc: Igor Grinberg <grinb...@compulab.co.il>
> Signed-off-by: Nikita Kiryanov <nik...@compulab.co.il>
> ---
> Changes in V2:
>   - s/BOARD_PRODUCT_NAME_*/PRODUCT_NAME_*
>   - Added documentation
>   - rewrote cl_eeprom_get_product_name() to take a buffer parameter
> 
>  board/compulab/common/eeprom.c | 30 ++
>  board/compulab/common/eeprom.h |  6 ++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/board/compulab/common/eeprom.c b/board/compulab/common/eeprom.c
> index 9f18a3d..6304468 100644
> --- a/board/compulab/common/eeprom.c
> +++ b/board/compulab/common/eeprom.c
> @@ -9,6 +9,7 @@
>  
>  #include 
>  #include 
> +#include "eeprom.h"
>  
>  #ifndef CONFIG_SYS_I2C_EEPROM_ADDR
>  # define CONFIG_SYS_I2C_EEPROM_ADDR  0x50
> @@ -25,6 +26,8 @@
>  #define BOARD_REV_OFFSET 0
>  #define BOARD_REV_OFFSET_LEGACY  6
>  #define BOARD_REV_SIZE   2
> +#define PRODUCT_NAME_OFFSET  128
> +#define PRODUCT_NAME_SIZE16
>  #define MAC_ADDR_OFFSET  4
>  #define MAC_ADDR_OFFSET_LEGACY   0

It seems that the time has come to move the above constants out of this file
into the eeprom.h, so they can be used by users of this "lib".
Don't you think?

>  
> @@ -151,3 +154,30 @@ u32 cl_eeprom_get_board_rev(uint eeprom_bus)
>  
>   return board_rev;
>  };
> +
> +/*
> + * Routine: cl_eeprom_get_board_rev
> + * Description: read system revision from eeprom

Seems like you have a successful copy/paste problem ;-)

> + *
> + * @buf: buffer to store the product name
> + * @eeprom_bus: i2c bus num of the eeprom
> + *
> + * @return: 0 on success, < 0 on failure
> + */
> +int cl_eeprom_get_product_name(uchar *buf, uint eeprom_bus)
> +{
> + int err;
> +
> + if (buf == NULL)
> + return -EINVAL;

I think that this check is not really necessary.
If someone passes NULL instead of buf - let it crash, no?

> +
> + err = cl_eeprom_setup(eeprom_bus);
> + if (err)
> + return err;
> +
> + err = cl_eeprom_read(PRODUCT_NAME_OFFSET, buf, PRODUCT_NAME_SIZE);
> + if (!err) /* Protect ourselves from invalid data (unterminated str) */

Why do you need the above check?
I think, you can just write '\0' anyway, no?

> + buf[PRODUCT_NAME_SIZE - 1] = '\0';
> +
> + return err;
> +}
> diff --git a/board/compulab/common/eeprom.h b/board/compulab/common/eeprom.h
> index e74c379..c0b4739 100644
> --- a/board/compulab/common/eeprom.h
> +++ b/board/compulab/common/eeprom.h
> @@ -9,10 +9,12 @@
>  
>  #ifndef _EEPROM_
>  #define _EEPROM_
> +#include 
>  
>  #ifdef CONFIG_SYS_I2C
>  int cl_eeprom_read_mac_addr(uchar *buf, uint eeprom_bus);
>  u32 cl_eeprom_get_board_rev(uint eeprom_bus);
> +int cl_eeprom_get_product_name(uchar *buf, uint eeprom_bus);
>  #else
>  static inline int cl_eeprom_read_mac_addr(uchar *buf, uint eeprom_bus)
>  {
> @@ -22,6 +24,10 @@ static inline u32 cl_eeprom_get_board_rev(uint eeprom_bus)
>  {
>   return 0;
>  }
> +static inline int cl_eeprom_get_product_name(uchar *buf, uint eeprom_bus)
> +{
> + return -ENOSYS;
> +}
>  #endif
>  
>  #endif
> 

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

Re: [U-Boot] [PATCH V2 4/4] arm: mx6: cm-fx6: modify device tree for old revisions of utilite

[Igor Grinberg]

On 09/06/15 11:48, Nikita Kiryanov wrote:
> Old revisions of Utilite (a miniature PC based on cm-fx6) do not have
> a card detect for mmc, and thus the kernel needs to be told that
> there's a persistent storage on usdhc3 to force it to probe the mmc
> card.
> 
> Check the baseboard revision and modify the device tree accordingly
> if needed.
> 
> Cc: Stefano Babic <sba...@denx.de>
> Cc: Igor Grinberg <grinb...@compulab.co.il>
> Signed-off-by: Nikita Kiryanov <nik...@compulab.co.il>
> ---
> Changes in V2:
>   - #define USDHC3_PATH instead of const variable usdhc3_path
>   - Do not update device tree on eeprom read failures in a more explicit
> way
> 
>  board/compulab/cm_fx6/cm_fx6.c | 20 
>  1 file changed, 20 insertions(+)
> 
> diff --git a/board/compulab/cm_fx6/cm_fx6.c b/board/compulab/cm_fx6/cm_fx6.c
> index 572111d..a21e7b0 100644
> --- a/board/compulab/cm_fx6/cm_fx6.c
> +++ b/board/compulab/cm_fx6/cm_fx6.c
> @@ -580,9 +580,14 @@ int cm_fx6_setup_ecspi(void) { return 0; }
>  #endif
>  
>  #ifdef CONFIG_OF_BOARD_SETUP
> +#define USDHC3_PATH  "/soc/aips-bus@0210/usdhc@02198000/"
>  int ft_board_setup(void *blob, bd_t *bd)
>  {
> + u32 baseboard_rev;
> + int nodeoffset;
>   uint8_t enetaddr[6];
> + char baseboard_name[16];

That would probably have the actual size (macro).

> + int err;
>  
>   /* MAC addr */
>   if (eth_getenv_enetaddr("ethaddr", enetaddr)) {
> @@ -596,6 +601,21 @@ int ft_board_setup(void *blob, bd_t *bd)
>enetaddr, 6, 1);
>   }
>  
> + baseboard_rev = cl_eeprom_get_board_rev(0);
> + err = cl_eeprom_get_product_name((uchar *)baseboard_name, 0);

I would replace the 0 with a define explaining what that is.

> + if (err || baseboard_rev == 0)
> + return 0; /* Assume not an early revision SB-FX6m baseboard */
> +
> + if (!strncmp("SB-FX6m", baseboard_name, 7) && baseboard_rev <= 120) {
> + fdt_shrink_to_minimum(blob); /* Make room for new properties */
> + nodeoffset = fdt_path_offset(blob, USDHC3_PATH);
> + fdt_delprop(blob, nodeoffset, "cd-gpios");
> + fdt_find_and_setprop(blob, USDHC3_PATH, "non-removable",
> +  NULL, 0, 1);
> + fdt_find_and_setprop(blob, USDHC3_PATH, "keep-power-in-suspend",
> +  NULL, 0, 1);
> + }
> +
>   return 0;
>  }
>  #endif
> 

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

Re: [U-Boot] [PATCH V2 2/4] compulab: eeprom: propagate error value in read_mac_addr()

[Igor Grinberg]

On 09/06/15 11:48, Nikita Kiryanov wrote:
> cl_eeprom_read_mac_addr() doesn't differentiate between success case and
> inability to access eeprom. Fix this by propagating the return value of
> cl_eeprom_setup().
> 
> Cc: Stefano Babic <sba...@denx.de>
> Cc: Igor Grinberg <grinb...@compulab.co.il>
> Signed-off-by: Nikita Kiryanov <nik...@compulab.co.il>

Acked-by: Igor Grinberg <grinb...@compulab.co.il>

> ---
> Changes in V2:
>   - New patch
> 
>  board/compulab/common/eeprom.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/board/compulab/common/eeprom.c b/board/compulab/common/eeprom.c
> index aaacd2e..9f18a3d 100644
> --- a/board/compulab/common/eeprom.c
> +++ b/board/compulab/common/eeprom.c
> @@ -105,9 +105,11 @@ void get_board_serial(struct tag_serialnr *serialnr)
>  int cl_eeprom_read_mac_addr(uchar *buf, uint eeprom_bus)
>  {
>   uint offset;
> + int err;
>  
> - if (cl_eeprom_setup(eeprom_bus))
> - return 0;
> + err = cl_eeprom_setup(eeprom_bus);
> + if (err)
> + return err;
>  
>   offset = (cl_eeprom_layout != LAYOUT_LEGACY) ?
>   MAC_ADDR_OFFSET : MAC_ADDR_OFFSET_LEGACY;
> 

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