RE: [PATCH 2/6] irqchip: Add interrupt controller support for Realtek DHC SoCs

[James Tai [戴志峰]]

Hi Thomas,

>> Signed-off-by: James Tai <james.tai@xxxxxxxxxxx>
>> Change-Id: I1e711c70414c97c2c8497bd4ac9e8bbd964225c3
>
>Please remove these internal change ids. They are not useful for anyone outside
>realtek.
>
I will fix it in the next patches.

>
>> +static unsigned int realtek_intc_get_ints(struct realtek_intc_data
>> +*data)
>
>static inline perhaps?
>
Yes. I will use 'static inline' to replace it.

>> +static unsigned int realtek_intc_get_inte(struct realtek_intc_data
>> +*data) {
>> +     unsigned int val;
>> +     unsigned long flags;
>
>        unsigned long flags;
>        unsigned int val;
>
>https://www.kernel.org/doc/html/latest/process/maintainer-tip.html
>
>Please fix the variable declarations all over the place.
>
I will fix it in the next patches.

>> +     spin_lock_irqsave(&data->lock, flags);
>
>This needs to be a raw spinlock.
>
I will fix it in the next patches.

>
>
>> +static void realtek_intc_enable_irq(struct irq_data *data) {
>> +     struct realtek_intc_data *intc_data =
>irq_data_get_irq_chip_data(data);
>> +     unsigned long flags;
>> +     u32 scpu_int_en, mask;
>
>See above.
>
I will fix it in the next patches.

>
>So you update the effective affinity even if it cannot be set or if the parent irq
>returns an error code?
>
>Aside of that setting it to cpu_online mask is just wrong. This is _NOT_ the
>effective affinity because the underlying GIC selects a single target CPU out of
>the caller provides cpu mask.
>
>That said, this is also completely inconsistent vs. the other interrupts which
>share that GIC interrupt instance. I.e. /proc/irq/$N/affinity and effective_affinity
>become random number generators. That'll confuse existing userspace tools.
>
>Having an affinity setter for demultiplexes interrupts is simply wrong.
>
I will use the 'irq_chip_set_affinity_parent' replace the 'realtek_intc_set_affinity'.

>> +static struct irq_chip realtek_intc_chip = {
>> +     .name = "realtek-intc",
>> +     .irq_mask = realtek_intc_mask_irq,
>> +     .irq_unmask = realtek_intc_unmask_irq,
>> +     .irq_enable = realtek_intc_enable_irq,
>> +     .irq_disable = realtek_intc_disable_irq,
>> +     .irq_set_affinity = realtek_intc_set_affinity,
>
>See docs vs. formatting of struct initializers.
>
I will fix it in the next patches.

>> +};
>> +
>> +
>> +     data->subset_data_num = info->cfg_num;
>> +     for (i = 0; i < info->cfg_num; i++) {
>> +             ret = realtek_intc_subset(node, data, i);
>> +             WARN(ret, "failed to init subset %d: %d", i, ret);
>
>If this fails, then you still expose the affected interrupts as functional?

I will modify the code to return an error code and stop the driver initialization.

>
>> +/**
>> + * realtek_intc_subset_cfg - subset interrupt mask
>> + * @ints_mask: inetrrupt mask
>> + */
>> +struct realtek_intc_subset_cfg {
>> +     unsigned int ints_mask;
>> +};
>> +
>> +/**
>> + * realtek_intc_info - interrupt controller data.
>> + * @isr_offset: interrupt status register offset.
>> + * @umsk_isr_offset: unmask interrupt status register offset.
>> + * @scpu_int_en_offset: interrupt enable register offset.
>> + * @cfg: cfg of the subset.
>> + * @cfg_num: number of cfg.
>> + */
>> +struct realtek_intc_info {
>> +     unsigned int isr_offset;
>> +     unsigned int umsk_isr_offset;
>> +     unsigned int scpu_int_en_offset;
>> +     const u32 *isr_to_scpu_int_en_mask;
>> +     const struct realtek_intc_subset_cfg *cfg;
>> +     int cfg_num;
>
>See formatting doc.

I will fix it in the next patches.

>> +
>> +#define IRQ_ALWAYS_ENABLED  (-1)
>
>U32_MAX ?
>
I will use 'U32_MAX' to replace '-1'.

>> +#define DISABLE_INTC (0)
>> +#define CLEAN_INTC_STATUS (0xfffffffe)
>
>That's what GENMASK() is for.
>
I will use ' GENMASK(31, 1) ' to replace '0xfffffffe '.

Thank you for your feedback.

Regards,
James