Messages in this thread Patch in this message |  | | Date | Tue, 11 Feb 2025 12:57:36 +0200 | | Subject | Re: [PATCH v5 3/3] drm/tidss: Add OLDI bridge support | | From | Tomi Valkeinen <> |
| |
Hi,
On 09/02/2025 18:09, Aradhya Bhatia wrote: > From: Aradhya Bhatia <a-bhatia1@ti.com> > > The AM62x and AM62Px SoCs feature 2 OLDI TXes each, which makes it > possible to connect them in dual-link or cloned single-link OLDI display > modes. The current OLDI support in tidss_dispc.c can only support for > a single OLDI TX, connected to a VP and doesn't really support > configuration of OLDIs in the other modes. The current OLDI support in > tidss_dispc.c also works on the principle that the OLDI output can only > be served by one, and only one, DSS video-port. This isn't the case in > the AM62Px SoC, where there are 2 DSS controllers present that share the > OLDI TXes. > > Having their own devicetree and their own bridge entity will help > support the various display modes and sharing possiblilities of the OLDI > hardware. > > For all these reasons, add support for the OLDI TXes as DRM bridges. > > Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> > Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev> > --- > drivers/gpu/drm/tidss/Makefile | 3 +- > drivers/gpu/drm/tidss/tidss_dispc.c | 20 +- > drivers/gpu/drm/tidss/tidss_dispc.h | 4 + > drivers/gpu/drm/tidss/tidss_dispc_regs.h | 14 + > drivers/gpu/drm/tidss/tidss_drv.c | 9 + > drivers/gpu/drm/tidss/tidss_drv.h | 5 + > drivers/gpu/drm/tidss/tidss_oldi.c | 558 +++++++++++++++++++++++ > drivers/gpu/drm/tidss/tidss_oldi.h | 51 +++ > 8 files changed, 662 insertions(+), 2 deletions(-) > create mode 100644 drivers/gpu/drm/tidss/tidss_oldi.c > create mode 100644 drivers/gpu/drm/tidss/tidss_oldi.h > > diff --git a/drivers/gpu/drm/tidss/Makefile b/drivers/gpu/drm/tidss/Makefile > index 312645271014..b6d6becf1683 100644 > --- a/drivers/gpu/drm/tidss/Makefile > +++ b/drivers/gpu/drm/tidss/Makefile > @@ -7,6 +7,7 @@ tidss-y := tidss_crtc.o \ > tidss_irq.o \ > tidss_plane.o \ > tidss_scale_coefs.o \ > - tidss_dispc.o > + tidss_dispc.o \ > + tidss_oldi.o > > obj-$(CONFIG_DRM_TIDSS) += tidss.o > diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c > index 1ad711f8d2a8..8631a89e6155 100644 > --- a/drivers/gpu/drm/tidss/tidss_dispc.c > +++ b/drivers/gpu/drm/tidss/tidss_dispc.c > @@ -466,6 +466,25 @@ static u32 dispc_vp_read(struct dispc_device *dispc, u32 hw_videoport, u16 reg) > return ioread32(base + reg); > } > > +void tidss_configure_oldi(struct tidss_device *tidss, u32 hw_videoport, > + u32 oldi_cfg) > +{ > + u32 count = 0; > + u32 oldi_reset_bit = BIT(5 + hw_videoport); > + > + dispc_vp_write(tidss->dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, oldi_cfg); > + > + if (oldi_cfg != 0) { > + while (!(oldi_reset_bit & dispc_read(tidss->dispc, DSS_SYSSTATUS)) && > + count < 10000) > + count++; > + > + if (!(oldi_reset_bit & dispc_read(tidss->dispc, DSS_SYSSTATUS))) > + dev_warn(tidss->dispc->dev, "%s: timeout waiting OLDI reset done\n", > + __func__); > + } > +}
The timeout sounds like an error. Better to return an error value, and handle it in tidss_oldi_config()?
You could also
if (!oldi_cfg) return 0;
But would it actually be nicer to have a separate void function for disabling?
> + > /* > * TRM gives bitfields as start:end, where start is the higher bit > * number. For example 7:0 > @@ -1310,7 +1329,6 @@ void dispc_vp_disable_clk(struct dispc_device *dispc, u32 hw_videoport) > * Calculate the percentage difference between the requested pixel clock rate > * and the effective rate resulting from calculating the clock divider value. > */ > -static > unsigned int dispc_pclk_diff(unsigned long rate, unsigned long real_rate) > { > int r = rate / 100, rr = real_rate / 100; > diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h > index 086327d51a90..fab248f2055a 100644 > --- a/drivers/gpu/drm/tidss/tidss_dispc.h > +++ b/drivers/gpu/drm/tidss/tidss_dispc.h > @@ -94,6 +94,10 @@ extern const struct dispc_features dispc_am62a7_feats; > extern const struct dispc_features dispc_am65x_feats; > extern const struct dispc_features dispc_j721e_feats; > > +void tidss_configure_oldi(struct tidss_device *tidss, u32 hw_videoport, > + u32 oldi_cfg); > +unsigned int dispc_pclk_diff(unsigned long rate, unsigned long real_rate); > + > void dispc_set_irqenable(struct dispc_device *dispc, dispc_irq_t mask); > dispc_irq_t dispc_read_and_clear_irqstatus(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..03f7098029e6 100644 > --- a/drivers/gpu/drm/tidss/tidss_dispc_regs.h > +++ b/drivers/gpu/drm/tidss/tidss_dispc_regs.h > @@ -226,6 +226,20 @@ enum dispc_common_regs { > #define DISPC_VP_DSS_DMA_THREADSIZE 0x170 /* J721E */ > #define DISPC_VP_DSS_DMA_THREADSIZE_STATUS 0x174 /* J721E */ > > +/* OLDI Config Bits (DISPC_VP_DSS_OLDI_CFG) */ > +#define OLDI_ENABLE BIT(0) > +#define OLDI_MAP (BIT(1) | BIT(2) | BIT(3)) > +#define OLDI_SRC BIT(4) > +#define OLDI_CLONE_MODE BIT(5) > +#define OLDI_MASTERSLAVE BIT(6) > +#define OLDI_DEPOL BIT(7) > +#define OLDI_MSB BIT(8) > +#define OLDI_LBEN BIT(9) > +#define OLDI_LBDATA BIT(10) > +#define OLDI_DUALMODESYNC BIT(11) > +#define OLDI_SOFTRST BIT(12) > +#define OLDI_TPATCFG BIT(13) > + > /* > * OLDI IO_CTRL register offsets. On AM654 the registers are found > * from CTRL_MMR0, there the syscon regmap should map 0x14 bytes from > diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c > index 7c8fd6407d82..27b9f86f1eb2 100644 > --- a/drivers/gpu/drm/tidss/tidss_drv.c > +++ b/drivers/gpu/drm/tidss/tidss_drv.c > @@ -24,6 +24,7 @@ > #include "tidss_drv.h" > #include "tidss_kms.h" > #include "tidss_irq.h" > +#include "tidss_oldi.h" > > /* Power management */ > > @@ -148,6 +149,10 @@ static int tidss_probe(struct platform_device *pdev) > return ret; > } > > + ret = tidss_oldi_init(tidss); > + if (ret) > + return dev_err_probe(dev, ret, "failed to init OLDI\n"); > + > pm_runtime_enable(dev); > > pm_runtime_set_autosuspend_delay(dev, 1000); > @@ -204,6 +209,8 @@ static int tidss_probe(struct platform_device *pdev) > pm_runtime_dont_use_autosuspend(dev); > pm_runtime_disable(dev); > > + tidss_oldi_deinit(tidss); > + > return ret; > } > > @@ -228,6 +235,8 @@ static void tidss_remove(struct platform_device *pdev) > pm_runtime_dont_use_autosuspend(dev); > pm_runtime_disable(dev); > > + tidss_oldi_deinit(tidss); > + > /* devm allocated dispc goes away with the dev so mark it NULL */ > dispc_remove(tidss); > > diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h > index d7f27b0b0315..6c0fe1d989ee 100644 > --- a/drivers/gpu/drm/tidss/tidss_drv.h > +++ b/drivers/gpu/drm/tidss/tidss_drv.h > @@ -11,8 +11,10 @@ > > #define TIDSS_MAX_PORTS 4 > #define TIDSS_MAX_PLANES 4 > +#define TIDSS_MAX_OLDI_TXES 2 > > typedef u32 dispc_irq_t; > +struct tidss_oldi; > > struct tidss_device { > struct drm_device ddev; /* DRM device for DSS */ > @@ -27,6 +29,9 @@ struct tidss_device { > unsigned int num_planes; > struct drm_plane *planes[TIDSS_MAX_PLANES]; > > + unsigned int num_oldis; > + struct tidss_oldi *oldis[TIDSS_MAX_OLDI_TXES]; > + > unsigned int irq; > > spinlock_t wait_lock; /* protects the irq masks */ > diff --git a/drivers/gpu/drm/tidss/tidss_oldi.c b/drivers/gpu/drm/tidss/tidss_oldi.c > new file mode 100644 > index 000000000000..4af13a01f546 > --- /dev/null > +++ b/drivers/gpu/drm/tidss/tidss_oldi.c > @@ -0,0 +1,558 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright (C) 2024 - Texas Instruments Incorporated > + * > + * Aradhya Bhatia <a-bhatia1@ti.com> > + */ > + > +#include <linux/clk.h> > +#include <linux/of.h> > +#include <linux/of_graph.h> > +#include <linux/mfd/syscon.h> > +#include <linux/media-bus-format.h> > +#include <linux/regmap.h> > + > +#include <drm/drm_atomic_helper.h> > +#include <drm/drm_bridge.h> > +#include <drm/drm_of.h> > + > +#include "tidss_dispc.h" > +#include "tidss_dispc_regs.h" > +#include "tidss_oldi.h" > + > +struct tidss_oldi { > + struct tidss_device *tidss; > + struct device *dev; > + > + struct drm_bridge bridge; > + struct drm_bridge *next_bridge; > + > + enum tidss_oldi_link_type link_type; > + const struct oldi_bus_format *bus_format; > + u32 oldi_instance; > + u32 companion_instance; > + u32 parent_vp; > + > + struct clk *serial; > + struct regmap *io_ctrl; > +}; > + > +static const struct oldi_bus_format oldi_bus_formats[] = { > + { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, 18, SPWG_18, MEDIA_BUS_FMT_RGB666_1X18 }, > + { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, 24, SPWG_24, MEDIA_BUS_FMT_RGB888_1X24 }, > + { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, 24, JEIDA_24, MEDIA_BUS_FMT_RGB888_1X24 }, > +}; > + > +#define OLDI_IDLE_CLK_HZ 25000000 /*25 MHz */ > + > +static inline struct tidss_oldi * > +drm_bridge_to_tidss_oldi(struct drm_bridge *bridge) > +{ > + return container_of(bridge, struct tidss_oldi, bridge); > +} > + > +static int tidss_oldi_bridge_attach(struct drm_bridge *bridge, > + enum drm_bridge_attach_flags flags) > +{ > + struct tidss_oldi *oldi = drm_bridge_to_tidss_oldi(bridge); > + > + if (!oldi->next_bridge) { > + dev_err(oldi->dev, > + "%s: OLDI%u Failure attach next bridge\n", > + __func__, oldi->oldi_instance); > + return -ENODEV; > + } > + > + if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) { > + dev_err(oldi->dev, > + "%s: OLDI%u DRM_BRIDGE_ATTACH_NO_CONNECTOR is mandatory.\n", > + __func__, oldi->oldi_instance); > + return -EINVAL; > + } > + > + return drm_bridge_attach(bridge->encoder, oldi->next_bridge, > + bridge, flags); > +} > + > +static int > +tidss_oldi_set_serial_clk(struct tidss_oldi *oldi, unsigned long rate) > +{ > + unsigned long new_rate; > + int ret; > + > + ret = clk_set_rate(oldi->serial, rate); > + if (ret) { > + dev_err(oldi->dev, > + "OLDI%u: failed to set serial clk rate to %lu Hz\n", > + oldi->oldi_instance, rate); > + return ret; > + } > + > + new_rate = clk_get_rate(oldi->serial); > + > + if (dispc_pclk_diff(rate, new_rate) > 5) > + dev_warn(oldi->dev, > + "OLDI%u Clock rate %lu differs over 5%% from requested %lu\n", > + oldi->oldi_instance, new_rate, rate); > + > + dev_dbg(oldi->dev, "OLDI%u: new rate %lu Hz (requested %lu Hz)\n", > + oldi->oldi_instance, clk_get_rate(oldi->serial), rate); > + > + return 0; > +} > + > +static void tidss_oldi_tx_power(struct tidss_oldi *oldi, bool enable) > +{ > + u32 mask; > + > + /* > + * The power control bits are Active Low, and remain powered off by > + * default. That is, the bits are set to 1. To power on the OLDI TXes, > + * the bits must be cleared to 0. Since there are cases where not all > + * OLDI TXes are being used, the power logic selectively powers them > + * on. > + * Setting the variable 'val' to particular bit masks, makes sure that > + * the unrequired OLDI TXes remain powered off. > + */ > + > + if (enable) { > + switch (oldi->link_type) { > + case OLDI_MODE_SINGLE_LINK: > + /* Power-on only the required OLDI TX's IO*/ > + mask = OLDI_PWRDOWN_TX(oldi->oldi_instance) | OLDI_PWRDN_BG; > + break; > + case OLDI_MODE_CLONE_SINGLE_LINK: > + case OLDI_MODE_DUAL_LINK: > + /* Power-on both the OLDI TXes' IOs */ > + mask = OLDI_PWRDOWN_TX(oldi->oldi_instance) | > + OLDI_PWRDOWN_TX(oldi->companion_instance) | > + OLDI_PWRDN_BG; > + break; > + default: > + /* > + * This code execution should never reach here as any > + * OLDI with an unsupported OLDI mode would never get > + * registered in the first place. > + * However, power-off the OLDI in concern just in case. > + */ > + mask = OLDI_PWRDOWN_TX(oldi->oldi_instance); > + enable = false; > + break; > + } > + } else { > + switch (oldi->link_type) { > + case OLDI_MODE_CLONE_SINGLE_LINK: > + case OLDI_MODE_DUAL_LINK: > + mask = OLDI_PWRDOWN_TX(oldi->oldi_instance) | > + OLDI_PWRDOWN_TX(oldi->companion_instance) | > + OLDI_PWRDN_BG; > + break; > + case OLDI_MODE_SINGLE_LINK: > + default: > + mask = OLDI_PWRDOWN_TX(oldi->oldi_instance); > + break; > + } > + } > + > + regmap_update_bits(oldi->io_ctrl, OLDI_PD_CTRL, mask, enable ? 0 : mask); > +} > + > +static int tidss_oldi_config(struct tidss_oldi *oldi) > +{ > + const struct oldi_bus_format *bus_fmt = NULL; > + u32 oldi_cfg = 0; > + > + bus_fmt = oldi->bus_format; > + > + /* > + * MASTERSLAVE and SRC bits of OLDI Config are always set to 0. > + */ > + > + if (bus_fmt->data_width == 24) > + oldi_cfg |= OLDI_MSB; > + else if (bus_fmt->data_width != 18) > + dev_warn(oldi->dev, > + "OLDI%u: DSS port width %d not supported\n", > + oldi->oldi_instance, bus_fmt->data_width); > + > + oldi_cfg |= OLDI_DEPOL; > + > + oldi_cfg = (oldi_cfg & (~OLDI_MAP)) | (bus_fmt->oldi_mode_reg_val << 1); > + > + oldi_cfg |= OLDI_SOFTRST; > + > + oldi_cfg |= OLDI_ENABLE; > + > + switch (oldi->link_type) { > + case OLDI_MODE_SINGLE_LINK: > + /* All configuration is done for this mode. */ > + break; > + > + case OLDI_MODE_CLONE_SINGLE_LINK: > + oldi_cfg |= OLDI_CLONE_MODE; > + break; > + > + case OLDI_MODE_DUAL_LINK: > + /* data-mapping field also indicates dual-link mode */ > + oldi_cfg |= BIT(3); > + oldi_cfg |= OLDI_DUALMODESYNC; > + break; > + > + default: > + dev_err(oldi->dev, "OLDI%u: Unsupported mode.\n", > + oldi->oldi_instance); > + return -EINVAL; > + } > + > + tidss_configure_oldi(oldi->tidss, oldi->parent_vp, oldi_cfg); > + > + return 0; > +} > + > +static void tidss_oldi_atomic_pre_enable(struct drm_bridge *bridge, > + struct drm_bridge_state *old_bridge_state) > +{ > + struct tidss_oldi *oldi = drm_bridge_to_tidss_oldi(bridge); > + struct drm_atomic_state *state = old_bridge_state->base.state; > + struct drm_connector *connector; > + struct drm_connector_state *conn_state; > + struct drm_crtc_state *crtc_state; > + struct drm_display_mode *mode; > + > + connector = drm_atomic_get_new_connector_for_encoder(state, > + bridge->encoder); > + if (WARN_ON(!connector)) > + return; > + > + conn_state = drm_atomic_get_new_connector_state(state, connector); > + if (WARN_ON(!conn_state)) > + return; > + > + crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc); > + if (WARN_ON(!crtc_state)) > + return; > + > + mode = &crtc_state->adjusted_mode; > + > + /* Configure the OLDI params*/ > + tidss_oldi_config(oldi); > + > + /* Set the OLDI serial clock (7 times the pixel clock) */ > + tidss_oldi_set_serial_clk(oldi, mode->clock * 7 * 1000); > + > + /* Enable OLDI IO power */ > + tidss_oldi_tx_power(oldi, true); > +} > + > +static void tidss_oldi_atomic_post_disable(struct drm_bridge *bridge, > + struct drm_bridge_state *old_bridge_state) > +{ > + struct tidss_oldi *oldi = drm_bridge_to_tidss_oldi(bridge); > + > + /* Disable OLDI IO power */ > + tidss_oldi_tx_power(oldi, false); > + > + /* Set the OLDI serial clock to IDLE Frequency */ > + tidss_oldi_set_serial_clk(oldi, OLDI_IDLE_CLK_HZ); > + > + /* Clear OLDI Config */ > + tidss_configure_oldi(oldi->tidss, oldi->parent_vp, 0); > +} > + > +#define MAX_INPUT_SEL_FORMATS 1 > + > +static u32 *tidss_oldi_atomic_get_input_bus_fmts(struct drm_bridge *bridge, > + struct drm_bridge_state *bridge_state, > + struct drm_crtc_state *crtc_state, > + struct drm_connector_state *conn_state, > + u32 output_fmt, > + unsigned int *num_input_fmts) > +{ > + struct tidss_oldi *oldi = drm_bridge_to_tidss_oldi(bridge); > + u32 *input_fmts; > + int i; > + > + *num_input_fmts = 0; > + > + for (i = 0; i < ARRAY_SIZE(oldi_bus_formats); i++) > + if (oldi_bus_formats[i].bus_fmt == output_fmt) > + break; > + > + if (i == ARRAY_SIZE(oldi_bus_formats)) > + return NULL; > + > + input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts), > + GFP_KERNEL); > + if (!input_fmts) > + return NULL; > + > + *num_input_fmts = 1; > + input_fmts[0] = oldi_bus_formats[i].input_bus_fmt; > + oldi->bus_format = &oldi_bus_formats[i]; > + > + return input_fmts; > +} > + > +static const struct drm_bridge_funcs tidss_oldi_bridge_funcs = { > + .attach = tidss_oldi_bridge_attach, Looks like an extra tab there?
Other than those two cosmetic issues, I think this looks fine. I also tested on AM62-SK.
However, one more thing. We'll have a separate OLDI bridge, but we still will have the old OLDI code for AM65x in the tidss_dispc.c. And to mix things up, we will have some new OLDI code there too (tidss_configure_oldi). Could you check the AM65x specific code and perhaps rename the functions to am65x or such, to make this clearer. Perhaps also the DISPC_VP_OLDI should be DISPC_VP_OLDI_AM65X, as that VP type shouldn't be used for anything else.
Tomi
|  |