Date: Wed, 24 Apr 2024 17:07:46 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: Jacob Keller <jacob.e.keller@...el.com>
Cc: "Temerkhanov, Sergey" <sergey.temerkhanov@...el.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"Kitszel, Przemyslaw" <przemyslaw.kitszel@...el.com>
Subject: Re: [Intel-wired-lan] [PATCH iwl-next v2] ice: Extend auxbus device
 naming

Wed, Apr 24, 2024 at 12:03:25AM CEST, jacob.e.keller@...el.com wrote:
>
>
>On 4/23/2024 6:14 AM, Jiri Pirko wrote:
>> Tue, Apr 23, 2024 at 01:56:55PM CEST, sergey.temerkhanov@...el.com wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Jiri Pirko <jiri@...nulli.us>
>>>> Sent: Tuesday, April 23, 2024 1:36 PM
>>>> To: Temerkhanov, Sergey <sergey.temerkhanov@...el.com>
>>>> Cc: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org; Kitszel,
>>>> Przemyslaw <przemyslaw.kitszel@...el.com>
>>>> Subject: Re: [PATCH iwl-next v2] ice: Extend auxbus device naming
>>>>
>>>> Tue, Apr 23, 2024 at 11:14:59AM CEST, sergey.temerkhanov@...el.com
>>>> wrote:
>>>>> Include segment/domain number in the device name to distinguish
>>>> between
>>>>> PCI devices located on different root complexes in multi-segment
>>>>> configurations. Naming is changed from ptp_<bus>_<slot>_clk<clock>  to
>>>>> ptp_<domain>_<bus>_<slot>_clk<clock>
>>>>
>>>> I don't understand why you need to encode pci properties of a parent device
>>>> into the auxiliary bus name. Could you please explain the motivation? Why
>>>> you need a bus instance per PF?
>>>>
>>>> The rest of the auxbus registrators don't do this. Could you please align? Just
>>>> have one bus for ice driver and that's it.
>>>
>>> This patch adds support for multi-segment PCIe configurations.
>>> An auxdev is created for each adapter, which has a clock, in the system. There can be
>> 
>> You are trying to change auxiliary bus name.
>> 
>> 
>>> more than one adapter present, so there exists a possibility of device naming conflict.
>>> To avoid it, auxdevs are named according to the PCI geographical addresses of the adapters.
>> 
>> Why? It's the auxdev, the name should not contain anything related to
>> PCI, no reason for it. I asked for motivation, you didn't provide any.
>> 
>
>We aren't creating one auxbus per PF. We're creating one auxbus per
>*clock*. The device has multiple clocks in some configurations.

Does not matter. Why you need separate bus for whatever-instance? Why
can't you just have one ice-ptp bus and put devices on it?


>
>We need to connect each PF to the same clock controller, because there
>is only one clock owner for the device in a multi-port card.

Connect how? But putting a PF-device on a per-clock bus? That sounds
quite odd. How did you figure out this usage of auxiliary bus?


>
>> Again, could you please avoid creating auxiliary bus per-PF and just
>> have one auxiliary but per-ice-driver?
>> 
>
>We can't have one per-ice driver, because we don't want to connect ports
>from a different device to the same clock. If you have two ice devices
>plugged in, the ports on each device are separate from each other.
>
>The goal here is to connect the clock ports to the clock owner.
>
>Worse, as described here, we do have some devices which combine multiple
>adapters together and tie their clocks together via HW signaling. In
>those cases the clocks *do* need to communicate across the device, but
>only to other ports on the same physical device, not to ports on a
>different device.
>
>Perhaps auxbus is the wrong approach here? but how else can we connect

Yeah, feels quite wrong.


>these ports without over-connecting to other ports? We could write
>bespoke code which finds these devices, but that felt like it was risky
>and convoluted.

Sounds you need something you have for DPLL subsystem. Feels to me that
your hw design is quite disconnected from the Linux device model :/


>
>Perhaps it would be ideal if something in the PCI layer could connect
>these together? I don't know how that would be implemented though..
>
>The fundamental problem is that we have a multi-function device with
>some shared functionality which we need to manage across function. In
>this case it is the clock should only have one entity, while the ports
>connected to it are controlled independently by PF. We tried a variety
>of ways to solve this in the past, mostly with hacky solutions.
>
>We need an entity which can find all the ports connected to a single
>clock, and the port needs to be able to get back to its clock. If we
>used a single auxdriver for this, that driver would have to maintain
>some hash tables or connections in order to locate which ports belonged
>to the clock. It would also need to figure out which port was the
>"owner" so that it could send owner-based requests through that port,
>since it would not inherently have access to the clock hardware since
>its a global entity and not tied to a port.
>
>In the current model, the driver can go back to the PF that spawned it
>to manage the clock, and uses the aux devices as a mechanism to connect
>to each port for purposes such as initializing the PHYs, and caching the
>PTP hardware time for timestamp extension.
>
>Maybe you disagree with this use of auxbus? Do you have any suggestions
>for a separate model?
>
>We could make use of ice_adapter, though we'd need some logic to manage
>devices which have multiple clocks, as well as devices like the one
>Sergey is working on which tie multiple adapters together.

Perhaps that is an answer. Maybe we can make DPLL much more simple after
that :)

Powered by blists - more mailing lists