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

[Steve Rae <srae@broadcom.com>]

On 14-05-30 08:58 AM, Stephen Warren wrote:
> On 05/29/2014 04:03 PM, Steve Rae wrote:
>>
>>
>> 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)?
>
> The eMMC HW partitions are separate block devices. So,
> get_device_and_partition() returns a block device that represents one of:
>
> a) eMMC "boot0" HW partition
> b) eMMC "boot1" HW partition
> c) eMMC "main data/user area" HW partition
>
> These HW partitions are entirely separate from (MBR/GPT) SW partitions,
> even though both are referred to as "partitions". That's why I call the
> former "HW partitions" rather than "partitions".
>
Agree -- and sometimes called "physical partitions"

>> Isn't it only created once (effectively the first time "mmc_init" is
>> called on that device)?
>
> The block_dev_desc_t initialization/creation does call mmc_init, yes...
>
>> 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...)
>
> Most U-Boot commands take a single device name (e.g. "mmc 0") and act
> just on that. If you want to do something on different block devices,
> you'd need to run separate commands, or perhaps have one command take a
> list of devices, and initialize each one in turn.
Agree - and can switch to different HW partitions with the existing "mmc 
dev 0 1" command

  What code are you
> looking at that handles multiple devices sequentially under program
> control rather than user command control?
>
Cannot go into too much detail here (yet) -- but imagine the situation 
where:
- lookup the GPT partition name (in User, Boot1, Boot2)
- do a block_write to the desired location...

So after discussing with a colleague, we would propose the following. 
Does this implement what you were proposing?:


Usage (example):

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

Implementation:
(1) The mmc->part_num_next_block_op needs to be initialized to -1.
(2) Each existing mmc_{bread,bwrite,berase} function needs modification:
if 0 <= mmc->part_num_next_block_op && mmc->part_num != 
mmc->part_num_next_block_op
     switch
     if switch failed
         mmc->part_num_next_block_op = -1
         return error
[... the original code ...]
if 0 <= mmc->part_num_next_block_op && mmc->part_num != 
mmc->part_num_next_block_op
     switch_back
     if switch_back failed
         mmc->part_num = mmc->part_num_next_block_op
     mmc->part_num_next_block_op = -1


Many Thanks, Steve