diff mbox series

[RFC] dt-bindings: nvmem: u-boot,env: add any-name MAC cells compatible

Message ID 20231214153620.23998-1-zajec5@gmail.com
State New
Headers
Series [RFC] dt-bindings: nvmem: u-boot,env: add any-name MAC cells compatible |

Commit Message

Rafał Miłecki Dec. 14, 2023, 3:36 p.m. UTC 
  From: Rafał Miłecki <rafal@milecki.pl>

So far we had a property for "ethaddr" NVMEM cell containing base
Ethernet MAC address. The problem is vendors often pick non-standard
names for storing MAC(s) (other than "ethaddr"). A few names were
noticed over years:
1. "wanaddr" (Edimax, ELECOM, EnGenius, I-O DATA, Sitecom)
2. "et1macaddr" (ASUS)
3. "eth1addr" (Buffalo)
4. "athaddr" (EnGenius)
5. "baseMAC" (Netgear)
6. "mac" (Netgear)
7. "mac_addr" (Moxa)
and more ("HW_LAN_MAC", "HW_WAN_MAC", "INIC_MAC_ADDR", "LAN_MAC_ADDR",
"RADIOADDR0", "RADIOADDR1", "WAN_MAC_ADDR", "lan1_mac_addr", "wanmac",
"wmac1", "wmac2").

It doesn't make sense to add property for every possible MAC cell name.
Instead allow specifying cells with "mac" compatible.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
List of devices and their U-Boot MAC variables:
alphanetworks,asl56026) wanmac
asus,rt-ac65p) et1macaddr
asus,rt-ac85p) et1macaddr
belkin,f9k1109v1) HW_WAN_MAC + HW_LAN_MAC 
buffalo,ls220de) eth1addr
buffalo,ls421de) eth1addr
checkpoint,l-50) lan1_mac_addr
dovado,tiny-ac) INIC_MAC_ADDR
dovado,tiny-ac) LAN_MAC_ADDR + WAN_MAC_ADDR
edimax,ra21s) wanaddr
edimax,rg21s) wanaddr
elecom,wrc-2533ghbk-i) wanaddr
elecom,wrc-2533ghbk2-t) wanaddr
engenius,ecb1200) athaddr
engenius,ecb1750) athaddr
engenius,epg5000) wanaddr
engenius,epg600) wanaddr
engenius,esr1200) wanaddr
engenius,esr1750) wanaddr
engenius,esr600) wanaddr
engenius,esr600h) wanaddr
engenius,esr900) wanaddr
enterasys,ws-ap3705i) RADIOADDR0 + RADIOADDR1
iodata,wn-ac1167dgr) wanaddr
iodata,wn-ac1167gr) wanaddr
iodata,wn-ac1600dgr) wanaddr
iodata,wn-ac1600dgr2) wanaddr
iodata,wn-ac733gr3) wanaddr
iodata,wn-ag300dgr) wanaddr
iodata,wnpr2600g) wanaddr
moxa,awk-1137c) mac_addr
netgear,wax220) mac
netgear,wndap620) baseMAC
netgear,wndap660) baseMAC
ocedo,panda) wmac1 + wmac2
sitecom,wlr-7100) wanaddr
sitecom,wlr-8100) wanaddr

 .../devicetree/bindings/nvmem/u-boot,env.yaml | 33 +++++++++++++++++++
 1 file changed, 33 insertions(+)

Comments

Simon Glass Dec. 14, 2023, 9:27 p.m. UTC | #1 
Hi Rafał,

On Thu, 14 Dec 2023 at 08:36, Rafał Miłecki <zajec5@gmail.com> wrote:
>
> From: Rafał Miłecki <rafal@milecki.pl>
>
> So far we had a property for "ethaddr" NVMEM cell containing base
> Ethernet MAC address. The problem is vendors often pick non-standard
> names for storing MAC(s) (other than "ethaddr"). A few names were
> noticed over years:
> 1. "wanaddr" (Edimax, ELECOM, EnGenius, I-O DATA, Sitecom)
> 2. "et1macaddr" (ASUS)
> 3. "eth1addr" (Buffalo)
> 4. "athaddr" (EnGenius)
> 5. "baseMAC" (Netgear)
> 6. "mac" (Netgear)
> 7. "mac_addr" (Moxa)
> and more ("HW_LAN_MAC", "HW_WAN_MAC", "INIC_MAC_ADDR", "LAN_MAC_ADDR",
> "RADIOADDR0", "RADIOADDR1", "WAN_MAC_ADDR", "lan1_mac_addr", "wanmac",
> "wmac1", "wmac2").
>
> It doesn't make sense to add property for every possible MAC cell name.
> Instead allow specifying cells with "mac" compatible.
>
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> List of devices and their U-Boot MAC variables:
> alphanetworks,asl56026) wanmac
> asus,rt-ac65p) et1macaddr
> asus,rt-ac85p) et1macaddr
> belkin,f9k1109v1) HW_WAN_MAC + HW_LAN_MAC
> buffalo,ls220de) eth1addr
> buffalo,ls421de) eth1addr
> checkpoint,l-50) lan1_mac_addr
> dovado,tiny-ac) INIC_MAC_ADDR
> dovado,tiny-ac) LAN_MAC_ADDR + WAN_MAC_ADDR
> edimax,ra21s) wanaddr
> edimax,rg21s) wanaddr
> elecom,wrc-2533ghbk-i) wanaddr
> elecom,wrc-2533ghbk2-t) wanaddr
> engenius,ecb1200) athaddr
> engenius,ecb1750) athaddr
> engenius,epg5000) wanaddr
> engenius,epg600) wanaddr
> engenius,esr1200) wanaddr
> engenius,esr1750) wanaddr
> engenius,esr600) wanaddr
> engenius,esr600h) wanaddr
> engenius,esr900) wanaddr
> enterasys,ws-ap3705i) RADIOADDR0 + RADIOADDR1
> iodata,wn-ac1167dgr) wanaddr
> iodata,wn-ac1167gr) wanaddr
> iodata,wn-ac1600dgr) wanaddr
> iodata,wn-ac1600dgr2) wanaddr
> iodata,wn-ac733gr3) wanaddr
> iodata,wn-ag300dgr) wanaddr
> iodata,wnpr2600g) wanaddr
> moxa,awk-1137c) mac_addr
> netgear,wax220) mac
> netgear,wndap620) baseMAC
> netgear,wndap660) baseMAC
> ocedo,panda) wmac1 + wmac2
> sitecom,wlr-7100) wanaddr
> sitecom,wlr-8100) wanaddr
>
>  .../devicetree/bindings/nvmem/u-boot,env.yaml | 33 +++++++++++++++++++
>  1 file changed, 33 insertions(+)
>

Are these upstream U-Boots, or just vendor forks?

Regards,
Simon
Rafał Miłecki Dec. 18, 2023, 10:02 p.m. UTC | #2 
On 14.12.2023 22:27, Simon Glass wrote:
> On Thu, 14 Dec 2023 at 08:36, Rafał Miłecki <zajec5@gmail.com> wrote:
>>
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> So far we had a property for "ethaddr" NVMEM cell containing base
>> Ethernet MAC address. The problem is vendors often pick non-standard
>> names for storing MAC(s) (other than "ethaddr"). A few names were
>> noticed over years:
>> 1. "wanaddr" (Edimax, ELECOM, EnGenius, I-O DATA, Sitecom)
>> 2. "et1macaddr" (ASUS)
>> 3. "eth1addr" (Buffalo)
>> 4. "athaddr" (EnGenius)
>> 5. "baseMAC" (Netgear)
>> 6. "mac" (Netgear)
>> 7. "mac_addr" (Moxa)
>> and more ("HW_LAN_MAC", "HW_WAN_MAC", "INIC_MAC_ADDR", "LAN_MAC_ADDR",
>> "RADIOADDR0", "RADIOADDR1", "WAN_MAC_ADDR", "lan1_mac_addr", "wanmac",
>> "wmac1", "wmac2").
>>
>> It doesn't make sense to add property for every possible MAC cell name.
>> Instead allow specifying cells with "mac" compatible.
>>
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> ---
>> List of devices and their U-Boot MAC variables:
>> alphanetworks,asl56026) wanmac
>> asus,rt-ac65p) et1macaddr
>> asus,rt-ac85p) et1macaddr
>> belkin,f9k1109v1) HW_WAN_MAC + HW_LAN_MAC
>> buffalo,ls220de) eth1addr
>> buffalo,ls421de) eth1addr
>> checkpoint,l-50) lan1_mac_addr
>> dovado,tiny-ac) INIC_MAC_ADDR
>> dovado,tiny-ac) LAN_MAC_ADDR + WAN_MAC_ADDR
>> edimax,ra21s) wanaddr
>> edimax,rg21s) wanaddr
>> elecom,wrc-2533ghbk-i) wanaddr
>> elecom,wrc-2533ghbk2-t) wanaddr
>> engenius,ecb1200) athaddr
>> engenius,ecb1750) athaddr
>> engenius,epg5000) wanaddr
>> engenius,epg600) wanaddr
>> engenius,esr1200) wanaddr
>> engenius,esr1750) wanaddr
>> engenius,esr600) wanaddr
>> engenius,esr600h) wanaddr
>> engenius,esr900) wanaddr
>> enterasys,ws-ap3705i) RADIOADDR0 + RADIOADDR1
>> iodata,wn-ac1167dgr) wanaddr
>> iodata,wn-ac1167gr) wanaddr
>> iodata,wn-ac1600dgr) wanaddr
>> iodata,wn-ac1600dgr2) wanaddr
>> iodata,wn-ac733gr3) wanaddr
>> iodata,wn-ag300dgr) wanaddr
>> iodata,wnpr2600g) wanaddr
>> moxa,awk-1137c) mac_addr
>> netgear,wax220) mac
>> netgear,wndap620) baseMAC
>> netgear,wndap660) baseMAC
>> ocedo,panda) wmac1 + wmac2
>> sitecom,wlr-7100) wanaddr
>> sitecom,wlr-8100) wanaddr
>>
>>   .../devicetree/bindings/nvmem/u-boot,env.yaml | 33 +++++++++++++++++++
>>   1 file changed, 33 insertions(+)
>>
> 
> Are these upstream U-Boots, or just vendor forks?

I guess most of those devices don't have upstream U-Boot support. Please
note that while upstreaming vendors changes would be great in most cases
it doesn't happen. Most vendors sadly don't care and most end users
don't have enough time for that. In practice we often stick with vendor
provided bootloader to flash and boot self build Linux system (like
OpenWrt).

I'm not sure if/how does it help with this PATCH but please note that
upstream U-Boot code also supports few extra variables.

There is generic eth_env_get_enetaddr_by_index() that supports variables
like "<%s><%d>addr" and "<%s>addr". Right now it's used only for
"eth<%d>addr" and "ethaddr" so that mostly limits us to "ethaddr",
"eth1addr", "eth2addr" and "eth3addr".

 From some rare cases: there are also "usbnet_devaddr" and "wolpassword".

So given that U-Boot oficially supports at least 6 env variables for
MAC and there are many used with custom U-Boots and firmwares this
binding would help a lot.
Krzysztof Kozlowski Dec. 25, 2023, 1:11 p.m. UTC | #3 
On 18/12/2023 23:02, Rafał Miłecki wrote:
> On 14.12.2023 22:27, Simon Glass wrote:
>> On Thu, 14 Dec 2023 at 08:36, Rafał Miłecki <zajec5@gmail.com> wrote:
>>>
>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>
>>> So far we had a property for "ethaddr" NVMEM cell containing base
>>> Ethernet MAC address. The problem is vendors often pick non-standard
>>> names for storing MAC(s) (other than "ethaddr"). A few names were
>>> noticed over years:
>>> 1. "wanaddr" (Edimax, ELECOM, EnGenius, I-O DATA, Sitecom)
>>> 2. "et1macaddr" (ASUS)
>>> 3. "eth1addr" (Buffalo)
>>> 4. "athaddr" (EnGenius)
>>> 5. "baseMAC" (Netgear)
>>> 6. "mac" (Netgear)
>>> 7. "mac_addr" (Moxa)
>>> and more ("HW_LAN_MAC", "HW_WAN_MAC", "INIC_MAC_ADDR", "LAN_MAC_ADDR",
>>> "RADIOADDR0", "RADIOADDR1", "WAN_MAC_ADDR", "lan1_mac_addr", "wanmac",
>>> "wmac1", "wmac2").
>>>
>>> It doesn't make sense to add property for every possible MAC cell name.
>>> Instead allow specifying cells with "mac" compatible.
>>>
>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>> ---
>>> List of devices and their U-Boot MAC variables:
>>> alphanetworks,asl56026) wanmac
>>> asus,rt-ac65p) et1macaddr
>>> asus,rt-ac85p) et1macaddr
>>> belkin,f9k1109v1) HW_WAN_MAC + HW_LAN_MAC
>>> buffalo,ls220de) eth1addr
>>> buffalo,ls421de) eth1addr
>>> checkpoint,l-50) lan1_mac_addr
>>> dovado,tiny-ac) INIC_MAC_ADDR
>>> dovado,tiny-ac) LAN_MAC_ADDR + WAN_MAC_ADDR
>>> edimax,ra21s) wanaddr
>>> edimax,rg21s) wanaddr
>>> elecom,wrc-2533ghbk-i) wanaddr
>>> elecom,wrc-2533ghbk2-t) wanaddr
>>> engenius,ecb1200) athaddr
>>> engenius,ecb1750) athaddr
>>> engenius,epg5000) wanaddr
>>> engenius,epg600) wanaddr
>>> engenius,esr1200) wanaddr
>>> engenius,esr1750) wanaddr
>>> engenius,esr600) wanaddr
>>> engenius,esr600h) wanaddr
>>> engenius,esr900) wanaddr
>>> enterasys,ws-ap3705i) RADIOADDR0 + RADIOADDR1
>>> iodata,wn-ac1167dgr) wanaddr
>>> iodata,wn-ac1167gr) wanaddr
>>> iodata,wn-ac1600dgr) wanaddr
>>> iodata,wn-ac1600dgr2) wanaddr
>>> iodata,wn-ac733gr3) wanaddr
>>> iodata,wn-ag300dgr) wanaddr
>>> iodata,wnpr2600g) wanaddr
>>> moxa,awk-1137c) mac_addr
>>> netgear,wax220) mac
>>> netgear,wndap620) baseMAC
>>> netgear,wndap660) baseMAC
>>> ocedo,panda) wmac1 + wmac2
>>> sitecom,wlr-7100) wanaddr
>>> sitecom,wlr-8100) wanaddr
>>>
>>>   .../devicetree/bindings/nvmem/u-boot,env.yaml | 33 +++++++++++++++++++
>>>   1 file changed, 33 insertions(+)
>>>
>>
>> Are these upstream U-Boots, or just vendor forks?
> 
> I guess most of those devices don't have upstream U-Boot support. Please

