Linux-ARM-Kernel Archive on lore.kernel.org
 help / Atom feed
* Re: [PATCH 6/7] net: phy: at803x: Add support to disable tx/rx delays
       [not found] ` <20190102091729.18582-7-vkoul@kernel.org>
@ 2019-01-02 13:40   ` Andrew Lunn
  2019-01-02 14:36     ` Vinod Koul
  0 siblings, 1 reply; 7+ messages in thread
From: Andrew Lunn @ 2019-01-02 13:40 UTC (permalink / raw)
  To: Vinod Koul
  Cc: Florian Fainelli, netdev, Bjorn Andersson, Niklas Cassel,
	David S . Miller, linux-arm-kernel

On Wed, Jan 02, 2019 at 02:47:28PM +0530, Vinod Koul wrote:
> Some controllers require the tx and rx delays to be disabled. So check
> the property and if present do not enable the delay and disable the
> delay explicitly.
> 
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  drivers/net/phy/at803x.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> index 63e3d3d774d1..9bfc0d381159 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -122,6 +122,17 @@ static inline int at803x_enable_tx_delay(struct phy_device *phydev)
>  					AT803X_DEBUG_TX_CLK_DLY_EN);
>  }
>  
> +static inline int at803x_disable_rx_delay(struct phy_device *phydev)
> +{
> +	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0,
> +					AT803X_DEBUG_RX_CLK_DLY_EN, 0);
> +}
> +
> +static inline int at803x_disable_tx_delay(struct phy_device *phydev)
> +{
> +	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_5,
> +					AT803X_DEBUG_TX_CLK_DLY_EN, 0);
> +}
>  /* save relevant PHY registers to private copy */
>  static void at803x_context_save(struct phy_device *phydev,
>  				struct at803x_context *context)
> @@ -250,12 +261,18 @@ static int at803x_probe(struct phy_device *phydev)
>  static int at803x_config_init(struct phy_device *phydev)
>  {
>  	bool rx_delay = false, tx_delay = false;
> +	bool rx_disable_prop, tx_disable_prop;
>  	int ret;
>  
>  	ret = genphy_config_init(phydev);
>  	if (ret < 0)
>  		return ret;
>  
> +	rx_disable_prop = device_property_read_bool(&phydev->mdio.dev,
> +						    "rx-delay-disable");
> +	tx_disable_prop = device_property_read_bool(&phydev->mdio.dev,
> +						    "rx-delay-disable");
> +

Hi Vinod

I understand why you are doing this, to not break backwards
compatibility, but it is ugly. Lets see if we can avoid it. Thinking
allowed here. Here are the use cases i can think of:

1) The DT does not specify any phy-mode. The board works because delays
are enable by the bootloader

2) The DT correctly specifies RXID/ID/TXID and the driver does the
right thing.

3) The DT incorrectly specifies no delay, the bootloader however sets
delays, and the driver does not disable the delay.

4) The DT correctly specifies no delay, but the driver does not
disable delays, and it does not work.

You are interested in 4) if i understand this patch correct.

1) should not be a problem. If phy-mode is not one of the RGMII
values, don't touch the delays.

2) works

3) is the tricky one. But i would also say that is a bug in the DT.
The question is, do we want to keep bug compatible?

I say don't add these new properties. If we have a phy-mode which
explicitly specifies no delay, clear the delay. And we then fixup
anything which breaks because of DT bugs.

	 Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/7] dt-bindings: net: Add Qualcomm ethqos binding
       [not found] ` <20190102091729.18582-2-vkoul@kernel.org>
@ 2019-01-02 14:07   ` Andrew Lunn
  2019-01-02 14:37     ` Vinod Koul
  0 siblings, 1 reply; 7+ messages in thread
From: Andrew Lunn @ 2019-01-02 14:07 UTC (permalink / raw)
  To: Vinod Koul
  Cc: Mark Rutland, devicetree, netdev, Bjorn Andersson, Rob Herring,
	Niklas Cassel, David S . Miller, linux-arm-kernel

> +	mdio {
> +		#address-cells = <0x1>;
> +		#size-cells = <0x0>;
> +		compatible = "snps,dwmac-mdio";
> +		phy1: phy@1 {

phy@4 since reg = 0x4

> +			device_type = "ethernet-phy";
> +			reg = <0x4>;

Thanks,
	Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 6/7] net: phy: at803x: Add support to disable tx/rx delays
  2019-01-02 13:40   ` [PATCH 6/7] net: phy: at803x: Add support to disable tx/rx delays Andrew Lunn
@ 2019-01-02 14:36     ` Vinod Koul
  0 siblings, 0 replies; 7+ messages in thread
From: Vinod Koul @ 2019-01-02 14:36 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Florian Fainelli, netdev, Bjorn Andersson, Niklas Cassel,
	David S . Miller, linux-arm-kernel


Hi Andrew,

Thanks for the comments,

On 02-01-19, 14:40, Andrew Lunn wrote:
> On Wed, Jan 02, 2019 at 02:47:28PM +0530, Vinod Koul wrote:
> > Some controllers require the tx and rx delays to be disabled. So check
> > the property and if present do not enable the delay and disable the
> > delay explicitly.
> > 
> > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > ---
> >  drivers/net/phy/at803x.c | 36 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> > 
> > diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> > index 63e3d3d774d1..9bfc0d381159 100644
> > --- a/drivers/net/phy/at803x.c
> > +++ b/drivers/net/phy/at803x.c
> > @@ -122,6 +122,17 @@ static inline int at803x_enable_tx_delay(struct phy_device *phydev)
> >  					AT803X_DEBUG_TX_CLK_DLY_EN);
> >  }
> >  
> > +static inline int at803x_disable_rx_delay(struct phy_device *phydev)
> > +{
> > +	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0,
> > +					AT803X_DEBUG_RX_CLK_DLY_EN, 0);
> > +}
> > +
> > +static inline int at803x_disable_tx_delay(struct phy_device *phydev)
> > +{
> > +	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_5,
> > +					AT803X_DEBUG_TX_CLK_DLY_EN, 0);
> > +}
> >  /* save relevant PHY registers to private copy */
> >  static void at803x_context_save(struct phy_device *phydev,
> >  				struct at803x_context *context)
> > @@ -250,12 +261,18 @@ static int at803x_probe(struct phy_device *phydev)
> >  static int at803x_config_init(struct phy_device *phydev)
> >  {
> >  	bool rx_delay = false, tx_delay = false;
> > +	bool rx_disable_prop, tx_disable_prop;
> >  	int ret;
> >  
> >  	ret = genphy_config_init(phydev);
> >  	if (ret < 0)
> >  		return ret;
> >  
> > +	rx_disable_prop = device_property_read_bool(&phydev->mdio.dev,
> > +						    "rx-delay-disable");
> > +	tx_disable_prop = device_property_read_bool(&phydev->mdio.dev,
> > +						    "rx-delay-disable");
> > +
> 
> Hi Vinod
> 
> I understand why you are doing this, to not break backwards
> compatibility, but it is ugly. Lets see if we can avoid it. Thinking

Agreed this is ugly and the reason to do this is not to break existing
users

> allowed here. Here are the use cases i can think of:
> 
> 1) The DT does not specify any phy-mode. The board works because delays
> are enable by the bootloader
> 
> 2) The DT correctly specifies RXID/ID/TXID and the driver does the
> right thing.
> 
> 3) The DT incorrectly specifies no delay, the bootloader however sets
> delays, and the driver does not disable the delay.
> 
> 4) The DT correctly specifies no delay, but the driver does not
> disable delays, and it does not work.
> 
> You are interested in 4) if i understand this patch correct.

that is correct reading of the patch :-)

