From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 98009C636CD for ; Tue, 7 Feb 2023 14:16:38 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=2VGTu8UWE4K1BpA/pw8OzrhzC+trwJT/PbqU6JG1abE=; b=BDQrhEhn9LnsVC DQB8kvsNQbrQY10XnxfBcfJK3RchYt0reOWnUaYxj00lRS2Jkl2i8rZ4+mFSDdfJ+t0QUgpjuw1Ok rJUPQH2EUM6tRox5T7sX71QeG5H6beLUJ7FN7AeUv/W9WbM4osHNIwU0FukeJzAJbOsaHJh2brS6r Nnk4+zCpaxYxNUjEiojDSfZG/8DHa7AARCKJzf/BZm3XcQ90AV654M0cH/8hXg6a8Bw8IxRqx0ewF MPmcXyaXBQBNASs5Gb86NHQ17mhz1QdKDcO8LE3cuBlDobdiF8lcqFrXtqvwRckqRf8OmjfcUub3w i/+K6/Ms6eLsKSiP3boQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1pPOl1-00CPmj-AD; Tue, 07 Feb 2023 14:15:39 +0000 Received: from mail-ej1-x630.google.com ([2a00:1450:4864:20::630]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1pPOkv-00CPl6-Ot for linux-arm-kernel@lists.infradead.org; Tue, 07 Feb 2023 14:15:37 +0000 Received: by mail-ej1-x630.google.com with SMTP id jg8so3829182ejc.6 for ; Tue, 07 Feb 2023 06:15:32 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=resnulli-us.20210112.gappssmtp.com; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=6cDJFZGP35WmTwNvh13XMaZ3dEGeMKfGhMJ2AldNDHg=; b=Vhq32ZJ4u6zqrSmFE0KvL2PVrAsvCSdBmw2TTx93VWykFIL1BGwmQLO1PIFX/QvSrq bog3lMZJsF2EjRsHuFcc94fmbXHNXU91IIWqPiswRIZ/WofIsOW9phKIuh7uAgloOjxX asuoQr7snGxV6ujFjUDz//1rG4S+5BFPzmfKa+y/DKn/1l63/cCVBD30gfc/0TFuQJKz B4UOcUeLm65O4xOTLd6YC0n/6T63Gl1lIvj+Jcq5//zCsijZz21HLvX7jhZ4d+Gqh/RZ 9OdpwRV2I2tpg7/SyrMK1JTqPvCHenhLtSAzEx3uOJai/AF2zpFdD40BZtAxAuKWrMJm 7Pig== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=6cDJFZGP35WmTwNvh13XMaZ3dEGeMKfGhMJ2AldNDHg=; b=jj4NVWYCqSXne+MK9rb2HWzhbLAjKj6nlD7ItFxfkwbj8XEV7MdfcF+7GqFUV50kGS tAGiDtL6i6LnIQdSYRWXz8dyA4St1oRMQXX21+h3NYqR3+xiL6sKdsnIlRdHDDWZrMlY khltKSGpKzpjdokmcCR6VHLX1IZOSM1aCrM9qsc//RNagOCWst/GBXHDGhw3oZ6ObOW1 POkyBfGvY0aY9xQInUU/Bs/16N6NQY2QOBz8JAM3lyRHKDBVKXV1FvQks643G6OGhECc 7iA4ZlHURgxoCsdVUIACWZZvDRCSB5wNDtvIWUjs8Gcs/qVFRRBVo4545SKZ5EqSpcq6 iyEQ== X-Gm-Message-State: AO0yUKVYZvpHP5i6jtpzx2SC33Hf9IFxAfr0pF3dHxxxD5YsC9yO4ghT tUjnJTdfRZSfNdr0Hds2DOcFWQ== X-Google-Smtp-Source: AK7set/pux159amjKJNrW1tsUGZ8GWkKvit8R380FU7rLWqD52YoCFqDh0LAPXDU4Q8w2OPuCcAtuA== X-Received: by 2002:a17:906:5a60:b0:879:9bf4:b88a with SMTP id my32-20020a1709065a6000b008799bf4b88amr3663870ejc.77.1675779331148; Tue, 07 Feb 2023 06:15:31 -0800 (PST) Received: from localhost ([86.61.181.4]) by smtp.gmail.com with ESMTPSA id s19-20020a1709067b9300b008878909859bsm6918772ejo.152.2023.02.07.06.15.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 07 Feb 2023 06:15:30 -0800 (PST) Date: Tue, 7 Feb 2023 15:15:29 +0100 From: Jiri Pirko To: "Kubalewski, Arkadiusz" Cc: Vadim Fedorenko , Jakub Kicinski , Jonathan Lemon , Paolo Abeni , "netdev@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-clk@vger.kernel.org" , "Olech, Milena" , "Michalik, Michal" Subject: Re: [RFC PATCH v5 1/4] dpll: Add DPLL framework base functions Message-ID: References: <20230117180051.2983639-1-vadfed@meta.com> <20230117180051.2983639-2-vadfed@meta.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230207_061534_038516_6D9B085D X-CRM114-Status: GOOD ( 36.69 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Mon, Feb 06, 2023 at 03:00:09AM CET, arkadiusz.kubalewski@intel.com wrote: >>From: Jiri Pirko >>Sent: Tuesday, January 31, 2023 3:01 PM >>To: Kubalewski, Arkadiusz >>Cc: Vadim Fedorenko ; Jakub Kicinski ; >>Jonathan Lemon ; Paolo Abeni ; >>netdev@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- >>clk@vger.kernel.org; Olech, Milena ; Michalik, >>Michal >>Subject: Re: [RFC PATCH v5 1/4] dpll: Add DPLL framework base functions >> >>Fri, Jan 27, 2023 at 07:12:41PM CET, arkadiusz.kubalewski@intel.com wrote: >>>>From: Jiri Pirko >>>>Sent: Thursday, January 19, 2023 6:16 PM >>>> >>>>Tue, Jan 17, 2023 at 07:00:48PM CET, vadfed@meta.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. > >>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? >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. [...] >>>>>+ >>>>>+/* 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. > >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. > >> >>> >>>> >>>>>+ * @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. > >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. > 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. >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 > DPLLA_PIN_STATE I think you made a mistake. Should it be: DPLLA_PIN_STATE DPLLA_PIN_PRIO ? > >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... >- 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. > >Thanks, >Arkadiusz > >> >>[...] > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel