[PATCH 0/3] Add ZynqMP SD Clock Tap Delays configuration support

[PATCH 0/3] Add ZynqMP SD Clock Tap Delays configuration support

[Manish Narani]

This patch series adds support to configure SD tap delays on ZynqMP platforms
using Xilinx firmware driver and clock framework APIs.

Manish Narani (3):
  firmware: xilinx: Add SDIO Tap Delay API
  dt-bindings: mmc: arasan: Document 'xlnx,zynqmp-8.9a' controller
  mmc: sdhci-of-arasan: Add support for ZynqMP Platform Tap Delays Setup

 .../devicetree/bindings/mmc/arasan,sdhci.txt       |  32 ++++
 drivers/firmware/xilinx/zynqmp.c                   |  32 ++++
 drivers/mmc/host/sdhci-of-arasan.c                 | 173 ++++++++++++++++++++-
 include/linux/firmware/xlnx-zynqmp.h               |  17 +-
 4 files changed, 252 insertions(+), 2 deletions(-)

-- 
2.1.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/3] firmware: xilinx: Add SDIO Tap Delay API

[Manish Narani]

Add API for setting SDIO Tap Delays on ZynqMP platforms.

Signed-off-by: Manish Narani <manish.narani@xilinx.com>
---
 drivers/firmware/xilinx/zynqmp.c     | 32 ++++++++++++++++++++++++++++++++
 include/linux/firmware/xlnx-zynqmp.h | 17 ++++++++++++++++-
 2 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/xilinx/zynqmp.c b/drivers/firmware/xilinx/zynqmp.c
index fd3d837..c6f9e72 100644
--- a/drivers/firmware/xilinx/zynqmp.c
+++ b/drivers/firmware/xilinx/zynqmp.c
@@ -664,6 +664,37 @@ static int zynqmp_pm_set_requirement(const u32 node, const u32 capabilities,
 				   qos, ack, NULL);
 }
 
