Re: [PATCH net-next v1 1/2] dt-bindings: net: dsa: Add KSZ8567 switch support

[Conor Dooley]

On Thu, Jan 25, 2024 at 11:57:19AM +0200, Vladimir Oltean wrote:
> Hi Conor,
> 
> On Tue, Jan 23, 2024 at 06:37:55PM +0000, Conor Dooley wrote:
> > On Tue, Jan 23, 2024 at 06:30:16PM +0100, Philippe Schenker wrote:
> > > > > Hi Conor, Thanks for your message!
> > > > > 
> > > > > I need the compatible to make sure the correct ID of the switch is
> > > > > being set in the driver as well as its features.
> > > > 
> > > > Are the features of this switch such that a driver for another ksz
> > > > switch would not work (even in a limited capacity) with the 8567?
> > > > Things like the register map changing or some feature being removed
> > > > are
> > > > examples of why it may not work.
> > > 
> > > Yes the ksz dsa driver is made so that it checks the ID of the attached
> > > chip and refuses to work if it doesn't match. [1]
> > 
> > That sounds counter productive to be honest. Why does the driver not
> > trust that the dt is correct? I saw this recently in some IIO drivers,
> > but it was shot down for this sort of reason.
> 
> If the hardware provides device ID registers, what is the best practice
> in reconciling them with the compatible string?
> 
> I see 2 extreme cases. Ethernet PHY devices seem to blindly trust the
> PHY ID from the "ethernet-phy-idXXXX.XXXX" compatible string, and phylib
> won't read the PHY ID from the standard MDIO registers when this is present.
> Whereas PCI seems to completely disregard the vendor ID and device ID
> from the "pciXXXX,XXXX" compatible string of function OF nodes. Both
> these subsystems have the "compatible" string optional.

The software is free to do either, even if the compatible is a mandatory
property. Which is the right thing to do varies though. In your first
example, the compatible might be used because the standard MDIO
registers do not provide that function for whatever reason and therefore
cannot be read. Or there might be some shenzen market ripoff of another
device that it claims to be in its ID registers but has some
incompatible difference in the programming model.

In general though, if a device can self identify, I would be inclined to
value the self identification over that provided by the devicetree.

My comments here were because of the default in the switch statement
that rejects probe if the ID is unknown as it excludes the use of
fallback compatibles.

In this particular case, and maybe some historical reasons prevent this
that I am unaware of, I would read the ID of the device and if that is
an ID that the driver is aware of, treat that as the truth. As far as I
can tell from my quick look, the driver does this.

The first potential mismatch then is where the ID is, but does not match
what the DT claims the device is. As I said, I'd be inclined to trust
the self identification here, but I can at least understand rejection of
probe. Again from a quick look, the driver seems to trust the self
identification.

If the driver don't recognise the ID then I would treat the device as if
it is what the match data says it is, so that fallback compatibles would
work. This is the case that this driver does not allow.

> I could see an advantage in using a precise device ID in the compatible
> string, rather than just one from which the _location_ of the device ID
> can be deduced, because the precise compatible string allows for much
> finer grained static analysis in the schema for the device. In case of a
> switch, that is the number of ports, supported interface modes for each
> port, etc. With an imprecise device ID, you don't know what you really
> get until you boot the board.

That is true, I think it is pretty desirable (although admittedly I am
rather biased) to add specific compatibles for validation reasons even
if the drivers make no use of them. Generally new bindings for such
cases require a fair bit of justification, but its harder to stop the
proliferation of existing cases, without retrofitting specific
compatibles to the dts users (in addition to the generic one), and
disallowing the generic compatible in isolation.

> > And despite the email, I have nothing to do with these switches, I am
> > just a sucker that signed up to review dt-bindings...
> 
> Same here, just a sucker with some switches and not many clues about
> other things :)

Attachment: signature.asc
Description: PGP signature