| Message ID | 20220711184325.1367393-3-mail@conchuod.ie (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add the JH7100's Monitor Core | expand |
On Mon, 11 Jul 2022 at 20:44, Conor Dooley <mail@conchuod.ie> wrote: > > 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> Reviewed-by: Emil Renner Berthing <emil.renner.berthing@canonical.com> Thanks! /Emil > --- > 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>; > + }; > }; > }; > }; > -- > 2.37.0 >
在 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. > }; > }; > };
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.
在 2022-07-13星期三的 14:55 +0000，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. The E24 is not in the core complex at all. It's just a dedicate CPU connected to another bus (well as I saw the document says the E24 bus is maximum 2G, I doubt whether it's the same bus with the U74 one). The U74 MC only allows S5 management cores to be part of it, not E24. BTW I don't think a core complex can have multiple TAPs for multiple harts, but E24 has its own TAP. > > Thanks, > Conor. >
On 13/07/2022 16:02, Icenowy Zheng wrote: > 在 2022-07-13星期三的 14:55 +0000，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. > > The E24 is not in the core complex at all. It's just a dedicate CPU > connected to another bus (well as I saw the document says the E24 bus > is maximum 2G, I doubt whether it's the same bus with the U74 one). > > The U74 MC only allows S5 management cores to be part of it, not E24. So is the correct topology more like: cpu-map { cluster0 { core0 { cpu = <&U74_0>; }; core1 { cpu = <&U74_1>; }; }; cluster1 { core0 { cpu = <&E24>; }; }; };
在 2022-07-13星期三的 15:09 +0000，Conor.Dooley@microchip.com写道： > > > On 13/07/2022 16:02, Icenowy Zheng wrote: > > 在 2022-07-13星期三的 14:55 +0000，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. > > > > The E24 is not in the core complex at all. It's just a dedicate CPU > > connected to another bus (well as I saw the document says the E24 > > bus > > is maximum 2G, I doubt whether it's the same bus with the U74 one). > > > > The U74 MC only allows S5 management cores to be part of it, not > > E24. > > So is the correct topology more like: > cpu-map { > cluster0 { > core0 { > cpu = <&U74_0>; > }; > core1 { > cpu = <&U74_1>; > }; > }; > cluster1 { > core0 { > cpu = <&E24>; > }; > }; > }; Considering E24 seems to see a total different bus connected to it, I don't think it even proper to add it to cpus node. And I don't think it has a hart id of 2, as your node describes. >
在 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"; Oh, by the way "sifive,e24" is not a documented compatible in the DT binding. If you really want to add it here, you need to add the compatible string to the DT binding first. > + 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>; > + }; > }; > }; > };
On 13/07/2022 16:15, 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"; > > Oh, by the way "sifive,e24" is not a documented compatible in the DT > binding. > > If you really want to add it here, you need to add the compatible > string to the DT binding first. Check patch 1/2. > >> + 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>; >> + }; >> }; >> }; >> }; > >
在 2022-07-13星期三的 15:16 +0000，Conor.Dooley@microchip.com写道： > On 13/07/2022 16:15, 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"; > > > > Oh, by the way "sifive,e24" is not a documented compatible in the > > DT > > binding. > > > > If you really want to add it here, you need to add the compatible > > string to the DT binding first. > > Check patch 1/2. Oh sorry I forgot this. Nevermind. > > > > > > + 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>; > > > + }; > > > }; > > > }; > > > }; > > > >
On 13/07/2022 16:12, Icenowy Zheng wrote: > 在 2022-07-13星期三的 15:09 +0000，Conor.Dooley@microchip.com写道： >> >> >> On 13/07/2022 16:02, Icenowy Zheng wrote: >>> 在 2022-07-13星期三的 14:55 +0000，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. >>> >>> The E24 is not in the core complex at all. It's just a dedicate CPU >>> connected to another bus (well as I saw the document says the E24 >>> bus >>> is maximum 2G, I doubt whether it's the same bus with the U74 one). >>> >>> The U74 MC only allows S5 management cores to be part of it, not >>> E24. >> >> So is the correct topology more like: >> cpu-map { >> cluster0 { >> core0 { >> cpu = <&U74_0>; >> }; >> core1 { >> cpu = <&U74_1>; >> }; >> }; >> cluster1 { >> core0 { >> cpu = <&E24>; >> }; >> }; >> }; > > Considering E24 seems to see a total different bus connected to it, I > don't think it even proper to add it to cpus node. Well, it is a CPU is it not? How one is supposed to document that a CPU is not attached to the same buses I do not know however. > > And I don't think it has a hart id of 2, as your node describes. Do you have any idea what it would be then?
在 2022-07-13星期三的 15:21 +0000，Conor.Dooley@microchip.com写道： > On 13/07/2022 16:12, Icenowy Zheng wrote: > > 在 2022-07-13星期三的 15:09 +0000，Conor.Dooley@microchip.com写道： > > > > > > > > > On 13/07/2022 16:02, Icenowy Zheng wrote: > > > > 在 2022-07-13星期三的 14:55 +0000，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. > > > > > > > > The E24 is not in the core complex at all. It's just a dedicate > > > > CPU > > > > connected to another bus (well as I saw the document says the > > > > E24 > > > > bus > > > > is maximum 2G, I doubt whether it's the same bus with the U74 > > > > one). > > > > > > > > The U74 MC only allows S5 management cores to be part of it, > > > > not > > > > E24. > > > > > > So is the correct topology more like: > > > cpu-map { > > > cluster0 { > > > core0 { > > > cpu = <&U74_0>; > > > }; > > > core1 { > > > cpu = <&U74_1>; > > > }; > > > }; > > > cluster1 { > > > core0 { > > > cpu = <&E24>; > > > }; > > > }; > > > }; > > > > Considering E24 seems to see a total different bus connected to it, > > I > > don't think it even proper to add it to cpus node. > > Well, it is a CPU is it not? How one is supposed to document that a > CPU is not attached to the same buses I do not know however. I don't think this kind of CPUs should exist in /cpus, they should just be seen as peripherals as the main system. The speciality of FU[57]40's management core is that they're in the same core complex with the CPU cores that run Linux, just cores with a different capability that we could not expand Linux to them. > > > > > And I don't think it has a hart id of 2, as your node describes. > > Do you have any idea what it would be then? As I asked one of my friend who has JTAG access to JH7110, the hart id is 0, the same with the first hart in U74-MC.
Rob/Krzk, Got a question about how to represent one of the cpu cores on this SoC at the end of the mail On 13/07/2022 16:26, Icenowy Zheng wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > 在 2022-07-13星期三的 15:21 +0000，Conor.Dooley@microchip.com写道： >> On 13/07/2022 16:12, Icenowy Zheng wrote: >>> 在 2022-07-13星期三的 15:09 +0000，Conor.Dooley@microchip.com写道： >>>> >>>> >>>> On 13/07/2022 16:02, Icenowy Zheng wrote: >>>>> 在 2022-07-13星期三的 14:55 +0000，Conor.Dooley@microchip.com写道： >>>>>> On 13/07/2022 15:26, Icenowy Zheng wrote: >>>>>>> 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. >>>>> >>>>> The E24 is not in the core complex at all. It's just a dedicate >>>>> CPU >>>>> connected to another bus (well as I saw the document says the >>>>> E24 >>>>> bus >>>>> is maximum 2G, I doubt whether it's the same bus with the U74 >>>>> one). >>>>> >>>>> The U74 MC only allows S5 management cores to be part of it, >>>>> not >>>>> E24. >>>> ---8<--- >>> >>> Considering E24 seems to see a total different bus connected to it, >>> I >>> don't think it even proper to add it to cpus node. >> >> Well, it is a CPU is it not? How one is supposed to document that a >> CPU is not attached to the same buses I do not know however. > > I don't think this kind of CPUs should exist in /cpus, they should just > be seen as peripherals as the main system. The speciality of FU[57]40's > management core is that they're in the same core complex with the CPU > cores that run Linux, just cores with a different capability that we > could not expand Linux to them. Maybe Rob or Krzysztof can shed some light on what would be the correct way to depict this cpu. > >> >>> >>> And I don't think it has a hart id of 2, as your node describes. >> >> Do you have any idea what it would be then? > > As I asked one of my friend who has JTAG access to JH7110, the hart id > is 0, the same with the first hart in U74-MC. Hmm, my understanding is that the regs property needs to be unique, so it'd have to stay as 2. Thanks, Conor.
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>; + }; }; }; };