We do not document properties used in all possible projects, like vendor
forks. Only upstream U-Boot matters.

> note that while upstreaming vendors changes would be great in most cases
> it doesn't happen. Most vendors sadly don't care and most end users
> don't have enough time for that. In practice we often stick with vendor
> provided bootloader to flash and boot self build Linux system (like
> OpenWrt).
> 
> I'm not sure if/how does it help with this PATCH but please note that
> upstream U-Boot code also supports few extra variables.
> 
> There is generic eth_env_get_enetaddr_by_index() that supports variables
> like "<%s><%d>addr" and "<%s>addr". Right now it's used only for
> "eth<%d>addr" and "ethaddr" so that mostly limits us to "ethaddr",
> "eth1addr", "eth2addr" and "eth3addr".
> 
>  From some rare cases: there are also "usbnet_devaddr" and "wolpassword".
> 
> So given that U-Boot oficially supports at least 6 env variables for
> MAC and there are many used with custom U-Boots and firmwares this
> binding would help a lot.

Please limit this to upstream U-Boot. Drop all custom and firmware ones.

Then, just fix upstream U-Boot to have only one property...

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml b/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml
index 0006f022d0af..68214b96f5c9 100644
--- a/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml
+++ b/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml
@@ -67,6 +67,34 @@  properties:
         description: The first argument is a MAC address offset.
         const: 1
 
+patternProperties:
+  ".*":
+    allOf:
+      - if:
+          type: object
+        then:
+          properties:
+            compatible:
+              enum:
+                - mac
+      - if:
+          properties:
+            compatible:
+              contains:
+                const: mac
+        then:
+          description:
+            Ethernet interfaces base MAC address.
+
+          properties:
+            compatible: true
+
+            "#nvmem-cell-cells":
+              description: The first argument is a MAC address offset.
+              const: 1
+
+          additionalProperties: false
+
 additionalProperties: false
 
 examples:
@@ -90,6 +118,11 @@  examples:
             mac: ethaddr {
                 #nvmem-cell-cells = <1>;
             };
+
+            wanaddr {
+                compatible = "mac";
+                #nvmem-cell-cells = <1>;
+            };
         };
     };
   - |