+/**
+ * zynqmp_pm_sdio_setphase() - PM call to set clock delays for SD clock
+ * @device_id:		Device ID of the SD controller
+ * @degrees:		Tap Delay value in degrees for Input/Output clocks
+ *
+ * This API function is to be used for setting the clock delays for SD
+ * clock.
+ *
+ * Return: Returns status, either success or error+reason
+ */
+static int zynqmp_pm_sdio_setphase(u32 device_id, int degrees)
+{
+	u32 node_id = (!device_id) ? NODE_SD_0 : NODE_SD_1;
+	enum tap_delay_type tap_type;
+	int ret;
+
+	if (degrees < INPUT_TAP_BOUNDARY) {
+		tap_type = PM_TAPDELAY_INPUT;
+	} else {
+		tap_type = PM_TAPDELAY_OUTPUT;
+		degrees -= INPUT_TAP_BOUNDARY;
+	}
+
+	ret = zynqmp_pm_ioctl(node_id, IOCTL_SET_SD_TAPDELAY, tap_type,
+			      degrees, NULL);
+	if (ret)
+		pr_err("Error setting Tap Delay\n");
+
+	return ret;
+}
+
 static const struct zynqmp_eemi_ops eemi_ops = {
 	.get_api_version = zynqmp_pm_get_api_version,
 	.get_chipid = zynqmp_pm_get_chipid,
@@ -687,6 +718,7 @@ static const struct zynqmp_eemi_ops eemi_ops = {
 	.set_requirement = zynqmp_pm_set_requirement,
 	.fpga_load = zynqmp_pm_fpga_load,
 	.fpga_get_status = zynqmp_pm_fpga_get_status,
+	.sdio_setphase = zynqmp_pm_sdio_setphase,
 };
 
 /**
diff --git a/include/linux/firmware/xlnx-zynqmp.h b/include/linux/firmware/xlnx-zynqmp.h
index 1262ea6..0fc4bf7 100644
--- a/include/linux/firmware/xlnx-zynqmp.h
+++ b/include/linux/firmware/xlnx-zynqmp.h
@@ -56,6 +56,9 @@
 #define XILINX_ZYNQMP_PM_FPGA_FULL	0x0U
 #define XILINX_ZYNQMP_PM_FPGA_PARTIAL	BIT(0)
 
+/* Input Tap Delay Boundary Value */
+#define INPUT_TAP_BOUNDARY		0x100
+
 enum pm_api_id {
 	PM_GET_API_VERSION = 1,
 	PM_REQUEST_NODE = 13,
@@ -92,7 +95,8 @@ enum pm_ret_status {
 };
 
 enum pm_ioctl_id {
-	IOCTL_SET_PLL_FRAC_MODE = 8,
+	IOCTL_SET_SD_TAPDELAY = 7,
+	IOCTL_SET_PLL_FRAC_MODE,
 	IOCTL_GET_PLL_FRAC_MODE,
 	IOCTL_SET_PLL_FRAC_DATA,
 	IOCTL_GET_PLL_FRAC_DATA,
@@ -251,6 +255,16 @@ enum zynqmp_pm_request_ack {
 	ZYNQMP_PM_REQUEST_ACK_NON_BLOCKING,
 };
 
+enum pm_node_id {
+	NODE_SD_0 = 39,
+	NODE_SD_1,
+};
+
+enum tap_delay_type {
+	PM_TAPDELAY_INPUT = 0,
+	PM_TAPDELAY_OUTPUT,
+};
+
 /**
  * struct zynqmp_pm_query_data - PM query data
  * @qid:	query ID
@@ -295,6 +309,7 @@ struct zynqmp_eemi_ops {
 			       const u32 capabilities,
 			       const u32 qos,
 			       const enum zynqmp_pm_request_ack ack);
+	int (*sdio_setphase)(u32 device_id, int degrees);
 };
 
 int zynqmp_pm_invoke_fn(u32 pm_api_id, u32 arg0, u32 arg1,
-- 
2.1.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/3] dt-bindings: mmc: arasan: Document 'xlnx, zynqmp-8.9a' controller

[Manish Narani]

Add documentation for 'xlnx,zynqmp-8.9a' SDHCI controller and optional
properties followed by example.

Signed-off-by: Manish Narani <manish.narani@xilinx.com>
---
 .../devicetree/bindings/mmc/arasan,sdhci.txt       | 32 ++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
index 1edbb04..6945b3b 100644
--- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
+++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
@@ -15,6 +15,9 @@ Required Properties:
     - "arasan,sdhci-5.1": generic Arasan SDHCI 5.1 PHY
     - "rockchip,rk3399-sdhci-5.1", "arasan,sdhci-5.1": rk3399 eMMC PHY
       For this device it is strongly suggested to include arasan,soc-ctl-syscon.
+    - "xlnx,zynqmp-8.9a": ZynqMP SDHCI 8.9a PHY
+      For this device it is strongly suggested to include clock-output-names and
+      #clock-cells.
     - "ti,am654-sdhci-5.1", "arasan,sdhci-5.1": TI AM654 MMC PHY
 	Note: This binding has been deprecated and moved to [5].
 
@@ -45,6 +48,24 @@ Optional Properties:
   - xlnx,int-clock-stable-broken: when present, the controller always reports
     that the internal clock is stable even when it is not.
 
+Optional Properties for "xlnx,zynqmp-8.9a":
+  - xlnx,tap-delay-mmc-hsd: Input/Output Tap Delays in degrees for MMC HS.
+  - xlnx,tap-delay-sd-hsd: Input/Output Tap Delays in degrees for SD HS.
+  - xlnx,tap-delay-sdr25: Input/Output Tap Delays in degrees for SDR25.
+  - xlnx,tap-delay-sdr50: Input/Output Tap Delays in degrees for SDR50.
+  - xlnx,tap-delay-sdr104: Input/Output Tap Delays in degrees for SDR104.
+  - xlnx,tap-delay-sd-ddr50: Input/Output Tap Delays in degrees for SD DDR50.
+  - xlnx,tap-delay-mmc-ddr52: Input/Output Tap Delays in degrees for MMC DDR52.
+  - xlnx,tap-delay-mmc-hs200: Input/Output Tap Delays in degrees for MMC HS200.
+
+  Above mentioned are the clock (phase) delays which are to be configured in the
+  controller while switching to particular speed mode. If not specified, driver
+  will configure the default value defined for particular mode in it.
+
+  - xlnx,mio-bank: When specified, this will indicate the MIO bank number in
+    which the command and data lines are configured. If not specified, driver
+    will assume this as 0.
+
 Example:
 	sdhci@e0100000 {
 		compatible = "arasan,sdhci-8.9a";
@@ -80,3 +101,14 @@ Example:
 		phy-names = "phy_arasan";
 		#clock-cells = <0>;
 	};
+
+	sdhci: mmc@ff160000 {
+		compatible = "xlnx,zynqmp-8.9a", "arasan,sdhci-8.9a";
+		interrupt-parent = <&gic>;
+		interrupts = <0 48 4>;
+		reg = <0x0 0xff160000 0x0 0x1000>;
+		clock-names = "clk_xin", "clk_ahb";
+		clock-output-names = "clk_sd0";
+		#clock-cells = <0>;
+		xlnx,tap-delay-sd-hsd = <21>, <6>;
+	};
-- 
2.1.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/3] mmc: sdhci-of-arasan: Add support for ZynqMP Platform Tap Delays Setup

[Manish Narani]

Apart from taps set by auto tuning, ZynqMP platform has feature to set
the tap values manually. Add support to read tap delay values from
DT and set the same in HW via ZynqMP SoC framework. Reading Tap
Delays from DT is optional, if the property is not available in DT the
driver will use the pre-defined Tap Delay Values.

Signed-off-by: Manish Narani <manish.narani@xilinx.com>
---
 drivers/mmc/host/sdhci-of-arasan.c | 173 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 172 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
index b12abf9..7af6cec 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -22,6 +22,7 @@
 #include <linux/phy/phy.h>
 #include <linux/regmap.h>
 #include <linux/of.h>
+#include <linux/firmware/xlnx-zynqmp.h>
 
 #include "cqhci.h"
 #include "sdhci-pltfm.h"
@@ -32,6 +33,10 @@
 
 #define PHY_CLK_TOO_SLOW_HZ		400000
 
+/* Default settings for ZynqMP Tap Delays */
+#define ZYNQMP_ITAP_DELAYS {0, 0x15, 0x15, 0, 0x15, 0, 0, 0x3D, 0x12, 0, 0}
+#define ZYNQMP_OTAP_DELAYS {0, 0x5, 0x6, 0, 0x5, 0x3, 0x3, 0x4, 0x6, 0x3, 0}
+
 /*
  * On some SoCs the syscon area has a feature where the upper 16-bits of
  * each 32-bit register act as a write mask for the lower 16-bits.  This allows
@@ -81,6 +86,7 @@ struct sdhci_arasan_soc_ctl_map {
  * @sdcardclk:		Pointer to normal 'struct clock' for sdcardclk_hw.
  * @soc_ctl_base:	Pointer to regmap for syscon for soc_ctl registers.
  * @soc_ctl_map:	Map to get offsets into soc_ctl registers.
+ * @of_data:		Platform specific runtime data storage pointer
  */
 struct sdhci_arasan_data {
 	struct sdhci_host *host;
@@ -101,6 +107,15 @@ struct sdhci_arasan_data {
 /* Controller immediately reports SDHCI_CLOCK_INT_STABLE after enabling the
  * internal clock even when the clock isn't stable */
 #define SDHCI_ARASAN_QUIRK_CLOCK_UNSTABLE BIT(1)
+
+	void *of_data;
+};
+
+struct sdhci_arasan_zynqmp_data {
+	void (*set_tap_delay)(struct sdhci_host *host);
+	const struct zynqmp_eemi_ops *eemi_ops;
+	u8 tapdly[MMC_TIMING_MMC_HS400 + 1][2]; /* [0] for input delay, */
+						/* [1] for output delay */
 };
 
 struct sdhci_arasan_of_data {
@@ -209,6 +224,16 @@ static void sdhci_arasan_set_clock(struct sdhci_host *host, unsigned int clock)
 		sdhci_arasan->is_phy_on = false;
 	}
 
+	/* Set the Input and Output Tap Delays */
+	if (host->version >= SDHCI_SPEC_300 &&
+	    host->timing != MMC_TIMING_LEGACY &&
+	    host->timing != MMC_TIMING_UHS_SDR12) {
+		struct sdhci_arasan_zynqmp_data *zynqmp_data =
+			sdhci_arasan->of_data;
+		if (zynqmp_data && zynqmp_data->set_tap_delay)
+			zynqmp_data->set_tap_delay(host);
+	}
+
 	sdhci_set_clock(host, clock);
 
 	if (sdhci_arasan->quirks & SDHCI_ARASAN_QUIRK_CLOCK_UNSTABLE)
@@ -487,6 +512,10 @@ static const struct of_device_id sdhci_arasan_of_match[] = {
 		.compatible = "arasan,sdhci-4.9a",
 		.data = &sdhci_arasan_data,
 	},
+	{
+		.compatible = "xlnx,zynqmp-8.9a",
+		.data = &sdhci_arasan_data,
+	},
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, sdhci_arasan_of_match);
@@ -517,6 +546,37 @@ static const struct clk_ops arasan_sdcardclk_ops = {
 };
 
 /**
+ * sdhci_zynqmp_sdcardclk_set_phase - Set the SD Clock Tap Delays
+ *
+ * Set the SD Clock Tap Delays for Input and Output paths
+ *
+ * @hw:			Pointer to the hardware clock structure.
+ * @degrees		The clock phase shift between 0 - 359.
+ * Return: 0 on success and error value on error
+ */
+static int sdhci_zynqmp_sdcardclk_set_phase(struct clk_hw *hw, int degrees)
+
+{
+	struct sdhci_arasan_data *sdhci_arasan =
+		container_of(hw, struct sdhci_arasan_data, sdcardclk_hw);
+	struct sdhci_arasan_zynqmp_data *zynqmp_data = sdhci_arasan->of_data;
+	const struct zynqmp_eemi_ops *eemi_ops = zynqmp_data->eemi_ops;
+	const char *clk_name = clk_hw_get_name(hw);
+	u32 device_id = !strcmp(clk_name, "clk_sd0") ? 0 : 1;
+
+	if (!eemi_ops->sdio_setphase)
+		return -ENODEV;
+
+	/* Set the Clock Phase */
+	return eemi_ops->sdio_setphase(device_id, degrees);
+}
+
+static const struct clk_ops zynqmp_sdcardclk_ops = {
+	.recalc_rate = sdhci_arasan_sdcardclk_recalc_rate,
+	.set_phase = sdhci_zynqmp_sdcardclk_set_phase,
+};
+
+/**
  * sdhci_arasan_update_clockmultiplier - Set corecfg_clockmultiplier
  *
  * The corecfg_clockmultiplier is supposed to contain clock multiplier
@@ -638,7 +698,10 @@ static int sdhci_arasan_register_sdclk(struct sdhci_arasan_data *sdhci_arasan,
 	sdcardclk_init.parent_names = &parent_clk_name;
 	sdcardclk_init.num_parents = 1;
 	sdcardclk_init.flags = CLK_GET_RATE_NOCACHE;
-	sdcardclk_init.ops = &arasan_sdcardclk_ops;
+	if (of_device_is_compatible(np, "xlnx,zynqmp-8.9a"))
+		sdcardclk_init.ops = &zynqmp_sdcardclk_ops;
+	else
+		sdcardclk_init.ops = &arasan_sdcardclk_ops;
 
 	sdhci_arasan->sdcardclk_hw.init = &sdcardclk_init;
 	sdhci_arasan->sdcardclk =
@@ -714,6 +777,108 @@ static int sdhci_arasan_add_host(struct sdhci_arasan_data *sdhci_arasan)
 	return ret;
 }
 
+static void sdhci_arasan_zynqmp_set_tap_delay(struct sdhci_host *host)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
+	struct sdhci_arasan_zynqmp_data *zynqmp_data = sdhci_arasan->of_data;
+
+	clk_set_phase(sdhci_arasan->sdcardclk,
+		      (int)zynqmp_data->tapdly[host->timing][0]);
+	clk_set_phase(sdhci_arasan->sdcardclk,
+		      (int)zynqmp_data->tapdly[host->timing][1] +
+		      INPUT_TAP_BOUNDARY);
+}
+
+static void arasan_dt_read_tap_delay(struct device *dev, u8 *tapdly,
+				     const char *prop, u8 itap_def, u8 otap_def)
+{
+	struct device_node *np = dev->of_node;
+
+	tapdly[0] = itap_def;
+	tapdly[1] = otap_def;
+
+	/*
+	 * Read Tap Delay values from DT, if the DT does not contain the
+	 * Tap Values then use the pre-defined values.
+	 */
+	if (of_property_read_variable_u8_array(np, prop, &tapdly[0], 2, 0)) {
+		dev_dbg(dev, "Using predefined tapdly for %s = %d %d\n",
+			prop, tapdly[0], tapdly[1]);
+	}
+}
+
+/**
+ * arasan_dt_parse_tap_delays - Read Tap Delay values from DT
+ *
+ * Called at initialization to parse the values of Tap Delays.
+ *
+ * @dev:		Pointer to our struct device.
+ */
+static int arasan_dt_parse_tap_delays(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct sdhci_host *host = platform_get_drvdata(pdev);
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
+	struct sdhci_arasan_zynqmp_data zynqmp_data;
+	const struct zynqmp_eemi_ops *eemi_ops;
+	u8 *itapdly, *otapdly;
+	u32 mio_bank = 0;
+
+	eemi_ops = zynqmp_pm_get_eemi_ops();
+	if (IS_ERR(eemi_ops))
+		return PTR_ERR(eemi_ops);
+
+	itapdly = (u8 [MMC_TIMING_MMC_HS400 + 1]) ZYNQMP_ITAP_DELAYS;
+	otapdly = (u8 [MMC_TIMING_MMC_HS400 + 1]) ZYNQMP_OTAP_DELAYS;
+
+	of_property_read_u32(pdev->dev.of_node, "xlnx,mio-bank", &mio_bank);
+	if (mio_bank == 2) {
+		otapdly[MMC_TIMING_UHS_SDR104] = 0x2;
+		otapdly[MMC_TIMING_MMC_HS200] = 0x2;
+	}
+
+	arasan_dt_read_tap_delay(dev, zynqmp_data.tapdly[MMC_TIMING_MMC_HS],
+				 "xlnx,tap-delay-mmc-hsd",
+				 itapdly[MMC_TIMING_MMC_HS],
+				 otapdly[MMC_TIMING_MMC_HS]);
+	arasan_dt_read_tap_delay(dev, zynqmp_data.tapdly[MMC_TIMING_SD_HS],
+				 "xlnx,tap-delay-sd-hsd",
+				 itapdly[MMC_TIMING_SD_HS],
+				 otapdly[MMC_TIMING_SD_HS]);
+	arasan_dt_read_tap_delay(dev, zynqmp_data.tapdly[MMC_TIMING_UHS_SDR25],
+				 "xlnx,tap-delay-sdr25",
+				 itapdly[MMC_TIMING_UHS_SDR25],
+				 otapdly[MMC_TIMING_UHS_SDR25]);
+	arasan_dt_read_tap_delay(dev, zynqmp_data.tapdly[MMC_TIMING_UHS_SDR50],
+				 "xlnx,tap-delay-sdr50",
+				 itapdly[MMC_TIMING_UHS_SDR50],
+				 otapdly[MMC_TIMING_UHS_SDR50]);
+	arasan_dt_read_tap_delay(dev, zynqmp_data.tapdly[MMC_TIMING_UHS_SDR104],
+				 "xlnx,tap-delay-sdr104",
+				 itapdly[MMC_TIMING_UHS_SDR104],
+				 otapdly[MMC_TIMING_UHS_SDR104]);
+	arasan_dt_read_tap_delay(dev, zynqmp_data.tapdly[MMC_TIMING_UHS_DDR50],
+				 "xlnx,tap-delay-sd-ddr50",
+				 itapdly[MMC_TIMING_UHS_DDR50],
+				 otapdly[MMC_TIMING_UHS_DDR50]);
+	arasan_dt_read_tap_delay(dev, zynqmp_data.tapdly[MMC_TIMING_MMC_DDR52],
+				 "xlnx,tap-delay-mmc-ddr52",
+				 itapdly[MMC_TIMING_MMC_DDR52],
+				 otapdly[MMC_TIMING_MMC_DDR52]);
+	arasan_dt_read_tap_delay(dev, zynqmp_data.tapdly[MMC_TIMING_MMC_HS200],
+				 "xlnx,tap-delay-mmc-hs200",
+				 itapdly[MMC_TIMING_MMC_HS200],
+				 otapdly[MMC_TIMING_MMC_HS200]);
+
+	zynqmp_data.set_tap_delay = sdhci_arasan_zynqmp_set_tap_delay;
+	zynqmp_data.eemi_ops = eemi_ops;
+	sdhci_arasan->of_data = &zynqmp_data;
+
+	return 0;
+}
+
 static int sdhci_arasan_probe(struct platform_device *pdev)
 {
 	int ret;
@@ -806,6 +971,12 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
 		goto unreg_clk;
 	}
 
+	if (of_device_is_compatible(pdev->dev.of_node, "xlnx,zynqmp-8.9a")) {
+		ret = arasan_dt_parse_tap_delays(&pdev->dev);
+		if (ret)
+			goto unreg_clk;
+	}
+
 	sdhci_arasan->phy = ERR_PTR(-ENODEV);
 	if (of_device_is_compatible(pdev->dev.of_node,
 				    "arasan,sdhci-5.1")) {
-- 
2.1.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] mmc: sdhci-of-arasan: Add support for ZynqMP Platform Tap Delays Setup

[Adrian Hunter]

On 11/06/19 12:56 PM, Manish Narani wrote:
> Apart from taps set by auto tuning, ZynqMP platform has feature to set
> the tap values manually. Add support to read tap delay values from
> DT and set the same in HW via ZynqMP SoC framework. Reading Tap
> Delays from DT is optional, if the property is not available in DT the
> driver will use the pre-defined Tap Delay Values.
> 
> Signed-off-by: Manish Narani <manish.narani@xilinx.com>

OK for SDHCI:

Acked-by: Adrian Hunter <adrian.hunter@intel.com>


> ---
>  drivers/mmc/host/sdhci-of-arasan.c | 173 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 172 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index b12abf9..7af6cec 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -22,6 +22,7 @@
>  #include <linux/phy/phy.h>
>  #include <linux/regmap.h>
>  #include <linux/of.h>
> +#include <linux/firmware/xlnx-zynqmp.h>
>  
>  #include "cqhci.h"
>  #include "sdhci-pltfm.h"
> @@ -32,6 +33,10 @@
>  
>  #define PHY_CLK_TOO_SLOW_HZ		400000
>  
> +/* Default settings for ZynqMP Tap Delays */
> +#define ZYNQMP_ITAP_DELAYS {0, 0x15, 0x15, 0, 0x15, 0, 0, 0x3D, 0x12, 0, 0}
> +#define ZYNQMP_OTAP_DELAYS {0, 0x5, 0x6, 0, 0x5, 0x3, 0x3, 0x4, 0x6, 0x3, 0}
> +
>  /*
>   * On some SoCs the syscon area has a feature where the upper 16-bits of
>   * each 32-bit register act as a write mask for the lower 16-bits.  This allows
> @@ -81,6 +86,7 @@ struct sdhci_arasan_soc_ctl_map {
>   * @sdcardclk:		Pointer to normal 'struct clock' for sdcardclk_hw.
>   * @soc_ctl_base:	Pointer to regmap for syscon for soc_ctl registers.
>   * @soc_ctl_map:	Map to get offsets into soc_ctl registers.
> + * @of_data:		Platform specific runtime data storage pointer
>   */
>  struct sdhci_arasan_data {
>  	struct sdhci_host *host;
> @@ -101,6 +107,15 @@ struct sdhci_arasan_data {
>  /* Controller immediately reports SDHCI_CLOCK_INT_STABLE after enabling the
>   * internal clock even when the clock isn't stable */
>  #define SDHCI_ARASAN_QUIRK_CLOCK_UNSTABLE BIT(1)
> +
> +	void *of_data;
> +};
> +
> +struct sdhci_arasan_zynqmp_data {
> +	void (*set_tap_delay)(struct sdhci_host *host);
> +	const struct zynqmp_eemi_ops *eemi_ops;
> +	u8 tapdly[MMC_TIMING_MMC_HS400 + 1][2]; /* [0] for input delay, */
> +						/* [1] for output delay */
>  };
>  
>  struct sdhci_arasan_of_data {
> @@ -209,6 +224,16 @@ static void sdhci_arasan_set_clock(struct sdhci_host *host, unsigned int clock)
>  		sdhci_arasan->is_phy_on = false;
>  	}
>  
> +	/* Set the Input and Output Tap Delays */
> +	if (host->version >= SDHCI_SPEC_300 &&
> +	    host->timing != MMC_TIMING_LEGACY &&
> +	    host->timing != MMC_TIMING_UHS_SDR12) {
> +		struct sdhci_arasan_zynqmp_data *zynqmp_data =
> +			sdhci_arasan->of_data;
> +		if (zynqmp_data && zynqmp_data->set_tap_delay)
> +			zynqmp_data->set_tap_delay(host);
> +	}
> +
>  	sdhci_set_clock(host, clock);
>  
>  	if (sdhci_arasan->quirks & SDHCI_ARASAN_QUIRK_CLOCK_UNSTABLE)
> @@ -487,6 +512,10 @@ static const struct of_device_id sdhci_arasan_of_match[] = {
>  		.compatible = "arasan,sdhci-4.9a",
>  		.data = &sdhci_arasan_data,
>  	},
> +	{
> +		.compatible = "xlnx,zynqmp-8.9a",
> +		.data = &sdhci_arasan_data,
> +	},
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, sdhci_arasan_of_match);
> @@ -517,6 +546,37 @@ static const struct clk_ops arasan_sdcardclk_ops = {
>  };
>  
>  /**
> + * sdhci_zynqmp_sdcardclk_set_phase - Set the SD Clock Tap Delays
> + *
> + * Set the SD Clock Tap Delays for Input and Output paths
> + *
> + * @hw:			Pointer to the hardware clock structure.
> + * @degrees		The clock phase shift between 0 - 359.
> + * Return: 0 on success and error value on error
> + */
> +static int sdhci_zynqmp_sdcardclk_set_phase(struct clk_hw *hw, int degrees)
> +
> +{
> +	struct sdhci_arasan_data *sdhci_arasan =
> +		container_of(hw, struct sdhci_arasan_data, sdcardclk_hw);
> +	struct sdhci_arasan_zynqmp_data *zynqmp_data = sdhci_arasan->of_data;
> +	const struct zynqmp_eemi_ops *eemi_ops = zynqmp_data->eemi_ops;
> +	const char *clk_name = clk_hw_get_name(hw);
> +	u32 device_id = !strcmp(clk_name, "clk_sd0") ? 0 : 1;
> +
> +	if (!eemi_ops->sdio_setphase)
> +		return -ENODEV;
> +
> +	/* Set the Clock Phase */
> +	return eemi_ops->sdio_setphase(device_id, degrees);
> +}
> +
> +static const struct clk_ops zynqmp_sdcardclk_ops = {
> +	.recalc_rate = sdhci_arasan_sdcardclk_recalc_rate,
> +	.set_phase = sdhci_zynqmp_sdcardclk_set_phase,
> +};
> +
> +/**
>   * sdhci_arasan_update_clockmultiplier - Set corecfg_clockmultiplier
>   *
>   * The corecfg_clockmultiplier is supposed to contain clock multiplier
> @@ -638,7 +698,10 @@ static int sdhci_arasan_register_sdclk(struct sdhci_arasan_data *sdhci_arasan,
>  	sdcardclk_init.parent_names = &parent_clk_name;
>  	sdcardclk_init.num_parents = 1;
>  	sdcardclk_init.flags = CLK_GET_RATE_NOCACHE;
> -	sdcardclk_init.ops = &arasan_sdcardclk_ops;
> +	if (of_device_is_compatible(np, "xlnx,zynqmp-8.9a"))
> +		sdcardclk_init.ops = &zynqmp_sdcardclk_ops;
> +	else
> +		sdcardclk_init.ops = &arasan_sdcardclk_ops;
>  
>  	sdhci_arasan->sdcardclk_hw.init = &sdcardclk_init;
>  	sdhci_arasan->sdcardclk =
> @@ -714,6 +777,108 @@ static int sdhci_arasan_add_host(struct sdhci_arasan_data *sdhci_arasan)
>  	return ret;
>  }
>  
> +static void sdhci_arasan_zynqmp_set_tap_delay(struct sdhci_host *host)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
> +	struct sdhci_arasan_zynqmp_data *zynqmp_data = sdhci_arasan->of_data;
> +
> +	clk_set_phase(sdhci_arasan->sdcardclk,
> +		      (int)zynqmp_data->tapdly[host->timing][0]);
> +	clk_set_phase(sdhci_arasan->sdcardclk,
> +		      (int)zynqmp_data->tapdly[host->timing][1] +
> +		      INPUT_TAP_BOUNDARY);
> +}
> +
> +static void arasan_dt_read_tap_delay(struct device *dev, u8 *tapdly,
> +				     const char *prop, u8 itap_def, u8 otap_def)
> +{
> +	struct device_node *np = dev->of_node;
> +
> +	tapdly[0] = itap_def;
> +	tapdly[1] = otap_def;
> +
> +	/*
> +	 * Read Tap Delay values from DT, if the DT does not contain the
> +	 * Tap Values then use the pre-defined values.
> +	 */
> +	if (of_property_read_variable_u8_array(np, prop, &tapdly[0], 2, 0)) {
> +		dev_dbg(dev, "Using predefined tapdly for %s = %d %d\n",
> +			prop, tapdly[0], tapdly[1]);
> +	}
> +}
> +
> +/**
> + * arasan_dt_parse_tap_delays - Read Tap Delay values from DT
> + *
> + * Called at initialization to parse the values of Tap Delays.
> + *
> + * @dev:		Pointer to our struct device.
> + */
> +static int arasan_dt_parse_tap_delays(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct sdhci_host *host = platform_get_drvdata(pdev);
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
> +	struct sdhci_arasan_zynqmp_data zynqmp_data;
> +	const struct zynqmp_eemi_ops *eemi_ops;
> +	u8 *itapdly, *otapdly;
> +	u32 mio_bank = 0;
> +
> +	eemi_ops = zynqmp_pm_get_eemi_ops();
> +	if (IS_ERR(eemi_ops))
> +		return PTR_ERR(eemi_ops);
> +
> +	itapdly = (u8 [MMC_TIMING_MMC_HS400 + 1]) ZYNQMP_ITAP_DELAYS;
> +	otapdly = (u8 [MMC_TIMING_MMC_HS400 + 1]) ZYNQMP_OTAP_DELAYS;
> +
> +	of_property_read_u32(pdev->dev.of_node, "xlnx,mio-bank", &mio_bank);
> +	if (mio_bank == 2) {
> +		otapdly[MMC_TIMING_UHS_SDR104] = 0x2;
> +		otapdly[MMC_TIMING_MMC_HS200] = 0x2;
> +	}
> +
> +	arasan_dt_read_tap_delay(dev, zynqmp_data.tapdly[MMC_TIMING_MMC_HS],
> +				 "xlnx,tap-delay-mmc-hsd",
> +				 itapdly[MMC_TIMING_MMC_HS],
> +				 otapdly[MMC_TIMING_MMC_HS]);
> +	arasan_dt_read_tap_delay(dev, zynqmp_data.tapdly[MMC_TIMING_SD_HS],
> +				 "xlnx,tap-delay-sd-hsd",
> +				 itapdly[MMC_TIMING_SD_HS],
> +				 otapdly[MMC_TIMING_SD_HS]);
> +	arasan_dt_read_tap_delay(dev, zynqmp_data.tapdly[MMC_TIMING_UHS_SDR25],
> +				 "xlnx,tap-delay-sdr25",
> +				 itapdly[MMC_TIMING_UHS_SDR25],
> +				 otapdly[MMC_TIMING_UHS_SDR25]);
> +	arasan_dt_read_tap_delay(dev, zynqmp_data.tapdly[MMC_TIMING_UHS_SDR50],
> +				 "xlnx,tap-delay-sdr50",
> +				 itapdly[MMC_TIMING_UHS_SDR50],
> +				 otapdly[MMC_TIMING_UHS_SDR50]);
> +	arasan_dt_read_tap_delay(dev, zynqmp_data.tapdly[MMC_TIMING_UHS_SDR104],
> +				 "xlnx,tap-delay-sdr104",
> +				 itapdly[MMC_TIMING_UHS_SDR104],
> +				 otapdly[MMC_TIMING_UHS_SDR104]);
> +	arasan_dt_read_tap_delay(dev, zynqmp_data.tapdly[MMC_TIMING_UHS_DDR50],
> +				 "xlnx,tap-delay-sd-ddr50",
> +				 itapdly[MMC_TIMING_UHS_DDR50],
> +				 otapdly[MMC_TIMING_UHS_DDR50]);
> +	arasan_dt_read_tap_delay(dev, zynqmp_data.tapdly[MMC_TIMING_MMC_DDR52],
> +				 "xlnx,tap-delay-mmc-ddr52",
> +				 itapdly[MMC_TIMING_MMC_DDR52],
> +				 otapdly[MMC_TIMING_MMC_DDR52]);
> +	arasan_dt_read_tap_delay(dev, zynqmp_data.tapdly[MMC_TIMING_MMC_HS200],
> +				 "xlnx,tap-delay-mmc-hs200",
> +				 itapdly[MMC_TIMING_MMC_HS200],
> +				 otapdly[MMC_TIMING_MMC_HS200]);
> +
> +	zynqmp_data.set_tap_delay = sdhci_arasan_zynqmp_set_tap_delay;
> +	zynqmp_data.eemi_ops = eemi_ops;
> +	sdhci_arasan->of_data = &zynqmp_data;
> +
> +	return 0;
> +}
> +
>  static int sdhci_arasan_probe(struct platform_device *pdev)
>  {
>  	int ret;
> @@ -806,6 +971,12 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
>  		goto unreg_clk;
>  	}
>  
> +	if (of_device_is_compatible(pdev->dev.of_node, "xlnx,zynqmp-8.9a")) {
> +		ret = arasan_dt_parse_tap_delays(&pdev->dev);
> +		if (ret)
> +			goto unreg_clk;
> +	}
> +
>  	sdhci_arasan->phy = ERR_PTR(-ENODEV);
>  	if (of_device_is_compatible(pdev->dev.of_node,
>  				    "arasan,sdhci-5.1")) {
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] mmc: sdhci-of-arasan: Add support for ZynqMP Platform Tap Delays Setup

[Ulf Hansson]

On Tue, 11 Jun 2019 at 11:57, Manish Narani <manish.narani@xilinx.com> wrote:
>
> Apart from taps set by auto tuning, ZynqMP platform has feature to set
> the tap values manually. Add support to read tap delay values from
> DT and set the same in HW via ZynqMP SoC framework. Reading Tap
> Delays from DT is optional, if the property is not available in DT the
> driver will use the pre-defined Tap Delay Values.
>
> Signed-off-by: Manish Narani <manish.narani@xilinx.com>
> ---
>  drivers/mmc/host/sdhci-of-arasan.c | 173 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 172 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index b12abf9..7af6cec 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -22,6 +22,7 @@
>  #include <linux/phy/phy.h>
>  #include <linux/regmap.h>
>  #include <linux/of.h>
> +#include <linux/firmware/xlnx-zynqmp.h>
>
>  #include "cqhci.h"
>  #include "sdhci-pltfm.h"
> @@ -32,6 +33,10 @@
>
>  #define PHY_CLK_TOO_SLOW_HZ            400000
>
> +/* Default settings for ZynqMP Tap Delays */
> +#define ZYNQMP_ITAP_DELAYS {0, 0x15, 0x15, 0, 0x15, 0, 0, 0x3D, 0x12, 0, 0}
> +#define ZYNQMP_OTAP_DELAYS {0, 0x5, 0x6, 0, 0x5, 0x3, 0x3, 0x4, 0x6, 0x3, 0}
> +
>  /*
>   * On some SoCs the syscon area has a feature where the upper 16-bits of
>   * each 32-bit register act as a write mask for the lower 16-bits.  This allows
> @@ -81,6 +86,7 @@ struct sdhci_arasan_soc_ctl_map {
>   * @sdcardclk:         Pointer to normal 'struct clock' for sdcardclk_hw.
>   * @soc_ctl_base:      Pointer to regmap for syscon for soc_ctl registers.
>   * @soc_ctl_map:       Map to get offsets into soc_ctl registers.
> + * @of_data:           Platform specific runtime data storage pointer
>   */
>  struct sdhci_arasan_data {
>         struct sdhci_host *host;
> @@ -101,6 +107,15 @@ struct sdhci_arasan_data {
>  /* Controller immediately reports SDHCI_CLOCK_INT_STABLE after enabling the
>   * internal clock even when the clock isn't stable */
>  #define SDHCI_ARASAN_QUIRK_CLOCK_UNSTABLE BIT(1)
> +
> +       void *of_data;
> +};
> +
> +struct sdhci_arasan_zynqmp_data {
> +       void (*set_tap_delay)(struct sdhci_host *host);
> +       const struct zynqmp_eemi_ops *eemi_ops;
> +       u8 tapdly[MMC_TIMING_MMC_HS400 + 1][2]; /* [0] for input delay, */
> +                                               /* [1] for output delay */
>  };

Please use two different structs, one for the clock provider data and
one for the mmc variant/platform data. This makes the code more
readable.

In regards to the mmc data part, I suggest to drop the
->set_tap_delay() callback, but rather use a boolean flag to indicate
whether clock phases needs to be changed for the variant. Potentially
that could even be skipped and instead call clk_set_phase()
unconditionally, as the clock core deals fine with clock providers
that doesn't support the ->set_phase() callback.

[...]

Otherwise this looks good to me!

When it comes to patch1, I need an ack from Michal to pick it up.

Kind regards
Uffe

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] mmc: sdhci-of-arasan: Add support for ZynqMP Platform Tap Delays Setup

[Michal Simek]

Hi,

On 17. 06. 19 13:15, Ulf Hansson wrote:
> On Tue, 11 Jun 2019 at 11:57, Manish Narani <manish.narani@xilinx.com> wrote:
>>
>> Apart from taps set by auto tuning, ZynqMP platform has feature to set
>> the tap values manually. Add support to read tap delay values from
>> DT and set the same in HW via ZynqMP SoC framework. Reading Tap
>> Delays from DT is optional, if the property is not available in DT the
>> driver will use the pre-defined Tap Delay Values.
>>
>> Signed-off-by: Manish Narani <manish.narani@xilinx.com>
>> ---
>>  drivers/mmc/host/sdhci-of-arasan.c | 173 ++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 172 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
>> index b12abf9..7af6cec 100644
>> --- a/drivers/mmc/host/sdhci-of-arasan.c
>> +++ b/drivers/mmc/host/sdhci-of-arasan.c
>> @@ -22,6 +22,7 @@
>>  #include <linux/phy/phy.h>
>>  #include <linux/regmap.h>
>>  #include <linux/of.h>
>> +#include <linux/firmware/xlnx-zynqmp.h>
>>
>>  #include "cqhci.h"
>>  #include "sdhci-pltfm.h"
>> @@ -32,6 +33,10 @@
>>
>>  #define PHY_CLK_TOO_SLOW_HZ            400000
>>
>> +/* Default settings for ZynqMP Tap Delays */
>> +#define ZYNQMP_ITAP_DELAYS {0, 0x15, 0x15, 0, 0x15, 0, 0, 0x3D, 0x12, 0, 0}
>> +#define ZYNQMP_OTAP_DELAYS {0, 0x5, 0x6, 0, 0x5, 0x3, 0x3, 0x4, 0x6, 0x3, 0}
>> +
>>  /*
>>   * On some SoCs the syscon area has a feature where the upper 16-bits of
>>   * each 32-bit register act as a write mask for the lower 16-bits.  This allows
>> @@ -81,6 +86,7 @@ struct sdhci_arasan_soc_ctl_map {
>>   * @sdcardclk:         Pointer to normal 'struct clock' for sdcardclk_hw.
>>   * @soc_ctl_base:      Pointer to regmap for syscon for soc_ctl registers.
>>   * @soc_ctl_map:       Map to get offsets into soc_ctl registers.
>> + * @of_data:           Platform specific runtime data storage pointer
>>   */
>>  struct sdhci_arasan_data {
>>         struct sdhci_host *host;
>> @@ -101,6 +107,15 @@ struct sdhci_arasan_data {
>>  /* Controller immediately reports SDHCI_CLOCK_INT_STABLE after enabling the
>>   * internal clock even when the clock isn't stable */
>>  #define SDHCI_ARASAN_QUIRK_CLOCK_UNSTABLE BIT(1)
>> +
>> +       void *of_data;
>> +};
>> +
>> +struct sdhci_arasan_zynqmp_data {
>> +       void (*set_tap_delay)(struct sdhci_host *host);
>> +       const struct zynqmp_eemi_ops *eemi_ops;
>> +       u8 tapdly[MMC_TIMING_MMC_HS400 + 1][2]; /* [0] for input delay, */
>> +                                               /* [1] for output delay */
>>  };
> 
> Please use two different structs, one for the clock provider data and
> one for the mmc variant/platform data. This makes the code more
> readable.

Origin version before sending that out was using two fields.
+	u32 itapdly[MMC_TIMING_MMC_HS400 + 1];
+	u32 otapdly[MMC_TIMING_MMC_HS400 + 1];

I did asked for putting it together to two dimensional array for
improving readability of this code. The reason was that you need to take
care about input/output together.
One thing I was also suggesting was to use instead of 2 just enum values
to specify IN=0/OUT/MAX to improve readability of this.
Do you think that using enum should be enough?


> In regards to the mmc data part, I suggest to drop the
> ->set_tap_delay() callback, but rather use a boolean flag to indicate
> whether clock phases needs to be changed for the variant. Potentially
> that could even be skipped and instead call clk_set_phase()
> unconditionally, as the clock core deals fine with clock providers
> that doesn't support the ->set_phase() callback.

In connection to another version of this driver for latest Xilinx chip
it would be better to keep set_tap_delay callback in the driver. The
reason is that new chip/ip is capable to setup tap delays directly
without asking firmware to do it. That's why for versal IP there is a
need to call different setup_tap_delay function.

> 
> [...]
> 
> Otherwise this looks good to me!
> 
> When it comes to patch1, I need an ack from Michal to pick it up.

I am waiting till Rob ack dt binding and then I wanted to talk to you if
you want to take it with 1/3 or if you want me to take all of them via
my tree.
In previous releases I was taking them via my tree because there were
several subsystem changing firmware interface. In this cycle there are
just small changes to firmware interface that's why taking it via your
tree shouldn't be a problem too.

Thanks,
Michal


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] mmc: sdhci-of-arasan: Add support for ZynqMP Platform Tap Delays Setup

[Ulf Hansson]

On Mon, 17 Jun 2019 at 13:28, Michal Simek <michal.simek@xilinx.com> wrote:
>
> Hi,
>
> On 17. 06. 19 13:15, Ulf Hansson wrote:
> > On Tue, 11 Jun 2019 at 11:57, Manish Narani <manish.narani@xilinx.com> wrote:
> >>
> >> Apart from taps set by auto tuning, ZynqMP platform has feature to set
> >> the tap values manually. Add support to read tap delay values from
> >> DT and set the same in HW via ZynqMP SoC framework. Reading Tap
> >> Delays from DT is optional, if the property is not available in DT the
> >> driver will use the pre-defined Tap Delay Values.
> >>
> >> Signed-off-by: Manish Narani <manish.narani@xilinx.com>
> >> ---
> >>  drivers/mmc/host/sdhci-of-arasan.c | 173 ++++++++++++++++++++++++++++++++++++-
> >>  1 file changed, 172 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> >> index b12abf9..7af6cec 100644
> >> --- a/drivers/mmc/host/sdhci-of-arasan.c
> >> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> >> @@ -22,6 +22,7 @@
> >>  #include <linux/phy/phy.h>
> >>  #include <linux/regmap.h>
> >>  #include <linux/of.h>
> >> +#include <linux/firmware/xlnx-zynqmp.h>
> >>
> >>  #include "cqhci.h"
> >>  #include "sdhci-pltfm.h"
> >> @@ -32,6 +33,10 @@
> >>
> >>  #define PHY_CLK_TOO_SLOW_HZ            400000
> >>
> >> +/* Default settings for ZynqMP Tap Delays */
> >> +#define ZYNQMP_ITAP_DELAYS {0, 0x15, 0x15, 0, 0x15, 0, 0, 0x3D, 0x12, 0, 0}
> >> +#define ZYNQMP_OTAP_DELAYS {0, 0x5, 0x6, 0, 0x5, 0x3, 0x3, 0x4, 0x6, 0x3, 0}
> >> +
> >>  /*
> >>   * On some SoCs the syscon area has a feature where the upper 16-bits of
> >>   * each 32-bit register act as a write mask for the lower 16-bits.  This allows
> >> @@ -81,6 +86,7 @@ struct sdhci_arasan_soc_ctl_map {
> >>   * @sdcardclk:         Pointer to normal 'struct clock' for sdcardclk_hw.
> >>   * @soc_ctl_base:      Pointer to regmap for syscon for soc_ctl registers.
> >>   * @soc_ctl_map:       Map to get offsets into soc_ctl registers.
> >> + * @of_data:           Platform specific runtime data storage pointer
> >>   */
> >>  struct sdhci_arasan_data {
> >>         struct sdhci_host *host;
> >> @@ -101,6 +107,15 @@ struct sdhci_arasan_data {
> >>  /* Controller immediately reports SDHCI_CLOCK_INT_STABLE after enabling the
> >>   * internal clock even when the clock isn't stable */
> >>  #define SDHCI_ARASAN_QUIRK_CLOCK_UNSTABLE BIT(1)
> >> +
> >> +       void *of_data;
> >> +};
> >> +
> >> +struct sdhci_arasan_zynqmp_data {
> >> +       void (*set_tap_delay)(struct sdhci_host *host);
> >> +       const struct zynqmp_eemi_ops *eemi_ops;
> >> +       u8 tapdly[MMC_TIMING_MMC_HS400 + 1][2]; /* [0] for input delay, */
> >> +                                               /* [1] for output delay */
> >>  };
> >
> > Please use two different structs, one for the clock provider data and
> > one for the mmc variant/platform data. This makes the code more
> > readable.
>
> Origin version before sending that out was using two fields.
> +       u32 itapdly[MMC_TIMING_MMC_HS400 + 1];
> +       u32 otapdly[MMC_TIMING_MMC_HS400 + 1];
>
> I did asked for putting it together to two dimensional array for
> improving readability of this code. The reason was that you need to take
> care about input/output together.
> One thing I was also suggesting was to use instead of 2 just enum values
> to specify IN=0/OUT/MAX to improve readability of this.
> Do you think that using enum should be enough?

Not sure I understand what you suggest here, sorry. I have no problem
with the enums.

The important point I am trying to make here, is that we should split
the clock provider data and the mmc variant data, simply because those
doesn't really belong to each each other.

Something like this:

struct sdhci_arasan_zynqmp_data {
         bool tap_delays;
         u8 tapdly[MMC_TIMING_MMC_HS400 + 1][2]; /* [0] for input
delay, [1] for output delay */
         + other variant specific data one may want to put here
}

These are just regular mmc OF data that are parsed as any other
property of the mmc device.

The "const struct zynqmp_eemi_ops *eemi_ops; should then be moved into
a clock provider specific struct, which is assigned when calling
sdhci_arasan_register_sdclk. I understand that all the clock data is
folded into struct sdhci_arasan_data today, but I think that should be
moved into a "sub-struct" for the clock specifics.

Moreover, when registering the clock, we should convert from using
devm_clk_register() into devm_clk_hw_register() as the first one is
now deprecated.

>
>
> > In regards to the mmc data part, I suggest to drop the
> > ->set_tap_delay() callback, but rather use a boolean flag to indicate
> > whether clock phases needs to be changed for the variant. Potentially
> > that could even be skipped and instead call clk_set_phase()
> > unconditionally, as the clock core deals fine with clock providers
> > that doesn't support the ->set_phase() callback.
>
> In connection to another version of this driver for latest Xilinx chip
> it would be better to keep set_tap_delay callback in the driver. The
> reason is that new chip/ip is capable to setup tap delays directly
> without asking firmware to do it. That's why for versal IP there is a
> need to call different setup_tap_delay function.

The ->set_tap_delay() callback is for ZyncMp pointing to
sdhci_arasan_zynqmp_set_tap_delay(). This function calls the
clk_set_phase() API.

What does ->set_tap_delay() do for the latest version?

>
> >
> > [...]
> >
> > Otherwise this looks good to me!
> >
> > When it comes to patch1, I need an ack from Michal to pick it up.
>
> I am waiting till Rob ack dt binding and then I wanted to talk to you if
> you want to take it with 1/3 or if you want me to take all of them via
> my tree.
> In previous releases I was taking them via my tree because there were
> several subsystem changing firmware interface. In this cycle there are
> just small changes to firmware interface that's why taking it via your
> tree shouldn't be a problem too.

Okay, then let's target this via my mmc tree this time.

Kind regards
Uffe

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] mmc: sdhci-of-arasan: Add support for ZynqMP Platform Tap Delays Setup

[Michal Simek]

On 17. 06. 19 14:21, Ulf Hansson wrote:
> On Mon, 17 Jun 2019 at 13:28, Michal Simek <michal.simek@xilinx.com> wrote:
>>
>> Hi,
>>
>> On 17. 06. 19 13:15, Ulf Hansson wrote:
>>> On Tue, 11 Jun 2019 at 11:57, Manish Narani <manish.narani@xilinx.com> wrote:
>>>>
>>>> Apart from taps set by auto tuning, ZynqMP platform has feature to set
>>>> the tap values manually. Add support to read tap delay values from
>>>> DT and set the same in HW via ZynqMP SoC framework. Reading Tap
>>>> Delays from DT is optional, if the property is not available in DT the
>>>> driver will use the pre-defined Tap Delay Values.
>>>>
>>>> Signed-off-by: Manish Narani <manish.narani@xilinx.com>
>>>> ---
>>>>  drivers/mmc/host/sdhci-of-arasan.c | 173 ++++++++++++++++++++++++++++++++++++-
>>>>  1 file changed, 172 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
>>>> index b12abf9..7af6cec 100644
>>>> --- a/drivers/mmc/host/sdhci-of-arasan.c
>>>> +++ b/drivers/mmc/host/sdhci-of-arasan.c
>>>> @@ -22,6 +22,7 @@
>>>>  #include <linux/phy/phy.h>
>>>>  #include <linux/regmap.h>
>>>>  #include <linux/of.h>
>>>> +#include <linux/firmware/xlnx-zynqmp.h>
>>>>
>>>>  #include "cqhci.h"
>>>>  #include "sdhci-pltfm.h"
>>>> @@ -32,6 +33,10 @@
>>>>
>>>>  #define PHY_CLK_TOO_SLOW_HZ            400000
>>>>
>>>> +/* Default settings for ZynqMP Tap Delays */
>>>> +#define ZYNQMP_ITAP_DELAYS {0, 0x15, 0x15, 0, 0x15, 0, 0, 0x3D, 0x12, 0, 0}
>>>> +#define ZYNQMP_OTAP_DELAYS {0, 0x5, 0x6, 0, 0x5, 0x3, 0x3, 0x4, 0x6, 0x3, 0}
>>>> +
>>>>  /*
>>>>   * On some SoCs the syscon area has a feature where the upper 16-bits of
>>>>   * each 32-bit register act as a write mask for the lower 16-bits.  This allows
>>>> @@ -81,6 +86,7 @@ struct sdhci_arasan_soc_ctl_map {
>>>>   * @sdcardclk:         Pointer to normal 'struct clock' for sdcardclk_hw.
>>>>   * @soc_ctl_base:      Pointer to regmap for syscon for soc_ctl registers.
>>>>   * @soc_ctl_map:       Map to get offsets into soc_ctl registers.
>>>> + * @of_data:           Platform specific runtime data storage pointer
>>>>   */
>>>>  struct sdhci_arasan_data {
>>>>         struct sdhci_host *host;
>>>> @@ -101,6 +107,15 @@ struct sdhci_arasan_data {
>>>>  /* Controller immediately reports SDHCI_CLOCK_INT_STABLE after enabling the
>>>>   * internal clock even when the clock isn't stable */
>>>>  #define SDHCI_ARASAN_QUIRK_CLOCK_UNSTABLE BIT(1)
>>>> +
>>>> +       void *of_data;
>>>> +};
>>>> +
>>>> +struct sdhci_arasan_zynqmp_data {
>>>> +       void (*set_tap_delay)(struct sdhci_host *host);
>>>> +       const struct zynqmp_eemi_ops *eemi_ops;
>>>> +       u8 tapdly[MMC_TIMING_MMC_HS400 + 1][2]; /* [0] for input delay, */
>>>> +                                               /* [1] for output delay */
>>>>  };
>>>
>>> Please use two different structs, one for the clock provider data and
>>> one for the mmc variant/platform data. This makes the code more
>>> readable.
>>
>> Origin version before sending that out was using two fields.
>> +       u32 itapdly[MMC_TIMING_MMC_HS400 + 1];
>> +       u32 otapdly[MMC_TIMING_MMC_HS400 + 1];
>>
>> I did asked for putting it together to two dimensional array for
>> improving readability of this code. The reason was that you need to take
>> care about input/output together.
>> One thing I was also suggesting was to use instead of 2 just enum values
>> to specify IN=0/OUT/MAX to improve readability of this.
>> Do you think that using enum should be enough?
> 
> Not sure I understand what you suggest here, sorry. I have no problem
> with the enums.
> 
> The important point I am trying to make here, is that we should split
> the clock provider data and the mmc variant data, simply because those
> doesn't really belong to each each other.
> 
> Something like this:
> 
> struct sdhci_arasan_zynqmp_data {
>          bool tap_delays;
>          u8 tapdly[MMC_TIMING_MMC_HS400 + 1][2]; /* [0] for input
> delay, [1] for output delay */
>          + other variant specific data one may want to put here
> }
> 
> These are just regular mmc OF data that are parsed as any other
> property of the mmc device.
> 
> The "const struct zynqmp_eemi_ops *eemi_ops; should then be moved into
> a clock provider specific struct, which is assigned when calling
> sdhci_arasan_register_sdclk. I understand that all the clock data is
> folded into struct sdhci_arasan_data today, but I think that should be
> moved into a "sub-struct" for the clock specifics.
> 
> Moreover, when registering the clock, we should convert from using
> devm_clk_register() into devm_clk_hw_register() as the first one is
> now deprecated.

Ok. I got your point.


>>
>>
>>> In regards to the mmc data part, I suggest to drop the
>>> ->set_tap_delay() callback, but rather use a boolean flag to indicate
>>> whether clock phases needs to be changed for the variant. Potentially
>>> that could even be skipped and instead call clk_set_phase()
>>> unconditionally, as the clock core deals fine with clock providers
>>> that doesn't support the ->set_phase() callback.
>>
>> In connection to another version of this driver for latest Xilinx chip
>> it would be better to keep set_tap_delay callback in the driver. The
>> reason is that new chip/ip is capable to setup tap delays directly
>> without asking firmware to do it. That's why for versal IP there is a
>> need to call different setup_tap_delay function.
> 
> The ->set_tap_delay() callback is for ZyncMp pointing to
> sdhci_arasan_zynqmp_set_tap_delay(). This function calls the
> clk_set_phase() API.
> 
> What does ->set_tap_delay() do for the latest version?

There is different set of default tap delays which should be programmed
and it is done just via writing to registers which are the part of
controller address space.

>>
>>>
>>> [...]
>>>
>>> Otherwise this looks good to me!
>>>
>>> When it comes to patch1, I need an ack from Michal to pick it up.
>>
>> I am waiting till Rob ack dt binding and then I wanted to talk to you if
>> you want to take it with 1/3 or if you want me to take all of them via
>> my tree.
>> In previous releases I was taking them via my tree because there were
>> several subsystem changing firmware interface. In this cycle there are
>> just small changes to firmware interface that's why taking it via your
>> tree shouldn't be a problem too.
> 
> Okay, then let's target this via my mmc tree this time.

okay. Not a problem.

Thanks,
Michal


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] mmc: sdhci-of-arasan: Add support for ZynqMP Platform Tap Delays Setup

[Ulf Hansson]

[...]

> >>
> >>
> >>> In regards to the mmc data part, I suggest to drop the
> >>> ->set_tap_delay() callback, but rather use a boolean flag to indicate
> >>> whether clock phases needs to be changed for the variant. Potentially
> >>> that could even be skipped and instead call clk_set_phase()
> >>> unconditionally, as the clock core deals fine with clock providers
> >>> that doesn't support the ->set_phase() callback.
> >>
> >> In connection to another version of this driver for latest Xilinx chip
> >> it would be better to keep set_tap_delay callback in the driver. The
> >> reason is that new chip/ip is capable to setup tap delays directly
> >> without asking firmware to do it. That's why for versal IP there is a
> >> need to call different setup_tap_delay function.
> >
> > The ->set_tap_delay() callback is for ZyncMp pointing to
> > sdhci_arasan_zynqmp_set_tap_delay(). This function calls the
> > clk_set_phase() API.
> >
> > What does ->set_tap_delay() do for the latest version?
>
> There is different set of default tap delays which should be programmed
> and it is done just via writing to registers which are the part of
> controller address space.

Okay, I see.

Not sure what makes most sense to do here, but it sounds to me like
another ->set_phase() callback should be implemented for the clock
provider. In other words, calling clk_set_phase() should continue to
works just fine for this case as well. If it turns out to be
inconvenient, we can always add the ->set_tap_delay() at a later point
when it makes more sense.

[...]

Kind regards
Uffe

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 3/3] mmc: sdhci-of-arasan: Add support for ZynqMP Platform Tap Delays Setup

[Manish Narani]

Hi Uffe,

Thanks for the review. Please find my comments below.

> -----Original Message-----
> From: Ulf Hansson <ulf.hansson@linaro.org>
> Sent: Monday, June 17, 2019 8:29 PM
> To: Michal Simek <michals@xilinx.com>
> Cc: Manish Narani <MNARANI@xilinx.com>; Rob Herring
> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Adrian
> Hunter <adrian.hunter@intel.com>; Rajan Vaja <RAJANV@xilinx.com>; Jolly
> Shah <JOLLYS@xilinx.com>; Nava kishore Manne <navam@xilinx.com>; Olof
> Johansson <olof@lixom.net>; linux-mmc@vger.kernel.org; DTML
> <devicetree@vger.kernel.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; Linux ARM <linux-arm-kernel@lists.infradead.org>
> Subject: Re: [PATCH 3/3] mmc: sdhci-of-arasan: Add support for ZynqMP
> Platform Tap Delays Setup
> 
> [...]
> 
> > >>
> > >>
> > >>> In regards to the mmc data part, I suggest to drop the
> > >>> ->set_tap_delay() callback, but rather use a boolean flag to indicate
> > >>> whether clock phases needs to be changed for the variant. Potentially
> > >>> that could even be skipped and instead call clk_set_phase()
> > >>> unconditionally, as the clock core deals fine with clock providers
> > >>> that doesn't support the ->set_phase() callback.

In the current implementation, I am taking care of both the input and
output clock delays with the single clock (which is output clock) registration
and differentiating these tap delays based on their values
(<256 then input delay and  >= 256 then output delay), because that is
zynqmp specific. If we want to make this generic, we may need to
register 'another' clock which will be there as an input (sampling) clock
and then we can make this 'clk_set_phase()' be called unconditionally
each for both the clocks and let the platforms handle their clock part.
What's your take on this?

Thanks,
Manish
> > >>
> > >> In connection to another version of this driver for latest Xilinx chip
> > >> it would be better to keep set_tap_delay callback in the driver. The
> > >> reason is that new chip/ip is capable to setup tap delays directly
> > >> without asking firmware to do it. That's why for versal IP there is a
> > >> need to call different setup_tap_delay function.
> > >
> > > The ->set_tap_delay() callback is for ZyncMp pointing to
> > > sdhci_arasan_zynqmp_set_tap_delay(). This function calls the
> > > clk_set_phase() API.
> > >
> > > What does ->set_tap_delay() do for the latest version?
> >
> > There is different set of default tap delays which should be programmed
> > and it is done just via writing to registers which are the part of
> > controller address space.
> 
> Okay, I see.
> 
> Not sure what makes most sense to do here, but it sounds to me like
> another ->set_phase() callback should be implemented for the clock
> provider. In other words, calling clk_set_phase() should continue to
> works just fine for this case as well. If it turns out to be
> inconvenient, we can always add the ->set_tap_delay() at a later point
> when it makes more sense.


