am335x: cpsw: phy ignores max-speed setting

am335x: cpsw: phy ignores max-speed setting

[Yegor Yefremov]

I' m trying to override max-speed setting for both CPSW connected
PHYs. This is my DTS section for configuring CPSW:

&mac {
        pinctrl-names = "default", "sleep";
        pinctrl-0 = <&cpsw_default>;
        pinctrl-1 = <&cpsw_sleep>;
        dual_emac = <1>;

        status = "okay";
};

&davinci_mdio {
        pinctrl-names = "default", "sleep";
        pinctrl-0 = <&davinci_mdio_default>;
        pinctrl-1 = <&davinci_mdio_sleep>;

        status = "okay";
};

&cpsw_emac0 {
        phy_id = <&davinci_mdio>, <0>;
        phy-mode = "rgmii-id";
        dual_emac_res_vlan = <1>;
        max-speed = <100>;
};

&cpsw_emac1 {
        phy_id = <&davinci_mdio>, <1>;
        phy-mode = "rgmii-id";
        dual_emac_res_vlan = <2>;
        max-speed = <100>;
};

But in drivers/net/phy/phy_device.c->of_set_phy_supported() routine I
don't get through node check, i.e. node == NULL. Any idea why?

static void of_set_phy_supported(struct phy_device *phydev)
{
        struct device_node *node = phydev->dev.of_node;
        u32 max_speed;

        if (!IS_ENABLED(CONFIG_OF_MDIO))
                return;

        if (!node)
                return;

        if (!of_property_read_u32(node, "max-speed", &max_speed)) {
                /* The default values for phydev->supported are
provided by the PHY
                 * driver "features" member, we want to reset to sane
defaults fist
                 * before supporting higher speeds.
                 */
                phydev->supported &= PHY_DEFAULT_FEATURES;

                switch (max_speed) {
                default:
                        return;

                case SPEED_1000:
                        phydev->supported |= PHY_1000BT_FEATURES;
                case SPEED_100:
                        phydev->supported |= PHY_100BT_FEATURES;
                case SPEED_10:
                        phydev->supported |= PHY_10BT_FEATURES;
                }
        }
}

Yegor

Re: am335x: cpsw: phy ignores max-speed setting

[Dave Taht]

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

ooh! ooh! I have a BQL enablement patch for the cpsw that I have no
means of testing against multiple phys. Could
you give the attached very small patch a shot along the way?

The results I get on the beaglebone vs netperf-wrapper are pretty
spectacular - huge increase in throughput, big reduction in
latency.

http://snapon.lab.bufferbloat.net/~d/beagle_bql/bql_makes_a_difference.png
http://snapon.lab.bufferbloat.net/~d/beagle_bql/beaglebonewithbql.png

On Thu, Nov 6, 2014 at 8:25 AM, Yegor Yefremov
<yegorslists@googlemail.com> wrote:
> I' m trying to override max-speed setting for both CPSW connected
> PHYs. This is my DTS section for configuring CPSW:
>
> &mac {
>         pinctrl-names = "default", "sleep";
>         pinctrl-0 = <&cpsw_default>;
>         pinctrl-1 = <&cpsw_sleep>;
>         dual_emac = <1>;
>
>         status = "okay";
> };
>
> &davinci_mdio {
>         pinctrl-names = "default", "sleep";
>         pinctrl-0 = <&davinci_mdio_default>;
>         pinctrl-1 = <&davinci_mdio_sleep>;
>
>         status = "okay";
> };
>
> &cpsw_emac0 {
>         phy_id = <&davinci_mdio>, <0>;
>         phy-mode = "rgmii-id";
>         dual_emac_res_vlan = <1>;
>         max-speed = <100>;
> };
>
> &cpsw_emac1 {
>         phy_id = <&davinci_mdio>, <1>;
>         phy-mode = "rgmii-id";
>         dual_emac_res_vlan = <2>;
>         max-speed = <100>;
> };
>
> But in drivers/net/phy/phy_device.c->of_set_phy_supported() routine I
> don't get through node check, i.e. node == NULL. Any idea why?
>
> static void of_set_phy_supported(struct phy_device *phydev)
> {
>         struct device_node *node = phydev->dev.of_node;
>         u32 max_speed;
>
>         if (!IS_ENABLED(CONFIG_OF_MDIO))
>                 return;
>
>         if (!node)
>                 return;
>
>         if (!of_property_read_u32(node, "max-speed", &max_speed)) {
>                 /* The default values for phydev->supported are
> provided by the PHY
>                  * driver "features" member, we want to reset to sane
> defaults fist
>                  * before supporting higher speeds.
>                  */
>                 phydev->supported &= PHY_DEFAULT_FEATURES;
>
>                 switch (max_speed) {
>                 default:
>                         return;
>
>                 case SPEED_1000:
>                         phydev->supported |= PHY_1000BT_FEATURES;
>                 case SPEED_100:
>                         phydev->supported |= PHY_100BT_FEATURES;
>                 case SPEED_10:
>                         phydev->supported |= PHY_10BT_FEATURES;
>                 }
>         }
> }
>
> Yegor
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Dave Täht

