| Message ID | 20230624121211.19711-1-mans@mansr.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND] net: cpsw: fix obtaining mac address for am3517 | expand |
On Sat, Jun 24, 2023 at 01:10:59PM +0100, Mans Rullgard wrote: > From: Jeroen Hofstee <jhofstee@victronenergy.com> > > Commit b6745f6e4e63 ("drivers: net: cpsw: davinci_emac: move reading mac > id to common file") did not only move the code for an am3517, it also > added the slave parameter, resulting in an invalid (all zero) mac address > being returned for an am3517, since it only has a single emac Hi Mans If there is only a single emac, why is the function being called with slave=1? Given the description, it seems like you are fixing the wrong problem. Andrew
Andrew Lunn <andrew@lunn.ch> writes: > On Sat, Jun 24, 2023 at 01:10:59PM +0100, Mans Rullgard wrote: >> From: Jeroen Hofstee <jhofstee@victronenergy.com> >> >> Commit b6745f6e4e63 ("drivers: net: cpsw: davinci_emac: move reading mac >> id to common file") did not only move the code for an am3517, it also >> added the slave parameter, resulting in an invalid (all zero) mac address >> being returned for an am3517, since it only has a single emac > > Hi Mans > > If there is only a single emac, why is the function being called with > slave=1? Given the description, it seems like you are fixing the wrong > problem. See previous discussion: https://lore.kernel.org/lkml/d8ad5cab-5183-cddf-fa9a-4e7b9b8c9377@victronenergy.com/
On Sat, Jun 24, 2023 at 03:24:47PM +0100, Måns Rullgård wrote: > Andrew Lunn <andrew@lunn.ch> writes: > > > On Sat, Jun 24, 2023 at 01:10:59PM +0100, Mans Rullgard wrote: > >> From: Jeroen Hofstee <jhofstee@victronenergy.com> > >> > >> Commit b6745f6e4e63 ("drivers: net: cpsw: davinci_emac: move reading mac > >> id to common file") did not only move the code for an am3517, it also > >> added the slave parameter, resulting in an invalid (all zero) mac address > >> being returned for an am3517, since it only has a single emac > > > > Hi Mans > > > > If there is only a single emac, why is the function being called with > > slave=1? Given the description, it seems like you are fixing the wrong > > problem. > > See previous discussion: > https://lore.kernel.org/lkml/d8ad5cab-5183-cddf-fa9a-4e7b9b8c9377@victronenergy.com/ Hi Måns O.K. did i mention memory of a goldfish? This is the sort of detail that should be in the commit message. Otherwise reviewers are going to ask for an explanation, even if it has been explained once, 6 years ago. I assume you also want this back ported to stable? Please add a Fixed: tag, and a Cc: stable@vger.kernel.org tag. And set the patch subject to [PATCH net v3] to indicate this is for the net tree, not net-next. Thanks Andrew
Andrew Lunn <andrew@lunn.ch> writes: > On Sat, Jun 24, 2023 at 03:24:47PM +0100, Måns Rullgård wrote: >> Andrew Lunn <andrew@lunn.ch> writes: >> >> > On Sat, Jun 24, 2023 at 01:10:59PM +0100, Mans Rullgard wrote: >> >> From: Jeroen Hofstee <jhofstee@victronenergy.com> >> >> >> >> Commit b6745f6e4e63 ("drivers: net: cpsw: davinci_emac: move reading mac >> >> id to common file") did not only move the code for an am3517, it also >> >> added the slave parameter, resulting in an invalid (all zero) mac address >> >> being returned for an am3517, since it only has a single emac >> > >> > Hi Mans >> > >> > If there is only a single emac, why is the function being called with >> > slave=1? Given the description, it seems like you are fixing the wrong >> > problem. >> >> See previous discussion: >> https://lore.kernel.org/lkml/d8ad5cab-5183-cddf-fa9a-4e7b9b8c9377@victronenergy.com/ > > Hi Måns > > O.K. did i mention memory of a goldfish? > > This is the sort of detail that should be in the commit > message. Otherwise reviewers are going to ask for an explanation, even > if it has been explained once, 6 years ago. > > I assume you also want this back ported to stable? Please add a Fixed: > tag, and a Cc: stable@vger.kernel.org tag. And set the patch subject > to [PATCH net v3] to indicate this is for the net tree, not net-next. I give up. It's not worth my time. This is why people hoard patches rather than sending them upstream.
Hi, * Måns Rullgård <mans@mansr.com> [230624 14:36]: > Andrew Lunn <andrew@lunn.ch> writes: > > I assume you also want this back ported to stable? Please add a Fixed: > > tag, and a Cc: stable@vger.kernel.org tag. And set the patch subject > > to [PATCH net v3] to indicate this is for the net tree, not net-next. > > I give up. It's not worth my time. This is why people hoard patches > rather than sending them upstream. Maybe just give it one more go filing the proper paperwork :) It would be nice to have it in stable too so IMO it's worth the few more hoops to addthe tags for automating picking it to stable kernels. Regards, Tony
diff --git a/drivers/net/ethernet/ti/cpsw-common.c b/drivers/net/ethernet/ti/cpsw-common.c index bfa81bbfce3f..465dc15f059d 100644 --- a/drivers/net/ethernet/ti/cpsw-common.c +++ b/drivers/net/ethernet/ti/cpsw-common.c @@ -74,8 +74,12 @@ int ti_cm_get_macid(struct device *dev, int slave, u8 *mac_addr) if (of_machine_is_compatible("ti,am33xx")) return cpsw_am33xx_cm_get_macid(dev, 0x630, slave, mac_addr); + /* + * There is only one emac / mac address on an am3517 so ignore the + * slave = 1 and always get the macid from slave 0. + */ if (of_device_is_compatible(dev->of_node, "ti,am3517-emac")) - return davinci_emac_3517_get_macid(dev, 0x110, slave, mac_addr); + return davinci_emac_3517_get_macid(dev, 0x110, 0, mac_addr); if (of_device_is_compatible(dev->of_node, "ti,dm816-emac")) return cpsw_am33xx_cm_get_macid(dev, 0x30, slave, mac_addr);