> 
> [...]
> 
> Kind regards
> Uffe
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 3/3] mmc: sdhci-of-arasan: Add support for ZynqMP Platform Tap Delays Setup

[Manish Narani]

Hi Uffe,


> -----Original Message-----
> From: Ulf Hansson <ulf.hansson@linaro.org>
> Sent: Monday, June 17, 2019 5:51 PM
[...]
> 
> The "const struct zynqmp_eemi_ops *eemi_ops; should then be moved into
> a clock provider specific struct, which is assigned when calling
> sdhci_arasan_register_sdclk. I understand that all the clock data is
> folded into struct sdhci_arasan_data today, but I think that should be
> moved into a "sub-struct" for the clock specifics.
> 
> Moreover, when registering the clock, we should convert from using
> devm_clk_register() into devm_clk_hw_register() as the first one is
> now deprecated.

Just a query here:
When we switch to using devm_clk_hw_register() here, it will register the clk_hw and return int.
Is there a way we can get the clk (related to the clk_hw registered) from the
clock framework?
I am asking this because we will need that clk pointer while calling clk_set_phase() function.

Thanks,
Manish
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] mmc: sdhci-of-arasan: Add support for ZynqMP Platform Tap Delays Setup

[Ulf Hansson]

On Wed, 19 Jun 2019 at 10:40, Manish Narani <MNARANI@xilinx.com> wrote:
>
> Hi Uffe,
>
>
> > -----Original Message-----
> > From: Ulf Hansson <ulf.hansson@linaro.org>
> > Sent: Monday, June 17, 2019 5:51 PM
> [...]
> >
> > The "const struct zynqmp_eemi_ops *eemi_ops; should then be moved into
> > a clock provider specific struct, which is assigned when calling
> > sdhci_arasan_register_sdclk. I understand that all the clock data is
> > folded into struct sdhci_arasan_data today, but I think that should be
> > moved into a "sub-struct" for the clock specifics.
> >
> > Moreover, when registering the clock, we should convert from using
> > devm_clk_register() into devm_clk_hw_register() as the first one is
> > now deprecated.
>
> Just a query here:
> When we switch to using devm_clk_hw_register() here, it will register the clk_hw and return int.
> Is there a way we can get the clk (related to the clk_hw registered) from the
> clock framework?
> I am asking this because we will need that clk pointer while calling clk_set_phase() function.

