Re: [PATCH 4/4] drm/tidss: Add OLDI bridge support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxx>
>> ---
>> 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@xxxxxx/
>>
>> ---
>>   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@xxxxxx>
>> + */
>> +
>> +#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@xxxxxx>
>> + */
>> +
>> +#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



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux