Message-ID: <Y0ZiQbqQ+DsHinOf@nanopsycho>
Date:   Wed, 12 Oct 2022 08:44:17 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Vadim Fedorenko <vfedorenko@...ek.ru>
Cc:     Jakub Kicinski <kuba@...nel.org>,
        Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>,
        netdev@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-clk@...r.kernel.org, Vadim Fedorenko <vadfed@...com>
Subject: Re: [RFC PATCH v3 1/6] dpll: Add DPLL framework base functions

Tue, Oct 11, 2022 at 11:31:25PM CEST, vfedorenko@...ek.ru wrote:
>On 11.10.2022 09:47, Jiri Pirko wrote:
>> Tue, Oct 11, 2022 at 10:32:22AM CEST, jiri@...nulli.us wrote:
>> > Mon, Oct 10, 2022 at 09:54:26PM CEST, vfedorenko@...ek.ru wrote:
>> > > On 10.10.2022 10:18, Jiri Pirko wrote:
>> > > > Mon, Oct 10, 2022 at 03:17:59AM CEST, vfedorenko@...ek.ru wrote:
>> > > > > From: Vadim Fedorenko <vadfed@...com>
>> 
>> [...]
>> 
>> 
>> > > I see your point. We do have hardware which allows changing type of SMA
>> > > connector, and even the direction, each SMA could be used as input/source or
>> > > output of different signals. But there are limitation, like not all SMAs can
>> > > produce IRIG-B signal or only some of them can be used to get GNSS 1PPS. The
>> > 
>> > Okay, so that is not the *type* of source, but rather attribute of it.
>> > Example:
>> > 
>> > $ dpll X show
>> > index 0
>> >   type EXT
>> >   signal 1PPS
>> >   supported_signals
>> >      1PPS 10MHz
>> > 
>> > $ dpll X set source index 1 signal_type 10MHz
>> > $ dpll X show
>> > index 0
>> >   type EXT
>> >   signal 10MHz
>> >   supported_signals
>> >      1PPS 10MHz
>> > 
>> > So one source with index 0 of type "EXT" (could be "SMA", does not
>> > matter) supports 1 signal types.
>> > 
>> > 
>> > Thinking about this more and to cover the case when one SMA could be
>> > potencially used for input and output. It already occured to me that
>> > source/output are quite similar, have similar/same attributes. What if
>> > they are merged together to say a "pin" object only with extra
>> > PERSONALITY attribute?
>> > 
>> > Example:
>> > 
>> > -> DPLL_CMD_PIN_GET - dump
>> >       ATTR_DEVICE_ID X
>> > 
>> > <- DPLL_CMD_PIN_GET
>> > 
>> >        ATTR_DEVICE_ID X
>> >        ATTR_PIN_INDEX 0
>> >        ATTR_PIN_TYPE EXT
>> >        ATTR_PIN_SIGNAL 1PPS   (selected signal)
>> >        ATTR_PIN_SUPPORTED_SIGNALS (nest)
>> >          ATTR_PIN_SIGNAL 1PPS
>> >          ATTR_PIN_SIGNAL 10MHZ
>> >        ATTR_PIN_PERSONALITY OUTPUT   (selected personality)
>> >        ATTR_PIN_SUPPORTED_PERSONALITIES (nest)
>> >          ATTR_PIN_PERSONALITY DISCONNECTED
>> >          ATTR_PIN_PERSONALITY INPUT
>> >          ATTR_PIN_PERSONALITY OUTPUT     (note this supports both input and
>> > 					  output)
>> > 
>> >        ATTR_DEVICE_ID X
>> >        ATTR_PIN_INDEX 1
>> >        ATTR_PIN_TYPE EXT
>> >        ATTR_PIN_SIGNAL 10MHz   (selected signal)
>> >        ATTR_PIN_SUPPORTED_SIGNALS (nest)
>> >          ATTR_PIN_SIGNAL 1PPS
>> >          ATTR_PIN_SIGNAL 10MHZ
>> >        ATTR_PIN_PERSONALITY DISCONNECTED   (selected personality - not
>> > 					    connected currently)
>> >        ATTR_PIN_SUPPORTED_PERSONALITIES (nest)
>> >          ATTR_PIN_PERSONALITY DISCONNECTED
>> >          ATTR_PIN_PERSONALITY INPUT      (note this supports only input)
>> > 
>> >        ATTR_DEVICE_ID X
>> >        ATTR_PIN_INDEX 2
>> >        ATTR_PIN_TYPE GNSS
>> >        ATTR_PIN_SIGNAL 1PPS   (selected signal)
>> >        ATTR_PIN_SUPPORTED_SIGNALS (nest)
>> >          ATTR_PIN_SIGNAL 1PPS
>> >        ATTR_PIN_PERSONALITY INPUT   (selected personality - note this is
>> > 				     now he selected source, being only
>> > 				     pin with INPUT personality)
>> >        ATTR_PIN_SUPPORTED_PERSONALITIES (nest)
>> >          ATTR_PIN_PERSONALITY DISCONNECTED
>> >          ATTR_PIN_PERSONALITY INPUT      (note this supports only input)
>> > 
>> >        ATTR_DEVICE_ID X
>> >        ATTR_PIN_INDEX 3
>> >        ATTR_PIN_TYPE SYNCE_ETH_PORT
>> >        ATTR_PIN_NETDEV_IFINDEX 20
>> >        ATTR_PIN_PERSONALITY OUTPUT   (selected personality)
>> >        ATTR_PIN_SUPPORTED_PERSONALITIES (nest)
>> >          ATTR_PIN_PERSONALITY DISCONNECTED
>> >          ATTR_PIN_PERSONALITY INPUT
>> >          ATTR_PIN_PERSONALITY OUTPUT     (note this supports both input and
>> > 					  output)
>> > 
>> >        ATTR_DEVICE_ID X
>> >        ATTR_PIN_INDEX 4
>> >        ATTR_PIN_TYPE SYNCE_ETH_PORT
>> >        ATTR_PIN_NETDEV_IFINDEX 30
>> >        ATTR_PIN_PERSONALITY OUTPUT   (selected personality)
>> >        ATTR_PIN_SUPPORTED_PERSONALITIES (nest)
>> >          ATTR_PIN_PERSONALITY DISCONNECTED
>> >          ATTR_PIN_PERSONALITY INPUT
>> >          ATTR_PIN_PERSONALITY OUTPUT     (note this supports both input and
>> > 					  output)
>> > 
>> > 
>> > This allows the user to actually see the full picture:
>> > 1) all input/output pins in a single list, no duplicates
>> > 2) each pin if of certain type (ATTR_PIN_TYPE) EXT/GNSS/SYNCE_ETH_PORT
>> > 3) the pins that can change signal type contain the selected and list of
>> >    supported signal types (ATTR_PIN_SIGNAL, ATTR_PIN_SUPPORTED_SIGNALS)
>> > 4) direction/connection of the pin to the DPLL is exposed over
>> >    ATTR_PIN_PERSONALITY. For each pin, the driver would expose it can
>> >    act as INPUT/OUTPUT and even more, it can indicate the pin can
>> >    disconnect from DPLL entirely (if possible).
>> > 5) user can select the source by setting ATTR_PIN_PERSONALITY of certain
>> >    pin to INPUT. Only one pin could be set to INPUT and that is the
>> >    souce of DPLL.
>> >    In case no pin have personality set to INPUT, the DPLL is
>> >    free-running.
>> 
>> Okay, thinking about it some more, I would leave the source select
>> indepentent from the ATTR_PIN_PERSONALITY and selectable using device
>> set cmd and separate attribute. It works better even more when consider
>> autoselect mode.
>> 
>> Well of course only pins with ATTR_PIN_PERSONALITY INPUT could be
>> selected as source.
>> 
>
>Overall, I agree with this proposal, and as I've already said, the work is
>going exactly the same way - to introduce pin object with separate set of
>attributes.
>I don't really like 'PERSONALITY' naming, I think we have to find a better
>name. It looks like DIRECTION is slightly better. And the
>CONNECTED/DISCONNECTED should be different attribute. And we also need

