On 13.06.2025 17:48, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/arch/riscv/aplic-priv.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: MIT */
> +
> +/*
> + * xen/arch/riscv/aplic-priv.h
> + *
> + * Private part of aplic.h header.
> + *
> + * RISC-V Advanced Platform-Level Interrupt Controller support
> + *
> + * Copyright (c) Microchip.
> + * Copyright (c) Vates.
> + */
> +
> +#ifndef ASM_RISCV_PRIV_APLIC_H
> +#define ASM_RISCV_PRIV_APLIC_H
> +
> +#include <xen/types.h>
> +
> +#include <asm/aplic.h>
> +#include <asm/imsic.h>
> +
> +struct aplic_priv {
> + /* base physical address and size */
I'm sure I did ask for this before, and such a request really is meant to apply
globally: Please can you abide by the comment style set forth in ./CODING_STYLE.
> +static int __init cf_check aplic_init(void)
> +{
> + dt_phandle imsic_phandle;
> + const __be32 *prop;
> + uint64_t size, paddr;
> + const struct dt_device_node *imsic_node;
> + const struct dt_device_node *node = aplic_info.node;
> + int rc;
> +
> + /* Check for associated imsic node */
> + if ( !dt_property_read_u32(node, "msi-parent", &imsic_phandle) )
> + panic("%s: IDC mode not supported\n", node->full_name);
> +
> + imsic_node = dt_find_node_by_phandle(imsic_phandle);
> + if ( !imsic_node )
> + panic("%s: unable to find IMSIC node\n", node->full_name);
> +
> + rc = imsic_init(imsic_node);
> + if ( rc == IRQ_M_EXT )
> + /* Machine mode imsic node, ignore this aplic node */
> + return 0;
> +
> + if ( rc )
> + panic("%s: Failed to initialize IMSIC\n", node->full_name);
> +
> + /* Find out number of interrupt sources */
> + if ( !dt_property_read_u32(node, "riscv,num-sources",
> + &aplic_info.num_irqs) )
> + panic("%s: failed to get number of interrupt sources\n",
> + node->full_name);
> +
> + if ( aplic_info.num_irqs > ARRAY_SIZE(aplic.regs->sourcecfg) )
> + aplic_info.num_irqs = ARRAY_SIZE(aplic.regs->sourcecfg);
> +
> + prop = dt_get_property(node, "reg", NULL);
> + dt_get_range(&prop, node, &paddr, &size);
> + if ( !paddr )
> + panic("%s: first MMIO resource not found\n", node->full_name);
> +
> + if ( !IS_ALIGNED(paddr, KB(4)) )
> + panic("%s: paddr of memory-mapped control region should be 4Kb
> aligned:%#lx\n",
> + __func__, paddr);
> +
> + if ( !IS_ALIGNED(size, KB(4)) || (size < KB(16)) )
> + panic("%s: memory-mapped control region should be a multiple of 4
> KiB in size and the smallest valid control is 16Kb: %#lx\n",
The line having grown so long should have served as an indication to abbreviate
the
text somewhat.
Also note the consmetic difference between this and the earlier message, as to a
blank (or not) after the latter colon. Please try to be consistent at least
within
a patch / function / whatever other unit.
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/aplic.h
> @@ -0,0 +1,64 @@
> +/* SPDX-License-Identifier: MIT */
> +
> +/*
> + * xen/arch/riscv/asm/include/aplic.h
> + *
> + * RISC-V Advanced Platform-Level Interrupt Controller support
> + *
> + * Copyright (c) Microchip.
> + */
> +
> +#ifndef ASM_RISCV_APLIC_H
> +#define ASM_RISCV_APLIC_H
> +
> +#include <xen/types.h>
> +
> +#include <asm/imsic.h>
> +
> +#define APLIC_DOMAINCFG_IE BIT(8, U)
> +#define APLIC_DOMAINCFG_DM BIT(2, U)
> +
> +struct aplic_regs {
> + uint32_t domaincfg;
> + uint32_t sourcecfg[1023];
> + uint8_t _reserved1[3008];
> +
> + uint32_t mmsiaddrcfg;
> + uint32_t mmsiaddrcfgh;
> + uint32_t smsiaddrcfg;
> + uint32_t smsiaddrcfgh;
> + uint8_t _reserved2[48];
> +
> + uint32_t setip[32];
> + uint8_t _reserved3[92];
> +
> + uint32_t setipnum;
> + uint8_t _reserved4[32];
> +
> + uint32_t in_clrip[32];
> + uint8_t _reserved5[92];
> +
> + uint32_t clripnum;
> + uint8_t _reserved6[32];
> +
> + uint32_t setie[32];
> + uint8_t _reserved7[92];
> +
> + uint32_t setienum;
> + uint8_t _reserved8[32];
> +
> + uint32_t clrie[32];
> + uint8_t _reserved9[92];
> +
> + uint32_t clrienum;
> + uint8_t _reserved10[32];
> +
> + uint32_t setipnum_le;
> + uint32_t setipnum_be;
> + uint8_t _reserved11[4088];
> +
> + uint32_t genmsi;
> + uint32_t target[1023];
> +};
Each time I see this I wonder whether it wouldn't be helpful if, at least for
the non-reserved fields, there would be comments clarifying their hex offset.
That way it would be easier to (a) compare with the spec and (b) cross-check
the array dimensions used.
Jan
