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

[Tom Rini]

On Wed, Jun 03, 2020 at 07:03:09PM +0000, Alex Nemirovsky wrote:
> 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. 

Ah right.

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

pr_debug and other functions, and a handful of other print functions are
in <linux/printk.h> and will both mean you don't need to wrap debug
statements but also leverage the logging functionality correctly so that
we can also discard messages to save space but also still easily bring
in useful information when a developer has a problem they're tracing.

> > and contains a whole
> > lot of debug code.
> 
> remove development debug code which is no longer used.

Yes, and probably then do some clean-up of the functions once that's
removed, if that then makes sense.

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

OK, so lets leave the stuff for other platforms out for now.  If there's
abstractions that are needed for them, you can note that's why there's a
hook when there is a hook.  But that's probably still better handled
when introducing / posting the platform that needs them for further
context.

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

Well, why is it written the way it's written?  It doesn't follow the
style of other network drivers and so yes, it needs to be written in the
same manner as other drivers.  So for example:
+union NI_HV_PT_PORT_STATIC_CFG_t {
+	struct {
+		unsigned int int_cfg              :  4; /* bits 3:0 */
+		unsigned int phy_mode             :  1; /* bits 4:4 */
+		unsigned int rmii_clksrc          :  1; /* bits 5:5 */
+		unsigned int inv_clk_in           :  1; /* bits 6:6 */
+		unsigned int inv_clk_out          :  1; /* bits 7:7 */
+		unsigned int inv_rxclk_out        :  1; /* bits 8:8 */
+		unsigned int tx_use_gefifo        :  1; /* bits 9:9 */
+		unsigned int smii_tx_stat         :  1; /* bits 10:10 */
+		unsigned int crs_polarity         :  1; /* bits 11:11 */
+		unsigned int lpbk_mode            :  2; /* bits 13:12 */
+		unsigned int gmii_like_half_duplex_en :  1; /* bits 14:14 */
+		unsigned int sup_tx_to_rx_lpbk_data :  1; /* bits 15:15 */
+		unsigned int rsrvd1               :  8;
+		unsigned int mac_addr6            :  8; /* bits 31:24 */
+	} bf;
+	unsigned int     wrd;
+};

But in other drivers we just do:
struct pdma_rxd_info2 {
        u32 PLEN1 : 14;
        u32 LS1 : 1;
        u32 UN_USED : 1;
        u32 PLEN0 : 14;
        u32 LS0 : 1;
        u32 DDONE : 1;
};

So yes, all of the places where you have a union we normally do a struct.

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

And any other non-IP-specific functionality.

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

To clean up the driver so it reads like a regular U-Boot network driver.
I see later in the patch that CA_IN / CA_OUT are just empty.

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

Yes.

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

Please look at the existing drivers for other IP blocks.  It looks like
there's PHY stuff in the driver, which should instead be leveraging and
if needed updating something under drivers/net/phy, there's debug
commands that should just be dropped.  Make sure there's no un-used
defines, structs, etc. 

Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200603/be8d8a38/attachment.sig>