I assume devm_clk_get() should work fine?

Kind regards
Uffe

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] mmc: sdhci-of-arasan: Add support for ZynqMP Platform Tap Delays Setup

[Ulf Hansson]

On Tue, 18 Jun 2019 at 06:59, Manish Narani <MNARANI@xilinx.com> wrote:
>
> Hi Uffe,
>
> Thanks for the review. Please find my comments below.
>
> > -----Original Message-----
> > From: Ulf Hansson <ulf.hansson@linaro.org>
> > Sent: Monday, June 17, 2019 8:29 PM
> > To: Michal Simek <michals@xilinx.com>
> > Cc: Manish Narani <MNARANI@xilinx.com>; Rob Herring
> > <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Adrian
> > Hunter <adrian.hunter@intel.com>; Rajan Vaja <RAJANV@xilinx.com>; Jolly
> > Shah <JOLLYS@xilinx.com>; Nava kishore Manne <navam@xilinx.com>; Olof
> > Johansson <olof@lixom.net>; linux-mmc@vger.kernel.org; DTML
> > <devicetree@vger.kernel.org>; Linux Kernel Mailing List <linux-
> > kernel@vger.kernel.org>; Linux ARM <linux-arm-kernel@lists.infradead.org>
> > Subject: Re: [PATCH 3/3] mmc: sdhci-of-arasan: Add support for ZynqMP
> > Platform Tap Delays Setup
> >
> > [...]
> >
> > > >>
> > > >>
> > > >>> In regards to the mmc data part, I suggest to drop the
> > > >>> ->set_tap_delay() callback, but rather use a boolean flag to indicate
> > > >>> whether clock phases needs to be changed for the variant. Potentially
> > > >>> that could even be skipped and instead call clk_set_phase()
> > > >>> unconditionally, as the clock core deals fine with clock providers
> > > >>> that doesn't support the ->set_phase() callback.
>
> In the current implementation, I am taking care of both the input and
> output clock delays with the single clock (which is output clock) registration
> and differentiating these tap delays based on their values
> (<256 then input delay and  >= 256 then output delay), because that is
> zynqmp specific. If we want to make this generic, we may need to
> register 'another' clock which will be there as an input (sampling) clock
> and then we can make this 'clk_set_phase()' be called unconditionally
> each for both the clocks and let the platforms handle their clock part.
> What's your take on this?

