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

[Steve Rae <srae@broadcom.com>]

On 14-05-29 01:30 PM, Stephen Warren wrote:
> 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.
>
Sorry, but now I am totally confused...
Doesn't the "block_dev_desc_t" contain the "device" information (not the 
"partition" information)?
Isn't it only created once (effectively the first time "mmc_init" is 
called on that device)?
So when I'm doing a block_read from the Boot1 partition, followed by a 
block_read from the User partition, I don't expect to see a 
"constructor" being executed (from a get_dev() or anything else...)

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