On 5/6/21 7:03 PM, Dave Stevenson wrote:
On Thu, 6 May 2021 at 13:48, Marek Vasut <marex@xxxxxxx> wrote:
On 5/6/21 11:45 AM, Dave Stevenson wrote:
Hi Marek
Hi,
I'm taking an interest as there are a number of Raspberry Pi users
trying to get this chip up and running (not there quite yet).
A couple of fairly minor comments
Is there any readily available display unit / expansion board with this
chip ?
Not that I'm aware of. It's a forum thread[1] where two different
users are trying to bring up the chip, each with their own boards. One
has just reported they have got it working with Jagan's patch set but
with a load of hacks to both bridge and DSI drivers.
If Laurent has a board then that may be a useful further test route.
I'm not 100% convinced that the Pi is doing exactly the right thing
with regard LP-11 state during initialisation, but hope to get into
the lab to check either tomorrow or early next week.
Good
[1] https://www.raspberrypi.org/forums/viewtopic.php?f=44&t=305690
[...]
+#define REG_DSI_LANE 0x10
+#define REG_DSI_LANE_LVDS_LINK_CFG_DUAL BIT(5) /* dual or 2x single */
The bit name here seems a little odd.
Bits 5&6 are the DSI channel mode on SN65DSI85, not the LVDS link
config (which appears to be reg 0x18 bit 4)
DSI_CHANNEL_MODE
00 – Dual-channel DSI receiver
01 – Single channel DSI receiver (default)
10 – Two single channel DSI receivers
11 – Reserved
SN65DSI83 and 84 require it to be set to 01. You have that end result,
but using an odd register name that only documents one of the 2 bits.
Is it worth documenting bit 7 as being the '85 Dual DSI link
LEFT_RIGHT_PIXELS flag at the same time? The chip isn't supported in
dual DSI mode at present, but defining all the registers up front
seems reasonable.
Yes, let's fix those.
[...]
+ /*
+ * 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);
+}
Whilst section 6.6 of the SN65DSI84 datasheet does list t_en as 1ms,
the initialization sequence listed in table 7.2 states
Init seq 3 - Set EN pin to Low
Wait 10 ms
Init seq 4 - Tie EN pin to High
Wait 10 ms
with the note that these are "Minimum recommended delay. It is fine to
exceed these."
Have you had alternate guidance from TI over that delay?
No, I trusted the timing diagrams in section 6.6 of the datasheet. I
would like to believe those are correct, while the init sequence listing
might be a copy-paste error.
It's a tough one to call as to which is going to be correct. I just
thought I'd flag it.
[...]
+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);
+ 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);
(Checkpatch whinge for "Alignment should match open parenthesis" on
several lines through this function)
Do you have any extra checkpatch settings somewhere ?
I get this on current next:
linux-2.6$ ./scripts/checkpatch.pl -f drivers/gpu/drm/bridge/ti-sn65dsi83.c
total: 0 errors, 0 warnings, 625 lines checked
Sorry, yes "./scripts/checkpatch.pl --strict --codespell". Too much
working in the linux-media realms where --strict is required :-/
So I can add a variable , assign it the value of
sn65dsi83_get_lvds_range(ctx) and then do
REG_RC_LVDS_PLL_LVDS_CLK_RANGE(val), but that doesn't really improve the
readability at all, it just adds extra indirection.