> 1) should not be a problem. If phy-mode is not one of the RGMII
> values, don't touch the delays.
> 
> 2) works
> 
> 3) is the tricky one. But i would also say that is a bug in the DT.
> The question is, do we want to keep bug compatible?
> 
> I say don't add these new properties. If we have a phy-mode which
> explicitly specifies no delay, clear the delay. And we then fixup
> anything which breaks because of DT bugs.

I do not mind fixing this and doing disable delays for rgmii mode, if we
agree that it would break devices and those should be fixed and not
treated as a regression due to this fix. As long as Dave agree to this,
I can spin a v2 and post :)

~Vinod

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/7] dt-bindings: net: Add Qualcomm ethqos binding
  2019-01-02 14:07   ` [PATCH 1/7] dt-bindings: net: Add Qualcomm ethqos binding Andrew Lunn
@ 2019-01-02 14:37     ` Vinod Koul
  0 siblings, 0 replies; 7+ messages in thread
From: Vinod Koul @ 2019-01-02 14:37 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Mark Rutland, devicetree, netdev, Bjorn Andersson, Rob Herring,
	Niklas Cassel, David S . Miller, linux-arm-kernel

On 02-01-19, 15:07, Andrew Lunn wrote:
> > +	mdio {
> > +		#address-cells = <0x1>;
> > +		#size-cells = <0x0>;
> > +		compatible = "snps,dwmac-mdio";
> > +		phy1: phy@1 {
> 
> phy@4 since reg = 0x4

Right, will fix this

-- 
~Vinod

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 3/7] dt-bindings: net: stmmac: Add the bindings documentation for delays
       [not found] ` <20190102091729.18582-4-vkoul@kernel.org>
@ 2019-01-11 15:01   ` Rob Herring
  2019-01-14 15:26     ` Vinod Koul
  2019-01-14 23:34     ` Florian Fainelli
  0 siblings, 2 replies; 7+ messages in thread
From: Rob Herring @ 2019-01-11 15:01 UTC (permalink / raw)
  To: Vinod Koul
  Cc: Mark Rutland, devicetree, netdev, Bjorn Andersson, Niklas Cassel,
	David S . Miller, linux-arm-kernel

On Wed, Jan 02, 2019 at 02:47:25PM +0530, Vinod Koul wrote:
> Some controllers require that phy delay should be disabled. So add

If the MAC requires it, then the compatible string should imply this. If 
it depends on the PHY, then okay.

> optional properties rx-disable-delay and tx-disable-delay for it.
> 
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  Documentation/devicetree/bindings/net/stmmac.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/stmmac.txt b/Documentation/devicetree/bindings/net/stmmac.txt
> index cb694062afff..ec18897e22c8 100644
> --- a/Documentation/devicetree/bindings/net/stmmac.txt
> +++ b/Documentation/devicetree/bindings/net/stmmac.txt
> @@ -74,6 +74,8 @@ Optional properties:
>  	- snps,mb: mixed-burst
>  	- snps,rb: rebuild INCRx Burst
>  - mdio: with compatible = "snps,dwmac-mdio", create and register mdio bus.
> +- rx-delay-disable: bool, when present disable the rx delay
> +- tx-delay-disable: bool, when present disable the tx delay

Needs a vendor prefix.

>  - Multiple RX Queues parameters: below the list of all the parameters to
>  				 configure the multiple RX queues:
>  	- snps,rx-queues-to-use: number of RX queues to be used in the driver
> -- 
> 2.20.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 3/7] dt-bindings: net: stmmac: Add the bindings documentation for delays
  2019-01-11 15:01   ` [PATCH 3/7] dt-bindings: net: stmmac: Add the bindings documentation for delays Rob Herring
@ 2019-01-14 15:26     ` Vinod Koul
  2019-01-14 23:34     ` Florian Fainelli
  1 sibling, 0 replies; 7+ messages in thread
From: Vinod Koul @ 2019-01-14 15:26 UTC (permalink / raw)
  To: Rob Herring
  Cc: Mark Rutland, devicetree, netdev, Bjorn Andersson, Niklas Cassel,
	David S . Miller, linux-arm-kernel

HI Rob,

On 11-01-19, 09:01, Rob Herring wrote:
> On Wed, Jan 02, 2019 at 02:47:25PM +0530, Vinod Koul wrote:
> > Some controllers require that phy delay should be disabled. So add
> 
> If the MAC requires it, then the compatible string should imply this. If 
> it depends on the PHY, then okay.

Thanks for the review, yes this was targeted to PHY. After feedback
from Andrew, I have removed this is v2 posted earlier!

-- 
~Vinod

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 3/7] dt-bindings: net: stmmac: Add the bindings documentation for delays
  2019-01-11 15:01   ` [PATCH 3/7] dt-bindings: net: stmmac: Add the bindings documentation for delays Rob Herring
  2019-01-14 15:26     ` Vinod Koul
@ 2019-01-14 23:34     ` Florian Fainelli
  1 sibling, 0 replies; 7+ messages in thread
