Re: [PATCH V2 03/12] net-next: mediatek: add embedded switch driver (ESW)

[Florian Fainelli]

On 26/02/16 07:24, John Crispin wrote:
> 
> Hi,
> 
> would the series be ok if we just dropped those parts and then have a
> driver in the kernel that wont do much with the out of tree patches ?
> 
> the problem here is that on one side people complain about vendors not
> sending code upstream. once they start being a good citizen and provide
> funding to send stuff upstream the feedback tends to be very bad as seen
> here.

I agree with David here, the feedback from Andrew is very constructive,
you just don't like the feedback you are being given, which is a
different thing. You can't always get a 12 series patches adding a new
driver accepted after second try, look at all the recent submissions
that occured, it took 5-6-7 maybe more submissions until things were in
a shape where they could be merged. If for your next submission you get
the feedback that switchdev/DSA is deprecated, and something new needs
to be used, then I would agree that feedback is not acceptable, I doubt
this will be the case unless we wait another 10 years to get these
patches out.

> we are planning on doing a DSA driver but one step at a time. this
> kind of feedback will inevitably lead to vendors doing second thoughts
> of upstream contributions.

If you are planning on a DSA driver, which sounds like a good plan, then
maybe drop the integrated switch parts for now, keep it as a local set
of patches for your testing, and just get the basic CPU Ethernet MAC
driver to work for data movement, so that part gets in, and later on,
when your DSA driver is ready, that's one less thing to take care of.
They ultimately are logically spearated drivers if you use DSA, a little
less if you use switchdev.
-- 
Florian

Re: [PATCH V2 03/12] net-next: mediatek: add embedded switch driver (ESW)

[David Miller]

From: Andrew Lunn 
Date: Fri, 26 Feb 2016 18:05:45 +0100

> I think it is great a vendor is providing funding to get code
> upstream. However, that code needs to conform with current kernel
> architecture and design philosophy.
> 
> We as a community also need to be consistent. We have recently push
> back on Microchip with there LAN9352 who want to do something very
> similar, introduce the MAC and a very dumb switch driver. They are now
> looking at what it means to do a DSA driver. There is also talk of
> writing a DSA driver for the ks8995 family.
> 
> As David said recently, a year ago this probably would of been
> accepted. But now, switches need to be DSA or switchdev.

+1

Re: [PATCH V2 03/12] net-next: mediatek: add embedded switch driver (ESW)

[David Miller]

From: Felix Fietkau 
Date: Fri, 26 Feb 2016 17:25:38 +0100

> In my opinion, leaving this part out does not make much sense and
> neither does deferring the entire patch series until we have a
> switchdev/DSA capable driver. This is just a starting point, which will
> be turned into a proper driver with the right APIs later.

I disagree, and we want people to concentrate on writing proper
switchdev/DSA drivers.

People like Andrew have offered to help in any way possible to make
this as easy as possible, so please take this seriously.

I would have accepted your arguments a year ago when we didn't have
the right infrastructure, but now we do and there is no real excuse
to submit partial or bastardized drivers for these kinds of hardware
anymore

Thanks in advance for your understanding, and I'm plan to stand
very firm on this.

Re: [PATCH V2 03/12] net-next: mediatek: add embedded switch driver (ESW)

[David Miller]

From: John Crispin 
Date: Fri, 26 Feb 2016 16:24:47 +0100

> the problem here is that on one side people complain about vendors not
> sending code upstream. once they start being a good citizen and provide
> funding to send stuff upstream the feedback tends to be very bad as seen
> here.

The feedback is not bad, on the contrary it is very positive and people
like Andrew want to help people like you write proper switch drivers.

If you were ignored, or rejected purely on the grounds of coding style
issues, that would be "very bad" feedback.

Re: [PATCH V2 03/12] net-next: mediatek: add embedded switch driver (ESW)

[David Miller]

From: Andrew Lunn 
Date: Fri, 26 Feb 2016 16:18:13 +0100

