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

[Mickey Rachamim <mickeyr@marvell.com>]

External Email

Hi Andrew, 
My name is Mickey and I'm the SW group manager in Marvell working on the Switchdev drivers for the Prestera Switching family.
In addition to Vadym's reply, please have my comments inline;

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.
> 

All Marvell Prestera (DX) devices are all based on CPSS SDK. This is one SDK 
and one build procedure that enables the Prestera driver to support all devices. 
This unified support enables us (and our customers) to have one SW 
implementation that will support variety of Prestera devices in same build/real-time 
execution. 
This approach also lead us with the implementation of the Prestera Switchdev drivers.
As having detailed familiarity (20Y) with Marvell Prestera old/current/future devices - 
this approach will be kept strictly also on the future.

> > 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