changes v4:
- remove unused phy_id variable
changes v3:
- export part of of_mdiobus_register_phy() and reuse it in tja11xx
driver
- coding style fixes
changes v2:
- use .match_phy_device
- add irq support
- add add delayed registration for PHY1
Oleksij Rempel (4):
dt-bindings: net: phy: Add support for NXP TJA11xx
net: phy: tja11xx: add initial TJA1102 support
net: mdio: of: export part of of_mdiobus_register_phy()
net: phy: tja11xx: add delayed registration of TJA1102 PHY1
.../devicetree/bindings/net/nxp,tja11xx.yaml | 61 ++++++
drivers/net/phy/nxp-tja11xx.c | 201 +++++++++++++++++-
drivers/of/of_mdio.c | 73 ++++---
include/linux/of_mdio.h | 11 +-
4 files changed, 308 insertions(+), 38 deletions(-)
create mode 100644 Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
--
2.25.1
TJA1102 is a dual PHY package with PHY0 having proper PHYID and PHY1
having no ID. On one hand it is possible to for PHY detection by
compatible, on other hand we should be able to reset complete chip
before PHY1 configured it, and we need to define dependencies for proper
power management.
We can solve it by defining PHY1 as child of PHY0:
tja1102_phy0: ethernet-phy@4 {
reg = <0x4>;
interrupts-extended = <&gpio5 8 IRQ_TYPE_LEVEL_LOW>;
reset-gpios = <&gpio5 9 GPIO_ACTIVE_LOW>;
reset-assert-us = <20>;
reset-deassert-us = <2000>;
tja1102_phy1: ethernet-phy@5 {
reg = <0x5>;
interrupts-extended = <&gpio5 8 IRQ_TYPE_LEVEL_LOW>;
};
};
The PHY1 should be a subnode of PHY0 and registered only after PHY0 was
completely reset and initialized.
Signed-off-by: Oleksij Rempel <[email protected]>
---
drivers/net/phy/nxp-tja11xx.c | 112 +++++++++++++++++++++++++++++++---
1 file changed, 105 insertions(+), 7 deletions(-)
diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
index a064e4ab3616..2bde9386baf1 100644
--- a/drivers/net/phy/nxp-tja11xx.c
+++ b/drivers/net/phy/nxp-tja11xx.c
@@ -6,11 +6,14 @@
#include <linux/delay.h>
#include <linux/ethtool.h>
#include <linux/kernel.h>
+#include <linux/mdio.h>
#include <linux/mii.h>
#include <linux/module.h>
#include <linux/phy.h>
#include <linux/hwmon.h>
#include <linux/bitfield.h>
+#include <linux/of_mdio.h>
+#include <linux/of_irq.h>
#define PHY_ID_MASK 0xfffffff0
#define PHY_ID_TJA1100 0x0180dc40
@@ -57,6 +60,8 @@
struct tja11xx_priv {
char *hwmon_name;
struct device *hwmon_dev;
+ struct phy_device *phydev;
+ struct work_struct phy_register_work;
};
struct tja11xx_phy_stats {
@@ -333,16 +338,12 @@ static const struct hwmon_chip_info tja11xx_hwmon_chip_info = {
.info = tja11xx_hwmon_info,
};
-static int tja11xx_probe(struct phy_device *phydev)
+static int tja11xx_hwmon_register(struct phy_device *phydev,
+ struct tja11xx_priv *priv)
{
struct device *dev = &phydev->mdio.dev;
- struct tja11xx_priv *priv;
int i;
- priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
- if (!priv)
- return -ENOMEM;
-
priv->hwmon_name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
if (!priv->hwmon_name)
return -ENOMEM;
@@ -360,6 +361,103 @@ static int tja11xx_probe(struct phy_device *phydev)
return PTR_ERR_OR_ZERO(priv->hwmon_dev);
}
+static int tja11xx_probe(struct phy_device *phydev)
+{
+ struct device *dev = &phydev->mdio.dev;
+ struct tja11xx_priv *priv;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->phydev = phydev;
+
+ return tja11xx_hwmon_register(phydev, priv);
+}
+
+static void tja1102_p1_register(struct work_struct *work)
+{
+ struct tja11xx_priv *priv = container_of(work, struct tja11xx_priv,
+ phy_register_work);
+ struct phy_device *phydev_phy0 = priv->phydev;
+ struct mii_bus *bus = phydev_phy0->mdio.bus;
+ struct device *dev = &phydev_phy0->mdio.dev;
+ struct device_node *np = dev->of_node;
+ struct device_node *child;
+ int ret;
+
+ for_each_available_child_of_node(np, child) {
+ struct phy_device *phy;
+ int addr;
+
+ addr = of_mdio_parse_addr(dev, child);
+ if (addr < 0) {
+ dev_err(dev, "Can't parse addr\n");
+ continue;
+ } else if (addr != phydev_phy0->mdio.addr + 1) {
+ /* Currently we care only about double PHY chip TJA1102.
+ * If some day NXP will decide to bring chips with more
+ * PHYs, this logic should be reworked.
+ */
+ dev_err(dev, "Unexpected address. Should be: %i\n",
+ phydev_phy0->mdio.addr + 1);
+ continue;
+ }
+
+ if (mdiobus_is_registered_device(bus, addr)) {
+ dev_err(dev, "device is already registered\n");
+ continue;
+ }
+
+ /* Real PHY ID of Port 1 is 0 */
+ phy = phy_device_create(bus, addr, PHY_ID_TJA1102, false, NULL);
+ if (IS_ERR(phy)) {
+ dev_err(dev, "Can't create PHY device for Port 1: %i\n",
+ addr);
+ continue;
+ }
+
+ /* Overwrite parent device. phy_device_create() set parent to
+ * the mii_bus->dev, which is not correct in case.
+ */
+ phy->mdio.dev.parent = dev;
+
+ ret = __of_mdiobus_register_phy(bus, phy, child, addr);
+ if (ret) {
+ /* All resources needed for Port 1 should be already
+ * available for Port 0. Both ports use the same
+ * interrupt line, so -EPROBE_DEFER would make no sense
+ * here.
+ */
+ dev_err(dev, "Can't register Port 1. Unexpected error: %i\n",
+ ret);
+ phy_device_free(phy);
+ }
+ }
+}
+
+static int tja1102_p0_probe(struct phy_device *phydev)
+{
+ struct device *dev = &phydev->mdio.dev;
+ struct tja11xx_priv *priv;
+ int ret;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->phydev = phydev;
+ INIT_WORK(&priv->phy_register_work, tja1102_p1_register);
+
+ ret = tja11xx_hwmon_register(phydev, priv);
+ if (ret)
+ return ret;
+
+ schedule_work(&priv->phy_register_work);
+
+ return 0;
+}
+
static int tja1102_match_phy_device(struct phy_device *phydev, bool port0)
{
int ret;
@@ -443,7 +541,7 @@ static struct phy_driver tja11xx_driver[] = {
}, {
.name = "NXP TJA1102 Port 0",
.features = PHY_BASIC_T1_FEATURES,
- .probe = tja11xx_probe,
+ .probe = tja1102_p0_probe,
.soft_reset = tja11xx_soft_reset,
.config_init = tja11xx_config_init,
.read_status = tja11xx_read_status,
--
2.25.1
TJA1102 is an dual T1 PHY chip. Both PHYs are separately addressable.
Both PHYs are similar but have different amount of functionality. For
example PHY 1 has no PHY ID and no health monitor.
Signed-off-by: Oleksij Rempel <[email protected]>
---
drivers/net/phy/nxp-tja11xx.c | 91 +++++++++++++++++++++++++++++++++++
1 file changed, 91 insertions(+)
diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
index b705d0bd798b..a064e4ab3616 100644
--- a/drivers/net/phy/nxp-tja11xx.c
+++ b/drivers/net/phy/nxp-tja11xx.c
@@ -15,6 +15,7 @@
#define PHY_ID_MASK 0xfffffff0
#define PHY_ID_TJA1100 0x0180dc40
#define PHY_ID_TJA1101 0x0180dd00
+#define PHY_ID_TJA1102 0x0180dc80
#define MII_ECTRL 17
#define MII_ECTRL_LINK_CONTROL BIT(15)
@@ -40,6 +41,10 @@
#define MII_INTSRC_TEMP_ERR BIT(1)
#define MII_INTSRC_UV_ERR BIT(3)
+#define MII_INTEN 22
+#define MII_INTEN_LINK_FAIL BIT(10)
+#define MII_INTEN_LINK_UP BIT(9)
+
#define MII_COMMSTAT 23
#define MII_COMMSTAT_LINK_UP BIT(15)
@@ -190,6 +195,7 @@ static int tja11xx_config_init(struct phy_device *phydev)
return ret;
break;
case PHY_ID_TJA1101:
+ case PHY_ID_TJA1102:
ret = phy_set_bits(phydev, MII_COMMCFG, MII_COMMCFG_AUTO_OP);
if (ret)
return ret;
@@ -354,6 +360,55 @@ static int tja11xx_probe(struct phy_device *phydev)
return PTR_ERR_OR_ZERO(priv->hwmon_dev);
}
+static int tja1102_match_phy_device(struct phy_device *phydev, bool port0)
+{
+ int ret;
+
+ if ((phydev->phy_id & PHY_ID_MASK) != PHY_ID_TJA1102)
+ return 0;
+
+ ret = phy_read(phydev, MII_PHYSID2);
+ if (ret < 0)
+ return ret;
+
+ /* TJA1102 Port 1 has phyid 0 and doesn't support temperature
+ * and undervoltage alarms.
+ */
+ if (port0)
+ return ret ? 1 : 0;
+
+ return !ret;
+}
+
+static int tja1102_p0_match_phy_device(struct phy_device *phydev)
+{
+ return tja1102_match_phy_device(phydev, true);
+}
+
+static int tja1102_p1_match_phy_device(struct phy_device *phydev)
+{
+ return tja1102_match_phy_device(phydev, false);
+}
+
+static int tja11xx_ack_interrupt(struct phy_device *phydev)
+{
+ int ret;
+
+ ret = phy_read(phydev, MII_INTSRC);
+
+ return (ret < 0) ? ret : 0;
+}
+
+static int tja11xx_config_intr(struct phy_device *phydev)
+{
+ int value = 0;
+
+ if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+ value = MII_INTEN_LINK_FAIL | MII_INTEN_LINK_UP;
+
+ return phy_write(phydev, MII_INTEN, value);
+}
+
static struct phy_driver tja11xx_driver[] = {
{
PHY_ID_MATCH_MODEL(PHY_ID_TJA1100),
@@ -385,6 +440,41 @@ static struct phy_driver tja11xx_driver[] = {
.get_sset_count = tja11xx_get_sset_count,
.get_strings = tja11xx_get_strings,
.get_stats = tja11xx_get_stats,
+ }, {
+ .name = "NXP TJA1102 Port 0",
+ .features = PHY_BASIC_T1_FEATURES,
+ .probe = tja11xx_probe,
+ .soft_reset = tja11xx_soft_reset,
+ .config_init = tja11xx_config_init,
+ .read_status = tja11xx_read_status,
+ .match_phy_device = tja1102_p0_match_phy_device,
+ .suspend = genphy_suspend,
+ .resume = genphy_resume,
+ .set_loopback = genphy_loopback,
+ /* Statistics */
+ .get_sset_count = tja11xx_get_sset_count,
+ .get_strings = tja11xx_get_strings,
+ .get_stats = tja11xx_get_stats,
+ .ack_interrupt = tja11xx_ack_interrupt,
+ .config_intr = tja11xx_config_intr,
+
+ }, {
+ .name = "NXP TJA1102 Port 1",
+ .features = PHY_BASIC_T1_FEATURES,
+ /* currently no probe for Port 1 is need */
+ .soft_reset = tja11xx_soft_reset,
+ .config_init = tja11xx_config_init,
+ .read_status = tja11xx_read_status,
+ .match_phy_device = tja1102_p1_match_phy_device,
+ .suspend = genphy_suspend,
+ .resume = genphy_resume,
+ .set_loopback = genphy_loopback,
+ /* Statistics */
+ .get_sset_count = tja11xx_get_sset_count,
+ .get_strings = tja11xx_get_strings,
+ .get_stats = tja11xx_get_stats,
+ .ack_interrupt = tja11xx_ack_interrupt,
+ .config_intr = tja11xx_config_intr,
}
};
@@ -393,6 +483,7 @@ module_phy_driver(tja11xx_driver);
static struct mdio_device_id __maybe_unused tja11xx_tbl[] = {
{ PHY_ID_MATCH_MODEL(PHY_ID_TJA1100) },
{ PHY_ID_MATCH_MODEL(PHY_ID_TJA1101) },
+ { PHY_ID_MATCH_MODEL(PHY_ID_TJA1102) },
{ }
};
--
2.25.1
This function will be needed in tja11xx driver for secondary PHY
support.
Signed-off-by: Oleksij Rempel <[email protected]>
---
drivers/of/of_mdio.c | 73 ++++++++++++++++++++++++-----------------
include/linux/of_mdio.h | 11 ++++++-
2 files changed, 52 insertions(+), 32 deletions(-)
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index 8270bbf505fb..d9e637b624ce 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -60,39 +60,15 @@ static struct mii_timestamper *of_find_mii_timestamper(struct device_node *node)
return register_mii_timestamper(arg.np, arg.args[0]);
}
-static int of_mdiobus_register_phy(struct mii_bus *mdio,
- struct device_node *child, u32 addr)
+int __of_mdiobus_register_phy(struct mii_bus *mdio, struct phy_device *phy,
+ struct device_node *child, u32 addr)
{
- struct mii_timestamper *mii_ts;
- struct phy_device *phy;
- bool is_c45;
int rc;
- u32 phy_id;
-
- mii_ts = of_find_mii_timestamper(child);
- if (IS_ERR(mii_ts))
- return PTR_ERR(mii_ts);
-
- is_c45 = of_device_is_compatible(child,
- "ethernet-phy-ieee802.3-c45");
-
- if (!is_c45 && !of_get_phy_id(child, &phy_id))
- phy = phy_device_create(mdio, addr, phy_id, 0, NULL);
- else
- phy = get_phy_device(mdio, addr, is_c45);
- if (IS_ERR(phy)) {
- if (mii_ts)
- unregister_mii_timestamper(mii_ts);
- return PTR_ERR(phy);
- }
rc = of_irq_get(child, 0);
- if (rc == -EPROBE_DEFER) {
- if (mii_ts)
- unregister_mii_timestamper(mii_ts);
- phy_device_free(phy);
+ if (rc == -EPROBE_DEFER)
return rc;
- }
+
if (rc > 0) {
phy->irq = rc;
mdio->irq[addr] = rc;
@@ -117,11 +93,48 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio,
/* All data is now stored in the phy struct;
* register it */
rc = phy_device_register(phy);
+ if (rc) {
+ of_node_put(child);
+ return rc;
+ }
+
+ dev_dbg(&mdio->dev, "registered phy %pOFn at address %i\n",
+ child, addr);
+ return 0;
+}
+EXPORT_SYMBOL(__of_mdiobus_register_phy);
+
+static int of_mdiobus_register_phy(struct mii_bus *mdio,
+ struct device_node *child, u32 addr)
+{
+ struct mii_timestamper *mii_ts;
+ struct phy_device *phy;
+ bool is_c45;
+ int rc;
+ u32 phy_id;
+
+ mii_ts = of_find_mii_timestamper(child);
+ if (IS_ERR(mii_ts))
+ return PTR_ERR(mii_ts);
+
+ is_c45 = of_device_is_compatible(child,
+ "ethernet-phy-ieee802.3-c45");
+
+ if (!is_c45 && !of_get_phy_id(child, &phy_id))
+ phy = phy_device_create(mdio, addr, phy_id, 0, NULL);
+ else
+ phy = get_phy_device(mdio, addr, is_c45);
+ if (IS_ERR(phy)) {
+ if (mii_ts)
+ unregister_mii_timestamper(mii_ts);
+ return PTR_ERR(phy);
+ }
+
+ rc = __of_mdiobus_register_phy(mdio, phy, child, addr);
if (rc) {
if (mii_ts)
unregister_mii_timestamper(mii_ts);
phy_device_free(phy);
- of_node_put(child);
return rc;
}
@@ -132,8 +145,6 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio,
if (mii_ts)
phy->mii_ts = mii_ts;
- dev_dbg(&mdio->dev, "registered phy %pOFn at address %i\n",
- child, addr);
return 0;
}
diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h
index 491a2b7e77c1..b99e9f932002 100644
--- a/include/linux/of_mdio.h
+++ b/include/linux/of_mdio.h
@@ -30,7 +30,9 @@ extern struct mii_bus *of_mdio_find_bus(struct device_node *mdio_np);
extern int of_phy_register_fixed_link(struct device_node *np);
extern void of_phy_deregister_fixed_link(struct device_node *np);
extern bool of_phy_is_fixed_link(struct device_node *np);
-
+extern int __of_mdiobus_register_phy(struct mii_bus *mdio,
+ struct phy_device *phy,
+ struct device_node *child, u32 addr);
static inline int of_mdio_parse_addr(struct device *dev,
const struct device_node *np)
@@ -118,6 +120,13 @@ static inline bool of_phy_is_fixed_link(struct device_node *np)
{
return false;
}
+
+static inline int __of_mdiobus_register_phy(struct mii_bus *mdio,
+ struct phy_device *phy,
+ struct device_node *child, u32 addr)
+{
+ return -ENOSYS;
+}
#endif
--
2.25.1
Document the NXP TJA11xx PHY bindings.
Signed-off-by: Oleksij Rempel <[email protected]>
---
.../devicetree/bindings/net/nxp,tja11xx.yaml | 61 +++++++++++++++++++
1 file changed, 61 insertions(+)
create mode 100644 Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
diff --git a/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
new file mode 100644
index 000000000000..42be0255512b
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
@@ -0,0 +1,61 @@
+# SPDX-License-Identifier: GPL-2.0+
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/nxp,tja11xx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NXP TJA11xx PHY
+
+maintainers:
+ - Andrew Lunn <[email protected]>
+ - Florian Fainelli <[email protected]>
+ - Heiner Kallweit <[email protected]>
+
+description:
+ Bindings for NXP TJA11xx automotive PHYs
+
+allOf:
+ - $ref: ethernet-phy.yaml#
+
+patternProperties:
+ "^ethernet-phy@[0-9a-f]+$":
+ type: object
+ description: |
+ Some packages have multiple PHYs. Secondary PHY should be defines as
+ subnode of the first (parent) PHY.
+
+ properties:
+ reg:
+ minimum: 0
+ maximum: 31
+ description:
+ The ID number for the child PHY. Should be +1 of parent PHY.
+
+ required:
+ - reg
+
+examples:
+ - |
+ mdio {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ tja1101_phy0: ethernet-phy@4 {
+ reg = <0x4>;
+ };
+ };
+ - |
+ mdio {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ tja1102_phy0: ethernet-phy@4 {
+ reg = <0x4>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ tja1102_phy1: ethernet-phy@5 {
+ reg = <0x5>;
+ };
+ };
+ };
--
2.25.1
On 3/12/2020 10:22 PM, Oleksij Rempel wrote:
> Document the NXP TJA11xx PHY bindings.
>
> Signed-off-by: Oleksij Rempel <[email protected]>
> ---
> .../devicetree/bindings/net/nxp,tja11xx.yaml | 61 +++++++++++++++++++
> 1 file changed, 61 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
>
> diff --git a/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> new file mode 100644
> index 000000000000..42be0255512b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> @@ -0,0 +1,61 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/nxp,tja11xx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP TJA11xx PHY
> +
> +maintainers:
> + - Andrew Lunn <[email protected]>
> + - Florian Fainelli <[email protected]>
> + - Heiner Kallweit <[email protected]>
> +
> +description:
> + Bindings for NXP TJA11xx automotive PHYs
> +
> +allOf:
> + - $ref: ethernet-phy.yaml#
> +
> +patternProperties:
> + "^ethernet-phy@[0-9a-f]+$":
> + type: object
> + description: |
> + Some packages have multiple PHYs. Secondary PHY should be defines as
> + subnode of the first (parent) PHY.
There are QSGMII PHYs which have 4 PHYs embedded and AFAICT they are
defined as 4 separate Ethernet PHY nodes and this would not be quite a
big stretch to represent them that way compared to how they are.
I would recommend doing the same thing and not bend the MDIO framework
to support the registration of "nested" Ethernet PHY nodes.
--
Florian
> > diff --git a/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> > new file mode 100644
> > index 000000000000..42be0255512b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> > @@ -0,0 +1,61 @@
> > +# SPDX-License-Identifier: GPL-2.0+
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/nxp,tja11xx.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: NXP TJA11xx PHY
> > +
> > +maintainers:
> > + - Andrew Lunn <[email protected]>
> > + - Florian Fainelli <[email protected]>
> > + - Heiner Kallweit <[email protected]>
> > +
> > +description:
> > + Bindings for NXP TJA11xx automotive PHYs
> > +
> > +allOf:
> > + - $ref: ethernet-phy.yaml#
> > +
> > +patternProperties:
> > + "^ethernet-phy@[0-9a-f]+$":
> > + type: object
> > + description: |
> > + Some packages have multiple PHYs. Secondary PHY should be defines as
> > + subnode of the first (parent) PHY.
>
>
> There are QSGMII PHYs which have 4 PHYs embedded and AFAICT they are
> defined as 4 separate Ethernet PHY nodes and this would not be quite a
> big stretch to represent them that way compared to how they are.
>
> I would recommend doing the same thing and not bend the MDIO framework
> to support the registration of "nested" Ethernet PHY nodes.
Hi Florian
The issue here is the missing PHY ID in the secondary PHY. Because of
that, the secondary does not probe in the normal way. We need the
primary to be involved to some degree. It needs to register it. What
i'm not so clear on is if it just needs to register it, or if these
sub nodes are actually needed, given the current code.
Andrew
On Fri, Mar 13, 2020 at 07:10:56PM +0100, Andrew Lunn wrote:
> > > diff --git a/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> > > new file mode 100644
> > > index 000000000000..42be0255512b
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> > > @@ -0,0 +1,61 @@
> > > +# SPDX-License-Identifier: GPL-2.0+
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/net/nxp,tja11xx.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: NXP TJA11xx PHY
> > > +
> > > +maintainers:
> > > + - Andrew Lunn <[email protected]>
> > > + - Florian Fainelli <[email protected]>
> > > + - Heiner Kallweit <[email protected]>
> > > +
> > > +description:
> > > + Bindings for NXP TJA11xx automotive PHYs
> > > +
> > > +allOf:
> > > + - $ref: ethernet-phy.yaml#
> > > +
> > > +patternProperties:
> > > + "^ethernet-phy@[0-9a-f]+$":
> > > + type: object
> > > + description: |
> > > + Some packages have multiple PHYs. Secondary PHY should be defines as
> > > + subnode of the first (parent) PHY.
> >
> >
> > There are QSGMII PHYs which have 4 PHYs embedded and AFAICT they are
> > defined as 4 separate Ethernet PHY nodes and this would not be quite a
> > big stretch to represent them that way compared to how they are.
> >
> > I would recommend doing the same thing and not bend the MDIO framework
> > to support the registration of "nested" Ethernet PHY nodes.
>
> Hi Florian
>
> The issue here is the missing PHY ID in the secondary PHY. Because of
> that, the secondary does not probe in the normal way. We need the
> primary to be involved to some degree. It needs to register it. What
> i'm not so clear on is if it just needs to register it, or if these
> sub nodes are actually needed, given the current code.
There are a bit more dependencies:
- PHY0 is responsible for health monitoring. If some thing wrong, it may
shut down complete chip.
- We have shared reset. It make no sense to probe PHY1 before PHY0 with
more controlling options will be probed
- It is possible bat dangerous to use PHY1 without PHY0.
Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On 3/13/2020 11:16 AM, Oleksij Rempel wrote:
> On Fri, Mar 13, 2020 at 07:10:56PM +0100, Andrew Lunn wrote:
>>>> diff --git a/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
>>>> new file mode 100644
>>>> index 000000000000..42be0255512b
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
>>>> @@ -0,0 +1,61 @@
>>>> +# SPDX-License-Identifier: GPL-2.0+
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/net/nxp,tja11xx.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: NXP TJA11xx PHY
>>>> +
>>>> +maintainers:
>>>> + - Andrew Lunn <[email protected]>
>>>> + - Florian Fainelli <[email protected]>
>>>> + - Heiner Kallweit <[email protected]>
>>>> +
>>>> +description:
>>>> + Bindings for NXP TJA11xx automotive PHYs
>>>> +
>>>> +allOf:
>>>> + - $ref: ethernet-phy.yaml#
>>>> +
>>>> +patternProperties:
>>>> + "^ethernet-phy@[0-9a-f]+$":
>>>> + type: object
>>>> + description: |
>>>> + Some packages have multiple PHYs. Secondary PHY should be defines as
>>>> + subnode of the first (parent) PHY.
>>>
>>>
>>> There are QSGMII PHYs which have 4 PHYs embedded and AFAICT they are
>>> defined as 4 separate Ethernet PHY nodes and this would not be quite a
>>> big stretch to represent them that way compared to how they are.
>>>
>>> I would recommend doing the same thing and not bend the MDIO framework
>>> to support the registration of "nested" Ethernet PHY nodes.
>>
>> Hi Florian
>>
>> The issue here is the missing PHY ID in the secondary PHY. Because of
>> that, the secondary does not probe in the normal way. We need the
>> primary to be involved to some degree. It needs to register it. What
>> i'm not so clear on is if it just needs to register it, or if these
>> sub nodes are actually needed, given the current code.
>
> There are a bit more dependencies:
> - PHY0 is responsible for health monitoring. If some thing wrong, it may
> shut down complete chip.
> - We have shared reset. It make no sense to probe PHY1 before PHY0 with
> more controlling options will be probed
> - It is possible bat dangerous to use PHY1 without PHY0.
probing is a software problem though. If we want to describe the PHY
package more correctly, we should be using a container node, something
like this maybe:
phy-package {
compatible = "nxp,tja1102";
ethernet-phy@4 {
reg = <4>;
};
ethernet-phy@5 {
reg = <5>;
};
};
--
Florian
On Fri, Mar 13, 2020 at 11:20:35AM -0700, Florian Fainelli wrote:
>
>
> On 3/13/2020 11:16 AM, Oleksij Rempel wrote:
> > On Fri, Mar 13, 2020 at 07:10:56PM +0100, Andrew Lunn wrote:
> >>>> diff --git a/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> >>>> new file mode 100644
> >>>> index 000000000000..42be0255512b
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> >>>> @@ -0,0 +1,61 @@
> >>>> +# SPDX-License-Identifier: GPL-2.0+
> >>>> +%YAML 1.2
> >>>> +---
> >>>> +$id: http://devicetree.org/schemas/net/nxp,tja11xx.yaml#
> >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>> +
> >>>> +title: NXP TJA11xx PHY
> >>>> +
> >>>> +maintainers:
> >>>> + - Andrew Lunn <[email protected]>
> >>>> + - Florian Fainelli <[email protected]>
> >>>> + - Heiner Kallweit <[email protected]>
> >>>> +
> >>>> +description:
> >>>> + Bindings for NXP TJA11xx automotive PHYs
> >>>> +
> >>>> +allOf:
> >>>> + - $ref: ethernet-phy.yaml#
> >>>> +
> >>>> +patternProperties:
> >>>> + "^ethernet-phy@[0-9a-f]+$":
> >>>> + type: object
> >>>> + description: |
> >>>> + Some packages have multiple PHYs. Secondary PHY should be defines as
> >>>> + subnode of the first (parent) PHY.
> >>>
> >>>
> >>> There are QSGMII PHYs which have 4 PHYs embedded and AFAICT they are
> >>> defined as 4 separate Ethernet PHY nodes and this would not be quite a
> >>> big stretch to represent them that way compared to how they are.
> >>>
> >>> I would recommend doing the same thing and not bend the MDIO framework
> >>> to support the registration of "nested" Ethernet PHY nodes.
> >>
> >> Hi Florian
> >>
> >> The issue here is the missing PHY ID in the secondary PHY. Because of
> >> that, the secondary does not probe in the normal way. We need the
> >> primary to be involved to some degree. It needs to register it. What
> >> i'm not so clear on is if it just needs to register it, or if these
> >> sub nodes are actually needed, given the current code.
> >
> > There are a bit more dependencies:
> > - PHY0 is responsible for health monitoring. If some thing wrong, it may
> > shut down complete chip.
> > - We have shared reset. It make no sense to probe PHY1 before PHY0 with
> > more controlling options will be probed
> > - It is possible bat dangerous to use PHY1 without PHY0.
>
> probing is a software problem though. If we want to describe the PHY
> package more correctly, we should be using a container node, something
> like this maybe:
>
> phy-package {
> compatible = "nxp,tja1102";
>
> ethernet-phy@4 {
> reg = <4>;
> };
>
> ethernet-phy@5 {
> reg = <5>;
> };
> };
Yes, this is almost the same as it is currently done:
phy-package {
reg = <4>;
ethernet-phy@5 {
reg = <5>;
};
};
Because the primary PHY0 can be autodetected by the bus scan.
But I have nothing against your suggestions. Please, some one should say the
last word here, how exactly it should be implemented?
Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
From: Oleksij Rempel <[email protected]>
Date: Fri, 13 Mar 2020 06:22:48 +0100
> changes v4:
> - remove unused phy_id variable
>
> changes v3:
> - export part of of_mdiobus_register_phy() and reuse it in tja11xx
> driver
> - coding style fixes
>
> changes v2:
> - use .match_phy_device
> - add irq support
> - add add delayed registration for PHY1
Florian, please properly follow up in the discussion of patch #1 so that they
can implement support properly.
Thank you.
On Fri, Mar 13, 2020 at 07:53:27PM +0100, Oleksij Rempel wrote:
> On Fri, Mar 13, 2020 at 11:20:35AM -0700, Florian Fainelli wrote:
> >
> >
> > On 3/13/2020 11:16 AM, Oleksij Rempel wrote:
> > > On Fri, Mar 13, 2020 at 07:10:56PM +0100, Andrew Lunn wrote:
> > >>>> diff --git a/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> > >>>> new file mode 100644
> > >>>> index 000000000000..42be0255512b
> > >>>> --- /dev/null
> > >>>> +++ b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> > >>>> @@ -0,0 +1,61 @@
> > >>>> +# SPDX-License-Identifier: GPL-2.0+
> > >>>> +%YAML 1.2
> > >>>> +---
> > >>>> +$id: http://devicetree.org/schemas/net/nxp,tja11xx.yaml#
> > >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > >>>> +
> > >>>> +title: NXP TJA11xx PHY
> > >>>> +
> > >>>> +maintainers:
> > >>>> + - Andrew Lunn <[email protected]>
> > >>>> + - Florian Fainelli <[email protected]>
> > >>>> + - Heiner Kallweit <[email protected]>
> > >>>> +
> > >>>> +description:
> > >>>> + Bindings for NXP TJA11xx automotive PHYs
> > >>>> +
> > >>>> +allOf:
> > >>>> + - $ref: ethernet-phy.yaml#
> > >>>> +
> > >>>> +patternProperties:
> > >>>> + "^ethernet-phy@[0-9a-f]+$":
> > >>>> + type: object
> > >>>> + description: |
> > >>>> + Some packages have multiple PHYs. Secondary PHY should be defines as
> > >>>> + subnode of the first (parent) PHY.
> > >>>
> > >>>
> > >>> There are QSGMII PHYs which have 4 PHYs embedded and AFAICT they are
> > >>> defined as 4 separate Ethernet PHY nodes and this would not be quite a
> > >>> big stretch to represent them that way compared to how they are.
> > >>>
> > >>> I would recommend doing the same thing and not bend the MDIO framework
> > >>> to support the registration of "nested" Ethernet PHY nodes.
> > >>
> > >> Hi Florian
> > >>
> > >> The issue here is the missing PHY ID in the secondary PHY. Because of
> > >> that, the secondary does not probe in the normal way. We need the
> > >> primary to be involved to some degree. It needs to register it. What
> > >> i'm not so clear on is if it just needs to register it, or if these
> > >> sub nodes are actually needed, given the current code.
> > >
> > > There are a bit more dependencies:
> > > - PHY0 is responsible for health monitoring. If some thing wrong, it may
> > > shut down complete chip.
> > > - We have shared reset. It make no sense to probe PHY1 before PHY0 with
> > > more controlling options will be probed
> > > - It is possible bat dangerous to use PHY1 without PHY0.
> >
> > probing is a software problem though. If we want to describe the PHY
> > package more correctly, we should be using a container node, something
> > like this maybe:
> >
> > phy-package {
> > compatible = "nxp,tja1102";
> >
> > ethernet-phy@4 {
> > reg = <4>;
> > };
> >
> > ethernet-phy@5 {
> > reg = <5>;
> > };
> > };
>
> Yes, this is almost the same as it is currently done:
>
> phy-package {
> reg = <4>;
>
> ethernet-phy@5 {
> reg = <5>;
> };
> };
>
> Because the primary PHY0 can be autodetected by the bus scan.
> But I have nothing against your suggestions. Please, some one should say the
> last word here, how exactly it should be implemented?
ping,
Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On 3/17/2020 4:56 AM, Oleksij Rempel wrote:
> On Fri, Mar 13, 2020 at 07:53:27PM +0100, Oleksij Rempel wrote:
>> On Fri, Mar 13, 2020 at 11:20:35AM -0700, Florian Fainelli wrote:
>>>
>>>
>>> On 3/13/2020 11:16 AM, Oleksij Rempel wrote:
>>>> On Fri, Mar 13, 2020 at 07:10:56PM +0100, Andrew Lunn wrote:
>>>>>>> diff --git a/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
>>>>>>> new file mode 100644
>>>>>>> index 000000000000..42be0255512b
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
>>>>>>> @@ -0,0 +1,61 @@
>>>>>>> +# SPDX-License-Identifier: GPL-2.0+
>>>>>>> +%YAML 1.2
>>>>>>> +---
>>>>>>> +$id: http://devicetree.org/schemas/net/nxp,tja11xx.yaml#
>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>>> +
>>>>>>> +title: NXP TJA11xx PHY
>>>>>>> +
>>>>>>> +maintainers:
>>>>>>> + - Andrew Lunn <[email protected]>
>>>>>>> + - Florian Fainelli <[email protected]>
>>>>>>> + - Heiner Kallweit <[email protected]>
>>>>>>> +
>>>>>>> +description:
>>>>>>> + Bindings for NXP TJA11xx automotive PHYs
>>>>>>> +
>>>>>>> +allOf:
>>>>>>> + - $ref: ethernet-phy.yaml#
>>>>>>> +
>>>>>>> +patternProperties:
>>>>>>> + "^ethernet-phy@[0-9a-f]+$":
>>>>>>> + type: object
>>>>>>> + description: |
>>>>>>> + Some packages have multiple PHYs. Secondary PHY should be defines as
>>>>>>> + subnode of the first (parent) PHY.
>>>>>>
>>>>>>
>>>>>> There are QSGMII PHYs which have 4 PHYs embedded and AFAICT they are
>>>>>> defined as 4 separate Ethernet PHY nodes and this would not be quite a
>>>>>> big stretch to represent them that way compared to how they are.
>>>>>>
>>>>>> I would recommend doing the same thing and not bend the MDIO framework
>>>>>> to support the registration of "nested" Ethernet PHY nodes.
>>>>>
>>>>> Hi Florian
>>>>>
>>>>> The issue here is the missing PHY ID in the secondary PHY. Because of
>>>>> that, the secondary does not probe in the normal way. We need the
>>>>> primary to be involved to some degree. It needs to register it. What
>>>>> i'm not so clear on is if it just needs to register it, or if these
>>>>> sub nodes are actually needed, given the current code.
>>>>
>>>> There are a bit more dependencies:
>>>> - PHY0 is responsible for health monitoring. If some thing wrong, it may
>>>> shut down complete chip.
>>>> - We have shared reset. It make no sense to probe PHY1 before PHY0 with
>>>> more controlling options will be probed
>>>> - It is possible bat dangerous to use PHY1 without PHY0.
>>>
>>> probing is a software problem though. If we want to describe the PHY
>>> package more correctly, we should be using a container node, something
>>> like this maybe:
>>>
>>> phy-package {
>>> compatible = "nxp,tja1102";
>>>
>>> ethernet-phy@4 {
>>> reg = <4>;
>>> };
>>>
>>> ethernet-phy@5 {
>>> reg = <5>;
>>> };
>>> };
>>
>> Yes, this is almost the same as it is currently done:
>>
>> phy-package {
>> reg = <4>;
>>
>> ethernet-phy@5 {
>> reg = <5>;
>> };
>> };
>>
>> Because the primary PHY0 can be autodetected by the bus scan.
>> But I have nothing against your suggestions. Please, some one should say the
>> last word here, how exactly it should be implemented?
It's not for me to decide, I was hoping the Device Tree maintainers
could chime in, your current approach would certainly work although it
feels visually awkward.
--
Florian
On Tue, Mar 17, 2020 at 12:48:47PM -0700, Florian Fainelli wrote:
>
>
> On 3/17/2020 4:56 AM, Oleksij Rempel wrote:
> > On Fri, Mar 13, 2020 at 07:53:27PM +0100, Oleksij Rempel wrote:
> >> On Fri, Mar 13, 2020 at 11:20:35AM -0700, Florian Fainelli wrote:
> >>>
> >>>
> >>> On 3/13/2020 11:16 AM, Oleksij Rempel wrote:
> >>>> On Fri, Mar 13, 2020 at 07:10:56PM +0100, Andrew Lunn wrote:
> >>>>>>> diff --git a/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> >>>>>>> new file mode 100644
> >>>>>>> index 000000000000..42be0255512b
> >>>>>>> --- /dev/null
> >>>>>>> +++ b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> >>>>>>> @@ -0,0 +1,61 @@
> >>>>>>> +# SPDX-License-Identifier: GPL-2.0+
> >>>>>>> +%YAML 1.2
> >>>>>>> +---
> >>>>>>> +$id: http://devicetree.org/schemas/net/nxp,tja11xx.yaml#
> >>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>>>>> +
> >>>>>>> +title: NXP TJA11xx PHY
> >>>>>>> +
> >>>>>>> +maintainers:
> >>>>>>> + - Andrew Lunn <[email protected]>
> >>>>>>> + - Florian Fainelli <[email protected]>
> >>>>>>> + - Heiner Kallweit <[email protected]>
> >>>>>>> +
> >>>>>>> +description:
> >>>>>>> + Bindings for NXP TJA11xx automotive PHYs
> >>>>>>> +
> >>>>>>> +allOf:
> >>>>>>> + - $ref: ethernet-phy.yaml#
> >>>>>>> +
> >>>>>>> +patternProperties:
> >>>>>>> + "^ethernet-phy@[0-9a-f]+$":
> >>>>>>> + type: object
> >>>>>>> + description: |
> >>>>>>> + Some packages have multiple PHYs. Secondary PHY should be defines as
> >>>>>>> + subnode of the first (parent) PHY.
> >>>>>>
> >>>>>>
> >>>>>> There are QSGMII PHYs which have 4 PHYs embedded and AFAICT they are
> >>>>>> defined as 4 separate Ethernet PHY nodes and this would not be quite a
> >>>>>> big stretch to represent them that way compared to how they are.
> >>>>>>
> >>>>>> I would recommend doing the same thing and not bend the MDIO framework
> >>>>>> to support the registration of "nested" Ethernet PHY nodes.
> >>>>>
> >>>>> Hi Florian
> >>>>>
> >>>>> The issue here is the missing PHY ID in the secondary PHY. Because of
> >>>>> that, the secondary does not probe in the normal way. We need the
> >>>>> primary to be involved to some degree. It needs to register it. What
> >>>>> i'm not so clear on is if it just needs to register it, or if these
> >>>>> sub nodes are actually needed, given the current code.
> >>>>
> >>>> There are a bit more dependencies:
> >>>> - PHY0 is responsible for health monitoring. If some thing wrong, it may
> >>>> shut down complete chip.
> >>>> - We have shared reset. It make no sense to probe PHY1 before PHY0 with
> >>>> more controlling options will be probed
> >>>> - It is possible bat dangerous to use PHY1 without PHY0.
> >>>
> >>> probing is a software problem though. If we want to describe the PHY
> >>> package more correctly, we should be using a container node, something
> >>> like this maybe:
> >>>
> >>> phy-package {
> >>> compatible = "nxp,tja1102";
> >>>
> >>> ethernet-phy@4 {
> >>> reg = <4>;
> >>> };
> >>>
> >>> ethernet-phy@5 {
> >>> reg = <5>;
> >>> };
> >>> };
> >>
> >> Yes, this is almost the same as it is currently done:
> >>
> >> phy-package {
> >> reg = <4>;
> >>
> >> ethernet-phy@5 {
> >> reg = <5>;
> >> };
> >> };
> >>
> >> Because the primary PHY0 can be autodetected by the bus scan.
> >> But I have nothing against your suggestions. Please, some one should say the
> >> last word here, how exactly it should be implemented?
>
> It's not for me to decide, I was hoping the Device Tree maintainers
> could chime in, your current approach would certainly work although it
> feels visually awkward.
Something like this is what I'd do:
ethernet-phy@4 {
compatible = "nxp,tja1102";
reg = <4 5>;
};
Rob
On 3/20/2020 4:05 PM, Rob Herring wrote:
>>>> Because the primary PHY0 can be autodetected by the bus scan.
>>>> But I have nothing against your suggestions. Please, some one should say the
>>>> last word here, how exactly it should be implemented?
>>
>> It's not for me to decide, I was hoping the Device Tree maintainers
>> could chime in, your current approach would certainly work although it
>> feels visually awkward.
>
> Something like this is what I'd do:
>
> ethernet-phy@4 {
> compatible = "nxp,tja1102";
> reg = <4 5>;
> };
But the parent (MDIO bus controller) has #address-cells = 1 and
#size-cells = 0, so how can this be made to work without creating two
nodes or a first node encapsulating another one?
--
Florian
On Sun, Mar 22, 2020 at 3:09 PM Florian Fainelli <[email protected]> wrote:
>
>
>
> On 3/20/2020 4:05 PM, Rob Herring wrote:
> >>>> Because the primary PHY0 can be autodetected by the bus scan.
> >>>> But I have nothing against your suggestions. Please, some one should say the
> >>>> last word here, how exactly it should be implemented?
> >>
> >> It's not for me to decide, I was hoping the Device Tree maintainers
> >> could chime in, your current approach would certainly work although it
> >> feels visually awkward.
> >
> > Something like this is what I'd do:
> >
> > ethernet-phy@4 {
> > compatible = "nxp,tja1102";
> > reg = <4 5>;
> > };
>
> But the parent (MDIO bus controller) has #address-cells = 1 and
> #size-cells = 0, so how can this be made to work without creating two
> nodes or a first node encapsulating another one?
That is the size of the address, not how many addresses there are. If
the device has 2 addresses, then 2 address entries seems entirely
appropriate.
Rob
>On Sun, Mar 22, 2020 at 3:09 PM Florian Fainelli <[email protected]> wrote:
>>
>> On 3/20/2020 4:05 PM, Rob Herring wrote:
>> >>>> Because the primary PHY0 can be autodetected by the bus scan.
>> >>>> But I have nothing against your suggestions. Please, some one should say the
>> >>>> last word here, how exactly it should be implemented?
>> >>
>> >> It's not for me to decide, I was hoping the Device Tree maintainers
>> >> could chime in, your current approach would certainly work although it
>> >> feels visually awkward.
>> >
>> > Something like this is what I'd do:
>> >
>> > ethernet-phy@4 {
>> > compatible = "nxp,tja1102";
>> > reg = <4 5>;
>> > };
>>
>> But the parent (MDIO bus controller) has #address-cells = 1 and
>> #size-cells = 0, so how can this be made to work without creating two
>> nodes or a first node encapsulating another one?
>
>That is the size of the address, not how many addresses there are. If
>the device has 2 addresses, then 2 address entries seems entirely
>appropriate.
>
>Rob
Yes, it is one device with two address. This is if you call the entire IC a device. If you look at it from a PHY perspective, it is two devices with 1 address.
If you just look at it as a single device, it gets difficult to add PHY specific properties in the future, e.g. master/slave selection.
In my opinion its important to have some kind of container for the entire IC, but likewise for the individual PHYs.
Christian
> Yes, it is one device with two address. This is if you call the entire IC a device. If you look at it from a PHY perspective, it is two devices with 1 address.
> If you just look at it as a single device, it gets difficult to add PHY specific properties in the future, e.g. master/slave selection.
> In my opinion its important to have some kind of container for the
> entire IC, but likewise for the individual PHYs.
Yes, we need some sort of representation of two devices.
Logically, the two PHYs are on the same MDIO bus, so you could have
two nodes on the main bus.
Or you consider the secondary PHY as being on an internal MDIO bus
which is transparently bridged to the main bus. This is what was
proposed in the last patchset.
Because this bridge is transparent, the rest of the PHY/MDIO framework
has no idea about it. So i prefer that we keep with two PHY nodes on
the main bus. But i still think we need the master PHY to register the
secondary PHY, due to the missing PHY ID, and the other constrains
like resets which the master PHY has to handle.
Andrew
Hi,
On Mon, Mar 23, 2020 at 04:14:23PM +0100, Andrew Lunn wrote:
> > Yes, it is one device with two address. This is if you call the entire IC a device. If you look at it from a PHY perspective, it is two devices with 1 address.
> > If you just look at it as a single device, it gets difficult to add PHY specific properties in the future, e.g. master/slave selection.
>
> > In my opinion its important to have some kind of container for the
> > entire IC, but likewise for the individual PHYs.
>
> Yes, we need some sort of representation of two devices.
>
> Logically, the two PHYs are on the same MDIO bus, so you could have
> two nodes on the main bus.
>
> Or you consider the secondary PHY as being on an internal MDIO bus
> which is transparently bridged to the main bus. This is what was
> proposed in the last patchset.
>
> Because this bridge is transparent, the rest of the PHY/MDIO framework
> has no idea about it. So i prefer that we keep with two PHY nodes on
> the main bus. But i still think we need the master PHY to register the
> secondary PHY, due to the missing PHY ID, and the other constrains
> like resets which the master PHY has to handle.
this discussion is stalled.
What is the final decision? What is the proper and mainlinable way?
Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Mon, Mar 23, 2020 at 04:14:23PM +0100, Andrew Lunn wrote:
> > Yes, it is one device with two address. This is if you call the entire IC a device. If you look at it from a PHY perspective, it is two devices with 1 address.
> > If you just look at it as a single device, it gets difficult to add PHY specific properties in the future, e.g. master/slave selection.
>
> > In my opinion its important to have some kind of container for the
> > entire IC, but likewise for the individual PHYs.
>
> Yes, we need some sort of representation of two devices.
>
> Logically, the two PHYs are on the same MDIO bus, so you could have
> two nodes on the main bus.
>
> Or you consider the secondary PHY as being on an internal MDIO bus
> which is transparently bridged to the main bus. This is what was
> proposed in the last patchset.
>
> Because this bridge is transparent, the rest of the PHY/MDIO framework
> has no idea about it. So i prefer that we keep with two PHY nodes on
> the main bus. But i still think we need the master PHY to register the
> secondary PHY, due to the missing PHY ID, and the other constrains
> like resets which the master PHY has to handle.
Yes, this is the way how current patches are implemented.
Should dt-binding documentation and PHY changes go via David's tree
upstream? If nobody has strong opinion against it, @David can you
please take them.
Regards,
Oleksij & Marc
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Fri, Mar 13, 2020 at 12:23 AM Oleksij Rempel <[email protected]> wrote:
>
> Document the NXP TJA11xx PHY bindings.
Given the discussion, I'd marked this one as "changes requested"
expecting a new version to review the schema. And gmail decided to
make a new thread due to the extra 'RE:'. So it fell off my radar.
This schema is fundamentally broken as there's no way to match for
when to apply this schema. How do we find a NXP TJA11xx PHY? I suppose
we can look for 'ethernet-phy' with a child node 'ethernet-phy', but
then that would apply to any phy like this one. This needs a
compatible string IMO given it is non-standard.
>
> Signed-off-by: Oleksij Rempel <[email protected]>
> ---
> .../devicetree/bindings/net/nxp,tja11xx.yaml | 61 +++++++++++++++++++
> 1 file changed, 61 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
>
> diff --git a/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> new file mode 100644
> index 000000000000..42be0255512b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> @@ -0,0 +1,61 @@
> +# SPDX-License-Identifier: GPL-2.0+
Dual license new bindings:
(GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/nxp,tja11xx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP TJA11xx PHY
> +
> +maintainers:
> + - Andrew Lunn <[email protected]>
> + - Florian Fainelli <[email protected]>
> + - Heiner Kallweit <[email protected]>
> +
> +description:
> + Bindings for NXP TJA11xx automotive PHYs
Perhaps some information about how this phy is special.
> +
> +allOf:
> + - $ref: ethernet-phy.yaml#
Not needed here as ethernet-phy.yaml already has a 'select' condition to apply.
> +
> +patternProperties:
> + "^ethernet-phy@[0-9a-f]+$":
> + type: object
> + description: |
> + Some packages have multiple PHYs. Secondary PHY should be defines as
> + subnode of the first (parent) PHY.
> +
> + properties:
> + reg:
> + minimum: 0
> + maximum: 31
> + description:
> + The ID number for the child PHY. Should be +1 of parent PHY.
> +
> + required:
> + - reg
> +
> +examples:
> + - |
> + mdio {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + tja1101_phy0: ethernet-phy@4 {
> + reg = <0x4>;
> + };
> + };
> + - |
> + mdio {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + tja1102_phy0: ethernet-phy@4 {
> + reg = <0x4>;
> + #address-cells = <1>;
> + #size-cells = <0>;
These aren't documented.
> +
> + tja1102_phy1: ethernet-phy@5 {
> + reg = <0x5>;
> + };
> + };
> + };
> --
> 2.25.1
>
@Rob, thank you for the review.
@David, should I send fixes or reworked initial patches?
On Tue, Apr 28, 2020 at 12:30:06PM -0500, Rob Herring wrote:
> On Fri, Mar 13, 2020 at 12:23 AM Oleksij Rempel <[email protected]> wrote:
> >
> > Document the NXP TJA11xx PHY bindings.
>
> Given the discussion, I'd marked this one as "changes requested"
> expecting a new version to review the schema. And gmail decided to
> make a new thread due to the extra 'RE:'. So it fell off my radar.
>
> This schema is fundamentally broken as there's no way to match for
> when to apply this schema. How do we find a NXP TJA11xx PHY? I suppose
> we can look for 'ethernet-phy' with a child node 'ethernet-phy', but
> then that would apply to any phy like this one. This needs a
> compatible string IMO given it is non-standard.
>
> >
> > Signed-off-by: Oleksij Rempel <[email protected]>
> > ---
> > .../devicetree/bindings/net/nxp,tja11xx.yaml | 61 +++++++++++++++++++
> > 1 file changed, 61 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> > new file mode 100644
> > index 000000000000..42be0255512b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> > @@ -0,0 +1,61 @@
> > +# SPDX-License-Identifier: GPL-2.0+
>
> Dual license new bindings:
>
> (GPL-2.0-only OR BSD-2-Clause)
>
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/nxp,tja11xx.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: NXP TJA11xx PHY
> > +
> > +maintainers:
> > + - Andrew Lunn <[email protected]>
> > + - Florian Fainelli <[email protected]>
> > + - Heiner Kallweit <[email protected]>
> > +
> > +description:
> > + Bindings for NXP TJA11xx automotive PHYs
>
> Perhaps some information about how this phy is special.
>
> > +
> > +allOf:
> > + - $ref: ethernet-phy.yaml#
>
> Not needed here as ethernet-phy.yaml already has a 'select' condition to apply.
>
> > +
> > +patternProperties:
> > + "^ethernet-phy@[0-9a-f]+$":
> > + type: object
> > + description: |
> > + Some packages have multiple PHYs. Secondary PHY should be defines as
> > + subnode of the first (parent) PHY.
> > +
> > + properties:
> > + reg:
> > + minimum: 0
> > + maximum: 31
> > + description:
> > + The ID number for the child PHY. Should be +1 of parent PHY.
> > +
> > + required:
> > + - reg
> > +
> > +examples:
> > + - |
> > + mdio {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + tja1101_phy0: ethernet-phy@4 {
> > + reg = <0x4>;
> > + };
> > + };
> > + - |
> > + mdio {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + tja1102_phy0: ethernet-phy@4 {
> > + reg = <0x4>;
>
> > + #address-cells = <1>;
> > + #size-cells = <0>;
>
> These aren't documented.
>
> > +
> > + tja1102_phy1: ethernet-phy@5 {
> > + reg = <0x5>;
> > + };
> > + };
> > + };
> > --
> > 2.25.1
> >
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On 4/28/2020 9:38 PM, Oleksij Rempel wrote:
> @Rob, thank you for the review.
>
> @David, should I send fixes or reworked initial patches?
You need to send incremental patches, once David applies the patches,
they are part of the git history for the trees he maintains.
--
Florian