Re: [PATCH net] net: dsa: microchip: ksz8795: really set the correct number of ports
On 16.09.2020 13:08, Matthias Schiffer wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> The KSZ9477 and KSZ8795 use the port_cnt field differently: For the
> KSZ9477, it includes the CPU port(s), while for the KSZ8795, it doesn't.
>
> It would be a good cleanup to make the handling of both drivers match,
> but as a first step, fix the recently broken assignment of num_ports in
> the KSZ8795 driver (which completely broke probing, as the CPU port
> index was always failing the num_ports check).
>
> Fixes: af199a1a9cb0 ("net: dsa: microchip: set the correct number of
> ports")
> Signed-off-by: Matthias Schiffer
> ---
Sorry about this. I assumed consistency between the two drivers.
Reviewed-by: Codrin Ciubotariu
Re: [PATCH net-next 3/7] net: macb: parse PHY nodes found under an MDIO node
On 21.07.2020 16:36, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > >> @@ -755,7 +765,6 @@ static int macb_mdiobus_register(struct macb *bp) >> * decrement it before returning. >> */ >>of_node_put(child); >> - >>return of_mdiobus_register(bp->mii_bus, np); >>} > > Please avoid white space changes like this. Sorry about this, it was not intended. Will fix in v2. Thanks! > > Otherwise this looks O.K. > > Andrew >
Re: [PATCH net-next 3/7] net: macb: parse PHY nodes found under an MDIO node
On 21.07.2020 16:36, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > >> @@ -755,7 +765,6 @@ static int macb_mdiobus_register(struct macb *bp) >> * decrement it before returning. >> */ >>of_node_put(child); >> - >>return of_mdiobus_register(bp->mii_bus, np); >>} > > Please avoid white space changes like this. Sorry about this, it was not intended. Will fix in v2. Thanks! > > Otherwise this looks O.K. > > Andrew >
Re: [PATCH net-next 2/7] macb: bindings doc: use an MDIO node as a container for PHY nodes
On 21.07.2020 16:29, Alexandre Belloni wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> Hi,
>
> The proper subject prefix is dt-bindings: net: macb:
Will fix in v2. Thanks!
>
> On 21/07/2020 13:02:29+0300, Codrin Ciubotariu wrote:
>> The MACB driver embeds an MDIO bus controller and for this reason there
>> was no need for an MDIO sub-node present to contain the PHY nodes. Adding
>> MDIO devies directly under an Ethernet node is deprecated, so an MDIO node
>> is included to contain of the PHY nodes (and other MDIO devices' nodes).
>>
>> Signed-off-by: Codrin Ciubotariu
>> ---
>> Documentation/devicetree/bindings/net/macb.txt | 15 ---
>> 1 file changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/macb.txt
>> b/Documentation/devicetree/bindings/net/macb.txt
>> index 0b61a90f1592..88d5199c2279 100644
>> --- a/Documentation/devicetree/bindings/net/macb.txt
>> +++ b/Documentation/devicetree/bindings/net/macb.txt
>> @@ -32,6 +32,11 @@ Required properties:
>> The MAC address will be determined using the optional properties
>> defined in ethernet.txt.
>>
>> +Optional subnodes:
>> +- mdio : specifies the MDIO bus in the MACB, used as a container for PHY
>> nodes or other
>> + nodes of devices present on the MDIO bus. Please see ethernet-phy.yaml in
>> the same
>> + directory for more details.
>> +
>> Optional properties for PHY child node:
>> - reset-gpios : Should specify the gpio for phy reset
>> - magic-packet : If present, indicates that the hardware supports waking
>> @@ -48,8 +53,12 @@ Examples:
>>local-mac-address = [3a 0e 03 04 05 06];
>>clock-names = "pclk", "hclk", "tx_clk";
>>clocks = <&clkc 30>, <&clkc 30>, <&clkc 13>;
>> - ethernet-phy@1 {
>> - reg = <0x1>;
>> - reset-gpios = <&pioE 6 1>;
>> + mdio {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + ethernet-phy@1 {
>> + reg = <0x1>;
>> + reset-gpios = <&pioE 6 1>;
>> + };
>>};
>>};
>> --
>> 2.25.1
>>
>
> --
> Alexandre Belloni, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>
Re: [PATCH net-next 3/7] net: macb: parse PHY nodes found under an MDIO node
On 21.07.2020 16:36, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > >> @@ -755,7 +765,6 @@ static int macb_mdiobus_register(struct macb *bp) >> * decrement it before returning. >> */ >>of_node_put(child); >> - >>return of_mdiobus_register(bp->mii_bus, np); >>} > > Please avoid white space changes like this. Sorry about this, it was not intended. Will fix in v2. Thanks! > > Otherwise this looks O.K. > > Andrew >
Re: [PATCH net-next v2 0/7] Add an MDIO sub-node under MACB
On 22.07.2020 13:32, Claudiu Beznea - M18063 wrote:
>
>
> On 21.07.2020 20:13, Codrin Ciubotariu wrote:
>> Adding the PHY nodes directly under the Ethernet node became deprecated,
>> so the aim of this patch series is to make MACB use an MDIO node as
>> container for MDIO devices.
>> This patch series starts with a small patch to use the device-managed
>> devm_mdiobus_alloc(). In the next two patches we update the bindings and
>> adapt macb driver to parse the device-tree PHY nodes from under an MDIO
>> node. The last patches add the MDIO node in the device-trees of sama5d2,
>> sama5d3, samad4 and sam9x60 boards.
>>
>
> Tested this series on sama5d2_xplained in the following scenarios:
>
> 1/ PHY bindings from patch 4/7:
> mdio {
> #address-cells = <1>;
> #size-cells = <0>;
> ethernet-phy@1 {
> reg = <0x1>;
> interrupt-parent = <&pioA>;
> interrupts = ;
> };
>
> 2/ PHY bindings before this series:
> ethernet-phy@1 {
> reg = <0x1>;
> interrupt-parent = <&pioA>;
> interrupts = ;
> };
>
> 3/ No PHY bindings at all.
>
> All 3 cases went OK.
>
> You can add:
> Tested-by: Claudiu Beznea
> Acked-by: Claudiu Beznea
Thank you very much Claudiu!
There is still one more case in my mind. macb could be a fixed-link with
an MDIO DSA switch. While the macb would have a fixed connection with a
port from the DSA switch, the switch could be configured using macb's
MDIO. The dt would be something like:
macb {
fixed-link {
...
};
mdio {
switch@0 {
...
};
};
};
To support this, in patch 3/7 I should first check for the mdio node to
return of_mdiobus_register() and then check if it's a fixed-link to
return simple mdiobus_register(). I will address this in v3...
Thanks and best regards,
Codrin
>
> Thank you,
> Claudiu Beznea
>
>> Changes in v2:
>> - renamed patch 2/7 from "macb: bindings doc: use an MDIO node as a
>> container for PHY nodes" to "dt-bindings: net: macb: use an MDIO
>> node as a container for PHY nodes"
>> - added back a newline removed by mistake in patch 3/7
>>
>> Codrin Ciubotariu (7):
>>net: macb: use device-managed devm_mdiobus_alloc()
>>dt-bindings: net: macb: use an MDIO node as a container for PHY nodes
>>net: macb: parse PHY nodes found under an MDIO node
>>ARM: dts: at91: sama5d2: add an mdio sub-node to macb
>>ARM: dts: at91: sama5d3: add an mdio sub-node to macb
>>ARM: dts: at91: sama5d4: add an mdio sub-node to macb
>>ARM: dts: at91: sam9x60: add an mdio sub-node to macb
>>
>> Documentation/devicetree/bindings/net/macb.txt | 15 ---
>> arch/arm/boot/dts/at91-sam9x60ek.dts | 8 ++--
>> arch/arm/boot/dts/at91-sama5d27_som1.dtsi | 16 ++--
>> arch/arm/boot/dts/at91-sama5d27_wlsom1.dtsi| 17 ++---
>> arch/arm/boot/dts/at91-sama5d2_ptc_ek.dts | 13 -
>> arch/arm/boot/dts/at91-sama5d2_xplained.dts| 12
>> arch/arm/boot/dts/at91-sama5d3_xplained.dts| 16
>> arch/arm/boot/dts/at91-sama5d4_xplained.dts| 12
>> drivers/net/ethernet/cadence/macb_main.c | 18 --
>> 9 files changed, 86 insertions(+), 41 deletions(-)
Re: [PATCH net-next v2 0/7] Add an MDIO sub-node under MACB
On 23.07.2020 10:51, Claudiu Beznea - M18063 wrote:
>
>
> On 22.07.2020 14:38, Codrin Ciubotariu - M19940 wrote:
>> On 22.07.2020 13:32, Claudiu Beznea - M18063 wrote:
>>>
>>>
>>> On 21.07.2020 20:13, Codrin Ciubotariu wrote:
Adding the PHY nodes directly under the Ethernet node became deprecated,
so the aim of this patch series is to make MACB use an MDIO node as
container for MDIO devices.
This patch series starts with a small patch to use the device-managed
devm_mdiobus_alloc(). In the next two patches we update the bindings and
adapt macb driver to parse the device-tree PHY nodes from under an MDIO
node. The last patches add the MDIO node in the device-trees of sama5d2,
sama5d3, samad4 and sam9x60 boards.
>>>
>>> Tested this series on sama5d2_xplained in the following scenarios:
>>>
>>> 1/ PHY bindings from patch 4/7:
>>> mdio {
>>> #address-cells = <1>;
>>> #size-cells = <0>;
>>> ethernet-phy@1 {
>>> reg = <0x1>;
>>> interrupt-parent = <&pioA>;
>>> interrupts = ;
>>> };
>>>
>>> 2/ PHY bindings before this series:
>>> ethernet-phy@1 {
>>> reg = <0x1>;
>>> interrupt-parent = <&pioA>;
>>> interrupts = ;
>>> };
>>>
>>> 3/ No PHY bindings at all.
>>>
>>> All 3 cases went OK.
>>>
>>> You can add:
>>> Tested-by: Claudiu Beznea
>>> Acked-by: Claudiu Beznea
>>
>> Thank you very much Claudiu!
>> There is still one more case in my mind. macb could be a fixed-link with
>> an MDIO DSA switch. While the macb would have a fixed connection with a
>> port from the DSA switch, the switch could be configured using macb's
>> MDIO. The dt would be something like:
>>
>> macb {
>> fixed-link {
>> ...
>> };
>> mdio {
>> switch@0 {
>> ...
>> };
>> };
>> };
>
> Do you have a setup for testing this? At the moment I don't know a
> configuration like this that macb is working with.
There isn't one that I am aware of, but we should address it.
>
>>
>> To support this, in patch 3/7 I should first check for the mdio node to
>> return of_mdiobus_register() and then check if it's a fixed-link to
>> return simple mdiobus_register(). I will address this in v3...>
>> Thanks and best regards,
>> Codrin
>>
>>>
>>> Thank you,
>>> Claudiu Beznea
>>>
Changes in v2:
- renamed patch 2/7 from "macb: bindings doc: use an MDIO node as a
container for PHY nodes" to "dt-bindings: net: macb: use an MDIO
node as a container for PHY nodes"
- added back a newline removed by mistake in patch 3/7
Codrin Ciubotariu (7):
net: macb: use device-managed devm_mdiobus_alloc()
dt-bindings: net: macb: use an MDIO node as a container for PHY nodes
net: macb: parse PHY nodes found under an MDIO node
ARM: dts: at91: sama5d2: add an mdio sub-node to macb
ARM: dts: at91: sama5d3: add an mdio sub-node to macb
ARM: dts: at91: sama5d4: add an mdio sub-node to macb
ARM: dts: at91: sam9x60: add an mdio sub-node to macb
Documentation/devicetree/bindings/net/macb.txt | 15 ---
arch/arm/boot/dts/at91-sam9x60ek.dts | 8 ++--
arch/arm/boot/dts/at91-sama5d27_som1.dtsi | 16 ++--
arch/arm/boot/dts/at91-sama5d27_wlsom1.dtsi| 17 ++---
arch/arm/boot/dts/at91-sama5d2_ptc_ek.dts | 13 -
arch/arm/boot/dts/at91-sama5d2_xplained.dts| 12
arch/arm/boot/dts/at91-sama5d3_xplained.dts| 16
arch/arm/boot/dts/at91-sama5d4_xplained.dts| 12
drivers/net/ethernet/cadence/macb_main.c | 18 --
9 files changed, 86 insertions(+), 41 deletions(-)
Re: [PATCH net-next v2 3/7] net: macb: parse PHY nodes found under an MDIO node
On 23.07.2020 21:59, Florian Fainelli wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> On 7/21/20 10:13 AM, Codrin Ciubotariu wrote:
>> The MACB embeds an MDIO bus controller. For this reason, the PHY nodes
>> were represented as sub-nodes in the MACB node. Generally, the
>> Ethernet controller is different than the MDIO controller, so the PHYs
>> are probed by a separate MDIO driver. Since adding the PHY nodes directly
>> under the ETH node became deprecated, we adjust the MACB driver to look
>> for an MDIO node and register the subnode MDIO devices.
>>
>> Signed-off-by: Codrin Ciubotariu
>> ---
>>
>> Changes in v2:
>> - readded newline removed by mistake;
>>
>> drivers/net/ethernet/cadence/macb_main.c | 10 ++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c
>> b/drivers/net/ethernet/cadence/macb_main.c
>> index 89fe7af5e408..b25c64b45148 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -740,10 +740,20 @@ static int macb_mii_probe(struct net_device *dev)
>> static int macb_mdiobus_register(struct macb *bp)
>> {
>>struct device_node *child, *np = bp->pdev->dev.of_node;
>> + struct device_node *mdio_node;
>> + int ret;
>>
>>if (of_phy_is_fixed_link(np))
>>return mdiobus_register(bp->mii_bus);
>
> Does not this need changing as well? Consider the use case of having
> your MACB Ethernet node have a fixed-link property to describe how it
> connects to a switch, and your MACB MDIO controller, expressed as a
> sub-node, describing the MDIO attached switch it connects to.
Right, this is what I was discussing with Claudiu on the other thread. I
am thinking to just move the look for mdio before checking for
fixed-link. This will probe the MDIO devices and simple mdiobus_register
will be called only if the mdio node is missing.
Thank you for your review(s)!
Best regards,
Codrin
>
>>
>> + /* if an MDIO node is present, it should contain the PHY nodes */
>> + mdio_node = of_get_child_by_name(np, "mdio");
>> + if (mdio_node) {
>> + ret = of_mdiobus_register(bp->mii_bus, mdio_node);
>> + of_node_put(mdio_node);
>> + return ret;
>> + }
>> +
>>/* Only create the PHY from the device tree if at least one PHY is
>> * described. Otherwise scan the entire MDIO bus. We do this to
>> support
>> * old device tree that did not follow the best practices and did not
>>
>
>
> --
> Florian
>
Re: [PATCH net-next v3 3/7] net: macb: parse PHY nodes found under an MDIO node
On 24.07.2020 20:40, Florian Fainelli wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> On 7/24/20 3:50 AM, Codrin Ciubotariu wrote:
>> The MACB embeds an MDIO bus controller. For this reason, the PHY nodes
>> were represented as sub-nodes in the MACB node. Generally, the
>> Ethernet controller is different than the MDIO controller, so the PHYs
>> are probed by a separate MDIO driver. Since adding the PHY nodes directly
>> under the ETH node became deprecated, we adjust the MACB driver to look
>> for an MDIO node and register the subnode MDIO devices.
>>
>> Signed-off-by: Codrin Ciubotariu
>> ---
>>
>> Changes in v3:
>> - moved the check for the mdio node at the beginnging of
>> macb_mdiobus_register(). This way, the mdio devices will be probed even
>> if macb is a fixed-link
>>
>> Changes in v2:
>> - readded newline removed by mistake;
>>
>> drivers/net/ethernet/cadence/macb_main.c | 10 ++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c
>> b/drivers/net/ethernet/cadence/macb_main.c
>> index 89fe7af5e408..cb0b3637651c 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -740,6 +740,16 @@ static int macb_mii_probe(struct net_device *dev)
>> static int macb_mdiobus_register(struct macb *bp)
>> {
>>struct device_node *child, *np = bp->pdev->dev.of_node;
>> + struct device_node *mdio_node;
>> + int ret;
>> +
>> + /* if an MDIO node is present, it should contain the PHY nodes */
>> + mdio_node = of_get_child_by_name(np, "mdio");
>> + if (mdio_node) {
>> + ret = of_mdiobus_register(bp->mii_bus, mdio_node);
>> + of_node_put(mdio_node);
>> + return ret;
>> + }
>
> This does take care of registering the MDIO bus controller when present
> as a sub-node, however if you also plan on making use of fixed-link, we
> will have already returned.
>
>>
>>if (of_phy_is_fixed_link(np))
>>return mdiobus_register(bp->mii_bus);
>>
>
> Really not sure what this is achieving, because we start off assuming
> that we have an OF driven configuration, but later on we register the
> MDIO bus with mdiobus_register() (and not of_mdiobus_register()), so no
> scanning of the MDIO bus will happen.
I agree that returning mdiobus_register() here makes no sense since
there are no other PHY devices to scan for and probe. I can replace this
with NULL.
The reason for fixed-link check here is to skip the following loop:
for_each_available_child_of_node(np, child)
if (of_mdiobus_child_is_phy(child)) {
of_node_put(child);
return of_mdiobus_register(bp->mii_bus, np);
}
of_mdiobus_child_is_phy() returns true when it finds the fixed-link
node, because it has no compatible.
>
> How does the driver currently support being provided a fixed-link
> property? Should not we at least have this pattern:
>
> */
> if (of_phy_is_fixed_link(dn)) {
> ret = of_phy_register_fixed_link(dn);
> if (ret)
> return ret;
>
> priv->phy_dn = dn;
> }
>
> It does not look like you are breaking anything here, because it does
> not look like this works at all.
From what I understand, the new phylink(_create) handles the fixed-link
case, so there is not reason to explicitly register the fixed-link.
Best regards,
Codrin
> --
> Florian
>
Re: [PATCH 1/2] net: dsa: microchip: set the correct number of ports in dsa_switch
On 01.07.2020 20:09, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> On Wed, Jul 01, 2020 at 07:51:27PM +0300, Codrin Ciubotariu wrote:
>> The number of ports is incorrectly set to the maximum available for a DSA
>> switch. Even if the extra ports are not used, this causes some functions
>> to be called later, like port_disable() and port_stp_state_set(). If the
>> driver doesn't check the port index, it will end up modifying unknown
>> registers.
>>
>> Fixes: b987e98e50ab ("dsa: add DSA switch driver for Microchip KSZ9477")
>
> Hi Codrin
>
> You don't indicate which tree this is for. net-next, or net? It looks
> like it fixes a real issue, so it probably should be for net. But
> patches to net should be minimal. Is it possible to do the
>
> ds->num_ports = swdev->port_cnt;
>
> without all the other changes? You can then have a refactoring patch
> in net-next.
This one should be for net. Ok then, I will send a simpler version of
this patch for net, just to fix the issue and another one like this one
for net-next.
Thanks and best regards,
Codrin
>
> Thanks
> Andrew
>
Re: [PATCH net-next] net: dsa: microchip: split adjust_link() in phylink_mac_link_{up|down}()
On 02.07.2020 13:19, Russell King - ARM Linux admin wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > On Thu, Jul 02, 2020 at 12:54:39PM +0300, Codrin Ciubotariu wrote: >> The DSA subsystem moved to phylink and adjust_link() became deprecated in >> the process. This patch removes adjust_link from the KSZ DSA switches and >> adds phylink_mac_link_up() and phylink_mac_link_down(). >> >> Signed-off-by: Codrin Ciubotariu > > For the code _transformation_ that the patch does: > > Reviewed-by: Russell King > > as it is equivalent. However, for a deeper review of what is going > on here, I've a question: > > $ grep live_ports * > ksz8795.c: dev->live_ports = dev->host_mask; > ksz8795.c: dev->live_ports |= BIT(port); > ksz_common.h: u16 live_ports; > ksz_common.c: dev->live_ports &= ~(1 << port); > ksz_common.c: dev->live_ports |= (1 << port) & dev->on_ports; > ksz_common.c: dev->live_ports &= ~(1 << port); > ksz9477.c: dev->live_ports = dev->host_mask; > ksz9477.c: dev->live_ports |= (1 << port); > > These are the only references to dev->live_ports, so the purpose of > dev->live_ports seems unclear; it seems it is only updated but never > read. Can it be removed, along with the locking in your new functions? Sure, I'll make a patch to clean things up. I will resend this patch in a series to mark the dependency. Thanks and best regards, Codrin > > Thanks. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! >
Re: [PATCH net] net: dsa: microchip: set the correct number of ports
On 02.07.2020 16:50, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> On Thu, Jul 02, 2020 at 12:44:50PM +0300, Codrin Ciubotariu wrote:
>> The number of ports is incorrectly set to the maximum available for a DSA
>> switch. Even if the extra ports are not used, this causes some functions
>> to be called later, like port_disable() and port_stp_state_set(). If the
>> driver doesn't check the port index, it will end up modifying unknown
>> registers.
>>
>> Fixes: b987e98e50ab ("dsa: add DSA switch driver for Microchip KSZ9477")
>> Signed-off-by: Codrin Ciubotariu
>
> Reviewed-by: Andrew Lunn
>
> Thanks for the minimum patch.
>
> If you wait about a week, net will get merged into net-next. You can
> then submit a refactoring patch based on your previous version.
>
> Andrew
>
Sure thing. This small version does the job, so I will see about the
refactoring, maybe I will group it with something else...
Thank you for your review!
Best regards,
Codrin
