[PATCH v2 0/2] phy: intel-lgm-sdxc: Add support for SDXC PHY

[PATCH v2 0/2] phy: intel-lgm-sdxc: Add support for SDXC PHY

[Ramuthevar,Vadivel MuruganX]

Add support for SDXC PHY on Intel's Lightning Mountain SoC.

Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
---
Ramuthevar Vadivel Murugan (2):
  dt-bindings: phy: intel-sdxc-phy: Add YAML schema for LGM SDXC PHY
  changes in v2:
     - As per Rob's review comments added syscon properties.

  phy: intel-lgm-sdxc: Add support for SDXC PHY
   changes in v2: 
    - No change

 .../bindings/phy/intel,lgm-sdxc-phy.yaml           |  50 +++++++
 drivers/phy/intel/Kconfig                          |   6 +
 drivers/phy/intel/Makefile                         |   1 +
 drivers/phy/intel/phy-intel-sdxc.c                 | 144 +++++++++++++++++++++
 4 files changed, 201 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml
 create mode 100644 drivers/phy/intel/phy-intel-sdxc.c

-- 
2.11.0

[PATCH v2 1/2] dt-bindings: phy: intel-sdxc-phy: Add YAML schema for LGM SDXC PHY

[Ramuthevar,Vadivel MuruganX]

From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>

Add a YAML schema to use the host controller driver with the
SDXC PHY on Intel's Lightning Mountain SoC.

Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
---
 .../bindings/phy/intel,lgm-sdxc-phy.yaml           | 52 ++++++++++++++++++++++
 .../devicetree/bindings/phy/intel,syscon.yaml      | 33 ++++++++++++++
 2 files changed, 85 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml
 create mode 100644 Documentation/devicetree/bindings/phy/intel,syscon.yaml

diff --git a/Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml b/Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml
new file mode 100644
index 000000000000..99647207b414
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml
@@ -0,0 +1,52 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/intel,lgm-sdxc-phy.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Intel Lightning Mountain(LGM) SDXC PHY Device Tree Bindings
+
+maintainers:
+  - Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
+
+allOf:
+  - $ref: "intel,syscon.yaml"
+
+description: Binding for SDXC PHY
+
+properties:
+  compatible:
+    const: intel,lgm-sdxc-phy
+
+  intel,syscon:
+    description: phandle to the sdxc through syscon
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    maxItems: 1
+
+  "#phy-cells":
+    const: 0
+
+required:
+  - "#phy-cells"
+  - compatible
+  - intel,syscon
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+    sdxc_phy: sdxc_phy {
+        compatible = "intel,lgm-sdxc-phy";
+        intel,syscon = <&sysconf>;
+        clocks = <&sdxc>;
+        clock-names = "sdxcclk";
+        #phy-cells = <0>;
+    };
+
+...
diff --git a/Documentation/devicetree/bindings/phy/intel,syscon.yaml b/Documentation/devicetree/bindings/phy/intel,syscon.yaml
new file mode 100644
index 000000000000..d0b78805e49f
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/intel,syscon.yaml
@@ -0,0 +1,33 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/intel,syscon.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Syscon for eMMC/SDXC PHY Device Tree Bindings
+
+maintainers:
+  - Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
+
+properties:
+  compatible:
+    const: intel,syscon
+
+  reg:
+    maxItems: 1
+
+  "#reset-cells":
+   const: 1
+
+required:
+  - compatible
+  - reg
+  - "#reset-cells"
+
+examples:
+  - |
+    sysconf: chiptop@e0020000 {
+       compatible = "intel,syscon", "syscon";
+       reg = <0xe0020000 0x100>;
+       #reset-cells = <1>;
+    };
-- 
2.11.0

[PATCH v2 2/2] phy: intel-lgm-sdxc: Add support for SDXC PHY

[Ramuthevar,Vadivel MuruganX]

From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>

Add support for SDXC PHY on Intel's Lightning Mountain SoC.

Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
---
 drivers/phy/intel/Kconfig          |   6 ++
 drivers/phy/intel/Makefile         |   1 +
 drivers/phy/intel/phy-intel-sdxc.c | 144 +++++++++++++++++++++++++++++++++++++
 3 files changed, 151 insertions(+)
 create mode 100644 drivers/phy/intel/phy-intel-sdxc.c

diff --git a/drivers/phy/intel/Kconfig b/drivers/phy/intel/Kconfig
index 4ea6a8897cd7..d6356c762a6b 100644
--- a/drivers/phy/intel/Kconfig
+++ b/drivers/phy/intel/Kconfig
@@ -7,3 +7,9 @@ config PHY_INTEL_EMMC
 	select GENERIC_PHY
 	help
 	  Enable this to support the Intel EMMC PHY
+
+config PHY_INTEL_SDXC
+       tristate "Intel SDXC PHY driver"
+       select GENERIC_PHY
+       help
+         Enable this to support the Intel SDXC PHY driver
diff --git a/drivers/phy/intel/Makefile b/drivers/phy/intel/Makefile
index 6b876a75599d..3c6e7523200c 100644
--- a/drivers/phy/intel/Makefile
+++ b/drivers/phy/intel/Makefile
@@ -1,2 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_PHY_INTEL_EMMC)            += phy-intel-emmc.o
+obj-$(CONFIG_PHY_INTEL_SDXC)            += phy-intel-sdxc.o
diff --git a/drivers/phy/intel/phy-intel-sdxc.c b/drivers/phy/intel/phy-intel-sdxc.c
new file mode 100644
index 000000000000..7e13fd9ced5b
--- /dev/null
+++ b/drivers/phy/intel/phy-intel-sdxc.c
@@ -0,0 +1,144 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel SDXC PHY driver
+ * Copyright (C) 2019 Intel, Corp.
+ */
+
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+/* SDXC PHY register definitions */
+#define SDXC_PHYCTRL_REG	0x88
+#define OTAPDLYENA_MASK		BIT(14)
+#define OTAPDLYSEL(x)		((x) << 10)
+#define OTAPDLYSEL_ALL		OTAPDLYSEL(GENMASK(3, 0))
+
+struct intel_sdxc_phy {
+	struct regmap *syscfg;
+	struct clk *sdxcclk;
+};
+
+static int intel_sdxc_phy_init(struct phy *phy)
+{
+	struct intel_sdxc_phy *priv = phy_get_drvdata(phy);
+
+	/*
+	 * We purposely get the clock here and not in probe to avoid the
+	 * circular dependency problem.  We expect:
+	 * - PHY driver to probe
+	 * - SDHCI driver to start probe
+	 * - SDHCI driver to register it's clock
+	 * - SDHCI driver to get the PHY
+	 * - SDHCI driver to init the PHY
+	 *
+	 * The clock is optional, so upon any error just return it like
+	 * any other error to user.
+	 */
+	priv->sdxcclk = clk_get_optional(&phy->dev, "sdxcclk");
+	if (IS_ERR(priv->sdxcclk)) {
+		dev_err(&phy->dev, "Error getting sdxcclk\n");
+		return PTR_ERR(priv->sdxcclk);
+	}
+
+	return 0;
+}
+
+static int intel_sdxc_phy_exit(struct phy *phy)
+{
+	struct intel_sdxc_phy *priv = phy_get_drvdata(phy);
+
+	clk_put(priv->sdxcclk);
+
+	return 0;
+}
+
+static int intel_sdxc_phy_power_on(struct phy *phy)
+{
+	struct intel_sdxc_phy *priv = phy_get_drvdata(phy);
+
+	/* Output tap delay: disable */
+	regmap_update_bits(priv->syscfg, SDXC_PHYCTRL_REG, OTAPDLYENA_MASK, 0);
+
+	/* Output tap delay */
+	regmap_update_bits(priv->syscfg, SDXC_PHYCTRL_REG, OTAPDLYSEL_ALL,
+			   OTAPDLYSEL_ALL);
+
+	return 0;
+}
+
+static int intel_sdxc_phy_power_off(struct phy *phy)
+{
+	/* Do nothing */
+	return 0;
+}
+
+static const struct phy_ops ops = {
+	.init		= intel_sdxc_phy_init,
+	.exit		= intel_sdxc_phy_exit,
+	.power_on	= intel_sdxc_phy_power_on,
+	.power_off	= intel_sdxc_phy_power_off,
+	.owner		= THIS_MODULE,
+};
+
+static int intel_sdxc_phy_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct intel_sdxc_phy *priv;
+	struct phy *generic_phy;
+	struct phy_provider *phy_provider;
+
+	if (!dev->parent || !dev->parent->of_node)
+		return -ENODEV;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	/* Get SDXC phy (accessed via chiptop) regmap */
+	priv->syscfg = syscon_regmap_lookup_by_phandle(dev->of_node,
+						       "intel,syscon");
+	if (IS_ERR(priv->syscfg)) {
+		dev_err(dev, "No syscon phandle for chiptop\n");
+		return PTR_ERR(priv->syscfg);
+	}
+
+	generic_phy = devm_phy_create(dev, dev->of_node, &ops);
+	if (IS_ERR(generic_phy)) {
+		dev_err(dev, "failed to create PHY\n");
+		return PTR_ERR(generic_phy);
+	}
+
+	phy_set_drvdata(generic_phy, priv);
+	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+
+	return PTR_ERR_OR_ZERO(phy_provider);
+}
+
+static const struct of_device_id intel_sdxc_phy_dt_ids[] = {
+	{ .compatible = "intel,lgm-sdxc-phy" },
+	{}
+};
+
+MODULE_DEVICE_TABLE(of, intel_sdxc_phy_dt_ids);
+
+static struct platform_driver intel_sdxc_driver = {
+	.probe		= intel_sdxc_phy_probe,
+	.driver		= {
+		.name	= "intel-sdxc-phy",
+		.of_match_table = intel_sdxc_phy_dt_ids,
+	},
+};
+
+module_platform_driver(intel_sdxc_driver);
+
+MODULE_AUTHOR("Peter Harliman Liem <peter.harliman.liem@intel.com>");
+MODULE_DESCRIPTION("Intel SDXC PHY driver");
+MODULE_LICENSE("GPL v2");
-- 
2.11.0

RE: [PATCH v2 1/2] dt-bindings: phy: intel-sdxc-phy: Add YAML schema for LGM SDXC PHY

[Langer, Thomas]

Hi Vadivel,

> +...
> diff --git a/Documentation/devicetree/bindings/phy/intel,syscon.yaml
> b/Documentation/devicetree/bindings/phy/intel,syscon.yaml
> new file mode 100644
> index 000000000000..d0b78805e49f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/intel,syscon.yaml
> @@ -0,0 +1,33 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/intel,syscon.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Syscon for eMMC/SDXC PHY Device Tree Bindings

This says the binding is for eMMC/SDXC

> +
> +maintainers:
> +  - Ramuthevar Vadivel Murugan
> <vadivel.muruganx.ramuthevar@linux.intel.com>
> +
> +properties:
> +  compatible:
> +    const: intel,syscon

but this is a generic syscon, behind which are many registers, not only for
eMMC/SDXC. Also, the registers will be different for each SoC and needed for
many different drivers, that is why in your example it is called "chiptop"
-> toplevel registers not belonging to a specific HW module.

Rob: Do you also think this "intel,syscon" is too generic?
     And the binding should be outside the "phy" folder?

What is the way to support different SoCs with this? 
Must the driver referencing this syscon be aware of these differences?

> +
> +  reg:
> +    maxItems: 1
> +
> +  "#reset-cells":
> +   const: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#reset-cells"
> +
> +examples:
> +  - |
> +    sysconf: chiptop@e0020000 {
> +       compatible = "intel,syscon", "syscon";
> +       reg = <0xe0020000 0x100>;
> +       #reset-cells = <1>;
> +    };
> --
> 2.11.0

Best regards,
Thomas

Re: [PATCH v2 1/2] dt-bindings: phy: intel-sdxc-phy: Add YAML schema for LGM SDXC PHY

[Ramuthevar, Vadivel MuruganX]

Hi Langer,

Thank you for the review comments.

On 28/8/2019 11:37 PM, Langer, Thomas wrote:
> Hi Vadivel,
>
>> +...
>> diff --git a/Documentation/devicetree/bindings/phy/intel,syscon.yaml
>> b/Documentation/devicetree/bindings/phy/intel,syscon.yaml
>> new file mode 100644
>> index 000000000000..d0b78805e49f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/intel,syscon.yaml
>> @@ -0,0 +1,33 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/phy/intel,syscon.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Syscon for eMMC/SDXC PHY Device Tree Bindings
> This says the binding is for eMMC/SDXC
Agreed, fix it.
>> +
>> +maintainers:
>> +  - Ramuthevar Vadivel Murugan
>> <vadivel.muruganx.ramuthevar@linux.intel.com>
>> +
>> +properties:
>> +  compatible:
>> +    const: intel,syscon
> but this is a generic syscon, behind which are many registers, not only for
> eMMC/SDXC. Also, the registers will be different for each SoC and needed for
> many different drivers, that is why in your example it is called "chiptop"
> -> toplevel registers not belonging to a specific HW module.
>
> Rob: Do you also think this "intel,syscon" is too generic?
>       And the binding should be outside the "phy" folder?
>
> What is the way to support different SoCs with this?
> Must the driver referencing this syscon be aware of these differences?

