[net-next,v7,0/6] RGMII Internal delay common property

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 internal delays for the Rx and Tx paths.

If the internal delay is tunable then the caller needs to pass the internal
delay array and the return will be the index in the array that was found in
the firmware node.

If the internal delay is fixed then the caller only needs to indicate which
delay to return and if the value is defined then the value in the firmware is
returned.

This series contains examples of both a configurable delay and a fixed delay.

Dan Murphy (6):
  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
  dt-bindings: net: dp83822: Add TI dp83822 phy
  net: phy: DP83822: Add ability to advertise Fiber connection

 .../devicetree/bindings/net/ethernet-phy.yaml |  11 ++
 .../devicetree/bindings/net/ti,dp83822.yaml   |  51 +++++++++
 .../devicetree/bindings/net/ti,dp83869.yaml   |  16 ++-
 drivers/net/phy/dp83822.c                     | 108 ++++++++++++++++--
 drivers/net/phy/dp83869.c                     |  53 ++++++++-
 drivers/net/phy/phy_device.c                  |  68 +++++++++++
 include/linux/phy.h                           |   4 +
 7 files changed, 299 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/ti,dp83822.yaml

All

On 6/17/20 1:20 PM, Dan Murphy wrote:
> The DP83822 can be configured to use the RGMII interface. There are
> independent fixed 3.5ns clock shift (aka internal delay) for the TX and RX
> paths. This allow either one to be set if the MII interface is RGMII and
> the value is set in the firmware node.

$subject is wrong.  I used the 83822 fiber patch as my base as it had 
90% of the work done that I needed for the

internal delay.  I will fix it in v8 after review comments.

Dan

On Wed, 17 Jun 2020 13:20:19 -0500 Dan Murphy wrote:
> +static int dp83822_config_init(struct phy_device *phydev)
> +{
> +	struct dp83822_private *dp83822 = phydev->priv;
> +	int rgmii_delay;
> +	int err = 0;
> +
> +	if (phy_interface_is_rgmii(phydev)) {
> +		if (dp83822->rx_int_delay)
> +			rgmii_delay = DP83822_RX_CLK_SHIFT;
> +
> +		if (dp83822->tx_int_delay)
> +			rgmii_delay |= DP83822_TX_CLK_SHIFT;
> +
> +		if (rgmii_delay)
> +			err = phy_set_bits_mmd(phydev, DP83822_DEVADDR,
> +					       MII_DP83822_RCSR, rgmii_delay);
> +	}
> +
> +	return dp8382x_disable_wol(phydev);
> +}


drivers/net/phy/dp83822.c: In function dp83822_config_init:
drivers/net/phy/dp83822.c:282:6: warning: variable err set but not used [-Wunused-but-set-variable]
  282 |  int err = 0;
      |      ^~~

On Wed, Jun 17, 2020 at 01:20:15PM -0500, Dan Murphy wrote:
> Add a helper function that will return the index in the array for the
> passed in internal delay value.  The helper requires the array, size and
> delay value.
> 
> The helper will then return the index for the exact match or return the
> index for the index to the closest smaller value.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
>  drivers/net/phy/phy_device.c | 68 ++++++++++++++++++++++++++++++++++++
>  include/linux/phy.h          |  4 +++
>  2 files changed, 72 insertions(+)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 04946de74fa0..611d4e68e3c6 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -31,6 +31,7 @@
>  #include <linux/mdio.h>
>  #include <linux/io.h>
>  #include <linux/uaccess.h>
> +#include <linux/property.h>
>  
>  MODULE_DESCRIPTION("PHY library");
>  MODULE_AUTHOR("Andy Fleming");
> @@ -2657,6 +2658,73 @@ void phy_get_pause(struct phy_device *phydev, bool *tx_pause, bool *rx_pause)
>  }
>  EXPORT_SYMBOL(phy_get_pause);
>  
> +/**
> + * phy_get_delay_index - returns the index of the internal delay
> + * @phydev: phy_device struct
> + * @dev: pointer to the devices device struct
> + * @delay_values: array of delays the PHY supports
> + * @size: the size of the delay array
> + * @is_rx: boolean to indicate to get the rx internal delay
> + *
> + * Returns the index within the array of internal delay passed in.
> + * Or if size == 0 then the delay read from the firmware is returned.
> + * The array must be in ascending order.
> + * Return errno if the delay is invalid or cannot be found.
> + */
> +s32 phy_get_internal_delay(struct phy_device *phydev, struct device *dev,
> +			   const int *delay_values, int size, bool is_rx)
> +{
> +	int ret;
> +	int i;
> +	s32 delay;
> +
> +	if (is_rx)
> +		ret = device_property_read_u32(dev, "rx-internal-delay-ps",
> +					       &delay);
> +	else
> +		ret = device_property_read_u32(dev, "tx-internal-delay-ps",
> +					       &delay);
> +	if (ret) {
> +		phydev_err(phydev, "internal delay not defined\n");

This is an optional property. So printing an error message seems heavy
handed.

Maybe it would be better to default to 0 if the property is not found,
and continue with the lookup in the table to find what value should be
written for a 0ps delay?

	Andrew

Andrew

On 6/17/20 8:51 PM, Andrew Lunn wrote:
> On Wed, Jun 17, 2020 at 01:20:15PM -0500, Dan Murphy wrote:
>> Add a helper function that will return the index in the array for the
>> passed in internal delay value.  The helper requires the array, size and
>> delay value.
>>
>> The helper will then return the index for the exact match or return the
>> index for the index to the closest smaller value.
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>>   drivers/net/phy/phy_device.c | 68 ++++++++++++++++++++++++++++++++++++
>>   include/linux/phy.h          |  4 +++
>>   2 files changed, 72 insertions(+)
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 04946de74fa0..611d4e68e3c6 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -31,6 +31,7 @@
>>   #include <linux/mdio.h>
>>   #include <linux/io.h>
>>   #include <linux/uaccess.h>
>> +#include <linux/property.h>
>>   
>>   MODULE_DESCRIPTION("PHY library");
>>   MODULE_AUTHOR("Andy Fleming");
>> @@ -2657,6 +2658,73 @@ void phy_get_pause(struct phy_device *phydev, bool *tx_pause, bool *rx_pause)
>>   }
>>   EXPORT_SYMBOL(phy_get_pause);
>>   
>> +/**
>> + * phy_get_delay_index - returns the index of the internal delay
>> + * @phydev: phy_device struct
>> + * @dev: pointer to the devices device struct
>> + * @delay_values: array of delays the PHY supports
>> + * @size: the size of the delay array
>> + * @is_rx: boolean to indicate to get the rx internal delay
>> + *
>> + * Returns the index within the array of internal delay passed in.
>> + * Or if size == 0 then the delay read from the firmware is returned.
>> + * The array must be in ascending order.
>> + * Return errno if the delay is invalid or cannot be found.
>> + */
>> +s32 phy_get_internal_delay(struct phy_device *phydev, struct device *dev,
>> +			   const int *delay_values, int size, bool is_rx)
>> +{
>> +	int ret;
>> +	int i;
>> +	s32 delay;
>> +
>> +	if (is_rx)
>> +		ret = device_property_read_u32(dev, "rx-internal-delay-ps",
>> +					       &delay);
>> +	else
>> +		ret = device_property_read_u32(dev, "tx-internal-delay-ps",
>> +					       &delay);
>> +	if (ret) {
>> +		phydev_err(phydev, "internal delay not defined\n");
> This is an optional property. So printing an error message seems heavy
> handed.

I will change this to phydev_info


> Maybe it would be better to default to 0 if the property is not found,
> and continue with the lookup in the table to find what value should be
> written for a 0ps delay?

If the property is not found what would we look up?  The property 
missing to me indicates that the phy is not adding the delay for that path.

If these properties are not present then the delay should not be set by 
the device driver.

This is why I return -EINVAL.  Maybe I should return -ENODATA instead.

Dan

>
> 	Andrew