Re: [net-next v4 1/6] net: marvell: prestera: Add driver for Prestera family ASIC devices

[Andrew Lunn]

> > > Currently 
> > > 
> > >     compatible = "marvell,prestera"
> > > 
> > > is used as default, so may be
> > > 
> > > you mean to support few matching including particular silicon too, like ?
> > > 
> > > 
> > >     compatible = "marvell,prestera"
> > >     compatible = "marvell,prestera-ac3x"
> > > 
> > > Would you please give an example ?
> > 
> > AFAICT "Prestera" is the general name for the Marvell
> > enterprise/data-centre silicon, comparable to the "LinkStreet"
> > designation for their lower end switching. The mv88e* drivers do not
> > mention LinkStreet in their compatible strings at all, choosing instead
> > to refer to chip IDs (I see mv88e6085, mv88e6190 + mv88e6250).
> > 
> > I do not have enough familiarity with the Prestera range to be able to
> > tell what commonality there is between the different versions (it
> > appears you need an NDA to get hold of the programming references), but
> > even just looking at your driver and the vendor code for the BobCat it
> > seems that AlleyCat3 uses an extended DSA header format, and requires a
> > firmware with message based access, in comparison to the BobCat which
> > uses register poking.
> > 
> > Based on that I'd recommend not using the bare "marvell,prestera"
> > compatible string, but instead something more specific.
> > "marvell,prestera-ac3x" seems like a suitable choice, assuming that's
> > how these chips are named/generally referred to.
> > 
> > Also I'd expand your Kconfig information to actually include "Marvell
> > Prestera 98DX326x" as that's the only supported chip range at present.
> > 
> 
> Yes, Prestera covers more range of devices. But it is planning to cover
> other devices too, and currently there is no device-specific DTS
> properties which are used in this version, but only the generic one -
> since only the MAC address node.
> 
> I mean that if there will be other Prestera devices supported then it
> will require to extend the DTS matching string in the driver just to
> support the same generic DTS properties for new device.
> 
> Anyway I will rise and discuss this question.

Hi Vadym

Lets start with how mv88e6xxx does this. The switches have ID
registers. Once you have read the ID registers, you know what device
you have, and you can select device specific code as needed. However,
these ID registers are in three different locations, depending on the
chip family. So the compatible string is all about where to read the
ID from, not about what specific chip is. So most device tree bindings
say "marvell,mv88e6085", but the 6390 family use "marvell,mv88e6190"
for example.

This naming scheme is actually odd compared to others. And that
oddness causes confusion. But it avoids a few problems. If you have
per chip compatible strings, what do you do when it conflicts with the
ID registers. If from day 1 you validate the compatible string against
the ID register and fail the probe if it is incorrect, you are
O.K. But if you decide to add this validation later, you are going to
find a number of device tree blobs which have the wrong compatible
string. Do you fail the probe on boards which have worked?

So what to do with this driver?

Does the prestera have ID registers? Are you using them in the driver?

Marvell is not particularly good at backwards compatibility. Does your
compatible string give you enough wiggle room you can easily introduce
another compatible string in order to find the ID registers when they
move?

	Andrew