| Message ID | 20241216031225.3746-4-damon.ding@rock-chips.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add eDP support for RK3588 | expand |
On Mon, Dec 16, 2024 at 11:12:17AM +0800, Damon Ding wrote: > RK3588 integrates the analogix eDP 1.3 TX controller IP and the HDMI/eDP > TX Combo PHY based on a Samsung IP block, and there are also two > independent eDP display interface on RK3588 Soc. > > Add just the basic support for now, i.e. RGB output up to 4K@60Hz, without > the tests of audio, PSR and other eDP 1.3 specific features. > > Signed-off-by: Damon Ding <damon.ding@rock-chips.com> > > --- > > Changes in v2: > - Add support for the other eDP output edp1 > --- > .../gpu/drm/rockchip/analogix_dp-rockchip.c | 82 ++++++++++++++++--- > include/drm/bridge/analogix_dp.h | 3 +- > 2 files changed, 74 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c > index 871606a31ef1..4c9a55776ada 100644 > --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c > +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c > @@ -51,10 +51,12 @@ struct rockchip_grf_reg_field { > /** > * struct rockchip_dp_chip_data - splite the grf setting of kind of chips > * @lcdc_sel: grf register field of lcdc_sel > + * @edp_mode: grf register field of edp_mode > * @chip_type: specific chip type > */ > struct rockchip_dp_chip_data { > const struct rockchip_grf_reg_field lcdc_sel; > + const struct rockchip_grf_reg_field edp_mode; > u32 chip_type; > }; > > @@ -134,12 +136,21 @@ static int rockchip_dp_poweron(struct analogix_dp_plat_data *plat_data) > return ret; > } > > + ret = rockchip_grf_field_write(dp->grf, &dp->data->edp_mode, 1); > + if (ret != 0) > + DRM_DEV_ERROR(dp->dev, "failed to set edp mode %d\n", ret); Is it to be called for non-eDP hosts too? Or for older hosts? > + > return ret; > } > > static int rockchip_dp_powerdown(struct analogix_dp_plat_data *plat_data) > { > struct rockchip_dp_device *dp = pdata_encoder_to_dp(plat_data); > + int ret; > + > + ret = rockchip_grf_field_write(dp->grf, &dp->data->edp_mode, 0); > + if (ret != 0) > + DRM_DEV_ERROR(dp->dev, "failed to set edp mode %d\n", ret); Same question. > > clk_disable_unprepare(dp->pclk); > > @@ -203,6 +214,10 @@ static void rockchip_dp_drm_encoder_enable(struct drm_encoder *encoder, > struct rockchip_dp_device *dp = encoder_to_dp(encoder); > struct drm_crtc *crtc; > struct drm_crtc_state *old_crtc_state; > + struct of_endpoint endpoint; > + struct device_node *remote_port, *remote_port_parent; > + char name[32]; > + u32 port_id; > int ret; > > crtc = rockchip_dp_drm_get_new_crtc(encoder, state); > @@ -220,13 +235,27 @@ static void rockchip_dp_drm_encoder_enable(struct drm_encoder *encoder, > return; > } > > - ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, encoder); > + ret = drm_of_encoder_active_endpoint(dp->dev->of_node, encoder, &endpoint); > if (ret < 0) > return; > > - DRM_DEV_DEBUG(dp->dev, "vop %s output to dp\n", (ret) ? "LIT" : "BIG"); > + remote_port_parent = of_graph_get_remote_port_parent(endpoint.local_node); > + if (remote_port_parent) { > + if (of_get_child_by_name(remote_port_parent, "ports")) { > + remote_port = of_graph_get_remote_port(endpoint.local_node); > + of_property_read_u32(remote_port, "reg", &port_id); > + of_node_put(remote_port); > + sprintf(name, "%s vp%d", remote_port_parent->full_name, port_id); > + } else { > + sprintf(name, "%s %s", > + remote_port_parent->full_name, endpoint.id ? "vopl" : "vopb"); > + } > + of_node_put(remote_port_parent); > + > + DRM_DEV_DEBUG(dp->dev, "%s output to edp\n", name); > + } The "output to dp" message got dropped. Is it expected? > > - ret = rockchip_grf_field_write(dp->grf, &dp->data->lcdc_sel, ret); > + ret = rockchip_grf_field_write(dp->grf, &dp->data->lcdc_sel, endpoint.id); > if (ret != 0) > DRM_DEV_ERROR(dp->dev, "Could not write to GRF: %d\n", ret); > > @@ -396,6 +425,7 @@ static int rockchip_dp_probe(struct platform_device *pdev) > const struct rockchip_dp_chip_data *dp_data; > struct drm_panel *panel = NULL; > struct rockchip_dp_device *dp; > + int id, i; > int ret; > > dp_data = of_device_get_match_data(dev); > @@ -410,9 +440,22 @@ static int rockchip_dp_probe(struct platform_device *pdev) > if (!dp) > return -ENOMEM; > > + id = of_alias_get_id(dev->of_node, "edp"); > + if (id < 0) > + id = 0; Alias is not documented. Please check how other platforms handle device -> ID conversion and consider following it (yes, the best option currently known is to hardcode bus addresses into the driver data). > + > + i = 0; > + while (is_rockchip(dp_data[i].chip_type)) > + i++; > + > + if (id >= i) { > + DRM_DEV_ERROR(dev, "invalid edp id: %d\n", id); > + return -ENODEV; > + } Is it applicable to non-eDP case? > + > dp->dev = dev; > dp->adp = ERR_PTR(-ENODEV); > - dp->data = dp_data; > + dp->data = &dp_data[id]; > dp->plat_data.panel = panel; > dp->plat_data.dev_type = dp->data->chip_type; > dp->plat_data.power_on = rockchip_dp_poweron; > @@ -464,19 +507,38 @@ static int rockchip_dp_resume(struct device *dev) > static DEFINE_RUNTIME_DEV_PM_OPS(rockchip_dp_pm_ops, rockchip_dp_suspend, > rockchip_dp_resume, NULL); > > -static const struct rockchip_dp_chip_data rk3399_edp = { > - .lcdc_sel = GRF_REG_FIELD(0x6250, 5, 5), > - .chip_type = RK3399_EDP, > +static const struct rockchip_dp_chip_data rk3399_edp[] = { > + { > + .lcdc_sel = GRF_REG_FIELD(0x6250, 5, 5), > + .chip_type = RK3399_EDP, > + }, > + { /* sentinel */ } > +}; > + > +static const struct rockchip_dp_chip_data rk3288_dp[] = { > + { > + .lcdc_sel = GRF_REG_FIELD(0x025c, 5, 5), > + .chip_type = RK3288_DP, > + }, > + { /* sentinel */ } > }; > > -static const struct rockchip_dp_chip_data rk3288_dp = { > - .lcdc_sel = GRF_REG_FIELD(0x025c, 5, 5), > - .chip_type = RK3288_DP, > +static const struct rockchip_dp_chip_data rk3588_edp[] = { > + { > + .edp_mode = GRF_REG_FIELD(0x0000, 0, 0), > + .chip_type = RK3588_EDP, > + }, > + { > + .edp_mode = GRF_REG_FIELD(0x0004, 0, 0), > + .chip_type = RK3588_EDP, > + }, > + { /* sentinel */ } > }; > > static const struct of_device_id rockchip_dp_dt_ids[] = { > {.compatible = "rockchip,rk3288-dp", .data = &rk3288_dp }, > {.compatible = "rockchip,rk3399-edp", .data = &rk3399_edp }, > + {.compatible = "rockchip,rk3588-edp", .data = &rk3588_edp }, > {} > }; > MODULE_DEVICE_TABLE(of, rockchip_dp_dt_ids); > diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h > index 6002c5666031..54086cb2d97d 100644 > --- a/include/drm/bridge/analogix_dp.h > +++ b/include/drm/bridge/analogix_dp.h > @@ -15,11 +15,12 @@ enum analogix_dp_devtype { > EXYNOS_DP, > RK3288_DP, > RK3399_EDP, > + RK3588_EDP, > }; > > static inline bool is_rockchip(enum analogix_dp_devtype type) > { > - return type == RK3288_DP || type == RK3399_EDP; > + return type == RK3288_DP || type == RK3399_EDP || type == RK3588_EDP; > } > > struct analogix_dp_plat_data { > -- > 2.34.1 >
Am Montag, 16. Dezember 2024, 09:57:41 CET schrieb Dmitry Baryshkov: > On Mon, Dec 16, 2024 at 11:12:17AM +0800, Damon Ding wrote: > > RK3588 integrates the analogix eDP 1.3 TX controller IP and the HDMI/eDP > > TX Combo PHY based on a Samsung IP block, and there are also two > > independent eDP display interface on RK3588 Soc. > > > > Add just the basic support for now, i.e. RGB output up to 4K@60Hz, without > > the tests of audio, PSR and other eDP 1.3 specific features. > > > > Signed-off-by: Damon Ding <damon.ding@rock-chips.com> > > > > --- > > > > Changes in v2: > > - Add support for the other eDP output edp1 > > --- > > .../gpu/drm/rockchip/analogix_dp-rockchip.c | 82 ++++++++++++++++--- > > include/drm/bridge/analogix_dp.h | 3 +- > > 2 files changed, 74 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c > > index 871606a31ef1..4c9a55776ada 100644 > > --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c > > +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c > > @@ -51,10 +51,12 @@ struct rockchip_grf_reg_field { > > /** > > * struct rockchip_dp_chip_data - splite the grf setting of kind of chips > > * @lcdc_sel: grf register field of lcdc_sel > > + * @edp_mode: grf register field of edp_mode > > * @chip_type: specific chip type > > */ > > struct rockchip_dp_chip_data { > > const struct rockchip_grf_reg_field lcdc_sel; > > + const struct rockchip_grf_reg_field edp_mode; > > u32 chip_type; > > }; > > > > @@ -134,12 +136,21 @@ static int rockchip_dp_poweron(struct analogix_dp_plat_data *plat_data) > > return ret; > > } > > > > + ret = rockchip_grf_field_write(dp->grf, &dp->data->edp_mode, 1); > > + if (ret != 0) > > + DRM_DEV_ERROR(dp->dev, "failed to set edp mode %d\n", ret); > > Is it to be called for non-eDP hosts too? Or for older hosts? The implementation of rockchip_grf_field_write (in patch1) seems to take care of checking if that field actually exists and doing nothing if not. I think eDP/DP is more a naming thing, the Analogix controller is called an eDP controller in all documentation things. Even back on rk3288, the Analogix-DP, still is called an eDP controller in documentation, with the only difference being that it does not contain another additional "dedicated" DP controller > > @@ -396,6 +425,7 @@ static int rockchip_dp_probe(struct platform_device *pdev) > > const struct rockchip_dp_chip_data *dp_data; > > struct drm_panel *panel = NULL; > > struct rockchip_dp_device *dp; > > + int id, i; > > int ret; > > > > dp_data = of_device_get_match_data(dev); > > @@ -410,9 +440,22 @@ static int rockchip_dp_probe(struct platform_device *pdev) > > if (!dp) > > return -ENOMEM; > > > > + id = of_alias_get_id(dev->of_node, "edp"); > > + if (id < 0) > > + id = 0; > > Alias is not documented. additionally, aliases should never be used to determine actual device functionionality - e.g. the hw-device-number > Please check how other platforms handle device -> ID conversion and > consider following it (yes, the best option currently known is to > hardcode bus addresses into the driver data). see for example the dw-dsi + dw-dsi2 for existing implementations. > > + i = 0; > > + while (is_rockchip(dp_data[i].chip_type)) > > + i++; > > + > > + if (id >= i) { > > + DRM_DEV_ERROR(dev, "invalid edp id: %d\n", id); > > + return -ENODEV; > > + } > > Is it applicable to non-eDP case? same as above, it's always called eDP in all pieces of documentation, (Compliant with DP 1.2 and eDP 1.3) Heiko > > + > > dp->dev = dev; > > dp->adp = ERR_PTR(-ENODEV); > > - dp->data = dp_data; > > + dp->data = &dp_data[id]; > > dp->plat_data.panel = panel; > > dp->plat_data.dev_type = dp->data->chip_type; > > dp->plat_data.power_on = rockchip_dp_poweron; > > @@ -464,19 +507,38 @@ static int rockchip_dp_resume(struct device *dev) > > static DEFINE_RUNTIME_DEV_PM_OPS(rockchip_dp_pm_ops, rockchip_dp_suspend, > > rockchip_dp_resume, NULL); > > > > -static const struct rockchip_dp_chip_data rk3399_edp = { > > - .lcdc_sel = GRF_REG_FIELD(0x6250, 5, 5), > > - .chip_type = RK3399_EDP, > > +static const struct rockchip_dp_chip_data rk3399_edp[] = { > > + { > > + .lcdc_sel = GRF_REG_FIELD(0x6250, 5, 5), > > + .chip_type = RK3399_EDP, > > + }, > > + { /* sentinel */ } > > +}; > > + > > +static const struct rockchip_dp_chip_data rk3288_dp[] = { > > + { > > + .lcdc_sel = GRF_REG_FIELD(0x025c, 5, 5), > > + .chip_type = RK3288_DP, > > + }, > > + { /* sentinel */ } > > }; > > > > -static const struct rockchip_dp_chip_data rk3288_dp = { > > - .lcdc_sel = GRF_REG_FIELD(0x025c, 5, 5), > > - .chip_type = RK3288_DP, > > +static const struct rockchip_dp_chip_data rk3588_edp[] = { > > + { > > + .edp_mode = GRF_REG_FIELD(0x0000, 0, 0), > > + .chip_type = RK3588_EDP, > > + }, > > + { > > + .edp_mode = GRF_REG_FIELD(0x0004, 0, 0), > > + .chip_type = RK3588_EDP, > > + }, > > + { /* sentinel */ } > > }; > > > > static const struct of_device_id rockchip_dp_dt_ids[] = { > > {.compatible = "rockchip,rk3288-dp", .data = &rk3288_dp }, > > {.compatible = "rockchip,rk3399-edp", .data = &rk3399_edp }, > > + {.compatible = "rockchip,rk3588-edp", .data = &rk3588_edp }, > > {} > > }; > > MODULE_DEVICE_TABLE(of, rockchip_dp_dt_ids); > > diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h > > index 6002c5666031..54086cb2d97d 100644 > > --- a/include/drm/bridge/analogix_dp.h > > +++ b/include/drm/bridge/analogix_dp.h > > @@ -15,11 +15,12 @@ enum analogix_dp_devtype { > > EXYNOS_DP, > > RK3288_DP, > > RK3399_EDP, > > + RK3588_EDP, > > }; > > > > static inline bool is_rockchip(enum analogix_dp_devtype type) > > { > > - return type == RK3288_DP || type == RK3399_EDP; > > + return type == RK3288_DP || type == RK3399_EDP || type == RK3588_EDP; > > } > > > > struct analogix_dp_plat_data { > >
On Mon, Dec 16, 2024 at 10:11:54AM +0100, Heiko Stübner wrote: > Am Montag, 16. Dezember 2024, 09:57:41 CET schrieb Dmitry Baryshkov: > > On Mon, Dec 16, 2024 at 11:12:17AM +0800, Damon Ding wrote: > > > RK3588 integrates the analogix eDP 1.3 TX controller IP and the HDMI/eDP > > > TX Combo PHY based on a Samsung IP block, and there are also two > > > independent eDP display interface on RK3588 Soc. > > > > > > Add just the basic support for now, i.e. RGB output up to 4K@60Hz, without > > > the tests of audio, PSR and other eDP 1.3 specific features. > > > > > > Signed-off-by: Damon Ding <damon.ding@rock-chips.com> > > > > > > --- > > > > > > Changes in v2: > > > - Add support for the other eDP output edp1 > > > --- > > > .../gpu/drm/rockchip/analogix_dp-rockchip.c | 82 ++++++++++++++++--- > > > include/drm/bridge/analogix_dp.h | 3 +- > > > 2 files changed, 74 insertions(+), 11 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c > > > index 871606a31ef1..4c9a55776ada 100644 > > > --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c > > > +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c > > > @@ -51,10 +51,12 @@ struct rockchip_grf_reg_field { > > > /** > > > * struct rockchip_dp_chip_data - splite the grf setting of kind of chips > > > * @lcdc_sel: grf register field of lcdc_sel > > > + * @edp_mode: grf register field of edp_mode > > > * @chip_type: specific chip type > > > */ > > > struct rockchip_dp_chip_data { > > > const struct rockchip_grf_reg_field lcdc_sel; > > > + const struct rockchip_grf_reg_field edp_mode; > > > u32 chip_type; > > > }; > > > > > > @@ -134,12 +136,21 @@ static int rockchip_dp_poweron(struct analogix_dp_plat_data *plat_data) > > > return ret; > > > } > > > > > > + ret = rockchip_grf_field_write(dp->grf, &dp->data->edp_mode, 1); > > > + if (ret != 0) > > > + DRM_DEV_ERROR(dp->dev, "failed to set edp mode %d\n", ret); > > > > Is it to be called for non-eDP hosts too? Or for older hosts? > > The implementation of rockchip_grf_field_write (in patch1) seems to > take care of checking if that field actually exists and doing nothing if > not. > > I think eDP/DP is more a naming thing, the Analogix controller is called > an eDP controller in all documentation things. > > Even back on rk3288, the Analogix-DP, still is called an eDP controller in > documentation, with the only difference being that it does not contain > another additional "dedicated" DP controller At the very least this should be mentioned in the commit message. > > > + i = 0; > > > + while (is_rockchip(dp_data[i].chip_type)) > > > + i++; > > > + > > > + if (id >= i) { > > > + DRM_DEV_ERROR(dev, "invalid edp id: %d\n", id); > > > + return -ENODEV; > > > + } > > > > Is it applicable to non-eDP case? > > same as above, it's always called eDP in all pieces of documentation, > (Compliant with DP 1.2 and eDP 1.3) Ack > > Heiko > > > > + > > > dp->dev = dev; > > > dp->adp = ERR_PTR(-ENODEV); > > > - dp->data = dp_data; > > > + dp->data = &dp_data[id]; > > > dp->plat_data.panel = panel; > > > dp->plat_data.dev_type = dp->data->chip_type; > > > dp->plat_data.power_on = rockchip_dp_poweron; > > > @@ -464,19 +507,38 @@ static int rockchip_dp_resume(struct device *dev) > > > static DEFINE_RUNTIME_DEV_PM_OPS(rockchip_dp_pm_ops, rockchip_dp_suspend, > > > rockchip_dp_resume, NULL); > > > > > > -static const struct rockchip_dp_chip_data rk3399_edp = { > > > - .lcdc_sel = GRF_REG_FIELD(0x6250, 5, 5), > > > - .chip_type = RK3399_EDP, > > > +static const struct rockchip_dp_chip_data rk3399_edp[] = { > > > + { > > > + .lcdc_sel = GRF_REG_FIELD(0x6250, 5, 5), > > > + .chip_type = RK3399_EDP, > > > + }, > > > + { /* sentinel */ } > > > +}; > > > + > > > +static const struct rockchip_dp_chip_data rk3288_dp[] = { > > > + { > > > + .lcdc_sel = GRF_REG_FIELD(0x025c, 5, 5), > > > + .chip_type = RK3288_DP, > > > + }, > > > + { /* sentinel */ } > > > }; > > > > > > -static const struct rockchip_dp_chip_data rk3288_dp = { > > > - .lcdc_sel = GRF_REG_FIELD(0x025c, 5, 5), > > > - .chip_type = RK3288_DP, > > > +static const struct rockchip_dp_chip_data rk3588_edp[] = { > > > + { > > > + .edp_mode = GRF_REG_FIELD(0x0000, 0, 0), > > > + .chip_type = RK3588_EDP, > > > + }, > > > + { > > > + .edp_mode = GRF_REG_FIELD(0x0004, 0, 0), > > > + .chip_type = RK3588_EDP, > > > + }, > > > + { /* sentinel */ } > > > }; > > > > > > static const struct of_device_id rockchip_dp_dt_ids[] = { > > > {.compatible = "rockchip,rk3288-dp", .data = &rk3288_dp }, > > > {.compatible = "rockchip,rk3399-edp", .data = &rk3399_edp }, > > > + {.compatible = "rockchip,rk3588-edp", .data = &rk3588_edp }, > > > {} > > > }; > > > MODULE_DEVICE_TABLE(of, rockchip_dp_dt_ids); > > > diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h > > > index 6002c5666031..54086cb2d97d 100644 > > > --- a/include/drm/bridge/analogix_dp.h > > > +++ b/include/drm/bridge/analogix_dp.h > > > @@ -15,11 +15,12 @@ enum analogix_dp_devtype { > > > EXYNOS_DP, > > > RK3288_DP, > > > RK3399_EDP, > > > + RK3588_EDP, > > > }; > > > > > > static inline bool is_rockchip(enum analogix_dp_devtype type) > > > { > > > - return type == RK3288_DP || type == RK3399_EDP; > > > + return type == RK3288_DP || type == RK3399_EDP || type == RK3588_EDP; > > > } > > > > > > struct analogix_dp_plat_data { > > > > > > > >
Hi, On 2024/12/16 17:11, Heiko Stübner wrote: > Am Montag, 16. Dezember 2024, 09:57:41 CET schrieb Dmitry Baryshkov: >> On Mon, Dec 16, 2024 at 11:12:17AM +0800, Damon Ding wrote: >>> RK3588 integrates the analogix eDP 1.3 TX controller IP and the HDMI/eDP >>> TX Combo PHY based on a Samsung IP block, and there are also two >>> independent eDP display interface on RK3588 Soc. >>> >>> Add just the basic support for now, i.e. RGB output up to 4K@60Hz, without >>> the tests of audio, PSR and other eDP 1.3 specific features. >>> >>> Signed-off-by: Damon Ding <damon.ding@rock-chips.com> >>> >>> --- >>> >>> Changes in v2: >>> - Add support for the other eDP output edp1 >>> --- >>> .../gpu/drm/rockchip/analogix_dp-rockchip.c | 82 ++++++++++++++++--- >>> include/drm/bridge/analogix_dp.h | 3 +- >>> 2 files changed, 74 insertions(+), 11 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c >>> index 871606a31ef1..4c9a55776ada 100644 >>> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c >>> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c >>> @@ -51,10 +51,12 @@ struct rockchip_grf_reg_field { >>> /** >>> * struct rockchip_dp_chip_data - splite the grf setting of kind of chips >>> * @lcdc_sel: grf register field of lcdc_sel >>> + * @edp_mode: grf register field of edp_mode >>> * @chip_type: specific chip type >>> */ >>> struct rockchip_dp_chip_data { >>> const struct rockchip_grf_reg_field lcdc_sel; >>> + const struct rockchip_grf_reg_field edp_mode; >>> u32 chip_type; >>> }; >>> >>> @@ -134,12 +136,21 @@ static int rockchip_dp_poweron(struct analogix_dp_plat_data *plat_data) >>> return ret; >>> } >>> >>> + ret = rockchip_grf_field_write(dp->grf, &dp->data->edp_mode, 1); >>> + if (ret != 0) >>> + DRM_DEV_ERROR(dp->dev, "failed to set edp mode %d\n", ret); >> >> Is it to be called for non-eDP hosts too? Or for older hosts? > > The implementation of rockchip_grf_field_write (in patch1) seems to > take care of checking if that field actually exists and doing nothing if > not. > > I think eDP/DP is more a naming thing, the Analogix controller is called > an eDP controller in all documentation things. > > Even back on rk3288, the Analogix-DP, still is called an eDP controller in > documentation, with the only difference being that it does not contain > another additional "dedicated" DP controller > >>> @@ -396,6 +425,7 @@ static int rockchip_dp_probe(struct platform_device *pdev) >>> const struct rockchip_dp_chip_data *dp_data; >>> struct drm_panel *panel = NULL; >>> struct rockchip_dp_device *dp; >>> + int id, i; >>> int ret; >>> >>> dp_data = of_device_get_match_data(dev); >>> @@ -410,9 +440,22 @@ static int rockchip_dp_probe(struct platform_device *pdev) >>> if (!dp) >>> return -ENOMEM; >>> >>> + id = of_alias_get_id(dev->of_node, "edp"); >>> + if (id < 0) >>> + id = 0; >> >> Alias is not documented. > > additionally, aliases should never be used to determine actual device > functionionality - e.g. the hw-device-number > >> Please check how other platforms handle device -> ID conversion and >> consider following it (yes, the best option currently known is to >> hardcode bus addresses into the driver data). > > see for example the dw-dsi + dw-dsi2 for existing implementations. > Indeed, I will fix it in the next version. > > >>> + i = 0; >>> + while (is_rockchip(dp_data[i].chip_type)) >>> + i++; >>> + >>> + if (id >= i) { >>> + DRM_DEV_ERROR(dev, "invalid edp id: %d\n", id); >>> + return -ENODEV; >>> + } >> >> Is it applicable to non-eDP case? > > same as above, it's always called eDP in all pieces of documentation, > (Compliant with DP 1.2 and eDP 1.3) > > Heiko > >>> + >>> dp->dev = dev; >>> dp->adp = ERR_PTR(-ENODEV); >>> - dp->data = dp_data; >>> + dp->data = &dp_data[id]; >>> dp->plat_data.panel = panel; >>> dp->plat_data.dev_type = dp->data->chip_type; >>> dp->plat_data.power_on = rockchip_dp_poweron; >>> @@ -464,19 +507,38 @@ static int rockchip_dp_resume(struct device *dev) >>> static DEFINE_RUNTIME_DEV_PM_OPS(rockchip_dp_pm_ops, rockchip_dp_suspend, >>> rockchip_dp_resume, NULL); >>> >>> -static const struct rockchip_dp_chip_data rk3399_edp = { >>> - .lcdc_sel = GRF_REG_FIELD(0x6250, 5, 5), >>> - .chip_type = RK3399_EDP, >>> +static const struct rockchip_dp_chip_data rk3399_edp[] = { >>> + { >>> + .lcdc_sel = GRF_REG_FIELD(0x6250, 5, 5), >>> + .chip_type = RK3399_EDP, >>> + }, >>> + { /* sentinel */ } >>> +}; >>> + >>> +static const struct rockchip_dp_chip_data rk3288_dp[] = { >>> + { >>> + .lcdc_sel = GRF_REG_FIELD(0x025c, 5, 5), >>> + .chip_type = RK3288_DP, >>> + }, >>> + { /* sentinel */ } >>> }; >>> >>> -static const struct rockchip_dp_chip_data rk3288_dp = { >>> - .lcdc_sel = GRF_REG_FIELD(0x025c, 5, 5), >>> - .chip_type = RK3288_DP, >>> +static const struct rockchip_dp_chip_data rk3588_edp[] = { >>> + { >>> + .edp_mode = GRF_REG_FIELD(0x0000, 0, 0), >>> + .chip_type = RK3588_EDP, >>> + }, >>> + { >>> + .edp_mode = GRF_REG_FIELD(0x0004, 0, 0), >>> + .chip_type = RK3588_EDP, >>> + }, >>> + { /* sentinel */ } >>> }; >>> >>> static const struct of_device_id rockchip_dp_dt_ids[] = { >>> {.compatible = "rockchip,rk3288-dp", .data = &rk3288_dp }, >>> {.compatible = "rockchip,rk3399-edp", .data = &rk3399_edp }, >>> + {.compatible = "rockchip,rk3588-edp", .data = &rk3588_edp }, >>> {} >>> }; >>> MODULE_DEVICE_TABLE(of, rockchip_dp_dt_ids); >>> diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h >>> index 6002c5666031..54086cb2d97d 100644 >>> --- a/include/drm/bridge/analogix_dp.h >>> +++ b/include/drm/bridge/analogix_dp.h >>> @@ -15,11 +15,12 @@ enum analogix_dp_devtype { >>> EXYNOS_DP, >>> RK3288_DP, >>> RK3399_EDP, >>> + RK3588_EDP, >>> }; >>> >>> static inline bool is_rockchip(enum analogix_dp_devtype type) >>> { >>> - return type == RK3288_DP || type == RK3399_EDP; >>> + return type == RK3288_DP || type == RK3399_EDP || type == RK3588_EDP; >>> } >>> >>> struct analogix_dp_plat_data { >> >> > > > > > > Best regards, Damon
diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c index 871606a31ef1..4c9a55776ada 100644 --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c @@ -51,10 +51,12 @@ struct rockchip_grf_reg_field { /** * struct rockchip_dp_chip_data - splite the grf setting of kind of chips * @lcdc_sel: grf register field of lcdc_sel + * @edp_mode: grf register field of edp_mode * @chip_type: specific chip type */ struct rockchip_dp_chip_data { const struct rockchip_grf_reg_field lcdc_sel; + const struct rockchip_grf_reg_field edp_mode; u32 chip_type; }; @@ -134,12 +136,21 @@ static int rockchip_dp_poweron(struct analogix_dp_plat_data *plat_data) return ret; } + ret = rockchip_grf_field_write(dp->grf, &dp->data->edp_mode, 1); + if (ret != 0) + DRM_DEV_ERROR(dp->dev, "failed to set edp mode %d\n", ret); + return ret; } static int rockchip_dp_powerdown(struct analogix_dp_plat_data *plat_data) { struct rockchip_dp_device *dp = pdata_encoder_to_dp(plat_data); + int ret; + + ret = rockchip_grf_field_write(dp->grf, &dp->data->edp_mode, 0); + if (ret != 0) + DRM_DEV_ERROR(dp->dev, "failed to set edp mode %d\n", ret); clk_disable_unprepare(dp->pclk); @@ -203,6 +214,10 @@ static void rockchip_dp_drm_encoder_enable(struct drm_encoder *encoder, struct rockchip_dp_device *dp = encoder_to_dp(encoder); struct drm_crtc *crtc; struct drm_crtc_state *old_crtc_state; + struct of_endpoint endpoint; + struct device_node *remote_port, *remote_port_parent; + char name[32]; + u32 port_id; int ret; crtc = rockchip_dp_drm_get_new_crtc(encoder, state); @@ -220,13 +235,27 @@ static void rockchip_dp_drm_encoder_enable(struct drm_encoder *encoder, return; } - ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, encoder); + ret = drm_of_encoder_active_endpoint(dp->dev->of_node, encoder, &endpoint); if (ret < 0) return; - DRM_DEV_DEBUG(dp->dev, "vop %s output to dp\n", (ret) ? "LIT" : "BIG"); + remote_port_parent = of_graph_get_remote_port_parent(endpoint.local_node); + if (remote_port_parent) { + if (of_get_child_by_name(remote_port_parent, "ports")) { + remote_port = of_graph_get_remote_port(endpoint.local_node); + of_property_read_u32(remote_port, "reg", &port_id); + of_node_put(remote_port); + sprintf(name, "%s vp%d", remote_port_parent->full_name, port_id); + } else { + sprintf(name, "%s %s", + remote_port_parent->full_name, endpoint.id ? "vopl" : "vopb"); + } + of_node_put(remote_port_parent); + + DRM_DEV_DEBUG(dp->dev, "%s output to edp\n", name); + } - ret = rockchip_grf_field_write(dp->grf, &dp->data->lcdc_sel, ret); + ret = rockchip_grf_field_write(dp->grf, &dp->data->lcdc_sel, endpoint.id); if (ret != 0) DRM_DEV_ERROR(dp->dev, "Could not write to GRF: %d\n", ret); @@ -396,6 +425,7 @@ static int rockchip_dp_probe(struct platform_device *pdev) const struct rockchip_dp_chip_data *dp_data; struct drm_panel *panel = NULL; struct rockchip_dp_device *dp; + int id, i; int ret; dp_data = of_device_get_match_data(dev); @@ -410,9 +440,22 @@ static int rockchip_dp_probe(struct platform_device *pdev) if (!dp) return -ENOMEM; + id = of_alias_get_id(dev->of_node, "edp"); + if (id < 0) + id = 0; + + i = 0; + while (is_rockchip(dp_data[i].chip_type)) + i++; + + if (id >= i) { + DRM_DEV_ERROR(dev, "invalid edp id: %d\n", id); + return -ENODEV; + } + dp->dev = dev; dp->adp = ERR_PTR(-ENODEV); - dp->data = dp_data; + dp->data = &dp_data[id]; dp->plat_data.panel = panel; dp->plat_data.dev_type = dp->data->chip_type; dp->plat_data.power_on = rockchip_dp_poweron; @@ -464,19 +507,38 @@ static int rockchip_dp_resume(struct device *dev) static DEFINE_RUNTIME_DEV_PM_OPS(rockchip_dp_pm_ops, rockchip_dp_suspend, rockchip_dp_resume, NULL); -static const struct rockchip_dp_chip_data rk3399_edp = { - .lcdc_sel = GRF_REG_FIELD(0x6250, 5, 5), - .chip_type = RK3399_EDP, +static const struct rockchip_dp_chip_data rk3399_edp[] = { + { + .lcdc_sel = GRF_REG_FIELD(0x6250, 5, 5), + .chip_type = RK3399_EDP, + }, + { /* sentinel */ } +}; + +static const struct rockchip_dp_chip_data rk3288_dp[] = { + { + .lcdc_sel = GRF_REG_FIELD(0x025c, 5, 5), + .chip_type = RK3288_DP, + }, + { /* sentinel */ } }; -static const struct rockchip_dp_chip_data rk3288_dp = { - .lcdc_sel = GRF_REG_FIELD(0x025c, 5, 5), - .chip_type = RK3288_DP, +static const struct rockchip_dp_chip_data rk3588_edp[] = { + { + .edp_mode = GRF_REG_FIELD(0x0000, 0, 0), + .chip_type = RK3588_EDP, + }, + { + .edp_mode = GRF_REG_FIELD(0x0004, 0, 0), + .chip_type = RK3588_EDP, + }, + { /* sentinel */ } }; static const struct of_device_id rockchip_dp_dt_ids[] = { {.compatible = "rockchip,rk3288-dp", .data = &rk3288_dp }, {.compatible = "rockchip,rk3399-edp", .data = &rk3399_edp }, + {.compatible = "rockchip,rk3588-edp", .data = &rk3588_edp }, {} }; MODULE_DEVICE_TABLE(of, rockchip_dp_dt_ids); diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h index 6002c5666031..54086cb2d97d 100644 --- a/include/drm/bridge/analogix_dp.h +++ b/include/drm/bridge/analogix_dp.h @@ -15,11 +15,12 @@ enum analogix_dp_devtype { EXYNOS_DP, RK3288_DP, RK3399_EDP, + RK3588_EDP, }; static inline bool is_rockchip(enum analogix_dp_devtype type) { - return type == RK3288_DP || type == RK3399_EDP; + return type == RK3288_DP || type == RK3399_EDP || type == RK3588_EDP; } struct analogix_dp_plat_data {
RK3588 integrates the analogix eDP 1.3 TX controller IP and the HDMI/eDP TX Combo PHY based on a Samsung IP block, and there are also two independent eDP display interface on RK3588 Soc. Add just the basic support for now, i.e. RGB output up to 4K@60Hz, without the tests of audio, PSR and other eDP 1.3 specific features. Signed-off-by: Damon Ding <damon.ding@rock-chips.com> --- Changes in v2: - Add support for the other eDP output edp1 --- .../gpu/drm/rockchip/analogix_dp-rockchip.c | 82 ++++++++++++++++--- include/drm/bridge/analogix_dp.h | 3 +- 2 files changed, 74 insertions(+), 11 deletions(-)