* 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 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
[parent not found: <20190102091729.18582-2-vkoul@kernel.org>]
* 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 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
[parent not found: <20190102091729.18582-4-vkoul@kernel.org>]
* 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, other threads:[~2019-01-14 23:35 UTC | newest] 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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).