lkml.org 
[lkml]   [2018]   [Jul]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH V2 19/19] irqchip: add C-SKY irqchip drivers
    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

    \
     
     \ /
      Last update: 2018-07-04 07:09    [W:4.238 / U:0.652 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site