| Message ID | c8bb7002e6d81a661c853dd21e0fe18e95887609.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 |
Commit message: "net: dsa: microchip: ". On Thu, Oct 12, 2023 at 12:55:55PM +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@helmholz.de> > --- > drivers/net/dsa/microchip/ksz8795.c | 5 +++++ > drivers/net/dsa/microchip/ksz8795_reg.h | 3 +++ > drivers/net/dsa/microchip/ksz_common.c | 3 +++ > drivers/net/dsa/microchip/ksz_common.h | 1 + > 4 files changed, 12 insertions(+) > > diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c > index 91aba470fb2f..78f3a668aa99 100644 > --- a/drivers/net/dsa/microchip/ksz8795.c > +++ b/drivers/net/dsa/microchip/ksz8795.c > @@ -1434,6 +1434,11 @@ int ksz8_setup(struct dsa_switch *ds) > for (i = 0; i < (dev->info->num_vlans / 4); i++) > ksz8_r_vlan_entries(dev, i); > > + if (ksz_is_ksz88x3(dev)) > + ksz_cfg(dev, KSZ88X3_REG_FVID_AND_HOST_MODE, > + KSZ88X3_PORT3_RMII_CLK_INTERNAL, > + dev->rmii_clk_internal); > + Can this be done in dev->dev_ops->phylink_mac_config() (which so far has no implementation) for port 3 of ksz88x3? > return ksz8_handle_global_errata(ds); > } > > 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) > diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c > index b800ace40ce1..0a0a53ce5b1b 100644 > --- a/drivers/net/dsa/microchip/ksz_common.c > +++ b/drivers/net/dsa/microchip/ksz_common.c > @@ -4160,6 +4160,9 @@ int ksz_switch_register(struct ksz_device *dev) > } > } > > + dev->rmii_clk_internal = of_property_read_bool(dev->dev->of_node, > + "microchip,rmii-clk-internal"); Port property. > + > ret = dsa_register_switch(dev->ds); > if (ret) { > dev->dev_ops->exit(dev); > diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h > index 8842efca0871..e5b0445fe2ca 100644 > --- a/drivers/net/dsa/microchip/ksz_common.h > +++ b/drivers/net/dsa/microchip/ksz_common.h > @@ -163,6 +163,7 @@ struct ksz_device { > phy_interface_t compat_interface; > bool synclko_125; > bool synclko_disable; > + bool rmii_clk_internal; > > struct vlan_table *vlan_cache; > > -- > 2.11.0
On Mon, 16 Oct 2023 13:37:08 +0300, Vladimir Oltean wrote: > > diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c > > index 91aba470fb2f..78f3a668aa99 100644 > > --- a/drivers/net/dsa/microchip/ksz8795.c > > +++ b/drivers/net/dsa/microchip/ksz8795.c > > @@ -1434,6 +1434,11 @@ int ksz8_setup(struct dsa_switch *ds) > > for (i = 0; i < (dev->info->num_vlans / 4); i++) > > ksz8_r_vlan_entries(dev, i); > > > > + if (ksz_is_ksz88x3(dev)) > > + ksz_cfg(dev, KSZ88X3_REG_FVID_AND_HOST_MODE, > > + KSZ88X3_PORT3_RMII_CLK_INTERNAL, > > + dev->rmii_clk_internal); > > + > > Can this be done in dev->dev_ops->phylink_mac_config() (which so far has no implementation) > for port 3 of ksz88x3? > > return ksz8_handle_global_errata(ds); > > } > > > > 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) > > diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c > > index b800ace40ce1..0a0a53ce5b1b 100644 > > --- a/drivers/net/dsa/microchip/ksz_common.c > > +++ b/drivers/net/dsa/microchip/ksz_common.c > > @@ -4160,6 +4160,9 @@ int ksz_switch_register(struct ksz_device *dev) > > } > > } > > > > + dev->rmii_clk_internal = of_property_read_bool(dev->dev->of_node, > > + "microchip,rmii-clk-internal"); > > Port property. Yes, I guess we can do it in phylink_mac_config. Or perhaps it would be better to put everything in ksz8_port_setup if you suggest this is a port specific property and not global? Something like: @@ -1312,8 +1314,15 @@ 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); + }
> > + 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"? If I understood you correctly you are refering to a solution like if: properties: compatible: enum: - microchip,ksz8863 - microchip,ksz8873 then: properties: 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. This was already suggested in v1, but was not a satisfactory solution according to Mr. Conor Dooley: >> On Tue, 10 Oct 2023 16:25:55 +0100, Conor Dooley wrote: >> > On Tue, Oct 10, 2023 at 03:18:54PM +0200, Ante Knezic wrote: >> > > Add documentation for selecting reference rmii clock on KSZ88X3 devices >> > > >> > > Signed-off-by: Ante Knezic <ante.knezic@helmholz.de> >> > > --- >> > > Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml | 6 ++++++ >> > > 1 file changed, 6 insertions(+) >> > > >> > > diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml >> > > index e51be1ac0362..3df5d2e72dba 100644 >> > > --- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml >> > > +++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml >> > > @@ -49,6 +49,12 @@ properties: >> > > Set if the output SYNCLKO clock should be disabled. Do not mix with >> > > microchip,synclko-125. >> > > >> > > + microchip,rmii-clk-internal: >> > > + $ref: /schemas/types.yaml#/definitions/flag >> > > + description: >> > > + Set if the RMII reference clock should be provided internally. >> > >> > > Applies only >> > > + to KSZ88X3 devices. >> > >> > This should be enforced by the schema, the example schema in the docs >> > should show you how to do this. >> >> I am guessing you are refering to limiting the property to ksz88x3 devices? >> Something like: >> >> if: >> properties: >> compatible: >> enum: >> - microchip,ksz8863 >> - microchip,ksz8873 >> then: >> properties: >> 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. > >Not quite. The definition of the property should be outside the if/then, >but one should be used to allow/disallow the property.
On Tue, Oct 17, 2023 at 09:35:48AM +0200, Ante Knezic wrote: > > > + 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 this bit can become the slightly simpler then: properties: microchip,rmii-clk-internal: false > > 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"? > > If I understood you correctly you are refering to a solution like > if: > properties: > compatible: > enum: > - microchip,ksz8863 > - microchip,ksz8873 > then: > properties: > 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. > > This was already suggested in v1, but was not a satisfactory solution > according to Mr. Conor Dooley: Yeah, we prefer not to have the property definitions inside the conditionals, but rather constrain or allow/disallow them there. Cheers, Conor. > > >> On Tue, 10 Oct 2023 16:25:55 +0100, Conor Dooley wrote: > >> > On Tue, Oct 10, 2023 at 03:18:54PM +0200, Ante Knezic wrote: > >> > > Add documentation for selecting reference rmii clock on KSZ88X3 devices > >> > > > >> > > Signed-off-by: Ante Knezic <ante.knezic@helmholz.de> > >> > > --- > >> > > Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml | 6 ++++++ > >> > > 1 file changed, 6 insertions(+) > >> > > > >> > > diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml > >> > > index e51be1ac0362..3df5d2e72dba 100644 > >> > > --- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml > >> > > +++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml > >> > > @@ -49,6 +49,12 @@ properties: > >> > > Set if the output SYNCLKO clock should be disabled. Do not mix with > >> > > microchip,synclko-125. > >> > > > >> > > + microchip,rmii-clk-internal: > >> > > + $ref: /schemas/types.yaml#/definitions/flag > >> > > + description: > >> > > + Set if the RMII reference clock should be provided internally. > >> > > >> > > Applies only > >> > > + to KSZ88X3 devices. > >> > > >> > This should be enforced by the schema, the example schema in the docs > >> > should show you how to do this. > >> > >> I am guessing you are refering to limiting the property to ksz88x3 devices? > >> Something like: > >> > >> if: > >> properties: > >> compatible: > >> enum: > >> - microchip,ksz8863 > >> - microchip,ksz8873 > >> then: > >> properties: > >> 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. > > > >Not quite. The definition of the property should be outside the if/then, > >but one should be used to allow/disallow the property. >
On Tue, Oct 17, 2023 at 08:48:27AM +0100, Conor Dooley wrote: > On Tue, Oct 17, 2023 at 09:35:48AM +0200, Ante Knezic wrote: > > > > + 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 this bit can become the slightly simpler > then: > properties: > microchip,rmii-clk-internal: false This looks better. I don't understand how the original formulation worked ("not: required:" when the property was never "required" in the first place - does that do anything?), but I understand how this one does. > > > 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"? > > > > If I understood you correctly you are refering to a solution like > > if: > > properties: > > compatible: > > enum: > > - microchip,ksz8863 > > - microchip,ksz8873 > > then: > > properties: > > 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. > > > > This was already suggested in v1, but was not a satisfactory solution > > according to Mr. Conor Dooley: > > Yeah, we prefer not to have the property definitions inside the > conditionals, but rather constrain or allow/disallow them there. > > Cheers, > Conor. Ok, now you know I didn't open the discussion on v1 :)
diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c index 91aba470fb2f..78f3a668aa99 100644 --- a/drivers/net/dsa/microchip/ksz8795.c +++ b/drivers/net/dsa/microchip/ksz8795.c @@ -1434,6 +1434,11 @@ int ksz8_setup(struct dsa_switch *ds) for (i = 0; i < (dev->info->num_vlans / 4); i++) ksz8_r_vlan_entries(dev, i); + if (ksz_is_ksz88x3(dev)) + ksz_cfg(dev, KSZ88X3_REG_FVID_AND_HOST_MODE, + KSZ88X3_PORT3_RMII_CLK_INTERNAL, + dev->rmii_clk_internal); + return ksz8_handle_global_errata(ds); } 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) diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c index b800ace40ce1..0a0a53ce5b1b 100644 --- a/drivers/net/dsa/microchip/ksz_common.c +++ b/drivers/net/dsa/microchip/ksz_common.c @@ -4160,6 +4160,9 @@ int ksz_switch_register(struct ksz_device *dev) } } + dev->rmii_clk_internal = of_property_read_bool(dev->dev->of_node, + "microchip,rmii-clk-internal"); + ret = dsa_register_switch(dev->ds); if (ret) { dev->dev_ops->exit(dev); diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h index 8842efca0871..e5b0445fe2ca 100644 --- a/drivers/net/dsa/microchip/ksz_common.h +++ b/drivers/net/dsa/microchip/ksz_common.h @@ -163,6 +163,7 @@ struct ksz_device { phy_interface_t compat_interface; bool synclko_125; bool synclko_disable; + bool rmii_clk_internal; struct vlan_table *vlan_cache;
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@helmholz.de> --- drivers/net/dsa/microchip/ksz8795.c | 5 +++++ drivers/net/dsa/microchip/ksz8795_reg.h | 3 +++ drivers/net/dsa/microchip/ksz_common.c | 3 +++ drivers/net/dsa/microchip/ksz_common.h | 1 + 4 files changed, 12 insertions(+)