[PATCH v2 0/2] phy: Add support for the HDMI TX PHY on Meson8/8b/8m2

[PATCH v2 0/2] phy: Add support for the HDMI TX PHY on Meson8/8b/8m2

[Martin Blumenstingl]

Amlogic Meson8/8b/8m2 SoCs embed a HDMI TX PHY. Unfortunately there is
no (public) documentation for this hardware. The best thing we have is
the 3.10 vendor kernel, which unfortunately doesn't document most
register bits (only a few are named there, the rest is all magic
numbers).
It is possible that this is a TranSwitch HDMI TX PHY based core, but
this is pure speculation.

Adding a driver for the HDMI TX PHY gets us one step closer to video
output on these older SoCs.


Changes since v1 at [0]:
- add a reg property to the dt-bindings
- parse the reg property (register offset) in the driver
- update copyright year to 2021 (spotted by Vinod, thanks!)
- drop "default ARCH_MESON" from the Kconfig entry


[0] https://patchwork.kernel.org/project/linux-amlogic/cover/20210604190338.2248295-1-martin.blumenstingl@googlemail.com/


Martin Blumenstingl (2):
  dt-bindings: phy: Add the Amlogic Meson8 HDMI TX PHY bindings
  phy: amlogic: Add a new driver for the HDMI TX PHY on Meson8/8b/8m2

 .../phy/amlogic,meson8-hdmi-tx-phy.yaml       |  55 ++++++
 drivers/phy/amlogic/Kconfig                   |  10 ++
 drivers/phy/amlogic/Makefile                  |   1 +
 drivers/phy/amlogic/phy-meson8-hdmi-tx.c      | 162 ++++++++++++++++++
 4 files changed, 228 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/amlogic,meson8-hdmi-tx-phy.yaml
 create mode 100644 drivers/phy/amlogic/phy-meson8-hdmi-tx.c

-- 
2.32.0

[PATCH v2 1/2] dt-bindings: phy: Add the Amlogic Meson8 HDMI TX PHY bindings

[Martin Blumenstingl]

Amlogic Meson8, Meson8b and Meson8m2 all include an identical (or at
least very similar) HDMI TX PHY. The PHY registers are part of the HHI
register area.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 .../phy/amlogic,meson8-hdmi-tx-phy.yaml       | 55 +++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/amlogic,meson8-hdmi-tx-phy.yaml

diff --git a/Documentation/devicetree/bindings/phy/amlogic,meson8-hdmi-tx-phy.yaml b/Documentation/devicetree/bindings/phy/amlogic,meson8-hdmi-tx-phy.yaml
new file mode 100644
index 000000000000..7faf35c260f3
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/amlogic,meson8-hdmi-tx-phy.yaml
@@ -0,0 +1,55 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/phy/amlogic,meson8-hdmi-tx-phy.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Amlogic Meson8, Meson8b and Meson8m2 HDMI TX PHY
+
+maintainers:
+  - Martin Blumenstingl <martin.blumenstingl@googlemail.com>
+
+description: |+
+  The HDMI TX PHY node should be the child of a syscon node with the
+  required property:
+
+  compatible = "amlogic,meson-hhi-sysctrl", "simple-mfd", "syscon"
+
+  Refer to the bindings described in
+  Documentation/devicetree/bindings/mfd/syscon.yaml
+
+properties:
+  $nodename:
+    pattern: "^hdmi-phy@[0-9a-f]+$"
+
+  compatible:
+    enum:
+      - amlogic,meson8-hdmi-tx-phy
+      - amlogic,meson8b-hdmi-tx-phy
+      - amlogic,meson8m2-hdmi-tx-phy
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    minItems: 1
+    description:
+      HDMI TMDS clock
+
+  "#phy-cells":
+    const: 0
+
+required:
+  - compatible
+  - "#phy-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    hdmi_tx_phy: hdmi-phy@3a0 {
+      compatible = "amlogic,meson8-hdmi-tx-phy";
+      reg = <0x3a0 0xc>;
+      clocks = <&tmds_clock>;
+      #phy-cells = <0>;
+    };
-- 
2.32.0

Re: [PATCH v2 1/2] dt-bindings: phy: Add the Amlogic Meson8 HDMI TX PHY bindings

