| Message ID | 20200328003249.1248978-1-martin.blumenstingl@googlemail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Amlogic 32-bit Meson SoC SDHC MMC controller driver | expand |
Hi Ulf, On Sat, Mar 28, 2020 at 1:33 AM Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: [...] > Martin Blumenstingl (3): > dt-bindings: mmc: Document the Amlogic Meson SDHC MMC host controller > clk: meson: add a driver for the Meson8/8b/8m2 SDHC clock controller > mmc: host: meson-mx-sdhc: new driver for the Amlogic Meson SDHC host I have Rob's reviewed-by for the dt-bindings patch and three tested-by's for the MMC driver in patch #3 (which means that patch #2 was implicitly tested as well) I tried to answer all your previous questions where possible, but for some of your questions I simply don't have an answer. is there anything from your side which is holding this driver back from being merged? +Cc Jerome, because he is the maintainer of the Amlogic clock controller drivers - where this series adds another one, so we need to coordinate where patches go. Thank you! Martin
On Sat, 25 Apr 2020 at 22:27, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > > Hi Ulf, > > On Sat, Mar 28, 2020 at 1:33 AM Martin Blumenstingl > <martin.blumenstingl@googlemail.com> wrote: > [...] > > Martin Blumenstingl (3): > > dt-bindings: mmc: Document the Amlogic Meson SDHC MMC host controller > > clk: meson: add a driver for the Meson8/8b/8m2 SDHC clock controller > > mmc: host: meson-mx-sdhc: new driver for the Amlogic Meson SDHC host > I have Rob's reviewed-by for the dt-bindings patch and three > tested-by's for the MMC driver in patch #3 (which means that patch #2 > was implicitly tested as well) > I tried to answer all your previous questions where possible, but for > some of your questions I simply don't have an answer. > > is there anything from your side which is holding this driver back > from being merged? Apologize for the delay. I will have a look asap. > > +Cc Jerome, because he is the maintainer of the Amlogic clock > controller drivers - where this series adds another one, so we need to > coordinate where patches go. It seems like you may need to resend the series so the clock maintainers (Stephen and Jerome) can get a look as well. Perhaps it's better if the series is queued together - and can help to do that, but then I need acks from Stephen/Jerome for the clock patch. Kind regards Uffe
On Sat 28 Mar 2020 at 01:32, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > Hello, > > this is the patchset for a driver for the Amlogic "SDHC" MMC controller > found on Meson6, Meson8, Meson8b and Meson8m2 SoCs. > > The public S805 (Meson8b) datasheet has some documentation starting on > page 74: [0] > > It's performance is still not as good as the driver from Amlogic's 3.10 > kernel, but it does not corrupt data anymore (as RFC v1 did). > > Special thanks to the people who supported me off-list - you are > amazing and deserve to be mentioned here: > - Xin Yin who helped me fix two more write corruption problems. I am > hoping that he will reply with Reviewed-by, Tested-by and Bug-fixed-by > - Jianxin Pan for sharing some of the internal workings of this MMC > controller with me > - Wei Wang for spotting the initial write corruption problem and helping > test this driver on his board. I have his permission to add his > Tested-by (off-list, he's Cc'ed so if there's any problem he can speak > up) > > > Changes since v4 at [4]: > - move the four clkin clock inputs to the start of the clock-names list > as suggested by Rob, affects patch #1 > - fixed #include statement in dt-bindings example in patch #1 > > Changes since v3 at [3]: > - split the clock bits into a separate clock controller driver because > of two reasons: 1) it keeps the MMC controller driver mostly clean of > the clock bits If the register is in the MMC controller register space and the MMC driver is the driver using these clocks, it is where the clocks belong. I don't get why it could be an issue ? Is the clock block is shared with another device, like on the Gx family ? > 2) the pure clock controller can use > devm_clk_hw_register() (instead of devm_clk_register(), which is > deprecated) and the MMC controller can act as a pure clock consumer. Why can't you use devm_clk_hw_register in an MMC driver ? Unless I missed something, it is provided by clk-provider.h, which can be included by any driver. I'm not sure I understand why the support for this device is split in two drivers. Using CCF clocks out of "drivers/clk" is encouraged. > This also affects the dt-bindings which is why I dropped Rob's > Reviewed-by. Thanks to Ulf for the suggestions > > Changes since v2 at [2]: > - rebased on top of v5.5-rc1 > - added Rob's and Xin Yin's Reviewed-by and Tested-by (thank you!) > - (note: Kevin had v2 of this series in -next for a few days so the > build test robots could play with it. I haven't received any negative > feedback in that time) > > Changes since RFC v1 at [1]: > - don't set MESON_SDHC_MISC_MANUAL_STOP to fix one of three write > corruption problems. the out-of-tree 3.10 "reference" driver doesn't > set it either > - check against data->flags instead of cmd->flags when testing for > MMC_DATA_WRITE as spotted by Xin Yin (many thanks!). This fixes > another write corruption problem > - clear the FIFOs after successfully transferring data as suggested by > Xin Yin (many thanks!). This is what the 3.10 driver did and fixes yet > another write corruption problem > - integrate the clock suggestions from Jianxin Pan so the driver is now > able to set up the clocks correctly for all known cases. documentation > is also added to the patch description. Thank you Jianxin for the > help! > - set the correct max_busy_timeout as suggested by Jianxin Pan (thanks!) > - convert the dt-bindings to .yaml (which is why I didn't add Rob's > Reviewed-by) > - switch to struct clk_parent_data as part of newer common clock > framework APIs to simplify the clock setup > - dropped CMD23 support because it seems to hurt read and write > performance by 10-20% in my tests. it's not clear why, but for now we > can live without this. > - use devm_platform_ioremap_resource instead of open-coding it > > > [0] https://dn.odroid.com/S805/Datasheet/S805_Datasheet%20V0.8%2020150126.pdf > [1] https://patchwork.kernel.org/cover/11035505/ > [2] http://lists.infradead.org/pipermail/linux-amlogic/2019-November/014576.html > [3] https://patchwork.kernel.org/cover/11283179/ > [4] https://patchwork.kernel.org/cover/11329017/ > > Martin Blumenstingl (3): > dt-bindings: mmc: Document the Amlogic Meson SDHC MMC host controller > clk: meson: add a driver for the Meson8/8b/8m2 SDHC clock controller > mmc: host: meson-mx-sdhc: new driver for the Amlogic Meson SDHC host > > .../bindings/mmc/amlogic,meson-mx-sdhc.yaml | 83 ++ > drivers/clk/meson/Kconfig | 9 + > drivers/clk/meson/Makefile | 1 + > drivers/clk/meson/meson-mx-sdhc.c | 212 ++++ > drivers/mmc/host/Kconfig | 14 + > drivers/mmc/host/Makefile | 1 + > drivers/mmc/host/meson-mx-sdhc.c | 1064 +++++++++++++++++ > .../dt-bindings/clock/meson-mx-sdhc-clkc.h | 8 + > 8 files changed, 1392 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mmc/amlogic,meson-mx-sdhc.yaml > create mode 100644 drivers/clk/meson/meson-mx-sdhc.c > create mode 100644 drivers/mmc/host/meson-mx-sdhc.c > create mode 100644 include/dt-bindings/clock/meson-mx-sdhc-clkc.h
Hi Jerome, On Mon, Apr 27, 2020 at 10:56 AM Jerome Brunet <jbrunet@baylibre.com> wrote: [...] > > Changes since v3 at [3]: > > - split the clock bits into a separate clock controller driver because > > of two reasons: 1) it keeps the MMC controller driver mostly clean of > > the clock bits > > If the register is in the MMC controller register space and the MMC > driver is the driver using these clocks, it is where the clocks belong. > I don't get why it could be an issue ? > > Is the clock block is shared with another device, like on the Gx family ? no, it is not shared with another device (to my knowledge). > > 2) the pure clock controller can use > > devm_clk_hw_register() (instead of devm_clk_register(), which is > > deprecated) and the MMC controller can act as a pure clock consumer. > > Why can't you use devm_clk_hw_register in an MMC driver ? > Unless I missed something, it is provided by clk-provider.h, which can be > included by any driver. indeed, I could use devm_clk_hw_register in the MMC driver. Ulfs concern was that a lot of code was needed for managing the clocks and I agree with him. so this is my way of keeping those details away from the MMC driver and have two separate drivers which are better to understand overall. Martin
On Mon 27 Apr 2020 at 18:23, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > Hi Jerome, > > On Mon, Apr 27, 2020 at 10:56 AM Jerome Brunet <jbrunet@baylibre.com> wrote: > [...] >> > Changes since v3 at [3]: >> > - split the clock bits into a separate clock controller driver because >> > of two reasons: 1) it keeps the MMC controller driver mostly clean of >> > the clock bits >> >> If the register is in the MMC controller register space and the MMC >> driver is the driver using these clocks, it is where the clocks belong. >> I don't get why it could be an issue ? >> >> Is the clock block is shared with another device, like on the Gx family ? > no, it is not shared with another device (to my knowledge). > >> > 2) the pure clock controller can use >> > devm_clk_hw_register() (instead of devm_clk_register(), which is >> > deprecated) and the MMC controller can act as a pure clock consumer. >> >> Why can't you use devm_clk_hw_register in an MMC driver ? >> Unless I missed something, it is provided by clk-provider.h, which can be >> included by any driver. > indeed, I could use devm_clk_hw_register in the MMC driver. > Ulfs concern was that a lot of code was needed for managing the clocks > and I agree with him. so this is my way of keeping those details away > from the MMC driver and have two separate drivers which are better to > understand overall. Martin, Ulf, I understand that CCF code might seems verbose and I'm happy to help review it if necessary but I don't think every driver out there should register some kind of fake clock controller driver everytime they wish to use CCF API. Yes the it might make the driver code cleaner but the overall architecture is harder to follow. CCF was made so driver from any subsystem *may* use it. Creating a controller for a single register is overkill. The HW architecture of this particular device does not justify it. > > > Martin
Jerome, Martin, On Mon, 27 Apr 2020 at 18:46, Jerome Brunet <jbrunet@baylibre.com> wrote: > > > On Mon 27 Apr 2020 at 18:23, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > > > Hi Jerome, > > > > On Mon, Apr 27, 2020 at 10:56 AM Jerome Brunet <jbrunet@baylibre.com> wrote: > > [...] > >> > Changes since v3 at [3]: > >> > - split the clock bits into a separate clock controller driver because > >> > of two reasons: 1) it keeps the MMC controller driver mostly clean of > >> > the clock bits > >> > >> If the register is in the MMC controller register space and the MMC > >> driver is the driver using these clocks, it is where the clocks belong. > >> I don't get why it could be an issue ? > >> > >> Is the clock block is shared with another device, like on the Gx family ? > > no, it is not shared with another device (to my knowledge). > > > >> > 2) the pure clock controller can use > >> > devm_clk_hw_register() (instead of devm_clk_register(), which is > >> > deprecated) and the MMC controller can act as a pure clock consumer. > >> > >> Why can't you use devm_clk_hw_register in an MMC driver ? > >> Unless I missed something, it is provided by clk-provider.h, which can be > >> included by any driver. > > indeed, I could use devm_clk_hw_register in the MMC driver. > > Ulfs concern was that a lot of code was needed for managing the clocks > > and I agree with him. so this is my way of keeping those details away > > from the MMC driver and have two separate drivers which are better to > > understand overall. > > Martin, Ulf, > > I understand that CCF code might seems verbose and I'm happy to help > review it if necessary but I don't think every driver out there should > register some kind of fake clock controller driver everytime they wish > to use CCF API. > > Yes the it might make the driver code cleaner but the overall > architecture is harder to follow. > > CCF was made so driver from any subsystem *may* use it. Creating a > controller for a single register is overkill. The HW architecture of > this particular device does not justify it. I fully understand your point and I agree with it. If I recall correctly, my point in the earlier review phase was that I wanted the driver to be nicely split into a clock provider part and into a mmc host driver part. I also raised the point of using devm_clk_hw_register() rather than the deprecated devm_clk_register(). I still think this makes sense. That said, perhaps a reasonable split could be to have two separate c-files (one for clock provider and one for mmc host), but both in the mmc subsystem. Kind regards Uffe
Hi Ulf, Jerome, On Mon, Apr 27, 2020 at 8:36 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > Jerome, Martin, > > On Mon, 27 Apr 2020 at 18:46, Jerome Brunet <jbrunet@baylibre.com> wrote: > > > > > > On Mon 27 Apr 2020 at 18:23, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > > > > > Hi Jerome, > > > > > > On Mon, Apr 27, 2020 at 10:56 AM Jerome Brunet <jbrunet@baylibre.com> wrote: > > > [...] > > >> > Changes since v3 at [3]: > > >> > - split the clock bits into a separate clock controller driver because > > >> > of two reasons: 1) it keeps the MMC controller driver mostly clean of > > >> > the clock bits > > >> > > >> If the register is in the MMC controller register space and the MMC > > >> driver is the driver using these clocks, it is where the clocks belong. > > >> I don't get why it could be an issue ? > > >> > > >> Is the clock block is shared with another device, like on the Gx family ? > > > no, it is not shared with another device (to my knowledge). > > > > > >> > 2) the pure clock controller can use > > >> > devm_clk_hw_register() (instead of devm_clk_register(), which is > > >> > deprecated) and the MMC controller can act as a pure clock consumer. > > >> > > >> Why can't you use devm_clk_hw_register in an MMC driver ? > > >> Unless I missed something, it is provided by clk-provider.h, which can be > > >> included by any driver. > > > indeed, I could use devm_clk_hw_register in the MMC driver. > > > Ulfs concern was that a lot of code was needed for managing the clocks > > > and I agree with him. so this is my way of keeping those details away > > > from the MMC driver and have two separate drivers which are better to > > > understand overall. > > > > Martin, Ulf, > > > > I understand that CCF code might seems verbose and I'm happy to help > > review it if necessary but I don't think every driver out there should > > register some kind of fake clock controller driver everytime they wish > > to use CCF API. > > > > Yes the it might make the driver code cleaner but the overall > > architecture is harder to follow. > > > > CCF was made so driver from any subsystem *may* use it. Creating a > > controller for a single register is overkill. The HW architecture of > > this particular device does not justify it. > > I fully understand your point and I agree with it. > > If I recall correctly, my point in the earlier review phase was that I > wanted the driver to be nicely split into a clock provider part and > into a mmc host driver part. I also raised the point of using > devm_clk_hw_register() rather than the deprecated devm_clk_register(). > I still think this makes sense. > > That said, perhaps a reasonable split could be to have two separate > c-files (one for clock provider and one for mmc host), but both in the > mmc subsystem. I'm fine with that - I'll do that in the next patch version I believe the amount of changes will be small because the clock driver already uses devm_clk_hw_register() Martin