Re: [PATCH v1 2/2] riscv: dts: starfive: add the missing monitor core

[<Conor.Dooley@microchip.com>]

On 13/07/2022 15:26, Icenowy Zheng wrote:
> 
> 在 2022-07-11星期一的 19:43 +0100，Conor Dooley写道：
>> From: Conor Dooley <conor.dooley@microchip.com>
>>
>> The JH7100 has a 32 bit monitor core that is missing from the device
>> tree. Add it (and its cpu-map entry) to more accurately reflect the
>> actual topology of the SoC.
>>
>> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
>> ---
>>  arch/riscv/boot/dts/starfive/jh7100.dtsi | 21 +++++++++++++++++++++
>>  1 file changed, 21 insertions(+)
>>
>> diff --git a/arch/riscv/boot/dts/starfive/jh7100.dtsi
>> b/arch/riscv/boot/dts/starfive/jh7100.dtsi
>> index c617a61e26e2..92fce5b66d3d 100644
>> --- a/arch/riscv/boot/dts/starfive/jh7100.dtsi
>> +++ b/arch/riscv/boot/dts/starfive/jh7100.dtsi
>> @@ -67,6 +67,23 @@ cpu1_intc: interrupt-controller {
>>                         };
>>                 };
>>
>> +               E24: cpu@2 {
>> +                       compatible = "sifive,e24", "riscv";
>> +                       reg = <2>;
>> +                       device_type = "cpu";
>> +                       i-cache-block-size = <32>;
>> +                       i-cache-sets = <256>;
>> +                       i-cache-size = <16384>;
>> +                       riscv,isa = "rv32imafc";
>> +                       status = "disabled";
>> +
>> +                       cpu2_intc: interrupt-controller {
>> +                               compatible = "riscv,cpu-intc";
>> +                               interrupt-controller;
>> +                               #interrupt-cells = <1>;
>> +                       };
>> +               };
>> +
>>                 cpu-map {
>>                         cluster0 {
>>                                 core0 {
>> @@ -76,6 +93,10 @@ core0 {
>>                                 core1 {
>>                                         cpu = <&U74_1>;
>>                                 };
>> +
>> +                               core2 {
>> +                                       cpu = <&E24>;
>> +                               };
> 
> Sorry but I think this change makes the topology more inaccurate.
> 
> The E24 core is very independent, just another CPU core connected the
> same bus -- even no coherency (E24 takes AHB, which is not coherency-
> sensible). Even the TAP of it is independent with the U74 TAP.
> 
> And by default it does not boot any proper code (if a debugger is
> attached, it will discover that the E24 is in consistently fault at 0x0
> (mtvec is 0x0 and when fault it jumps to 0x0 and fault again), until
> its clock is just shutdown by Linux cleaning up unused clocks.)
> 
> Personally I think it should be implemented as a remoteproc instead.

Maybe I am missing something, but I don't quite get what the detail
of how we access this in code has to do with the devicetree?
It is added here in a disabled state, and will not be used by Linux.
The various SiFive SoCs & SiFive corecomplex users that have a hart
not capable of running Linux also have that hart documented in the
devicetree.
To me, what we are choosing to do with this hart does not really
matter very much, since this is a description of what the hardware
actually looks like.

Thanks,
Conor.