[Rob Herring]

On Tue, 29 Jun 2021 20:20:46 +0200, Martin Blumenstingl wrote:
> Amlogic Meson8, Meson8b and Meson8m2 all include an identical (or at
> least very similar) HDMI TX PHY. The PHY registers are part of the HHI
> register area.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  .../phy/amlogic,meson8-hdmi-tx-phy.yaml       | 55 +++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/amlogic,meson8-hdmi-tx-phy.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH v2 2/2] phy: amlogic: Add a new driver for the HDMI TX PHY on Meson8/8b/8m2

[Martin Blumenstingl]

Amlogic Meson8/8b/8m2 have a built-in HDMI PHY in the HHI register
region. Unfortunately only few register bits are documented. For
HHI_HDMI_PHY_CNTL0 the magic numbers are taken from the 3.10 vendor
kernel.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/phy/amlogic/Kconfig              |  10 ++
 drivers/phy/amlogic/Makefile             |   1 +
 drivers/phy/amlogic/phy-meson8-hdmi-tx.c | 162 +++++++++++++++++++++++
 3 files changed, 173 insertions(+)
 create mode 100644 drivers/phy/amlogic/phy-meson8-hdmi-tx.c

diff --git a/drivers/phy/amlogic/Kconfig b/drivers/phy/amlogic/Kconfig
index db5d0cd757e3..486ca23aba32 100644
--- a/drivers/phy/amlogic/Kconfig
+++ b/drivers/phy/amlogic/Kconfig
@@ -2,6 +2,16 @@
 #
 # Phy drivers for Amlogic platforms
 #
+config PHY_MESON8_HDMI_TX
+	tristate "Meson8, Meson8b and Meson8m2 HDMI TX PHY driver"
+	depends on (ARCH_MESON && ARM) || COMPILE_TEST
+	depends on OF
+	select MFD_SYSCON
+	help
+	  Enable this to support the HDMI TX PHYs found in Meson8,
+	  Meson8b and Meson8m2 SoCs.
+	  If unsure, say N.
+
 config PHY_MESON8B_USB2
 	tristate "Meson8, Meson8b, Meson8m2 and GXBB USB2 PHY driver"
 	default ARCH_MESON
diff --git a/drivers/phy/amlogic/Makefile b/drivers/phy/amlogic/Makefile
index 8fa07fbd0d92..c0886c850bb0 100644
--- a/drivers/phy/amlogic/Makefile
+++ b/drivers/phy/amlogic/Makefile
@@ -1,4 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0-only
+obj-$(CONFIG_PHY_MESON8_HDMI_TX)		+= phy-meson8-hdmi-tx.o
 obj-$(CONFIG_PHY_MESON8B_USB2)			+= phy-meson8b-usb2.o
 obj-$(CONFIG_PHY_MESON_GXL_USB2)		+= phy-meson-gxl-usb2.o
 obj-$(CONFIG_PHY_MESON_G12A_USB2)		+= phy-meson-g12a-usb2.o
