| Message ID | 20230323103154.264546-1-herve.codina@bootlin.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add support for QMC HDLC and PHY | expand |
Hi all, I haven't received any feedback on this RFC. Have you had a chance to review it ? Best regards, Hervé On Thu, 23 Mar 2023 11:31:50 +0100 Herve Codina <herve.codina@bootlin.com> wrote: > Hi, > > I have a system where I need to handle an HDLC interface. > > The HDLC data are transferred using a TDM bus on which a PEF2256 is > present. The PEF2256 transfers data from/to the TDM bus to/from E1 line. > This PEF2256 is also connected to a PowerQUICC SoC for the control path > and the TDM is connected to the SoC (QMC component) for the data path. > > From the HDLC driver, I need to handle data using the QMC and carrier > detection using the PEF2256 (E1 line carrier). > > The HDLC driver consider the PEF2256 as a generic PHY. > So, the design is the following: > > +----------+ +-------------+ +---------+ > | HDLC drv | <-data-> | QMC channel | <-- TDM --> | PEF2256 | > +----------+ +-------------+ | | <--> E1 > ^ +---------+ +---------+ | | > +-> | Gen PHY | <-> | PEF2256 | <- local bus -> | | > +---------+ | PHY drv | +---------+ > +---------+ > > In order to implement this, I had to: > 1 - Extend the generic PHY API to support get_status() and notification > on status change. > 2 - Introduce a new kind of generic PHY named "basic phy". This PHY > familly can provide a link status in the get_status() data. > 3 - Support the PEF2256 PHY as a "basic phy" > > The purpose of this RFC series is to discuss this design. > > The QMC driver code is available on linux-next. In this series: > - patch 1: driver HDLC using the QMC channel > - patch 2: Extend the generic PHY API > - patch 3: Use the "basic phy" in the HDLC driver > - patch 4: Implement the PEF2256 PHY driver > > I did 2 patches for the HDLC driver in order to point the new PHY family > usage in the HDLC driver. In the end, these two patches will be squashed > and the bindings will be added. > > Hope to have some feedback on this proposal. > > Best regards, > Hervé > > Herve Codina (4): > net: wan: Add support for QMC HDLC > phy: Extend API to support 'status' get and notification > net: wan: fsl_qmc_hdlc: Add PHY support > phy: lantiq: Add PEF2256 PHY support > > drivers/net/wan/Kconfig | 12 + > drivers/net/wan/Makefile | 1 + > drivers/net/wan/fsl_qmc_hdlc.c | 558 ++++++++++++++++++++++++ > drivers/phy/lantiq/Kconfig | 15 + > drivers/phy/lantiq/Makefile | 1 + > drivers/phy/lantiq/phy-lantiq-pef2256.c | 131 ++++++ > drivers/phy/phy-core.c | 88 ++++ > include/linux/phy/phy-basic.h | 27 ++ > include/linux/phy/phy.h | 89 +++- > 9 files changed, 921 insertions(+), 1 deletion(-) > create mode 100644 drivers/net/wan/fsl_qmc_hdlc.c > create mode 100644 drivers/phy/lantiq/phy-lantiq-pef2256.c > create mode 100644 include/linux/phy/phy-basic.h >
On Thu, Mar 23, 2023 at 11:31:50AM +0100, Herve Codina wrote: > Hi, > > I have a system where I need to handle an HDLC interface. > > The HDLC data are transferred using a TDM bus on which a PEF2256 is > present. The PEF2256 transfers data from/to the TDM bus to/from E1 line. > This PEF2256 is also connected to a PowerQUICC SoC for the control path > and the TDM is connected to the SoC (QMC component) for the data path. > > From the HDLC driver, I need to handle data using the QMC and carrier > detection using the PEF2256 (E1 line carrier). > > The HDLC driver consider the PEF2256 as a generic PHY. > So, the design is the following: > > +----------+ +-------------+ +---------+ > | HDLC drv | <-data-> | QMC channel | <-- TDM --> | PEF2256 | > +----------+ +-------------+ | | <--> E1 > ^ +---------+ +---------+ | | > +-> | Gen PHY | <-> | PEF2256 | <- local bus -> | | > +---------+ | PHY drv | +---------+ > +---------+ Hi Herver Sorry, i'm late to the conversation. I'm looking at this from two different perspectives. I help maintain Ethernet PHYs. And i have hacked on the IDT 82P2288 E1/T1/J1 framer. I think there is a block missing from this diagram. There appears to be an MFD driver for the PEF2256? At least, i see an include for linux/mfd/pef2256.h. When i look at the 'phy' driver, i don't see anything a typical PHY driver used for networking would have. A networking PHY driver often has the ability to change between modes, like SGMII, QSGMII, 10GBASER. The equivalent here would be changing between E1, T1 and J1. It has the ability to change the speed, 1G, 2.5G, 10G etc. This could be implied via the mode, E1 is 2.048Mbps, T1 1.544Mbps, and i forget what J1 is. The PEF2256 also seems to support E1/T1/J1. How is its modes configured? In fact, this PHY driver does not seem to do any configuration of any sort on the framer. All it seems to be doing is take notification from one chain and send them out another chain! I also wounder if this get_status() call is sufficient. Don't you also want Red, Yellow and Blue alarms? It is not just the carrier is down, but why it is down. Overall, i don't see why you want a PHY. What value does it add? Andrew
Hi Andrew, On Thu, 13 Apr 2023 14:48:30 +0200 Andrew Lunn <andrew@lunn.ch> wrote: > On Thu, Mar 23, 2023 at 11:31:50AM +0100, Herve Codina wrote: > > Hi, > > > > I have a system where I need to handle an HDLC interface. > > > > The HDLC data are transferred using a TDM bus on which a PEF2256 is > > present. The PEF2256 transfers data from/to the TDM bus to/from E1 line. > > This PEF2256 is also connected to a PowerQUICC SoC for the control path > > and the TDM is connected to the SoC (QMC component) for the data path. > > > > From the HDLC driver, I need to handle data using the QMC and carrier > > detection using the PEF2256 (E1 line carrier). > > > > The HDLC driver consider the PEF2256 as a generic PHY. > > So, the design is the following: > > > > +----------+ +-------------+ +---------+ > > | HDLC drv | <-data-> | QMC channel | <-- TDM --> | PEF2256 | > > +----------+ +-------------+ | | <--> E1 > > ^ +---------+ +---------+ | | > > +-> | Gen PHY | <-> | PEF2256 | <- local bus -> | | > > +---------+ | PHY drv | +---------+ > > +---------+ > > Hi Herver > > Sorry, i'm late to the conversation. I'm looking at this from two > different perspectives. I help maintain Ethernet PHYs. And i have > hacked on the IDT 82P2288 E1/T1/J1 framer. > > I think there is a block missing from this diagram. There appears to > be an MFD driver for the PEF2256? At least, i see an include for > linux/mfd/pef2256.h. Indeed, there is the MFD driver and this MFD driver does the PEF2256 setup (line configuration, speed, ...). > > When i look at the 'phy' driver, i don't see anything a typical PHY > driver used for networking would have. A networking PHY driver often > has the ability to change between modes, like SGMII, QSGMII, 10GBASER. > The equivalent here would be changing between E1, T1 and J1. It has > the ability to change the speed, 1G, 2.5G, 10G etc. This could be > implied via the mode, E1 is 2.048Mbps, T1 1.544Mbps, and i forget what > J1 is. The PEF2256 also seems to support E1/T1/J1. How is its modes > configured? All of these are set by the MFD driver during its probe(). The expected setting come from several properties present in the pef2256 DT node. The binding can be found here: https://lore.kernel.org/all/20230328092645.634375-2-herve.codina@bootlin.com/ Further more, the QMC HDLC is not the only PEF2256 consumer. The PEF2256 is also used for audio path (ie audio over E1) and so the configuration is shared between network and audio. The setting cannot be handle by the network part as the PEF2256 must be available and correctly configured even if the network part is not present. > > In fact, this PHY driver does not seem to do any configuration of any > sort on the framer. All it seems to be doing is take notification from > one chain and send them out another chain! Configuration is done by the parent MFD driver. The PHY driver has nothing more to do. > > I also wounder if this get_status() call is sufficient. Don't you also > want Red, Yellow and Blue alarms? It is not just the carrier is down, > but why it is down. I don't need them in my use case but if needed can't they be added later? Also, from the HDLC device point of view what can be done with these alarms? > > Overall, i don't see why you want a PHY. What value does it add? I need to detect carrier on/off according to the E1 link state. The HDLC driver is a driver for a QMC device. The QMC device present in some PowerPC SOC offers the possibility to send data over a TDM bus. From the QMC HDLC driver I don't want to refer the PEF2256 as the driver has nothing to do with the PEF2256 directly. The QMC HDLC driver send data to a TDM bus using the QMC device. The PEF2256 is an interface in the data path between the QMC output (TDM bus) and the E1 line. We send HDLC over E1 because there is this kind of interface but we would have sent HDLC over anything else if this interface was different. Using a PHY to represent this interface was coherent for me. Using the generic PHY subsystem allows to abstract the specific provider (PEF2256) from the consumer (QMC HDLC). Best regards, Hervé
> > When i look at the 'phy' driver, i don't see anything a typical PHY > > driver used for networking would have. A networking PHY driver often > > has the ability to change between modes, like SGMII, QSGMII, 10GBASER. > > The equivalent here would be changing between E1, T1 and J1. It has > > the ability to change the speed, 1G, 2.5G, 10G etc. This could be > > implied via the mode, E1 is 2.048Mbps, T1 1.544Mbps, and i forget what > > J1 is. The PEF2256 also seems to support E1/T1/J1. How is its modes > > configured? > > All of these are set by the MFD driver during its probe(). > The expected setting come from several properties present in the pef2256 > DT node. The binding can be found here: > https://lore.kernel.org/all/20230328092645.634375-2-herve.codina@bootlin.com/ I'm surprised to see so much in the binding. I assume you are familiar with DAHDI. It allows nearly everything to be configured at runtime. The systems i've used allow you to select the clock configuration, line build out, user side vs networks side signalling CRC4 enables or not, etc. > Further more, the QMC HDLC is not the only PEF2256 consumer. > The PEF2256 is also used for audio path (ie audio over E1) and so the > configuration is shared between network and audio. The setting cannot be > handle by the network part as the PEF2256 must be available and correctly > configured even if the network part is not present. But there is no reason why the MFD could not provide a generic PHY to actually configure the 'PHY'. The HDLC driver can then also use the generic PHY. It would make your generic PHY less 'pointless'. I'm not saying it has to be this way, but it is an option. > > In fact, this PHY driver does not seem to do any configuration of any > > sort on the framer. All it seems to be doing is take notification from > > one chain and send them out another chain! > > Configuration is done by the parent MFD driver. > The PHY driver has nothing more to do. > > > > > I also wounder if this get_status() call is sufficient. Don't you also > > want Red, Yellow and Blue alarms? It is not just the carrier is down, > > but why it is down. > > I don't need them in my use case but if needed can't they be added later? > Also, from the HDLC device point of view what can be done with these alarms? https://elixir.bootlin.com/linux/latest/source/Documentation/networking/ethtool-netlink.rst#L472 > Requests link state information. Link up/down flag (as provided by > ``ETHTOOL_GLINK`` ioctl command) is provided. Optionally, extended > state might be provided as well. In general, extended state > describes reasons for why a port is down, or why it operates in some > non-obvious mode. The colour of the Alarm gives you an idea which end of the system has the problem. > > Overall, i don't see why you want a PHY. What value does it add? > > I need to detect carrier on/off according to the E1 link state. Why not just use the MFD notifier? What is the value of a PHY driver translating one notifier into another? And why is the notifier specific to the PEF2256? What would happen if i used a analog devices DS2155, DS21Q55, and DS2156, or the IDT 82P2281? Would each have its own notifier? And hence each would need its own PHY which translates one notifier into another? There are enough E1/T1/J1 framers we should have a generic API between the framer and the HDLC device. Andrew
Hi Andrew On Fri, 14 Apr 2023 18:15:30 +0200 Andrew Lunn <andrew@lunn.ch> wrote: > > > When i look at the 'phy' driver, i don't see anything a typical PHY > > > driver used for networking would have. A networking PHY driver often > > > has the ability to change between modes, like SGMII, QSGMII, 10GBASER. > > > The equivalent here would be changing between E1, T1 and J1. It has > > > the ability to change the speed, 1G, 2.5G, 10G etc. This could be > > > implied via the mode, E1 is 2.048Mbps, T1 1.544Mbps, and i forget what > > > J1 is. The PEF2256 also seems to support E1/T1/J1. How is its modes > > > configured? > > > > All of these are set by the MFD driver during its probe(). > > The expected setting come from several properties present in the pef2256 > > DT node. The binding can be found here: > > https://lore.kernel.org/all/20230328092645.634375-2-herve.codina@bootlin.com/ > > I'm surprised to see so much in the binding. I assume you are familiar > with DAHDI. It allows nearly everything to be configured at > runtime. The systems i've used allow you to select the clock > configuration, line build out, user side vs networks side signalling > CRC4 enables or not, etc. Well, I am not familiar with DAHDI at all. I didn't even know about the DAHDI project. The project seems to use specific kernel driver and I would like to avoid these external drivers. > > > Further more, the QMC HDLC is not the only PEF2256 consumer. > > The PEF2256 is also used for audio path (ie audio over E1) and so the > > configuration is shared between network and audio. The setting cannot be > > handle by the network part as the PEF2256 must be available and correctly > > configured even if the network part is not present. > > But there is no reason why the MFD could not provide a generic PHY to > actually configure the 'PHY'. The HDLC driver can then also use the > generic PHY. It would make your generic PHY less 'pointless'. I'm not > saying it has to be this way, but it is an option. If the pef2256 PHY provides a configure function, who is going to call this configure(). I mean the one calling the configure will be the configuration owner. None of the MFD child can own the configuration as this configuration will impact other children. So the MFD (top level node) owns the configuration. > > > > In fact, this PHY driver does not seem to do any configuration of any > > > sort on the framer. All it seems to be doing is take notification from > > > one chain and send them out another chain! > > > > Configuration is done by the parent MFD driver. > > The PHY driver has nothing more to do. > > > > > > > > I also wounder if this get_status() call is sufficient. Don't you also > > > want Red, Yellow and Blue alarms? It is not just the carrier is down, > > > but why it is down. > > > > I don't need them in my use case but if needed can't they be added later? > > Also, from the HDLC device point of view what can be done with these alarms? > > https://elixir.bootlin.com/linux/latest/source/Documentation/networking/ethtool-netlink.rst#L472 Thanks for pointing this interface. It is specific to ethtool but I can see the idea. The 'get_status' I proposed could be extended later to provide more information related to the colour of the alarm if needed. ethtool and related interfaces are very well fitted with Ethernet and related PHYs. I am not sure that ethtool will be usable for the pef2256. > > > Requests link state information. Link up/down flag (as provided by > > ``ETHTOOL_GLINK`` ioctl command) is provided. Optionally, extended > > state might be provided as well. In general, extended state > > describes reasons for why a port is down, or why it operates in some > > non-obvious mode. > > The colour of the Alarm gives you an idea which end of the system has > the problem. > > > > Overall, i don't see why you want a PHY. What value does it add? > > > > I need to detect carrier on/off according to the E1 link state. > > Why not just use the MFD notifier? What is the value of a PHY driver > translating one notifier into another? I translated to another notifier to keep the core (MFD) pef2256 API independent. But indeed this could be changed. If changed, the MFD pef2256 will have to handle the full struct phy_status_basic as the translation will not be there anymore. Right now, this structure is pretty simple and contains only the link state flag. But in the future, this PHY structure can move to something more complex and I am not sure that filling this struct is the MFD pef2256 responsibility. The PHY pef2256 is responsible for the correct structure contents not sure that this should be moved to the MFD part. > > And why is the notifier specific to the PEF2256? What would happen if > i used a analog devices DS2155, DS21Q55, and DS2156, or the IDT > 82P2281? Would each have its own notifier? And hence each would need > its own PHY which translates one notifier into another? Each of them should have their own notifier if they can notify. At least they will need their own notifier at they PHY driver level. Having or not a translation from something else would depend on each device PHY driver implementation. Also, maybe some PHY will not be able to provide notifications but only the get_status(). In this case, the notifier is not needed. The proposed QMC HDLC driver handles this case switching to polling mode if the PHY is not able to provide notification. > > There are enough E1/T1/J1 framers we should have a generic API between > the framer and the HDLC device. > I agree. I would like this first implementation without too much API restriction in order to see how it goes. The actual proposal imposes nothing on the PHY internal implementation. the pef2256 implementation chooses to have two notifiers (one at MFD level and one at PHY level) but it was not imposed by the API. Best regards, Hervé
> > I'm surprised to see so much in the binding. I assume you are familiar > > with DAHDI. It allows nearly everything to be configured at > > runtime. The systems i've used allow you to select the clock > > configuration, line build out, user side vs networks side signalling > > CRC4 enables or not, etc. > > Well, I am not familiar with DAHDI at all. > I didn't even know about the DAHDI project. > The project seems to use specific kernel driver and I would like to avoid > these external drivers. DAHDI is kind of the reference. Pretty much any Linux system being used as a open source PBX runs Asterisk, and has the out of tree DAHDI code to provide access to E1/T1 hardware and analogue POTS. I doubt it will ever be merged into mainline, but i think it gives a good idea what is needed to fully make use of such hardware. I don't know what you application is. Are you using libpri for signalling? How are you exposing the B channel to user space so that libpri can use it? > > > Further more, the QMC HDLC is not the only PEF2256 consumer. > > > The PEF2256 is also used for audio path (ie audio over E1) and so the > > > configuration is shared between network and audio. The setting cannot be > > > handle by the network part as the PEF2256 must be available and correctly > > > configured even if the network part is not present. > > > > But there is no reason why the MFD could not provide a generic PHY to > > actually configure the 'PHY'. The HDLC driver can then also use the > > generic PHY. It would make your generic PHY less 'pointless'. I'm not > > saying it has to be this way, but it is an option. > > If the pef2256 PHY provides a configure function, who is going to call this > configure(). I mean the one calling the configure will be the configuration > owner. None of the MFD child can own the configuration as this configuration > will impact other children. So the MFD (top level node) owns the configuration. Fine. Nothing unusual there. The netdev owns the configuration for an Ethernet device. The MAC driver passes a subset down to any generic PHY being used to implement a SERDES. You could have the same architecture here. The MFD implements a standardised netlink API for configuring E1/T1/J1 devices. Part of it gets passed to the framer, which could be part of a generic PHY. I assume you also need to configure the HDLC hardware. It needs to know if it is using the whole E1 channel in unframed mode, or it should do a fractional link, using a subset of slots, or is just using one slot for 64Kbps, which would be an ISDN B channel. > > > > In fact, this PHY driver does not seem to do any configuration of any > > > > sort on the framer. All it seems to be doing is take notification from > > > > one chain and send them out another chain! > > > > > > Configuration is done by the parent MFD driver. > > > The PHY driver has nothing more to do. > > > > > > > > > > > I also wounder if this get_status() call is sufficient. Don't you also > > > > want Red, Yellow and Blue alarms? It is not just the carrier is down, > > > > but why it is down. > > > > > > I don't need them in my use case but if needed can't they be added later? > > > Also, from the HDLC device point of view what can be done with these alarms? > > > > https://elixir.bootlin.com/linux/latest/source/Documentation/networking/ethtool-netlink.rst#L472 > > Thanks for pointing this interface. > It is specific to ethtool but I can see the idea. Don't equate ethtool with Ethernet. Any netdev can implement it, and a HDLC device is a netdev. So it could well return link down reason. > But indeed this could be changed. > If changed, the MFD pef2256 will have to handle the full struct > phy_status_basic as the translation will not be there anymore. > Right now, this structure is pretty simple and contains only the link state > flag. But in the future, this PHY structure can move to something more > complex and I am not sure that filling this struct is the MFD pef2256 > responsibility. The PHY pef2256 is responsible for the correct structure > contents not sure that this should be moved to the MFD part. Framers, like Ethernet PHYs, are reasonably well defined, because there are clear standards to follow. You could put the datasheets for the various frames side by side and quickly get an idea of the common properties. So you could define a structure now. In order to make it extendable, just avoid 0 having any meaning other than UNKNOWN. If you look at ethernet, SPEED_UNKNOWN is 0, DUPLEX_UNKNOWN is 0. So if a new field is added, we know if a driver has not filled it in. > > And why is the notifier specific to the PEF2256? What would happen if > > i used a analog devices DS2155, DS21Q55, and DS2156, or the IDT > > 82P2281? Would each have its own notifier? And hence each would need > > its own PHY which translates one notifier into another? > > Each of them should have their own notifier if they can notify. I doubt you will find a framer which cannot report lost of framing. It is too much a part of the standards. There are signalling actions you need to do when a link goes does. So all framer drivers will have a notifier. > At least they will need their own notifier at they PHY driver level. > Having or not a translation from something else would depend on each device > PHY driver implementation. Have you ever look at Ethernet PHYs? They all export the same API. You can plug any Linux MAC driver into any Linux Ethernet PHY driver. It should be the same here. You should be able to plug any HDLC driver into any Framer. I should be able to take the same SoC you have implementing the TDM interface, and plug it into the IDT framer i know of. We want a standardised API between the HDLC device and the framer. > I would like this first implementation without too much API restriction > in order to see how it goes. > The actual proposal imposes nothing on the PHY internal implementation. > the pef2256 implementation chooses to have two notifiers (one at MFD > level and one at PHY level) but it was not imposed by the API. What i would like to see is some indication the API you are proposing is generic, and could be implemented by multiple frames and HDLC devices. The interface between the HDLC driver and the framer should be generic. The HDLC driver has an abstract reference to a framer. The framer has a reference to a netdev for the HDLC device. You can keep this API very simple, just have link up/down notification, since that is all you want at the moment. But the implementation should give hints how it can be extended. Andrew
On Mon, 17 Apr 2023 15:12:14 +0200 Andrew Lunn <andrew@lunn.ch> wrote: > > > I'm surprised to see so much in the binding. I assume you are familiar > > > with DAHDI. It allows nearly everything to be configured at > > > runtime. The systems i've used allow you to select the clock > > > configuration, line build out, user side vs networks side signalling > > > CRC4 enables or not, etc. > > > > Well, I am not familiar with DAHDI at all. > > I didn't even know about the DAHDI project. > > The project seems to use specific kernel driver and I would like to avoid > > these external drivers. > > DAHDI is kind of the reference. Pretty much any Linux system being > used as a open source PBX runs Asterisk, and has the out of tree DAHDI > code to provide access to E1/T1 hardware and analogue POTS. I doubt it > will ever be merged into mainline, but i think it gives a good idea > what is needed to fully make use of such hardware. > > I don't know what you application is. Are you using libpri for > signalling? How are you exposing the B channel to user space so that > libpri can use it? My application (probably a subset of what we can do with E1 lines) does not use signaling. I don't expose any channel to the user space but just: - One hdlc interface using one timeslot. - One or more dai links (audio channels) that can be used using ALSA library. > > > > > Further more, the QMC HDLC is not the only PEF2256 consumer. > > > > The PEF2256 is also used for audio path (ie audio over E1) and so the > > > > configuration is shared between network and audio. The setting cannot be > > > > handle by the network part as the PEF2256 must be available and correctly > > > > configured even if the network part is not present. > > > > > > But there is no reason why the MFD could not provide a generic PHY to > > > actually configure the 'PHY'. The HDLC driver can then also use the > > > generic PHY. It would make your generic PHY less 'pointless'. I'm not > > > saying it has to be this way, but it is an option. > > > > If the pef2256 PHY provides a configure function, who is going to call this > > configure(). I mean the one calling the configure will be the configuration > > owner. None of the MFD child can own the configuration as this configuration > > will impact other children. So the MFD (top level node) owns the configuration. > > Fine. Nothing unusual there. The netdev owns the configuration for an > Ethernet device. The MAC driver passes a subset down to any generic > PHY being used to implement a SERDES. > > You could have the same architecture here. The MFD implements a > standardised netlink API for configuring E1/T1/J1 devices. Part of it > gets passed to the framer, which could be part of a generic PHY. I > assume you also need to configure the HDLC hardware. It needs to know > if it is using the whole E1 channel in unframed mode, or it should do > a fractional link, using a subset of slots, or is just using one slot > for 64Kbps, which would be an ISDN B channel. The HDLC driver uses a QMC channel and the DT binding related to this QMC channel defines the timeslots used by this channel. From the QMC HDLC nothing is configured. This is done at the QMC level. https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/soc/fsl/cpm_qe/fsl,cpm1-scc-qmc.yaml#n91 > > > > > > In fact, this PHY driver does not seem to do any configuration of any > > > > > sort on the framer. All it seems to be doing is take notification from > > > > > one chain and send them out another chain! > > > > > > > > Configuration is done by the parent MFD driver. > > > > The PHY driver has nothing more to do. > > > > > > > > > > > > > > I also wounder if this get_status() call is sufficient. Don't you also > > > > > want Red, Yellow and Blue alarms? It is not just the carrier is down, > > > > > but why it is down. > > > > > > > > I don't need them in my use case but if needed can't they be added later? > > > > Also, from the HDLC device point of view what can be done with these alarms? > > > > > > https://elixir.bootlin.com/linux/latest/source/Documentation/networking/ethtool-netlink.rst#L472 > > > > Thanks for pointing this interface. > > It is specific to ethtool but I can see the idea. > > Don't equate ethtool with Ethernet. Any netdev can implement it, and a > HDLC device is a netdev. So it could well return link down reason. Ok. So the link down reason should be returned by the newly introduced QMC HDLC. Adding this in the generic PHY infrastructure I used (drivers/phy) is adding some specific netdev stuff in this subsystem. Don't forget that the pef2256 can be used without any network part by the audio subsystem in order to have the user audio data sent to E1 using pure ALSA path. > > > But indeed this could be changed. > > If changed, the MFD pef2256 will have to handle the full struct > > phy_status_basic as the translation will not be there anymore. > > Right now, this structure is pretty simple and contains only the link state > > flag. But in the future, this PHY structure can move to something more > > complex and I am not sure that filling this struct is the MFD pef2256 > > responsibility. The PHY pef2256 is responsible for the correct structure > > contents not sure that this should be moved to the MFD part. > > Framers, like Ethernet PHYs, are reasonably well defined, because > there are clear standards to follow. You could put the datasheets for > the various frames side by side and quickly get an idea of the common > properties. So you could define a structure now. In order to make it > extendable, just avoid 0 having any meaning other than UNKNOWN. If you > look at ethernet, SPEED_UNKNOWN is 0, DUPLEX_UNKNOWN is 0. So if a new > field is added, we know if a driver has not filled it in. > > > > And why is the notifier specific to the PEF2256? What would happen if > > > i used a analog devices DS2155, DS21Q55, and DS2156, or the IDT > > > 82P2281? Would each have its own notifier? And hence each would need > > > its own PHY which translates one notifier into another? > > > > Each of them should have their own notifier if they can notify. > > I doubt you will find a framer which cannot report lost of framing. It > is too much a part of the standards. There are signalling actions you > need to do when a link goes does. So all framer drivers will have a > notifier. > > > At least they will need their own notifier at they PHY driver level. > > Having or not a translation from something else would depend on each device > > PHY driver implementation. > > Have you ever look at Ethernet PHYs? They all export the same API. You > can plug any Linux MAC driver into any Linux Ethernet PHY driver. It > should be the same here. You should be able to plug any HDLC driver > into any Framer. I should be able to take the same SoC you have > implementing the TDM interface, and plug it into the IDT framer i know > of. We want a standardised API between the HDLC device and the framer. Can you tell me more. I thought I was providing a "standardised" API between the HDLC device and the framer. Maybe it was not as complete as you could expect but I had the feeling that it was standardised. Right now it is just a link state notification provided by a simple 'basic PHY'. Any HDLC device that uses a 'basic PHY' can be notified of any changes of this link state provided by the PHY without knowing what is the exact PHY behind this 'basic phy'. > > > I would like this first implementation without too much API restriction > > in order to see how it goes. > > The actual proposal imposes nothing on the PHY internal implementation. > > the pef2256 implementation chooses to have two notifiers (one at MFD > > level and one at PHY level) but it was not imposed by the API. > > What i would like to see is some indication the API you are proposing > is generic, and could be implemented by multiple frames and HDLC > devices. The interface between the HDLC driver and the framer should > be generic. The HDLC driver has an abstract reference to a framer. The > framer has a reference to a netdev for the HDLC device. Here, I do not fully understand. Why does the framer need a reference to the netdev ? > > You can keep this API very simple, just have link up/down > notification, since that is all you want at the moment. But the > implementation should give hints how it can be extended. > Best regards, Hervé
> My application (probably a subset of what we can do with E1 lines) does > not use signaling. > > I don't expose any channel to the user space but just: > - One hdlc interface using one timeslot. > - One or more dai links (audio channels) that can be used using ALSA library. Well, that obviously does what you need for you application, but it does not sound very generic. I don't think you could build a PBX on top of that. I'd think you can build an E1 trunk to VoIP gateway. So i also have to wounder how generic vs one specific use case your building blocks are. Looking at the binding, i assume you could in fact instantiate 32 hdlc instances? And that 'transparent' actually means DAI? So you can instantiate as many DAI as you want. If you wanted to implement signalling, a third type could be added? > I thought I was providing a "standardised" API between the HDLC device > and the framer. Maybe it was not as complete as you could expect but I had > the feeling that it was standardised. You are abusing the Generic PHY interface, for something which is not a PHY. Your PHY driver does nothing a PHY driver normally does, because you keep arguing the MFD does it all. So to me this is the wrong abstraction. You need an abstraction of a framer. You do however want a similar API to generic PHY. devm_of_framer_optional_get() for the consumer of a framer, which looks in DT for a phandle to a device. devm_framer_create() which registers a framer provider, your MFD, with the framer core. The hdlc driver can then get an instance of the framer, and either put a notifier call block on its chain, or register a function to be called when there is change in status. What i also don't like is you have a very static configuration. You are putting configuration into DT, which i'm surprised Rob Herring and Krzysztof Kozlowski accepted. How you use E1 slots is not a hardware property, it is purely software configuration, so should not be in DT. So there should be a mechanism to configure how slots are used. You can then dynamically instantiate HDLC interfaces and DAI links. This is something which should be in the framer core. But since you have managed to get this binding accepted, you can skip this. But implementing the basic framer abstraction will give a place holder for somebody to implement a much more generic solution in the future. Andrew
Hi Andrew, On Mon, 17 Apr 2023 19:22:26 +0200 Andrew Lunn <andrew@lunn.ch> wrote: > > My application (probably a subset of what we can do with E1 lines) does > > not use signaling. > > > > I don't expose any channel to the user space but just: > > - One hdlc interface using one timeslot. > > - One or more dai links (audio channels) that can be used using ALSA library. > > Well, that obviously does what you need for you application, but it > does not sound very generic. I don't think you could build a PBX on > top of that. I'd think you can build an E1 trunk to VoIP gateway. So > i also have to wounder how generic vs one specific use case your > building blocks are. > > Looking at the binding, i assume you could in fact instantiate 32 hdlc > instances? And that 'transparent' actually means DAI? So you can > instantiate as many DAI as you want. If you wanted to implement > signalling, a third type could be added? > > > I thought I was providing a "standardised" API between the HDLC device > > and the framer. Maybe it was not as complete as you could expect but I had > > the feeling that it was standardised. > > You are abusing the Generic PHY interface, for something which is not > a PHY. Your PHY driver does nothing a PHY driver normally does, > because you keep arguing the MFD does it all. So to me this is the > wrong abstraction. > > You need an abstraction of a framer. You do however want a similar API > to generic PHY. devm_of_framer_optional_get() for the consumer of a > framer, which looks in DT for a phandle to a device. > devm_framer_create() which registers a framer provider, your MFD, with > the framer core. The hdlc driver can then get an instance of the > framer, and either put a notifier call block on its chain, or register > a function to be called when there is change in status. > > What i also don't like is you have a very static configuration. You > are putting configuration into DT, which i'm surprised Rob Herring and > Krzysztof Kozlowski accepted. How you use E1 slots is not a hardware > property, it is purely software configuration, so should not be in DT. > So there should be a mechanism to configure how slots are used. You > can then dynamically instantiate HDLC interfaces and DAI links. This > is something which should be in the framer core. But since you have > managed to get this binding accepted, you can skip this. But > implementing the basic framer abstraction will give a place holder for > somebody to implement a much more generic solution in the future. > > Andrew I can move to a basic framer abstraction as you suggested. At least: - devm_of_framer_optional_get() - devm_framer_create() - framer_notifier_register() or something similar. Where do you expect to see this framer abstraction and the pef2256 framer part ? driver/net/wan/framer/, other place ? I think driver/net/wan/framer/ can be a good place to start as only HDLC will use this abstraction. I can use the framer abstraction from the QMC HDLC driver itself or try to move it to the HDLC core. Do you think it will be interesting to have it move to the HDLC core ? Best regards, Hervé
> I can move to a basic framer abstraction as you suggested. At least: > - devm_of_framer_optional_get() > - devm_framer_create() > - framer_notifier_register() or something similar. > > Where do you expect to see this framer abstraction and the pef2256 > framer part ? > driver/net/wan/framer/, other place ? That seems like a good location. > I think driver/net/wan/framer/ can be a good place to start as only HDLC > will use this abstraction. > I can use the framer abstraction from the QMC HDLC driver itself or try > to move it to the HDLC core. Do you think it will be interesting to have > it move to the HDLC core ? Having it in the core would be nice. But i don't know that code, so i cannot say how easy/hard it will be do to. hdlc.c already seems to have some code for carrier. So you need to be careful not to break that. Andrew
Hi, I have a system where I need to handle an HDLC interface. The HDLC data are transferred using a TDM bus on which a PEF2256 is present. The PEF2256 transfers data from/to the TDM bus to/from E1 line. This PEF2256 is also connected to a PowerQUICC SoC for the control path and the TDM is connected to the SoC (QMC component) for the data path. From the HDLC driver, I need to handle data using the QMC and carrier detection using the PEF2256 (E1 line carrier). The HDLC driver consider the PEF2256 as a generic PHY. So, the design is the following: +----------+ +-------------+ +---------+ | HDLC drv | <-data-> | QMC channel | <-- TDM --> | PEF2256 | +----------+ +-------------+ | | <--> E1 ^ +---------+ +---------+ | | +-> | Gen PHY | <-> | PEF2256 | <- local bus -> | | +---------+ | PHY drv | +---------+ +---------+ In order to implement this, I had to: 1 - Extend the generic PHY API to support get_status() and notification on status change. 2 - Introduce a new kind of generic PHY named "basic phy". This PHY familly can provide a link status in the get_status() data. 3 - Support the PEF2256 PHY as a "basic phy" The purpose of this RFC series is to discuss this design. The QMC driver code is available on linux-next. In this series: - patch 1: driver HDLC using the QMC channel - patch 2: Extend the generic PHY API - patch 3: Use the "basic phy" in the HDLC driver - patch 4: Implement the PEF2256 PHY driver I did 2 patches for the HDLC driver in order to point the new PHY family usage in the HDLC driver. In the end, these two patches will be squashed and the bindings will be added. Hope to have some feedback on this proposal. Best regards, Hervé Herve Codina (4): net: wan: Add support for QMC HDLC phy: Extend API to support 'status' get and notification net: wan: fsl_qmc_hdlc: Add PHY support phy: lantiq: Add PEF2256 PHY support drivers/net/wan/Kconfig | 12 + drivers/net/wan/Makefile | 1 + drivers/net/wan/fsl_qmc_hdlc.c | 558 ++++++++++++++++++++++++ drivers/phy/lantiq/Kconfig | 15 + drivers/phy/lantiq/Makefile | 1 + drivers/phy/lantiq/phy-lantiq-pef2256.c | 131 ++++++ drivers/phy/phy-core.c | 88 ++++ include/linux/phy/phy-basic.h | 27 ++ include/linux/phy/phy.h | 89 +++- 9 files changed, 921 insertions(+), 1 deletion(-) create mode 100644 drivers/net/wan/fsl_qmc_hdlc.c create mode 100644 drivers/phy/lantiq/phy-lantiq-pef2256.c create mode 100644 include/linux/phy/phy-basic.h