Re: [PATCH 1/5] Documentation: bindings: clk: Add bindings for i.MX8MM BLK_CTL

[Frieder Schrempf <frieder.schrempf@kontron.de>]

Hi Tim,

On 16.12.20 22:24, Tim Harvey wrote:
>   'On Thu, Dec 10, 2020 at 7:15 AM Frieder Schrempf
> <frieder.schrempf@kontron.de> wrote:
>>
>> Hi,
>>
>> On 30.11.20 16:43, Adam Ford wrote:
>>> On Mon, Nov 30, 2020 at 5:47 AM Frieder Schrempf
>>> <frieder.schrempf@kontron.de> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 07.10.20 22:50, Marek Vasut wrote:
>>>>> On 10/7/20 10:17 PM, Adam Ford wrote:
>>>>>> On Wed, Oct 7, 2020 at 3:08 PM Adam Ford <aford173@gmail.com> wrote:
>>>>>>>
>>>>>>> On Wed, Oct 7, 2020 at 3:03 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>>>
>>>>>>>> On 10/7/20 9:52 PM, Adam Ford wrote:
>>>>>>>>> On Sun, Oct 4, 2020 at 12:53 AM Marek Vasut <marex@denx.de> wrote:
>>>>>>>>>>
>>>>>>>>>> Add the i.MX8MM BLK_CTL compatible string to the list.
>>>>>>>> [...]
>>>>>>>>>> ---
>>>>>>>>>>     Documentation/devicetree/bindings/clock/fsl,imx-blk-ctl.yaml | 1 +
>>>>>>>>>>     1 file changed, 1 insertion(+)
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Is there a DTSI change part of this patch?  I was going to try to test
>>>>>>>>> it, but  I am not seeing a change to the imx8mm.dtsi, and I am not
>>>>>>>>> sure where to put the node.
>>>>>>>>
>>>>>>>> There are in fact quite a few other pieces you need to have in place,
>>>>>>>> this patchset in itself is not particularly useful, it is just infra for
>>>>>>>> the LCDIF and MIPI DSIM block control. You might want to wait until they
>>>>>>>> all land in next and test that result.
>>>>>>>
>>>>>>> I have several patches in place, the GPCv2, this block driver,
>>>>>>> enabling GPU DT node, I'm also working on the DSIM patch you posted.
>>>>>>> I was hoping to test them all together and reply to the various
>>>>>>> threads with tested-by.  I also want to get my device tree stuff ready
>>>>>>> on the beacon boards so when everything lands, I can post DTS updates
>>>>>>> to enable the LCDIF, DSI, and the HDMI bridge.
>>>>>>>
>>>>>>> If you have a repo somewhere that has all these combined, I can just
>>>>>>> work on the final layer to enable the device tree plumbing on my
>>>>>>> board.  I just need the imx8mm.dtsi changes for this, DSIM, and the
>>>>>>> LCDIF so I can finish the task.
>>>>>>
>>>>>> On that note, I also have a i.MX8M Nano board which is similar to my
>>>>>> 8MM.  If I understood the 8MM clock block driver better, I hope to
>>>>>> adapt your changes for the Nano too.  Once the GPCv2 driver is
>>>>>> accepted, I was also going to look at updating it to support the Nano
>>>>>> as well which also has the same DSIM and LCDIF as the 8MM as well and
>>>>>> a better GPU than the Mini but lacking the VPU.
>>>>>
>>>>> I don't have a branch, but I sent you the collected patches off-list.
>>>>>
>>>>
>>>> I would also be interested in the patch collection for BLK_CTL, DSIM,
>>>> etc. Marek, would you mind sending me those, too?
>>>>
>>>> Adam, did you already set up a branch and do some tests with the full stack?
>>>
>>> Frieder,
>>>
>>> I have been monitoring some of the activity on the BLK_CTL.  It seems
>>> like there is some disagreement on how to connect the power domain
>>> controller with the BLK_CTL.  Someone reported that it causes a hang
>>> on the 8MP, so until that gets resolved, I doubt we'll be able to use
>>> the display system.  Some of the DSIM changes happening are being
>>> pushed back for further changes, so it seems like having the full
>>> integration might be a while.
>>
>> I have pulled all the latest patches, including Marek's off-list patches
>> together in one branch based on v5.10-rc7 [1] if anyone is interested.
>>
>> I added some fixes on top, that I needed to get my display behind
>> another Toshiba DSI-DPI bridge working. Those are probably not
>> upstreamable at all and need further investigation.
>>
>> I'm hoping to reply to the individual threads for more feedback. I see
>> that there are some blocking issues, but we hopefully get them resolved
>> somehow.
>>
>> Thanks
>> Frieder
>>
>> [1] https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ffschrempf%2Flinux%2Fcommits%2Fv5.10-mx8mm-graphics&amp;data=04%7C01%7Cfrieder.schrempf%40kontron.de%7C4b06e39a1030405300be08d8a20913f6%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C637437507175831939%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=n1Y4HwamMfPukFuVEurgt7KOM9SMLkercFhgFMuE6ro%3D&amp;reserved=0
>>
> 
> Frieder,
> 
> Thanks for sharing your repo as it's getting hard to track these
> patchsets (gpc/blk-ctl/power-domain/exynos/dsim). I'm also working on
> display support for IMX8MM and in my case I'm trying to connect to a
> RaspberryPi 7in display which I see Marek has been doing some work on
> to split out drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c to
> separate bridge, regulator/backlight, and simple-panel driver.

thanks for the feedback and good to know of other people caring about 
upstream support.

