mbox series

[net-next,v5,0/4] RGMII Internal delay common property

Message ID 20200602164522.3276-1-dmurphy@ti.com
Headers show
Series RGMII Internal delay common property | expand

Message

Dan Murphy June 2, 2020, 4:45 p.m. UTC 
Hello

The RGMII internal delay is a common setting found in most RGMII capable PHY
devices.  It was found that many vendor specific device tree properties exist
to do the same function. This creates a common property to be used for PHY's
that have tunable internal delays for the Rx and Tx paths.

Dan Murphy (4):
  dt-bindings: net: Add tx and rx internal delays
  net: phy: Add a helper to return the index for of the internal delay
  dt-bindings: net: Add RGMII internal delay for DP83869
  net: dp83869: Add RGMII internal delay configuration

 .../devicetree/bindings/net/ethernet-phy.yaml | 13 +++
 .../devicetree/bindings/net/ti,dp83869.yaml   | 16 +++-
 drivers/net/phy/dp83869.c                     | 82 ++++++++++++++++++-
 drivers/net/phy/phy_device.c                  | 51 ++++++++++++
 include/linux/phy.h                           |  2 +
 5 files changed, 160 insertions(+), 4 deletions(-)

Comments

Florian Fainelli June 2, 2020, 10:33 p.m. UTC | #1 
On 6/2/2020 9:45 AM, Dan Murphy wrote:
> Add RGMII internal delay configuration for Rx and Tx.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---

[snip]