Not sure exactly what you are suggesting, but my gut feeling says it
sounds good.

How is tap delays managed for both the input clock and the output
clock? Is some managed by the clock provider (which is probably
firmware in your case) and some managed by the MMC controller?

[...]

Kind regards
Uffe

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 3/3] mmc: sdhci-of-arasan: Add support for ZynqMP Platform Tap Delays Setup

[Manish Narani]

Hi Uffe,


> -----Original Message-----
> From: Ulf Hansson <ulf.hansson@linaro.org>
> Sent: Wednesday, June 19, 2019 7:09 PM
> To: Manish Narani <MNARANI@xilinx.com>
> Cc: Michal Simek <michals@xilinx.com>; Rob Herring <robh+dt@kernel.org>;
> Mark Rutland <mark.rutland@arm.com>; Adrian Hunter
> <adrian.hunter@intel.com>; Rajan Vaja <RAJANV@xilinx.com>; Jolly Shah
> <JOLLYS@xilinx.com>; Nava kishore Manne <navam@xilinx.com>; Olof
> Johansson <olof@lixom.net>; linux-mmc@vger.kernel.org; DTML
> <devicetree@vger.kernel.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; Linux ARM <linux-arm-kernel@lists.infradead.org>
> Subject: Re: [PATCH 3/3] mmc: sdhci-of-arasan: Add support for ZynqMP
> Platform Tap Delays Setup
> 
> On Wed, 19 Jun 2019 at 10:40, Manish Narani <MNARANI@xilinx.com> wrote:
> >
> > Hi Uffe,
> >
> >
> > > -----Original Message-----
> > > From: Ulf Hansson <ulf.hansson@linaro.org>
> > > Sent: Monday, June 17, 2019 5:51 PM
> > [...]
> > >
> > > The "const struct zynqmp_eemi_ops *eemi_ops; should then be moved into
> > > a clock provider specific struct, which is assigned when calling
> > > sdhci_arasan_register_sdclk. I understand that all the clock data is
> > > folded into struct sdhci_arasan_data today, but I think that should be
> > > moved into a "sub-struct" for the clock specifics.
> > >
> > > Moreover, when registering the clock, we should convert from using
> > > devm_clk_register() into devm_clk_hw_register() as the first one is
> > > now deprecated.
> >
> > Just a query here:
> > When we switch to using devm_clk_hw_register() here, it will register the
> clk_hw and return int.
> > Is there a way we can get the clk (related to the clk_hw registered) from the
> > clock framework?
> > I am asking this because we will need that clk pointer while calling
> clk_set_phase() function.
> 
> I assume devm_clk_get() should work fine?

This clock does not come through ZynqMP Clock framework. We are initializing it in this 'sdhci-of-arasan' driver and getting only the clock name from "clock_output_names" property. So I think devm_clk_get() will not work here for our case.
I have gone through the clock framework and I found one function which may be used to create clock from clock hw, that is ' clk_hw_create_clk()' which can be used from our driver, however this needs change in the clock framework as below :

---
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index aa51756..4dc69ff 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3420,6 +3420,7 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
 
        return clk;
 }
+EXPORT_SYMBOL_GPL(clk_hw_create_clk);
 
 static int clk_cpy_name(const char **dst_p, const char *src, bool must_exist)
 {
diff --git a/drivers/clk/clk.h b/drivers/clk/clk.h
index d8400d6..2319899 100644
--- a/drivers/clk/clk.h
+++ b/drivers/clk/clk.h
@@ -22,17 +22,9 @@ static inline struct clk_hw *of_clk_get_hw(struct device_node *np,
 struct clk_hw *clk_find_hw(const char *dev_id, const char *con_id);
 
 #ifdef CONFIG_COMMON_CLK
-struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
-                             const char *dev_id, const char *con_id);
 void __clk_put(struct clk *clk);
 #else
 /* All these casts to avoid ifdefs in clkdev... */
-static inline struct clk *
-clk_hw_create_clk(struct device *dev, struct clk_hw *hw, const char *dev_id,
-                 const char *con_id)
-{
-       return (struct clk *)hw;
-}
 static struct clk_hw *__clk_get_hw(struct clk *clk)
 {
        return (struct clk_hw *)clk;
diff --git a/include/linux/clk.h b/include/linux/clk.h
index f689fc5..d3f60fe 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -18,6 +18,7 @@
 
 struct device;
 struct clk;
+struct clk_hw;
 struct device_node;
 struct of_phandle_args;
 
@@ -934,4 +935,15 @@ static inline struct clk *of_clk_get_from_provider(struct of_phandle_args *clksp
 }
 #endif
 
+#ifdef CONFIG_COMMON_CLK
+struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
+                             const char *dev_id, const char *con_id);
+#else
+static inline struct clk *
+clk_hw_create_clk(struct device *dev, struct clk_hw *hw, const char *dev_id,
+                 const char *con_id)
+{
+       return (struct clk *)hw;
+}
+#endif
 #endif
---

This change should help other drivers (outside 'drivers/clk/') as well for getting the clock created from clk_hw.
Is this fine to do?

Thanks,
Manish
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 3/3] mmc: sdhci-of-arasan: Add support for ZynqMP Platform Tap Delays Setup

