[U-Boot] [PATCH] mmc: add wrappers for MMC block_{read, write, erase}

[Stephen Warren]

On 05/29/2014 01:44 PM, Steve Rae wrote:
> 
> 
> On 14-05-29 11:51 AM, Stephen Warren wrote:
>> On 05/29/2014 11:58 AM, Steve Rae wrote:
>>> Hi, Stephen
>>>
>>> On 14-05-29 09:25 AM, Stephen Warren wrote:
>>>> On 05/28/2014 04:15 PM, Steve Rae wrote:
>>>>> Each wrapper function:
>>>>> - switches to the specified physical partition, then
>>>>> - performs the original function, and then
>>>>> - switches back to the original physical partition
>>>>> where the physical partition (aka HW partition) is
>>>>>     0=User, 1=Boot1, 2=Boot2, etc.
>>>>
>>>> This feels wrong; why wouldn't mmc_get_dev() return a block_dev_desc_t
>>>> containing block_read/block_write functions that do the HW partition
>>>> switching. That way, this is all completely hidden, and all client code
>>>> only knows about block devices, rather than having to know about
>>>> MMC-specific mmc_block_read/write/erase_hwpart() functions.
>>>>
>>> This goes back to the initial discussion on this mailing list (which was
>>> never resolved):
>>>    http://lists.denx.de/pipermail/u-boot/2014-April/178171.html
>>> This issue is that the three callback functions defined in
>>> 'block_desc_t' do not accept the "partition number" as an argument.
>>> It was suggested that we could overwrite those functions; but the
>>> "partition number" still needs to be passed in by:
>>> (1) overloading the "int dev_num" argument, or
>>> (2) adding another argument to the callback functions
>>> I assumed that neither of these was acceptable, so I have proposed these
>>> wrappers...
>>
>> Can't the data simply be stored in the block_desc_t itself?
>
> If I understand this suggestion, are you proposing:
> - add an "unsigned int specified_hw_part" to the block_desc_t

Yes.

> Then the usage would become:
> mmc->block_dev.specified_hw_part = 1;      /* specify Boot1 partition */

The only code that would need to assign that field is
disk/part.c:get_dev() or something called from it. that is the function
that's responsible for looking up or creating the block_dev_desc_t
"handle" for a user-specified storage device, so it's exactly the place
for this kind of object "constructor" code to execute.

> mmc->block_dev.block_read(0, 0, 1, buf);  /* read first block (from
> Boot1 partition) */

Yes.

> mmc->block_dev.specified_hw_part = 0;      /* specify User partition */
> mmc->block_dev.block_read(0, 0, 1, buf);  /* read first block (from User
> partition) */
> 
> I don't think this is a good idea...

Oh, but it is:-)