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