From: Christian Lamparter <chunkeey@gmail•com>
To: Florian Fainelli <f.fainelli@gmail•com>
Cc: Andrew Lunn <andrew@lunn•ch>,
netdev@vger•kernel.org, Vivien Didelot <vivien.didelot@gmail•com>
Subject: Re: [PATCH v1] net: dsa: qca8k: implement DT-based ports <-> phy translation
Date: Wed, 06 Feb 2019 22:57:07 +0100 [thread overview]
Message-ID: <3897570.9hORRSCDvi@debian64> (raw)
In-Reply-To: <699dfb8a-11d4-b27a-e762-ff4fdff1a3d7@gmail.com>
On Tuesday, February 5, 2019 11:29:36 PM CET Florian Fainelli wrote:
> On 2/5/19 2:12 PM, Christian Lamparter wrote:
> > On Tuesday, February 5, 2019 10:29:34 PM CET Andrew Lunn wrote:
> >>> For now, I added the DT binding update to the patch as well.
> >>> But if this is indeed the way to go, it'll get a separate patch.
> >>
> >> Hi Christian
> >>
> >> You need to be careful with the DT binding. You need to keep backwards
> >> compatible with it. An old DT blob needs to keep working. I don't
> >> think this is true with this change.
> >
> > Do you mean because of the
> >
> > - switch0@0 {
> > + switch@10 {
> > compatible = "qca,qca8337";
> > #address-cells = <1>;
> > #size-cells = <0>;
> >
> > - reg = <0>;
> > + reg = <0x10>;
> >
> > change?
> >
> > or because I removed the phy-handles?>
> > The reg = <0x10>; will be necessary regardless. Because this
> > is really a bug in the existing binding example and if it is
> > copied it will prevent the qca8k driver from loading.
> > This is due to a resource conflict, because there will be
> > already a "phy_port1: phy@0" registered at reg = <0>;
> > So this never worked would have worked.
>
> That part is fine, it is the removal of the phy-handle properties that
> is possibly a problem, but in hindsight, I do not believe it will be a
> compatibility issue. Lack of "phy-handle" property within the core DSA
> layer means: utilize the switch's internal MDIO bus (ds->slave_mii_bus)
> instance, which you are not removing, you are just changing how the PHYs
> map to port numbers.
>
Ok, thanks.
I think I'm almost ready for v2. I have fully addressed the compatibility
issue by forking off the qca8k_switch_ops depending on whenever a phy-handle
property on one of the ports was found or not. If there was no phy-handle the
driver adds the slave-bus accessors to the ops which tells DSA to allocate
the slave bus and allows the phys can be enumerated. If the phy-handles are
found the driver will not have the accessors and DSA will not setup a
redundant/fake bus and this prevents the second/double/duplicated discovery
and enumeration of the same PHYs again.
Cheers,
Christian
Attached is a preview:
---
commit 96bc70b4d6e290c450b9af728d9ca0f6db877f13
Author: Christian Lamparter <chunkeey@gmail•com>
Date: Fri Feb 1 22:54:32 2019 +0100
net: dsa: qca8k: extend slave-bus implementations
This patch implements accessors for the QCA8337 MDIO access
through the MDIO_MASTER register, which makes it possible to
access the PHYs on slave-bus through the switch. In cases
where the switch ports are already mapped via external
"phy-phandles", the internal mdio-bus is disabled in order to
prevent a duplicated discovery and enumeration of the same
PHYs.
Signed-off-by: Christian Lamparter <chunkeey@gmail•com>
---
Changes from v2:
- Make it compatible with existing configurations
Changes from v1:
- drop DT port <-> phy mapping
- added register definitions for the MDIO control register
- implemented new slave-mdio bus accessors
- DT-binding: fix switch's PSEUDO_PHY address. It's 0x10 not 0.
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index a4b6cda38016..2f1b4b0a3507 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -613,19 +613,61 @@ qca8k_adjust_link(struct dsa_switch *ds, int port, struct phy_device *phy)
}
static int
-qca8k_phy_read(struct dsa_switch *ds, int phy, int regnum)
+qca8k_port_to_phy(int port)
+{
+ if (port < 1 || port > QCA8K_MDIO_MASTER_MAX_PORTS)
+ return -EINVAL;
+
+ return port - 1;
+}
+
+static int
+qca8k_phy_write(struct dsa_switch *ds, int port, int regnum, u16 data)
{
struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
+ u32 val, phy;
+
+ phy = qca8k_port_to_phy(port);
+ if (phy < 0 || (regnum < 0 || regnum >= QCA8K_MDIO_MASTER_MAX_REG))
+ return -EINVAL;
+
+ val = QCA8K_MDIO_MASTER_BUSY | QCA8K_MDIO_MASTER_EN |
+ QCA8K_MDIO_MASTER_WRITE | QCA8K_MDIO_MASTER_PHY_ADDR(phy) |
+ QCA8K_MDIO_MASTER_REG_ADDR(regnum) |
+ QCA8K_MDIO_MASTER_DATA(data);
- return mdiobus_read(priv->bus, phy, regnum);
+ qca8k_write(priv, QCA8K_MDIO_MASTER_CTRL, val);
+
+ return qca8k_busy_wait(priv, QCA8K_MDIO_MASTER_CTRL,
+ QCA8K_MDIO_MASTER_BUSY);
}
+
static int
-qca8k_phy_write(struct dsa_switch *ds, int phy, int regnum, u16 val)
+qca8k_phy_read(struct dsa_switch *ds, int port, int regnum)
{
struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
+ u32 val, phy;
+
+ phy = qca8k_port_to_phy(port);
+ if (phy < 0 || (regnum < 0 || regnum >= QCA8K_MDIO_MASTER_MAX_REG))
+ return 0xffff;
+
+ val = QCA8K_MDIO_MASTER_BUSY | QCA8K_MDIO_MASTER_EN |
+ QCA8K_MDIO_MASTER_READ | QCA8K_MDIO_MASTER_PHY_ADDR(phy) |
+ QCA8K_MDIO_MASTER_REG_ADDR(regnum);
+
+ qca8k_write(priv, QCA8K_MDIO_MASTER_CTRL, val);
- return mdiobus_write(priv->bus, phy, regnum, val);
+ if (qca8k_busy_wait(priv, QCA8K_MDIO_MASTER_CTRL,
+ QCA8K_MDIO_MASTER_BUSY)) {
+ return 0xffff;
+ }
+
+ val = (qca8k_read(priv, QCA8K_MDIO_MASTER_CTRL) &
+ QCA8K_MDIO_MASTER_DATA_MASK);
+
+ return val;
}
static void
@@ -868,8 +910,6 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
.setup = qca8k_setup,
.adjust_link = qca8k_adjust_link,
.get_strings = qca8k_get_strings,
- .phy_read = qca8k_phy_read,
- .phy_write = qca8k_phy_write,
.get_ethtool_stats = qca8k_get_ethtool_stats,
.get_sset_count = qca8k_get_sset_count,
.get_mac_eee = qca8k_get_mac_eee,
@@ -884,6 +924,38 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
.port_fdb_dump = qca8k_port_fdb_dump,
};
+/* Special code to detect DeviceTrees that use the phy-handle
+ * to map external PHYs to the ports. Only if no phy-handle is
+ * detected the slave bus accessors are getting enabled.
+ */
+static int qca8k_detect_slave_bus(struct qca8k_priv *priv)
+{
+ struct device_node *ports, *port;
+ bool found = false;
+
+ ports = of_get_child_by_name(priv->dev->of_node, "ports");
+ if (!ports) {
+ dev_err(priv->dev, "no ports child node found.\n");
+ return -EINVAL;
+ }
+
+ for_each_available_child_of_node(ports, port) {
+ if (of_property_read_bool(port, "phy-handle")) {
+ found = true;
+ break;
+ }
+ }
+
+ if (found) {
+ dev_info(priv->dev, "uses external mdio-bus.\n");
+ } else {
+ priv->ops.phy_read = qca8k_phy_read;
+ priv->ops.phy_write = qca8k_phy_write;
+ }
+
+ return 0;
+}
+
static int
qca8k_sw_probe(struct mdio_device *mdiodev)
{
@@ -912,7 +984,12 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
return -ENOMEM;
priv->ds->priv = priv;
- priv->ds->ops = &qca8k_switch_ops;
+ priv->ops = qca8k_switch_ops;
+ if (qca8k_detect_slave_bus(priv))
+ return -EINVAL;
+
+ priv->ds->ops = &priv->ops;
+
mutex_init(&priv->reg_mutex);
dev_set_drvdata(&mdiodev->dev, priv);
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 613fe5c50236..38d06661f0a8 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -48,6 +48,18 @@
#define QCA8K_MIB_FLUSH BIT(24)
#define QCA8K_MIB_CPU_KEEP BIT(20)
#define QCA8K_MIB_BUSY BIT(17)
+#define QCA8K_MDIO_MASTER_CTRL 0x3c
+#define QCA8K_MDIO_MASTER_BUSY BIT(31)
+#define QCA8K_MDIO_MASTER_EN BIT(30)
+#define QCA8K_MDIO_MASTER_READ BIT(27)
+#define QCA8K_MDIO_MASTER_WRITE 0
+#define QCA8K_MDIO_MASTER_SUP_PRE BIT(26)
+#define QCA8K_MDIO_MASTER_PHY_ADDR(x) ((x) << 21)
+#define QCA8K_MDIO_MASTER_REG_ADDR(x) ((x) << 16)
+#define QCA8K_MDIO_MASTER_DATA(x) (x)
+#define QCA8K_MDIO_MASTER_DATA_MASK GENMASK(15, 0)
+#define QCA8K_MDIO_MASTER_MAX_PORTS 5
+#define QCA8K_MDIO_MASTER_MAX_REG 32
#define QCA8K_GOL_MAC_ADDR0 0x60
#define QCA8K_GOL_MAC_ADDR1 0x64
#define QCA8K_REG_PORT_STATUS(_i) (0x07c + (_i) * 4)
@@ -168,6 +180,7 @@ struct qca8k_priv {
struct dsa_switch *ds;
struct mutex reg_mutex;
struct device *dev;
+ struct dsa_switch_ops ops;
};
struct qca8k_mib_desc {
next prev parent reply other threads:[~2019-02-06 21:57 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-04 21:35 [PATCH v1] net: dsa: qca8k: implement DT-based ports <-> phy translation Christian Lamparter
2019-02-04 22:11 ` Florian Fainelli
2019-02-04 22:26 ` Andrew Lunn
2019-02-04 23:55 ` Christian Lamparter
2019-02-05 2:45 ` Andrew Lunn
2019-02-05 12:48 ` Christian Lamparter
2019-02-05 13:09 ` Andrew Lunn
2019-02-05 21:08 ` Christian Lamparter
2019-02-05 21:29 ` Andrew Lunn
2019-02-05 22:12 ` Christian Lamparter
2019-02-05 22:29 ` Florian Fainelli
2019-02-06 21:57 ` Christian Lamparter [this message]
2019-02-06 22:29 ` Florian Fainelli
2019-02-06 22:32 ` Florian Fainelli
2019-02-07 0:43 ` Christian Lamparter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3897570.9hORRSCDvi@debian64 \
--to=chunkeey@gmail$(echo .)com \
--cc=andrew@lunn$(echo .)ch \
--cc=f.fainelli@gmail$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=vivien.didelot@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox