[PATCH 00/14] net: ks8851: Unify KS8851 SPI and MLL drivers

[PATCH 00/14] net: ks8851: Unify KS8851 SPI and MLL drivers

[Marek Vasut]

The KS8851SNL/SNLI and KS8851-16MLL/MLLI/MLLU are very much the same pieces
of silicon, except the former has an SPI interface, while the later has a
parallel bus interface. Thus far, Linux has two separate drivers for each
and they are diverging considerably.

This series unifies them into a single driver with small SPI and parallel
bus specific parts. The approach here is to first separate out the SPI
specific parts into a separate file, then add parallel bus accessors in
another separate file and then finally remove the old parallel bus driver.
The reason for replacing the old parallel bus driver is because the SPI
bus driver is much higher quality.

Marek Vasut (14):
  net: ks8851: Factor out spi->dev in probe()/remove()
  net: ks8851: Replace dev_err() with netdev_err() in IRQ handler
  net: ks8851: Pass device pointer into ks8851_init_mac()
  net: ks8851: Use devm_alloc_etherdev()
  net: ks8851: Use dev_{get,set}_drvdata()
  net: ks8851: Remove ks8851_rdreg32()
  net: ks8851: Use 16-bit writes to program MAC address
  net: ks8851: Use 16-bit read of RXFC register
  net: ks8851: Split out SPI specific entries in struct ks8851_net
  net: ks8851: Split out SPI specific code from probe() and remove()
  net: ks8851: Implement register and FIFO accessor callbacks
  net: ks8851: Separate SPI operations into separate file
  net: ks8851: Implement Parallel bus operations
  net: ks8851: Remove ks8851_mll.c

 drivers/net/ethernet/micrel/Makefile     |    4 +-
 drivers/net/ethernet/micrel/ks8851.c     |  487 +-------
 drivers/net/ethernet/micrel/ks8851.h     |  123 ++
 drivers/net/ethernet/micrel/ks8851_mll.c | 1345 ----------------------
 drivers/net/ethernet/micrel/ks8851_par.c |  201 ++++
 drivers/net/ethernet/micrel/ks8851_spi.c |  311 +++++
 6 files changed, 690 insertions(+), 1781 deletions(-)
 delete mode 100644 drivers/net/ethernet/micrel/ks8851_mll.c
 create mode 100644 drivers/net/ethernet/micrel/ks8851_par.c
 create mode 100644 drivers/net/ethernet/micrel/ks8851_spi.c

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: David S. Miller <davem@davemloft.net>
Cc: Lukas Wunner <lukas@wunner.de>
Cc: Petr Stetiar <ynezz@true.cz>
Cc: YueHaibing <yuehaibing@huawei.com>

-- 
2.25.1

[PATCH 01/14] net: ks8851: Factor out spi->dev in probe()/remove()

[Marek Vasut]

Pull out the spi->dev into one common place in the function instead of
having it repeated over and over again. This is done in preparation for
unifying ks8851 and ks8851-mll drivers. No functional change.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: David S. Miller <davem@davemloft.net>
Cc: Lukas Wunner <lukas@wunner.de>
Cc: Petr Stetiar <ynezz@true.cz>
Cc: YueHaibing <yuehaibing@huawei.com>
---
 drivers/net/ethernet/micrel/ks8851.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
