Code review comment for lp:~tusharbehera/linaro-image-tools/origen-generic

Hello Tushar,

thanks for working on this!

The general refactoring looks good to me, I have some comments that
follows inline, but nothing earth-shaking.

On Thu, Dec 6, 2012 at 11:02 AM, Tushar Behera <email address hidden> wrote:
>
> === modified file 'linaro_image_tools/media_create/boards.py'
> --- linaro_image_tools/media_create/boards.py 2012-10-22 06:57:20 +0000
> +++ linaro_image_tools/media_create/boards.py 2012-12-06 10:01:23 +0000
> @@ -425,37 +425,26 @@
> # Support for dtb_files as per hwpack v3 format.
> dtb_files = None
>
> - # Samsung v310 implementation notes and terminology
> + # Samsung Boot-loader implementation notes and terminology
> #
> # * BL0, BL1 etc. are the various bootloaders in order of execution
> - # * BL0 is the first stage bootloader, located in ROM; it loads a 32s
> - # long BL1 from MMC offset +1s and runs it
> - # * BL1 is the secondary program loader (SPL), a small (< 14k) version
> + # * BL0 is the first stage bootloader, located in ROM; it loads BL1/SPL
> + # from MMC offset +1s and runs it.
> + # * BL1 is the secondary program loader (SPL), a small version
> # of U-Boot with a checksum; it inits DRAM and loads a 1024s long BL2
> - # to DRAM from MMC offset +65s
> - # * BL2 is U-Boot; it loads its 32s (16 KiB) long environment from MMC
> - # offset +33s which tells it to load a boot.scr from the first FAT
> - # partition of the MMC
> - #
> - # Layout:
> - # +0s: part table / MBR, 1s long
> - # +1s: BL1/SPL, 32s long
> - # +33s: U-Boot environment, 32s long
> - # +65s: U-Boot, 1024s long
> - # >= +1089s: FAT partition with boot script (boot.scr), kernel (uImage)
> - # and initrd (uInitrd)

Is the layout part not relevant anymore? Did it change or can it
change in the future?
It was handy to have around, at least for somebody looking at the code
and with not much knowledge of the Samsung layouts. If it is not
relevant anymore, fine by me to remove it.

