On 12 December 2016 at 06:27, Simon Glass <s...@chromium.org> wrote:
> Hi Nathan,
>
> On 11 December 2016 at 08:58, Nathan Rossi <nat...@nathanrossi.com> wrote:
>> Add two functions for use by board implementations to decode the memory
>> banks of the /memory node so as to populate the global data with
>> ram_size and board info for memory banks.
>>
>> The fdtdec_setup_memory_size() function decodes the first memory bank
>> and sets up the gd->ram_size with the size of the memory bank. This
>> function should be called from the boards dram_init().
>>
>> The fdtdec_setup_memory_banksize() function decode the memory banks
>> (up to the CONFIG_NR_DRAM_BANKS) and populates the base address and size
>> into the gd->bd->bi_dram array of banks. This function should be called
>> from the boards dram_init_banksize().
>>
>> Signed-off-by: Nathan Rossi <nat...@nathanrossi.com>
>> Cc: Simon Glass <s...@chromium.org>
>> Cc: Michal Simek <mon...@monstr.eu>
>> ---
>> This implementation of decoding has been tested on zynq and zynqmp
>> boards with address/size cells of (1, 1), (1, 2), (2, 1), (2, 2) and
>> up to 2 memory banks.
>> ---
>> include/fdtdec.h | 25 +++++++++++++++++++++++++
>> lib/fdtdec.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 79 insertions(+)
>
> Reviewed-by: Simon Glass <s...@chromium.org>
>
> Please see nit below.
>
>>
>> diff --git a/include/fdtdec.h b/include/fdtdec.h
>> index 27887c8c21..59a204b571 100644
>> --- a/include/fdtdec.h
>> +++ b/include/fdtdec.h
>> @@ -976,6 +976,31 @@ struct display_timing {
>> */
>> int fdtdec_decode_display_timing(const void *blob, int node, int index,
>> struct display_timing *config);
>> +
>> +/**
>> + * fdtdec_setup_memory_size() - decode and setup gd->ram_size
>> + *
>> + * Decode the /memory 'reg' property to determine the size of the first
>> memory
>> + * bank, populate the global data with the size of the first bank of memory.
>> + * This function should be called from the boards dram_init().
>> + *
>> + * @return 0 if OK, -EINVAL if the /memory node or reg property is missing
>> or
>> + * invalid
>> + */
>> +int fdtdec_setup_memory_size(void);
>> +
>> +/**
>> + * fdtdec_setup_memory_banksize() - decode and populate gd->bd->bi_dram
>> + *
>> + * Decode the /memory 'reg' property to determine the address and size of
>> the
>> + * memory banks. Use this data to populate the global data board info with
>> the
>> + * phys address and size of memory banks. This function should be called
>> from
>> + * the boards dram_init_banksize().
>> + *
>> + * @return 0 if OK, negative on error
>
> Good to be specific, if e.g. it can only return -EINVAL.
I will update this based on the change below.
>
>> + */
>> +int fdtdec_setup_memory_banksize(void);
>> +
>> /**
>> * Set up the device tree ready for use
>> */
>> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
>> index 4e619c49a2..bc3be017b6 100644
>> --- a/lib/fdtdec.c
>> +++ b/lib/fdtdec.c
>> @@ -1174,6 +1174,60 @@ int fdtdec_decode_display_timing(const void *blob,
>> int parent, int index,
>> return ret;
>> }
>>
>> +int fdtdec_setup_memory_size(void)
>> +{
>> + int ret, mem;
>> + struct fdt_resource res;
>> +
>> + mem = fdt_path_offset(gd->fdt_blob, "/memory");
>> + if (mem < 0) {
>> + debug("%s: Missing /memory node\n", __func__);
>> + return -EINVAL;
>> + }
>> +
>> + ret = fdt_get_resource(gd->fdt_blob, mem, "reg", 0, &res);
>> + if (ret != 0) {
>> + debug("%s: Unable to decode first memory bank\n", __func__);
>> + return -EINVAL;
>> + }
>> +
>> + gd->ram_size = (phys_size_t)(res.end - res.start + 1);
>> + debug("%s: Initial DRAM size %llx\n", __func__, (u64)gd->ram_size);
>> +
>> + return 0;
>> +}
>> +
>> +int fdtdec_setup_memory_banksize(void)
>> +{
>> + int bank, ret, mem;
>> + struct fdt_resource res;
>> +
>> + mem = fdt_path_offset(gd->fdt_blob, "/memory");
>> + if (mem < 0) {
>> + debug("%s: Missing /memory node\n", __func__);
>> + return -EINVAL;
>> + }
>> +
>> + for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
>> + ret = fdt_get_resource(gd->fdt_blob, mem, "reg", bank, &res);
>> + if (ret == -FDT_ERR_NOTFOUND)
>> + break;
>> + if (ret != 0)
>> + return ret;
>
> The return above return -EINVAL, but this one returns a -FDT_ERR_...
> which is different. So my suggestion here would be to return -EINVAL
> here, unless you want to change the function to always return an FDT
> error (although fdtdec_decode_memory_region() returns an errno error
> so perhaps it is better to be consistent with that).
Agreed, returning only -EINVAL in both error conditions makes more
sense than returning an -FDT_ERR_* error. This also makes it
consistent with the function above this function. Will send out v2.
Regards,
Nathan
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