index 33305c9c5a62..d1e0116c9728 100644
--- a/drivers/net/ethernet/micrel/ks8851.c
+++ b/drivers/net/ethernet/micrel/ks8851.c
@@ -1413,6 +1413,7 @@ static SIMPLE_DEV_PM_OPS(ks8851_pm_ops, ks8851_suspend, ks8851_resume);
 
 static int ks8851_probe(struct spi_device *spi)
 {
+	struct device *dev = &spi->dev;
 	struct net_device *ndev;
 	struct ks8851_net *ks;
 	int ret;
@@ -1431,7 +1432,7 @@ static int ks8851_probe(struct spi_device *spi)
 	ks->spidev = spi;
 	ks->tx_space = 6144;
 
-	gpio = of_get_named_gpio_flags(spi->dev.of_node, "reset-gpios",
+	gpio = of_get_named_gpio_flags(dev->of_node, "reset-gpios",
 				       0, NULL);
 	if (gpio == -EPROBE_DEFER) {
 		ret = gpio;
@@ -1440,15 +1441,15 @@ static int ks8851_probe(struct spi_device *spi)
 
 	ks->gpio = gpio;
 	if (gpio_is_valid(gpio)) {
-		ret = devm_gpio_request_one(&spi->dev, gpio,
+		ret = devm_gpio_request_one(dev, gpio,
 					    GPIOF_OUT_INIT_LOW, "ks8851_rst_n");
 		if (ret) {
-			dev_err(&spi->dev, "reset gpio request failed\n");
+			dev_err(dev, "reset gpio request failed\n");
 			goto err_gpio;
 		}
 	}
 
-	ks->vdd_io = devm_regulator_get(&spi->dev, "vdd-io");
+	ks->vdd_io = devm_regulator_get(dev, "vdd-io");
 	if (IS_ERR(ks->vdd_io)) {
 		ret = PTR_ERR(ks->vdd_io);
 		goto err_reg_io;
@@ -1456,12 +1457,12 @@ static int ks8851_probe(struct spi_device *spi)
 
 	ret = regulator_enable(ks->vdd_io);
 	if (ret) {
-		dev_err(&spi->dev, "regulator vdd_io enable fail: %d\n",
+		dev_err(dev, "regulator vdd_io enable fail: %d\n",
 			ret);
 		goto err_reg_io;
 	}
 
-	ks->vdd_reg = devm_regulator_get(&spi->dev, "vdd");
+	ks->vdd_reg = devm_regulator_get(dev, "vdd");
 	if (IS_ERR(ks->vdd_reg)) {
 		ret = PTR_ERR(ks->vdd_reg);
 		goto err_reg;
@@ -1469,7 +1470,7 @@ static int ks8851_probe(struct spi_device *spi)
 
 	ret = regulator_enable(ks->vdd_reg);
 	if (ret) {
-		dev_err(&spi->dev, "regulator vdd enable fail: %d\n",
+		dev_err(dev, "regulator vdd enable fail: %d\n",
 			ret);
 		goto err_reg;
 	}
@@ -1509,7 +1510,7 @@ static int ks8851_probe(struct spi_device *spi)
 	ks->mii.mdio_read	= ks8851_phy_read;
 	ks->mii.mdio_write	= ks8851_phy_write;
 
-	dev_info(&spi->dev, "message enable is %d\n", msg_enable);
+	dev_info(dev, "message enable is %d\n", msg_enable);
 
 	/* set the default message enable */
 	ks->msg_enable = netif_msg_init(msg_enable, (NETIF_MSG_DRV |
@@ -1519,7 +1520,7 @@ static int ks8851_probe(struct spi_device *spi)
 	skb_queue_head_init(&ks->txq);
 
 	ndev->ethtool_ops = &ks8851_ethtool_ops;
-	SET_NETDEV_DEV(ndev, &spi->dev);
+	SET_NETDEV_DEV(ndev, dev);
 
 	spi_set_drvdata(spi, ks);
 
@@ -1534,7 +1535,7 @@ static int ks8851_probe(struct spi_device *spi)
 	/* simple check for a valid chip being connected to the bus */
 	cider = ks8851_rdreg16(ks, KS_CIDER);
 	if ((cider & ~CIDER_REV_MASK) != CIDER_ID) {
-		dev_err(&spi->dev, "failed to read device ID\n");
+		dev_err(dev, "failed to read device ID\n");
 		ret = -ENODEV;
 		goto err_id;
 	}
@@ -1547,7 +1548,7 @@ static int ks8851_probe(struct spi_device *spi)
 
 	ret = register_netdev(ndev);
 	if (ret) {
-		dev_err(&spi->dev, "failed to register network device\n");
+		dev_err(dev, "failed to register network device\n");
 		goto err_netdev;
 	}
 
@@ -1573,9 +1574,10 @@ static int ks8851_probe(struct spi_device *spi)
 static int ks8851_remove(struct spi_device *spi)
 {
 	struct ks8851_net *priv = spi_get_drvdata(spi);
+	struct device *dev = &spi->dev;
 
 	if (netif_msg_drv(priv))
-		dev_info(&spi->dev, "remove\n");
+		dev_info(dev, "remove\n");
 
 	unregister_netdev(priv->netdev);
 	if (gpio_is_valid(priv->gpio))
-- 
2.25.1

Re: [PATCH 01/14] net: ks8851: Factor out spi->dev in probe()/remove()

[Andrew Lunn]

On Tue, Mar 24, 2020 at 12:42:50AM +0100, Marek Vasut wrote:
> Pull out the spi->dev into one common place in the function instead of
> having it repeated over and over again. This is done in preparation for
> unifying ks8851 and ks8851-mll drivers. No functional change.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Lukas Wunner <lukas@wunner.de>
> Cc: Petr Stetiar <ynezz@true.cz>
> Cc: YueHaibing <yuehaibing@huawei.com>
> ---
>  drivers/net/ethernet/micrel/ks8851.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
> index 33305c9c5a62..d1e0116c9728 100644
> --- a/drivers/net/ethernet/micrel/ks8851.c
> +++ b/drivers/net/ethernet/micrel/ks8851.c
> @@ -1413,6 +1413,7 @@ static SIMPLE_DEV_PM_OPS(ks8851_pm_ops, ks8851_suspend, ks8851_resume);
>  
>  static int ks8851_probe(struct spi_device *spi)
>  {
> +	struct device *dev = &spi->dev;
>  	struct net_device *ndev;
>  	struct ks8851_net *ks;
>  	int ret;

Hi Marek

The naming in probe appears to be different to the rest of the
driver. Normally dev is a strict net_device. Here it is a struct
device and ndev is a net_device. Sometimes netdev is also used.

It might be a bigger change than what you want to do, but it would be
nice if it was consistent everywhere.

     Andrew

Re: [PATCH 01/14] net: ks8851: Factor out spi->dev in probe()/remove()

[Lukas Wunner]

On Tue, Mar 24, 2020 at 12:42:50AM +0100, Marek Vasut wrote:
> -	gpio = of_get_named_gpio_flags(spi->dev.of_node, "reset-gpios",
> +	gpio = of_get_named_gpio_flags(dev->of_node, "reset-gpios",
>  				       0, NULL);
>  	if (gpio == -EPROBE_DEFER) {
>  		ret = gpio;

Nit: It would be nice to unwrap the line since it is shorter than
80 chars even with "0, NULL);" appended.


> @@ -1456,12 +1457,12 @@ static int ks8851_probe(struct spi_device *spi)
>  
>  	ret = regulator_enable(ks->vdd_io);
>  	if (ret) {
> -		dev_err(&spi->dev, "regulator vdd_io enable fail: %d\n",
> +		dev_err(dev, "regulator vdd_io enable fail: %d\n",
>  			ret);
>  		goto err_reg_io;
>  	}

Same here.


> @@ -1469,7 +1470,7 @@ static int ks8851_probe(struct spi_device *spi)
>  
>  	ret = regulator_enable(ks->vdd_reg);
>  	if (ret) {
> -		dev_err(&spi->dev, "regulator vdd enable fail: %d\n",
> +		dev_err(dev, "regulator vdd enable fail: %d\n",
>  			ret);
>  		goto err_reg;
>  	}

Same.

Thanks,

Lukas

[PATCH 02/14] net: ks8851: Replace dev_err() with netdev_err() in IRQ handler

[Marek Vasut]

Use netdev_err() instead of dev_err() to avoid accessing the spidev->dev
in the interrupt handler. This is the only place which uses the spidev
in this function, so replace it with netdev_err() to get rid of it. This
is done in preparation for unifying the KS8851 SPI and parallel drivers.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: David S. Miller <davem@davemloft.net>
Cc: Lukas Wunner <lukas@wunner.de>
Cc: Petr Stetiar <ynezz@true.cz>
Cc: YueHaibing <yuehaibing@huawei.com>
---
 drivers/net/ethernet/micrel/ks8851.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
index d1e0116c9728..8f4d7c0af723 100644
--- a/drivers/net/ethernet/micrel/ks8851.c
+++ b/drivers/net/ethernet/micrel/ks8851.c
@@ -631,7 +631,7 @@ static irqreturn_t ks8851_irq(int irq, void *_ks)
 		handled |= IRQ_RXI;
 
 	if (status & IRQ_SPIBEI) {
-		dev_err(&ks->spidev->dev, "%s: spi bus error\n", __func__);
+		netdev_err(ks->netdev, "%s: spi bus error\n", __func__);
 		handled |= IRQ_SPIBEI;
 	}
 
-- 
2.25.1

Re: [PATCH 02/14] net: ks8851: Replace dev_err() with netdev_err() in IRQ handler

[Andrew Lunn]

On Tue, Mar 24, 2020 at 12:42:51AM +0100, Marek Vasut wrote:
> Use netdev_err() instead of dev_err() to avoid accessing the spidev->dev
> in the interrupt handler. This is the only place which uses the spidev
> in this function, so replace it with netdev_err() to get rid of it. This
> is done in preparation for unifying the KS8851 SPI and parallel drivers.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Lukas Wunner <lukas@wunner.de>
> Cc: Petr Stetiar <ynezz@true.cz>
> Cc: YueHaibing <yuehaibing@huawei.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH 03/14] net: ks8851: Pass device pointer into ks8851_init_mac()

[Marek Vasut]

Since the driver probe function already has a struct device *dev pointer,
pass it as a parameter to ks8851_init_mac() to avoid fishing it out via
ks->spidev. This is the only reference to spidev in the function, so get
rid of it. This is done in preparation for unifying the KS8851 SPI and
parallel drivers.

No functional change.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: David S. Miller <davem@davemloft.net>
Cc: Lukas Wunner <lukas@wunner.de>
Cc: Petr Stetiar <ynezz@true.cz>
Cc: YueHaibing <yuehaibing@huawei.com>
---
 drivers/net/ethernet/micrel/ks8851.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
index 8f4d7c0af723..601a74d750b2 100644
--- a/drivers/net/ethernet/micrel/ks8851.c
+++ b/drivers/net/ethernet/micrel/ks8851.c
@@ -409,6 +409,7 @@ static void ks8851_read_mac_addr(struct net_device *dev)
 /**
  * ks8851_init_mac - initialise the mac address
  * @ks: The device structure
+ * @ddev: The device structure pointer
  *
  * Get or create the initial mac address for the device and then set that
  * into the station address register. A mac address supplied in the device
@@ -416,12 +417,12 @@ static void ks8851_read_mac_addr(struct net_device *dev)
  * we try that. If no valid mac address is found we use eth_random_addr()
  * to create a new one.
  */
-static void ks8851_init_mac(struct ks8851_net *ks)
+static void ks8851_init_mac(struct ks8851_net *ks, struct device *ddev)
 {
 	struct net_device *dev = ks->netdev;
 	const u8 *mac_addr;
 
-	mac_addr = of_get_mac_address(ks->spidev->dev.of_node);
+	mac_addr = of_get_mac_address(ddev->of_node);
 	if (!IS_ERR(mac_addr)) {
 		ether_addr_copy(dev->dev_addr, mac_addr);
 		ks8851_write_mac_addr(dev);
@@ -1544,7 +1545,7 @@ static int ks8851_probe(struct spi_device *spi)
 	ks->rc_ccr = ks8851_rdreg16(ks, KS_CCR);
 
 	ks8851_read_selftest(ks);
-	ks8851_init_mac(ks);
+	ks8851_init_mac(ks, dev);
 
 	ret = register_netdev(ndev);
 	if (ret) {
-- 
2.25.1

Re: [PATCH 03/14] net: ks8851: Pass device pointer into ks8851_init_mac()

[Andrew Lunn]

On Tue, Mar 24, 2020 at 12:42:52AM +0100, Marek Vasut wrote:
> Since the driver probe function already has a struct device *dev pointer,
> pass it as a parameter to ks8851_init_mac() to avoid fishing it out via
> ks->spidev. This is the only reference to spidev in the function, so get
> rid of it. This is done in preparation for unifying the KS8851 SPI and
> parallel drivers.
> 
> No functional change.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Lukas Wunner <lukas@wunner.de>
> Cc: Petr Stetiar <ynezz@true.cz>
> Cc: YueHaibing <yuehaibing@huawei.com>
> ---
>  drivers/net/ethernet/micrel/ks8851.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
> index 8f4d7c0af723..601a74d750b2 100644
> --- a/drivers/net/ethernet/micrel/ks8851.c
> +++ b/drivers/net/ethernet/micrel/ks8851.c
> @@ -409,6 +409,7 @@ static void ks8851_read_mac_addr(struct net_device *dev)
>  /**
>   * ks8851_init_mac - initialise the mac address
>   * @ks: The device structure
> + * @ddev: The device structure pointer
>   *
>   * Get or create the initial mac address for the device and then set that
>   * into the station address register. A mac address supplied in the device
> @@ -416,12 +417,12 @@ static void ks8851_read_mac_addr(struct net_device *dev)
>   * we try that. If no valid mac address is found we use eth_random_addr()
>   * to create a new one.
>   */
> -static void ks8851_init_mac(struct ks8851_net *ks)
> +static void ks8851_init_mac(struct ks8851_net *ks, struct device *ddev)
>  {
>  	struct net_device *dev = ks->netdev;
>  	const u8 *mac_addr;
>  
> -	mac_addr = of_get_mac_address(ks->spidev->dev.of_node);
> +	mac_addr = of_get_mac_address(ddev->of_node);


Hi Marek

The name ddev is a bit odd. Looking at the code, i see why. dev is
normally a struct net_device, which this function already has.

You could avoid this oddness by directly passing of_node.

    Andrew

Re: [PATCH 03/14] net: ks8851: Pass device pointer into ks8851_init_mac()

[Lukas Wunner]

On Tue, Mar 24, 2020 at 02:06:22AM +0100, Andrew Lunn wrote:
> On Tue, Mar 24, 2020 at 12:42:52AM +0100, Marek Vasut wrote:
> > Since the driver probe function already has a struct device *dev pointer,
> > pass it as a parameter to ks8851_init_mac() to avoid fishing it out via
> > ks->spidev. This is the only reference to spidev in the function, so get
> > rid of it. This is done in preparation for unifying the KS8851 SPI and
> > parallel drivers.
[...]
> > -static void ks8851_init_mac(struct ks8851_net *ks)
> > +static void ks8851_init_mac(struct ks8851_net *ks, struct device *ddev)
> >  {
> >  	struct net_device *dev = ks->netdev;
> >  	const u8 *mac_addr;
> >  
> > -	mac_addr = of_get_mac_address(ks->spidev->dev.of_node);
> > +	mac_addr = of_get_mac_address(ddev->of_node);
> 
> The name ddev is a bit odd. Looking at the code, i see why. dev is
> normally a struct net_device, which this function already has.
> 
> You could avoid this oddness by directly passing of_node.

Actually after adding the invocation of of_get_mac_address() with
commit 566bd54b067d ("net: ks8851: Support DT-provided MAC address")
I've had regrets that I should have used device_get_mac_address()
instead since it's platform-agnostic, hence would work with ACPI
as well as DT-based systems.

device_get_mac_address() needs a struct device, so I'd prefer
using that instead of passing an of_node.

I agree that "ddev" is somewhat odd.  Some drivers name it "device"
or "pdev" (which however collides with the naming of platform_devices).
Another idea would be to move the handy ndev_to_dev() static inline
from apm/xgene/xgene_enet_main.h to include/linux/netdevice.h and
use that with "struct net_device *dev", which we already have in
ks8851_init_mac().

Thanks,

Lukas

[PATCH 04/14] net: ks8851: Use devm_alloc_etherdev()

[Marek Vasut]

Use device managed version of alloc_etherdev() to simplify the code.
No functional change intended.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: David S. Miller <davem@davemloft.net>
Cc: Lukas Wunner <lukas@wunner.de>
Cc: Petr Stetiar <ynezz@true.cz>
Cc: YueHaibing <yuehaibing@huawei.com>
---
 drivers/net/ethernet/micrel/ks8851.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
index 601a74d750b2..cc1137be3d8f 100644
--- a/drivers/net/ethernet/micrel/ks8851.c
+++ b/drivers/net/ethernet/micrel/ks8851.c
@@ -1421,7 +1421,7 @@ static int ks8851_probe(struct spi_device *spi)
 	unsigned cider;
 	int gpio;
 
-	ndev = alloc_etherdev(sizeof(struct ks8851_net));
+	ndev = devm_alloc_etherdev(dev, sizeof(struct ks8851_net));
 	if (!ndev)
 		return -ENOMEM;
 
@@ -1435,10 +1435,8 @@ static int ks8851_probe(struct spi_device *spi)
 
 	gpio = of_get_named_gpio_flags(dev->of_node, "reset-gpios",
 				       0, NULL);
-	if (gpio == -EPROBE_DEFER) {
-		ret = gpio;
-		goto err_gpio;
-	}
+	if (gpio == -EPROBE_DEFER)
+		return gpio;
 
 	ks->gpio = gpio;
 	if (gpio_is_valid(gpio)) {
@@ -1446,7 +1444,7 @@ static int ks8851_probe(struct spi_device *spi)
 					    GPIOF_OUT_INIT_LOW, "ks8851_rst_n");
 		if (ret) {
 			dev_err(dev, "reset gpio request failed\n");
-			goto err_gpio;
+			return ret;
 		}
 	}
 
@@ -1567,8 +1565,6 @@ static int ks8851_probe(struct spi_device *spi)
 err_reg:
 	regulator_disable(ks->vdd_io);
 err_reg_io:
-err_gpio:
-	free_netdev(ndev);
 	return ret;
 }
 
@@ -1585,7 +1581,6 @@ static int ks8851_remove(struct spi_device *spi)
 		gpio_set_value(priv->gpio, 0);
 	regulator_disable(priv->vdd_reg);
 	regulator_disable(priv->vdd_io);
-	free_netdev(priv->netdev);
 
 	return 0;
 }
-- 
2.25.1

Re: [PATCH 04/14] net: ks8851: Use devm_alloc_etherdev()

[Andrew Lunn]

On Tue, Mar 24, 2020 at 12:42:53AM +0100, Marek Vasut wrote:
> Use device managed version of alloc_etherdev() to simplify the code.
> No functional change intended.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Lukas Wunner <lukas@wunner.de>
> Cc: Petr Stetiar <ynezz@true.cz>
> Cc: YueHaibing <yuehaibing@huawei.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH 05/14] net: ks8851: Use dev_{get,set}_drvdata()

[Marek Vasut]

Replace spi_{get,set}_drvdata() with dev_{get,set}_drvdata(), which
works for both SPI and platform drivers. This is done in preparation
for unifying the KS8851 SPI and parallel bus drivers.

There should be no functional change.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: David S. Miller <davem@davemloft.net>
Cc: Lukas Wunner <lukas@wunner.de>
Cc: Petr Stetiar <ynezz@true.cz>
Cc: YueHaibing <yuehaibing@huawei.com>
---
 drivers/net/ethernet/micrel/ks8851.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
index cc1137be3d8f..1c0a0364b047 100644
--- a/drivers/net/ethernet/micrel/ks8851.c
+++ b/drivers/net/ethernet/micrel/ks8851.c
@@ -1521,7 +1521,7 @@ static int ks8851_probe(struct spi_device *spi)
 	ndev->ethtool_ops = &ks8851_ethtool_ops;
 	SET_NETDEV_DEV(ndev, dev);
 
-	spi_set_drvdata(spi, ks);
+	dev_set_drvdata(dev, ks);
 
 	netif_carrier_off(ks->netdev);
 	ndev->if_port = IF_PORT_100BASET;
@@ -1570,8 +1570,8 @@ static int ks8851_probe(struct spi_device *spi)
 
 static int ks8851_remove(struct spi_device *spi)
 {
-	struct ks8851_net *priv = spi_get_drvdata(spi);
 	struct device *dev = &spi->dev;
+	struct ks8851_net *priv = dev_get_drvdata(dev);
 
 	if (netif_msg_drv(priv))
 		dev_info(dev, "remove\n");
-- 
2.25.1

Re: [PATCH 05/14] net: ks8851: Use dev_{get,set}_drvdata()

[Andrew Lunn]

On Tue, Mar 24, 2020 at 12:42:54AM +0100, Marek Vasut wrote:
> Replace spi_{get,set}_drvdata() with dev_{get,set}_drvdata(), which
> works for both SPI and platform drivers. This is done in preparation
> for unifying the KS8851 SPI and parallel bus drivers.
> 
> There should be no functional change.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Lukas Wunner <lukas@wunner.de>
> Cc: Petr Stetiar <ynezz@true.cz>
> Cc: YueHaibing <yuehaibing@huawei.com>
> ---
>  drivers/net/ethernet/micrel/ks8851.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
> index cc1137be3d8f..1c0a0364b047 100644
> --- a/drivers/net/ethernet/micrel/ks8851.c
> +++ b/drivers/net/ethernet/micrel/ks8851.c
> @@ -1521,7 +1521,7 @@ static int ks8851_probe(struct spi_device *spi)
>  	ndev->ethtool_ops = &ks8851_ethtool_ops;
>  	SET_NETDEV_DEV(ndev, dev);
>  
> -	spi_set_drvdata(spi, ks);
> +	dev_set_drvdata(dev, ks);
>  
>  	netif_carrier_off(ks->netdev);
>  	ndev->if_port = IF_PORT_100BASET;
> @@ -1570,8 +1570,8 @@ static int ks8851_probe(struct spi_device *spi)
>  
>  static int ks8851_remove(struct spi_device *spi)
>  {
> -	struct ks8851_net *priv = spi_get_drvdata(spi);
>  	struct device *dev = &spi->dev;
> +	struct ks8851_net *priv = dev_get_drvdata(dev);

Reverse Christmas tree. You need to split this.

	Andrew

[PATCH 06/14] net: ks8851: Remove ks8851_rdreg32()

[Marek Vasut]

The ks8851_rdreg32() is used only in one place, to read two registers
using a single read. To make it easier to support 16-bit accesses via
parallel bus later on, replace this single read with two 16-bit reads
from each of the registers and drop the ks8851_rdreg32() altogether.

If this has noticeable performance impact on the SPI variant of KS8851,
then we should consider using regmap to abstract the SPI and parallel
bus options and in case of SPI, permit regmap to merge register reads
of neighboring registers into single, longer, read.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: David S. Miller <davem@davemloft.net>
Cc: Lukas Wunner <lukas@wunner.de>
Cc: Petr Stetiar <ynezz@true.cz>
Cc: YueHaibing <yuehaibing@huawei.com>
---
 drivers/net/ethernet/micrel/ks8851.c | 25 ++-----------------------
 1 file changed, 2 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
index 1c0a0364b047..be9478f39009 100644
--- a/drivers/net/ethernet/micrel/ks8851.c
+++ b/drivers/net/ethernet/micrel/ks8851.c
@@ -296,25 +296,6 @@ static unsigned ks8851_rdreg16(struct ks8851_net *ks, unsigned reg)
 	return le16_to_cpu(rx);
 }
 
-/**
- * ks8851_rdreg32 - read 32 bit register from device
- * @ks: The chip information
- * @reg: The register address
- *
- * Read a 32bit register from the chip.
- *
- * Note, this read requires the address be aligned to 4 bytes.
-*/
-static unsigned ks8851_rdreg32(struct ks8851_net *ks, unsigned reg)
-{
-	__le32 rx = 0;
-
-	WARN_ON(reg & 3);
-
-	ks8851_rdreg(ks, MK_OP(0xf, reg), (u8 *)&rx, 4);
-	return le32_to_cpu(rx);
-}
-
 /**
  * ks8851_soft_reset - issue one of the soft reset to the device
  * @ks: The device state.
@@ -508,7 +489,6 @@ static void ks8851_rx_pkts(struct ks8851_net *ks)
 	unsigned rxfc;
 	unsigned rxlen;
 	unsigned rxstat;
-	u32 rxh;
 	u8 *rxpkt;
 
 	rxfc = ks8851_rdreg8(ks, KS_RXFC);
@@ -527,9 +507,8 @@ static void ks8851_rx_pkts(struct ks8851_net *ks)
 	 */
 
 	for (; rxfc != 0; rxfc--) {
-		rxh = ks8851_rdreg32(ks, KS_RXFHSR);
-		rxstat = rxh & 0xffff;
-		rxlen = (rxh >> 16) & 0xfff;
+		rxstat = ks8851_rdreg16(ks, KS_RXFHSR);
+		rxlen = ks8851_rdreg16(ks, KS_RXFHBCR) & RXFHBCR_CNT_MASK;
 
 		netif_dbg(ks, rx_status, ks->netdev,
 			  "rx: stat 0x%04x, len 0x%04x\n", rxstat, rxlen);
-- 
2.25.1

Re: [PATCH 06/14] net: ks8851: Remove ks8851_rdreg32()

[Andrew Lunn]

> @@ -527,9 +507,8 @@ static void ks8851_rx_pkts(struct ks8851_net *ks)
>  	 */
>  
>  	for (; rxfc != 0; rxfc--) {
> -		rxh = ks8851_rdreg32(ks, KS_RXFHSR);
> -		rxstat = rxh & 0xffff;
> -		rxlen = (rxh >> 16) & 0xfff;
> +		rxstat = ks8851_rdreg16(ks, KS_RXFHSR);
> +		rxlen = ks8851_rdreg16(ks, KS_RXFHBCR) & RXFHBCR_CNT_MASK;

Hi Marek

Is there anything in the datasheet about these registers? Does reading
them clear an interrupt etc? A 32bit read is i assume one SPI
transaction, where as this is now two transactions, so no longer
atomic.

	Andrew

Re: [PATCH 06/14] net: ks8851: Remove ks8851_rdreg32()

[Marek Vasut]

On 3/24/20 2:30 AM, Andrew Lunn wrote:
>> @@ -527,9 +507,8 @@ static void ks8851_rx_pkts(struct ks8851_net *ks)
>>  	 */
>>  
>>  	for (; rxfc != 0; rxfc--) {
>> -		rxh = ks8851_rdreg32(ks, KS_RXFHSR);
>> -		rxstat = rxh & 0xffff;
>> -		rxlen = (rxh >> 16) & 0xfff;
>> +		rxstat = ks8851_rdreg16(ks, KS_RXFHSR);
>> +		rxlen = ks8851_rdreg16(ks, KS_RXFHBCR) & RXFHBCR_CNT_MASK;
> 
> Hi Marek
> 
> Is there anything in the datasheet about these registers? Does reading
> them clear an interrupt etc? A 32bit read is i assume one SPI
> transaction, where as this is now two transactions, so no longer
> atomic.

Nope, they're just two registers holding packet metadata.
There are separate interrupt registers and separate register to clear
the packet from the RX FIFO, so reading these two registers does not
have to be atomic.

Re: [PATCH 06/14] net: ks8851: Remove ks8851_rdreg32()

[Lukas Wunner]

On Tue, Mar 24, 2020 at 12:42:55AM +0100, Marek Vasut wrote:
> The ks8851_rdreg32() is used only in one place, to read two registers
> using a single read. To make it easier to support 16-bit accesses via
> parallel bus later on, replace this single read with two 16-bit reads
> from each of the registers and drop the ks8851_rdreg32() altogether.

This doubles the SPI transactions necessary to read the RX queue status,
which happens for each received packet, so I expect the performance
impact to be noticeable.  Can you keep the 32-bit variant for SPI
and instead just introduce a 32-bit read for the MLL chip which performs
two 16-bit reads internally?

Thanks,

Lukas

Re: [PATCH 06/14] net: ks8851: Remove ks8851_rdreg32()

[Marek Vasut]

On 3/24/20 8:22 AM, Lukas Wunner wrote:
> On Tue, Mar 24, 2020 at 12:42:55AM +0100, Marek Vasut wrote:
>> The ks8851_rdreg32() is used only in one place, to read two registers
>> using a single read. To make it easier to support 16-bit accesses via
>> parallel bus later on, replace this single read with two 16-bit reads
>> from each of the registers and drop the ks8851_rdreg32() altogether.
> 
> This doubles the SPI transactions necessary to read the RX queue status,
> which happens for each received packet, so I expect the performance
> impact to be noticeable.  Can you keep the 32-bit variant for SPI
> and instead just introduce a 32-bit read for the MLL chip which performs
> two 16-bit reads internally?

Please test it before I'll be forced to rework the harder half of this
patchset. I don't have the SPI variant of the chip to collect those
statistics.

But the real fix here would be to convert the driver to regmap in the
end and permit regmap to merge neighboring register accesses over SPI.

[PATCH 07/14] net: ks8851: Use 16-bit writes to program MAC address

[Marek Vasut]

On the SPI variant of KS8851, the MAC address can be programmed with
either 8/16/32-bit writes. To make it easier to support the 16-bit
parallel option of KS8851 too, switch both the MAC address programming
and readout to 16-bit operations.

Remove ks8851_wrreg8() as it is not used anywhere anymore.

There should be no functional change.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: David S. Miller <davem@davemloft.net>
Cc: Lukas Wunner <lukas@wunner.de>
Cc: Petr Stetiar <ynezz@true.cz>
Cc: YueHaibing <yuehaibing@huawei.com>
---
 drivers/net/ethernet/micrel/ks8851.c | 47 ++++++++--------------------
 1 file changed, 13 insertions(+), 34 deletions(-)

diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
index be9478f39009..3150f1b928c0 100644
--- a/drivers/net/ethernet/micrel/ks8851.c
+++ b/drivers/net/ethernet/micrel/ks8851.c
@@ -185,36 +185,6 @@ static void ks8851_wrreg16(struct ks8851_net *ks, unsigned reg, unsigned val)
 		netdev_err(ks->netdev, "spi_sync() failed\n");
 }
 
-/**
- * ks8851_wrreg8 - write 8bit register value to chip
- * @ks: The chip state
- * @reg: The register address
- * @val: The value to write
- *
- * Issue a write to put the value @val into the register specified in @reg.
- */
-static void ks8851_wrreg8(struct ks8851_net *ks, unsigned reg, unsigned val)
-{
-	struct spi_transfer *xfer = &ks->spi_xfer1;
-	struct spi_message *msg = &ks->spi_msg1;
-	__le16 txb[2];
-	int ret;
-	int bit;
-
-	bit = 1 << (reg & 3);
-
-	txb[0] = cpu_to_le16(MK_OP(bit, reg) | KS_SPIOP_WR);
-	txb[1] = val;
-
-	xfer->tx_buf = txb;
-	xfer->rx_buf = NULL;
-	xfer->len = 3;
-
-	ret = spi_sync(ks->spidev, msg);
-	if (ret < 0)
-		netdev_err(ks->netdev, "spi_sync() failed\n");
-}
-
 /**
  * ks8851_rdreg - issue read register command and return the data
  * @ks: The device state
@@ -349,6 +319,7 @@ static void ks8851_set_powermode(struct ks8851_net *ks, unsigned pwrmode)
 static int ks8851_write_mac_addr(struct net_device *dev)
 {
 	struct ks8851_net *ks = netdev_priv(dev);
+	u16 val;
 	int i;
 
 	mutex_lock(&ks->lock);
@@ -358,8 +329,12 @@ static int ks8851_write_mac_addr(struct net_device *dev)
 	 * the first write to the MAC address does not take effect.
 	 */
 	ks8851_set_powermode(ks, PMECR_PM_NORMAL);
-	for (i = 0; i < ETH_ALEN; i++)
-		ks8851_wrreg8(ks, KS_MAR(i), dev->dev_addr[i]);
+
+	for (i = 0; i < ETH_ALEN; i += 2) {
+		val = (dev->dev_addr[i] << 8) | dev->dev_addr[i + 1];
+		ks8851_wrreg16(ks, KS_MAR(i + 1), val);
+	}
+
 	if (!netif_running(dev))
 		ks8851_set_powermode(ks, PMECR_PM_SOFTDOWN);
 
@@ -377,12 +352,16 @@ static int ks8851_write_mac_addr(struct net_device *dev)
 static void ks8851_read_mac_addr(struct net_device *dev)
 {
 	struct ks8851_net *ks = netdev_priv(dev);
+	u16 reg;
 	int i;
 
 	mutex_lock(&ks->lock);
 
-	for (i = 0; i < ETH_ALEN; i++)
-		dev->dev_addr[i] = ks8851_rdreg8(ks, KS_MAR(i));
+	for (i = 0; i < ETH_ALEN; i += 2) {
+		reg = ks8851_rdreg16(ks, KS_MAR(i + 1));
+		dev->dev_addr[i] = reg & 0xff;
+		dev->dev_addr[i + 1] = reg >> 8;
+	}
 
 	mutex_unlock(&ks->lock);
 }
-- 
2.25.1

Re: [PATCH 07/14] net: ks8851: Use 16-bit writes to program MAC address

[Andrew Lunn]

On Tue, Mar 24, 2020 at 12:42:56AM +0100, Marek Vasut wrote:
> On the SPI variant of KS8851, the MAC address can be programmed with
> either 8/16/32-bit writes. To make it easier to support the 16-bit
> parallel option of KS8851 too, switch both the MAC address programming
> and readout to 16-bit operations.
> 
> Remove ks8851_wrreg8() as it is not used anywhere anymore.
> 
> There should be no functional change.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Lukas Wunner <lukas@wunner.de>
> Cc: Petr Stetiar <ynezz@true.cz>
> Cc: YueHaibing <yuehaibing@huawei.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH 07/14] net: ks8851: Use 16-bit writes to program MAC address

[Michal Kubecek]

On Tue, Mar 24, 2020 at 12:42:56AM +0100, Marek Vasut wrote:
> On the SPI variant of KS8851, the MAC address can be programmed with
> either 8/16/32-bit writes. To make it easier to support the 16-bit
> parallel option of KS8851 too, switch both the MAC address programming
> and readout to 16-bit operations.
> 
> Remove ks8851_wrreg8() as it is not used anywhere anymore.
> 
> There should be no functional change.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Lukas Wunner <lukas@wunner.de>
> Cc: Petr Stetiar <ynezz@true.cz>
> Cc: YueHaibing <yuehaibing@huawei.com>
> ---
[...]
> +
> +	for (i = 0; i < ETH_ALEN; i += 2) {
> +		val = (dev->dev_addr[i] << 8) | dev->dev_addr[i + 1];
> +		ks8851_wrreg16(ks, KS_MAR(i + 1), val);
> +	}
[...]
> +	for (i = 0; i < ETH_ALEN; i += 2) {
> +		reg = ks8851_rdreg16(ks, KS_MAR(i + 1));
> +		dev->dev_addr[i] = reg & 0xff;
> +		dev->dev_addr[i + 1] = reg >> 8;
> +	}

I know nothing about the hardware but this seems inconsistent: while
writing, you put addr[i] into upper part of the 16-bit value and
addr[i+1] into lower but for read you do the opposite. Is it correct?

Michal Kubecek

Re: [PATCH 07/14] net: ks8851: Use 16-bit writes to program MAC address

[Lukas Wunner]

On Tue, Mar 24, 2020 at 12:42:56AM +0100, Marek Vasut wrote:
> On the SPI variant of KS8851, the MAC address can be programmed with
> either 8/16/32-bit writes. To make it easier to support the 16-bit
> parallel option of KS8851 too, switch both the MAC address programming
> and readout to 16-bit operations.
[...]
>  static int ks8851_write_mac_addr(struct net_device *dev)
>  {
>  	struct ks8851_net *ks = netdev_priv(dev);
> +	u16 val;
>  	int i;
>  
>  	mutex_lock(&ks->lock);
> @@ -358,8 +329,12 @@ static int ks8851_write_mac_addr(struct net_device *dev)
>  	 * the first write to the MAC address does not take effect.
>  	 */
>  	ks8851_set_powermode(ks, PMECR_PM_NORMAL);
> -	for (i = 0; i < ETH_ALEN; i++)
> -		ks8851_wrreg8(ks, KS_MAR(i), dev->dev_addr[i]);
> +
> +	for (i = 0; i < ETH_ALEN; i += 2) {
> +		val = (dev->dev_addr[i] << 8) | dev->dev_addr[i + 1];
> +		ks8851_wrreg16(ks, KS_MAR(i + 1), val);
> +	}
> +

This looks like it won't work on little-endian machines:  The MAC bytes
are stored in dev->dev_addr as 012345, but in the EEPROM they're stored
as 543210.  The first 16-bit value that you write is 10 on big-endian
and 01 on little-endian if I'm not mistaken.

By only writing 8-bit values, the original author elegantly sidestepped
this issue.

Maybe the simplest and most readable solution is something like:

      u8 val[2];
      ...
      val[0] = dev->dev_addr[i+1];
      val[1] = dev->dev_addr;

Then cast val to a u16 when passing it to ks8851_wrreg16().

Alternatively, use cpu_to_be16().


>  static void ks8851_read_mac_addr(struct net_device *dev)
>  {
>  	struct ks8851_net *ks = netdev_priv(dev);
> +	u16 reg;
>  	int i;
>  
>  	mutex_lock(&ks->lock);
>  
> -	for (i = 0; i < ETH_ALEN; i++)
> -		dev->dev_addr[i] = ks8851_rdreg8(ks, KS_MAR(i));
> +	for (i = 0; i < ETH_ALEN; i += 2) {
> +		reg = ks8851_rdreg16(ks, KS_MAR(i + 1));
> +		dev->dev_addr[i] = reg & 0xff;
> +		dev->dev_addr[i + 1] = reg >> 8;
> +	}

Same here.

These seem to be the only two places where KS_MAR() is used.
You may want to adjust that macro so that you don't have to add 1
in each of the two places.

Thanks,

Lukas

Re: [PATCH 07/14] net: ks8851: Use 16-bit writes to program MAC address

[Andrew Lunn]

On Tue, Mar 24, 2020 at 09:13:11AM +0100, Lukas Wunner wrote:
> On Tue, Mar 24, 2020 at 12:42:56AM +0100, Marek Vasut wrote:
> > On the SPI variant of KS8851, the MAC address can be programmed with
> > either 8/16/32-bit writes. To make it easier to support the 16-bit
> > parallel option of KS8851 too, switch both the MAC address programming
> > and readout to 16-bit operations.
> [...]
> >  static int ks8851_write_mac_addr(struct net_device *dev)
> >  {
> >  	struct ks8851_net *ks = netdev_priv(dev);
> > +	u16 val;
> >  	int i;
> >  
> >  	mutex_lock(&ks->lock);
> > @@ -358,8 +329,12 @@ static int ks8851_write_mac_addr(struct net_device *dev)
> >  	 * the first write to the MAC address does not take effect.
> >  	 */
> >  	ks8851_set_powermode(ks, PMECR_PM_NORMAL);
> > -	for (i = 0; i < ETH_ALEN; i++)
> > -		ks8851_wrreg8(ks, KS_MAR(i), dev->dev_addr[i]);
> > +
> > +	for (i = 0; i < ETH_ALEN; i += 2) {
> > +		val = (dev->dev_addr[i] << 8) | dev->dev_addr[i + 1];
> > +		ks8851_wrreg16(ks, KS_MAR(i + 1), val);
> > +	}
> > +
> 
> This looks like it won't work on little-endian machines:  The MAC bytes
> are stored in dev->dev_addr as 012345, but in the EEPROM they're stored
> as 543210.  The first 16-bit value that you write is 10 on big-endian
> and 01 on little-endian if I'm not mistaken.
> 
> By only writing 8-bit values, the original author elegantly sidestepped
> this issue.
> 
> Maybe the simplest and most readable solution is something like:
> 
>       u8 val[2];
>       ...
>       val[0] = dev->dev_addr[i+1];
>       val[1] = dev->dev_addr;
> 
> Then cast val to a u16 when passing it to ks8851_wrreg16().
> 
> Alternatively, use cpu_to_be16().

Hi Lukas

There is a cpu_to_be16() inside ks8851_wrreg16(). Something i already
checked, because i wondered about endianess issues as well.

	 Andrew

Re: [PATCH 07/14] net: ks8851: Use 16-bit writes to program MAC address

[Lukas Wunner]

On Tue, Mar 24, 2020 at 01:25:53PM +0100, Andrew Lunn wrote:
> On Tue, Mar 24, 2020 at 09:13:11AM +0100, Lukas Wunner wrote:
> > On Tue, Mar 24, 2020 at 12:42:56AM +0100, Marek Vasut wrote:
> > > On the SPI variant of KS8851, the MAC address can be programmed with
> > > either 8/16/32-bit writes. To make it easier to support the 16-bit
> > > parallel option of KS8851 too, switch both the MAC address programming
> > > and readout to 16-bit operations.
> > [...]
> > >  static int ks8851_write_mac_addr(struct net_device *dev)
> > >  {
> > >  	struct ks8851_net *ks = netdev_priv(dev);
> > > +	u16 val;
> > >  	int i;
> > >  
> > >  	mutex_lock(&ks->lock);
> > > @@ -358,8 +329,12 @@ static int ks8851_write_mac_addr(struct net_device *dev)
> > >  	 * the first write to the MAC address does not take effect.
> > >  	 */
> > >  	ks8851_set_powermode(ks, PMECR_PM_NORMAL);
> > > -	for (i = 0; i < ETH_ALEN; i++)
> > > -		ks8851_wrreg8(ks, KS_MAR(i), dev->dev_addr[i]);
> > > +
> > > +	for (i = 0; i < ETH_ALEN; i += 2) {
> > > +		val = (dev->dev_addr[i] << 8) | dev->dev_addr[i + 1];
> > > +		ks8851_wrreg16(ks, KS_MAR(i + 1), val);
> > > +	}
> > > +
> > 
> > This looks like it won't work on little-endian machines:  The MAC bytes
> > are stored in dev->dev_addr as 012345, but in the EEPROM they're stored
> > as 543210.  The first 16-bit value that you write is 10 on big-endian
> > and 01 on little-endian if I'm not mistaken.
> > 
> > By only writing 8-bit values, the original author elegantly sidestepped
> > this issue.
> > 
> > Maybe the simplest and most readable solution is something like:
> > 
> >       u8 val[2];
> >       ...
> >       val[0] = dev->dev_addr[i+1];
> >       val[1] = dev->dev_addr;
> > 
> > Then cast val to a u16 when passing it to ks8851_wrreg16().
> > 
> > Alternatively, use cpu_to_be16().
> 
> There is a cpu_to_be16() inside ks8851_wrreg16(). Something i already
> checked, because i wondered about endianess issues as well.

There's a cpu_to_le16() in ks8851_wrreg16(), not a cpu_to_be16().

Thanks,

Lukas

Re: [PATCH 07/14] net: ks8851: Use 16-bit writes to program MAC address

[Marek Vasut]

On 3/24/20 1:36 PM, Lukas Wunner wrote:
> On Tue, Mar 24, 2020 at 01:25:53PM +0100, Andrew Lunn wrote:
>> On Tue, Mar 24, 2020 at 09:13:11AM +0100, Lukas Wunner wrote:
>>> On Tue, Mar 24, 2020 at 12:42:56AM +0100, Marek Vasut wrote:
>>>> On the SPI variant of KS8851, the MAC address can be programmed with
>>>> either 8/16/32-bit writes. To make it easier to support the 16-bit
>>>> parallel option of KS8851 too, switch both the MAC address programming
>>>> and readout to 16-bit operations.
>>> [...]
>>>>  static int ks8851_write_mac_addr(struct net_device *dev)
>>>>  {
>>>>  	struct ks8851_net *ks = netdev_priv(dev);
>>>> +	u16 val;
>>>>  	int i;
>>>>  
>>>>  	mutex_lock(&ks->lock);
>>>> @@ -358,8 +329,12 @@ static int ks8851_write_mac_addr(struct net_device *dev)
>>>>  	 * the first write to the MAC address does not take effect.
>>>>  	 */
>>>>  	ks8851_set_powermode(ks, PMECR_PM_NORMAL);
>>>> -	for (i = 0; i < ETH_ALEN; i++)
>>>> -		ks8851_wrreg8(ks, KS_MAR(i), dev->dev_addr[i]);
>>>> +
>>>> +	for (i = 0; i < ETH_ALEN; i += 2) {
>>>> +		val = (dev->dev_addr[i] << 8) | dev->dev_addr[i + 1];
>>>> +		ks8851_wrreg16(ks, KS_MAR(i + 1), val);
>>>> +	}
>>>> +
>>>
>>> This looks like it won't work on little-endian machines:  The MAC bytes
>>> are stored in dev->dev_addr as 012345, but in the EEPROM they're stored
>>> as 543210.  The first 16-bit value that you write is 10 on big-endian
>>> and 01 on little-endian if I'm not mistaken.
>>>
>>> By only writing 8-bit values, the original author elegantly sidestepped
>>> this issue.
>>>
>>> Maybe the simplest and most readable solution is something like:
>>>
>>>       u8 val[2];
>>>       ...
>>>       val[0] = dev->dev_addr[i+1];
>>>       val[1] = dev->dev_addr;
>>>
>>> Then cast val to a u16 when passing it to ks8851_wrreg16().
>>>
>>> Alternatively, use cpu_to_be16().
>>
>> There is a cpu_to_be16() inside ks8851_wrreg16(). Something i already
>> checked, because i wondered about endianess issues as well.
> 
> There's a cpu_to_le16() in ks8851_wrreg16(), not a cpu_to_be16().

I have a feeling this whole thing might be more messed up then we
thought. At least the KS8851-16MLL has an "endian mode" bit in the CCR
register, the SPI variant does not.

So what I think you need to do here is write exactly the registers
0x14/0x12/0x10 and let the accessors swap the endianness as needed.

Re: [PATCH 07/14] net: ks8851: Use 16-bit writes to program MAC address

[Marek Vasut]

On 3/24/20 2:09 PM, Marek Vasut wrote:
> On 3/24/20 1:36 PM, Lukas Wunner wrote:
>> On Tue, Mar 24, 2020 at 01:25:53PM +0100, Andrew Lunn wrote:
>>> On Tue, Mar 24, 2020 at 09:13:11AM +0100, Lukas Wunner wrote:
>>>> On Tue, Mar 24, 2020 at 12:42:56AM +0100, Marek Vasut wrote:
>>>>> On the SPI variant of KS8851, the MAC address can be programmed with
>>>>> either 8/16/32-bit writes. To make it easier to support the 16-bit
>>>>> parallel option of KS8851 too, switch both the MAC address programming
>>>>> and readout to 16-bit operations.
>>>> [...]
>>>>>  static int ks8851_write_mac_addr(struct net_device *dev)
>>>>>  {
>>>>>  	struct ks8851_net *ks = netdev_priv(dev);
>>>>> +	u16 val;
>>>>>  	int i;
>>>>>  
>>>>>  	mutex_lock(&ks->lock);
>>>>> @@ -358,8 +329,12 @@ static int ks8851_write_mac_addr(struct net_device *dev)
>>>>>  	 * the first write to the MAC address does not take effect.
>>>>>  	 */
>>>>>  	ks8851_set_powermode(ks, PMECR_PM_NORMAL);
>>>>> -	for (i = 0; i < ETH_ALEN; i++)
>>>>> -		ks8851_wrreg8(ks, KS_MAR(i), dev->dev_addr[i]);
>>>>> +
>>>>> +	for (i = 0; i < ETH_ALEN; i += 2) {
>>>>> +		val = (dev->dev_addr[i] << 8) | dev->dev_addr[i + 1];
>>>>> +		ks8851_wrreg16(ks, KS_MAR(i + 1), val);
>>>>> +	}
>>>>> +
>>>>
>>>> This looks like it won't work on little-endian machines:  The MAC bytes
>>>> are stored in dev->dev_addr as 012345, but in the EEPROM they're stored
>>>> as 543210.  The first 16-bit value that you write is 10 on big-endian
>>>> and 01 on little-endian if I'm not mistaken.
>>>>
>>>> By only writing 8-bit values, the original author elegantly sidestepped
>>>> this issue.
>>>>
>>>> Maybe the simplest and most readable solution is something like:
>>>>
>>>>       u8 val[2];
>>>>       ...
>>>>       val[0] = dev->dev_addr[i+1];
>>>>       val[1] = dev->dev_addr;
>>>>
>>>> Then cast val to a u16 when passing it to ks8851_wrreg16().
>>>>
>>>> Alternatively, use cpu_to_be16().
>>>
>>> There is a cpu_to_be16() inside ks8851_wrreg16(). Something i already
>>> checked, because i wondered about endianess issues as well.
>>
>> There's a cpu_to_le16() in ks8851_wrreg16(), not a cpu_to_be16().
> 
> I have a feeling this whole thing might be more messed up then we
> thought. At least the KS8851-16MLL has an "endian mode" bit in the CCR
> register, the SPI variant does not.
> 
> So what I think you need to do here is write exactly the registers
> 0x14/0x12/0x10 and let the accessors swap the endianness as needed.

I'll try to get the parallel version with switched endianness, but that
might take a few days.

However, the SPI variant should be able to write the MAC with the code
above just fine, no matter what the host endianness is, right ?

Re: [PATCH 07/14] net: ks8851: Use 16-bit writes to program MAC address

[Lukas Wunner]

On Tue, Mar 24, 2020 at 02:09:18PM +0100, Marek Vasut wrote:
> I have a feeling this whole thing might be more messed up then we
> thought. At least the KS8851-16MLL has an "endian mode" bit in the CCR
> register, the SPI variant does not.

On the MLL variant of this chip, pin 10 can be pulled up to force it
into big endian mode, otherwise it's in little-endian mode.  Obviously
this should be configured by the board designer such that it matches
the CPU's endianness.

Of course we *could* support inverted endianness in case the hardware
engineer botched the board layout.  Not sure if we have to.

In the CCR register that you mention, you can determine whether the
pin is pulled up or not.  If it is in big-endian mode and you're
on a little-endian CPU, you're hosed and the only option that you've
got is to invert endianness in software, i.e. in the accessors.

If the pin is pulled to ground or not connected (again, can be
determined from CCR) then you're able to switch the endianness by
setting bit 11 in the RXFDPR register.  No need to convert it in
the accessors in this case.

Thanks,

Lukas

Re: [PATCH 07/14] net: ks8851: Use 16-bit writes to program MAC address

[Marek Vasut]

On 3/24/20 3:47 PM, Lukas Wunner wrote:
> On Tue, Mar 24, 2020 at 02:09:18PM +0100, Marek Vasut wrote:
>> I have a feeling this whole thing might be more messed up then we
>> thought. At least the KS8851-16MLL has an "endian mode" bit in the CCR
>> register, the SPI variant does not.
> 
> On the MLL variant of this chip, pin 10 can be pulled up to force it
> into big endian mode, otherwise it's in little-endian mode.  Obviously
> this should be configured by the board designer such that it matches
> the CPU's endianness.

Sadly, that's not the case on the device I have here right now.
So I'm suffering the performance impact of having to endian-swap on
every 16bit access.

> Of course we *could* support inverted endianness in case the hardware
> engineer botched the board layout.  Not sure if we have to.
> 
> In the CCR register that you mention, you can determine whether the
> pin is pulled up or not.  If it is in big-endian mode and you're
> on a little-endian CPU, you're hosed and the only option that you've
> got is to invert endianness in software, i.e. in the accessors.

Yes

> If the pin is pulled to ground or not connected (again, can be
> determined from CCR) then you're able to switch the endianness by
> setting bit 11 in the RXFDPR register.  No need to convert it in
> the accessors in this case.

That's not the setup I have right now, sadly.

[PATCH 08/14] net: ks8851: Use 16-bit read of RXFC register

[Marek Vasut]

The RXFC register is the only one being read using 8-bit accessors.
To make it easier to support the 16-bit accesses used by the parallel
bus variant of KS8851, use 16-bit accessor to read RXFC register as
well as neighboring RXFCTR register.

Remove ks8851_rdreg8() as it is not used anywhere anymore.

There should be no functional change.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: David S. Miller <davem@davemloft.net>
Cc: Lukas Wunner <lukas@wunner.de>
Cc: Petr Stetiar <ynezz@true.cz>
Cc: YueHaibing <yuehaibing@huawei.com>
---
 drivers/net/ethernet/micrel/ks8851.c | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
index 3150f1b928c0..03d349208794 100644
--- a/drivers/net/ethernet/micrel/ks8851.c
+++ b/drivers/net/ethernet/micrel/ks8851.c
@@ -236,21 +236,6 @@ static void ks8851_rdreg(struct ks8851_net *ks, unsigned op,
 		memcpy(rxb, trx + 2, rxl);
 }
 
-/**
- * ks8851_rdreg8 - read 8 bit register from device
- * @ks: The chip information
- * @reg: The register address
- *
- * Read a 8bit register from the chip, returning the result
-*/
-static unsigned ks8851_rdreg8(struct ks8851_net *ks, unsigned reg)
-{
-	u8 rxb[1];
-
-	ks8851_rdreg(ks, MK_OP(1 << (reg & 3), reg), rxb, 1);
-	return rxb[0];
-}
-
 /**
  * ks8851_rdreg16 - read 16 bit register from device
  * @ks: The chip information
@@ -470,7 +455,7 @@ static void ks8851_rx_pkts(struct ks8851_net *ks)
 	unsigned rxstat;
 	u8 *rxpkt;
 
-	rxfc = ks8851_rdreg8(ks, KS_RXFC);
+	rxfc = (ks8851_rdreg16(ks, KS_RXFCTR) >> 8) & 0xff;
 
 	netif_dbg(ks, rx_status, ks->netdev,
 		  "%s: %d packets\n", __func__, rxfc);
-- 
2.25.1

Re: [PATCH 08/14] net: ks8851: Use 16-bit read of RXFC register

[Andrew Lunn]

> @@ -470,7 +455,7 @@ static void ks8851_rx_pkts(struct ks8851_net *ks)
>  	unsigned rxstat;
>  	u8 *rxpkt;
>  
> -	rxfc = ks8851_rdreg8(ks, KS_RXFC);
> +	rxfc = (ks8851_rdreg16(ks, KS_RXFCTR) >> 8) & 0xff;

The datasheet says:

2. When software driver reads back Receive Frame Count (RXFCTR)
Register; the KSZ8851 will update both Receive Frame Header Status and
Byte Count Registers (RXFHSR/RXFHBCR)

Are you sure there is no side affect here?

    Andrew

Re: [PATCH 08/14] net: ks8851: Use 16-bit read of RXFC register

[Marek Vasut]

On 3/24/20 2:50 AM, Andrew Lunn wrote:
>> @@ -470,7 +455,7 @@ static void ks8851_rx_pkts(struct ks8851_net *ks)
>>  	unsigned rxstat;
>>  	u8 *rxpkt;
>>  
>> -	rxfc = ks8851_rdreg8(ks, KS_RXFC);
>> +	rxfc = (ks8851_rdreg16(ks, KS_RXFCTR) >> 8) & 0xff;
> 
> The datasheet says:
> 
> 2. When software driver reads back Receive Frame Count (RXFCTR)
> Register; the KSZ8851 will update both Receive Frame Header Status and
> Byte Count Registers (RXFHSR/RXFHBCR)
> 
> Are you sure there is no side affect here?

Yes, look at the RXFC register 0x9c itself. It's a 16bit register, 0x9c
is the LSByte and 0x9d is the MSByte.

What happened here before was readout of register 0x9d, MSByte of RXFC,
which triggers the update of RXFHSR/RXFHBCR. What happens now is the
readout of the whole RXFC as 16bit value, which also triggers the update.

Re: [PATCH 08/14] net: ks8851: Use 16-bit read of RXFC register

[Andrew Lunn]

On Tue, Mar 24, 2020 at 01:50:53PM +0100, Marek Vasut wrote:
> On 3/24/20 2:50 AM, Andrew Lunn wrote:
> >> @@ -470,7 +455,7 @@ static void ks8851_rx_pkts(struct ks8851_net *ks)
> >>  	unsigned rxstat;
> >>  	u8 *rxpkt;
> >>  
> >> -	rxfc = ks8851_rdreg8(ks, KS_RXFC);
> >> +	rxfc = (ks8851_rdreg16(ks, KS_RXFCTR) >> 8) & 0xff;
> > 
> > The datasheet says:
> > 
> > 2. When software driver reads back Receive Frame Count (RXFCTR)
> > Register; the KSZ8851 will update both Receive Frame Header Status and
> > Byte Count Registers (RXFHSR/RXFHBCR)
> > 
> > Are you sure there is no side affect here?
> 
> Yes, look at the RXFC register 0x9c itself. It's a 16bit register, 0x9c
> is the LSByte and 0x9d is the MSByte.
> 
> What happened here before was readout of register 0x9d, MSByte of RXFC,
> which triggers the update of RXFHSR/RXFHBCR. What happens now is the
> readout of the whole RXFC as 16bit value, which also triggers the update.

Hi Marek

It would be nice to indicate in the commit message that things like
this have been considered. As a reviewer, these are the sort of
questions which goes through my mind. If there is a comment it has
been considered, i get the answer to my questions without having to
ask.

	Andrew

Re: [PATCH 08/14] net: ks8851: Use 16-bit read of RXFC register

[Lukas Wunner]

On Tue, Mar 24, 2020 at 12:42:57AM +0100, Marek Vasut wrote:
> The RXFC register is the only one being read using 8-bit accessors.
> To make it easier to support the 16-bit accesses used by the parallel
> bus variant of KS8851, use 16-bit accessor to read RXFC register as
> well as neighboring RXFCTR register.

This means that an additional 8 bits need to be transferred over the
SPI bus whenever a set of packets is read from the RX queue.  This
should be avoided.  I'd suggest adding a separate hook to read RXFC
and thus keep the 8-bit read function for the SPI variant.

Thanks,

Lukas

Re: [PATCH 08/14] net: ks8851: Use 16-bit read of RXFC register

[Marek Vasut]

On 3/24/20 11:41 AM, Lukas Wunner wrote:
> On Tue, Mar 24, 2020 at 12:42:57AM +0100, Marek Vasut wrote:
>> The RXFC register is the only one being read using 8-bit accessors.
>> To make it easier to support the 16-bit accesses used by the parallel
>> bus variant of KS8851, use 16-bit accessor to read RXFC register as
>> well as neighboring RXFCTR register.
> 
> This means that an additional 8 bits need to be transferred over the
> SPI bus whenever a set of packets is read from the RX queue.  This
> should be avoided.  I'd suggest adding a separate hook to read RXFC
> and thus keep the 8-bit read function for the SPI variant.

See my comment about the 32bit read and regmap. It is slightly less
efficient, but it also makes the conversion much easier. Can you check
on the real hardware whether the is measurable performance impact ?

[PATCH 09/14] net: ks8851: Split out SPI specific entries in struct ks8851_net

[Marek Vasut]

Add a new struct ks8851_net_spi, which embeds the original
struct ks8851_net and contains the entries specific only to
the SPI variant of KS8851.

There should be no functional change.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: David S. Miller <davem@davemloft.net>
Cc: Lukas Wunner <lukas@wunner.de>
Cc: Petr Stetiar <ynezz@true.cz>
Cc: YueHaibing <yuehaibing@huawei.com>
---
 drivers/net/ethernet/micrel/ks8851.c | 76 +++++++++++++++++-----------
 1 file changed, 47 insertions(+), 29 deletions(-)

diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
index 03d349208794..56f505ae6458 100644
--- a/drivers/net/ethernet/micrel/ks8851.c
+++ b/drivers/net/ethernet/micrel/ks8851.c
@@ -102,7 +102,6 @@ union ks8851_tx_hdr {
  */
 struct ks8851_net {
 	struct net_device	*netdev;
-	struct spi_device	*spidev;
 	struct mutex		lock;
 	spinlock_t		statelock;
 
@@ -126,17 +125,30 @@ struct ks8851_net {
 
 	struct sk_buff_head	txq;
 
-	struct spi_message	spi_msg1;
-	struct spi_message	spi_msg2;
-	struct spi_transfer	spi_xfer1;
-	struct spi_transfer	spi_xfer2[2];
-
 	struct eeprom_93cx6	eeprom;
 	struct regulator	*vdd_reg;
 	struct regulator	*vdd_io;
 	int			gpio;
 };
 
+/**
+ * struct ks8851_net_spi - KS8851 SPI driver private data
+ * @ks8851: KS8851 driver common private data
+ * @spidev: The spi device we're bound to.
+ * @spi_msg1: pre-setup SPI transfer with one message, @spi_xfer1.
+ * @spi_msg2: pre-setup SPI transfer with two messages, @spi_xfer2.
+ */
+struct ks8851_net_spi {
+	struct ks8851_net	ks8851;	/* Must be first */
+	struct spi_device	*spidev;
+	struct spi_message	spi_msg1;
+	struct spi_message	spi_msg2;
+	struct spi_transfer	spi_xfer1;
+	struct spi_transfer	spi_xfer2[2];
+};
+
+#define to_ks8851_spi(ks) container_of((ks), struct ks8851_net_spi, ks8851)
+
 static int msg_enable;
 
 /* SPI frame opcodes */
@@ -168,8 +180,9 @@ static int msg_enable;
  */
 static void ks8851_wrreg16(struct ks8851_net *ks, unsigned reg, unsigned val)
 {
-	struct spi_transfer *xfer = &ks->spi_xfer1;
-	struct spi_message *msg = &ks->spi_msg1;
+	struct ks8851_net_spi *kss = to_ks8851_spi(ks);
+	struct spi_transfer *xfer = &kss->spi_xfer1;
+	struct spi_message *msg = &kss->spi_msg1;
 	__le16 txb[2];
 	int ret;
 
@@ -180,7 +193,7 @@ static void ks8851_wrreg16(struct ks8851_net *ks, unsigned reg, unsigned val)
 	xfer->rx_buf = NULL;
 	xfer->len = 4;
 
-	ret = spi_sync(ks->spidev, msg);
+	ret = spi_sync(kss->spidev, msg);
 	if (ret < 0)
 		netdev_err(ks->netdev, "spi_sync() failed\n");
 }
@@ -198,6 +211,7 @@ static void ks8851_wrreg16(struct ks8851_net *ks, unsigned reg, unsigned val)
 static void ks8851_rdreg(struct ks8851_net *ks, unsigned op,
 			 u8 *rxb, unsigned rxl)
 {
+	struct ks8851_net_spi *kss = to_ks8851_spi(ks);
 	struct spi_transfer *xfer;
 	struct spi_message *msg;
 	__le16 *txb = (__le16 *)ks->txd;
@@ -206,9 +220,9 @@ static void ks8851_rdreg(struct ks8851_net *ks, unsigned op,
 
 	txb[0] = cpu_to_le16(op | KS_SPIOP_RD);
 
-	if (ks->spidev->master->flags & SPI_MASTER_HALF_DUPLEX) {
-		msg = &ks->spi_msg2;
-		xfer = ks->spi_xfer2;
+	if (kss->spidev->master->flags & SPI_MASTER_HALF_DUPLEX) {
+		msg = &kss->spi_msg2;
+		xfer = kss->spi_xfer2;
 
 		xfer->tx_buf = txb;
 		xfer->rx_buf = NULL;
@@ -219,18 +233,18 @@ static void ks8851_rdreg(struct ks8851_net *ks, unsigned op,
 		xfer->rx_buf = trx;
 		xfer->len = rxl;
 	} else {
-		msg = &ks->spi_msg1;
-		xfer = &ks->spi_xfer1;
+		msg = &kss->spi_msg1;
+		xfer = &kss->spi_xfer1;
 
 		xfer->tx_buf = txb;
 		xfer->rx_buf = trx;
 		xfer->len = rxl + 2;
 	}
 
-	ret = spi_sync(ks->spidev, msg);
+	ret = spi_sync(kss->spidev, msg);
 	if (ret < 0)
 		netdev_err(ks->netdev, "read: spi_sync() failed\n");
-	else if (ks->spidev->master->flags & SPI_MASTER_HALF_DUPLEX)
+	else if (kss->spidev->master->flags & SPI_MASTER_HALF_DUPLEX)
 		memcpy(rxb, trx, rxl);
 	else
 		memcpy(rxb, trx + 2, rxl);
@@ -398,8 +412,9 @@ static void ks8851_init_mac(struct ks8851_net *ks, struct device *ddev)
  */
 static void ks8851_rdfifo(struct ks8851_net *ks, u8 *buff, unsigned len)
 {
-	struct spi_transfer *xfer = ks->spi_xfer2;
-	struct spi_message *msg = &ks->spi_msg2;
+	struct ks8851_net_spi *kss = to_ks8851_spi(ks);
+	struct spi_transfer *xfer = kss->spi_xfer2;
+	struct spi_message *msg = &kss->spi_msg2;
 	u8 txb[1];
 	int ret;
 
@@ -418,7 +433,7 @@ static void ks8851_rdfifo(struct ks8851_net *ks, u8 *buff, unsigned len)
 	xfer->tx_buf = NULL;
 	xfer->len = len;
 
-	ret = spi_sync(ks->spidev, msg);
+	ret = spi_sync(kss->spidev, msg);
 	if (ret < 0)
 		netdev_err(ks->netdev, "%s: spi_sync() failed\n", __func__);
 }
@@ -642,8 +657,9 @@ static inline unsigned calc_txlen(unsigned len)
  */
 static void ks8851_wrpkt(struct ks8851_net *ks, struct sk_buff *txp, bool irq)
 {
-	struct spi_transfer *xfer = ks->spi_xfer2;
-	struct spi_message *msg = &ks->spi_msg2;
+	struct ks8851_net_spi *kss = to_ks8851_spi(ks);
+	struct spi_transfer *xfer = kss->spi_xfer2;
+	struct spi_message *msg = &kss->spi_msg2;
 	unsigned fid = 0;
 	int ret;
 
@@ -670,7 +686,7 @@ static void ks8851_wrpkt(struct ks8851_net *ks, struct sk_buff *txp, bool irq)
 	xfer->rx_buf = NULL;
 	xfer->len = ALIGN(txp->len, 4);
 
-	ret = spi_sync(ks->spidev, msg);
+	ret = spi_sync(kss->spidev, msg);
 	if (ret < 0)
 		netdev_err(ks->netdev, "%s: spi_sync() failed\n", __func__);
 }
@@ -1358,22 +1374,24 @@ static SIMPLE_DEV_PM_OPS(ks8851_pm_ops, ks8851_suspend, ks8851_resume);
 static int ks8851_probe(struct spi_device *spi)
 {
 	struct device *dev = &spi->dev;
+	struct ks8851_net_spi *kss;
 	struct net_device *ndev;
 	struct ks8851_net *ks;
 	int ret;
 	unsigned cider;
 	int gpio;
 
-	ndev = devm_alloc_etherdev(dev, sizeof(struct ks8851_net));
+	ndev = devm_alloc_etherdev(dev, sizeof(struct ks8851_net_spi));
 	if (!ndev)
 		return -ENOMEM;
 
 	spi->bits_per_word = 8;
 
 	ks = netdev_priv(ndev);
+	kss = to_ks8851_spi(ks);
 
 	ks->netdev = ndev;
-	ks->spidev = spi;
+	kss->spidev = spi;
 	ks->tx_space = 6144;
 
 	gpio = of_get_named_gpio_flags(dev->of_node, "reset-gpios",
@@ -1430,12 +1448,12 @@ static int ks8851_probe(struct spi_device *spi)
 
 	/* initialise pre-made spi transfer messages */
 
-	spi_message_init(&ks->spi_msg1);
-	spi_message_add_tail(&ks->spi_xfer1, &ks->spi_msg1);
+	spi_message_init(&kss->spi_msg1);
+	spi_message_add_tail(&kss->spi_xfer1, &kss->spi_msg1);
 
-	spi_message_init(&ks->spi_msg2);
-	spi_message_add_tail(&ks->spi_xfer2[0], &ks->spi_msg2);
-	spi_message_add_tail(&ks->spi_xfer2[1], &ks->spi_msg2);
+	spi_message_init(&kss->spi_msg2);
+	spi_message_add_tail(&kss->spi_xfer2[0], &kss->spi_msg2);
+	spi_message_add_tail(&kss->spi_xfer2[1], &kss->spi_msg2);
 
 	/* setup EEPROM state */
 
-- 
2.25.1

Re: [PATCH 09/14] net: ks8851: Split out SPI specific entries in struct ks8851_net

[Andrew Lunn]

> + * struct ks8851_net_spi - KS8851 SPI driver private data
> + * @ks8851: KS8851 driver common private data
> + * @spidev: The spi device we're bound to.
> + * @spi_msg1: pre-setup SPI transfer with one message, @spi_xfer1.
> + * @spi_msg2: pre-setup SPI transfer with two messages, @spi_xfer2.
> + */
> +struct ks8851_net_spi {
> +	struct ks8851_net	ks8851;	/* Must be first */
> +	struct spi_device	*spidev;
> +	struct spi_message	spi_msg1;
> +	struct spi_message	spi_msg2;
> +	struct spi_transfer	spi_xfer1;
> +	struct spi_transfer	spi_xfer2[2];
> +};
> +
> +#define to_ks8851_spi(ks) container_of((ks), struct ks8851_net_spi, ks8851)

Since you are using container_of(), ks8851 does not need to be first.

      Andrew

Re: [PATCH 09/14] net: ks8851: Split out SPI specific entries in struct ks8851_net

[Lukas Wunner]

On Tue, Mar 24, 2020 at 12:42:58AM +0100, Marek Vasut wrote:
> +/**
> + * struct ks8851_net_spi - KS8851 SPI driver private data
> + * @ks8851: KS8851 driver common private data
> + * @spidev: The spi device we're bound to.
> + * @spi_msg1: pre-setup SPI transfer with one message, @spi_xfer1.
> + * @spi_msg2: pre-setup SPI transfer with two messages, @spi_xfer2.

You need to remove these kerneldoc entries further up from struct ks8851_net.


> +struct ks8851_net_spi {
> +	struct ks8851_net	ks8851;	/* Must be first */
> +	struct spi_device	*spidev;
> +	struct spi_message	spi_msg1;
> +	struct spi_message	spi_msg2;
> +	struct spi_transfer	spi_xfer1;
> +	struct spi_transfer	spi_xfer2[2];
> +};
> +
> +#define to_ks8851_spi(ks) container_of((ks), struct ks8851_net_spi, ks8851)

Since it's always the first entry in the struct, a cast instead of
container_of() would seem to be sufficient.

Thanks,

Lukas

Re: [PATCH 09/14] net: ks8851: Split out SPI specific entries in struct ks8851_net

[Andrew Lunn]

On Tue, Mar 24, 2020 at 11:49:19AM +0100, Lukas Wunner wrote:
> On Tue, Mar 24, 2020 at 12:42:58AM +0100, Marek Vasut wrote:
> > +/**
> > + * struct ks8851_net_spi - KS8851 SPI driver private data
> > + * @ks8851: KS8851 driver common private data
> > + * @spidev: The spi device we're bound to.
> > + * @spi_msg1: pre-setup SPI transfer with one message, @spi_xfer1.
> > + * @spi_msg2: pre-setup SPI transfer with two messages, @spi_xfer2.
> 
> You need to remove these kerneldoc entries further up from struct ks8851_net.
> 
> 
> > +struct ks8851_net_spi {
> > +	struct ks8851_net	ks8851;	/* Must be first */
> > +	struct spi_device	*spidev;
> > +	struct spi_message	spi_msg1;
> > +	struct spi_message	spi_msg2;
> > +	struct spi_transfer	spi_xfer1;
> > +	struct spi_transfer	spi_xfer2[2];
> > +};
> > +
> > +#define to_ks8851_spi(ks) container_of((ks), struct ks8851_net_spi, ks8851)
> 
> Since it's always the first entry in the struct, a cast instead of
> container_of() would seem to be sufficient.

Hi Lukus

That is bad practice. container_of() will always get it correct,
making it safer.

       Andrew

[PATCH 10/14] net: ks8851: Split out SPI specific code from probe() and remove()

[Marek Vasut]

Factor out common code into ks8851_probe_common() and
ks8851_remove_common() to permit both SPI and parallel
bus driver variants to use the common code path for
both probing and removal.

There should be no functional change.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: David S. Miller <davem@davemloft.net>
Cc: Lukas Wunner <lukas@wunner.de>
Cc: Petr Stetiar <ynezz@true.cz>
Cc: YueHaibing <yuehaibing@huawei.com>
---
 drivers/net/ethernet/micrel/ks8851.c | 69 ++++++++++++++++------------
 1 file changed, 40 insertions(+), 29 deletions(-)

diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
index 56f505ae6458..608c8fe4b5e8 100644
--- a/drivers/net/ethernet/micrel/ks8851.c
+++ b/drivers/net/ethernet/micrel/ks8851.c
@@ -1371,27 +1371,14 @@ static int ks8851_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(ks8851_pm_ops, ks8851_suspend, ks8851_resume);
 
-static int ks8851_probe(struct spi_device *spi)
+static int ks8851_probe_common(struct net_device *ndev, struct device *dev)
 {
-	struct device *dev = &spi->dev;
-	struct ks8851_net_spi *kss;
-	struct net_device *ndev;
-	struct ks8851_net *ks;
-	int ret;
+	struct ks8851_net *ks = netdev_priv(ndev);
 	unsigned cider;
 	int gpio;
-
-	ndev = devm_alloc_etherdev(dev, sizeof(struct ks8851_net_spi));
-	if (!ndev)
-		return -ENOMEM;
-
-	spi->bits_per_word = 8;
-
-	ks = netdev_priv(ndev);
-	kss = to_ks8851_spi(ks);
+	int ret;
 
 	ks->netdev = ndev;
-	kss->spidev = spi;
 	ks->tx_space = 6144;
 
 	gpio = of_get_named_gpio_flags(dev->of_node, "reset-gpios",
@@ -1446,17 +1433,7 @@ static int ks8851_probe(struct spi_device *spi)
 	INIT_WORK(&ks->tx_work, ks8851_tx_work);
 	INIT_WORK(&ks->rxctrl_work, ks8851_rxctrl_work);
 
-	/* initialise pre-made spi transfer messages */
-
-	spi_message_init(&kss->spi_msg1);
-	spi_message_add_tail(&kss->spi_xfer1, &kss->spi_msg1);
-
-	spi_message_init(&kss->spi_msg2);
-	spi_message_add_tail(&kss->spi_xfer2[0], &kss->spi_msg2);
-	spi_message_add_tail(&kss->spi_xfer2[1], &kss->spi_msg2);
-
 	/* setup EEPROM state */
-
 	ks->eeprom.data = ks;
 	ks->eeprom.width = PCI_EEPROM_WIDTH_93C46;
 	ks->eeprom.register_read = ks8851_eeprom_regread;
@@ -1487,7 +1464,6 @@ static int ks8851_probe(struct spi_device *spi)
 	netif_carrier_off(ks->netdev);
 	ndev->if_port = IF_PORT_100BASET;
 	ndev->netdev_ops = &ks8851_netdev_ops;
-	ndev->irq = spi->irq;
 
 	/* issue a global soft reset to reset the device. */
 	ks8851_soft_reset(ks, GRR_GSR);
@@ -1529,9 +1505,8 @@ static int ks8851_probe(struct spi_device *spi)
 	return ret;
 }
 
-static int ks8851_remove(struct spi_device *spi)
+static int ks8851_remove_common(struct device *dev)
 {
-	struct device *dev = &spi->dev;
 	struct ks8851_net *priv = dev_get_drvdata(dev);
 
 	if (netif_msg_drv(priv))
@@ -1546,6 +1521,42 @@ static int ks8851_remove(struct spi_device *spi)
 	return 0;
 }
 
+static int ks8851_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct ks8851_net_spi *kss;
+	struct net_device *ndev;
+	struct ks8851_net *ks;
+
+	ndev = devm_alloc_etherdev(dev, sizeof(struct ks8851_net_spi));
+	if (!ndev)
+		return -ENOMEM;
+
+	spi->bits_per_word = 8;
+
+	ks = netdev_priv(ndev);
+	kss = to_ks8851_spi(ks);
+
+	kss->spidev = spi;
+
+	/* initialise pre-made spi transfer messages */
+	spi_message_init(&kss->spi_msg1);
+	spi_message_add_tail(&kss->spi_xfer1, &kss->spi_msg1);
+
+	spi_message_init(&kss->spi_msg2);
+	spi_message_add_tail(&kss->spi_xfer2[0], &kss->spi_msg2);
+	spi_message_add_tail(&kss->spi_xfer2[1], &kss->spi_msg2);
+
+	ndev->irq = spi->irq;
+
+	return ks8851_probe_common(ndev, dev);
+}
+
+static int ks8851_remove(struct spi_device *spi)
+{
+	return ks8851_remove_common(&spi->dev);
+}
+
 static const struct of_device_id ks8851_match_table[] = {
 	{ .compatible = "micrel,ks8851" },
 	{ }
-- 
2.25.1

Re: [PATCH 10/14] net: ks8851: Split out SPI specific code from probe() and remove()

[Andrew Lunn]

On Tue, Mar 24, 2020 at 12:42:59AM +0100, Marek Vasut wrote:
> Factor out common code into ks8851_probe_common() and
> ks8851_remove_common() to permit both SPI and parallel
> bus driver variants to use the common code path for
> both probing and removal.
> 
> There should be no functional change.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Lukas Wunner <lukas@wunner.de>
> Cc: Petr Stetiar <ynezz@true.cz>
> Cc: YueHaibing <yuehaibing@huawei.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH 11/14] net: ks8851: Implement register and FIFO accessor callbacks

[Marek Vasut]

The register and FIFO accessors are bus specific. Implement callbacks so
that each variant of the KS8851 can implement matching accessors and use
the rest of the common code.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: David S. Miller <davem@davemloft.net>
Cc: Lukas Wunner <lukas@wunner.de>
Cc: Petr Stetiar <ynezz@true.cz>
Cc: YueHaibing <yuehaibing@huawei.com>
---
 drivers/net/ethernet/micrel/ks8851.c | 65 +++++++++++++++++++++++-----
 1 file changed, 55 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
index 608c8fe4b5e8..d5bdbd9122bf 100644
--- a/drivers/net/ethernet/micrel/ks8851.c
+++ b/drivers/net/ethernet/micrel/ks8851.c
@@ -129,6 +129,15 @@ struct ks8851_net {
 	struct regulator	*vdd_reg;
 	struct regulator	*vdd_io;
 	int			gpio;
+
+	unsigned int		(*rdreg16)(struct ks8851_net *ks,
+					   unsigned int reg);
+	void			(*wrreg16)(struct ks8851_net *ks,
+					   unsigned int reg, unsigned int val);
+	void			(*rdfifo)(struct ks8851_net *ks, u8 *buff,
+					  unsigned int len);
+	void			(*wrfifo)(struct ks8851_net *ks,
+					  struct sk_buff *txp, bool irq);
 };
 
 /**
@@ -171,14 +180,15 @@ static int msg_enable;
  */
 
 /**
- * ks8851_wrreg16 - write 16bit register value to chip
+ * ks8851_wrreg16_spi - write 16bit register value to chip via SPI
  * @ks: The chip state
  * @reg: The register address
  * @val: The value to write
  *
  * Issue a write to put the value @val into the register specified in @reg.
  */
-static void ks8851_wrreg16(struct ks8851_net *ks, unsigned reg, unsigned val)
+static void ks8851_wrreg16_spi(struct ks8851_net *ks, unsigned int reg,
+			       unsigned int val)
 {
 	struct ks8851_net_spi *kss = to_ks8851_spi(ks);
 	struct spi_transfer *xfer = &kss->spi_xfer1;
@@ -198,6 +208,20 @@ static void ks8851_wrreg16(struct ks8851_net *ks, unsigned reg, unsigned val)
 		netdev_err(ks->netdev, "spi_sync() failed\n");
 }
 
+/**
+ * ks8851_wrreg16 - write 16bit register value to chip
+ * @ks: The chip state
+ * @reg: The register address
+ * @val: The value to write
+ *
+ * Issue a write to put the value @val into the register specified in @reg.
+ */
+static void ks8851_wrreg16(struct ks8851_net *ks, unsigned int reg,
+			   unsigned int val)
+{
+	ks->wrreg16(ks, reg, val);
+}
+
 /**
  * ks8851_rdreg - issue read register command and return the data
  * @ks: The device state
@@ -251,13 +275,14 @@ static void ks8851_rdreg(struct ks8851_net *ks, unsigned op,
 }
 
 /**
- * ks8851_rdreg16 - read 16 bit register from device
+ * ks8851_rdreg16_spi - read 16 bit register from device via SPI
  * @ks: The chip information
  * @reg: The register address
  *
  * Read a 16bit register from the chip, returning the result
 */
-static unsigned ks8851_rdreg16(struct ks8851_net *ks, unsigned reg)
+static unsigned int ks8851_rdreg16_spi(struct ks8851_net *ks,
+				       unsigned int reg)
 {
 	__le16 rx = 0;
 
@@ -265,6 +290,19 @@ static unsigned ks8851_rdreg16(struct ks8851_net *ks, unsigned reg)
 	return le16_to_cpu(rx);
 }
 
+/**
+ * ks8851_rdreg16 - read 16 bit register from device
+ * @ks: The chip information
+ * @reg: The register address
+ *
+ * Read a 16bit register from the chip, returning the result
+ */
+static unsigned int ks8851_rdreg16(struct ks8851_net *ks,
+				   unsigned int reg)
+{
+	return ks->rdreg16(ks, reg);
+}
+
 /**
  * ks8851_soft_reset - issue one of the soft reset to the device
  * @ks: The device state.
@@ -402,7 +440,7 @@ static void ks8851_init_mac(struct ks8851_net *ks, struct device *ddev)
 }
 
 /**
- * ks8851_rdfifo - read data from the receive fifo
+ * ks8851_rdfifo_spi - read data from the receive fifo via SPI
  * @ks: The device state.
  * @buff: The buffer address
  * @len: The length of the data to read
@@ -410,7 +448,8 @@ static void ks8851_init_mac(struct ks8851_net *ks, struct device *ddev)
  * Issue an RXQ FIFO read command and read the @len amount of data from
  * the FIFO into the buffer specified by @buff.
  */
-static void ks8851_rdfifo(struct ks8851_net *ks, u8 *buff, unsigned len)
+static void ks8851_rdfifo_spi(struct ks8851_net *ks, u8 *buff,
+			      unsigned int len)
 {
 	struct ks8851_net_spi *kss = to_ks8851_spi(ks);
 	struct spi_transfer *xfer = kss->spi_xfer2;
@@ -516,7 +555,7 @@ static void ks8851_rx_pkts(struct ks8851_net *ks)
 
 				rxpkt = skb_put(skb, rxlen) - 8;
 
-				ks8851_rdfifo(ks, rxpkt, rxalign + 8);
+				ks->rdfifo(ks, rxpkt, rxalign + 8);
 
 				if (netif_msg_pktdata(ks))
 					ks8851_dbg_dumpkkt(ks, rxpkt);
@@ -645,7 +684,7 @@ static inline unsigned calc_txlen(unsigned len)
 }
 
 /**
- * ks8851_wrpkt - write packet to TX FIFO
+ * ks8851_wrpkt_spi - write packet to TX FIFO via SPI
  * @ks: The device state.
  * @txp: The sk_buff to transmit.
  * @irq: IRQ on completion of the packet.
@@ -655,7 +694,8 @@ static inline unsigned calc_txlen(unsigned len)
  * needs, such as IRQ on completion. Send the header and the packet data to
  * the device.
  */
-static void ks8851_wrpkt(struct ks8851_net *ks, struct sk_buff *txp, bool irq)
+static void ks8851_wrpkt_spi(struct ks8851_net *ks, struct sk_buff *txp,
+			     bool irq)
 {
 	struct ks8851_net_spi *kss = to_ks8851_spi(ks);
 	struct spi_transfer *xfer = kss->spi_xfer2;
@@ -727,7 +767,7 @@ static void ks8851_tx_work(struct work_struct *work)
 
 		if (txb != NULL) {
 			ks8851_wrreg16(ks, KS_RXQCR, ks->rc_rxqcr | RXQCR_SDA);
-			ks8851_wrpkt(ks, txb, last);
+			ks->wrfifo(ks, txb, last);
 			ks8851_wrreg16(ks, KS_RXQCR, ks->rc_rxqcr);
 			ks8851_wrreg16(ks, KS_TXQCR, TXQCR_METFE);
 
@@ -1535,6 +1575,11 @@ static int ks8851_probe(struct spi_device *spi)
 	spi->bits_per_word = 8;
 
 	ks = netdev_priv(ndev);
+	ks->rdreg16 = ks8851_rdreg16_spi;
+	ks->wrreg16 = ks8851_wrreg16_spi;
+	ks->rdfifo = ks8851_rdfifo_spi;
+	ks->wrfifo = ks8851_wrpkt_spi;
+
 	kss = to_ks8851_spi(ks);
 
 	kss->spidev = spi;
-- 
2.25.1

Re: [PATCH 11/14] net: ks8851: Implement register and FIFO accessor callbacks

[Lukas Wunner]

On Tue, Mar 24, 2020 at 12:43:00AM +0100, Marek Vasut wrote:
> The register and FIFO accessors are bus specific. Implement callbacks so
> that each variant of the KS8851 can implement matching accessors and use
> the rest of the common code.
[...]
> +	unsigned int		(*rdreg16)(struct ks8851_net *ks,
> +					   unsigned int reg);
> +	void			(*wrreg16)(struct ks8851_net *ks,
> +					   unsigned int reg, unsigned int val);
> +	void			(*rdfifo)(struct ks8851_net *ks, u8 *buff,
> +					  unsigned int len);
> +	void			(*wrfifo)(struct ks8851_net *ks,
> +					  struct sk_buff *txp, bool irq);

Using callbacks entails a dereference for each invocation.

A cheaper approach is to just declare the function signatures
in ks8851.h and provide non-static implementations in
ks8851_spi.c and ks8851_mll.c, so I'd strongly prefer that.

Even better, since this only concerns the register accessors
(which are inlined anyway by the compiler), it would be best
to have them in header files (e.g. ks8851_spi.h / ks8851_par.h)
which are included by the common ks8851.c based on the target
which is being compiled.  That involves a bit of kbuild magic
though to generate two different .o from the same .c file,
each with specific "-include ..." CFLAGS.

Thanks,

Lukas

Re: [PATCH 11/14] net: ks8851: Implement register and FIFO accessor callbacks

[Marek Vasut]

On 3/24/20 2:45 PM, Lukas Wunner wrote:
> On Tue, Mar 24, 2020 at 12:43:00AM +0100, Marek Vasut wrote:
>> The register and FIFO accessors are bus specific. Implement callbacks so
>> that each variant of the KS8851 can implement matching accessors and use
>> the rest of the common code.
> [...]
>> +	unsigned int		(*rdreg16)(struct ks8851_net *ks,
>> +					   unsigned int reg);
>> +	void			(*wrreg16)(struct ks8851_net *ks,
>> +					   unsigned int reg, unsigned int val);
>> +	void			(*rdfifo)(struct ks8851_net *ks, u8 *buff,
>> +					  unsigned int len);
>> +	void			(*wrfifo)(struct ks8851_net *ks,
>> +					  struct sk_buff *txp, bool irq);
> 
> Using callbacks entails a dereference for each invocation.

Yes indeed, the SPI stack which you use to talk to the KS8851 SPI is
also full of those.

> A cheaper approach is to just declare the function signatures
> in ks8851.h and provide non-static implementations in
> ks8851_spi.c and ks8851_mll.c, so I'd strongly prefer that.
> 
> Even better, since this only concerns the register accessors
> (which are inlined anyway by the compiler), it would be best
> to have them in header files (e.g. ks8851_spi.h / ks8851_par.h)
> which are included by the common ks8851.c based on the target
> which is being compiled.  That involves a bit of kbuild magic
> though to generate two different .o from the same .c file,
> each with specific "-include ..." CFLAGS.

Before we go down the complex and ugly path, can you check whether this
actually has performance impact ? I would expect that since this is an
SPI-connected device, this here shouldn't have that much impact. But I
might be wrong, I don't have the hardware.

Also note that having this dereference in place, it permits me to easily
implement accessors for both LE and BE variant of the parallel bus device.

Re: [PATCH 11/14] net: ks8851: Implement register and FIFO accessor callbacks

[Lukas Wunner]

On Tue, Mar 24, 2020 at 03:10:59PM +0100, Marek Vasut wrote:
> On 3/24/20 2:45 PM, Lukas Wunner wrote:
> > On Tue, Mar 24, 2020 at 12:43:00AM +0100, Marek Vasut wrote:
> >> The register and FIFO accessors are bus specific. Implement callbacks so
> >> that each variant of the KS8851 can implement matching accessors and use
> >> the rest of the common code.
> > [...]
> >> +	unsigned int		(*rdreg16)(struct ks8851_net *ks,
> >> +					   unsigned int reg);
> >> +	void			(*wrreg16)(struct ks8851_net *ks,
> >> +					   unsigned int reg, unsigned int val);
> >> +	void			(*rdfifo)(struct ks8851_net *ks, u8 *buff,
> >> +					  unsigned int len);
> >> +	void			(*wrfifo)(struct ks8851_net *ks,
> >> +					  struct sk_buff *txp, bool irq);
> > 
> > Using callbacks entails a dereference for each invocation.
> 
> Yes indeed, the SPI stack which you use to talk to the KS8851 SPI is
> also full of those.

Apples and oranges.  Low-level SPI drivers provide callbacks to the
SPI core because it would be too expensive (space-wise) to link the
SPI core into every low-level driver.  Whereas in this case, you're
generating two separate modules anyway, so there's no need at all
to use callbacks.


> > A cheaper approach is to just declare the function signatures
> > in ks8851.h and provide non-static implementations in
> > ks8851_spi.c and ks8851_mll.c, so I'd strongly prefer that.
> > 
> > Even better, since this only concerns the register accessors
> > (which are inlined anyway by the compiler), it would be best
> > to have them in header files (e.g. ks8851_spi.h / ks8851_par.h)
> > which are included by the common ks8851.c based on the target
> > which is being compiled.  That involves a bit of kbuild magic
> > though to generate two different .o from the same .c file,
> > each with specific "-include ..." CFLAGS.
> 
> Before we go down the complex and ugly path, can you check whether this
> actually has performance impact ? I would expect that since this is an
> SPI-connected device, this here shouldn't have that much impact. But I
> might be wrong, I don't have the hardware.

I can test it, but the devices are in the office, I won't return there
before Thursday.  That said, I don't think it's a proper approach to
make the code more expensive even though it's perfectly possible to
avoid any performance impact, and shrug off concerns with the argument
that the impact should be measured first.


> Also note that having this dereference in place, it permits me to easily
> implement accessors for both LE and BE variant of the parallel bus device.

My understanding is that you're supposed to configure the chip to use
the native endianness of your architecture on ->probe() such that
conversions become unnecessary and the same accessor can be used for
LE and BE.  So why do you need two accessors?

Thanks,

Lukas

Re: [PATCH 11/14] net: ks8851: Implement register and FIFO accessor callbacks

[Marek Vasut]

On 3/24/20 3:29 PM, Lukas Wunner wrote:
> On Tue, Mar 24, 2020 at 03:10:59PM +0100, Marek Vasut wrote:
>> On 3/24/20 2:45 PM, Lukas Wunner wrote:
>>> On Tue, Mar 24, 2020 at 12:43:00AM +0100, Marek Vasut wrote:
>>>> The register and FIFO accessors are bus specific. Implement callbacks so
>>>> that each variant of the KS8851 can implement matching accessors and use
>>>> the rest of the common code.
>>> [...]
>>>> +	unsigned int		(*rdreg16)(struct ks8851_net *ks,
>>>> +					   unsigned int reg);
>>>> +	void			(*wrreg16)(struct ks8851_net *ks,
>>>> +					   unsigned int reg, unsigned int val);
>>>> +	void			(*rdfifo)(struct ks8851_net *ks, u8 *buff,
>>>> +					  unsigned int len);
>>>> +	void			(*wrfifo)(struct ks8851_net *ks,
>>>> +					  struct sk_buff *txp, bool irq);
>>>
>>> Using callbacks entails a dereference for each invocation.
>>
>> Yes indeed, the SPI stack which you use to talk to the KS8851 SPI is
>> also full of those.
> 
> Apples and oranges.  Low-level SPI drivers provide callbacks to the
> SPI core because it would be too expensive (space-wise) to link the
> SPI core into every low-level driver.  Whereas in this case, you're
> generating two separate modules anyway, so there's no need at all
> to use callbacks.
> 
> 
>>> A cheaper approach is to just declare the function signatures
>>> in ks8851.h and provide non-static implementations in
>>> ks8851_spi.c and ks8851_mll.c, so I'd strongly prefer that.
>>>
>>> Even better, since this only concerns the register accessors
>>> (which are inlined anyway by the compiler), it would be best
>>> to have them in header files (e.g. ks8851_spi.h / ks8851_par.h)
>>> which are included by the common ks8851.c based on the target
>>> which is being compiled.  That involves a bit of kbuild magic
>>> though to generate two different .o from the same .c file,
>>> each with specific "-include ..." CFLAGS.
>>
>> Before we go down the complex and ugly path, can you check whether this
>> actually has performance impact ? I would expect that since this is an
>> SPI-connected device, this here shouldn't have that much impact. But I
>> might be wrong, I don't have the hardware.
> 
> I can test it, but the devices are in the office, I won't return there
> before Thursday.  That said, I don't think it's a proper approach to
> make the code more expensive even though it's perfectly possible to
> avoid any performance impact, and shrug off concerns with the argument
> that the impact should be measured first.

I cannot measure the impact on the SPI device, but I would like to know
the numbers to see whether it's worth it all, before I start creating a
more complex solution.

Since the SPI bus is limited to 40 MHz per datasheet, I don't think the
pointer dereference is gonna introduce any performance problem.

I can at least try skipping the dereference on the parallel bus option
and see if it makes a difference.

>> Also note that having this dereference in place, it permits me to easily
>> implement accessors for both LE and BE variant of the parallel bus device.
> 
> My understanding is that you're supposed to configure the chip to use
> the native endianness of your architecture on ->probe() such that
> conversions become unnecessary and the same accessor can be used for
> LE and BE.  So why do you need two accessors?

Because I have a device here which is configured the "wrong" way thus far.

Re: [PATCH 11/14] net: ks8851: Implement register and FIFO accessor callbacks

[Marek Vasut]

On 3/24/20 2:45 PM, Lukas Wunner wrote:
> On Tue, Mar 24, 2020 at 12:43:00AM +0100, Marek Vasut wrote:
>> The register and FIFO accessors are bus specific. Implement callbacks so
>> that each variant of the KS8851 can implement matching accessors and use
>> the rest of the common code.
> [...]
>> +	unsigned int		(*rdreg16)(struct ks8851_net *ks,
>> +					   unsigned int reg);
>> +	void			(*wrreg16)(struct ks8851_net *ks,
>> +					   unsigned int reg, unsigned int val);
>> +	void			(*rdfifo)(struct ks8851_net *ks, u8 *buff,
>> +					  unsigned int len);
>> +	void			(*wrfifo)(struct ks8851_net *ks,
>> +					  struct sk_buff *txp, bool irq);
> 
> Using callbacks entails a dereference for each invocation.
> 
> A cheaper approach is to just declare the function signatures
> in ks8851.h and provide non-static implementations in
> ks8851_spi.c and ks8851_mll.c, so I'd strongly prefer that.
> 
> Even better, since this only concerns the register accessors
> (which are inlined anyway by the compiler), it would be best
> to have them in header files (e.g. ks8851_spi.h / ks8851_par.h)
> which are included by the common ks8851.c based on the target
> which is being compiled.  That involves a bit of kbuild magic
> though to generate two different .o from the same .c file,
> each with specific "-include ..." CFLAGS.

Seems this also fails when both options are compiled in, then there is a
symbol name collision. Thoughts ?

[PATCH 12/14] net: ks8851: Separate SPI operations into separate file

[Marek Vasut]

Pull all the SPI bus specific code into a separate file, so that it is
not mixed with the common code. No functional change.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: David S. Miller <davem@davemloft.net>
Cc: Lukas Wunner <lukas@wunner.de>
Cc: Petr Stetiar <ynezz@true.cz>
Cc: YueHaibing <yuehaibing@huawei.com>
---
 drivers/net/ethernet/micrel/Makefile     |   2 +-
 drivers/net/ethernet/micrel/ks8851.c     | 404 +----------------------
 drivers/net/ethernet/micrel/ks8851.h     | 123 +++++++
 drivers/net/ethernet/micrel/ks8851_spi.c | 311 +++++++++++++++++
 4 files changed, 439 insertions(+), 401 deletions(-)
 create mode 100644 drivers/net/ethernet/micrel/ks8851_spi.c

diff --git a/drivers/net/ethernet/micrel/Makefile b/drivers/net/ethernet/micrel/Makefile
index 6d8ac5527aef..bb5c6c53fa02 100644
--- a/drivers/net/ethernet/micrel/Makefile
+++ b/drivers/net/ethernet/micrel/Makefile
@@ -4,6 +4,6 @@
 #
 
 obj-$(CONFIG_KS8842) += ks8842.o
-obj-$(CONFIG_KS8851) += ks8851.o
+obj-$(CONFIG_KS8851) += ks8851.o ks8851_spi.o
 obj-$(CONFIG_KS8851_MLL) += ks8851_mll.o
 obj-$(CONFIG_KSZ884X_PCI) += ksz884x.o
diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
index d5bdbd9122bf..dd8d8a88c7e9 100644
--- a/drivers/net/ethernet/micrel/ks8851.c
+++ b/drivers/net/ethernet/micrel/ks8851.c
@@ -22,192 +22,14 @@
 #include <linux/eeprom_93cx6.h>
 #include <linux/regulator/consumer.h>
 
-#include <linux/spi/spi.h>
 #include <linux/gpio.h>
 #include <linux/of_gpio.h>
 #include <linux/of_net.h>
 
 #include "ks8851.h"
 
-/**
- * struct ks8851_rxctrl - KS8851 driver rx control
- * @mchash: Multicast hash-table data.
- * @rxcr1: KS_RXCR1 register setting
- * @rxcr2: KS_RXCR2 register setting
- *
- * Representation of the settings needs to control the receive filtering
- * such as the multicast hash-filter and the receive register settings. This
- * is used to make the job of working out if the receive settings change and
- * then issuing the new settings to the worker that will send the necessary
- * commands.
- */
-struct ks8851_rxctrl {
-	u16	mchash[4];
-	u16	rxcr1;
-	u16	rxcr2;
-};
-
-/**
- * union ks8851_tx_hdr - tx header data
- * @txb: The header as bytes
- * @txw: The header as 16bit, little-endian words
- *
- * A dual representation of the tx header data to allow
- * access to individual bytes, and to allow 16bit accesses
- * with 16bit alignment.
- */
-union ks8851_tx_hdr {
-	u8	txb[6];
-	__le16	txw[3];
-};
-
-/**
- * struct ks8851_net - KS8851 driver private data
- * @netdev: The network device we're bound to
- * @spidev: The spi device we're bound to.
- * @lock: Lock to ensure that the device is not accessed when busy.
- * @statelock: Lock on this structure for tx list.
- * @mii: The MII state information for the mii calls.
- * @rxctrl: RX settings for @rxctrl_work.
- * @tx_work: Work queue for tx packets
- * @rxctrl_work: Work queue for updating RX mode and multicast lists
- * @txq: Queue of packets for transmission.
- * @spi_msg1: pre-setup SPI transfer with one message, @spi_xfer1.
- * @spi_msg2: pre-setup SPI transfer with two messages, @spi_xfer2.
- * @txh: Space for generating packet TX header in DMA-able data
- * @rxd: Space for receiving SPI data, in DMA-able space.
- * @txd: Space for transmitting SPI data, in DMA-able space.
- * @msg_enable: The message flags controlling driver output (see ethtool).
- * @fid: Incrementing frame id tag.
- * @rc_ier: Cached copy of KS_IER.
- * @rc_ccr: Cached copy of KS_CCR.
- * @rc_rxqcr: Cached copy of KS_RXQCR.
- * @eeprom: 93CX6 EEPROM state for accessing on-board EEPROM.
- * @vdd_reg:	Optional regulator supplying the chip
- * @vdd_io: Optional digital power supply for IO
- * @gpio: Optional reset_n gpio
- *
- * The @lock ensures that the chip is protected when certain operations are
- * in progress. When the read or write packet transfer is in progress, most
- * of the chip registers are not ccessible until the transfer is finished and
- * the DMA has been de-asserted.
- *
- * The @statelock is used to protect information in the structure which may
- * need to be accessed via several sources, such as the network driver layer
- * or one of the work queues.
- *
- * We align the buffers we may use for rx/tx to ensure that if the SPI driver
- * wants to DMA map them, it will not have any problems with data the driver
- * modifies.
- */
-struct ks8851_net {
-	struct net_device	*netdev;
-	struct mutex		lock;
-	spinlock_t		statelock;
-
-	union ks8851_tx_hdr	txh ____cacheline_aligned;
-	u8			rxd[8];
-	u8			txd[8];
-
-	u32			msg_enable ____cacheline_aligned;
-	u16			tx_space;
-	u8			fid;
-
-	u16			rc_ier;
-	u16			rc_rxqcr;
-	u16			rc_ccr;
-
-	struct mii_if_info	mii;
-	struct ks8851_rxctrl	rxctrl;
-
-	struct work_struct	tx_work;
-	struct work_struct	rxctrl_work;
-
-	struct sk_buff_head	txq;
-
-	struct eeprom_93cx6	eeprom;
-	struct regulator	*vdd_reg;
-	struct regulator	*vdd_io;
-	int			gpio;
-
-	unsigned int		(*rdreg16)(struct ks8851_net *ks,
-					   unsigned int reg);
-	void			(*wrreg16)(struct ks8851_net *ks,
-					   unsigned int reg, unsigned int val);
-	void			(*rdfifo)(struct ks8851_net *ks, u8 *buff,
-					  unsigned int len);
-	void			(*wrfifo)(struct ks8851_net *ks,
-					  struct sk_buff *txp, bool irq);
-};
-
-/**
- * struct ks8851_net_spi - KS8851 SPI driver private data
- * @ks8851: KS8851 driver common private data
- * @spidev: The spi device we're bound to.
- * @spi_msg1: pre-setup SPI transfer with one message, @spi_xfer1.
- * @spi_msg2: pre-setup SPI transfer with two messages, @spi_xfer2.
- */
-struct ks8851_net_spi {
-	struct ks8851_net	ks8851;	/* Must be first */
-	struct spi_device	*spidev;
-	struct spi_message	spi_msg1;
-	struct spi_message	spi_msg2;
-	struct spi_transfer	spi_xfer1;
-	struct spi_transfer	spi_xfer2[2];
-};
-
-#define to_ks8851_spi(ks) container_of((ks), struct ks8851_net_spi, ks8851)
-
 static int msg_enable;
 
-/* SPI frame opcodes */
-#define KS_SPIOP_RD	(0x00)
-#define KS_SPIOP_WR	(0x40)
-#define KS_SPIOP_RXFIFO	(0x80)
-#define KS_SPIOP_TXFIFO	(0xC0)
-
-/* shift for byte-enable data */
-#define BYTE_EN(_x)	((_x) << 2)
-
-/* turn register number and byte-enable mask into data for start of packet */
-#define MK_OP(_byteen, _reg) (BYTE_EN(_byteen) | (_reg)  << (8+2) | (_reg) >> 6)
-
-/* SPI register read/write calls.
- *
- * All these calls issue SPI transactions to access the chip's registers. They
- * all require that the necessary lock is held to prevent accesses when the
- * chip is busy transferring packet data (RX/TX FIFO accesses).
- */
-
-/**
- * ks8851_wrreg16_spi - write 16bit register value to chip via SPI
- * @ks: The chip state
- * @reg: The register address
- * @val: The value to write
- *
- * Issue a write to put the value @val into the register specified in @reg.
- */
-static void ks8851_wrreg16_spi(struct ks8851_net *ks, unsigned int reg,
-			       unsigned int val)
-{
-	struct ks8851_net_spi *kss = to_ks8851_spi(ks);
-	struct spi_transfer *xfer = &kss->spi_xfer1;
-	struct spi_message *msg = &kss->spi_msg1;
-	__le16 txb[2];
-	int ret;
-
-	txb[0] = cpu_to_le16(MK_OP(reg & 2 ? 0xC : 0x03, reg) | KS_SPIOP_WR);
-	txb[1] = cpu_to_le16(val);
-
-	xfer->tx_buf = txb;
-	xfer->rx_buf = NULL;
-	xfer->len = 4;
-
-	ret = spi_sync(kss->spidev, msg);
-	if (ret < 0)
-		netdev_err(ks->netdev, "spi_sync() failed\n");
-}
-
 /**
  * ks8851_wrreg16 - write 16bit register value to chip
  * @ks: The chip state
@@ -222,74 +44,6 @@ static void ks8851_wrreg16(struct ks8851_net *ks, unsigned int reg,
 	ks->wrreg16(ks, reg, val);
 }
 
-/**
- * ks8851_rdreg - issue read register command and return the data
- * @ks: The device state
- * @op: The register address and byte enables in message format.
- * @rxb: The RX buffer to return the result into
- * @rxl: The length of data expected.
- *
- * This is the low level read call that issues the necessary spi message(s)
- * to read data from the register specified in @op.
- */
-static void ks8851_rdreg(struct ks8851_net *ks, unsigned op,
-			 u8 *rxb, unsigned rxl)
-{
-	struct ks8851_net_spi *kss = to_ks8851_spi(ks);
-	struct spi_transfer *xfer;
-	struct spi_message *msg;
-	__le16 *txb = (__le16 *)ks->txd;
-	u8 *trx = ks->rxd;
-	int ret;
-
-	txb[0] = cpu_to_le16(op | KS_SPIOP_RD);
-
-	if (kss->spidev->master->flags & SPI_MASTER_HALF_DUPLEX) {
-		msg = &kss->spi_msg2;
-		xfer = kss->spi_xfer2;
-
-		xfer->tx_buf = txb;
-		xfer->rx_buf = NULL;
-		xfer->len = 2;
-
-		xfer++;
-		xfer->tx_buf = NULL;
-		xfer->rx_buf = trx;
-		xfer->len = rxl;
-	} else {
-		msg = &kss->spi_msg1;
-		xfer = &kss->spi_xfer1;
-
-		xfer->tx_buf = txb;
-		xfer->rx_buf = trx;
-		xfer->len = rxl + 2;
-	}
-
-	ret = spi_sync(kss->spidev, msg);
-	if (ret < 0)
-		netdev_err(ks->netdev, "read: spi_sync() failed\n");
-	else if (kss->spidev->master->flags & SPI_MASTER_HALF_DUPLEX)
-		memcpy(rxb, trx, rxl);
-	else
-		memcpy(rxb, trx + 2, rxl);
-}
-
-/**
- * ks8851_rdreg16_spi - read 16 bit register from device via SPI
- * @ks: The chip information
- * @reg: The register address
- *
- * Read a 16bit register from the chip, returning the result
-*/
-static unsigned int ks8851_rdreg16_spi(struct ks8851_net *ks,
-				       unsigned int reg)
-{
-	__le16 rx = 0;
-
-	ks8851_rdreg(ks, MK_OP(reg & 2 ? 0xC : 0x3, reg), (u8 *)&rx, 2);
-	return le16_to_cpu(rx);
-}
-
 /**
  * ks8851_rdreg16 - read 16 bit register from device
  * @ks: The chip information
@@ -439,44 +193,6 @@ static void ks8851_init_mac(struct ks8851_net *ks, struct device *ddev)
 	ks8851_write_mac_addr(dev);
 }
 
-/**
- * ks8851_rdfifo_spi - read data from the receive fifo via SPI
- * @ks: The device state.
- * @buff: The buffer address
- * @len: The length of the data to read
- *
- * Issue an RXQ FIFO read command and read the @len amount of data from
- * the FIFO into the buffer specified by @buff.
- */
-static void ks8851_rdfifo_spi(struct ks8851_net *ks, u8 *buff,
-			      unsigned int len)
-{
-	struct ks8851_net_spi *kss = to_ks8851_spi(ks);
-	struct spi_transfer *xfer = kss->spi_xfer2;
-	struct spi_message *msg = &kss->spi_msg2;
-	u8 txb[1];
-	int ret;
-
-	netif_dbg(ks, rx_status, ks->netdev,
-		  "%s: %d@%p\n", __func__, len, buff);
-
-	/* set the operation we're issuing */
-	txb[0] = KS_SPIOP_RXFIFO;
-
-	xfer->tx_buf = txb;
-	xfer->rx_buf = NULL;
-	xfer->len = 1;
-
-	xfer++;
-	xfer->rx_buf = buff;
-	xfer->tx_buf = NULL;
-	xfer->len = len;
-
-	ret = spi_sync(kss->spidev, msg);
-	if (ret < 0)
-		netdev_err(ks->netdev, "%s: spi_sync() failed\n", __func__);
-}
-
 /**
  * ks8851_dbg_dumpkkt - dump initial packet contents to debug
  * @ks: The device state
@@ -683,54 +399,6 @@ static inline unsigned calc_txlen(unsigned len)
 	return ALIGN(len + 4, 4);
 }
 
-/**
- * ks8851_wrpkt_spi - write packet to TX FIFO via SPI
- * @ks: The device state.
- * @txp: The sk_buff to transmit.
- * @irq: IRQ on completion of the packet.
- *
- * Send the @txp to the chip. This means creating the relevant packet header
- * specifying the length of the packet and the other information the chip
- * needs, such as IRQ on completion. Send the header and the packet data to
- * the device.
- */
-static void ks8851_wrpkt_spi(struct ks8851_net *ks, struct sk_buff *txp,
-			     bool irq)
-{
-	struct ks8851_net_spi *kss = to_ks8851_spi(ks);
-	struct spi_transfer *xfer = kss->spi_xfer2;
-	struct spi_message *msg = &kss->spi_msg2;
-	unsigned fid = 0;
-	int ret;
-
-	netif_dbg(ks, tx_queued, ks->netdev, "%s: skb %p, %d@%p, irq %d\n",
-		  __func__, txp, txp->len, txp->data, irq);
-
-	fid = ks->fid++;
-	fid &= TXFR_TXFID_MASK;
-
-	if (irq)
-		fid |= TXFR_TXIC;	/* irq on completion */
-
-	/* start header at txb[1] to align txw entries */
-	ks->txh.txb[1] = KS_SPIOP_TXFIFO;
-	ks->txh.txw[1] = cpu_to_le16(fid);
-	ks->txh.txw[2] = cpu_to_le16(txp->len);
-
-	xfer->tx_buf = &ks->txh.txb[1];
-	xfer->rx_buf = NULL;
-	xfer->len = 5;
-
-	xfer++;
-	xfer->tx_buf = txp->data;
-	xfer->rx_buf = NULL;
-	xfer->len = ALIGN(txp->len, 4);
-
-	ret = spi_sync(kss->spidev, msg);
-	if (ret < 0)
-		netdev_err(ks->netdev, "%s: spi_sync() failed\n", __func__);
-}
-
 /**
  * ks8851_done_tx - update and then free skbuff after transmitting
  * @ks: The device state
@@ -1382,7 +1050,7 @@ static int ks8851_read_selftest(struct ks8851_net *ks)
 
 #ifdef CONFIG_PM_SLEEP
 
-static int ks8851_suspend(struct device *dev)
+int ks8851_suspend(struct device *dev)
 {
 	struct ks8851_net *ks = dev_get_drvdata(dev);
 	struct net_device *netdev = ks->netdev;
@@ -1395,7 +1063,7 @@ static int ks8851_suspend(struct device *dev)
 	return 0;
 }
 
-static int ks8851_resume(struct device *dev)
+int ks8851_resume(struct device *dev)
 {
 	struct ks8851_net *ks = dev_get_drvdata(dev);
 	struct net_device *netdev = ks->netdev;
@@ -1409,9 +1077,7 @@ static int ks8851_resume(struct device *dev)
 }
 #endif
 
-static SIMPLE_DEV_PM_OPS(ks8851_pm_ops, ks8851_suspend, ks8851_resume);
-
-static int ks8851_probe_common(struct net_device *ndev, struct device *dev)
+int ks8851_probe_common(struct net_device *ndev, struct device *dev)
 {
 	struct ks8851_net *ks = netdev_priv(ndev);
 	unsigned cider;
@@ -1545,7 +1211,7 @@ static int ks8851_probe_common(struct net_device *ndev, struct device *dev)
 	return ret;
 }
 
-static int ks8851_remove_common(struct device *dev)
+int ks8851_remove_common(struct device *dev)
 {
 	struct ks8851_net *priv = dev_get_drvdata(dev);
 
@@ -1561,68 +1227,6 @@ static int ks8851_remove_common(struct device *dev)
 	return 0;
 }
 
-static int ks8851_probe(struct spi_device *spi)
-{
-	struct device *dev = &spi->dev;
-	struct ks8851_net_spi *kss;
-	struct net_device *ndev;
-	struct ks8851_net *ks;
-
-	ndev = devm_alloc_etherdev(dev, sizeof(struct ks8851_net_spi));
-	if (!ndev)
-		return -ENOMEM;
-
-	spi->bits_per_word = 8;
-
-	ks = netdev_priv(ndev);
-	ks->rdreg16 = ks8851_rdreg16_spi;
-	ks->wrreg16 = ks8851_wrreg16_spi;
-	ks->rdfifo = ks8851_rdfifo_spi;
-	ks->wrfifo = ks8851_wrpkt_spi;
-
-	kss = to_ks8851_spi(ks);
-
-	kss->spidev = spi;
-
-	/* initialise pre-made spi transfer messages */
-	spi_message_init(&kss->spi_msg1);
-	spi_message_add_tail(&kss->spi_xfer1, &kss->spi_msg1);
-
-	spi_message_init(&kss->spi_msg2);
-	spi_message_add_tail(&kss->spi_xfer2[0], &kss->spi_msg2);
-	spi_message_add_tail(&kss->spi_xfer2[1], &kss->spi_msg2);
-
-	ndev->irq = spi->irq;
-
-	return ks8851_probe_common(ndev, dev);
-}
-
-static int ks8851_remove(struct spi_device *spi)
-{
-	return ks8851_remove_common(&spi->dev);
-}
-
-static const struct of_device_id ks8851_match_table[] = {
-	{ .compatible = "micrel,ks8851" },
-	{ }
-};
-MODULE_DEVICE_TABLE(of, ks8851_match_table);
-
-static struct spi_driver ks8851_driver = {
-	.driver = {
-		.name = "ks8851",
-		.of_match_table = ks8851_match_table,
-		.pm = &ks8851_pm_ops,
-	},
-	.probe = ks8851_probe,
-	.remove = ks8851_remove,
-};
-module_spi_driver(ks8851_driver);
-
-MODULE_DESCRIPTION("KS8851 Network driver");
-MODULE_AUTHOR("Ben Dooks <ben@simtec.co.uk>");
-MODULE_LICENSE("GPL");
-
 module_param_named(message, msg_enable, int, 0);
 MODULE_PARM_DESC(message, "Message verbosity level (0=none, 31=all)");
 MODULE_ALIAS("spi:ks8851");
diff --git a/drivers/net/ethernet/micrel/ks8851.h b/drivers/net/ethernet/micrel/ks8851.h
index 8f834aef8e32..8b2acbc06f15 100644
--- a/drivers/net/ethernet/micrel/ks8851.h
+++ b/drivers/net/ethernet/micrel/ks8851.h
@@ -7,6 +7,9 @@
  * KS8851 register definitions
 */
 
+#ifndef __KS8851_H__
+#define __KS8851_H__
+
 #define KS_CCR					0x08
 #define CCR_LE					(1 << 10)   /* KSZ8851-16MLL */
 #define CCR_EEPROM				(1 << 9)
@@ -300,3 +303,123 @@
 #define TXFR_TXIC				(1 << 15)
 #define TXFR_TXFID_MASK				(0x3f << 0)
 #define TXFR_TXFID_SHIFT			(0)
+
+/**
+ * struct ks8851_rxctrl - KS8851 driver rx control
+ * @mchash: Multicast hash-table data.
+ * @rxcr1: KS_RXCR1 register setting
+ * @rxcr2: KS_RXCR2 register setting
+ *
+ * Representation of the settings needs to control the receive filtering
+ * such as the multicast hash-filter and the receive register settings. This
+ * is used to make the job of working out if the receive settings change and
+ * then issuing the new settings to the worker that will send the necessary
+ * commands.
+ */
+struct ks8851_rxctrl {
+	u16	mchash[4];
+	u16	rxcr1;
+	u16	rxcr2;
+};
+
+/**
+ * union ks8851_tx_hdr - tx header data
+ * @txb: The header as bytes
+ * @txw: The header as 16bit, little-endian words
+ *
+ * A dual representation of the tx header data to allow
+ * access to individual bytes, and to allow 16bit accesses
+ * with 16bit alignment.
+ */
+union ks8851_tx_hdr {
+	u8	txb[6];
+	__le16	txw[3];
+};
+
+/**
+ * struct ks8851_net - KS8851 driver private data
+ * @netdev: The network device we're bound to
+ * @spidev: The spi device we're bound to.
+ * @lock: Lock to ensure that the device is not accessed when busy.
+ * @statelock: Lock on this structure for tx list.
+ * @mii: The MII state information for the mii calls.
+ * @rxctrl: RX settings for @rxctrl_work.
+ * @tx_work: Work queue for tx packets
+ * @rxctrl_work: Work queue for updating RX mode and multicast lists
+ * @txq: Queue of packets for transmission.
+ * @spi_msg1: pre-setup SPI transfer with one message, @spi_xfer1.
+ * @spi_msg2: pre-setup SPI transfer with two messages, @spi_xfer2.
+ * @txh: Space for generating packet TX header in DMA-able data
+ * @rxd: Space for receiving SPI data, in DMA-able space.
+ * @txd: Space for transmitting SPI data, in DMA-able space.
+ * @msg_enable: The message flags controlling driver output (see ethtool).
+ * @fid: Incrementing frame id tag.
+ * @rc_ier: Cached copy of KS_IER.
+ * @rc_ccr: Cached copy of KS_CCR.
+ * @rc_rxqcr: Cached copy of KS_RXQCR.
+ * @eeprom: 93CX6 EEPROM state for accessing on-board EEPROM.
+ * @vdd_reg:	Optional regulator supplying the chip
+ * @vdd_io: Optional digital power supply for IO
+ * @gpio: Optional reset_n gpio
+ *
+ * The @lock ensures that the chip is protected when certain operations are
+ * in progress. When the read or write packet transfer is in progress, most
+ * of the chip registers are not ccessible until the transfer is finished and
+ * the DMA has been de-asserted.
+ *
+ * The @statelock is used to protect information in the structure which may
+ * need to be accessed via several sources, such as the network driver layer
+ * or one of the work queues.
+ *
+ * We align the buffers we may use for rx/tx to ensure that if the SPI driver
+ * wants to DMA map them, it will not have any problems with data the driver
+ * modifies.
+ */
+struct ks8851_net {
+	struct net_device	*netdev;
+	struct mutex		lock;
+	spinlock_t		statelock;
+
+	union ks8851_tx_hdr	txh ____cacheline_aligned;
+	u8			rxd[8];
+	u8			txd[8];
+
+	u32			msg_enable ____cacheline_aligned;
+	u16			tx_space;
+	u8			fid;
+
+	u16			rc_ier;
+	u16			rc_rxqcr;
+	u16			rc_ccr;
+
+	struct mii_if_info	mii;
+	struct ks8851_rxctrl	rxctrl;
+
+	struct work_struct	tx_work;
+	struct work_struct	rxctrl_work;
+
+	struct sk_buff_head	txq;
+
+	struct eeprom_93cx6	eeprom;
+	struct regulator	*vdd_reg;
+	struct regulator	*vdd_io;
+	int			gpio;
+
+	unsigned int		(*rdreg16)(struct ks8851_net *ks,
+					   unsigned int reg);
+	void			(*wrreg16)(struct ks8851_net *ks,
+					   unsigned int reg, unsigned int val);
+	void			(*rdfifo)(struct ks8851_net *ks, u8 *buff,
+					  unsigned int len);
+	void			(*wrfifo)(struct ks8851_net *ks,
+					  struct sk_buff *txp, bool irq);
+};
+
+int ks8851_probe_common(struct net_device *ndev, struct device *dev);
+int ks8851_remove_common(struct device *dev);
+int ks8851_suspend(struct device *dev);
+int ks8851_resume(struct device *dev);
+
+static SIMPLE_DEV_PM_OPS(ks8851_pm_ops, ks8851_suspend, ks8851_resume);
+
+#endif
diff --git a/drivers/net/ethernet/micrel/ks8851_spi.c b/drivers/net/ethernet/micrel/ks8851_spi.c
new file mode 100644
index 000000000000..07f1b8f09fc0
--- /dev/null
+++ b/drivers/net/ethernet/micrel/ks8851_spi.c
@@ -0,0 +1,311 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* drivers/net/ethernet/micrel/ks8851.c
+ *
+ * Copyright 2009 Simtec Electronics
+ *	http://www.simtec.co.uk/
+ *	Ben Dooks <ben@simtec.co.uk>
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#define DEBUG
+
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/netdevice.h>
+#include <linux/etherdevice.h>
+#include <linux/ethtool.h>
+#include <linux/cache.h>
+#include <linux/crc32.h>
+#include <linux/mii.h>
+#include <linux/eeprom_93cx6.h>
+#include <linux/regulator/consumer.h>
+
+#include <linux/spi/spi.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+#include <linux/of_net.h>
+
+#include "ks8851.h"
+
+/**
+ * struct ks8851_net_spi - KS8851 SPI driver private data
+ * @ks8851: KS8851 driver common private data
+ * @spidev: The spi device we're bound to.
+ * @spi_msg1: pre-setup SPI transfer with one message, @spi_xfer1.
+ * @spi_msg2: pre-setup SPI transfer with two messages, @spi_xfer2.
+ */
+struct ks8851_net_spi {
+	struct ks8851_net	ks8851;	/* Must be first */
+	struct spi_device	*spidev;
+	struct spi_message	spi_msg1;
+	struct spi_message	spi_msg2;
+	struct spi_transfer	spi_xfer1;
+	struct spi_transfer	spi_xfer2[2];
+};
+
+#define to_ks8851_spi(ks) container_of((ks), struct ks8851_net_spi, ks8851)
+
+/* SPI frame opcodes */
+#define KS_SPIOP_RD	0x00
+#define KS_SPIOP_WR	0x40
+#define KS_SPIOP_RXFIFO	0x80
+#define KS_SPIOP_TXFIFO	0xC0
+
+/* shift for byte-enable data */
+#define BYTE_EN(_x)	((_x) << 2)
+
+/* turn register number and byte-enable mask into data for start of packet */
+#define MK_OP(_byteen, _reg)	\
+	(BYTE_EN(_byteen) | (_reg) << (8 + 2) | (_reg) >> 6)
+
+/* SPI register read/write calls.
+ *
+ * All these calls issue SPI transactions to access the chip's registers. They
+ * all require that the necessary lock is held to prevent accesses when the
+ * chip is busy transferring packet data (RX/TX FIFO accesses).
+ */
+
+/**
+ * ks8851_wrreg16_spi - write 16bit register value to chip via SPI
+ * @ks: The chip state
+ * @reg: The register address
+ * @val: The value to write
+ *
+ * Issue a write to put the value @val into the register specified in @reg.
+ */
+static void ks8851_wrreg16_spi(struct ks8851_net *ks, unsigned int reg,
+			       unsigned int val)
+{
+	struct ks8851_net_spi *kss = to_ks8851_spi(ks);
+	struct spi_transfer *xfer = &kss->spi_xfer1;
+	struct spi_message *msg = &kss->spi_msg1;
+	__le16 txb[2];
+	int ret;
+
+	txb[0] = cpu_to_le16(MK_OP(reg & 2 ? 0xC : 0x03, reg) | KS_SPIOP_WR);
+	txb[1] = cpu_to_le16(val);
+
+	xfer->tx_buf = txb;
+	xfer->rx_buf = NULL;
+	xfer->len = 4;
+
+	ret = spi_sync(kss->spidev, msg);
+	if (ret < 0)
+		netdev_err(ks->netdev, "spi_sync() failed\n");
+}
+
+/**
+ * ks8851_rdreg - issue read register command and return the data
+ * @ks: The device state
+ * @op: The register address and byte enables in message format.
+ * @rxb: The RX buffer to return the result into
+ * @rxl: The length of data expected.
+ *
+ * This is the low level read call that issues the necessary spi message(s)
+ * to read data from the register specified in @op.
+ */
+static void ks8851_rdreg(struct ks8851_net *ks, unsigned int op,
+			 u8 *rxb, unsigned int rxl)
+{
+	struct ks8851_net_spi *kss = to_ks8851_spi(ks);
+	struct spi_transfer *xfer;
+	struct spi_message *msg;
+	__le16 *txb = (__le16 *)ks->txd;
+	u8 *trx = ks->rxd;
+	int ret;
+
+	txb[0] = cpu_to_le16(op | KS_SPIOP_RD);
+
+	if (kss->spidev->master->flags & SPI_MASTER_HALF_DUPLEX) {
+		msg = &kss->spi_msg2;
+		xfer = kss->spi_xfer2;
+
+		xfer->tx_buf = txb;
+		xfer->rx_buf = NULL;
+		xfer->len = 2;
+
+		xfer++;
+		xfer->tx_buf = NULL;
+		xfer->rx_buf = trx;
+		xfer->len = rxl;
+	} else {
+		msg = &kss->spi_msg1;
+		xfer = &kss->spi_xfer1;
+
+		xfer->tx_buf = txb;
+		xfer->rx_buf = trx;
+		xfer->len = rxl + 2;
+	}
+
+	ret = spi_sync(kss->spidev, msg);
+	if (ret < 0)
+		netdev_err(ks->netdev, "read: spi_sync() failed\n");
+	else if (kss->spidev->master->flags & SPI_MASTER_HALF_DUPLEX)
+		memcpy(rxb, trx, rxl);
+	else
+		memcpy(rxb, trx + 2, rxl);
+}
+
+/**
+ * ks8851_rdreg16_spi - read 16 bit register from device via SPI
+ * @ks: The chip information
+ * @reg: The register address
+ *
+ * Read a 16bit register from the chip, returning the result
+ */
+static unsigned int ks8851_rdreg16_spi(struct ks8851_net *ks, unsigned int reg)
+{
+	__le16 rx = 0;
+
+	ks8851_rdreg(ks, MK_OP(reg & 2 ? 0xC : 0x3, reg), (u8 *)&rx, 2);
+	return le16_to_cpu(rx);
+}
+
+/**
+ * ks8851_rdfifo_spi - read data from the receive fifo via SPI
+ * @ks: The device state.
+ * @buff: The buffer address
+ * @len: The length of the data to read
+ *
+ * Issue an RXQ FIFO read command and read the @len amount of data from
+ * the FIFO into the buffer specified by @buff.
+ */
+static void ks8851_rdfifo_spi(struct ks8851_net *ks, u8 *buff, unsigned int len)
+{
+	struct ks8851_net_spi *kss = to_ks8851_spi(ks);
+	struct spi_transfer *xfer = kss->spi_xfer2;
+	struct spi_message *msg = &kss->spi_msg2;
+	u8 txb[1];
+	int ret;
+
+	netif_dbg(ks, rx_status, ks->netdev,
+		  "%s: %d@%p\n", __func__, len, buff);
+
+	/* set the operation we're issuing */
+	txb[0] = KS_SPIOP_RXFIFO;
+
+	xfer->tx_buf = txb;
+	xfer->rx_buf = NULL;
+	xfer->len = 1;
+
+	xfer++;
+	xfer->rx_buf = buff;
+	xfer->tx_buf = NULL;
+	xfer->len = len;
+
+	ret = spi_sync(kss->spidev, msg);
+	if (ret < 0)
+		netdev_err(ks->netdev, "%s: spi_sync() failed\n", __func__);
+}
+
+/**
+ * ks8851_wrpkt_spi - write packet to TX FIFO via SPI
+ * @ks: The device state.
+ * @txp: The sk_buff to transmit.
+ * @irq: IRQ on completion of the packet.
+ *
+ * Send the @txp to the chip. This means creating the relevant packet header
+ * specifying the length of the packet and the other information the chip
+ * needs, such as IRQ on completion. Send the header and the packet data to
+ * the device.
+ */
+static void ks8851_wrpkt_spi(struct ks8851_net *ks, struct sk_buff *txp,
+			     bool irq)
+{
+	struct ks8851_net_spi *kss = to_ks8851_spi(ks);
+	struct spi_transfer *xfer = kss->spi_xfer2;
+	struct spi_message *msg = &kss->spi_msg2;
+	unsigned int fid = 0;
+	int ret;
+
+	netif_dbg(ks, tx_queued, ks->netdev, "%s: skb %p, %d@%p, irq %d\n",
+		  __func__, txp, txp->len, txp->data, irq);
+
+	fid = ks->fid++;
+	fid &= TXFR_TXFID_MASK;
+
+	if (irq)
+		fid |= TXFR_TXIC;	/* irq on completion */
+
+	/* start header at txb[1] to align txw entries */
+	ks->txh.txb[1] = KS_SPIOP_TXFIFO;
+	ks->txh.txw[1] = cpu_to_le16(fid);
+	ks->txh.txw[2] = cpu_to_le16(txp->len);
+
+	xfer->tx_buf = &ks->txh.txb[1];
+	xfer->rx_buf = NULL;
+	xfer->len = 5;
+
+	xfer++;
+	xfer->tx_buf = txp->data;
+	xfer->rx_buf = NULL;
+	xfer->len = ALIGN(txp->len, 4);
+
+	ret = spi_sync(kss->spidev, msg);
+	if (ret < 0)
+		netdev_err(ks->netdev, "%s: spi_sync() failed\n", __func__);
+}
+
+static int ks8851_probe_spi(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct ks8851_net_spi *kss;
+	struct net_device *ndev;
+	struct ks8851_net *ks;
+
+	ndev = devm_alloc_etherdev(dev, sizeof(struct ks8851_net_spi));
+	if (!ndev)
+		return -ENOMEM;
+
+	spi->bits_per_word = 8;
+
+	ks = netdev_priv(ndev);
+	ks->rdreg16 = ks8851_rdreg16_spi;
+	ks->wrreg16 = ks8851_wrreg16_spi;
+	ks->rdfifo = ks8851_rdfifo_spi;
+	ks->wrfifo = ks8851_wrpkt_spi;
+
+	kss = to_ks8851_spi(ks);
+
+	kss->spidev = spi;
+
+	/* initialise pre-made spi transfer messages */
+	spi_message_init(&kss->spi_msg1);
+	spi_message_add_tail(&kss->spi_xfer1, &kss->spi_msg1);
+
+	spi_message_init(&kss->spi_msg2);
+	spi_message_add_tail(&kss->spi_xfer2[0], &kss->spi_msg2);
+	spi_message_add_tail(&kss->spi_xfer2[1], &kss->spi_msg2);
+
+	ndev->irq = spi->irq;
+
+	return ks8851_probe_common(ndev, dev);
+}
+
+static int ks8851_remove_spi(struct spi_device *spi)
+{
+	return ks8851_remove_common(&spi->dev);
+}
+
+static const struct of_device_id ks8851_match_table[] = {
+	{ .compatible = "micrel,ks8851" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ks8851_match_table);
+
+static struct spi_driver ks8851_driver = {
+	.driver = {
+		.name = "ks8851",
+		.of_match_table = ks8851_match_table,
+		.pm = &ks8851_pm_ops,
+	},
+	.probe = ks8851_probe_spi,
+	.remove = ks8851_remove_spi,
+};
+module_spi_driver(ks8851_driver);
+
+MODULE_DESCRIPTION("KS8851 Network driver");
+MODULE_AUTHOR("Ben Dooks <ben@simtec.co.uk>");
+MODULE_LICENSE("GPL");
-- 
2.25.1

[PATCH 13/14] net: ks8851: Implement Parallel bus operations

[Marek Vasut]

Implement accessors for KS8851-16MLL/MLLI/MLLU parallel bus variant of
the KS8851. This is based off the ks8851_mll.c , which is a driver for
exactly the same hardware, however the ks8851.c code is much higher
quality. Hence, this patch pulls out the relevant information from the
ks8851_mll.c on how to access the bus, but uses the common ks8851.c
code. To make this patch reviewable, instead of rewriting ks8851_mll.c,
ks8851_mll.c is removed in a separate subsequent patch.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: David S. Miller <davem@davemloft.net>
Cc: Lukas Wunner <lukas@wunner.de>
Cc: Petr Stetiar <ynezz@true.cz>
Cc: YueHaibing <yuehaibing@huawei.com>
---
 drivers/net/ethernet/micrel/Makefile     |   2 +-
 drivers/net/ethernet/micrel/ks8851_par.c | 201 +++++++++++++++++++++++
 2 files changed, 202 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/ethernet/micrel/ks8851_par.c

diff --git a/drivers/net/ethernet/micrel/Makefile b/drivers/net/ethernet/micrel/Makefile
index bb5c6c53fa02..bff104b15aaa 100644
--- a/drivers/net/ethernet/micrel/Makefile
+++ b/drivers/net/ethernet/micrel/Makefile
@@ -5,5 +5,5 @@
 
 obj-$(CONFIG_KS8842) += ks8842.o
 obj-$(CONFIG_KS8851) += ks8851.o ks8851_spi.o
-obj-$(CONFIG_KS8851_MLL) += ks8851_mll.o
+obj-$(CONFIG_KS8851_MLL) += ks8851.o ks8851_par.o
 obj-$(CONFIG_KSZ884X_PCI) += ksz884x.o
diff --git a/drivers/net/ethernet/micrel/ks8851_par.c b/drivers/net/ethernet/micrel/ks8851_par.c
new file mode 100644
index 000000000000..2b1be1293c42
--- /dev/null
+++ b/drivers/net/ethernet/micrel/ks8851_par.c
@@ -0,0 +1,201 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* drivers/net/ethernet/micrel/ks8851.c
+ *
+ * Copyright 2009 Simtec Electronics
+ *	http://www.simtec.co.uk/
+ *	Ben Dooks <ben@simtec.co.uk>
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#define DEBUG
+
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/netdevice.h>
+#include <linux/etherdevice.h>
+#include <linux/ethtool.h>
+#include <linux/cache.h>
+#include <linux/crc32.h>
+#include <linux/mii.h>
+#include <linux/eeprom_93cx6.h>
+#include <linux/regulator/consumer.h>
+
+#include <linux/platform_device.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+#include <linux/of_net.h>
+
+#include "ks8851.h"
+
+#define BE3             0x8000      /* Byte Enable 3 */
+#define BE2             0x4000      /* Byte Enable 2 */
+#define BE1             0x2000      /* Byte Enable 1 */
+#define BE0             0x1000      /* Byte Enable 0 */
+
+/**
+ * struct ks8851_net_par - KS8851 Parallel driver private data
+ * @ks8851: KS8851 driver common private data
+ * @hw_addr	: start address of data register.
+ * @hw_addr_cmd	: start address of command register.
+ * @cmd_reg_cache	: command register cached.
+ */
+struct ks8851_net_par {
+	struct ks8851_net	ks8851;	/* Must be first */
+	void __iomem		*hw_addr;
+	void __iomem		*hw_addr_cmd;
+	u16			cmd_reg_cache;
+};
+
+#define to_ks8851_par(ks) container_of((ks), struct ks8851_net_par, ks8851)
+
+/**
+ * ks8851_wrreg16_par - write 16bit register value to chip
+ * @ks: The chip state
+ * @reg: The register address
+ * @val: The value to write
+ *
+ * Issue a write to put the value @val into the register specified in @reg.
+ */
+static void ks8851_wrreg16_par(struct ks8851_net *ks, unsigned int reg,
+			       unsigned int val)
+{
+	struct ks8851_net_par *ksp = to_ks8851_par(ks);
+
+	ksp->cmd_reg_cache = (u16)reg | ((BE3 | BE2) >> (reg & 0x02));
+	iowrite16(ksp->cmd_reg_cache, ksp->hw_addr_cmd);
+	iowrite16(val, ksp->hw_addr);
+}
+
+/**
+ * ks8851_rdreg16_par - read 16 bit register from device via SPI
+ * @ks: The chip information
+ * @reg: The register address
+ *
+ * Read a 16bit register from the chip, returning the result
+ */
+static unsigned int ks8851_rdreg16_par(struct ks8851_net *ks, unsigned int reg)
+{
+	struct ks8851_net_par *ksp = to_ks8851_par(ks);
+
+	ksp->cmd_reg_cache = (u16)reg | ((BE3 | BE2) >> (reg & 0x02));
+	iowrite16(ksp->cmd_reg_cache, ksp->hw_addr_cmd);
+	return ioread16(ksp->hw_addr);
+}
+
+/**
+ * ks8851_rdfifo_par - read data from the receive fifo
+ * @ks: The device state.
+ * @buff: The buffer address
+ * @len: The length of the data to read
+ *
+ * Issue an RXQ FIFO read command and read the @len amount of data from
+ * the FIFO into the buffer specified by @buff.
+ */
+static void ks8851_rdfifo_par(struct ks8851_net *ks, u8 *buff, unsigned int len)
+{
+	struct ks8851_net_par *ksp = to_ks8851_par(ks);
+	u16 *wptr = (u16 *)(buff + 2);
+
+	netif_dbg(ks, rx_status, ks->netdev,
+		  "%s: %d@%p\n", __func__, len, buff);
+
+	len >>= 1;
+	while (len--)
+		*wptr++ = be16_to_cpu(ioread16(ksp->hw_addr));
+}
+
+/**
+ * ks8851_wrpkt_par - write packet to TX FIFO
+ * @ks: The device state.
+ * @txp: The sk_buff to transmit.
+ * @irq: IRQ on completion of the packet.
+ *
+ * Send the @txp to the chip. This means creating the relevant packet header
+ * specifying the length of the packet and the other information the chip
+ * needs, such as IRQ on completion. Send the header and the packet data to
+ * the device.
+ */
+static void ks8851_wrpkt_par(struct ks8851_net *ks, struct sk_buff *txp,
+			     bool irq)
+{
+	struct ks8851_net_par *ksp = to_ks8851_par(ks);
+	unsigned int len = ALIGN(txp->len, 4);
+	u16 *wptr = (u16 *)txp->data;
+	unsigned int fid = 0;
+
+	netif_dbg(ks, tx_queued, ks->netdev, "%s: skb %p, %d@%p, irq %d\n",
+		  __func__, txp, txp->len, txp->data, irq);
+
+	fid = ks->fid++;
+	fid &= TXFR_TXFID_MASK;
+
+	if (irq)
+		fid |= TXFR_TXIC;	/* irq on completion */
+
+	iowrite16(cpu_to_be16(fid), ksp->hw_addr);
+	iowrite16(cpu_to_be16(txp->len), ksp->hw_addr);
+
+	len >>= 1;
+	while (len--)
+		iowrite16(cpu_to_be16(*wptr++), ksp->hw_addr);
+}
+
+static int ks8851_probe_par(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct ks8851_net_par *ksp;
+	struct net_device *ndev;
+	struct ks8851_net *ks;
+
+	ndev = devm_alloc_etherdev(dev, sizeof(struct ks8851_net_par));
+	if (!ndev)
+		return -ENOMEM;
+
+	ks = netdev_priv(ndev);
+	ks->rdreg16 = ks8851_rdreg16_par;
+	ks->wrreg16 = ks8851_wrreg16_par;
+	ks->rdfifo = ks8851_rdfifo_par;
+	ks->wrfifo = ks8851_wrpkt_par;
+
+	ksp = to_ks8851_par(ks);
+
+	ksp->hw_addr = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(ksp->hw_addr))
+		return PTR_ERR(ksp->hw_addr);
+
+	ksp->hw_addr_cmd = devm_platform_ioremap_resource(pdev, 1);
+	if (IS_ERR(ksp->hw_addr_cmd))
+		return PTR_ERR(ksp->hw_addr_cmd);
+
+	ndev->irq = platform_get_irq(pdev, 0);
+
+	return ks8851_probe_common(ndev, dev);
+}
+
+static int ks8851_remove_par(struct platform_device *pdev)
+{
+	return ks8851_remove_common(&pdev->dev);
+}
+
+static const struct of_device_id ks8851_match_table[] = {
+	{ .compatible = "micrel,ks8851-mll" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ks8851_match_table);
+
+static struct platform_driver ks8851_driver = {
+	.driver = {
+		.name = "ks8851",
+		.of_match_table = ks8851_match_table,
+		.pm = &ks8851_pm_ops,
+	},
+	.probe = ks8851_probe_par,
+	.remove = ks8851_remove_par,
+};
+module_platform_driver(ks8851_driver);
+
+MODULE_DESCRIPTION("KS8851 Network driver");
+MODULE_AUTHOR("Ben Dooks <ben@simtec.co.uk>");
+MODULE_LICENSE("GPL");
-- 
2.25.1

Re: [PATCH 13/14] net: ks8851: Implement Parallel bus operations

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 8599 bytes --]

Hi Marek,

I love your patch! Yet something to improve:

[auto build test ERROR on net/master]
[also build test ERROR on net-next/master linus/master ipvs/master v5.6-rc7 next-20200323]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Marek-Vasut/net-ks8851-Unify-KS8851-SPI-and-MLL-drivers/20200324-074805
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 6cd6cbf593bfa3ae6fc3ed34ac21da4d35045425
config: x86_64-kexec (attached as .config)
compiler: gcc-7 (Debian 7.5.0-5) 7.5.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   ld: drivers/net/ethernet/micrel/ks8851.o: in function `ks8851_set_eeprom':
>> drivers/net/ethernet/micrel/ks8851.c:875: undefined reference to `eeprom_93cx6_wren'
>> ld: drivers/net/ethernet/micrel/ks8851.c:880: undefined reference to `eeprom_93cx6_read'
>> ld: drivers/net/ethernet/micrel/ks8851.c:890: undefined reference to `eeprom_93cx6_write'
>> ld: drivers/net/ethernet/micrel/ks8851.c:891: undefined reference to `eeprom_93cx6_wren'
   ld: drivers/net/ethernet/micrel/ks8851.o: in function `ks8851_get_eeprom':
>> drivers/net/ethernet/micrel/ks8851.c:914: undefined reference to `eeprom_93cx6_multiread'

vim +875 drivers/net/ethernet/micrel/ks8851.c

51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  856  
a84afa40e07b68 drivers/net/ks8851.c                 Sebastien Jan 2010-05-05  857  static int ks8851_set_eeprom(struct net_device *dev,
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  858  			     struct ethtool_eeprom *ee, u8 *data)
a84afa40e07b68 drivers/net/ks8851.c                 Sebastien Jan 2010-05-05  859  {
a84afa40e07b68 drivers/net/ks8851.c                 Sebastien Jan 2010-05-05  860  	struct ks8851_net *ks = netdev_priv(dev);
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  861  	int offset = ee->offset;
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  862  	int len = ee->len;
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  863  	u16 tmp;
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  864  
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  865  	/* currently only support byte writing */
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  866  	if (len != 1)
a84afa40e07b68 drivers/net/ks8851.c                 Sebastien Jan 2010-05-05  867  		return -EINVAL;
a84afa40e07b68 drivers/net/ks8851.c                 Sebastien Jan 2010-05-05  868  
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  869  	if (ee->magic != KS_EEPROM_MAGIC)
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  870  		return -EINVAL;
a84afa40e07b68 drivers/net/ks8851.c                 Sebastien Jan 2010-05-05  871  
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  872  	if (ks8851_eeprom_claim(ks))
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  873  		return -ENOENT;
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  874  
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21 @875  	eeprom_93cx6_wren(&ks->eeprom, true);
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  876  
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  877  	/* ethtool currently only supports writing bytes, which means
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  878  	 * we have to read/modify/write our 16bit EEPROMs */
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  879  
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21 @880  	eeprom_93cx6_read(&ks->eeprom, offset/2, &tmp);
a84afa40e07b68 drivers/net/ks8851.c                 Sebastien Jan 2010-05-05  881  
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  882  	if (offset & 1) {
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  883  		tmp &= 0xff;
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  884  		tmp |= *data << 8;
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  885  	} else {
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  886  		tmp &= 0xff00;
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  887  		tmp |= *data;
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  888  	}
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  889  
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21 @890  	eeprom_93cx6_write(&ks->eeprom, offset/2, tmp);
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21 @891  	eeprom_93cx6_wren(&ks->eeprom, false);
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  892  
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  893  	ks8851_eeprom_release(ks);
a84afa40e07b68 drivers/net/ks8851.c                 Sebastien Jan 2010-05-05  894  
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  895  	return 0;
a84afa40e07b68 drivers/net/ks8851.c                 Sebastien Jan 2010-05-05  896  }
a84afa40e07b68 drivers/net/ks8851.c                 Sebastien Jan 2010-05-05  897  
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  898  static int ks8851_get_eeprom(struct net_device *dev,
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  899  			     struct ethtool_eeprom *ee, u8 *data)
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  900  {
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  901  	struct ks8851_net *ks = netdev_priv(dev);
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  902  	int offset = ee->offset;
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  903  	int len = ee->len;
a84afa40e07b68 drivers/net/ks8851.c                 Sebastien Jan 2010-05-05  904  
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  905  	/* must be 2 byte aligned */
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  906  	if (len & 1 || offset & 1)
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  907  		return -EINVAL;
a84afa40e07b68 drivers/net/ks8851.c                 Sebastien Jan 2010-05-05  908  
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  909  	if (ks8851_eeprom_claim(ks))
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  910  		return -ENOENT;
a84afa40e07b68 drivers/net/ks8851.c                 Sebastien Jan 2010-05-05  911  
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  912  	ee->magic = KS_EEPROM_MAGIC;
a84afa40e07b68 drivers/net/ks8851.c                 Sebastien Jan 2010-05-05  913  
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21 @914  	eeprom_93cx6_multiread(&ks->eeprom, offset/2, (__le16 *)data, len/2);
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  915  	ks8851_eeprom_release(ks);
a84afa40e07b68 drivers/net/ks8851.c                 Sebastien Jan 2010-05-05  916  
51b7b1c34e1958 drivers/net/ethernet/micrel/ks8851.c Ben Dooks     2011-11-21  917  	return 0;
a84afa40e07b68 drivers/net/ks8851.c                 Sebastien Jan 2010-05-05  918  }
a84afa40e07b68 drivers/net/ks8851.c                 Sebastien Jan 2010-05-05  919  

:::::: The code at line 875 was first introduced by commit
:::::: 51b7b1c34e195886e38ee93ff2a8a203745f897f KSZ8851-SNL: Add ethtool support for EEPROM via eeprom_93cx6

:::::: TO: Ben Dooks <ben@simtec.co.uk>
:::::: CC: David S. Miller <davem@davemloft.net>

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27775 bytes --]

[PATCH 14/14] net: ks8851: Remove ks8851_mll.c

[Marek Vasut]

The ks8851_mll.c is replaced by ks8851_par.c, which is using common code
from ks8851.c, just like ks8851_spi.c . Remove this old ad-hoc driver.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: David S. Miller <davem@davemloft.net>
Cc: Lukas Wunner <lukas@wunner.de>
Cc: Petr Stetiar <ynezz@true.cz>
Cc: YueHaibing <yuehaibing@huawei.com>
---
 drivers/net/ethernet/micrel/ks8851_mll.c | 1345 ----------------------
 1 file changed, 1345 deletions(-)
 delete mode 100644 drivers/net/ethernet/micrel/ks8851_mll.c

diff --git a/drivers/net/ethernet/micrel/ks8851_mll.c b/drivers/net/ethernet/micrel/ks8851_mll.c
deleted file mode 100644
index 58579baf3f7a..000000000000
--- a/drivers/net/ethernet/micrel/ks8851_mll.c
+++ /dev/null
@@ -1,1345 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/**
- * drivers/net/ethernet/micrel/ks8851_mll.c
- * Copyright (c) 2009 Micrel Inc.
- */
-
-/* Supports:
- * KS8851 16bit MLL chip from Micrel Inc.
- */
-
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
-#include <linux/interrupt.h>
-#include <linux/module.h>
-#include <linux/kernel.h>
-#include <linux/netdevice.h>
-#include <linux/etherdevice.h>
-#include <linux/ethtool.h>
-#include <linux/cache.h>
-#include <linux/crc32.h>
-#include <linux/crc32poly.h>
-#include <linux/mii.h>
-#include <linux/platform_device.h>
-#include <linux/delay.h>
-#include <linux/slab.h>
-#include <linux/ks8851_mll.h>
-#include <linux/of.h>
-#include <linux/of_device.h>
-#include <linux/of_net.h>
-
-#include "ks8851.h"
-
-#define	DRV_NAME	"ks8851_mll"
-
-static u8 KS_DEFAULT_MAC_ADDRESS[] = { 0x00, 0x10, 0xA1, 0x86, 0x95, 0x11 };
-#define MAX_RECV_FRAMES			255
-#define MAX_BUF_SIZE			2048
-#define TX_BUF_SIZE			2000
-#define RX_BUF_SIZE			2000
-
-#define RXCR1_FILTER_MASK    		(RXCR1_RXINVF | RXCR1_RXAE | \
-					 RXCR1_RXMAFMA | RXCR1_RXPAFMA)
-#define RXQCR_CMD_CNTL                	(RXQCR_RXFCTE|RXQCR_ADRFE)
-
-#define	ENUM_BUS_NONE			0
-#define	ENUM_BUS_8BIT			1
-#define	ENUM_BUS_16BIT			2
-#define	ENUM_BUS_32BIT			3
-
-#define MAX_MCAST_LST			32
-#define HW_MCAST_SIZE			8
-
-/**
- * union ks_tx_hdr - tx header data
- * @txb: The header as bytes
- * @txw: The header as 16bit, little-endian words
- *
- * A dual representation of the tx header data to allow
- * access to individual bytes, and to allow 16bit accesses
- * with 16bit alignment.
- */
-union ks_tx_hdr {
-	u8      txb[4];
-	__le16  txw[2];
-};
-
-/**
- * struct ks_net - KS8851 driver private data
- * @net_device 	: The network device we're bound to
- * @hw_addr	: start address of data register.
- * @hw_addr_cmd	: start address of command register.
- * @txh    	: temporaly buffer to save status/length.
- * @lock	: Lock to ensure that the device is not accessed when busy.
- * @pdev	: Pointer to platform device.
- * @mii		: The MII state information for the mii calls.
- * @frame_head_info   	: frame header information for multi-pkt rx.
- * @statelock	: Lock on this structure for tx list.
- * @msg_enable	: The message flags controlling driver output (see ethtool).
- * @frame_cnt  	: number of frames received.
- * @bus_width  	: i/o bus width.
- * @rc_rxqcr	: Cached copy of KS_RXQCR.
- * @rc_txcr	: Cached copy of KS_TXCR.
- * @rc_ier	: Cached copy of KS_IER.
- * @sharedbus  	: Multipex(addr and data bus) mode indicator.
- * @cmd_reg_cache	: command register cached.
- * @cmd_reg_cache_int	: command register cached. Used in the irq handler.
- * @promiscuous	: promiscuous mode indicator.
- * @all_mcast  	: mutlicast indicator.
- * @mcast_lst_size   	: size of multicast list.
- * @mcast_lst    	: multicast list.
- * @mcast_bits    	: multicast enabed.
- * @mac_addr   		: MAC address assigned to this device.
- * @fid    		: frame id.
- * @extra_byte    	: number of extra byte prepended rx pkt.
- * @enabled    		: indicator this device works.
- *
- * The @lock ensures that the chip is protected when certain operations are
- * in progress. When the read or write packet transfer is in progress, most
- * of the chip registers are not accessible until the transfer is finished and
- * the DMA has been de-asserted.
- *
- * The @statelock is used to protect information in the structure which may
- * need to be accessed via several sources, such as the network driver layer
- * or one of the work queues.
- *
- */
-
-/* Receive multiplex framer header info */
-struct type_frame_head {
-	u16	sts;         /* Frame status */
-	u16	len;         /* Byte count */
-};
-
-struct ks_net {
-	struct net_device	*netdev;
-	void __iomem    	*hw_addr;
-	void __iomem    	*hw_addr_cmd;
-	union ks_tx_hdr		txh ____cacheline_aligned;
-	struct mutex      	lock; /* spinlock to be interrupt safe */
-	struct platform_device *pdev;
-	struct mii_if_info	mii;
-	struct type_frame_head	*frame_head_info;
-	spinlock_t		statelock;
-	u32			msg_enable;
-	u32			frame_cnt;
-	int			bus_width;
-
-	u16			rc_rxqcr;
-	u16			rc_txcr;
-	u16			rc_ier;
-	u16			sharedbus;
-	u16			cmd_reg_cache;
-	u16			cmd_reg_cache_int;
-	u16			promiscuous;
-	u16			all_mcast;
-	u16			mcast_lst_size;
-	u8			mcast_lst[MAX_MCAST_LST][ETH_ALEN];
-	u8			mcast_bits[HW_MCAST_SIZE];
-	u8			mac_addr[6];
-	u8                      fid;
-	u8			extra_byte;
-	u8			enabled;
-};
-
-static int msg_enable;
-
-#define BE3             0x8000      /* Byte Enable 3 */
-#define BE2             0x4000      /* Byte Enable 2 */
-#define BE1             0x2000      /* Byte Enable 1 */
-#define BE0             0x1000      /* Byte Enable 0 */
-
-/* register read/write calls.
- *
- * All these calls issue transactions to access the chip's registers. They
- * all require that the necessary lock is held to prevent accesses when the
- * chip is busy transferring packet data (RX/TX FIFO accesses).
- */
-
-/**
- * ks_rdreg16 - read 16 bit register from device
- * @ks	  : The chip information
- * @offset: The register address
- *
- * Read a 16bit register from the chip, returning the result
- */
-
-static u16 ks_rdreg16(struct ks_net *ks, int offset)
-{
-	ks->cmd_reg_cache = (u16)offset | ((BE3 | BE2) >> (offset & 0x02));
-	iowrite16(ks->cmd_reg_cache, ks->hw_addr_cmd);
-	return ioread16(ks->hw_addr);
-}
-
-/**
- * ks_wrreg16 - write 16bit register value to chip
- * @ks: The chip information
- * @offset: The register address
- * @value: The value to write
- *
- */
-
-static void ks_wrreg16(struct ks_net *ks, int offset, u16 value)
-{
-	ks->cmd_reg_cache = (u16)offset | ((BE3 | BE2) >> (offset & 0x02));
-	iowrite16(ks->cmd_reg_cache, ks->hw_addr_cmd);
-	iowrite16(value, ks->hw_addr);
-}
-
-/**
- * ks_inblk - read a block of data from QMU. This is called after sudo DMA mode enabled.
- * @ks: The chip state
- * @wptr: buffer address to save data
- * @len: length in byte to read
- *
- */
-static inline void ks_inblk(struct ks_net *ks, u16 *wptr, u32 len)
-{
-	len >>= 1;
-	while (len--)
-		*wptr++ = be16_to_cpu(ioread16(ks->hw_addr));
-}
-
-/**
- * ks_outblk - write data to QMU. This is called after sudo DMA mode enabled.
- * @ks: The chip information
- * @wptr: buffer address
- * @len: length in byte to write
- *
- */
-static inline void ks_outblk(struct ks_net *ks, u16 *wptr, u32 len)
-{
-	len >>= 1;
-	while (len--)
-		iowrite16(cpu_to_be16(*wptr++), ks->hw_addr);
-}
-
-static void ks_disable_int(struct ks_net *ks)
-{
-	ks_wrreg16(ks, KS_IER, 0x0000);
-}  /* ks_disable_int */
-
-static void ks_enable_int(struct ks_net *ks)
-{
-	ks_wrreg16(ks, KS_IER, ks->rc_ier);
-}  /* ks_enable_int */
-
-/**
- * ks_tx_fifo_space - return the available hardware buffer size.
- * @ks: The chip information
- *
- */
-static inline u16 ks_tx_fifo_space(struct ks_net *ks)
-{
-	return ks_rdreg16(ks, KS_TXMIR) & 0x1fff;
-}
-
-/**
- * ks_save_cmd_reg - save the command register from the cache.
- * @ks: The chip information
- *
- */
-static inline void ks_save_cmd_reg(struct ks_net *ks)
-{
-	/*ks8851 MLL has a bug to read back the command register.
-	* So rely on software to save the content of command register.
-	*/
-	ks->cmd_reg_cache_int = ks->cmd_reg_cache;
-}
-
-/**
- * ks_restore_cmd_reg - restore the command register from the cache and
- * 	write to hardware register.
- * @ks: The chip information
- *
- */
-static inline void ks_restore_cmd_reg(struct ks_net *ks)
-{
-	ks->cmd_reg_cache = ks->cmd_reg_cache_int;
-	iowrite16(ks->cmd_reg_cache, ks->hw_addr_cmd);
-}
-
-/**
- * ks_set_powermode - set power mode of the device
- * @ks: The chip information
- * @pwrmode: The power mode value to write to KS_PMECR.
- *
- * Change the power mode of the chip.
- */
-static void ks_set_powermode(struct ks_net *ks, unsigned pwrmode)
-{
-	unsigned pmecr;
-
-	netif_dbg(ks, hw, ks->netdev, "setting power mode %d\n", pwrmode);
-
-	ks_rdreg16(ks, KS_GRR);
-	pmecr = ks_rdreg16(ks, KS_PMECR);
-	pmecr &= ~PMECR_PM_MASK;
-	pmecr |= pwrmode;
-
-	ks_wrreg16(ks, KS_PMECR, pmecr);
-}
-
-/**
- * ks_read_config - read chip configuration of bus width.
- * @ks: The chip information
- *
- */
-static void ks_read_config(struct ks_net *ks)
-{
-	u16 reg_data = 0;
-
-	/* Regardless of bus width, 8 bit read should always work.*/
-	reg_data = ks_rdreg16(ks, KS_CCR);
-
-	/* addr/data bus are multiplexed */
-	ks->sharedbus = (reg_data & CCR_SHARED) == CCR_SHARED;
-
-	/* There are garbage data when reading data from QMU,
-	depending on bus-width.
-	*/
-
-	if (reg_data & CCR_8BIT) {
-		ks->bus_width = ENUM_BUS_8BIT;
-		ks->extra_byte = 1;
-	} else if (reg_data & CCR_16BIT) {
-		ks->bus_width = ENUM_BUS_16BIT;
-		ks->extra_byte = 2;
-	} else {
-		ks->bus_width = ENUM_BUS_32BIT;
-		ks->extra_byte = 4;
-	}
-}
-
-/**
- * ks_soft_reset - issue one of the soft reset to the device
- * @ks: The device state.
- * @op: The bit(s) to set in the GRR
- *
- * Issue the relevant soft-reset command to the device's GRR register
- * specified by @op.
- *
- * Note, the delays are in there as a caution to ensure that the reset
- * has time to take effect and then complete. Since the datasheet does
- * not currently specify the exact sequence, we have chosen something
- * that seems to work with our device.
- */
-static void ks_soft_reset(struct ks_net *ks, unsigned op)
-{
-	/* Disable interrupt first */
-	ks_wrreg16(ks, KS_IER, 0x0000);
-	ks_wrreg16(ks, KS_GRR, op);
-	mdelay(10);	/* wait a short time to effect reset */
-	ks_wrreg16(ks, KS_GRR, 0);
-	mdelay(1);	/* wait for condition to clear */
-}
-
-
-static void ks_enable_qmu(struct ks_net *ks)
-{
-	u16 w;
-
-	w = ks_rdreg16(ks, KS_TXCR);
-	/* Enables QMU Transmit (TXCR). */
-	ks_wrreg16(ks, KS_TXCR, w | TXCR_TXE);
-
-	/*
-	 * RX Frame Count Threshold Enable and Auto-Dequeue RXQ Frame
-	 * Enable
-	 */
-
-	w = ks_rdreg16(ks, KS_RXQCR);
-	ks_wrreg16(ks, KS_RXQCR, w | RXQCR_RXFCTE);
-
-	/* Enables QMU Receive (RXCR1). */
-	w = ks_rdreg16(ks, KS_RXCR1);
-	ks_wrreg16(ks, KS_RXCR1, w | RXCR1_RXE);
-	ks->enabled = true;
-}  /* ks_enable_qmu */
-
-static void ks_disable_qmu(struct ks_net *ks)
-{
-	u16	w;
-
-	w = ks_rdreg16(ks, KS_TXCR);
-
-	/* Disables QMU Transmit (TXCR). */
-	w  &= ~TXCR_TXE;
-	ks_wrreg16(ks, KS_TXCR, w);
-
-	/* Disables QMU Receive (RXCR1). */
-	w = ks_rdreg16(ks, KS_RXCR1);
-	w &= ~RXCR1_RXE ;
-	ks_wrreg16(ks, KS_RXCR1, w);
-
-	ks->enabled = false;
-
-}  /* ks_disable_qmu */
-
-/**
- * ks_read_qmu - read 1 pkt data from the QMU.
- * @ks: The chip information
- * @buf: buffer address to save 1 pkt
- * @len: Pkt length
- * Here is the sequence to read 1 pkt:
- *	1. set sudo DMA mode
- *	2. read prepend data
- *	3. read pkt data
- *	4. reset sudo DMA Mode
- */
-static inline void ks_read_qmu(struct ks_net *ks, u16 *buf, u32 len)
-{
-	u32 r =  ks->extra_byte & 0x1 ;
-	u32 w = ks->extra_byte - r;
-
-	/* 1. set sudo DMA mode */
-	ks_wrreg16(ks, KS_RXFDPR, RXFDPR_RXFPAI);
-	ks_wrreg16(ks, KS_RXQCR, ks->rc_rxqcr | RXQCR_SDA);
-
-	/* 2. read prepend data */
-	/**
-	 * read 4 + extra bytes and discard them.
-	 * extra bytes for dummy, 2 for status, 2 for len
-	 */
-
-	/* use likely(r) for 8 bit access for performance */
-	if (unlikely(r))
-		ioread8(ks->hw_addr);
-	ks_inblk(ks, buf, w + 2 + 2);
-
-	/* 3. read pkt data */
-	ks_inblk(ks, buf, ALIGN(len, 4));
-
-	/* 4. reset sudo DMA Mode */
-	ks_wrreg16(ks, KS_RXQCR, ks->rc_rxqcr);
-}
-
-/**
- * ks_rcv - read multiple pkts data from the QMU.
- * @ks: The chip information
- * @netdev: The network device being opened.
- *
- * Read all of header information before reading pkt content.
- * It is not allowed only port of pkts in QMU after issuing
- * interrupt ack.
- */
-static void ks_rcv(struct ks_net *ks, struct net_device *netdev)
-{
-	u32	i;
-	struct type_frame_head *frame_hdr = ks->frame_head_info;
-	struct sk_buff *skb;
-
-	ks->frame_cnt = ks_rdreg16(ks, KS_RXFCTR) >> 8;
-
-	/* read all header information */
-	for (i = 0; i < ks->frame_cnt; i++) {
-		/* Checking Received packet status */
-		frame_hdr->sts = ks_rdreg16(ks, KS_RXFHSR);
-		/* Get packet len from hardware */
-		frame_hdr->len = ks_rdreg16(ks, KS_RXFHBCR);
-		frame_hdr++;
-	}
-
-	frame_hdr = ks->frame_head_info;
-	while (ks->frame_cnt--) {
-		if (unlikely(!(frame_hdr->sts & RXFSHR_RXFV) ||
-			     frame_hdr->len >= RX_BUF_SIZE ||
-			     frame_hdr->len <= 0)) {
-
-			/* discard an invalid packet */
-			ks_wrreg16(ks, KS_RXQCR, (ks->rc_rxqcr | RXQCR_RRXEF));
-			netdev->stats.rx_dropped++;
-			if (!(frame_hdr->sts & RXFSHR_RXFV))
-				netdev->stats.rx_frame_errors++;
-			else
-				netdev->stats.rx_length_errors++;
-			frame_hdr++;
-			continue;
-		}
-
-		skb = netdev_alloc_skb(netdev, frame_hdr->len + 16);
-		if (likely(skb)) {
-			skb_reserve(skb, 2);
-			/* read data block including CRC 4 bytes */
-			ks_read_qmu(ks, (u16 *)skb->data, frame_hdr->len);
-			skb_put(skb, frame_hdr->len - 4);
-			skb->protocol = eth_type_trans(skb, netdev);
-			netif_rx(skb);
-			/* exclude CRC size */
-			netdev->stats.rx_bytes += frame_hdr->len - 4;
-			netdev->stats.rx_packets++;
-		} else {
-			ks_wrreg16(ks, KS_RXQCR, (ks->rc_rxqcr | RXQCR_RRXEF));
-			netdev->stats.rx_dropped++;
-		}
-		frame_hdr++;
-	}
-}
-
-/**
- * ks_update_link_status - link status update.
- * @netdev: The network device being opened.
- * @ks: The chip information
- *
- */
-
-static void ks_update_link_status(struct net_device *netdev, struct ks_net *ks)
-{
-	/* check the status of the link */
-	u32 link_up_status;
-	if (ks_rdreg16(ks, KS_P1SR) & P1SR_LINK_GOOD) {
-		netif_carrier_on(netdev);
-		link_up_status = true;
-	} else {
-		netif_carrier_off(netdev);
-		link_up_status = false;
-	}
-	netif_dbg(ks, link, ks->netdev,
-		  "%s: %s\n", __func__, link_up_status ? "UP" : "DOWN");
-}
-
-/**
- * ks_irq - device interrupt handler
- * @irq: Interrupt number passed from the IRQ handler.
- * @pw: The private word passed to register_irq(), our struct ks_net.
- *
- * This is the handler invoked to find out what happened
- *
- * Read the interrupt status, work out what needs to be done and then clear
- * any of the interrupts that are not needed.
- */
-
-static irqreturn_t ks_irq(int irq, void *pw)
-{
-	struct net_device *netdev = pw;
-	struct ks_net *ks = netdev_priv(netdev);
-	unsigned long flags;
-	u16 status;
-
-	spin_lock_irqsave(&ks->statelock, flags);
-	/*this should be the first in IRQ handler */
-	ks_save_cmd_reg(ks);
-
-	status = ks_rdreg16(ks, KS_ISR);
-	if (unlikely(!status)) {
-		ks_restore_cmd_reg(ks);
-		spin_unlock_irqrestore(&ks->statelock, flags);
-		return IRQ_NONE;
-	}
-
-	ks_wrreg16(ks, KS_ISR, status);
-
-	if (likely(status & IRQ_RXI))
-		ks_rcv(ks, netdev);
-
-	if (unlikely(status & IRQ_LCI))
-		ks_update_link_status(netdev, ks);
-
-	if (unlikely(status & IRQ_TXI))
-		netif_wake_queue(netdev);
-
-	if (unlikely(status & IRQ_LDI)) {
-
-		u16 pmecr = ks_rdreg16(ks, KS_PMECR);
-		pmecr &= ~PMECR_WKEVT_MASK;
-		ks_wrreg16(ks, KS_PMECR, pmecr | PMECR_WKEVT_LINK);
-	}
-
-	if (unlikely(status & IRQ_RXOI))
-		ks->netdev->stats.rx_over_errors++;
-	/* this should be the last in IRQ handler*/
-	ks_restore_cmd_reg(ks);
-	spin_unlock_irqrestore(&ks->statelock, flags);
-	return IRQ_HANDLED;
-}
-
-
-/**
- * ks_net_open - open network device
- * @netdev: The network device being opened.
- *
- * Called when the network device is marked active, such as a user executing
- * 'ifconfig up' on the device.
- */
-static int ks_net_open(struct net_device *netdev)
-{
-	struct ks_net *ks = netdev_priv(netdev);
-	int err;
-
-#define	KS_INT_FLAGS	IRQF_TRIGGER_LOW
-	/* lock the card, even if we may not actually do anything
-	 * else at the moment.
-	 */
-
-	netif_dbg(ks, ifup, ks->netdev, "%s - entry\n", __func__);
-
-	/* reset the HW */
-	err = request_irq(netdev->irq, ks_irq, KS_INT_FLAGS, DRV_NAME, netdev);
-
-	if (err) {
-		pr_err("Failed to request IRQ: %d: %d\n", netdev->irq, err);
-		return err;
-	}
-
-	/* wake up powermode to normal mode */
-	ks_set_powermode(ks, PMECR_PM_NORMAL);
-	mdelay(1);	/* wait for normal mode to take effect */
-
-	ks_wrreg16(ks, KS_ISR, 0xffff);
-	ks_enable_int(ks);
-	ks_enable_qmu(ks);
-	netif_start_queue(ks->netdev);
-
-	netif_dbg(ks, ifup, ks->netdev, "network device up\n");
-
-	return 0;
-}
-
-/**
- * ks_net_stop - close network device
- * @netdev: The device being closed.
- *
- * Called to close down a network device which has been active. Cancell any
- * work, shutdown the RX and TX process and then place the chip into a low
- * power state whilst it is not being used.
- */
-static int ks_net_stop(struct net_device *netdev)
-{
-	struct ks_net *ks = netdev_priv(netdev);
-
-	netif_info(ks, ifdown, netdev, "shutting down\n");
-
-	netif_stop_queue(netdev);
-
-	mutex_lock(&ks->lock);
-
-	/* turn off the IRQs and ack any outstanding */
-	ks_wrreg16(ks, KS_IER, 0x0000);
-	ks_wrreg16(ks, KS_ISR, 0xffff);
-
-	/* shutdown RX/TX QMU */
-	ks_disable_qmu(ks);
-	ks_disable_int(ks);
-
-	/* set powermode to soft power down to save power */
-	ks_set_powermode(ks, PMECR_PM_SOFTDOWN);
-	free_irq(netdev->irq, netdev);
-	mutex_unlock(&ks->lock);
-	return 0;
-}
-
-
-/**
- * ks_write_qmu - write 1 pkt data to the QMU.
- * @ks: The chip information
- * @pdata: buffer address to save 1 pkt
- * @len: Pkt length in byte
- * Here is the sequence to write 1 pkt:
- *	1. set sudo DMA mode
- *	2. write status/length
- *	3. write pkt data
- *	4. reset sudo DMA Mode
- *	5. reset sudo DMA mode
- *	6. Wait until pkt is out
- */
-static void ks_write_qmu(struct ks_net *ks, u8 *pdata, u16 len)
-{
-	/* start header at txb[0] to align txw entries */
-	ks->txh.txw[0] = 0;
-	ks->txh.txw[1] = cpu_to_le16(len);
-
-	/* 1. set sudo-DMA mode */
-	ks_wrreg16(ks, KS_RXQCR, ks->rc_rxqcr | RXQCR_SDA);
-	/* 2. write status/lenth info */
-	ks_outblk(ks, ks->txh.txw, 4);
-	/* 3. write pkt data */
-	ks_outblk(ks, (u16 *)pdata, ALIGN(len, 4));
-	/* 4. reset sudo-DMA mode */
-	ks_wrreg16(ks, KS_RXQCR, ks->rc_rxqcr);
-	/* 5. Enqueue Tx(move the pkt from TX buffer into TXQ) */
-	ks_wrreg16(ks, KS_TXQCR, TXQCR_METFE);
-	/* 6. wait until TXQCR_METFE is auto-cleared */
-	while (ks_rdreg16(ks, KS_TXQCR) & TXQCR_METFE)
-		;
-}
-
-/**
- * ks_start_xmit - transmit packet
- * @skb		: The buffer to transmit
- * @netdev	: The device used to transmit the packet.
- *
- * Called by the network layer to transmit the @skb.
- * spin_lock_irqsave is required because tx and rx should be mutual exclusive.
- * So while tx is in-progress, prevent IRQ interrupt from happenning.
- */
-static netdev_tx_t ks_start_xmit(struct sk_buff *skb, struct net_device *netdev)
-{
-	netdev_tx_t retv = NETDEV_TX_OK;
-	struct ks_net *ks = netdev_priv(netdev);
-	unsigned long flags;
-
-	spin_lock_irqsave(&ks->statelock, flags);
-
-	/* Extra space are required:
-	*  4 byte for alignment, 4 for status/length, 4 for CRC
-	*/
-
-	if (likely(ks_tx_fifo_space(ks) >= skb->len + 12)) {
-		ks_write_qmu(ks, skb->data, skb->len);
-		/* add tx statistics */
-		netdev->stats.tx_bytes += skb->len;
-		netdev->stats.tx_packets++;
-		dev_kfree_skb(skb);
-	} else
-		retv = NETDEV_TX_BUSY;
-	spin_unlock_irqrestore(&ks->statelock, flags);
-	return retv;
-}
-
-/**
- * ks_start_rx - ready to serve pkts
- * @ks		: The chip information
- *
- */
-static void ks_start_rx(struct ks_net *ks)
-{
-	u16 cntl;
-
-	/* Enables QMU Receive (RXCR1). */
-	cntl = ks_rdreg16(ks, KS_RXCR1);
-	cntl |= RXCR1_RXE ;
-	ks_wrreg16(ks, KS_RXCR1, cntl);
-}  /* ks_start_rx */
-
-/**
- * ks_stop_rx - stop to serve pkts
- * @ks		: The chip information
- *
- */
-static void ks_stop_rx(struct ks_net *ks)
-{
-	u16 cntl;
-
-	/* Disables QMU Receive (RXCR1). */
-	cntl = ks_rdreg16(ks, KS_RXCR1);
-	cntl &= ~RXCR1_RXE ;
-	ks_wrreg16(ks, KS_RXCR1, cntl);
-
-}  /* ks_stop_rx */
-
-static unsigned long const ethernet_polynomial = CRC32_POLY_BE;
-
-static unsigned long ether_gen_crc(int length, u8 *data)
-{
-	long crc = -1;
-	while (--length >= 0) {
-		u8 current_octet = *data++;
-		int bit;
-
-		for (bit = 0; bit < 8; bit++, current_octet >>= 1) {
-			crc = (crc << 1) ^
-				((crc < 0) ^ (current_octet & 1) ?
-			ethernet_polynomial : 0);
-		}
-	}
-	return (unsigned long)crc;
-}  /* ether_gen_crc */
-
-/**
-* ks_set_grpaddr - set multicast information
-* @ks : The chip information
-*/
-
-static void ks_set_grpaddr(struct ks_net *ks)
-{
-	u8	i;
-	u32	index, position, value;
-
-	memset(ks->mcast_bits, 0, sizeof(u8) * HW_MCAST_SIZE);
-
-	for (i = 0; i < ks->mcast_lst_size; i++) {
-		position = (ether_gen_crc(6, ks->mcast_lst[i]) >> 26) & 0x3f;
-		index = position >> 3;
-		value = 1 << (position & 7);
-		ks->mcast_bits[index] |= (u8)value;
-	}
-
-	for (i  = 0; i < HW_MCAST_SIZE; i++) {
-		if (i & 1) {
-			ks_wrreg16(ks, (u16)((KS_MAHTR0 + i) & ~1),
-				(ks->mcast_bits[i] << 8) |
-				ks->mcast_bits[i - 1]);
-		}
-	}
-}  /* ks_set_grpaddr */
-
-/**
-* ks_clear_mcast - clear multicast information
-*
-* @ks : The chip information
-* This routine removes all mcast addresses set in the hardware.
-*/
-
-static void ks_clear_mcast(struct ks_net *ks)
-{
-	u16	i, mcast_size;
-	for (i = 0; i < HW_MCAST_SIZE; i++)
-		ks->mcast_bits[i] = 0;
-
-	mcast_size = HW_MCAST_SIZE >> 2;
-	for (i = 0; i < mcast_size; i++)
-		ks_wrreg16(ks, KS_MAHTR0 + (2*i), 0);
-}
-
-static void ks_set_promis(struct ks_net *ks, u16 promiscuous_mode)
-{
-	u16		cntl;
-	ks->promiscuous = promiscuous_mode;
-	ks_stop_rx(ks);  /* Stop receiving for reconfiguration */
-	cntl = ks_rdreg16(ks, KS_RXCR1);
-
-	cntl &= ~RXCR1_FILTER_MASK;
-	if (promiscuous_mode)
-		/* Enable Promiscuous mode */
-		cntl |= RXCR1_RXAE | RXCR1_RXINVF;
-	else
-		/* Disable Promiscuous mode (default normal mode) */
-		cntl |= RXCR1_RXPAFMA;
-
-	ks_wrreg16(ks, KS_RXCR1, cntl);
-
-	if (ks->enabled)
-		ks_start_rx(ks);
-
-}  /* ks_set_promis */
-
-static void ks_set_mcast(struct ks_net *ks, u16 mcast)
-{
-	u16	cntl;
-
-	ks->all_mcast = mcast;
-	ks_stop_rx(ks);  /* Stop receiving for reconfiguration */
-	cntl = ks_rdreg16(ks, KS_RXCR1);
-	cntl &= ~RXCR1_FILTER_MASK;
-	if (mcast)
-		/* Enable "Perfect with Multicast address passed mode" */
-		cntl |= (RXCR1_RXAE | RXCR1_RXMAFMA | RXCR1_RXPAFMA);
-	else
-		/**
-		 * Disable "Perfect with Multicast address passed
-		 * mode" (normal mode).
-		 */
-		cntl |= RXCR1_RXPAFMA;
-
-	ks_wrreg16(ks, KS_RXCR1, cntl);
-
-	if (ks->enabled)
-		ks_start_rx(ks);
-}  /* ks_set_mcast */
-
-static void ks_set_rx_mode(struct net_device *netdev)
-{
-	struct ks_net *ks = netdev_priv(netdev);
-	struct netdev_hw_addr *ha;
-
-	/* Turn on/off promiscuous mode. */
-	if ((netdev->flags & IFF_PROMISC) == IFF_PROMISC)
-		ks_set_promis(ks,
-			(u16)((netdev->flags & IFF_PROMISC) == IFF_PROMISC));
-	/* Turn on/off all mcast mode. */
-	else if ((netdev->flags & IFF_ALLMULTI) == IFF_ALLMULTI)
-		ks_set_mcast(ks,
-			(u16)((netdev->flags & IFF_ALLMULTI) == IFF_ALLMULTI));
-	else
-		ks_set_promis(ks, false);
-
-	if ((netdev->flags & IFF_MULTICAST) && netdev_mc_count(netdev)) {
-		if (netdev_mc_count(netdev) <= MAX_MCAST_LST) {
-			int i = 0;
-
-			netdev_for_each_mc_addr(ha, netdev) {
-				if (i >= MAX_MCAST_LST)
-					break;
-				memcpy(ks->mcast_lst[i++], ha->addr, ETH_ALEN);
-			}
-			ks->mcast_lst_size = (u8)i;
-			ks_set_grpaddr(ks);
-		} else {
-			/**
-			 * List too big to support so
-			 * turn on all mcast mode.
-			 */
-			ks->mcast_lst_size = MAX_MCAST_LST;
-			ks_set_mcast(ks, true);
-		}
-	} else {
-		ks->mcast_lst_size = 0;
-		ks_clear_mcast(ks);
-	}
-} /* ks_set_rx_mode */
-
-static void ks_set_mac(struct ks_net *ks, u8 *data)
-{
-	u16 *pw = (u16 *)data;
-	u16 w, u;
-
-	ks_stop_rx(ks);  /* Stop receiving for reconfiguration */
-
-	u = *pw++;
-	w = ((u & 0xFF) << 8) | ((u >> 8) & 0xFF);
-	ks_wrreg16(ks, KS_MARH, w);
-
-	u = *pw++;
-	w = ((u & 0xFF) << 8) | ((u >> 8) & 0xFF);
-	ks_wrreg16(ks, KS_MARM, w);
-
-	u = *pw;
-	w = ((u & 0xFF) << 8) | ((u >> 8) & 0xFF);
-	ks_wrreg16(ks, KS_MARL, w);
-
-	memcpy(ks->mac_addr, data, ETH_ALEN);
-
-	if (ks->enabled)
-		ks_start_rx(ks);
-}
-
-static int ks_set_mac_address(struct net_device *netdev, void *paddr)
-{
-	struct ks_net *ks = netdev_priv(netdev);
-	struct sockaddr *addr = paddr;
-	u8 *da;
-
-	memcpy(netdev->dev_addr, addr->sa_data, netdev->addr_len);
-
-	da = (u8 *)netdev->dev_addr;
-
-	ks_set_mac(ks, da);
-	return 0;
-}
-
-static int ks_net_ioctl(struct net_device *netdev, struct ifreq *req, int cmd)
-{
-	struct ks_net *ks = netdev_priv(netdev);
-
-	if (!netif_running(netdev))
-		return -EINVAL;
-
-	return generic_mii_ioctl(&ks->mii, if_mii(req), cmd, NULL);
-}
-
-static const struct net_device_ops ks_netdev_ops = {
-	.ndo_open		= ks_net_open,
-	.ndo_stop		= ks_net_stop,
-	.ndo_do_ioctl		= ks_net_ioctl,
-	.ndo_start_xmit		= ks_start_xmit,
-	.ndo_set_mac_address	= ks_set_mac_address,
-	.ndo_set_rx_mode	= ks_set_rx_mode,
-	.ndo_validate_addr	= eth_validate_addr,
-};
-
-/* ethtool support */
-
-static void ks_get_drvinfo(struct net_device *netdev,
-			       struct ethtool_drvinfo *di)
-{
-	strlcpy(di->driver, DRV_NAME, sizeof(di->driver));
-	strlcpy(di->version, "1.00", sizeof(di->version));
-	strlcpy(di->bus_info, dev_name(netdev->dev.parent),
-		sizeof(di->bus_info));
-}
-
-static u32 ks_get_msglevel(struct net_device *netdev)
-{
-	struct ks_net *ks = netdev_priv(netdev);
-	return ks->msg_enable;
-}
-
-static void ks_set_msglevel(struct net_device *netdev, u32 to)
-{
-	struct ks_net *ks = netdev_priv(netdev);
-	ks->msg_enable = to;
-}
-
-static int ks_get_link_ksettings(struct net_device *netdev,
-				 struct ethtool_link_ksettings *cmd)
-{
-	struct ks_net *ks = netdev_priv(netdev);
-
-	mii_ethtool_get_link_ksettings(&ks->mii, cmd);
-
-	return 0;
-}
-
-static int ks_set_link_ksettings(struct net_device *netdev,
-				 const struct ethtool_link_ksettings *cmd)
-{
-	struct ks_net *ks = netdev_priv(netdev);
-	return mii_ethtool_set_link_ksettings(&ks->mii, cmd);
-}
-
-static u32 ks_get_link(struct net_device *netdev)
-{
-	struct ks_net *ks = netdev_priv(netdev);
-	return mii_link_ok(&ks->mii);
-}
-
-static int ks_nway_reset(struct net_device *netdev)
-{
-	struct ks_net *ks = netdev_priv(netdev);
-	return mii_nway_restart(&ks->mii);
-}
-
-static const struct ethtool_ops ks_ethtool_ops = {
-	.get_drvinfo	= ks_get_drvinfo,
-	.get_msglevel	= ks_get_msglevel,
-	.set_msglevel	= ks_set_msglevel,
-	.get_link	= ks_get_link,
-	.nway_reset	= ks_nway_reset,
-	.get_link_ksettings = ks_get_link_ksettings,
-	.set_link_ksettings = ks_set_link_ksettings,
-};
-
-/* MII interface controls */
-
-/**
- * ks_phy_reg - convert MII register into a KS8851 register
- * @reg: MII register number.
- *
- * Return the KS8851 register number for the corresponding MII PHY register
- * if possible. Return zero if the MII register has no direct mapping to the
- * KS8851 register set.
- */
-static int ks_phy_reg(int reg)
-{
-	switch (reg) {
-	case MII_BMCR:
-		return KS_P1MBCR;
-	case MII_BMSR:
-		return KS_P1MBSR;
-	case MII_PHYSID1:
-		return KS_PHY1ILR;
-	case MII_PHYSID2:
-		return KS_PHY1IHR;
-	case MII_ADVERTISE:
-		return KS_P1ANAR;
-	case MII_LPA:
-		return KS_P1ANLPR;
-	}
-
-	return 0x0;
-}
-
-/**
- * ks_phy_read - MII interface PHY register read.
- * @netdev: The network device the PHY is on.
- * @phy_addr: Address of PHY (ignored as we only have one)
- * @reg: The register to read.
- *
- * This call reads data from the PHY register specified in @reg. Since the
- * device does not support all the MII registers, the non-existent values
- * are always returned as zero.
- *
- * We return zero for unsupported registers as the MII code does not check
- * the value returned for any error status, and simply returns it to the
- * caller. The mii-tool that the driver was tested with takes any -ve error
- * as real PHY capabilities, thus displaying incorrect data to the user.
- */
-static int ks_phy_read(struct net_device *netdev, int phy_addr, int reg)
-{
-	struct ks_net *ks = netdev_priv(netdev);
-	int ksreg;
-	int result;
-
-	ksreg = ks_phy_reg(reg);
-	if (!ksreg)
-		return 0x0;	/* no error return allowed, so use zero */
-
-	mutex_lock(&ks->lock);
-	result = ks_rdreg16(ks, ksreg);
-	mutex_unlock(&ks->lock);
-
-	return result;
-}
-
-static void ks_phy_write(struct net_device *netdev,
-			     int phy, int reg, int value)
-{
-	struct ks_net *ks = netdev_priv(netdev);
-	int ksreg;
-
-	ksreg = ks_phy_reg(reg);
-	if (ksreg) {
-		mutex_lock(&ks->lock);
-		ks_wrreg16(ks, ksreg, value);
-		mutex_unlock(&ks->lock);
-	}
-}
-
-/**
- * ks_read_selftest - read the selftest memory info.
- * @ks: The device state
- *
- * Read and check the TX/RX memory selftest information.
- */
-static int ks_read_selftest(struct ks_net *ks)
-{
-	unsigned both_done = MBIR_TXMBF | MBIR_RXMBF;
-	int ret = 0;
-	unsigned rd;
-
-	rd = ks_rdreg16(ks, KS_MBIR);
-
-	if ((rd & both_done) != both_done) {
-		netdev_warn(ks->netdev, "Memory selftest not finished\n");
-		return 0;
-	}
-
-	if (rd & MBIR_TXMBFA) {
-		netdev_err(ks->netdev, "TX memory selftest fails\n");
-		ret |= 1;
-	}
-
-	if (rd & MBIR_RXMBFA) {
-		netdev_err(ks->netdev, "RX memory selftest fails\n");
-		ret |= 2;
-	}
-
-	netdev_info(ks->netdev, "the selftest passes\n");
-	return ret;
-}
-
-static void ks_setup(struct ks_net *ks)
-{
-	u16	w;
-
-	/**
-	 * Configure QMU Transmit
-	 */
-
-	/* Setup Transmit Frame Data Pointer Auto-Increment (TXFDPR) */
-	ks_wrreg16(ks, KS_TXFDPR, TXFDPR_TXFPAI);
-
-	/* Setup Receive Frame Data Pointer Auto-Increment */
-	ks_wrreg16(ks, KS_RXFDPR, RXFDPR_RXFPAI);
-
-	/* Setup Receive Frame Threshold - 1 frame (RXFCTFC) */
-	ks_wrreg16(ks, KS_RXFCTR, 1 & RXFCTR_RXFCT_MASK);
-
-	/* Setup RxQ Command Control (RXQCR) */
-	ks->rc_rxqcr = RXQCR_CMD_CNTL;
-	ks_wrreg16(ks, KS_RXQCR, ks->rc_rxqcr);
-
-	/**
-	 * set the force mode to half duplex, default is full duplex
-	 *  because if the auto-negotiation fails, most switch uses
-	 *  half-duplex.
-	 */
-
-	w = ks_rdreg16(ks, KS_P1MBCR);
-	w &= ~BMCR_FULLDPLX;
-	ks_wrreg16(ks, KS_P1MBCR, w);
-
-	w = TXCR_TXFCE | TXCR_TXPE | TXCR_TXCRC | TXCR_TCGIP;
-	ks_wrreg16(ks, KS_TXCR, w);
-
-	w = RXCR1_RXFCE | RXCR1_RXBE | RXCR1_RXUE | RXCR1_RXME | RXCR1_RXIPFCC;
-
-	if (ks->promiscuous)         /* bPromiscuous */
-		w |= (RXCR1_RXAE | RXCR1_RXINVF);
-	else if (ks->all_mcast) /* Multicast address passed mode */
-		w |= (RXCR1_RXAE | RXCR1_RXMAFMA | RXCR1_RXPAFMA);
-	else                                   /* Normal mode */
-		w |= RXCR1_RXPAFMA;
-
-	ks_wrreg16(ks, KS_RXCR1, w);
-}  /*ks_setup */
-
-
-static void ks_setup_int(struct ks_net *ks)
-{
-	ks->rc_ier = 0x00;
-	/* Clear the interrupts status of the hardware. */
-	ks_wrreg16(ks, KS_ISR, 0xffff);
-
-	/* Enables the interrupts of the hardware. */
-	ks->rc_ier = (IRQ_LCI | IRQ_TXI | IRQ_RXI);
-}  /* ks_setup_int */
-
-static int ks_hw_init(struct ks_net *ks)
-{
-#define	MHEADER_SIZE	(sizeof(struct type_frame_head) * MAX_RECV_FRAMES)
-	ks->promiscuous = 0;
-	ks->all_mcast = 0;
-	ks->mcast_lst_size = 0;
-
-	ks->frame_head_info = devm_kmalloc(&ks->pdev->dev, MHEADER_SIZE,
-					   GFP_KERNEL);
-	if (!ks->frame_head_info)
-		return false;
-
-	ks_set_mac(ks, KS_DEFAULT_MAC_ADDRESS);
-	return true;
-}
-
-#if defined(CONFIG_OF)
-static const struct of_device_id ks8851_ml_dt_ids[] = {
-	{ .compatible = "micrel,ks8851-mll" },
-	{ /* sentinel */ }
-};
-MODULE_DEVICE_TABLE(of, ks8851_ml_dt_ids);
-#endif
-
-static int ks8851_probe(struct platform_device *pdev)
-{
-	int err;
-	struct net_device *netdev;
-	struct ks_net *ks;
-	u16 id, data;
-	const char *mac;
-
-	netdev = alloc_etherdev(sizeof(struct ks_net));
-	if (!netdev)
-		return -ENOMEM;
-
-	SET_NETDEV_DEV(netdev, &pdev->dev);
-
-	ks = netdev_priv(netdev);
-	ks->netdev = netdev;
-
-	ks->hw_addr = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(ks->hw_addr)) {
-		err = PTR_ERR(ks->hw_addr);
-		goto err_free;
-	}
-
-	ks->hw_addr_cmd = devm_platform_ioremap_resource(pdev, 1);
-	if (IS_ERR(ks->hw_addr_cmd)) {
-		err = PTR_ERR(ks->hw_addr_cmd);
-		goto err_free;
-	}
-
-	netdev->irq = platform_get_irq(pdev, 0);
-
-	if ((int)netdev->irq < 0) {
-		err = netdev->irq;
-		goto err_free;
-	}
-
-	ks->pdev = pdev;
-
-	mutex_init(&ks->lock);
-	spin_lock_init(&ks->statelock);
-
-	netdev->netdev_ops = &ks_netdev_ops;
-	netdev->ethtool_ops = &ks_ethtool_ops;
-
-	/* setup mii state */
-	ks->mii.dev             = netdev;
-	ks->mii.phy_id          = 1,
-	ks->mii.phy_id_mask     = 1;
-	ks->mii.reg_num_mask    = 0xf;
-	ks->mii.mdio_read       = ks_phy_read;
-	ks->mii.mdio_write      = ks_phy_write;
-
-	netdev_info(netdev, "message enable is %d\n", msg_enable);
-	/* set the default message enable */
-	ks->msg_enable = netif_msg_init(msg_enable, (NETIF_MSG_DRV |
-						     NETIF_MSG_PROBE |
-						     NETIF_MSG_LINK));
-	ks_read_config(ks);
-
-	/* simple check for a valid chip being connected to the bus */
-	if ((ks_rdreg16(ks, KS_CIDER) & ~CIDER_REV_MASK) != CIDER_ID) {
-		netdev_err(netdev, "failed to read device ID\n");
-		err = -ENODEV;
-		goto err_free;
-	}
-
-	if (ks_read_selftest(ks)) {
-		netdev_err(netdev, "failed to read device ID\n");
-		err = -ENODEV;
-		goto err_free;
-	}
-
-	err = register_netdev(netdev);
-	if (err)
-		goto err_free;
-
-	platform_set_drvdata(pdev, netdev);
-
-	ks_soft_reset(ks, GRR_GSR);
-	ks_hw_init(ks);
-	ks_disable_qmu(ks);
-	ks_setup(ks);
-	ks_setup_int(ks);
-
-	data = ks_rdreg16(ks, KS_OBCR);
-	ks_wrreg16(ks, KS_OBCR, data | OBCR_ODS_16mA);
-
-	/* overwriting the default MAC address */
-	if (pdev->dev.of_node) {
-		mac = of_get_mac_address(pdev->dev.of_node);
-		if (!IS_ERR(mac))
-			ether_addr_copy(ks->mac_addr, mac);
-	} else {
-		struct ks8851_mll_platform_data *pdata;
-
-		pdata = dev_get_platdata(&pdev->dev);
-		if (!pdata) {
-			netdev_err(netdev, "No platform data\n");
-			err = -ENODEV;
-			goto err_pdata;
-		}
-		memcpy(ks->mac_addr, pdata->mac_addr, ETH_ALEN);
-	}
-	if (!is_valid_ether_addr(ks->mac_addr)) {
-		/* Use random MAC address if none passed */
-		eth_random_addr(ks->mac_addr);
-		netdev_info(netdev, "Using random mac address\n");
-	}
-	netdev_info(netdev, "Mac address is: %pM\n", ks->mac_addr);
-
-	memcpy(netdev->dev_addr, ks->mac_addr, ETH_ALEN);
-
-	ks_set_mac(ks, netdev->dev_addr);
-
-	id = ks_rdreg16(ks, KS_CIDER);
-
-	netdev_info(netdev, "Found chip, family: 0x%x, id: 0x%x, rev: 0x%x\n",
-		    (id >> 8) & 0xff, (id >> 4) & 0xf, (id >> 1) & 0x7);
-	return 0;
-
-err_pdata:
-	unregister_netdev(netdev);
-err_free:
-	free_netdev(netdev);
-	return err;
-}
-
-static int ks8851_remove(struct platform_device *pdev)
-{
-	struct net_device *netdev = platform_get_drvdata(pdev);
-
-	unregister_netdev(netdev);
-	free_netdev(netdev);
-	return 0;
-
-}
-
-static struct platform_driver ks8851_platform_driver = {
-	.driver = {
-		.name = DRV_NAME,
-		.of_match_table	= of_match_ptr(ks8851_ml_dt_ids),
-	},
-	.probe = ks8851_probe,
-	.remove = ks8851_remove,
-};
-
-module_platform_driver(ks8851_platform_driver);
-
-MODULE_DESCRIPTION("KS8851 MLL Network driver");
-MODULE_AUTHOR("David Choi <david.choi@micrel.com>");
-MODULE_LICENSE("GPL");
-module_param_named(message, msg_enable, int, 0);
-MODULE_PARM_DESC(message, "Message verbosity level (0=none, 31=all)");
-
-- 
2.25.1

Re: [PATCH 14/14] net: ks8851: Remove ks8851_mll.c

[Lukas Wunner]

On Tue, Mar 24, 2020 at 12:43:03AM +0100, Marek Vasut wrote:
> The ks8851_mll.c is replaced by ks8851_par.c, which is using common code
> from ks8851.c, just like ks8851_spi.c . Remove this old ad-hoc driver.

Hm, have you checked whether ks8851_mll.c contains functionality
that is currently missing in ks8851.c and which is worth salvaging?

Thanks,

Lukas

Re: [PATCH 14/14] net: ks8851: Remove ks8851_mll.c

[Marek Vasut]

On 3/24/20 3:08 PM, Lukas Wunner wrote:
> On Tue, Mar 24, 2020 at 12:43:03AM +0100, Marek Vasut wrote:
>> The ks8851_mll.c is replaced by ks8851_par.c, which is using common code
>> from ks8851.c, just like ks8851_spi.c . Remove this old ad-hoc driver.
> 
> Hm, have you checked whether ks8851_mll.c contains functionality
> that is currently missing in ks8851.c and which is worth salvaging?

There's 8bit and 32bit bus support. The former was broken and the later
I don't think is even supported by any chip in existence.

btw is there something which has the KS8851 SPI option, so I can test
that one too ?