[Vadivel] : most of the IP drivers are using syscon, please suggest me 
to keep in the right place since

it is common to all(w.r.t Intel's Lightning Mountain).

Best Regards
Vadivel
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  "#reset-cells":
>> +   const: 1
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - "#reset-cells"
>> +
>> +examples:
>> +  - |
>> +    sysconf: chiptop@e0020000 {
>> +       compatible = "intel,syscon", "syscon";
>> +       reg = <0xe0020000 0x100>;
>> +       #reset-cells = <1>;
>> +    };
>> --
>> 2.11.0
> Best regards,
> Thomas

Re: [PATCH v2 1/2] dt-bindings: phy: intel-sdxc-phy: Add YAML schema for LGM SDXC PHY

[Rob Herring]

On Wed, Aug 28, 2019 at 08:43:14PM +0800, Ramuthevar,Vadivel MuruganX wrote:
> From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
> 
> Add a YAML schema to use the host controller driver with the
> SDXC PHY on Intel's Lightning Mountain SoC.
> 
> Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
> ---
>  .../bindings/phy/intel,lgm-sdxc-phy.yaml           | 52 ++++++++++++++++++++++
>  .../devicetree/bindings/phy/intel,syscon.yaml      | 33 ++++++++++++++
>  2 files changed, 85 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml
>  create mode 100644 Documentation/devicetree/bindings/phy/intel,syscon.yaml
> 
> diff --git a/Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml b/Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml
> new file mode 100644
> index 000000000000..99647207b414
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml
> @@ -0,0 +1,52 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/intel,lgm-sdxc-phy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Intel Lightning Mountain(LGM) SDXC PHY Device Tree Bindings
> +
> +maintainers:
> +  - Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
> +
> +allOf:
> +  - $ref: "intel,syscon.yaml"

You don't need this. It should be selected and applied by the compatible 
string matching.

> +
> +description: Binding for SDXC PHY
> +
> +properties:
> +  compatible:
> +    const: intel,lgm-sdxc-phy
> +
> +  intel,syscon:
> +    description: phandle to the sdxc through syscon
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    maxItems: 1
> +
> +  "#phy-cells":
> +    const: 0
> +
> +required:
> +  - "#phy-cells"
> +  - compatible
> +  - intel,syscon
> +  - clocks
> +  - clock-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    sdxc_phy: sdxc_phy {
> +        compatible = "intel,lgm-sdxc-phy";
> +        intel,syscon = <&sysconf>;

Make this a child of the below node and then you don't need this.

If there's a register address range associated with this, then add a reg 
property.

> +        clocks = <&sdxc>;
> +        clock-names = "sdxcclk";
> +        #phy-cells = <0>;
> +    };
> +
> +...
> diff --git a/Documentation/devicetree/bindings/phy/intel,syscon.yaml b/Documentation/devicetree/bindings/phy/intel,syscon.yaml
> new file mode 100644
> index 000000000000..d0b78805e49f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/intel,syscon.yaml
> @@ -0,0 +1,33 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/intel,syscon.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Syscon for eMMC/SDXC PHY Device Tree Bindings

Nothing else in this h/w block? If there are other functions, then this 
should not be in bindings/phy/.

> +
> +maintainers:
> +  - Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
> +
> +properties:
> +  compatible:
> +    const: intel,syscon

Needs to be more specific and reflect h/w blocks. 'syscon' is a Linux 
thing to some extent.

> +
> +  reg:
> +    maxItems: 1
> +
> +  "#reset-cells":
> +   const: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#reset-cells"
> +
> +examples:
> +  - |
> +    sysconf: chiptop@e0020000 {
> +       compatible = "intel,syscon", "syscon";
> +       reg = <0xe0020000 0x100>;
> +       #reset-cells = <1>;
> +    };
> -- 
> 2.11.0
> 

Re: [PATCH v2 1/2] dt-bindings: phy: intel-sdxc-phy: Add YAML schema for LGM SDXC PHY

[Ramuthevar, Vadivel MuruganX]

Hi Rob,

Thank you for review comments.

On 2/9/2019 9:38 PM, Rob Herring wrote:
> On Wed, Aug 28, 2019 at 08:43:14PM +0800, Ramuthevar,Vadivel MuruganX wrote:
>> From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
>>
>> Add a YAML schema to use the host controller driver with the
>> SDXC PHY on Intel's Lightning Mountain SoC.
>>
>> Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
>> ---
>>   .../bindings/phy/intel,lgm-sdxc-phy.yaml           | 52 ++++++++++++++++++++++
>>   .../devicetree/bindings/phy/intel,syscon.yaml      | 33 ++++++++++++++
>>   2 files changed, 85 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml
>>   create mode 100644 Documentation/devicetree/bindings/phy/intel,syscon.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml b/Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml
>> new file mode 100644
>> index 000000000000..99647207b414
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml
>> @@ -0,0 +1,52 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/phy/intel,lgm-sdxc-phy.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Intel Lightning Mountain(LGM) SDXC PHY Device Tree Bindings
>> +
>> +maintainers:
>> +  - Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
>> +
>> +allOf:
>> +  - $ref: "intel,syscon.yaml"
> You don't need this. It should be selected and applied by the compatible
> string matching.
Agreed, fix it in the next patch.
>> +
>> +description: Binding for SDXC PHY
>> +
>> +properties:
>> +  compatible:
>> +    const: intel,lgm-sdxc-phy
>> +
>> +  intel,syscon:
>> +    description: phandle to the sdxc through syscon
>> +
>> +  clocks:
>> +    maxItems: 1
>> +
>> +  clock-names:
>> +    maxItems: 1
>> +
>> +  "#phy-cells":
>> +    const: 0
>> +
>> +required:
>> +  - "#phy-cells"
>> +  - compatible
>> +  - intel,syscon
>> +  - clocks
>> +  - clock-names
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    sdxc_phy: sdxc_phy {
>> +        compatible = "intel,lgm-sdxc-phy";
>> +        intel,syscon = <&sysconf>;
> Make this a child of the below node and then you don't need this.
>
> If there's a register address range associated with this, then add a reg
> property.

Thanks for comments,  I have defined herewith example

sysconf: chiptop@e0020000 {
             compatible = "intel,syscon";
             reg = <0xe0020000 0x100>;

             emmc_phy: emmc_phy {
                 compatible = "intel,lgm-emmc-phy";
                 intel,syscon = <&sysconf>;
                 clocks = <&emmc>;
                 clock-names = "emmcclk";
                 #phy-cells = <0>;
            };
};

Is this way need to add right?

>> +        clocks = <&sdxc>;
>> +        clock-names = "sdxcclk";
>> +        #phy-cells = <0>;
>> +    };
>> +
>> +...
>> diff --git a/Documentation/devicetree/bindings/phy/intel,syscon.yaml b/Documentation/devicetree/bindings/phy/intel,syscon.yaml
>> new file mode 100644
>> index 000000000000..d0b78805e49f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/intel,syscon.yaml
>> @@ -0,0 +1,33 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/phy/intel,syscon.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Syscon for eMMC/SDXC PHY Device Tree Bindings
> Nothing else in this h/w block? If there are other functions, then this
> should not be in bindings/phy/.
Drop this one.
>> +
>> +maintainers:
>> +  - Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
>> +
>> +properties:
>> +  compatible:
>> +    const: intel,syscon
> Needs to be more specific and reflect h/w blocks. 'syscon' is a Linux
> thing to some extent.
Agreed, will drop it.

Best Regards
vadivel
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  "#reset-cells":
>> +   const: 1
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - "#reset-cells"
>> +
>> +examples:
>> +  - |
>> +    sysconf: chiptop@e0020000 {
>> +       compatible = "intel,syscon", "syscon";
>> +       reg = <0xe0020000 0x100>;
>> +       #reset-cells = <1>;
>> +    };
>> -- 
>> 2.11.0
>>

Re: [PATCH v2 1/2] dt-bindings: phy: intel-sdxc-phy: Add YAML schema for LGM SDXC PHY

[Rob Herring]

On Tue, Sep 3, 2019 at 2:57 AM Ramuthevar, Vadivel MuruganX
<vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
>
> Hi Rob,
>
> Thank you for review comments.
>
> On 2/9/2019 9:38 PM, Rob Herring wrote:
> > On Wed, Aug 28, 2019 at 08:43:14PM +0800, Ramuthevar,Vadivel MuruganX wrote:
> >> From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
> >>
> >> Add a YAML schema to use the host controller driver with the
> >> SDXC PHY on Intel's Lightning Mountain SoC.
> >>
> >> Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
> >> ---
> >>   .../bindings/phy/intel,lgm-sdxc-phy.yaml           | 52 ++++++++++++++++++++++
> >>   .../devicetree/bindings/phy/intel,syscon.yaml      | 33 ++++++++++++++
> >>   2 files changed, 85 insertions(+)
> >>   create mode 100644 Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml
> >>   create mode 100644 Documentation/devicetree/bindings/phy/intel,syscon.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml b/Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml
> >> new file mode 100644
> >> index 000000000000..99647207b414
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml
> >> @@ -0,0 +1,52 @@
> >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/phy/intel,lgm-sdxc-phy.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: Intel Lightning Mountain(LGM) SDXC PHY Device Tree Bindings
> >> +
> >> +maintainers:
> >> +  - Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
> >> +
> >> +allOf:
> >> +  - $ref: "intel,syscon.yaml"
> > You don't need this. It should be selected and applied by the compatible
> > string matching.
> Agreed, fix it in the next patch.
> >> +
> >> +description: Binding for SDXC PHY
> >> +
> >> +properties:
> >> +  compatible:
> >> +    const: intel,lgm-sdxc-phy
> >> +
> >> +  intel,syscon:
> >> +    description: phandle to the sdxc through syscon
> >> +
> >> +  clocks:
> >> +    maxItems: 1
> >> +
> >> +  clock-names:
> >> +    maxItems: 1
> >> +
> >> +  "#phy-cells":
> >> +    const: 0
> >> +
> >> +required:
> >> +  - "#phy-cells"
> >> +  - compatible
> >> +  - intel,syscon
> >> +  - clocks
> >> +  - clock-names
> >> +
> >> +additionalProperties: false
> >> +
> >> +examples:
> >> +  - |
> >> +    sdxc_phy: sdxc_phy {
> >> +        compatible = "intel,lgm-sdxc-phy";
> >> +        intel,syscon = <&sysconf>;
> > Make this a child of the below node and then you don't need this.
> >
> > If there's a register address range associated with this, then add a reg
> > property.
>
> Thanks for comments,  I have defined herewith example
>
> sysconf: chiptop@e0020000 {
>              compatible = "intel,syscon";

Needs to be SoC specific value.

>              reg = <0xe0020000 0x100>;
>
>              emmc_phy: emmc_phy {
>                  compatible = "intel,lgm-emmc-phy";
>                  intel,syscon = <&sysconf>;

This is redundant because you can just get the parent node.

If there's a defined register range within the 'intel,syscon' block
then define it here with 'reg'.

>                  clocks = <&emmc>;
>                  clock-names = "emmcclk";
>                  #phy-cells = <0>;
>             };
> };
>
> Is this way need to add right?
>
> >> +        clocks = <&sdxc>;
> >> +        clock-names = "sdxcclk";
> >> +        #phy-cells = <0>;
> >> +    };
> >> +
> >> +...

Re: [PATCH v2 1/2] dt-bindings: phy: intel-sdxc-phy: Add YAML schema for LGM SDXC PHY

[Ramuthevar, Vadivel MuruganX]

Hi Rob,

  Thank you so much for prompt reply.

On 3/9/2019 5:19 PM, Rob Herring wrote:
> On Tue, Sep 3, 2019 at 2:57 AM Ramuthevar, Vadivel MuruganX
> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
>> Hi Rob,
>>
>> Thank you for review comments.
>>
>> On 2/9/2019 9:38 PM, Rob Herring wrote:
>>> On Wed, Aug 28, 2019 at 08:43:14PM +0800, Ramuthevar,Vadivel MuruganX wrote:
>>>> From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
>>>>
>>>> Add a YAML schema to use the host controller driver with the
>>>> SDXC PHY on Intel's Lightning Mountain SoC.
>>>>
>>>> Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
>>>> ---
>>>>    .../bindings/phy/intel,lgm-sdxc-phy.yaml           | 52 ++++++++++++++++++++++
>>>>    .../devicetree/bindings/phy/intel,syscon.yaml      | 33 ++++++++++++++
>>>>    2 files changed, 85 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml
>>>>    create mode 100644 Documentation/devicetree/bindings/phy/intel,syscon.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml b/Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml
>>>> new file mode 100644
>>>> index 000000000000..99647207b414
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml
>>>> @@ -0,0 +1,52 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/phy/intel,lgm-sdxc-phy.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Intel Lightning Mountain(LGM) SDXC PHY Device Tree Bindings
>>>> +
>>>> +maintainers:
>>>> +  - Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
>>>> +
>>>> +allOf:
>>>> +  - $ref: "intel,syscon.yaml"
>>> You don't need this. It should be selected and applied by the compatible
>>> string matching.
>> Agreed, fix it in the next patch.
>>>> +
>>>> +description: Binding for SDXC PHY
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: intel,lgm-sdxc-phy
>>>> +
>>>> +  intel,syscon:
>>>> +    description: phandle to the sdxc through syscon
>>>> +
>>>> +  clocks:
>>>> +    maxItems: 1
>>>> +
>>>> +  clock-names:
>>>> +    maxItems: 1
>>>> +
>>>> +  "#phy-cells":
>>>> +    const: 0
>>>> +
>>>> +required:
>>>> +  - "#phy-cells"
>>>> +  - compatible
>>>> +  - intel,syscon
>>>> +  - clocks
>>>> +  - clock-names
>>>> +
>>>> +additionalProperties: false
>>>> +
>>>> +examples:
>>>> +  - |
>>>> +    sdxc_phy: sdxc_phy {
>>>> +        compatible = "intel,lgm-sdxc-phy";
>>>> +        intel,syscon = <&sysconf>;
>>> Make this a child of the below node and then you don't need this.
>>>
>>> If there's a register address range associated with this, then add a reg
>>> property.
>> Thanks for comments,  I have defined herewith example
>>
>> sysconf: chiptop@e0020000 {
>>               compatible = "intel,syscon";
> Needs to be SoC specific value.
Agreed! it should be "intel, lgm-syscon"
>>               reg = <0xe0020000 0x100>;
>>
>>               emmc_phy: emmc_phy {
>>                   compatible = "intel,lgm-emmc-phy";
>>                   intel,syscon = <&sysconf>;
> This is redundant because you can just get the parent node.
>
> If there's a defined register range within the 'intel,syscon' block
> then define it here with 'reg'.

Agreed!, avoided redundant

sysconf: chiptop@e0020000 {
             compatible = "intel,lgm-syscon";
             emmc_phy: emmc_phy {
                 compatible = "intel,lgm-emmc-phy";
                 reg = <0xe0020000 0x100>;
                 clocks = <&emmc>;
                 clock-names = "emmcclk";
                 #phy-cells = <0>;
            };
};

if this is correct, then will send updated patch-set.

Best Regards
Vadivel
>>                   clocks = <&emmc>;
>>                   clock-names = "emmcclk";
>>                   #phy-cells = <0>;
>>              };
>> };
>>
>> Is this way need to add right?
>>
>>>> +        clocks = <&sdxc>;
>>>> +        clock-names = "sdxcclk";
>>>> +        #phy-cells = <0>;
>>>> +    };
>>>> +
>>>> +...

Re: [PATCH v2 1/2] dt-bindings: phy: intel-sdxc-phy: Add YAML schema for LGM SDXC PHY

[Rob Herring]

On Tue, Sep 3, 2019 at 11:08 AM Ramuthevar, Vadivel MuruganX
<vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
>
> Hi Rob,
>
>   Thank you so much for prompt reply.
>
> On 3/9/2019 5:19 PM, Rob Herring wrote:
> > On Tue, Sep 3, 2019 at 2:57 AM Ramuthevar, Vadivel MuruganX
> > <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
> >> Hi Rob,
> >>
> >> Thank you for review comments.
> >>
> >> On 2/9/2019 9:38 PM, Rob Herring wrote:
> >>> On Wed, Aug 28, 2019 at 08:43:14PM +0800, Ramuthevar,Vadivel MuruganX wrote:
> >>>> From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
> >>>>
> >>>> Add a YAML schema to use the host controller driver with the
> >>>> SDXC PHY on Intel's Lightning Mountain SoC.
> >>>>
> >>>> Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
> >>>> ---
> >>>>    .../bindings/phy/intel,lgm-sdxc-phy.yaml           | 52 ++++++++++++++++++++++
> >>>>    .../devicetree/bindings/phy/intel,syscon.yaml      | 33 ++++++++++++++
> >>>>    2 files changed, 85 insertions(+)
> >>>>    create mode 100644 Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml
> >>>>    create mode 100644 Documentation/devicetree/bindings/phy/intel,syscon.yaml
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml b/Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml
> >>>> new file mode 100644
> >>>> index 000000000000..99647207b414
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml
> >>>> @@ -0,0 +1,52 @@
> >>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>>> +%YAML 1.2
> >>>> +---
> >>>> +$id: http://devicetree.org/schemas/phy/intel,lgm-sdxc-phy.yaml#
> >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>> +
> >>>> +title: Intel Lightning Mountain(LGM) SDXC PHY Device Tree Bindings
> >>>> +
> >>>> +maintainers:
> >>>> +  - Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
> >>>> +
> >>>> +allOf:
> >>>> +  - $ref: "intel,syscon.yaml"
> >>> You don't need this. It should be selected and applied by the compatible
> >>> string matching.
> >> Agreed, fix it in the next patch.
> >>>> +
> >>>> +description: Binding for SDXC PHY
> >>>> +
> >>>> +properties:
> >>>> +  compatible:
> >>>> +    const: intel,lgm-sdxc-phy
> >>>> +
> >>>> +  intel,syscon:
> >>>> +    description: phandle to the sdxc through syscon
> >>>> +
> >>>> +  clocks:
> >>>> +    maxItems: 1
> >>>> +
> >>>> +  clock-names:
> >>>> +    maxItems: 1
> >>>> +
> >>>> +  "#phy-cells":
> >>>> +    const: 0
> >>>> +
> >>>> +required:
> >>>> +  - "#phy-cells"
> >>>> +  - compatible
> >>>> +  - intel,syscon
> >>>> +  - clocks
> >>>> +  - clock-names
> >>>> +
> >>>> +additionalProperties: false
> >>>> +
> >>>> +examples:
> >>>> +  - |
> >>>> +    sdxc_phy: sdxc_phy {
> >>>> +        compatible = "intel,lgm-sdxc-phy";
> >>>> +        intel,syscon = <&sysconf>;
> >>> Make this a child of the below node and then you don't need this.
> >>>
> >>> If there's a register address range associated with this, then add a reg
> >>> property.
> >> Thanks for comments,  I have defined herewith example
> >>
> >> sysconf: chiptop@e0020000 {
> >>               compatible = "intel,syscon";
> > Needs to be SoC specific value.
> Agreed! it should be "intel, lgm-syscon"
> >>               reg = <0xe0020000 0x100>;
> >>
> >>               emmc_phy: emmc_phy {
> >>                   compatible = "intel,lgm-emmc-phy";
> >>                   intel,syscon = <&sysconf>;
> > This is redundant because you can just get the parent node.
> >
> > If there's a defined register range within the 'intel,syscon' block
> > then define it here with 'reg'.
>
> Agreed!, avoided redundant
>
> sysconf: chiptop@e0020000 {
>              compatible = "intel,lgm-syscon";
>              emmc_phy: emmc_phy {
>                  compatible = "intel,lgm-emmc-phy";
>                  reg = <0xe0020000 0x100>;

This is the same addresses you had for the parent, so that doesn't
seem right. The parent should have the entire range and then the child
nodes only the addresses for their functions. However, if the
registers are all interleaved then you can really put 'reg' in the
child nodes and just have it only in the parent. We don't want to have
overlapping addresses in DT.

Rob

Re: [PATCH v2 1/2] dt-bindings: phy: intel-sdxc-phy: Add YAML schema for LGM SDXC PHY

[Ramuthevar, Vadivel MuruganX]

Hi Rob,

    Thank you for your suggestions and clarifications.

On 3/9/2019 6:34 PM, Rob Herring wrote:
> On Tue, Sep 3, 2019 at 11:08 AM Ramuthevar, Vadivel MuruganX
> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
>> Hi Rob,
>>
>>    Thank you so much for prompt reply.
>>
>> On 3/9/2019 5:19 PM, Rob Herring wrote:
>>> On Tue, Sep 3, 2019 at 2:57 AM Ramuthevar, Vadivel MuruganX
>>> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
>>>> Hi Rob,
>>>>
>>>> Thank you for review comments.
>>>>
>>>> On 2/9/2019 9:38 PM, Rob Herring wrote:
>>>>> On Wed, Aug 28, 2019 at 08:43:14PM +0800, Ramuthevar,Vadivel MuruganX wrote:
>>>>>> From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
>>>>>>
>>>>>> Add a YAML schema to use the host controller driver with the
>>>>>> SDXC PHY on Intel's Lightning Mountain SoC.
>>>>>>
>>>>>> Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
>>>>>> ---
>>>>>>     .../bindings/phy/intel,lgm-sdxc-phy.yaml           | 52 ++++++++++++++++++++++
>>>>>>     .../devicetree/bindings/phy/intel,syscon.yaml      | 33 ++++++++++++++
>>>>>>     2 files changed, 85 insertions(+)
>>>>>>     create mode 100644 Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml
>>>>>>     create mode 100644 Documentation/devicetree/bindings/phy/intel,syscon.yaml
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml b/Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml
>>>>>> new file mode 100644
>>>>>> index 000000000000..99647207b414
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml
>>>>>> @@ -0,0 +1,52 @@
>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>> +%YAML 1.2
>>>>>> +---
>>>>>> +$id: http://devicetree.org/schemas/phy/intel,lgm-sdxc-phy.yaml#
>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>> +
>>>>>> +title: Intel Lightning Mountain(LGM) SDXC PHY Device Tree Bindings
>>>>>> +
>>>>>> +maintainers:
>>>>>> +  - Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
>>>>>> +
>>>>>> +allOf:
>>>>>> +  - $ref: "intel,syscon.yaml"
>>>>> You don't need this. It should be selected and applied by the compatible
>>>>> string matching.
>>>> Agreed, fix it in the next patch.
>>>>>> +
>>>>>> +description: Binding for SDXC PHY
>>>>>> +
>>>>>> +properties:
>>>>>> +  compatible:
>>>>>> +    const: intel,lgm-sdxc-phy
>>>>>> +
>>>>>> +  intel,syscon:
>>>>>> +    description: phandle to the sdxc through syscon
>>>>>> +
>>>>>> +  clocks:
>>>>>> +    maxItems: 1
>>>>>> +
>>>>>> +  clock-names:
>>>>>> +    maxItems: 1
>>>>>> +
>>>>>> +  "#phy-cells":
>>>>>> +    const: 0
>>>>>> +
>>>>>> +required:
>>>>>> +  - "#phy-cells"
>>>>>> +  - compatible
>>>>>> +  - intel,syscon
>>>>>> +  - clocks
>>>>>> +  - clock-names
>>>>>> +
>>>>>> +additionalProperties: false
>>>>>> +
>>>>>> +examples:
>>>>>> +  - |
>>>>>> +    sdxc_phy: sdxc_phy {
>>>>>> +        compatible = "intel,lgm-sdxc-phy";
>>>>>> +        intel,syscon = <&sysconf>;
>>>>> Make this a child of the below node and then you don't need this.
>>>>>
>>>>> If there's a register address range associated with this, then add a reg
>>>>> property.
>>>> Thanks for comments,  I have defined herewith example
>>>>
>>>> sysconf: chiptop@e0020000 {
>>>>                compatible = "intel,syscon";
>>> Needs to be SoC specific value.
>> Agreed! it should be "intel, lgm-syscon"
>>>>                reg = <0xe0020000 0x100>;
>>>>
>>>>                emmc_phy: emmc_phy {
>>>>                    compatible = "intel,lgm-emmc-phy";
>>>>                    intel,syscon = <&sysconf>;
>>> This is redundant because you can just get the parent node.
>>>
>>> If there's a defined register range within the 'intel,syscon' block
>>> then define it here with 'reg'.
>> Agreed!, avoided redundant
>>
>> sysconf: chiptop@e0020000 {
>>               compatible = "intel,lgm-syscon";
>>               emmc_phy: emmc_phy {
>>                   compatible = "intel,lgm-emmc-phy";
>>                   reg = <0xe0020000 0x100>;
> This is the same addresses you had for the parent, so that doesn't
> seem right. The parent should have the entire range and then the child
> nodes only the addresses for their functions. However, if the
> registers are all interleaved then you can really put 'reg' in the
> child nodes and just have it only in the parent. We don't want to have
> overlapping addresses in DT.
syscon is parent node, which has the base address for all the peripheral 
registers and used by child nodes.
child nodes have only offsets, we do not specify in device tree.

Best Regards
vadivel
> Rob

Re: [PATCH v2 1/2] dt-bindings: phy: intel-sdxc-phy: Add YAML schema for LGM SDXC PHY

[Rob Herring]

On Tue, Sep 3, 2019 at 11:52 AM Ramuthevar, Vadivel MuruganX
<vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
>
> Hi Rob,
>
>     Thank you for your suggestions and clarifications.
>
> On 3/9/2019 6:34 PM, Rob Herring wrote:
> > On Tue, Sep 3, 2019 at 11:08 AM Ramuthevar, Vadivel MuruganX
> > <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
> >> Hi Rob,
> >>
> >>    Thank you so much for prompt reply.
> >>
> >> On 3/9/2019 5:19 PM, Rob Herring wrote:
> >>> On Tue, Sep 3, 2019 at 2:57 AM Ramuthevar, Vadivel MuruganX
> >>> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
> >>>> Hi Rob,
> >>>>
> >>>> Thank you for review comments.
> >>>>
> >>>> On 2/9/2019 9:38 PM, Rob Herring wrote:
> >>>>> On Wed, Aug 28, 2019 at 08:43:14PM +0800, Ramuthevar,Vadivel MuruganX wrote:
> >>>>>> From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
> >>>>>>
> >>>>>> Add a YAML schema to use the host controller driver with the
> >>>>>> SDXC PHY on Intel's Lightning Mountain SoC.
> >>>>>>
> >>>>>> Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
> >>>>>> ---
> >>>>>>     .../bindings/phy/intel,lgm-sdxc-phy.yaml           | 52 ++++++++++++++++++++++
> >>>>>>     .../devicetree/bindings/phy/intel,syscon.yaml      | 33 ++++++++++++++
> >>>>>>     2 files changed, 85 insertions(+)
> >>>>>>     create mode 100644 Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml
> >>>>>>     create mode 100644 Documentation/devicetree/bindings/phy/intel,syscon.yaml
> >>>>>>
> >>>>>> diff --git a/Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml b/Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml
> >>>>>> new file mode 100644
> >>>>>> index 000000000000..99647207b414
> >>>>>> --- /dev/null
> >>>>>> +++ b/Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml
> >>>>>> @@ -0,0 +1,52 @@
> >>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>>>>> +%YAML 1.2
> >>>>>> +---
> >>>>>> +$id: http://devicetree.org/schemas/phy/intel,lgm-sdxc-phy.yaml#
> >>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>>>> +
> >>>>>> +title: Intel Lightning Mountain(LGM) SDXC PHY Device Tree Bindings
> >>>>>> +
> >>>>>> +maintainers:
> >>>>>> +  - Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
> >>>>>> +
> >>>>>> +allOf:
> >>>>>> +  - $ref: "intel,syscon.yaml"
> >>>>> You don't need this. It should be selected and applied by the compatible
> >>>>> string matching.
> >>>> Agreed, fix it in the next patch.
> >>>>>> +
> >>>>>> +description: Binding for SDXC PHY
> >>>>>> +
> >>>>>> +properties:
> >>>>>> +  compatible:
> >>>>>> +    const: intel,lgm-sdxc-phy
> >>>>>> +
> >>>>>> +  intel,syscon:
> >>>>>> +    description: phandle to the sdxc through syscon
> >>>>>> +
> >>>>>> +  clocks:
> >>>>>> +    maxItems: 1
> >>>>>> +
> >>>>>> +  clock-names:
> >>>>>> +    maxItems: 1
> >>>>>> +
> >>>>>> +  "#phy-cells":
> >>>>>> +    const: 0
> >>>>>> +
> >>>>>> +required:
> >>>>>> +  - "#phy-cells"
> >>>>>> +  - compatible
> >>>>>> +  - intel,syscon
> >>>>>> +  - clocks
> >>>>>> +  - clock-names
> >>>>>> +
> >>>>>> +additionalProperties: false
> >>>>>> +
> >>>>>> +examples:
> >>>>>> +  - |
> >>>>>> +    sdxc_phy: sdxc_phy {
> >>>>>> +        compatible = "intel,lgm-sdxc-phy";
> >>>>>> +        intel,syscon = <&sysconf>;
> >>>>> Make this a child of the below node and then you don't need this.
> >>>>>
> >>>>> If there's a register address range associated with this, then add a reg
> >>>>> property.
> >>>> Thanks for comments,  I have defined herewith example
> >>>>
> >>>> sysconf: chiptop@e0020000 {
> >>>>                compatible = "intel,syscon";
> >>> Needs to be SoC specific value.
> >> Agreed! it should be "intel, lgm-syscon"
> >>>>                reg = <0xe0020000 0x100>;
> >>>>
> >>>>                emmc_phy: emmc_phy {
> >>>>                    compatible = "intel,lgm-emmc-phy";
> >>>>                    intel,syscon = <&sysconf>;
> >>> This is redundant because you can just get the parent node.
> >>>
> >>> If there's a defined register range within the 'intel,syscon' block
> >>> then define it here with 'reg'.
> >> Agreed!, avoided redundant
> >>
> >> sysconf: chiptop@e0020000 {
> >>               compatible = "intel,lgm-syscon";
> >>               emmc_phy: emmc_phy {
> >>                   compatible = "intel,lgm-emmc-phy";
> >>                   reg = <0xe0020000 0x100>;
> > This is the same addresses you had for the parent, so that doesn't
> > seem right. The parent should have the entire range and then the child
> > nodes only the addresses for their functions. However, if the
> > registers are all interleaved then you can really put 'reg' in the
> > child nodes and just have it only in the parent. We don't want to have
> > overlapping addresses in DT.
> syscon is parent node, which has the base address for all the peripheral
> registers and used by child nodes.
> child nodes have only offsets, we do not specify in device tree.

Right, and I'm asking you to add the offsets whether Linux uses them or not.

Rob

Re: [PATCH v2 1/2] dt-bindings: phy: intel-sdxc-phy: Add YAML schema for LGM SDXC PHY

[Ramuthevar, Vadivel MuruganX]

Hi Rob,

   Thank you so much for the conclusion.

On 4/9/2019 5:34 AM, Rob Herring wrote:
> On Tue, Sep 3, 2019 at 11:52 AM Ramuthevar, Vadivel MuruganX
> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
>> Hi Rob,
>>
>>      Thank you for your suggestions and clarifications.
>>
>> On 3/9/2019 6:34 PM, Rob Herring wrote:
>>> On Tue, Sep 3, 2019 at 11:08 AM Ramuthevar, Vadivel MuruganX
>>> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
>>>> Hi Rob,
>>>>
>>>>     Thank you so much for prompt reply.
>>>>
>>>> On 3/9/2019 5:19 PM, Rob Herring wrote:
>>>>> On Tue, Sep 3, 2019 at 2:57 AM Ramuthevar, Vadivel MuruganX
>>>>> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
>>>>>> Hi Rob,
>>>>>>
>>>>>> Thank you for review comments.
>>>>>>
>>>>>> On 2/9/2019 9:38 PM, Rob Herring wrote:
>>>>>>> On Wed, Aug 28, 2019 at 08:43:14PM +0800, Ramuthevar,Vadivel MuruganX wrote:
>>>>>>>> From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
>>>>>>>>
>>>>>>>> Add a YAML schema to use the host controller driver with the
>>>>>>>> SDXC PHY on Intel's Lightning Mountain SoC.
>>>>>>>>
>>>>>>>> Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
>>>>>>>> ---
>>>>>>>>      .../bindings/phy/intel,lgm-sdxc-phy.yaml           | 52 ++++++++++++++++++++++
>>>>>>>>      .../devicetree/bindings/phy/intel,syscon.yaml      | 33 ++++++++++++++
>>>>>>>>      2 files changed, 85 insertions(+)
>>>>>>>>      create mode 100644 Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml
>>>>>>>>      create mode 100644 Documentation/devicetree/bindings/phy/intel,syscon.yaml
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml b/Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml
>>>>>>>> new file mode 100644
>>>>>>>> index 000000000000..99647207b414
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/Documentation/devicetree/bindings/phy/intel,lgm-sdxc-phy.yaml
>>>>>>>> @@ -0,0 +1,52 @@
>>>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>>>> +%YAML 1.2
>>>>>>>> +---
>>>>>>>> +$id: http://devicetree.org/schemas/phy/intel,lgm-sdxc-phy.yaml#
>>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>>>> +
>>>>>>>> +title: Intel Lightning Mountain(LGM) SDXC PHY Device Tree Bindings
>>>>>>>> +
>>>>>>>> +maintainers:
>>>>>>>> +  - Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
>>>>>>>> +
>>>>>>>> +allOf:
>>>>>>>> +  - $ref: "intel,syscon.yaml"
>>>>>>> You don't need this. It should be selected and applied by the compatible
>>>>>>> string matching.
>>>>>> Agreed, fix it in the next patch.
>>>>>>>> +
>>>>>>>> +description: Binding for SDXC PHY
>>>>>>>> +
>>>>>>>> +properties:
>>>>>>>> +  compatible:
>>>>>>>> +    const: intel,lgm-sdxc-phy
>>>>>>>> +
>>>>>>>> +  intel,syscon:
>>>>>>>> +    description: phandle to the sdxc through syscon
>>>>>>>> +
>>>>>>>> +  clocks:
>>>>>>>> +    maxItems: 1
>>>>>>>> +
>>>>>>>> +  clock-names:
>>>>>>>> +    maxItems: 1
>>>>>>>> +
>>>>>>>> +  "#phy-cells":
>>>>>>>> +    const: 0
>>>>>>>> +
>>>>>>>> +required:
>>>>>>>> +  - "#phy-cells"
>>>>>>>> +  - compatible
>>>>>>>> +  - intel,syscon
>>>>>>>> +  - clocks
>>>>>>>> +  - clock-names
>>>>>>>> +
>>>>>>>> +additionalProperties: false
>>>>>>>> +
>>>>>>>> +examples:
>>>>>>>> +  - |
>>>>>>>> +    sdxc_phy: sdxc_phy {
>>>>>>>> +        compatible = "intel,lgm-sdxc-phy";
>>>>>>>> +        intel,syscon = <&sysconf>;
>>>>>>> Make this a child of the below node and then you don't need this.
>>>>>>>
>>>>>>> If there's a register address range associated with this, then add a reg
>>>>>>> property.
>>>>>> Thanks for comments,  I have defined herewith example
>>>>>>
>>>>>> sysconf: chiptop@e0020000 {
>>>>>>                 compatible = "intel,syscon";
>>>>> Needs to be SoC specific value.
>>>> Agreed! it should be "intel, lgm-syscon"
>>>>>>                 reg = <0xe0020000 0x100>;
>>>>>>
>>>>>>                 emmc_phy: emmc_phy {
>>>>>>                     compatible = "intel,lgm-emmc-phy";
>>>>>>                     intel,syscon = <&sysconf>;
>>>>> This is redundant because you can just get the parent node.
>>>>>
>>>>> If there's a defined register range within the 'intel,syscon' block
>>>>> then define it here with 'reg'.
>>>> Agreed!, avoided redundant
>>>>
>>>> sysconf: chiptop@e0020000 {
>>>>                compatible = "intel,lgm-syscon";
>>>>                emmc_phy: emmc_phy {
>>>>                    compatible = "intel,lgm-emmc-phy";
>>>>                    reg = <0xe0020000 0x100>;
>>> This is the same addresses you had for the parent, so that doesn't
>>> seem right. The parent should have the entire range and then the child
>>> nodes only the addresses for their functions. However, if the
>>> registers are all interleaved then you can really put 'reg' in the
>>> child nodes and just have it only in the parent. We don't want to have
>>> overlapping addresses in DT.
>> syscon is parent node, which has the base address for all the peripheral
>> registers and used by child nodes.
>> child nodes have only offsets, we do not specify in device tree.
> Right, and I'm asking you to add the offsets whether Linux uses them or not.

Agreed!, will update the patch.

Best Regards
vadivel
> Rob