Re: [PATCH net-next V2 2/3] net: stmmac: dwmac-loongson: Add new multi-chan IP core support

[Yanteng Si <si.yanteng@linux.dev>]

在 4/21/25 12:20 PM, Huacai Chen 写道:
> Hi, Yanteng,
>
> On Mon, Apr 21, 2025 at 10:04 AM Yanteng Si <si.yanteng@linux.dev> wrote:
>>
>> 在 4/16/25 10:41 PM, Huacai Chen 写道:
>>> Add a new multi-chan IP core (0x12) support which is used in Loongson-
>>> 2K3000/Loongson-3B6000M. Compared with the 0x10 core, the new 0x12 core
>>> reduces channel numbers from 8 to 4, but checksum is supported for all
>>> channels.
>>>
>>> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
>>> Tested-by: Henry Chen <chenx97@aosc.io>
>>> Tested-by: Biao Dong <dongbiao@loongson.cn>
>>> Signed-off-by: Baoqi Zhang <zhangbaoqi@loongson.cn>
>>> Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
>>> ---
>>>    .../ethernet/stmicro/stmmac/dwmac-loongson.c  | 62 +++++++++++--------
>>>    1 file changed, 37 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
>>> index 2fb7a137b312..57917f26ab4d 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
>>> @@ -68,10 +68,11 @@
>>>
>>>    #define PCI_DEVICE_ID_LOONGSON_GMAC 0x7a03
>>>    #define PCI_DEVICE_ID_LOONGSON_GNET 0x7a13
>>> -#define DWMAC_CORE_LS_MULTICHAN      0x10    /* Loongson custom ID */
>>> -#define CHANNEL_NUM                  8
>>> +#define DWMAC_CORE_MULTICHAN_V1      0x10    /* Loongson custom ID 0x10 */
>>> +#define DWMAC_CORE_MULTICHAN_V2      0x12    /* Loongson custom ID 0x12 */
>>>
>>>    struct loongson_data {
>>> +     u32 multichan;
>> In order to make the logic clearer, I suggest splitting this patch.：
>>
>>
>> 2/4  Add multichan for loongson_data
>>
>> 3/4 Add new multi-chan IP core support
> I don't think the patch is unclear now, the multichan flag is really a
> combination of DWMAC_CORE_MULTICHAN_V1 and DWMAC_CORE_MULTICHAN_V2.
OK, please describe this code modification in the commit message.
>
>>
>>>        u32 loongson_id;
>>>        struct device *dev;
>>>    };
>>> @@ -119,18 +120,29 @@ static void loongson_default_data(struct pci_dev *pdev,
>>>        plat->dma_cfg->pbl = 32;
>>>        plat->dma_cfg->pblx8 = true;
>>>
>>> -     if (ld->loongson_id == DWMAC_CORE_LS_MULTICHAN) {
>>> -             plat->rx_queues_to_use = CHANNEL_NUM;
>>> -             plat->tx_queues_to_use = CHANNEL_NUM;
>>> +     switch (ld->loongson_id) {
>>> +     case DWMAC_CORE_MULTICHAN_V1:
>> How about adding some comments? For example:
>>
>> case DWMAC_CORE_MULTICHAN_V1:   /* 2K2000 */
>> case DWMAC_CORE_MULTICHAN_V2:   /* 2K3000 and 3B6000M */
> Do you know why we deprecate PRID (a.k.a SOC name) detection and
> prefer CPUCFG detection in cpu_probe()? Because SOC-types and function
> features are orthogonal, they should not be strictly bound. There will
> be other SOCs using MULTICHAN_V1 or MULTICHAN_V2, should we update the
> comments every time when a new SOC publishes? There may also be one
> SOC with different IP-cores, then how to comment on this case?

In my opinion, having something is better than having nothing.

I've come up with a way to address your concerns:


case DWMAC_CORE_MULTICHAN_V1:   /* 2K2000 ... */
case DWMAC_CORE_MULTICHAN_V2:   /* 2K3000, 3B6000M ...*/


These limited comments are very friendly to developers outside

Loongson Company. Of course, I don't want to discuss this matter

any further. If you don't want to add code comments, I won't object

to this patch because of that.

>
>> ...
>>
>>> +             ld->multichan = 1;
>>> +             plat->rx_queues_to_use = 8;
>>> +             plat->tx_queues_to_use = 8;
>>>
>>>                /* Only channel 0 supports checksum,
>>>                 * so turn off checksum to enable multiple channels.
>>>                 */
>>> -             for (int i = 1; i < CHANNEL_NUM; i++)
>>> +             for (int i = 1; i < 8; i++)
>>>                        plat->tx_queues_cfg[i].coe_unsupported = 1;
>>> -     } else {
>>> +
>>> +             break;
>>> +     case DWMAC_CORE_MULTICHAN_V2:
>>> +             ld->multichan = 1;
>>> +             plat->rx_queues_to_use = 4;
>>> +             plat->tx_queues_to_use = 4;
>>> +             break;
>>> +     default:
>>> +             ld->multichan = 0;
>>>                plat->tx_queues_to_use = 1;
>>>                plat->rx_queues_to_use = 1;
>>> +             break;
>>>        }
>>>    }
>>>
>>> @@ -328,14 +340,14 @@ static struct mac_device_info *loongson_dwmac_setup(void *apriv)
>>>                return NULL;
>>>
>>>        /* The Loongson GMAC and GNET devices are based on the DW GMAC
>>> -      * v3.50a and v3.73a IP-cores. But the HW designers have changed the
>>> -      * GMAC_VERSION.SNPSVER field to the custom 0x10 value on the
>>> -      * network controllers with the multi-channels feature
>>> +      * v3.50a and v3.73a IP-cores. But the HW designers have changed
>>> +      * the GMAC_VERSION.SNPSVER field to the custom 0x10/0x12 value
>>> +      * on the network controllers with the multi-channels feature
>>>         * available to emphasize the differences: multiple DMA-channels,
>>>         * AV feature and GMAC_INT_STATUS CSR flags layout. Get back the
>>>         * original value so the correct HW-interface would be selected.
>>>         */
>>> -     if (ld->loongson_id == DWMAC_CORE_LS_MULTICHAN) {
>>> +     if (ld->multichan) {
>>>                priv->synopsys_id = DWMAC_CORE_3_70;
>>>                *dma = dwmac1000_dma_ops;
>>>                dma->init_chan = loongson_dwmac_dma_init_channel;
>>> @@ -356,13 +368,13 @@ static struct mac_device_info *loongson_dwmac_setup(void *apriv)
>>>        if (mac->multicast_filter_bins)
>>>                mac->mcast_bits_log2 = ilog2(mac->multicast_filter_bins);
>>>
>>> -     /* Loongson GMAC doesn't support the flow control. LS2K2000
>>> -      * GNET doesn't support the half-duplex link mode.
>>> +     /* Loongson GMAC doesn't support the flow control. Loongson GNET
>>> +      * without multi-channel doesn't support the half-duplex link mode.
>>>         */
>>>        if (pdev->device == PCI_DEVICE_ID_LOONGSON_GMAC) {
>>>                mac->link.caps = MAC_10 | MAC_100 | MAC_1000;
>>>        } else {
>>> -             if (ld->loongson_id == DWMAC_CORE_LS_MULTICHAN)
>>> +             if (ld->multichan)
>>>                        mac->link.caps = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
>>>                                         MAC_10 | MAC_100 | MAC_1000;
>>>                else
>>> @@ -391,9 +403,11 @@ static int loongson_dwmac_msi_config(struct pci_dev *pdev,
>>>                                     struct plat_stmmacenet_data *plat,
>>>                                     struct stmmac_resources *res)
>>>    {
>>> -     int i, ret, vecs;
>>> +     int i, ch_num, ret, vecs;
>>>
>>> -     vecs = roundup_pow_of_two(CHANNEL_NUM * 2 + 1);
>>> +     ch_num = min(plat->tx_queues_to_use, plat->rx_queues_to_use);
>> I'm curious. Will there still be hardware with RX not equal to TX in the
>> future?
> Currently rx queue number is equal to tx queue number, but
> min(plat->tx_queues_to_use, plat->rx_queues_to_use) is still the best
> solution. If not, which one should be used here? tx_queues_to_use or
> rx_queues_to_use?

All right.


Thanks,

Yanteng

>
> Huacai
>
>>
>> Thanks,
>>
>> Yanteng
>>
>>