Message-ID: <DM6PR11MB46576A241EA1519BC559B5C09B00A@DM6PR11MB4657.namprd11.prod.outlook.com>
Date: Wed, 26 Jul 2023 21:11:40 +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: Wednesday, July 26, 2023 8:38 AM
>

[...]
 
>>>>>>
>>>>>>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.
>>>
>>>There are 3 pin states:
>>>disconnected
>>>connected
>>>selectable
>>>
>>>When the last source disconnects, go to your internal freerun.
>>>When some source gets selectable or connected, go to your internal
>>>automatic mode.
>>>
>>
>>This would make the driver to check if all the sources are disconnected
>>each time someone disconnects a source. Which in first place is not
>>efficient, but also dpll design already allows different driver instances
>>to
>>control separated sources, which in this case would force a driver to
>>implement
>>additional communication between the instances just to allow such hidden
>>FREERUN mode.
>>Which seems another argument not to do this in the way you are proposing:
>>inefficient and unnecessarily complicated.
>>
>>We know that you could also implement FREERUN mode by disconnecting all
>>the
>>sources, even if HW doesn't support it explicitly.
>>
>>>>From user perspactive, the mode didn't change.
>>>
>>
>>The user didn't change the mode, the mode shall not change.
>>You wrote to do it silently, so user didn't change the mode but it would
>have
>>changed, and we would have pretended the different working mode of DPLL
>doesn't
>>exist.
>>
>>>>From user perepective, this is exacly the behaviour he requested.
>>>
>>
>>IMHO this is wrong and comes from the definition of pin state DISCONNECTED,
>>which is not sharp, for our HW means that the input will not be considered
>>as valid input, but is not disconnecting anything, as input is still
>>monitored and measured.
>>Shall we have additional mode like PIN_STATE_NOT_SELECTABLE? As it is not
>>possible to actually disconnect a pin..
>>
>>>
>>>>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.
>>>
>>>Yes, that is when you set all pins to disconnect. no mode change needed.
>>>
>>
>>We don't disconnect anything, we used a pin state DISCONNECTED as this
>>seemed
>>most appropriate.
>>
>>>
>>>>Let's assume your HW doesn't have a FREERUN, can't you just create it by
>>>>disconnecting all the sources?
>>>
>>>Yep, that's what we do.
>>>
>>
>>No, you were saying that the mode doesn't exist and that your hardware
>>doesn't
>>support it. At the same time it can be achieved by manually 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.
>>>
>>>I tried to explain multiple times. Let the user have clean an abstracted
>>>api, with clear semantics. Simple as that. Your internal freerun mode is
>>>just something to abstract out, it is not needed to expose it.
>>>
>>
>>Our hardware can support in total 4 modes, and 2 are now supported in ice.
>>I don't get the idea for abstraction of hardware switches, modes or
>>capabilities, and having those somehow achievable through different
>>functionalities.
>>
>>I think we already discussed this long enough to make a decision..
>>Though I am not convinced by your arguments, and you are not convinced by
>>mine.
>>
>>Perhaps someone else could step in and cut the rope, so we could go further
>>with this?
>
>Or, even better, please drop this for the initial patchset and have this
>as a follow-up. Thanks!
>
>

On the responses from Jakub and Paolo, they supported the idea of having
such mode.

Although Jakub have asked if there could be better name then FREERUN, also
suggested DETACHED and STANDALONE.
For me DETACHED seems pretty good, STANDALONE a bit too far..
I am biased by the FREERUN from chip docs and don't have strong opinion
on any of those..

Any suggestions?

Thank you!
Arkadiusz

[...]

Powered by blists - more mailing lists