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

On 12/06/2012 06:37 PM, Milo Casagrande wrote:
> 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.
>

The layout part is not fixed anymore. In the earlier case (for SMDKV310
and Origen), the layout was like below.

--------------------------------------------
 | BL1 | ENV | BL2 |
--------------------------------------------
 1 +32s 33 +32s 65 +1024 1089
--------------------------------------------

The newer layout is a bit flexible wherein ENV may lie before or after
BL2, depending on the specific u-boot. That is why the offset of ENV and
BL2 is no more fixed, rather should be provided through board config.

>> @@ -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.
>

Right. The layouts for the u-boot of the Samsung boards are not fixed
anymore. So above assert statements make no sense.

> 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.
>

Ok, I will only define these for subclasses for which the values are
different (OrigenQuad and Arndale in future).

> 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
>

As a note, it currently fails with l-m-c because of addition of two new
variables samsung_bl2_start and samsung_env_start. I have got fixes for
them; after testing I will push them to the same branch on lp.
--
Tushar Behera