Re: [V3, 2/2] drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 and SN65DSI84 driver

[Mike Looijmans <mike.looijmans@topic.nl>]

See below...


Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topicproducts.com
W: www.topic.nl

Please consider the environment before printing this e-mail
On 25-05-2021 12:53, Marek Vasut wrote:
> On 5/17/21 3:23 PM, Mike Looijmans wrote:
>
> Which system/soc are you testing this on ?

On a raspberry-pi 4 at the moment.

>
> [...]
>
>>> +static void sn65dsi83_pre_enable(struct drm_bridge *bridge)
>>> +{
>>> +    struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
>>> +
>>> +    /*
>>> +     * Reset the chip, pull EN line low for t_reset=10ms,
>>> +     * then high for t_en=1ms.
>>> +     */
>>> +    regcache_mark_dirty(ctx->regmap);
>>> +    gpiod_set_value(ctx->enable_gpio, 0);
>>> +    usleep_range(10000, 11000);
>>> +    gpiod_set_value(ctx->enable_gpio, 1);
>>> +    usleep_range(1000, 1100);
>> Taking the chip out of reset here is not needed and may even disrupt 
>> things, as the DSI hasn't set up anything yet so the clock may not be 
>> running. I'd move this all into enable and get rid of the pre_enable 
>> call. Similar for post_disable.
>
> I am still waiting for someone to confirm the behavior of the DSI 
> clock and data lanes in pre_enable/enable() . The datasheet says the 
> HS clock have to be running and data lanes must be in LP state during 
> init, but I don't think that is guaranteed currently in either 
> pre_enable or enable.

Neither is suited. pre-enable may have nothing, while enable has the 
data lanes sending video according to the docs. So on many systems 
either this driver or the DSI driver will need changes to make it work 
properly.

On the raspberrypi, the DSI has the clock running in pre-enable, hence 
putting everything in pre-enable instead of in enable makes the chip work.

Alternatively, one can modify the RPi DSI code to start sending data 
after the enable calls. That also works on my setup, with everything in 
enable.

The major point here is that you should pick one and only one callback: 
pre-enable or enable. The GPIO reset code as well as writing the 
registers should be done in that one method.

Same for (post)disable for symmetry. There's no point keeping the chip 
awake after a disable.


It's pretty likely for a DSI driver to have the clock active in order to 
allow the panel driver to send MIPI commands and things like that. So 
everything in pre_enable makes sense.

I don't know how the platform you're testing on is behaving in this respect?


>
> [...]
>
>>> +static void sn65dsi83_enable(struct drm_bridge *bridge)
>>> +{
>>> +    struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
>>> +    unsigned int pval;
>>> +    u16 val;
>>> +    int ret;
>>> +
>>> +    /* Clear reset, disable PLL */
>>> +    regmap_write(ctx->regmap, REG_RC_RESET, 0x00);
>> Writing 0 to the RESET register is a no-op. Has no effect whatsoever, 
>> just wasting time and code.
>
> I would rather keep it to make sure the register is initialized.

Why? It's marked "volatile" so the regmap cache will not touch it. On 
the I2C level, there's absolutely no reason to do this.

>
>>> +    regmap_write(ctx->regmap, REG_RC_PLL_EN, 0x00);
>>> +
>>> +    /* Reference clock derived from DSI link clock. */
>>> +    regmap_write(ctx->regmap, REG_RC_LVDS_PLL,
>>> + REG_RC_LVDS_PLL_LVDS_CLK_RANGE(sn65dsi83_get_lvds_range(ctx)) |
>>> +        REG_RC_LVDS_PLL_HS_CLK_SRC_DPHY);
>>> +    regmap_write(ctx->regmap, REG_DSI_CLK,
>>> + REG_DSI_CLK_CHA_DSI_CLK_RANGE(sn65dsi83_get_dsi_range(ctx)));
>>> +    regmap_write(ctx->regmap, REG_RC_DSI_CLK,
>>> + REG_RC_DSI_CLK_DSI_CLK_DIVIDER(sn65dsi83_get_dsi_div(ctx)));
>>> +
>>> +    /* Set number of DSI lanes and LVDS link config. */
>>> +    regmap_write(ctx->regmap, REG_DSI_LANE,
>>> +        REG_DSI_LANE_LVDS_LINK_CFG_DUAL |
>>> +        REG_DSI_LANE_CHA_DSI_LANES(~(ctx->dsi_lanes - 1)) |
>>> +        /* CHB is DSI85-only, set to default on DSI83/DSI84 */
>>> +        REG_DSI_LANE_CHB_DSI_LANES(3));
>>> +    /* No equalization. */
>>> +    regmap_write(ctx->regmap, REG_DSI_EQ, 0x00);
>>> +
>>> +    /* RGB888 is the only format supported so far. */
>>> +    val = (ctx->mode.flags & DRM_MODE_FLAG_NHSYNC ?
>>> +           REG_LVDS_FMT_HS_NEG_POLARITY : 0) |
>>> +          (ctx->mode.flags & DRM_MODE_FLAG_NVSYNC ?
>>> +           REG_LVDS_FMT_VS_NEG_POLARITY : 0) |
>>> +          REG_LVDS_FMT_CHA_24BPP_MODE;
>>> +    if (ctx->lvds_dual_link)
>>> +        val |= REG_LVDS_FMT_CHB_24BPP_MODE;
>>> +    else
>>> +        val |= REG_LVDS_FMT_LVDS_LINK_CFG;
>>> +
>>> +    regmap_write(ctx->regmap, REG_LVDS_FMT, val);
>>> +    regmap_write(ctx->regmap, REG_LVDS_VCOM, 0x05);
>>> +    regmap_write(ctx->regmap, REG_LVDS_LANE,
>>> +        (ctx->lvds_dual_link_even_odd_swap ?
>>> +         REG_LVDS_LANE_EVEN_ODD_SWAP : 0) |
>>> +        REG_LVDS_LANE_CHA_LVDS_TERM |
>>> +        REG_LVDS_LANE_CHB_LVDS_TERM);
>>> +    regmap_write(ctx->regmap, REG_LVDS_CM, 0x00);
>>> +
>>> +    regmap_bulk_write(ctx->regmap, REG_VID_CHA_ACTIVE_LINE_LENGTH_LOW,
>>> +              &ctx->mode.hdisplay, 2);
>>
>> I think this ignores the endian format of the data. So this would 
>> only work on little-endian systems, right?
>
> Likely, can you double-check that ?
> Some cpu_to_le16() could help here then.

I'd add a small helper function that does the endian conversion and 
register write, e.g.

static int sn65dsi83_write16bit(struct sn65dsi83 *ctx, unsigned int reg, 
u16 value)


>>> +    regmap_bulk_write(ctx->regmap, 
>>> REG_VID_CHA_VERTICAL_DISPLAY_SIZE_LOW,
>>> +              &ctx->mode.vdisplay, 2);
>>> +    val = 32 + 1;    /* 32 + 1 pixel clock to ensure proper 
>>> operation */
>>> +    regmap_bulk_write(ctx->regmap, REG_VID_CHA_SYNC_DELAY_LOW, 
>>> &val, 2);
>>> +    val = ctx->mode.hsync_end - ctx->mode.hsync_start;
>
> [...]


-- 
Mike Looijmans