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 C5ADDC5475B for ; Fri, 1 Mar 2024 09:13:41 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 03D7710EC2F; Fri, 1 Mar 2024 09:13:41 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=bootlin.com header.i=@bootlin.com header.b="n6vUYUWC"; dkim-atps=neutral Received: from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net [217.70.183.198]) by gabe.freedesktop.org (Postfix) with ESMTPS id 354BD10EC2D for ; Fri, 1 Mar 2024 09:13:38 +0000 (UTC) Received: by mail.gandi.net (Postfix) with ESMTPSA id 4B667C000D; Fri, 1 Mar 2024 09:13:35 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=gm1; t=1709284416; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=oKUC9cfoNF8xTSgffCz2iTSEH+f4q6CdFZYQZOYiENU=; b=n6vUYUWCwRejbttVHqHkGoksDAI0/HC1as0gYFx2cZgCLFwWjI7orlgQhnTl9BVVp+HYGE GKemOe1wb1Ocqrp1eZA6BPuxQmiFmpx4CEWaXjz5Oai56OA54ijVGzr7SJ5zQlG8dkbJUL NhJZ6QU1CnlMSI78BmNd76cK75qlJidMad7HVYHQlZGohkGz+BMfTWipwaU4rYsD+UuI0L N+uOVfA9DAsBttmxH1JGCf29zKdRE1G3We8z9p+ZapTRvuwkiaydN4KheoSqlmChdNouYj d+tPnTDKwtARF4JbsJLrM618IHlUeQzPW40UIiXpcaesqZ52g/xpA3fORFrvWw== Date: Fri, 1 Mar 2024 10:13:33 +0100 From: Luca Ceresoli To: Frieder Schrempf Cc: Alexander Stein , Andrzej Hajda , Neil Armstrong , Robert Foss , Laurent Pinchart , Jonas Karlman , Jernej Skrabec , David Airlie , Daniel Vetter , dri-devel@lists.freedesktop.org Subject: Re: [PATCH 1/1] drm/bridge: ti-sn65dsi83: Fix enable error path Message-ID: <20240301101333.54f1fbd3@booty> In-Reply-To: <9480d2fa-136b-47bf-ad76-c8d6d7c9c070@kontron.de> References: <20230504065316.2640739-1-alexander.stein@ew.tq-group.com> <1885005.tdWV9SEqCh@steina-w> <20240227184144.19729521@booty> <3798602.kQq0lBPeGt@steina-w> <20240229104723.7aa71075@booty> <9480d2fa-136b-47bf-ad76-c8d6d7c9c070@kontron.de> Organization: Bootlin X-Mailer: Claws Mail 4.0.0 (GTK+ 3.24.33; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-GND-Sasl: luca.ceresoli@bootlin.com 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: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On Thu, 29 Feb 2024 11:48:27 +0100 Frieder Schrempf wrote: > 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 130000 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. It looks like [1] (or the other patches mentioned by Michael in the same thread?) should be applied indeed. I'm going to test those patches ASAP, perhaps next week. However I'm not sure this is related to the problem being discussed here: the patches about pre_enable_prev_first are touching atomic_pre_enable and atomic_pre_disable. Here Alexander reported when removing the bridge atomic_disable is not called, and atomic_disable is not affected by pre_enable_prev_first. > [1] https://patchwork.freedesktop.org/patch/529288/ > [2] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=4fb912e5e19075874379cfcf074d90bd51ebf8ea Luca -- Luca Ceresoli, Bootlin Embedded Linux and Kernel engineering https://bootlin.com