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

[Alex Nemirovsky]

Hi Tom,

Thanks for your feedback.

> On Jun 3, 2020, at 3:03 PM, Tom Rini <trini at konsulko.com> wrote:
> 
> On Wed, Jun 03, 2020 at 09:10:46PM +0000, Alex Nemirovsky wrote:
>> 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?
> 
> Yes, and work with the HW design tool folks to generate output that's
> more acceptable to various open source projects.  You aren't the first
> team to have to push for this, I can tell you from experience.
> 
> But since it's generated output, it can be transformed too via sed, etc.
> Thanks!
> 
> -- 
> Tom