On Wed, Oct 18, 2023 at 11:24:14AM +0200, Ante Knezic wrote:
> 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.
>
> Signed-off-by: Ante Knezic <ante.knezic@xxxxxxxxxxx>
> ---
> drivers/net/dsa/microchip/ksz8795.c | 10 +++++++++-
> drivers/net/dsa/microchip/ksz8795_reg.h | 3 +++
> drivers/net/dsa/microchip/ksz_common.h | 1 +
> 3 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
> index 91aba470fb2f..b50ad9552c65 100644
> --- a/drivers/net/dsa/microchip/ksz8795.c
> +++ b/drivers/net/dsa/microchip/ksz8795.c
> @@ -1312,8 +1312,16 @@ void ksz8_port_setup(struct ksz_device *dev, int port, bool cpu_port)
> ksz_port_cfg(dev, port, P_PRIO_CTRL, PORT_802_1P_ENABLE, true);
>
> if (cpu_port) {
> - if (!ksz_is_ksz88x3(dev))
> + if (!ksz_is_ksz88x3(dev)) {
> ksz8795_cpu_interface_select(dev, port);
> + } else {
> + dev->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,
> + dev->rmii_clk_internal);
Odd code placement, and it looks too crammed/shifted to the right due to indentation.
The calling pattern is as follows: ksz8_port_setup() has 2 callers.
One is from
ds->ops->port_setup()
-> ksz_port_setup()
-> filters out everything except user ports
-> dev->dev_ops->port_setup()
-> ksz8_port_setup()
and the other is from
ds->ops->setup() // switch-wide
-> dev->dev_ops->config_cpu_port()
-> ksz8_config_cpu_port()
-> ksz8_port_setup()
So user ports and CPU ports meet at ksz8_port_setup() from different
call paths, but I think it's strange to continue this coding pattern for
port stuff that's not common between user ports and CPU ports. For that
reason, the placement of the existing ksz8795_cpu_interface_select() is
also weird, when it could have been directly placed under
ksz8_config_cpu_port(), and it would have not confusingly shared a code
path with user ports.
Could you please add a dedicated ksz88x3_config_rmii_clk(), called
directly from ksz8_config_cpu_port()? It can have this as first step:
if (!ksz_is_ksz88x3(dev))
return 0;
and then the rest of the code can have a single level of indentation,
which would look much more natural.
> + }
>
> member = dsa_user_ports(ds);
> } else {
>
