Re: [U-Boot] [PATCH 14/30] dm: part: Convert partition API use to linker lists

[Simon Glass]

Hi Stephen,

On 16 February 2016 at 23:41, Stephen Warren  wrote:
> On 02/14/2016 07:16 PM, Simon Glass wrote:
>> We can use linker lists instead of explicitly declaring each function.
>> This makes the code shorter by avoiding switch() statements and lots of
>> header file declarations.
>>
>> While this does clean up the code it introduces a few code issues with SPL.
>> SPL never needs to print partition information since this all happens from
>> commands. SPL mostly doesn't need to obtain information about a partition
>> either, except in a few cases. Add these cases so that the code will be
>> dropped from each partition driver when not needed. This avoids code bloat.
>>
>> I think this is still a win, since it is not a bad thing to be explicit
>> about which features are used in SPL. But others may like to weigh in.
>
> This patch changes the order in which partition types are detected,
> which matters when multiple partition types match (a GPT often/always
> has a protective MBR too). Consequently, this breaks GPT support.
>
> By hacking around this (turning off DOS partition support), and fixing
> the PCIe issue I mentioned elsewhere, test/py seems to work for me with
> this series applied.

Thanks for finding this and for your comment on the other patch too.
I'll fix the ordering for v2.

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

Re: [U-Boot] [PATCH 14/30] dm: part: Convert partition API use to linker lists

[Simon Glass]

Hi Bin,