> @@ -493,12 +482,12 @@
> cls.kernel_flavors = None
> cls.mmc_option = None
> cls.mmc_part_offset = None
> - cls.SAMSUNG_V310_BL1_START = None
> - cls.SAMSUNG_V310_BL1_LEN = None
> - cls.SAMSUNG_V310_ENV_START = None
> - cls.SAMSUNG_V310_ENV_LEN = None
> - cls.SAMSUNG_V310_BL2_START = None
> - cls.SAMSUNG_V310_BL2_LEN = None
> + cls.samsung_bl1_start = None
> + cls.samsung_bl1_len = None
> + cls.samsung_env_start = None
> + cls.samsung_env_len = None
> + cls.samsung_bl2_start = None
> + cls.samsung_bl2_len = None
>
> # Set new values from metadata.
> cls.kernel_addr = cls.get_metadata_field('kernel_addr')
> @@ -594,31 +583,30 @@
> loader_start = cls.get_metadata_field('loader_start')
> if loader_start is not None:
> cls.LOADER_START_S = int(loader_start)
> +
> samsung_bl1_start = cls.get_metadata_field('samsung_bl1_start')
> if samsung_bl1_start is not None:
> - cls.SAMSUNG_V310_BL1_START = int(samsung_bl1_start)
> + cls.samsung_bl1_start = int(samsung_bl1_start)
> +
> samsung_bl1_len = cls.get_metadata_field('samsung_bl1_len')
> if samsung_bl1_len is not None:
> - cls.SAMSUNG_V310_BL1_LEN = int(samsung_bl1_len)
> + cls.samsung_bl1_len = int(samsung_bl1_len)
> +
> + samsung_bl2_len = cls.get_metadata_field('samsung_bl2_len')
> + if samsung_bl2_len is not None:
> + cls.samsung_bl2_len = int(samsung_bl2_len)
> +
> + samsung_bl2_start = cls.get_metadata_field('samsung_bl2_start')
> + if samsung_bl2_start is not None:
> + cls.samsung_bl2_start = int(samsung_bl2_start)
> +
> samsung_env_len = cls.get_metadata_field('samsung_env_len')
> if samsung_env_len is not None:
> - cls.SAMSUNG_V310_ENV_LEN = int(samsung_env_len)
> - assert cls.SAMSUNG_V310_ENV_LEN * SECTOR_SIZE == 16 * 1024, (
> - "BL1 expects u-boot environment to be 16 KiB")
> - samsung_bl2_len = cls.get_metadata_field('samsung_bl2_len')
> - if samsung_bl2_len is not None:
> - cls.SAMSUNG_V310_BL2_LEN = int(samsung_bl2_len)
> - assert cls.SAMSUNG_V310_BL2_LEN * SECTOR_SIZE == 512 * 1024, (
> - "BL1 expects BL2 (u-boot) to be 512 KiB")
> + cls.samsung_env_len = int(samsung_env_len)
>
> - if (cls.SAMSUNG_V310_BL1_START and cls.SAMSUNG_V310_BL1_LEN):
> - cls.SAMSUNG_V310_ENV_START = (cls.SAMSUNG_V310_BL1_START +
> - cls.SAMSUNG_V310_BL1_LEN)
> - assert cls.SAMSUNG_V310_ENV_START == 33, (
> - "BL1 expects u-boot environment at +33s")
> - if (cls.SAMSUNG_V310_ENV_START and cls.SAMSUNG_V310_ENV_LEN):
> - cls.SAMSUNG_V310_BL2_START = (cls.SAMSUNG_V310_ENV_START +
> - cls.SAMSUNG_V310_ENV_LEN)
> + samsung_env_start = cls.get_metadata_field('samsung_env_start')
> + if samsung_env_start is not None:
> + cls.samsung_env_start = int(samsung_env_start)

Any particular reasons to remove the assert statements at this point?
The old behavior was to read and calculate the value, making sure
correct values were stored. It is somehow related to the layouts: it
is not that fixed anymore?
If the configuration can be flexible and boards work even with
different values set, that's OK.

Even if some might argue the use of asserts, I didn't see anything
wrong with having them there as a form of precaution (instead of
raising errors/exceptions).

> class SMDKV310Config(SamsungConfig):
> @@ -1781,6 +1769,13 @@
> boot_script = 'boot.scr'
> mmc_part_offset = 1
> mmc_option = '0:2'
> + samsung_bl1_start = 1
> + samsung_bl1_len = 32
> + samsung_env_start = 33
> + samsung_env_len = 32
> + samsung_bl2_start = 65
> + samsung_bl2_len = 1024
> +
>
> @classmethod
> def _get_boot_env(cls, is_live, is_lowmem, consoles, rootfs_id,
> @@ -1805,6 +1800,12 @@
> boot_script = 'boot.scr'
> mmc_part_offset = 1
> mmc_option = '0:2'
> + samsung_bl1_start = 1
> + samsung_bl1_len = 32
> + samsung_env_start = 33
> + samsung_env_len = 32
> + samsung_bl2_start = 65
> + samsung_bl2_len = 1024

I'm not sure these last additions are necessary: if the configs
inherit from SamsungConfig, and from BoardConfig, it should be
sufficient to define them in the parent class, without having to
duplicate values in the subclasses.

Also, consider that in the (near) future we might move away from class
methods and class attributes toward instance methods and instance
attributes. So, if in doubt about the class-approach and inheritance,
with the instance approach it will really be just sufficient to
definethem in the parent class.

--
Milo Casagrande
Infrastructure Engineer
Linaro.org <www.linaro.org> │ Open source software for ARM SoCs