[RESEND,net-next,v2,1/8] dt-bindings: net: dwmac-sun8i: Clean up clock delay chain descriptions

Commit Message

Chen-Yu Tsai May 13, 2018, 7:14 p.m. UTC

The clock delay chains found in the glue layer for dwmac-sun8i are only
used with RGMII PHYs. They are not intended for non-RGMII PHYs, such as
MII external PHYs or the internal PHY. Also, a recent SoC has a smaller
range of possible values for the delay chain.

This patch reformats the delay chain section of the device tree binding
to make it clear that the delay chains only apply to RGMII PHYs, and
make it easier to add the R40-specific bits later.

Signed-off-by: Chen-Yu Tsai <[e-mail]>
Reviewed-by: Rob Herring <[e-mail]>
Acked-by: Maxime Ripard <[e-mail]>
---
 Documentation/devicetree/bindings/net/dwmac-sun8i.txt | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Comments

Andrew Lunn May 13, 2018, 7:49 p.m. UTC | #1

On Mon, May 14, 2018 at 03:14:18AM +0800, Chen-Yu Tsai wrote:
> The clock delay chains found in the glue layer for dwmac-sun8i are only
> used with RGMII PHYs. They are not intended for non-RGMII PHYs, such as
> MII external PHYs or the internal PHY. Also, a recent SoC has a smaller
> range of possible values for the delay chain.
> 
> This patch reformats the delay chain section of the device tree binding
> to make it clear that the delay chains only apply to RGMII PHYs, and
> make it easier to add the R40-specific bits later.
> 
> Signed-off-by: Chen-Yu Tsai <[e-mail]>
> Reviewed-by: Rob Herring <[e-mail]>
> Acked-by: Maxime Ripard <[e-mail]>
> ---
>  Documentation/devicetree/bindings/net/dwmac-sun8i.txt | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/dwmac-sun8i.txt b/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
> index 3d6d5fa0c4d5..e04ce75e24a3 100644
> --- a/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
> +++ b/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
> @@ -28,10 +28,13 @@ Required properties:
>    - allwinner,sun8i-a83t-system-controller
>  
>  Optional properties:
> -- allwinner,tx-delay-ps: TX clock delay chain value in ps. Range value is 0-700. Default is 0)
> -- allwinner,rx-delay-ps: RX clock delay chain value in ps. Range value is 0-3100. Default is 0)
> -Both delay properties need to be a multiple of 100. They control the delay for
> -external PHY.
> +- allwinner,tx-delay-ps: TX clock delay chain value in ps.
> +			 Range is 0-700. Default is 0.
> +- allwinner,rx-delay-ps: RX clock delay chain value in ps.
> +			 Range is 0-3100. Default is 0.
> +Both delay properties need to be a multiple of 100. They control the
> +clock delay for external RGMII PHY. They do not apply to the internal
> +PHY or external non-RGMII PHYs.

Hi Chen-Yu

Are these delays the MAC applies? Not the PHY. It would be good to
make it clear here these are MAC imposed delays.

     Andrew

Chen-Yu Tsai May 13, 2018, 7:53 p.m. UTC | #2

