Message-ID: <Y5CofhLCykjsFie6@nanopsycho>
Date:   Wed, 7 Dec 2022 15:51:42 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>,
        Vadim Fedorenko <vfedorenko@...ek.ru>,
        Jonathan Lemon <jonathan.lemon@...il.com>,
        Paolo Abeni <pabeni@...hat.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>
Subject: Re: [RFC PATCH v4 0/4] Create common DPLL/clock configuration API

Wed, Dec 07, 2022 at 03:47:40AM CET, kuba@...nel.org wrote:
>On Fri, 2 Dec 2022 17:12:06 +0100 Jiri Pirko wrote:
>> >But this is only doable with assumption, that the board is internally capable
>> >of such internal board level communication, which in case of separated
>> >firmwares handling multiple dplls might not be the case, or it would require
>> >to have some other sw component feel that gap.  
>> 
>> Yep, you have the knowledge of sharing inside the driver, so you should
>> do it there. For multiple instances, use in-driver notifier for example.
>
>No, complexity in the drivers is not a good idea. The core should cover
>the complexity and let the drivers be simple.

Really, even in case only one driver actually consumes the complexicity?
I understand having a "libs" to do common functionality for drivers,
even in case there is one. But this case, I don't see any benefit.


>
>> >For complex boards with multiple dplls/sync channels, multiple ports,
>> >multiple firmware instances, it seems to be complicated to share a pin if
>> >each driver would have own copy and should notify all the other about changes.
>> >
>> >To summarize, that is certainly true, shared pins idea complicates stuff
>> >inside of dpll subsystem.
>> >But at the same time it removes complexity from all the drivers which would use  
>> 
>> There are currently 3 drivers for dpll I know of. This in ptp_ocp and
>> mlx5 there is no concept of sharing pins. You you are talking about a
>> single driver.
>> 
>> What I'm trying to say is, looking at the code, the pin sharing,
>> references and locking makes things uncomfortably complex. You are so
>> far the only driver to need this, do it internally. If in the future
>> other driver appears, this code would be eventually pushed into dpll
>> core. No impact on UAPI from what I see. Please keep things as simple as
>> possible.
>
>But the pin is shared for one driver. Who cares if it's not shared in
>another. The user space must be able to reason about the constraints.

Sorry, I don't follow :/ Could you please explain what do you mean by
this?

>
>You are suggesting drivers to magically flip state in core objects
>because of some hidden dependencies?!

It's not a state flip. It's more like a well propagated event of a state
change. The async changes may happen anyway, so the userspace needs
to handle them. Why is this different?


>
>> >it and is easier for the userspace due to common identification of pins.  
>> 
>> By identification, you mean "description" right? I see no problem of 2
>> instances have the same pin "description"/label.
>>
>> >This solution scales up without any additional complexity in the driver,
>> >and without any need for internal per-board communication channels.
>> >
>> >Not sure if this is good or bad, but with current version, both approaches are
>> >possible, so it pretty much depending on the driver to initialize dplls with
>> >separated pin objects as you have suggested (and take its complexity into
>> >driver) or just share them.
>> >  
>> >>
>> >>3) I don't like the concept of muxed pins and hierarchies of pins. Why
>> >>   does user care? If pin is muxed, the rest of the pins related to this
>> >>   one should be in state disabled/disconnected. The user only cares
>> >>   about to see which pins are related to each other. It can be easily
>> >>   exposed by "muxid" like this:
>> >>   pin 1
>> >>   pin 2
>> >>   pin 3 muxid 100
>> >>   pin 4 muxid 100
>> >>   pin 5 muxid 101
>> >>   pin 6 muxid 101
>> >>   In this example pins 3,4 and 5,6 are muxed, therefore the user knows
>> >>   if he connects one, the other one gets disconnected (or will have to
>> >>   disconnect the first one explicitly first).
>> >
>> >Currently DPLLA_PIN_PARENT_IDX is doing the same thing as you described, it
>> >groups MUXed pins, the parent pin index here was most straightforward to me,  
>> 
>> There is a big difference if we model flat list of pins with a set of
>> attributes for each, comparing to a tree of pins, some acting as leaf,
>> node and root. Do we really need such complexicity? What value does it
>> bring to the user to expose this?
>
>The fact that you can't auto select from devices behind muxes.

Why? What's wrong with the mechanism I described in another part of this
thread?

Extending my example from above

   pin 1 source
   pin 2 output
   pin 3 muxid 100 source
   pin 4 muxid 100 source
   pin 5 muxid 101 source
   pin 6 muxid 101 source
   pin 7 output

User now can set individial prios for sources:

dpll x pin 1 set prio 10
etc
The result would be:

   pin 1 source prio 10
   pin 2 output
   pin 3 muxid 100 source prio 8
   pin 4 muxid 100 source prio 20
   pin 5 muxid 101 source prio 50
   pin 6 muxid 101 source prio 60
   pin 7 output

Now when auto is enabled, the pin 3 is selected. Why would user need to
manually select between 3 and 4? This is should be abstracted out by the
driver.

Actually, this is neat as you have one cmd to do selection in manual
mode and you have uniform way of configuring/monitoring selection in
autosel. Would the muxed pin make this better?

For muxed pin being output, only one pin from mux would be set:

   pin 1 source
   pin 2 output
   pin 3 muxid 100 disconnected
   pin 4 muxid 100 disconnected
   pin 5 muxid 101 output
   pin 6 muxid 101 disconnected
   pin 7 output


>The HW topology is of material importance to user space.

Interesting. When I was working on linecards, you said that the user
does not care about the inner HW topology. And it makes sense. When
things could be abstracted out to make iface clean, I think they should.


>How many times does Arkadiusz have to explain this :|

Pardon my ignorance, I don't see why exactly we need mux hierarchy
(trees) exposed to user.

Powered by blists - more mailing lists