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 E6109C54731 for ; Tue, 27 Aug 2024 18:30:29 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To: Content-Transfer-Encoding:Content-Type:MIME-Version:References:Subject:Cc:To: From:Date:Message-ID:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=WI1N1DgwDsJPwlRve7vkKC6KRtuD+miK3xK7WkyW/yM=; b=M2wGpiBjivsNFvt5mYH399R5Py E9iDzcT6KkzmVbmsyeUhkQXmQm+Zd8+XrloC8RMOXIgT6s3bYy59lnJXo7TeXjDZq5sl5Ec73ChOw PlNtyW2dCdundT8pl2B4eiEAyif91n/ydIZjCvY/tDvhvfJRSBzbPzdav6OMZZdb1+so10Wg8kQGW DpiRzTbtRIwog0rR8kVoI3nc2H9DU85OsJeYlaPkZ0IOyOqWw6RGjH4z04nDEt1yU/E9r63qsaeHf Zm9l49TE7oz0hMyPb0XHICBIogG/KP5COF7kUu3XohWgCC/iTmACahX1UwZsoIeONJJe3b3gFeGtQ xAAgRa1A==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sj0xO-0000000CVKr-1SL5; Tue, 27 Aug 2024 18:30:18 +0000 Received: from mail-wm1-x32c.google.com ([2a00:1450:4864:20::32c]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sj0wa-0000000CVF5-00bg; Tue, 27 Aug 2024 18:29:29 +0000 Received: by mail-wm1-x32c.google.com with SMTP id 5b1f17b1804b1-4281c164408so50380095e9.1; Tue, 27 Aug 2024 11:29:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1724783365; x=1725388165; darn=lists.infradead.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:subject:cc:to:from:date:message-id:from:to :cc:subject:date:message-id:reply-to; bh=WI1N1DgwDsJPwlRve7vkKC6KRtuD+miK3xK7WkyW/yM=; b=frJGruMWlZ96Z8PNvFxrjB8tGHV8bwq58MB9iULOprhQj3Ux30o/ZXqdnebGxlTatj VTGgx0oUC29mC2C38B8+w6IwXhtyQ/Pm0h8A06gvXWCaBO4auE+NNfpeiKE9vg+b1wmU MdU96bxr9NqvSnKV8HpXPEroT9vWQwJiiyDRpWF8Irhl6vs2cl3BzYmII2c9NwfLzSHr a/gcwXs4K5JxQFctOGk/3iQ8FhKy+QDjiaaXIqF2FqpbgPeykEWirh00Lnlr2IzqR+ue QjEa2TEbacYR6yznz+GTM57sn1IzhYZHIHJcAEWGB62bdmTJJxne1O52t8x92btvok1Q FPgA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1724783365; x=1725388165; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:subject:cc:to:from:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=WI1N1DgwDsJPwlRve7vkKC6KRtuD+miK3xK7WkyW/yM=; b=h8pc6nFPhPpVQDTUJ61HVMKHDoApYymiL2a8Ho4MH8ixjBGE3UmArGFXywNa/eP9y/ r/6azjeqRsqfYl7rAFbjVvcwdBYShCEwzgD5MtpA7jx8M0VZCgd9Qe89P9fjbUHwuxkn rUP8pDDMTC123RRR/inH+h8HmFssNYuGWFs2rwqO9CreTRS7DfuibmZEGYrKelGKFrzj BFhaAMfq85q81IDT/U8dGColAT2TuUpFodzqYtKSqvNuwaL6UTGmEvhCOv8vmtfaI6y+ +2Zx6317ziCMIDvUjc3onxN5y5Hy6vLNmP/PkB/9D1vFyGqkDQhurgqiLm1g0AEUe6yQ p2TA== X-Forwarded-Encrypted: i=1; AJvYcCUkyJoxtt9Hjj2dsxER9jjfcvakgip6/8wgu5B6ZtMldoovydsq406T8/xZlSUnZA/q1ES/4fZL+Ijan9cEtDY=@lists.infradead.org, AJvYcCXNo++VC1fWSD02pHRtXOHSqqRWFnc0lmpIhDf51zsQWyQSV6FQAzuR5Rfq0q/8xHXGp+yKJ0XrLcBqznPw9QiE@lists.infradead.org X-Gm-Message-State: AOJu0YyAQJHjfL1ICCtlJ1iXCCTUiOUbPfjmFMemsQ7tHdWCUJVHVW6t 5SNuyRedCn/xWYS0PIp4VPH79RrtWuaemzqpJI0Ctsurj1qGUJMa X-Google-Smtp-Source: AGHT+IEh8e9df02+VsFiaG+m2FW0LXgC8j4lKzwsfi4bbOggJsY8PNcGwyF+AYmZjvqrw4VScPcODg== X-Received: by 2002:a5d:4903:0:b0:368:d0d:a5d6 with SMTP id ffacd0b85a97d-3748c81a27cmr1883813f8f.50.1724783364861; Tue, 27 Aug 2024 11:29:24 -0700 (PDT) Received: from Ansuel-XPS. (host-87-1-209-141.retail.telecomitalia.it. [87.1.209.141]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-3730817a2f4sm13703619f8f.61.2024.08.27.11.29.23 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 27 Aug 2024 11:29:24 -0700 (PDT) Message-ID: <66ce1b04.df0a0220.a2131.6def@mx.google.com> X-Google-Original-Message-ID: Date: Tue, 27 Aug 2024 20:29:20 +0200 From: Christian Marangi To: Rob Herring Cc: Lorenzo Bianconi , Conor Dooley , Lorenzo Bianconi , Benjamin Larsson , Linus Walleij , Krzysztof Kozlowski , Conor Dooley , Sean Wang , Matthias Brugger , AngeloGioacchino Del Regno , linux-mediatek@lists.infradead.org, linux-gpio@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, upstream@airoha.com Subject: Re: [PATCH v2 1/2] dt-bindings: pinctrl: airoha: Add EN7581 pinctrl controller References: <20240822-en7581-pinctrl-v2-0-ba1559173a7f@kernel.org> <20240822-en7581-pinctrl-v2-1-ba1559173a7f@kernel.org> <20240822-taste-deceptive-03d0ad56ae2e@spud> <20240823-darkened-cartload-d2621f33eab8@spud> <66c8c50f.050a0220.d7871.f209@mx.google.com> <20240826-kinsman-crunching-e3b75297088c@spud> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240827_112928_075821_39F76E84 X-CRM114-Status: GOOD ( 65.17 ) 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: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Aug 27, 2024 at 09:35:07AM -0500, Rob Herring wrote: > On Tue, Aug 27, 2024 at 3:47 AM Lorenzo Bianconi > wrote: > > > > > > > > On Fri, Aug 23, 2024 at 11:17:34PM +0200, Lorenzo Bianconi wrote: > > > > > On Fri, Aug 23, 2024 at 05:14:30PM +0100, Conor Dooley wrote: > > > > > > On Thu, Aug 22, 2024 at 10:50:52PM +0200, Benjamin Larsson wrote: > > > > > > > On 22/08/2024 18:06, Conor Dooley wrote: > > > > > > > > > > > > > > > > > > > > > Hi. > > > > > > > > > > > > > > > before looking at v1: > > > > > > > > I would really like to see an explanation for why this is a correct > > > > > > > > model of the hardware as part of the commit message. To me this screams > > > > > > > > syscon/MFD and instead of describing this as a child of a syscon and > > > > > > > > using regmap to access it you're doing whatever this is... > > > > > > > > > > > > > > Can you post a link to a good example dts that uses syscon/MFD ? > > > > > > > > > > > > > > It is not only pinctrl, pwm and gpio that are entangled in each other. A > > > > > > > good example would help with developing a proper implementation. > > > > > > > > > > > > Off the top of my head, no unfortunately. Maybe Rob or Krzk have a good > > > > > > example. I would suggest to start by looking at drivers within gpio or > > > > > > pinctrl that use syscon_to_regmap() where the argument is sourced from > > > > > > either of_node->parent or dev.parent->of_node (which you use depends on > > > > > > whether or not you have a child node or not). > > > > > > > > > > > > I recently had some questions myself for Rob about child nodes for mfd > > > > > > devices and when they were suitable to use: > > > > > > https://lore.kernel.org/all/20240815200003.GA2956351-robh@kernel.org/ > > > > > > > > > > > > Following Rob's line of thought, I'd kinda expect an mfd driver to create > > > > > > the devices for gpio and pwm using devm_mfd_add_devices() and the > > > > > > pinctrl to have a child node. > > > > > > > > > > Just to not get confused and staring to focus on the wrong kind of > > > > > API/too complex solution, I would suggest to check the example from > > > > > Lorenzo. > > > > > > > > > > The pinctrl/gpio is an entire separate block and is mapped separately. > > > > > What is problematic is that chip SCU is a mix and address are not in > > > > > order and is required by many devices. (clock, pinctrl, gpio...) > > > > > > > > > > IMHO a mfd is overkill and wouldn't suite the task. MDF still support a > > > > > single big region and in our case we need to map 2 different one (gpio > > > > > AND chip SCU) (or for clock SCU and chip SCU) > > > > > > > > > > Similar problem is present in many other place and syscon is just for > > > > > the task. > > > > > > > > > > Simple proposed solution is: > > > > > - chip SCU entirely mapped and we use syscon > > > > > > That seems reasonable. > > > > ack > > > > > > > > > > - pinctrl mapped and reference chip SCU by phandle > > > > > > ref by phandle shouldn't be needed here, looking up by compatible should > > > suffice, no? > > > > ack, I think it would be fine > > > > > > > > > > - pwm a child of pinctrl as it's scrambled in the pinctrl mapped regs > > > > > > The pwm is not a child of the pinctrl though, they're both subfunctions of > > > the register region they happen to both be in. I don't agree with that > > > appraisal, sounds like an MFD to me. > > > > ack > > > > > > > > > > Hope this can clear any confusion. > > > > > > > > To clarify the hw architecture we are discussing about 3 memory regions: > > > > - chip_scu: <0x1fa20000 0x384> > > > > - scu: <0x1fb00020 0x94c> > > > ^ > > > I'm highly suspicious of a register region that begins at 0x20. What is > > > at 0x1fb00000? > > > > sorry, my fault > > > > > > > > > - gpio: <0x1fbf0200 0xbc> > > > > > > Do you have a link to the register map documentation for this hardware? > > > > > > > The memory regions above are used by the following IC blocks: > > > > - clock: chip_scu and scu > > > > > > What is the differentiation between these two different regions? Do they > > > provide different clocks? Are registers from both of them required in > > > order to provide particular clocks? > > > > In chip-scu and scu memory regions we have heterogeneous registers. > > Regarding clocks in chip-scu we have fixed clock registers (e.g. spi > > clock divider, switch clock source and divider, main bus clock source > > and divider, ...) while in scu (regarding clock configuration) we have > > pcie clock regs (e.g. reset and other registers). This is the reason > > why, in en7581-scu driver, we need both of them, but we can access > > chip-scu via the compatible string (please take a look at the dts > > below). > > > > > > > > > - pinctrl (io-muxing/gpio_chip/irq_chip): chip_scu and gpio > > > > > > Ditto here. Are these actually two different sets of iomuxes, or are > > > registers from both required to mux a particular pin? > > > > Most of the io-muxes configuration registers are in chip-scu block, > > just pwm ones are in gpio memory block. > > Gpio block is mainly used for gpio_chip and irq_chip functionalities. > > > > > > > > > - pwm: gpio > > > > > > > > clock and pinctrl devices share the chip_scu memory region but they need to > > > > access even other separated memory areas (scu and gpio respectively). > > > > pwm needs to just read/write few gpio registers. > > > > As pointed out in my previous email, we can define the chip_scu block as > > > > syscon node that can be accessed via phandle by clock and pinctrl drivers. > > > > clock driver will map scu area while pinctrl one will map gpio memory block. > > > > pwm can be just a child of pinctrl node. > > > > > > As I mentioned above, the last statement here I disagree with. As > > > someone that's currently in the process of fixing making a mess of this > > > exact kind of thing, I'm going to strongly advocate not taking shortcuts > > > like this. > > > > > > IMO, all three of these regions need to be described as syscons in some > > > form, how exactly it's hard to say without a better understanding of the > > > breakdown between regions. > > > > > > If, for example, the chip_scu only provides a few "helper" bits, I'd > > > expect something like > > > > > > syscon@1fa20000 { > > > compatible = "chip-scu", "syscon"; > > > reg = <0x1fa2000 0x384>; > > > }; > > > > > > syscon@1fb00000 { > > > compatible = "scu", "syscon", "simple-mfd"; > > > #clock-cells = 1; > > > }; > > > > > > syscon@1fbf0200 { > > > compatible = "gpio-scu", "syscon", "simple-mfd"; > > > #pwm-cells = 1; > > > > > > pinctrl@x { > > > compatible = "pinctrl"; > > > reg = x; > > > #pinctrl-cells = 1; > > > #gpio-cells = 1; > > > }; > > > }; > > > > > > > ack, so we could use the following dts nodes for the discussed memory > > regions (chip-scu, scu and gpio): > > > > syscon@1fa20000 { > > compatible = "airoha,chip-scu", "syscon"; > > reg = <0x0 0x1fa2000 0x0 0x384>; > > }; > > > > clock-controller@1fb00000 { > > compatible = "airoha,en7581-scu", "syscon"; > > reg = <0x0 0x1fb00000 0x0 0x94c>; > > #clock-cells = <1>; > > #reset-cells = <1>; > > }; > > > > mfd@1fbf0200 { > > compatible = "airoha,en7581-gpio-mfd", "simple-mfd"; > > reg = <0x0 0x1fbf0200 0x0 0xc0>; > > > > pio: pinctrl { > > compatible = "airoha,en7581-pinctrl" > > gpio-controller; > > #gpio-cells = <2>; > > > > interrupt-controller; > > #interrupt-cells = <2>; > > interrupt-parent = <&gic>; > > interrupts = ; > > } > > > > pwm: pwm { > > compatible = "airoha,en7581-pwm"; > > #pwm-cells = <3>; > > } > > }; > > I think this can be simplified down to this: > > mfd@1fbf0200 { > compatible = "airoha,en7581-gpio-mfd"; // MFD is a Linuxism. What > is this h/w block called? > reg = <0x0 0x1fbf0200 0x0 0xc0>; > gpio-controller; > #gpio-cells = <2>; > interrupt-controller; > #interrupt-cells = <2>; > interrupts = ; > > #pwm-cells = <3>; > > pio: pinctrl { > foo-pins {}; > bar-pins {}; > }; > }; > > Maybe we keep the compatible in 'pinctrl'... > Hi Rob, thanks a lot for the hint, I hope we can finally find a solution on how to implement this. In Documentation the block is called GPIO Controller. As explained it does expose pinctrl function AND pwm (with regs in the middle) Is this semplification really needed? It does pose some problem driver wise (on where to put the driver, in what subsystem) and also on the yaml side with mixed property for pinctrl and pwm controller. I feel mixing the 2 thing might cause some confusion on the 2 block device that are well separated aside from the unlucky position of the regs. The suggested MFD implementation would consist of - main node with MFD (map the entire GPIO controller regs) - 2 child for PWM and pinctrl (no regs) - driver in mfd/ - driver in pinctrl/ - driver in pwm/ An alternative is the previous solution with pinctrl mapping all the GPIO controller regs and PWM a child but Conor suggested that a MFD structure might be better suited for the task. We have both implemented and ready to be submitted. Hope we can find a common decision on how to implement this simple but annoying block of devices. -- Ansuel