* [PATCH v2 0/2] phy: Add support for the HDMI TX PHY on Meson8/8b/8m2 @ 2021-06-29 18:20 Martin Blumenstingl 2021-06-29 18:20 ` [PATCH v2 1/2] dt-bindings: phy: Add the Amlogic Meson8 HDMI TX PHY bindings Martin Blumenstingl 2021-06-29 18:20 ` [PATCH v2 2/2] phy: amlogic: Add a new driver for the HDMI TX PHY on Meson8/8b/8m2 Martin Blumenstingl 0 siblings, 2 replies; 7+ messages in thread From: Martin Blumenstingl @ 2021-06-29 18:20 UTC (permalink / raw) To: robh+dt, linux-phy, devicetree, linux-amlogic Cc: kishon, vkoul, linux-arm-kernel, linux-kernel, 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/2] dt-bindings: phy: Add the Amlogic Meson8 HDMI TX PHY bindings 2021-06-29 18:20 [PATCH v2 0/2] phy: Add support for the HDMI TX PHY on Meson8/8b/8m2 Martin Blumenstingl @ 2021-06-29 18:20 ` Martin Blumenstingl 2021-07-14 21:09 ` Rob Herring 2021-06-29 18:20 ` [PATCH v2 2/2] phy: amlogic: Add a new driver for the HDMI TX PHY on Meson8/8b/8m2 Martin Blumenstingl 1 sibling, 1 reply; 7+ messages in thread From: Martin Blumenstingl @ 2021-06-29 18:20 UTC (permalink / raw) To: robh+dt, linux-phy, devicetree, linux-amlogic Cc: kishon, vkoul, linux-arm-kernel, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: phy: Add the Amlogic Meson8 HDMI TX PHY bindings 2021-06-29 18:20 ` [PATCH v2 1/2] dt-bindings: phy: Add the Amlogic Meson8 HDMI TX PHY bindings Martin Blumenstingl @ 2021-07-14 21:09 ` Rob Herring 0 siblings, 0 replies; 7+ messages in thread From: Rob Herring @ 2021-07-14 21:09 UTC (permalink / raw) To: Martin Blumenstingl Cc: vkoul, devicetree, linux-phy, linux-arm-kernel, robh+dt, kishon, linux-kernel, linux-amlogic 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 2/2] phy: amlogic: Add a new driver for the HDMI TX PHY on Meson8/8b/8m2 2021-06-29 18:20 [PATCH v2 0/2] phy: Add support for the HDMI TX PHY on Meson8/8b/8m2 Martin Blumenstingl 2021-06-29 18:20 ` [PATCH v2 1/2] dt-bindings: phy: Add the Amlogic Meson8 HDMI TX PHY bindings Martin Blumenstingl @ 2021-06-29 18:20 ` Martin Blumenstingl 2021-07-20 8:36 ` Vinod Koul 1 sibling, 1 reply; 7+ messages in thread From: Martin Blumenstingl @ 2021-06-29 18:20 UTC (permalink / raw) To: robh+dt, linux-phy, devicetree, linux-amlogic Cc: kishon, vkoul, linux-arm-kernel, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] phy: amlogic: Add a new driver for the HDMI TX PHY on Meson8/8b/8m2 2021-06-29 18:20 ` [PATCH v2 2/2] phy: amlogic: Add a new driver for the HDMI TX PHY on Meson8/8b/8m2 Martin Blumenstingl @ 2021-07-20 8:36 ` Vinod Koul 2021-07-20 22:08 ` Martin Blumenstingl 0 siblings, 1 reply; 7+ messages in thread From: Vinod Koul @ 2021-07-20 8:36 UTC (permalink / raw) To: Martin Blumenstingl Cc: robh+dt, linux-phy, devicetree, linux-amlogic, kishon, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] phy: amlogic: Add a new driver for the HDMI TX PHY on Meson8/8b/8m2 2021-07-20 8:36 ` Vinod Koul @ 2021-07-20 22:08 ` Martin Blumenstingl 2021-07-22 9:08 ` Vinod Koul 0 siblings, 1 reply; 7+ messages in thread From: Martin Blumenstingl @ 2021-07-20 22:08 UTC (permalink / raw) To: Vinod Koul Cc: robh+dt, linux-phy, devicetree, linux-amlogic, kishon, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] phy: amlogic: Add a new driver for the HDMI TX PHY on Meson8/8b/8m2 2021-07-20 22:08 ` Martin Blumenstingl @ 2021-07-22 9:08 ` Vinod Koul 0 siblings, 0 replies; 7+ messages in thread From: Vinod Koul @ 2021-07-22 9:08 UTC (permalink / raw) To: Martin Blumenstingl Cc: robh+dt, linux-phy, devicetree, linux-amlogic, kishon, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-07-22 9:08 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-29 18:20 [PATCH v2 0/2] phy: Add support for the HDMI TX PHY on Meson8/8b/8m2 Martin Blumenstingl 2021-06-29 18:20 ` [PATCH v2 1/2] dt-bindings: phy: Add the Amlogic Meson8 HDMI TX PHY bindings Martin Blumenstingl 2021-07-14 21:09 ` Rob Herring 2021-06-29 18:20 ` [PATCH v2 2/2] phy: amlogic: Add a new driver for the HDMI TX PHY on Meson8/8b/8m2 Martin Blumenstingl 2021-07-20 8:36 ` Vinod Koul 2021-07-20 22:08 ` Martin Blumenstingl 2021-07-22 9:08 ` Vinod Koul
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).