Hi Ulf, On Thu, May 11, 2017 at 10:44 PM, Martin Blumenstingl <martin.blumenstingl@xxxxxxxxxxxxxx> wrote: > Hi Ulf, > > On Thu, May 11, 2017 at 11:39 AM, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote: >> On 10 May 2017 at 21:22, Martin Blumenstingl >> <martin.blumenstingl@xxxxxxxxxxxxxx> wrote: >>> Hi Ulf, >>> >>> On Wed, May 10, 2017 at 10:44 AM, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote: >>>> On 6 May 2017 at 19:18, Martin Blumenstingl >>>> <martin.blumenstingl@xxxxxxxxxxxxxx> wrote: >>>>> This is the successor to Carlo Caione's "Add support for Amlogic Meson >>>>> MMC driver" series (v5) from [0]. >>>>> >>>>> I would like you to specifically review: >>>>> - whether I've (ab)used the MMC framework properly (as this is my first >>>>> "larger" contribution to an MMC driver) >>>> >>>> I take a look soonish. >>> thank you! >>> >>>>> - I think I have improved the locking compared to Carlo's version, >>>>> however I'd still like feedback on whether this looks sane now or if I >>>>> can improve that even further >>>>> >>>>> (notable) changes since Carlo's latest version are: >>>>> - renamed the driver to meson-mx-sdio (Amlogic's reference kernel calls >>>>> the driver "aml_sdio" as there is a second MMC controller in these SoCs >>>>> which they call the "SDHC controller"). do the same with our driver to >>>> >>>> I don't like to renaming drivers, just because there are a reference >>>> kernel using a different name. >>>> >>>> What's is really the difference between controllers? Why do they have >>>> two variants? >>> the driver from this thread is for the "SDSC/SDHC/SDXC card and SDIO >>> interface with 1-bit and 4-bit data bus width supporting spec version >>> 2.x/3.x/4.x DS/HS modes up to UHS-I SDR50" (quote taken from the S805 >>> datasheet: [0]) >>> the "other" controller is an "eMMC and MMC card interface with >>> 1/4/8-bit data bus width supporting spec version 4.4x/4.5x HS200 (up >>> to 100MHz clock), compatible with standard iNAND interface" (again, >>> quote taken from the S805 datasheet: [0]) >>> >>>> >>>> Can they be managed by the same driver? >>>> >>>>> avoid confusion once we add support for the second controller (which uses >>>>> a completely different register layout) >>>> >>>> Besides that, do they behave differently in some other way? >>> both drivers/controllers have a totally different register layout - I >>> don't see any way how they both could be handled by one driver (the >>> registers for the controllers from this series are not part of the >>> documentation, but the registers from the 8-bit capable controller >>> are, see page 76 and following from the S805 datasheet: [0]) >> >> Okay, I am starting to understand. :-) > good to hear that my explanation made (at least some) sense! > >> The spec in section 13 describes a controller supporting "MMC/SD/SDIO" >> cards. However, there is yet another controller which @subject series >> implement support for. >> You don't happen to have a public datasheet for this controller as >> well? If not, never mind. > unfortunately the controller from @subject does not show up in any of > the public datasheets, but we have a header file from Amlogic which > documents it just as well as the datasheet (probably) would: [0] > >> Then, meson actually have three different MMC/SD/SDIO controllers, the >> one in the S805 spec, the one supported by the recently up-streamed >> meson-gx-mmc.c driver - and the one being supported in this series. >> Right? And all of them are completely separate and non-compatible? > yes, unfortunately it seems that there are three different MMC > controller IP blocks from Amlogic > > let me explain this a bit more in detail: > the code-names of the newer (64-bit) SoCs all start with GX (GXBB, > GXL, GXM), so we call these "Meson GX". > the code-names of the older (32-bit) SoCs (at least the ones which are > not too old) all start with Meson X (where X is a number: Meson6, > Meson8, Meson8b and Meson8m2), Amlogic's vendor code sometimes calls > these "MX" (or Meson MX) > > the MMC IP blocks in Meson GX SoCs are easy: the same IP block is > embedded three times -> the driver for this is meson-gx-mmc.c (already > upstream) > the MMC IP blocks in Meson MX SoCs are more complicated: 1x 1/4-bit > which will be handled by the driver from @subject (meson-mx-sdio.c) > and 1x 1/4/8-bit for which there is only the vendor driver (plus the > description in the S805 datasheet) available. > > so it looks like the older Meson MX SoCs may end up with two different > drivers, while there's a third driver (for a totally different IP > block) for the newer Meson GX SoCs. > > [snip] >>>>> - use the common clock framework internally for managing the MMC clock >>>>> (there is a fixed-factor clock in the controller which takes clk81 as >>>>> input and divides it's clock by two and a divider clock which takes >>>>> the result from the fixed-factor clock as input) >>>>> - support the regulators provided by the MMC framework >>>>> - support for GPIO-based card-detection and read-only-detection through >>>>> the MMC framework >>>>> - use of the <linux/bitfield.h> FIELD_PREP and FIELD_GET macros where it >>>>> make sense (and thus the code easier to read) >>>>> - re-worked locking (based on the locking in dw_mmc as that also provides >>>>> multiple "MMC slots") >>>> >>>> Lots of changes! >>>> >>>> Before even start to review (or someone else), you really need to make >>>> this review-able. >>>> >>>> So, please, one change per patch - and make sure to write good >>>> changelogs. Then I can start to review. >>> from the mainline tree's perspective this is a new driver: >> >> Yes, I get it now. >> >> I first thought this series was related to the recently up-streamed >> meson-gx-mmc.c driver. Apologize for my ignorance. > should I put some more context/details in the description (maybe > something like: "this is a new driver/binding because the Meson MX > SoCs use a different IP block than the Meson GX SoCs (the IP block > from the latter is already supported by the meson-gx-mmc driver)"? > >>> Carlo (the original author) initially sent this driver for review more >>> than 14 months ago. unfortunately it was never merged since you >>> spotted some issues while reviewing that code, see [1]. >> >> Yes, I recall that now. > the 14 month delay was one of the reasons why I decided to re-post it > only after all features were implemented > >>> I would have sent smaller patches for a driver which is already in mainline. >> >> Right. >> >>> >>> I also wanted to avoid extra complexity for the internal mux if it was >>> added later on (if we want to avoid breaking devicetree backwards >>> compatibility then the driver would have to support both: parsing from >>> the mmc node directly and parsing child "slot" nodes). we won't have >>> to change the DT bindings when the first version that will be >>> mainlined already has support for the mux >>> >>> please let me know if there's anything I can do to make the code >>> easier to review. >> >> No worries, let me have a look as is! > okay, perfect! just take your time and let me know what needs to be changed. I know that you're probably very busy, but I will still ask: did you have time to look into this series yet (or do you have any ETA)? Thank you! Regards, Martin -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
