Message-ID: <ZHCMYJGMYS5a2+Bf@nanopsycho>
Date: Fri, 26 May 2023 12:39:28 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
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

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?



>
>
>>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.



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