Message-ID: <DM6PR11MB465713389A234771BD29DF149B02A@DM6PR11MB4657.namprd11.prod.outlook.com>
Date: Mon, 24 Jul 2023 15:03:55 +0000
From: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
To: Jiri Pirko <jiri@...nulli.us>
CC: "kuba@...nel.org" <kuba@...nel.org>, Vadim Fedorenko
	<vadim.fedorenko@...ux.dev>, 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>, 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>, Bart Van Assche
	<bvanassche@....org>
Subject: RE: [PATCH 09/11] ice: implement dpll interface to control cgu

>From: Jiri Pirko <jiri@...nulli.us>
>Sent: Saturday, July 22, 2023 8:37 AM
>
>Fri, Jul 21, 2023 at 09:48:18PM CEST, arkadiusz.kubalewski@...el.com wrote:
>>>From: Jiri Pirko <jiri@...nulli.us>
>>>Sent: Friday, July 21, 2023 5:46 PM
>>>
>>>Fri, Jul 21, 2023 at 03:36:17PM CEST, arkadiusz.kubalewski@...el.com wrote:
>>>>>From: Jiri Pirko <jiri@...nulli.us>
>>>>>Sent: Friday, July 21, 2023 2:02 PM
>>>>>
>>>>>Fri, Jul 21, 2023 at 01:17:59PM CEST, arkadiusz.kubalewski@...el.com
>>>>>wrote:
>>>>>>>From: Jiri Pirko <jiri@...nulli.us>
>>>>>>>Sent: Friday, July 21, 2023 9:33 AM
>>>>>>>
>>>>>>>Thu, Jul 20, 2023 at 07:31:14PM CEST, arkadiusz.kubalewski@...el.com
>>>>>>>wrote:
>>>>>>>>>From: Jiri Pirko <jiri@...nulli.us>
>>>>>>>>>Sent: Thursday, July 20, 2023 4:09 PM
>>>>>>>>>
>>>>>>>>>Thu, Jul 20, 2023 at 11:19:01AM CEST, vadim.fedorenko@...ux.dev
>>>>>>>>>wrote:
>>>>>>>>>>From: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>
>>>>>>>>>
>>>>>>>>>[...]
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>+/**
>>>>>>>>>>+ * ice_dpll_pin_enable - enable a pin on dplls
>>>>>>>>>>+ * @hw: board private hw structure
>>>>>>>>>>+ * @pin: pointer to a pin
>>>>>>>>>>+ * @pin_type: type of pin being enabled
>>>>>>>>>>+ * @extack: error reporting
>>>>>>>>>>+ *
>>>>>>>>>>+ * Enable a pin on both dplls. Store current state in pin->flags.
>>>>>>>>>>+ *
>>>>>>>>>>+ * Context: Called under pf->dplls.lock
>>>>>>>>>>+ * Return:
>>>>>>>>>>+ * * 0 - OK
>>>>>>>>>>+ * * negative - error
>>>>>>>>>>+ */
>>>>>>>>>>+static int
>>>>>>>>>>+ice_dpll_pin_enable(struct ice_hw *hw, struct ice_dpll_pin *pin,
>>>>>>>>>>+		    enum ice_dpll_pin_type pin_type,
>>>>>>>>>>+		    struct netlink_ext_ack *extack)
>>>>>>>>>>+{
>>>>>>>>>>+	u8 flags = 0;
>>>>>>>>>>+	int ret;
>>>>>>>>>>+
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>I don't follow. Howcome you don't check if the mode is freerun here or
>>>>>>>>>not? Is it valid to enable a pin when freerun mode? What happens?
>>>>>>>>>
>>>>>>>>
>>>>>>>>Because you are probably still thinking the modes are somehow connected
>>>>>>>>to the state of the pin, but it is the other way around.
>>>>>>>>The dpll device mode is a state of DPLL before pins are even
>>>>>>>>considered.
>>>>>>>>If the dpll is in mode FREERUN, it shall not try to synchronize or
>>>>>>>>monitor
>>>>>>>>any of the pins.
>>>>>>>>
>>>>>>>>>Also, I am probably slow, but I still don't see anywhere in this
>>>>>>>>>patchset any description about why we need the freerun mode. What is
>>>>>>>>>diffrerent between:
>>>>>>>>>1) freerun mode
>>>>>>>>>2) automatic mode & all pins disabled?
>>>>>>>>
>>>>>>>>The difference:
>>>>>>>>Case I:
>>>>>>>>1. set dpll to FREERUN and configure the source as if it would be in
>>>>>>>>AUTOMATIC
>>>>>>>>2. switch to AUTOMATIC
>>>>>>>>3. connecting to the valid source takes ~50 seconds
>>>>>>>>
>>>>>>>>Case II:
>>>>>>>>1. set dpll to AUTOMATIC, set all the source to disconnected
>>>>>>>>2. switch one valid source to SELECTABLE
>>>>>>>>3. connecting to the valid source takes ~10 seconds
>>>>>>>>
>>>>>>>>Basically in AUTOMATIC mode the sources are still monitored even when
>>>>>>>>they
>>>>>>>>are not in SELECTABLE state, while in FREERUN there is no such
>>>>>>>>monitoring,
>>>>>>>>so in the end process of synchronizing with the source takes much
>>>>>>>>longer as
>>>>>>>>dpll need to start the process from scratch.
>>>>>>>
>>>>>>>I believe this is implementation detail of your HW. How you do it is up
>>>>>>>to you. User does not have any visibility to this behaviour, therefore
>>>>>>>makes no sense to expose UAPI that is considering it. Please drop it at
>>>>>>>least for the initial patchset version. If you really need it later on
>>>>>>>(which I honestly doubt), you can send it as a follow-up patchset.
>>>>>>>
>>>>>>
>>>>>>And we will have the same discussion later.. But implementation is
>>>>>>already
>>>>>>there.
>>>>>
>>>>>Yeah, it wouldn't block the initial submission. I would like to see this
>>>>>merged, so anything which is blocking us and is totally optional (as
>>>>>this freerun mode) is better to be dropped.
>>>>>
>>>>
>>>>It is not blocking anything. Most of it was defined and available for
>>>>long time already. Only ice implementing set_mode is a new part.
>>>>No clue what is the problem you are implying here.
>>>
>>>Problem is that I believe you freerun mode should not exist. I believe
>>>it is wrong.
>>>
>>>
>>>>
>>>>>
>>>>>>As said in our previous discussion, without mode_set there is no point to
>>>>>>have
>>>>>>command DEVICE_SET at all, and there you said that you are ok with having
>>>>>>the
>>>>>>command as a placeholder, which doesn't make sense, since it is not used.
>>>>>
>>>>>I don't see any problem in having enum value reserved. But it does not
>>>>>need to be there at all. You can add it to the end of the list when
>>>>>needed. No problem. This is not an argument.
>>>>>
>>>>
>>>>The argument is that I already implemented and tested, and have the need
>>>>for the
>>>>existence to set_mode to configure DPLL, which is there to switch the mode
>>>>between AUTOMATIC and FREERUN.
>>>>
>>>>>
>>>>>>
>>>>>>Also this is not HW implementation detail but a synchronizer chip
>>>>>>feature,
>>>>>>once dpll is in FREERUN mode, the measurements like phase offset between
>>>>>>the
>>>>>>input and dpll's output won't be available.
>>>>>>
>>>>>>For the user there is a difference..
>>>>>>Enabling the FREERUN mode is a reset button on the dpll's state machine,
>>>>>>where disconnecting sources is not, as they are still used, monitored and
>>>>>>measured.
>>>>>
>>>>>So it is not a mode! Mode is either "automatic" or "manual". Then we
>>>>>have a state to indicate the state of the state machine (unlocked, locked,
>>>>>holdover, holdover-acq). So what you seek is a way for the user to
>>>>>expliticly set the state to "unlocked" and reset of the state machine.
>>>>>
>>>>>Please don't mix config and state. I think we untangled this in the past
>>>>>:/
>>>>
>>>>I don't mix anything, this is the way dpll works, which means mode of dpll.
>>>
>>>You do. You want to force-change the state yet you mangle the mode in.
>>>The fact that some specific dpll implemented it as mode does not mean it
>>>has to be exposed like that to user. We have to find the right
>>>abstraction.
>>>
>>
>>Just to make it clear:
>>
>>AUTOMATIC:
>>- inputs monitored, validated, phase measurements available
>>- possible states: unlocked, locked, locked-ho-acq, holdover
>>
>>FREERUN:
>>- inputs not monitored, not validated, no phase measurements available
>>- possible states: unlocked
>
>This is your implementation of DPLL. Others may have it done
>differently. But the fact the input is monitored or not, does not make
>any difference from user perspective.
>
>When he has automatic mode and does:
>1) disconnect all pins
>2) reset state    (however you implement it in the driver is totaly up
>		   to the device, you may go to your freerun dpll mode
>		   internally and to automatic back, up to you)
> -> state will go to unlocked
>
>The behaviour is exactly the same, without any special mode.

In this case there is special reset button, which doesn't exist in
reality, actually your suggestion to go into FREERUN and back to AUTOMATIC
to pretend the some kind of reset has happened, where in reality dpll went to
FREERUN and AUTOMATIC.
For me it seems it seems like unnecessary complication of user's life.
The idea of FREERUN mode is to run dpll on its system clock, so all the
"external" dpll sources shall be disconnected when dpll is in FREERUN.
Let's assume your HW doesn't have a FREERUN, can't you just create it by
disconnecting all the sources? 
BTW, what chip are you using on mlx5 for this?
I don't understand why the user would have to mangle state of all the pins just
to stop dpll's work if he could just go into FREERUN and voila. Also what if
user doesn't want change the configuration of the pins at all, and he just want
to desynchronize it's dpll for i.e. testing reason.

>
>We are talking about UAPI here. It should provide the abstraction, leaving
>the
>internal implementation behind the curtain. What is important is:
>1) clear configuration knobs
>2) the outcome (hw behaviour)
>
>
>
>>
>>>
>>>>
>>>>>
>>>>>Perhaps you just need an extra cmd like DPLL_CMD_DEVICE_STATE_RESET cmd
>>>>>to hit this button.
>>>>>
>>>>
>>>>As already said there are measurement in place in AUTOMATIC, there are no
>>>>such
>>>>thing in FREERUN. Going into FREERUN resets the state machine of dpll
>>>>which
>>>>is a side effect of going to FREERUN.
>>>>
>>>>>
>>>>>
>>>>>>So probably most important fact that you are missing here: assuming the
>>>>>>user
>>>>>>disconnects the pin that dpll was locked with, our dpll doesn't go into
>>>>>>UNLOCKED
>>>>>>state but into HOLDOVER.
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>Isn't the behaviour of 1) and 2) exactly the same? If no, why? This
>>>>>>>>>needs to be documented, please.
>>>>>>>>>
>>>>>>>>
>>>>>>>>Sure will add the description of FREERUN to the docs.
>>>>>>>
>>>>>>>No, please drop it from this patchset. I have no clue why you readded
>>>>>>>it in the first place in the last patchset version.
>>>>>>>
>>>>>>
>>>>>>mode_set was there from the very beginning.. now implemented in ice
>>>>>>driver
>>>>>>as it should.
>>>>>
>>>>>I don't understand the fixation on a callback to be implemented. Just
>>>>>remove it. It can be easily added when needed. No problem.
>>>>>
>>>>
>>>>Well, I don't understand the fixation about removing it.
>>>
>>>It is needed only for your freerun mode, which is questionable. This
>>>discussion it not about mode_set. I don't care about it, if it is
>>>needed, should be there, if not, so be it.
>>>
>>>As you say, you need existance of your freerun mode to justify existence
>>>of mode_set(). Could you please, please drop both for now so we can
>>>move on? I'm tired of this. Thanks!
>>>
>>
>>Reason for dpll subsystem is to control the dpll. So the mode_set and
>>different modes are there for the same reason.
>>Explained this multiple times already, we need a way to let the user switch
>>to FREERUN, so all the activities on dpll are stopped.
>>
>>>
>>>>set_mode was there for a long time, now the callback is properly
>>>>implemented
>>>>and you are trying to imply that this is not needed.
>>>>We require it, as there is no other other way to stop AUTOMATIC mode dpll
>>>>to do its work.
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>Another question, I asked the last time as well, but was not heard:
>>>>>>>>>Consider example where you have 2 netdevices, eth0 and eth1, each
>>>>>>>>>connected with a single DPLL pin:
>>>>>>>>>eth0 - DPLL pin 10 (DPLL device id 2)
>>>>>>>>>eth1 - DPLL pin 11 (DPLL device id 2)
>>>>>>>>>
>>>>>>>>>You have a SyncE daemon running on top eth0 and eth1.
>>>>>>>>>
>>>>>>>>>Could you please describe following 2 flows?
>>>>>>>>>
>>>>>>>>>1) SyncE daemon selects eth0 as a source of clock
>>>>>>>>>2) SyncE daemon selects eth1 as a source of clock
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>For mlx5 it goes like:
>>>>>>>>>
>>>>>>>>>DPLL device mode is MANUAL.
>>>>>>>>>1)
>>>>>>>>> SynceE daemon uses RTNetlink to obtain DPLL pin number of eth0
>>>>>>>>>    -> pin_id: 10
>>>>>>>>> SenceE daemon will use PIN_GET with pin_id 10 to get DPLL device id
>>>>>>>>>    -> device_id: 2
>>>>>>>>
>>>>>>>>Not sure if it needs to obtain the dpll id in this step, but it doesn't
>>>>>>>>relate to the dpll interface..
>>>>>>>
>>>>>>>Sure it has to. The PIN_SET accepts pin_id and device_id attrs as input.
>>>>>>>You need to set the state on a pin on a certain DPLL device.
>>>>>>>
>>>>>>
>>>>>>The thing is pin can be connected to multiple dplls and SyncE daemon shall
>>>>>>know already something about the dpll it is managing.
>>>>>>Not saying it is not needed, I am saying this is not a moment the SyncE
>>>>>>daemon
>>>>>>learns it.
>>>>>
>>>>>Moment or not, it is needed for the cmd, that is why I have it there.
>>>>>
>>>>>
>>>>>>But let's park it, as this is not really relevant.
>>>>>
>>>>>Agreed.
>>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>> SynceE daemon does PIN_SET cmd on pin_id 10, device_id 2 -> state =
>>>>>>>>>CONNECTED
>>>>>>>>>
>>>>>>>>>2)
>>>>>>>>> SynceE daemon uses RTNetlink to obtain DPLL pin number of eth1
>>>>>>>>>    -> pin_id: 11
>>>>>>>>> SenceE daemon will use PIN_GET with pin_id 11 to get DPLL device id
>>>>>>>>>    -> device_id: 2
>>>>>>>>> SynceE daemon does PIN_SET cmd on pin_id 10, device_id 2 -> state =
>>>>>>>>>CONNECTED
>>>>>>>>> (that will in HW disconnect previously connected pin 10, there
>>>>>>>>>will be
>>>>>>>>>  notification of pin_id 10, device_id -> state DISCONNECT)
>>>>>>>>>
>>>>>>>>
>>>>>>>>This flow is similar for ice, but there are some differences, although
>>>>>>>>they come from the fact, the ice is using AUTOMATIC mode and recovered
>>>>>>>>clock pins which are not directly connected to a dpll (connected
>>>>>>>>through
>>>>>>>>the MUX pin).
>>>>>>>>
>>>>>>>>1)
>>>>>>>>a) SyncE daemon uses RTNetlink to obtain DPLL pin number of eth0 ->
>>>>>>>>pin_id: 13
>>>>>>>>b) SyncE daemon uses PIN_GET to find a parent MUX type pin -> pin_id: 2
>>>>>>>>   (in case of dpll_id is needed, would be find in this response also)
>>>>>>>>c) SyncE daemon uses PIN_SET to set parent MUX type pin (pin_id: 2) to
>>>>>>>>   pin-state: SELECTABLE and highest priority (i.e. pin-prio:0, while
>>>>>>>>all the
>>>>>>>>   other pins shall be lower prio i.e. pin-prio:1)
>>>>>>>
>>>>>>>Yeah, for this you need pin_id 2 and device_id. Because you are setting
>>>>>>>state on DPLL device.
>>>>>>>
>>>>>>>
>>>>>>>>d) SyncE daemon uses PIN_SET to set state of pin_id:13 to CONNECTED
>>>>>>>>with
>>>>>>>>   parent pin (pin-id:2)
>>>>>>>
>>>>>>>For this you need pin_id and pin_parent_id because you set the state on
>>>>>>>a parent pin.
>>>>>>>
>>>>>>>
>>>>>>>Yeah, this is exactly why I initially was in favour of hiding all the
>>>>>>>muxes and magic around it hidden from the user. Now every userspace app
>>>>>>>working with this has to implement a logic of tracking pin and the mux
>>>>>>>parents (possibly multiple levels) and configure everything. But it
>>>>>>>just
>>>>>>>need a simple thing: "select this pin as a source" :/
>>>>>>>
>>>>>>>
>>>>>>>Jakub, isn't this sort of unnecessary HW-details complexicity exposure
>>>>>>>in UAPI you were against in the past? Am I missing something?
>>>>>>>
>>>>>>
>>>>>>Multiple level of muxes possibly could be hidden in the driver, but the
>>>>>>fact
>>>>>>they exist is not possible to be hidden from the user if the DPLL is in
>>>>>>AUTOMATIC mode.
>>>>>>For MANUAL mode dpll the muxes could be also hidden.
>>>>>>Yeah, we have in ice most complicated scenario of AUTOMATIC mode + MUXED
>>>>>>type
>>>>>>pin.
>>>>>
>>>>>Sure, but does user care how complicated things are inside? The syncE
>>>>>daemon just cares for: "select netdev x as a source". However it is done
>>>>>internally is irrelevant to him. With the existing UAPI, the syncE
>>>>>daemon needs to learn individual device dpll/pin/mux topology and
>>>>>work with it.
>>>>>
>>>>
>>>>This is dpll subsystem not SyncE one.
>>>
>>>SyncE is very legit use case of the UAPI. I would say perhaps the most
>>>important.
>>>
>>
>>But it is still a dpll subsystem.
>>
>>Thank you!
>>Arkadiusz
>>
>>>
>>>>
>>>>>Do we need a dpll library to do this magic?
>>>>>
>>>>
>>>>IMHO rather SyncE library :)
>>>>
>>>>Thank you!
>>>>Arkadiusz
>>>>
>>>>>
>>>>>>
>>>>>>Thank you!
>>>>>>Arkadiusz
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>2) (basically the same, only eth1 would get different pin_id.)
>>>>>>>>a) SyncE daemon uses RTNetlink to obtain DPLL pin number of eth0 ->
>>>>>>>>pin_id: 14
>>>>>>>>b) SyncE daemon uses PIN_GET to find parent MUX type pin -> pin_id: 2
>>>>>>>>c) SyncE daemon uses PIN_SET to set parent MUX type pin (pin_id: 2) to
>>>>>>>>   pin-state: SELECTABLE and highest priority (i.e. pin-prio:0, while
>>>>>>>>all the
>>>>>>>>   other pins shall be lower prio i.e. pin-prio:1)
>>>>>>>>d) SyncE daemon uses PIN_SET to set state of pin_id:14 to CONNECTED
>>>>>>>>with
>>>>>>>>   parent pin (pin-id:2)
>>>>>>>>
>>>>>>>>Where step c) is required due to AUTOMATIC mode, and step d) required
>>>>>>>>due to
>>>>>>>>phy recovery clock pin being connected through the MUX type pin.
>>>>>>>>
>>>>>>>>Thank you!
>>>>>>>>Arkadiusz
>>>>>>>>
>>>>>>>>>
>>>>>>>>>Thanks!
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>[...]
>>>>>>

Powered by blists - more mailing lists