On Wed, Oct 10, 2018 at 12:11 AM Pankaj Bansal pankaj.bansal@nxp.com wrote:
Hi Joe,
-----Original Message----- From: Joe Hershberger [mailto:joe.hershberger@ni.com] Sent: Wednesday, October 10, 2018 9:29 AM To: Pankaj Bansal pankaj.bansal@nxp.com Cc: Joseph Hershberger joseph.hershberger@ni.com; u-boot <u- boot@lists.denx.de> Subject: Re: [U-Boot] [PATCH v3 2/6] driver: net: fsl-mc: remove unused strcture elements
On Tue, Oct 9, 2018 at 9:59 PM Pankaj Bansal pankaj.bansal@nxp.com wrote:
The phydev structure is present in both ldpaa_eth_priv and wriop_dpmac_info. the phydev in wriop_dpmac_info is not being used
As the phydev is created based on phy_addr and bus members of wriop_dpmac_info, it is appropriate to keep phydev in
wriop_dpmac_info.
Also phy_regs is not being used, therefore remove it
Signed-off-by: Pankaj Bansal pankaj.bansal@nxp.com Acked-by: Joe Hershberger joe.hershberger@ni.com
Notes: V3: - No change
Please be sure you are running scripts/checkpatch.pl on your patches. v2 had a number of issues I had to fix up. I'm pretty sure this was one of them.
I had run checkpatch script on all the versions of patches I sent. I use this command "./scripts/checkpatch.pl 0001-something.patch" This "no return at the end of function" issue was not reported by checkpatch script.
Can you please tell me which issues in V2 are you referring to? Because when I ran checkpatch.pl, it gave me no errors or warnings but 7 checks regarding alignment in board/freescale/ls2080aqds/eth.c. I did not do any changes for that because that code was not part of my patch and I think that was done so that line doesn't exceed 80 characters.
This patch specifically had a complaint "CHECK: braces {} should be used on all arms of this statement" that I fixed up.
This issue doesn't last long, since the code in question is fixed in "driver: net: fsl-mc: Modify the dpmac link detection method", but we prefer not to have patches that have issues. patman will tell you about it.
I've applied v3 and it looks like "Freescale AArch64" is still warning ( https://travis-ci.org/jhershbe/u-boot/jobs/439762814 ) ...
---------
aarch64: + ls2080a_emu +drivers/net/ldpaa_eth/ldpaa_eth.c: In function 'ldpaa_get_dpmac_state': +drivers/net/ldpaa_eth/ldpaa_eth.c:408:6: error: unused variable 'err' [-Werror=unused-variable] + int err; + ^~~ +drivers/net/ldpaa_eth/ldpaa_eth.c:407:6: error: unused variable 'phy_num' [-Werror=unused-variable] + int phy_num, phys_detected; + ^~~~~~~ +drivers/net/ldpaa_eth/ldpaa_eth.c:405:21: error: unused variable 'phydev' [-Werror=unused-variable] + struct phy_device *phydev = NULL; + ^~~~~~ +drivers/net/ldpaa_eth/ldpaa_eth.c: In function 'ldpaa_eth_stop': +drivers/net/ldpaa_eth/ldpaa_eth.c:594:6: error: unused variable 'phy_num' [-Werror=unused-variable] + int phy_num; +drivers/net/ldpaa_eth/ldpaa_eth.c:593:21: error: unused variable 'phydev' [-Werror=unused-variable] +cc1: all warnings being treated as errors +make[3]: *** [drivers/net/ldpaa_eth/ldpaa_eth.o] Error 1 +make[2]: *** [drivers/net/ldpaa_eth] Error 2 +make[1]: *** [drivers/net] Error 2 +make: *** [sub-make] Error 2
---------
Both "driver: net: fsl-mc: Modify the dpmac link detection method" and "driver: net: fsl-mc: Add support of multiple phys for dpmac" introduce unused variable warnings on ls2080a_emu.
-Joe
You would do yourself a favor to use tools/patman/patman.
This is a good advice. I will use patman from now onwards to prepare and send patches.
V2: - change (phydev && bus != NULL) to (phydev && bus) - after free phydev just pass NULL into wriop_set_phy_dev()drivers/net/ldpaa_eth/ldpaa_eth.c | 56 +++++++++++++++------------ drivers/net/ldpaa_eth/ldpaa_eth.h | 1 - drivers/net/ldpaa_eth/ldpaa_wriop.c | 2 + include/fsl-mc/ldpaa_wriop.h | 1 - 4 files changed, 33 insertions(+), 27 deletions(-)
diff --git a/drivers/net/ldpaa_eth/ldpaa_eth.c b/drivers/net/ldpaa_eth/ldpaa_eth.c index 82a684bea2..ca3459cc33 100644 --- a/drivers/net/ldpaa_eth/ldpaa_eth.c +++ b/drivers/net/ldpaa_eth/ldpaa_eth.c @@ -35,7 +35,7 @@ static int init_phy(struct eth_device *dev) return -1; }
priv->phydev = phydev;
wriop_set_phy_dev(priv->dpmac_id, phydev); return phy_config(phydev);} @@ -388,6 +388,7 @@ static int ldpaa_eth_open(struct eth_device
*net_dev, bd_t *bd)
struct mii_dev *bus; phy_interface_t enet_if; struct dpni_queue d_queue;
struct phy_device *phydev = NULL; if (net_dev->state == ETH_STATE_ACTIVE) return 0;@@ -408,38 +409,41 @@ static int ldpaa_eth_open(struct eth_device
*net_dev, bd_t *bd)
goto err_dpmac_setup;#ifdef CONFIG_PHYLIB
if (priv->phydev) {err = phy_startup(priv->phydev);
phydev = wriop_get_phy_dev(priv->dpmac_id);if (phydev) {err = phy_startup(phydev); if (err) { printf("%s: Could not initialize\n",
priv->phydev->dev->name);
phydev->dev->name); goto err_dpmac_bind; } }#else
priv->phydev = (struct phy_device *)malloc(sizeof(structphy_device));
memset(priv->phydev, 0, sizeof(struct phy_device));
phydev = (struct phy_device *)malloc(sizeof(struct phy_device));memset(phydev, 0, sizeof(struct phy_device));wriop_set_phy_dev(priv->dpmac_id, phydev);
priv->phydev->speed = SPEED_1000;priv->phydev->link = 1;priv->phydev->duplex = DUPLEX_FULL;
phydev->speed = SPEED_1000;phydev->link = 1;phydev->duplex = DUPLEX_FULL;#endif
bus = wriop_get_mdio(priv->dpmac_id); enet_if = wriop_get_enet_if(priv->dpmac_id); if ((bus == NULL) && (enet_if == PHY_INTERFACE_MODE_XGMII)) {
priv->phydev = (struct phy_device *)
phydev = (struct phy_device *) malloc(sizeof(struct phy_device));
memset(priv->phydev, 0, sizeof(struct phy_device));
memset(phydev, 0, sizeof(struct phy_device));wriop_set_phy_dev(priv->dpmac_id, phydev);
priv->phydev->speed = SPEED_10000;priv->phydev->link = 1;priv->phydev->duplex = DUPLEX_FULL;
phydev->speed = SPEED_10000;phydev->link = 1;phydev->duplex = DUPLEX_FULL; }
if (!priv->phydev->link) {printf("%s: No link.\n", priv->phydev->dev->name);
if (!phydev->link) {printf("%s: No link.\n", phydev->dev->name); err = -1; goto err_dpmac_bind; }@@ -476,17 +480,17 @@ static int ldpaa_eth_open(struct eth_device
*net_dev, bd_t *bd)
return err; }
dpmac_link_state.rate = priv->phydev->speed;
dpmac_link_state.rate = phydev->speed;
if (priv->phydev->autoneg == AUTONEG_DISABLE)
if (phydev->autoneg == AUTONEG_DISABLE) dpmac_link_state.options &= ~DPMAC_LINK_OPT_AUTONEG; else dpmac_link_state.options |= DPMAC_LINK_OPT_AUTONEG;
if (priv->phydev->duplex == DUPLEX_HALF)
if (phydev->duplex == DUPLEX_HALF) dpmac_link_state.options |=DPMAC_LINK_OPT_HALF_DUPLEX;
dpmac_link_state.up = priv->phydev->link;
dpmac_link_state.up = phydev->link; err = dpmac_set_link_state(dflt_mc_io, MC_CMD_NO_FLAGS, priv->dpmac_handle,&dpmac_link_state); @@ -530,7 +534,7 @@ static int
ldpaa_eth_open(struct eth_device *net_dev, bd_t *bd)
goto err_qdid; }
return priv->phydev->link;
return phydev->link;err_qdid: err_get_queue: @@ -556,6 +560,7 @@ static void ldpaa_eth_stop(struct eth_device *net_dev) #ifdef CONFIG_PHYLIB struct mii_dev *bus = wriop_get_mdio(priv->dpmac_id); #endif
struct phy_device *phydev = NULL; if ((net_dev->state == ETH_STATE_PASSIVE) || (net_dev->state == ETH_STATE_INIT)) @@ -588,11 +593,12 @@static void ldpaa_eth_stop(struct eth_device *net_dev) printf("dpni_disable() failed\n");
#ifdef CONFIG_PHYLIB
if (priv->phydev && bus != NULL)phy_shutdown(priv->phydev);
phydev = wriop_get_phy_dev(priv->dpmac_id);if (phydev && bus)phy_shutdown(phydev); else {
free(priv->phydev);priv->phydev = NULL;
free(phydev);wriop_set_phy_dev(priv->dpmac_id, NULL); }#endif
diff --git a/drivers/net/ldpaa_eth/ldpaa_eth.h b/drivers/net/ldpaa_eth/ldpaa_eth.h index ee784a55ee..3f9154b5bb 100644 --- a/drivers/net/ldpaa_eth/ldpaa_eth.h +++ b/drivers/net/ldpaa_eth/ldpaa_eth.h @@ -127,7 +127,6 @@ struct ldpaa_eth_priv { uint16_t tx_flow_id;
enum ldpaa_eth_type type; /* 1G or 10G ethernet */
struct phy_device *phydev;};
struct dprc_endpoint dpmac_endpoint; diff --git a/drivers/net/ldpaa_eth/ldpaa_wriop.c b/drivers/net/ldpaa_eth/ldpaa_wriop.c index 0731a795c8..afbb1ca91e 100644 --- a/drivers/net/ldpaa_eth/ldpaa_wriop.c +++ b/drivers/net/ldpaa_eth/ldpaa_wriop.c @@ -26,6 +26,7 @@ void wriop_init_dpmac(int sd, int dpmac_id, int
lane_prtcl)
dpmac_info[dpmac_id].enabled = 0; dpmac_info[dpmac_id].id = 0; dpmac_info[dpmac_id].phy_addr = -1;
dpmac_info[dpmac_id].phydev = NULL; dpmac_info[dpmac_id].enet_if = PHY_INTERFACE_MODE_NONE; enet_if = wriop_dpmac_enet_if(dpmac_id, lane_prtcl); @@ -42,6+43,7 @@ void wriop_init_dpmac_enet_if(int dpmac_id, phy_interface_t
enet_if)
dpmac_info[dpmac_id].id = dpmac_id; dpmac_info[dpmac_id].phy_addr = -1; dpmac_info[dpmac_id].enet_if = enet_if;
dpmac_info[dpmac_id].phydev = NULL;}
diff --git a/include/fsl-mc/ldpaa_wriop.h b/include/fsl-mc/ldpaa_wriop.h index 07e5130264..8971c6c55b 100644 --- a/include/fsl-mc/ldpaa_wriop.h +++ b/include/fsl-mc/ldpaa_wriop.h @@ -41,7 +41,6 @@ struct wriop_dpmac_info { u8 id; u8 board_mux; int phy_addr;
void *phy_regs; phy_interface_t enet_if; struct phy_device *phydev; struct mii_dev *bus;-- 2.17.1
U-Boot mailing list U-Boot@lists.denx.de
https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis
ts.denx.de%2Flistinfo%2Fu-
boot&data=02%7C01%7Cpankaj.bansal%40nxp.
com%7Cbced1ce32f8e455148d408d62e64c7f5%7C686ea1d3bc2b4c6fa92c d99c5c301
635%7C0%7C0%7C636747407731429023&sdata=4cwaAt8C4V91TNo iNFGcoznoGEn
POsCGDmh9maFv%2FOA%3D&reserved=0
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot