Re: [PATCH 3/4] PCI: rockchip-host: Refactor IRQ handling with info arrays

[Hans Zhang <18255117159@163.com>]

On 2025/5/13 18:40, Krzysztof Wilczyński wrote:
> Hello,
> 
> Thank you for the patch and the proposed changes.
> 
>> Replace repetitive if-conditions for IRQ status checks with structured
>> arrays (`pcie_subsys_irq_info` and `pcie_client_irq_info`) and loop-based
>> logging. This simplifies maintenance and reduces code duplication.
> [...]
>> +static const struct rockchip_irq_info pcie_subsys_irq_info[] = {
>> +	{ PCIE_CORE_INT_PRFPE,
>> +	  "parity error detected while reading from the PNP receive FIFO RAM" },
>> +	{ PCIE_CORE_INT_CRFPE,
>> +	  "parity error detected while reading from the Completion Receive FIFO RAM" },
>> +	{ PCIE_CORE_INT_RRPE,
>> +	  "parity error detected while reading from replay buffer RAM" },
>> +	{ PCIE_CORE_INT_PRFO, "overflow occurred in the PNP receive FIFO" },
>> +	{ PCIE_CORE_INT_CRFO,
>> +	  "overflow occurred in the completion receive FIFO" },
>> +	{ PCIE_CORE_INT_RT, "replay timer timed out" },
>> +	{ PCIE_CORE_INT_RTR,
>> +	  "replay timer rolled over after 4 transmissions of the same TLP" },
>> +	{ PCIE_CORE_INT_PE, "phy error detected on receive side" },
>> +	{ PCIE_CORE_INT_MTR, "malformed TLP received from the link" },
>> +	{ PCIE_CORE_INT_UCR, "Unexpected Completion received from the link" },
>> +	{ PCIE_CORE_INT_FCE,
>> +	  "an error was observed in the flow control advertisements from the other side" },
>> +	{ PCIE_CORE_INT_CT, "a request timed out waiting for completion" },
>> +	{ PCIE_CORE_INT_UTC, "unmapped TC error" },
>> +	{ PCIE_CORE_INT_MMVC, "MSI mask register changes" },
>> +};
>> +
>> +static const struct rockchip_irq_info pcie_client_irq_info[] = {
>> +	{ PCIE_CLIENT_INT_LEGACY_DONE, "legacy done" },
>> +	{ PCIE_CLIENT_INT_MSG, "message done" },
>> +	{ PCIE_CLIENT_INT_HOT_RST, "hot reset" },
>> +	{ PCIE_CLIENT_INT_DPA, "dpa" },
>> +	{ PCIE_CLIENT_INT_FATAL_ERR, "fatal error" },
>> +	{ PCIE_CLIENT_INT_NFATAL_ERR, "Non fatal error" },
>> +	{ PCIE_CLIENT_INT_CORR_ERR, "correctable error" },
>> +	{ PCIE_CLIENT_INT_PHY, "phy" },
>> +};
>> +
>>   static void rockchip_pcie_enable_bw_int(struct rockchip_pcie *rockchip)
>>   {
>>   	u32 status;
>> @@ -411,47 +450,11 @@ static irqreturn_t rockchip_pcie_subsys_irq_handler(int irq, void *arg)
>>   	if (reg & PCIE_CLIENT_INT_LOCAL) {
>>   		dev_dbg(dev, "local interrupt received\n");
>>   		sub_reg = rockchip_pcie_read(rockchip, PCIE_CORE_INT_STATUS);
>> -		if (sub_reg & PCIE_CORE_INT_PRFPE)
>> -			dev_dbg(dev, "parity error detected while reading from the PNP receive FIFO RAM\n");
>> -
>> -		if (sub_reg & PCIE_CORE_INT_CRFPE)
>> -			dev_dbg(dev, "parity error detected while reading from the Completion Receive FIFO RAM\n");
>> -
>> -		if (sub_reg & PCIE_CORE_INT_RRPE)
>> -			dev_dbg(dev, "parity error detected while reading from replay buffer RAM\n");
>> -
>> -		if (sub_reg & PCIE_CORE_INT_PRFO)
>> -			dev_dbg(dev, "overflow occurred in the PNP receive FIFO\n");
>> -
>> -		if (sub_reg & PCIE_CORE_INT_CRFO)
>> -			dev_dbg(dev, "overflow occurred in the completion receive FIFO\n");
>> -
>> -		if (sub_reg & PCIE_CORE_INT_RT)
>> -			dev_dbg(dev, "replay timer timed out\n");
>> -
>> -		if (sub_reg & PCIE_CORE_INT_RTR)
>> -			dev_dbg(dev, "replay timer rolled over after 4 transmissions of the same TLP\n");
>> -
>> -		if (sub_reg & PCIE_CORE_INT_PE)
>> -			dev_dbg(dev, "phy error detected on receive side\n");
>>   
>> -		if (sub_reg & PCIE_CORE_INT_MTR)
>> -			dev_dbg(dev, "malformed TLP received from the link\n");
>> -
>> -		if (sub_reg & PCIE_CORE_INT_UCR)
>> -			dev_dbg(dev, "Unexpected Completion received from the link\n");
>> -
>> -		if (sub_reg & PCIE_CORE_INT_FCE)
>> -			dev_dbg(dev, "an error was observed in the flow control advertisements from the other side\n");
>> -
>> -		if (sub_reg & PCIE_CORE_INT_CT)
>> -			dev_dbg(dev, "a request timed out waiting for completion\n");
>> -
>> -		if (sub_reg & PCIE_CORE_INT_UTC)
>> -			dev_dbg(dev, "unmapped TC error\n");
>> -
>> -		if (sub_reg & PCIE_CORE_INT_MMVC)
>> -			dev_dbg(dev, "MSI mask register changes\n");
>> +		for (int i = 0; i < ARRAY_SIZE(pcie_subsys_irq_info); i++) {
>> +			if (sub_reg & pcie_subsys_irq_info[i].bit)
>> +				dev_dbg(dev, "%s\n", pcie_subsys_irq_info[i].msg);
>> +		}
>>   
>>   		rockchip_pcie_write(rockchip, sub_reg, PCIE_CORE_INT_STATUS);
>>   	} else if (reg & PCIE_CLIENT_INT_PHY) {
>> @@ -473,29 +476,12 @@ static irqreturn_t rockchip_pcie_client_irq_handler(int irq, void *arg)
>>   	u32 reg;
>>   
>>   	reg = rockchip_pcie_read(rockchip, PCIE_CLIENT_INT_STATUS);
>> -	if (reg & PCIE_CLIENT_INT_LEGACY_DONE)
>> -		dev_dbg(dev, "legacy done interrupt received\n");
>> -
>> -	if (reg & PCIE_CLIENT_INT_MSG)
>> -		dev_dbg(dev, "message done interrupt received\n");
>>   
>> -	if (reg & PCIE_CLIENT_INT_HOT_RST)
>> -		dev_dbg(dev, "hot reset interrupt received\n");
>> -
>> -	if (reg & PCIE_CLIENT_INT_DPA)
>> -		dev_dbg(dev, "dpa interrupt received\n");
>> -
>> -	if (reg & PCIE_CLIENT_INT_FATAL_ERR)
>> -		dev_dbg(dev, "fatal error interrupt received\n");
>> -
>> -	if (reg & PCIE_CLIENT_INT_NFATAL_ERR)
>> -		dev_dbg(dev, "non fatal error interrupt received\n");
>> -
>> -	if (reg & PCIE_CLIENT_INT_CORR_ERR)
>> -		dev_dbg(dev, "correctable error interrupt received\n");
>> -
>> -	if (reg & PCIE_CLIENT_INT_PHY)
>> -		dev_dbg(dev, "phy interrupt received\n");
>> +	for (int i = 0; i < ARRAY_SIZE(pcie_client_irq_info); i++) {
>> +		if (reg & pcie_client_irq_info[i].bit)
>> +			dev_dbg(dev, "%s interrupt received\n",
>> +				pcie_client_irq_info[i].msg);
> 
> Why do you think that this is better?
> 
> Other patches in this series seem sensible, but this one does not stands
> out as something that needs to be changed.
> 

Dear Krzysztof

Thank you very much for your reply.

In the interrupt handling function: rockchip_pcie_subsys_irq_handler, 
personally, I think the amount of code can be simplified and it looks 
more concise.

There are many repetitive "interrupt received" print messages in the 
interrupt handling function: rockchip_pcie_client_irq_handler. My 
original intention was to simplify it.

If you think it's not necessary, I will drop this patch.

Best regards,
Hans