lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <DM6PR11MB4657468307AD658AD2A763FB9BB79@DM6PR11MB4657.namprd11.prod.outlook.com>
Date:   Tue, 7 Mar 2023 12:23:27 +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>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>,
        "Olech, Milena" <milena.olech@...el.com>,
        "Michalik, Michal" <michal.michalik@...el.com>
Subject: RE: [RFC PATCH v5 1/4] dpll: Add DPLL framework base functions

>From: Jiri Pirko <jiri@...nulli.us>
>Sent: Tuesday, February 7, 2023 3:15 PM
>
>Mon, Feb 06, 2023 at 03:00:09AM CET, arkadiusz.kubalewski@...el.com wrote:
>>>From: Jiri Pirko <jiri@...nulli.us>
>>>Sent: Tuesday, January 31, 2023 3:01 PM
>>>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>; netdev@...r.kernel.org;
>>>linux-arm-kernel@...ts.infradead.org; linux- clk@...r.kernel.org;
>>>Olech, Milena <milena.olech@...el.com>; Michalik, Michal
>>><michal.michalik@...el.com>
>>>Subject: Re: [RFC PATCH v5 1/4] dpll: Add DPLL framework base
>>>functions
>>>
>>>Fri, Jan 27, 2023 at 07:12:41PM CET, arkadiusz.kubalewski@...el.com
>>wrote:
>>>>>From: Jiri Pirko <jiri@...nulli.us>
>>>>>Sent: Thursday, January 19, 2023 6:16 PM
>>>>>
>>>>>Tue, Jan 17, 2023 at 07:00:48PM CET, vadfed@...a.com wrote:
>
>[...]
>
>
>>>>>>+			 struct dpll_pin_ops *ops, void *priv) {
>>>>>>+	struct dpll_pin *pin;
>>>>>>+	int ret;
>>>>>>+
>>>>>>+	mutex_lock(&dpll_pin_owner->lock);
>>>>>>+	pin = dpll_pin_get_by_description(dpll_pin_owner,
>>>>>>+					  shared_pin_description);
>>>>>>+	if (!pin) {
>>>>>>+		ret = -EINVAL;
>>>>>>+		goto unlock;
>>>>>>+	}
>>>>>>+	ret = dpll_pin_register(dpll, pin, ops, priv);
>>>>>>+unlock:
>>>>>>+	mutex_unlock(&dpll_pin_owner->lock);
>>>>>>+
>>>>>>+	return ret;
>>>>>
>>>>>I don't understand why there should be a separate function to
>>>>>register the shared pin. As I see it, there is a pin object that
>>>>>could be registered with 2 or more dpll devices. What about having:
>>>>>
>>>>>pin = dpll_pin_alloc(desc, type, ops, priv)
>>>>>dpll_pin_register(dpll_1, pin); dpll_pin_register(dpll_2, pin);
>>>>>dpll_pin_register(dpll_3, pin);
>>>>>
>>>>
>>>>IMHO your example works already, but it would possible only if the
>>>>same driver instance initializes all dplls.
>>>
>>>It should be only one instance of dpll to be shared between driver
>>>instances as I wrote in the reply to the "ice" part. There might he
>>>some pins created alongside with this.
>>>
>>
>>pin = dpll_pin_alloc(desc, type, ops, priv) dpll_pin_register(dpll_1,
>>pin); dpll_pin_register(dpll_2, pin); dpll_pin_register(dpll_3, pin); ^
>>there is registration of a single pin by a 3 dpll instances, and a
>>kernel module instance which registers them has a reference to the pin
>>and all dplls, thus it can just register them all without any problems,
>>don't need to call dpll_shared_pin_register(..).
>>
>>Now imagine 2 kernel module instances.
>>One (#1) creates one dpll, registers pins with it.
>>Second (#2) creates second dpll, and want to use/register pins of dpll
>>registered by the first instance (#1).
>
>Sure, both instances should be available to both module instances, using
>the suggested get/put create/reference system.
>Whichever module instance does register shared pin can use
>dpll_pin_register(), I see no problem with that.
>

In v6 those suggestions are implemented.
AFAIK Vadim shall send it soon.

>
>>
>>>My point is, the first driver instance which creates dpll registers
>>>also the pins. The other driver instance does not do anything, just
>>>gets reference to the dpll.
>>>
>>>On cleanup path, the last driver instance tearing down would
>>>unregister dpll pins (Could be done automatically by dpll_device_put()).
>>>
>>>There might be some other pins (Synce) created per driver instance
>>>(per-PF). You have to distinguish these 2 groups.
>>>
>>>
>>>>dpll_shared_pin_register is designed for driver instances without the
>>>>pin
>>>
>>>I think we need to make sure the terms are correct "sharing" is
>>>between multiple dpll instances. However, if 2 driver instances are
>>>sharing the same dpll instance, this instance has pins. There is no
>>>sharing unless there is another dpll instance in picture. Correct?
>>>
>>
>>Yes!
>>If two kernel module intances sharing a dpll instance, the pins belong
>>to the dpll instance, and yes each kernel module instance can register
>>pins with that dpll instance just with: dpll_pin_register(dpll_1, pin);
>>
>>dpll_shared_pin_register(..) shall be used when separated kernel module
>>instances are initializing separated dpll instances, and those
>>instances are
>
>Why exacly would they do that? Could you please draw me an example?
>

I think we shall not follow this discussion as in v6 we already
have the mechanics you suggested, but sure:
+----------+                 
|i0 - GPS  |--------------\
+----------+              |
+----------+              |
|i1 - SMA1 |------------\ |
+----------+            | |
+----------+            | |
|i2 - SMA2 |----------\ | |
+----------+          | | |
                      | | |
+---------------------|-|-|-------------------------------------------+
| Channel A / FW0     | | |     +-------------+   +---+   +--------+  |
|                     | | |-i0--|Synchronizer0|---|   |---| PHY0.0 |--|
|         +---+       | | |     |             |   |   |   +--------+  |
| PHY0.0--|   |       | |---i1--|             |---| M |---| PHY0.1 |--|
|         |   |       | | |     | +-----+     |   | A |   +--------+  |
| PHY0.1--| M |       |-----i2--| |DPLL0|     |   | C |---| PHY0.2 |--|
|         | U |       | | |     | +-----+     |   | 0 |   +--------+  |
| PHY0.2--| X |--+----------i3--| +-----+     |---|   |---| ...    |--|
|         | 0 |  |    | | |     | |DPLL1|     |   |   |   +--------+  |
| ...   --|   |  | /--------i4--| +-----+     |---|   |---| PHY0.7 |--|
|         |   |  | |  | | |     +-------------+   +---+   +--------+  |
| PHY0.7--|   |  | |  | | |                                           |
|         +---+  | |  | | |                                           |
+----------------|-|--|-|-|-------------------------------------------+
| Channel B / FW1| |  | | |     +-------------+   +---+   +--------+  |
|                | |  | | \-i0--|Synchronizer1|---|   |---| PHY1.0 |--|
|         +---+  | |  | |       |             |   |   |   +--------+  |
| PHY1.0--|   |  | |  | \---i1--|             |---| M |---| PHY1.1 |--|
|         |   |  | |  |         | +-----+     |   | A |   +--------+  |
| PHY1.1--| M |  | |  \-----i2--| |DPLL0|     |   | C |---| PHY1.2 |--|
|         | U |  | |            | +-----+     |   | 1 |   +--------+  |
| PHY1.2--| X |  \-|--------i3--| +-----+     |---|   |---| ...    |--|
|         | 1 |    |            | |DPLL1|     |   |   |   +--------+  |
| ...   --|   |----+--------i4--| +-----+     |---|   |---| PHY1.7 |--|
|         |   |                 +-------------+   +---+   +--------+  |
| PHY1.7--|   |                                                       |
|         +---+                                                       |
+---------------------------------------------------------------------+

>
>>physically sharing their pins.
>>
>>>
>
>[...]
>
>
>>>>>>+static int dpll_msg_add_pin_modes(struct sk_buff *msg,
>>>>>>+				   const struct dpll_device *dpll,
>>>>>>+				   const struct dpll_pin *pin) {
>>>>>>+	enum dpll_pin_mode i;
>>>>>>+	bool active;
>>>>>>+
>>>>>>+	for (i = DPLL_PIN_MODE_UNSPEC + 1; i <= DPLL_PIN_MODE_MAX; i++) {
>>>>>>+		if (dpll_pin_mode_active(dpll, pin, i, &active))
>>>>>>+			return 0;
>>>>>>+		if (active)
>>>>>>+			if (nla_put_s32(msg, DPLLA_PIN_MODE, i))
>>>>>
>>>>>Why this is signed?
>>>>>
>>>>
>>>>Because enums are signed.
>>>
>>>You use negative values in enums? Don't do that here. Have all netlink
>>>atrributes unsigned please.
>>>
>>
>>No, we don't use negative values, but enum is a signed type by itself.
>>Doesn't seem right thing to do, put signed-type value into unsigned type TLV.
>>This smells very bad.
>
>Well, then all existing uses that carry enum over netlink attributes smell
>bad. The enum values are all unsigned, I see no reason to use S*.
>Please be consistent with the rest of the Netlink uAPI.
>

Yes, exactly, don't know why to follow bad practicies, saying "that's how it's
done". Is there any reasoning behind this?

>
>[...]
>
>>>>>>+
>>>>>>+/* dpll_pin_signal_type - signal types
>>>>>>+ *
>>>>>>+ * @DPLL_PIN_SIGNAL_TYPE_UNSPEC - unspecified value
>>>>>>+ * @DPLL_PIN_SIGNAL_TYPE_1_PPS - a 1Hz signal
>>>>>>+ * @DPLL_PIN_SIGNAL_TYPE_10_MHZ - a 10 MHz signal
>>>>>
>>>>>Why we need to have 1HZ and 10MHZ hardcoded as enums? Why can't we
>>>>>work with HZ value directly? For example, supported freq:
>>>>>1, 10000000
>>>>>or:
>>>>>1, 1000
>>>>>
>>>>>freq set 10000000
>>>>>freq set 1
>>>>>
>>>>>Simple and easy.
>>>>>
>>>>
>>>>AFAIR, we wanted to have most commonly used frequencies as enums +
>>>>custom_freq for some exotic ones (please note that there is also
>>>>possible 2PPS, which is
>>>>0.5 Hz).
>>>
>>>In this exotic case, user might add divider netlink attribute to
>>>divide the frequency pass in the attr. No problem.
>>>
>>>
>>>>This was design decision we already agreed on.
>>>>The userspace shall get definite list of comonly used frequencies
>>>>that can be used with given HW, it clearly enums are good for this.
>>>
>>>I don't see why. Each instance supports a set of frequencies. It would
>>>pass the values to the userspace.
>>>
>>>I fail to see the need to have some fixed values listed in enums.
>>>Mixing approaches for a single attribute is wrong. In ethtool we also
>>>don't have enum values for 10,100,1000mbits etc. It's just a number.
>>>
>>
>>In ethtool there are defines for linkspeeds.
>>There must be list of defines/enums to check the driver if it is supported.
>>Especially for ANY_FREQ we don't want to call driver 25 milions times or
>>more.
>
>Any is not really *any* is it? A simple range wouldn't do then? It would be
>much better to tell the user the boundaries.
>

In v6 those suggestions are implemented.

>
>>
>>Also, we have to move supported frequencies to the dpll_pin_alloc as it
>>is constant argument, supported frequencies shall not change @ runtime?
>>In such case there seems to be only one way to pass in a nice way, as a
>>bitmask?
>
>array of numbers (perhaps using defines for most common values), I don't
>see any problem in that. But you are talking about in-kernel API. Does not
>matter that much. What we are discussing is uAPI and that matters a lot.
>
>
>>
>>Back to the userspace part, do you suggest to have DPLLA_PIN_FREQ
>>attribute and translate kernelspace enum values to userspace defines
>>like DPLL_FREQ_1_HZ, etc? also with special define for supported ones
>>ANY_FREQ?
>
>Whichever is convenient. My focus here is uAPI.
>

In v6 those suggestions are implemented.

>
>>
>>>
>>>>
>>>>>
>>>>>>+ * @DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ - custom frequency signal,
>>>>>>+ value
>>>>>>defined
>>>>>>+ *	with pin's DPLLA_PIN_SIGNAL_TYPE_CUSTOM_FREQ attribute
>>>>>>+ **/
>>>>>>+enum dpll_pin_signal_type {
>>>>>>+	DPLL_PIN_SIGNAL_TYPE_UNSPEC,
>>>>>>+	DPLL_PIN_SIGNAL_TYPE_1_PPS,
>>>>>>+	DPLL_PIN_SIGNAL_TYPE_10_MHZ,
>>>>>>+	DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ,
>>>>>>+
>>>>>>+	__DPLL_PIN_SIGNAL_TYPE_MAX,
>>>>>>+};
>>>>>>+
>>>>>>+#define DPLL_PIN_SIGNAL_TYPE_MAX (__DPLL_PIN_SIGNAL_TYPE_MAX - 1)
>>>>>>+
>>>>>>+/* dpll_pin_mode - available pin states
>>>>>>+ *
>>>>>>+ * @DPLL_PIN_MODE_UNSPEC - unspecified value
>>>>>>+ * @DPLL_PIN_MODE_CONNECTED - pin connected
>>>>>>+ * @DPLL_PIN_MODE_DISCONNECTED - pin disconnected
>>>>>>+ * @DPLL_PIN_MODE_SOURCE - pin used as an input pin
>>>>>>+ * @DPLL_PIN_MODE_OUTPUT - pin used as an output pin  **/ enum
>>>>>>+dpll_pin_mode {
>>>>>>+	DPLL_PIN_MODE_UNSPEC,
>>>>>>+	DPLL_PIN_MODE_CONNECTED,
>>>>>>+	DPLL_PIN_MODE_DISCONNECTED,
>>>>>>+	DPLL_PIN_MODE_SOURCE,
>>>>>>+	DPLL_PIN_MODE_OUTPUT,
>>>>>
>>>>>I don't follow. I see 2 enums:
>>>>>CONNECTED/DISCONNECTED
>>>>>SOURCE/OUTPUT
>>>>>why this is mangled together? How is it supposed to be working. Like
>>>>>a bitarray?
>>>>>
>>>>
>>>>The userspace shouldn't worry about bits, it recieves a list of
>>>attributes.
>>>>For current/active mode: DPLLA_PIN_MODE, and for supported modes:
>>>>DPLLA_PIN_MODE_SUPPORTED. I.e.
>>>>
>>>>	DPLLA_PIN_IDX			0
>>>>	DPLLA_PIN_MODE			1,3
>>>>	DPLLA_PIN_MODE_SUPPORTED	1,2,3,4
>>>
>>>I believe that mixing apples and oranges in a single attr is not correct.
>>>Could you please split to separate attrs as drafted below?
>>>
>>>>
>>>>The reason for existance of both DPLL_PIN_MODE_CONNECTED and
>>>>DPLL_PIN_MODE_DISCONNECTED, is that the user must request it somehow,
>>>>and bitmask is not a way to go for userspace.
>>>
>>>What? See nla_bitmap.
>>>
>>
>>AFAIK, nla_bitmap is not yet merged.
>
>NLA_BITFIELD32
>
>
>>
>>>Anyway, why can't you have:
>>>DPLLA_PIN_CONNECTED     u8 1/0 (bool)
>>>DPLLA_PIN_DIRECTION     enum { SOURCE/OUTPUT }
>>
>>Don't get it, why this shall be u8 with bool value, doesn't make much
>>sense for userspace.
>
>Could be NLA_FLAG.
>
>
>>All the other attributes have enum type, we can go with separated
>>attribute:
>>DPLLA_PIN_STATE		enum { CONNECTED/DISCONNECTED }
>
>Yeah, why not. I think this is probably better and more explicit than
>NLA_FLAG.
>
>
>>Just be consistent and clear, and yes u8 is enough it to keep it, as
>>well as all of attribute enum values, so we can use u8 instead of u32 for
>>all of them.
>
>Yes, that is what is done normally for attrs like this.
>
>

In v6, there are enums and attributes:
DPLL_A_PIN_STATE	enum { CONNECTED/DISCONNECTED }
DPLL_A_PIN_DIRECTION	enum { SOURCE/OUTPUT }

also new capabilities attributes DPLL_A_PIN_DPLL_CAPS
a bitmap - implicit from u32 value.

>>
>>Actually for "connected/disconnected"-part there are 2 valid use-cases
>>on my
>>mind:
>>- pin can be connected with a number of "parents" (dplls or muxed-pins)
>>- pin is disconnected entirely
>>Second case can be achieved with control over first one, thus not need
>>for any special approach here. Proper control would be to let userspace
>>connect or disconnect a pin per each node it can be connected with, right?
>>
>>Then example dump of "get-pins" could look like this:
>>DPLL_PIN	(nested)
>>	DPLLA_PIN_IDX		0
>>	DPLLA_PIN_TYPE		DPLL_PIN_TYPE_EXT
>>	DPLLA_PIN_DIRECTION	SOURCE
>>	...
>>	DPLLA_DPLL			(nested)
>>		DPLLA_ID		0
>>		DPLLA_NAME		pci_0000:00:00.0
>
>Nit, make sure you have this as 2 attrs, busname, devname.

Sure.

>
>
>>		DPLLA_PIN_STATE		CONNECTED
>>	DPLLA_DPLL			(nested)
>>		DPLLA_ID		1
>>		DPLLA_NAME		pci_0000:00:00.0
>>		DPLLA_PIN_STATE		DISCONNECTED
>>
>>DPLL_PIN	(nested)
>>	DPLLA_PIN_IDX		1
>>	DPLLA_PIN_TYPE		DPLL_PIN_TYPE_MUX
>>	DPLLA_PIN_DIRECTION	SOURCE
>>	...
>>	DPLLA_DPLL			(nested)
>>		DPLLA_ID		0
>>		DPLLA_NAME		pci_0000:00:00.0
>>		DPLLA_PIN_STATE		DISCONNECTED
>>	DPLLA_DPLL			(nested)
>>		DPLLA_ID		1
>>		DPLLA_NAME		pci_0000:00:00.0
>>		DPLLA_PIN_STATE		CONNECTED
>>
>>DPLL_PIN	(nested)
>>	DPLLA_PIN_IDX		2
>>	DPLLA_PIN_TYPE		DPLL_PIN_TYPE_MUX
>>	DPLLA_PIN_DIRECTION	SOURCE
>>	...
>>	DPLLA_DPLL			(nested)
>>		DPLLA_ID		0
>>		DPLLA_NAME		pci_0000:00:00.0
>>		DPLLA_PIN_STATE		DISCONNECTED
>>	DPLLA_DPLL			(nested)
>>		DPLLA_ID		1
>>		DPLLA_NAME		pci_0000:00:00.0
>>		DPLLA_PIN_STATE		DISCONNECTED
>
>Okay.
>
>
>>
>>(similar for muxed pins)
>>DPLL_PIN	(nested)
>>	DPLLA_PIN_IDX		3
>>	DPLLA_PIN_TYPE		DPLL_PIN_TYPE_SYNCE_ETH_PORT
>>	DPLLA_PIN_DIRECTION	SOURCE
>>	DPLLA_PIN_PARENT		(nested)
>>		DPLLA_PIN_IDX		1
>>		DPLLA_PIN_STATE		DISCONNECTED
>>	DPLLA_PIN_PARENT		(nested)
>>		DPLLA_PIN_IDX		2
>>		DPLLA_PIN_STATE		CONNECTED
>>
>>DPLL_PIN	(nested)
>>	DPLLA_PIN_IDX		4
>>	DPLLA_PIN_TYPE		DPLL_PIN_TYPE_SYNCE_ETH_PORT
>>	DPLLA_PIN_DIRECTION	SOURCE
>>	DPLLA_PIN_PARENT		(nested)
>>		DPLLA_PIN_IDX		1
>>		DPLLA_PIN_STATE		CONNECTED
>>	DPLLA_PIN_PARENT		(nested)
>>		DPLLA_PIN_IDX		2
>>		DPLLA_PIN_STATE		DISCONNECTED
>
>Looks fine.
>
>
>>
>>For DPLL_MODE_MANUAL a DPLLA_PIN_STATE would serve also as signal
>>selector mechanism.
>
>Yep, I already make this point in earlier rfc review comment.
>

Thanks for that :)

>
>>In above example DPLL_ID=0 has only "connected" DPLL_PIN_IDX=0, now
>>when different pin "connect" is requested:
>>
>>dpll-set request:
>>DPLLA_DPLL	(nested)
>>	DPLLA_ID=0
>>	DPLLA_NAME=pci_0000:00:00.0
>>DPLLA_PIN
>>	DPLLA_PIN_IDX=2
>>	DPLLA_PIN_CONNECTED=1
>>
>>Former shall "disconnect"..
>>And now, dump pin-get:
>>DPLL_PIN	(nested)
>>	DPLLA_PIN_IDX		0
>>	...
>>	DPLLA_DPLL			(nested)
>>		DPLLA_ID		0
>>		DPLLA_NAME		pci_0000:00:00.0
>>		DPLLA_PIN_STATE		DISCONNECTED
>>...
>>DPLL_PIN	(nested)
>>	DPLLA_PIN_IDX		2
>>	...
>>	DPLLA_DPLL			(nested)
>>		DPLLA_ID		0
>>		DPLLA_NAME		pci_0000:00:00.0
>>		DPLLA_PIN_STATE		CONNECTED
>>
>>At least that shall happen on hardware level, right?
>>
>>As I can't find a use-case to have a pin "connected" but not "selected"
>>in case of DPLL_MODE_MANUAL.
>
>Exactly.
>
>
>>
>>A bit different is with DPLL_MODE_AUTOMATIC, the pins that connects
>>with dpll directly could be all connected, and their selection is
>>auto-controlled with a DPLLA_PIN_PRIO.
>>But still the user may also request to disconnect a pin - not use it at
>>all (instead of configuring lowest priority - which allows to use it,
>>if all other pins propagate invalid signal).
>>
>>Thus, for DPLL_MODE_AUTOMATIC all ablove is the same with a one
>>difference, each pin/dpll pair would have a prio, like suggested in the
>other email.
>>DPLLA_PIN	(nested)
>>	...
>>	DPLLA_DPLL	(nested)
>>		...
>>		DPLLA_PIN_CONNECTED	<connected value>
>>		DPLLA_PIN_STATE		<prio value>
>
>I think you made a mistake. Should it be:
>		DPLLA_PIN_STATE		<connected value>
>		DPLLA_PIN_PRIO		<prio value>
>?
>

Yes, exactly.

>
>>
>>Which basically means that both DPLL_A_PIN_PRIO and DPLLA_PIN_STATE
>>shall be a property of a PIN-DPLL pair, and configured as such.
>
>Yes.
>
>
>>
>>
>>>DPLLA_PIN_CAPS          nla_bitfield(CAN_CHANGE_CONNECTED,
>>>CAN_CHANGE_DIRECTION)
>>>
>>>We can use the capabilitis bitfield eventually for other purposes as
>>>well, it is going to be handy I'm sure.
>>>
>>
>>Well, in general I like the idea, altough the details...
>>We have 3 configuration levels:
>>- DPLL
>>- DPLL/PIN
>>- PIN
>>
>>Considering that, there is space for 3 of such CAPABILITIES attributes, but:
>>- DPLL can only configure MODE for now, so we can only convert
>>DPLL_A_MODE_SUPPORTED to a bitfield, and add DPLL_CAPS later if
>>required
>
>Can't do that. It's uAPI, once you have ATTR there, it's there for
>eternity...
>

I am not saying to remove something but add in the future.

>
>>- DPLL/PIN pair has configurable DPLLA_PIN_PRIO and DPLLA_PIN_STATE, so
>>we could introduce DPLLA_PIN_DPLL_CAPS for them
>
>Yeah.
>
>
>>- PIN has now configurable frequency (but this is done by providing
>>list of supported ones - no need for extra attribute). We already know
>>that pin shall also have optional features, like phase offset, embedded
>>sync.
>>For embedded sync if supported it shall also be a set of supported
>>frequencies.
>>Possibly for phase offset we could use similar CAPS field, but don't
>>think will manage this into next version.
>>
>>>
>>>
>>>>
>>>>
>>>>>
>>>>>>+
>>>>>>+	__DPLL_PIN_MODE_MAX,
>>>>>>+};
>>>>>>+
>>>
>>>[...]
>>>
>>>
>>>>>>+/**
>>>>>>+ * dpll_mode - Working-modes a dpll can support. Modes
>>>>>>+differentiate
>>>>>>>how
>>>>>>+ * dpll selects one of its sources to syntonize with a source.
>>>>>>+ *
>>>>>>+ * @DPLL_MODE_UNSPEC - invalid
>>>>>>+ * @DPLL_MODE_MANUAL - source can be only selected by sending a
>>>>>>+ request
>>>>>>to dpll
>>>>>>+ * @DPLL_MODE_AUTOMATIC - highest prio, valid source, auto
>>>>>>+ selected by
>>>>>>dpll
>>>>>>+ * @DPLL_MODE_HOLDOVER - dpll forced into holdover mode
>>>>>>+ * @DPLL_MODE_FREERUN - dpll driven on system clk, no holdover
>>>>>>available
>>>>>>+ * @DPLL_MODE_NCO - dpll driven by Numerically Controlled
>>>>>>+ Oscillator
>>>>>
>>>>>Why does the user care which oscilator is run internally. It's
>>>>>freerun, isn't it? If you want to expose oscilator type, you should
>>>>>do it elsewhere.
>>>>>
>>>>
>>>>In NCO user might change frequency of an output, in freerun cannot.
>>>
>>>How this could be done?
>>>
>>
>>I guess by some internal synchronizer frequency dividers. Same as other
>>output (different then input) frequencies are achievable there.
>
>I ment uAPI wise. Speak Netlink.
>

1. DPLL_MODE_NCO is returned with DPLL_A_MODE_SUPPORTED when HW supports it.
2. DPLL_MODE_NCO is requested by the user if user wants control output
frequency or output frequency offset of a dpll.

>From the documentation of ZL80032:
* Numerically controlled oscillator (NCO) behavior allows system software to 
steer DPLL frequency or synthesizer frequency with resolution better than 0.005
ppt
* Similar to freerun mode, but with frequency control. The output clock is the
configured frequency with a frequency offset specified by the dpll_df_offset_x
register. This write-only register changes the output frequency offset of the
DPLL


Thank you,
Arkadiusz

>>
>>Thanks,
>>Arkadiusz
>>
>>>
>>>[...]
>>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