Re: [PATCH v3 05/11] net: stmmac: dwmac-rk: Add internal phy support

[David.Wu]

Hi Chen-Yu,

在 2017/8/10 10:40, Chen-Yu Tsai 写道:

Hi David,

On Wed, Aug 9, 2017 at 5:38 PM, David.Wu  wrote:

Hello Corentin, Chen-Yu


在 2017/8/9 16:45, Corentin Labbe 写道:


On Thu, Aug 03, 2017 at 07:06:33PM +0800, Chen-Yu Tsai wrote:


On Thu, Aug 3, 2017 at 1:38 AM, Florian Fainelli 
wrote:


On 08/01/2017 11:21 PM, David Wu wrote:


To make internal phy work, need to configure the phy_clock,
phy cru_reset and related registers.

Signed-off-by: David Wu 
---
   .../devicetree/bindings/net/rockchip-dwmac.txt |  6 +-
   drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 81
++
   2 files changed, 86 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
b/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
index 8f42755..ec39b31 100644
--- a/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
+++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
@@ -25,7 +25,8 @@ Required properties:
- clock-names: One name for each entry in the clocks property.
- phy-mode: See ethernet.txt file in the same directory.
- pinctrl-names: Names corresponding to the numbered pinctrl states.
- - pinctrl-0: pin-control mode. can be <_pins> or <_pins>.
+ - pinctrl-0: pin-control mode. can be <_pins>, <_pins> or
led pins
+   for internal phy mode.
- clock_in_out: For RGMII, it must be "input", means main
clock(125MHz)
  is not sourced from SoC's PLL, but input from PHY; For RMII,
"input" means
  PHY provides the reference clock(50MHz), "output" means GMAC
provides the
@@ -40,6 +41,9 @@ Optional properties:
- tx_delay: Delay value for TXD timing. Range value is 0~0x7F, 0x30
as default.
- rx_delay: Delay value for RXD timing. Range value is 0~0x7F, 0x10
as default.
- phy-supply: phandle to a regulator if the PHY needs one
+ - clocks: < MAC_PHY>: Clock selector for internal macphy
+ - phy-is-internal: A boolean property allows us to know that MAC will
connect to
+   internal phy.



This is incorrect in two ways:

- this is a property of the PHY, so it should be documented as such in
Documentation/devicetree/bindings/net/phy.txt so other bindings can
re-use it

- if it was specific to your MAC you would expect a vendor prefix to
this property, which is not there

An alternative way to implement the external/internal logic selection
would be mandate that your Ethernet PHY node have a compatible string
like this:

phy@0 {
  compatible = "ethernet-phy-id1234.d400",
"ethernet-phy-802.3-c22";
};

Then you don't need this phy-is-internal property, because you can
locate the PHY node by the phy-handle (see more about that in a reply to
patch 10) and you can determine ahead of time whether this PHY is
internal or not based on its compatible string.



This doesn't really work for us (sunxi). The "internal" PHY of the H3
is also available in the X-Powers AC200 combo chip, which would be
external. Same ID. So if someone were to be stupid and put the two
together, you wouldn't know which one it actually is. Generically
it doesn't make sense to match against the ID either. The PHY is
only integrated or inlined into the SoC, meaning it could be moved
elsewhere or even be a discreet part.

The way I see it is more like a reversed pinmux. The system should
select either the internal set or external set of xMII pins to use.

A phy-is-internal property in the PHY node would work for us. We
already need a PHY node to describe its clocks and resets.

A side note to this solution is that, since the internal PHY is defined
at the .dtsi level, any external PHYs at the same address would need to
make sure to delete the property, which is kind of counterintuitive, but
it is how device tree works. One would want to put the internal PHY's
address, assuming it is configurable, on something that is rarely used.



Hello David, Florian, Andrew

Could someone ack this ? or nack if you think that the chance for having
two PHY id both internal and external is too low.
Anyway, we need a choice.



I think we should be specific to the situation, for us we have the
possibility that the Mac only picks up internal PHY, so this can be fixed at
the. DTSi level, also possible INTERNL PHY's Mac can also be used to connect
external PHY, while cutting off the connection with the internal PHY, so we
should define the internal PHY at the DTS level, so I think Florian's
approach is acceptable.


So it looks like you have the clock for the internal/integrated PHY at the
MAC level. I think this lets you define/add the PHY at the board level more
easily without a lot of duplication?

Sunxi has the clock and reset in the PHY node, which is defined in the dtsi
file. (This part is already done.)


Okay, that is better difined in the PHY node, i'll move them later.



ChenYu

Re: [PATCH v3 05/11] net: stmmac: dwmac-rk: Add internal phy support

[Chen-Yu Tsai]

Hi David,

On Wed, Aug 9, 2017 at 5:38 PM, David.Wu  wrote:
> Hello Corentin, Chen-Yu
>
>
> 在 2017/8/9 16:45, Corentin Labbe 写道:
>>
>> On Thu, Aug 03, 2017 at 07:06:33PM +0800, Chen-Yu Tsai wrote:
>>>
>>> On Thu, Aug 3, 2017 at 1:38 AM, Florian Fainelli 
>>> wrote:

 On 08/01/2017 11:21 PM, David Wu wrote:
>
> To make internal phy work, need to configure the phy_clock,
> phy cru_reset and related registers.
>
> Signed-off-by: David Wu 
> ---
>   .../devicetree/bindings/net/rockchip-dwmac.txt |  6 +-
>   drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 81
> ++
>   2 files changed, 86 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
> b/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
> index 8f42755..ec39b31 100644
> --- a/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
> +++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
> @@ -25,7 +25,8 @@ Required properties:
>- clock-names: One name for each entry in the clocks property.
>- phy-mode: See ethernet.txt file in the same directory.
>- pinctrl-names: Names corresponding to the numbered pinctrl states.
> - - pinctrl-0: pin-control mode. can be <_pins> or <_pins>.
> + - pinctrl-0: pin-control mode. can be <_pins>, <_pins> or
> led pins
> +   for internal phy mode.
>- clock_in_out: For RGMII, it must be "input", means main
> clock(125MHz)
>  is not sourced from SoC's PLL, but input from PHY; For RMII,
> "input" means
>  PHY provides the reference clock(50MHz), "output" means GMAC
> provides the
> @@ -40,6 +41,9 @@ Optional properties:
>- tx_delay: Delay value for TXD timing. Range value is 0~0x7F, 0x30
> as default.
>- rx_delay: Delay value for RXD timing. Range value is 0~0x7F, 0x10
> as default.
>- phy-supply: phandle to a regulator if the PHY needs one
> + - clocks: < MAC_PHY>: Clock selector for internal macphy
> + - phy-is-internal: A boolean property allows us to know that MAC will
> connect to
> +   internal phy.


 This is incorrect in two ways:

 - this is a property of the PHY, so it should be documented as such in
 Documentation/devicetree/bindings/net/phy.txt so other bindings can
 re-use it

 - if it was specific to your MAC you would expect a vendor prefix to
 this property, which is not there

 An alternative way to implement the external/internal logic selection
 would be mandate that your Ethernet PHY node have a compatible string
 like this:

 phy@0 {
  compatible = "ethernet-phy-id1234.d400",
 "ethernet-phy-802.3-c22";
 };

 Then you don't need this phy-is-internal property, because you can
 locate the PHY node by the phy-handle (see more about that in a reply to
 patch 10) and you can determine ahead of time whether this PHY is
 internal or not based on its compatible string.
>>>
>>>
>>> This doesn't really work for us (sunxi). The "internal" PHY of the H3
>>> is also available in the X-Powers AC200 combo chip, which would be
>>> external. Same ID. So if someone were to be stupid and put the two
>>> together, you wouldn't know which one it actually is. Generically
>>> it doesn't make sense to match against the ID either. The PHY is
>>> only integrated or inlined into the SoC, meaning it could be moved
>>> elsewhere or even be a discreet part.
>>>
>>> The way I see it is more like a reversed pinmux. The system should
>>> select either the internal set or external set of xMII pins to use.
>>>
>>> A phy-is-internal property in the PHY node would work for us. We
>>> already need a PHY node to describe its clocks and resets.
>>>
>>> A side note to this solution is that, since the internal PHY is defined
>>> at the .dtsi level, any external PHYs at the same address would need to
>>> make sure to delete the property, which is kind of counterintuitive, but
>>> it is how device tree works. One would want to put the internal PHY's
>>> address, assuming it is configurable, on something that is rarely used.
>>>
>>
>> Hello David, Florian, Andrew
>>
>> Could someone ack this ? or nack if you think that the chance for having
>> two PHY id both internal and external is too low.
>> Anyway, we need a choice.
>>
>
> I think we should be specific to the situation, for us we have the
> possibility that the Mac only picks up internal PHY, so this can be fixed at
> the. DTSi level, also possible INTERNL PHY's Mac can also be used to connect
> external PHY, while cutting off the connection with the internal PHY, so we
> should define the internal PHY at the DTS level, so I think Florian's
> approach is acceptable.

So it looks like you have the clock for the internal/integrated PHY 

Re: [PATCH v3 05/11] net: stmmac: dwmac-rk: Add internal phy support

[Florian Fainelli]

On August 9, 2017 1:45:41 AM PDT, Corentin Labbe  
wrote:
>On Thu, Aug 03, 2017 at 07:06:33PM +0800, Chen-Yu Tsai wrote:
>> On Thu, Aug 3, 2017 at 1:38 AM, Florian Fainelli
> wrote:
>> > On 08/01/2017 11:21 PM, David Wu wrote:
>> >> To make internal phy work, need to configure the phy_clock,
>> >> phy cru_reset and related registers.
>> >>
>> >> Signed-off-by: David Wu 
>> >> ---
>> >>  .../devicetree/bindings/net/rockchip-dwmac.txt |  6 +-
>> >>  drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 81
>++
>> >>  2 files changed, 86 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git
>a/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
>b/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
>> >> index 8f42755..ec39b31 100644
>> >> --- a/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
>> >> +++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
>> >> @@ -25,7 +25,8 @@ Required properties:
>> >>   - clock-names: One name for each entry in the clocks property.
>> >>   - phy-mode: See ethernet.txt file in the same directory.
>> >>   - pinctrl-names: Names corresponding to the numbered pinctrl
>states.
>> >> - - pinctrl-0: pin-control mode. can be <_pins> or
><_pins>.
>> >> + - pinctrl-0: pin-control mode. can be <_pins>,
><_pins> or led pins
>> >> +   for internal phy mode.
>> >>   - clock_in_out: For RGMII, it must be "input", means main
>clock(125MHz)
>> >> is not sourced from SoC's PLL, but input from PHY; For RMII,
>"input" means
>> >> PHY provides the reference clock(50MHz), "output" means GMAC
>provides the
>> >> @@ -40,6 +41,9 @@ Optional properties:
>> >>   - tx_delay: Delay value for TXD timing. Range value is 0~0x7F,
>0x30 as default.
>> >>   - rx_delay: Delay value for RXD timing. Range value is 0~0x7F,
>0x10 as default.
>> >>   - phy-supply: phandle to a regulator if the PHY needs one
>> >> + - clocks: < MAC_PHY>: Clock selector for internal macphy
>> >> + - phy-is-internal: A boolean property allows us to know that MAC
>will connect to
>> >> +   internal phy.
>> >
>> > This is incorrect in two ways:
>> >
>> > - this is a property of the PHY, so it should be documented as such
>in
>> > Documentation/devicetree/bindings/net/phy.txt so other bindings can
>> > re-use it
>> >
>> > - if it was specific to your MAC you would expect a vendor prefix
>to
>> > this property, which is not there
>> >
>> > An alternative way to implement the external/internal logic
>selection
>> > would be mandate that your Ethernet PHY node have a compatible
>string
>> > like this:
>> >
>> > phy@0 {
>> > compatible = "ethernet-phy-id1234.d400",
>"ethernet-phy-802.3-c22";
>> > };
>> >
>> > Then you don't need this phy-is-internal property, because you can
>> > locate the PHY node by the phy-handle (see more about that in a
>reply to
>> > patch 10) and you can determine ahead of time whether this PHY is
>> > internal or not based on its compatible string.
>> 
>> This doesn't really work for us (sunxi). The "internal" PHY of the H3
>> is also available in the X-Powers AC200 combo chip, which would be
>> external. Same ID. So if someone were to be stupid and put the two
>> together, you wouldn't know which one it actually is. Generically
>> it doesn't make sense to match against the ID either. The PHY is
>> only integrated or inlined into the SoC, meaning it could be moved
>> elsewhere or even be a discreet part.

It actually makes a lot of sense to differentiate on the PHY ID because you are 
supposed to allocate an unique ID based on how the integration of the PHY is 
done. Not doing that is making a sloppy job at integrating HW blocks, but such 
is life and there is no shortage of creativity amongst HW engineers when they 
are not given feedback from SW people.

>> 
>> The way I see it is more like a reversed pinmux. The system should
>> select either the internal set or external set of xMII pins to use.
>> 
>> A phy-is-internal property in the PHY node would work for us. We
>> already need a PHY node to describe its clocks and resets.
>> 
>> A side note to this solution is that, since the internal PHY is
>defined
>> at the .dtsi level, any external PHYs at the same address would need
>to
>> make sure to delete the property, which is kind of counterintuitive,
>but
>> it is how device tree works. One would want to put the internal PHY's
>> address, assuming it is configurable, on something that is rarely
>used.
>> 
>
>Hello David, Florian, Andrew
>
>Could someone ack this ? or nack if you think that the chance for
>having two PHY id both internal and external is too low.
>Anyway, we need a choice.

Let's move forward with the 'phy-is-integrated' Boolean property 
('phy-is-internal' is too close from the proprietary "internal" phy-mode IMHO). 
Andrew, does that also work for you?

-- 
Florian

Re: [PATCH v3 05/11] net: stmmac: dwmac-rk: Add internal phy support

[David.Wu]

Hello Corentin, Chen-Yu

在 2017/8/9 16:45, Corentin Labbe 写道:

On Thu, Aug 03, 2017 at 07:06:33PM +0800, Chen-Yu Tsai wrote:

On Thu, Aug 3, 2017 at 1:38 AM, Florian Fainelli  wrote:

On 08/01/2017 11:21 PM, David Wu wrote:

To make internal phy work, need to configure the phy_clock,
phy cru_reset and related registers.

Signed-off-by: David Wu 
---
  .../devicetree/bindings/net/rockchip-dwmac.txt |  6 +-
  drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 81 ++
  2 files changed, 86 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.txt 
b/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
index 8f42755..ec39b31 100644
--- a/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
+++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
@@ -25,7 +25,8 @@ Required properties:
   - clock-names: One name for each entry in the clocks property.
   - phy-mode: See ethernet.txt file in the same directory.
   - pinctrl-names: Names corresponding to the numbered pinctrl states.
- - pinctrl-0: pin-control mode. can be <_pins> or <_pins>.
+ - pinctrl-0: pin-control mode. can be <_pins>, <_pins> or led pins
+   for internal phy mode.
   - clock_in_out: For RGMII, it must be "input", means main clock(125MHz)
 is not sourced from SoC's PLL, but input from PHY; For RMII, "input" means
 PHY provides the reference clock(50MHz), "output" means GMAC provides the
@@ -40,6 +41,9 @@ Optional properties:
   - tx_delay: Delay value for TXD timing. Range value is 0~0x7F, 0x30 as 
default.
   - rx_delay: Delay value for RXD timing. Range value is 0~0x7F, 0x10 as 
default.
   - phy-supply: phandle to a regulator if the PHY needs one
+ - clocks: < MAC_PHY>: Clock selector for internal macphy
+ - phy-is-internal: A boolean property allows us to know that MAC will connect 
to
+   internal phy.


This is incorrect in two ways:

- this is a property of the PHY, so it should be documented as such in
Documentation/devicetree/bindings/net/phy.txt so other bindings can
re-use it

- if it was specific to your MAC you would expect a vendor prefix to
this property, which is not there

An alternative way to implement the external/internal logic selection
would be mandate that your Ethernet PHY node have a compatible string
like this:

phy@0 {
 compatible = "ethernet-phy-id1234.d400", "ethernet-phy-802.3-c22";
};

Then you don't need this phy-is-internal property, because you can
locate the PHY node by the phy-handle (see more about that in a reply to
patch 10) and you can determine ahead of time whether this PHY is
internal or not based on its compatible string.


This doesn't really work for us (sunxi). The "internal" PHY of the H3
is also available in the X-Powers AC200 combo chip, which would be
external. Same ID. So if someone were to be stupid and put the two
together, you wouldn't know which one it actually is. Generically
it doesn't make sense to match against the ID either. The PHY is
only integrated or inlined into the SoC, meaning it could be moved
elsewhere or even be a discreet part.

The way I see it is more like a reversed pinmux. The system should
select either the internal set or external set of xMII pins to use.

A phy-is-internal property in the PHY node would work for us. We
already need a PHY node to describe its clocks and resets.

A side note to this solution is that, since the internal PHY is defined
at the .dtsi level, any external PHYs at the same address would need to
make sure to delete the property, which is kind of counterintuitive, but
it is how device tree works. One would want to put the internal PHY's
address, assuming it is configurable, on something that is rarely used.



Hello David, Florian, Andrew

Could someone ack this ? or nack if you think that the chance for having two 
PHY id both internal and external is too low.
Anyway, we need a choice.



I think we should be specific to the situation, for us we have the 
possibility that the Mac only picks up internal PHY, so this can be 
fixed at the. DTSi level, also possible INTERNL PHY's Mac can also be 
used to connect external PHY, while cutting off the connection with the 
internal PHY, so we should define the internal PHY at the DTS level, so 
I think Florian's approach is acceptable.



Regards

Re: [PATCH v3 05/11] net: stmmac: dwmac-rk: Add internal phy support

[Corentin Labbe]

On Thu, Aug 03, 2017 at 07:06:33PM +0800, Chen-Yu Tsai wrote:
> On Thu, Aug 3, 2017 at 1:38 AM, Florian Fainelli  wrote:
> > On 08/01/2017 11:21 PM, David Wu wrote:
> >> To make internal phy work, need to configure the phy_clock,
> >> phy cru_reset and related registers.
> >>
> >> Signed-off-by: David Wu 
> >> ---
> >>  .../devicetree/bindings/net/rockchip-dwmac.txt |  6 +-
> >>  drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 81 
> >> ++
> >>  2 files changed, 86 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.txt 
> >> b/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
> >> index 8f42755..ec39b31 100644
> >> --- a/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
> >> +++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
> >> @@ -25,7 +25,8 @@ Required properties:
> >>   - clock-names: One name for each entry in the clocks property.
> >>   - phy-mode: See ethernet.txt file in the same directory.
> >>   - pinctrl-names: Names corresponding to the numbered pinctrl states.
> >> - - pinctrl-0: pin-control mode. can be <_pins> or <_pins>.
> >> + - pinctrl-0: pin-control mode. can be <_pins>, <_pins> or led 
> >> pins
> >> +   for internal phy mode.
> >>   - clock_in_out: For RGMII, it must be "input", means main clock(125MHz)
> >> is not sourced from SoC's PLL, but input from PHY; For RMII, "input" 
> >> means
> >> PHY provides the reference clock(50MHz), "output" means GMAC provides 
> >> the
> >> @@ -40,6 +41,9 @@ Optional properties:
> >>   - tx_delay: Delay value for TXD timing. Range value is 0~0x7F, 0x30 as 
> >> default.
> >>   - rx_delay: Delay value for RXD timing. Range value is 0~0x7F, 0x10 as 
> >> default.
> >>   - phy-supply: phandle to a regulator if the PHY needs one
> >> + - clocks: < MAC_PHY>: Clock selector for internal macphy
> >> + - phy-is-internal: A boolean property allows us to know that MAC will 
> >> connect to
> >> +   internal phy.
> >
> > This is incorrect in two ways:
> >
> > - this is a property of the PHY, so it should be documented as such in
> > Documentation/devicetree/bindings/net/phy.txt so other bindings can
> > re-use it
> >
> > - if it was specific to your MAC you would expect a vendor prefix to
> > this property, which is not there
> >
> > An alternative way to implement the external/internal logic selection
> > would be mandate that your Ethernet PHY node have a compatible string
> > like this:
> >
> > phy@0 {
> > compatible = "ethernet-phy-id1234.d400", "ethernet-phy-802.3-c22";
> > };
> >
> > Then you don't need this phy-is-internal property, because you can
> > locate the PHY node by the phy-handle (see more about that in a reply to
> > patch 10) and you can determine ahead of time whether this PHY is
> > internal or not based on its compatible string.
> 
> This doesn't really work for us (sunxi). The "internal" PHY of the H3
> is also available in the X-Powers AC200 combo chip, which would be
> external. Same ID. So if someone were to be stupid and put the two
> together, you wouldn't know which one it actually is. Generically
> it doesn't make sense to match against the ID either. The PHY is
> only integrated or inlined into the SoC, meaning it could be moved
> elsewhere or even be a discreet part.
> 
> The way I see it is more like a reversed pinmux. The system should
> select either the internal set or external set of xMII pins to use.
> 
> A phy-is-internal property in the PHY node would work for us. We
> already need a PHY node to describe its clocks and resets.
> 
> A side note to this solution is that, since the internal PHY is defined
> at the .dtsi level, any external PHYs at the same address would need to
> make sure to delete the property, which is kind of counterintuitive, but
> it is how device tree works. One would want to put the internal PHY's
> address, assuming it is configurable, on something that is rarely used.
> 

Hello David, Florian, Andrew

Could someone ack this ? or nack if you think that the chance for having two 
PHY id both internal and external is too low.
Anyway, we need a choice.

Regards

Re: [PATCH v3 05/11] net: stmmac: dwmac-rk: Add internal phy support

[David.Wu]

Hi Florian & ChenYu

在 2017/8/3 1:38, Florian Fainelli 写道:

This is incorrect in two ways:

- this is a property of the PHY, so it should be documented as such in
Documentation/devicetree/bindings/net/phy.txt so other bindings can
re-use it

- if it was specific to your MAC you would expect a vendor prefix to
this property, which is not there

An alternative way to implement the external/internal logic selection
would be mandate that your Ethernet PHY node have a compatible string
like this:

phy@0 {
compatible = "ethernet-phy-id1234.d400", "ethernet-phy-802.3-c22";
};

Then you don't need this phy-is-internal property, because you can
locate the PHY node by the phy-handle (see more about that in a reply to
patch 10) and you can determine ahead of time whether this PHY is
internal or not based on its compatible string.


We may implement a read_bool_property after parsed phy phandle at 
stmmac_platform.c, which would make MAC driver know it is a internal phy.

Re: [PATCH v3 05/11] net: stmmac: dwmac-rk: Add internal phy support

[Chen-Yu Tsai]

On Thu, Aug 3, 2017 at 1:38 AM, Florian Fainelli  wrote:
> On 08/01/2017 11:21 PM, David Wu wrote:
>> To make internal phy work, need to configure the phy_clock,
>> phy cru_reset and related registers.
>>
>> Signed-off-by: David Wu 
>> ---
>>  .../devicetree/bindings/net/rockchip-dwmac.txt |  6 +-
>>  drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 81 
>> ++
>>  2 files changed, 86 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.txt 
>> b/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
>> index 8f42755..ec39b31 100644
>> --- a/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
>> +++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
>> @@ -25,7 +25,8 @@ Required properties:
>>   - clock-names: One name for each entry in the clocks property.
>>   - phy-mode: See ethernet.txt file in the same directory.
>>   - pinctrl-names: Names corresponding to the numbered pinctrl states.
>> - - pinctrl-0: pin-control mode. can be <_pins> or <_pins>.
>> + - pinctrl-0: pin-control mode. can be <_pins>, <_pins> or led 
>> pins
>> +   for internal phy mode.
>>   - clock_in_out: For RGMII, it must be "input", means main clock(125MHz)
>> is not sourced from SoC's PLL, but input from PHY; For RMII, "input" 
>> means
>> PHY provides the reference clock(50MHz), "output" means GMAC provides the
>> @@ -40,6 +41,9 @@ Optional properties:
>>   - tx_delay: Delay value for TXD timing. Range value is 0~0x7F, 0x30 as 
>> default.
>>   - rx_delay: Delay value for RXD timing. Range value is 0~0x7F, 0x10 as 
>> default.
>>   - phy-supply: phandle to a regulator if the PHY needs one
>> + - clocks: < MAC_PHY>: Clock selector for internal macphy
>> + - phy-is-internal: A boolean property allows us to know that MAC will 
>> connect to
>> +   internal phy.
>
> This is incorrect in two ways:
>
> - this is a property of the PHY, so it should be documented as such in
> Documentation/devicetree/bindings/net/phy.txt so other bindings can
> re-use it
>
> - if it was specific to your MAC you would expect a vendor prefix to
> this property, which is not there
>
> An alternative way to implement the external/internal logic selection
> would be mandate that your Ethernet PHY node have a compatible string
> like this:
>
> phy@0 {
> compatible = "ethernet-phy-id1234.d400", "ethernet-phy-802.3-c22";
> };
>
> Then you don't need this phy-is-internal property, because you can
> locate the PHY node by the phy-handle (see more about that in a reply to
> patch 10) and you can determine ahead of time whether this PHY is
> internal or not based on its compatible string.

This doesn't really work for us (sunxi). The "internal" PHY of the H3
is also available in the X-Powers AC200 combo chip, which would be
external. Same ID. So if someone were to be stupid and put the two
together, you wouldn't know which one it actually is. Generically
it doesn't make sense to match against the ID either. The PHY is
only integrated or inlined into the SoC, meaning it could be moved
elsewhere or even be a discreet part.

The way I see it is more like a reversed pinmux. The system should
select either the internal set or external set of xMII pins to use.

A phy-is-internal property in the PHY node would work for us. We
already need a PHY node to describe its clocks and resets.

A side note to this solution is that, since the internal PHY is defined
at the .dtsi level, any external PHYs at the same address would need to
make sure to delete the property, which is kind of counterintuitive, but
it is how device tree works. One would want to put the internal PHY's
address, assuming it is configurable, on something that is rarely used.


Regards
ChenYu

Re: [PATCH v3 05/11] net: stmmac: dwmac-rk: Add internal phy support

[Florian Fainelli]

On 08/01/2017 11:21 PM, David Wu wrote:
> To make internal phy work, need to configure the phy_clock,
> phy cru_reset and related registers.
> 
> Signed-off-by: David Wu 
> ---
>  .../devicetree/bindings/net/rockchip-dwmac.txt |  6 +-
>  drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 81 
> ++
>  2 files changed, 86 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.txt 
> b/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
> index 8f42755..ec39b31 100644
> --- a/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
> +++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
> @@ -25,7 +25,8 @@ Required properties:
>   - clock-names: One name for each entry in the clocks property.
>   - phy-mode: See ethernet.txt file in the same directory.
>   - pinctrl-names: Names corresponding to the numbered pinctrl states.
> - - pinctrl-0: pin-control mode. can be <_pins> or <_pins>.
> + - pinctrl-0: pin-control mode. can be <_pins>, <_pins> or led 
> pins
> +   for internal phy mode.
>   - clock_in_out: For RGMII, it must be "input", means main clock(125MHz)
> is not sourced from SoC's PLL, but input from PHY; For RMII, "input" means
> PHY provides the reference clock(50MHz), "output" means GMAC provides the
> @@ -40,6 +41,9 @@ Optional properties:
>   - tx_delay: Delay value for TXD timing. Range value is 0~0x7F, 0x30 as 
> default.
>   - rx_delay: Delay value for RXD timing. Range value is 0~0x7F, 0x10 as 
> default.
>   - phy-supply: phandle to a regulator if the PHY needs one
> + - clocks: < MAC_PHY>: Clock selector for internal macphy
> + - phy-is-internal: A boolean property allows us to know that MAC will 
> connect to
> +   internal phy.

This is incorrect in two ways:

- this is a property of the PHY, so it should be documented as such in
Documentation/devicetree/bindings/net/phy.txt so other bindings can
re-use it

- if it was specific to your MAC you would expect a vendor prefix to
this property, which is not there

An alternative way to implement the external/internal logic selection
would be mandate that your Ethernet PHY node have a compatible string
like this:

phy@0 {
compatible = "ethernet-phy-id1234.d400", "ethernet-phy-802.3-c22";
};

Then you don't need this phy-is-internal property, because you can
locate the PHY node by the phy-handle (see more about that in a reply to
patch 10) and you can determine ahead of time whether this PHY is
internal or not based on its compatible string.

Thank you

>  
>  Example:
>  
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c 
> b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> index a8e8fd5..7b80ab9 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> @@ -41,6 +41,7 @@ struct rk_gmac_ops {
>   void (*set_to_rmii)(struct rk_priv_data *bsp_priv);
>   void (*set_rgmii_speed)(struct rk_priv_data *bsp_priv, int speed);
>   void (*set_rmii_speed)(struct rk_priv_data *bsp_priv, int speed);
> + void (*internal_phy_powerup)(struct rk_priv_data *bsp_priv);
>  };
>  
>  struct rk_priv_data {
> @@ -52,6 +53,7 @@ struct rk_priv_data {
>  
>   bool clk_enabled;
>   bool clock_input;
> + bool internal_phy;
>  
>   struct clk *clk_mac;
>   struct clk *gmac_clkin;
> @@ -61,6 +63,9 @@ struct rk_priv_data {
>   struct clk *clk_mac_refout;
>   struct clk *aclk_mac;
>   struct clk *pclk_mac;
> + struct clk *clk_macphy;
> +
> + struct reset_control *macphy_reset;
>  
>   int tx_delay;
>   int rx_delay;
> @@ -750,6 +755,50 @@ static void rk3399_set_rmii_speed(struct rk_priv_data 
> *bsp_priv, int speed)
>   .set_rmii_speed = rk3399_set_rmii_speed,
>  };
>  
> +#define RK_GRF_MACPHY_CON0   0xb00
> +#define RK_GRF_MACPHY_CON1   0xb04
> +#define RK_GRF_MACPHY_CON2   0xb08
> +#define RK_GRF_MACPHY_CON3   0xb0c
> +
> +#define RK_MACPHY_ENABLE GRF_BIT(0)
> +#define RK_MACPHY_DISABLEGRF_CLR_BIT(0)
> +#define RK_MACPHY_CFG_CLK_50MGRF_BIT(14)
> +#define RK_GMAC2PHY_RMII_MODE(GRF_BIT(6) | GRF_CLR_BIT(7))
> +#define RK_GRF_CON2_MACPHY_IDHIWORD_UPDATE(0x1234, 0x, 0)
> +#define RK_GRF_CON3_MACPHY_IDHIWORD_UPDATE(0x35, 0x3f, 0)
> +
> +static void rk_gmac_internal_phy_powerup(struct rk_priv_data *priv)
> +{
> + if (priv->ops->internal_phy_powerup)
> + priv->ops->internal_phy_powerup(priv);
> +
> + regmap_write(priv->grf, RK_GRF_MACPHY_CON0, RK_MACPHY_CFG_CLK_50M);
> + regmap_write(priv->grf, RK_GRF_MACPHY_CON0, RK_GMAC2PHY_RMII_MODE);
> +
> + regmap_write(priv->grf, RK_GRF_MACPHY_CON2, RK_GRF_CON2_MACPHY_ID);
> + regmap_write(priv->grf, RK_GRF_MACPHY_CON3, RK_GRF_CON3_MACPHY_ID);
> +
> + if (priv->macphy_reset) {
> + /* macphy needs to be disabled 

[PATCH v3 05/11] net: stmmac: dwmac-rk: Add internal phy support

[David Wu]

To make internal phy work, need to configure the phy_clock,
phy cru_reset and related registers.

Signed-off-by: David Wu 
---
 .../devicetree/bindings/net/rockchip-dwmac.txt |  6 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 81 ++
 2 files changed, 86 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.txt 
b/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
index 8f42755..ec39b31 100644
--- a/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
+++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
@@ -25,7 +25,8 @@ Required properties:
  - clock-names: One name for each entry in the clocks property.
  - phy-mode: See ethernet.txt file in the same directory.
  - pinctrl-names: Names corresponding to the numbered pinctrl states.
- - pinctrl-0: pin-control mode. can be <_pins> or <_pins>.
+ - pinctrl-0: pin-control mode. can be <_pins>, <_pins> or led pins
+   for internal phy mode.
  - clock_in_out: For RGMII, it must be "input", means main clock(125MHz)
is not sourced from SoC's PLL, but input from PHY; For RMII, "input" means
PHY provides the reference clock(50MHz), "output" means GMAC provides the
@@ -40,6 +41,9 @@ Optional properties:
  - tx_delay: Delay value for TXD timing. Range value is 0~0x7F, 0x30 as 
default.
  - rx_delay: Delay value for RXD timing. Range value is 0~0x7F, 0x10 as 
default.
  - phy-supply: phandle to a regulator if the PHY needs one
+ - clocks: < MAC_PHY>: Clock selector for internal macphy
+ - phy-is-internal: A boolean property allows us to know that MAC will connect 
to
+   internal phy.
 
 Example:
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c 
b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
index a8e8fd5..7b80ab9 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
@@ -41,6 +41,7 @@ struct rk_gmac_ops {
void (*set_to_rmii)(struct rk_priv_data *bsp_priv);
void (*set_rgmii_speed)(struct rk_priv_data *bsp_priv, int speed);
void (*set_rmii_speed)(struct rk_priv_data *bsp_priv, int speed);
+   void (*internal_phy_powerup)(struct rk_priv_data *bsp_priv);
 };
 
 struct rk_priv_data {
@@ -52,6 +53,7 @@ struct rk_priv_data {
 
bool clk_enabled;
bool clock_input;
+   bool internal_phy;
 
struct clk *clk_mac;
struct clk *gmac_clkin;
@@ -61,6 +63,9 @@ struct rk_priv_data {
struct clk *clk_mac_refout;
struct clk *aclk_mac;
struct clk *pclk_mac;
+   struct clk *clk_macphy;
+
+   struct reset_control *macphy_reset;
 
int tx_delay;
int rx_delay;
@@ -750,6 +755,50 @@ static void rk3399_set_rmii_speed(struct rk_priv_data 
*bsp_priv, int speed)
.set_rmii_speed = rk3399_set_rmii_speed,
 };
 
+#define RK_GRF_MACPHY_CON0 0xb00
+#define RK_GRF_MACPHY_CON1 0xb04
+#define RK_GRF_MACPHY_CON2 0xb08
+#define RK_GRF_MACPHY_CON3 0xb0c
+
+#define RK_MACPHY_ENABLE   GRF_BIT(0)
+#define RK_MACPHY_DISABLE  GRF_CLR_BIT(0)
+#define RK_MACPHY_CFG_CLK_50M  GRF_BIT(14)
+#define RK_GMAC2PHY_RMII_MODE  (GRF_BIT(6) | GRF_CLR_BIT(7))
+#define RK_GRF_CON2_MACPHY_ID  HIWORD_UPDATE(0x1234, 0x, 0)
+#define RK_GRF_CON3_MACPHY_ID  HIWORD_UPDATE(0x35, 0x3f, 0)
+
+static void rk_gmac_internal_phy_powerup(struct rk_priv_data *priv)
+{
+   if (priv->ops->internal_phy_powerup)
+   priv->ops->internal_phy_powerup(priv);
+
+   regmap_write(priv->grf, RK_GRF_MACPHY_CON0, RK_MACPHY_CFG_CLK_50M);
+   regmap_write(priv->grf, RK_GRF_MACPHY_CON0, RK_GMAC2PHY_RMII_MODE);
+
+   regmap_write(priv->grf, RK_GRF_MACPHY_CON2, RK_GRF_CON2_MACPHY_ID);
+   regmap_write(priv->grf, RK_GRF_MACPHY_CON3, RK_GRF_CON3_MACPHY_ID);
+
+   if (priv->macphy_reset) {
+   /* macphy needs to be disabled before trying to reset it */
+   regmap_write(priv->grf, RK_GRF_MACPHY_CON0, RK_MACPHY_DISABLE);
+   if (priv->macphy_reset)
+   reset_control_assert(priv->macphy_reset);
+   usleep_range(10, 20);
+   if (priv->macphy_reset)
+   reset_control_deassert(priv->macphy_reset);
+   usleep_range(10, 20);
+   regmap_write(priv->grf, RK_GRF_MACPHY_CON0, RK_MACPHY_ENABLE);
+   msleep(30);
+   }
+}
+
+static void rk_gmac_internal_phy_powerdown(struct rk_priv_data *priv)
+{
+   regmap_write(priv->grf, RK_GRF_MACPHY_CON0, RK_MACPHY_DISABLE);
+   if (priv->macphy_reset)
+   reset_control_assert(priv->macphy_reset);
+}
+
 static int gmac_clk_init(struct rk_priv_data *bsp_priv)
 {
struct device *dev = _priv->pdev->dev;
@@ -803,6 +852,14 @@ static int gmac_clk_init(struct rk_priv_data *bsp_priv)