> On Fri, Feb 26, 2016 at 03:21:35PM +0100, John Crispin wrote:
>> The ESW is found in many of the old 100mbit MIPS based SoCs. it has 5
>> external ports, 1 cpu port and 1 further port that the internal HW
>> offloading engine connects to.
>> 
>> This driver is very basic and only provides basic init and irq support.
>> The SoC and switch core both have support for a special tag making DSA
>> support possible.
> 
> Hi Crispin
> 
> There was recently a discussion about adding switches without using
> DSA or switchdev. It was pretty much decided we would not accept such
> drivers.
> 
> Sorry

+1

Re: [PATCH V2 03/12] net-next: mediatek: add embedded switch driver (ESW)

[Andrew Lunn]

On Fri, Feb 26, 2016 at 05:25:38PM +0100, Felix Fietkau wrote:
> On 2016-02-26 16:18, Andrew Lunn wrote:
> > On Fri, Feb 26, 2016 at 03:21:35PM +0100, John Crispin wrote:
> >> The ESW is found in many of the old 100mbit MIPS based SoCs. it has 5
> >> external ports, 1 cpu port and 1 further port that the internal HW
> >> offloading engine connects to.
> >> 
> >> This driver is very basic and only provides basic init and irq support.
> >> The SoC and switch core both have support for a special tag making DSA
> >> support possible.
> > 
> > Hi Crispin
> > 
> > There was recently a discussion about adding switches without using
> > DSA or switchdev. It was pretty much decided we would not accept such
> > drivers.
> For exactly this reason, the code does not provide any non-standard API
> for allowing the user to configure the switch. The hardware needs to be
> programmed with some defaults for the driver to be functional (since the
> switch logic is built into the SoC).
>
> 
> In my opinion, leaving this part out does not make much sense and
> neither does deferring the entire patch series until we have a
> switchdev/DSA capable driver. This is just a starting point, which will
> be turned into a proper driver with the right APIs later.

You are however introducing APIs which become things you need to
support. Your device tree binding for example. The device tree
maintainers consider this a stable API you need to maintain forever.
Can you guarantee that you can maintain this binding, and also support
the DSA binding? How messy is it going to make your code supporting
two bindings?

You currently have one netdev interface for five switch ports. When
you have a DSA driver, you have 5 additional netdev interfaces, one
per port, and your original interface is now unusable. Isn't that an
API change.

You need to find incremental steps towards a switchdev/DSA driver
which are not going to hinder you in the long run.

Andrew

Re: [PATCH V2 03/12] net-next: mediatek: add embedded switch driver (ESW)

[Andrew Lunn]

> the problem here is that on one side people complain about vendors not
> sending code upstream. once they start being a good citizen and provide
> funding to send stuff upstream the feedback tends to be very bad as seen
> here. we are planning on doing a DSA driver but one step at a time. this
> kind of feedback will inevitably lead to vendors doing second thoughts
> of upstream contributions.

I think it is great a vendor is providing funding to get code
upstream. However, that code needs to conform with current kernel
architecture and design philosophy.

We as a community also need to be consistent. We have recently push
back on Microchip with there LAN9352 who want to do something very
similar, introduce the MAC and a very dumb switch driver. They are now
looking at what it means to do a DSA driver. There is also talk of
writing a DSA driver for the ks8995 family.

As David said recently, a year ago this probably would of been
accepted. But now, switches need to be DSA or switchdev.

  Andrew

Re: [PATCH V2 03/12] net-next: mediatek: add embedded switch driver (ESW)

[Felix Fietkau]

On 2016-02-26 16:18, Andrew Lunn wrote:
> On Fri, Feb 26, 2016 at 03:21:35PM +0100, John Crispin wrote:
>> The ESW is found in many of the old 100mbit MIPS based SoCs. it has 5
>> external ports, 1 cpu port and 1 further port that the internal HW
>> offloading engine connects to.
>> 
>> This driver is very basic and only provides basic init and irq support.
>> The SoC and switch core both have support for a special tag making DSA
>> support possible.
> 
> Hi Crispin
> 
> There was recently a discussion about adding switches without using
> DSA or switchdev. It was pretty much decided we would not accept such
> drivers.
For exactly this reason, the code does not provide any non-standard API
for allowing the user to configure the switch. The hardware needs to be
programmed with some defaults for the driver to be functional (since the
switch logic is built into the SoC).

In my opinion, leaving this part out does not make much sense and
neither does deferring the entire patch series until we have a
switchdev/DSA capable driver. This is just a starting point, which will
be turned into a proper driver with the right APIs later.

- Felix

Re: [PATCH V2 03/12] net-next: mediatek: add embedded switch driver (ESW)

[John Crispin]

On 26/02/2016 16:18, Andrew Lunn wrote:
> On Fri, Feb 26, 2016 at 03:21:35PM +0100, John Crispin wrote:
>> The ESW is found in many of the old 100mbit MIPS based SoCs. it has 5
>> external ports, 1 cpu port and 1 further port that the internal HW
>> offloading engine connects to.
>>
>> This driver is very basic and only provides basic init and irq support.
>> The SoC and switch core both have support for a special tag making DSA
>> support possible.
> 
> Hi Crispin
> 
> There was recently a discussion about adding switches without using
> DSA or switchdev. It was pretty much decided we would not accept such
> drivers.
> 
> Sorry
> 
>   Andrew
> 
> ___
> Linux-mediatek mailing list
> linux-media...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
> 

Hi,

would the series be ok if we just dropped those parts and then have a
driver in the kernel that wont do much with the out of tree patches ?

the problem here is that on one side people complain about vendors not
sending code upstream. once they start being a good citizen and provide
funding to send stuff upstream the feedback tends to be very bad as seen
here. we are planning on doing a DSA driver but one step at a time. this
kind of feedback will inevitably lead to vendors doing second thoughts
of upstream contributions.

John

Re: [PATCH V2 03/12] net-next: mediatek: add embedded switch driver (ESW)

[Andrew Lunn]

On Fri, Feb 26, 2016 at 03:21:35PM +0100, John Crispin wrote:
> The ESW is found in many of the old 100mbit MIPS based SoCs. it has 5
> external ports, 1 cpu port and 1 further port that the internal HW
> offloading engine connects to.
> 
> This driver is very basic and only provides basic init and irq support.
> The SoC and switch core both have support for a special tag making DSA
> support possible.

Hi Crispin

There was recently a discussion about adding switches without using
DSA or switchdev. It was pretty much decided we would not accept such
drivers.

Sorry

Andrew

[PATCH V2 03/12] net-next: mediatek: add embedded switch driver (ESW)

[John Crispin]

The ESW is found in many of the old 100mbit MIPS based SoCs. it has 5
external ports, 1 cpu port and 1 further port that the internal HW
offloading engine connects to.

This driver is very basic and only provides basic init and irq support.
The SoC and switch core both have support for a special tag making DSA
support possible.

I have managed to work out all the names of the registers of the ESW,
however the init for the phys uses lots of voodoo magic which is based
on the SDK driver code. It will be hard to find out the real names for
the phy registers and their bits.

Signed-off-by: John Crispin 
---
 drivers/net/ethernet/mediatek/esw_rt3050.c |  642 
 drivers/net/ethernet/mediatek/esw_rt3050.h |   21 +
 2 files changed, 663 insertions(+)
 create mode 100644 drivers/net/ethernet/mediatek/esw_rt3050.c
 create mode 100644 drivers/net/ethernet/mediatek/esw_rt3050.h

diff --git a/drivers/net/ethernet/mediatek/esw_rt3050.c 
b/drivers/net/ethernet/mediatek/esw_rt3050.c
new file mode 100644
index 000..8da4bec
--- /dev/null
+++ b/drivers/net/ethernet/mediatek/esw_rt3050.c
@@ -0,0 +1,642 @@
+/*   This program is free software; you can redistribute it and/or modify
+ *   it under the terms of the GNU General Public License as published by
+ *   the Free Software Foundation; version 2 of the License
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *   GNU General Public License for more details.
+ *
+ *   Copyright (C) 2009-2016 John Crispin 
+ *   Copyright (C) 2009-2016 Felix Fietkau 
+ *   Copyright (C) 2013-2016 Michael Lee 
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "mtk_eth_soc.h"
+
+/* HW limitations for this switch:
+ * - No large frame support (PKT_MAX_LEN at most 1536)
+ * - Can't have untagged vlan and tagged vlan on one port at the same time,
+ *   though this might be possible using the undocumented PPE.
+ */
+
+#define RT305X_ESW_REG_ISR 0x00
+#define RT305X_ESW_REG_IMR 0x04
+#define RT305X_ESW_REG_FCT00x08
+#define RT305X_ESW_REG_PFC10x14
+#define RT305X_ESW_REG_ATS 0x24
+#define RT305X_ESW_REG_ATS00x28
+#define RT305X_ESW_REG_ATS10x2c
+#define RT305X_ESW_REG_ATS20x30
+#define RT305X_ESW_REG_PVIDC(_n)   (0x40 + 4 * (_n))
+#define RT305X_ESW_REG_VLANI(_n)   (0x50 + 4 * (_n))
+#define RT305X_ESW_REG_VMSC(_n)(0x70 + 4 * (_n))
+#define RT305X_ESW_REG_POA 0x80
+#define RT305X_ESW_REG_FPA 0x84
+#define RT305X_ESW_REG_SOCPC   0x8c
+#define RT305X_ESW_REG_POC00x90
+#define RT305X_ESW_REG_POC10x94
+#define RT305X_ESW_REG_POC20x98
+#define RT305X_ESW_REG_SGC 0x9c
+#define RT305X_ESW_REG_STRT0xa0
+#define RT305X_ESW_REG_PCR00xc0
+#define RT305X_ESW_REG_PCR10xc4
+#define RT305X_ESW_REG_FPA20xc8
+#define RT305X_ESW_REG_FCT20xcc
+#define RT305X_ESW_REG_SGC20xe4
+#define RT305X_ESW_REG_P0LED   0xa4
+#define RT305X_ESW_REG_P1LED   0xa8
+#define RT305X_ESW_REG_P2LED   0xac
+#define RT305X_ESW_REG_P3LED   0xb0
+#define RT305X_ESW_REG_P4LED   0xb4
+#define RT305X_ESW_REG_PXPC(_x)(0xe8 + (4 * _x))
+#define RT305X_ESW_REG_P1PC0xec
+#define RT305X_ESW_REG_P2PC0xf0
+#define RT305X_ESW_REG_P3PC0xf4
+#define RT305X_ESW_REG_P4PC0xf8
+#define RT305X_ESW_REG_P5PC0xfc
+
+#define RT305X_ESW_LED_LINK0
+#define RT305X_ESW_LED_100M1
+#define RT305X_ESW_LED_DUPLEX  2
+#define RT305X_ESW_LED_ACTIVITY3
+#define RT305X_ESW_LED_COLLISION   4
+#define RT305X_ESW_LED_LINKACT 5
+#define RT305X_ESW_LED_DUPLCOLL6
+#define RT305X_ESW_LED_10MACT  7
+#define RT305X_ESW_LED_100MACT 8
+/* Additional led states not in datasheet: */
+#define RT305X_ESW_LED_BLINK   10
+#define RT305X_ESW_LED_ON  12
+
+#define RT305X_ESW_LINK_S  25
+#define RT305X_ESW_DUPLEX_S9
+#define RT305X_ESW_SPD_S   0
+
+#define RT305X_ESW_PCR0_WT_NWAY_DATA_S 16
+#define RT305X_ESW_PCR0_WT_PHY_CMD BIT(13)
+#define RT305X_ESW_PCR0_CPU_PHY_REG_S  8
+
+#define RT305X_ESW_PCR1_WT_DONEBIT(0)
+
+#define RT305X_ESW_ATS_TIMEOUT (5 * HZ)
+#define RT305X_ESW_PHY_TIMEOUT (5 * HZ)
+
+#define RT305X_ESW_PVIDC_PVID_M0xfff
+#define RT305X_ESW_PVIDC_PVID_S12
+
+#define RT305X_ESW_VLANI_VID_M 0xfff
+#define RT305X_ESW_VLANI_VID_S 12
+
+#define RT305X_ESW_VMSC_MSC_M