Re: [PATCH] staging: mt7621-dts: remove obsolete switch node

[Andrew Lunn]

On Fri, Jan 08, 2021 at 10:51:55AM +0800, DENG Qingfang wrote:
> This was for OpenWrt's swconfig driver, which never made it upstream,
> and was also superseded by MT7530 DSA driver.

What about
Documentation/devicetree/bindings/net/mediatek,mt7620-gsw.txt ?
Should that also be removed?

   Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 net] staging: octeon: Drop on uncorrectable alignment or FCS error

[Andrew Lunn]

> diff --git a/drivers/staging/octeon/ethernet-rx.c 
> b/drivers/staging/octeon/ethernet-rx.c
> index 2c16230..9ebd665 100644
> --- a/drivers/staging/octeon/ethernet-rx.c
> +++ b/drivers/staging/octeon/ethernet-rx.c
> @@ -69,15 +69,17 @@ static inline int cvm_oct_check_rcv_error(struct cvmx_wqe 
> *work)
>   else
>   port = work->word1.cn38xx.ipprt;
>  
> - if ((work->word2.snoip.err_code == 10) && (work->word1.len <= 64)) {
> + if ((work->word2.snoip.err_code == 10) && (work->word1.len <= 64))

It would be nice to replace all these err_code magic numbers with #defines.

You should also replace 64 with ETH_ZLEN + ETH_FCS_LEN. I also wonder
if <= should be just < ?

>   /*
>* Ignore length errors on min size packets. Some
>* equipment incorrectly pads packets to 64+4FCS
>* instead of 60+4FCS.  Note these packets still get
>* counted as frame errors.
>*/
> - } else if (work->word2.snoip.err_code == 5 ||
> -work->word2.snoip.err_code == 7) {
> + return 0;
> +
> + if (work->word2.snoip.err_code == 5 ||
> + work->word2.snoip.err_code == 7) {
>   /*
>* We received a packet with either an alignment error
>* or a FCS error. This may be signalling that we are
> @@ -108,7 +110,10 @@ static inline int cvm_oct_check_rcv_error(struct 
> cvmx_wqe *work)
>   /* Port received 0xd5 preamble */
>   work->packet_ptr.s.addr += i + 1;
>   work->word1.len -= i + 5;
> - } else if ((*ptr & 0xf) == 0xd) {
> + return 0;
> + }
> +
> + if ((*ptr & 0xf) == 0xd) {

The comments are not so clear what is going on here. Can this
incorrectly match a destination MAC address of xD:XX:XX:XX:XX:XX.

>   /* Port received 0xd preamble */
>   work->packet_ptr.s.addr += i;
>   work->word1.len -= i + 4;

Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 net] staging: octeon: repair "fixed-link" support

[Andrew Lunn]

> --- a/drivers/staging/octeon/ethernet.c
> +++ b/drivers/staging/octeon/ethernet.c
> @@ -13,6 +13,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -892,6 +893,14 @@ static int cvm_oct_probe(struct platform_device *pdev)
>   break;
>   }
>  
> + if (priv->of_node && 
> of_phy_is_fixed_link(priv->of_node)) {
> + if (of_phy_register_fixed_link(priv->of_node)) {
> + netdev_err(dev, "Failed to register 
> fixed link for interface %d, port %d\n",
> +interface, priv->port);
> + dev->netdev_ops = NULL;
> + }
> + }
> +
>   if (!dev->netdev_ops) {
>   free_netdev(dev);
>   } else if (register_netdev(dev) < 0) {
> -- 
> 2.10.2

I would also expect a call to of_phy_deregister_fixed_link() somewhere
in the driver.

   Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 net] staging: octeon: repair "fixed-link" support

[Andrew Lunn]

> + if (priv->of_node && 
> of_phy_is_fixed_link(priv->of_node)) {
> + if (of_phy_register_fixed_link(priv->of_node)) {
> + netdev_err(dev, "Failed to register 
> fixed link for interface %d, port %d\n",
> +interface, priv->port);
> + dev->netdev_ops = NULL;
> + }
> + }
> +
>   if (!dev->netdev_ops) {
>   free_netdev(dev);

Setting dev->netdev_ops to NULL to indicate an error is pretty
odd. But this is staging. How about cleaning this
up. of_phy_register_fixed_link() returns an -errno which you should
return.

Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: FW: [PATCH v2] staging: intel-gwdpa: gswip: Introduce Gigabit Ethernet Switch (GSWIP) device driver

[Andrew Lunn]

> > We are trying to upstream the datapath code for Intel new NoC gateway
> > (please refer to intel-gwdpa.txt at the end of the patch). It consists of
> > ethernet, WIFI and passive optics handling. Since the code is quite huge, we
> > have broken it into parts for internal review.
> > 
> > As we have seen past upstream example such as fsl/dpaa, we thought that it
> > is better for us to start the upstreaming of the driver into staging folder
> > to get feedback from the community.
> > 
> > Is this the right approach? Or do we upstream all the drivers into
> > drivers/soc folder when we have all the drivers ready?
> 
> Why is drivers/soc/ the place to put networking drivers?
> 
> Please please please work with the Intel Linux kernel developers who
> know how to do this type of thing and do not require the kernel
> community to teach you all the proper development model and methods
> here.

I see a lot in common with dpaa2 here. You have a non traditional
hardware architecture. That means it does not nicely fit into the tree
as other drivers do.

There also appears to of been a huge amount of code development behind
closed doors, same as dpaa2. And because of the non traditional
architecture, you have had to make all sorts of design decisions. And
because that happened behind closed door, i'm sure some are
wrong. dpaa2 has been in staging for around 2 1/2 years now. It takes
that amount of time to discuss how non traditional hardware should be
supported in Linux, an re-write the drivers as needed because of the
wrong design decisions.

I kind of expect you are going to have a similar experience. So as
well as getting the Intel Linux kernel developers involved for process
and architecture support, you might want to look at how the dpaa2
drivers have evolved, what they got wrong, what they got right. How is
your hardware similar and different. And look at what parts of dpaa2
have moved out of staging, and maybe consider that code as a good
model to follow.

  Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 4/4] [RFC] staging/net: move AF_X25 into drivers/staging

[Andrew Lunn]

On Wed, Dec 11, 2019 at 08:10:34AM +0100, Krzysztof Hałasa wrote:
> Arnd,
> 
> Arnd Bergmann  writes:
> 
> > - Most other supported HDLC hardware that we supoprt is for the ISA or
> >   PCI buses.
> 
> I would be surprised if there is anybody left with ISA sync serial
> stuff, but the PCI hardware still has some users - these machines don't
> need to be upgraded yearly. Most people have migrated away, though.

Hi Krzysztof, Arnd

I have a use case for hdlc_cisco and hdlc_raw_eth. But it uses a lot
of out of tree code, the DAHDI driver framework for E1 cards, and an
E1 card which is not part of DAHDI.

Given how much of this is out of tree, i would understand if you
eventually decide to remove this HDLC code.

   Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v1] staging: intel-dpa: gswip: Introduce Gigabit Ethernet Switch (GSWIP) device driver

[Andrew Lunn]

On Mon, Nov 04, 2019 at 01:20:09PM +0100, Greg KH wrote:
> On Mon, Nov 04, 2019 at 07:22:20PM +0800, Jack Ping CHNG wrote:
> > This driver enables the Intel's LGM SoC GSWIP block.
> > GSWIP is a core module tailored for L2/L3/L4+ data plane and QoS functions.
> > It allows CPUs and other accelerators connected to the SoC datapath
> > to enqueue and dequeue packets through DMAs.
> > Most configuration values are stored in tables such as
> > Parsing and Classification Engine tables, Buffer Manager tables and
> > Pseudo MAC tables.
> 
> Why is this being submitted to staging?  What is wrong with the "real"
> part of the kernel for this?

Or even, what is wrong with the current driver?
drivers/net/dsa/lantiq_gswip.c?

Jack, your patch does not seem to of made it to any of the lists. So i
cannot comment on it contents. If this is a switch driver, please
ensure you Cc: the usual suspects for switch drivers.

   Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 02/20] staging: wfx: add support for I/O access

[Andrew Lunn]

On Thu, Sep 19, 2019 at 10:52:35AM +, Jerome Pouiller wrote:
> +static int wfx_sdio_copy_from_io(void *priv, unsigned int reg_id,
> +  void *dst, size_t count)
> +{
> + struct wfx_sdio_priv *bus = priv;
> + unsigned int sdio_addr = reg_id << 2;
> + int ret;
> +
> + BUG_ON(reg_id > 7);

Hi Jerome

BUG_ON should only be used when the system is corrupted, and there is
no alternative than to stop the machine, so it does not further
corrupt itself. Accessing a register which does not exist is not a
reason the kill the machine. A WARN() and a return of -EINVAL would be
better.

Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: octeon-ethernet: fix incorrect PHY mode

[Andrew Lunn]

> -static void cvm_set_rgmii_delay(struct device_node *np, int iface, int port)
> +static void cvm_set_rgmii_delay(struct octeon_ethernet *priv, int iface,
> + int port)
>  {
> + struct device_node *np = priv->of_node;
>   u32 delay_value;
> + bool rx_delay;
> + bool tx_delay;
>  
> - if (!of_property_read_u32(np, "rx-delay", _value))
> + /* By default, both RX/TX delay is enabled in
> +  * __cvmx_helper_rgmii_enable().
> +  */
> + rx_delay = true;
> + tx_delay = true;
> +
> + if (!of_property_read_u32(np, "rx-delay", _value)) {
>   cvmx_write_csr(CVMX_ASXX_RX_CLK_SETX(port, iface), delay_value);
> - if (!of_property_read_u32(np, "tx-delay", _value))
> + rx_delay = delay_value > 0;
> + }
> + if (!of_property_read_u32(np, "tx-delay", _value)) {
>   cvmx_write_csr(CVMX_ASXX_TX_CLK_SETX(port, iface), delay_value);
> + tx_delay = delay_value > 0;
> + }
> +
> + if (!rx_delay && !tx_delay)
> + priv->phy_mode = PHY_INTERFACE_MODE_RGMII_ID;
> + else if (!rx_delay)
> + priv->phy_mode = PHY_INTERFACE_MODE_RGMII_RXID;
> + else if (!tx_delay)
> + priv->phy_mode = PHY_INTERFACE_MODE_RGMII_TXID;
> + else
> + priv->phy_mode = PHY_INTERFACE_MODE_RGMII;

Humm, that is unique, as far as i know. Every other MAC driver uses
of_get_phy_mode() to get the value out of device tree. The proprietary
delay values can then be used to fine tune the basic delay setting
read from DT.

Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: mt7621-eth/ethtool.c: Correction of SPDX license identifier

[Andrew Lunn]

> See the patch about all of this from Thomas on lkml yesterday for
> why this is the case.

Hi Greg

Thanks for the info. I had not seen this, and i guess other have not
as well. So here is a link to the patch.

https://lkml.org/lkml/2019/1/28/1975

Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: mt7621-eth/ethtool.c: Correction of SPDX license identifier

[Andrew Lunn]

On Wed, Jan 30, 2019 at 02:48:27PM +, Carlos Henrique Lima Melara wrote:
>   This patch fix the checkpatch.p1 warning:
> 
>   WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
>   +/*
> 
>   It includes the SPDX expression for GPL-2.0 and corrects the comment 
> format to suit the kernel's coding style.
> 
> Signed-off-by: Carlos (Charles) Henrique Lima Melara 
> 
> ---
>  drivers/staging/mt7621-eth/ethtool.c | 22 --
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/staging/mt7621-eth/ethtool.c 
> b/drivers/staging/mt7621-eth/ethtool.c
> index 40a7d47be913..49d417de64c8 100644
> --- a/drivers/staging/mt7621-eth/ethtool.c
> +++ b/drivers/staging/mt7621-eth/ethtool.c
> @@ -1,15 +1,17 @@
> -/*   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
> +// SPDX-License-Identifier: GPL-2.0

Hi Carlos

drivers/staging/mt7621-eth$ grep LICENSE *
gsw_mt7621.c:MODULE_LICENSE("GPL");
mtk_eth_soc.c:MODULE_LICENSE("GPL");

And include/linux/module.h 
says:

/*
 * The following license idents are currently accepted as indicating free
 * software modules
 *
 *  "GPL"   [GNU Public License v2 or later]
 *  "GPL v2"[GNU Public License v2]

So the SPDX string probably does not match the MODULE_LICENSE.

   Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] dt-bindings: net: dsa: ksz9477: fix indentation for switch spi bindings

[Andrew Lunn]

On Tue, Dec 25, 2018 at 12:45:11PM +0100, Sergio Paracuellos wrote:
> Hi David,
> 
> On Mon, Dec 24, 2018 at 11:15 PM David Miller  wrote:
> >
> > From: Sergio Paracuellos 
> > Date: Sat, 22 Dec 2018 08:39:09 +0100
> >
> > > Switch bindings for spi managed mode are using spaces instead of tabs.
> > > Fix them to get a file with a proper kernel indentation style.
> > >
> > > Signed-off-by: Sergio Paracuellos 
> >
> > This doesn't apply to any of my trees so I'm going to assume this was
> > targetting the devicetree GIT tree or something like that.
> 
> Actually, this was rebased onto linux-next. Which tree do you want me
> to rebase onto?

Hi Sergio

It should be based on net-next. However, that is closed now, so please
wait until it reopens in about two weeks time.

 Thanks
Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 1/2] net: dsa: ksz9477: add I2C managed mode support

[Andrew Lunn]

On Sun, Dec 16, 2018 at 10:49:52AM +0100, Sergio Paracuellos wrote:
> In this mode the switch device and the internal phys will be managed via
> I2C interface.
> 
> Signed-off-by: Sergio Paracuellos 
> ---
> Changes v2:
> - Use dev->txbuf as transmition buffer which is allocated using
>   kernel allocators avoiding some possible DMA issues using the
>   stack.
>  drivers/net/dsa/microchip/Kconfig   |   6 +
>  drivers/net/dsa/microchip/Makefile  |   1 +
>  drivers/net/dsa/microchip/ksz9477_i2c.c | 269 
>  3 files changed, 276 insertions(+)
>  create mode 100644 drivers/net/dsa/microchip/ksz9477_i2c.c
> 
> diff --git a/drivers/net/dsa/microchip/Kconfig 
> b/drivers/net/dsa/microchip/Kconfig
> index a8caf9249d50..162fec43d204 100644
> --- a/drivers/net/dsa/microchip/Kconfig
> +++ b/drivers/net/dsa/microchip/Kconfig
> @@ -14,3 +14,9 @@ config NET_DSA_MICROCHIP_KSZ9477_SPI
>   depends on NET_DSA_MICROCHIP_KSZ9477 && SPI
>   help
> Select to enable support for registering switches configured through 
> SPI.
> +
> +config NET_DSA_MICROCHIP_KSZ9477_I2C
> + tristate "KSZ series I2C connected switch driver"
> + depends on NET_DSA_MICROCHIP_KSZ9477 && I2C
> + help
> +   Select to enable support for registering switches configured through 
> I2C.
> diff --git a/drivers/net/dsa/microchip/Makefile 
> b/drivers/net/dsa/microchip/Makefile
> index 3142c18b8f57..eb9b768face2 100644
> --- a/drivers/net/dsa/microchip/Makefile
> +++ b/drivers/net/dsa/microchip/Makefile
> @@ -1,3 +1,4 @@
>  obj-$(CONFIG_NET_DSA_MICROCHIP_KSZ_COMMON)   += ksz_common.o
>  obj-$(CONFIG_NET_DSA_MICROCHIP_KSZ9477)  += ksz9477.o
>  obj-$(CONFIG_NET_DSA_MICROCHIP_KSZ9477_SPI)  += ksz9477_spi.o
> +obj-$(CONFIG_NET_DSA_MICROCHIP_KSZ9477_I2C)  += ksz9477_i2c.o
> diff --git a/drivers/net/dsa/microchip/ksz9477_i2c.c 
> b/drivers/net/dsa/microchip/ksz9477_i2c.c
> new file mode 100644
> index ..3c9cba6e254b
> --- /dev/null
> +++ b/drivers/net/dsa/microchip/ksz9477_i2c.c
> @@ -0,0 +1,269 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Microchip KSZ series register access through I2C
> + *
> + * Author: Sergio Paracuellos 
> + */
> +
> +#include 
> +#include 
> +#include 
> +
> +#include "ksz_priv.h"
> +
> +/* Enough to read all switch port registers. */
> +#define I2C_TX_BUF_LEN  0x100
> +
> +/**
> + * ksz_i2c_read_reg - issue read register command
> + * @client: i2c client structure
> + * @reg: The register address.
> + * @val: The buffer to return the result into.
> + * @len: The length of data expected.
> + *
> + * This is the low level read call that issues the necessary i2c message(s)
> + * to read data from the register specified in @reg.
> + */
> +static int ksz_i2c_read_reg(struct i2c_client *client, u32 reg, u8 *val,
> + unsigned int len)
> +{
> + struct i2c_adapter *adapter = client->adapter;
> + struct i2c_msg msg[2];
> + int ret;
> +
> + msg[0].addr = client->addr;
> + msg[0].flags = 0;
> + msg[0].len = 2;
> + msg[0].buf = val;
> +
> + msg[1].addr = client->addr;
> + msg[1].flags = I2C_M_RD;
> + msg[1].len = len;
> + msg[1].buf = val;
> +
> + ret = i2c_transfer(adapter, msg, 2);
> + return (ret != 2) ? -EREMOTEIO : 0;
> +}
> +
> +static int ksz_i2c_read(struct ksz_device *dev, u32 reg, u8 *data,
> + unsigned int len)
> +{
> + struct i2c_client *client = dev->priv;
> + int ret;
> +
> + len = (len > I2C_TX_BUF_LEN) ? I2C_TX_BUF_LEN : len;
> + dev->txbuf[0] = (u8)(reg >> 8);
> + dev->txbuf[1] = (u8)reg;
> +
> + ret = ksz_i2c_read_reg(client, reg, dev->txbuf, len);
> + if (!ret)
> + memcpy(data, dev->txbuf, len);
> +
> + return ret;
> +}
> +
> +static int ksz_i2c_read8(struct ksz_device *dev, u32 reg, u8 *val)
> +{
> + return ksz_i2c_read(dev, reg, val, 1);
> +}
> +
> +static int ksz_i2c_read16(struct ksz_device *dev, u32 reg, u16 *val)
> +{
> + int ret = ksz_i2c_read(dev, reg, (u8 *)val, 2);
> +
> + if (!ret)
> + *val = be16_to_cpu(*val);
> +
> + return ret;
> +}
> +
> +static int ksz_i2c_read24(struct ksz_device *dev, u32 reg, u32 *val)
> +{
> + int ret;
> +
> + *val = 0;
> + ret = ksz_i2c_read(dev, reg, (u8 *)val, 3);
> +
> + if (!ret) {
> + *val = be32_to_cpu(*val);
> + /* convert to 24 bit */
> + *val >>= 8;
> + }
> +
> + return ret;
> +}
> +
> +static int ksz_i2c_read32(struct ksz_device *dev, u32 reg, u32 *val)
> +{
> + int ret = ksz_i2c_read(dev, reg, (u8 *)val, 4);
> +
> + if (!ret)
> + *val = be32_to_cpu(*val);
> +
> + return ret;
> +}
> +
> +/**
> + * ksz_i2c_write_reg - issue write register command
> + * @client: i2c client structure
> + * @reg: The register address.
> + * @val: value to write
> + * @len: The length of data
> + *
> + * This is the low level write call 

Re: [PATCH 1/2] net: dsa: ksz9477: add I2C managed mode support

[Andrew Lunn]

> I'll change these two to to get memory from kernel allocators instead
> of using the stack. Thanks for let me know this.

There appear to be other cases as well. Please review the whole
driver.

Thanks
Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] dt-bindings: net: dsa: ksz9477: add sample of switch bindings managed in i2c mode

[Andrew Lunn]

On Sun, Dec 16, 2018 at 08:57:41AM +0100, Sergio Paracuellos wrote:
> Add device-tree binding example of the ksz9477 switch managed in i2c mode.
> 
> Cc: devicet...@vger.kernel.org
> Signed-off-by: Sergio Paracuellos 
> ---
>  .../devicetree/bindings/net/dsa/ksz.txt   | 50 +++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/ksz.txt 
> b/Documentation/devicetree/bindings/net/dsa/ksz.txt
> index 0f407fb371ce..9e715358cebb 100644
> --- a/Documentation/devicetree/bindings/net/dsa/ksz.txt
> +++ b/Documentation/devicetree/bindings/net/dsa/ksz.txt
> @@ -74,3 +74,53 @@ Ethernet switch connected via SPI to the host, CPU port 
> wired to eth0:
>   };
>   };
>   };
> +
> +Ethernet switch connected via I2C to the host, CPU port wired to eth0:
> +
> + eth0: ethernet@10001000 {
> + fixed-link {
> + speed = <1000>;
> + full-duplex;
> + };
> + };
> +
> + i2c0: i2c@f8008000 {
> + ksz9897: ksz9897@0 {

Hi Sergio

You should use a generic name here. Plus the @X needs to be the same as the reg 
value.
So switch: ksz9897@5f.

   Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] net: dsa: ksz9477: add I2C managed mode support

[Andrew Lunn]

On Sun, Dec 16, 2018 at 08:57:40AM +0100, Sergio Paracuellos wrote:
> +static int ksz_i2c_read_reg(struct i2c_client *client, u32 reg, u8 *val,
> + unsigned int len)
> +{
> + struct i2c_adapter *adapter = client->adapter;
> + struct i2c_msg msg[2];
> + u8 txd[2];

Hi Sergio

I'm not sure that having the TX buffer on the stack is safe. If the
i2c bus master is using DMA, you then DMA from the stack, which some
architectures memory models do no allow. You have to use memory which
comes from an alloc function.

> + int ret;
> +
> + txd[0] = (u8)(reg >> 8);
> + txd[1] = (u8)reg;
> +
> + msg[0].addr = client->addr;
> + msg[0].flags = 0;
> + msg[0].len = 2;
> + msg[0].buf = txd;
> +
> + msg[1].addr = client->addr;
> + msg[1].flags = I2C_M_RD;
> + msg[1].len = len;
> + msg[1].buf = val;

You potentially have the same issue with val.

Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [v2, 1/5] net: dpaa2: move DPAA2 PTP driver out of staging/

[Andrew Lunn]

> +++ b/drivers/net/ethernet/freescale/dpaa2/Kconfig
> @@ -0,0 +1,15 @@
> +config FSL_DPAA2_ETH
> + tristate "Freescale DPAA2 Ethernet"
> + depends on FSL_MC_BUS && FSL_MC_DPIO

Could you add in here COMPILE_TEST? 

> + depends on NETDEVICES && ETHERNET

With the move out of staging, i don't think these two are required.

 Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] net: dpaa2: move DPAA2 PTP driver out of staging/

[Andrew Lunn]

> > struct dprtc_cmd_get_irq - Putting pad at the beginning of a struct seems 
> > very
> > odd. And it is not the only example.
> 
> [Y.b. Lu] This should depended on MC firmware and APIs I think. Once the MC 
> improves this, the APIs could be updated to fix this.

That is going to be hard to do. Ideally the driver should work with
any firmware version. You don't really want to force the user to
upgrade the driver/kernel and the firmware at the same time. So you
cannot for example remove this pad. What you might be able to do in
newer versions is actually use the space. But you have to be sure the
current code is correctly ignoring it and setting it to zero.

Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] net: dpaa2: move DPAA2 PTP driver out of staging/

[Andrew Lunn]

On Thu, Sep 27, 2018 at 07:12:27PM +0800, Yangbo Lu wrote:
> This patch is to move DPAA2 PTP driver out of staging/
> since the dpaa2-eth had been moved out.
> 
> Signed-off-by: Yangbo Lu 
> ---
>  drivers/net/ethernet/freescale/Kconfig |9 +
>  drivers/net/ethernet/freescale/dpaa2/Kconfig   |   15 +++
>  drivers/net/ethernet/freescale/dpaa2/Makefile  |6 --
>  .../ethernet/freescale/dpaa2}/dprtc-cmd.h  |0
>  .../rtc => net/ethernet/freescale/dpaa2}/dprtc.c   |0
>  .../rtc => net/ethernet/freescale/dpaa2}/dprtc.h   |0
>  .../rtc => net/ethernet/freescale/dpaa2}/rtc.c |0
>  .../rtc => net/ethernet/freescale/dpaa2}/rtc.h |0
>  drivers/staging/fsl-dpaa2/Kconfig  |8 
>  drivers/staging/fsl-dpaa2/Makefile |1 -
>  drivers/staging/fsl-dpaa2/rtc/Makefile |7 ---
>  11 files changed, 20 insertions(+), 26 deletions(-)
>  create mode 100644 drivers/net/ethernet/freescale/dpaa2/Kconfig
>  rename drivers/{staging/fsl-dpaa2/rtc => 
> net/ethernet/freescale/dpaa2}/dprtc-cmd.h (100%)
>  rename drivers/{staging/fsl-dpaa2/rtc => 
> net/ethernet/freescale/dpaa2}/dprtc.c (100%)
>  rename drivers/{staging/fsl-dpaa2/rtc => 
> net/ethernet/freescale/dpaa2}/dprtc.h (100%)
>  rename drivers/{staging/fsl-dpaa2/rtc => net/ethernet/freescale/dpaa2}/rtc.c 
> (100%)
>  rename drivers/{staging/fsl-dpaa2/rtc => net/ethernet/freescale/dpaa2}/rtc.h 
> (100%)

Hi Yangbo

Calling a ptp driver rtc.[ch] seems rather odd. Could you fixup the
name, change it to ptp.[ch]. Also, some of the function names, and
structures, rtc_probe->ptp_probe, rtc_remove->ptp_remove,
rtc_match_id_table-> ptp_match_id_table, etc.

ptp_dpaa2_adjfreq() probably should return err, not 0.
ptp_dpaa2_gettime() again does not return the error.
If fact, it seems like all the main functions ignore errors.

kzalloc() could be changed to devm_kzalloc() to simplify the cleanup
Can ptp_dpaa2_caps be made const?
dpaa2_phc_index does not appear to be used.
dev_set_drvdata(dev, NULL); is not needed.
Can rtc_drv be made const?
Is rtc.h used by anything other than rtc.c? It seems like it can be removed.

It seems like there is a lot of code in dprtc.c which is unused. rtc.c
does nothing with interrupts for example. Do you plan to make use of
this extra code? Or can it be removed leaving just what is needed?

struct dprtc_cmd_get_irq - Putting pad at the beginning of a struct
seems very odd. And it is not the only example.

  Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH net-next v2 2/2] dpaa2-eth: Move DPAA2 Ethernet driver from staging to drivers/net

[Andrew Lunn]

On Wed, Aug 29, 2018 at 03:50:02AM -0700, Joe Perches wrote:
> On Wed, 2018-08-29 at 04:42 -0500, Ioana Radulescu wrote:
> > The DPAA2 Ethernet driver supports Freescale/NXP SoCs with DPAA2
> > (DataPath Acceleration Architecture v2). The driver manages
> > network objects discovered on the fsl-mc bus.
> 
> Please use git 'format-patch -M' to make the diff
> smaller and more readable.

Hi Joe

I asked for this.

This is a request to move the driver from staging into the main
tree. We want to review the code in order to see if it has reached
mainline quality.

If the patch just lists a rename, not actually code, i cannot review
it, so i will just NACK it. We need to see the code.

Once the code has been reviewed and has all the needed Acked-by:, then
-M could be used. But this driver is not that far yet.

Thanks

Andrew


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH net-next] [RFC] dpaa2-eth: Move DPAA2 Ethernet driver from staging to drivers/net

[Andrew Lunn]

> +static int dpaa2_eth_probe(struct fsl_mc_device *dpni_dev)
> +{
> + struct device *dev;
> + struct net_device *net_dev = NULL;
> + struct dpaa2_eth_priv *priv = NULL;
> + int err = 0;
> +
> + dev = _dev->dev;
> +
> + /* Net device */
> + net_dev = alloc_etherdev_mq(sizeof(*priv), DPAA2_ETH_MAX_TX_QUEUES);
> + if (!net_dev) {
> + dev_err(dev, "alloc_etherdev_mq() failed\n");
> + return -ENOMEM;
> + }
> +
> + SET_NETDEV_DEV(net_dev, dev);
> + dev_set_drvdata(dev, net_dev);
> +
> + priv = netdev_priv(net_dev);
> + priv->net_dev = net_dev;
> +
> + priv->iommu_domain = iommu_get_domain_for_dev(dev);
> +
> + /* Obtain a MC portal */
> + err = fsl_mc_portal_allocate(dpni_dev, FSL_MC_IO_ATOMIC_CONTEXT_PORTAL,
> +  >mc_io);
> + if (err) {
> + if (err == -ENXIO)
> + err = -EPROBE_DEFER;
> + else
> + dev_err(dev, "MC portal allocation failed\n");
> + goto err_portal_alloc;
> + }
> +
> + /* MC objects initialization and configuration */
> + err = setup_dpni(dpni_dev);
> + if (err)
> + goto err_dpni_setup;
> +
> + err = setup_dpio(priv);
> + if (err)
> + goto err_dpio_setup;
> +
> + setup_fqs(priv);
> +
> + err = setup_dpbp(priv);
> + if (err)
> + goto err_dpbp_setup;
> +
> + err = bind_dpni(priv);
> + if (err)
> + goto err_bind;
> +
> + /* Add a NAPI context for each channel */
> + add_ch_napi(priv);
> +
> + /* Percpu statistics */
> + priv->percpu_stats = alloc_percpu(*priv->percpu_stats);
> + if (!priv->percpu_stats) {
> + dev_err(dev, "alloc_percpu(percpu_stats) failed\n");
> + err = -ENOMEM;
> + goto err_alloc_percpu_stats;
> + }
> + priv->percpu_extras = alloc_percpu(*priv->percpu_extras);
> + if (!priv->percpu_extras) {
> + dev_err(dev, "alloc_percpu(percpu_extras) failed\n");
> + err = -ENOMEM;
> + goto err_alloc_percpu_extras;
> + }
> +
> + err = netdev_init(net_dev);
> + if (err)
> + goto err_netdev_init;

At the end of netdev_init() you call netdev_register(). From that
point on, you device is live. Its .ndo's can be called

> +
> + /* Configure checksum offload based on current interface flags */
> + err = set_rx_csum(priv, !!(net_dev->features & NETIF_F_RXCSUM));
> + if (err)
> + goto err_csum;
> +
> + err = set_tx_csum(priv, !!(net_dev->features &
> +(NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM)));
> + if (err)
> + goto err_csum;
> +
> + err = alloc_rings(priv);
> + if (err)
> + goto err_alloc_rings;

How well does the device work if it has not allocated the rings yet,
when it is asked to do something?

> +
> + net_dev->ethtool_ops = _ethtool_ops;
> +
> + err = setup_irqs(dpni_dev);

How well do it work without interrupts being set up?

> + if (err) {
> + netdev_warn(net_dev, "Failed to set link interrupt, fall back 
> to polling\n");
> + priv->poll_thread = kthread_run(poll_link_state, priv,
> + "%s_poll_link", net_dev->name);
> + if (IS_ERR(priv->poll_thread)) {
> + netdev_err(net_dev, "Error starting polling thread\n");
> + goto err_poll_thread;
> + }
> + priv->do_link_poll = true;
> + }

Probably the correct place to register the netdev is here.

 Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: fsl-dpaa2/ethsw: Fix TCI values overwrite

[Andrew Lunn]

On Tue, Mar 27, 2018 at 08:10:50AM -0500, Razvan Stefanescu wrote:
> Previous implementation overwrites PCP value, assuming the default value is
> 0, instead of 7.
> 
> Avoid this by modifying helper function ethsw_port_set_tci() to
> ethsw_port_set_pvid() and make it update only the vlan_id of the tci_cfg
> struct.

Hi Razvan

It is a good idea to explain acronyms, especially for staging, since
there are patches for all sorts of devices, can you cannot expect
everybody to know network specific acronyms.

By PCP you mean Priority Code Point. TCI i have no idea about.

Looking at the code, i think you are changing the flow to become
read/modify/write, instead of just write, which is overwriting the
previously configured Priority Code Point?

Please try to add more details to your change logs, to help us
understand the change.

   Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v6 0/6] staging: Introduce DPAA2 Ethernet Switch driver

[Andrew Lunn]

On Thu, Mar 15, 2018 at 01:56:42PM +0300, Dan Carpenter wrote:
> On Thu, Mar 15, 2018 at 12:44:37AM +0100, Andrew Lunn wrote:
> > On Wed, Mar 14, 2018 at 10:55:52AM -0500, Razvan Stefanescu wrote:
> > > This patchset introduces the Ethernet Switch Driver for Freescale/NXP SoCs
> > > with DPAA2 (DataPath Acceleration Architecture v2). The driver manages
> > > switch objects discovered on the fsl-mc bus. A description of the driver
> > > can be found in the associated README file.
> > 
> > Hi Greg
> > 
> > This code has much better quality than the usual stuff in staging. I
> > see no reason not to merge it. 
> 
> Yeah.  It seems pretty decent.  Stuart, Laurentiu, care to comment?
> 
> Meanwhile, netdev and DaveM aren't even on the CC list and they're the
> ones to ultimately decide.

The patches are for staging, so it is GregKH who decides at this
point, not really DaveM.

   Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v6 0/6] staging: Introduce DPAA2 Ethernet Switch driver

[Andrew Lunn]

On Wed, Mar 14, 2018 at 10:55:52AM -0500, Razvan Stefanescu wrote:
> This patchset introduces the Ethernet Switch Driver for Freescale/NXP SoCs
> with DPAA2 (DataPath Acceleration Architecture v2). The driver manages
> switch objects discovered on the fsl-mc bus. A description of the driver
> can be found in the associated README file.

Hi Greg

This code has much better quality than the usual stuff in staging. I
see no reason not to merge it. 

Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 2/6] staging: fsl-dpaa2/ethsw: Add Freescale DPAA2 Ethernet Switch driver

[Andrew Lunn]

> Hello Andrew,
> 
> The current driver implementation uses only a single FDB for the switch,
> so  it is not possible configure multiple flooding domains to accommodate
> ports partitioning.

Ah, O.K. Rather than break somebodies network by wrongly flooding, it
would be better to return -EOPNOTSUPP when the requirement for the
second FDB is met. The offload to hardware will then not happen, and
the software bridge will do all the work.

Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 2/6] staging: fsl-dpaa2/ethsw: Add Freescale DPAA2 Ethernet Switch driver

[Andrew Lunn]

> +/* For the moment, only flood setting needs to be updated */
> +static int port_bridge_join(struct net_device *netdev,
> + struct net_device *upper_dev)
> +{
> + struct ethsw_port_priv *port_priv = netdev_priv(netdev);
> + struct ethsw_core *ethsw = port_priv->ethsw_data;
> + int i, err;
> +
> + for (i = 0; i < ethsw->sw_attr.num_ifs; i++)
> + if (ethsw->ports[i]->bridge_dev &&
> + (ethsw->ports[i]->bridge_dev != upper_dev)) {
> + netdev_err(netdev,
> +"Another switch port is connected to %s\n",
> +ethsw->ports[i]->bridge_dev->name);
> + return -EINVAL;
> + }
> +
> + /* Enable flooding */
> + err = ethsw_port_set_flood(port_priv, 1);
> + if (!err)
> + port_priv->bridge_dev = upper_dev;
> +
> + return err;
> +}

