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

[Vadym Kochan]

Hi Andrew,

On Fri, Aug 14, 2020 at 03:18:15PM +0200, Andrew Lunn wrote:
> > > > 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?
> 

ASIC device specific handling is serviced by the firmware, current
driver's logic does not have PP specific code and relies on the FW ABI
which is PP-generic, and it looks like this how it should work for
boards with other ASICs, of course these boards should follow the
Marvell's Switchdev board design.

> 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