Date:   Fri, 9 Dec 2022 00:46:32 +0000
From:   "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
To:     Jakub Kicinski <kuba@...nel.org>,
        "netdev.dump@...il.com" <netdev.dump@...il.com>
CC:     'Jiri Pirko' <jiri@...nulli.us>,
        '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

>From: Jakub Kicinski <kuba@...nel.org>
>Sent: Thursday, December 8, 2022 12:22 AM
>
>On Wed, 7 Dec 2022 15:09:03 +0100 netdev.dump@...il.com wrote:
>> > -----Original Message-----
>> > From: Jakub Kicinski <kuba@...nel.org>
>> > Sent: Wednesday, December 7, 2022 3:48 AM
>> > Subject: Re: [RFC PATCH v4 0/4] Create common DPLL/clock configuration
>API
>> >
>> > On Fri, 2 Dec 2022 17:12:06 +0100 Jiri Pirko wrote:
>>  [...]
>> capable
>>  [...]
>> require
>>  [...]
>
>Please fix line wrapping in your email client.
>And add a name to your account configuration :/
>
>> > > 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.
>>
>> But how does Driver A know where to connect its pin to? It makes sense to
>> share
>
>I think we discussed using serial numbers.

Right now, driver can find dpll with:
struct dpll_device *dpll_device_get_by_cookie(u8 cookie[DPLL_COOKIE_LEN],
                                              enum dpll_type type, u8 idx);
Where arguments would be the same as given when first instance have allocated
dpll with:
struct dpll_device
*dpll_device_alloc(struct dpll_device_ops *ops, enum dpll_type type,
                   const u8 cookie[DPLL_COOKIE_LEN], u8 dev_driver_idx,
                   void *priv, struct device *parent);

Which means all driver instances must know those values if they need to share
dpll or pins.

>
>> pins between the DPLLs exposed by a single driver, but not really outside
>of
>> it.
>> And that can be done simply by putting the pin ptr from the DPLLA into
>the
>> pin
>> list of DPLLB.
>
>Are you saying within the driver it's somehow easier? The driver state
>is mostly per bus device, so I don't see how.
>
>> If we want the kitchen-and-sink solution, we need to think about corner
>> cases.
>> Which pin should the API give to the userspace app - original, or
>> muxed/parent?
>
>IDK if I parse but I think both. If selected pin is not directly
>attached the core should configure muxes.

If there is real need for muxed pin (hardware with support for priority based
Auto-selection or some other hardware constraints), then both.
As priority is set on mux-type/parent pin, but selection of muxed pin would be
done manually with DPLL_CMD_DEVICE_SET and given DPLLA_SOURCE_PIN_IDX.
If the hardware doesn't support priority based auto-selection, then it might
be better to just add new pin into existing dpll without any mux-type pins,
this way your driver would be simpler. But also possible to follow the same
approach, add mux-type parent on one instance and register new pins from
different instances with that parent, both are propagated to userspace app.

>
>> How would a teardown look like - if Driver A registered DPLLA with Pin1
>and
>> Driver B added the muxed pin then how should Driver A properly
>> release its pins? Should it just send a message to driver B and trust
>that
>> it
>> will receive it in time before we tear everything apart?
>
>Trivial.

With current version...
Driver A creates dpll (as it was initialized first).
Driver B:
- allocates new pin
- finds existing parent pin (find dpll (dpll_device_get_by_cookie), find pin
(dpll_pin_get_by_description).
- registers new pin with parent pin found.

For dealloc Driver B shall deregister and free it's pin:
dpll_pin_deregister(struct dpll_device *dpll, struct dpll_pin *pin);
dpll_pin_free(struct dpll_pin *pin);
This shall be done before Driver A deregisters dpll.
As long as there is a reference to the Driver B registered pin in dpll created
by Driver A, the dpll won't be deregistered.

>
>> There are many problems with that approach, and the submitted patch is
>not
>> explaining any of them. E.g. it contains the dpll_muxed_pin_register but
>no
>> free
>> counterpart + no flows.
>
>SMOC.
>

_register counterpart is _deregister
_alloc counterpart is _free
Whichever pin-register function the one would use, it shall use
dpll_pin_deregister(..) when releasing resource.

>> If we want to get shared pins, we need a good example of how this
>mechanism
>> can be used.
>
>Agreed.

Shall be provided in next version of patch series.

>
>> > > 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.
>> >
>> > You are suggesting drivers to magically flip state in core objects
>> > because of some hidden dependencies?!
>>
>> If we want to go outside the device, we'd need some universal language
>> to describe external connections - such as the devicetree. I don't see
>how
>> we can reliably implement inter-driver dependency otherwise.
>
>There's plenty examples in the tree. If we can't use serial number
>directly we can compare the driver pointer + whatever you'd compare
>in the driver internal solution.
>
>> I think this would be better served in the userspace with a board-
>specific
>> config file. Especially since the pins can be externally connected
>anyway.
>
>Opinions vary, progress is not being made.

Powered by blists - more mailing lists