Hi Andrew, > On Mar 24, 2015, at 21:02 , Andrew Gabbasov <andrew_gabba...@mentor.com> > wrote: > >> From: Troy Kisky [mailto:troy.ki...@boundarydevices.com] >> Sent: Tuesday, March 24, 2015 9:03 PM >> To: Gabbasov, Andrew; peng....@freescale.com; u-boot@lists.denx.de >> Cc: Eric Nelson >> Subject: Re: [U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR >> only if it is still not ready >> >> On 3/24/2015 9:33 AM, Andrew Gabbasov wrote: >>> Hi Troy, >>> >>>> From: Troy Kisky [mailto:troy.ki...@boundarydevices.com] >>>> Sent: Monday, March 23, 2015 11:09 PM >>>> To: Gabbasov, Andrew; peng....@freescale.com; u-boot@lists.denx.de >>>> Cc: Eric Nelson >>>> Subject: Re: [U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for >>>> OCR only if it is still not ready >>>> >>>> On 3/23/2015 12:38 AM, Andrew Gabbasov wrote: >>>>> Hi Troy, >>>>> >>>>>> From: Troy Kisky [mailto:troy.ki...@boundarydevices.com] >>>>>> Sent: Friday, March 20, 2015 9:39 PM >>>>>> To: peng....@freescale.com; Gabbasov, Andrew; u-boot@lists.denx.de >>>>>> Cc: Eric Nelson >>>>>> Subject: Re: [U-Boot] [PATCH 4/6] mmc: Continue polling MMC card >>>>>> for OCR only if it is still not ready >>>>>> >>>>>> [skipped] >>>>>> >>>>>> Here's another patch that solves the problem a little earlier. It >>>>>> has this disadvantage of being slightly bigger, though it makes the >>>>>> code look >>>>> better. >>>>>> >>>>>> https://github.com/boundarydevices/u-boot-imx6/commit/c0260ca >>>>>> >>>>> >>>>> I have a couple of doubts regarding that patch. >>>>> >>>>> First, my personal taste objects to such duplicating of the code (I >>>>> mean setting of version, ocr, rca, etc fields of mmc structure). >>>>> If we'll have to change or add anything to these settings, we'll >>>>> have to make the same change in 2 different place, which is >>>>> error-prone and extremely inconvenient from maintenance point of view. >>>>> >>>>> Second, what about SPI mode? Doesn't this patch skip retrieving of >>>>> OCR register with a special command for SPI host case (thus setting >>>>> ocr field incorrectly), if the card comes to ready state with the >>>>> first attempt? >>>> >>>> That's a good argument for a subroutine to be doing that work instead >>>> of >>> in two >>>> places. >>> >>> In some sense the second function of these two >>> (mmc_complete_op_cond()) is exactly such subroutine ;-) It is doing >>> this work of final settings and actions after making OCR polling. >>> Although completing the polling itself in some cases too. >>> Actually, it seems that introducing of one more service function would >>> make the code a little too "chopped" into too small pieces, and also >>> further less similar to SD (as opposed to MMC) case. >>> >>> Thanks. >>> >> >> I've already said that I'm fine with any patch that fixes the problem, so > there is >> no need to convince me of anything. I just wanted to show that setting the >> pending flag, when the command is actually complete, does not make a lot > of >> sense. > > I absolutely agree. In fact, this flag in current implementation just > indicates > the branch that the MMC card case is being processed. If the version field, > differing SD and MMC cases, would be set a little earlier, for example, > directly in mmc_start_init(), we could just use !IS_SD() instead of this > pending flag. And thinking further, it's not quite clear why the splitting > of OCR polling was done for MMC case only, but not for SD too. > So, there is still the room for further improving the code, may be even > some reorganizing, but I had to stop somewhere, unless there is > the direct necessity ;-) >
Your patch fixes the current problem nicely. There is a lot of cruft around, we’ll get around looking it at some point in the future (yeah, maybe) :) Thanks for the good work, it’s fine as it is right now. > Thanks. > > Best regards, > Andrew > Regards — Pantelis _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
