Hi Hans, (As a side note, your mailer just did something nasty with the wrapping which made the code snippets totally unreadable. I'm going to drop them.) On Wed, Feb 19, 2014 at 01:14:58PM +0100, Hans de Goede wrote: > >>>> + wmb(); /* Ensure idma_des hit main mem before we start the idmac */ > >>> > >>> wmb ensure the proper ordering of the instructions, not flushing > >>> the caches like what your comment implies. > >> > >> Since I put that comment there, allow me to explain. A modern ARM > >> cpu core has 2 or more units handling stores. One for regular > >> memory stores, and one for io-mem stores. Regular mem stores can > >> be re-ordered, io stores cannot. Normally there is no "syncing" > >> between the 2 store units. Cache flushing is not an issue here > >> since the memory holding the descriptors for the idma controller > >> is allocated cache coherent, which on arm means it is not cached. > >> > >> What is an issue here is the io-store starting the idmac hitting > >> the io-mem before the descriptors hit the main-mem, the wmb() > >> ensures this does not happen. > > > > To expand a bit, my point was not that it was functionnally > > wrong. Since you put a barrier in there, and that it resides in a > > cache coherent section, we're fine. > > > > My point was that the comment itself was misleading. > > Well as explained above, the purpose of the wmb is to ensure that > the descriptors hit main memory, before the following writel (in the > caller of this function) starts the controller. So I don't see > exactly how the comment is wrong. > > If you've a better wording for the comment, suggestions are welcome. Your first reply was great :) But if you feel like it enough, fine. [ codeless snip..] > >>> I'd rather put it at a debug loglevel. > >> > >> Erm, this only happens if something is seriously wrong. > > > > Still. Something would be seriously wrong in the MMC > > driver/controller. You don't want to bloat the whole kernel logs > > with the dump of your registers just because the MMC is > > failing. This is of no interest to anyone but someone that would > > actually try to debug what's wrong. > > This is not a complete register dump, this writes a single line to > the kernel log saying that an io error happened, and printing the > error flags set in the status register. We cannot be much shorter > then this without simply not notifying the user that an io error has > happened, and not notifying the user is wrong IMHO. Ok. > >>>> + /* And put it back in reset */ > >>>> + sunxi_mmc_exit_host(host); > >>> > >>> Hu? If it's in reset, how can it generate some IRQs? > >> > >> Yes, that is why we do the whole dance of init controller, get > >> irq, disable irq, drop it back in reset (until the mmc subsys > >> does a power on of the mmc card / sdio dev). > >> > >> Sometime the controller asserts the irq in reset for some reason, > >> so without the dance as soon as we do the devm_request_irq we get > >> an irq, and worse, not only do we get an irq, we cannot clear it > >> since writing to the interrupt status register does not work when > >> the controller is in reset, so we get stuck re-entering the irq > >> handler. > > > > Hmmm, I see. It probably deserves some commenting here too then. > > This call is the mirror of the sunxi_mmc_init_host a few lines > higher, which has this comment: > > /* Make sure the controller is in a sane state before enabling irqs */ > > Which attempts to explain why we do the init controller, claim irq, > disable irq, put controller back in reset sequence. Again suggestions > for a better comment are welcome. And again, the second part of your first reply was great :) > >>>> + ret = mmc_add_host(mmc); > >>>> + if (ret) > >>>> + goto error_free_dma; > >>>> + > >>>> + dev_info(&pdev->dev, "base:0x%p irq:%u\n", host->reg_base, host->irq); > >>>> + platform_set_drvdata(pdev, mmc); > >>> > >>> This should be before the registration. Otherwise, you're racy. > >> > >> Nope, we only need this to get the data on sunxi_mmc_remove, > >> everywhere else the data is found through the mmc-host struct. > > > > Still, if anyone makes a following patch using the platform_device > > for some reason, we will have a race condition, without any way to > > notice it. > > > > Plus, you're doing all the other bits of initialization of your > > structures much earlier, why not be consistent and having all of > > them at the same place? > > Most platform drivers I've worked on do platform_set_drvdata as late > as possible, so that the drvdata does not get set and never cleared > in error paths. You don't actually have to clear it, and some frameworks actually require you to call dev_set_drvdata before registration, so that statement looks quite odd to me. Thanks! Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
Attachment:
signature.asc
Description: Digital signature
