Message-ID: <9a3608cf-21bb-18b1-796a-7325a613b641@novek.ru>
Date:   Wed, 12 Oct 2022 21:12:43 +0100
From:   Vadim Fedorenko <vfedorenko@...ek.ru>
To:     Jiri Pirko <jiri@...nulli.us>
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

On 12.10.2022 07:44, Jiri Pirko wrote:
> 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.

Yes, that was the first thought, but implementation details are not great for now.

> 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.

Both ways will work I believe. It's more like our will to give users full 
picture or hide it somehow with the implementation.
> 
> Could you elaborate a bit more why auto-select mode and priorities are
> problematic with muxes in picture?

AFAIU, some mux devices are not smart enough to make a decision suitable for 
autoselect for the pins they have. In this case the autoselect process is done 
in the DPLL device, which selects mux and not the pin directly. At the same time 
there could be muxes that are smart enough to make a decision, and it will be 
autoselect on top of autoselect (and several more layers) and it doesn't sound 
great to me. I believe Arkadiusz will explain the mux a bit better.

> 
> 
>>
>>>>
>>>> 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