| Message ID | 20120629055516.11091.82899.stgit@build.warmcat.com |
|---|---|
| State | New |
| Headers | show |
On Friday 29 June 2012, Andy Green wrote: > +static int omap_panda_netdev_event(struct notifier_block *this, > + unsigned long event, void *ptr) > +{ > + struct net_device *dev = ptr; > + struct sockaddr sa; > + int n; > + > + if (event != NETDEV_REGISTER) > + return NOTIFY_DONE; > + > + n = omap_device_path_need_mac(dev->dev.parent); > + if (n < 0) > + return NOTIFY_DONE; > + > + sa.sa_family = dev->type; > + omap2_die_id_to_ethernet_mac(sa.sa_data, n); > + dev->netdev_ops->ndo_set_mac_address(dev, &sa); > + > + return NOTIFY_DONE; > +} > + > +static struct notifier_block omap_panda_netdev_notifier = { > + .notifier_call = omap_panda_netdev_event, > + .priority = 1, > +}; > + > +int omap_register_mac_device_fixup_paths(const char * const *paths, int count) > +{ > + mac_device_fixup_paths = paths; > + count_mac_device_fixup_paths = count; > + > + return register_netdevice_notifier(&omap_panda_netdev_notifier); > +} The omap_panda_netdev_event and omap_panda_netdev_notifier symbols should probably lose the "panda_" part of their names, because they are now located in a common file and are not panda specific any more. Arnd
On 06/29/12 16:51, the mail apparently from Arnd Bergmann included: > On Friday 29 June 2012, Andy Green wrote: >> +static int omap_panda_netdev_event(struct notifier_block *this, >> + unsigned long event, void *ptr) >> +{ >> + struct net_device *dev = ptr; >> + struct sockaddr sa; >> + int n; >> + >> + if (event != NETDEV_REGISTER) >> + return NOTIFY_DONE; >> + >> + n = omap_device_path_need_mac(dev->dev.parent); >> + if (n < 0) >> + return NOTIFY_DONE; >> + >> + sa.sa_family = dev->type; >> + omap2_die_id_to_ethernet_mac(sa.sa_data, n); >> + dev->netdev_ops->ndo_set_mac_address(dev, &sa); >> + >> + return NOTIFY_DONE; >> +} >> + >> +static struct notifier_block omap_panda_netdev_notifier = { >> + .notifier_call = omap_panda_netdev_event, >> + .priority = 1, >> +}; >> + >> +int omap_register_mac_device_fixup_paths(const char * const *paths, int count) >> +{ >> + mac_device_fixup_paths = paths; >> + count_mac_device_fixup_paths = count; >> + >> + return register_netdevice_notifier(&omap_panda_netdev_notifier); >> +} > > The omap_panda_netdev_event and omap_panda_netdev_notifier symbols should probably > lose the "panda_" part of their names, because they are now located in a common > file and are not panda specific any more. Good point thanks, I'll sort it out after waiting for any more comments. -Andy
* Andy Green <andy.green@linaro.org> [120628 22:59]: > From: Andy Green <andy@warmcat.com> > > This exposes a new API in devices.c that lets a board register > a list of device paths representing network devices that have > no arrangements for their MAC address to be set by the board. > > It watches network device registrations via a notifier and > gives the devices requiring them a synthetic - but constant for > a given board - MAC address immediately. ... > --- a/arch/arm/mach-omap2/devices.c > +++ b/arch/arm/mach-omap2/devices.c > @@ -39,6 +42,9 @@ > #define L3_MODULES_MAX_LEN 12 > #define L3_MODULES 3 > > +static const char * const *mac_device_fixup_paths; > +int count_mac_device_fixup_paths; This too should be static it seems to me. Or just make the paths array NULL terminated to get rid of the count. > static int __init omap3_l3_init(void) > { > struct omap_hwmod *oh; > @@ -627,6 +633,89 @@ static void omap_init_vout(void) > static inline void omap_init_vout(void) {} > #endif > > +static int omap_device_path_need_mac(struct device *dev) > +{ > + const char **try = (const char **)mac_device_fixup_paths; This cast you should be able to remove by setting the types right to start with? > + const char *path; > + int count = count_mac_device_fixup_paths; > + const char *p; > + int len; > + struct device *devn; > + > + while (count--) { > + > + p = *try + strlen(*try); > + devn = dev; > + > + while (devn) { > + > + path = dev_name(devn); > + len = strlen(path); > + > + if ((p - *try) < len) { > + devn = NULL; > + continue; > + } > + > + p -= len; > + > + if (strncmp(path, p, len)) { > + devn = NULL; > + continue; > + } > + > + devn = devn->parent; > + if (p == *try) > + return count; > + > + if (devn != NULL && (p - *try) < 2) > + devn = NULL; > + > + p--; > + if (devn != NULL && *p != '/') > + devn = NULL; > + } > + > + try++; > + } > + > + return -ENOENT; > +} I don't quite like having this device parsing code here. This should probably be a generic helper function somewhere under drivers. I would assume other SoCs could use it too? > +static int omap_panda_netdev_event(struct notifier_block *this, > + unsigned long event, void *ptr) > +{ > + struct net_device *dev = ptr; > + struct sockaddr sa; > + int n; > + > + if (event != NETDEV_REGISTER) > + return NOTIFY_DONE; > + > + n = omap_device_path_need_mac(dev->dev.parent); > + if (n < 0) > + return NOTIFY_DONE; > + > + sa.sa_family = dev->type; > + omap2_die_id_to_ethernet_mac(sa.sa_data, n); > + dev->netdev_ops->ndo_set_mac_address(dev, &sa); > + > + return NOTIFY_DONE; > +} > + > +static struct notifier_block omap_panda_netdev_notifier = { > + .notifier_call = omap_panda_netdev_event, > + .priority = 1, > +}; This is a bit similar to platform data callback functions that we are trying to get rid of. And as the question "how do we replace platform data callback functions" is still open for things like this, few questions come to mind that should be discussed: 1. How is this a better solution to passing the generated mac address in platform data to the driver? 2. Is this really how we want to pass the board generated mac addresses and other dynamically generated data to the drivers that are device tree based? 3. What about mac address in board-generic.c when booting panda with device tree? Also, what happens if the user has set the mac address and you replugs the cable or device? Do we now overwrite it? Might be worth checking that this follows the standard behaviour.. Regards, Tony
On 06/29/12 17:40, the mail apparently from Tony Lindgren included: > * Andy Green <andy.green@linaro.org> [120628 22:59]: >> From: Andy Green <andy@warmcat.com> >> >> This exposes a new API in devices.c that lets a board register >> a list of device paths representing network devices that have >> no arrangements for their MAC address to be set by the board. >> >> It watches network device registrations via a notifier and >> gives the devices requiring them a synthetic - but constant for >> a given board - MAC address immediately. > ... > >> --- a/arch/arm/mach-omap2/devices.c >> +++ b/arch/arm/mach-omap2/devices.c >> @@ -39,6 +42,9 @@ >> #define L3_MODULES_MAX_LEN 12 >> #define L3_MODULES 3 >> >> +static const char * const *mac_device_fixup_paths; >> +int count_mac_device_fixup_paths; > > This too should be static it seems to me. Or just make the > paths array NULL terminated to get rid of the count. > >> static int __init omap3_l3_init(void) >> { >> struct omap_hwmod *oh; >> @@ -627,6 +633,89 @@ static void omap_init_vout(void) >> static inline void omap_init_vout(void) {} >> #endif >> >> +static int omap_device_path_need_mac(struct device *dev) >> +{ >> + const char **try = (const char **)mac_device_fixup_paths; > > This cast you should be able to remove by setting the types right > to start with? I guess so, I recall meddling with it and throwing a cast at it. >> + const char *path; >> + int count = count_mac_device_fixup_paths; >> + const char *p; >> + int len; >> + struct device *devn; >> + >> + while (count--) { >> + >> + p = *try + strlen(*try); >> + devn = dev; >> + >> + while (devn) { >> + >> + path = dev_name(devn); >> + len = strlen(path); >> + >> + if ((p - *try) < len) { >> + devn = NULL; >> + continue; >> + } >> + >> + p -= len; >> + >> + if (strncmp(path, p, len)) { >> + devn = NULL; >> + continue; >> + } >> + >> + devn = devn->parent; >> + if (p == *try) >> + return count; >> + >> + if (devn != NULL && (p - *try) < 2) >> + devn = NULL; >> + >> + p--; >> + if (devn != NULL && *p != '/') >> + devn = NULL; >> + } >> + >> + try++; >> + } >> + >> + return -ENOENT; >> +} > > I don't quite like having this device parsing code here. This should > probably be a generic helper function somewhere under drivers. I would > assume other SoCs could use it too? OK... is it a job for drivers/misc or is there a better idea? >> +static int omap_panda_netdev_event(struct notifier_block *this, >> + unsigned long event, void *ptr) >> +{ >> + struct net_device *dev = ptr; >> + struct sockaddr sa; >> + int n; >> + >> + if (event != NETDEV_REGISTER) >> + return NOTIFY_DONE; >> + >> + n = omap_device_path_need_mac(dev->dev.parent); >> + if (n < 0) >> + return NOTIFY_DONE; >> + >> + sa.sa_family = dev->type; >> + omap2_die_id_to_ethernet_mac(sa.sa_data, n); >> + dev->netdev_ops->ndo_set_mac_address(dev, &sa); >> + >> + return NOTIFY_DONE; >> +} >> + >> +static struct notifier_block omap_panda_netdev_notifier = { >> + .notifier_call = omap_panda_netdev_event, >> + .priority = 1, >> +}; > > This is a bit similar to platform data callback functions that we are > trying to get rid of. And as the question "how do we replace platform > data callback functions" is still open for things like this, few > questions come to mind that should be discussed: > > 1. How is this a better solution to passing the generated mac address in > platform data to the driver? Well, I initially did this over a year ago as a generic way to pass platform data to devices that are appearing through deferred or async probing, like USB bus and SDIO bus. It was not understood what the goal was by the people looking after those subsystems. > 2. Is this really how we want to pass the board generated mac addresses > and other dynamically generated data to the drivers that are device > tree based? The issue is that both these busses have an async probe, in the case of USB stack the maintainer was not interested last year in adding platform data. Maybe it changed but that's my understanding. > 3. What about mac address in board-generic.c when booting panda with > device tree? I don't mind adapting it for that case. > Also, what happens if the user has set the mac address and you replugs the > cable or device? Do we now overwrite it? Might be worth checking that this > follows the standard behaviour.. This is only useful for devices that are soldered on your board, and have deterministic "device paths" as is the case with Panda Ethernet and Wlan module that both benefit from this treatment. If you mean pulling the RJ45 cable, it doesn't change anything about the MAC I can confirm. -Andy
* Andy Green <andy.green@linaro.org> [120629 03:12]: > On 06/29/12 17:40, the mail apparently from Tony Lindgren included: > > > >This is a bit similar to platform data callback functions that we are > >trying to get rid of. And as the question "how do we replace platform > >data callback functions" is still open for things like this, few > >questions come to mind that should be discussed: > > > >1. How is this a better solution to passing the generated mac address in > > platform data to the driver? > > Well, I initially did this over a year ago as a generic way to pass > platform data to devices that are appearing through deferred or > async probing, like USB bus and SDIO bus. It was not understood > what the goal was by the people looking after those subsystems. OK.. > >2. Is this really how we want to pass the board generated mac addresses > > and other dynamically generated data to the drivers that are device > > tree based? > > The issue is that both these busses have an async probe, in the case > of USB stack the maintainer was not interested last year in adding > platform data. Maybe it changed but that's my understanding. OK, I'd like to hear Arnds comments on the #2 above too as this is a more generic issue. > >3. What about mac address in board-generic.c when booting panda with > > device tree? > > I don't mind adapting it for that case. Just to try to think about some alternatives, how about something like this: This all could be a driver called soft-mac or something that does what your patches are doing. Except then it would be completely generic and would be able to take device names and mac addresses from platform data or from devicetree. > >Also, what happens if the user has set the mac address and you replugs the > >cable or device? Do we now overwrite it? Might be worth checking that this > >follows the standard behaviour.. > > This is only useful for devices that are soldered on your board, and > have deterministic "device paths" as is the case with Panda Ethernet > and Wlan module that both benefit from this treatment. If you mean > pulling the RJ45 cable, it doesn't change anything about the MAC I > can confirm. OK Tony
On Friday 29 June 2012, Tony Lindgren wrote: > * Andy Green <andy.green@linaro.org> [120629 03:12]: > > >2. Is this really how we want to pass the board generated mac addresses > > > and other dynamically generated data to the drivers that are device > > > tree based? > > > > The issue is that both these busses have an async probe, in the case > > of USB stack the maintainer was not interested last year in adding > > platform data. Maybe it changed but that's my understanding. > > OK, I'd like to hear Arnds comments on the #2 above too as this is > a more generic issue. In case we have a device tree, we should just be using the USB binding to find the specific device node, and add the property there. Then the device driver can use of_get_mac_address() on the usb device itself. I'm not sure what it takes to add the link for the device node in the usb probing code, but my feeling is that it's not too hard. Right now, USB is probed entirely without DT, so the patch is about the best we can do. > > >3. What about mac address in board-generic.c when booting panda with > > > device tree? > > > > I don't mind adapting it for that case. > > Just to try to think about some alternatives, how about something like > this: This all could be a driver called soft-mac or something that does > what your patches are doing. Except then it would be completely generic > and would be able to take device names and mac addresses from platform > data or from devicetree. That driver would be completely generic to all platforms, but be very specific to finding the mac address of a device, as opposed to other properties. I suspect that if we do that, we will still need a way to bind a device_node to a usb device for the purpose of finding other properties, such as external regulators or clocks that are connected to a hardwired USB device. Normally USB tends to just work because the device is expected to be hot-pluggable anyway. If the USB device is soldered to the board, the hardware designers can take some shortcuts Arnd
* Arnd Bergmann <arnd@arndb.de> [120629 06:50]: > On Friday 29 June 2012, Tony Lindgren wrote: > > * Andy Green <andy.green@linaro.org> [120629 03:12]: > > > >2. Is this really how we want to pass the board generated mac addresses > > > > and other dynamically generated data to the drivers that are device > > > > tree based? > > > > > > The issue is that both these busses have an async probe, in the case > > > of USB stack the maintainer was not interested last year in adding > > > platform data. Maybe it changed but that's my understanding. > > > > OK, I'd like to hear Arnds comments on the #2 above too as this is > > a more generic issue. > > In case we have a device tree, we should just be using the USB binding > to find the specific device node, and add the property there. Then > the device driver can use of_get_mac_address() on the usb device itself. But would you generate the mac address then in the bootloader already? > I'm not sure what it takes to add the link for the device node in the > usb probing code, but my feeling is that it's not too hard. > > Right now, USB is probed entirely without DT, so the patch is about > the best we can do. Right, but that still assumes a static mac from the bootloader unless we do a generic driver as below? Or do you have some other ideas? > > > >3. What about mac address in board-generic.c when booting panda with > > > > device tree? > > > > > > I don't mind adapting it for that case. > > > > Just to try to think about some alternatives, how about something like > > this: This all could be a driver called soft-mac or something that does > > what your patches are doing. Except then it would be completely generic > > and would be able to take device names and mac addresses from platform > > data or from devicetree. > > That driver would be completely generic to all platforms, but be > very specific to finding the mac address of a device, as opposed to > other properties. > > I suspect that if we do that, we will still need a way to bind a > device_node to a usb device for the purpose of finding other > properties, such as external regulators or clocks that are connected > to a hardwired USB device. Yes that generic driver could be gind to a usb device and we could use deferred probe to wait for the mac address to be available. > Normally USB tends to just work because the device is expected to > be hot-pluggable anyway. If the USB device is soldered to the > board, the hardware designers can take some shortcuts But that too still depends on the order of loading of the multiple host controller modules. Beagle has both EHCI and musb. Regards, Tony
On 06/29/12 21:55, the mail apparently from Tony Lindgren included: > * Arnd Bergmann <arnd@arndb.de> [120629 06:50]: >> On Friday 29 June 2012, Tony Lindgren wrote: >>> * Andy Green <andy.green@linaro.org> [120629 03:12]: >>>>> 2. Is this really how we want to pass the board generated mac addresses >>>>> and other dynamically generated data to the drivers that are device >>>>> tree based? >>>> >>>> The issue is that both these busses have an async probe, in the case >>>> of USB stack the maintainer was not interested last year in adding >>>> platform data. Maybe it changed but that's my understanding. >>> >>> OK, I'd like to hear Arnds comments on the #2 above too as this is >>> a more generic issue. >> >> In case we have a device tree, we should just be using the USB binding >> to find the specific device node, and add the property there. Then >> the device driver can use of_get_mac_address() on the usb device itself. > > But would you generate the mac address then in the bootloader already? I'm pretty sure correct answer does not introduce more dependencies on bootloader code. >> I'm not sure what it takes to add the link for the device node in the >> usb probing code, but my feeling is that it's not too hard. >> >> Right now, USB is probed entirely without DT, so the patch is about >> the best we can do. > > Right, but that still assumes a static mac from the bootloader unless > we do a generic driver as below? Or do you have some other ideas? What happens without this patch is randomized MAC address assigned by Linux in smsc Ethernet case. -Andy
On 06/29/12 21:45, the mail apparently from Arnd Bergmann included: > On Friday 29 June 2012, Tony Lindgren wrote: >> * Andy Green <andy.green@linaro.org> [120629 03:12]: >>>> 2. Is this really how we want to pass the board generated mac addresses >>>> and other dynamically generated data to the drivers that are device >>>> tree based? >>> >>> The issue is that both these busses have an async probe, in the case >>> of USB stack the maintainer was not interested last year in adding >>> platform data. Maybe it changed but that's my understanding. >> >> OK, I'd like to hear Arnds comments on the #2 above too as this is >> a more generic issue. > > In case we have a device tree, we should just be using the USB binding > to find the specific device node, and add the property there. Then > the device driver can use of_get_mac_address() on the usb device itself. > > I'm not sure what it takes to add the link for the device node in the > usb probing code, but my feeling is that it's not too hard. > > Right now, USB is probed entirely without DT, so the patch is about > the best we can do. Yes none of this was really hard the problem with more generic approach was getting comprehension and not auto-reject at the other subsystems. To be fair USB is USB and DT is Arm-specific issue. >>>> 3. What about mac address in board-generic.c when booting panda with >>>> device tree? >>> >>> I don't mind adapting it for that case. >> >> Just to try to think about some alternatives, how about something like >> this: This all could be a driver called soft-mac or something that does >> what your patches are doing. Except then it would be completely generic >> and would be able to take device names and mac addresses from platform >> data or from devicetree. > > That driver would be completely generic to all platforms, but be > very specific to finding the mac address of a device, as opposed to > other properties. > > I suspect that if we do that, we will still need a way to bind a > device_node to a usb device for the purpose of finding other > properties, such as external regulators or clocks that are connected > to a hardwired USB device. There's no standardized exposure of logical regulators over USB afaik so you won't be able to 'find' them without a VID:PID -bound specific driver that already knew about them. > Normally USB tends to just work because the device is expected to > be hot-pluggable anyway. If the USB device is soldered to the > board, the hardware designers can take some shortcuts Tony's point about modularized host drivers coming in random order seems to be a fair one. We don't build ehci modular so we don't care about it but from maintainer pov it's a legit issue. -Andy
On Friday 29 June 2012, Tony Lindgren wrote: > * Arnd Bergmann <arnd@arndb.de> [120629 06:50]: > > On Friday 29 June 2012, Tony Lindgren wrote: > > > * Andy Green <andy.green@linaro.org> [120629 03:12]: > > > > >2. Is this really how we want to pass the board generated mac addresses > > > > > and other dynamically generated data to the drivers that are device > > > > > tree based? > > > > > > > > The issue is that both these busses have an async probe, in the case > > > > of USB stack the maintainer was not interested last year in adding > > > > platform data. Maybe it changed but that's my understanding. > > > > > > OK, I'd like to hear Arnds comments on the #2 above too as this is > > > a more generic issue. > > > > In case we have a device tree, we should just be using the USB binding > > to find the specific device node, and add the property there. Then > > the device driver can use of_get_mac_address() on the usb device itself. > > But would you generate the mac address then in the bootloader already? Good question. I think we definitely want to have the option of overiding the mac address from the boot loader or a predefined device tree blob. IMHO the precedence should be a) mac-address property from DT if set b) hardcoded address from device EEPROM if present c) platform-provided address, if supported by platform d) random_ether_addr() A lot of devices already provide the mac address in the device tree, wherever that comes from. Ideally that would always be possible but never necessary. > > I'm not sure what it takes to add the link for the device node in the > > usb probing code, but my feeling is that it's not too hard. > > > > Right now, USB is probed entirely without DT, so the patch is about > > the best we can do. > > Right, but that still assumes a static mac from the bootloader unless > we do a generic driver as below? Or do you have some other ideas? The boot loader could theoretically have the same algorithm that Andy's patch implements, unless it is configured to something else by the user. In many cases, you don't want the boot loader to be that smart, but sometimes it is the right solution. > > Normally USB tends to just work because the device is expected to > > be hot-pluggable anyway. If the USB device is soldered to the > > board, the hardware designers can take some shortcuts > > But that too still depends on the order of loading of the multiple > host controller modules. Beagle has both EHCI and musb. In the device tree, we can uniquely identify the USB hosts, that is not a problem here. The problem exists only if you base your path on the linux specific name for the devices. It's definitely ugly to rely on that, but I think we can actually guarantee that it's stable as long as we enforce in the Kconfig/Kbuild that the controller that has the ethernet device is always built-in on Panda and comes before the other one if both are built-in. I would much prefer not to have to rely on those things, but I believe it is actually guaranteed at the moment for the cases that matter. While you could have both controllers as loadable modules and load them in reverse order that would also imply that you cannot actually boot from the network, so you know that you have a file system on which you can set the mac address of the device using udev, before you start using networking. Arnd
On Friday 29 June 2012, Andy Green wrote: > On 06/29/12 21:45, the mail apparently from Arnd Bergmann included: > > On Friday 29 June 2012, Tony Lindgren wrote: > > In case we have a device tree, we should just be using the USB binding > > to find the specific device node, and add the property there. Then > > the device driver can use of_get_mac_address() on the usb device itself. > > > > I'm not sure what it takes to add the link for the device node in the > > usb probing code, but my feeling is that it's not too hard. > > > > Right now, USB is probed entirely without DT, so the patch is about > > the best we can do. > > Yes none of this was really hard the problem with more generic approach > was getting comprehension and not auto-reject at the other subsystems. > To be fair USB is USB and DT is Arm-specific issue. I don't understand. I'm sure that there are powerpc, x86 or mips systems with DT that have hardwired USB ports, so it's just a matter of time until someone forgets to stick an EEPROM on the network port of one of those. > >>>> 3. What about mac address in board-generic.c when booting panda with > >>>> device tree? > >>> > >>> I don't mind adapting it for that case. > >> > >> Just to try to think about some alternatives, how about something like > >> this: This all could be a driver called soft-mac or something that does > >> what your patches are doing. Except then it would be completely generic > >> and would be able to take device names and mac addresses from platform > >> data or from devicetree. > > > > That driver would be completely generic to all platforms, but be > > very specific to finding the mac address of a device, as opposed to > > other properties. > > > > I suspect that if we do that, we will still need a way to bind a > > device_node to a usb device for the purpose of finding other > > properties, such as external regulators or clocks that are connected > > to a hardwired USB device. > > There's no standardized exposure of logical regulators over USB afaik so > you won't be able to 'find' them without a VID:PID -bound specific > driver that already knew about them. The point is that as soon as we have implement the standard DT bindings for USB, we get a way to do this. I would assume that someone has stumbled over this issue before and just added a hack to their driver to hardwire some regulator, rather than doing it properly. > > Normally USB tends to just work because the device is expected to > > be hot-pluggable anyway. If the USB device is soldered to the > > board, the hardware designers can take some shortcuts > > Tony's point about modularized host drivers coming in random order seems > to be a fair one. We don't build ehci modular so we don't care about it > but from maintainer pov it's a legit issue. Right, it's definitely ugly. The DT binding would take care of it because it gives us a way to identify the device, but I think it also makes sense to have your patches for the common case that ehci is built-in. Arnd
* Arnd Bergmann <arnd@arndb.de> [120629 07:37]: > On Friday 29 June 2012, Tony Lindgren wrote: > > * Arnd Bergmann <arnd@arndb.de> [120629 06:50]: > > > On Friday 29 June 2012, Tony Lindgren wrote: > > > > * Andy Green <andy.green@linaro.org> [120629 03:12]: > > > > > >2. Is this really how we want to pass the board generated mac addresses > > > > > > and other dynamically generated data to the drivers that are device > > > > > > tree based? > > > > > > > > > > The issue is that both these busses have an async probe, in the case > > > > > of USB stack the maintainer was not interested last year in adding > > > > > platform data. Maybe it changed but that's my understanding. > > > > > > > > OK, I'd like to hear Arnds comments on the #2 above too as this is > > > > a more generic issue. > > > > > > In case we have a device tree, we should just be using the USB binding > > > to find the specific device node, and add the property there. Then > > > the device driver can use of_get_mac_address() on the usb device itself. > > > > But would you generate the mac address then in the bootloader already? > > Good question. I think we definitely want to have the option of overiding > the mac address from the boot loader or a predefined device tree blob. > > IMHO the precedence should be > > a) mac-address property from DT if set > b) hardcoded address from device EEPROM if present > c) platform-provided address, if supported by platform > d) random_ether_addr() And all should be naturally settable with ifconfig too without being overwritten by the notifier code :) > A lot of devices already provide the mac address in the device tree, > wherever that comes from. Ideally that would always be possible but > never necessary. > > > > I'm not sure what it takes to add the link for the device node in the > > > usb probing code, but my feeling is that it's not too hard. > > > > > > Right now, USB is probed entirely without DT, so the patch is about > > > the best we can do. > > > > Right, but that still assumes a static mac from the bootloader unless > > we do a generic driver as below? Or do you have some other ideas? > > The boot loader could theoretically have the same algorithm that Andy's > patch implements, unless it is configured to something else by the > user. In many cases, you don't want the boot loader to be that smart, > but sometimes it is the right solution. OK that would mean bootloader modifying the .dtb then. > > > Normally USB tends to just work because the device is expected to > > > be hot-pluggable anyway. If the USB device is soldered to the > > > board, the hardware designers can take some shortcuts > > > > But that too still depends on the order of loading of the multiple > > host controller modules. Beagle has both EHCI and musb. Sorry I meant Panda instead of Beagle here.. > In the device tree, we can uniquely identify the USB hosts, that > is not a problem here. The problem exists only if you base your > path on the linux specific name for the devices. > It's definitely ugly to rely on that, but I think we can actually > guarantee that it's stable as long as we enforce in the > Kconfig/Kbuild that the controller that has the ethernet device > is always built-in on Panda and comes before the other one if > both are built-in. I would much prefer not to have to rely on > those things, but I believe it is actually guaranteed at the > moment for the cases that matter. While you could have both > controllers as loadable modules and load them in reverse > order that would also imply that you cannot actually boot from > the network, so you know that you have a file system on which > you can set the mac address of the device using udev, before > you start using networking. OK. So how about we do the following: 1. Make Andy's notifier part into a generic mac notifier driver that can take platform data for the driver name mac address pair. Devicetree binding could be added as needed. 2. Pass the Panda mac information as platform data to this driver for now with a comment on the usb path naming being potentially wrong in the loadable modules case. 3. Add devicetree support to the driver once the USB binding is available. That will make my life much easier as then I'm not stuck with more legacy code to deal with ;) Also writing the driver part should be trivial as all the code is there already. Regards, Tony
On Friday 29 June 2012, Tony Lindgren wrote: > > A lot of devices already provide the mac address in the device tree, > > wherever that comes from. Ideally that would always be possible but > > never necessary. > > > > > > I'm not sure what it takes to add the link for the device node in the > > > > usb probing code, but my feeling is that it's not too hard. > > > > > > > > Right now, USB is probed entirely without DT, so the patch is about > > > > the best we can do. > > > > > > Right, but that still assumes a static mac from the bootloader unless > > > we do a generic driver as below? Or do you have some other ideas? > > > > The boot loader could theoretically have the same algorithm that Andy's > > patch implements, unless it is configured to something else by the > > user. In many cases, you don't want the boot loader to be that smart, > > but sometimes it is the right solution. > > OK that would mean bootloader modifying the .dtb then. Right. > > In the device tree, we can uniquely identify the USB hosts, that > > is not a problem here. The problem exists only if you base your > > path on the linux specific name for the devices. > > It's definitely ugly to rely on that, but I think we can actually > > guarantee that it's stable as long as we enforce in the > > Kconfig/Kbuild that the controller that has the ethernet device > > is always built-in on Panda and comes before the other one if > > both are built-in. I would much prefer not to have to rely on > > those things, but I believe it is actually guaranteed at the > > moment for the cases that matter. While you could have both > > controllers as loadable modules and load them in reverse > > order that would also imply that you cannot actually boot from > > the network, so you know that you have a file system on which > > you can set the mac address of the device using udev, before > > you start using networking. > > OK. So how about we do the following: > > 1. Make Andy's notifier part into a generic mac notifier > driver that can take platform data for the driver name > mac address pair. Devicetree binding could be added as > needed. I don't think the devicetree bindign should be part of that driver but instead should go into the individual network device drivers, otherwise it sounds ok to me. Obviously, this still needs buy-in from the networking people. > 2. Pass the Panda mac information as platform data to this > driver for now with a comment on the usb path naming being > potentially wrong in the loadable modules case. IMHO code outside of the platform driver world would be more appropriate here. It's not actually a platform device because it's more of an abstract concept to define a mac address than physical hardware. A new file with a single global function in net/ethernet/ that you can use to register a callback sounds like the easiest approach to me. > 3. Add devicetree support to the driver once the USB binding > is available. Here you mean the smsc95xx driver, not the new code, right? > That will make my life much easier as then I'm not stuck with > more legacy code to deal with ;) Also writing the driver part > should be trivial as all the code is there already. Sounds ok to me, if we can get Andy to reshuffle his code once more ;-) Arnd
* Arnd Bergmann <arnd@arndb.de> [120629 08:09]: > On Friday 29 June 2012, Tony Lindgren wrote: > > > > OK. So how about we do the following: > > > > 1. Make Andy's notifier part into a generic mac notifier > > driver that can take platform data for the driver name > > mac address pair. Devicetree binding could be added as > > needed. > > I don't think the devicetree bindign should be part of that > driver but instead should go into the individual network > device drivers, otherwise it sounds ok to me. > Obviously, this still needs buy-in from the networking > people. Yes you're right. The fixed bootloader generated mac address should be passed directly to the ethernet driver in question. > > 2. Pass the Panda mac information as platform data to this > > driver for now with a comment on the usb path naming being > > potentially wrong in the loadable modules case. > > IMHO code outside of the platform driver world would be more > appropriate here. It's not actually a platform device because > it's more of an abstract concept to define a mac address than > physical hardware. Well we still need to also pass the mac address generated by the SoC specific kernel init code. It seems that platform data would be the obvious way to pass that. Or do you have some other way in mind for that? > A new file with a single global function in net/ethernet/ > that you can use to register a callback sounds like the > easiest approach to me. Yes I agree this should live under net/ethernet somewhere. > > 3. Add devicetree support to the driver once the USB binding > > is available. > > Here you mean the smsc95xx driver, not the new code, right? Yes you're right, we only need the mac address binding for the smsc95xx driver. And in addition to that we need to support passing the SoC init code generated mac address somewhere to the whatever this new soft-mac driver will be called. Regards, Tony
On Sunday 01 July 2012, Tony Lindgren wrote: > > > 2. Pass the Panda mac information as platform data to this > > > driver for now with a comment on the usb path naming being > > > potentially wrong in the loadable modules case. > > > > IMHO code outside of the platform driver world would be more > > appropriate here. It's not actually a platform device because > > it's more of an abstract concept to define a mac address than > > physical hardware. > > Well we still need to also pass the mac address generated by > the SoC specific kernel init code. It seems that platform data > would be the obvious way to pass that. Or do you have some other > way in mind for that? My point is that for platform data you need a platform device of some sort, but this new piece of infrastructure does not look like it should be a device. I think a reasonable interface would be something as simple as void register_eth_mac_fixup(const char *path, const u8 *mac); Instead of registering a device from the platform, we just call this function, and leave the code built-in. Arnd
* Arnd Bergmann <arnd@arndb.de> [120702 03:24]: > On Sunday 01 July 2012, Tony Lindgren wrote: > > > > 2. Pass the Panda mac information as platform data to this > > > > driver for now with a comment on the usb path naming being > > > > potentially wrong in the loadable modules case. > > > > > > IMHO code outside of the platform driver world would be more > > > appropriate here. It's not actually a platform device because > > > it's more of an abstract concept to define a mac address than > > > physical hardware. > > > > Well we still need to also pass the mac address generated by > > the SoC specific kernel init code. It seems that platform data > > would be the obvious way to pass that. Or do you have some other > > way in mind for that? > > My point is that for platform data you need a platform device of > some sort, but this new piece of infrastructure does not look > like it should be a device. OK > I think a reasonable interface would be something as simple as > > void register_eth_mac_fixup(const char *path, const u8 *mac); > > Instead of registering a device from the platform, we just call > this function, and leave the code built-in. Sounds good to me. Tony
diff --git a/arch/arm/mach-omap2/common.h b/arch/arm/mach-omap2/common.h index 5d99c1b..ae4d8a9 100644 --- a/arch/arm/mach-omap2/common.h +++ b/arch/arm/mach-omap2/common.h @@ -116,6 +116,8 @@ static inline int omap_mux_late_init(void) #endif extern void omap2_init_common_infrastructure(void); +extern int omap_register_mac_device_fixup_paths(const char * const *paths, + int count); extern struct sys_timer omap2_timer; extern struct sys_timer omap3_timer; diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c index 91ef6699..d0a3d2d 100644 --- a/arch/arm/mach-omap2/devices.c +++ b/arch/arm/mach-omap2/devices.c @@ -18,12 +18,15 @@ #include <linux/slab.h> #include <linux/of.h> #include <linux/platform_data/omap4-keypad.h> +#include <linux/netdevice.h> +#include <linux/if_ether.h> #include <mach/hardware.h> #include <mach/irqs.h> #include <asm/mach-types.h> #include <asm/mach/map.h> #include <asm/pmu.h> +#include <mach/id.h> #include "iomap.h" #include <plat/board.h> @@ -39,6 +42,9 @@ #define L3_MODULES_MAX_LEN 12 #define L3_MODULES 3 +static const char * const *mac_device_fixup_paths; +int count_mac_device_fixup_paths; + static int __init omap3_l3_init(void) { struct omap_hwmod *oh; @@ -627,6 +633,89 @@ static void omap_init_vout(void) static inline void omap_init_vout(void) {} #endif +static int omap_device_path_need_mac(struct device *dev) +{ + const char **try = (const char **)mac_device_fixup_paths; + const char *path; + int count = count_mac_device_fixup_paths; + const char *p; + int len; + struct device *devn; + + while (count--) { + + p = *try + strlen(*try); + devn = dev; + + while (devn) { + + path = dev_name(devn); + len = strlen(path); + + if ((p - *try) < len) { + devn = NULL; + continue; + } + + p -= len; + + if (strncmp(path, p, len)) { + devn = NULL; + continue; + } + + devn = devn->parent; + if (p == *try) + return count; + + if (devn != NULL && (p - *try) < 2) + devn = NULL; + + p--; + if (devn != NULL && *p != '/') + devn = NULL; + } + + try++; + } + + return -ENOENT; +} + +static int omap_panda_netdev_event(struct notifier_block *this, + unsigned long event, void *ptr) +{ + struct net_device *dev = ptr; + struct sockaddr sa; + int n; + + if (event != NETDEV_REGISTER) + return NOTIFY_DONE; + + n = omap_device_path_need_mac(dev->dev.parent); + if (n < 0) + return NOTIFY_DONE; + + sa.sa_family = dev->type; + omap2_die_id_to_ethernet_mac(sa.sa_data, n); + dev->netdev_ops->ndo_set_mac_address(dev, &sa); + + return NOTIFY_DONE; +} + +static struct notifier_block omap_panda_netdev_notifier = { + .notifier_call = omap_panda_netdev_event, + .priority = 1, +}; + +int omap_register_mac_device_fixup_paths(const char * const *paths, int count) +{ + mac_device_fixup_paths = paths; + count_mac_device_fixup_paths = count; + + return register_netdevice_notifier(&omap_panda_netdev_notifier); +} + /*-------------------------------------------------------------------------*/ static int __init omap2_init_devices(void)