| Message ID | 20190510214550.18657-5-tpiepho@impinj.com |
|---|---|
| State | Deferred |
| Delegated to: | David Miller |
| Headers | show |
| Series | [1/5] dt-bindings: phy: dp83867: Describe how driver behavesw.r.t rgmii delay | expand |
On 10.05.2019 23:46, Trent Piepho wrote: > The variables used to store u32 DT properties were signed ints. This > doesn't work properly if the value of the property were to overflow. > Use unsigned variables so this doesn't happen. > In patch 3 you added a check for DT properties being out of range. I think this would be good also for the three properties here. The delay values are only 4 bits wide, so you might also consider to switch to u8 or u16. Please note that net-next is closed currently. Please resubmit the patches once it's open again, and please annotate them properly with net-next. > Cc: Andrew Lunn <andrew@lunn.ch> > Cc: Florian Fainelli <f.fainelli@gmail.com> > Cc: Heiner Kallweit <hkallweit1@gmail.com> > Signed-off-by: Trent Piepho <tpiepho@impinj.com> > --- > drivers/net/phy/dp83867.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c > index a46cc9427fb3..edd9e27425e8 100644 > --- a/drivers/net/phy/dp83867.c > +++ b/drivers/net/phy/dp83867.c > @@ -82,9 +82,9 @@ enum { > }; > > struct dp83867_private { > - int rx_id_delay; > - int tx_id_delay; > - int fifo_depth; > + u32 rx_id_delay; > + u32 tx_id_delay; > + u32 fifo_depth; > int io_impedance; > int port_mirroring; > bool rxctrl_strap_quirk; >
On 11.05.2019 12:41, Heiner Kallweit wrote: > On 10.05.2019 23:46, Trent Piepho wrote: >> The variables used to store u32 DT properties were signed ints. This >> doesn't work properly if the value of the property were to overflow. >> Use unsigned variables so this doesn't happen. >> > In patch 3 you added a check for DT properties being out of range. > I think this would be good also for the three properties here. > The delay values are only 4 bits wide, so you might also consider > to switch to u8 or u16. > I briefly looked over the rest of the driver. What is plain wrong is to allocate memory for the private data structure in the config_init callback. This has to be done in the probe callback. An example is marvell_probe(). As you seem to work on this driver, can you provide a patch for this? > Please note that net-next is closed currently. Please resubmit the > patches once it's open again, and please annotate them properly > with net-next. > >> Cc: Andrew Lunn <andrew@lunn.ch> >> Cc: Florian Fainelli <f.fainelli@gmail.com> >> Cc: Heiner Kallweit <hkallweit1@gmail.com> >> Signed-off-by: Trent Piepho <tpiepho@impinj.com> >> --- >> drivers/net/phy/dp83867.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c >> index a46cc9427fb3..edd9e27425e8 100644 >> --- a/drivers/net/phy/dp83867.c >> +++ b/drivers/net/phy/dp83867.c >> @@ -82,9 +82,9 @@ enum { >> }; >> >> struct dp83867_private { >> - int rx_id_delay; >> - int tx_id_delay; >> - int fifo_depth; >> + u32 rx_id_delay; >> + u32 tx_id_delay; >> + u32 fifo_depth; >> int io_impedance; >> int port_mirroring; >> bool rxctrl_strap_quirk; >> >
On Sat, 2019-05-11 at 14:32 +0200, Heiner Kallweit wrote: > On 11.05.2019 12:41, Heiner Kallweit wrote: > > On 10.05.2019 23:46, Trent Piepho wrote: > > > The variables used to store u32 DT properties were signed > > > ints. This > > > doesn't work properly if the value of the property were to > > > overflow. > > > Use unsigned variables so this doesn't happen. > > > > > > > In patch 3 you added a check for DT properties being out of range. > > I think this would be good also for the three properties here. > > The delay values are only 4 bits wide, so you might also consider > > to switch to u8 or u16. > > > > I briefly looked over the rest of the driver. What is plain wrong > is to allocate memory for the private data structure in the > config_init callback. This has to be done in the probe callback. > An example is marvell_probe(). As you seem to work on this driver, > can you provide a patch for this? It only allocates the data once, so it is not a memory leak. But yes, totally wrong place to do it. I can fix this. It also does not set the signal line impedance from DT value unless unless also configuring clock skew, even though they are orthogonal concepts. And fails to verify anything read from the DT. Perhaps you could tell me if the approach I've taken in patch 3, "Add ability to disable output clock", and patch 4, "Disable tx/rx delay when not configured", are considered acceptable? I can conceive of arguments for alternate approaches. I would like to add support for these into u-boot too, but typically u-boot follows the kernel DT bindings, so I want to finalize the kernel DT semantics before sending patches to u-boot. > > Please note that net-next is closed currently. Please resubmit the > > patches once it's open again, and please annotate them properly > > with net-next. Sorry, didn't know about this policy. Been years since I've submitted net patches. Is there a description somewhere of how this is done? Googling net-next wasn't helpful. I gather new patches are only allowed when the kernel merge window is open? And they can't be queued on patchwork or a topic branch until this happens?
On 13.05.2019 21:58, Trent Piepho wrote: > On Sat, 2019-05-11 at 14:32 +0200, Heiner Kallweit wrote: >> On 11.05.2019 12:41, Heiner Kallweit wrote: >>> On 10.05.2019 23:46, Trent Piepho wrote: >>>> The variables used to store u32 DT properties were signed >>>> ints. This >>>> doesn't work properly if the value of the property were to >>>> overflow. >>>> Use unsigned variables so this doesn't happen. >>>> >>> >>> In patch 3 you added a check for DT properties being out of range. >>> I think this would be good also for the three properties here. >>> The delay values are only 4 bits wide, so you might also consider >>> to switch to u8 or u16. >>> >> >> I briefly looked over the rest of the driver. What is plain wrong >> is to allocate memory for the private data structure in the >> config_init callback. This has to be done in the probe callback. >> An example is marvell_probe(). As you seem to work on this driver, >> can you provide a patch for this? > > It only allocates the data once, so it is not a memory leak. But yes, > totally wrong place to do it. I can fix this. It also does not set > the signal line impedance from DT value unless unless also configuring > clock skew, even though they are orthogonal concepts. And fails to > verify anything read from the DT. > > Perhaps you could tell me if the approach I've taken in patch 3, > "Add ability to disable output clock", and patch 4, "Disable tx/rx > delay when not configured", are considered acceptable? I can conceive > of arguments for alternate approaches. I would like to add support for > these into u-boot too, but typically u-boot follows the kernel DT > bindings, so I want to finalize the kernel DT semantics before sending > patches to u-boot. > I lack experience with these TI PHY's. Maybe Andrew or Florian can advise. >>> Please note that net-next is closed currently. Please resubmit the >>> patches once it's open again, and please annotate them properly >>> with net-next. > > Sorry, didn't know about this policy. Been years since I've submitted > net patches. Is there a description somewhere of how this is done? > Googling net-next wasn't helpful. I gather new patches are only > allowed when the kernel merge window is open? And they can't be queued > on patchwork or a topic branch until this happens? > https://www.kernel.org/doc/Documentation/networking/netdev-FAQ.txt And the easy way to check whether net-next is open: http://vger.kernel.org/~davem/net-next.html
> > Perhaps you could tell me if the approach I've taken in patch 3, > > "Add ability to disable output clock", and patch 4, "Disable tx/rx > > delay when not configured", are considered acceptable? I can conceive > > of arguments for alternate approaches. I would like to add support for > > these into u-boot too, but typically u-boot follows the kernel DT > > bindings, so I want to finalize the kernel DT semantics before sending > > patches to u-boot. > > > I lack experience with these TI PHY's. Maybe Andrew or Florian can advise. Hi Trent I already deleted the patches. For patch 3: + if (dp83867->clk_output_sel > DP83867_CLK_O_SEL_REF_CLK && + dp83867->clk_output_sel != DP83867_CLK_O_SEL_OFF) { + phydev_err(phydev, "ti,clk-output-sel value %u out of range\n", + dp83867->clk_output_sel); + return -EINVAL; + } This last bit looks odd. If it is not OFF, it is invalid? Are there any in tree users of DP83867_CLK_O_SEL_REF_CLK? We have to be careful changing its meaning. But if nobody is actually using it... Patch 4: This is harder. Ideally we want to fix this. At some point, somebody is going to want 'rgmii' to actually mean 'rgmii', because that is what their hardware needs. Could you add a WARN_ON() for 'rgmii' but the PHY is actually adding a delay? And add a comment about setting the correct thing in device tree? Hopefully we will then get patches correcting DT blobs. And if we later do need to fix 'rgmii', we will break less board. > >>> Please note that net-next is closed currently. Please resubmit the > >>> patches once it's open again, and please annotate them properly > >>> with net-next. > > > > Sorry, didn't know about this policy. Been years since I've submitted > > net patches. Is there a description somewhere of how this is done? > > Googling net-next wasn't helpful. I gather new patches are only > > allowed when the kernel merge window is open? And they can't be queued > > on patchwork or a topic branch until this happens? You can post patches while it is closed for review, but add "RFC" in the subject so it is clear you just want comments. You will still need to resubmit once it opens up again. Andrew
On Mon, 2019-05-13 at 22:46 +0200, Andrew Lunn wrote: > > > Perhaps you could tell me if the approach I've taken in patch 3, > > > "Add ability to disable output clock", and patch 4, "Disable tx/rx > > > delay when not configured", are considered acceptable? I can conceive > > > of arguments for alternate approaches. I would like to add support for > > > these into u-boot too, but typically u-boot follows the kernel DT > > > bindings, so I want to finalize the kernel DT semantics before sending > > > patches to u-boot. > > > > > > > I lack experience with these TI PHY's. Maybe Andrew or Florian can advise. > > Hi Trent > > I already deleted the patches. For patch 3: > > + if (dp83867->clk_output_sel > DP83867_CLK_O_SEL_REF_CLK && > + dp83867->clk_output_sel != DP83867_CLK_O_SEL_OFF) { > + phydev_err(phydev, "ti,clk-output-sel value %u out of range\n", > + dp83867->clk_output_sel); > + return -EINVAL; > + } > > This last bit looks odd. If it is not OFF, it is invalid? The valid values are in the range 0 to DP83867_CLK_O_SEL_REF_CLK and also DP83867_CLK_O_SEL_OFF. Thus invalid values are those greater than DP83867_CLK_O_SEL_REF_CLK which are not DP83867_CLK_O_SEL_OFF. > > Are there any in tree users of DP83867_CLK_O_SEL_REF_CLK? We have to > be careful changing its meaning. But if nobody is actually using it... Nope. I doubt this will affect anyone. They'd need to strap the phy to get a different configuration, and the explicitly add a property, which isn't in the example DTS files, to change the configuration to something they didn't want, and then depend on a driver bug ignoring the erroneous setting they added. > Patch 4: > > This is harder. Ideally we want to fix this. At some point, somebody > is going to want 'rgmii' to actually mean 'rgmii', because that is > what their hardware needs. > > Could you add a WARN_ON() for 'rgmii' but the PHY is actually adding a > delay? And add a comment about setting the correct thing in device > tree? Hopefully we will then get patches correcting DT blobs. And if > we later do need to fix 'rgmii', we will break less board. Yes I can do this. Should it warn on any use of "rgmii"? If so, how would someone make the warning go away if they actually want rgmii mode with no delay? I suspect hsdk.dts is an example of an in-tree broken board that uses "rgmii" would it should have used "rgmii-id". Unfortunately, phy dts nodes don't usually provide a compatible that identifies the phy, using instead run-time auto-detection based on PHY id registers, so there's no way to tell from the dts files what boards use this phy unless they also specify one of the phy specific properties. Which is how I found hsdk.dts (and no other boards).
> > Hi Trent > > > > I already deleted the patches. For patch 3: > > > > + if (dp83867->clk_output_sel > DP83867_CLK_O_SEL_REF_CLK && > > + dp83867->clk_output_sel != DP83867_CLK_O_SEL_OFF) { > > + phydev_err(phydev, "ti,clk-output-sel value %u out of range\n", > > + dp83867->clk_output_sel); > > + return -EINVAL; > > + } > > > > This last bit looks odd. If it is not OFF, it is invalid? > > The valid values are in the range 0 to DP83867_CLK_O_SEL_REF_CLK and > also DP83867_CLK_O_SEL_OFF. Thus invalid values are those greater than > DP83867_CLK_O_SEL_REF_CLK which are not DP83867_CLK_O_SEL_OFF. Hi Trent O.K. > > Are there any in tree users of DP83867_CLK_O_SEL_REF_CLK? We have to > > be careful changing its meaning. But if nobody is actually using it... > > Nope. I doubt this will affect anyone. They'd need to strap the phy > to get a different configuration, and the explicitly add a property, > which isn't in the example DTS files, to change the configuration to > something they didn't want, and then depend on a driver bug ignoring > the erroneous setting they added. O.K, then this patch is O.K. Does the binding documentation need updating? > > Patch 4: > > > > This is harder. Ideally we want to fix this. At some point, somebody > > is going to want 'rgmii' to actually mean 'rgmii', because that is > > what their hardware needs. > > > > Could you add a WARN_ON() for 'rgmii' but the PHY is actually adding a > > delay? And add a comment about setting the correct thing in device > > tree? Hopefully we will then get patches correcting DT blobs. And if > > we later do need to fix 'rgmii', we will break less board. > > Yes I can do this. Should it warn on any use of "rgmii"? No, i would only warn when there is a delay configured by strapping. If you want the PHY to be left alone, you should use PHY_INTERFACE_MODE_NA, which should be the default if there is no phy-mode property. If DT actually asked for "rgmii", it either means it is wrong and rgmii-id should be used to match the strapping, or both the strapping and the DT is wrong and somebody really does want "rgmii". > If so, how would someone make the warning go away if they actually > want rgmii mode with no delay? We take the warning out, and implement "rgmii" correctly, and let boards break which have broken DT. We have done this before, but without a period of time with a warning. > I suspect hsdk.dts is an example of an in-tree broken board that uses > "rgmii" would it should have used "rgmii-id". O.K, so when you submit the patch Cc: Alexey Brodkin <abrodkin@synopsys.com> Andrew
On Mon, 2019-05-13 at 23:43 +0200, Andrew Lunn wrote: > > > > > > Are there any in tree users of DP83867_CLK_O_SEL_REF_CLK? We have to > > > be careful changing its meaning. But if nobody is actually using it... > > > > Nope. I doubt this will affect anyone. They'd need to strap the phy > > to get a different configuration, and the explicitly add a property, > > which isn't in the example DTS files, to change the configuration to > > something they didn't want, and then depend on a driver bug ignoring > > the erroneous setting they added. > > O.K, then this patch is O.K. Does the binding documentation need > updating? Device tree binding patch was split out of the commit and was patch 2 of the series, https://patchwork.ozlabs.org/patch/1098349/ > > > Patch 4: > > > > > > This is harder. Ideally we want to fix this. At some point, somebody > > > is going to want 'rgmii' to actually mean 'rgmii', because that is > > > what their hardware needs. > > > > > > Could you add a WARN_ON() for 'rgmii' but the PHY is actually adding a > > > delay? And add a comment about setting the correct thing in device > > > tree? Hopefully we will then get patches correcting DT blobs. And if > > > we later do need to fix 'rgmii', we will break less board. > > > > Yes I can do this. Should it warn on any use of "rgmii"? > > No, i would only warn when there is a delay configured by > strapping. If you want the PHY to be left alone, you should use > PHY_INTERFACE_MODE_NA, which should be the default if there is no > phy-mode property. If DT actually asked for "rgmii", it either means > it is wrong and rgmii-id should be used to match the strapping, or > both the strapping and the DT is wrong and somebody really does want > "rgmii". Ok, seems reasonable. I've put in a phydev_warn() when the interface is 'rgmii' and the strapping is set to have a delay. I'm checking the strapping config register for this, rather than the current phy configuration of delay values. The previous behavior of 'rgmii' mode was "keep strapping default", rather than "keep current phy configuration", which isn't exactly the same thing. Here is the message: phydev_warn(phydev, "PHY has delays via pin strapping, but phy-mode = 'rgmii'\n" "Should be 'rgmii-id' to use internal delays\n");
diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c index a46cc9427fb3..edd9e27425e8 100644 --- a/drivers/net/phy/dp83867.c +++ b/drivers/net/phy/dp83867.c @@ -82,9 +82,9 @@ enum { }; struct dp83867_private { - int rx_id_delay; - int tx_id_delay; - int fifo_depth; + u32 rx_id_delay; + u32 tx_id_delay; + u32 fifo_depth; int io_impedance; int port_mirroring; bool rxctrl_strap_quirk;
The variables used to store u32 DT properties were signed ints. This doesn't work properly if the value of the property were to overflow. Use unsigned variables so this doesn't happen. Cc: Andrew Lunn <andrew@lunn.ch> Cc: Florian Fainelli <f.fainelli@gmail.com> Cc: Heiner Kallweit <hkallweit1@gmail.com> Signed-off-by: Trent Piepho <tpiepho@impinj.com> --- drivers/net/phy/dp83867.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)