Hi Razvan

That is not what i was meaning.

brctl addbr br0
brctl addbr br1
brctl addif br0 lan0
brctl addif br0 lan1
brctl addif br1 lan2
brctl addif br1 lan3

Is there somewhere in the code which sets the scope for the flooding?
lan0 can flood to lan1, but it should not flood to lan2 or lan3, since
they are in a different bridge. I was expecting that
ethsw_port_set_flood() takes upper_dev, in order to configure which
ports it should flood to.

  Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 2/6] staging: fsl-dpaa2/ethsw: Add Freescale DPAA2 Ethernet Switch driver

[Andrew Lunn]

> +static int port_netdevice_event(struct notifier_block *unused,
> + unsigned long event, void *ptr)
> +{
> + struct net_device *netdev = netdev_notifier_info_to_dev(ptr);
> + struct netdev_notifier_changeupper_info *info = ptr;
> + struct net_device *upper_dev;
> + int err = 0;
> +
> + if (netdev->netdev_ops != _port_ops)
> + return NOTIFY_DONE;
> +
> + /* Handle just upper dev link/unlink for the moment */
> + if (event == NETDEV_CHANGEUPPER) {
> + upper_dev = info->upper_dev;
> + if (netif_is_bridge_master(upper_dev)) {
> + if (info->linking)
> + err = port_bridge_join(netdev);
> + else
> + err = port_bridge_leave(netdev);
> + }
> + }
> +
> + return notifier_from_errno(err);
> +}

I could be missing something here, but don't you need to pass to
port_bridge_join() which bridge the port is joining. There can be
multiple bridges, so you need to ensure the port joins the correct
bridge.

Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 2/6] staging: fsl-dpaa2/ethsw: Add Freescale DPAA2 Ethernet Switch driver

[Andrew Lunn]

On Mon, Mar 12, 2018 at 03:49:51AM -0500, Razvan Stefanescu wrote:

> +static irqreturn_t ethsw_irq0_handler(int irq_num, void *arg)
> +{
> + return IRQ_WAKE_THREAD;
> +}
> +

> +static int ethsw_setup_irqs(struct fsl_mc_device *sw_dev)
> +{
> + struct device *dev = _dev->dev;
> + struct ethsw_core *ethsw = dev_get_drvdata(dev);
> + u32 mask = DPSW_IRQ_EVENT_LINK_CHANGED;
> + struct fsl_mc_device_irq *irq;
> + int err;
> +
> + err = fsl_mc_allocate_irqs(sw_dev);
> + if (err) {
> + dev_err(dev, "MC irqs allocation failed\n");
> + return err;
> + }
> +
> + if (WARN_ON(sw_dev->obj_desc.irq_count != DPSW_IRQ_NUM)) {
> + err = -EINVAL;
> + goto free_irq;
> + }
> +
> + err = dpsw_set_irq_enable(ethsw->mc_io, 0, ethsw->dpsw_handle,
> +   DPSW_IRQ_INDEX_IF, 0);
> + if (err) {
> + dev_err(dev, "dpsw_set_irq_enable err %d\n", err);
> + goto free_irq;
> + }
> +
> + irq = sw_dev->irqs[DPSW_IRQ_INDEX_IF];
> +
> + err = devm_request_threaded_irq(dev, irq->msi_desc->irq,
> + ethsw_irq0_handler,
> + ethsw_irq0_handler_thread,
> + IRQF_NO_SUSPEND | IRQF_ONESHOT,
> + dev_name(dev), dev);

Hi Razvan

You can pass NULL instead of ethsw_irq0_handler.

Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 7/8] netdev: octeon-ethernet: Add Cavium Octeon III support.

[Andrew Lunn]

On Tue, Nov 28, 2017 at 04:55:39PM -0800, David Daney wrote:
> +static int bgx_probe(struct platform_device *pdev)
> +{
> + struct mac_platform_data platform_data;
> + const __be32 *reg;
> + u32 port;
> + u64 addr;
> + struct device_node *child;
> + struct platform_device *new_dev;
> + struct platform_device *pki_dev;
> + int numa_node, interface;
> + int i;
> + int r = 0;
> + char id[64];
> + u64 data;
> +
> + reg = of_get_property(pdev->dev.of_node, "reg", NULL);
> + addr = of_translate_address(pdev->dev.of_node, reg);
> + interface = (addr >> 24) & 0xf;
> + numa_node = (addr >> 36) & 0x7;

Hi David

You have these two a few times in the code. Maybe add a helper to do
it? The NUMA one i assume could go somewhere in the SoC code?

> +static int bgx_mix_init_from_fdt(void)
> +{
> + struct device_node  *node;
> + struct device_node  *parent = NULL;
> + int mix = 0;

> + /* Get the lmac index */
> + reg = of_get_property(lmac_fdt_node, "reg", NULL);
> + if (!reg)
> + goto err;
> +
> + mix_port_lmacs[mix].lmac = *reg;

I don't think of_get_property() deals with endianness. Is there any
danger of this driver being used on hardware with the other endianness
to what you have tested?

> +/**
> + * bgx_pki_init_from_param - Initialize the list of lmacs that connect to the
> + *pki from information in the "pki_port" parameter.
> + *
> + *The pki_port parameter format is as follows:
> + *pki_port=nbl
> + *where:
> + *   n = node
> + *   b = bgx
> + *   l = lmac
> + *
> + *Commas must be used to separate multiple lmacs:
> + *pki_port=000,100,110
> + *
> + *Asterisks (*) specify all possible characters in
> + *the subset:
> + *pki_port=00* (all lmacs of node0 bgx0).
> + *
> + *Missing lmacs identifiers default to all
> + *possible characters in the subset:
> + *pki_port=00 (all lmacs on node0 bgx0)
> + *
> + *Brackets ('[' and ']') specify the valid
> + *characters in the subset:
> + *pki_port=00[01] (lmac0 and lmac1 of node0 bgx0).
> + *
> + * Returns 0 if successful.
> + * Returns <0 for error codes.

I've not used kerneldoc much, but i suspect this is wrongly formated:

https://www.kernel.org/doc/html/v4.9/kernel-documentation.html#function-documentation

> +int bgx_port_ethtool_set_settings(struct net_device  *netdev,
> +   struct ethtool_cmd*cmd)
> +{
> + struct bgx_port_priv *p = bgx_port_netdev2priv(netdev);
> +
> + if (!capable(CAP_NET_ADMIN))
> + return -EPERM;

Not required. The enforces this. See dev_ethtool()

> +
> + if (p->phydev)
> + return phy_ethtool_sset(p->phydev, cmd);
> +
> + return -EOPNOTSUPP;
> +}
> +EXPORT_SYMBOL(bgx_port_ethtool_set_settings);
> +
> +int bgx_port_ethtool_nway_reset(struct net_device *netdev)
> +{
> + struct bgx_port_priv *p = bgx_port_netdev2priv(netdev);
> +
> + if (!capable(CAP_NET_ADMIN))
> + return -EPERM;

Also not needed.

> +static void bgx_port_adjust_link(struct net_device *netdev)
> +{
> + struct bgx_port_priv*priv = bgx_port_netdev2priv(netdev);
> + boollink_changed = false;
> + unsigned intlink;
> + unsigned intspeed;
> + unsigned intduplex;
> +
> + mutex_lock(>lock);
> +
> + if (!priv->phydev->link && priv->last_status.link)
> + link_changed = true;
> +
> + if (priv->phydev->link &&
> + (priv->last_status.link != priv->phydev->link ||
> +  priv->last_status.duplex != priv->phydev->duplex ||
> +  priv->last_status.speed != priv->phydev->speed))
> + link_changed = true;
> +
> + link = priv->phydev->link;
> + priv->last_status.link = priv->phydev->link;
> +
> + speed = priv->phydev->speed;
> + priv->last_status.speed = priv->phydev->speed;
> +
> + duplex = priv->phydev->duplex;
> + priv->last_status.duplex = priv->phydev->duplex;
> +
> + mutex_unlock(>lock);
> +
> + if (link_changed) {
> + struct port_status status;
> +
> + phy_print_status(priv->phydev);
> +
> + status.link = link ? 1 : 0;
> + status.duplex = duplex;
> + status.speed = speed;
> + if (!link) {
> + netif_carrier_off(netdev);
> +  /* Let TX drain. FIXME check that it is drained. */
> + mdelay(50);
> + }
> +  

Re: [PATCH v4 7/8] netdev: octeon-ethernet: Add Cavium Octeon III support.

[Andrew Lunn]

On Wed, Nov 29, 2017 at 10:11:38PM +0300, Dan Carpenter wrote:
> On Wed, Nov 29, 2017 at 09:37:15PM +0530, Souptick Joarder wrote:
> > >> +static int bgx_port_sgmii_set_link_speed(struct bgx_port_priv *priv, 
> > >> struct port_status status)
> > >> +{
> > >> +   u64 data;
> > >> +   u64 prtx;
> > >> +   u64 miscx;
> > >> +   int timeout;
> > >> +
> > 
> > >> +
> > >> +   switch (status.speed) {
> > >> +   case 10:
> > >
> > > In my opinion, instead of hard coding the value, is it fine to use ENUM ?
> >Similar comments applicable in other places where hard coded values are 
> > used.
> > 
> 
> 10 means 10M right?  That's not really a magic number.  It's fine.

There are also :
uapi/linux/ethtool.h:#define SPEED_10   10
uapi/linux/ethtool.h:#define SPEED_100  100
uapi/linux/ethtool.h:#define SPEED_1000 1000
uapi/linux/ethtool.h:#define SPEED_11
uapi/linux/ethtool.h:#define SPEED_10   10

 Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 7/8] netdev: octeon-ethernet: Add Cavium Octeon III support.

[Andrew Lunn]

On Wed, Nov 29, 2017 at 04:00:01PM +0530, Souptick Joarder wrote:

Hi Souptick

Please trim the code when giving reviews. We don't want to have to
page through 8K lines of code it find a few comments mixed in. Just
keep the beginning of the function you are commented on to make the
context clear. Cut the rest.

Thanks
Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 1/8] dt-bindings: Add Cavium Octeon Common Ethernet Interface.

[Andrew Lunn]

On Tue, Nov 28, 2017 at 04:55:33PM -0800, David Daney wrote:
> From: Carlos Munoz 
> 
> Add bindings for Common Ethernet Interface (BGX) block.
> 
> Acked-by: Rob Herring 
> Signed-off-by: Carlos Munoz 
> Signed-off-by: Steven J. Hill 
> Signed-off-by: David Daney 
> ---
>  .../devicetree/bindings/net/cavium-bgx.txt | 61 
> ++
>  1 file changed, 61 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/cavium-bgx.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/cavium-bgx.txt 
> b/Documentation/devicetree/bindings/net/cavium-bgx.txt
> new file mode 100644
> index ..830c5f08
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/cavium-bgx.txt
> @@ -0,0 +1,61 @@
> +* Common Ethernet Interface (BGX) block
> +
> +Properties:
> +
> +- compatible: "cavium,octeon-7890-bgx": Compatibility with all cn7xxx SOCs.
> +
> +- reg: The base address of the BGX block.
> +
> +- #address-cells: Must be <1>.
> +
> +- #size-cells: Must be <0>.  BGX addresses have no size component.
> +
> +A BGX block has several children, each representing an Ethernet
> +interface.
> +
> +
> +* Ethernet Interface (BGX port) connects to PKI/PKO
> +
> +Properties:
> +
> +- compatible: "cavium,octeon-7890-bgx-port": Compatibility with all
> +   cn7xxx SOCs.
> +
> +   "cavium,octeon-7360-xcv": Compatibility with cn73xx SOCs
> +   for RGMII.
> +
> +- reg: The index of the interface within the BGX block.
> +
> +Optional properties:
> +
> +- local-mac-address: Mac address for the interface.
> +
> +- phy-handle: phandle to the phy node connected to the interface.
> +
> +- phy-mode: described in ethernet.txt.
> +
> +- fixed-link: described in fixed-link.txt.
> +
> +Example:
> +
> + ethernet-mac-nexus@11800e000 {
> + compatible = "cavium,octeon-7890-bgx";
> + reg = <0x00011800 0xe000 0x 0x0100>;

Hi David

In the probe function we have:

+   reg = of_get_property(pdev->dev.of_node, "reg", NULL);
+   addr = of_translate_address(pdev->dev.of_node, reg);
+   interface = (addr >> 24) & 0xf;
+   numa_node = (addr >> 36) & 0x7;

Is this documented somewhere? The numa_node is particularly
interesting. MMIO changes depends on what node this is in the cluster?

Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3] staging: fsl-mc: move bus driver out of staging

[Andrew Lunn]

On Tue, Nov 28, 2017 at 05:27:57PM +0200, laurentiu.tu...@nxp.com wrote:
> diff --git a/drivers/staging/fsl-mc/bus/dpmcp.h b/drivers/bus/fsl-mc/dpmcp.h
> similarity index 100%
> rename from drivers/staging/fsl-mc/bus/dpmcp.h
> rename to drivers/bus/fsl-mc/dpmcp.h
> diff --git a/drivers/staging/fsl-mc/bus/dpmng-cmd.h 
> b/drivers/bus/fsl-mc/dpmng-cmd.h
> similarity index 100%
> rename from drivers/staging/fsl-mc/bus/dpmng-cmd.h
> rename to drivers/bus/fsl-mc/dpmng-cmd.h
> diff --git a/drivers/staging/fsl-mc/bus/dprc-cmd.h 
> b/drivers/bus/fsl-mc/dprc-cmd.h
> similarity index 100%
> rename from drivers/staging/fsl-mc/bus/dprc-cmd.h
> rename to drivers/bus/fsl-mc/dprc-cmd.h
> diff --git a/drivers/staging/fsl-mc/bus/dprc-driver.c 
> b/drivers/bus/fsl-mc/dprc-driver.c
> similarity index 99%
> rename from drivers/staging/fsl-mc/bus/dprc-driver.c
> rename to drivers/bus/fsl-mc/dprc-driver.c

Hi Laurentiu

It is hard to review code when we don't see it. 

   Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 7/8] netdev: octeon-ethernet: Add Cavium Octeon III support.

[Andrew Lunn]

> + if (link_changed != 0) {
> + struct port_status status;
> +
> + if (link_changed > 0) {
> + netdev_info(netdev, "Link is up - %d/%s\n",
> + priv->phydev->speed,
> + priv->phydev->duplex == DUPLEX_FULL ?
> + "Full" : "Half");
> + } else {
> + netdev_info(netdev, "Link is down\n");
> + }

phy_print_status()

Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 7/8] netdev: octeon-ethernet: Add Cavium Octeon III support.

[Andrew Lunn]

> + priv->phy_np = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0);
> + priv->phy_mode = of_get_phy_mode(pdev->dev.of_node);
> + /* If phy-mode absent, default to SGMII. */
> + if (priv->phy_mode < 0)
> + priv->phy_mode = PHY_INTERFACE_MODE_SGMII;
> +
> + if (priv->phy_mode == PHY_INTERFACE_MODE_1000BASEX)
> + priv->mode_1000basex = true;
> +
> + if (of_phy_is_fixed_link(pdev->dev.of_node))
> + priv->bgx_as_phy = true;
> +

...

> + priv->mode = bgx_port_get_mode(priv->node, priv->bgx, priv->index);
> +

It might be a good idea to verify priv->phy_mode and priv->mode are
compatible.

> + switch (priv->mode) {
> + case PORT_MODE_SGMII:
> + case PORT_MODE_RGMII:
> + priv->get_link = bgx_port_get_sgmii_link;
> + priv->set_link = bgx_port_set_xgmii_link;
> + break;
> + case PORT_MODE_XAUI:
> + case PORT_MODE_RXAUI:
> + case PORT_MODE_XLAUI:
> + case PORT_MODE_XFI:
> + case PORT_MODE_10G_KR:
> + case PORT_MODE_40G_KR4:
> + priv->get_link = bgx_port_get_xaui_link;
> + priv->set_link = bgx_port_set_xaui_link;
> + break;


  Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 5/6] staging: fsl-dpaa2/ethsw: Add README

[Andrew Lunn]

On Tue, Oct 03, 2017 at 10:07:41AM +, Razvan Stefanescu wrote:
> > -Original Message-
> > From: linux-arm-kernel [mailto:linux-arm-kernel-boun...@lists.infradead.org]
> > On Behalf Of Andrew Lunn
> > Sent: Tuesday, September 19, 2017 3:18 PM
> > To: Razvan Stefanescu <razvan.stefane...@nxp.com>
> > Cc: de...@driverdev.osuosl.org; Ruxandra Ioana Radulescu
> > <ruxandra.radule...@nxp.com>; a...@arndb.de; gre...@linuxfoundation.org;
> > Alexandru Marginean <alexandru.margin...@nxp.com>; linux-
> > ker...@vger.kernel.org; ag...@suse.de; stuyo...@gmail.com; Bogdan
> > Purcareata <bogdan.purcare...@nxp.com>; linux-arm-
> > ker...@lists.infradead.org; Laurentiu Tudor <laurentiu.tu...@nxp.com>
> > Subject: Re: [PATCH 5/6] staging: fsl-dpaa2/ethsw: Add README
> > 
> > On Tue, Sep 19, 2017 at 12:01:37PM +0300, Razvan Stefanescu wrote:
> > > +Driver uses the switch device driver model and exposes each switch port 
> > > as
> > > +a network interface, which can be included in a bridge. Traffic switched
> > > +between ports is offloaded into the hardware. Exposed network interfaces
> > > +are not used for I/O, they are used just for configuration. This
> > > +limitation is going to be addressed in the future.
> > 
> > Hi Razvan
> > 
> > Could you briefly describe how Ethernet frames get from the CPU to the
> > switch. This is what decided if you should write a plain switchdev
> > driver, or a DSA driver.
> > 
> > Andrew
> > 
> Hello Andrew,
> 
> CPU frame handling will be added in a later. Each netdevice associated 
> to a switch port will have I/O capabilities like dpaa2-ethernet devices.
> The dpaa2-ethsw will use ACLs to redirect specific types of frames
> (i.e BPDUs) to CPU.

Hi Razvan

I looked at the architecture documentation after i posted this
email. It looks like each switch port will get its own DMA queues, etc
on the host. It is not sharing one host interface to get packets to
the switch, which is what DSA does. So a pure switchdev driver is the
correct solution here.

  Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 3/6] staging: fsl-dpaa2/ethsw: Add ethtool support

[Andrew Lunn]

Hi Razvan

> +static void ethsw_get_drvinfo(struct net_device *netdev,
> +   struct ethtool_drvinfo *drvinfo)
> +{
> + struct ethsw_port_priv *port_priv = netdev_priv(netdev);
> + u16 version_major, version_minor;
> + int err;
> +
> + strlcpy(drvinfo->driver, KBUILD_MODNAME, sizeof(drvinfo->driver));
> + strlcpy(drvinfo->version, ethsw_drv_version, sizeof(drvinfo->version));

Software driver versions are mostly useless. I would suggest you
remove this.

   Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 5/6] staging: fsl-dpaa2/ethsw: Add README

[Andrew Lunn]

On Tue, Sep 19, 2017 at 12:01:37PM +0300, Razvan Stefanescu wrote:
> +Driver uses the switch device driver model and exposes each switch port as
> +a network interface, which can be included in a bridge. Traffic switched
> +between ports is offloaded into the hardware. Exposed network interfaces
> +are not used for I/O, they are used just for configuration. This
> +limitation is going to be addressed in the future.

Hi Razvan

Could you briefly describe how Ethernet frames get from the CPU to the
switch. This is what decided if you should write a plain switchdev
driver, or a DSA driver.

Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/6] staging: Introduce DPAA2 Ethernet Switch driver

[Andrew Lunn]

On Tue, Sep 19, 2017 at 12:01:32PM +0300, Razvan Stefanescu wrote:
> This patchset introduces the Ethernet Switch Driver for Freescale/NXP SoCs
> with DPAA2 (DataPath Acceleration Architecture v2). The driver manages
> switch objects discovered on the fsl-mc bus. A description of the driver
> can be found in the associated README file.

Hi Razvan

You should probably Cc: netdev with these patches, if you want people
who have written switch drivers to review you code. You can also drop
linux-arm-kernel, since there is nothing ARM specific in these
patches.

Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtlwifi: Improve debugging by using debugfs

[Andrew Lunn]

On Fri, Aug 25, 2017 at 08:47:00AM -0500, Larry Finger wrote:
> On 08/24/2017 08:54 PM, Andrew Lunn wrote:
> >netdev frowns upon debugfs. You should try to keep this altogether,
> >making it easy to throw away before the driver is moved out of
> >staging.
> >
> >You might want to look at ethtool -d. That will be accepted.
> 
> Andrew,
> 
> What is the problem with debugfs?

You should probably look back in the discussions on the netdev
mailling list. But basically, anything you want to export should
follow generic well defined interface, which can be used by other
drivers. debugfs tends to be a mess, a wild west, each driver doing
its own thing, not standardisation. It is O.K. for your own
development work, you can have your own out of tree patches adding in
debugfs, but such patches are unlikely to be accepted into mainline.
David has threatened to simply rip out all debugfs code from all
network drivers. There is push back on adding any new debugfs code,
and some driver writers have taken out debugfs code in their own
drivers, often replacing it with something generic all drivers can
use.
 
> Please suggest which driver has the best example of an ethtool -d
> implementation that we might study.

The API is very simple. In your ethtool_ops you need to function:
.get_regs_len, returns an int, the number of registers you can dump.
.get_regs returns the contents.

There are plenty of examples, e.g.:

http://elixir.free-electrons.com/linux/latest/source/drivers/net/ethernet/amd/pcnet32.c#L1436
 
If you need something more structured, look around, see if other
drivers have similar needs, and propose something generic.

> The first version of the debugfs changes were sent to wireless-drivers on
> July 2. Why are we first hearing of this objection nearly 2 months later?

Different reviewers tend to review different things. I personally
don't care so much who the vendor is, but look out for some specific
things i feel qualified to review. Ethernet PHYs and MDIO drivers,
drivers which make use of Ethernet PHYs, APIs which allow userspace to
write to registers, debugfs, changing MTUs, etc.

There are plenty of patches on netdev for me to review, so i don't
cast a wider net to other lists, like for example wireless-drivers.
You would of got this comment sooner or later, since you should be
posting to netdev at some point anyway. I can also imaging there is
also some reluctance to review staging code, until it is getting close
to moving out of staging.

Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtlwifi: Improve debugging by using debugfs

[Andrew Lunn]

On Thu, Aug 24, 2017 at 04:28:08PM -0500, Larry Finger wrote:
> The changes in this commit are also being sent to the main rtlwifi
> drivers in wireless-next; however, these changes will also be useful for
> any debugging of r8822be before it gets moved into the main tree.
> 
> Use debugfs to dump register and btcoex status, and also write registers
> and h2c.
> 
> We create topdir in /sys/kernel/debug/rtlwifi/, and use the MAC address
> as subdirectory with several entries to dump mac_reg, bb_reg, rf_reg etc.
> An example is
> /sys/kernel/debug/rtlwifi/00-11-22-33-44-55-66/mac_0
> 
> This change permits examination of device registers in a dynamic manner,
> a feature not available with the current debug mechanism.

Hi Larry

netdev frowns upon debugfs. You should try to keep this altogether,
making it easy to throw away before the driver is moved out of
staging.

You might want to look at ethtool -d. That will be accepted.

Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [net-next 1/2] net: ethernet: slicoss: add slicoss gigabit ethernet driver

[Andrew Lunn]

> > +   val = MII_BMCR << 16 | SLIC_PCR_AUTONEG |
> > +SLIC_PCR_AUTONEG_RST;
> > +   slic_write(sdev, SLIC_REG_WPHY, val);

> Thats essentially what I meant by setting a flag in the irq handler. The mdio
> function would have to check somehow if the irq has been fired (be it by means
> of a flag or a completion that is set by the irq handler and checked by the 
> mdio function). So I agree that it should work (if reading via the AP command
> is actually possible).

It seems odd you have a nice simple way to do writes, but reads are
very complex. There might be a simple read method hiding somewhere.

 Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [net-next 1/2] net: ethernet: slicoss: add slicoss gigabit ethernet driver

[Andrew Lunn]

> The link state is retrieved by a command to the application processor that is 
> running 
> on the network card. Also the register to set the phy configuration is 
> write-only, so
> it is not even possible to do the usual mdio bit-banging in the Phy read() 
> and write()
> functions (however there seems to be another application processor command 
> reserved 
> for retrieving the PHY settings, but I have not tried it yet). 

>> +val = MII_BMCR << 16 | SLIC_PCR_AUTONEG |
>> + SLIC_PCR_AUTONEG_RST;
>> +slic_write(sdev, SLIC_REG_WPHY, val);

This actually looks a lot like an MDIO write operation. The upper 16
bits are the register, and the lower 16 bits are the data. What you
don't have is the address. But maybe it is limited to one address.

If the processor command reserved for read works in a similar way, you
have enough to do an MDIO bus.

> Please also note that I do not have any datasheets or other documentation for 
> the hardware, 
> all I have as a reference is the driver code in staging. So I do not know 
> which 
> PHYs are actually used (the comments in the code mention Marvell and Cicada 
> but this is
> not very specific).

If you can get the read working look at registers 2 and 3. Compare
what you get with the values at the end of marvell.c.

 Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [net-next 1/2] net: ethernet: slicoss: add slicoss gigabit ethernet driver

[Andrew Lunn]

> +static const char slic_stats_strings[][ETH_GSTRING_LEN] = {
> + "rx_packets ",
> + "rx_bytes   ",
> + "rx_multicasts  ",
> + "rx_errors  ",
> + "rx_buff_miss   ",
> + "rx_tp_csum ",
> + "rx_tp_oflow",
> + "rx_tp_hlen ",
> + "rx_ip_csum ",
> + "rx_ip_len  ",

Are there any other drivers which pad the statistics strings?

> +static void slic_set_link_autoneg(struct slic_device *sdev)
> +{
> + unsigned int subid = sdev->pdev->subsystem_device;
> + u32 val;
> +
> + if (sdev->is_fiber) {
> + /* We've got a fiber gigabit interface, and register 4 is
> +  * different in fiber mode than in copper mode.
> +  */
> + /* advertise FD only @1000 Mb */
> + val = MII_ADVERTISE << 16 | SLIC_PAR_ADV1000XFD |
> +   SLIC_PAR_ASYMPAUSE_FIBER;
> + /* enable PAUSE frames */
> + slic_write(sdev, SLIC_REG_WPHY, val);
> + /* reset phy, enable auto-neg  */
> + val = MII_BMCR << 16 | SLIC_PCR_RESET | SLIC_PCR_AUTONEG |
> +   SLIC_PCR_AUTONEG_RST;
> + slic_write(sdev, SLIC_REG_WPHY, val);
> + } else {/* copper gigabit */
> + /* We've got a copper gigabit interface, and register 4 is
> +  * different in copper mode than in fiber mode.
> +  */
> + /* advertise 10/100 Mb modes   */
> + val = MII_ADVERTISE << 16 | SLIC_PAR_ADV100FD |
> +   SLIC_PAR_ADV100HD | SLIC_PAR_ADV10FD | SLIC_PAR_ADV10HD;
> + /* enable PAUSE frames  */
> + val |= SLIC_PAR_ASYMPAUSE;
> + /* required by the Cicada PHY  */
> + val |= SLIC_PAR_802_3;
> + slic_write(sdev, SLIC_REG_WPHY, val);
> +
> + /* advertise FD only @1000 Mb  */
> + val = MII_CTRL1000 << 16 | SLIC_PGC_ADV1000FD;
> + slic_write(sdev, SLIC_REG_WPHY, val);
> +
> + if (subid != PCI_SUBDEVICE_ID_ALACRITECH_CICADA) {
> +  /* if a Marvell PHY enable auto crossover */
> + val = SLIC_MIICR_REG_16 | SLIC_MRV_REG16_XOVERON;
> + slic_write(sdev, SLIC_REG_WPHY, val);
> +
> + /* reset phy, enable auto-neg  */
> + val = MII_BMCR << 16 | SLIC_PCR_RESET |
> +   SLIC_PCR_AUTONEG | SLIC_PCR_AUTONEG_RST;
> + slic_write(sdev, SLIC_REG_WPHY, val);
> + } else {
> + /* enable and restart auto-neg (don't reset)  */
> + val = MII_BMCR << 16 | SLIC_PCR_AUTONEG |
> +   SLIC_PCR_AUTONEG_RST;
> + slic_write(sdev, SLIC_REG_WPHY, val);
> + }
> + }
> + sdev->autoneg = true;
> +}

Could this be pulled out into a standard PHY driver? All the SLIC
SLIC_PCR_ defines seems to be the same as those in mii.h. This could
be a standard PHY hidden behind a single register.

   Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 4/4] net: phy: leds: add support for led triggers on phy link state change

[Andrew Lunn]

On Mon, Oct 17, 2016 at 10:49:55AM -0500, Zach Brown wrote:
> Create an option CONFIG_LED_TRIGGER_PHY (default n), which will create a
> set of led triggers for each instantiated PHY device. There is one LED
> trigger per link-speed, per-phy.
> The triggers are registered during phy_attach and unregistered during
> phy_detach.
> 
> This allows for a user to configure their system to allow a set of LEDs
> not controlled by the phy to represent link state changes on the phy.
> LEDS controlled by the phy are unaffected.
> 
> For example, we have a board where some of the leds in the
> RJ45 socket are controlled by the phy, but others are not. Using the
> triggers provided by this patch the leds not controlled by the phy can
> be configured to show the current speed of the ethernet connection. The
> leds controlled by the phy are unaffected.

Is there a clear path how we generalise this, so it could also be used
to control PHY LEDs?

The idea i had a while ago was that the PHY LEDS would also have a
list of triggers which mapped to what the PHY controller could do. So
to enable the PHY LED to show RxTx activity, you would enable the RxTx
trigger on the PHY LED.

This needs an extension to the PHY code, in that these triggers are
specific to the LED, not general across all LEDs. But this is not too
big an issue.

We could end up that if a PHY LED can be used as a generic LED, your
'manual' trigger could be used on it. But it could also support the
PHY driven 'automatic' link status indication trigger. Do we want two
triggers for the same or similar information?

 Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 3/3] net: phy: leds: add support for led triggers on phy link state change

[Andrew Lunn]

> Do you have suggestions on how to better handle the choice of the array size
> and the speeds?

phydev->supported lists the speeds this phy supports.

Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC v3 3/3] phy,leds: add support for led triggers on phy link state change

[Andrew Lunn]

> Andrew, are you happy with this implementation?

Sorry, been to busy with other things to follow the discussion.

What would be nice to see is a comment about how the link to LEDs in
the PHYs is made. Often there is a couple of LEDs in the RJ45 socket
driven by the PHY. They can show link, speed, packet Rx/Tx, etc. How
are these triggers related to these LEDs?

Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 21/44] power/reset: gpio-poweroff: Register with kernel poweroff handler

[Andrew Lunn]

On Mon, Oct 06, 2014 at 10:28:23PM -0700, Guenter Roeck wrote:
 Register with kernel poweroff handler instead of setting pm_power_off
 directly. Register with a low priority value of 64 to reflect that
 the original code only sets pm_power_off if it was not already set.
 
 Other changes:
 
 Drop note that there can not be an additional instance of this driver.
 The original reason no longer applies, it should be obvious that there
 can only be one instance of the driver if static variables are used to
 reflect its state, and support for multiple instances can now be added
 easily if needed by avoiding static variables.
 
 Do not create an error message if another poweroff handler has already been
 registered. This is perfectly normal and acceptable.
 
 Do not display a warning traceback if the poweroff handler fails to
 power off the system. There may be other poweroff handlers.

I would prefer to keep the warning traceback. We found on some
hardware the GPIO transitions were too fast and it failed to power
off. Seeing the traceback gives an idea where to go look for the
problem.

Other than that,

Acked-by: Andrew Lunn and...@lunn.ch

 
 Cc: Sebastian Reichel s...@kernel.org
 Cc: Dmitry Eremin-Solenikov dbarysh...@gmail.com
 Cc: David Woodhouse dw...@infradead.org
 Signed-off-by: Guenter Roeck li...@roeck-us.net
 ---
  drivers/power/reset/gpio-poweroff.c | 36 ++--
  1 file changed, 18 insertions(+), 18 deletions(-)
 
 diff --git a/drivers/power/reset/gpio-poweroff.c 
 b/drivers/power/reset/gpio-poweroff.c
 index ce849bc..e95a7a1 100644
 --- a/drivers/power/reset/gpio-poweroff.c
 +++ b/drivers/power/reset/gpio-poweroff.c
 @@ -14,18 +14,18 @@
  #include linux/kernel.h
  #include linux/init.h
  #include linux/delay.h
 +#include linux/notifier.h
 +#include linux/pm.h
  #include linux/platform_device.h
  #include linux/gpio/consumer.h
  #include linux/of_platform.h
  #include linux/module.h
  
 -/*
 - * Hold configuration here, cannot be more than one instance of the driver
 - * since pm_power_off itself is global.
 - */
  static struct gpio_desc *reset_gpio;
  
 -static void gpio_poweroff_do_poweroff(void)
 +static int gpio_poweroff_do_poweroff(struct notifier_block *this,
 +  unsigned long unused1, void *unused2)
 +
  {
   BUG_ON(!reset_gpio);
  
 @@ -42,20 +42,18 @@ static void gpio_poweroff_do_poweroff(void)
   /* give it some time */
   mdelay(3000);
  
 - WARN_ON(1);
 + return NOTIFY_DONE;
  }
  
 +static struct notifier_block gpio_poweroff_nb = {
 + .notifier_call = gpio_poweroff_do_poweroff,
 + .priority = 64,
 +};
 +
  static int gpio_poweroff_probe(struct platform_device *pdev)
  {
   bool input = false;
 -
 - /* If a pm_power_off function has already been added, leave it alone */
 - if (pm_power_off != NULL) {
 - dev_err(pdev-dev,
 - %s: pm_power_off function already registered,
 -__func__);
 - return -EBUSY;
 - }
 + int err;
  
   reset_gpio = devm_gpiod_get(pdev-dev, NULL);
   if (IS_ERR(reset_gpio))
 @@ -77,14 +75,16 @@ static int gpio_poweroff_probe(struct platform_device 
 *pdev)
   }
   }
  
 - pm_power_off = gpio_poweroff_do_poweroff;
 - return 0;
 + err = register_poweroff_handler(gpio_poweroff_nb);
 + if (err)
 + dev_err(pdev-dev, Failed to register poweroff handler\n);
 +
 + return err;
  }
  
  static int gpio_poweroff_remove(struct platform_device *pdev)
  {
 - if (pm_power_off == gpio_poweroff_do_poweroff)
 - pm_power_off = NULL;
 + unregister_poweroff_handler(gpio_poweroff_nb);
  
   return 0;
  }
 -- 
 1.9.1
 
 
 ___
 linux-arm-kernel mailing list
 linux-arm-ker...@lists.infradead.org
 http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 23/44] power/reset: qnap-poweroff: Register with kernel poweroff handler

[Andrew Lunn]

On Mon, Oct 06, 2014 at 10:28:25PM -0700, Guenter Roeck wrote:
 Register with kernel poweroff handler instead of setting pm_power_off
 directly. Register with default priority value of 128 to reflect that
 the original code generates an error if another poweroff handler has
 already been registered when the driver is loaded.
 
 Cc: Sebastian Reichel s...@kernel.org
 Cc: Dmitry Eremin-Solenikov dbarysh...@gmail.com
 Cc: David Woodhouse dw...@infradead.org
 Signed-off-by: Guenter Roeck li...@roeck-us.net

Acked-by: Andrew Lunn and...@lunn.ch

Thanks
Andrew

 ---
  drivers/power/reset/qnap-poweroff.c | 28 ++--
  1 file changed, 14 insertions(+), 14 deletions(-)
 
 diff --git a/drivers/power/reset/qnap-poweroff.c 
 b/drivers/power/reset/qnap-poweroff.c
 index a75db7f..c474980 100644
 --- a/drivers/power/reset/qnap-poweroff.c
 +++ b/drivers/power/reset/qnap-poweroff.c
 @@ -16,7 +16,9 @@
  
  #include linux/kernel.h
  #include linux/module.h
 +#include linux/notifier.h
  #include linux/platform_device.h
 +#include linux/pm.h
  #include linux/serial_reg.h
  #include linux/kallsyms.h
  #include linux/of.h
 @@ -55,7 +57,8 @@ static void __iomem *base;
  static unsigned long tclk;
  static const struct power_off_cfg *cfg;
  
 -static void qnap_power_off(void)
 +static int qnap_power_off(struct notifier_block *this, unsigned long unused1,
 +   void *unused2)
  {
   const unsigned divisor = ((tclk + (8 * cfg-baud)) / (16 * cfg-baud));
  
 @@ -72,14 +75,20 @@ static void qnap_power_off(void)
  
   /* send the power-off command to PIC */
   writel(cfg-cmd, UART1_REG(TX));
 +
 + return NOTIFY_DONE;
  }
  
 +static struct notifier_block qnap_poweroff_nb = {
 + .notifier_call = qnap_power_off,
 + .priority = 128,
 +};
 +
  static int qnap_power_off_probe(struct platform_device *pdev)
  {
   struct device_node *np = pdev-dev.of_node;
   struct resource *res;
   struct clk *clk;
 - char symname[KSYM_NAME_LEN];
  
   const struct of_device_id *match =
   of_match_node(qnap_power_off_of_match_table, np);
 @@ -106,22 +115,13 @@ static int qnap_power_off_probe(struct platform_device 
 *pdev)
  
   tclk = clk_get_rate(clk);
  
 - /* Check that nothing else has already setup a handler */
 - if (pm_power_off) {
 - lookup_symbol_name((ulong)pm_power_off, symname);
 - dev_err(pdev-dev,
 - pm_power_off already claimed %p %s,
 - pm_power_off, symname);
 - return -EBUSY;
 - }
 - pm_power_off = qnap_power_off;
 -
 - return 0;
 + return register_poweroff_handler(qnap_poweroff_nb);
  }
  
  static int qnap_power_off_remove(struct platform_device *pdev)
  {
 - pm_power_off = NULL;
 + unregister_poweroff_handler(qnap_poweroff_nb);
 +
   return 0;
  }
  
 -- 
 1.9.1
 
 
 ___
 linux-arm-kernel mailing list
 linux-arm-ker...@lists.infradead.org
 http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 20/44] power/reset: restart-poweroff: Register with kernel poweroff handler

[Andrew Lunn]

On Mon, Oct 06, 2014 at 10:28:22PM -0700, Guenter Roeck wrote:
 Register with kernel poweroff handler instead of seting pm_power_off
 directly.  Register as poweroff handler of last resort since the driver
 does not really power off the system but executes a restart.

I would not say last resort, this is how it is designed to work. There
is no way to turn the power off from with linux, it is designed that
u-boot will put the hardware into minimal power consumption until the
power button is pressed.

Other than that, 

Acked-by: Andrew Lunn and...@lunn.ch

Thanks
Andrew

 
 Cc: Sebastian Reichel s...@kernel.org
 Cc: Dmitry Eremin-Solenikov dbarysh...@gmail.com
 Cc: David Woodhouse dw...@infradead.org
 Signed-off-by: Guenter Roeck li...@roeck-us.net
 ---
  drivers/power/reset/restart-poweroff.c | 25 -
  1 file changed, 12 insertions(+), 13 deletions(-)
 
 diff --git a/drivers/power/reset/restart-poweroff.c 
 b/drivers/power/reset/restart-poweroff.c
 index edd707e..5437697 100644
 --- a/drivers/power/reset/restart-poweroff.c
 +++ b/drivers/power/reset/restart-poweroff.c
 @@ -12,35 +12,34 @@
   */
  #include linux/kernel.h
  #include linux/init.h
 +#include linux/notifier.h
  #include linux/platform_device.h
  #include linux/of_platform.h
  #include linux/module.h
 +#include linux/pm.h
  #include linux/reboot.h
 -#include asm/system_misc.h
  
 -static void restart_poweroff_do_poweroff(void)
 +static int restart_poweroff_do_poweroff(struct notifier_block *this,
 + unsigned long unused1, void *unused2)
  {
   reboot_mode = REBOOT_HARD;
   machine_restart(NULL);
 +
 + return NOTIFY_DONE;
  }
  
 +static struct notifier_block restart_poweroff_handler = {
 + .notifier_call = restart_poweroff_do_poweroff,
 +};
 +
  static int restart_poweroff_probe(struct platform_device *pdev)
  {
 - /* If a pm_power_off function has already been added, leave it alone */
 - if (pm_power_off != NULL) {
 - dev_err(pdev-dev,
 - pm_power_off function already registered);
 - return -EBUSY;
 - }
 -
 - pm_power_off = restart_poweroff_do_poweroff;
 - return 0;
 + return register_poweroff_handler(restart_poweroff_handler);
  }
  
  static int restart_poweroff_remove(struct platform_device *pdev)
  {
 - if (pm_power_off == restart_poweroff_do_poweroff)
 - pm_power_off = NULL;
 + unregister_poweroff_handler(restart_poweroff_handler);
  
   return 0;
  }
 -- 
 1.9.1
 
 
 ___
 linux-arm-kernel mailing list
 linux-arm-ker...@lists.infradead.org
 http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 06/44] gpio-poweroff: Drop reference to pm_power_off from devicetree bindings

[Andrew Lunn]

On Mon, Oct 06, 2014 at 10:28:08PM -0700, Guenter Roeck wrote:
 pm_power_off is an implementation detail. Replace it with a more generic
 description of the driver's functionality.
 
 Cc: Rob Herring robh...@kernel.org
 Cc: Pawel Moll pawel.m...@arm.com
 Cc: Mark Rutland mark.rutl...@arm.com
 Signed-off-by: Guenter Roeck li...@roeck-us.net

Acked-by: Andrew Lunn and...@lunn.ch

Thanks
Andrew
 ---
  Documentation/devicetree/bindings/gpio/gpio-poweroff.txt | 10 +-
  1 file changed, 5 insertions(+), 5 deletions(-)
 
 diff --git a/Documentation/devicetree/bindings/gpio/gpio-poweroff.txt 
 b/Documentation/devicetree/bindings/gpio/gpio-poweroff.txt
 index d4eab92..c95a1a6 100644
 --- a/Documentation/devicetree/bindings/gpio/gpio-poweroff.txt
 +++ b/Documentation/devicetree/bindings/gpio/gpio-poweroff.txt
 @@ -2,12 +2,12 @@ Driver a GPIO line that can be used to turn the power off.
  
  The driver supports both level triggered and edge triggered power off.
  At driver load time, the driver will request the given gpio line and
 -install a pm_power_off handler. If the optional properties 'input' is
 -not found, the GPIO line will be driven in the inactive
 +install a handler to power off the system. If the optional properties
 +'input' is not found, the GPIO line will be driven in the inactive
  state. Otherwise its configured as an input.
  
 -When the pm_power_off is called, the gpio is configured as an output,
 -and drive active, so triggering a level triggered power off
 +When the the poweroff handler is called, the gpio is configured as an
 +output, and drive active, so triggering a level triggered power off
  condition. This will also cause an inactive-active edge condition, so
  triggering positive edge triggered power off. After a delay of 100ms,
  the GPIO is set to inactive, thus causing an active-inactive edge,
 @@ -24,7 +24,7 @@ Required properties:
  
  Optional properties:
  - input : Initially configure the GPIO line as an input. Only reconfigure
 -  it to an output when the pm_power_off function is called. If this optional
 +  it to an output when the poweroff handler is called. If this optional
property is not specified, the GPIO is initialized as an output in its
inactive state.
  
 -- 
 1.9.1
 
 
 ___
 linux-arm-kernel mailing list
 linux-arm-ker...@lists.infradead.org
 http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 07/44] qnap-poweroff: Drop reference to pm_power_off from devicetree bindings

[Andrew Lunn]

On Mon, Oct 06, 2014 at 10:28:09PM -0700, Guenter Roeck wrote:
 Replace reference to pm_power_off (which is an implementation detail)
 and replace it with a more generic description of the driver's functionality.

Acked-by: Andrew Lunn and...@lunn.ch

Thanks
Andrew

 
 Cc: Rob Herring robh...@kernel.org
 Cc: Pawel Moll pawel.m...@arm.com
 Cc: Mark Rutland mark.rutl...@arm.com
 Signed-off-by: Guenter Roeck li...@roeck-us.net
 ---
  Documentation/devicetree/bindings/power_supply/qnap-poweroff.txt | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
 
 diff --git a/Documentation/devicetree/bindings/power_supply/qnap-poweroff.txt 
 b/Documentation/devicetree/bindings/power_supply/qnap-poweroff.txt
 index af25e77..1e2260a 100644
 --- a/Documentation/devicetree/bindings/power_supply/qnap-poweroff.txt
 +++ b/Documentation/devicetree/bindings/power_supply/qnap-poweroff.txt
 @@ -3,8 +3,8 @@
  QNAP NAS devices have a microcontroller controlling the main power
  supply. This microcontroller is connected to UART1 of the Kirkwood and
  Orion5x SoCs. Sending the character 'A', at 19200 baud, tells the
 -microcontroller to turn the power off. This driver adds a handler to
 -pm_power_off which is called to turn the power off.
 +microcontroller to turn the power off. This driver installs a handler
 +to power off the system.
  
  Synology NAS devices use a similar scheme, but a different baud rate,
  9600, and a different character, '1'.
 -- 
 1.9.1
 
 
 ___
 linux-arm-kernel mailing list
 linux-arm-ker...@lists.infradead.org
 http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel