From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steve Rae Date: Fri, 30 May 2014 09:56:00 -0700 Subject: [U-Boot] [PATCH] mmc: add wrappers for MMC block_{read, write, erase} In-Reply-To: <5388AA9C.8030504@wwwdotorg.org> References: <1401315346-30231-1-git-send-email-srae@broadcom.com> <53875F6D.5010708@wwwdotorg.org> <5387753E.9060106@broadcom.com> <538781B6.90703@wwwdotorg.org> <53878E10.4010609@broadcom.com> <538798E1.2040703@wwwdotorg.org> <5387AEB8.8030304@broadcom.com> <5388AA9C.8030504@wwwdotorg.org> Message-ID: <5388B820.7030806@broadcom.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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