Re: [PATCH V2 19/19] irqchip: add C-SKY irqchip drivers

[Guo Ren <ren_guo@c-sky.com>]

On Tue, Jul 03, 2018 at 11:28:03AM +0200, Thomas Gleixner wrote:
> -EEMPTYCHANGELOG
Ok, I'll seperate this patchset's changelog from cover-letter in next
version patch.

> > +#ifdef CONFIG_CSKY_VECIRQ_LEGENCY
> 
> I assume you meant _LEGACY
Yes.

> > +#include <asm/reg_ops.h>
> > +#endif
> 
> Also why making the include conditional. Just include it always and be done
> with it.
Ok.

> > +static void __iomem *reg_base;
> > +
> > +#define INTC_ICR	0x00
> > +#define INTC_ISR	0x00
> > +#define INTC_NEN31_00	0x10
> > +#define INTC_NEN63_32	0x28
> > +#define INTC_IFR31_00	0x08
> > +#define INTC_IFR63_32	0x20
> > +#define INTC_SOURCE	0x40
> > +
> > +#define INTC_IRQS	64
> > +
> > +#define INTC_ICR_AVE	BIT(31)
> > +
> > +#define VEC_IRQ_BASE	32
> > +
> > +static struct irq_domain *root_domain;
> > +
> > +static void __init ck_set_gc(void __iomem *reg_base, u32 irq_base,
> > +			     u32 mask_reg)
> > +{
> > +	struct irq_chip_generic *gc;
> > +
> > +	gc = irq_get_domain_generic_chip(root_domain, irq_base);
> > +	gc->reg_base = reg_base;
> > +	gc->chip_types[0].regs.mask = mask_reg;
> > +	gc->chip_types[0].chip.irq_mask = irq_gc_mask_clr_bit;
> > +	gc->chip_types[0].chip.irq_unmask = irq_gc_mask_set_bit;
> > +}
> > +
> > +static struct irq_domain *root_domain;
> 
> You have the same declaration 10 lines above already....
Opps, thx. I'll remove.

> > +static void ck_irq_handler(struct pt_regs *regs)
> > +{
> > +#ifdef CONFIG_CSKY_VECIRQ_LEGENCY
> > +	irq_hw_number_t irq = ((mfcr("psr") >> 16) & 0xff) - VEC_IRQ_BASE;
> > +#else
> > +	irq_hw_number_t irq = readl_relaxed(reg_base + INTC_ISR) & 0x3f;
> > +#endif
> 
> You can avoid the ifdeffery by doing:
> 
> 	irq_hw_number_t irq;
> 
> 	if (IS_ENABLED(CONFIG_CSKY_VECIRQ_LEGENCY))
> 		irq = ((mfcr("psr") >> 16) & 0xff) - VEC_IRQ_BASE;
> 	else
> 		irq = readl_relaxed(reg_base + INTC_ISR) & 0x3f;
> 
> which makes the whole thing more readable.
Ok, good idea.

> > +#define expand_byte_to_word(i) (i|(i<<8)|(i<<16)|(i<<24))
> > +static inline void setup_irq_channel(void __iomem *reg_base)
> 
> Please do not glue a define and a function declaration togetther w/o a new
> line in between. That's really hard to parse.
> 
> Also please make that expand macro an inline function.
> 
> > +{
> > +	int i;
> 
> Bah: writel_relaxed(u32 value, ...). So why 'int' ? Just because?
> 
> > +
> > +	/*
> > +	 * There are 64 irq nums and irq-channels and one byte per channel.
> > +	 * Setup every channel with the same hwirq num.
> 
> This is magic and not understandable for an outsider.
> 
> > +	 */
> > +	for (i = 0; i < INTC_IRQS; i += 4) {
> > +		writel_relaxed(expand_byte_to_word(i) + 0x00010203,
> 
> No magic numbers please w/o explanation. And '+' is the wrong operator
> here, really.  Stick that into the expand function:
> 
> static inline u32 build_intc_ctrl(u32 idx)
> {
> 	u32 res;
> 
> 	/*
> 	 * Set the same index for each channel (or whatever
> 	 * this means in reality).
> 	 */
> 	res = idx | (idx << 8) | (idx || 16) | (idx << 24);
> 
> 	/*
> 	 * Set the channel magic number in descending order because
> 	 * the most significant bit comes first. (Replace with
> 	 * something which has not been pulled out of thin air).
> 	 */
> 	return res | 0x00010203;
> }
> 
> Hmm?
That's Ok. And here is my implementation:

static inline u32 build_intc_ctrl(u32 idx)
{
	/*
	 * One channel is one byte in a word-width register, so
	 * there are four channels in a word-width register.
	 *
	 * Set the same index for each channel and it will make
	 * "irq num = channel num".
	 */
	return (idx             | ((idx + 1) << 8) |
	      ((idx + 2) << 16) | ((idx + 3) << 24));
}
Hmm? (No magic number)

> > +#ifndef CONFIG_CSKY_VECIRQ_LEGENCY
> > +	writel_relaxed(INTC_ICR_AVE, reg_base + INTC_ICR);
> > +#else
> > +	writel_relaxed(0, reg_base + INTC_ICR);
> > +#endif
> 
> See above.
Ok, use if (IS_ENABLED(CONFIG_CSKY_VECIRQ_LEGENCY))

> > +++ b/drivers/irqchip/irq-csky-v2.c
> > @@ -0,0 +1,191 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (C) 2018 Hangzhou C-SKY Microsystems co.,ltd.
> 
> Please stick an empty newline here for separation
Ok

> > +	static void __iomem	*reg_base;
> > +	irq_hw_number_t		hwirq;
> > +
> > +	reg_base = *this_cpu_ptr(&intcl_reg);
> 
> Wheee!
> 
> 	static void __iomem *reg_base = this_cpu_read(intcl_reg);
> 	irq_hw_number_t	hwirq;
> 
> Hmm?
Thx for the tips and I'll use this_cpu_read() without static.
 	void __iomem *reg_base = this_cpu_read(intcl_reg);

> > +	writel_relaxed(cpu, INTCG_base + INTCG_CIDSTR + (4*(d->hwirq - COMM_IRQ_BASE)));
> 
> Spaces between '4' and '*' and '(d->)' please. And to avoid the overly long
> line use a local variable to calculate the value.
Ok.

> > +	} else
> > +		irq_set_chip_and_handler(irq, &csky_irq_chip, handle_fasteoi_irq);
> 
> The else path wants curly braces as well.
Ok.

> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (C) 2018 Hangzhou NationalChip Science & Technology Co.,Ltd.
> 
> See above
Ok, stick an empty newline

> > +		writel_relaxed(expand_byte_to_word(i) + 0x03020100,
> 
> This magic number is the reverse of the above magic. Is that intentional.
> 
> > +			       reg_base + INTC_SOURCE + i);
> > +	}
> 
> See above. 
No magic number and use inline func.


> > +static int __init
> > +intc_init(struct device_node *node, struct device_node *parent)
> > +{
> > +	u32 clr = IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN;
> > +	int ret;
> 
> Aside of that the whole thing might share the code with the other one, but
> it might not be worth it. At least this wants to be documented in the
> changelog why sharing the code is not useful...
Do you mean merge irq-csky-v1.c irq-csky-v2.c irq-nationalchip.c into
one file eg: irq-csky.c? 

Guo Ren