Re: [PATCH 2/5] net: MOXA ART: connect to PHY and add ethtool support

2013-11-22 Thread Ben Hutchings 
On Fri, 2013-11-22 at 15:57 +0100, Jonas Jensen wrote:
> The kernel now has a MDIO bus driver and a phy_driver (RTL8201CP),
> connect to this PHY using OF and add ethtool support.
> 
> Signed-off-by: Jonas Jensen 
> ---
> 
> Notes:
> Applies to next-20131122
> 
>  drivers/net/ethernet/moxa/moxart_ether.c | 179 
> ++-
>  drivers/net/ethernet/moxa/moxart_ether.h |   1 +
>  2 files changed, 179 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/moxa/moxart_ether.c 
> b/drivers/net/ethernet/moxa/moxart_ether.c
> index 3c14afd..bcc6005 100644
> --- a/drivers/net/ethernet/moxa/moxart_ether.c
> +++ b/drivers/net/ethernet/moxa/moxart_ether.c
> @@ -26,9 +26,15 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>  
>  #include "moxart_ether.h"
>  
> +#define DRV_NAME"moxart-ethernet"
> +#define DRV_VERSION "0.2"
> +#define MOXART_NUM_STATS16

MOXART_NUM_STATS should be defined as ARRAY_SIZE(ethtool_stats_keys).

[...]
> +static void moxart_get_drvinfo(struct net_device *ndev,
> +struct ethtool_drvinfo *info)
> +{
> + strlcpy(info->driver, DRV_NAME, sizeof(info->driver));
> + strlcpy(info->version, DRV_VERSION, sizeof(info->version));
> + strlcpy(info->bus_info, dev_name(>dev), sizeof(info->bus_info));
> + info->n_stats = MOXART_NUM_STATS;
[...]

Don't initialise n_stats here; the core will initialise it using your
get_sset_count implementation.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/5] net: MOXA ART: connect to PHY and add ethtool support

2013-11-22 Thread Florian Fainelli 
Hello Jonas,

> +   if (!priv->phy_dev)
> +   return -ENODEV;

This should not be required since you will fail probing the interface
if you cannot connect to the PHY device.

> +
> +   return phy_ethtool_gset(priv->phy_dev, cmd);
> +}
> +
> +static int moxart_set_settings(struct net_device *ndev, struct ethtool_cmd 
> *cmd)
> +{
> +   struct moxart_mac_priv_t *priv = netdev_priv(ndev);
> +
> +   if (!priv->phy_dev)
> +   return -ENODEV;

Same here

> +
> +   return phy_ethtool_sset(priv->phy_dev, cmd);
> +}
> +
> +static int moxart_nway_reset(struct net_device *ndev)
> +{
> +   struct moxart_mac_priv_t *priv = netdev_priv(ndev);
> +
> +   if (!priv->phy_dev)
> +   return -EINVAL;

And here

[snip]

> +   if (!priv->phy_dev)
> +   return -ENODEV;

And here

> +
> +   return phy_mii_ioctl(priv->phy_dev, ir, cmd);
> +}
> +
>  static void moxart_mac_free_memory(struct net_device *ndev)
>  {
> struct moxart_mac_priv_t *priv = netdev_priv(ndev);
> @@ -169,6 +314,10 @@ static int moxart_mac_open(struct net_device *ndev)
> moxart_update_mac_address(ndev);
> moxart_mac_setup_desc_ring(ndev);
> moxart_mac_enable(ndev);
> +
> +   if (priv->phy_dev)
> +   phy_start(priv->phy_dev);
> +
> netif_start_queue(ndev);
>
> netdev_dbg(ndev, "%s: IMR=0x%x, MACCR=0x%x\n",
> @@ -184,6 +333,9 @@ static int moxart_mac_stop(struct net_device *ndev)
>
> napi_disable(>napi);
>
> +   if (priv->phy_dev)
> +   phy_stop(priv->phy_dev);
> +
> netif_stop_queue(ndev);
>
> /* disable all interrupts */
> @@ -429,12 +581,22 @@ static struct net_device_ops moxart_netdev_ops = {
> .ndo_set_mac_address= moxart_set_mac_address,
> .ndo_validate_addr  = eth_validate_addr,
> .ndo_change_mtu = eth_change_mtu,
> +   .ndo_do_ioctl   = moxart_do_ioctl,
>  };
>
> +static void moxart_adjust_link(struct net_device *ndev)
> +{
> +#ifdef DEBUG
> +   struct moxart_mac_priv_t *priv = netdev_priv(ndev);
> +
> +   phy_print_status(priv->phy_dev);
> +#endif

This is too simplistic, you should at least do the following:

- check for an update in the PHY duplex setting and update the
Ethernet MAC with this new setting
- check for an update in the PHY speed settings and update relevant
MAC registers (RX/TX clock speed, PHY speed etc...)

Also, it is a good practice to retain the previous link
state/duplex/speed, compare them against the phydev values and call
phy_print_status() if there is any change.

> +}
> +
>  static int moxart_mac_probe(struct platform_device *pdev)
>  {
> struct device *p_dev = >dev;
> -   struct device_node *node = p_dev->of_node;
> +   struct device_node *node = p_dev->of_node, *phy_node;
> struct net_device *ndev;
> struct moxart_mac_priv_t *priv;
> struct resource *res;
> @@ -455,6 +617,20 @@ static int moxart_mac_probe(struct platform_device *pdev)
> priv = netdev_priv(ndev);
> priv->ndev = ndev;
>
> +   phy_node = of_parse_phandle(node, "phy-handle", 0);
> +   if (!phy_node)
> +   dev_err(p_dev, "of_parse_phandle failed\n");
> +
> +   if (phy_node) {
> +   priv->phy_dev = of_phy_connect(priv->ndev, phy_node,
> +  _adjust_link,
> +  0, PHY_INTERFACE_MODE_MII);

Unless the hardware supports anything but MII, this is fine. A nicer
binding would include a "phy-mode" or "phy-connection-type" property
which describes how the Ethernet MAC and PHY are connected to each
other. You can then retrieve that property using of_get_phy_mode().
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/5] net: MOXA ART: connect to PHY and add ethtool support

2013-11-22 Thread Florian Fainelli 
2013/11/22 Jonas Jensen :
> On 22 November 2013 15:57, Jonas Jensen  wrote:
>>  drivers/net/ethernet/moxa/moxart_ether.c | 179 
>> ++-
>>  drivers/net/ethernet/moxa/moxart_ether.h |   1 +
>>  2 files changed, 179 insertions(+), 1 deletion(-)
>
> I see now I forgot to include the devicetree binding document.
>
> I'll wait for comments.

You are doing two things in your patch, so that means two separate patches.
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/5] net: MOXA ART: connect to PHY and add ethtool support

2013-11-22 Thread Jonas Jensen 
On 22 November 2013 15:57, Jonas Jensen  wrote:
>  drivers/net/ethernet/moxa/moxart_ether.c | 179 
> ++-
>  drivers/net/ethernet/moxa/moxart_ether.h |   1 +
>  2 files changed, 179 insertions(+), 1 deletion(-)

I see now I forgot to include the devicetree binding document.

I'll wait for comments.


Thanks,
Jonas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/5] net: MOXA ART: connect to PHY and add ethtool support

2013-11-22 Thread Jonas Jensen 
On 22 November 2013 15:57, Jonas Jensen jonas.jen...@gmail.com wrote:
  drivers/net/ethernet/moxa/moxart_ether.c | 179 
 ++-
  drivers/net/ethernet/moxa/moxart_ether.h |   1 +
  2 files changed, 179 insertions(+), 1 deletion(-)

I see now I forgot to include the devicetree binding document.

I'll wait for comments.


Thanks,
Jonas
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/5] net: MOXA ART: connect to PHY and add ethtool support

2013-11-22 Thread Florian Fainelli 
2013/11/22 Jonas Jensen jonas.jen...@gmail.com:
 On 22 November 2013 15:57, Jonas Jensen jonas.jen...@gmail.com wrote:
  drivers/net/ethernet/moxa/moxart_ether.c | 179 
 ++-
  drivers/net/ethernet/moxa/moxart_ether.h |   1 +
  2 files changed, 179 insertions(+), 1 deletion(-)

 I see now I forgot to include the devicetree binding document.

 I'll wait for comments.

You are doing two things in your patch, so that means two separate patches.
-- 
Florian
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/5] net: MOXA ART: connect to PHY and add ethtool support

2013-11-22 Thread Florian Fainelli 
Hello Jonas,

 +   if (!priv-phy_dev)
 +   return -ENODEV;

This should not be required since you will fail probing the interface
if you cannot connect to the PHY device.

 +
 +   return phy_ethtool_gset(priv-phy_dev, cmd);
 +}
 +
 +static int moxart_set_settings(struct net_device *ndev, struct ethtool_cmd 
 *cmd)
 +{
 +   struct moxart_mac_priv_t *priv = netdev_priv(ndev);
 +
 +   if (!priv-phy_dev)
 +   return -ENODEV;

Same here

 +
 +   return phy_ethtool_sset(priv-phy_dev, cmd);
 +}
 +
 +static int moxart_nway_reset(struct net_device *ndev)
 +{
 +   struct moxart_mac_priv_t *priv = netdev_priv(ndev);
 +
 +   if (!priv-phy_dev)
 +   return -EINVAL;

And here

[snip]

 +   if (!priv-phy_dev)
 +   return -ENODEV;

And here

 +
 +   return phy_mii_ioctl(priv-phy_dev, ir, cmd);
 +}
 +
  static void moxart_mac_free_memory(struct net_device *ndev)
  {
 struct moxart_mac_priv_t *priv = netdev_priv(ndev);
 @@ -169,6 +314,10 @@ static int moxart_mac_open(struct net_device *ndev)
 moxart_update_mac_address(ndev);
 moxart_mac_setup_desc_ring(ndev);
 moxart_mac_enable(ndev);
 +
 +   if (priv-phy_dev)
 +   phy_start(priv-phy_dev);
 +
 netif_start_queue(ndev);

 netdev_dbg(ndev, %s: IMR=0x%x, MACCR=0x%x\n,
 @@ -184,6 +333,9 @@ static int moxart_mac_stop(struct net_device *ndev)

 napi_disable(priv-napi);

 +   if (priv-phy_dev)
 +   phy_stop(priv-phy_dev);
 +
 netif_stop_queue(ndev);

 /* disable all interrupts */
 @@ -429,12 +581,22 @@ static struct net_device_ops moxart_netdev_ops = {
 .ndo_set_mac_address= moxart_set_mac_address,
 .ndo_validate_addr  = eth_validate_addr,
 .ndo_change_mtu = eth_change_mtu,
 +   .ndo_do_ioctl   = moxart_do_ioctl,
  };

 +static void moxart_adjust_link(struct net_device *ndev)
 +{
 +#ifdef DEBUG
 +   struct moxart_mac_priv_t *priv = netdev_priv(ndev);
 +
 +   phy_print_status(priv-phy_dev);
 +#endif

This is too simplistic, you should at least do the following:

- check for an update in the PHY duplex setting and update the
Ethernet MAC with this new setting
- check for an update in the PHY speed settings and update relevant
MAC registers (RX/TX clock speed, PHY speed etc...)

Also, it is a good practice to retain the previous link
state/duplex/speed, compare them against the phydev values and call
phy_print_status() if there is any change.

 +}
 +
  static int moxart_mac_probe(struct platform_device *pdev)
  {
 struct device *p_dev = pdev-dev;
 -   struct device_node *node = p_dev-of_node;
 +   struct device_node *node = p_dev-of_node, *phy_node;
 struct net_device *ndev;
 struct moxart_mac_priv_t *priv;
 struct resource *res;
 @@ -455,6 +617,20 @@ static int moxart_mac_probe(struct platform_device *pdev)
 priv = netdev_priv(ndev);
 priv-ndev = ndev;

 +   phy_node = of_parse_phandle(node, phy-handle, 0);
 +   if (!phy_node)
 +   dev_err(p_dev, of_parse_phandle failed\n);
 +
 +   if (phy_node) {
 +   priv-phy_dev = of_phy_connect(priv-ndev, phy_node,
 +  moxart_adjust_link,
 +  0, PHY_INTERFACE_MODE_MII);

Unless the hardware supports anything but MII, this is fine. A nicer
binding would include a phy-mode or phy-connection-type property
which describes how the Ethernet MAC and PHY are connected to each
other. You can then retrieve that property using of_get_phy_mode().
-- 
Florian
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/5] net: MOXA ART: connect to PHY and add ethtool support

2013-11-22 Thread Ben Hutchings 
On Fri, 2013-11-22 at 15:57 +0100, Jonas Jensen wrote:
 The kernel now has a MDIO bus driver and a phy_driver (RTL8201CP),
 connect to this PHY using OF and add ethtool support.
 
 Signed-off-by: Jonas Jensen jonas.jen...@gmail.com
 ---
 
 Notes:
 Applies to next-20131122
 
  drivers/net/ethernet/moxa/moxart_ether.c | 179 
 ++-
  drivers/net/ethernet/moxa/moxart_ether.h |   1 +
  2 files changed, 179 insertions(+), 1 deletion(-)
 
 diff --git a/drivers/net/ethernet/moxa/moxart_ether.c 
 b/drivers/net/ethernet/moxa/moxart_ether.c
 index 3c14afd..bcc6005 100644
 --- a/drivers/net/ethernet/moxa/moxart_ether.c
 +++ b/drivers/net/ethernet/moxa/moxart_ether.c
 @@ -26,9 +26,15 @@
  #include linux/of_irq.h
  #include linux/crc32.h
  #include linux/crc32c.h
 +#include linux/phy.h
 +#include linux/of_mdio.h
  
  #include moxart_ether.h
  
 +#define DRV_NAMEmoxart-ethernet
 +#define DRV_VERSION 0.2
 +#define MOXART_NUM_STATS16

MOXART_NUM_STATS should be defined as ARRAY_SIZE(ethtool_stats_keys).

[...]
 +static void moxart_get_drvinfo(struct net_device *ndev,
 +struct ethtool_drvinfo *info)
 +{
 + strlcpy(info-driver, DRV_NAME, sizeof(info-driver));
 + strlcpy(info-version, DRV_VERSION, sizeof(info-version));
 + strlcpy(info-bus_info, dev_name(ndev-dev), sizeof(info-bus_info));
 + info-n_stats = MOXART_NUM_STATS;
[...]

Don't initialise n_stats here; the core will initialise it using your
get_sset_count implementation.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/