From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id B7313C531DC for ; Tue, 20 Aug 2024 18:32:35 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:Cc:List-Subscribe: List-Help:List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To: Content-Type:MIME-Version:References:Message-ID:Subject:To:Date:From:Reply-To :Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=C07kwkQPZHrXmRCk/7LVIjuwNmyBogh+obKLGkkmYFE=; b=JNZUg9v2g56mcohIazYPLE+byZ b3L/obGqzwQcDEgscWUAIW+DocVraslqiOOLuchFr6RhJ1E5V5spbz3l+8AdED8Ot96QzIUFPG8uV 1emZEBJZoOjsiZJmdIy83K6UN4PK5xZ0+2yp1b/Fv3Ff7ailiDLWgCkBo7XLdpCgh8z8N6ffxrVdn Qx07HDAS0tyIE++BE29L9SJkyaeHZ+NLpn864dcMxm3DT12AiiYTQ89TqEfwbe2qU38RhZAhcyHDG a9/UxVQMNURi2uoYLbhRU7dqiEUODcTvwGBdRF9FqpxbR8KfdwY83+y2hssQHmQx+zG8kXjqOulnf upgXEfag==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sgTeb-00000006KIT-02HF; Tue, 20 Aug 2024 18:32:25 +0000 Received: from mail-ej1-x62b.google.com ([2a00:1450:4864:20::62b]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sgTdn-00000006K33-0Wzy for linux-arm-kernel@lists.infradead.org; Tue, 20 Aug 2024 18:31:36 +0000 Received: by mail-ej1-x62b.google.com with SMTP id a640c23a62f3a-a7a94478a4eso1183910466b.1 for ; Tue, 20 Aug 2024 11:31:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=google; t=1724178693; x=1724783493; darn=lists.infradead.org; h=in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:cc:to:date:from:from:to:cc :subject:date:message-id:reply-to; bh=C07kwkQPZHrXmRCk/7LVIjuwNmyBogh+obKLGkkmYFE=; b=RzzwjgnS5nQCEkL5XwwzMbuqYiZaQFepBiC7GzGrC2Bf0Y2SorvuTDparsmlc6ZN0n Q1EdJIVcKEBDXu+rBLLI/nxPL4x26SzKZeXkndZlgrITIbwU1S5GDX3QuqaeyOJe5SWV U7ZKuh5vMI18m2MrbeDTY5tWJxZz48Ij35ZnW9wGf7TkQiw761z+T1I37fW99h0ygW8e +/X5qW5Zx2Dyr7m/6iZ622j8RSZJPrQBHkRC10PyL64Twnp6HttvXgm2ZtbsKUSgCdYf TqMxmzX76WgpPYdOtyxTiZ6/LZH/yVjz2MySetiJTh49FYshWJT4XjKntEhspjYVN+uq Uc9Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1724178693; x=1724783493; h=in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:cc:to:date:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=C07kwkQPZHrXmRCk/7LVIjuwNmyBogh+obKLGkkmYFE=; b=o7XFNiT0uuZSwsdoAwReux95Z9095RerugIQ8d7oa0UZErk0BK9ymP1ePWjUxVT0Tn fmNnTTwOz0Zza2gzHXkFq+B4+TOWZDhBa1RwG0yt137qro6xYRKOwrbgXutO79Tmg4+n tOy7Zn0liNpFlkt0HNEzkgFNOVCBJFbmsp08bo/8fEMQ9zaLFinE4Wjw2N0vPMThgpZf FtYTW+38EcfsBIrqDhMDPw60uz1oc4Pe773shH6yRwCuD/425iSVd8bgb23LUZLkrckl S5LR7ozCUNxmJgzgd31YV74MdEQ356DndfnIVIZl4qvY8kX7jtkCLwpF1YimpsnmyxG5 PZig== X-Forwarded-Encrypted: i=1; AJvYcCWvsh9N348Nv25LWVcCoYEzLk/mg8ovicwq3ETJCn5cJnT+Xrm+DF4TISUU/pG2gr//9tTyku3XND+yGQCStmw1@lists.infradead.org X-Gm-Message-State: AOJu0Yxcv8n7t1I3U+iH/t4sN52yIM8YU/x163x18IhA5K/m6u9eja3w gqGXjgCJT2/Jjd4gzA/eJaknfeAiPXVq4ThhWnT+WnNN2qDO4K6ag3OVPUbjBhE= X-Google-Smtp-Source: AGHT+IGOs6VYvfV4LBlh3n6XPG9LIKhdxKViotPxcCB+fUrUkrU6+w914efXSB1ZJv8NT+vGMTojLg== X-Received: by 2002:a17:907:1b13:b0:a86:6a9a:d719 with SMTP id a640c23a62f3a-a866a9adcddmr29207866b.29.1724178693023; Tue, 20 Aug 2024 11:31:33 -0700 (PDT) Received: from localhost ([87.13.33.30]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-a838396940fsm796113566b.189.2024.08.20.11.31.32 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 20 Aug 2024 11:31:32 -0700 (PDT) From: Andrea della Porta X-Google-Original-From: Andrea della Porta Date: Tue, 20 Aug 2024 20:31:38 +0200 To: Andrew Lunn Subject: Re: [PATCH 10/11] net: macb: Add support for RP1's MACB variant Message-ID: Mail-Followup-To: Andrew Lunn , Andrea della Porta , Michael Turquette , Stephen Boyd , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Florian Fainelli , Broadcom internal kernel review list , Linus Walleij , Catalin Marinas , Will Deacon , Derek Kiernan , Dragan Cvetic , Arnd Bergmann , Greg Kroah-Hartman , Nicolas Ferre , Claudiu Beznea , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Saravana Kannan , Bjorn Helgaas , linux-clk@vger.kernel.org, devicetree@vger.kernel.org, linux-rpi-kernel@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org, netdev@vger.kernel.org, linux-pci@vger.kernel.org, linux-arch@vger.kernel.org, Lee Jones , Stefan Wahren References: <775000dfb3a35bc691010072942253cb022750e1.1724159867.git.andrea.porta@suse.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240820_113135_179007_F1334790 X-CRM114-Status: GOOD ( 29.05 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Catalin Marinas , Michael Turquette , Claudiu Beznea , Eric Dumazet , Dragan Cvetic , Will Deacon , linux-clk@vger.kernel.org, linux-arch@vger.kernel.org, Rob Herring , Florian Fainelli , Lee Jones , Saravana Kannan , Broadcom internal kernel review list , linux-pci@vger.kernel.org, Jakub Kicinski , Paolo Abeni , Linus Walleij , devicetree@vger.kernel.org, Conor Dooley , Arnd Bergmann , linux-gpio@vger.kernel.org, linux-rpi-kernel@lists.infradead.org, Bjorn Helgaas , Andrea della Porta , linux-arm-kernel@lists.infradead.org, Derek Kiernan , Stephen Boyd , Greg Kroah-Hartman , linux-kernel@vger.kernel.org, Stefan Wahren , netdev@vger.kernel.org, Krzysztof Kozlowski , "David S. Miller" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Andrew, On 17:13 Tue 20 Aug , Andrew Lunn wrote: > > +static unsigned int txdelay = 35; > > +module_param(txdelay, uint, 0644); > > Networking does not like module parameters. > > This is also unused in this patch! So i suggest you just delete it. > > > + > > /* This structure is only used for MACB on SiFive FU540 devices */ > > struct sifive_fu540_macb_mgmt { > > void __iomem *reg; > > @@ -334,7 +337,7 @@ static int macb_mdio_wait_for_idle(struct macb *bp) > > u32 val; > > > > return readx_poll_timeout(MACB_READ_NSR, bp, val, val & MACB_BIT(IDLE), > > - 1, MACB_MDIO_TIMEOUT); > > + 100, MACB_MDIO_TIMEOUT); > > } > > Please take this patch out of the series, and break it up. This is one > patch, with a good explanation why you need 1->100. > > > static int macb_mdio_read_c22(struct mii_bus *bus, int mii_id, int regnum) > > @@ -493,6 +496,19 @@ static int macb_mdio_write_c45(struct mii_bus *bus, int mii_id, > > return status; > > } > > > > +static int macb_mdio_reset(struct mii_bus *bus) > > +{ > > + struct macb *bp = bus->priv; > > + > > + if (bp->phy_reset_gpio) { > > + gpiod_set_value_cansleep(bp->phy_reset_gpio, 1); > > + msleep(bp->phy_reset_ms); > > + gpiod_set_value_cansleep(bp->phy_reset_gpio, 0); > > + } > > + > > + return 0; > > +} > > + > > static void macb_init_buffers(struct macb *bp) > > { > > struct macb_queue *queue; > > @@ -969,6 +985,7 @@ static int macb_mii_init(struct macb *bp) > > bp->mii_bus->write = &macb_mdio_write_c22; > > bp->mii_bus->read_c45 = &macb_mdio_read_c45; > > bp->mii_bus->write_c45 = &macb_mdio_write_c45; > > + bp->mii_bus->reset = &macb_mdio_reset; > > This is one patch. > > > snprintf(bp->mii_bus->id, MII_BUS_ID_SIZE, "%s-%x", > > bp->pdev->name, bp->pdev->id); > > bp->mii_bus->priv = bp; > > @@ -1640,6 +1657,11 @@ static int macb_rx(struct macb_queue *queue, struct napi_struct *napi, > > > > macb_init_rx_ring(queue); > > queue_writel(queue, RBQP, queue->rx_ring_dma); > > +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT > > + if (bp->hw_dma_cap & HW_DMA_CAP_64B) > > + macb_writel(bp, RBQPH, > > + upper_32_bits(queue->rx_ring_dma)); > > +#endif > > How does this affect a disto kernel? Do you actually need the #ifdef? > What does bp->hw_dma_cap contain when CONFIG_ARCH_DMA_ADDR_T_64BIT is > not defined? > > Again, this should be a patch of its own, with a good commit message. > > Interrupt coalescing should be a patch of its own, etc. > > Andrew > Thanks for the feedback, I agree on all the observations. Please do note however that, as mentioned in the cover letter, this patch is not intended to be included upstream and is provided just as a quick way for anyone interested in testing the RP1 functionality using the Ethernet MAC. As such, this patch has not been polished nor splitted into manageable bits. Ii'm taknge note of your comments however and will come back to them in a future patch that deals specifically with macb. Many thanks, Andrea > --- > pw-bot: cr