From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steve Rae Date: Thu, 29 May 2014 15:03:36 -0700 Subject: [U-Boot] [PATCH] mmc: add wrappers for MMC block_{read, write, erase} In-Reply-To: <538798E1.2040703@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> Message-ID: <5387AEB8.8030304@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-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:-) >