Re: [PATCH v2] Add support for Realtek RTL838x/RTL839x switch SoCs

[Bert Vermeulen <bert@biot.com>]

On 12/23/20 5:18 PM, Marc Zyngier wrote:

Marc,

Thanks for reviewing. We will rework as needed, however:

> On Wed, 23 Dec 2020 15:06:24 +0000,
> Bert Vermeulen <bert@biot.com> wrote:
[...]

>> +/* Interrupt numbers/bits */
>> +#define RTL8380_IRQ_UART0		31
>> +#define RTL8380_IRQ_UART1		30
>> +#define RTL8380_IRQ_TC0			29
>> +#define RTL8380_IRQ_TC1			28
>> +#define RTL8380_IRQ_OCPTO		27
>> +#define RTL8380_IRQ_HLXTO		26
>> +#define RTL8380_IRQ_SLXTO		25
>> +#define RTL8380_IRQ_NIC			24
>> +#define RTL8380_IRQ_GPIO_ABCD		23
>> +#define RTL8380_IRQ_GPIO_EFGH		22
>> +#define RTL8380_IRQ_RTC			21
>> +#define RTL8380_IRQ_SWCORE		20
>> +#define RTL8380_IRQ_WDT_IP1		19
>> +#define RTL8380_IRQ_WDT_IP2		18
> 
> Why do we need any of this? The mapping should be explicit in the DT.
> 
>> +
>> +/* Global Interrupt Mask Register */
>> +#define RTL8380_ICTL_GIMR	0x00
>> +/* Global Interrupt Status Register */
>> +#define RTL8380_ICTL_GISR	0x04
>> +
>> +/* Cascaded interrupts */
>> +#define RTL8380_CPU_IRQ_SHARED0		(MIPS_CPU_IRQ_BASE + 2)
>> +#define RTL8380_CPU_IRQ_UART		(MIPS_CPU_IRQ_BASE + 3)
>> +#define RTL8380_CPU_IRQ_SWITCH		(MIPS_CPU_IRQ_BASE + 4)
>> +#define RTL8380_CPU_IRQ_SHARED1		(MIPS_CPU_IRQ_BASE + 5)
>> +#define RTL8380_CPU_IRQ_EXTERNAL	(MIPS_CPU_IRQ_BASE + 6)
>> +#define RTL8380_CPU_IRQ_COUNTER		(MIPS_CPU_IRQ_BASE + 7)
>> +
>> +
>> +/* Interrupt routing register */
>> +#define RTL8380_IRR0		0x08
>> +#define RTL8380_IRR1		0x0c
>> +#define RTL8380_IRR2		0x10
>> +#define RTL8380_IRR3		0x14
>> +
>> +/* Cascade map */
>> +#define RTL8380_IRQ_CASCADE_UART0	2
>> +#define RTL8380_IRQ_CASCADE_UART1	1
>> +#define RTL8380_IRQ_CASCADE_TC0		5
>> +#define RTL8380_IRQ_CASCADE_TC1		1
>> +#define RTL8380_IRQ_CASCADE_OCPTO	1
>> +#define RTL8380_IRQ_CASCADE_HLXTO	1
>> +#define RTL8380_IRQ_CASCADE_SLXTO	1
>> +#define RTL8380_IRQ_CASCADE_NIC		4
>> +#define RTL8380_IRQ_CASCADE_GPIO_ABCD	4
>> +#define RTL8380_IRQ_CASCADE_GPIO_EFGH	4
>> +#define RTL8380_IRQ_CASCADE_RTC		4
>> +#define RTL8380_IRQ_CASCADE_SWCORE	3
>> +#define RTL8380_IRQ_CASCADE_WDT_IP1	4
>> +#define RTL8380_IRQ_CASCADE_WDT_IP2	5
>> +
>> +/* Pack cascade map into interrupt routing registers */
>> +#define RTL8380_IRR0_SETTING (\
>> +	(RTL8380_IRQ_CASCADE_UART0	<< 28) | \
>> +	(RTL8380_IRQ_CASCADE_UART1	<< 24) | \
>> +	(RTL8380_IRQ_CASCADE_TC0	<< 20) | \
>> +	(RTL8380_IRQ_CASCADE_TC1	<< 16) | \
>> +	(RTL8380_IRQ_CASCADE_OCPTO	<< 12) | \
>> +	(RTL8380_IRQ_CASCADE_HLXTO	<< 8)  | \
>> +	(RTL8380_IRQ_CASCADE_SLXTO	<< 4)  | \
>> +	(RTL8380_IRQ_CASCADE_NIC	<< 0))
>> +#define RTL8380_IRR1_SETTING (\
>> +	(RTL8380_IRQ_CASCADE_GPIO_ABCD	<< 28) | \
>> +	(RTL8380_IRQ_CASCADE_GPIO_EFGH	<< 24) | \
>> +	(RTL8380_IRQ_CASCADE_RTC	<< 20) | \
>> +	(RTL8380_IRQ_CASCADE_SWCORE	<< 16))
>> +#define RTL8380_IRR2_SETTING	0
>> +#define RTL8380_IRR3_SETTING	0

[...]

>> +	/* Set up interrupt routing */
>> +	writel(RTL8380_IRR0_SETTING, REG(RTL8380_IRR0));
>> +	writel(RTL8380_IRR1_SETTING, REG(RTL8380_IRR1));
>> +	writel(RTL8380_IRR2_SETTING, REG(RTL8380_IRR2));
>> +	writel(RTL8380_IRR3_SETTING, REG(RTL8380_IRR3));
> 
> What is this doing?

It's fairly evident considering the comments -- routing of secondary IRQs 
onto the CPU IRQs. But as to packing this into DTS I'm not sure.

DTS syntax being what it is, this would inevitably get more complex and 
harder to understand. Do you have an example where this is done in a better way?

thanks,


-- 
Bert Vermeulen
bert@biot.com