[RFC PATCH v4 0/4] Create common DPLL/clock configuration API
Kubalewski, Arkadiusz
arkadiusz.kubalewski at intel.com
Wed Jan 11 08:27:20 PST 2023
BR, Arkadiusz
>-----Original Message-----
>From: Maciek Machnikowski <maciek at machnikowski.net>
>Sent: Wednesday, January 11, 2023 4:54 PM
>To: Kubalewski, Arkadiusz <arkadiusz.kubalewski at intel.com>; Jiri Pirko
><jiri at resnulli.us>
>Cc: Jakub Kicinski <kuba at kernel.org>; 'Vadim Fedorenko'
><vfedorenko at novek.ru>; 'Jonathan Lemon' <jonathan.lemon at gmail.com>; 'Paolo
>Abeni' <pabeni at redhat.com>; netdev at vger.kernel.org; linux-arm-
>kernel at lists.infradead.org; linux-clk at vger.kernel.org
>Subject: Re: [RFC PATCH v4 0/4] Create common DPLL/clock configuration API
>
>
>
>On 1/11/2023 4:30 PM, Kubalewski, Arkadiusz wrote:
>>> From: Maciek Machnikowski <maciek at machnikowski.net>
>>> Sent: Wednesday, January 11, 2023 3:40 PM
>>> To: Kubalewski, Arkadiusz <arkadiusz.kubalewski at intel.com>; Jiri Pirko
>>> <jiri at resnulli.us>
>>>
>>>
>>> On 1/11/2023 3:17 PM, Kubalewski, Arkadiusz wrote:
>>>>> From: Maciek Machnikowski <maciek at machnikowski.net>
>>>>> Sent: Tuesday, January 10, 2023 3:59 PM
>>>>> To: Kubalewski, Arkadiusz <arkadiusz.kubalewski at intel.com>; Jiri Pirko
>>>>> <jiri at resnulli.us>
>>>>>
>>>>> On 1/10/2023 11:54 AM, Kubalewski, Arkadiusz wrote:
>>>>>>> From: Jiri Pirko <jiri at resnulli.us>
>>>>>>> Sent: Monday, January 9, 2023 5:30 PM
>>>>>>>>
>>>>>>>> Hi guys,
>>>>>>>>
>>>>>>>> We have been trying to figure out feasibility of new approach
>>> proposed
>>>>> on
>>>>>>> our
>>>>>>>> latest meeting - to have a single object which encapsulates
>multiple
>>>>>>> DPLLs.
>>>>>>>>
>>>>>>>> Please consider following example:
>>>>>>>>
>>>>>>>> Shared common inputs:
>>>>>>>> i0 - GPS / external
>>>>>>>> i1 - SMA1 / external
>>>>>>>> i2 - SMA2 / external
>>>>>>>> i3 - MUX0 / clk recovered from PHY0.X driven by MAC0
>>>>>>>> i4 - MUX1 / clk recovered from PHY1.X driven by MAC1
>>>>>>>>
>>>>>>>> +---------------------------------------------------------+
>>>>>>>> | Channel A / FW0 +---+ |
>>>>>>>> | i0--| | |
>>>>>>>> | +---+ | | |
>>>>>>>> | PHY0.0--| | i1--| D | |
>>>>>>>> | | | | P | |
>>>>>>>> | PHY0.1--| M | i2--| L | +---+ +--------+ |
>>>>>>>> | | U | | L |---| |---| PHY0.0 |--|
>>>>>>>> | PHY0.2--| X |-+---------i3--| 0 | | | +--------+ |
>>>>>>>> | | 0 | |+------+ | |---| M |---| PHY0.1 |--|
>>>>>>>> | ... --| | || MUX1 |-i4--| | | A | +--------+ |
>>>>>>>> | | | |+------+ +---+ | C |---| PHY0.2 |--|
>>>>>>>> | PHY0.7--| | | i0--| | | 0 | +--------+ |
>>>>>>>> | +---+ | | |---| |---| ... |--|
>>>>>>>> | | i1--| D | | | +--------+ |
>>>>>>>> | | | P |---| |---| PHY0.7 |--|
>>>>>>>> | | i2--| L | +---+ +--------+ |
>>>>>>>> | | | L | |
>>>>>>>> | \---------i3--| 1 | |
>>>>>>>> | +------+ | | |
>>>>>>>> | | MUX1 |-i4--| | |
>>>>>>>> | +------+ +---+ |
>>>>>>>> +---------------------------------------------------------+
>>>>>>>> | Channel B / FW1 +---+ |
>>>>>>>> | i0--| | |
>>>>>>>> | | | |
>>>>>>>> | i1--| D | |
>>>>>>>> | +---+ | P | |
>>>>>>>> | PHY1.0--| | i2--| L | +---+ +--------+ |
>>>>>>>> | | | +------+ | L |---| |---| PHY1.0 |--|
>>>>>>>> | PHY1.1--| M | | MUX0 |-i3--| 0 | | | +--------+ |
>>>>>>>> | | U | +------+ | |---| M |---| PHY1.1 |--|
>>>>>>>> | PHY1.2--| X |-+---------i4--| | | A | +--------+ |
>>>>>>>> | | 1 | | +---+ | C |---| PHY1.2 |--|
>>>>>>>> | ... --| | | i0--| | | 1 | +--------+ |
>>>>>>>> | | | | | |---| |---| ... |--|
>>>>>>>> | PHY1.7--| | | i1--| D | | | +--------+ |
>>>>>>>> | +---+ | | P |---| |---| PHY1.7 |--|
>>>>>>>> | | i2--| L | +---+ +--------+ |
>>>>>>>> | |+------+ | L | |
>>>>>>>> | || MUX0 |-i3--| 1 | |
>>>>>>>> | |+------+ | | |
>>>>>>>> | \---------i4--| | |
>>>>>>>> | +---+ |
>>>>>>>> +---------------------------------------------------------+
>>>>>>>
>>>>>>> What is "a channel" here? Are these 2 channels part of the same
>>> physival
>>>>>>> chip? Could you add the synchronizer chip/device entities to your
>>>>> drawing?
>>>>>>>
>>>>>>
>>>>>> No.
>>>>>> A "Synchronization Channel" on a switch would allow to separate
>groups
>>>>>> of physical ports. Each channel/group has own "Synchronizer Chip",
>>> which
>>>>> is
>>>>>> used to drive PHY clocks of that group.
>>>>>>
>>>>>> "Synchronizer chip" would be the 2 DPLLs on old draw, something like
>>>>> this:
>>>>>> +--------------------------------------------------------------+
>>>>>> | Channel A / FW0 +-------------+ +---+ +--------+ |
>>>>>> | i0--|Synchronizer0|---| |---| PHY0.0 |--|
>>>>>> | +---+ | | | | +--------+ |
>>>>>> | PHY0.0--| | i1--| |---| M |---| PHY0.1 |--|
>>>>>> | | | | +-----+ | | A | +--------+ |
>>>>>> | PHY0.1--| M | i2--| |DPLL0| | | C |---| PHY0.2 |--|
>>>>>> | | U | | +-----+ | | 0 | +--------+ |
>>>>>> | PHY0.2--| X |--+---i3--| +-----+ |---| |---| ... |--|
>>>>>> | | 0 | | | |DPLL1| | | | +--------+ |
>>>>>> | ... --| | | /-i4--| +-----+ |---| |---| PHY0.7 |--|
>>>>>> | | | | | +-------------+ +---+ +--------+ |
>>>>>> | PHY0.7--| | | | |
>>>>>> | +---+ | | |
>>>>>> +----------------|-|-------------------------------------------+
>>>>>> | Channel B / FW1| | +-------------+ +---+ +--------+ |
>>>>>> | | | i0--|Synchronizer1|---| |---| PHY1.0 |--|
>>>>>> | +---+ | | | | | | +--------+ |
>>>>>> | PHY1.0--| | | | i1--| |---| M |---| PHY1.1 |--|
>>>>>> | | | | | | +-----+ | | A | +--------+ |
>>>>>> | PHY1.1--| M | | | i2--| |DPLL0| | | C |---| PHY1.2 |--|
>>>>>> | | U | | | | +-----+ | | 1 | +--------+ |
>>>>>> | PHY1.2--| X | \-|-i3--| +-----+ |---| |---| ... |--|
>>>>>> | | 1 | | | |DPLL1| | | | +--------+ |
>>>>>> | ... --| |----+-i4--| +-----+ |---| |---| PHY1.7 |--|
>>>>>> | | | +-------------+ +---+ +--------+ |
>>>>>> | PHY1.7--| | |
>>>>>> | +---+ |
>>>>>> +--------------------------------------------------------------+
>>>>>> Also, please keep in mind that is an example, there could be easily 4
>>>>>> (or more) channels wired similarly.
>>>>>>
>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> This model tries to put too much into the synchronizer subsystem. The
>>>>> synchronizer device should only model inputs, DPLLs and outputs.
>>>>>
>>>>> The PHY lane to Synchronizer input muxing should be done in the
>>>>> PHY/netdev subsystem. That's why I wanted to start with the full model
>>>>> to specifically address this topic.
>>>>>
>>>>> The netdev should have an assigned list of Synchronizer inputs that it
>>>>> can recover its SyncE clocks into. It can be done by having a
>connection
>>>>> between the synchronizer input object(s) and the netdev, just like the
>>>>> netdev is connected to PHC clocks in the PHC subsystem. This is the
>>>>> model I initially presented about a year ago for solving this specific
>>>>> issue.
>>>>>
>>>>> Analogically, the netdev will be connected to a given output, however
>>>>> changing anything in the physical clock configuration sounds
>dangerous.
>>>>>
>>>>> Does that sound reasonable?
>>>>>
>>>>> Regards
>>>>> Maciek
>>>>
>>>> It sounds reasonable to some point.
>>>> You have mentioned list of Synchronizer inputs. If there is a list of
>>> inputs
>>>> it means it was created somewhere. I assume dpll subsystem? If so you
>>> would
>>>> like to export that list out of dpll subsystem, thus other entities
>would
>>> need
>>>> to find such list, then find particular source and somehow register
>with
>>> it.
>>>> All of this was proposed as part of netdev, I don't see any benefit in
>>> having
>>>> this parts separated from dpll, as only dpll would use it, right?
>>>> The same behavior is now provided by the MUX type pin, enclosed within
>>> dpll
>>>> subsystem.
>>>>
>>>> BR,
>>>> Arkadiusz
>>>
>>> The synchronizer object should expose the list of inputs that represent
>>> possible sources of a given chip. The list will be the same for all
>>> DPLLs used by the same device, so it can be a single set of sources
>>> linked to multiple DPLLs inside the package. A netdev can then point to
>>> a given input of a synchronizer that it's connected to.
>>> The phy lane->recovered clock (or directly a synchronizer input) muxing
>>> should stay in the netdev subsystem, or in the PHY driver.
>>>
>>> The reason, and benefit, of such split is when you create a board with a
>>> netdev X and a synchronizer Y that is not instantiated by the same
>>> driver. In this scenario you'd get the ice driver to instantiate
>>> connections and the DPLL vendor's driver for the synchronizer. In such
>>> case the netdev driver will simply send a netlink message to the
>>> input/source with a requested configuration, such as expected frequency,
>>> and everything from this point can be handled by a completely different
>>> driver creating clean and logical split.
>>>
>>> If we mix the phy lanes into the DPLL subsystem it'll get very
>>> challenging to add PHY lanes to the existing synchronizer exposed by a
>>> different driver.
>>
>> This is possible right now:
>> 1. obtain a dpll object:
>> struct dpll_device *dpll_device_get_by_clock_id(u64 clock_id,
>> enum dpll_type type, u8 idx);
>> 2. register new pin with muxed type pin:
>> int dpll_muxed_pin_register(struct dpll_device *dpll,
>> const char *parent_pin_description,
>> struct dpll_pin *pin,
>> struct dpll_pin_ops *ops, void *priv);
>>
>> To find dpll driver must know clock_id, type of dpll and its index given
>> when dpll was registered.
>> To register a pin, parent_pin_description of MUX type pin given on
>registering
>> it with dpll device.
>
>That would mean you need to repeat this process for all the DPLLs that
>are co-packaged in a single synchronizer. Some chips have up to 8 DPLLs,
>so you'd need to register number of phy lanes x number of DPLL times,
>say 8x8 = 64 times - that's simply too messy in the long term.
>
No, the pins are shared also MUX-type ones, adding to one adds to all.
>>> Exporting and link between the synchronizer and the netdev is still a
>>> must no matter which way we go. And IMO it's best to link netdev to
>>> synchronizer sources, as that's the most natural way.
>>>
>>
>> The link is now just information for userspace Linux network interface
>index
>> in DPLLA_PIN_NETIFINDEX attribute.
>
>That's not the right way. We need to know:
>- which DPLL pins/sources are driven by which netdev (to be able to
> identify a source of frequency that is currently driving a given DPLL)
This is possible as explained above.
>- which DPLL generates a frequency for a given netdev (to know which
> DPLL to check for a specific netdev)
>
I agree here, we need something like it.
BR,
Arkadiusz.
>So we need to have 2 connections.
More information about the linux-arm-kernel
mailing list