Re: [RFC PATCH 1/2] net: macb: Add CAP to disable hardware TX checksum offloading
On 25/05/2018 at 23:44, Jennifer Dahm wrote:
Certain PHYs have significant bugs in their TX checksum offloading
that cannot be solved in software. In order to accommodate these PHYS,
add a CAP to disable this hardware.
Signed-off-by: Jennifer Dahm
---
drivers/net/ethernet/cadence/macb.h | 1 +
drivers/net/ethernet/cadence/macb_main.c | 8 ++--
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/cadence/macb.h
b/drivers/net/ethernet/cadence/macb.h
index 8665982..6b85e97 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -635,6 +635,7 @@
#define MACB_CAPS_USRIO_DISABLED 0x0010
#define MACB_CAPS_JUMBO 0x0020
#define MACB_CAPS_GEM_HAS_PTP 0x0040
+#define MACB_CAPS_DISABLE_TX_HW_CSUM 0x0080
Nitpicking: "DISABLED" tends to be at the end of the name...
#define MACB_CAPS_FIFO_MODE 0x1000
#define MACB_CAPS_GIGABIT_MODE_AVAILABLE 0x2000
#define MACB_CAPS_SG_DISABLED 0x4000
diff --git a/drivers/net/ethernet/cadence/macb_main.c
b/drivers/net/ethernet/cadence/macb_main.c
index 3e93df5..a5d564b 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -3360,8 +3360,12 @@ static int macb_init(struct platform_device *pdev)
dev->hw_features |= MACB_NETIF_LSO;
/* Checksum offload is only available on gem with packet buffer */
- if (macb_is_gem(bp) && !(bp->caps & MACB_CAPS_FIFO_MODE))
- dev->hw_features |= NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
+ if (macb_is_gem(bp) && !(bp->caps & MACB_CAPS_FIFO_MODE)) {
+ if (!(bp->caps & MACB_CAPS_DISABLE_TX_HW_CSUM))
Why not the other way around? negating a "disabled" feature is always
challenge ;-)
+ dev->hw_features |= NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
+ else
+ dev->hw_features |= NETIF_F_RXCSUM;
+ }
if (bp->caps & MACB_CAPS_SG_DISABLED)
dev->hw_features &= ~NETIF_F_SG;
dev->features = dev->hw_features;
--
Nicolas Ferre
Re: [RFC PATCH 2/2] net: macb: Disable TX checksum offloading on all Zynq
On 25/05/2018 at 23:44, Jennifer Dahm wrote:
The Zynq ethernet hardware has checksum offloading bugs that cause
small UDP packets (<= 2 bytes) to be sent with an incorrect checksum
(0x) and forwarded UDP packets to be re-checksummed, which is
illegal behavior. The best solution we have right now is to disable
hardware TX checksum offloading entirely.
Signed-off-by: Jennifer Dahm
Adding some xilinx people I know...
---
drivers/net/ethernet/cadence/macb_main.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/cadence/macb_main.c
b/drivers/net/ethernet/cadence/macb_main.c
index a5d564b..e8cc68a 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -3807,7 +3807,8 @@ static const struct macb_config zynqmp_config = {
};
static const struct macb_config zynq_config = {
- .caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_NO_GIGABIT_HALF,
+ .caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_NO_GIGABIT_HALF
+ | MACB_CAPS_DISABLE_TX_HW_CSUM,
.dma_burst_length = 16,
.clk_init = macb_clk_init,
.init = macb_init,
--
Nicolas Ferre
Re: [RFC PATCH 0/2] net: macb: Disable TX checksum offloading on all Zynq
strlen(message) * sizeof(char); frame_len = sizeof(struct custom_udp) + message_len; frame = malloc(frame_len); frame->s_port = htons(0); frame->d_port = htons(port); frame->length = htons(frame_len); frame->check = htons(0xABCD); memmove(frame->data, message, message_len); ret = write(sockfd, frame, frame_len); free(frame); return ret; } Thanks a lot for the detailed testing procedure. It will definitively help us reproduce the bug. As a conclusion, I would say that we can use this patch as a last resort if we are sure that: 1/ the driver doesn't do weird things with TX CSUM in the code 2/ the hw is unable to handle these particular cases. Best regards, Nicolas ``` Jennifer Dahm (1): net/macb: Disable TX checksum offloading on all Zynq-7000 drivers/net/ethernet/cadence/macb.h | 1 + drivers/net/ethernet/cadence/macb_main.c | 11 ++++--- 2 files changed, 9 insertions(+), 3 deletions(-) -- Nicolas Ferre
Re: [PATCH v3 2/2] net: macb: Try to retrieve MAC addess from nvmem provider
On 27/03/2018 at 11:52, Mike Looijmans wrote:
Call of_get_nvmem_mac_address() to fetch the MAC address from an nvmem
cell, if one is provided in the device tree. This allows the address to
be stored in an I2C EEPROM device for example.
Signed-off-by: Mike Looijmans <mike.looijm...@topic.nl>
For this part:
Acked-by: Nicolas Ferre <nicolas.fe...@microchip.com>
---
drivers/net/ethernet/cadence/macb_main.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/cadence/macb_main.c
b/drivers/net/ethernet/cadence/macb_main.c
index e84afcf..eabe14f 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -3950,10 +3950,16 @@ static int macb_probe(struct platform_device *pdev)
dev->max_mtu = ETH_DATA_LEN;
mac = of_get_mac_address(np);
- if (mac)
+ if (mac) {
ether_addr_copy(bp->dev->dev_addr, mac);
- else
- macb_get_hwaddr(bp);
+ } else {
+ err = of_get_nvmem_mac_address(np, bp->dev->dev_addr);
+ if (err) {
+ if (err == -EPROBE_DEFER)
+ goto err_out_free_netdev;
+ macb_get_hwaddr(bp);
+ }
+ }
err = of_get_phy_mode(np);
if (err < 0) {
--
Nicolas Ferre
Re: [PATCH 2/2] net: macb: kill useless use of list_empty()
On 05/12/2017 at 21:18, Julia Cartwright wrote:
> The list_for_each_entry() macro already handles the case where the list
> is empty (by not executing the loop body). It's not necessary to handle
> this case specially, so stop doing so.
>
> Cc: Rafal Ozieblo <raf...@cadence.com>
> Signed-off-by: Julia Cartwright <ju...@ni.com>
> ---
> This is an additional cleanup patch found when looking at this code.
>
>Julia
Acked-by: Nicolas Ferre <nicolas.fe...@microchip.com>
Thanks
>
> drivers/net/ethernet/cadence/macb_main.c | 34
>
> 1 file changed, 13 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb_main.c
> b/drivers/net/ethernet/cadence/macb_main.c
> index e7ef104a077d..3643c6ad2322 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -2815,25 +2815,22 @@ static int gem_add_flow_filter(struct net_device
> *netdev,
> spin_lock_irqsave(>rx_fs_lock, flags);
>
> /* find correct place to add in list */
> - if (list_empty(>rx_fs_list.list))
> - list_add(>list, >rx_fs_list.list);
> - else {
> - list_for_each_entry(item, >rx_fs_list.list, list) {
> - if (item->fs.location > newfs->fs.location) {
> - list_add_tail(>list, >list);
> - added = true;
> - break;
> - } else if (item->fs.location == fs->location) {
> - netdev_err(netdev, "Rule not added: location %d
> not free!\n",
> - fs->location);
> - ret = -EBUSY;
> - goto err;
> - }
> + list_for_each_entry(item, >rx_fs_list.list, list) {
> + if (item->fs.location > newfs->fs.location) {
> + list_add_tail(>list, >list);
> + added = true;
> + break;
> + } else if (item->fs.location == fs->location) {
> + netdev_err(netdev, "Rule not added: location %d not
> free!\n",
> + fs->location);
> + ret = -EBUSY;
> + goto err;
> }
> - if (!added)
> - list_add_tail(>list, >rx_fs_list.list);
> }
>
> + if (!added)
> + list_add_tail(>list, >rx_fs_list.list);
> +
> gem_prog_cmp_regs(bp, fs);
> bp->rx_fs_list.count++;
> /* enable filtering if NTUPLE on */
> @@ -2859,11 +2856,6 @@ static int gem_del_flow_filter(struct net_device
> *netdev,
>
> spin_lock_irqsave(>rx_fs_lock, flags);
>
> - if (list_empty(>rx_fs_list.list)) {
> - spin_unlock_irqrestore(>rx_fs_lock, flags);
> - return -EINVAL;
> - }
> -
> list_for_each_entry(item, >rx_fs_list.list, list) {
> if (item->fs.location == cmd->fs.location) {
> /* disable screener regs for the flow entry */
>
--
Nicolas Ferre
Re: [PATCH 1/2] net: macb: reduce scope of rx_fs_lock-protected regions
On 05/12/2017 at 21:17, Julia Cartwright wrote:
> Commit ae8223de3df5 ("net: macb: Added support for RX filtering")
> introduces a lock, rx_fs_lock which is intended to protect the list of
> rx_flow items and synchronize access to the hardware rx filtering
> registers.
>
> However, the region protected by this lock is overscoped, unnecessarily
> including things like slab allocation. Reduce this lock scope to only
> include operations which must be performed atomically: list traversal,
> addition, and removal, and hitting the macb filtering registers.
>
> This fixes the use of kmalloc w/ GFP_KERNEL in atomic context.
>
> Fixes: ae8223de3df5 ("net: macb: Added support for RX filtering")
> Cc: Rafal Ozieblo <raf...@cadence.com>
> Cc: Julia Lawall <julia.law...@lip6.fr>
> Signed-off-by: Julia Cartwright <ju...@ni.com>
> ---
> While Julia Lawall's cocci-generated patch fixes the problem, the right
> solution is to obviate the problem altogether.
>
> Thanks,
>The Other Julia
Julia,
Thanks for your patch, it seems good indeed. Here is my:
Acked-by: Nicolas Ferre <nicolas.fe...@microchip.com>
As the patch by Julia L. is already in net-next, I suspect that you
would need to add a kind of revert patch if we want to come back to a
more usual GFP_KERNEL for the kmalloc.
Best regards,
Nicolas
> drivers/net/ethernet/cadence/macb_main.c | 20 ++--
> 1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb_main.c
> b/drivers/net/ethernet/cadence/macb_main.c
> index c5fa87cdc6c4..e7ef104a077d 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -2796,6 +2796,7 @@ static int gem_add_flow_filter(struct net_device
> *netdev,
> struct macb *bp = netdev_priv(netdev);
> struct ethtool_rx_flow_spec *fs = >fs;
> struct ethtool_rx_fs_item *item, *newfs;
> + unsigned long flags;
> int ret = -EINVAL;
> bool added = false;
>
> @@ -2811,6 +2812,8 @@ static int gem_add_flow_filter(struct net_device
> *netdev,
> htonl(fs->h_u.tcp_ip4_spec.ip4dst),
> htons(fs->h_u.tcp_ip4_spec.psrc),
> htons(fs->h_u.tcp_ip4_spec.pdst));
>
> + spin_lock_irqsave(>rx_fs_lock, flags);
> +
> /* find correct place to add in list */
> if (list_empty(>rx_fs_list.list))
> list_add(>list, >rx_fs_list.list);
> @@ -2837,9 +2840,11 @@ static int gem_add_flow_filter(struct net_device
> *netdev,
> if (netdev->features & NETIF_F_NTUPLE)
> gem_enable_flow_filters(bp, 1);
>
> + spin_unlock_irqrestore(>rx_fs_lock, flags);
> return 0;
>
> err:
> + spin_unlock_irqrestore(>rx_fs_lock, flags);
> kfree(newfs);
> return ret;
> }
> @@ -2850,9 +2855,14 @@ static int gem_del_flow_filter(struct net_device
> *netdev,
> struct macb *bp = netdev_priv(netdev);
> struct ethtool_rx_fs_item *item;
> struct ethtool_rx_flow_spec *fs;
> + unsigned long flags;
>
> - if (list_empty(>rx_fs_list.list))
> + spin_lock_irqsave(>rx_fs_lock, flags);
> +
> + if (list_empty(>rx_fs_list.list)) {
> + spin_unlock_irqrestore(>rx_fs_lock, flags);
> return -EINVAL;
> + }
>
> list_for_each_entry(item, >rx_fs_list.list, list) {
> if (item->fs.location == cmd->fs.location) {
> @@ -2869,12 +2879,14 @@ static int gem_del_flow_filter(struct net_device
> *netdev,
> gem_writel_n(bp, SCRT2, fs->location, 0);
>
> list_del(>list);
> - kfree(item);
> bp->rx_fs_list.count--;
> + spin_unlock_irqrestore(>rx_fs_lock, flags);
> + kfree(item);
> return 0;
> }
> }
>
> + spin_unlock_irqrestore(>rx_fs_lock, flags);
> return -EINVAL;
> }
>
> @@ -2943,11 +2955,8 @@ static int gem_get_rxnfc(struct net_device *netdev,
> struct ethtool_rxnfc *cmd,
> static int gem_set_rxnfc(struct net_device *netdev, struct ethtool_rxnfc
> *cmd)
> {
> struct macb *bp = netdev_priv(netdev);
> - unsigned long flags;
> int ret;
>
> - spin_lock_irqsave(>rx_fs_lock, flags);
> -
> switch (cmd->cmd) {
> case ETHTOOL_SRXCLSRLINS:
> if ((cmd->fs.location >= bp->max_tuples)
> @@ -2966,7 +2975,6 @@ static int gem_set_rxnfc(struct net_device *netdev,
> struct ethtool_rxnfc *cmd)
> ret = -EOPNOTSUPP;
> }
>
> - spin_unlock_irqrestore(>rx_fs_lock, flags);
> return ret;
> }
>
>
--
Nicolas Ferre
Re: [PATCH v2 2/2] net: macb: add of_node_put to error paths
On 08/11/2017 at 09:56, Michael Grzeschik wrote: > We add the call of_node_put(bp->phy_node) to all associated error > paths for memory clean up. > > Signed-off-by: Michael Grzeschik <m.grzesc...@pengutronix.de> Acked-by: Nicolas Ferre <nicolas.fe...@microchip.com> > --- > v2: removed extra of_node_put from macb_remove > > drivers/net/ethernet/cadence/macb_main.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/net/ethernet/cadence/macb_main.c > b/drivers/net/ethernet/cadence/macb_main.c > index b7846d6e9234e..0f24ca5a24b53 100644 > --- a/drivers/net/ethernet/cadence/macb_main.c > +++ b/drivers/net/ethernet/cadence/macb_main.c > @@ -611,6 +611,7 @@ static int macb_mii_init(struct macb *bp) > err_out_unregister_bus: > mdiobus_unregister(bp->mii_bus); > err_out_free_mdiobus: > + of_node_put(bp->phy_node); > if (np && of_phy_is_fixed_link(np)) > of_phy_deregister_fixed_link(np); > mdiobus_free(bp->mii_bus); > @@ -3554,6 +3555,7 @@ static int macb_probe(struct platform_device *pdev) > err_out_unregister_mdio: > phy_disconnect(dev->phydev); > mdiobus_unregister(bp->mii_bus); > + of_node_put(bp->phy_node); > if (np && of_phy_is_fixed_link(np)) > of_phy_deregister_fixed_link(np); > mdiobus_free(bp->mii_bus); > -- Nicolas Ferre
Re: [PATCH v2 1/2] net: macb: add of_phy_deregister_fixed_link to error paths
On 08/11/2017 at 09:56, Michael Grzeschik wrote:
> We add the call of_phy_deregister_fixed_link to all associated
> error paths for memory clean up.
>
> Signed-off-by: Michael Grzeschik <m.grzesc...@pengutronix.de>
Acked-by: Nicolas Ferre <nicolas.fe...@microchip.com>
> ---
> v2: removed extra parenthesis
>
> drivers/net/ethernet/cadence/macb_main.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/net/ethernet/cadence/macb_main.c
> b/drivers/net/ethernet/cadence/macb_main.c
> index 6df2cad61647a..b7846d6e9234e 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -611,6 +611,8 @@ static int macb_mii_init(struct macb *bp)
> err_out_unregister_bus:
> mdiobus_unregister(bp->mii_bus);
> err_out_free_mdiobus:
> + if (np && of_phy_is_fixed_link(np))
> + of_phy_deregister_fixed_link(np);
> mdiobus_free(bp->mii_bus);
> err_out:
> return err;
> @@ -3552,6 +3554,8 @@ static int macb_probe(struct platform_device *pdev)
> err_out_unregister_mdio:
> phy_disconnect(dev->phydev);
> mdiobus_unregister(bp->mii_bus);
> + if (np && of_phy_is_fixed_link(np))
> + of_phy_deregister_fixed_link(np);
> mdiobus_free(bp->mii_bus);
>
> /* Shutdown the PHY if there is a GPIO reset */
> @@ -3574,6 +3578,7 @@ static int macb_remove(struct platform_device *pdev)
> {
> struct net_device *dev;
> struct macb *bp;
> + struct device_node *np = pdev->dev.of_node;
>
> dev = platform_get_drvdata(pdev);
>
> @@ -3582,6 +3587,8 @@ static int macb_remove(struct platform_device *pdev)
> if (dev->phydev)
> phy_disconnect(dev->phydev);
> mdiobus_unregister(bp->mii_bus);
> + if (np && of_phy_is_fixed_link(np))
> + of_phy_deregister_fixed_link(np);
> dev->phydev = NULL;
> mdiobus_free(bp->mii_bus);
>
>
--
Nicolas Ferre
Re: [PATCH v2] net: macb: add of_node_put to error paths
On 07/11/2017 at 10:59, Michael Grzeschik wrote: > We add the call of_node_put(bp->phy_node) to all associated error > paths for memory clean up. > > Signed-off-by: Michael Grzeschik <m.grzesc...@pengutronix.de> Thanks for your quick update: Acked-by: Nicolas Ferre <nicolas.fe...@microchip.com> Best regards, Nicolas > --- > v1 -> v2: removed extra of_node_put from macb_remove > > drivers/net/ethernet/cadence/macb_main.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/net/ethernet/cadence/macb_main.c > b/drivers/net/ethernet/cadence/macb_main.c > index 2c2acd011329a..c66df096e81a5 100644 > --- a/drivers/net/ethernet/cadence/macb_main.c > +++ b/drivers/net/ethernet/cadence/macb_main.c > @@ -611,6 +611,7 @@ static int macb_mii_init(struct macb *bp) > err_out_unregister_bus: > mdiobus_unregister(bp->mii_bus); > err_out_free_mdiobus: > + of_node_put(bp->phy_node); > if ((np) && (of_phy_is_fixed_link(np))) > of_phy_deregister_fixed_link(np); > mdiobus_free(bp->mii_bus); > @@ -3554,6 +3555,7 @@ static int macb_probe(struct platform_device *pdev) > err_out_unregister_mdio: > phy_disconnect(dev->phydev); > mdiobus_unregister(bp->mii_bus); > + of_node_put(bp->phy_node); > if ((np) && (of_phy_is_fixed_link(np))) > of_phy_deregister_fixed_link(np); > mdiobus_free(bp->mii_bus); > -- Nicolas Ferre
Re: [PATCH 2/2] net: macb: add of_node_put to error paths
On 06/11/2017 at 12:10, Michael Grzeschik wrote: > We add the call of_node_put(bp->phy_node) to all associated error > paths for memory clean up. > > Signed-off-by: Michael Grzeschik <m.grzesc...@pengutronix.de> > --- > drivers/net/ethernet/cadence/macb_main.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/net/ethernet/cadence/macb_main.c > b/drivers/net/ethernet/cadence/macb_main.c > index 2c2acd011329a..2698a1fde39c7 100644 > --- a/drivers/net/ethernet/cadence/macb_main.c > +++ b/drivers/net/ethernet/cadence/macb_main.c > @@ -611,6 +611,7 @@ static int macb_mii_init(struct macb *bp) > err_out_unregister_bus: > mdiobus_unregister(bp->mii_bus); > err_out_free_mdiobus: > + of_node_put(bp->phy_node); okay > if ((np) && (of_phy_is_fixed_link(np))) > of_phy_deregister_fixed_link(np); > mdiobus_free(bp->mii_bus); > @@ -3554,6 +3555,7 @@ static int macb_probe(struct platform_device *pdev) > err_out_unregister_mdio: > phy_disconnect(dev->phydev); > mdiobus_unregister(bp->mii_bus); > + of_node_put(bp->phy_node); yes > if ((np) && (of_phy_is_fixed_link(np))) > of_phy_deregister_fixed_link(np); > mdiobus_free(bp->mii_bus); > @@ -3587,6 +3589,7 @@ static int macb_remove(struct platform_device *pdev) > if (dev->phydev) > phy_disconnect(dev->phydev); > mdiobus_unregister(bp->mii_bus); > + of_node_put(bp->phy_node); Isn't this call already done some lines below, just before "free_netdev(dev);"? > if ((np) && (of_phy_is_fixed_link(np))) > of_phy_deregister_fixed_link(np); > dev->phydev = NULL; > Thanks for your patch. Regards, -- Nicolas Ferre
Re: [PATCH 1/2] net: macb: add of_phy_deregister_fixed_link to error paths
On 06/11/2017 at 12:10, Michael Grzeschik wrote:
> We add the call of_phy_deregister_fixed_link to all associated
> error paths for memory clean up.
>
> Signed-off-by: Michael Grzeschik <m.grzesc...@pengutronix.de>
Acked-by: Nicolas Ferre <nicolas.fe...@microchip.com>
Thanks a lot for your quick answer!
Best regards,
Nicolas
> ---
> drivers/net/ethernet/cadence/macb_main.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/net/ethernet/cadence/macb_main.c
> b/drivers/net/ethernet/cadence/macb_main.c
> index 6df2cad61647a..2c2acd011329a 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -611,6 +611,8 @@ static int macb_mii_init(struct macb *bp)
> err_out_unregister_bus:
> mdiobus_unregister(bp->mii_bus);
> err_out_free_mdiobus:
> + if ((np) && (of_phy_is_fixed_link(np)))
> + of_phy_deregister_fixed_link(np);
> mdiobus_free(bp->mii_bus);
> err_out:
> return err;
> @@ -3552,6 +3554,8 @@ static int macb_probe(struct platform_device *pdev)
> err_out_unregister_mdio:
> phy_disconnect(dev->phydev);
> mdiobus_unregister(bp->mii_bus);
> + if ((np) && (of_phy_is_fixed_link(np)))
> + of_phy_deregister_fixed_link(np);
> mdiobus_free(bp->mii_bus);
>
> /* Shutdown the PHY if there is a GPIO reset */
> @@ -3574,6 +3578,7 @@ static int macb_remove(struct platform_device *pdev)
> {
> struct net_device *dev;
> struct macb *bp;
> + struct device_node *np = pdev->dev.of_node;
>
> dev = platform_get_drvdata(pdev);
>
> @@ -3582,6 +3587,8 @@ static int macb_remove(struct platform_device *pdev)
> if (dev->phydev)
> phy_disconnect(dev->phydev);
> mdiobus_unregister(bp->mii_bus);
> + if ((np) && (of_phy_is_fixed_link(np)))
> + of_phy_deregister_fixed_link(np);
> dev->phydev = NULL;
> mdiobus_free(bp->mii_bus);
>
>
--
Nicolas Ferre
Re: [PATCH v2] net: macb: add fixed-link node support
+ }
> + bp->phy_node = of_node_get(np);
>
> - /* fallback to standard phy registration if no phy were
> - * found during dt phy registration
> - */
> - if (!err && !phy_find_first(bp->mii_bus)) {
> - for (i = 0; i < PHY_MAX_ADDR; i++) {
> - struct phy_device *phydev;
> -
> - phydev = mdiobus_scan(bp->mii_bus, i);
> - if (IS_ERR(phydev) &&
> - PTR_ERR(phydev) != -ENODEV) {
> - err = PTR_ERR(phydev);
> - break;
> + err = mdiobus_register(bp->mii_bus);
> + } else {
> + /* try dt phy registration */
> + err = of_mdiobus_register(bp->mii_bus, np);
> +
> + /* fallback to standard phy registration if no phy were
> + * found during dt phy registration
> + */
> + if (!err && !phy_find_first(bp->mii_bus)) {
> + for (i = 0; i < PHY_MAX_ADDR; i++) {
> + struct phy_device *phydev;
> +
> + phydev = mdiobus_scan(bp->mii_bus, i);
> + if (IS_ERR(phydev) &&
> + PTR_ERR(phydev) != -ENODEV) {
> + err = PTR_ERR(phydev);
> + break;
> + }
> }
> - }
>
> - if (err)
> - goto err_out_unregister_bus;
> + if (err)
> + goto err_out_unregister_bus;
> + }
> }
> } else {
> for (i = 0; i < PHY_MAX_ADDR; i++)
> @@ -3438,6 +3457,7 @@ static int macb_remove(struct platform_device *pdev)
> clk_disable_unprepare(bp->hclk);
> clk_disable_unprepare(bp->pclk);
> clk_disable_unprepare(bp->rx_clk);
> + of_node_put(bp->phy_node);
> free_netdev(dev);
> }
>
> diff --git a/drivers/net/ethernet/cadence/macb.h
> b/drivers/net/ethernet/cadence/macb.h
> index ec037b0fa2a4d..2510661102bad 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -930,6 +930,7 @@ struct macb {
> struct macb_or_gem_ops macbgem_ops;
>
> struct mii_bus *mii_bus;
> + struct device_node *phy_node;
> int link;
> int speed;
> int duplex;
>
--
Nicolas Ferre
[PATCH] net: macb: Adding Support for Jumbo Frames up to 10240 Bytes in SAMA5D3
From: vishnuvardhan <vardhanraj4...@gmail.com>
As per the SAMA5D3 device specification it supports Jumbo frames.
But the suggested flag and length of bytes it supports was not updated
in this driver config_structure.
The maximum jumbo frames the device supports :
10240 bytes as per the device spec.
While changing the MTU value greater than 1500, it threw error:
sudo ifconfig eth1 mtu 9000
SIOCSIFMTU: Invalid argument
Add this support to driver so that it works as expected and designed.
Signed-off-by: vishnuvardhan <vardhanraj4...@gmail.com>
[nicolas.fe...@microchip.com: modify slightly commit msg]
Signed-off-by: Nicolas Ferre <nicolas.fe...@microchip.com>
---
drivers/net/ethernet/cadence/macb_main.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/cadence/macb_main.c
b/drivers/net/ethernet/cadence/macb_main.c
index e69ebdd65658..26d25749c3e4 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -3314,10 +3314,11 @@ static const struct macb_config sama5d2_config = {
static const struct macb_config sama5d3_config = {
.caps = MACB_CAPS_SG_DISABLED | MACB_CAPS_GIGABIT_MODE_AVAILABLE
- | MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII,
+ | MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII | MACB_CAPS_JUMBO,
.dma_burst_length = 16,
.clk_init = macb_clk_init,
.init = macb_init,
+ .jumbo_max_len = 10240,
};
static const struct macb_config sama5d4_config = {
--
2.9.0
Re: [PATCH][-next] net: macb: remove extraneous return when MACB_EXT_DESC is defined
On 04/07/2017 at 17:09, Colin King wrote:
> From: Colin Ian King <colin.k...@canonical.com>
>
> When macro MACB_EXT_DESC is defined we end up with two identical
> return statements and just one is sufficient. Remove the extra
> return.
>
> Detected by CoverityScan, CID#1449361 ("Structurally dead code")
>
> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
Acked-by: Nicolas Ferre <nicolas.fe...@microchip.com>
> ---
> drivers/net/ethernet/cadence/macb_main.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb_main.c
> b/drivers/net/ethernet/cadence/macb_main.c
> index 41e5711544fc..e69ebdd65658 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -146,7 +146,6 @@ static unsigned int macb_adj_dma_desc_idx(struct macb
> *bp, unsigned int desc_idx
> default:
> break;
> }
> - return desc_idx;
> #endif
> return desc_idx;
> }
>
--
Nicolas Ferre
Re: [PATCH] net: macb: add fixed-link node support
Le 16/06/2017 à 10:55, Michael Grzeschik a écrit :
> In case the MACB is directly connected to a
> non-mdio PHY/device, it should be possible to provide
> a fixed link configuration in the DT.
>
> Signed-off-by: Michael Grzeschik <m.grzesc...@pengutronix.de>
> ---
> drivers/net/ethernet/cadence/macb.c | 21 +
> drivers/net/ethernet/cadence/macb.h | 2 +-
> 2 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb.c
> b/drivers/net/ethernet/cadence/macb.c
> index 91f7492623d3f..91e36efe1d5fb 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -425,6 +425,16 @@ static int macb_mii_probe(struct net_device *dev)
> int phy_irq;
> int ret;
>
> + if (bp->phy_node) {
> + phydev = of_phy_connect(dev, bp->phy_node,
> + _handle_link_change, 0,
> + bp->phy_interface);
> + if (!phydev)
> + return -ENODEV;
> +
> + return 0;
I understand that we can avoid the part with phy_connect_direct() of the
remaining of this function, but what about the way to amend the phy
features with the possibilities of the MAC
(phydev->supported/advertising). The bp fields initialization seem
redundant and handled elsewhere anyway.
> + }
> +
> phydev = phy_find_first(bp->mii_bus);
> if (!phydev) {
> netdev_err(dev, "no PHY found\n");
> @@ -498,6 +508,17 @@ static int macb_mii_init(struct macb *bp)
> dev_set_drvdata(>dev->dev, bp->mii_bus);
>
> np = bp->pdev->dev.of_node;
> + if (of_phy_is_fixed_link(np)) {
For now, we are not sure that the np is != NULL.
The PCI support for this driver does use the pdata.
(Cf: 83a77e9ec415 (net: macb: Added PCI wrapper for Platform Driver.)
).
> + if (of_phy_register_fixed_link(np) < 0) {
> + dev_err(>pdev->dev,
> + "broken fixed-link specification\n");
> + goto err_out_unregister_bus;
> + }
> + bp->phy_node = of_node_get(np);
> +
> + np = NULL;
> + }
> +
> if (np) {
Even if the case np == NULL is catch somewhere in
of_phy_register_fixed_link(), why not put the code here?
> /* try dt phy registration */
> err = of_mdiobus_register(bp->mii_bus, np);
> diff --git a/drivers/net/ethernet/cadence/macb.h
> b/drivers/net/ethernet/cadence/macb.h
> index ec037b0fa2a4d..7efc726e0a113 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -928,7 +928,7 @@ struct macb {
> dma_addr_t rx_buffers_dma;
>
> struct macb_or_gem_ops macbgem_ops;
> -
Nit: no need to remove the empty line.
> + struct device_node *phy_node;
> struct mii_bus *mii_bus;
> int link;
> int speed;
>
Thanks for your patch.
Best regards,
--
Nicolas Ferre
Re: [PATCH] net: macb: fix phy interrupt parsing
Le 26/04/2017 à 12:06, Alexandre Belloni a écrit :
> Since 83a77e9ec415, the phydev irq is explicitly set to PHY_POLL when
> there is no pdata. It doesn't work on DT enabled platforms because the
> phydev irq is already set by libphy before.
>
> Fixes: 83a77e9ec415 ("net: macb: Added PCI wrapper for Platform Driver.")
Means 4.10+
> Signed-off-by: Alexandre Belloni <alexandre.bell...@free-electrons.com>
Acked-by: Nicolas Ferre <nicolas.fe...@microchip.com>
Seems a good candidate for net stable.
Bye,
> ---
> drivers/net/ethernet/cadence/macb.c | 18 ++
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb.c
> b/drivers/net/ethernet/cadence/macb.c
> index e131ecd17ab9..004223a866fe 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -432,15 +432,17 @@ static int macb_mii_probe(struct net_device *dev)
> }
>
> pdata = dev_get_platdata(>pdev->dev);
> - if (pdata && gpio_is_valid(pdata->phy_irq_pin)) {
> - ret = devm_gpio_request(>pdev->dev, pdata->phy_irq_pin,
> - "phy int");
> - if (!ret) {
> - phy_irq = gpio_to_irq(pdata->phy_irq_pin);
> - phydev->irq = (phy_irq < 0) ? PHY_POLL : phy_irq;
> + if (pdata) {
> + if (gpio_is_valid(pdata->phy_irq_pin)) {
> + ret = devm_gpio_request(>pdev->dev,
> + pdata->phy_irq_pin, "phy int");
> + if (!ret) {
> + phy_irq = gpio_to_irq(pdata->phy_irq_pin);
> + phydev->irq = (phy_irq < 0) ? PHY_POLL :
> phy_irq;
> + }
> + } else {
> + phydev->irq = PHY_POLL;
> }
> - } else {
> - phydev->irq = PHY_POLL;
> }
>
> /* attach the mac to the phy */
>
--
Nicolas Ferre
Re: [PATCH net-next 05/14] net: macb: Use net_device_stats from struct net_device
Le 07/04/2017 à 10:17, Tobias Klauser a écrit :
> Instead of using a private copy of struct net_device_stats in struct
> macb, use stats from struct net_device.
I agree with the initiative but I read this in the documentation of this
struct member...
@stats: Statistics struct, which was left as a legacy, use
rtnl_link_stats64 instead
Is it still permitted to use it? Would it be to directly move to the
most up-to-date code?
Regards,
> Cc: Nicolas Ferre <nicolas.fe...@atmel.com>
> Signed-off-by: Tobias Klauser <tklau...@distanz.ch>
> ---
> drivers/net/ethernet/cadence/macb.c | 40
> ++---
> drivers/net/ethernet/cadence/macb.h | 1 -
> 2 files changed, 20 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb.c
> b/drivers/net/ethernet/cadence/macb.c
> index 30606b11b128..5cbd1e7a926a 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -684,8 +684,8 @@ static void macb_tx_error_task(struct work_struct *work)
> netdev_vdbg(bp->dev, "txerr skb %u (data %p) TX
> complete\n",
> macb_tx_ring_wrap(bp, tail),
> skb->data);
> - bp->stats.tx_packets++;
> - bp->stats.tx_bytes += skb->len;
> + bp->dev->stats.tx_packets++;
> + bp->dev->stats.tx_bytes += skb->len;
> }
> } else {
> /* "Buffers exhausted mid-frame" errors may only happen
> @@ -778,8 +778,8 @@ static void macb_tx_interrupt(struct macb_queue *queue)
> netdev_vdbg(bp->dev, "skb %u (data %p) TX
> complete\n",
> macb_tx_ring_wrap(bp, tail),
> skb->data);
> - bp->stats.tx_packets++;
> - bp->stats.tx_bytes += skb->len;
> + bp->dev->stats.tx_packets++;
> + bp->dev->stats.tx_bytes += skb->len;
> }
>
> /* Now we can safely release resources */
> @@ -911,14 +911,14 @@ static int gem_rx(struct macb *bp, int budget)
> if (!(ctrl & MACB_BIT(RX_SOF) && ctrl & MACB_BIT(RX_EOF))) {
> netdev_err(bp->dev,
> "not whole frame pointed by descriptor\n");
> - bp->stats.rx_dropped++;
> + bp->dev->stats.rx_dropped++;
> break;
> }
> skb = bp->rx_skbuff[entry];
> if (unlikely(!skb)) {
> netdev_err(bp->dev,
> "inconsistent Rx descriptor chain\n");
> - bp->stats.rx_dropped++;
> + bp->dev->stats.rx_dropped++;
> break;
> }
> /* now everything is ready for receiving packet */
> @@ -938,8 +938,8 @@ static int gem_rx(struct macb *bp, int budget)
> GEM_BFEXT(RX_CSUM, ctrl) & GEM_RX_CSUM_CHECKED_MASK)
> skb->ip_summed = CHECKSUM_UNNECESSARY;
>
> - bp->stats.rx_packets++;
> - bp->stats.rx_bytes += skb->len;
> + bp->dev->stats.rx_packets++;
> + bp->dev->stats.rx_bytes += skb->len;
>
> #if defined(DEBUG) && defined(VERBOSE_DEBUG)
> netdev_vdbg(bp->dev, "received skb of length %u, csum: %08x\n",
> @@ -984,7 +984,7 @@ static int macb_rx_frame(struct macb *bp, unsigned int
> first_frag,
>*/
> skb = netdev_alloc_skb(bp->dev, len + NET_IP_ALIGN);
> if (!skb) {
> - bp->stats.rx_dropped++;
> + bp->dev->stats.rx_dropped++;
> for (frag = first_frag; ; frag++) {
> desc = macb_rx_desc(bp, frag);
> desc->addr &= ~MACB_BIT(RX_USED);
> @@ -1030,8 +1030,8 @@ static int macb_rx_frame(struct macb *bp, unsigned int
> first_frag,
> __skb_pull(skb, NET_IP_ALIGN);
> skb->protocol = eth_type_trans(skb, bp->dev);
>
> - bp->stats.rx_packets++;
> - bp->stats.rx_bytes += skb->len;
> + bp->dev->stats.rx_packets++;
> + bp->dev->stats.rx_bytes += skb->len;
> ne
Re: [PATCH 4.10-rc3 03/13] net: macb: fix build errors when linux/phy*.h is removed from net/dsa.h
Le 31/01/2017 à 20:18, Russell King a écrit : > drivers/net/ethernet/cadence/macb.h:862:33: sparse: expected ; at end of > declaration > drivers/net/ethernet/cadence/macb.h:862:33: sparse: Expected } at end of > struct-union-enum-specifier > drivers/net/ethernet/cadence/macb.h:862:33: sparse: got phy_interface > drivers/net/ethernet/cadence/macb.h:877:1: sparse: Expected ; at the end of > type declaration > drivers/net/ethernet/cadence/macb.h:877:1: sparse: got } > In file included from drivers/net/ethernet/cadence/macb_pci.c:29:0: > drivers/net/ethernet/cadence/macb.h:862:2: error: unknown type name > 'phy_interface_t' > phy_interface_t phy_interface; > ^~~ > > Add linux/phy.h to macb.h > > Signed-off-by: Russell King <rmk+ker...@armlinux.org.uk> Acked-by: Nicolas Ferre <nicolas.fe...@atmel.com> > --- > drivers/net/ethernet/cadence/macb.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/net/ethernet/cadence/macb.h > b/drivers/net/ethernet/cadence/macb.h > index d67adad67be1..383da8cf5f6d 100644 > --- a/drivers/net/ethernet/cadence/macb.h > +++ b/drivers/net/ethernet/cadence/macb.h > @@ -10,6 +10,8 @@ > #ifndef _MACB_H > #define _MACB_H > > +#include > + > #define MACB_GREGS_NBR 16 > #define MACB_GREGS_VERSION 2 > #define MACB_MAX_QUEUES 8 > -- Nicolas Ferre
Re: [PATCH net-next v2] macb: Common code to enable ptp support for MACB/GEM
Le 27/01/2017 à 11:26, Rafal Ozieblo a écrit :
>> -Original Message-
>> From: Harini Katakam [mailto:harinikatakamli...@gmail.com]
>> Sent: 27 stycznia 2017 06:43
>> Subject: Re: [PATCH net-next v2] macb: Common code to enable ptp support for
>> MACB/GEM
>>
>> Hi Rafal,
>>
>> On Thu, Jan 26, 2017 at 8:45 PM, Rafal Ozieblo <raf...@cadence.com> wrote:
>>>> -Original Message-
>>>> From: Andrei Pistirica [mailto:andrei.pistir...@microchip.com]
>>>> Sent: 19 stycznia 2017 16:56
>>>> Subject: [PATCH net-next v2] macb: Common code to enable ptp support for
>>>> MACB/GEM
>>>>
>>>>
>>>> +static inline bool gem_has_ptp(struct macb *bp)
>>>> +{
>>>> + return !!(bp->caps & MACB_CAPS_GEM_HAS_PTP);
>>>> +}
>>> Why don't you use hardware capabilities here? Would it be better to read it
>>> from hardware instead adding it to many configuration?
>>
>> If you are referring to TSU bit in DCFG5, then we will be relying on
>> Cadence IP's information irrespective of the SoC capability
>> and whether the PTP support was adequate.
>> I think the capability approach gives better control and
>> it is not really much to add.
>>
>> Regards,
>> Harini
>>
> Yes, I'm referring to TSU bit.
> What if SoC contains multiple Cadence GEMs, some with PTP support and others
> without?
Simply define different DT compatibility strings and we're good.
> Relevant will be checking both, hardware capabilities and SoC capabilities
> from "caps" field.
>
--
Nicolas Ferre
Re: [PATCH net-next v2] macb: Common code to enable ptp support for MACB/GEM
Le 27/01/2017 à 06:42, Harini Katakam a écrit :
> Hi Rafal,
>
> On Thu, Jan 26, 2017 at 8:45 PM, Rafal Ozieblo <raf...@cadence.com> wrote:
>>> -Original Message-
>>> From: Andrei Pistirica [mailto:andrei.pistir...@microchip.com]
>>> Sent: 19 stycznia 2017 16:56
>>> Subject: [PATCH net-next v2] macb: Common code to enable ptp support for
>>> MACB/GEM
>>>
>>>
>>> +static inline bool gem_has_ptp(struct macb *bp)
>>> +{
>>> + return !!(bp->caps & MACB_CAPS_GEM_HAS_PTP);
>>> +}
>> Why don't you use hardware capabilities here? Would it be better to read it
>> from hardware instead adding it to many configuration?
>
> If you are referring to TSU bit in DCFG5, then we will be relying on
> Cadence IP's information irrespective of the SoC capability
> and whether the PTP support was adequate.
> I think the capability approach gives better control and
> it is not really much to add.
Yes, absolutely. In fact we already had this discussion and decided that
this capability scheme was giving much more control at low cost.
Regards,
--
Nicolas Ferre
Re: [PATCH net-next v2] macb: Common code to enable ptp support for MACB/GEM
Le 19/01/2017 à 17:07, Nicolas Ferre a écrit :
> Le 19/01/2017 à 08:56, Andrei Pistirica a écrit :
>> This patch does the following:
>> - MACB/GEM-PTP interface
>> - registers and bitfields for TSU
>> - capability flags to enable PTP per platform basis
>>
>> Signed-off-by: Andrei Pistirica <andrei.pistir...@microchip.com>
>
> Acked-by: Nicolas Ferre <nicolas.fe...@atmel.com>
Harini or Rafal, do you plan to review this patch and add your
"Reviewed-by" tags? It can be useful to make this support move forward.
Regards,
>> ---
>> Patch history:
>>
>> Version 1:
>> This is just the common code for MACB/GEM-PTP support.
>> Code is based on the comments related to the following patch series:
>> - [RFC PATCH net-next v1-to-4 1/2] macb: Add 1588 support in Cadence GEM
>> - [RFC PATCH net-next v1-to-4 2/2] macb: Enable 1588 support in SAMA5Dx
>> platforms
>>
>> Version 2:
>> - Cosmetic changes and PTP capability flag changed doe to overlapping with
>> JUMBO.
>>
>> Note: Patch on net-next: January 19.
>>
>> drivers/net/ethernet/cadence/macb.c | 32 +++-
>> drivers/net/ethernet/cadence/macb.h | 74
>> +
>> 2 files changed, 104 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/cadence/macb.c
>> b/drivers/net/ethernet/cadence/macb.c
>> index c0fb80a..ff1e648 100644
>> --- a/drivers/net/ethernet/cadence/macb.c
>> +++ b/drivers/net/ethernet/cadence/macb.c
>> @@ -2085,6 +2085,9 @@ static int macb_open(struct net_device *dev)
>>
>> netif_tx_start_all_queues(dev);
>>
>> +if (bp->ptp_info)
>> +bp->ptp_info->ptp_init(dev);
>> +
>> return 0;
>> }
>>
>> @@ -2106,6 +2109,9 @@ static int macb_close(struct net_device *dev)
>>
>> macb_free_consistent(bp);
>>
>> +if (bp->ptp_info)
>> +bp->ptp_info->ptp_remove(dev);
>> +
>> return 0;
>> }
>>
>> @@ -2379,6 +2385,17 @@ static int macb_set_ringparam(struct net_device
>> *netdev,
>> return 0;
>> }
>>
>> +static int macb_get_ts_info(struct net_device *netdev,
>> +struct ethtool_ts_info *info)
>> +{
>> +struct macb *bp = netdev_priv(netdev);
>> +
>> +if (bp->ptp_info)
>> +return bp->ptp_info->get_ts_info(netdev, info);
>> +
>> +return ethtool_op_get_ts_info(netdev, info);
>> +}
>> +
>> static const struct ethtool_ops macb_ethtool_ops = {
>> .get_regs_len = macb_get_regs_len,
>> .get_regs = macb_get_regs,
>> @@ -2396,7 +2413,7 @@ static const struct ethtool_ops gem_ethtool_ops = {
>> .get_regs_len = macb_get_regs_len,
>> .get_regs = macb_get_regs,
>> .get_link = ethtool_op_get_link,
>> -.get_ts_info= ethtool_op_get_ts_info,
>> +.get_ts_info= macb_get_ts_info,
>> .get_ethtool_stats = gem_get_ethtool_stats,
>> .get_strings= gem_get_ethtool_strings,
>> .get_sset_count = gem_get_sset_count,
>> @@ -2409,6 +2426,7 @@ static const struct ethtool_ops gem_ethtool_ops = {
>> static int macb_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
>> {
>> struct phy_device *phydev = dev->phydev;
>> +struct macb *bp = netdev_priv(dev);
>>
>> if (!netif_running(dev))
>> return -EINVAL;
>> @@ -2416,7 +2434,17 @@ static int macb_ioctl(struct net_device *dev, struct
>> ifreq *rq, int cmd)
>> if (!phydev)
>> return -ENODEV;
>>
>> -return phy_mii_ioctl(phydev, rq, cmd);
>> +if (!bp->ptp_info)
>> +return phy_mii_ioctl(phydev, rq, cmd);
>> +
>> +switch (cmd) {
>> +case SIOCSHWTSTAMP:
>> +return bp->ptp_info->set_hwtst(dev, rq, cmd);
>> +case SIOCGHWTSTAMP:
>> +return bp->ptp_info->get_hwtst(dev, rq);
>> +default:
>> +return phy_mii_ioctl(phydev, rq, cmd);
>> +}
>> }
>>
>> static int macb_set_features(struct net_device *netdev,
>> diff --git a/drivers/net/ethernet/cadence/macb.h
>> b/drivers/net/ethernet/cadence/macb.h
>> index d67adad..94ddedd 100644
>> --- a/drivers/net/ethernet/cadence/macb.h
>> +++ b/drivers/net/ethernet/cadence/macb.h
>> @@ -131,6 +131,20 @@
>>
Re: [PATCH net-next v2] macb: Common code to enable ptp support for MACB/GEM
Le 19/01/2017 à 08:56, Andrei Pistirica a écrit :
> This patch does the following:
> - MACB/GEM-PTP interface
> - registers and bitfields for TSU
> - capability flags to enable PTP per platform basis
>
> Signed-off-by: Andrei Pistirica <andrei.pistir...@microchip.com>
Acked-by: Nicolas Ferre <nicolas.fe...@atmel.com>
Thanks, regards,
> ---
> Patch history:
>
> Version 1:
> This is just the common code for MACB/GEM-PTP support.
> Code is based on the comments related to the following patch series:
> - [RFC PATCH net-next v1-to-4 1/2] macb: Add 1588 support in Cadence GEM
> - [RFC PATCH net-next v1-to-4 2/2] macb: Enable 1588 support in SAMA5Dx
> platforms
>
> Version 2:
> - Cosmetic changes and PTP capability flag changed doe to overlapping with
> JUMBO.
>
> Note: Patch on net-next: January 19.
>
> drivers/net/ethernet/cadence/macb.c | 32 +++-
> drivers/net/ethernet/cadence/macb.h | 74
> +
> 2 files changed, 104 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb.c
> b/drivers/net/ethernet/cadence/macb.c
> index c0fb80a..ff1e648 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -2085,6 +2085,9 @@ static int macb_open(struct net_device *dev)
>
> netif_tx_start_all_queues(dev);
>
> + if (bp->ptp_info)
> + bp->ptp_info->ptp_init(dev);
> +
> return 0;
> }
>
> @@ -2106,6 +2109,9 @@ static int macb_close(struct net_device *dev)
>
> macb_free_consistent(bp);
>
> + if (bp->ptp_info)
> + bp->ptp_info->ptp_remove(dev);
> +
> return 0;
> }
>
> @@ -2379,6 +2385,17 @@ static int macb_set_ringparam(struct net_device
> *netdev,
> return 0;
> }
>
> +static int macb_get_ts_info(struct net_device *netdev,
> + struct ethtool_ts_info *info)
> +{
> + struct macb *bp = netdev_priv(netdev);
> +
> + if (bp->ptp_info)
> + return bp->ptp_info->get_ts_info(netdev, info);
> +
> + return ethtool_op_get_ts_info(netdev, info);
> +}
> +
> static const struct ethtool_ops macb_ethtool_ops = {
> .get_regs_len = macb_get_regs_len,
> .get_regs = macb_get_regs,
> @@ -2396,7 +2413,7 @@ static const struct ethtool_ops gem_ethtool_ops = {
> .get_regs_len = macb_get_regs_len,
> .get_regs = macb_get_regs,
> .get_link = ethtool_op_get_link,
> - .get_ts_info= ethtool_op_get_ts_info,
> + .get_ts_info= macb_get_ts_info,
> .get_ethtool_stats = gem_get_ethtool_stats,
> .get_strings= gem_get_ethtool_strings,
> .get_sset_count = gem_get_sset_count,
> @@ -2409,6 +2426,7 @@ static const struct ethtool_ops gem_ethtool_ops = {
> static int macb_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
> {
> struct phy_device *phydev = dev->phydev;
> + struct macb *bp = netdev_priv(dev);
>
> if (!netif_running(dev))
> return -EINVAL;
> @@ -2416,7 +2434,17 @@ static int macb_ioctl(struct net_device *dev, struct
> ifreq *rq, int cmd)
> if (!phydev)
> return -ENODEV;
>
> - return phy_mii_ioctl(phydev, rq, cmd);
> + if (!bp->ptp_info)
> + return phy_mii_ioctl(phydev, rq, cmd);
> +
> + switch (cmd) {
> + case SIOCSHWTSTAMP:
> + return bp->ptp_info->set_hwtst(dev, rq, cmd);
> + case SIOCGHWTSTAMP:
> + return bp->ptp_info->get_hwtst(dev, rq);
> + default:
> + return phy_mii_ioctl(phydev, rq, cmd);
> + }
> }
>
> static int macb_set_features(struct net_device *netdev,
> diff --git a/drivers/net/ethernet/cadence/macb.h
> b/drivers/net/ethernet/cadence/macb.h
> index d67adad..94ddedd 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -131,6 +131,20 @@
> #define GEM_RXIPCCNT 0x01a8 /* IP header Checksum Error Counter */
> #define GEM_RXTCPCCNT0x01ac /* TCP Checksum Error Counter */
> #define GEM_RXUDPCCNT0x01b0 /* UDP Checksum Error Counter */
> +#define GEM_TISUBN 0x01bc /* 1588 Timer Increment Sub-ns */
> +#define GEM_TSH 0x01c0 /* 1588 Timer Seconds High */
> +#define GEM_TSL 0x01d0 /* 1588 Timer Seconds Low */
> +#define GEM_TN 0x01d4 /* 1588 Timer Nanoseconds */
> +#define GEM_TA 0x01d8 /* 1588 Timer Adj
Re: [PATCH net-next] macb: Common code to enable ptp support for SAMA5Dx platforms.
);
> + unsigned int (*get_tsu_rate)(struct macb *bp);
> + int (*get_ts_info)(struct net_device *dev,
> +struct ethtool_ts_info *info);
> + int (*get_hwtst)(struct net_device *netdev,
> + struct ifreq *ifr);
> + int (*set_hwtst)(struct net_device *netdev,
> + struct ifreq *ifr, int cmd);
> +};
> +
> struct macb_config {
> u32 caps;
> unsigned intdma_burst_length;
> @@ -874,6 +941,8 @@ struct macb {
> unsigned intjumbo_max_len;
>
> u32 wol;
> +
> + struct macb_ptp_info*ptp_info; /* macb-ptp interface */
> };
>
> static inline bool macb_is_gem(struct macb *bp)
> @@ -881,4 +950,9 @@ static inline bool macb_is_gem(struct macb *bp)
> return !!(bp->caps & MACB_CAPS_MACB_IS_GEM);
> }
>
> +static inline bool gem_has_ptp(struct macb *bp)
> +{
> + return !!(bp->caps & MACB_CAPS_GEM_HAS_PTP);
> +}
> +
> #endif /* _MACB_H */
Otherwise, I'm okay with the rest.
I suggest to people that will keep the ball rolling on this topic to
take advantage of the chunks of code that Andrei developed with the help
of Richard and the best practices discussed. I think particularly, if it
makes sense with HW, about:
- gem_ptp_do_[rt]xstamp(bp, skb) dereference scheme
- gem_ptp_adjfine() rationale
- gem_get_ptp_peer() if needed
Regards,
--
Nicolas Ferre
Re: [RFC PATCH net-next v4 1/2] macb: Add 1588 support in Cadence GEM.
Le 03/01/2017 à 11:47, Rafal Ozieblo a écrit : >> From: Harini Katakam [mailto:harinikatakamli...@gmail.com] >> Sent: 3 stycznia 2017 06:06 >> Subject: Re: [RFC PATCH net-next v4 1/2] macb: Add 1588 support in Cadence >> GEM. >> >> Hi Richard, >> >> On Mon, Jan 2, 2017 at 9:43 PM, Richard Cochran <richardcoch...@gmail.com> >> wrote: >>> On Mon, Jan 02, 2017 at 03:47:07PM +0100, Nicolas Ferre wrote: >>>> Le 02/01/2017 à 12:31, Richard Cochran a écrit : >>>>> This Cadence IP core is a complete disaster. >>>> >>>> Well, it evolved and propose several options to different SoC >>>> integrators. This is not something unusual... >>>> I suspect as well that some other network adapters have the same >>>> weakness concerning PTP timestamp in single register as the early >>>> revisions of this IP. >>> >>> It appears that this core can neither latch the time on read or write, >>> or even latch time stamps. I have worked with many different PTP HW >>> implementations, even early ones like on the ixp4xx, and it is no >>> exaggeration to say that this one is uniquely broken. >>> >>>> I suspect that Rafal tend to jump too quickly to the latest IP >>>> revisions and add more options to this series: let's not try to pour >>>> too much things into this code right now. >>> >>> Why can't you check the IP version in the driver? >> >> There is an IP revision register but it would be probably be >> better to rely on "caps" from the compatibility strings - to cover SoC >> specific >> implementations. Also, when this extended BD is added (with timestamp), >> additional words will need to be added statically which will be >> consistent with Andrei's CONFIG_ checks. > We can distinguish IP cores with and without PTP support by reading > Design Configuration Register. But to distinguish IP cores with > timestamps in buffer descriptors and which support only event > registers, we can only check IP version by reading the revision ID > register and base on that. > I agree with Harini, compatibility strings could be better. But we > might end up with many different configuration in the future. Compatibility strings and associated configurations are cheap. It's not a problem to have many different configurations and clearer for this particular "composite" feature. > We could use only descriptor approach but there are many Atmel's > cores on the market which support only event registers. Yes and once in silicon, it's hard to modify ;-) Regards, -- Nicolas Ferre
Re: [RFC PATCH net-next v4 1/2] macb: Add 1588 support in Cadence GEM.
Le 02/01/2017 à 12:31, Richard Cochran a écrit : > On Mon, Jan 02, 2017 at 09:36:10AM +, Rafal Ozieblo wrote: >> According Cadence Hardware team: >> "It is just that some customers prefer to have the time in the descriptors >> as that is provided per frame. >> The registers are simply overwritten when a new event frame is >> transmitted/received and so software could miss it." >> The question is are you sure that you read timestamp for current frame? (not >> for the next frame). > > AFAICT, having the time stamp in the descriptor is not universally > supported. Looking at the Xilinx Zynq 7000 TRM, I can't find any > mention of this. This is why I proposed to address options incrementally: without timestamp support in descriptor (this patch series), then adding this feature in another patch series. Rafal, this is why Andrei noted that the case covered by this series is not adapted to GEM-GXL and doesn't address the "timestamp in descriptor" case. > This Cadence IP core is a complete disaster. Well, it evolved and propose several options to different SoC integrators. This is not something unusual... I suspect as well that some other network adapters have the same weakness concerning PTP timestamp in single register as the early revisions of this IP. > Unless someone can tell us how this IP works in all of its > incarnations, this series is going nowhere. We're already as v4 (thanks to your fruitful contributions BTW) for this series and will try to add features for other IP options & revisions incrementally. I suspect that Rafal tend to jump too quickly to the latest IP revisions and add more options to this series: let's not try to pour too much things into this code right now. FYI, Andrei will be back online next week. Regards, -- Nicolas Ferre
Re: [RFC PATCH net-next v3 1/2] macb: Add 1588 support in Cadence GEM.
Le 07/12/2016 à 20:39, Richard Cochran a écrit :
> On Wed, Dec 07, 2016 at 08:21:51PM +0200, Andrei Pistirica wrote:
>> +#ifdef CONFIG_MACB_USE_HWSTAMP
>> +void gem_ptp_init(struct net_device *ndev);
>> +void gem_ptp_remove(struct net_device *ndev);
>> +
>> +void gem_ptp_do_txstamp(struct macb *bp, struct sk_buff *skb);
>> +void gem_ptp_do_rxstamp(struct macb *bp, struct sk_buff *skb);
>
> These are in the hot path, and so you should do the test before
> calling the global function, something like this:
>
> void gem_ptp_txstamp(struct macb *bp, struct sk_buff *skb);
>
> static void gem_ptp_do_txstamp(struct macb *bp, struct sk_buff *skb)
> {
> if (!bp->hwts_tx_en)
> return;
> gem_ptp_txstamp(bp, skb);
> }
>
> Ditto for Rx.
Hi Richard,
So you mean that as the "global" function won't be "inlined" by the
compiler as the function is not "static" neither in the same file and
that the jump will be implemented anyway. And this even if the function
is only called at a single location...
This way, if we add a kind or accessors function like the one that you
propose, with the test in it, the branch prediction can play his role
without breaking the processor pipeline as the accessors function will
be inlined by the compiler: Am I right?
So, yes, makes sense. Thanks for the hint.
Regards,
--
Nicolas Ferre
Re: [RFC PATCH 2/2] Documentation: devictree: Add macb mdio bindings
Le 05/12/2016 à 03:55, Harini Katakam a écrit : > Hi Rob, > > > Thanks for the review. > On Sun, Dec 4, 2016 at 3:05 AM, Rob Herring <r...@kernel.org> wrote: >> On Mon, Nov 28, 2016 at 03:19:27PM +0530, Harini Katakam wrote: > >>> +Required properties: >>> +- compatible: Should be "cdns,macb-mdio" >> >> Only one version ever? This needs more specific compatible strings. >> > > This is part of the Cadence MAC in a way. > I can use revision number from the Cadence spec I was working with. > But it is not necessarily specific that version. Yes it is: you must specify the first precise SoC needing/implementing it: "cdns,zynqmp-gem-mdio" or "xlnx,zynq-gem-mdio" or anything looking like this. So, if an Atmel implementation is slightly different, we can still use our own "atmel,sama5d2-gem-mdio" or "cdns,sama5d2-gem-mdio" compatibility string. On the other hand, if it's strictly the same, we can use the "xlnx,zynq-gem-mdio" compatibility without any problem... > I'll take care of the other comments in the next version. > > Regards, > Harini > -- Nicolas Ferre
Re: [PATCH v2] net: macb: Write only necessary bits in NCR in macb reset
Le 29/11/2016 à 06:56, Harini Katakam a écrit :
> In macb_reset_hw, use read-modify-write to disable RX and TX.
> Existing settings, for ex. management port enable,
> are being cleared in the current implementation.
> Also certain reserved bits are read only.
> Hence it is better to use read-modify-write.
> Use the same method for clearing statistics as well.
>
> Signed-off-by: Harini Katakam <hari...@xilinx.com>
Acked-by: Nicolas Ferre <nicolas.fe...@atmel.com>
> ---
>
> v2:
> Make ctrl type as u32
> Improve commit description
>
> ---
> drivers/net/ethernet/cadence/macb.c | 8 ++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb.c
> b/drivers/net/ethernet/cadence/macb.c
> index 0e489bb..2ce3407 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -1744,14 +1744,18 @@ static void macb_reset_hw(struct macb *bp)
> {
> struct macb_queue *queue;
> unsigned int q;
> + u32 ctrl;
>
> /* Disable RX and TX (XXX: Should we halt the transmission
>* more gracefully?)
>*/
> - macb_writel(bp, NCR, 0);
> + ctrl = macb_readl(bp, NCR);
> + ctrl &= ~(MACB_BIT(RE) | MACB_BIT(TE));
> + macb_writel(bp, NCR, ctrl);
>
> /* Clear the stats registers (XXX: Update stats first?) */
> - macb_writel(bp, NCR, MACB_BIT(CLRSTAT));
> + ctrl |= MACB_BIT(CLRSTAT);
> + macb_writel(bp, NCR, ctrl);
>
> /* Clear all status flags */
> macb_writel(bp, TSR, -1);
>
--
Nicolas Ferre
Re: [PATCH v2 1/1] net: macb: ensure ordering write to re-enable RX smoothly
Le 28/11/2016 à 14:55, Zumeng Chen a écrit :
> When a hardware issue happened as described by inline comments, the register
> write pattern looks like the following:
>
>
> + wmb();
>
>
> There might be a memory barrier between these two write operations, so add wmb
> to ensure an flip from 0 to 1 for NCR.
>
> Signed-off-by: Zumeng Chen <zumeng.c...@windriver.com>
Acked-by: Nicolas Ferre <nicolas.fe...@atmel.com>
Thanks, best regards,
> ---
>
> V2 changes:
>
> Add the same wmb for at91ether as well based on reviewer's suggestion.
>
> Cheers,
> drivers/net/ethernet/cadence/macb.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/net/ethernet/cadence/macb.c
> b/drivers/net/ethernet/cadence/macb.c
> index 533653b..6d7cfa7 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -1156,6 +1156,7 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
> if (status & MACB_BIT(RXUBR)) {
> ctrl = macb_readl(bp, NCR);
> macb_writel(bp, NCR, ctrl & ~MACB_BIT(RE));
> + wmb();
> macb_writel(bp, NCR, ctrl | MACB_BIT(RE));
>
> if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
> @@ -2770,6 +2771,7 @@ static irqreturn_t at91ether_interrupt(int irq, void
> *dev_id)
> if (intstatus & MACB_BIT(RXUBR)) {
> ctl = macb_readl(lp, NCR);
> macb_writel(lp, NCR, ctl & ~MACB_BIT(RE));
> + wmb();
> macb_writel(lp, NCR, ctl | MACB_BIT(RE));
> }
>
>
--
Nicolas Ferre
Re: [PATCH] net: macb: Write only necessary bits in NCR in macb reset
Le 28/11/2016 à 10:23, Harini Katakam a écrit :
> In macb_reset_hw, use read-modify-write to disable RX and TX.
> This way exiting settings and reserved bits wont be disturbed.
Yes, indeed... but I would have liked a line about how you discovered it
was an issue for you ; what did it break, etc...
> Use the same method for clearing statistics as well.
>
> Signed-off-by: Harini Katakam <hari...@xilinx.com>
> ---
> drivers/net/ethernet/cadence/macb.c | 9 ++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb.c
> b/drivers/net/ethernet/cadence/macb.c
> index 0e489bb..80ccfc4 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -1743,15 +1743,18 @@ static void macb_init_rings(struct macb *bp)
> static void macb_reset_hw(struct macb *bp)
> {
> struct macb_queue *queue;
> - unsigned int q;
> + unsigned int q, ctrl;
I would have preferred a different line with u32 type.
> /* Disable RX and TX (XXX: Should we halt the transmission
>* more gracefully?)
>*/
> - macb_writel(bp, NCR, 0);
> + ctrl = macb_readl(bp, NCR);
> + ctrl &= ~(MACB_BIT(RE) | MACB_BIT(TE));
> + macb_writel(bp, NCR, ctrl);
>
> /* Clear the stats registers (XXX: Update stats first?) */
> - macb_writel(bp, NCR, MACB_BIT(CLRSTAT));
> + ctrl |= MACB_BIT(CLRSTAT);
> + macb_writel(bp, NCR, ctrl);
>
> /* Clear all status flags */
> macb_writel(bp, TSR, -1);
>
--
Nicolas Ferre
Re: [PATCH 1/1] net: macb: ensure ordering write to re-enable RX smoothly
Le 28/11/2016 à 08:57, Zumeng Chen a écrit :
> When a hardware issue happened as described by inline comments, the register
> write pattern looks like the following:
>
>
> + wmb();
>
>
> There might be a memory barrier between these two write operations, so add wmb
> to ensure an flip from 0 to 1 for NCR.
>
> Signed-off-by: Zumeng Chen <zumeng.c...@windriver.com>
> ---
> drivers/net/ethernet/cadence/macb.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/ethernet/cadence/macb.c
> b/drivers/net/ethernet/cadence/macb.c
> index 533653b..2f9c5b2 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -1156,6 +1156,7 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
> if (status & MACB_BIT(RXUBR)) {
> ctrl = macb_readl(bp, NCR);
> macb_writel(bp, NCR, ctrl & ~MACB_BIT(RE));
> + wmb();
> macb_writel(bp, NCR, ctrl | MACB_BIT(RE));
>
> if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
It seems that there is exactly the same pattern in function
at91ether_interrupt() can you fix both locations in your patch please?
Thanks, best regards,
--
Nicolas Ferre
Re: [PATCH 1/1] net: macb: fix the RX queue reset in macb_rx()
Le 25/11/2016 à 09:49, Cyrille Pitchen a écrit :
> On macb only (not gem), when a RX queue corruption was detected from
> macb_rx(), the RX queue was reset: during this process the RX ring
> buffer descriptor was initialized by macb_init_rx_ring() but we forgot
> to also set bp->rx_tail to 0.
>
> Indeed, when processing the received frames, bp->rx_tail provides the
> macb driver with the index in the RX ring buffer of the next buffer to
> process. So when the whole ring buffer is reset we must also reset
> bp->rx_tail so the driver is synchronized again with the hardware.
>
> Since macb_init_rx_ring() is called from many locations, currently from
> macb_rx() and macb_init_rings(), we'd rather add the "bp->rx_tail = 0;"
> line inside macb_init_rx_ring() than add the very same line after each
> call of this function.
>
> Without this fix, the rx queue is not reset properly to recover from
> queue corruption and connection drop may occur.
>
> Signed-off-by: Cyrille Pitchen <cyrille.pitc...@atmel.com>
> Fixes: 9ba723b081a2 ("net: macb: remove BUG_ON() and reset the queue to
> handle RX errors")
Acked-by: Nicolas Ferre <nicolas.fe...@atmel.com>
Thanks.
Regards,
> ---
> drivers/net/ethernet/cadence/macb.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb.c
> b/drivers/net/ethernet/cadence/macb.c
> index 0e489bb82456..8ee303b8da08 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -991,6 +991,7 @@ static inline void macb_init_rx_ring(struct macb *bp)
> addr += bp->rx_buffer_size;
> }
> bp->rx_ring[bp->rx_ring_size - 1].addr |= MACB_BIT(RX_WRAP);
> + bp->rx_tail = 0;
> }
>
> static int macb_rx(struct macb *bp, int budget)
> @@ -1736,8 +1737,6 @@ static void macb_init_rings(struct macb *bp)
> bp->queues[0].tx_head = 0;
> bp->queues[0].tx_tail = 0;
> bp->queues[0].tx_ring[bp->tx_ring_size - 1].ctrl |= MACB_BIT(TX_WRAP);
> -
> - bp->rx_tail = 0;
> }
>
> static void macb_reset_hw(struct macb *bp)
>
--
Nicolas Ferre
Re: [PATCH net-next] cadence: Add hardware PTP support.
Le 18/11/2016 à 11:47, Rafal Ozieblo a écrit : > Signed-off-by: Rafal Ozieblo <raf...@cadence.com> Note to David: This is more a RFC: we are discussing the addition of this feature and how to cover all macb revisions and optional features implemented in actual products. regards, > --- > Documentation/devicetree/bindings/net/macb.txt | 1 + > drivers/net/ethernet/cadence/macb.c| 742 > - > drivers/net/ethernet/cadence/macb.h| 217 +++- > 3 files changed, 950 insertions(+), 10 deletions(-) [..] -- Nicolas Ferre
Re: [RFC PATCH 2/2] net: macb: Add 64 bit addressing support for GEM
Le 18/11/2016 à 10:59, Rafal Ozieblo a écrit : >> >> -Original Message- >> From: Harini Katakam [mailto:harinikatakamli...@gmail.com] >> Sent: 18 listopada 2016 10:44 >> To: Rafal Ozieblo >> Cc: Nicolas Ferre; Andrei Pistirica; harini.kata...@xilinx.com; >> netdev@vger.kernel.org; linux-ker...@vger.kernel.org >> >> Subject: Re: [RFC PATCH 2/2] net: macb: Add 64 bit addressing support for >> GEM >> >> >> Hi Rafal, >> >> >> >> On Fri, Nov 18, 2016 at 3:00 PM, Rafal Ozieblo <raf...@cadence.com> wrote: >> >>>> -----Original Message- >>>> >>>> From: Nicolas Ferre [mailto:nicolas.fe...@atmel.com] >>>> >>>> Sent: 18 listopada 2016 10:10 >>>> >>>> To: Rafal Ozieblo; Harini Katakam; Andrei Pistirica >>>> >>>> Cc: harini.kata...@xilinx.com; netdev@vger.kernel.org; >>>> >>>> linux-ker...@vger.kernel.org >>>> >>>> Subject: Re: [RFC PATCH 2/2] net: macb: Add 64 bit addressing support >>>> >>>> for GEM >>>> >>>> >>>> >>>> Le 18/11/2016 à 09:59, Rafal Ozieblo a écrit : >>>> >>>>> Hello, >>>>> >>>>> >>>>> >>>>>> From: Harini Katakam [mailto:harinikatakamli...@gmail.com] >>>>>> >>>>>> Sent: 18 listopada 2016 05:30 >>>>>> >>>>>> To: Rafal Ozieblo >>>>>> >>>>>> Cc: Nicolas Ferre; harini.kata...@xilinx.com; >>>>>> >>>>>> netdev@vger.kernel.org; linux-ker...@vger.kernel.org >>>>>> >>>>>> Subject: Re: [RFC PATCH 2/2] net: macb: Add 64 bit addressing >>>>>> >>>>>> support for GEM >>>>>> >>>>>> >>>>>> >>>>>> Hi Rafal, >>>>>> >>>>>> >>>>>> >>>>>> On Thu, Nov 17, 2016 at 7:05 PM, Rafal Ozieblo <raf...@cadence.com> >>>>>> wrote: >>>>>>> -Original Message- >>>>>>> >>>>>>> From: Nicolas Ferre [mailto:nicolas.fe...@atmel.com] >>>>>>> >>>>>>> Sent: 17 listopada
Re: [RFC PATCH 2/2] net: macb: Add 64 bit addressing support for GEM
Le 18/11/2016 à 09:59, Rafal Ozieblo a écrit :
> Hello,
>
>> From: Harini Katakam [mailto:harinikatakamli...@gmail.com]
>> Sent: 18 listopada 2016 05:30
>> To: Rafal Ozieblo
>> Cc: Nicolas Ferre; harini.kata...@xilinx.com; netdev@vger.kernel.org;
>> linux-ker...@vger.kernel.org
>> Subject: Re: [RFC PATCH 2/2] net: macb: Add 64 bit addressing support for GEM
>>
>> Hi Rafal,
>>
>> On Thu, Nov 17, 2016 at 7:05 PM, Rafal Ozieblo <raf...@cadence.com> wrote:
>>> -Original Message-
>>> From: Nicolas Ferre [mailto:nicolas.fe...@atmel.com]
>>> Sent: 17 listopada 2016 14:29
>>> To: Harini Katakam; Rafal Ozieblo
>>> Cc: harini.kata...@xilinx.com; netdev@vger.kernel.org;
>>> linux-ker...@vger.kernel.org
>>> Subject: Re: [RFC PATCH 2/2] net: macb: Add 64 bit addressing support
>>> for GEM
>>>
>>>> Le 17/11/2016 à 13:21, Harini Katakam a écrit :
>>>>> Hi Rafal,
>>>>>
>>>>> On Thu, Nov 17, 2016 at 5:20 PM, Rafal Ozieblo <raf...@cadence.com> wrote:
>>>>>> Hello,
>>>>>> I think, there could a bug in your patch.
>>>>>>
>>>>>>> +
>>>>>>> +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
>>>>>>> + dmacfg |= GEM_BIT(ADDR64); #endif
>>>>>>
>>>>>> You enable 64 bit addressing (64b dma bus width) always when appropriate
>>>>>> architecture config option is enabled.
>>>>>> But there are some legacy controllers which do not support that feature.
>>>>>> According Cadence hardware team:
>>>>>> "64 bit addressing was added in July 2013. Earlier version do not have
>>>>>> it.
>>>>>> This feature was enhanced in release August 2014 to have separate upper
>>>>>> address values for transmit and receive."
>>>>>>
>>>>>>> /* Bitfields in NSR */
>>>>>>> @@ -474,6 +479,10 @@
>>>>>>> struct macb_dma_desc {
>>>>>> > u32 addr;
>>>>>>> u32 ctrl;
>>>>>>> +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
>>>>>>> + u32 addrh;
>>>>>>> + u32 resvd;
>>>>>>> +#endif
>>>>>>> };
>>>>>>
>>>>>> It will not work for legacy hardware. Old descriptor is 2 words wide,
>>>>>> the new one is 4 words wide.
>>>>>> If you enable CONFIG_ARCH_DMA_ADDR_T_64BIT but hardware doesn't
>>>>>> support it at all, you will miss every second descriptor.
>>>>>>
>>>>>
>>>>> True, this feature is not available in all of Cadence IP versions.
>>>>> In fact, the IP version Zynq does not support this. But the one in ZynqMP
>>>>> does.
>>>>> So, we enable kernel config for 64 bit DMA addressing for this SoC
>>>>> and hence the driver picks it up. My assumption was that if the
>>>>> legacy IP does not support
>>>>> 64 bit addressing, then this DMA option wouldn't be enabled.
>>>>>
>>>>> There is a design config register in Cadence IP which is being read
>>>>> to check for 64 bit address support - DMA mask is set based on that.
>>>>> But the addition of two descriptor words cannot be based on this runtime
>>>>> check.
>>>>> For this reason, all the static changes were placed under this check.
>>>>
>>>> We have quite a bunch of options in this driver to determinate what is the
>>>> real capacity of the underlying hardware.
>>>> If HW configuration registers are not appropriate, and it seems they are
>>>> not, I would advice to simply use the DT compatibility string.
>>>>
>>>> Best regards,
>>>> --
>>>> Nicolas Ferre
>>>
>>> HW configuration registers are appropriate. The issue is that this code
>>> doesn’t use the capability bit to switch between different dma descriptors
>>> (2 words vs. 4 words).
>>> DMA descriptor size is chosen based on kernel configuration, not based on
>>> hardware capabilities.
>>
>> HW configuration register does give appropriate information.
>> But addition of two address words in the macb descriptor structure is a
>> static change.
>>
>> +static inline void macb_set_addr(struct macb_dma_de
Re: [RFC PATCH 2/2] net: macb: Add 64 bit addressing support for GEM
Le 17/11/2016 à 13:21, Harini Katakam a écrit :
> Hi Rafal,
>
> On Thu, Nov 17, 2016 at 5:20 PM, Rafal Ozieblo <raf...@cadence.com> wrote:
>> Hello,
>> I think, there could a bug in your patch.
>>
>>> +
>>> +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
>>> + dmacfg |= GEM_BIT(ADDR64);
>>> +#endif
>>
>> You enable 64 bit addressing (64b dma bus width) always when appropriate
>> architecture config option is enabled.
>> But there are some legacy controllers which do not support that feature.
>> According Cadence hardware team:
>> "64 bit addressing was added in July 2013. Earlier version do not have it.
>> This feature was enhanced in release August 2014 to have separate upper
>> address values for transmit and receive."
>>
>>> /* Bitfields in NSR */
>>> @@ -474,6 +479,10 @@
>>> struct macb_dma_desc {
>> > u32 addr;
>>> u32 ctrl;
>>> +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
>>> + u32 addrh;
>>> + u32 resvd;
>>> +#endif
>>> };
>>
>> It will not work for legacy hardware. Old descriptor is 2 words wide, the
>> new one is 4 words wide.
>> If you enable CONFIG_ARCH_DMA_ADDR_T_64BIT but hardware doesn't support it
>> at all,
>> you will miss every second descriptor.
>>
>
> True, this feature is not available in all of Cadence IP versions.
> In fact, the IP version Zynq does not support this. But the one in ZynqMP
> does.
> So, we enable kernel config for 64 bit DMA addressing for this SoC and hence
> the driver picks it up. My assumption was that if the legacy IP does not
> support
> 64 bit addressing, then this DMA option wouldn't be enabled.
>
> There is a design config register in Cadence IP which is being read to
> check for 64 bit address support - DMA mask is set based on that.
> But the addition of two descriptor words cannot be based on this runtime
> check.
> For this reason, all the static changes were placed under this check.
We have quite a bunch of options in this driver to determinate what is
the real capacity of the underlying hardware.
If HW configuration registers are not appropriate, and it seems they are
not, I would advice to simply use the DT compatibility string.
Best regards,
--
Nicolas Ferre
Re: Bug in cadence macb driver
Le 27/10/2016 à 09:22, Steve Bennett a écrit : > Hi, > > We are seeing the following problem in the cadence macb driver. > > BUG: scheduling while atomic: kworker/0:1/44/0x0002 > Modules linked in: 8021q of_serial beacon_sigproc io_processor emsattr > ems_dma_fifo xaxi_dma libgpioreg leds_gpio > CPU: 0 PID: 44 Comm: kworker/0:1 Not tainted 4.4.0 #1 > Hardware name: Xilinx Zynq Platform > Workqueue: events macb_tx_error_task > [] (unwind_backtrace) from [] (show_stack+0x10/0x14) > [] (show_stack) from [] (dump_stack+0x88/0xd8) > [] (dump_stack) from [] (__schedule_bug+0x48/0x60) > [] (__schedule_bug) from [] (__schedule+0x80/0x4c4) > [] (__schedule) from [] (schedule+0xac/0xd4) > [] (schedule) from [] > (schedule_hrtimeout_range_clock+0xc0/0xf4) > [] (schedule_hrtimeout_range_clock) from [] > (usleep_range+0x44/0x48) > [] (usleep_range) from [] (macb_tx_error_task+0xa0/0x270) > [] (macb_tx_error_task) from [] > (process_one_work+0x1b8/0x2f0) > [] (process_one_work) from [] (worker_thread+0x2e4/0x3f0) > [] (worker_thread) from [] (kthread+0xdc/0xf0) > [] (kthread) from [] (ret_from_fork+0x14/0x3c) > > Here we are running at 10M half duplex, so tx is slow. > The problem was introduced in e4bfd971b where a spinlock is taken before I can't find this commit ID. Can you please give the one on Linus' tree together with the commit subject? Thanks, > calling macb_halt_tx() which may then sleep waiting for the current packet to > finish > transmitting. > > We don't need the spinlock on our platform since we don't have multiple > queues, but > this should be fixed properly in the general case. > > Cheers, > Steve > > -- > µWeb: Embedded Web Framework - http://uweb.workware.net.au/ > WorkWare Systems Pty Ltd > W: www.workware.net.au P: +61 434 921 300 > E: ste...@workware.net.au > > > > > > -- Nicolas Ferre
Re: [PATCH] net: macb: NULL out phydev after removing mdio bus
Le 07/10/2016 à 17:13, Xander Huff a écrit : > From: Nathan Sullivan <nathan.sulli...@ni.com> > > To ensure the dev->phydev pointer is not used after becoming invalid in > mdiobus_unregister, set it to NULL. This happens when removing the macb > driver without first taking its interface down, since unregister_netdev > will end up calling macb_close. > > Signed-off-by: Xander Huff <xander.h...@ni.com> > Signed-off-by: Nathan Sullivan <nathan.sulli...@ni.com> > Signed-off-by: Brad Mouring <brad.mour...@ni.com> Acked-by: Nicolas Ferre <nicolas.fe...@atmel.com> > --- > drivers/net/ethernet/cadence/macb.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/ethernet/cadence/macb.c > b/drivers/net/ethernet/cadence/macb.c > index 63144bb..b32444a 100644 > --- a/drivers/net/ethernet/cadence/macb.c > +++ b/drivers/net/ethernet/cadence/macb.c > @@ -3117,6 +3117,7 @@ static int macb_remove(struct platform_device *pdev) > if (dev->phydev) > phy_disconnect(dev->phydev); > mdiobus_unregister(bp->mii_bus); > + dev->phydev = NULL; > mdiobus_free(bp->mii_bus); > > /* Shutdown the PHY if there is a GPIO reset */ > -- Nicolas Ferre
Re: [PATCH -next] net: macb: fix missing unlock on error in macb_start_xmit()
Le 12/09/2016 à 19:40, Helmut Buchsbaum a écrit :
> On 09/10/2016 01:17 PM, Wei Yongjun wrote:
>> From: Wei Yongjun <weiyongj...@huawei.com>
>>
>> Fix missing unlock before return from function macb_start_xmit()
>> in the error handling case.
>>
>> Fixes: 007e4ba3ee13 ("net: macb: initialize checksum when using
>> checksum offloading")
>> Signed-off-by: Wei Yongjun <weiyongj...@huawei.com>
>> ---
>> drivers/net/ethernet/cadence/macb.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/cadence/macb.c
>> b/drivers/net/ethernet/cadence/macb.c
>> index 0294b6a..63144bb 100644
>> --- a/drivers/net/ethernet/cadence/macb.c
>> +++ b/drivers/net/ethernet/cadence/macb.c
>> @@ -1398,7 +1398,7 @@ static int macb_start_xmit(struct sk_buff *skb, struct
>> net_device *dev)
>>
>> if (macb_clear_csum(skb)) {
>> dev_kfree_skb_any(skb);
>> -return NETDEV_TX_OK;
>> +goto unlock;
>> }
>>
>> /* Map socket buffer for DMA transfer */
>>
> You are definitely right. Sorry I missed that obvious point and for
> causing any inconveniences.
>
> BTW, I see there are obviously quite a few users of MACB
> implementations. I'm just curious if anybody else ever encountered the
> checksum problem or if this a matter of Zynq implementation only.
I've just verified that we are affected by this issue as well on sama5d2
(Microchip / Atmel cortex-A5 MPUs).
Thanks for the fix,
--
Nicolas Ferre
Re: [PATCH] net: phy: Ensure the state machine is called when phy is UP
Hi all, I come back to this thread to re-start the conversation as I still have the issue... Le 16/04/2016 à 00:45, Alexandre Belloni a écrit : > On 16/04/2016 at 00:30:26 +0200, Andrew Lunn wrote : >> On Sat, Apr 16, 2016 at 12:17:11AM +0200, Alexandre Belloni wrote: >>> On 16/04/2016 at 00:05:08 +0200, Andrew Lunn wrote : >>>>> Trace without my patch: >>>>> libphy: MACB_mii_bus: probed >>>>> macb f802.ethernet eth0: Cadence GEM rev 0x00020120 at 0xf802 irq >>>>> 27 (fc:c2:3d:0c:6e:05) >>>>> Micrel KSZ8081 or KSZ8091 f802.etherne:01: attached PHY driver >>>>> [Micrel KSZ8081 or KSZ8091] (mii_bus:phy_addr=f802.etherne:01, >>>>> irq=171) >>>>> Micrel KSZ8081 or KSZ8091 f802.etherne:01: PHY state change READY -> >>>>> READY >>>>> [...] >>>>> Micrel KSZ8081 or KSZ8091 f802.etherne:01: PHY state change READY -> >>>>> READY >>>> >>>> Are there some state changes before this? How is it getting to state >>>> READY? It would expect it to start in DOWN, from when the phy device >>>> was created in phy_device_create(). >>>> >>> >>> No other changes. I forgot to mention that this is when booting with a >>> cable plugged in. Unplugging and replugging the cable makes the link >>> detection work fine even without the patch. >> >> Are you tftpbooting? I.e. has the boot loader already done an auto >> negotiation? >> > > Yes. Yes indeed: this is my use-case: load the kernel from U-Boot using tftp and having the rootfs in NAND flash so, no NFS rootfs for me. >> I've looked at the code and i still don't see how it gets to READY. >> What i do see is that when you connect the phy to the MAC, the >> interrupt handler is installed. So maybe there are some PHY interrupts >> before the interface is opened? Could you put a print in >> phy_interrupt(). >> > > That is indeed the case, and I'm not sure why because > 99f81afc139c6edd14d77a91ee91685a414a1c66 is trying to disable AN at > boot. I don't know what happens to the phy, but this patch does fix the issue for me: Tested-by: Nicolas Ferre <nicolas.fe...@atmel.com> The other alternative that I'm considering seriously as I'm still struggled with this is to simply remove the phy IRQ from my board DT. Best regards, -- Nicolas Ferre
Re: [PATCH -next] net: macb: fix missing unlock on error in macb_start_xmit()
Le 10/09/2016 à 13:17, Wei Yongjun a écrit :
> From: Wei Yongjun <weiyongj...@huawei.com>
>
> Fix missing unlock before return from function macb_start_xmit()
> in the error handling case.
>
> Fixes: 007e4ba3ee13 ("net: macb: initialize checksum when using
> checksum offloading")
> Signed-off-by: Wei Yongjun <weiyongj...@huawei.com>
Yes,
Acked-by: Nicolas Ferre <nicolas.fe...@atmel.com>
Thanks. Best regards,
> ---
> drivers/net/ethernet/cadence/macb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb.c
> b/drivers/net/ethernet/cadence/macb.c
> index 0294b6a..63144bb 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -1398,7 +1398,7 @@ static int macb_start_xmit(struct sk_buff *skb, struct
> net_device *dev)
>
> if (macb_clear_csum(skb)) {
> dev_kfree_skb_any(skb);
> - return NETDEV_TX_OK;
> + goto unlock;
> }
>
> /* Map socket buffer for DMA transfer */
>
>
--
Nicolas Ferre
Re: [PATCH] net: macb: Increase DMA buffer size
Le 24/08/2016 à 20:25, Xander Huff a écrit : > From: Nathan Sullivan <nathan.sulli...@ni.com> > > In recent testing with the RT patchset, we have seen cases where the > transmit ring can fill even with up to 200 txbds in the ring. Increase > the size of the DMA rings to avoid overruns. > > Signed-off-by: Nathan Sullivan <nathan.sulli...@ni.com> > Acked-by: Ben Shelton <ben.shel...@ni.com> > Acked-by: Jaeden Amero <jaeden.am...@ni.com> > Natinst-ReviewBoard-ID: 83662 > --- > drivers/net/ethernet/cadence/macb.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/cadence/macb.c > b/drivers/net/ethernet/cadence/macb.c > index 3256839..86a8e20 100644 > --- a/drivers/net/ethernet/cadence/macb.c > +++ b/drivers/net/ethernet/cadence/macb.c > @@ -35,12 +35,12 @@ > > #include "macb.h" > > -#define MACB_RX_BUFFER_SIZE 128 > +#define MACB_RX_BUFFER_SIZE 1536 This change seems not covered by the commit message. Can you please separate the changes in 2 patches or elaborate a bit more the reason for this RX buffer size change. Bye, > #define RX_BUFFER_MULTIPLE 64 /* bytes */ > #define RX_RING_SIZE 512 /* must be power of 2 */ > #define RX_RING_BYTES(sizeof(struct macb_dma_desc) * > RX_RING_SIZE) > > -#define TX_RING_SIZE 128 /* must be power of 2 */ > +#define TX_RING_SIZE 512 /* must be power of 2 */ > #define TX_RING_BYTES(sizeof(struct macb_dma_desc) * > TX_RING_SIZE) > > /* level of occupied TX descriptors under which we wake up TX process */ > -- Nicolas Ferre
Re: [PATCHv3] net: ethernet: macb: Add support for rx_clk
Le 15/08/2016 à 21:44, Shubhrajyoti Datta a écrit :
> Some of the platforms like zynqmp ultrascale+ has a
> separate clock gate for the rx clock. Add an optional
> rx_clk so that the clock can be enabled.
>
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.da...@xilinx.com>
Fine with me:
Acked-by: Nicolas Ferre <nicolas.fe...@atmel.com>
> ---
> v2:
> fix warnng
> v3
> Add that rx applies to zcu mpsoc
>
> Documentation/devicetree/bindings/net/macb.txt | 1 +
> drivers/net/ethernet/cadence/macb.c| 31
> +-
> drivers/net/ethernet/cadence/macb.h| 4 +++-
> 3 files changed, 30 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/macb.txt
> b/Documentation/devicetree/bindings/net/macb.txt
> index b5a42df..1506e94 100644
> --- a/Documentation/devicetree/bindings/net/macb.txt
> +++ b/Documentation/devicetree/bindings/net/macb.txt
> @@ -21,6 +21,7 @@ Required properties:
> - clock-names: Tuple listing input clock names.
> Required elements: 'pclk', 'hclk'
> Optional elements: 'tx_clk'
> + Optional elements: 'rx_clk' applies to cdns,zynqmp-gem
> - clocks: Phandles to input clocks.
>
> Optional properties for PHY child node:
> diff --git a/drivers/net/ethernet/cadence/macb.c
> b/drivers/net/ethernet/cadence/macb.c
> index 89c0cfa..278c6e3 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -2303,7 +2303,8 @@ static void macb_probe_queues(void __iomem *mem,
> }
>
> static int macb_clk_init(struct platform_device *pdev, struct clk **pclk,
> - struct clk **hclk, struct clk **tx_clk)
> + struct clk **hclk, struct clk **tx_clk,
> + struct clk **rx_clk)
> {
> int err;
>
> @@ -2325,6 +2326,10 @@ static int macb_clk_init(struct platform_device *pdev,
> struct clk **pclk,
> if (IS_ERR(*tx_clk))
> *tx_clk = NULL;
>
> + *rx_clk = devm_clk_get(>dev, "rx_clk");
> + if (IS_ERR(*rx_clk))
> + *rx_clk = NULL;
> +
> err = clk_prepare_enable(*pclk);
> if (err) {
> dev_err(>dev, "failed to enable pclk (%u)\n", err);
> @@ -2343,8 +2348,17 @@ static int macb_clk_init(struct platform_device *pdev,
> struct clk **pclk,
> goto err_disable_hclk;
> }
>
> + err = clk_prepare_enable(*rx_clk);
> + if (err) {
> + dev_err(>dev, "failed to enable rx_clk (%u)\n", err);
> + goto err_disable_txclk;
> + }
> +
> return 0;
>
> +err_disable_txclk:
> + clk_disable_unprepare(*tx_clk);
> +
> err_disable_hclk:
> clk_disable_unprepare(*hclk);
>
> @@ -2728,12 +2742,14 @@ static const struct net_device_ops
> at91ether_netdev_ops = {
> };
>
> static int at91ether_clk_init(struct platform_device *pdev, struct clk
> **pclk,
> - struct clk **hclk, struct clk **tx_clk)
> + struct clk **hclk, struct clk **tx_clk,
> + struct clk **rx_clk)
> {
> int err;
>
> *hclk = NULL;
> *tx_clk = NULL;
> + *rx_clk = NULL;
>
> *pclk = devm_clk_get(>dev, "ether_clk");
> if (IS_ERR(*pclk))
> @@ -2857,13 +2873,13 @@ MODULE_DEVICE_TABLE(of, macb_dt_ids);
> static int macb_probe(struct platform_device *pdev)
> {
> int (*clk_init)(struct platform_device *, struct clk **,
> - struct clk **, struct clk **)
> + struct clk **, struct clk **, struct clk **)
> = macb_clk_init;
> int (*init)(struct platform_device *) = macb_init;
> struct device_node *np = pdev->dev.of_node;
> struct device_node *phy_node;
> const struct macb_config *macb_config = NULL;
> - struct clk *pclk, *hclk = NULL, *tx_clk = NULL;
> + struct clk *pclk, *hclk = NULL, *tx_clk = NULL, *rx_clk = NULL;
> unsigned int queue_mask, num_queues;
> struct macb_platform_data *pdata;
> bool native_io;
> @@ -2891,7 +2907,7 @@ static int macb_probe(struct platform_device *pdev)
> }
> }
>
> - err = clk_init(pdev, , , _clk);
> + err = clk_init(pdev, , , _clk, _clk);
> if (err)
> return err;
>
> @@ -2927,6 +2943,7 @@ static int macb_probe(struct platform_device *pdev)
> bp->pclk = pclk;
> bp->hclk = hclk;
> bp->tx_clk = tx_clk;
> + bp->rx_clk = rx_clk;
> if (macb_config)
>
Re: [RFC PATCH 2/3] net: macb: Add support for 1588 for Zynq Ultrascale+ MPSoC
Le 21/09/2015 à 19:49, Harini Katakam a écrit : > On Fri, Sep 11, 2015 at 1:27 PM, Harini Katakam > <harini.kata...@xilinx.com> wrote: >> Cadence GEM in Zynq Ultrascale+ MPSoC supports 1588 and provides a >> 102 bit time counter with 48 bits for seconds, 30 bits for nsecs and >> 24 bits for sub-nsecs. The timestamp is made available to the SW through >> registers as well as (more precisely) through upper two words in >> an extended BD. >> >> This patch does the following: >> - Adds MACB_CAPS_TSU in zynqmp_config. >> - Registers to ptp clock framework (after checking for timestamp support in >> IP and capability in config). >> - TX BD and RX BD control registers are written to populate timestamp in >> extended BD words. >> - Timer initialization is done by writing time of day to the timer counter. >> - ns increment register is programmed as NS_PER_SEC/TSU_CLK. >> For a 24 bit subns precision, the subns increment equals >> remainder of (NS_PER_SEC/TSU_CLK) * (2^24). >> TSU (Time stamp unit) clock is obtained by the driver from devicetree. >> - HW time stamp capabilities are advertised via ethtool and macb ioctl is >> updated accordingly. >> - For all PTP event frames, nanoseconds and the lower 5 bits of seconds are >> obtained from the BD. This offers a precise timestamp. The upper bits >> (which dont vary between consecutive packets) are obtained from the >> TX/RX PTP event/PEER registers. The timestamp obtained thus is updated >> in skb for upper layers to access. >> - The drivers register functions with ptp to perform time and frequency >> adjustment. >> - Time adjustment is done by writing to the 1558_ADJUST register. >> The controller will read the delta in this register and update the timer >> counter register. Alternatively, for large time offset adjustments, >> the driver reads the secs and nsecs counter values, adds/subtracts the >> delta and updates the timer counter. In order to be as precise as possible, >> nsecs counter is read again if secs has incremented during the counter >> read. >> - Frequency adjustment is not directly supported by this IP. >> addend is the initial value ns increment and similarly addendesub. >> The ppb (parts per billion) provided is used as >> ns_incr = addend +/- (ppb/rate). >> Similarly the remainder of the above is used to populate subns increment. >> In case the ppb requested is negative AND subns adjustment greater than >> the addendsub, ns_incr is reduced by 1 and subns_incr is adjusted in >> positive accordingly. >> >> Signed-off-by: Harini Katakam <hari...@xilinx.com>: >> --- >> drivers/net/ethernet/cadence/macb.c | 372 >> ++- >> drivers/net/ethernet/cadence/macb.h | 64 ++ >> 2 files changed, 428 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/net/ethernet/cadence/macb.c >> b/drivers/net/ethernet/cadence/macb.c >> index bb2932c..b531008 100644 >> --- a/drivers/net/ethernet/cadence/macb.c >> +++ b/drivers/net/ethernet/cadence/macb.c >> @@ -30,6 +30,8 @@ >> #include >> #include [..] >> + unsigned intns_incr; >> + unsigned intsubns_incr; >> }; >> >> static inline bool macb_is_gem(struct macb *bp) >> -- >> 1.7.9.5 > > Ping > > Thanks. Harini, I come back to this patch of last year and I'm sorry about being so late answering you. Andrei who is added to the discussion will have some time to deal with this feature and we would like to make some progress with it. He already had some work done on his side before I recall your email. So, could you please re-send your original 1588 patch with Andrei in copy so that we can all (re-)start the discussion and progress for adding this feature. We must also note that some hardware differences between our platforms may have an impact on the code and how we implement things (as highlighted on this forum: http://www.at91.com/discussions/viewtopic.php/f,12/t,25462.html). Anyway, we'll overcome this and have a widely tested solution at the end of the day! Thanks for your patience, bye! PS: for some reason, I only have this "ping" part of your email but not the original one -- Nicolas Ferre
Re: [PATCH] net: macb: Add 64 bit addressing support for GEM
#define GEM_IER(hw_q) (0x0600 + ((hw_q) << 2))
> #define GEM_IDR(hw_q)(0x0620 + ((hw_q) << 2))
> @@ -249,6 +252,8 @@
> #define GEM_RXBS_SIZE8
> #define GEM_DDRP_OFFSET 24 /* disc_when_no_ahb */
> #define GEM_DDRP_SIZE1
> +#define GEM_ADDR64_OFFSET30 /* Address bus width - 64b or 32b */
> +#define GEM_ADDR64_SIZE 1
>
>
> /* Bitfields in NSR */
> @@ -474,6 +479,10 @@
> struct macb_dma_desc {
> u32 addr;
> u32 ctrl;
> +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> + u32 addrh;
> + u32 resvd;
> +#endif
> };
>
> /* DMA descriptor bitfields */
> @@ -777,6 +786,7 @@ struct macb_queue {
> unsigned intIDR;
> unsigned intIMR;
> unsigned intTBQP;
> + unsigned intTBQPH;
>
> unsigned inttx_head, tx_tail;
> struct macb_dma_desc*tx_ring;
>
--
Nicolas Ferre
Re: [RFC PATCH 1/2] net: macb: Correct CAPS mask
Le 01/08/2016 à 09:20, Harini Katakam a écrit :
> USRIO and JUMBO CAPS have the same mask.
> Fix the same.
>
> Signed-off-by: Harini Katakam <hari...@xilinx.com>
Hi,
Indeed there's a bug...
> ---
> drivers/net/ethernet/cadence/macb.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb.h
> b/drivers/net/ethernet/cadence/macb.h
> index 36893d8..b6fcf10 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -403,11 +403,11 @@
> #define MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII 0x0004
> #define MACB_CAPS_NO_GIGABIT_HALF0x0008
> #define MACB_CAPS_USRIO_DISABLED 0x0010
> +#define MACB_CAPS_JUMBO 0x0020
> #define MACB_CAPS_FIFO_MODE 0x1000
> #define MACB_CAPS_GIGABIT_MODE_AVAILABLE 0x2000
> #define MACB_CAPS_SG_DISABLED0x4000
> #define MACB_CAPS_MACB_IS_GEM0x8000
> -#define MACB_CAPS_JUMBO 0x0010
Acked-by: Nicolas Ferre <nicolas.fe...@atmel.com>
Can you please send it independently without the RFC tag in the subject
and with the following tags in the body as well:
Fixes: ce721a702197 ("net: ethernet: cadence-macb: Add disabled usrio caps")
Cc: sta...@vger.kernel.org # v4.5+
> /* Bit manipulation macros */
> #define MACB_BIT(name) \
>
Thanks, bye,
--
Nicolas Ferre
Re: [RFC PATCH v2 2/4] net: ethernet: xilinx: Add gmii2rgmii converter support
Le 04/07/2016 13:47, Appana Durga Kedareswara Rao a écrit :
> Hi Nicolas,
>
> Thanks for the review...
>
>>> diff --git a/include/linux/xilinx_gmii2rgmii.h
>>> b/include/linux/xilinx_gmii2rgmii.h
>>> new file mode 100644
>>> index 000..b328ee7
>>> --- /dev/null
>>> +++ b/include/linux/xilinx_gmii2rgmii.h
>>> @@ -0,0 +1,24 @@
>>
>>
>> Here, header of the file seems needed.
>
> Sure will fix in the next version...
>
>>
>>> +#ifndef _GMII2RGMII_H
>>> +#define _GMII2RGMII_H
>>> +
>>> +#include
>>> +#include
>>> +#include
>>> +
>>> +#define XILINX_GMII2RGMII_FULLDPLX BMCR_FULLDPLX
>>> +#define XILINX_GMII2RGMII_SPEED1000BMCR_SPEED1000
>>> +#define XILINX_GMII2RGMII_SPEED100 BMCR_SPEED100
>>> +#define XILINX_GMII2RGMII_REG_NUM 0x10
>>> +
>>> +struct gmii2rgmii {
>>> + struct net_device *dev;
>>> + struct mii_bus *mii_bus;
>>> + struct phy_device *gmii2rgmii_phy_dev;
>>> + void*platform_data;
>>> + int (*mdio_write)(struct mii_bus *bus, int mii_id, int reg,
>>> + u16 val);
>>> + void (*fix_mac_speed)(struct gmii2rgmii *xphy, unsigned int speed);
>>> +};
>>> +
>>> +extern int gmii2rgmii_phyprobe(struct gmii2rgmii *xphy); #endif
>>
>> I see a compilation issue here:
>>
>> You should provide a way to have this function even if the NET_VENDOR_XILINX
>> config option is not selected (test to compile with the sama5_defconfig and
>> you'll see).
>
> Ok will fix in the next version...
>
>>
>> What about making this function void in case of !XILINX?
>
> This is one way to get rid of compilation error. Changes will be look like
> below
>
> #ifdef CONFIG_NET_VENDOR_XILINX
You may need to have:
#if defined(CONFIG_NET_VENDOR_XILINX) && defined(CONFIG_XILINX_GMII2RGMII)
> extern int gmii2rgmii_phyprobe(struct gmii2rgmii *xphy);
> #else
> extern void gmii2rgmii_phyprobe(struct gmii2rgmii *xphy);
No need for the line above...
> void gmii2rgmii_phyprobe(struct gmii2rgmii *xphy)
> {
> }
On one single line, like:
static inline void gmii2rgmii_phyprobe(struct gmii2rgmii *xphy) { }
> #endif
> For me the changes are looking odd...
For me, it's okay...
>
> Other possible ways
> 1) Put a config check around phyprobe api in the macb driver.
> #ifdef CONFIG_XILINX_GMII2RGMII
>gmii2rgmii_phyprobe(>converter_phy);
> #endif
Nope!
> 2) Select NET_VENDOR_XILINX in the macb Kconfig
> @ -22,6 +22,7 @@ config MACB
> tristate "Cadence MACB/GEM support"
> depends on HAS_DMA
> select PHYLIB
> + select NET_VENDOR_XILINX
> Please let me know which one you prefer will fix that and will post v3...
First one with my changes is the best. But maybe wait for more feedback...
Bye,
--
Nicolas Ferre
Re: [RFC PATCH v2 2/4] net: ethernet: xilinx: Add gmii2rgmii converter support
phy-handle", 0);
> + if (phy_node) {
> + phydev = of_phy_attach(xphy->dev, phy_node, 0, 0);
> + if (!phydev) {
> + netdev_err(xphy->dev,
> +"%s: no gmii to rgmii converter found\n",
> +xphy->dev->name);
> + return -1;
> + }
> + xphy->gmii2rgmii_phy_dev = phydev;
> + xphy->fix_mac_speed = xgmii2rgmii_fix_mac_speed;
> + } else {
> + xphy->gmii2rgmii_phy_dev = 0;
> + xphy->fix_mac_speed = 0;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(gmii2rgmii_phyprobe);
> +
> +MODULE_DESCRIPTION("Xilinx GMII2RGMII converter driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/xilinx_gmii2rgmii.h
> b/include/linux/xilinx_gmii2rgmii.h
> new file mode 100644
> index 000..b328ee7
> --- /dev/null
> +++ b/include/linux/xilinx_gmii2rgmii.h
> @@ -0,0 +1,24 @@
Here, header of the file seems needed.
> +#ifndef _GMII2RGMII_H
> +#define _GMII2RGMII_H
> +
> +#include
> +#include
> +#include
> +
> +#define XILINX_GMII2RGMII_FULLDPLX BMCR_FULLDPLX
> +#define XILINX_GMII2RGMII_SPEED1000 BMCR_SPEED1000
> +#define XILINX_GMII2RGMII_SPEED100 BMCR_SPEED100
> +#define XILINX_GMII2RGMII_REG_NUM0x10
> +
> +struct gmii2rgmii {
> + struct net_device *dev;
> + struct mii_bus *mii_bus;
> + struct phy_device *gmii2rgmii_phy_dev;
> + void *platform_data;
> + int (*mdio_write)(struct mii_bus *bus, int mii_id, int reg,
> + u16 val);
> + void (*fix_mac_speed)(struct gmii2rgmii *xphy, unsigned int speed);
> +};
> +
> +extern int gmii2rgmii_phyprobe(struct gmii2rgmii *xphy);
> +#endif
I see a compilation issue here:
You should provide a way to have this function even if the
NET_VENDOR_XILINX config option is not selected (test to compile with
the sama5_defconfig and you'll see).
What about making this function void in case of !XILINX?
(so, NACK for the series as it is).
Bye,
--
Nicolas Ferre
Re: [RFC PATCH 2/2] net: macb: Add gmii2rgmii phy converter support
Le 01/07/2016 11:02, Appana Durga Kedareswara Rao a écrit :
> Hi Nicolas Ferre,
>
> Thanks for the quick review...
>
>>
>> Le 01/07/2016 08:20, Kedareswara rao Appana a écrit :
>>> This patch adds support for gmii2rgmii phy converter in the macb
>>> driver.
>>
>> Okay, I'd like more explanation here.
>> Hints & key words:
>> - dt property
>> - mdio bus
>> - mac speed
>
> Sure will fix in the next version...
>
>>
>>
>>> Signed-off-by: Kedareswara rao Appana <appa...@xilinx.com>
>>> ---
>>> drivers/net/ethernet/cadence/macb.c | 21 -
>>> drivers/net/ethernet/cadence/macb.h |3 +++
>>> 2 files changed, 23 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/cadence/macb.c
>>> b/drivers/net/ethernet/cadence/macb.c
>>> index cb07d95..de0ad71 100644
>>> --- a/drivers/net/ethernet/cadence/macb.c
>>> +++ b/drivers/net/ethernet/cadence/macb.c
>>> @@ -257,6 +257,14 @@ static int macb_mdio_write(struct mii_bus *bus, int
>> mii_id, int regnum,
>>> return 0;
>>> }
>>>
>>> +static inline void macb_hw_fix_mac_speed(struct macb *bp,
>>> +struct phy_device *phydev)
>>> +{
>>> + if (likely(bp->converter_phy.fix_mac_speed))
>>
>> What is the purpose of this branch bias? The code isn't in some hot path, so
>> I
>> suspect that its not needed.
>
> If we won't put this check driver will crash with NULL pointer dereference
> for the below cases
I know that...
> ---> Converter driver is disabled
> ---> Converter driver is enabled but the converter probe is not called from
> the macb driver.
I didn't make myself clear: It's not the branch itself that I'm talking
about it's the branch profiling directive "likely()" that seems not
necessary.
>>> + bp->converter_phy.fix_mac_speed(>converter_phy,
>>> + phydev->speed);
>>> +}
>>> +
>>> /**
>>> * macb_set_tx_clk() - Set a clock to a new frequency
>>> * @clkPointer to the clock to change
>>> @@ -329,6 +337,7 @@ static void macb_handle_link_change(struct
>> net_device *dev)
>>> reg |= GEM_BIT(GBE);
>>>
>>> macb_or_gem_writel(bp, NCFGR, reg);
>>> + macb_hw_fix_mac_speed(bp, phydev);
>>>
>>> bp->speed = phydev->speed;
>>> bp->duplex = phydev->duplex;
>>> @@ -2884,7 +2893,7 @@ static int macb_probe(struct platform_device
>> *pdev)
>>> struct clk **, struct clk **)
>>> = macb_clk_init;
>>> int (*init)(struct platform_device *) = macb_init;
>>> - struct device_node *np = pdev->dev.of_node;
>>> + struct device_node *np = pdev->dev.of_node, *np1, *np11;
>>
>> Nitpicking:
>> Be more explicit on variable names. Simple name for the iterator is okay, the
>> other is better if changed.
>> I also like to see variable on separated lines.
>
> Ok sure will fix in the next version...
>
>>
>>> struct device_node *phy_node;
>>> const struct macb_config *macb_config = NULL;
>>> struct clk *pclk, *hclk = NULL, *tx_clk = NULL; @@ -3011,6 +3020,16
>>> @@ static int macb_probe(struct platform_device *pdev)
>>> goto err_out_free_netdev;
>>>
>>> phydev = bp->phy_dev;
>>> + np1 = of_get_next_child(np, NULL);
>>> + for_each_child_of_node(np1, np11) {
>>> + if (of_device_is_compatible(np11, "xlnx,gmiitorgmii")) {
>>
>> This would definitively need documentation and at least link in the macb
>> binding
>> to show how this emulated phy connect to the mdio bus...
>>
>> I'm not able to judge if the node arrangement is okay: I let Florian tell
>> his view
>> on this...
>
> Ok will document in the macb binding doc.
>
>>
>>> + bp->converter_phy.dev = dev;
>>> + bp->converter_phy.mii_bus = bp->mii_bus;
>>> + bp->converter_phy.mdio_write = macb_mdio_write;
>>> + bp->converter_phy.platform_data = bp->pdev-
>>> dev.of_node;
>>> + gmii2rgmii_phyprobe(>converter_phy);
>>> +
Re: [RFC PATCH 2/2] net: macb: Add gmii2rgmii phy converter support
Le 01/07/2016 08:20, Kedareswara rao Appana a écrit :
> This patch adds support for gmii2rgmii phy converter
> in the macb driver.
Okay, I'd like more explanation here.
Hints & key words:
- dt property
- mdio bus
- mac speed
> Signed-off-by: Kedareswara rao Appana <appa...@xilinx.com>
> ---
> drivers/net/ethernet/cadence/macb.c | 21 -
> drivers/net/ethernet/cadence/macb.h |3 +++
> 2 files changed, 23 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb.c
> b/drivers/net/ethernet/cadence/macb.c
> index cb07d95..de0ad71 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -257,6 +257,14 @@ static int macb_mdio_write(struct mii_bus *bus, int
> mii_id, int regnum,
> return 0;
> }
>
> +static inline void macb_hw_fix_mac_speed(struct macb *bp,
> + struct phy_device *phydev)
> +{
> + if (likely(bp->converter_phy.fix_mac_speed))
What is the purpose of this branch bias? The code isn't in some hot
path, so I suspect that its not needed.
> + bp->converter_phy.fix_mac_speed(>converter_phy,
> + phydev->speed);
> +}
> +
> /**
> * macb_set_tx_clk() - Set a clock to a new frequency
> * @clk Pointer to the clock to change
> @@ -329,6 +337,7 @@ static void macb_handle_link_change(struct net_device
> *dev)
> reg |= GEM_BIT(GBE);
>
> macb_or_gem_writel(bp, NCFGR, reg);
> + macb_hw_fix_mac_speed(bp, phydev);
>
> bp->speed = phydev->speed;
> bp->duplex = phydev->duplex;
> @@ -2884,7 +2893,7 @@ static int macb_probe(struct platform_device *pdev)
> struct clk **, struct clk **)
> = macb_clk_init;
> int (*init)(struct platform_device *) = macb_init;
> - struct device_node *np = pdev->dev.of_node;
> + struct device_node *np = pdev->dev.of_node, *np1, *np11;
Nitpicking:
Be more explicit on variable names. Simple name for the iterator is
okay, the other is better if changed.
I also like to see variable on separated lines.
> struct device_node *phy_node;
> const struct macb_config *macb_config = NULL;
> struct clk *pclk, *hclk = NULL, *tx_clk = NULL;
> @@ -3011,6 +3020,16 @@ static int macb_probe(struct platform_device *pdev)
> goto err_out_free_netdev;
>
> phydev = bp->phy_dev;
> + np1 = of_get_next_child(np, NULL);
> + for_each_child_of_node(np1, np11) {
> + if (of_device_is_compatible(np11, "xlnx,gmiitorgmii")) {
This would definitively need documentation and at least link in the macb
binding to show how this emulated phy connect to the mdio bus...
I'm not able to judge if the node arrangement is okay: I let Florian
tell his view on this...
> + bp->converter_phy.dev = dev;
> + bp->converter_phy.mii_bus = bp->mii_bus;
> + bp->converter_phy.mdio_write = macb_mdio_write;
> + bp->converter_phy.platform_data = bp->pdev->dev.of_node;
> + gmii2rgmii_phyprobe(>converter_phy);
> + }
> + }
>
> netif_carrier_off(dev);
>
> diff --git a/drivers/net/ethernet/cadence/macb.h
> b/drivers/net/ethernet/cadence/macb.h
> index 8a13824..735bce2 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -10,6 +10,8 @@
> #ifndef _MACB_H
> #define _MACB_H
>
> +#include
No, put it in the macb.c.
> +
> #define MACB_GREGS_NBR 16
> #define MACB_GREGS_VERSION 2
> #define MACB_MAX_QUEUES 8
> @@ -846,6 +848,7 @@ struct macb {
> unsigned intjumbo_max_len;
>
> u32 wol;
> + struct gmii2rgmii converter_phy;
> };
>
> static inline bool macb_is_gem(struct macb *bp)
If Florian and phy guys are okay with the approach, I'm fine with this
patch, once corrected.
Thanks, bye,
--
Nicolas Ferre
Re: [PATCH net] net: macb: Probe MDIO bus before registering netdev
Le 03/05/2016 03:38, Florian Fainelli a écrit :
> The current sequence makes us register for a network device prior to
> registering and probing the MDIO bus which could lead to some unwanted
> consequences, like a thread of execution calling into ndo_open before
> register_netdev() returns, while the MDIO bus is not ready yet.
>
> Rework the sequence to register for the MDIO bus, and therefore attach
> to a PHY prior to calling register_netdev(), which implies reworking the
> error path a bit.
>
> Signed-off-by: Florian Fainelli <f.faine...@gmail.com>
Thanks a lot Florian for this follow-up and the advices you had given to
Alexandre during the debug session when you spotted this problem.
Acked-by: Nicolas Ferre <nicolas.fe...@atmel.com>
> ---
> Tracking down the exact commit which started doing that was a little
> difficult, so I can't really provide a proper Fixes tag yet that does
> not reference 4-5 commits
Yes, indeed. Macb is moving quite a bit those days ;-)
Bye,
> drivers/net/ethernet/cadence/macb.c | 31 +++
> 1 file changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb.c
> b/drivers/net/ethernet/cadence/macb.c
> index 48a7d7dee846..e9b470a5ddd0 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -441,7 +441,7 @@ static int macb_mii_init(struct macb *bp)
> snprintf(bp->mii_bus->id, MII_BUS_ID_SIZE, "%s-%x",
> bp->pdev->name, bp->pdev->id);
> bp->mii_bus->priv = bp;
> - bp->mii_bus->parent = >dev->dev;
> + bp->mii_bus->parent = >pdev->dev;
> pdata = dev_get_platdata(>pdev->dev);
>
> dev_set_drvdata(>dev->dev, bp->mii_bus);
> @@ -3019,29 +3019,36 @@ static int macb_probe(struct platform_device *pdev)
> if (err)
> goto err_out_free_netdev;
>
> + err = macb_mii_init(bp);
> + if (err)
> + goto err_out_free_netdev;
> +
> + phydev = bp->phy_dev;
> +
> + netif_carrier_off(dev);
> +
> err = register_netdev(dev);
> if (err) {
> dev_err(>dev, "Cannot register net device, aborting.\n");
> - goto err_out_unregister_netdev;
> + goto err_out_unregister_mdio;
> }
>
> - err = macb_mii_init(bp);
> - if (err)
> - goto err_out_unregister_netdev;
> -
> - netif_carrier_off(dev);
> + phy_attached_info(phydev);
>
> netdev_info(dev, "Cadence %s rev 0x%08x at 0x%08lx irq %d (%pM)\n",
> macb_is_gem(bp) ? "GEM" : "MACB", macb_readl(bp, MID),
> dev->base_addr, dev->irq, dev->dev_addr);
>
> - phydev = bp->phy_dev;
> - phy_attached_info(phydev);
> -
> return 0;
>
> -err_out_unregister_netdev:
> - unregister_netdev(dev);
> +err_out_unregister_mdio:
> + phy_disconnect(bp->phy_dev);
> + mdiobus_unregister(bp->mii_bus);
> + mdiobus_free(bp->mii_bus);
> +
> + /* Shutdown the PHY if there is a GPIO reset */
> + if (bp->reset_gpio)
> + gpiod_set_value(bp->reset_gpio, 0);
>
> err_out_free_netdev:
> free_netdev(dev);
>
--
Nicolas Ferre
Re: [PATCH] macb: fix mdiobus_scan() error check
Le 01/05/2016 00:47, Sergei Shtylyov a écrit :
> Now mdiobus_scan() returns ERR_PTR(-ENODEV) instead of NULL if the PHY
> device ID was read as all ones. As this was not an error before, this
> value should be filtered out now in this driver.
>
> Fixes: b74766a0a0fe ("phylib: don't return NULL from get_phy_device()")
> Signed-off-by: Sergei Shtylyov <sergei.shtyl...@cogentembedded.com>
Acked-by: Nicolas Ferre <nicolas.fe...@atmel.com>
Thanks.
>
> ---
> The patch is against DaveM's 'net-next.git' repo.
>
> drivers/net/ethernet/cadence/macb.c |3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> Index: net-next/drivers/net/ethernet/cadence/macb.c
> ===
> --- net-next.orig/drivers/net/ethernet/cadence/macb.c
> +++ net-next/drivers/net/ethernet/cadence/macb.c
> @@ -458,7 +458,8 @@ static int macb_mii_init(struct macb *bp
> struct phy_device *phydev;
>
> phydev = mdiobus_scan(bp->mii_bus, i);
> - if (IS_ERR(phydev)) {
> + if (IS_ERR(phydev) &&
> + PTR_ERR(phydev) != -ENODEV) {
> err = PTR_ERR(phydev);
> break;
> }
>
>
--
Nicolas Ferre
Re: [PATCH v2] net: macb: do not scan PHYs manually
> +
> + /* fallback to standard phy registration if no phy were
> + * found during dt phy registration
> + */
> + if (!phy_find_first(bp->mii_bus)) {
> + for (i = 0; i < PHY_MAX_ADDR; i++) {
> + struct phy_device *phydev;
> +
> + phydev = mdiobus_scan(bp->mii_bus, i);
> + if (IS_ERR(phydev)) {
> + err = PTR_ERR(phydev);
> + break;
> + }
> + }
> +
> + if (err)
> + goto err_out_unregister_bus;
> + }
> +
> + return err;
> +
> +err_out_unregister_bus:
> + mdiobus_unregister(bp->mii_bus);
> + return err;
> +}
> +
> +static int macb_mii_pdata_init(struct macb *bp,
> +struct macb_platform_data *pdata)
> +{
> + if (pdata)
> + bp->mii_bus->phy_mask = pdata->phy_mask;
> +
> + return mdiobus_register(bp->mii_bus);
> +}
> +
> static int macb_mii_init(struct macb *bp)
> {
> struct macb_platform_data *pdata;
> struct device_node *np;
> - int err = -ENXIO, i;
> + int err = -ENXIO;
>
> /* Enable management port */
> macb_writel(bp, NCR, MACB_BIT(MPE));
> @@ -446,33 +497,10 @@ static int macb_mii_init(struct macb *bp)
> dev_set_drvdata(>dev->dev, bp->mii_bus);
>
> np = bp->pdev->dev.of_node;
> - if (np) {
> - /* try dt phy registration */
> - err = of_mdiobus_register(bp->mii_bus, np);
> -
> - /* fallback to standard phy registration if no phy were
> - * found during dt phy registration
> - */
> - if (!err && !phy_find_first(bp->mii_bus)) {
> - for (i = 0; i < PHY_MAX_ADDR; i++) {
> - struct phy_device *phydev;
> -
> - phydev = mdiobus_scan(bp->mii_bus, i);
> - if (IS_ERR(phydev)) {
> - err = PTR_ERR(phydev);
> - break;
> - }
> - }
> -
> - if (err)
> - goto err_out_unregister_bus;
> - }
> - } else {
> - if (pdata)
> - bp->mii_bus->phy_mask = pdata->phy_mask;
> -
> - err = mdiobus_register(bp->mii_bus);
> - }
> + if (np)
> + err = macb_mii_of_init(bp, np);
> + else
> + err = macb_mii_pdata_init(bp, pdata);
>
> if (err)
> goto err_out_free_mdiobus;
I'm okay with this. Thanks for having taken the initiative to implement it.
Bye,
--
Nicolas Ferre
Re: [PATCH RFT v2 2/2] macb: kill PHY reset code
Le 29/04/2016 00:15, Sergei Shtylyov a écrit :
> With the 'phylib' now being aware of the "reset-gpios" PHY node property,
> there should be no need to frob the PHY reset in this driver anymore...
>
> Signed-off-by: Sergei Shtylyov <sergei.shtyl...@cogentembedded.com>
Acked-by: Nicolas Ferre <nicolas.fe...@atmel.com>
So I queue my DT patch through arm-soc.
Thanks Sergei, bye.
> ---
> drivers/net/ethernet/cadence/macb.c | 17 -
> drivers/net/ethernet/cadence/macb.h |1 -
> 2 files changed, 18 deletions(-)
>
> Index: net-next/drivers/net/ethernet/cadence/macb.c
> ===
> --- net-next.orig/drivers/net/ethernet/cadence/macb.c
> +++ net-next/drivers/net/ethernet/cadence/macb.c
> @@ -2884,7 +2884,6 @@ static int macb_probe(struct platform_de
> = macb_clk_init;
> int (*init)(struct platform_device *) = macb_init;
> struct device_node *np = pdev->dev.of_node;
> - struct device_node *phy_node;
> const struct macb_config *macb_config = NULL;
> struct clk *pclk, *hclk = NULL, *tx_clk = NULL;
> unsigned int queue_mask, num_queues;
> @@ -2977,18 +2976,6 @@ static int macb_probe(struct platform_de
> else
> macb_get_hwaddr(bp);
>
> - /* Power up the PHY if there is a GPIO reset */
> - phy_node = of_get_next_available_child(np, NULL);
> - if (phy_node) {
> - int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0);
> -
> - if (gpio_is_valid(gpio)) {
> - bp->reset_gpio = gpio_to_desc(gpio);
> - gpiod_direction_output(bp->reset_gpio, 1);
> - }
> - }
> - of_node_put(phy_node);
> -
> err = of_get_phy_mode(np);
> if (err < 0) {
> pdata = dev_get_platdata(>dev);
> @@ -3054,10 +3041,6 @@ static int macb_remove(struct platform_d
> mdiobus_unregister(bp->mii_bus);
> mdiobus_free(bp->mii_bus);
>
> - /* Shutdown the PHY if there is a GPIO reset */
> - if (bp->reset_gpio)
> - gpiod_set_value(bp->reset_gpio, 0);
> -
> unregister_netdev(dev);
> clk_disable_unprepare(bp->tx_clk);
> clk_disable_unprepare(bp->hclk);
> Index: net-next/drivers/net/ethernet/cadence/macb.h
> ===
> --- net-next.orig/drivers/net/ethernet/cadence/macb.h
> +++ net-next/drivers/net/ethernet/cadence/macb.h
> @@ -832,7 +832,6 @@ struct macb {
> unsigned intdma_burst_length;
>
> phy_interface_t phy_interface;
> - struct gpio_desc*reset_gpio;
>
> /* AT91RM9200 transmit */
> struct sk_buff *skb;/* holds skb until xmit
> interrupt completes */
>
>
--
Nicolas Ferre
Re: [PATCH v2] net: macb: do not scan PHYs manually
Le 28/04/2016 16:46, Nathan Sullivan a écrit :
> Since of_mdiobus_register and mdiobus_register will scan automatically,
> do not manually scan for PHY devices in the macb ethernet driver. Doing
> so will result in many nonexistent PHYs on the MDIO bus if the MDIO
> lines are floating or grounded, such as when they are not used.
>
> Signed-off-by: Nathan Sullivan <nathan.sulli...@ni.com>
Well, as explained in the commit message that added this feature and in
the comment, if no phy is specified in the DT we end up without phy...
There are AT91 platforms which lack specification for the phy node in
the DT. So, I don't know if there is a better way to deal with this case
but I see this removal as risky.
Bye,
> ---
> drivers/net/ethernet/cadence/macb.c | 19 +--
> 1 file changed, 1 insertion(+), 18 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb.c
> b/drivers/net/ethernet/cadence/macb.c
> index 48a7d7d..6506b4e 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -424,7 +424,7 @@ static int macb_mii_init(struct macb *bp)
> {
> struct macb_platform_data *pdata;
> struct device_node *np;
> - int err = -ENXIO, i;
> + int err = -ENXIO;
>
> /* Enable management port */
> macb_writel(bp, NCR, MACB_BIT(MPE));
> @@ -450,23 +450,6 @@ static int macb_mii_init(struct macb *bp)
> if (np) {
> /* try dt phy registration */
> err = of_mdiobus_register(bp->mii_bus, np);
> -
> - /* fallback to standard phy registration if no phy were
> -found during dt phy registration */
> - if (!err && !phy_find_first(bp->mii_bus)) {
> - for (i = 0; i < PHY_MAX_ADDR; i++) {
> - struct phy_device *phydev;
> -
> - phydev = mdiobus_scan(bp->mii_bus, i);
> - if (IS_ERR(phydev)) {
> - err = PTR_ERR(phydev);
> - break;
> - }
> - }
> -
> - if (err)
> - goto err_out_unregister_bus;
> - }
> } else {
> if (pdata)
> bp->mii_bus->phy_mask = pdata->phy_mask;
>
--
Nicolas Ferre
Re: [PATCH] ARM: dts: at91: VInCo: fix phy reset gpio flag
Le 26/04/2016 20:27, Sergei Shtylyov a écrit : > On 04/26/2016 08:17 PM, David Miller wrote: > >>> I plan to queue this patch through arm-soc for 4.7. >> >> Ok. > > How about this patch going thru your net-next repo instead? > I'd like to keep the kernel bisectable... if my phylib/macb patches get > merged > earlier than this one, that board would be broken... Sergei, David, I don't think that there is a big risk for this board to be tested in the meantime as it's not widely deployed yet. And as I'm aware of the issue and basically maintaining this DT file, I think that I'll be informed if people try an unlikely arrangement of patches on this board. So either way, I'm okay. But I do think it's not worth thinking too much about this case. Bye, -- Nicolas Ferre
[PATCH] ARM: dts: at91: VInCo: fix phy reset gpio flag
Fix gpio active flag for the phy reset-gpios property. The line is
active low instead of active high.
Actually, this flags was never used by the macb driver.
Reported-by: Sergei Shtylyov <sergei.shtyl...@cogentembedded.com>
Cc: Andrew Lunn <and...@lunn.ch>
Cc: David Miller <da...@davemloft.net>
Signed-off-by: Nicolas Ferre <nicolas.fe...@atmel.com>
---
Hi,
Thanks for having reported this bug to me.
I hope that we will be able to move forward for changing the phy
reset code in the macb driver.
I plan to queue this patch through arm-soc for 4.7.
Thanks, best regards,
Nicolas
arch/arm/boot/dts/at91-vinco.dts | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/boot/dts/at91-vinco.dts b/arch/arm/boot/dts/at91-vinco.dts
index 79aec55e1ebc..6a366ee952a8 100644
--- a/arch/arm/boot/dts/at91-vinco.dts
+++ b/arch/arm/boot/dts/at91-vinco.dts
@@ -118,7 +118,7 @@
ethernet-phy@1 {
reg = <0x1>;
- reset-gpios = < 8
GPIO_ACTIVE_HIGH>;
+ reset-gpios = < 8 GPIO_ACTIVE_LOW>;
interrupt-parent = <>;
interrupts = <15 IRQ_TYPE_EDGE_FALLING>;
};
@@ -162,7 +162,7 @@
reg = <0x1>;
interrupt-parent = <>;
interrupts = <31 IRQ_TYPE_EDGE_FALLING>;
- reset-gpios = < 6
GPIO_ACTIVE_HIGH>;
+ reset-gpios = < 6 GPIO_ACTIVE_LOW>;
};
};
--
2.1.3
Re: [PATCH RFT 2/2] macb: kill PHY reset code
Le 12/04/2016 15:54, Sergei Shtylyov a écrit :
> Hello.
>
> On 4/12/2016 12:22 PM, Nicolas Ferre wrote:
>
>>>> With the 'phylib' now being aware of the "reset-gpios" PHY node
>>>> property,
>>>> there should be no need to frob the PHY reset in this driver anymore...
>>>>
>>>> Signed-off-by: Sergei Shtylyov <sergei.shtyl...@cogentembedded.com>
>>>>
>>>> ---
>>>> drivers/net/ethernet/cadence/macb.c | 17 -
>>>> drivers/net/ethernet/cadence/macb.h |1 -
>>>> 2 files changed, 18 deletions(-)
>>>>
>>>> Index: net-next/drivers/net/ethernet/cadence/macb.c
>>>> ===
>>>> --- net-next.orig/drivers/net/ethernet/cadence/macb.c
>>>> +++ net-next/drivers/net/ethernet/cadence/macb.c
> [...]
>>>> @@ -2977,18 +2976,6 @@ static int macb_probe(struct platform_de
>>>>else
>>>>macb_get_hwaddr(bp);
>>>>
>>>> - /* Power up the PHY if there is a GPIO reset */
>>>> - phy_node = of_get_next_available_child(np, NULL);
>>>> - if (phy_node) {
>>>> - int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0);
>>>> -
>>>> - if (gpio_is_valid(gpio)) {
>>>> - bp->reset_gpio = gpio_to_desc(gpio);
>>>> - gpiod_direction_output(bp->reset_gpio, 1);
>>>
>>> Hi Sergei
>>>
>>> The code you are deleting would of ignored the flags in the gpio
>>
>> I don't parse this.
>
>> The code deleted does take the flag into account.
>
> Not really -- you need to call of_get_named_gpio_flags() (with a valid
> last argument) for that.
Yep,
>> And the DT property
>> associated to it seems correct to me (I mean, with proper flag
>> specification).
>
> It apparently is not as it have GPIO_ACTIVE_HIGH and the driver assumes
> active-low reset signal.
Yes, logic was inverted and... anyway, the flag never used for real...
Thanks Sergei.
No problem for me accepting a patch for the at91-vinco.dts.
Bye,
--
Nicolas Ferre
Re: [PATCH RFT 2/2] macb: kill PHY reset code
Le 12/04/2016 15:40, Andrew Lunn a écrit :
> On Tue, Apr 12, 2016 at 11:22:10AM +0200, Nicolas Ferre wrote:
>> Le 11/04/2016 04:28, Andrew Lunn a écrit :
>>> On Sat, Apr 09, 2016 at 01:25:03AM +0300, Sergei Shtylyov wrote:
>>>> With the 'phylib' now being aware of the "reset-gpios" PHY node
>>>> property,
>>>> there should be no need to frob the PHY reset in this driver anymore...
>>>>
>>>> Signed-off-by: Sergei Shtylyov <sergei.shtyl...@cogentembedded.com>
>>>>
>>>> ---
>>>> drivers/net/ethernet/cadence/macb.c | 17 -
>>>> drivers/net/ethernet/cadence/macb.h |1 -
>>>> 2 files changed, 18 deletions(-)
>>>>
>>>> Index: net-next/drivers/net/ethernet/cadence/macb.c
>>>> ===
>>>> --- net-next.orig/drivers/net/ethernet/cadence/macb.c
>>>> +++ net-next/drivers/net/ethernet/cadence/macb.c
>>>> @@ -2884,7 +2884,6 @@ static int macb_probe(struct platform_de
>>>> = macb_clk_init;
>>>>int (*init)(struct platform_device *) = macb_init;
>>>>struct device_node *np = pdev->dev.of_node;
>>>> - struct device_node *phy_node;
>>>>const struct macb_config *macb_config = NULL;
>>>>struct clk *pclk, *hclk = NULL, *tx_clk = NULL;
>>>>unsigned int queue_mask, num_queues;
>>>> @@ -2977,18 +2976,6 @@ static int macb_probe(struct platform_de
>>>>else
>>>>macb_get_hwaddr(bp);
>>>>
>>>> - /* Power up the PHY if there is a GPIO reset */
>>>> - phy_node = of_get_next_available_child(np, NULL);
>>>> - if (phy_node) {
>>>> - int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0);
>>>> -
>>>> - if (gpio_is_valid(gpio)) {
>>>> - bp->reset_gpio = gpio_to_desc(gpio);
>>>> - gpiod_direction_output(bp->reset_gpio, 1);
>>>
>>> Hi Sergei
>>>
>>> The code you are deleting would of ignored the flags in the gpio
>> I don't parse this.
>>
>> The code deleted does take the flag into account. And the DT property
>> associated to it seems correct to me (I mean, with proper flag
>> specification).
>
> Hi Nicolas
>
> of_get_named_gpio() does not do anything with the flags. So for
> example,
>
> gpios = < 12 GPIO_ACTIVE_LOW>;
>
> the GPIO_ACTIVE_LOW would be ignored. If you want the flags to be
> respected, you need to use the gpiod API for all calls, in particular,
> you need to use something which calls gpiod_get_index(), since that is
> the only function to call gpiod_parse_flags() to translate
> GPIO_ACTIVE_LOW into a flag within the gpio descriptor.
Ok, I remember what confused me now: this code, used to be something around:
devm_gpiod_get_optional(>pdev->dev, "phy-reset", GPIOD_OUT_HIGH);
before it has been changed to the chunk above... So, yes, the DT flag
was not handled anyway...
Sorry for the noise and thanks for the clarification.
Bye,
--
Nicolas Ferre
Re: GMII2RGMII Converter support in macb driver
> + 0,
> 0);
>
> +if (!phydev) {
>
> + dev_err(>pdev->dev, "%s:
> no gmii to rgmii converter found\n",
>
> + dev->name);
>
> + return -1;
>
> +}
>
> +bp->gmii2rgmii_phy_dev = phydev;
>
> + } else
>
> +bp->gmii2rgmii_phy_dev = NULL;
>
> +
>
> phydev = phy_find_first(bp->mii_bus);
>
> if (!phydev) {
>
> netdev_err(dev, "no PHY found\n");
>
> @@ -402,6 +435,8 @@ static int macb_mii_probe(struct net_device *dev)
>
>
> bp->phy_interface);
>
> if (ret) {
>
> netdev_err(dev, "Could not attach to PHY\n");
>
> +if (bp->gmii2rgmii_phy_dev)
>
> +
> phy_disconnect(bp->gmii2rgmii_phy_dev);
>
> return ret;
>
> }
>
> @@ -3368,6 +3403,9 @@ static int macb_probe(struct platform_device *pdev)
>
> bp->phy_interface = err;
>
> }
>
> + bp->gmii2rgmii_phy_node =
> of_parse_phandle(bp->pdev->dev.of_node,
>
> +
>
> "gmii2rgmii-phy-handle", 0);
>
> +
>
> macb_reset_phy(pdev);
>
>/* IP specific init */
>
> @@ -3422,6 +3460,8 @@ static int macb_remove(struct platform_device *pdev)
>
> bp = netdev_priv(dev);
>
> if (bp->phy_dev)
>
> phy_disconnect(bp->phy_dev);
>
> +if (bp->gmii2rgmii_phy_dev)
>
> +
> phy_disconnect(bp->gmii2rgmii_phy_dev);
>
>
>
> But doing above changes making driver looks odd.
>
> could you please suggest any better option to add support for this IP in
> the macb driver?
Appana,
I certainly can't prototype the solution based on your datasheet and the
code sent... do a sensible proposal, then we can evaluate.
As the IP is separated from the Eth controller, make it a separate
driver (an emulated phy one for instance... even if I don't know if it
makes sense).
I don't know if others have already made such an adaptation layer
between GMII to RGMII but I'm pretty sure it can't be inserted into the
macb driver.
Bye,
--
Nicolas Ferre
Re: [PATCH RFT 2/2] macb: kill PHY reset code
Le 11/04/2016 20:51, Andrew Lunn a écrit : > On Mon, Apr 11, 2016 at 09:39:02PM +0300, Sergei Shtylyov wrote: >> Hello. >> >> On 04/11/2016 09:19 PM, Andrew Lunn wrote: >> >>>>> The code you are deleting would of ignored the flags in the gpio >>>>> property, i.e. active low. >>>> >>>>Hm, you're right -- I forgot about that... :-/ >>>> >>>>> The new code in the previous patch does >>>>> however take the flags into account. Did you check if there are any >>>>> device trees which have flags, which were never used, but are now >>>>> going to be used and thus break... >>>> >>>>Checked this now and found out arch/arm/boot/dts/ar91-vinco.dts. >>>> Looks like it needs to be fixed indeed... >>>> >>> And this is where it gets tricky. You are breaking backwards >>> compatibility by now respecting the flag. An old DT blob is not going >>> to work. >> >>Do we care that much about the DT blobs that are just *wrong*? > > Wrong, but currently works. > >>> You potentially need to add a new property and deprecate the old one. >> >>I would like to avoid that... > > You will need the agreement from the at91-vinco maintainer. If the at91-vinco has to be modified, you have my agreement that it can be modified. Bye, -- Nicolas Ferre
Re: [PATCH RFT 2/2] macb: kill PHY reset code
Le 11/04/2016 04:28, Andrew Lunn a écrit :
> On Sat, Apr 09, 2016 at 01:25:03AM +0300, Sergei Shtylyov wrote:
>> With the 'phylib' now being aware of the "reset-gpios" PHY node property,
>> there should be no need to frob the PHY reset in this driver anymore...
>>
>> Signed-off-by: Sergei Shtylyov <sergei.shtyl...@cogentembedded.com>
>>
>> ---
>> drivers/net/ethernet/cadence/macb.c | 17 -
>> drivers/net/ethernet/cadence/macb.h |1 -
>> 2 files changed, 18 deletions(-)
>>
>> Index: net-next/drivers/net/ethernet/cadence/macb.c
>> ===
>> --- net-next.orig/drivers/net/ethernet/cadence/macb.c
>> +++ net-next/drivers/net/ethernet/cadence/macb.c
>> @@ -2884,7 +2884,6 @@ static int macb_probe(struct platform_de
>>= macb_clk_init;
>> int (*init)(struct platform_device *) = macb_init;
>> struct device_node *np = pdev->dev.of_node;
>> -struct device_node *phy_node;
>> const struct macb_config *macb_config = NULL;
>> struct clk *pclk, *hclk = NULL, *tx_clk = NULL;
>> unsigned int queue_mask, num_queues;
>> @@ -2977,18 +2976,6 @@ static int macb_probe(struct platform_de
>> else
>> macb_get_hwaddr(bp);
>>
>> -/* Power up the PHY if there is a GPIO reset */
>> -phy_node = of_get_next_available_child(np, NULL);
>> -if (phy_node) {
>> -int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0);
>> -
>> -if (gpio_is_valid(gpio)) {
>> -bp->reset_gpio = gpio_to_desc(gpio);
>> -gpiod_direction_output(bp->reset_gpio, 1);
>
> Hi Sergei
>
> The code you are deleting would of ignored the flags in the gpio
I don't parse this.
The code deleted does take the flag into account. And the DT property
associated to it seems correct to me (I mean, with proper flag
specification).
> property, i.e. active low. The new code in the previous patch does
> however take the flags into account. Did you check if there are any
> device trees which have flags, which were never used, but are now
> going to be used and thus break...
Flag was used and you are saying that it's taken into account in new
code... So, what's the issue?
I see a difference in the way the "value" of gpiod_* functions is used.
There may be a misunderstanding here...
Bye,
--
Nicolas Ferre
Re: [PATCH 1/1] net: macb: replace macb_writel() call by queue_writel() to update queue ISR
Le 24/03/2016 15:40, Cyrille Pitchen a écrit :
> macb_interrupt() should not use macb_writel(bp, ISR, ) but only
> queue_writel(queue, ISR, ).
>
> There is one IRQ and one set of {ISR, IER, IDR, IMR} [1] registers per
> queue on gem hardware, though only queue0 is actually used for now to
> receive frames: other queues can already be used to transmit frames.
>
> The queue_readl() and queue_writel() helper macros are designed to access
> the relevant IRQ registers.
>
> [1]
> ISR: Interrupt Status Register
> IER: Interrupt Enable Register
> IDR: Interrupt Disable Register
> IMR: Interrupt Mask Register
>
> Signed-off-by: Cyrille Pitchen <cyrille.pitc...@atmel.com>
> Fixes: bfbb92c44670 ("net: macb: Handle the RXUBR interrupt on all devices")
Acked-by: Nicolas Ferre <nicolas.fe...@atmel.com>
Thanks!
> ---
> drivers/net/ethernet/cadence/macb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb.c
> b/drivers/net/ethernet/cadence/macb.c
> index 39447a337149..c9c6b2762a39 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -1146,7 +1146,7 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
> macb_writel(bp, NCR, ctrl | MACB_BIT(RE));
>
> if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
> - macb_writel(bp, ISR, MACB_BIT(RXUBR));
> + queue_writel(queue, ISR, MACB_BIT(RXUBR));
> }
>
> if (status & MACB_BIT(ISR_ROVR)) {
>
--
Nicolas Ferre
Re: [PATCH] macb: fix PHY reset
Le 22/03/2016 22:33, Sergei Shtylyov a écrit :
> On 03/22/2016 11:07 PM, David Miller wrote:
>
>>> On 03/22/2016 10:27 PM, Sergei Shtylyov wrote:
>>>
>>>> The driver calls gpiod_set_value() with GPIOD_OUT_* instead of 0 and
>>>> 1, as
>>>> a result the PHY isn't really put back into reset state in
>>>> macb_remove().
>>>> Moreover, the driver assumes that something else has set the GPIO
>>>> direction
>>>> to output, so if it has not, the PHY wouldn't be taken out of reset in
>>>
>>> s/wouldn't/may not/, sorry. Do I need to resend?
>>
>> No need, I fixed it up by hand.
>>
>> Applied, thanks.
>
> Oops, forgot another tag:
>
> Fixes: 270c499f0993 ("net/macb: Update device tree binding for resetting PHY
> using GPIO")
>
>Too late probably... :-(
Too late also:
Acked-by: Nicolas Ferre <nicolas.fe...@atmel.com>
Thanks Sergei!
Bye,
--
Nicolas Ferre
Re: [PATCH 2/5] net: macb: Fix coding style warnings
Le 14/03/2016 21:53, Michal Simek a écrit : > On 13.3.2016 20:10, Moritz Fischer wrote: >> This commit takes care of the coding style warnings >> that are mostly due to a different comment style and >> lines over 80 chars, as well as a dangling else. >> >> Signed-off-by: Moritz Fischer <moritz.fisc...@ettus.com> >> --- >> drivers/net/ethernet/cadence/macb.c | 101 >> +++- >> 1 file changed, 43 insertions(+), 58 deletions(-) >> >> diff --git a/drivers/net/ethernet/cadence/macb.c >> b/drivers/net/ethernet/cadence/macb.c >> index 4370f37..c2d31c5 100644 >> --- a/drivers/net/ethernet/cadence/macb.c >> +++ b/drivers/net/ethernet/cadence/macb.c >> @@ -58,8 +58,7 @@ >> >> #define GEM_MTU_MIN_SIZE68 >> >> -/* >> - * Graceful stop timeouts in us. We should allow up to >> +/* Graceful stop timeouts in us. We should allow up to >> * 1 frame time (10 Mbits/s, full-duplex, ignoring collisions) >> */ >> #define MACB_HALT_TIMEOUT 1230 >> @@ -127,8 +126,7 @@ static void hw_writel(struct macb *bp, int offset, u32 >> value) >> writel_relaxed(value, bp->regs + offset); >> } >> >> -/* >> - * Find the CPU endianness by using the loopback bit of NCR register. When >> the >> +/* Find the CPU endianness by using the loopback bit of NCR register. When >> the > > TBH: I would rather see this converting to kernel-doc format instead of > using this networking block. As there is hardly any kernel-doc comments in this driver, I won't force Moritz to move this one to it. I would advice, if someone want to move to kernel-doc for some function comments, to do it in a separate patch (series). > Also splitting this to more patches will be better. Just by categories > but that's just my opinion. Well, yes... but I won't be too picky for such a patch. So here is my: Acked-by: Nicolas Ferre <nicolas.fe...@atmel.com> Thank for your feedback, bye, -- Nicolas Ferre
Re: [PATCH 1/5] net: macb: Fix coding style error message
Le 13/03/2016 20:10, Moritz Fischer a écrit :
> checkpatch.pl gave the following error:
>
> ERROR: space required before the open parenthesis '('
> + for(; p < end; p++, offset += 4)
>
> Signed-off-by: Moritz Fischer <moritz.fisc...@ettus.com>
Acked-by: Nicolas Ferre <nicolas.fe...@atmel.com>
> ---
> drivers/net/ethernet/cadence/macb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb.c
> b/drivers/net/ethernet/cadence/macb.c
> index 50c9410..4370f37 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -496,7 +496,7 @@ static void macb_update_stats(struct macb *bp)
>
> WARN_ON((unsigned long)(end - p - 1) != (MACB_TPF - MACB_PFR) / 4);
>
> - for(; p < end; p++, offset += 4)
> + for (; p < end; p++, offset += 4)
> *p += bp->macb_reg_readl(bp, offset);
> }
>
>
--
Nicolas Ferre
Re: [PATCH 4/5] net: macb: Use ether_addr_copy over memcpy
Le 13/03/2016 20:10, Moritz Fischer a écrit : > Checkpatch suggests using ether_addr_copy over memcpy > to copy the mac address. > > Signed-off-by: Moritz Fischer <moritz.fisc...@ettus.com> Yes: Acked-by: Nicolas Ferre <nicolas.fe...@atmel.com> > --- > drivers/net/ethernet/cadence/macb.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/cadence/macb.c > b/drivers/net/ethernet/cadence/macb.c > index 53400f6..a0c01e5 100644 > --- a/drivers/net/ethernet/cadence/macb.c > +++ b/drivers/net/ethernet/cadence/macb.c > @@ -2891,7 +2891,7 @@ static int macb_probe(struct platform_device *pdev) > > mac = of_get_mac_address(np); > if (mac) > - memcpy(bp->dev->dev_addr, mac, ETH_ALEN); > + ether_addr_copy(bp->dev->dev_addr, mac); > else > macb_get_hwaddr(bp); > > -- Nicolas Ferre
Re: [PATCH 5/5] net: macb: Fix simple typo.
Le 14/03/2016 21:47, Michal Simek a écrit : > On 13.3.2016 20:10, Moritz Fischer wrote: >> Signed-off-by: Moritz Fischer <moritz.fisc...@ettus.com> >> --- >> drivers/net/ethernet/cadence/macb.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/cadence/macb.c >> b/drivers/net/ethernet/cadence/macb.c >> index a0c01e5..681e5bf 100644 >> --- a/drivers/net/ethernet/cadence/macb.c >> +++ b/drivers/net/ethernet/cadence/macb.c >> @@ -127,7 +127,7 @@ static void hw_writel(struct macb *bp, int offset, u32 >> value) >> } >> >> /* Find the CPU endianness by using the loopback bit of NCR register. When >> the >> - * CPU is in big endian we need to program swaped mode for management >> + * CPU is in big endian we need to program swapped mode for management >> * descriptor access. >> */ >> static bool hw_is_native_io(void __iomem *addr) >> > > Remove dot at the end of subject and feel free to add my: > Acked-by: Michal Simek <michal.si...@xilinx.com> Yes, same for me, no dot. But anyway, here is my: Acked-by: Nicolas Ferre <nicolas.fe...@atmel.com> Thanks Moritz for the patches and Michal for the reviews. Bye, -- Nicolas Ferre
Re: [PATCH 0/5] net: macb: Checkpatch cleanups
Le 13/03/2016 20:10, Moritz Fischer a écrit : > Hi all, > > I backed out the variable scope changes and made a separate > patch for the ether_addr_copy change. > > Changes from v1: As it's v2, it's better to add it in each subject of the patch series like: "[PATCH v2 0/5] net: macb: Checkpatch cleanups" > * Backed out variable scope changes > * Separated out ether_addr_copy into it's own commit > * Fixed typo in comments as suggested by Joe > > Cheers, > > Moritz > > Moritz Fischer (5): > net: macb: Fix coding style error message > net: macb: Fix coding style warnings > net: macb: Address checkpatch 'check' suggestions > net: macb: Use ether_addr_copy over memcpy > net: macb: Fix simple typo. > > drivers/net/ethernet/cadence/macb.c | 153 > +----------- > 1 file changed, 70 insertions(+), 83 deletions(-) > -- Nicolas Ferre
[PATCH] net: macb: fix default configuration for GMAC on AT91
On AT91 SoCs, the User Register (USRIO) exposes a switch to configure the
"Reduced" or "Traditional" version of the Media Independent Interface
(RMII vs. MII or RGMII vs. GMII).
As on the older EMAC version, on GMAC, this switch is set by default to the
non-reduced type of interface, so use the existing capability and extend it to
GMII as well. We then keep the current logic in the macb_init() function.
The capabilities of sama5d2, sama5d4 and sama5d3 GEM interface are updated in
the macb_config structure to be able to properly enable them with a traditional
interface (GMII or MII).
Reported-by: Romain HENRIET <romain.henr...@l-acoustics.com>
Signed-off-by: Nicolas Ferre <nicolas.fe...@atmel.com>
---
drivers/net/ethernet/cadence/macb.c | 13 +++--
drivers/net/ethernet/cadence/macb.h | 2 +-
2 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/cadence/macb.c
b/drivers/net/ethernet/cadence/macb.c
index 7ccf2298a5fa..3ce6095ced3d 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -2440,9 +2440,9 @@ static int macb_init(struct platform_device *pdev)
if (bp->phy_interface == PHY_INTERFACE_MODE_RGMII)
val = GEM_BIT(RGMII);
else if (bp->phy_interface == PHY_INTERFACE_MODE_RMII &&
-(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII))
+(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
val = MACB_BIT(RMII);
- else if (!(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII))
+ else if (!(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
val = MACB_BIT(MII);
if (bp->caps & MACB_CAPS_USRIO_HAS_CLKEN)
@@ -2774,7 +2774,7 @@ static int at91ether_init(struct platform_device *pdev)
}
static const struct macb_config at91sam9260_config = {
- .caps = MACB_CAPS_USRIO_HAS_CLKEN | MACB_CAPS_USRIO_DEFAULT_IS_MII,
+ .caps = MACB_CAPS_USRIO_HAS_CLKEN | MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII,
.clk_init = macb_clk_init,
.init = macb_init,
};
@@ -2787,21 +2787,22 @@ static const struct macb_config pc302gem_config = {
};
static const struct macb_config sama5d2_config = {
- .caps = 0,
+ .caps = MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII,
.dma_burst_length = 16,
.clk_init = macb_clk_init,
.init = macb_init,
};
static const struct macb_config sama5d3_config = {
- .caps = MACB_CAPS_SG_DISABLED | MACB_CAPS_GIGABIT_MODE_AVAILABLE,
+ .caps = MACB_CAPS_SG_DISABLED | MACB_CAPS_GIGABIT_MODE_AVAILABLE
+ | MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII,
.dma_burst_length = 16,
.clk_init = macb_clk_init,
.init = macb_init,
};
static const struct macb_config sama5d4_config = {
- .caps = 0,
+ .caps = MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII,
.dma_burst_length = 4,
.clk_init = macb_clk_init,
.init = macb_init,
diff --git a/drivers/net/ethernet/cadence/macb.h
b/drivers/net/ethernet/cadence/macb.h
index 9ba416d5afff..8a13824ef802 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -400,7 +400,7 @@
/* Capability mask bits */
#define MACB_CAPS_ISR_CLEAR_ON_WRITE 0x0001
#define MACB_CAPS_USRIO_HAS_CLKEN 0x0002
-#define MACB_CAPS_USRIO_DEFAULT_IS_MII 0x0004
+#define MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII0x0004
#define MACB_CAPS_NO_GIGABIT_HALF 0x0008
#define MACB_CAPS_USRIO_DISABLED 0x0010
#define MACB_CAPS_FIFO_MODE0x1000
--
2.1.3
Re: [PATCH 0/3] net: macb: Fix coding style issues
Le 07/03/2016 17:17, Moritz Fischer a écrit : > Hi Nicolas, > > this series deals with most of the checkpatch warnings > generated for macb. There are two BUG_ON()'s that I didn't touch, yet, > that were suggested by checkpatch, that I can address in a follow up > commit if needed. > Let me know if you want me to split the fixes differently or squash > them into one commit. Hi, I'm not usually fond of this type of patches, but I must admit that this series corrects some style issues. So, I would like more feedback from Michal and Cyrille as these changes may delay some of the not-merged-yet features or more important work-in-progress on their side. On the other hand, if we all think it's a calm period for this macb driver, we may find interesting to merge some "cleanup and style" enhancements. Thanks, bye, > Moritz Fischer (3): > net: macb: Fix coding style error message > net: macb: Fix more coding style issues > net: macb: Address checkpatch 'check' suggestions > > drivers/net/ethernet/cadence/macb.c | 157 > > 1 file changed, 71 insertions(+), 86 deletions(-) > -- Nicolas Ferre
Re: [PATCH 7/9] net: macb: avoid uninitialized variables
Le 27/01/2016 16:51, Nicolas Ferre a écrit :
> Le 27/01/2016 15:04, Arnd Bergmann a écrit :
>> The macb_clk_init function returns three clock pointers, unless
>> the it fails to get the first ones. We correctly handle the
>> failure case by propagating the error from macb_probe, but
>> gcc does not realize this and incorrectly warns about a later
>> use of those:
>>
>> In file included from /git/arm-soc/drivers/net/ethernet/cadence/macb.c:12:0:
>> drivers/net/ethernet/cadence/macb.c: In function 'macb_probe':
>> include/linux/clk.h:484:2: error: 'tx_clk' may be used uninitialized in this
>> function [-Werror=maybe-uninitialized]
>> clk_disable(clk);
>> ^
>> drivers/net/ethernet/cadence/macb.c:2822:28: note: 'tx_clk' was declared here
>> struct clk *pclk, *hclk, *tx_clk;
>> ^
>> In file included from /git/arm-soc/drivers/net/ethernet/cadence/macb.c:12:0:
>> include/linux/clk.h:484:2: error: 'hclk' may be used uninitialized in this
>> function [-Werror=maybe-uninitialized]
>> clk_disable(clk);
>> ^
>> drivers/net/ethernet/cadence/macb.c:2822:21: note: 'hclk' was declared here
>> struct clk *pclk, *hclk, *tx_clk;
>> ^
>>
>> This shuts up the misleading warnings by ensuring that the
>> macb_clk_init() always stores something into all three pointers.
>>
>> Signed-off-by: Arnd Bergmann <a...@arndb.de>
>
> Okay Arnd, thanks!
>
> Acked-by: Nicolas Ferre <nicolas.fe...@atmel.com>
Oh, crap: actually this warning has just been fixed by Sudip Mukherjee
and is already queued by David here:
https://patchwork.ozlabs.org/patch/572610/
So, sorry, I've shot too fast: NACK...
Bye,
>> ---
>> drivers/net/ethernet/cadence/macb.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/net/ethernet/cadence/macb.c
>> b/drivers/net/ethernet/cadence/macb.c
>> index 9d9984a87d42..d3aa74f9db79 100644
>> --- a/drivers/net/ethernet/cadence/macb.c
>> +++ b/drivers/net/ethernet/cadence/macb.c
>> @@ -2268,6 +2268,7 @@ static int macb_clk_init(struct platform_device *pdev,
>> struct clk **pclk,
>> {
>> int err;
>>
>> +*tx_clk = *hclk = NULL;
>> *pclk = devm_clk_get(>dev, "pclk");
>> if (IS_ERR(*pclk)) {
>> err = PTR_ERR(*pclk);
>>
>
>
--
Nicolas Ferre
Re: [PATCH 7/9] net: macb: avoid uninitialized variables
Le 27/01/2016 15:04, Arnd Bergmann a écrit :
> The macb_clk_init function returns three clock pointers, unless
> the it fails to get the first ones. We correctly handle the
> failure case by propagating the error from macb_probe, but
> gcc does not realize this and incorrectly warns about a later
> use of those:
>
> In file included from /git/arm-soc/drivers/net/ethernet/cadence/macb.c:12:0:
> drivers/net/ethernet/cadence/macb.c: In function 'macb_probe':
> include/linux/clk.h:484:2: error: 'tx_clk' may be used uninitialized in this
> function [-Werror=maybe-uninitialized]
> clk_disable(clk);
> ^
> drivers/net/ethernet/cadence/macb.c:2822:28: note: 'tx_clk' was declared here
> struct clk *pclk, *hclk, *tx_clk;
> ^
> In file included from /git/arm-soc/drivers/net/ethernet/cadence/macb.c:12:0:
> include/linux/clk.h:484:2: error: 'hclk' may be used uninitialized in this
> function [-Werror=maybe-uninitialized]
> clk_disable(clk);
> ^
> drivers/net/ethernet/cadence/macb.c:2822:21: note: 'hclk' was declared here
> struct clk *pclk, *hclk, *tx_clk;
> ^
>
> This shuts up the misleading warnings by ensuring that the
> macb_clk_init() always stores something into all three pointers.
>
> Signed-off-by: Arnd Bergmann <a...@arndb.de>
Okay Arnd, thanks!
Acked-by: Nicolas Ferre <nicolas.fe...@atmel.com>
> ---
> drivers/net/ethernet/cadence/macb.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/ethernet/cadence/macb.c
> b/drivers/net/ethernet/cadence/macb.c
> index 9d9984a87d42..d3aa74f9db79 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -2268,6 +2268,7 @@ static int macb_clk_init(struct platform_device *pdev,
> struct clk **pclk,
> {
> int err;
>
> + *tx_clk = *hclk = NULL;
> *pclk = devm_clk_get(>dev, "pclk");
> if (IS_ERR(*pclk)) {
> err = PTR_ERR(*pclk);
>
--
Nicolas Ferre
Re: [PATCH 2/3] net: macb: fix build warning
Le 25/01/2016 07:13, Sudip Mukherjee a écrit : > We are getting build warning about: > macb.c:2889:13: warning: 'tx_clk' may be used uninitialized in this function > macb.c:2888:11: warning: 'hclk' may be used uninitialized in this function > > In reality they are not used uninitialized as clk_init() will initialize > them, this patch will just silence the warning. > > Signed-off-by: Sudip Mukherjee <su...@vectorindia.org> Acked-by: Nicolas Ferre <nicolas.fe...@atmel.com> Thanks for your patch. Bye, > --- > drivers/net/ethernet/cadence/macb.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/cadence/macb.c > b/drivers/net/ethernet/cadence/macb.c > index 9d9984a..50c9410 100644 > --- a/drivers/net/ethernet/cadence/macb.c > +++ b/drivers/net/ethernet/cadence/macb.c > @@ -2823,7 +2823,7 @@ static int macb_probe(struct platform_device *pdev) > struct device_node *np = pdev->dev.of_node; > struct device_node *phy_node; > const struct macb_config *macb_config = NULL; > - struct clk *pclk, *hclk, *tx_clk; > + struct clk *pclk, *hclk = NULL, *tx_clk = NULL; > unsigned int queue_mask, num_queues; > struct macb_platform_data *pdata; > bool native_io; > -- Nicolas Ferre
Re: [PATCH v5 net-next 3/3] dt-bindings: net: macb: Add NP4 macb variant
Le 05/01/2016 14:39, Neil Armstrong a écrit :
> Add NP4 macb SoC variant.
>
> Signed-off-by: Neil Armstrong <narmstr...@baylibre.com>
Acked-by: Nicolas Ferre <nicolas.fe...@atmel.com>
Neil, thanks for your understanding and reactivity concerning this patch
series.
Bye,
> ---
> Documentation/devicetree/bindings/net/macb.txt | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/net/macb.txt
> b/Documentation/devicetree/bindings/net/macb.txt
> index 38c8e84..5c397ca 100644
> --- a/Documentation/devicetree/bindings/net/macb.txt
> +++ b/Documentation/devicetree/bindings/net/macb.txt
> @@ -4,6 +4,7 @@ Required properties:
> - compatible: Should be "cdns,[-]{macb|gem}"
>Use "cdns,at91sam9260-macb" for Atmel at91sam9 SoCs or the 10/100Mbit IP
>available on sama5d3 SoCs.
> + Use "cdns,np4-macb" for NP4 SoC devices.
>Use "cdns,at32ap7000-macb" for other 10/100 usage or use the generic form:
> "cdns,macb".
>Use "cdns,pc302-gem" for Picochip picoXcell pc302 and later devices based
> on
>the Cadence GEM, or the generic form: "cdns,gem".
>
--
Nicolas Ferre
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 net-next 2/3] net: macb: Add NP4 macb config using USRIO_DISABLED
Le 05/01/2016 14:39, Neil Armstrong a écrit :
> Declare a new NP4 SoC variant having USRIO_DISABLED as capability bit.
>
> Signed-off-by: Neil Armstrong <narmstr...@baylibre.com>
Acked-by: Nicolas Ferre <nicolas.fe...@atmel.com>
> ---
> drivers/net/ethernet/cadence/macb.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/net/ethernet/cadence/macb.c
> b/drivers/net/ethernet/cadence/macb.c
> index fa53bc3..d12ee07 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -2781,6 +2781,11 @@ static const struct macb_config emac_config = {
> .init = at91ether_init,
> };
>
> +static const struct macb_config np4_config = {
> + .caps = MACB_CAPS_USRIO_DISABLED,
> + .clk_init = macb_clk_init,
> + .init = macb_init,
> +};
>
> static const struct macb_config zynqmp_config = {
> .caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO,
> @@ -2801,6 +2806,7 @@ static const struct of_device_id macb_dt_ids[] = {
> { .compatible = "cdns,at32ap7000-macb" },
> { .compatible = "cdns,at91sam9260-macb", .data = _config },
> { .compatible = "cdns,macb" },
> + { .compatible = "cdns,np4-macb", .data = _config },
> { .compatible = "cdns,pc302-gem", .data = _config },
> { .compatible = "cdns,gem", .data = _config },
> { .compatible = "atmel,sama5d2-gem", .data = _config },
>
--
Nicolas Ferre
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 net-next 1/3] net: ethernet: cadence-macb: Add disabled usrio caps
Le 05/01/2016 14:39, Neil Armstrong a écrit :
> On some platforms, the macb integration does not use the USRIO
> register to configure the (R)MII port and clocks.
> When the register is not implemented and the MACB error signal
> is connected to the bus error, reading or writing to the USRIO
> register can trigger some Imprecise External Aborts on ARM platforms.
>
> Signed-off-by: Neil Armstrong <narmstr...@baylibre.com>
Acked-by: Nicolas Ferre <nicolas.fe...@atmel.com>
Thanks!
> ---
> drivers/net/ethernet/cadence/macb.c | 27 +++
> drivers/net/ethernet/cadence/macb.h | 1 +
> 2 files changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb.c
> b/drivers/net/ethernet/cadence/macb.c
> index 8b45bc9..fa53bc3 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -2124,7 +2124,8 @@ static void macb_get_regs(struct net_device *dev,
> struct ethtool_regs *regs,
> regs_buff[10] = macb_tx_dma(>queues[0], tail);
> regs_buff[11] = macb_tx_dma(>queues[0], head);
>
> - regs_buff[12] = macb_or_gem_readl(bp, USRIO);
> + if (!(bp->caps & MACB_CAPS_USRIO_DISABLED))
> + regs_buff[12] = macb_or_gem_readl(bp, USRIO);
> if (macb_is_gem(bp)) {
> regs_buff[13] = gem_readl(bp, DMACFG);
> }
> @@ -2403,19 +2404,21 @@ static int macb_init(struct platform_device *pdev)
> dev->hw_features &= ~NETIF_F_SG;
> dev->features = dev->hw_features;
>
> - val = 0;
> - if (bp->phy_interface == PHY_INTERFACE_MODE_RGMII)
> - val = GEM_BIT(RGMII);
> - else if (bp->phy_interface == PHY_INTERFACE_MODE_RMII &&
> - (bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII))
> - val = MACB_BIT(RMII);
> - else if (!(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII))
> - val = MACB_BIT(MII);
> + if (!(bp->caps & MACB_CAPS_USRIO_DISABLED)) {
> + val = 0;
> + if (bp->phy_interface == PHY_INTERFACE_MODE_RGMII)
> + val = GEM_BIT(RGMII);
> + else if (bp->phy_interface == PHY_INTERFACE_MODE_RMII &&
> + (bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII))
> + val = MACB_BIT(RMII);
> + else if (!(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII))
> + val = MACB_BIT(MII);
>
> - if (bp->caps & MACB_CAPS_USRIO_HAS_CLKEN)
> - val |= MACB_BIT(CLKEN);
> + if (bp->caps & MACB_CAPS_USRIO_HAS_CLKEN)
> + val |= MACB_BIT(CLKEN);
>
> - macb_or_gem_writel(bp, USRIO, val);
> + macb_or_gem_writel(bp, USRIO, val);
> + }
>
> /* Set MII management clock divider */
> val = macb_mdc_clk_div(bp);
> diff --git a/drivers/net/ethernet/cadence/macb.h
> b/drivers/net/ethernet/cadence/macb.h
> index 5c03e81..0d4ecfc 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -400,6 +400,7 @@
> #define MACB_CAPS_USRIO_HAS_CLKEN0x0002
> #define MACB_CAPS_USRIO_DEFAULT_IS_MII 0x0004
> #define MACB_CAPS_NO_GIGABIT_HALF0x0008
> +#define MACB_CAPS_USRIO_DISABLED 0x0010
> #define MACB_CAPS_FIFO_MODE 0x1000
> #define MACB_CAPS_GIGABIT_MODE_AVAILABLE 0x2000
> #define MACB_CAPS_SG_DISABLED0x4000
>
--
Nicolas Ferre
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 net-next 2/3] net: macb: Add NPx macb config using USRIO_DISABLED cap
Le 05/01/2016 13:20, Neil Armstrong a écrit :
> On 01/04/2016 11:38 AM, Nicolas Ferre wrote:
>> Le 04/01/2016 10:42, Neil Armstrong a écrit :
>>> static const struct macb_config zynqmp_config = {
>>> .caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO,
>>> @@ -2801,6 +2806,7 @@ static const struct of_device_id macb_dt_ids[] = {
>>> { .compatible = "cdns,at32ap7000-macb" },
>>> { .compatible = "cdns,at91sam9260-macb", .data = _config },
>>> { .compatible = "cdns,macb" },
>>> + { .compatible = "cdns,npx-macb", .data = _config },
>>
>> I can accept that, but I think that you'd better make your device tree
>> compatibility string *not* generic. Name it by the first NPx SoC or
>> perfectly compatible SoC family that has this configuration and you'll
>> be able to make the NP(x+1) compatible with it.
> Well, the first Soc having this configuration is Np4, would cdns,np4-macb be
> ok ?
Yes, absolutely.
Thanks
>> It has proven to be much more future proof and even if in the early days
>> of DT on ARM we accepted some binding with generic strings like this one
>> below, It has proven to be a mistake.
>>
>>> { .compatible = "cdns,gem", .data = _config },
>>> { .compatible = "atmel,sama5d2-gem", .data = _config },
>>>
>>
>>
>
> Neil
>
--
Nicolas Ferre
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 net-next 2/3] net: macb: Add NPx macb config using USRIO_DISABLED cap
Le 04/01/2016 10:42, Neil Armstrong a écrit :
> Declare a new SoC variant for NPx SoCs having USRIO_DISABLED as
> capability bit.
>
> Signed-off-by: Neil Armstrong <narmstr...@baylibre.com>
> ---
> drivers/net/ethernet/cadence/macb.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/net/ethernet/cadence/macb.c
> b/drivers/net/ethernet/cadence/macb.c
> index fa53bc3..a9e27a7 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -2781,6 +2781,11 @@ static const struct macb_config emac_config = {
> .init = at91ether_init,
> };
>
> +static const struct macb_config npx_config = {
> + .caps = MACB_CAPS_USRIO_DISABLED,
> + .clk_init = macb_clk_init,
> + .init = macb_init,
> +};
>
> static const struct macb_config zynqmp_config = {
> .caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO,
> @@ -2801,6 +2806,7 @@ static const struct of_device_id macb_dt_ids[] = {
> { .compatible = "cdns,at32ap7000-macb" },
> { .compatible = "cdns,at91sam9260-macb", .data = _config },
> { .compatible = "cdns,macb" },
> + { .compatible = "cdns,npx-macb", .data = _config },
I can accept that, but I think that you'd better make your device tree
compatibility string *not* generic. Name it by the first NPx SoC or
perfectly compatible SoC family that has this configuration and you'll
be able to make the NP(x+1) compatible with it.
It has proven to be much more future proof and even if in the early days
of DT on ARM we accepted some binding with generic strings like this one
below, It has proven to be a mistake.
> { .compatible = "cdns,gem", .data = _config },
> { .compatible = "atmel,sama5d2-gem", .data = _config },
>
--
Nicolas Ferre
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 net-next] net: ethernet: cadence-macb: Add disabled usrio caps
Le 04/01/2016 10:01, Neil Armstrong a écrit :
> On some platforms, the macb integration does not use the USRIO
> register to configure the (R)MII port and clocks.
> When the register is not implemented and the MACB error signal
> is connected to the bus error, reading or writing to the USRIO
> register can trigger some Imprecise External Aborts on ARM platforms.
>
> Signed-off-by: Neil Armstrong <narmstr...@baylibre.com>
> ---
> drivers/net/ethernet/cadence/macb.c | 27 +++
> drivers/net/ethernet/cadence/macb.h | 1 +
> 2 files changed, 16 insertions(+), 12 deletions(-)
>
> Nicolas,
> I post only the first patch of the previous set posted here :
> http://lkml.kernel.org/r/1449582726-6148-1-git-send-email-narmstr...@baylibre.com
> to hopefully make it into the 4.5 merge time,
> I'll post the vendor prefix once this patch will hit mainline.
Okay, but I don't see how you will activate this capability (or I lost
track of it). So, before I can accept one solution, can you please
repost the whole solution as a v4.
Thanks. Bye,
>
> Regards,
> Neil
>
> diff --git a/drivers/net/ethernet/cadence/macb.c
> b/drivers/net/ethernet/cadence/macb.c
> index 8b45bc9..fa53bc3 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -2124,7 +2124,8 @@ static void macb_get_regs(struct net_device *dev,
> struct ethtool_regs *regs,
> regs_buff[10] = macb_tx_dma(>queues[0], tail);
> regs_buff[11] = macb_tx_dma(>queues[0], head);
>
> - regs_buff[12] = macb_or_gem_readl(bp, USRIO);
> + if (!(bp->caps & MACB_CAPS_USRIO_DISABLED))
> + regs_buff[12] = macb_or_gem_readl(bp, USRIO);
> if (macb_is_gem(bp)) {
> regs_buff[13] = gem_readl(bp, DMACFG);
> }
> @@ -2403,19 +2404,21 @@ static int macb_init(struct platform_device *pdev)
> dev->hw_features &= ~NETIF_F_SG;
> dev->features = dev->hw_features;
>
> - val = 0;
> - if (bp->phy_interface == PHY_INTERFACE_MODE_RGMII)
> - val = GEM_BIT(RGMII);
> - else if (bp->phy_interface == PHY_INTERFACE_MODE_RMII &&
> - (bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII))
> - val = MACB_BIT(RMII);
> - else if (!(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII))
> - val = MACB_BIT(MII);
> + if (!(bp->caps & MACB_CAPS_USRIO_DISABLED)) {
> + val = 0;
> + if (bp->phy_interface == PHY_INTERFACE_MODE_RGMII)
> + val = GEM_BIT(RGMII);
> + else if (bp->phy_interface == PHY_INTERFACE_MODE_RMII &&
> + (bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII))
> + val = MACB_BIT(RMII);
> + else if (!(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII))
> + val = MACB_BIT(MII);
>
> - if (bp->caps & MACB_CAPS_USRIO_HAS_CLKEN)
> - val |= MACB_BIT(CLKEN);
> + if (bp->caps & MACB_CAPS_USRIO_HAS_CLKEN)
> + val |= MACB_BIT(CLKEN);
>
> - macb_or_gem_writel(bp, USRIO, val);
> + macb_or_gem_writel(bp, USRIO, val);
> + }
>
> /* Set MII management clock divider */
> val = macb_mdc_clk_div(bp);
> diff --git a/drivers/net/ethernet/cadence/macb.h
> b/drivers/net/ethernet/cadence/macb.h
> index 5c03e81..0d4ecfc 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -400,6 +400,7 @@
> #define MACB_CAPS_USRIO_HAS_CLKEN0x0002
> #define MACB_CAPS_USRIO_DEFAULT_IS_MII 0x0004
> #define MACB_CAPS_NO_GIGABIT_HALF0x0008
> +#define MACB_CAPS_USRIO_DISABLED 0x0010
> #define MACB_CAPS_FIFO_MODE 0x1000
> #define MACB_CAPS_GIGABIT_MODE_AVAILABLE 0x2000
> #define MACB_CAPS_SG_DISABLED0x4000
>
--
Nicolas Ferre
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net/macb: add proper header file
Le 17/12/2015 13:15, Sudip Mukherjee a écrit :
> mips allmodconfig build fails with the error:
>
> drivers/net/ethernet/cadence/macb.c: In function 'macb_probe':
> drivers/net/ethernet/cadence/macb.c:2908:2: error: implicit declaration of
> function 'devm_gpiod_get_optional' [-Werror=implicit-function-declaration]
> bp->reset_gpio = devm_gpiod_get_optional(>pdev->dev, "phy-reset",
> ^
> drivers/net/ethernet/cadence/macb.c:2909:8: error: 'GPIOD_OUT_HIGH'
> undeclared (first use in this function)
> GPIOD_OUT_HIGH);
> ^
> drivers/net/ethernet/cadence/macb.c:2909:8: note: each undeclared identifier
> is reported only once for each function it appears in
> drivers/net/ethernet/cadence/macb.c: In function 'macb_remove':
> drivers/net/ethernet/cadence/macb.c:2979:3: error: implicit declaration of
> function 'gpiod_set_value' [-Werror=implicit-function-declaration]
>gpiod_set_value(bp->reset_gpio, 0);
>^
>
> Add the proper header file to resolve it.
>
> Fixes: 5833e0526820 ("net/macb: add support for resetting PHY using GPIO")
> Cc: Gregory CLEMENT <gregory.clem...@free-electrons.com>
> Signed-off-by: Sudip Mukherjee <su...@vectorindia.org>
This one is fixed and handled by Gregory here:
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-December/394433.html
Thanks a lot anyay, bye,
> ---
>
> build log with next-20151217 is at:
> https://travis-ci.org/sudipm-mukherjee/parport/jobs/97388463
>
> drivers/net/ethernet/cadence/macb.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/ethernet/cadence/macb.c
> b/drivers/net/ethernet/cadence/macb.c
> index 0123646..988ee14 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -19,6 +19,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
>
--
Nicolas Ferre
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net 1/2] net: cadence: macb: Disable USRIO register on some platforms
Le 08/12/2015 10:21, Neil Armstrong a écrit : > Hi Josh, > > 2015-12-07 20:32 GMT+01:00 Josh Cartwright <jo...@ni.com>: >> On Mon, Dec 07, 2015 at 11:58:33AM +0100, Neil Armstrong wrote: >>> On some platforms, the macb integration does not use the USRIO >>> register to configure the (R)MII port and clocks. >>> When the register is not implemented and the MACB error signal >>> is connected to the bus error, reading or writing to the USRIO >>> register can trigger some Imprecise External Aborts on ARM platforms. >>> --- >> >> Does this make sense to even be a separate bool device tree property? >> >> This sort of configuration is typically done by: >>1. Creating a new 'caps' bit; relevant codepaths check that bit >>2. Creating a new "compatible" string for your platform's macb >> instance >>3. Creating a new 'struct macb_config' instance for your platform, >> setting any relevant caps bits when it is selected. >> >> Josh > > I see the point, but according to the User Guide : >> User I/O Register >> The MACB design provides up to 16 inputs and 16 outputs, >> for which the state of the I/O may >> be read or set under the control of the processor interface. >> If the user I/O is disabled as a configuration option, this address space is >> defined >> as reserved, and hence will be a read-only register of value 0x0. > > On the design I worked on, the macb_user_* signals were commented, > thus disabling this register. > > The implementation is not mandatory, and the "generic" macb compatible > "cdns,macb" should disable > usage of USRIO register by default and be only used for platform > specific macb instances... > > Is it OK if I add a new 'caps' bit and use it for the "generic" macb instance > ? I would say no as some platform may already use this compatibility string. So If you need a different capability set, please create a new compatible string and use this one. > For the device tree property, it should be safe to have the generic > instances of macb and gem to > rely on these properties instead of hardcoded instances. > (it's the biggest aim of device tree, no ? no more hardcoded 'caps' bit ?) > The "no-usrio" and other should eventually map 'caps' bits along the > generic instances. It has been decided a log time ago to use these capabilities and to have mixed approach to the actual configuration of the IP: - from the compatibility string - from the configuration registers. I may be sometimes challenging to figure out where the final property comes from but it has proven to be pretty well adapted to any kind of situation. So let's continue with this and not insert additional properties to this binding. Best regards, -- Nicolas Ferre -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 net-next 2/3] net: ethernet: cadence-macb: Add fallback to read DT provided caps
Le 08/12/2015 16:00, Arnd Bergmann a écrit : > On Tuesday 08 December 2015 14:52:05 Neil Armstrong wrote: >> Add 1:1 mapping of software defines caps parsing from DT in case the >> generic macb compatible form is used. >> These properties will provide support for futures implementations >> only defined from DT without need to update the driver code to support >> new variants. >> >> Signed-off-by: Neil Armstrong <narmstr...@baylibre.com> >> > > Translating the Linux implementation specific configuration into > DT properties directly is usually not the best way. > > Could we instead have a lookup table by compatible string to set the > flags? It seems that there are lots of different flags but only a > couple of different users of this IP block. Also, the fact that > you are now adding yet another quirk tells me that the set you > define today is unlikely to cover all the future requirements. This is basically what I told Neil in my previous email. I understand you point Neil, but I don't find it makes sense and Arnd described it better that I did. So please find a proper compatibility string and simply use it. What about: "cdns,the_name_of_the_product_that_first_implemented_this_no_usrio_special_case-gem"? Bye, -- Nicolas Ferre -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 0/6] net/macb: fix for AVR32 and clean up
Le 27/07/2015 10:10, David Miller a écrit : From: Andy Shevchenko andriy.shevche...@linux.intel.com Date: Fri, 24 Jul 2015 21:23:58 +0300 It seems no one had tested recently the driver on AVR32 platforms such as ATNGW100. This series bring it back to work. Series applied, thanks. David, I would have liked to have more time to go through this series: can you hold on a little bit? Bye, -- Nicolas Ferre -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [NET-NEXT PATCH] net: macb: Change capability mask for jumbo support
Le 23/07/2015 12:14, Harini Katakam a écrit : JUMBO and NO_GIGABIT_HALF have the same capability masks. Change one of them. Signed-off-by: Harini Katakam hari...@xilinx.com Yes, indeed: Acked-by: Nicolas Ferre nicolas.fe...@atmel.com --- drivers/net/ethernet/cadence/macb.h |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h index d746559..8fb80b2 100644 --- a/drivers/net/ethernet/cadence/macb.h +++ b/drivers/net/ethernet/cadence/macb.h @@ -399,7 +399,7 @@ #define MACB_CAPS_GIGABIT_MODE_AVAILABLE 0x2000 #define MACB_CAPS_SG_DISABLED0x4000 #define MACB_CAPS_MACB_IS_GEM0x8000 -#define MACB_CAPS_JUMBO 0x0008 +#define MACB_CAPS_JUMBO 0x0010 /* Bit manipulation macros */ #define MACB_BIT(name) \ -- Nicolas Ferre -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net: macb: Add SG support for Zynq SOC family
Le 06/07/2015 06:32, Punnaiah Choudary Kalluri a écrit :
Enable SG support for Zynq SOC family devices.
Signed-off-by: Punnaiah Choudary Kalluri punn...@xilinx.com
Acked-by: Nicolas Ferre nicolas.fe...@atmel.com
---
drivers/net/ethernet/cadence/macb.c |6 ++
1 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/cadence/macb.c
b/drivers/net/ethernet/cadence/macb.c
index caeb395..a4e3f86 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -2741,8 +2741,7 @@ static const struct macb_config emac_config = {
static const struct macb_config zynqmp_config = {
- .caps = MACB_CAPS_SG_DISABLED | MACB_CAPS_GIGABIT_MODE_AVAILABLE |
- MACB_CAPS_JUMBO,
+ .caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO,
.dma_burst_length = 16,
.clk_init = macb_clk_init,
.init = macb_init,
@@ -2750,8 +2749,7 @@ static const struct macb_config zynqmp_config = {
};
static const struct macb_config zynq_config = {
- .caps = MACB_CAPS_SG_DISABLED | MACB_CAPS_GIGABIT_MODE_AVAILABLE |
- MACB_CAPS_NO_GIGABIT_HALF,
+ .caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_NO_GIGABIT_HALF,
.dma_burst_length = 16,
.clk_init = macb_clk_init,
.init = macb_init,
--
Nicolas Ferre
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: macb: zynq: why is SG disabled?
Le 02/07/2015 04:33, Punnaiah Choudary Kalluri a écrit : Hi Nicolae and Cyrille, SG feature was not tested for Zynq using macb driver but tested it using the emacps Driver in Xilinx tree (this driver is deprecated recently) We will test and enable this feature in driver for Zynq. Ok, fine: we will be glad to merge patches that would enable this feature on Zynq. Bye, -Original Message- From: Cyrille Pitchen [mailto:cyrille.pitc...@atmel.com] Sent: Wednesday, July 01, 2015 10:34 PM To: Nicolae Rosia; Michal Simek; Punnaiah Choudary Kalluri; netdev@vger.kernel.org; Nicolas Ferre; linux-arm-ker...@lists.infradead.org Subject: Re: macb: zynq: why is SG disabled? Le 01/07/2015 17:14, Nicolae Rosia a écrit : Hello, After reading the GEM part of Zynq7000 Technical Reference Manual [0], I think that SG should be supported. Is there a reason why SG is disabled in macb for Zynq? Best regards, Nicolae Rosia Hi Nicolae, when the scatter-gather patch was introduced, the feature was enabled only on tested boards to avoid regressions on other boards. So SG is enabled on sama5d4x and sama5d2x SoCs. SG is disabled on purpose on sama5d3x. For Zynq, I think the feature is still disabled just because it has never been tested. Best regards, Cyrille -- Nicolas Ferre -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next] net: macb: replace literal constant with NET_IP_ALIGN
Le 30/06/2015 19:25, Nicolae Rosia a écrit :
Signed-off-by: Nicolae Rosia nicolae.ro...@certsign.ro
Acked-by: Nicolas Ferre nicolas.fe...@atmel.com
Thanks, bye.
---
drivers/net/ethernet/cadence/macb.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/cadence/macb.c
b/drivers/net/ethernet/cadence/macb.c
index caeb395..dbb5160 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -2554,9 +2554,9 @@ static void at91ether_rx(struct net_device *dev)
while (lp-rx_ring[lp-rx_tail].addr MACB_BIT(RX_USED)) {
p_recv = lp-rx_buffers + lp-rx_tail * AT91ETHER_MAX_RBUFF_SZ;
pktlen = MACB_BF(RX_FRMLEN, lp-rx_ring[lp-rx_tail].ctrl);
- skb = netdev_alloc_skb(dev, pktlen + 2);
+ skb = netdev_alloc_skb(dev, pktlen + NET_IP_ALIGN);
if (skb) {
- skb_reserve(skb, 2);
+ skb_reserve(skb, NET_IP_ALIGN);
memcpy(skb_put(skb, pktlen), p_recv, pktlen);
skb-protocol = eth_type_trans(skb, dev);
--
Nicolas Ferre
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/4] net/macb: bindings doc/trivial: fix sama5d4 comment
On sama5d4, we only have a GEM IP that is configured to do 10/100 Mbits. So the use of Gigabit can be confusing. Signed-off-by: Nicolas Ferre nicolas.fe...@atmel.com --- Documentation/devicetree/bindings/net/macb.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/net/macb.txt b/Documentation/devicetree/bindings/net/macb.txt index 0ae6974383d7..97349e3f3ff2 100644 --- a/Documentation/devicetree/bindings/net/macb.txt +++ b/Documentation/devicetree/bindings/net/macb.txt @@ -8,7 +8,7 @@ Required properties: Use cdns,pc302-gem for Picochip picoXcell pc302 and later devices based on the Cadence GEM, or the generic form: cdns,gem. Use atmel,sama5d3-gem for the Gigabit IP available on Atmel sama5d3 SoCs. - Use atmel,sama5d4-gem for the Gigabit IP available on Atmel sama5d4 SoCs. + Use atmel,sama5d4-gem for the GEM IP (10/100) available on Atmel sama5d4 SoCs. Use cdns,zynqmp-gem for Zynq Ultrascale+ MPSoC. - reg: Address and length of the register set for the device - interrupts: Should contain macb interrupt -- 2.1.3 -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 4/4] net/macb: add config for Atmel sama5d2 SoCs
From: Cyrille Pitchen cyrille.pitc...@atmel.com
Add the compatible string for Atmel sama5d2 SoC family as the configuration
options differ from other instances of the GEM.
Signed-off-by: Cyrille Pitchen cyrille.pitc...@atmel.com
Signed-off-by: Nicolas Ferre nicolas.fe...@atmel.com
---
drivers/net/ethernet/cadence/macb.c | 8
1 file changed, 8 insertions(+)
diff --git a/drivers/net/ethernet/cadence/macb.c
b/drivers/net/ethernet/cadence/macb.c
index 740d04fd2223..caeb39561567 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -2713,6 +2713,13 @@ static const struct macb_config pc302gem_config = {
.init = macb_init,
};
+static const struct macb_config sama5d2_config = {
+ .caps = 0,
+ .dma_burst_length = 16,
+ .clk_init = macb_clk_init,
+ .init = macb_init,
+};
+
static const struct macb_config sama5d3_config = {
.caps = MACB_CAPS_SG_DISABLED | MACB_CAPS_GIGABIT_MODE_AVAILABLE,
.dma_burst_length = 16,
@@ -2756,6 +2763,7 @@ static const struct of_device_id macb_dt_ids[] = {
{ .compatible = cdns,macb },
{ .compatible = cdns,pc302-gem, .data = pc302gem_config },
{ .compatible = cdns,gem, .data = pc302gem_config },
+ { .compatible = atmel,sama5d2-gem, .data = sama5d2_config },
{ .compatible = atmel,sama5d3-gem, .data = sama5d3_config },
{ .compatible = atmel,sama5d4-gem, .data = sama5d4_config },
{ .compatible = cdns,at91rm9200-emac, .data = emac_config },
--
2.1.3
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/4] net/macb: bindings doc: add sama5d2 compatibility sting
Add sama5d2 to the biding documentation for this use of the GEM IP. Signed-off-by: Nicolas Ferre nicolas.fe...@atmel.com --- Documentation/devicetree/bindings/net/macb.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/devicetree/bindings/net/macb.txt b/Documentation/devicetree/bindings/net/macb.txt index 97349e3f3ff2..b5d79761ac97 100644 --- a/Documentation/devicetree/bindings/net/macb.txt +++ b/Documentation/devicetree/bindings/net/macb.txt @@ -7,6 +7,7 @@ Required properties: Use cdns,at32ap7000-macb for other 10/100 usage or use the generic form: cdns,macb. Use cdns,pc302-gem for Picochip picoXcell pc302 and later devices based on the Cadence GEM, or the generic form: cdns,gem. + Use atmel,sama5d2-gem for the GEM IP (10/100) available on Atmel sama5d2 SoCs. Use atmel,sama5d3-gem for the Gigabit IP available on Atmel sama5d3 SoCs. Use atmel,sama5d4-gem for the GEM IP (10/100) available on Atmel sama5d4 SoCs. Use cdns,zynqmp-gem for Zynq Ultrascale+ MPSoC. -- 2.1.3 -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/4] net/macb: bindings doc: fix compatibility string
In the driver and the DT bindings we use the atmel prefix. Fix it in the binding documentation. Signed-off-by: Nicolas Ferre nicolas.fe...@atmel.com --- Documentation/devicetree/bindings/net/macb.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/net/macb.txt b/Documentation/devicetree/bindings/net/macb.txt index 8ec5fdf444e9..0ae6974383d7 100644 --- a/Documentation/devicetree/bindings/net/macb.txt +++ b/Documentation/devicetree/bindings/net/macb.txt @@ -7,8 +7,8 @@ Required properties: Use cdns,at32ap7000-macb for other 10/100 usage or use the generic form: cdns,macb. Use cdns,pc302-gem for Picochip picoXcell pc302 and later devices based on the Cadence GEM, or the generic form: cdns,gem. - Use cdns,sama5d3-gem for the Gigabit IP available on Atmel sama5d3 SoCs. - Use cdns,sama5d4-gem for the Gigabit IP available on Atmel sama5d4 SoCs. + Use atmel,sama5d3-gem for the Gigabit IP available on Atmel sama5d3 SoCs. + Use atmel,sama5d4-gem for the Gigabit IP available on Atmel sama5d4 SoCs. Use cdns,zynqmp-gem for Zynq Ultrascale+ MPSoC. - reg: Address and length of the register set for the device - interrupts: Should contain macb interrupt -- 2.1.3 -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/4] net/macb: add sama5d2 support
Hi, This series is basically the support for another flavor of the GEM IP configuration. It ended up being a series because of some little fixes made to the binding documentation before adding the new compatibility string. Bye, v2: - fix bindings - add sama5d2 compatibility string to the binding documentation Cyrille Pitchen (1): net/macb: add config for Atmel sama5d2 SoCs Nicolas Ferre (3): net/macb: bindings doc: fix compatibility string net/macb: bindings doc/trivial: fix sama5d4 comment net/macb: bindings doc: add sama5d2 compatibility sting Documentation/devicetree/bindings/net/macb.txt | 5 +++-- drivers/net/ethernet/cadence/macb.c| 8 2 files changed, 11 insertions(+), 2 deletions(-) -- 2.1.3 -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net/macb: add config for Atmel sama5d2 SoCs
Le 18/06/2015 15:30, Alexandre Belloni a écrit :
On 18/06/2015 at 12:18:19 +0200, Nicolas Ferre wrote :
From: Cyrille Pitchen cyrille.pitc...@atmel.com
Add the compatible string for Atmel sama5d2 SoC family as the configuration
options differ from other instances of the GEM.
Signed-off-by: Cyrille Pitchen cyrille.pitc...@atmel.com
Signed-off-by: Nicolas Ferre nicolas.fe...@atmel.com
---
drivers/net/ethernet/cadence/macb.c | 8
1 file changed, 8 insertions(+)
diff --git a/drivers/net/ethernet/cadence/macb.c
b/drivers/net/ethernet/cadence/macb.c
index 740d04fd2223..caeb39561567 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -2713,6 +2713,13 @@ static const struct macb_config pc302gem_config = {
.init = macb_init,
};
+static const struct macb_config sama5d2_config = {
+.caps = 0,
+.dma_burst_length = 16,
+.clk_init = macb_clk_init,
+.init = macb_init,
+};
+
static const struct macb_config sama5d3_config = {
.caps = MACB_CAPS_SG_DISABLED | MACB_CAPS_GIGABIT_MODE_AVAILABLE,
.dma_burst_length = 16,
@@ -2756,6 +2763,7 @@ static const struct of_device_id macb_dt_ids[] = {
{ .compatible = cdns,macb },
{ .compatible = cdns,pc302-gem, .data = pc302gem_config },
{ .compatible = cdns,gem, .data = pc302gem_config },
+{ .compatible = atmel,sama5d2-gem, .data = sama5d2_config },
This compatible has to be documented
Sure, I re-send a series right now (and add some documentation fixes).
Thanks, bye,
{ .compatible = atmel,sama5d3-gem, .data = sama5d3_config },
{ .compatible = atmel,sama5d4-gem, .data = sama5d4_config },
{ .compatible = cdns,at91rm9200-emac, .data = emac_config },
--
2.1.3
--
Nicolas Ferre
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] net/macb: add config for Atmel sama5d2 SoCs
From: Cyrille Pitchen cyrille.pitc...@atmel.com
Add the compatible string for Atmel sama5d2 SoC family as the configuration
options differ from other instances of the GEM.
Signed-off-by: Cyrille Pitchen cyrille.pitc...@atmel.com
Signed-off-by: Nicolas Ferre nicolas.fe...@atmel.com
---
drivers/net/ethernet/cadence/macb.c | 8
1 file changed, 8 insertions(+)
diff --git a/drivers/net/ethernet/cadence/macb.c
b/drivers/net/ethernet/cadence/macb.c
index 740d04fd2223..caeb39561567 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -2713,6 +2713,13 @@ static const struct macb_config pc302gem_config = {
.init = macb_init,
};
+static const struct macb_config sama5d2_config = {
+ .caps = 0,
+ .dma_burst_length = 16,
+ .clk_init = macb_clk_init,
+ .init = macb_init,
+};
+
static const struct macb_config sama5d3_config = {
.caps = MACB_CAPS_SG_DISABLED | MACB_CAPS_GIGABIT_MODE_AVAILABLE,
.dma_burst_length = 16,
@@ -2756,6 +2763,7 @@ static const struct of_device_id macb_dt_ids[] = {
{ .compatible = cdns,macb },
{ .compatible = cdns,pc302-gem, .data = pc302gem_config },
{ .compatible = cdns,gem, .data = pc302gem_config },
+ { .compatible = atmel,sama5d2-gem, .data = sama5d2_config },
{ .compatible = atmel,sama5d3-gem, .data = sama5d3_config },
{ .compatible = atmel,sama5d4-gem, .data = sama5d4_config },
{ .compatible = cdns,at91rm9200-emac, .data = emac_config },
--
2.1.3
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] net: macb: Document zynq gem dt binding
Le 22/05/2015 16:22, Nathan Sullivan a écrit :
Signed-off-by: Nathan Sullivan nathan.sulli...@ni.com
Acked-by: Nicolas Ferre nicolas.fe...@atmel.com
---
Documentation/devicetree/bindings/net/cdns-emac.txt |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/net/cdns-emac.txt
b/Documentation/devicetree/bindings/net/cdns-emac.txt
index abd67c1..4451ee9 100644
--- a/Documentation/devicetree/bindings/net/cdns-emac.txt
+++ b/Documentation/devicetree/bindings/net/cdns-emac.txt
@@ -3,7 +3,8 @@
Required properties:
- compatible: Should be cdns,[chip-]{emac}
Use cdns,at91rm9200-emac Atmel at91rm9200 SoC.
- or the generic form: cdns,emac.
+ Use cdns,zynq-gem Xilinx Zynq-7xxx SoC.
+ Or the generic form: cdns,emac.
- reg: Address and length of the register set for the device
- interrupts: Should contain macb interrupt
- phy-mode: see ethernet.txt file in the same directory.
--
Nicolas Ferre
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/3] net: macb: Disable half duplex gigabit on Zynq
Le 22/05/2015 16:22, Nathan Sullivan a écrit :
According to the Zynq TRM, gigabit half duplex is not supported. Add a
new cap and compatible string so Zynq can avoid advertising that mode.
Signed-off-by: Nathan Sullivan nathan.sulli...@ni.com
Acked-by: Nicolas Ferre nicolas.fe...@atmel.com
---
drivers/net/ethernet/cadence/macb.c | 12
drivers/net/ethernet/cadence/macb.h |1 +
2 files changed, 13 insertions(+)
diff --git a/drivers/net/ethernet/cadence/macb.c
b/drivers/net/ethernet/cadence/macb.c
index 61aa570..e7c0ef6 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -350,6 +350,9 @@ static int macb_mii_probe(struct net_device *dev)
else
phydev-supported = PHY_BASIC_FEATURES;
+ if (bp-caps MACB_CAPS_NO_GIGABIT_HALF)
+ phydev-supported = ~SUPPORTED_1000baseT_Half;
+
phydev-advertising = phydev-supported;
bp-link = 0;
@@ -2693,6 +2696,14 @@ static const struct macb_config emac_config = {
.init = at91ether_init,
};
+static const struct macb_config zynq_config = {
+ .caps = MACB_CAPS_SG_DISABLED | MACB_CAPS_GIGABIT_MODE_AVAILABLE |
+ MACB_CAPS_NO_GIGABIT_HALF,
+ .dma_burst_length = 16,
+ .clk_init = macb_clk_init,
+ .init = macb_init,
+};
+
static const struct of_device_id macb_dt_ids[] = {
{ .compatible = cdns,at32ap7000-macb },
{ .compatible = cdns,at91sam9260-macb, .data = at91sam9260_config },
@@ -2703,6 +2714,7 @@ static const struct of_device_id macb_dt_ids[] = {
{ .compatible = atmel,sama5d4-gem, .data = sama5d4_config },
{ .compatible = cdns,at91rm9200-emac, .data = emac_config },
{ .compatible = cdns,emac, .data = emac_config },
+ { .compatible = cdns,zynq-gem, .data = zynq_config },
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, macb_dt_ids);
diff --git a/drivers/net/ethernet/cadence/macb.h
b/drivers/net/ethernet/cadence/macb.h
index eb7d76f..24b1d9b 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -393,6 +393,7 @@
#define MACB_CAPS_ISR_CLEAR_ON_WRITE 0x0001
#define MACB_CAPS_USRIO_HAS_CLKEN0x0002
#define MACB_CAPS_USRIO_DEFAULT_IS_MII 0x0004
+#define MACB_CAPS_NO_GIGABIT_HALF0x0008
#define MACB_CAPS_FIFO_MODE 0x1000
#define MACB_CAPS_GIGABIT_MODE_AVAILABLE 0x2000
#define MACB_CAPS_SG_DISABLED0x4000
--
Nicolas Ferre
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