diff --git a/drivers/phy/amlogic/phy-meson8-hdmi-tx.c b/drivers/phy/amlogic/phy-meson8-hdmi-tx.c
new file mode 100644
index 000000000000..ba5a4de54811
--- /dev/null
+++ b/drivers/phy/amlogic/phy-meson8-hdmi-tx.c
@@ -0,0 +1,162 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Meson8, Meson8b and Meson8m2 HDMI TX PHY.
+ *
+ * Copyright (C) 2021 Martin Blumenstingl <martin.blumenstingl@googlemail.com>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+
+#define HHI_HDMI_PHY_CNTL0				0x0
+	#define HHI_HDMI_PHY_CNTL0_HDMI_CTL1		GENMASK(31, 16)
+	#define HHI_HDMI_PHY_CNTL0_HDMI_CTL0		GENMASK(15, 0)
+
+#define HHI_HDMI_PHY_CNTL1				0x4
+	#define HHI_HDMI_PHY_CNTL1_CLOCK_ENABLE		BIT(1)
+	#define HHI_HDMI_PHY_CNTL1_SOFT_RESET		BIT(0)
+
+#define HHI_HDMI_PHY_CNTL2				0x8
+
+struct phy_meson8_hdmi_tx_priv {
+	struct regmap		*hhi;
+	struct clk		*tmds_clk;
+	unsigned int		reg_offset;
+};
+
+static int phy_meson8_hdmi_tx_init(struct phy *phy)
+{
+	struct phy_meson8_hdmi_tx_priv *priv = phy_get_drvdata(phy);
+
+	return clk_prepare_enable(priv->tmds_clk);
+}
+
+static int phy_meson8_hdmi_tx_exit(struct phy *phy)
+{
+	struct phy_meson8_hdmi_tx_priv *priv = phy_get_drvdata(phy);
+
+	clk_disable_unprepare(priv->tmds_clk);
+
+	return 0;
+}
+
+static int phy_meson8_hdmi_tx_power_on(struct phy *phy)
+{
+	struct phy_meson8_hdmi_tx_priv *priv = phy_get_drvdata(phy);
+	unsigned int i;
+	u16 hdmi_ctl0;
+
+	if (clk_get_rate(priv->tmds_clk) >= 2970UL * 1000 * 1000)
+		hdmi_ctl0 = 0x1e8b;
+	else
+		hdmi_ctl0 = 0x4d0b;
+
+	regmap_write(priv->hhi, priv->reg_offset + HHI_HDMI_PHY_CNTL0,
+		     FIELD_PREP(HHI_HDMI_PHY_CNTL0_HDMI_CTL1, 0x08c3) |
+		     FIELD_PREP(HHI_HDMI_PHY_CNTL0_HDMI_CTL0, hdmi_ctl0));
+
+	regmap_write(priv->hhi, priv->reg_offset + HHI_HDMI_PHY_CNTL1, 0x0);
+
+	/* Reset three times, just like the vendor driver does */
+	for (i = 0; i < 3; i++) {
+		regmap_write(priv->hhi, priv->reg_offset + HHI_HDMI_PHY_CNTL1,
+			     HHI_HDMI_PHY_CNTL1_CLOCK_ENABLE |
+			     HHI_HDMI_PHY_CNTL1_SOFT_RESET);
+		usleep_range(1000, 2000);
+
+		regmap_write(priv->hhi, priv->reg_offset + HHI_HDMI_PHY_CNTL1,
+			     HHI_HDMI_PHY_CNTL1_CLOCK_ENABLE);
+		usleep_range(1000, 2000);
+	}
+
+	return 0;
+}
+
+static int phy_meson8_hdmi_tx_power_off(struct phy *phy)
+{
+	struct phy_meson8_hdmi_tx_priv *priv = phy_get_drvdata(phy);
+
+	regmap_write(priv->hhi, priv->reg_offset + HHI_HDMI_PHY_CNTL0,
+		     FIELD_PREP(HHI_HDMI_PHY_CNTL0_HDMI_CTL1, 0x0841) |
+		     FIELD_PREP(HHI_HDMI_PHY_CNTL0_HDMI_CTL0, 0x8d00));
+
+	return 0;
+}
+
+static const struct phy_ops phy_meson8_hdmi_tx_ops = {
+	.init		= phy_meson8_hdmi_tx_init,
+	.exit		= phy_meson8_hdmi_tx_exit,
+	.power_on	= phy_meson8_hdmi_tx_power_on,
+	.power_off	= phy_meson8_hdmi_tx_power_off,
+	.owner		= THIS_MODULE,
+};
+
+static int phy_meson8_hdmi_tx_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct phy_meson8_hdmi_tx_priv *priv;
+	struct phy_provider *phy_provider;
+	struct phy *phy;
+	u32 reg[2];
+	int ret;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	ret = device_property_read_u32_array(&pdev->dev, "reg", reg,
+					     ARRAY_SIZE(reg));
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "Failed to parse the 'reg' property\n");
+
+	priv->reg_offset = reg[0];
+
+	priv->hhi = syscon_node_to_regmap(np->parent);
+	if (IS_ERR(priv->hhi))
+		return PTR_ERR(priv->hhi);
+
+	priv->tmds_clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(priv->tmds_clk))
+		return PTR_ERR(priv->tmds_clk);
+
+	phy = devm_phy_create(&pdev->dev, np, &phy_meson8_hdmi_tx_ops);
+	if (IS_ERR(phy))
+		return PTR_ERR(phy);
+
+	phy_set_drvdata(phy, priv);
+
+	phy_provider = devm_of_phy_provider_register(&pdev->dev,
+						     of_phy_simple_xlate);
+
+	return PTR_ERR_OR_ZERO(phy_provider);
+}
+
+static const struct of_device_id phy_meson8_hdmi_tx_of_match[] = {
+	{ .compatible = "amlogic,meson8-hdmi-tx-phy" },
+	{ .compatible = "amlogic,meson8b-hdmi-tx-phy" },
+	{ .compatible = "amlogic,meson8m2-hdmi-tx-phy" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, phy_meson8_hdmi_tx_of_match);
+
+static struct platform_driver phy_meson8_hdmi_tx_driver = {
+	.probe	= phy_meson8_hdmi_tx_probe,
+	.driver	= {
+		.name		= "phy-meson8-hdmi-tx",
+		.of_match_table	= phy_meson8_hdmi_tx_of_match,
+	},
+};
+module_platform_driver(phy_meson8_hdmi_tx_driver);
+
+MODULE_AUTHOR("Martin Blumenstingl <martin.blumenstingl@googlemail.com>");
+MODULE_DESCRIPTION("Meson8, Meson8b and Meson8m2 HDMI TX PHY driver");
+MODULE_LICENSE("GPL v2");
-- 
2.32.0

Re: [PATCH v2 2/2] phy: amlogic: Add a new driver for the HDMI TX PHY on Meson8/8b/8m2

[Vinod Koul]

On 29-06-21, 20:20, Martin Blumenstingl wrote:
> Amlogic Meson8/8b/8m2 have a built-in HDMI PHY in the HHI register
> region. Unfortunately only few register bits are documented. For
> HHI_HDMI_PHY_CNTL0 the magic numbers are taken from the 3.10 vendor
> kernel.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/phy/amlogic/Kconfig              |  10 ++
>  drivers/phy/amlogic/Makefile             |   1 +
>  drivers/phy/amlogic/phy-meson8-hdmi-tx.c | 162 +++++++++++++++++++++++
>  3 files changed, 173 insertions(+)
>  create mode 100644 drivers/phy/amlogic/phy-meson8-hdmi-tx.c
> 
> diff --git a/drivers/phy/amlogic/Kconfig b/drivers/phy/amlogic/Kconfig
> index db5d0cd757e3..486ca23aba32 100644
> --- a/drivers/phy/amlogic/Kconfig
> +++ b/drivers/phy/amlogic/Kconfig
> @@ -2,6 +2,16 @@
>  #
>  # Phy drivers for Amlogic platforms
>  #
> +config PHY_MESON8_HDMI_TX
> +	tristate "Meson8, Meson8b and Meson8m2 HDMI TX PHY driver"
> +	depends on (ARCH_MESON && ARM) || COMPILE_TEST
> +	depends on OF
> +	select MFD_SYSCON
> +	help
> +	  Enable this to support the HDMI TX PHYs found in Meson8,
> +	  Meson8b and Meson8m2 SoCs.
> +	  If unsure, say N.
> +
>  config PHY_MESON8B_USB2
>  	tristate "Meson8, Meson8b, Meson8m2 and GXBB USB2 PHY driver"
>  	default ARCH_MESON
> diff --git a/drivers/phy/amlogic/Makefile b/drivers/phy/amlogic/Makefile
> index 8fa07fbd0d92..c0886c850bb0 100644
> --- a/drivers/phy/amlogic/Makefile
> +++ b/drivers/phy/amlogic/Makefile
> @@ -1,4 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> +obj-$(CONFIG_PHY_MESON8_HDMI_TX)		+= phy-meson8-hdmi-tx.o
>  obj-$(CONFIG_PHY_MESON8B_USB2)			+= phy-meson8b-usb2.o
>  obj-$(CONFIG_PHY_MESON_GXL_USB2)		+= phy-meson-gxl-usb2.o
>  obj-$(CONFIG_PHY_MESON_G12A_USB2)		+= phy-meson-g12a-usb2.o
> diff --git a/drivers/phy/amlogic/phy-meson8-hdmi-tx.c b/drivers/phy/amlogic/phy-meson8-hdmi-tx.c
> new file mode 100644
> index 000000000000..ba5a4de54811
> --- /dev/null
> +++ b/drivers/phy/amlogic/phy-meson8-hdmi-tx.c
> @@ -0,0 +1,162 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Meson8, Meson8b and Meson8m2 HDMI TX PHY.
> + *
> + * Copyright (C) 2021 Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +
> +#define HHI_HDMI_PHY_CNTL0				0x0
> +	#define HHI_HDMI_PHY_CNTL0_HDMI_CTL1		GENMASK(31, 16)
> +	#define HHI_HDMI_PHY_CNTL0_HDMI_CTL0		GENMASK(15, 0)
> +
> +#define HHI_HDMI_PHY_CNTL1				0x4
> +	#define HHI_HDMI_PHY_CNTL1_CLOCK_ENABLE		BIT(1)
> +	#define HHI_HDMI_PHY_CNTL1_SOFT_RESET		BIT(0)
> +
> +#define HHI_HDMI_PHY_CNTL2				0x8
> +
> +struct phy_meson8_hdmi_tx_priv {
> +	struct regmap		*hhi;
> +	struct clk		*tmds_clk;
> +	unsigned int		reg_offset;
> +};
> +
> +static int phy_meson8_hdmi_tx_init(struct phy *phy)
> +{
> +	struct phy_meson8_hdmi_tx_priv *priv = phy_get_drvdata(phy);
> +
> +	return clk_prepare_enable(priv->tmds_clk);
> +}
> +
> +static int phy_meson8_hdmi_tx_exit(struct phy *phy)
> +{
> +	struct phy_meson8_hdmi_tx_priv *priv = phy_get_drvdata(phy);
> +
> +	clk_disable_unprepare(priv->tmds_clk);
> +
> +	return 0;
> +}
> +
> +static int phy_meson8_hdmi_tx_power_on(struct phy *phy)
> +{
> +	struct phy_meson8_hdmi_tx_priv *priv = phy_get_drvdata(phy);
> +	unsigned int i;
> +	u16 hdmi_ctl0;
> +
> +	if (clk_get_rate(priv->tmds_clk) >= 2970UL * 1000 * 1000)
> +		hdmi_ctl0 = 0x1e8b;
> +	else
> +		hdmi_ctl0 = 0x4d0b;

magic numbers..? I guess these are register offsets, would be better to
define..

> +
> +	regmap_write(priv->hhi, priv->reg_offset + HHI_HDMI_PHY_CNTL0,
> +		     FIELD_PREP(HHI_HDMI_PHY_CNTL0_HDMI_CTL1, 0x08c3) |
> +		     FIELD_PREP(HHI_HDMI_PHY_CNTL0_HDMI_CTL0, hdmi_ctl0));
> +
> +	regmap_write(priv->hhi, priv->reg_offset + HHI_HDMI_PHY_CNTL1, 0x0);
> +
> +	/* Reset three times, just like the vendor driver does */
> +	for (i = 0; i < 3; i++) {
> +		regmap_write(priv->hhi, priv->reg_offset + HHI_HDMI_PHY_CNTL1,
> +			     HHI_HDMI_PHY_CNTL1_CLOCK_ENABLE |
> +			     HHI_HDMI_PHY_CNTL1_SOFT_RESET);
> +		usleep_range(1000, 2000);
> +
> +		regmap_write(priv->hhi, priv->reg_offset + HHI_HDMI_PHY_CNTL1,
> +			     HHI_HDMI_PHY_CNTL1_CLOCK_ENABLE);
> +		usleep_range(1000, 2000);
> +	}
> +
> +	return 0;
> +}
> +
> +static int phy_meson8_hdmi_tx_power_off(struct phy *phy)
> +{
> +	struct phy_meson8_hdmi_tx_priv *priv = phy_get_drvdata(phy);
> +
> +	regmap_write(priv->hhi, priv->reg_offset + HHI_HDMI_PHY_CNTL0,
> +		     FIELD_PREP(HHI_HDMI_PHY_CNTL0_HDMI_CTL1, 0x0841) |
> +		     FIELD_PREP(HHI_HDMI_PHY_CNTL0_HDMI_CTL0, 0x8d00));

more magic..

> +
> +	return 0;
> +}
> +
> +static const struct phy_ops phy_meson8_hdmi_tx_ops = {
> +	.init		= phy_meson8_hdmi_tx_init,
> +	.exit		= phy_meson8_hdmi_tx_exit,
> +	.power_on	= phy_meson8_hdmi_tx_power_on,
> +	.power_off	= phy_meson8_hdmi_tx_power_off,
> +	.owner		= THIS_MODULE,
> +};
> +
> +static int phy_meson8_hdmi_tx_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct phy_meson8_hdmi_tx_priv *priv;
> +	struct phy_provider *phy_provider;
> +	struct phy *phy;
> +	u32 reg[2];
> +	int ret;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	ret = device_property_read_u32_array(&pdev->dev, "reg", reg,
> +					     ARRAY_SIZE(reg));

we have reg as single property, why array with 2 entries here?

> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "Failed to parse the 'reg' property\n");
> +
> +	priv->reg_offset = reg[0];

