Message ID | 20161125153032.14617-13-oliver@schinagl.nl |
---|---|
State | Changes Requested |
Delegated to: | Joe Hershberger |
Headers | show |
Hi Olliver On Fri, 2016-11-25 at 16:30 +0100, Olliver Schinagl wrote: > This patch adds a method for the board to set the MAC address if the > environment is not yet set. The environment based MAC addresses are > not > touched, but if the fdt has an alias set, it is parsed and put into > the > environment. > > E.g. The environment contains ethaddr and eth1addr, and the fdt > contains > an ethernet1 nothing happens. If the fdt contains ethernet2 however, > it > is parsed and inserted into the environment as eth2addr. My humble understanding of device tree fixup is that it works the other way around (e.g. it is the device tree that usually gets fixed up). So the least I would advice for this patch is to change its naming but most possibly such code also does not belong into the common fdt_support implementation. > Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> > --- > common/fdt_support.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/common/fdt_support.c b/common/fdt_support.c > index c34a13c..f127392 100644 > --- a/common/fdt_support.c > +++ b/common/fdt_support.c > @@ -465,6 +465,11 @@ int fdt_fixup_memory(void *blob, u64 start, u64 > size) > return fdt_fixup_memory_banks(blob, &start, &size, 1); > } > > +__weak int board_get_enetaddr(const int i, unsigned char *mac_addr) > +{ > + return -ENOSYS; > +} > + > void fdt_fixup_ethernet(void *fdt) > { > int i, prop; > @@ -507,7 +512,8 @@ void fdt_fixup_ethernet(void *fdt) > if (fdt_eth_addr) > eth_parse_enetaddr(fdt_eth_addr, > mac_addr); > else > - continue; > + if (board_get_enetaddr(i, mac_addr) > < 0) > + continue; > > do_fixup_by_path(fdt, path, "mac-address", > &mac_addr, 6, 0); Cheers Marcel
On Wed, Nov 30, 2016 at 09:00:51AM +0000, Marcel Ziswiler wrote: > Hi Olliver > > On Fri, 2016-11-25 at 16:30 +0100, Olliver Schinagl wrote: > > This patch adds a method for the board to set the MAC address if the > > environment is not yet set. The environment based MAC addresses are > > not > > touched, but if the fdt has an alias set, it is parsed and put into > > the > > environment. > > > > E.g. The environment contains ethaddr and eth1addr, and the fdt > > contains > > an ethernet1 nothing happens. If the fdt contains ethernet2 however, > > it > > is parsed and inserted into the environment as eth2addr. > > My humble understanding of device tree fixup is that it works the other > way around (e.g. it is the device tree that usually gets fixed up). So > the least I would advice for this patch is to change its naming but > most possibly such code also does not belong into the common > fdt_support implementation. I don't really have the context of this patch, but in the DT at least, you can specify the mac address using the local-mac-address property. I guess we should honor that too. But I don't really know how that's related to an alias. If the device is probed and the property is there, use it, otherwise don't. Maxime
Hey maime, Sorry for constantly getting your e-mail address wrong! Sorry! On 30-11-16 10:12, maxime.ripard@free-electrons.com wrote: > On Wed, Nov 30, 2016 at 09:00:51AM +0000, Marcel Ziswiler wrote: >> Hi Olliver >> >> On Fri, 2016-11-25 at 16:30 +0100, Olliver Schinagl wrote: >>> This patch adds a method for the board to set the MAC address if the >>> environment is not yet set. The environment based MAC addresses are >>> not >>> touched, but if the fdt has an alias set, it is parsed and put into >>> the >>> environment. >>> >>> E.g. The environment contains ethaddr and eth1addr, and the fdt >>> contains >>> an ethernet1 nothing happens. If the fdt contains ethernet2 however, >>> it >>> is parsed and inserted into the environment as eth2addr. >> My humble understanding of device tree fixup is that it works the other >> way around (e.g. it is the device tree that usually gets fixed up). So >> the least I would advice for this patch is to change its naming but >> most possibly such code also does not belong into the common >> fdt_support implementation. First, yes I noticed this as well, the nameing is the wrong way around. It took me a few reading times as well. I guess I did not full understand Hans's suggestion comment. So there's some work needed here. > I don't really have the context of this patch, but in the DT at least, > you can specify the mac address using the local-mac-address > property. I guess we should honor that too. But I don't really know > how that's related to an alias. If the device is probed and the > property is there, use it, otherwise don't. This came from the sdio_realtek module on some sunxi boards, where the device tree has configured it obviously for linux, but u-boot has no notion of it. But u-boot IS responsible to generate (the same) MAC address for the device. So yes, u-boot inserts a mac address into the device-tree for a found alias. E.g. ethernet2 is an alias without a MAC address configured for it. Thus u-boot is used to generate the MAC address for this node and inserts it into the dtb. What I haven't double checked (just blindly assumed, which is the mother) if this code also inserts the mac into the environment, but the kernel only gets this information via the dtb anyway, right? Either via the dtb, or via the MAC register bytes where it is stored in some drivers. olliver > > Maxime >
Hi, On 30-11-16 11:50, Olliver Schinagl wrote: > Hey maime, > > Sorry for constantly getting your e-mail address wrong! Sorry! > > On 30-11-16 10:12, maxime.ripard@free-electrons.com wrote: >> On Wed, Nov 30, 2016 at 09:00:51AM +0000, Marcel Ziswiler wrote: >>> Hi Olliver >>> >>> On Fri, 2016-11-25 at 16:30 +0100, Olliver Schinagl wrote: >>>> This patch adds a method for the board to set the MAC address if the >>>> environment is not yet set. The environment based MAC addresses are >>>> not >>>> touched, but if the fdt has an alias set, it is parsed and put into >>>> the >>>> environment. >>>> >>>> E.g. The environment contains ethaddr and eth1addr, and the fdt >>>> contains >>>> an ethernet1 nothing happens. If the fdt contains ethernet2 however, >>>> it >>>> is parsed and inserted into the environment as eth2addr. >>> My humble understanding of device tree fixup is that it works the other >>> way around (e.g. it is the device tree that usually gets fixed up). So >>> the least I would advice for this patch is to change its naming but >>> most possibly such code also does not belong into the common >>> fdt_support implementation. > First, yes I noticed this as well, the nameing is the wrong way around. It took me a few reading times as well. I guess I did not full understand Hans's suggestion comment. So there's some work needed here. >> I don't really have the context of this patch, but in the DT at least, >> you can specify the mac address using the local-mac-address >> property. I guess we should honor that too. But I don't really know >> how that's related to an alias. If the device is probed and the >> property is there, use it, otherwise don't. > This came from the sdio_realtek module on some sunxi boards, where the device tree has configured it obviously for linux, but u-boot has no notion of it. But u-boot IS responsible to generate (the same) MAC address for the device. So yes, u-boot inserts a mac address into the device-tree for a found alias. > > E.g. ethernet2 is an alias without a MAC address configured for it. Thus u-boot is used to generate the MAC address for this node and inserts it into the dtb. What I haven't double checked (just blindly assumed, which is the mother) if this code also inserts the mac into the environment, but the kernel only gets this information via the dtb anyway, right? Either via the dtb, or via the MAC register bytes where it is stored in some drivers. About the setting of the MAC in the environment for devices u-boot knows nothing about, but which have an alias in the dt, this is not necessary, it was done in the old code as a way to make the u-boot fdt code pick up the MAC and inject it into the dtb passed to the kernel. If this can now happen without setting it in the env that step can be skipped. Regards, Hans p.s. I've NOT been following these (and related threads). If anyone has any questions for me, please send me a direct mail which does not have "PATCH" in the subject, otherwise I will likely not seeing as I'm mostly ignoring these threads (sorry -ENOTIME).
On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl <oliver@schinagl.nl> wrote: > This patch adds a method for the board to set the MAC address if the > environment is not yet set. The environment based MAC addresses are not > touched, but if the fdt has an alias set, it is parsed and put into the > environment. > > E.g. The environment contains ethaddr and eth1addr, and the fdt contains > an ethernet1 nothing happens. If the fdt contains ethernet2 however, it > is parsed and inserted into the environment as eth2addr. > > Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> > --- > common/fdt_support.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/common/fdt_support.c b/common/fdt_support.c > index c34a13c..f127392 100644 > --- a/common/fdt_support.c > +++ b/common/fdt_support.c > @@ -465,6 +465,11 @@ int fdt_fixup_memory(void *blob, u64 start, u64 size) > return fdt_fixup_memory_banks(blob, &start, &size, 1); > } > > +__weak int board_get_enetaddr(const int i, unsigned char *mac_addr) Ugh. This collides with a function in board/v38b/ethaddr.c and in board/intercontrol/digsy_mtc/digsy_mtc.c Also, it's so generic, and only gets called by the fdt fixup stuff... This function should probably be named in such a way that its association with fdt is clear. > +{ > + return -ENOSYS; > +} > + > void fdt_fixup_ethernet(void *fdt) > { > int i, prop; > @@ -507,7 +512,8 @@ void fdt_fixup_ethernet(void *fdt) > if (fdt_eth_addr) > eth_parse_enetaddr(fdt_eth_addr, mac_addr); > else > - continue; > + if (board_get_enetaddr(i, mac_addr) < 0) > + continue; > > do_fixup_by_path(fdt, path, "mac-address", > &mac_addr, 6, 0); > -- > 2.10.2 > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot
On Wed, Nov 30, 2016 at 4:54 AM, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > > On 30-11-16 11:50, Olliver Schinagl wrote: >> >> Hey maime, >> >> Sorry for constantly getting your e-mail address wrong! Sorry! >> >> On 30-11-16 10:12, maxime.ripard@free-electrons.com wrote: >>> >>> On Wed, Nov 30, 2016 at 09:00:51AM +0000, Marcel Ziswiler wrote: >>>> >>>> Hi Olliver >>>> >>>> On Fri, 2016-11-25 at 16:30 +0100, Olliver Schinagl wrote: >>>>> >>>>> This patch adds a method for the board to set the MAC address if the >>>>> environment is not yet set. The environment based MAC addresses are >>>>> not >>>>> touched, but if the fdt has an alias set, it is parsed and put into >>>>> the >>>>> environment. >>>>> >>>>> E.g. The environment contains ethaddr and eth1addr, and the fdt >>>>> contains >>>>> an ethernet1 nothing happens. If the fdt contains ethernet2 however, >>>>> it >>>>> is parsed and inserted into the environment as eth2addr. >>>> >>>> My humble understanding of device tree fixup is that it works the other >>>> way around (e.g. it is the device tree that usually gets fixed up). So >>>> the least I would advice for this patch is to change its naming but >>>> most possibly such code also does not belong into the common >>>> fdt_support implementation. >> >> First, yes I noticed this as well, the nameing is the wrong way around. It >> took me a few reading times as well. I guess I did not full understand >> Hans's suggestion comment. So there's some work needed here. >>> >>> I don't really have the context of this patch, but in the DT at least, >>> you can specify the mac address using the local-mac-address >>> property. I guess we should honor that too. But I don't really know >>> how that's related to an alias. If the device is probed and the >>> property is there, use it, otherwise don't. >> >> This came from the sdio_realtek module on some sunxi boards, where the >> device tree has configured it obviously for linux, but u-boot has no notion >> of it. But u-boot IS responsible to generate (the same) MAC address for the >> device. So yes, u-boot inserts a mac address into the device-tree for a >> found alias. >> >> E.g. ethernet2 is an alias without a MAC address configured for it. Thus >> u-boot is used to generate the MAC address for this node and inserts it into >> the dtb. What I haven't double checked (just blindly assumed, which is the >> mother) if this code also inserts the mac into the environment, but the >> kernel only gets this information via the dtb anyway, right? Either via the >> dtb, or via the MAC register bytes where it is stored in some drivers. > > > About the setting of the MAC in the environment for devices u-boot > knows nothing about, but which have an alias in the dt, this is > not necessary, it was done in the old code as a way to make the > u-boot fdt code pick up the MAC and inject it into the dtb > passed to the kernel. If this can now happen without setting > it in the env that step can be skipped. I agree that the env should not be updated. -Joe
Hi, On 25 November 2016 at 08:30, Olliver Schinagl <oliver@schinagl.nl> wrote: > This patch adds a method for the board to set the MAC address if the > environment is not yet set. The environment based MAC addresses are not > touched, but if the fdt has an alias set, it is parsed and put into the > environment. > > E.g. The environment contains ethaddr and eth1addr, and the fdt contains > an ethernet1 nothing happens. If the fdt contains ethernet2 however, it > is parsed and inserted into the environment as eth2addr. > > Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> > --- > common/fdt_support.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/common/fdt_support.c b/common/fdt_support.c > index c34a13c..f127392 100644 > --- a/common/fdt_support.c > +++ b/common/fdt_support.c > @@ -465,6 +465,11 @@ int fdt_fixup_memory(void *blob, u64 start, u64 size) > return fdt_fixup_memory_banks(blob, &start, &size, 1); > } > > +__weak int board_get_enetaddr(const int i, unsigned char *mac_addr) > +{ > + return -ENOSYS; > +} > + > void fdt_fixup_ethernet(void *fdt) > { > int i, prop; > @@ -507,7 +512,8 @@ void fdt_fixup_ethernet(void *fdt) > if (fdt_eth_addr) > eth_parse_enetaddr(fdt_eth_addr, mac_addr); > else > - continue; > + if (board_get_enetaddr(i, mac_addr) < 0) > + continue; > > do_fixup_by_path(fdt, path, "mac-address", > &mac_addr, 6, 0); > -- > 2.10.2 > Much as I don't want to pin this on any one patch, but I feel that our DT fixup stuff is a bit out of control. We have so many functions that do this, and they are called from various places. At some point we need to think about the infrastructure. IMO we should move to a linker list approach a bit like SPL did recently (SPL_LOAD_IMAGE_METHOD). Then boards and subsystems can register (at build-time) a fixup routine and they all get called one after the other. We could also (later) add run-time support for registering fixups, that drivers could use. Regards, Simon
Hey Joe, On 30-11-16 21:40, Joe Hershberger wrote: > On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl <oliver@schinagl.nl> wrote: >> This patch adds a method for the board to set the MAC address if the >> environment is not yet set. The environment based MAC addresses are not >> touched, but if the fdt has an alias set, it is parsed and put into the >> environment. >> >> E.g. The environment contains ethaddr and eth1addr, and the fdt contains >> an ethernet1 nothing happens. If the fdt contains ethernet2 however, it >> is parsed and inserted into the environment as eth2addr. >> >> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> >> --- >> common/fdt_support.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/common/fdt_support.c b/common/fdt_support.c >> index c34a13c..f127392 100644 >> --- a/common/fdt_support.c >> +++ b/common/fdt_support.c >> @@ -465,6 +465,11 @@ int fdt_fixup_memory(void *blob, u64 start, u64 size) >> return fdt_fixup_memory_banks(blob, &start, &size, 1); >> } >> >> +__weak int board_get_enetaddr(const int i, unsigned char *mac_addr) > Ugh. This collides with a function in board/v38b/ethaddr.c and in > board/intercontrol/digsy_mtc/digsy_mtc.c > > Also, it's so generic, and only gets called by the fdt fixup stuff... > This function should probably be named in such a way that its > association with fdt is clear. I did not notice that, sorry! But naming suggestions are welcome :) Right now, I use it in two unrelated spots however. from the fdt as seen above and in a subclass driver (which will come up for review) as suggested by Simon. There I do: +static int sunxi_gmac_eth_read_rom_hwaddr(struct udevice *dev) +{ + struct eth_pdata *pdata = dev_get_platdata(dev); + + return board_get_enetaddr(dev->seq, pdata->enetaddr); +} + const struct eth_ops sunxi_gmac_eth_ops = { .start = designware_eth_start, .send = designware_eth_send, @@ -102,6 +110,7 @@ const struct eth_ops sunxi_gmac_eth_ops = { .free_pkt = designware_eth_free_pkt, .stop = designware_eth_stop, .write_hwaddr = designware_eth_write_hwaddr, + .read_rom_hwaddr = sunxi_gmac_eth_read_rom_hwaddr, }; which is completly unrelated to the fdt. So naming suggestion or overal suggestion how to handle this nice and generically? Based from the name however I would think that all board_get_enetaddr's work the same however so should have been interchangeable? Or was that silly thinking? Olliver > >> +{ >> + return -ENOSYS; >> +} >> + >> void fdt_fixup_ethernet(void *fdt) >> { >> int i, prop; >> @@ -507,7 +512,8 @@ void fdt_fixup_ethernet(void *fdt) >> if (fdt_eth_addr) >> eth_parse_enetaddr(fdt_eth_addr, mac_addr); >> else >> - continue; >> + if (board_get_enetaddr(i, mac_addr) < 0) >> + continue; >> >> do_fixup_by_path(fdt, path, "mac-address", >> &mac_addr, 6, 0); >> -- >> 2.10.2 >> >> _______________________________________________ >> U-Boot mailing list >> U-Boot@lists.denx.de >> http://lists.denx.de/mailman/listinfo/u-boot
Hi Oliver, On 2 December 2016 at 03:16, Olliver Schinagl <oliver@schinagl.nl> wrote: > Hey Joe, > > > > On 30-11-16 21:40, Joe Hershberger wrote: >> >> On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl <oliver@schinagl.nl> >> wrote: >>> >>> This patch adds a method for the board to set the MAC address if the >>> environment is not yet set. The environment based MAC addresses are not >>> touched, but if the fdt has an alias set, it is parsed and put into the >>> environment. >>> >>> E.g. The environment contains ethaddr and eth1addr, and the fdt contains >>> an ethernet1 nothing happens. If the fdt contains ethernet2 however, it >>> is parsed and inserted into the environment as eth2addr. >>> >>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> >>> --- >>> common/fdt_support.c | 8 +++++++- >>> 1 file changed, 7 insertions(+), 1 deletion(-) >>> >>> diff --git a/common/fdt_support.c b/common/fdt_support.c >>> index c34a13c..f127392 100644 >>> --- a/common/fdt_support.c >>> +++ b/common/fdt_support.c >>> @@ -465,6 +465,11 @@ int fdt_fixup_memory(void *blob, u64 start, u64 >>> size) >>> return fdt_fixup_memory_banks(blob, &start, &size, 1); >>> } >>> >>> +__weak int board_get_enetaddr(const int i, unsigned char *mac_addr) >> >> Ugh. This collides with a function in board/v38b/ethaddr.c and in >> board/intercontrol/digsy_mtc/digsy_mtc.c >> >> Also, it's so generic, and only gets called by the fdt fixup stuff... >> This function should probably be named in such a way that its >> association with fdt is clear. > > I did not notice that, sorry! But naming suggestions are welcome :) > > Right now, I use it in two unrelated spots however. > > from the fdt as seen above and in a subclass driver (which will come up for > review) as suggested by Simon. > > There I do: > > +static int sunxi_gmac_eth_read_rom_hwaddr(struct udevice *dev) > +{ > + struct eth_pdata *pdata = dev_get_platdata(dev); > + > + return board_get_enetaddr(dev->seq, pdata->enetaddr); > +} > + > const struct eth_ops sunxi_gmac_eth_ops = { > .start = designware_eth_start, > .send = designware_eth_send, > @@ -102,6 +110,7 @@ const struct eth_ops sunxi_gmac_eth_ops = { > .free_pkt = designware_eth_free_pkt, > .stop = designware_eth_stop, > .write_hwaddr = designware_eth_write_hwaddr, > + .read_rom_hwaddr = sunxi_gmac_eth_read_rom_hwaddr, > }; > > which is completly unrelated to the fdt. > > So naming suggestion or overal suggestion how to handle this nice and > generically? > > Based from the name however I would think that all board_get_enetaddr's work > the same however so should have been interchangeable? Or was that silly > thinking? Would it be possible to use a name without 'board' in it? I think this / hope is actually sunxi-specific code, not board-specific? Regards, Simon
Hey Simon, On 05-12-16 07:24, Simon Glass wrote: > Hi Oliver, > > On 2 December 2016 at 03:16, Olliver Schinagl <oliver@schinagl.nl> wrote: >> Hey Joe, >> >> >> >> On 30-11-16 21:40, Joe Hershberger wrote: >>> On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl <oliver@schinagl.nl> >>> wrote: >>>> This patch adds a method for the board to set the MAC address if the >>>> environment is not yet set. The environment based MAC addresses are not >>>> touched, but if the fdt has an alias set, it is parsed and put into the >>>> environment. >>>> >>>> E.g. The environment contains ethaddr and eth1addr, and the fdt contains >>>> an ethernet1 nothing happens. If the fdt contains ethernet2 however, it >>>> is parsed and inserted into the environment as eth2addr. >>>> >>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> >>>> --- >>>> common/fdt_support.c | 8 +++++++- >>>> 1 file changed, 7 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/common/fdt_support.c b/common/fdt_support.c >>>> index c34a13c..f127392 100644 >>>> --- a/common/fdt_support.c >>>> +++ b/common/fdt_support.c >>>> @@ -465,6 +465,11 @@ int fdt_fixup_memory(void *blob, u64 start, u64 >>>> size) >>>> return fdt_fixup_memory_banks(blob, &start, &size, 1); >>>> } >>>> >>>> +__weak int board_get_enetaddr(const int i, unsigned char *mac_addr) >>> Ugh. This collides with a function in board/v38b/ethaddr.c and in >>> board/intercontrol/digsy_mtc/digsy_mtc.c >>> >>> Also, it's so generic, and only gets called by the fdt fixup stuff... >>> This function should probably be named in such a way that its >>> association with fdt is clear. >> I did not notice that, sorry! But naming suggestions are welcome :) >> >> Right now, I use it in two unrelated spots however. >> >> from the fdt as seen above and in a subclass driver (which will come up for >> review) as suggested by Simon. >> >> There I do: >> >> +static int sunxi_gmac_eth_read_rom_hwaddr(struct udevice *dev) >> +{ >> + struct eth_pdata *pdata = dev_get_platdata(dev); >> + >> + return board_get_enetaddr(dev->seq, pdata->enetaddr); >> +} >> + >> const struct eth_ops sunxi_gmac_eth_ops = { >> .start = designware_eth_start, >> .send = designware_eth_send, >> @@ -102,6 +110,7 @@ const struct eth_ops sunxi_gmac_eth_ops = { >> .free_pkt = designware_eth_free_pkt, >> .stop = designware_eth_stop, >> .write_hwaddr = designware_eth_write_hwaddr, >> + .read_rom_hwaddr = sunxi_gmac_eth_read_rom_hwaddr, >> }; >> >> which is completly unrelated to the fdt. >> >> So naming suggestion or overal suggestion how to handle this nice and >> generically? >> >> Based from the name however I would think that all board_get_enetaddr's work >> the same however so should have been interchangeable? Or was that silly >> thinking? > Would it be possible to use a name without 'board' in it? I think this > / hope is actually sunxi-specific code, not board-specific? You are actually correct, we take the serial number of the SoC (sunxi specific) and generate a serial/MAC from it. So nothing to do with the board. So I can just name it sunxi_gen_enetaddr(). Would that be then (much) better? The reason why I went to 'board' with my mind, is because a) the original mac gen code and b) the location was in board/sunxi/board.c. I think it is thus also sensible to move it out of board/sunxi/board.c as indeed, it has nothing to do with board(s). Olliver > > Regards, > Simon
Hi Oliver, On 5 December 2016 at 03:28, Olliver Schinagl <oliver@schinagl.nl> wrote: > Hey Simon, > > > > On 05-12-16 07:24, Simon Glass wrote: >> >> Hi Oliver, >> >> On 2 December 2016 at 03:16, Olliver Schinagl <oliver@schinagl.nl> wrote: >>> >>> Hey Joe, >>> >>> >>> >>> On 30-11-16 21:40, Joe Hershberger wrote: >>>> >>>> On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl <oliver@schinagl.nl> >>>> wrote: >>>>> >>>>> This patch adds a method for the board to set the MAC address if the >>>>> environment is not yet set. The environment based MAC addresses are not >>>>> touched, but if the fdt has an alias set, it is parsed and put into the >>>>> environment. >>>>> >>>>> E.g. The environment contains ethaddr and eth1addr, and the fdt >>>>> contains >>>>> an ethernet1 nothing happens. If the fdt contains ethernet2 however, it >>>>> is parsed and inserted into the environment as eth2addr. >>>>> >>>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> >>>>> --- >>>>> common/fdt_support.c | 8 +++++++- >>>>> 1 file changed, 7 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/common/fdt_support.c b/common/fdt_support.c >>>>> index c34a13c..f127392 100644 >>>>> --- a/common/fdt_support.c >>>>> +++ b/common/fdt_support.c >>>>> @@ -465,6 +465,11 @@ int fdt_fixup_memory(void *blob, u64 start, u64 >>>>> size) >>>>> return fdt_fixup_memory_banks(blob, &start, &size, 1); >>>>> } >>>>> >>>>> +__weak int board_get_enetaddr(const int i, unsigned char *mac_addr) >>>> >>>> Ugh. This collides with a function in board/v38b/ethaddr.c and in >>>> board/intercontrol/digsy_mtc/digsy_mtc.c >>>> >>>> Also, it's so generic, and only gets called by the fdt fixup stuff... >>>> This function should probably be named in such a way that its >>>> association with fdt is clear. >>> >>> I did not notice that, sorry! But naming suggestions are welcome :) >>> >>> Right now, I use it in two unrelated spots however. >>> >>> from the fdt as seen above and in a subclass driver (which will come up >>> for >>> review) as suggested by Simon. >>> >>> There I do: >>> >>> +static int sunxi_gmac_eth_read_rom_hwaddr(struct udevice *dev) >>> +{ >>> + struct eth_pdata *pdata = dev_get_platdata(dev); >>> + >>> + return board_get_enetaddr(dev->seq, pdata->enetaddr); >>> +} >>> + >>> const struct eth_ops sunxi_gmac_eth_ops = { >>> .start = designware_eth_start, >>> .send = designware_eth_send, >>> @@ -102,6 +110,7 @@ const struct eth_ops sunxi_gmac_eth_ops = { >>> .free_pkt = designware_eth_free_pkt, >>> .stop = designware_eth_stop, >>> .write_hwaddr = designware_eth_write_hwaddr, >>> + .read_rom_hwaddr = sunxi_gmac_eth_read_rom_hwaddr, >>> }; >>> >>> which is completly unrelated to the fdt. >>> >>> So naming suggestion or overal suggestion how to handle this nice and >>> generically? >>> >>> Based from the name however I would think that all board_get_enetaddr's >>> work >>> the same however so should have been interchangeable? Or was that silly >>> thinking? >> >> Would it be possible to use a name without 'board' in it? I think this >> / hope is actually sunxi-specific code, not board-specific? > > You are actually correct, we take the serial number of the SoC (sunxi > specific) and generate a serial/MAC from it. So nothing to do with the > board. So I can just name it sunxi_gen_enetaddr(). Would that be then (much) > better? > > The reason why I went to 'board' with my mind, is because a) the original > mac gen code and b) the location was in board/sunxi/board.c. I think it is > thus also sensible to move it out of board/sunxi/board.c as indeed, it has > nothing to do with board(s). That sounds good to me - and you should be able to call it directly from the driver and avoid any weak functions, right? Regards, Simon
On December 7, 2016 4:47:23 AM CET, Simon Glass <sjg@chromium.org> wrote: >Hi Oliver, > >On 5 December 2016 at 03:28, Olliver Schinagl <oliver@schinagl.nl> >wrote: >> Hey Simon, >> >> >> >> On 05-12-16 07:24, Simon Glass wrote: >>> >>> Hi Oliver, >>> >>> On 2 December 2016 at 03:16, Olliver Schinagl <oliver@schinagl.nl> >wrote: >>>> >>>> Hey Joe, >>>> >>>> >>>> >>>> On 30-11-16 21:40, Joe Hershberger wrote: >>>>> >>>>> On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl ><oliver@schinagl.nl> >>>>> wrote: >>>>>> >>>>>> This patch adds a method for the board to set the MAC address if >the >>>>>> environment is not yet set. The environment based MAC addresses >are not >>>>>> touched, but if the fdt has an alias set, it is parsed and put >into the >>>>>> environment. >>>>>> >>>>>> E.g. The environment contains ethaddr and eth1addr, and the fdt >>>>>> contains >>>>>> an ethernet1 nothing happens. If the fdt contains ethernet2 >however, it >>>>>> is parsed and inserted into the environment as eth2addr. >>>>>> >>>>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> >>>>>> --- >>>>>> common/fdt_support.c | 8 +++++++- >>>>>> 1 file changed, 7 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/common/fdt_support.c b/common/fdt_support.c >>>>>> index c34a13c..f127392 100644 >>>>>> --- a/common/fdt_support.c >>>>>> +++ b/common/fdt_support.c >>>>>> @@ -465,6 +465,11 @@ int fdt_fixup_memory(void *blob, u64 start, >u64 >>>>>> size) >>>>>> return fdt_fixup_memory_banks(blob, &start, &size, 1); >>>>>> } >>>>>> >>>>>> +__weak int board_get_enetaddr(const int i, unsigned char >*mac_addr) >>>>> >>>>> Ugh. This collides with a function in board/v38b/ethaddr.c and in >>>>> board/intercontrol/digsy_mtc/digsy_mtc.c >>>>> >>>>> Also, it's so generic, and only gets called by the fdt fixup >stuff... >>>>> This function should probably be named in such a way that its >>>>> association with fdt is clear. >>>> >>>> I did not notice that, sorry! But naming suggestions are welcome :) >>>> >>>> Right now, I use it in two unrelated spots however. >>>> >>>> from the fdt as seen above and in a subclass driver (which will >come up >>>> for >>>> review) as suggested by Simon. >>>> >>>> There I do: >>>> >>>> +static int sunxi_gmac_eth_read_rom_hwaddr(struct udevice *dev) >>>> +{ >>>> + struct eth_pdata *pdata = dev_get_platdata(dev); >>>> + >>>> + return board_get_enetaddr(dev->seq, pdata->enetaddr); >>>> +} >>>> + >>>> const struct eth_ops sunxi_gmac_eth_ops = { >>>> .start = designware_eth_start, >>>> .send = designware_eth_send, >>>> @@ -102,6 +110,7 @@ const struct eth_ops sunxi_gmac_eth_ops = { >>>> .free_pkt = designware_eth_free_pkt, >>>> .stop = designware_eth_stop, >>>> .write_hwaddr = designware_eth_write_hwaddr, >>>> + .read_rom_hwaddr = sunxi_gmac_eth_read_rom_hwaddr, >>>> }; >>>> >>>> which is completly unrelated to the fdt. >>>> >>>> So naming suggestion or overal suggestion how to handle this nice >and >>>> generically? >>>> >>>> Based from the name however I would think that all >board_get_enetaddr's >>>> work >>>> the same however so should have been interchangeable? Or was that >silly >>>> thinking? >>> >>> Would it be possible to use a name without 'board' in it? I think >this >>> / hope is actually sunxi-specific code, not board-specific? >> >> You are actually correct, we take the serial number of the SoC (sunxi >> specific) and generate a serial/MAC from it. So nothing to do with >the >> board. So I can just name it sunxi_gen_enetaddr(). Would that be then >(much) >> better? >> >> The reason why I went to 'board' with my mind, is because a) the >original >> mac gen code and b) the location was in board/sunxi/board.c. I think >it is >> thus also sensible to move it out of board/sunxi/board.c as indeed, >it has >> nothing to do with board(s). > >That sounds good to me - and you should be able to call it directly >from the driver and avoid any weak functions, right? The subclass driver can, the fdt fixup however still needs a weak fdt_get_enetaddr()? (Which in our case calls then sunxi_get_enetaddr() i think.) > >Regards, >Simon
Hi Oliver, On 7 December 2016 at 02:26, Olliver Schinagl <oliver@schinagl.nl> wrote: > > > On December 7, 2016 4:47:23 AM CET, Simon Glass <sjg@chromium.org> wrote: >>Hi Oliver, >> >>On 5 December 2016 at 03:28, Olliver Schinagl <oliver@schinagl.nl> >>wrote: >>> Hey Simon, >>> >>> >>> >>> On 05-12-16 07:24, Simon Glass wrote: >>>> >>>> Hi Oliver, >>>> >>>> On 2 December 2016 at 03:16, Olliver Schinagl <oliver@schinagl.nl> >>wrote: >>>>> >>>>> Hey Joe, >>>>> >>>>> >>>>> >>>>> On 30-11-16 21:40, Joe Hershberger wrote: >>>>>> >>>>>> On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl >><oliver@schinagl.nl> >>>>>> wrote: >>>>>>> >>>>>>> This patch adds a method for the board to set the MAC address if >>the >>>>>>> environment is not yet set. The environment based MAC addresses >>are not >>>>>>> touched, but if the fdt has an alias set, it is parsed and put >>into the >>>>>>> environment. >>>>>>> >>>>>>> E.g. The environment contains ethaddr and eth1addr, and the fdt >>>>>>> contains >>>>>>> an ethernet1 nothing happens. If the fdt contains ethernet2 >>however, it >>>>>>> is parsed and inserted into the environment as eth2addr. >>>>>>> >>>>>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> >>>>>>> --- >>>>>>> common/fdt_support.c | 8 +++++++- >>>>>>> 1 file changed, 7 insertions(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/common/fdt_support.c b/common/fdt_support.c >>>>>>> index c34a13c..f127392 100644 >>>>>>> --- a/common/fdt_support.c >>>>>>> +++ b/common/fdt_support.c >>>>>>> @@ -465,6 +465,11 @@ int fdt_fixup_memory(void *blob, u64 start, >>u64 >>>>>>> size) >>>>>>> return fdt_fixup_memory_banks(blob, &start, &size, 1); >>>>>>> } >>>>>>> >>>>>>> +__weak int board_get_enetaddr(const int i, unsigned char >>*mac_addr) >>>>>> >>>>>> Ugh. This collides with a function in board/v38b/ethaddr.c and in >>>>>> board/intercontrol/digsy_mtc/digsy_mtc.c >>>>>> >>>>>> Also, it's so generic, and only gets called by the fdt fixup >>stuff... >>>>>> This function should probably be named in such a way that its >>>>>> association with fdt is clear. >>>>> >>>>> I did not notice that, sorry! But naming suggestions are welcome :) >>>>> >>>>> Right now, I use it in two unrelated spots however. >>>>> >>>>> from the fdt as seen above and in a subclass driver (which will >>come up >>>>> for >>>>> review) as suggested by Simon. >>>>> >>>>> There I do: >>>>> >>>>> +static int sunxi_gmac_eth_read_rom_hwaddr(struct udevice *dev) >>>>> +{ >>>>> + struct eth_pdata *pdata = dev_get_platdata(dev); >>>>> + >>>>> + return board_get_enetaddr(dev->seq, pdata->enetaddr); >>>>> +} >>>>> + >>>>> const struct eth_ops sunxi_gmac_eth_ops = { >>>>> .start = designware_eth_start, >>>>> .send = designware_eth_send, >>>>> @@ -102,6 +110,7 @@ const struct eth_ops sunxi_gmac_eth_ops = { >>>>> .free_pkt = designware_eth_free_pkt, >>>>> .stop = designware_eth_stop, >>>>> .write_hwaddr = designware_eth_write_hwaddr, >>>>> + .read_rom_hwaddr = sunxi_gmac_eth_read_rom_hwaddr, >>>>> }; >>>>> >>>>> which is completly unrelated to the fdt. >>>>> >>>>> So naming suggestion or overal suggestion how to handle this nice >>and >>>>> generically? >>>>> >>>>> Based from the name however I would think that all >>board_get_enetaddr's >>>>> work >>>>> the same however so should have been interchangeable? Or was that >>silly >>>>> thinking? >>>> >>>> Would it be possible to use a name without 'board' in it? I think >>this >>>> / hope is actually sunxi-specific code, not board-specific? >>> >>> You are actually correct, we take the serial number of the SoC (sunxi >>> specific) and generate a serial/MAC from it. So nothing to do with >>the >>> board. So I can just name it sunxi_gen_enetaddr(). Would that be then >>(much) >>> better? >>> >>> The reason why I went to 'board' with my mind, is because a) the >>original >>> mac gen code and b) the location was in board/sunxi/board.c. I think >>it is >>> thus also sensible to move it out of board/sunxi/board.c as indeed, >>it has >>> nothing to do with board(s). >> >>That sounds good to me - and you should be able to call it directly >>from the driver and avoid any weak functions, right? > The subclass driver can, the fdt fixup however still needs a weak fdt_get_enetaddr()? (Which in our case calls then sunxi_get_enetaddr() i think.) OK - I feel that the fdt fixups need a bit of thought. At the moment it is all pretty ad-hoc. Regards, Simon
Hey simon On December 8, 2016 11:21:32 PM CET, Simon Glass <sjg@chromium.org> wrote: >Hi Oliver, > >On 7 December 2016 at 02:26, Olliver Schinagl <oliver@schinagl.nl> >wrote: >> >> >> On December 7, 2016 4:47:23 AM CET, Simon Glass <sjg@chromium.org> >wrote: >>>Hi Oliver, >>> >>>On 5 December 2016 at 03:28, Olliver Schinagl <oliver@schinagl.nl> >>>wrote: >>>> Hey Simon, >>>> >>>> >>>> >>>> On 05-12-16 07:24, Simon Glass wrote: >>>>> >>>>> Hi Oliver, >>>>> >>>>> On 2 December 2016 at 03:16, Olliver Schinagl <oliver@schinagl.nl> >>>wrote: >>>>>> >>>>>> Hey Joe, >>>>>> >>>>>> >>>>>> >>>>>> On 30-11-16 21:40, Joe Hershberger wrote: >>>>>>> >>>>>>> On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl >>><oliver@schinagl.nl> >>>>>>> wrote: >>>>>>>> >>>>>>>> This patch adds a method for the board to set the MAC address >if >>>the >>>>>>>> environment is not yet set. The environment based MAC addresses >>>are not >>>>>>>> touched, but if the fdt has an alias set, it is parsed and put >>>into the >>>>>>>> environment. >>>>>>>> >>>>>>>> E.g. The environment contains ethaddr and eth1addr, and the fdt >>>>>>>> contains >>>>>>>> an ethernet1 nothing happens. If the fdt contains ethernet2 >>>however, it >>>>>>>> is parsed and inserted into the environment as eth2addr. >>>>>>>> >>>>>>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> >>>>>>>> --- >>>>>>>> common/fdt_support.c | 8 +++++++- >>>>>>>> 1 file changed, 7 insertions(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/common/fdt_support.c b/common/fdt_support.c >>>>>>>> index c34a13c..f127392 100644 >>>>>>>> --- a/common/fdt_support.c >>>>>>>> +++ b/common/fdt_support.c >>>>>>>> @@ -465,6 +465,11 @@ int fdt_fixup_memory(void *blob, u64 >start, >>>u64 >>>>>>>> size) >>>>>>>> return fdt_fixup_memory_banks(blob, &start, &size, >1); >>>>>>>> } >>>>>>>> >>>>>>>> +__weak int board_get_enetaddr(const int i, unsigned char >>>*mac_addr) >>>>>>> >>>>>>> Ugh. This collides with a function in board/v38b/ethaddr.c and >in >>>>>>> board/intercontrol/digsy_mtc/digsy_mtc.c >>>>>>> >>>>>>> Also, it's so generic, and only gets called by the fdt fixup >>>stuff... >>>>>>> This function should probably be named in such a way that its >>>>>>> association with fdt is clear. >>>>>> >>>>>> I did not notice that, sorry! But naming suggestions are welcome >:) >>>>>> >>>>>> Right now, I use it in two unrelated spots however. >>>>>> >>>>>> from the fdt as seen above and in a subclass driver (which will >>>come up >>>>>> for >>>>>> review) as suggested by Simon. >>>>>> >>>>>> There I do: >>>>>> >>>>>> +static int sunxi_gmac_eth_read_rom_hwaddr(struct udevice *dev) >>>>>> +{ >>>>>> + struct eth_pdata *pdata = dev_get_platdata(dev); >>>>>> + >>>>>> + return board_get_enetaddr(dev->seq, pdata->enetaddr); >>>>>> +} >>>>>> + >>>>>> const struct eth_ops sunxi_gmac_eth_ops = { >>>>>> .start = designware_eth_start, >>>>>> .send = designware_eth_send, >>>>>> @@ -102,6 +110,7 @@ const struct eth_ops sunxi_gmac_eth_ops = { >>>>>> .free_pkt = designware_eth_free_pkt, >>>>>> .stop = designware_eth_stop, >>>>>> .write_hwaddr = designware_eth_write_hwaddr, >>>>>> + .read_rom_hwaddr = sunxi_gmac_eth_read_rom_hwaddr, >>>>>> }; >>>>>> >>>>>> which is completly unrelated to the fdt. >>>>>> >>>>>> So naming suggestion or overal suggestion how to handle this nice >>>and >>>>>> generically? >>>>>> >>>>>> Based from the name however I would think that all >>>board_get_enetaddr's >>>>>> work >>>>>> the same however so should have been interchangeable? Or was that >>>silly >>>>>> thinking? >>>>> >>>>> Would it be possible to use a name without 'board' in it? I think >>>this >>>>> / hope is actually sunxi-specific code, not board-specific? >>>> >>>> You are actually correct, we take the serial number of the SoC >(sunxi >>>> specific) and generate a serial/MAC from it. So nothing to do with >>>the >>>> board. So I can just name it sunxi_gen_enetaddr(). Would that be >then >>>(much) >>>> better? >>>> >>>> The reason why I went to 'board' with my mind, is because a) the >>>original >>>> mac gen code and b) the location was in board/sunxi/board.c. I >think >>>it is >>>> thus also sensible to move it out of board/sunxi/board.c as indeed, >>>it has >>>> nothing to do with board(s). >>> >>>That sounds good to me - and you should be able to call it directly >>>from the driver and avoid any weak functions, right? >> The subclass driver can, the fdt fixup however still needs a weak >fdt_get_enetaddr()? (Which in our case calls then sunxi_get_enetaddr() >i think.) > >OK - I feel that the fdt fixups need a bit of thought. At the moment >it is all pretty ad-hoc. Yeah i think i agree, but i guess thats a separate cleanup step generally, no? > >Regards, >Simon
Hi Oliver, On 9 December 2016 at 02:25, Olliver Schinagl <oliver@schinagl.nl> wrote: > Hey simon > > On December 8, 2016 11:21:32 PM CET, Simon Glass <sjg@chromium.org> wrote: >>Hi Oliver, >> >>On 7 December 2016 at 02:26, Olliver Schinagl <oliver@schinagl.nl> >>wrote: >>> >>> >>> On December 7, 2016 4:47:23 AM CET, Simon Glass <sjg@chromium.org> >>wrote: >>>>Hi Oliver, >>>> >>>>On 5 December 2016 at 03:28, Olliver Schinagl <oliver@schinagl.nl> >>>>wrote: >>>>> Hey Simon, >>>>> >>>>> >>>>> >>>>> On 05-12-16 07:24, Simon Glass wrote: >>>>>> >>>>>> Hi Oliver, >>>>>> >>>>>> On 2 December 2016 at 03:16, Olliver Schinagl <oliver@schinagl.nl> >>>>wrote: >>>>>>> >>>>>>> Hey Joe, >>>>>>> >>>>>>> >>>>>>> >>>>>>> On 30-11-16 21:40, Joe Hershberger wrote: >>>>>>>> >>>>>>>> On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl >>>><oliver@schinagl.nl> >>>>>>>> wrote: >>>>>>>>> >>>>>>>>> This patch adds a method for the board to set the MAC address >>if >>>>the >>>>>>>>> environment is not yet set. The environment based MAC addresses >>>>are not >>>>>>>>> touched, but if the fdt has an alias set, it is parsed and put >>>>into the >>>>>>>>> environment. >>>>>>>>> >>>>>>>>> E.g. The environment contains ethaddr and eth1addr, and the fdt >>>>>>>>> contains >>>>>>>>> an ethernet1 nothing happens. If the fdt contains ethernet2 >>>>however, it >>>>>>>>> is parsed and inserted into the environment as eth2addr. >>>>>>>>> >>>>>>>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> >>>>>>>>> --- >>>>>>>>> common/fdt_support.c | 8 +++++++- >>>>>>>>> 1 file changed, 7 insertions(+), 1 deletion(-) >>>>>>>>> >>>>>>>>> diff --git a/common/fdt_support.c b/common/fdt_support.c >>>>>>>>> index c34a13c..f127392 100644 >>>>>>>>> --- a/common/fdt_support.c >>>>>>>>> +++ b/common/fdt_support.c >>>>>>>>> @@ -465,6 +465,11 @@ int fdt_fixup_memory(void *blob, u64 >>start, >>>>u64 >>>>>>>>> size) >>>>>>>>> return fdt_fixup_memory_banks(blob, &start, &size, >>1); >>>>>>>>> } >>>>>>>>> >>>>>>>>> +__weak int board_get_enetaddr(const int i, unsigned char >>>>*mac_addr) >>>>>>>> >>>>>>>> Ugh. This collides with a function in board/v38b/ethaddr.c and >>in >>>>>>>> board/intercontrol/digsy_mtc/digsy_mtc.c >>>>>>>> >>>>>>>> Also, it's so generic, and only gets called by the fdt fixup >>>>stuff... >>>>>>>> This function should probably be named in such a way that its >>>>>>>> association with fdt is clear. >>>>>>> >>>>>>> I did not notice that, sorry! But naming suggestions are welcome >>:) >>>>>>> >>>>>>> Right now, I use it in two unrelated spots however. >>>>>>> >>>>>>> from the fdt as seen above and in a subclass driver (which will >>>>come up >>>>>>> for >>>>>>> review) as suggested by Simon. >>>>>>> >>>>>>> There I do: >>>>>>> >>>>>>> +static int sunxi_gmac_eth_read_rom_hwaddr(struct udevice *dev) >>>>>>> +{ >>>>>>> + struct eth_pdata *pdata = dev_get_platdata(dev); >>>>>>> + >>>>>>> + return board_get_enetaddr(dev->seq, pdata->enetaddr); >>>>>>> +} >>>>>>> + >>>>>>> const struct eth_ops sunxi_gmac_eth_ops = { >>>>>>> .start = designware_eth_start, >>>>>>> .send = designware_eth_send, >>>>>>> @@ -102,6 +110,7 @@ const struct eth_ops sunxi_gmac_eth_ops = { >>>>>>> .free_pkt = designware_eth_free_pkt, >>>>>>> .stop = designware_eth_stop, >>>>>>> .write_hwaddr = designware_eth_write_hwaddr, >>>>>>> + .read_rom_hwaddr = sunxi_gmac_eth_read_rom_hwaddr, >>>>>>> }; >>>>>>> >>>>>>> which is completly unrelated to the fdt. >>>>>>> >>>>>>> So naming suggestion or overal suggestion how to handle this nice >>>>and >>>>>>> generically? >>>>>>> >>>>>>> Based from the name however I would think that all >>>>board_get_enetaddr's >>>>>>> work >>>>>>> the same however so should have been interchangeable? Or was that >>>>silly >>>>>>> thinking? >>>>>> >>>>>> Would it be possible to use a name without 'board' in it? I think >>>>this >>>>>> / hope is actually sunxi-specific code, not board-specific? >>>>> >>>>> You are actually correct, we take the serial number of the SoC >>(sunxi >>>>> specific) and generate a serial/MAC from it. So nothing to do with >>>>the >>>>> board. So I can just name it sunxi_gen_enetaddr(). Would that be >>then >>>>(much) >>>>> better? >>>>> >>>>> The reason why I went to 'board' with my mind, is because a) the >>>>original >>>>> mac gen code and b) the location was in board/sunxi/board.c. I >>think >>>>it is >>>>> thus also sensible to move it out of board/sunxi/board.c as indeed, >>>>it has >>>>> nothing to do with board(s). >>>> >>>>That sounds good to me - and you should be able to call it directly >>>>from the driver and avoid any weak functions, right? >>> The subclass driver can, the fdt fixup however still needs a weak >>fdt_get_enetaddr()? (Which in our case calls then sunxi_get_enetaddr() >>i think.) >> >>OK - I feel that the fdt fixups need a bit of thought. At the moment >>it is all pretty ad-hoc. > > Yeah i think i agree, but i guess thats a separate cleanup step generally, no? OK - are you able to take a look at this? Regards, Simon
Hi Oliver, On Sun, Dec 11, 2016 at 3:27 PM, Simon Glass <sjg@chromium.org> wrote: > Hi Oliver, > > On 9 December 2016 at 02:25, Olliver Schinagl <oliver@schinagl.nl> wrote: >> Hey simon >> >> On December 8, 2016 11:21:32 PM CET, Simon Glass <sjg@chromium.org> wrote: >>>Hi Oliver, >>> >>>On 7 December 2016 at 02:26, Olliver Schinagl <oliver@schinagl.nl> >>>wrote: >>>> >>>> >>>> On December 7, 2016 4:47:23 AM CET, Simon Glass <sjg@chromium.org> >>>wrote: >>>>>Hi Oliver, >>>>> >>>>>On 5 December 2016 at 03:28, Olliver Schinagl <oliver@schinagl.nl> >>>>>wrote: >>>>>> Hey Simon, >>>>>> >>>>>> >>>>>> >>>>>> On 05-12-16 07:24, Simon Glass wrote: >>>>>>> >>>>>>> Hi Oliver, >>>>>>> >>>>>>> On 2 December 2016 at 03:16, Olliver Schinagl <oliver@schinagl.nl> >>>>>wrote: >>>>>>>> >>>>>>>> Hey Joe, >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On 30-11-16 21:40, Joe Hershberger wrote: >>>>>>>>> >>>>>>>>> On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl >>>>><oliver@schinagl.nl> >>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>> This patch adds a method for the board to set the MAC address >>>if >>>>>the >>>>>>>>>> environment is not yet set. The environment based MAC addresses >>>>>are not >>>>>>>>>> touched, but if the fdt has an alias set, it is parsed and put >>>>>into the >>>>>>>>>> environment. >>>>>>>>>> >>>>>>>>>> E.g. The environment contains ethaddr and eth1addr, and the fdt >>>>>>>>>> contains >>>>>>>>>> an ethernet1 nothing happens. If the fdt contains ethernet2 >>>>>however, it >>>>>>>>>> is parsed and inserted into the environment as eth2addr. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> >>>>>>>>>> --- >>>>>>>>>> common/fdt_support.c | 8 +++++++- >>>>>>>>>> 1 file changed, 7 insertions(+), 1 deletion(-) >>>>>>>>>> >>>>>>>>>> diff --git a/common/fdt_support.c b/common/fdt_support.c >>>>>>>>>> index c34a13c..f127392 100644 >>>>>>>>>> --- a/common/fdt_support.c >>>>>>>>>> +++ b/common/fdt_support.c >>>>>>>>>> @@ -465,6 +465,11 @@ int fdt_fixup_memory(void *blob, u64 >>>start, >>>>>u64 >>>>>>>>>> size) >>>>>>>>>> return fdt_fixup_memory_banks(blob, &start, &size, >>>1); >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> +__weak int board_get_enetaddr(const int i, unsigned char >>>>>*mac_addr) >>>>>>>>> >>>>>>>>> Ugh. This collides with a function in board/v38b/ethaddr.c and >>>in >>>>>>>>> board/intercontrol/digsy_mtc/digsy_mtc.c >>>>>>>>> >>>>>>>>> Also, it's so generic, and only gets called by the fdt fixup >>>>>stuff... >>>>>>>>> This function should probably be named in such a way that its >>>>>>>>> association with fdt is clear. >>>>>>>> >>>>>>>> I did not notice that, sorry! But naming suggestions are welcome >>>:) >>>>>>>> >>>>>>>> Right now, I use it in two unrelated spots however. >>>>>>>> >>>>>>>> from the fdt as seen above and in a subclass driver (which will >>>>>come up >>>>>>>> for >>>>>>>> review) as suggested by Simon. >>>>>>>> >>>>>>>> There I do: >>>>>>>> >>>>>>>> +static int sunxi_gmac_eth_read_rom_hwaddr(struct udevice *dev) >>>>>>>> +{ >>>>>>>> + struct eth_pdata *pdata = dev_get_platdata(dev); >>>>>>>> + >>>>>>>> + return board_get_enetaddr(dev->seq, pdata->enetaddr); >>>>>>>> +} >>>>>>>> + >>>>>>>> const struct eth_ops sunxi_gmac_eth_ops = { >>>>>>>> .start = designware_eth_start, >>>>>>>> .send = designware_eth_send, >>>>>>>> @@ -102,6 +110,7 @@ const struct eth_ops sunxi_gmac_eth_ops = { >>>>>>>> .free_pkt = designware_eth_free_pkt, >>>>>>>> .stop = designware_eth_stop, >>>>>>>> .write_hwaddr = designware_eth_write_hwaddr, >>>>>>>> + .read_rom_hwaddr = sunxi_gmac_eth_read_rom_hwaddr, >>>>>>>> }; >>>>>>>> >>>>>>>> which is completly unrelated to the fdt. >>>>>>>> >>>>>>>> So naming suggestion or overal suggestion how to handle this nice >>>>>and >>>>>>>> generically? >>>>>>>> >>>>>>>> Based from the name however I would think that all >>>>>board_get_enetaddr's >>>>>>>> work >>>>>>>> the same however so should have been interchangeable? Or was that >>>>>silly >>>>>>>> thinking? >>>>>>> >>>>>>> Would it be possible to use a name without 'board' in it? I think >>>>>this >>>>>>> / hope is actually sunxi-specific code, not board-specific? >>>>>> >>>>>> You are actually correct, we take the serial number of the SoC >>>(sunxi >>>>>> specific) and generate a serial/MAC from it. So nothing to do with >>>>>the >>>>>> board. So I can just name it sunxi_gen_enetaddr(). Would that be >>>then >>>>>(much) >>>>>> better? >>>>>> >>>>>> The reason why I went to 'board' with my mind, is because a) the >>>>>original >>>>>> mac gen code and b) the location was in board/sunxi/board.c. I >>>think >>>>>it is >>>>>> thus also sensible to move it out of board/sunxi/board.c as indeed, >>>>>it has >>>>>> nothing to do with board(s). >>>>> >>>>>That sounds good to me - and you should be able to call it directly >>>>>from the driver and avoid any weak functions, right? >>>> The subclass driver can, the fdt fixup however still needs a weak >>>fdt_get_enetaddr()? (Which in our case calls then sunxi_get_enetaddr() >>>i think.) >>> >>>OK - I feel that the fdt fixups need a bit of thought. At the moment >>>it is all pretty ad-hoc. >> >> Yeah i think i agree, but i guess thats a separate cleanup step generally, no? > > OK - are you able to take a look at this? Any update on this or any of the other patches in your series that I did not apply last release? I was expecting a v2 to address some things such as this patch. Thanks! -Joe
Hey Joe, On 26-03-17 16:10, Joe Hershberger wrote: > Hi Oliver, > > On Sun, Dec 11, 2016 at 3:27 PM, Simon Glass <sjg@chromium.org> wrote: >> Hi Oliver, >> >> On 9 December 2016 at 02:25, Olliver Schinagl <oliver@schinagl.nl> wrote: >>> Hey simon >>> >>> On December 8, 2016 11:21:32 PM CET, Simon Glass <sjg@chromium.org> wrote: >>>> Hi Oliver, >>>> >>>> On 7 December 2016 at 02:26, Olliver Schinagl <oliver@schinagl.nl> >>>> wrote: >>>>> >>>>> >>>>> On December 7, 2016 4:47:23 AM CET, Simon Glass <sjg@chromium.org> >>>> wrote: >>>>>> Hi Oliver, >>>>>> >>>>>> On 5 December 2016 at 03:28, Olliver Schinagl <oliver@schinagl.nl> >>>>>> wrote: >>>>>>> Hey Simon, >>>>>>> >>>>>>> >>>>>>> >>>>>>> On 05-12-16 07:24, Simon Glass wrote: >>>>>>>> >>>>>>>> Hi Oliver, >>>>>>>> >>>>>>>> On 2 December 2016 at 03:16, Olliver Schinagl <oliver@schinagl.nl> >>>>>> wrote: >>>>>>>>> >>>>>>>>> Hey Joe, >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On 30-11-16 21:40, Joe Hershberger wrote: >>>>>>>>>> >>>>>>>>>> On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl >>>>>> <oliver@schinagl.nl> >>>>>>>>>> wrote: >>>>>>>>>>> >>>>>>>>>>> This patch adds a method for the board to set the MAC address >>>> if >>>>>> the >>>>>>>>>>> environment is not yet set. The environment based MAC addresses >>>>>> are not >>>>>>>>>>> touched, but if the fdt has an alias set, it is parsed and put >>>>>> into the >>>>>>>>>>> environment. >>>>>>>>>>> >>>>>>>>>>> E.g. The environment contains ethaddr and eth1addr, and the fdt >>>>>>>>>>> contains >>>>>>>>>>> an ethernet1 nothing happens. If the fdt contains ethernet2 >>>>>> however, it >>>>>>>>>>> is parsed and inserted into the environment as eth2addr. >>>>>>>>>>> >>>>>>>>>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> >>>>>>>>>>> --- >>>>>>>>>>> common/fdt_support.c | 8 +++++++- >>>>>>>>>>> 1 file changed, 7 insertions(+), 1 deletion(-) >>>>>>>>>>> >>>>>>>>>>> diff --git a/common/fdt_support.c b/common/fdt_support.c >>>>>>>>>>> index c34a13c..f127392 100644 >>>>>>>>>>> --- a/common/fdt_support.c >>>>>>>>>>> +++ b/common/fdt_support.c >>>>>>>>>>> @@ -465,6 +465,11 @@ int fdt_fixup_memory(void *blob, u64 >>>> start, >>>>>> u64 >>>>>>>>>>> size) >>>>>>>>>>> return fdt_fixup_memory_banks(blob, &start, &size, >>>> 1); >>>>>>>>>>> } >>>>>>>>>>> >>>>>>>>>>> +__weak int board_get_enetaddr(const int i, unsigned char >>>>>> *mac_addr) >>>>>>>>>> >>>>>>>>>> Ugh. This collides with a function in board/v38b/ethaddr.c and >>>> in >>>>>>>>>> board/intercontrol/digsy_mtc/digsy_mtc.c >>>>>>>>>> >>>>>>>>>> Also, it's so generic, and only gets called by the fdt fixup >>>>>> stuff... >>>>>>>>>> This function should probably be named in such a way that its >>>>>>>>>> association with fdt is clear. >>>>>>>>> >>>>>>>>> I did not notice that, sorry! But naming suggestions are welcome >>>> :) >>>>>>>>> >>>>>>>>> Right now, I use it in two unrelated spots however. >>>>>>>>> >>>>>>>>> from the fdt as seen above and in a subclass driver (which will >>>>>> come up >>>>>>>>> for >>>>>>>>> review) as suggested by Simon. >>>>>>>>> >>>>>>>>> There I do: >>>>>>>>> >>>>>>>>> +static int sunxi_gmac_eth_read_rom_hwaddr(struct udevice *dev) >>>>>>>>> +{ >>>>>>>>> + struct eth_pdata *pdata = dev_get_platdata(dev); >>>>>>>>> + >>>>>>>>> + return board_get_enetaddr(dev->seq, pdata->enetaddr); >>>>>>>>> +} >>>>>>>>> + >>>>>>>>> const struct eth_ops sunxi_gmac_eth_ops = { >>>>>>>>> .start = designware_eth_start, >>>>>>>>> .send = designware_eth_send, >>>>>>>>> @@ -102,6 +110,7 @@ const struct eth_ops sunxi_gmac_eth_ops = { >>>>>>>>> .free_pkt = designware_eth_free_pkt, >>>>>>>>> .stop = designware_eth_stop, >>>>>>>>> .write_hwaddr = designware_eth_write_hwaddr, >>>>>>>>> + .read_rom_hwaddr = sunxi_gmac_eth_read_rom_hwaddr, >>>>>>>>> }; >>>>>>>>> >>>>>>>>> which is completly unrelated to the fdt. >>>>>>>>> >>>>>>>>> So naming suggestion or overal suggestion how to handle this nice >>>>>> and >>>>>>>>> generically? >>>>>>>>> >>>>>>>>> Based from the name however I would think that all >>>>>> board_get_enetaddr's >>>>>>>>> work >>>>>>>>> the same however so should have been interchangeable? Or was that >>>>>> silly >>>>>>>>> thinking? >>>>>>>> >>>>>>>> Would it be possible to use a name without 'board' in it? I think >>>>>> this >>>>>>>> / hope is actually sunxi-specific code, not board-specific? >>>>>>> >>>>>>> You are actually correct, we take the serial number of the SoC >>>> (sunxi >>>>>>> specific) and generate a serial/MAC from it. So nothing to do with >>>>>> the >>>>>>> board. So I can just name it sunxi_gen_enetaddr(). Would that be >>>> then >>>>>> (much) >>>>>>> better? >>>>>>> >>>>>>> The reason why I went to 'board' with my mind, is because a) the >>>>>> original >>>>>>> mac gen code and b) the location was in board/sunxi/board.c. I >>>> think >>>>>> it is >>>>>>> thus also sensible to move it out of board/sunxi/board.c as indeed, >>>>>> it has >>>>>>> nothing to do with board(s). >>>>>> >>>>>> That sounds good to me - and you should be able to call it directly >>>>> >from the driver and avoid any weak functions, right? >>>>> The subclass driver can, the fdt fixup however still needs a weak >>>> fdt_get_enetaddr()? (Which in our case calls then sunxi_get_enetaddr() >>>> i think.) >>>> >>>> OK - I feel that the fdt fixups need a bit of thought. At the moment >>>> it is all pretty ad-hoc. >>> >>> Yeah i think i agree, but i guess thats a separate cleanup step generally, no? >> >> OK - are you able to take a look at this? > > Any update on this or any of the other patches in your series that I > did not apply last release? I was expecting a v2 to address some > things such as this patch. I have a few after rebasing u-boot-net/master and I just started yesterday to get back up to speed. I'll double check all review comments and send a v2 with the remaining patches. Sorry for taking so long here! Olliver > > Thanks! > -Joe >
Hey Simon, On 01-12-16 03:20, Simon Glass wrote: > Hi, > > On 25 November 2016 at 08:30, Olliver Schinagl <oliver@schinagl.nl> wrote: >> This patch adds a method for the board to set the MAC address if the >> environment is not yet set. The environment based MAC addresses are not >> touched, but if the fdt has an alias set, it is parsed and put into the >> environment. >> >> E.g. The environment contains ethaddr and eth1addr, and the fdt contains >> an ethernet1 nothing happens. If the fdt contains ethernet2 however, it >> is parsed and inserted into the environment as eth2addr. >> >> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> >> --- >> common/fdt_support.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/common/fdt_support.c b/common/fdt_support.c >> index c34a13c..f127392 100644 >> --- a/common/fdt_support.c >> +++ b/common/fdt_support.c >> @@ -465,6 +465,11 @@ int fdt_fixup_memory(void *blob, u64 start, u64 size) >> return fdt_fixup_memory_banks(blob, &start, &size, 1); >> } >> >> +__weak int board_get_enetaddr(const int i, unsigned char *mac_addr) >> +{ >> + return -ENOSYS; >> +} >> + >> void fdt_fixup_ethernet(void *fdt) >> { >> int i, prop; >> @@ -507,7 +512,8 @@ void fdt_fixup_ethernet(void *fdt) >> if (fdt_eth_addr) >> eth_parse_enetaddr(fdt_eth_addr, mac_addr); >> else >> - continue; >> + if (board_get_enetaddr(i, mac_addr) < 0) >> + continue; >> >> do_fixup_by_path(fdt, path, "mac-address", >> &mac_addr, 6, 0); >> -- >> 2.10.2 >> > > Much as I don't want to pin this on any one patch, but I feel that our > DT fixup stuff is a bit out of control. We have so many functions that > do this, and they are called from various places. At some point we > need to think about the infrastructure. > > IMO we should move to a linker list approach a bit like SPL did > recently (SPL_LOAD_IMAGE_METHOD). Then boards and subsystems can > register (at build-time) a fixup routine and they all get called one > after the other. > > We could also (later) add run-time support for registering fixups, > that drivers could use. We kinda left this out in the cold last time (or I did?) and I just sent my v5 of this patch-series (without this one). The gist from what I remember here was, to not introduce a weak function here, and have boards go back-forth to other subsystems. I belive one suggestion was to use a more specific name, lets say sunxi_get_hwaddr() for example and use that. Quite obviously this being the common/fdt_support, it would have to be generic. So I'm still at loss as to how to handle this here. Some background here, what the idea was I belive, that certain network adapters rely on the fdt to supply the MAC address to the driver, and so u-boot injects the mac address into the fdt. We currently hack this together I belive, by generating environment variables for non-existant devices (if the fdt has ethernet aliases) and when doing this bit of code, inject the generated environment variables into the fdt. I can't rewrite/fixup the fdt stuff, but am also not sure what an acceptable solution for this would be. Granted we really are hacking this into u-boot but I suppose we cannot escape this for legacy reasons. Devices expect the same generated mac address as before. Again, bar rewriting this stuff, what can we do as the current code in board/sunxi/board.c is just as ugly a wart. Olliver > > Regards, > Simon >
Resend with maxime's correct e-mail address and Jagain added to CC On 10-04-17 17:46, Olliver Schinagl wrote: > Hey Simon, > > On 01-12-16 03:20, Simon Glass wrote: >> Hi, >> >> On 25 November 2016 at 08:30, Olliver Schinagl <oliver@schinagl.nl> >> wrote: >>> This patch adds a method for the board to set the MAC address if the >>> environment is not yet set. The environment based MAC addresses are not >>> touched, but if the fdt has an alias set, it is parsed and put into the >>> environment. >>> >>> E.g. The environment contains ethaddr and eth1addr, and the fdt contains >>> an ethernet1 nothing happens. If the fdt contains ethernet2 however, it >>> is parsed and inserted into the environment as eth2addr. >>> >>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> >>> --- >>> common/fdt_support.c | 8 +++++++- >>> 1 file changed, 7 insertions(+), 1 deletion(-) >>> >>> diff --git a/common/fdt_support.c b/common/fdt_support.c >>> index c34a13c..f127392 100644 >>> --- a/common/fdt_support.c >>> +++ b/common/fdt_support.c >>> @@ -465,6 +465,11 @@ int fdt_fixup_memory(void *blob, u64 start, u64 >>> size) >>> return fdt_fixup_memory_banks(blob, &start, &size, 1); >>> } >>> >>> +__weak int board_get_enetaddr(const int i, unsigned char *mac_addr) >>> +{ >>> + return -ENOSYS; >>> +} >>> + >>> void fdt_fixup_ethernet(void *fdt) >>> { >>> int i, prop; >>> @@ -507,7 +512,8 @@ void fdt_fixup_ethernet(void *fdt) >>> if (fdt_eth_addr) >>> eth_parse_enetaddr(fdt_eth_addr, >>> mac_addr); >>> else >>> - continue; >>> + if (board_get_enetaddr(i, mac_addr) < 0) >>> + continue; >>> >>> do_fixup_by_path(fdt, path, "mac-address", >>> &mac_addr, 6, 0); >>> -- >>> 2.10.2 >>> >> >> Much as I don't want to pin this on any one patch, but I feel that our >> DT fixup stuff is a bit out of control. We have so many functions that >> do this, and they are called from various places. At some point we >> need to think about the infrastructure. >> >> IMO we should move to a linker list approach a bit like SPL did >> recently (SPL_LOAD_IMAGE_METHOD). Then boards and subsystems can >> register (at build-time) a fixup routine and they all get called one >> after the other. >> >> We could also (later) add run-time support for registering fixups, >> that drivers could use. > > We kinda left this out in the cold last time (or I did?) and I just sent > my v5 of this patch-series (without this one). > > The gist from what I remember here was, to not introduce a weak function > here, and have boards go back-forth to other subsystems. I belive one > suggestion was to use a more specific name, lets say sunxi_get_hwaddr() > for example and use that. Quite obviously this being the > common/fdt_support, it would have to be generic. > > So I'm still at loss as to how to handle this here. > > Some background here, what the idea was I belive, that certain network > adapters rely on the fdt to supply the MAC address to the driver, and so > u-boot injects the mac address into the fdt. > > We currently hack this together I belive, by generating environment > variables for non-existant devices (if the fdt has ethernet aliases) and > when doing this bit of code, inject the generated environment variables > into the fdt. > > I can't rewrite/fixup the fdt stuff, but am also not sure what an > acceptable solution for this would be. Granted we really are hacking > this into u-boot but I suppose we cannot escape this for legacy reasons. > Devices expect the same generated mac address as before. > > Again, bar rewriting this stuff, what can we do as the current code in > board/sunxi/board.c is just as ugly a wart. > > Olliver > >> >> Regards, >> Simon >>
diff --git a/common/fdt_support.c b/common/fdt_support.c index c34a13c..f127392 100644 --- a/common/fdt_support.c +++ b/common/fdt_support.c @@ -465,6 +465,11 @@ int fdt_fixup_memory(void *blob, u64 start, u64 size) return fdt_fixup_memory_banks(blob, &start, &size, 1); } +__weak int board_get_enetaddr(const int i, unsigned char *mac_addr) +{ + return -ENOSYS; +} + void fdt_fixup_ethernet(void *fdt) { int i, prop; @@ -507,7 +512,8 @@ void fdt_fixup_ethernet(void *fdt) if (fdt_eth_addr) eth_parse_enetaddr(fdt_eth_addr, mac_addr); else - continue; + if (board_get_enetaddr(i, mac_addr) < 0) + continue; do_fixup_by_path(fdt, path, "mac-address", &mac_addr, 6, 0);
This patch adds a method for the board to set the MAC address if the environment is not yet set. The environment based MAC addresses are not touched, but if the fdt has an alias set, it is parsed and put into the environment. E.g. The environment contains ethaddr and eth1addr, and the fdt contains an ethernet1 nothing happens. If the fdt contains ethernet2 however, it is parsed and inserted into the environment as eth2addr. Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> --- common/fdt_support.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)