| Message ID | 1556024950-20752-1-git-send-email-grygorii.strashko@ti.com |
|---|---|
| Headers | show |
| Series | net: ethernet: ti: clean up and optimizations | expand |
On Tue, Apr 23, 2019 at 04:08:54PM +0300, Grygorii Strashko wrote: > index 1142fcfce566..3ef5257c9bc7 100644 > --- a/drivers/net/ethernet/ti/cpsw_ale.c > +++ b/drivers/net/ethernet/ti/cpsw_ale.c > > MODULE_LICENSE("GPL v2"); > MODULE_DESCRIPTION("TI CPSW ALE driver"); Hi Grygorii If this is no longer a module, the MODULE_* macros don't make much sense. Andrew
>On Tue, Apr 23, 2019 at 04:08:56PM +0300, Grygorii Strashko wrote: > Use local variable struct device *dev in probe to simplify code. > > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > --- > drivers/net/ethernet/ti/cpsw.c | 65 +++++++++++++++++----------------- > 1 file changed, 33 insertions(+), 32 deletions(-) > > diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c > index 13a339c64892..0b8cc4e6d9cd 100644 > --- a/drivers/net/ethernet/ti/cpsw.c > +++ b/drivers/net/ethernet/ti/cpsw.c > @@ -3458,6 +3458,7 @@ static const struct soc_device_attribute cpsw_soc_devices[] = { > > static int cpsw_probe(struct platform_device *pdev) > { > + struct device *dev = &pdev->dev; > struct clk *clk; > struct cpsw_platform_data *data; > struct net_device *ndev; It seems odd not to keep with the column layout? Plus it would make it reverse Christmas tree. Andrew
On Tue, Apr 23, 2019 at 04:08:57PM +0300, Grygorii Strashko wrote: > Drop pinctrl_pm_select_default_state call from probe as default > pinctrl state is set by DD core. > > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Tue, Apr 23, 2019 at 04:08:59PM +0300, Grygorii Strashko wrote: > Drop unnecessary wrapper function cpsw_tx_packet_submit() which is used > only in one place. > > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Tue, Apr 23, 2019 at 04:09:01PM +0300, Grygorii Strashko wrote: > Use ALE_PORT_HOST define for host port in cpsw_ale_set_allmulti() instead > of constants. > > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Tue, Apr 23, 2019 at 04:09:07PM +0300, Grygorii Strashko wrote: > + ret = cpsw_init_common(cpsw, ss_regs, ale_ageout, > + (u32 __force)ss_res->start + CPSW2_BD_OFFSET, > + descs_pool_size); Hi Grygorii Could you put it onto your TODO list to remove this nasty casting. Thanks Andrew
On 23.04.19 21:40, Andrew Lunn wrote: >> On Tue, Apr 23, 2019 at 04:08:56PM +0300, Grygorii Strashko wrote: >> Use local variable struct device *dev in probe to simplify code. >> >> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >> --- >> drivers/net/ethernet/ti/cpsw.c | 65 +++++++++++++++++----------------- >> 1 file changed, 33 insertions(+), 32 deletions(-) >> >> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c >> index 13a339c64892..0b8cc4e6d9cd 100644 >> --- a/drivers/net/ethernet/ti/cpsw.c >> +++ b/drivers/net/ethernet/ti/cpsw.c >> @@ -3458,6 +3458,7 @@ static const struct soc_device_attribute cpsw_soc_devices[] = { >> >> static int cpsw_probe(struct platform_device *pdev) >> { >> + struct device *dev = &pdev->dev; >> struct clk *clk; >> struct cpsw_platform_data *data; >> struct net_device *ndev; > > It seems odd not to keep with the column layout? > Plus it would make it reverse Christmas tree. It's has never been optimized for Christmas tree and I'll keep cur layout for now. -- Best regards, grygorii
On 24.04.19 03:27, Jakub Kicinski wrote: > On Tue, 23 Apr 2019 16:08:53 +0300, Grygorii Strashko wrote: >> Both drivers CPSW and EMAC can't work without CPDMA, hence simplify build >> of those drivers by always linking davinci_cpdma and drop TI_DAVINCI_CPDMA >> config option. >> Note. the davinci_emac driver module was changed to "ti_davinci_emac" to >> make build work. >> >> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > > Last time I tried including the same object file in two different > modules it backfired pretty badly. Would you mind sparing some > details as to why you need to do this? I've tried it as modules and inbuid and see no issues. Issue with build happens if EXPORT_SYMBOL_GPL() not removed. It's not expected and not supported to load more then one TI Net driver at same hw, so modules also are just be loaded fine. Why? It's to get rid of CONFIG options which were historically added, but are not required now (and even can't be disabled of if disabled will cause build failures) - result simplified build/Kconfig and code. -- Best regards, grygorii
Andrew, On 23.04.19 16:58, Andrew Lunn wrote: > On Tue, Apr 23, 2019 at 04:08:52PM +0300, Grygorii Strashko wrote: >> +// SPDX-License-Identifier: GPL-2.0 >> /* >> * Copyright (C) 2006, 2007 Eugene Konev >> * >> - * This program is free software; you can redistribute it and/or modify >> - * it under the terms of the GNU General Public License as published by >> - * the Free Software Foundation; either version 2 of the License, or >> - * (at your option) any later version. > > Your SPDX says version 2. The text says version 2 or later. You need > to add a + . Thanks for your comments. I'll update and resend. -- Best regards, grygorii
On 24.04.19 15:22, Ivan Khoronzhuk wrote: > On Tue, Apr 23, 2019 at 04:09:02PM +0300, Grygorii Strashko wrote: >> Now CPSW ALE will set/clean Host port bit in Unregistered Multicast Flood >> Mask (UNREG_MCAST_FLOOD_MASK) for every VLAN without checking if this port >> belongs to VLAN or not when ALLMULTI mode flag is set for nedev. This is >> working in non dual_mac mode, but in dual_mac - it causes >> enabling/disabling ALLMULTI flag for both ports. >> >> Hence fix it by adding additional parameter to cpsw_ale_set_allmulti() to >> specify ALE port number for which ALLMULTI has to be enabled and check if >> port belongs to VLAN before modifying UNREG_MCAST_FLOOD_MASK. >> >> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >> --- >> drivers/net/ethernet/ti/cpsw.c | 12 +++++++++--- >> drivers/net/ethernet/ti/cpsw_ale.c | 12 ++++++++++-- >> drivers/net/ethernet/ti/cpsw_ale.h | 2 +- >> 3 files changed, 20 insertions(+), 6 deletions(-) >> > [...] >> + vlan_members = >> + cpsw_ale_get_vlan_member_list(ale_entry, >> + ale->vlan_field_bits); > Seems like declaration for cpsw_ale_get_vlan_member_list() is not > part of the patchset > > It's define :) DEFINE_ALE_FIELD1(vlan_member_list, 0) -- Best regards, grygorii
On 25.04.19 16:38, Murali Karicheri wrote: > On 04/23/2019 09:09 AM, Grygorii Strashko wrote: >> move common hw init code in separate function as preparation for adding new >> switchdev driver. > All of the changes indicates a better name for the file is cpsw_common.c; commit log, new function cpsw_init_common() etc. Any specific reason to use cpsw_priv.[ch]? I've been thinking about this, but cpsw_coomon.c is used by davinici_emac also. More over, I have a hope that it'd be possible to get rid of cpsw_common.c at all by switching to nvmem. Also, I'd be very appreciated if you'd be able to cut your replies. It's a little bit hard to find your comments > >> >> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >> --- >> drivers/net/ethernet/ti/Makefile | 2 +- >> drivers/net/ethernet/ti/cpsw.c | 106 ++--------------------- >> drivers/net/ethernet/ti/cpsw_priv.c | 128 ++++++++++++++++++++++++++++ >> drivers/net/ethernet/ti/cpsw_priv.h | 3 + >> 4 files changed, 137 insertions(+), 102 deletions(-) >> create mode 100644 drivers/net/ethernet/ti/cpsw_priv.c >> >> diff --git a/drivers/net/ethernet/ti/Makefile b/drivers/net/ethernet/ti/Makefile >> index 2f159a71f88e..de1561596646 100644 ... -- Best regards, grygorii