| Message ID | 20230713202123.231445-2-alex@shruggie.ro |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | None | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success |
On Thu, Jul 13, 2023 at 11:21:23PM +0300, Alexandru Ardelean wrote: > For VSC8351 and similar PHYs, a new property was added to generate a clock > signal on the CLKOUT pin. Sorry, didn't think about it on v1, but I would imagine other vendors' PHYs have similar functionality. We should have something common. We have the clock binding for clocks already, so we should consider if that should be used here. It may look like an overkill for what you need, but things always start out that way. What if you want to turn the clock on and off as well? > This change documents the change in the device-tree bindings doc. That's obvious. > > Signed-off-by: Alexandru Ardelean <alex@shruggie.ro> > --- > > Changelog v1 -> v2: > * https://lore.kernel.org/netdev/20230706081554.1616839-2-alex@shruggie.ro/ > * changed property name 'vsc8531,clkout-freq-mhz' -> 'mscc,clkout-freq-mhz' > as requested by Rob > * added 'net-next' tag as requested by Andrew > > Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt > index 0a3647fe331b..085d0e8a834e 100644 > --- a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt > +++ b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt > @@ -31,6 +31,10 @@ Optional properties: > VSC8531_LINK_100_ACTIVITY (2), > VSC8531_LINK_ACTIVITY (0) and > VSC8531_DUPLEX_COLLISION (8). > +- mscc,clkout-freq-mhz : For VSC8531 and similar PHYs, this will output > + a clock signal on the CLKOUT pin of the chip. > + The supported values are 25, 50 & 125 Mhz. > + Default value is no clock signal on the CLKOUT pin. > - load-save-gpios : GPIO used for the load/save operation of the PTP > hardware clock (PHC). > > @@ -69,5 +73,6 @@ Example: > vsc8531,edge-slowdown = <7>; > vsc8531,led-0-mode = <VSC8531_LINK_1000_ACTIVITY>; > vsc8531,led-1-mode = <VSC8531_LINK_100_ACTIVITY>; > + mscc,clkout-freq-mhz = <50>; > load-save-gpios = <&gpio 10 GPIO_ACTIVE_HIGH>; > }; > -- > 2.41.0 >
On Fri, Jul 14, 2023 at 8:24 PM Rob Herring <robh@kernel.org> wrote: > > On Thu, Jul 13, 2023 at 11:21:23PM +0300, Alexandru Ardelean wrote: > > For VSC8351 and similar PHYs, a new property was added to generate a clock > > signal on the CLKOUT pin. > > Sorry, didn't think about it on v1, but I would imagine other vendors' > PHYs have similar functionality. We should have something common. We > have the clock binding for clocks already, so we should consider if > that should be used here. It may look like an overkill for what you > need, but things always start out that way. What if you want to turn the > clock on and off as well? So, there's the adin.c PHY driver which has a similar functionality with the adin_config_clk_out(). Something in the micrel.c PHY driver (with micrel,rmii-reference-clock-select-25-mhz); hopefully I did not misread the code about that one. And the at803x.c PHY driver has a 'qca,clk-out-frequency' property too. Now with the mscc.c driver, there is a common-ality that could use a framework. @Rob are you suggesting something like registering a clock provider (somewhere in the PHY framework) and let the PHY drivers use it? Usually, these clock signals (once enabled on startup), don't get turned off; but I've worked mostly on reference designs; somewhere down the line some people get different requirements. These clocks get connected back to the MAC (usually), and are usually like a "fixed-clock" driver. In our case, turning off the clock would be needed if the PHY negotiates a non-gigabit link; i.e 100 or 10 Mbps; in that case, the CLKOUT signal is not needed and it can be turned off. Maybe start out with a hook in 'struct phy_driver'? Like "int (*config_clk_out)(struct phy_device *dev);" or something? And underneath, this delegates to the CLK framework? I'd let Andrew (or someone in netdev) have a final feedback here. I can (probably) try to allocate some time to do this change based on the MSCC driver in the next weeks, if there's a consensus. Thanks Alex > > > This change documents the change in the device-tree bindings doc. > > That's obvious. > > > > > Signed-off-by: Alexandru Ardelean <alex@shruggie.ro> > > --- > > > > Changelog v1 -> v2: > > * https://lore.kernel.org/netdev/20230706081554.1616839-2-alex@shruggie.ro/ > > * changed property name 'vsc8531,clkout-freq-mhz' -> 'mscc,clkout-freq-mhz' > > as requested by Rob > > * added 'net-next' tag as requested by Andrew > > > > Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt > > index 0a3647fe331b..085d0e8a834e 100644 > > --- a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt > > +++ b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt > > @@ -31,6 +31,10 @@ Optional properties: > > VSC8531_LINK_100_ACTIVITY (2), > > VSC8531_LINK_ACTIVITY (0) and > > VSC8531_DUPLEX_COLLISION (8). > > +- mscc,clkout-freq-mhz : For VSC8531 and similar PHYs, this will output > > + a clock signal on the CLKOUT pin of the chip. > > + The supported values are 25, 50 & 125 Mhz. > > + Default value is no clock signal on the CLKOUT pin. > > - load-save-gpios : GPIO used for the load/save operation of the PTP > > hardware clock (PHC). > > > > @@ -69,5 +73,6 @@ Example: > > vsc8531,edge-slowdown = <7>; > > vsc8531,led-0-mode = <VSC8531_LINK_1000_ACTIVITY>; > > vsc8531,led-1-mode = <VSC8531_LINK_100_ACTIVITY>; > > + mscc,clkout-freq-mhz = <50>; > > load-save-gpios = <&gpio 10 GPIO_ACTIVE_HIGH>; > > }; > > -- > > 2.41.0 > >
> So, there's the adin.c PHY driver which has a similar functionality > with the adin_config_clk_out(). > Something in the micrel.c PHY driver (with > micrel,rmii-reference-clock-select-25-mhz); hopefully I did not > misread the code about that one. > And the at803x.c PHY driver has a 'qca,clk-out-frequency' property too. > > Now with the mscc.c driver, there is a common-ality that could use a framework. > > @Rob are you suggesting something like registering a clock provider > (somewhere in the PHY framework) and let the PHY drivers use it? > Usually, these clock signals (once enabled on startup), don't get > turned off; but I've worked mostly on reference designs; somewhere > down the line some people get different requirements. > These clocks get connected back to the MAC (usually), and are usually > like a "fixed-clock" driver. They are not necessarily fixed clocks. The clock you are adding here has three frequencies. Two frequencies is common for PHY devices. So you need to use something more than clk-fixed-rate.c. Also, mostly PHYs allows the clock to be gated. > In our case, turning off the clock would be needed if the PHY > negotiates a non-gigabit link; i.e 100 or 10 Mbps; in that case, the > CLKOUT signal is not needed and it can be turned off. Who does not need it? The PHY, or the MAC? If it is the MAC, it should really be the MAC driver which uses the common clock API to turn it off. Just watch out for deadlocks with phydev->lock. > Maybe start out with a hook in 'struct phy_driver'? > Like "int (*config_clk_out)(struct phy_device *dev);" or something? > And underneath, this delegates to the CLK framework? Yes, have phy_device.c implement that registration/unregister of the clock, deal with locking, and call into the PHY driver to actually manipulate the clock. You missed the requested frequency in the function prototype. I would also call it refclk. Three is sometimes confusion about the different clocks. Traditionally, clk_enable() can be called in atomic context, but that is not allowed with phylib, it always assume thread context. I don't know if the clock framework has some helpers for that, but i also don't see there being a real need for MAC to enable the clock in atomic context. Andrew
On Sun, Jul 16, 2023 at 6:07 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > So, there's the adin.c PHY driver which has a similar functionality > > with the adin_config_clk_out(). > > Something in the micrel.c PHY driver (with > > micrel,rmii-reference-clock-select-25-mhz); hopefully I did not > > misread the code about that one. > > And the at803x.c PHY driver has a 'qca,clk-out-frequency' property too. > > > > Now with the mscc.c driver, there is a common-ality that could use a framework. > > > > @Rob are you suggesting something like registering a clock provider > > (somewhere in the PHY framework) and let the PHY drivers use it? > > Usually, these clock signals (once enabled on startup), don't get > > turned off; but I've worked mostly on reference designs; somewhere > > down the line some people get different requirements. > > These clocks get connected back to the MAC (usually), and are usually > > like a "fixed-clock" driver. > > They are not necessarily fixed clocks. The clock you are adding here > has three frequencies. Two frequencies is common for PHY devices. So > you need to use something more than clk-fixed-rate.c. Also, mostly > PHYs allows the clock to be gated. > > > In our case, turning off the clock would be needed if the PHY > > negotiates a non-gigabit link; i.e 100 or 10 Mbps; in that case, the > > CLKOUT signal is not needed and it can be turned off. > > Who does not need it? The PHY, or the MAC? If it is the MAC, it should > really be the MAC driver which uses the common clock API to turn it > off. Just watch out for deadlocks with phydev->lock. The MAC needs the clock in GMII mode, when going in gigabit mode. > > > Maybe start out with a hook in 'struct phy_driver'? > > Like "int (*config_clk_out)(struct phy_device *dev);" or something? > > And underneath, this delegates to the CLK framework? > > Yes, have phy_device.c implement that registration/unregister of the > clock, deal with locking, and call into the PHY driver to actually > manipulate the clock. You missed the requested frequency in the > function prototype. I would also call it refclk. Three is sometimes > confusion about the different clocks. Ack. Then something like: int (*config_refclk)(struct phy_device *dev, uint32_t frequency); > > Traditionally, clk_enable() can be called in atomic context, but that > is not allowed with phylib, it always assume thread context. I don't > know if the clock framework has some helpers for that, but i also > don't see there being a real need for MAC to enable the clock in > atomic context. > > Andrew
diff --git a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt index 0a3647fe331b..085d0e8a834e 100644 --- a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt +++ b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt @@ -31,6 +31,10 @@ Optional properties: VSC8531_LINK_100_ACTIVITY (2), VSC8531_LINK_ACTIVITY (0) and VSC8531_DUPLEX_COLLISION (8). +- mscc,clkout-freq-mhz : For VSC8531 and similar PHYs, this will output + a clock signal on the CLKOUT pin of the chip. + The supported values are 25, 50 & 125 Mhz. + Default value is no clock signal on the CLKOUT pin. - load-save-gpios : GPIO used for the load/save operation of the PTP hardware clock (PHC). @@ -69,5 +73,6 @@ Example: vsc8531,edge-slowdown = <7>; vsc8531,led-0-mode = <VSC8531_LINK_1000_ACTIVITY>; vsc8531,led-1-mode = <VSC8531_LINK_100_ACTIVITY>; + mscc,clkout-freq-mhz = <50>; load-save-gpios = <&gpio 10 GPIO_ACTIVE_HIGH>; };
For VSC8351 and similar PHYs, a new property was added to generate a clock signal on the CLKOUT pin. This change documents the change in the device-tree bindings doc. Signed-off-by: Alexandru Ardelean <alex@shruggie.ro> --- Changelog v1 -> v2: * https://lore.kernel.org/netdev/20230706081554.1616839-2-alex@shruggie.ro/ * changed property name 'vsc8531,clkout-freq-mhz' -> 'mscc,clkout-freq-mhz' as requested by Rob * added 'net-next' tag as requested by Andrew Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt | 5 +++++ 1 file changed, 5 insertions(+)