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 C0C75EB64D8 for ; Fri, 16 Jun 2023 15:59:15 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 658D485BFB; Fri, 16 Jun 2023 17:59:13 +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 996EC85F2B; Fri, 16 Jun 2023 17:59:11 +0200 (CEST) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by phobos.denx.de (Postfix) with ESMTP id 88D468574A for ; Fri, 16 Jun 2023 17:59:07 +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 D80C81FB; Fri, 16 Jun 2023 08:59:50 -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 68FB43F71E; Fri, 16 Jun 2023 08:59:06 -0700 (PDT) Date: Fri, 16 Jun 2023 16:59:02 +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: <20230616165902.0b6eea86@donnerap.cambridge.arm.com> In-Reply-To: References: <20221206004549.29015-1-andre.przywara@arm.com> <04c52801-a6d0-033b-c7f4-613e0b789d96@gmail.com> <20230612012056.65883683@slackpad.lan> 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 Mon, 12 Jun 2023 15:18:17 -0600 Sam Edwards wrote: Hi Sam, something regarding "reset" below ... > On 6/11/23 18:20, Andre Przywara wrote: > > Thanks for the update and the list! Can you confirm where you > > still needed code changes compared to say my github branch plus the > > changes we already discussed? Trying some guesses below, please confirm > > or deny: =20 >=20 > Preeeeetttttyyy much everything I've changed locally has been submitted=20 > to the list or discussed in the relevant patchset. Have you updated your= =20 > GitHub branch recently (past couple of weeks)? I haven't been watching=20 > it but I can pull it again and see which of my local changes are still=20 > required. >=20 > >> I have a build of U-Boot for my target, complete with: > >> - UART3 initialized correctly =20 > >=20 > > this is problematic because of the other pinmux used on your board, > > which cannot easily be encoded alongside the existing UART3 pinmux? =20 >=20 > Actually no, my board's UART3 is on PB6/PB7, nice and standard. >=20 > >> - DRAM coming up correctly > >> - SPL sets configured boot clock correctly =20 > >=20 > > This should work as per github? =20 >=20 > Yep, everything was working satisfactorily once I figured out I needed=20 > to set CONFIG_SYS_CLK_FREQ to get acceptable boot speeds. >=20 > >> - SPI-NAND support (SPL and U-Boot proper) =20 > >=20 > > This is with Icenow's series? Any D1 specific changes needed there? =20 >=20 > Yes, with Icenowy's series (322733). >=20 > I learned that the BROM sets the boot medium code to 0x04 when it's an=20 > SPI-*NAND* (while older chips used 0x03 for "it's either SPI-NOR or=20 > SPI-NAND, good luck figuring out which"). Since `env_get_location`=20 > assumes BOOT_DEVICE_SPI is NOR (and my target needs to load env from UBI= =20 > iff booting from NAND), I'm hoping I can convince Icenowy to separate=20 > out the SPI-NAND and SPI-NOR load methods entirely (vs. her current=20 > try-NAND-then-NOR approach) with the aid of some disambiguation logic to= =20 > probe for an SPI-NAND on the older chips known to report these as 0x03. >=20 > I also needed Maksim's patch series (355747) to support the D1 SPI master. >=20 > >> - MMC support (SPL and U-Boot proper) > >> - SPL boot from FEL =20 > >=20 > > again worked already in github? =20 >=20 > Yes and yes. I was just confirming they're good; no local work needed=20 > from me here. >=20 > >> - USB gadget support =20 > >=20 > > So with the fixed SUNXI_SRAMC_BASE you said it worked? What about the > > USB PHY? That needs at least wiring in the compatible string? If you > > have such a patch, can you please rebase it on top of my v2 USB PHY > > series and post that? =20 >=20 > Yes, with corrected SUNXI_SRAMC_BASE -- though I also needed my patches=20 > to make musb-new/sunxi.c use the UDC gadget model in DM (series 358842),= =20 > as I don't think there's another way to init the controller at runtime. >=20 > I can't say whether the endpoint limit is correct or that mUSB *host*=20 > operation works. >=20 > The USB PHY only required that CONFIG_PHY_SUN4I_USB be enabled. The=20 > correct compatible is already wired up. It does look like your PHY=20 > series drops the explicit requirement to set PHY_SUN4I_USB so that's=20 > better than what I was doing (adding a `select` directive under R528). >=20 > I can test your series if you want but I doubt it intersects my work=20 > here in any significant way. >=20 > >> - Ethernet MAC+PHY support =20 > >=20 > > Anything surprising here? Is that using an already supported external > > PHY? =20 >=20 > The only surprise was this was how I noticed that CONFIG_CLK_SUN20I_D1=20 > was not being implicitly enabled. Enabling that was then how I found=20 > that the clock driver wasn't compatible with current upstream (which I=20 > already mentioned). >=20 > The PHY is external and already supported, adding it to my DT required=20 > very little work. >=20 > >> - I=C2=B2C support * > >> - GPIO support (LEDs, buttons, misc. board management) =20 > >=20 > > again should work out of the box, minus your board specific > > configuration? =20 >=20 > GPIO is completely OotB, yes. I=C2=B2C is OotB once MVTWSI_CONTROL_CLEAR_= IFLG=20 > is set correctly. (The pinctrl requirements for it are a little harder,=20 > more on that below.) >=20 > >> - `reset` working (requries CONFIG_SYSRESET unset, WDT key) =20 > >=20 > > Isn't "CONFIG_SYSRESET unset" a hack? I dimly remember we had this for > > some other SoC initially, but later got rid of it? =20 >=20 > Unsetting it is required for reset_cpu() to be defined. Your patchset=20 > updates that function (albeit without adding the WDT key, so the current= =20 > patch is broken) to support NCAT2 already. U-Boot has no driver for=20 > "allwinner,sun20i-d1-wdt-reset", so this is the only way for `reset` to=20 > work. So I had a look at this, and it's a bit surprising: The watchdog driver already supports "allwinner,sun20i-d1-wdt" for a while. We don't need to list the "-reset" version, because the normal compatible name acts as a fallback string. However the DT node in the base .dtsi sets: status =3D "reserved"; for this (and the other) watchdog, so U-Boot's DM (correctly!) ignores those devices. Trying to figure out why. Adding: &wdt { status =3D "okay"; }; to sun8i-t113s.dtsi fixes that for me, now the reset command works. > > For the WDT key: it seems like Linux got a nice patch to integrate this > > neatly into the driver without quirking this too much, do you have > > something ready for U-Boot as well? Would love to see it on the list > > then ;-) =20 >=20 > I just hacked the correct value into the function; nothing really=20 > suitable for the list, sorry. CONFIG_SYSRESET is only applicable for U-Boot proper, so we need reset_cpu for the SPL still. That is not as critical, since the SPL resets the board only in case of an error, but should be fixed nevertheless. I will have a stab at this. Cheers, Andre > >> - PSCI, nonsec =20 > >=20 > > ah yeah, owe you some reviews on this one ... =20 >=20 > It occurs to me that my work *might* support H6 as well (they both use=20 > CPUX blocks, right?) so perhaps it would be better if I de-RFC'd my=20 > series and instead worked to upstream it chasing H6, for you to come=20 > along later and tack on NCAT2 support with your R528 patchset? >=20 > >> - Able to boot Linux ;) > >> > >> * Requires nonzero `MVTWSI_CONTROL_CLEAR_IFLG` for NCAT2, and a patch = to > >> the pinctrl driver to configure the proper mux function for my necessa= ry > >> pins. =20 > >=20 > > Are those pinmuxes straight forward to add to the pinctrl driver? Or > > are there conflicts similar to UART3? =20 >=20 > The conflict is that I'm on i2c2 + muxval 2. I suspect this one's going=20 > to be a downstream patch to add the necessary line: > { "i2c2", 2 }, /* PE12-PE13 */ >=20 > ...and since no other assignment for i2c2 uses muxval 2, the only hope=20 > for this to be supported upstream would be for the pinctrl driver to=20 > include the full pin->muxval LUT. >=20 > >> I figured I'd share this list as a sort of checklist for your own work, > >> too. The remainder of my efforts now will probably be focused on > >> mainlining this stuff (let me know how else I can be of help), and then > >> I'm afraid I'll have to disappear back downstream to the Turing Pi 2 > >> development effort, but maybe our paths will cross again in the kernel > >> lists. :) =20 > >=20 > > Yeah, as you may know, the DT has to go through the kernel list. DT > > patches can be tedious to upstream, there is now much attention to > > every detail. Running checkpatch and dtbs_check should reveal most > > issues beforehand, though. =20 >=20 > At this time I have no interest in upstreaming the DT. That might change= =20 > in the future, but for now it's very much meant to be out-of-tree. >=20 > > Cheers, > > Andre =20 >=20 > Likewise, > Sam