linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: Jiri Pirko <jiri@resnulli.us>
To: Vadim Fedorenko <vfedorenko@novek.ru>
Cc: Jakub Kicinski <kuba@kernel.org>,
	Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>,
	netdev@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-clk@vger.kernel.org, Vadim Fedorenko <vadfed@fb.com>
Subject: Re: [RFC PATCH v3 1/6] dpll: Add DPLL framework base functions
Date: Thu, 13 Oct 2022 08:55:34 +0200	[thread overview]
Message-ID: <Y0e2Zn4pbhPnKGQJ@nanopsycho> (raw)
In-Reply-To: <9a3608cf-21bb-18b1-796a-7325a613b641@novek.ru>

Wed, Oct 12, 2022 at 10:12:43PM CEST, vfedorenko@novek.ru wrote:
>On 12.10.2022 07:44, Jiri Pirko wrote:
>> Tue, Oct 11, 2022 at 11:31:25PM CEST, vfedorenko@novek.ru wrote:
>> > On 11.10.2022 09:47, Jiri Pirko wrote:
>> > > Tue, Oct 11, 2022 at 10:32:22AM CEST, jiri@resnulli.us wrote:
>> > > > Mon, Oct 10, 2022 at 09:54:26PM CEST, vfedorenko@novek.ru wrote:
>> > > > > On 10.10.2022 10:18, Jiri Pirko wrote:
>> > > > > > Mon, Oct 10, 2022 at 03:17:59AM CEST, vfedorenko@novek.ru wrote:
>> > > > > > > From: Vadim Fedorenko <vadfed@fb.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.


From what you write in this reply, I have a feeling that these details
are not really interesting for user to see. So I tend to lean forward to
abstract this out and leave the details to HW/FW/driver.


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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2022-10-13  6:57 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-10-10  1:17 [RFC PATCH v3 0/5] Create common DPLL/clock configuration API Vadim Fedorenko
2022-10-10  1:17 ` [RFC PATCH v3 1/6] dpll: Add DPLL framework base functions Vadim Fedorenko
2022-10-10  9:18   ` Jiri Pirko
2022-10-10 19:54     ` Vadim Fedorenko
2022-10-11  8:32       ` Jiri Pirko
2022-10-11  8:47         ` Jiri Pirko
2022-10-11 21:31           ` Vadim Fedorenko
2022-10-12  6:44             ` Jiri Pirko
2022-10-12 20:12               ` Vadim Fedorenko
2022-10-13  6:55                 ` Jiri Pirko [this message]
2022-10-13 15:17                   ` Jakub Kicinski
2022-10-14  9:52                     ` Jiri Pirko
2022-10-11 21:23         ` Vadim Fedorenko
2022-10-12 10:44           ` Jiri Pirko
2022-10-12 10:47   ` Jiri Pirko
2022-10-12 20:15     ` Vadim Fedorenko
2022-10-13  6:56       ` Jiri Pirko
2022-10-12 16:51   ` Jiri Pirko
2022-10-12 20:17     ` Vadim Fedorenko
2022-10-10  1:18 ` [RFC PATCH v3 2/6] dpll: add netlink events Vadim Fedorenko
2022-10-10  1:18 ` [RFC PATCH v3 3/6] dpll: add support for source selection modes Vadim Fedorenko
2022-10-10 14:13   ` Jiri Pirko
2022-10-10 20:03     ` Vadim Fedorenko
2022-10-10  1:18 ` [RFC PATCH v3 4/6] dpll: get source/output name Vadim Fedorenko
2022-10-10  9:45   ` Jiri Pirko
2022-10-10 19:55     ` Vadim Fedorenko
2022-10-11  7:28       ` Jiri Pirko
2022-10-10  1:18 ` [RFC PATCH v3 5/6] dpll: documentation on DPLL subsystem interface Vadim Fedorenko
2022-10-10  1:18 ` [RFC PATCH v3 6/6] ptp_ocp: implement DPLL ops Vadim Fedorenko
2022-10-10 15:42   ` Jiri Pirko
2022-10-10 20:14     ` Vadim Fedorenko
2022-10-11  7:30       ` Jiri Pirko
2022-10-12 15:14   ` Jiri Pirko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Y0e2Zn4pbhPnKGQJ@nanopsycho \
    --to=jiri@resnulli.us \
    --cc=arkadiusz.kubalewski@intel.com \
    --cc=kuba@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=vadfed@fb.com \
    --cc=vfedorenko@novek.ru \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).