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

Alex Nemirovsky Alex.Nemirovsky at cortina-access.com
Wed Jun 3 23:10:46 CEST 2020


Hi Tom

> On Jun 3, 2020, at 1:59 PM, Tom Rini <trini at konsulko.com> wrote:
> 
> 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?  

The SW team didn’t create these structures by hand. They are autogenerated 
my our HW design tools and adopted by the SW team.  
To be clear, I am not trying to defend the use of this auto generated structures, just explaining its origin.
Obviously it will take significant effort for us to recreate these structures in the desired form.

Therefore, it must be asked.  Is it required to be recreated in the structure form that you request and C code modified 
accordingly  to be accepted into the u-boot upstream tree going forward?


> 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



More information about the U-Boot mailing list