Re: [PATCH net-next v5 00/13] dsa: Broadcom Starfighter 2 switch support

Re: [PATCH net-next v5 00/13] dsa: Broadcom Starfighter 2 switch support

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Wed, 27 Aug 2014 18:12:29 -0700 (PDT)

> From: Florian Fainelli <f.fainelli@gmail.com>
> Date: Wed, 27 Aug 2014 17:04:14 -0700
> 
>> This patch series adds support for the Broadcom Starfighter 2 (Roboswitch
>> successor) using the existing DSA infrastructure. This integrated switch
>> is heavily used in Set Top Box, Cable gateways and DSL gateways products
>> from Broadcom, and to a larger extent the new ARM-based Wi-Fi routers although
>> slightly differently.
> 
> Series applied, thanks Florian.

Florian, I had to make an adjustment to make your series work.

The Tulip dmfe driver internally has static functions named phy_read()
and phy_write().  I had to add a commit to rename them before your
series.

Please when adjusting globally visible interfaces, do an allmodconfig
build to catch these kinds of problems.

Thanks.

Re: [PATCH net-next v5 00/13] dsa: Broadcom Starfighter 2 switch support

[Florian Fainelli]

2014-08-27 18:30 GMT-07:00 David Miller <davem@davemloft.net>:
> From: David Miller <davem@davemloft.net>
> Date: Wed, 27 Aug 2014 18:12:29 -0700 (PDT)
>
>> From: Florian Fainelli <f.fainelli@gmail.com>
>> Date: Wed, 27 Aug 2014 17:04:14 -0700
>>
>>> This patch series adds support for the Broadcom Starfighter 2 (Roboswitch
>>> successor) using the existing DSA infrastructure. This integrated switch
>>> is heavily used in Set Top Box, Cable gateways and DSL gateways products
>>> from Broadcom, and to a larger extent the new ARM-based Wi-Fi routers although
>>> slightly differently.
>>
>> Series applied, thanks Florian.
>
> Florian, I had to make an adjustment to make your series work.
>
> The Tulip dmfe driver internally has static functions named phy_read()
> and phy_write().  I had to add a commit to rename them before your
> series.

Ah, I should have caught that.

>
> Please when adjusting globally visible interfaces, do an allmodconfig
> build to catch these kinds of problems.

Will do next time, thanks!

>
> Thanks.



-- 
Florian

Re: [PATCH net-next v5 11/13] net: dsa: add Broadcom SF2 switch driver

[Alexander Duyck]

On 09/09/2014 12:45 PM, Florian Fainelli wrote:
> On 09/09/2014 12:32 PM, Alexander Duyck wrote:
>> On Wed, Aug 27, 2014 at 5:04 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>> Add support for the Broadcom Starfigther 2 switch chip using a DSA
>>> driver. This switch driver supports the following features:
>>>
>>> - configuration of the external switch port interface: MII, RevMII,
>>>   RGMII and RGMII_NO_ID are supported
>>> - support for the per-port MIB counters
>>> - support for link interrupts for special ports (e.g: MoCA)
>>> - powering up/down of switch memories to conserve power when ports are
>>>   unused
>>>
>>> Finally, update the compatible property for the DSA core code to match
>>> our switch top-level compatible node.
>>>
>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>> ---
>>> Changes in v4:
>>> - fixed typo on the word Starfighter
>>> - fixed a few checkpatch.pl warnings
>>>
>>> No changes in v3
>>>
>>> Changes in v2:
>>> - add support for reading to special MDIO phys (0 and 30)
>>> - added more power down optimization
>>> - added VLAN separation
>>>
>>>  drivers/net/dsa/Kconfig        |  11 +
>>>  drivers/net/dsa/Makefile       |   1 +
>>>  drivers/net/dsa/bcm_sf2.c      | 626 +++++++++++++++++++++++++++++++++++++++++
>>>  drivers/net/dsa/bcm_sf2.h      | 140 +++++++++
>>>  drivers/net/dsa/bcm_sf2_regs.h | 227 +++++++++++++++
>>>  net/dsa/dsa.c                  |   1 +
>>>  6 files changed, 1006 insertions(+)
>>>  create mode 100644 drivers/net/dsa/bcm_sf2.c
>>>  create mode 100644 drivers/net/dsa/bcm_sf2.h
>>>  create mode 100644 drivers/net/dsa/bcm_sf2_regs.h
>> [...]
>>
>>> diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
>>> new file mode 100644
>>> index 000000000000..bb7cb8e283b1
>>> --- /dev/null
>>> +++ b/drivers/net/dsa/bcm_sf2.c
>> [...]
>>
>>> +static char *bcm_sf2_sw_probe(struct mii_bus *bus, int sw_addr)
>>> +{
>>> +       return "Broadcom Starfighter 2";
>>> +}
>>> +
>> I hadn't noticed before but with this driver it seems like you could
>> potentially load on any DSA enabled device could you not?  It seems
>> like this would be problematic since you could end up registering
>> before another DSA driver and prevent it from being able to load since
>> you always return success.  Isn't there any test you could run to
>> determine if the switch is actually there or not?
> Unfortunately the current DSA device/driver model is kind of messed up
> for that, which is something I plan on fixing, although it would take a
> little bit more time. The way it works currently is:
>
> - you register a DSA platform device, feed it with Device Tree or
> C-struct configuration data
> - you register a switch driver
> - the DSA platform code will eventually iterate over all switch devices,
> call into their probe function and based on a non-NULL return, accept to
> register this switch device
> - the probe function only accepts MDIO connected switches, anything else
> has to find another way to tell that it is there
>
> so all of this works okay until you have a switch which is memory-mapped
> into the CPU address space and which is not on the MDIO bus.
>
> A short term solution could be to change the probe argument to be more
> generic and pass a void *bus pointer or something allowing us to do a
> tad more things, including verifying a register to see if the switch is
> there.

I would probably just rewrite the call to accept dsa_chip_data instead
of passing it the mii_bus and sw_addr.  Then you can just access data
like the of_node directly.  I'm also thinking it might make more sense
to make the mii_bus pointer in the dsa_chip_data a bit more type
agnostic by simply treating it as a parent device.  It seems like most
of the code is already there in dsa via the dev_find_class check that is
checking for "mdio_bus".

> The way I would like to fix this model though is to allow switch drivers to:
>
> - specify their own configuration data, since for instance, external
> switches usually have a pretty fixed set of configuration options:
> number of ports, fixed CPU port, while keeping platform-driven
> configuration data as well
>
> - be backed by their host interface device/driver, e.g: allow a SPI,
> PHY, PCI(e), USB drivers to register a switch driver, such that there
> really is a struct device pointer we can refer to for various operations
> (DMA, PM...)

This is the kind of situation I am looking at.  In my case I have a PCIe
interface with one of the BARs providing access to switch registers.  As
such I would want to be able to provide a PCI device and sort out the
eligibility to run the driver by checking for the PCI vendor and device ID.

> I will cook some patches that do that in the next few days.
> --
> Florian

I'll keep an eye open for them.  I might start submitting a few patches
myself as I should be pushing my driver in the next week or two.

Thanks,

Alex

Re: [PATCH net-next v5 11/13] net: dsa: add Broadcom SF2 switch driver

[Florian Fainelli]

On 09/09/14 19:17, Alexander Duyck wrote:

[snip]

>>>
>>>> +static char *bcm_sf2_sw_probe(struct mii_bus *bus, int sw_addr)
>>>> +{
>>>> +       return "Broadcom Starfighter 2";
>>>> +}
>>>> +
>>> I hadn't noticed before but with this driver it seems like you could
>>> potentially load on any DSA enabled device could you not?  It seems
>>> like this would be problematic since you could end up registering
>>> before another DSA driver and prevent it from being able to load since
>>> you always return success.  Isn't there any test you could run to
>>> determine if the switch is actually there or not?
>> Unfortunately the current DSA device/driver model is kind of messed up
>> for that, which is something I plan on fixing, although it would take a
>> little bit more time. The way it works currently is:
>>
>> - you register a DSA platform device, feed it with Device Tree or
>> C-struct configuration data
>> - you register a switch driver
>> - the DSA platform code will eventually iterate over all switch devices,
>> call into their probe function and based on a non-NULL return, accept to
>> register this switch device
>> - the probe function only accepts MDIO connected switches, anything else
>> has to find another way to tell that it is there
>>
>> so all of this works okay until you have a switch which is memory-mapped
>> into the CPU address space and which is not on the MDIO bus.
>>
>> A short term solution could be to change the probe argument to be more
>> generic and pass a void *bus pointer or something allowing us to do a
>> tad more things, including verifying a register to see if the switch is
>> there.
>
> I would probably just rewrite the call to accept dsa_chip_data instead
> of passing it the mii_bus and sw_addr.  Then you can just access data
> like the of_node directly.  I'm also thinking it might make more sense
> to make the mii_bus pointer in the dsa_chip_data a bit more type
> agnostic by simply treating it as a parent device.  It seems like most
> of the code is already there in dsa via the dev_find_class check that is
> checking for "mdio_bus".

Yes, I like that.

>
>> The way I would like to fix this model though is to allow switch drivers to:
>>
>> - specify their own configuration data, since for instance, external
>> switches usually have a pretty fixed set of configuration options:
>> number of ports, fixed CPU port, while keeping platform-driven
>> configuration data as well
>>
>> - be backed by their host interface device/driver, e.g: allow a SPI,
>> PHY, PCI(e), USB drivers to register a switch driver, such that there
>> really is a struct device pointer we can refer to for various operations
>> (DMA, PM...)
>
> This is the kind of situation I am looking at.  In my case I have a PCIe
> interface with one of the BARs providing access to switch registers.  As
> such I would want to be able to provide a PCI device and sort out the
> eligibility to run the driver by checking for the PCI vendor and device ID.

I see, bcm_sf2 is kind of similar here, thanks to Device Tree we can do 
a lot of things without being backed by an actual struct device, but 
there are other situations where this is not desirable, like yours. In 
my case a platform_device/driver would be more appropriate anyway.

>
>> I will cook some patches that do that in the next few days.
>> --
>> Florian
>
> I'll keep an eye open for them.  I might start submitting a few patches
> myself as I should be pushing my driver in the next week or two.

Great! I will also keep an eye on it too, I got some patches I would 
like to send that add suspend/resume support, Wake-on-LAN and EEE to 
DSA/bcm_sf2; but we should probably get the device/driver model right first.

Those are pretty trivial patches anyway that just add some layering 
around the DSA and the DSA switch drivers.

NB: I have not yet addressed your suggestion of replacing tag_protocol 
with an enum, feel free to send that first.
--
Florian

[PATCH net-next v5 00/13] dsa: Broadcom Starfighter 2 switch support

[Florian Fainelli]

Hi all,

This patch series adds support for the Broadcom Starfighter 2 (Roboswitch
successor) using the existing DSA infrastructure. This integrated switch
is heavily used in Set Top Box, Cable gateways and DSL gateways products
from Broadcom, and to a larger extent the new ARM-based Wi-Fi routers although
slightly differently.

Changes in v5 are the introduction of ETH_P_XDSA as suggested by Alexander to
help capture applications see this is a multiplexed DSA approach now.

Changes in v4 are the introducing of an indirection level for DSA switch tag
protocols receive and transmit functions.

I intentionnaly did not address one comment from Alexander who suggested to
move port_names and port_dn in a separate structure since that involves
touching arch/arm/ and arch/blackfin/ code which I am not yet comfortable
doing.

Notable changes in v3 is the preliminary patch that reworks the skb->protocol
override helpers for non-Ethertype switch tags, based on feedback from
Alexander Duyck.

The biggest changes from v1 of this patch series are:

- use the new fixed PHY helpers
- improved the switch driver with more complete features (interrupts,
  (RG)MII configuration, memory arrays power down/up, port disabling/enable
  VLAN separation

Future work will focus on bringing the upstream driver in feature parity with
the current downstream driver, including:

- adding Wake-on-LAN support to the switch
- adding suspend/resume callbacks for S2/S3 Power Management modes
- extending the switch register interface to cover BCM5310X SoCs

Florian Fainelli (13):
  net: dsa: reduce number of protocol hooks
  net: phy: add generic UniMAC MDIO bus driver
  net: phy: provide stub for fixed_phy_set_link_update
  net: dsa: provide a switch device device tree node pointer
  net: dsa: retain a per-port device_node pointer
  net: dsa: allow for more complex PHY setups
  net: dsa: allow switches to work without tagging
  net: dsa: allow drivers to do link adjustment
  net: dsa: allow updating fixed PHY link information
  net: dsa: add Broadcom tag RX/TX handler
  net: dsa: add Broadcom SF2 switch driver
  Documentation: devicetree: update dsa binding with optional properties
  Documentation: devicetree: add Broadcom Starfighter 2 binding

 .../bindings/net/broadcom-mdio-unimac.txt          |  39 ++
 .../devicetree/bindings/net/broadcom-sf2.txt       |  78 +++
 Documentation/devicetree/bindings/net/dsa/dsa.txt  |  17 +
 drivers/net/dsa/Kconfig                            |  11 +
 drivers/net/dsa/Makefile                           |   1 +
 drivers/net/dsa/bcm_sf2.c                          | 626 +++++++++++++++++++++
 drivers/net/dsa/bcm_sf2.h                          | 140 +++++
 drivers/net/dsa/bcm_sf2_regs.h                     | 227 ++++++++
 drivers/net/phy/Kconfig                            |   8 +
 drivers/net/phy/Makefile                           |   1 +
 drivers/net/phy/mdio-bcm-unimac.c                  | 212 +++++++
 include/linux/netdevice.h                          |  26 +-
 include/linux/phy_fixed.h                          |  17 +-
 include/net/dsa.h                                  |  43 +-
 include/uapi/linux/if_ether.h                      |   1 +
 net/dsa/Kconfig                                    |   3 +
 net/dsa/Makefile                                   |   1 +
 net/dsa/dsa.c                                      |  48 +-
 net/dsa/dsa_priv.h                                 |  16 +-
 net/dsa/slave.c                                    | 165 ++++--
 net/dsa/tag_brcm.c                                 | 173 ++++++
 net/dsa/tag_dsa.c                                  |   8 +-
 net/dsa/tag_edsa.c                                 |   8 +-
 net/dsa/tag_trailer.c                              |   8 +-
 net/ethernet/eth.c                                 |   7 +-
 25 files changed, 1772 insertions(+), 112 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/broadcom-mdio-unimac.txt
 create mode 100644 Documentation/devicetree/bindings/net/broadcom-sf2.txt
 create mode 100644 drivers/net/dsa/bcm_sf2.c
 create mode 100644 drivers/net/dsa/bcm_sf2.h
 create mode 100644 drivers/net/dsa/bcm_sf2_regs.h
 create mode 100644 drivers/net/phy/mdio-bcm-unimac.c
 create mode 100644 net/dsa/tag_brcm.c

-- 
1.9.1