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

Hi Jia,

I have a Milk-V Titan V1.2 (UltraRISC DP1000) and rebased this series ont=
o
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 com=
e
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, whil=
e
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
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D

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

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

       i2c0_pins: i2c0-pins {
               pins =3D "PA12", "PA13";
               function =3D "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 multip=
le of 4
  dw-apb-uart 20300000.serial: error -EINVAL: Error applying setting, rev=
erse 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=3D0x000001=
00

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 fai=
l
to apply their pinctrl, so there is no console device, so init cannot ope=
n
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 revie=
w
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 =3D 0-15, PB0..PB7 =3D 16-23, PC0..PC7 =3D =
24-31,
PD0..PD7 =3D 32-39, LPC0..LPC12 =3D 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 =3D <12 13>;
               function =3D "i2c";
               bias-pull-up;
               drive-strength =3D <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
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D

This one is independent of the merge split - it is in the series as poste=
d
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 =3D <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. [email protected]@7 unit-addresses do not match reg
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D

In dp1000.dtsi the second cluster is:

  cpu4: cpu@4 { reg =3D <0x10>; ... }
  cpu5: cpu@5 { reg =3D <0x11>; ... }
  cpu6: cpu@6 { reg =3D <0x12>; ... }
  cpu7: cpu@7 { reg =3D <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. [email protected]@13=
.
Harmless at runtime, but it is a dtc/dtbs_check complaint waiting to
happen.


What I would suggest
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D

This is your series and you are active upstream, so I would rather hand y=
ou
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 <[email protected]>

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-rest=
art
nodes on 7/9) I have not touched - they are yours to resolve and none of
them affect booting.

Thanks,
Igor