| Message ID | 20220410104626.11517-2-josua@solid-run.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | adin: add support for 125MHz clk-out | expand |
On 10/04/2022 12:46, Josua Mayer wrote: > The ADIN1300 supports generating certain clocks on its GP_CLK pin. > Add a DT property to specify the frequency. > > Due to the complexity of the clock configuration register, for now only > 125MHz is documented. Thank you for your patch. There is something to discuss/improve. Adjust subject prefix to the subsystem (dt-bindings, not dt, missing net). > > Signed-off-by: Josua Mayer <josua@solid-run.com> > --- > Documentation/devicetree/bindings/net/adi,adin.yaml | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/Documentation/devicetree/bindings/net/adi,adin.yaml b/Documentation/devicetree/bindings/net/adi,adin.yaml > index 1129f2b58e98..4e421bf5193d 100644 > --- a/Documentation/devicetree/bindings/net/adi,adin.yaml > +++ b/Documentation/devicetree/bindings/net/adi,adin.yaml > @@ -36,6 +36,11 @@ properties: > enum: [ 4, 8, 12, 16, 20, 24 ] > default: 8 > > + adi,clk-out-frequency: Use types defined by the dtschema, so "adi,clk-out-hz". Then no need for type/ref. > + description: Clock output frequency in Hertz. > + $ref: /schemas/types.yaml#/definitions/uint32 > + enum: [125000000] If only one value, then "const: 125000000", but why do you need such value in DT if it is always the same? Best regards, Krzysztof
\o/ Thank you for the quick reply! Am 10.04.22 um 17:21 schrieb Krzysztof Kozlowski: > On 10/04/2022 12:46, Josua Mayer wrote: >> The ADIN1300 supports generating certain clocks on its GP_CLK pin. >> Add a DT property to specify the frequency. >> >> Due to the complexity of the clock configuration register, for now only >> 125MHz is documented. > Thank you for your patch. There is something to discuss/improve. > > Adjust subject prefix to the subsystem (dt-bindings, not dt, missing net). Ack. So something like dt-bindings: net: adin: document clk-out property >> Signed-off-by: Josua Mayer <josua@solid-run.com> >> --- >> Documentation/devicetree/bindings/net/adi,adin.yaml | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/net/adi,adin.yaml b/Documentation/devicetree/bindings/net/adi,adin.yaml >> index 1129f2b58e98..4e421bf5193d 100644 >> --- a/Documentation/devicetree/bindings/net/adi,adin.yaml >> +++ b/Documentation/devicetree/bindings/net/adi,adin.yaml >> @@ -36,6 +36,11 @@ properties: >> enum: [ 4, 8, 12, 16, 20, 24 ] >> default: 8 >> >> + adi,clk-out-frequency: > Use types defined by the dtschema, so "adi,clk-out-hz". Then no need for > type/ref. That sounds useful, I was not aware. The only inspiration I used was the at803x driver ... It seemed natural to share the property name as it serves the same purpose here. >> + description: Clock output frequency in Hertz. >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + enum: [125000000] > If only one value, then "const: 125000000", but why do you need such > value in DT if it is always the same? Yes yes yes. From my understanding of the adin1300 data-sheet, it can provide either a 25MHz clock or a 125MHz clock on its GP_CLK pin. So for the context of this feature we would have two options. However because we found the documentation very confusing we skipped the 25MHz option. Actually my statement above omits some of the options. - There are actually two 125MHz clocks, the first called "recovered" and the second "free running". - One can let the phy choose the rate based on its internal state. This is indicated on page 73 of the datasheet (https://www.analog.com/media/en/technical-documentation/data-sheets/adin1300.pdf) Because of this confusion we wanted to for now only allow selecting the free-running 125MHz clock. Sincerely Josua Mayer
On 10/04/2022 20:41, Josua Mayer wrote: >> >> Adjust subject prefix to the subsystem (dt-bindings, not dt, missing net). > Ack. So something like > dt-bindings: net: adin: document clk-out property Yes. >>> Signed-off-by: Josua Mayer <josua@solid-run.com> >>> --- >>> Documentation/devicetree/bindings/net/adi,adin.yaml | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/net/adi,adin.yaml b/Documentation/devicetree/bindings/net/adi,adin.yaml >>> index 1129f2b58e98..4e421bf5193d 100644 >>> --- a/Documentation/devicetree/bindings/net/adi,adin.yaml >>> +++ b/Documentation/devicetree/bindings/net/adi,adin.yaml >>> @@ -36,6 +36,11 @@ properties: >>> enum: [ 4, 8, 12, 16, 20, 24 ] >>> default: 8 >>> >>> + adi,clk-out-frequency: >> Use types defined by the dtschema, so "adi,clk-out-hz". Then no need for >> type/ref. > That sounds useful, I was not aware. The only inspiration I used was the > at803x driver ... > It seemed natural to share the property name as it serves the same > purpose here. Indeed ar803x uses such property. In general reusing properties is a good idea, but not all properties are good enough to copy. I don't know why adi,clk-out-frequency got accepted because we really stick to common units when possible. https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml >>> + description: Clock output frequency in Hertz. >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + enum: [125000000] >> If only one value, then "const: 125000000", but why do you need such >> value in DT if it is always the same? > Yes yes yes. > From my understanding of the adin1300 data-sheet, it can provide either > a 25MHz clock or a 125MHz clock on its GP_CLK pin. So for the context of > this feature we would have two options. However because we found the > documentation very confusing we skipped the 25MHz option. > > Actually my statement above omits some of the options. > - There are actually two 125MHz clocks, the first called "recovered" and > the second "free running". > - One can let the phy choose the rate based on its internal state. > This is indicated on page 73 of the datasheet > (https://www.analog.com/media/en/technical-documentation/data-sheets/adin1300.pdf) > > Because of this confusion we wanted to for now only allow selecting the > free-running 125MHz clock. Hm, so you do not insist on actual frequency but rather type of the clock (freerunning instead of recovered and 25 MHz). Then the frequency does not look enough because it does not offer you the choice of clock (freerunning or recovered) and instead you could have enum like: adi,phy-output-clock: $ref: /schemas/types.yaml#/definitions/string enum: [125mhz-freerunning, 125mhz-recovered, 25mhz-freeruning....] Judging by page 24 you have 5 or more options... This could be also numeric ID (enum [0, 1, 2, 3 ...]) with some explanation, but strings seem easier to understand. The binding should describe the hardware, not implementation in Linux, therefore you should actually list all possible choices. The driver then can just return EINVAL on unsupported choices (or map them back to only one supported). Best regards, Krzysztof
\o/ Am 10.04.22 um 22:01 schrieb Krzysztof Kozlowski: > On 10/04/2022 20:41, Josua Mayer wrote: >>> Adjust subject prefix to the subsystem (dt-bindings, not dt, missing net). >> Ack. So something like >> dt-bindings: net: adin: document clk-out property > Yes. Great, I will have it changed in a future revision! >>>> Signed-off-by: Josua Mayer <josua@solid-run.com> >>>> --- >>>> Documentation/devicetree/bindings/net/adi,adin.yaml | 5 +++++ >>>> 1 file changed, 5 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/net/adi,adin.yaml b/Documentation/devicetree/bindings/net/adi,adin.yaml >>>> index 1129f2b58e98..4e421bf5193d 100644 >>>> --- a/Documentation/devicetree/bindings/net/adi,adin.yaml >>>> +++ b/Documentation/devicetree/bindings/net/adi,adin.yaml >>>> @@ -36,6 +36,11 @@ properties: >>>> enum: [ 4, 8, 12, 16, 20, 24 ] >>>> default: 8 >>>> >>>> + adi,clk-out-frequency: >>> Use types defined by the dtschema, so "adi,clk-out-hz". Then no need for >>> type/ref. >> That sounds useful, I was not aware. The only inspiration I used was the >> at803x driver ... >> It seemed natural to share the property name as it serves the same >> purpose here. > Indeed ar803x uses such property. In general reusing properties is a > good idea, but not all properties are good enough to copy. I don't know > why adi,clk-out-frequency got accepted because we really stick to common > units when possible. > > https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml > >>>> + description: Clock output frequency in Hertz. >>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>> + enum: [125000000] >>> If only one value, then "const: 125000000", but why do you need such >>> value in DT if it is always the same? >> Yes yes yes. >> From my understanding of the adin1300 data-sheet, it can provide either >> a 25MHz clock or a 125MHz clock on its GP_CLK pin. So for the context of >> this feature we would have two options. However because we found the >> documentation very confusing we skipped the 25MHz option. >> >> Actually my statement above omits some of the options. >> - There are actually two 125MHz clocks, the first called "recovered" and >> the second "free running". >> - One can let the phy choose the rate based on its internal state. >> This is indicated on page 73 of the datasheet >> (https://www.analog.com/media/en/technical-documentation/data-sheets/adin1300.pdf) >> >> Because of this confusion we wanted to for now only allow selecting the >> free-running 125MHz clock. > Hm, so you do not insist on actual frequency but rather type of the > clock (freerunning instead of recovered and 25 MHz). Then the frequency > does not look enough because it does not offer you the choice of clock > (freerunning or recovered) and instead you could have enum like: > adi,phy-output-clock: > $ref: /schemas/types.yaml#/definitions/string > enum: [125mhz-freerunning, 125mhz-recovered, 25mhz-freeruning....] > > Judging by page 24 you have 5 or more options... This could be also > numeric ID (enum [0, 1, 2, 3 ...]) with some explanation, but strings > seem easier to understand. I agree that strings are more meaningful here, especially considering how each entry carries at least two pieces of information. If we are not to reuse the qca,clk-out-frequency name, then an enum seems the easiest way to describe the available settings from the clock config register! > The binding should describe the hardware, not implementation in Linux, > therefore you should actually list all possible choices. The driver then > can just return EINVAL on unsupported choices (or map them back to only > one supported). I have prepared a draft for the entries that should exist, it covers five of the 6 available bits. Maybe you can comment if this is understandable? 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 Bit no. 3 (GE_REF_CLK_EN) is special in that it can be enabled independently from the 5 choices above, and it controls a different pin. Therefore it deserves its own property, perhaps a flag or boolean adi,phy-output-ref-clock. Any opinion if this should be added, or we can omit it completely? sincerely Josua Mayer
On Mon, 11 Apr 2022 10:42:18 +0300 Josua Mayer wrote: > > The binding should describe the hardware, not implementation in Linux, > > therefore you should actually list all possible choices. The driver then > > can just return EINVAL on unsupported choices (or map them back to only > > one supported). > > I have prepared a draft for the entries that should exist, it covers > five of the 6 available bits. Maybe you can comment if this is > understandable? > > 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 > > Bit no. 3 (GE_REF_CLK_EN) is special in that it can be enabled > independently from the 5 choices above, > and it controls a different pin. Therefore it deserves its own property, > perhaps a flag or boolean adi,phy-output-ref-clock. > Any opinion if this should be added, or we can omit it completely? Noob question - can you explain how this property describes HW? I thought we had a framework for clock config, and did not require vendor specific properties of this sort. The recovered vs free running makes the entire thing sound like a SyncE related knob, irrelevant to normal HW operation.
> Noob question - can you explain how this property describes HW? > I thought we had a framework for clock config, and did not require > vendor specific properties of this sort. > > The recovered vs free running makes the entire thing sound like > a SyncE related knob, irrelevant to normal HW operation. It is not necessarily SyncE. Fast Ethernet is based around a 25MHz clock. Something needs to provide that clock. Sometimes the SoC/MAC provides it, and passes it to the PHY. Sometimes the PHY provides it, and passes it to the SoC/MAC. There are a couple of PHYs which make use of the common clock framework, when the SoC is the clock source. However, i don't think there are any PHYs which provide a clock to the common clock framework when they are the source. We do however have a number of vendor properties to control the PHY clock output, disable the PHY clock output, select the PHY clock output, etc. There is not too much standardisation here, and it is made worse by some PHYs needing a reset once the clock is ticking, some MACs stop the clock when the link is administrative down, some PHYs stop the clock a short time after the link goes down which can be bad for the MAC etc. Andrew
On Mon, 11 Apr 2022 22:59:59 +0200 Andrew Lunn wrote: > > Noob question - can you explain how this property describes HW? > > I thought we had a framework for clock config, and did not require > > vendor specific properties of this sort. > > > > The recovered vs free running makes the entire thing sound like > > a SyncE related knob, irrelevant to normal HW operation. > > It is not necessarily SyncE. Fast Ethernet is based around a 25MHz > clock. Something needs to provide that clock. Sometimes the SoC/MAC > provides it, and passes it to the PHY. Sometimes the PHY provides it, > and passes it to the SoC/MAC. > > There are a couple of PHYs which make use of the common clock > framework, when the SoC is the clock source. However, i don't think > there are any PHYs which provide a clock to the common clock framework > when they are the source. We do however have a number of vendor > properties to control the PHY clock output, disable the PHY clock > output, select the PHY clock output, etc. There is not too much > standardisation here, and it is made worse by some PHYs needing a > reset once the clock is ticking, some MACs stop the clock when the > link is administrative down, some PHYs stop the clock a short time > after the link goes down which can be bad for the MAC etc. I see. Why would the MAC/SoC care if the clock is recovered or free running, tho?
On Mon, Apr 11, 2022 at 02:33:15PM -0700, Jakub Kicinski wrote: > On Mon, 11 Apr 2022 22:59:59 +0200 Andrew Lunn wrote: > > > Noob question - can you explain how this property describes HW? > > > I thought we had a framework for clock config, and did not require > > > vendor specific properties of this sort. > > > > > > The recovered vs free running makes the entire thing sound like > > > a SyncE related knob, irrelevant to normal HW operation. > > > > It is not necessarily SyncE. Fast Ethernet is based around a 25MHz > > clock. Something needs to provide that clock. Sometimes the SoC/MAC > > provides it, and passes it to the PHY. Sometimes the PHY provides it, > > and passes it to the SoC/MAC. > > > > There are a couple of PHYs which make use of the common clock > > framework, when the SoC is the clock source. However, i don't think > > there are any PHYs which provide a clock to the common clock framework > > when they are the source. We do however have a number of vendor > > properties to control the PHY clock output, disable the PHY clock > > output, select the PHY clock output, etc. There is not too much > > standardisation here, and it is made worse by some PHYs needing a > > reset once the clock is ticking, some MACs stop the clock when the > > link is administrative down, some PHYs stop the clock a short time > > after the link goes down which can be bad for the MAC etc. > > I see. Why would the MAC/SoC care if the clock is recovered or free > running, tho? Autoneg determines which link peer will be the master and which will be the slave. The master provides the clock for the link. So the slave PHY might want to pass through the recovered clock so it does not need to deal with drift between the recovered clock and its own clock. Andrew
diff --git a/Documentation/devicetree/bindings/net/adi,adin.yaml b/Documentation/devicetree/bindings/net/adi,adin.yaml index 1129f2b58e98..4e421bf5193d 100644 --- a/Documentation/devicetree/bindings/net/adi,adin.yaml +++ b/Documentation/devicetree/bindings/net/adi,adin.yaml @@ -36,6 +36,11 @@ properties: enum: [ 4, 8, 12, 16, 20, 24 ] default: 8 + adi,clk-out-frequency: + description: Clock output frequency in Hertz. + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [125000000] + unevaluatedProperties: false examples:
The ADIN1300 supports generating certain clocks on its GP_CLK pin. Add a DT property to specify the frequency. Due to the complexity of the clock configuration register, for now only 125MHz is documented. Signed-off-by: Josua Mayer <josua@solid-run.com> --- Documentation/devicetree/bindings/net/adi,adin.yaml | 5 +++++ 1 file changed, 5 insertions(+)