[Manish Narani]

Hi Uffe,


> -----Original Message-----
> From: Ulf Hansson <ulf.hansson@linaro.org>
> Sent: Wednesday, June 19, 2019 8:11 PM
> To: Manish Narani <MNARANI@xilinx.com>
> Cc: Michal Simek <michals@xilinx.com>; Rob Herring <robh+dt@kernel.org>;
> Mark Rutland <mark.rutland@arm.com>; Adrian Hunter
> <adrian.hunter@intel.com>; Rajan Vaja <RAJANV@xilinx.com>; Jolly Shah
> <JOLLYS@xilinx.com>; Nava kishore Manne <navam@xilinx.com>; Olof
> Johansson <olof@lixom.net>; linux-mmc@vger.kernel.org; DTML
> <devicetree@vger.kernel.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; Linux ARM <linux-arm-kernel@lists.infradead.org>
> Subject: Re: [PATCH 3/3] mmc: sdhci-of-arasan: Add support for ZynqMP
> Platform Tap Delays Setup
> 
> On Tue, 18 Jun 2019 at 06:59, Manish Narani <MNARANI@xilinx.com> wrote:
> >
> > Hi Uffe,
> >
> > Thanks for the review. Please find my comments below.
> >
> > > -----Original Message-----
> > > From: Ulf Hansson <ulf.hansson@linaro.org>
> > > Sent: Monday, June 17, 2019 8:29 PM
> > > To: Michal Simek <michals@xilinx.com>
> > > Cc: Manish Narani <MNARANI@xilinx.com>; Rob Herring
> > > <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Adrian
> > > Hunter <adrian.hunter@intel.com>; Rajan Vaja <RAJANV@xilinx.com>; Jolly
> > > Shah <JOLLYS@xilinx.com>; Nava kishore Manne <navam@xilinx.com>;
> Olof
> > > Johansson <olof@lixom.net>; linux-mmc@vger.kernel.org; DTML
> > > <devicetree@vger.kernel.org>; Linux Kernel Mailing List <linux-
> > > kernel@vger.kernel.org>; Linux ARM <linux-arm-
> kernel@lists.infradead.org>
> > > Subject: Re: [PATCH 3/3] mmc: sdhci-of-arasan: Add support for ZynqMP
> > > Platform Tap Delays Setup
> > >
> > > [...]
> > >
> > > > >>
> > > > >>
> > > > >>> In regards to the mmc data part, I suggest to drop the
> > > > >>> ->set_tap_delay() callback, but rather use a boolean flag to indicate
> > > > >>> whether clock phases needs to be changed for the variant. Potentially
> > > > >>> that could even be skipped and instead call clk_set_phase()
> > > > >>> unconditionally, as the clock core deals fine with clock providers
> > > > >>> that doesn't support the ->set_phase() callback.
> >
> > In the current implementation, I am taking care of both the input and
> > output clock delays with the single clock (which is output clock) registration
> > and differentiating these tap delays based on their values
> > (<256 then input delay and  >= 256 then output delay), because that is
> > zynqmp specific. If we want to make this generic, we may need to
> > register 'another' clock which will be there as an input (sampling) clock
> > and then we can make this 'clk_set_phase()' be called unconditionally
> > each for both the clocks and let the platforms handle their clock part.
> > What's your take on this?
> 
> Not sure exactly what you are suggesting, but my gut feeling says it
> sounds good.
> 
> How is tap delays managed for both the input clock and the output
> clock? Is some managed by the clock provider (which is probably
> firmware in your case) and some managed by the MMC controller?

Yes, for the existing "xlnx,zynqmp-8.9a" controller, the tap delays will be managed by the firmware, however in the upcoming "xlnx,versal-8.9a" variant the tap delays will be managed by the MMC controller itself.
I will include the Versal one in the next series of patches.

Thanks,
Manish
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] mmc: sdhci-of-arasan: Add support for ZynqMP Platform Tap Delays Setup

[Ulf Hansson]

On Thu, 20 Jun 2019 at 10:14, Manish Narani <MNARANI@xilinx.com> wrote:
>
> Hi Uffe,
>
>
> > -----Original Message-----
> > From: Ulf Hansson <ulf.hansson@linaro.org>
> > Sent: Wednesday, June 19, 2019 7:09 PM
> > To: Manish Narani <MNARANI@xilinx.com>
> > Cc: Michal Simek <michals@xilinx.com>; Rob Herring <robh+dt@kernel.org>;
> > Mark Rutland <mark.rutland@arm.com>; Adrian Hunter
> > <adrian.hunter@intel.com>; Rajan Vaja <RAJANV@xilinx.com>; Jolly Shah
> > <JOLLYS@xilinx.com>; Nava kishore Manne <navam@xilinx.com>; Olof
> > Johansson <olof@lixom.net>; linux-mmc@vger.kernel.org; DTML
> > <devicetree@vger.kernel.org>; Linux Kernel Mailing List <linux-
> > kernel@vger.kernel.org>; Linux ARM <linux-arm-kernel@lists.infradead.org>
> > Subject: Re: [PATCH 3/3] mmc: sdhci-of-arasan: Add support for ZynqMP
> > Platform Tap Delays Setup
> >
> > On Wed, 19 Jun 2019 at 10:40, Manish Narani <MNARANI@xilinx.com> wrote:
> > >
> > > Hi Uffe,
> > >
> > >
> > > > -----Original Message-----
> > > > From: Ulf Hansson <ulf.hansson@linaro.org>
> > > > Sent: Monday, June 17, 2019 5:51 PM
> > > [...]
> > > >
> > > > The "const struct zynqmp_eemi_ops *eemi_ops; should then be moved into
> > > > a clock provider specific struct, which is assigned when calling
> > > > sdhci_arasan_register_sdclk. I understand that all the clock data is
> > > > folded into struct sdhci_arasan_data today, but I think that should be
> > > > moved into a "sub-struct" for the clock specifics.
> > > >
> > > > Moreover, when registering the clock, we should convert from using
> > > > devm_clk_register() into devm_clk_hw_register() as the first one is
> > > > now deprecated.
> > >
> > > Just a query here:
> > > When we switch to using devm_clk_hw_register() here, it will register the
> > clk_hw and return int.
> > > Is there a way we can get the clk (related to the clk_hw registered) from the
> > > clock framework?
> > > I am asking this because we will need that clk pointer while calling
> > clk_set_phase() function.
> >
> > I assume devm_clk_get() should work fine?
>
> This clock does not come through ZynqMP Clock framework. We are initializing it in this 'sdhci-of-arasan' driver and getting only the clock name from "clock_output_names" property. So I think devm_clk_get() will not work here for our case.

Well, I guess you need to register an OF clock provider to allow the
clock lookup to work. Apologize, but I don't have the time, currently
to point you in the exact direction.

However, in principle, my point is, there should be no difference
whether the clock is registered via the "ZynqMP Clock framework" or
via the mmc driver. The *clk_get() thing need to work, otherwise I
consider the clock registration in the mmc driver to be a hack. If you
see what I mean.

> I have gone through the clock framework and I found one function which may be used to create clock from clock hw, that is ' clk_hw_create_clk()' which can be used from our driver, however this needs change in the clock framework as below :
>
> ---
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index aa51756..4dc69ff 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3420,6 +3420,7 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
>
>         return clk;
>  }
> +EXPORT_SYMBOL_GPL(clk_hw_create_clk);
>
>  static int clk_cpy_name(const char **dst_p, const char *src, bool must_exist)
>  {
> diff --git a/drivers/clk/clk.h b/drivers/clk/clk.h
> index d8400d6..2319899 100644
> --- a/drivers/clk/clk.h
> +++ b/drivers/clk/clk.h
> @@ -22,17 +22,9 @@ static inline struct clk_hw *of_clk_get_hw(struct device_node *np,
>  struct clk_hw *clk_find_hw(const char *dev_id, const char *con_id);
>
>  #ifdef CONFIG_COMMON_CLK
> -struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
> -                             const char *dev_id, const char *con_id);
>  void __clk_put(struct clk *clk);
>  #else
>  /* All these casts to avoid ifdefs in clkdev... */
> -static inline struct clk *
> -clk_hw_create_clk(struct device *dev, struct clk_hw *hw, const char *dev_id,
> -                 const char *con_id)
> -{
> -       return (struct clk *)hw;
> -}
>  static struct clk_hw *__clk_get_hw(struct clk *clk)
>  {
>         return (struct clk_hw *)clk;
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index f689fc5..d3f60fe 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -18,6 +18,7 @@
>
>  struct device;
>  struct clk;
> +struct clk_hw;
>  struct device_node;
>  struct of_phandle_args;
>
> @@ -934,4 +935,15 @@ static inline struct clk *of_clk_get_from_provider(struct of_phandle_args *clksp
>  }
>  #endif
>
> +#ifdef CONFIG_COMMON_CLK
> +struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
> +                             const char *dev_id, const char *con_id);
> +#else
> +static inline struct clk *
> +clk_hw_create_clk(struct device *dev, struct clk_hw *hw, const char *dev_id,
> +                 const char *con_id)
> +{
> +       return (struct clk *)hw;
> +}
> +#endif
>  #endif
> ---
>
> This change should help other drivers (outside 'drivers/clk/') as well for getting the clock created from clk_hw.
> Is this fine to do?

I think this is the wrong approach, see why further above.

Kind regards
Uffe

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel