Re: [PATCH v6 2/2] mmc: host: meson-mx-sdhc: new driver for the Amlogic Meson SDHC host

[Martin Blumenstingl <martin.blumenstingl@googlemail.com>]

[-- Attachment #1: Type: text/plain, Size: 1691 bytes --]

Hi Jerome,

On Tue, May 5, 2020 at 6:05 PM Jerome Brunet <jbrunet@baylibre.com> wrote:
[...]
> > 2. Keep the existing approach, with devm_clk_get(). I am fine with
> > this as well, we can always switch to 1) later on.
>
> I have a problem with this approach.
> The dt-bindings would include "#clock-cells = <1>" for a device that
> does not actually provide and only needs it has a temporary work around.
> Those bindings are supposed to be stable ...
actually I don't see a problem here because this specific MMC
controller has a clock controller built into it.
Rob also didn't see any problem with this when he reviewed the dt-bindings

> I have proposed 2 other short term solutions, let's see how it goes
since I was also curious how this turns out I first implemented your
suggestion to use a similar clk_hw registration style as
dwmac-meson8b.
That made the code easier to read - thank you for the suggestion!

On top of that I switched from clk_hw_onecell_data to directly
accessing "clk_hw.clk".
Unfortunately the diffstat is not as great as I hoped, it saves 21
lines (11 in the driver code, 6 in the soc.dtsi, 5 in the dt-bindings)
Once devm_clk_hw_get_clk() is implemented 8 lines have to be added
again for error checking.
I attached the patch for the drivers/mmc/host/meson-mx-sdhc* parts if
you are curious (it'll apply only on top of my github branch, not on
this series).

Please let me know if you want me to submit an updated series where I
directly access "clk_hw.clk" to get the "struct clk" or if I should
keep clk_hw_onecell_data.
If it's the former then I'll also add a TODO comment for the
conversion to devm_clk_hw_get_clk() so it's easy to find.


Regards,
Martin

[-- Attachment #2: temp.diff --]
[-- Type: text/x-patch, Size: 3726 bytes --]

diff --git a/drivers/mmc/host/meson-mx-sdhc-clkc.c b/drivers/mmc/host/meson-mx-sdhc-clkc.c
index a36483d859f8..d2e36517e0f6 100644
--- a/drivers/mmc/host/meson-mx-sdhc-clkc.c
+++ b/drivers/mmc/host/meson-mx-sdhc-clkc.c
@@ -7,6 +7,7 @@
 
 #include <dt-bindings/clock/meson-mx-sdhc-clkc.h>
 
+#include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/device.h>
 #include <linux/platform_device.h>
@@ -22,7 +23,6 @@ struct meson_mx_sdhc_clkc {
 	struct clk_gate			tx_clk_en;
 	struct clk_gate			rx_clk_en;
 	struct clk_gate			sd_clk_en;
-	struct clk_hw_onecell_data	hw_onecell_data;
 };
 
 static const struct clk_parent_data meson_mx_sdhc_src_sel_parents[4] = {
@@ -83,17 +83,14 @@ static int meson_mx_sdhc_gate_clk_hw_register(struct device *dev,
 					     &clk_gate_ops, hw);
 }
 
-int meson_mx_sdhc_register_clkc(struct device *dev, void __iomem *base)
+int meson_mx_sdhc_register_clkc(struct device *dev, void __iomem *base,
+				struct clk_bulk_data *clk_bulk_data)
 {
 	struct clk_parent_data div_parent = { 0 };
-	struct clk_hw_onecell_data *onecell_data;
 	struct meson_mx_sdhc_clkc *clkc_data;
 	int ret;
 
-	clkc_data = devm_kzalloc(dev, struct_size(clkc_data,
-						  hw_onecell_data.hws,
-						  MESON_SDHC_NUM_BUILTIN_CLKS),
-				 GFP_KERNEL);
+	clkc_data = devm_kzalloc(dev, sizeof(*clkc_data), GFP_KERNEL);
 	if (!clkc_data)
 		return -ENOMEM;
 
@@ -150,16 +147,14 @@ int meson_mx_sdhc_register_clkc(struct device *dev, void __iomem *base)
 	if (ret)
 		return ret;
 
-	onecell_data = &clkc_data->hw_onecell_data;
-	onecell_data->hws[SDHC_CLKID_SRC_SEL] = &clkc_data->src_sel.hw;
-	onecell_data->hws[SDHC_CLKID_DIV] = &clkc_data->div.hw;
-	onecell_data->hws[SDHC_CLKID_MOD_CLK] = &clkc_data->mod_clk_en.hw;
-	onecell_data->hws[SDHC_CLKID_SD_CLK] = &clkc_data->sd_clk_en.hw;
-	onecell_data->hws[SDHC_CLKID_TX_CLK] = &clkc_data->tx_clk_en.hw;
-	onecell_data->hws[SDHC_CLKID_RX_CLK] = &clkc_data->rx_clk_en.hw;
+	/*
+	 * TODO: Replace clk_hw.clk with devm_clk_hw_get_clk() once that is
+	 * available.
+	 */
+	clk_bulk_data[0].clk = clkc_data->mod_clk_en.hw.clk;
+	clk_bulk_data[1].clk = clkc_data->sd_clk_en.hw.clk;
+	clk_bulk_data[2].clk = clkc_data->tx_clk_en.hw.clk;
+	clk_bulk_data[3].clk = clkc_data->rx_clk_en.hw.clk;
 
-	onecell_data->num = MESON_SDHC_NUM_BUILTIN_CLKS;
-
-	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
-					   onecell_data);
+	return 0;
 }
diff --git a/drivers/mmc/host/meson-mx-sdhc.c b/drivers/mmc/host/meson-mx-sdhc.c
index 3c54d5f91fbc..322570570c98 100644
--- a/drivers/mmc/host/meson-mx-sdhc.c
+++ b/drivers/mmc/host/meson-mx-sdhc.c
@@ -803,16 +803,7 @@ static int meson_mx_sdhc_probe(struct platform_device *pdev)
 
 	meson_mx_sdhc_init_hw(mmc);
 
-	ret = meson_mx_sdhc_register_clkc(dev, base);
-	if (ret)
-		goto err_disable_pclk;
-
-	host->bulk_clks[0].id = "mod_clk";
-	host->bulk_clks[1].id = "sd_clk";
-	host->bulk_clks[2].id = "tx_clk";
-	host->bulk_clks[3].id = "rx_clk";
-	ret = devm_clk_bulk_get(dev, MESON_SDHC_NUM_BULK_CLKS,
-				host->bulk_clks);
+	ret = meson_mx_sdhc_register_clkc(dev, base, host->bulk_clks);
 	if (ret)
 		goto err_disable_pclk;
 
diff --git a/drivers/mmc/host/meson-mx-sdhc.h b/drivers/mmc/host/meson-mx-sdhc.h
index 2aa1e4401173..230e8fbe6b3f 100644
--- a/drivers/mmc/host/meson-mx-sdhc.h
+++ b/drivers/mmc/host/meson-mx-sdhc.h
@@ -133,6 +133,9 @@
 	#define MESON_SDHC_CLK2_RX_CLK_PHASE			GENMASK(11, 0)
 	#define MESON_SDHC_CLK2_SD_CLK_PHASE			GENMASK(23, 12)
 
-int meson_mx_sdhc_register_clkc(struct device *dev, void __iomem *base);
+struct clk_bulk_data;
+
+int meson_mx_sdhc_register_clkc(struct device *dev, void __iomem *base,
+				struct clk_bulk_data *clk_bulk_data);
 
 #endif /* _MESON_MX_SDHC_H_ */