| Message ID | 20210218161451.3489955-1-steen.hegelund@microchip.com |
|---|---|
| Headers | show |
| Series | Adding the Sparx5 Serdes driver | expand |
On Thu, Feb 18, 2021 at 05:14:49PM +0100, Steen Hegelund wrote: > Provide new phy configuration interfaces for media type and speed that > allows e.g. PHYs used for ethernet to be configured with this > information. > > Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com> > Signed-off-by: Steen Hegelund <steen.hegelund@microchip.com> > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > --- > drivers/phy/phy-core.c | 30 ++++++++++++++++++++++++++++++ > include/linux/phy/phy.h | 26 ++++++++++++++++++++++++++ > 2 files changed, 56 insertions(+) > > diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c > index 71cb10826326..ccb575b13777 100644 > --- a/drivers/phy/phy-core.c > +++ b/drivers/phy/phy-core.c > @@ -373,6 +373,36 @@ int phy_set_mode_ext(struct phy *phy, enum phy_mode mode, int submode) > } > EXPORT_SYMBOL_GPL(phy_set_mode_ext); > > +int phy_set_media(struct phy *phy, enum phy_media media) > +{ > + int ret; > + > + if (!phy || !phy->ops->set_media) > + return 0; > + > + mutex_lock(&phy->mutex); > + ret = phy->ops->set_media(phy, media); > + mutex_unlock(&phy->mutex); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(phy_set_media); > + > +int phy_set_speed(struct phy *phy, int speed) > +{ > + int ret; > + > + if (!phy || !phy->ops->set_speed) > + return 0; > + > + mutex_lock(&phy->mutex); > + ret = phy->ops->set_speed(phy, speed); > + mutex_unlock(&phy->mutex); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(phy_set_speed); > + > int phy_reset(struct phy *phy) > { > int ret; > diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h > index e435bdb0bab3..0ed434d02196 100644 > --- a/include/linux/phy/phy.h > +++ b/include/linux/phy/phy.h > @@ -44,6 +44,12 @@ enum phy_mode { > PHY_MODE_DP > }; > > +enum phy_media { > + PHY_MEDIA_DEFAULT, > + PHY_MEDIA_SR, > + PHY_MEDIA_DAC, > +}; > + > /** > * union phy_configure_opts - Opaque generic phy configuration > * > @@ -64,6 +70,8 @@ union phy_configure_opts { > * @power_on: powering on the phy > * @power_off: powering off the phy > * @set_mode: set the mode of the phy > + * @set_media: set the media type of the phy (optional) > + * @set_speed: set the speed of the phy (optional) > * @reset: resetting the phy > * @calibrate: calibrate the phy > * @release: ops to be performed while the consumer relinquishes the PHY > @@ -75,6 +83,8 @@ struct phy_ops { > int (*power_on)(struct phy *phy); > int (*power_off)(struct phy *phy); > int (*set_mode)(struct phy *phy, enum phy_mode mode, int submode); > + int (*set_media)(struct phy *phy, enum phy_media media); > + int (*set_speed)(struct phy *phy, int speed); > > /** > * @configure: > @@ -215,6 +225,8 @@ int phy_power_off(struct phy *phy); > int phy_set_mode_ext(struct phy *phy, enum phy_mode mode, int submode); > #define phy_set_mode(phy, mode) \ > phy_set_mode_ext(phy, mode, 0) > +int phy_set_media(struct phy *phy, enum phy_media media); > +int phy_set_speed(struct phy *phy, int speed); > int phy_configure(struct phy *phy, union phy_configure_opts *opts); > int phy_validate(struct phy *phy, enum phy_mode mode, int submode, > union phy_configure_opts *opts); > @@ -344,6 +356,20 @@ static inline int phy_set_mode_ext(struct phy *phy, enum phy_mode mode, > #define phy_set_mode(phy, mode) \ > phy_set_mode_ext(phy, mode, 0) > > +static inline int phy_set_media(struct phy *phy, enum phy_media media) > +{ > + if (!phy) > + return 0; I'm curious, why do you check for the NULL in all newly introduced functions? How is it possible that calls to phy_*() supply NULL as the main struct? Thanks > + return -ENODEV; > +} > + > +static inline int phy_set_speed(struct phy *phy, int speed) > +{ > + if (!phy) > + return 0; > + return -ENODEV; > +} > + > static inline enum phy_mode phy_get_mode(struct phy *phy) > { > return PHY_MODE_INVALID; > -- > 2.30.0 >
Hi Leon, On Sun, 2021-02-21 at 07:59 +0200, Leon Romanovsky wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Thu, Feb 18, 2021 at 05:14:49PM +0100, Steen Hegelund wrote: > > Provide new phy configuration interfaces for media type and speed > > that > > allows e.g. PHYs used for ethernet to be configured with this > > information. > > > > Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com> > > Signed-off-by: Steen Hegelund <steen.hegelund@microchip.com> > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > > --- > > ... > > int phy_validate(struct phy *phy, enum phy_mode mode, int submode, > > union phy_configure_opts *opts); > > @@ -344,6 +356,20 @@ static inline int phy_set_mode_ext(struct phy > > *phy, enum phy_mode mode, > > #define phy_set_mode(phy, mode) \ > > phy_set_mode_ext(phy, mode, 0) > > > > +static inline int phy_set_media(struct phy *phy, enum phy_media > > media) > > +{ > > + if (!phy) > > + return 0; > > I'm curious, why do you check for the NULL in all newly introduced > functions? > How is it possible that calls to phy_*() supply NULL as the main > struct? > > Thanks I do not know the history of that, but all the functions in the interface that takes a phy as input and returns a status follow that pattern. Maybe Kishon and Vinod knows the origin? > > > + return -ENODEV; > > +} > > + > > +static inline int phy_set_speed(struct phy *phy, int speed) > > +{ > > + if (!phy) > > + return 0; > > + return -ENODEV; > > +} > > + > > static inline enum phy_mode phy_get_mode(struct phy *phy) > > { > > return PHY_MODE_INVALID; > > -- > > 2.30.0 > > Best Regards Steen
Hi Leon, On 22/02/21 1:30 pm, Steen Hegelund wrote: > Hi Leon, > > On Sun, 2021-02-21 at 07:59 +0200, Leon Romanovsky wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you >> know the content is safe >> >> On Thu, Feb 18, 2021 at 05:14:49PM +0100, Steen Hegelund wrote: >>> Provide new phy configuration interfaces for media type and speed >>> that >>> allows e.g. PHYs used for ethernet to be configured with this >>> information. >>> >>> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com> >>> Signed-off-by: Steen Hegelund <steen.hegelund@microchip.com> >>> Reviewed-by: Andrew Lunn <andrew@lunn.ch> >>> Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com> >>> --- >>> > > ... > >>> int phy_validate(struct phy *phy, enum phy_mode mode, int submode, >>> union phy_configure_opts *opts); >>> @@ -344,6 +356,20 @@ static inline int phy_set_mode_ext(struct phy >>> *phy, enum phy_mode mode, >>> #define phy_set_mode(phy, mode) \ >>> phy_set_mode_ext(phy, mode, 0) >>> >>> +static inline int phy_set_media(struct phy *phy, enum phy_media >>> media) >>> +{ >>> + if (!phy) >>> + return 0; >> >> I'm curious, why do you check for the NULL in all newly introduced >> functions? >> How is it possible that calls to phy_*() supply NULL as the main >> struct? >> >> Thanks > > I do not know the history of that, but all the functions in the > interface that takes a phy as input and returns a status follow that > pattern. Maybe Kishon and Vinod knows the origin? It is to make handling optional PHYs simpler. See here for the origin :-) http://lore.kernel.org/r/1391264157-2112-1-git-send-email-andrew@lunn.ch Thanks Kishon > >> >>> + return -ENODEV; >>> +} >>> + >>> +static inline int phy_set_speed(struct phy *phy, int speed) >>> +{ >>> + if (!phy) >>> + return 0; >>> + return -ENODEV; >>> +} >>> + >>> static inline enum phy_mode phy_get_mode(struct phy *phy) >>> { >>> return PHY_MODE_INVALID; >>> -- >>> 2.30.0 >>> > > Best Regards > Steen >
On Tue, Feb 23, 2021 at 05:52:14PM +0530, Kishon Vijay Abraham I wrote: > Hi Leon, > > On 22/02/21 1:30 pm, Steen Hegelund wrote: > > Hi Leon, > > > > On Sun, 2021-02-21 at 07:59 +0200, Leon Romanovsky wrote: > >> EXTERNAL EMAIL: Do not click links or open attachments unless you > >> know the content is safe > >> > >> On Thu, Feb 18, 2021 at 05:14:49PM +0100, Steen Hegelund wrote: > >>> Provide new phy configuration interfaces for media type and speed > >>> that > >>> allows e.g. PHYs used for ethernet to be configured with this > >>> information. > >>> > >>> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com> > >>> Signed-off-by: Steen Hegelund <steen.hegelund@microchip.com> > >>> Reviewed-by: Andrew Lunn <andrew@lunn.ch> > >>> Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > >>> --- > >>> > > > > ... > > > >>> int phy_validate(struct phy *phy, enum phy_mode mode, int submode, > >>> union phy_configure_opts *opts); > >>> @@ -344,6 +356,20 @@ static inline int phy_set_mode_ext(struct phy > >>> *phy, enum phy_mode mode, > >>> #define phy_set_mode(phy, mode) \ > >>> phy_set_mode_ext(phy, mode, 0) > >>> > >>> +static inline int phy_set_media(struct phy *phy, enum phy_media > >>> media) > >>> +{ > >>> + if (!phy) > >>> + return 0; > >> > >> I'm curious, why do you check for the NULL in all newly introduced > >> functions? > >> How is it possible that calls to phy_*() supply NULL as the main > >> struct? > >> > >> Thanks > > > > I do not know the history of that, but all the functions in the > > interface that takes a phy as input and returns a status follow that > > pattern. Maybe Kishon and Vinod knows the origin? > > It is to make handling optional PHYs simpler. See here for the origin :-) > http://lore.kernel.org/r/1391264157-2112-1-git-send-email-andrew@lunn.ch Thanks for the pointer, it is good to know. I personally would do it differently, but whatever. > > Thanks > Kishon > > > >> > >>> + return -ENODEV; > >>> +} > >>> + > >>> +static inline int phy_set_speed(struct phy *phy, int speed) > >>> +{ > >>> + if (!phy) > >>> + return 0; > >>> + return -ENODEV; > >>> +} > >>> + > >>> static inline enum phy_mode phy_get_mode(struct phy *phy) > >>> { > >>> return PHY_MODE_INVALID; > >>> -- > >>> 2.30.0 > >>> > > > > Best Regards > > Steen > >
Hi Kishon, Vinod, Andrew, Jacub, and David, I just wanted to know if you think that the Generic PHY subsystem might not be the right place for this Ethernet SerDes PHY driver after all. Originally I chose this subsystem for historic reasons: The Microchip/Microsemi Ocelot SerDes driver was added here when it was upstreamed. On the other hand the Ocelot Serdes can do both PCIe and Ethernet, so it might fit the signature of a generic PHY better. At the moment the acceptance of the Sparx5 Serdes driver is blocking us from adding the Sparx5 SwitchDev driver (to net), so it is really important for us to resolve which subsystem the Serdes driver belongs to. I am very much looking forward to your response. BR Steen On Thu, 2021-02-18 at 17:14 +0100, Steen Hegelund wrote: > Adding the Sparx5 Serdes driver > > This series of patches provides the serdes driver for the Microchip > Sparx5 > ethernet switch. > > The serdes driver supports the 10G and 25G serdes instances available > in the > Sparx5. > > The Sparx5 serdes support several interface modes with several speeds > and also > allows the client to change the mode and the speed according to > changing in the > environment such as changing cables from DAC to fiber. > > The serdes driver is to be used by the Sparx5 switchdev driver that > will follow in subsequent series. > > Sparx5 Architecture: > ==================== > > Below is a diagram of the Ethernet transport part of the Sparx5 chip. > > The diagram shows the switch core that sends/receives traffic via the > Frame Bus > and passes to the Port Modules. > The Port Modules are able to talk to a SerDes via a Port Muxing > configuration. > The SerDes instances (33 in all) then passes the traffic on its lanes > to the > attached cuPHY or SFP module. > > +---------------------------------------------------------+ > | | > | Switch Core | > | | > +----------------------------+----------------------------+ > | > -------+--------------+------+-------+--------------+-----+ Frame > Bus > | | | | > +------+-----+ +------+-----+ +------+-----+ +------+-----+ > |1G/2.G Port | |5G Port | |10G Port | |25GG Port | > |Modules | |Modules | |Modules | |Modules | > |MAC, PCS | |MAC, PCS | |MAC, PCS | |MAC, PCS | > +------+-----+ +------+-----+ +------+-----+ +------+-----+ > | | | | > -------+-+------------+-------+------+----------+---+-----+ Port > Muxing > | | | > +-----+----+ +-----+----+ +--+-------+ > |SerDes 5G | |SerDes 10G| |SerDes 25G| SerDes > Driver > |Lane (13) | |Lane (12) | |Lane (8) | Controls > these > +-----+----+ +-----+----+ +-----+----+ > | | | > to cuPHY to cuPHY to cuPHY > or SFP or SFP or SFP > > The 33 SerDes instances are handled internally by 2 SerDes macros > types: > > - A 10G SerDes macro that supports the following rates and modes: > - 100 Mbps: > - 100BASE-FX > - 1.25 Gbps: > - SGMII > - 1000BASE-X > - 1000BASE-KX > - 3.125 Gbps: > - 2.5GBASE-X > - 2.5GBASE-KX > - 5 Gbps: > - QSGMII > - USGMII > - 5.15625 Gbps: > - 5GBASE-KR > - 5G-USXGMII > - 10 Gbps: > - 10G-USGMII > - 10.3125 Gbps: > - 10GBASE-R > - 10GBASE-KR > - USXGMII > > - A 25G SerDes macro that supports the following rates and modes: > - 1.25 Gbps: > - SGMII > - 1000BASE-X > - 1000BASE-KX > - 3.125 Gbps: > - 2.5GBASE-X > - 2.5GBASE-KX > - 5 Gbps: > - QSGMII > - USGMII > - 5.15625 Gbps: > - 5GBASE-KR > - 5G-USXGMII > - 10 Gbps: > - 10G-USGMII > - 10.3125 Gbps: > - 10GBASE-R > - 10GBASE-KR > - USXGMII > - 10.3125 Gbps: > - 10GBASE-R > - 10GBASE-KR > - USXGMII > - 25.78125 Gbps: > - 25GBASE-KR > - 25GBASE-CR > - 25GBASE-SR > - 25GBASE-LR > - 25GBASE-ER > > The SerDes driver handles these SerDes instances and configures them > based on > the selected mode, speed and media type. > > In the current version of the SerDes driver only a subset of the above > modes > are supported: the modes that can be tested on our current evaluation > boards > (PCB134 and PCB35). > > The first 13 10G SerDes macros are limited to 6G, and this gives the > SerDes > instance architecture shown on the diagram above. > > The Port Muxing allows a Port Module to use a specific SerDes instance, > but not > all combinations are allowed. > This is functionality as well as the configuration of the Port Modules > is > handled by the SwitchDev Driver. > > The Sparx5 Chip Register Model can be browsed at this location: > https://github.com/microchip-ung/sparx-5_reginfo > and the datasheet is available here: > https://ww1.microchip.com/downloads/en/DeviceDoc/SparX-5_Family_L2L3_Enterprise_10G_Ethernet_Switches_Datasheet_00003822B.pdf > > History: > -------- > v14 -> v15: > Changed the default interface function return value to -ENODEV. > Moved the CMU initialization, so that it is always done before the > serdes instance configuration. > Added a reference to the Sparx5 datasheet. > > v13 -> v14: > Changed the 25g apply, 10g apply and the CMU configuration > functions to > use a table based register update structure. > The table entries still need serdes indices/instances so the table > must > be generated per serdes. > > v12 -> v13: > Interface changes: > - Added set_media and set_speed interfaces on the generic phy > - Removed the ethernet SerDes configuration structure and its > header file. > Implementation changes: > - Implemented the new media and speed interfaces in the Serdes > driver > - Removed the configure interface function in the SerDes driver > - The existing configuration function is now only used > internally > > v11 -> v12: > Used bitfields for bools in configuration structures. > Removed vertical alignment in structures. > Removed CONFIG_DEBUG_KERNEL guard around warning checks > > v10 -> v11: > Rebased on v5.11-rc1 > > v9 -> v10: > Only add the new folder to the phy Kconfig (no sort fix) > Corrected the serdes mode conversion for 2.5G mode. > Clarified the SGMII and 1000BASEX conversion. > Improved some of the more cryptic error messages. > Expanded the validate function a bit, and removed the link status > from the return value. > > v8 -> v9: > Replace pr_err with dev_err > Expanded the description here in the cover letter (should probably > og into > the driver, at least part of it). > > v7 -> v8: > Provide the IO targets as offsets from the start of the IO range > Initialise resource index > > v6 -> v7: > This series changes the way the IO targets are provided to the > driver. > Now only one IO range is available in the DT, and the driver has a > table > to map its targets (as their order is still not sequential), thus > reducing > the DT needed information and binding requirements. > The register access macros have been converted to functions. > > - Bindings: > - reg prop: minItems set to 1 > - reg-names prop: removed > - Driver > - Use one IO range and map targets via this. > - Change register access macros to use functions. > - Provided a new header files with reg access functions. > - Device tree > - Provide only one IO range > > v5 -> v6: > Series error: This had the same content as v5 > > v4 -> v5: > - Bindings: > - Removed .yaml from compatible string > - reg prop: removed description and added minItems > - reg-names prop: removed description and added const name list > and > minItems > - #phy-cells prop: removed description and added maxItems > - Configuration interface > - Removed include of linux/phy.h > - Added include of linux/types.h > - Driver > - Added include of linux/phy.h > > v3 -> v4: > - Add a reg-names item to the binding description > - Add a clocks item to the binding description > - Removed the clock parameter from the configuration interface > - Use the clock dt node to get the coreclock, and using that when > doing the actual serdes configuration > - Added a clocks entry with a system clock reference to the serdes > node in > the device tree > > v2 -> v3: > - Sorted the Kconfig sourced folders > - Sorted the Makefile included folders > - Changed the configuration interface documentation to use kernel > style > > v1 -> v2: Fixed kernel test robot warnings > - Made these structures static: > - media_presets_25g > - mode_presets_25g > - media_presets_10g > - mode_presets_10g > - Removed these duplicate initializations: > - sparx5_sd25g28_params.cfg_rx_reserve_15_8 > - sparx5_sd25g28_params.cfg_pi_en > - sparx5_sd25g28_params.cfg_cdrck_en > - sparx5_sd10g28_params.cfg_cdrck_en > > Steen Hegelund (4): > dt-bindings: phy: Add sparx5-serdes bindings > phy: Add media type and speed serdes configuration interfaces > phy: Add Sparx5 ethernet serdes PHY driver > arm64: dts: sparx5: Add Sparx5 serdes driver node > > .../bindings/phy/microchip,sparx5-serdes.yaml | 100 + > arch/arm64/boot/dts/microchip/sparx5.dtsi | 8 + > drivers/phy/Kconfig | 1 + > drivers/phy/Makefile | 1 + > drivers/phy/microchip/Kconfig | 12 + > drivers/phy/microchip/Makefile | 6 + > drivers/phy/microchip/sparx5_serdes.c | 2480 +++++++++++++++ > drivers/phy/microchip/sparx5_serdes.h | 136 + > drivers/phy/microchip/sparx5_serdes_regs.h | 2695 +++++++++++++++++ > drivers/phy/phy-core.c | 30 + > include/linux/phy/phy.h | 26 + > 11 files changed, 5495 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/phy/microchip,sparx5-serdes.yaml > create mode 100644 drivers/phy/microchip/Kconfig > create mode 100644 drivers/phy/microchip/Makefile > create mode 100644 drivers/phy/microchip/sparx5_serdes.c > create mode 100644 drivers/phy/microchip/sparx5_serdes.h > create mode 100644 drivers/phy/microchip/sparx5_serdes_regs.h > > -- > 2.30.0 >
On Mon, 15 Mar 2021 16:04:24 +0100 Steen Hegelund wrote: > Hi Kishon, Vinod, Andrew, Jacub, and David, > > I just wanted to know if you think that the Generic PHY subsystem might > not be the right place for this Ethernet SerDes PHY driver after all. > > Originally I chose this subsystem for historic reasons: The > Microchip/Microsemi Ocelot SerDes driver was added here when it was > upstreamed. > On the other hand the Ocelot Serdes can do both PCIe and Ethernet, so > it might fit the signature of a generic PHY better. Are you saying this PHY is Ethernet only? > At the moment the acceptance of the Sparx5 Serdes driver is blocking us > from adding the Sparx5 SwitchDev driver (to net), so it is really > important for us to resolve which subsystem the Serdes driver belongs > to. > > I am very much looking forward to your response. FWIW even if this is merged via gen phy subsystem we can pull it into net-next as well to unblock your other work in this dev cycle. You just need to send the patches as a pull request, based on merge-base between the gen phy tree and net-next.
Hello Steen, On 15-03-21, 16:04, Steen Hegelund wrote: > Hi Kishon, Vinod, Andrew, Jacub, and David, > > I just wanted to know if you think that the Generic PHY subsystem might > not be the right place for this Ethernet SerDes PHY driver after all. > > Originally I chose this subsystem for historic reasons: The > Microchip/Microsemi Ocelot SerDes driver was added here when it was > upstreamed. > On the other hand the Ocelot Serdes can do both PCIe and Ethernet, so > it might fit the signature of a generic PHY better. > > At the moment the acceptance of the Sparx5 Serdes driver is blocking us > from adding the Sparx5 SwitchDev driver (to net), so it is really > important for us to resolve which subsystem the Serdes driver belongs > to. > > I am very much looking forward to your response. Generic PHY IMO is the right place for this series, I shall review it shortly and do the needful. I have asked Kishon to check the new phy API and ack it... Thanks -- ~Vinod
Hi Jacub, On Mon, 2021-03-15 at 10:26 -0700, Jakub Kicinski wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Mon, 15 Mar 2021 16:04:24 +0100 Steen Hegelund wrote: > > Hi Kishon, Vinod, Andrew, Jacub, and David, > > > > I just wanted to know if you think that the Generic PHY subsystem might > > not be the right place for this Ethernet SerDes PHY driver after all. > > > > Originally I chose this subsystem for historic reasons: The > > Microchip/Microsemi Ocelot SerDes driver was added here when it was > > upstreamed. > > On the other hand the Ocelot Serdes can do both PCIe and Ethernet, so > > it might fit the signature of a generic PHY better. > > Are you saying this PHY is Ethernet only? Yes this particular PHY is Ethernet only (but the Sparx5 also has a separate PCI PHY). > > > At the moment the acceptance of the Sparx5 Serdes driver is blocking us > > from adding the Sparx5 SwitchDev driver (to net), so it is really > > important for us to resolve which subsystem the Serdes driver belongs > > to. > > > > I am very much looking forward to your response. > > FWIW even if this is merged via gen phy subsystem we can pull it into > net-next as well to unblock your other work in this dev cycle. You just > need to send the patches as a pull request, based on merge-base between > the gen phy tree and net-next. -- BR Steen -=-=-=-=-=-=-=-=-=-=-=-=-=-= steen.hegelund@microchip.com
Hi Vinod, On Tue, 2021-03-16 at 10:23 +0530, Vinod Koul wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Hello Steen, > > On 15-03-21, 16:04, Steen Hegelund wrote: > > Hi Kishon, Vinod, Andrew, Jacub, and David, > > > > I just wanted to know if you think that the Generic PHY subsystem might > > not be the right place for this Ethernet SerDes PHY driver after all. > > > > Originally I chose this subsystem for historic reasons: The > > Microchip/Microsemi Ocelot SerDes driver was added here when it was > > upstreamed. > > On the other hand the Ocelot Serdes can do both PCIe and Ethernet, so > > it might fit the signature of a generic PHY better. > > > > At the moment the acceptance of the Sparx5 Serdes driver is blocking us > > from adding the Sparx5 SwitchDev driver (to net), so it is really > > important for us to resolve which subsystem the Serdes driver belongs > > to. > > > > I am very much looking forward to your response. > > Generic PHY IMO is the right place for this series, I shall review it > shortly and do the needful. I have asked Kishon to check the new phy API > and ack it... > > Thanks > -- > ~Vinod Thank you very much for the confirmation. BR Steen
On 18/02/21 9:44 pm, Steen Hegelund wrote: > Provide new phy configuration interfaces for media type and speed that > allows e.g. PHYs used for ethernet to be configured with this > information. > > Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com> > Signed-off-by: Steen Hegelund <steen.hegelund@microchip.com> > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com> Acked-By: Kishon Vijay Abraham I <kishon@ti.com> > --- > drivers/phy/phy-core.c | 30 ++++++++++++++++++++++++++++++ > include/linux/phy/phy.h | 26 ++++++++++++++++++++++++++ > 2 files changed, 56 insertions(+) > > diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c > index 71cb10826326..ccb575b13777 100644 > --- a/drivers/phy/phy-core.c > +++ b/drivers/phy/phy-core.c > @@ -373,6 +373,36 @@ int phy_set_mode_ext(struct phy *phy, enum phy_mode mode, int submode) > } > EXPORT_SYMBOL_GPL(phy_set_mode_ext); > > +int phy_set_media(struct phy *phy, enum phy_media media) > +{ > + int ret; > + > + if (!phy || !phy->ops->set_media) > + return 0; > + > + mutex_lock(&phy->mutex); > + ret = phy->ops->set_media(phy, media); > + mutex_unlock(&phy->mutex); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(phy_set_media); > + > +int phy_set_speed(struct phy *phy, int speed) > +{ > + int ret; > + > + if (!phy || !phy->ops->set_speed) > + return 0; > + > + mutex_lock(&phy->mutex); > + ret = phy->ops->set_speed(phy, speed); > + mutex_unlock(&phy->mutex); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(phy_set_speed); > + > int phy_reset(struct phy *phy) > { > int ret; > diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h > index e435bdb0bab3..0ed434d02196 100644 > --- a/include/linux/phy/phy.h > +++ b/include/linux/phy/phy.h > @@ -44,6 +44,12 @@ enum phy_mode { > PHY_MODE_DP > }; > > +enum phy_media { > + PHY_MEDIA_DEFAULT, > + PHY_MEDIA_SR, > + PHY_MEDIA_DAC, > +}; > + > /** > * union phy_configure_opts - Opaque generic phy configuration > * > @@ -64,6 +70,8 @@ union phy_configure_opts { > * @power_on: powering on the phy > * @power_off: powering off the phy > * @set_mode: set the mode of the phy > + * @set_media: set the media type of the phy (optional) > + * @set_speed: set the speed of the phy (optional) > * @reset: resetting the phy > * @calibrate: calibrate the phy > * @release: ops to be performed while the consumer relinquishes the PHY > @@ -75,6 +83,8 @@ struct phy_ops { > int (*power_on)(struct phy *phy); > int (*power_off)(struct phy *phy); > int (*set_mode)(struct phy *phy, enum phy_mode mode, int submode); > + int (*set_media)(struct phy *phy, enum phy_media media); > + int (*set_speed)(struct phy *phy, int speed); > > /** > * @configure: > @@ -215,6 +225,8 @@ int phy_power_off(struct phy *phy); > int phy_set_mode_ext(struct phy *phy, enum phy_mode mode, int submode); > #define phy_set_mode(phy, mode) \ > phy_set_mode_ext(phy, mode, 0) > +int phy_set_media(struct phy *phy, enum phy_media media); > +int phy_set_speed(struct phy *phy, int speed); > int phy_configure(struct phy *phy, union phy_configure_opts *opts); > int phy_validate(struct phy *phy, enum phy_mode mode, int submode, > union phy_configure_opts *opts); > @@ -344,6 +356,20 @@ static inline int phy_set_mode_ext(struct phy *phy, enum phy_mode mode, > #define phy_set_mode(phy, mode) \ > phy_set_mode_ext(phy, mode, 0) > > +static inline int phy_set_media(struct phy *phy, enum phy_media media) > +{ > + if (!phy) > + return 0; > + return -ENODEV; > +} > + > +static inline int phy_set_speed(struct phy *phy, int speed) > +{ > + if (!phy) > + return 0; > + return -ENODEV; > +} > + > static inline enum phy_mode phy_get_mode(struct phy *phy) > { > return PHY_MODE_INVALID; >
On 18-02-21, 17:14, Steen Hegelund wrote: > Adding the Sparx5 Serdes driver > > This series of patches provides the serdes driver for the Microchip Sparx5 > ethernet switch. > > The serdes driver supports the 10G and 25G serdes instances available in the > Sparx5. > > The Sparx5 serdes support several interface modes with several speeds and also > allows the client to change the mode and the speed according to changing in the > environment such as changing cables from DAC to fiber. Applied patch 1 thru 3... thanks -- ~Vinod
Adding the Sparx5 Serdes driver This series of patches provides the serdes driver for the Microchip Sparx5 ethernet switch. The serdes driver supports the 10G and 25G serdes instances available in the Sparx5. The Sparx5 serdes support several interface modes with several speeds and also allows the client to change the mode and the speed according to changing in the environment such as changing cables from DAC to fiber. The serdes driver is to be used by the Sparx5 switchdev driver that will follow in subsequent series. Sparx5 Architecture: ==================== Below is a diagram of the Ethernet transport part of the Sparx5 chip. The diagram shows the switch core that sends/receives traffic via the Frame Bus and passes to the Port Modules. The Port Modules are able to talk to a SerDes via a Port Muxing configuration. The SerDes instances (33 in all) then passes the traffic on its lanes to the attached cuPHY or SFP module. +---------------------------------------------------------+ | | | Switch Core | | | +----------------------------+----------------------------+ | -------+--------------+------+-------+--------------+-----+ Frame Bus | | | | +------+-----+ +------+-----+ +------+-----+ +------+-----+ |1G/2.G Port | |5G Port | |10G Port | |25GG Port | |Modules | |Modules | |Modules | |Modules | |MAC, PCS | |MAC, PCS | |MAC, PCS | |MAC, PCS | +------+-----+ +------+-----+ +------+-----+ +------+-----+ | | | | -------+-+------------+-------+------+----------+---+-----+ Port Muxing | | | +-----+----+ +-----+----+ +--+-------+ |SerDes 5G | |SerDes 10G| |SerDes 25G| SerDes Driver |Lane (13) | |Lane (12) | |Lane (8) | Controls these +-----+----+ +-----+----+ +-----+----+ | | | to cuPHY to cuPHY to cuPHY or SFP or SFP or SFP The 33 SerDes instances are handled internally by 2 SerDes macros types: - A 10G SerDes macro that supports the following rates and modes: - 100 Mbps: - 100BASE-FX - 1.25 Gbps: - SGMII - 1000BASE-X - 1000BASE-KX - 3.125 Gbps: - 2.5GBASE-X - 2.5GBASE-KX - 5 Gbps: - QSGMII - USGMII - 5.15625 Gbps: - 5GBASE-KR - 5G-USXGMII - 10 Gbps: - 10G-USGMII - 10.3125 Gbps: - 10GBASE-R - 10GBASE-KR - USXGMII - A 25G SerDes macro that supports the following rates and modes: - 1.25 Gbps: - SGMII - 1000BASE-X - 1000BASE-KX - 3.125 Gbps: - 2.5GBASE-X - 2.5GBASE-KX - 5 Gbps: - QSGMII - USGMII - 5.15625 Gbps: - 5GBASE-KR - 5G-USXGMII - 10 Gbps: - 10G-USGMII - 10.3125 Gbps: - 10GBASE-R - 10GBASE-KR - USXGMII - 10.3125 Gbps: - 10GBASE-R - 10GBASE-KR - USXGMII - 25.78125 Gbps: - 25GBASE-KR - 25GBASE-CR - 25GBASE-SR - 25GBASE-LR - 25GBASE-ER The SerDes driver handles these SerDes instances and configures them based on the selected mode, speed and media type. In the current version of the SerDes driver only a subset of the above modes are supported: the modes that can be tested on our current evaluation boards (PCB134 and PCB35). The first 13 10G SerDes macros are limited to 6G, and this gives the SerDes instance architecture shown on the diagram above. The Port Muxing allows a Port Module to use a specific SerDes instance, but not all combinations are allowed. This is functionality as well as the configuration of the Port Modules is handled by the SwitchDev Driver. The Sparx5 Chip Register Model can be browsed at this location: https://github.com/microchip-ung/sparx-5_reginfo and the datasheet is available here: https://ww1.microchip.com/downloads/en/DeviceDoc/SparX-5_Family_L2L3_Enterprise_10G_Ethernet_Switches_Datasheet_00003822B.pdf History: -------- v14 -> v15: Changed the default interface function return value to -ENODEV. Moved the CMU initialization, so that it is always done before the serdes instance configuration. Added a reference to the Sparx5 datasheet. v13 -> v14: Changed the 25g apply, 10g apply and the CMU configuration functions to use a table based register update structure. The table entries still need serdes indices/instances so the table must be generated per serdes. v12 -> v13: Interface changes: - Added set_media and set_speed interfaces on the generic phy - Removed the ethernet SerDes configuration structure and its header file. Implementation changes: - Implemented the new media and speed interfaces in the Serdes driver - Removed the configure interface function in the SerDes driver - The existing configuration function is now only used internally v11 -> v12: Used bitfields for bools in configuration structures. Removed vertical alignment in structures. Removed CONFIG_DEBUG_KERNEL guard around warning checks v10 -> v11: Rebased on v5.11-rc1 v9 -> v10: Only add the new folder to the phy Kconfig (no sort fix) Corrected the serdes mode conversion for 2.5G mode. Clarified the SGMII and 1000BASEX conversion. Improved some of the more cryptic error messages. Expanded the validate function a bit, and removed the link status from the return value. v8 -> v9: Replace pr_err with dev_err Expanded the description here in the cover letter (should probably og into the driver, at least part of it). v7 -> v8: Provide the IO targets as offsets from the start of the IO range Initialise resource index v6 -> v7: This series changes the way the IO targets are provided to the driver. Now only one IO range is available in the DT, and the driver has a table to map its targets (as their order is still not sequential), thus reducing the DT needed information and binding requirements. The register access macros have been converted to functions. - Bindings: - reg prop: minItems set to 1 - reg-names prop: removed - Driver - Use one IO range and map targets via this. - Change register access macros to use functions. - Provided a new header files with reg access functions. - Device tree - Provide only one IO range v5 -> v6: Series error: This had the same content as v5 v4 -> v5: - Bindings: - Removed .yaml from compatible string - reg prop: removed description and added minItems - reg-names prop: removed description and added const name list and minItems - #phy-cells prop: removed description and added maxItems - Configuration interface - Removed include of linux/phy.h - Added include of linux/types.h - Driver - Added include of linux/phy.h v3 -> v4: - Add a reg-names item to the binding description - Add a clocks item to the binding description - Removed the clock parameter from the configuration interface - Use the clock dt node to get the coreclock, and using that when doing the actual serdes configuration - Added a clocks entry with a system clock reference to the serdes node in the device tree v2 -> v3: - Sorted the Kconfig sourced folders - Sorted the Makefile included folders - Changed the configuration interface documentation to use kernel style v1 -> v2: Fixed kernel test robot warnings - Made these structures static: - media_presets_25g - mode_presets_25g - media_presets_10g - mode_presets_10g - Removed these duplicate initializations: - sparx5_sd25g28_params.cfg_rx_reserve_15_8 - sparx5_sd25g28_params.cfg_pi_en - sparx5_sd25g28_params.cfg_cdrck_en - sparx5_sd10g28_params.cfg_cdrck_en Steen Hegelund (4): dt-bindings: phy: Add sparx5-serdes bindings phy: Add media type and speed serdes configuration interfaces phy: Add Sparx5 ethernet serdes PHY driver arm64: dts: sparx5: Add Sparx5 serdes driver node .../bindings/phy/microchip,sparx5-serdes.yaml | 100 + arch/arm64/boot/dts/microchip/sparx5.dtsi | 8 + drivers/phy/Kconfig | 1 + drivers/phy/Makefile | 1 + drivers/phy/microchip/Kconfig | 12 + drivers/phy/microchip/Makefile | 6 + drivers/phy/microchip/sparx5_serdes.c | 2480 +++++++++++++++ drivers/phy/microchip/sparx5_serdes.h | 136 + drivers/phy/microchip/sparx5_serdes_regs.h | 2695 +++++++++++++++++ drivers/phy/phy-core.c | 30 + include/linux/phy/phy.h | 26 + 11 files changed, 5495 insertions(+) create mode 100644 Documentation/devicetree/bindings/phy/microchip,sparx5-serdes.yaml create mode 100644 drivers/phy/microchip/Kconfig create mode 100644 drivers/phy/microchip/Makefile create mode 100644 drivers/phy/microchip/sparx5_serdes.c create mode 100644 drivers/phy/microchip/sparx5_serdes.h create mode 100644 drivers/phy/microchip/sparx5_serdes_regs.h -- 2.30.0