thttp://www.bufferbloat.net/projects/bloat/wiki/Upcoming_Talks

[-- Attachment #2: 0001-Add-BQL-support-to-the-TI-cpsw-driver.patch --]
[-- Type: text/x-patch, Size: 2369 bytes --]

From 7eccb26dc8f6d09660b22fcbd868572d050df26f Mon Sep 17 00:00:00 2001
From: Dave Taht <dave.taht@bufferbloat.net>
Date: Thu, 6 Nov 2014 08:45:30 -0800
Subject: [PATCH] Add BQL support to the TI cpsw driver

Tested on the beaglebone black.

I get a huge improvement in both throughput and latency.

Latency goes from 60ms worst case with pfifo_fast, and 12ms worst case with
sch_fq to 2.5ms with BQL enabled.

Throughput improved also.
---
 drivers/net/ethernet/ti/cpsw.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index d879448..5934fbc 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -118,7 +118,7 @@ do {								\
 #define CPDMA_TXCP		0x40
 #define CPDMA_RXCP		0x60
 
-#define CPSW_POLL_WEIGHT	64
+#define CPSW_POLL_WEIGHT	16
 #define CPSW_MIN_PACKET_SIZE	60
 #define CPSW_MAX_PACKET_SIZE	(1500 + 14 + 4 + 4)
 
@@ -693,6 +693,7 @@ static void cpsw_tx_handler(void *token, int len, int status)
 	cpts_tx_timestamp(priv->cpts, skb);
 	ndev->stats.tx_packets++;
 	ndev->stats.tx_bytes += len;
+	netdev_completed_queue(ndev,1,len);
 	dev_kfree_skb_any(skb);
 }
 
@@ -1307,6 +1308,8 @@ static int cpsw_ndo_open(struct net_device *ndev)
 		cpsw_set_coalesce(ndev, &coal);
 	}
 
+	netdev_reset_queue(ndev);
+	dev_info(priv->dev, "BQL enabled\n");
 	napi_enable(&priv->napi);
 	cpdma_ctlr_start(priv->dma);
 	cpsw_intr_enable(priv);
@@ -1341,6 +1344,7 @@ static int cpsw_ndo_stop(struct net_device *ndev)
 	netif_stop_queue(priv->ndev);
 	napi_disable(&priv->napi);
 	netif_carrier_off(priv->ndev);
+	netdev_reset_queue(priv->ndev);
 
 	if (cpsw_common_res_usage_state(priv) <= 1) {
 		cpts_unregister(priv->cpts);
@@ -1361,6 +1365,7 @@ static netdev_tx_t cpsw_ndo_start_xmit(struct sk_buff *skb,
 {
 	struct cpsw_priv *priv = netdev_priv(ndev);
 	int ret;
+	int len;
 
 	ndev->trans_start = jiffies;
 
@@ -1375,9 +1380,11 @@ static netdev_tx_t cpsw_ndo_start_xmit(struct sk_buff *skb,
 		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
 
 	skb_tx_timestamp(skb);
-
+	len = max(skb->len, CPSW_MIN_PACKET_SIZE);
+	netdev_sent_queue(ndev,len);
 	ret = cpsw_tx_packet_submit(ndev, priv, skb);
 	if (unlikely(ret != 0)) {
+		netdev_completed_queue(ndev,1,len);
 		cpsw_err(priv, tx_err, "desc submit failed\n");
 		goto fail;
 	}
-- 
1.9.1

Re: am335x: cpsw: phy ignores max-speed setting

[Eric Dumazet]

On Thu, 2014-11-06 at 08:51 -0800, Dave Taht wrote:
> ooh! ooh! I have a BQL enablement patch for the cpsw that I have no
> means of testing against multiple phys. Could
> you give the attached very small patch a shot along the way?
> 
> The results I get on the beaglebone vs netperf-wrapper are pretty
> spectacular - huge increase in throughput, big reduction in
> latency.

Please send this patch inline, so that we can comment, and start a new
thread.

@@ -1375,9 +1380,11 @@ static netdev_tx_t cpsw_ndo_start_xmit(struct sk_buff *skb,
                skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
 
        skb_tx_timestamp(skb);
-
+       len = max(skb->len, CPSW_MIN_PACKET_SIZE);
+       netdev_sent_queue(ndev,len);
        ret = cpsw_tx_packet_submit(ndev, priv, skb);
        if (unlikely(ret != 0)) {
+               netdev_completed_queue(ndev,1,len);
<<you can not do that, its racy with other netdev_completed_queue() calls from TX completion >>
                cpsw_err(priv, tx_err, "desc submit failed\n");
                goto fail;
        }


You need to call netdev_sent_queue(ndev, len); at the correct place,
because we can not 'undo' it.

Re: am335x: cpsw: phy ignores max-speed setting

[Joe Perches]

On Thu, 2014-11-06 at 08:51 -0800, Dave Taht wrote:
> ooh! ooh! I have a BQL enablement patch for the cpsw that I have no
> means of testing against multiple phys. Could
> you give the attached very small patch a shot along the way?

One trivial bit and another possible patch below it

this
+       dev_info(priv->dev, "BQL enabled\n");
might be better as:
+	cpsw_info(priv, link, "BQL enabled\n");

Is this the change that matters most?

-#define CPSW_POLL_WEIGHT       64
+#define CPSW_POLL_WEIGHT       16

If so, maybe this could be limited by a sysctl:

Something like:

 Documentation/sysctl/net.txt | 9 +++++++++
 include/linux/netdevice.h    | 1 +
 net/core/dev.c               | 7 +++++++
 net/core/sysctl_net_core.c   | 7 +++++++
 4 files changed, 24 insertions(+)

diff --git a/Documentation/sysctl/net.txt b/Documentation/sysctl/net.txt
index 04892b8..1fe0ebd 100644
--- a/Documentation/sysctl/net.txt
+++ b/Documentation/sysctl/net.txt
@@ -50,6 +50,15 @@ The maximum number of packets that kernel can handle on a NAPI interrupt,
 it's a Per-CPU variable.
 Default: 64
 
+napi_add_weight_max
+-------------------
+
+Limit the maximum number of packets that a device can register in a
+call to netif_napi_add.  This is disabled by default so the value in the
+specific device call is used, but it may be useful in throughput and
+latency testing.
+Default: 0 (off)
+
 default_qdisc
 --------------
 
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 68fe8a0..31857de 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3380,6 +3380,7 @@ void netdev_stats_to_stats64(struct rtnl_link_stats64 *stats64,
 extern int		netdev_max_backlog;
 extern int		netdev_tstamp_prequeue;
 extern int		weight_p;
+extern int		sysctl_napi_add_weight_max;
 extern int		bpf_jit_enable;
 
 bool netdev_has_upper_dev(struct net_device *dev, struct net_device *upper_dev);
diff --git a/net/core/dev.c b/net/core/dev.c
index c934680..aa9bd8d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3016,6 +3016,7 @@ EXPORT_SYMBOL(netdev_max_backlog);
 int netdev_tstamp_prequeue __read_mostly = 1;
 int netdev_budget __read_mostly = 300;
 int weight_p __read_mostly = 64;            /* old backlog weight */
+int sysctl_napi_add_weight_max __read_mostly = 0; /* disabled by default */
 
 /* Called with irq disabled */
 static inline void ____napi_schedule(struct softnet_data *sd,
@@ -4506,6 +4507,12 @@ void netif_napi_add(struct net_device *dev, struct napi_struct *napi,
 	if (weight > NAPI_POLL_WEIGHT)
 		pr_err_once("netif_napi_add() called with weight %d on device %s\n",
 			    weight, dev->name);
+	if (sysctl_napi_add_weight_max > 0 &&
+	    weight > sysctl_napi_add_weight_max) {
+		pr_notice("netif_napi_add() requested weight %d reduced to sysctl napi_add_weight_max limit %d on device %s\n",
+			  weight, sysctl_napi_add_weight_max, dev->name);
+		weight = sysctl_napi_add_weight_max;
+	}
 	napi->weight = weight;
 	list_add(&napi->dev_list, &dev->napi_list);
 	napi->dev = dev;
diff --git a/net/core/sysctl_net_core.c b/net/core/sysctl_net_core.c
index cf9cd13..c90e524 100644
--- a/net/core/sysctl_net_core.c
+++ b/net/core/sysctl_net_core.c
@@ -257,6 +257,13 @@ static struct ctl_table net_core_table[] = {
 		.proc_handler	= proc_dointvec
 	},
 	{
+		.procname	= "napi_add_weight_max",
+		.data		= &sysctl_napi_add_weight_max,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec
+	},
+	{
 		.procname	= "netdev_max_backlog",
 		.data		= &netdev_max_backlog,
 		.maxlen		= sizeof(int),

Re: am335x: cpsw: phy ignores max-speed setting

[Joe Perches]

On Tue, 2014-11-11 at 10:44 -0800, Dave Taht wrote:
> On Nov 6, 2014 11:19 AM, "Joe Perches" <joe@perches.com> wrote:
[]
> > Is this the change that matters most?
> >
> > -#define CPSW_POLL_WEIGHT       64
> > +#define CPSW_POLL_WEIGHT       16
[]
> > If so, maybe this could be limited by a sysctl:
[]
> I'd like autotuning for the underlying rate somehow....

That might be difficult to achieve with variable rates.

Re: am335x: cpsw: phy ignores max-speed setting

[Florian Fainelli]

On 11/06/2014 08:25 AM, Yegor Yefremov wrote:
> I' m trying to override max-speed setting for both CPSW connected
> PHYs. This is my DTS section for configuring CPSW:
> 
> &mac {
>         pinctrl-names = "default", "sleep";
>         pinctrl-0 = <&cpsw_default>;
>         pinctrl-1 = <&cpsw_sleep>;
>         dual_emac = <1>;
> 
>         status = "okay";
> };
> 
> &davinci_mdio {
>         pinctrl-names = "default", "sleep";
>         pinctrl-0 = <&davinci_mdio_default>;
>         pinctrl-1 = <&davinci_mdio_sleep>;
> 
>         status = "okay";
> };
> 
> &cpsw_emac0 {
>         phy_id = <&davinci_mdio>, <0>;
>         phy-mode = "rgmii-id";
>         dual_emac_res_vlan = <1>;
>         max-speed = <100>;
> };
> 
> &cpsw_emac1 {
>         phy_id = <&davinci_mdio>, <1>;
>         phy-mode = "rgmii-id";
>         dual_emac_res_vlan = <2>;
>         max-speed = <100>;
> };
> 
> But in drivers/net/phy/phy_device.c->of_set_phy_supported() routine I
> don't get through node check, i.e. node == NULL. Any idea why?

Yes, because the 'max-speed' property is placed at the Ethernet MAC node
level, not the PHY node as of_set_phy_supported() expect its.

This driver does not appear to use the standard Ethernet PHY device tree
node, so I am not sure what are your options here.

> 
> static void of_set_phy_supported(struct phy_device *phydev)
> {
>         struct device_node *node = phydev->dev.of_node;
>         u32 max_speed;
> 
>         if (!IS_ENABLED(CONFIG_OF_MDIO))
>                 return;
> 
>         if (!node)
>                 return;
> 
>         if (!of_property_read_u32(node, "max-speed", &max_speed)) {
>                 /* The default values for phydev->supported are
> provided by the PHY
>                  * driver "features" member, we want to reset to sane
> defaults fist
>                  * before supporting higher speeds.
>                  */
>                 phydev->supported &= PHY_DEFAULT_FEATURES;
> 
>                 switch (max_speed) {
>                 default:
>                         return;
> 
>                 case SPEED_1000:
>                         phydev->supported |= PHY_1000BT_FEATURES;
>                 case SPEED_100:
>                         phydev->supported |= PHY_100BT_FEATURES;
>                 case SPEED_10:
>                         phydev->supported |= PHY_10BT_FEATURES;
>                 }
>         }
> }
> 
> Yegor
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: am335x: cpsw: phy ignores max-speed setting

[Yegor Yefremov]

On Thu, Nov 6, 2014 at 5:58 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 11/06/2014 08:25 AM, Yegor Yefremov wrote:
>> I' m trying to override max-speed setting for both CPSW connected
>> PHYs. This is my DTS section for configuring CPSW:
>>
>> &mac {
>>         pinctrl-names = "default", "sleep";
>>         pinctrl-0 = <&cpsw_default>;
>>         pinctrl-1 = <&cpsw_sleep>;
>>         dual_emac = <1>;
>>
>>         status = "okay";
>> };
>>
>> &davinci_mdio {
>>         pinctrl-names = "default", "sleep";
>>         pinctrl-0 = <&davinci_mdio_default>;
>>         pinctrl-1 = <&davinci_mdio_sleep>;
>>
>>         status = "okay";
>> };
>>
>> &cpsw_emac0 {
>>         phy_id = <&davinci_mdio>, <0>;
>>         phy-mode = "rgmii-id";
>>         dual_emac_res_vlan = <1>;
>>         max-speed = <100>;
>> };
>>
>> &cpsw_emac1 {
>>         phy_id = <&davinci_mdio>, <1>;
>>         phy-mode = "rgmii-id";
>>         dual_emac_res_vlan = <2>;
>>         max-speed = <100>;
>> };
>>
>> But in drivers/net/phy/phy_device.c->of_set_phy_supported() routine I
>> don't get through node check, i.e. node == NULL. Any idea why?
>
> Yes, because the 'max-speed' property is placed at the Ethernet MAC node
> level, not the PHY node as of_set_phy_supported() expect its.
>
> This driver does not appear to use the standard Ethernet PHY device tree
> node, so I am not sure what are your options here.

The SoC has a complex structure, i.e. Ethernet controller has a switch
inside with two slaves.

A workaround would be perhaps to handle this option in cpsw driver.

Mugunthan, what do you think about this?

>> static void of_set_phy_supported(struct phy_device *phydev)
>> {
>>         struct device_node *node = phydev->dev.of_node;
>>         u32 max_speed;
>>
>>         if (!IS_ENABLED(CONFIG_OF_MDIO))
>>                 return;
>>
>>         if (!node)
>>                 return;
>>
>>         if (!of_property_read_u32(node, "max-speed", &max_speed)) {
>>                 /* The default values for phydev->supported are
>> provided by the PHY
>>                  * driver "features" member, we want to reset to sane
>> defaults fist
>>                  * before supporting higher speeds.
>>                  */
>>                 phydev->supported &= PHY_DEFAULT_FEATURES;
>>
>>                 switch (max_speed) {
>>                 default:
>>                         return;
>>
>>                 case SPEED_1000:
>>                         phydev->supported |= PHY_1000BT_FEATURES;
>>                 case SPEED_100:
>>                         phydev->supported |= PHY_100BT_FEATURES;
>>                 case SPEED_10:
>>                         phydev->supported |= PHY_10BT_FEATURES;
>>                 }
>>         }
>> }
>>
>> Yegor
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>

Re: am335x: cpsw: phy ignores max-speed setting

[Mugunthan V N]

On Friday 07 November 2014 12:41 PM, Yegor Yefremov wrote:
> On Thu, Nov 6, 2014 at 5:58 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> On 11/06/2014 08:25 AM, Yegor Yefremov wrote:
>>> I' m trying to override max-speed setting for both CPSW connected
>>> PHYs. This is my DTS section for configuring CPSW:
>>>
>>> &mac {
>>>         pinctrl-names = "default", "sleep";
>>>         pinctrl-0 = <&cpsw_default>;
>>>         pinctrl-1 = <&cpsw_sleep>;
>>>         dual_emac = <1>;
>>>
>>>         status = "okay";
>>> };
>>>
>>> &davinci_mdio {
>>>         pinctrl-names = "default", "sleep";
>>>         pinctrl-0 = <&davinci_mdio_default>;
>>>         pinctrl-1 = <&davinci_mdio_sleep>;
>>>
>>>         status = "okay";
>>> };
>>>
>>> &cpsw_emac0 {
>>>         phy_id = <&davinci_mdio>, <0>;
>>>         phy-mode = "rgmii-id";
>>>         dual_emac_res_vlan = <1>;
>>>         max-speed = <100>;
>>> };
>>>
>>> &cpsw_emac1 {
>>>         phy_id = <&davinci_mdio>, <1>;
>>>         phy-mode = "rgmii-id";
>>>         dual_emac_res_vlan = <2>;
>>>         max-speed = <100>;
>>> };
>>>
>>> But in drivers/net/phy/phy_device.c->of_set_phy_supported() routine I
>>> don't get through node check, i.e. node == NULL. Any idea why?
>>
>> Yes, because the 'max-speed' property is placed at the Ethernet MAC node
>> level, not the PHY node as of_set_phy_supported() expect its.
>>
>> This driver does not appear to use the standard Ethernet PHY device tree
>> node, so I am not sure what are your options here.
> 
> The SoC has a complex structure, i.e. Ethernet controller has a switch
> inside with two slaves.
> 
> A workaround would be perhaps to handle this option in cpsw driver.
> 
> Mugunthan, what do you think about this?

Yes, CPSW is different from what linux networking is designed for, it
has two ethernet slave and acts as a Layer 2 switch.

* If you need on a run time basis you can change the supported speed via
ethtool advertise interface
* If needed on boot for nfs then need to think of how it can be achieved
in cpsw driver. Will have a look and update this thread.

Regards
Mugunthan V N

Re: am335x: cpsw: phy ignores max-speed setting

[Yegor Yefremov]

On Fri, Nov 7, 2014 at 5:11 PM, Mugunthan V N <mugunthanvnm@ti.com> wrote:
> On Friday 07 November 2014 12:41 PM, Yegor Yefremov wrote:
>> On Thu, Nov 6, 2014 at 5:58 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>> On 11/06/2014 08:25 AM, Yegor Yefremov wrote:
>>>> I' m trying to override max-speed setting for both CPSW connected
>>>> PHYs. This is my DTS section for configuring CPSW:
>>>>
>>>> &mac {
>>>>         pinctrl-names = "default", "sleep";
>>>>         pinctrl-0 = <&cpsw_default>;
>>>>         pinctrl-1 = <&cpsw_sleep>;
>>>>         dual_emac = <1>;
>>>>
>>>>         status = "okay";
>>>> };
>>>>
>>>> &davinci_mdio {
>>>>         pinctrl-names = "default", "sleep";
>>>>         pinctrl-0 = <&davinci_mdio_default>;
>>>>         pinctrl-1 = <&davinci_mdio_sleep>;
>>>>
>>>>         status = "okay";
>>>> };
>>>>
>>>> &cpsw_emac0 {
>>>>         phy_id = <&davinci_mdio>, <0>;
>>>>         phy-mode = "rgmii-id";
>>>>         dual_emac_res_vlan = <1>;
>>>>         max-speed = <100>;
>>>> };
>>>>
>>>> &cpsw_emac1 {
>>>>         phy_id = <&davinci_mdio>, <1>;
>>>>         phy-mode = "rgmii-id";
>>>>         dual_emac_res_vlan = <2>;
>>>>         max-speed = <100>;
>>>> };
>>>>
>>>> But in drivers/net/phy/phy_device.c->of_set_phy_supported() routine I
>>>> don't get through node check, i.e. node == NULL. Any idea why?
>>>
>>> Yes, because the 'max-speed' property is placed at the Ethernet MAC node
>>> level, not the PHY node as of_set_phy_supported() expect its.
>>>
>>> This driver does not appear to use the standard Ethernet PHY device tree
>>> node, so I am not sure what are your options here.
>>
>> The SoC has a complex structure, i.e. Ethernet controller has a switch
>> inside with two slaves.
>>
>> A workaround would be perhaps to handle this option in cpsw driver.
>>
>> Mugunthan, what do you think about this?
>
> Yes, CPSW is different from what linux networking is designed for, it
> has two ethernet slave and acts as a Layer 2 switch.
>
> * If you need on a run time basis you can change the supported speed via
> ethtool advertise interface
> * If needed on boot for nfs then need to think of how it can be achieved
> in cpsw driver. Will have a look and update this thread.

ethtool is working, but requires a custom script, so DTS solution
would suite better.

Thanks.

Yegor

Re: am335x: cpsw: phy ignores max-speed setting

[Lennart Sorensen]

On Thu, Nov 06, 2014 at 05:25:13PM +0100, Yegor Yefremov wrote:
> I' m trying to override max-speed setting for both CPSW connected
> PHYs. This is my DTS section for configuring CPSW:
> 
> &mac {
>         pinctrl-names = "default", "sleep";
>         pinctrl-0 = <&cpsw_default>;
>         pinctrl-1 = <&cpsw_sleep>;
>         dual_emac = <1>;
> 
>         status = "okay";
> };
> 
> &davinci_mdio {
>         pinctrl-names = "default", "sleep";
>         pinctrl-0 = <&davinci_mdio_default>;
>         pinctrl-1 = <&davinci_mdio_sleep>;
> 
>         status = "okay";
> };
> 
> &cpsw_emac0 {
>         phy_id = <&davinci_mdio>, <0>;
>         phy-mode = "rgmii-id";
>         dual_emac_res_vlan = <1>;
>         max-speed = <100>;
> };
> 
> &cpsw_emac1 {
>         phy_id = <&davinci_mdio>, <1>;
>         phy-mode = "rgmii-id";
>         dual_emac_res_vlan = <2>;
>         max-speed = <100>;
> };
> 
> But in drivers/net/phy/phy_device.c->of_set_phy_supported() routine I
> don't get through node check, i.e. node == NULL. Any idea why?
> 
> static void of_set_phy_supported(struct phy_device *phydev)
> {
>         struct device_node *node = phydev->dev.of_node;
>         u32 max_speed;

Did you try adding a printk here to make sure it is actually called?

>         if (!IS_ENABLED(CONFIG_OF_MDIO))
>                 return;

Do you have CONFIG_OF_MDIO on?  I would think so.

>         if (!node)
>                 return;
> 
>         if (!of_property_read_u32(node, "max-speed", &max_speed)) {
>                 /* The default values for phydev->supported are
> provided by the PHY
>                  * driver "features" member, we want to reset to sane
> defaults fist
>                  * before supporting higher speeds.
>                  */
>                 phydev->supported &= PHY_DEFAULT_FEATURES;
> 
>                 switch (max_speed) {
>                 default:
>                         return;
> 
>                 case SPEED_1000:
>                         phydev->supported |= PHY_1000BT_FEATURES;
>                 case SPEED_100:
>                         phydev->supported |= PHY_100BT_FEATURES;
>                 case SPEED_10:
>                         phydev->supported |= PHY_10BT_FEATURES;
>                 }
>         }
> }

-- 
Len Sorensen