lkml.org 
[lkml]   [2015]   [Jul]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH V5 1/2] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup sources
Date
Hi Thomas,

Thank you for the high-quality review. Please check my comments inline below.

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@linutronix.de]
> Sent: 2015年7月17日 16:50
> To: Wang Shenwei-B38339
> Cc: shawn.guo@linaro.org; jason@lakedaemon.net;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH V5 1/2] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup
> sources
>
> On Fri, 17 Jul 2015, Shenwei Wang wrote:
> > +++ b/drivers/irqchip/irq-imx-gpcv2.c
> > @@ -0,0 +1,311 @@
> > +
> > +/*
> > + * Copyright (C) 2015 Freescale Semiconductor, Inc.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > +modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#include <linux/of_address.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/slab.h>
> > +#include <linux/syscore_ops.h>
> > +#include "irqchip.h"
>
> This file is deprecated. Use linux/irqchip.h instead.
>
Okay.

> > +static struct imx_irq_gpcv2 *gpcv2_get_chip_data(void) {
> > + struct irq_data *data;
> > + int virq;
> > +
> > + /* Since GPCv2 is the default IRQ domain, its private data can
> > + * be gotten from any irq descriptor. Here we use interrupt #19
> > + * which is for snvs-rtc.
> > + */
>
> Yuck. What kind of logic is that?
>
> Use some random hardcoded number to find something which has been set by
> this driver?
>
> > + virq = irq_find_mapping(0, 19);
> > +
> > + for (data = irq_get_irq_data(virq); data;
> > + data = data->parent_data) {
> > + if (!data->domain)
> > + continue;
> > +
> > + if (!strcmp(data->domain->name, "GPCv2"))
>
> So you are relying on internal knowledge of the irq domain code which sets the
> domain name to the chip name if the domain name is not initialized by other
> means.
>
> Brilliant, NOT!
>
> In other words you are interested in the irq chip associated with that domain and
> not with the domain itself.
>
> Care to explain what you are trying to do and why you think there are no better
> ways to figure that out?
>
When I wrote the driver, there were two options to let other modules like suspend
and cpuidle drivers to access this instance of imx_irq_gpcv2:
Option #1 is to use the private data pointer of the irqdomain.
Option #2 is to export a global variable here.
I selected option #1 because it could decouple this irq driver from those who may
use this instance. But option #2 is more direct and simple.

> > + return data->chip_data;
> > + }
> > +
> > + return 0;
>
> This wants to be 'return NULL;' if at all.
>
Okay.

> > +}
> > +
> > +static int gpcv2_wakeup_source_save(void) {
> > + struct imx_irq_gpcv2 *cd;
> > + void __iomem *reg;
> > + int i;
> > +
> > + cd = gpcv2_get_chip_data();
> > + if (!cd)
> > + return 0;
> > +
> > + for (i = 0; i < IMR_NUM; i++) {
>
> Why is IMR_NUM a hard coded constant and not provided by DT?
>
It can be provided by DT. However, as it is a fixed number and will never change once the
Chip is produced I selected to hard code it.

> > + reg = cd->gpc_base + cd->cpu2wakeup + i * 4;
> > + cd->enabled_irqs[i] = readl_relaxed(reg);
> > + writel_relaxed(cd->wakeup_sources[i], reg);
> > + }
> > +
> > + pr_devel("%s ----\r\n", __func__);
> > + pr_devel("Enabled IRQ:\t %08X %08X %08X %08X\r\n",
> > + cd->enabled_irqs[0],
> > + cd->enabled_irqs[1],
> > + cd->enabled_irqs[2],
> > + cd->enabled_irqs[3]);
> > + pr_devel("Wakeup Sources:\t %08X %08X %08X %08X\r\n",
> > + cd->wakeup_sources[0],
> > + cd->wakeup_sources[1],
> > + cd->wakeup_sources[2],
> > + cd->wakeup_sources[3]);
>
> Do we really need this debug stuff in mainline?
>
Ok to remove it.

> > +static int imx_gpcv2_irq_set_wake(struct irq_data *d, unsigned int
> > +on) {
> > + unsigned int idx = d->hwirq / 32;
> > + struct imx_irq_gpcv2 *cd = d->chip_data;
> > + u32 mask, val;
> > + unsigned long flags;
> > + void __iomem *reg;
>
> Can you come up with an even less readable way to arrange those declarations?
>
Will change the declarations like following:
u32 mask, val;
unsigned long flags;
void __iomem *reg;

unsigned int idx = d->hwirq / 32;
struct imx_irq_gpcv2 *cd = d->chip_data;

> > + spin_lock_irqsave(&cd->lock, flags);
>
> That wants to be a raw spinlock.
>
Will change it to raw_spin_lock

> > + reg = cd->gpc_base + cd->cpu2wakeup + idx * 4;
>
> This lacks a comment explaining the magic math
>
> > +static void imx_gpcv2_irq_unmask(struct irq_data *d) {
> > + void __iomem *reg;
> > + struct imx_irq_gpcv2 *cd = d->chip_data;
> > + u32 val;
> > + unsigned long flags;
>
> Another random variant of arranging declarations.
>
Will change it like following:
u32 val;
unsigned long flags;
void __iomem *reg;
struct imx_irq_gpcv2 *cd = d->chip_data;

>>+ spin_lock_irqsave(&cd->lock, flags);
>
> This callback is always called with interrupts disabled, so this wants to be
> raw_spin_lock()
>
Okay.

> > + reg = cd->gpc_base + cd->cpu2wakeup + d->hwirq / 32 * 4;
> > + val = readl_relaxed(reg);
> > + val &= ~(1 << d->hwirq % 32);
> > + writel_relaxed(val, reg);
> > + irq_chip_unmask_parent(d);
>
> Why needs this to be called under cd->lock?
>
You are right. Irq_chip_unmask_parent(d) should be moved under cd->lock.

> If there is a requirement to do so, then it needs a comment.
>
> > + spin_unlock_irqrestore(&cd->lock, flags);
>
> > +static int imx_gpcv2_domain_xlate(struct irq_domain *domain,
> > + struct device_node *controller,
> > + const u32 *intspec,
> > + unsigned int intsize,
> > + unsigned long *out_hwirq,
> > + unsigned int *out_type)
> > +{
> > + if (domain->of_node != controller)
> > + return -EINVAL; /* Shouldn't happen, really... */
>
> Tail comments are horrible to read. Either your comment has a value then put it
> above the if construct or just get rid of it.
>
Will remove it.

> > +static int __init imx_gpcv2_irqchip_init(struct device_node *node,
> > + struct device_node *parent) {
> > + struct irq_domain *parent_domain, *domain;
> > + int i, val;
> > + struct imx_irq_gpcv2 *cd;
> > +
> > + if (!parent) {
> > + pr_err("%s: no parent, giving up\n", node->full_name);
> > + return -ENODEV;
> > + }
> > +
> > + parent_domain = irq_find_host(parent);
> > + if (!parent_domain) {
> > + pr_err("%s: unable to obtain parent domain\n", node->full_name);
> > + return -ENXIO;
> > + }
> > +
> > + cd = kzalloc(sizeof(struct imx_irq_gpcv2), GFP_KERNEL);
> > + BUG_ON(!cd);
>
> This BUG_ON does not make any sense as you just return an error code if
> something else goes wrong after that.
>
Make sense. Will remove it.

> > + cd->gpc_base = of_iomap(node, 0);
> > + if (!cd->gpc_base) {
> > + pr_err("fsl-gpcv2: unable to map gpc registers\n");
> > + kzfree(cd);
>
> Surely this needs kzfree because cd contains security relevant data, right?
>
It is not secure data. Will change it to kfree.

> > + /*
> > + * Due to hardware design requirement, need to make sure GPR
>
> s/requirement/failure/ right?
>
Agree.

> > + * interrupt(#32) is unmasked during RUN mode to avoid entering
> > + * DSM by mistake.
> > + */
> > + writel_relaxed(~0x1, cd->gpc_base + cd->cpu2wakeup);
>
> Magic constant '~0x1' ?
>
> > +
> > + /* Mask the wakeup sources in M/F power domain */
> > + cd->mfmix_mask[0] = 0x54010000;
> > + cd->mfmix_mask[1] = 0xc00;
> > + cd->mfmix_mask[2] = 0x0;
> > + cd->mfmix_mask[3] = 0x400010;
>
> Again, really intuitive and self explaining magic constants.
>
> > +
> > +IRQCHIP_DECLARE(imx_gpcv2, "fsl,imx7d-gpc", imx_gpcv2_irqchip_init);
>
> I'm not a DT expert, but AFAIK there is a requirement to document the bindings
> somewhere. -ENODOCUMENT!
>
Will add this string in Documentation/devicetree/bindings/power/fsl,imx-gpc.txt

> > +
> > +enum gpcv2_mode {
> > + WAIT_CLOCKED, /* wfi only */
>
> Again, tail comments are horrible to read. Use proper KernelDoc if you want to
> document stuff. Though I doubt that any of these comments have any value at all.
>
> Aside of that these names are pretty generic and prone to create a namespace
> clash sooner or later.
>
Will remove the tail comments.

> > + WAIT_UNCLOCKED, /* WAIT */
> > + WAIT_UNCLOCKED_POWER_OFF, /* WAIT + SRPG */
> > + STOP_POWER_ON, /* just STOP */
> > + STOP_POWER_OFF, /* STOP + SRPG */
> > +};
> > +
> > +enum gpcv2_slot {
>
> What is this enum for?
>
It defined all the power domains that are controlled by GPCv2 block.

> > + CORE0_A7,
> > + CORE1_A7,
> > + SCU_A7,
> > + FAST_MEGA_MIX,
> > + MIPI_PHY,
> > + PCIE_PHY,
> > + USB_OTG1_PHY,
> > + USB_OTG2_PHY,
> > + USB_HSIC_PHY,
> > + CORE0_M4,
>
> Namespace?
>
> > +};
>
> > +struct imx_irq_gpcv2 {
> > + spinlock_t lock;
>
> raw_spinlock_t
>
> > + void __iomem *gpc_base;
> > + struct regmap *anatop;
> > + struct regmap *imx_src;
> > + u32 wakeup_sources[IMR_NUM];
> > + u32 enabled_irqs[IMR_NUM];
> > + u32 mfmix_mask[IMR_NUM];
> > + u32 wakeupmix_mask[IMR_NUM];
> > + u32 lpsrmix_mask[IMR_NUM];
> > + u32 cpu2wakeup;
> > + void (*lpm_env_setup)(struct imx_irq_gpcv2 *);
> > + void (*lpm_env_clean)(struct imx_irq_gpcv2 *);
> > + void (*lpm_cpu_power_gate)(struct imx_irq_gpcv2 *, u32, bool);
> > + void (*lpm_plat_power_gate)(struct imx_irq_gpcv2 *, bool);
> > + void (*set_mode)(struct imx_irq_gpcv2 *, enum gpcv2_mode mode);
> > + void (*standby)(struct imx_irq_gpcv2 *);
> > + void (*suspend)(struct imx_irq_gpcv2 *);
> > + void (*set_slot)(struct imx_irq_gpcv2 *cd, u32 index,
> > + enum gpcv2_slot m_core, bool mode, bool ack);
> > + void (*clear_slots)(struct imx_irq_gpcv2 *);
> > + void (*lpm_enable_core)(struct imx_irq_gpcv2 *,
> > + bool enable, u32 offset);
> > +
> > +
> > + void (*suspend_fn_in_ocram)(void __iomem *ocram_vbase);
> > + void __iomem *ocram_vbase;
>
> How many of these struct members are actually relevant to this driver and what
> is the purpose of those? A proper KernelDoc comment would shed some light on
> it.
>
This struct defines the properties and functions that GPCv2 block provides. Since GPCv2 has
two key functions: Irq wakeup source management and power management, the
intention of the struct is to share data and methods among irqchip, suspend, and cpuidle drivers.

> > +};
> > +
> > +void ca7_cpu_resume(void);
> > +void imx7_suspend(void __iomem *ocram_vbase);
>
> How is that related to the irqchip driver?
>
These two functions are used by suspend driver.

Thanks,
Shenwei


> Thanks,
>
> tglx
\
 
 \ /
  Last update: 2015-07-21 17:21    [W:0.118 / U:0.240 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site