[PATCH v4 1/2] net: cortina_ni: Add eth support for Cortina Access CAxxxx SoCs

[Alex Nemirovsky]

Hi Tom,

> On Jun 3, 2020, at 8:03 AM, Tom Rini <trini at konsulko.com> wrote:
> 
> On Wed, Jun 03, 2020 at 01:05:18AM -0700, Alex Nemirovsky wrote:
>> From: Aaron Tseng <aaron.tseng at cortina-access.com>
>> 
>> Add Cortina Access Ethernet device driver for CAxxxx SoCs.
>> This driver supports only the DM_ETH network model.
>> 
>> Signed-off-by: Aaron Tseng <aaron.tseng at cortina-access.com>
>> Signed-off-by: Alex Nemirovsky <alex.nemirovsky at cortina-access.com>
>> 
>> CC: Joe Hershberger <joe.hershberger at ni.com>
>> CC: Abbie Chang <abbie.chang at Cortina-Access.com>
>> CC: Tom Rini <trini at konsulko.com>
>> 
>> ---
>> 
>> Changes in v4: None
>> Changes in v3:
>> - Changed commit comment to state that only DM model is supported
>> - Removed blank line at end of C file
>> 
>> Changes in v2:
>> - Remove legacy mode support
>> - Add support for additional SoC variants
>> - Remove unused variables
>> 
>> MAINTAINERS              |    4 +
>> drivers/net/Kconfig      |    7 +
>> drivers/net/Makefile     |    1 +
>> drivers/net/cortina_ni.c | 1909 ++++++++++++++++++++++++++++++++++++++++++++++
>> drivers/net/cortina_ni.h |  592 ++++++++++++++
>> 5 files changed, 2513 insertions(+)
>> create mode 100644 drivers/net/cortina_ni.c
>> create mode 100644 drivers/net/cortina_ni.h
> 
> So, is there a similar driver in upstream Linux?

We don’t have ANY Linux drivers upstream. 
Let me see if I can parse the comments below into action items for us.
Let me know if I got it right or missed something.

>  This driver doesn't
> quite fit right.  


> At an "easy" level, there's still the customer macro
> around debug statements and not using pr_debug(),

find and convert all debug statement to using U-Boot specific  pr_debug().

> and contains a whole
> lot of debug code.

remove development debug code which is no longer used.

>  There's also code for a saturn platform, is that
> being upstreamed?  
Yes, it will be upstreamed to u-boot.The approach is to first focus on getting
all our u-boot drivers upstream for the presidio SoC engineering board.
Most of the drivers are designed to work with other SoC platforms as those
SoC reuse HW logic for our peripherals.  

> There's a ton of union usage which is also uncommon.
 Is there an action item here?

> A small item is it looks like this has its own crc function, when we
> have those available already.

reuse existing CRC functions from u-boot instead of providing our own.

>  There's also things like:
>> +enum ca_status_t ca_mdio_read(CA_IN       unsigned int          addr,
>> +			      CA_IN       unsigned int		offset,
>> +			      CA_OUT      unsigned short	*data)
>> +{
> 
> Where CA_IN / CA_OUT just don't fit.

What specifically is the action item here?
> 
> Which brings me back to why I asked the first question, this feels a lot
> like a driver for an RTOS or some other system was adapted to U-Boot,
> rather than writing a new driver for U-Boot based on internal knowledge
> of the part in question.  And I think that applies to a lot of the
> drivers as seen in the NAND review as well.  Thanks!
If I understand you correctly, you would like for the drivers to more directly REUSE
native U-BOOT core functions already provided instead of providing
our own which essentially duplicate the same core functions. 

Did I misunderstand anything or do you have something more to add
as action items for us?
> 
> -- 
> Tom