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

[Dragan Simic]

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 at 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 at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip