Re: [PATCH 6/7] [PATCH] MIPS: OCTEON: Add support for OCTEON III interrupt controller.

[David Daney <ddaney@caviumnetworks.com>]

On 02/05/2016 01:06 AM, Thomas Gleixner wrote:
> On Thu, 4 Feb 2016, David Daney wrote:
>> +static int octeon_irq_ciu_set_type(struct irq_data *data, unsigned int t)
>> +{
>> +	irqd_set_trigger_type(data, t);
>> +
>> +	if (irqd_get_trigger_type(data) & IRQ_TYPE_EDGE_BOTH)
>> +		irq_set_handler_locked(data, handle_edge_irq);
>> +	else
>> +		irq_set_handler_locked(data, handle_level_irq);
>> +
>> +	return IRQ_SET_MASK_OK;
>
> That doesn't make any sense. First you store the type 't' in irq data, then
> you query irq data for the type and at the end you tell the core to set the
> type in irq data.
>
>       if (t & IRQ_TYPE_EDGE_BOTH)
>       	...
>       else
>          ...
>       return IRQ_SET_MASK_OK;
>
> does the same, right?

Yes, clearly that is better.  I don't know what we were thinking there.


>
>> +int octeon_irq_ciu3_xlat(struct irq_domain *d,
>
> static ?

To be used in follow-on patch for MSI-X irq_chip driver residing in a 
separate file.

The idea was to not be changing the to/from static multiple times as the 
new patches were merged.  If you think it preferable, I can make them 
all static and then remove the static later, when needed.

>
>> +			 struct device_node *node,
>> +			 const u32 *intspec,
>> +			 unsigned int intsize,
>> +			 unsigned long *out_hwirq,
>> +			 unsigned int *out_type)
>> +{
>> +
>> +void octeon_irq_ciu3_enable(struct irq_data *data)
>
> static
>
>> +void octeon_irq_ciu3_disable(struct irq_data *data)
>> +void octeon_irq_ciu3_ack(struct irq_data *data)
>> +void octeon_irq_ciu3_mask(struct irq_data *data)
>> +void octeon_irq_ciu3_mask_ack(struct irq_data *data)
>> +int octeon_irq_ciu3_set_affinity(struct irq_data *data,
>> +				 const struct cpumask *dest, bool force)
>
> ditto

Same, see above.


>
>> +int octeon_irq_ciu3_mapx(struct irq_domain *d, unsigned int virq,
>> +			 irq_hw_number_t hw, struct irq_chip *chip)
>
> ....

Yep.

>
>> +void octeon_ciu3_mbox_send(int cpu, unsigned int mbox)
>> +{
>> +	struct octeon_ciu3_info *ciu3_info;
>> +	unsigned int intsn;
>> +	union cvmx_ciu3_iscx_w1s isc_w1s;
>> +	u64 isc_w1s_addr;
>> +
>> +	if (WARN_ON_ONCE(mbox >= CIU3_MBOX_PER_CORE))
>> +		return;
>> +
>> +	intsn = octeon_irq_ciu3_mbox_intsn_for_cpu(cpu, mbox);
>> +	ciu3_info = per_cpu(octeon_ciu3_info, cpu);
>> +	isc_w1s_addr = ciu3_info->ciu3_addr + CIU3_ISC_W1S(intsn);
>> +
>> +	isc_w1s.u64 = 0;
>> +	isc_w1s.s.raw = 1;
>> +
>> +	cvmx_write_csr(isc_w1s_addr, isc_w1s.u64);
>> +	cvmx_read_csr(isc_w1s_addr);
>> +}
>> +EXPORT_SYMBOL(octeon_ciu3_mbox_send);
>
> Why need modules that function?

Similar to the MSI-X irq_chip thing, but this time for follow-on 
kexec/kdump patches which may be implemented as a module.

I will remove the EXPORT_SYMBOL().

>
> Thanks,
>
> 	tglx
>