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

[Lina Iyer <ilina@codeaurora.org>]

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