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 15CC8C61DA4 for ; Wed, 15 Mar 2023 12:25:47 +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=nVZ/Ln2VI2+GbZt0ReHQ/daTjhTOiCo4Kl1eyd53Sgc=; b=0mZMTxePXKIH6S Dp5u7cZtmXu9gLlHWxMVFYvoFTbf71OlU/p0uXrgXRGaOWhuSAwITdqbWTTTJ2tRkUlTpi6N3k9wr Q0r3wcxUWJ7Uy17udvNW+36DkUj/VwDU9w3GzE41dYrHzGL6QjAJRvpUneQBQ9YZd9nbp/pG0uAbu UQQ3U56E1N1OBY3K8xTMDfs+cyEPplyWXczNJo7fKqmY2jS8JgzTWHZp15eEnn5zyaTX90PVzNIkH 7ln2pykdk1KIL1banwOZBijarxAyyY4jr2uu37Bqx2nfFGeED4D6IKlgI8g+1QVso5lijyGme2L2j kqyLov9wNga1DILUa4Hg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1pcQBW-00DKa5-2O; Wed, 15 Mar 2023 12:24:50 +0000 Received: from mail-wm1-x329.google.com ([2a00:1450:4864:20::329]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1pcQBS-00DKYR-1j for linux-arm-kernel@lists.infradead.org; Wed, 15 Mar 2023 12:24:48 +0000 Received: by mail-wm1-x329.google.com with SMTP id j42-20020a05600c1c2a00b003ed363619ddso461525wms.1 for ; Wed, 15 Mar 2023 05:24:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=resnulli-us.20210112.gappssmtp.com; s=20210112; t=1678883084; 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=N8Q16rN2NC1bUs0QIqv2nI8RfdbcEWG6Vm+CziVAYyg=; b=XrRTA92pEHcNrFy9rxSNXyh4JU2gg74xZKKlfIIYz1x3OPCPRnTtvpF3V6ZJ1uZjdc ZFUUa+aLky5M2zoTjnw/ltjuwh8XYetA0lhkRl74E5/RjxjSdn02nGk9r94BFKkjNsoA D6QgrmBjeRd13wiaaviVqy6udNv3bgPaqsZUeDaSa37j7dcxvyAlnUQLCRyh9OIGSObH QT5Q6RtKpgZ4EgaXgLztjzPyij1o5tTKbjjhQbYh6jWFJqGuNoa8Y4ptbS6//GC275d1 vzQQacT2e9zvNBrwRXIxMiZjJZYDcmNmNSCw8ek7kkAtNOEWgHLoX0wkKo9NUwsEct7H mJ5Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678883084; 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=N8Q16rN2NC1bUs0QIqv2nI8RfdbcEWG6Vm+CziVAYyg=; b=ZSv3wju+GzSQz9Ai07p77I9cuzbEdFCavz9Ml4ZqXdZ++ZGeDS/ie344001PLmoZ1o /PvIKeirhvfUx+s+PxBuBZdvp4xfJC3PaQOTZb6j+0oYttGxmZ9itINQCZR+5ne3xeip kqTniS9/NrlCFbcD3kGEAbxkYOugWbTd05T4zCKesjn7UhO7bGDnuQKLAj6Mu4JXh4KF 8YSWrOAubGrJcKnm8Eaywlof74Sg/pd4AJoJ7K421pPGChsYSxd7kc5OAPPkxlJyhuYU gFfsEGP/KUKuaqpXYNpefTcmhKc3h50Q60zOzXPLAXyYT8L13VNZvKntAIn4NkNn5l+k mxLw== X-Gm-Message-State: AO0yUKULqcgi1tYeNXOrM78l/D6W5U2ypLhtztRTT9bsF65qyhHxmMdg VUawcM6N9AJfs4/tDjbU298HPA== X-Google-Smtp-Source: AK7set9CxeX6fv+kK+SIgm8U/7uBdHsaSF7lKLMdNpjeUv3AfGy51YiWbwR75EdQ9qxaVheAt/ls6Q== X-Received: by 2002:a05:600c:1912:b0:3ed:2217:4636 with SMTP id j18-20020a05600c191200b003ed22174636mr10441630wmq.19.1678883084004; Wed, 15 Mar 2023 05:24:44 -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 k26-20020a7bc31a000000b003eb596cbc54sm1856832wmj.0.2023.03.15.05.24.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 15 Mar 2023 05:24:43 -0700 (PDT) Date: Wed, 15 Mar 2023 13:24:42 +0100 From: Jiri Pirko To: Vadim Fedorenko Cc: Vadim Fedorenko , Jakub Kicinski , Arkadiusz Kubalewski , Jonathan Lemon , Paolo Abeni , poros@redhat.com, mschmidt@redhat.com, netdev@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-clk@vger.kernel.org Subject: Re: [PATCH RFC v6 6/6] ptp_ocp: implement DPLL ops Message-ID: References: <20230312022807.278528-1-vadfed@meta.com> <20230312022807.278528-7-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_052446_574762_E23784B3 X-CRM114-Status: GOOD ( 43.05 ) 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 Wed, Mar 15, 2023 at 01:10:33AM CET, vadim.fedorenko@linux.dev wrote: >On 14/03/2023 10:05, Jiri Pirko wrote: >> Sun, Mar 12, 2023 at 03:28:07AM CET, vadfed@meta.com wrote: >> > Implement basic DPLL operations in ptp_ocp driver as the >> > simplest example of using new subsystem. >> > >> > Signed-off-by: Vadim Fedorenko >> > --- >> > drivers/ptp/Kconfig | 1 + >> > drivers/ptp/ptp_ocp.c | 209 ++++++++++++++++++++++++++++++++++++++++-- >> > 2 files changed, 200 insertions(+), 10 deletions(-) >> > >> > diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig >> > index fe4971b65c64..8c4cfabc1bfa 100644 >> > --- a/drivers/ptp/Kconfig >> > +++ b/drivers/ptp/Kconfig >> > @@ -177,6 +177,7 @@ config PTP_1588_CLOCK_OCP >> > depends on COMMON_CLK >> > select NET_DEVLINK >> > select CRC16 >> > + select DPLL >> > help >> > This driver adds support for an OpenCompute time card. >> > >> > diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c >> > index 4bbaccd543ad..02c95e724ec8 100644 >> > --- a/drivers/ptp/ptp_ocp.c >> > +++ b/drivers/ptp/ptp_ocp.c >> > @@ -23,6 +23,8 @@ >> > #include >> > #include >> > #include >> > +#include >> > +#include >> >> Don't include UAPI directly. I'm pretty sure I had this comment earlier. >> > >Ah, yeah, you've mentioned it for ice driver last time, but I forgot to check >it here. Removed. > >> >> > >> > #define PCI_VENDOR_ID_FACEBOOK 0x1d9b >> > #define PCI_DEVICE_ID_FACEBOOK_TIMECARD 0x0400 >> > @@ -267,6 +269,7 @@ struct ptp_ocp_sma_connector { >> > bool fixed_dir; >> > bool disabled; >> > u8 default_fcn; >> > + struct dpll_pin *dpll_pin; >> > }; >> > >> > struct ocp_attr_group { >> > @@ -353,6 +356,7 @@ struct ptp_ocp { >> > struct ptp_ocp_signal signal[4]; >> > struct ptp_ocp_sma_connector sma[4]; >> >> It is quite customary to use defines for const numbers like this. Why >> don't you do that in ptp_ocp? > >Historical reasons. Jonathan might answer this question. >I will add it to my TODO list for the driver. > >> > const struct ocp_sma_op *sma_op; >> > + struct dpll_device *dpll; >> > }; >> > >> > #define OCP_REQ_TIMESTAMP BIT(0) >> > @@ -2689,16 +2693,9 @@ sma4_show(struct device *dev, struct device_attribute *attr, char *buf) >> > } >> > >> > static int >> > -ptp_ocp_sma_store(struct ptp_ocp *bp, const char *buf, int sma_nr) >> > +ptp_ocp_sma_store_val(struct ptp_ocp *bp, int val, enum ptp_ocp_sma_mode mode, int sma_nr) >> > { >> > struct ptp_ocp_sma_connector *sma = &bp->sma[sma_nr - 1]; >> > - enum ptp_ocp_sma_mode mode; >> > - int val; >> > - >> > - mode = sma->mode; >> > - val = sma_parse_inputs(bp->sma_op->tbl, buf, &mode); >> > - if (val < 0) >> > - return val; >> > >> > if (sma->fixed_dir && (mode != sma->mode || val & SMA_DISABLE)) >> > return -EOPNOTSUPP; >> > @@ -2733,6 +2730,21 @@ ptp_ocp_sma_store(struct ptp_ocp *bp, const char *buf, int sma_nr) >> > return val; >> > } >> > >> > +static int >> > +ptp_ocp_sma_store(struct ptp_ocp *bp, const char *buf, int sma_nr) >> > +{ >> > + struct ptp_ocp_sma_connector *sma = &bp->sma[sma_nr - 1]; >> > + enum ptp_ocp_sma_mode mode; >> > + int val; >> > + >> > + mode = sma->mode; >> > + val = sma_parse_inputs(bp->sma_op->tbl, buf, &mode); >> > + if (val < 0) >> > + return val; >> > + >> > + return ptp_ocp_sma_store_val(bp, val, mode, sma_nr); >> > +} >> > + >> > static ssize_t >> > sma1_store(struct device *dev, struct device_attribute *attr, >> > const char *buf, size_t count) >> > @@ -4171,12 +4183,151 @@ ptp_ocp_detach(struct ptp_ocp *bp) >> > device_unregister(&bp->dev); >> > } >> > >> > +static int ptp_ocp_dpll_pin_to_sma(const struct ptp_ocp *bp, const struct dpll_pin *pin) >> > +{ >> > + int i; >> > + >> > + for (i = 0; i < 4; i++) { >> > + if (bp->sma[i].dpll_pin == pin) >> > + return i; >> >> Just pass &bp->sma[i] as a priv to dpll_pin_register(). >> and get that pointer directly in pin ops. No need for lookup and use of >> sma_nr at all. > >I'm still thinking about the change that you proposed to remove these >_priv() helpers. I have to look into ice code to be sure we won't break >any assumptions in it with moving to additional (void *) parameter. There are basically 2 ways: someop(struct subsystemobj *x, void *priv) { struct *mine = priv; } or: someop(struct subsystemobj *x) { struct *mine = subsystem_get_priv(x); } Both are more or less equal. The first has benefit that the caller most usually has direct access to the priv, so it can just easily pass it on. Also, you as the driver write see right away there is a priv arg and makes you want to use it and not figure out odd lookups to get to the same priv. > >> >> > + } >> > + return -1; >> > +} >> > + >> > +static int ptp_ocp_dpll_lock_status_get(const struct dpll_device *dpll, >> > + enum dpll_lock_status *status, >> > + struct netlink_ext_ack *extack) >> > +{ >> > + struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll); >> >> No need to cast (void *), remove it. >> >Fixed everywhere in the code, thanks. > > >> >> > + int sync; >> > + >> > + sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC; >> > + *status = sync ? DPLL_LOCK_STATUS_LOCKED : DPLL_LOCK_STATUS_UNLOCKED; >> > + >> > + return 0; >> > +} >> > + >> > +static int ptp_ocp_dpll_source_idx_get(const struct dpll_device *dpll, >> > + u32 *idx, struct netlink_ext_ack *extack) >> > +{ >> > + struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll); >> > + >> > + if (bp->pps_select) { >> > + *idx = ioread32(&bp->pps_select->gpio1); >> > + return 0; >> > + } >> > + return -EINVAL; >> > +} >> > + >> > +static int ptp_ocp_dpll_mode_get(const struct dpll_device *dpll, >> > + u32 *mode, struct netlink_ext_ack *extack) >> > +{ >> > + *mode = DPLL_MODE_AUTOMATIC; >> > + >> >> No need for empty line, I believe. > >Ok. > >> > + return 0; >> > +} >> > + >> > +static bool ptp_ocp_dpll_mode_supported(const struct dpll_device *dpll, >> > + const enum dpll_mode mode, >> > + struct netlink_ext_ack *extack) >> > +{ >> > + if (mode == DPLL_MODE_AUTOMATIC) >> > + return true; >> > + >> > + return false; >> >> Just: >> return mode == DPLL_MODE_AUTOMATIC; >> >Done, thanks! > >> > +} >> > + >> > +static int ptp_ocp_dpll_direction_get(const struct dpll_pin *pin, >> > + const struct dpll_device *dpll, >> > + enum dpll_pin_direction *direction, >> > + struct netlink_ext_ack *extack) >> > +{ >> > + struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll); >> > + int sma_nr = ptp_ocp_dpll_pin_to_sma(bp, pin); >> > + >> > + if (sma_nr < 0) >> > + return -EINVAL; >> > + >> > + *direction = bp->sma[sma_nr].mode == SMA_MODE_IN ? DPLL_PIN_DIRECTION_SOURCE : >> > + DPLL_PIN_DIRECTION_OUTPUT; >> > + return 0; >> > +} >> > + >> > +static int ptp_ocp_dpll_direction_set(const struct dpll_pin *pin, >> > + const struct dpll_device *dpll, >> > + enum dpll_pin_direction direction, >> > + struct netlink_ext_ack *extack) >> > +{ >> > + struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll); >> > + int sma_nr = ptp_ocp_dpll_pin_to_sma(bp, pin); >> > + enum ptp_ocp_sma_mode mode; >> > + >> > + if (sma_nr < 0) >> > + return -EINVAL; >> > + >> > + mode = direction == DPLL_PIN_DIRECTION_SOURCE ? SMA_MODE_IN : SMA_MODE_OUT; >> > + return ptp_ocp_sma_store_val(bp, 0, mode, sma_nr); >> > +} >> > + >> > +static int ptp_ocp_dpll_frequency_set(const struct dpll_pin *pin, >> > + const struct dpll_device *dpll, >> > + const u32 frequency, >> >> Why you need const for u32? > >No need, true, removed. > >> >> > + struct netlink_ext_ack *extack) >> > +{ >> > + struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll); >> > + int sma_nr = ptp_ocp_dpll_pin_to_sma(bp, pin); >> > + int val = frequency == 10000000 ? 0 : 1; >> >> This is weird. I believe you should handle unsupported frequencies >> gracefully. >> > >Currently hardware supports fixed frequencies only. That's why I have to >do this kind of "dance". Hopefully we will improve it to support variable >frequency. > >> >> > + >> > + if (sma_nr < 0) >> > + return -EINVAL; >> > + >> > + >> >> Avoid double empty lines. >> > >Yep. > >> >> > + return ptp_ocp_sma_store_val(bp, val, bp->sma[sma_nr].mode, sma_nr); >> > +} >> > + >> > +static int ptp_ocp_dpll_frequency_get(const struct dpll_pin *pin, >> > + const struct dpll_device *dpll, >> > + u32 *frequency, >> > + struct netlink_ext_ack *extack) >> > +{ >> > + struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll); >> > + int sma_nr = ptp_ocp_dpll_pin_to_sma(bp, pin); >> > + u32 val; >> > + >> > + if (sma_nr < 0) >> > + return -EINVAL; >> > + >> > + val = bp->sma_op->get(bp, sma_nr); >> > + if (!val) >> > + *frequency = 1000000; >> > + else >> > + *frequency = 0; >> >> I don't follow. How the frequency could be 0? Does that mean that the >> pin is disabled? If yes, you should rather implement >> .state_on_dpll_get >> .state_on_dpll_set >> >> and leave the frequency constant. > >It's actually a mistake. It should be 1 which means 1Hz, PPS. The value >returned by sma_op->get is actually the type of the signal where 0 is 1PPS, 1 >is 10Mhz and so on. So you support freq change? In the comment above you wrote "Currently hardware supports fixed frequencies only". I'm confused. The point is: 1) if you support freq change, you should sanitize the input properly in frequency_set() and return correct actual value in frequency_get() 2) if you don't support freq change, avoid filling-up these ops entirely. > >> >> >> > + return 0; >> > +} >> > + >> > +static struct dpll_device_ops dpll_ops = { >> >> const >> > >Will change it together with API part. Otherwise it doesn't compile. > >> >> > + .lock_status_get = ptp_ocp_dpll_lock_status_get, >> > + .source_pin_idx_get = ptp_ocp_dpll_source_idx_get, >> >> Should be called "source_pin_get" and return (struct dpll_pin *) >> On the driver api level, no reason to pass indexes. Work with objects. >> > >Good point, will improve it. > >> >> > + .mode_get = ptp_ocp_dpll_mode_get, >> > + .mode_supported = ptp_ocp_dpll_mode_supported, >> > +}; >> > + >> > +static struct dpll_pin_ops dpll_pins_ops = { >> >> const >> > >Yep. > >> >> > + .frequency_get = ptp_ocp_dpll_frequency_get, >> > + .frequency_set = ptp_ocp_dpll_frequency_set, >> > + .direction_get = ptp_ocp_dpll_direction_get, >> > + .direction_set = ptp_ocp_dpll_direction_set, >> > +}; >> > + >> > static int >> > ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id) >> > { >> > + struct dpll_pin_properties prop; >> > struct devlink *devlink; >> > + char sma[4] = "SMA0"; >> >> Won't fit. Just use: >> char *sma = "SMA0" >> to be safe >> >Got it. > >> >> > struct ptp_ocp *bp; >> > - int err; >> > + int err, i; >> > + u64 clkid; >> > >> > devlink = devlink_alloc(&ptp_ocp_devlink_ops, sizeof(*bp), &pdev->dev); >> > if (!devlink) { >> > @@ -4226,8 +4377,44 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id) >> > >> > ptp_ocp_info(bp); >> > devlink_register(devlink); >> > - return 0; >> > >> > + clkid = pci_get_dsn(pdev); >> > + bp->dpll = dpll_device_get(clkid, 0, THIS_MODULE); >> > + if (!bp->dpll) { >> > + dev_err(&pdev->dev, "dpll_device_alloc failed\n"); >> > + goto out; >> > + } >> > + >> > + err = dpll_device_register(bp->dpll, DPLL_TYPE_PPS, &dpll_ops, bp, &pdev->dev); >> > + if (err) >> > + goto out; >> > + >> > + prop.description = &sma[0]; >> > + prop.freq_supported = DPLL_PIN_FREQ_SUPP_MAX; >> > + prop.type = DPLL_PIN_TYPE_EXT; >> > + prop.any_freq_max = 10000000; >> > + prop.any_freq_min = 0; >> > + prop.capabilities = DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE; >> > + >> > + for (i = 0; i < 4; i++) { >> > + sma[3] = 0x31 + i; >> >> Ugh. Just use the static const array as I suggested in the dpll patch >> reply. Helps you avoid this sort of "magic". >> >well, yes. but at the same time Arkadiusz raised a good question about >accessing driver's memory from the subsystem code - doesn't look clean. It is completely clean and we do it everywhere in the kernel. No problem what so ever. > >> >> > + bp->sma[i].dpll_pin = dpll_pin_get(clkid, i, THIS_MODULE, &prop); >> > + if (IS_ERR_OR_NULL(bp->sma[i].dpll_pin)) { >> >> How it could be NULL? >> > >Allocation fail? No? I mean, I don't see such possibility of return value of dpll_pin_get() Do you see it or are you just guessing? :) > >> >> > + bp->sma[i].dpll_pin = NULL; >> >> This would not be needed if the error path iterates over >> indexes which were successul. >> > >Yeah, I'll make it the same way it's done in ice. > > >> >> > + goto out_dpll; >> > + } >> > + err = dpll_pin_register(bp->dpll, bp->sma[i].dpll_pin, &dpll_pins_ops, bp, NULL); >> > + if (err) >> > + goto out_dpll; >> > + } >> > + >> > + return 0; >> > +out_dpll: >> > + for (i = 0; i < 4; i++) { >> > + if (bp->sma[i].dpll_pin) >> >> You don't do unregister of pin here. >> >Yeah, actually error path and unload path should be re-implemented. > >> >> > + dpll_pin_put(bp->sma[i].dpll_pin); >> > + } >> > + dpll_device_put(bp->dpll); >> > out: >> > ptp_ocp_detach(bp); >> > out_disable: >> > @@ -4243,6 +4430,8 @@ ptp_ocp_remove(struct pci_dev *pdev) >> > struct ptp_ocp *bp = pci_get_drvdata(pdev); >> > struct devlink *devlink = priv_to_devlink(bp); >> > >> > + dpll_device_unregister(bp->dpll); >> > + dpll_device_put(bp->dpll); >> > devlink_unregister(devlink); >> > ptp_ocp_detach(bp); >> > pci_disable_device(pdev); >> > -- >> > 2.34.1 >> > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel