From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id B2ADDC433FE for ; Tue, 18 Jan 2022 16:59:16 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1346883AbiARQ7Q convert rfc822-to-8bit (ORCPT ); Tue, 18 Jan 2022 11:59:16 -0500 Received: from aposti.net ([89.234.176.197]:56650 "EHLO aposti.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231494AbiARQ7O (ORCPT ); Tue, 18 Jan 2022 11:59:14 -0500 Date: Tue, 18 Jan 2022 16:58:58 +0000 From: Paul Cercueil Subject: Re: [PATCH v5 2/7] drm/ingenic: Add support for JZ4780 and HDMI output To: "H. Nikolaus Schaller" Cc: Mark Rutland , Paul Boddie , Geert Uytterhoeven , Neil Armstrong , David Airlie , dri-devel , linux-mips , Andrzej Hajda , Laurent Pinchart , Miquel Raynal , Sam Ravnborg , Jernej Skrabec , Harry Wentland , OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS , Kees Cook , Jonas Karlman , Mark Brown , Maxime Ripard , Thomas Bogendoerfer , Liam Girdwood , Robert Foss , linux-kernel , Rob Herring , "Eric W. Biederman" , Daniel Vetter , Hans Verkuil , Discussions about the Letux Kernel Message-Id: In-Reply-To: References: <2c7d0aa7d3ef480ebb996d37c27cbaa6f722728b.1633436959.git.hns@goldelico.com> <7CEBB741-2218-40A7-9800-B3A154895274@goldelico.com> <229EBE4C-6555-41DE-962F-D82798AEC650@goldelico.com> <5BC61397-AF28-45CD-83F6-FA2C760F3995@goldelico.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1; format=flowed Content-Transfer-Encoding: 8BIT Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Nikolaus, Le mar., janv. 18 2022 at 15:50:01 +0100, H. Nikolaus Schaller a écrit : > Hi Paul, > any insights on the JZ_REG_LCD_OSDC issue below? Sorry, I missed your previous email. I blame the holidays ;) > There is a second, unrelated issue with the introduction of > > "drm/bridge: display-connector: implement bus fmts callbacks" > > which breaks bus format negotiations. > > These are the two last unsolved issues to submit a fully working > driver. > >> Am 22.12.2021 um 15:03 schrieb H. Nikolaus Schaller >> : >> >>> Am 08.11.2021 um 10:37 schrieb Paul Cercueil >>> : >>> >>> Hi Nikolaus, >>> >>> Le dim., nov. 7 2021 at 21:25:38 +0100, H. Nikolaus Schaller >>> a écrit : >>>> Hi Paul, >>>>>>>> >>>>>>>> @@ -1274,7 +1319,7 @@ static int ingenic_drm_bind(struct >>>>>>>> device *dev, bool has_components) >>>>>>>> /* Enable OSD if available */ >>>>>>>> if (soc_info->has_osd) >>>>>>>> - regmap_write(priv->map, JZ_REG_LCD_OSDC, >>>>>>>> JZ_LCD_OSDC_OSDEN); >>>>>>>> + regmap_set_bits(priv->map, JZ_REG_LCD_OSDC, >>>>>>>> JZ_LCD_OSDC_OSDEN); >>>>>>> This change is unrelated to this patch, and I'm not even sure >>>>>>> it's a valid change. The driver shouldn't rely on previous >>>>>>> register values. >>>>>> I think I already commented that I think the driver should also >>>>>> not reset >>>>>> previous register values to zero. >>>>> You did comment this, yes, but I don't agree. The driver >>>>> *should* reset the registers to zero. It should *not* have to >>>>> rely on whatever was configured before. >>>>> Even if I did agree, this is a functional change unrelated to >>>>> JZ4780 support, so it would have to be splitted to its own patch. >>>> Well it is in preparation of setting more bits that are only >>>> available for the jz4780. >>>> But it will be splitted into its own patch for other reasons - if >>>> we ever make osd working... >>>>>> If I counted correctly this register has 18 bits which seem to >>>>>> include >>>>>> some interrupt masks (which could be initialized somewhere >>>>>> else) and we >>>>>> write a constant 0x1. >>>>>> Of course most other bits are clearly OSD related (alpha >>>>>> blending), >>>>>> i.e. they can have any value (incl. 0) if OSD is disabled. But >>>>>> here we >>>>>> enable it. I think there may be missing some setting for the >>>>>> other bits. >>>>>> So are you sure, that we can unconditionally reset *all* bits >>>>>> except JZ_LCD_OSDC_OSDEN for the jz4780? >>>>>> Well I have no experience with OSD being enabled at all. I.e. I >>>>>> have no >>>>>> test scenario. >> >> It turns out that the line >> >> ret = clk_prepare_enable(priv->lcd_clk); >> >> initializes JZ_REG_LCD_OSDC to 0x00010005 (i.e. printk tells 0x0 >> before). > > more detailled test shows that it is the underlying > > clk_enable(priv->lcd_clk) > > (i.e. not the prepare). >> >> and writing >> >> regmap_write(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN); >> >> overwrites it with 0x00000001. >> >> This makes the HDMI monitor go/stay black until I write manually >> 0x5 to >> JZ_REG_LCD_OSDC. >> >> This means that JZ_LCD_OSDC_ALPHAEN must be enabled on jz4780 as >> well. >> Hence overwriting just with JZ_LCD_OSDC_OSDEN breaks it. >> >> Now the questions: >> a) 0x5 is understandable that it sets JZ_LCD_OSDC_ALPHAEN - but why >> is it needed? >> Is this a not well documented difference between jz4740/60/70/80? >> b) how can clk_prepare_enable() write 0x00010005 to >> JZ_REG_LCD_OSDC? Bug or feature? >> Something in cgu driver going wrong? > > I now suspect that it is an undocumented SoC feature. Not at all. If the clock is disabled, the LCD controller is disabled, so all the registers read zero, this makes sense. You can only read the registers when the clock is enabled. On some SoCs, reading disabled registers can even cause a complete lockup. Why is this JZ_LCD_OSDC_ALPHAEN bit needed now? I remember it working fine last time I tried, and now I indeed get a black screen unless this bit is set. The PM doesn't make it obvious that the bit is required, but that wouldn't be surprising. Anyway, feel free to send a patch to fix it (with a Fixes: tag). Ideally something like this: u32 osdc = 0; ... if (soc_info->has_osd) osdc |= JZ_LCD_OSDC_OSDEN; if (soc_info->has_alpha) osdc |= JZ_LCD_OSDC_ALPHAEN; regmap_write(priv->map, JZ_REG_LCD_OSDC, osdc); This also ensures that the OSDC register is properly initialized in the !has_osd case. The driver shouldn't rely on pre-boot values in the registers. Cheers, -Paul > >> c) what do your SoC or panels do if you write 0x5 to >> JZ_REG_LCD_OSDC? >> d) 0x00010005 also has bit 16 set which is undocumented... But this >> is a don't care >> according to jz4780 PM. > > BR and thanks, > Nikolaus > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id BE0D8C4332F for ; Tue, 18 Jan 2022 16:59:17 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 3187F10E11D; Tue, 18 Jan 2022 16:59:17 +0000 (UTC) Received: from aposti.net (aposti.net [89.234.176.197]) by gabe.freedesktop.org (Postfix) with ESMTPS id E021910E11D for ; Tue, 18 Jan 2022 16:59:15 +0000 (UTC) Date: Tue, 18 Jan 2022 16:58:58 +0000 From: Paul Cercueil Subject: Re: [PATCH v5 2/7] drm/ingenic: Add support for JZ4780 and HDMI output To: "H. Nikolaus Schaller" Message-Id: In-Reply-To: References: <2c7d0aa7d3ef480ebb996d37c27cbaa6f722728b.1633436959.git.hns@goldelico.com> <7CEBB741-2218-40A7-9800-B3A154895274@goldelico.com> <229EBE4C-6555-41DE-962F-D82798AEC650@goldelico.com> <5BC61397-AF28-45CD-83F6-FA2C760F3995@goldelico.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1; format=flowed Content-Transfer-Encoding: quoted-printable X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , Paul Boddie , Geert Uytterhoeven , Neil Armstrong , David Airlie , dri-devel , linux-mips , Andrzej Hajda , Laurent Pinchart , Miquel Raynal , Sam Ravnborg , Jernej Skrabec , OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS , Kees Cook , Jonas Karlman , Liam Girdwood , Mark Brown , Maxime Ripard , Discussions about the Letux Kernel , Thomas Bogendoerfer , linux-kernel , Robert Foss , Rob Herring , "Eric W. Biederman" , Hans Verkuil Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" Hi Nikolaus, Le mar., janv. 18 2022 at 15:50:01 +0100, H. Nikolaus Schaller=20 a =E9crit : > Hi Paul, > any insights on the JZ_REG_LCD_OSDC issue below? Sorry, I missed your previous email. I blame the holidays ;) > There is a second, unrelated issue with the introduction of >=20 > "drm/bridge: display-connector: implement bus fmts callbacks" >=20 > which breaks bus format negotiations. >=20 > These are the two last unsolved issues to submit a fully working=20 > driver. >=20 >> Am 22.12.2021 um 15:03 schrieb H. Nikolaus Schaller=20 >> : >>=20 >>> Am 08.11.2021 um 10:37 schrieb Paul Cercueil=20 >>> : >>>=20 >>> Hi Nikolaus, >>>=20 >>> Le dim., nov. 7 2021 at 21:25:38 +0100, H. Nikolaus Schaller=20 >>> a =E9crit : >>>> Hi Paul, >>>>>>>>=20 >>>>>>>> @@ -1274,7 +1319,7 @@ static int ingenic_drm_bind(struct=20 >>>>>>>> device *dev, bool has_components) >>>>>>>> /* Enable OSD if available */ >>>>>>>> if (soc_info->has_osd) >>>>>>>> - regmap_write(priv->map, JZ_REG_LCD_OSDC,=20 >>>>>>>> JZ_LCD_OSDC_OSDEN); >>>>>>>> + regmap_set_bits(priv->map, JZ_REG_LCD_OSDC,=20 >>>>>>>> JZ_LCD_OSDC_OSDEN); >>>>>>> This change is unrelated to this patch, and I'm not even sure=20 >>>>>>> it's a valid change. The driver shouldn't rely on previous=20 >>>>>>> register values. >>>>>> I think I already commented that I think the driver should also=20 >>>>>> not reset >>>>>> previous register values to zero. >>>>> You did comment this, yes, but I don't agree. The driver=20 >>>>> *should* reset the registers to zero. It should *not* have to=20 >>>>> rely on whatever was configured before. >>>>> Even if I did agree, this is a functional change unrelated to=20 >>>>> JZ4780 support, so it would have to be splitted to its own patch. >>>> Well it is in preparation of setting more bits that are only=20 >>>> available for the jz4780. >>>> But it will be splitted into its own patch for other reasons - if=20 >>>> we ever make osd working... >>>>>> If I counted correctly this register has 18 bits which seem to=20 >>>>>> include >>>>>> some interrupt masks (which could be initialized somewhere=20 >>>>>> else) and we >>>>>> write a constant 0x1. >>>>>> Of course most other bits are clearly OSD related (alpha=20 >>>>>> blending), >>>>>> i.e. they can have any value (incl. 0) if OSD is disabled. But=20 >>>>>> here we >>>>>> enable it. I think there may be missing some setting for the=20 >>>>>> other bits. >>>>>> So are you sure, that we can unconditionally reset *all* bits >>>>>> except JZ_LCD_OSDC_OSDEN for the jz4780? >>>>>> Well I have no experience with OSD being enabled at all. I.e. I=20 >>>>>> have no >>>>>> test scenario. >>=20 >> It turns out that the line >>=20 >> ret =3D clk_prepare_enable(priv->lcd_clk); >>=20 >> initializes JZ_REG_LCD_OSDC to 0x00010005 (i.e. printk tells 0x0=20 >> before). >=20 > more detailled test shows that it is the underlying >=20 > clk_enable(priv->lcd_clk) >=20 > (i.e. not the prepare). >>=20 >> and writing >>=20 >> regmap_write(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN); >>=20 >> overwrites it with 0x00000001. >>=20 >> This makes the HDMI monitor go/stay black until I write manually=20 >> 0x5 to >> JZ_REG_LCD_OSDC. >>=20 >> This means that JZ_LCD_OSDC_ALPHAEN must be enabled on jz4780 as=20 >> well. >> Hence overwriting just with JZ_LCD_OSDC_OSDEN breaks it. >>=20 >> Now the questions: >> a) 0x5 is understandable that it sets JZ_LCD_OSDC_ALPHAEN - but why=20 >> is it needed? >> Is this a not well documented difference between jz4740/60/70/80? >> b) how can clk_prepare_enable() write 0x00010005 to=20 >> JZ_REG_LCD_OSDC? Bug or feature? >> Something in cgu driver going wrong? >=20 > I now suspect that it is an undocumented SoC feature. Not at all. If the clock is disabled, the LCD controller is disabled,=20 so all the registers read zero, this makes sense. You can only read the=20 registers when the clock is enabled. On some SoCs, reading disabled=20 registers can even cause a complete lockup. Why is this JZ_LCD_OSDC_ALPHAEN bit needed now? I remember it working=20 fine last time I tried, and now I indeed get a black screen unless this=20 bit is set. The PM doesn't make it obvious that the bit is required,=20 but that wouldn't be surprising. Anyway, feel free to send a patch to fix it (with a Fixes: tag).=20 Ideally something like this: u32 osdc =3D 0; ... if (soc_info->has_osd) osdc |=3D JZ_LCD_OSDC_OSDEN; if (soc_info->has_alpha) osdc |=3D JZ_LCD_OSDC_ALPHAEN; regmap_write(priv->map, JZ_REG_LCD_OSDC, osdc); This also ensures that the OSDC register is properly initialized in the=20 !has_osd case. The driver shouldn't rely on pre-boot values in the=20 registers. Cheers, -Paul >=20 >> c) what do your SoC or panels do if you write 0x5 to=20 >> JZ_REG_LCD_OSDC? >> d) 0x00010005 also has bit 16 set which is undocumented... But this=20 >> is a don't care >> according to jz4780 PM. >=20 > BR and thanks, > Nikolaus >=20