| 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: <519cc025-0782-4f96-a169-1fe87b280173@norik.com>
Date: Wed, 11 Dec 2024 08:57:17 +0100
From: Andrej Picej <andrej.picej@...ik.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc: andrzej.hajda@...el.com, neil.armstrong@...aro.org, rfoss@...nel.org,
Laurent.pinchart@...asonboard.com, jonas@...boo.se,
jernej.skrabec@...il.com, airlied@...il.com, simona@...ll.ch,
maarten.lankhorst@...ux.intel.com, mripard@...nel.org, tzimmermann@...e.de,
robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
shawnguo@...nel.org, s.hauer@...gutronix.de, kernel@...gutronix.de,
festevam@...il.com, marex@...x.de, dri-devel@...ts.freedesktop.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
upstream@...ts.phytec.de
Subject: Re: [PATCH v5 2/3] drm/bridge: ti-sn65dsi83: Add ti,lvds-vod-swing
optional properties
On 10. 12. 24 14:59, Dmitry Baryshkov wrote:
> On Tue, Dec 10, 2024 at 02:41:01PM +0100, Andrej Picej wrote:
>>
>>
>> On 10. 12. 24 12:43, Dmitry Baryshkov wrote:
>>> On Tue, Dec 10, 2024 at 10:19:00AM +0100, Andrej Picej wrote:
>>>> Add a optional properties to change LVDS output voltage. This should not
>>>> be static as this depends mainly on the connected display voltage
>>>> requirement. We have three properties:
>>>> - "ti,lvds-termination-ohms", which sets near end termination,
>>>> - "ti,lvds-vod-swing-data-microvolt" and
>>>> - "ti,lvds-vod-swing-clock-microvolt" which both set LVDS differential
>>>> output voltage for data and clock lanes. They are defined as an array
>>>> with min and max values. The appropriate bitfield will be set if
>>>> selected constraints can be met.
>>>>
>>>> If "ti,lvds-termination-ohms" is not defined the default of 200 Ohm near
>>>> end termination will be used. Selecting only one:
>>>> "ti,lvds-vod-swing-data-microvolt" or
>>>> "ti,lvds-vod-swing-clock-microvolt" can be done, but the output voltage
>>>> constraint for only data/clock lanes will be met. Setting both is
>>>> recommended.
>>>>
>>>> Signed-off-by: Andrej Picej <andrej.picej@...ik.com>
>>>> ---
>>>> Changes in v5:
>>>> - specify default values in sn65dsi83_parse_lvds_endpoint,
>>>> - move sn65dsi83_parse_lvds_endpoint for channel B up, outside if,
>>>> Changes in v4:
>>>> - fix typo in commit message bitfiled -> bitfield
>>>> - use arrays (lvds_vod_swing_conf and lvds_term_conf) in private data, instead
>>>> of separate variables for channel A/B
>>>> - add more checks on return value of "of_property_read_u32_array"
>>>> Changes in v3:
>>>> - use microvolts for default array values 1000 mV -> 1000000 uV.
>>>> Changes in v2:
>>>> - use datasheet tables to get the proper configuration
>>>> - since major change was done change the authorship to myself
>>>> ---
>>>> drivers/gpu/drm/bridge/ti-sn65dsi83.c | 142 +++++++++++++++++++++++++-
>>>> 1 file changed, 139 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>>>> index 57a7ed13f996..f9578b38da28 100644
>>>> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>>>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>>>> @@ -132,6 +132,16 @@
>>>> #define REG_IRQ_STAT_CHA_SOT_BIT_ERR BIT(2)
>>>> #define REG_IRQ_STAT_CHA_PLL_UNLOCK BIT(0)
>>>> +enum sn65dsi83_channel {
>>>> + CHANNEL_A,
>>>> + CHANNEL_B
>>>> +};
>>>> +
>>>> +enum sn65dsi83_lvds_term {
>>>> + OHM_100,
>>>> + OHM_200
>>>> +};
>>>> +
>>>> enum sn65dsi83_model {
>>>> MODEL_SN65DSI83,
>>>> MODEL_SN65DSI84,
>>>> @@ -147,6 +157,8 @@ struct sn65dsi83 {
>>>> struct regulator *vcc;
>>>> bool lvds_dual_link;
>>>> bool lvds_dual_link_even_odd_swap;
>>>> + int lvds_vod_swing_conf[2];
>>>> + int lvds_term_conf[2];
>>>> };
>>>> static const struct regmap_range sn65dsi83_readable_ranges[] = {
>>>> @@ -237,6 +249,36 @@ static const struct regmap_config sn65dsi83_regmap_config = {
>>>> .max_register = REG_IRQ_STAT,
>>>> };
>>>> +static const int lvds_vod_swing_data_table[2][4][2] = {
>>>> + { /* 100 Ohm */
>>>> + { 180000, 313000 },
>>>> + { 215000, 372000 },
>>>> + { 250000, 430000 },
>>>> + { 290000, 488000 },
>>>> + },
>>>> + { /* 200 Ohm */
>>>> + { 150000, 261000 },
>>>> + { 200000, 346000 },
>>>> + { 250000, 428000 },
>>>> + { 300000, 511000 },
>>>> + },
>>>> +};
>>>> +
>>>> +static const int lvds_vod_swing_clock_table[2][4][2] = {
>>>> + { /* 100 Ohm */
>>>> + { 140000, 244000 },
>>>> + { 168000, 290000 },
>>>> + { 195000, 335000 },
>>>> + { 226000, 381000 },
>>>> + },
>>>> + { /* 200 Ohm */
>>>> + { 117000, 204000 },
>>>> + { 156000, 270000 },
>>>> + { 195000, 334000 },
>>>> + { 234000, 399000 },
>>>> + },
>>>> +};
>>>> +
>>>> static struct sn65dsi83 *bridge_to_sn65dsi83(struct drm_bridge *bridge)
>>>> {
>>>> return container_of(bridge, struct sn65dsi83, bridge);
>>>> @@ -435,12 +477,16 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
>>>> 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_VCOM,
>>>> + REG_LVDS_VCOM_CHA_LVDS_VOD_SWING(ctx->lvds_vod_swing_conf[CHANNEL_A]) |
>>>> + REG_LVDS_VCOM_CHB_LVDS_VOD_SWING(ctx->lvds_vod_swing_conf[CHANNEL_B]));
>>>> 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);
>>>> + (ctx->lvds_term_conf[CHANNEL_A] ?
>>>> + REG_LVDS_LANE_CHA_LVDS_TERM : 0) |
>>>> + (ctx->lvds_term_conf[CHANNEL_B] ?
>>>> + REG_LVDS_LANE_CHB_LVDS_TERM : 0));
>>>> regmap_write(ctx->regmap, REG_LVDS_CM, 0x00);
>>>> le16val = cpu_to_le16(mode->hdisplay);
>>>> @@ -576,10 +622,100 @@ static const struct drm_bridge_funcs sn65dsi83_funcs = {
>>>> .atomic_get_input_bus_fmts = sn65dsi83_atomic_get_input_bus_fmts,
>>>> };
>>>> +static int sn65dsi83_select_lvds_vod_swing(struct device *dev,
>>>> + u32 lvds_vod_swing_data[2], u32 lvds_vod_swing_clk[2], u8 lvds_term)
>>>> +{
>>>> + int i;
>>>> +
>>>> + for (i = 0; i <= 3; i++) {
>>>> + if (lvds_vod_swing_data_table[lvds_term][i][0] >= lvds_vod_swing_data[0] &&
>>>> + lvds_vod_swing_data_table[lvds_term][i][1] <= lvds_vod_swing_data[1] &&
>>>> + lvds_vod_swing_clock_table[lvds_term][i][0] >= lvds_vod_swing_clk[0] &&
>>>> + lvds_vod_swing_clock_table[lvds_term][i][1] <= lvds_vod_swing_clk[1])
>>>> + return i;
>>>> + }
>>>> +
>>>> + dev_err(dev, "failed to find appropriate LVDS_VOD_SWING configuration\n");
>>>> + return -EINVAL;
>>>> +}
>>>> +
>>>> +static int sn65dsi83_parse_lvds_endpoint(struct sn65dsi83 *ctx, int channel)
>>>> +{
>>>> + struct device *dev = ctx->dev;
>>>> + struct device_node *endpoint;
>>>> + int endpoint_reg;
>>>> + /* Set so the property can be freely selected if not defined */
>>>> + u32 lvds_vod_swing_data[2] = { 0, 1000000 };
>>>> + u32 lvds_vod_swing_clk[2] = { 0, 1000000 };
>>>> + u32 lvds_term;
>>>> + u8 lvds_term_conf = 0x1;
>>>> + int lvds_vod_swing_conf = 0x1;
>>>
>>> Magic values
>>
>> Can you please elaborate.
>>
>> I can use:
>> u8 lvds_term_conf = OHM_200;
>>
>> What about lvds_vod_swing_conf? Should I create additional define for it?
>> But this doesn't solve a hidden meaning? Maybe additional comment above?
>> Would like to avoid using voltages for it, since then we are reverse
>> engineering the table in datasheet to match the default reg value.
>
> I think the following example solves both problems:
>
> lvds_term = 200;
> of_property_read_u32(..., &lvds_term);
>
> if (lvds_term == 100)
> ctx->lvds_term_conf[channel] = OHM_100;
> else if (lvds_term == 200)
> ctx->lvds_term_conf[channel] = OHM_200;
> else
> return -EINVAL;
>
> The same approach can be applied to lvds_vod_swing_conf, resulting in
> removal of magic values.
Sorry, but I think it is not that easy when it comes to the
lvds_vod_swing_conf. We should assign default value if
"ti,lvds-vod-swing-data-microvolt" and
"ti,lvds-vod-swing-clock-microvolt" are not defined. Default value of
the lvds_vod_swing_conf is 0x1, but this doesn't have any straight
forward meaning like OHM_200 for example.
What we can do in that case is that we copy the values from defined
datasheet tables to the "lvds_vod_swing_data[2]" and
"lvds_vod_swing_clk[2]" arrays and then run the
sn65dsi83_select_lvds_vod_swing with it, which will return the default
value (0x1).
/* If both properties are not defined assign default limits */
if (ret_data && ret_clock) {
memcpy(lvds_vod_swing_data,
lvds_vod_swing_data_table[ctx->lvds_term_conf[channel]][1],
sizeof(lvds_vod_swing_data));
memcpy(lvds_vod_swing_clk,
lvds_vod_swing_clock_table[ctx->lvds_term_conf[channel]][1],
sizeof(lvds_vod_swing_clk));
}
lvds_vod_swing_conf = sn65dsi83_select_lvds_vod_swing(dev,
lvds_vod_swing_data, lvds_vod_swing_clk,
ctx->lvds_term_conf[channel]);
if (lvds_vod_swing_conf < 0) {
ret = lvds_vod_swing_conf;
goto exit;
}
ctx->lvds_vod_swing_conf[channel] = lvds_vod_swing_conf;
I'm not sure if using this approach gets rid of the problem with magic
values.
Or maybe I'm not seeing the obvious solution so please bear with me.
>
>>
>>>
>>>> + int ret = 0;
>>>> + int ret_data;
>>>> + int ret_clock;
>>>> +
>>>> + if (channel == CHANNEL_A)
>>>> + endpoint_reg = 2;
>>>> + else
>>>> + endpoint_reg = 3;
>>>> +
>>>> + endpoint = of_graph_get_endpoint_by_regs(dev->of_node, endpoint_reg, -1);
>>>> + if (!of_property_read_u32(endpoint, "ti,lvds-termination-ohms", &lvds_term)) {
>>>
>>> The code has been better before:
>>> provide default for lvds_term, read the property (keeping the default in
>>> case of an error), then use the lvds_term to set up lvds_term_conf, as
>>> expected.
>>
>> Ok, will revert back.
>>
>>>
>>>> + if (lvds_term == 100)
>>>> + lvds_term_conf = OHM_100;
>>>> + else
>>>> + lvds_term_conf = OHM_200;
>>>> + }
>>>> +
>>>> + ctx->lvds_term_conf[channel] = lvds_term_conf;
>>>> +
>>>> + ret_data = of_property_read_u32_array(endpoint,
>>>> + "ti,lvds-vod-swing-data-microvolt", lvds_vod_swing_data,
>>>> + ARRAY_SIZE(lvds_vod_swing_data));
>>>> + if (ret_data != 0 && ret_data != -EINVAL) {
>>>> + ret = ret_data;
>>>> + goto exit;
>>>> + }
>>>> +
>>>> + ret_clock = of_property_read_u32_array(endpoint,
>>>> + "ti,lvds-vod-swing-clock-microvolt", lvds_vod_swing_clk,
>>>> + ARRAY_SIZE(lvds_vod_swing_clk));
>>>> + if (ret_clock != 0 && ret_clock != -EINVAL) {
>>>> + ret = ret_clock;
>>>> + goto exit;
>>>> + }
>>>> +
>>>> + /* If any of the two properties is defined. */
>>>> + if (!ret_data || !ret_clock) {
>>>> + lvds_vod_swing_conf = sn65dsi83_select_lvds_vod_swing(dev,
>>>> + lvds_vod_swing_data, lvds_vod_swing_clk,
>>>> + lvds_term_conf);
>>>> + if (lvds_vod_swing_conf < 0) {
>>>> + ret = lvds_vod_swing_conf;
>>>> + goto exit;
>>>> + }
>>>> + }
>>>> +
>>>> + ctx->lvds_vod_swing_conf[channel] = lvds_vod_swing_conf;
>>>> + ret = 0;
>>>> +exit:
>>>> + of_node_put(endpoint);
>>>> + return ret;
>>>> +}
>>>> +
>>>> static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model)
>>>> {
>>>> struct drm_bridge *panel_bridge;
>>>> struct device *dev = ctx->dev;
>>>> + int ret;
>>>> +
>>>> + ret = sn65dsi83_parse_lvds_endpoint(ctx, CHANNEL_A);
>>>> + if (ret < 0)
>>>> + return ret;
>>>> +
>>>> + ret = sn65dsi83_parse_lvds_endpoint(ctx, CHANNEL_B);
>>>> + if (ret < 0)
>>>> + return ret;
>>>> ctx->lvds_dual_link = false;
>>>> ctx->lvds_dual_link_even_odd_swap = false;
>>>> --
>>>> 2.34.1
>>>>
>>>
>
Powered by blists - more mailing lists