diff mbox series

[v2,1/3] dt-bindings: net: fsl-fec add mdc/mdio bitbang option

Message ID 20201129220000.16550-1-adrien.grassein@gmail.com
State New
Headers show
Series [v2,1/3] dt-bindings: net: fsl-fec add mdc/mdio bitbang option | expand

Commit Message

Adrien Grassein Nov. 29, 2020, 9:59 p.m. UTC
Add dt-bindings explanation for the two new gpios
(mdio and mdc) used for bitbanging.

Signed-off-by: Adrien Grassein <adrien.grassein@gmail.com>
---
 Documentation/devicetree/bindings/net/fsl-fec.txt | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Andrew Lunn Nov. 29, 2020, 11:04 p.m. UTC | #1
On Sun, Nov 29, 2020 at 11:51:43PM +0100, Adrien Grassein wrote:
> Hi Andrew,
> 
> Please find my answers below.
> 
> Le dim. 29 nov. 2020 à 23:41, Andrew Lunn <andrew@lunn.ch> a écrit :
> 
>     On Sun, Nov 29, 2020 at 10:59:58PM +0100, Adrien Grassein wrote:
>     > Add dt-bindings explanation for the two new gpios
>     > (mdio and mdc) used for bitbanging.
> 
>     Hi Adrien
> 
>     What is missing is an explanation of why!
> 
> I'm sorry, it's my first upstreaming attempt.

Hi Adrien

Please take a look at

https://www.kernel.org/doc/html/latest/networking/netdev-FAQ.html

It is normal to have a patch 0/X which explains the big picture.

Then the commit message for each patch should explain why you are
doing something. That is much more important than what you are doing,
i can see that from the patch itself.

> I am currently upstreaming the "Nitrogen 8m Mini board" that seems to not use a
> "normal" mdio bus but a "bitbanged" one with the fsl fec driver.

Any idea why?

Anyway, you should not replicate code, don't copy bitbanging code into
the FEC. Just use the existing bit-banger MDIO bus master driver.

	   Andrew
Florian Fainelli Nov. 30, 2020, 10:08 p.m. UTC | #2
On 11/29/2020 3:04 PM, Andrew Lunn wrote:
> On Sun, Nov 29, 2020 at 11:51:43PM +0100, Adrien Grassein wrote:
>> Hi Andrew,
>>
>> Please find my answers below.
>>
>> Le dim. 29 nov. 2020 à 23:41, Andrew Lunn <andrew@lunn.ch> a écrit :
>>
>>     On Sun, Nov 29, 2020 at 10:59:58PM +0100, Adrien Grassein wrote:
>>     > Add dt-bindings explanation for the two new gpios
>>     > (mdio and mdc) used for bitbanging.
>>
>>     Hi Adrien
>>
>>     What is missing is an explanation of why!
>>
>> I'm sorry, it's my first upstreaming attempt.
> 
> Hi Adrien
> 
> Please take a look at
> 
> https://www.kernel.org/doc/html/latest/networking/netdev-FAQ.html
> 
> It is normal to have a patch 0/X which explains the big picture.
> 
> Then the commit message for each patch should explain why you are
> doing something. That is much more important than what you are doing,
> i can see that from the patch itself.
> 
>> I am currently upstreaming the "Nitrogen 8m Mini board" that seems to not use a
>> "normal" mdio bus but a "bitbanged" one with the fsl fec driver.
> 
> Any idea why?
> 
> Anyway, you should not replicate code, don't copy bitbanging code into
> the FEC. Just use the existing bit-banger MDIO bus master driver.

Right there should be no need for you to modify the FEC driver at all,
there is an existing generic bitbanged MDIO bus driver here:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/mdio/mdio-gpio.c

with its binding here:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/mdio-gpio.txt

so all you should need to do is make sure that you place a
"virtual,mdio-gpio" node, declare the PHY devices that are present on
that bus, and have your FEC nodes point to those PHY devices with an
appropriate 'phy-handle' property.
Andrew Lunn Nov. 30, 2020, 10:26 p.m. UTC | #3
> >> I am currently upstreaming the "Nitrogen 8m Mini board" that seems to not use a
> >> "normal" mdio bus but a "bitbanged" one with the fsl fec driver.
> > 
> > Any idea why?
> > 
> > Anyway, you should not replicate code, don't copy bitbanging code into
> > the FEC. Just use the existing bit-banger MDIO bus master driver.
> 
> Right there should be no need for you to modify the FEC driver at all,
> there is an existing generic bitbanged MDIO bus driver here:

Hi Florian

Speculation on my part, until i hear back on the Why? question, but
i'm guessing the board has a wrong pullup on the MDIO line. It takes
too long for the PHY/FEC to pull the line low at the default
2.5MHz. bit-banging is much slower, so it works.

If i'm right, there is a much simpler fix for this. Use the
clock-frequency property for the MDIO bus to slow the clock down.

	Andrew
Adrien Grassein Dec. 4, 2020, 8:09 p.m. UTC | #4
Hi all,

I'm not a kernel expert, but I try to find why these patches were
needed in the FSL/Boundary kernel.

I found that the pin muxing in the original kernel was not good for FEC.
It's now working well with the mainline code.

You can delete these patches.

Thanks a lot,
Regards,

Adrien

Le lun. 30 nov. 2020 à 23:26, Andrew Lunn <andrew@lunn.ch> a écrit :
>
> > >> I am currently upstreaming the "Nitrogen 8m Mini board" that seems to not use a
> > >> "normal" mdio bus but a "bitbanged" one with the fsl fec driver.
> > >
> > > Any idea why?
> > >
> > > Anyway, you should not replicate code, don't copy bitbanging code into
> > > the FEC. Just use the existing bit-banger MDIO bus master driver.
> >
> > Right there should be no need for you to modify the FEC driver at all,
> > there is an existing generic bitbanged MDIO bus driver here:
>
> Hi Florian
>
> Speculation on my part, until i hear back on the Why? question, but
> i'm guessing the board has a wrong pullup on the MDIO line. It takes
> too long for the PHY/FEC to pull the line low at the default
> 2.5MHz. bit-banging is much slower, so it works.
>
> If i'm right, there is a much simpler fix for this. Use the
> clock-frequency property for the MDIO bus to slow the clock down.
>
>         Andrew
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
index 9b543789cd52..e9fa992354b7 100644
--- a/Documentation/devicetree/bindings/net/fsl-fec.txt
+++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
@@ -22,6 +22,10 @@  Optional properties:
 - fsl,err006687-workaround-present: If present indicates that the system has
   the hardware workaround for ERR006687 applied and does not need a software
   workaround.
+- mdc-gpios: Bitbanged MDIO Management Data Clock GPIO. If specified,
+mdio-gpios should be specified too.
+- mdio-gpios: Bitbanged MDIO Management Data I/O GPIO. If specified,
+mdc-gpios should be specified too.
 - fsl,stop-mode: register bits of stop mode control, the format is
 		 <&gpr req_gpr req_bit>.
 		 gpr is the phandle to general purpose register node.