> 
> Marek,
> 
> Thanks for your recent work on splitting out the rpi display driver so
> that it can be bound via device-tree. I have found that I need to move
> the tc358762_init to enable vs pre-enable when using it with the
> in-progress samsung-dsim driver else the driver fails writes due to
> not being enabled yet:
> diff --git a/drivers/gpu/drm/bridge/tc358762.c
> b/drivers/gpu/drm/bridge/tc358762.c
> index 1bfdfc6..0d88e61 100644
> --- a/drivers/gpu/drm/bridge/tc358762.c
> +++ b/drivers/gpu/drm/bridge/tc358762.c
> @@ -153,11 +153,17 @@ static void tc358762_pre_enable(struct drm_bridge *bridge)
>          if (ret < 0)
>                  dev_err(ctx->dev, "error enabling regulators (%d)\n", ret);
> 
> +       ctx->pre_enabled = true;
> +}
> +
> +static void tc358762_enable(struct drm_bridge *bridge)
> +{
> +       struct tc358762 *ctx = bridge_to_tc358762(bridge);
> +       int ret;
> +
>          ret = tc358762_init(ctx);
>          if (ret < 0)
>                  dev_err(ctx->dev, "error initializing bridge (%d)\n", ret);
> -
> -       ctx->pre_enabled = true;
>   }
> 
>   static int tc358762_attach(struct drm_bridge *bridge,
> @@ -172,6 +178,7 @@ static int tc358762_attach(struct drm_bridge *bridge,
>   static const struct drm_bridge_funcs tc358762_bridge_funcs = {
>          .post_disable = tc358762_post_disable,
>          .pre_enable = tc358762_pre_enable,
> +       .enable = tc358762_enable,
>          .attach = tc358762_attach,
>   };
> 
> Frieder, I did find that your "drm/exynos: Fix PLL PMS offset for P
> value bitfield" patch breaks the samsung_dsim_host_transfer for me
> with the tc358762 bridge in the rpi panel. If I have that patch I get
> a timeout on the transfer with some added debugging:
> [    4.386387] tc358762_write 0x0210=0x00000003 0
> [    4.387031] samsung_dsim_host_transfer ret: 0
> [    4.387038] tc358762_write 0x0164=0x00000005 0
> [    4.387375] samsung_dsim_host_transfer ret: 0
> [    4.387379] tc358762_write 0x0168=0x00000005 0
> [    4.387409] samsung_dsim_host_transfer ret: 0
> [    4.387413] tc358762_write 0x0144=0x00000000 0
> [    4.387741] samsung_dsim_host_transfer ret: 0
> [    4.387745] tc358762_write 0x0148=0x00000000 0
> [    4.387773] samsung_dsim_host_transfer ret: 0
> [    4.387777] tc358762_write 0x0114=0x00000003 0
> [    4.387804] samsung_dsim_host_transfer ret: 0
> [    4.387808] tc358762_write 0x0450=0x00000000 0
> [    4.387834] samsung_dsim_host_transfer ret: 0
> [    4.387838] tc358762_write 0x0420=0x00100150 0
> [    4.388168] samsung_dsim_host_transfer ret: 0
> [    4.388172] tc358762_write 0x0464=0x0000040f 0
> [    4.388200] samsung_dsim_host_transfer ret: 0
> [    4.493346] tc358762_write 0x0104=0x00000001 0
> [    5.509341] imx-dsim-dsi 32e10000.mipi_dsi: xfer timed out: 29 06
> 00 00 04 01 01 00 00 00
> [    5.509345] samsung_dsim_host_transfer ret: -110
> [    5.509348] tc358762_write mipi_dsi_generic_write failed err=-110
> [    5.509352] tc358762_write 0x0204=0x00000001 -110
> [    5.617336] tc358762_init failed err=-110
> [    5.617344] tc358762 32e10000.mipi_dsi.0: error initializing bridge (-110)
> 
> Here is your patch which causes this issue for me:
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> b/drivers/gpu/drm/bridge/samsung-dsim.c
> index cb1ec3c..fc7c1d0 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -174,7 +174,7 @@
>   /* DSIM_PLLCTRL */
>   #define DSIM_FREQ_BAND(x)              ((x) << 24)
>   #define DSIM_PLL_EN                    (1 << 23)
> -#define DSIM_PLL_P(x)                  ((x) << 13)
> +#define DSIM_PLL_P(x)                  ((x) << 14)
>   #define DSIM_PLL_M(x)                  ((x) << 4)
>   #define DSIM_PLL_S(x)                  ((x) << 1)

As I already mentioned in the commit message of this change, I have no 
idea how the "correct" fix should look like or if there even is anything 
to fix here at all. It's just what I needed to get my setup working and 
I found it really odd that the NXP vendor implementation differs from 
the upstream Exynos driver in this place.

I have some other hardware setups with different bridges (LVDS/HDMI) 
behind the DSI and if I find some time, I will try them and see if they 
behave differently. Unfortunately I don't have any hardware to connect 
the RPi display to the i.MX8MM to test your setup.

Best regards
Frieder

> 
> I'm not very knowledgeable about MIPI DSI and find it strange that
> several writes in tc35872_init succeed until the failing writes:
>          tc358762_write(ctx, PPI_STARTPPI, PPI_START_FUNCTION);
>          tc358762_write(ctx, DSI_STARTDSI, DSI_RX_START);
> 
> For what its worth I've backported Marek's rpi backlight/reglator and
> simple-pannel driver to the NXP imx_5.4.47_2.2.0 kernel and do not see
> any MIPI DSI write failure there, although I have the same behavior of
> the display not showing anything.
> 
> Marek, are you using the rpi panel with IMX8MM? While I now have the
> drivers probing without error and have a functional backlight,
> regulator I see nothing on the display.