[net-next,v2,1/3] net: ethernet: ti: am65-cpsw: Move mode specific config to mac_config()
Message ID | 20230403110106.983994-2-s-vadapalli@ti.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp2223687vqo; Mon, 3 Apr 2023 04:22:10 -0700 (PDT) X-Google-Smtp-Source: AKy350bNTkODm3exmGYvoOd0PlTwVbkZCNzkadm7D3JND9y4/YJUbCuI8rSOqIpe3aY3fx0dTKhF X-Received: by 2002:a05:6a20:b29a:b0:dc:925f:62f1 with SMTP id ei26-20020a056a20b29a00b000dc925f62f1mr17615727pzb.6.1680520930650; Mon, 03 Apr 2023 04:22:10 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1680520930; cv=none; d=google.com; s=arc-20160816; b=CnzE+jLoXwJIg8wXzQAANKK1fbmoxW25kb/pCWlg075cEKASaHLSEDxGqRX0O5EXU6 grqLctZy1z+oHvTosIWETeVCcGRnjfKpuRSqB81W0ssnEU6vUxPENx7KilOfMNMEQyI8 6sX+QoohgA6Te/D5HyQrpo50T3ka6E6B9FJ+6apabqijJKHrTRVH9rycNG1E5wRNsJnN s6Ytr7uIsqP3FEw6EIpV+rbTDiEQtjqq3c5AxBfV4YG1KiyAWIBocUVRxNQH6VKvi3Gq iGGa4ZJIp6T2BfHVoMcVmgGKvitTqDigsc/m5pUx8S2eDCulnZcWZQSlmR77KFoFNirK Rewg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=Wnf4BjVfhzbMQx2Bis6Iz0+bTqMXHMv/UcZGWxk0XQ0=; b=sGKZ0HH8XbgMVbF8ZuYintpszkS8Z62sCzHqtRRBDN6+SLzMiKmKz/o4x3gzixkl7i I/AL8AjMjk0FXaOWyn0nCzDLGEIoZQbhgrF0IyeG1mAgcPFE2zqlFsCA1IUHIBgH5DAG BWoBJsRF+usgsksxIuhun90HlvKc+hNMgAMn+i0VFSwGbtbD/0fE50DRf2gPIPLjwZ1X F7e5sz8rFQpJF8XFk9ihYacYPri42hpP8UFMfTNPdTDx2j6mNTjbNyLBpElx0yprCQAv mgfqhmSszljxm6rQmKe1jMHO8lz3F5GCbUbl1WD5q2GnsJW0PGZJBZvdW9paEp493asG 1Heg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@ti.com header.s=ti-com-17Q1 header.b=YCpD36VB; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=NONE dis=NONE) header.from=ti.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id 145-20020a621997000000b0062dbafd7a3esi8279040pfz.100.2023.04.03.04.21.58; Mon, 03 Apr 2023 04:22:10 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@ti.com header.s=ti-com-17Q1 header.b=YCpD36VB; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=NONE dis=NONE) header.from=ti.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231707AbjDCLBu (ORCPT <rfc822;zwp10758@gmail.com> + 99 others); Mon, 3 Apr 2023 07:01:50 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50320 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231401AbjDCLBd (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 3 Apr 2023 07:01:33 -0400 Received: from fllv0016.ext.ti.com (fllv0016.ext.ti.com [198.47.19.142]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id EDD44113FD; Mon, 3 Apr 2023 04:01:31 -0700 (PDT) Received: from lelv0265.itg.ti.com ([10.180.67.224]) by fllv0016.ext.ti.com (8.15.2/8.15.2) with ESMTP id 333B1Emq016885; Mon, 3 Apr 2023 06:01:14 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ti.com; s=ti-com-17Q1; t=1680519674; bh=Wnf4BjVfhzbMQx2Bis6Iz0+bTqMXHMv/UcZGWxk0XQ0=; h=From:To:CC:Subject:Date:In-Reply-To:References; b=YCpD36VBHwThu6tgBvSpMF3m37xY+cfpGoSQTQwU8L/Arm9aOF7pii3e7bUk8JbwO EfLeyly1P/0gK10kGsuM1Sr+sdyM8/jsXe//6w9WyPcZBQK5XU2qdf+8XWjpGV2jVz xT2PUlQd9Fb+BDNQzbgI6Jsj62ipif6oNgfiOFMc= Received: from DLEE107.ent.ti.com (dlee107.ent.ti.com [157.170.170.37]) by lelv0265.itg.ti.com (8.15.2/8.15.2) with ESMTPS id 333B1E6T013295 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=FAIL); Mon, 3 Apr 2023 06:01:14 -0500 Received: from DLEE105.ent.ti.com (157.170.170.35) by DLEE107.ent.ti.com (157.170.170.37) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2507.16; Mon, 3 Apr 2023 06:01:13 -0500 Received: from lelv0327.itg.ti.com (10.180.67.183) by DLEE105.ent.ti.com (157.170.170.35) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2507.16 via Frontend Transport; Mon, 3 Apr 2023 06:01:13 -0500 Received: from uda0492258.dhcp.ti.com (ileaxei01-snat2.itg.ti.com [10.180.69.6]) by lelv0327.itg.ti.com (8.15.2/8.15.2) with ESMTP id 333B17wp101591; Mon, 3 Apr 2023 06:01:11 -0500 From: Siddharth Vadapalli <s-vadapalli@ti.com> To: <davem@davemloft.net>, <edumazet@google.com>, <kuba@kernel.org>, <linux@armlinux.org.uk>, <pabeni@redhat.com>, <rogerq@kernel.org> CC: <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>, <srk@ti.com>, <s-vadapalli@ti.com> Subject: [PATCH net-next v2 1/3] net: ethernet: ti: am65-cpsw: Move mode specific config to mac_config() Date: Mon, 3 Apr 2023 16:31:04 +0530 Message-ID: <20230403110106.983994-2-s-vadapalli@ti.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20230403110106.983994-1-s-vadapalli@ti.com> References: <20230403110106.983994-1-s-vadapalli@ti.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-EXCLAIMER-MD-CONFIG: e1e8a2fd-e40a-4ac6-ac9b-f7e9cc9ee180 X-Spam-Status: No, score=-2.5 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,SPF_HELO_PASS, SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1762153286092031579?= X-GMAIL-MSGID: =?utf-8?q?1762153915332053411?= |
Series |
Add support for J784S4 CPSW9G
|
|
Commit Message
Siddharth Vadapalli
April 3, 2023, 11:01 a.m. UTC
Move the interface mode specific configuration to the mac_config()
callback am65_cpsw_nuss_mac_config().
Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
drivers/net/ethernet/ti/am65-cpsw-nuss.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
Comments
Hello Russell, Thank you for reviewing the patch. On 03/04/23 16:38, Russell King (Oracle) wrote: > On Mon, Apr 03, 2023 at 04:31:04PM +0530, Siddharth Vadapalli wrote: >> Move the interface mode specific configuration to the mac_config() >> callback am65_cpsw_nuss_mac_config(). >> >> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> >> --- >> drivers/net/ethernet/ti/am65-cpsw-nuss.c | 10 +++++++--- >> 1 file changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c >> index d17757ecbf42..74e099828978 100644 >> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c >> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c >> @@ -1504,12 +1504,17 @@ static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned in >> phylink_config); >> struct am65_cpsw_port *port = container_of(slave, struct am65_cpsw_port, slave); >> struct am65_cpsw_common *common = port->common; >> + u32 mac_control = 0; >> >> if (common->pdata.extra_modes & BIT(state->interface)) { >> - if (state->interface == PHY_INTERFACE_MODE_SGMII) >> + if (state->interface == PHY_INTERFACE_MODE_SGMII) { >> + mac_control |= CPSW_SL_CTL_EXT_EN; >> writel(ADVERTISE_SGMII, >> port->sgmii_base + AM65_CPSW_SGMII_MR_ADV_ABILITY_REG); >> + } >> >> + if (mac_control) >> + cpsw_sl_ctl_set(port->slave.mac_sl, mac_control); >> writel(AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE, >> port->sgmii_base + AM65_CPSW_SGMII_CONTROL_REG); >> } >> @@ -1553,8 +1558,7 @@ static void am65_cpsw_nuss_mac_link_up(struct phylink_config *config, struct phy >> >> if (speed == SPEED_1000) >> mac_control |= CPSW_SL_CTL_GIG; >> - if (interface == PHY_INTERFACE_MODE_SGMII) >> - mac_control |= CPSW_SL_CTL_EXT_EN; >> + /* TODO: Verify whether in-band is necessary for 10 Mbps RGMII */ >> if (speed == SPEED_10 && phy_interface_mode_is_rgmii(interface)) >> /* Can be used with in band mode only */ >> mac_control |= CPSW_SL_CTL_EXT_EN; > > I'm afraid I can see you haven't thought this patch through properly. > > am65_cpsw_nuss_mac_link_down() will call > cpsw_sl_ctl_reset(port->slave.mac_sl); which has the effect of clearing > to zero the entire MAC control register. This will clear > CPSW_SL_CTL_EXT_EN that was set in am65_cpsw_nuss_mac_config() which is > not what you want to be doing. Right! I missed noticing this. It appeared to me that simply moving the code from mac_link_up() to mac_config() would suffice. However, as rightly pointed out by you, it doesn't account for the case where the interface state is toggled. > > Given that we have the 10Mbps issue with RGMII, I think what you want > to be doing is: > > 1. Set CPSW_SL_CTL_EXT_EN in am65_cpsw_nuss_mac_config() if in SGMII > mode, otherwise clear this bit. > > 2. Clear the mac_control register in am65_cpsw_nuss_mac_link_down() > if in RMGII mode, otherwise preserve the state of > CPSW_SL_CTL_EXT_EN but clear all other bits. > > 3. Set CPSW_SL_CTL_EXT_EN in am65_cpsw_nuss_mac_link_up() if in > RGMII mode and 10Mbps. > I will implement these changes and post the v3 series. Thank you for the feedback. Please let me know if I may add a "Suggested-by" tag. Regards, Siddharth.
On 03-04-2023 16:38, Russell King (Oracle) wrote: > On Mon, Apr 03, 2023 at 04:31:04PM +0530, Siddharth Vadapalli wrote: >> Move the interface mode specific configuration to the mac_config() >> callback am65_cpsw_nuss_mac_config(). >> >> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> >> --- >> drivers/net/ethernet/ti/am65-cpsw-nuss.c | 10 +++++++--- >> 1 file changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c >> index d17757ecbf42..74e099828978 100644 >> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c >> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c >> @@ -1504,12 +1504,17 @@ static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned in >> phylink_config); >> struct am65_cpsw_port *port = container_of(slave, struct am65_cpsw_port, slave); >> struct am65_cpsw_common *common = port->common; >> + u32 mac_control = 0; >> >> if (common->pdata.extra_modes & BIT(state->interface)) { >> - if (state->interface == PHY_INTERFACE_MODE_SGMII) >> + if (state->interface == PHY_INTERFACE_MODE_SGMII) { >> + mac_control |= CPSW_SL_CTL_EXT_EN; >> writel(ADVERTISE_SGMII, >> port->sgmii_base + AM65_CPSW_SGMII_MR_ADV_ABILITY_REG); >> + } >> >> + if (mac_control) >> + cpsw_sl_ctl_set(port->slave.mac_sl, mac_control); >> writel(AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE, >> port->sgmii_base + AM65_CPSW_SGMII_CONTROL_REG); >> } >> @@ -1553,8 +1558,7 @@ static void am65_cpsw_nuss_mac_link_up(struct phylink_config *config, struct phy >> >> if (speed == SPEED_1000) >> mac_control |= CPSW_SL_CTL_GIG; >> - if (interface == PHY_INTERFACE_MODE_SGMII) >> - mac_control |= CPSW_SL_CTL_EXT_EN; >> + /* TODO: Verify whether in-band is necessary for 10 Mbps RGMII */ >> if (speed == SPEED_10 && phy_interface_mode_is_rgmii(interface)) >> /* Can be used with in band mode only */ >> mac_control |= CPSW_SL_CTL_EXT_EN; > > I'm afraid I can see you haven't thought this patch through properly. > > am65_cpsw_nuss_mac_link_down() will call > cpsw_sl_ctl_reset(port->slave.mac_sl); which has the effect of clearing > to zero the entire MAC control register. This will clear > CPSW_SL_CTL_EXT_EN that was set in am65_cpsw_nuss_mac_config() which is > not what you want to be doing. > > Given that we have the 10Mbps issue with RGMII, I think what you want > to be doing is: > > 1. Set CPSW_SL_CTL_EXT_EN in am65_cpsw_nuss_mac_config() if in SGMII > mode, otherwise clear this bit. > > 2. Clear the mac_control register in am65_cpsw_nuss_mac_link_down() > if in RMGII mode, otherwise preserve the state of > CPSW_SL_CTL_EXT_EN but clear all other bits. > > 3. Set CPSW_SL_CTL_EXT_EN in am65_cpsw_nuss_mac_link_up() if in > RGMII mode and 10Mbps. I plan to implement it as follows: 1. Add a member "u32 mode_config" to "struct am65_cpsw_slave_data" in "am65-cpsw-nuss.h". 2. In am65_cpsw_nuss_mac_config(), store the value of mac_control in "port->slave.mode_config". 3. In am65_cpsw_nuss_mac_link_down(), after the reset via cpsw_sl_ctl_reset(), execute: cpsw_sl_ctl_set(port->slave.mac_sl, port->slave.mode_config) in order to restore the configuration performed in am65_cpsw_nuss_mac_config(). Please let me know in case of any suggestions to implement it in a better manner. Regards, Siddharth.
On Mon, Apr 03, 2023 at 06:31:52PM +0530, Siddharth Vadapalli wrote: > > > On 03-04-2023 16:38, Russell King (Oracle) wrote: > > On Mon, Apr 03, 2023 at 04:31:04PM +0530, Siddharth Vadapalli wrote: > >> Move the interface mode specific configuration to the mac_config() > >> callback am65_cpsw_nuss_mac_config(). > >> > >> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> > >> --- > >> drivers/net/ethernet/ti/am65-cpsw-nuss.c | 10 +++++++--- > >> 1 file changed, 7 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c > >> index d17757ecbf42..74e099828978 100644 > >> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c > >> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c > >> @@ -1504,12 +1504,17 @@ static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned in > >> phylink_config); > >> struct am65_cpsw_port *port = container_of(slave, struct am65_cpsw_port, slave); > >> struct am65_cpsw_common *common = port->common; > >> + u32 mac_control = 0; > >> > >> if (common->pdata.extra_modes & BIT(state->interface)) { > >> - if (state->interface == PHY_INTERFACE_MODE_SGMII) > >> + if (state->interface == PHY_INTERFACE_MODE_SGMII) { > >> + mac_control |= CPSW_SL_CTL_EXT_EN; > >> writel(ADVERTISE_SGMII, > >> port->sgmii_base + AM65_CPSW_SGMII_MR_ADV_ABILITY_REG); > >> + } > >> > >> + if (mac_control) > >> + cpsw_sl_ctl_set(port->slave.mac_sl, mac_control); > >> writel(AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE, > >> port->sgmii_base + AM65_CPSW_SGMII_CONTROL_REG); > >> } > >> @@ -1553,8 +1558,7 @@ static void am65_cpsw_nuss_mac_link_up(struct phylink_config *config, struct phy > >> > >> if (speed == SPEED_1000) > >> mac_control |= CPSW_SL_CTL_GIG; > >> - if (interface == PHY_INTERFACE_MODE_SGMII) > >> - mac_control |= CPSW_SL_CTL_EXT_EN; > >> + /* TODO: Verify whether in-band is necessary for 10 Mbps RGMII */ > >> if (speed == SPEED_10 && phy_interface_mode_is_rgmii(interface)) > >> /* Can be used with in band mode only */ > >> mac_control |= CPSW_SL_CTL_EXT_EN; > > > > I'm afraid I can see you haven't thought this patch through properly. > > > > am65_cpsw_nuss_mac_link_down() will call > > cpsw_sl_ctl_reset(port->slave.mac_sl); which has the effect of clearing > > to zero the entire MAC control register. This will clear > > CPSW_SL_CTL_EXT_EN that was set in am65_cpsw_nuss_mac_config() which is > > not what you want to be doing. > > > > Given that we have the 10Mbps issue with RGMII, I think what you want > > to be doing is: > > > > 1. Set CPSW_SL_CTL_EXT_EN in am65_cpsw_nuss_mac_config() if in SGMII > > mode, otherwise clear this bit. > > > > 2. Clear the mac_control register in am65_cpsw_nuss_mac_link_down() > > if in RMGII mode, otherwise preserve the state of > > CPSW_SL_CTL_EXT_EN but clear all other bits. > > > > 3. Set CPSW_SL_CTL_EXT_EN in am65_cpsw_nuss_mac_link_up() if in > > RGMII mode and 10Mbps. > > I plan to implement it as follows: > 1. Add a member "u32 mode_config" to "struct am65_cpsw_slave_data" in > "am65-cpsw-nuss.h". > 2. In am65_cpsw_nuss_mac_config(), store the value of mac_control in > "port->slave.mode_config". > 3. In am65_cpsw_nuss_mac_link_down(), after the reset via > cpsw_sl_ctl_reset(), execute: > cpsw_sl_ctl_set(port->slave.mac_sl, port->slave.mode_config) in order to > restore the configuration performed in am65_cpsw_nuss_mac_config(). > > Please let me know in case of any suggestions to implement it in a > better manner. Do you think this complexity is really worth it? Let's look at what's available: cpsw_sl_ctl_set() - sets bits in the mac control register cpsw_sl_ctl_clr() - clears bits in the mac control register cpsw_sl_ctl_reset() - sets the mac control register to zero So, in mac_config(), we can do: if (interface == SGMII) cpsw_sl_ctl_set(CPSW_SL_CTL_EXT_EN); else cpsw_sl_ctl_clr(CPSW_SL_CTL_EXT_EN); in mac_link_down(): u32 ctl; ctl = CPSW_SL_CTL_GMII_EN | CPSW_SL_CTL_GIG | CPSW_SL_CTL_IFCTL_A | CPSW_SL_CTL_FULLDUPLEX | CPSW_SL_CTL_RX_FLOW_EN | CPSW_SL_CTL_TX_FLOW_EN; if (phy_interface_mode_is_rgmii(interface)) ctl |= CPSW_SL_CTL_EXT_EN; cpsw_sl_ctl_clr(ctl); This ensures that we don't touch any bits in mac_link_down() which we aren't modifying in the corresponding mac_link_up() implementation. Q: do we really need to clear the mac control register on link down? If we don't, then we can do better, but we need an additional helper which allows read-modify-write of the mac control register using a mask value and a value of bits to set. Then we can have mac_link_up() setting and clearing the bits as necessary - but I would still keep the clearing of CPSW_SL_CTL_EXT_EN for RGMII modes in mac_link_down() for now.
On 03-04-2023 18:43, Russell King (Oracle) wrote: > On Mon, Apr 03, 2023 at 06:31:52PM +0530, Siddharth Vadapalli wrote: >> >> >> On 03-04-2023 16:38, Russell King (Oracle) wrote: >>> On Mon, Apr 03, 2023 at 04:31:04PM +0530, Siddharth Vadapalli wrote: >>>> Move the interface mode specific configuration to the mac_config() >>>> callback am65_cpsw_nuss_mac_config(). >>>> >>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> >>>> --- >>>> drivers/net/ethernet/ti/am65-cpsw-nuss.c | 10 +++++++--- >>>> 1 file changed, 7 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c >>>> index d17757ecbf42..74e099828978 100644 >>>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c >>>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c >>>> @@ -1504,12 +1504,17 @@ static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned in >>>> phylink_config); >>>> struct am65_cpsw_port *port = container_of(slave, struct am65_cpsw_port, slave); >>>> struct am65_cpsw_common *common = port->common; >>>> + u32 mac_control = 0; >>>> >>>> if (common->pdata.extra_modes & BIT(state->interface)) { >>>> - if (state->interface == PHY_INTERFACE_MODE_SGMII) >>>> + if (state->interface == PHY_INTERFACE_MODE_SGMII) { >>>> + mac_control |= CPSW_SL_CTL_EXT_EN; >>>> writel(ADVERTISE_SGMII, >>>> port->sgmii_base + AM65_CPSW_SGMII_MR_ADV_ABILITY_REG); >>>> + } >>>> >>>> + if (mac_control) >>>> + cpsw_sl_ctl_set(port->slave.mac_sl, mac_control); >>>> writel(AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE, >>>> port->sgmii_base + AM65_CPSW_SGMII_CONTROL_REG); >>>> } >>>> @@ -1553,8 +1558,7 @@ static void am65_cpsw_nuss_mac_link_up(struct phylink_config *config, struct phy >>>> >>>> if (speed == SPEED_1000) >>>> mac_control |= CPSW_SL_CTL_GIG; >>>> - if (interface == PHY_INTERFACE_MODE_SGMII) >>>> - mac_control |= CPSW_SL_CTL_EXT_EN; >>>> + /* TODO: Verify whether in-band is necessary for 10 Mbps RGMII */ >>>> if (speed == SPEED_10 && phy_interface_mode_is_rgmii(interface)) >>>> /* Can be used with in band mode only */ >>>> mac_control |= CPSW_SL_CTL_EXT_EN; >>> >>> I'm afraid I can see you haven't thought this patch through properly. >>> >>> am65_cpsw_nuss_mac_link_down() will call >>> cpsw_sl_ctl_reset(port->slave.mac_sl); which has the effect of clearing >>> to zero the entire MAC control register. This will clear >>> CPSW_SL_CTL_EXT_EN that was set in am65_cpsw_nuss_mac_config() which is >>> not what you want to be doing. >>> >>> Given that we have the 10Mbps issue with RGMII, I think what you want >>> to be doing is: >>> >>> 1. Set CPSW_SL_CTL_EXT_EN in am65_cpsw_nuss_mac_config() if in SGMII >>> mode, otherwise clear this bit. >>> >>> 2. Clear the mac_control register in am65_cpsw_nuss_mac_link_down() >>> if in RMGII mode, otherwise preserve the state of >>> CPSW_SL_CTL_EXT_EN but clear all other bits. >>> >>> 3. Set CPSW_SL_CTL_EXT_EN in am65_cpsw_nuss_mac_link_up() if in >>> RGMII mode and 10Mbps. >> >> I plan to implement it as follows: >> 1. Add a member "u32 mode_config" to "struct am65_cpsw_slave_data" in >> "am65-cpsw-nuss.h". >> 2. In am65_cpsw_nuss_mac_config(), store the value of mac_control in >> "port->slave.mode_config". >> 3. In am65_cpsw_nuss_mac_link_down(), after the reset via >> cpsw_sl_ctl_reset(), execute: >> cpsw_sl_ctl_set(port->slave.mac_sl, port->slave.mode_config) in order to >> restore the configuration performed in am65_cpsw_nuss_mac_config(). >> >> Please let me know in case of any suggestions to implement it in a >> better manner. > > Do you think this complexity is really worth it? > > Let's look at what's available: > > cpsw_sl_ctl_set() - sets bits in the mac control register > cpsw_sl_ctl_clr() - clears bits in the mac control register > cpsw_sl_ctl_reset() - sets the mac control register to zero > > So, in mac_config(), we can do: > > if (interface == SGMII) > cpsw_sl_ctl_set(CPSW_SL_CTL_EXT_EN); > else > cpsw_sl_ctl_clr(CPSW_SL_CTL_EXT_EN); While this will work for patch 1/3, once I add support for USXGMII mode as in patch 3/3, I believe that I have to invert it, beginning by invoking a cpsw_sl_ctl_clr(CPSW_SL_CTL_EXT_EN) at the start in mac_config() followed by switching through the modes. If the mode is SGMII, then I invoke cpsw_sl_ctl_set(CPSW_SL_CTL_EXT_EN), along with the write to the MR_ADV_ABILITY_REG register to advertise SGMII. If the mode is USXGMII, then I invoke: cpsw_sl_ctl_set(CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN) > > > in mac_link_down(): > > u32 ctl; > > ctl = CPSW_SL_CTL_GMII_EN | CPSW_SL_CTL_GIG | > CPSW_SL_CTL_IFCTL_A | CPSW_SL_CTL_FULLDUPLEX | > CPSW_SL_CTL_RX_FLOW_EN | CPSW_SL_CTL_TX_FLOW_EN; > if (phy_interface_mode_is_rgmii(interface)) > ctl |= CPSW_SL_CTL_EXT_EN; > > cpsw_sl_ctl_clr(ctl); > > This ensures that we don't touch any bits in mac_link_down() which we > aren't modifying in the corresponding mac_link_up() implementation. This is very helpful. Thank you for the suggestion :) > > Q: do we really need to clear the mac control register on link down? > If we don't, then we can do better, but we need an additional helper > which allows read-modify-write of the mac control register using a > mask value and a value of bits to set. Then we can have mac_link_up() > setting and clearing the bits as necessary - but I would still keep > the clearing of CPSW_SL_CTL_EXT_EN for RGMII modes in mac_link_down() > for now. Clearing the entire register is not necessary. As long as those bits that were set during mac_link_up() are cleared, it should be sufficient. For now, I will implement the changes assuming that the mac control register will be cleared on link down, based on the above suggestions provided by you. Regards, Siddharth.
On Mon, Apr 03, 2023 at 07:20:21PM +0530, Siddharth Vadapalli wrote: > > > On 03-04-2023 18:43, Russell King (Oracle) wrote: > > On Mon, Apr 03, 2023 at 06:31:52PM +0530, Siddharth Vadapalli wrote: > >> > >> > >> On 03-04-2023 16:38, Russell King (Oracle) wrote: > >>> On Mon, Apr 03, 2023 at 04:31:04PM +0530, Siddharth Vadapalli wrote: > >>>> Move the interface mode specific configuration to the mac_config() > >>>> callback am65_cpsw_nuss_mac_config(). > >>>> > >>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> > >>>> --- > >>>> drivers/net/ethernet/ti/am65-cpsw-nuss.c | 10 +++++++--- > >>>> 1 file changed, 7 insertions(+), 3 deletions(-) > >>>> > >>>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c > >>>> index d17757ecbf42..74e099828978 100644 > >>>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c > >>>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c > >>>> @@ -1504,12 +1504,17 @@ static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned in > >>>> phylink_config); > >>>> struct am65_cpsw_port *port = container_of(slave, struct am65_cpsw_port, slave); > >>>> struct am65_cpsw_common *common = port->common; > >>>> + u32 mac_control = 0; > >>>> > >>>> if (common->pdata.extra_modes & BIT(state->interface)) { > >>>> - if (state->interface == PHY_INTERFACE_MODE_SGMII) > >>>> + if (state->interface == PHY_INTERFACE_MODE_SGMII) { > >>>> + mac_control |= CPSW_SL_CTL_EXT_EN; > >>>> writel(ADVERTISE_SGMII, > >>>> port->sgmii_base + AM65_CPSW_SGMII_MR_ADV_ABILITY_REG); > >>>> + } > >>>> > >>>> + if (mac_control) > >>>> + cpsw_sl_ctl_set(port->slave.mac_sl, mac_control); > >>>> writel(AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE, > >>>> port->sgmii_base + AM65_CPSW_SGMII_CONTROL_REG); > >>>> } > >>>> @@ -1553,8 +1558,7 @@ static void am65_cpsw_nuss_mac_link_up(struct phylink_config *config, struct phy > >>>> > >>>> if (speed == SPEED_1000) > >>>> mac_control |= CPSW_SL_CTL_GIG; > >>>> - if (interface == PHY_INTERFACE_MODE_SGMII) > >>>> - mac_control |= CPSW_SL_CTL_EXT_EN; > >>>> + /* TODO: Verify whether in-band is necessary for 10 Mbps RGMII */ > >>>> if (speed == SPEED_10 && phy_interface_mode_is_rgmii(interface)) > >>>> /* Can be used with in band mode only */ > >>>> mac_control |= CPSW_SL_CTL_EXT_EN; > >>> > >>> I'm afraid I can see you haven't thought this patch through properly. > >>> > >>> am65_cpsw_nuss_mac_link_down() will call > >>> cpsw_sl_ctl_reset(port->slave.mac_sl); which has the effect of clearing > >>> to zero the entire MAC control register. This will clear > >>> CPSW_SL_CTL_EXT_EN that was set in am65_cpsw_nuss_mac_config() which is > >>> not what you want to be doing. > >>> > >>> Given that we have the 10Mbps issue with RGMII, I think what you want > >>> to be doing is: > >>> > >>> 1. Set CPSW_SL_CTL_EXT_EN in am65_cpsw_nuss_mac_config() if in SGMII > >>> mode, otherwise clear this bit. > >>> > >>> 2. Clear the mac_control register in am65_cpsw_nuss_mac_link_down() > >>> if in RMGII mode, otherwise preserve the state of > >>> CPSW_SL_CTL_EXT_EN but clear all other bits. > >>> > >>> 3. Set CPSW_SL_CTL_EXT_EN in am65_cpsw_nuss_mac_link_up() if in > >>> RGMII mode and 10Mbps. > >> > >> I plan to implement it as follows: > >> 1. Add a member "u32 mode_config" to "struct am65_cpsw_slave_data" in > >> "am65-cpsw-nuss.h". > >> 2. In am65_cpsw_nuss_mac_config(), store the value of mac_control in > >> "port->slave.mode_config". > >> 3. In am65_cpsw_nuss_mac_link_down(), after the reset via > >> cpsw_sl_ctl_reset(), execute: > >> cpsw_sl_ctl_set(port->slave.mac_sl, port->slave.mode_config) in order to > >> restore the configuration performed in am65_cpsw_nuss_mac_config(). > >> > >> Please let me know in case of any suggestions to implement it in a > >> better manner. > > > > Do you think this complexity is really worth it? > > > > Let's look at what's available: > > > > cpsw_sl_ctl_set() - sets bits in the mac control register > > cpsw_sl_ctl_clr() - clears bits in the mac control register > > cpsw_sl_ctl_reset() - sets the mac control register to zero > > > > So, in mac_config(), we can do: > > > > if (interface == SGMII) > > cpsw_sl_ctl_set(CPSW_SL_CTL_EXT_EN); > > else > > cpsw_sl_ctl_clr(CPSW_SL_CTL_EXT_EN); > > While this will work for patch 1/3, once I add support for USXGMII mode > as in patch 3/3, I believe that I have to invert it, beginning by > invoking a cpsw_sl_ctl_clr(CPSW_SL_CTL_EXT_EN) at the start in > mac_config() followed by switching through the modes. If the mode is > SGMII, then I invoke cpsw_sl_ctl_set(CPSW_SL_CTL_EXT_EN), along with the > write to the MR_ADV_ABILITY_REG register to advertise SGMII. If the mode > is USXGMII, then I invoke: > cpsw_sl_ctl_set(CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN) For patch 1, I did leave out the write for MR_ADV_ABILITY_REG, I had assumed you'd get the idea on that and merge the if() condition you already had with my suggestion above (which isn't literal code!) In patch 3, you simply need to add: if (interface == USXGMII) cpsw_sl_ctl_set(CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN); else cpsw_sl_ctl_clr(CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN); Thanks.
On 03-04-2023 19:25, Russell King (Oracle) wrote: > On Mon, Apr 03, 2023 at 07:20:21PM +0530, Siddharth Vadapalli wrote: >> >> >> On 03-04-2023 18:43, Russell King (Oracle) wrote: >>> On Mon, Apr 03, 2023 at 06:31:52PM +0530, Siddharth Vadapalli wrote: >>>> >>>> >>>> On 03-04-2023 16:38, Russell King (Oracle) wrote: >>>>> On Mon, Apr 03, 2023 at 04:31:04PM +0530, Siddharth Vadapalli wrote: >>>>>> Move the interface mode specific configuration to the mac_config() >>>>>> callback am65_cpsw_nuss_mac_config(). >>>>>> >>>>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> >>>>>> --- >>>>>> drivers/net/ethernet/ti/am65-cpsw-nuss.c | 10 +++++++--- >>>>>> 1 file changed, 7 insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c >>>>>> index d17757ecbf42..74e099828978 100644 >>>>>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c >>>>>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c >>>>>> @@ -1504,12 +1504,17 @@ static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned in >>>>>> phylink_config); >>>>>> struct am65_cpsw_port *port = container_of(slave, struct am65_cpsw_port, slave); >>>>>> struct am65_cpsw_common *common = port->common; >>>>>> + u32 mac_control = 0; >>>>>> >>>>>> if (common->pdata.extra_modes & BIT(state->interface)) { >>>>>> - if (state->interface == PHY_INTERFACE_MODE_SGMII) >>>>>> + if (state->interface == PHY_INTERFACE_MODE_SGMII) { >>>>>> + mac_control |= CPSW_SL_CTL_EXT_EN; >>>>>> writel(ADVERTISE_SGMII, >>>>>> port->sgmii_base + AM65_CPSW_SGMII_MR_ADV_ABILITY_REG); >>>>>> + } >>>>>> >>>>>> + if (mac_control) >>>>>> + cpsw_sl_ctl_set(port->slave.mac_sl, mac_control); >>>>>> writel(AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE, >>>>>> port->sgmii_base + AM65_CPSW_SGMII_CONTROL_REG); >>>>>> } >>>>>> @@ -1553,8 +1558,7 @@ static void am65_cpsw_nuss_mac_link_up(struct phylink_config *config, struct phy >>>>>> >>>>>> if (speed == SPEED_1000) >>>>>> mac_control |= CPSW_SL_CTL_GIG; >>>>>> - if (interface == PHY_INTERFACE_MODE_SGMII) >>>>>> - mac_control |= CPSW_SL_CTL_EXT_EN; >>>>>> + /* TODO: Verify whether in-band is necessary for 10 Mbps RGMII */ >>>>>> if (speed == SPEED_10 && phy_interface_mode_is_rgmii(interface)) >>>>>> /* Can be used with in band mode only */ >>>>>> mac_control |= CPSW_SL_CTL_EXT_EN; >>>>> >>>>> I'm afraid I can see you haven't thought this patch through properly. >>>>> >>>>> am65_cpsw_nuss_mac_link_down() will call >>>>> cpsw_sl_ctl_reset(port->slave.mac_sl); which has the effect of clearing >>>>> to zero the entire MAC control register. This will clear >>>>> CPSW_SL_CTL_EXT_EN that was set in am65_cpsw_nuss_mac_config() which is >>>>> not what you want to be doing. >>>>> >>>>> Given that we have the 10Mbps issue with RGMII, I think what you want >>>>> to be doing is: >>>>> >>>>> 1. Set CPSW_SL_CTL_EXT_EN in am65_cpsw_nuss_mac_config() if in SGMII >>>>> mode, otherwise clear this bit. >>>>> >>>>> 2. Clear the mac_control register in am65_cpsw_nuss_mac_link_down() >>>>> if in RMGII mode, otherwise preserve the state of >>>>> CPSW_SL_CTL_EXT_EN but clear all other bits. >>>>> >>>>> 3. Set CPSW_SL_CTL_EXT_EN in am65_cpsw_nuss_mac_link_up() if in >>>>> RGMII mode and 10Mbps. >>>> >>>> I plan to implement it as follows: >>>> 1. Add a member "u32 mode_config" to "struct am65_cpsw_slave_data" in >>>> "am65-cpsw-nuss.h". >>>> 2. In am65_cpsw_nuss_mac_config(), store the value of mac_control in >>>> "port->slave.mode_config". >>>> 3. In am65_cpsw_nuss_mac_link_down(), after the reset via >>>> cpsw_sl_ctl_reset(), execute: >>>> cpsw_sl_ctl_set(port->slave.mac_sl, port->slave.mode_config) in order to >>>> restore the configuration performed in am65_cpsw_nuss_mac_config(). >>>> >>>> Please let me know in case of any suggestions to implement it in a >>>> better manner. >>> >>> Do you think this complexity is really worth it? >>> >>> Let's look at what's available: >>> >>> cpsw_sl_ctl_set() - sets bits in the mac control register >>> cpsw_sl_ctl_clr() - clears bits in the mac control register >>> cpsw_sl_ctl_reset() - sets the mac control register to zero >>> >>> So, in mac_config(), we can do: >>> >>> if (interface == SGMII) >>> cpsw_sl_ctl_set(CPSW_SL_CTL_EXT_EN); >>> else >>> cpsw_sl_ctl_clr(CPSW_SL_CTL_EXT_EN); >> >> While this will work for patch 1/3, once I add support for USXGMII mode >> as in patch 3/3, I believe that I have to invert it, beginning by >> invoking a cpsw_sl_ctl_clr(CPSW_SL_CTL_EXT_EN) at the start in >> mac_config() followed by switching through the modes. If the mode is >> SGMII, then I invoke cpsw_sl_ctl_set(CPSW_SL_CTL_EXT_EN), along with the >> write to the MR_ADV_ABILITY_REG register to advertise SGMII. If the mode >> is USXGMII, then I invoke: >> cpsw_sl_ctl_set(CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN) > > For patch 1, I did leave out the write for MR_ADV_ABILITY_REG, I had > assumed you'd get the idea on that and merge the if() condition you > already had with my suggestion above (which isn't literal code!) > > In patch 3, you simply need to add: > > if (interface == USXGMII) > cpsw_sl_ctl_set(CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN); > else > cpsw_sl_ctl_clr(CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN); Thank you for answering all my questions and sharing your valuable feedback. I will implement all the suggestions in the v3 series. Regards, Siddharth.
diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c index d17757ecbf42..74e099828978 100644 --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c @@ -1504,12 +1504,17 @@ static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned in phylink_config); struct am65_cpsw_port *port = container_of(slave, struct am65_cpsw_port, slave); struct am65_cpsw_common *common = port->common; + u32 mac_control = 0; if (common->pdata.extra_modes & BIT(state->interface)) { - if (state->interface == PHY_INTERFACE_MODE_SGMII) + if (state->interface == PHY_INTERFACE_MODE_SGMII) { + mac_control |= CPSW_SL_CTL_EXT_EN; writel(ADVERTISE_SGMII, port->sgmii_base + AM65_CPSW_SGMII_MR_ADV_ABILITY_REG); + } + if (mac_control) + cpsw_sl_ctl_set(port->slave.mac_sl, mac_control); writel(AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE, port->sgmii_base + AM65_CPSW_SGMII_CONTROL_REG); } @@ -1553,8 +1558,7 @@ static void am65_cpsw_nuss_mac_link_up(struct phylink_config *config, struct phy if (speed == SPEED_1000) mac_control |= CPSW_SL_CTL_GIG; - if (interface == PHY_INTERFACE_MODE_SGMII) - mac_control |= CPSW_SL_CTL_EXT_EN; + /* TODO: Verify whether in-band is necessary for 10 Mbps RGMII */ if (speed == SPEED_10 && phy_interface_mode_is_rgmii(interface)) /* Can be used with in band mode only */ mac_control |= CPSW_SL_CTL_EXT_EN;