From: Florian Fainelli @ 2019-01-14 23:34 UTC (permalink / raw)
  To: Rob Herring, Vinod Koul
  Cc: Mark Rutland, devicetree, netdev, Bjorn Andersson, Niklas Cassel,
	David S . Miller, linux-arm-kernel

On 1/11/19 7:01 AM, Rob Herring wrote:
> On Wed, Jan 02, 2019 at 02:47:25PM +0530, Vinod Koul wrote:
>> Some controllers require that phy delay should be disabled. So add
> 
> If the MAC requires it, then the compatible string should imply this. If 
> it depends on the PHY, then okay.
> 
>> optional properties rx-disable-delay and tx-disable-delay for it.
>>
>> Signed-off-by: Vinod Koul <vkoul@kernel.org>
>> ---
>>  Documentation/devicetree/bindings/net/stmmac.txt | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/stmmac.txt b/Documentation/devicetree/bindings/net/stmmac.txt
>> index cb694062afff..ec18897e22c8 100644
>> --- a/Documentation/devicetree/bindings/net/stmmac.txt
>> +++ b/Documentation/devicetree/bindings/net/stmmac.txt
>> @@ -74,6 +74,8 @@ Optional properties:
>>  	- snps,mb: mixed-burst
>>  	- snps,rb: rebuild INCRx Burst
>>  - mdio: with compatible = "snps,dwmac-mdio", create and register mdio bus.
>> +- rx-delay-disable: bool, when present disable the rx delay
>> +- tx-delay-disable: bool, when present disable the tx delay
> 
> Needs a vendor prefix.

Indeed, and it would actually be nicer to allow specifying delays
directly in ps units, see:

Documentation/devicetree/bindings/net/dwmac-sun8i.txt
Documentation/devicetree/bindings/net/mediatek-dwmac.txt

for examples.
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20190102091729.18582-1-vkoul@kernel.org>
     [not found] ` <20190102091729.18582-7-vkoul@kernel.org>
2019-01-02 13:40   ` [PATCH 6/7] net: phy: at803x: Add support to disable tx/rx delays Andrew Lunn
2019-01-02 14:36     ` Vinod Koul
     [not found] ` <20190102091729.18582-2-vkoul@kernel.org>
2019-01-02 14:07   ` [PATCH 1/7] dt-bindings: net: Add Qualcomm ethqos binding Andrew Lunn
2019-01-02 14:37     ` Vinod Koul
     [not found] ` <20190102091729.18582-4-vkoul@kernel.org>
2019-01-11 15:01   ` [PATCH 3/7] dt-bindings: net: stmmac: Add the bindings documentation for delays Rob Herring
2019-01-14 15:26     ` Vinod Koul
2019-01-14 23:34     ` Florian Fainelli

Linux-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org infradead-linux-arm-kernel@archiver.kernel.org
	public-inbox-index linux-arm-kernel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox