mbox series

[v3,0/5] add dsa switch support for ar9331

Message ID 20191021053811.19818-1-o.rempel@pengutronix.de
Headers show
Series add dsa switch support for ar9331 | expand

Message

Oleksij Rempel Oct. 21, 2019, 5:38 a.m. UTC 
changes v3:
- ag71xx: ag71xx_mac_config: ignore MLO_AN_INBAND mode. It is not
  supported by HW and SW.
- ag71xx: ag71xx_mac_validate: return all supported bits on
  PHY_INTERFACE_MODE_NA

changes v2:
- move Atheros AR9331 TAG format to separate patch
- use netdev_warn_once in the tag driver to reduce potential message spam
- typo fixes
- reorder tag driver alphabetically 
- configure switch to maximal frame size
- use mdiobus_read/write
- fail if mdio sub node is not found
- add comment for post reset state
- remove deprecated comment about device id
- remove phy-handle option for node with fixed-link
- ag71xx: set 1G support only for GMII mode

This patch series provides dsa switch support for Atheros ar9331 WiSoC.
As side effect ag71xx needed to be ported to phylink to make the switch
driver (as well phylink based) work properly.

Oleksij Rempel (5):
  net: ag71xx: port to phylink
  dt-bindings: net: dsa: qca,ar9331 switch documentation
  MIPS: ath79: ar9331: add ar9331-switch node
  net: dsa: add support for Atheros AR9331 TAG format
  net: dsa: add support for Atheros AR9331 build-in switch

 .../devicetree/bindings/net/dsa/ar9331.txt    | 148 ++++
 arch/mips/boot/dts/qca/ar9331.dtsi            | 127 ++-
 arch/mips/boot/dts/qca/ar9331_dpt_module.dts  |  13 +
 drivers/net/dsa/Kconfig                       |   2 +
 drivers/net/dsa/Makefile                      |   1 +
 drivers/net/dsa/qca/Kconfig                   |  11 +
 drivers/net/dsa/qca/Makefile                  |   2 +
 drivers/net/dsa/qca/ar9331.c                  | 823 ++++++++++++++++++
 drivers/net/ethernet/atheros/Kconfig          |   2 +-
 drivers/net/ethernet/atheros/ag71xx.c         | 146 ++--
 include/net/dsa.h                             |   2 +
 net/dsa/Kconfig                               |   6 +
 net/dsa/Makefile                              |   1 +
 net/dsa/tag_ar9331.c                          |  97 +++
 14 files changed, 1321 insertions(+), 60 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/dsa/ar9331.txt
 create mode 100644 drivers/net/dsa/qca/Kconfig
 create mode 100644 drivers/net/dsa/qca/Makefile
 create mode 100644 drivers/net/dsa/qca/ar9331.c
 create mode 100644 net/dsa/tag_ar9331.c

Comments

