Add support for SDXC PHY on Intel's Lightning Mountain SoC.
Signed-off-by: Ramuthevar Vadivel Murugan <[email protected]>
---
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
From: Ramuthevar Vadivel Murugan <[email protected]>
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 <[email protected]>
---
.../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 <[email protected]>
+
+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 <[email protected]>
+
+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
From: Ramuthevar Vadivel Murugan <[email protected]>
Add support for SDXC PHY on Intel's Lightning Mountain SoC.
Signed-off-by: Ramuthevar Vadivel Murugan <[email protected]>
---
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 <[email protected]>");
+MODULE_DESCRIPTION("Intel SDXC PHY driver");
+MODULE_LICENSE("GPL v2");
--
2.11.0
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
> <[email protected]>
> +
> +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
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
>> <[email protected]>
>> +
>> +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
On Wed, Aug 28, 2019 at 08:43:14PM +0800, Ramuthevar,Vadivel MuruganX wrote:
> From: Ramuthevar Vadivel Murugan <[email protected]>
>
> 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 <[email protected]>
> ---
> .../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 <[email protected]>
> +
> +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 <[email protected]>
> +
> +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
>
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 <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> .../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 <[email protected]>
>> +
>> +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 <[email protected]>
>> +
>> +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
>>
On Tue, Sep 3, 2019 at 2:57 AM Ramuthevar, Vadivel MuruganX
<[email protected]> 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 <[email protected]>
> >>
> >> 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 <[email protected]>
> >> ---
> >> .../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 <[email protected]>
> >> +
> >> +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>;
> >> + };
> >> +
> >> +...
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
> <[email protected]> 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 <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> ---
>>>> .../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 <[email protected]>
>>>> +
>>>> +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>;
>>>> + };
>>>> +
>>>> +...
On Tue, Sep 3, 2019 at 11:08 AM Ramuthevar, Vadivel MuruganX
<[email protected]> 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
> > <[email protected]> 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 <[email protected]>
> >>>>
> >>>> 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 <[email protected]>
> >>>> ---
> >>>> .../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 <[email protected]>
> >>>> +
> >>>> +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
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
> <[email protected]> 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
>>> <[email protected]> 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 <[email protected]>
>>>>>>
>>>>>> 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 <[email protected]>
>>>>>> ---
>>>>>> .../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 <[email protected]>
>>>>>> +
>>>>>> +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
On Tue, Sep 3, 2019 at 11:52 AM Ramuthevar, Vadivel MuruganX
<[email protected]> 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
> > <[email protected]> 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
> >>> <[email protected]> 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 <[email protected]>
> >>>>>>
> >>>>>> 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 <[email protected]>
> >>>>>> ---
> >>>>>> .../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 <[email protected]>
> >>>>>> +
> >>>>>> +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
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
> <[email protected]> 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
>>> <[email protected]> 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
>>>>> <[email protected]> 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 <[email protected]>
>>>>>>>>
>>>>>>>> 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 <[email protected]>
>>>>>>>> ---
>>>>>>>> .../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 <[email protected]>
>>>>>>>> +
>>>>>>>> +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