| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <fd5e0b25-2e43-42dd-a6c3-103528c23593@ti.com>
Date: Mon, 15 Jul 2024 15:32:36 +0530
From: Aradhya Bhatia <a-bhatia1@...com>
To: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>,
Jyri Sarha
<jyri.sarha@....fi>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>,
Thomas Zimmermann <tzimmermann@...e.de>,
Neil Armstrong <neil.armstrong@...aro.org>,
Laurent Pinchart
<Laurent.pinchart@...asonboard.com>,
David Airlie <airlied@...il.com>, Daniel
Vetter <daniel@...ll.ch>,
Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski
<krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>
CC: DRI Development List <dri-devel@...ts.freedesktop.org>,
Devicetree List
<devicetree@...r.kernel.org>,
Linux Kernel List
<linux-kernel@...r.kernel.org>,
Nishanth Menon <nm@...com>, Vignesh
Raghavendra <vigneshr@...com>,
Praneeth Bajjuri <praneeth@...com>, Udit Kumar
<u-kumar1@...com>,
Francesco Dolcini <francesco@...cini.it>,
Alexander
Sverdlin <alexander.sverdlin@...mens.com>,
Randolph Sapp <rs@...com>, Devarsh
Thakkar <devarsht@...com>,
Jayesh Choudhary <j-choudhary@...com>, Jai Luthra
<j-luthra@...com>
Subject: Re: [PATCH 4/4] drm/tidss: Add OLDI bridge support
Hi Tomi,
Thank you for reviewing the patches!
On 27/06/24 18:41, Tomi Valkeinen wrote:
> On 11/05/2024 22:30, Aradhya Bhatia wrote:
>> Up till now, the OLDI support in tidss was integrated within the tidss
>> dispc.
>> This was fine till the OLDI was one-to-mapped with the DSS video-port
>> (VP).
>> The AM62 and AM62P SoCs have 2 OLDI TXes that can support dual-lvds /
>> lvds-clone
>> modes.
>>
>> Add OLDI TXes as separate DRM bridge entities to better support the
>> new LVDS
>> configurations.
>>
>> Signed-off-by: Aradhya Bhatia <a-bhatia1@...com>
>> ---
>> Note:
>>
>> The OLDI configuration should happen before the video-port
>> configuration takes
>> place in tidss_crtc_atomic_enable hook. I have posted a patch allowing
>> DRM
>> bridges to get enabled before the CRTC of that bridge is enabled[0].
>> This patch
>> uses the bridge hooks introduced in [0], and hence will not compile
>> without [0].
>>
>> [0]: Dependency Patch: Introduce early_enable / late_disable drm
>> bridge APIs
>> https://lore.kernel.org/all/20240511153051.1355825-7-a-bhatia1@ti.com/
>>
>> ---
>> drivers/gpu/drm/tidss/Makefile | 3 +-
>> drivers/gpu/drm/tidss/tidss_dispc.c | 11 +-
>> drivers/gpu/drm/tidss/tidss_dispc.h | 4 +
>> drivers/gpu/drm/tidss/tidss_drv.c | 13 +-
>> drivers/gpu/drm/tidss/tidss_drv.h | 4 +
>> drivers/gpu/drm/tidss/tidss_oldi.c | 568 ++++++++++++++++++++++++++++
>> drivers/gpu/drm/tidss/tidss_oldi.h | 73 ++++
>> 7 files changed, 673 insertions(+), 3 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..4961da3989c0 100644
>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>> @@ -466,6 +466,16 @@ static u32 dispc_vp_read(struct dispc_device
>> *dispc, u32 hw_videoport, u16 reg)
>> return ioread32(base + reg);
>> }
>> +u32 tidss_get_status(struct tidss_device *tidss)
>> +{
>> + return dispc_read(tidss->dispc, DSS_SYSSTATUS);
>> +}
>> +
>> +void tidss_configure_oldi(struct tidss_device *tidss, u32
>> hw_videoport, u32 val)
>> +{
>> + return dispc_vp_write(tidss->dispc, hw_videoport,
>> DISPC_VP_DSS_OLDI_CFG, val);
>> +}
>> +
>> /*
>> * TRM gives bitfields as start:end, where start is the higher bit
>> * number. For example 7:0
>> @@ -1310,7 +1320,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..800a73457aff 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;
>> +u32 tidss_get_status(struct tidss_device *tidss);
>> +void tidss_configure_oldi(struct tidss_device *tidss, u32
>> hw_videoport, u32 val);
>> +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_drv.c
>> b/drivers/gpu/drm/tidss/tidss_drv.c
>> index d15f836dca95..fd90e8498cc2 100644
>> --- a/drivers/gpu/drm/tidss/tidss_drv.c
>> +++ b/drivers/gpu/drm/tidss/tidss_drv.c
>> @@ -23,6 +23,7 @@
>> #include "tidss_drv.h"
>> #include "tidss_kms.h"
>> #include "tidss_irq.h"
>> +#include "tidss_oldi.h"
>> /* Power management */
>> @@ -140,10 +141,17 @@ static int tidss_probe(struct platform_device
>> *pdev)
>> spin_lock_init(&tidss->wait_lock);
>> + ret = tidss_oldi_init(tidss);
>> + if (ret) {
>> + if (ret != -EPROBE_DEFER)
>> + dev_err(dev, "failed to init OLDI (%d)\n", ret);
>> + return ret;
>> + }
>> +
>> ret = dispc_init(tidss);
>> if (ret) {
>> dev_err(dev, "failed to initialize dispc: %d\n", ret);
>> - return ret;
>> + goto err_oldi_deinit;
>> }
>> pm_runtime_enable(dev);
>> @@ -202,6 +210,9 @@ static int tidss_probe(struct platform_device *pdev)
>> pm_runtime_dont_use_autosuspend(dev);
>> pm_runtime_disable(dev);
>> +err_oldi_deinit:
>> + tidss_oldi_deinit(tidss);
>
> Shouldn't this be called tidss_remove()?
Yes!
>
>> +
>> return ret;
>> }
>> diff --git a/drivers/gpu/drm/tidss/tidss_drv.h
>> b/drivers/gpu/drm/tidss/tidss_drv.h
>> index d7f27b0b0315..4ccdc177d171 100644
>> --- a/drivers/gpu/drm/tidss/tidss_drv.h
>> +++ b/drivers/gpu/drm/tidss/tidss_drv.h
>> @@ -11,6 +11,7 @@
>> #define TIDSS_MAX_PORTS 4
>> #define TIDSS_MAX_PLANES 4
>> +#define TIDSS_MAX_OLDI_TXES 2
>> typedef u32 dispc_irq_t;
>> @@ -27,6 +28,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];
>
> You should probably have "struct tidss_oldi;" in this file.
Okay! I will add that.
>
>> +
>> 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..fd96ca815542
>> --- /dev/null
>> +++ b/drivers/gpu/drm/tidss/tidss_oldi.c
>> @@ -0,0 +1,568 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (C) 2023 - Texas Instruments Incorporated
>> + *
>> + * Aradhya Bhatia <a-bhati1@...com>
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.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_panel.h>
>> +#include <drm/drm_of.h>
>> +#include <drm/drm_crtc.h>
>> +#include <drm/drm_probe_helper.h>
>> +
>> +#include "tidss_drv.h"
>> +#include "tidss_oldi.h"
>> +
>> +struct tidss_oldi {
>> + struct tidss_device *tidss;
>> + struct device *dev;
>> +
>> + struct drm_bridge bridge;
>> + struct drm_bridge *next_bridge;
>> +
>> + struct drm_panel *panel;
>> +
>> + enum tidss_oldi_link_type link_type;
>> + const struct oldi_bus_format *bus_format;
>> + u32 oldi_instance;
>> + u32 parent_vp;
>> +
>> + struct clk *s_clk;
>> + 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 },
>> +};
>> +
>> +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%d Failure attach next bridge\n",
>> + __func__, oldi->oldi_instance);
>
> Here and in other places: the oldi_instance is unsigned, so %u.
Okay!
>
>> + return -ENODEV;
>> + }
>> +
>> + if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
>> + dev_err(oldi->dev,
>> + "%s: OLDI%d 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
>> +oldi_set_serial_clk(struct tidss_oldi *oldi, unsigned long rate)
>
> This should be "tidss_oldi_set_serial_clk".
Okay!
>
>> +{
>> + unsigned long new_rate;
>> + int ret;
>> +
>> + ret = clk_set_rate(oldi->s_clk, rate);
>> + if (ret) {
>> + dev_err(oldi->dev,
>> + "OLDI%d: failed to set serial clk rate to %lu Hz\n",
>> + oldi->oldi_instance, rate);
>> + return ret;
>> + }
>> +
>> + new_rate = clk_get_rate(oldi->s_clk);
>> +
>> + if (dispc_pclk_diff(rate, new_rate) > 5)
>> + dev_warn(oldi->dev,
>> + "OLDI%d Clock rate %lu differs over 5%% from requested
>> %lu\n",
>> + oldi->oldi_instance, new_rate, rate);
>> +
>> + dev_dbg(oldi->dev, "OLDI%d: new rate %lu Hz (requested %lu Hz)\n",
>> + oldi->oldi_instance, clk_get_rate(oldi->s_clk), rate);
>> +
>> + return 0;
>> +}
>> +
>> +static void tidss_oldi_tx_power(struct tidss_oldi *oldi, bool power)
>> +{
>> + u32 val = 0, mask;
>
> Unnecessary initialization.
>
> Isn't the val variable unnecessary? You could just:
>
> regmap_update_bits(oldi->io_ctrl, OLDI_PD_CTRL, mask, power ? 0 : mask);
>
> Oh, hmm, wait... You sometimes turn off the oldis, even if power == true
> (the default cases)? I guess that's for the OLDI_MODE_UNSUPPORTED case.
> But why is tidss_oldi_tx_power() even called in that scenario?
>
> Also, I think usually "power" would be named "enable".
Ideally, it would never go in the default case. It's only supposed to be
a defensive programming.
I will add a comment mentioning the same. I am also planning to make
some changes around the OLDI selection logic for different OLDI modes,
which will help me drop the val variable entirely.
>
>> +
>> + if (WARN_ON(!oldi->io_ctrl))
>
> if oldi->io_ctrl == NULL, hasn't the probe already failed and this can
> never be called? The WARN_ON(), and the whole if() feels unnecessary to me.
I will drop this.
>
>> + return;
>> +
>> + /*
>> + * 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 (power) {
>> + val = 0;
>> + switch (oldi->link_type) {
>> + case OLDI_MODE_SINGLE_LINK:
>> + if (oldi->oldi_instance == OLDI(0))
>> + mask = OLDI_PWRDN_TX(0) | OLDI_PWRDN_BG;
>> + else if (oldi->oldi_instance == OLDI(1))
>> + mask = OLDI_PWRDN_TX(1) | OLDI_PWRDN_BG;
>> +
>> + break;
>> + case OLDI_MODE_CLONE_SINGLE_LINK:
>> + case OLDI_MODE_DUAL_LINK:
>> + mask = OLDI_PWRDN_TX(0) | OLDI_PWRDN_TX(1) | OLDI_PWRDN_BG;
>> + break;
>> + default:
>> + if (oldi->oldi_instance == OLDI(0))
>> + mask = OLDI_PWRDN_TX(0);
>> + else if (oldi->oldi_instance == OLDI(1))
>> + mask = OLDI_PWRDN_TX(1);
>> +
>> + val = mask;
>> + break;
>> + }
>> + } else {
>> + switch (oldi->link_type) {
>> + case OLDI_MODE_CLONE_SINGLE_LINK:
>> + case OLDI_MODE_DUAL_LINK:
>> + mask = OLDI_PWRDN_TX(0) | OLDI_PWRDN_TX(1) | OLDI_PWRDN_BG;
>> + break;
>> + case OLDI_MODE_SINGLE_LINK:
>> + default:
>> + if (oldi->oldi_instance == OLDI(0))
>> + mask = OLDI_PWRDN_TX(0);
>> + else if (oldi->oldi_instance == OLDI(1))
>> + mask = OLDI_PWRDN_TX(1);
>> +
>> + break;
>> + }
>> + val = mask;
>> + }
>> +
>> + regmap_update_bits(oldi->io_ctrl, OLDI_PD_CTRL, mask, val);
>> +}
>> +
>> +static int tidss_oldi_config(struct tidss_oldi *oldi)
>> +{
>> + const struct oldi_bus_format *bus_fmt = NULL;
>> + u32 oldi_reset_bit = BIT(5);
>> + int count = 0;
>> + 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%d: 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%d: Unsupported mode.\n",
>> + oldi->oldi_instance);
>> + return -EINVAL;
>> + }
>> +
>> + tidss_configure_oldi(oldi->tidss, oldi->parent_vp, oldi_cfg);
>
> If you expose this kind of function, I think you might as well move the
> below while loop into that function, and return an error code from
> tidss_configure_oldi. Then you don't need to expose tidss_get_status()>
> To handle the other call to tidss_configure_oldi() where you pass 0 as
> the oldi_cfg, you could either skip the while loop for oldi_cfg == 0
> (are you sure you don't need to wait in that case too?), or have a
> separate function in tidss for disabling oldi.
Yeah, we don't need to wait in the OLDI reset status when we are
disabling. Writing 0 to OLDI CFG during disable will assert the reset.
Since we are not de-asserting it until we enable, waiting is not required.
I will skip the wait when oldi_cfg == 0 as you suggested.
>
> But looking at the AM62a TRM, this doesn't seem to follow the steps. It
> has:
>
> 4. Software checks that the PLLs are stable and locked.
>
> 5. Release OLDITX_SW_RST = 1 (by configuring DSS0_VP_DSS_OLDI_CFG[12]
> SOFTRST register bit).
>
> But you set the OLDITX_SW_RST while configuring.
Hmm, I followed convention from the dispc_enable_oldi() function in
tidss_dispc.c.
I think the TRM there asks for bits to be written one by one, but
functionally it hasn't ever been an issue.
>
>> + while (!(oldi_reset_bit & tidss_get_status(oldi->tidss)) &&
>> + count < 10000)
>> + count++;
>> +
>> + if (!(oldi_reset_bit & tidss_get_status(oldi->tidss)))
>> + dev_warn(oldi->dev, "%s: OLDI%d timeout waiting OLDI reset
>> done\n",
>> + __func__, oldi->oldi_instance);
>> +
>> + return 0;
>> +}
>> +
>> +static void tidss_oldi_atomic_early_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);
>> +
>> + /* Enable the OLDI serial clock (7 times the pixel clock) */
>
> This doesn't enable the clk, just sets the rate.
Okay!
>
>> + oldi_set_serial_clk(oldi, mode->clock * 7 * 1000);
>> +
>> + /* Enable OLDI IO power */
>> + tidss_oldi_tx_power(oldi, true);
>> +}
>> +
>> +static void tidss_oldi_atomic_late_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);
>> +
>> + /* Disable OLDI clock by setting IDLE Frequency */
>
> And this doesn't disable the clock...
Okay!
>
> What is the reason to change the freq at disable time anyway, if the
> power and OLDI are disabled?
There is a clock-divider is present between the serial PLL, and the DSS
VP clock input. When the PLL generates the serial frequency (s_clk)
(s_clk = 7x pixel frequency), the clock-divider divides the frequency by
7, thereby ensuring that the DSS VP clock input gets s_clk / 7 value.
((s_clk / 7) => ((7x pixel freq / 7) => (pixel freq))
Once the OLDI is disabled, the by-7 clock divider also gets disabled.
This puts the clock divider in the by-pass mode.
The DSS VP then directly receives serial clock from the PLL (instead
of the 1/7th serial clock).
The HW design team has mentioned that receving such a high frequency
clock can be damaging to the DSS hardware. Hence we drop it down to
lower levels before disbaling OLDI.
>
>> + 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 int tidss_oldi_atomic_check(struct drm_bridge *bridge,
>> + struct drm_bridge_state *bridge_state,
>> + struct drm_crtc_state *crtc_state,
>> + struct drm_connector_state *conn_state)
>> +{
>> + /*
>> + * There might be flags negotiation supported in future but
>> + * set the bus flags in atomic_check statically for now.
>> + */
>> +
>> + /* Not sure what this is required for, at the moment */
>
> Do you mean that this is required (for OLDI to work) but you don't know
> why, or that this is not required?
That is a stray comment that got in. Since DSS is doing most checks, we
don't need an atomic check yet.
I will drop the atomic_check part from tidss oldi completely.
>
>> + bridge_state->input_bus_cfg.flags =
>> bridge->timings->input_bus_flags;
>> +
>> + return 0;
>> +}
>> +
>> +static const struct drm_bridge_funcs tidss_oldi_bridge_funcs = {
>> + .attach = tidss_oldi_bridge_attach,
>> + .atomic_check = tidss_oldi_atomic_check,
>> + .atomic_early_enable = tidss_oldi_atomic_early_enable,
>> + .atomic_late_disable = tidss_oldi_atomic_late_disable,
>> + .atomic_get_input_bus_fmts = tidss_oldi_atomic_get_input_bus_fmts,
>> + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>> + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
>> + .atomic_reset = drm_atomic_helper_bridge_reset,
>> +};
>> +
>> +static int get_oldi_mode(struct device_node *oldi_tx)
>> +{
>> + struct device_node *companion;
>> + struct device_node *port0, *port1;
>> + int pixel_order;
>> +
>> + /*
>> + * Find if the OLDI is paired with another OLDI for combined OLDI
>> + * operation (dual-lvds or clone).
>> + */
>> + companion = of_parse_phandle(oldi_tx, "ti,companion-oldi", 0);
>> + if (!companion) {
>> + if (of_property_read_bool(oldi_tx, "ti,secondary-oldi"))
>> + return OLDI_MODE_SECONDARY;
>> +
>> + /*
>> + * The OLDI TX does not have a companion, nor is it a
>> + * secondary OLDI. It will operate independently.
>> + */
>> + return OLDI_MODE_SINGLE_LINK;
>> + }
>> +
>> + /*
>> + * We need to work out if the sink is expecting us to function in
>> + * dual-link mode. We do this by looking at the DT port nodes we are
>> + * connected to, if they are marked as expecting even pixels and
>> + * odd pixels than we need to enable vertical stripe output.
>> + */
>> + port0 = of_graph_get_port_by_id(oldi_tx, 1);
>> + port1 = of_graph_get_port_by_id(companion, 1);
>> + pixel_order = drm_of_lvds_get_dual_link_pixel_order(port0, port1);
>> + of_node_put(port0);
>> + of_node_put(port1);
>> + of_node_put(companion);
>> +
>> + 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_MODE_CLONE_SINGLE_LINK;
>> +
>> + case DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS:
>> + return OLDI_MODE_DUAL_LINK;
>> +
>> + /* Unsupported OLDI Modes */
>> + case DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS:
>> + default:
>> + return OLDI_MODE_UNSUPPORTED;
>> + }
>> +}
>> +
>> +static u32 get_parent_dss_vp(struct device_node *oldi_tx, u32
>> *parent_vp)
>
> The return value should be int.
Okay!
>
>> +{
>> + struct device_node *ep, *dss_port;
>> + int ret = 0;
>
> Unnecessary initialization.
Okay!
>
>> +
>> + ep = of_graph_get_endpoint_by_regs(oldi_tx, OLDI_INPUT_PORT, -1);
>> + if (ep) {
>> + dss_port = of_graph_get_remote_port(ep);
>> + if (!dss_port) {
>> + ret = -ENODEV;
>> + goto err_return_ep_port;
>> + }
>> +
>> + ret = of_property_read_u32(dss_port, "reg", parent_vp);
>> +
>> + of_node_put(dss_port);
>> +err_return_ep_port:
>> + of_node_put(ep);
>> + return ret;
>> + }
>> +
>> + return -ENODEV;
>> +}
>> +
>> +static const struct drm_bridge_timings default_tidss_oldi_timings = {
>> + .input_bus_flags = DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE
>> + | DRM_BUS_FLAG_DE_HIGH,
>> +};
>> +
>> +void tidss_oldi_deinit(struct tidss_device *tidss)
>> +{
>> + for (int i = 0; i < tidss->num_oldis; i++) {
>> + if (tidss->oldis[i]) {
>> + drm_bridge_remove(&tidss->oldis[i]->bridge);
>> + tidss->oldis[i] = NULL;
>> + }
>> + }
>> +}
>> +
>> +int tidss_oldi_init(struct tidss_device *tidss)
>> +{
>> + struct tidss_oldi *oldi;
>> + struct device_node *child;
>> + struct drm_bridge *bridge;
>> + struct drm_panel *panel;
>> + u32 parent_vp, oldi_instance;
>> + enum tidss_oldi_link_type link_type = OLDI_MODE_UNSUPPORTED;
>> + struct device_node *oldi_parent;
>> + int ret = 0;
>> +
>> + tidss->num_oldis = 0;
>> +
>> + oldi_parent = of_get_child_by_name(tidss->dev->of_node,
>> "oldi-txes");
>> + if (!oldi_parent)
>> + /* Return gracefully */
>> + return 0;
>> +
>> + for_each_child_of_node(oldi_parent, child) {
>> + ret = get_parent_dss_vp(child, &parent_vp);
>> + if (ret) {
>> + if (ret == -ENODEV) {
>> + /*
>> + * ENODEV means that this particular OLDI node
>> + * is not connected with the DSS, which is not
>> + * a harmful case. There could be another OLDI
>> + * which may still be connected.
>> + * Continue to search for that.
>> + */
>> + ret = 0;
>> + continue;
>> + }
>> + goto err_put_node;
>> + }
>> +
>> + ret = of_property_read_u32(child, "reg", &oldi_instance);
>> + if (ret)
>> + goto err_put_node;
>> +
>> + /*
>> + * Now that its confirmed that OLDI is connected with DSS, let's
>> + * continue getting the OLDI sinks ahead and other OLDI
>> + * properties.
>> + */
>> + ret = drm_of_find_panel_or_bridge(child, OLDI_OURPUT_PORT, -1,
>> + &panel, &bridge);
>> + if (ret) {
>> + /*
>> + * Either there was no OLDI sink in the devicetree, or
>> + * the OLDI sink has not been added yet. In any case,
>> + * return.
>> + * We don't want to have an OLDI node connected to DSS
>> + * but not to any sink.
>> + */
>> + if (ret != -EPROBE_DEFER)
>> + dev_err(tidss->dev,
>> + "no panel/bridge for OLDI%d. Error %d\n",
>> + oldi_instance, ret);
>> + goto err_put_node;
>> + }
>> +
>> + if (panel) {
>> + bridge = devm_drm_panel_bridge_add(tidss->dev, panel);
>> + if (IS_ERR(bridge)) {
>> + ret = PTR_ERR(bridge);
>> + goto err_put_node;
>> + }
>> + }
>> +
>> + link_type = get_oldi_mode(child);
>> + if (link_type == OLDI_MODE_UNSUPPORTED) {
>> + dev_err(tidss->dev,
>> + "OLDI%d: Unsupported OLDI connection.\n",
>> + oldi_instance);
>> + ret = -EINVAL;
>> + goto err_put_node;
>> + } else if (link_type == OLDI_MODE_SECONDARY) {
>> + /*
>> + * This is the secondary OLDI node, which serves as a
>> + * companinon to the primary OLDI, when it is configured
>> + * for the dual-lvds mode. Since the primary OLDI will
>> + * be a part of bridge chain, no need to put this one
>> + * too. Continue onto the next OLDI node.
>> + */
>> + continue;
>> + }
>> +
>> + oldi = devm_kzalloc(tidss->dev, sizeof(*oldi), GFP_KERNEL);
>> + if (!oldi) {
>> + ret = -ENOMEM;
>> + goto err_put_node;
>> + }
>> +
>> + oldi->link_type = link_type;
>> + oldi->oldi_instance = oldi_instance;
>> + oldi->dev = tidss->dev;
>> + oldi->next_bridge = bridge;
>> + oldi->panel = panel;
>> +
>> + oldi->io_ctrl = syscon_regmap_lookup_by_phandle(child,
>> + "ti,oldi-io-ctrl");
>> + if (IS_ERR(oldi->io_ctrl)) {
>> + dev_err(oldi->dev,
>> + "%s: oldi%d syscon_regmap_lookup_by_phandle failed
>> %ld\n",
>> + __func__, oldi_instance, PTR_ERR(oldi->io_ctrl));
>> + ret = PTR_ERR(oldi->io_ctrl);
>> + goto err_put_node;
>> + }
>> +
>> + oldi->s_clk = of_clk_get_by_name(child, "s_clk");
>> + if (IS_ERR(oldi->s_clk)) {
>> + dev_err(oldi->dev,
>> + "%s: oldi%d Failed to get s_clk: %ld\n",
>> + __func__, oldi_instance, PTR_ERR(oldi->s_clk));
>> + ret = PTR_ERR(oldi->s_clk);
>> + goto err_put_node;
>> + }
>> +
>> + /* Register the bridge. */
>> + oldi->bridge.of_node = child;
>> + oldi->bridge.driver_private = oldi;
>> + oldi->bridge.funcs = &tidss_oldi_bridge_funcs;
>> + oldi->bridge.timings = &default_tidss_oldi_timings;
>> +
>> + tidss->oldis[tidss->num_oldis++] = oldi;
>> + oldi->tidss = tidss;
>> +
>> + drm_bridge_add(&oldi->bridge);
>> + }
>> +
>> +err_put_node:
>> + of_node_put(child);
>> + of_node_put(oldi_parent);
>> + return ret;
>
> I usually have the non-error code path at the end of the main function
> code, with "return 0", and the error cases separately below. Reading the
> above, it makes me think that if we reach the end of the
> for_each_child_of_node block, we're in an error state as we pass the
> err_put_node...
>
Alright! I will put the nodes and return 0 in case everything is
successful.
>> +}
>> diff --git a/drivers/gpu/drm/tidss/tidss_oldi.h
>> b/drivers/gpu/drm/tidss/tidss_oldi.h
>> new file mode 100644
>> index 000000000000..5ad02ddea11a
>> --- /dev/null
>> +++ b/drivers/gpu/drm/tidss/tidss_oldi.h
>> @@ -0,0 +1,73 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2023 - Texas Instruments Incorporated
>> + *
>> + * Aradhya Bhatia <a-bhati1@...com>
>> + */
>> +
>> +#ifndef __TIDSS_OLDI_H__
>> +#define __TIDSS_OLDI_H__
>> +
>> +#include <linux/media-bus-format.h>
>> +
>> +#include "tidss_drv.h"
>> +#include "tidss_dispc.h"
>> +
>> +struct tidss_oldi;
>> +
>> +/* OLDI Instances */
>> +#define OLDI(n) n
>
> This is an odd macro... At least the n should be inside parenthesis,
> but... Isn't a plain number just fine, why do you need a macro?
Just for better readability.
(oldi_instance == 0) OR (oldi_instance == 1) might have confused people.
However, the only place I needed to check oldi_instance values was TX
power enable/disable. I have decided to refactor the logic there
slightly and that won't require the checks anymore. I can drop this
macro now.
>
>> +
>> +/* OLDI PORTS */
>> +#define OLDI_INPUT_PORT 0
>> +#define OLDI_OURPUT_PORT 1
>> +
>> +/* OLDI Config Bits */
>> +#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)
>
> These are bits of the DISPC_VP_DSS_OLDI_CFG register, so it would be
> nicer to have these in the tidss_dispc_regs.h.
Okay!
>
>> +
>> +/* Control MMR Register */
>> +
>> +/* Register offsets */
>> +#define OLDI_PD_CTRL 0x100
>> +#define OLDI_LB_CTRL 0x104
>> +
>> +/* Power control bits */
>> +#define OLDI_PWRDN_TX(n) BIT(n)
>> +
>> +/* LVDS Bandgap reference Enable/Disable */
>> +#define OLDI_PWRDN_BG BIT(8)
>> +
>> +#define OLDI_IDLE_CLK_HZ 25000000 /*25 MHz */
>> +
>> +enum tidss_oldi_link_type {
>> + OLDI_MODE_UNSUPPORTED,
>> + OLDI_MODE_SINGLE_LINK,
>> + OLDI_MODE_CLONE_SINGLE_LINK,
>> + OLDI_MODE_DUAL_LINK,
>> + OLDI_MODE_SECONDARY,
>> +};
>> +
>> +enum oldi_mode_reg_val { SPWG_18 = 0, JEIDA_24 = 1, SPWG_24 = 2 };
>> +
>> +struct oldi_bus_format {
>> + u32 bus_fmt;
>> + u32 data_width;
>> + enum oldi_mode_reg_val oldi_mode_reg_val;
>> + u32 input_bus_fmt;
>> +};
>> +
>> +int tidss_oldi_init(struct tidss_device *tidss);
>> +void tidss_oldi_deinit(struct tidss_device *tidss);
>> +
>> +#endif /* __TIDSS_OLDI_H__ */
>
Regards
Aradhya
Powered by blists - more mailing lists