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