Re: [PATCH v3 1/2] drivers: irqchip: pdc: Add PDC interrupt controller for QCOM SoCs

[Lina Iyer]

On Tue, Feb 06 2018 at 20:34 +0000, Marc Zyngier wrote:
> On Tue, 06 Feb 2018 18:09:04 +0000,
> Lina Iyer wrote:
> > +#define PDC_MAX_IRQS		126
>
> From v2: "Is that an absolute, architectural maximum? Or should it
> come from the DT (being the sum of all ranges that are provided by
> this PDC)?"
Architectural maximum. Sorry I forgot to respond earlier.

> > +static inline void pdc_reg_write(int reg, u32 i, u32 val)
> > +{
> > +	writel_relaxed(val, pdc_base + reg + i * sizeof(u32));
> > +}
> > +
> > +static inline u32 pdc_reg_read(int reg, u32 i)
> > +{
> > +	return readl_relaxed(pdc_base + reg + i * sizeof(u32));
> > +}
> > +
> > +static inline void pdc_enable_intr(struct irq_data *d, bool on)
>
> You can loose the "inline" on these 3 function, I believe the compiler
> will do a pretty good job specialising them.
Ok.

> > + * 3'b0 00  Level sensitive active low    LOW
> > + * 3'b0 01  Rising edge sensitive         NOT USED
> > + * 3'b0 10  Falling edge sensitive        LOW
> > + * 3'b0 11  Dual Edge sensitive           NOT USED
> > + * 3'b1 00  Level senstive active High    HIGH
>
> sensitive
Ok

> > +
> > +static struct irq_chip qcom_pdc_gic_chip = {
>
> const?
>
> > +	.name			= "pdc-gic",
>
> Just call it "PDC".
OK to both.

> > +	.irq_eoi		= irq_chip_eoi_parent,
> > +	.irq_mask		= qcom_pdc_gic_mask,
> > +	.irq_unmask		= qcom_pdc_gic_unmask,
> > +	.irq_retrigger		= irq_chip_retrigger_hierarchy,
> > +	.irq_set_type		= qcom_pdc_gic_set_type,
> > +	.flags			= IRQCHIP_MASK_ON_SUSPEND |
> > +				  IRQCHIP_SET_TYPE_MASKED |
> > +				  IRQCHIP_SKIP_SET_WAKE,
> > +	.irq_set_vcpu_affinity	= irq_chip_set_vcpu_affinity_parent,
> > +#ifdef CONFIG_SMP
> > +	.irq_set_affinity	= irq_chip_set_affinity_parent,
> > +#endif
>
> Is this supposed to work on any architecture other than arm64? If not,
> then you can loose the CONFIG_SMP, as there is no !SMP config on arm64.
Only ARM64 afaict. But who knows ? :)

> > +};
> > +
> > +static irq_hw_number_t get_parent_hwirq(int pin)
> > +{
> > +	int i;
> > +	struct pdc_pin_region *region;
> > +
> > +	for (i = 0; i < pdc_region_cnt; i++) {
> > +		region = &pdc_region[i];
> > +		if (pin >= region->pin_base &&
> > +		   pin < region->pin_base + region->cnt)
> > +			return (region->parent_base + pin - region->pin_base);
> > +	}
> > +
> > +	WARN_ON(1);
> > +	return pin;
>
> Do not return a valid value in case of error. Please return an error
> value that you will check in the caller. Otherwise you're feeding the
> GIC with a SPI number that is actually a PDC pin number. That is not
> going to have any positive effect.
How about the max of irq_hw_number_t ?

> > +static int qcom_pdc_alloc(struct irq_domain *domain,
> > +			unsigned int virq, unsigned int nr_irqs, void *data)
> > +{
> > +	struct irq_fwspec *fwspec = data;
> > +	struct irq_fwspec parent_fwspec;
> > +	irq_hw_number_t hwirq, parent_hwirq;
> > +	unsigned int type;
> > +	int ret;
> > +
> > +	ret = qcom_pdc_translate(domain, fwspec, &hwirq, &type);
> > +	if (ret)
> > +		return -EINVAL;
> > +
> > +	parent_hwirq = get_parent_hwirq(hwirq);
> > +	ret  = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> > +					    &qcom_pdc_gic_chip,
> > +					    (void *)parent_hwirq);
>
> Nothing else in this driver should be concerned with the parent hwirq,
> so NULL should be an appropriate value for chip_data.
Yes, I forgot to remove it. I don't need this anymore.

> > +	region = kcalloc(n, sizeof(*region), GFP_KERNEL);
> > +	if (!region)
> > +		return -ENOMEM;
> > +
> > +	ret = of_property_read_u32_array(np, "qcom,pdc-ranges", region, n);
> > +	if (ret) {
> > +		kfree(region);
> > +		return -EINVAL;
> > +	}
> > +
> > +	pdc_region = (struct pdc_pin_region *)region;
>
> Oh please... Don't resort to that kind of hack. Next thing you know,
> someone will add a u8 field to that structure and you'll spend the
> following 2 hours trying to understand why it all went so wrong.
> Allocate a bounce buffer if you want, copy fields one by one, I don't
> care. Just don't do that. :-(
:) ok.

> > +	pdc_region_cnt = n / 3;
> > +
> > +	return 0;
> > +}
> > +
> > +int qcom_pdc_init(struct device_node *node, struct device_node *parent)
>
> static.
OK.

> > +{
> > +	struct irq_domain *parent_domain, *pdc_domain;
> > +	int ret;
> > +
> > +	pdc_base = of_iomap(node, 0);
> > +	if (!pdc_base) {
> > +		pr_err("%s: unable to map PDC registers\n", node->full_name);
> > +		return -ENXIO;
> > +	}
> > +
> > +	parent_domain = irq_find_host(parent);
> > +	if (!parent_domain) {
> > +		pr_err("%s: unable to obtain PDC parent domain\n",
> > +		      node->full_name);
>
> Use %pOF instead of %s and node->full_name in all occurrences in this
> function  (see commit ce4fecf1fe15).
Sure.

> Thanks,
Once again thanks Marc.

-- Lina