From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx.socionext.com (mx.socionext.com [202.248.49.38]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 665FC2046B2; Mon, 3 Feb 2025 11:55:53 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=202.248.49.38 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1738583755; cv=none; b=ijIDk3W0GaFHg3d3jlR82ZlYlOdYHJeKrcc2er8owbnyANgMC7+VpL3/lbJDWrsBrsiF0HUwhjl/gdlAMoHwyer4cMO1YMjHgxPfl/kKCcinie7Ep4am6At+/CAOqRikrspvVZ54WUEeP+lf7C583STRURQ4rlp3we7Lqk0+IQE= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1738583755; c=relaxed/simple; bh=xLxCbpoA/4X6rIaPV3F//sTbwXjhJIN657Zw92ewmUI=; h=Message-ID:Date:MIME-Version:Subject:From:To:Cc:References: In-Reply-To:Content-Type; b=u9tyVQQ4wrW+Ru5T92QgJV5WpncN9Y9phzRXqIeVsAjP1tXmUb4BQJ0Ry+t4bru/h4oqSJ4++UscYEYEKZn4gplinUQecPzypS1ok9VpYkNhLJHTBFz2ACHQ15J+tGfnopdm6HfGMNfBaK0nOsfmM79Kq92y7GyU/h4nZydKG8g= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=socionext.com; spf=pass smtp.mailfrom=socionext.com; arc=none smtp.client-ip=202.248.49.38 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=socionext.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=socionext.com Received: from unknown (HELO kinkan2-ex.css.socionext.com) ([172.31.9.52]) by mx.socionext.com with ESMTP; 03 Feb 2025 20:55:52 +0900 Received: from mail.mfilter.local (mail-arc01.css.socionext.com [10.213.46.36]) by kinkan2-ex.css.socionext.com (Postfix) with ESMTP id 475BF208E5DC; Mon, 3 Feb 2025 20:55:52 +0900 (JST) Received: from iyokan2.css.socionext.com ([172.31.9.53]) by m-FILTER with ESMTP; Mon, 3 Feb 2025 20:55:52 +0900 Received: from [10.212.246.222] (unknown [10.212.246.222]) by iyokan2.css.socionext.com (Postfix) with ESMTP id 672281A9; Mon, 3 Feb 2025 20:55:51 +0900 (JST) Message-ID: <2b22ecba-93b8-479c-ab16-42cf24cf2c06@socionext.com> Date: Mon, 3 Feb 2025 20:55:51 +0900 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH net v4 3/3] net: stmmac: Specify hardware capability value when FIFO size isn't specified From: Kunihiko Hayashi To: "Russell King (Oracle)" Cc: Guenter Roeck , Alexandre Torgue , Jose Abreu , Andrew Lunn , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Maxime Coquelin , Yanteng Si , Furong Xu <0x1207@gmail.com>, Joao Pinto , netdev@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org References: <20250127013820.2941044-1-hayashi.kunihiko@socionext.com> <20250127013820.2941044-4-hayashi.kunihiko@socionext.com> <4e98f967-f636-46fb-9eca-d383b9495b86@roeck-us.net> <905127b5-96c8-4866-8f69-d9d8a7091c99@socionext.com> Content-Language: en-US In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 2025/02/03 20:32, Kunihiko Hayashi wrote: > Hi, > > On 2025/02/03 18:23, Russell King (Oracle) wrote: >> On Mon, Feb 03, 2025 at 11:45:05AM +0900, Kunihiko Hayashi wrote: >>> Hi all, >>> >>> On 2025/02/02 5:35, Russell King (Oracle) wrote: >>>> On Sat, Feb 01, 2025 at 11:14:41AM -0800, Guenter Roeck wrote: >>>>> Hi, >>>>> >>>>> On Mon, Jan 27, 2025 at 10:38:20AM +0900, Kunihiko Hayashi wrote: >>>>>> When Tx/Rx FIFO size is not specified in advance, the driver >> checks if >>>>>> the value is zero and sets the hardware capability value in >> functions >>>>>> where that value is used. >>>>>> >>>>>> Consolidate the check and settings into function stmmac_hw_init() >> and >>>>>> remove redundant other statements. >>>>>> >>>>>> If FIFO size is zero and the hardware capability also doesn't have >>>> upper >>>>>> limit values, return with an error message. >>>>>> >>>>>> Signed-off-by: Kunihiko Hayashi >>>>> >>>>> This patch breaks qemu's stmmac emulation, for example for >>>>> npcm750-evb. The error message is: >>>>> stmmaceth f0804000.eth: Can't specify Rx FIFO size >>> >>> Sorry for inconvenience. >>> >>>> Interesting. I looked at QEMU to see whether anything in the Debian >>>> stable version of QEMU might possibly have STMMAC emulation, but >>>> drew a blank... Even trying to find where in QEMU it emulates the >>>> STMMAC. I do see that it does include this, so maybe I can use that >>>> to test some of my stmmac changes. Thanks! >>>> >>>>> The setup function called for the emulated hardware is >>>> dwmac1000_setup(). >>>>> That function does not set the DMA rx or tx fifo size. >>>>> >>>>> At the same time, the rx and tx fifo size is not provided in the >>>>> devicetree file (nuvoton-npcm750.dtsi), so the failure is obvious. >>>>> >>>>> I understand that the real hardware may be based on a more recent >>>>> version of the DWMAC IP which provides the DMA tx/rx fifo size, but >>>>> I do wonder: Are the benefits of this patch so substantial that it >>>>> warrants breaking the qemu emulation of this network interface > >>>> Please see my message sent a while back on an earlier revision of this >>>> patch series. I reviewed the stmmac driver for the fifo sizes and >>>> documented what I found. >>>> >>>> https://lore.kernel.org/r/Z4_ZilVFKacuAUE8@shell.armlinux.org.uk >>>> >>>> To save clicking on the link, I'll reproduce the relevant part below. >>>> It appears that dwmac1000 has no way to specify the FIFO size, and >>>> thus would have priv->dma_cap.rx_fifo_size and >>>> priv->dma_cap.tx_fifo_size set to zero. >>>> >>>> Given the responses, I'm now of the opinion that the patch series is >>>> wrong, and probably should be reverted - I never really understood >>>> the motivation why the series was necessary. It seemed to me to be a >>>> "wouldn't it be nice if" series rather than something that is >>>> functionally necessary. >>>> >>>> >>>> Here's the extract from my previous email: >>>> >>>> Now looking at the defintions: >>>> >>>> drivers/net/ethernet/stmicro/stmmac/dwmac4.h:#define >> GMAC_HW_RXFIFOSIZE >>>> GENMASK(4, 0) >>>> drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h:#define >>>> XGMAC_HWFEAT_RXFIFOSIZE GENMASK(4, 0) >>>> >>>> So there's a 5-bit bitfield that describes the receive FIFO size for >>>> these two MACs. Then we have: >>>> >>>> drivers/net/ethernet/stmicro/stmmac/common.h:#define >>>> DMA_HW_FEAT_RXFIFOSIZE 0x00080000 /* Rx FIFO > 2048 Bytes */ >>>> >>>> which is used here: >>>> >>>> drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c: >>>> dma_cap->rxfifo_over_2048 = (hw_cap & DMA_HW_FEAT_RXFIFOSIZE) >> 19; >>>> >>>> which is only used to print a Y/N value in a debugfs file, otherwise >>>> having no bearing on driver behaviour. >>>> >>>> So, I suspect MACs other than xgmac2 or dwmac4 do not have the ability >>>> to describe the hardware FIFO sizes in hardware, thus why there's the >>>> override and no checking of what the platform provided - and doing so >>>> would break the driver. This is my interpretation from the code alone. >>>> >>> >>> The {tx,rx}_queus_to_use are referenced in stmmac_ethtool.c, >> stmmac_tc.c, >>> and stmmac_selftests.c as the number of queues, so I've thought that >>> these variables should not be non-zero. >> >> Huh? We're talking about {tx,rx}_fifo_size, not _queues_to_use. > > Sorry, this topic is just about {tx,rx}_fifo_size. > Above comments are not needed here. > > These value are only referenced in stmmac_main.c and stmmac_selftest.c. > As far as I can see, there is no usage that requires the values to be > non-zero. Comment to myself: This is incorrect as it depends on how the values are used. Thank you, --- Best Regards Kunihiko Hayashi