On 16 February 2016 at 07:25, Bin Meng  wrote:
> Hi Simon,
>
> On Mon, Feb 15, 2016 at 10:16 AM, Simon Glass  wrote:
>> We can use linker lists instead of explicitly declaring each function.
>> This makes the code shorter by avoiding switch() statements and lots of
>> header file declarations.
>>
>> While this does clean up the code it introduces a few code issues with SPL.
>> SPL never needs to print partition information since this all happens from
>> commands. SPL mostly doesn't need to obtain information about a partition
>> either, except in a few cases. Add these cases so that the code will be
>> dropped from each partition driver when not needed. This avoids code bloat.
>>
>> I think this is still a win, since it is not a bad thing to be explicit
>> about which features are used in SPL. But others may like to weigh in.
>>
>> Signed-off-by: Simon Glass 
>> ---
>>
>>  disk/part.c   | 184 
>> +-
>>  disk/part_amiga.c |  16 +++--
>>  disk/part_dos.c   |   9 ++-
>>  disk/part_efi.c   |  10 ++-
>>  disk/part_iso.c   |  16 +++--
>>  disk/part_mac.c   |  16 +++--
>>  include/part.h|  79 ++-
>>  7 files changed, 157 insertions(+), 173 deletions(-)
>>
>
> [snip]
>
>> diff --git a/disk/part_amiga.c b/disk/part_amiga.c
>> index 5702c95..0f569f0 100644
>> --- a/disk/part_amiga.c
>> +++ b/disk/part_amiga.c
>> @@ -207,7 +207,7 @@ struct bootcode_block *get_bootcode(struct blk_desc 
>> *dev_desc)
>>   * Test if the given partition has an Amiga partition table/Rigid
>>   * Disk block
>>   */
>> -int test_part_amiga(struct blk_desc *dev_desc)
>> +static int test_part_amiga(struct blk_desc *dev_desc)
>>  {
>>  struct rigid_disk_block *rdb;
>>  struct bootcode_block *bootcode;
>> @@ -291,8 +291,8 @@ static struct partition_block *find_partition(struct 
>> blk_desc *dev_desc,
>>  /*
>>   * Get info about a partition
>>   */
>> -int get_partition_info_amiga(struct blk_desc *dev_desc, int part,
>> -disk_partition_t *info)
>> +static int get_partition_info_amiga(struct blk_desc *dev_desc, int part,
>> +   disk_partition_t *info)
>>  {
>>  struct partition_block *p = find_partition(dev_desc, part-1);
>>  struct amiga_part_geometry *g;
>> @@ -319,7 +319,7 @@ int get_partition_info_amiga(struct blk_desc *dev_desc, 
>> int part,
>>  return 0;
>>  }
>>
>> -void print_part_amiga(struct blk_desc *dev_desc)
>> +static void print_part_amiga(struct blk_desc *dev_desc)
>>  {
>>  struct rigid_disk_block *rdb;
>>  struct bootcode_block *boot;
>> @@ -379,4 +379,12 @@ void print_part_amiga(struct blk_desc *dev_desc)
>>  }
>>  }
>>
>> +U_BOOT_PART_TYPE(amiga) = {
>> +   .name   = "AMIGA",
>> +   .part_type  = PART_TYPE_AMIGA,
>> +   .get_info   = get_partition_info_amiga,
>> +   .print  = print_part_amiga,
>> +   .test   = test_part_amiga,
>> +};
>> +
>>  #endif
>> diff --git a/disk/part_dos.c b/disk/part_dos.c
>> index ea0315c..7567ed3 100644
>> --- a/disk/part_dos.c
>> +++ b/disk/part_dos.c
>> @@ -87,7 +87,7 @@ static int test_block_type(unsigned char *buffer)
>>  }
>>
>>
>> -int test_part_dos(struct blk_desc *dev_desc)
>> +static int test_part_dos(struct blk_desc *dev_desc)
>>  {
>> ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buffer, dev_desc->blksz);
>>
>> @@ -295,5 +295,12 @@ int get_partition_info_dos(struct blk_desc *dev_desc, 
>> int part,
>> return get_partition_info_extended(dev_desc, 0, 0, 1, part, info, 0);
>>  }
>>
>> +U_BOOT_PART_TYPE(dos) = {
>> +   .name   = "DOS",
>> +   .part_type  = PART_TYPE_DOS,
>> +   .get_info   = part_get_info_ptr(get_partition_info_dos),
>
> Does this compile?

These are defined in part.h. I had to add these to avoid bloating the
code since these functions are never called in SPL.

>
>> +   .print  = part_print_ptr(print_part_dos),
>
> and this?
>
>> +   .test   = test_part_dos,
>> +};
>>
>>  #endif
>> diff --git a/disk/part_efi.c b/disk/part_efi.c
>> index 7bd840f..6f80877 100644
>> --- a/disk/part_efi.c
>> +++ b/disk/part_efi.c
>> @@ -319,7 +319,7 @@ int get_partition_info_efi_by_name(struct blk_desc 
>> *dev_desc,
>> return -2;
>>  }
>>
>> -int test_part_efi(struct blk_desc *dev_desc)
>> +static int test_part_efi(struct blk_desc *dev_desc)
>>  {
>> ALLOC_CACHE_ALIGN_BUFFER_PAD(legacy_mbr, legacymbr, 1, 
>> dev_desc->blksz);
>>
>> @@ -953,4 +953,12 @@ static int is_pte_valid(gpt_entry * pte)
>> return 1;
>> }
>>  }
>> +
>> +U_BOOT_PART_TYPE(efi) = {
>> +   .name   = "EFI",
>> +   .part_type  = PART_TYPE_EFI,
>> +   .get_info   = part_get_info_ptr(get_partition_info_efi),
>> +   .print  = part_print_ptr(print_part_efi),
>
> and these?
>
>> +   .test   = 

Re: [U-Boot] [PATCH 14/30] dm: part: Convert partition API use to linker lists

[Stephen Warren]

On 02/14/2016 07:16 PM, Simon Glass wrote:
> We can use linker lists instead of explicitly declaring each function.
> This makes the code shorter by avoiding switch() statements and lots of
> header file declarations.
> 
> While this does clean up the code it introduces a few code issues with SPL.
> SPL never needs to print partition information since this all happens from
> commands. SPL mostly doesn't need to obtain information about a partition
> either, except in a few cases. Add these cases so that the code will be
> dropped from each partition driver when not needed. This avoids code bloat.
> 
> I think this is still a win, since it is not a bad thing to be explicit
> about which features are used in SPL. But others may like to weigh in.

This patch changes the order in which partition types are detected,
which matters when multiple partition types match (a GPT often/always
has a protective MBR too). Consequently, this breaks GPT support.

By hacking around this (turning off DOS partition support), and fixing
the PCIe issue I mentioned elsewhere, test/py seems to work for me with
this series applied.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 14/30] dm: part: Convert partition API use to linker lists

[Tom Rini]

On Sun, Feb 14, 2016 at 07:16:43PM -0700, Simon Glass wrote:

> We can use linker lists instead of explicitly declaring each function.
> This makes the code shorter by avoiding switch() statements and lots of
> header file declarations.
> 
> While this does clean up the code it introduces a few code issues with SPL.
> SPL never needs to print partition information since this all happens from
> commands. SPL mostly doesn't need to obtain information about a partition
> either, except in a few cases. Add these cases so that the code will be
> dropped from each partition driver when not needed. This avoids code bloat.
> 
> I think this is still a win, since it is not a bad thing to be explicit
> about which features are used in SPL. But others may like to weigh in.
> 
> Signed-off-by: Simon Glass 

Since there's a question here:

Reviewed-by: Tom Rini 

-- 
Tom


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