Re: [RFC PATCH v5 1/4] dpll: Add DPLL framework base functions

Jiri Pirko
2023-01-20T12:57:48Z

Thu, Jan 19, 2023 at 06:16:13PM CET, jiri@resnulli.us wrote:
>Tue, Jan 17, 2023 at 07:00:48PM CET, vadfed@meta.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.

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.

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

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.

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?

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? …