Message-ID: <20230417173941.0206f696@bootlin.com>
Date:   Mon, 17 Apr 2023 17:39:41 +0200
From:   Herve Codina <herve.codina@...tlin.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, Vinod Koul <vkoul@...nel.org>,
        Kishon Vijay Abraham I <kishon@...nel.org>,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        linux-phy@...ts.infradead.org,
        Christophe Leroy <christophe.leroy@...roup.eu>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [RFC PATCH 0/4] Add support for QMC HDLC and PHY

On Mon, 17 Apr 2023 15:12:14 +0200
Andrew Lunn <andrew@...n.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é

-- 
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Powered by blists - more mailing lists