Re: [PATCH v3] clk: set initial best mux parent to current parent with CLK_MUX_ROUND_CLOSEST

[Maxime Ripard]

On Tue, Mar 12, 2024 at 04:52:29PM +0800, Yang Xiwen wrote:
> On 3/11/2024 5:34 PM, Maxime Ripard wrote:
> > On Thu, Mar 07, 2024 at 07:18:05PM +0800, Yang Xiwen wrote:
> > > On 3/7/2024 4:48 PM, Maxime Ripard wrote:
> > > > Hi,
> > > > 
> > > > On Thu, Mar 07, 2024 at 10:03:50AM +0800, Yang Xiwen via B4 Relay wrote:
> > > > > From: Yang Xiwen 
> > > > > 
> > > > > Originally, the initial clock rate is hardcoded to 0, this can lead to
> > > > > some problem when setting a very small rate with 
> > > > > CLK_MUX_ROUND_CLOSEST.
> > > > > 
> > > > > For example, if the lowest possible rate provided by the mux is 
> > > > > 1000Hz,
> > > > > setting a rate below 500Hz will fail, because no clock can provide a
> > > > > better rate than the non-existant 0Hz. But it should succeed with 
> > > > > 1000Hz
> > > > > being set.
> > > > > 
> > > > > Setting the initial best parent to current parent could solve this 
> > > > > bug.
> > > > > 
> > > > > Signed-off-by: Yang Xiwen 
> > > > I don't think it would be the way to go. The biggest issue to me is that
> > > > it's inconsistent, and only changing the behaviour for a given flag
> > > > doesn't solve that.
> > > 
> > > I think the current behavior is odd but conforms to the document if
> > > CLK_MUX_ROUND_CLOSEST is not specified.
> > clk_mux_determine_rate_flags isn't documented, and the determine_rate
> > clk_ops documentation doesn't mention it can return an error.
> > 
> > > If i understand correctly, the default behavior of mux clocks is to
> > > select the closest rate lower than requested rate, and
> > > CLK_MUX_ROUND_CLOSEST removes the "lower than" limitation, which is
> > > what this version tries to accomplish.
> > The situation is not as clear-cut as you make it to be, unfortunately.
> > The determine_rate clk_ops implementation states:
> > 
> >Given a target rate as input, returns the closest rate actually
> >supported by the clock, and optionally the parent clock that should be
> >used to provide the clock rate.
> > 
> > So CLK_MUX_ROUND_CLOSEST shouldn't exist, because that's what
> > determine_rate expects so it should always be there.
> > 
> > Now, the "actually supported by the clock" can be interpreted in
> > multiple ways, and most importantly, doesn't state what the behaviour is
> > if we can't find a rate actually supported by the clock.
> > 
> > But now, this situation has been ambiguous for a while and thus drivers
> > kind of relied on that ambiguity.
> > 
> > So the way to fix it up is:
> > 
> >- Assess what drivers are relying on
> >- Document the current behaviour in clk_ops determine_rate
> 
> 
> From my investigation, it's totally a mess, especially for platform clk
> drivers (PLL). Some drivers always round down, the others round to nearest,
> with or without a specific flag to switch between them, depend on the
> division functions they choose. Fixing all of them seems needs quite a lot
> of time and would probably introduce some regressions.

I agree it's a mess, but that's a mess you wanted to clean up in the
first place :)

> We'd probably only have to say both rounding to nearest and rounding down
> are acceptable, though either one is preferred.
> 
> 
> >- Make clk_mux_determine_rate_flags consistent with that
> 
> 
> I think we must keep existing flags and document the current behavior
> correctly because of the massive existing users of clk_mux.
>
> That's why i'm going to only fix CLK_MUX_ROUND_CLOSEST users. Hopefully it
> won't cause too many regressions.

Which, without a documentation, makes it more of a mess.

> 
> >- Run that through kernelci to make sure we don't have any regression
> 
> 
> We don't. I run 'tools/testing/kunit/kunit.py run --kunitconfig
> drivers/clk/.kunitconfig' each time before i send patches.

That's kunit, not kernelci (https://linux.kernelci.org/job/)

> 
> Over all, it seems quite a lot of work here.
>
> The situation here becomes even more complex when it comes to U-Boot clk
> framework. They chose slightly different prototypes and stated
> clk_set_rate() can fail with -ve. It's a great burden for clk driver authors
> and maintainers when they try to port their drivers to U-Boot. Let's Cc
> U-Boot clk maintainers as well, and see how we can resolve the mess here.

I mean, eventually, that's on them. If U-Boot chose to have a
somewhat-similar-but-not-really clock framework, there's nothing Linux
can solve here, even though I definitely can see the frustration for the
developpers that have to work on both.

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v4 00/16] Introduce initial TI's J784S4 and AM69 support

[Maxime Ripard]

Hi,

On Sun, Oct 01, 2023 at 10:25:29PM +0530, Apurva Nandan wrote:
> This series will introduce basic support (SD and UART) support for Texas
> Instruments J784S4 EVM.
> 
> The J784S4 SoC device tree patches are taken from kernel patch submissions
> and will be updated as they are accepted and merged to the kernel tree.
> All other patches are specific to SPL and u-boot and do not have
> dependency on other trees. Appreciate a review for acceptance to u-boot
> tree.
> 
> Here are some of the salient features of the J784S4 automotive grade 
> application processor:
> 
> The J784S4 SoC belongs to the K3 Multicore SoC architecture
> platform, providing advanced system integration in automotive,
> ADAS and industrial applications requiring AI at the network edge.
> This SoC extends the K3 Jacinto 7 family of SoCs with focus on
> raising performance and integration while providing interfaces,
> memory architecture and compute performance for multi-sensor, high
> concurrency applications.
> 
> Some highlights of this SoC are:
> * Up to 8 Cortex-A72s, four clusters of lockstep capable dual Cortex-R5F MCUs,
>   4 C7x floating point vector DSPs with Matrix Multiply Accelerator(MMA) for
>   deep learning and CNN.
> * 3D GPU: Automotive grade IMG BXS-4-64 MC1 
> * Vision Processing Accelerator (VPAC) with image signal processor and Depth
>   and Motion Processing Accelerator (DMPAC)
> * Three CSI2.0 4L RX plus two CSI2.0 4L TX, two DSI Tx, one eDP/DP and one
>   DPI interface.
> * Integrated gigabit ethernet switch, up to 8 ports (TDA4VH), two ports
>   support 10Gb USXGMII; Two 4 lane PCIe-GEN3 controllers, USB3.0 Dual-role
>   device subsystems, Up to 20 MCANs, among other peripherals.
> 
> See J784S4 Technical Reference Manual (SPRUJ52 - JUNE 2022)
> for further details: http://www.ti.com/lit/zip/spruj52
> 
> In addtion, the J784S4 EVM board is designed for TI J784S4 SoC. It 
> supports the following interfaces:
> * 32 GB DDR4 RAM
> * x2 Gigabit Ethernet interfaces capable of working in Switch and MAC mode
> * x1 Input Audio Jack, x1 Output Audio Jack
> * x1 USB2.0 Hub with two Type A host and x1 USB 3.1 Type-C Port
> * x2 4L PCIe connector
> * x1 UHS-1 capable micro-SD card slot
> * 512 Mbit OSPI flash, 1 Gbit Octal NAND flash, 512 Mbit QSPI flash,
>   UFS flash.
> * x6 UART through UART-USB bridge
> * XDS110 for onboard JTAG debug using USB
> * Temperature sensors, user push buttons and LEDs
> * 40-pin User Expansion Connector
> * x2 ENET Expansion Connector, x1 GESI expander, x2 Display connector
> * x1 15-pin CSI header
> * x6 MCAN instances

I've tested this series on top of 2024.01-rc2, and while it worked fine
before, the (external) MMC is now broken:

U-Boot SPL 2024.01-rc2-00021-g8e3ced8fbad8 (Nov 15 2023 - 12:07:18 +0100)
SYSFW ABI: 3.1 (firmware rev 0x0009 '9.1.2--v09.01.02 (Kool Koala)')
SPL initial stack usage: 13416 bytes
Trying to boot from MMC2
Warning: Did not detect image signing certificate. Skipping authentication to 
prevent boot failure. This will fail on Security Enforcing(HS-SE) devices
Warning: Did not detect image signing certificate. Skipping authentication to 
prevent boot failure. This will fail on Security Enforcing(HS-SE) devices
Warning: Did not detect image signing certificate. Skipping authentication to 
prevent boot failure. This will fail on Security Enforcing(HS-SE) devices
Loading Environment from nowhere... OK
Warning: Did not detect image signing certificate. Skipping authentication to 
prevent boot failure. This will fail on Security Enforcing(HS-SE) devices
Warning: Did not detect image signing certificate. Skipping authentication to 
prevent boot failure. This will fail on Security Enforcing(HS-SE) devices
Starting ATF on ARM64 core...

NOTICE:  BL31: v2.9(release):v2.9.0
NOTICE:  BL31: Built : 12:07:09, Nov 15 2023
I/TC:
I/TC: OP-TEE version: 4.0.0 (gcc version 13.2.1 20230728 (Red Hat Cross 
13.2.1-1) (GCC)) #1 Wed Nov 15 11:07:14 UTC 2023 aarch64
I/TC: WARNING: This OP-TEE configuration might be insecure!
I/TC: WARNING: Please check 
https://optee.readthedocs.io/en/latest/architecture/porting_guidelines.html
I/TC: Primary CPU initializing
I/TC: SYSFW ABI: 3.1 (firmware rev 0x0009 '9.1.2--v09.01.02 (Kool Koala)')
I/TC: HUK Initialized
I/TC: Activated SA2UL device
I/TC: Enabled firewalls for SA2UL TRNG device
I/TC: SA2UL TRNG initialized
I/TC: SA2UL Drivers initialized
I/TC: Primary CPU switching to normal world boot

U-Boot SPL 2024.01-rc2-00021-g8e3ced8fbad8 (Nov 15 2023 - 12:07:30 +0100)
SYSFW ABI: 3.1 (firmware rev 0x0009 '9.1.2--v09.01.02 (Kool Koala)')
Trying to boot from MMC2
Warning: Did not detect image signing certificate. Skipping authentication to 
prevent boot failure. This will fail on Security Enforcing(HS-SE) devices
Warning: Did not detect image signing certificate. Skipping authentication to 
prevent boot failure. This will fail on Security Enforcing(HS-SE) devices


U-Boot 2024.01-rc2-00021-g8e3ced8fbad8 (Nov 15 2023 - 12:07:30 +0100)

Re: eMMC broken on SK-AM62

[Maxime Ripard]

Hi Nishanth,

On Thu, Nov 09, 2023 at 06:23:03AM -0600, Nishanth Menon wrote:
> On 11:29-20231109, Mattijs Korpershoek wrote:
> > Hi Maxime, Nishanth,
> > 
> > On mer., nov. 08, 2023 at 16:05, Maxime Ripard  wrote:
> > 
> > > Hi Nishanth,
> > >
> > > I've been trying for the last few days to get an SK-AM62 to boot from
> > > the eMMC user data area.
> > >
> > > I finally got the jumper configuration to make the CPU boot from there
> > > thanks to Mattijs, but it looks like the 2024.01-rc2 release doesn't
> > > support booting from there:
> > >
> > > U-Boot SPL 2024.01-rc2 (Nov 08 2023 - 14:11:12 +0100)
> > > SYSFW ABI: 3.1 (firmware rev 0x0009 '9.1.5--v09.01.05 (Kool Koala)')
> > > SPL initial stack usage: 13368 bytes
> > > Trying to boot from MMC1
> > > MMC Device 0 not found
> > > spl: could not find mmc device 0. error: -19
> > > SPL: failed to boot from all boot devices
> > > ### ERROR ### Please RESET the board ###
> > >
> > > It looks like you tried to fix it with 0f1c1e8b368b ("arm: mach-k3:
> > > am625: Add support for UDA FS"), but it looks like it's not enough.
> > 
> > I confirm I reproduce the issue on AM62x SK EVM.
> > 
> > Beagle play seems to boot fine:
> > 
> > U-Boot SPL 2024.01-rc2 (Nov 09 2023 - 11:24:04 +0100)
> > SYSFW ABI: 3.1 (firmware rev 0x0009 '9.1.1--v09.01.01 (Kool Koala)')
> > SPL initial stack usage: 13400 bytes
> > Trying to boot from MMC1
> > Warning: Detected image signing certificate on GP device. Skipping 
> > certificate to prevent boot failure. This will fail if the image was also 
> > encrypted
> > Warning: Detected image signing certificate on GP device. Skipping 
> > certificate to prevent boot failure. This will fail if the image was also 
> > encrypted
> > Warning: Detected image signing certificate on GP device. Skipping 
> > certificate to prevent boot failure. This will fail if the image was also 
> > encrypted
> > Warning: Detected image signing certificate on GP device. Skipping 
> > certificate to prevent boot failure. This will fail if the image was also 
> > encrypted
> > Warning: Detected image signing certificate on GP device. Skipping 
> > certificate to prevent boot failure. This will fail if the image was also 
> > encrypted
> > Starting ATF on ARM64 core...
> > 
> > NOTICE:  BL31: v2.8(release):08.06.00.007
> > NOTICE:  BL31: Built : 11:17:33, Nov  9 2023
> > 
> > U-Boot SPL 2024.01-rc2 (Nov 09 2023 - 11:24:30 +0100)
> > SYSFW ABI: 3.1 (firmware rev 0x0009 '9.1.1--v09.01.01 (Kool Koala)')
> > SPL initial stack usage: 1888 bytes
> > Trying to boot from MMC1
> > Warning: Detected image signing certificate on GP device. Skipping 
> > certificate to prevent boot failure. This will fail if the image was also 
> > encrypted
> > Warning: Detected image signing certificate on GP device. Skipping 
> > certificate to prevent boot failure. This will fail if the image was also 
> > encrypted
> > 
> > 
> > U-Boot 2024.01-rc2 (Nov 09 2023 - 11:24:30 +0100)
> > 
> > SoC:   AM62X SR1.0 GP
> > Model: BeagleBoard.org BeaglePlay
> > DRAM:  2 GiB
> > Core:  98 devices, 27 uclasses, devicetree: separate
> > MMC:   mmc@fa1: 0, mmc@fa0: 1, mmc@fa2: 2
> > Loading Environment from nowhere... OK
> > In:serial@280
> > Out:   serial@280
> > Err:   serial@280
> > Net:   No ethernet found.
> > 
> > Press SPACE to abort autoboot in 2 seconds
> > =>
> > 
> > Note that Beagle Play does not boot tiboot3 from UDA (User Data Area)
> > but from mmc0boot0 so the boot flow is not exactly the same.
> > 
> > >
> > > Thanks!
> > > Maxime
> 
> There is nothing complex here - sorry for the late response, Sync DT
> with v6.7-rc1 should take care of this:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/arm64/boot/dts/ti?id=c412c2f26eed08b1836ccf79f5547b67c1b55d5d
> Vs:
> https://github.com/u-boot/u-boot/blob/master/arch/arm/dts/k3-am625-sk-u-boot.dtsi#L93
> (notice no sdhci0 with bootph properties)

I backported that linux patch onto 2023.10 and 2024.01-rc2, both work
like a charm.

Thanks!
Maxime


signature.asc
Description: PGP signature

eMMC broken on SK-AM62

[Maxime Ripard]

Hi Nishanth,

I've been trying for the last few days to get an SK-AM62 to boot from
the eMMC user data area.

I finally got the jumper configuration to make the CPU boot from there
thanks to Mattijs, but it looks like the 2024.01-rc2 release doesn't
support booting from there:

U-Boot SPL 2024.01-rc2 (Nov 08 2023 - 14:11:12 +0100)
SYSFW ABI: 3.1 (firmware rev 0x0009 '9.1.5--v09.01.05 (Kool Koala)')
SPL initial stack usage: 13368 bytes
Trying to boot from MMC1
MMC Device 0 not found
spl: could not find mmc device 0. error: -19
SPL: failed to boot from all boot devices
### ERROR ### Please RESET the board ###

It looks like you tried to fix it with 0f1c1e8b368b ("arm: mach-k3:
am625: Add support for UDA FS"), but it looks like it's not enough.

Thanks!
Maxime


signature.asc
Description: PGP signature

Re: [PATCH V3 3/3] arm: dts: k3-am62: Bump dtsi from linux v6.5-rc1

[Maxime Ripard]

On Thu, 27 Jul 2023 04:03:31 -0500, Nishanth Menon wrote:
> Update the am62 and am625 device-trees from linux v6.5-rc1. This needed
> the following tweaks to the u-boot specific dtsi as well:
> - Switch tick-timer to the main_timer as it's now defined in the main dtsi
> - Secure proxies are defined in SoC dtsi
> - Drop duplicate nodes - u-boot.dtsi is includes in r5-sk, no need for
> 
> [ ... ]

Reviewed-by: Maxime Ripard 

Thanks!
Maxime

Re: [PATCH V3 0/3] arm: dts: Sync k3-am62 with upstream kernel

[Maxime Ripard]

On Thu, 27 Jul 2023 04:03:28 -0500, Nishanth Menon wrote:
> Hopefully, third time is a charm.. ;)
> 
> I have picked up part of Sjoerd's series[1] to keep it constrained just
> to dts sync at this point rather than adding newer functionality on top.
> 
> 
> [ ... ]

Tested-by: Maxime Ripard 

Thanks!
Maxime

Re: [PATCH 0/4] net: ti: am65-cpsw-nuss: Fix DT binding handling of pinctrl

[Maxime Ripard]

Hi Ravi,

On Wed, Jul 26, 2023 at 02:44:00PM +0530, Ravi Gunasekaran wrote:
> On 7/20/23 7:42 PM, Maxime Ripard wrote:
> > Hi
> > 
> > Sorry, I didn't receive Roger mail so I'll reply here
> > 
> > On Thu, Jul 20, 2023 at 09:00:19AM -0500, Nishanth Menon wrote:
> >> On 16:56-20230720, Roger Quadros wrote:
> >>> Hi,
> >>>
> >>> On 20/07/2023 16:33, Ravi Gunasekaran wrote:
> >>>>
> >>>>
> >>>> On 7/20/2023 3:25 PM, Maxime Ripard wrote:
> >>>>> Hi,
> >>>>>
> >>>>> This series is based on:
> >>>>> https://lore.kernel.org/all/20230713072019.3153871-1...@ti.com/
> >>>>>
> >>>>> It fixes the issue of Linux booting from the DT embedded by U-boot. The
> >>>>> main issue there is that U-Boot doesn't handle the MDIO child node that
> >>>>> might have resources attached to it.
> >>>>>
> >>>>> Thus, any pinctrl configuration that could be attached to the MDIO
> >>>>> child node is effectively ignored. Unfortunately, starting with 6.5-rc1,
> >>>>> Linux does just that.
> >>>
> >>> I didn't get this part. Linux does not ignore pinctrl configuration 
> >>> attached
> >>> to MDIO child node. What changed in 6.5-rc1?
> > 
> > Linux doesn't ignore it, but Linux started putting pinctrl configuration
> > on the MDIO node, which is ignored by U-Boot.
> > 
> >>>>> This was solved by duplicating the pinctrl configuration onto the MAC
> >>>
> >>> If I remember right, there is no driver model driver for MDIO in u-boot 
> >>> and
> >>> adding the mdio pinctrl into CPSW DT node was a hack in u-boot.
> > 
> > Yeah, but we now have in the U-Boot DT two nodes referencing the same
> > pinctrl configuration: the CPSW and the MDIO node. Linux then tries to
> > assign that configuration to both devices and it fails.
> 
> This response might be late, as I know things have moved ahead after this
> discussion. But I just wanted to understand the root cause little bit more.
> Is the issue mainly because the same mdio pinctrl node(phandle) is referenced 
> in
> two other nodes? Or is it because of same pins being updated in two different
> places in the kernel?
> 
> If it is the former, then would a duplicate mdio node help keep the changes
> to minimum?

Neither, actually :/ The issue is that, with the changes made by
Nishanth to bring the 6.5-rc1 DTS in, the same pinctrl node is
referenced in two separate nodes, and Linux sees that as conflicting
users of the same pins.

One quick workaround would be to remove the MDIO pinctrl property, and
add it to the MAC pinctrl property.

That's fairly dangerous if either get extended though, so it might not
be a proper solution long term.

I hope it's clearer

Maxime


signature.asc
Description: PGP signature

Re: [PATCH 3/3] arm: dts: k3-am62: Bump dtsi from linux v6.5-rc1

[Maxime Ripard]

On Wed, Jul 26, 2023 at 11:26:05AM +0530, Ravi Gunasekaran wrote:
> On 7/25/23 11:47 PM, Tom Rini wrote:
> > On Tue, Jul 25, 2023 at 09:09:34AM -0500, Nishanth Menon wrote:
> >> On 15:56-20230725, Maxime Ripard wrote:
> >>> Hi,
> >>>
> >>> On Tue, Jul 25, 2023 at 07:58:56AM -0500, Nishanth Menon wrote:
> >>>> Update the am62 and am625 device-trees from linux v6.3-rc5 This needed 
> >>>> the followin
> >>>> tweaks to the u-boot specific dtsi as well:
> >>>> - Switch tick-timer to the main_timer as it's now defined in the main 
> >>>> dtsi
> >>>> - Secure proxies are defined in Soc dtsis
> >>>> - Drop duplicate nodes - u-boot.dtsi is includes in r5-sk, no need for
> >>>>   either the definitions from main.dtsi OR duplication from u-boot.dtsi
> >>>> - Add mdio pins to the cpsw3g pinctrl in u-boot dtsi. It moved to a 
> >>>> subnode in the
> >>>>   linux dtsi that u-boot doesn't use/support
> >>>>
> >>>> Cc: Francesco Dolcini 
> >>>> Cc: Sjoerd Simons 
> >>>> Cc: Wadim Egorov 
> >>>> Signed-off-by: Nishanth Menon 
> >>>> ---
> >>>>
> >>>> I decided not to pick up changes from Roger and Maxime as they are'nt
> >>>> regression fixes, instead the fixups can be done on top of the basic
> >>>> sync.
> >>>
> >>> This breaks Linux network interfaces so I very much considers this a 
> >>> regression.
> >> The current u-boot.dtsi I am confused - pinctrl mdio is still a
> >> problem, is'nt it?
> >> https://github.com/u-boot/u-boot/blob/master/arch/arm/dts/k3-am625-sk-u-boot.dtsi#L132
> >> The follow on fixups from both your and Roger's series should fix things
> >> up, correct?
> > 
> > I'm a little confused here too.  For each of these cases, what happens?
> > - Today, one of these platforms, we pass the kernel the in-memory U-Boot
> >   device tree.  And if this is functional for networking, how/why?
> 
> In here, the mdio pinctrl configuration is present only in one node (CPSW).
> So there is no conflict when the in-memory DT is passed by U-Boot to kernel.
> 
> > - "Tomorrow", one of these platforms, with Nishanth's series applied, we
> >   pass the in-memory U-Boot device tree
> 
> In this case, the DT passed by U-Boot has the mdio pinctrl info present in
> two nodes - one in CPSW and other in MDIO. This causes failure in Linux as
> both nodes refer to same pinctrl configuration.

Yeah, exactly. The "Tomorrow" case probably makes it functional for
U-Boot but breaks Linux in the process. It was functional in Linux
before because it has everything needed to set up the network and didn't
rely on U-Boot for that at all.

Maxime


signature.asc
Description: PGP signature

Re: [PATCH 3/3] arm: dts: k3-am62: Bump dtsi from linux v6.5-rc1

[Maxime Ripard]

Hi,

On Tue, Jul 25, 2023 at 07:58:56AM -0500, Nishanth Menon wrote:
> Update the am62 and am625 device-trees from linux v6.3-rc5 This needed the 
> followin
> tweaks to the u-boot specific dtsi as well:
> - Switch tick-timer to the main_timer as it's now defined in the main dtsi
> - Secure proxies are defined in Soc dtsis
> - Drop duplicate nodes - u-boot.dtsi is includes in r5-sk, no need for
>   either the definitions from main.dtsi OR duplication from u-boot.dtsi
> - Add mdio pins to the cpsw3g pinctrl in u-boot dtsi. It moved to a subnode 
> in the
>   linux dtsi that u-boot doesn't use/support
> 
> Cc: Francesco Dolcini 
> Cc: Sjoerd Simons 
> Cc: Wadim Egorov 
> Signed-off-by: Nishanth Menon 
> ---
> 
> I decided not to pick up changes from Roger and Maxime as they are'nt
> regression fixes, instead the fixups can be done on top of the basic
> sync.

This breaks Linux network interfaces so I very much considers this a regression.

Maxime


signature.asc
Description: PGP signature

[PATCH v4 2/2] fixup! arm: dts: k3-am62: Bump dtsi from linux v6.5-rc1

[Maxime Ripard]

The MDIO pinctrl nodes can't be duplicated between the child and device,
because if we ever boot Linux with our DT it will try to attach that
pinctrl configuration to both the MAC and MDIO devices, which will
result in failure to probe.

Signed-off-by: Maxime Ripard 
---
 arch/arm/dts/k3-am625-sk-u-boot.dtsi | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/dts/k3-am625-sk-u-boot.dtsi 
b/arch/arm/dts/k3-am625-sk-u-boot.dtsi
index a3f207baa304..c1685bc9ca39 100644
--- a/arch/arm/dts/k3-am625-sk-u-boot.dtsi
+++ b/arch/arm/dts/k3-am625-sk-u-boot.dtsi
@@ -120,7 +120,6 @@
 
  {
bootph-pre-ram;
-   pinctrl-0 = <_mdio1_pins_default _rgmii1_pins_default>;
 };
 
 _port1 {

-- 
2.41.0

[PATCH v4 0/2] net: ti: am65-cpsw-nuss: Fix DT binding handling of pinctrl

[Maxime Ripard]

Hi,

This series is based on:
https://lore.kernel.org/all/20230713072019.3153871-1...@ti.com/
https://lore.kernel.org/u-boot/20230614222853.574427-1-dannenb...@ti.com/
https://lore.kernel.org/u-boot/20230722193151.117345-1-rog...@kernel.org/

It fixes the issue of Linux booting from the DT embedded by U-boot. The
main issue there is that U-Boot doesn't handle the MDIO child node that
might have resources attached to it.

Thus, any pinctrl configuration that could be attached to the MDIO
child node is effectively ignored. Unfortunately, starting with 6.5-rc1,
Linux does just that.

This was solved by duplicating the pinctrl configuration onto the MAC
device node. Unfortunately, this doesn't work for Linux since now it has
two devices competing for the same pins.

Let me know what you think,
Maxime

Signed-off-by: Maxime Ripard 
---
Changes in v4:
- Squashed fixup patches
- Link to v3: 
https://lore.kernel.org/r/20230724-ti-mdio-pinmux-v3-0-fc8359b00...@kernel.org

Changes in v3:
- Rebase on top of latest Roger series
- Link to v2: 
https://lore.kernel.org/r/20230721-ti-mdio-pinmux-v2-0-4bb443e09...@kernel.org

Changes in v2:
- Drop the pinctrl API change in favour of a dummy driver
- Link to v1: 
https://lore.kernel.org/r/20230720-ti-mdio-pinmux-v1-0-0bd3bd1cf...@kernel.org

---
Maxime Ripard (2):
  net: ti: am65-cpsw-nuss: Enforce pinctrl state on the MDIO child node
  fixup! arm: dts: k3-am62: Bump dtsi from linux v6.5-rc1

 arch/arm/dts/k3-am625-sk-u-boot.dtsi |  1 -
 drivers/net/ti/Kconfig   |  1 +
 drivers/net/ti/am65-cpsw-nuss.c  | 60 
 3 files changed, 61 insertions(+), 1 deletion(-)
---
base-commit: db7126c87c44c45c448c68925d366510f7a09b11
change-id: 20230720-ti-mdio-pinmux-a12525dba973

Best regards,
-- 
Maxime Ripard 

[PATCH v4 1/2] net: ti: am65-cpsw-nuss: Enforce pinctrl state on the MDIO child node

[Maxime Ripard]

The binding represents the MDIO controller as a child device tree
node of the MAC device tree node.

The U-Boot driver mostly ignores that child device tree node and just
hardcodes the resources it uses to support both the MAC and MDIO in a
single driver.

However, some resources like pinctrl muxing states are thus ignored.
This has been a problem with some device trees that will put some
pinctrl states on the MDIO device tree node, like the SK-AM62 Device
Tree does.

Let's rework the driver a bit to create a dummy MDIO driver that we will
then get during our initialization to force the core to select the right
muxing.

Signed-off-by: Maxime Ripard 
---
 drivers/net/ti/Kconfig  |  1 +
 drivers/net/ti/am65-cpsw-nuss.c | 60 +
 2 files changed, 61 insertions(+)

diff --git a/drivers/net/ti/Kconfig b/drivers/net/ti/Kconfig
index d9f1c019a885..02660e4fbb44 100644
--- a/drivers/net/ti/Kconfig
+++ b/drivers/net/ti/Kconfig
@@ -41,6 +41,7 @@ endchoice
 config TI_AM65_CPSW_NUSS
bool "TI K3 AM65x MCU CPSW Nuss Ethernet controller driver"
depends on ARCH_K3
+   imply DM_MDIO
imply MISC_INIT_R
imply MISC
imply SYSCON
diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c
index ce52106e5238..51a8167d14a9 100644
--- a/drivers/net/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ti/am65-cpsw-nuss.c
@@ -15,6 +15,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -605,14 +606,62 @@ static const struct soc_attr k3_mdio_soc_data[] = {
{ /* sentinel */ },
 };
 
+static ofnode am65_cpsw_find_mdio(ofnode parent)
+{
+   ofnode node;
+
+   ofnode_for_each_subnode(node, parent)
+   if (ofnode_device_is_compatible(node, "ti,cpsw-mdio"))
+   return node;
+
+   return ofnode_null();
+}
+
+static int am65_cpsw_mdio_setup(struct udevice *dev)
+{
+   struct am65_cpsw_priv *priv = dev_get_priv(dev);
+   struct am65_cpsw_common *cpsw_common = priv->cpsw_common;
+   struct udevice *mdio_dev;
+   ofnode mdio;
+   int ret;
+
+   mdio = am65_cpsw_find_mdio(dev_ofnode(cpsw_common->dev));
+   if (!ofnode_valid(mdio))
+   return 0;
+
+   /*
+* The MDIO controller is represented in the DT binding by a
+* subnode of the MAC controller.
+*
+* We don't have a DM driver for the MDIO device yet, and thus any
+* pinctrl setting on its node will be ignored.
+*
+* However, we do need to make sure the pins states tied to the
+* MDIO node are configured properly. Fortunately, the core DM
+* does that for use when we get a device, so we can work around
+* that whole issue by just requesting a dummy MDIO driver to
+* probe, and our pins will get muxed.
+*/
+   ret = uclass_get_device_by_ofnode(UCLASS_MDIO, mdio, _dev);
+   if (ret)
+   return ret;
+
+   return 0;
+}
+
 static int am65_cpsw_mdio_init(struct udevice *dev)
 {
struct am65_cpsw_priv *priv = dev_get_priv(dev);
struct am65_cpsw_common *cpsw_common = priv->cpsw_common;
+   int ret;
 
if (!priv->has_phy || cpsw_common->bus)
return 0;
 
+   ret = am65_cpsw_mdio_setup(dev);
+   if (ret)
+   return ret;
+
cpsw_common->bus = cpsw_mdio_init(dev->name,
  cpsw_common->mdio_base,
  cpsw_common->bus_freq,
@@ -868,3 +917,14 @@ U_BOOT_DRIVER(am65_cpsw_nuss_port) = {
.plat_auto  = sizeof(struct eth_pdata),
.flags = DM_FLAG_ALLOC_PRIV_DMA | DM_FLAG_OS_PREPARE,
 };
+
+static const struct udevice_id am65_cpsw_mdio_ids[] = {
+   { .compatible = "ti,cpsw-mdio" },
+   { }
+};
+
+U_BOOT_DRIVER(am65_cpsw_mdio) = {
+   .name   = "am65_cpsw_mdio",
+   .id = UCLASS_MDIO,
+   .of_match   = am65_cpsw_mdio_ids,
+};

-- 
2.41.0

[PATCH v3 1/3] net: ti: am65-cpsw-nuss: Enforce pinctrl state on the MDIO child node

[Maxime Ripard]

The binding represents the MDIO controller as a child device tree
node of the MAC device tree node.

The U-Boot driver mostly ignores that child device tree node and just
hardcodes the resources it uses to support both the MAC and MDIO in a
single driver.

However, some resources like pinctrl muxing states are thus ignored.
This has been a problem with some device trees that will put some
pinctrl states on the MDIO device tree node, like the SK-AM62 Device
Tree does.

Let's rework the driver a bit to create a dummy MDIO driver that we will
then get during our initialization to force the core to select the right
muxing.

Signed-off-by: Maxime Ripard 
---
 drivers/net/ti/Kconfig  |  1 +
 drivers/net/ti/am65-cpsw-nuss.c | 67 +
 2 files changed, 68 insertions(+)

diff --git a/drivers/net/ti/Kconfig b/drivers/net/ti/Kconfig
index d9f1c019a885..02660e4fbb44 100644
--- a/drivers/net/ti/Kconfig
+++ b/drivers/net/ti/Kconfig
@@ -41,6 +41,7 @@ endchoice
 config TI_AM65_CPSW_NUSS
bool "TI K3 AM65x MCU CPSW Nuss Ethernet controller driver"
depends on ARCH_K3
+   imply DM_MDIO
imply MISC_INIT_R
imply MISC
imply SYSCON
diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c
index ce52106e5238..29a4284a9b70 100644
--- a/drivers/net/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ti/am65-cpsw-nuss.c
@@ -15,6 +15,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -605,14 +606,69 @@ static const struct soc_attr k3_mdio_soc_data[] = {
{ /* sentinel */ },
 };
 
+static ofnode am65_cpsw_find_mdio(ofnode parent)
+{
+   ofnode node;
+
+   ofnode_for_each_subnode(node, parent)
+   if (ofnode_device_is_compatible(node, "ti,cpsw-mdio"))
+   return node;
+
+   return ofnode_null();
+}
+
+static int am65_cpsw_mdio_setup(struct udevice *dev)
+{
+   struct am65_cpsw_priv *priv = dev_get_priv(dev);
+   struct am65_cpsw_common *cpsw_common = priv->cpsw_common;
+   struct udevice *mdio_dev;
+   ofnode mdio;
+   int ret;
+
+   mdio = am65_cpsw_find_mdio(dev_ofnode(cpsw_common->dev));
+   if (!ofnode_valid(mdio))
+   return -ENODEV;
+
+   /*
+* The MDIO controller is represented in the DT binding by a
+* subnode of the MAC controller.
+*
+* Our driver largely ignores that and supports MDIO by
+* hardcoding the register offsets.
+*
+* However, some resources (clocks, pinctrl) might be tied to
+* the MDIO node, and thus ignored.
+*
+* Clocks are not a concern at the moment since it's shared
+* between the MAC and MDIO, so the driver handles both at the
+* same time.
+*
+* However, we do need to make sure the pins states tied to the
+* MDIO node are configured properly. Fortunately, the core DM
+* does that for use when we get a device, so we can work around
+* that whole issue by just requesting a dummy MDIO driver to
+* probe, and our pins will get muxed.
+*/
+   ret = uclass_get_device_by_ofnode(UCLASS_MDIO, mdio, _dev);
+   if (ret)
+   return ret;
+
+   return 0;
+}
+
 static int am65_cpsw_mdio_init(struct udevice *dev)
 {
struct am65_cpsw_priv *priv = dev_get_priv(dev);
struct am65_cpsw_common *cpsw_common = priv->cpsw_common;
+   int ret;
 
if (!priv->has_phy || cpsw_common->bus)
return 0;
 
+   ret = am65_cpsw_mdio_setup(dev);
+   if (ret)
+   return ret;
+
cpsw_common->bus = cpsw_mdio_init(dev->name,
  cpsw_common->mdio_base,
  cpsw_common->bus_freq,
@@ -868,3 +924,14 @@ U_BOOT_DRIVER(am65_cpsw_nuss_port) = {
.plat_auto  = sizeof(struct eth_pdata),
.flags = DM_FLAG_ALLOC_PRIV_DMA | DM_FLAG_OS_PREPARE,
 };
+
+static const struct udevice_id am65_cpsw_mdio_ids[] = {
+   { .compatible = "ti,cpsw-mdio" },
+   { }
+};
+
+U_BOOT_DRIVER(am65_cpsw_mdio) = {
+   .name   = "am65_cpsw_mdio",
+   .id = UCLASS_MDIO,
+   .of_match   = am65_cpsw_mdio_ids,
+};

-- 
2.41.0

[PATCH v3 2/3] fixup! arm: dts: k3-am62: Bump dtsi from linux v6.5-rc1

[Maxime Ripard]

The MDIO pinctrl nodes can't be duplicated between the child and device,
because if we ever boot Linux with our DT it will try to attach that
pinctrl configuration to both the MAC and MDIO devices, which will
result in failure to probe.

Signed-off-by: Maxime Ripard 
---
 arch/arm/dts/k3-am625-sk-u-boot.dtsi | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/dts/k3-am625-sk-u-boot.dtsi 
b/arch/arm/dts/k3-am625-sk-u-boot.dtsi
index a3f207baa304..c1685bc9ca39 100644
--- a/arch/arm/dts/k3-am625-sk-u-boot.dtsi
+++ b/arch/arm/dts/k3-am625-sk-u-boot.dtsi
@@ -120,7 +120,6 @@
 
  {
bootph-pre-ram;
-   pinctrl-0 = <_mdio1_pins_default _rgmii1_pins_default>;
 };
 
 _port1 {

-- 
2.41.0

[PATCH v3 3/3] fixup! net: ti: am65-cpsw-nuss: Enforce pinctrl state on the MDIO child node

[Maxime Ripard]

---
 drivers/net/ti/am65-cpsw-nuss.c | 13 +++--
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c
index 29a4284a9b70..51a8167d14a9 100644
--- a/drivers/net/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ti/am65-cpsw-nuss.c
@@ -627,21 +627,14 @@ static int am65_cpsw_mdio_setup(struct udevice *dev)
 
mdio = am65_cpsw_find_mdio(dev_ofnode(cpsw_common->dev));
if (!ofnode_valid(mdio))
-   return -ENODEV;
+   return 0;
 
/*
 * The MDIO controller is represented in the DT binding by a
 * subnode of the MAC controller.
 *
-* Our driver largely ignores that and supports MDIO by
-* hardcoding the register offsets.
-*
-* However, some resources (clocks, pinctrl) might be tied to
-* the MDIO node, and thus ignored.
-*
-* Clocks are not a concern at the moment since it's shared
-* between the MAC and MDIO, so the driver handles both at the
-* same time.
+* We don't have a DM driver for the MDIO device yet, and thus any
+* pinctrl setting on its node will be ignored.
 *
 * However, we do need to make sure the pins states tied to the
 * MDIO node are configured properly. Fortunately, the core DM

-- 
2.41.0

[PATCH v3 0/3] net: ti: am65-cpsw-nuss: Fix DT binding handling of pinctrl

[Maxime Ripard]

Hi,

This series is based on:
https://lore.kernel.org/all/20230713072019.3153871-1...@ti.com/
https://lore.kernel.org/u-boot/20230614222853.574427-1-dannenb...@ti.com/
https://lore.kernel.org/u-boot/20230722193151.117345-1-rog...@kernel.org/

It fixes the issue of Linux booting from the DT embedded by U-boot. The
main issue there is that U-Boot doesn't handle the MDIO child node that
might have resources attached to it.

Thus, any pinctrl configuration that could be attached to the MDIO
child node is effectively ignored. Unfortunately, starting with 6.5-rc1,
Linux does just that.

This was solved by duplicating the pinctrl configuration onto the MAC
device node. Unfortunately, this doesn't work for Linux since now it has
two devices competing for the same pins.

Let me know what you think,
Maxime

Signed-off-by: Maxime Ripard 
---
Changes in v3:
- Rebase on top of latest Roger series
- Link to v2: 
https://lore.kernel.org/r/20230721-ti-mdio-pinmux-v2-0-4bb443e09...@kernel.org

Changes in v2:
- Drop the pinctrl API change in favour of a dummy driver
- Link to v1: 
https://lore.kernel.org/r/20230720-ti-mdio-pinmux-v1-0-0bd3bd1cf...@kernel.org

---
Maxime Ripard (3):
  net: ti: am65-cpsw-nuss: Enforce pinctrl state on the MDIO child node
  fixup! arm: dts: k3-am62: Bump dtsi from linux v6.5-rc1
  fixup! net: ti: am65-cpsw-nuss: Enforce pinctrl state on the MDIO child 
node

 arch/arm/dts/k3-am625-sk-u-boot.dtsi |  1 -
 drivers/net/ti/Kconfig   |  1 +
 drivers/net/ti/am65-cpsw-nuss.c  | 60 
 3 files changed, 61 insertions(+), 1 deletion(-)
---
base-commit: db7126c87c44c45c448c68925d366510f7a09b11
change-id: 20230720-ti-mdio-pinmux-a12525dba973

Best regards,
-- 
Maxime Ripard 

Re: [PATCH v3 3/3] fixup! net: ti: am65-cpsw-nuss: Enforce pinctrl state on the MDIO child node

[Maxime Ripard]

Hi,

On Mon, Jul 24, 2023 at 03:52:54PM +0200, Maxime Ripard wrote:
> ---
>  drivers/net/ti/am65-cpsw-nuss.c | 13 +++--
>  1 file changed, 3 insertions(+), 10 deletions(-)

Sorry, it should have been squashed. I'll send a v4 shortly.

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v2 1/3] net: ti: am65-cpsw-nuss: Enforce pinctrl state on the MDIO child node

[Maxime Ripard]

On Sat, Jul 22, 2023 at 11:25:50AM +0300, Roger Quadros wrote:
> On 21/07/2023 16:07, Maxime Ripard wrote:
> > The binding represents the MDIO controller as a child device tree
> > node of the MAC device tree node.
> > 
> > The U-Boot driver mostly ignores that child device tree node and just
> > hardcodes the resources it uses to support both the MAC and MDIO in a
> > single driver.
> > 
> > However, some resources like pinctrl muxing states are thus ignored.
> > This has been a problem with some device trees that will put some
> > pinctrl states on the MDIO device tree node, like the SK-AM62 Device
> > Tree does.
> 
> You don't clearly explain the problem.

I'm sorry to hear that.

> I think you need to mention that there wash a hackish solution in
> place that was duplicating MDIO pinctrl node in the CPSW node. And
> this causes problems in Linux (if booted with u-boot's device tree)
> due to 2 drivers (CPSW and MDIO) racing for the same MDIO pinctrl
> resource.

I mean, ultimately, this hack was due to nothing but a workaround around
the fact that U-Boot was ignoring the MDIO child node resources. This is
the root cause. And once fixed, that hack can go away.

> Then mention how you are fixing it.

I'm fixing it by "rework[ing] the driver a bit to create a dummy MDIO
driver that we will then get during our initialization to force the core
to select the right muxing."

If that's still not a clear explanation, please provide a better one so
that we can move forward.

> By deleting the duplicate MDIO pinctrl entry from CPSW node.

Not really, no. If I was only deleting the duplitate pinctrl entry,
U-Boot would still be broken.

> > Let's rework the driver a bit to create a dummy MDIO driver that we will
> > then get during our initialization to force the core to select the right
> > muxing.
> > 
> > Signed-off-by: Maxime Ripard 
> > ---
> >  drivers/net/ti/Kconfig  |  1 +
> >  drivers/net/ti/am65-cpsw-nuss.c | 67 
> > +
> >  2 files changed, 68 insertions(+)
> > 
> > diff --git a/drivers/net/ti/Kconfig b/drivers/net/ti/Kconfig
> > index e13dbc940182..08c81f79adf9 100644
> > --- a/drivers/net/ti/Kconfig
> > +++ b/drivers/net/ti/Kconfig
> > @@ -41,6 +41,7 @@ endchoice
> >  config TI_AM65_CPSW_NUSS
> > bool "TI K3 AM65x MCU CPSW Nuss Ethernet controller driver"
> > depends on ARCH_K3
> > +   imply DM_MDIO
> > imply MISC_INIT_R
> > imply MISC
> > select PHYLIB
> > diff --git a/drivers/net/ti/am65-cpsw-nuss.c 
> > b/drivers/net/ti/am65-cpsw-nuss.c
> > index 3069550d53c2..ac7907e7ef70 100644
> > --- a/drivers/net/ti/am65-cpsw-nuss.c
> > +++ b/drivers/net/ti/am65-cpsw-nuss.c
> > @@ -15,6 +15,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -580,14 +581,69 @@ static const struct soc_attr k3_mdio_soc_data[] = {
> > { /* sentinel */ },
> >  };
> >  
> > +static ofnode am65_cpsw_find_mdio(ofnode parent)
> > +{
> > +   ofnode node;
> > +
> > +   ofnode_for_each_subnode(node, parent)
> > +   if (ofnode_device_is_compatible(node, "ti,cpsw-mdio"))
> > +   return node;
> > +
> > +   return ofnode_null();
> > +}
> > +
> > +static int am65_cpsw_mdio_setup(struct udevice *dev)
> > +{
> > +   struct am65_cpsw_priv *priv = dev_get_priv(dev);
> > +   struct am65_cpsw_common *cpsw_common = priv->cpsw_common;
> > +   struct udevice *mdio_dev;
> > +   ofnode mdio;
> > +   int ret;
> > +
> > +   mdio = am65_cpsw_find_mdio(dev_ofnode(cpsw_common->dev));
> > +   if (!ofnode_valid(mdio))
> > +   return -ENODEV;
> 
> Do we really want to treat this as an error?
> 
> As per Linux/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml
> mdio node is not a required property/child.

Ack, I'll change it.

> > +
> > +   /*
> > +* The MDIO controller is represented in the DT binding by a
> > +* subnode of the MAC controller.
> > +*
> > +* Our driver largely ignores that and supports MDIO by
> > +* hardcoding the register offsets.
> > +*
> > +* However, some resources (clocks, pinctrl) might be tied to
> > +* the MDIO node, and thus ignored.
> > +*
> > +* Clocks are not a concern at the moment since it's shared
> > +* between the MAC and MDIO, so the driver handles both at the
> > +* same time.
> 
> I think you can drop the above 3 paras and instead say
> "We don't yet have a DM device driver for the MDIO device and so its
> pinctrl setting will be ignored."

Ok.

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v2 0/4] net: ti: am65-cpsw-nuss: Drop custom properties

[Maxime Ripard]

Hi,

On Sat, Jul 22, 2023 at 10:31:47PM +0300, Roger Quadros wrote:
> This series gets rid of 2 custom properties (mac_efuse and cpsw-phy-sel)
> that were used in u-boot for the cpsw3g node.
> 
> This makes it easier for us to have u-boot DT in sync with Linux.
> 
> Last 2 patches are RFC so they can come separately. i.e. squashed with
> DT cleanup series from Nishanth.

Thanks for cleaning this up, hopefully it will solve our ranges
conversation :)

Tested-by: Maxime Ripard 

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v2 3/3] fixup! arm: dts: k3-am62: Bump dtsi from linux v6.5-rc1

[Maxime Ripard]

Hi,

Thanks for your review

On Sat, Jul 22, 2023 at 11:32:37AM +0300, Roger Quadros wrote:
> On 21/07/2023 16:07, Maxime Ripard wrote:
> > Dropping ranges entirely doesn't work since the register offset on the
> > MDIO device node will now be completely off, so we need to adjust it to
> > the right value without the translation.
> > 
> > We also need to have an empty ranges property for the reg address to be
> > properly evaluated.
> > 
> > Signed-off-by: Maxime Ripard 
> > ---
> >  arch/arm/dts/k3-am625-sk-u-boot.dtsi | 6 +-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/dts/k3-am625-sk-u-boot.dtsi 
> > b/arch/arm/dts/k3-am625-sk-u-boot.dtsi
> > index db814ed02a7e..77c9e4cb87f7 100644
> > --- a/arch/arm/dts/k3-am625-sk-u-boot.dtsi
> > +++ b/arch/arm/dts/k3-am625-sk-u-boot.dtsi
> > @@ -119,8 +119,8 @@
> >  };
> >  
> >   {
> > -   /delete-property/ ranges;
> 
> cpsw-phy-sel will be broken in u-boot after you remove
> /delete-property/ ranges.

I don't think it would?

If we look at k3-am62-main.dtsi, we have:

ranges = <0x00 0x00 0x00 0x0800 0x00 0x20>;

There's an address-cells and size-cells of 2, so it translates any child
node address between the range 0x00-0x20 to
0x0800-0x820.

cpsw-mdio is thus translated to 0x08000f00.

Nishanth patch was dropping that ranges property, which means that
(aside from breaking the address translation if we follow the spec),
cpsw-mdio now has the address of its reg property: 0xf00.

...

> To fix this up we need to teach the am65-cpsw driver to fetch
> the cpsw-phy-sel address from the phys property instead and drop
> the cpsw-phy-sel child.
> 
> > bootph-pre-ram;
> > +   ranges;
> 
> You don't have to add ranges here. am62-main.dtsi should have it in
> the cpsw3g node.

...

So I'm adding a new 1:1 address translation for spec-compliant code to
still work.

...

> >  
> > cpsw-phy-sel@04044 {
> > compatible = "ti,am64-phy-gmii-sel";
> > @@ -129,6 +129,10 @@
> > };
> >  };
> >  
> > +_mdio {
> > +   reg = <0x0 0x8000f00 0x0 0x100>;
> > +};
> > +

... And I'm setting the MDIO reg property to what the address was prior
to the ranges property removal. The driver, if it follows ranges
properly, will get exactly the same address.

Now, onto your other comments:

> > --- a/arch/arm/dts/k3-am625-sk-u-boot.dtsi
> > +++ b/arch/arm/dts/k3-am625-sk-u-boot.dtsi
> > @@ -119,8 +119,8 @@
> >  };
> >
> >   {
> > -   /delete-property/ ranges;
>
> cpsw-phy-sel will be broken in u-boot after you remove
> /delete-property/ ranges.
>
> To fix this up we need to teach the am65-cpsw driver to fetch
> the cpsw-phy-sel address from the phys property instead and drop
> the cpsw-phy-sel child.

I don't know the TI platform that well, but my understanding is that the
address used to be 0x00104044, which was probably interpreted as such by
U-Boot if we were missing ranges. I added back ranges to comply to the
spec but with a 1:1 mapping so that address won't change.

How is it broken exactly?

> > @@ -129,6 +129,10 @@
> > };
> >  };
> >
> > +_mdio {
> > +   reg = <0x0 0x8000f00 0x0 0x100>;
> > +};
> > +

> This should not be required. The u-boot driver is still hard-coding
> the MDIO address and Linux should get the right address based on
> address translation of the child cpsw3g_mdio node.

As pointed above, Linux will not get the same address anymore (and will
actually return -EINVAL when decoding it), so no, it's very much needed.

Maxime


signature.asc
Description: PGP signature

[PATCH v2 3/3] fixup! arm: dts: k3-am62: Bump dtsi from linux v6.5-rc1

[Maxime Ripard]

Dropping ranges entirely doesn't work since the register offset on the
MDIO device node will now be completely off, so we need to adjust it to
the right value without the translation.

We also need to have an empty ranges property for the reg address to be
properly evaluated.

Signed-off-by: Maxime Ripard 
---
 arch/arm/dts/k3-am625-sk-u-boot.dtsi | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm/dts/k3-am625-sk-u-boot.dtsi 
b/arch/arm/dts/k3-am625-sk-u-boot.dtsi
index db814ed02a7e..77c9e4cb87f7 100644
--- a/arch/arm/dts/k3-am625-sk-u-boot.dtsi
+++ b/arch/arm/dts/k3-am625-sk-u-boot.dtsi
@@ -119,8 +119,8 @@
 };
 
  {
-   /delete-property/ ranges;
bootph-pre-ram;
+   ranges;
 
cpsw-phy-sel@04044 {
compatible = "ti,am64-phy-gmii-sel";
@@ -129,6 +129,10 @@
};
 };
 
+_mdio {
+   reg = <0x0 0x8000f00 0x0 0x100>;
+};
+
 _port1 {
bootph-pre-ram;
 };

-- 
2.41.0

[PATCH v2 2/3] fixup! arm: dts: k3-am62: Bump dtsi from linux v6.5-rc1

[Maxime Ripard]

The MDIO pinctrl nodes can't be duplicated between the child and device,
because if we ever boot Linux with our DT it will try to attach that
pinctrl configuration to both the MAC and MDIO devices, which will
result in failure to probe.

Signed-off-by: Maxime Ripard 
---
 arch/arm/dts/k3-am625-sk-u-boot.dtsi | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/dts/k3-am625-sk-u-boot.dtsi 
b/arch/arm/dts/k3-am625-sk-u-boot.dtsi
index 379165373aca..db814ed02a7e 100644
--- a/arch/arm/dts/k3-am625-sk-u-boot.dtsi
+++ b/arch/arm/dts/k3-am625-sk-u-boot.dtsi
@@ -121,7 +121,6 @@
  {
/delete-property/ ranges;
bootph-pre-ram;
-   pinctrl-0 = <_mdio1_pins_default _rgmii1_pins_default>;
 
cpsw-phy-sel@04044 {
compatible = "ti,am64-phy-gmii-sel";

-- 
2.41.0

[PATCH v2 1/3] net: ti: am65-cpsw-nuss: Enforce pinctrl state on the MDIO child node

[Maxime Ripard]

The binding represents the MDIO controller as a child device tree
node of the MAC device tree node.

The U-Boot driver mostly ignores that child device tree node and just
hardcodes the resources it uses to support both the MAC and MDIO in a
single driver.

However, some resources like pinctrl muxing states are thus ignored.
This has been a problem with some device trees that will put some
pinctrl states on the MDIO device tree node, like the SK-AM62 Device
Tree does.

Let's rework the driver a bit to create a dummy MDIO driver that we will
then get during our initialization to force the core to select the right
muxing.

Signed-off-by: Maxime Ripard 
---
 drivers/net/ti/Kconfig  |  1 +
 drivers/net/ti/am65-cpsw-nuss.c | 67 +
 2 files changed, 68 insertions(+)

diff --git a/drivers/net/ti/Kconfig b/drivers/net/ti/Kconfig
index e13dbc940182..08c81f79adf9 100644
--- a/drivers/net/ti/Kconfig
+++ b/drivers/net/ti/Kconfig
@@ -41,6 +41,7 @@ endchoice
 config TI_AM65_CPSW_NUSS
bool "TI K3 AM65x MCU CPSW Nuss Ethernet controller driver"
depends on ARCH_K3
+   imply DM_MDIO
imply MISC_INIT_R
imply MISC
select PHYLIB
diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c
index 3069550d53c2..ac7907e7ef70 100644
--- a/drivers/net/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ti/am65-cpsw-nuss.c
@@ -15,6 +15,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -580,14 +581,69 @@ static const struct soc_attr k3_mdio_soc_data[] = {
{ /* sentinel */ },
 };
 
+static ofnode am65_cpsw_find_mdio(ofnode parent)
+{
+   ofnode node;
+
+   ofnode_for_each_subnode(node, parent)
+   if (ofnode_device_is_compatible(node, "ti,cpsw-mdio"))
+   return node;
+
+   return ofnode_null();
+}
+
+static int am65_cpsw_mdio_setup(struct udevice *dev)
+{
+   struct am65_cpsw_priv *priv = dev_get_priv(dev);
+   struct am65_cpsw_common *cpsw_common = priv->cpsw_common;
+   struct udevice *mdio_dev;
+   ofnode mdio;
+   int ret;
+
+   mdio = am65_cpsw_find_mdio(dev_ofnode(cpsw_common->dev));
+   if (!ofnode_valid(mdio))
+   return -ENODEV;
+
+   /*
+* The MDIO controller is represented in the DT binding by a
+* subnode of the MAC controller.
+*
+* Our driver largely ignores that and supports MDIO by
+* hardcoding the register offsets.
+*
+* However, some resources (clocks, pinctrl) might be tied to
+* the MDIO node, and thus ignored.
+*
+* Clocks are not a concern at the moment since it's shared
+* between the MAC and MDIO, so the driver handles both at the
+* same time.
+*
+* However, we do need to make sure the pins states tied to the
+* MDIO node are configured properly. Fortunately, the core DM
+* does that for use when we get a device, so we can work around
+* that whole issue by just requesting a dummy MDIO driver to
+* probe, and our pins will get muxed.
+*/
+   ret = uclass_get_device_by_ofnode(UCLASS_MDIO, mdio, _dev);
+   if (ret)
+   return ret;
+
+   return 0;
+}
+
 static int am65_cpsw_mdio_init(struct udevice *dev)
 {
struct am65_cpsw_priv *priv = dev_get_priv(dev);
struct am65_cpsw_common *cpsw_common = priv->cpsw_common;
+   int ret;
 
if (!priv->has_phy || cpsw_common->bus)
return 0;
 
+   ret = am65_cpsw_mdio_setup(dev);
+   if (ret)
+   return ret;
+
cpsw_common->bus = cpsw_mdio_init(dev->name,
  cpsw_common->mdio_base,
  cpsw_common->bus_freq,
@@ -854,3 +910,14 @@ U_BOOT_DRIVER(am65_cpsw_nuss_port) = {
.plat_auto  = sizeof(struct eth_pdata),
.flags = DM_FLAG_ALLOC_PRIV_DMA | DM_FLAG_OS_PREPARE,
 };
+
+static const struct udevice_id am65_cpsw_mdio_ids[] = {
+   { .compatible = "ti,cpsw-mdio" },
+   { }
+};
+
+U_BOOT_DRIVER(am65_cpsw_mdio) = {
+   .name   = "am65_cpsw_mdio",
+   .id = UCLASS_MDIO,
+   .of_match   = am65_cpsw_mdio_ids,
+};

-- 
2.41.0

[PATCH v2 0/3] net: ti: am65-cpsw-nuss: Fix DT binding handling of pinctrl

[Maxime Ripard]

Hi,

This series is based on:
https://lore.kernel.org/all/20230713072019.3153871-1...@ti.com/
https://lore.kernel.org/all/20230720215935.107398-1-rog...@kernel.org/

It fixes the issue of Linux booting from the DT embedded by U-boot. The
main issue there is that U-Boot doesn't handle the MDIO child node that
might have resources attached to it.

Thus, any pinctrl configuration that could be attached to the MDIO
child node is effectively ignored. Unfortunately, starting with 6.5-rc1,
Linux does just that.

This was solved by duplicating the pinctrl configuration onto the MAC
device node. Unfortunately, this doesn't work for Linux since now it has
two devices competing for the same pins.

Let me know what you think,
Maxime

Signed-off-by: Maxime Ripard 
---
Changes in v2:
- Drop the pinctrl API change in favour of a dummy driver
- Link to v1: 
https://lore.kernel.org/r/20230720-ti-mdio-pinmux-v1-0-0bd3bd1cf...@kernel.org

---
Maxime Ripard (3):
  net: ti: am65-cpsw-nuss: Enforce pinctrl state on the MDIO child node
  fixup! arm: dts: k3-am62: Bump dtsi from linux v6.5-rc1
  fixup! arm: dts: k3-am62: Bump dtsi from linux v6.5-rc1

 arch/arm/dts/k3-am625-sk-u-boot.dtsi |  7 ++--
 drivers/net/ti/Kconfig   |  1 +
 drivers/net/ti/am65-cpsw-nuss.c  | 67 
 3 files changed, 73 insertions(+), 2 deletions(-)
---
base-commit: e410677c49839480c8299eed90fe71557e0a37e9
change-id: 20230720-ti-mdio-pinmux-a12525dba973

Best regards,
-- 
Maxime Ripard 

Re: [PATCH 0/2] net: ti: am65-cpsw-nuss: Drop custom property "mac_efuse"

[Maxime Ripard]

Hi,

On Fri, Jul 21, 2023 at 12:59:33AM +0300, Roger Quadros wrote:
> Hi,
> 
> We need to track the Device tree in Linux.
> The approved property for MAC address EFUSE is "ti,syscon-efuse".
> 
> Use that and drop custom property "mac_efuse".

On a SK-AM62:

Reviewed-by: Maxime Ripard 
Tested-by: Maxime Ripard 

Thanks!
Maxime


signature.asc
Description: PGP signature

Re: [PATCH 2/4] net: ti: am65-cpsw-nuss: Enforce pinctrl state on the MDIO child node

[Maxime Ripard]

On Thu, Jul 20, 2023 at 06:47:43PM +0300, Roger Quadros wrote:
> On 20/07/2023 12:55, Maxime Ripard wrote:
> > The binding represents the MDIO controller as a child device tree
> > node of the MAC device tree node.
> > 
> > The U-Boot driver mostly ignores that child device tree node and just
> > hardcodes the resources it uses to support both the MAC and MDIO in a
> > single driver.
> > 
> > However, some resources like pinctrl muxing states are thus ignored.
> > This has been a problem with device trees since Linux 6.5-rc1 which will
> > put some pinctrl states on the MDIO device tree node.
> > 
> > Let's rework the driver a bit to fetch the pinctrl state from the MDIO
> > node and apply it.
> > 
> > Signed-off-by: Maxime Ripard 
> > ---
> >  drivers/net/ti/am65-cpsw-nuss.c | 49 
> > +
> >  1 file changed, 49 insertions(+)
> > 
> > diff --git a/drivers/net/ti/am65-cpsw-nuss.c 
> > b/drivers/net/ti/am65-cpsw-nuss.c
> > index f674b0baa359..2d579bf4af2c 100644
> > --- a/drivers/net/ti/am65-cpsw-nuss.c
> > +++ b/drivers/net/ti/am65-cpsw-nuss.c
> > @@ -15,6 +15,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -696,6 +697,50 @@ out:
> > return ret;
> >  }
> >  
> > +static ofnode am65_cpsw_find_mdio(ofnode parent)
> > +{
> > +   ofnode node;
> > +
> > +   ofnode_for_each_subnode(node, parent)
> > +   if (ofnode_device_is_compatible(node, "ti,cpsw-mdio"))
> > +   return node;
> > +
> > +   return ofnode_null();
> > +}
> > +
> > +static int am65_cpsw_setup_mdio(struct udevice *dev)
> > +{
> > +   ofnode mdio;
> > +   int ret;
> > +
> > +   mdio = am65_cpsw_find_mdio(dev_ofnode(dev));
> > +   if (!ofnode_valid(mdio))
> > +   return -ENODEV;
> > +
> > +   /*
> > +* The MDIO controller is represented in the DT binding by a
> > +* subnode of the MAC controller.
> > +*
> > +* Our driver largely ignores that and supports MDIO by
> > +* hardcoding the register offsets.
> > +*
> > +* However, some resources (clocks, pinctrl) might be tied to
> > +* the MDIO node, and thus ignored.
> > +*
> > +* Clocks are not a concern at the moment since it's shared
> > +* between the MAC and MDIO, so the driver handles both at the
> > +* same time.
> > +*
> > +* However, we do need to make sure the pins states tied to the
> > +* MDIO node are configured properly.
> > +*/
>
> Please mention this as a HACK: that needs to go away when davinci_mdio
> and this driver properly supports MDIO infrastructure.

Will do

> > +   ret = pinctrl_select_state_by_ofnode(dev, mdio, "default");
> 
> Instead of modifying the API, can we have a dummy driver for the 
> "ti,davinci_mdio"
> device that registers as class UCLASS_MDIO but does nothing else?
> 
> Then you can drop patch 1 and instead do
> 
>   ret = uclass_get_device_by_ofnode(UCLASS_MDIO, node, _dev);
> if (!ret)
>   pinctrl_select_state(mdio_dev, "default");

Good idea, thanks :)

Maxime


signature.asc
Description: PGP signature

Re: [PATCH 4/4] fixup! arm: dts: k3-am62: Bump dtsi from linux v6.5-rc1

[Maxime Ripard]

On Fri, Jul 21, 2023 at 12:14:35PM +0300, Roger Quadros wrote:
> 
> 
> On 21/07/2023 10:46, Maxime Ripard wrote:
> > On Thu, Jul 20, 2023 at 06:27:56PM +0300, Roger Quadros wrote:
> >>
> >>
> >> On 20/07/2023 12:55, Maxime Ripard wrote:
> >>> Dropping ranges entirely doesn't work since the register offset on the
> >>> MDIO device node will now be completely off, so we need to adjust it to
> >>> the right value without the translation.
> >>>
> >>> We also need to have an empty ranges property for the reg address to be
> >>> properly evaluated.
> >>>
> >>> Signed-off-by: Maxime Ripard 
> >>> ---
> >>>  arch/arm/dts/k3-am625-sk-u-boot.dtsi | 6 +-
> >>>  1 file changed, 5 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/arch/arm/dts/k3-am625-sk-u-boot.dtsi 
> >>> b/arch/arm/dts/k3-am625-sk-u-boot.dtsi
> >>> index fe1c7408a89d..2ec3fff99f32 100644
> >>> --- a/arch/arm/dts/k3-am625-sk-u-boot.dtsi
> >>> +++ b/arch/arm/dts/k3-am625-sk-u-boot.dtsi
> >>> @@ -122,8 +122,8 @@
> >>>   reg = <0x0 0x800 0x0 0x20>,
> >>> <0x0 0x43000200 0x0 0x8>;
> >>>   reg-names = "cpsw_nuss", "mac_efuse";
> >>> - /delete-property/ ranges;
> >>>   bootph-pre-ram;
> >>> + ranges;
> >>>  
> >>>   cpsw-phy-sel@04044 {
> >>>   compatible = "ti,am64-phy-gmii-sel";
> >>> @@ -132,6 +132,10 @@
> >>>   };
> >>>  };
> >>>  
> >>> +_mdio {
> >>> + reg = <0x0 0x8000f00 0x0 0x100>;
> >>> +};
> >>> +
> >>
> >> Why this change?
> >>
> >> Linux DT has
> >> cpsw3g_mdio: mdio@f00 {
> >> compatible = "ti,cpsw-mdio","ti,davinci_mdio";
> >> reg = <0x00 0xf00 0x00 0x100>;
> >>
> >> And it is a child of cpsw3g node.
> > 
> > Right, but Linux also has on the cpsw3g node:
> > ranges = <0x00 0x00 0x00 0x0800 0x00 0x20>;
> > 
> > Which means that child node don't get a 1:1 mapping but we get a
> > 0x0800 offset.
> 
> The child nodes should get 0x0800 offset because they sit inside
> CPSW address range.

Right.

> The addition of cpsw-phy-sel@04044 node in u-boot.dtsi is wrong
> as it sits in the control module and not in CPSW address range.
> 
> I'll send out the patch to teach am65-cpsw u-boot driver to correctly
> fetch the cpsw-phy-sel via syscon handle phy.
> 
> The /delete-property/ ranges can then be removed.

Great.

> > 
> > Nishanth's patch was removing the ranges property because the
> > translation is troublesome for the new cpsw-phy-sel@04044 node (I
> > assume), so we need to add the offset to the mdio node so that it still
> > expresses the same base address.
> 
> No, ranges property should be there else address translation will fail
> for MDIO node. And that's why you don't need to change the cpsw3g_mdio
> address in this patch.

Sure. My point was that, with the /delete-property/ ranges above, that
translation doesn't occur anymore and the MDIO base address is wrong.
This patch is an attempt at fixing that, removing the delete-property is
another.

Maxime


signature.asc
Description: PGP signature

Re: [PATCH 4/4] fixup! arm: dts: k3-am62: Bump dtsi from linux v6.5-rc1

[Maxime Ripard]

On Thu, Jul 20, 2023 at 06:27:56PM +0300, Roger Quadros wrote:
> 
> 
> On 20/07/2023 12:55, Maxime Ripard wrote:
> > Dropping ranges entirely doesn't work since the register offset on the
> > MDIO device node will now be completely off, so we need to adjust it to
> > the right value without the translation.
> > 
> > We also need to have an empty ranges property for the reg address to be
> > properly evaluated.
> > 
> > Signed-off-by: Maxime Ripard 
> > ---
> >  arch/arm/dts/k3-am625-sk-u-boot.dtsi | 6 +-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/dts/k3-am625-sk-u-boot.dtsi 
> > b/arch/arm/dts/k3-am625-sk-u-boot.dtsi
> > index fe1c7408a89d..2ec3fff99f32 100644
> > --- a/arch/arm/dts/k3-am625-sk-u-boot.dtsi
> > +++ b/arch/arm/dts/k3-am625-sk-u-boot.dtsi
> > @@ -122,8 +122,8 @@
> > reg = <0x0 0x800 0x0 0x20>,
> >   <0x0 0x43000200 0x0 0x8>;
> > reg-names = "cpsw_nuss", "mac_efuse";
> > -   /delete-property/ ranges;
> > bootph-pre-ram;
> > +   ranges;
> >  
> > cpsw-phy-sel@04044 {
> > compatible = "ti,am64-phy-gmii-sel";
> > @@ -132,6 +132,10 @@
> > };
> >  };
> >  
> > +_mdio {
> > +   reg = <0x0 0x8000f00 0x0 0x100>;
> > +};
> > +
> 
> Why this change?
> 
> Linux DT has
> cpsw3g_mdio: mdio@f00 {
> compatible = "ti,cpsw-mdio","ti,davinci_mdio";
> reg = <0x00 0xf00 0x00 0x100>;
> 
> And it is a child of cpsw3g node.

Right, but Linux also has on the cpsw3g node:
ranges = <0x00 0x00 0x00 0x0800 0x00 0x20>;

Which means that child node don't get a 1:1 mapping but we get a
0x0800 offset.

Nishanth's patch was removing the ranges property because the
translation is troublesome for the new cpsw-phy-sel@04044 node (I
assume), so we need to add the offset to the mdio node so that it still
expresses the same base address.

Maxime


signature.asc
Description: PGP signature

Re: [PATCH 0/4] net: ti: am65-cpsw-nuss: Fix DT binding handling of pinctrl

[Maxime Ripard]

Hi

Sorry, I didn't receive Roger mail so I'll reply here

On Thu, Jul 20, 2023 at 09:00:19AM -0500, Nishanth Menon wrote:
> On 16:56-20230720, Roger Quadros wrote:
> > Hi,
> > 
> > On 20/07/2023 16:33, Ravi Gunasekaran wrote:
> > > 
> > > 
> > > On 7/20/2023 3:25 PM, Maxime Ripard wrote:
> > >> Hi,
> > >>
> > >> This series is based on:
> > >> https://lore.kernel.org/all/20230713072019.3153871-1...@ti.com/
> > >>
> > >> It fixes the issue of Linux booting from the DT embedded by U-boot. The
> > >> main issue there is that U-Boot doesn't handle the MDIO child node that
> > >> might have resources attached to it.
> > >>
> > >> Thus, any pinctrl configuration that could be attached to the MDIO
> > >> child node is effectively ignored. Unfortunately, starting with 6.5-rc1,
> > >> Linux does just that.
> > 
> > I didn't get this part. Linux does not ignore pinctrl configuration attached
> > to MDIO child node. What changed in 6.5-rc1?

Linux doesn't ignore it, but Linux started putting pinctrl configuration
on the MDIO node, which is ignored by U-Boot.

> > >> This was solved by duplicating the pinctrl configuration onto the MAC
> > 
> > If I remember right, there is no driver model driver for MDIO in u-boot and
> > adding the mdio pinctrl into CPSW DT node was a hack in u-boot.

Yeah, but we now have in the U-Boot DT two nodes referencing the same
pinctrl configuration: the CPSW and the MDIO node. Linux then tries to
assign that configuration to both devices and it fails.

> > >> device node. Unfortunately, this doesn't work for Linux since now it has
> > >> two devices competing for the same pins.
> > 
> > What has really changed here is that you are passing u-boot's device
> > tree to Linux.

I didn't change anything. This is the default boot process if you don't
provide a DT in the ESP partition.

> > This has nothing to do with 6.5-rc1 right?

The issue started to occur with Nishanth patch to sync with Linux
6.5-rc1 device trees, so there's definitely a connection.

> > I suppose your solution is still a hack but of lesser evil than
> > duplicating MDIO pinctrl in CPSW node.
> > 
> > The proper solution would be to implement driver model for the
> > davinci MDIO driver. Siddharth has been working on this. If that is
> > close to ready we should just use that instead.
> 
> But this allows for a cleaner device tree while the driver can be fixed
> up independently, correct? Unfortunately, Siddharth's driver model work,
> from what I understand, is around 2024 timeframe.. which is probably not
> something that helps us in the community at this point.

Yeah, at the moment I have to choose between getting the MMC to work in
Linux or the Ethernet ports. The former is working thanks to your patch,
the latter is broken because of it. Ideally I'd like both :)

Maxime


signature.asc
Description: PGP signature

[PATCH 4/4] fixup! arm: dts: k3-am62: Bump dtsi from linux v6.5-rc1

[Maxime Ripard]

Dropping ranges entirely doesn't work since the register offset on the
MDIO device node will now be completely off, so we need to adjust it to
the right value without the translation.

We also need to have an empty ranges property for the reg address to be
properly evaluated.

Signed-off-by: Maxime Ripard 
---
 arch/arm/dts/k3-am625-sk-u-boot.dtsi | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm/dts/k3-am625-sk-u-boot.dtsi 
b/arch/arm/dts/k3-am625-sk-u-boot.dtsi
index fe1c7408a89d..2ec3fff99f32 100644
--- a/arch/arm/dts/k3-am625-sk-u-boot.dtsi
+++ b/arch/arm/dts/k3-am625-sk-u-boot.dtsi
@@ -122,8 +122,8 @@
reg = <0x0 0x800 0x0 0x20>,
  <0x0 0x43000200 0x0 0x8>;
reg-names = "cpsw_nuss", "mac_efuse";
-   /delete-property/ ranges;
bootph-pre-ram;
+   ranges;
 
cpsw-phy-sel@04044 {
compatible = "ti,am64-phy-gmii-sel";
@@ -132,6 +132,10 @@
};
 };
 
+_mdio {
+   reg = <0x0 0x8000f00 0x0 0x100>;
+};
+
 _port1 {
bootph-pre-ram;
 };

-- 
2.41.0

[PATCH 2/4] net: ti: am65-cpsw-nuss: Enforce pinctrl state on the MDIO child node

[Maxime Ripard]

The binding represents the MDIO controller as a child device tree
node of the MAC device tree node.

The U-Boot driver mostly ignores that child device tree node and just
hardcodes the resources it uses to support both the MAC and MDIO in a
single driver.

However, some resources like pinctrl muxing states are thus ignored.
This has been a problem with device trees since Linux 6.5-rc1 which will
put some pinctrl states on the MDIO device tree node.

Let's rework the driver a bit to fetch the pinctrl state from the MDIO
node and apply it.

Signed-off-by: Maxime Ripard 
---
 drivers/net/ti/am65-cpsw-nuss.c | 49 +
 1 file changed, 49 insertions(+)

diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c
index f674b0baa359..2d579bf4af2c 100644
--- a/drivers/net/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ti/am65-cpsw-nuss.c
@@ -15,6 +15,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -696,6 +697,50 @@ out:
return ret;
 }
 
+static ofnode am65_cpsw_find_mdio(ofnode parent)
+{
+   ofnode node;
+
+   ofnode_for_each_subnode(node, parent)
+   if (ofnode_device_is_compatible(node, "ti,cpsw-mdio"))
+   return node;
+
+   return ofnode_null();
+}
+
+static int am65_cpsw_setup_mdio(struct udevice *dev)
+{
+   ofnode mdio;
+   int ret;
+
+   mdio = am65_cpsw_find_mdio(dev_ofnode(dev));
+   if (!ofnode_valid(mdio))
+   return -ENODEV;
+
+   /*
+* The MDIO controller is represented in the DT binding by a
+* subnode of the MAC controller.
+*
+* Our driver largely ignores that and supports MDIO by
+* hardcoding the register offsets.
+*
+* However, some resources (clocks, pinctrl) might be tied to
+* the MDIO node, and thus ignored.
+*
+* Clocks are not a concern at the moment since it's shared
+* between the MAC and MDIO, so the driver handles both at the
+* same time.
+*
+* However, we do need to make sure the pins states tied to the
+* MDIO node are configured properly.
+*/
+   ret = pinctrl_select_state_by_ofnode(dev, mdio, "default");
+   if (ret)
+   return ret;
+
+   return 0;
+}
+
 static int am65_cpsw_probe_nuss(struct udevice *dev)
 {
struct am65_cpsw_common *cpsw_common = dev_get_priv(dev);
@@ -728,6 +773,10 @@ static int am65_cpsw_probe_nuss(struct udevice *dev)
AM65_CPSW_CPSW_NU_ALE_BASE;
cpsw_common->mdio_base = cpsw_common->ss_base + AM65_CPSW_MDIO_BASE;
 
+   ret = am65_cpsw_setup_mdio(dev);
+   if (ret)
+   goto out;
+
ports_np = dev_read_subnode(dev, "ethernet-ports");
if (!ofnode_valid(ports_np)) {
ret = -ENOENT;

-- 
2.41.0

[PATCH 3/4] fixup! arm: dts: k3-am62: Bump dtsi from linux v6.5-rc1

[Maxime Ripard]

The MDIO pinctrl nodes can't be duplicated between the child and device,
because if we ever boot Linux with our DT it will try to attach that
pinctrl configuration to both the MAC and MDIO devices, which will
result in failure to probe.

Signed-off-by: Maxime Ripard 
---
 arch/arm/dts/k3-am625-sk-u-boot.dtsi | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/dts/k3-am625-sk-u-boot.dtsi 
b/arch/arm/dts/k3-am625-sk-u-boot.dtsi
index 76589c7025a0..fe1c7408a89d 100644
--- a/arch/arm/dts/k3-am625-sk-u-boot.dtsi
+++ b/arch/arm/dts/k3-am625-sk-u-boot.dtsi
@@ -124,7 +124,6 @@
reg-names = "cpsw_nuss", "mac_efuse";
/delete-property/ ranges;
bootph-pre-ram;
-   pinctrl-0 = <_mdio1_pins_default _rgmii1_pins_default>;
 
cpsw-phy-sel@04044 {
compatible = "ti,am64-phy-gmii-sel";

-- 
2.41.0

[PATCH 1/4] pinctrl: Create a select_state variant with the ofnode

[Maxime Ripard]

Some drivers might not follow entirely the driver/device association,
and thus might support what should be multiple devices in a single
driver.

Such a driver is am65-cpsw-nuss, where the MAC and MDIO controllers are
different device tree nodes, with their own resources (including pinctrl
pins) but supported by a single driver tied to the MAC device in U-Boot.

In order to get the proper pinctrl state, we would need to get the
state from the MDIO device tree node, but tie it to the MAC device since
it's the only thing we have access to.

Signed-off-by: Maxime Ripard 
---
 drivers/pinctrl/pinctrl-uclass.c | 15 +--
 include/dm/pinctrl.h | 26 --
 2 files changed, 29 insertions(+), 12 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-uclass.c b/drivers/pinctrl/pinctrl-uclass.c
index 73dd7b1038bb..9e28f1858cbd 100644
--- a/drivers/pinctrl/pinctrl-uclass.c
+++ b/drivers/pinctrl/pinctrl-uclass.c
@@ -53,7 +53,8 @@ static int pinctrl_config_one(struct udevice *config)
  * @statename: state name, like "default"
  * @return: 0 on success, or negative error code on failure
  */
-static int pinctrl_select_state_full(struct udevice *dev, const char 
*statename)
+static int pinctrl_select_state_full(struct udevice *dev, ofnode node,
+const char *statename)
 {
char propname[32]; /* long enough */
const fdt32_t *list;
@@ -61,7 +62,7 @@ static int pinctrl_select_state_full(struct udevice *dev, 
const char *statename)
struct udevice *config;
int state, size, i, ret;
 
-   state = dev_read_stringlist_search(dev, "pinctrl-names", statename);
+   state = ofnode_stringlist_search(node, "pinctrl-names", statename);
if (state < 0) {
char *end;
/*
@@ -74,7 +75,7 @@ static int pinctrl_select_state_full(struct udevice *dev, 
const char *statename)
}
 
snprintf(propname, sizeof(propname), "pinctrl-%d", state);
-   list = dev_read_prop(dev, propname, );
+   list = ofnode_get_property(node, propname, );
if (!list)
return -ENOSYS;
 
@@ -293,20 +294,22 @@ static int pinctrl_select_state_simple(struct udevice 
*dev)
return ops->set_state_simple(pctldev, dev);
 }
 
-int pinctrl_select_state(struct udevice *dev, const char *statename)
+int pinctrl_select_state_by_ofnode(struct udevice *dev, ofnode node,
+  const char *statename)
 {
/*
 * Some device which is logical like mmc.blk, do not have
 * a valid ofnode.
 */
-   if (!dev_has_ofnode(dev))
+   if (!ofnode_valid(node))
return 0;
+
/*
 * Try full-implemented pinctrl first.
 * If it fails or is not implemented, try simple one.
 */
if (CONFIG_IS_ENABLED(PINCTRL_FULL))
-   return pinctrl_select_state_full(dev, statename);
+   return pinctrl_select_state_full(dev, node, statename);
 
return pinctrl_select_state_simple(dev);
 }
diff --git a/include/dm/pinctrl.h b/include/dm/pinctrl.h
index e3e50afeaff0..be4679b7f20d 100644
--- a/include/dm/pinctrl.h
+++ b/include/dm/pinctrl.h
@@ -502,6 +502,24 @@ static inline int pinctrl_generic_set_state(struct udevice 
*pctldev,
 #endif
 
 #if CONFIG_IS_ENABLED(PINCTRL)
+/**
+ * pinctrl_select_state_by_ofnode() - Set a device to a given state using the 
given ofnode
+ * @dev:   Peripheral device
+ * @ofnode:Device Tree node to get the state from
+ * @statename: State name, like "default"
+ *
+ * Return: 0 on success, or negative error code on failure
+ */
+int pinctrl_select_state_by_ofnode(struct udevice *dev, ofnode node, const 
char *statename);
+#else
+static inline int pinctrl_select_state_by_ofnode(struct udevice *dev,
+ofnode node,
+const char *statename)
+{
+   return -ENOSYS;
+}
+#endif
+
 /**
  * pinctrl_select_state() - Set a device to a given state
  * @dev:   Peripheral device
@@ -509,14 +527,10 @@ static inline int pinctrl_generic_set_state(struct 
udevice *pctldev,
  *
  * Return: 0 on success, or negative error code on failure
  */
-int pinctrl_select_state(struct udevice *dev, const char *statename);
-#else
-static inline int pinctrl_select_state(struct udevice *dev,
-  const char *statename)
+static inline int pinctrl_select_state(struct udevice *dev, const char 
*statename)
 {
-   return -ENOSYS;
+   return pinctrl_select_state_by_ofnode(dev, dev_ofnode(dev), statename);
 }
-#endif
 
 /**
  * pinctrl_request() - Request a particular pinctrl function

-- 
2.41.0

[PATCH 0/4] net: ti: am65-cpsw-nuss: Fix DT binding handling of pinctrl

[Maxime Ripard]

Hi,

This series is based on:
https://lore.kernel.org/all/20230713072019.3153871-1...@ti.com/

It fixes the issue of Linux booting from the DT embedded by U-boot. The
main issue there is that U-Boot doesn't handle the MDIO child node that
might have resources attached to it.

Thus, any pinctrl configuration that could be attached to the MDIO
child node is effectively ignored. Unfortunately, starting with 6.5-rc1,
Linux does just that.

This was solved by duplicating the pinctrl configuration onto the MAC
device node. Unfortunately, this doesn't work for Linux since now it has
two devices competing for the same pins.

Let me know what you think,
Maxime

Signed-off-by: Maxime Ripard 
---
Maxime Ripard (4):
  pinctrl: Create a select_state variant with the ofnode
  net: ti: am65-cpsw-nuss: Enforce pinctrl state on the MDIO child node
  fixup! arm: dts: k3-am62: Bump dtsi from linux v6.5-rc1
  fixup! arm: dts: k3-am62: Bump dtsi from linux v6.5-rc1

 arch/arm/dts/k3-am625-sk-u-boot.dtsi |  7 --
 drivers/net/ti/am65-cpsw-nuss.c  | 49 
 drivers/pinctrl/pinctrl-uclass.c | 15 ++-
 include/dm/pinctrl.h | 26 ++-
 4 files changed, 83 insertions(+), 14 deletions(-)
---
base-commit: acff6e7c553d5a839e885730a4018465a34ba5a7
change-id: 20230720-ti-mdio-pinmux-a12525dba973

Best regards,
-- 
Maxime Ripard 

Re: [RFC PATCH 4/4] arm: dts: k3-am62: Bump dtsi from linux v6.5-rc1

[Maxime Ripard]

On Wed, Jul 19, 2023 at 09:05:55AM -0500, Nishanth Menon wrote:
> On 10:27-20230719, Maxime Ripard wrote:
> > Hi Nishanth,
> > 
> > On Thu, Jul 13, 2023 at 02:20:19AM -0500, Nishanth Menon wrote:
> [...]
> 
> > > - Add mdio pins to the cpsw3g pinctrl in u-boot dtsi. It moved to a 
> > > subnode in the
> > >   linux dtsi that u-boot doesn't use/support
> > 
> > Unfortunately, this introduces a regression in Linux where the pinctrl
> > node is now affected to both the MAC and PHY, and we end up not being
> > able to probe both.
> 
> 
> Uggh this is the case where u-boot hands over it's device tree to
> linux kernel to boot, I presume?

Yes, it is :)

I've started to look into making the MAC driver grab the MDIO pinctrl
state, but it doesn't look trivial to do since the pinctrl API doesn't
support taking an ofnode pointer instead of a device. Shouldn't be too
hard either though.

Do you know if there's any other way to fix this?

Maxime


signature.asc
Description: PGP signature

Re: [RFC PATCH 4/4] arm: dts: k3-am62: Bump dtsi from linux v6.5-rc1

[Maxime Ripard]

Hi Nishanth,

On Thu, Jul 13, 2023 at 02:20:19AM -0500, Nishanth Menon wrote:
> Update the am62 and am625 device-trees from linux v6.3-rc5 This needed the 
> followin
> tweaks to the u-boot specific dtsi as well:
> - Switch tick-timer to the main_timer as it's now defined in the main dtsi
> - Secure proxies are defined in Soc dtsis
> - Drop duplicate nodes - u-boot.dtsi is includes in r5-sk, no need for
>   either the definitions from main.dtsi OR duplication from u-boot.dtsi
> - Add mdio pins to the cpsw3g pinctrl in u-boot dtsi. It moved to a subnode 
> in the
>   linux dtsi that u-boot doesn't use/support

Unfortunately, this introduces a regression in Linux where the pinctrl
node is now affected to both the MAC and PHY, and we end up not being
able to probe both.

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v7 00/23] Migration to using binman for bootloader

[Maxime Ripard]

On Mon, Jul 17, 2023 at 06:38:22PM +0530, Neha Malcom Francis wrote:
> Hi Maxime
> 
> On 17/07/23 18:19, Maxime Ripard wrote:
> > Hi,
> > 
> > On Fri, Jul 14, 2023 at 07:20:47PM +0530, Neha Malcom Francis wrote:
> > > This series aims to eliminate the use of additional custom repositories
> > > such as k3-image-gen (K3 Image Generation) repo and core-secdev-k3 (K3
> > > Security Development Tools) that was plumbed into the U-Boot build flow
> > > to generate boot images for TI K3 platform devices. And instead, we move
> > > towards using binman that aligns better with the community standard build
> > > flow.
> > > 
> > > This series uses binman for all K3 platforms supported on U-Boot 
> > > currently;
> > > both HS (High Security, both SE and FS) and GP (General Purpose) devices.
> > > 
> > > Background on using k3-image-gen:
> > >   * TI K3 devices require a SYSFW (System Firmware) image consisting
> > >   of a signed system firmware image and board configuration binaries,
> > >   this is needed to bring up system firmware during U-Boot R5 SPL
> > >   startup.
> > >   * Board configuration data contain board-specific information
> > >   such as resource management, power management and security.
> > > 
> > > Background on using core-secdev-k3:
> > >   * Contains resources to sign x509 certificates for HS devices
> > > 
> > > Series intends to use binman to take over the packaging and signing for
> > > the R5 bootloader images tiboot3.bin (and sysfw.itb, for non-combined
> > > boot flow) instead of k3-image-gen.
> > > 
> > > Series also packages the A72/A53 bootloader images (tispl.bin and
> > > u-boot.img) using ATF, OPTEE and DM (Device Manager)
> > 
> > Tested-by: Maxime Ripard 
> > 
> 
> Thanks for testing!
> 
> > It took a while to figure out that the tiboot3.bin file was for the
> > HS-FS variant now, while I was using a GP board.
>
> Depends on the board, based on the commonly available boards tiboot3.bin is
> symlinked to either GP or HS-FS.

Sure, I guess that my point was that prior to your patches we would
specify the firmware that match our board and thus tiboot3.bin was
always the right one.

Now, it can be the wrong one and we have, most likely, to use the proper
image instead of following the symlink. But the doc still says we should
pick the symlink.

> > Maybe we should clarify that in the doc?
> 
> But yes, I will factor this into the docs.

Thanks :)

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v7 00/23] Migration to using binman for bootloader

[Maxime Ripard]

Hi,

On Fri, Jul 14, 2023 at 07:20:47PM +0530, Neha Malcom Francis wrote:
> This series aims to eliminate the use of additional custom repositories
> such as k3-image-gen (K3 Image Generation) repo and core-secdev-k3 (K3
> Security Development Tools) that was plumbed into the U-Boot build flow
> to generate boot images for TI K3 platform devices. And instead, we move
> towards using binman that aligns better with the community standard build
> flow.
> 
> This series uses binman for all K3 platforms supported on U-Boot currently;
> both HS (High Security, both SE and FS) and GP (General Purpose) devices.
> 
> Background on using k3-image-gen:
>   * TI K3 devices require a SYSFW (System Firmware) image consisting
>   of a signed system firmware image and board configuration binaries,
>   this is needed to bring up system firmware during U-Boot R5 SPL
>   startup.
>   * Board configuration data contain board-specific information
>   such as resource management, power management and security.
> 
> Background on using core-secdev-k3:
>   * Contains resources to sign x509 certificates for HS devices
> 
> Series intends to use binman to take over the packaging and signing for
> the R5 bootloader images tiboot3.bin (and sysfw.itb, for non-combined
> boot flow) instead of k3-image-gen.
> 
> Series also packages the A72/A53 bootloader images (tispl.bin and
> u-boot.img) using ATF, OPTEE and DM (Device Manager)

Tested-by: Maxime Ripard 

It took a while to figure out that the tiboot3.bin file was for the
HS-FS variant now, while I was using a GP board.

Maybe we should clarify that in the doc?

Maxime


signature.asc
Description: PGP signature

Re: [RFC PATCH 0/4] arm: dts: Sync k3-am62 with upstream

[Maxime Ripard]

Hi,

On Thu, Jul 13, 2023 at 02:20:15AM -0500, Nishanth Menon wrote:
> This series depends on Neha's binman series[1] and is intended for
> review and I will repost once binman series is merged in.
> 
> I have picked up part of Sjoerd's series[2] to keep it constrained just
> to dts sync at this point rather than adding newer functionality on top.
> 
> You can find the full series in [3].
> 
> Attempting am625 as a start of a series of clean ups for device tree for
> u-boot, but will love to get any review comments to improve this series
> up.

Tested-by: Maxime Ripard 

This also fixes an MMC issue when booting Linux with the U-Boot device tree.

Maxime


signature.asc
Description: PGP signature

Re: [PATCH] sunxi-nand: fix the PIO instead of DMA implementation

[Maxime Ripard]

On Thu, Jun 30, 2022 at 10:04:31AM +0200, Markus Hoffrogge wrote:
> Hi Miquel, hi Maxime,
> 
> could you probably run a test on a newer Allwinner SOC - like e.g.
> SUNI8, SUNI9, SUNI50 or newer, since I can test it on a SUNI7 (A20)
> board only?
> 
> The only aspect that makes me wonder is, that it did work on your
> hardware before at all.
> 
> Maybe this is, since that NFC_CTL_RAM_METHOD flag does not matter on
> newer SOC, if the DMA channel is not enabled for the NFC.

I don't have any of those boards anymore, but that sounds plausible

Maxime


signature.asc
Description: PGP signature

Re: [PATCH] sunxi-nand: fix the PIO instead of DMA implementation

[Maxime Ripard]

On Thu, Jun 30, 2022 at 09:13:22AM +0200, Miquel Raynal wrote:
> Hi Markus,
> 
> + Maxime, for the record :)
> 
> mhoffro...@gmail.com wrote on Thu, 30 Jun 2022 01:26:39 +0200:
> 
> > The sunxi nand SPL loader was broken at least for SUN4I,
> > SUN5I and SUN7I SOCs since the implementation change
> > from DMA to PIO usage - commit 6ddbb1e.
> > 
> > Root cause for this issue is the NFC control flag NFC_CTL_RAM_METHOD
> > being set by method nand_apply_config.
> > 
> > This flag controls the bus being used for the NFCs internal RAM access.
> > It must be set for the DMA use case only.
> > See A33_Nand_Flash_Controller_Specification.pdf page 12.
> > 
> > This fix is tested by myself on a Cubietruck A20 board.
> > Others should test it on new generation SOCs as well.
> 
> Good to know that someone tackled this, Maxime already reported some
> time ago that it was broken for a number of boards but I never took the
> time to investigate, apologies.

Oh, that's awesome, thanks for tackling this :)

Maxime


signature.asc
Description: PGP signature

Re: using device-tree fragments/overlays

[Maxime Ripard]

Hi,

On Thu, Oct 14, 2021 at 08:13:55AM -0700, Tim Harvey wrote:
> On Wed, Oct 13, 2021 at 1:38 AM Peter Robinson  wrote:
> >
> > Hi Tim,
> >
> > > I'm working on some features for the imx8mm-venice boards I support
> > > which depend on making small changes to the dt before booting Linux.
> > >
> > > The purpose is not to have any of this apply to the U-Boot controlling
> > > dt but instead to the Linux kernel dt applied within ft_board_setup. I
> > > could apply these changes with code but it would be way more readable
> > > if there was a way to store these as fragments in the FIT image and
> > > apply them.
> > >
> > > The fragments would be small and have to do with two UART's routed to
> > > a multi-protocol RS232/RS485 transceiver that can be used as either of
> > > the following:
> > >  - two tx/rx UART's (the default configuration for my dt's)
> > >  - one rs232 uart with CTS/RTS flow control
> > >  - one rs485 uart
> > >
> > > Here would be an example dt overlay to apply over the default
> > > arch/arm/dts/imx8mm-venice-gw73xx-0x.dts to change it from 2x RS232
> > > UARTS with TX/RX (uart2/uart4) to RS485 half duplex:
> > >
> > > /* For RS485 HD:
> > >  *  - UART4_TX is DE for RS485 transmitter
> > >  *  - RS485_EN needs to be pulled high
> > >  *  - RS485_HALF needs to be pulled high
> > >  *  - RS485_TERM enables on-chip termination
> > >  */
> > >  {
> > > rts-gpios = < 29 GPIO_ACTIVE_HIGH>;
> > > rs485-term-gpios = < 0 GPIO_ACTIVE_HIGH>;
> > > };
> > >
> > >  {
> > > status = "disabled";
> > > };
> > >
> > >  {
> > > pinctrl-names = "default";
> > > pinctrl-0 = <_hog>;
> > >
> > > pinctrl_hog: hoggrp {
> > > fsl,pins = <
> > > MX8MM_IOMUXC_GPIO1_IO00_GPIO1_IO0
> > > 0x4104 /* RS485_TERM */
> > > MX8MM_IOMUXC_SAI1_RXFS_GPIO4_IO0
> > > 0x4144 /* RS485_EN */
> > > MX8MM_IOMUXC_SAI1_RXD0_GPIO4_IO2
> > > 0x4144 /* RS485_HALF */
> > > >;
> > > };
> > >
> > > pinctrl_uart2: uart2grp {
> > > fsl,pins = <
> > > MX8MM_IOMUXC_UART2_RXD_UART2_DCE_RX 0x140
> > > MX8MM_IOMUXC_UART2_TXD_UART2_DCE_TX 0x140
> > > MX8MM_IOMUXC_UART4_TXD_GPIO5_IO29   0x140
> > > >;
> > > };
> > > };
> > >
> > > Anyone do anything like this before or work with dt fragments/overlays
> > > in U-Boot or have any other suggestions?
> >
> > A feature for overlays landed upstream in May [1], so the 2021,07
> > release, so I'm sure there's most of the functionality you require
> > already upstream. Not sure how a user would specify which of the
> > variants to use or how it would load it from the FIT but I'm sure the
> > patch series provides you a good start point.
> >
> > Peter
> >
> > [1] https://lists.denx.de/pipermail/u-boot/2021-May/448794.html
> 
> 
> Peter,
> 
> Thanks - this is very helpful and is what I was looking for. My plan
> is to use the hwconfig env var for users to select how they want
> serial configured. I can use the methods used in the extension_apply
> function and it seems appropriate for me to put dtbo's on the root
> filesystem with the dtb's when using this.

If you have control over the FIT image creation and the uboot scripts,
it might not even needed:

https://source.denx.de/u-boot/u-boot/-/blob/master/doc/uImage.FIT/overlay-fdt-boot.txt

Maxime


signature.asc
Description: PGP signature

Re: [U-Boot] [PATCH v3 13/20] spl: nand: sunxi: use PIO instead of DMA

[Maxime Ripard]

Hi,

On Tue, Jul 06, 2021 at 06:00:33PM +0200, Miquel Raynal wrote:
> Hi Maxime,
> 
> Maxime Ripard  wrote on Thu, 24 Jun 2021 14:05:40
> +0200:
> 
> > Hi Miquel,
> > 
> > On Wed, Feb 28, 2018 at 08:51:55PM +0100, Miquel Raynal wrote:
> > > SPL support was first written to support only the earlier generations of
> > > Allwinner SoCs, and was only really enabled on the A13 / GR8. However,
> > > those old SoCs had a DMA engine that has been replaced since the A31 by
> > > another DMA controller that is no longer compatible.
> > > 
> > > Since the code directly uses that DMA controller, it cannot operate
> > > properly on the later SoCs, while the NAND controller has not changed.
> > > 
> > > There's two paths forward, the first one would have been to add support
> > > for that DMA controller too, the second to just remove the DMA usage
> > > entirely and rely on PIO.
> > > 
> > > The later has been chosen because CPU overload at this stage is not an
> > > issue and it makes the driver more generic, and easier to understand.
> > > 
> > > Signed-off-by: Miquel Raynal 
> > > Acked-by: Boris Brezillon   
> > 
> > I'm a bit late to the party, but this bricks the CHIP Pro too. While
> > U-Boot proper seems to be flashed properly (re-reading it from the NAND
> > after flashing brings up the same CRC than the original image), the SPL
> > will only read 0s.
> > 
> > The transfer does complete though, so maybe it's just the copy from the
> > SRAM to the main memory that doesn't work?
> > 
> > The offset looks correct though, so I'm not sure.
> 
> Strange... I really have no idea what's going on here and especially I
> don't have any suitable board with me these days to troubleshoot this.
> 
> Sorry for not being helpful at all on this one :-)

Paul has two CHIP Pro, and there might be more at the Bootlin office

Maxime


signature.asc
Description: PGP signature

Re: [PATCH] cmd: ums: Enable by default for sunxi

[Maxime Ripard]

On Tue, Jul 06, 2021 at 04:57:32PM +0100, Andre Przywara wrote:
> On Tue, 6 Jul 2021 19:56:24 +0530
> Jagan Teki  wrote:
> 
> Hi Jagan,
> 
> thanks for the response!
> 
> > On Tue, Jul 6, 2021 at 4:53 AM Andre Przywara  
> > wrote:
> > >
> > > The ums command (presenting a U-Boot block device as a USB mass storage
> > > device) is very useful for accessing eMMC devices via USB-OTG.
> > >
> > > At the moment we enable USB fastboot by default for Allwinner devices,
> > > so it makes sense to do the same with USB mass storage, which is
> > > actually more versatile and can be accessed on any USB host easily.
> > >
> > > Signed-off-by: Andre Przywara 
> > > ---
> > >  cmd/Kconfig | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > > index a9fb4eead29..7b1c96910a8 100644
> > > --- a/cmd/Kconfig
> > > +++ b/cmd/Kconfig
> > > @@ -1347,6 +1347,7 @@ config CMD_USB_MASS_STORAGE
> > > bool "UMS usb mass storage"
> > > select USB_FUNCTION_MASS_STORAGE
> > > depends on BLK && USB_GADGET
> > > +   default y if ARCH_SUNXI && USB_MUSB_GADGET
> > 
> > UMS or any secondary-emmc accessible are considered in production
> > u-boot config instead of mainline u-boot since we have fastboot
> > already available for this kind of purpose. This might be one of the
> > reason not conisidered UMS by default till now, atleast on Allwinner.
> 
> Well, I consider fastboot inferior to UMS. I understand that
> Android phone hackers use it all the time, but I think having a block
> device is far more versatile - you can partition it as you like, create
> and populate a filesystem, dd an image to it, extract or update
> firmware, etc. - most without extra software, even on Windows or Mac.
> I think fastboot offers a reboot command, but are there other killer
> reasons for it? And having the ums command in addition to fastboot does
> not hurt, both would still work.

fastboot abstracts away the storage medium, whereas UMS doesn't. You
cannot run UMS on a NAND or SPI-NOR, while this causes no issue at all
to fastboot.

Similarly, UMS exposes the whole partition to the host. You then need to
have a driver for that FS on that machine, otherwise you just can't
access it.

Sure, it's convenient, but it's far from being ubiquitous and the silver
bullet you claim it is.

> So are there any real arguments for not adding ums as well? I am not
> sure many people tweak their config, so I would like to offer a decent
> out-of-the-box experience, even for casual users.

As usual, we also have to balance that with the size limit, but I'm not
sure it's an issue these days?

Maxime


signature.asc
Description: PGP signature

Re: [PATCH 03/12] Revert "usb: gadget: fastboot: use correct max packet size"

[Maxime Ripard]

Hi Peng,

On Tue, Jun 29, 2021 at 10:27:27AM +0800, Peng Fan (OSS) wrote:
> Maxime
> 
> On 2021/6/25 21:05, Maxime Ripard wrote:
> > This reverts commit 27c9141b1114fd5721437abbb1c694e45e765f19.
> > ---
> >   drivers/usb/gadget/f_fastboot.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/gadget/f_fastboot.c 
> > b/drivers/usb/gadget/f_fastboot.c
> > index 8ba55aab9f8f..1fcffaf9dd26 100644
> > --- a/drivers/usb/gadget/f_fastboot.c
> > +++ b/drivers/usb/gadget/f_fastboot.c
> > @@ -427,7 +427,7 @@ static unsigned int rx_bytes_expected(struct usb_ep *ep)
> >   {
> > int rx_remain = fastboot_data_remaining();
> > unsigned int rem;
> > -   unsigned int maxpacket = usb_endpoint_maxp(ep->desc);
> > +   unsigned int maxpacket = ep->maxpacket;
> 
> Have you ever checked what's the value here of ep->maxpacket and
> usb_endpoint_maxp(ep->desc); in your failure case?

ep->maxpacket is 512, usb_endpoint_maxp(ep->desc) is 18277.

It might just be that there's a bug somewhere else that is covered by
the fact that we wouldn't get any transfer longer than 512 with
ep->maxpacket.

Is there a way to send USB data of arbitrary size without using
fastboot?

Maxime

Re: [PATCH 03/12] Revert "usb: gadget: fastboot: use correct max packet size"

[Maxime Ripard]

Hi Sean,

On Fri, Jun 25, 2021 at 09:47:11AM -0400, Sean Anderson wrote:
> On 6/25/21 9:05 AM, Maxime Ripard wrote:
> > This reverts commit 27c9141b1114fd5721437abbb1c694e45e765f19.
> 
> Why does this need to be reverted?

As I stated in the cover letter, I don't really know enough about USB to
know if we actually need to revert or not, but this introduces an issue
I reported here:

https://lore.kernel.org/u-boot/20210623101550.ezsc5dkaiqnqifvg@gilmour/T/#u

Maxime


signature.asc
Description: PGP signature

[PATCH 12/12] configs: chip pro: Fix NAND Controller name

[Maxime Ripard]

The controller name in Linux isn't sunxi_nand.0 but
1c03000.nand-controller, preventing us from passing the mtdparts
variable directly on the kernel command line.

Let's adjust our value to match what Linux expects.

Signed-off-by: Maxime Ripard 
---
 configs/CHIP_pro_defconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/configs/CHIP_pro_defconfig b/configs/CHIP_pro_defconfig
index d8b8f42d..b9e72ddefde5 100644
--- a/configs/CHIP_pro_defconfig
+++ b/configs/CHIP_pro_defconfig
@@ -9,8 +9,8 @@ CONFIG_DEFAULT_DEVICE_TREE="sun5i-gr8-chip-pro"
 CONFIG_SPL_I2C_SUPPORT=y
 # CONFIG_CMD_FLASH is not set
 CONFIG_CMD_MTDPARTS=y
-CONFIG_MTDIDS_DEFAULT="nand0=sunxi-nand.0"
-CONFIG_MTDPARTS_DEFAULT="mtdparts=sunxi-nand.0:256k(spl),256k(spl-backup),2m(uboot),2m(uboot-backup),-(UBI)"
+CONFIG_MTDIDS_DEFAULT="nand0=1c03000.nand-controller"
+CONFIG_MTDPARTS_DEFAULT="mtdparts=1c03000.nand-controller:256k(spl),256k(spl-backup),2m(uboot),2m(uboot-backup),-(UBI)"
 CONFIG_ENV_IS_IN_UBI=y
 CONFIG_ENV_UBI_PART="UBI"
 CONFIG_ENV_UBI_VOLUME="uboot-env"
-- 
2.31.1

[PATCH 11/12] configs: chip pro: fix the offsets

[Maxime Ripard]

The CHIP Pro MTD partition table sets the U-Boot offset at
512kB (0x8), and its backup at 2.5MB (0x28).

However, the default value for both of these on sunxi is 8MB (0x80),
which doesn't work in our case.

Fix this.

Signed-off-by: Maxime Ripard 
---
 configs/CHIP_pro_defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/configs/CHIP_pro_defconfig b/configs/CHIP_pro_defconfig
index c8e2e2ae2ca3..d8b8f42d 100644
--- a/configs/CHIP_pro_defconfig
+++ b/configs/CHIP_pro_defconfig
@@ -20,6 +20,8 @@ CONFIG_MTD_RAW_NAND=y
 CONFIG_SYS_NAND_BLOCK_SIZE=0x4
 CONFIG_SYS_NAND_PAGE_SIZE=0x1000
 CONFIG_SYS_NAND_OOBSIZE=0x100
+CONFIG_SYS_NAND_U_BOOT_OFFS=0x8
+CONFIG_SYS_NAND_U_BOOT_OFFS_REDUND=0x28
 CONFIG_AXP_ALDO3_VOLT=3300
 CONFIG_AXP_ALDO4_VOLT=3300
 CONFIG_CONS_INDEX=2
-- 
2.31.1

[PATCH 09/12] fastboot: nand: Fix return error code

[Maxime Ripard]

Both mtdparts_init() and find_dev_and_part() will return 0 on success
but 1 on failure.

Since the calling functions of fb_nand_lookup expects a negative error
code on failure, we can't just return the returned value of these
functions.

This fixes an issue with the logic that detects whether we support
fastboot slots or not by calling fb_nand_lookup and assuming that
anything >= 0 means the partition is there.

Signed-off-by: Maxime Ripard 
---
 drivers/fastboot/fb_nand.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/fastboot/fb_nand.c b/drivers/fastboot/fb_nand.c
index e07df33d3665..9d832fd8ca9a 100644
--- a/drivers/fastboot/fb_nand.c
+++ b/drivers/fastboot/fb_nand.c
@@ -44,14 +44,14 @@ static int fb_nand_lookup(const char *partname,
if (ret) {
pr_err("Cannot initialize MTD partitions\n");
fastboot_fail("cannot init mtdparts", response);
-   return ret;
+   return -EINVAL;
}
 
ret = find_dev_and_part(partname, , , part);
if (ret) {
pr_err("cannot find partition: '%s'\n", partname);
fastboot_fail("cannot find partition", response);
-   return ret;
+   return -ENODEV;
}
 
if (dev->id->type != MTD_DEV_TYPE_NAND) {
-- 
2.31.1

[PATCH 10/12] configs: chip pro: Disable UMS

[Maxime Ripard]

The CHIP Pro doesn't have any block storage device, so enabling UMS
makes little sense.

Signed-off-by: Maxime Ripard 
---
 configs/CHIP_pro_defconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/configs/CHIP_pro_defconfig b/configs/CHIP_pro_defconfig
index d2eddc7e9ff0..c8e2e2ae2ca3 100644
--- a/configs/CHIP_pro_defconfig
+++ b/configs/CHIP_pro_defconfig
@@ -26,4 +26,3 @@ CONFIG_CONS_INDEX=2
 CONFIG_USB_EHCI_HCD=y
 CONFIG_USB_OHCI_HCD=y
 CONFIG_USB_MUSB_GADGET=y
-CONFIG_USB_FUNCTION_MASS_STORAGE=y
-- 
2.31.1

[PATCH 08/12] clk: sunxi: Add missing clock compatible

[Maxime Ripard]

The NextThingCo GR8 is a derivative of the sun5i family, and thus should
be considered similar to the A10s and A13 as far as clocks go.

The compatible was missing from the clock driver so far, leading to all
the drivers depending on it being non-functional.

Signed-off-by: Maxime Ripard 
---
 drivers/clk/sunxi/clk_a10s.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/clk/sunxi/clk_a10s.c b/drivers/clk/sunxi/clk_a10s.c
index 184f61ab234c..99d8a8ab1b2e 100644
--- a/drivers/clk/sunxi/clk_a10s.c
+++ b/drivers/clk/sunxi/clk_a10s.c
@@ -59,6 +59,8 @@ static const struct udevice_id a10s_ccu_ids[] = {
  .data = (ulong)_ccu_desc },
{ .compatible = "allwinner,sun5i-a13-ccu",
  .data = (ulong)_ccu_desc },
+   { .compatible = "nextthing,gr8-ccu",
+ .data = (ulong)_ccu_desc },
{ }
 };
 
-- 
2.31.1

[PATCH 07/12] configs: chip pro: Disable video output

[Maxime Ripard]

The CHIP Pro doesn't have a video output, so it doesn't make sense to
have the video support enabled.

A side effect is that it prevents the messages from the HDMI controller
about a missing DDC controller to retrieve the EDID, which doesn't make
much sense since the GR8 doesn't have an HDMI controller to begin with.

Signed-off-by: Maxime Ripard 
---
 configs/CHIP_pro_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configs/CHIP_pro_defconfig b/configs/CHIP_pro_defconfig
index 7f10fd2b886e..d2eddc7e9ff0 100644
--- a/configs/CHIP_pro_defconfig
+++ b/configs/CHIP_pro_defconfig
@@ -4,6 +4,7 @@ CONFIG_SPL=y
 CONFIG_MACH_SUN5I=y
 CONFIG_DRAM_TIMINGS_DDR3_800E_1066G_1333J=y
 CONFIG_USB0_VBUS_PIN="PB10"
+# CONFIG_VIDEO_SUNXI is not set
 CONFIG_DEFAULT_DEVICE_TREE="sun5i-gr8-chip-pro"
 CONFIG_SPL_I2C_SUPPORT=y
 # CONFIG_CMD_FLASH is not set
-- 
2.31.1

[PATCH 06/12] fastboot: Add missing newlines

[Maxime Ripard]

Most of the error messages in the NAND fastboot support are missing a \n
at the end, add it where relevant.

Signed-off-by: Maxime Ripard 
---
 drivers/fastboot/fb_nand.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/fastboot/fb_nand.c b/drivers/fastboot/fb_nand.c
index eb8a36f29222..e07df33d3665 100644
--- a/drivers/fastboot/fb_nand.c
+++ b/drivers/fastboot/fb_nand.c
@@ -49,13 +49,13 @@ static int fb_nand_lookup(const char *partname,
 
ret = find_dev_and_part(partname, , , part);
if (ret) {
-   pr_err("cannot find partition: '%s'", partname);
+   pr_err("cannot find partition: '%s'\n", partname);
fastboot_fail("cannot find partition", response);
return ret;
}
 
if (dev->id->type != MTD_DEV_TYPE_NAND) {
-   pr_err("partition '%s' is not stored on a NAND device",
+   pr_err("partition '%s' is not stored on a NAND device\n",
  partname);
fastboot_fail("not a NAND device", response);
return -EINVAL;
@@ -179,7 +179,7 @@ void fastboot_nand_flash_write(const char *cmd, void 
*download_buffer,
 
ret = fb_nand_lookup(cmd, , , response);
if (ret) {
-   pr_err("invalid NAND device");
+   pr_err("invalid NAND device\n");
fastboot_fail("invalid NAND device", response);
return;
}
@@ -243,7 +243,7 @@ void fastboot_nand_erase(const char *cmd, char *response)
 
ret = fb_nand_lookup(cmd, , , response);
if (ret) {
-   pr_err("invalid NAND device");
+   pr_err("invalid NAND device\n");
fastboot_fail("invalid NAND device", response);
return;
}
@@ -254,7 +254,7 @@ void fastboot_nand_erase(const char *cmd, char *response)
 
ret = _fb_nand_erase(mtd, part);
if (ret) {
-   pr_err("failed erasing from device %s", mtd->name);
+   pr_err("failed erasing from device %s\n", mtd->name);
fastboot_fail("failed erasing from device", response);
return;
}
-- 
2.31.1

[PATCH 05/12] mtd: Allow for a longer mtdparts table

[Maxime Ripard]

20 characters is fairly short for a partition table, let's increase it a
bit.

Signed-off-by: Maxime Ripard 
---
 drivers/mtd/mtd_uboot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/mtd_uboot.c b/drivers/mtd/mtd_uboot.c
index c53ec657a34d..3b54d0ba6e95 100644
--- a/drivers/mtd/mtd_uboot.c
+++ b/drivers/mtd/mtd_uboot.c
@@ -15,7 +15,7 @@
 #include 
 #include 
 
-#define MTD_NAME_MAX_LEN 20
+#define MTD_NAME_MAX_LEN 64
 
 void board_mtdparts_default(const char **mtdids, const char **mtdparts);
 
-- 
2.31.1

[PATCH 04/12] Kconfig: Migrate SPL_PANIC_ON_RAW_IMAGE

[Maxime Ripard]

The SPL_PANIC_ON_RAW_IMAGE can be fairly useful to avoid the SPL getting
stuck without a message when the image retrieved isn't a valid image.

Let's convert it to Kconfig.

Signed-off-by: Maxime Ripard 
---
 README   | 10 --
 common/spl/Kconfig   | 14 ++
 scripts/config_whitelist.txt |  1 -
 3 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/README b/README
index ad13092bbb7a..7e4b6d3e723b 100644
--- a/README
+++ b/README
@@ -2145,16 +2145,6 @@ The following options need to be configured:
CONFIG_SPL_STACK
Adress of the start of the stack SPL will use
 
-   CONFIG_SPL_PANIC_ON_RAW_IMAGE
-   When defined, SPL will panic() if the image it has
-   loaded does not have a signature.
-   Defining this is useful when code which loads images
-   in SPL cannot guarantee that absolutely all read errors
-   will be caught.
-   An example is the LPC32XX MLC NAND driver, which will
-   consider that a completely unreadable NAND block is bad,
-   and thus should be skipped silently.
-
CONFIG_SPL_RELOC_STACK
Adress of the start of the stack SPL will use after
relocation.  If unspecified, this is equal to
diff --git a/common/spl/Kconfig b/common/spl/Kconfig
index fa80524cfb2c..e3541bd252d2 100644
--- a/common/spl/Kconfig
+++ b/common/spl/Kconfig
@@ -201,6 +201,20 @@ config SPL_LEGACY_IMAGE_SUPPORT
  is y. If this is not set, SPL will move on to other available
  boot media to find a suitable image.
 
+config SPL_PANIC_ON_RAW_IMAGE
+   bool "Panic if the image loaded doesn't have a header"
+   default y
+   help
+ When defined, SPL will panic() if the image it has loaded does not
+ have a signature.
+
+ Defining this is useful when code which loads images in SPL cannot
+ guarantee that absolutely all read errors will be caught.
+
+ An example is the LPC32XX MLC NAND driver, which will consider that a
+ completely unreadable NAND block is bad, and thus should be skipped
+ silently.
+
 config SPL_LEGACY_IMAGE_CRC_CHECK
bool "Check CRC of Legacy images"
depends on SPL_LEGACY_IMAGE_SUPPORT
diff --git a/scripts/config_whitelist.txt b/scripts/config_whitelist.txt
index 8f92b82719a2..90054af20fb7 100644
--- a/scripts/config_whitelist.txt
+++ b/scripts/config_whitelist.txt
@@ -1515,7 +1515,6 @@ CONFIG_SPL_NAND_RAW_ONLY
 CONFIG_SPL_NAND_SOFTECC
 CONFIG_SPL_NAND_WORKSPACE
 CONFIG_SPL_PAD_TO
-CONFIG_SPL_PANIC_ON_RAW_IMAGE
 CONFIG_SPL_PBL_PAD
 CONFIG_SPL_PPAACT_ADDR
 CONFIG_SPL_RELOC_MALLOC_ADDR
-- 
2.31.1

[PATCH 03/12] Revert "usb: gadget: fastboot: use correct max packet size"

[Maxime Ripard]

This reverts commit 27c9141b1114fd5721437abbb1c694e45e765f19.
---
 drivers/usb/gadget/f_fastboot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c
index 8ba55aab9f8f..1fcffaf9dd26 100644
--- a/drivers/usb/gadget/f_fastboot.c
+++ b/drivers/usb/gadget/f_fastboot.c
@@ -427,7 +427,7 @@ static unsigned int rx_bytes_expected(struct usb_ep *ep)
 {
int rx_remain = fastboot_data_remaining();
unsigned int rem;
-   unsigned int maxpacket = usb_endpoint_maxp(ep->desc);
+   unsigned int maxpacket = ep->maxpacket;
 
if (rx_remain <= 0)
return 0;
-- 
2.31.1

[PATCH 01/12] Revert "sunxi: spl: remove DMA related settings of the NAND controller"

[Maxime Ripard]

This reverts commit 136e32593335e031558a573158b6180fc80b551f.
---
 board/sunxi/board.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/board/sunxi/board.c b/board/sunxi/board.c
index 21651a1bfca4..80c222114f9a 100644
--- a/board/sunxi/board.c
+++ b/board/sunxi/board.c
@@ -395,6 +395,11 @@ static void nand_clock_setup(void)
 #if defined CONFIG_MACH_SUN6I || defined CONFIG_MACH_SUN8I || \
 defined CONFIG_MACH_SUN9I || defined CONFIG_MACH_SUN50I
setbits_le32(>ahb_reset0_cfg, (1 << AHB_GATE_OFFSET_NAND0));
+#endif
+#ifdef CONFIG_MACH_SUN9I
+   setbits_le32(>ahb_gate1, (1 << AHB_GATE_OFFSET_DMA));
+#else
+   setbits_le32(>ahb_gate0, (1 << AHB_GATE_OFFSET_DMA));
 #endif
setbits_le32(>nand0_clk_cfg, CCM_NAND_CTRL_ENABLE | AHB_DIV_1);
 }
-- 
2.31.1

[PATCH 02/12] Revert "spl: nand: sunxi: use PIO instead of DMA"

[Maxime Ripard]

This reverts commit 6ddbb1e936c78cdef1e7395039fa7020c5c75326.
---
 drivers/mtd/nand/raw/sunxi_nand_spl.c | 141 --
 1 file changed, 85 insertions(+), 56 deletions(-)

diff --git a/drivers/mtd/nand/raw/sunxi_nand_spl.c 
b/drivers/mtd/nand/raw/sunxi_nand_spl.c
index 85d8013b1a6b..c097a2b8b94d 100644
--- a/drivers/mtd/nand/raw/sunxi_nand_spl.c
+++ b/drivers/mtd/nand/raw/sunxi_nand_spl.c
@@ -8,10 +8,12 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
 #include 
+#include 
 
 /* registers */
 #define NFC_CTL0x
@@ -69,7 +71,6 @@
 #define NFC_SEND_CMD3  (1 << 28)
 #define NFC_SEND_CMD4  (1 << 29)
 #define NFC_RAW_CMD(0 << 30)
-#define NFC_ECC_CMD(1 << 30)
 #define NFC_PAGE_CMD   (2 << 30)
 
 #define NFC_ST_CMD_INT_FLAG(1 << 1)
@@ -84,6 +85,22 @@
 #define NFC_CMD_RNDOUT 0x05
 #define NFC_CMD_READSTART  0x30
 
+#define SUNXI_DMA_CFG_REG0  0x300
+#define SUNXI_DMA_SRC_START_ADDR_REG0   0x304
+#define SUNXI_DMA_DEST_START_ADDRR_REG0 0x308
+#define SUNXI_DMA_DDMA_BC_REG0  0x30C
+#define SUNXI_DMA_DDMA_PARA_REG00x318
+
+#define SUNXI_DMA_DDMA_CFG_REG_LOADING  (1 << 31)
+#define SUNXI_DMA_DDMA_CFG_REG_DMA_DEST_DATA_WIDTH_32 (2 << 25)
+#define SUNXI_DMA_DDMA_CFG_REG_DDMA_DST_DRQ_TYPE_DRAM (1 << 16)
+#define SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_DATA_WIDTH_32 (2 << 9)
+#define SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_ADDR_MODE_IO (1 << 5)
+#define SUNXI_DMA_DDMA_CFG_REG_DDMA_SRC_DRQ_TYPE_NFC (3 << 0)
+
+#define SUNXI_DMA_DDMA_PARA_REG_SRC_WAIT_CYC (0x0F << 0)
+#define SUNXI_DMA_DDMA_PARA_REG_SRC_BLK_SIZE (0x7F << 8)
+
 struct nfc_config {
int page_size;
int ecc_strength;
@@ -256,74 +273,86 @@ static const int ecc_bytes[] = {32, 46, 54, 60, 74, 88, 
102, 110, 116};
 static int nand_read_page(const struct nfc_config *conf, u32 offs,
  void *dest, int len)
 {
+   dma_addr_t dst = (dma_addr_t)dest;
int nsectors = len / conf->ecc_size;
u16 rand_seed = 0;
-   int oob_chunk_sz = ecc_bytes[conf->ecc_strength];
-   int page = offs / conf->page_size;
-   u32 ecc_st;
-   int i;
+   u32 val;
+   int page;
+
+   page = offs / conf->page_size;
 
if (offs % conf->page_size || len % conf->ecc_size ||
len > conf->page_size || len < 0)
return -EINVAL;
 
+   /* clear ecc status */
+   writel(0, SUNXI_NFC_BASE + NFC_ECC_ST);
+
/* Choose correct seed if randomized */
if (conf->randomize)
rand_seed = random_seed[page % conf->nseeds];
 
-   /* Retrieve data from SRAM (PIO) */
-   for (i = 0; i < nsectors; i++) {
-   int data_off = i * conf->ecc_size;
-   int oob_off = conf->page_size + (i * oob_chunk_sz);
-   u8 *data = dest + data_off;
-
-   /* Clear ECC status and restart ECC engine */
-   writel(0, SUNXI_NFC_BASE + NFC_ECC_ST);
-   writel((rand_seed << 16) | (conf->ecc_strength << 12) |
-  (conf->randomize ? NFC_ECC_RANDOM_EN : 0) |
-  (conf->ecc_size == 512 ? NFC_ECC_BLOCK_SIZE : 0) |
-  NFC_ECC_EN | NFC_ECC_EXCEPTION,
-  SUNXI_NFC_BASE + NFC_ECC_CTL);
-
-   /* Move the data in SRAM */
-   nand_change_column(data_off);
-   writel(conf->ecc_size, SUNXI_NFC_BASE + NFC_CNT);
-   nand_exec_cmd(NFC_DATA_TRANS);
-
-   /*
-* Let the ECC engine consume the ECC bytes and possibly correct
-* the data.
-*/
-   nand_change_column(oob_off);
-   nand_exec_cmd(NFC_DATA_TRANS | NFC_ECC_CMD);
-
-   /* Get the ECC status */
-   ecc_st = readl(SUNXI_NFC_BASE + NFC_ECC_ST);
-
-   /* ECC error detected. */
-   if (ecc_st & 0x)
-   return -EIO;
-
-   /*
-* Return 1 if the first chunk is empty (needed for
-* configuration detection).
-*/
-   if (!i && (ecc_st & 0x1))
-   return 1;
-
-   /* Retrieve the data from SRAM */
-   memcpy_fromio(data, SUNXI_NFC_BASE + NFC_RAM0_BASE,
- conf->ecc_size);
-
-   /* Stop the ECC engine */
-   writel(readl(SUNXI_NFC_BASE + NFC_ECC_CTL) & ~NFC_ECC_EN,
-  SUNXI_NFC_BASE + NFC_ECC_CTL);
-
-   if (data_off + conf->ecc_size >= len)
-   break;
+   writel((rand_seed << 16) | (conf->ecc_strength << 12) |
+   (conf->randomize ? NFC_ECC_RANDOM_EN : 0) |
+   (conf->ecc_size == 512 ? NFC_ECC_BLOCK_SIZE : 0) |
+   NFC_ECC_EN | NFC_ECC_PIPELINE | NFC_ECC_EXCEPTION,
+ 

[PATCH 00/12] Random Fixes for the CHIP Pro

[Maxime Ripard]

Hi,

These patches fix the NextThingCo CHIP Pro support in upstream U-Boot.

The three reverts are not supposed to be merged at this point: the first two
will break the NAND support on newer Allwinner SoCs, and it's not clear what
the fastboot one does exactly, so I'm a bit concerned about the side effects.

The following patches fixes a couple of things around the GR8, CHIP Pro, and
fastboot support in general.

Let me know what you think,
Maxime

Maxime Ripard (12):
  Revert "sunxi: spl: remove DMA related settings of the NAND
controller"
  Revert "spl: nand: sunxi: use PIO instead of DMA"
  Revert "usb: gadget: fastboot: use correct max packet size"
  Kconfig: Migrate SPL_PANIC_ON_RAW_IMAGE
  mtd: Allow for a longer mtdparts table
  fastboot: Add missing newlines
  configs: chip pro: Disable video output
  clk: sunxi: Add missing clock compatible
  fastboot: nand: Fix return error code
  configs: chip pro: Disable UMS
  configs: chip pro: fix the offsets
  configs: chip pro: Fix NAND Controller name

 README|  10 --
 board/sunxi/board.c   |   5 +
 common/spl/Kconfig|  14 +++
 configs/CHIP_pro_defconfig|   8 +-
 drivers/clk/sunxi/clk_a10s.c  |   2 +
 drivers/fastboot/fb_nand.c|  14 +--
 drivers/mtd/mtd_uboot.c   |   2 +-
 drivers/mtd/nand/raw/sunxi_nand_spl.c | 141 --
 drivers/usb/gadget/f_fastboot.c   |   2 +-
 scripts/config_whitelist.txt  |   1 -
 10 files changed, 120 insertions(+), 79 deletions(-)

-- 
2.31.1

Re: [U-Boot] [PATCH v3 13/20] spl: nand: sunxi: use PIO instead of DMA

[Maxime Ripard]

Hi Miquel,

On Wed, Feb 28, 2018 at 08:51:55PM +0100, Miquel Raynal wrote:
> SPL support was first written to support only the earlier generations of
> Allwinner SoCs, and was only really enabled on the A13 / GR8. However,
> those old SoCs had a DMA engine that has been replaced since the A31 by
> another DMA controller that is no longer compatible.
> 
> Since the code directly uses that DMA controller, it cannot operate
> properly on the later SoCs, while the NAND controller has not changed.
> 
> There's two paths forward, the first one would have been to add support
> for that DMA controller too, the second to just remove the DMA usage
> entirely and rely on PIO.
> 
> The later has been chosen because CPU overload at this stage is not an
> issue and it makes the driver more generic, and easier to understand.
> 
> Signed-off-by: Miquel Raynal 
> Acked-by: Boris Brezillon 

I'm a bit late to the party, but this bricks the CHIP Pro too. While
U-Boot proper seems to be flashed properly (re-reading it from the NAND
after flashing brings up the same CRC than the original image), the SPL
will only read 0s.

The transfer does complete though, so maybe it's just the copy from the
SRAM to the main memory that doesn't work?

The offset looks correct though, so I'm not sure.

Maxime

Re: [PATCH V2 16/17] usb: gadget: fastboot: use correct max packet size

[Maxime Ripard]

Hi,

Adding Andre, Lukasz and Marek in Cc

On Mon, Jan 25, 2021 at 09:43:59PM +0800, peng.fan at nxp.com wrote:
> From: Li Jun 
> 
> Change to use wMaxPacketSize of current speed EP desc for request
> length wrap up.
> 
> Reviewed-by: Peter Chen 
> Signed-off-by: Li Jun 
> Signed-off-by: Peng Fan 
> ---
>  drivers/usb/gadget/f_fastboot.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c
> index c2abdd66a8..2ef75a1388 100644
> --- a/drivers/usb/gadget/f_fastboot.c
> +++ b/drivers/usb/gadget/f_fastboot.c
> @@ -427,7 +427,7 @@ static unsigned int rx_bytes_expected(struct usb_ep *ep)
>  {
>   int rx_remain = fastboot_data_remaining();
>   unsigned int rem;
> - unsigned int maxpacket = ep->maxpacket;
> + unsigned int maxpacket = usb_endpoint_maxp(ep->desc);

Unfortunately this introduces a regression on sunxi, where a fastboot
flash command will stall on the download part forever.

Interestingly, this occurs on the last packet of the download and only
if the packet size is a power of two, and over 512.

This can be easily tested using

dd if=/dev/random of=test-custom.bin bs=1 count=512
fastboot flash uboot test-custom.bin

However, 511, 513 (or any other size, really) will work just fine.

Maxime


signature.asc
Description: PGP signature

Re: [RFC PATCH 0/3] fastboot: sunxi: Determine MMC device at runtime

[Maxime Ripard]

On Mon, May 24, 2021 at 11:43:43AM -0400, Sean Anderson wrote:
> 
> 
> On 5/24/21 11:28 AM, Maxime Ripard wrote:
> > On Mon, May 24, 2021 at 10:37:31AM -0400, Sean Anderson wrote:
> >>
> >>
> >> On 5/23/21 8:36 PM, Andre Przywara wrote:
> >>> At the moment the fastboot code relies on the Kconfig variable
> >>> CONFIG_FASTBOOT_FLASH_MMC_DEV to point to the MMC device to use for the
> >>> flash command. This value needs to be the *U-Boot device number*, which
> >>> is picked by the U-Boot device model at runtime. This makes it quite
> >>> tricky and fragile to fix this variable at compile time, as other DT
> >>> nodes and aliases influence the enumeration process.
> >>>
> >>> To make this process more robust, allow the device number to be picked at
> >>> runtime, which sounds like a better fit to find this device number. Patch
> >>> 1/3 introduces a weak function for that purpose.
> >>> Patch 2/3 then implements this function for the Allwinner platform. The
> >>> code follows the idea behind the existing Kconfig defaults: Use the eMMC
> >>> device, if that exists, or the SD card otherwise. This patch is actually
> >>> not sunxi specific, so might be adopted by other platforms as well.
> >>> Patch 3/3 then drops the existing Kconfig defaults for sunxi, to clean
> >>> this up and remove the implicit assumption that the eMMC device is always
> >>> device 1 (as least for the fastboot code).
> >>>
> >>> I would be curious if others think this is the right way forward.
> >>> The fact that the U-Boot device numbers are determined at runtime
> >>> seems to conflict with the idea of a Kconfig variable in the first place,
> >>> hence this series. This brings us one step closer to the end goal of
> >>> removing the "eMMC is device 1" assumption.
> >>
> >> I would actually favor removing CONFIG_FASTBOOT_FLASH_MMC_DEV
> >> altogether, and just specifying the device explicitly in fastboot
> >> commands. If you need to dynamically change the device, you can create
> >> some aliases. E.g. you could have something like
> >>
> >> "fastboot_aliases=setenv fastboot_partition_alias_user ${mmcdev}.0:0"
> >>
> >> and then run this variable just before calling `fastboot 0` (or whatever
> >> your usb device is).
> >
> > If we're going that way, maybe we can just pass the interface on the
> > command line like dfu does?
> 
> That could work, but I don't think it's necessary. At the moment there
> are many different ways to specify partitions (KConfig, Aliases, "U-boot
> syntax", GPT partition labels). I would rather pare things down to the
> minimum necessary ways than add yet another bit of state to specify
> partitions.
> 
> > That way the new requirement would be very obvious instead of
> > introducing a new environment variable no one really expects?
> 
> I'm not sure what you mean here. This alias system has been in place for
> a while, and it's very convenient for mapping a stable name to some
> arbitrary device and partition.

I don't deny the fact that it can be convenient. What I was pointing out
is that it's been optional the whole time, that I've been using fastboot
for like 5 years and didn't realize that this feature was there, and
it's only implemented for mmc.

To make it required, without any warning, would be fairly confusing.

Maxime


signature.asc
Description: PGP signature

Re: [RFC PATCH 0/3] fastboot: sunxi: Determine MMC device at runtime

[Maxime Ripard]

On Mon, May 24, 2021 at 10:37:31AM -0400, Sean Anderson wrote:
> 
> 
> On 5/23/21 8:36 PM, Andre Przywara wrote:
> > At the moment the fastboot code relies on the Kconfig variable
> > CONFIG_FASTBOOT_FLASH_MMC_DEV to point to the MMC device to use for the
> > flash command. This value needs to be the *U-Boot device number*, which
> > is picked by the U-Boot device model at runtime. This makes it quite
> > tricky and fragile to fix this variable at compile time, as other DT
> > nodes and aliases influence the enumeration process.
> >
> > To make this process more robust, allow the device number to be picked at
> > runtime, which sounds like a better fit to find this device number. Patch
> > 1/3 introduces a weak function for that purpose.
> > Patch 2/3 then implements this function for the Allwinner platform. The
> > code follows the idea behind the existing Kconfig defaults: Use the eMMC
> > device, if that exists, or the SD card otherwise. This patch is actually
> > not sunxi specific, so might be adopted by other platforms as well.
> > Patch 3/3 then drops the existing Kconfig defaults for sunxi, to clean
> > this up and remove the implicit assumption that the eMMC device is always
> > device 1 (as least for the fastboot code).
> >
> > I would be curious if others think this is the right way forward.
> > The fact that the U-Boot device numbers are determined at runtime
> > seems to conflict with the idea of a Kconfig variable in the first place,
> > hence this series. This brings us one step closer to the end goal of
> > removing the "eMMC is device 1" assumption.
> 
> I would actually favor removing CONFIG_FASTBOOT_FLASH_MMC_DEV
> altogether, and just specifying the device explicitly in fastboot
> commands. If you need to dynamically change the device, you can create
> some aliases. E.g. you could have something like
> 
> "fastboot_aliases=setenv fastboot_partition_alias_user ${mmcdev}.0:0"
> 
> and then run this variable just before calling `fastboot 0` (or whatever
> your usb device is).

If we're going that way, maybe we can just pass the interface on the
command line like dfu does? That way the new requirement would be very
obvious instead of introducing a new environment variable no one really
expects?

Maxime


signature.asc
Description: PGP signature

Re: [PATCH] sunxi: Select environment MMC based on boot device

[Maxime Ripard]

Hi,

On Mon, Apr 19, 2021 at 04:35:04PM +0100, Andre Przywara wrote:
> On Sun, 18 Apr 2021 22:16:21 -0500
> Samuel Holland  wrote:
> 
> (CC:ing Maxime)
> 
> Hi,
> 
> > Currently, the environment is always stored in eMMC if eMMC is enabled
> > in the config. This means images written to SD and eMMC will cross-
> > contaminate their environments unless the configuration is changed.
> > 
> > By dropping the device number from the environment location string and
> > implementing mmc_get_env_dev, we will always use the environment from
> > the boot device when booting from SD/eMMC.
> 
> Yeah, indeed, thanks for sending this. I will have a closer look and
> test tonight.
> 
> AFAIR Maxime had reservations towards this approach in the past, he
> mentioned that this fixed environment location had a use case?
> 
> Maxime, can you comment whether this is still needed? I think being
> able to just "dd" (or "mmc write" in U-Boot) the very same image to an
> eMMC is a very compelling use case, to install firmware from a bootable 
> SD card. I have patches to a similar effect, including loading the env
> from SPI when booting from SPI, and a U-Boot menu to make this user
> friendly, so would like to know whether we can push this forward.

I don't recall, really, and it's really a matter of policy. If you want
to change it please go ahead, you're in charge now ;)

The only thing that would be great to keep as is would be fastboot (or
at least until we have some way to select which device we want to
flash).

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v3 07/10] arm: sunxi: add support for DIP detection to CHIP board

[Maxime Ripard]

Hi,

On Thu, Apr 15, 2021 at 10:38:43AM +0200, Köry Maincent wrote:
> > >  arch/arm/mach-sunxi/Kconfig |   9 
> > >  board/sunxi/Makefile|   1 +
> > >  board/sunxi/chip.c  | 104 
> > >  3 files changed, 114 insertions(+)
> > >  create mode 100644 board/sunxi/chip.c
> > > 
> > > diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
> > > index 49ef217f08..2b6ba873ff 100644
> > > --- a/arch/arm/mach-sunxi/Kconfig
> > > +++ b/arch/arm/mach-sunxi/Kconfig
> > > @@ -1017,3 +1017,12 @@ config PINEPHONE_DT_SELECTION
> > > correct PinePhone hardware revision during boot.
> > >  
> > >  endif
> > > +
> > > +config CHIP_DIP_SCAN
> > > + bool "Enable DIPs detection for CHIP board"
> > > + select SUPPORT_EXTENSION_SCAN
> > > + select W1
> > > + select W1_GPIO
> > > + select W1_EEPROM
> > > + select W1_EEPROM_DS24XXX
> > > + imply CMD_EXTENSION
> > > diff --git a/board/sunxi/Makefile b/board/sunxi/Makefile
> > > index c4e13f8c38..d96b7897b6 100644
> > > --- a/board/sunxi/Makefile
> > > +++ b/board/sunxi/Makefile
> > > @@ -11,3 +11,4 @@ obj-$(CONFIG_SUN7I_GMAC)+= gmac.o
> > >  obj-$(CONFIG_MACH_SUN4I) += dram_sun4i_auto.o
> > >  obj-$(CONFIG_MACH_SUN5I) += dram_sun5i_auto.o
> > >  obj-$(CONFIG_MACH_SUN7I) += dram_sun5i_auto.o
> > > +obj-$(CONFIG_CHIP_DIP_SCAN)  += chip.o
> > > diff --git a/board/sunxi/chip.c b/board/sunxi/chip.c
> > > new file mode 100644
> > > index 00..9be2ede57b
> > > --- /dev/null
> > > +++ b/board/sunxi/chip.c
> > > @@ -0,0 +1,104 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * (C) Copyright 2021
> > > + * Köry Maincent, Bootlin, 
> > > + * Based on initial code from Maxime Ripard
> > > + */
> > > +
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +
> > > +#include 
> > > +
> > > +#include 
> > > +
> > > +#ifdef CONFIG_CMD_EXTENSION  
> > 
> > You depend on CMD_EXTENSION already, so that code won't even be compiled
> > if that configuration option is false
> 
> It is wrong, the build of this code depend on CHIP_DIP_SCAN and this config
> *imply* CMD_EXTENSION.

Indeed :)

> Therefore it can be build even if CMD_EXTENSION is false.

It's not really clear to me what would be the point though? Nothing in
that file will be compiled if CMD_EXTENSION, so it looks like (from a
functional point of view) CMD_EXTENSION doesn't bring anything over
SUPPORT_EXTENSION_SCAN

Why do we need both?

Maxime


signature.asc
Description: PGP signature

Re: Getting rid of falcon mode

[Maxime Ripard]

On Tue, Apr 13, 2021 at 10:13:50AM -0400, Tom Rini wrote:
> On Tue, Apr 13, 2021 at 09:08:01AM -0500, Alex G. wrote:
> > Hi Maxime,
> > 
> > On 4/13/21 3:56 AM, Maxime Ripard wrote:
> > > Hi,
> > > 
> > > On Mon, Apr 12, 2021 at 04:32:49PM -0500, Alex G. wrote:
> > > > ## Introduction
> > > > 
> > > > Today we use "falcon mode" to mean "boot linux straight from SPL". This
> > > > designation makes sense, since falcons "fly at high speed and change
> > > > direction rapidly" according to Wikipedia.
> > > > 
> > > > The way we implement falcon mode is to reserve two areas of storage:
> > > >* kernel area/partition
> > > >* dtb area/partition
> > > > By using some "special cases", and "spl export", SPL can more or less 
> > > > figure
> > > > out how to skip u-boot.
> > > > 
> > > > 
> > > > ## The plot twist
> > > > 
> > > > People familiar with FIT, will have recognized that the above is 
> > > > achievable
> > > > with a very basic FIT image. With some advantages:
> > > > 
> > > >  - No "special cases" in SPL code
> > > >  - Signed kernel images
> > > >  - Signed kernel devicetree
> > > >  - Devicetree overlays
> > > >  - Automatic selection of correct devicetree
> > > > 
> > > > 
> > > > ## The problems
> > > > 
> > > > The advantages of FIT are not obvious by looking at SPL code. A 
> > > > noticeable
> > > > amount of SPL code is hidden under #ifdef CONFIG_SPL_OS_BOOT, leading 
> > > > one to
> > > > believe that SPL_OS_BOOT is very important. It must be since it takes 
> > > > up so
> > > > much code.
> > > > 
> > > > Enabling falcon mode is not well documented, and requires a lot of 
> > > > trial and
> > > > error. I've had to define 7 macros, and one new function to get it 
> > > > working
> > > > on my board -- and vividly remember the grief. This is an antiquated 
> > > > way of
> > > > doing things, and completely ignores the u-boot devicetree -- we could 
> > > > just
> > > > as well have defined those seven values in the dtb.
> > > > 
> > > > SPL assumes that it must load u-boot, unless in instances under
> > > > CONFIG_SPL_OS_BOOT. This has cause me a huge amount of grief and 
> > > > confusion
> > > > over the past several months. I have no less than three patch series 
> > > > trying
> > > > to address shortfalls there. It's awful.
> > > > 
> > > > 
> > > > ## The proposal
> > > > 
> > > > I propose we drop falcon mode support for legacy images.
> > > > 
> > > >- Drop CONFIG_SPL_OS_BOOT. Support for this is implied by SPL_FIT
> > > >- Drop the "dtb area/partition". The dtb is derived from the FIT
> > > >- Drop "spl export"
> > > > 
> > > > 
> > > > How do we deal with devicetrees in the FIT then? The options are to use 
> > > > a
> > > > modified devicetree which has the desired "/chosen" node, or use DTB
> > > > overlays.
> > > > 
> > > > What are the reasons why we shouldn't go this route?
> > > 
> > > I can see at least two, that are mainly due to a FIT image being
> > > essentially a compiled device tree:
> > > 
> > >- Not all platforms have enough head-space in their SPL to have libfdt
> > >  in addition to what is already there.
> > 
> > Do we know which platforms fall in this category? We can investigate if it
> > might be possible to disable just enough of the legacy support to make room
> > for libfdt.
> 
> sunxi is both modern and small, so a good thing to benchmark.

Another issue with sunxi is that it implements PSCI in U-Boot, so you'll
need in the SPL as well if you want to do falcon boot. This is doable on
the later SoCs that have a bigger SRAM, but for that kind of test you'll
want to test on the A10 or A13

Maxime


signature.asc
Description: PGP signature

Re: Getting rid of falcon mode

[Maxime Ripard]

On Tue, Apr 13, 2021 at 09:08:01AM -0500, Alex G. wrote:
> On 4/13/21 3:56 AM, Maxime Ripard wrote:
> > On Mon, Apr 12, 2021 at 04:32:49PM -0500, Alex G. wrote:
> > > ## Introduction
> > > 
> > > Today we use "falcon mode" to mean "boot linux straight from SPL". This
> > > designation makes sense, since falcons "fly at high speed and change
> > > direction rapidly" according to Wikipedia.
> > > 
> > > The way we implement falcon mode is to reserve two areas of storage:
> > >* kernel area/partition
> > >* dtb area/partition
> > > By using some "special cases", and "spl export", SPL can more or less 
> > > figure
> > > out how to skip u-boot.
> > > 
> > > 
> > > ## The plot twist
> > > 
> > > People familiar with FIT, will have recognized that the above is 
> > > achievable
> > > with a very basic FIT image. With some advantages:
> > > 
> > >  - No "special cases" in SPL code
> > >  - Signed kernel images
> > >  - Signed kernel devicetree
> > >  - Devicetree overlays
> > >  - Automatic selection of correct devicetree
> > > 
> > > 
> > > ## The problems
> > > 
> > > The advantages of FIT are not obvious by looking at SPL code. A noticeable
> > > amount of SPL code is hidden under #ifdef CONFIG_SPL_OS_BOOT, leading one 
> > > to
> > > believe that SPL_OS_BOOT is very important. It must be since it takes up 
> > > so
> > > much code.
> > > 
> > > Enabling falcon mode is not well documented, and requires a lot of trial 
> > > and
> > > error. I've had to define 7 macros, and one new function to get it working
> > > on my board -- and vividly remember the grief. This is an antiquated way 
> > > of
> > > doing things, and completely ignores the u-boot devicetree -- we could 
> > > just
> > > as well have defined those seven values in the dtb.
> > > 
> > > SPL assumes that it must load u-boot, unless in instances under
> > > CONFIG_SPL_OS_BOOT. This has cause me a huge amount of grief and confusion
> > > over the past several months. I have no less than three patch series 
> > > trying
> > > to address shortfalls there. It's awful.
> > > 
> > > 
> > > ## The proposal
> > > 
> > > I propose we drop falcon mode support for legacy images.
> > > 
> > >- Drop CONFIG_SPL_OS_BOOT. Support for this is implied by SPL_FIT
> > >- Drop the "dtb area/partition". The dtb is derived from the FIT
> > >- Drop "spl export"
> > > 
> > > 
> > > How do we deal with devicetrees in the FIT then? The options are to use a
> > > modified devicetree which has the desired "/chosen" node, or use DTB
> > > overlays.
> > > 
> > > What are the reasons why we shouldn't go this route?
> > 
> > I can see at least two, that are mainly due to a FIT image being
> > essentially a compiled device tree:
> > 
> >- Not all platforms have enough head-space in their SPL to have libfdt
> >  in addition to what is already there.
> 
> Do we know which platforms fall in this category? We can investigate if it
> might be possible to disable just enough of the legacy support to make room
> for libfdt.

The earliest Allwinner SoC (A10, A13) must be fairly close to their
limit. I haven't checked for a while but they had 13kB overall iirc.

> >- Parsing a DT is fairly slow too. Last time I checked booting a FIT
> >  image took around 150ms more than a legacy image on a Cortex-A7. If
> >  one is using the Falcon mode in the first place chances are it's to
> >  improve the boot-time, so this seems like a step backward.
> 
> I'm skeptical of the 150ms delta, as I also did heavy measurements on this a
> few months back. But maybe there's something that causes certain platforms
> to bog down, (caching issues ?).

I found my old notes back, and while the 150ms figure was correct, the
FIT parsing was only half that (the other one was actually patching the
DT to fixup whatever was needed).

80ms is still fairly expensive to just parse the image. I'm not sure
indeed whether sunxi has the MMU enabled in the SPL though. I think it
is but might very well be wrong.

> I've used grabserial (e.g. "grabserial -d /dev/ttyACM0 -b 200 -m"U-Boot
> SPL" -t") to time the boot. If you have similar logs, maybe there's
> something there that tells us why parsing the FIT bogs down.

Unfortunately this was part of my former job and don't have access to
them anymore :/

Maxime


signature.asc
Description: PGP signature

Re: Getting rid of falcon mode

[Maxime Ripard]

Hi,

On Mon, Apr 12, 2021 at 04:32:49PM -0500, Alex G. wrote:
> ## Introduction
> 
> Today we use "falcon mode" to mean "boot linux straight from SPL". This
> designation makes sense, since falcons "fly at high speed and change
> direction rapidly" according to Wikipedia.
> 
> The way we implement falcon mode is to reserve two areas of storage:
>   * kernel area/partition
>   * dtb area/partition
> By using some "special cases", and "spl export", SPL can more or less figure
> out how to skip u-boot.
> 
> 
> ## The plot twist
> 
> People familiar with FIT, will have recognized that the above is achievable
> with a very basic FIT image. With some advantages:
> 
> - No "special cases" in SPL code
> - Signed kernel images
> - Signed kernel devicetree
> - Devicetree overlays
> - Automatic selection of correct devicetree
> 
> 
> ## The problems
> 
> The advantages of FIT are not obvious by looking at SPL code. A noticeable
> amount of SPL code is hidden under #ifdef CONFIG_SPL_OS_BOOT, leading one to
> believe that SPL_OS_BOOT is very important. It must be since it takes up so
> much code.
> 
> Enabling falcon mode is not well documented, and requires a lot of trial and
> error. I've had to define 7 macros, and one new function to get it working
> on my board -- and vividly remember the grief. This is an antiquated way of
> doing things, and completely ignores the u-boot devicetree -- we could just
> as well have defined those seven values in the dtb.
> 
> SPL assumes that it must load u-boot, unless in instances under
> CONFIG_SPL_OS_BOOT. This has cause me a huge amount of grief and confusion
> over the past several months. I have no less than three patch series trying
> to address shortfalls there. It's awful.
> 
> 
> ## The proposal
> 
> I propose we drop falcon mode support for legacy images.
> 
>   - Drop CONFIG_SPL_OS_BOOT. Support for this is implied by SPL_FIT
>   - Drop the "dtb area/partition". The dtb is derived from the FIT
>   - Drop "spl export"
> 
> 
> How do we deal with devicetrees in the FIT then? The options are to use a
> modified devicetree which has the desired "/chosen" node, or use DTB
> overlays.
> 
> What are the reasons why we shouldn't go this route?

I can see at least two, that are mainly due to a FIT image being
essentially a compiled device tree:

  - Not all platforms have enough head-space in their SPL to have libfdt
in addition to what is already there.

  - Parsing a DT is fairly slow too. Last time I checked booting a FIT
image took around 150ms more than a legacy image on a Cortex-A7. If
one is using the Falcon mode in the first place chances are it's to
improve the boot-time, so this seems like a step backward.

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v3 08/10] configs: CHIP: add support for DIP detect functionality

[Maxime Ripard]

On Wed, Mar 24, 2021 at 11:04:18AM +0100, Kory Maincent wrote:
> This commit enables using the extension board detection mechanism on
> CHIP boards
> 
> Signed-off-by: Kory Maincent 

Acked-by: Maxime Ripard 

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v3 07/10] arm: sunxi: add support for DIP detection to CHIP board

[Maxime Ripard]

Hi,

On Wed, Mar 24, 2021 at 11:04:17AM +0100, Kory Maincent wrote:
> Add the extension_board_scan specific function to scan the information
> of the EEPROM on one-wire and fill the extension struct.
> Add the Kconfig symbol to enable the needs to detect DIPs.
> 
> Signed-off-by: Kory Maincent 
> ---
> 
> Need the following patches series to fix a one-wire gpio issue.
> https://lists.denx.de/pipermail/u-boot/2021-February/440073.html
> 
> Change since v1:
> - Replace TARGET_CHIP options by CHIP_DIP_SCAN
> 
> Change since v2:
> - Fix typo
> - Place the Kconfig symbol in that patch
> 
>  arch/arm/mach-sunxi/Kconfig |   9 
>  board/sunxi/Makefile|   1 +
>  board/sunxi/chip.c  | 104 
>  3 files changed, 114 insertions(+)
>  create mode 100644 board/sunxi/chip.c
> 
> diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
> index 49ef217f08..2b6ba873ff 100644
> --- a/arch/arm/mach-sunxi/Kconfig
> +++ b/arch/arm/mach-sunxi/Kconfig
> @@ -1017,3 +1017,12 @@ config PINEPHONE_DT_SELECTION
> correct PinePhone hardware revision during boot.
>  
>  endif
> +
> +config CHIP_DIP_SCAN
> + bool "Enable DIPs detection for CHIP board"
> + select SUPPORT_EXTENSION_SCAN
> + select W1
> + select W1_GPIO
> + select W1_EEPROM
> + select W1_EEPROM_DS24XXX
> + imply CMD_EXTENSION
> diff --git a/board/sunxi/Makefile b/board/sunxi/Makefile
> index c4e13f8c38..d96b7897b6 100644
> --- a/board/sunxi/Makefile
> +++ b/board/sunxi/Makefile
> @@ -11,3 +11,4 @@ obj-$(CONFIG_SUN7I_GMAC)+= gmac.o
>  obj-$(CONFIG_MACH_SUN4I) += dram_sun4i_auto.o
>  obj-$(CONFIG_MACH_SUN5I) += dram_sun5i_auto.o
>  obj-$(CONFIG_MACH_SUN7I) += dram_sun5i_auto.o
> +obj-$(CONFIG_CHIP_DIP_SCAN)  += chip.o
> diff --git a/board/sunxi/chip.c b/board/sunxi/chip.c
> new file mode 100644
> index 00..9be2ede57b
> --- /dev/null
> +++ b/board/sunxi/chip.c
> @@ -0,0 +1,104 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * (C) Copyright 2021
> + * Köry Maincent, Bootlin, 
> + * Based on initial code from Maxime Ripard
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +#include 
> +
> +#ifdef CONFIG_CMD_EXTENSION

You depend on CMD_EXTENSION already, so that code won't even be compiled
if that configuration option is false

Once fixed,
Reviewed-by: Maxime Ripard 

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v3 06/10] w1: replace dt detection by automatic detection

[Maxime Ripard]

On Wed, Mar 24, 2021 at 11:04:16AM +0100, Kory Maincent wrote:
> This patch changes the functioning of the detection of w1 devices.
> The old way was a comparison between detected w1 and the ones described in
> the device tree. Now it will just look for the driver matching the family
> id of the w1 detected.
> 
> The patch is inspired from Maxime Ripart code.

It's Ripard, but otherwise

Reviewed-by: Maxime Ripard 

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v3 02/10] cmd: add support for a new "extension" command

[Maxime Ripard]

gt; + return CMD_RET_USAGE;
> +}
> +
> +U_BOOT_CMD(extension, 3, 1, do_extensionops,
> + "Extension board management sub system",
> + "scan - scan plugged extension(s) board(s)\n"
> + "extension list - lists available extension(s) board(s)\n"
> + "extension apply  - applies DT overlays 
> corresponding to extension boards\n"
> +);
> diff --git a/doc/usage/extension.rst b/doc/usage/extension.rst
> new file mode 100644
> index 00..2b88398b18
> --- /dev/null
> +++ b/doc/usage/extension.rst
> @@ -0,0 +1,111 @@
> +.. SPDX-License-Identifier: GPL-2.0+
> +.. Copyright 2021, Kory Maincent 
> +
> +U-Boot extension board usage (CONFIG_EXTENSION)
> +=======
> +
> +Synopsis
> +
> +
> +::
> +
> +extension scan
> +extension list
> +extension apply 
> +
> +Description
> +---
> +
> +The "extension" command proposes a generic U-Boot mechanism to detect
> +extension boards connected to the HW platform, and apply the appropriate
> +Device Tree overlays depending on the detected extension boards.
> +
> +The "extension" command comes with three sub-commands:
> +
> + - "extension scan" makes the generic code call the board-specific
> +   extension_board_scan() function to retrieve the list of detected
> +   extension boards.
> +
> + - "extension list" allows to list the detected extension boards.
> +
> + - "extension apply |all" allows to apply the Device Tree
> +   overlay(s) corresponding to one, or all, extension boards
> +
> +The latter requires two environment variables to exist:
> +
> + - extension_overlay_addr: the RAM address where to load the Device
> +   Tree overlays
> +
> + - extension_overlay_cmd: the U-Boot command to load one overlay.
> +   Indeed, the location and mechanism to load DT overlays is very setup
> +   specific.

I guess we should make it explicit that the extension_overlay_name
variable will be set and usable by that command

With that fixed,
Reviewed-by: Maxime Ripard 

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v3 01/10] fdt_support: move fdt_valid from cmd_fdt.c to fdt_support.c

[Maxime Ripard]

On Wed, Mar 24, 2021 at 11:04:11AM +0100, Kory Maincent wrote:
> Move the fdt_valid function to fdt_support.
> This changes allow to be able to test the validity of a devicetree in
> other c files.
> 
> Update code syntax.
> 
> Signed-off-by: Kory Maincent 
> Reviewed-by: Tom Rini 

Reviewed-by: Maxime Ripard 

Thanks!
Maxime


signature.asc
Description: PGP signature

Re: Help needed with uboot for CHIP

[Maxime Ripard]

On Tue, Mar 23, 2021 at 12:01:36AM +0530, Gunjan Gupta wrote:
> Ok...
> 
> What about supporting SLC. I dont have a Chip pro, but are you sure that
> the current logic works for that too?

The CHIP Pro has support for the NAND enabled, and if there's an issue
then it should be fixed indeed.

Maxime

Re: Help needed with uboot for CHIP

[Maxime Ripard]

Hi,

On Sun, Mar 21, 2021 at 08:04:08PM +0530, Gunjan Gupta wrote:
> I figured out the why SPL was not loading U-Boot from NAND.
> 
> Apparently the logic to use PIO instead of DMA that was introduced in
> 6ddbb1e936c78cdef1e7395039fa7020c5c75326
> 
> may
> be was not as generic as it was supposed to be. I tried adding hexdump in
> nand_read_page and found that on CHIP the logic was not reading and real
> data. Instead all of the bytes were set to 0s.
> 
> I have created a patch to provide an option to use the older version of
> nand_read_page function that was present just before the above mentioned
> commit.
> 
> With the attached patches and some config changes the Upstream U-Boot works
> fine on CHIP. I have tested the attached patches on CHIP with 8GB Hynix
> nand along with the following extra configurations.

An MLC NAND without proper support will work fine until it won't. I
would strongly discourage you to use it.

> CONFIG_MTD=y
> CONFIG_CMD_MTD=y
> CONFIG_CMD_MTDPARTS=y
> CONFIG_MTDIDS_DEFAULT="nand0=sunxi-nand.0"
> CONFIG_MTDPARTS_DEFAULT="mtdparts=sunxi-nand.0:4m(spl),4m(spl-backup),4m(uboot),4m(env),-(yaffs2)"
> CONFIG_MTD_RAW_NAND=y
> CONFIG_SYS_NAND_BLOCK_SIZE=0x40
> CONFIG_SYS_NAND_PAGE_SIZE=0x4000
> CONFIG_SYS_NAND_OOBSIZE=0x680
> CONFIG_ENV_IS_IN_NAND=y
> CONFIG_ENV_SIZE=0x40
> CONFIG_ENV_OFFSET=0xc0
> CONFIG_FASTBOOT_FLASH_NAND=y
> CONFIG_YAFFS2=y
> 
> @Andre will it be possible to get the attached patches merged into the
> U-Boot?

And we definitely shouldn't make it easy

Maxime

Re: [PATCH v2 07/12] w1-eeprom: remove sandbox eeprom driver

[Maxime Ripard]

Hi,

On Tue, Mar 02, 2021 at 10:58:08AM +0100, Kory Maincent wrote:
> Remove the sandbox w1 eeprom driver. The detection of w1 is
> now automatic and does not depend on the devicetree, therefore this driver
> can not be loaded anymore.
> 
> Signed-off-by: Kory Maincent 

It's not really great to remove some tests in general, and it should at
least be mentioned in your cover letter.

It's not really clear to me why you need to remove it. Sure, you won't
be able to rely on the bus discovering itself in the sandbox, but AFAIK
other discoverable buses have a way to register devices found on that
bus, through the DT or some API

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v2 10/12] sun5i: add support for DIP detection to CHIP board

[Maxime Ripard]

On Tue, Mar 02, 2021 at 05:14:44PM +0100, Köry Maincent wrote:
> Hello Maxime,
> 
> Thanks for your review.
> 
> On Tue, 2 Mar 2021 17:05:38 +0100
> Maxime Ripard  wrote:
>  
> > The split of that patch and the previous one is weird: you're adding a
> > Kconfig symbol doing close to nothing in patch 9, and you add the logic
> > behind it here
> > 
> > It would be more natural to have the Kconfig option and the code here,
> > and 
> 
> Ok I will merge them into one commit.

Sorry my sentence was incomplete, I meant to say

and enable the Kconfig symbol in another patch

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v2 10/12] sun5i: add support for DIP detection to CHIP board

[Maxime Ripard]

Hi,

On Tue, Mar 02, 2021 at 10:58:11AM +0100, Kory Maincent wrote:
> Add the extension_board_scan specific function to scan the information
> of the EEPROM on one-wire and fill the extension struct.
> 
> Signed-off-by: Kory Maincent 
> ---
>  arch/arm/mach-sunxi/Kconfig |   2 +
>  board/sunxi/Makefile|   1 +
>  board/sunxi/chip.c  | 104 
>  3 files changed, 107 insertions(+)
>  create mode 100644 board/sunxi/chip.c
> 
> diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
> index 4160d52501..2b6ba873ff 100644
> --- a/arch/arm/mach-sunxi/Kconfig
> +++ b/arch/arm/mach-sunxi/Kconfig
> @@ -1020,7 +1020,9 @@ endif
>  
>  config CHIP_DIP_SCAN
>   bool "Enable DIPs detection for CHIP board"
> + select SUPPORT_EXTENSION_SCAN
>   select W1
>   select W1_GPIO
>   select W1_EEPROM
>   select W1_EEPROM_DS24XXX
> + imply CMD_EXTENSION
> diff --git a/board/sunxi/Makefile b/board/sunxi/Makefile
> index c4e13f8c38..d96b7897b6 100644
> --- a/board/sunxi/Makefile
> +++ b/board/sunxi/Makefile
> @@ -11,3 +11,4 @@ obj-$(CONFIG_SUN7I_GMAC)+= gmac.o
>  obj-$(CONFIG_MACH_SUN4I) += dram_sun4i_auto.o
>  obj-$(CONFIG_MACH_SUN5I) += dram_sun5i_auto.o
>  obj-$(CONFIG_MACH_SUN7I) += dram_sun5i_auto.o
> +obj-$(CONFIG_CHIP_DIP_SCAN)  += chip.o

The split of that patch and the previous one is weird: you're adding a
Kconfig symbol doing close to nothing in patch 9, and you add the logic
behind it here

It would be more natural to have the Kconfig option and the code here,
and 

> diff --git a/board/sunxi/chip.c b/board/sunxi/chip.c
> new file mode 100644
> index 00..fc3d14e497
> --- /dev/null
> +++ b/board/sunxi/chip.c
> @@ -0,0 +1,104 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * (C) Copyright 2021
> + * Köry Maincent, Bootlin, 
> + * Based on initial code from Maxime Ripard
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +#include 
> +
> +#ifdef CONFIG_CMD_EXTENSION
> +
> +#define for_each_1w_device(b, d) \
> + for (device_find_first_child(b, d); *d; device_find_next_child(d))

The name is inconsistent with the rest of the 1-wire acronyms in the
file (1w vs w1)

Also, reusing macro arguments is fairly dangerous but since it's used
only once we don't really need that macro?

Maxime


signature.asc
Description: PGP signature

Re: [PATCH 2/4] pinephone_defconfig: reduce boot delay

[Maxime Ripard]

On Thu, Feb 25, 2021 at 05:02:40PM +, André Przywara wrote:
> On 20/02/2021 12:14, Nicolas Boulenguez wrote:
> > From: Arnaud Ferraris 
> > 
> > On a cellular phone, the vast majority of users can be expected to
> > have no serial console connected and prefer a short boot.
> 
> It's a bit tricky to break in with a delay of 0, but indeed most users
> won't care, so looks fine to me.
> 
> But it's missing a Signed-off-by: line.

I'm not sure we should start accepting custom boards changes like it's
done here, this will only lead to inconsistencies between boards in the
long run.

And a defconfig is really easy to change anyway, even when it's
integrated in build systems

Maxime


signature.asc
Description: PGP signature

Re: [PATCH] sunxi: board: Splitting CHIP defconfig for nand.

[Maxime Ripard]

On Fri, Feb 19, 2021 at 06:00:13PM +, André Przywara wrote:
> On 24/01/2021 16:19, Alexandre GRIVEAUX wrote:
> 
> Hi Alexandre,
> 
> (CCing: Kory, plus using Maxime's and Boris' newer emails)
> 
> > This patch split CHIP defconfig to add nand 4G and 8G support.
> > 
> > Some CONFIG was put at the end of defconfig to ease comparison between:
> > - CHIP Pro
> > - CHIP with Toshiba 4G
> > - CHIP with Hynix 8G
> > 
> > Witch are essentialy the same board with differents nand and memory.
> > 
> > Values was taken from now defunct compagny script "chip-update-firmware.sh"
> 
> Mmh, I am puzzled how this is supposed to work? I thought that the NAND
> in the non-Pro version was not supported (MLC?). So booting via USB was
> the only option?
> Has this changed?
> 
> Can someone confirm that this works?

The CHIP-Pro is indeed different, it's using a different SoC and NAND
(SLC) chip.

All the variants of the CHIP use an MLC NAND that isn't supported at all
by U-Boot.

I would discourage you from merging any MLC NAND related patch until
some form of MLC support is merged into U-Boot.

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v3] video: sunxi_display: Convert to DM_VIDEO

[Maxime Ripard]

On Sun, Feb 21, 2021 at 09:24:18AM -0700, Simon Glass wrote:
> > > > +static int sunxi_de_bind(struct udevice *dev)
> > > > +{
> > > > +   struct video_uc_plat *plat = dev_get_uclass_plat(dev);
> > > > +
> > > > +   plat->size = LCD_MAX_WIDTH * LCD_MAX_HEIGHT *
> > > > +(1 << LCD_MAX_LOG2_BPP) / 8;
> > >
> > > Should use enum video_log2_bpp here. Also see VNBYTES().
> >
> > LCD_MAX_LOG2_BPP is defined as VIDEO_BPP32 at the very top. Sure will
> > use VNBYTES.
> >
> > On a related topic: IIUC this is called several times, for a start once
> > before relocation, where it's supposed to give an upper bound?
> > Are subsequent calls then expected to be more precise? Our 32MB frame
> > buffer is *very* generous, for the usual FullHD display we just need
> > 8MB. But we would only know this in probe(), when we have learned the
> > output device and the video modes it supports.
> > So is there a way to restrict (and possibly also move) the framebuffer
> > in probe()?
> 
> I suppose you can, but that is not what is expected. You don't save
> memory for U-Boot (and it doesn't matter), but making it smaller so
> that linux uses less would be worthwhile. We just need to make sure
> this is documented in video.h and tested.
> 
> To be clear, you have my review thg, so these are things that can be done 
> later.

Seems Andre seems to be motivated to rework that driver, I guess we
could also rework how this is done.

Ideally, we should be describing the reserved buffer through a
reserved-memory node in the DT instead of carving out some memory for
Linux. That way, if we don't have simplefb support, or it's replaced by
something else eventually, we have a chance a claiming that memory back
at some point, instead of just ignoring it.

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v3] video: sunxi_display: Convert to DM_VIDEO

[Maxime Ripard]

Hi Andre,

On Sun, Feb 21, 2021 at 12:07:35AM +, Andre Przywara wrote:
> > >  enum sunxi_monitor {
> > > sunxi_monitor_none,
> > > sunxi_monitor_dvi,
> > > @@ -59,12 +68,11 @@ enum sunxi_monitor {
> > >  #define SUNXI_MONITOR_LAST sunxi_monitor_composite_pal_nc
> > >
> > >  struct sunxi_display {
> > > -   GraphicDevice graphic_device;
> > > enum sunxi_monitor monitor;
> > > unsigned int depth;
> > > unsigned int fb_addr;
> > > unsigned int fb_size;  
> > 
> > These last three are repeated from the uclass info. But fb_addr seems
> > to sometimes be different from the ucass frame buffer, in which case I
> > wonder how output actually works.
> > 
> > If you do actually need these, can you please document them here?
> 
> I think this might be mostly non-DM legacy, but for the composite
> overscan we tweak the framebuffer address. I haven't fully wrapped
> my mind around this, though, and ideally we can indeed lose those
> extra members.
> I will try to find some device with composite input to test it.

So the overscan will put the edges of any displayed image out of the
visible area of the screen, right? The amount of content non-visible due
to this isn't really standard and any display will behave differently
there.

One way to deal with this is to rescale the composited image to a
slightly smaller and center it in the middle of the screen.

We don't have that option though, so the code will instead render in a
slightly smaller resolution, centered in the framebuffer.

Note that composite isn't the only output that you can test this on:
HDMI TVs will also use overscan.


signature.asc
Description: PGP signature

Re: [PATCH v3] video: sunxi_display: Convert to DM_VIDEO

[Maxime Ripard]

On Fri, Feb 05, 2021 at 01:07:48AM +, Andre Przywara wrote:
> From: Jagan Teki 
> 
> DM_VIDEO migration deadline is already expired, but around
> 80 Allwinner boards are still using video in a legacy way.
> 
> = WARNING ==
> This board does not use CONFIG_DM_VIDEO Please update
> the board to use CONFIG_DM_VIDEO before the v2019.07 release.
> Failure to update by the deadline may result in board removal.
> See doc/driver-model/migration.rst for more info.
> 
> 
> Convert the legacy video driver over to the DM_VIDEO framework. This is
> a minimal conversion: it doesn't use the DT for finding its resources,
> nor does it use DM clocks or DM devices for the outputs (LCD, HDMI, CVBS).
> 
> Tested in Bananapi M1+ Plus 1920x1200 HDMI out. (Jagan)
> 
> Signed-off-by: Jagan Teki 
> [Andre: rebase and smaller fixes]
> Signed-off-by: Andre Przywara 

Acked-by: Maxime Ripard 

Thanks!
Maxime

Re: [PATCH] sunxi: display: introduce "sunxi-video" environment variable

[Maxime Ripard]

Hi,

On Mon, Jan 11, 2021 at 10:13:32AM +0100, Giulio Benetti wrote:
> From: Giulio Benetti 
> 
> At the moment you can specify lcd_mode only through hardcoded
> CONFIG_VIDEO_LCD_MODE but this way, if you have a board with multiple lcds
> compatible, you have to rebuild u-boot for every single lcd, so let's add
> sunxi-video environment variable to give the possibility to specify lcd
> settings at runtime. It's mandatory to "saveenv" after setting sunxi-video
> variable and then "reset" since sunxi_display driver apply those settings
> only on startup.
> 
> Signed-off-by: Giulio Benetti 

What's different from the video= parameter that already exists (and
should work like you suggest)?

Maxime


signature.asc
Description: PGP signature

Re: [PATCH] MAINTAINERS, git-mailrc: Update sunxi maintainers

[Maxime Ripard]

On Wed, Oct 28, 2020 at 11:37:43PM +, Andre Przywara wrote:
> Maxime mentioned that he feels not having the time to be an Allwinner
> maintainer anymore. Take over from him.
> 
> Maxime, many thanks for your great work in the past! I hope I can still
> relay the occasional technical question to you in the future.
> 
> Cc: Maxime Ripard 
> Signed-off-by: Andre Przywara 

Thanks for taking over, and I'll keep working on the linux-side, so I'm
not going anywhere if you need help :)

Acked-by: Maxime Ripard 

Maxime


signature.asc
Description: PGP signature

Re: [PATCH] arm: sunxi: disable obsolete VIDEO config

[Maxime Ripard]

On Mon, Oct 19, 2020 at 10:50:49AM -0400, Tom Rini wrote:
> On Mon, Oct 19, 2020 at 11:17:54AM +0200, Maxime Ripard wrote:
> > On Sun, Oct 18, 2020 at 09:18:15PM +0200, Anatolij Gustschin wrote:
> > > DM_VIDEO conversion deadline has passed, disable VIDEO config
> > > option by default. Boards should convert to DM_VIDEO if they
> > > need video console support.
> > > 
> > > Signed-off-by: Anatolij Gustschin 
> > > Cc: Jagan Teki 
> > > Cc: Maxime Ripard 
> > > ---
> > >  arch/arm/mach-sunxi/Kconfig | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
> > > index be0822bfb7..f672bb8b4e 100644
> > > --- a/arch/arm/mach-sunxi/Kconfig
> > > +++ b/arch/arm/mach-sunxi/Kconfig
> > > @@ -760,7 +760,7 @@ config VIDEO_SUNXI
> > >   depends on !MACH_SUN50I_H6
> > >   select VIDEO
> > >   imply VIDEO_DT_SIMPLEFB
> > > - default y
> > > + default n
> > 
> > default n is the default with Kconfig iirc
> > 
> > Otherwise, I can't say that I agree with the forced conversion strategy
> > to the DM in general. We're struggling to keep U-Boot working from one
> > release to the other, so adding pressure to convert drivers feels a bit
> > too much.
> > 
> > I can see where you're coming from though, so I guess that's a
> > reluctantly-acked-by ? :)
> 
> Can you please elaborate on your struggles?

I'm sure that the fact that we don't have a lot of time is part of that
reason really, but it seems that the forced (or at least strongly
encouraged) conversion to Kconfig and the device model created a lot of
reworks that in turn created some regressions.

Even though they are to be expected in every patch, it looks to me that
the forced conversion encouraged people to create patches that were not
properly tested on every platform. So we ended up with more patches, and
a higher probability of breakage.

For example, I've recently discovered that since at least 2020.01 we
have a regression in our musb setup, where we used to be able to just
use a USB gadget right away, but it looks like now we have to use usb
reset. I haven't had the time to look into why yet.

Some boards also have lost ethernet for a while too because it turns out
that the generic PHY reset bindings aren't supported by the the
designware net driver and when we changed for those bindings, and then
sync'd the DTS between linux and u-boot, then the PHY wasn't reset
properly anymore (I do have a patch for that one though).

It feels like almost every release is broken in some new way, and it's
hard to just keep up.

Ultimately, I think that this is mostly due to us not having enough time
to test and debug each release. If someone with some knowledge of
Allwinner SoCs was willing to step up and take over my seat, then I'd be
more than happy to help with the transition.

> The only thing right now keeping me from having some sunxi target in
> my HW CI loop is I didn't have anything that wasn't leaking in power
> via UART, even, seemingly. Thanks!

Yeah, the older one especially are pretty bad for that. Which one do you
have?

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v2 5/5] sunxi: add drivers and configs needed for LCD display

[Maxime Ripard]

On Sun, Oct 25, 2020 at 10:56:32AM +0100, Martin Cerveny wrote:
> 
> 
> On Mon, 19 Oct 2020, Maxime Ripard wrote:
> > On Fri, Oct 16, 2020 at 07:17:05PM +0200, Martin Cerveny wrote:
> > > Add PWM and dummy power regulator support.
> > > Modify phase of data signal for LCD display.
> > > 
> > > Signed-off-by: Martin Cerveny 
> > > ---
> > >  configs/LicheePi_Zero_defconfig | 4 
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/configs/LicheePi_Zero_defconfig 
> > > b/configs/LicheePi_Zero_defconfig
> > > index 04d7b64504..ba1c085ec3 100644
> > > --- a/configs/LicheePi_Zero_defconfig
> > > +++ b/configs/LicheePi_Zero_defconfig
> > > @@ -3,5 +3,9 @@ CONFIG_ARCH_SUNXI=y
> > >  CONFIG_SPL=y
> > >  CONFIG_MACH_SUN8I_V3S=y
> > >  CONFIG_DRAM_CLK=360
> > > +CONFIG_VIDEO_LCD_DCLK_PHASE=0
> > 
> > I'm not sure you answered on that one either, but you'd need some
> > display timings as well, right? If so, why do we need that change in the
> > first place if we're expected to change it anyway?
> > 
> 
> I done in config all needed setup to enable LCD
> (PHASE and PWM drivers).
> 
> Enabling LCD (with different display timimgs) depends now only on DTS/DTB.

Indeed, I missed that one.

With that being said, it doesn't really change the fact that you'd still
need some changes to the DT (and thus the code) to enable it.

And those changes wouldn't follow the DT bindings of the display
controller either, so there's no chance of them getting upstream at any
point in time.

So yeah, while your first patches should definitely go in, this one
doesn't seem that interesting

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v2 4/5] sunxi: video: v3s: Enable LCD support

[Maxime Ripard]

On Sun, Oct 25, 2020 at 10:46:04AM +0100, Martin Cerveny wrote:
> 
> On Mon, 19 Oct 2020, Maxime Ripard wrote:
> > On Fri, Oct 16, 2020 at 07:17:04PM +0200, Martin Cerveny wrote:
> > > Enable support for V3s LCD display with following changes:
> > > 
> > > V3s has 2x VI and 1x UI channels (use UI channel).
> > > V3s uses PLL3 (PLL_VIDEO) for both DE2 and TCON0 pixelclock.
> > > V3s does not support doubleclock for PLL3.
> > > V3s supports resolution upto 1024x1024.
> > > V3s does not support HDMI.
> > > 
> > > Signed-off-by: Martin Cerveny 
> > > ---
> > >  arch/arm/include/asm/arch-sunxi/clock_sun6i.h |  5 ++--
> > >  arch/arm/include/asm/arch-sunxi/gpio.h|  1 +
> > >  arch/arm/mach-sunxi/Kconfig   |  1 +
> > >  drivers/video/sunxi/lcdc.c|  5 ++--
> > >  drivers/video/sunxi/sunxi_de2.c   | 25 ---
> > >  drivers/video/sunxi/sunxi_dw_hdmi.c   |  2 ++
> > >  drivers/video/sunxi/sunxi_lcd.c   |  9 ++-
> > >  7 files changed, 40 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/arch/arm/include/asm/arch-sunxi/clock_sun6i.h 
> > > b/arch/arm/include/asm/arch-sunxi/clock_sun6i.h
> > > index ee387127f3..9efe05d103 100644
> > > --- a/arch/arm/include/asm/arch-sunxi/clock_sun6i.h
> > > +++ b/arch/arm/include/asm/arch-sunxi/clock_sun6i.h
> > > @@ -329,7 +329,7 @@ struct sunxi_ccm_reg {
> > >  #define AHB_GATE_OFFSET_DE   12
> > >  #define AHB_GATE_OFFSET_HDMI 11
> > >  #define AHB_GATE_OFFSET_TVE  9
> > > -#ifndef CONFIG_SUNXI_DE2
> > > +#if !defined(CONFIG_SUNXI_DE2) || defined(CONFIG_MACH_SUN8I_V3S)
> > >  #define AHB_GATE_OFFSET_LCD1 5
> > >  #define AHB_GATE_OFFSET_LCD0 4
> > >  #else
> > > @@ -476,7 +476,7 @@ struct sunxi_ccm_reg {
> > >  #define AHB_RESET_OFFSET_HDMI11
> > >  #define AHB_RESET_OFFSET_HDMI2   10
> > >  #define AHB_RESET_OFFSET_TVE 9
> > > -#ifndef CONFIG_SUNXI_DE2
> > > +#if !defined(CONFIG_SUNXI_DE2) || defined(CONFIG_MACH_SUN8I_V3S)
> > 
> > These two changes are confusing. The V3S has a DE2, so having that
> > condition is weird. I'd just add an elif there
> 
> The use of "CONFIG_SUNXI_DE2" is misleading.
> The platform connectivity (AHB) depends on platform not on DE2.

Feel free to fix it :)

> Names are also weird. Should be "TCON".

That one is less weird. TCON used to be called LCDC as well in the older
SoCs.

> AHB gating is described in platform docs and not in DE2 docs.
> 
> header:
> 
> ahb_gate1;/* 0x64 ahb module clock gating 1 */
> u32 ahb_reset1_cfg;   /* 0x2c4 AHB1 Reset 1 config */
> /* ahb_gate1 offsets */
> #define AHB_GATE_OFFSET_LCD0 ...
> /* ahb_reset1 offsets */
> #define AHB_RESET_OFFSET_LCD0
> 
> V3S datasheet:
> ("LCD0" (it is fixed used in drivers/video/sunxi/lcdc.c) is TCON (==TCON0)
> and does not have "LCD1" (==TCON1))
> 
> Offset: 0x0064 Register Name: BUS_CLK_GATING_REG1
> Bit: 4 TCON_GATING
> Offset: 0x02C4 Register Name: BUS_SOFT_RST_REG1
> Bit: 4 TCON_RST
> 
> > >  #define AHB_RESET_OFFSET_LCD15
> > >  #define AHB_RESET_OFFSET_LCD04
> > >  #else
> > > @@ -510,6 +510,7 @@ struct sunxi_ccm_reg {
> > >  #define CCM_DE2_CTRL_PLL_MASK(3 << 24)
> > >  #define CCM_DE2_CTRL_PLL6_2X (0 << 24)
> > >  #define CCM_DE2_CTRL_PLL10   (1 << 24)
> > > +#define CCM_DE2_CTRL_PLL3_V3S(0 << 24)
> > >  #define CCM_DE2_CTRL_GATE(0x1 << 31)
> > > 
> > >  /* CCU security switch, H3 only */
> > > diff --git a/arch/arm/include/asm/arch-sunxi/gpio.h 
> > > b/arch/arm/include/asm/arch-sunxi/gpio.h
> > > index d83dfdf605..9b580fbe26 100644
> > > --- a/arch/arm/include/asm/arch-sunxi/gpio.h
> > > +++ b/arch/arm/include/asm/arch-sunxi/gpio.h
> > > @@ -181,6 +181,7 @@ enum sunxi_gpio_number {
> > >  #define SUN5I_GPE_SDC2   3
> > >  #define SUN8I_GPE_TWI2   3
> > >  #define SUN50I_GPE_TWI2  3
> > > +#define SUN8I_V3S_GPE_LCD0   3
> > > 
> > >  #define SUNXI_GPF_SDC0   2
> > >  #define SUNXI_GPF_UART0  4
> > > diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
> > > index be0822b

Re: [PATCH 6/6] sunxi: add PineCube board

[Maxime Ripard]

On Mon, Oct 26, 2020 at 10:21:00PM +0800, Icenowy Zheng wrote:
> PineCube is an IP camera development kit released by Pine64.
> 
> It comes with the following compoents:
> 
> - A mainboard with Sochip S3 SoC, a 16MByte SPI Flash, AXP209 PMIC,
> a power-only microUSB connector, a USB Type-A connector, a 10/100Mbps
> Ethernet port and FPC connectors for camera and daughter board.
> - An OV5640-based camera module which is connected to the parallel CSI
> bus of the mainboard.
> - A daughterboard with several buttons, a SD slot, some IR LEDs, a
> microphone and a speaker connector.
> 
> As the device tree is synchronized in a previous commit, just add
> MAINTAINER item and a defconfig.
> 
> Signed-off-by: Icenowy Zheng 
> ---
>  board/sunxi/MAINTAINERS|  5 +
>  configs/pinecube_defconfig | 17 +
>  2 files changed, 22 insertions(+)
>  create mode 100644 configs/pinecube_defconfig
> 
> diff --git a/board/sunxi/MAINTAINERS b/board/sunxi/MAINTAINERS
> index 1180b86db3..5c53b2c878 100644
> --- a/board/sunxi/MAINTAINERS
> +++ b/board/sunxi/MAINTAINERS
> @@ -440,6 +440,11 @@ M:   Vasily Khoruzhick 
>  S:   Maintained
>  F:   configs/pinebook_defconfig
>  
> +PINECUBE BOARD:
> +M:   Icenowy Zheng 
> +S:   Maintained
> +F:   configs/pinecube_defconfig
> +
>  PINE64 BOARDS
>  M:   Andre Przywara 
>  S:   Maintained
> diff --git a/configs/pinecube_defconfig b/configs/pinecube_defconfig
> new file mode 100644
> index 00..107562ee49
> --- /dev/null
> +++ b/configs/pinecube_defconfig
> @@ -0,0 +1,17 @@
> +CONFIG_ARM=y
> +CONFIG_ARCH_SUNXI=y
> +CONFIG_SPL=y
> +CONFIG_MACH_SUN8I_V3S=y
> +CONFIG_SUNXI_DRAM_DDR3_1333=y
> +CONFIG_DRAM_CLK=504
> +CONFIG_DRAM_ODT_EN=y
> +CONFIG_I2C0_ENABLE=y
> +CONFIG_DEFAULT_DEVICE_TREE="sun8i-s3-pinecube"
> +CONFIG_SPL_I2C_SUPPORT=y
> +# CONFIG_NETDEVICES is not set
> +CONFIG_AXP209_POWER=y
> +CONFIG_AXP_DCDC2_VOLT=1250
> +CONFIG_AXP_DCDC3_VOLT=3300
> +CONFIG_AXP_ALDO3_VOLT_SLOPE_08=y
> +CONFIG_AXP_ALDO3_INRUSH_QUIRK=y

It would be worth mentioning in the commit log why you need those
options.

With that fixed, the whole series is
Acked-by: Maxime Ripard 

Thanks!
Maxime


signature.asc
Description: PGP signature

Re: [PATCH 4/8] sunxi: board: Add PinePhone DT selection logic

[Maxime Ripard]

On Wed, Oct 21, 2020 at 08:38:21PM -0500, Samuel Holland wrote:
> On 10/21/20 1:56 PM, Jagan Teki wrote:
> > On Thu, Sep 3, 2020 at 10:37 AM Samuel Holland  wrote:
> >>
> >> There are two different publicly-released revisions of the PinePhone
> >> hardware, versions 1.1 and 1.2; and they need different device trees.
> >> Since some GPIO pins were rerouted, we can use that to distinguish
> >> between them.
> >>
> >> Signed-off-by: Samuel Holland 
> >> ---
> >>  arch/arm/mach-sunxi/Kconfig |  7 +++
> >>  board/sunxi/board.c | 21 +
> >>  2 files changed, 28 insertions(+)
> >>
> >> diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
> >> index be0822bfb7d..8421f3b6854 100644
> >> --- a/arch/arm/mach-sunxi/Kconfig
> >> +++ b/arch/arm/mach-sunxi/Kconfig
> >> @@ -1010,4 +1010,11 @@ config PINE64_DT_SELECTION
> >>   option, the device tree selection code specific to Pine64 which
> >>   utilizes the DRAM size will be enabled.
> >>
> >> +config PINEPHONE_DT_SELECTION
> >> +   bool "Enable PinePhone device tree selection code"
> >> +   depends on MACH_SUN50I
> >> +   help
> >> + Enable this option to automatically select the device tree for 
> >> the
> >> + correct PinePhone hardware revision during boot.
> >> +
> >>  endif
> >> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> >> index fb0d5bf4743..3d64ed18664 100644
> >> --- a/board/sunxi/board.c
> >> +++ b/board/sunxi/board.c
> >> @@ -27,6 +27,7 @@
> >>  #include 
> >>  #include 
> >>  #include 
> >> +#include 
> >>  #include 
> >>  #include 
> >>  #include 
> >> @@ -920,6 +921,26 @@ int board_fit_config_name_match(const char *name)
> >> best_dt_name = "sun50i-a64-pine64";
> >> }
> >>  #endif
> >> +#ifdef CONFIG_PINEPHONE_DT_SELECTION
> > 
> > Look like these board detection CONFIG items are keep on increasing.
> > Better to have something like CONFIG_SUNXI_DT_SELECTION for all dt
> > selection code.
> 
> Are you sure? This is in SPL, where we are always running out of space. And a
> single SPL binary cannot work on both Pine A64 and PinePhone anyway, since 
> they
> have different DRAM types. I think the space savings is worth the cost of the
> extra config symbol (especially if more boards need special handling in the 
> future).

I agree, it will save some space and it's not like it's unmaintainable
at the moment.

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v5 0/9] sunxi: binman fixes and SCP firmware support

[Maxime Ripard]

On Wed, Oct 21, 2020 at 09:12:07PM -0500, Samuel Holland wrote:
> This is a rework of my previous patch series adding SCP firmware support
> for system suspend, on top of the binman rewrite of mksunxi_fit_atf.sh.
> Now that the generic binman updates have all been merged, this patchset
> is based on u-boot/master.
> 
> - Patches 1-2 apply to binman FIT support generally.
> - Patches 3-5 fix small issues in the new sunxi binman description.
> - Patch 6 is unchanged (improvements to FIT_IMAGE_TINY).
> - Patches 7-9 implement the SCP firmware support as in previous versions
>   of this series, but using binman instead of a shell script.
> 
Acked-by: Maxime Ripard 

Thanks!
Maxime


signature.asc
Description: PGP signature

Re: [PATCH v2 5/5] sunxi: add drivers and configs needed for LCD display

[Maxime Ripard]

On Fri, Oct 16, 2020 at 07:17:05PM +0200, Martin Cerveny wrote:
> Add PWM and dummy power regulator support.
> Modify phase of data signal for LCD display.
> 
> Signed-off-by: Martin Cerveny 
> ---
>  configs/LicheePi_Zero_defconfig | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/configs/LicheePi_Zero_defconfig b/configs/LicheePi_Zero_defconfig
> index 04d7b64504..ba1c085ec3 100644
> --- a/configs/LicheePi_Zero_defconfig
> +++ b/configs/LicheePi_Zero_defconfig
> @@ -3,5 +3,9 @@ CONFIG_ARCH_SUNXI=y
>  CONFIG_SPL=y
>  CONFIG_MACH_SUN8I_V3S=y
>  CONFIG_DRAM_CLK=360
> +CONFIG_VIDEO_LCD_DCLK_PHASE=0

I'm not sure you answered on that one either, but you'd need some
display timings as well, right? If so, why do we need that change in the
first place if we're expected to change it anyway?

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v2 4/5] sunxi: video: v3s: Enable LCD support

[Maxime Ripard]

On Fri, Oct 16, 2020 at 07:17:04PM +0200, Martin Cerveny wrote:
> Enable support for V3s LCD display with following changes:
> 
> V3s has 2x VI and 1x UI channels (use UI channel).
> V3s uses PLL3 (PLL_VIDEO) for both DE2 and TCON0 pixelclock.
> V3s does not support doubleclock for PLL3.
> V3s supports resolution upto 1024x1024.
> V3s does not support HDMI.
> 
> Signed-off-by: Martin Cerveny 
> ---
>  arch/arm/include/asm/arch-sunxi/clock_sun6i.h |  5 ++--
>  arch/arm/include/asm/arch-sunxi/gpio.h|  1 +
>  arch/arm/mach-sunxi/Kconfig   |  1 +
>  drivers/video/sunxi/lcdc.c|  5 ++--
>  drivers/video/sunxi/sunxi_de2.c   | 25 ---
>  drivers/video/sunxi/sunxi_dw_hdmi.c   |  2 ++
>  drivers/video/sunxi/sunxi_lcd.c   |  9 ++-
>  7 files changed, 40 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm/include/asm/arch-sunxi/clock_sun6i.h 
> b/arch/arm/include/asm/arch-sunxi/clock_sun6i.h
> index ee387127f3..9efe05d103 100644
> --- a/arch/arm/include/asm/arch-sunxi/clock_sun6i.h
> +++ b/arch/arm/include/asm/arch-sunxi/clock_sun6i.h
> @@ -329,7 +329,7 @@ struct sunxi_ccm_reg {
>  #define AHB_GATE_OFFSET_DE   12
>  #define AHB_GATE_OFFSET_HDMI 11
>  #define AHB_GATE_OFFSET_TVE  9
> -#ifndef CONFIG_SUNXI_DE2
> +#if !defined(CONFIG_SUNXI_DE2) || defined(CONFIG_MACH_SUN8I_V3S)
>  #define AHB_GATE_OFFSET_LCD1 5
>  #define AHB_GATE_OFFSET_LCD0 4
>  #else
> @@ -476,7 +476,7 @@ struct sunxi_ccm_reg {
>  #define AHB_RESET_OFFSET_HDMI11
>  #define AHB_RESET_OFFSET_HDMI2   10
>  #define AHB_RESET_OFFSET_TVE 9
> -#ifndef CONFIG_SUNXI_DE2
> +#if !defined(CONFIG_SUNXI_DE2) || defined(CONFIG_MACH_SUN8I_V3S)

These two changes are confusing. The V3S has a DE2, so having that
condition is weird. I'd just add an elif there

>  #define AHB_RESET_OFFSET_LCD15
>  #define AHB_RESET_OFFSET_LCD04
>  #else
> @@ -510,6 +510,7 @@ struct sunxi_ccm_reg {
>  #define CCM_DE2_CTRL_PLL_MASK(3 << 24)
>  #define CCM_DE2_CTRL_PLL6_2X (0 << 24)
>  #define CCM_DE2_CTRL_PLL10   (1 << 24)
> +#define CCM_DE2_CTRL_PLL3_V3S(0 << 24)
>  #define CCM_DE2_CTRL_GATE(0x1 << 31)
>  
>  /* CCU security switch, H3 only */
> diff --git a/arch/arm/include/asm/arch-sunxi/gpio.h 
> b/arch/arm/include/asm/arch-sunxi/gpio.h
> index d83dfdf605..9b580fbe26 100644
> --- a/arch/arm/include/asm/arch-sunxi/gpio.h
> +++ b/arch/arm/include/asm/arch-sunxi/gpio.h
> @@ -181,6 +181,7 @@ enum sunxi_gpio_number {
>  #define SUN5I_GPE_SDC2   3
>  #define SUN8I_GPE_TWI2   3
>  #define SUN50I_GPE_TWI2  3
> +#define SUN8I_V3S_GPE_LCD0   3
>  
>  #define SUNXI_GPF_SDC0   2
>  #define SUNXI_GPF_UART0  4
> diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
> index be0822bfb7..dc0ee2cdef 100644
> --- a/arch/arm/mach-sunxi/Kconfig
> +++ b/arch/arm/mach-sunxi/Kconfig
> @@ -258,6 +258,7 @@ config MACH_SUN8I_V3S
>   select CPU_V7_HAS_NONSEC
>   select CPU_V7_HAS_VIRT
>   select ARCH_SUPPORT_PSCI
> + select SUNXI_DE2
>   select SUNXI_GEN_SUN6I
>   select SUNXI_DRAM_DW
>   select SUNXI_DRAM_DW_16BIT
> diff --git a/drivers/video/sunxi/lcdc.c b/drivers/video/sunxi/lcdc.c
> index 73033c3b85..3d50f9d567 100644
> --- a/drivers/video/sunxi/lcdc.c
> +++ b/drivers/video/sunxi/lcdc.c
> @@ -244,7 +244,7 @@ void lcdc_pll_set(struct sunxi_ccm_reg *ccm, int tcon, 
> int dotclock,
>* not sync to higher frequencies.
>*/
>   for (m = min_m; m <= max_m; m++) {
> -#ifndef CONFIG_SUNXI_DE2
> +#if !defined(CONFIG_SUNXI_DE2) || defined(CONFIG_MACH_SUN8I_V3S)
>   n = (m * dotclock) / step;
>  
>   if ((n >= 9) && (n <= 127)) {
> @@ -262,7 +262,7 @@ void lcdc_pll_set(struct sunxi_ccm_reg *ccm, int tcon, 
> int dotclock,
>   if (!(m & 1))
>   continue;
>  #endif
> -
> +#ifndef CONFIG_MACH_SUN8I_V3S
>   /* No double clock on DE2 */
>   n = (m * dotclock) / (step * 2);
>   if ((n >= 9) && (n <= 127)) {
> @@ -275,6 +275,7 @@ void lcdc_pll_set(struct sunxi_ccm_reg *ccm, int tcon, 
> int dotclock,
>   best_double = 1;
>   }
>   }
> +#endif

I'm still not seeing any indication as to where you're getting this from

>   }
>  
>  #ifdef CONFIG_MACH_SUN6I
> diff --git a/drivers/video/sunxi/sunxi_de2.c b/drivers/video/sunxi/sunxi_de2.c
> index b657e163f0..49d41eb243 100644
> --- a/drivers/video/sunxi/sunxi_de2.c
> +++ b/drivers/video/sunxi/sunxi_de2.c
> @@ -26,12 +26,21 @@
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> +#ifdef CONFIG_MACH_SUN8I_V3S
> +enum {
> + /* Maximum LCD size we support */
> + LCD_MAX_WIDTH   = 1024,
> + LCD_MAX_HEIGHT  = 1024,
> + 

Re: [PATCH] arm: sunxi: disable obsolete VIDEO config

[Maxime Ripard]

On Sun, Oct 18, 2020 at 09:18:15PM +0200, Anatolij Gustschin wrote:
> DM_VIDEO conversion deadline has passed, disable VIDEO config
> option by default. Boards should convert to DM_VIDEO if they
> need video console support.
> 
> Signed-off-by: Anatolij Gustschin 
> Cc: Jagan Teki 
> Cc: Maxime Ripard 
> ---
>  arch/arm/mach-sunxi/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
> index be0822bfb7..f672bb8b4e 100644
> --- a/arch/arm/mach-sunxi/Kconfig
> +++ b/arch/arm/mach-sunxi/Kconfig
> @@ -760,7 +760,7 @@ config VIDEO_SUNXI
>   depends on !MACH_SUN50I_H6
>   select VIDEO
>   imply VIDEO_DT_SIMPLEFB
> - default y
> + default n

default n is the default with Kconfig iirc

Otherwise, I can't say that I agree with the forced conversion strategy
to the DM in general. We're struggling to keep U-Boot working from one
release to the other, so adding pressure to convert drivers feels a bit
too much.

I can see where you're coming from though, so I guess that's a
reluctantly-acked-by ? :)

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v2 2/5] ARM: dts: sun8i: v3s: Add simple-framebuffer

[Maxime Ripard]

On Fri, Oct 16, 2020 at 07:17:02PM +0200, Martin Cerveny wrote:
> Add support for "allwinner,simple-framebuffer" with "mixer0-lcd0" pipeline.
> 
> Signed-off-by: Martin Cerveny 

Acked-by: Maxime Ripard 

Maxime


signature.asc
Description: PGP signature

Re: [PATCH 4/5] arm: dts: v3s: Add support for simple-framebuffer with DE2/TCON

[Maxime Ripard]

On Wed, Sep 16, 2020 at 05:53:35PM +0200, Martin Cerveny wrote:
> On Wed, 16 Sep 2020, Maxime Ripard wrote:
> > On Wed, Sep 16, 2020 at 04:10:51PM +0200, Martin Cerveny wrote:
> > > Add support for DE2 and TCON connected LCD display.
> > > Add support for export as "allwinner,simple-framebuffer"
> > > with "mixer0-lcd0" pipeline.
> > > 
> > > Signed-off-by: Martin Cerveny 
> > 
> > You shouldn't do multiple things at once. If you need to, then split it 
> > into several patches.
> > 
> > Maxime
> 
> How to split - is three sufficient ?
> - pwm pins & registration
> - LCD pins & DE/TCOM registration
> - choosen framebuffer-lcd

Yep, that sounds reasonable.

For the last one though, we generally favor just sync'ing the Linux
device tree

Maxime

Re: [PATCH 5/5] sunxi: add drivers and configs needed for LCD display

[Maxime Ripard]

On Wed, Sep 16, 2020 at 05:44:58PM +0200, Martin Cerveny wrote:
> 
> 
> On Wed, 16 Sep 2020, Maxime Ripard wrote:
> > On Wed, Sep 16, 2020 at 04:10:52PM +0200, Martin Cerveny wrote:
> > > Add PWM and dummy power regulator support.
> > > Modify data timings for LCD displays.
> > > 
> > > Signed-off-by: Martin Cerveny 
> > > ---
> > >  configs/LicheePi_Zero_defconfig | 4 
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/configs/LicheePi_Zero_defconfig 
> > > b/configs/LicheePi_Zero_defconfig
> > > index 04d7b64504..ba1c085ec3 100644
> > > --- a/configs/LicheePi_Zero_defconfig
> > > +++ b/configs/LicheePi_Zero_defconfig
> > > @@ -3,5 +3,9 @@ CONFIG_ARCH_SUNXI=y
> > >  CONFIG_SPL=y
> > >  CONFIG_MACH_SUN8I_V3S=y
> > >  CONFIG_DRAM_CLK=360
> > > +CONFIG_VIDEO_LCD_DCLK_PHASE=0
> > >  CONFIG_DEFAULT_DEVICE_TREE="sun8i-v3s-licheepi-zero"
> > >  # CONFIG_NETDEVICES is not set
> > > +CONFIG_DM_REGULATOR=y
> > > +CONFIG_DM_PWM=y
> > > +CONFIG_PWM_SUNXI=y
> > 
> > The LicheePi Zero doesn't have any display by default. Also you're
> > mentionning in your commit log that you "modify data timings", but I
> > don't see any timings in there?
> 
> CONFIG_VIDEO_LCD_DCLK_PHASE is LCD data clock phase,
> standard "1" corrupts output on display,
> see
> - register "TCON0_IO_POL_REG" in V3S manual
> - arch/arm/mach-sunxi/Kconfig

So just call the phase of the data signal. Timings for LCD displays have
a very different meaning.

Maxime

Re: [PATCH 5/5] sunxi: add drivers and configs needed for LCD display

[Maxime Ripard]

On Wed, Sep 16, 2020 at 04:10:52PM +0200, Martin Cerveny wrote:
> Add PWM and dummy power regulator support.
> Modify data timings for LCD displays.
> 
> Signed-off-by: Martin Cerveny 
> ---
>  configs/LicheePi_Zero_defconfig | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/configs/LicheePi_Zero_defconfig b/configs/LicheePi_Zero_defconfig
> index 04d7b64504..ba1c085ec3 100644
> --- a/configs/LicheePi_Zero_defconfig
> +++ b/configs/LicheePi_Zero_defconfig
> @@ -3,5 +3,9 @@ CONFIG_ARCH_SUNXI=y
>  CONFIG_SPL=y
>  CONFIG_MACH_SUN8I_V3S=y
>  CONFIG_DRAM_CLK=360
> +CONFIG_VIDEO_LCD_DCLK_PHASE=0
>  CONFIG_DEFAULT_DEVICE_TREE="sun8i-v3s-licheepi-zero"
>  # CONFIG_NETDEVICES is not set
> +CONFIG_DM_REGULATOR=y
> +CONFIG_DM_PWM=y
> +CONFIG_PWM_SUNXI=y

The LicheePi Zero doesn't have any display by default. Also you're
mentionning in your commit log that you "modify data timings", but I
don't see any timings in there?

Maxime


signature.asc
Description: PGP signature

Re: [PATCH 4/5] arm: dts: v3s: Add support for simple-framebuffer with DE2/TCON

[Maxime Ripard]

On Wed, Sep 16, 2020 at 04:10:51PM +0200, Martin Cerveny wrote:
> Add support for DE2 and TCON connected LCD display.
> Add support for export as "allwinner,simple-framebuffer"
> with "mixer0-lcd0" pipeline.
> 
> Signed-off-by: Martin Cerveny 

You shouldn't do multiple things at once. If you need to, then split it into 
several patches.

Maxime


signature.asc
Description: PGP signature

Re: [PATCH 1/5] sunxi: video: No double clock on DE2

[Maxime Ripard]

On Wed, Sep 16, 2020 at 04:10:48PM +0200, Martin Cerveny wrote:
> Weird code or comment. This is variant is tested on V3s.
> 
> Signed-off-by: Martin Cerveny 

Generally speaking your commit logs are fairly concise, but it really
becomes an issue when you're allegedly fixing a bug.

There's a bunch of questions here that are completely up in the air:

  - What issue are you actually trying to fix, how can one reproduce it
  - You claim that there's no double clock on the DE2, according to
what?
  - DE2 is used on way more SoCs than just the V3s, did you check/test
on those SoCs as well?

In general, a good commit log should not explain what you're doing but
*why* you're doing it. The what can be quite easily figured out from the
patch content, the why can't today, and it will be even harder in a
year's time.

Maxime


signature.asc
Description: PGP signature