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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8EB50C4708E for ; Wed, 7 Dec 2022 14:51:54 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229489AbiLGOvw (ORCPT ); Wed, 7 Dec 2022 09:51:52 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41708 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229523AbiLGOvu (ORCPT ); Wed, 7 Dec 2022 09:51:50 -0500 Received: from mail-ej1-x630.google.com (mail-ej1-x630.google.com [IPv6:2a00:1450:4864:20::630]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id AC4294E6B9 for ; Wed, 7 Dec 2022 06:51:45 -0800 (PST) Received: by mail-ej1-x630.google.com with SMTP id ud5so14586323ejc.4 for ; Wed, 07 Dec 2022 06:51:45 -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=YOoLyltdiLP2JEEnnOjDDBjmowt4f2XyOeFTDUjuF0o=; b=8LPvLa5f8GWVmsFwwTg1U5kbOjmfPOIweDVg0JgDkU66U1Ai1CbGUzeH5erXRynjOi bpCw2zeV3JTSuD4QxVcpaCSD65NM5j/LicODL+gOZ/fyGnZm2JdE/LAmtPIs08k3OafD gzY126/r2xXz3k3USm6gxKL86ySXNxid+nHRkNG9XNIXdoisTW6DyfyWeTrF+CE7MuvH phmUUNN96h8w6chWW22UHnVkGmKHWUs1ME3SN0+GJGAZs1eY3Peh+8H1UGD9QwSobNdY kNDYVcWIBg/M02ThYpRLRfaSzbvWC9q7qVOtW8q9ueoTalt19EPU8s/8Tbgnk1gMB+Bs oX/A== 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=YOoLyltdiLP2JEEnnOjDDBjmowt4f2XyOeFTDUjuF0o=; b=1Q2kQ3SnaFzJODUnjWzkczUvhLwX3WBO3MQH59ECs472+OvjlHqPypi5S/D3OC2mLO GlphAd+oxO4/7sKtNy9vOuaHXqMaHuKrPuq2W9snU3hH0Pqmwd7YdN+3bIaza1RJn95o /fgx/kzRa1Uk2PgU3185bl0Bs3xpJF2PgvmwGwCp1K1aoFGEQxp378p04cF5UsQWSeE+ wmOeZloCrQbMTxtUCNmwK24lqn01R8FYYNEQ6e1HXunxkVz2OSKsZQZyTDuDeELLtQgk rWeeCgOGSPvbss8pSzwqpM/3WBvOA8zcVj037Xa1mNd6VsZue3hGC1LAFP9D9H8IESU1 YB9A== X-Gm-Message-State: ANoB5pk6W633maLJcqWjqiBIQ0jrhtjD1YUgJ/Tvue8Vp11qXDFN3V3N EqFzbr2V4u6zbpvONl/8zUZVvg== X-Google-Smtp-Source: AA0mqf4OhmBvoNlAGnMbABNrcBrV4WGf+zkPhfefEbSuw49mlb/2DuiPLNVZwEQyVvDzmJlJx3pPTA== X-Received: by 2002:a17:906:39c8:b0:7ad:79c0:5482 with SMTP id i8-20020a17090639c800b007ad79c05482mr67114184eje.730.1670424704218; Wed, 07 Dec 2022 06:51:44 -0800 (PST) Received: from localhost (host-213-179-129-39.customer.m-online.net. [213.179.129.39]) by smtp.gmail.com with ESMTPSA id s26-20020a056402015a00b00461bacee867sm2294555edu.25.2022.12.07.06.51.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 07 Dec 2022 06:51:43 -0800 (PST) Date: Wed, 7 Dec 2022 15:51:42 +0100 From: Jiri Pirko To: Jakub Kicinski Cc: "Kubalewski, Arkadiusz" , Vadim Fedorenko , Jonathan Lemon , Paolo Abeni , "netdev@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-clk@vger.kernel.org" Subject: Re: [RFC PATCH v4 0/4] Create common DPLL/clock configuration API Message-ID: References: <20221129213724.10119-1-vfedorenko@novek.ru> <20221206184740.28cb7627@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20221206184740.28cb7627@kernel.org> Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Wed, Dec 07, 2022 at 03:47:40AM CET, kuba@kernel.org wrote: >On Fri, 2 Dec 2022 17:12:06 +0100 Jiri Pirko wrote: >> >But this is only doable with assumption, that the board is internally capable >> >of such internal board level communication, which in case of separated >> >firmwares handling multiple dplls might not be the case, or it would require >> >to have some other sw component feel that gap. >> >> Yep, you have the knowledge of sharing inside the driver, so you should >> do it there. For multiple instances, use in-driver notifier for example. > >No, complexity in the drivers is not a good idea. The core should cover >the complexity and let the drivers be simple. Really, even in case only one driver actually consumes the complexicity? I understand having a "libs" to do common functionality for drivers, even in case there is one. But this case, I don't see any benefit. > >> >For complex boards with multiple dplls/sync channels, multiple ports, >> >multiple firmware instances, it seems to be complicated to share a pin if >> >each driver would have own copy and should notify all the other about changes. >> > >> >To summarize, that is certainly true, shared pins idea complicates stuff >> >inside of dpll subsystem. >> >But at the same time it removes complexity from all the drivers which would use >> >> There are currently 3 drivers for dpll I know of. This in ptp_ocp and >> mlx5 there is no concept of sharing pins. You you are talking about a >> single driver. >> >> What I'm trying to say is, looking at the code, the pin sharing, >> references and locking makes things uncomfortably complex. You are so >> far the only driver to need this, do it internally. If in the future >> other driver appears, this code would be eventually pushed into dpll >> core. No impact on UAPI from what I see. Please keep things as simple as >> possible. > >But the pin is shared for one driver. Who cares if it's not shared in >another. The user space must be able to reason about the constraints. Sorry, I don't follow :/ Could you please explain what do you mean by this? > >You are suggesting drivers to magically flip state in core objects >because of some hidden dependencies?! It's not a state flip. It's more like a well propagated event of a state change. The async changes may happen anyway, so the userspace needs to handle them. Why is this different? > >> >it and is easier for the userspace due to common identification of pins. >> >> By identification, you mean "description" right? I see no problem of 2 >> instances have the same pin "description"/label. >> >> >This solution scales up without any additional complexity in the driver, >> >and without any need for internal per-board communication channels. >> > >> >Not sure if this is good or bad, but with current version, both approaches are >> >possible, so it pretty much depending on the driver to initialize dplls with >> >separated pin objects as you have suggested (and take its complexity into >> >driver) or just share them. >> > >> >> >> >>3) I don't like the concept of muxed pins and hierarchies of pins. Why >> >> does user care? If pin is muxed, the rest of the pins related to this >> >> one should be in state disabled/disconnected. The user only cares >> >> about to see which pins are related to each other. It can be easily >> >> exposed by "muxid" like this: >> >> pin 1 >> >> pin 2 >> >> pin 3 muxid 100 >> >> pin 4 muxid 100 >> >> pin 5 muxid 101 >> >> pin 6 muxid 101 >> >> In this example pins 3,4 and 5,6 are muxed, therefore the user knows >> >> if he connects one, the other one gets disconnected (or will have to >> >> disconnect the first one explicitly first). >> > >> >Currently DPLLA_PIN_PARENT_IDX is doing the same thing as you described, it >> >groups MUXed pins, the parent pin index here was most straightforward to me, >> >> There is a big difference if we model flat list of pins with a set of >> attributes for each, comparing to a tree of pins, some acting as leaf, >> node and root. Do we really need such complexicity? What value does it >> bring to the user to expose this? > >The fact that you can't auto select from devices behind muxes. Why? What's wrong with the mechanism I described in another part of this thread? Extending my example from above pin 1 source pin 2 output pin 3 muxid 100 source pin 4 muxid 100 source pin 5 muxid 101 source pin 6 muxid 101 source pin 7 output User now can set individial prios for sources: dpll x pin 1 set prio 10 etc The result would be: pin 1 source prio 10 pin 2 output pin 3 muxid 100 source prio 8 pin 4 muxid 100 source prio 20 pin 5 muxid 101 source prio 50 pin 6 muxid 101 source prio 60 pin 7 output Now when auto is enabled, the pin 3 is selected. Why would user need to manually select between 3 and 4? This is should be abstracted out by the driver. Actually, this is neat as you have one cmd to do selection in manual mode and you have uniform way of configuring/monitoring selection in autosel. Would the muxed pin make this better? For muxed pin being output, only one pin from mux would be set: pin 1 source pin 2 output pin 3 muxid 100 disconnected pin 4 muxid 100 disconnected pin 5 muxid 101 output pin 6 muxid 101 disconnected pin 7 output >The HW topology is of material importance to user space. Interesting. When I was working on linecards, you said that the user does not care about the inner HW topology. And it makes sense. When things could be abstracted out to make iface clean, I think they should. >How many times does Arkadiusz have to explain this :| Pardon my ignorance, I don't see why exactly we need mux hierarchy (trees) exposed to user. 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 20585C4708E for ; Wed, 7 Dec 2022 14:53:06 +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=54ue7JeuN4q3E79Fay5Jjl4YbLk4lRlrIuGTJ/L9P+w=; b=NJam5zbjDPtZCa /MAZ8wczwhopiufV/qwmm6IFn9bXkhkRvizEwbyS5gIwe2PwIJ9t58+miwprY6MvtHYjTxUVM7lDJ F+3Xb1KrH6kL1a0Z/yBcpL0uRqAqQXQ72vG7M7BPJTPdU9IBE1rriGyKCIBHeYRX3XDFyvnhNBieT hwqfDx23MyVnTzXnKWI31q0icqo4WWd14VY9DMN2wWJ2TiJ7HDhv279S8UuvOt9joyaTTMj/1aj3U 5NS1zVw/ShHO6veEvHipWh1eNSI3kzo7uxi3rIafBLFArEgsZjLmIzlpkkwGn7+sJsUlZXGO0tbXT jGKwaDGzkLOdqyUL8gvw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1p2vm5-005LCX-BG; Wed, 07 Dec 2022 14:51:53 +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 1p2vlz-005L2A-Rg for linux-arm-kernel@lists.infradead.org; Wed, 07 Dec 2022 14:51:50 +0000 Received: by mail-ej1-x630.google.com with SMTP id t17so14609522eju.1 for ; Wed, 07 Dec 2022 06:51:45 -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=YOoLyltdiLP2JEEnnOjDDBjmowt4f2XyOeFTDUjuF0o=; b=8LPvLa5f8GWVmsFwwTg1U5kbOjmfPOIweDVg0JgDkU66U1Ai1CbGUzeH5erXRynjOi bpCw2zeV3JTSuD4QxVcpaCSD65NM5j/LicODL+gOZ/fyGnZm2JdE/LAmtPIs08k3OafD gzY126/r2xXz3k3USm6gxKL86ySXNxid+nHRkNG9XNIXdoisTW6DyfyWeTrF+CE7MuvH phmUUNN96h8w6chWW22UHnVkGmKHWUs1ME3SN0+GJGAZs1eY3Peh+8H1UGD9QwSobNdY kNDYVcWIBg/M02ThYpRLRfaSzbvWC9q7qVOtW8q9ueoTalt19EPU8s/8Tbgnk1gMB+Bs oX/A== 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=YOoLyltdiLP2JEEnnOjDDBjmowt4f2XyOeFTDUjuF0o=; b=Vs1rZOKNN2muKpPOpQBMZVCjMKrV5qD2itmQEqmkAJwNVMvPR29ebiPM2DWKngI37g 2l6w2TNfv65QcVkit9JJ8tje4pD6DX8PgnX5fNSMuX/CKNStiFZScosOBHlnuSB1jRCj 8TJCK62TXqgNFnaW4PLa8SrGGJvsnDs1P0s2VUl6R1G/lSEyaXzM1o2DFuEX+tJ2D9nl gct3COM5lXfmJ9wFzwGpBBfqADTkY2ge/8SNga8xiGMWuexhihcAuVcx9q9cI3jbLq69 AxGirxOrVoCcPfy2uYB/TUxyEg5UNTVKMs8w1UhjQ1w7XZp1n7H8t84oCfDP6N+H/F1L cHnQ== X-Gm-Message-State: ANoB5plcFtYzkwdY8zS6aimmK4q5og0JmMSWNT+KBUvL1F1QdbKWeEpW G62xmfPRnYI/w5NSUid3KnRi4Q== X-Google-Smtp-Source: AA0mqf4OhmBvoNlAGnMbABNrcBrV4WGf+zkPhfefEbSuw49mlb/2DuiPLNVZwEQyVvDzmJlJx3pPTA== X-Received: by 2002:a17:906:39c8:b0:7ad:79c0:5482 with SMTP id i8-20020a17090639c800b007ad79c05482mr67114184eje.730.1670424704218; Wed, 07 Dec 2022 06:51:44 -0800 (PST) Received: from localhost (host-213-179-129-39.customer.m-online.net. [213.179.129.39]) by smtp.gmail.com with ESMTPSA id s26-20020a056402015a00b00461bacee867sm2294555edu.25.2022.12.07.06.51.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 07 Dec 2022 06:51:43 -0800 (PST) Date: Wed, 7 Dec 2022 15:51:42 +0100 From: Jiri Pirko To: Jakub Kicinski Cc: "Kubalewski, Arkadiusz" , Vadim Fedorenko , Jonathan Lemon , Paolo Abeni , "netdev@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-clk@vger.kernel.org" Subject: Re: [RFC PATCH v4 0/4] Create common DPLL/clock configuration API Message-ID: References: <20221129213724.10119-1-vfedorenko@novek.ru> <20221206184740.28cb7627@kernel.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20221206184740.28cb7627@kernel.org> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20221207_065148_168155_FF8A697E X-CRM114-Status: GOOD ( 34.60 ) 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, Dec 07, 2022 at 03:47:40AM CET, kuba@kernel.org wrote: >On Fri, 2 Dec 2022 17:12:06 +0100 Jiri Pirko wrote: >> >But this is only doable with assumption, that the board is internally capable >> >of such internal board level communication, which in case of separated >> >firmwares handling multiple dplls might not be the case, or it would require >> >to have some other sw component feel that gap. >> >> Yep, you have the knowledge of sharing inside the driver, so you should >> do it there. For multiple instances, use in-driver notifier for example. > >No, complexity in the drivers is not a good idea. The core should cover >the complexity and let the drivers be simple. Really, even in case only one driver actually consumes the complexicity? I understand having a "libs" to do common functionality for drivers, even in case there is one. But this case, I don't see any benefit. > >> >For complex boards with multiple dplls/sync channels, multiple ports, >> >multiple firmware instances, it seems to be complicated to share a pin if >> >each driver would have own copy and should notify all the other about changes. >> > >> >To summarize, that is certainly true, shared pins idea complicates stuff >> >inside of dpll subsystem. >> >But at the same time it removes complexity from all the drivers which would use >> >> There are currently 3 drivers for dpll I know of. This in ptp_ocp and >> mlx5 there is no concept of sharing pins. You you are talking about a >> single driver. >> >> What I'm trying to say is, looking at the code, the pin sharing, >> references and locking makes things uncomfortably complex. You are so >> far the only driver to need this, do it internally. If in the future >> other driver appears, this code would be eventually pushed into dpll >> core. No impact on UAPI from what I see. Please keep things as simple as >> possible. > >But the pin is shared for one driver. Who cares if it's not shared in >another. The user space must be able to reason about the constraints. Sorry, I don't follow :/ Could you please explain what do you mean by this? > >You are suggesting drivers to magically flip state in core objects >because of some hidden dependencies?! It's not a state flip. It's more like a well propagated event of a state change. The async changes may happen anyway, so the userspace needs to handle them. Why is this different? > >> >it and is easier for the userspace due to common identification of pins. >> >> By identification, you mean "description" right? I see no problem of 2 >> instances have the same pin "description"/label. >> >> >This solution scales up without any additional complexity in the driver, >> >and without any need for internal per-board communication channels. >> > >> >Not sure if this is good or bad, but with current version, both approaches are >> >possible, so it pretty much depending on the driver to initialize dplls with >> >separated pin objects as you have suggested (and take its complexity into >> >driver) or just share them. >> > >> >> >> >>3) I don't like the concept of muxed pins and hierarchies of pins. Why >> >> does user care? If pin is muxed, the rest of the pins related to this >> >> one should be in state disabled/disconnected. The user only cares >> >> about to see which pins are related to each other. It can be easily >> >> exposed by "muxid" like this: >> >> pin 1 >> >> pin 2 >> >> pin 3 muxid 100 >> >> pin 4 muxid 100 >> >> pin 5 muxid 101 >> >> pin 6 muxid 101 >> >> In this example pins 3,4 and 5,6 are muxed, therefore the user knows >> >> if he connects one, the other one gets disconnected (or will have to >> >> disconnect the first one explicitly first). >> > >> >Currently DPLLA_PIN_PARENT_IDX is doing the same thing as you described, it >> >groups MUXed pins, the parent pin index here was most straightforward to me, >> >> There is a big difference if we model flat list of pins with a set of >> attributes for each, comparing to a tree of pins, some acting as leaf, >> node and root. Do we really need such complexicity? What value does it >> bring to the user to expose this? > >The fact that you can't auto select from devices behind muxes. Why? What's wrong with the mechanism I described in another part of this thread? Extending my example from above pin 1 source pin 2 output pin 3 muxid 100 source pin 4 muxid 100 source pin 5 muxid 101 source pin 6 muxid 101 source pin 7 output User now can set individial prios for sources: dpll x pin 1 set prio 10 etc The result would be: pin 1 source prio 10 pin 2 output pin 3 muxid 100 source prio 8 pin 4 muxid 100 source prio 20 pin 5 muxid 101 source prio 50 pin 6 muxid 101 source prio 60 pin 7 output Now when auto is enabled, the pin 3 is selected. Why would user need to manually select between 3 and 4? This is should be abstracted out by the driver. Actually, this is neat as you have one cmd to do selection in manual mode and you have uniform way of configuring/monitoring selection in autosel. Would the muxed pin make this better? For muxed pin being output, only one pin from mux would be set: pin 1 source pin 2 output pin 3 muxid 100 disconnected pin 4 muxid 100 disconnected pin 5 muxid 101 output pin 6 muxid 101 disconnected pin 7 output >The HW topology is of material importance to user space. Interesting. When I was working on linecards, you said that the user does not care about the inner HW topology. And it makes sense. When things could be abstracted out to make iface clean, I think they should. >How many times does Arkadiusz have to explain this :| Pardon my ignorance, I don't see why exactly we need mux hierarchy (trees) exposed to user. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel