[PATCH v3 0/4] arm64: dts: imx8mm-kontron: Add HDMI and LVDS display support
This add support for the display bridges (DSI->LVDS and DSI->HDMI) on the BL i.MX8MM and the 7" LVDS panel in a separate overlay. Only one of the interfaces (HDMI or LVDS) is supported at the same time. Enabling the LVDS overlay will disable the HDMI interface. * Patch 1 and 2: Add the necessary binding changes * Patch 3: Extend the BL devicetree * Patch 4: Add the LVDS panel overlay Changes for v3: * Add A-b tag from Krzysztof * Fix LVDS bridge input port reference Changes for v2: * Patch 1: Add link to commit message * Patch 2: Add Conors A-b tag * Patch 3: Remove blank lines from hdmi node * Patch 3: Fix order of lvds and hdmi nodes within i2c * Patch 3: Remove the unneeded deletion of samsung,pll-clock-frequency * Patch 3: Use the existing MIPI DSI output port from imx8mm.dtsi * Patch 4: Update copyright year * Patch 4: Use exisitng MIPI DSI output port from imx8mm.dtsi * Patch 4: Fix pinctrl for GPIO hogs * Patch 4: Fix property order in i2c2 node * Patch 4: Use generic node name for touchscreen Frieder Schrempf (4): dt-bindings: vendor-prefixes: Add Jenson Display dt-bindings: display: panel-lvds: Add compatible for Jenson BL-JT60050-01A arm64: dts: imx8mm-kontron: Add support for display bridges on BL i.MX8MM arm64: dts: imx8mm-kontron: Add DL (Display-Line) overlay with LVDS support .../bindings/display/panel/panel-lvds.yaml| 2 + .../devicetree/bindings/vendor-prefixes.yaml | 2 + arch/arm64/boot/dts/freescale/Makefile| 4 + .../boot/dts/freescale/imx8mm-kontron-bl.dts | 131 .../boot/dts/freescale/imx8mm-kontron-dl.dtso | 189 ++ 5 files changed, 328 insertions(+) create mode 100644 arch/arm64/boot/dts/freescale/imx8mm-kontron-dl.dtso -- 2.46.0
[PATCH v3 2/4] dt-bindings: display: panel-lvds: Add compatible for Jenson BL-JT60050-01A
From: Frieder Schrempf The Jenson BL-JT60050-01A is a 7" 1024x600 LVDS display. Signed-off-by: Frieder Schrempf Acked-by: Conor Dooley --- Changes for v3: * none Changes for v2: * Add tag from Conor (thanks!) --- Documentation/devicetree/bindings/display/panel/panel-lvds.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/display/panel/panel-lvds.yaml b/Documentation/devicetree/bindings/display/panel/panel-lvds.yaml index 155d8ffa8f6ef..5af2d69300751 100644 --- a/Documentation/devicetree/bindings/display/panel/panel-lvds.yaml +++ b/Documentation/devicetree/bindings/display/panel/panel-lvds.yaml @@ -50,6 +50,8 @@ properties: - hannstar,hsd101pww2 # Hydis Technologies 7" WXGA (800x1280) TFT LCD LVDS panel - hydis,hv070wx2-1e0 + # Jenson Display BL-JT60050-01A 7" WSVGA (1024x600) color TFT LCD LVDS panel + - jenson,bl-jt60050-01a - tbs,a711-panel - const: panel-lvds -- 2.46.0
Re: [PATCH v2 0/4] arm64: dts: imx8mm-kontron: Add HDMI and LVDS display support
On 08.10.24 11:04 AM, Shawn Guo wrote: > On Tue, Oct 08, 2024 at 09:21:05AM +0200, Frieder Schrempf wrote: >> On 28.08.24 9:46 AM, Frieder Schrempf wrote: >>> From: Frieder Schrempf >>> >>> This add support for the display bridges (DSI->LVDS and DSI->HDMI) >>> on the BL i.MX8MM and the 7" LVDS panel in a separate overlay. >>> >>> Only one of the interfaces (HDMI or LVDS) is supported at the same >>> time. Enabling the LVDS overlay will disable the HDMI interface. >>> >>> * Patch 1 and 2: Add the necessary binding changes >>> * Patch 3: Extend the BL devicetree >>> * Patch 4: Add the LVDS panel overlay >>> >>> Changes for v2: >>> * Patch 1: Add link to commit message >>> * Patch 2: Add Conors A-b tag >>> * Patch 3: Remove blank lines from hdmi node >>> * Patch 3: Fix order of lvds and hdmi nodes within i2c >>> * Patch 3: Remove the unneeded deletion of samsung,pll-clock-frequency >>> * Patch 3: Use the existing MIPI DSI output port from imx8mm.dtsi >>> * Patch 4: Update copyright year >>> * Patch 4: Use exisitng MIPI DSI output port from imx8mm.dtsi >>> * Patch 4: Fix pinctrl for GPIO hogs >>> * Patch 4: Fix property order in i2c2 node >>> * Patch 4: Use generic node name for touchscreen >>> >>> Frieder Schrempf (4): >>> dt-bindings: vendor-prefixes: Add Jenson Display >>> dt-bindings: display: panel-lvds: Add compatible for Jenson >>> BL-JT60050-01A >>> arm64: dts: imx8mm-kontron: Add support for display bridges on BL >>> i.MX8MM >>> arm64: dts: imx8mm-kontron: Add DL (Display-Line) overlay with LVDS >>> support >> >> Gentle ping for this series. Neil proposed to apply path 1 and 2 to >> drm-misc-next. Shawn, can you review/apply patch 3 and 4, please? > > I'm getting this: > > OVL arch/arm64/boot/dts/freescale/imx8mm-kontron-dl.dtb > Failed to apply 'arch/arm64/boot/dts/freescale/imx8mm-kontron-dl.dtbo': > FDT_ERR_NOTFOUND > make[4]: *** [../scripts/Makefile.dtbs:83: > arch/arm64/boot/dts/freescale/imx8mm-kontron-dl.dtb] Error 1 Thanks for noticing. It seems like I missed to commit something before preparing the patches. I just sent out a v3 that is fixed.
Re: [PATCH v2 0/4] arm64: dts: imx8mm-kontron: Add HDMI and LVDS display support
On 28.08.24 9:46 AM, Frieder Schrempf wrote: > From: Frieder Schrempf > > This add support for the display bridges (DSI->LVDS and DSI->HDMI) > on the BL i.MX8MM and the 7" LVDS panel in a separate overlay. > > Only one of the interfaces (HDMI or LVDS) is supported at the same > time. Enabling the LVDS overlay will disable the HDMI interface. > > * Patch 1 and 2: Add the necessary binding changes > * Patch 3: Extend the BL devicetree > * Patch 4: Add the LVDS panel overlay > > Changes for v2: > * Patch 1: Add link to commit message > * Patch 2: Add Conors A-b tag > * Patch 3: Remove blank lines from hdmi node > * Patch 3: Fix order of lvds and hdmi nodes within i2c > * Patch 3: Remove the unneeded deletion of samsung,pll-clock-frequency > * Patch 3: Use the existing MIPI DSI output port from imx8mm.dtsi > * Patch 4: Update copyright year > * Patch 4: Use exisitng MIPI DSI output port from imx8mm.dtsi > * Patch 4: Fix pinctrl for GPIO hogs > * Patch 4: Fix property order in i2c2 node > * Patch 4: Use generic node name for touchscreen > > Frieder Schrempf (4): > dt-bindings: vendor-prefixes: Add Jenson Display > dt-bindings: display: panel-lvds: Add compatible for Jenson > BL-JT60050-01A > arm64: dts: imx8mm-kontron: Add support for display bridges on BL > i.MX8MM > arm64: dts: imx8mm-kontron: Add DL (Display-Line) overlay with LVDS > support Gentle ping for this series. Neil proposed to apply path 1 and 2 to drm-misc-next. Shawn, can you review/apply patch 3 and 4, please? Thanks Frieder
Re: [PATCH v2 2/4] dt-bindings: display: panel-lvds: Add compatible for Jenson BL-JT60050-01A
On 13.09.24 11:18 AM, Neil Armstrong wrote: > Hi, > > On 28/08/2024 09:46, Frieder Schrempf wrote: >> From: Frieder Schrempf >> >> The Jenson BL-JT60050-01A is a 7" 1024x600 LVDS display. >> >> Signed-off-by: Frieder Schrempf >> Acked-by: Conor Dooley >> --- >> Changes for v2: >> * Add tag from Conor (thanks!) >> --- >> Documentation/devicetree/bindings/display/panel/panel-lvds.yaml | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/display/panel/panel- >> lvds.yaml b/Documentation/devicetree/bindings/display/panel/panel- >> lvds.yaml >> index 155d8ffa8f6ef..5af2d69300751 100644 >> --- a/Documentation/devicetree/bindings/display/panel/panel-lvds.yaml >> +++ b/Documentation/devicetree/bindings/display/panel/panel-lvds.yaml >> @@ -50,6 +50,8 @@ properties: >> - hannstar,hsd101pww2 >> # Hydis Technologies 7" WXGA (800x1280) TFT LCD LVDS panel >> - hydis,hv070wx2-1e0 >> + # Jenson Display BL-JT60050-01A 7" WSVGA (1024x600) color >> TFT LCD LVDS panel >> + - jenson,bl-jt60050-01a >> - tbs,a711-panel >> - const: panel-lvds > > How do you want to deal with that, I can apply both patches 1 & 2 to > drm-misc-next, is that ok ? I'm not sure who you are asking, but to me that's fine.
Re: i.MX8MP IMX-LCDIF Underrun Question(s)
On 09.09.24 5:00 AM, Peng Fan wrote: >> Subject: Re: i.MX8MP IMX-LCDIF Underrun Question(s) >> >> On 06.09.24 3:46 AM, Adam Ford wrote: >>> I have been testing various settings on the HDMI out of the i.MX8MP. >>> >>> I noticed that sometimes my monitor would not sync, but sometimes >> it >>> would on the same resolution/refresh rate. Frieder noted the LCDIF >>> was sometimes underflowing, so read up on it a little bit. >>> >>> In the comments of the LCDIF driver, it's noted: >>> Set FIFO Panic watermarks, low 1/3, high 2/3 . >>> >>> However, in the downstream kernels, NXP changes the threshold to >> 1/2 >>> and 3/4 on the LCDIF that drives the HDMI, while leaving the other >>> LCDIF interfaces at the default. >>> >>> When I increased the threshold to 1/2 and 3/4, it appeared that >>> several resolutions that my monitor was struggling to sync started >>> working, and it appeared to sync faster. I don't have an HDMI >>> analyzer, so I cannot verify much beyond knowing if my monitor can >> or >>> cannot sync. >> >> For me this change doesn't seem to cause any improved behavior. My >> monitor still fails to sync every few times I run "modetest -s" . >> >> Also we have a downstream kernel based on 6.1 with backported >> HDMI support and I don't see the issues there. But I need to make >> some further tests to make any reliable statements. >> > > Downstream kernel has some NOC settings that not supported > in upstream kernel, that maybe the issue. > > If you check 6.6 kernel, you could see some noc related settings > in imx8mp-blk-ctl.c imx8m-blk-ctl.c and gpcv2.c. You may give a > try with those noc settings applied and see whether that would > improve. With "downstream kernel" I didn't mean linux-imx. I meant our Kontron linux-ktn, which is based on mainline. I didn't test linux-imx to see if the issue occurs there, but that might be one task for further debugging.
Re: i.MX8MP IMX-LCDIF Underrun Question(s)
On 06.09.24 3:46 AM, Adam Ford wrote: > I have been testing various settings on the HDMI out of the i.MX8MP. > > I noticed that sometimes my monitor would not sync, but sometimes it > would on the same resolution/refresh rate. Frieder noted the LCDIF > was sometimes underflowing, so read up on it a little bit. > > In the comments of the LCDIF driver, it's noted: > Set FIFO Panic watermarks, low 1/3, high 2/3 . > > However, in the downstream kernels, NXP changes the threshold to 1/2 > and 3/4 on the LCDIF that drives the HDMI, while leaving the other > LCDIF interfaces at the default. > > When I increased the threshold to 1/2 and 3/4, it appeared that > several resolutions that my monitor was struggling to sync started > working, and it appeared to sync faster. I don't have an HDMI > analyzer, so I cannot verify much beyond knowing if my monitor can or > cannot sync. For me this change doesn't seem to cause any improved behavior. My monitor still fails to sync every few times I run "modetest -s" . Also we have a downstream kernel based on 6.1 with backported HDMI support and I don't see the issues there. But I need to make some further tests to make any reliable statements. > > Could the threshold and underrun cause this monitor intermittent > issue, or would this be something else? > > Does it make sense to have a flag or device tree option to override > the default thresholds to change the panic level? > > adam
Re: [PATCH] drm/bridge: imx8mp-hdmi-tx: allow 0.5% margin with selected clock
On 04.09.24 10:31 AM, Dominique Martinet wrote:
> This allows the hdmi driver to pick e.g. 64.8MHz instead of 65Mhz when we
> cannot output the exact frequency, enabling the imx8mp HDMI output to
> support more modes
>
> Signed-off-by: Dominique Martinet
Reviewed-by: Frieder Schrempf
Tested-by: Frieder Schrempf
> ---
> This completes the patch series sent by Adam Ford here:
> https://lkml.kernel.org/r/20240904023310.163371-1-aford...@gmail.com
>
> and makes the cheap screens we recommend work with our imx8mp board
> without further kludging.
>
>
> drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
> b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
> index 13bc570c5473..9431cd5e06c3 100644
> --- a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
> +++ b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
> @@ -23,6 +23,7 @@ imx8mp_hdmi_mode_valid(struct dw_hdmi *dw_hdmi, void *data,
> const struct drm_display_mode *mode)
> {
> struct imx8mp_hdmi *hdmi = (struct imx8mp_hdmi *)data;
> + long round_rate;
>
> if (mode->clock < 13500)
> return MODE_CLOCK_LOW;
> @@ -30,8 +31,9 @@ imx8mp_hdmi_mode_valid(struct dw_hdmi *dw_hdmi, void *data,
> if (mode->clock > 297000)
> return MODE_CLOCK_HIGH;
>
> - if (clk_round_rate(hdmi->pixclk, mode->clock * 1000) !=
> - mode->clock * 1000)
> + round_rate = clk_round_rate(hdmi->pixclk, mode->clock * 1000);
> + /* accept 0.5% = 1/200 = 5/1000 tolerance */
> + if (abs(round_rate - mode->clock * 1000) > mode->clock * 5)
> return MODE_CLOCK_RANGE;
>
> /* We don't support double-clocked and Interlaced modes */
[PATCH v2 0/4] arm64: dts: imx8mm-kontron: Add HDMI and LVDS display support
From: Frieder Schrempf This add support for the display bridges (DSI->LVDS and DSI->HDMI) on the BL i.MX8MM and the 7" LVDS panel in a separate overlay. Only one of the interfaces (HDMI or LVDS) is supported at the same time. Enabling the LVDS overlay will disable the HDMI interface. * Patch 1 and 2: Add the necessary binding changes * Patch 3: Extend the BL devicetree * Patch 4: Add the LVDS panel overlay Changes for v2: * Patch 1: Add link to commit message * Patch 2: Add Conors A-b tag * Patch 3: Remove blank lines from hdmi node * Patch 3: Fix order of lvds and hdmi nodes within i2c * Patch 3: Remove the unneeded deletion of samsung,pll-clock-frequency * Patch 3: Use the existing MIPI DSI output port from imx8mm.dtsi * Patch 4: Update copyright year * Patch 4: Use exisitng MIPI DSI output port from imx8mm.dtsi * Patch 4: Fix pinctrl for GPIO hogs * Patch 4: Fix property order in i2c2 node * Patch 4: Use generic node name for touchscreen Frieder Schrempf (4): dt-bindings: vendor-prefixes: Add Jenson Display dt-bindings: display: panel-lvds: Add compatible for Jenson BL-JT60050-01A arm64: dts: imx8mm-kontron: Add support for display bridges on BL i.MX8MM arm64: dts: imx8mm-kontron: Add DL (Display-Line) overlay with LVDS support .../bindings/display/panel/panel-lvds.yaml| 2 + .../devicetree/bindings/vendor-prefixes.yaml | 2 + arch/arm64/boot/dts/freescale/Makefile| 4 + .../boot/dts/freescale/imx8mm-kontron-bl.dts | 131 .../boot/dts/freescale/imx8mm-kontron-dl.dtso | 189 ++ 5 files changed, 328 insertions(+) create mode 100644 arch/arm64/boot/dts/freescale/imx8mm-kontron-dl.dtso -- 2.46.0
[PATCH v2 2/4] dt-bindings: display: panel-lvds: Add compatible for Jenson BL-JT60050-01A
From: Frieder Schrempf The Jenson BL-JT60050-01A is a 7" 1024x600 LVDS display. Signed-off-by: Frieder Schrempf Acked-by: Conor Dooley --- Changes for v2: * Add tag from Conor (thanks!) --- Documentation/devicetree/bindings/display/panel/panel-lvds.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/display/panel/panel-lvds.yaml b/Documentation/devicetree/bindings/display/panel/panel-lvds.yaml index 155d8ffa8f6ef..5af2d69300751 100644 --- a/Documentation/devicetree/bindings/display/panel/panel-lvds.yaml +++ b/Documentation/devicetree/bindings/display/panel/panel-lvds.yaml @@ -50,6 +50,8 @@ properties: - hannstar,hsd101pww2 # Hydis Technologies 7" WXGA (800x1280) TFT LCD LVDS panel - hydis,hv070wx2-1e0 + # Jenson Display BL-JT60050-01A 7" WSVGA (1024x600) color TFT LCD LVDS panel + - jenson,bl-jt60050-01a - tbs,a711-panel - const: panel-lvds -- 2.46.0
Re: drm/bridge/imx8mp-hdmi-tx: Allow inexact pixel clock frequencies (Was: [PATCH V8 10/12] drm/bridge: imx: add bridge wrapper driver for i.MX8MP DWC HDMI)
Hi Adam, On 27.08.24 2:25 AM, Adam Ford wrote: > On Wed, Aug 21, 2024 at 8:59 PM Adam Ford wrote: >> >> On Wed, Aug 21, 2024 at 7:45 AM Adam Ford wrote: >>> >>> On Tue, Aug 20, 2024 at 10:58 PM Dominique MARTINET >>> wrote: Adam Ford wrote on Tue, Aug 20, 2024 at 09:49:03PM -0500: >>> However, this check is a bit overcautious in that it only allows exact >>> rate matches. IIRC HDMI allows a rate mismatch of +- 0.5%, so this >>> check could be relaxed quite a bit to allow for that. >> >> I checked with a 1080p display that reports 23 possible modes via EDID. >> Out of these 15 are accepted by the driver with the strict check. >> >> Two more would be accepted with a relaxed check that allows a 0.5% >> margin. >> >> For the remaining six modes, the pixel clock would deviate up to ~5% >> from what the display expects. Still, if I remove the check altogether, >> all 23 modes seem to work just fine. I can confirm the displays I tested also seem pretty tolerant in practice (up to ~3-4% at least), but I agree with Lucas that this isn't something we can rely on for a general purpose driver as having examples of things being tolerant doesn't mean all hardware will be as flexible.. >> I'd really like to be able to add more PHY PLL setpoints for displays >> that use non-CEA-861 modes. Unfortunately I didn't manage to figure out >> the fractional-n PLL parameter calculation so far. >> >> The i.MX8MP Reference Manual provides formulas to calculate the >> parameters based on the register values and I tried to make sense of it >> using the existing register values in the driver. But somehow it doesn't >> add up for me. >> >> Lucas, did you already work with the PLL parameters? By any chance, do >> you now how the math behind them works? > > I spent a little time trying to understand it a bit. I created a PMS > calculator similar to the one used on the DSI controller, Great! I'll admit this also flies over my head and I don't have the time to study this, so this is much appreciated. > except that > the M seems to be fixed at a value of 62 per the data sheet, so it's > more of a PS calculator. Hmm... PHY_REG2 in the datasheet only lists '0011_1110b - 62' as example(?) values, but it doesn't say other values are reserved either, I'm not sure what to make of it. In the current driver PHY_REG_02 (driver macro) is assigned the first field of .pll_div_regs, which goes anywhere from 0x4b to 0x7b -- pretty far from 62(0x3e)... >>> >>> OK. I will experiment with increasing the range of M from being fixed >>> at 3e to a range of 3b to 7b to see if my PMS integer calculator can >>> get more accurate. >>> Since other frequencies have been adjusting this main diviser ratio we actually tried copying neighboring values and adjusting only that reg 2 (so the M if I get this right?), but the end result ended up not synchronizing properly every time... We didn't have time to check with a scope if the generated signal was ugly or if it just didn't lock properly, but the display worked when we just re-used the neighboring values without changing anything despite being ~3-4% off, so we took the easy way out here and didn't dig much further. > Anyway, When I run my P-S calculator to generate the 'best rate' I get > a value that's usually 0.2% variance from nominal, but I only verified > a handful of values: > > Several which were +0.2% > 29760 vs 29700 (4k@30) > 14880 vs 14850 (1080p60) > 74400 vs 74200 > > One value was -0.16% > 2480 vs 2520 > > If the M value was a bit more flexible, we might be able to reduce > that variance more. Yes, I think the M value could be more flexible, but that'd require checking if it actually works, whereas having slightly off frequencies will most likely be OK so I don't think it's worth the effort -- happy to stick to what the datasheet describes. > If / when I get some time, I might play with trying to disable the > fractional mode and just use the PMS calculator for simplicity despite > the inaccuracy. Maybe we could fall back to using the PMS calculator > if the desired frequency isn't in the look-up-table. That'd be greatly appreciated, I don't have any strong opinion on whether that should completely replace the table, or only be used if there is no exact match in the table as these are values we know will work; but we can definitely test any patch you can throw at us here. >>> >>> I can't promise it'll be quick. I am not fully certain I understand >>> how the whole thing works, but as a rule, I don't generally like look >>> up tables if they can be calculated dynamically. >> >> I updated m
Re: drm/bridge/imx8mp-hdmi-tx: Allow inexact pixel clock frequencies (Was: [PATCH V8 10/12] drm/bridge: imx: add bridge wrapper driver for i.MX8MP DWC HDMI)
Hi Dominique, hi Lucas,
On 17.06.24 6:32 PM, Lucas Stach wrote:
> Hi Dominique,
>
> Am Montag, dem 17.06.2024 um 15:16 +0900 schrieb Dominique MARTINET:
>> Adam Ford wrote on Sat, Feb 03, 2024 at 10:52:50AM -0600:
>>> From: Lucas Stach
>>>
>>> Add a simple wrapper driver for the DWC HDMI bridge driver that
>>> implements the few bits that are necessary to abstract the i.MX8MP
>>> SoC integration.
>>
>> Hi Lucas, Adam,
>> (trimmed ccs a bit)
>>
>> First, thank you for the effort of upstreaming all of this!! It's really
>> appreciated, and with display working I'll really be wanting to upstream
>> our DTS as well as soon as I have time (which is going to be a while,
>> but better late than never ?)
>>
>> Until then, it's been a few months but I've got a question on this bit:
>>
>>> diff --git a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
>>> b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
>>> new file mode 100644
>>> index ..89fc432ac611
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
>>> +static enum drm_mode_status
>>> +imx8mp_hdmi_mode_valid(struct dw_hdmi *dw_hdmi, void *data,
>>> + const struct drm_display_info *info,
>>> + const struct drm_display_mode *mode)
>>> +{
>>> + struct imx8mp_hdmi *hdmi = (struct imx8mp_hdmi *)data;
>>> +
>>> + if (mode->clock < 13500)
>>> + return MODE_CLOCK_LOW;
>>> +
>>> + if (mode->clock > 297000)
>>> + return MODE_CLOCK_HIGH;
>>> +
>>> + if (clk_round_rate(hdmi->pixclk, mode->clock * 1000) !=
>>> + mode->clock * 1000)
>>> + return MODE_CLOCK_RANGE;
>>
>> Do you know why such a check is here?
>
> Sending a HDMI signal with a different rate than what the display
> expects rarely ends well, so this check avoids that.
>
> However, this check is a bit overcautious in that it only allows exact
> rate matches. IIRC HDMI allows a rate mismatch of +- 0.5%, so this
> check could be relaxed quite a bit to allow for that.
I checked with a 1080p display that reports 23 possible modes via EDID.
Out of these 15 are accepted by the driver with the strict check.
Two more would be accepted with a relaxed check that allows a 0.5% margin.
For the remaining six modes, the pixel clock would deviate up to ~5%
from what the display expects. Still, if I remove the check altogether,
all 23 modes seem to work just fine.
>>
>> When plugging in a screen with no frequency identically supported in its
>> EDID this check causes the screen to stay black, and we've been telling
>> customers to override the EDID but it's a huge pain.
>>
>> Commit 6ad082bee902 ("phy: freescale: add Samsung HDMI PHY") already
>> "fixed" the samsung hdmi phy driver to return the next frequency if an
>> exact match hasn't been found (NXP tree's match frequencies exactly, but
>> this gets the first clock with pixclk <= rate), so if this check is also
>> relaxed our displays would work out of the box.
>>
>> I also don't see any other bridge doing this kind of check.
>> drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c has a similar check with a
>> 0.5% leeway, and all the other drivers don't check anything.
>> If you want to add some level of safety, I think we could make this work
>> with a 5% margin easily... Printing a warning in dmesg could work if
>> you're worried about artifacts, but litteraly anything is better than a
>> black screen with no error message in my opinion.
>>
>>
>> In practice the screen I'm looking at has an EDID which only supports
>> 51.2MHz and the closest frequency supported by the Samsung HDMI phy is
>> 50.4MHz, so that's a ~1.5% difference and it'd be great if it could work
>> out of the box.
>
> For rate mismatches larger than the 0.5% allowed by the HDMI spec it
> would be better to actually add PHY PLL parameters matching those
> rates.
I'd really like to be able to add more PHY PLL setpoints for displays
that use non-CEA-861 modes. Unfortunately I didn't manage to figure out
the fractional-n PLL parameter calculation so far.
The i.MX8MP Reference Manual provides formulas to calculate the
parameters based on the register values and I tried to make sense of it
using the existing register values in the driver. But somehow it doesn't
add up for me.
Lucas, did you already work with the PLL parameters? By any chance, do
you now how the math behind them works?
>
> We could potentially add some more leeway for displays like yours that
> aren't actually HDMI (as it doesn't seem to have a standard HDMI
> resolution). But that's more of a last resort option, as it may
> introduce other problems for displays that aren't as tolerant with
> their input rates. Remember the mode_valid call is used to filter modes
> that aren't compatible with the source, so for a display with multiple
> modes allowing too much leeway may lead to incompatible modes not
> getting tossed, in turn allowing userspace to set a mode that the
> display may not like due to too much rate deviation, instead of only
> p
[PATCH 0/4] arm64: dts: imx8mm-kontron: Add HDMI and LVDS display support
From: Frieder Schrempf This add support for the display bridges (DSI->LVDS and DSI->HDMI) on the BL i.MX8MM and the 7" LVDS panel in a separate overlay. Only one of the interfaces (HDMI or LVDS) is supported at the same time. Enabling the LVDS overlay will disable the HDMI interface. * Patch 1 and 2: Add the necessary binding changes * Patch 3: Extend the BL devicetree * Patch 4: Add the LVDS panel overlay Frieder Schrempf (4): dt-bindings: vendor-prefixes: Add Jenson Display dt-bindings: display: panel-lvds: Add compatible for Jenson BL-JT60050-01A arm64: dts: imx8mm-kontron: Add support for display bridges on BL i.MX8MM arm64: dts: imx8mm-kontron: Add DL (Display-Line) overlay with LVDS support .../bindings/display/panel/panel-lvds.yaml| 2 + .../devicetree/bindings/vendor-prefixes.yaml | 2 + arch/arm64/boot/dts/freescale/Makefile| 4 + .../boot/dts/freescale/imx8mm-kontron-bl.dts | 146 .../boot/dts/freescale/imx8mm-kontron-dl.dtso | 210 ++ 5 files changed, 364 insertions(+) create mode 100644 arch/arm64/boot/dts/freescale/imx8mm-kontron-dl.dtso -- 2.45.2
[PATCH 2/4] dt-bindings: display: panel-lvds: Add compatible for Jenson BL-JT60050-01A
From: Frieder Schrempf The Jenson BL-JT60050-01A is a 7" 1024x600 LVDS display. Signed-off-by: Frieder Schrempf --- Documentation/devicetree/bindings/display/panel/panel-lvds.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/display/panel/panel-lvds.yaml b/Documentation/devicetree/bindings/display/panel/panel-lvds.yaml index 155d8ffa8f6ef..5af2d69300751 100644 --- a/Documentation/devicetree/bindings/display/panel/panel-lvds.yaml +++ b/Documentation/devicetree/bindings/display/panel/panel-lvds.yaml @@ -50,6 +50,8 @@ properties: - hannstar,hsd101pww2 # Hydis Technologies 7" WXGA (800x1280) TFT LCD LVDS panel - hydis,hv070wx2-1e0 + # Jenson Display BL-JT60050-01A 7" WSVGA (1024x600) color TFT LCD LVDS panel + - jenson,bl-jt60050-01a - tbs,a711-panel - const: panel-lvds -- 2.45.2
Re: [PATCH V2 2/2] drm/bridge: samsung-dsim: Fix porch calcalcuation rounding
On 25.04.24 22:30, Adam Ford wrote:
> On Thu, Apr 25, 2024 at 4:19 AM Marek Szyprowski
> wrote:
>>
>> On 12.02.2024 00:09, Adam Ford wrote:
>>> When using video sync pulses, the HFP, HBP, and HSA are divided between
>>> the available lanes if there is more than one lane. For certain
>>> timings and lane configurations, the HFP may not be evenly divisible.
>>> If the HFP is rounded down, it ends up being too small which can cause
>>> some monitors to not sync properly. In these instances, adjust htotal
>>> and hsync to round the HFP up, and recalculate the htotal.
>>>
>>> Tested-by: Frieder Schrempf # Kontron BL
>>> i.MX8MM with HDMI monitor
>>> Signed-off-by: Adam Ford
>>
>> Tested-by: Marek Szyprowski
>
> Thank you very much for testing!
>
>>
>>> ---
>>> V2: No changes
>>>
>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
>>> b/drivers/gpu/drm/bridge/samsung-dsim.c
>>> index 8476650c477c..52939211fe93 100644
>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>>> @@ -1606,6 +1606,27 @@ static int samsung_dsim_atomic_check(struct
>>> drm_bridge *bridge,
>>> adjusted_mode->flags |= (DRM_MODE_FLAG_PHSYNC |
>>> DRM_MODE_FLAG_PVSYNC);
>>> }
>>>
>>> + /*
>>> + * When using video sync pulses, the HFP, HBP, and HSA are divided
>>> between
>>> + * the available lanes if there is more than one lane. For certain
>>> + * timings and lane configurations, the HFP may not be evenly
>>> divisible.
>>> + * If the HFP is rounded down, it ends up being too small which can
>>> cause
>>> + * some monitors to not sync properly. In these instances, adjust
>>> htotal
>>> + * and hsync to round the HFP up, and recalculate the htotal. Through
>>> trial
>>> + * and error, it appears that the HBP and HSA do not appearto need
>>> the same
>>> + * correction that HFP does.
>>> + */
>>> + if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE && dsi->lanes >
>>> 1) {
>
> Frieder & Marek S,
>
> Marek V is proposing we eliminate the check against the flags and do
> it unconditionally. If I send you both a different patch, would you
> be willing to try them on your platforms? I don't want to risk
> breaking a board.
> I used the check above from the NXP downstream kernel, so it felt
> safe, but I am not as familiar with the different DSI modes, so I am
> not sure what the impact would be if this read:
>
> if (dsi->lanes > 1) {
>
> Does anyone else have an opinion on this?
My test only covers hardware with the ADV7535 which sets
MIPI_DSI_MODE_VIDEO_SYNC_PULSE. Doing the test without the check for
this flag won't make any difference in this case and it's therefore not
worth repeating the test.
Re: sn65dsi83: dsi burst mode
On 07.03.24 09:09, Sean Nyekjaer wrote: > Hi, > > We are using the stm32mp1 together with the sn65dsi83 bridge. > The ti,sn65dsi83 driver is (hard) enabling MIPI_DSI_MODE_VIDEO_BURST, then > the st,stm32-dsi driver is adding +20% to the clock speed. > > That means our LVDS is +20% higher than expected. > > Any proposals for a fix? Could we add a devicetree option to opt out of the > burst mode? FYI, this issue has been raised before: https://lkml.org/lkml/2022/10/4/455.
Re: [PATCH v2 1/2] drm/bridge: Fix improper bridge init order with pre_enable_prev_first
Hi,
On 28.03.23 19:07, Jagan Teki wrote:
> For a given bridge pipeline if any bridge sets pre_enable_prev_first
> flag then the pre_enable for the previous bridge will be called before
> pre_enable of this bridge and opposite is done for post_disable.
>
> These are the potential bridge flags to alter bridge init order in order
> to satisfy the MIPI DSI host and downstream panel or bridge to function.
> However the existing pre_enable_prev_first logic with associated bridge
> ordering has broken for both pre_enable and post_disable calls.
>
> [pre_enable]
>
> The altered bridge ordering has failed if two consecutive bridges on a
> given pipeline enables the pre_enable_prev_first flag.
>
> Example:
> - Panel
> - Bridge 1
> - Bridge 2 pre_enable_prev_first
> - Bridge 3
> - Bridge 4 pre_enable_prev_first
> - Bridge 5 pre_enable_prev_first
> - Bridge 6
> - Encoder
>
> In this example, Bridge 4 and Bridge 5 have pre_enable_prev_first.
>
> The logic looks for a bridge which enabled pre_enable_prev_first flag
> on each iteration and assigned the previou bridge to limit pointer
> if the bridge doesn't enable pre_enable_prev_first flags.
>
> If control found Bridge 2 is pre_enable_prev_first then the iteration
> looks for Bridge 3 and found it is not pre_enable_prev_first and assigns
> it's previous Bridge 4 to limit pointer and calls pre_enable of Bridge 3
> and Bridge 2 and assign iter pointer with limit which is Bridge 4.
>
> Here is the actual problem, for the next iteration control look for
> Bridge 5 instead of Bridge 4 has iter pointer in previous iteration
> moved to Bridge 4 so this iteration skips the Bridge 4. The iteration
> found Bridge 6 doesn't pre_enable_prev_first flags so the limit assigned
> to Encoder. From next iteration Encoder skips as it is the last bridge
> for reverse order pipeline.
>
> So, the resulting pre_enable bridge order would be,
> - Panel, Bridge 1, Bridge 3, Bridge 2, Bridge 6, Bridge 5.
>
> This patch fixes this by assigning limit to next pointer instead of
> previous bridge since the iteration always looks for bridge that does
> NOT request prev so assigning next makes sure the last bridge on a
> given iteration what exactly the limit bridge is.
>
> So, the resulting pre_enable bridge order with fix would be,
> - Panel, Bridge 1, Bridge 3, Bridge 2, Bridge 6, Bridge 5, Bridge 4,
> Encoder.
>
> [post_disable]
>
> The altered bridge ordering has failed if two consecutive bridges on a
> given pipeline enables the pre_enable_prev_first flag.
>
> Example:
> - Panel
> - Bridge 1
> - Bridge 2 pre_enable_prev_first
> - Bridge 3
> - Bridge 4 pre_enable_prev_first
> - Bridge 5 pre_enable_prev_first
> - Bridge 6
> - Encoder
>
> In this example Bridge 5 and Bridge 4 have pre_enable_prev_first.
>
> The logic looks for a bridge which enabled pre_enable_prev_first flags
> on each iteration and assigned the previou bridge to next and next to
> limit pointer if the bridge does enable pre_enable_prev_first flag.
>
> If control starts from Bridge 6 then it found next Bridge 5 is
> pre_enable_prev_first and immediately the next assigned to previous
> Bridge 6 and limit assignments to next Bridge 6 and call post_enable
> of Bridge 6 even though the next consecutive Bridge 5 is enabled with
> pre_enable_prev_first. This clearly misses the logic to find the state
> of next conducive bridge as everytime the next and limit assigns
> previous bridge if given bridge enabled pre_enable_prev_first.
>
> So, the resulting post_disable bridge order would be,
> - Encoder, Bridge 6, Bridge 5, Bridge 4, Bridge 3, Bridge 2, Bridge 1,
> Panel.
>
> This patch fixes this by assigning next with previou bridge only if the
> bridge doesn't enable pre_enable_prev_first flag and the next further
> assign it to limit. This way we can find the bridge that NOT requested
> prev to disable last.
>
> So, the resulting pre_enable bridge order with fix would be,
> - Encoder, Bridge 4, Bridge 5, Bridge 6, Bridge 2, Bridge 3, Bridge 1,
> Panel.
>
> Validated the bridge init ordering by incorporating dummy bridges in
> the sun6i-mipi-dsi pipeline
>
> Fixes: 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to
> alter bridge init order")
> Signed-off-by: Jagan Teki
This patch is now almost 1 year old and it has been tested and reviewed
and there have been multiple pings.
Is there anything missing? Why is it not applied yet?
Andrzej, Neil, Robert: As DRM bridge maintainers, can you take care of this?
Thanks
Frieder
Re: [PATCH 1/1] drm/bridge: ti-sn65dsi83: Fix enable error path
On 29.02.24 10:47, Luca Ceresoli wrote:
> Hello Alexander,
>
> On Wed, 28 Feb 2024 09:15:46 +0100
> Alexander Stein wrote:
>
>
> [...]
>
>> Oh I mistook this DSI-LVDS bridge with the DSI-DP bridge on a different
>> board, my bad. I hope I can provide some insights. My platform is
>> imx8mm-tqma8mqml-mba8mx-lvds-tm070jvhg33.dtb.
>> I can easily cause a PLL lock failure by reducing the delay for the
>> enable-gpios 'gpio_delays'. This will result in a PLL lock faiure.
>> On my platform the vcc-supply counters do look sane:
>>> /sys/kernel/debug/regulator/SN65DSI83_1V8/open_count:1
>>> /sys/kernel/debug/regulator/SN65DSI83_1V8/use_count:0
>
> Interesting. Thanks for taking time to report your initial issue!
>
>> Once I remove the ti_sn65dsi83 module, the open_count decrements to 0 as
>> well. Looks sane to me.
>>
>> If I revert commit c81cd8f7c774 ("Revert "drm/bridge: ti-sn65dsi83:
>> Fix enable error path""), vcc-supply counters are:
>>> /sys/kernel/debug/regulator/SN65DSI83_1V8/open_count:1
>>> /sys/kernel/debug/regulator/SN65DSI83_1V8/use_count:1
>>
>> So in my case the use_count does not decrease! If I remove the module
>> ti_sn65dsi83, I get the WARN_ON (enable_count is still non-zero):
>>> WARNING: CPU: 2 PID: 402 at drivers/regulator/core.c:2398
>>> _regulator_put+0x15c/0x164
>>
>> This is on 6.8.0-rc6-next-20240228 with the following diff applied:
>> --->8---
>> diff --git a/arch/arm64/boot/dts/freescale/mba8mx.dtsi
>> b/arch/arm64/boot/dts/freescale/mba8mx.dtsi
>> index 427467df42bf..8461e1fd396f 100644
>> --- a/arch/arm64/boot/dts/freescale/mba8mx.dtsi
>> +++ b/arch/arm64/boot/dts/freescale/mba8mx.dtsi
>> @@ -285,7 +285,7 @@ &i2c3 {
>> dsi_lvds_bridge: bridge@2d {
>> compatible = "ti,sn65dsi84";
>> reg = <0x2d>;
>> - enable-gpios = <&gpio_delays 0 13 0>;
>> + enable-gpios = <&gpio_delays 0 0 0>;
>> vcc-supply = <®_sn65dsi83_1v8>;
>> status = "disabled";
>>
>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>> b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>> index 4814b7b6d1fd..57a7ed13f996 100644
>> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>> @@ -478,7 +478,6 @@ static void sn65dsi83_atomic_pre_enable(struct
>> drm_bridge *bridge,
>> dev_err(ctx->dev, "failed to lock PLL, ret=%i\n", ret);
>> /* On failure, disable PLL again and exit. */
>> regmap_write(ctx->regmap, REG_RC_PLL_EN, 0x00);
>> - regulator_disable(ctx->vcc);
>> return;
>> }
>> --->8---
>>
>> So my patch indeed did fix an actual problem. On the other hand it seems
>> sn65dsi83_atomic_disable is not called in my case for some reason.
>
> So you remove the module and atomic_disable is not called, after
> having called atomic_pre_enable?
>
> I'm very possibly missing something, but this looks like a bug in the
> DRM bridge code at first sight.
I'm just guessing, but could it be that this patch [1] would fix it?
It looks like nobody cared to pick this up, although there are several
reports for defects caused by [2] and this patch is supposed to fix them.
[1] https://patchwork.freedesktop.org/patch/529288/
[2]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=4fb912e5e19075874379cfcf074d90bd51ebf8ea
Re: [PATCH] drm: bridge: samsung-dsim: Don't use FORCE_STOP_STATE
On 29.01.24 10:20, Frieder Schrempf wrote: > On 26.01.24 19:28, Dave Airlie wrote: >> Just FYI this conflictted pretty heavily with drm-misc-next changes in >> the same area, someone should check drm-tip has the correct >> resolution, I'm not really sure what is definitely should be. >> >> Dave. > > Thanks! I took a quick look at what is now in Linus' tree and it looks > correct to me. The only thing I'm missing is my Reviewed-by tag which > got lost somewhere, but I can get over that. Apparently I missed the point here. I was looking at the wrong trees (drm-next and master instead of drm-misc-next and drm-tip). Sorry for the noise. Michael already pointed out the correct details.
Re: [PATCH] drm: bridge: samsung-dsim: Don't use FORCE_STOP_STATE
On 26.01.24 19:28, Dave Airlie wrote: > Just FYI this conflictted pretty heavily with drm-misc-next changes in > the same area, someone should check drm-tip has the correct > resolution, I'm not really sure what is definitely should be. > > Dave. Thanks! I took a quick look at what is now in Linus' tree and it looks correct to me. The only thing I'm missing is my Reviewed-by tag which got lost somewhere, but I can get over that.
Re: [PATCH 2/2] drm/bridge: samsung-dsim: Fix porch calcalcuation rounding
On 25.01.24 19:44, Adam Ford wrote: > On Mon, Dec 11, 2023 at 9:33 PM Adam Ford wrote: >> >> When using video sync pulses, the HFP, HBP, and HSA are divided between >> the available lanes if there is more than one lane. For certain >> timings and lane configurations, the HFP may not be evenly divisible. >> If the HFP is rounded down, it ends up being too small which can cause >> some monitors to not sync properly. In these instances, adjust htotal >> and hsync to round the HFP up, and recalculate the htotal. >> >> Tested-by: Frieder Schrempf # Kontron BL >> i.MX8MM with HDMI monitor >> Signed-off-by: Adam Ford > > Gentle nudge on this one. Basically this fixes an issue with the 8MP, > but it's still unknown why it doesn't work on 8MM or 8MN, but Frieder > confirmed there are no regressions on 8MM or 8MN. I only tested two specific display setups on i.MX8MM. So of course I can't confirm the absence of regressions in general. Anyway, I think this should be applied.
Re: [PATCH] drm: bridge: samsung-dsim: Don't use FORCE_STOP_STATE
On 01.12.23 10:04, Michael Walle wrote:
>> The FORCE_STOP_STATE bit is unsuitable to force the DSI link into LP-11
>> mode. It seems the bridge internally queues DSI packets and when the
>> FORCE_STOP_STATE bit is cleared, they are sent in close succession
>> without any useful timing (this also means that the DSI lanes won't go
>> into LP-11 mode). The length of this gibberish varies between 1ms and
>> 5ms. This sometimes breaks an attached bridge (TI SN65DSI84 in this
>> case). In our case, the bridge will fail in about 1 per 500 reboots.
>>
>> The FORCE_STOP_STATE handling was introduced to have the DSI lanes in
>> LP-11 state during the .pre_enable phase. But as it turns out, none of
>> this is needed at all. Between samsung_dsim_init() and
>> samsung_dsim_set_display_enable() the lanes are already in LP-11 mode.
>> The code as it was before commit 20c827683de0 ("drm: bridge:
>> samsung-dsim: Fix init during host transfer") and 0c14d3130654 ("drm:
>> bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec") was correct
>> in this regard.
>>
>> This patch basically reverts both commits. It was tested on an i.MX8M
>> SoC with an SN65DSI84 bridge. The signals were probed and the DSI
>> packets were decoded during initialization and link start-up. After this
>> patch the first DSI packet on the link is a VSYNC packet and the timing
>> is correct.
>>
>> Command mode between .pre_enable and .enable was also briefly tested by
>> a quick hack. There was no DSI link partner which would have responded,
>> but it was made sure the DSI packet was send on the link. As a side
>> note, the command mode seems to just work in HS mode. I couldn't find
>> that the bridge will handle commands in LP mode.
>>
>> Fixes: 20c827683de0 ("drm: bridge: samsung-dsim: Fix init during host
>> transfer")
>> Fixes: 0c14d3130654 ("drm: bridge: samsung-dsim: Fix i.MX8M enable
>> flow to meet spec")
>> Signed-off-by: Michael Walle
>> ---
>> Let me know wether this should be two commits each reverting one, but
>> both
>> commits appeared first in kernel 6.5.
>
> Are there any news?
Inki, are you picking this up? Or if not, who will?
Re: [PATCH 1/2] drm/bridge: samsung-dsim: Set P divider based on min/max of fin pll
Hi Adam,
On 12.12.23 04:32, Adam Ford wrote:
> The P divider should be set based on the min and max values of
> the fin pll which may vary between different platforms.
> These ranges are defined per platform, but hard-coded values
> were used instead which resulted in a smaller range available
> on the i.MX8M[MNP] than what was possible.
>
> Fixes: 846307185f0f ("drm/bridge: samsung-dsim: update PLL reference clock")
> Signed-off-by: Adam Ford
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> b/drivers/gpu/drm/bridge/samsung-dsim.c
> index be5914caa17d..239d253a7d71 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -573,8 +573,8 @@ static unsigned long samsung_dsim_pll_find_pms(struct
> samsung_dsim *dsi,
> u16 _m, best_m;
> u8 _s, best_s;
>
> - p_min = DIV_ROUND_UP(fin, (12 * MHZ));
> - p_max = fin / (6 * MHZ);
> + p_min = DIV_ROUND_UP(fin, (driver_data->pll_fin_max * MHZ));
> + p_max = fin / (driver_data->pll_fin_min * MHZ);
I did some tinkering with the PLL settings the other day and this is
literally one of the things I came up with.
So I'm happy to provide:
Reviewed-by: Frieder Schrempf
Tested-by: Frieder Schrempf
Regarding the P divider, I'm also wondering if there is an upper limit
for the p-value (not for the resulting fin_pll) that we should take into
account, too. The problem is that we have conflicts in the documentation
(again) so we don't really know what the correct limit would be.
There are the following ranges given in the RMs:
* 1..63 (i.MX8MM RM 13.7.8.18.4)
* 1..33 (i.MX8MM RM 13.7.10.1)
* 1..63 (i.MX8MP RM 13.2.3.1.5.2)
* 1..63 (i.MX8MP RM 13.7.2.4)
Unfortunately there are similar discrepancies for the other parameters
and limits.
Thanks
Frieder
>
> for (_p = p_min; _p <= p_max; ++_p) {
> for (_s = 0; _s <= 5; ++_s) {
Re: [PATCH v5 04/10] drm: bridge: samsung-dsim: complete the CLKLANE_STOP setting
On 07.12.23 15:16, Dario Binacchi wrote:
> The patch completes the setting of CLKLANE_STOP for the imx8mn and imx8mp
> platforms (i. e. not exynos).
This also affects i.MX8MM, so better just mention i.MX in general in the
commit message.
>
> Co-developed-by: Michael Trimarchi
> Signed-off-by: Michael Trimarchi
> Signed-off-by: Dario Binacchi
> ---
>
> (no changes since v1)
>
> drivers/gpu/drm/bridge/samsung-dsim.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 15bf05b2bbe4..13f181c99d7e 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -96,6 +96,7 @@
> #define DSIM_MFLUSH_VS BIT(29)
> /* This flag is valid only for exynos3250/3472/5260/5430 */
> #define DSIM_CLKLANE_STOPBIT(30)
> +#define DSIM_NON_CONTINUOUS_CLKLANE BIT(31)
>
> /* DSIM_ESCMODE */
> #define DSIM_TX_TRIGGER_RST BIT(4)
> @@ -945,8 +946,12 @@ static int samsung_dsim_init_link(struct samsung_dsim
> *dsi)
>* power consumption.
>*/
> if (driver_data->has_clklane_stop &&
> - dsi->mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS)
> + dsi->mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) {
> + if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
> + reg |= DSIM_NON_CONTINUOUS_CLKLANE;
> +
> reg |= DSIM_CLKLANE_STOP;
> + }
I really wonder what the difference between DSIM_NON_CONTINUOUS_CLKLANE
and DSIM_CLKLANE_STOP is.
If Exynos only has the latter, it's pretty clear what to use. But as
i.MX has both of these bits, should both be set? Or is setting
DSIM_NON_CONTINUOUS_CLKLANE enough and we should leave DSIM_CLKLANE_STOP
alone?
Maybe someone has a clue here. The description of the bits in the RM is:
DSIM_NON_CONTINUOUS_CLKLANE - Non-continuous clock mode
DSIM_CLKLANE_STOP - PHY clock lane On/Off for ESD
> samsung_dsim_write(dsi, DSIM_CONFIG_REG, reg);
>
> lanes_mask = BIT(dsi->lanes) - 1;
Re: [PATCH] drm/bridge: samsung-dsim: check the return value only if necessary
On 07.12.23 17:10, Dario Binacchi wrote:
> It was useless to check again the "ret" variable if the function
> register_host() was not called.
>
> Signed-off-by: Dario Binacchi
Reviewed-by: Frieder Schrempf
> ---
>
> drivers/gpu/drm/bridge/samsung-dsim.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> b/drivers/gpu/drm/bridge/samsung-dsim.c
> index be5914caa17d..98cd589e4427 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -2020,11 +2020,11 @@ int samsung_dsim_probe(struct platform_device *pdev)
> else
> dsi->bridge.timings = &samsung_dsim_bridge_timings_de_high;
>
> - if (dsi->plat_data->host_ops && dsi->plat_data->host_ops->register_host)
> + if (dsi->plat_data->host_ops &&
> dsi->plat_data->host_ops->register_host) {
> ret = dsi->plat_data->host_ops->register_host(dsi);
> -
> - if (ret)
> - goto err_disable_runtime;
> + if (ret)
> + goto err_disable_runtime;
> + }
>
> return 0;
>
Re: [PATCH v2 1/2] drm: bridge: adv7511: fix reading edid segments
On 30.11.23 16:57, Frieder Schrempf wrote:
> Hi Emil,
>
> On 27.10.23 14:22, Emil Abildgaard Svendsen wrote:
>> [Sie erhalten nicht häufig E-Mails von e...@bang-olufsen.dk. Weitere
>> Informationen, warum dies wichtig ist, finden Sie unter
>> https://aka.ms/LearnAboutSenderIdentification ]
>>
>> Currently reading EDID only works because usually only two EDID blocks
>> of 128 bytes is used. Where an EDID segment holds 256 bytes or two EDID
>> blocks. And the first EDID segment read works fine but E-EDID specifies
>> up to 128 segments.
>>
>> The logic is broken so change EDID segment index to multiple of 256
>> bytes and not 128 (block size).
>>
>> Fixes: 9c8af882bf12 ("drm: Add adv7511 encoder driver")
>>
>> Signed-off-by: Emil Svendsen
>> ---
>> v2:
>> - Split into two patches.
>> - Add Fixes tag.
>>
>> v1: https://lore.kernel.org/all/20231026113029.575846-1-e...@bang-olufsen.dk/
>
> Unfortunately this v2 series breaks my setup in the same way as I
> reported for v1 [1].
To be more specific: It's actually patch 2/2 that breaks it. Patch 1/2
alone seems to work fine in my case.
Re: [PATCH v2 1/2] drm: bridge: adv7511: fix reading edid segments
Hi Emil,
On 27.10.23 14:22, Emil Abildgaard Svendsen wrote:
> [Sie erhalten nicht häufig E-Mails von e...@bang-olufsen.dk. Weitere
> Informationen, warum dies wichtig ist, finden Sie unter
> https://aka.ms/LearnAboutSenderIdentification ]
>
> Currently reading EDID only works because usually only two EDID blocks
> of 128 bytes is used. Where an EDID segment holds 256 bytes or two EDID
> blocks. And the first EDID segment read works fine but E-EDID specifies
> up to 128 segments.
>
> The logic is broken so change EDID segment index to multiple of 256
> bytes and not 128 (block size).
>
> Fixes: 9c8af882bf12 ("drm: Add adv7511 encoder driver")
>
> Signed-off-by: Emil Svendsen
> ---
> v2:
> - Split into two patches.
> - Add Fixes tag.
>
> v1: https://lore.kernel.org/all/20231026113029.575846-1-e...@bang-olufsen.dk/
Unfortunately this v2 series breaks my setup in the same way as I
reported for v1 [1].
Also, please add me to CC for future versions otherwise I will likely
miss them.
Thanks
Frieder
[1]
https://patchwork.kernel.org/project/dri-devel/patch/20231026113029.575846-1-e...@bang-olufsen.dk/#25572510
Re: [PATCH] drm: bridge: samsung-dsim: Don't use FORCE_STOP_STATE
Hi Michael,
On 13.11.23 17:43, Michael Walle wrote:
> The FORCE_STOP_STATE bit is unsuitable to force the DSI link into LP-11
> mode. It seems the bridge internally queues DSI packets and when the
> FORCE_STOP_STATE bit is cleared, they are sent in close succession
> without any useful timing (this also means that the DSI lanes won't go
> into LP-11 mode). The length of this gibberish varies between 1ms and
> 5ms. This sometimes breaks an attached bridge (TI SN65DSI84 in this
> case). In our case, the bridge will fail in about 1 per 500 reboots.
>
> The FORCE_STOP_STATE handling was introduced to have the DSI lanes in
> LP-11 state during the .pre_enable phase. But as it turns out, none of
> this is needed at all. Between samsung_dsim_init() and
> samsung_dsim_set_display_enable() the lanes are already in LP-11 mode.
> The code as it was before commit 20c827683de0 ("drm: bridge:
> samsung-dsim: Fix init during host transfer") and 0c14d3130654 ("drm:
> bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec") was correct
> in this regard.
>
> This patch basically reverts both commits. It was tested on an i.MX8M
> SoC with an SN65DSI84 bridge. The signals were probed and the DSI
> packets were decoded during initialization and link start-up. After this
> patch the first DSI packet on the link is a VSYNC packet and the timing
> is correct.
>
> Command mode between .pre_enable and .enable was also briefly tested by
> a quick hack. There was no DSI link partner which would have responded,
> but it was made sure the DSI packet was send on the link. As a side
> note, the command mode seems to just work in HS mode. I couldn't find
> that the bridge will handle commands in LP mode.
>
> Fixes: 20c827683de0 ("drm: bridge: samsung-dsim: Fix init during host
> transfer")
> Fixes: 0c14d3130654 ("drm: bridge: samsung-dsim: Fix i.MX8M enable flow to
> meet spec")
> Signed-off-by: Michael Walle
Thanks for the fix. Your explanation sounds convincing.
Unfortunately I'm currently not able to understand why I had to
introduce these changes in the first place. What I tried to fix was
exactly this kind of issue where the display stays black every few
hundred boot cycles.
My current guess would be that the issue I was seeing was already fixed
with dd9e329af723 ("drm/bridge: ti-sn65dsi83: Fix enable/disable flow to
meet spec") and I didn't properly test both changes separately.
My cheap scope is not able to capture the DSI signals and I admit that
we didn't use our more expensive equipment to verify the changes back then.
Instead, we had an automated test setup to do cyclic on/off switching
for the display and check for a black screen using a sensor. It is quite
a hassle to set up and I'm currently not planning to spend that much
effort to verify this change again.
Anyway, I currently don't see any reasons to not revert my changes. Your
revert looks correct and seems to work fine as far as I can tell.
Reviewed-by: Frieder Schrempf
Thanks
Frieder
Re: [PATCH] drm: bridge: adv7511: fix reading edid segments
On 26.10.23 13:30, Emil Abildgaard Svendsen wrote: > [Sie erhalten nicht häufig E-Mails von e...@bang-olufsen.dk. Weitere > Informationen, warum dies wichtig ist, finden Sie unter > https://aka.ms/LearnAboutSenderIdentification ] > > Currently reading EDID only works because usually only two EDID blocks > of 128 bytes is used. Where an EDID segment holds 256 bytes or two EDID > blocks. And the first EDID segment read works fine but E-EDID specifies > up to 128 segments. > > The logic is broken so change EDID segment index to multiple of 256 > bytes and not 128 (block size). > > Also change check of DDC status. Instead of silently not reading EDID > when in "IDLE" state [1]. Check if the DDC controller is in reset > instead (no HPD). > > [1] > ADV7511 Programming Guide: Table 11: DDCController Status: > > 0xC8 [3:0] DDC Controller State > > In Reset (No Hot Plug Detected) > 0001Reading EDID > 0010IDLE (Waiting for HDCP Requested) > 0011Initializing HDCP > 0100HDCP Enabled > 0101Initializing HDCP Repeater > > Signed-off-by: Emil Svendsen This patch somehow seems to break the reported display modes with my setup (i.MX8MM DSI -> ADV7535 -> FullHD screen) when I test on current linux-next. Without this patch I get 30 valid modes reported by modetest. With this patch applied I only get 5 valid modes. The screen still comes up with the 1080p default mode though, that is now not even in the list anymore.
Re: [RFC] drm: bridge: samsung-dsim: Recalculate timings when rounding HFP up
Hi Adam, On 13.10.23 05:10, Adam Ford wrote: > When using video sync pulses, the HFP, HBP, and HSA are divided between > the available lanes if there is more than one lane. For certain > timings and lane configurations, the HFP may not be evenly divisible > and it gets rounded down which can cause certain resolutions and > refresh rates to not sync. > > ie. 720p60 on some monitors show hss of 1390 and hdisplay of 1280. This > yields an HFP of 110. When taking the HFP of 110 divides along 4 lanes, > the result is 27.5 which rounds down to 27 and causes some monitors not > to sync. > > The solultion is to round HFP up by finding the remainder of HFP / > the number of lanes and increasing the hsync_start, hsync_end, and > htotal to compensate when there is a remainder. > > Signed-off-by: Adam Ford > --- > This adds support for 720p60 in the i.MX8M Plus. > > NXP uses a look-up table in their downstream code to accomplish this. > Using this calculation, the driver can adjust without the need for a > complicated table and should be flexible for different timings and > resolutions depending on the monitor. > > I don't have a DSI analyzer, and this appears to only work on > i.MX8M Plus but not Mini and Nano for some reason, despite their > having a similar DSI bridge. I just want to report that I have tested this patch (on top of current linux-next) on our Kontron BL i.MX8MM board with the ADV7535 bridge and I don't see any change when trying the 30 different modes the monitor reports as supported. 18 of those work and 12 don't work. So at least there is no negative impact in this case. Tested-by: Frieder Schrempf # Kontron BL i.MX8MM with HDMI monitor Thanks Frieder
Re: [PATCH 0/5] drm/bridge: samsung-dsim: fix various modes with ADV7535 bridge
Hi Michael, On 06.09.23 11:56, Michael Tretter wrote: > Hi Frieder, > > On Wed, 06 Sep 2023 11:31:45 +0200, Frieder Schrempf wrote: >> On 04.09.23 16:02, Frieder Schrempf wrote: >>> On 28.08.23 17:59, Michael Tretter wrote: >>>> I tested the i.MX8M Nano EVK with the NXP supplied MIPI-DSI adapter, >>>> which uses an ADV7535 MIPI-DSI to HDMI converter. I found that a few >>>> modes were working, but in many modes my monitor stayed dark. >>>> >>>> This series fixes the Samsung DSIM bridge driver to bring up a few more >>>> modes: >>>> >>>> The driver read the rate of the PLL ref clock only during probe. >>>> However, if the clock is re-parented to the VIDEO_PLL, changes to the >>>> pixel clock have an effect on the PLL ref clock. Therefore, the driver >>>> must read and potentially update the PLL ref clock on every modeset. >>>> >>>> I also found that the rounding mode of the porches and active area has >>>> an effect on the working modes. If the driver rounds up instead of >>>> rounding down and be calculates them in Hz instead of kHz, more modes >>>> start to work. >>>> >>>> The following table shows the modes that were working in my test without >>>> this patch set and the modes that are working now: >>>> >>>> |Mode | Before | Now | >>>> | 1920x1080-60.00 | X | X | >>>> | 1920x1080-59.94 || X | >>>> | 1920x1080-50.00 || X | >>>> | 1920x1080-30.00 || X | >>>> | 1920x1080-29.97 || X | >>>> | 1920x1080-25.00 || X | >>>> | 1920x1080-24.00 || | >>>> | 1920x1080-23.98 || | >>>> | 1680x1050-59.88 || X | >>>> | 1280x1024-75.03 | X | X | >>>> | 1280x1024-60.02 | X | X | >>>> | 1200x960-59.99 || X | >>>> | 1152x864-75.00 | X | X | >>>> | 1280x720-60.00 || | >>>> | 1280x720-59.94 || | >>>> | 1280x720-50.00 || X | >>>> | 1024x768-75.03 || X | >>>> | 1024x768-60.00 || X | >>>> | 800x600-75.00 | X | X | >>>> | 800x600-60.32 | X | X | >>>> | 720x576-50.00 | X | X | >>>> | 720x480-60.00 || | >>>> | 720x480-59.94 | X | | >>>> | 640x480-75.00 | X | X | >>>> | 640x480-60.00 || X | >>>> | 640x480-59.94 || X | >>>> | 720x400-70.08 || | >>>> >>>> Interestingly, the 720x480-59.94 mode stopped working. However, I am >>>> able to bring up the 720x480 modes by manually hacking the active area >>>> (hsa) to 40 and carefully adjusting the clocks, but something still >>>> seems to be off. >>>> >>>> Unfortunately, a few more modes are still not working at all. The NXP >>>> downstream kernel has some quirks to handle some of the modes especially >>>> wrt. to the porches, but I cannot figure out, what the driver should >>>> actually do in these cases. Maybe there is still an error in the >>>> calculation of the porches and someone at NXP can chime in. >>> >>> Thanks for working on this! We tested these patches with our Kontron BL >>> i.MX8MM board and a "10.1inch HDMI LCD (E)" display from Waveshare [1]. >>> >>> Without this series we don't get an image with the default mode of the >>> display (1024x600). With this series applied, it's now working. >> >> Minor correction: The display does work, but there is some flickering >> and occasional black screens if you let it run for some time. So there >> is still some sync issue. > > What is the parent of the dsi_phy_ref clock in your test case? It's the IMX8MM_CLK_24M default from imx8mm.dtsi. > Make sure that > the lcdif_pixel and dsi_phy_ref clocks are driven by the same PLL. > > I saw occasional black screens, if the lcdif_pixel and dsi_phy_ref clock had > different parents, and fixed it by changing the assigned-parent of > IMX8MN_CLK_DSI_PHY_REF to IMX8MN_VIDEO_PLL1_OUT in the device tree. If the > clocks are not in sync, it seems that the ADV3575 has issues correctly > reconstructing the pixel clock and HDMI monitors loose sync. > > This is something that must be configured in the board device tree, though. Ok, I tried to set the parent of IMX8MM_CLK_DSI_PHY_REF to IMX8MM_VIDEO_PLL1_OUT. I also noticed, that I forgot to remove the samsung,pll-clock-frequency from the mipi_dsi node in imx8mm.dtsi. This is necessary to make use of your PLL input clock improvements. Otherwise I get 23.76 MHz for the DSIM PLL input (derived from IMX8MM_VIDEO_PLL1_OUT) which results in a HS clock of 300.96 MHz while the display requests 301.5 MHz (50.25 MHz pixel clock). This is too low and the display doesn't work at all. Unfortunately all of this doesn't result in a more stable image on the 10" Waveshare display. It seems like the display turns black whenever the Qt app does a lot of framebuffer updates, e. g. during animations, etc. Anyway, thanks for the help! Best regards Frieder
Re: [PATCH 0/5] drm/bridge: samsung-dsim: fix various modes with ADV7535 bridge
On 04.09.23 16:02, Frieder Schrempf wrote: > Hi Michael, > > On 28.08.23 17:59, Michael Tretter wrote: >> I tested the i.MX8M Nano EVK with the NXP supplied MIPI-DSI adapter, >> which uses an ADV7535 MIPI-DSI to HDMI converter. I found that a few >> modes were working, but in many modes my monitor stayed dark. >> >> This series fixes the Samsung DSIM bridge driver to bring up a few more >> modes: >> >> The driver read the rate of the PLL ref clock only during probe. >> However, if the clock is re-parented to the VIDEO_PLL, changes to the >> pixel clock have an effect on the PLL ref clock. Therefore, the driver >> must read and potentially update the PLL ref clock on every modeset. >> >> I also found that the rounding mode of the porches and active area has >> an effect on the working modes. If the driver rounds up instead of >> rounding down and be calculates them in Hz instead of kHz, more modes >> start to work. >> >> The following table shows the modes that were working in my test without >> this patch set and the modes that are working now: >> >> |Mode | Before | Now | >> | 1920x1080-60.00 | X | X | >> | 1920x1080-59.94 || X | >> | 1920x1080-50.00 || X | >> | 1920x1080-30.00 || X | >> | 1920x1080-29.97 || X | >> | 1920x1080-25.00 || X | >> | 1920x1080-24.00 || | >> | 1920x1080-23.98 || | >> | 1680x1050-59.88 || X | >> | 1280x1024-75.03 | X | X | >> | 1280x1024-60.02 | X | X | >> | 1200x960-59.99 || X | >> | 1152x864-75.00 | X | X | >> | 1280x720-60.00 || | >> | 1280x720-59.94 || | >> | 1280x720-50.00 || X | >> | 1024x768-75.03 || X | >> | 1024x768-60.00 || X | >> | 800x600-75.00 | X | X | >> | 800x600-60.32 | X | X | >> | 720x576-50.00 | X | X | >> | 720x480-60.00 || | >> | 720x480-59.94 | X | | >> | 640x480-75.00 | X | X | >> | 640x480-60.00 || X | >> | 640x480-59.94 || X | >> | 720x400-70.08 || | >> >> Interestingly, the 720x480-59.94 mode stopped working. However, I am >> able to bring up the 720x480 modes by manually hacking the active area >> (hsa) to 40 and carefully adjusting the clocks, but something still >> seems to be off. >> >> Unfortunately, a few more modes are still not working at all. The NXP >> downstream kernel has some quirks to handle some of the modes especially >> wrt. to the porches, but I cannot figure out, what the driver should >> actually do in these cases. Maybe there is still an error in the >> calculation of the porches and someone at NXP can chime in. > > Thanks for working on this! We tested these patches with our Kontron BL > i.MX8MM board and a "10.1inch HDMI LCD (E)" display from Waveshare [1]. > > Without this series we don't get an image with the default mode of the > display (1024x600). With this series applied, it's now working. Minor correction: The display does work, but there is some flickering and occasional black screens if you let it run for some time. So there is still some sync issue. Anyway it's better than not working at all. > > For the whole series: > > Tested-by: Frieder Schrempf # Kontron BL > i.MX8MM + Waveshare 10.1inch HDMI LCD (E) > > Thanks > Frieder > > [1] https://www.waveshare.com/10.1inch-hdmi-lcd-e.htm
Re: [PATCH 0/5] drm/bridge: samsung-dsim: fix various modes with ADV7535 bridge
Hi Michael, On 28.08.23 17:59, Michael Tretter wrote: > I tested the i.MX8M Nano EVK with the NXP supplied MIPI-DSI adapter, > which uses an ADV7535 MIPI-DSI to HDMI converter. I found that a few > modes were working, but in many modes my monitor stayed dark. > > This series fixes the Samsung DSIM bridge driver to bring up a few more > modes: > > The driver read the rate of the PLL ref clock only during probe. > However, if the clock is re-parented to the VIDEO_PLL, changes to the > pixel clock have an effect on the PLL ref clock. Therefore, the driver > must read and potentially update the PLL ref clock on every modeset. > > I also found that the rounding mode of the porches and active area has > an effect on the working modes. If the driver rounds up instead of > rounding down and be calculates them in Hz instead of kHz, more modes > start to work. > > The following table shows the modes that were working in my test without > this patch set and the modes that are working now: > > |Mode | Before | Now | > | 1920x1080-60.00 | X | X | > | 1920x1080-59.94 || X | > | 1920x1080-50.00 || X | > | 1920x1080-30.00 || X | > | 1920x1080-29.97 || X | > | 1920x1080-25.00 || X | > | 1920x1080-24.00 || | > | 1920x1080-23.98 || | > | 1680x1050-59.88 || X | > | 1280x1024-75.03 | X | X | > | 1280x1024-60.02 | X | X | > | 1200x960-59.99 || X | > | 1152x864-75.00 | X | X | > | 1280x720-60.00 || | > | 1280x720-59.94 || | > | 1280x720-50.00 || X | > | 1024x768-75.03 || X | > | 1024x768-60.00 || X | > | 800x600-75.00 | X | X | > | 800x600-60.32 | X | X | > | 720x576-50.00 | X | X | > | 720x480-60.00 || | > | 720x480-59.94 | X | | > | 640x480-75.00 | X | X | > | 640x480-60.00 || X | > | 640x480-59.94 || X | > | 720x400-70.08 || | > > Interestingly, the 720x480-59.94 mode stopped working. However, I am > able to bring up the 720x480 modes by manually hacking the active area > (hsa) to 40 and carefully adjusting the clocks, but something still > seems to be off. > > Unfortunately, a few more modes are still not working at all. The NXP > downstream kernel has some quirks to handle some of the modes especially > wrt. to the porches, but I cannot figure out, what the driver should > actually do in these cases. Maybe there is still an error in the > calculation of the porches and someone at NXP can chime in. Thanks for working on this! We tested these patches with our Kontron BL i.MX8MM board and a "10.1inch HDMI LCD (E)" display from Waveshare [1]. Without this series we don't get an image with the default mode of the display (1024x600). With this series applied, it's now working. For the whole series: Tested-by: Frieder Schrempf # Kontron BL i.MX8MM + Waveshare 10.1inch HDMI LCD (E) Thanks Frieder [1] https://www.waveshare.com/10.1inch-hdmi-lcd-e.htm
Re: [PATCH v2 4/4] drm/bridge: imx: add driver for HDMI TX Parallel Video Interface
On 16.12.22 22:07, Lucas Stach wrote: > This IP block is found in the HDMI subsystem of the i.MX8MP SoC. It has a > full timing generator and can switch between different video sources. On > the i.MX8MP however the only supported source is the LCDIF. The block > just needs to be powered up and told about the polarity of the video > sync signals to act in bypass mode. > > Signed-off-by: Lucas Stach > Tested-by: Marek Vasut I tested this on our Kontron BL i.MX8MP board. Feel free to add: Tested-by: Frieder Schrempf Lucas, any chance that you can revive this series and bring it over the finish line?
Re: [PATCH v2 2/4] drm/bridge: imx: add bridge wrapper driver for i.MX8MP DWC HDMI
On 16.12.22 22:07, Lucas Stach wrote: > Add a simple wrapper driver for the DWC HDMI bridge driver that > implements the few bits that are necessary to abstract the i.MX8MP > SoC integration. > > Signed-off-by: Lucas Stach > Reviewed-by: Laurent Pinchart > Tested-by: Marek Vasut I tested this on our Kontron BL i.MX8MP board. Feel free to add: Tested-by: Frieder Schrempf
[PATCH v2] drm: bridge: samsung-dsim: Fix init during host transfer
From: Frieder Schrempf
In case the downstream bridge or panel uses DSI transfers before the
DSI host was actually initialized through samsung_dsim_atomic_enable()
which clears the stop state (LP11) mode, all transfers will fail.
This happens with downstream bridges that are controlled by DSI
commands such as the tc358762.
As documented in [1] DSI hosts are expected to allow transfers
outside the normal bridge enable/disable flow.
To fix this make sure that stop state is cleared in
samsung_dsim_host_transfer() which restores the previous
behavior.
We also factor out the common code to enable/disable stop state
to samsung_dsim_set_stop_state().
[1] https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
Fixes: 0c14d3130654 ("drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet
spec")
Reported-by: Tim Harvey
Signed-off-by: Frieder Schrempf
---
Changes for v2:
* Fix reversed stop state enable/disable in samsung_dsim_set_stop_state()
Hi Tim, could you please give this patch a try and report back if
it fixes your problem? Thanks!
---
drivers/gpu/drm/bridge/samsung-dsim.c | 27 +--
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
b/drivers/gpu/drm/bridge/samsung-dsim.c
index 043b8109e64a..73ec60757dbc 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -1386,6 +1386,18 @@ static void samsung_dsim_disable_irq(struct samsung_dsim
*dsi)
disable_irq(dsi->irq);
}
+static void samsung_dsim_set_stop_state(struct samsung_dsim *dsi, bool enable)
+{
+ u32 reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
+
+ if (enable)
+ reg |= DSIM_FORCE_STOP_STATE;
+ else
+ reg &= ~DSIM_FORCE_STOP_STATE;
+
+ samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
+}
+
static int samsung_dsim_init(struct samsung_dsim *dsi)
{
const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
@@ -1445,15 +1457,12 @@ static void samsung_dsim_atomic_enable(struct
drm_bridge *bridge,
struct drm_bridge_state
*old_bridge_state)
{
struct samsung_dsim *dsi = bridge_to_dsi(bridge);
- u32 reg;
if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
samsung_dsim_set_display_mode(dsi);
samsung_dsim_set_display_enable(dsi, true);
} else {
- reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
- reg &= ~DSIM_FORCE_STOP_STATE;
- samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
+ samsung_dsim_set_stop_state(dsi, false);
}
dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
@@ -1463,16 +1472,12 @@ static void samsung_dsim_atomic_disable(struct
drm_bridge *bridge,
struct drm_bridge_state
*old_bridge_state)
{
struct samsung_dsim *dsi = bridge_to_dsi(bridge);
- u32 reg;
if (!(dsi->state & DSIM_STATE_ENABLED))
return;
- if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
- reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
- reg |= DSIM_FORCE_STOP_STATE;
- samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
- }
+ if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
+ samsung_dsim_set_stop_state(dsi, true);
dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
}
@@ -1775,6 +1780,8 @@ static ssize_t samsung_dsim_host_transfer(struct
mipi_dsi_host *host,
if (ret)
return ret;
+ samsung_dsim_set_stop_state(dsi, false);
+
ret = mipi_dsi_create_packet(&xfer.packet, msg);
if (ret < 0)
return ret;
--
2.41.0
[PATCH] drm: bridge: samsung-dsim: Fix init during host transfer
From: Frieder Schrempf
In case the downstream bridge or panel uses DSI transfers before the
DSI host was actually initialized through samsung_dsim_atomic_enable()
which clears the stop state (LP11) mode, all transfers will fail.
This happens with downstream bridges that are controlled by DSI
commands such as the tc358762.
As documented in [1] DSI hosts are expected to allow transfers
outside the normal bridge enable/disable flow.
To fix this make sure that stop state is cleared in
samsung_dsim_host_transfer() which restores the previous
behavior.
We also factor out the common code to enable/disable stop state
to samsung_dsim_set_stop_state().
[1] https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
Fixes: 0c14d3130654 ("drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet
spec")
Reported-by: Tim Harvey
Signed-off-by: Frieder Schrempf
---
Hi Tim, could you please give this patch a try and report back if
it fixes your problem? Thanks!
---
drivers/gpu/drm/bridge/samsung-dsim.c | 27 +--
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
b/drivers/gpu/drm/bridge/samsung-dsim.c
index 043b8109e64a..92ea60e0fbf1 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -1386,6 +1386,18 @@ static void samsung_dsim_disable_irq(struct samsung_dsim
*dsi)
disable_irq(dsi->irq);
}
+static void samsung_dsim_set_stop_state(struct samsung_dsim *dsi, bool enable)
+{
+ u32 reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
+
+ if (enable)
+ reg &= ~DSIM_FORCE_STOP_STATE;
+ else
+ reg |= DSIM_FORCE_STOP_STATE;
+
+ samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
+}
+
static int samsung_dsim_init(struct samsung_dsim *dsi)
{
const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
@@ -1445,15 +1457,12 @@ static void samsung_dsim_atomic_enable(struct
drm_bridge *bridge,
struct drm_bridge_state
*old_bridge_state)
{
struct samsung_dsim *dsi = bridge_to_dsi(bridge);
- u32 reg;
if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
samsung_dsim_set_display_mode(dsi);
samsung_dsim_set_display_enable(dsi, true);
} else {
- reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
- reg &= ~DSIM_FORCE_STOP_STATE;
- samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
+ samsung_dsim_set_stop_state(dsi, false);
}
dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
@@ -1463,16 +1472,12 @@ static void samsung_dsim_atomic_disable(struct
drm_bridge *bridge,
struct drm_bridge_state
*old_bridge_state)
{
struct samsung_dsim *dsi = bridge_to_dsi(bridge);
- u32 reg;
if (!(dsi->state & DSIM_STATE_ENABLED))
return;
- if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
- reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
- reg |= DSIM_FORCE_STOP_STATE;
- samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
- }
+ if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
+ samsung_dsim_set_stop_state(dsi, true);
dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
}
@@ -1775,6 +1780,8 @@ static ssize_t samsung_dsim_host_transfer(struct
mipi_dsi_host *host,
if (ret)
return ret;
+ samsung_dsim_set_stop_state(dsi, false);
+
ret = mipi_dsi_create_packet(&xfer.packet, msg);
if (ret < 0)
return ret;
--
2.41.0
Re: [PATCH v4] drm: adv7511: Fix low refresh rate register for ADV7533/5
On 19.07.23 08:01, Alexandru Ardelean wrote:
> From: Bogdan Togorean
>
> For ADV7533 and ADV7535 low refresh rate is selected using
> bits [3:2] of 0x4a main register.
> So depending on ADV model write 0xfb or 0x4a register.
>
> Fixes: 2437e7cd88e8 ("drm/bridge: adv7533: Initial support for ADV7533")
> Reviewed-by: Robert Foss
> Reviewed-by: Nuno Sa
> Signed-off-by: Bogdan Togorean
> Signed-off-by: Alexandru Ardelean
Reviewed-by: Frieder Schrempf
Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec
On 19.07.23 18:34, Tim Harvey wrote:
> On Wed, Jul 19, 2023 at 12:05 AM Frieder Schrempf
> wrote:
>>
>> Hi Tim,
>>
>> On 19.07.23 01:03, Tim Harvey wrote:
>>> On Thu, Jul 13, 2023 at 3:01 AM Frieder Schrempf
>>> wrote:
>>>>
>>>> Hi Tim,
>>>>
>>>> On 13.07.23 09:18, Frieder Schrempf wrote:
>>>>> Hi Tim,
>>>>>
>>>>> On 13.07.23 00:34, Tim Harvey wrote:
>>>>>> On Wed, May 3, 2023 at 9:33 AM Frieder Schrempf wrote:
>>>>>>>
>>>>>>> From: Frieder Schrempf
>>>>>>>
>>>>>>> According to the documentation [1] the proper enable flow is:
>>>>>>>
>>>>>>> 1. Enable DSI link and keep data lanes in LP-11 (stop state)
>>>>>>> 2. Disable stop state to bring data lanes into HS mode
>>>>>>>
>>>>>>> Currently we do this all at once within enable(), which doesn't
>>>>>>> allow to meet the requirements of some downstream bridges.
>>>>>>>
>>>>>>> To fix this we now enable the DSI in pre_enable() and force it
>>>>>>> into stop state using the FORCE_STOP_STATE bit in the ESCMODE
>>>>>>> register until enable() is called where we reset the bit.
>>>>>>>
>>>>>>> We currently do this only for i.MX8M as Exynos uses a different
>>>>>>> init flow where samsung_dsim_init() is called from
>>>>>>> samsung_dsim_host_transfer().
>>>>>>>
>>>>>>> [1]
>>>>>>> https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
>>>>>>>
>>>>>>> Signed-off-by: Frieder Schrempf
>>>>>>> ---
>>>>>>> Changes for v2:
>>>>>>> * Drop RFC
>>>>>>> ---
>>>>>>> drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++--
>>>>>>> 1 file changed, 23 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>>>> b/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>>>> index e0a402a85787..9775779721d9 100644
>>>>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>>>> @@ -859,6 +859,10 @@ static int samsung_dsim_init_link(struct
>>>>>>> samsung_dsim *dsi)
>>>>>>> reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>>>>>>> reg &= ~DSIM_STOP_STATE_CNT_MASK;
>>>>>>> reg |=
>>>>>>> DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
>>>>>>> +
>>>>>>> + if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
>>>>>>> + reg |= DSIM_FORCE_STOP_STATE;
>>>>>>> +
>>>>>>> samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>>>>>>>
>>>>>>> reg = DSIM_BTA_TIMEOUT(0xff) | DSIM_LPDR_TIMEOUT(0x);
>>>>>>> @@ -1340,6 +1344,9 @@ static void samsung_dsim_atomic_pre_enable(struct
>>>>>>> drm_bridge *bridge,
>>>>>>> ret = samsung_dsim_init(dsi);
>>>>>>> if (ret)
>>>>>>> return;
>>>>>>> +
>>>>>>> + samsung_dsim_set_display_mode(dsi);
>>>>>>> + samsung_dsim_set_display_enable(dsi, true);
>>>>>>> }
>>>>>>> }
>>>>>>>
>>>>>>> @@ -1347,9 +1354,16 @@ static void samsung_dsim_atomic_enable(struct
>>>>>>> drm_bridge *bridge,
>>>>>>>struct drm_bridge_state
>>>>>>> *old_bridge_state)
>>>>>>> {
>>>>>>> struct samsung_dsim *dsi = bridge_to_dsi(bridge);
>>>>>>> + u32 reg;
>>>>>>>
>>>>>>> - samsung_dsim_set_display_mode(dsi);
>>>>>>> - samsung_dsim_set_display_enable(dsi, true);
>>>>>>> + if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw
Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec
Hi Tim,
On 19.07.23 01:03, Tim Harvey wrote:
> On Thu, Jul 13, 2023 at 3:01 AM Frieder Schrempf
> wrote:
>>
>> Hi Tim,
>>
>> On 13.07.23 09:18, Frieder Schrempf wrote:
>>> Hi Tim,
>>>
>>> On 13.07.23 00:34, Tim Harvey wrote:
>>>> On Wed, May 3, 2023 at 9:33 AM Frieder Schrempf wrote:
>>>>>
>>>>> From: Frieder Schrempf
>>>>>
>>>>> According to the documentation [1] the proper enable flow is:
>>>>>
>>>>> 1. Enable DSI link and keep data lanes in LP-11 (stop state)
>>>>> 2. Disable stop state to bring data lanes into HS mode
>>>>>
>>>>> Currently we do this all at once within enable(), which doesn't
>>>>> allow to meet the requirements of some downstream bridges.
>>>>>
>>>>> To fix this we now enable the DSI in pre_enable() and force it
>>>>> into stop state using the FORCE_STOP_STATE bit in the ESCMODE
>>>>> register until enable() is called where we reset the bit.
>>>>>
>>>>> We currently do this only for i.MX8M as Exynos uses a different
>>>>> init flow where samsung_dsim_init() is called from
>>>>> samsung_dsim_host_transfer().
>>>>>
>>>>> [1]
>>>>> https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
>>>>>
>>>>> Signed-off-by: Frieder Schrempf
>>>>> ---
>>>>> Changes for v2:
>>>>> * Drop RFC
>>>>> ---
>>>>> drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++--
>>>>> 1 file changed, 23 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>> b/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>> index e0a402a85787..9775779721d9 100644
>>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>> @@ -859,6 +859,10 @@ static int samsung_dsim_init_link(struct
>>>>> samsung_dsim *dsi)
>>>>> reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>>>>> reg &= ~DSIM_STOP_STATE_CNT_MASK;
>>>>> reg |=
>>>>> DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
>>>>> +
>>>>> + if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
>>>>> + reg |= DSIM_FORCE_STOP_STATE;
>>>>> +
>>>>> samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>>>>>
>>>>> reg = DSIM_BTA_TIMEOUT(0xff) | DSIM_LPDR_TIMEOUT(0x);
>>>>> @@ -1340,6 +1344,9 @@ static void samsung_dsim_atomic_pre_enable(struct
>>>>> drm_bridge *bridge,
>>>>> ret = samsung_dsim_init(dsi);
>>>>> if (ret)
>>>>> return;
>>>>> +
>>>>> + samsung_dsim_set_display_mode(dsi);
>>>>> + samsung_dsim_set_display_enable(dsi, true);
>>>>> }
>>>>> }
>>>>>
>>>>> @@ -1347,9 +1354,16 @@ static void samsung_dsim_atomic_enable(struct
>>>>> drm_bridge *bridge,
>>>>>struct drm_bridge_state
>>>>> *old_bridge_state)
>>>>> {
>>>>> struct samsung_dsim *dsi = bridge_to_dsi(bridge);
>>>>> + u32 reg;
>>>>>
>>>>> - samsung_dsim_set_display_mode(dsi);
>>>>> - samsung_dsim_set_display_enable(dsi, true);
>>>>> + if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
>>>>> + samsung_dsim_set_display_mode(dsi);
>>>>> + samsung_dsim_set_display_enable(dsi, true);
>>>>> + } else {
>>>>> + reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>>>>> + reg &= ~DSIM_FORCE_STOP_STATE;
>>>>> + samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>>>>> + }
>>>>>
>>>>> dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
>>>>> }
>>>>> @@ -1358,10 +1372,17 @@ static void samsung_dsim_atomic_disable(struct
>>>>> drm_bridge *bridge,
>>>>>
Re: [PATCH 1/1] drm/bridge: Fix handling of bridges with pre_enable_prev_first flag
On 14.07.23 19:16, Dave Stevenson wrote:
> Hi Frieder
>
> On Mon, 10 Jul 2023 at 08:46, Frieder Schrempf
> wrote:
>>
>> On 07.07.23 21:00, Vladimir Lypak wrote:
>>> [Sie erhalten nicht häufig E-Mails von vladimir.ly...@gmail.com. Weitere
>>> Informationen, warum dies wichtig ist, finden Sie unter
>>> https://aka.ms/LearnAboutSenderIdentification ]
>>>
>>> In function drm_atomic_bridge_chain_post_disable handling of
>>> pre_enable_prev_first flag is broken because "next" variable will always
>>> end up set to value of "bridge". This breaks loop which should disable
>>> bridges in reverse:
>>>
>>> next = list_next_entry(bridge, chain_node);
>>>
>>> if (next->pre_enable_prev_first) {
>>> /* next bridge had requested that prev
>>> * was enabled first, so disabled last
>>> */
>>> limit = next;
>>>
>>> /* Find the next bridge that has NOT requested
>>> * prev to be enabled first / disabled last
>>> */
>>> list_for_each_entry_from(next, &encoder->bridge_chain,
>>> chain_node) {
>>> // Next condition is always true. It is likely meant to be inversed
>>> // according to comment above. But doing this uncovers another problem:
>>> // it won't work if there are few bridges with this flag set at the end.
>>> if (next->pre_enable_prev_first) {
>>> next = list_prev_entry(next, chain_node);
>>> limit = next;
>>> // Here we always set next = limit = branch at first iteration.
>>> break;
>>> }
>>> }
>>>
>>> /* Call these bridges in reverse order */
>>> list_for_each_entry_from_reverse(next, &encoder->bridge_chain,
>>> chain_node) {
>>> // This loop never executes past this branch.
>>> if (next == bridge)
>>> break;
>>>
>>> drm_atomic_bridge_call_post_disable(next, old_state);
>>>
>>> In this patch logic for handling the flag is simplified. Temporary
>>> "iter" variable is introduced instead of "next" which is used only
>>> inside inner loops.
>>>
>>> Fixes: 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter
>>> bridge init order")
>>> Signed-off-by: Vladimir Lypak
>>
>> I haven't had a chance to look at this, but I still want to reference
>> another patch by Jagan that intends to fix some bug in this area:
>>
>> https://patchwork.kernel.org/project/dri-devel/patch/20230328170752.1102347-1-ja...@amarulasolutions.com/
>>
>> +Cc: Jagan
>>
>> Dave, as you introduced this feature, did you have a chance to look at
>> Jagan's and Vladimir's patches?
>
> Sorry, they'd fallen off my radar.
> I'm having a look at the moment, but will probably need to carry it
> over to Monday.
Sure, take your time. I just wanted to make sure that you are aware of
these patches and the existence of a bug in the code.
Thanks!
Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec
Hi Tim,
On 13.07.23 09:18, Frieder Schrempf wrote:
> Hi Tim,
>
> On 13.07.23 00:34, Tim Harvey wrote:
>> On Wed, May 3, 2023 at 9:33 AM Frieder Schrempf wrote:
>>>
>>> From: Frieder Schrempf
>>>
>>> According to the documentation [1] the proper enable flow is:
>>>
>>> 1. Enable DSI link and keep data lanes in LP-11 (stop state)
>>> 2. Disable stop state to bring data lanes into HS mode
>>>
>>> Currently we do this all at once within enable(), which doesn't
>>> allow to meet the requirements of some downstream bridges.
>>>
>>> To fix this we now enable the DSI in pre_enable() and force it
>>> into stop state using the FORCE_STOP_STATE bit in the ESCMODE
>>> register until enable() is called where we reset the bit.
>>>
>>> We currently do this only for i.MX8M as Exynos uses a different
>>> init flow where samsung_dsim_init() is called from
>>> samsung_dsim_host_transfer().
>>>
>>> [1]
>>> https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
>>>
>>> Signed-off-by: Frieder Schrempf
>>> ---
>>> Changes for v2:
>>> * Drop RFC
>>> ---
>>> drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++--
>>> 1 file changed, 23 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
>>> b/drivers/gpu/drm/bridge/samsung-dsim.c
>>> index e0a402a85787..9775779721d9 100644
>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>>> @@ -859,6 +859,10 @@ static int samsung_dsim_init_link(struct samsung_dsim
>>> *dsi)
>>> reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>>> reg &= ~DSIM_STOP_STATE_CNT_MASK;
>>> reg |= DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
>>> +
>>> + if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
>>> + reg |= DSIM_FORCE_STOP_STATE;
>>> +
>>> samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>>>
>>> reg = DSIM_BTA_TIMEOUT(0xff) | DSIM_LPDR_TIMEOUT(0x);
>>> @@ -1340,6 +1344,9 @@ static void samsung_dsim_atomic_pre_enable(struct
>>> drm_bridge *bridge,
>>> ret = samsung_dsim_init(dsi);
>>> if (ret)
>>> return;
>>> +
>>> + samsung_dsim_set_display_mode(dsi);
>>> + samsung_dsim_set_display_enable(dsi, true);
>>> }
>>> }
>>>
>>> @@ -1347,9 +1354,16 @@ static void samsung_dsim_atomic_enable(struct
>>> drm_bridge *bridge,
>>>struct drm_bridge_state
>>> *old_bridge_state)
>>> {
>>> struct samsung_dsim *dsi = bridge_to_dsi(bridge);
>>> + u32 reg;
>>>
>>> - samsung_dsim_set_display_mode(dsi);
>>> - samsung_dsim_set_display_enable(dsi, true);
>>> + if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
>>> + samsung_dsim_set_display_mode(dsi);
>>> + samsung_dsim_set_display_enable(dsi, true);
>>> + } else {
>>> + reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>>> + reg &= ~DSIM_FORCE_STOP_STATE;
>>> + samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>>> + }
>>>
>>> dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
>>> }
>>> @@ -1358,10 +1372,17 @@ static void samsung_dsim_atomic_disable(struct
>>> drm_bridge *bridge,
>>> struct drm_bridge_state
>>> *old_bridge_state)
>>> {
>>> struct samsung_dsim *dsi = bridge_to_dsi(bridge);
>>> + u32 reg;
>>>
>>> if (!(dsi->state & DSIM_STATE_ENABLED))
>>> return;
>>>
>>> + if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
>>> + reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>>> + reg |= DSIM_FORCE_STOP_STATE;
>>> + samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>>> + }
>>> +
>>> dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
>>> }
>>>
>>> --
>>> 2.40.0
>>>
>>
>> Hi Frieder,
>>
>> I found thi
Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec
Hi Tim,
On 13.07.23 00:34, Tim Harvey wrote:
> On Wed, May 3, 2023 at 9:33 AM Frieder Schrempf wrote:
>>
>> From: Frieder Schrempf
>>
>> According to the documentation [1] the proper enable flow is:
>>
>> 1. Enable DSI link and keep data lanes in LP-11 (stop state)
>> 2. Disable stop state to bring data lanes into HS mode
>>
>> Currently we do this all at once within enable(), which doesn't
>> allow to meet the requirements of some downstream bridges.
>>
>> To fix this we now enable the DSI in pre_enable() and force it
>> into stop state using the FORCE_STOP_STATE bit in the ESCMODE
>> register until enable() is called where we reset the bit.
>>
>> We currently do this only for i.MX8M as Exynos uses a different
>> init flow where samsung_dsim_init() is called from
>> samsung_dsim_host_transfer().
>>
>> [1]
>> https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
>>
>> Signed-off-by: Frieder Schrempf
>> ---
>> Changes for v2:
>> * Drop RFC
>> ---
>> drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++--
>> 1 file changed, 23 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
>> b/drivers/gpu/drm/bridge/samsung-dsim.c
>> index e0a402a85787..9775779721d9 100644
>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>> @@ -859,6 +859,10 @@ static int samsung_dsim_init_link(struct samsung_dsim
>> *dsi)
>> reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>> reg &= ~DSIM_STOP_STATE_CNT_MASK;
>> reg |= DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
>> +
>> + if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
>> + reg |= DSIM_FORCE_STOP_STATE;
>> +
>> samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>>
>> reg = DSIM_BTA_TIMEOUT(0xff) | DSIM_LPDR_TIMEOUT(0x);
>> @@ -1340,6 +1344,9 @@ static void samsung_dsim_atomic_pre_enable(struct
>> drm_bridge *bridge,
>> ret = samsung_dsim_init(dsi);
>> if (ret)
>> return;
>> +
>> + samsung_dsim_set_display_mode(dsi);
>> + samsung_dsim_set_display_enable(dsi, true);
>> }
>> }
>>
>> @@ -1347,9 +1354,16 @@ static void samsung_dsim_atomic_enable(struct
>> drm_bridge *bridge,
>>struct drm_bridge_state
>> *old_bridge_state)
>> {
>> struct samsung_dsim *dsi = bridge_to_dsi(bridge);
>> + u32 reg;
>>
>> - samsung_dsim_set_display_mode(dsi);
>> - samsung_dsim_set_display_enable(dsi, true);
>> + if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
>> + samsung_dsim_set_display_mode(dsi);
>> + samsung_dsim_set_display_enable(dsi, true);
>> + } else {
>> + reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>> + reg &= ~DSIM_FORCE_STOP_STATE;
>> + samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>> + }
>>
>> dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
>> }
>> @@ -1358,10 +1372,17 @@ static void samsung_dsim_atomic_disable(struct
>> drm_bridge *bridge,
>> struct drm_bridge_state
>> *old_bridge_state)
>> {
>> struct samsung_dsim *dsi = bridge_to_dsi(bridge);
>> + u32 reg;
>>
>> if (!(dsi->state & DSIM_STATE_ENABLED))
>> return;
>>
>> + if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
>> + reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>> + reg |= DSIM_FORCE_STOP_STATE;
>> + samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>> + }
>> +
>> dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
>> }
>>
>> --
>> 2.40.0
>>
>
> Hi Frieder,
>
> I found this patch to break mipi-dsi display on my board which has:
> - FocalTech FT5406 10pt touch controller (with no interrupt)
> - Powertip PH800480T013-IDF02 compatible panel
> - Toshiba TC358762 compatible DSI to DBI bridge
> - ATTINY based regulator used for backlight controller and panel enable
>
> I enable this via a dt overlay in a pending patch
> imx8mm-venice-gw72xx-0x-rpidsi.dtso [1] which works on 6.4 but not
> 6.5-rc1 which has this patch.
>
> T
Re: [PATCH 1/1] drm/bridge: Fix handling of bridges with pre_enable_prev_first flag
On 07.07.23 21:00, Vladimir Lypak wrote:
> [Sie erhalten nicht häufig E-Mails von vladimir.ly...@gmail.com. Weitere
> Informationen, warum dies wichtig ist, finden Sie unter
> https://aka.ms/LearnAboutSenderIdentification ]
>
> In function drm_atomic_bridge_chain_post_disable handling of
> pre_enable_prev_first flag is broken because "next" variable will always
> end up set to value of "bridge". This breaks loop which should disable
> bridges in reverse:
>
> next = list_next_entry(bridge, chain_node);
>
> if (next->pre_enable_prev_first) {
> /* next bridge had requested that prev
> * was enabled first, so disabled last
> */
> limit = next;
>
> /* Find the next bridge that has NOT requested
> * prev to be enabled first / disabled last
> */
> list_for_each_entry_from(next, &encoder->bridge_chain,
> chain_node) {
> // Next condition is always true. It is likely meant to be inversed
> // according to comment above. But doing this uncovers another problem:
> // it won't work if there are few bridges with this flag set at the end.
> if (next->pre_enable_prev_first) {
> next = list_prev_entry(next, chain_node);
> limit = next;
> // Here we always set next = limit = branch at first iteration.
> break;
> }
> }
>
> /* Call these bridges in reverse order */
> list_for_each_entry_from_reverse(next, &encoder->bridge_chain,
> chain_node) {
> // This loop never executes past this branch.
> if (next == bridge)
> break;
>
> drm_atomic_bridge_call_post_disable(next, old_state);
>
> In this patch logic for handling the flag is simplified. Temporary
> "iter" variable is introduced instead of "next" which is used only
> inside inner loops.
>
> Fixes: 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter
> bridge init order")
> Signed-off-by: Vladimir Lypak
I haven't had a chance to look at this, but I still want to reference
another patch by Jagan that intends to fix some bug in this area:
https://patchwork.kernel.org/project/dri-devel/patch/20230328170752.1102347-1-ja...@amarulasolutions.com/
+Cc: Jagan
Dave, as you introduced this feature, did you have a chance to look at
Jagan's and Vladimir's patches?
Re: [PATCH v2 2/2] drm/bridge: ti-sn65dsi83: Fix enable/disable flow to meet spec
On 17.05.23 00:22, Fabio Estevam wrote:
> On Thu, May 4, 2023 at 6:12 AM Alexander Stein
> wrote:
>>
>> Am Mittwoch, 3. Mai 2023, 18:33:07 CEST schrieb Frieder Schrempf:
>>> From: Frieder Schrempf
>>>
>>> The datasheet describes the following initialization flow including
>>> minimum delay times between each step:
>>>
>>> 1. DSI data lanes need to be in LP-11 and the clock lane in HS mode
>>> 2. toggle EN signal
>>> 3. initialize registers
>>> 4. enable PLL
>>> 5. soft reset
>>> 6. enable DSI stream
>>> 7. check error status register
>>>
>>> To meet this requirement we need to make sure the host bridge's
>>> pre_enable() is called first by using the pre_enable_prev_first
>>> flag.
>>>
>>> Furthermore we need to split enable() into pre_enable() which covers
>>> steps 2-5 from above and enable() which covers step 7 and is called
>>> after the host bridge's enable().
>>>
>>> Signed-off-by: Frieder Schrempf
>>
>> Tested-by: Alexander Stein
>> #TQMa8MxML/MBa8Mx
>
> Should this have a Fixes tag so that it could be backported to stable kernels?
As this depends on the support for the pre_enable_prev_first flag,
currently the only candidates for backporting would be 6.3 and 6.4.
I can't tell if there are DSI host drivers which already implement the
proper init flow and would benefit from a backport.
Anyway, it shouldn't be a problem either so I guess the proper tags
would look like:
Cc: # 6.3.x, 6.4.x
Fixes: ceb515ba29ba ("drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 and
SN65DSI84 driver")
[PATCH v2 2/2] drm/bridge: ti-sn65dsi83: Fix enable/disable flow to meet spec
From: Frieder Schrempf
The datasheet describes the following initialization flow including
minimum delay times between each step:
1. DSI data lanes need to be in LP-11 and the clock lane in HS mode
2. toggle EN signal
3. initialize registers
4. enable PLL
5. soft reset
6. enable DSI stream
7. check error status register
To meet this requirement we need to make sure the host bridge's
pre_enable() is called first by using the pre_enable_prev_first
flag.
Furthermore we need to split enable() into pre_enable() which covers
steps 2-5 from above and enable() which covers step 7 and is called
after the host bridge's enable().
Signed-off-by: Frieder Schrempf
---
Changes for v2:
* Drop RFC
---
drivers/gpu/drm/bridge/ti-sn65dsi83.c | 19 ---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index 75286c9afbb9..a82f10b8109f 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -321,8 +321,8 @@ static u8 sn65dsi83_get_dsi_div(struct sn65dsi83 *ctx)
return dsi_div - 1;
}
-static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
- struct drm_bridge_state *old_bridge_state)
+static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
+ struct drm_bridge_state
*old_bridge_state)
{
struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
struct drm_atomic_state *state = old_bridge_state->base.state;
@@ -484,11 +484,22 @@ static void sn65dsi83_atomic_enable(struct drm_bridge
*bridge,
/* Trigger reset after CSR register update. */
regmap_write(ctx->regmap, REG_RC_RESET, REG_RC_RESET_SOFT_RESET);
+ /* Wait for 10ms after soft reset as specified in datasheet */
+ usleep_range(1, 12000);
+}
+
+static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
+ struct drm_bridge_state *old_bridge_state)
+{
+ struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
+ unsigned int pval;
+
/* Clear all errors that got asserted during initialization. */
regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
regmap_write(ctx->regmap, REG_IRQ_STAT, pval);
- usleep_range(1, 12000);
+ /* Wait for 1ms and check for errors in status register */
+ usleep_range(1000, 1100);
regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
if (pval)
dev_err(ctx->dev, "Unexpected link status 0x%02x\n", pval);
@@ -555,6 +566,7 @@ static const struct drm_bridge_funcs sn65dsi83_funcs = {
.attach = sn65dsi83_attach,
.detach = sn65dsi83_detach,
.atomic_enable = sn65dsi83_atomic_enable,
+ .atomic_pre_enable = sn65dsi83_atomic_pre_enable,
.atomic_disable = sn65dsi83_atomic_disable,
.mode_valid = sn65dsi83_mode_valid,
@@ -697,6 +709,7 @@ static int sn65dsi83_probe(struct i2c_client *client)
ctx->bridge.funcs = &sn65dsi83_funcs;
ctx->bridge.of_node = dev->of_node;
+ ctx->bridge.pre_enable_prev_first = true;
drm_bridge_add(&ctx->bridge);
ret = sn65dsi83_host_attach(ctx);
--
2.40.0
[PATCH v2 1/2] drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec
From: Frieder Schrempf
According to the documentation [1] the proper enable flow is:
1. Enable DSI link and keep data lanes in LP-11 (stop state)
2. Disable stop state to bring data lanes into HS mode
Currently we do this all at once within enable(), which doesn't
allow to meet the requirements of some downstream bridges.
To fix this we now enable the DSI in pre_enable() and force it
into stop state using the FORCE_STOP_STATE bit in the ESCMODE
register until enable() is called where we reset the bit.
We currently do this only for i.MX8M as Exynos uses a different
init flow where samsung_dsim_init() is called from
samsung_dsim_host_transfer().
[1] https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
Signed-off-by: Frieder Schrempf
---
Changes for v2:
* Drop RFC
---
drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++--
1 file changed, 23 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
b/drivers/gpu/drm/bridge/samsung-dsim.c
index e0a402a85787..9775779721d9 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -859,6 +859,10 @@ static int samsung_dsim_init_link(struct samsung_dsim *dsi)
reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
reg &= ~DSIM_STOP_STATE_CNT_MASK;
reg |= DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
+
+ if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
+ reg |= DSIM_FORCE_STOP_STATE;
+
samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
reg = DSIM_BTA_TIMEOUT(0xff) | DSIM_LPDR_TIMEOUT(0x);
@@ -1340,6 +1344,9 @@ static void samsung_dsim_atomic_pre_enable(struct
drm_bridge *bridge,
ret = samsung_dsim_init(dsi);
if (ret)
return;
+
+ samsung_dsim_set_display_mode(dsi);
+ samsung_dsim_set_display_enable(dsi, true);
}
}
@@ -1347,9 +1354,16 @@ static void samsung_dsim_atomic_enable(struct drm_bridge
*bridge,
struct drm_bridge_state
*old_bridge_state)
{
struct samsung_dsim *dsi = bridge_to_dsi(bridge);
+ u32 reg;
- samsung_dsim_set_display_mode(dsi);
- samsung_dsim_set_display_enable(dsi, true);
+ if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
+ samsung_dsim_set_display_mode(dsi);
+ samsung_dsim_set_display_enable(dsi, true);
+ } else {
+ reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
+ reg &= ~DSIM_FORCE_STOP_STATE;
+ samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
+ }
dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
}
@@ -1358,10 +1372,17 @@ static void samsung_dsim_atomic_disable(struct
drm_bridge *bridge,
struct drm_bridge_state
*old_bridge_state)
{
struct samsung_dsim *dsi = bridge_to_dsi(bridge);
+ u32 reg;
if (!(dsi->state & DSIM_STATE_ENABLED))
return;
+ if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
+ reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
+ reg |= DSIM_FORCE_STOP_STATE;
+ samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
+ }
+
dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
}
--
2.40.0
[PATCH v2 0/2] Init flow fixes for Samsung DSIM and TI SN65DSI84
From: Frieder Schrempf This patchset contains a proposal to fix the initialization flow for the display pipeline used on our i.MX8MM Kontron boards: i.MX8MM LCDIF -> i.MX8MM DSIM -> TI SN65DSI84 -> 7" LVDS Panel Without these changes the display works most of the time, but fails to come up occassionally when booting or doing on/off cycling tests with: echo 0 > /sys/devices/platform/soc@0/32c0.bus/32e0.lcdif/graphics/fb0/blank echo 1 > /sys/devices/platform/soc@0/32c0.bus/32e0.lcdif/graphics/fb0/blank All the changes intend to follow the documentation provided here: https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation Changes for v2: * Drop RFC * Drop non-working Exynos cleanup patch 3/3 Frieder Schrempf (2): drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec drm/bridge: ti-sn65dsi83: Fix enable/disable flow to meet spec drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++-- drivers/gpu/drm/bridge/ti-sn65dsi83.c | 19 --- 2 files changed, 39 insertions(+), 5 deletions(-) -- 2.40.0
Re: [PATCH V3 7/7] drm: bridge: samsung-dsim: Let blanking calcuation work in non-burst mode
On 02.05.23 03:07, Adam Ford wrote:
> The blanking calculation currently uses burst_clk_rate for calculating
> the settings. Since it's possible to use this in non-burst mode, it's
> possible that where won't be burst_clk_rate. Instead, cache the
"possible that burst_clk_rate is 0"
> clock rate configured from of samsung_dsim_set_pll and use it instead.
>
> Signed-off-by: Adam Ford Tested-by: Chen-Yu Tsai
>
Maybe this patch should be squashed into patch 6/7 as otherwise
burst_clk_rate could be 0 here causing bisection issues?
Apart from that:
Tested on Kontron BL i.MX8MM with SN65DSI84 and ADV7535 bridges.
Tested-by: Frieder Schrempf
Reviewed-by: Frieder Schrempf
> ---
> drivers/gpu/drm/bridge/samsung-dsim.c | 4 +++-
> include/drm/bridge/samsung-dsim.h | 1 +
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 53099461cdc2..1dc913db2cb3 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -652,6 +652,8 @@ static unsigned long samsung_dsim_set_pll(struct
> samsung_dsim *dsi,
> reg = samsung_dsim_read(dsi, DSIM_STATUS_REG);
> } while ((reg & DSIM_PLL_STABLE) == 0);
>
> + dsi->hs_clock = fout;
> +
> return fout;
> }
>
> @@ -960,7 +962,7 @@ static void samsung_dsim_set_display_mode(struct
> samsung_dsim *dsi)
> u32 reg;
>
> if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO) {
> - int byte_clk_khz = dsi->burst_clk_rate / 1000 / 8;
> + int byte_clk_khz = dsi->hs_clock / 1000 / 8;> int hfp
> = (m->hsync_start - m->hdisplay) * byte_clk_khz / m->clock;
> int hbp = (m->htotal - m->hsync_end) * byte_clk_khz / m->clock;
> int hsa = (m->hsync_end - m->hsync_start) * byte_clk_khz /
> m->clock;
> diff --git a/include/drm/bridge/samsung-dsim.h
> b/include/drm/bridge/samsung-dsim.h
> index 76ea8a1720cc..14176e6e9040 100644
> --- a/include/drm/bridge/samsung-dsim.h
> +++ b/include/drm/bridge/samsung-dsim.h
> @@ -94,6 +94,7 @@ struct samsung_dsim {
>
> u32 pll_clk_rate;
> u32 burst_clk_rate;
> + u32 hs_clock;
> u32 esc_clk_rate;
> u32 lanes;
> u32 mode_flags;
Re: [PATCH V3 6/7] drm: bridge: samsung-dsim: Support non-burst mode
On 02.05.23 03:07, Adam Ford wrote: > The high-speed clock is hard-coded to the burst-clock > frequency specified in the device tree. However, when > using devices like certain bridge chips without burst mode > and varying resolutions and refresh rates, it may be > necessary to set the high-speed clock dynamically based > on the desired pixel clock for the connected device. > > This also removes the need to set a clock speed from > the device tree for non-burst mode operation, since the > pixel clock rate is the rate requested from the attached > device like an HDMI bridge chip. This should have no > impact for people using burst-mode and setting the burst > clock rate is still required for those users. > > Signed-off-by: Adam Ford > Tested-by: Chen-Yu Tsai Tested on Kontron BL i.MX8MM with SN65DSI84 and ADV7535 bridges. Tested-by: Frieder Schrempf Reviewed-by: Frieder Schrempf
Re: [PATCH V3 5/7] drm: bridge: samsung-dsim: Dynamically configure DPHY timing
On 02.05.23 03:07, Adam Ford wrote:
> The DPHY timings are currently hard coded. Since the input
> clock can be variable, the phy timings need to be variable
> too. Add an additional variable to the driver data to enable
> this feature to prevent breaking boards that don't support it.
>
> The phy_mipi_dphy_get_default_config function configures the
> DPHY timings in pico-seconds, and a small macro converts those
> timings into clock cycles based on the pixel clock rate.
>
> Signed-off-by: Adam Ford
> Tested-by: Chen-Yu Tsai
A few nitpicks below, otherwise:
Tested on Kontron BL i.MX8MM with SN65DSI84 and ADV7535 bridges.
Tested-by: Frieder Schrempf
Reviewed-by: Frieder Schrempf
> ---
> drivers/gpu/drm/bridge/samsung-dsim.c | 79 +++
> include/drm/bridge/samsung-dsim.h | 1 +
> 2 files changed, 70 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 2dc02a9e37c0..99642230a54a 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -18,9 +18,7 @@
> #include
> #include
> #include
> -
> #include
> -
Unrelated blank lines removed above!?
> #include
> #include
> #include
> @@ -218,6 +216,8 @@
>
> #define OLD_SCLK_MIPI_CLK_NAME "pll_clk"
>
> +#define PS_TO_CYCLE(PS, MHz) DIV64_U64_ROUND_CLOSEST(((PS) * (MHz)),
> 1ULL)
Should macro arguments PS and MHz better be all lower-case?
Also, MHz is actually in Hz, right? So it should be renamed.
> +
> static const char *const clk_names[5] = {
> "bus_clk",
> "sclk_mipi",
> @@ -487,6 +487,7 @@ static const struct samsung_dsim_driver_data
> imx8mm_dsi_driver_data = {
> .m_min = 64,
> .m_max = 1023,
> .min_freq = 1050,
> + .dynamic_dphy = 1,
> };
>
> static const struct samsung_dsim_driver_data *
> @@ -698,13 +699,50 @@ static void samsung_dsim_set_phy_ctrl(struct
> samsung_dsim *dsi)
> const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
> const unsigned int *reg_values = driver_data->reg_values;
> u32 reg;
> + struct drm_display_mode *m = &dsi->mode;
> + int bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
> + struct phy_configure_opts_mipi_dphy cfg;
> + int clk_prepare, lpx, clk_zero, clk_post, clk_trail;
> + int hs_exit, hs_prepare, hs_zero, hs_trail;
> + unsigned long long clock_in_hz = m->clock * 1000;
>
> if (driver_data->has_freqband)
> return;
>
> + /* The dynamic_phy has the ability to adjust PHY Timing settings */
> + if (driver_data->dynamic_dphy) {
> + phy_mipi_dphy_get_default_config(clock_in_hz, bpp, dsi->lanes,
> &cfg);
> +
> + /*
> + * TODO:
> + * The tech reference manual for i.MX8M Mini/Nano/Plus
> + * doesn't state what the definition of the PHYTIMING
> + * bits are beyond their address and bit position.
> + * After reviewing NXP's downstream code, it appears
> + * that the various PHYTIMING registers take the number
> + * of cycles and use various dividers on them. This
> + * calculation does not result in an exact match to the
> + * downstream code, but it is very close, and it appears
> + * to sync at a variety of resolutions. If someone
> + * can get a more accurate mathematical equation needed
> + * for these registers, this should be updated.
> + */
> +
> + lpx = PS_TO_CYCLE(cfg.lpx, clock_in_hz);
> + hs_exit = PS_TO_CYCLE(cfg.hs_exit, clock_in_hz);
> + clk_prepare = PS_TO_CYCLE(cfg.clk_prepare, clock_in_hz);
> + clk_zero = PS_TO_CYCLE(cfg.clk_zero, clock_in_hz);
> + clk_post = PS_TO_CYCLE(cfg.clk_post, clock_in_hz);
> + clk_trail = PS_TO_CYCLE(cfg.clk_trail, clock_in_hz);
> + hs_prepare = PS_TO_CYCLE(cfg.hs_prepare, clock_in_hz);
> + hs_zero = PS_TO_CYCLE(cfg.hs_zero, clock_in_hz);
> + hs_trail = PS_TO_CYCLE(cfg.hs_trail, clock_in_hz);
> + }
> +
> /* B D-PHY: D-PHY Master & Slave Analog Block control */
> reg = reg_values[PHYCTRL_ULPS_EXIT] | reg_values[PHYCTRL_VREG_LP] |
> reg_values[PHYCTRL_SLEW_UP];
> +
> samsung_dsim_write(dsi, DSIM_PHYCTRL_REG, reg);
>
> /*
> @@ -712,7 +750,11 @@ static void samsung_dsim_set_phy_ctrl(struct
> samsung_dsim *dsi)
&g
Re: [PATCH V3 4/7] drm: bridge: samsung-dsim: Select GENERIC_PHY_MIPI_DPHY
On 02.05.23 03:07, Adam Ford wrote: > In order to support variable DPHY timings, it's necessary > to enable GENERIC_PHY_MIPI_DPHY so phy_mipi_dphy_get_default_config > can be used to determine the nominal values for a given resolution > and refresh rate. > > Signed-off-by: Adam Ford This fixes the build error which existed in v2! Tested on Kontron BL i.MX8MM with SN65DSI84 and ADV7535 bridges. Tested-by: Frieder Schrempf Reviewed-by: Frieder Schrempf
Re: [PATCH V3 3/7] drm: bridge: samsung-dsim: Fetch pll-clock-frequency automatically
On 02.05.23 03:07, Adam Ford wrote: > Make the pll-clock-frequency optional. If it's present, use it > to maintain backwards compatibility with existing hardware. If it > is absent, read clock rate of "sclk_mipi" to determine the rate. > > Signed-off-by: Adam Ford > Tested-by: Chen-Yu Tsai Tested on Kontron BL i.MX8MM with SN65DSI84 and ADV7535 bridges. Tested-by: Frieder Schrempf Reviewed-by: Frieder Schrempf
Re: [PATCH V3 2/7] drm: bridge: samsung-dsim: Fix PMS Calculator on imx8m[mnp]
On 02.05.23 03:07, Adam Ford wrote:
> According to Table 13-45 of the i.MX8M Mini Reference Manual, the min
> and max values for M and the frequency range for the VCO_out
> calculator were incorrect. This information was contradicted in other
> parts of the mini, nano and plus manuals. After reaching out to my
> NXP Rep, when confronting him about discrepencies in the Nano manual,
> he responded with:
> "Yes it is definitely wrong, the one that is part
> of the NOTE in MIPI_DPHY_M_PLLPMS register table against PMS_P,
> PMS_M and PMS_S is not correct. I will report this to Doc team,
> the one customer should be take into account is the Table 13-40
> DPHY PLL Parameters and the Note above."
>
> These updated values also match what is used in the NXP downstream
> kernel.
>
> To fix this, make new variables to hold the min and max values of m
> and the minimum value of VCO_out, and update the PMS calculator to
> use these new variables instead of using hard-coded values to keep
> the backwards compatibility with other parts using this driver.
>
> Fixes: 4d562c70c4dc ("drm: bridge: samsung-dsim: Add i.MX8M Mini/Nano
> support")
> Signed-off-by: Adam Ford
> Reviewed-by: Lucas Stach
> Tested-by: Chen-Yu Tsai
Tested on Kontron BL i.MX8MM with SN65DSI84 and ADV7535 bridges.
Tested-by: Frieder Schrempf
Reviewed-by: Frieder Schrempf
Re: [PATCH V3 1/7] drm: bridge: samsung-dsim: fix blanking packet size calculation
On 02.05.23 03:07, Adam Ford wrote: > From: Lucas Stach > > Scale the blanking packet sizes to match the ratio between HS clock > and DPI interface clock. The controller seems to do internal scaling > to the number of active lanes, so we don't take those into account. > > Signed-off-by: Lucas Stach > Signed-off-by: Adam Ford > Tested-by: Chen-Yu Tsai Tested-by: Frieder Schrempf
Re: [RFC PATCH 3/3] drm: bridge: samsung-dsim: Remove init quirk for Exynos
On 18.04.23 15:12, Marek Szyprowski wrote: > On 18.04.2023 12:42, Frieder Schrempf wrote: >> From: Frieder Schrempf >> >> Assuming that with the init flow fixed to meet the documentation at >> [1] and the pre_enable_prev_first flag set in downstream bridge/panel >> drivers which require it, we can use the default flow for Exynos as >> already done for i.MX8M. >> >> [1] >> https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation >> >> Signed-off-by: Frieder Schrempf >> --- >> I have no idea if my assumptions are correct and if this works at all. >> There's a very good chance it doesn't... > > Unfortunately this change breaks all Exynos boards with DSI panels. I've > check all 4 panels that are in mainline and none worked. Ok, thanks for testing anyway! As already mentioned this was rather a shot in the dark, as I didn't even bother trying to fully understand what's going on on the Exynos side. For now I will just remove this patch from the series in the next iteration.
[RFC PATCH 3/3] drm: bridge: samsung-dsim: Remove init quirk for Exynos
From: Frieder Schrempf
Assuming that with the init flow fixed to meet the documentation at
[1] and the pre_enable_prev_first flag set in downstream bridge/panel
drivers which require it, we can use the default flow for Exynos as
already done for i.MX8M.
[1] https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
Signed-off-by: Frieder Schrempf
---
I have no idea if my assumptions are correct and if this works at all.
There's a very good chance it doesn't...
---
drivers/gpu/drm/bridge/samsung-dsim.c | 39 ---
1 file changed, 11 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
b/drivers/gpu/drm/bridge/samsung-dsim.c
index 9775779721d9..8c68b767ae50 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -1336,18 +1336,12 @@ static void samsung_dsim_atomic_pre_enable(struct
drm_bridge *bridge,
dsi->state |= DSIM_STATE_ENABLED;
- /*
-* For Exynos-DSIM the downstream bridge, or panel are expecting
-* the host initialization during DSI transfer.
-*/
- if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
- ret = samsung_dsim_init(dsi);
- if (ret)
- return;
+ ret = samsung_dsim_init(dsi);
+ if (ret)
+ return;
- samsung_dsim_set_display_mode(dsi);
- samsung_dsim_set_display_enable(dsi, true);
- }
+ samsung_dsim_set_display_mode(dsi);
+ samsung_dsim_set_display_enable(dsi, true);
}
static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
@@ -1356,14 +1350,9 @@ static void samsung_dsim_atomic_enable(struct drm_bridge
*bridge,
struct samsung_dsim *dsi = bridge_to_dsi(bridge);
u32 reg;
- if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
- samsung_dsim_set_display_mode(dsi);
- samsung_dsim_set_display_enable(dsi, true);
- } else {
- reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
- reg &= ~DSIM_FORCE_STOP_STATE;
- samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
- }
+ reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
+ reg &= ~DSIM_FORCE_STOP_STATE;
+ samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
}
@@ -1377,11 +1366,9 @@ static void samsung_dsim_atomic_disable(struct
drm_bridge *bridge,
if (!(dsi->state & DSIM_STATE_ENABLED))
return;
- if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
- reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
- reg |= DSIM_FORCE_STOP_STATE;
- samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
- }
+ reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
+ reg |= DSIM_FORCE_STOP_STATE;
+ samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
}
@@ -1680,10 +1667,6 @@ static ssize_t samsung_dsim_host_transfer(struct
mipi_dsi_host *host,
if (!(dsi->state & DSIM_STATE_ENABLED))
return -EINVAL;
- ret = samsung_dsim_init(dsi);
- if (ret)
- return ret;
-
ret = mipi_dsi_create_packet(&xfer.packet, msg);
if (ret < 0)
return ret;
--
2.40.0
[RFC PATCH 2/3] drm/bridge: ti-sn65dsi83: Fix enable/disable flow to meet spec
From: Frieder Schrempf
The datasheet describes the following initialization flow including
minimum delay times between each step:
1. DSI data lanes need to be in LP-11 and the clock lane in HS mode
2. toggle EN signal
3. initialize registers
4. enable PLL
5. soft reset
6. enable DSI stream
7. check error status register
To meet this requirement we need to make sure the host bridge's
pre_enable() is called first by using the pre_enable_prev_first
flag.
Furthermore we need to split enable() into pre_enable() which covers
steps 2-5 from above and enable() which covers step 7 and is called
after the host bridge's enable().
Signed-off-by: Frieder Schrempf
---
drivers/gpu/drm/bridge/ti-sn65dsi83.c | 19 ---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index 75286c9afbb9..a82f10b8109f 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -321,8 +321,8 @@ static u8 sn65dsi83_get_dsi_div(struct sn65dsi83 *ctx)
return dsi_div - 1;
}
-static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
- struct drm_bridge_state *old_bridge_state)
+static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
+ struct drm_bridge_state
*old_bridge_state)
{
struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
struct drm_atomic_state *state = old_bridge_state->base.state;
@@ -484,11 +484,22 @@ static void sn65dsi83_atomic_enable(struct drm_bridge
*bridge,
/* Trigger reset after CSR register update. */
regmap_write(ctx->regmap, REG_RC_RESET, REG_RC_RESET_SOFT_RESET);
+ /* Wait for 10ms after soft reset as specified in datasheet */
+ usleep_range(1, 12000);
+}
+
+static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
+ struct drm_bridge_state *old_bridge_state)
+{
+ struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
+ unsigned int pval;
+
/* Clear all errors that got asserted during initialization. */
regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
regmap_write(ctx->regmap, REG_IRQ_STAT, pval);
- usleep_range(1, 12000);
+ /* Wait for 1ms and check for errors in status register */
+ usleep_range(1000, 1100);
regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
if (pval)
dev_err(ctx->dev, "Unexpected link status 0x%02x\n", pval);
@@ -555,6 +566,7 @@ static const struct drm_bridge_funcs sn65dsi83_funcs = {
.attach = sn65dsi83_attach,
.detach = sn65dsi83_detach,
.atomic_enable = sn65dsi83_atomic_enable,
+ .atomic_pre_enable = sn65dsi83_atomic_pre_enable,
.atomic_disable = sn65dsi83_atomic_disable,
.mode_valid = sn65dsi83_mode_valid,
@@ -697,6 +709,7 @@ static int sn65dsi83_probe(struct i2c_client *client)
ctx->bridge.funcs = &sn65dsi83_funcs;
ctx->bridge.of_node = dev->of_node;
+ ctx->bridge.pre_enable_prev_first = true;
drm_bridge_add(&ctx->bridge);
ret = sn65dsi83_host_attach(ctx);
--
2.40.0
[RFC PATCH 1/3] drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec
From: Frieder Schrempf
According to the documentation [1] the proper enable flow is:
1. Enable DSI link and keep data lanes in LP-11 (stop state)
2. Disable stop state to bring data lanes into HS mode
Currently we do this all at once within enable(), which doesn't
allow to meet the requirements of some downstream bridges.
To fix this we now enable the DSI in pre_enable() and force it
into stop state using the FORCE_STOP_STATE bit in the ESCMODE
register until enable() is called where we reset the bit.
We currently do this only for i.MX8M as Exynos uses a different
init flow where samsung_dsim_init() is called from
samsung_dsim_host_transfer().
[1] https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
Signed-off-by: Frieder Schrempf
---
drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++--
1 file changed, 23 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
b/drivers/gpu/drm/bridge/samsung-dsim.c
index e0a402a85787..9775779721d9 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -859,6 +859,10 @@ static int samsung_dsim_init_link(struct samsung_dsim *dsi)
reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
reg &= ~DSIM_STOP_STATE_CNT_MASK;
reg |= DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
+
+ if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
+ reg |= DSIM_FORCE_STOP_STATE;
+
samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
reg = DSIM_BTA_TIMEOUT(0xff) | DSIM_LPDR_TIMEOUT(0x);
@@ -1340,6 +1344,9 @@ static void samsung_dsim_atomic_pre_enable(struct
drm_bridge *bridge,
ret = samsung_dsim_init(dsi);
if (ret)
return;
+
+ samsung_dsim_set_display_mode(dsi);
+ samsung_dsim_set_display_enable(dsi, true);
}
}
@@ -1347,9 +1354,16 @@ static void samsung_dsim_atomic_enable(struct drm_bridge
*bridge,
struct drm_bridge_state
*old_bridge_state)
{
struct samsung_dsim *dsi = bridge_to_dsi(bridge);
+ u32 reg;
- samsung_dsim_set_display_mode(dsi);
- samsung_dsim_set_display_enable(dsi, true);
+ if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
+ samsung_dsim_set_display_mode(dsi);
+ samsung_dsim_set_display_enable(dsi, true);
+ } else {
+ reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
+ reg &= ~DSIM_FORCE_STOP_STATE;
+ samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
+ }
dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
}
@@ -1358,10 +1372,17 @@ static void samsung_dsim_atomic_disable(struct
drm_bridge *bridge,
struct drm_bridge_state
*old_bridge_state)
{
struct samsung_dsim *dsi = bridge_to_dsi(bridge);
+ u32 reg;
if (!(dsi->state & DSIM_STATE_ENABLED))
return;
+ if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
+ reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
+ reg |= DSIM_FORCE_STOP_STATE;
+ samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
+ }
+
dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
}
--
2.40.0
[RFC PATCH 0/3] Init flow fixes for Samsung DSIM and TI SN65DSI84
From: Frieder Schrempf This patchset contains a proposal to fix the initialization flow for the display pipeline used on our i.MX8MM Kontron boards: i.MX8MM LCDIF -> i.MX8MM DSIM -> TI SN65DSI84 -> 7" LVDS Panel Without these changes the display works most of the time, but fails to come up occassionally when booting or doing on/off cycling tests with: echo 0 > /sys/devices/platform/soc@0/32c0.bus/32e0.lcdif/graphics/fb0/blank echo 1 > /sys/devices/platform/soc@0/32c0.bus/32e0.lcdif/graphics/fb0/blank I also added a bit of a speculative patch 3/3 for cleaning up the Exynos init flow, which is the main reason this is sent as RFC as I have no idea if this is correct/working at all. Frieder Schrempf (3): drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec drm/bridge: ti-sn65dsi83: Fix enable/disable flow to meet spec drm: bridge: samsung-dsim: Remove init quirk for Exynos drivers/gpu/drm/bridge/samsung-dsim.c | 34 +++ drivers/gpu/drm/bridge/ti-sn65dsi83.c | 19 --- 2 files changed, 35 insertions(+), 18 deletions(-) -- 2.40.0
Re: IMX8MM: assign panel to mipi_dsi in a device tree
On 05.03.23 20:45, Patrick Boettcher wrote:
> [Sie erhalten nicht häufig E-Mails von patrick.boettc...@posteo.de. Weitere
> Informationen, warum dies wichtig ist, finden Sie unter
> https://aka.ms/LearnAboutSenderIdentification ]
>
> On Mon, 6 Mar 2023 00:05:03 +0530
> Jagan Teki wrote:
>
>> On Sun, Mar 5, 2023 at 11:39 PM Patrick Boettcher
>> wrote:
>>>
>>> Hi list,
>>>
>>> After several days of trying I realize my too small/old brain is
>>> unable to map around how to assign/connect a panel to the
>>> mipi_dsi-node in a device.
>>>
>>> We are using a 'tdo,tl070wsh30' panel connected to the
>>> mipi-dsi-interface of a imx8mm.
>>>
>>> Of all the references I found on the in public repositories none of
>>> them is using this exact panel. Well.
>>>
>>> Looking at other device trees I came up with the following dts-node
>>> add to the mipi_dsi-node:
>>>
>>> &mipi_dsi {
>>> #address-cells = <1>;
>>> #size-cells = <0>;
>>> status = "okay";
>>>
>>> port@0 {
>>> reg = <0>;
>>> mipi_dsi_panel0_out: endpoint {
>>> remote-endpoint = <&panel0_in>;
>>> attach-bridge;
>>> };
>>> };
>>>
>>> panel@0 {
>>> compatible = "tdo,tl070wsh30";
>>> reg = <0>;
>>>
>>> pinctrl-0 = <&pinctrl_mipi_dsi>;
>>> pinctrl-names = "default";
>>> reset-gpios = <&gpio4 4 GPIO_ACTIVE_LOW>;
>>> enable-gpios = <&gpio4 6 GPIO_ACTIVE_LOW>;
>>>
>>> backlight = <&panel_gpio_backlight>;
>>> power-supply = <&panel_gpio_regulator>;
>>>
>>> dsi-lanes = <4>;
>>>
>>> video-mode = <0>;
>>>
>>> panel-width-mm = <157>;
>>> panel-height-mm = <86>;
>>>
>>> status = "okay";
>>>
>>> port {
>>> panel0_in: endpoint {
>>> remote-endpoint
>>> =<&mipi_dsi_panel0_out>; };
>>> };
>>> };
>>> };
>>>
>>>
>>> You'll see that I used the attach-bridge-option, which is maybe not
>>> necessary. I found this during a debug-print-session in the
>>> drm-bridge-driver, it wasn't attaching a bridge. But maybe I don't
>>> need a bridge as the panel-driver contains everything to control the
>>> controllers of the panel. I don't really know.
>>>
>>> However, with this I have the following messages:
>>>
>>> [0.393985] [drm:drm_bridge_attach] *ERROR* failed to attach
>>> bridge /soc@0/bus@32c0/mipi_dsi@32e1 to encoder DSI-34: -19
>>> [0.405626] imx_sec_dsim_drv 32e1.mipi_dsi: Failed to attach
>>> bridge: 32e1.mipi_dsi [0.413974] imx_sec_dsim_drv
>>> 32e1.mipi_dsi: failed to bind sec dsim bridge: -517
>>>
>>> The panel driver is never instantiated.
>>>
>>> I'm using 5.15.51 (-imx). mipi_dsi and the panel-driver are built
>>> into the kernel.
>>
>> Please share the source repo link?
>
> I'm using this one (via yocto's recipe):
>
> github.com/nxp-imx/linux-imx.git, branch lf-5.15.y
This is the NXP vendor kernel which differs a lot from mainline,
especially in the graphics/display area. You probably won't get much
support from this list as most people here (including me) would advise
against using the vendor kernel at all.
>
>> B/W if you may try the mainline
>> code base of imx8mm dsi please check here.
>> https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fopenedev%2Fkernel%2Fcommits%2Fimx8mm-dsi-v16&data=05%7C01%7Cfrieder.schrempf%40kontron.de%7Cecab86504a844ecbe8fb08db1dc80c14%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C638136517256300234%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=D2Ri4bUaPvkRCiuYWihOgC8PkGC8BGAVG9qnGhlBhEE%3D&reserved=0
>
> Do you really think this is a kernel version problem not only an
> integration issue? I mean NXP's version is coming with device-tree's
> using panels, and, although I can't test, I believe they are working,
> aren't they?
Probably they are working and probably you can get your setup working,
too, somehow. But you would need to ask for help in the NXP community or
elsewhere. The other option is to switch to mainline (recommended) and
use Jagan's patches for DSIM support that are about to be upstreamed.
Re: [PATCH v12 00/18] drm: Add Samsung MIPI DSIM bridge
On 17.02.23 19:22, Rasmus Villemoes wrote:
> On 07/02/2023 10.09, Rasmus Villemoes wrote:
>
>> I managed to get the whole chain lcdif -> mipi -> bridge -> dp-connector
>> to probe with these settings
>>
> [...]
>> Now hotplug-detect doesn't work with the current sn65dsi86 driver, but
>> that's a separate issue; when I boot with a monitor attached, its edid
>> is correctly read out. But I still don't get any output, and the monitor
>> says "no signal" - my naive attempt (which has worked fine in other
>> cases) was to just dd /dev/urandom to /dev/fb0, so I'm clearly missing
>> some important step.
>
> No idea if it's important, but in the NXP kernel, there's a
>
> display-subsystem {
> compatible = "fsl,imx-display-subsystem";
> ports = <&lcdif1_disp>,
> <&lcdif2_disp>,
> <&lcdif3_disp>;
> };
>
> node in imx8mp.dtsi, and when commenting out that node, the graphics
> ceases to work, even if all the devices in the lcdif->mipi->bridge chain
> actually probes. However, adding a corresponding node in mainline, which
> does have a driver for that "fsl,imx-display-subsystem", makes no
> difference; with or without that, I do get a /dev/fb0 device and the
> whole chain probes, but again the monitor says no signal.
The NXP kernel is completely different. AFAIK it uses the component
helpers to bundle all subdrivers from a central driver
(display-subsystem). This is not how the mainline approach using the
bridge driver interface works. So you can't compare them.
Did you look at this extensive thread with findings from Adam?
https://lore.kernel.org/lkml/CAHCN7xJ=N1vWVTBjArskJ59fyaLzmAGWfc0E=_igizrdnr_...@mail.gmail.com/
It is related to HDMI, but I guess a lot of things are valid for DP, too.
Anyway, we need to get this series merged. Otherwise we can't work on
fixing all the other issues on top.
Re: [PATCH v12 00/18] drm: Add Samsung MIPI DSIM bridge
On 03.02.23 13:29, Rasmus Villemoes wrote: > On 01/02/2023 23.00, Marek Vasut wrote: >> On 1/30/23 13:45, Rasmus Villemoes wrote: >>> On 27/01/2023 12.30, Marek Vasut wrote: On 1/27/23 12:04, Jagan Teki wrote: >>> >> Thanks, but that's exactly what I'm doing, and I don't see any >> modification of imx8mp.dtsi in that branch. I'm basically looking for >> help to do the equivalent of >> >> 88775338cd58 - arm64: dts: imx8mm: Add MIPI DSI pipeline >> f964f67dd6ee - arm64: dts: imx8mm: Add eLCDIF node support >> >> for imx8mp in order to test those patches on our boards (we have two >> variants). > > Marek, any help here, thanks. Try attached patch. >>> >>> Thanks. I removed the lcdif2 and ldb nodes I had added from Alexander's >>> patch (94e6197dadc9 in linux-next) in order to apply it. I get a couple >>> of errors during boot: >>> >>> clk: /soc@0/bus@32c0/mipi_dsi@32e6: failed to reparent >>> media_apb to sys_pll1_266m: -22 >>> >>> and enabling a pr_debug in clk_core_set_parent_nolock() shows that this >>> is because >>> >>> clk_core_set_parent_nolock: clk sys_pll1_266m can not be parent of clk >>> media_apb >>> >>> Further, the mipi_dsi fails to probe due to >>> >>> /soc@0/bus@32c0/mipi_dsi@32e6: failed to get >>> 'samsung,burst-clock-frequency' property >>> >>> All other .dtsi files seem to have those samsung,burst-clock-frequency >>> and samsung,esc-clock-frequency properties, so I suppose those should >>> also go into the imx8mp.dtsi and are not something that the board .dts >>> file should supply(?). >> >> No, that samsung,esc-clock-frequency (should be some 10-20 MHz, based on >> your panel/bridge) and samsung,burst-clock-frequency (that's the HS >> clock) should go into board DT, as those are property of the attached >> panel/bridge. > > OK. > > But I simply can't make that match what I see in that branch. For > example, there's imx8mm-icore-mx8mm-ctouch2-of10.dts and > imx8mm-icore-mx8mm-edimm2.2.dts which both seem to have a ti,sn65dsi84 > bridge, neither override the values defined in imx8mm.dtsi, which are > > samsung,burst-clock-frequency = <89100>; > samsung,esc-clock-frequency = <5400>; > > and that 891MHz value seems to be out of range for the dsi84 bridge - > under Recommended Operating Conditions, the data sheet says "DSI HS > clock input frequency", min 40, max 500 MHz. Please note that the value in samsung,burst-clock-frequency is double the clock rate of the effective DSI HS clock. I can confirm that a SN65DSI84 is able to work with the default settings in general. Still the LVDS clock is derived from the DSI clock and the sn65dsi83 driver calculates its PLL values expecting a DSI input clock matching the panel mode. So you might have to tune this value. > > There's also the "clk sys_pll1_266m can not be parent of clk media_apb". > Are you sure about those assigned-clocks and assigned-clock-parents > settings? > > Rasmus >
Re: [PATCH v12 00/18] drm: Add Samsung MIPI DSIM bridge
On 26.01.23 15:44, Jagan Teki wrote: > This series supports common bridge support for Samsung MIPI DSIM > which is used in Exynos and i.MX8MM SoC's. > > The final bridge supports both the Exynos and i.MX8M Mini/Nano/Plus. > > Patch 0001 - 0004: adding devm_drm_of_dsi_get_bridge > > Patch 0005 - 0006: optional PHY, PMS_P offset > > Patch 0007 : introduce hw_type > > Patch 0008 : fixing host init > > Patch 0009 : atomic_check > > Patch 0010 : input_bus_flags > > Patch 0011 : atomic_get_input_bus_fmts > > Patch 0012 - 0013: component vs bridge > > Patch 0014 : DSIM bridge > > Patch 0015 - 0016: i.MX8M Mini/Nano > > Patch 0017 - 0018: i.MX8M Plus > > Changes for v12: > - collect RB from Marek V > - add te_irq_handler hook > - fix comments from Marek V > - update atomic_get_input_bus_fmts logic > > Changes for v11: > - collect RB from Frieder Schrempf > - collect ACK from Rob > - collect ACK from Robert > - fix BIT macro replacements > - fix checkpatch --strict warnings > - fix unneeded commit text > - drop extra lines > > Changes for v10: > - rebase on drm-misc-next > - add drm_of_dsi_find_panel_or_bridge > - add devm_drm_of_dsi_get_bridge > - fix host initialization (Thanks to Marek Szyprowski) > - rearrange the tiny patches for easy to review > - update simple names for enum hw_type > - add is_hw_exynos macro > - rework on commit messages > > Changes for v9: > - rebase on drm-misc-next > - drop drm bridge attach fix for Exynos > - added prepare_prev_first flag > - added pre_enable_prev_first flag > - fix bridge chain order for exynos > - added fix for Exynos host init for first DSI transfer > - added MEDIA_BUS_FMT_FIXED > - return MEDIA_BUS_FMT_RGB888_1X24 output_fmt if supported output_fmt > list is unsupported. > - added MEDIA_BUS_FMT_YUYV10_1X20 > - added MEDIA_BUS_FMT_YUYV12_1X24 > > Changes for v8: > * fixed comment lines > * fixed commit messages > * fixed video mode bits > * collect Marek Ack > * fixed video mode bit names > * update input formats logic > * added imx8mplus support > > Changes for v7: > * fix the drm bridge attach chain for exynos drm dsi driver > * fix the hw_type checking logic > > Changes for v6: > * handle previous bridge for exynos dsi while attaching bridge > > Changes for v5: > * bridge changes to support multi-arch > * updated and clear commit messages > * add hw_type via plat data > * removed unneeded quirk > * rebased on linux-next > > Changes for v4: > * include Inki Dae in MAINTAINERS > * remove dsi_driver probe in exynos_drm_drv to support multi-arch build > * update init handling to ensure host init done on first cmd transfer > > Changes for v3: > * fix the mult-arch build > * fix dsi host init > * updated commit messages > > Changes for v2: > * fix bridge handling > * fix dsi host init > * correct the commit messages > > Tested in Engicam i.Core MX8M Mini SoM. > > Repo: > https://github.com/openedev/kernel/tree/imx8mm-dsi-v12 > > v11: > https://lore.kernel.org/all/20230123151212.269082-1-ja...@amarulasolutions.com/ I have tested this series (again) on Kontron DL i.MX8MM (= Kontron BL i.MX8MM + 7" LVDS display connected through SN65DSI84 bridge). Tested-by: Frieder Schrempf # Kontron DL i.MX8MM
Re: [PATCH v11 17/18] dt-bindings: display: exynos: dsim: Add NXP i.MX8M Plus support
On 23.01.23 13:23, Jagan Teki wrote: > Samsung MIPI DSIM bridge can also be found in i.MX8M Plus SoC. > > Add dt-bingings for it. > > Signed-off-by: Jagan Teki > --- > Changes for v11: > - collect ACK from Rob Tag is missing > Changes for v10, v9: > - none > > Documentation/devicetree/bindings/display/exynos/exynos_dsim.txt | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Documentation/devicetree/bindings/display/exynos/exynos_dsim.txt > b/Documentation/devicetree/bindings/display/exynos/exynos_dsim.txt > index 5133d4d39190..2a5f0889ec32 100644 > --- a/Documentation/devicetree/bindings/display/exynos/exynos_dsim.txt > +++ b/Documentation/devicetree/bindings/display/exynos/exynos_dsim.txt > @@ -8,6 +8,7 @@ Required properties: > "samsung,exynos5422-mipi-dsi" /* for Exynos5422/5800 SoCs */ > "samsung,exynos5433-mipi-dsi" /* for Exynos5433 SoCs */ > "fsl,imx8mm-mipi-dsim" /* for i.MX8M Mini/Nano SoCs */ > + "fsl,imx8mp-mipi-dsim" /* for i.MX8M Plus SoCs */ >- reg: physical base address and length of the registers set for the device >- interrupts: should contain DSI interrupt >- clocks: list of clock specifiers, must contain an entry for each required
Re: [PATCH v11 3/3] drm: exynos: dsi: Restore proper bridge chain order
On 20.01.23 20:42, Dave Stevenson wrote:
> Hi Jagan
>
> On Fri, 20 Jan 2023 at 19:10, Jagan Teki wrote:
>>
>> Hi Dave,
>>
>> On Sat, Jan 21, 2023 at 12:26 AM Dave Stevenson
>> wrote:
>>>
>>> Hi Jagan
>>>
>>> Responding due to Marek's comment on the "Add Samsung MIPI DSIM
>>> bridge" series, although I know very little about the Exynos
>>> specifics, and may well be missing context of what you're trying to
>>> achieve.
>>>
>>> On Mon, 12 Dec 2022 at 18:29, Jagan Teki wrote:
Restore the proper bridge chain by finding the previous bridge
in the chain instead of passing NULL.
This establishes a proper bridge chain while attaching downstream
bridges.
Reviewed-by: Marek Vasut
Signed-off-by: Marek Szyprowski
Signed-off-by: Jagan Teki
---
Changes for v11:
- add bridge.pre_enable_prev_first
Changes for v10:
- collect Marek review tag
drivers/gpu/drm/exynos/exynos_drm_dsi.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index ec673223d6b7..9d10a89d28f1 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -1428,7 +1428,8 @@ static int exynos_dsi_attach(struct drm_bridge
*bridge,
{
struct exynos_dsi *dsi = bridge_to_dsi(bridge);
- return drm_bridge_attach(bridge->encoder, dsi->out_bridge, NULL,
flags);
+ return drm_bridge_attach(bridge->encoder, dsi->out_bridge, bridge,
+flags);
>>>
>>> Agreed on this change.
>>>
}
static const struct drm_bridge_funcs exynos_dsi_bridge_funcs = {
@@ -1474,7 +1475,10 @@ static int exynos_dsi_host_attach(struct
mipi_dsi_host *host,
drm_bridge_add(&dsi->bridge);
- drm_bridge_attach(encoder, &dsi->bridge, NULL, 0);
+ drm_bridge_attach(encoder, &dsi->bridge,
+ list_first_entry_or_null(&encoder->bridge_chain,
+ struct drm_bridge,
+ chain_node), 0);
>>>
>>> What bridge are you expecting between the encoder and this bridge?
>>> The encoder is the drm_simple_encoder_init encoder that you've created
>>> in exynos_dsi_bind, so separating that from the bridge you're also
>>> creating here seems weird.
>>>
/*
* This is a temporary solution and should be made by more generic
way.
@@ -1709,6 +1713,7 @@ static int exynos_dsi_probe(struct platform_device
*pdev)
dsi->bridge.funcs = &exynos_dsi_bridge_funcs;
dsi->bridge.of_node = dev->of_node;
dsi->bridge.type = DRM_MODE_CONNECTOR_DSI;
+ dsi->bridge.pre_enable_prev_first = true;
>>>
>>> Setting dsi->bridge.pre_enable_prev_first on what is presumably the
>>> DSI host controller seems a little odd.
>>> Same question again - what bridge are you expecting to be upstream of
>>> the DSI host that needs to be preenabled before it? Whilst it's
>>> possible that there's another bridge, I'd have expected that to be the
>>> first link from your encoder as they appear to both belong to the same
>>> bit of driver.
>>
>> Let me answer all together here. I can explain a bit about one of the
>> pipelines used in Exynos. Exynos DSI DRM drivers have some strict host
>> initialization which is not the same as what we used in i.MX8M even
>> though it uses the same DSIM IP.
>>
>> Exynos5433 Decon -> Exynos MIC -> Exynos DSI -> s6e3ha2 DSI panel
>>
>> Here MIC is the bridge, Exynos DSI is the bridge and the requirement
>> is to expect the upstream bridge to pre_enable first from DSI which
>> means the MIC.
>
> That makes sense for the pre_enable_prev_first flag.
>
> The drm_bridge_attach(... list_first_entry_or_null) still seems a
> little weird. I think you are making the assumption that there is only
> ever going to be the zero or one bridge (the MIC) between encoder and
> DSI bridge - the DSI bridge is linking itself to the first entry off
> the encoder bridge_chain (or NULL to link to the encoder). Is that
> reasonable? I've no idea!
I think the assumption is reasonable for now, as it covers both types of
chains this driver is currently used in (Exynos and i.MX). And IIUC this
change is mainly needed for (backward) compatibility with the somewhat
"special" chain in Exynos.
>
> I must confess to not having looked at the attaching sequence
> recently, and I'm about to head home for the weekend.
> I have no real knowledge of how Exynos is working, and am aware that
> you're having to rejuggle stuff to try and support i.MX8M and Exynos,
> so leave that one up to you.
I strongly vote for leaving this as is for now. We already spent too
much time finding a satisfying solution that covers
Re: [PATCH v10 18/18] drm: bridge: samsung-dsim: Add i.MX8M Plus support
On 14.12.22 13:59, Jagan Teki wrote: > From: Marek Vasut > > Add extras to support i.MX8M Plus. The main change is the removal of > HS/VS/DE signal inversion in the LCDIFv3-DSIM glue logic, otherwise > the implementation of this IP in i.MX8M Plus is very much compatible > with the i.MX8M Mini/Nano one. > > Signed-off-by: Marek Vasut > Signed-off-by: Jagan Teki Reviewed-by: Frieder Schrempf
Re: [PATCH v10 16/18] drm: bridge: samsung-dsim: Add i.MX8M Mini/Nano support
On 14.12.22 13:59, Jagan Teki wrote: > Samsung MIPI DSIM master can also be found in i.MX8M Mini/Nano SoC. > > Add compatible and associated driver_data for it. > > Reviewed-by: Laurent Pinchart > Signed-off-by: Marek Szyprowski > Signed-off-by: Jagan Teki Reviewed-by: Frieder Schrempf
Re: [PATCH v10 11/18] drm: exynos: dsi: Add atomic_get_input_bus_fmts
On 14.12.22 13:59, Jagan Teki wrote: > Finding the right input bus format throughout the pipeline is hard > so add atomic_get_input_bus_fmts callback and initialize with the > proper input format from list of supported output formats. > > This format can be used in pipeline for negotiating bus format between > the DSI-end of this bridge and the other component closer to pipeline > components. > > List of Pixel formats are taken from, > AN13573 i.MX 8/RT MIPI DSI/CSI-2, Rev. 0, 21 March 2022 > 3.7.4 Pixel formats > Table 14. DSI pixel packing formats > > Tested-by: Marek Szyprowski > Signed-off-by: Jagan Teki Reviewed-by: Frieder Schrempf
Re: [PATCH v10 10/18] drm: exynos: dsi: Add input_bus_flags
On 14.12.22 13:58, Jagan Teki wrote: > LCDIF-DSIM glue logic inverts the HS/VS/DE signals and expecting > the i.MX8M Mini/Nano DSI host to add additional Data Enable signal > active low (DE_LOW). This makes the valid data transfer on each > horizontal line. > > So, add additional bus flags DE_LOW setting via input_bus_flags > for i.MX8M Mini/Nano platforms. > > Suggested-by: Marek Vasut > Signed-off-by: Jagan Teki Reviewed-by: Frieder Schrempf
Re: [PATCH v10 09/18] drm: exynos: dsi: Add atomic check
On 15.12.22 09:39, Frieder Schrempf wrote: > On 14.12.22 13:58, Jagan Teki wrote: >> Look like an explicit fixing up of mode_flags is required for DSIM IP >> present in i.MX8M Mini/Nano SoCs. >> >> At least the LCDIF + DSIM needs active low sync polarities in order >> to correlate the correct sync flags of the surrounding components in >> the chain to make sure the whole pipeline can work properly. >> >> On the other hand the i.MX 8M Mini Applications Processor Reference Manual, >> Rev. 3, 11/2020 says. >> "13.6.3.5.2 RGB interface >> Vsync, Hsync, and VDEN are active high signals." >> >> i.MX 8M Mini Applications Processor Reference Manual Rev. 3, 11/2020 >> 3.6.3.5.2 RGB interface >> i.MX 8M Nano Applications Processor Reference Manual Rev. 2, 07/2022 >> 13.6.2.7.2 RGB interface >> both claim "Vsync, Hsync, and VDEN are active high signals.", the >> LCDIF must generate inverted HS/VS/DE signals, i.e. active LOW. >> >> No clear evidence about whether it can be documentation issues or >> something, so added a comment FIXME for this and updated the active low >> sync polarities using SAMSUNG_DSIM_TYPE_IMX8MM hw_type. By the way, the commit message mentions a FIXME comment above. But it's not in the patch. The commit message probably needs an update. >> >> Comments are suggested by Marek Vasut. >> >> Signed-off-by: Jagan Teki > > Reviewed-by: Frieder Schrempf
Re: [PATCH v10 09/18] drm: exynos: dsi: Add atomic check
On 14.12.22 13:58, Jagan Teki wrote: > Look like an explicit fixing up of mode_flags is required for DSIM IP > present in i.MX8M Mini/Nano SoCs. > > At least the LCDIF + DSIM needs active low sync polarities in order > to correlate the correct sync flags of the surrounding components in > the chain to make sure the whole pipeline can work properly. > > On the other hand the i.MX 8M Mini Applications Processor Reference Manual, > Rev. 3, 11/2020 says. > "13.6.3.5.2 RGB interface > Vsync, Hsync, and VDEN are active high signals." > > i.MX 8M Mini Applications Processor Reference Manual Rev. 3, 11/2020 > 3.6.3.5.2 RGB interface > i.MX 8M Nano Applications Processor Reference Manual Rev. 2, 07/2022 > 13.6.2.7.2 RGB interface > both claim "Vsync, Hsync, and VDEN are active high signals.", the > LCDIF must generate inverted HS/VS/DE signals, i.e. active LOW. > > No clear evidence about whether it can be documentation issues or > something, so added a comment FIXME for this and updated the active low > sync polarities using SAMSUNG_DSIM_TYPE_IMX8MM hw_type. > > Comments are suggested by Marek Vasut. > > Signed-off-by: Jagan Teki Reviewed-by: Frieder Schrempf
Re: [PATCH v10 08/18] drm: exynos: dsi: Handle proper host initialization
On 14.12.22 13:58, Jagan Teki wrote: > From: Marek Szyprowski > > Host transfer() in the DSI master will invoke only when the DSI commands > are sent from DSI devices like DSI Panel or DSI bridges and this host > the transfer wouldn't invoke for I2C-based-DSI bridge drivers. > > Handling DSI host initialization in transfer calls misses the controller > setup for I2C configured DSI bridges. > > This patch updates the DSI host initialization by calling host to init > from bridge pre_enable as the bridge pre_enable API is invoked by core > as it is common across all classes of DSI device drivers. > > The host init during pre_enable is conditional and not invoked for Exynos > as existing downstream drm panels and bridges in Exynos are expecting > the host initialization during DSI transfer. > > Signed-off-by: Jagan Teki > Signed-off-by: Marek Szyprowski Reviewed-by: Frieder Schrempf
Re: [PATCH v10 07/18] drm: exynos: dsi: Introduce hw_type platform data
On 14.12.22 13:58, Jagan Teki wrote:
> Samsung MIPI DSIM controller is common DSI IP that can be used
> in various SoCs like Exynos, i.MX8M Mini/Nano/Plus.
>
> Add hw_type enum via platform_data so that accessing the different
> controller data between various platforms becomes easy and meaningful.
>
> Suggested-by: Marek Szyprowski
> Signed-off-by: Jagan Teki
> ---
> Changes for v10:
> - split from previous series patch
> "drm: bridge: Generalize Exynos-DSI driver into a Samsung DSIM bridge"
> - update enum type names
>
> drivers/gpu/drm/exynos/exynos_drm_dsi.c | 84 -
> 1 file changed, 69 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> index 7a845badb1b2..fdaf514b39f2 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> @@ -254,6 +254,16 @@ struct exynos_dsi_transfer {
> #define DSIM_STATE_CMD_LPM BIT(2)
> #define DSIM_STATE_VIDOUT_AVAILABLE BIT(3)
>
> +enum exynos_dsi_type {
> + DSIM_TYPE_EXYNOS3250,
> + DSIM_TYPE_EXYNOS4210,
> + DSIM_TYPE_EXYNOS5410,
> + DSIM_TYPE_EXYNOS5422,
> + DSIM_TYPE_EXYNOS5433,
> +
The empty line looks a bit awkward to me. Otherwise:
Reviewed-by: Frieder Schrempf
> + DSIM_TYPE_COUNT,
> +};
> +
[...]
Re: [PATCH v10 05/18] drm: exynos: dsi: Mark PHY as optional
On 14.12.22 13:58, Jagan Teki wrote: > The same Samsung MIPI DSIM master can also be used in NXP's > i.MX8M Mini/Nano/Plus SoC. > > In i.MX8M Mini/Nano/Plus SoC the DSI Phy requires a MIPI DPHY > bit to reset in order to activate the PHY and that can be done > via upstream i.MX8M blk-ctrl driver. > > So, mark the phy get as optional. > > Reviewed-by: Marek Vasut > Signed-off-by: Jagan Teki Reviewed-by: Frieder Schrempf
Re: [PATCH v10 2/2] drm: exynos: dsi: Properly name HSA/HBP/HFP/HSE bits
On 12.12.22 15:57, Jagan Teki wrote: > HSA/HBP/HFP/HSE mode bits in Processor Reference Manuals specify > a naming conversion as 'disable mode bit' due to its bit definition, > 0 = Enable and 1 = Disable. > > For HSE bit, the i.MX 8M Mini/Nano/Plus Applications Processor > Reference Manual named this bit as 'HseDisableMode' but the bit > definition is quite opposite like > 0 = Disables transfer > 1 = Enables transfer > which clearly states that HSE is not a disable bit. > > HSE is named as per the manual even though it is not a disable > bit however the driver logic for handling HSE is based on the > MIPI_DSI_MODE_VIDEO_HSE flag itself. > > Cc: Nicolas Boichat > Reviewed-by: Marek Vasut > Signed-off-by: Jagan Teki Reviewed-by: Frieder Schrempf
Re: [PATCH v10 1/2] drm: exynos: dsi: Fix MIPI_DSI*_NO_* mode flags
On 12.12.22 15:57, Jagan Teki wrote:
> HFP/HBP/HSA/EOT_PACKET modes in Exynos DSI host specifies
> 0 = Enable and 1 = Disable.
>
> The logic for checking these mode flags was correct before
> the MIPI_DSI*_NO_* mode flag conversion.
>
> This patch is trying to fix this MIPI_DSI*_NO_* mode flags handling
> Exynos DSI host and update the mode_flags in relevant panel drivers.
>
> Fixes: <0f3b68b66a6d> ("drm/dsi: Add _NO_ to MIPI_DSI_* flags disabling
> features")
> Reviewed-by: Marek Vasut
> Reviewed-by: Nicolas Boichat
> Reported-by: Sébastien Szymanski
> Signed-off-by: Jagan Teki
Reviewed-by: Frieder Schrempf
Re: [PATCH v11 3/3] drm: exynos: dsi: Restore proper bridge chain order
On 12.12.22 19:29, Jagan Teki wrote: > Restore the proper bridge chain by finding the previous bridge > in the chain instead of passing NULL. > > This establishes a proper bridge chain while attaching downstream > bridges. > > Reviewed-by: Marek Vasut > Signed-off-by: Marek Szyprowski > Signed-off-by: Jagan Teki Reviewed-by: Frieder Schrempf
Re: [PATCH v11 2/3] drm/bridge: tc358764: Enable pre_enable_prev_first flag
On 12.12.22 19:29, Jagan Teki wrote: > From: Marek Szyprowski > > Enable the drm bridge pre_enable_prev_first flag so that the > previous bridge pre_enable should be called first before the > pre_enable for the tc358764 bridge is called. > > This makes sure that the previous bridge should be initialized > properly before the tc358764 bridge is powered up. > > Signed-off-by: Marek Szyprowski > Signed-off-by: Jagan Teki Reviewed-by: Frieder Schrempf
Re: [PATCH v11 1/3] drm: panel: Enable prepare_prev_first flag for samsung-s6e panels
On 12.12.22 19:29, Jagan Teki wrote: > Enable the drm panel prepare_prev_first flag so-that the previous > controller should be prepared first before the prepare for the > panel is called. > > samsung-s6e3ha2, samsung-s6e63j0x03 and samsung-s6e8aa0 are the > effected samsung-s6e panels for this change. > > This makes sure that the previous controller, likely to be a DSI > host controller should be initialized to LP-11 before the panel > is powered up. > > Signed-off-by: Marek Szyprowski > Signed-off-by: Jagan Teki Reviewed-by: Frieder Schrempf
Re: [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi83: Add enable delay property
On 12.12.22 10:23, Krzysztof Kozlowski wrote: > On 12/12/2022 10:09, Frieder Schrempf wrote: >>>>> This does seem like a hardware bug right there, can you double-check >>>>> this with the hardware engineer ? >>>> >>>> Yep, checked with hardware engineer. An 470nF is attached, together >>>> with an >>>> open drain output and only the internal pull-up. So yes ~113ms rising >>>> time >>>> until 0.7 x VCC. >>> >>> I don't suppose you can have that capacitor reduced or better yet, some >>> external pull up added, can you ? >> >> Actually our HW engineers have implemented a similar RC circuit to >> provide a hardware delay for the EN signal. I think this is due to a >> design note in the datasheet (see chapter 7.4.1) and therefore it's >> probably widely spread. > > If I read section 7.4.1 correctly, it would be enough to just add delay > Ten=1ms instead of the capacitor, right? And that would be > device-specific. But if one chooses the capacitor solution, it becomes > now board specific. Yes, seems like that's the case.
Re: [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi83: Add enable delay property
On 12.12.22 10:32, Laurent Pinchart wrote: > On Mon, Dec 12, 2022 at 10:09:45AM +0100, Frieder Schrempf wrote: >> On 09.12.22 15:49, Marek Vasut wrote: >>> On 12/9/22 14:38, Alexander Stein wrote: >>>> Am Freitag, 9. Dezember 2022, 13:43:02 CET schrieb Marek Vasut: >>>>> On 12/9/22 13:21, Alexander Stein wrote: >>>>>> Am Freitag, 9. Dezember 2022, 13:02:10 CET schrieb Marek Vasut: >>>>>>> On 12/9/22 10:36, Alexander Stein wrote: >>>>>>>> Am Freitag, 9. Dezember 2022, 10:07:45 CET schrieb Krzysztof Kozlowski: >>>>>>>>> On 09/12/2022 09:54, Alexander Stein wrote: >>>>>>>>>> Am Freitag, 9. Dezember 2022, 09:39:49 CET schrieb Krzysztof >>>>>>>>>> Kozlowski: >>>>>>>>>>> On 09/12/2022 09:33, Alexander Stein wrote: >>>>>>>>>>>> It takes some time until the enable GPIO has settled when turning >>>>>>>>>>>> on. >>>>>>>>>>>> This delay is platform specific and may be caused by e.g. voltage >>>>>>>>>>>> shifts, capacitors etc. >>>>>>>>>>>> >>>>>>>>>>>> Signed-off-by: Alexander Stein >>>>>>>>>>>> --- >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> .../devicetree/bindings/display/bridge/ti,sn65dsi83.yaml | 4 >>>>>>>>>>>> >>>>>>>>>>>> 1 file changed, 4 insertions(+) >>>>>>>>>>>> >>>>>>>>>>>> diff --git >>>>>>>>>>>> a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml >>>>>>>>>>>> b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml >>>>>>>>>>>> index 48a97bb3e2e0d..3f50d497cf8ac 100644 >>>>>>>>>>>> --- >>>>>>>>>>>> a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml >>>>>>>>>>>> +++ >>>>>>>>>>>> b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml >>>>>>>>>>>> >>>>>>>>>>>> @@ -32,6 +32,10 @@ properties: >>>>>>>>>>>> maxItems: 1 >>>>>>>>>>>> description: GPIO specifier for bridge_en pin (active >>>>>>>>>>>> high). >>>>>>>>>>>> >>>>>>>>>>>> + ti,enable-delay-us: >>>>>>>>>>>> + default: 1 >>>>>>>>>>>> + description: Enable time delay for enable-gpios >>>>>>>>>>> >>>>>>>>>>> Aren't you now mixing two separate delays? One for entire block (I >>>>>>>>>>> would assume mostly fixed delay) and one depending on regulators >>>>>>>>>>> (regulator-ramp-delay, regulator-enable-ramp-delay). Maybe you miss >>>>>>>>>>> the >>>>>>>>>>> second delays in your power supply? If so, the first one might be >>>>>>>>>>> fixed >>>>>>>>>>> and hard-coded in the driver? >>>>>>>>>> >>>>>>>>>> Apparently there are two different delays: reset time (t_reset) of >>>>>>>>>> 10ms as >>>>>>>>>> specified by datasheet. This is already ensured by a following delay >>>>>>>>>> after >>>>>>>>>> requesting enable_gpio as low and switching the GPIO to low in >>>>>>>>>> disable >>>>>>>>>> path. >>>>>>>>>> >>>>>>>>>> When enabling this GPIO it takes some time until it is valid on the >>>>>>>>>> chip, >>>>>>>>>> this is what this series is about. It's highly platform specific. >>>>>>>>>> >>>>>>>>>> Unfortunately this is completely unrelated to the vcc-supply >>>>>>&g
Re: [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi83: Add enable delay property
On 09.12.22 15:49, Marek Vasut wrote: > On 12/9/22 14:38, Alexander Stein wrote: >> Am Freitag, 9. Dezember 2022, 13:43:02 CET schrieb Marek Vasut: >>> On 12/9/22 13:21, Alexander Stein wrote: Hi Marek, Am Freitag, 9. Dezember 2022, 13:02:10 CET schrieb Marek Vasut: > On 12/9/22 10:36, Alexander Stein wrote: >> Hello Krzysztof, > > Hi, > >> Am Freitag, 9. Dezember 2022, 10:07:45 CET schrieb Krzysztof >> Kozlowski: >>> On 09/12/2022 09:54, Alexander Stein wrote: Hello Krzysztof, thanks for the fast feedback. Am Freitag, 9. Dezember 2022, 09:39:49 CET schrieb Krzysztof >> Kozlowski: > On 09/12/2022 09:33, Alexander Stein wrote: >> It takes some time until the enable GPIO has settled when turning >> on. >> This delay is platform specific and may be caused by e.g. voltage >> shifts, capacitors etc. >> >> Signed-off-by: Alexander Stein >> --- >> >> >> .../devicetree/bindings/display/bridge/ti,sn65dsi83.yaml | 4 >> >> 1 file changed, 4 insertions(+) >> >> diff --git >> a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml >> b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml >> index 48a97bb3e2e0d..3f50d497cf8ac 100644 >> --- >> a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml >> +++ >> b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml >> >> @@ -32,6 +32,10 @@ properties: >> maxItems: 1 >> description: GPIO specifier for bridge_en pin (active >> high). >> >> + ti,enable-delay-us: >> + default: 1 >> + description: Enable time delay for enable-gpios > > Aren't you now mixing two separate delays? One for entire block > on (I > would assume mostly fixed delay) and one depending on regulators > (regulator-ramp-delay, regulator-enable-ramp-delay). Maybe you > miss > the > second delays in your power supply? If so, the first one might be > fixed > and hard-coded in the driver? Apparently there are two different delays: reset time (t_reset) of 10ms as specified by datasheet. This is already ensured by a following delay after requesting enable_gpio as low and switching the GPIO to low in disable path. When enabling this GPIO it takes some time until it is valid on the chip, this is what this series is about. It's highly platform specific. Unfortunately this is completely unrelated to the vcc-supply regulator. This one has to be enabled before the enable GPIO can be enabled. So there is no regulator-ramp-delay. >>> >>> Your driver does one after another - regulator followed >>> immediately by >>> gpio - so this as well can be a delay from regulator (maybe not ramp >>> but >>> enable delay). > > The chip has two separate input pins: > > VCC -- power supply that's regulator > EN -- reset line, that's GPIO > > Alexander is talking about EN line here. > >> But this will introduce a section which must not be interrupted or >> delayed. >> This is impossible as the enable gpio is attached to an i2c >> expander in >> my >> case. >> >> Given the following time chart: >> vcc set EN >> >> enable GPIO PAD >> >> | |<-- t_raise -->| >> | >> | <-- t_vcc_gpio --> | | >> | <-- t_enable_delay --> | >> >> t_raise is the time from changing the GPIO output at the expander >> until >> voltage on the EN (input) pad from the bridge has reached high >> voltage >> level. This is an electrical characteristic I can not change and >> have to >> take into account. >> t_vcc_gpio is the time from enabling supply voltage to enabling the >> bridge >> (removing from reset). Minimum t_vcc_gpio is something which can be >> addressed by the regulator and is no problem so far. But there is no >> upper bound to it. > > What exactly is your EN signal rise time (should be ns or so)? Can you > look at that with a scope , maybe even with relation to the VCC > regulator > ? I checked EN rise time using a scope, it's ~110ms. I not an expert in hardware but on the mainboard there is some capacitor attached to this line, which increased the time, independent from the internal pull-up. >>> >>> This does seem like a hardware bug righ
Re: [PATCH v8 06/14] drm: bridge: samsung-dsim: Handle proper DSI host initialization
On 05.12.22 16:37, Frieder Schrempf wrote: > Hi Dave, > > On 05.12.22 16:20, Dave Stevenson wrote: >> Hi Frieder >> >> On Mon, 5 Dec 2022 at 07:30, Frieder Schrempf >> wrote: >>> >>> On 02.12.22 15:55, Dave Stevenson wrote: >>>> Hi Marek >>>> >>>> On Fri, 2 Dec 2022 at 12:21, Marek Vasut wrote: >>>>> >>>>> On 12/2/22 11:52, Marek Szyprowski wrote: >>>>>> Hi, >>>>>> >>>>>> Sorry for delay, I was on a sick leave last 2 weeks. >>>>>> >>>>>> On 28.11.2022 15:43, Jagan Teki wrote: >>>>>>> ,On Sat, Nov 26, 2022 at 3:44 AM Marek Vasut wrote: >>>>>>>> On 11/23/22 21:09, Jagan Teki wrote: >>>>>>>>> On Sat, Nov 19, 2022 at 7:45 PM Marek Vasut wrote: >>>>>>>>>> On 11/17/22 14:04, Marek Szyprowski wrote: >>>>>>>>>>> On 17.11.2022 05:58, Marek Vasut wrote: >>>>>>>>>>>> On 11/10/22 19:38, Jagan Teki wrote: >>>>>>>>>>>>> DSI host initialization handling in previous exynos dsi driver has >>>>>>>>>>>>> some pitfalls. It initializes the host during host transfer() hook >>>>>>>>>>>>> that is indeed not the desired call flow for I2C and any other DSI >>>>>>>>>>>>> configured downstream bridges. >>>>>>>>>>>>> >>>>>>>>>>>>> Host transfer() is usually triggered for downstream DSI panels or >>>>>>>>>>>>> bridges and I2C-configured-DSI bridges miss these host >>>>>>>>>>>>> initialization >>>>>>>>>>>>> as these downstream bridges use bridge operations hooks like >>>>>>>>>>>>> pre_enable, >>>>>>>>>>>>> and enable in order to initialize or set up the host. >>>>>>>>>>>>> >>>>>>>>>>>>> This patch is trying to handle the host init handler to satisfy >>>>>>>>>>>>> all >>>>>>>>>>>>> downstream panels and bridges. Added the DSIM_STATE_REINITIALIZED >>>>>>>>>>>>> state >>>>>>>>>>>>> flag to ensure that host init is also done on first cmd transfer, >>>>>>>>>>>>> this >>>>>>>>>>>>> helps existing DSI panels work on exynos platform (form Marek >>>>>>>>>>>>> Szyprowski). >>>>>>>>>>>>> >>>>>>>>>>>>> v8, v7, v6, v5: >>>>>>>>>>>>> * none >>>>>>>>>>>>> >>>>>>>>>>>>> v4: >>>>>>>>>>>>> * update init handling to ensure host init done on first cmd >>>>>>>>>>>>> transfer >>>>>>>>>>>>> >>>>>>>>>>>>> v3: >>>>>>>>>>>>> * none >>>>>>>>>>>>> >>>>>>>>>>>>> v2: >>>>>>>>>>>>> * check initialized state in samsung_dsim_init >>>>>>>>>>>>> >>>>>>>>>>>>> v1: >>>>>>>>>>>>> * keep DSI init in host transfer >>>>>>>>>>>>> >>>>>>>>>>>>> Signed-off-by: Marek Szyprowski >>>>>>>>>>>>> Signed-off-by: Jagan Teki >>>>>>>>>>>>> --- >>>>>>>>>>>>> drivers/gpu/drm/bridge/samsung-dsim.c | 25 >>>>>>>>>>>>> + >>>>>>>>>>>>> include/drm/bridge/samsung-dsim.h | 5 +++-- >>>>>>>>>>>>> 2 files changed, 20 insertions(+), 10 deletions(-) >>>>>>>>>>>>> >>>>>>>>>>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c >>>>>>>>>>>>> b/drivers/gpu/drm/bridge/sa
Re: [PATCH v3 3/5] drm/bridge: Introduce pre_enable_prev_first to alter bridge init order
Hi Dave,
On 02.12.22 15:28, Dave Stevenson wrote:
> DSI sink devices typically want the DSI host powered up and configured
> before they are powered up. pre_enable is the place this would normally
> happen, but they are called in reverse order from panel/connector towards
> the encoder, which is the "wrong" order.
>
> Add a new flag pre_enable_prev_first that any bridge can set
> to swap the order of pre_enable (and post_disable) for that and the
> immediately previous bridge.
> Should the immediately previous bridge also set the
> pre_enable_prev_first flag, the previous bridge to that will be called
> before either of those which requested pre_enable_prev_first.
>
> eg:
> - Panel
> - Bridge 1
> - Bridge 2 pre_enable_prev_first
> - Bridge 3
> - Bridge 4 pre_enable_prev_first
> - Bridge 5 pre_enable_prev_first
> - Bridge 6
> - Encoder
> Would result in pre_enable's being called as Panel, Bridge 1, Bridge 3,
> Bridge 2, Bridge 6, Bridge 5, Bridge 4, Encoder.
>
> Signed-off-by: Dave Stevenson
I tested this patchset together with the pending Samsung DSIM bridge
conversion for i.MX8MM [1] and additional patches to fix the
enable/disable sequence for my setup [2] on a Kontron DL i.MX8MM board
featuring a TI SN65DSI84 and a 7" LVDS panel.
Using this the initialization sequence of the TI SN65DSI84 specified in
the datasheet can be achieved. The correct order was verified by tracing
the function calls and capturing the DSI data signal and the SN65DSI84
enable GPIO (EN) on a scope.
I've also gone through the code of this patch and it all makes sense to
me. There is only one small nitpick (see below).
Tested-by: Frieder Schrempf
Reviewed-by: Frieder Schrempf
Thanks
Frieder
[1]
https://patchwork.kernel.org/project/dri-devel/cover/20221110183853.3678209-1-ja...@amarulasolutions.com/
[2]
https://git.kontron-electronics.de/sw/misc/linux/-/commits/v6.1-dsim-mx8mm
> ---
> drivers/gpu/drm/drm_bridge.c | 144 +--
> include/drm/drm_bridge.h | 8 ++
> 2 files changed, 128 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index bb7fc09267af..41051869d6bf 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -581,6 +581,25 @@ void drm_atomic_bridge_chain_disable(struct drm_bridge
> *bridge,
[...]>
> - bridge->funcs->atomic_post_disable(bridge,
> -old_bridge_state);
> - } else if (bridge->funcs->post_disable) {
> - bridge->funcs->post_disable(bridge);
> - }
> + if (limit)
> + bridge = limit;
Nit: Maybe add a comment for the code above to explain that this is for
skipping the already post_disabled bridge. Just like for pre_enable below?
> }
> }
> EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);
>
[...]
Re: [PATCH v8 06/14] drm: bridge: samsung-dsim: Handle proper DSI host initialization
Hi Dave, On 05.12.22 16:20, Dave Stevenson wrote: > Hi Frieder > > On Mon, 5 Dec 2022 at 07:30, Frieder Schrempf > wrote: >> >> On 02.12.22 15:55, Dave Stevenson wrote: >>> Hi Marek >>> >>> On Fri, 2 Dec 2022 at 12:21, Marek Vasut wrote: >>>> >>>> On 12/2/22 11:52, Marek Szyprowski wrote: >>>>> Hi, >>>>> >>>>> Sorry for delay, I was on a sick leave last 2 weeks. >>>>> >>>>> On 28.11.2022 15:43, Jagan Teki wrote: >>>>>> ,On Sat, Nov 26, 2022 at 3:44 AM Marek Vasut wrote: >>>>>>> On 11/23/22 21:09, Jagan Teki wrote: >>>>>>>> On Sat, Nov 19, 2022 at 7:45 PM Marek Vasut wrote: >>>>>>>>> On 11/17/22 14:04, Marek Szyprowski wrote: >>>>>>>>>> On 17.11.2022 05:58, Marek Vasut wrote: >>>>>>>>>>> On 11/10/22 19:38, Jagan Teki wrote: >>>>>>>>>>>> DSI host initialization handling in previous exynos dsi driver has >>>>>>>>>>>> some pitfalls. It initializes the host during host transfer() hook >>>>>>>>>>>> that is indeed not the desired call flow for I2C and any other DSI >>>>>>>>>>>> configured downstream bridges. >>>>>>>>>>>> >>>>>>>>>>>> Host transfer() is usually triggered for downstream DSI panels or >>>>>>>>>>>> bridges and I2C-configured-DSI bridges miss these host >>>>>>>>>>>> initialization >>>>>>>>>>>> as these downstream bridges use bridge operations hooks like >>>>>>>>>>>> pre_enable, >>>>>>>>>>>> and enable in order to initialize or set up the host. >>>>>>>>>>>> >>>>>>>>>>>> This patch is trying to handle the host init handler to satisfy all >>>>>>>>>>>> downstream panels and bridges. Added the DSIM_STATE_REINITIALIZED >>>>>>>>>>>> state >>>>>>>>>>>> flag to ensure that host init is also done on first cmd transfer, >>>>>>>>>>>> this >>>>>>>>>>>> helps existing DSI panels work on exynos platform (form Marek >>>>>>>>>>>> Szyprowski). >>>>>>>>>>>> >>>>>>>>>>>> v8, v7, v6, v5: >>>>>>>>>>>> * none >>>>>>>>>>>> >>>>>>>>>>>> v4: >>>>>>>>>>>> * update init handling to ensure host init done on first cmd >>>>>>>>>>>> transfer >>>>>>>>>>>> >>>>>>>>>>>> v3: >>>>>>>>>>>> * none >>>>>>>>>>>> >>>>>>>>>>>> v2: >>>>>>>>>>>> * check initialized state in samsung_dsim_init >>>>>>>>>>>> >>>>>>>>>>>> v1: >>>>>>>>>>>> * keep DSI init in host transfer >>>>>>>>>>>> >>>>>>>>>>>> Signed-off-by: Marek Szyprowski >>>>>>>>>>>> Signed-off-by: Jagan Teki >>>>>>>>>>>> --- >>>>>>>>>>>> drivers/gpu/drm/bridge/samsung-dsim.c | 25 >>>>>>>>>>>> + >>>>>>>>>>>> include/drm/bridge/samsung-dsim.h | 5 +++-- >>>>>>>>>>>> 2 files changed, 20 insertions(+), 10 deletions(-) >>>>>>>>>>>> >>>>>>>>>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c >>>>>>>>>>>> b/drivers/gpu/drm/bridge/samsung-dsim.c >>>>>>>>>>>> index bb1f45fd5a88..ec7e01ae02ea 100644 >>>>>>>>>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c >>>>>>>>>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c >>>>>>>>>>>> @@ -1234,12 +1234,17 @@ static void samsung_dsim_d
Re: [PATCH v8 00/14] drm: bridge: Add Samsung MIPI DSIM bridge
On 10.11.22 19:38, Jagan Teki wrote: > This series supports common bridge support for Samsung MIPI DSIM > which is used in Exynos and i.MX8MM SoC's. > > The final bridge supports both the Exynos and i.MX8M Mini/Nano/Plus. > > Changes for v8: > * fixed comment lines > * fixed commit messages > * fixed video mode bits > * collect Marek Ack > * fixed video mode bit names > * update input formats logic > * added imx8mplus support > > Changes for v7: > * fix the drm bridge attach chain for exynos drm dsi driver > * fix the hw_type checking logic > > Changes for v6: > * handle previous bridge for exynos dsi while attaching bridge > > Changes for v5: > * bridge changes to support multi-arch > * updated and clear commit messages > * add hw_type via plat data > * removed unneeded quirk > * rebased on linux-next > > Changes for v4: > * include Inki Dae in MAINTAINERS > * remove dsi_driver probe in exynos_drm_drv to support multi-arch build > * update init handling to ensure host init done on first cmd transfer > > Changes for v3: > * fix the mult-arch build > * fix dsi host init > * updated commit messages > > Changes for v2: > * fix bridge handling > * fix dsi host init > * correct the commit messages > > Patch 0001: Fix MIPI_DSI*_NO_* mode bits > > Patch 0002: Properly name HSA/HBP/HFP/HSE bits > > Patch 0003: Samsung DSIM bridge > > Patch 0004: PHY optional > > Patch 0005: OF-graph or Child node lookup > > Patch 0006: DSI host initialization > > Patch 0007: atomic check > > Patch 0008: PMS_P offset via plat data > > Patch 0009: atomic_get_input_bus_fmts > > Patch 0010: input_bus_flags > > Patch 0011: document fsl,imx8mm-mipi-dsim > > Patch 0012: add i.MX8M Mini/Nano DSIM support > > Patch 0013: document fsl,imx8mp-mipi-dsim > > Patch 0014: add i.MX8M Plus DSIM support > > Tested in Engicam i.Core MX8M Mini SoM. > > Repo: > https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v8 > > Any inputs? > Jagan. I tested this on the Kontron DL i.MX8MM which uses a TI SN65DSI84 bridge and a Jenson 7" LVDS Display. Tested-by: Frieder Schrempf # Kontron DL i.MX8MM
Re: [PATCH v8 02/14] drm: exynos: dsi: Properly name HSA/HBP/HFP/HSE bits
On 10.11.22 19:38, Jagan Teki wrote: > HSA/HBP/HFP/HSE mode bits in Exynos DSI host specify a naming > conversion as 'disable mode bit' due to its bit definition, > 0 = Enable and 1 = Disable. > > Fix the naming convention of the mode bits. > > Signed-off-by: Jagan Teki > --- > drivers/gpu/drm/exynos/exynos_drm_dsi.c | 16 > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c > b/drivers/gpu/drm/exynos/exynos_drm_dsi.c > index b5305b145ddb..fce7f0a7e4ee 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c > @@ -75,10 +75,10 @@ > #define DSIM_MAIN_PIX_FORMAT_RGB565 (0x4 << 12) > #define DSIM_SUB_VC (((x) & 0x3) << 16) > #define DSIM_MAIN_VC (((x) & 0x3) << 18) > -#define DSIM_HSA_MODE(1 << 20) > -#define DSIM_HBP_MODE(1 << 21) > -#define DSIM_HFP_MODE(1 << 22) > -#define DSIM_HSE_MODE(1 << 23) > +#define DSIM_HSA_DISABLE (1 << 20) > +#define DSIM_HBP_DISABLE (1 << 21) > +#define DSIM_HFP_DISABLE (1 << 22) > +#define DSIM_HSE_DISABLE (1 << 23) > #define DSIM_AUTO_MODE (1 << 24) > #define DSIM_VIDEO_MODE (1 << 25) > #define DSIM_BURST_MODE (1 << 26) > @@ -804,13 +804,13 @@ static int exynos_dsi_init_link(struct exynos_dsi *dsi) > if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_AUTO_VERT) > reg |= DSIM_AUTO_MODE; > if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_HSE) > - reg |= DSIM_HSE_MODE; > + reg |= DSIM_HSE_DISABLE; Please add a comment to explain that the DSIM_HSE_DISABLE bit as named in the datasheet actually has inverted logic (set = HSE enabled). > if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_NO_HFP) > - reg |= DSIM_HFP_MODE; > + reg |= DSIM_HFP_DISABLE; > if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_NO_HBP) > - reg |= DSIM_HBP_MODE; > + reg |= DSIM_HBP_DISABLE; > if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_NO_HSA) > - reg |= DSIM_HSA_MODE; > + reg |= DSIM_HSA_DISABLE; > } > > if (dsi->mode_flags & MIPI_DSI_MODE_NO_EOT_PACKET)
Re: [PATCH v8 01/14] drm: exynos: dsi: Fix MIPI_DSI*_NO_* mode flags
On 14.11.22 04:16, Marek Vasut wrote: > On 11/14/22 02:11, Nicolas Boichat wrote: >> On Sun, Nov 13, 2022 at 8:29 AM Marek Vasut wrote: >>> >>> On 11/11/22 13:12, Nicolas Boichat wrote: >>> >>> [...] >>> >>>>>> BTW, are you sure DSIM_HSE_MODE is correct now? >>>>> >>>>> Yes, we have tested in imx8m platforms as well. Sébastien Szymanski >>>>> initially observed this issue on the imx8m platform. >>>> >>>> I'll repeat, are you sure about HSE specifically? You invert the >>>> polarity for HBP, HFP, and HSA, which makes sense given your patch >>>> 02/14. >>>> >>>> I'm concerned about HSE. Is the bit really a disable bit? >>>> MIPI_DSI_MODE_VIDEO_HSE is supposed to be an enable flag, so you >>>> should not do `reg |= DSIM_HSE_DISABLE;`, probably. >>> >>> I suspect the HSE bit is a misnomer, but its handling in the driver is >>> correct. >>> >>> i.MX 8M Plus Applications Processor Reference Manual, Rev. 1, 06/2021 >>> Page 5436 >>> >>> 23 HseDisableMode >>> >>> In Vsync pulse and Vporch area, MIPI DSI master transfers only Hsync >>> start packet to MIPI DSI slave at MIPI DSI spec 1.1r02. This bit >>> transfers Hsync end packet in Vsync pulse and Vporch area (optional). >>> >>> 0 = Disables transfer >>> 1 = Enables transfer >>> >>> In command mode, this bit is ignored. >> >> Okay. I'd suggest adding a comment in the code, it'd be so tempting to >> attempt to "fix" this as the if/or pattern looks different from the >> others. >> >> But it's up to you all. > > I agree. Clearly the discrepancy is confusing and leads to mistakes. +1 for a comment in the code that explains the misnamed bit. Otherwise: Reviewed-By: Frieder Schrempf
Re: [PATCH v8 06/14] drm: bridge: samsung-dsim: Handle proper DSI host initialization
On 02.12.22 15:55, Dave Stevenson wrote:
> Hi Marek
>
> On Fri, 2 Dec 2022 at 12:21, Marek Vasut wrote:
>>
>> On 12/2/22 11:52, Marek Szyprowski wrote:
>>> Hi,
>>>
>>> Sorry for delay, I was on a sick leave last 2 weeks.
>>>
>>> On 28.11.2022 15:43, Jagan Teki wrote:
,On Sat, Nov 26, 2022 at 3:44 AM Marek Vasut wrote:
> On 11/23/22 21:09, Jagan Teki wrote:
>> On Sat, Nov 19, 2022 at 7:45 PM Marek Vasut wrote:
>>> On 11/17/22 14:04, Marek Szyprowski wrote:
On 17.11.2022 05:58, Marek Vasut wrote:
> On 11/10/22 19:38, Jagan Teki wrote:
>> DSI host initialization handling in previous exynos dsi driver has
>> some pitfalls. It initializes the host during host transfer() hook
>> that is indeed not the desired call flow for I2C and any other DSI
>> configured downstream bridges.
>>
>> Host transfer() is usually triggered for downstream DSI panels or
>> bridges and I2C-configured-DSI bridges miss these host initialization
>> as these downstream bridges use bridge operations hooks like
>> pre_enable,
>> and enable in order to initialize or set up the host.
>>
>> This patch is trying to handle the host init handler to satisfy all
>> downstream panels and bridges. Added the DSIM_STATE_REINITIALIZED
>> state
>> flag to ensure that host init is also done on first cmd transfer,
>> this
>> helps existing DSI panels work on exynos platform (form Marek
>> Szyprowski).
>>
>> v8, v7, v6, v5:
>> * none
>>
>> v4:
>> * update init handling to ensure host init done on first cmd transfer
>>
>> v3:
>> * none
>>
>> v2:
>> * check initialized state in samsung_dsim_init
>>
>> v1:
>> * keep DSI init in host transfer
>>
>> Signed-off-by: Marek Szyprowski
>> Signed-off-by: Jagan Teki
>> ---
>> drivers/gpu/drm/bridge/samsung-dsim.c | 25
>> +
>> include/drm/bridge/samsung-dsim.h | 5 +++--
>> 2 files changed, 20 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
>> b/drivers/gpu/drm/bridge/samsung-dsim.c
>> index bb1f45fd5a88..ec7e01ae02ea 100644
>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>> @@ -1234,12 +1234,17 @@ static void samsung_dsim_disable_irq(struct
>> samsung_dsim *dsi)
>> disable_irq(dsi->irq);
>> }
>> -static int samsung_dsim_init(struct samsung_dsim *dsi)
>> +static int samsung_dsim_init(struct samsung_dsim *dsi, unsigned int
>> flag)
>> {
>> const struct samsung_dsim_driver_data *driver_data =
>> dsi->driver_data;
>> +if (dsi->state & flag)
>> +return 0;
>> +
>> samsung_dsim_reset(dsi);
>> -samsung_dsim_enable_irq(dsi);
>> +
>> +if (!(dsi->state & DSIM_STATE_INITIALIZED))
>> +samsung_dsim_enable_irq(dsi);
>> if (driver_data->reg_values[RESET_TYPE] == DSIM_FUNCRST)
>> samsung_dsim_enable_lane(dsi, BIT(dsi->lanes) - 1);
>> @@ -1250,6 +1255,8 @@ static int samsung_dsim_init(struct
>> samsung_dsim *dsi)
>> samsung_dsim_set_phy_ctrl(dsi);
>> samsung_dsim_init_link(dsi);
>> +dsi->state |= flag;
>> +
>> return 0;
>> }
>> @@ -1269,6 +1276,10 @@ static void
>> samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
>> }
>> dsi->state |= DSIM_STATE_ENABLED;
>> +
>> +ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED);
>> +if (ret)
>> +return;
>> }
>> static void samsung_dsim_atomic_enable(struct drm_bridge
>> *bridge,
>> @@ -1458,12 +1469,9 @@ static ssize_t
>> samsung_dsim_host_transfer(struct mipi_dsi_host *host,
>> if (!(dsi->state & DSIM_STATE_ENABLED))
>> return -EINVAL;
>> -if (!(dsi->state & DSIM_STATE_INITIALIZED)) {
>> -ret = samsung_dsim_init(dsi);
>> -if (ret)
>> -return ret;
>> -dsi->state |= DSIM_STATE_INITIALIZED;
>> -}
>> +ret = samsung_dsim_init(dsi, DSIM_STATE_REINITIALIZED);
> This triggers full controller reset and reprogramming upon first
> command transfer, is such heavy handed reload really necessary ?
Yes it is, otherwise the proper DSI panels doesn't work with Ex
Re: [PATCH] drm/bridge: ti-sn65dsi83: Fix delay after reset deassert to match spec
On 24.11.22 12:00, Alexander Stein wrote:
> Am Dienstag, 22. November 2022, 09:12:18 CET schrieb Frieder Schrempf:
>> From: Frieder Schrempf
>>
>> The datasheet specifies a delay of 10 milliseconds, but the current
>> driver only waits for 1 ms. Fix this to make sure the initialization
>> sequence meets the spec.
>>
>> Fixes: ceb515ba29ba ("drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 and
>> SN65DSI84 driver") Signed-off-by: Frieder Schrempf
>>
>> ---
>> drivers/gpu/drm/bridge/ti-sn65dsi83.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>> b/drivers/gpu/drm/bridge/ti-sn65dsi83.c index 7ba9467fff12..047c14ddbbf1
>> 100644
>> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>> @@ -346,7 +346,7 @@ static void sn65dsi83_atomic_enable(struct drm_bridge
>> *bridge,
>>
>> /* Deassert reset */
>> gpiod_set_value_cansleep(ctx->enable_gpio, 1);
>> -usleep_range(1000, 1100);
>> +usleep_range(1, 11000);
>>
>> /* Get the LVDS format from the bridge state. */
>> bridge_state = drm_atomic_get_new_bridge_state(state, bridge);
>
> How about using fsleep?
>
> Either way:
> Reviewed-by: Alexander Stein
Thanks for the review! I didn't know about fsleep. Anyway this would
probably be a separate change as the driver currently uses usleep
everywhere else. But I will keep it in mind for the future.
[PATCH] drm/bridge: ti-sn65dsi83: Fix delay after reset deassert to match spec
From: Frieder Schrempf
The datasheet specifies a delay of 10 milliseconds, but the current
driver only waits for 1 ms. Fix this to make sure the initialization
sequence meets the spec.
Fixes: ceb515ba29ba ("drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 and SN65DSI84
driver")
Signed-off-by: Frieder Schrempf
---
drivers/gpu/drm/bridge/ti-sn65dsi83.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index 7ba9467fff12..047c14ddbbf1 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -346,7 +346,7 @@ static void sn65dsi83_atomic_enable(struct drm_bridge
*bridge,
/* Deassert reset */
gpiod_set_value_cansleep(ctx->enable_gpio, 1);
- usleep_range(1000, 1100);
+ usleep_range(1, 11000);
/* Get the LVDS format from the bridge state. */
bridge_state = drm_atomic_get_new_bridge_state(state, bridge);
--
2.38.1
Re: [PATCH v8 09/14] drm: bridge: samsung-dsim: Add atomic_get_input_bus_fmts
On 15.11.22 09:09, Marek Szyprowski wrote:
> Hi Jagan,
>
> On 14.11.2022 18:07, Jagan Teki wrote:
>> On Mon, Nov 14, 2022 at 8:10 PM Marek Szyprowski
>> wrote:
>>> On 14.11.2022 11:57, Marek Szyprowski wrote:
On 10.11.2022 19:38, Jagan Teki wrote:
> Finding the right input bus format throughout the pipeline is hard
> so add atomic_get_input_bus_fmts callback and initialize with the
> proper input format from list of supported output formats.
>
> This format can be used in pipeline for negotiating bus format between
> the DSI-end of this bridge and the other component closer to pipeline
> components.
>
> List of Pixel formats are taken from,
> AN13573 i.MX 8/RT MIPI DSI/CSI-2, Rev. 0, 21 March 2022
> 3.7.4 Pixel formats
> Table 14. DSI pixel packing formats
>
> v8:
> * added pixel formats supported by NXP AN13573 i.MX 8/RT MIPI DSI/CSI-2
>
> v7, v6, v5, v4:
> * none
>
> v3:
> * include media-bus-format.h
>
> v2:
> * none
>
> v1:
> * new patch
>
> Signed-off-by: Jagan Teki
> ---
>drivers/gpu/drm/bridge/samsung-dsim.c | 53 +++
>1 file changed, 53 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 0fe153b29e4f..33e5ae9c865f 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -15,6 +15,7 @@
>#include
>#include
>#include
> +#include
>#include
>#include
>@@ -1321,6 +1322,57 @@ static void
> samsung_dsim_atomic_post_disable(struct drm_bridge *bridge,
>pm_runtime_put_sync(dsi->dev);
>}
>+/*
> + * This pixel output formats list referenced from,
> + * AN13573 i.MX 8/RT MIPI DSI/CSI-2, Rev. 0, 21 March 2022
> + * 3.7.4 Pixel formats
> + * Table 14. DSI pixel packing formats
> + */
> +static const u32 samsung_dsim_pixel_output_fmts[] = {
> +MEDIA_BUS_FMT_UYVY8_1X16,
> +MEDIA_BUS_FMT_RGB101010_1X30,
> +MEDIA_BUS_FMT_RGB121212_1X36,
> +MEDIA_BUS_FMT_RGB565_1X16,
> +MEDIA_BUS_FMT_RGB666_1X18,
> +MEDIA_BUS_FMT_RGB888_1X24,
> +};
> +
> +static bool samsung_dsim_pixel_output_fmt_supported(u32 fmt)
> +{
> +int i;
> +
> +for (i = 0; i < ARRAY_SIZE(samsung_dsim_pixel_output_fmts); i++) {
> +if (samsung_dsim_pixel_output_fmts[i] == fmt)
> +return true;
> +}
> +
> +return false;
> +}
> +
> +static u32 *
> +samsung_dsim_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)
> +{
> +u32 *input_fmts;
> +
> +if (!samsung_dsim_pixel_output_fmt_supported(output_fmt))
> +return NULL;
Please add support for MEDIA_BUS_FMT_FIXED and maybe default to
MEDIA_BUS_FMT_RGB888_1X24 if requested format is not matched.
Otherwise the above check breaks all current clients of the Samsung
DSIM/Exynos DSI. I didn't dig into the bus matching code yet, but all
DSI panels requests such format on my test systems...
>>> I've checked a bit more the bus format related code and it looks that
>>> there are more issues. The DSI panels don't use the MEDIA_BUS_FMT_*
>>> formats thus the bridge negotiation code selects MEDIA_BUS_FMT_FIXED for
>>> them. On Arndale board with Toshiba tc358764 bridge the
>>> MEDIA_BUS_FMT_RGB888_1X7X4_SPWG output_fmt is requested in
>>> samsung_dsim_atomic_get_input_bus_fmts() (forwarded from the LVDS panel,
>> dsim => tc358764 => panel-simple
>>
>> If I understand the bridge format negotiation correctly - If
>> atomic_get_input_bus_fmts is not implemented in tc358764 then
>> MEDIA_BUS_FMT_FIXED will be the output_fmt for dsim so we can assign
>> MEDIA_BUS_FMT_RGB888_1X24 for FIXED formats.
>>
>> from include/drm/drm_bridge.h:
>>
>> * This method is called on all elements of the bridge chain as
>> part of
>> * the bus format negotiation process that happens in
>> * drm_atomic_bridge_chain_select_bus_fmts().
>> * This method is optional. When not implemented, the core will
>> bypass
>> * bus format negotiation on this element of the bridge without
>> * failing, and the previous element in the chain will be passed
>> * MEDIA_BUS_FMT_FIXED as its output bus format.
>>
>> As I can see tc358764 is not implemented either
>> atomic_get_input_bus_fmts or atomic API, so I think dsim gets the
>> MEDIA_BUS_FMT_FIXED bridge pipeline. I h
Re: [PATCH v8 00/14] drm: bridge: Add Samsung MIPI DSIM bridge
Hi Jagan, On 10.11.22 19:38, Jagan Teki wrote: > This series supports common bridge support for Samsung MIPI DSIM > which is used in Exynos and i.MX8MM SoC's. > > The final bridge supports both the Exynos and i.MX8M Mini/Nano/Plus. > > Changes for v8: > * fixed comment lines > * fixed commit messages > * fixed video mode bits > * collect Marek Ack > * fixed video mode bit names > * update input formats logic > * added imx8mplus support Did you miss to collect all the Tested-by tags from v7, or did you drop them deliberately? Best regards Frieder
Re: [PATCH v7 00/10] drm: bridge: Add Samsung MIPI DSIM bridge
On 05.10.22 17:12, Jagan Teki wrote: > This series supports common bridge support for Samsung MIPI DSIM > which is used in Exynos and i.MX8MM SoC's. > > The final bridge supports both the Exynos and i.MX8MM DSI devices. > > Changes for v7: > * fix the drm bridge attach chain for exynos drm dsi driver > * fix the hw_type checking logic > > Changes for v6: > * handle previous bridge for exynos dsi while attaching bridge > > Changes for v5: > * bridge changes to support multi-arch > * updated and clear commit messages > * add hw_type via plat data > * removed unneeded quirk > * rebased on linux-next > > Changes for v4: > * include Inki Dae in MAINTAINERS > * remove dsi_driver probe in exynos_drm_drv to support multi-arch build > * update init handling to ensure host init done on first cmd transfer > > Changes for v3: > * fix the mult-arch build > * fix dsi host init > * updated commit messages > > Changes for v2: > * fix bridge handling > * fix dsi host init > * correct the commit messages > > Patch 0001: Samsung DSIM bridge > > Patch 0002: PHY optional > > Patch 0003: OF-graph or Child node lookup > > Patch 0004: DSI host initialization > > Patch 0005: atomic check > > Patch 0006: PMS_P offset via plat data > > Patch 0007: atomic_get_input_bus_fmts > > Patch 0008: input_bus_flags > > Patch 0009: document fsl,imx8mm-mipi-dsim > > Patch 0010: add i.MX8MM DSIM support > > Tested in Engicam i.Core MX8M Mini SoM. > > Repo: > https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v7 > > Any inputs? > Jagan. I tested this on the Kontron DL i.MX8MM which uses a TI SN65DSI84 bridge and a Jenson 7" LVDS Display. Thanks for your work, Jagan! Tested-by: Frieder Schrempf # Kontron DL i.MX8MM
Re: [PATCH v4 02/12] drm: bridge: Add Samsung DSIM bridge driver
Hi Jagan, Marek, Am 07.09.22 um 12:04 schrieb Marek Szyprowski: > Hi Jagan, > > On 06.09.2022 21:07, Jagan Teki wrote: >> On Mon, Sep 5, 2022 at 4:54 PM Marek Szyprowski >> wrote: >>> On 02.09.2022 12:47, Marek Szyprowski wrote: On 29.08.2022 20:40, Jagan Teki wrote: > Samsung MIPI DSIM controller is common DSI IP that can be used in > various > SoCs like Exynos, i.MX8M Mini/Nano. > > In order to access this DSI controller between various platform SoCs, > the ideal way to incorporate this in the drm stack is via the drm bridge > driver. > > This patch is trying to differentiate platform-specific and bridge > driver > code and keep maintaining the exynos_drm_dsi.c code as platform-specific > glue code and samsung-dsim.c as a common bridge driver code. > > - Exynos specific glue code is exynos specific te_irq, host_attach, and > detach code along with conventional component_ops. > > - Samsung DSIM is a bridge driver which is common across all > platforms and > the respective platform-specific glue will initialize at the end > of the > probe. The platform-specific operations and other glue calls will > invoke > on associate code areas. > > v4: > * include Inki Dae in MAINTAINERS > * remove dsi_driver probe in exynos_drm_drv to support multi-arch build This breaks Exynos DRM completely as the Exynos DRM driver is not able to wait until the DSI driver is probed and registered as component. I will show how to rework this the way it is done in drivers/gpu/drm/exynos/exynos_dp.c and drivers/gpu/drm/bridge/analogix/analogix_dp_core.c soon... >>> I've finally had some time to implement such approach, see >>> https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fprotect2.fireeye.com%2Fv1%2Furl%3Fk%3Dc5d024d9-a4ab8e4e-c5d1af96-74fe4860001d-625a8324a9797375%26q%3D1%26e%3D489b94d4-84fb-408e-b679-a8d27acf2930%26u%3Dhttps%253A%252F%252Fgithub.com%252Fmszyprow%252Flinux%252Ftree%252Fv6.0-dsi-v4-reworked&data=05%7C01%7Cfrieder.schrempf%40kontron.de%7C8ed9bd90703b48c9d43708da90b86876%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C637981418959345104%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=vLwOMBbdNo1C5x%2BESY3SU%2FwaYKBmAgIyafLOLmd3BlM%3D&reserved=0 >>> >>> If you want me to send the patches against your v4 patchset, let me >>> know, but imho my changes are much more readable after squashing to the >>> original patches. >>> >>> Now the driver is fully multi-arch safe and ready for further >>> extensions. I've removed the weak functions, reworked the way the >>> plat_data is used (dropped the patch related to it) and restored >>> exynos-dsi driver as a part of the Exynos DRM drivers/subsystem. Feel >>> free to resend the above as v5 after testing on your hardware. At least >>> it properly works now on all Exynos boards I have, both compiled into >>> the kernel or as modules. >> Thanks. I've seen the repo added on top of Dave patches - does it mean >> these depends on Dave changes as well? > > Yes and no. My rework doesn't change anything with this dependency. It > comes from my patch "drm: exynos: dsi: Restore proper bridge chain > order" already included in your series (patch #1). Without it exynos-dsi > driver hacks the list of bridges to ensure the order of pre_enable calls > needed for proper operation. This works somehow with DSI panels on my > test systems, but it has been reported that it doesn't work with a bit > more complex display pipelines. Only that patch depends on the Dave's > patches. If you remove it, you would need to adjust the code in the > exynos_drm_dsi.c and samsung-dsim.c respectively. imho it would be > better to keep it and merge Dave's patches together with dsi changes, as > they are the first real client of it. I tried to test Jagan's v4 on our Kontron BL i.MX8MM with SN65DSI84 LVDS bridge and it didn't work due to driver dependency issues. Instead trying Marek's branch rebased to 6.0-rc4 and including the necessary DT changes from Jagan's v4 looks good and produces an image on the display. Only one thing caused problems: In Jagan's patch for the LCDIF node [1] there's a typo (missing 's') in the assigned-clock-rates property. Without fixing this the display doesn't work. Jagan, can you come up with a v5 including Marek's fixes? And if you like, I think you could also start sending the DT changes for i.MX8MM, so we can start reviewing/testing them? Thanks Frieder [1]: https://github.com/openedev/kernel/commit/89a6252cec47f3593d4f2b7ea3132f4745c4fdb3
Re: [PATCH V2 0/3] DSI host and peripheral initialisation ordering
Hi Dave, Am 06.07.22 um 12:27 schrieb Dave Stevenson: > Hi Frieder. > > Apologies Lucas - I missed your response. > > On Wed, 6 Jul 2022 at 08:09, Frieder Schrempf > wrote: >> >> Am 10.06.22 um 09:52 schrieb Lucas Stach: >>> Hi, >>> >>> Am Mittwoch, dem 11.05.2022 um 16:58 +0200 schrieb Marek Szyprowski: >>>> Hi Dave, >>>> >>>> On 05.04.2022 13:43, Dave Stevenson wrote: >>>>> On Fri, 18 Mar 2022 at 12:25, Dave Stevenson >>>>> wrote: >>>>>> On Fri, 4 Mar 2022 at 15:18, Dave Stevenson >>>>>> wrote: >>>>>>> Hi All >>>>>> A gentle ping on this series. Any comments on the approach? >>>>>> Thanks. >>>>> I realise the merge window has just closed and therefore folks have >>>>> been busy, but no responses on this after a month? >>>>> >>>>> Do I give up and submit a patch to document that DSI is broken and no one >>>>> cares? >>>> >>>> Thanks for pointing this patchset in the 'drm: bridge: Add Samsung MIPI >>>> DSIM bridge' thread, otherwise I would miss it since I'm not involved >>>> much in the DRM development. >>>> >>>> This resolves most of the issues in the Exynos DSI and its recent >>>> conversion to the drm bridge framework. I've added the needed >>>> prepare_upstream_first flags to the panels and everything works fine >>>> without the bridge chain order hacks. >>>> >>>> Feel free to add: >>>> >>>> Tested-by: Marek Szyprowski >>>> >>>> >>>> The only remaining thing to resolve is the moment of enabling DSI host. >>>> The proper sequence is: >>>> >>>> 1. host power on, 2. device power on, 3. host init, 4. device init, 5. >>>> video enable. >>>> >>>> #1 is done in dsi's pre_enable, #2 is done in panel's prepare. #3 was so >>>> far done in the first host transfer call, which usually happens in >>>> panel's prepare, then the #4 happens. Then video enable is done in the >>>> enable callbacks. >>>> >>>> Jagan wants to move it to the dsi host pre_enable() to let it work with >>>> DSI bridges controlled over different interfaces >>>> (https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2F20220504114021.33265-6-jagan%40amarulasolutions.com%2F&data=05%7C01%7Cfrieder.schrempf%40kontron.de%7Ca82478efb8434ac07dfb08da5f3a1b75%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C637927000416444951%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=vgnJ8L7fhloUV83p%2Be6ziUKHbL9apqcLWpDvMUcOOoY%3D&reserved=0 >>>> ). This however fails on Exynos with DSI panels, because when dsi's >>>> pre_enable is called, the dsi device is not yet powered. I've discussed >>>> this with Andrzej Hajda and we came to the conclusion that this can be >>>> resolved by adding the init() callback to the struct mipi_dsi_host_ops. >>>> Then DSI client (next bridge or panel) would call it after powering self >>>> on, but before sending any DSI commands in its pre_enable/prepare >>>> functions. >>>> >>>> I've prepared a prototype of such solution. This approach finally >>>> resolved all the initialization issues! The bridge chain finally matches >>>> the hardware, no hack are needed, and everything is controlled by the >>>> DRM core. This prototype also includes the Jagan's patches, which add >>>> IMX support to Samsung DSIM. If one is interested, here is my git repo >>>> with all the PoC patches: >>>> >>>> https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmszyprow%2Flinux%2Ftree%2Fv5.18-next-20220511-dsi-rework&data=05%7C01%7Cfrieder.schrempf%40kontron.de%7Ca82478efb8434ac07dfb08da5f3a1b75%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C637927000416444951%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=ka2wikbVhc4RxFGARjTh0ixGKiaNHloW9dVkejA5kEg%3D&reserved=0 >>> >>> While this needs rework on the bridge chip side, I fear that we need >>> something like that to allow the bridge to control the sequencing of >>> the DSI host init. While most bridges that aren't controlled via the >>> DSI channel might be fine with just in
Re: [PATCH V2 0/3] DSI host and peripheral initialisation ordering
Am 10.06.22 um 09:52 schrieb Lucas Stach: > Hi, > > Am Mittwoch, dem 11.05.2022 um 16:58 +0200 schrieb Marek Szyprowski: >> Hi Dave, >> >> On 05.04.2022 13:43, Dave Stevenson wrote: >>> On Fri, 18 Mar 2022 at 12:25, Dave Stevenson >>> wrote: On Fri, 4 Mar 2022 at 15:18, Dave Stevenson wrote: > Hi All A gentle ping on this series. Any comments on the approach? Thanks. >>> I realise the merge window has just closed and therefore folks have >>> been busy, but no responses on this after a month? >>> >>> Do I give up and submit a patch to document that DSI is broken and no one >>> cares? >> >> Thanks for pointing this patchset in the 'drm: bridge: Add Samsung MIPI >> DSIM bridge' thread, otherwise I would miss it since I'm not involved >> much in the DRM development. >> >> This resolves most of the issues in the Exynos DSI and its recent >> conversion to the drm bridge framework. I've added the needed >> prepare_upstream_first flags to the panels and everything works fine >> without the bridge chain order hacks. >> >> Feel free to add: >> >> Tested-by: Marek Szyprowski >> >> >> The only remaining thing to resolve is the moment of enabling DSI host. >> The proper sequence is: >> >> 1. host power on, 2. device power on, 3. host init, 4. device init, 5. >> video enable. >> >> #1 is done in dsi's pre_enable, #2 is done in panel's prepare. #3 was so >> far done in the first host transfer call, which usually happens in >> panel's prepare, then the #4 happens. Then video enable is done in the >> enable callbacks. >> >> Jagan wants to move it to the dsi host pre_enable() to let it work with >> DSI bridges controlled over different interfaces >> (https://lore.kernel.org/all/20220504114021.33265-6-ja...@amarulasolutions.com/ >> >> ). This however fails on Exynos with DSI panels, because when dsi's >> pre_enable is called, the dsi device is not yet powered. I've discussed >> this with Andrzej Hajda and we came to the conclusion that this can be >> resolved by adding the init() callback to the struct mipi_dsi_host_ops. >> Then DSI client (next bridge or panel) would call it after powering self >> on, but before sending any DSI commands in its pre_enable/prepare functions. >> >> I've prepared a prototype of such solution. This approach finally >> resolved all the initialization issues! The bridge chain finally matches >> the hardware, no hack are needed, and everything is controlled by the >> DRM core. This prototype also includes the Jagan's patches, which add >> IMX support to Samsung DSIM. If one is interested, here is my git repo >> with all the PoC patches: >> >> https://github.com/mszyprow/linux/tree/v5.18-next-20220511-dsi-rework > > While this needs rework on the bridge chip side, I fear that we need > something like that to allow the bridge to control the sequencing of > the DSI host init. While most bridges that aren't controlled via the > DSI channel might be fine with just initializing the host right before > a video signal is driven, there are some that need a different > sequencing. > > The chip I'm currently looking at is a TC368767, where the datasheet > states that the DSI lanes must be in LP-11 before the reset is > released. While the datasheet doesn't specify what happens if that > sequence is violated, Marek Vasut found that the chip enters a test > mode if the lanes are not in LP-11 at that point and I can confirm this > observation. The SN65DSI84 also has this requirement according to the datasheet. > Now with the TC358767 being a DSI to (e)DP converter, we need to > release the chip from reset pretty early to establish the DP AUX > connection to communicate with the display, in order to find out which > video modes we can drive. As the chip is controlled via i2c in my case, > initializing the DSI host on first DSI command transaction is out and > doing so before the bridge pre_enable is way too late. > > What I would need for this chip to work properly is an explicit call, > like the mipi_dsi_host_init() added in the PoC above, to allow the > bridge driver to kick the DSI host initialization before releasing the > chip from reset state. So to sum up on the missing parts: 1. This series needs more reviews, but is generally agreed on. 2. Jagan will integrate Marek's mipi_dsi_host_init() PoC when respinning his series "drm: bridge: Add Samsung MIPI DSIM bridge". 3. Bridge drivers need to be adjusted to call mipi_dsi_host_init() if required. Did I get anything wrong here, or is there some point that I'm missing? Jagan, do you have any plan to pick up the threads? Thanks Frieder
Re: [PATCH][RESEND] drm/bridge: ti-sn65dsi83: Check link status register after enabling the bridge
Am 05.04.22 um 23:36 schrieb Marek Vasut: > On 4/5/22 17:24, Dave Stevenson wrote: > > Hi, > >> If we can initialise the DSI host before the bridge for the >> pre_enable, then all the configuration moves to the atomic_pre_enable >> and there should be no need to have the delay. >> >> I can't 100% guarantee that, but one of the folks on the Pi forums is >> using [1] which does that, and is reporting it working well. (He's >> also using the DSI85 to take 2 DSI links and drive 2 LVDS single link >> panels) > > It seems to me that checking whether the bridge got correctly > initialized is orthogonal to the aforementioned patchset though ? It's the delay that is ugly. >>> >>> You do need to wait a little after the initialization and before >>> checking the status, so that delay is not going away no matter how you >>> frob with the DSI bridge. >>> Put the check in atomic_enable which will be slightly later than configuration in pre_enable? Check that sufficient jiffies have passed if you needed. Or wire up the IRQ line from the SN65DSI83 rather than polling the IRQ Status register. Delayed workqueue if the IRQ isn't wired up. >>> >>> Are you able to do such deferred non-atomic operations in atomic_enable >>> callback ? >> >> atomic_enable returns void so you can't fail the atomic_enable. >> All you're doing is reading a register and potentially logging an >> error - surely that can be done from any context. >> If I read it right your log message is triggered by any bit being set in REG_IRQ_STAT. So an inconveniently timed correctable DSI error will set bit 4 and log the error even though it's been corrected. Likewise bit 7 / CHA_SYNCH_ERR could get triggered by an H or V sync packet being received in that 10-12ms window (we're in atomic_enable, so video is already running). >>> >>> There should be no bits set in the IRQ_STAT register if everything works >>> as it should. >> >> On a perfect link, yes. > > If your hardware is broken, then you really do want to know about it. > >> Looking at the top 4 bits. >> >> Bit 4 >> CHA_COR_ECC_ERR >> When the DSI channel A packet processor detects a correctable ECC >> error, this bit is set. >> >> The error is corrected, so why do we care? The display is still working. > > If you get a lot of those correctable errors, your display might not > work at all. So we do care. > >> Bit 5 >> CHA_UNC_ECC_ERR >> When the DSI channel A packet processor detects an uncorrectable ECC >> error, this bit is set; >> Bit 6 >> CHA_CRC_ERR >> When the DSI channel A packet processor detects a data stream CRC error, >> this bit is set >> >> It doesn't say what behaviour the DSI83 will take under these >> circumstances, but shouldn't be fatal to the display. > > See above, it is an error, hardware is broken, you want to know about > this and fix the hardware. > >> Bit 7 >> CHA_SYNCH_ERR >> When the DSI channel A packet processor detects an HS or VS >> synchronization error, that is, an unexpected sync packet; this bit is >> set. >> >> It's happened, but shouldn't be fatal, so why do we care? The display >> should pick up correctly at the start of the next frame. > > Or maybe it won't because of noise on the DSI link, fix the hardware. > > Sorry, I am not happy about hiding errors and trying to somehow justify > they are OK. They are not, the hardware is likely broken and it should > be fixed, that is the right way to handle these errors. > >> As I've already said, we're in atomic_enable and video is therefore >> running, this is highly plausibly going to happen. We've delayed for >> 4-5ms in sn65dsi83_atomic_enable, so we're a third of the way through >> a frame at 60fps. The chances of seeing a VS or HS packet at an >> unexpected time is therefore high. >> >> Bits 2 & 3 look equally inconsequential. >> >> Bit 0 as PLL unlock would cause grief. >> If it's the PLL being unlocked that is the issue then it should only be checking bit 0. Or possibly reading the actual PLL lock status from REG_RC_LVDS_PLL_PLL_EN_STAT. Although as we've already checked that the PLL is locked via REG_RC_LVDS_PLL_PLL_EN_STAT earlier in the atomic_enable, I'm now a little confused as to the condition you are actually wanting to detect. >>> >>> Any outstanding errors which are currently hidden and only show up >>> sporadically at the worst possible moment. >> >> If you were constantly looking at the status then that would be >> reasonable. >> To be looking during one specific 10-12ms period of time for some >> fairly generic status values seems a little redundant, and a waste of >> time in delaying the atomic_enable. > > Sorry, I disagree. I think 10ms extra time in atomic_enable() is a good > trade-off for knowing about possible hardware problems sooner rather > than later. Isn't the delay at this point even required (regardless of the debugging information), as the init se
Re: [PATCH v6 0/6] drm: exynos: dsi: Convert drm bridge
Am 10.03.22 um 14:03 schrieb Frieder Schrempf: > Hi Jagan, > > Am 09.03.22 um 15:01 schrieb Jagan Teki: >> Hi Frieder, >> >> On Wed, Mar 9, 2022 at 6:54 PM Frieder Schrempf >> wrote: >>> >>> Hi Jagan, >>> >>> Am 03.03.22 um 17:36 schrieb Jagan Teki: >>>> Updated series about drm bridge conversion of exynos dsi. >>>> >>>> Previous version can be accessible, here [1]. >>>> >>>> Patch 1: tc358764 panel_bridge API >>>> >>>> Patch 2: connector reset >>>> >>>> Patch 3: bridge attach in MIC >>>> >>>> Patch 4: panel_bridge API >>>> >>>> Patch 5: bridge conversion >>>> >>>> Patch 6: atomic functions >>>> >>>> [1] >>>> https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.amarulasolutions.com%2Fcover%2F1839&data=04%7C01%7Cfrieder.schrempf%40kontron.de%7Cc99f637dd67444dfc38208da01d55963%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C637824313083236643%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=qF5bVwelZ35cQQygW3PvPUZkQlyFalUDsyDVjDnngiU%3D&reserved=0 >>>> >>>> Any inputs? >>> >>> Thanks for your efforts. I didn't follow the whole history, but I'm >>> looking forward and hope to see upstream support for the i.MX8MM DSIM in >>> the not too distant future. >>> >>> Can you give me a short update about the state of this patchset? Are >>> there still any major obstacles? >>> >>> I can't help with testing on Exynos, but if you have the matching >>> follow-up patches for i.MX8MM support somewhere around I could do some >>> tests with those on i.MX8MM. >> >> Unfortunately, it is getting slow due to existing exynos dsi drivers. >> Idea is to push exynos and then move the bridge as per Mailing-list >> discussion. I have initial series to support i.MX8MM on linux-next [1] >> which is working on my setup. However I'm waiting for this series to >> move further to send those on the mailing list. Indeed I'm solely >> relaying on Marek testing to move further as I too don't have Exynos >> hardware to validate. > > Thanks for the status update. Let's hope Marek or others with access to > the hardware can provide further testing. > > And thanks for providing the git tree for i.MX8MM. I will try to do some > tests on our hardware. Sorry, forgot to say that if you could cc me on future iterations of this patchset and the upcoming i.MX8MM patches, that would be great, thanks!
