[PATCH v2 1/3] clk: meson: add DT documentation for emmc clock controller

On Thu, Jul 12, 2018 at 5:29 PM Yixun Lan [off-list ref] wrote:

HI Rob

see my comments

On 07/12/2018 10:17 PM, Rob Herring wrote:

quoted

On Wed, Jul 11, 2018 at 8:47 PM Yixun Lan [off-list ref] wrote:

quoted

Hi Rob

see my comments

On 07/12/18 03:43, Rob Herring wrote:

quoted

On Tue, Jul 10, 2018 at 04:36:56PM +0000, Yixun Lan wrote:

quoted

Document the MMC sub clock controller driver, the potential consumer
of this driver is MMC or NAND.

So you all have decided to properly model this now?

Yes, ;-)

quoted

quoted

Signed-off-by: Yixun Lan <redacted>
---
 .../bindings/clock/amlogic,mmc-clkc.txt       | 31 +++++++++++++++++++
 1 file changed, 31 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt

diff --git a/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt b/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
new file mode 100644
index 000000000000..ff6b4bf3ecf9
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt

@@ -0,0 +1,31 @@
+* Amlogic MMC Sub Clock Controller Driver
+
+The Amlogic MMC clock controller generates and supplies clock to support
+MMC and NAND controller
+
+Required Properties:
+
+- compatible: should be:
+            "amlogic,meson-gx-mmc-clkc"
+            "amlogic,meson-axg-mmc-clkc"
+
+- #clock-cells: should be 1.
+- clocks: phandles to clocks corresponding to the clock-names property
+- clock-names: list of parent clock names
+    - "clkin0", "clkin1"
+
+Parent node should have the following properties :
+- compatible: "syscon", "simple-mfd, and "amlogic,meson-axg-mmc-clkc"

You don't need "simple-mfd" and probably not syscon either. The order is
wrong too. Most specific first.

Ok, I will drop "simple-mfd"..

but the syscon is a must, since this mmc clock model access registers
via the regmap interface

A syscon compatible should not be the only way to get a regmap.

do you have any suggestion about other function that I can use? is
devm_regmap_init_mmio() feasible

quoted

Removing lines 56/57 of drivers/mfd/syscon.c should be sufficient.

I'm not sure what's the valid point of removing compatible 'syscon' in
driver/mfd/syscon.c, sounds this will break a lot DT/or need to fix?
will you propose a patch for this? then I can certainly adjust here

Removing the 2 lines will simply allow any node to be a syscon. If
there's a specific driver for a node, then that makes sense to allow
that.

quoted

Why do you need a regmap in the first place? What else needs to access
this register directly?

Yes, the SD_EMMC_CLOCK register contain several bits which not fit well
into common clock model, and they need to be access in the NAND or eMMC
driver itself, Martin had explained this in early thread[1]

In this register
Bit[31] select NAND or eMMC function
Bit[25] enable SDIO IRQ
Bit[24] Clock always on
Bit[15:14] SRAM Power down

[1]
https://lkml.kernel.org/r/CAFBinCBeyXf6LNaZzAw6WnsxzDAv8E=Yp2eem0xCPWMEUi6pnQ at mail.gmail.com

quoted

Don't you need a patch removing the clock code
from within the emmc driver? It's not even using regmap, so using
regmap here doesn't help.

No, and current eMMC driver still use iomap to access the register

Which means a read-modify-write can corrupt the register value if both
users don't access thru regmap. Changes are probably infrequent enough
that you get lucky...

I think we probably would like to take two steps approach.
first, from the hardware perspective, the NAND and eMMC(port C) driver
can't exist at same time, since they share the pins, clock, internal
ram, So we have to only enable one of NAND or eMMC in DT, not enable
both of them.

Yes, of course.

Second, we might like to convert eMMC driver to also use mmc-clkc model.

IMO, this should be done as part of merging this series. Otherwise, we
have duplicated code for the same thing.

Rob