On Thu, May 09, 2019 at 04:48:44PM +0200, Andrew Lunn wrote:
> On Wed, May 08, 2019 at 11:13:29PM +0200, Michael Grzeschik wrote:
> > Cc: tristram...@microchip.com
> > Signed-off-by: Michael Grzeschik <m.grzesc...@pengutronix.de>
> > ---
> >  drivers/net/dsa/microchip/Kconfig       |   16 +
> >  drivers/net/dsa/microchip/Makefile      |    2 +
> >  drivers/net/dsa/microchip/ksz8863.c     | 1026 +++++++++++++++++++++++
> >  drivers/net/dsa/microchip/ksz8863_reg.h |  605 +++++++++++++
> >  drivers/net/dsa/microchip/ksz8863_smi.c |  105 +++
> >  drivers/net/dsa/microchip/ksz_priv.h    |    3 +
> >  include/net/dsa.h                       |    2 +
> >  net/dsa/Kconfig                         |    7 +
> >  net/dsa/tag_ksz.c                       |   45 +
> >  9 files changed, 1811 insertions(+)
> >  create mode 100644 drivers/net/dsa/microchip/ksz8863.c
> >  create mode 100644 drivers/net/dsa/microchip/ksz8863_reg.h
> >  create mode 100644 drivers/net/dsa/microchip/ksz8863_smi.c
> > 
> > diff --git a/drivers/net/dsa/microchip/Kconfig 
> > b/drivers/net/dsa/microchip/Kconfig
> > index bea29fde9f3d1..a6fa6ae972951 100644
> > --- a/drivers/net/dsa/microchip/Kconfig
> > +++ b/drivers/net/dsa/microchip/Kconfig
> > @@ -14,3 +14,19 @@ config NET_DSA_MICROCHIP_KSZ9477_SPI
> >     depends on NET_DSA_MICROCHIP_KSZ9477 && SPI
> >     help
> >       Select to enable support for registering switches configured through 
> > SPI.
> > +
> > +menuconfig NET_DSA_MICROCHIP_KSZ8863
> > +   tristate "Microchip KSZ8863 series switch support"
> > +   depends on NET_DSA
> > +   select NET_DSA_TAG_KSZ8863
> > +   select NET_DSA_MICROCHIP_KSZ_COMMON
> > +   help
> > +     This driver adds support for Microchip KSZ8863 switch chips.
> > +
> > +config NET_DSA_MICROCHIP_KSZ8863_SMI
> 
> > +   tristate "KSZ series SMI connected switch driver"
> > +   depends on NET_DSA_MICROCHIP_KSZ8863
> > +   default y
> > +   help
> > +     Select to enable support for registering switches configured through 
> > SMI.
> 
> SMI is a synonym for MDIO. So we should make it clear, this is a
> proprietary version. "... through Microchip SMI".
> 
> You might also want to either depend on or select mdio-bitbang, since
> that is the other driver which supports Microchip SMI.

Right, I will add the comment and include the depend to mdio-bitbang.

> > +static int ksz_spi_read(struct ksz_device *dev, u32 reg, u8 *data,
> > +                   unsigned int len)
> > +{
> > +   int i = 0;
> > +
> > +   for (i = 0; i < len; i++)
> > +           data[i] = (u8)mdiobus_read(dev->bus, 0,
> > +                                       (reg + i) | MII_ADDR_SMI0);
> 
> mdiobus_read() and mdiobus_write() can return an error, which is why
> is returns an int. Please check for the error and return it.

Thanks for pointing this out. I will fix it!

> > diff --git a/include/net/dsa.h b/include/net/dsa.h
> > index 6aaaadd6a413c..57fbf3e722362 100644
> > --- a/include/net/dsa.h
> > +++ b/include/net/dsa.h
> > @@ -44,6 +44,7 @@ struct phylink_link_state;
> >  #define DSA_TAG_PROTO_TRAILER_VALUE                11
> >  #define DSA_TAG_PROTO_8021Q_VALUE          12
> >  #define DSA_TAG_PROTO_SJA1105_VALUE                13
> > +#define DSA_TAG_PROTO_KSZ8863_VALUE                14
> >  
> >  enum dsa_tag_protocol {
> >     DSA_TAG_PROTO_NONE              = DSA_TAG_PROTO_NONE_VALUE,
> > @@ -60,6 +61,7 @@ enum dsa_tag_protocol {
> >     DSA_TAG_PROTO_TRAILER           = DSA_TAG_PROTO_TRAILER_VALUE,
> >     DSA_TAG_PROTO_8021Q             = DSA_TAG_PROTO_8021Q_VALUE,
> >     DSA_TAG_PROTO_SJA1105           = DSA_TAG_PROTO_SJA1105_VALUE,
> > +   DSA_TAG_PROTO_KSZ8863           = DSA_TAG_PROTO_KSZ8863_VALUE,
> >  };
> 
> Please put all the tag driver changes into a separate patch.

Right, I will ad that to another patch.

> > +static struct sk_buff *ksz8863_rcv(struct sk_buff *skb, struct net_device 
> > *dev,
> > +                              struct packet_type *pt)
> > +{
> > +   /* Tag decoding */
> > +   u8 *tag = skb_tail_pointer(skb) - KSZ_EGRESS_TAG_LEN;
> > +   unsigned int port = tag[0] & 1;
> 
> Does this device only have 2 ports, 0 and 1?
> 

Yes this is a very basic "switch" only managing two physical ports.

Refer Page 28 Chapter 3.7 "Tail Tagging Mode"
in the Reference Manual of the switch:

http://ww1.microchip.com/downloads/en/DeviceDoc/00002335B.pdf

> > +   unsigned int len = KSZ_EGRESS_TAG_LEN;
> > +
> > +   return ksz_common_rcv(skb, dev, port, len);
> > +}
> 
>   Andrew
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Attachment: signature.asc
Description: PGP signature

Reply via email to