| Message ID | 20210629182047.893415-3-martin.blumenstingl@googlemail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | phy: Add support for the HDMI TX PHY on Meson8/8b/8m2 | expand |
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?
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
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
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");
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