Re: [PATCH 0/9] riscv: ultrarisc: add DP1000 SoC DT and pinctrl support

[Igor Sakulin <is@nsa.green>]

Hi Jia,

I have a Milk-V Titan V1.2 (UltraRISC DP1000) and rebased this series onto
v7.2-rc5 to get the board running on a mainline kernel. It works, and the
board now boots mainline with the upstream devicetree - but not with the
series exactly as posted. Three things needed fixing, and two of them come
from the series having been split during merge. Details below, and I am
happy to give a Tested-by on a v2.

Context for anyone reading this later: patches 4/9 (pinctrl binding), 6/9
(pinctrl driver) and 9/9 (defconfig) were taken and are in v7.2-rc5, while
every DTS patch was left behind. So mainline can currently drive this SoC
but cannot describe any board that uses it.


1. The DTS speaks a pinctrl dialect the merged driver rejects
=============================================================

The driver was revised in review; the DTS was not updated to match, and it
is the DTS that was dropped. Two independent skews:

  a) "pins" is a string list in the DTS:

       i2c0_pins: i2c0-pins {
               pins = "PA12", "PA13";
               function = "func0";
       };

     but the merged binding declares it as an integer array
     (items: minimum/maximum) and the merged driver registers pins by
     number - UR_DP1000_PIN(12, "PA12", ...).

  b) "function" uses the legacy generic names "func0"/"func1", while the
     merged driver only knows the semantic ones: gpio, i2c, uart, spi,
     pwm, lpc, espi. This is the point Krzysztof and Linus both raised on
     4/9 and 6/9; the driver was cleaned up in response, the DTS was not.

Booting v7.2-rc5 with the unmodified DTB gives, in order:

  OF: size of pins in node /soc/pinmux@11081000/i2c0-pins is not a multiple of 4
  dw-apb-uart 20300000.serial: error -EINVAL: Error applying setting, reverse things back
  (the same for all four UARTs and all four I2Cs)
  Warning: unable to open an initial console.
  ...
  Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000100

The panic points nowhere near the cause. The chain is: string "pins" are
parsed as a u32 array, so every pinmux group fails, so all four UARTs fail
to apply their pinctrl, so there is no console device, so init cannot open
one and exits. Storage and networking are fine throughout - the machine
dies over a devicetree property format.

Worth noting for anyone bisecting this: fixing "pins" alone is not enough.
The "not a multiple of 4" messages go away and "Error applying setting"
remains, because func0 is still not a function the driver knows. Both
changes are required, which is why this needed hardware rather than review
to find - each fix on its own still produces an unbootable board.

The fix is mechanical. Pin numbering taken from the merged driver rather
than typed by hand: PA0..PA15 = 0-15, PB0..PB7 = 16-23, PC0..PC7 = 24-31,
PD0..PD7 = 32-39, LPC0..LPC12 = 40-52. Functions by group: i2c*-pins ->
"i2c", uart*-pins -> "uart", spi*-pins -> "spi", the rest are already
"gpio". So the example above becomes:

       i2c0_pins: i2c0-pins {
               pins = <12 13>;
               function = "i2c";
               bias-pull-up;
               drive-strength = <33>;
       };

Both dp1000-milkv-titan-pinctrl.dtsi and dp1000-rongda-m0-pinctrl.dtsi
need this. I can only test the Titan.


2. riscv,cbop-block-size is missing
===================================

This one is independent of the merge split - it is in the series as posted
and also in the vendor's own devicetree, so it looks like an omission
rather than something the rebase introduced.

The CPU nodes advertise "zicbop" in riscv,isa-extensions but never state
the prefetch block size, so the kernel refuses the extension, once per
hart:

  Zicbop detected in ISA string, disabling as no cbop-block-size found

Every other cache block size on this SoC reads 64 - riscv,cbom-block-size,
riscv,cboz-block-size, i-cache-block-size, d-cache-block-size and the
L2/L3/LLC cache-block-size properties - so 64 is not a guess. Adding

  riscv,cbop-block-size = <64>;

to all eight CPU nodes in dp1000.dtsi silences it and Zicbop then appears
in /proc/cpuinfo:

  rv64imafdch_zicbom_zicbop_zicboz_ziccrse_zicntr_...
                     ^^^^^^ absent before

Verified on hardware: message count 8 -> 0.


3. cpu@4..cpu@7 unit-addresses do not match reg
===============================================

In dp1000.dtsi the second cluster is:

  cpu4: cpu@4 { reg = <0x10>; ... }
  cpu5: cpu@5 { reg = <0x11>; ... }
  cpu6: cpu@6 { reg = <0x12>; ... }
  cpu7: cpu@7 { reg = <0x13>; ... }

The reg values are right - OpenSBI on this board reports
"Domain0 HARTs 0*,1*,2*,3*,16*,17*,18*,19*", so hart IDs really are 0-3
and 16-19 - but the unit-addresses should follow reg, i.e. cpu@10..cpu@13.
Harmless at runtime, but it is a dtc/dtbs_check complaint waiting to
happen.


What I would suggest
====================

This is your series and you are active upstream, so I would rather hand you
the fixes than post a competing v2. Happy to do either:

  - send you the three diffs off-list or as a reply here, or
  - post a v2 with you as author and the fixes folded in, if you prefer.

Either way, on a v2 that carries fixes 1 and 2 you can add:

Tested-by: Igor Sakulin <is@nsa.green>

Tested on Milk-V Titan V1.2 (UltraRISC DP1000, 8 harts, 62 GiB), Linux
v7.2-rc5 with the upstream DTB: boots to userspace, NVMe root, eth0 up,
all four i2c controllers present (all four failed before), RTC bound
(rtc-ds1307 on i2c2 as the DTS declares), and a Mellanox card on RC0
training at Gen3 x16 with 17 MSI-X vectors. Zero "Error applying setting",
zero "unable to open an initial console", no panic.

The other review comments on the v1 thread (shenrongda vendor prefix on
1/9, fixed-clock node naming on 5/9, the disabled gpio-poweroff/gpio-restart
nodes on 7/9) I have not touched - they are yours to resolve and none of
them affect booting.

Thanks,
Igor