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

Tue May 25 14:23:43 UTC 2021

Comments inline.

(Without this top-post, the company mail server will inject a signature 
in some weird location.)

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 at topicproducts.com
W: www.topic.nl

Please consider the environment before printing this e-mail
On 25-05-2021 15:00, Marek Vasut wrote:
> On 5/25/21 2:08 PM, Mike Looijmans wrote:
>> 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.
>
> Ah, OK, it seems this bridge is popular on RPi.
> Is there some adapter board with such a bridge for RPi available ?

Nope, but about 4 subscribers on the RPi forum have created their own 
PCB. I'm working for a company that did their own PCB too and my job for 
them is to get it to work...

The DSI-to-LVDS bridge is a lot cheaper (and simpler) than a 
HDMI-to-LVDS bridge. In hardware that is.

>
>>> [...]
>>>
>>>>> +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.
>
> I think someone from the RPi foundation mentioned this before.

Yeah, he mentioned mentioning this on the RPi forum too.

>
>> 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.
>
> Why , please elaborate ? It seems to be if there was no need for those 
> two callbacks, there would be no two callbacks in the API in the first 
> place. There is a chance you will get disable()->enable() sequence 
> without going through post_disable()->pre_enable() as far as I can tell.

The datasheet states that "the DSI CLK lanes MUST be in HS state and the 
DSI data lanes MUST be driven to LP11 state" when the reset de-asserts 
(goes high) and when the CSR registers are being written.

Your driver now de-asserts the reset in the pre_enable and writes the 
CSR registers in enable. This is the "least likely to work" option.

Both the reset and the CSR writing are to be done in the same context. 
So either everything in "pre_enable", or everything in "enable". Which 
one is correct is up to the maintainers, I also don't know which is 
best. The other callback can just remain unused.

If the choice is to do the chip initialization in "pre_enable" then do 
all the de-initialization in "post_disable". If the choice is to do the 
chip initialization in "enable" then do all the de-initialization in 
"disable".

If for some platform the choice happens to be wrong, it's a very simple 
patch (just change the "ops" pointers) to change it and make it work for 
that platform.

Alternatively, it's possible to make it a runtime choice through 
devicetree or so as to whether the CSR initializes at "enable" or 
"pre-enable".

>
>> 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.
>
> That's how the RPi behaves, on MX8M the DSI clock are active only in 
> enable. But that might be wrong, see below.
>
>> I don't know how the platform you're testing on is behaving in this 
>> respect?
>
> iMX8M{M,N}.
>
> And I suspect the DSI behaves differently than on RPi. And that is why 
> I would like to get some clarification on what (clock, data, LP and 
> HS) is enabled where from the maintainers.

Suspect so. As I wrote before, any DSI that adheres to the documentation 
would never work with this chip. The chip won't work without clock and 
it also won't work if the DSI is already sending video data is my 
experience.

>
>>> [...]
>>>
>>>>> +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.
>
> It still does trigger a write into the hardware.

And in hardware it does absolutely nothing. It says so in the datasheet. 
And even so, the chip's reset has just been asserted, so you can be 
pretty sure it's in a well-defined state already.

>
>>>>> +    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)
>
> That just adds unnecessary indirection and makes the code harder to 
> read, so I won't do that. val = cpu_to_le16(...) looks good enough and 
> there are already such sequences, i.e. val = ... ; 
> regmap_bulk_write(...);

I'd think the indirection would make it easier to read the code, but 
it's up to you really.

__le16 val = cpu_to_le16(ctx->mode.hdisplay);
regmap_bulk_write(ctx->regmap, REG_VID_CHA_ACTIVE_LINE_LENGTH_LOW, &val, 
sizeof(val));

versus

sn65dsi83_write16bit(ctx, REG_VID_CHA_ACTIVE_LINE_LENGTH_LOW, 
ctx->mode.hdisplay);

-- 
Mike Looijmans