Message-ID: <DM6PR11MB46578F9EF0B6A8697F47EE9D9BCC9@DM6PR11MB4657.namprd11.prod.outlook.com>
Date:   Fri, 27 Jan 2023 18:12:48 +0000
From:   "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
To:     Jiri Pirko <jiri@...nulli.us>, Vadim Fedorenko <vadfed@...a.com>
CC:     Jakub Kicinski <kuba@...nel.org>,
        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>,
        "Olech, Milena" <milena.olech@...el.com>,
        "Michalik, Michal" <michal.michalik@...el.com>
Subject: RE: [RFC PATCH v5 1/4] dpll: Add DPLL framework base functions

>From: Jiri Pirko <jiri@...nulli.us>
>Sent: Friday, January 20, 2023 1:57 PM
>
>Thu, Jan 19, 2023 at 06:16:13PM CET, jiri@...nulli.us wrote:
>>Tue, Jan 17, 2023 at 07:00:48PM CET, vadfed@...a.com wrote:
>
>[...]
>
>
>>>+/**
>>>+ * dpll_cmd - Commands supported by the dpll generic netlink family
>>>+ *
>>>+ * @DPLL_CMD_UNSPEC - invalid message type
>>>+ * @DPLL_CMD_DEVICE_GET - Get list of dpll devices (dump) or attributes
>>of
>>>+ *	single dpll device and it's pins
>>>+ * @DPLL_CMD_DEVICE_SET - Set attributes for a dpll
>>>+ * @DPLL_CMD_PIN_SET - Set attributes for a pin
>>>+ **/
>>>+enum dpll_cmd {
>>>+	DPLL_CMD_UNSPEC,
>>>+	DPLL_CMD_DEVICE_GET,
>>>+	DPLL_CMD_DEVICE_SET,
>>>+	DPLL_CMD_PIN_SET,
>>
>>Have pin get to get list of pins, then you can have 1:1 mapping to
>>events and loose the enum dpll_event_change. This is the usual way to do
>>stuff. Events have the same cmd and message format as get.
>
>I was thinking about that a bit more.
>1) There is 1:n relationship between PIN and DPLL(s).
>2) The pin configuration is independent on DPLL, with an
>   exeption of PRIO.
>

DPLL configuration is dependant on PIN configuration.
If the pin is configured with a frequency/mode all of dplls connected with it
shall know about it.

>Therefore as I suggested in the reply to this patch, the pin should be
>separate entity, allocated and having ops unrelated to DPLL. It is just
>registered to the DPLLs that are using the pin.
>

Don't mind having them as separated entities (as they are already separated in
kernel space, allocated separately and registered with dpll/s as needed).
You might also remember that first version had the "global" list of pins, which
was removed due to the comments. For having get/dump pins command it would be
better to fallback to that approach, with global XA of pins in dpll_core.c.

>The pin ops should not have dpll pointer as arg, again with exception of
>PRIO.
>

Sure, probably with this approach we could remove dpll pointer for global-pin
attributes.

>DPLL_CMD_DEVICE_GET should not contain pins at all.
>
>There should be DPLL_CMD_PIN_GET added which can dump and will be used
>to get the list of pins in the system.
>- if DPLL handle is passed to DPLL_CMD_PIN_GET, it will dump only pins
>  related to the specified DPLL.
>

Sure, we agreed on that already.

>DPLL_CMD_PIN_GET message will contain pin-specific attrs and will have a
>list of connected DPLLs:
>       DPLLA_PIN_IDX
>       DPLLA_PIN_DESCRIPTION
>       DPLLA_PIN_TYPE
>       DPLLA_PIN_SIGNAL_TYPE
>       DPLLA_PIN_SIGNAL_TYPE_SUPPORTED
>       DPLLA_PIN_CUSTOM_FREQ
>       DPLLA_PIN_MODE
>       DPLLA_PIN_MODE_SUPPORTED
>       DPLLA_PIN_PARENT_IDX
>       DPLLA_PIN_DPLL    (nested)
>          DPLLA_DPLL_HANDLE   "dpll_0"
>          DPLLA_PIN_PRIO    1
>       DPLLA_PIN_DPLL    (nested)
>          DPLLA_DPLL_HANDLE   "dpll_1"
>          DPLLA_PIN_PRIO    2
>
>Please take the names lightly. My point is to show 2 nests for 2
>DPLLS connected, on each the pin has different prio.
>
>Does this make sense?
>

Seems good, I guess dpll referenced by giving their bus/name, as below.

>One issue to be solved is the pin indexing. As pin would be separate
>entity, the indexing would be global and therefore not predictable. We
>would have to figure it out differntly. Pehaps something like this:
>
>$ dpll dev show
>pci/0000:08:00.0: dpll 1             first dpll on 0000:08:00.0
>pci/0000:08:00.0: dpll 2             second dpll on the same pci device
>pci/0000:09:00.0: dpll 1             first dpll on 0000:09:00.0
>pci/0000:09:00.0: dpll 2             second dpll on the same pci device
>
>$ dpll pin show
>pci/0000:08:00.0: pin 1 desc SOMELABEL_A
>  dpll 1:                          This refers to DPLL 1 on the same pci
>device
>    prio 80
>  dpll 2:                          This refers to DPLL 2 on the same pci
>device
>    prio 100
>pci/0000:08:00.0: pin 2 desc SOMELABEL_B
>  dpll 1:
>    prio 80
>  dpll 2:
>    prio 100
>pci/0000:08:00.0: pin 3 desc SOMELABEL_C
>  dpll 1:
>    prio 80
>  dpll 2:
>    prio 100
>pci/0000:08:00.0: pin 4 desc SOMELABEL_D
>  dpll 1:
>    prio 80
>  dpll 2:
>    prio 100
>pci/0000:09:00.0: pin 1 desc SOMEOTHERLABEL_A
>  dpll 1:
>    prio 80
>  dpll 2:
>    prio 100
>pci/0000:09:00.0: pin 2 desc SOMEOTHERLABEL_B
>  dpll 1:
>    prio 80
>  dpll 2:
>    prio 100
>pci/0000:09:00.0: pin 3 desc SOMEOTHERLABEL_C
>  dpll 1:
>    prio 80
>  dpll 2:
>    prio 100
>pci/0000:09:00.0: pin 4 desc SOMEOTHERLABEL_C
>  dpll 1:
>    prio 80
>  dpll 2:
>    prio 100
>
>Note there are 2 groups of pins, one for each pci device.
>
>Setting some attribute command would looks like:
>To set DPLL mode:
>$ dpll dev set pci/0000:08:00.0 dpll 1 mode freerun
>   netlink:
>   DPLL_CMD_DEVICE_SET
>      DPLLA_BUS_NAME "pci"
>      DPLLA_DEV_NAME "0000:08:00.0"
>      DPLLA_DPLL_INDEX 1
>      DPLLA_DPLL_MODE 3
>
>$ dpll dev set pci/0000:08:00.0 dpll 2 mode automatic
>
>
>To set signal frequency in HZ:
>$ dpll pin set pci/0000:08:00.0 pin 3 frequency 10000000
>   netlink:
>   DPLL_CMD_PIN_SET
>      DPLLA_BUS_NAME "pci"
>      DPLLA_DEV_NAME "0000:08:00.0"
>      DPLLA_PIN_INDEX 3
>      DPLLA_PIN_FREQUENCY 10000000
>
>$ dpll pin set pci/0000:08:00.0 pin 1 frequency 1
>
>
>To set individual of one pin for 2 DPLLs:
>$ dpll pin set pci/0000:08:00.0 pin 1 dpll 1 prio 40
>   netlink:
>   DPLL_CMD_PIN_SET
>      DPLLA_BUS_NAME "pci"
>      DPLLA_DEV_NAME "0000:08:00.0"
>      DPLLA_PIN_INDEX 1
>      DPLLA_DPLL_INDEX 1
>      DPLLA_PIN_PRIO 40
>
>$ dpll pin set pci/0000:08:00.0 pin 1 dpll 2 prio 80
>
>
>Isn't this neat?
>
>
>[...]

Seems so.

As I suggested in other response on this thread, the best solution for
pin index, would be IMHO to allow registering driver assign index to a pin.
This would also solve/simplify sharing the pins. As the other driver would
just pass that index to register it with second dpll.

Also all dplls shall have the possibility to be notified that a pin which was
registered with them has changed (except prio which is set only for single
dpll, and for prio only one callback shall be invoked).
This is because not all dplls which are sharing pins will be controlled by the
same FW/driver instance.
Currently for registering a pin with second dpll, the caller passes another
set of callback ops, which is then held in dpll_pin_ref - for all dplls which
were registered with the pin. But as this is not always needed (i.e. same
instance controlling all dplls and pins), thus kernel module can pass ops=NULL
when registering a pin with second dpll, efectively executing only one callback
per pin.

Will try to have it done in next version.

Thanks,
Arkadiusz

Powered by blists - more mailing lists