| Message ID | 20220928175223.15225-1-a-bhatia1@ti.com |
|---|---|
| Headers | show |
| Series | Add DSS support for AM625 SoC | expand |
On 28/09/2022 20:52, 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. > > Add IO CTRL support and control the OLDI TX power for AM625. > > Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> > --- > drivers/gpu/drm/tidss/tidss_dispc.c | 55 ++++++++++++++++++------ > drivers/gpu/drm/tidss/tidss_dispc_regs.h | 6 +++ > 2 files changed, 49 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c > index 88008ad39b55..68444e0cd8d7 100644 > --- a/drivers/gpu/drm/tidss/tidss_dispc.c > +++ b/drivers/gpu/drm/tidss/tidss_dispc.c > @@ -921,21 +921,52 @@ 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) { > + val = power ? 0 : OLDI_PWRDN_TX; > + > + 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); > + > + } else if (dispc->feat->subrev == DISPC_AM625) { > + if (power) { > + switch (dispc->oldi_mode) { > + case OLDI_SINGLE_LINK_SINGLE_MODE: > + /* Power down OLDI TX 1 */ > + val = OLDI1_PWRDN_TX; > + break; > + > + case OLDI_SINGLE_LINK_CLONE_MODE: > + case OLDI_DUAL_LINK_MODE: > + /* No Power down */ > + val = 0; > + break; > + > + default: > + /* Power down both the OLDI TXes */ > + val = OLDI0_PWRDN_TX | OLDI1_PWRDN_TX; > + break; > + } > + } else { > + /* Power down both the OLDI TXes */ > + val = OLDI0_PWRDN_TX | OLDI1_PWRDN_TX; > + } Ugh, I hate power-down bits. So you "enable" it to disable it =). What's the default value or the register here? Or will this always be called? I.e. if we only use DPI, do we power down the OLDIs somewhere (or does it matter)? > + > + regmap_update_bits(dispc->oldi_io_ctrl, OLDI_PD_CTRL, > + OLDI0_PWRDN_TX | OLDI1_PWRDN_TX, val); > + } > } > > static void dispc_set_num_datalines(struct dispc_device *dispc, > @@ -2831,7 +2862,7 @@ int dispc_init(struct tidss_device *tidss) > dispc->vp_data[i].gamma_table = gamma_table; > } > > - if (feat->subrev == DISPC_AM65X) { > + if (feat->subrev == DISPC_AM65X || feat->subrev == DISPC_AM625) { > r = dispc_init_am65x_oldi_io_ctrl(dev, dispc); > if (r) > return r; > diff --git a/drivers/gpu/drm/tidss/tidss_dispc_regs.h b/drivers/gpu/drm/tidss/tidss_dispc_regs.h > index 13feedfe5d6d..510bee70b3b8 100644 > --- a/drivers/gpu/drm/tidss/tidss_dispc_regs.h > +++ b/drivers/gpu/drm/tidss/tidss_dispc_regs.h > @@ -238,6 +238,12 @@ enum dispc_common_regs { > #define OLDI_DAT3_IO_CTRL 0x0C > #define OLDI_CLK_IO_CTRL 0x10 > > +/* Only for AM625 OLDI TX */ > +#define OLDI_PD_CTRL 0x100 > +#define OLDI_LB_CTRL 0x104 > + > #define OLDI_PWRDN_TX BIT(8) > +#define OLDI0_PWRDN_TX BIT(0) > +#define OLDI1_PWRDN_TX BIT(1) Maybe these (the new and old ones) should be platform-prefixed. And organized so that the register and its bits are together. Tomi
Hi Tomi Thank you for the comprehensive feedback across all the patches. I am working on them. I do have some concerns which I have talked about, below. On 12-Oct-22 17:53, Tomi Valkeinen wrote: > On 28/09/2022 20:52, 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 | >> | | | | | | >> +------+ +---------+ +-------+ > > Can you have single link on OLDI 1 (OLDI 0 off)? I don't know if that > make sense on this platform, but if the pins for OLDI 0 and 1 are > different, there might be a reason on some cases for that. HW does not support a case where single link is enabled over OLDI 1 with OLDI 0 off, even though the pins are different. One could still put 2 panel nodes in DT to set OLDI in a Clone Mode and simply not use OLDI 0 pins, but I dont think that is a valid case that should be supported. > >> 2. OLDI_SINGLE_LINK_CLONE_MODE >> Duplicate Output over OLDI 0 and 1. >> +------+ +---------+ +-------+ >> | | | | | | >> | CRTC +---+--->| ENCODER +----->| PANEL | >> | | | | | | | >> +------+ | +---------+ +-------+ >> | > > I think you've got a tab in the line above, but otherwise use spaces. > >> | +---------+ +-------+ >> | | | | | >> +--->| 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 confgure the OLDI modes using of and lvds DRM helper > > "configuring" > >> functions. >> >> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> >> --- >> drivers/gpu/drm/tidss/tidss_dispc.c | 11 +++ >> drivers/gpu/drm/tidss/tidss_dispc.h | 8 ++ >> drivers/gpu/drm/tidss/tidss_drv.h | 3 + >> drivers/gpu/drm/tidss/tidss_kms.c | 146 +++++++++++++++++++++++----- >> 4 files changed, 145 insertions(+), 23 deletions(-) >> >> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c >> b/drivers/gpu/drm/tidss/tidss_dispc.c >> index 34f0da4bb3e3..88008ad39b55 100644 >> --- a/drivers/gpu/drm/tidss/tidss_dispc.c >> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c >> @@ -354,6 +354,8 @@ struct dispc_device { >> bool is_enabled; >> + enum dispc_oldi_modes oldi_mode; >> + >> struct dss_vp_data vp_data[TIDSS_MAX_PORTS]; >> u32 *fourccs; >> @@ -1958,6 +1960,15 @@ const u32 *dispc_plane_formats(struct >> dispc_device *dispc, unsigned int *len) >> return dispc->fourccs; >> } >> +int dispc_configure_oldi_mode(struct dispc_device *dispc, >> + enum dispc_oldi_modes oldi_mode) >> +{ >> + WARN_ON(!dispc); >> + >> + dispc->oldi_mode = oldi_mode; >> + return 0; >> +} > > I think "configure" means more than just storing the value. Maybe > dispc_set_oldi_mode(). And an empty line above the return. > >> + >> static s32 pixinc(int pixels, u8 ps) >> { >> if (pixels == 1) >> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h >> b/drivers/gpu/drm/tidss/tidss_dispc.h >> index b66418e583ee..45cce1054832 100644 >> --- a/drivers/gpu/drm/tidss/tidss_dispc.h >> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h >> @@ -64,6 +64,13 @@ enum dispc_dss_subrevision { >> DISPC_AM625, >> }; >> +enum dispc_oldi_modes { >> + OLDI_MODE_OFF, /* OLDI turned off / tied off in >> IP. */ >> + OLDI_SINGLE_LINK_SINGLE_MODE, /* Single Output over OLDI >> 0. */ >> + OLDI_SINGLE_LINK_CLONE_MODE, /* Duplicate Output over OLDI >> 0 and 1. */ >> + OLDI_DUAL_LINK_MODE, /* Combined Output over OLDI 0 >> and 1. */ >> +}; >> + >> struct dispc_features { >> int min_pclk_khz; >> int max_pclk_khz[DISPC_VP_MAX_BUS_TYPE]; >> @@ -131,6 +138,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); >> +int dispc_configure_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 d7f27b0b0315..2252ba0222ca 100644 >> --- a/drivers/gpu/drm/tidss/tidss_drv.h >> +++ b/drivers/gpu/drm/tidss/tidss_drv.h >> @@ -12,6 +12,9 @@ >> #define TIDSS_MAX_PORTS 4 >> #define TIDSS_MAX_PLANES 4 >> +/* For AM625-DSS with 2 OLDI TXes */ >> +#define TIDSS_MAX_BRIDGE_PER_PIPE 2 > > "BRIDGES"? > >> + >> typedef u32 dispc_irq_t; >> struct tidss_device { >> diff --git a/drivers/gpu/drm/tidss/tidss_kms.c >> b/drivers/gpu/drm/tidss/tidss_kms.c >> index 666e527a0acf..73afe390f36d 100644 >> --- a/drivers/gpu/drm/tidss/tidss_kms.c >> +++ b/drivers/gpu/drm/tidss/tidss_kms.c >> @@ -107,32 +107,84 @@ static const struct drm_mode_config_funcs >> mode_config_funcs = { >> .atomic_commit = drm_atomic_helper_commit, >> }; >> +static int tidss_get_oldi_mode(struct tidss_device *tidss) > > Return enum dispc_oldi_modes, not int. > >> +{ >> + int pixel_order; >> + struct device_node *dss_ports, *oldi0_port, *oldi1_port; >> + >> + dss_ports = of_get_next_child(tidss->dev->of_node, NULL); > > Hmm you get the next child and hope that it's the ports node? > > In any case, I think you can call of_graph_get_port_by_id() with the > tidss->dev->of_node and it'll do the right thing. I think this will only work if the child of dss node is just "ports", but we've been using "dss_ports" as the child. However, you are right. I shouldn't expect the first child to be dss_ports. I will use the "of_get_child_by_name" helper to get the dss_ports node. > >> + oldi0_port = of_graph_get_port_by_id(dss_ports, 0); >> + oldi1_port = of_graph_get_port_by_id(dss_ports, 2); > > I think you need to of_put these at some point. > >> + if (!(oldi0_port && oldi1_port)) >> + return OLDI_SINGLE_LINK_SINGLE_MODE; > > This one matches also for !oldi0 && oldi1. If oldi1 cannot be used in > single-link mode, the above should take it into account. Right. I will print a warning if somebody's trying to use (!oldi0 && oldi1) but since its a single link requirement, I will still set the OLDI for single link single mode. > >> + >> + /* >> + * 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. >> + */ >> + return OLDI_SINGLE_LINK_CLONE_MODE; >> + >> + case DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS: >> + case DRM_LVDS_DUAL_LINK_ODD_EVEN_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 IP and an cannot be change vis SW. These >> + * properties have been used to merely identify if a Dual Link >> + * configuration is required. Swapping this property in the >> panel >> + * port DT nodes will not make any difference. >> + */ > > But if they are in the wrong order, shouldn't we fail or at least give a > warning? > >> + return OLDI_DUAL_LINK_MODE; >> + >> + default: >> + return OLDI_MODE_OFF; >> + } >> +} >> + >> 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_BRIDGE_PER_PIPE]; >> u32 enc_type; >> + u32 num_bridges; >> }; >> const struct dispc_features *feat = tidss->feat; >> - u32 max_vps = feat->num_vps; >> + u32 max_ports = feat->num_max_ports; >> u32 max_planes = feat->num_planes; >> struct pipe pipes[TIDSS_MAX_PORTS]; >> u32 num_pipes = 0; >> + u32 pipe_number = 0; >> u32 crtc_mask; >> + u32 num_oldi = 0; >> + u32 oldi0_port = 0; >> + u32 hw_vp = 0; >> + enum dispc_oldi_modes oldi_mode; >> /* first find all the connected panels & bridges */ >> - for (i = 0; i < max_vps; i++) { >> + for (i = 0; i < max_ports; i++) { >> struct drm_panel *panel; >> struct drm_bridge *bridge; >> + bool bridge_req = true; >> u32 enc_type = DRM_MODE_ENCODER_NONE; >> int ret; >> @@ -146,6 +198,11 @@ static int tidss_dispc_modeset_init(struct >> tidss_device *tidss) >> return ret; >> } >> + /* default number of bridges required for a panel/bridge*/ >> + pipe_number = num_pipes; >> + pipes[pipe_number].num_bridges = 1; >> + hw_vp = i; >> + >> if (panel) { >> u32 conn_type; >> @@ -155,7 +212,43 @@ static int tidss_dispc_modeset_init(struct >> tidss_device *tidss) >> case DISPC_VP_OLDI: >> enc_type = DRM_MODE_ENCODER_LVDS; >> conn_type = DRM_MODE_CONNECTOR_LVDS; >> + >> + /* >> + * A single DSS controller cannot support 2 >> + * independent displays. If 2nd node is detected, >> + * it is for Dual Link Mode or Clone Mode. >> + * >> + * A new pipe instance is not required. >> + */ >> + if (++num_oldi == 2) { >> + pipe_number = oldi0_port; >> + hw_vp = i; >> + >> + /* 2nd OLDI DT node detected. Get its mode */ >> + oldi_mode = tidss_get_oldi_mode(tidss); >> + bridge_req = false; >> + >> + /* >> + * A separate panel bridge will only be >> + * required if 2 panels are connected for >> + * the OLDI Clone Mode. >> + */ >> + if (oldi_mode == OLDI_SINGLE_LINK_CLONE_MODE) { >> + bridge_req = true; >> + (pipes[pipe_number].num_bridges)++; >> + } >> + } else { >> + /* >> + * First OLDI DT node detected. Save it >> + * in case there is another node for Dual >> + * Link Mode or Clone Mode. >> + */ >> + oldi0_port = i; >> + oldi_mode = OLDI_SINGLE_LINK_SINGLE_MODE; >> + } >> + dispc_configure_oldi_mode(tidss->dispc, oldi_mode); >> break; >> + >> case DISPC_VP_DPI: >> enc_type = DRM_MODE_ENCODER_DPI; >> conn_type = DRM_MODE_CONNECTOR_DPI; >> @@ -173,19 +266,23 @@ static int tidss_dispc_modeset_init(struct >> tidss_device *tidss) >> return -EINVAL; >> } >> - bridge = devm_drm_panel_bridge_add(dev, panel); >> - if (IS_ERR(bridge)) { >> - dev_err(dev, >> - "failed to set up panel bridge for port %d\n", >> - i); >> - return PTR_ERR(bridge); >> + if (bridge_req) { >> + bridge = devm_drm_panel_bridge_add(dev, panel); >> + if (IS_ERR(bridge)) { >> + dev_err(dev, >> + "failed to set up panel bridge for port %d\n", >> + i); >> + return PTR_ERR(bridge); >> + } >> } >> } >> - pipes[num_pipes].hw_videoport = i; >> - pipes[num_pipes].bridge = bridge; >> - pipes[num_pipes].enc_type = enc_type; >> - num_pipes++; >> + if (bridge_req) { >> + pipes[pipe_number].hw_videoport = hw_vp; >> + pipes[pipe_number].bridge[pipes[pipe_number].num_bridges >> - 1] = bridge; >> + pipes[pipe_number].enc_type = enc_type; >> + num_pipes++; >> + } > > I need to look at this with better time. But I started to wonder, would > it be clearer to first figure out the oldi setup before the loop, rather > than figuring it out inside the loop. I'm not sure if it would help > much, though. > I had not thought about taking this approach, but it might actually be better. These patches, at the moment, do not support a case where a clone mode or dual link mode is used on a bridge instead of a panel. My edits inside the loop are panel dependent. If we do have oldi setup information prior to the beginning of the loop, the panel dependency can be removed and some commond code can be written to support an additional encoder - bridge connection should it be required. Let me know what you think! If this apparch is better indeed, I will make these changes before sending out the next revision. Regards Aradhya
This patch series adds a new compatible for the Display SubSyetem controller on TI's AM625 SoC. It further adds the required support for the same in the tidss driver. The AM625-DSS is a newer version of the DSS from the AM65X version with the major change being the addition of another OLDI TX. With the help of 2 OLDI TXes, the AM625 DSS supports OLDI displays with a resolution of upto 2K. This patch set further adds support for the OLDI on AM625, and is, in essence, the second version of the following patch series: https://patchwork.kernel.org/project/dri-devel/list/?series=660970&state=%2A&archive=both The changes in the above-mentioned series forced some re-works in this series, and are better reviewed as a single set. TODO: - Support for OLDI Bridges that work on clone / dual-link modes is yet to be added. - The pixel clock for the OLDI VP passes through a clock divider, which was being explicitly set in previous series, but that was not the right way. That patch has been dropped and a newer implementation is in works. Note: - Due to lack of hardware, only dual-link mode has been tested. Changelog: V5: - Rebase for current merge window - Add max DT ports in DSS features - Combine the OLDI support series (Changes from OLDI support series v1) - Address Tomi Valkeinen's comments 1. Update the OLDI link detection approach 2. Add port #3 for 2nd OLDI TX 3. Configure 2 panel-bridges for cloned panels 4. Drop the OLDI clock set patch 5. Drop rgb565-to-888 patch V4: - Rebase for current merge window - Add acked and reviewed by tags V3: - Change yaml enum in alphabetical order - Correct a typo V2: - Remove redundant regsiter array Aradhya Bhatia (6): dt-bindings: display: ti,am65x-dss: Add am625 dss compatible dt-bindings: display: ti: am65x-dss: Add new port for am625-dss drm/tidss: Add support for AM625 DSS drm/tidss: Add support to configure OLDI mode for am625-dss. drm/tidss: Add IO CTRL and Power support for OLDI TX in am625 drm/tidss: Enable Dual and Duplicate Modes for OLDI .../bindings/display/ti/ti,am65x-dss.yaml | 22 ++- drivers/gpu/drm/tidss/tidss_dispc.c | 155 ++++++++++++++++-- drivers/gpu/drm/tidss/tidss_dispc.h | 11 ++ drivers/gpu/drm/tidss/tidss_dispc_regs.h | 6 + drivers/gpu/drm/tidss/tidss_drv.c | 1 + drivers/gpu/drm/tidss/tidss_drv.h | 3 + drivers/gpu/drm/tidss/tidss_kms.c | 146 ++++++++++++++--- 7 files changed, 304 insertions(+), 40 deletions(-)