Re: [PATCH v2 2/3] irqchip: Add the Sophgo SG2042 MSI interrupt controller

[Chen Wang <unicorn_wang@outlook.com>]

On 2024/12/12 0:32, Christophe JAILLET wrote:
> Le 09/12/2024 à 08:12, Chen Wang a écrit :
>> From: Chen Wang <unicorn_wang@outlook.com>
>>
>> Add driver for Sophgo SG2042 MSI interrupt controller.
>>
>> Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
>
> ...
>
>> +#define SG2042_VECTOR_MIN    64
>> +#define SG2042_VECTOR_MAX    95
>
> ...
>
>> +static struct irq_chip sg2042_msi_middle_irq_chip = {
>
> const?
Yes, I will add this in next version, thanks.
>
>> +    .name            = "SG2042 MSI",
>> +    .irq_ack        = sg2042_msi_irq_ack,
>> +    .irq_mask        = irq_chip_mask_parent,
>> +    .irq_unmask        = irq_chip_unmask_parent,
>> +#ifdef CONFIG_SMP
>> +    .irq_set_affinity    = irq_chip_set_affinity_parent,
>> +#endif
>> +    .irq_compose_msi_msg    = sg2042_msi_irq_compose_msi_msg,
>> +};
>
> ...
>
>> +static int sg2042_msi_probe(struct platform_device *pdev)
>> +{
>> +    struct of_phandle_args args = {};
>> +    struct sg2042_msi_data *data;
>> +    int ret;
>> +
>> +    data = devm_kzalloc(&pdev->dev, sizeof(struct sg2042_msi_data), 
>> GFP_KERNEL);
>> +    if (!data)
>> +        return -ENOMEM;
>> +
>> +    data->reg_clr = devm_platform_ioremap_resource_byname(pdev, "clr");
>> +    if (IS_ERR(data->reg_clr)) {
>> +        dev_err(&pdev->dev, "Failed to map clear register\n");
>> +        return PTR_ERR(data->reg_clr);
>> +    }
>> +
>> +    if (of_property_read_u64(pdev->dev.of_node, 
>> "sophgo,msi-doorbell-addr",
>> +                 &data->doorbell_addr)) {
>> +        dev_err(&pdev->dev, "Unable to parse MSI doorbell addr\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    ret = of_parse_phandle_with_args(pdev->dev.of_node, "msi-ranges",
>> +                     "#interrupt-cells", 0, &args);
>> +    if (ret) {
>> +        dev_err(&pdev->dev, "Unable to parse MSI vec base\n");
>> +        return ret;
>> +    }
>> +    data->irq_first = (u32)args.args[0];
>> +
>> +    ret = of_property_read_u32_index(pdev->dev.of_node, "msi-ranges",
>> +                     args.args_count + 1, &data->num_irqs);
>> +    if (ret) {
>> +        dev_err(&pdev->dev, "Unable to parse MSI vec number\n");
>> +        return ret;
>> +    }
>> +
>> +    if (data->irq_first < SG2042_VECTOR_MIN ||
>> +        (data->irq_first + data->num_irqs - 1) > SG2042_VECTOR_MAX) {
>> +        dev_err(&pdev->dev, "msi-ranges is incorrect!\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    mutex_init(&data->msi_map_lock);
>> +
>> +    data->msi_map = bitmap_zalloc(data->num_irqs, GFP_KERNEL);
>
> IIUC, num_irqs is between 0 and (SG2042_VECTOR_MAX - 
> SG2042_VECTOR_MIN) (maybe + or -1).
> So around 32.
>
> Would it make sence to use DECLARE_BITMAP(msi_map, <correct_size>) in 
> sg2042_msi_data to avoid this allocation and an indirection at runtime?

This is also a good choice. I will double check this.

Thanks,

Chen

>
>> +    if (!data->msi_map)
>> +        return -ENOMEM;
>> +
>> +    ret = sg2042_msi_init_domains(data, pdev->dev.of_node);
>> +    if (ret)
>> +        bitmap_free(data->msi_map);
>> +
>> +    return ret;
>> +}
>
> ...
>
> CJ