| Message ID | 492ba34018bd5035bcc33402746df121df172f73.1697811160.git.ante.knezic@helmholz.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: microchip: enable setting rmii reference | expand |
On Fri, Oct 20, 2023 at 04:25:04PM +0200, Ante Knezic wrote: > +static void ksz88x3_config_rmii_clk(struct ksz_device *dev) > +{ > + bool rmii_clk_internal; > + > + if (!ksz_is_ksz88x3(dev)) > + return; > + > + rmii_clk_internal = of_property_read_bool(dev->dev->of_node, > + "microchip,rmii-clk-internal"); > + > + ksz_cfg(dev, KSZ88X3_REG_FVID_AND_HOST_MODE, > + KSZ88X3_PORT3_RMII_CLK_INTERNAL, rmii_clk_internal); > +} Sorry, I didn't realize on v3 that you didn't completely apply my feedback on v2. Can "microchip,rmii-clk-internal" be a port device tree property? You have indeed moved its parsing to port code, but it is still located directly under the switch node in the device tree. I'm thinking that if this property was also applicable to other switches with multiple RMII ports, the setting would be per port rather than global.
On Fri, 20 Oct 2023 17:37:59 +0300, Vladimir Oltean wrote: > Sorry, I didn't realize on v3 that you didn't completely apply my > feedback on v2. Can "microchip,rmii-clk-internal" be a port device tree > property? You have indeed moved its parsing to port code, but it is > still located directly under the switch node in the device tree. > > I'm thinking that if this property was also applicable to other switches > with multiple RMII ports, the setting would be per port rather than global. As far as I am aware only the KSZ8863 and KSZ8873 have this property available, but the biggger issue might be in scaling this to port property as the register "Forward Invalid VID Frame and Host Mode" where the setting is applied is located under "Advanced Control Registers" section which is actually global at least looking from the switch point of view. Usually port properties are more applicable when registers in question are located under "Port Registers" section. This is somewhat similar to for example enabling the tail tag mode which is again used only by the port 3 interface and is control from "Global Control 1" register. With this in mind - if you still believe we should move this to port dt property, then should we forbid setting the property for any other port other than port 3, and can/should this be enforced by the dt schema?
On Mon, Oct 23, 2023 at 09:27:00AM +0200, Ante Knezic wrote: > On Fri, 20 Oct 2023 17:37:59 +0300, Vladimir Oltean wrote: > > > Sorry, I didn't realize on v3 that you didn't completely apply my > > feedback on v2. Can "microchip,rmii-clk-internal" be a port device tree > > property? You have indeed moved its parsing to port code, but it is > > still located directly under the switch node in the device tree. > > > > I'm thinking that if this property was also applicable to other switches > > with multiple RMII ports, the setting would be per port rather than global. > > As far as I am aware only the KSZ8863 and KSZ8873 have this property available, > but the biggger issue might be in scaling this to port property as the register > "Forward Invalid VID Frame and Host Mode" where the setting is applied is > located under "Advanced Control Registers" section which is actually global at > least looking from the switch point of view. Usually port properties are more > applicable when registers in question are located under "Port Registers" section. > This is somewhat similar to for example enabling the tail tag mode which is > again used only by the port 3 interface and is control from "Global Control 1" > register. > With this in mind - if you still believe we should move this to port dt > property, then should we forbid setting the property for any other port other > than port 3, and can/should this be enforced by the dt schema? > If I see it correctly, KSZ9897R supports RMII on two ports (6 and 7) with configurable clock direction. See page 124 "5.2.3.2 XMII Port Control 1 Register" http://ww1.microchip.com/downloads/en/DeviceDoc/00002330B.pdf Regards, Oleksij
On Mon, 23 Oct 2023 09:58:48 +0200, Oleksij Rempel wrote: > If I see it correctly, KSZ9897R supports RMII on two ports (6 and 7) > with configurable clock direction. See page 124 "5.2.3.2 XMII Port Control 1 > Register" > http://ww1.microchip.com/downloads/en/DeviceDoc/00002330B.pdf Clock direction is possible I guess with other devices as well, but I don't see this specific property (routing REFCLKO to REFCLKI internally when switch is used as clock provider) for any other, including KSZ9897? I am no expert on micrel switches, but this to me looks like something specific only to KSZ88X3 devices as the clocking seems a bit different on KSZ9897 and alike. KSZ88X3 may generate clock to REFCLKO but still needs this clock fed back to REFCLKI (or will be routed internally with the "microchip-rmii-internal" property). This is managed differently on KSZ9897?
On Mon, Oct 23, 2023 at 10:22:30AM +0200, Ante Knezic wrote: > On Mon, 23 Oct 2023 09:58:48 +0200, Oleksij Rempel wrote: > > > If I see it correctly, KSZ9897R supports RMII on two ports (6 and 7) > > with configurable clock direction. See page 124 "5.2.3.2 XMII Port Control 1 > > Register" > > http://ww1.microchip.com/downloads/en/DeviceDoc/00002330B.pdf > > Clock direction is possible I guess with other devices as well, but I don't see > this specific property (routing REFCLKO to REFCLKI internally when switch is > used as clock provider) for any other, including KSZ9897? > > I am no expert on micrel switches, but this to me looks like something specific > only to KSZ88X3 devices as the clocking seems a bit different on KSZ9897 and > alike. KSZ88X3 may generate clock to REFCLKO but still needs this clock fed > back to REFCLKI (or will be routed internally with the "microchip-rmii-internal" > property). This is managed differently on KSZ9897? Here is KSZ8873 as initial reference: https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/00002348A.pdf 3.3.9 RMII INTERFACE OPERATION: "When EN_REFCLKO_3 is high, KSZ8873RLL will output a 50 MHz in REFCLKO_3. Register 198 bit[3] is used to select internal or external reference clock. Internal reference clock means that the clock for the RMII of KSZ8873RLL will be provided by the KSZ8873RLL internally and the REFCLKI_3 pin is unconnected. For the external reference clock, the clock will provide to KSZ8873RLL via REFCLKI_3." KSZ9897: http://ww1.microchip.com/downloads/en/DeviceDoc/00002330B.pdf 4.11.2 REDUCED MEDIA INDEPENDENT INTERFACE (RMII) "The user selects one of the two RMII clocking modes by setting the appropriate strapping option. The clocking mode is selected separately for ports 6 and 7. While in RMII Normal Mode, the port will require an external 50MHz signal to be input to TX_CLKx/REFCLKIx from an external source. This mode is selected by strapping the appropriate pin (RXD6_1 for port 6; RXD7_1 for port 7) high during reset. While in RMII Clock Mode, the port will output a 50MHz clock on RX_CLKx/REFCLKOx, which is derived from the 25MHz crystal or oscillator attached to the XI clock input. The TX_CLKx/REFCLKIx input is unused in this mode. This mode is selected by strapping the appropriate pin (RXD6_1 for port 6; RXD7_1 for port 7) low during reset. " Information about corresponding bits I linked in previous email. I do not see much differences. Regards, Oleksij
On Mon, 23 Oct 2023 10:41:50 +0200, Oleksij Rempel wrote: > Here is KSZ8873 as initial reference: > https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/00002348A.pdf > 3.3.9 RMII INTERFACE OPERATION: > "When EN_REFCLKO_3 is high, KSZ8873RLL will output a 50 MHz in REFCLKO_3. > Register 198 bit[3] is used to select internal or external reference > clock. Internal reference clock means that the clock for the RMII of > KSZ8873RLL will be provided by the KSZ8873RLL internally and the > REFCLKI_3 pin is unconnected. For the external reference clock, the > clock will provide to KSZ8873RLL via REFCLKI_3." > > KSZ9897: > http://ww1.microchip.com/downloads/en/DeviceDoc/00002330B.pdf > 4.11.2 REDUCED MEDIA INDEPENDENT INTERFACE (RMII) The upper paragraph refers to the case when switch is acting as a clock provider (regardless whether its set as internal or external reference clock). You can see this if you look at the next paragraph: "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 ..." So rmii-clk-internal property does not select whether switch is acting as a clock provider or clock consumer which is what you are refering to I believe? The clock provider/consumer is set via strapping pins. Real case scenario: I have a board where switch is acting as a clock provider, generating output to REFCLKO pin and feeding it to uC. This board does not have externally routed copper track from REFCLKO to REFCLKI, thus making the RMII interface not operable, unless the rmii-clk-internal bit is set. If this bit is not set, only way to make it running is to solder a jumper wire from REFCLKO to REFCLKI.
On Mon, Oct 23, 2023 at 10:57:50AM +0200, Ante Knezic wrote: > On Mon, 23 Oct 2023 10:41:50 +0200, Oleksij Rempel wrote: > > > Here is KSZ8873 as initial reference: > > https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/00002348A.pdf > > 3.3.9 RMII INTERFACE OPERATION: > > "When EN_REFCLKO_3 is high, KSZ8873RLL will output a 50 MHz in REFCLKO_3. > > Register 198 bit[3] is used to select internal or external reference > > clock. Internal reference clock means that the clock for the RMII of > > KSZ8873RLL will be provided by the KSZ8873RLL internally and the > > REFCLKI_3 pin is unconnected. For the external reference clock, the > > clock will provide to KSZ8873RLL via REFCLKI_3." > > > > KSZ9897: > > http://ww1.microchip.com/downloads/en/DeviceDoc/00002330B.pdf > > 4.11.2 REDUCED MEDIA INDEPENDENT INTERFACE (RMII) > > The upper paragraph refers to the case when switch is acting as a clock > provider (regardless whether its set as internal or external reference > clock). You can see this if you look at the next paragraph: > "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 ..." > So rmii-clk-internal property does not select whether switch is acting > as a clock provider or clock consumer which is what you are refering to > I believe? The clock provider/consumer is set via strapping pins. > > Real case scenario: I have a board where switch is acting as a clock > provider, generating output to REFCLKO pin and feeding it to uC. > This board does not have externally routed copper track from REFCLKO > to REFCLKI, thus making the RMII interface not operable, unless the > rmii-clk-internal bit is set. > If this bit is not set, only way to make it running is to solder a > jumper wire from REFCLKO to REFCLKI. In case of KSZ8873 we seems to have something like: Switch MAC<-. | PLL -> clk sel -> REFCLKO \-----< REFCLKI Clock select in this case is controlled by Register 198 (0xC6). In case of KSZ9897 we probably have something like: Switch MAC<-. | PLL -> clk sel -> REFCLKO \--x--< REFCLKI | Gate REFCLKI if REFCLKO is used. In both cases: - KSZ8873, Setting bit3 in Register 198 (0xC6) will control use of clk select - KSZ9897, setting bit2 in Register 0xN301, will controll use of clk select and probably gate REFCLKI. So far, it looks very similar to me and it is usually handled by phy-mode rmii vs revrmii. Correct? So, the main question is still, do we need this kind of configuration per port or it is enough to have it per switch? For some reasons KSZ8863MLL datasheet provides RMII clock select configuration for two ports (port 1 and 3) https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/DataSheets/KSZ8863MLL-FLL-RLL-Data-Sheet-00002335C.pdf May be there are variants with two RMIIs? Something similar but with multiple RMII interfaces seems to be supported by KSZ8864CNX: https://eu.mouser.com/datasheet/2/268/00002229A-1082534.pdf And all KSZ9xxx series seems to handle it per port as well.
On Mon, Oct 23, 2023 at 09:27:00AM +0200, Ante Knezic wrote: > As far as I am aware only the KSZ8863 and KSZ8873 have this property available, > but the biggger issue might be in scaling this to port property as the register > "Forward Invalid VID Frame and Host Mode" where the setting is applied is > located under "Advanced Control Registers" section which is actually global at > least looking from the switch point of view. Usually port properties are more > applicable when registers in question are located under "Port Registers" section. > This is somewhat similar to for example enabling the tail tag mode which is > again used only by the port 3 interface and is control from "Global Control 1" > register. > With this in mind - if you still believe we should move this to port dt > property, then should we forbid setting the property for any other port other > than port 3, and can/should this be enforced by the dt schema? I have no doubt that RMII settings are port settings. Scaling up the implementation to multiple ports on other switches doesn't mean that the DT binding shouldn't be per port. Anyway, the per-port access to a global switch setting is indeed a common theme with the old Micrel switches. I once tried to introduce the concept of "wacky" regmap regfields for that: https://patchwork.kernel.org/project/netdevbpf/patch/20230316161250.3286055-3-vladimir.oltean@nxp.com/ but I don't have hardware to test and nobody who does picked up upon the regfield idea, it seems.
On Mon, 23 Oct 2023 13:49:16 +0200, Oleksij Rempel wrote: > In case of KSZ8873 we seems to have something like: > > Switch MAC<-. > | > PLL -> clk sel -> REFCLKO > \-----< REFCLKI > > Clock select in this case is controlled by Register 198 (0xC6). > > In case of KSZ9897 we probably have something like: > > Switch MAC<-. > | > PLL -> clk sel -> REFCLKO > \--x--< REFCLKI > | > Gate REFCLKI if REFCLKO is used. > > In both cases: > - KSZ8873, Setting bit3 in Register 198 (0xC6) will control use of clk > select > - KSZ9897, setting bit2 in Register 0xN301, will controll use of clk > select and probably gate REFCLKI. > > So far, it looks very similar to me and it is usually handled by > phy-mode rmii vs revrmii. Correct? Thats correct I guess with one important point: default setting for KSZ88X3 is not to gate back to REFCLKI, while KSZ9897 will (correct me if I am wrong) automatically gate to REFCLKI and does not have the ability to gate/not-to-gate REFCLKO to REFCLKI with any register setting. Thats more-less what this patch is all about. Something that is automatically active (and can not be changed during run-time?) on KSZ9897 needs to be manually configured on KSZ88X3. > So, the main question is still, do we need this kind of configuration > per port or it is enough to have it per switch? Thats something on which more experienced developers/maintainers may have more to say, my impression is that its somewhat specific to KSZ88X3 to have this option available and the location of the register itself also makes a point on its own. I may be wrong about this of course. > For some reasons KSZ8863MLL datasheet provides RMII clock select > configuration for two ports (port 1 and 3) > https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/DataSheets/KSZ8863MLL-FLL-RLL-Data-Sheet-00002335C.pdf > May be there are variants with two RMIIs? As you noticed KSZ8863 provides configuration for port1 as well, but I was not able to find any reference to what it may actually select. The product identification system for ksz8863 does not mention devices with 2 RMII interfaces. The KSZ8873 has this bit set to "Reserved" so I can't really tell whats going on there... > Something similar but with multiple RMII interfaces seems to be > supported by KSZ8864CNX: > https://eu.mouser.com/datasheet/2/268/00002229A-1082534.pdf > As well as these datasheets are sometimes muddled and difficult to get a grasp of by looking at the KSZ8873 datasheet it seems to me that it can select between "clock" (default) and "normal" mode, which is something similar to the rmii-clk-internal property of KSZ88X3 devices, however this mode of operation seems to be selectable only at boot time by strap pins (see description of Register 12 (Global Control 10). > And all KSZ9xxx series seems to handle it per port as well. I guess you are refering to register 87 (RMII Management Control Register)? But this only selects whether to enable clock output on RXC pin?
On Mon, 23 Oct 2023 15:19:24 +0300, Vladimir Oltean wrote: > I have no doubt that RMII settings are port settings. Scaling up the implementation > to multiple ports on other switches doesn't mean that the DT binding shouldn't be > per port. > > Anyway, the per-port access to a global switch setting is indeed a common theme > with the old Micrel switches. I once tried to introduce the concept of "wacky" > regmap regfields for that: > https://patchwork.kernel.org/project/netdevbpf/patch/20230316161250.3286055-3-vladimir.oltean@nxp.com/ > > but I don't have hardware to test and nobody who does picked up upon the regfield > idea, it seems. Ok so I see about moving this to port property.
On Mon, Oct 23, 2023 at 02:41:30PM +0200, Ante Knezic wrote: > On Mon, 23 Oct 2023 13:49:16 +0200, Oleksij Rempel wrote: > > > In case of KSZ8873 we seems to have something like: > > > > Switch MAC<-. > > | > > PLL -> clk sel -> REFCLKO > > \-----< REFCLKI > > > > Clock select in this case is controlled by Register 198 (0xC6). > > > > In case of KSZ9897 we probably have something like: > > > > Switch MAC<-. > > | > > PLL -> clk sel -> REFCLKO > > \--x--< REFCLKI > > | > > Gate REFCLKI if REFCLKO is used. > > > > In both cases: > > - KSZ8873, Setting bit3 in Register 198 (0xC6) will control use of clk > > select > > - KSZ9897, setting bit2 in Register 0xN301, will controll use of clk > > select and probably gate REFCLKI. > > > > So far, it looks very similar to me and it is usually handled by > > phy-mode rmii vs revrmii. Correct? > > Thats correct I guess with one important point: default setting for KSZ88X3 is > not to gate back to REFCLKI, while KSZ9897 will (correct me if I am wrong) > automatically gate to REFCLKI and does not have the ability to gate/not-to-gate > REFCLKO to REFCLKI with any register setting. Thats more-less what this patch > is all about. Something that is automatically active (and can not be changed > during run-time?) on KSZ9897 needs to be manually configured on KSZ88X3. If I see it correctly, in both cases there is only one bit to configure direction. The code need to support two interface modes: - PHY_INTERFACE_MODE_RMII (MAC mode) PLL is the clock provider. REFCLKO is used. - PHY_INTERFACE_MODE_REVRMII (PHY mode) PLL is not used, REFCLKI is the clock provider. Linkmodes for KSZ9xxx series are implemented in the ksz_set_xmii() function. I already did some work to configure CPU interface, where which can be at least partially reused for your work: https://lore.kernel.org/all/20230517121034.3801640-2-o.rempel@pengutronix.de/ (Looks I forgot to complete mainlining for this patch) If implanted as described, no new devicetree properties will be needed. > > So, the main question is still, do we need this kind of configuration > > per port or it is enough to have it per switch? > > Thats something on which more experienced developers/maintainers may have more > to say, my impression is that its somewhat specific to KSZ88X3 to have this > option available and the location of the register itself also makes a point on > its own. I may be wrong about this of course. > > > For some reasons KSZ8863MLL datasheet provides RMII clock select > > configuration for two ports (port 1 and 3) > > https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/DataSheets/KSZ8863MLL-FLL-RLL-Data-Sheet-00002335C.pdf > > May be there are variants with two RMIIs? > > As you noticed KSZ8863 provides configuration for port1 as well, but I was not > able to find any reference to what it may actually select. The product > identification system for ksz8863 does not mention devices with 2 RMII > interfaces. The KSZ8873 has this bit set to "Reserved" so I can't really > tell whats going on there... > > > Something similar but with multiple RMII interfaces seems to be > > supported by KSZ8864CNX: > > https://eu.mouser.com/datasheet/2/268/00002229A-1082534.pdf > > > > As well as these datasheets are sometimes muddled and difficult to get a grasp of > by looking at the KSZ8873 datasheet it seems to me that it can select between > "clock" (default) and "normal" mode, which is something similar to the > rmii-clk-internal property of KSZ88X3 devices, however this mode of operation > seems to be selectable only at boot time by strap pins (see description of > Register 12 (Global Control 10). Ack. No need to worry about this one for now, it was only as example of a ksz8 style device with multiple RMII ports. > > And all KSZ9xxx series seems to handle it per port as well. > > I guess you are refering to register 87 (RMII Management Control Register)? > But this only selects whether to enable clock output on RXC pin? RXC seems to be not correct description or may be just internal signal within the IP core. This register points to the strap pin affecting value of this bit. Reading strap description in KSZ9477S or KSZ9897S give better idea about meaning of this bit: 0: MII: PHY Mode (Default) RMII: Clock Mode. RMII 50MHz reference clock is output on REFCLKO6. (Default) RGMII: No effect 1: MII: MAC Mode RMII: Normal Mode. RMII 50MHz reference clock is input on REFCLKI6. RGMII: No effect Regards, Oleksij
On Mon, 23 Oct 2023 16:36:35 +0200, Oleksij Rempel wrote: > If I see it correctly, in both cases there is only one bit to configure > direction. The code need to support two interface modes: > - PHY_INTERFACE_MODE_RMII (MAC mode) PLL is the clock provider. REFCLKO > is used. > - PHY_INTERFACE_MODE_REVRMII (PHY mode) PLL is not used, REFCLKI is the clock provider. As you suggested, it looks like KSZ9897 clocking mode depends on RMII interface mode (with strapping pins), but I don't see this for KSZ8863. The PHY/MAC mode is selected with Register 0x35 bit 7 and the clocking mode is selected via strapping pins EN_REFCLKO and SMTXD32 (and additional register 0xC6 bit 3). I guess its possible for the KSZ8863 to be the clock provider/consumer regardless of PHY/MAC mode? Table 3-5: RMII CLOCK SETTING of KSZ8863 datasheet describes the available clocking modes. If we try to create a relation between KSZ9897 and KSZ8863: KSZ9897 "Normal Mode" is equivalent to KSZ8863 mode described in first column of table 3-5: - EN_REFCLKO = 0, 0xC6(3) = 0 -> external 50Mhz OSC input to REFCLKI and X1 pin directly KSZ9897 "Clock Mode" is equivalent to KSZ8863 mode described in fourth/fifth column (difference is only clock frequency) of table 3-5: - EN_REFCLKO = 1, 0xC6(3) = 1 -> 50/25Mhz on X1 pin, 50/25Mhz RMII clock goes to REFCLKI internally. REFCLKI can be pulled down by resistor. That leaves us with additional columns 2 and 3 of table 3-5 for KSZ8863, that are similar to KSZ9897 Clock mode, but REFCLKI needs to be fed externally from REFCLKO. > I already did some work to configure CPU interface, where which can be at least > partially reused for your work: > https://lore.kernel.org/all/20230517121034.3801640-2-o.rempel@pengutronix.de/ > (Looks I forgot to complete mainlining for this patch) > > If implanted as described, no new devicetree properties will be needed. I don't quite get how the proposed patch might effect this topic? By setting PHY/MAC mode? As noted, I dont see the same relation between clock and MII mode for KSZ8863 as for KSZ9897?
On Tue, Oct 24, 2023 at 09:56:43AM +0200, Ante Knezic wrote: > On Mon, 23 Oct 2023 16:36:35 +0200, Oleksij Rempel wrote: > > If I see it correctly, in both cases there is only one bit to configure > > direction. The code need to support two interface modes: > > - PHY_INTERFACE_MODE_RMII (MAC mode) PLL is the clock provider. REFCLKO > > is used. > > - PHY_INTERFACE_MODE_REVRMII (PHY mode) PLL is not used, REFCLKI is the > clock provider. > > As you suggested, it looks like KSZ9897 clocking mode depends on RMII interface > mode (with strapping pins), but I don't see this for KSZ8863. The PHY/MAC mode > is selected with Register 0x35 bit 7 and the clocking mode is selected via > strapping pins EN_REFCLKO and SMTXD32 (and additional register 0xC6 bit 3). > I guess its possible for the KSZ8863 to be the clock provider/consumer > regardless of PHY/MAC mode? Register 0x35 bit 7 is for MII mode Register 0xC6 bit 3 is for RMII mode MII != RMII > Table 3-5: RMII CLOCK SETTING of KSZ8863 datasheet describes the available > clocking modes. If we try to create a relation between KSZ9897 and KSZ8863: > > KSZ9897 "Normal Mode" is equivalent to KSZ8863 mode described in first column > of table 3-5: > - EN_REFCLKO = 0, 0xC6(3) = 0 -> external 50Mhz OSC input to REFCLKI and X1 > pin directly > > KSZ9897 "Clock Mode" is equivalent to KSZ8863 mode described in fourth/fifth > column (difference is only clock frequency) of table 3-5: > - EN_REFCLKO = 1, 0xC6(3) = 1 -> 50/25Mhz on X1 pin, 50/25Mhz RMII clock goes > to REFCLKI internally. REFCLKI can be pulled down by resistor. > > That leaves us with additional columns 2 and 3 of table 3-5 for KSZ8863, that > are similar to KSZ9897 Clock mode, but REFCLKI needs to be fed externally from > REFCLKO. All of 5 variants described in "Table 3-5: RMII CLOCK SETTING of KSZ8863" can be boiled down to two main configurations: REFCLKI is used as clock source for internal MAC == Normal Mode or RevRMII mode. REFCLKI is not used as clock source for internal MAC == Clock Mode or RMII mode. Variants 1, 2, 3 describe only how can we feed REFCLKI from outside of the chip. Even variant 2 and 3 make the switch to be an actually physical clock provider, we still need to use REFCLKI and wire it outside of the chip which make it practically a Normal Mode or RevRMII mode. > > I already did some work to configure CPU interface, where which can be at least > > partially reused for your work: > > https://lore.kernel.org/all/20230517121034.3801640-2-o.rempel@pengutronix.de/ > > (Looks I forgot to complete mainlining for this patch) > > > > If implanted as described, no new devicetree properties will be needed. > > I don't quite get how the proposed patch might effect this topic? You will need to add ksz8_phylink_mac_link_up() as this patch already dose. > By setting PHY/MAC mode? As noted, I dont see the same relation between clock and > MII mode for KSZ8863 as for KSZ9897? I hope current mail will clear it. Regards, Oleksij
On Tue, 24 Oct 2023 12:09:15 +0200, Oleksij Rampel wrote: > > As you suggested, it looks like KSZ9897 clocking mode depends on RMII interface > > mode (with strapping pins), but I don't see this for KSZ8863. The PHY/MAC mode > > is selected with Register 0x35 bit 7 and the clocking mode is selected via > > strapping pins EN_REFCLKO and SMTXD32 (and additional register 0xC6 bit 3). > > I guess its possible for the KSZ8863 to be the clock provider/consumer > > regardless of PHY/MAC mode? > >Register 0x35 bit 7 is for MII mode >Register 0xC6 bit 3 is for RMII mode > >MII != RMII Yes, right you are. Looks like I got lost in the datasheets... > > Table 3-5: RMII CLOCK SETTING of KSZ8863 datasheet describes the available > > clocking modes. If we try to create a relation between KSZ9897 and KSZ8863: > > > > KSZ9897 "Normal Mode" is equivalent to KSZ8863 mode described in first column > > of table 3-5: > > - EN_REFCLKO = 0, 0xC6(3) = 0 -> external 50Mhz OSC input to REFCLKI and X1 > > pin directly > > > > KSZ9897 "Clock Mode" is equivalent to KSZ8863 mode described in fourth/fifth > > column (difference is only clock frequency) of table 3-5: > > - EN_REFCLKO = 1, 0xC6(3) = 1 -> 50/25Mhz on X1 pin, 50/25Mhz RMII clock goes > > to REFCLKI internally. REFCLKI can be pulled down by resistor. > > > > That leaves us with additional columns 2 and 3 of table 3-5 for KSZ8863, that > > are similar to KSZ9897 Clock mode, but REFCLKI needs to be fed externally from > > REFCLKO. > > All of 5 variants described in "Table 3-5: RMII CLOCK SETTING of KSZ8863" > can be boiled down to two main configurations: > > REFCLKI is used as clock source for internal MAC == Normal Mode or > RevRMII mode. > REFCLKI is not used as clock source for internal MAC == Clock Mode or > RMII mode. > > Variants 1, 2, 3 describe only how can we feed REFCLKI from outside of > the chip. Even variant 2 and 3 make the switch to be an actually > physical clock provider, we still need to use REFCLKI and wire it > outside of the chip which make it practically a Normal Mode or RevRMII mode. That is correct, I guess its a matter of nomenclature, but how do you "tell" the switch whether it has REFCLKI routed externally or not if not by setting the 0xC6 bit 3? Is there another way to achieve this? > > > I already did some work to configure CPU interface, where which can be at least > > > partially reused for your work: > > > https://lore.kernel.org/all/20230517121034.3801640-2-o.rempel@pengutronix.de/ > > > (Looks I forgot to complete mainlining for this patch) > > > > > > If implanted as described, no new devicetree properties will be needed. > > > > I don't quite get how the proposed patch might effect this topic? > > You will need to add ksz8_phylink_mac_link_up() as this patch already > dose. > > > By setting PHY/MAC mode? As noted, I dont see the same relation between clock and > > MII mode for KSZ8863 as for KSZ9897? > > I hope current mail will clear it. I tried your patch but it does not do it for me. As stated, my hw platform does not have REFCLKI routed externally so a state at column 4/5 is expected.
On Tue, Oct 24, 2023 at 03:08:32PM +0200, Ante Knezic wrote: > On Tue, 24 Oct 2023 12:09:15 +0200, Oleksij Rampel wrote: > > > > As you suggested, it looks like KSZ9897 clocking mode depends on RMII interface > > > mode (with strapping pins), but I don't see this for KSZ8863. The PHY/MAC mode > > > is selected with Register 0x35 bit 7 and the clocking mode is selected via > > > strapping pins EN_REFCLKO and SMTXD32 (and additional register 0xC6 bit 3). > > > I guess its possible for the KSZ8863 to be the clock provider/consumer > > > regardless of PHY/MAC mode? > > > >Register 0x35 bit 7 is for MII mode > >Register 0xC6 bit 3 is for RMII mode > > > >MII != RMII > > Yes, right you are. Looks like I got lost in the datasheets... > > > > Table 3-5: RMII CLOCK SETTING of KSZ8863 datasheet describes the available > > > clocking modes. If we try to create a relation between KSZ9897 and KSZ8863: > > > > > > KSZ9897 "Normal Mode" is equivalent to KSZ8863 mode described in first column > > > of table 3-5: > > > - EN_REFCLKO = 0, 0xC6(3) = 0 -> external 50Mhz OSC input to REFCLKI and X1 > > > pin directly > > > > > > KSZ9897 "Clock Mode" is equivalent to KSZ8863 mode described in fourth/fifth > > > column (difference is only clock frequency) of table 3-5: > > > - EN_REFCLKO = 1, 0xC6(3) = 1 -> 50/25Mhz on X1 pin, 50/25Mhz RMII clock goes > > > to REFCLKI internally. REFCLKI can be pulled down by resistor. > > > > > > That leaves us with additional columns 2 and 3 of table 3-5 for KSZ8863, that > > > are similar to KSZ9897 Clock mode, but REFCLKI needs to be fed externally from > > > REFCLKO. > > > > All of 5 variants described in "Table 3-5: RMII CLOCK SETTING of KSZ8863" > > can be boiled down to two main configurations: > > > > REFCLKI is used as clock source for internal MAC == Normal Mode or > > RevRMII mode. > > REFCLKI is not used as clock source for internal MAC == Clock Mode or > > RMII mode. > > > > Variants 1, 2, 3 describe only how can we feed REFCLKI from outside of > > the chip. Even variant 2 and 3 make the switch to be an actually > > physical clock provider, we still need to use REFCLKI and wire it > > outside of the chip which make it practically a Normal Mode or RevRMII mode. > > That is correct, I guess its a matter of nomenclature, but how do you > "tell" the switch whether it has REFCLKI routed externally or not if not by > setting the 0xC6 bit 3? Is there another way to achieve this? I do not see any other way to "tell" it. The only thing to change in you patches is a different way to tell it to the kernel. Instead of introducing a new devicetree property, you need to reuse phy-mode property. > > > > I already did some work to configure CPU interface, where which can be at least > > > > partially reused for your work: > > > > https://lore.kernel.org/all/20230517121034.3801640-2-o.rempel@pengutronix.de/ > > > > (Looks I forgot to complete mainlining for this patch) > > > > > > > > If implanted as described, no new devicetree properties will be needed. > > > > > > I don't quite get how the proposed patch might effect this topic? > > > > You will need to add ksz8_phylink_mac_link_up() as this patch already > > dose. > > > > > By setting PHY/MAC mode? As noted, I dont see the same relation between clock and > > > MII mode for KSZ8863 as for KSZ9897? > > > > I hope current mail will clear it. > > I tried your patch but it does not do it for me. As stated, my hw platform does > not have REFCLKI routed externally so a state at column 4/5 is expected. My patches do not address your problem. It is just example which already creates ksz8_phylink_mac_link_up() and do some CPU port configuration, which you will need do too, but for other registers. Or just for get it to avoid confusion. What you need is to set 0xC6 bit 3 for PHY_INTERFACE_MODE_REVRMII and clear it for PHY_INTERFACE_MODE_RMII. Since phy-mode for RMII was never set correctly, it will most probably break every single devicetree using KSZ switches. It is the price of fixing things :/ Regards, Oleksij
On Tue, 24 Oct 2023 16:24:26 +0200, Oleksij Rampel wrote: > > That is correct, I guess its a matter of nomenclature, but how do you > > "tell" the switch whether it has REFCLKI routed externally or not if not by > > setting the 0xC6 bit 3? Is there another way to achieve this? > > I do not see any other way to "tell" it. The only thing to change in you > patches is a different way to tell it to the kernel. > Instead of introducing a new devicetree property, you need to reuse > phy-mode property. > ... > Since phy-mode for RMII was never set correctly, it will most probably > break every single devicetree using KSZ switches. It is the price of fixing > things :/ To Vladimir Oltean: What are your thoughts on this?
On Fri, Oct 27, 2023 at 08:37:43AM +0200, Ante Knezic wrote: > On Tue, 24 Oct 2023 16:24:26 +0200, Oleksij Rampel wrote: > > > > That is correct, I guess its a matter of nomenclature, but how do you > > > "tell" the switch whether it has REFCLKI routed externally or not if not by > > > setting the 0xC6 bit 3? Is there another way to achieve this? > > > > I do not see any other way to "tell" it. The only thing to change in you > > patches is a different way to tell it to the kernel. > > Instead of introducing a new devicetree property, you need to reuse > > phy-mode property. > > > ... > > > Since phy-mode for RMII was never set correctly, it will most probably > > break every single devicetree using KSZ switches. It is the price of fixing > > things :/ > > To Vladimir Oltean: What are your thoughts on this? > Sorry for the delayed response. It's complicated. In the Google-searchable RMII spec, the REF_CLK is an input from the perspective of the PHY, and an input or output from the perspective of the MAC. Additionally, some RMII PHYs like NXP TJA1110 provide "extensions" to the RMII spec, where the PHY itself can provide the REF_CLK based on an internal 25 MHz crystal (see "nxp,rmii-refclk-in"). My understanding is that some other PHYs, like KSZ8061RNB, go even further and make the REF_CLK be always an output from the PHY's perspective (this is not configurable). It is noteworthy that on Wikipedia, it says directly that REF_CLK can also be driven from the PHY to the MAC, which itself represents an extension to what the spec says. Additionally, some MACs like the NXP SJA1105 switch ports are not as flexible as the standard would suggest. In RMII MAC mode, this switch always wants to drive the REF_CLK pin itself. And in RMII PHY (self-called REV-MII) mode, the SJA1105 always wants the REF_CLK to be driven externally (from the PHY or external oscillator). So, for example the SJA1105 in RMII MAC mode cannot be connected to the KSZ8061RNB, as both would attempt to drive the REF_CLK signal. In addition to all of that, the MAC/PHY roles are not just about the direction of the REF_CLK, but also about the /J/ /K/ codewords that are placed by the PHY in the inter packet gap on RXD[1:0]. A MAC doesn't do this, and if it did, the PHY wouldn't expect it, and AFAIK, would blindly propagate those code words onto the BASE-T wire, which is undesirable. So, my opinion is that although what Oleksij would like to see is admirable, I don't think that the REF_CLK direction is a matter of RMII MAC vs PHY role, and thus, we wouldn't need to change "rmii" to "rev-rmii" and cause breakage everywhere. It's just that - a matter of REF_CLK direction. It's true, though, that this is a generic problem and that the generic bindings for RMII that we currently have are under-specified. We could try to devise an extended RMII binding which makes it clear for both the MAC and the PHY who is responsible to drive this signal. You are not attempting that, you are just coming up with yet another vendor-specific MAC property which solves a generic problem. I can't say I am completely opposed to that, either, which is why I haven't really spoken out against it. The PHY maintainers would also have to weigh in, and not all of them are CCed here. Now as to Conor's previous question about describing the REF_CLK as a CCF clock, which is a bit related, honestly I haven't really analyzed the merits of doing that. We know from the RMII standard that has a known expected frequency of 50 MHz, it's just a matter of who provides it. Just one small thing to note is that I have heard of RMII PHYs which, when REF_CLK is an input from their perspective, don't even have their registers accessible until that REF_CLK is turned on. This may be problematic, because the attached MAC driver may also own the MDIO bus on which said PHY is located, and it practically needs to turn on the RMII REF_CLK before the PHY on the MDIO bus could even be probed. Currently, IIUC, the way in which that works is that RMII MAC drivers are simply coded up to do just that, and I guess that worked so far. With CCF, I guess you would describe this by making the MAC be a "clocks" provider for the PHY. But it could also be the other way around, and in that case, the MAC would be the parent of the PHY, but it would also depend on a component that the PHY provides. I hope fw_devlink doesn't mind reverse dependencies like that... I am afraid that creating a CCF style binding for REF_CLK will be an enormous hammer for a very small nail and will see very limited adoption to other drivers, but I might as well be wrong about it. Compatibility between RMII MACs and PHYs which may or may not be CCF-ready might also be a concern.
> So, my opinion is that although what Oleksij would like to see is > admirable, I don't think that the REF_CLK direction is a matter of RMII > MAC vs PHY role, and thus, we wouldn't need to change "rmii" to "rev-rmii" > and cause breakage everywhere. It's just that - a matter of REF_CLK > direction. It's true, though, that this is a generic problem and that > the generic bindings for RMII that we currently have are under-specified. > We could try to devise an extended RMII binding which makes it clear for > both the MAC and the PHY who is responsible to drive this signal. You > are not attempting that, you are just coming up with yet another > vendor-specific MAC property which solves a generic problem. I can't say > I am completely opposed to that, either, which is why I haven't really > spoken out against it. The PHY maintainers would also have to weigh in, > and not all of them are CCed here. I would recommend looking around other PHYs and find a property which does what you want, and copy it. We do have all sorts of properties. There are some to enable the REF_CLK out of the PHY. Some to disable the REF_CLK out, some to disable it when the link is down, some to indicate what frequency it should tick at, etc. If you want to go the extra mile, maybe you can make a summary of all these properties, and maybe we can produce a guide line for what we want the properties to be called going forward. > I am afraid that creating a CCF style binding for REF_CLK will be an > enormous hammer for a very small nail and will see very limited adoption > to other drivers, but I might as well be wrong about it. Compatibility > between RMII MACs and PHYs which may or may not be CCF-ready might also > be a concern. I also don't think using the CCF makes too much sense, except for where the SoC provides the lock, and already has a CCF covering it. I would also be hesitant to add more dependencies between the MAC and the PHY. The DT often has circular dependencies and we have had issues with probing being deferred because the core does not always understand these circular dependencies. Andrew
On Mon, Oct 30, 2023 at 07:42:25PM +0200, Vladimir Oltean wrote: > On Fri, Oct 27, 2023 at 08:37:43AM +0200, Ante Knezic wrote: > > On Tue, 24 Oct 2023 16:24:26 +0200, Oleksij Rampel wrote: > > > > > > That is correct, I guess its a matter of nomenclature, but how do you > > > > "tell" the switch whether it has REFCLKI routed externally or not if not by > > > > setting the 0xC6 bit 3? Is there another way to achieve this? > > > > > > I do not see any other way to "tell" it. The only thing to change in you > > > patches is a different way to tell it to the kernel. > > > Instead of introducing a new devicetree property, you need to reuse > > > phy-mode property. > > > > > ... > > > > > Since phy-mode for RMII was never set correctly, it will most probably > > > break every single devicetree using KSZ switches. It is the price of fixing > > > things :/ > > > > To Vladimir Oltean: What are your thoughts on this? > > > > In addition to all of that, the MAC/PHY roles are not just about the > direction of the REF_CLK, but also about the /J/ /K/ codewords that are > placed by the PHY in the inter packet gap on RXD[1:0]. A MAC doesn't do > this, and if it did, the PHY wouldn't expect it, and AFAIK, would > blindly propagate those code words onto the BASE-T wire, which is > undesirable. Interesting detail. I didn't knew it, it would be good to document it somewhere near to revrmii binding :) Regards, Oleksij
On Tue, Oct 31, 2023 at 02:00:05AM +0100, Andrew Lunn wrote: > > So, my opinion is that although what Oleksij would like to see is > > admirable, I don't think that the REF_CLK direction is a matter of RMII > > MAC vs PHY role, and thus, we wouldn't need to change "rmii" to "rev-rmii" > > and cause breakage everywhere. It's just that - a matter of REF_CLK > > direction. It's true, though, that this is a generic problem and that > > the generic bindings for RMII that we currently have are under-specified. > > We could try to devise an extended RMII binding which makes it clear for > > both the MAC and the PHY who is responsible to drive this signal. You > > are not attempting that, you are just coming up with yet another > > vendor-specific MAC property which solves a generic problem. I can't say > > I am completely opposed to that, either, which is why I haven't really > > spoken out against it. The PHY maintainers would also have to weigh in, > > and not all of them are CCed here. > > I would recommend looking around other PHYs and find a property which > does what you want, and copy it. > > We do have all sorts of properties. There are some to enable the > REF_CLK out of the PHY. Some to disable the REF_CLK out, some to > disable it when the link is down, some to indicate what frequency it > should tick at, etc. > > If you want to go the extra mile, maybe you can make a summary of all > these properties, and maybe we can produce a guide line for what we > want the properties to be called going forward. > > > I am afraid that creating a CCF style binding for REF_CLK will be an > > enormous hammer for a very small nail and will see very limited adoption > > to other drivers, but I might as well be wrong about it. Compatibility > > between RMII MACs and PHYs which may or may not be CCF-ready might also > > be a concern. > > I also don't think using the CCF makes too much sense, except for > where the SoC provides the lock, and already has a CCF covering it. > > I would also be hesitant to add more dependencies between the MAC and > the PHY. The DT often has circular dependencies and we have had issues > with probing being deferred because the core does not always > understand these circular dependencies. Heh, this are unsolved problems making me pain in different projects. Here are some real life examples, which are unsolved in one or another project and till now didn't went mainline: 1. In scenarios where PHYs require an RMII clock from the MAC, initialization becomes complex. This is often resolved through bootloader and kernel modifications. Right now it kind of works and postponed until it will make real pain :) 2. Complexity increases in designs with multiple PHYs used by different MACs but connected to one MDIO bus. Same is here, there was already some regressions but the pain is still not enough for making things right. 3. For some MACs like STMMAC, configuration is challenging without an external clock from the PHY. For example, VLAN configuration isn't possible with EEE enabled unless deep power saving states are disabled during register access. If I remember it correctly, there was floating discussions and patches trying to address similar issues. Transferring these issues to KSZ8863, we might face difficulties configuring STMMAC if KSZ8863, acting as the clock provider, isn't enabled early before MAC driver probing, a tricky scenario in the DSA framework. Working on deep sleep states for the KSZ switch driver, I find that dynamic clock control, potentially offered by CCF, could be quite handy. Please do not see this answer as a request to Ante for complex rework. It's more of a red flag notifying that the clocking issue is still unsolved, and someone (may be me), sooner or later, will have enough motivation to jump into this wasp nest :) Regards, Oleksij
On Tue, Oct 31, 2023 at 08:28:47AM +0100, Oleksij Rempel wrote: > Transferring these issues to KSZ8863, we might face difficulties configuring > STMMAC if KSZ8863, acting as the clock provider, isn't enabled early before MAC > driver probing, a tricky scenario in the DSA framework. So for each driver, there are 2 components to using CCF for MAC/PHY interface clocks, one is providing what you have, and the other is requesting what you need. To avoid circular dependencies, we should make the clock provider per DSA port be independent of the DSA driver itself. That is because, as you point out in your example, the conduit interface (stmmac) may depend on a clock provided by the switch, but the switch also depends on the conduit being fully probed. Stronger separation / more granular dependencies was one reason why I wanted for more DSA drivers to follow Colin Foster's suit, but I stopped working on that too: https://lore.kernel.org/lkml/20221222134844.lbzyx5hz7z5n763n@skbuf/ but in the case of interface clocks, the separation is not so clear. I would expect that for most if not all switches, the interface clock is implicitly provided by enabling and configuring the respective MAC, the same MAC that is intrinsically controlled through phylink by the main DSA (switching IP) driver. So we have 2 APIs for controlling the same piece of hardware, I'm not sure how conflicting requests are supposed to be resolved. This piece of the puzzle is quite complicated to fit into the larger architecture in a coherent way, although I'm not arguing that there will also be benefits.
diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c index 4bf4d67557dc..b0a305b7c37e 100644 --- a/drivers/net/dsa/microchip/ksz8795.c +++ b/drivers/net/dsa/microchip/ksz8795.c @@ -1358,6 +1358,9 @@ static void ksz8795_cpu_interface_select(struct ksz_device *dev, int port) { struct ksz_port *p = &dev->ports[port]; + if (!ksz_is_ksz87xx(dev)) + return; + if (!p->interface && dev->compat_interface) { dev_warn(dev->dev, "Using legacy switch \"phy-mode\" property, because it is missing on port %d node. " @@ -1391,18 +1394,28 @@ void ksz8_port_setup(struct ksz_device *dev, int port, bool cpu_port) /* enable 802.1p priority */ ksz_port_cfg(dev, port, P_PRIO_CTRL, PORT_802_1P_ENABLE, true); - if (cpu_port) { - if (!ksz_is_ksz88x3(dev)) - ksz8795_cpu_interface_select(dev, port); - + if (cpu_port) member = dsa_user_ports(ds); - } else { + else member = BIT(dsa_upstream_port(ds, port)); - } ksz8_cfg_port_member(dev, port, member); } +static void ksz88x3_config_rmii_clk(struct ksz_device *dev) +{ + bool rmii_clk_internal; + + if (!ksz_is_ksz88x3(dev)) + return; + + rmii_clk_internal = of_property_read_bool(dev->dev->of_node, + "microchip,rmii-clk-internal"); + + ksz_cfg(dev, KSZ88X3_REG_FVID_AND_HOST_MODE, + KSZ88X3_PORT3_RMII_CLK_INTERNAL, rmii_clk_internal); +} + void ksz8_config_cpu_port(struct dsa_switch *ds) { struct ksz_device *dev = ds->priv; @@ -1419,6 +1432,9 @@ void ksz8_config_cpu_port(struct dsa_switch *ds) ksz8_port_setup(dev, dev->cpu_port, true); + ksz8795_cpu_interface_select(dev, dev->cpu_port); + ksz88x3_config_rmii_clk(dev); + for (i = 0; i < dev->phy_port_cnt; i++) { ksz_port_stp_state_set(ds, i, BR_STATE_DISABLED); } diff --git a/drivers/net/dsa/microchip/ksz8795_reg.h b/drivers/net/dsa/microchip/ksz8795_reg.h index 3c9dae53e4d8..beca974e0171 100644 --- a/drivers/net/dsa/microchip/ksz8795_reg.h +++ b/drivers/net/dsa/microchip/ksz8795_reg.h @@ -22,6 +22,9 @@ #define KSZ8863_GLOBAL_SOFTWARE_RESET BIT(4) #define KSZ8863_PCS_RESET BIT(0) +#define KSZ88X3_REG_FVID_AND_HOST_MODE 0xC6 +#define KSZ88X3_PORT3_RMII_CLK_INTERNAL BIT(3) + #define REG_SW_CTRL_0 0x02 #define SW_NEW_BACKOFF BIT(7)
Microchip KSZ8863/KSZ8873 have the ability to select between internal and external RMII reference clock. By default, reference clock needs to be provided via REFCLKI_3 pin. If required, device can be setup to provide RMII clock internally so that REFCLKI_3 pin can be left unconnected. Add a new "microchip,rmii-clk-internal" property which will set RMII clock reference to internal. If property is not set, reference clock needs to be provided externally. While at it, move the ksz8795_cpu_interface_select() to ksz8_config_cpu_port() to get a cleaner call path for cpu port. Signed-off-by: Ante Knezic <ante.knezic@helmholz.de> --- drivers/net/dsa/microchip/ksz8795.c | 28 ++++++++++++++++++++++------ drivers/net/dsa/microchip/ksz8795_reg.h | 3 +++ 2 files changed, 25 insertions(+), 6 deletions(-)