Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

From: Dragan Simic <dsimic@manjaro.org>
To: Alban Browaeys <alban.browaeys@gmail.com>
Cc: Conor Dooley <conor@kernel.org>,
	dev@folker-schwesinger.de, Vinod Koul <vkoul@kernel.org>,
	Kishon Vijay Abraham I <kishon@kernel.org>,
	Heiko Stuebner <heiko@sntech.de>,
	Chris Ruehl <chris.ruehl@gtsys.com.hk>,
	Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Christopher Obbard <chris.obbard@collabora.com>,
	Doug Anderson <dianders@chromium.org>,
	Brian Norris <briannorris@chromium.org>,
	Jensen Huang <jensenhuang@friendlyarm.com>,
	linux-phy@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org,
	linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org,
	devicetree@vger.kernel.org
Subject: Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line
Date: Sun, 31 Mar 2024 21:26:13 +0200	[thread overview]
Message-ID: <46d0b87e704ed5a7a0fcc9dcfdbeec2e@manjaro.org> (raw)
In-Reply-To: <871f0b24a38208d9c5d6abc87d83b067162c103e.camel@gmail.com>

Hello Alban,

On 2024-03-28 18:00, Alban Browaeys wrote:
> Le mardi 26 mars 2024 à 19:46 +0000, Conor Dooley a écrit :
>> On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via B4
>> Relay wrote:
>> > From: Folker Schwesinger <dev@folker-schwesinger.de>
>> > -	if (of_property_read_bool(dev->of_node, "rockchip,enable-
>> > strobe-pulldown"))
>> > -		rk_phy->enable_strobe_pulldown =
>> > PHYCTRL_REN_STRB_ENABLE;
>> > +	if (of_property_read_bool(dev->of_node, "rockchip,disable-
>> > strobe-pulldown"))
>> > +		rk_phy->enable_strobe_pulldown =
>> > PHYCTRL_REN_STRB_DISABLE;
>> 
>> Unfortunately you cannot do this.
>> Previously no property at all meant disabled and a property was
>> required
>> to enable it. With this change the absence of a property means that
>> it
>> will be enabled.
>> An old devicetree is that wanted this to be disabled would have no
>> property and will now end up with it enabled. This is an ABI break
>> and is
>> clearly not backwards compatible, that's a NAK unless it is
>> demonstrable
>> that noone actually wants to disable it at all.
> 
> But the patch that introduced the new default to disable the pulldown
> explicitely introduced a regression for at least 4 boards.
> It took time to sort out that the default to disable pulldown was the
> culprit but still.
> Will we carry this new behavor that breaks the default design for
> rk3399 because since the regression was introduced new board definition
> might have expceted this new behavior.
> 
> Could the best option be to revert to énot set a default enable/disable
> pulldown" (as before the commit that introduced the regression) and
> allow one to force the pulldown via the enable/disable pulldown
> property?
> I mean the commit that introduced a default value for the pulldown did
> not seem to be about fixing anything. But it broke a lot. ANd it was
> really really hard to find the description of this commit to understand
> that one had to enable pulldown to restore hs400.

Quite frankly, I think it's better to leave the default as-is, and
to fix the dts files for the boards that have been (or will be) tested
to work as expected and reliably in the HS400 mode.  Perhaps this is
also a good opportunity to revisit the reliability of the HS400 mode
on various boards.

In other words, it could be that some boards now rely on the pull-down
being disabled by default, so enabling it by default might actually
break such boards.  I know, the troublesome commit that disabled the
pull-down caused breakage, but fixing that might actually cause more
breakage at this point.

> In more than 3 years, only one board maintainer noticed that this
> property was required to get back HS400  and thanks to a user telling
> me that this board was working I found from this board that this
> property was "missing" from most board definitions (while it was not
> required before).

A couple of years ago I've also spent some time debugging HS400 not
working on a Rock 4, but ended up with limiting the speed to HS200 as
a workaround, so I agree about the whole thing being a mess.

> I am all for not breaking ABI. But what about not reverting a patch
> that already broke ABI because this patch introduced a new ABI that we
> don't want to break?
> I mean shouldn't a new commit with a new ABI that regressed the kernel
> be reverted?
> 
> Mind fixing the initial regression 8b5c2b45b8f0 "phy: rockchip: set
> pulldown for strobe line in dts" does not necessarily mean changing the
> default to the opposite value but could also be reverting to not
> setting a default.
> Though I don't know if there are pros to setting a default.
> 
> 
>> If this patch fixes a problem on a board that you have, I would
>> suggest
>> that you add the property to enable it, as the binding tells you to.
>> 
>> Thanks,
>> Conor.
> 
> 
> Regards,
> Alban
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip