| Message ID | 20210312133602.31105-3-bmeng.cn@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Priyanka Jain |
| Headers | show |
| Series | ppc: qemu: Add eTSEC support | expand |
On Fri, Mar 12, 2021 at 09:35:43PM +0800, Bin Meng wrote: > Introduce a helper API ofnode_phy_is_fixed_link() to detect whether > the ethernet controller connects to a fixed-link pseudo-PHY device. > > Note there are two ways to describe a fixed PHY attached to an > Ethernet device: > > - the new DT binding, where 'fixed-link' is a sub-node of the > Ethernet device > - the old DT binding, where 'fixed-link' is a property with 5 > cells encoding various information about the fixed PHY > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > Reviewed-by: Simon Glass <sjg@chromium.org> > --- I ran a 'grep -r "ofnode_get_property.*fixed-link" .' and saw no in-tree users of the old binding. Why do we bother to be compatible with something which isn't used?
On Sat, Mar 13, 2021 at 02:14:36PM +0200, Vladimir Oltean wrote: > On Fri, Mar 12, 2021 at 09:35:43PM +0800, Bin Meng wrote: > > Introduce a helper API ofnode_phy_is_fixed_link() to detect whether > > the ethernet controller connects to a fixed-link pseudo-PHY device. > > > > Note there are two ways to describe a fixed PHY attached to an > > Ethernet device: > > > > - the new DT binding, where 'fixed-link' is a sub-node of the > > Ethernet device > > - the old DT binding, where 'fixed-link' is a property with 5 > > cells encoding various information about the fixed PHY > > > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > > Reviewed-by: Simon Glass <sjg@chromium.org> > > --- > > I ran a 'grep -r "ofnode_get_property.*fixed-link" .' and saw no in-tree > users of the old binding. Why do we bother to be compatible with > something which isn't used? Ah, I see what's going on. QEMU fixes up the device tree here: https://github.com/qemu/qemu/blob/master/hw/ppc/e500.c#L239 and adds an old-style fixed-link binding. Can't you modify it to add a new-style fixed-link property? It's not like you didn't have to modify it for the "ranges" property too :) https://github.com/qemu/qemu/commit/e5943b00d35efc68ca72ed304cfca98a9f3a647c
On Fri, Mar 12, 2021 at 09:35:43PM +0800, Bin Meng wrote: > +bool ofnode_phy_is_fixed_link(ofnode eth_node, ofnode *phy_node) > +{ > + bool found = false; > + ofnode node, subnode; > + int len; > + > + /* new binding */ > + subnode = ofnode_find_subnode(eth_node, "fixed-link"); > + if (ofnode_valid(subnode)) { > + node = subnode; > + found = true; > + } > + > + /* old binding */ > + if (ofnode_get_property(eth_node, "fixed-link", &len) && Maybe "else if" here? If an old-style and a new-style binding exist, we should prefer looking at the new one. And with that "else if", we could remove the "found" variable: if (ofnode_valid(subnode)) { } else if (ofnode_get_property(eth_node, "fixed-link", &len) && len == (5 * sizeof(__be32))) { } else { return false; } if (phy_node) *phy_node = node; > + len == (5 * sizeof(__be32))) { > + node = eth_node; > + found = true; > + } > + > + if (found && phy_node) > + *phy_node = node; > + > + return found; > +}
Hi Vladimir, On Sat, Mar 13, 2021 at 9:03 PM Vladimir Oltean <olteanv@gmail.com> wrote: > > On Fri, Mar 12, 2021 at 09:35:43PM +0800, Bin Meng wrote: > > +bool ofnode_phy_is_fixed_link(ofnode eth_node, ofnode *phy_node) > > +{ > > + bool found = false; > > + ofnode node, subnode; > > + int len; > > + > > + /* new binding */ > > + subnode = ofnode_find_subnode(eth_node, "fixed-link"); > > + if (ofnode_valid(subnode)) { > > + node = subnode; > > + found = true; > > + } > > + > > + /* old binding */ > > + if (ofnode_get_property(eth_node, "fixed-link", &len) && > > Maybe "else if" here? If an old-style and a new-style binding exist, we > should prefer looking at the new one. > Agree. Will do in v3. > And with that "else if", we could remove the "found" variable: Regards, Bin
Hi Vladimir, On Sat, Mar 13, 2021 at 8:29 PM Vladimir Oltean <olteanv@gmail.com> wrote: > > On Sat, Mar 13, 2021 at 02:14:36PM +0200, Vladimir Oltean wrote: > > On Fri, Mar 12, 2021 at 09:35:43PM +0800, Bin Meng wrote: > > > Introduce a helper API ofnode_phy_is_fixed_link() to detect whether > > > the ethernet controller connects to a fixed-link pseudo-PHY device. > > > > > > Note there are two ways to describe a fixed PHY attached to an > > > Ethernet device: > > > > > > - the new DT binding, where 'fixed-link' is a sub-node of the > > > Ethernet device > > > - the old DT binding, where 'fixed-link' is a property with 5 > > > cells encoding various information about the fixed PHY > > > > > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > --- > > > > I ran a 'grep -r "ofnode_get_property.*fixed-link" .' and saw no in-tree > > users of the old binding. Why do we bother to be compatible with > > something which isn't used? > > Ah, I see what's going on. > QEMU fixes up the device tree here: > https://github.com/qemu/qemu/blob/master/hw/ppc/e500.c#L239 > and adds an old-style fixed-link binding. > Can't you modify it to add a new-style fixed-link property? I am afraid that may break guests that haven't supported new-style DT bindings yet. > It's not like you didn't have to modify it for the "ranges" property too :) > https://github.com/qemu/qemu/commit/e5943b00d35efc68ca72ed304cfca98a9f3a647c Regards, Bin
diff --git a/drivers/core/of_extra.c b/drivers/core/of_extra.c index 653344529e..a16f9a8dc1 100644 --- a/drivers/core/of_extra.c +++ b/drivers/core/of_extra.c @@ -130,3 +130,29 @@ int ofnode_decode_memory_region(ofnode config_node, const char *mem_type, return 0; } + +bool ofnode_phy_is_fixed_link(ofnode eth_node, ofnode *phy_node) +{ + bool found = false; + ofnode node, subnode; + int len; + + /* new binding */ + subnode = ofnode_find_subnode(eth_node, "fixed-link"); + if (ofnode_valid(subnode)) { + node = subnode; + found = true; + } + + /* old binding */ + if (ofnode_get_property(eth_node, "fixed-link", &len) && + len == (5 * sizeof(__be32))) { + node = eth_node; + found = true; + } + + if (found && phy_node) + *phy_node = node; + + return found; +} diff --git a/include/dm/of_extra.h b/include/dm/of_extra.h index ca15df21b0..3641f68fe3 100644 --- a/include/dm/of_extra.h +++ b/include/dm/of_extra.h @@ -86,4 +86,22 @@ int ofnode_decode_memory_region(ofnode config_node, const char *mem_type, const char *suffix, fdt_addr_t *basep, fdt_size_t *sizep); +/** + * ofnode_phy_is_fixed_link() - Detect fixed-link pseudo-PHY device + * + * This function detects whether the ethernet controller connects to a + * fixed-link pseudo-PHY device. + * + * This function supports the following two DT bindings: + * - the new DT binding, where 'fixed-link' is a sub-node of the + * Ethernet device + * - the old DT binding, where 'fixed-link' is a property with 5 + * cells encoding various information about the fixed PHY + * + * @param eth_node ofnode containing the fixed-link subnode/property + * @param phy_node if fixed-link PHY detected, containing the PHY ofnode + * @return true if a fixed-link pseudo-PHY device exists, false otherwise + */ +bool ofnode_phy_is_fixed_link(ofnode eth_node, ofnode *phy_node); + #endif