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 phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 1DD5DEB64D7 for ; Tue, 20 Jun 2023 12:42:58 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id C8105862B6; Tue, 20 Jun 2023 14:42:55 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Received: by phobos.denx.de (Postfix, from userid 109) id 079BE8621C; Tue, 20 Jun 2023 14:42:54 +0200 (CEST) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by phobos.denx.de (Postfix) with ESMTP id 0C3C6862B6 for ; Tue, 20 Jun 2023 14:42:47 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=andre.przywara@arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id F11661063; Tue, 20 Jun 2023 05:43:29 -0700 (PDT) Received: from donnerap.cambridge.arm.com (usa-sjc-imap-foss1.foss.arm.com [10.121.207.14]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id A8A1E3F71E; Tue, 20 Jun 2023 05:42:45 -0700 (PDT) Date: Tue, 20 Jun 2023 13:42:43 +0100 From: Andre Przywara To: Sam Edwards Cc: u-boot@lists.denx.de Subject: Re: [RFC PATCH 00/17] sunxi: rework pinctrl and add T113s support Message-ID: <20230620134243.3af983b8@donnerap.cambridge.arm.com> In-Reply-To: <71369065-8727-c011-0f29-fa33e0eb785c@gmail.com> References: <20221206004549.29015-1-andre.przywara@arm.com> <04c52801-a6d0-033b-c7f4-613e0b789d96@gmail.com> <20230612012056.65883683@slackpad.lan> <20230615010754.503f4912@slackpad.lan> <71369065-8727-c011-0f29-fa33e0eb785c@gmail.com> Organization: ARM X-Mailer: Claws Mail 3.18.0 (GTK+ 2.24.32; aarch64-unknown-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.8 at phobos.denx.de X-Virus-Status: Clean On Sun, 18 Jun 2023 13:01:33 -0600 Sam Edwards wrote: Hi Sam, thanks for the reply, that's very helpful. > On 6/14/23 18:07, Andre Przywara wrote: > > So I finally found some time to address some issues in the series, > > especially in the first patches (pinctrl rework and preparation). > > I pushed a branch to https://github.com/apritzel/u-boot/commits/r528-rc > > I need to do more testing, most importantly regression testing on other > > SoCs, and will only be able to post something next week, I guess. > >=20 > > If you could briefly list the things that are still missing, I could > > try to pick some low hanging fruits. =20 >=20 > Rebasing onto this branch ended up eliminating a good chunk of my local=20 > hack commits. I've verified that everything is still working (but have=20 > not yet retested NAND SPL). Great, thanks! > The remaining local changes I have are two additions to cpu_sunxi_ncat2.h: > +#define SUNXI_R_CPUCFG_BASE 0x07000400 /* for PSCI */ > +#define SUNXI_RTC_BASE 0x07090000 /* for FEL */ Right, I will definitely take the PSCI bit, but not so sure about FEL yet. > The former can probably be brought into my PSCI series somehow (unless=20 > we expect more chips with CPUX blocks which might move those soft entry=20 > registers around, then it should be defined in cpu_sunxi_*.h). The=20 > latter is to support a reimplementation of Allwinner's `efex` command=20 > that I'm using for development (it pokes the magic number 0x5AA5A55A=20 > into RTC's GP_DATA_REG[2] and then resets; SPL clears that magic number=20 > and then does an early branch to BROM+0x0020 -- exactly what Allwinner's= =20 > fork does). So yeah, the request of a "Enter FEL" command came up multiple times, but so far no one could be bothered to implement this properly. The idea would be to have a generic command (more like "fel-reset" than efex), and allow each SoC (family) to implement this differently, as every SoC requires something a bit different here (32-bit vs. 64-bit, having an RTC vs not, etc). If you could post your solution somewhere, we could start this effort. There was some patch for the H3 already, and it's relatively straight-forward on the newer SoCs (H616, IIRC), so if at least two popular, but different SoCs would be supported, we could make sure to have the right abstractions in place. > I've also noticed exactly(!) one formatting difference in our clk_d1.c: > - .num_gates =3D ARRAY_SIZE(d1_gates), > + .num_gates =3D ARRAY_SIZE(d1_gates), >=20 > Up to you if you prefer to align the =3D or not, but it does look=20 > inconsistent when .gates and .resets are aligned and .num_* aren't -=20 > might be a nitpick that comes up in patch review. Well, this is how it is in all the other clock drivers, so I chose to stay consistent with them ;-) > > Interesting, indeed this is left at 0, which I think will result in 288 > > MHz. =20 >=20 > Correct, at least that's what I was seeing. >=20 > > What is that frequency in your case? Do you know what the BSP > > programs? =20 >=20 > 1008 MHz, both. >=20 > > Traditionally we used something conservative that works > > without cooling and with the default voltage, but I don't know that > > value for the T113s. =20 >=20 > For what it's worth, this board has a bare T113-s3 and the current OS=20 > does not reclock from 1008 MHz at all, and I don't know of any users of=20 > the board having stability issues. >=20 > In my own case, it idles at that clock at around ~35=C2=B0C. OK, many thanks, it looks like 1008 MHz it is, then. > > I think CLK_SUN20I_D1 should be set by default now, so can you check > > that this is fixed? =20 >=20 > It is now gone from my defconfig and still working, so indeed this is fix= ed. >=20 > > Why would we need H6 PSCI support? On the ARMv8 parts we use Trusted > > Firmware-A (TF-A) to provide PSCI services, which has a much more mature > > implementation. =20 >=20 > It's not about the H6 and more about me being unsure whether R528/T113=20 > is the first ARMv7-based SoC to use the new CPU management registers. If= =20 > it's not, and there's another such chip supported in U-Boot that just=20 > lacks PSCI, it would make more sense for me to land my PSCI series=20 > independently of our work here, and then you can add the R528 case=20 > later. It sounds like R528/T113 may be the first such chip needing this=20 > new code, though, so this may have to wait until the R528 series lands. >=20 > > How would this conflict, exactly? I don't see any other I2C2 > > definition? =20 >=20 > Well, no, the other definitions haven't landed in U-Boot yet. But they=20 > do exist in the kernel, datasheets, and physical chips themselves: >=20 > PB0/PB1/PB8/PB9/PE4/PE5: i2c2 defined as muxval 4 > PC0/PC1/PD20/PD21/PG6/PG7/PG14/PG15: i2c2 defined as muxval 3 > PE12/PE13: i2c2 defined as muxval 2 >=20 > Defining i2c2=3D2 universally would mean that the pins for i2c2 cannot be= =20 > changed, since it would conflict with every other definition. Well, we are well aware that the current pinmux code is limited, but we figured it does work for all practical purposes, and U-Boot has a far tighter scope than Linux, so we don't need to support every theoretically possible use case. So if the TuringPi 2 board would be upstream, and you need I2C2, you would just set the precedence for muxval 2. It's then the next board's problem to live with that. Either by adding the port number (PB/PC/PD/PG) to the definitions, or by reverting to a DT solution: https://lore.kernel.org/linux-sunxi/20221110014255.20711-1-andre.przywara@a= rm.com/ > > And what do you need I2C2 for, exactly? =20 >=20 > Pins PE12/PE13 host an I=C2=B2C bus with the board ID EEPROM and an Ether= net=20 > switch that should be reset (and have a few registers set to configure=20 > proper port isolation) very shortly after power-on. >=20 > > Well, there are shortcuts. I sketched some simpler idea in the comment > > at the top of pinctrl-sunxi.c. =20 >=20 > My shortcut for the time being will probably be, "downstream patch." see below ;-) > >> At this time I have no interest in upstreaming the DT. =20 > >=20 > > Why not? > > =20 > >> That might change > >> in the future, but for now it's very much meant to be out-of-tree. =20 > >=20 > > Why is this? This only increases your update burden, and we might break > > something and not realise that, if your DT is not in the tree. > > The question to ask should be rather: why *not* to upstream the DT? > > Please keep in mind that this would block U-Boot support, since we need > > the DT approved in the kernel before we could merge it into U-Boot. =20 >=20 > Currently, downstream is still fairly dependent on the Tina Linux=20 > kernel, not mainline. This is a situation I'd like to change, but it's a= =20 > push for another day -- my focus right now is only on improving the=20 > bootloader situation. Ah, depending on the BSP kernel is indeed quite bad. I wonder what features of the kernel you rely on that upstream does not have? Or is it more about the BMC userland parts that are married to the Allwinner kernel and its own interfaces? > This means that there are actually two DTs: one for the kernel, using=20 > the Tina Linux binding values, and one for U-Boot's control FDT, which=20 > can only support U-Boot right now (and cannot yet be tested on a real=20 > kernel). So neither DT is acceptable upstream: the former uses=20 > incompatible values/includes, and the latter isn't meant for Linux. So as you probably know, conceptually there is only one DT per machine, as the DT describes the hardware. That's why try hard to align U-Boot and Linux at least, and IIUC the BSDs copied the version in the Linux kernel tree as well. Allwinner seems to have little clue or interest here, and we gave up on their (quite misguided) interpretation of the DT a long time ago. > Even after(/if) this situation is resolved, the unified DT will probably= =20 > remain in a state of flux for a while, until some drivers can be updated= =20 > upstream (there's a slight mess with the I=C2=B2C driver that needs to be= =20 > cleaned up and we have to use GPIO-bitbanged I=C2=B2C until then, for=20 > example) so it'll take more work before we have a "final" DT. At *that*=20 > time, upstreaming would be a good idea... Final DT is a noble goal, but in reality there will always be room for improvement and additions. So what we typically do is to start with a simple .dts for the kernel tree, describing the basic peripherals, and everything that already works and is not subject to debate. If in doubt, include a node, and we will comment. Could you prepare such a patch? This should not contradict any DT nodes that U-Boot uses, so it's not a double effort. This would mean we have a *second* board DT for the T113s SoC in the kernel, which always helps to improve quality and prevents hacks that just work on the MangoPi. Besides, the TuringPi board is an actually useful application of the SoC, deployed and available, in contrast to just some development board from Chinese websites. And once this is merged, we could just copy this over to U-Boot and add the defconfig and any other support patches there. > ...but for now it's very much meant to be out-of-tree. :) >=20 > (I also do not work for the company that produced this board -- I'm just= =20 Ah, that would have been a first anyway ;-) > a contributor to the firmware project. Whether the project would even=20 > use the mainline version of its DT in the first place is, though likely,= =20 > ultimately not my call.) Yeah, I understand it's not the most grateful job to chase up on doing things properly and stay on with the upstreaming process. Ultimately it's the right thing to do, though, and will save you hassle over time. Plus we (the community) will help you with that, and you'd get a second commit in the kernel ;-) Cheers, Andre