> +
>  enum {
>  	DP83869_PORT_MIRRORING_KEEP,
>  	DP83869_PORT_MIRRORING_EN,
> @@ -108,6 +113,8 @@ enum {
>  struct dp83869_private {
>  	int tx_fifo_depth;
>  	int rx_fifo_depth;
> +	s32 rx_id_delay;
> +	s32 tx_id_delay;
>  	int io_impedance;
>  	int port_mirroring;
>  	bool rxctrl_strap_quirk;
> @@ -232,6 +239,22 @@ static int dp83869_of_init(struct phy_device *phydev)
>  				 &dp83869->tx_fifo_depth))
>  		dp83869->tx_fifo_depth = DP83869_PHYCR_FIFO_DEPTH_4_B_NIB;
>  
> +	ret = of_property_read_u32(of_node, "rx-internal-delay-ps",
> +				   &dp83869->rx_id_delay);
> +	if (ret) {
> +		dp83869->rx_id_delay =
> +				dp83869_internal_delay[DP83869_CLK_DELAY_DEF];
> +		ret = 0;
> +	}
> +
> +	ret = of_property_read_u32(of_node, "tx-internal-delay-ps",
> +				   &dp83869->tx_id_delay);
> +	if (ret) {
> +		dp83869->tx_id_delay =
> +				dp83869_internal_delay[DP83869_CLK_DELAY_DEF];
> +		ret = 0;
> +	}

It is still not clear to me why is not the parsing being done by the PHY
library helper directly?
Dan Murphy June 2, 2020, 11:10 p.m. UTC | #2 
Florian

On 6/2/20 5:33 PM, Florian Fainelli wrote:
>
> On 6/2/2020 9:45 AM, Dan Murphy wrote:
>> Add RGMII internal delay configuration for Rx and Tx.
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
> [snip]
>
>> +
>>   enum {
>>   	DP83869_PORT_MIRRORING_KEEP,
>>   	DP83869_PORT_MIRRORING_EN,
>> @@ -108,6 +113,8 @@ enum {
>>   struct dp83869_private {
>>   	int tx_fifo_depth;
>>   	int rx_fifo_depth;
>> +	s32 rx_id_delay;
>> +	s32 tx_id_delay;
>>   	int io_impedance;
>>   	int port_mirroring;
>>   	bool rxctrl_strap_quirk;
>> @@ -232,6 +239,22 @@ static int dp83869_of_init(struct phy_device *phydev)
>>   				 &dp83869->tx_fifo_depth))
>>   		dp83869->tx_fifo_depth = DP83869_PHYCR_FIFO_DEPTH_4_B_NIB;
>>   
>> +	ret = of_property_read_u32(of_node, "rx-internal-delay-ps",
>> +				   &dp83869->rx_id_delay);
>> +	if (ret) {
>> +		dp83869->rx_id_delay =
>> +				dp83869_internal_delay[DP83869_CLK_DELAY_DEF];
>> +		ret = 0;
>> +	}
>> +
>> +	ret = of_property_read_u32(of_node, "tx-internal-delay-ps",
>> +				   &dp83869->tx_id_delay);
>> +	if (ret) {
>> +		dp83869->tx_id_delay =
>> +				dp83869_internal_delay[DP83869_CLK_DELAY_DEF];
>> +		ret = 0;
>> +	}
> It is still not clear to me why is not the parsing being done by the PHY
> library helper directly?

Why would we do that for these properties and not any other?

Unless there is a new precedence being set here by having the PHY 
framework do all the dt node parsing for common properties.

Dan
Florian Fainelli June 2, 2020, 11:13 p.m. UTC | #3 
On 6/2/2020 4:10 PM, Dan Murphy wrote:
> Florian
> 
> On 6/2/20 5:33 PM, Florian Fainelli wrote:
>>
>> On 6/2/2020 9:45 AM, Dan Murphy wrote:
>>> Add RGMII internal delay configuration for Rx and Tx.
>>>
>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>> ---
>> [snip]
>>
>>> +
>>>   enum {
>>>       DP83869_PORT_MIRRORING_KEEP,
>>>       DP83869_PORT_MIRRORING_EN,
>>> @@ -108,6 +113,8 @@ enum {
>>>   struct dp83869_private {
>>>       int tx_fifo_depth;
>>>       int rx_fifo_depth;
>>> +    s32 rx_id_delay;
>>> +    s32 tx_id_delay;
>>>       int io_impedance;
>>>       int port_mirroring;
>>>       bool rxctrl_strap_quirk;
>>> @@ -232,6 +239,22 @@ static int dp83869_of_init(struct phy_device
>>> *phydev)
>>>                    &dp83869->tx_fifo_depth))
>>>           dp83869->tx_fifo_depth = DP83869_PHYCR_FIFO_DEPTH_4_B_NIB;
>>>   +    ret = of_property_read_u32(of_node, "rx-internal-delay-ps",
>>> +                   &dp83869->rx_id_delay);
>>> +    if (ret) {
>>> +        dp83869->rx_id_delay =
>>> +                dp83869_internal_delay[DP83869_CLK_DELAY_DEF];
>>> +        ret = 0;
>>> +    }
>>> +
>>> +    ret = of_property_read_u32(of_node, "tx-internal-delay-ps",
>>> +                   &dp83869->tx_id_delay);
>>> +    if (ret) {
>>> +        dp83869->tx_id_delay =
>>> +                dp83869_internal_delay[DP83869_CLK_DELAY_DEF];
>>> +        ret = 0;
>>> +    }
>> It is still not clear to me why is not the parsing being done by the PHY
>> library helper directly?
> 
> Why would we do that for these properties and not any other?

Those properties have a standard name, which makes them suitable for
parsing by the core PHY library.

> 
> Unless there is a new precedence being set here by having the PHY
> framework do all the dt node parsing for common properties.

You could parse the vendor properties through the driver, let the PHY
library parse the standard properties, and resolve any ordering
precedence within the driver. In general, I would favor standard
properties over vendor properties.

Does this help?
Dan Murphy June 2, 2020, 11:18 p.m. UTC | #4 
Florian

On 6/2/20 6:13 PM, Florian Fainelli wrote:
>
> On 6/2/2020 4:10 PM, Dan Murphy wrote:
>> Florian
>>
>> On 6/2/20 5:33 PM, Florian Fainelli wrote:
>>> On 6/2/2020 9:45 AM, Dan Murphy wrote:
>>>> Add RGMII internal delay configuration for Rx and Tx.
>>>>
>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>>> ---
>>> [snip]
>>>
>>>> +
>>>>    enum {
>>>>        DP83869_PORT_MIRRORING_KEEP,
>>>>        DP83869_PORT_MIRRORING_EN,
>>>> @@ -108,6 +113,8 @@ enum {
>>>>    struct dp83869_private {
>>>>        int tx_fifo_depth;
>>>>        int rx_fifo_depth;
>>>> +    s32 rx_id_delay;
>>>> +    s32 tx_id_delay;
>>>>        int io_impedance;
>>>>        int port_mirroring;
>>>>        bool rxctrl_strap_quirk;
>>>> @@ -232,6 +239,22 @@ static int dp83869_of_init(struct phy_device
>>>> *phydev)
>>>>                     &dp83869->tx_fifo_depth))
>>>>            dp83869->tx_fifo_depth = DP83869_PHYCR_FIFO_DEPTH_4_B_NIB;
>>>>    +    ret = of_property_read_u32(of_node, "rx-internal-delay-ps",
>>>> +                   &dp83869->rx_id_delay);
>>>> +    if (ret) {
>>>> +        dp83869->rx_id_delay =
>>>> +                dp83869_internal_delay[DP83869_CLK_DELAY_DEF];
>>>> +        ret = 0;
>>>> +    }
>>>> +
>>>> +    ret = of_property_read_u32(of_node, "tx-internal-delay-ps",
>>>> +                   &dp83869->tx_id_delay);
>>>> +    if (ret) {
>>>> +        dp83869->tx_id_delay =
>>>> +                dp83869_internal_delay[DP83869_CLK_DELAY_DEF];
>>>> +        ret = 0;
>>>> +    }
>>> It is still not clear to me why is not the parsing being done by the PHY
>>> library helper directly?
>> Why would we do that for these properties and not any other?
> Those properties have a standard name, which makes them suitable for
> parsing by the core PHY library.
>> Unless there is a new precedence being set here by having the PHY
>> framework do all the dt node parsing for common properties.
> You could parse the vendor properties through the driver, let the PHY
> library parse the standard properties, and resolve any ordering
> precedence within the driver. In general, I would favor standard
> properties over vendor properties.
>
> Does this help?

Ok so new precedence then.

Because there are common properties like tx-fifo-depth, rx-fifo-depth, 
enet-phy-lane-swap and max_speed that the PHY framework should parse as 
well.

Dan
Dan Murphy June 2, 2020, 11:25 p.m. UTC | #5 
Florian

On 6/2/20 6:18 PM, Dan Murphy wrote:
> Florian
>
> On 6/2/20 6:13 PM, Florian Fainelli wrote:
>>
>> On 6/2/2020 4:10 PM, Dan Murphy wrote:
>>> Florian
>>>
>>> On 6/2/20 5:33 PM, Florian Fainelli wrote:
>>>> On 6/2/2020 9:45 AM, Dan Murphy wrote:
>>>>> Add RGMII internal delay configuration for Rx and Tx.
>>>>>
>>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>>>> ---
>>>> [snip]
>>>>
>>>>> +
>>>>>    enum {
>>>>>        DP83869_PORT_MIRRORING_KEEP,
>>>>>        DP83869_PORT_MIRRORING_EN,
>>>>> @@ -108,6 +113,8 @@ enum {
>>>>>    struct dp83869_private {
>>>>>        int tx_fifo_depth;
>>>>>        int rx_fifo_depth;
>>>>> +    s32 rx_id_delay;
>>>>> +    s32 tx_id_delay;
>>>>>        int io_impedance;
>>>>>        int port_mirroring;
>>>>>        bool rxctrl_strap_quirk;
>>>>> @@ -232,6 +239,22 @@ static int dp83869_of_init(struct phy_device
>>>>> *phydev)
>>>>>                     &dp83869->tx_fifo_depth))
>>>>>            dp83869->tx_fifo_depth = DP83869_PHYCR_FIFO_DEPTH_4_B_NIB;
>>>>>    +    ret = of_property_read_u32(of_node, "rx-internal-delay-ps",
>>>>> +                   &dp83869->rx_id_delay);
>>>>> +    if (ret) {
>>>>> +        dp83869->rx_id_delay =
>>>>> + dp83869_internal_delay[DP83869_CLK_DELAY_DEF];
>>>>> +        ret = 0;
>>>>> +    }
>>>>> +
>>>>> +    ret = of_property_read_u32(of_node, "tx-internal-delay-ps",
>>>>> +                   &dp83869->tx_id_delay);
>>>>> +    if (ret) {
>>>>> +        dp83869->tx_id_delay =
>>>>> + dp83869_internal_delay[DP83869_CLK_DELAY_DEF];
>>>>> +        ret = 0;
>>>>> +    }
>>>> It is still not clear to me why is not the parsing being done by 
>>>> the PHY
>>>> library helper directly?
>>> Why would we do that for these properties and not any other?
>> Those properties have a standard name, which makes them suitable for
>> parsing by the core PHY library.
>>> Unless there is a new precedence being set here by having the PHY
>>> framework do all the dt node parsing for common properties.
>> You could parse the vendor properties through the driver, let the PHY
>> library parse the standard properties, and resolve any ordering
>> precedence within the driver. In general, I would favor standard
>> properties over vendor properties.
>>
>> Does this help?
>
> Ok so new precedence then.
>
> Because there are common properties like tx-fifo-depth, rx-fifo-depth, 
> enet-phy-lane-swap and max_speed that the PHY framework should parse 
> as well.
>
I am assuming that the retrieval of the property and getting the index 
should be 2 separate APIs?

One API to get the property value and one API to find the index value.

Dan


> Dan
>