Yeah, it is a matter of implementation. I just thought that this is
possible to be done in a single attribute, because when the pin is
disconnected, the direction has no meaning.


>attribute PRIORITY to be able to configure (or hint) auto-select mode. There

Sure, I didn't put the PRIORITY attribute to the example, but I believe
it is very straightforward extension to add it.


>are also special objects called muxes, which consist of several inputs and
>one output, but they cannot synthonise signal, only pass one of the inputs to
>output. We are still in kind of discussion whether to have them as separate
>objects, or extend the amount of pins of DPLL device in this case. The
>problem again in the auto-select mode and priorities. It would be great to
>hear your thoughts about such objects.

Does the mux have any attribute/configuration valuable for the user.
If yes, it might make sense to have it as a separate object. Then we
can have it as a tree of object of type MUX with leaves of PIN.

The question really is, if the user needs to know about muxes and work
with them, or they can be abstracted out by the driver.

Could you elaborate a bit more why auto-select mode and priorities are
problematic with muxes in picture?


>
>> > 
>> > This would introduce quite nice flexibility, exposes source/output
>> > capabilities and provides good visilibity of current configuration.
>> > 
>> > 
>> > > interface was created to cover such case. I believe we have to improve it to
>> > > cover SyncE configuration better, but I personally don't have SyncE hardware
>> > > ready to test and that's why I have to rely on suggestions from yours or
>> > > Arkadiusz's experience. From what I can see now there is need for special
>> > > attribute to link source to net device, and I'm happy to add it. In case of
>> > > fixed configuration of sources, the device should provide only one type as
>> > > supported and that's it.
>> > > 
>> 
>> [...]
>

Powered by blists - more mailing lists