Hi all,
This series adds support for the LVDS controller on the Rockchip RK3566 and
RK3568. First patch adds the support in rockchip-lvds.c driver, setting all
the needed GRF registers. Second patch adds device tree bindings, and third
patch adds a note in the documentation.
LVDS controller on rk356x does share DSI DPHY with MIPI DSI, and all
groundwork on enabling it is done by Chris Morgan.
Tested on Autogramma Monitor RockChip, custom board based on Radxa Rock 3
Computing Module Plus.
Alibek Omarov (3):
drm/rockchip: lvds: add rk3568 support
arm64: dts: rockchip: rk356x: add LVDS bindings
dt-bindings: display: rockchip-lvds: add compatible string for RK3568
.../display/rockchip/rockchip-lvds.txt | 1 +
arch/arm64/boot/dts/rockchip/rk356x.dtsi | 25 +++
drivers/gpu/drm/rockchip/rockchip_lvds.c | 144 +++++++++++++++++-
drivers/gpu/drm/rockchip/rockchip_lvds.h | 10 ++
4 files changed, 173 insertions(+), 7 deletions(-)
--
2.34.1
One of the ports of RK3568 can be configured as LVDS, re-using the DSI DPHY
Signed-off-by: Alibek Omarov <[email protected]>
---
drivers/gpu/drm/rockchip/rockchip_lvds.c | 144 +++++++++++++++++++++--
drivers/gpu/drm/rockchip/rockchip_lvds.h | 10 ++
2 files changed, 147 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
index 68f6ebb33460..83c60240af85 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
@@ -433,6 +433,90 @@ static void px30_lvds_encoder_disable(struct drm_encoder *encoder)
drm_panel_unprepare(lvds->panel);
}
+static int rk3568_lvds_poweron(struct rockchip_lvds *lvds)
+{
+ int ret;
+
+ ret = clk_enable(lvds->pclk);
+ if (ret < 0) {
+ DRM_DEV_ERROR(lvds->dev, "failed to enable lvds pclk %d\n", ret);
+ return ret;
+ }
+
+ ret = pm_runtime_get_sync(lvds->dev);
+ if (ret < 0) {
+ DRM_DEV_ERROR(lvds->dev, "failed to get pm runtime: %d\n", ret);
+ clk_disable(lvds->pclk);
+ return ret;
+ }
+
+ /* Enable LVDS mode */
+ return regmap_update_bits(lvds->grf, RK3568_GRF_VO_CON2,
+ RK3568_LVDS0_MODE_EN(1),
+ RK3568_LVDS0_MODE_EN(1));
+}
+
+static void rk3568_lvds_poweroff(struct rockchip_lvds *lvds)
+{
+ regmap_update_bits(lvds->grf, RK3568_GRF_VO_CON2,
+ RK3568_LVDS0_MODE_EN(1) | RK3568_LVDS0_P2S_EN(1),
+ RK3568_LVDS0_MODE_EN(0) | RK3568_LVDS0_P2S_EN(0));
+
+ pm_runtime_put(lvds->dev);
+ clk_disable(lvds->pclk);
+}
+
+static int rk3568_lvds_grf_config(struct drm_encoder *encoder,
+ struct drm_display_mode *mode)
+{
+ struct rockchip_lvds *lvds = encoder_to_lvds(encoder);
+
+ if (lvds->output != DISPLAY_OUTPUT_LVDS) {
+ DRM_DEV_ERROR(lvds->dev, "Unsupported display output %d\n",
+ lvds->output);
+ return -EINVAL;
+ }
+
+ /* Set format */
+ return regmap_update_bits(lvds->grf, RK3568_GRF_VO_CON0,
+ RK3568_LVDS0_SELECT(3),
+ RK3568_LVDS0_SELECT(lvds->format));
+}
+
+static void rk3568_lvds_encoder_enable(struct drm_encoder *encoder)
+{
+ struct rockchip_lvds *lvds = encoder_to_lvds(encoder);
+ struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode;
+ int ret;
+
+ drm_panel_prepare(lvds->panel);
+
+ ret = rk3568_lvds_poweron(lvds);
+ if (ret) {
+ DRM_DEV_ERROR(lvds->dev, "failed to power on LVDS: %d\n", ret);
+ drm_panel_unprepare(lvds->panel);
+ return;
+ }
+
+ ret = rk3568_lvds_grf_config(encoder, mode);
+ if (ret) {
+ DRM_DEV_ERROR(lvds->dev, "failed to configure LVDS: %d\n", ret);
+ drm_panel_unprepare(lvds->panel);
+ return;
+ }
+
+ drm_panel_enable(lvds->panel);
+}
+
+static void rk3568_lvds_encoder_disable(struct drm_encoder *encoder)
+{
+ struct rockchip_lvds *lvds = encoder_to_lvds(encoder);
+
+ drm_panel_disable(lvds->panel);
+ rk3568_lvds_poweroff(lvds);
+ drm_panel_unprepare(lvds->panel);
+}
+
static const
struct drm_encoder_helper_funcs rk3288_lvds_encoder_helper_funcs = {
.enable = rk3288_lvds_encoder_enable,
@@ -447,6 +531,13 @@ struct drm_encoder_helper_funcs px30_lvds_encoder_helper_funcs = {
.atomic_check = rockchip_lvds_encoder_atomic_check,
};
+static const
+struct drm_encoder_helper_funcs rk3568_lvds_encoder_helper_funcs = {
+ .enable = rk3568_lvds_encoder_enable,
+ .disable = rk3568_lvds_encoder_disable,
+ .atomic_check = rockchip_lvds_encoder_atomic_check,
+};
+
static int rk3288_lvds_probe(struct platform_device *pdev,
struct rockchip_lvds *lvds)
{
@@ -491,6 +582,26 @@ static int rk3288_lvds_probe(struct platform_device *pdev,
return 0;
}
+static int rockchip_lvds_phy_probe(struct platform_device *pdev,
+ struct rockchip_lvds *lvds)
+{
+ int ret;
+
+ lvds->dphy = devm_phy_get(&pdev->dev, "dphy");
+ if (IS_ERR(lvds->dphy))
+ return PTR_ERR(lvds->dphy);
+
+ ret = phy_init(lvds->dphy);
+ if (ret)
+ return ret;
+
+ ret = phy_set_mode(lvds->dphy, PHY_MODE_LVDS);
+ if (ret)
+ return ret;
+
+ return phy_power_on(lvds->dphy);
+}
+
static int px30_lvds_probe(struct platform_device *pdev,
struct rockchip_lvds *lvds)
{
@@ -503,20 +614,28 @@ static int px30_lvds_probe(struct platform_device *pdev,
if (ret)
return ret;
- /* PHY */
- lvds->dphy = devm_phy_get(&pdev->dev, "dphy");
- if (IS_ERR(lvds->dphy))
- return PTR_ERR(lvds->dphy);
+ return rockchip_lvds_phy_probe(pdev, lvds);
+}
- ret = phy_init(lvds->dphy);
+static int rk3568_lvds_probe(struct platform_device *pdev,
+ struct rockchip_lvds *lvds)
+{
+ int ret;
+
+ ret = regmap_update_bits(lvds->grf, RK3568_GRF_VO_CON0,
+ RK3568_LVDS0_MSBSEL(1),
+ RK3568_LVDS0_MSBSEL(1));
if (ret)
return ret;
- ret = phy_set_mode(lvds->dphy, PHY_MODE_LVDS);
+ ret = regmap_update_bits(lvds->grf, RK3568_GRF_VO_CON2,
+ RK3568_LVDS0_P2S_EN(1),
+ RK3568_LVDS0_P2S_EN(1));
+
if (ret)
return ret;
- return phy_power_on(lvds->dphy);
+ return rockchip_lvds_phy_probe(pdev, lvds);
}
static const struct rockchip_lvds_soc_data rk3288_lvds_data = {
@@ -529,6 +648,11 @@ static const struct rockchip_lvds_soc_data px30_lvds_data = {
.helper_funcs = &px30_lvds_encoder_helper_funcs,
};
+static const struct rockchip_lvds_soc_data rk3568_lvds_data = {
+ .probe = rk3568_lvds_probe,
+ .helper_funcs = &rk3568_lvds_encoder_helper_funcs,
+};
+
static const struct of_device_id rockchip_lvds_dt_ids[] = {
{
.compatible = "rockchip,rk3288-lvds",
@@ -538,6 +662,10 @@ static const struct of_device_id rockchip_lvds_dt_ids[] = {
.compatible = "rockchip,px30-lvds",
.data = &px30_lvds_data
},
+ {
+ .compatible = "rockchip,rk3568-lvds",
+ .data = &rk3568_lvds_data
+ },
{}
};
MODULE_DEVICE_TABLE(of, rockchip_lvds_dt_ids);
@@ -612,6 +740,8 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
encoder = &lvds->encoder.encoder;
encoder->possible_crtcs = drm_of_find_possible_crtcs(drm_dev,
dev->of_node);
+ rockchip_drm_encoder_set_crtc_endpoint_id(&lvds->encoder,
+ dev->of_node, 0, 0);
ret = drm_simple_encoder_init(drm_dev, encoder, DRM_MODE_ENCODER_LVDS);
if (ret < 0) {
diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.h b/drivers/gpu/drm/rockchip/rockchip_lvds.h
index 4ce967d23813..57decb33f779 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.h
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.h
@@ -120,4 +120,14 @@
#define PX30_LVDS_P2S_EN(val) HIWORD_UPDATE(val, 6, 6)
#define PX30_LVDS_VOP_SEL(val) HIWORD_UPDATE(val, 1, 1)
+#define RK3568_GRF_VO_CON0 0x0360
+#define RK3568_LVDS0_SELECT(val) HIWORD_UPDATE(val, 5, 4)
+#define RK3568_LVDS0_MSBSEL(val) HIWORD_UPDATE(val, 3, 3)
+
+#define RK3568_GRF_VO_CON2 0x0368
+#define RK3568_LVDS0_DCLK_INV_SEL(val) HIWORD_UPDATE(val, 9, 9)
+#define RK3568_LVDS0_DCLK_DIV2_SEL(val) HIWORD_UPDATE(val, 8, 8)
+#define RK3568_LVDS0_MODE_EN(val) HIWORD_UPDATE(val, 1, 1)
+#define RK3568_LVDS0_P2S_EN(val) HIWORD_UPDATE(val, 0, 0)
+
#endif /* _ROCKCHIP_LVDS_ */
--
2.34.1
On Thu, Jan 19, 2023 at 09:48:03PM +0300, Alibek Omarov wrote:
> One of the ports of RK3568 can be configured as LVDS, re-using the DSI DPHY
>
> Signed-off-by: Alibek Omarov <[email protected]>
> ---
> drivers/gpu/drm/rockchip/rockchip_lvds.c | 144 +++++++++++++++++++++--
> drivers/gpu/drm/rockchip/rockchip_lvds.h | 10 ++
> 2 files changed, 147 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> index 68f6ebb33460..83c60240af85 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> @@ -433,6 +433,90 @@ static void px30_lvds_encoder_disable(struct drm_encoder *encoder)
> drm_panel_unprepare(lvds->panel);
> }
>
> +static int rk3568_lvds_poweron(struct rockchip_lvds *lvds)
> +{
> + int ret;
> +
> + ret = clk_enable(lvds->pclk);
> + if (ret < 0) {
> + DRM_DEV_ERROR(lvds->dev, "failed to enable lvds pclk %d\n", ret);
> + return ret;
> + }
> +
> + ret = pm_runtime_get_sync(lvds->dev);
> + if (ret < 0) {
> + DRM_DEV_ERROR(lvds->dev, "failed to get pm runtime: %d\n", ret);
> + clk_disable(lvds->pclk);
> + return ret;
> + }
> +
> + /* Enable LVDS mode */
> + return regmap_update_bits(lvds->grf, RK3568_GRF_VO_CON2,
> + RK3568_LVDS0_MODE_EN(1),
> + RK3568_LVDS0_MODE_EN(1));
Isn't this the same as:
regmap_write(lvds->grf, RK3568_GRF_VO_CON2, RK3568_LVDS0_MODE_EN(1));
Unless I am missing something I find a plain regmap_write() easier to
read.
> +}
> +
> +static void rk3568_lvds_poweroff(struct rockchip_lvds *lvds)
> +{
> + regmap_update_bits(lvds->grf, RK3568_GRF_VO_CON2,
> + RK3568_LVDS0_MODE_EN(1) | RK3568_LVDS0_P2S_EN(1),
> + RK3568_LVDS0_MODE_EN(0) | RK3568_LVDS0_P2S_EN(0));
Same here:
regmap_write(lvds->grf, RK3568_GRF_VO_CON2,
RK3568_LVDS0_MODE_EN(0) | RK3568_LVDS0_P2S_EN(0));
What about the RK3568_LVDS0_P2S_EN bit? This is set in probe() and
cleared here. For symmetry reasons shouldn't it be set in
rk3568_lvds_poweron() instead?
> +
> + pm_runtime_put(lvds->dev);
> + clk_disable(lvds->pclk);
> +}
> +
> +static int rk3568_lvds_grf_config(struct drm_encoder *encoder,
> + struct drm_display_mode *mode)
> +{
> + struct rockchip_lvds *lvds = encoder_to_lvds(encoder);
> +
> + if (lvds->output != DISPLAY_OUTPUT_LVDS) {
> + DRM_DEV_ERROR(lvds->dev, "Unsupported display output %d\n",
> + lvds->output);
> + return -EINVAL;
> + }
> +
> + /* Set format */
> + return regmap_update_bits(lvds->grf, RK3568_GRF_VO_CON0,
> + RK3568_LVDS0_SELECT(3),
> + RK3568_LVDS0_SELECT(lvds->format));
It seems lvds->format does not match what the register expects. We
have:
#define LVDS_VESA_24 0
#define LVDS_JEIDA_24 1
#define LVDS_VESA_18 2
#define LVDS_JEIDA_18 3
According to the reference manual the register expects:
lvdsformat_lvds0_select
2'b00: VESA 24bit
2'b01: JEIDA 24bit
2'b10: JEIDA 18bit
2'b11: VESA 18bit
I only have the RK3568 manual but no PX30 or RK3288 manual, so I can't say if
they changed the register mapping between the SoCs or if it's wrong on
the other SoCs as well.
BTW you correctly set the mask to RK3568_LVDS0_SELECT(3), but for the
PX30 it looks wrong:
return regmap_update_bits(lvds->grf, PX30_LVDS_GRF_PD_VO_CON1,
PX30_LVDS_FORMAT(lvds->format),
PX30_LVDS_FORMAT(lvds->format));
I really think regmap_write() would be better to use here to avoid such
things.
> +
> +static void rk3568_lvds_encoder_enable(struct drm_encoder *encoder)
> +{
> + struct rockchip_lvds *lvds = encoder_to_lvds(encoder);
> + struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode;
'mode' is unused.
> + int ret;
> +
> + drm_panel_prepare(lvds->panel);
> +
> + ret = rk3568_lvds_poweron(lvds);
> + if (ret) {
> + DRM_DEV_ERROR(lvds->dev, "failed to power on LVDS: %d\n", ret);
> + drm_panel_unprepare(lvds->panel);
> + return;
> + }
> +
> + ret = rk3568_lvds_grf_config(encoder, mode);
> + if (ret) {
> + DRM_DEV_ERROR(lvds->dev, "failed to configure LVDS: %d\n", ret);
> + drm_panel_unprepare(lvds->panel);
> + return;
> + }
> +
> + drm_panel_enable(lvds->panel);
> +}
> +
> +static void rk3568_lvds_encoder_disable(struct drm_encoder *encoder)
> +{
> + struct rockchip_lvds *lvds = encoder_to_lvds(encoder);
> +
> + drm_panel_disable(lvds->panel);
> + rk3568_lvds_poweroff(lvds);
> + drm_panel_unprepare(lvds->panel);
> +}
> +
> static const
> struct drm_encoder_helper_funcs rk3288_lvds_encoder_helper_funcs = {
> .enable = rk3288_lvds_encoder_enable,
> @@ -447,6 +531,13 @@ struct drm_encoder_helper_funcs px30_lvds_encoder_helper_funcs = {
> .atomic_check = rockchip_lvds_encoder_atomic_check,
> };
>
> +static const
> +struct drm_encoder_helper_funcs rk3568_lvds_encoder_helper_funcs = {
> + .enable = rk3568_lvds_encoder_enable,
> + .disable = rk3568_lvds_encoder_disable,
> + .atomic_check = rockchip_lvds_encoder_atomic_check,
> +};
> +
> static int rk3288_lvds_probe(struct platform_device *pdev,
> struct rockchip_lvds *lvds)
> {
> @@ -491,6 +582,26 @@ static int rk3288_lvds_probe(struct platform_device *pdev,
> return 0;
> }
>
> +static int rockchip_lvds_phy_probe(struct platform_device *pdev,
> + struct rockchip_lvds *lvds)
> +{
> + int ret;
> +
> + lvds->dphy = devm_phy_get(&pdev->dev, "dphy");
> + if (IS_ERR(lvds->dphy))
> + return PTR_ERR(lvds->dphy);
> +
> + ret = phy_init(lvds->dphy);
> + if (ret)
> + return ret;
> +
> + ret = phy_set_mode(lvds->dphy, PHY_MODE_LVDS);
> + if (ret)
> + return ret;
> +
> + return phy_power_on(lvds->dphy);
> +}
You factor out the steps done for px30 to a separate function in order
to reuse it on rk3568. You could make a separate patch from this to
make it easier to understand and to verify that there is no functional
change involved for the px30.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Hello Sascha,
On Fri, 2023-01-20 at 10:16 +0100, Sascha Hauer wrote:
> > + /* Enable LVDS mode */
> > + return regmap_update_bits(lvds->grf, RK3568_GRF_VO_CON2,
> > + RK3568_LVDS0_MODE_EN(1),
> > + RK3568_LVDS0_MODE_EN(1));
>
> Isn't this the same as:
>
> regmap_write(lvds->grf, RK3568_GRF_VO_CON2,
> RK3568_LVDS0_MODE_EN(1));
>
> Unless I am missing something I find a plain regmap_write() easier to
> read.
the former is setting a bit in a RMW operation, the latter is a plain
write, isn't it?
--
Alexander Sverdlin
Siemens AG
http://www.siemens.com
On Fri, Jan 20, 2023 at 09:31:43AM +0000, Sverdlin, Alexander wrote:
> Hello Sascha,
>
> On Fri, 2023-01-20 at 10:16 +0100, Sascha Hauer wrote:
> > > +???????/* Enable LVDS mode */
> > > +???????return regmap_update_bits(lvds->grf, RK3568_GRF_VO_CON2,
> > > +???????????????????????????????? RK3568_LVDS0_MODE_EN(1),
> > > +???????????????????????????????? RK3568_LVDS0_MODE_EN(1));
> >
> > Isn't this the same as:
> >
> > ????????regmap_write(lvds->grf, RK3568_GRF_VO_CON2,
> > RK3568_LVDS0_MODE_EN(1));
> >
> > Unless I am missing something I find a plain regmap_write() easier to
> > read.
>
> the former is setting a bit in a RMW operation, the latter is a plain
> write, isn't it?
That's right from the view what the function itself does. Note the
registers that are accessed here are a bit special. They effectively
are 16bit wide, the upper 16bit contain a mask. Only the bits set in the
mask are actually modified in the lower bits. See the register bit
definitions:
#define HIWORD_UPDATE(v, h, l) ((GENMASK(h, l) << 16) | ((v) << (l)))
#define RK3568_LVDS0_SELECT(val) HIWORD_UPDATE(val, 5, 4)
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |