Add a compatible and platform datas to support HDMI for rk3568 SoC.
version 2:
- Add the clocks needed for the phy.
Benjamin Gaignard (2):
dt-bindings: display: rockchip: Add compatible for rk3568 HDMI
drm/rockchip: dw_hdmi: add rk3568 support
.../display/rockchip/rockchip,dw-hdmi.yaml | 6 +-
drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 68 +++++++++++++++++++
2 files changed, 73 insertions(+), 1 deletion(-)
--
2.25.1
Add a new dw_hdmi_plat_data struct and new compatible for rk3568.
This version of the HDMI hardware block need two clocks to provide
phy reference clock: hclk_vio and hclk.
Signed-off-by: Benjamin Gaignard <[email protected]>
---
version 2:
- Add the clocks needed for the phy.
drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 68 +++++++++++++++++++++
1 file changed, 68 insertions(+)
diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
index 830bdd5e9b7ce..dc0e255e45745 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
@@ -50,6 +50,10 @@
#define RK3399_GRF_SOC_CON20 0x6250
#define RK3399_HDMI_LCDC_SEL BIT(6)
+#define RK3568_GRF_VO_CON1 0x0364
+#define RK3568_HDMI_SDAIN_MSK BIT(15)
+#define RK3568_HDMI_SCLIN_MSK BIT(14)
+
#define HIWORD_UPDATE(val, mask) (val | (mask) << 16)
/**
@@ -71,6 +75,8 @@ struct rockchip_hdmi {
const struct rockchip_hdmi_chip_data *chip_data;
struct clk *vpll_clk;
struct clk *grf_clk;
+ struct clk *hclk_vio;
+ struct clk *hclk_vop;
struct dw_hdmi *hdmi;
struct phy *phy;
};
@@ -216,6 +222,26 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
return PTR_ERR(hdmi->grf_clk);
}
+ hdmi->hclk_vio = devm_clk_get(hdmi->dev, "hclk_vio");
+ if (PTR_ERR(hdmi->hclk_vio) == -ENOENT) {
+ hdmi->hclk_vio = NULL;
+ } else if (PTR_ERR(hdmi->hclk_vio) == -EPROBE_DEFER) {
+ return -EPROBE_DEFER;
+ } else if (IS_ERR(hdmi->hclk_vio)) {
+ dev_err(hdmi->dev, "failed to get hclk_vio clock\n");
+ return PTR_ERR(hdmi->hclk_vio);
+ }
+
+ hdmi->hclk_vop = devm_clk_get(hdmi->dev, "hclk");
+ if (PTR_ERR(hdmi->hclk_vop) == -ENOENT) {
+ hdmi->hclk_vop = NULL;
+ } else if (PTR_ERR(hdmi->hclk_vop) == -EPROBE_DEFER) {
+ return -EPROBE_DEFER;
+ } else if (IS_ERR(hdmi->hclk_vop)) {
+ dev_err(hdmi->dev, "failed to get hclk_vop clock\n");
+ return PTR_ERR(hdmi->hclk_vop);
+ }
+
return 0;
}
@@ -467,6 +493,19 @@ static const struct dw_hdmi_plat_data rk3399_hdmi_drv_data = {
.use_drm_infoframe = true,
};
+static struct rockchip_hdmi_chip_data rk3568_chip_data = {
+ .lcdsel_grf_reg = -1,
+};
+
+static const struct dw_hdmi_plat_data rk3568_hdmi_drv_data = {
+ .mode_valid = dw_hdmi_rockchip_mode_valid,
+ .mpll_cfg = rockchip_mpll_cfg,
+ .cur_ctr = rockchip_cur_ctr,
+ .phy_config = rockchip_phy_config,
+ .phy_data = &rk3568_chip_data,
+ .use_drm_infoframe = true,
+};
+
static const struct of_device_id dw_hdmi_rockchip_dt_ids[] = {
{ .compatible = "rockchip,rk3228-dw-hdmi",
.data = &rk3228_hdmi_drv_data
@@ -480,6 +519,9 @@ static const struct of_device_id dw_hdmi_rockchip_dt_ids[] = {
{ .compatible = "rockchip,rk3399-dw-hdmi",
.data = &rk3399_hdmi_drv_data
},
+ { .compatible = "rockchip,rk3568-dw-hdmi",
+ .data = &rk3568_hdmi_drv_data
+ },
{},
};
MODULE_DEVICE_TABLE(of, dw_hdmi_rockchip_dt_ids);
@@ -536,6 +578,28 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
return ret;
}
+ ret = clk_prepare_enable(hdmi->hclk_vio);
+ if (ret) {
+ dev_err(hdmi->dev, "Failed to enable HDMI hclk_vio: %d\n",
+ ret);
+ return ret;
+ }
+
+ ret = clk_prepare_enable(hdmi->hclk_vop);
+ if (ret) {
+ dev_err(hdmi->dev, "Failed to enable HDMI hclk_vop: %d\n",
+ ret);
+ return ret;
+ }
+
+ if (hdmi->chip_data == &rk3568_chip_data) {
+ regmap_write(hdmi->regmap, RK3568_GRF_VO_CON1,
+ HIWORD_UPDATE(RK3568_HDMI_SDAIN_MSK |
+ RK3568_HDMI_SCLIN_MSK,
+ RK3568_HDMI_SDAIN_MSK |
+ RK3568_HDMI_SCLIN_MSK));
+ }
+
hdmi->phy = devm_phy_optional_get(dev, "hdmi");
if (IS_ERR(hdmi->phy)) {
ret = PTR_ERR(hdmi->phy);
@@ -559,6 +623,8 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
ret = PTR_ERR(hdmi->hdmi);
drm_encoder_cleanup(encoder);
clk_disable_unprepare(hdmi->vpll_clk);
+ clk_disable_unprepare(hdmi->hclk_vio);
+ clk_disable_unprepare(hdmi->hclk_vop);
}
return ret;
@@ -571,6 +637,8 @@ static void dw_hdmi_rockchip_unbind(struct device *dev, struct device *master,
dw_hdmi_unbind(hdmi->hdmi);
clk_disable_unprepare(hdmi->vpll_clk);
+ clk_disable_unprepare(hdmi->hclk_vio);
+ clk_disable_unprepare(hdmi->hclk_vop);
}
static const struct component_ops dw_hdmi_rockchip_ops = {
--
2.25.1
Define a new compatible for rk3568 HDMI.
This version of HDMI hardware block needs two new clocks hclk_vio and hclk
to provide phy reference clocks.
Signed-off-by: Benjamin Gaignard <[email protected]>
---
version 2:
- Add the clocks needed for the phy.
.../bindings/display/rockchip/rockchip,dw-hdmi.yaml | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
index 75cd9c686e985..cb8643b3a8b84 100644
--- a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
+++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
@@ -23,6 +23,7 @@ properties:
- rockchip,rk3288-dw-hdmi
- rockchip,rk3328-dw-hdmi
- rockchip,rk3399-dw-hdmi
+ - rockchip,rk3568-dw-hdmi
reg-io-width:
const: 4
@@ -51,8 +52,11 @@ properties:
- vpll
- enum:
- grf
+ - hclk_vio
+ - vpll
+ - enum:
+ - hclk
- vpll
- - const: vpll
ddc-i2c-bus:
$ref: /schemas/types.yaml#/definitions/phandle
--
2.25.1
On Wed, 07 Jul 2021 14:03:22 +0200, Benjamin Gaignard wrote:
> Define a new compatible for rk3568 HDMI.
> This version of HDMI hardware block needs two new clocks hclk_vio and hclk
> to provide phy reference clocks.
>
> Signed-off-by: Benjamin Gaignard <[email protected]>
> ---
> version 2:
> - Add the clocks needed for the phy.
>
> .../bindings/display/rockchip/rockchip,dw-hdmi.yaml | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
Reviewed-by: Rob Herring <[email protected]>
Hello Benjamin,
The HDMI TX block in the RK3568 requires two power supplies, which have
to be enabled in some cases (at least on the RK3568 EVB1 the voltages
VDDA0V9_IMAGE and VCCA1V8_IMAGE are disabled by default). It would be
great if this was considered by the driver and the device tree binding.
I am not sure, though, whether this is a RK3568 specific or
rockchip_dw_hdmi specific thing. Maybe it can even enter the Synopsis DW
HDMI driver.
On 7/7/21 2:03 PM, Benjamin Gaignard wrote:
> Define a new compatible for rk3568 HDMI.
> This version of HDMI hardware block needs two new clocks hclk_vio and hclk
> to provide phy reference clocks.
>
> Signed-off-by: Benjamin Gaignard <[email protected]>
> ---
> version 2:
> - Add the clocks needed for the phy.
>
> .../bindings/display/rockchip/rockchip,dw-hdmi.yaml | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> index 75cd9c686e985..cb8643b3a8b84 100644
> --- a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> @@ -23,6 +23,7 @@ properties:
> - rockchip,rk3288-dw-hdmi
> - rockchip,rk3328-dw-hdmi
> - rockchip,rk3399-dw-hdmi
> + - rockchip,rk3568-dw-hdmi
>
> reg-io-width:
> const: 4
> @@ -51,8 +52,11 @@ properties:
> - vpll
> - enum:
> - grf
> + - hclk_vio
> + - vpll
> + - enum:
> + - hclk
> - vpll
> - - const: vpll
The description and documentation of the clocks are somewhat misleading
IMHO. This is not caused by your patches, of course. But maybe this is a
chance to clean them up a bit.
It seems that the CEC clock is an optional clock of the dw-hdmi driver.
Shouldn't it be documented in the synopsys,dw-hdmi.yaml?
Also, it would be nice if the clocks hclk_vio and hclk featured a
description in the binding.
BTW, I am not too familiar with the syntax here, but shouldn't items in
clocks and items in clock-names be aligned (currently, there is a plain
list vs. an enum structure)?
Best regards,
Michael
>
> ddc-i2c-bus:
> $ref: /schemas/types.yaml#/definitions/phandle
>
Hi Michael,
Am Dienstag, 13. Juli 2021, 10:44:00 CEST schrieb Michael Riesch:
> The HDMI TX block in the RK3568 requires two power supplies, which have
> to be enabled in some cases (at least on the RK3568 EVB1 the voltages
> VDDA0V9_IMAGE and VCCA1V8_IMAGE are disabled by default). It would be
> great if this was considered by the driver and the device tree binding.
> I am not sure, though, whether this is a RK3568 specific or
> rockchip_dw_hdmi specific thing. Maybe it can even enter the Synopsis DW
> HDMI driver.
I do remember that this discussion happened many years back already.
And yes the supplies are needed for all but back then there was opposition
as these are supposedly phy-related supplies, not for the dw-hdmi itself.
[There are variants with an external phy, like on the rk3328]
See discussion on [0]
[0] https://dri-devel.freedesktop.narkive.com/pen2zWo1/patch-v3-1-2-drm-bridge-dw-hdmi-support-optional-supply-regulators
> On 7/7/21 2:03 PM, Benjamin Gaignard wrote:
> > Define a new compatible for rk3568 HDMI.
> > This version of HDMI hardware block needs two new clocks hclk_vio and hclk
> > to provide phy reference clocks.
> >
> > Signed-off-by: Benjamin Gaignard <[email protected]>
> > ---
> > version 2:
> > - Add the clocks needed for the phy.
> >
> > .../bindings/display/rockchip/rockchip,dw-hdmi.yaml | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> > index 75cd9c686e985..cb8643b3a8b84 100644
> > --- a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> > +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> > @@ -23,6 +23,7 @@ properties:
> > - rockchip,rk3288-dw-hdmi
> > - rockchip,rk3328-dw-hdmi
> > - rockchip,rk3399-dw-hdmi
> > + - rockchip,rk3568-dw-hdmi
> >
> > reg-io-width:
> > const: 4
> > @@ -51,8 +52,11 @@ properties:
> > - vpll
> > - enum:
> > - grf
> > + - hclk_vio
> > + - vpll
> > + - enum:
> > + - hclk
> > - vpll
> > - - const: vpll
>
> The description and documentation of the clocks are somewhat misleading
> IMHO. This is not caused by your patches, of course. But maybe this is a
> chance to clean them up a bit.
>
> It seems that the CEC clock is an optional clock of the dw-hdmi driver.
> Shouldn't it be documented in the synopsys,dw-hdmi.yaml?
>
> Also, it would be nice if the clocks hclk_vio and hclk featured a
> description in the binding.
>
> BTW, I am not too familiar with the syntax here, but shouldn't items in
> clocks and items in clock-names be aligned (currently, there is a plain
> list vs. an enum structure)?
>
> Best regards,
> Michael
>
> >
> > ddc-i2c-bus:
> > $ref: /schemas/types.yaml#/definitions/phandle
> >
>
On 2021-07-07 13:03, Benjamin Gaignard wrote:
> Define a new compatible for rk3568 HDMI.
> This version of HDMI hardware block needs two new clocks hclk_vio and hclk
> to provide phy reference clocks.
Do you know what hclk_vio is and whether it's actually necessary? I
don't see any mention of it downstream, and based on previous experience
I'm suspicious that it might be just the parent of hclk, and thus should
not need to be explicitly claimed by the device or baked into it's binding.
Robin.
> Signed-off-by: Benjamin Gaignard <[email protected]>
> ---
> version 2:
> - Add the clocks needed for the phy.
>
> .../bindings/display/rockchip/rockchip,dw-hdmi.yaml | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> index 75cd9c686e985..cb8643b3a8b84 100644
> --- a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> @@ -23,6 +23,7 @@ properties:
> - rockchip,rk3288-dw-hdmi
> - rockchip,rk3328-dw-hdmi
> - rockchip,rk3399-dw-hdmi
> + - rockchip,rk3568-dw-hdmi
>
> reg-io-width:
> const: 4
> @@ -51,8 +52,11 @@ properties:
> - vpll
> - enum:
> - grf
> + - hclk_vio
> + - vpll
> + - enum:
> + - hclk
> - vpll
> - - const: vpll
>
> ddc-i2c-bus:
> $ref: /schemas/types.yaml#/definitions/phandle
>
Hi Benjamin,
Am 07.07.21 um 14:03 schrieb Benjamin Gaignard:
> Add a new dw_hdmi_plat_data struct and new compatible for rk3568.
> This version of the HDMI hardware block need two clocks to provide
> phy reference clock: hclk_vio and hclk.
>
> Signed-off-by: Benjamin Gaignard <[email protected]>
> ---
> version 2:
> - Add the clocks needed for the phy.
If got Alega's comment correct, it wasn't about the hclks.
It looks like for this variant, there is another reference clock
required (for the phy) like vpll is already (looks like downstream uses
HPLL ( = "HDMI-PLL" ?) for that - which also has to switch the frequency
according the the drm mode rate - the two clocks you added here are get
just enabled (and disabled) here.
Alega, Andy: Is it really required to enable hclk_vio and hclk(_vop) in
the hdmi driver? Are they required to be enabled for the other output
variants (i.e. mipi, dsi, rgb ....) as well and shouldn't better be
enabled in the (not-yet existing) vop2 driver?
Overall: I'm not sure of the benefit of adding this hdmi variant for a
SoC where the display driver isn't implemented upstream yet. The "VOP2"
IP seems widely new and should probably be ported first. (even if the
HDMI part seems a low hanging fruit according to the vendor sources)
Best,
Alex
>
> drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 68 +++++++++++++++++++++
> 1 file changed, 68 insertions(+)
>
> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> index 830bdd5e9b7ce..dc0e255e45745 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> @@ -50,6 +50,10 @@
> #define RK3399_GRF_SOC_CON20 0x6250
> #define RK3399_HDMI_LCDC_SEL BIT(6)
>
> +#define RK3568_GRF_VO_CON1 0x0364
> +#define RK3568_HDMI_SDAIN_MSK BIT(15)
> +#define RK3568_HDMI_SCLIN_MSK BIT(14)
> +
> #define HIWORD_UPDATE(val, mask) (val | (mask) << 16)
>
> /**
> @@ -71,6 +75,8 @@ struct rockchip_hdmi {
> const struct rockchip_hdmi_chip_data *chip_data;
> struct clk *vpll_clk;
> struct clk *grf_clk;
> + struct clk *hclk_vio;
> + struct clk *hclk_vop;
> struct dw_hdmi *hdmi;
> struct phy *phy;
> };
> @@ -216,6 +222,26 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
> return PTR_ERR(hdmi->grf_clk);
> }
>
> + hdmi->hclk_vio = devm_clk_get(hdmi->dev, "hclk_vio");
> + if (PTR_ERR(hdmi->hclk_vio) == -ENOENT) {
> + hdmi->hclk_vio = NULL;
> + } else if (PTR_ERR(hdmi->hclk_vio) == -EPROBE_DEFER) {
> + return -EPROBE_DEFER;
> + } else if (IS_ERR(hdmi->hclk_vio)) {
> + dev_err(hdmi->dev, "failed to get hclk_vio clock\n");
> + return PTR_ERR(hdmi->hclk_vio);
> + }
> +
> + hdmi->hclk_vop = devm_clk_get(hdmi->dev, "hclk");
> + if (PTR_ERR(hdmi->hclk_vop) == -ENOENT) {
> + hdmi->hclk_vop = NULL;
> + } else if (PTR_ERR(hdmi->hclk_vop) == -EPROBE_DEFER) {
> + return -EPROBE_DEFER;
> + } else if (IS_ERR(hdmi->hclk_vop)) {
> + dev_err(hdmi->dev, "failed to get hclk_vop clock\n");
> + return PTR_ERR(hdmi->hclk_vop);
> + }
> +
> return 0;
> }
>
> @@ -467,6 +493,19 @@ static const struct dw_hdmi_plat_data rk3399_hdmi_drv_data = {
> .use_drm_infoframe = true,
> };
>
> +static struct rockchip_hdmi_chip_data rk3568_chip_data = {
> + .lcdsel_grf_reg = -1,
> +};
> +
> +static const struct dw_hdmi_plat_data rk3568_hdmi_drv_data = {
> + .mode_valid = dw_hdmi_rockchip_mode_valid,
> + .mpll_cfg = rockchip_mpll_cfg,
> + .cur_ctr = rockchip_cur_ctr,
> + .phy_config = rockchip_phy_config,
> + .phy_data = &rk3568_chip_data,
> + .use_drm_infoframe = true,
> +};
> +
> static const struct of_device_id dw_hdmi_rockchip_dt_ids[] = {
> { .compatible = "rockchip,rk3228-dw-hdmi",
> .data = &rk3228_hdmi_drv_data
> @@ -480,6 +519,9 @@ static const struct of_device_id dw_hdmi_rockchip_dt_ids[] = {
> { .compatible = "rockchip,rk3399-dw-hdmi",
> .data = &rk3399_hdmi_drv_data
> },
> + { .compatible = "rockchip,rk3568-dw-hdmi",
> + .data = &rk3568_hdmi_drv_data
> + },
> {},
> };
> MODULE_DEVICE_TABLE(of, dw_hdmi_rockchip_dt_ids);
> @@ -536,6 +578,28 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
> return ret;
> }
>
> + ret = clk_prepare_enable(hdmi->hclk_vio);
> + if (ret) {
> + dev_err(hdmi->dev, "Failed to enable HDMI hclk_vio: %d\n",
> + ret);
> + return ret;
> + }
> +
> + ret = clk_prepare_enable(hdmi->hclk_vop);
> + if (ret) {
> + dev_err(hdmi->dev, "Failed to enable HDMI hclk_vop: %d\n",
> + ret);
> + return ret;
> + }
> +
> + if (hdmi->chip_data == &rk3568_chip_data) {
> + regmap_write(hdmi->regmap, RK3568_GRF_VO_CON1,
> + HIWORD_UPDATE(RK3568_HDMI_SDAIN_MSK |
> + RK3568_HDMI_SCLIN_MSK,
> + RK3568_HDMI_SDAIN_MSK |
> + RK3568_HDMI_SCLIN_MSK));
> + }
> +
> hdmi->phy = devm_phy_optional_get(dev, "hdmi");
> if (IS_ERR(hdmi->phy)) {
> ret = PTR_ERR(hdmi->phy);
> @@ -559,6 +623,8 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
> ret = PTR_ERR(hdmi->hdmi);
> drm_encoder_cleanup(encoder);
> clk_disable_unprepare(hdmi->vpll_clk);
> + clk_disable_unprepare(hdmi->hclk_vio);
> + clk_disable_unprepare(hdmi->hclk_vop);
> }
>
> return ret;
> @@ -571,6 +637,8 @@ static void dw_hdmi_rockchip_unbind(struct device *dev, struct device *master,
>
> dw_hdmi_unbind(hdmi->hdmi);
> clk_disable_unprepare(hdmi->vpll_clk);
> + clk_disable_unprepare(hdmi->hclk_vio);
> + clk_disable_unprepare(hdmi->hclk_vop);
> }
>
> static const struct component_ops dw_hdmi_rockchip_ops = {
>
Hi Alex:
On 7/13/21 7:40 PM, Alex Bee wrote:
> Hi Benjamin,
>
> Am 07.07.21 um 14:03 schrieb Benjamin Gaignard:
>> Add a new dw_hdmi_plat_data struct and new compatible for rk3568.
>> This version of the HDMI hardware block need two clocks to provide
>> phy reference clock: hclk_vio and hclk.
>>
>> Signed-off-by: Benjamin Gaignard <[email protected]>
>> ---
>> version 2:
>> - Add the clocks needed for the phy.
>
> If got Alega's comment correct, it wasn't about the hclks.
> It looks like for this variant, there is another reference clock
> required (for the phy) like vpll is already (looks like downstream
> uses HPLL ( = "HDMI-PLL" ?) for that - which also has to switch the
> frequency according the the drm mode rate - the two clocks you added
> here are get just enabled (and disabled) here.
Yes, it's HPLL, and the frequency of HPLL and drm mode rate(vop dclk)
should keep 1:1.
>
> Alega, Andy: Is it really required to enable hclk_vio and hclk(_vop)
> in the hdmi driver? Are they required to be enabled for the other
> output variants (i.e. mipi, dsi, rgb ....) as well and shouldn't
> better be enabled in the (not-yet existing) vop2 driver?
hclk_vop should be enabled, other wise you can't access hdmi registers.
This is only required for HDMI(mipi dis, edp, rgb don't need it)
>
> Overall: I'm not sure of the benefit of adding this hdmi variant for a
> SoC where the display driver isn't implemented upstream yet. The
> "VOP2" IP seems widely new and should probably be ported first. (even
> if the HDMI part seems a low hanging fruit according to the vendor
> sources)
Yes, VOP2 IP is widely totaly new and complicated, I have a plan to do
the upstream. But I am in a rush now, so please give me a lite time????.
>
> Best,
> Alex
>
>>
>> drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 68 +++++++++++++++++++++
>> 1 file changed, 68 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>> b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>> index 830bdd5e9b7ce..dc0e255e45745 100644
>> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>> @@ -50,6 +50,10 @@
>> #define RK3399_GRF_SOC_CON20 0x6250
>> #define RK3399_HDMI_LCDC_SEL BIT(6)
>> +#define RK3568_GRF_VO_CON1 0x0364
>> +#define RK3568_HDMI_SDAIN_MSK BIT(15)
>> +#define RK3568_HDMI_SCLIN_MSK BIT(14)
>> +
>> #define HIWORD_UPDATE(val, mask) (val | (mask) << 16)
>> /**
>> @@ -71,6 +75,8 @@ struct rockchip_hdmi {
>> const struct rockchip_hdmi_chip_data *chip_data;
>> struct clk *vpll_clk;
>> struct clk *grf_clk;
>> + struct clk *hclk_vio;
>> + struct clk *hclk_vop;
>> struct dw_hdmi *hdmi;
>> struct phy *phy;
>> };
>> @@ -216,6 +222,26 @@ static int rockchip_hdmi_parse_dt(struct
>> rockchip_hdmi *hdmi)
>> return PTR_ERR(hdmi->grf_clk);
>> }
>> + hdmi->hclk_vio = devm_clk_get(hdmi->dev, "hclk_vio");
>> + if (PTR_ERR(hdmi->hclk_vio) == -ENOENT) {
>> + hdmi->hclk_vio = NULL;
>> + } else if (PTR_ERR(hdmi->hclk_vio) == -EPROBE_DEFER) {
>> + return -EPROBE_DEFER;
>> + } else if (IS_ERR(hdmi->hclk_vio)) {
>> + dev_err(hdmi->dev, "failed to get hclk_vio clock\n");
>> + return PTR_ERR(hdmi->hclk_vio);
>> + }
>> +
>> + hdmi->hclk_vop = devm_clk_get(hdmi->dev, "hclk");
>> + if (PTR_ERR(hdmi->hclk_vop) == -ENOENT) {
>> + hdmi->hclk_vop = NULL;
>> + } else if (PTR_ERR(hdmi->hclk_vop) == -EPROBE_DEFER) {
>> + return -EPROBE_DEFER;
>> + } else if (IS_ERR(hdmi->hclk_vop)) {
>> + dev_err(hdmi->dev, "failed to get hclk_vop clock\n");
>> + return PTR_ERR(hdmi->hclk_vop);
>> + }
>> +
>> return 0;
>> }
>> @@ -467,6 +493,19 @@ static const struct dw_hdmi_plat_data
>> rk3399_hdmi_drv_data = {
>> .use_drm_infoframe = true,
>> };
>> +static struct rockchip_hdmi_chip_data rk3568_chip_data = {
>> + .lcdsel_grf_reg = -1,
>> +};
>> +
>> +static const struct dw_hdmi_plat_data rk3568_hdmi_drv_data = {
>> + .mode_valid = dw_hdmi_rockchip_mode_valid,
>> + .mpll_cfg = rockchip_mpll_cfg,
>> + .cur_ctr = rockchip_cur_ctr,
>> + .phy_config = rockchip_phy_config,
>> + .phy_data = &rk3568_chip_data,
>> + .use_drm_infoframe = true,
>> +};
>> +
>> static const struct of_device_id dw_hdmi_rockchip_dt_ids[] = {
>> { .compatible = "rockchip,rk3228-dw-hdmi",
>> .data = &rk3228_hdmi_drv_data
>> @@ -480,6 +519,9 @@ static const struct of_device_id
>> dw_hdmi_rockchip_dt_ids[] = {
>> { .compatible = "rockchip,rk3399-dw-hdmi",
>> .data = &rk3399_hdmi_drv_data
>> },
>> + { .compatible = "rockchip,rk3568-dw-hdmi",
>> + .data = &rk3568_hdmi_drv_data
>> + },
>> {},
>> };
>> MODULE_DEVICE_TABLE(of, dw_hdmi_rockchip_dt_ids);
>> @@ -536,6 +578,28 @@ static int dw_hdmi_rockchip_bind(struct device
>> *dev, struct device *master,
>> return ret;
>> }
>> + ret = clk_prepare_enable(hdmi->hclk_vio);
>> + if (ret) {
>> + dev_err(hdmi->dev, "Failed to enable HDMI hclk_vio: %d\n",
>> + ret);
>> + return ret;
>> + }
>> +
>> + ret = clk_prepare_enable(hdmi->hclk_vop);
>> + if (ret) {
>> + dev_err(hdmi->dev, "Failed to enable HDMI hclk_vop: %d\n",
>> + ret);
>> + return ret;
>> + }
>> +
>> + if (hdmi->chip_data == &rk3568_chip_data) {
>> + regmap_write(hdmi->regmap, RK3568_GRF_VO_CON1,
>> + HIWORD_UPDATE(RK3568_HDMI_SDAIN_MSK |
>> + RK3568_HDMI_SCLIN_MSK,
>> + RK3568_HDMI_SDAIN_MSK |
>> + RK3568_HDMI_SCLIN_MSK));
>> + }
>> +
>> hdmi->phy = devm_phy_optional_get(dev, "hdmi");
>> if (IS_ERR(hdmi->phy)) {
>> ret = PTR_ERR(hdmi->phy);
>> @@ -559,6 +623,8 @@ static int dw_hdmi_rockchip_bind(struct device
>> *dev, struct device *master,
>> ret = PTR_ERR(hdmi->hdmi);
>> drm_encoder_cleanup(encoder);
>> clk_disable_unprepare(hdmi->vpll_clk);
>> + clk_disable_unprepare(hdmi->hclk_vio);
>> + clk_disable_unprepare(hdmi->hclk_vop);
>> }
>> return ret;
>> @@ -571,6 +637,8 @@ static void dw_hdmi_rockchip_unbind(struct device
>> *dev, struct device *master,
>> dw_hdmi_unbind(hdmi->hdmi);
>> clk_disable_unprepare(hdmi->vpll_clk);
>> + clk_disable_unprepare(hdmi->hclk_vio);
>> + clk_disable_unprepare(hdmi->hclk_vop);
>> }
>> static const struct component_ops dw_hdmi_rockchip_ops = {
>>
>
>
>
>
Hello Heiko,
On 7/13/21 10:49 AM, Heiko Stübner wrote:
> Hi Michael,
>
> Am Dienstag, 13. Juli 2021, 10:44:00 CEST schrieb Michael Riesch:
>> The HDMI TX block in the RK3568 requires two power supplies, which have
>> to be enabled in some cases (at least on the RK3568 EVB1 the voltages
>> VDDA0V9_IMAGE and VCCA1V8_IMAGE are disabled by default). It would be
>> great if this was considered by the driver and the device tree binding.
>> I am not sure, though, whether this is a RK3568 specific or
>> rockchip_dw_hdmi specific thing. Maybe it can even enter the Synopsis DW
>> HDMI driver.
>
> I do remember that this discussion happened many years back already.
> And yes the supplies are needed for all but back then there was opposition
> as these are supposedly phy-related supplies, not for the dw-hdmi itself.
> [There are variants with an external phy, like on the rk3328]
>
> See discussion on [0]
>
> [0] https://dri-devel.freedesktop.narkive.com/pen2zWo1/patch-v3-1-2-drm-bridge-dw-hdmi-support-optional-supply-regulators
Thanks for the pointer. My summary of this discussion would be the
following:
- There was no consensus on how to handle the issue. The voltages still
have to be enabled from the outside of the driver.
- Open question: rockchip-specific or general solution? (one may detect
a tendency towards a rockchip-specific solution)
- Open question: separation of the phy from the dw_hdmi IP core?
First of all, IMHO the driver should enable those voltages, otherwise we
will have the same discussion again in 5-6 years :-)
Then, the rockchip,dw-hdmi binding features a property "phys",
presumably to handle external phys (e.g., for the RK3328). This fact and
the referenced discussion suggest a rockchip-specific solution.
In the Rockchip documentation (at least for RK3328, RK3399 and RK3568),
there are two extra voltages denoted as "HDMI PHY analog power". It
would be tempting to add the internal phy to the device tree and glue it
to the dw-hdmi using the "phys" property. However, as pointed out in the
referenced discussion, the configuration registers of the phy are
somewhat interleaved with the dw-hdmi registers and a clear separation
may be tricky.
As a more pragmatic alternative, we could add optional supplies to the
rockchip,dw-hdmi binding and evaluate the "phys" property. If the latter
is not specified, the internal phy is used and the supplies must be
enabled. Would such an approach be acceptable?
Best regards,
Michael
>> On 7/7/21 2:03 PM, Benjamin Gaignard wrote:
>>> Define a new compatible for rk3568 HDMI.
>>> This version of HDMI hardware block needs two new clocks hclk_vio and hclk
>>> to provide phy reference clocks.
>>>
>>> Signed-off-by: Benjamin Gaignard <[email protected]>
>>> ---
>>> version 2:
>>> - Add the clocks needed for the phy.
>>>
>>> .../bindings/display/rockchip/rockchip,dw-hdmi.yaml | 6 +++++-
>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
>>> index 75cd9c686e985..cb8643b3a8b84 100644
>>> --- a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
>>> +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
>>> @@ -23,6 +23,7 @@ properties:
>>> - rockchip,rk3288-dw-hdmi
>>> - rockchip,rk3328-dw-hdmi
>>> - rockchip,rk3399-dw-hdmi
>>> + - rockchip,rk3568-dw-hdmi
>>>
>>> reg-io-width:
>>> const: 4
>>> @@ -51,8 +52,11 @@ properties:
>>> - vpll
>>> - enum:
>>> - grf
>>> + - hclk_vio
>>> + - vpll
>>> + - enum:
>>> + - hclk
>>> - vpll
>>> - - const: vpll
>>
>> The description and documentation of the clocks are somewhat misleading
>> IMHO. This is not caused by your patches, of course. But maybe this is a
>> chance to clean them up a bit.
>>
>> It seems that the CEC clock is an optional clock of the dw-hdmi driver.
>> Shouldn't it be documented in the synopsys,dw-hdmi.yaml?
>>
>> Also, it would be nice if the clocks hclk_vio and hclk featured a
>> description in the binding.
>>
>> BTW, I am not too familiar with the syntax here, but shouldn't items in
>> clocks and items in clock-names be aligned (currently, there is a plain
>> list vs. an enum structure)?
>>
>> Best regards,
>> Michael
>>
>>>
>>> ddc-i2c-bus:
>>> $ref: /schemas/types.yaml#/definitions/phandle
>>>
>>
>
>
>
>
On 2021-07-14 10:19, Michael Riesch wrote:
> Hello Heiko,
>
> On 7/13/21 10:49 AM, Heiko Stübner wrote:
>> Hi Michael,
>>
>> Am Dienstag, 13. Juli 2021, 10:44:00 CEST schrieb Michael Riesch:
>>> The HDMI TX block in the RK3568 requires two power supplies, which have
>>> to be enabled in some cases (at least on the RK3568 EVB1 the voltages
>>> VDDA0V9_IMAGE and VCCA1V8_IMAGE are disabled by default). It would be
>>> great if this was considered by the driver and the device tree binding.
>>> I am not sure, though, whether this is a RK3568 specific or
>>> rockchip_dw_hdmi specific thing. Maybe it can even enter the Synopsis DW
>>> HDMI driver.
>>
>> I do remember that this discussion happened many years back already.
>> And yes the supplies are needed for all but back then there was opposition
>> as these are supposedly phy-related supplies, not for the dw-hdmi itself.
>> [There are variants with an external phy, like on the rk3328]
>>
>> See discussion on [0]
>>
>> [0] https://dri-devel.freedesktop.narkive.com/pen2zWo1/patch-v3-1-2-drm-bridge-dw-hdmi-support-optional-supply-regulators
>
> Thanks for the pointer. My summary of this discussion would be the
> following:
>
> - There was no consensus on how to handle the issue. The voltages still
> have to be enabled from the outside of the driver.
> - Open question: rockchip-specific or general solution? (one may detect
> a tendency towards a rockchip-specific solution)
> - Open question: separation of the phy from the dw_hdmi IP core?
>
> First of all, IMHO the driver should enable those voltages, otherwise we
> will have the same discussion again in 5-6 years :-)
>
> Then, the rockchip,dw-hdmi binding features a property "phys",
> presumably to handle external phys (e.g., for the RK3328). This fact and
> the referenced discussion suggest a rockchip-specific solution.
FWIW I've long thought that cleaning up the phy situation in dw-hdmi
would be a good idea. It's always seemed a bit sketchy that on RK3328 we
still validate modes against the tables for the Synopsys phy which isn't
relevant, and if that does allow a clock rate through that the actual
phy rejects then things just go horribly wrong and the display breaks.
> In the Rockchip documentation (at least for RK3328, RK3399 and RK3568),
> there are two extra voltages denoted as "HDMI PHY analog power". It
> would be tempting to add the internal phy to the device tree and glue it
> to the dw-hdmi using the "phys" property. However, as pointed out in the
> referenced discussion, the configuration registers of the phy are
> somewhat interleaved with the dw-hdmi registers and a clear separation
> may be tricky.
Conceptually I don't think there's any issue with the HDMI node being
its own phy provider where appropriate. At the DT level it should simply
be a case of having both sets of properties, e.g.:
&hdmi {
#phy-cells = <0>;
phys = <&hdmi>;
};
And at the driver level AFAICS it's pretty much just a case of dw-hdmi
additionally registering itself as a phy provider if the internal phy is
present - the only difference then should be that it can end up calling
back into itself via the common phy API rather than directly via
internal special-cases.
Robin.
> As a more pragmatic alternative, we could add optional supplies to the
> rockchip,dw-hdmi binding and evaluate the "phys" property. If the latter
> is not specified, the internal phy is used and the supplies must be
> enabled. Would such an approach be acceptable?
>
> Best regards,
> Michael
>
>>> On 7/7/21 2:03 PM, Benjamin Gaignard wrote:
>>>> Define a new compatible for rk3568 HDMI.
>>>> This version of HDMI hardware block needs two new clocks hclk_vio and hclk
>>>> to provide phy reference clocks.
>>>>
>>>> Signed-off-by: Benjamin Gaignard <[email protected]>
>>>> ---
>>>> version 2:
>>>> - Add the clocks needed for the phy.
>>>>
>>>> .../bindings/display/rockchip/rockchip,dw-hdmi.yaml | 6 +++++-
>>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
>>>> index 75cd9c686e985..cb8643b3a8b84 100644
>>>> --- a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
>>>> +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
>>>> @@ -23,6 +23,7 @@ properties:
>>>> - rockchip,rk3288-dw-hdmi
>>>> - rockchip,rk3328-dw-hdmi
>>>> - rockchip,rk3399-dw-hdmi
>>>> + - rockchip,rk3568-dw-hdmi
>>>>
>>>> reg-io-width:
>>>> const: 4
>>>> @@ -51,8 +52,11 @@ properties:
>>>> - vpll
>>>> - enum:
>>>> - grf
>>>> + - hclk_vio
>>>> + - vpll
>>>> + - enum:
>>>> + - hclk
>>>> - vpll
>>>> - - const: vpll
>>>
>>> The description and documentation of the clocks are somewhat misleading
>>> IMHO. This is not caused by your patches, of course. But maybe this is a
>>> chance to clean them up a bit.
>>>
>>> It seems that the CEC clock is an optional clock of the dw-hdmi driver.
>>> Shouldn't it be documented in the synopsys,dw-hdmi.yaml?
>>>
>>> Also, it would be nice if the clocks hclk_vio and hclk featured a
>>> description in the binding.
>>>
>>> BTW, I am not too familiar with the syntax here, but shouldn't items in
>>> clocks and items in clock-names be aligned (currently, there is a plain
>>> list vs. an enum structure)?
>>>
>>> Best regards,
>>> Michael
>>>
>>>>
>>>> ddc-i2c-bus:
>>>> $ref: /schemas/types.yaml#/definitions/phandle
>>>>
>>>
>>
>>
>>
>>
>
> _______________________________________________
> Linux-rockchip mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>