| Message ID | 20220509143635.26233-2-josua@solid-run.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | adin: add support for clock output | expand |
On 09/05/2022 16:36, Josua Mayer wrote: > The ADIN1300 supports generating certain clocks on its GP_CLK pin, as > well as providing the reference clock on CLK25_REF. > > Add DT properties to configure both pins. > > Signed-off-by: Josua Mayer <josua@solid-run.com> Don't attach new patchsets to some old patchsets. Basically this makes the thread buried deep and possible ignored. > --- > V3 -> V4: changed type of adi,phy-output-reference-clock to boolean > V1 -> V2: changed clkout property to enum > V1 -> V2: added property for CLK25_REF pin > > .../devicetree/bindings/net/adi,adin.yaml | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On Mon, 9 May 2022 17:36:33 +0300 Josua Mayer wrote: > + adi,phy-output-clock: > + description: Select clock output on GP_CLK pin. Three clocks are available: > + A 25MHz reference, a free-running 125MHz and a recovered 125MHz. > + The phy can also automatically switch between the reference and the > + respective 125MHz clocks based on its internal state. > + $ref: /schemas/types.yaml#/definitions/string > + enum: > + - 25mhz-reference > + - 125mhz-free-running > + - 125mhz-recovered > + - adaptive-free-running > + - adaptive-recovered I'm still not convinced that exposing the free running vs recovered distinction from the start is a good idea. Intuitively it'd seem that it's better to use the recovered clock to feed the wire side of the MAC, this patch set uses the free running. So I'd personally strip the last part off and add it later if needed. But I won't fuss, if we get an ack from one of the PHY maintainers - I'll merge as is. Andrew?
> On Mon, 9 May 2022 17:36:33 +0300 Josua Mayer wrote: > > + adi,phy-output-clock: > > + description: Select clock output on GP_CLK pin. Three clocks are available: > > + A 25MHz reference, a free-running 125MHz and a recovered 125MHz. > > + The phy can also automatically switch between the reference and the > > + respective 125MHz clocks based on its internal state. > > + $ref: /schemas/types.yaml#/definitions/string > > + enum: > > + - 25mhz-reference > > + - 125mhz-free-running > > + - 125mhz-recovered > > + - adaptive-free-running > > + - adaptive-recovered > > I'm still not convinced that exposing the free running vs recovered > distinction from the start is a good idea. Intuitively it'd seem that > it's better to use the recovered clock to feed the wire side of the MAC, > this patch set uses the free running. So I'd personally strip the last > part off and add it later if needed. FWIW, the recovered clock only works if there is a link. AFAIR on the AR8031 you can have the free-running one enabled even if there is no link, which might sometimes be useful. -michael
On Wed, 11 May 2022 14:58:55 +0200 Michael Walle wrote: > > I'm still not convinced that exposing the free running vs recovered > > distinction from the start is a good idea. Intuitively it'd seem that > > it's better to use the recovered clock to feed the wire side of the MAC, > > this patch set uses the free running. So I'd personally strip the last > > part off and add it later if needed. > > FWIW, the recovered clock only works if there is a link. AFAIR on the > AR8031 you can have the free-running one enabled even if there is no > link, which might sometimes be useful. Is that true for all PHYs? I've seen "larger" devices mention holdover or some other form of automatic fallback in the DPLL if input clock is lost. I thought that's the case here, too: > > > + The phy can also automatically switch between the reference and the > > > + respective 125MHz clocks based on its internal state.
Am 2022-05-11 18:11, schrieb Jakub Kicinski: > On Wed, 11 May 2022 14:58:55 +0200 Michael Walle wrote: >> > I'm still not convinced that exposing the free running vs recovered >> > distinction from the start is a good idea. Intuitively it'd seem that >> > it's better to use the recovered clock to feed the wire side of the MAC, >> > this patch set uses the free running. So I'd personally strip the last >> > part off and add it later if needed. >> >> FWIW, the recovered clock only works if there is a link. AFAIR on the >> AR8031 you can have the free-running one enabled even if there is no >> link, which might sometimes be useful. > > Is that true for all PHYs? I've seen "larger" devices mention holdover > or some other form of automatic fallback in the DPLL if input clock is > lost. I certainly can't speak of 'all' PHYs, who can ;) But how is the switchover for example? hitless? will there be a brief period of no clock at all? The point I wanted to add is that the user should have the choice or at least you should clearly mention that. If you drop the suffix and just use "25mhz" is that now the recovered one or the free-running one. And why is that one preferred over the other? Eg. if I were a designer for a cheapo board and I'd need a 125MHz clock and want to save some bucks for the 125MHz osc and the PHY could supply one, I'd use the free-running mode. Just to avoid any surprises with a switchover or whatever. > I thought that's the case here, too: > >> > > + The phy can also automatically switch between the reference and the >> > > + respective 125MHz clocks based on its internal state. I read "reference" as being the 25MHz (maybe when the PHY is in 10Mbit mode? I didn't read the datasheet) because the first mode is called 25mhz-reference. So it might be switching between 25MHz and 125MHz? I don't know. -michael
On Wed, 11 May 2022 19:10:36 +0200 Michael Walle wrote: > Am 2022-05-11 18:11, schrieb Jakub Kicinski: > > On Wed, 11 May 2022 14:58:55 +0200 Michael Walle wrote: > >> FWIW, the recovered clock only works if there is a link. AFAIR on the > >> AR8031 you can have the free-running one enabled even if there is no > >> link, which might sometimes be useful. > > > > Is that true for all PHYs? I've seen "larger" devices mention holdover > > or some other form of automatic fallback in the DPLL if input clock is > > lost. > > I certainly can't speak of 'all' PHYs, who can ;) But how is the > switchover for example? hitless? will there be a brief period of > no clock at all? > > The point I wanted to add is that the user should have the choice or > at least you should clearly mention that. If you drop the suffix and > just > use "25mhz" is that now the recovered one or the free-running one. And > why is that one preferred over the other? Eg. if I were a designer for a > cheapo board and I'd need a 125MHz clock and want to save some bucks > for the 125MHz osc and the PHY could supply one, I'd use the > free-running mode. Just to avoid any surprises with a switchover > or whatever. It's pure speculation on my side. I don't even know if PHYs use the recovered clock to clock its output towards the MAC or that's a different clock domain. My concern is that people will start to use DT to configure SyncE which is entirely a runtime-controllable thing, and doesn't belong. Hence my preference to hide the recovered vs free-running detail if we can pick one that makes most sense for now. Again, if you feel strongly that the current design is indeed needed to configure pure SOC<>PHY / MAC<>PHY clocking, just send a review tag and I'll apply :) > > I thought that's the case here, too: > > I read "reference" as being the 25MHz (maybe when the PHY is in 10Mbit > mode? I didn't read the datasheet) because the first mode is called > 25mhz-reference. So it might be switching between 25MHz and 125MHz? > I don't know. I couldn't grok that from the datasheet. Anyway, it'd be good to make it clearer that the second sentence refers to the "adaptive" mode and not the behavior of the recovered clock when lock is lost. Just put (adaptive) in the sentence somewhere.
Am 2022-05-11 21:42, schrieb Jakub Kicinski: > On Wed, 11 May 2022 19:10:36 +0200 Michael Walle wrote: >> Am 2022-05-11 18:11, schrieb Jakub Kicinski: >> > On Wed, 11 May 2022 14:58:55 +0200 Michael Walle wrote: >> >> FWIW, the recovered clock only works if there is a link. AFAIR on the >> >> AR8031 you can have the free-running one enabled even if there is no >> >> link, which might sometimes be useful. >> > >> > Is that true for all PHYs? I've seen "larger" devices mention holdover >> > or some other form of automatic fallback in the DPLL if input clock is >> > lost. >> >> I certainly can't speak of 'all' PHYs, who can ;) But how is the >> switchover for example? hitless? will there be a brief period of >> no clock at all? >> >> The point I wanted to add is that the user should have the choice or >> at least you should clearly mention that. If you drop the suffix and >> just >> use "25mhz" is that now the recovered one or the free-running one. And >> why is that one preferred over the other? Eg. if I were a designer for >> a >> cheapo board and I'd need a 125MHz clock and want to save some bucks >> for the 125MHz osc and the PHY could supply one, I'd use the >> free-running mode. Just to avoid any surprises with a switchover >> or whatever. > > It's pure speculation on my side. I don't even know if PHYs use > the recovered clock to clock its output towards the MAC or that's > a different clock domain. > > My concern is that people will start to use DT to configure SyncE which > is entirely a runtime-controllable thing, and doesn't belong. Hence > my preference to hide the recovered vs free-running detail if we can > pick one that makes most sense for now. I see. That makes sense, but then wouldn't it make more sense to pick the (simple) free-running one? As for SyncE you'd need the recovered clock. > Again, if you feel strongly that the current design is indeed needed > to configure pure SOC<>PHY / MAC<>PHY clocking, just send a review tag > and I'll apply :) I just wanted to add my two cents :) I'm fine with either one. >> > I thought that's the case here, too: >> >> I read "reference" as being the 25MHz (maybe when the PHY is in 10Mbit >> mode? I didn't read the datasheet) because the first mode is called >> 25mhz-reference. So it might be switching between 25MHz and 125MHz? >> I don't know. > > I couldn't grok that from the datasheet. Anyway, it'd be good to make > it clearer that the second sentence refers to the "adaptive" mode and > not the behavior of the recovered clock when lock is lost. Just put > (adaptive) in the sentence somewhere. Mh, there is not much there, whatever "heartbeat clock" means. -michael
On Thu, 12 May 2022 23:20:18 +0200 Michael Walle wrote: > > It's pure speculation on my side. I don't even know if PHYs use > > the recovered clock to clock its output towards the MAC or that's > > a different clock domain. > > > > My concern is that people will start to use DT to configure SyncE which > > is entirely a runtime-controllable thing, and doesn't belong. Hence > > my preference to hide the recovered vs free-running detail if we can > > pick one that makes most sense for now. > > I see. That makes sense, but then wouldn't it make more sense to pick > the (simple) free-running one? As for SyncE you'd need the recovered > clock. Sounds good.
\o/ I am not sure I can follow your conversation here ... Am 13.05.22 um 01:44 schrieb Jakub Kicinski: > On Thu, 12 May 2022 23:20:18 +0200 Michael Walle wrote: >>> It's pure speculation on my side. I don't even know if PHYs use >>> the recovered clock to clock its output towards the MAC or that's >>> a different clock domain. >>> >>> My concern is that people will start to use DT to configure SyncE which >>> is entirely a runtime-controllable thing, and doesn't belong. Okay. However phy drivers do not seem to implement runtime control of those clock output pins currently, so they are configured once in DT. >>> Hence >>> my preference to hide the recovered vs free-running detail if we can >>> pick one that makes most sense for now. I am not a fan of hiding information. The clock configuration register clearly supports this distinction. Is this a political stance to say users may not "accidentally" enable SyncE by patching DT? If so we should print a warning message when someone selects it? >> >> I see. That makes sense, but then wouldn't it make more sense to pick >> the (simple) free-running one? As for SyncE you'd need the recovered >> clock. > > Sounds good. Yep, it seems recovered clock is only for SyncE - and only if there is a master clock on the network. Sadly however documentation is sparse and I do not know if the adi phys would fall back to using their internal clock, or just refuse to operate at all.
On Sun, 15 May 2022 10:16:47 +0300 Josua Mayer wrote: > Am 13.05.22 um 01:44 schrieb Jakub Kicinski: > > On Thu, 12 May 2022 23:20:18 +0200 Michael Walle wrote: > >>> It's pure speculation on my side. I don't even know if PHYs use > >>> the recovered clock to clock its output towards the MAC or that's > >>> a different clock domain. > >>> > >>> My concern is that people will start to use DT to configure SyncE which > >>> is entirely a runtime-controllable thing, and doesn't belong. > Okay. > However phy drivers do not seem to implement runtime control of those > clock output pins currently, so they are configured once in DT. To me that means nobody needs the recovered clock. > >>> Hence > >>> my preference to hide the recovered vs free-running detail if we can > >>> pick one that makes most sense for now. > I am not a fan of hiding information. The clock configuration register > clearly supports this distinction. Unless you expose all registers as a direct API to the user you'll be "hiding information". I don't think we are exposing all possible registers for this PHY, the two bits in question are no different. > Is this a political stance to say users may not "accidentally" enable > SyncE by patching DT? > If so we should print a warning message when someone selects it? Why would we add a feature and then print a warning? We can always add the support later, once we have a use case for it. > >> I see. That makes sense, but then wouldn't it make more sense to pick > >> the (simple) free-running one? As for SyncE you'd need the recovered > >> clock. > > > > Sounds good. > > Yep, it seems recovered clock is only for SyncE - and only if there is a > master clock on the network. Sadly however documentation is sparse and I > do not know if the adi phys would fall back to using their internal > clock, or just refuse to operate at all.
Am 16.05.22 um 20:43 schrieb Jakub Kicinski: > On Sun, 15 May 2022 10:16:47 +0300 Josua Mayer wrote: >> Am 13.05.22 um 01:44 schrieb Jakub Kicinski: >>> On Thu, 12 May 2022 23:20:18 +0200 Michael Walle wrote: >>>>> It's pure speculation on my side. I don't even know if PHYs use >>>>> the recovered clock to clock its output towards the MAC or that's >>>>> a different clock domain. >>>>> >>>>> My concern is that people will start to use DT to configure SyncE which >>>>> is entirely a runtime-controllable thing, and doesn't belong. >> Okay. >> However phy drivers do not seem to implement runtime control of those >> clock output pins currently, so they are configured once in DT. > To me that means nobody needs the recovered clock. Doesn't need it, or is overwhelmed by the idea of figuring out how to implement it properly. >>>>> Hence >>>>> my preference to hide the recovered vs free-running detail if we can >>>>> pick one that makes most sense for now. >> I am not a fan of hiding information. The clock configuration register >> clearly supports this distinction. > Unless you expose all registers as a direct API to the user you'll be > "hiding information". I don't think we are exposing all possible > registers for this PHY, the two bits in question are no different. > >> Is this a political stance to say users may not "accidentally" enable >> SyncE by patching DT? >> If so we should print a warning message when someone selects it? > Why would we add a feature and then print a warning? We can always add > the support later, once we have a use case for it. I would not call it a feature. We can e.g. not print a warning, and instead put in the DT binding a note that the recovered variants are for SyncE which Linux does not support. As to why we would add the -recovered options, for starters this allows curious developers to search for the term to get an idea which PHYs would technically support it. That it would also allow tinkering with SyncE to me is a plus, but for you clearly a minus, and I can not make a strong case. So I can imagine to change the bindings as follows: 1. remove the -recovered variants 2. add an explicit note in the commit message that the recovered clock is not implemented because we do not have infrastructure for SyncE 3. keep the -free-running suffix, we should imo only hide it on the day SyncE can be toggled by another means. > I see. That makes sense, but then wouldn't it make more sense to pick > the (simple) free-running one? As for SyncE you'd need the recovered > clock. >>> Sounds good. >> Yep, it seems recovered clock is only for SyncE - and only if there is a >> master clock on the network. Sadly however documentation is sparse and I >> do not know if the adi phys would fall back to using their internal >> clock, or just refuse to operate at all.
On Mon, 16 May 2022 22:48:20 +0300 Josua Mayer wrote: > So I can imagine to change the bindings as follows: > 1. remove the -recovered variants > 2. add an explicit note in the commit message that the recovered clock > is not implemented because we do not have infrastructure for SyncE > 3. keep the -free-running suffix, we should imo only hide it on the day > SyncE can be toggled by another means. SGTM, thanks!
Am 17.05.22 um 01:40 schrieb Jakub Kicinski: > On Mon, 16 May 2022 22:48:20 +0300 Josua Mayer wrote: >> So I can imagine to change the bindings as follows: >> 1. remove the -recovered variants >> 2. add an explicit note in the commit message that the recovered clock >> is not implemented because we do not have infrastructure for SyncE >> 3. keep the -free-running suffix, we should imo only hide it on the day >> SyncE can be toggled by another means. > > SGTM, thanks! Thank you for your comments, I am sending v5 shortly!
diff --git a/Documentation/devicetree/bindings/net/adi,adin.yaml b/Documentation/devicetree/bindings/net/adi,adin.yaml index 1129f2b58e98..cc4f128222ba 100644 --- a/Documentation/devicetree/bindings/net/adi,adin.yaml +++ b/Documentation/devicetree/bindings/net/adi,adin.yaml @@ -36,6 +36,23 @@ properties: enum: [ 4, 8, 12, 16, 20, 24 ] default: 8 + adi,phy-output-clock: + description: Select clock output on GP_CLK pin. Three clocks are available: + A 25MHz reference, a free-running 125MHz and a recovered 125MHz. + The phy can also automatically switch between the reference and the + respective 125MHz clocks based on its internal state. + $ref: /schemas/types.yaml#/definitions/string + enum: + - 25mhz-reference + - 125mhz-free-running + - 125mhz-recovered + - adaptive-free-running + - adaptive-recovered + + adi,phy-output-reference-clock: + description: Enable 25MHz reference clock output on CLK25_REF pin. + type: boolean + unevaluatedProperties: false examples:
The ADIN1300 supports generating certain clocks on its GP_CLK pin, as well as providing the reference clock on CLK25_REF. Add DT properties to configure both pins. Signed-off-by: Josua Mayer <josua@solid-run.com> --- V3 -> V4: changed type of adi,phy-output-reference-clock to boolean V1 -> V2: changed clkout property to enum V1 -> V2: added property for CLK25_REF pin .../devicetree/bindings/net/adi,adin.yaml | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)