On Sun, May 13, 2018 at 12:49 PM, Andrew Lunn <[e-mail]> wrote:
> On Mon, May 14, 2018 at 03:14:18AM +0800, Chen-Yu Tsai wrote:
>> The clock delay chains found in the glue layer for dwmac-sun8i are only
>> used with RGMII PHYs. They are not intended for non-RGMII PHYs, such as
>> MII external PHYs or the internal PHY. Also, a recent SoC has a smaller
>> range of possible values for the delay chain.
>>
>> This patch reformats the delay chain section of the device tree binding
>> to make it clear that the delay chains only apply to RGMII PHYs, and
>> make it easier to add the R40-specific bits later.
>>
>> Signed-off-by: Chen-Yu Tsai <[e-mail]>
>> Reviewed-by: Rob Herring <[e-mail]>
>> Acked-by: Maxime Ripard <[e-mail]>
>> ---
>>  Documentation/devicetree/bindings/net/dwmac-sun8i.txt | 11 +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/dwmac-sun8i.txt b/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
>> index 3d6d5fa0c4d5..e04ce75e24a3 100644
>> --- a/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
>> +++ b/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
>> @@ -28,10 +28,13 @@ Required properties:
>>    - allwinner,sun8i-a83t-system-controller
>>
>>  Optional properties:
>> -- allwinner,tx-delay-ps: TX clock delay chain value in ps. Range value is 0-700. Default is 0)
>> -- allwinner,rx-delay-ps: RX clock delay chain value in ps. Range value is 0-3100. Default is 0)
>> -Both delay properties need to be a multiple of 100. They control the delay for
>> -external PHY.
>> +- allwinner,tx-delay-ps: TX clock delay chain value in ps.
>> +                      Range is 0-700. Default is 0.
>> +- allwinner,rx-delay-ps: RX clock delay chain value in ps.
>> +                      Range is 0-3100. Default is 0.
>> +Both delay properties need to be a multiple of 100. They control the
>> +clock delay for external RGMII PHY. They do not apply to the internal
>> +PHY or external non-RGMII PHYs.
>
> Hi Chen-Yu
>
> Are these delays the MAC applies? Not the PHY. It would be good to
> make it clear here these are MAC imposed delays.

Yes these are applied on the MAC side. Being described in the device
tree bindings for the MAC, I thought this was implied to be the case?
Are there known exceptions?

Thanks
ChenYu

Andrew Lunn May 13, 2018, 8:05 p.m. UTC | #3

> > Hi Chen-Yu
> >
> > Are these delays the MAC applies? Not the PHY. It would be good to
> > make it clear here these are MAC imposed delays.
> 
> Yes these are applied on the MAC side. Being described in the device
> tree bindings for the MAC, I thought this was implied to be the case?
> Are there known exceptions?

There is frequent confusion with this. Most of the time, the PHY does
the delay, not the MAC, based on the phy-mode. So the MAC doing it is
an exception in itself.

Do you actually need these delays for the board you adding support
for? Does the PHY not support adding the needed delays? If you don't
need the delays, i would not even implement them.

    Andrew

Icenowy Zheng May 13, 2018, 8:10 p.m. UTC | #4

于 2018年5月14日 GMT+08:00 上午4:05:29, Andrew Lunn <[e-mail]> 写到:
>> > Hi Chen-Yu
>> >
>> > Are these delays the MAC applies? Not the PHY. It would be good to
>> > make it clear here these are MAC imposed delays.
>> 
>> Yes these are applied on the MAC side. Being described in the device
>> tree bindings for the MAC, I thought this was implied to be the case?
>> Are there known exceptions?
>
>There is frequent confusion with this. Most of the time, the PHY does
>the delay, not the MAC, based on the phy-mode. So the MAC doing it is
>an exception in itself.
>
>Do you actually need these delays for the board you adding support
>for? Does the PHY not support adding the needed delays? If you don't
>need the delays, i would not even implement them.

They are needed if the rx/tx have not equal length.

For several boards if the delay is not present, the network
is unusable. (I have at least met this on Banana Pi M3, an
Allwinner A83T SBC w/ RTL8211E)

>
>    Andrew
>
>_______________________________________________
>linux-arm-kernel mailing list
>[e-mail]
>http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Chen-Yu Tsai May 13, 2018, 8:11 p.m. UTC | #5

On Sun, May 13, 2018 at 1:05 PM, Andrew Lunn <[e-mail]> wrote:
>> > Hi Chen-Yu
>> >
>> > Are these delays the MAC applies? Not the PHY. It would be good to
>> > make it clear here these are MAC imposed delays.
>>
>> Yes these are applied on the MAC side. Being described in the device
>> tree bindings for the MAC, I thought this was implied to be the case?
>> Are there known exceptions?
>
> There is frequent confusion with this. Most of the time, the PHY does
> the delay, not the MAC, based on the phy-mode. So the MAC doing it is
> an exception in itself.
>
> Do you actually need these delays for the board you adding support
> for? Does the PHY not support adding the needed delays? If you don't
> need the delays, i would not even implement them.

Yes this is already used on the Bananapi M3. This patch merely reformats
the description and adds a note saying this only applies to RGMII mode.

ChenYu

Andrew Lunn May 13, 2018, 8:27 p.m. UTC | #6

On Mon, May 14, 2018 at 04:10:58AM +0800, Icenowy Zheng wrote:
> 
> 
> 于 2018年5月14日 GMT+08:00 上午4:05:29, Andrew Lunn <[e-mail]> 写到:
> >> > Hi Chen-Yu
> >> >
> >> > Are these delays the MAC applies? Not the PHY. It would be good to
> >> > make it clear here these are MAC imposed delays.
> >> 
> >> Yes these are applied on the MAC side. Being described in the device
> >> tree bindings for the MAC, I thought this was implied to be the case?
> >> Are there known exceptions?
> >
> >There is frequent confusion with this. Most of the time, the PHY does
> >the delay, not the MAC, based on the phy-mode. So the MAC doing it is
> >an exception in itself.
> >
> >Do you actually need these delays for the board you adding support
> >for? Does the PHY not support adding the needed delays? If you don't
> >need the delays, i would not even implement them.
> 
> They are needed if the rx/tx have not equal length.
> 

> For several boards if the delay is not present, the network
> is unusable. (I have at least met this on Banana Pi M3, an
> Allwinner A83T SBC w/ RTL8211E)

The RTL8211E does not appear to support adding delays, via
registers. You can do it with strapping resistors. The RTL8211F does
support delays via a register. So for the Banana Pi M3 it is
justified.

However, in general, i would recommend letting the PHY do it, if it
can.

	Andrew

Andrew Lunn May 13, 2018, 8:29 p.m. UTC | #7

On Sun, May 13, 2018 at 01:11:08PM -0700, Chen-Yu Tsai wrote:
> On Sun, May 13, 2018 at 1:05 PM, Andrew Lunn <[e-mail]> wrote:
> >> > Hi Chen-Yu
> >> >
> >> > Are these delays the MAC applies? Not the PHY. It would be good to
> >> > make it clear here these are MAC imposed delays.
> >>
> >> Yes these are applied on the MAC side. Being described in the device
> >> tree bindings for the MAC, I thought this was implied to be the case?
> >> Are there known exceptions?
> >
> > There is frequent confusion with this. Most of the time, the PHY does
> > the delay, not the MAC, based on the phy-mode. So the MAC doing it is
> > an exception in itself.
> >
> > Do you actually need these delays for the board you adding support
> > for? Does the PHY not support adding the needed delays? If you don't
> > need the delays, i would not even implement them.
> 
> Yes this is already used on the Bananapi M3. This patch merely reformats
> the description and adds a note saying this only applies to RGMII mode.

Yes, the current code is needed for the Bananapi M3. But you have
another patch which extends the code to support a smaller range. Do
you have a board which actually needs this? If not, i would not add
that new code.

     Andrew

Chen-Yu Tsai May 14, 2018, 4:59 a.m. UTC | #8

On Sun, May 13, 2018 at 1:29 PM, Andrew Lunn <[e-mail]> wrote:
> On Sun, May 13, 2018 at 01:11:08PM -0700, Chen-Yu Tsai wrote:
>> On Sun, May 13, 2018 at 1:05 PM, Andrew Lunn <[e-mail]> wrote:
>> >> > Hi Chen-Yu
>> >> >
>> >> > Are these delays the MAC applies? Not the PHY. It would be good to
>> >> > make it clear here these are MAC imposed delays.
>> >>
>> >> Yes these are applied on the MAC side. Being described in the device
>> >> tree bindings for the MAC, I thought this was implied to be the case?
>> >> Are there known exceptions?
>> >
>> > There is frequent confusion with this. Most of the time, the PHY does
>> > the delay, not the MAC, based on the phy-mode. So the MAC doing it is
>> > an exception in itself.
>> >
>> > Do you actually need these delays for the board you adding support
>> > for? Does the PHY not support adding the needed delays? If you don't
>> > need the delays, i would not even implement them.
>>
>> Yes this is already used on the Bananapi M3. This patch merely reformats
>> the description and adds a note saying this only applies to RGMII mode.
>
> Yes, the current code is needed for the Bananapi M3. But you have
> another patch which extends the code to support a smaller range. Do
> you have a board which actually needs this? If not, i would not add
> that new code.

IIRC the delay on the PHY side is either 2ns or none. The delay on the
MAC side here is an order smaller, likely fine tuning to cope with board
design deficiencies.

Currently no other board requires this, but this is already part of the
binding. The new stuff limits the range for a specific SoC, simply because
that is the range supported by the control register. Not implementing, i.e.
supporting the whole range from the property, which might get truncated,
doesn't make much sense to me.

Regards
ChenYu

Icenowy Zheng May 14, 2018, 5:18 a.m. UTC | #9

于 2018年5月14日 GMT+08:00 下午12:59:22, Chen-Yu Tsai <[e-mail]> 写到:
>On Sun, May 13, 2018 at 1:29 PM, Andrew Lunn <[e-mail]> wrote:
>> On Sun, May 13, 2018 at 01:11:08PM -0700, Chen-Yu Tsai wrote:
>>> On Sun, May 13, 2018 at 1:05 PM, Andrew Lunn <[e-mail]> wrote:
>>> >> > Hi Chen-Yu
>>> >> >
>>> >> > Are these delays the MAC applies? Not the PHY. It would be good
>to
>>> >> > make it clear here these are MAC imposed delays.
>>> >>
>>> >> Yes these are applied on the MAC side. Being described in the
>device
>>> >> tree bindings for the MAC, I thought this was implied to be the
>case?
>>> >> Are there known exceptions?
>>> >
>>> > There is frequent confusion with this. Most of the time, the PHY
>does
>>> > the delay, not the MAC, based on the phy-mode. So the MAC doing it
>is
>>> > an exception in itself.
>>> >
>>> > Do you actually need these delays for the board you adding support
>>> > for? Does the PHY not support adding the needed delays? If you
>don't
>>> > need the delays, i would not even implement them.
>>>
>>> Yes this is already used on the Bananapi M3. This patch merely
>reformats
>>> the description and adds a note saying this only applies to RGMII
>mode.
>>
>> Yes, the current code is needed for the Bananapi M3. But you have
>> another patch which extends the code to support a smaller range. Do
>> you have a board which actually needs this? If not, i would not add
>> that new code.
>
>IIRC the delay on the PHY side is either 2ns or none. The delay on the
>MAC side here is an order smaller, likely fine tuning to cope with
>board
>design deficiencies.

And the weird thing is that the delay fails to work on some batches
of RTL8211E, notably many cases are shown on Pine A64+
board.

P.S. The delay is intended to be set via wires on PCB, other than
by software. Although Realtek provided some magic numbers
to Pine64, in order to fix the network problem due to PHY
delay failure.

>
>Currently no other board requires this, but this is already part of the
>binding. The new stuff limits the range for a specific SoC, simply
>because
>that is the range supported by the control register. Not implementing,
>i.e.
>supporting the whole range from the property, which might get
>truncated,
>doesn't make much sense to me.
>
>Regards
>ChenYu
>
>_______________________________________________
>linux-arm-kernel mailing list
>[e-mail]
>http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Maxime Ripard May 14, 2018, 7:21 a.m. UTC | #10

On Sun, May 13, 2018 at 09:59:22PM -0700, Chen-Yu Tsai wrote:
> On Sun, May 13, 2018 at 1:29 PM, Andrew Lunn <[e-mail]> wrote:
> > On Sun, May 13, 2018 at 01:11:08PM -0700, Chen-Yu Tsai wrote:
> >> On Sun, May 13, 2018 at 1:05 PM, Andrew Lunn <[e-mail]> wrote:
> >> >> > Hi Chen-Yu
> >> >> >
> >> >> > Are these delays the MAC applies? Not the PHY. It would be good to
> >> >> > make it clear here these are MAC imposed delays.
> >> >>
> >> >> Yes these are applied on the MAC side. Being described in the device
> >> >> tree bindings for the MAC, I thought this was implied to be the case?
> >> >> Are there known exceptions?
> >> >
> >> > There is frequent confusion with this. Most of the time, the PHY does
> >> > the delay, not the MAC, based on the phy-mode. So the MAC doing it is
> >> > an exception in itself.
> >> >
> >> > Do you actually need these delays for the board you adding support
> >> > for? Does the PHY not support adding the needed delays? If you don't
> >> > need the delays, i would not even implement them.
> >>
> >> Yes this is already used on the Bananapi M3. This patch merely reformats
> >> the description and adds a note saying this only applies to RGMII mode.
> >
> > Yes, the current code is needed for the Bananapi M3. But you have
> > another patch which extends the code to support a smaller range. Do
> > you have a board which actually needs this? If not, i would not add
> > that new code.
> 
> IIRC the delay on the PHY side is either 2ns or none. The delay on the
> MAC side here is an order smaller, likely fine tuning to cope with board
> design deficiencies.
> 
> Currently no other board requires this, but this is already part of the
> binding. The new stuff limits the range for a specific SoC, simply because
> that is the range supported by the control register. Not implementing, i.e.
> supporting the whole range from the property, which might get truncated,
> doesn't make much sense to me.

With that driver we don't, but the previous design had the same
feature and it was used on more boards. It was simply initialized
statically in U-Boot, and not in Linux through the DT like it is done
here.

Maxime

Andrew Lunn May 14, 2018, 12:03 p.m. UTC | #11

> > Currently no other board requires this, but this is already part of the
> > binding. The new stuff limits the range for a specific SoC, simply because
> > that is the range supported by the control register. Not implementing, i.e.
> > supporting the whole range from the property, which might get truncated,
> > doesn't make much sense to me.
> 
> With that driver we don't, but the previous design had the same
> feature and it was used on more boards. It was simply initialized
> statically in U-Boot, and not in Linux through the DT like it is done
> here.

If there are boards which do need it, then i'm fine with it. I would
also prefer that Linux does set it, and don't rely on U-boot.

   Andrew

Patch

diff --git a/Documentation/devicetree/bindings/net/dwmac-sun8i.txt b/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
index 3d6d5fa0c4d5..e04ce75e24a3 100644
--- a/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
+++ b/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
@@ -28,10 +28,13 @@  Required properties:
   - allwinner,sun8i-a83t-system-controller
 
 Optional properties:
-- allwinner,tx-delay-ps: TX clock delay chain value in ps. Range value is 0-700. Default is 0)
-- allwinner,rx-delay-ps: RX clock delay chain value in ps. Range value is 0-3100. Default is 0)
-Both delay properties need to be a multiple of 100. They control the delay for
-external PHY.
+- allwinner,tx-delay-ps: TX clock delay chain value in ps.
+			 Range is 0-700. Default is 0.
+- allwinner,rx-delay-ps: RX clock delay chain value in ps.
+			 Range is 0-3100. Default is 0.
+Both delay properties need to be a multiple of 100. They control the
+clock delay for external RGMII PHY. They do not apply to the internal
+PHY or external non-RGMII PHYs.
 
 Optional properties for the following compatibles:
   - "allwinner,sun8i-h3-emac",