Re: [RFC PATCH v4 0/4] Create common DPLL/clock configuration API

[Maciek Machnikowski <maciek@machnikowski.net>]

On 1/11/2023 3:17 PM, Kubalewski, Arkadiusz wrote:
>> From: Maciek Machnikowski <maciek@machnikowski.net>
>> Sent: Tuesday, January 10, 2023 3:59 PM
>> To: Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; Jiri Pirko
>> <jiri@resnulli.us>
>>
>> On 1/10/2023 11:54 AM, Kubalewski, Arkadiusz wrote:
>>>> From: Jiri Pirko <jiri@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.

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.

Thanks,
Maciek