From: Jiri Pirko <jiri@resnulli.us>
To: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@intel.com>
Cc: Vadim Fedorenko <vfedorenko@novek.ru>,
Jakub Kicinski <kuba@kernel.org>,
Jonathan Lemon <jonathan.lemon@gmail.com>,
Paolo Abeni <pabeni@redhat.com>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
Vadim Fedorenko <vadfed@fb.com>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
"linux-clk@vger.kernel.org" <linux-clk@vger.kernel.org>
Subject: Re: [RFC PATCH v4 4/4] ptp_ocp: implement DPLL ops
Date: Fri, 2 Dec 2022 13:48:31 +0100 [thread overview]
Message-ID: <Y4n0H9BbzaX5pCpQ@nanopsycho> (raw)
In-Reply-To: <DM6PR11MB4657D9753412AD9DEE7FAB7D9B179@DM6PR11MB4657.namprd11.prod.outlook.com>
Fri, Dec 02, 2022 at 12:27:32PM CET, arkadiusz.kubalewski@intel.com wrote:
>>From: Jiri Pirko <jiri@resnulli.us>
>>Sent: Wednesday, November 30, 2022 1:41 PM
>>
>>Tue, Nov 29, 2022 at 10:37:24PM CET, vfedorenko@novek.ru wrote:
>>>From: Vadim Fedorenko <vadfed@fb.com>
[...]
>>>+static int ptp_ocp_dpll_get_attr(struct dpll_device *dpll, struct
>>dpll_attr *attr)
>>>+{
>>>+ struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>>>+ int sync;
>>>+
>>>+ sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC;
>>>+ dpll_attr_lock_status_set(attr, sync ? DPLL_LOCK_STATUS_LOCKED :
>>DPLL_LOCK_STATUS_UNLOCKED);
>>
>>get,set,confuse. This attr thing sucks, sorry :/
>
>Once again, I feel obligated to add some explanations :)
>
>getter is ops called by dpll subsystem, it requires data, so here value shall
>be set for the caller, right?
>Also have explained the reason why this attr struct and functions are done this
>way in the response to cover letter concerns.
Okay, I will react there.
>
>>
>>
>>>+
>>>+ return 0;
>>>+}
>>>+
>>>+static int ptp_ocp_dpll_pin_get_attr(struct dpll_device *dpll, struct
>>dpll_pin *pin,
>>>+ struct dpll_pin_attr *attr)
>>>+{
>>>+ dpll_pin_attr_type_set(attr, DPLL_PIN_TYPE_EXT);
>>
>>This is exactly what I was talking about in the cover letter. This is
>>const, should be put into static struct and passed to
>>dpll_device_alloc().
>
>Actually this type or some other parameters might change in the run-time,
No. This should not change.
If the pin is SyncE port, it's that for all lifetime of pin. It cannot
turn to be a EXT/SMA connector all of the sudden. This should be
definitelly fixed, it's a device topology.
Can you explain the exact scenario when the change of personality of pin
can happen? Perhaps I'm missing something.
>depends on the device, it is up to the driver how it will handle any getter,
>if driver knows it won't change it could also have some static member and copy
>the data with: dpll_pin_attr_copy(...);
>
>>
>>
>>>+ return 0;
>>>+}
>>>+
>>>+static struct dpll_device_ops dpll_ops = {
>>>+ .get = ptp_ocp_dpll_get_attr,
>>>+};
>>>+
>>>+static struct dpll_pin_ops dpll_pin_ops = {
>>>+ .get = ptp_ocp_dpll_pin_get_attr,
>>>+};
>>>+
>>> static int
>>> ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>> {
>>>+ const u8 dpll_cookie[DPLL_COOKIE_LEN] = { "OCP" };
>>>+ char pin_desc[PIN_DESC_LEN];
>>> struct devlink *devlink;
>>>+ struct dpll_pin *pin;
>>> struct ptp_ocp *bp;
>>>- int err;
>>>+ int err, i;
>>>
>>> devlink = devlink_alloc(&ptp_ocp_devlink_ops, sizeof(*bp), &pdev-
>>>dev);
>>> if (!devlink) {
>>>@@ -4230,6 +4263,20 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct
>>pci_device_id *id)
>>>
>>> ptp_ocp_info(bp);
>>> devlink_register(devlink);
>>>+
>>>+ bp->dpll = dpll_device_alloc(&dpll_ops, DPLL_TYPE_PPS, dpll_cookie,
>>pdev->bus->number, bp, &pdev->dev);
>>>+ if (!bp->dpll) {
>>>+ dev_err(&pdev->dev, "dpll_device_alloc failed\n");
>>>+ goto out;
>>>+ }
>>>+ dpll_device_register(bp->dpll);
>>
>>You still have the 2 step init process. I believe it would be better to
>>just have dpll_device_create/destroy() to do it in one shot.
>
>For me either is ok, but due to pins alloc/register as explained below I would
>leave it as it is.
Please don't, it has no value. Just adds unnecesary code. Have it nice
and simple.
>
>>
>>
>>>+
>>>+ for (i = 0; i < 4; i++) {
>>>+ snprintf(pin_desc, PIN_DESC_LEN, "sma%d", i + 1);
>>>+ pin = dpll_pin_alloc(pin_desc, PIN_DESC_LEN);
>>>+ dpll_pin_register(bp->dpll, pin, &dpll_pin_ops, bp);
>>
>>Same here, no point of having 2 step init.
>
>The alloc of a pin is not required if the pin already exist and would be just
>registered with another dpll.
Please don't. Have a pin created on a single DPLL. Why you make things
compitated here? I don't follow.
>Once we decide to entirely drop shared pins idea this could be probably done,
>although other kernel code usually use this twostep approach?
No, it does not. It's is used whatever fits on the individual usecase.
>
>>
>>
>>>+ }
>>>+
>>> return 0;
>>
>>
>>Btw, did you consider having dpll instance here as and auxdev? It would
>>be suitable I believe. It is quite simple to do it. See following patch
>>as an example:
>
>I haven't think about it, definetly gonna take a look to see if there any
>benefits in ice.
Please do. The proper separation and bus/device modelling is at least
one of the benefits. The other one is that all dpll drivers would
happily live in drivers/dpll/ side by side.
>
>Thanks,
>Arkadiusz
>
>>
>>commit bd02fd76d1909637c95e8ef13e7fd1e748af910d
>>Author: Jiri Pirko <jiri@nvidia.com>
>>Date: Mon Jul 25 10:29:17 2022 +0200
>>
>> mlxsw: core_linecards: Introduce per line card auxiliary device
>>
>>
>>
>>
>>>
>>> out:
>>>@@ -4247,6 +4294,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_free(bp->dpll);
>>> devlink_unregister(devlink);
>>> ptp_ocp_detach(bp);
>>> pci_disable_device(pdev);
>>>--
>>>2.27.0
>>>
next prev parent reply other threads:[~2022-12-02 12:48 UTC|newest]
Thread overview: 87+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-29 21:37 [RFC PATCH v4 0/4] Create common DPLL/clock configuration API Vadim Fedorenko
2022-11-29 21:37 ` [RFC PATCH v4 1/4] dpll: add dpll_attr/dpll_pin_attr helper classes Vadim Fedorenko
2022-11-29 21:37 ` [RFC PATCH v4 2/4] dpll: Add DPLL framework base functions Vadim Fedorenko
2022-11-30 15:21 ` Jiri Pirko
2022-11-30 16:23 ` Jiri Pirko
2022-12-23 16:45 ` Kubalewski, Arkadiusz
2023-01-02 12:28 ` Jiri Pirko
2022-11-30 16:37 ` Jiri Pirko
2022-12-02 11:27 ` Kubalewski, Arkadiusz
2022-12-02 12:39 ` Jiri Pirko
2022-12-02 14:54 ` Kubalewski, Arkadiusz
2022-12-02 16:15 ` Jiri Pirko
[not found] ` <20221202212206.3619bd5f@kernel.org>
2022-12-05 10:32 ` Jiri Pirko
2022-12-06 0:19 ` Jakub Kicinski
2022-12-06 8:50 ` Jiri Pirko
2022-12-06 17:27 ` Jakub Kicinski
2022-12-07 13:10 ` Jiri Pirko
2022-12-07 16:59 ` Jakub Kicinski
2022-12-08 8:14 ` Jiri Pirko
2022-12-08 16:19 ` Jakub Kicinski
2022-12-08 16:33 ` Jiri Pirko
2022-12-08 17:05 ` Jakub Kicinski
2022-12-09 9:29 ` Jiri Pirko
2022-12-09 16:19 ` Jakub Kicinski
2022-12-12 13:36 ` Jiri Pirko
2022-12-13 18:08 ` Kubalewski, Arkadiusz
2022-12-14 7:32 ` Jiri Pirko
2022-11-29 21:37 ` [RFC PATCH v4 3/4] dpll: documentation on DPLL subsystem interface Vadim Fedorenko
2022-12-19 9:13 ` Paolo Abeni
2023-01-12 13:45 ` Kubalewski, Arkadiusz
2022-11-29 21:37 ` [RFC PATCH v4 4/4] ptp_ocp: implement DPLL ops Vadim Fedorenko
2022-11-30 12:41 ` Jiri Pirko
2022-12-02 11:27 ` Kubalewski, Arkadiusz
2022-12-02 12:48 ` Jiri Pirko [this message]
2022-12-02 14:39 ` Kubalewski, Arkadiusz
2022-12-02 16:20 ` Jiri Pirko
2022-12-08 0:35 ` Kubalewski, Arkadiusz
2022-12-08 8:19 ` Jiri Pirko
2022-12-07 2:33 ` Jakub Kicinski
2022-12-07 13:19 ` Jiri Pirko
[not found] ` <20221207090524.3f562eeb@kernel.org>
2022-12-08 11:22 ` Jiri Pirko
2022-12-09 0:36 ` Jakub Kicinski
2022-12-09 9:32 ` Jiri Pirko
2022-11-30 12:32 ` [RFC PATCH v4 0/4] Create common DPLL/clock configuration API Jiri Pirko
2022-12-02 11:27 ` Kubalewski, Arkadiusz
2022-12-02 16:12 ` Jiri Pirko
2022-12-07 2:47 ` Jakub Kicinski
2022-12-07 14:09 ` netdev.dump
2022-12-07 23:21 ` Jakub Kicinski
2022-12-08 11:28 ` Jiri Pirko
2022-12-09 0:39 ` Jakub Kicinski
2022-12-09 0:56 ` Kubalewski, Arkadiusz
2022-12-08 18:08 ` Maciek Machnikowski
2022-12-09 11:07 ` Jiri Pirko
2022-12-09 14:09 ` Maciek Machnikowski
2022-12-09 16:31 ` Jakub Kicinski
2022-12-09 17:11 ` Maciek Machnikowski
2022-12-12 13:58 ` Jiri Pirko
2023-01-09 14:43 ` Kubalewski, Arkadiusz
2023-01-09 16:30 ` Jiri Pirko
2023-01-10 10:54 ` Kubalewski, Arkadiusz
2023-01-10 14:28 ` Jiri Pirko
[not found] ` <645a5bfd-0092-2f39-0ff2-3ffb27ccf8fe@machnikowski.net>
2023-01-11 14:17 ` Kubalewski, Arkadiusz
2023-01-11 14:40 ` Maciek Machnikowski
2023-01-11 15:30 ` Kubalewski, Arkadiusz
2023-01-11 15:54 ` Maciek Machnikowski
2023-01-11 16:27 ` Kubalewski, Arkadiusz
2023-01-10 20:05 ` Jakub Kicinski
2023-01-11 8:19 ` Jiri Pirko
2023-01-11 14:16 ` Kubalewski, Arkadiusz
2023-01-11 15:04 ` Jiri Pirko
2023-01-11 15:30 ` Kubalewski, Arkadiusz
2023-01-11 16:14 ` Jiri Pirko
2023-01-12 12:15 ` Kubalewski, Arkadiusz
2023-01-12 14:43 ` Jiri Pirko
2022-12-09 0:46 ` Kubalewski, Arkadiusz
2022-12-07 14:51 ` Jiri Pirko
[not found] ` <20221207091946.3115742f@kernel.org>
2022-12-08 12:02 ` Jiri Pirko
2022-12-09 0:54 ` Jakub Kicinski
2022-12-08 18:23 ` Kubalewski, Arkadiusz
2022-12-08 0:27 ` Kubalewski, Arkadiusz
2022-12-08 11:58 ` Jiri Pirko
2022-12-08 23:05 ` Kubalewski, Arkadiusz
2022-12-09 10:01 ` Jiri Pirko
2023-01-12 12:23 ` Kubalewski, Arkadiusz
2023-01-12 14:50 ` Jiri Pirko
2023-01-12 19:09 ` Jakub Kicinski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y4n0H9BbzaX5pCpQ@nanopsycho \
--to=jiri@resnulli.us \
--cc=arkadiusz.kubalewski@intel.com \
--cc=jonathan.lemon@gmail.com \
--cc=kuba@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-clk@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=vadfed@fb.com \
--cc=vfedorenko@novek.ru \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).