Message-ID: <DM6PR11MB4657605F659D6E2B9B2C1A219B4DA@DM6PR11MB4657.namprd11.prod.outlook.com>
Date: Mon, 5 Jun 2023 10:07:01 +0000
From: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
To: Jiri Pirko <jiri@...nulli.us>
CC: Vadim Fedorenko <vadfed@...a.com>, Jakub Kicinski <kuba@...nel.org>,
	Jonathan Lemon <jonathan.lemon@...il.com>, Paolo Abeni <pabeni@...hat.com>,
	"Olech, Milena" <milena.olech@...el.com>, "Michalik, Michal"
	<michal.michalik@...el.com>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, Vadim Fedorenko
	<vadim.fedorenko@...ux.dev>, poros <poros@...hat.com>, mschmidt
	<mschmidt@...hat.com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>
Subject: RE: [RFC PATCH v7 0/8] Create common DPLL configuration API

>From: Jiri Pirko <jiri@...nulli.us>
>Sent: Friday, May 26, 2023 12:39 PM
>
>Fri, May 26, 2023 at 12:14:00PM CEST, arkadiusz.kubalewski@...el.com wrote:
>>>From: Jiri Pirko <jiri@...nulli.us>
>>>Sent: Wednesday, May 17, 2023 12:19 PM
>>>
>>>Let me summarize the outcome of the discussion between me and Jakub
>>>regarding attributes, handles and ID lookups in the RFCv7 thread:
>>>
>>>--------------------------------------------------------------
>>>** Needed changes for RFCv8 **
>>>
>>>1) No scoped indexes.
>>>   The indexes passed from driver to dpll core during call of:
>>>        dpll_device_get() - device_idx
>>>        dpll_pin_get() - pin_idx
>>>   should be for INTERNAL kernel use only and NOT EXPOSED over uapi.
>>>   Therefore following attributes need to be removed:
>>>   DPLL_A_PIN_IDX
>>>   DPLL_A_PIN_PARENT_IDX
>>>
>>
>>Seems doable.
>>So just to be clear, configuring a pin-pair (MUXed pins) will now be done
>>with DPLL_A_PIN_PARENT nested attribute.
>>I.e. configuring state of pin on parent:
>>DPLL_CMD_PIN_SET
>>	DPLL_A_PIN_ID		(id of pin being configured)
>>	DPLL_A_PIN_PARENT	(nested)
>>		DPLL_A_PIN_ID	(id of parent pin)
>>		DPLL_A_PIN_STATE(expected state)
>>		...		(other pin-pair attributes to be set)
>>
>>Is that ok, or we need separated attribute like DPLL_A_PIN_PARENT_ID??
>>I think there is no need for separated one, documentation shall just
>>reflect that.
>>Also we have nested attribute DPLL_A_DEVICE which is used to show
>connections
>>between PIN and multiple dpll devices, to make it consistent I will rename
>>it to `DPLL_A_DEVICE_PARENT` and make configuration set cmd for the pin-dpll
>>pair similar to the above:
>>DPLL_CMD_PIN_SET
>>	DPLL_A_PIN_ID		(id of pin being configured)
>>	DPLL_A_DEVICE_PARENT	(nested)
>
>It is a parent of pin, not device. The name is confusing. But see below.
>
>
>>		DPLL_A_ID	(id of parent dpll)
>>		DPLL_A_PIN_STATE(expected state)
>>		...		(other pin-dpll attributes to be set)
>>
>>Does it make sense?
>
>Yeah, good idea. I like this. We will have consistent approach for
>parent pin and device. To take it even further, we can have one nested
>attr for parent and decide the parent type according to the id attr
>given:
>
>DPLL_CMD_PIN_SET
>	DPLL_A_PIN_ID		(id of pin being configured)
>	DPLL_A_PIN_PARENT	(nested)
>		DPLL_A_PIN_ID	(id of parent pin)
>		DPLL_A_PIN_STATE(expected state)
>		...		(other pin-pair attributes to be set)
>
>DPLL_CMD_PIN_SET
>	DPLL_A_PIN_ID		(id of pin being configured)
>	DPLL_A_PIN_PARENT	(nested)
>		DPLL_A_ID	(id of parent dpll)
>		DPLL_A_PIN_STATE(expected state)
>		...		(other pin-dpll attributes to be set)
>
>
>Same for PIN_GET
>
>Makes sense?
>

Sure, fixed.

>
>
>>
>>
>>>2) For device, the handle will be DPLL_A_ID == dpll->id.
>>>   This will be the only handle for device for every
>>>   device related GET, SET command and every device related notification.
>>>   Note: this ID is not deterministing and may be different depending on
>>>   order of device probes etc.
>>>
>>
>>Seems doable.
>>
>>>3) For pin, the handle will be DPLL_A_PIN_ID == pin->id
>>>   This will be the only handle for pin for every
>>>   pin related GET, SET command and every pin related notification.
>>>   Note: this ID is not deterministing and may be different depending on
>>>   order of device probes etc.
>>>
>>
>>Seems doable.
>>
>>>4) Remove attribute:
>>>   DPLL_A_PIN_LABEL
>>>   and replace it with:
>>>   DPLL_A_PIN_PANEL_LABEL (string)
>>>   DPLL_A_PIN_XXX (string)
>>>   where XXX is a label type, like for example:
>>>     DPLL_A_PIN_BOARD_LABEL
>>>     DPLL_A_PIN_BOARD_TRACE
>>>     DPLL_A_PIN_PACKAGE_PIN
>>>
>>
>>Sorry, I don't get this idea, what are those types?
>>What are they for?
>
>The point is to make the driver developer to think before passing
>randomly constructed label strings. For example, "board_label" would lead
>the developer to check how the pin is labeled on the board. The
>"panel_label" indicates this is label on a panel. Also, developer can
>fill multiple labels for the same pin.
>

Ok, makes sense, added as suggested.

Thank you,
Arkadiusz

>
>
>>
>>>5) Make sure you expose following attributes for every device and
>>>   pin GET/DUMP command reply message:
>>>   DPLL_A_MODULE_NAME
>>>   DPLL_A_CLOCK_ID
>>>
>>
>>Seems doable.
>>
>>>6) Remove attributes:
>>>   DPLL_A_DEV_NAME
>>>   DPLL_A_BUS_NAME
>>>   as they no longer have any value and do no make sense (even in RFCv7)
>>>
>>
>>Seems doable.
>>
>>>
>>>--------------------------------------------------------------
>>>** Lookup commands **
>>>
>>>Basically these would allow user to query DEVICE_ID and PIN_ID
>>>according to provided atributes (see examples below).
>>>
>>>These would be from my perspective optional for this patchsets.
>>>I believe we can do it as follow-up if needed. For example for mlx5
>>>I don't have usecase for it, since I can consistently get PIN_ID
>>>using RT netlink for given netdev. But I can imagine that for non-SyncE
>>>dpll driver this would make sense to have.
>>>
>>>1) Introduce CMD_GET_ID - query the kernel for a dpll device
>>>                          specified by given attrs
>>>   Example:
>>>   -> DPLL_A_MODULE_NAME
>>>      DPLL_A_CLOCK_ID
>>>      DPLL_A_TYPE
>>>   <- DPLL_A_ID
>>>   Example:
>>>   -> DPLL_A_MODULE_NAME
>>>      DPLL_A_CLOCK_ID
>>>   <- DPLL_A_ID
>>>   Example:
>>>   -> DPLL_A_MODULE_NAME
>>>   <- -EINVAL (Extack: "multiple devices matched")
>>>
>>>   If user passes a subset of attrs which would not result in
>>>   a single match, kernel returns -EINVAL and proper extack message.
>>>
>>
>>Seems ok.
>>
>>>2) Introduce CMD_GET_PIN_ID - query the kernel for a dpll pin
>>>                              specified by given attrs
>>>   Example:
>>>   -> DPLL_A_MODULE_NAME
>>>      DPLL_A_CLOCK_ID
>>>      DPLL_A_PIN_TYPE
>>>      DPLL_A_PIN_PANEL_LABEL
>>>   <- DPLL_A_PIN_ID
>>>   Example:
>>>   -> DPLL_A_MODULE_NAME
>>>      DPLL_A_CLOCK_ID
>>>   <- DPLL_A_PIN_ID    (There was only one pin for given module/clock_id)
>>>   Example:
>>>   -> DPLL_A_MODULE_NAME
>>>      DPLL_A_CLOCK_ID
>>>   <- -EINVAL (Extack: "multiple pins matched")
>>>
>>>   If user passes a subset of attrs which would not result in
>>>   a single match, kernel returns -EINVAL and proper extack message.
>>
>>
>>Seems ok.
>>
>>Will try to implement those now.
>
>Cool, thx!
>
>
>>
>>Thank you,
>>Arkadiusz

Powered by blists - more mailing lists