Hello David, Grant, Rob, Chris and everybody,
this series improves ethoc behavior in gigabit environment:
- first patch disables gigabit advertisement in the attached PHY making
possible to use gigabit link without any additional setup;
- second patch adds support to set up MII management bus frequency, adding
new fields to platform data and to OF bindings;
- third patch documents existing and newly added OF bindings.
These changes allow to use KC-705 board with 50MHz xtensa core and OpenCores
10/100 Mbps MAC connected to gigabit network without any additional setup.
Max Filippov (3):
net: ethoc: don't advertise gigabit speed on attached PHY
net: ethoc: set up MII management bus clock
net: ethoc: document OF bindings
.../devicetree/bindings/net/opencores-ethoc.txt | 25 +++++++++++++++++++
drivers/net/ethernet/ethoc.c | 29 ++++++++++++++++++++--
include/net/ethoc.h | 2 ++
3 files changed, 54 insertions(+), 2 deletions(-)
create mode 100644 Documentation/devicetree/bindings/net/opencores-ethoc.txt
--
1.8.1.4
MII management bus clock is derived from the MAC clock by dividing it by
MIIMODER register CLKDIV field value. This value may need to be set up
in case it is undefined or its default value is too high (and
communication with PHY is too slow) or too low (and communication with
PHY is impossible). The value of CLKDIV is not specified directly, but
as a pair of MAC frequency and desired MII management bus frequency, as
these parameters are natural.
Signed-off-by: Max Filippov <[email protected]>
---
drivers/net/ethernet/ethoc.c | 27 +++++++++++++++++++++++++--
include/net/ethoc.h | 2 ++
2 files changed, 27 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c
index 0aa1a05..5831406 100644
--- a/drivers/net/ethernet/ethoc.c
+++ b/drivers/net/ethernet/ethoc.c
@@ -919,6 +919,9 @@ static int ethoc_probe(struct platform_device *pdev)
int num_bd;
int ret = 0;
bool random_mac = false;
+ u32 eth_clkfreq = 0;
+ u32 mii_clkfreq = 0;
+ struct ethoc_platform_data *pdata = dev_get_platdata(&pdev->dev);
/* allocate networking device */
netdev = alloc_etherdev(sizeof(struct ethoc));
@@ -1032,8 +1035,7 @@ static int ethoc_probe(struct platform_device *pdev)
}
/* Allow the platform setup code to pass in a MAC address. */
- if (dev_get_platdata(&pdev->dev)) {
- struct ethoc_platform_data *pdata = dev_get_platdata(&pdev->dev);
+ if (pdata) {
memcpy(netdev->dev_addr, pdata->hwaddr, IFHWADDRLEN);
priv->phy_id = pdata->phy_id;
} else {
@@ -1071,6 +1073,27 @@ static int ethoc_probe(struct platform_device *pdev)
if (random_mac)
netdev->addr_assign_type = NET_ADDR_RANDOM;
+ /* Allow the platform setup code to adjust MII management bus clock. */
+ if (pdata) {
+ eth_clkfreq = pdata->eth_clkfreq;
+ mii_clkfreq = pdata->mii_mgmt_clkfreq;
+ } else {
+ of_property_read_u32(pdev->dev.of_node,
+ "clock-frequency", ð_clkfreq);
+ of_property_read_u32(pdev->dev.of_node,
+ "mii-mgmt-clock-frequency", &mii_clkfreq);
+ }
+ if (eth_clkfreq && mii_clkfreq) {
+ u32 clkdiv = MIIMODER_CLKDIV(eth_clkfreq / mii_clkfreq + 1);
+
+ if (!clkdiv)
+ clkdiv = 2;
+ dev_dbg(&pdev->dev, "setting MII clkdiv to %u\n", clkdiv);
+ ethoc_write(priv, MIIMODER,
+ (ethoc_read(priv, MIIMODER) & MIIMODER_NOPRE) |
+ clkdiv);
+ }
+
/* register MII bus */
priv->mdio = mdiobus_alloc();
if (!priv->mdio) {
diff --git a/include/net/ethoc.h b/include/net/ethoc.h
index 96f3789..b292474 100644
--- a/include/net/ethoc.h
+++ b/include/net/ethoc.h
@@ -16,6 +16,8 @@
struct ethoc_platform_data {
u8 hwaddr[IFHWADDRLEN];
s8 phy_id;
+ u32 eth_clkfreq;
+ u32 mii_mgmt_clkfreq;
};
#endif /* !LINUX_NET_ETHOC_H */
--
1.8.1.4
Signed-off-by: Max Filippov <[email protected]>
---
.../devicetree/bindings/net/opencores-ethoc.txt | 25 ++++++++++++++++++++++
1 file changed, 25 insertions(+)
create mode 100644 Documentation/devicetree/bindings/net/opencores-ethoc.txt
diff --git a/Documentation/devicetree/bindings/net/opencores-ethoc.txt b/Documentation/devicetree/bindings/net/opencores-ethoc.txt
new file mode 100644
index 0000000..f7c6c94
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/opencores-ethoc.txt
@@ -0,0 +1,25 @@
+* OpenCores MAC 10/100 Mbps
+
+Required properties:
+- compatible: Should be "opencores,ethoc".
+- reg: Address and length of the register set for the device and of its
+ descriptor memory.
+- interrupts: Should contain ethoc interrupt.
+
+Optional properties:
+- local-mac-address: 6 bytes, mac address
+- clock-frequency: basic MAC frequency.
+- mii-mgmt-clock-frequency: frequency of MII management bus. Together
+ with clock-frequency determines the value programmed into the CLKDIV
+ field of MIIMODER register.
+
+Examples:
+
+ enet0: ethoc@fd030000 {
+ compatible = "opencores,ethoc";
+ reg = <0xfd030000 0x4000 0xfd800000 0x4000>;
+ interrupts = <1>;
+ local-mac-address = [00 50 c2 13 6f 00];
+ clock-frequency = <50000000>;
+ mii-mgmt-clock-frequency = <5000000>;
+ };
--
1.8.1.4
OpenCores 10/100 Mbps MAC does not support speeds above 100 Mbps, but does
not disable advertisement when PHY supports them. This results in
non-functioning network when the MAC is connected to a gigabit PHY connected
to a gigabit switch.
The fix is to disable gigabit speed advertisement on attached PHY
unconditionally.
Signed-off-by: Max Filippov <[email protected]>
---
drivers/net/ethernet/ethoc.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c
index 4de8cfd..0aa1a05 100644
--- a/drivers/net/ethernet/ethoc.c
+++ b/drivers/net/ethernet/ethoc.c
@@ -712,6 +712,8 @@ static int ethoc_open(struct net_device *dev)
netif_start_queue(dev);
}
+ priv->phy->advertising &= ~(ADVERTISED_1000baseT_Full |
+ ADVERTISED_1000baseT_Half);
phy_start(priv->phy);
napi_enable(&priv->napi);
--
1.8.1.4
On Mon, 2014-01-27 at 07:59 +0400, Max Filippov wrote:
> OpenCores 10/100 Mbps MAC does not support speeds above 100 Mbps, but does
> not disable advertisement when PHY supports them. This results in
> non-functioning network when the MAC is connected to a gigabit PHY connected
> to a gigabit switch.
>
> The fix is to disable gigabit speed advertisement on attached PHY
> unconditionally.
>
> Signed-off-by: Max Filippov <[email protected]>
> ---
> drivers/net/ethernet/ethoc.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c
> index 4de8cfd..0aa1a05 100644
> --- a/drivers/net/ethernet/ethoc.c
> +++ b/drivers/net/ethernet/ethoc.c
> @@ -712,6 +712,8 @@ static int ethoc_open(struct net_device *dev)
> netif_start_queue(dev);
> }
>
> + priv->phy->advertising &= ~(ADVERTISED_1000baseT_Full |
> + ADVERTISED_1000baseT_Half);
> phy_start(priv->phy);
> napi_enable(&priv->napi);
>
This is not sufficient to disable gigabit speeds; the supported mask
should also be limited. And it should be done even before the net
device is registered.
Rather than poking into the phy_device structure directly from this
driver, I think you should add a function in phylib for this purpose.
Ben.
--
Ben Hutchings
If at first you don't succeed, you're doing about average.
On Sun, Jan 26, 2014 at 9:59 PM, Max Filippov <[email protected]> wrote:
> Signed-off-by: Max Filippov <[email protected]>
> ---
> .../devicetree/bindings/net/opencores-ethoc.txt | 25 ++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/opencores-ethoc.txt
>
> diff --git a/Documentation/devicetree/bindings/net/opencores-ethoc.txt b/Documentation/devicetree/bindings/net/opencores-ethoc.txt
> new file mode 100644
> index 0000000..f7c6c94
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/opencores-ethoc.txt
> @@ -0,0 +1,25 @@
> +* OpenCores MAC 10/100 Mbps
> +
> +Required properties:
> +- compatible: Should be "opencores,ethoc".
There are not different versions of IP or is the version probeable?
> +- reg: Address and length of the register set for the device and of its
> + descriptor memory.
> +- interrupts: Should contain ethoc interrupt.
> +
> +Optional properties:
> +- local-mac-address: 6 bytes, mac address
There's a patch in progress to move all the standard network
properties to a common location, so you can remove this.
> +- clock-frequency: basic MAC frequency.
> +- mii-mgmt-clock-frequency: frequency of MII management bus. Together
> + with clock-frequency determines the value programmed into the CLKDIV
> + field of MIIMODER register.
> +
> +Examples:
> +
> + enet0: ethoc@fd030000 {
> + compatible = "opencores,ethoc";
> + reg = <0xfd030000 0x4000 0xfd800000 0x4000>;
> + interrupts = <1>;
> + local-mac-address = [00 50 c2 13 6f 00];
> + clock-frequency = <50000000>;
> + mii-mgmt-clock-frequency = <5000000>;
> + };
> --
> 1.8.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello.
On 27-01-2014 18:10, Rob Herring wrote:
>> Signed-off-by: Max Filippov <[email protected]>
>> ---
>> .../devicetree/bindings/net/opencores-ethoc.txt | 25 ++++++++++++++++++++++
>> 1 file changed, 25 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/net/opencores-ethoc.txt
>> diff --git a/Documentation/devicetree/bindings/net/opencores-ethoc.txt b/Documentation/devicetree/bindings/net/opencores-ethoc.txt
>> new file mode 100644
>> index 0000000..f7c6c94
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/opencores-ethoc.txt
>> @@ -0,0 +1,25 @@
[...]
>> +- reg: Address and length of the register set for the device and of its
>> + descriptor memory.
>> +- interrupts: Should contain ethoc interrupt.
>> +
>> +Optional properties:
>> +- local-mac-address: 6 bytes, mac address
> There's a patch in progress to move all the standard network
> properties to a common location, so you can remove this.
Haven't you asked me to replace the descriptions of common Ethernet props
with the references to the common bindings file? I'd prefer this prop to
remain on its place in this case, just the description replaced.
Unfortunately, my patch will now have to wait till 3.15 (I guess), the same as
these ones.
WBR, Sergei
Hi Rob,
On Mon, Jan 27, 2014 at 6:10 PM, Rob Herring <[email protected]> wrote:
> On Sun, Jan 26, 2014 at 9:59 PM, Max Filippov <[email protected]> wrote:
>> Signed-off-by: Max Filippov <[email protected]>
>> ---
>> .../devicetree/bindings/net/opencores-ethoc.txt | 25 ++++++++++++++++++++++
>> 1 file changed, 25 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/net/opencores-ethoc.txt
>>
>> diff --git a/Documentation/devicetree/bindings/net/opencores-ethoc.txt b/Documentation/devicetree/bindings/net/opencores-ethoc.txt
>> new file mode 100644
>> index 0000000..f7c6c94
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/opencores-ethoc.txt
>> @@ -0,0 +1,25 @@
>> +* OpenCores MAC 10/100 Mbps
>> +
>> +Required properties:
>> +- compatible: Should be "opencores,ethoc".
>
> There are not different versions of IP or is the version probeable?
AFAIK there's single version of this 10/100 MAC.
It doesn't have any identification registers.
>> +- reg: Address and length of the register set for the device and of its
>> + descriptor memory.
>> +- interrupts: Should contain ethoc interrupt.
>> +
>> +Optional properties:
>> +- local-mac-address: 6 bytes, mac address
>
> There's a patch in progress to move all the standard network
> properties to a common location, so you can remove this.
Will do.
>> +- clock-frequency: basic MAC frequency.
>> +- mii-mgmt-clock-frequency: frequency of MII management bus. Together
>> + with clock-frequency determines the value programmed into the CLKDIV
>> + field of MIIMODER register.
>> +
>> +Examples:
>> +
>> + enet0: ethoc@fd030000 {
>> + compatible = "opencores,ethoc";
>> + reg = <0xfd030000 0x4000 0xfd800000 0x4000>;
>> + interrupts = <1>;
>> + local-mac-address = [00 50 c2 13 6f 00];
>> + clock-frequency = <50000000>;
>> + mii-mgmt-clock-frequency = <5000000>;
>> + };
>> --
>> 1.8.1.4
--
Thanks.
-- Max
Hi Ben,
On Mon, Jan 27, 2014 at 2:18 PM, Ben Hutchings <[email protected]> wrote:
> On Mon, 2014-01-27 at 07:59 +0400, Max Filippov wrote:
>> OpenCores 10/100 Mbps MAC does not support speeds above 100 Mbps, but does
>> not disable advertisement when PHY supports them. This results in
>> non-functioning network when the MAC is connected to a gigabit PHY connected
>> to a gigabit switch.
>>
>> The fix is to disable gigabit speed advertisement on attached PHY
>> unconditionally.
>>
>> Signed-off-by: Max Filippov <[email protected]>
>> ---
>> drivers/net/ethernet/ethoc.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c
>> index 4de8cfd..0aa1a05 100644
>> --- a/drivers/net/ethernet/ethoc.c
>> +++ b/drivers/net/ethernet/ethoc.c
>> @@ -712,6 +712,8 @@ static int ethoc_open(struct net_device *dev)
>> netif_start_queue(dev);
>> }
>>
>> + priv->phy->advertising &= ~(ADVERTISED_1000baseT_Full |
>> + ADVERTISED_1000baseT_Half);
>> phy_start(priv->phy);
>> napi_enable(&priv->napi);
>>
>
> This is not sufficient to disable gigabit speeds; the supported mask
> should also be limited. And it should be done even before the net
I tried that, but when I also limit supported mask the phy driver doesn't
touch gigabit advertising register int the genphy_config_advert at all.
That's probably right for ethtool interface, but ethoc doesn't support
ethtool.
> device is registered.
>
> Rather than poking into the phy_device structure directly from this
> driver, I think you should add a function in phylib for this purpose.
Like below?
---8<---
>From 347331f399626ecaa9a8e54252f55e0b6788772f Mon Sep 17 00:00:00 2001
From: Max Filippov <[email protected]>
Date: Mon, 27 Jan 2014 04:01:40 +0400
Subject: [PATCH 1/3] net: ethoc: don't advertise gigabit speed on attached PHY
OpenCores 10/100 Mbps MAC does not support speeds above 100 Mbps, but does
not disable advertisement when PHY supports them. This results in
non-functioning network when the MAC is connected to a gigabit PHY connected
to a gigabit switch.
The fix is to disable gigabit speed advertisement on attached PHY
unconditionally.
Signed-off-by: Max Filippov <[email protected]>
---
drivers/net/ethernet/ethoc.c | 3 +++
include/linux/phy.h | 5 +++++
2 files changed, 8 insertions(+)
diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c
index 4de8cfd..e817d58 100644
--- a/drivers/net/ethernet/ethoc.c
+++ b/drivers/net/ethernet/ethoc.c
@@ -688,6 +688,9 @@ static int ethoc_mdio_probe(struct net_device *dev)
}
priv->phy = phy;
+ phy_update_adv(phy,
+ ~(ADVERTISED_1000baseT_Full |
+ ADVERTISED_1000baseT_Half), 0);
return 0;
}
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 48a4dc3..0282a8d 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -559,6 +559,11 @@ static inline int phy_read_status(struct phy_device *phydev) {
return phydev->drv->read_status(phydev);
}
+static inline void phy_update_adv(struct phy_device *phydev, u32 mask, u32 set)
+{
+ phydev->advertising = (phydev->advertising & mask) | set;
+}
+
int genphy_setup_forced(struct phy_device *phydev);
int genphy_restart_aneg(struct phy_device *phydev);
int genphy_config_aneg(struct phy_device *phydev);
--
1.8.1.4
--
Thanks.
-- Max
2014-01-27 Max Filippov <[email protected]>:
> Hi Ben,
>
> On Mon, Jan 27, 2014 at 2:18 PM, Ben Hutchings <[email protected]> wrote:
>> On Mon, 2014-01-27 at 07:59 +0400, Max Filippov wrote:
>>> OpenCores 10/100 Mbps MAC does not support speeds above 100 Mbps, but does
>>> not disable advertisement when PHY supports them. This results in
>>> non-functioning network when the MAC is connected to a gigabit PHY connected
>>> to a gigabit switch.
>>>
>>> The fix is to disable gigabit speed advertisement on attached PHY
>>> unconditionally.
>>>
>>> Signed-off-by: Max Filippov <[email protected]>
>>> ---
>>> drivers/net/ethernet/ethoc.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c
>>> index 4de8cfd..0aa1a05 100644
>>> --- a/drivers/net/ethernet/ethoc.c
>>> +++ b/drivers/net/ethernet/ethoc.c
>>> @@ -712,6 +712,8 @@ static int ethoc_open(struct net_device *dev)
>>> netif_start_queue(dev);
>>> }
>>>
>>> + priv->phy->advertising &= ~(ADVERTISED_1000baseT_Full |
>>> + ADVERTISED_1000baseT_Half);
>>> phy_start(priv->phy);
>>> napi_enable(&priv->napi);
>>>
>>
>> This is not sufficient to disable gigabit speeds; the supported mask
>> should also be limited. And it should be done even before the net
>
> I tried that, but when I also limit supported mask the phy driver doesn't
> touch gigabit advertising register int the genphy_config_advert at all.
> That's probably right for ethtool interface, but ethoc doesn't support
> ethtool.
This is not right for libphy either, phydev->supported must reflect
that you support Gigabit or not.
Since ethoc supports libphy, there really is no reason not to
implement the ethtool_{get,set}_settings callbacks using
phy_mii_ioctl().
>
>> device is registered.
>>
>> Rather than poking into the phy_device structure directly from this
>> driver, I think you should add a function in phylib for this purpose.
All drivers are currently modifying phydev->advertising and
phydev->supported directly, most of them do it correctly as far as I
checked. It does make some sense to add a helper function though.
>
> Like below?
>
> ---8<---
> From 347331f399626ecaa9a8e54252f55e0b6788772f Mon Sep 17 00:00:00 2001
> From: Max Filippov <[email protected]>
> Date: Mon, 27 Jan 2014 04:01:40 +0400
> Subject: [PATCH 1/3] net: ethoc: don't advertise gigabit speed on attached PHY
>
> OpenCores 10/100 Mbps MAC does not support speeds above 100 Mbps, but does
> not disable advertisement when PHY supports them. This results in
> non-functioning network when the MAC is connected to a gigabit PHY connected
> to a gigabit switch.
>
> The fix is to disable gigabit speed advertisement on attached PHY
> unconditionally.
>
> Signed-off-by: Max Filippov <[email protected]>
> ---
> drivers/net/ethernet/ethoc.c | 3 +++
> include/linux/phy.h | 5 +++++
> 2 files changed, 8 insertions(+)
>
> diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c
> index 4de8cfd..e817d58 100644
> --- a/drivers/net/ethernet/ethoc.c
> +++ b/drivers/net/ethernet/ethoc.c
> @@ -688,6 +688,9 @@ static int ethoc_mdio_probe(struct net_device *dev)
> }
>
> priv->phy = phy;
> + phy_update_adv(phy,
> + ~(ADVERTISED_1000baseT_Full |
> + ADVERTISED_1000baseT_Half), 0);
> return 0;
> }
>
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 48a4dc3..0282a8d 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -559,6 +559,11 @@ static inline int phy_read_status(struct phy_device *phydev) {
> return phydev->drv->read_status(phydev);
> }
>
> +static inline void phy_update_adv(struct phy_device *phydev, u32 mask, u32 set)
> +{
> + phydev->advertising = (phydev->advertising & mask) | set;
> +}
> +
This should be a preliminary patch to this patchset, so you first
introduce the function, then use it in the driver, but the idea looks
good. There is room for updating quite some drivers out there since
those used to modify phydev->advertising and phydev->supported
directly without using an accessor.
> int genphy_setup_forced(struct phy_device *phydev);
> int genphy_restart_aneg(struct phy_device *phydev);
> int genphy_config_aneg(struct phy_device *phydev);
> --
> 1.8.1.4
>
>
> --
> Thanks.
> -- Max
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Florian
2014-01-27 Max Filippov <[email protected]>:
> Hi Rob,
>
> On Mon, Jan 27, 2014 at 6:10 PM, Rob Herring <[email protected]> wrote:
>> On Sun, Jan 26, 2014 at 9:59 PM, Max Filippov <[email protected]> wrote:
>>> Signed-off-by: Max Filippov <[email protected]>
>>> ---
>>> .../devicetree/bindings/net/opencores-ethoc.txt | 25 ++++++++++++++++++++++
>>> 1 file changed, 25 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/net/opencores-ethoc.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/opencores-ethoc.txt b/Documentation/devicetree/bindings/net/opencores-ethoc.txt
>>> new file mode 100644
>>> index 0000000..f7c6c94
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/net/opencores-ethoc.txt
>>> @@ -0,0 +1,25 @@
>>> +* OpenCores MAC 10/100 Mbps
>>> +
>>> +Required properties:
>>> +- compatible: Should be "opencores,ethoc".
>>
>> There are not different versions of IP or is the version probeable?
>
> AFAIK there's single version of this 10/100 MAC.
> It doesn't have any identification registers.
Since this is an IP block that people can modify due to its open
source nature, it would have been good to define a revision register
or such which would allow software to gate specific code based on that
revision.
>
>>> +- reg: Address and length of the register set for the device and of its
>>> + descriptor memory.
>>> +- interrupts: Should contain ethoc interrupt.
>>> +
>>> +Optional properties:
>>> +- local-mac-address: 6 bytes, mac address
>>
>> There's a patch in progress to move all the standard network
>> properties to a common location, so you can remove this.
>
> Will do.
>
>>> +- clock-frequency: basic MAC frequency.
>>> +- mii-mgmt-clock-frequency: frequency of MII management bus. Together
>>> + with clock-frequency determines the value programmed into the CLKDIV
>>> + field of MIIMODER register.
>>> +
>>> +Examples:
>>> +
>>> + enet0: ethoc@fd030000 {
>>> + compatible = "opencores,ethoc";
>>> + reg = <0xfd030000 0x4000 0xfd800000 0x4000>;
>>> + interrupts = <1>;
>>> + local-mac-address = [00 50 c2 13 6f 00];
>>> + clock-frequency = <50000000>;
>>> + mii-mgmt-clock-frequency = <5000000>;
5Mhz management clock? Can't you make it work with the standard 2.5Mhz
management clock? Is not there a risk not to be able to "talk" to some
PHY chips out there which do not support > 2.5Mhz management clock?
Since this is an ethoc specific property, should it be prefixed with "ethoc,"?
--
Florian
On Mon, Jan 27, 2014 at 11:36 PM, Florian Fainelli <[email protected]> wrote:
> 2014-01-27 Max Filippov <[email protected]>:
>> On Mon, Jan 27, 2014 at 2:18 PM, Ben Hutchings <[email protected]> wrote:
>>> On Mon, 2014-01-27 at 07:59 +0400, Max Filippov wrote:
>>>> OpenCores 10/100 Mbps MAC does not support speeds above 100 Mbps, but does
>>>> not disable advertisement when PHY supports them. This results in
>>>> non-functioning network when the MAC is connected to a gigabit PHY connected
>>>> to a gigabit switch.
>>>>
>>>> The fix is to disable gigabit speed advertisement on attached PHY
>>>> unconditionally.
>>>>
>>>> Signed-off-by: Max Filippov <[email protected]>
>>>> ---
>>>> drivers/net/ethernet/ethoc.c | 2 ++
>>>> 1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c
>>>> index 4de8cfd..0aa1a05 100644
>>>> --- a/drivers/net/ethernet/ethoc.c
>>>> +++ b/drivers/net/ethernet/ethoc.c
>>>> @@ -712,6 +712,8 @@ static int ethoc_open(struct net_device *dev)
>>>> netif_start_queue(dev);
>>>> }
>>>>
>>>> + priv->phy->advertising &= ~(ADVERTISED_1000baseT_Full |
>>>> + ADVERTISED_1000baseT_Half);
>>>> phy_start(priv->phy);
>>>> napi_enable(&priv->napi);
>>>>
>>>
>>> This is not sufficient to disable gigabit speeds; the supported mask
>>> should also be limited. And it should be done even before the net
>>
>> I tried that, but when I also limit supported mask the phy driver doesn't
>> touch gigabit advertising register int the genphy_config_advert at all.
>> That's probably right for ethtool interface, but ethoc doesn't support
>> ethtool.
>
> This is not right for libphy either, phydev->supported must reflect
> that you support Gigabit or not.
I'm sorry, does that mean that I must not touch phydev->supported,
or that I must update it too and somehow fix genphy_config_advert?
> Since ethoc supports libphy, there really is no reason not to
> implement the ethtool_{get,set}_settings callbacks using
> phy_mii_ioctl().
Ok, I'll add that.
>>> device is registered.
>>>
>>> Rather than poking into the phy_device structure directly from this
>>> driver, I think you should add a function in phylib for this purpose.
>
> All drivers are currently modifying phydev->advertising and
> phydev->supported directly, most of them do it correctly as far as I
> checked. It does make some sense to add a helper function though.
[...]
>> diff --git a/include/linux/phy.h b/include/linux/phy.h
>> index 48a4dc3..0282a8d 100644
>> --- a/include/linux/phy.h
>> +++ b/include/linux/phy.h
>> @@ -559,6 +559,11 @@ static inline int phy_read_status(struct phy_device *phydev) {
>> return phydev->drv->read_status(phydev);
>> }
>>
>> +static inline void phy_update_adv(struct phy_device *phydev, u32 mask, u32 set)
>> +{
>> + phydev->advertising = (phydev->advertising & mask) | set;
>> +}
>> +
>
> This should be a preliminary patch to this patchset, so you first
> introduce the function, then use it in the driver, but the idea looks
> good. There is room for updating quite some drivers out there since
> those used to modify phydev->advertising and phydev->supported
> directly without using an accessor.
What about those that do something like this:
phydev->advertising = phydev->supported;
leave them as is, or provide read accessor for phydev->supported?
--
Thanks.
-- Max
On Mon, Jan 27, 2014 at 11:45 PM, Florian Fainelli <[email protected]> wrote:
> 2014-01-27 Max Filippov <[email protected]>:
>> On Mon, Jan 27, 2014 at 6:10 PM, Rob Herring <[email protected]> wrote:
>>> On Sun, Jan 26, 2014 at 9:59 PM, Max Filippov <[email protected]> wrote:
[...]
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/net/opencores-ethoc.txt
>>>> @@ -0,0 +1,25 @@
>>>> +* OpenCores MAC 10/100 Mbps
>>>> +
>>>> +Required properties:
>>>> +- compatible: Should be "opencores,ethoc".
>>>
>>> There are not different versions of IP or is the version probeable?
>>
>> AFAIK there's single version of this 10/100 MAC.
>> It doesn't have any identification registers.
>
> Since this is an IP block that people can modify due to its open
> source nature, it would have been good to define a revision register
> or such which would allow software to gate specific code based on that
> revision.
Probably yes, though I haven't heard of any modified versions of this MAC
out there.
[...]
>>>> +Examples:
>>>> +
>>>> + enet0: ethoc@fd030000 {
>>>> + compatible = "opencores,ethoc";
>>>> + reg = <0xfd030000 0x4000 0xfd800000 0x4000>;
>>>> + interrupts = <1>;
>>>> + local-mac-address = [00 50 c2 13 6f 00];
>>>> + clock-frequency = <50000000>;
>>>> + mii-mgmt-clock-frequency = <5000000>;
>
> 5Mhz management clock? Can't you make it work with the standard 2.5Mhz
> management clock? Is not there a risk not to be able to "talk" to some
> PHY chips out there which do not support > 2.5Mhz management clock?
Yes I can, it just didn't occur to me that there is a standard 2.5MHz setting.
> Since this is an ethoc specific property, should it be prefixed with "ethoc,"?
Is it worth keeping this parameter at all, or just always default to 2.5MHz?
--
Thanks.
-- Max
2014-01-27 Max Filippov <[email protected]>:
> On Mon, Jan 27, 2014 at 11:45 PM, Florian Fainelli <[email protected]> wrote:
>> 2014-01-27 Max Filippov <[email protected]>:
>>> On Mon, Jan 27, 2014 at 6:10 PM, Rob Herring <[email protected]> wrote:
>>>> On Sun, Jan 26, 2014 at 9:59 PM, Max Filippov <[email protected]> wrote:
>
> [...]
>
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/net/opencores-ethoc.txt
>>>>> @@ -0,0 +1,25 @@
>>>>> +* OpenCores MAC 10/100 Mbps
>>>>> +
>>>>> +Required properties:
>>>>> +- compatible: Should be "opencores,ethoc".
>>>>
>>>> There are not different versions of IP or is the version probeable?
>>>
>>> AFAIK there's single version of this 10/100 MAC.
>>> It doesn't have any identification registers.
>>
>> Since this is an IP block that people can modify due to its open
>> source nature, it would have been good to define a revision register
>> or such which would allow software to gate specific code based on that
>> revision.
>
> Probably yes, though I haven't heard of any modified versions of this MAC
> out there.
>
> [...]
>
>>>>> +Examples:
>>>>> +
>>>>> + enet0: ethoc@fd030000 {
>>>>> + compatible = "opencores,ethoc";
>>>>> + reg = <0xfd030000 0x4000 0xfd800000 0x4000>;
>>>>> + interrupts = <1>;
>>>>> + local-mac-address = [00 50 c2 13 6f 00];
>>>>> + clock-frequency = <50000000>;
>>>>> + mii-mgmt-clock-frequency = <5000000>;
>>
>> 5Mhz management clock? Can't you make it work with the standard 2.5Mhz
>> management clock? Is not there a risk not to be able to "talk" to some
>> PHY chips out there which do not support > 2.5Mhz management clock?
>
> Yes I can, it just didn't occur to me that there is a standard 2.5MHz setting.
>
>> Since this is an ethoc specific property, should it be prefixed with "ethoc,"?
>
> Is it worth keeping this parameter at all, or just always default to 2.5MHz?
Your example lists 5Mhz, so I assume this is of some use for you on
the platform you are working with? My concern is that this might not
be compatible with all PHY devices out there and might create hard to
debug issues where you get some MDIO transactions to succeeds and some
which don't.
--
Florian
(fixing Rob's email)
2014-01-27 Max Filippov <[email protected]>:
> On Mon, Jan 27, 2014 at 11:36 PM, Florian Fainelli <[email protected]> wrote:
>> 2014-01-27 Max Filippov <[email protected]>:
>>> On Mon, Jan 27, 2014 at 2:18 PM, Ben Hutchings <[email protected]> wrote:
>>>> On Mon, 2014-01-27 at 07:59 +0400, Max Filippov wrote:
>>>>> OpenCores 10/100 Mbps MAC does not support speeds above 100 Mbps, but does
>>>>> not disable advertisement when PHY supports them. This results in
>>>>> non-functioning network when the MAC is connected to a gigabit PHY connected
>>>>> to a gigabit switch.
>>>>>
>>>>> The fix is to disable gigabit speed advertisement on attached PHY
>>>>> unconditionally.
>>>>>
>>>>> Signed-off-by: Max Filippov <[email protected]>
>>>>> ---
>>>>> drivers/net/ethernet/ethoc.c | 2 ++
>>>>> 1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c
>>>>> index 4de8cfd..0aa1a05 100644
>>>>> --- a/drivers/net/ethernet/ethoc.c
>>>>> +++ b/drivers/net/ethernet/ethoc.c
>>>>> @@ -712,6 +712,8 @@ static int ethoc_open(struct net_device *dev)
>>>>> netif_start_queue(dev);
>>>>> }
>>>>>
>>>>> + priv->phy->advertising &= ~(ADVERTISED_1000baseT_Full |
>>>>> + ADVERTISED_1000baseT_Half);
>>>>> phy_start(priv->phy);
>>>>> napi_enable(&priv->napi);
>>>>>
>>>>
>>>> This is not sufficient to disable gigabit speeds; the supported mask
>>>> should also be limited. And it should be done even before the net
>>>
>>> I tried that, but when I also limit supported mask the phy driver doesn't
>>> touch gigabit advertising register int the genphy_config_advert at all.
>>> That's probably right for ethtool interface, but ethoc doesn't support
>>> ethtool.
>>
>> This is not right for libphy either, phydev->supported must reflect
>> that you support Gigabit or not.
>
> I'm sorry, does that mean that I must not touch phydev->supported,
> or that I must update it too and somehow fix genphy_config_advert?
What I meant is that your driver has to set both phydev->advertising
and phydev->supported. genphy_config_advert() is doing
phdev->advertising &= phydev->supported, leaving phydev->supported to
something more restrictive will produce unexpected results. This is
also important if phy_driver::config_aneg() is not using the generic
implementation and directly uses/modifies phydev->advertising and
phydev->supported.
It seems like there is room for improvements in that area regardless
of how we access things. For instance, specifying the PHY interface
mode (MII, GMII etc...) should already provide a hint of what
phydev->supported should look like. You cannot do Gigabit with MII for
instance.
>
>> Since ethoc supports libphy, there really is no reason not to
>> implement the ethtool_{get,set}_settings callbacks using
>> phy_mii_ioctl().
>
> Ok, I'll add that.
>
>>>> device is registered.
>>>>
>>>> Rather than poking into the phy_device structure directly from this
>>>> driver, I think you should add a function in phylib for this purpose.
>>
>> All drivers are currently modifying phydev->advertising and
>> phydev->supported directly, most of them do it correctly as far as I
>> checked. It does make some sense to add a helper function though.
>
> [...]
>
>>> diff --git a/include/linux/phy.h b/include/linux/phy.h
>>> index 48a4dc3..0282a8d 100644
>>> --- a/include/linux/phy.h
>>> +++ b/include/linux/phy.h
>>> @@ -559,6 +559,11 @@ static inline int phy_read_status(struct phy_device *phydev) {
>>> return phydev->drv->read_status(phydev);
>>> }
>>>
>>> +static inline void phy_update_adv(struct phy_device *phydev, u32 mask, u32 set)
>>> +{
>>> + phydev->advertising = (phydev->advertising & mask) | set;
>>> +}
>>> +
>>
>> This should be a preliminary patch to this patchset, so you first
>> introduce the function, then use it in the driver, but the idea looks
>> good. There is room for updating quite some drivers out there since
>> those used to modify phydev->advertising and phydev->supported
>> directly without using an accessor.
>
> What about those that do something like this:
>
> phydev->advertising = phydev->supported;
>
> leave them as is, or provide read accessor for phydev->supported?
Those should be just fine since genphy_config_advert() does
phydev->advertising &= phydev->supported, so the end result will be
identical. You mean a write accessor instead of a read accessor?
--
Florian