Re: [PATCH v2 2/2] can: loongson_canfd: Add RXDMA support

[Vincent Mailhol <mailhol@kernel.org>]

On 15/06/2026 at 03:35, Binbin Zhou wrote:
> Hi Vincent:
> 
> Thanks for your detailed review and sorry for my late reply.
> 
> On Mon, Jun 8, 2026 at 8:13 PM Vincent Mailhol <mailhol@kernel.org> wrote:
>>
>> On 08/06/2026 at 10:49, Binbin Zhou wrote:
>>> Add optional DMA support for RX path using the Loongson APB CMC DMA
>>> engine. When a DMA channel is successfully requested, the driver:
>>>
>>> - Uses DMA cyclic transfers to write incoming CAN frames directly to
>>>   a coherent DMA buffer
>>> - Replaces RXBNEI (RX buffer not empty interrupt) with DMADI (DMA
>>>   done interrupt)
>>> - Dynamically switches between DMA and PIO modes based on channel
>>>   availability
>>>
>>> This significantly reduces CPU intervention under high RX load,
>>> especially beneficial for CAN FD at higher data rates.
>>>
>>> Co-developed-by: Bingxiong Li <libingxiong@loongson.cn>
>>> Signed-off-by: Bingxiong Li <libingxiong@loongson.cn>
>>> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
>>> ---
>>
>> Please check the W=2 warnings:
> 
> OK. I usually compile with `W=1` and didn't notice these issues.

The W=2 used to be too spammy and people simply ignored it. I went
through a quest to reduce that noise. See

  660e899103e2 ("kbuild: remove gcc's -Wtype-limits")

with this, the output is less bloated and I think a bit more useful. Not
that you need to systematically address all warnings, but at least,
review the list.

>>   $ make W=12 drivers/net/can/loongson_canfd.o
>>     DESCEND objtool
>>     INSTALL libsubcmd_headers
>>     CC      drivers/net/can/loongson_canfd.o
>>   drivers/net/can/loongson_canfd.c:1917:9: warning: missing initializer for field 'driver_data' of 'const struct acpi_device_id' [-Wmissing-field-initializers]
>>    1917 |         { "LOON0015" },
>>         |         ^
>>   In file included from ./include/linux/acpi.h:16,
>>                    from drivers/net/can/loongson_canfd.c:8:
>>   ./include/linux/mod_devicetable.h:219:24: note: 'driver_data' declared here
>>     219 |         kernel_ulong_t driver_data;
>>         |                        ^~~~~~~~~~~
>>   drivers/net/can/loongson_canfd.c: In function 'loongson_canfd_start_xmit':
>>   drivers/net/can/loongson_canfd.c:1116:13: warning: 'buf_id' may be used uninitialized [-Wmaybe-uninitialized]
>>    1116 |         u32 buf_id, tx_stat, i = 0;
>>         |             ^~~~~~
>>   drivers/net/can/loongson_canfd.c:1116:13: note: 'buf_id' was declared here
>>    1116 |         u32 buf_id, tx_stat, i = 0;
>>         |             ^~~~~~
>>   In function 'loongson_canfd_insert_frame',
>>       inlined from 'loongson_canfd_start_xmit' at drivers/net/can/loongson_canfd.c:1144:7:
>>   drivers/net/can/loongson_canfd.c:1079:15: warning: 'meta1' may be used uninitialized [-Wmaybe-uninitialized]
>>    1079 |         meta1 |= FIELD_PREP(REG_FRAME_META1_DLC, can_fd_len2dlc(cf->len));
>>         |               ^~
>>   drivers/net/can/loongson_canfd.c: In function 'loongson_canfd_start_xmit':
>>   drivers/net/can/loongson_canfd.c:1052:20: note: 'meta1' was declared here
>>    1052 |         u32 meta0, meta1;
>>         |                    ^~~~~
>>   drivers/net/can/loongson_canfd.c: In function 'loongson_canfd_probe':
>>   drivers/net/can/loongson_canfd.c:1807:13: warning: 'ret' may be used uninitialized [-Wmaybe-uninitialized]
>>    1807 |         int ret, irq;
>>         |             ^~~
>>   drivers/net/can/loongson_canfd.c:1807:13: note: 'ret' was declared here
>>    1807 |         int ret, irq;
>>         |             ^~~
>>   drivers/net/can/loongson_canfd.c: At top level:
>>   drivers/net/can/loongson_canfd.c:47:9: warning: macro 'LOONGSON_CANFD_RX_FR_CNT' is not used [-Wunused-macros]
>>      47 | #define LOONGSON_CANFD_RX_FR_CNT        0x50    /* Receive Message Count Register */
>>         |         ^~~~~~~~~~~~~~~~~~~~~~~~
> 
> Regarding the definitions of these controller registers, even though
> they aren’t currently referenced, can they be retained to make the
> code seem more complete?

Only keep the registers for features which are not yet implemented. For
the register which are already handled in a different way without
relying on the macro, please remove.

I can think, for example, of:

  LOONGSON_CANFD_TX_DATA_10

You handled the tx data in a for loop and this macro is purely dead and
do not serve anymore documentation purpose. The only thing it does now
is to add confusion.

(...)

>>>  config CAN_LOONGSON_CANFD
>>>       tristate "Loongson CAN-FD controller"
>>> -     depends on HAS_IOMEM || COMPILE_TEST
>>> +     depends on HAS_IOMEM && (LOONGSON2_APB_CMC_DMA || COMPILE_TEST)
>>
>> The logic is odd here. If your driver can be COMPILE_TESTed without
>> HAS_IOMEM, then patch 1 should be:
>>
>>         depends on HAS_IOMEM || COMPILE_TEST
>>
>> and patch 2 should be:
>>
>>         depends on (HAS_IOMEM && LOONGSON2_APB_CMC_DMA) || COMPILE_TEST
>>
>> If your driver need HAS_IOMEM even for a compile test, then patch 1
>> should be:
>>
>>         depends on HAS_IOMEM
>>
>> and patch 2 should be:
>>
>>         depends on HAS_IOMEM && (LOONGSON2_APB_CMC_DMA || COMPILE_TEST)
> 
> For me, the 2nd way is correct.

Ack.

Just for confirmation, do you think that your device will ever be used
in production on a board which would not have LOONGSON2_APB_CMC_DMA? If
yes, it could make sense to add the RXDMA as a separate module.
Otherwise, it is good as-is.

>>
>> Here, you are doing a weird mix.
>>
>>>       select REGMAP_MMIO
>>>       help
>>>         This is a canfd driver switch for the Loongson platform,


Yours sincerely,
Vincent Mailhol