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 CEA2AC7618B for ; Wed, 15 Mar 2023 14:44:15 +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=Wgg144Yf1luL4UqtE+CykXHgiU7MNAm/aEvy3wECjjc=; b=LoTctsCCtKsig4 1oWvZ3dKIeM/PVBgFxVWtOkZLo3NGJSH+C8vErNW+NxbNSJmP89SzFShItSw8te118zWzO5rniD4X IlGacb9JPoLNF8HXoCdnBy7eMLpfpWyAqFMKnPFaK65hwesB+QAd++0Ra49Tv1gondbPmhS8fo/if Y/tz7FARxiVCdwPiqdmUeogAbz/g9cy9EivVdkUnfUxP6vql6nhtOfm/dRyUsWWm5knJACQzMTq2Y nfIXqePxhpK59PI2k1PZZ0+LtLZ21jFnS2t0lPqRIEP0/YHX4aTaBsSua2VxAq4oFEbcZHGBx15U0 9FVXZ7/fm8gkuGXs2C7w==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1pcSLZ-00Ddtw-2x; Wed, 15 Mar 2023 14:43:21 +0000 Received: from mail-wm1-x32c.google.com ([2a00:1450:4864:20::32c]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1pcSLV-00DdsD-0i for linux-arm-kernel@lists.infradead.org; Wed, 15 Mar 2023 14:43:20 +0000 Received: by mail-wm1-x32c.google.com with SMTP id bi20so2822620wmb.2 for ; Wed, 15 Mar 2023 07:43:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=resnulli-us.20210112.gappssmtp.com; s=20210112; t=1678891394; 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=G/vajOgVJDGRyPsxvLCJVF/iXOTJH808d6bXOCfkt/4=; b=Xc/mE1ze/aKwECzjVQuYYFfcwJPstGHaCM8oGgRmksCbJrKA+J0LGug3x42Xewk34o j031ICgUVeeq30dUFXMk2tcMZkWmI/ztwTq6Apecy1LA5Vfg7d41PoHxgtig2QLjZIG1 vK91xzTghoD4d1KqiKRIKkKgaXZaNufR3R1PMfI46kH3D6SWFZq/8bVrfKAm58ymmolF AITe9/1pAofnnka0RNKJbYDLzg08SvGFZPI866mi/zJvP8ZQEeWL9MOUHxiLDHLmMYKw yJnSlUmLY6kX72ppy27330uJ8FY5cB4a4YYZFoFxkMsbIsV8gnm+Oum2KNjpB/qAWouI yBAA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678891394; 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=G/vajOgVJDGRyPsxvLCJVF/iXOTJH808d6bXOCfkt/4=; b=CP8RBtIObJd99W2vqKIoMVx96RYhjqj5HyMIYII1bjtt6TAxDUoanzoeNFRg1XjWrc WyHZhlTXufdlArwjBwhGkQpwwjVScBwllktNIQqOV3vQX457Z10hPQFkyZslXwH2+wDB KeTzJpOGN8yfQawhPo2xP+OZRo2j1F+WadacqKtgm5C2D0Oj82wQUG2LDD7FOoCpNpwR bKSIrA9dy0mM4hMP6qSvHs0BO5TDptiR6JGi3AuK7jAkA+JnnBiTs8eZOUQv7wH+LYO+ 1R8BFlLBBt4uDgcilmOWlnxfsF9NS562IZX5iAhzLKAjYgYQEyOCqTga30+ruSaTiiEn 60HQ== X-Gm-Message-State: AO0yUKWx7dpmmLKcv8wXUwBwd88Ksshfx8Bd4H6wx6clp86ihoWKQ51/ S32XXy75r1JawdPNElCrDPUtpg== X-Google-Smtp-Source: AK7set8RNWBOtRp7eGHGijYKEIg//aBJ8DcuA/QvJ1qGKHco6K1DLauD56YV7b5Z67TmzNwKaX1WFA== X-Received: by 2002:a05:600c:198e:b0:3eb:2f3b:4477 with SMTP id t14-20020a05600c198e00b003eb2f3b4477mr17709955wmq.28.1678891393663; Wed, 15 Mar 2023 07:43:13 -0700 (PDT) Received: from localhost (host-213-179-129-39.customer.m-online.net. [213.179.129.39]) by smtp.gmail.com with ESMTPSA id y16-20020a05600c365000b003ed23845666sm2003884wmq.45.2023.03.15.07.43.04 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 15 Mar 2023 07:43:06 -0700 (PDT) Date: Wed, 15 Mar 2023 15:43:03 +0100 From: Jiri Pirko To: "Kubalewski, Arkadiusz" Cc: Vadim Fedorenko , Jakub Kicinski , Jonathan Lemon , Paolo Abeni , Vadim Fedorenko , poros , mschmidt , "netdev@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-clk@vger.kernel.org" , "Olech, Milena" , "Michalik, Michal" Subject: Re: [PATCH RFC v6 2/6] dpll: Add DPLL framework base functions Message-ID: References: <20230312022807.278528-1-vadfed@meta.com> <20230312022807.278528-3-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-20230315_074317_520019_F797B4E4 X-CRM114-Status: GOOD ( 20.37 ) 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 Tue, Mar 14, 2023 at 07:35:55PM CET, arkadiusz.kubalewski@intel.com wrote: >>From: Jiri Pirko >>Sent: Tuesday, March 14, 2023 4:45 PM >> >>[...] >> >> >>>diff --git a/MAINTAINERS b/MAINTAINERS >>>index edd3d562beee..0222b19af545 100644 >>>--- a/MAINTAINERS >>>+++ b/MAINTAINERS >>>@@ -6289,6 +6289,15 @@ F: >> Documentation/networking/device_drivers/ethernet/freescale/dpaa2/swit >>ch-drive >>> F: drivers/net/ethernet/freescale/dpaa2/dpaa2-switch* >>> F: drivers/net/ethernet/freescale/dpaa2/dpsw* >>> >>>+DPLL CLOCK SUBSYSTEM >> >>Why "clock"? You don't mention "clock" anywhere else. >> >>[...] >> >> >>>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c >>>new file mode 100644 >>>index 000000000000..3fc151e16751 >>>--- /dev/null >>>+++ b/drivers/dpll/dpll_core.c >>>@@ -0,0 +1,835 @@ >>>+// SPDX-License-Identifier: GPL-2.0 >>>+/* >>>+ * dpll_core.c - Generic DPLL Management class support. >> >>Why "class" ? >> >>[...] >> >> >>>+static int >>>+dpll_msg_add_pin_freq(struct sk_buff *msg, const struct dpll_pin *pin, >>>+ struct netlink_ext_ack *extack, bool dump_any_freq) >>>+{ >>>+ enum dpll_pin_freq_supp fs; >>>+ struct dpll_pin_ref *ref; >>>+ unsigned long i; >>>+ u32 freq; >>>+ >>>+ xa_for_each((struct xarray *)&pin->dpll_refs, i, ref) { >>>+ if (ref && ref->ops && ref->dpll) >>>+ break; >>>+ } >>>+ if (!ref || !ref->ops || !ref->dpll) >>>+ return -ENODEV; >>>+ if (!ref->ops->frequency_get) >>>+ return -EOPNOTSUPP; >>>+ if (ref->ops->frequency_get(pin, ref->dpll, &freq, extack)) >>>+ return -EFAULT; >>>+ if (nla_put_u32(msg, DPLL_A_PIN_FREQUENCY, freq)) >>>+ return -EMSGSIZE; >>>+ if (!dump_any_freq) >>>+ return 0; >>>+ for (fs = DPLL_PIN_FREQ_SUPP_UNSPEC + 1; >>>+ fs <= DPLL_PIN_FREQ_SUPP_MAX; fs++) { >>>+ if (test_bit(fs, &pin->prop.freq_supported)) { >>>+ if (nla_put_u32(msg, DPLL_A_PIN_FREQUENCY_SUPPORTED, >>>+ dpll_pin_freq_value[fs])) >> >>This is odd. As I suggested in the yaml patch, better to treat all >>supported frequencies the same, no matter if it is range or not. The you >>don't need this weird bitfield. >> >>You can have a macro to help driver to assemble array of supported >>frequencies and ranges. >> > >I understand suggestion on yaml, but here I am confused. >How do they relate to the supported frequency passed between driver and dpll >subsystem? >This bitfield is not visible to the userspace, and sure probably adding macro >can be useful. My point is to avoid the bitfield and to treat supported frequencies and ranges in the same way. It can look similar to this: in dpll.h: struct struct dpll_pin_frequency { u64 min; u64 max; }; #define DPLL_PIN_FREQUENCY_RANGE(_min, _mac) \ { \ .min = _min, \ .max = _max, \ } #define DPLL_PIN_FREQUENCY(_val) DPLL_PIN_FREQUENCY_RANGE(_val, _val) #define DPLL_PIN_FREQUENCY_1PPS DPLL_PIN_FREQUENCY(1) #define DPLL_PIN_FREQUENCY_10MHZ DPLL_PIN_FREQUENCY(1000000) Then in driver you have: static const struct dpll_pin_frequency pcp_dpll_pin_freqs[] = { DPLL_PIN_FREQUENCY_1PPS, DPLL_PIN_FREQUENCY_10MHZ, DPLL_PIN_FREQUENCY(4000000), DPLL_PIN_FREQUENCY_RANGE(500, 1000), DPLL_PIN_FREQUENCY_RANGE(9000, 10000), }; static const struct dpll_pin_properties pcp_dpll_pin_props = { .label = "SMA", .frequencies_supported = pcp_dpll_pin_freqs, .frequencies_supported_count = ARRAY_SIZE(pcp_dpll_pin_freqs), .type = DPLL_PIN_TYPE_EXT, .capabilities = DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE, }; Then the dpll core could very easily iterate over .frequencies_supported array and dump the supported values and ranges to user in uniform way. > >> >>>+ return -EMSGSIZE; >>>+ } >>>+ } >>>+ if (pin->prop.any_freq_min && pin->prop.any_freq_max) { >>>+ if (nla_put_u32(msg, DPLL_A_PIN_ANY_FREQUENCY_MIN, >>>+ pin->prop.any_freq_min)) >>>+ return -EMSGSIZE; >>>+ if (nla_put_u32(msg, DPLL_A_PIN_ANY_FREQUENCY_MAX, >>>+ pin->prop.any_freq_max)) >>>+ return -EMSGSIZE; >>>+ } >>>+ >>>+ return 0; >>>+} >>>+ >> >>[...] >> >> >>>+static int >>>+dpll_cmd_pin_on_dpll_get(struct sk_buff *msg, struct dpll_pin *pin, >>>+ struct dpll_device *dpll, >>>+ struct netlink_ext_ack *extack) >>>+{ >>>+ struct dpll_pin_ref *ref; >>>+ int ret; >>>+ >>>+ if (nla_put_u32(msg, DPLL_A_PIN_IDX, pin->dev_driver_id)) >>>+ return -EMSGSIZE; >>>+ if (nla_put_string(msg, DPLL_A_PIN_DESCRIPTION, pin->prop.description)) >>>+ return -EMSGSIZE; >>>+ if (nla_put_u8(msg, DPLL_A_PIN_TYPE, pin->prop.type)) >>>+ return -EMSGSIZE; >>>+ if (nla_put_u32(msg, DPLL_A_PIN_DPLL_CAPS, pin->prop.capabilities)) >>>+ return -EMSGSIZE; >>>+ ret = dpll_msg_add_pin_direction(msg, pin, extack); >>>+ if (ret) >>>+ return ret; >>>+ ret = dpll_msg_add_pin_freq(msg, pin, extack, true); >>>+ if (ret && ret != -EOPNOTSUPP) >>>+ return ret; >>>+ ref = dpll_xa_ref_dpll_find(&pin->dpll_refs, dpll); >>>+ if (!ref) >> >>How exactly this can happen? Looks to me like only in case of a bug. >>WARN_ON() perhaps (put directly into dpll_xa_ref_dpll_find()? > >Yes, makes sense. > >> >> >>>+ return -EFAULT; >>>+ ret = dpll_msg_add_pin_prio(msg, pin, ref, extack); >>>+ if (ret && ret != -EOPNOTSUPP) >>>+ return ret; >>>+ ret = dpll_msg_add_pin_on_dpll_state(msg, pin, ref, extack); >>>+ if (ret && ret != -EOPNOTSUPP) >>>+ return ret; >>>+ ret = dpll_msg_add_pin_parents(msg, pin, extack); >>>+ if (ret) >>>+ return ret; >>>+ if (pin->rclk_dev_name) >> >>Use && and single if >> > >Make sense to me. > >> >>>+ if (nla_put_string(msg, DPLL_A_PIN_RCLK_DEVICE, >>>+ pin->rclk_dev_name)) >>>+ return -EMSGSIZE; >>>+ >>>+ return 0; >>>+} >>>+ >> >>[...] >> >> >>>+static int >>>+dpll_pin_freq_set(struct dpll_pin *pin, struct nlattr *a, >>>+ struct netlink_ext_ack *extack) >>>+{ >>>+ u32 freq = nla_get_u32(a); >>>+ struct dpll_pin_ref *ref; >>>+ unsigned long i; >>>+ int ret; >>>+ >>>+ if (!dpll_pin_is_freq_supported(pin, freq)) >>>+ return -EINVAL; >>>+ >>>+ xa_for_each(&pin->dpll_refs, i, ref) { >>>+ ret = ref->ops->frequency_set(pin, ref->dpll, freq, extack); >>>+ if (ret) >>>+ return -EFAULT; >> >>return what the op returns: ret > >Why would we return here a driver return code, userspace can have this info >from extack. IMHO return values of dpll subsystem shall be not dependent on >what is returned from the driver. Why not to return it? The driver had some problem, errno suggests what that was. It is completely desired to pass that along and actually, it's been done like this in the rest of the netlink ops I can think of. Why would you want to hide it? Extack carries string message, not related to this directly. > >> >> >>>+ dpll_pin_notify(ref->dpll, pin, DPLL_A_PIN_FREQUENCY); >>>+ } >>>+ >>>+ return 0; >>>+} >>>+ >> >>[...] >> >> >>>+static int >>>+dpll_pin_direction_set(struct dpll_pin *pin, struct nlattr *a, >>>+ struct netlink_ext_ack *extack) >>>+{ >>>+ enum dpll_pin_direction direction = nla_get_u8(a); >>>+ struct dpll_pin_ref *ref; >>>+ unsigned long i; >>>+ >>>+ if (!(DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE & pin->prop.capabilities)) >>>+ return -EOPNOTSUPP; >>>+ >>>+ xa_for_each(&pin->dpll_refs, i, ref) { >>>+ if (ref->ops->direction_set(pin, ref->dpll, direction, extack)) >> >>ret = .. >>if (ret) >> return ret; >> >>Please use this pattern in other ops call code as well. >> > >This is the same as above (return code by driver) explanation. Same reply as above. > >> >>>+ return -EFAULT; >>>+ dpll_pin_notify(ref->dpll, pin, DPLL_A_PIN_DIRECTION); >>>+ } >>>+ >>>+ return 0; >> >>[...] > >Thanks, >Arkadiusz _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel