| Message ID | 1b8db5331638f1380ec2ba6e00235c8d5d7a882c.1697107915.git.ante.knezic@helmholz.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: microchip: enable setting rmii | expand |
On Thu, Oct 12, 2023 at 12:55:56PM +0200, Ante Knezic wrote: > Add documentation for selecting reference rmii clock on KSZ88X3 devices > > Signed-off-by: Ante Knezic <ante.knezic@helmholz.de> > --- > .../devicetree/bindings/net/dsa/microchip,ksz.yaml | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml > index 41014f5c01c4..eaa347b04db1 100644 > --- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml > +++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml > @@ -72,6 +72,25 @@ properties: > interrupts: > maxItems: 1 > > + microchip,rmii-clk-internal: > + $ref: /schemas/types.yaml#/definitions/flag > + description: > + Set if the RMII reference clock is provided internally. Otherwise > + reference clock should be provided externally. I regret not asking this on the previous iteration - how come you need a custom property? In the externally provided case would there not be a clocks property pointing to the RMII reference clock, that would be absent when provided by the itnernal reference? Cheers, Conor. > + > +if: > + not: > + properties: > + compatible: > + enum: > + - microchip,ksz8863 > + - microchip,ksz8873 > +then: > + not: > + required: > + - microchip,rmii-clk-internal > + > + > required: > - compatible > - reg > -- > 2.11.0 >
On Thu, 12 Oct 2023 16:18:09 +0100, Conor Dooley wrote: > On Thu, Oct 12, 2023 at 12:55:56PM +0200, Ante Knezic wrote: > > Add documentation for selecting reference rmii clock on KSZ88X3 devices > > > > Signed-off-by: Ante Knezic <ante.knezic@helmholz.de> > > --- > > .../devicetree/bindings/net/dsa/microchip,ksz.yaml | 19 +++++++++++++++++++ > > 1 file changed, 19 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml > > index 41014f5c01c4..eaa347b04db1 100644 > > --- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml > > +++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml > > @@ -72,6 +72,25 @@ properties: > > interrupts: > > maxItems: 1 > > > > + microchip,rmii-clk-internal: > > + $ref: /schemas/types.yaml#/definitions/flag > > + description: > > + Set if the RMII reference clock is provided internally. Otherwise > > + reference clock should be provided externally. > > I regret not asking this on the previous iteration - how come you need a > custom property? In the externally provided case would there not be a > clocks property pointing to the RMII reference clock, that would be > absent when provided by the itnernal reference? > > Cheers, > Conor. In both cases (external and internal), the KSZ88X3 is actually providing the RMII reference clock. Difference is only will the clock be routed as external copper track (pin REFCLKO -> pin REFCLKI), or will it be routed internally. So, this should not affect the clock relation between the uC and the switch device? This property has no effect if KSZ88X3 is not providing the reference clock. Maybe I should provide more info in the commit message of both patches as well?
On Thu, Oct 12, 2023 at 12:55:56PM +0200, Ante Knezic wrote: > Add documentation for selecting reference rmii clock on KSZ88X3 devices > > Signed-off-by: Ante Knezic <ante.knezic@helmholz.de> > --- > .../devicetree/bindings/net/dsa/microchip,ksz.yaml | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml > index 41014f5c01c4..eaa347b04db1 100644 > --- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml > +++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml > @@ -72,6 +72,25 @@ properties: > interrupts: > maxItems: 1 > > + microchip,rmii-clk-internal: > + $ref: /schemas/types.yaml#/definitions/flag > + description: > + Set if the RMII reference clock is provided internally. Otherwise > + reference clock should be provided externally. > + > +if: > + not: > + properties: > + compatible: > + enum: > + - microchip,ksz8863 > + - microchip,ksz8873 > +then: > + not: > + required: > + - microchip,rmii-clk-internal I think that what you want to express is that microchip,rmii-clk-internal is only defined for microchip,ksz8863 and microchip,ksz8873. Can't you describe that as "if: properties: compatible: (...) then: properties: microchip,rmii-clk-internal"? Also, this is a port interface property, so I would like to see it in the xMII port and not global to the switch. The KSZ8873/KSZ8863 is somewhat special in that it only contains a single xMII port, but that doesn't mean that RMII settings are switch-wide. > + > + > required: > - compatible > - reg > -- > 2.11.0 >
On Mon, Oct 16, 2023 at 09:53:49AM +0200, Ante Knezic wrote: > On Thu, 12 Oct 2023 16:18:09 +0100, Conor Dooley wrote: > > On Thu, Oct 12, 2023 at 12:55:56PM +0200, Ante Knezic wrote: > > > Add documentation for selecting reference rmii clock on KSZ88X3 devices > > > > > > Signed-off-by: Ante Knezic <ante.knezic@helmholz.de> > > > --- > > > .../devicetree/bindings/net/dsa/microchip,ksz.yaml | 19 +++++++++++++++++++ > > > 1 file changed, 19 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml > > > index 41014f5c01c4..eaa347b04db1 100644 > > > --- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml > > > +++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml > > > @@ -72,6 +72,25 @@ properties: > > > interrupts: > > > maxItems: 1 > > > > > > + microchip,rmii-clk-internal: > > > + $ref: /schemas/types.yaml#/definitions/flag > > > + description: > > > + Set if the RMII reference clock is provided internally. Otherwise > > > + reference clock should be provided externally. > > > > I regret not asking this on the previous iteration - how come you need a > > custom property? In the externally provided case would there not be a > > clocks property pointing to the RMII reference clock, that would be > > absent when provided by the itnernal reference? > In both cases (external and internal), the KSZ88X3 is actually providing the > RMII reference clock. > Difference is only will the clock be routed as external > copper track (pin REFCLKO -> pin REFCLKI), or will it be routed internally. The switch always provides it's own external reference, wut? Why would anyone actually bother doing this instead of just using the internal reference? > So, this should not affect the clock relation between the uC and the switch > device? > This property has no effect if KSZ88X3 is not providing the reference clock. This appears to contradict with the above, unless I am misunderstanding something. > Maybe I should provide more info in the commit message of both patches as well? What I would have expected to see is that when the reference clock is provided externally that there would be a clocks property in the DT node, pointing at that external clock & when there was not, then no property. Likely that ship has already said, as I don't see clocks present in the current binding. How does the driver get the frequency of the RMII reference clock when an external reference is provided?
On Tue, 17 Oct 2023 09:00:15 +0100, Conor Dooley wrote: > > In both cases (external and internal), the KSZ88X3 is actually providing the > > RMII reference clock. > > Difference is only will the clock be routed as external > > copper track (pin REFCLKO -> pin REFCLKI), or will it be routed internally. > > The switch always provides it's own external reference, wut? Why would > anyone actually bother doing this instead of just using the internal > reference? Thats a good question... Other KSZ chips don't have the ability to route clock internally, and these two (ksz8863 and ksz8873) are actually by default the expecting to route it externally. Why this is so is a matter of HW design. The KSZ88x3 does not have to provide the reference clock, it can be provided externally, by some other device, for example the uC. BUT in case when it is provided by the switch we have the option to route the internal block that is generating the clock to the clock input externally (as a copper track) or internally. To quote the manual: "When EN_REFCLKO_3 is high, KSZ8863RLL outputs a 50 MHz in REFCLKO_3. Register 198 bit [3] is used to select the internal or external reference clock. Internal reference clock means that the clock for the RMII of KSZ8863RLL is provided by KSZ8863RLL internally and the REFCLKI_3 pin is unconnected. For the external reference clock, the clock provides to KSZ8863RLL via REFCLKI_3. If KSZ8863RLL does not provide the reference clock, this 50 MHz reference clock with divide-by-2 (25 MHz) has to be used in X1 pin instead of the 25 MHz crystal, since the clock skew of these two clock sources impacts the RMII timing before Rev A3 part. The Rev A3 part can connect the external 50 MHz reference clock to X1 pin and SMTXC3/REFCLKI_3 pins directly with strap pins of pin 17 SMTXD33/EN_REFCLKO_3 and pin 18 SMTXD32 to be pulled down." > > So, this should not affect the clock relation between the uC and the switch > > device? > > > This property has no effect if KSZ88X3 is not providing the reference clock. > This appears to contradict with the above, unless I am misunderstanding > something. There are actually 5 RMII clock configuration modes depending on the setting of the EN_REFCLKO_3, register 0xC6 and SMTXD32 pin (which affects the expected clock, 25 or 50 Mhz), but the patch covers the case in which the switch is generating the reference clock (outputing it to REFCLKO_3), because EN_REFCLK0_3 is pulled up. By clearing/setting the 0xC6 bit 3 we can choose whether to connect the REFCLKO to REFCLKI externally, or internally. If reference clock is being provided to the ksz88x3 by some other device, then there is no point in setting the 0xC6 bit 3 because other device is providing the clock to REFCLKI_3 and REFCLKO should not be connected and should not generate the clock (as EN_REFCLKI is pulled down). > What I would have expected to see is that when the reference clock is > provided externally that there would be a clocks property in the DT > node, pointing at that external clock & when there was not, then > no property. Likely that ship has already said, as I don't see clocks > present in the current binding. How does the driver get the frequency of > the RMII reference clock when an external reference is provided? In case when ksz88x3 is generating the reference clock the devices rmii interface should be configured to external clock usage, for example something like rmii-clock-ext for TI cpsw. This should be true regardles of whether the ksz88x3 node has "microchip,rmii-clk-internal" set or not.
> The switch always provides it's own external reference, wut? Why would > anyone actually bother doing this instead of just using the internal > reference? I think you are getting provider and consumer mixed up. Lets simplify to just a MAC and a PHY. There needs to be a shared clock between these two. Sometimes the PHY is the provider and the MAC is the consumer, sometimes the MAC is the provider, and the PHY is the consumer. Sometimes the hardware gives you no choices, sometimes it does. Sometimes a third party provides the clock, and both are consumers. With the KSZ, we are talking about a switch, so there are multiple MACs and PHYs. They can all share the same clock, so long as you have one provider, and the rest are consumers. Or each pair can figure out its provider/consumer etc. How this is described in DT has evolved over time. We don't have clean clock provider/consumer relationships. The PHYs and MACs are generally not CCF consumers/providers. They just have a property to enable the to output a clock, or maybe a property to disable the clock output in order to save power. There are a few exceptions, but that tends to be where the clock provider is already CCF clock, e.g. a SoC clock. Andrew
On Tue, Oct 17, 2023 at 02:59:46PM +0200, Andrew Lunn wrote: > > The switch always provides it's own external reference, wut? Why would > > anyone actually bother doing this instead of just using the internal > > reference? > > I think you are getting provider and consumer mixed up. The comment suggested that it was acting as both, via an external loopback: > > > In both cases (external and internal), the KSZ88X3 is actually providing the > > > RMII reference clock. Difference is only will the clock be routed as external > > > copper track (pin REFCLKO -> pin REFCLKI), or will it be routed internally. If there's another interpretation for that, it's lost on me. A later mail goes on to say: > > > The KSZ88x3 does not have to provide the reference clock, it can be provided > > > externally, by some other device, for example the uC. So I think I was just picking up on a mistaken explanation. > Lets simplify to just a MAC and a PHY. There needs to be a shared > clock between these two. Sometimes the PHY is the provider and the MAC > is the consumer, sometimes the MAC is the provider, and the PHY is the > consumer. Sometimes the hardware gives you no choices, sometimes it > does. Sometimes a third party provides the clock, and both are > consumers. > > With the KSZ, we are talking about a switch, so there are multiple > MACs and PHYs. They can all share the same clock, so long as you have > one provider, and the rest are consumers. Or each pair can figure out > its provider/consumer etc. Thanks for the explanation. I'm still not really sure why someone would want to employ external loopback, instead of the internal one though. > How this is described in DT has evolved over time. We don't have clean > clock provider/consumer relationships. The PHYs and MACs are generally > not CCF consumers/providers. They just have a property to enable the > to output a clock, or maybe a property to disable the clock output in > order to save power. There are a few exceptions, but that tends to be > where the clock provider is already CCF clock, e.g. a SoC clock. Yeah, I did acknowledge that at the end of my mail (although I managed to typo "that ship has sailed"). Doing ccf stuff doesn't seem viable given there's currently no required clocks in the binding despite there likely being some in use. I'm fine acking the binding with the change I suggested, I was just looking to understand why a clocks property could not be used and I think I have my answer to that now :) Cheers, Conor.
diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml index 41014f5c01c4..eaa347b04db1 100644 --- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml +++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml @@ -72,6 +72,25 @@ properties: interrupts: maxItems: 1 + microchip,rmii-clk-internal: + $ref: /schemas/types.yaml#/definitions/flag + description: + Set if the RMII reference clock is provided internally. Otherwise + reference clock should be provided externally. + +if: + not: + properties: + compatible: + enum: + - microchip,ksz8863 + - microchip,ksz8873 +then: + not: + required: + - microchip,rmii-clk-internal + + required: - compatible - reg
Add documentation for selecting reference rmii clock on KSZ88X3 devices Signed-off-by: Ante Knezic <ante.knezic@helmholz.de> --- .../devicetree/bindings/net/dsa/microchip,ksz.yaml | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)