Re: [PATCH v14 4/5] hw/riscv: virt: Add PMU DT node to the device tree
On 30/11/2022 08:13, Atish Kumar Patra wrote: > In Linux DT binding or dt schema repo ? I am a bit confused now as we > talked about both above :). I asked about a dt-binding and not a schema change, so the former ;)
Re: [PATCH v14 4/5] hw/riscv: virt: Add PMU DT node to the device tree
+CC Rob, which I probably should've done earlier, so
context all preserved
On 29/11/2022 09:42, Conor Dooley wrote:
> On 29/11/2022 09:27, Atish Kumar Patra wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
>> content is safe
>>
>> On Mon, Nov 28, 2022 at 11:32 PM wrote:
>>>
>>> On 29/11/2022 07:08, Andrew Jones wrote:
EXTERNAL EMAIL: Do not click links or open attachments unless you know the
content is safe
On Mon, Nov 28, 2022 at 09:10:03PM +, conor.doo...@microchip.com wrote:
> On 28/11/2022 20:41, Atish Kumar Patra wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>> the content is safe
>>
>> On Mon, Nov 28, 2022 at 12:38 PM wrote:
>>>
>>> On 28/11/2022 20:16, Atish Kumar Patra wrote:
On Thu, Nov 24, 2022 at 5:17 AM Conor Dooley
wrote:
>
> On Wed, Aug 24, 2022 at 03:17:00PM -0700, Atish Patra wrote:
>> Qemu virt machine can support few cache events and cycle/instret
>> counters.
>> It also supports counter overflow for these events.
>>
>> Add a DT node so that OpenSBI/Linux kernel is aware of the virt
>> machine
>> capabilities. There are some dummy nodes added for testing as well.
>
> Hey Atish!
>
> I was fiddling with dumping the virt machine dtb again today to check
> some dt-binding changes I was making for the isa string would play
> nicely with the virt machine & I noticed that this patch has
> introduced
> a new validation failure:
>
> ./build/qemu-system-riscv64 -nographic -machine virt,dumpdtb=qemu.dtb
>
> dt-validate -p
> ../linux/Documentation/devicetree/bindings/processed-schema.json
> qemu.dtb
> /home/conor/stuff/qemu/qemu.dtb: soc: pmu:
> {'riscv,event-to-mhpmcounters': [[1, 1, 524281, 2, 2, 524284, 65561,
> 65561, 524280, 65563, 65563, 524280, 65569, 65569, 524280, 0, 0, 0,
> 0, 0]], 'compatible': ['riscv,pmu']} should not be valid under
> {'type': 'object'}
> From schema:
> /home/conor/.local/lib/python3.10/site-packages/dtschema/schemas/simple-bus.yaml
>
> I assume this is the aforementioned "dummy" node & you have no
> intention
> of creating a binding for this?
>
It is a dummy node from Linux kernel perspective. OpenSbi use this
node to figure out the hpmcounter mappings.
>>>
>>> Aye, but should it not have a binding anyway, since they're not
>>> meant to be linux specific?
>>>
>> It is documented in OpenSBI.
>> https://github.com/riscv-software-src/opensbi/blob/master/docs/pmu_support.md
>>
>> Are you suggesting that any non-Linux specific DT nodes should be part
>> of Linux DT binding as well ?
>
> I thought the point was that they were *not* meant to be linux specific,
> just happening to be housed there.
>
I'm not sure if there's an official policy on where DT nodes should be
specified, but it looks like Samuel's opinion is that they should live
in the Linux kernel, whether they're used there or not [1].
[1] http://lists.infradead.org/pipermail/opensbi/2022-October/003522.html
>>>
>>> Yah, that was also my understanding. See also U-Boot moving to unify
>>> their custom bindings into the linux repo:
>>> https://lore.kernel.org/linux-devicetree/20220930001410.2802843-1-...@chromium.org/
>>>
>>
>> This adds the U-Boot specific DT properties to the dts schema itself,
>> not Linux kernel DT bindings.
>
> Yeah, sorry. I muddled things up a little there. My point was that they
> are trying to get to a stage where dt-validate and those tools work for
> them too. I'm not sure were I said "linux repo" rather than "dt-schema
> repo" when I double checked the file paths in the link before pasting it
> to make sure it was the dt-schema one.. I blame it being early.
>
>> I am not opposed to adding PMU DT bindings to Linux but there should
>> be a clear policy on this.
>> What about OpenSBI domain DT bindings ?
>> If every other DT based open source project starts adding their DT
>> binding to the Linux kernel, that may go downhill pretty soon.
Rob, perhaps you can be a source of clarity here :) My early morning
muddling didn't help things.
> Maybe I am misunderstanding, but I had thought the goal was to get to
> user-independent bindings. Rob and Krzysztof certainly labour the point
> that the bindings should not reflect how one operating system's drivers
> would like to see them & u-boot or FreeBSD using a property is grounds
> for it not being removed from the bindings in the linux tree.
>
> I'll go and actually ask Rob.
I did go & ask Rob, to which he said "I'll apply it even if no driver."
Do you want to whip up a binding, or shall I?
Re: [PATCH v14 4/5] hw/riscv: virt: Add PMU DT node to the device tree
On 29/11/2022 09:27, Atish Kumar Patra wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> On Mon, Nov 28, 2022 at 11:32 PM wrote:
>>
>> On 29/11/2022 07:08, Andrew Jones wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
>>> content is safe
>>>
>>> On Mon, Nov 28, 2022 at 09:10:03PM +, conor.doo...@microchip.com wrote:
On 28/11/2022 20:41, Atish Kumar Patra wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know
> the content is safe
>
> On Mon, Nov 28, 2022 at 12:38 PM wrote:
>>
>> On 28/11/2022 20:16, Atish Kumar Patra wrote:
>>> On Thu, Nov 24, 2022 at 5:17 AM Conor Dooley
>>> wrote:
On Wed, Aug 24, 2022 at 03:17:00PM -0700, Atish Patra wrote:
> Qemu virt machine can support few cache events and cycle/instret
> counters.
> It also supports counter overflow for these events.
>
> Add a DT node so that OpenSBI/Linux kernel is aware of the virt
> machine
> capabilities. There are some dummy nodes added for testing as well.
Hey Atish!
I was fiddling with dumping the virt machine dtb again today to check
some dt-binding changes I was making for the isa string would play
nicely with the virt machine & I noticed that this patch has introduced
a new validation failure:
./build/qemu-system-riscv64 -nographic -machine virt,dumpdtb=qemu.dtb
dt-validate -p
../linux/Documentation/devicetree/bindings/processed-schema.json
qemu.dtb
/home/conor/stuff/qemu/qemu.dtb: soc: pmu:
{'riscv,event-to-mhpmcounters': [[1, 1, 524281, 2, 2, 524284, 65561,
65561, 524280, 65563, 65563, 524280, 65569, 65569, 524280, 0, 0, 0, 0,
0]], 'compatible': ['riscv,pmu']} should not be valid under {'type':
'object'}
From schema:
/home/conor/.local/lib/python3.10/site-packages/dtschema/schemas/simple-bus.yaml
I assume this is the aforementioned "dummy" node & you have no
intention
of creating a binding for this?
>>>
>>> It is a dummy node from Linux kernel perspective. OpenSbi use this
>>> node to figure out the hpmcounter mappings.
>>
>> Aye, but should it not have a binding anyway, since they're not
>> meant to be linux specific?
>>
> It is documented in OpenSBI.
> https://github.com/riscv-software-src/opensbi/blob/master/docs/pmu_support.md
>
> Are you suggesting that any non-Linux specific DT nodes should be part
> of Linux DT binding as well ?
I thought the point was that they were *not* meant to be linux specific,
just happening to be housed there.
>>>
>>> I'm not sure if there's an official policy on where DT nodes should be
>>> specified, but it looks like Samuel's opinion is that they should live
>>> in the Linux kernel, whether they're used there or not [1].
>>>
>>> [1] http://lists.infradead.org/pipermail/opensbi/2022-October/003522.html
>>
>> Yah, that was also my understanding. See also U-Boot moving to unify
>> their custom bindings into the linux repo:
>> https://lore.kernel.org/linux-devicetree/20220930001410.2802843-1-...@chromium.org/
>>
>
> This adds the U-Boot specific DT properties to the dts schema itself,
> not Linux kernel DT bindings.
Yeah, sorry. I muddled things up a little there. My point was that they
are trying to get to a stage where dt-validate and those tools work for
them too. I'm not sure were I said "linux repo" rather than "dt-schema
repo" when I double checked the file paths in the link before pasting it
to make sure it was the dt-schema one.. I blame it being early.
> I am not opposed to adding PMU DT bindings to Linux but there should
> be a clear policy on this.
> What about OpenSBI domain DT bindings ?
> If every other DT based open source project starts adding their DT
> binding to the Linux kernel, that may go downhill pretty soon.
Maybe I am misunderstanding, but I had thought the goal was to get to
user-independent bindings. Rob and Krzysztof certainly labour the point
that the bindings should not reflect how one operating system's drivers
would like to see them & u-boot or FreeBSD using a property is grounds
for it not being removed from the bindings in the linux tree.
I'll go and actually ask Rob.
Re: [PATCH v14 4/5] hw/riscv: virt: Add PMU DT node to the device tree
On 29/11/2022 07:08, Andrew Jones wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> On Mon, Nov 28, 2022 at 09:10:03PM +, conor.doo...@microchip.com wrote:
>> On 28/11/2022 20:41, Atish Kumar Patra wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
>>> content is safe
>>>
>>> On Mon, Nov 28, 2022 at 12:38 PM wrote:
On 28/11/2022 20:16, Atish Kumar Patra wrote:
> On Thu, Nov 24, 2022 at 5:17 AM Conor Dooley
> wrote:
>>
>> On Wed, Aug 24, 2022 at 03:17:00PM -0700, Atish Patra wrote:
>>> Qemu virt machine can support few cache events and cycle/instret
>>> counters.
>>> It also supports counter overflow for these events.
>>>
>>> Add a DT node so that OpenSBI/Linux kernel is aware of the virt machine
>>> capabilities. There are some dummy nodes added for testing as well.
>>
>> Hey Atish!
>>
>> I was fiddling with dumping the virt machine dtb again today to check
>> some dt-binding changes I was making for the isa string would play
>> nicely with the virt machine & I noticed that this patch has introduced
>> a new validation failure:
>>
>> ./build/qemu-system-riscv64 -nographic -machine virt,dumpdtb=qemu.dtb
>>
>> dt-validate -p
>> ../linux/Documentation/devicetree/bindings/processed-schema.json qemu.dtb
>> /home/conor/stuff/qemu/qemu.dtb: soc: pmu:
>> {'riscv,event-to-mhpmcounters': [[1, 1, 524281, 2, 2, 524284, 65561,
>> 65561, 524280, 65563, 65563, 524280, 65569, 65569, 524280, 0, 0, 0, 0,
>> 0]], 'compatible': ['riscv,pmu']} should not be valid under {'type':
>> 'object'}
>> From schema:
>> /home/conor/.local/lib/python3.10/site-packages/dtschema/schemas/simple-bus.yaml
>>
>> I assume this is the aforementioned "dummy" node & you have no intention
>> of creating a binding for this?
>>
>
> It is a dummy node from Linux kernel perspective. OpenSbi use this
> node to figure out the hpmcounter mappings.
Aye, but should it not have a binding anyway, since they're not
meant to be linux specific?
>>> It is documented in OpenSBI.
>>> https://github.com/riscv-software-src/opensbi/blob/master/docs/pmu_support.md
>>>
>>> Are you suggesting that any non-Linux specific DT nodes should be part
>>> of Linux DT binding as well ?
>>
>> I thought the point was that they were *not* meant to be linux specific,
>> just happening to be housed there.
>>
>
> I'm not sure if there's an official policy on where DT nodes should be
> specified, but it looks like Samuel's opinion is that they should live
> in the Linux kernel, whether they're used there or not [1].
>
> [1] http://lists.infradead.org/pipermail/opensbi/2022-October/003522.html
Yah, that was also my understanding. See also U-Boot moving to unify
their custom bindings into the linux repo:
https://lore.kernel.org/linux-devicetree/20220930001410.2802843-1-...@chromium.org/
Re: [PATCH v14 4/5] hw/riscv: virt: Add PMU DT node to the device tree
On 28/11/2022 20:41, Atish Kumar Patra wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> On Mon, Nov 28, 2022 at 12:38 PM wrote:
>>
>> On 28/11/2022 20:16, Atish Kumar Patra wrote:
>>> On Thu, Nov 24, 2022 at 5:17 AM Conor Dooley
>>> wrote:
On Wed, Aug 24, 2022 at 03:17:00PM -0700, Atish Patra wrote:
> Qemu virt machine can support few cache events and cycle/instret counters.
> It also supports counter overflow for these events.
>
> Add a DT node so that OpenSBI/Linux kernel is aware of the virt machine
> capabilities. There are some dummy nodes added for testing as well.
Hey Atish!
I was fiddling with dumping the virt machine dtb again today to check
some dt-binding changes I was making for the isa string would play
nicely with the virt machine & I noticed that this patch has introduced
a new validation failure:
./build/qemu-system-riscv64 -nographic -machine virt,dumpdtb=qemu.dtb
dt-validate -p
../linux/Documentation/devicetree/bindings/processed-schema.json qemu.dtb
/home/conor/stuff/qemu/qemu.dtb: soc: pmu: {'riscv,event-to-mhpmcounters':
[[1, 1, 524281, 2, 2, 524284, 65561, 65561, 524280, 65563, 65563, 524280,
65569, 65569, 524280, 0, 0, 0, 0, 0]], 'compatible': ['riscv,pmu']} should
not be valid under {'type': 'object'}
From schema:
/home/conor/.local/lib/python3.10/site-packages/dtschema/schemas/simple-bus.yaml
I assume this is the aforementioned "dummy" node & you have no intention
of creating a binding for this?
>>>
>>> It is a dummy node from Linux kernel perspective. OpenSbi use this
>>> node to figure out the hpmcounter mappings.
>>
>> Aye, but should it not have a binding anyway, since they're not
>> meant to be linux specific?
>>
> It is documented in OpenSBI.
> https://github.com/riscv-software-src/opensbi/blob/master/docs/pmu_support.md
>
> Are you suggesting that any non-Linux specific DT nodes should be part
> of Linux DT binding as well ?
I thought the point was that they were *not* meant to be linux specific,
just happening to be housed there.
Re: [PATCH v14 4/5] hw/riscv: virt: Add PMU DT node to the device tree
On 28/11/2022 20:16, Atish Kumar Patra wrote:
> On Thu, Nov 24, 2022 at 5:17 AM Conor Dooley
> wrote:
>>
>> On Wed, Aug 24, 2022 at 03:17:00PM -0700, Atish Patra wrote:
>>> Qemu virt machine can support few cache events and cycle/instret counters.
>>> It also supports counter overflow for these events.
>>>
>>> Add a DT node so that OpenSBI/Linux kernel is aware of the virt machine
>>> capabilities. There are some dummy nodes added for testing as well.
>>
>> Hey Atish!
>>
>> I was fiddling with dumping the virt machine dtb again today to check
>> some dt-binding changes I was making for the isa string would play
>> nicely with the virt machine & I noticed that this patch has introduced
>> a new validation failure:
>>
>> ./build/qemu-system-riscv64 -nographic -machine virt,dumpdtb=qemu.dtb
>>
>> dt-validate -p
>> ../linux/Documentation/devicetree/bindings/processed-schema.json qemu.dtb
>> /home/conor/stuff/qemu/qemu.dtb: soc: pmu: {'riscv,event-to-mhpmcounters':
>> [[1, 1, 524281, 2, 2, 524284, 65561, 65561, 524280, 65563, 65563, 524280,
>> 65569, 65569, 524280, 0, 0, 0, 0, 0]], 'compatible': ['riscv,pmu']} should
>> not be valid under {'type': 'object'}
>> From schema:
>> /home/conor/.local/lib/python3.10/site-packages/dtschema/schemas/simple-bus.yaml
>>
>> I assume this is the aforementioned "dummy" node & you have no intention
>> of creating a binding for this?
>>
>
> It is a dummy node from Linux kernel perspective. OpenSbi use this
> node to figure out the hpmcounter mappings.
Aye, but should it not have a binding anyway, since they're not
meant to be linux specific?
>>> Acked-by: Alistair Francis
>>> Signed-off-by: Atish Patra
>>> Signed-off-by: Atish Patra
>>> ---
>>> hw/riscv/virt.c| 16 +
>>> target/riscv/pmu.c | 57 ++
>>> target/riscv/pmu.h | 1 +
>>> 3 files changed, 74 insertions(+)
>>>
>>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>>> index ff8c0df5cd47..befa9d2c26ac 100644
>>> --- a/hw/riscv/virt.c
>>> +++ b/hw/riscv/virt.c
>>> @@ -30,6 +30,7 @@
>>> #include "hw/char/serial.h"
>>> #include "target/riscv/cpu.h"
>>> #include "hw/core/sysbus-fdt.h"
>>> +#include "target/riscv/pmu.h"
>>> #include "hw/riscv/riscv_hart.h"
>>> #include "hw/riscv/virt.h"
>>> #include "hw/riscv/boot.h"
>>> @@ -708,6 +709,20 @@ static void create_fdt_socket_aplic(RISCVVirtState *s,
>>> aplic_phandles[socket] = aplic_s_phandle;
>>> }
>>>
>>> +static void create_fdt_pmu(RISCVVirtState *s)
>>> +{
>>> +char *pmu_name;
>>> +MachineState *mc = MACHINE(s);
>>> +RISCVCPU hart = s->soc[0].harts[0];
>>> +
>>> +pmu_name = g_strdup_printf("/soc/pmu");
>>> +qemu_fdt_add_subnode(mc->fdt, pmu_name);
>>> +qemu_fdt_setprop_string(mc->fdt, pmu_name, "compatible", "riscv,pmu");
>>> +riscv_pmu_generate_fdt_node(mc->fdt, hart.cfg.pmu_num, pmu_name);
>>> +
>>> +g_free(pmu_name);
>>> +}
>>> +
>>> static void create_fdt_sockets(RISCVVirtState *s, const MemMapEntry
>>> *memmap,
>>> bool is_32_bit, uint32_t *phandle,
>>> uint32_t *irq_mmio_phandle,
>>> @@ -1036,6 +1051,7 @@ static void create_fdt(RISCVVirtState *s, const
>>> MemMapEntry *memmap,
>>>
>>> create_fdt_flash(s, memmap);
>>> create_fdt_fw_cfg(s, memmap);
>>> +create_fdt_pmu(s);
>>>
>>> update_bootargs:
>>> if (cmdline && *cmdline) {
>>> diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
>>> index a5f504e53c88..b8e56d2b7b8e 100644
>>> --- a/target/riscv/pmu.c
>>> +++ b/target/riscv/pmu.c
>>> @@ -20,11 +20,68 @@
>>> #include "cpu.h"
>>> #include "pmu.h"
>>> #include "sysemu/cpu-timers.h"
>>> +#include "sysemu/device_tree.h"
>>>
>>> #define RISCV_TIMEBASE_FREQ 10 /* 1Ghz */
>>> #define MAKE_32BIT_MASK(shift, length) \
>>> (((uint32_t)(~0UL) >> (32 - (length))) << (shift))
>>>
>>> +/*
>>> + * To keep it simple, any event can be mapped to any programmable counters
>>> in
>>> + * QEMU. The generic cycle & instruction count events can also be monitored
>>> + * using programmable counters. In that case, mcycle & minstret must
>>> continue
>>> + * to provide the correct value as well. Heterogeneous PMU per hart is not
>>> + * supported yet. Thus, number of counters are same across all harts.
>>> + */
>>> +void riscv_pmu_generate_fdt_node(void *fdt, int num_ctrs, char *pmu_name)
>>> +{
>>> +uint32_t fdt_event_ctr_map[20] = {};
>>> +uint32_t cmask;
>>> +
>>> +/* All the programmable counters can map to any event */
>>> +cmask = MAKE_32BIT_MASK(3, num_ctrs);
>>> +
>>> + /*
>>> +* The event encoding is specified in the SBI specification
>>> +* Event idx is a 20bits wide number encoded as follows:
>>> +* event_idx[19:16] = type
>>> +* event_idx[15:0] = code
>>> +* The code field in cache events are encoded as follows:
>>> +* event_idx.code[15:3] = cache_id
>>> +* event_idx.code[2:1] = op_id
>>> +*
Re: [PATCH] hw/riscv: microchip_pfsoc: fix kernel panics due to missing peripherals
On 16/08/2022 01:40, Philippe Mathieu-Daudé wrote: > [You don't often get email from f4...@amsat.org. Learn why this is important > at https://aka.ms/LearnAboutSenderIdentification ] > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > Hi Conor, > > On 15/8/22 00:48, conor.doo...@microchip.com wrote: >> On 14/08/2022 23:08, Alistair Francis wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the >>> content is safe >>> >>> On Sat, Aug 13, 2022 at 11:51 PM Conor Dooley wrote: QEMU support for PolarFire SoC seems to be fairly out of date at this point. Running with a recent HSS, U-Boot etc doesn't work, partly due to the unimplemented cache controller that the HSS tries to read from (it needs to know the ways configuration now) and the rest seems to be down to 64 bit address DMA to the sd card (not 100% on that yet). There's some patches floating around internally that supposedly fixed things for QEMU v6.something but I could not replicate & they're fairly conflicty at this point. Plan is to clean them up, but no point sitting on this patch until then as I have no ETA for that at this point. >>> >>> Awesome! It is great to see Microchip supporting open source projects >> >> Better late than never ehh.. >> As I said, no ETA yet as I don't know just how far off the sd card stuff >> is, but it's in the todo pile. In the meantime, I'll keep an eye out here >> which I am ~certain we haven't been doing so far. I've added QEMU stuff >> to my build/test scripts now that I've got the direct kernel boot working >> for me so hopefully once things get fixed, they'll stay that way. > > Please Cc me and Cédric in your future posts regarding SD card, or open > a GitLab issue describing the problem. Willdo. Need to do some more digging first :) Thanks.
Re: [PATCH] hw/riscv: microchip_pfsoc: fix kernel panics due to missing peripherals
On 14/08/2022 23:08, Alistair Francis wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > On Sat, Aug 13, 2022 at 11:51 PM Conor Dooley wrote: >> QEMU support for PolarFire SoC seems to be fairly out of date at this >> point. Running with a recent HSS, U-Boot etc doesn't work, partly due >> to the unimplemented cache controller that the HSS tries to read from >> (it needs to know the ways configuration now) and the rest seems to be >> down to 64 bit address DMA to the sd card (not 100% on that yet). >> There's some patches floating around internally that supposedly fixed >> things for QEMU v6.something but I could not replicate & they're fairly >> conflicty at this point. Plan is to clean them up, but no point sitting >> on this patch until then as I have no ETA for that at this point. > > Awesome! It is great to see Microchip supporting open source projects Better late than never ehh.. As I said, no ETA yet as I don't know just how far off the sd card stuff is, but it's in the todo pile. In the meantime, I'll keep an eye out here which I am ~certain we haven't been doing so far. I've added QEMU stuff to my build/test scripts now that I've got the direct kernel boot working for me so hopefully once things get fixed, they'll stay that way. Thanks, Conor.
Re: [PATCH v2 3/4] hw/riscv: virt: fix syscon subnode paths
On 09/08/2022 00:10, Rob Herring wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> On Mon, Aug 8, 2022 at 4:10 PM wrote:
>>
>> On 08/08/2022 22:28, Jessica Clarke wrote:
>>> Moreover, what is the point of regmap in this
>>> case? Its existence suggests the point is for them to *not* be children
>>> of the syscon, otherwise you wouldn’t need an explicit phandle, you’d
>>> just look at the parent. Moving the nodes whilst keeping the property
>>> doesn’t make sense to me.
>>
>> That's how syscon bindings are constructed, makes it easier to follow
>> I suppose if they functions are children of the syscon node. Strictly
>> I think they don't need to be under the syscon itself, I think they can
>> also go at the top level - they just aren't valid under the /soc node
>> as it has been defined as a "simple-bus".
>>
>> It would appear that the original patch 0e404da007 ("riscv/virt: Add
>> syscon reboot and poweroff DT nodes") that added them put them at the
>> top level and it was in the refactor that they got moved to the soc bus.*
>> Maybe the solution would be to put them back at the top level?
>
> Perhaps.
>
> The other option is adding 'simple-mfd' to the 'test' node compatible.
> That would work for Linux. Not sure for FreeBSD.
Right, of course I was missing something in my understanding. The probe
flow on Linux came back to me on the bike this morning after reading this
and I felt like an idiot for missing that in the devicetrees I looked at!
@Jess, which does FreeBSD prefer? top level or add the extra compatible?
Thanks,
Conor.
Re: [PATCH v2 3/4] hw/riscv: virt: fix syscon subnode paths
On 08/08/2022 22:28, Jessica Clarke wrote:
> On 8 Aug 2022, at 22:06, Conor Dooley wrote:
>>
>> From: Conor Dooley
>>
>> The subnodes of the syscon have been added to the incorrect paths.
>> Rather than add them as subnodes, they were originally added to "/foo"
>> and a later patch moved them to "/soc/foo". Both are incorrect & they
>> should have been added as "/soc/test@###/foo" as "/soc/test" is the
>> syscon node. Fix both the reboot and poweroff subnodes to avoid errors
>> such as:
>>
>> /stuff/qemu/qemu.dtb: soc: poweroff: {'value': [[21845]], 'offset': [[0]],
>> 'regmap': [[4]], 'compatible': ['syscon-poweroff']} should not be valid
>> under {'type': 'object'}
>>From schema:
>> /home/conor/.local/lib/python3.9/site-packages/dtschema/schemas/simple-bus.yaml
>> /stuff/qemu/qemu.dtb: soc: reboot: {'value': [[30583]], 'offset': [[0]],
>> 'regmap': [[4]], 'compatible': ['syscon-reboot']} should not be valid under
>> {'type': 'object'}
>>From schema:
>> /home/conor/.local/lib/python3.9/site-packages/dtschema/schemas/simple-bus.yaml
>>
>> Reported-by: Rob Herring
>> Link:
>> https://lore.kernel.org/linux-riscv/20220803170552.ga2250266-r...@kernel.org/
>> Fixes: 18df0b4695 ("hw/riscv: virt: Allow creating multiple NUMA sockets")
>> Fixes: 0e404da007 ("riscv/virt: Add syscon reboot and poweroff DT nodes")
>> Reviewed-by: Alistair Francis
>> Signed-off-by: Conor Dooley
>
> This breaks FreeBSD’s driver (well, it just won’t probe/attach), which
> is written to handle syscon-poweroff/reboot existing as a child of a
> simplebus not a syscon.
I know next to nothing about FreeBSD unfortunately or how it handles
buses. My understanding of simple-bus was that it is supposed to
represent a bus with "things" mapped to addresses, relying on the "reg"
property. And then syscon is used when there is some multifunction
register region that controls multiple features of the hardware.
Since simple-bus defines a reg property and the function nodes do not
define one, I'd like to know how FreeBSD's driver handles that.
> Moreover, what is the point of regmap in this
> case? Its existence suggests the point is for them to *not* be children
> of the syscon, otherwise you wouldn’t need an explicit phandle, you’d
> just look at the parent. Moving the nodes whilst keeping the property
> doesn’t make sense to me.
That's how syscon bindings are constructed, makes it easier to follow
I suppose if they functions are children of the syscon node. Strictly
I think they don't need to be under the syscon itself, I think they can
also go at the top level - they just aren't valid under the /soc node
as it has been defined as a "simple-bus".
It would appear that the original patch 0e404da007 ("riscv/virt: Add
syscon reboot and poweroff DT nodes") that added them put them at the
top level and it was in the refactor that they got moved to the soc bus.*
Maybe the solution would be to put them back at the top level?
dt-validate will consider it valid, but what does the FreeBSD driver
think of that?
Thanks for your input Jess,
Conor.
* On reflection it looks like my fixes tags are not correct and that
0e404da007 was actually correct but the refactor broke things.
>
> Jess
>
>> ---
>> hw/riscv/virt.c | 6 --
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>> index 8b2978076e..a98b054545 100644
>> --- a/hw/riscv/virt.c
>> +++ b/hw/riscv/virt.c
>> @@ -896,7 +896,8 @@ static void create_fdt_reset(RISCVVirtState *s, const
>> MemMapEntry *memmap,
>> test_phandle = qemu_fdt_get_phandle(mc->fdt, name);
>> g_free(name);
>>
>> -name = g_strdup_printf("/soc/reboot");
>> +name = g_strdup_printf("/soc/test@%lx/reboot",
>> +(long)memmap[VIRT_TEST].base);
>> qemu_fdt_add_subnode(mc->fdt, name);
>> qemu_fdt_setprop_string(mc->fdt, name, "compatible", "syscon-reboot");
>> qemu_fdt_setprop_cell(mc->fdt, name, "regmap", test_phandle);
>> @@ -904,7 +905,8 @@ static void create_fdt_reset(RISCVVirtState *s, const
>> MemMapEntry *memmap,
>> qemu_fdt_setprop_cell(mc->fdt, name, "value", FINISHER_RESET);
>> g_free(name);
>>
>> -name = g_strdup_printf("/soc/poweroff");
>> +name = g_strdup_printf("/soc/test@%lx/poweroff",
>> +(long)memmap[VIRT_TEST].base);
>> qemu_fdt_add_subnode(mc->fdt, name);
>> qemu_fdt_setprop_string(mc->fdt, name, "compatible", "syscon-poweroff");
>> qemu_fdt_setprop_cell(mc->fdt, name, "regmap", test_phandle);
>> --
>> 2.37.1
>>
>>
>> ___
>> linux-riscv mailing list
>> linux-ri...@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
>
Re: [PATCH 1/5] target/riscv: Ignore the S and U letters when formatting ISA strings
On 08/08/2022 21:51, Alistair Francis wrote: > On Tue, Aug 9, 2022 at 2:14 AM wrote: >> I guess this patch can then be safely ignored :) >> Glad to have cleared this up as I was rather confused by what I saw. > > Great! Do you mind resending the series then with this patch dropped? > It just makes it easier for me to track and manage Aye, willdo.
Re: [PATCH 1/5] target/riscv: Ignore the S and U letters when formatting ISA strings
On 08/08/2022 16:03, Tsukasa OI wrote: > I agree with Alistair. **I** removed 'S' and 'U' from the ISA string > and it should be working in the latest development branch (I tested). Yeah, I saw what you as I looked at the commit log while trying to understand why there were invalid strings appearing when the code looked like it was impossible. Certainly I found it very very odd. I didn't just revive a 2 year old patch without taking a look at the code. > Besides, this function alone generates the ISA string for DTB and > there's no way such ISA strings with invalid 'S' and 'U' can be > generated. It's definitely a behavior of QEMU 7.0 or before. Hmm, it would seem that you're right - I have retested on a fresh clone. I did checkout v7.1.0-rc1 before running the first build that I saw the invalid string on as I'd been on some hacked up & fossilised version prior to that. Perhaps some build artifacts were not correctly removed, consider me quite confused! I do recall the configure script saying something about my build directory when I kicked it off, so it is likely down to that. Unfortunately my bash history is out of order so I will not be able to replicate the conditions, having many terminals open does have it's downsides. > Please. Please make sure that you are testing the right version of QEMU. Heh. Please, please give me some allowance for reasonably believing I was in fact on the latest qemu/master after checking it out and building! I guess this patch can then be safely ignored :) Glad to have cleared this up as I was rather confused by what I saw. Thanks Tsukasa/Alistair.
Re: [PATCH 1/5] target/riscv: Ignore the S and U letters when formatting ISA strings
On 07/08/2022 23:53, Alistair Francis wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> On Sat, Aug 6, 2022 at 2:08 AM Conor Dooley wrote:
>>
>> From: Palmer Dabbelt
>>
>> The ISA strings we're providing from QEMU aren't actually legal RISC-V
>> ISA strings, as both S and U cannot exist as single-letter extensions
>> and must instead be multi-letter strings. We're still using the ISA
>> strings inside QEMU to track the availiable extensions, so just strip
>> out the S and U extensions when formatting ISA strings.
>>
>> Signed-off-by: Palmer Dabbelt
>> [Conor: rebased on 7.1.0-rc1 & slightly tweaked the commit message]
>> Signed-off-by: Conor Dooley
>> ---
>> target/riscv/cpu.c | 18 +-
>> 1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index ac6f82ebd0..95fdc03b3d 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -1122,7 +1122,23 @@ char *riscv_isa_string(RISCVCPU *cpu)
>> char *p = isa_str + snprintf(isa_str, maxlen, "rv%d",
>> TARGET_LONG_BITS);
>> for (i = 0; i < sizeof(riscv_single_letter_exts) - 1; i++) {
>> if (cpu->env.misa_ext & RV(riscv_single_letter_exts[i])) {
>> -*p++ = qemu_tolower(riscv_single_letter_exts[i]);
>
> riscv_single_letter_exts doesn't contain S or U, is this patch still required?
Seemingly, yes. This is what ends up in the dtb:
/home/rob/riscv-virt.dtb: cpu@0: riscv,isa:0: 'rv64imafdcsuh' is not one of
['rv64imac', 'rv64imafdc']
From schema:
/home/rob/proj/git/linux-dt/Documentation/devicetree/bindings/riscv/cpus.yaml
With Palmer's patch applied, the dtb's isa string becomes
rv64imafdch_zicsr_zifencei_zba_zbb_zbc_zbs
while in n /proc/cpuinfo it is rv64imafdch
The short_isa_string flag (I think that's it's name) is not
set for the dtb creation. meant to note this under the ---
for this patch but obviously I forgot.
Thanks,
Conor.
>
> Alistair
>
>> +char lower = qemu_tolower(riscv_single_letter_exts[i]);
>> +switch (lower) {
>> +case 's':
>> +case 'u':
>> +/*
>> +* The 's' and 'u' letters shouldn't show up in ISA
>> strings as
>> +* they're not extensions, but they should show up in
>> MISA.
>> +* Since we use these letters interally as a pseudo ISA
>> string
>> +* to set MISA it's easier to just strip them out when
>> +* formatting the ISA string.
>> +*/
>> +break;
>> +
>> +default:
>> +*p++ = lower;
>> +break;
>> +}
>> }
>> }
>> *p = '\0';
>> --
>> 2.37.1
>>
>>