Andrew Lunn Oct. 21, 2019, 3:49 p.m. UTC | #1 
> +static struct sk_buff *ar9331_tag_rcv(struct sk_buff *skb,
> +				      struct net_device *ndev,
> +				      struct packet_type *pt)
> +{
> +	u8 ver, port;
> +	u16 hdr;
> +
> +	if (unlikely(!pskb_may_pull(skb, AR9331_HDR_LEN)))
> +		return NULL;
> +
> +	hdr = le16_to_cpu(*(__le16 *)skb_mac_header(skb));
> +
> +	ver = FIELD_GET(AR9331_HDR_VERSION_MASK, hdr);
> +	if (unlikely(ver != AR9331_HDR_VERSION)) {
> +		netdev_warn_once(ndev, "%s:%i wrong header version 0x%2x\n",
> +				 __func__, __LINE__, hdr);
> +		return NULL;
> +	}
> +
> +	if (unlikely(hdr & AR9331_HDR_FROM_CPU)) {
> +		netdev_warn_once(ndev, "%s:%i packet should not be from cpu 0x%2x\n",
> +				 __func__, __LINE__, hdr);
> +		return NULL;
> +	}
> +
> +	skb_pull(skb, AR9331_HDR_LEN);
> +	skb_set_mac_header(skb, -ETH_HLEN);

No other tag driver calls skb_set_mac_header().  Also, the -ETH_HLEN
looks odd, give you have just pulled off AR9331_HDR_LEN?

What other tag drivers use is skb_pull_rcsum().

     Andrew
Oleksij Rempel Oct. 21, 2019, 6:36 p.m. UTC | #2 
On Mon, Oct 21, 2019 at 05:49:00PM +0200, Andrew Lunn wrote:
> > +static struct sk_buff *ar9331_tag_rcv(struct sk_buff *skb,
> > +				      struct net_device *ndev,
> > +				      struct packet_type *pt)
> > +{
> > +	u8 ver, port;
> > +	u16 hdr;
> > +
> > +	if (unlikely(!pskb_may_pull(skb, AR9331_HDR_LEN)))
> > +		return NULL;
> > +
> > +	hdr = le16_to_cpu(*(__le16 *)skb_mac_header(skb));
> > +
> > +	ver = FIELD_GET(AR9331_HDR_VERSION_MASK, hdr);
> > +	if (unlikely(ver != AR9331_HDR_VERSION)) {
> > +		netdev_warn_once(ndev, "%s:%i wrong header version 0x%2x\n",
> > +				 __func__, __LINE__, hdr);
> > +		return NULL;
> > +	}
> > +
> > +	if (unlikely(hdr & AR9331_HDR_FROM_CPU)) {
> > +		netdev_warn_once(ndev, "%s:%i packet should not be from cpu 0x%2x\n",
> > +				 __func__, __LINE__, hdr);
> > +		return NULL;
> > +	}
> > +
> > +	skb_pull(skb, AR9331_HDR_LEN);
> > +	skb_set_mac_header(skb, -ETH_HLEN);
> 
> No other tag driver calls skb_set_mac_header().  Also, the -ETH_HLEN
> looks odd, give you have just pulled off AR9331_HDR_LEN?

Hm.. is it corrected somewhere else? Any way, B.T.M.A.N need a proper
value ant it seems to work correctly. So I remove it.

> What other tag drivers use is skb_pull_rcsum().

It is build in switch and internal Ethernet controller do not set csum. There is
nothing to recalculate... on other hand, it adds no overhead. So, I have
nothing against it. Are there other arguments?

Regards,
Oleksij
Russell King (Oracle) Oct. 21, 2019, 10:21 p.m. UTC | #3 
On Mon, Oct 21, 2019 at 07:38:07AM +0200, Oleksij Rempel wrote:
> +static void ag71xx_mac_validate(struct phylink_config *config,
> +			    unsigned long *supported,
> +			    struct phylink_link_state *state)
>  {
> -	struct ag71xx *ag = netdev_priv(ndev);
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> +
> +	if (state->interface != PHY_INTERFACE_MODE_NA &&
> +	    state->interface != PHY_INTERFACE_MODE_GMII &&
> +	    state->interface != PHY_INTERFACE_MODE_MII) {
> +		bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
> +		return;
> +	}
> +
> +	phylink_set(mask, MII);
> +
> +	/* flow control is not supported */
> +
> +	phylink_set(mask, 10baseT_Half);
> +	phylink_set(mask, 10baseT_Full);
> +	phylink_set(mask, 100baseT_Half);
> +	phylink_set(mask, 100baseT_Full);
>  
> -	ag71xx_link_adjust(ag, true);
> +	if (state->interface == PHY_INTERFACE_MODE_NA &&
> +	    state->interface == PHY_INTERFACE_MODE_GMII) {

This is always false.

Apart from that, from just reading the patch I have no further concerns.

Thanks.
Oleksij Rempel Oct. 22, 2019, 5:12 a.m. UTC | #4 
On 22.10.19 00:21, Russell King - ARM Linux admin wrote:
> On Mon, Oct 21, 2019 at 07:38:07AM +0200, Oleksij Rempel wrote:
>> +static void ag71xx_mac_validate(struct phylink_config *config,
>> +			    unsigned long *supported,
>> +			    struct phylink_link_state *state)
>>   {
>> -	struct ag71xx *ag = netdev_priv(ndev);
>> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
>> +
>> +	if (state->interface != PHY_INTERFACE_MODE_NA &&
>> +	    state->interface != PHY_INTERFACE_MODE_GMII &&
>> +	    state->interface != PHY_INTERFACE_MODE_MII) {
>> +		bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
>> +		return;
>> +	}
>> +
>> +	phylink_set(mask, MII);
>> +
>> +	/* flow control is not supported */
>> +
>> +	phylink_set(mask, 10baseT_Half);
>> +	phylink_set(mask, 10baseT_Full);
>> +	phylink_set(mask, 100baseT_Half);
>> +	phylink_set(mask, 100baseT_Full);
>>   
>> -	ag71xx_link_adjust(ag, true);
>> +	if (state->interface == PHY_INTERFACE_MODE_NA &&
>> +	    state->interface == PHY_INTERFACE_MODE_GMII) {
> 
> This is always false.

... I shame to myself :(

> Apart from that, from just reading the patch I have no further concerns.

ok. thx!

Kind regards,
Oleksij Rempel