Message ID | 20230125113529.13952-1-a-bhatia1@ti.com |
---|---|
Headers | show |
Series | Add DSS support for AM625 SoC | expand |
On 25/01/2023 13:35, Aradhya Bhatia wrote: > Make DSS Video Ports agnostic of output bus types. > > DSS controllers have had a 1-to-1 coupling between its VPs and its > output ports. This no longer stands true for the new AM625 DSS. This > coupling, hence, has been removed by renaming the 'vp_bus_type' to > 'output_port_bus_type' because the VPs are essentially agnostic of the > bus type and it is the output ports which have a bus type. > > The AM625 DSS has 2 VPs but requires 3 output ports to support its > Dual-Link OLDI video output coming from a single VP. Not a biggie, but this sentence is a bit odd here at the end. Shouldn't it be after the "...stands true for the new AM625 DSS."? > Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> > --- > drivers/gpu/drm/tidss/tidss_dispc.c | 47 +++++++++++++++++------------ > drivers/gpu/drm/tidss/tidss_dispc.h | 21 +++++++------ > drivers/gpu/drm/tidss/tidss_drv.h | 5 +-- > drivers/gpu/drm/tidss/tidss_irq.h | 2 +- > drivers/gpu/drm/tidss/tidss_kms.c | 12 ++++---- > 5 files changed, 48 insertions(+), 39 deletions(-) > > diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c > index 165365b515e1..c1c4faccbddc 100644 > --- a/drivers/gpu/drm/tidss/tidss_dispc.c > +++ b/drivers/gpu/drm/tidss/tidss_dispc.c > @@ -61,7 +61,7 @@ const struct dispc_features dispc_k2g_feats = { > .min_pclk_khz = 4375, > > .max_pclk_khz = { > - [DISPC_VP_DPI] = 150000, > + [DISPC_PORT_DPI] = 150000, > }, > > /* > @@ -96,7 +96,6 @@ const struct dispc_features dispc_k2g_feats = { > .vp_name = { "vp1" }, > .ovr_name = { "ovr1" }, > .vpclk_name = { "vp1" }, > - .vp_bus_type = { DISPC_VP_DPI }, > > .vp_feat = { .color = { > .has_ctm = true, > @@ -109,6 +108,9 @@ const struct dispc_features dispc_k2g_feats = { > .vid_name = { "vid1" }, > .vid_lite = { false }, > .vid_order = { 0 }, > + > + .num_output_ports = 1, > + .output_port_bus_type = { DISPC_PORT_DPI }, > }; Just thinking out loud, as these will get more complex in the future, maybe we should finally group them with struct. E.g. we could define struct array for vps, like (just hacky example): struct { const char *name; const char *clkname; struct tidss_vp_feat feat; } vps[TIDSS_MAX_PORTS]; and then use them as: .vps = { { .name = "kala", .clkname = "kissa", .feat.color.has_ctm = true, }, { .name = "kala2", .clkname = "kissa2", .feat.color.has_ctm = false, }, }, Perhaps something to try in the future. > static const u16 tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { > @@ -140,8 +142,8 @@ static const u16 tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { > > const struct dispc_features dispc_am65x_feats = { > .max_pclk_khz = { > - [DISPC_VP_DPI] = 165000, > - [DISPC_VP_OLDI] = 165000, > + [DISPC_PORT_DPI] = 165000, > + [DISPC_PORT_OLDI] = 165000, > }, > > .scaling = { > @@ -171,7 +173,6 @@ const struct dispc_features dispc_am65x_feats = { > .vp_name = { "vp1", "vp2" }, > .ovr_name = { "ovr1", "ovr2" }, > .vpclk_name = { "vp1", "vp2" }, > - .vp_bus_type = { DISPC_VP_OLDI, DISPC_VP_DPI }, > > .vp_feat = { .color = { > .has_ctm = true, > @@ -185,6 +186,9 @@ const struct dispc_features dispc_am65x_feats = { > .vid_name = { "vid", "vidl1" }, > .vid_lite = { false, true, }, > .vid_order = { 1, 0 }, > + > + .num_output_ports = 2, > + .output_port_bus_type = { DISPC_PORT_OLDI, DISPC_PORT_DPI }, > }; > > static const u16 tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { > @@ -229,8 +233,8 @@ static const u16 tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { > > const struct dispc_features dispc_j721e_feats = { > .max_pclk_khz = { > - [DISPC_VP_DPI] = 170000, > - [DISPC_VP_INTERNAL] = 600000, > + [DISPC_PORT_DPI] = 170000, > + [DISPC_PORT_INTERNAL] = 600000, > }, > > .scaling = { > @@ -260,9 +264,7 @@ const struct dispc_features dispc_j721e_feats = { > .vp_name = { "vp1", "vp2", "vp3", "vp4" }, > .ovr_name = { "ovr1", "ovr2", "ovr3", "ovr4" }, > .vpclk_name = { "vp1", "vp2", "vp3", "vp4" }, > - /* Currently hard coded VP routing (see dispc_initial_config()) */ > - .vp_bus_type = { DISPC_VP_INTERNAL, DISPC_VP_DPI, > - DISPC_VP_INTERNAL, DISPC_VP_DPI, }, > + I think this line feed is extra. > .vp_feat = { .color = { > .has_ctm = true, > .gamma_size = 1024, > @@ -273,6 +275,11 @@ const struct dispc_features dispc_j721e_feats = { > .vid_name = { "vid1", "vidl1", "vid2", "vidl2" }, > .vid_lite = { 0, 1, 0, 1, }, > .vid_order = { 1, 3, 0, 2 }, > + > + .num_output_ports = 4, > + /* Currently hard coded VP routing (see dispc_initial_config()) */ > + .output_port_bus_type = { DISPC_PORT_INTERNAL, DISPC_PORT_DPI, > + DISPC_PORT_INTERNAL, DISPC_PORT_DPI, }, Indent doesn't look right (but it might be just because this is a diff). > }; > > static const u16 *dispc_common_regmap; > @@ -287,12 +294,12 @@ struct dispc_device { > > void __iomem *base_common; > void __iomem *base_vid[TIDSS_MAX_PLANES]; > - void __iomem *base_ovr[TIDSS_MAX_PORTS]; > - void __iomem *base_vp[TIDSS_MAX_PORTS]; > + void __iomem *base_ovr[TIDSS_MAX_VPS]; > + void __iomem *base_vp[TIDSS_MAX_VPS]; > > struct regmap *oldi_io_ctrl; > > - struct clk *vp_clk[TIDSS_MAX_PORTS]; > + struct clk *vp_clk[TIDSS_MAX_VPS]; > > const struct dispc_features *feat; > > @@ -300,7 +307,7 @@ struct dispc_device { > > bool is_enabled; > > - struct dss_vp_data vp_data[TIDSS_MAX_PORTS]; > + struct dss_vp_data vp_data[TIDSS_MAX_VPS]; > > u32 *fourccs; > u32 num_fourccs; > @@ -851,7 +858,7 @@ int dispc_vp_bus_check(struct dispc_device *dispc, u32 hw_videoport, > return -EINVAL; > } > > - if (dispc->feat->vp_bus_type[hw_videoport] != DISPC_VP_OLDI && > + if (dispc->feat->output_port_bus_type[hw_videoport] != DISPC_PORT_OLDI && Hmm, so is the hw_videoport a vp index or an output index? Sounds like the former, so it's not right, even if at the moment they're identical. We need some kind of mapping between those. If the mapping can be changed (or just defined in the DT), I think we need a variable in struct dispc_device, which tells the output to which a videoport is connected to. Or vice versa, I'm not sure which direction we need more. If the mapping is always the same on all SoC (but I don't think so), we can have it in the feats. Also, I wonder if output_port is a good name as it has "port" in it (like video port), and it's a bit long-ish. Would just "output" be enough? We could, of course, shorten it to OP, but that looks odd to me =). > fmt->is_oldi_fmt) { > dev_dbg(dispc->dev, "%s: %s is not OLDI-port\n", > __func__, dispc->feat->vp_name[hw_videoport]); > @@ -955,7 +962,7 @@ void dispc_vp_prepare(struct dispc_device *dispc, u32 hw_videoport, > if (WARN_ON(!fmt)) > return; > > - if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) { > + if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) { > dispc_oldi_tx_power(dispc, true); > > dispc_enable_oldi(dispc, hw_videoport, fmt); > @@ -1014,7 +1021,7 @@ void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport, > align = true; > > /* always use DE_HIGH for OLDI */ > - if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) > + if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) > ieo = false; > > dispc_vp_write(dispc, hw_videoport, DISPC_VP_POL_FREQ, > @@ -1040,7 +1047,7 @@ void dispc_vp_disable(struct dispc_device *dispc, u32 hw_videoport) > > void dispc_vp_unprepare(struct dispc_device *dispc, u32 hw_videoport) > { > - if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) { > + if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) { > dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 0); > > dispc_oldi_tx_power(dispc, false); > @@ -1116,10 +1123,10 @@ enum drm_mode_status dispc_vp_mode_valid(struct dispc_device *dispc, > const struct drm_display_mode *mode) > { > u32 hsw, hfp, hbp, vsw, vfp, vbp; > - enum dispc_vp_bus_type bus_type; > + enum dispc_port_bus_type bus_type; > int max_pclk; > > - bus_type = dispc->feat->vp_bus_type[hw_videoport]; > + bus_type = dispc->feat->output_port_bus_type[hw_videoport]; > > max_pclk = dispc->feat->max_pclk_khz[bus_type]; > > diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h > index e49432f0abf5..30fb44158347 100644 > --- a/drivers/gpu/drm/tidss/tidss_dispc.h > +++ b/drivers/gpu/drm/tidss/tidss_dispc.h > @@ -50,11 +50,11 @@ struct dispc_errata { > bool i2000; /* DSS Does Not Support YUV Pixel Data Formats */ > }; > > -enum dispc_vp_bus_type { > - DISPC_VP_DPI, /* DPI output */ > - DISPC_VP_OLDI, /* OLDI (LVDS) output */ > - DISPC_VP_INTERNAL, /* SoC internal routing */ > - DISPC_VP_MAX_BUS_TYPE, > +enum dispc_port_bus_type { > + DISPC_PORT_DPI, /* DPI output */ > + DISPC_PORT_OLDI, /* OLDI (LVDS) output */ > + DISPC_PORT_INTERNAL, /* SoC internal routing */ > + DISPC_PORT_MAX_BUS_TYPE, Okay, so here you have just "port", not "output_port". In the DT, they're ports, so... Maybe we could use that name too, and for video port always use "vp". The current "hw_videoport" could be easily mistaken with "port". I don't recall why I chose to use "hw" prefix there. I think I wanted to separate it from some other videoport, but... I don't know what that "other" is =). Tomi
On 25/01/2023 13:35, Aradhya Bhatia wrote: > The newer version of DSS (AM625-DSS) has 2 OLDI TXes at its disposal. > These can be configured to support the following modes: > > 1. OLDI_SINGLE_LINK_SINGLE_MODE > Single Output over OLDI 0. > +------+ +---------+ +-------+ > | | | | | | > | CRTC +------->+ ENCODER +----->| PANEL | > | | | | | | > +------+ +---------+ +-------+ > > 2. OLDI_SINGLE_LINK_CLONE_MODE > Duplicate Output over OLDI 0 and 1. > +------+ +---------+ +-------+ > | | | | | | > | CRTC +---+--->| ENCODER +----->| PANEL | > | | | | | | | > +------+ | +---------+ +-------+ > | > | +---------+ +-------+ > | | | | | > +--->| ENCODER +----->| PANEL | > | | | | > +---------+ +-------+ > > 3. OLDI_DUAL_LINK_MODE > Combined Output over OLDI 0 and 1. > +------+ +---------+ +-------+ > | | | +----->| | > | CRTC +------->+ ENCODER | | PANEL | > | | | +----->| | > +------+ +---------+ +-------+ > > Following the above pathways for different modes, 2 encoder/panel-bridge > pipes get created for clone mode, and 1 pipe in cases of single link and > dual link mode. > > Add support for confguring the OLDI modes using OF and LVDS DRM helper > functions. > > Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> > --- > drivers/gpu/drm/tidss/tidss_dispc.c | 24 ++- > drivers/gpu/drm/tidss/tidss_dispc.h | 12 ++ > drivers/gpu/drm/tidss/tidss_drv.h | 3 + > drivers/gpu/drm/tidss/tidss_encoder.c | 4 +- > drivers/gpu/drm/tidss/tidss_encoder.h | 3 +- > drivers/gpu/drm/tidss/tidss_kms.c | 221 ++++++++++++++++++++++++-- > 6 files changed, 245 insertions(+), 22 deletions(-) > > diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c > index b55ccbcaa67f..37a73e309330 100644 > --- a/drivers/gpu/drm/tidss/tidss_dispc.c > +++ b/drivers/gpu/drm/tidss/tidss_dispc.c > @@ -88,6 +88,8 @@ const struct dispc_features dispc_k2g_feats = { > > .subrev = DISPC_K2G, > > + .has_oldi = false, > + > .common = "common", > > .common_regs = tidss_k2g_common_regs, > @@ -166,6 +168,8 @@ const struct dispc_features dispc_am625_feats = { > > .subrev = DISPC_AM625, > > + .has_oldi = true, > + > .common = "common", > .common_regs = tidss_am65x_common_regs, > > @@ -218,6 +222,8 @@ const struct dispc_features dispc_am65x_feats = { > > .subrev = DISPC_AM65X, > > + .has_oldi = true, > + > .common = "common", > .common_regs = tidss_am65x_common_regs, > > @@ -309,6 +315,8 @@ const struct dispc_features dispc_j721e_feats = { > > .subrev = DISPC_J721E, > > + .has_oldi = false, > + > .common = "common_m", > .common_regs = tidss_j721e_common_regs, > > @@ -361,6 +369,8 @@ struct dispc_device { > > struct dss_vp_data vp_data[TIDSS_MAX_VPS]; > > + enum dispc_oldi_modes oldi_mode; > + > u32 *fourccs; > u32 num_fourccs; > > @@ -1963,6 +1973,12 @@ const u32 *dispc_plane_formats(struct dispc_device *dispc, unsigned int *len) > return dispc->fourccs; > } > > +void dispc_set_oldi_mode(struct dispc_device *dispc, > + enum dispc_oldi_modes oldi_mode) > +{ > + dispc->oldi_mode = oldi_mode; > +} > + > static s32 pixinc(int pixels, u8 ps) > { > if (pixels == 1) > @@ -2647,7 +2663,7 @@ int dispc_runtime_resume(struct dispc_device *dispc) > REG_GET(dispc, DSS_SYSSTATUS, 2, 2), > REG_GET(dispc, DSS_SYSSTATUS, 3, 3)); > > - if (dispc->feat->subrev == DISPC_AM65X) > + if (dispc->feat->has_oldi) > dev_dbg(dispc->dev, "OLDI RESETDONE %d,%d,%d\n", > REG_GET(dispc, DSS_SYSSTATUS, 5, 5), > REG_GET(dispc, DSS_SYSSTATUS, 6, 6), > @@ -2688,7 +2704,7 @@ static int dispc_iomap_resource(struct platform_device *pdev, const char *name, > return 0; > } > > -static int dispc_init_am65x_oldi_io_ctrl(struct device *dev, > +static int dispc_init_am6xx_oldi_io_ctrl(struct device *dev, > struct dispc_device *dispc) > { > dispc->oldi_io_ctrl = > @@ -2827,8 +2843,8 @@ int dispc_init(struct tidss_device *tidss) > dispc->vp_data[i].gamma_table = gamma_table; > } > > - if (feat->subrev == DISPC_AM65X) { > - r = dispc_init_am65x_oldi_io_ctrl(dev, dispc); > + if (feat->has_oldi) { > + r = dispc_init_am6xx_oldi_io_ctrl(dev, dispc); > if (r) > return r; > } > diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h > index 971f2856f015..880bc7de68b3 100644 > --- a/drivers/gpu/drm/tidss/tidss_dispc.h > +++ b/drivers/gpu/drm/tidss/tidss_dispc.h > @@ -64,6 +64,15 @@ enum dispc_dss_subrevision { > DISPC_J721E, > }; > > +enum dispc_oldi_modes { > + OLDI_MODE_SINGLE_LINK, /* Single output over OLDI 0. */ > + OLDI_MODE_CLONE_SINGLE_LINK, /* Cloned output over OLDI 0 and 1. */ > + OLDI_MODE_DUAL_LINK, /* Combined output over OLDI 0 and 1. */ > + OLDI_MODE_OFF, /* OLDI TXes not connected in OF. */ > + OLDI_MODE_UNSUPPORTED, /* Unsupported OLDI configuration in OF. */ > + OLDI_MODE_UNAVAILABLE, /* OLDI TXes not available in SoC. */ > +}; > + > struct dispc_features { > int min_pclk_khz; > int max_pclk_khz[DISPC_PORT_MAX_BUS_TYPE]; > @@ -72,6 +81,8 @@ struct dispc_features { > > enum dispc_dss_subrevision subrev; > > + bool has_oldi; > + > const char *common; > const u16 *common_regs; > u32 num_vps; > @@ -131,6 +142,7 @@ int dispc_plane_setup(struct dispc_device *dispc, u32 hw_plane, > u32 hw_videoport); > int dispc_plane_enable(struct dispc_device *dispc, u32 hw_plane, bool enable); > const u32 *dispc_plane_formats(struct dispc_device *dispc, unsigned int *len); > +void dispc_set_oldi_mode(struct dispc_device *dispc, enum dispc_oldi_modes oldi_mode); > > int dispc_init(struct tidss_device *tidss); > void dispc_remove(struct tidss_device *tidss); > diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h > index 0ce7ee5ccd5b..58892f065c16 100644 > --- a/drivers/gpu/drm/tidss/tidss_drv.h > +++ b/drivers/gpu/drm/tidss/tidss_drv.h > @@ -13,6 +13,9 @@ > #define TIDSS_MAX_PLANES 4 > #define TIDSS_MAX_OUTPUT_PORTS 4 > > +/* For AM625-DSS with 2 OLDI TXes */ > +#define TIDSS_MAX_BRIDGES_PER_PIPE 2 > + > typedef u32 dispc_irq_t; > > struct tidss_device { > diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c b/drivers/gpu/drm/tidss/tidss_encoder.c > index 0d4865e9c03d..bd2a7358d7b0 100644 > --- a/drivers/gpu/drm/tidss/tidss_encoder.c > +++ b/drivers/gpu/drm/tidss/tidss_encoder.c > @@ -70,7 +70,8 @@ static const struct drm_encoder_funcs encoder_funcs = { > }; > > struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss, > - u32 encoder_type, u32 possible_crtcs) > + u32 encoder_type, u32 possible_crtcs, > + u32 possible_clones) > { > struct drm_encoder *enc; > int ret; > @@ -80,6 +81,7 @@ struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss, > return ERR_PTR(-ENOMEM); > > enc->possible_crtcs = possible_crtcs; > + enc->possible_clones = possible_clones; > > ret = drm_encoder_init(&tidss->ddev, enc, &encoder_funcs, > encoder_type, NULL); > diff --git a/drivers/gpu/drm/tidss/tidss_encoder.h b/drivers/gpu/drm/tidss/tidss_encoder.h > index ace877c0e0fd..01c62ba3ef16 100644 > --- a/drivers/gpu/drm/tidss/tidss_encoder.h > +++ b/drivers/gpu/drm/tidss/tidss_encoder.h > @@ -12,6 +12,7 @@ > struct tidss_device; > > struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss, > - u32 encoder_type, u32 possible_crtcs); > + u32 encoder_type, u32 possible_crtcs, > + u32 possible_clones); > > #endif > diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c > index d449131935d2..8322ee6310bf 100644 > --- a/drivers/gpu/drm/tidss/tidss_kms.c > +++ b/drivers/gpu/drm/tidss/tidss_kms.c > @@ -13,6 +13,7 @@ > #include <drm/drm_of.h> > #include <drm/drm_panel.h> > #include <drm/drm_vblank.h> > +#include <linux/of.h> > > #include "tidss_crtc.h" > #include "tidss_dispc.h" > @@ -104,26 +105,129 @@ static const struct drm_mode_config_funcs mode_config_funcs = { > .atomic_commit = drm_atomic_helper_commit, > }; > > +static enum dispc_oldi_modes tidss_get_oldi_mode(struct tidss_device *tidss) > +{ > + int pixel_order; > + enum dispc_oldi_modes oldi_mode; > + struct device_node *oldi0_port, *oldi1_port; > + > + /* > + * For am625-dss, the OLDI ports are expected at port reg = 0 and 2, > + * and for am65x-dss, the OLDI port is expected only at port reg = 0. > + */ > + const u32 portnum_oldi0 = 0, portnum_oldi1 = 2; > + > + oldi0_port = of_graph_get_port_by_id(tidss->dev->of_node, portnum_oldi0); > + oldi1_port = of_graph_get_port_by_id(tidss->dev->of_node, portnum_oldi1); > + > + if (!(oldi0_port || oldi1_port)) { > + /* Keep OLDI TXes OFF if neither OLDI port is present. */ > + oldi_mode = OLDI_MODE_OFF; > + } else if (oldi0_port && !oldi1_port) { > + /* > + * OLDI0 port found, but not OLDI1 port. Setting single > + * link output mode. > + */ > + oldi_mode = OLDI_MODE_SINGLE_LINK; > + } else if (!oldi0_port && oldi1_port) { > + /* > + * The 2nd OLDI TX cannot be operated alone. This use case is > + * not supported in the HW. Since the pins for OLDIs 0 and 1 are > + * separate, one could theoretically set a clone mode over OLDIs > + * 0 and 1 and just simply not use the OLDI 0. This is a hacky > + * way to enable only OLDI TX 1 and hence is not officially > + * supported. > + */ > + dev_warn(tidss->dev, > + "Single Mode over OLDI 1 is not supported in HW.\n"); > + oldi_mode = OLDI_MODE_UNSUPPORTED; > + } else { > + /* > + * OLDI Ports found for both the OLDI TXes. The DSS is to be > + * configured in either Dual Link or Clone Mode. > + */ > + pixel_order = drm_of_lvds_get_dual_link_pixel_order(oldi0_port, > + oldi1_port); > + switch (pixel_order) { > + case -EINVAL: > + /* > + * The dual link properties were not found in at least > + * one of the sink nodes. Since 2 OLDI ports are present > + * in the DT, it can be safely assumed that the required > + * configuration is Clone Mode. > + */ > + oldi_mode = OLDI_MODE_CLONE_SINGLE_LINK; > + break; > + > + case DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS: > + /* > + * Note that the OLDI TX 0 transmits the odd set of > + * pixels while the OLDI TX 1 transmits the even set. > + * This is a fixed configuration in the HW and an cannot > + * be change via SW. > + */ > + dev_warn(tidss->dev, > + "EVEN-ODD Dual-Link Mode is not supported in HW.\n"); > + oldi_mode = OLDI_MODE_UNSUPPORTED; > + break; > + > + case DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS: > + oldi_mode = OLDI_MODE_DUAL_LINK; > + break; > + > + default: > + oldi_mode = OLDI_MODE_UNSUPPORTED; > + break; > + } > + } > + > + of_node_put(oldi0_port); > + of_node_put(oldi1_port); > + > + return oldi_mode; > +} > + > static int tidss_dispc_modeset_init(struct tidss_device *tidss) > { > struct device *dev = tidss->dev; > unsigned int fourccs_len; > const u32 *fourccs = dispc_plane_formats(tidss->dispc, &fourccs_len); > - unsigned int i; > + unsigned int i, j; > > struct pipe { > u32 hw_videoport; > - struct drm_bridge *bridge; > + struct drm_bridge *bridge[TIDSS_MAX_BRIDGES_PER_PIPE]; > u32 enc_type; > + u32 num_bridges; > }; > > const struct dispc_features *feat = tidss->feat; > u32 output_ports = feat->num_output_ports; > u32 max_planes = feat->num_planes; > > - struct pipe pipes[TIDSS_MAX_VPS]; > + struct pipe pipes[TIDSS_MAX_VPS] = {0}; > + > u32 num_pipes = 0; > u32 crtc_mask; > + enum dispc_oldi_modes oldi_mode = OLDI_MODE_UNAVAILABLE; > + u32 num_oldi = 0; > + u32 num_encoders = 0; > + u32 oldi_pipe_index = 0; > + > + if (feat->has_oldi) { > + oldi_mode = tidss_get_oldi_mode(tidss); > + > + if ((oldi_mode == OLDI_MODE_DUAL_LINK || > + oldi_mode == OLDI_MODE_CLONE_SINGLE_LINK) && > + feat->subrev == DISPC_AM65X) { > + dev_warn(tidss->dev, > + "am65x-dss does not support this OLDI mode.\n"); > + > + oldi_mode = OLDI_MODE_UNSUPPORTED; > + } Shouldn't OLDI_MODE_UNSUPPORTED be handled as an error? It means the DT is faulty, doesn't it? Maybe it could even be renamed to OLDI_MODE_ERROR. Or tidss_get_oldi_mode() could return a negative error code. > + > + dispc_set_oldi_mode(tidss->dispc, oldi_mode); > + } Would it be better to move the above dispc_set_oldi_mode() to be outside the if block? Then oldi mode would be set to OLDI_MODE_UNAVAILABLE on SoCs that don't have OLDI. tidss_get_oldi_mode and dispc_set_oldi_mode sound like opposites, but they're totally different things. Maybe tidss_get_oldi_mode should rather be something about parsing oldi dt properties or such. > /* first find all the connected panels & bridges */ > > @@ -179,10 +283,87 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss) > } > } > > - pipes[num_pipes].hw_videoport = i; > - pipes[num_pipes].bridge = bridge; > - pipes[num_pipes].enc_type = enc_type; > - num_pipes++; > + if (feat->output_port_bus_type[i] == DISPC_PORT_OLDI) { > + switch (oldi_mode) { > + case OLDI_MODE_UNSUPPORTED: > + case OLDI_MODE_OFF: > + /* > + * Either the OLDI ports are not connected in > + * OF, or their configuration mode is not > + * supported. > + * In both the cases, the OLDI sink ports shall > + * not be logically connected to DSS ports. > + * > + * However, since other dss ports might still > + * be in use (eg, for DPI), the driver shall > + * continue to find the next connected sink in > + * OF. > + */ > + dev_dbg(dev, "OLDI disconnected on port %d\n", i); > + continue; > + > + case OLDI_MODE_DUAL_LINK: > + /* > + * The 2nd OLDI port of a dual-link sink does > + * not require a separate bridge entity. > + */ > + if (num_oldi) { I think if (num_oldi > 0) makes it more readable. > + drm_panel_bridge_remove(bridge); > + continue; > + } > + > + fallthrough; > + > + case OLDI_MODE_CLONE_SINGLE_LINK: > + case OLDI_MODE_SINGLE_LINK: > + /* > + * Setting up pipe parameters when 1st OLDI > + * port is detected. > + */ > + if (!num_oldi) { And here if (num_oldi == 0). > + pipes[num_pipes].hw_videoport = i; > + pipes[num_pipes].enc_type = enc_type; > + > + /* > + * Saving the pipe index in case its > + * required for 2nd OLDI Port. > + */ > + oldi_pipe_index = num_pipes; > + > + /* > + * Incrememnt num_pipe when 1st oldi > + * port is discovered. For the 2nd OLDI > + * port, num_pipe need not be > + * incremented because the 2nd > + * Encoder-to-Bridge connection will > + * still be the part of the first OLDI > + * Port pipe. > + */ > + num_pipes++; > + } > + > + /* > + * Bridge is required to be added only if the > + * detected port is the first OLDI port (of any > + * mode) or a subsequent port in Clone Mode. > + */ If I understand right, you're saying that bridge is not required to be added for the second port for dual link? It maybe be clearer to state that. But if so, isn't this still always adding the bridge here, for all modes? Or what is the comment referring to? Tomi
On 25/01/2023 13:35, Aradhya Bhatia wrote: > The ctrl mmr module of the AM625 is different from the AM65X SoC. Thus > the ctrl mmr registers that supported the OLDI TX power have become > different in AM625 SoC. > > The common mode voltage of the LVDS buffers becomes random when the > bandgap reference is turned off. This causes uncertainity in the LVDS > Data and Clock signal outputs, making it behave differently under > different conditions and panel setups. The bandgap reference must be > powered on before using the OLDI IOs, to keep the common voltage trimmed > down to desired levels. > > Add support to enable/disable OLDI IO signals as well as the bandgap > reference circuit for the LVDS signals. > > Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> > --- > > Note: > - Dropped Tomi Valkeinen's reviewed-by tag in this patch because I did > not implement one of his comments which suggested to remove the > 'oldi_supported' variable. While the oldi support is indeed based on > SoC variations, keeping that variable helps take into account the case > where an OLDI supporting SoC by-passes OLDI TXes and gives out DPI > video signals straight from DSS. Hmm why is that relevent for this patch? It doesn't use oldi_supported or the new has_oldi. > drivers/gpu/drm/tidss/tidss_dispc.c | 57 +++++++++++++++++++----- > drivers/gpu/drm/tidss/tidss_dispc_regs.h | 40 ++++++++++++----- > 2 files changed, 76 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c > index 37a73e309330..0e03557bc142 100644 > --- a/drivers/gpu/drm/tidss/tidss_dispc.c > +++ b/drivers/gpu/drm/tidss/tidss_dispc.c > @@ -934,21 +934,56 @@ int dispc_vp_bus_check(struct dispc_device *dispc, u32 hw_videoport, > > static void dispc_oldi_tx_power(struct dispc_device *dispc, bool power) > { > - u32 val = power ? 0 : OLDI_PWRDN_TX; > + u32 val; > > if (WARN_ON(!dispc->oldi_io_ctrl)) > return; > > - regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT0_IO_CTRL, > - OLDI_PWRDN_TX, val); > - regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT1_IO_CTRL, > - OLDI_PWRDN_TX, val); > - regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT2_IO_CTRL, > - OLDI_PWRDN_TX, val); > - regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT3_IO_CTRL, > - OLDI_PWRDN_TX, val); > - regmap_update_bits(dispc->oldi_io_ctrl, OLDI_CLK_IO_CTRL, > - OLDI_PWRDN_TX, val); > + if (dispc->feat->subrev == DISPC_AM65X) { Slight nitpick, but I think switch-case makes sense for the subrev. Even if there are just two options here, using switch makes the structure clearer. > + val = power ? 0 : AM65X_OLDI_PWRDN_TX; > + > + regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_DAT0_IO_CTRL, > + AM65X_OLDI_PWRDN_TX, val); > + regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_DAT1_IO_CTRL, > + AM65X_OLDI_PWRDN_TX, val); > + regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_DAT2_IO_CTRL, > + AM65X_OLDI_PWRDN_TX, val); > + regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_DAT3_IO_CTRL, > + AM65X_OLDI_PWRDN_TX, val); > + regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_CLK_IO_CTRL, > + AM65X_OLDI_PWRDN_TX, val); > + > + } else if (dispc->feat->subrev == DISPC_AM625) { > + if (power) { > + switch (dispc->oldi_mode) { > + case OLDI_MODE_SINGLE_LINK: > + /* Power down OLDI TX 1 */ > + val = AM625_OLDI1_PWRDN_TX; > + break; > + > + case OLDI_MODE_CLONE_SINGLE_LINK: > + case OLDI_MODE_DUAL_LINK: > + /* No Power down */ > + val = 0; > + break; > + > + default: > + /* Power down both OLDI TXes and LVDS Bandgap */ > + val = AM625_OLDI0_PWRDN_TX | AM625_OLDI1_PWRDN_TX | > + AM625_OLDI_PWRDN_BG; > + break; > + } > + > + } else { > + /* Power down both OLDI TXes and LVDS Bandgap */ > + val = AM625_OLDI0_PWRDN_TX | AM625_OLDI1_PWRDN_TX | > + AM625_OLDI_PWRDN_BG; > + } > + > + regmap_update_bits(dispc->oldi_io_ctrl, AM625_OLDI_PD_CTRL, > + AM625_OLDI0_PWRDN_TX | AM625_OLDI1_PWRDN_TX | > + AM625_OLDI_PWRDN_BG, val); > + } > } > > static void dispc_set_num_datalines(struct dispc_device *dispc, > diff --git a/drivers/gpu/drm/tidss/tidss_dispc_regs.h b/drivers/gpu/drm/tidss/tidss_dispc_regs.h > index 13feedfe5d6d..b2a148e96022 100644 > --- a/drivers/gpu/drm/tidss/tidss_dispc_regs.h > +++ b/drivers/gpu/drm/tidss/tidss_dispc_regs.h > @@ -227,17 +227,37 @@ enum dispc_common_regs { > #define DISPC_VP_DSS_DMA_THREADSIZE_STATUS 0x174 /* J721E */ > > /* > - * OLDI IO_CTRL register offsets. On AM654 the registers are found > - * from CTRL_MMR0, there the syscon regmap should map 0x14 bytes from > - * CTRLMMR0P1_OLDI_DAT0_IO_CTRL to CTRLMMR0P1_OLDI_CLK_IO_CTRL > - * register range. > + * OLDI IO and PD CTRL register offsets. > + * These registers are found in the CTRL_MMR0, where the syscon regmap should map > + * > + * 1. 0x14 bytes from CTRLMMR0P1_OLDI_DAT0_IO_CTRL to CTRLMMR0P1_OLDI_CLK_IO_CTRL > + * register range for the AM65X DSS, and > + * > + * 2. 0x200 bytes from OLDI0_DAT0_IO_CTRL to OLDI_LB_CTRL register range for the > + * AM625 DSS. > */ > -#define OLDI_DAT0_IO_CTRL 0x00 > -#define OLDI_DAT1_IO_CTRL 0x04 > -#define OLDI_DAT2_IO_CTRL 0x08 > -#define OLDI_DAT3_IO_CTRL 0x0C > -#define OLDI_CLK_IO_CTRL 0x10 > > -#define OLDI_PWRDN_TX BIT(8) > +/* -- For AM65X OLDI TX -- */ > +/* Register offsets */ > +#define AM65X_OLDI_DAT0_IO_CTRL 0x00 > +#define AM65X_OLDI_DAT1_IO_CTRL 0x04 > +#define AM65X_OLDI_DAT2_IO_CTRL 0x08 > +#define AM65X_OLDI_DAT3_IO_CTRL 0x0C > +#define AM65X_OLDI_CLK_IO_CTRL 0x10 > + > +/* Power control bits */ > +#define AM65X_OLDI_PWRDN_TX BIT(8) > + > +/* -- For AM625 OLDI TX -- */ > +/* Register offsets */ > +#define AM625_OLDI_PD_CTRL 0x100 > +#define AM625_OLDI_LB_CTRL 0x104 > + > +/* Power control bits */ > +#define AM625_OLDI0_PWRDN_TX BIT(0) > +#define AM625_OLDI1_PWRDN_TX BIT(1) > + > +/* LVDS Bandgap reference Enable/Disable */ > +#define AM625_OLDI_PWRDN_BG BIT(8) > > #endif /* __TIDSS_DISPC_REGS_H */ Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> Tomi
On 25/01/2023 13:35, Aradhya Bhatia wrote: > Add support for the DSS controller on TI's new AM625 SoC in the tidss > driver. > > The first video port (VP0) in am625-dss can output OLDI signals through > 2 OLDI TXes. A 3rd output port has been added with "DISPC_PORT_OLDI" bus > type. Not a big thing here as you add support for a new SoC, but the ordering of the patches is not optimal. Here you add the AM625 DSS support, but then you continue actually adding the DSS support (well, mainly OLDI) in the following patches. I think patch 6 could be before this patch. Parts of patch 4 could also be before this patch. The AM65X renames from patch 5 could be before this patch. I'm mainly thinking of a case where someone uses AM625 and is bisecting a problem. What happens if his board uses OLDI, and he happens to hit one of these patches during bisect? If the display just stays black, but otherwise everything works fine, then no problem. But if it crashes or starts spamming sync losts or such or gives errors, it's not so nice. Tomi
Hi Tomi, Thanks for the review! On 03-Feb-23 16:53, Tomi Valkeinen wrote: > On 25/01/2023 13:35, Aradhya Bhatia wrote: >> Make DSS Video Ports agnostic of output bus types. >> >> DSS controllers have had a 1-to-1 coupling between its VPs and its >> output ports. This no longer stands true for the new AM625 DSS. This >> coupling, hence, has been removed by renaming the 'vp_bus_type' to >> 'output_port_bus_type' because the VPs are essentially agnostic of the >> bus type and it is the output ports which have a bus type. >> >> The AM625 DSS has 2 VPs but requires 3 output ports to support its >> Dual-Link OLDI video output coming from a single VP. > > Not a biggie, but this sentence is a bit odd here at the end. Shouldn't > it be after the "...stands true for the new AM625 DSS."? Yes! It should be. Will make the edit. > >> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> >> --- >> drivers/gpu/drm/tidss/tidss_dispc.c | 47 +++++++++++++++++------------ >> drivers/gpu/drm/tidss/tidss_dispc.h | 21 +++++++------ >> drivers/gpu/drm/tidss/tidss_drv.h | 5 +-- >> drivers/gpu/drm/tidss/tidss_irq.h | 2 +- >> drivers/gpu/drm/tidss/tidss_kms.c | 12 ++++---- >> 5 files changed, 48 insertions(+), 39 deletions(-) >> >> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c >> index 165365b515e1..c1c4faccbddc 100644 >> --- a/drivers/gpu/drm/tidss/tidss_dispc.c >> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c >> @@ -61,7 +61,7 @@ const struct dispc_features dispc_k2g_feats = { >> .min_pclk_khz = 4375, >> .max_pclk_khz = { >> - [DISPC_VP_DPI] = 150000, >> + [DISPC_PORT_DPI] = 150000, >> }, >> /* >> @@ -96,7 +96,6 @@ const struct dispc_features dispc_k2g_feats = { >> .vp_name = { "vp1" }, >> .ovr_name = { "ovr1" }, >> .vpclk_name = { "vp1" }, >> - .vp_bus_type = { DISPC_VP_DPI }, >> .vp_feat = { .color = { >> .has_ctm = true, >> @@ -109,6 +108,9 @@ const struct dispc_features dispc_k2g_feats = { >> .vid_name = { "vid1" }, >> .vid_lite = { false }, >> .vid_order = { 0 }, >> + >> + .num_output_ports = 1, >> + .output_port_bus_type = { DISPC_PORT_DPI }, >> }; > > Just thinking out loud, as these will get more complex in the future, > maybe we should finally group them with struct. E.g. we could define > struct array for vps, like (just hacky example): > > struct { > const char *name; > const char *clkname; > struct tidss_vp_feat feat; > } vps[TIDSS_MAX_PORTS]; > > and then use them as: > > .vps = { > { > .name = "kala", > .clkname = "kissa", > .feat.color.has_ctm = true, > }, { > .name = "kala2", > .clkname = "kissa2", > .feat.color.has_ctm = false, > }, > }, > > Perhaps something to try in the future. > Yes, agreed! Having that structure will tidy this up. I will keep this under future work. >> static const u16 tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { >> @@ -140,8 +142,8 @@ static const u16 tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { >> const struct dispc_features dispc_am65x_feats = { >> .max_pclk_khz = { >> - [DISPC_VP_DPI] = 165000, >> - [DISPC_VP_OLDI] = 165000, >> + [DISPC_PORT_DPI] = 165000, >> + [DISPC_PORT_OLDI] = 165000, >> }, >> .scaling = { >> @@ -171,7 +173,6 @@ const struct dispc_features dispc_am65x_feats = { >> .vp_name = { "vp1", "vp2" }, >> .ovr_name = { "ovr1", "ovr2" }, >> .vpclk_name = { "vp1", "vp2" }, >> - .vp_bus_type = { DISPC_VP_OLDI, DISPC_VP_DPI }, >> .vp_feat = { .color = { >> .has_ctm = true, >> @@ -185,6 +186,9 @@ const struct dispc_features dispc_am65x_feats = { >> .vid_name = { "vid", "vidl1" }, >> .vid_lite = { false, true, }, >> .vid_order = { 1, 0 }, >> + >> + .num_output_ports = 2, >> + .output_port_bus_type = { DISPC_PORT_OLDI, DISPC_PORT_DPI }, >> }; >> static const u16 tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { >> @@ -229,8 +233,8 @@ static const u16 tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { >> const struct dispc_features dispc_j721e_feats = { >> .max_pclk_khz = { >> - [DISPC_VP_DPI] = 170000, >> - [DISPC_VP_INTERNAL] = 600000, >> + [DISPC_PORT_DPI] = 170000, >> + [DISPC_PORT_INTERNAL] = 600000, >> }, >> .scaling = { >> @@ -260,9 +264,7 @@ const struct dispc_features dispc_j721e_feats = { >> .vp_name = { "vp1", "vp2", "vp3", "vp4" }, >> .ovr_name = { "ovr1", "ovr2", "ovr3", "ovr4" }, >> .vpclk_name = { "vp1", "vp2", "vp3", "vp4" }, >> - /* Currently hard coded VP routing (see dispc_initial_config()) */ >> - .vp_bus_type = { DISPC_VP_INTERNAL, DISPC_VP_DPI, >> - DISPC_VP_INTERNAL, DISPC_VP_DPI, }, >> + > > I think this line feed is extra. Okay! Will remove that from all SoC feat structs. > >> .vp_feat = { .color = { >> .has_ctm = true, >> .gamma_size = 1024, >> @@ -273,6 +275,11 @@ const struct dispc_features dispc_j721e_feats = { >> .vid_name = { "vid1", "vidl1", "vid2", "vidl2" }, >> .vid_lite = { 0, 1, 0, 1, }, >> .vid_order = { 1, 3, 0, 2 }, >> + >> + .num_output_ports = 4, >> + /* Currently hard coded VP routing (see dispc_initial_config()) */ >> + .output_port_bus_type = { DISPC_PORT_INTERNAL, DISPC_PORT_DPI, >> + DISPC_PORT_INTERNAL, DISPC_PORT_DPI, }, > > Indent doesn't look right (but it might be just because this is a diff). I may have missed indenting this. > >> }; >> static const u16 *dispc_common_regmap; >> @@ -287,12 +294,12 @@ struct dispc_device { >> void __iomem *base_common; >> void __iomem *base_vid[TIDSS_MAX_PLANES]; >> - void __iomem *base_ovr[TIDSS_MAX_PORTS]; >> - void __iomem *base_vp[TIDSS_MAX_PORTS]; >> + void __iomem *base_ovr[TIDSS_MAX_VPS]; >> + void __iomem *base_vp[TIDSS_MAX_VPS]; >> struct regmap *oldi_io_ctrl; >> - struct clk *vp_clk[TIDSS_MAX_PORTS]; >> + struct clk *vp_clk[TIDSS_MAX_VPS]; >> const struct dispc_features *feat; >> @@ -300,7 +307,7 @@ struct dispc_device { >> bool is_enabled; >> - struct dss_vp_data vp_data[TIDSS_MAX_PORTS]; >> + struct dss_vp_data vp_data[TIDSS_MAX_VPS]; >> u32 *fourccs; >> u32 num_fourccs; >> @@ -851,7 +858,7 @@ int dispc_vp_bus_check(struct dispc_device *dispc, u32 hw_videoport, >> return -EINVAL; >> } >> - if (dispc->feat->vp_bus_type[hw_videoport] != DISPC_VP_OLDI && >> + if (dispc->feat->output_port_bus_type[hw_videoport] != DISPC_PORT_OLDI && > > Hmm, so is the hw_videoport a vp index or an output index? Sounds like > the former, so it's not right, even if at the moment they're identical. > We need some kind of mapping between those. > It is indeed a vp index. And yes, I can come up with a mapping mechanism. > If the mapping can be changed (or just defined in the DT), I think we > need a variable in struct dispc_device, which tells the output to which > a videoport is connected to. Or vice versa, I'm not sure which direction > we need more. If the mapping is always the same on all SoC (but I don't > think so), we can have it in the feats. > As of now, the mapping is always same. But I would like to make is generalized for future. Hence, I am considering to keep the variable in struct dispc_device. My question though would be, how would one be able to find which kind of device is the port connected to, if it is connected to a bridge? For example, in case of panels, we have a "connector_type" variable in drm_panel which tells what kind of sink it is. But there is no such thing in drm_bridge. This is required because what if we can connect an videoport to either an LVDS/OLDI bridge or a DPI bridge. Also, implementing this might mean removal of the part of code which confirms that the panel's "connector_type" indeed expects what the VP can provide, unless there is a way to find out what the sink is before calling the drm_of_find_panel_or_bridge API. On the direction, the primary requirement of hw_videoport has been in the tidss_dispc.c file where the HW registers are getting configured. 'hw_videoport' is a frequently passed parameter in function calls. So, at a first glance the former option might makes more sense for direction, i.e. to have a variable which tells the output to which a videoport is connected to. And while, there is also the tidss_kms.c file, which deals with initializing encoders and attaching bridges. This is where the output_port is required more often. But I am yet to think of a case where the above direction could be an issue. > Also, I wonder if output_port is a good name as it has "port" in it > (like video port), and it's a bit long-ish. Would just "output" be > enough? We could, of course, shorten it to OP, but that looks odd to me =). > >> fmt->is_oldi_fmt) { >> dev_dbg(dispc->dev, "%s: %s is not OLDI-port\n", >> __func__, dispc->feat->vp_name[hw_videoport]); >> @@ -955,7 +962,7 @@ void dispc_vp_prepare(struct dispc_device *dispc, u32 hw_videoport, >> if (WARN_ON(!fmt)) >> return; >> - if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) { >> + if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) { >> dispc_oldi_tx_power(dispc, true); >> dispc_enable_oldi(dispc, hw_videoport, fmt); >> @@ -1014,7 +1021,7 @@ void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport, >> align = true; >> /* always use DE_HIGH for OLDI */ >> - if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) >> + if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) >> ieo = false; >> dispc_vp_write(dispc, hw_videoport, DISPC_VP_POL_FREQ, >> @@ -1040,7 +1047,7 @@ void dispc_vp_disable(struct dispc_device *dispc, u32 hw_videoport) >> void dispc_vp_unprepare(struct dispc_device *dispc, u32 hw_videoport) >> { >> - if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) { >> + if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) { >> dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 0); >> dispc_oldi_tx_power(dispc, false); >> @@ -1116,10 +1123,10 @@ enum drm_mode_status dispc_vp_mode_valid(struct dispc_device *dispc, >> const struct drm_display_mode *mode) >> { >> u32 hsw, hfp, hbp, vsw, vfp, vbp; >> - enum dispc_vp_bus_type bus_type; >> + enum dispc_port_bus_type bus_type; >> int max_pclk; >> - bus_type = dispc->feat->vp_bus_type[hw_videoport]; >> + bus_type = dispc->feat->output_port_bus_type[hw_videoport]; >> max_pclk = dispc->feat->max_pclk_khz[bus_type]; >> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h >> index e49432f0abf5..30fb44158347 100644 >> --- a/drivers/gpu/drm/tidss/tidss_dispc.h >> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h >> @@ -50,11 +50,11 @@ struct dispc_errata { >> bool i2000; /* DSS Does Not Support YUV Pixel Data Formats */ >> }; >> -enum dispc_vp_bus_type { >> - DISPC_VP_DPI, /* DPI output */ >> - DISPC_VP_OLDI, /* OLDI (LVDS) output */ >> - DISPC_VP_INTERNAL, /* SoC internal routing */ >> - DISPC_VP_MAX_BUS_TYPE, >> +enum dispc_port_bus_type { >> + DISPC_PORT_DPI, /* DPI output */ >> + DISPC_PORT_OLDI, /* OLDI (LVDS) output */ >> + DISPC_PORT_INTERNAL, /* SoC internal routing */ >> + DISPC_PORT_MAX_BUS_TYPE, > > Okay, so here you have just "port", not "output_port". In the DT, > they're ports, so... Maybe we could use that name too, and for video > port always use "vp". The current "hw_videoport" could be easily > mistaken with "port". I see what you are saying and how somebody could confusre hw_videoport for a physical connection (i.e. port). I have always understoof hw_videoport to be a thing of the actual VP inside the SoC, but that may be because I have been working on this, and not just trying to understand the code from a high level. How about if I change the output_port to "out_port"? I am okay to keep "output" as the name to. I am saying this becuase I think, only keeping "port" might just confuse more, as you mentioned above. And then we can change "hw_videoport" to "vp_index", perhaps, or even keep that as it is? Becuase if we do have a VP structure in future like you suggested above, "vps" and "vp" would become a close overlap. For eg, "vps[vp]". How do these sound to you? > > I don't recall why I chose to use "hw" prefix there. I think I wanted to > separate it from some other videoport, but... I don't know what that > "other" is =). > Perhaps because just a "videoport" might be confused with a connector on the board, as in "HDMI port"? And the prefix might be a way to clarify that its the DSS controller's VP. =) Regards Aradhya
Hi Tomi, On 03-Feb-23 20:42, Tomi Valkeinen wrote: > On 25/01/2023 13:35, Aradhya Bhatia wrote: >> The newer version of DSS (AM625-DSS) has 2 OLDI TXes at its disposal. >> These can be configured to support the following modes: >> >> 1. OLDI_SINGLE_LINK_SINGLE_MODE >> Single Output over OLDI 0. >> +------+ +---------+ +-------+ >> | | | | | | >> | CRTC +------->+ ENCODER +----->| PANEL | >> | | | | | | >> +------+ +---------+ +-------+ >> >> 2. OLDI_SINGLE_LINK_CLONE_MODE >> Duplicate Output over OLDI 0 and 1. >> +------+ +---------+ +-------+ >> | | | | | | >> | CRTC +---+--->| ENCODER +----->| PANEL | >> | | | | | | | >> +------+ | +---------+ +-------+ >> | >> | +---------+ +-------+ >> | | | | | >> +--->| ENCODER +----->| PANEL | >> | | | | >> +---------+ +-------+ >> >> 3. OLDI_DUAL_LINK_MODE >> Combined Output over OLDI 0 and 1. >> +------+ +---------+ +-------+ >> | | | +----->| | >> | CRTC +------->+ ENCODER | | PANEL | >> | | | +----->| | >> +------+ +---------+ +-------+ >> >> Following the above pathways for different modes, 2 encoder/panel-bridge >> pipes get created for clone mode, and 1 pipe in cases of single link and >> dual link mode. >> >> Add support for confguring the OLDI modes using OF and LVDS DRM helper >> functions. >> >> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> >> --- >> drivers/gpu/drm/tidss/tidss_dispc.c | 24 ++- >> drivers/gpu/drm/tidss/tidss_dispc.h | 12 ++ >> drivers/gpu/drm/tidss/tidss_drv.h | 3 + >> drivers/gpu/drm/tidss/tidss_encoder.c | 4 +- >> drivers/gpu/drm/tidss/tidss_encoder.h | 3 +- >> drivers/gpu/drm/tidss/tidss_kms.c | 221 ++++++++++++++++++++++++-- >> 6 files changed, 245 insertions(+), 22 deletions(-) >> >> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c >> index b55ccbcaa67f..37a73e309330 100644 >> --- a/drivers/gpu/drm/tidss/tidss_dispc.c >> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c >> @@ -88,6 +88,8 @@ const struct dispc_features dispc_k2g_feats = { >> .subrev = DISPC_K2G, >> + .has_oldi = false, >> + >> .common = "common", >> .common_regs = tidss_k2g_common_regs, >> @@ -166,6 +168,8 @@ const struct dispc_features dispc_am625_feats = { >> .subrev = DISPC_AM625, >> + .has_oldi = true, >> + >> .common = "common", >> .common_regs = tidss_am65x_common_regs, >> @@ -218,6 +222,8 @@ const struct dispc_features dispc_am65x_feats = { >> .subrev = DISPC_AM65X, >> + .has_oldi = true, >> + >> .common = "common", >> .common_regs = tidss_am65x_common_regs, >> @@ -309,6 +315,8 @@ const struct dispc_features dispc_j721e_feats = { >> .subrev = DISPC_J721E, >> + .has_oldi = false, >> + >> .common = "common_m", >> .common_regs = tidss_j721e_common_regs, >> @@ -361,6 +369,8 @@ struct dispc_device { >> struct dss_vp_data vp_data[TIDSS_MAX_VPS]; >> + enum dispc_oldi_modes oldi_mode; >> + >> u32 *fourccs; >> u32 num_fourccs; >> @@ -1963,6 +1973,12 @@ const u32 *dispc_plane_formats(struct dispc_device *dispc, unsigned int *len) >> return dispc->fourccs; >> } >> +void dispc_set_oldi_mode(struct dispc_device *dispc, >> + enum dispc_oldi_modes oldi_mode) >> +{ >> + dispc->oldi_mode = oldi_mode; >> +} >> + >> static s32 pixinc(int pixels, u8 ps) >> { >> if (pixels == 1) >> @@ -2647,7 +2663,7 @@ int dispc_runtime_resume(struct dispc_device *dispc) >> REG_GET(dispc, DSS_SYSSTATUS, 2, 2), >> REG_GET(dispc, DSS_SYSSTATUS, 3, 3)); >> - if (dispc->feat->subrev == DISPC_AM65X) >> + if (dispc->feat->has_oldi) >> dev_dbg(dispc->dev, "OLDI RESETDONE %d,%d,%d\n", >> REG_GET(dispc, DSS_SYSSTATUS, 5, 5), >> REG_GET(dispc, DSS_SYSSTATUS, 6, 6), >> @@ -2688,7 +2704,7 @@ static int dispc_iomap_resource(struct platform_device *pdev, const char *name, >> return 0; >> } >> -static int dispc_init_am65x_oldi_io_ctrl(struct device *dev, >> +static int dispc_init_am6xx_oldi_io_ctrl(struct device *dev, >> struct dispc_device *dispc) >> { >> dispc->oldi_io_ctrl = >> @@ -2827,8 +2843,8 @@ int dispc_init(struct tidss_device *tidss) >> dispc->vp_data[i].gamma_table = gamma_table; >> } >> - if (feat->subrev == DISPC_AM65X) { >> - r = dispc_init_am65x_oldi_io_ctrl(dev, dispc); >> + if (feat->has_oldi) { >> + r = dispc_init_am6xx_oldi_io_ctrl(dev, dispc); >> if (r) >> return r; >> } >> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h >> index 971f2856f015..880bc7de68b3 100644 >> --- a/drivers/gpu/drm/tidss/tidss_dispc.h >> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h >> @@ -64,6 +64,15 @@ enum dispc_dss_subrevision { >> DISPC_J721E, >> }; >> +enum dispc_oldi_modes { >> + OLDI_MODE_SINGLE_LINK, /* Single output over OLDI 0. */ >> + OLDI_MODE_CLONE_SINGLE_LINK, /* Cloned output over OLDI 0 and 1. */ >> + OLDI_MODE_DUAL_LINK, /* Combined output over OLDI 0 and 1. */ >> + OLDI_MODE_OFF, /* OLDI TXes not connected in OF. */ >> + OLDI_MODE_UNSUPPORTED, /* Unsupported OLDI configuration in OF. */ >> + OLDI_MODE_UNAVAILABLE, /* OLDI TXes not available in SoC. */ >> +}; >> + >> struct dispc_features { >> int min_pclk_khz; >> int max_pclk_khz[DISPC_PORT_MAX_BUS_TYPE]; >> @@ -72,6 +81,8 @@ struct dispc_features { >> enum dispc_dss_subrevision subrev; >> + bool has_oldi; >> + >> const char *common; >> const u16 *common_regs; >> u32 num_vps; >> @@ -131,6 +142,7 @@ int dispc_plane_setup(struct dispc_device *dispc, u32 hw_plane, >> u32 hw_videoport); >> int dispc_plane_enable(struct dispc_device *dispc, u32 hw_plane, bool enable); >> const u32 *dispc_plane_formats(struct dispc_device *dispc, unsigned int *len); >> +void dispc_set_oldi_mode(struct dispc_device *dispc, enum dispc_oldi_modes oldi_mode); >> int dispc_init(struct tidss_device *tidss); >> void dispc_remove(struct tidss_device *tidss); >> diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h >> index 0ce7ee5ccd5b..58892f065c16 100644 >> --- a/drivers/gpu/drm/tidss/tidss_drv.h >> +++ b/drivers/gpu/drm/tidss/tidss_drv.h >> @@ -13,6 +13,9 @@ >> #define TIDSS_MAX_PLANES 4 >> #define TIDSS_MAX_OUTPUT_PORTS 4 >> +/* For AM625-DSS with 2 OLDI TXes */ >> +#define TIDSS_MAX_BRIDGES_PER_PIPE 2 >> + >> typedef u32 dispc_irq_t; >> struct tidss_device { >> diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c b/drivers/gpu/drm/tidss/tidss_encoder.c >> index 0d4865e9c03d..bd2a7358d7b0 100644 >> --- a/drivers/gpu/drm/tidss/tidss_encoder.c >> +++ b/drivers/gpu/drm/tidss/tidss_encoder.c >> @@ -70,7 +70,8 @@ static const struct drm_encoder_funcs encoder_funcs = { >> }; >> struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss, >> - u32 encoder_type, u32 possible_crtcs) >> + u32 encoder_type, u32 possible_crtcs, >> + u32 possible_clones) >> { >> struct drm_encoder *enc; >> int ret; >> @@ -80,6 +81,7 @@ struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss, >> return ERR_PTR(-ENOMEM); >> enc->possible_crtcs = possible_crtcs; >> + enc->possible_clones = possible_clones; >> ret = drm_encoder_init(&tidss->ddev, enc, &encoder_funcs, >> encoder_type, NULL); >> diff --git a/drivers/gpu/drm/tidss/tidss_encoder.h b/drivers/gpu/drm/tidss/tidss_encoder.h >> index ace877c0e0fd..01c62ba3ef16 100644 >> --- a/drivers/gpu/drm/tidss/tidss_encoder.h >> +++ b/drivers/gpu/drm/tidss/tidss_encoder.h >> @@ -12,6 +12,7 @@ >> struct tidss_device; >> struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss, >> - u32 encoder_type, u32 possible_crtcs); >> + u32 encoder_type, u32 possible_crtcs, >> + u32 possible_clones); >> #endif >> diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c >> index d449131935d2..8322ee6310bf 100644 >> --- a/drivers/gpu/drm/tidss/tidss_kms.c >> +++ b/drivers/gpu/drm/tidss/tidss_kms.c >> @@ -13,6 +13,7 @@ >> #include <drm/drm_of.h> >> #include <drm/drm_panel.h> >> #include <drm/drm_vblank.h> >> +#include <linux/of.h> >> #include "tidss_crtc.h" >> #include "tidss_dispc.h" >> @@ -104,26 +105,129 @@ static const struct drm_mode_config_funcs mode_config_funcs = { >> .atomic_commit = drm_atomic_helper_commit, >> }; >> +static enum dispc_oldi_modes tidss_get_oldi_mode(struct tidss_device *tidss) >> +{ >> + int pixel_order; >> + enum dispc_oldi_modes oldi_mode; >> + struct device_node *oldi0_port, *oldi1_port; >> + >> + /* >> + * For am625-dss, the OLDI ports are expected at port reg = 0 and 2, >> + * and for am65x-dss, the OLDI port is expected only at port reg = 0. >> + */ >> + const u32 portnum_oldi0 = 0, portnum_oldi1 = 2; >> + >> + oldi0_port = of_graph_get_port_by_id(tidss->dev->of_node, portnum_oldi0); >> + oldi1_port = of_graph_get_port_by_id(tidss->dev->of_node, portnum_oldi1); >> + >> + if (!(oldi0_port || oldi1_port)) { >> + /* Keep OLDI TXes OFF if neither OLDI port is present. */ >> + oldi_mode = OLDI_MODE_OFF; >> + } else if (oldi0_port && !oldi1_port) { >> + /* >> + * OLDI0 port found, but not OLDI1 port. Setting single >> + * link output mode. >> + */ >> + oldi_mode = OLDI_MODE_SINGLE_LINK; >> + } else if (!oldi0_port && oldi1_port) { >> + /* >> + * The 2nd OLDI TX cannot be operated alone. This use case is >> + * not supported in the HW. Since the pins for OLDIs 0 and 1 are >> + * separate, one could theoretically set a clone mode over OLDIs >> + * 0 and 1 and just simply not use the OLDI 0. This is a hacky >> + * way to enable only OLDI TX 1 and hence is not officially >> + * supported. >> + */ >> + dev_warn(tidss->dev, >> + "Single Mode over OLDI 1 is not supported in HW.\n"); >> + oldi_mode = OLDI_MODE_UNSUPPORTED; >> + } else { >> + /* >> + * OLDI Ports found for both the OLDI TXes. The DSS is to be >> + * configured in either Dual Link or Clone Mode. >> + */ >> + pixel_order = drm_of_lvds_get_dual_link_pixel_order(oldi0_port, >> + oldi1_port); >> + switch (pixel_order) { >> + case -EINVAL: >> + /* >> + * The dual link properties were not found in at least >> + * one of the sink nodes. Since 2 OLDI ports are present >> + * in the DT, it can be safely assumed that the required >> + * configuration is Clone Mode. >> + */ >> + oldi_mode = OLDI_MODE_CLONE_SINGLE_LINK; >> + break; >> + >> + case DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS: >> + /* >> + * Note that the OLDI TX 0 transmits the odd set of >> + * pixels while the OLDI TX 1 transmits the even set. >> + * This is a fixed configuration in the HW and an cannot >> + * be change via SW. >> + */ >> + dev_warn(tidss->dev, >> + "EVEN-ODD Dual-Link Mode is not supported in HW.\n"); >> + oldi_mode = OLDI_MODE_UNSUPPORTED; >> + break; >> + >> + case DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS: >> + oldi_mode = OLDI_MODE_DUAL_LINK; >> + break; >> + >> + default: >> + oldi_mode = OLDI_MODE_UNSUPPORTED; >> + break; >> + } >> + } >> + >> + of_node_put(oldi0_port); >> + of_node_put(oldi1_port); >> + >> + return oldi_mode; >> +} >> + >> static int tidss_dispc_modeset_init(struct tidss_device *tidss) >> { >> struct device *dev = tidss->dev; >> unsigned int fourccs_len; >> const u32 *fourccs = dispc_plane_formats(tidss->dispc, &fourccs_len); >> - unsigned int i; >> + unsigned int i, j; >> struct pipe { >> u32 hw_videoport; >> - struct drm_bridge *bridge; >> + struct drm_bridge *bridge[TIDSS_MAX_BRIDGES_PER_PIPE]; >> u32 enc_type; >> + u32 num_bridges; >> }; >> const struct dispc_features *feat = tidss->feat; >> u32 output_ports = feat->num_output_ports; >> u32 max_planes = feat->num_planes; >> - struct pipe pipes[TIDSS_MAX_VPS]; >> + struct pipe pipes[TIDSS_MAX_VPS] = {0}; >> + >> u32 num_pipes = 0; >> u32 crtc_mask; >> + enum dispc_oldi_modes oldi_mode = OLDI_MODE_UNAVAILABLE; >> + u32 num_oldi = 0; >> + u32 num_encoders = 0; >> + u32 oldi_pipe_index = 0; >> + >> + if (feat->has_oldi) { >> + oldi_mode = tidss_get_oldi_mode(tidss); >> + >> + if ((oldi_mode == OLDI_MODE_DUAL_LINK || >> + oldi_mode == OLDI_MODE_CLONE_SINGLE_LINK) && >> + feat->subrev == DISPC_AM65X) { >> + dev_warn(tidss->dev, >> + "am65x-dss does not support this OLDI mode.\n"); >> + >> + oldi_mode = OLDI_MODE_UNSUPPORTED; >> + } > > Shouldn't OLDI_MODE_UNSUPPORTED be handled as an error? It means the DT > is faulty, doesn't it? Maybe it could even be renamed to > OLDI_MODE_ERROR. Or tidss_get_oldi_mode() could return a negative error > code. > The idea was to let the framework continue configuring the 2nd videoport for DPI, even if the OLDI DT is wrong. But I have come across more examples recently where that is not the case. DT error for one pipe has resulted in returning of an error code. Will make the change. >> + >> + dispc_set_oldi_mode(tidss->dispc, oldi_mode); >> + } > > Would it be better to move the above dispc_set_oldi_mode() to be outside > the if block? Then oldi mode would be set to OLDI_MODE_UNAVAILABLE on > SoCs that don't have OLDI. Ahh, yes! Will make the change. > > tidss_get_oldi_mode and dispc_set_oldi_mode sound like opposites, but > they're totally different things. Maybe tidss_get_oldi_mode should > rather be something about parsing oldi dt properties or such. Okay! Is 'tidss_parse_oldi_properties' acceptable? This is just something I came up with now. I can think of more if this is not good. > >> /* first find all the connected panels & bridges */ >> @@ -179,10 +283,87 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss) >> } >> } >> - pipes[num_pipes].hw_videoport = i; >> - pipes[num_pipes].bridge = bridge; >> - pipes[num_pipes].enc_type = enc_type; >> - num_pipes++; >> + if (feat->output_port_bus_type[i] == DISPC_PORT_OLDI) { >> + switch (oldi_mode) { >> + case OLDI_MODE_UNSUPPORTED: >> + case OLDI_MODE_OFF: >> + /* >> + * Either the OLDI ports are not connected in >> + * OF, or their configuration mode is not >> + * supported. >> + * In both the cases, the OLDI sink ports shall >> + * not be logically connected to DSS ports. >> + * >> + * However, since other dss ports might still >> + * be in use (eg, for DPI), the driver shall >> + * continue to find the next connected sink in >> + * OF. >> + */ >> + dev_dbg(dev, "OLDI disconnected on port %d\n", i); >> + continue; >> + >> + case OLDI_MODE_DUAL_LINK: >> + /* >> + * The 2nd OLDI port of a dual-link sink does >> + * not require a separate bridge entity. >> + */ >> + if (num_oldi) { > > I think if (num_oldi > 0) makes it more readable. Alright! > >> + drm_panel_bridge_remove(bridge); >> + continue; >> + } >> + >> + fallthrough; >> + >> + case OLDI_MODE_CLONE_SINGLE_LINK: >> + case OLDI_MODE_SINGLE_LINK: >> + /* >> + * Setting up pipe parameters when 1st OLDI >> + * port is detected. >> + */ >> + if (!num_oldi) { > > And here if (num_oldi == 0). Okay! > >> + pipes[num_pipes].hw_videoport = i; >> + pipes[num_pipes].enc_type = enc_type; >> + >> + /* >> + * Saving the pipe index in case its >> + * required for 2nd OLDI Port. >> + */ >> + oldi_pipe_index = num_pipes; >> + >> + /* >> + * Incrememnt num_pipe when 1st oldi >> + * port is discovered. For the 2nd OLDI >> + * port, num_pipe need not be >> + * incremented because the 2nd >> + * Encoder-to-Bridge connection will >> + * still be the part of the first OLDI >> + * Port pipe. >> + */ >> + num_pipes++; >> + } >> + >> + /* >> + * Bridge is required to be added only if the >> + * detected port is the first OLDI port (of any >> + * mode) or a subsequent port in Clone Mode. >> + */ > > If I understand right, you're saying that bridge is not required to be > added for the second port for dual link? It maybe be clearer to state > that. But if so, isn't this still always adding the bridge here, for all > modes? Or what is the comment referring to? > Well, yes. I am saying that the bridge should be added only when either one of the following 2 DT cases are detected. i. First oldi DT (of any mode, including clone) OR ii. Second oldi DT only under clone mode Bridge is not required for the 2nd DT connection of Dual Link mode. I will make the comment clearer. If the loop is parsing the port under dual link mode, this part of the code will not be reached because of the "continue" clause above under the OLDI_MODE_DUAL_LINK case. Moreover, there won't be a 2nd port to parse if the mode is single link. That said, I can provide an if condition which ensures that, that code is only run when above conditions are met. Regards Aradhya
On 03-Feb-23 20:49, Tomi Valkeinen wrote: > On 25/01/2023 13:35, Aradhya Bhatia wrote: >> The ctrl mmr module of the AM625 is different from the AM65X SoC. Thus >> the ctrl mmr registers that supported the OLDI TX power have become >> different in AM625 SoC. >> >> The common mode voltage of the LVDS buffers becomes random when the >> bandgap reference is turned off. This causes uncertainity in the LVDS >> Data and Clock signal outputs, making it behave differently under >> different conditions and panel setups. The bandgap reference must be >> powered on before using the OLDI IOs, to keep the common voltage trimmed >> down to desired levels. >> >> Add support to enable/disable OLDI IO signals as well as the bandgap >> reference circuit for the LVDS signals. >> >> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> >> --- >> >> Note: >> - Dropped Tomi Valkeinen's reviewed-by tag in this patch because I did >> not implement one of his comments which suggested to remove the >> 'oldi_supported' variable. While the oldi support is indeed based on >> SoC variations, keeping that variable helps take into account the case >> where an OLDI supporting SoC by-passes OLDI TXes and gives out DPI >> video signals straight from DSS. > > Hmm why is that relevent for this patch? It doesn't use oldi_supported > or the new has_oldi. It doesn't. Not directly atleast. In the previous version of this patch, there was a mention of 'oldi_supported' and your tag was conditional to that variable getting removed. Instead, I renamed the variable. > >> drivers/gpu/drm/tidss/tidss_dispc.c | 57 +++++++++++++++++++----- >> drivers/gpu/drm/tidss/tidss_dispc_regs.h | 40 ++++++++++++----- >> 2 files changed, 76 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c >> index 37a73e309330..0e03557bc142 100644 >> --- a/drivers/gpu/drm/tidss/tidss_dispc.c >> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c >> @@ -934,21 +934,56 @@ int dispc_vp_bus_check(struct dispc_device *dispc, u32 hw_videoport, >> static void dispc_oldi_tx_power(struct dispc_device *dispc, bool power) >> { >> - u32 val = power ? 0 : OLDI_PWRDN_TX; >> + u32 val; >> if (WARN_ON(!dispc->oldi_io_ctrl)) >> return; >> - regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT0_IO_CTRL, >> - OLDI_PWRDN_TX, val); >> - regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT1_IO_CTRL, >> - OLDI_PWRDN_TX, val); >> - regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT2_IO_CTRL, >> - OLDI_PWRDN_TX, val); >> - regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT3_IO_CTRL, >> - OLDI_PWRDN_TX, val); >> - regmap_update_bits(dispc->oldi_io_ctrl, OLDI_CLK_IO_CTRL, >> - OLDI_PWRDN_TX, val); >> + if (dispc->feat->subrev == DISPC_AM65X) { > > Slight nitpick, but I think switch-case makes sense for the subrev. Even > if there are just two options here, using switch makes the structure clearer. Alright. I will make the edit. > >> + val = power ? 0 : AM65X_OLDI_PWRDN_TX; >> + >> + regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_DAT0_IO_CTRL, >> + AM65X_OLDI_PWRDN_TX, val); >> + regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_DAT1_IO_CTRL, >> + AM65X_OLDI_PWRDN_TX, val); >> + regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_DAT2_IO_CTRL, >> + AM65X_OLDI_PWRDN_TX, val); >> + regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_DAT3_IO_CTRL, >> + AM65X_OLDI_PWRDN_TX, val); >> + regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_CLK_IO_CTRL, >> + AM65X_OLDI_PWRDN_TX, val); >> + >> + } else if (dispc->feat->subrev == DISPC_AM625) { >> + if (power) { >> + switch (dispc->oldi_mode) { >> + case OLDI_MODE_SINGLE_LINK: >> + /* Power down OLDI TX 1 */ >> + val = AM625_OLDI1_PWRDN_TX; >> + break; >> + >> + case OLDI_MODE_CLONE_SINGLE_LINK: >> + case OLDI_MODE_DUAL_LINK: >> + /* No Power down */ >> + val = 0; >> + break; >> + >> + default: >> + /* Power down both OLDI TXes and LVDS Bandgap */ >> + val = AM625_OLDI0_PWRDN_TX | AM625_OLDI1_PWRDN_TX | >> + AM625_OLDI_PWRDN_BG; >> + break; >> + } >> + >> + } else { >> + /* Power down both OLDI TXes and LVDS Bandgap */ >> + val = AM625_OLDI0_PWRDN_TX | AM625_OLDI1_PWRDN_TX | >> + AM625_OLDI_PWRDN_BG; >> + } >> + >> + regmap_update_bits(dispc->oldi_io_ctrl, AM625_OLDI_PD_CTRL, >> + AM625_OLDI0_PWRDN_TX | AM625_OLDI1_PWRDN_TX | >> + AM625_OLDI_PWRDN_BG, val); >> + } >> } >> static void dispc_set_num_datalines(struct dispc_device *dispc, >> diff --git a/drivers/gpu/drm/tidss/tidss_dispc_regs.h b/drivers/gpu/drm/tidss/tidss_dispc_regs.h >> index 13feedfe5d6d..b2a148e96022 100644 >> --- a/drivers/gpu/drm/tidss/tidss_dispc_regs.h >> +++ b/drivers/gpu/drm/tidss/tidss_dispc_regs.h >> @@ -227,17 +227,37 @@ enum dispc_common_regs { >> #define DISPC_VP_DSS_DMA_THREADSIZE_STATUS 0x174 /* J721E */ >> /* >> - * OLDI IO_CTRL register offsets. On AM654 the registers are found >> - * from CTRL_MMR0, there the syscon regmap should map 0x14 bytes from >> - * CTRLMMR0P1_OLDI_DAT0_IO_CTRL to CTRLMMR0P1_OLDI_CLK_IO_CTRL >> - * register range. >> + * OLDI IO and PD CTRL register offsets. >> + * These registers are found in the CTRL_MMR0, where the syscon regmap should map >> + * >> + * 1. 0x14 bytes from CTRLMMR0P1_OLDI_DAT0_IO_CTRL to CTRLMMR0P1_OLDI_CLK_IO_CTRL >> + * register range for the AM65X DSS, and >> + * >> + * 2. 0x200 bytes from OLDI0_DAT0_IO_CTRL to OLDI_LB_CTRL register range for the >> + * AM625 DSS. >> */ >> -#define OLDI_DAT0_IO_CTRL 0x00 >> -#define OLDI_DAT1_IO_CTRL 0x04 >> -#define OLDI_DAT2_IO_CTRL 0x08 >> -#define OLDI_DAT3_IO_CTRL 0x0C >> -#define OLDI_CLK_IO_CTRL 0x10 >> -#define OLDI_PWRDN_TX BIT(8) >> +/* -- For AM65X OLDI TX -- */ >> +/* Register offsets */ >> +#define AM65X_OLDI_DAT0_IO_CTRL 0x00 >> +#define AM65X_OLDI_DAT1_IO_CTRL 0x04 >> +#define AM65X_OLDI_DAT2_IO_CTRL 0x08 >> +#define AM65X_OLDI_DAT3_IO_CTRL 0x0C >> +#define AM65X_OLDI_CLK_IO_CTRL 0x10 >> + >> +/* Power control bits */ >> +#define AM65X_OLDI_PWRDN_TX BIT(8) >> + >> +/* -- For AM625 OLDI TX -- */ >> +/* Register offsets */ >> +#define AM625_OLDI_PD_CTRL 0x100 >> +#define AM625_OLDI_LB_CTRL 0x104 >> + >> +/* Power control bits */ >> +#define AM625_OLDI0_PWRDN_TX BIT(0) >> +#define AM625_OLDI1_PWRDN_TX BIT(1) >> + >> +/* LVDS Bandgap reference Enable/Disable */ >> +#define AM625_OLDI_PWRDN_BG BIT(8) >> #endif /* __TIDSS_DISPC_REGS_H */ > > Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > > Tomi > Regards Aradhya
On 03-Feb-23 21:03, Tomi Valkeinen wrote: > On 25/01/2023 13:35, Aradhya Bhatia wrote: >> Add support for the DSS controller on TI's new AM625 SoC in the tidss >> driver. >> >> The first video port (VP0) in am625-dss can output OLDI signals through >> 2 OLDI TXes. A 3rd output port has been added with "DISPC_PORT_OLDI" bus >> type. > > Not a big thing here as you add support for a new SoC, but the ordering > of the patches is not optimal. Here you add the AM625 DSS support, but > then you continue actually adding the DSS support (well, mainly OLDI) in > the following patches. > > I think patch 6 could be before this patch. Parts of patch 4 could also > be before this patch. The AM65X renames from patch 5 could be before > this patch. I can move whole of Patch 6 and even of Patch 4 before this one. I have mentioned 'AM625-DSS' in a couple comments which I can make generic, and the rest everything is SoC-agnostic. I haven't tried this, but my concern is if we break patch 5 into 2 separate patches, i. AM65X rename plus SoC based switch case, and ii. Addition of AM625 SoC case then I might have to overwrite some changes implemented during (i) in (ii). I don't suppose that would be okay, would it? Also, is it important to keep the compatible-addition patches of DT-binding and driver next to each other in the series? Or should the DT-binding patches should be the first ones? Just curious! =) > > I'm mainly thinking of a case where someone uses AM625 and is bisecting > a problem. What happens if his board uses OLDI, and he happens to hit > one of these patches during bisect? If the display just stays black, but > otherwise everything works fine, then no problem. But if it crashes or > starts spamming sync losts or such or gives errors, it's not so nice. > You are right! This certainly makes sense. Regards Aradhya
On 05/02/2023 16:31, Aradhya Bhatia wrote: > > > On 03-Feb-23 21:03, Tomi Valkeinen wrote: >> On 25/01/2023 13:35, Aradhya Bhatia wrote: >>> Add support for the DSS controller on TI's new AM625 SoC in the tidss >>> driver. >>> >>> The first video port (VP0) in am625-dss can output OLDI signals through >>> 2 OLDI TXes. A 3rd output port has been added with "DISPC_PORT_OLDI" bus >>> type. >> >> Not a big thing here as you add support for a new SoC, but the ordering >> of the patches is not optimal. Here you add the AM625 DSS support, but >> then you continue actually adding the DSS support (well, mainly OLDI) in >> the following patches. >> >> I think patch 6 could be before this patch. Parts of patch 4 could also >> be before this patch. The AM65X renames from patch 5 could be before >> this patch. > > I can move whole of Patch 6 and even of Patch 4 before this one. I have > mentioned 'AM625-DSS' in a couple comments which I can make generic, > and the rest everything is SoC-agnostic. > > I haven't tried this, but my concern is if we break patch 5 into 2 > separate patches, > > i. AM65X rename plus SoC based switch case, and > ii. Addition of AM625 SoC case > > then I might have to overwrite some changes implemented during (i) in > (ii). I don't suppose that would be okay, would it? I'm not sure I follow here. Wouldn't (i) be a valid patch in its own? Nothing wrong in expanding that later (even if you end up changing a lot of it). That said, I don't think this is a very important topic. There are only a few commits in the history that might be problematic. A simple fix would be to add all the features first, and only last add the compatible string for am625. Or do all the changes for am625 in a single patch, and try to implement all the generic restructuring work before that. Here we do have to change the vp-to-output mapping management, so maybe the second option won't be simple enough, and it's better to do the am625 changes in pieces, as in the first option. So, it's really up to you. Just wanted to raise this possible issue so that you are aware of it and can do any easy fixes (if there are such). > Also, is it important to keep the compatible-addition patches of > DT-binding and driver next to each other in the series? Or should > the DT-binding patches should be the first ones? Just curious! =) I believe the convention is to have the DT-binding changes before you add the compatible string to the driver (if I recall right checkpatch or some other checking tool complains if you add a driver for a compatible that doesn't have a DT binding). Generic restructurings could be before the DT patch, of course, but usually I like to keep the DT binding changes at the very beginning of the series. Tomi
On 05/02/2023 15:08, Aradhya Bhatia wrote: > Hi Tomi, > > Thanks for the review! > > On 03-Feb-23 16:53, Tomi Valkeinen wrote: >> On 25/01/2023 13:35, Aradhya Bhatia wrote: >>> Make DSS Video Ports agnostic of output bus types. >>> >>> DSS controllers have had a 1-to-1 coupling between its VPs and its >>> output ports. This no longer stands true for the new AM625 DSS. This >>> coupling, hence, has been removed by renaming the 'vp_bus_type' to >>> 'output_port_bus_type' because the VPs are essentially agnostic of the >>> bus type and it is the output ports which have a bus type. >>> >>> The AM625 DSS has 2 VPs but requires 3 output ports to support its >>> Dual-Link OLDI video output coming from a single VP. >> >> Not a biggie, but this sentence is a bit odd here at the end. Shouldn't >> it be after the "...stands true for the new AM625 DSS."? > > Yes! It should be. Will make the edit. > >> >>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> >>> --- >>> drivers/gpu/drm/tidss/tidss_dispc.c | 47 +++++++++++++++++------------ >>> drivers/gpu/drm/tidss/tidss_dispc.h | 21 +++++++------ >>> drivers/gpu/drm/tidss/tidss_drv.h | 5 +-- >>> drivers/gpu/drm/tidss/tidss_irq.h | 2 +- >>> drivers/gpu/drm/tidss/tidss_kms.c | 12 ++++---- >>> 5 files changed, 48 insertions(+), 39 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c >>> index 165365b515e1..c1c4faccbddc 100644 >>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c >>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c >>> @@ -61,7 +61,7 @@ const struct dispc_features dispc_k2g_feats = { >>> .min_pclk_khz = 4375, >>> .max_pclk_khz = { >>> - [DISPC_VP_DPI] = 150000, >>> + [DISPC_PORT_DPI] = 150000, >>> }, >>> /* >>> @@ -96,7 +96,6 @@ const struct dispc_features dispc_k2g_feats = { >>> .vp_name = { "vp1" }, >>> .ovr_name = { "ovr1" }, >>> .vpclk_name = { "vp1" }, >>> - .vp_bus_type = { DISPC_VP_DPI }, >>> .vp_feat = { .color = { >>> .has_ctm = true, >>> @@ -109,6 +108,9 @@ const struct dispc_features dispc_k2g_feats = { >>> .vid_name = { "vid1" }, >>> .vid_lite = { false }, >>> .vid_order = { 0 }, >>> + >>> + .num_output_ports = 1, >>> + .output_port_bus_type = { DISPC_PORT_DPI }, >>> }; >> >> Just thinking out loud, as these will get more complex in the future, >> maybe we should finally group them with struct. E.g. we could define >> struct array for vps, like (just hacky example): >> >> struct { >> const char *name; >> const char *clkname; >> struct tidss_vp_feat feat; >> } vps[TIDSS_MAX_PORTS]; >> >> and then use them as: >> >> .vps = { >> { >> .name = "kala", >> .clkname = "kissa", >> .feat.color.has_ctm = true, >> }, { >> .name = "kala2", >> .clkname = "kissa2", >> .feat.color.has_ctm = false, >> }, >> }, >> >> Perhaps something to try in the future. >> > > Yes, agreed! Having that structure will tidy this up. > I will keep this under future work. > >>> static const u16 tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { >>> @@ -140,8 +142,8 @@ static const u16 tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { >>> const struct dispc_features dispc_am65x_feats = { >>> .max_pclk_khz = { >>> - [DISPC_VP_DPI] = 165000, >>> - [DISPC_VP_OLDI] = 165000, >>> + [DISPC_PORT_DPI] = 165000, >>> + [DISPC_PORT_OLDI] = 165000, >>> }, >>> .scaling = { >>> @@ -171,7 +173,6 @@ const struct dispc_features dispc_am65x_feats = { >>> .vp_name = { "vp1", "vp2" }, >>> .ovr_name = { "ovr1", "ovr2" }, >>> .vpclk_name = { "vp1", "vp2" }, >>> - .vp_bus_type = { DISPC_VP_OLDI, DISPC_VP_DPI }, >>> .vp_feat = { .color = { >>> .has_ctm = true, >>> @@ -185,6 +186,9 @@ const struct dispc_features dispc_am65x_feats = { >>> .vid_name = { "vid", "vidl1" }, >>> .vid_lite = { false, true, }, >>> .vid_order = { 1, 0 }, >>> + >>> + .num_output_ports = 2, >>> + .output_port_bus_type = { DISPC_PORT_OLDI, DISPC_PORT_DPI }, >>> }; >>> static const u16 tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { >>> @@ -229,8 +233,8 @@ static const u16 tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { >>> const struct dispc_features dispc_j721e_feats = { >>> .max_pclk_khz = { >>> - [DISPC_VP_DPI] = 170000, >>> - [DISPC_VP_INTERNAL] = 600000, >>> + [DISPC_PORT_DPI] = 170000, >>> + [DISPC_PORT_INTERNAL] = 600000, >>> }, >>> .scaling = { >>> @@ -260,9 +264,7 @@ const struct dispc_features dispc_j721e_feats = { >>> .vp_name = { "vp1", "vp2", "vp3", "vp4" }, >>> .ovr_name = { "ovr1", "ovr2", "ovr3", "ovr4" }, >>> .vpclk_name = { "vp1", "vp2", "vp3", "vp4" }, >>> - /* Currently hard coded VP routing (see dispc_initial_config()) */ >>> - .vp_bus_type = { DISPC_VP_INTERNAL, DISPC_VP_DPI, >>> - DISPC_VP_INTERNAL, DISPC_VP_DPI, }, >>> + >> >> I think this line feed is extra. > > Okay! Will remove that from all SoC feat structs. > >> >>> .vp_feat = { .color = { >>> .has_ctm = true, >>> .gamma_size = 1024, >>> @@ -273,6 +275,11 @@ const struct dispc_features dispc_j721e_feats = { >>> .vid_name = { "vid1", "vidl1", "vid2", "vidl2" }, >>> .vid_lite = { 0, 1, 0, 1, }, >>> .vid_order = { 1, 3, 0, 2 }, >>> + >>> + .num_output_ports = 4, >>> + /* Currently hard coded VP routing (see dispc_initial_config()) */ >>> + .output_port_bus_type = { DISPC_PORT_INTERNAL, DISPC_PORT_DPI, >>> + DISPC_PORT_INTERNAL, DISPC_PORT_DPI, }, >> >> Indent doesn't look right (but it might be just because this is a diff). > > I may have missed indenting this. > >> >>> }; >>> static const u16 *dispc_common_regmap; >>> @@ -287,12 +294,12 @@ struct dispc_device { >>> void __iomem *base_common; >>> void __iomem *base_vid[TIDSS_MAX_PLANES]; >>> - void __iomem *base_ovr[TIDSS_MAX_PORTS]; >>> - void __iomem *base_vp[TIDSS_MAX_PORTS]; >>> + void __iomem *base_ovr[TIDSS_MAX_VPS]; >>> + void __iomem *base_vp[TIDSS_MAX_VPS]; >>> struct regmap *oldi_io_ctrl; >>> - struct clk *vp_clk[TIDSS_MAX_PORTS]; >>> + struct clk *vp_clk[TIDSS_MAX_VPS]; >>> const struct dispc_features *feat; >>> @@ -300,7 +307,7 @@ struct dispc_device { >>> bool is_enabled; >>> - struct dss_vp_data vp_data[TIDSS_MAX_PORTS]; >>> + struct dss_vp_data vp_data[TIDSS_MAX_VPS]; >>> u32 *fourccs; >>> u32 num_fourccs; >>> @@ -851,7 +858,7 @@ int dispc_vp_bus_check(struct dispc_device *dispc, u32 hw_videoport, >>> return -EINVAL; >>> } >>> - if (dispc->feat->vp_bus_type[hw_videoport] != DISPC_VP_OLDI && >>> + if (dispc->feat->output_port_bus_type[hw_videoport] != DISPC_PORT_OLDI && >> >> Hmm, so is the hw_videoport a vp index or an output index? Sounds like >> the former, so it's not right, even if at the moment they're identical. >> We need some kind of mapping between those. >> > > It is indeed a vp index. And yes, I can come up with a mapping mechanism. > >> If the mapping can be changed (or just defined in the DT), I think we >> need a variable in struct dispc_device, which tells the output to which >> a videoport is connected to. Or vice versa, I'm not sure which direction >> we need more. If the mapping is always the same on all SoC (but I don't >> think so), we can have it in the feats. >> > > As of now, the mapping is always same. But I would like to make is > generalized for future. Hence, I am considering to keep the variable in > struct dispc_device. > > My question though would be, how would one be able to find which kind > of device is the port connected to, if it is connected to a bridge? For > example, in case of panels, we have a "connector_type" variable in > drm_panel which tells what kind of sink it is. But there is no such > thing in drm_bridge. > > This is required because what if we can connect an videoport to either > an LVDS/OLDI bridge or a DPI bridge. The connector type shouldn't matter. The DSS has VPs and outputs. The VPs are "generic" and identical to each other, except in their possible connections to the outputs. The outputs, at least at the moment, are DPI, LVDS and internal, where internal is basically just DPI. Those are the three different cases we are interested in within the dss driver, right? Does it matter where the DPI or LVDS output goes? So what I'm saying is that the DSS device tree data should already define what kind of configuration we need, and there's no need to look further into the panel/bridge nodes. > Also, implementing this might mean removal of the part of code which > confirms that the panel's "connector_type" indeed expects what the VP > can provide, unless there is a way to find out what the sink is before > calling the drm_of_find_panel_or_bridge API. Hmm, well, each DSS output (port in DT) is of a certain type, so we should be able to validate that the output and the panel's connector_type match. > On the direction, the primary requirement of hw_videoport has been in > the tidss_dispc.c file where the HW registers are getting configured. > 'hw_videoport' is a frequently passed parameter in function calls. So, > at a first glance the former option might makes more sense for > direction, i.e. to have a variable which tells the output to which a > videoport is connected to. Makes sense. > And while, there is also the tidss_kms.c file, which deals with > initializing encoders and attaching bridges. This is where the > output_port is required more often. But I am yet to think of a case > where the above direction could be an issue. > > >> Also, I wonder if output_port is a good name as it has "port" in it >> (like video port), and it's a bit long-ish. Would just "output" be >> enough? We could, of course, shorten it to OP, but that looks odd to me =). >> > >>> fmt->is_oldi_fmt) { >>> dev_dbg(dispc->dev, "%s: %s is not OLDI-port\n", >>> __func__, dispc->feat->vp_name[hw_videoport]); >>> @@ -955,7 +962,7 @@ void dispc_vp_prepare(struct dispc_device *dispc, u32 hw_videoport, >>> if (WARN_ON(!fmt)) >>> return; >>> - if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) { >>> + if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) { >>> dispc_oldi_tx_power(dispc, true); >>> dispc_enable_oldi(dispc, hw_videoport, fmt); >>> @@ -1014,7 +1021,7 @@ void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport, >>> align = true; >>> /* always use DE_HIGH for OLDI */ >>> - if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) >>> + if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) >>> ieo = false; >>> dispc_vp_write(dispc, hw_videoport, DISPC_VP_POL_FREQ, >>> @@ -1040,7 +1047,7 @@ void dispc_vp_disable(struct dispc_device *dispc, u32 hw_videoport) >>> void dispc_vp_unprepare(struct dispc_device *dispc, u32 hw_videoport) >>> { >>> - if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) { >>> + if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) { >>> dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 0); >>> dispc_oldi_tx_power(dispc, false); >>> @@ -1116,10 +1123,10 @@ enum drm_mode_status dispc_vp_mode_valid(struct dispc_device *dispc, >>> const struct drm_display_mode *mode) >>> { >>> u32 hsw, hfp, hbp, vsw, vfp, vbp; >>> - enum dispc_vp_bus_type bus_type; >>> + enum dispc_port_bus_type bus_type; >>> int max_pclk; >>> - bus_type = dispc->feat->vp_bus_type[hw_videoport]; >>> + bus_type = dispc->feat->output_port_bus_type[hw_videoport]; >>> max_pclk = dispc->feat->max_pclk_khz[bus_type]; >>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h >>> index e49432f0abf5..30fb44158347 100644 >>> --- a/drivers/gpu/drm/tidss/tidss_dispc.h >>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h >>> @@ -50,11 +50,11 @@ struct dispc_errata { >>> bool i2000; /* DSS Does Not Support YUV Pixel Data Formats */ >>> }; >>> -enum dispc_vp_bus_type { >>> - DISPC_VP_DPI, /* DPI output */ >>> - DISPC_VP_OLDI, /* OLDI (LVDS) output */ >>> - DISPC_VP_INTERNAL, /* SoC internal routing */ >>> - DISPC_VP_MAX_BUS_TYPE, >>> +enum dispc_port_bus_type { >>> + DISPC_PORT_DPI, /* DPI output */ >>> + DISPC_PORT_OLDI, /* OLDI (LVDS) output */ >>> + DISPC_PORT_INTERNAL, /* SoC internal routing */ >>> + DISPC_PORT_MAX_BUS_TYPE, >> >> Okay, so here you have just "port", not "output_port". In the DT, >> they're ports, so... Maybe we could use that name too, and for video >> port always use "vp". The current "hw_videoport" could be easily >> mistaken with "port". > > I see what you are saying and how somebody could confusre hw_videoport > for a physical connection (i.e. port). I have always understoof > hw_videoport to be a thing of the actual VP inside the SoC, but that may > be because I have been working on this, and not just trying to > understand the code from a high level. > > How about if I change the output_port to "out_port"? I am okay to keep > "output" as the name to. I am saying this becuase I think, only keeping > "port" might just confuse more, as you mentioned above. Yes, I agree "port" is not good. Other than that, no strong opinions. Whatever name you pick, someone will find it confusing ;). Just keep it consistent, so that all enums, parameters, etc. use it a consistent manner. If I had to choose, I'd go with the "output", but I'm fine with other names too. > And then we can change "hw_videoport" to "vp_index", perhaps, or even > keep that as it is? Becuase if we do have a VP structure in future > like you suggested above, "vps" and "vp" would become a close overlap. > For eg, "vps[vp]". That is true. I think that was the reason I chose hw_videoport instead of videoport or vp, although vps[hw_videoport] is only barely better. vp_index is ok for me, or maybe vp_idx or vp_num to have it a bit shorter. Tomi
On 05/02/2023 15:42, Aradhya Bhatia wrote: > Hi Tomi, > > On 03-Feb-23 20:42, Tomi Valkeinen wrote: >> On 25/01/2023 13:35, Aradhya Bhatia wrote: >>> The newer version of DSS (AM625-DSS) has 2 OLDI TXes at its disposal. >>> These can be configured to support the following modes: >>> >>> 1. OLDI_SINGLE_LINK_SINGLE_MODE >>> Single Output over OLDI 0. >>> +------+ +---------+ +-------+ >>> | | | | | | >>> | CRTC +------->+ ENCODER +----->| PANEL | >>> | | | | | | >>> +------+ +---------+ +-------+ >>> >>> 2. OLDI_SINGLE_LINK_CLONE_MODE >>> Duplicate Output over OLDI 0 and 1. >>> +------+ +---------+ +-------+ >>> | | | | | | >>> | CRTC +---+--->| ENCODER +----->| PANEL | >>> | | | | | | | >>> +------+ | +---------+ +-------+ >>> | >>> | +---------+ +-------+ >>> | | | | | >>> +--->| ENCODER +----->| PANEL | >>> | | | | >>> +---------+ +-------+ >>> >>> 3. OLDI_DUAL_LINK_MODE >>> Combined Output over OLDI 0 and 1. >>> +------+ +---------+ +-------+ >>> | | | +----->| | >>> | CRTC +------->+ ENCODER | | PANEL | >>> | | | +----->| | >>> +------+ +---------+ +-------+ >>> >>> Following the above pathways for different modes, 2 encoder/panel-bridge >>> pipes get created for clone mode, and 1 pipe in cases of single link and >>> dual link mode. >>> >>> Add support for confguring the OLDI modes using OF and LVDS DRM helper >>> functions. >>> >>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> >>> --- >>> drivers/gpu/drm/tidss/tidss_dispc.c | 24 ++- >>> drivers/gpu/drm/tidss/tidss_dispc.h | 12 ++ >>> drivers/gpu/drm/tidss/tidss_drv.h | 3 + >>> drivers/gpu/drm/tidss/tidss_encoder.c | 4 +- >>> drivers/gpu/drm/tidss/tidss_encoder.h | 3 +- >>> drivers/gpu/drm/tidss/tidss_kms.c | 221 ++++++++++++++++++++++++-- >>> 6 files changed, 245 insertions(+), 22 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c >>> index b55ccbcaa67f..37a73e309330 100644 >>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c >>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c >>> @@ -88,6 +88,8 @@ const struct dispc_features dispc_k2g_feats = { >>> .subrev = DISPC_K2G, >>> + .has_oldi = false, >>> + >>> .common = "common", >>> .common_regs = tidss_k2g_common_regs, >>> @@ -166,6 +168,8 @@ const struct dispc_features dispc_am625_feats = { >>> .subrev = DISPC_AM625, >>> + .has_oldi = true, >>> + >>> .common = "common", >>> .common_regs = tidss_am65x_common_regs, >>> @@ -218,6 +222,8 @@ const struct dispc_features dispc_am65x_feats = { >>> .subrev = DISPC_AM65X, >>> + .has_oldi = true, >>> + >>> .common = "common", >>> .common_regs = tidss_am65x_common_regs, >>> @@ -309,6 +315,8 @@ const struct dispc_features dispc_j721e_feats = { >>> .subrev = DISPC_J721E, >>> + .has_oldi = false, >>> + >>> .common = "common_m", >>> .common_regs = tidss_j721e_common_regs, >>> @@ -361,6 +369,8 @@ struct dispc_device { >>> struct dss_vp_data vp_data[TIDSS_MAX_VPS]; >>> + enum dispc_oldi_modes oldi_mode; >>> + >>> u32 *fourccs; >>> u32 num_fourccs; >>> @@ -1963,6 +1973,12 @@ const u32 *dispc_plane_formats(struct dispc_device *dispc, unsigned int *len) >>> return dispc->fourccs; >>> } >>> +void dispc_set_oldi_mode(struct dispc_device *dispc, >>> + enum dispc_oldi_modes oldi_mode) >>> +{ >>> + dispc->oldi_mode = oldi_mode; >>> +} >>> + >>> static s32 pixinc(int pixels, u8 ps) >>> { >>> if (pixels == 1) >>> @@ -2647,7 +2663,7 @@ int dispc_runtime_resume(struct dispc_device *dispc) >>> REG_GET(dispc, DSS_SYSSTATUS, 2, 2), >>> REG_GET(dispc, DSS_SYSSTATUS, 3, 3)); >>> - if (dispc->feat->subrev == DISPC_AM65X) >>> + if (dispc->feat->has_oldi) >>> dev_dbg(dispc->dev, "OLDI RESETDONE %d,%d,%d\n", >>> REG_GET(dispc, DSS_SYSSTATUS, 5, 5), >>> REG_GET(dispc, DSS_SYSSTATUS, 6, 6), >>> @@ -2688,7 +2704,7 @@ static int dispc_iomap_resource(struct platform_device *pdev, const char *name, >>> return 0; >>> } >>> -static int dispc_init_am65x_oldi_io_ctrl(struct device *dev, >>> +static int dispc_init_am6xx_oldi_io_ctrl(struct device *dev, >>> struct dispc_device *dispc) >>> { >>> dispc->oldi_io_ctrl = >>> @@ -2827,8 +2843,8 @@ int dispc_init(struct tidss_device *tidss) >>> dispc->vp_data[i].gamma_table = gamma_table; >>> } >>> - if (feat->subrev == DISPC_AM65X) { >>> - r = dispc_init_am65x_oldi_io_ctrl(dev, dispc); >>> + if (feat->has_oldi) { >>> + r = dispc_init_am6xx_oldi_io_ctrl(dev, dispc); >>> if (r) >>> return r; >>> } >>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h >>> index 971f2856f015..880bc7de68b3 100644 >>> --- a/drivers/gpu/drm/tidss/tidss_dispc.h >>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h >>> @@ -64,6 +64,15 @@ enum dispc_dss_subrevision { >>> DISPC_J721E, >>> }; >>> +enum dispc_oldi_modes { >>> + OLDI_MODE_SINGLE_LINK, /* Single output over OLDI 0. */ >>> + OLDI_MODE_CLONE_SINGLE_LINK, /* Cloned output over OLDI 0 and 1. */ >>> + OLDI_MODE_DUAL_LINK, /* Combined output over OLDI 0 and 1. */ >>> + OLDI_MODE_OFF, /* OLDI TXes not connected in OF. */ >>> + OLDI_MODE_UNSUPPORTED, /* Unsupported OLDI configuration in OF. */ >>> + OLDI_MODE_UNAVAILABLE, /* OLDI TXes not available in SoC. */ >>> +}; >>> + >>> struct dispc_features { >>> int min_pclk_khz; >>> int max_pclk_khz[DISPC_PORT_MAX_BUS_TYPE]; >>> @@ -72,6 +81,8 @@ struct dispc_features { >>> enum dispc_dss_subrevision subrev; >>> + bool has_oldi; >>> + >>> const char *common; >>> const u16 *common_regs; >>> u32 num_vps; >>> @@ -131,6 +142,7 @@ int dispc_plane_setup(struct dispc_device *dispc, u32 hw_plane, >>> u32 hw_videoport); >>> int dispc_plane_enable(struct dispc_device *dispc, u32 hw_plane, bool enable); >>> const u32 *dispc_plane_formats(struct dispc_device *dispc, unsigned int *len); >>> +void dispc_set_oldi_mode(struct dispc_device *dispc, enum dispc_oldi_modes oldi_mode); >>> int dispc_init(struct tidss_device *tidss); >>> void dispc_remove(struct tidss_device *tidss); >>> diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h >>> index 0ce7ee5ccd5b..58892f065c16 100644 >>> --- a/drivers/gpu/drm/tidss/tidss_drv.h >>> +++ b/drivers/gpu/drm/tidss/tidss_drv.h >>> @@ -13,6 +13,9 @@ >>> #define TIDSS_MAX_PLANES 4 >>> #define TIDSS_MAX_OUTPUT_PORTS 4 >>> +/* For AM625-DSS with 2 OLDI TXes */ >>> +#define TIDSS_MAX_BRIDGES_PER_PIPE 2 >>> + >>> typedef u32 dispc_irq_t; >>> struct tidss_device { >>> diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c b/drivers/gpu/drm/tidss/tidss_encoder.c >>> index 0d4865e9c03d..bd2a7358d7b0 100644 >>> --- a/drivers/gpu/drm/tidss/tidss_encoder.c >>> +++ b/drivers/gpu/drm/tidss/tidss_encoder.c >>> @@ -70,7 +70,8 @@ static const struct drm_encoder_funcs encoder_funcs = { >>> }; >>> struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss, >>> - u32 encoder_type, u32 possible_crtcs) >>> + u32 encoder_type, u32 possible_crtcs, >>> + u32 possible_clones) >>> { >>> struct drm_encoder *enc; >>> int ret; >>> @@ -80,6 +81,7 @@ struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss, >>> return ERR_PTR(-ENOMEM); >>> enc->possible_crtcs = possible_crtcs; >>> + enc->possible_clones = possible_clones; >>> ret = drm_encoder_init(&tidss->ddev, enc, &encoder_funcs, >>> encoder_type, NULL); >>> diff --git a/drivers/gpu/drm/tidss/tidss_encoder.h b/drivers/gpu/drm/tidss/tidss_encoder.h >>> index ace877c0e0fd..01c62ba3ef16 100644 >>> --- a/drivers/gpu/drm/tidss/tidss_encoder.h >>> +++ b/drivers/gpu/drm/tidss/tidss_encoder.h >>> @@ -12,6 +12,7 @@ >>> struct tidss_device; >>> struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss, >>> - u32 encoder_type, u32 possible_crtcs); >>> + u32 encoder_type, u32 possible_crtcs, >>> + u32 possible_clones); >>> #endif >>> diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c >>> index d449131935d2..8322ee6310bf 100644 >>> --- a/drivers/gpu/drm/tidss/tidss_kms.c >>> +++ b/drivers/gpu/drm/tidss/tidss_kms.c >>> @@ -13,6 +13,7 @@ >>> #include <drm/drm_of.h> >>> #include <drm/drm_panel.h> >>> #include <drm/drm_vblank.h> >>> +#include <linux/of.h> >>> #include "tidss_crtc.h" >>> #include "tidss_dispc.h" >>> @@ -104,26 +105,129 @@ static const struct drm_mode_config_funcs mode_config_funcs = { >>> .atomic_commit = drm_atomic_helper_commit, >>> }; >>> +static enum dispc_oldi_modes tidss_get_oldi_mode(struct tidss_device *tidss) >>> +{ >>> + int pixel_order; >>> + enum dispc_oldi_modes oldi_mode; >>> + struct device_node *oldi0_port, *oldi1_port; >>> + >>> + /* >>> + * For am625-dss, the OLDI ports are expected at port reg = 0 and 2, >>> + * and for am65x-dss, the OLDI port is expected only at port reg = 0. >>> + */ >>> + const u32 portnum_oldi0 = 0, portnum_oldi1 = 2; >>> + >>> + oldi0_port = of_graph_get_port_by_id(tidss->dev->of_node, portnum_oldi0); >>> + oldi1_port = of_graph_get_port_by_id(tidss->dev->of_node, portnum_oldi1); >>> + >>> + if (!(oldi0_port || oldi1_port)) { >>> + /* Keep OLDI TXes OFF if neither OLDI port is present. */ >>> + oldi_mode = OLDI_MODE_OFF; >>> + } else if (oldi0_port && !oldi1_port) { >>> + /* >>> + * OLDI0 port found, but not OLDI1 port. Setting single >>> + * link output mode. >>> + */ >>> + oldi_mode = OLDI_MODE_SINGLE_LINK; >>> + } else if (!oldi0_port && oldi1_port) { >>> + /* >>> + * The 2nd OLDI TX cannot be operated alone. This use case is >>> + * not supported in the HW. Since the pins for OLDIs 0 and 1 are >>> + * separate, one could theoretically set a clone mode over OLDIs >>> + * 0 and 1 and just simply not use the OLDI 0. This is a hacky >>> + * way to enable only OLDI TX 1 and hence is not officially >>> + * supported. >>> + */ >>> + dev_warn(tidss->dev, >>> + "Single Mode over OLDI 1 is not supported in HW.\n"); >>> + oldi_mode = OLDI_MODE_UNSUPPORTED; >>> + } else { >>> + /* >>> + * OLDI Ports found for both the OLDI TXes. The DSS is to be >>> + * configured in either Dual Link or Clone Mode. >>> + */ >>> + pixel_order = drm_of_lvds_get_dual_link_pixel_order(oldi0_port, >>> + oldi1_port); >>> + switch (pixel_order) { >>> + case -EINVAL: >>> + /* >>> + * The dual link properties were not found in at least >>> + * one of the sink nodes. Since 2 OLDI ports are present >>> + * in the DT, it can be safely assumed that the required >>> + * configuration is Clone Mode. >>> + */ >>> + oldi_mode = OLDI_MODE_CLONE_SINGLE_LINK; >>> + break; >>> + >>> + case DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS: >>> + /* >>> + * Note that the OLDI TX 0 transmits the odd set of >>> + * pixels while the OLDI TX 1 transmits the even set. >>> + * This is a fixed configuration in the HW and an cannot >>> + * be change via SW. >>> + */ >>> + dev_warn(tidss->dev, >>> + "EVEN-ODD Dual-Link Mode is not supported in HW.\n"); >>> + oldi_mode = OLDI_MODE_UNSUPPORTED; >>> + break; >>> + >>> + case DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS: >>> + oldi_mode = OLDI_MODE_DUAL_LINK; >>> + break; >>> + >>> + default: >>> + oldi_mode = OLDI_MODE_UNSUPPORTED; >>> + break; >>> + } >>> + } >>> + >>> + of_node_put(oldi0_port); >>> + of_node_put(oldi1_port); >>> + >>> + return oldi_mode; >>> +} >>> + >>> static int tidss_dispc_modeset_init(struct tidss_device *tidss) >>> { >>> struct device *dev = tidss->dev; >>> unsigned int fourccs_len; >>> const u32 *fourccs = dispc_plane_formats(tidss->dispc, &fourccs_len); >>> - unsigned int i; >>> + unsigned int i, j; >>> struct pipe { >>> u32 hw_videoport; >>> - struct drm_bridge *bridge; >>> + struct drm_bridge *bridge[TIDSS_MAX_BRIDGES_PER_PIPE]; >>> u32 enc_type; >>> + u32 num_bridges; >>> }; >>> const struct dispc_features *feat = tidss->feat; >>> u32 output_ports = feat->num_output_ports; >>> u32 max_planes = feat->num_planes; >>> - struct pipe pipes[TIDSS_MAX_VPS]; >>> + struct pipe pipes[TIDSS_MAX_VPS] = {0}; >>> + >>> u32 num_pipes = 0; >>> u32 crtc_mask; >>> + enum dispc_oldi_modes oldi_mode = OLDI_MODE_UNAVAILABLE; >>> + u32 num_oldi = 0; >>> + u32 num_encoders = 0; >>> + u32 oldi_pipe_index = 0; >>> + >>> + if (feat->has_oldi) { >>> + oldi_mode = tidss_get_oldi_mode(tidss); >>> + >>> + if ((oldi_mode == OLDI_MODE_DUAL_LINK || >>> + oldi_mode == OLDI_MODE_CLONE_SINGLE_LINK) && >>> + feat->subrev == DISPC_AM65X) { >>> + dev_warn(tidss->dev, >>> + "am65x-dss does not support this OLDI mode.\n"); >>> + >>> + oldi_mode = OLDI_MODE_UNSUPPORTED; >>> + } >> >> Shouldn't OLDI_MODE_UNSUPPORTED be handled as an error? It means the DT >> is faulty, doesn't it? Maybe it could even be renamed to >> OLDI_MODE_ERROR. Or tidss_get_oldi_mode() could return a negative error >> code. >> > > The idea was to let the framework continue configuring the 2nd videoport > for DPI, even if the OLDI DT is wrong. But I have come across more > examples recently where that is not the case. DT error for one pipe has > resulted in returning of an error code. > > Will make the change. My opinion is that the DT has to be correct. If it isn't, just fail and exit as soon as possible. There shouldn't be any reasons for the drivers to be trying to cope with a broken DT. >>> + >>> + dispc_set_oldi_mode(tidss->dispc, oldi_mode); >>> + } >> >> Would it be better to move the above dispc_set_oldi_mode() to be outside >> the if block? Then oldi mode would be set to OLDI_MODE_UNAVAILABLE on >> SoCs that don't have OLDI. > > Ahh, yes! Will make the change. > >> >> tidss_get_oldi_mode and dispc_set_oldi_mode sound like opposites, but >> they're totally different things. Maybe tidss_get_oldi_mode should >> rather be something about parsing oldi dt properties or such. > > Okay! Is 'tidss_parse_oldi_properties' acceptable? This is just > something I came up with now. I can think of more if this is not good. Sounds fine. Tomi
Hi Tomi, On 06-Feb-23 16:28, Tomi Valkeinen wrote: > On 05/02/2023 16:31, Aradhya Bhatia wrote: >> >> >> On 03-Feb-23 21:03, Tomi Valkeinen wrote: >>> On 25/01/2023 13:35, Aradhya Bhatia wrote: >>>> Add support for the DSS controller on TI's new AM625 SoC in the tidss >>>> driver. >>>> >>>> The first video port (VP0) in am625-dss can output OLDI signals through >>>> 2 OLDI TXes. A 3rd output port has been added with "DISPC_PORT_OLDI" bus >>>> type. >>> >>> Not a big thing here as you add support for a new SoC, but the ordering >>> of the patches is not optimal. Here you add the AM625 DSS support, but >>> then you continue actually adding the DSS support (well, mainly OLDI) in >>> the following patches. >>> >>> I think patch 6 could be before this patch. Parts of patch 4 could also >>> be before this patch. The AM65X renames from patch 5 could be before >>> this patch. >> >> I can move whole of Patch 6 and even of Patch 4 before this one. I have >> mentioned 'AM625-DSS' in a couple comments which I can make generic, >> and the rest everything is SoC-agnostic. >> >> I haven't tried this, but my concern is if we break patch 5 into 2 >> separate patches, >> >> i. AM65X rename plus SoC based switch case, and >> ii. Addition of AM625 SoC case >> >> then I might have to overwrite some changes implemented during (i) in >> (ii). I don't suppose that would be okay, would it? > > I'm not sure I follow here. Wouldn't (i) be a valid patch in its own? > Nothing wrong in expanding that later (even if you end up changing a lot > of it). > (i) would be a valid patch, but implementing (ii) would over-write certain changes done in (i), albeit small changes in terms of brackets and indents. That didn't feel right initially and hence the question. > That said, I don't think this is a very important topic. There are only > a few commits in the history that might be problematic. A simple fix > would be to add all the features first, and only last add the compatible > string for am625. > > Or do all the changes for am625 in a single patch, and try to implement > all the generic restructuring work before that. > > Here we do have to change the vp-to-output mapping management, so maybe > the second option won't be simple enough, and it's better to do the > am625 changes in pieces, as in the first option. > Yeah, the first option does seem a little less complicated. Will try to re-order this as much clearly as possible. > So, it's really up to you. Just wanted to raise this possible issue so > that you are aware of it and can do any easy fixes (if there are such). > >> Also, is it important to keep the compatible-addition patches of >> DT-binding and driver next to each other in the series? Or should >> the DT-binding patches should be the first ones? Just curious! =) > > I believe the convention is to have the DT-binding changes before you > add the compatible string to the driver (if I recall right checkpatch or > some other checking tool complains if you add a driver for a compatible > that doesn't have a DT binding). Generic restructurings could be before > the DT patch, of course, but usually I like to keep the DT binding > changes at the very beginning of the series. > Okay, I will keep the compatible-append in the binding as the first patch in the series, before the other general structurings. Thank you! Regards Aradhya
On 06-Feb-23 18:35, Tomi Valkeinen wrote: > On 05/02/2023 15:08, Aradhya Bhatia wrote: >> Hi Tomi, >> >> Thanks for the review! >> >> On 03-Feb-23 16:53, Tomi Valkeinen wrote: >>> On 25/01/2023 13:35, Aradhya Bhatia wrote: >>>> Make DSS Video Ports agnostic of output bus types. >>>> >>>> DSS controllers have had a 1-to-1 coupling between its VPs and its >>>> output ports. This no longer stands true for the new AM625 DSS. This >>>> coupling, hence, has been removed by renaming the 'vp_bus_type' to >>>> 'output_port_bus_type' because the VPs are essentially agnostic of the >>>> bus type and it is the output ports which have a bus type. >>>> >>>> The AM625 DSS has 2 VPs but requires 3 output ports to support its >>>> Dual-Link OLDI video output coming from a single VP. >>> >>> Not a biggie, but this sentence is a bit odd here at the end. Shouldn't >>> it be after the "...stands true for the new AM625 DSS."? >> >> Yes! It should be. Will make the edit. >> >>> >>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> >>>> --- >>>> drivers/gpu/drm/tidss/tidss_dispc.c | 47 +++++++++++++++++------------ >>>> drivers/gpu/drm/tidss/tidss_dispc.h | 21 +++++++------ >>>> drivers/gpu/drm/tidss/tidss_drv.h | 5 +-- >>>> drivers/gpu/drm/tidss/tidss_irq.h | 2 +- >>>> drivers/gpu/drm/tidss/tidss_kms.c | 12 ++++---- >>>> 5 files changed, 48 insertions(+), 39 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c >>>> index 165365b515e1..c1c4faccbddc 100644 >>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c >>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c >>>> @@ -61,7 +61,7 @@ const struct dispc_features dispc_k2g_feats = { >>>> .min_pclk_khz = 4375, >>>> .max_pclk_khz = { >>>> - [DISPC_VP_DPI] = 150000, >>>> + [DISPC_PORT_DPI] = 150000, >>>> }, >>>> /* >>>> @@ -96,7 +96,6 @@ const struct dispc_features dispc_k2g_feats = { >>>> .vp_name = { "vp1" }, >>>> .ovr_name = { "ovr1" }, >>>> .vpclk_name = { "vp1" }, >>>> - .vp_bus_type = { DISPC_VP_DPI }, >>>> .vp_feat = { .color = { >>>> .has_ctm = true, >>>> @@ -109,6 +108,9 @@ const struct dispc_features dispc_k2g_feats = { >>>> .vid_name = { "vid1" }, >>>> .vid_lite = { false }, >>>> .vid_order = { 0 }, >>>> + >>>> + .num_output_ports = 1, >>>> + .output_port_bus_type = { DISPC_PORT_DPI }, >>>> }; >>> >>> Just thinking out loud, as these will get more complex in the future, >>> maybe we should finally group them with struct. E.g. we could define >>> struct array for vps, like (just hacky example): >>> >>> struct { >>> const char *name; >>> const char *clkname; >>> struct tidss_vp_feat feat; >>> } vps[TIDSS_MAX_PORTS]; >>> >>> and then use them as: >>> >>> .vps = { >>> { >>> .name = "kala", >>> .clkname = "kissa", >>> .feat.color.has_ctm = true, >>> }, { >>> .name = "kala2", >>> .clkname = "kissa2", >>> .feat.color.has_ctm = false, >>> }, >>> }, >>> >>> Perhaps something to try in the future. >>> >> >> Yes, agreed! Having that structure will tidy this up. >> I will keep this under future work. >> >>>> static const u16 tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { >>>> @@ -140,8 +142,8 @@ static const u16 tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { >>>> const struct dispc_features dispc_am65x_feats = { >>>> .max_pclk_khz = { >>>> - [DISPC_VP_DPI] = 165000, >>>> - [DISPC_VP_OLDI] = 165000, >>>> + [DISPC_PORT_DPI] = 165000, >>>> + [DISPC_PORT_OLDI] = 165000, >>>> }, >>>> .scaling = { >>>> @@ -171,7 +173,6 @@ const struct dispc_features dispc_am65x_feats = { >>>> .vp_name = { "vp1", "vp2" }, >>>> .ovr_name = { "ovr1", "ovr2" }, >>>> .vpclk_name = { "vp1", "vp2" }, >>>> - .vp_bus_type = { DISPC_VP_OLDI, DISPC_VP_DPI }, >>>> .vp_feat = { .color = { >>>> .has_ctm = true, >>>> @@ -185,6 +186,9 @@ const struct dispc_features dispc_am65x_feats = { >>>> .vid_name = { "vid", "vidl1" }, >>>> .vid_lite = { false, true, }, >>>> .vid_order = { 1, 0 }, >>>> + >>>> + .num_output_ports = 2, >>>> + .output_port_bus_type = { DISPC_PORT_OLDI, DISPC_PORT_DPI }, >>>> }; >>>> static const u16 tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { >>>> @@ -229,8 +233,8 @@ static const u16 tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { >>>> const struct dispc_features dispc_j721e_feats = { >>>> .max_pclk_khz = { >>>> - [DISPC_VP_DPI] = 170000, >>>> - [DISPC_VP_INTERNAL] = 600000, >>>> + [DISPC_PORT_DPI] = 170000, >>>> + [DISPC_PORT_INTERNAL] = 600000, >>>> }, >>>> .scaling = { >>>> @@ -260,9 +264,7 @@ const struct dispc_features dispc_j721e_feats = { >>>> .vp_name = { "vp1", "vp2", "vp3", "vp4" }, >>>> .ovr_name = { "ovr1", "ovr2", "ovr3", "ovr4" }, >>>> .vpclk_name = { "vp1", "vp2", "vp3", "vp4" }, >>>> - /* Currently hard coded VP routing (see dispc_initial_config()) */ >>>> - .vp_bus_type = { DISPC_VP_INTERNAL, DISPC_VP_DPI, >>>> - DISPC_VP_INTERNAL, DISPC_VP_DPI, }, >>>> + >>> >>> I think this line feed is extra. >> >> Okay! Will remove that from all SoC feat structs. >> >>> >>>> .vp_feat = { .color = { >>>> .has_ctm = true, >>>> .gamma_size = 1024, >>>> @@ -273,6 +275,11 @@ const struct dispc_features dispc_j721e_feats = { >>>> .vid_name = { "vid1", "vidl1", "vid2", "vidl2" }, >>>> .vid_lite = { 0, 1, 0, 1, }, >>>> .vid_order = { 1, 3, 0, 2 }, >>>> + >>>> + .num_output_ports = 4, >>>> + /* Currently hard coded VP routing (see dispc_initial_config()) */ >>>> + .output_port_bus_type = { DISPC_PORT_INTERNAL, DISPC_PORT_DPI, >>>> + DISPC_PORT_INTERNAL, DISPC_PORT_DPI, }, >>> >>> Indent doesn't look right (but it might be just because this is a diff). >> >> I may have missed indenting this. >> >>> >>>> }; >>>> static const u16 *dispc_common_regmap; >>>> @@ -287,12 +294,12 @@ struct dispc_device { >>>> void __iomem *base_common; >>>> void __iomem *base_vid[TIDSS_MAX_PLANES]; >>>> - void __iomem *base_ovr[TIDSS_MAX_PORTS]; >>>> - void __iomem *base_vp[TIDSS_MAX_PORTS]; >>>> + void __iomem *base_ovr[TIDSS_MAX_VPS]; >>>> + void __iomem *base_vp[TIDSS_MAX_VPS]; >>>> struct regmap *oldi_io_ctrl; >>>> - struct clk *vp_clk[TIDSS_MAX_PORTS]; >>>> + struct clk *vp_clk[TIDSS_MAX_VPS]; >>>> const struct dispc_features *feat; >>>> @@ -300,7 +307,7 @@ struct dispc_device { >>>> bool is_enabled; >>>> - struct dss_vp_data vp_data[TIDSS_MAX_PORTS]; >>>> + struct dss_vp_data vp_data[TIDSS_MAX_VPS]; >>>> u32 *fourccs; >>>> u32 num_fourccs; >>>> @@ -851,7 +858,7 @@ int dispc_vp_bus_check(struct dispc_device *dispc, u32 hw_videoport, >>>> return -EINVAL; >>>> } >>>> - if (dispc->feat->vp_bus_type[hw_videoport] != DISPC_VP_OLDI && >>>> + if (dispc->feat->output_port_bus_type[hw_videoport] != DISPC_PORT_OLDI && >>> >>> Hmm, so is the hw_videoport a vp index or an output index? Sounds like >>> the former, so it's not right, even if at the moment they're identical. >>> We need some kind of mapping between those. >>> >> >> It is indeed a vp index. And yes, I can come up with a mapping mechanism. >> >>> If the mapping can be changed (or just defined in the DT), I think we >>> need a variable in struct dispc_device, which tells the output to which >>> a videoport is connected to. Or vice versa, I'm not sure which direction >>> we need more. If the mapping is always the same on all SoC (but I don't >>> think so), we can have it in the feats. >>> >> >> As of now, the mapping is always same. But I would like to make is >> generalized for future. Hence, I am considering to keep the variable in >> struct dispc_device. >> >> My question though would be, how would one be able to find which kind >> of device is the port connected to, if it is connected to a bridge? For >> example, in case of panels, we have a "connector_type" variable in >> drm_panel which tells what kind of sink it is. But there is no such >> thing in drm_bridge. >> >> This is required because what if we can connect an videoport to either >> an LVDS/OLDI bridge or a DPI bridge. > > The connector type shouldn't matter. > > The DSS has VPs and outputs. The VPs are "generic" and identical to each > other, except in their possible connections to the outputs. The outputs, > at least at the moment, are DPI, LVDS and internal, where internal is > basically just DPI. > > Those are the three different cases we are interested in within the dss > driver, right? Does it matter where the DPI or LVDS output goes? > I believe it does. =) While the VPs do always transmit DPI signals, the code in tidss_dispc.c uses the information of the bus connected at the endpoint to configure the OLDI parameters, and to turn OLDI IOs on and off in dispc_vp_(prepare/unprepare). Up until now, the outputs have been fixed (VP0 -> OLDI, VP1 -> DPI), and the code used the enum dispc_vp_bus_type to differntiate between LVDS or DPI requirements. But for a general case where output from VP0 could either use the OLDI TXes and send out LVDS signals OR bypass the OLDI TXes and send out DPI signals directly, we would need a mechanism to find out which sink is present at the end, LVDS or DPI. I assumed, with that mechanism, we could (re)configure the vp-to-output mapping, which then would be used in the various places in tidss_dispc.c. > So what I'm saying is that the DSS device tree data should already > define what kind of configuration we need, and there's no need to look > further into the panel/bridge nodes. > >> Also, implementing this might mean removal of the part of code which >> confirms that the panel's "connector_type" indeed expects what the VP >> can provide, unless there is a way to find out what the sink is before >> calling the drm_of_find_panel_or_bridge API. > > Hmm, well, each DSS output (port in DT) is of a certain type, so we > should be able to validate that the output and the panel's > connector_type match. > >> On the direction, the primary requirement of hw_videoport has been in >> the tidss_dispc.c file where the HW registers are getting configured. >> 'hw_videoport' is a frequently passed parameter in function calls. So, >> at a first glance the former option might makes more sense for >> direction, i.e. to have a variable which tells the output to which a >> videoport is connected to. > > Makes sense. > >> And while, there is also the tidss_kms.c file, which deals with >> initializing encoders and attaching bridges. This is where the >> output_port is required more often. But I am yet to think of a case >> where the above direction could be an issue. >> >> >>> Also, I wonder if output_port is a good name as it has "port" in it >>> (like video port), and it's a bit long-ish. Would just "output" be >>> enough? We could, of course, shorten it to OP, but that looks odd to me =). >>> >> >>>> fmt->is_oldi_fmt) { >>>> dev_dbg(dispc->dev, "%s: %s is not OLDI-port\n", >>>> __func__, dispc->feat->vp_name[hw_videoport]); >>>> @@ -955,7 +962,7 @@ void dispc_vp_prepare(struct dispc_device *dispc, u32 hw_videoport, >>>> if (WARN_ON(!fmt)) >>>> return; >>>> - if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) { >>>> + if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) { >>>> dispc_oldi_tx_power(dispc, true); >>>> dispc_enable_oldi(dispc, hw_videoport, fmt); >>>> @@ -1014,7 +1021,7 @@ void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport, >>>> align = true; >>>> /* always use DE_HIGH for OLDI */ >>>> - if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) >>>> + if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) >>>> ieo = false; >>>> dispc_vp_write(dispc, hw_videoport, DISPC_VP_POL_FREQ, >>>> @@ -1040,7 +1047,7 @@ void dispc_vp_disable(struct dispc_device *dispc, u32 hw_videoport) >>>> void dispc_vp_unprepare(struct dispc_device *dispc, u32 hw_videoport) >>>> { >>>> - if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI) { >>>> + if (dispc->feat->output_port_bus_type[hw_videoport] == DISPC_PORT_OLDI) { >>>> dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 0); >>>> dispc_oldi_tx_power(dispc, false); >>>> @@ -1116,10 +1123,10 @@ enum drm_mode_status dispc_vp_mode_valid(struct dispc_device *dispc, >>>> const struct drm_display_mode *mode) >>>> { >>>> u32 hsw, hfp, hbp, vsw, vfp, vbp; >>>> - enum dispc_vp_bus_type bus_type; >>>> + enum dispc_port_bus_type bus_type; >>>> int max_pclk; >>>> - bus_type = dispc->feat->vp_bus_type[hw_videoport]; >>>> + bus_type = dispc->feat->output_port_bus_type[hw_videoport]; >>>> max_pclk = dispc->feat->max_pclk_khz[bus_type]; >>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h >>>> index e49432f0abf5..30fb44158347 100644 >>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.h >>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h >>>> @@ -50,11 +50,11 @@ struct dispc_errata { >>>> bool i2000; /* DSS Does Not Support YUV Pixel Data Formats */ >>>> }; >>>> -enum dispc_vp_bus_type { >>>> - DISPC_VP_DPI, /* DPI output */ >>>> - DISPC_VP_OLDI, /* OLDI (LVDS) output */ >>>> - DISPC_VP_INTERNAL, /* SoC internal routing */ >>>> - DISPC_VP_MAX_BUS_TYPE, >>>> +enum dispc_port_bus_type { >>>> + DISPC_PORT_DPI, /* DPI output */ >>>> + DISPC_PORT_OLDI, /* OLDI (LVDS) output */ >>>> + DISPC_PORT_INTERNAL, /* SoC internal routing */ >>>> + DISPC_PORT_MAX_BUS_TYPE, >>> >>> Okay, so here you have just "port", not "output_port". In the DT, >>> they're ports, so... Maybe we could use that name too, and for video >>> port always use "vp". The current "hw_videoport" could be easily >>> mistaken with "port". >> >> I see what you are saying and how somebody could confusre hw_videoport >> for a physical connection (i.e. port). I have always understoof >> hw_videoport to be a thing of the actual VP inside the SoC, but that may >> be because I have been working on this, and not just trying to >> understand the code from a high level. >> >> How about if I change the output_port to "out_port"? I am okay to keep >> "output" as the name to. I am saying this becuase I think, only keeping >> "port" might just confuse more, as you mentioned above. > > Yes, I agree "port" is not good. Other than that, no strong opinions. > Whatever name you pick, someone will find it confusing ;). Just keep it > consistent, so that all enums, parameters, etc. use it a consistent > manner. If I had to choose, I'd go with the "output", but I'm fine with > other names too. > Alright! I am good with using "output". >> And then we can change "hw_videoport" to "vp_index", perhaps, or even >> keep that as it is? Becuase if we do have a VP structure in future >> like you suggested above, "vps" and "vp" would become a close overlap. >> For eg, "vps[vp]". > > That is true. I think that was the reason I chose hw_videoport instead > of videoport or vp, although vps[hw_videoport] is only barely better. > > vp_index is ok for me, or maybe vp_idx or vp_num to have it a bit shorter. > "vp_idx" seems better! Regards Aradhya
On 06-Feb-23 19:12, Tomi Valkeinen wrote: > On 05/02/2023 15:42, Aradhya Bhatia wrote: >> Hi Tomi, >> >> On 03-Feb-23 20:42, Tomi Valkeinen wrote: >>> On 25/01/2023 13:35, Aradhya Bhatia wrote: >>>> The newer version of DSS (AM625-DSS) has 2 OLDI TXes at its disposal. >>>> These can be configured to support the following modes: >>>> >>>> 1. OLDI_SINGLE_LINK_SINGLE_MODE >>>> Single Output over OLDI 0. >>>> +------+ +---------+ +-------+ >>>> | | | | | | >>>> | CRTC +------->+ ENCODER +----->| PANEL | >>>> | | | | | | >>>> +------+ +---------+ +-------+ >>>> >>>> 2. OLDI_SINGLE_LINK_CLONE_MODE >>>> Duplicate Output over OLDI 0 and 1. >>>> +------+ +---------+ +-------+ >>>> | | | | | | >>>> | CRTC +---+--->| ENCODER +----->| PANEL | >>>> | | | | | | | >>>> +------+ | +---------+ +-------+ >>>> | >>>> | +---------+ +-------+ >>>> | | | | | >>>> +--->| ENCODER +----->| PANEL | >>>> | | | | >>>> +---------+ +-------+ >>>> >>>> 3. OLDI_DUAL_LINK_MODE >>>> Combined Output over OLDI 0 and 1. >>>> +------+ +---------+ +-------+ >>>> | | | +----->| | >>>> | CRTC +------->+ ENCODER | | PANEL | >>>> | | | +----->| | >>>> +------+ +---------+ +-------+ >>>> >>>> Following the above pathways for different modes, 2 encoder/panel-bridge >>>> pipes get created for clone mode, and 1 pipe in cases of single link and >>>> dual link mode. >>>> >>>> Add support for confguring the OLDI modes using OF and LVDS DRM helper >>>> functions. >>>> >>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> >>>> --- >>>> drivers/gpu/drm/tidss/tidss_dispc.c | 24 ++- >>>> drivers/gpu/drm/tidss/tidss_dispc.h | 12 ++ >>>> drivers/gpu/drm/tidss/tidss_drv.h | 3 + >>>> drivers/gpu/drm/tidss/tidss_encoder.c | 4 +- >>>> drivers/gpu/drm/tidss/tidss_encoder.h | 3 +- >>>> drivers/gpu/drm/tidss/tidss_kms.c | 221 ++++++++++++++++++++++++-- >>>> 6 files changed, 245 insertions(+), 22 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c >>>> index b55ccbcaa67f..37a73e309330 100644 >>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c >>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c >>>> @@ -88,6 +88,8 @@ const struct dispc_features dispc_k2g_feats = { >>>> .subrev = DISPC_K2G, >>>> + .has_oldi = false, >>>> + >>>> .common = "common", >>>> .common_regs = tidss_k2g_common_regs, >>>> @@ -166,6 +168,8 @@ const struct dispc_features dispc_am625_feats = { >>>> .subrev = DISPC_AM625, >>>> + .has_oldi = true, >>>> + >>>> .common = "common", >>>> .common_regs = tidss_am65x_common_regs, >>>> @@ -218,6 +222,8 @@ const struct dispc_features dispc_am65x_feats = { >>>> .subrev = DISPC_AM65X, >>>> + .has_oldi = true, >>>> + >>>> .common = "common", >>>> .common_regs = tidss_am65x_common_regs, >>>> @@ -309,6 +315,8 @@ const struct dispc_features dispc_j721e_feats = { >>>> .subrev = DISPC_J721E, >>>> + .has_oldi = false, >>>> + >>>> .common = "common_m", >>>> .common_regs = tidss_j721e_common_regs, >>>> @@ -361,6 +369,8 @@ struct dispc_device { >>>> struct dss_vp_data vp_data[TIDSS_MAX_VPS]; >>>> + enum dispc_oldi_modes oldi_mode; >>>> + >>>> u32 *fourccs; >>>> u32 num_fourccs; >>>> @@ -1963,6 +1973,12 @@ const u32 *dispc_plane_formats(struct dispc_device *dispc, unsigned int *len) >>>> return dispc->fourccs; >>>> } >>>> +void dispc_set_oldi_mode(struct dispc_device *dispc, >>>> + enum dispc_oldi_modes oldi_mode) >>>> +{ >>>> + dispc->oldi_mode = oldi_mode; >>>> +} >>>> + >>>> static s32 pixinc(int pixels, u8 ps) >>>> { >>>> if (pixels == 1) >>>> @@ -2647,7 +2663,7 @@ int dispc_runtime_resume(struct dispc_device *dispc) >>>> REG_GET(dispc, DSS_SYSSTATUS, 2, 2), >>>> REG_GET(dispc, DSS_SYSSTATUS, 3, 3)); >>>> - if (dispc->feat->subrev == DISPC_AM65X) >>>> + if (dispc->feat->has_oldi) >>>> dev_dbg(dispc->dev, "OLDI RESETDONE %d,%d,%d\n", >>>> REG_GET(dispc, DSS_SYSSTATUS, 5, 5), >>>> REG_GET(dispc, DSS_SYSSTATUS, 6, 6), >>>> @@ -2688,7 +2704,7 @@ static int dispc_iomap_resource(struct platform_device *pdev, const char *name, >>>> return 0; >>>> } >>>> -static int dispc_init_am65x_oldi_io_ctrl(struct device *dev, >>>> +static int dispc_init_am6xx_oldi_io_ctrl(struct device *dev, >>>> struct dispc_device *dispc) >>>> { >>>> dispc->oldi_io_ctrl = >>>> @@ -2827,8 +2843,8 @@ int dispc_init(struct tidss_device *tidss) >>>> dispc->vp_data[i].gamma_table = gamma_table; >>>> } >>>> - if (feat->subrev == DISPC_AM65X) { >>>> - r = dispc_init_am65x_oldi_io_ctrl(dev, dispc); >>>> + if (feat->has_oldi) { >>>> + r = dispc_init_am6xx_oldi_io_ctrl(dev, dispc); >>>> if (r) >>>> return r; >>>> } >>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h >>>> index 971f2856f015..880bc7de68b3 100644 >>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.h >>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h >>>> @@ -64,6 +64,15 @@ enum dispc_dss_subrevision { >>>> DISPC_J721E, >>>> }; >>>> +enum dispc_oldi_modes { >>>> + OLDI_MODE_SINGLE_LINK, /* Single output over OLDI 0. */ >>>> + OLDI_MODE_CLONE_SINGLE_LINK, /* Cloned output over OLDI 0 and 1. */ >>>> + OLDI_MODE_DUAL_LINK, /* Combined output over OLDI 0 and 1. */ >>>> + OLDI_MODE_OFF, /* OLDI TXes not connected in OF. */ >>>> + OLDI_MODE_UNSUPPORTED, /* Unsupported OLDI configuration in OF. */ >>>> + OLDI_MODE_UNAVAILABLE, /* OLDI TXes not available in SoC. */ >>>> +}; >>>> + >>>> struct dispc_features { >>>> int min_pclk_khz; >>>> int max_pclk_khz[DISPC_PORT_MAX_BUS_TYPE]; >>>> @@ -72,6 +81,8 @@ struct dispc_features { >>>> enum dispc_dss_subrevision subrev; >>>> + bool has_oldi; >>>> + >>>> const char *common; >>>> const u16 *common_regs; >>>> u32 num_vps; >>>> @@ -131,6 +142,7 @@ int dispc_plane_setup(struct dispc_device *dispc, u32 hw_plane, >>>> u32 hw_videoport); >>>> int dispc_plane_enable(struct dispc_device *dispc, u32 hw_plane, bool enable); >>>> const u32 *dispc_plane_formats(struct dispc_device *dispc, unsigned int *len); >>>> +void dispc_set_oldi_mode(struct dispc_device *dispc, enum dispc_oldi_modes oldi_mode); >>>> int dispc_init(struct tidss_device *tidss); >>>> void dispc_remove(struct tidss_device *tidss); >>>> diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h >>>> index 0ce7ee5ccd5b..58892f065c16 100644 >>>> --- a/drivers/gpu/drm/tidss/tidss_drv.h >>>> +++ b/drivers/gpu/drm/tidss/tidss_drv.h >>>> @@ -13,6 +13,9 @@ >>>> #define TIDSS_MAX_PLANES 4 >>>> #define TIDSS_MAX_OUTPUT_PORTS 4 >>>> +/* For AM625-DSS with 2 OLDI TXes */ >>>> +#define TIDSS_MAX_BRIDGES_PER_PIPE 2 >>>> + >>>> typedef u32 dispc_irq_t; >>>> struct tidss_device { >>>> diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c b/drivers/gpu/drm/tidss/tidss_encoder.c >>>> index 0d4865e9c03d..bd2a7358d7b0 100644 >>>> --- a/drivers/gpu/drm/tidss/tidss_encoder.c >>>> +++ b/drivers/gpu/drm/tidss/tidss_encoder.c >>>> @@ -70,7 +70,8 @@ static const struct drm_encoder_funcs encoder_funcs = { >>>> }; >>>> struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss, >>>> - u32 encoder_type, u32 possible_crtcs) >>>> + u32 encoder_type, u32 possible_crtcs, >>>> + u32 possible_clones) >>>> { >>>> struct drm_encoder *enc; >>>> int ret; >>>> @@ -80,6 +81,7 @@ struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss, >>>> return ERR_PTR(-ENOMEM); >>>> enc->possible_crtcs = possible_crtcs; >>>> + enc->possible_clones = possible_clones; >>>> ret = drm_encoder_init(&tidss->ddev, enc, &encoder_funcs, >>>> encoder_type, NULL); >>>> diff --git a/drivers/gpu/drm/tidss/tidss_encoder.h b/drivers/gpu/drm/tidss/tidss_encoder.h >>>> index ace877c0e0fd..01c62ba3ef16 100644 >>>> --- a/drivers/gpu/drm/tidss/tidss_encoder.h >>>> +++ b/drivers/gpu/drm/tidss/tidss_encoder.h >>>> @@ -12,6 +12,7 @@ >>>> struct tidss_device; >>>> struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss, >>>> - u32 encoder_type, u32 possible_crtcs); >>>> + u32 encoder_type, u32 possible_crtcs, >>>> + u32 possible_clones); >>>> #endif >>>> diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c >>>> index d449131935d2..8322ee6310bf 100644 >>>> --- a/drivers/gpu/drm/tidss/tidss_kms.c >>>> +++ b/drivers/gpu/drm/tidss/tidss_kms.c >>>> @@ -13,6 +13,7 @@ >>>> #include <drm/drm_of.h> >>>> #include <drm/drm_panel.h> >>>> #include <drm/drm_vblank.h> >>>> +#include <linux/of.h> >>>> #include "tidss_crtc.h" >>>> #include "tidss_dispc.h" >>>> @@ -104,26 +105,129 @@ static const struct drm_mode_config_funcs mode_config_funcs = { >>>> .atomic_commit = drm_atomic_helper_commit, >>>> }; >>>> +static enum dispc_oldi_modes tidss_get_oldi_mode(struct tidss_device *tidss) >>>> +{ >>>> + int pixel_order; >>>> + enum dispc_oldi_modes oldi_mode; >>>> + struct device_node *oldi0_port, *oldi1_port; >>>> + >>>> + /* >>>> + * For am625-dss, the OLDI ports are expected at port reg = 0 and 2, >>>> + * and for am65x-dss, the OLDI port is expected only at port reg = 0. >>>> + */ >>>> + const u32 portnum_oldi0 = 0, portnum_oldi1 = 2; >>>> + >>>> + oldi0_port = of_graph_get_port_by_id(tidss->dev->of_node, portnum_oldi0); >>>> + oldi1_port = of_graph_get_port_by_id(tidss->dev->of_node, portnum_oldi1); >>>> + >>>> + if (!(oldi0_port || oldi1_port)) { >>>> + /* Keep OLDI TXes OFF if neither OLDI port is present. */ >>>> + oldi_mode = OLDI_MODE_OFF; >>>> + } else if (oldi0_port && !oldi1_port) { >>>> + /* >>>> + * OLDI0 port found, but not OLDI1 port. Setting single >>>> + * link output mode. >>>> + */ >>>> + oldi_mode = OLDI_MODE_SINGLE_LINK; >>>> + } else if (!oldi0_port && oldi1_port) { >>>> + /* >>>> + * The 2nd OLDI TX cannot be operated alone. This use case is >>>> + * not supported in the HW. Since the pins for OLDIs 0 and 1 are >>>> + * separate, one could theoretically set a clone mode over OLDIs >>>> + * 0 and 1 and just simply not use the OLDI 0. This is a hacky >>>> + * way to enable only OLDI TX 1 and hence is not officially >>>> + * supported. >>>> + */ >>>> + dev_warn(tidss->dev, >>>> + "Single Mode over OLDI 1 is not supported in HW.\n"); >>>> + oldi_mode = OLDI_MODE_UNSUPPORTED; >>>> + } else { >>>> + /* >>>> + * OLDI Ports found for both the OLDI TXes. The DSS is to be >>>> + * configured in either Dual Link or Clone Mode. >>>> + */ >>>> + pixel_order = drm_of_lvds_get_dual_link_pixel_order(oldi0_port, >>>> + oldi1_port); >>>> + switch (pixel_order) { >>>> + case -EINVAL: >>>> + /* >>>> + * The dual link properties were not found in at least >>>> + * one of the sink nodes. Since 2 OLDI ports are present >>>> + * in the DT, it can be safely assumed that the required >>>> + * configuration is Clone Mode. >>>> + */ >>>> + oldi_mode = OLDI_MODE_CLONE_SINGLE_LINK; >>>> + break; >>>> + >>>> + case DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS: >>>> + /* >>>> + * Note that the OLDI TX 0 transmits the odd set of >>>> + * pixels while the OLDI TX 1 transmits the even set. >>>> + * This is a fixed configuration in the HW and an cannot >>>> + * be change via SW. >>>> + */ >>>> + dev_warn(tidss->dev, >>>> + "EVEN-ODD Dual-Link Mode is not supported in HW.\n"); >>>> + oldi_mode = OLDI_MODE_UNSUPPORTED; >>>> + break; >>>> + >>>> + case DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS: >>>> + oldi_mode = OLDI_MODE_DUAL_LINK; >>>> + break; >>>> + >>>> + default: >>>> + oldi_mode = OLDI_MODE_UNSUPPORTED; >>>> + break; >>>> + } >>>> + } >>>> + >>>> + of_node_put(oldi0_port); >>>> + of_node_put(oldi1_port); >>>> + >>>> + return oldi_mode; >>>> +} >>>> + >>>> static int tidss_dispc_modeset_init(struct tidss_device *tidss) >>>> { >>>> struct device *dev = tidss->dev; >>>> unsigned int fourccs_len; >>>> const u32 *fourccs = dispc_plane_formats(tidss->dispc, &fourccs_len); >>>> - unsigned int i; >>>> + unsigned int i, j; >>>> struct pipe { >>>> u32 hw_videoport; >>>> - struct drm_bridge *bridge; >>>> + struct drm_bridge *bridge[TIDSS_MAX_BRIDGES_PER_PIPE]; >>>> u32 enc_type; >>>> + u32 num_bridges; >>>> }; >>>> const struct dispc_features *feat = tidss->feat; >>>> u32 output_ports = feat->num_output_ports; >>>> u32 max_planes = feat->num_planes; >>>> - struct pipe pipes[TIDSS_MAX_VPS]; >>>> + struct pipe pipes[TIDSS_MAX_VPS] = {0}; >>>> + >>>> u32 num_pipes = 0; >>>> u32 crtc_mask; >>>> + enum dispc_oldi_modes oldi_mode = OLDI_MODE_UNAVAILABLE; >>>> + u32 num_oldi = 0; >>>> + u32 num_encoders = 0; >>>> + u32 oldi_pipe_index = 0; >>>> + >>>> + if (feat->has_oldi) { >>>> + oldi_mode = tidss_get_oldi_mode(tidss); >>>> + >>>> + if ((oldi_mode == OLDI_MODE_DUAL_LINK || >>>> + oldi_mode == OLDI_MODE_CLONE_SINGLE_LINK) && >>>> + feat->subrev == DISPC_AM65X) { >>>> + dev_warn(tidss->dev, >>>> + "am65x-dss does not support this OLDI mode.\n"); >>>> + >>>> + oldi_mode = OLDI_MODE_UNSUPPORTED; >>>> + } >>> >>> Shouldn't OLDI_MODE_UNSUPPORTED be handled as an error? It means the DT >>> is faulty, doesn't it? Maybe it could even be renamed to >>> OLDI_MODE_ERROR. Or tidss_get_oldi_mode() could return a negative error >>> code. >>> >> >> The idea was to let the framework continue configuring the 2nd videoport >> for DPI, even if the OLDI DT is wrong. But I have come across more >> examples recently where that is not the case. DT error for one pipe has >> resulted in returning of an error code. >> >> Will make the change. > > My opinion is that the DT has to be correct. If it isn't, just fail and > exit as soon as possible. There shouldn't be any reasons for the drivers > to be trying to cope with a broken DT. Understood! Will error out in those cases! > >>>> + >>>> + dispc_set_oldi_mode(tidss->dispc, oldi_mode); >>>> + } >>> >>> Would it be better to move the above dispc_set_oldi_mode() to be outside >>> the if block? Then oldi mode would be set to OLDI_MODE_UNAVAILABLE on >>> SoCs that don't have OLDI. >> >> Ahh, yes! Will make the change. >> >>> >>> tidss_get_oldi_mode and dispc_set_oldi_mode sound like opposites, but >>> they're totally different things. Maybe tidss_get_oldi_mode should >>> rather be something about parsing oldi dt properties or such. >> >> Okay! Is 'tidss_parse_oldi_properties' acceptable? This is just >> something I came up with now. I can think of more if this is not good. > > Sounds fine. > Okay! Regards Aradhya
On 06/02/2023 19:34, Aradhya Bhatia wrote: > > On 06-Feb-23 18:35, Tomi Valkeinen wrote: >> On 05/02/2023 15:08, Aradhya Bhatia wrote: >>> Hi Tomi, >>> >>> Thanks for the review! >>> >>> On 03-Feb-23 16:53, Tomi Valkeinen wrote: >>>> On 25/01/2023 13:35, Aradhya Bhatia wrote: >>>>> Make DSS Video Ports agnostic of output bus types. >>>>> >>>>> DSS controllers have had a 1-to-1 coupling between its VPs and its >>>>> output ports. This no longer stands true for the new AM625 DSS. This >>>>> coupling, hence, has been removed by renaming the 'vp_bus_type' to >>>>> 'output_port_bus_type' because the VPs are essentially agnostic of the >>>>> bus type and it is the output ports which have a bus type. >>>>> >>>>> The AM625 DSS has 2 VPs but requires 3 output ports to support its >>>>> Dual-Link OLDI video output coming from a single VP. >>>> >>>> Not a biggie, but this sentence is a bit odd here at the end. Shouldn't >>>> it be after the "...stands true for the new AM625 DSS."? >>> >>> Yes! It should be. Will make the edit. >>> >>>> >>>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> >>>>> --- >>>>> drivers/gpu/drm/tidss/tidss_dispc.c | 47 +++++++++++++++++------------ >>>>> drivers/gpu/drm/tidss/tidss_dispc.h | 21 +++++++------ >>>>> drivers/gpu/drm/tidss/tidss_drv.h | 5 +-- >>>>> drivers/gpu/drm/tidss/tidss_irq.h | 2 +- >>>>> drivers/gpu/drm/tidss/tidss_kms.c | 12 ++++---- >>>>> 5 files changed, 48 insertions(+), 39 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c >>>>> index 165365b515e1..c1c4faccbddc 100644 >>>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c >>>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c >>>>> @@ -61,7 +61,7 @@ const struct dispc_features dispc_k2g_feats = { >>>>> .min_pclk_khz = 4375, >>>>> .max_pclk_khz = { >>>>> - [DISPC_VP_DPI] = 150000, >>>>> + [DISPC_PORT_DPI] = 150000, >>>>> }, >>>>> /* >>>>> @@ -96,7 +96,6 @@ const struct dispc_features dispc_k2g_feats = { >>>>> .vp_name = { "vp1" }, >>>>> .ovr_name = { "ovr1" }, >>>>> .vpclk_name = { "vp1" }, >>>>> - .vp_bus_type = { DISPC_VP_DPI }, >>>>> .vp_feat = { .color = { >>>>> .has_ctm = true, >>>>> @@ -109,6 +108,9 @@ const struct dispc_features dispc_k2g_feats = { >>>>> .vid_name = { "vid1" }, >>>>> .vid_lite = { false }, >>>>> .vid_order = { 0 }, >>>>> + >>>>> + .num_output_ports = 1, >>>>> + .output_port_bus_type = { DISPC_PORT_DPI }, >>>>> }; >>>> >>>> Just thinking out loud, as these will get more complex in the future, >>>> maybe we should finally group them with struct. E.g. we could define >>>> struct array for vps, like (just hacky example): >>>> >>>> struct { >>>> const char *name; >>>> const char *clkname; >>>> struct tidss_vp_feat feat; >>>> } vps[TIDSS_MAX_PORTS]; >>>> >>>> and then use them as: >>>> >>>> .vps = { >>>> { >>>> .name = "kala", >>>> .clkname = "kissa", >>>> .feat.color.has_ctm = true, >>>> }, { >>>> .name = "kala2", >>>> .clkname = "kissa2", >>>> .feat.color.has_ctm = false, >>>> }, >>>> }, >>>> >>>> Perhaps something to try in the future. >>>> >>> >>> Yes, agreed! Having that structure will tidy this up. >>> I will keep this under future work. >>> >>>>> static const u16 tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { >>>>> @@ -140,8 +142,8 @@ static const u16 tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { >>>>> const struct dispc_features dispc_am65x_feats = { >>>>> .max_pclk_khz = { >>>>> - [DISPC_VP_DPI] = 165000, >>>>> - [DISPC_VP_OLDI] = 165000, >>>>> + [DISPC_PORT_DPI] = 165000, >>>>> + [DISPC_PORT_OLDI] = 165000, >>>>> }, >>>>> .scaling = { >>>>> @@ -171,7 +173,6 @@ const struct dispc_features dispc_am65x_feats = { >>>>> .vp_name = { "vp1", "vp2" }, >>>>> .ovr_name = { "ovr1", "ovr2" }, >>>>> .vpclk_name = { "vp1", "vp2" }, >>>>> - .vp_bus_type = { DISPC_VP_OLDI, DISPC_VP_DPI }, >>>>> .vp_feat = { .color = { >>>>> .has_ctm = true, >>>>> @@ -185,6 +186,9 @@ const struct dispc_features dispc_am65x_feats = { >>>>> .vid_name = { "vid", "vidl1" }, >>>>> .vid_lite = { false, true, }, >>>>> .vid_order = { 1, 0 }, >>>>> + >>>>> + .num_output_ports = 2, >>>>> + .output_port_bus_type = { DISPC_PORT_OLDI, DISPC_PORT_DPI }, >>>>> }; >>>>> static const u16 tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { >>>>> @@ -229,8 +233,8 @@ static const u16 tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { >>>>> const struct dispc_features dispc_j721e_feats = { >>>>> .max_pclk_khz = { >>>>> - [DISPC_VP_DPI] = 170000, >>>>> - [DISPC_VP_INTERNAL] = 600000, >>>>> + [DISPC_PORT_DPI] = 170000, >>>>> + [DISPC_PORT_INTERNAL] = 600000, >>>>> }, >>>>> .scaling = { >>>>> @@ -260,9 +264,7 @@ const struct dispc_features dispc_j721e_feats = { >>>>> .vp_name = { "vp1", "vp2", "vp3", "vp4" }, >>>>> .ovr_name = { "ovr1", "ovr2", "ovr3", "ovr4" }, >>>>> .vpclk_name = { "vp1", "vp2", "vp3", "vp4" }, >>>>> - /* Currently hard coded VP routing (see dispc_initial_config()) */ >>>>> - .vp_bus_type = { DISPC_VP_INTERNAL, DISPC_VP_DPI, >>>>> - DISPC_VP_INTERNAL, DISPC_VP_DPI, }, >>>>> + >>>> >>>> I think this line feed is extra. >>> >>> Okay! Will remove that from all SoC feat structs. >>> >>>> >>>>> .vp_feat = { .color = { >>>>> .has_ctm = true, >>>>> .gamma_size = 1024, >>>>> @@ -273,6 +275,11 @@ const struct dispc_features dispc_j721e_feats = { >>>>> .vid_name = { "vid1", "vidl1", "vid2", "vidl2" }, >>>>> .vid_lite = { 0, 1, 0, 1, }, >>>>> .vid_order = { 1, 3, 0, 2 }, >>>>> + >>>>> + .num_output_ports = 4, >>>>> + /* Currently hard coded VP routing (see dispc_initial_config()) */ >>>>> + .output_port_bus_type = { DISPC_PORT_INTERNAL, DISPC_PORT_DPI, >>>>> + DISPC_PORT_INTERNAL, DISPC_PORT_DPI, }, >>>> >>>> Indent doesn't look right (but it might be just because this is a diff). >>> >>> I may have missed indenting this. >>> >>>> >>>>> }; >>>>> static const u16 *dispc_common_regmap; >>>>> @@ -287,12 +294,12 @@ struct dispc_device { >>>>> void __iomem *base_common; >>>>> void __iomem *base_vid[TIDSS_MAX_PLANES]; >>>>> - void __iomem *base_ovr[TIDSS_MAX_PORTS]; >>>>> - void __iomem *base_vp[TIDSS_MAX_PORTS]; >>>>> + void __iomem *base_ovr[TIDSS_MAX_VPS]; >>>>> + void __iomem *base_vp[TIDSS_MAX_VPS]; >>>>> struct regmap *oldi_io_ctrl; >>>>> - struct clk *vp_clk[TIDSS_MAX_PORTS]; >>>>> + struct clk *vp_clk[TIDSS_MAX_VPS]; >>>>> const struct dispc_features *feat; >>>>> @@ -300,7 +307,7 @@ struct dispc_device { >>>>> bool is_enabled; >>>>> - struct dss_vp_data vp_data[TIDSS_MAX_PORTS]; >>>>> + struct dss_vp_data vp_data[TIDSS_MAX_VPS]; >>>>> u32 *fourccs; >>>>> u32 num_fourccs; >>>>> @@ -851,7 +858,7 @@ int dispc_vp_bus_check(struct dispc_device *dispc, u32 hw_videoport, >>>>> return -EINVAL; >>>>> } >>>>> - if (dispc->feat->vp_bus_type[hw_videoport] != DISPC_VP_OLDI && >>>>> + if (dispc->feat->output_port_bus_type[hw_videoport] != DISPC_PORT_OLDI && >>>> >>>> Hmm, so is the hw_videoport a vp index or an output index? Sounds like >>>> the former, so it's not right, even if at the moment they're identical. >>>> We need some kind of mapping between those. >>>> >>> >>> It is indeed a vp index. And yes, I can come up with a mapping mechanism. >>> >>>> If the mapping can be changed (or just defined in the DT), I think we >>>> need a variable in struct dispc_device, which tells the output to which >>>> a videoport is connected to. Or vice versa, I'm not sure which direction >>>> we need more. If the mapping is always the same on all SoC (but I don't >>>> think so), we can have it in the feats. >>>> >>> >>> As of now, the mapping is always same. But I would like to make is >>> generalized for future. Hence, I am considering to keep the variable in >>> struct dispc_device. >>> >>> My question though would be, how would one be able to find which kind >>> of device is the port connected to, if it is connected to a bridge? For >>> example, in case of panels, we have a "connector_type" variable in >>> drm_panel which tells what kind of sink it is. But there is no such >>> thing in drm_bridge. >>> >>> This is required because what if we can connect an videoport to either >>> an LVDS/OLDI bridge or a DPI bridge. >> >> The connector type shouldn't matter. >> >> The DSS has VPs and outputs. The VPs are "generic" and identical to each >> other, except in their possible connections to the outputs. The outputs, >> at least at the moment, are DPI, LVDS and internal, where internal is >> basically just DPI. >> >> Those are the three different cases we are interested in within the dss >> driver, right? Does it matter where the DPI or LVDS output goes? >> > > I believe it does. =) > > While the VPs do always transmit DPI signals, the code in tidss_dispc.c > uses the information of the bus connected at the endpoint to configure > the OLDI parameters, and to turn OLDI IOs on and off in > dispc_vp_(prepare/unprepare). > > Up until now, the outputs have been fixed (VP0 -> OLDI, VP1 -> DPI), and > the code used the enum dispc_vp_bus_type to differntiate between LVDS or > DPI requirements. But for a general case where output from VP0 could > either use the OLDI TXes and send out LVDS signals OR bypass the OLDI > TXes and send out DPI signals directly, we would need a mechanism to > find out which sink is present at the end, LVDS or DPI. > > I assumed, with that mechanism, we could (re)configure the vp-to-output > mapping, which then would be used in the various places in > tidss_dispc.c. But we should already know all that. Say, on AM625, we have three ports (or "outputs") (defined in DT), OLDI1, DPI, OLDI2. If there's an endpoint configured for the first port, we know we need to set up OLDI, and we need a VP for it. If the hardware mapping between VPs and outputs is hardcoded, we know directly that VP0 is needed, and it's used for OLDI. So now we have the mapping of VP0 -> OLDI (port1). If (say) VP0 could alternatively be used for DPI output, then we'd see the second port, DPI, having an endpoint configured. Having both OLDI1 and DPI endpoints would be invalid, of course. So "how would one be able to find which kind of device is the port connected to" is irrelevant, I think. We know the output port, so we know the bus type, and we don't really need to know anything else about what's there behind the bus. Or is there some detail I'm missing here? Tomi