and mystery deepens, no use of reg[1],  leftover artifacts?

> +
> +	priv->hhi = syscon_node_to_regmap(np->parent);
> +	if (IS_ERR(priv->hhi))
> +		return PTR_ERR(priv->hhi);
> +
> +	priv->tmds_clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(priv->tmds_clk))
> +		return PTR_ERR(priv->tmds_clk);
> +
> +	phy = devm_phy_create(&pdev->dev, np, &phy_meson8_hdmi_tx_ops);
> +	if (IS_ERR(phy))
> +		return PTR_ERR(phy);
> +
> +	phy_set_drvdata(phy, priv);
> +
> +	phy_provider = devm_of_phy_provider_register(&pdev->dev,
> +						     of_phy_simple_xlate);
> +
> +	return PTR_ERR_OR_ZERO(phy_provider);
> +}
> +
> +static const struct of_device_id phy_meson8_hdmi_tx_of_match[] = {
> +	{ .compatible = "amlogic,meson8-hdmi-tx-phy" },
> +	{ .compatible = "amlogic,meson8b-hdmi-tx-phy" },
> +	{ .compatible = "amlogic,meson8m2-hdmi-tx-phy" },
> +	{ /* sentinel */ }

I see that all three are handled similarly, no difference!

Maybe also add a amlogic,meson8-hdmi compatible and use that?

-- 
~Vinod

Re: [PATCH v2 2/2] phy: amlogic: Add a new driver for the HDMI TX PHY on Meson8/8b/8m2

[Martin Blumenstingl]

Hi Vinod,

On Tue, Jul 20, 2021 at 10:37 AM Vinod Koul <vkoul@kernel.org> wrote:
[...]
> > +     if (clk_get_rate(priv->tmds_clk) >= 2970UL * 1000 * 1000)
> > +             hdmi_ctl0 = 0x1e8b;
> > +     else
> > +             hdmi_ctl0 = 0x4d0b;
>
> magic numbers..? I guess these are register offsets, would be better to
> define..
Unfortunately these are register values, not offsets
The only "documentation" I have is:
- documentation for the bits/bit-fields in these registers [0]
- some reference code with magic values from the vendor BSP: [1]

HDMI_CTL0/HDMI_CTL1 (the names from the datasheet) is not very
specific and I could not find any other explanation on what the values
mean.
That's why I cannot offer more than these magic values (same situation
for your finding below).

[...]
> > +     ret = device_property_read_u32_array(&pdev->dev, "reg", reg,
> > +                                          ARRAY_SIZE(reg));
>
> we have reg as single property, why array with 2 entries here?
My thought when Rob requested a "reg" property in the dt-bindings was
that I should use offset and size.
I am not validating the size here, which would be in reg[1].
If it's fine for Rob as well then I'll switch the dt-bindings to just
have the offset inside the reg property.

[...]
> > +static const struct of_device_id phy_meson8_hdmi_tx_of_match[] = {
> > +     { .compatible = "amlogic,meson8-hdmi-tx-phy" },
> > +     { .compatible = "amlogic,meson8b-hdmi-tx-phy" },
> > +     { .compatible = "amlogic,meson8m2-hdmi-tx-phy" },
> > +     { /* sentinel */ }
>
> I see that all three are handled similarly, no difference!
So far this is correct, they're all treated the same.
However, it happened to me (multiple times) in the past that later on
I would spot a difference hidden in the vendor BSP.
One example is commit f004be596c28f9 ("phy: amlogic: meson8b-usb2: Add
a compatible string for Meson8m2").
I know that other parts of the graphics pipeline are different on
Meson8 compared to the other two SoCs (because Meson8b/Meson8m2 have
some reset lines which need to be toggled after updating the video
clocks. these resets don't exist on Meson8).
So I decided to play safe and add compatible strings for every SoC so
I can easily handle any differences in the future (in case I find
any).


Best regards,
Martin


[0] https://dn.odroid.com/S805/Datasheet/S805_Datasheet%20V0.8%2020150126.pdf
[1] https://github.com/endlessm/linux-meson/blob/d6e13c220931110fe676ede6da69fc61a7cb04b6/arch/arm/mach-meson8/hdmi_tx_hw/hdmi_tx_hw.c#L2350-L2381

Re: [PATCH v2 2/2] phy: amlogic: Add a new driver for the HDMI TX PHY on Meson8/8b/8m2

[Vinod Koul]

On 21-07-21, 00:08, Martin Blumenstingl wrote:
> Hi Vinod,
> 
> On Tue, Jul 20, 2021 at 10:37 AM Vinod Koul <vkoul@kernel.org> wrote:
> [...]
> > > +     if (clk_get_rate(priv->tmds_clk) >= 2970UL * 1000 * 1000)
> > > +             hdmi_ctl0 = 0x1e8b;
> > > +     else
> > > +             hdmi_ctl0 = 0x4d0b;
> >
> > magic numbers..? I guess these are register offsets, would be better to
> > define..
> Unfortunately these are register values, not offsets
> The only "documentation" I have is:
> - documentation for the bits/bit-fields in these registers [0]
> - some reference code with magic values from the vendor BSP: [1]
> 
> HDMI_CTL0/HDMI_CTL1 (the names from the datasheet) is not very
> specific and I could not find any other explanation on what the values
> mean.
> That's why I cannot offer more than these magic values (same situation
> for your finding below).

Ok, Can you add a comment that register documentation not available ...


> > > +     ret = device_property_read_u32_array(&pdev->dev, "reg", reg,
> > > +                                          ARRAY_SIZE(reg));
> >
> > we have reg as single property, why array with 2 entries here?
> My thought when Rob requested a "reg" property in the dt-bindings was
> that I should use offset and size.
> I am not validating the size here, which would be in reg[1].
> If it's fine for Rob as well then I'll switch the dt-bindings to just
> have the offset inside the reg property.

So the property is reg address and size. Two would imply you are using
two reg values.

So I would recommend to use:
        reg_offset = platform_get_resource(pdev, IORESOURCE_MEM, 0);

and skip this reg array.


> 
> [...]
> > > +static const struct of_device_id phy_meson8_hdmi_tx_of_match[] = {
> > > +     { .compatible = "amlogic,meson8-hdmi-tx-phy" },
> > > +     { .compatible = "amlogic,meson8b-hdmi-tx-phy" },
> > > +     { .compatible = "amlogic,meson8m2-hdmi-tx-phy" },
> > > +     { /* sentinel */ }
> >
> > I see that all three are handled similarly, no difference!
> So far this is correct, they're all treated the same.
> However, it happened to me (multiple times) in the past that later on
> I would spot a difference hidden in the vendor BSP.
> One example is commit f004be596c28f9 ("phy: amlogic: meson8b-usb2: Add
> a compatible string for Meson8m2").
> I know that other parts of the graphics pipeline are different on
> Meson8 compared to the other two SoCs (because Meson8b/Meson8m2 have
> some reset lines which need to be toggled after updating the video
> clocks. these resets don't exist on Meson8).
> So I decided to play safe and add compatible strings for every SoC so
> I can easily handle any differences in the future (in case I find
> any).

Correct, that is why you need to *keep* the SoC specific compatible and
document them. But use a generic one when you don't have any delta

Above would become:
        { .compatible = "amlogic,meson8-hdmi" },

with DTS specifying:
        compatible = "amlogic,meson8-hdmi-tx-phy", "amlogic,meson8-hdmi";

That way if required you can always use the specific one

-- 
~Vinod