The main goal of this series is to extend the DW XPCS device support in
the kernel. Particularly the patchset adds the DW XPCS MCI/APB3 management
interface support to the MDIO subsystem (basically it's a way to access
the memory-mapped DW XPCS devices), modifies the DW XPCS PCS-driver to
being registered as a standard MDIO-device driver and finally extends the
STMMAC MDIO sub-module functionality so one would be able to support both
SMA- and MI-based ways to communicate with the XPCS-device. The later way
will implies to have an XPCS-device passed to the DW MAC node via the
generic "pcs-handle" property.
The series traditionally starts with the cleanup patches, which can be
also considered as preparations. First redundant sentinel entry and the
header files are removed. Then two errno cleanups are introduced: return
EINVAL from the soft-reset method and make sure that the validate method
returns EINVAL straight away if the requested interface isn't supported by
the XPCS device instance.
Afterwards three preparation patches are introduced. First one just moves
the generic DW XPCS IDs macro-based declarations from the internal header
file to the external one where some other IDs also reside. Second patch
updates the XPCS registration procedure to avoid dummy MDIO-device
creation if it is already available in the framework of the specified
MDIO-bus. Finally third patch splits up the xpcs_create() method to a set
of the coherent sub-function for the sake of the further easier updates
and to have it looking less complicated.
The next three patches add the DW XPCS Management Interface driver to the
MDIO subsystem, update the DW XPCS driver to support a PCS-device
registered on a MDIO-bus and add the DT-bindings for both of these
objects. Note the hierarchical design (having a DW XPCS device defined as
a sub-node of the DW XPCS management interface node) has been chosen for
several reasons. First Synopsys calls the MCI/APB3 interface as Management
Interface with two possible ways of the MMD CSRs access (direct and
indirect), which is basically a bus between the system and the PCS-device
with a possibility to have more than just one device attached. So the
chosen design looks more correct from the HW representation point of view.
Second the drivers for the SMA/MDIO-capable controllers (STMMAC and it's
glue layers for example) will still be able to manually register an
MDIO-bus but from now with custom XPCS-device identifiers. So the DW XPCS
driver will get to be attached to that device activating respective
internal functionality (see patches 10, 13 and 16). Thus if there is no
way to auto-identify the XPCS device capabilities it can be done based on
the custom device IDs or DT-node compatible string (see patch 10). But if
it's, then the device IDs will be auto-detected by the MDIO-subsystem and
the DW XPCS driver will still get attached to the device on the MDIO-bus.
All of that AFAICS would have been impossible (or much harder) to
implement should the Management Interface MDIO-bus be just internally
created in the DW XPCS driver.
Afterwards two patches add the "pcs-handle" DT-property support to the DW
XPCS driver. The first one of them is a preparation patch, which just
converts the name of the currently available XPCS device registration
method to having "_byaddr" suffix. It better identifies the method and
will make it more distinguishable from the method being added in the next
patch. The second patch introduces a new function which responsibility is
to create the DW XPCS descriptor based on the "pcs-handle" property and
PCS-PHY interface. The prototype is specifically selected to be looking
similar to another XPCS device registration method and to the phylink
fwnode-based connect interface function.
Finally after two small preparations the STMMAC driver is finally updated
to support the DW XPCS devices specified via the "pcs-handle" firmware
node. Thus the STMMAC driver from now will be able to handle the cases
with the DW XPCS devices accessible over the memory-based management
interface or over the external MDIO-buses.
That's it for now. Thanks for review in advance. Any tests are very
welcome. After this series is merged in, I'll submit another one which
adds the generic 10GBase-R and 10GBase-X interfaces support to the STMMAC
and DW XPCS driver with proper CSRs re-initialization, PMA initialization
and reference clock selection as it's defined by the Synopsys DW XPCS HW
manual.
Signed-off-by: Serge Semin <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Eric Dumazet <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: Paolo Abeni <[email protected]>
Cc: Alexandre Torgue <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Serge Semin (16):
net: pcs: xpcs: Drop sentinel entry from 2500basex ifaces list
net: pcs: xpcs: Drop redundant workqueue.h include directive
net: pcs: xpcs: Return EINVAL in the internal methods
net: pcs: xpcs: Explicitly return error on caps validation
net: pcs: xpcs: Move native device ID macro to linux/pcs/pcs-xpcs.h
net: pcs: xpcs: Avoid creating dummy XPCS MDIO device
net: pcs: xpcs: Split up xpcs_create() content to sub-functions
dt-bindings: net: Add Synopsys DW xPCS bindings
net: mdio: Add Synopsys DW XPCS management interface support
net: pcs: xpcs: Add generic DW XPCS MDIO-device support
net: pcs: xpcs: Change xpcs_create_mdiodev() suffix to "byaddr"
net: pcs: xpcs: Add xpcs_create_bynode() method
net: stmmac: intel: Register generic MDIO device
net: stmmac: Pass netdev to XPCS setup function
net: stmmac: Add dedicated XPCS cleanup method
net: stmmac: Add externally detected DW XPCS support
.../bindings/net/pcs/snps,dw-xpcs.yaml | 88 ++++
.../bindings/net/snps,dw-xpcs-mi.yaml | 88 ++++
drivers/net/dsa/sja1105/sja1105_mdio.c | 2 +-
.../net/ethernet/stmicro/stmmac/dwmac-intel.c | 31 +-
drivers/net/ethernet/stmicro/stmmac/stmmac.h | 3 +-
.../net/ethernet/stmicro/stmmac/stmmac_main.c | 14 +-
.../net/ethernet/stmicro/stmmac/stmmac_mdio.c | 51 ++-
drivers/net/mdio/Kconfig | 8 +
drivers/net/mdio/Makefile | 1 +
drivers/net/mdio/mdio-dw-xpcs.c | 384 ++++++++++++++++++
drivers/net/pcs/Kconfig | 6 +-
drivers/net/pcs/pcs-xpcs.c | 285 ++++++++++---
drivers/net/pcs/pcs-xpcs.h | 7 +-
include/linux/pcs/pcs-xpcs.h | 36 +-
14 files changed, 908 insertions(+), 96 deletions(-)
create mode 100644 Documentation/devicetree/bindings/net/pcs/snps,dw-xpcs.yaml
create mode 100644 Documentation/devicetree/bindings/net/snps,dw-xpcs-mi.yaml
create mode 100644 drivers/net/mdio/mdio-dw-xpcs.c
--
2.42.1
There is nothing CM workqueue-related in the driver. So the respective
include directive can be dropped.
While at it add an empty line delimiter between the generic and local path
include directives.
Signed-off-by: Serge Semin <[email protected]>
---
drivers/net/pcs/pcs-xpcs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index dc7c374da495..7f8c63922a4b 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -10,7 +10,7 @@
#include <linux/pcs/pcs-xpcs.h>
#include <linux/mdio.h>
#include <linux/phylink.h>
-#include <linux/workqueue.h>
+
#include "pcs-xpcs.h"
#define phylink_pcs_to_xpcs(pl_pcs) \
--
2.42.1
If an unsupported interface is requested to be validated then there is no
need in further capabilities and-ing since the local array will be left
initialized with zeros. Let's explicitly return EINVAL error in that case
in order to inform the caller about invalid link-state interface. In any
case the phylink_validate_mac_and_pcs() would terminate with error further
link-state validation so the suggested update won't change the validation
procedure semantics.
Signed-off-by: Serge Semin <[email protected]>
---
drivers/net/pcs/pcs-xpcs.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 92c47da61db4..46afeb5510c0 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -613,14 +613,15 @@ static int xpcs_validate(struct phylink_pcs *pcs, unsigned long *supported,
xpcs = phylink_pcs_to_xpcs(pcs);
compat = xpcs_find_compat(xpcs->id, state->interface);
+ if (!compat)
+ return -EINVAL;
/* Populate the supported link modes for this PHY interface type.
* FIXME: what about the port modes and autoneg bit? This masks
* all those away.
*/
- if (compat)
- for (i = 0; compat->supported[i] != __ETHTOOL_LINK_MODE_MASK_NBITS; i++)
- set_bit(compat->supported[i], xpcs_supported);
+ for (i = 0; compat->supported[i] != __ETHTOOL_LINK_MODE_MASK_NBITS; i++)
+ set_bit(compat->supported[i], xpcs_supported);
linkmode_and(supported, supported, xpcs_supported);
--
2.42.1
Synopsys DesignWare XPCS IP-core is a Physical Coding Sublayer (PCS) layer
providing an interface between the Media Access Control (MAC) and Physical
Medium Attachment Sublayer (PMA) through a Media independent interface.
From software point of view it exposes IEEE std. Clause 45 CSR space and
can be accessible either by MDIO or MCI/APB3 bus interfaces. The later
case is described by means of a dedicated DT-bindings which imply having
the DW XPCS Management Interface defined as a DT-supernode which child the
PCSs nodes would be (in the same way as the standard MDIO buses and
devices are normally defined).
Besides of that DW XPCS DT-nodes can have the interrupts and clock source
properties specified. The former one indicates the Clause 73/37
auto-negotiation events like: negotiation page received, AN is completed
or incompatible link partner. The clock DT-properties can describe up to
two clock sources: internal one and the one connected to the chip pad.
Either of them is supposed to be used as the device reference clocks.
Finally the DW XPCS IP-core can be optionally synthesized with a
vendor-specific interface connected to Synopsys PMA (also called
DesignWare Consumer/Enterprise PHY). Alas that isn't auto-detectable
anyhow so if the DW XPCS device has the respective PMA attached then it
should be reflected in the DT-node compatible string so the driver would
be aware of the PMA-specific device capabilities (mainly connected with
CSRs available for the fine-tunings).
Signed-off-by: Serge Semin <[email protected]>
---
.../bindings/net/pcs/snps,dw-xpcs.yaml | 88 +++++++++++++++++++
.../bindings/net/snps,dw-xpcs-mi.yaml | 88 +++++++++++++++++++
2 files changed, 176 insertions(+)
create mode 100644 Documentation/devicetree/bindings/net/pcs/snps,dw-xpcs.yaml
create mode 100644 Documentation/devicetree/bindings/net/snps,dw-xpcs-mi.yaml
diff --git a/Documentation/devicetree/bindings/net/pcs/snps,dw-xpcs.yaml b/Documentation/devicetree/bindings/net/pcs/snps,dw-xpcs.yaml
new file mode 100644
index 000000000000..9694ef51abad
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/pcs/snps,dw-xpcs.yaml
@@ -0,0 +1,88 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/pcs/snps,dw-xpcs.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Synopsys DesignWare Ethernet PCS
+
+maintainers:
+ - Jose Abreu <[email protected]>
+
+description:
+ Synopsys DesignWare Ethernet Physical Coding Sublayer provides an interface
+ between Media Access Control and Physical Medium Attachment Sublayer through
+ the Media Independent Interface (XGMII, USXGMII, XLGMII, GMII, etc)
+ controlled by means of the IEEE std. Clause 45 registers set. The PCS can be
+ optionally synthesized with a vendor-specific interface connected to
+ Synopsys PMA (also called DesignWare Consumer/Enterprise PHY) although in
+ general it can be used to communicate with any compatible PHY.
+
+properties:
+ compatible:
+ oneOf:
+ - description: Synopsys DesignWare XPCS with none or unknown PMA
+ const: snps,dw-xpcs
+ - description: Synopsys DesignWare XPCS with Consumer Gen1 3G PMA
+ const: snps,dw-xpcs-gen1-3g
+ - description: Synopsys DesignWare XPCS with Consumer Gen2 3G PMA
+ const: snps,dw-xpcs-gen2-3g
+ - description: Synopsys DesignWare XPCS with Consumer Gen2 6G PMA
+ const: snps,dw-xpcs-gen2-6g
+ - description: Synopsys DesignWare XPCS with Consumer Gen4 3G PMA
+ const: snps,dw-xpcs-gen4-3g
+ - description: Synopsys DesignWare XPCS with Consumer Gen4 6G PMA
+ const: snps,dw-xpcs-gen4-6g
+ - description: Synopsys DesignWare XPCS with Consumer Gen5 10G PMA
+ const: snps,dw-xpcs-gen5-10g
+ - description: Synopsys DesignWare XPCS with Consumer Gen5 12G PMA
+ const: snps,dw-xpcs-gen5-12g
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ description:
+ System interface interrupt output (sbd_intr_o) indicating Clause 73/37
+ auto-negotiation events like':' Page received, AN is completed or
+ incompatible link partner.
+ maxItems: 1
+
+ clocks:
+ description:
+ PCS/PMA interface be can clocked either by internal reference clock
+ source or by an externally connected (via a pad) clock generator.
+ minItems: 1
+ maxItems: 2
+
+ clock-names:
+ minItems: 1
+ maxItems: 2
+ items:
+ enum: [ core, pad ]
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ mdio-bus {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ xgmac_pcs: ethernet-pcs@0 {
+ compatible = "snps,dw-xpcs";
+ reg = <0>;
+
+ interrupts = <79 IRQ_TYPE_LEVEL_HIGH>;
+
+ clocks = <&ccu_core>, <&ccu_pad>;
+ clock-names = "core", "pad";
+ };
+ };
+...
diff --git a/Documentation/devicetree/bindings/net/snps,dw-xpcs-mi.yaml b/Documentation/devicetree/bindings/net/snps,dw-xpcs-mi.yaml
new file mode 100644
index 000000000000..67ddba9d61fd
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/snps,dw-xpcs-mi.yaml
@@ -0,0 +1,88 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/snps,dw-xpcs-mi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Synopsys DesignWare Ethernet PCS Management Interface
+
+maintainers:
+ - Serge Semin <[email protected]>
+
+description:
+ Synopsys DesignWare Ethernet PCS provides an interface between MAC and PMA
+ through the Media Independent Interface. The PCS CSRs can be accessible over
+ the Ethernet MDIO bus or directly by means of the APB3/MCI interfaces. In the
+ later case the XPCS can be mapped right to the system IO memory space.
+
+allOf:
+ - $ref: mdio.yaml#
+
+properties:
+ compatible:
+ const: snps,dw-xpcs-mi
+
+ reg:
+ items:
+ - description:
+ DW XPCS CSRs space can be either 'directly' or 'indirectly'
+ accessible. In the former case all Clause 45 registers are
+ contiguously mapped within the address space MMD '[20:16]',
+ Reg '[15:0]'. In the later case the space is divided to the
+ multiple 256 register sets. There is a special viewport CSR
+ which is responsible for the set selection. The upper part of
+ the CSR address is supposed to be written in there thus the
+ corresponding subset would be mapped over the lowest 255 CSRs.
+
+ reg-names:
+ items:
+ - enum: [ direct, indirect ]
+
+ reg-io-width:
+ description:
+ The way the CSRs are mapped to the memory is platform depended. Since
+ each Clause 45 CSR is of 16-bits wide the access instructions must be
+ two bytes aligned at least.
+ default: 2
+ enum: [ 2, 4 ]
+
+ clocks:
+ items:
+ - description: Peripheral MCI/APB3 bus clock source
+
+ clock-names:
+ items:
+ - const: pclk
+
+patternProperties:
+ 'ethernet-pcs@[0-9a-f]+$':
+ type: object
+
+ $ref: pcs/snps,dw-xpcs.yaml#
+
+required:
+ - compatible
+ - reg
+ - reg-names
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ mdio@1f05d000 {
+ compatible = "snps,dw-xpcs-mi";
+ reg = <0x1f05d000 0x1000>;
+ reg-names = "indirect";
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ clocks = <&ccu_pclk>;
+ clock-names = "pclk";
+
+ reg-io-width = <4>;
+
+ ethernet-pcs@0 {
+ compatible = "snps,dw-xpcs";
+ reg = <0>;
+ };
+ };
--
2.42.1
As a final preparation before adding the DW XPCS MDIO-device driver
support let's split the xpcs_create() function code up to a set of small
sub-functions. Thus the method implementation will get to look simpler and
turn to be more coherent. Further updates will just touch the new methods
a bit: add platform-specific device info, add device reference clock
getting and enabling.
This update implies the xpcs_create() to call the next static methods:
xpcs_create_data() - create the DW XPCS device descriptor,
xpcs_init_id() - find XPCS ID instance and save it in the device
descriptor,
xpcs_init_iface() - find MAC/PCS interface descriptor and perform
basice initialization specific to it: soft-reset, disable polling.
The update doesn't cause any semantic change but merely makes the code
better looking and ready for adding new features support.
Note in addition to that the xpcs_destroy() is moved to be below the
xpcs_create_mdiodev() function as the driver now implies having the
protagonist-then-antagonist functions definition order.
Signed-off-by: Serge Semin <[email protected]>
---
drivers/net/pcs/pcs-xpcs.c | 102 +++++++++++++++++++++++++------------
1 file changed, 70 insertions(+), 32 deletions(-)
diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index a53376472394..ea6f56339595 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -1365,70 +1365,97 @@ static const struct phylink_pcs_ops xpcs_phylink_ops = {
.pcs_link_up = xpcs_link_up,
};
-static struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
- phy_interface_t interface)
+static struct dw_xpcs *xpcs_create_data(struct mdio_device *mdiodev)
{
struct dw_xpcs *xpcs;
- u32 xpcs_id;
- int i, ret;
xpcs = kzalloc(sizeof(*xpcs), GFP_KERNEL);
if (!xpcs)
return ERR_PTR(-ENOMEM);
xpcs->mdiodev = mdiodev;
+ xpcs->pcs.ops = &xpcs_phylink_ops;
+ xpcs->pcs.neg_mode = true;
+ xpcs->pcs.poll = true;
+
+ return xpcs;
+}
+
+static void xpcs_free_data(struct dw_xpcs *xpcs)
+{
+ kfree(xpcs);
+}
+
+static int xpcs_init_id(struct dw_xpcs *xpcs)
+{
+ u32 xpcs_id;
+ int i, ret;
xpcs_id = xpcs_get_id(xpcs);
for (i = 0; i < ARRAY_SIZE(xpcs_id_list); i++) {
const struct xpcs_id *entry = &xpcs_id_list[i];
- const struct xpcs_compat *compat;
if ((xpcs_id & entry->mask) != entry->id)
continue;
xpcs->id = entry;
- compat = xpcs_find_compat(entry, interface);
- if (!compat) {
- ret = -ENODEV;
- goto out;
- }
+ break;
+ }
- ret = xpcs_dev_flag(xpcs);
- if (ret)
- goto out;
+ if (!xpcs->id)
+ return -ENODEV;
- xpcs->pcs.ops = &xpcs_phylink_ops;
- xpcs->pcs.neg_mode = true;
+ ret = xpcs_dev_flag(xpcs);
+ if (ret < 0)
+ return ret;
- if (xpcs->dev_flag != DW_DEV_TXGBE) {
- xpcs->pcs.poll = true;
+ return 0;
+}
- ret = xpcs_soft_reset(xpcs, compat);
- if (ret)
- goto out;
- }
+static int xpcs_init_iface(struct dw_xpcs *xpcs, phy_interface_t interface)
+{
+ const struct xpcs_compat *compat;
- return xpcs;
+ compat = xpcs_find_compat(xpcs->id, interface);
+ if (!compat)
+ return -EINVAL;
+
+ if (xpcs->dev_flag == DW_DEV_TXGBE) {
+ xpcs->pcs.poll = false;
+ return 0;
}
- ret = -ENODEV;
+ return xpcs_soft_reset(xpcs, compat);
+}
+
+static struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
+ phy_interface_t interface)
+{
+ struct dw_xpcs *xpcs;
+ int ret;
+
+ xpcs = xpcs_create_data(mdiodev);
+ if (IS_ERR(xpcs))
+ return xpcs;
+
+ ret = xpcs_init_id(xpcs);
+ if (ret)
+ goto out;
+
+ ret = xpcs_init_iface(xpcs, interface);
+ if (ret)
+ goto out;
+
+ return xpcs;
out:
- kfree(xpcs);
+ xpcs_free_data(xpcs);
return ERR_PTR(ret);
}
-void xpcs_destroy(struct dw_xpcs *xpcs)
-{
- if (xpcs)
- mdio_device_put(xpcs->mdiodev);
- kfree(xpcs);
-}
-EXPORT_SYMBOL_GPL(xpcs_destroy);
-
struct dw_xpcs *xpcs_create_mdiodev(struct mii_bus *bus, int addr,
phy_interface_t interface)
{
@@ -1455,4 +1482,15 @@ struct dw_xpcs *xpcs_create_mdiodev(struct mii_bus *bus, int addr,
}
EXPORT_SYMBOL_GPL(xpcs_create_mdiodev);
+void xpcs_destroy(struct dw_xpcs *xpcs)
+{
+ if (!xpcs)
+ return;
+
+ mdio_device_put(xpcs->mdiodev);
+
+ xpcs_free_data(xpcs);
+}
+EXPORT_SYMBOL_GPL(xpcs_destroy);
+
MODULE_LICENSE("GPL v2");
--
2.42.1
Recently the memory mapped Synopsys DW XPCS management interface support
was added to the kernel. In that case the DW XPCS device can be registered
via a standard MDIO-bus subsystem. In order to have such devices fully
accessible and properly configured let's add a respective functionality to
the DW XPCS driver.
The main goal of this update is to add a functionality to activate
vendor-specific XPCS capabilities in the driver (like a limited number of
network interfaces, linkmodes, PMA-specific initializations). It's reached
by having DW XPCS devices registered as the platform devices (OF, ACPI,
legacy platform ,etc). From that point of view the suggested update is
threefold. First the driver is now capable to be attached to the
OF-devices registered on the MDIO-bus with the "snps,dw-xpcs*" compatible
string. Second it's possible to have the driver bound to the DW XPCS
device with no OF/ACPI-nodes by means of defining the mdio_board_info
descriptor and registering one with mdiobus_register_board_info() (see
dwmac-intel.c for example). Thirdly it's still possible to use the
unregistered device to auto-detect the DW XPCS device on the MDIO bus. In
all these cases the DW XPCS device info can be passed by means of the
driver-data pointer (of_device_id.data or device.platform_data).
In addition to that the update provides the DW XPCS reference clock
sources request and enabling. These clocks are named as "core" and "pad"
as per the DT-bindings. Note normally they are mutually exclusive: only
one of them can be used at a time, but the system software is responsible
for switching between them. Such functionality will be added later in the
framework of the pma_config() internal callback.
Note all the platform resources initialization is performed in the
externally called xpcs_create() method as before this update. The only
crucial update is that it now makes sure that the device is bound to the
DW XPCS driver if it's possible. Otherwise the legacy auto-detection
procedure takes place as before. Moreover due to that semantic there is no
device probe() and remove() methods defined since there is nothing left to
initialize/de-initialize on these stages.
Signed-off-by: Serge Semin <[email protected]>
---
drivers/net/pcs/Kconfig | 6 +-
drivers/net/pcs/pcs-xpcs.c | 112 ++++++++++++++++++++++++++++++++---
drivers/net/pcs/pcs-xpcs.h | 6 ++
include/linux/pcs/pcs-xpcs.h | 27 +++++++++
4 files changed, 141 insertions(+), 10 deletions(-)
diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
index 87cf308fc6d8..f6aa437473de 100644
--- a/drivers/net/pcs/Kconfig
+++ b/drivers/net/pcs/Kconfig
@@ -6,11 +6,11 @@
menu "PCS device drivers"
config PCS_XPCS
- tristate
+ tristate "Synopsys DesignWare Ethernet XPCS"
select PHYLINK
help
- This module provides helper functions for Synopsys DesignWare XPCS
- controllers.
+ This module provides a driver and helper functions for Synopsys
+ DesignWare XPCS controllers.
config PCS_LYNX
tristate
diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index ea6f56339595..183a37929b60 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -6,10 +6,14 @@
* Author: Jose Abreu <[email protected]>
*/
+#include <linux/clk.h>
#include <linux/delay.h>
-#include <linux/pcs/pcs-xpcs.h>
+#include <linux/device.h>
#include <linux/mdio.h>
+#include <linux/module.h>
+#include <linux/pcs/pcs-xpcs.h>
#include <linux/phylink.h>
+#include <linux/property.h>
#include "pcs-xpcs.h"
@@ -1386,17 +1390,57 @@ static void xpcs_free_data(struct dw_xpcs *xpcs)
kfree(xpcs);
}
+static int xpcs_init_clks(struct dw_xpcs *xpcs)
+{
+ static const char *ids[DW_XPCS_NUM_CLKS] = {
+ [DW_XPCS_CLK_CORE] = "core",
+ [DW_XPCS_CLK_PAD] = "pad",
+ };
+ struct device *dev = &xpcs->mdiodev->dev;
+ int ret, i;
+
+ for (i = 0; i < DW_XPCS_NUM_CLKS; ++i)
+ xpcs->clks[i].id = ids[i];
+
+ ret = clk_bulk_get_optional(dev, DW_XPCS_NUM_CLKS, xpcs->clks);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to get clocks\n");
+
+ ret = clk_bulk_prepare_enable(DW_XPCS_NUM_CLKS, xpcs->clks);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to enable clocks\n");
+
+ return 0;
+}
+
+static void xpcs_clear_clks(struct dw_xpcs *xpcs)
+{
+ clk_bulk_disable_unprepare(DW_XPCS_NUM_CLKS, xpcs->clks);
+
+ clk_bulk_put(DW_XPCS_NUM_CLKS, xpcs->clks);
+}
+
static int xpcs_init_id(struct dw_xpcs *xpcs)
{
- u32 xpcs_id;
+ const struct dw_xpcs_info *info;
int i, ret;
- xpcs_id = xpcs_get_id(xpcs);
+ info = device_get_match_data(&xpcs->mdiodev->dev) ?:
+ dev_get_platdata(&xpcs->mdiodev->dev);
+ if (!info) {
+ xpcs->info.did = DW_XPCS_ID_NATIVE;
+ xpcs->info.pma = DW_XPCS_PMA_UNKNOWN;
+ } else {
+ xpcs->info = *info;
+ }
+
+ if (xpcs->info.did == DW_XPCS_ID_NATIVE)
+ xpcs->info.did = xpcs_get_id(xpcs);
for (i = 0; i < ARRAY_SIZE(xpcs_id_list); i++) {
const struct xpcs_id *entry = &xpcs_id_list[i];
- if ((xpcs_id & entry->mask) != entry->id)
+ if ((xpcs->info.did & entry->mask) != entry->id)
continue;
xpcs->id = entry;
@@ -1436,21 +1480,32 @@ static struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
struct dw_xpcs *xpcs;
int ret;
+ ret = device_attach(&mdiodev->dev);
+ if (ret < 0 && ret != -ENODEV)
+ return ERR_PTR(ret);
+
xpcs = xpcs_create_data(mdiodev);
if (IS_ERR(xpcs))
return xpcs;
+ ret = xpcs_init_clks(xpcs);
+ if (ret)
+ goto out_free_data;
+
ret = xpcs_init_id(xpcs);
if (ret)
- goto out;
+ goto out_clear_clks;
ret = xpcs_init_iface(xpcs, interface);
if (ret)
- goto out;
+ goto out_clear_clks;
return xpcs;
-out:
+out_clear_clks:
+ xpcs_clear_clks(xpcs);
+
+out_free_data:
xpcs_free_data(xpcs);
return ERR_PTR(ret);
@@ -1489,8 +1544,51 @@ void xpcs_destroy(struct dw_xpcs *xpcs)
mdio_device_put(xpcs->mdiodev);
+ xpcs_clear_clks(xpcs);
+
xpcs_free_data(xpcs);
}
EXPORT_SYMBOL_GPL(xpcs_destroy);
+DW_XPCS_INFO_DECLARE(xpcs_generic, DW_XPCS_ID_NATIVE, DW_XPCS_PMA_UNKNOWN);
+DW_XPCS_INFO_DECLARE(xpcs_pma_gen1_3g, DW_XPCS_ID_NATIVE, DW_XPCS_PMA_GEN1_3G);
+DW_XPCS_INFO_DECLARE(xpcs_pma_gen2_3g, DW_XPCS_ID_NATIVE, DW_XPCS_PMA_GEN2_3G);
+DW_XPCS_INFO_DECLARE(xpcs_pma_gen2_6g, DW_XPCS_ID_NATIVE, DW_XPCS_PMA_GEN2_6G);
+DW_XPCS_INFO_DECLARE(xpcs_pma_gen4_3g, DW_XPCS_ID_NATIVE, DW_XPCS_PMA_GEN4_3G);
+DW_XPCS_INFO_DECLARE(xpcs_pma_gen4_6g, DW_XPCS_ID_NATIVE, DW_XPCS_PMA_GEN4_6G);
+DW_XPCS_INFO_DECLARE(xpcs_pma_gen5_10g, DW_XPCS_ID_NATIVE, DW_XPCS_PMA_GEN5_10G);
+DW_XPCS_INFO_DECLARE(xpcs_pma_gen5_12g, DW_XPCS_ID_NATIVE, DW_XPCS_PMA_GEN5_12G);
+
+static const struct of_device_id xpcs_of_ids[] = {
+ { .compatible = "snps,dw-xpcs", .data = &xpcs_generic },
+ { .compatible = "snps,dw-xpcs-gen1-3g", .data = &xpcs_pma_gen1_3g },
+ { .compatible = "snps,dw-xpcs-gen2-3g", .data = &xpcs_pma_gen2_3g },
+ { .compatible = "snps,dw-xpcs-gen2-6g", .data = &xpcs_pma_gen2_6g },
+ { .compatible = "snps,dw-xpcs-gen4-3g", .data = &xpcs_pma_gen4_3g },
+ { .compatible = "snps,dw-xpcs-gen4-6g", .data = &xpcs_pma_gen4_6g },
+ { .compatible = "snps,dw-xpcs-gen5-10g", .data = &xpcs_pma_gen5_10g },
+ { .compatible = "snps,dw-xpcs-gen5-12g", .data = &xpcs_pma_gen5_12g },
+ { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, xpcs_of_ids);
+
+static struct mdio_device_id __maybe_unused xpcs_mdio_ids[] = {
+ { DW_XPCS_ID, DW_XPCS_ID_MASK },
+ { NXP_SJA1105_XPCS_ID, DW_XPCS_ID_MASK },
+ { NXP_SJA1110_XPCS_ID, DW_XPCS_ID_MASK },
+ { }
+};
+MODULE_DEVICE_TABLE(mdio, xpcs_mdio_ids);
+
+static struct mdio_driver xpcs_driver = {
+ .mdiodrv.driver = {
+ .name = "dwxpcs",
+ .of_match_table = xpcs_of_ids,
+ .probe_type = PROBE_FORCE_SYNCHRONOUS,
+ },
+};
+mdio_module_driver(xpcs_driver);
+
+MODULE_DESCRIPTION("DWC Ethernet XPCS platform driver");
+MODULE_AUTHOR("Jose Abreu <[email protected]>");
MODULE_LICENSE("GPL v2");
diff --git a/drivers/net/pcs/pcs-xpcs.h b/drivers/net/pcs/pcs-xpcs.h
index 369e9196f45a..45fea2641d23 100644
--- a/drivers/net/pcs/pcs-xpcs.h
+++ b/drivers/net/pcs/pcs-xpcs.h
@@ -6,6 +6,9 @@
* Author: Jose Abreu <[email protected]>
*/
+#include <linux/bits.h>
+#include <linux/pcs/pcs-xpcs.h>
+
/* Vendor regs access */
#define DW_VENDOR BIT(15)
@@ -117,6 +120,9 @@
/* VR MII EEE Control 1 defines */
#define DW_VR_MII_EEE_TRN_LPI BIT(0) /* Transparent Mode Enable */
+#define DW_XPCS_INFO_DECLARE(_name, _did, _pma) \
+ static const struct dw_xpcs_info _name = { .did = _did, .pma = _pma }
+
int xpcs_read(struct dw_xpcs *xpcs, int dev, u32 reg);
int xpcs_write(struct dw_xpcs *xpcs, int dev, u32 reg, u16 val);
int xpcs_read_vpcs(struct dw_xpcs *xpcs, int reg);
diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h
index 8dfe90295f12..53adbffb4c0a 100644
--- a/include/linux/pcs/pcs-xpcs.h
+++ b/include/linux/pcs/pcs-xpcs.h
@@ -7,9 +7,12 @@
#ifndef __LINUX_PCS_XPCS_H
#define __LINUX_PCS_XPCS_H
+#include <linux/clk.h>
+#include <linux/mdio.h>
#include <linux/phy.h>
#include <linux/phylink.h>
+#define DW_XPCS_ID_NATIVE 0x00000000
#define NXP_SJA1105_XPCS_ID 0x00000010
#define NXP_SJA1110_XPCS_ID 0x00000020
#define DW_XPCS_ID 0x7996ced0
@@ -30,9 +33,33 @@
struct xpcs_id;
+enum dw_xpcs_pma {
+ DW_XPCS_PMA_UNKNOWN = 0,
+ DW_XPCS_PMA_GEN1_3G,
+ DW_XPCS_PMA_GEN2_3G,
+ DW_XPCS_PMA_GEN2_6G,
+ DW_XPCS_PMA_GEN4_3G,
+ DW_XPCS_PMA_GEN4_6G,
+ DW_XPCS_PMA_GEN5_10G,
+ DW_XPCS_PMA_GEN5_12G,
+};
+
+enum dw_xpcs_clock {
+ DW_XPCS_CLK_CORE,
+ DW_XPCS_CLK_PAD,
+ DW_XPCS_NUM_CLKS,
+};
+
+struct dw_xpcs_info {
+ u32 did;
+ u32 pma;
+};
+
struct dw_xpcs {
struct mdio_device *mdiodev;
+ struct dw_xpcs_info info;
const struct xpcs_id *id;
+ struct clk_bulk_data clks[DW_XPCS_NUM_CLKS];
struct phylink_pcs pcs;
phy_interface_t interface;
int dev_flag;
--
2.42.1
Synopsys DesignWare XPCS IP-core can be synthesized with the device CSRs
being accessible over MCI or APB3 interface instead of the MDIO bus (see
the CSR_INTERFACE HDL parameter). Thus all the PCS registers can be just
memory mapped and be a subject of standard MMIO operations of course
taking into account the way the Clause C45 CSRs mapping is defined. This
commit is about adding a device driver for the DW XPCS Management
Interface platform device and registering it in the framework of the
kernel MDIO subsystem.
DW XPCS platform device is supposed to be described by the respective
compatible string "snps,dw-xpcs-mi", CSRs memory space and optional
peripheral bus clock source. Note depending on the INDIRECT_ACCESS DW XPCS
IP-core synthesize parameter the memory-mapped reg-space can be
represented as either directly or indirectly mapped Clause 45 space. In
the former case the particular address is determined based on the MMD
device and the registers offset (5 + 16 bits all together) within the
device reg-space. In the later case there is only 256 lower address bits
are utilized for the registers mapping. The upper bits are supposed to be
written into the respective viewport CSR in order to reach the entire C45
space.
Signed-off-by: Serge Semin <[email protected]>
---
drivers/net/mdio/Kconfig | 8 +
drivers/net/mdio/Makefile | 1 +
drivers/net/mdio/mdio-dw-xpcs.c | 384 ++++++++++++++++++++++++++++++++
3 files changed, 393 insertions(+)
create mode 100644 drivers/net/mdio/mdio-dw-xpcs.c
diff --git a/drivers/net/mdio/Kconfig b/drivers/net/mdio/Kconfig
index 4a7a303be2f7..39f7ce8087bf 100644
--- a/drivers/net/mdio/Kconfig
+++ b/drivers/net/mdio/Kconfig
@@ -185,6 +185,14 @@ config MDIO_IPQ8064
This driver supports the MDIO interface found in the network
interface units of the IPQ8064 SoC
+config MDIO_DW_XPCS
+ tristate "Synopsys DesignWare XPCS MI bus support"
+ depends on HAS_IOMEM
+ select MDIO_DEVRES
+ help
+ This driver supports the MCI/APB3 Management Interface responsible
+ for communicating with the Synopsys DesignWare XPCS devices.
+
config MDIO_REGMAP
tristate
help
diff --git a/drivers/net/mdio/Makefile b/drivers/net/mdio/Makefile
index 1015f0db4531..6389d4c3b862 100644
--- a/drivers/net/mdio/Makefile
+++ b/drivers/net/mdio/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_MDIO_BCM_IPROC) += mdio-bcm-iproc.o
obj-$(CONFIG_MDIO_BCM_UNIMAC) += mdio-bcm-unimac.o
obj-$(CONFIG_MDIO_BITBANG) += mdio-bitbang.o
obj-$(CONFIG_MDIO_CAVIUM) += mdio-cavium.o
+obj-$(CONFIG_MDIO_DW_XPCS) += mdio-dw-xpcs.o
obj-$(CONFIG_MDIO_GPIO) += mdio-gpio.o
obj-$(CONFIG_MDIO_HISI_FEMAC) += mdio-hisi-femac.o
obj-$(CONFIG_MDIO_I2C) += mdio-i2c.o
diff --git a/drivers/net/mdio/mdio-dw-xpcs.c b/drivers/net/mdio/mdio-dw-xpcs.c
new file mode 100644
index 000000000000..c47f0a54d31b
--- /dev/null
+++ b/drivers/net/mdio/mdio-dw-xpcs.c
@@ -0,0 +1,384 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Synopsys DesignWare XPCS Management Interface driver
+ *
+ * Copyright (C) 2023 BAIKAL ELECTRONICS, JSC
+ */
+
+#include <linux/atomic.h>
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/mdio.h>
+#include <linux/module.h>
+#include <linux/of_mdio.h>
+#include <linux/phy.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/property.h>
+#include <linux/sizes.h>
+
+/* Page select register for the indirect MMIO CSRs access */
+#define DW_VR_CSR_VIEWPORT 0xff
+
+struct dw_xpcs_mi {
+ struct platform_device *pdev;
+ struct mii_bus *bus;
+ bool reg_indir;
+ int reg_width;
+ void __iomem *reg_base;
+ struct clk *pclk;
+};
+
+static inline ptrdiff_t dw_xpcs_mmio_addr_format(int dev, int reg)
+{
+ return FIELD_PREP(0x1f0000, dev) | FIELD_PREP(0xffff, reg);
+}
+
+static inline u16 dw_xpcs_mmio_addr_page(ptrdiff_t csr)
+{
+ return FIELD_GET(0x1fff00, csr);
+}
+
+static inline ptrdiff_t dw_xpcs_mmio_addr_offset(ptrdiff_t csr)
+{
+ return FIELD_GET(0xff, csr);
+}
+
+static int dw_xpcs_mmio_read_reg_indirect(struct dw_xpcs_mi *dxmi,
+ int dev, int reg)
+{
+ ptrdiff_t csr, ofs;
+ u16 page;
+ int ret;
+
+ csr = dw_xpcs_mmio_addr_format(dev, reg);
+ page = dw_xpcs_mmio_addr_page(csr);
+ ofs = dw_xpcs_mmio_addr_offset(csr);
+
+ ret = pm_runtime_resume_and_get(&dxmi->pdev->dev);
+ if (ret)
+ return ret;
+
+ switch (dxmi->reg_width) {
+ case 4:
+ writel(page, dxmi->reg_base + (DW_VR_CSR_VIEWPORT << 2));
+ ret = readl(dxmi->reg_base + (ofs << 2));
+ break;
+ default:
+ writew(page, dxmi->reg_base + (DW_VR_CSR_VIEWPORT << 1));
+ ret = readw(dxmi->reg_base + (ofs << 1));
+ break;
+ }
+
+ pm_runtime_put(&dxmi->pdev->dev);
+
+ return ret;
+}
+
+static int dw_xpcs_mmio_write_reg_indirect(struct dw_xpcs_mi *dxmi,
+ int dev, int reg, u16 val)
+{
+ ptrdiff_t csr, ofs;
+ u16 page;
+ int ret;
+
+ csr = dw_xpcs_mmio_addr_format(dev, reg);
+ page = dw_xpcs_mmio_addr_page(csr);
+ ofs = dw_xpcs_mmio_addr_offset(csr);
+
+ ret = pm_runtime_resume_and_get(&dxmi->pdev->dev);
+ if (ret)
+ return ret;
+
+ switch (dxmi->reg_width) {
+ case 4:
+ writel(page, dxmi->reg_base + (DW_VR_CSR_VIEWPORT << 2));
+ writel(val, dxmi->reg_base + (ofs << 2));
+ break;
+ default:
+ writew(page, dxmi->reg_base + (DW_VR_CSR_VIEWPORT << 1));
+ writew(val, dxmi->reg_base + (ofs << 1));
+ break;
+ }
+
+ pm_runtime_put(&dxmi->pdev->dev);
+
+ return 0;
+}
+
+static int dw_xpcs_mmio_read_reg_direct(struct dw_xpcs_mi *dxmi,
+ int dev, int reg)
+{
+ ptrdiff_t csr;
+ int ret;
+
+ csr = dw_xpcs_mmio_addr_format(dev, reg);
+
+ ret = pm_runtime_resume_and_get(&dxmi->pdev->dev);
+ if (ret)
+ return ret;
+
+ switch (dxmi->reg_width) {
+ case 4:
+ ret = readl(dxmi->reg_base + (csr << 2));
+ break;
+ default:
+ ret = readw(dxmi->reg_base + (csr << 1));
+ break;
+ }
+
+ pm_runtime_put(&dxmi->pdev->dev);
+
+ return ret;
+}
+
+static int dw_xpcs_mmio_write_reg_direct(struct dw_xpcs_mi *dxmi,
+ int dev, int reg, u16 val)
+{
+ ptrdiff_t csr;
+ int ret;
+
+ csr = dw_xpcs_mmio_addr_format(dev, reg);
+
+ ret = pm_runtime_resume_and_get(&dxmi->pdev->dev);
+ if (ret)
+ return ret;
+
+ switch (dxmi->reg_width) {
+ case 4:
+ writel(val, dxmi->reg_base + (csr << 2));
+ break;
+ default:
+ writew(val, dxmi->reg_base + (csr << 1));
+ break;
+ }
+
+ pm_runtime_put(&dxmi->pdev->dev);
+
+ return 0;
+}
+
+static int dw_xpcs_mmio_read_c22(struct mii_bus *bus, int addr, int reg)
+{
+ struct dw_xpcs_mi *dxmi = bus->priv;
+
+ if (addr != 0)
+ return -ENODEV;
+
+ if (dxmi->reg_indir)
+ return dw_xpcs_mmio_read_reg_indirect(dxmi, MDIO_MMD_VEND2, reg);
+ else
+ return dw_xpcs_mmio_read_reg_direct(dxmi, MDIO_MMD_VEND2, reg);
+}
+
+static int dw_xpcs_mmio_write_c22(struct mii_bus *bus, int addr, int reg, u16 val)
+{
+ struct dw_xpcs_mi *dxmi = bus->priv;
+
+ if (addr != 0)
+ return -ENODEV;
+
+ if (dxmi->reg_indir)
+ return dw_xpcs_mmio_write_reg_indirect(dxmi, MDIO_MMD_VEND2, reg, val);
+ else
+ return dw_xpcs_mmio_write_reg_direct(dxmi, MDIO_MMD_VEND2, reg, val);
+}
+
+static int dw_xpcs_mmio_read_c45(struct mii_bus *bus, int addr, int dev, int reg)
+{
+ struct dw_xpcs_mi *dxmi = bus->priv;
+
+ if (addr != 0)
+ return -ENODEV;
+
+ if (dxmi->reg_indir)
+ return dw_xpcs_mmio_read_reg_indirect(dxmi, dev, reg);
+ else
+ return dw_xpcs_mmio_read_reg_direct(dxmi, dev, reg);
+}
+
+static int dw_xpcs_mmio_write_c45(struct mii_bus *bus, int addr, int dev,
+ int reg, u16 val)
+{
+ struct dw_xpcs_mi *dxmi = bus->priv;
+
+ if (addr != 0)
+ return -ENODEV;
+
+ if (dxmi->reg_indir)
+ return dw_xpcs_mmio_write_reg_indirect(dxmi, dev, reg, val);
+ else
+ return dw_xpcs_mmio_write_reg_direct(dxmi, dev, reg, val);
+}
+
+static struct dw_xpcs_mi *dw_xpcs_mi_create_data(struct platform_device *pdev)
+{
+ struct dw_xpcs_mi *dxmi;
+
+ dxmi = devm_kzalloc(&pdev->dev, sizeof(*dxmi), GFP_KERNEL);
+ if (!dxmi)
+ return ERR_PTR(-ENOMEM);
+
+ dxmi->pdev = pdev;
+
+ dev_set_drvdata(&pdev->dev, dxmi);
+
+ return dxmi;
+}
+
+static int dw_xpcs_mi_init_res(struct dw_xpcs_mi *dxmi)
+{
+ struct device *dev = &dxmi->pdev->dev;
+ struct resource *res;
+
+ if (!device_property_read_u32(dev, "reg-io-width", &dxmi->reg_width)) {
+ if (dxmi->reg_width != 2 && dxmi->reg_width != 4) {
+ dev_err(dev, "Invalid regspace data width\n");
+ return -EINVAL;
+ }
+ } else {
+ dxmi->reg_width = 2;
+ }
+
+ res = platform_get_resource_byname(dxmi->pdev, IORESOURCE_MEM, "direct") ?:
+ platform_get_resource_byname(dxmi->pdev, IORESOURCE_MEM, "indirect");
+ if (!res) {
+ dev_err(dev, "No regspace found\n");
+ return -EINVAL;
+ }
+
+ if (!strcmp(res->name, "indirect"))
+ dxmi->reg_indir = true;
+
+ if ((dxmi->reg_indir && resource_size(res) < dxmi->reg_width * SZ_256) ||
+ (!dxmi->reg_indir && resource_size(res) < dxmi->reg_width * SZ_2M)) {
+ dev_err(dev, "Invalid regspace size\n");
+ return -EINVAL;
+ }
+
+ dxmi->reg_base = devm_ioremap_resource(dev, res);
+ if (IS_ERR(dxmi->reg_base)) {
+ dev_err(dev, "Failed to map regspace\n");
+ return PTR_ERR(dxmi->reg_base);
+ }
+
+ return 0;
+}
+
+static int dw_xpcs_mi_init_clk(struct dw_xpcs_mi *dxmi)
+{
+ struct device *dev = &dxmi->pdev->dev;
+ int ret;
+
+ dxmi->pclk = devm_clk_get_optional(dev, "pclk");
+ if (IS_ERR(dxmi->pclk))
+ return dev_err_probe(dev, PTR_ERR(dxmi->pclk),
+ "Failed to get ref clock\n");
+
+ pm_runtime_set_active(dev);
+ ret = devm_pm_runtime_enable(dev);
+ if (ret) {
+ dev_err(dev, "Failed to enable runtime-PM\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+static int dw_xpcs_mi_init_mdio(struct dw_xpcs_mi *dxmi)
+{
+ struct device *dev = &dxmi->pdev->dev;
+ static atomic_t id = ATOMIC_INIT(-1);
+ int ret;
+
+ dxmi->bus = devm_mdiobus_alloc_size(dev, 0);
+ if (!dxmi->bus)
+ return -ENOMEM;
+
+ dxmi->bus->name = "DW XPCS MI";
+ dxmi->bus->read = dw_xpcs_mmio_read_c22;
+ dxmi->bus->write = dw_xpcs_mmio_write_c22;
+ dxmi->bus->read_c45 = dw_xpcs_mmio_read_c45;
+ dxmi->bus->write_c45 = dw_xpcs_mmio_write_c45;
+ dxmi->bus->phy_mask = ~0;
+ dxmi->bus->parent = dev;
+ dxmi->bus->priv = dxmi;
+
+ snprintf(dxmi->bus->id, MII_BUS_ID_SIZE,
+ "dwxpcs-%x", atomic_inc_return(&id));
+
+ ret = devm_of_mdiobus_register(dev, dxmi->bus, dev_of_node(dev));
+ if (ret) {
+ dev_err(dev, "Failed to create MDIO bus\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+static int dw_xpcs_mi_probe(struct platform_device *pdev)
+{
+ struct dw_xpcs_mi *dxmi;
+ int ret;
+
+ dxmi = dw_xpcs_mi_create_data(pdev);
+ if (IS_ERR(dxmi))
+ return PTR_ERR(dxmi);
+
+ ret = dw_xpcs_mi_init_res(dxmi);
+ if (ret)
+ return ret;
+
+ ret = dw_xpcs_mi_init_clk(dxmi);
+ if (ret)
+ return ret;
+
+ ret = dw_xpcs_mi_init_mdio(dxmi);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static int __maybe_unused dw_xpcs_mi_pm_runtime_suspend(struct device *dev)
+{
+ struct dw_xpcs_mi *dxmi = dev_get_drvdata(dev);
+
+ clk_disable_unprepare(dxmi->pclk);
+
+ return 0;
+}
+
+static int __maybe_unused dw_xpcs_mi_pm_runtime_resume(struct device *dev)
+{
+ struct dw_xpcs_mi *dxmi = dev_get_drvdata(dev);
+
+ return clk_prepare_enable(dxmi->pclk);
+}
+
+const struct dev_pm_ops dw_xpcs_mi_pm_ops = {
+ SET_RUNTIME_PM_OPS(dw_xpcs_mi_pm_runtime_suspend, dw_xpcs_mi_pm_runtime_resume, NULL)
+};
+
+static const struct of_device_id dw_xpcs_mi_of_ids[] = {
+ { .compatible = "snps,dw-xpcs-mi" },
+ { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, dw_xpcs_mi_of_ids);
+
+static struct platform_driver dw_xpcs_mi_driver = {
+ .probe = dw_xpcs_mi_probe,
+ .driver = {
+ .name = "dw-xpcs-mi",
+ .pm = &dw_xpcs_mi_pm_ops,
+ .of_match_table = dw_xpcs_mi_of_ids,
+ },
+};
+
+module_platform_driver(dw_xpcs_mi_driver);
+
+MODULE_DESCRIPTION("Synopsys DesignWare XPCS Management Interface driver");
+MODULE_AUTHOR("Serge Semin <[email protected]>");
+MODULE_LICENSE("GPL v2");
--
2.42.1
It's possible to have the XPCS device accessible over a dedicated
management interface which makes the XPCS device available over the MMIO
space. In that case the management interface will be registered as a
separate MDIO bus and the DW xGMAC device will be equipped with the
"pcs-handle" property pointing to the XPCS device instead of
auto-detecting it on the internal MDIO bus. In such configurations the SMA
interface (embedded into the DW xGMAC MDIO interface) might be absent.
Thus passing the MII bus interface handler to the stmmac_xpcs_setup()
method won't let us reach the externally supplied XPCS device especially
if the SMA bus isn't configured. Let's fix it by converting the
stmmac_xpcs_setup(struct mii_bus *bus) prototype to
stmmac_xpcs_setup(struct net_device *ndev).
Note this is a preparation patch before adding the support of the XPCS
devices specified via the "pcs-handle" property.
Signed-off-by: Serge Semin <[email protected]>
---
drivers/net/ethernet/stmicro/stmmac/stmmac.h | 2 +-
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +-
drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 5 ++---
3 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index cd7a9768de5f..d8a1c84880c5 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -343,7 +343,7 @@ enum stmmac_state {
int stmmac_mdio_unregister(struct net_device *ndev);
int stmmac_mdio_register(struct net_device *ndev);
int stmmac_mdio_reset(struct mii_bus *mii);
-int stmmac_xpcs_setup(struct mii_bus *mii);
+int stmmac_xpcs_setup(struct net_device *ndev);
void stmmac_set_ethtool_ops(struct net_device *netdev);
int stmmac_init_tstamp_counter(struct stmmac_priv *priv, u32 systime_flags);
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 3e50fd53a617..c3641db00f96 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -7605,7 +7605,7 @@ int stmmac_dvr_probe(struct device *device,
priv->plat->speed_mode_2500(ndev, priv->plat->bsp_priv);
if (priv->plat->mdio_bus_data && priv->plat->mdio_bus_data->has_xpcs) {
- ret = stmmac_xpcs_setup(priv->mii);
+ ret = stmmac_xpcs_setup(ndev);
if (ret)
goto error_xpcs_setup;
}
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
index aa75e4f1e212..e6133510e28d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
@@ -495,9 +495,8 @@ int stmmac_mdio_reset(struct mii_bus *bus)
return 0;
}
-int stmmac_xpcs_setup(struct mii_bus *bus)
+int stmmac_xpcs_setup(struct net_device *ndev)
{
- struct net_device *ndev = bus->priv;
struct stmmac_priv *priv;
struct dw_xpcs *xpcs;
int mode, addr;
@@ -507,7 +506,7 @@ int stmmac_xpcs_setup(struct mii_bus *bus)
/* Try to probe the XPCS by scanning all addresses. */
for (addr = 0; addr < PHY_MAX_ADDR; addr++) {
- xpcs = xpcs_create_byaddr(bus, addr, mode);
+ xpcs = xpcs_create_byaddr(priv->mii, addr, mode);
if (IS_ERR(xpcs))
continue;
--
2.42.1
The DW XPCS driver has been updated to being bindable with the respective
MDIO device registered during the MDIO bus probe procedure. As an example
of using that feature let's convert the Intel mGBE low-level driver to
registering the MDIO-device board info. Thus the registered DW XPCS device
will be a subject of the fine-tunings performed during the MDIO-device
probe procedures.
Signed-off-by: Serge Semin <[email protected]>
---
.../net/ethernet/stmicro/stmmac/dwmac-intel.c | 31 ++++++++++++++++++-
1 file changed, 30 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
index 60283543ffc8..7642c11abc59 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
@@ -4,6 +4,7 @@
#include <linux/clk-provider.h>
#include <linux/pci.h>
+#include <linux/phy.h>
#include <linux/dmi.h>
#include "dwmac-intel.h"
#include "dwmac4.h"
@@ -585,6 +586,28 @@ static int intel_mgbe_common_data(struct pci_dev *pdev,
/* Intel mgbe SGMII interface uses pcs-xcps */
if (plat->phy_interface == PHY_INTERFACE_MODE_SGMII ||
plat->phy_interface == PHY_INTERFACE_MODE_1000BASEX) {
+ struct mdio_board_info *xpcs_info;
+
+ xpcs_info = devm_kzalloc(&pdev->dev,
+ sizeof(*xpcs_info) + MII_BUS_ID_SIZE,
+ GFP_KERNEL);
+ if (!xpcs_info) {
+ ret = -ENOMEM;
+ goto err_alloc_info;
+ }
+
+ xpcs_info->bus_id = (void *)xpcs_info + sizeof(*xpcs_info);
+ snprintf((char *)xpcs_info->bus_id, MII_BUS_ID_SIZE,
+ "stmmac-%x", plat->bus_id);
+
+ snprintf(xpcs_info->modalias, MDIO_NAME_SIZE, "dwxpcs");
+
+ xpcs_info->mdio_addr = INTEL_MGBE_XPCS_ADDR;
+
+ ret = mdiobus_register_board_info(xpcs_info, 1);
+ if (ret)
+ goto err_alloc_info;
+
plat->mdio_bus_data->has_xpcs = true;
plat->mdio_bus_data->xpcs_an_inband = true;
}
@@ -600,7 +623,7 @@ static int intel_mgbe_common_data(struct pci_dev *pdev,
fwnode_handle_put(fixed_node);
}
- /* Ensure mdio bus scan skips intel serdes and pcs-xpcs */
+ /* Ensure mdio bus PHY-scan skips intel serdes and pcs-xpcs */
plat->mdio_bus_data->phy_mask = 1 << INTEL_MGBE_ADHOC_ADDR;
plat->mdio_bus_data->phy_mask |= 1 << INTEL_MGBE_XPCS_ADDR;
@@ -618,6 +641,12 @@ static int intel_mgbe_common_data(struct pci_dev *pdev,
plat->msi_tx_base_vec = 1;
return 0;
+
+err_alloc_info:
+ clk_disable_unprepare(clk);
+ clk_unregister_fixed_rate(clk);
+
+ return ret;
}
static int ehl_common_data(struct pci_dev *pdev,
--
2.42.1
In particular the xpcs_soft_reset() and xpcs_do_config() functions
currently return -1 if invalid auto-negotiation mode is specified. That
value can be then passed to the generic kernel subsystems which require a
standard kernel errno value. Even though the error conditions are very
specific (memory corruption or buggy implementation) using a hard-coded -1
literal doesn't seem correct anyway.
Signed-off-by: Serge Semin <[email protected]>
---
drivers/net/pcs/pcs-xpcs.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 7f8c63922a4b..92c47da61db4 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -292,7 +292,7 @@ static int xpcs_soft_reset(struct dw_xpcs *xpcs,
dev = MDIO_MMD_VEND2;
break;
default:
- return -1;
+ return -EINVAL;
}
ret = xpcs_write(xpcs, dev, MDIO_CTRL1, MDIO_CTRL1_RESET);
@@ -889,7 +889,7 @@ int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface,
return ret;
break;
default:
- return -1;
+ return -EINVAL;
}
if (compat->pma_config) {
--
2.42.1
The fwnode-based way of creating XPCS descriptor is about to be added. In
order to have a function name distinguishable from the already implemented
xpcs_create_mdiodev() method convert the later name to be
xpcs_create_byaddr() which BTW better describes the method semantic in
anyway.
Signed-off-by: Serge Semin <[email protected]>
---
drivers/net/dsa/sja1105/sja1105_mdio.c | 2 +-
drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 2 +-
drivers/net/pcs/pcs-xpcs.c | 6 +++---
include/linux/pcs/pcs-xpcs.h | 4 ++--
4 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/net/dsa/sja1105/sja1105_mdio.c b/drivers/net/dsa/sja1105/sja1105_mdio.c
index 833e55e4b961..9101079e365d 100644
--- a/drivers/net/dsa/sja1105/sja1105_mdio.c
+++ b/drivers/net/dsa/sja1105/sja1105_mdio.c
@@ -409,7 +409,7 @@ static int sja1105_mdiobus_pcs_register(struct sja1105_private *priv)
priv->phy_mode[port] != PHY_INTERFACE_MODE_2500BASEX)
continue;
- xpcs = xpcs_create_mdiodev(bus, port, priv->phy_mode[port]);
+ xpcs = xpcs_create_byaddr(bus, port, priv->phy_mode[port]);
if (IS_ERR(xpcs)) {
rc = PTR_ERR(xpcs);
goto out_pcs_free;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
index fa9e7e7040b9..aa75e4f1e212 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
@@ -507,7 +507,7 @@ int stmmac_xpcs_setup(struct mii_bus *bus)
/* Try to probe the XPCS by scanning all addresses. */
for (addr = 0; addr < PHY_MAX_ADDR; addr++) {
- xpcs = xpcs_create_mdiodev(bus, addr, mode);
+ xpcs = xpcs_create_byaddr(bus, addr, mode);
if (IS_ERR(xpcs))
continue;
diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 183a37929b60..e376e255f1d3 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -1511,8 +1511,8 @@ static struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
return ERR_PTR(ret);
}
-struct dw_xpcs *xpcs_create_mdiodev(struct mii_bus *bus, int addr,
- phy_interface_t interface)
+struct dw_xpcs *xpcs_create_byaddr(struct mii_bus *bus, int addr,
+ phy_interface_t interface)
{
struct mdio_device *mdiodev;
struct dw_xpcs *xpcs;
@@ -1535,7 +1535,7 @@ struct dw_xpcs *xpcs_create_mdiodev(struct mii_bus *bus, int addr,
return xpcs;
}
-EXPORT_SYMBOL_GPL(xpcs_create_mdiodev);
+EXPORT_SYMBOL_GPL(xpcs_create_byaddr);
void xpcs_destroy(struct dw_xpcs *xpcs)
{
diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h
index 53adbffb4c0a..b11bbb5e820a 100644
--- a/include/linux/pcs/pcs-xpcs.h
+++ b/include/linux/pcs/pcs-xpcs.h
@@ -73,8 +73,8 @@ int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface,
void xpcs_get_interfaces(struct dw_xpcs *xpcs, unsigned long *interfaces);
int xpcs_config_eee(struct dw_xpcs *xpcs, int mult_fact_100ns,
int enable);
-struct dw_xpcs *xpcs_create_mdiodev(struct mii_bus *bus, int addr,
- phy_interface_t interface);
+struct dw_xpcs *xpcs_create_byaddr(struct mii_bus *bus, int addr,
+ phy_interface_t interface);
void xpcs_destroy(struct dw_xpcs *xpcs);
#endif /* __LINUX_PCS_XPCS_H */
--
2.42.1
It's now possible to have the DW XPCS device defined as a standard
platform device for instance in the platform DT-file. Although it's
pointless unless there is a way to have the device found by the client
drivers (STMMAC/DW *MAC, NXP SJA1105 Eth Switch, etc). Provide such
ability by means of the xpcs_create_bynode() method. It needs to be
supplied with the device fwnode which is equipped with the "pcs-handle"
property pointing to the DW XPCS fw-node (in this regards it looks similar
to the phylink interface). That node will be then used to find the
MDIO-device instance in order to create the DW XPCS descriptor.
Signed-off-by: Serge Semin <[email protected]>
---
drivers/net/pcs/pcs-xpcs.c | 26 ++++++++++++++++++++++++++
include/linux/pcs/pcs-xpcs.h | 3 +++
2 files changed, 29 insertions(+)
diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index e376e255f1d3..c3336895a124 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -9,9 +9,11 @@
#include <linux/clk.h>
#include <linux/delay.h>
#include <linux/device.h>
+#include <linux/fwnode.h>
#include <linux/mdio.h>
#include <linux/module.h>
#include <linux/pcs/pcs-xpcs.h>
+#include <linux/phy.h>
#include <linux/phylink.h>
#include <linux/property.h>
@@ -1511,6 +1513,30 @@ static struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
return ERR_PTR(ret);
}
+struct dw_xpcs *xpcs_create_bynode(const struct fwnode_handle *fwnode,
+ phy_interface_t interface)
+{
+ struct fwnode_handle *pcs_node;
+ struct mdio_device *mdiodev;
+ struct dw_xpcs *xpcs;
+
+ pcs_node = fwnode_find_reference(fwnode, "pcs-handle", 0);
+ if (IS_ERR(pcs_node))
+ return ERR_CAST(pcs_node);
+
+ mdiodev = fwnode_mdio_find_device(pcs_node);
+ fwnode_handle_put(pcs_node);
+ if (!mdiodev)
+ return ERR_PTR(-ENODEV);
+
+ xpcs = xpcs_create(mdiodev, interface);
+ if (IS_ERR(xpcs))
+ mdio_device_put(mdiodev);
+
+ return xpcs;
+}
+EXPORT_SYMBOL_GPL(xpcs_create_bynode);
+
struct dw_xpcs *xpcs_create_byaddr(struct mii_bus *bus, int addr,
phy_interface_t interface)
{
diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h
index b11bbb5e820a..2981dcdf65d4 100644
--- a/include/linux/pcs/pcs-xpcs.h
+++ b/include/linux/pcs/pcs-xpcs.h
@@ -8,6 +8,7 @@
#define __LINUX_PCS_XPCS_H
#include <linux/clk.h>
+#include <linux/fwnode.h>
#include <linux/mdio.h>
#include <linux/phy.h>
#include <linux/phylink.h>
@@ -73,6 +74,8 @@ int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface,
void xpcs_get_interfaces(struct dw_xpcs *xpcs, unsigned long *interfaces);
int xpcs_config_eee(struct dw_xpcs *xpcs, int mult_fact_100ns,
int enable);
+struct dw_xpcs *xpcs_create_bynode(const struct fwnode_handle *fwnode,
+ phy_interface_t interface);
struct dw_xpcs *xpcs_create_byaddr(struct mii_bus *bus, int addr,
phy_interface_t interface);
void xpcs_destroy(struct dw_xpcs *xpcs);
--
2.42.1
It's possible to have the DW XPCS device accessible over an external bus
(external MDIO or DW XPCS management interface). Thus it will be futile to
try to detect the device on the local SMA interface. Besides such platform
setup isn't supported by the STMMAC driver anyway since the
stmmac_mdio_bus_data instance might not be created and even if it is there
is no code path which would set the stmmac_mdio_bus_data.has_xpcs flag
thus activating the XPCS device setup.
So in order to solve the denoted problem a pretty much standard approach
is implemented: DT "pcs-handle" property is used to get the phandle
referencing the DT-node describing the DW XPCS device; device node will be
parsed by the xpcs_create_bynode() method implemented in the DW XPCS
driver in a way as it's done for PHY-node; the node is used to find the
MDIO-device instance, which in its turn will be used to create the XPCS
descriptor.
Note as a nice side effect of the provided change the conditional
stmmac_xpcs_setup() method execution can be converted to the conditional
statements implemented in the function itself. Thus the stmmac_open() will
turn to look a bit simpler meanwhile stmmac_xpcs_setup() will provide the
optional XPCS device semantic.
Signed-off-by: Serge Semin <[email protected]>
---
.../net/ethernet/stmicro/stmmac/stmmac_main.c | 8 ++---
.../net/ethernet/stmicro/stmmac/stmmac_mdio.c | 34 ++++++++++++-------
2 files changed, 24 insertions(+), 18 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 379552240ac9..a33ba00d091d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -7604,11 +7604,9 @@ int stmmac_dvr_probe(struct device *device,
if (priv->plat->speed_mode_2500)
priv->plat->speed_mode_2500(ndev, priv->plat->bsp_priv);
- if (priv->plat->mdio_bus_data && priv->plat->mdio_bus_data->has_xpcs) {
- ret = stmmac_xpcs_setup(ndev);
- if (ret)
- goto error_xpcs_setup;
- }
+ ret = stmmac_xpcs_setup(ndev);
+ if (ret)
+ goto error_xpcs_setup;
ret = stmmac_phy_setup(priv);
if (ret) {
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
index 101fa50c3c96..b906be363b61 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
@@ -499,25 +499,33 @@ int stmmac_xpcs_setup(struct net_device *ndev)
{
struct stmmac_priv *priv;
struct dw_xpcs *xpcs;
- int mode, addr;
+ int ret, mode, addr;
priv = netdev_priv(ndev);
mode = priv->plat->phy_interface;
- /* Try to probe the XPCS by scanning all addresses. */
- for (addr = 0; addr < PHY_MAX_ADDR; addr++) {
- xpcs = xpcs_create_byaddr(priv->mii, addr, mode);
- if (IS_ERR(xpcs))
- continue;
-
- priv->hw->xpcs = xpcs;
- break;
+ /* If PCS-node is specified use it to create the XPCS descriptor */
+ if (fwnode_property_present(priv->plat->port_node, "pcs-handle")) {
+ xpcs = xpcs_create_bynode(priv->plat->port_node, mode);
+ ret = PTR_ERR_OR_ZERO(xpcs);
+ } else if (priv->plat->mdio_bus_data && priv->plat->mdio_bus_data->has_xpcs) {
+ /* Try to probe the XPCS by scanning all addresses */
+ for (ret = -ENODEV, addr = 0; addr < PHY_MAX_ADDR; addr++) {
+ xpcs = xpcs_create_byaddr(priv->mii, addr, mode);
+ if (IS_ERR(xpcs))
+ continue;
+
+ ret = 0;
+ break;
+ }
+ } else {
+ return 0;
}
- if (!priv->hw->xpcs) {
- dev_warn(priv->device, "No xPCS found\n");
- return -ENODEV;
- }
+ if (ret)
+ return dev_err_probe(priv->device, ret, "No xPCS found\n");
+
+ priv->hw->xpcs = xpcs;
return 0;
}
--
2.42.1
Currently the XPCS handler destruction is performed in the
stmmac_mdio_unregister() method. It doesn't look well because the handler
isn't originally created in the corresponding protagonist
stmmac_mdio_unregister(), but in the stmmac_xpcs_setup() function. In
order to have a bit more coherent MDIO and XPCS setup/cleanup procedures
let's move the DW XPCS destruction to the dedicated stmmac_xpcs_clean()
method.
Note besides of that this change is a preparation to adding the PCS device
supplied by means of the "pcs-handle" property. It's required since DW
XPCS IP-core can be synthesized embedded into the chip with directly
accessible CSRs. In that case the SMA interface can be absent so no
corresponding MDIO bus will be registered.
Signed-off-by: Serge Semin <[email protected]>
---
drivers/net/ethernet/stmicro/stmmac/stmmac.h | 1 +
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 +++++-
drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 14 +++++++++++---
3 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index d8a1c84880c5..1709de519813 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -344,6 +344,7 @@ int stmmac_mdio_unregister(struct net_device *ndev);
int stmmac_mdio_register(struct net_device *ndev);
int stmmac_mdio_reset(struct mii_bus *mii);
int stmmac_xpcs_setup(struct net_device *ndev);
+void stmmac_xpcs_clean(struct net_device *ndev);
void stmmac_set_ethtool_ops(struct net_device *netdev);
int stmmac_init_tstamp_counter(struct stmmac_priv *priv, u32 systime_flags);
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index c3641db00f96..379552240ac9 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -7639,8 +7639,9 @@ int stmmac_dvr_probe(struct device *device,
error_netdev_register:
phylink_destroy(priv->phylink);
-error_xpcs_setup:
error_phy_setup:
+ stmmac_xpcs_clean(ndev);
+error_xpcs_setup:
if (priv->hw->pcs != STMMAC_PCS_TBI &&
priv->hw->pcs != STMMAC_PCS_RTBI)
stmmac_mdio_unregister(ndev);
@@ -7682,6 +7683,9 @@ void stmmac_dvr_remove(struct device *dev)
if (priv->plat->stmmac_rst)
reset_control_assert(priv->plat->stmmac_rst);
reset_control_assert(priv->plat->stmmac_ahb_rst);
+
+ stmmac_xpcs_clean(ndev);
+
if (priv->hw->pcs != STMMAC_PCS_TBI &&
priv->hw->pcs != STMMAC_PCS_RTBI)
stmmac_mdio_unregister(ndev);
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
index e6133510e28d..101fa50c3c96 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
@@ -522,6 +522,17 @@ int stmmac_xpcs_setup(struct net_device *ndev)
return 0;
}
+void stmmac_xpcs_clean(struct net_device *ndev)
+{
+ struct stmmac_priv *priv = netdev_priv(ndev);
+
+ if (!priv->hw->xpcs)
+ return;
+
+ xpcs_destroy(priv->hw->xpcs);
+ priv->hw->xpcs = NULL;
+}
+
/**
* stmmac_mdio_register
* @ndev: net device structure
@@ -674,9 +685,6 @@ int stmmac_mdio_unregister(struct net_device *ndev)
if (!priv->mii)
return 0;
- if (priv->hw->xpcs)
- xpcs_destroy(priv->hw->xpcs);
-
mdiobus_unregister(priv->mii);
priv->mii->priv = NULL;
mdiobus_free(priv->mii);
--
2.42.1
On Tue, Dec 05, 2023 at 01:35:31PM +0300, Serge Semin wrote:
> @@ -1436,21 +1480,32 @@ static struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
> struct dw_xpcs *xpcs;
> int ret;
>
> + ret = device_attach(&mdiodev->dev);
> + if (ret < 0 && ret != -ENODEV)
> + return ERR_PTR(ret);
> +
> xpcs = xpcs_create_data(mdiodev);
> if (IS_ERR(xpcs))
> return xpcs;
>
> + ret = xpcs_init_clks(xpcs);
> + if (ret)
> + goto out_free_data;
> +
> ret = xpcs_init_id(xpcs);
> if (ret)
> - goto out;
> + goto out_clear_clks;
>
> ret = xpcs_init_iface(xpcs, interface);
> if (ret)
> - goto out;
> + goto out_clear_clks;
>
> return xpcs;
[ 4.083518] Unable to handle kernel NULL pointer dereference at virtual address 00000000000000d0
[ 4.092356] Mem abort info:
[ 4.095164] ESR = 0x0000000096000004
[ 4.098932] EC = 0x25: DABT (current EL), IL = 32 bits
[ 4.104277] SET = 0, FnV = 0
[ 4.107352] EA = 0, S1PTW = 0
[ 4.110505] FSC = 0x04: level 0 translation fault
[ 4.115408] Data abort info:
[ 4.118296] ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
[ 4.123807] CM = 0, WnR = 0, TnD = 0, TagAccess = 0
[ 4.128877] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
[ 4.134214] [00000000000000d0] user address but active_mm is swapper
[ 4.140595] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
[ 4.146882] Modules linked in:
[ 4.149944] CPU: 0 PID: 11 Comm: kworker/u4:0 Not tainted 6.7.0-rc3-00719-g75be5ea8e111-dirty #1551
[ 4.164524] Workqueue: events_unbound deferred_probe_work_func
[ 4.177372] pc : __device_attach+0x3c/0x1bc
[ 4.181570] lr : __device_attach+0x38/0x1bc
[ 4.185767] sp : ffff8000800f3800
[ 4.189087] x29: ffff8000800f3820 x28: 0000000000000001 x27: ffff063781bda150
[ 4.196252] x26: ffff063781bda150 x25: ffff063780827480 x24: ffffcb9a08138a40
[ 4.203416] x23: ffff063781114080 x22: 0000000000000000 x21: 0000000000000004
[ 4.210579] x20: ffff06378123a400 x19: ffff06378123a480 x18: ffffcb9a07c703a0
[ 4.217743] x17: ffffcb9a07c703a4 x16: 00000000000000d4 x15: ffffcb9a07be70fc
[ 4.224906] x14: ffffcb9a08299638 x13: 0000000000000053 x12: ffff003000000200
[ 4.232069] x11: 0000000000000000 x10: 0000000000000000 x9 : 0000000000000000
[ 4.239233] x8 : 0000000000000000 x7 : 0000000000000000 x6 : 000000000000003a
[ 4.246396] x5 : ffff0637809a037b x4 : ffffcb9a087b0d47 x3 : ffff10300000020f
[ 4.253560] x2 : ffffcb9a0910c561 x1 : 0000000000000000 x0 : ffff06378123a480
[ 4.260724] Call trace:
[ 4.263172] __device_attach+0x3c/0x1bc
[ 4.267020] device_attach+0x14/0x20
[ 4.270606] xpcs_create+0x24/0x384
[ 4.274107] xpcs_create_byaddr+0x74/0xa0
[ 4.278129] sja1105_mdiobus_register+0xf8/0x478
[ 4.282763] sja1105_setup+0xb4/0x1194
[ 4.286524] dsa_register_switch+0xab0/0x11f8
[ 4.290895] sja1105_probe+0x2bc/0x2e4
[ 4.294654] spi_probe+0xa4/0xc4
[ 4.297890] really_probe+0x16c/0x3fc
[ 4.301564] __driver_probe_device+0xa4/0x168
[ 4.305935] driver_probe_device+0x3c/0x220
[ 4.310131] __device_attach_driver+0x128/0x1cc
[ 4.314676] bus_for_each_drv+0xf4/0x14c
[ 4.318610] __device_attach+0xfc/0x1bc
[ 4.322457] device_initial_probe+0x14/0x20
[ 4.326654] bus_probe_device+0x94/0x100
[ 4.330587] deferred_probe_work_func+0xa0/0xfc
[ 4.335132] process_scheduled_works+0x210/0x318
[ 4.339764] worker_thread+0x28c/0x450
[ 4.343523] kthread+0xfc/0x184
[ 4.346669] ret_from_fork+0x10/0x20
[ 4.350256] Code: 2a0103f6 f81f83a8 9431ccd8 f9402688 (39434109)
[ 4.356366] ---[ end trace 0000000000000000 ]---
I haven't looked at the code at all, but disassembling drivers/base/dd.lst,
I think the NPD is at dev->p->dead (0xa68 + 0x3c = 0xaa4).
0000000000000a68 <__device_attach>:
; {
a68: d503233f paciasp
a6c: d10143ff sub sp, sp, #0x50
a70: a9027bfd stp x29, x30, [sp, #0x20]
a74: a90357f6 stp x22, x21, [sp, #0x30]
a78: a9044ff4 stp x20, x19, [sp, #0x40]
a7c: 910083fd add x29, sp, #0x20
a80: d5384108 mrs x8, SP_EL0
; mutex_lock(&dev->mutex);
a84: 91020013 add x19, x0, #0x80
a88: f9423508 ldr x8, [x8, #0x468]
a8c: aa0003f4 mov x20, x0
a90: aa1303e0 mov x0, x19
a94: 2a0103f6 mov w22, w1
a98: f81f83a8 stur x8, [x29, #-0x8]
a9c: 94000000 bl 0xa9c <__device_attach+0x34>
0000000000000a9c: R_AARCH64_CALL26 mutex_lock
; if (dev->p->dead) {
aa0: f9402688 ldr x8, [x20, #0x48]
aa4: 39434109 ldrb w9, [x8, #0xd0]
aa8: 37000129 tbnz w9, #0x0, 0xacc <__device_attach+0x64>
; } else if (dev->driver) {
aac: f9403689 ldr x9, [x20, #0x68]
ab0: b40003e9 cbz x9, 0xb2c <__device_attach+0xc4>
; return dev->p && klist_node_attached(&dev->p->knode_driver);
ab4: b40002a8 cbz x8, 0xb08 <__device_attach+0xa0>
ab8: 91012100 add x0, x8, #0x48
abc: 94000000 bl 0xabc <__device_attach+0x54>
0000000000000abc: R_AARCH64_CALL26 klist_node_attached
; if (device_is_bound(dev)) {
ac0: 34000240 cbz w0, 0xb08 <__device_attach+0xa0>
ac4: 52800035 mov w21, #0x1
ac8: 14000002 b 0xad0 <__device_attach+0x68>
acc: 2a1f03f5 mov w21, wzr
; mutex_unlock(&dev->mutex);
ad0: aa1303e0 mov x0, x19
ad4: 94000000 bl 0xad4 <__device_attach+0x6c>
0000000000000ad4: R_AARCH64_CALL26 mutex_unlock
ad8: d5384108 mrs x8, SP_EL0
adc: f9423508 ldr x8, [x8, #0x468]
ae0: f85f83a9 ldur x9, [x29, #-0x8]
ae4: eb09011f cmp x8, x9
ae8: 540008c1 b.ne 0xc00 <__device_attach+0x198>
; return ret;
aec: 2a1503e0 mov w0, w21
af0: a9444ff4 ldp x20, x19, [sp, #0x40]
af4: a94357f6 ldp x22, x21, [sp, #0x30]
af8: a9427bfd ldp x29, x30, [sp, #0x20]
afc: 910143ff add sp, sp, #0x50
b00: d50323bf autiasp
b04: d65f03c0 ret
; ret = driver_sysfs_add(dev);
b08: aa1403e0 mov x0, x20
b0c: 97ffff21 bl 0x790 <driver_sysfs_add>
; if (!ret) {
b10: 340006c0 cbz w0, 0xbe8 <__device_attach+0x180>
; bus_notify(dev, BUS_NOTIFY_DRIVER_NOT_BOUND);
b14: aa1403e0 mov x0, x20
b18: 528000e1 mov w1, #0x7
b1c: 94000000 bl 0xb1c <__device_attach+0xb4>
0000000000000b1c: R_AARCH64_CALL26 bus_notify
b20: 2a1f03f5 mov w21, wzr
; dev->driver = NULL;
b24: f900369f str xzr, [x20, #0x68]
b28: 17ffffea b 0xad0 <__device_attach+0x68>
b2c: 120002c8 and w8, w22, #0x1
; if (dev->parent)
b30: f9402280 ldr x0, [x20, #0x40]
; struct device_attach_data data = {
b34: a900fff4 stp x20, xzr, [sp, #0x8]
b38: 39004bff strb wzr, [sp, #0x12]
b3c: 390043e8 strb w8, [sp, #0x10]
; if (dev->parent)
b40: b4000060 cbz x0, 0xb4c <__device_attach+0xe4>
; return __pm_runtime_resume(dev, RPM_GET_PUT);
b44: 52800081 mov w1, #0x4
b48: 94000000 bl 0xb48 <__device_attach+0xe0>
0000000000000b48: R_AARCH64_CALL26 __pm_runtime_resume
; ret = bus_for_each_drv(dev->bus, NULL, &data,
b4c: f9403280 ldr x0, [x20, #0x60]
b50: 90000003 adrp x3, 0x0 <driver_deferred_probe_add>
0000000000000b50: R_AARCH64_ADR_PREL_PG_HI21 .text+0x17ac
b54: 91000063 add x3, x3, #0x0
0000000000000b54: R_AARCH64_ADD_ABS_LO12_NC .text+0x17ac
b58: 910023e2 add x2, sp, #0x8
b5c: aa1f03e1 mov x1, xzr
b60: 94000000 bl 0xb60 <__device_attach+0xf8>
0000000000000b60: R_AARCH64_CALL26 bus_for_each_drv
b64: 39404be8 ldrb w8, [sp, #0x12]
; if (!ret && allow_async && data.have_async) {
b68: 7100001f cmp w0, #0x0
b6c: 1a9f07e9 cset w9, ne
; ret = bus_for_each_drv(dev->bus, NULL, &data,
b70: 2a0003f5 mov w21, w0
b74: 7100011f cmp w8, #0x0
; if (!ret && allow_async && data.have_async) {
b78: 520002c8 eor w8, w22, #0x1
b7c: 1a9f17ea cset w10, eq
b80: 2a0a0108 orr w8, w8, w10
b84: 2a080136 orr w22, w9, w8
b88: 360000f6 tbz w22, #0x0, 0xba4 <__device_attach+0x13c>
; return __pm_runtime_idle(dev, RPM_ASYNC);
b8c: aa1403e0 mov x0, x20
b90: 52800021 mov w1, #0x1
b94: 94000000 bl 0xb94 <__device_attach+0x12c>
0000000000000b94: R_AARCH64_CALL26 __pm_runtime_idle
; if (dev->parent)
b98: f9402280 ldr x0, [x20, #0x40]
b9c: b50000e0 cbnz x0, 0xbb8 <__device_attach+0x150>
ba0: 14000008 b 0xbc0 <__device_attach+0x158>
; asm_volatile_goto(
ba4: d503201f nop
Hi Vladimir
On Tue, Dec 05, 2023 at 01:13:51PM +0200, Vladimir Oltean wrote:
> On Tue, Dec 05, 2023 at 01:35:31PM +0300, Serge Semin wrote:
> > @@ -1436,21 +1480,32 @@ static struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
> > struct dw_xpcs *xpcs;
> > int ret;
> >
> > + ret = device_attach(&mdiodev->dev);
> > + if (ret < 0 && ret != -ENODEV)
> > + return ERR_PTR(ret);
> > +
> > xpcs = xpcs_create_data(mdiodev);
> > if (IS_ERR(xpcs))
> > return xpcs;
> >
> > + ret = xpcs_init_clks(xpcs);
> > + if (ret)
> > + goto out_free_data;
> > +
> > ret = xpcs_init_id(xpcs);
> > if (ret)
> > - goto out;
> > + goto out_clear_clks;
> >
> > ret = xpcs_init_iface(xpcs, interface);
> > if (ret)
> > - goto out;
> > + goto out_clear_clks;
> >
> > return xpcs;
>
> [ 4.083518] Unable to handle kernel NULL pointer dereference at virtual address 00000000000000d0
> [ 4.092356] Mem abort info:
> [ 4.095164] ESR = 0x0000000096000004
> [ 4.098932] EC = 0x25: DABT (current EL), IL = 32 bits
> [ 4.104277] SET = 0, FnV = 0
> [ 4.107352] EA = 0, S1PTW = 0
> [ 4.110505] FSC = 0x04: level 0 translation fault
> [ 4.115408] Data abort info:
> [ 4.118296] ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
> [ 4.123807] CM = 0, WnR = 0, TnD = 0, TagAccess = 0
> [ 4.128877] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
> [ 4.134214] [00000000000000d0] user address but active_mm is swapper
> [ 4.140595] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
> [ 4.146882] Modules linked in:
> [ 4.149944] CPU: 0 PID: 11 Comm: kworker/u4:0 Not tainted 6.7.0-rc3-00719-g75be5ea8e111-dirty #1551
> [ 4.164524] Workqueue: events_unbound deferred_probe_work_func
> [ 4.177372] pc : __device_attach+0x3c/0x1bc
> [ 4.181570] lr : __device_attach+0x38/0x1bc
> [ 4.185767] sp : ffff8000800f3800
> [ 4.189087] x29: ffff8000800f3820 x28: 0000000000000001 x27: ffff063781bda150
> [ 4.196252] x26: ffff063781bda150 x25: ffff063780827480 x24: ffffcb9a08138a40
> [ 4.203416] x23: ffff063781114080 x22: 0000000000000000 x21: 0000000000000004
> [ 4.210579] x20: ffff06378123a400 x19: ffff06378123a480 x18: ffffcb9a07c703a0
> [ 4.217743] x17: ffffcb9a07c703a4 x16: 00000000000000d4 x15: ffffcb9a07be70fc
> [ 4.224906] x14: ffffcb9a08299638 x13: 0000000000000053 x12: ffff003000000200
> [ 4.232069] x11: 0000000000000000 x10: 0000000000000000 x9 : 0000000000000000
> [ 4.239233] x8 : 0000000000000000 x7 : 0000000000000000 x6 : 000000000000003a
> [ 4.246396] x5 : ffff0637809a037b x4 : ffffcb9a087b0d47 x3 : ffff10300000020f
> [ 4.253560] x2 : ffffcb9a0910c561 x1 : 0000000000000000 x0 : ffff06378123a480
> [ 4.260724] Call trace:
> [ 4.263172] __device_attach+0x3c/0x1bc
> [ 4.267020] device_attach+0x14/0x20
> [ 4.270606] xpcs_create+0x24/0x384
> [ 4.274107] xpcs_create_byaddr+0x74/0xa0
> [ 4.278129] sja1105_mdiobus_register+0xf8/0x478
> [ 4.282763] sja1105_setup+0xb4/0x1194
> [ 4.286524] dsa_register_switch+0xab0/0x11f8
> [ 4.290895] sja1105_probe+0x2bc/0x2e4
> [ 4.294654] spi_probe+0xa4/0xc4
> [ 4.297890] really_probe+0x16c/0x3fc
> [ 4.301564] __driver_probe_device+0xa4/0x168
> [ 4.305935] driver_probe_device+0x3c/0x220
> [ 4.310131] __device_attach_driver+0x128/0x1cc
> [ 4.314676] bus_for_each_drv+0xf4/0x14c
> [ 4.318610] __device_attach+0xfc/0x1bc
> [ 4.322457] device_initial_probe+0x14/0x20
> [ 4.326654] bus_probe_device+0x94/0x100
> [ 4.330587] deferred_probe_work_func+0xa0/0xfc
> [ 4.335132] process_scheduled_works+0x210/0x318
> [ 4.339764] worker_thread+0x28c/0x450
> [ 4.343523] kthread+0xfc/0x184
> [ 4.346669] ret_from_fork+0x10/0x20
> [ 4.350256] Code: 2a0103f6 f81f83a8 9431ccd8 f9402688 (39434109)
> [ 4.356366] ---[ end trace 0000000000000000 ]---
>
> I haven't looked at the code at all, but disassembling drivers/base/dd.lst,
> I think the NPD is at dev->p->dead (0xa68 + 0x3c = 0xaa4).
>
> 0000000000000a68 <__device_attach>:
> ; {
> a68: d503233f paciasp
> a6c: d10143ff sub sp, sp, #0x50
> a70: a9027bfd stp x29, x30, [sp, #0x20]
> a74: a90357f6 stp x22, x21, [sp, #0x30]
> a78: a9044ff4 stp x20, x19, [sp, #0x40]
> a7c: 910083fd add x29, sp, #0x20
> a80: d5384108 mrs x8, SP_EL0
> ; mutex_lock(&dev->mutex);
> a84: 91020013 add x19, x0, #0x80
> a88: f9423508 ldr x8, [x8, #0x468]
> a8c: aa0003f4 mov x20, x0
> a90: aa1303e0 mov x0, x19
> a94: 2a0103f6 mov w22, w1
> a98: f81f83a8 stur x8, [x29, #-0x8]
> a9c: 94000000 bl 0xa9c <__device_attach+0x34>
> 0000000000000a9c: R_AARCH64_CALL26 mutex_lock
> ; if (dev->p->dead) {
> aa0: f9402688 ldr x8, [x20, #0x48]
> aa4: 39434109 ldrb w9, [x8, #0xd0]
> aa8: 37000129 tbnz w9, #0x0, 0xacc <__device_attach+0x64>
> ; } else if (dev->driver) {
> aac: f9403689 ldr x9, [x20, #0x68]
> ab0: b40003e9 cbz x9, 0xb2c <__device_attach+0xc4>
> ; return dev->p && klist_node_attached(&dev->p->knode_driver);
> ab4: b40002a8 cbz x8, 0xb08 <__device_attach+0xa0>
> ab8: 91012100 add x0, x8, #0x48
> abc: 94000000 bl 0xabc <__device_attach+0x54>
> 0000000000000abc: R_AARCH64_CALL26 klist_node_attached
> ; if (device_is_bound(dev)) {
> ac0: 34000240 cbz w0, 0xb08 <__device_attach+0xa0>
> ac4: 52800035 mov w21, #0x1
> ac8: 14000002 b 0xad0 <__device_attach+0x68>
> acc: 2a1f03f5 mov w21, wzr
> ; mutex_unlock(&dev->mutex);
> ad0: aa1303e0 mov x0, x19
> ad4: 94000000 bl 0xad4 <__device_attach+0x6c>
> 0000000000000ad4: R_AARCH64_CALL26 mutex_unlock
> ad8: d5384108 mrs x8, SP_EL0
> adc: f9423508 ldr x8, [x8, #0x468]
> ae0: f85f83a9 ldur x9, [x29, #-0x8]
> ae4: eb09011f cmp x8, x9
> ae8: 540008c1 b.ne 0xc00 <__device_attach+0x198>
> ; return ret;
> aec: 2a1503e0 mov w0, w21
> af0: a9444ff4 ldp x20, x19, [sp, #0x40]
> af4: a94357f6 ldp x22, x21, [sp, #0x30]
> af8: a9427bfd ldp x29, x30, [sp, #0x20]
> afc: 910143ff add sp, sp, #0x50
> b00: d50323bf autiasp
> b04: d65f03c0 ret
> ; ret = driver_sysfs_add(dev);
> b08: aa1403e0 mov x0, x20
> b0c: 97ffff21 bl 0x790 <driver_sysfs_add>
> ; if (!ret) {
> b10: 340006c0 cbz w0, 0xbe8 <__device_attach+0x180>
> ; bus_notify(dev, BUS_NOTIFY_DRIVER_NOT_BOUND);
> b14: aa1403e0 mov x0, x20
> b18: 528000e1 mov w1, #0x7
> b1c: 94000000 bl 0xb1c <__device_attach+0xb4>
> 0000000000000b1c: R_AARCH64_CALL26 bus_notify
> b20: 2a1f03f5 mov w21, wzr
> ; dev->driver = NULL;
> b24: f900369f str xzr, [x20, #0x68]
> b28: 17ffffea b 0xad0 <__device_attach+0x68>
> b2c: 120002c8 and w8, w22, #0x1
> ; if (dev->parent)
> b30: f9402280 ldr x0, [x20, #0x40]
> ; struct device_attach_data data = {
> b34: a900fff4 stp x20, xzr, [sp, #0x8]
> b38: 39004bff strb wzr, [sp, #0x12]
> b3c: 390043e8 strb w8, [sp, #0x10]
> ; if (dev->parent)
> b40: b4000060 cbz x0, 0xb4c <__device_attach+0xe4>
> ; return __pm_runtime_resume(dev, RPM_GET_PUT);
> b44: 52800081 mov w1, #0x4
> b48: 94000000 bl 0xb48 <__device_attach+0xe0>
> 0000000000000b48: R_AARCH64_CALL26 __pm_runtime_resume
> ; ret = bus_for_each_drv(dev->bus, NULL, &data,
> b4c: f9403280 ldr x0, [x20, #0x60]
> b50: 90000003 adrp x3, 0x0 <driver_deferred_probe_add>
> 0000000000000b50: R_AARCH64_ADR_PREL_PG_HI21 .text+0x17ac
> b54: 91000063 add x3, x3, #0x0
> 0000000000000b54: R_AARCH64_ADD_ABS_LO12_NC .text+0x17ac
> b58: 910023e2 add x2, sp, #0x8
> b5c: aa1f03e1 mov x1, xzr
> b60: 94000000 bl 0xb60 <__device_attach+0xf8>
> 0000000000000b60: R_AARCH64_CALL26 bus_for_each_drv
> b64: 39404be8 ldrb w8, [sp, #0x12]
> ; if (!ret && allow_async && data.have_async) {
> b68: 7100001f cmp w0, #0x0
> b6c: 1a9f07e9 cset w9, ne
> ; ret = bus_for_each_drv(dev->bus, NULL, &data,
> b70: 2a0003f5 mov w21, w0
> b74: 7100011f cmp w8, #0x0
> ; if (!ret && allow_async && data.have_async) {
> b78: 520002c8 eor w8, w22, #0x1
> b7c: 1a9f17ea cset w10, eq
> b80: 2a0a0108 orr w8, w8, w10
> b84: 2a080136 orr w22, w9, w8
> b88: 360000f6 tbz w22, #0x0, 0xba4 <__device_attach+0x13c>
> ; return __pm_runtime_idle(dev, RPM_ASYNC);
> b8c: aa1403e0 mov x0, x20
> b90: 52800021 mov w1, #0x1
> b94: 94000000 bl 0xb94 <__device_attach+0x12c>
> 0000000000000b94: R_AARCH64_CALL26 __pm_runtime_idle
> ; if (dev->parent)
> b98: f9402280 ldr x0, [x20, #0x40]
> b9c: b50000e0 cbnz x0, 0xbb8 <__device_attach+0x150>
> ba0: 14000008 b 0xbc0 <__device_attach+0x158>
> ; asm_volatile_goto(
> ba4: d503201f nop
Omg, thank you very much for testing the series straight away and
sorry for the immediate trouble it caused. I'll need some more time
for investigation. I'll get back to this topic a bit later on this
week.
-Serge(y)
On Tue, Dec 05, 2023 at 02:35:46PM +0300, Serge Semin wrote:
> Omg, thank you very much for testing the series straight away and
> sorry for the immediate trouble it caused. I'll need some more time
> for investigation. I'll get back to this topic a bit later on this
> week.
Don't worry, I got suspicious when I was CCed to review only a one-line
change in patch 11/16. It's never about that one line, is it?)
Anyway, the NULL dev->p is a symptom of device_add() not having been
called, most likely from mdio_device_register().
I'll be honest and say that I still don't quite understand what you're
trying to achieve. You're trying to bind the hardcoded mdio_devices
created by xpcs_create() to a driver? That was attempted a while ago by
Sean Anderson with the Lynx PCS. Are you aware of the fact that even in
the good case in which binding the driver actually works, the user can
then come along and unbind it from the PCS device, and phylink isn't
prepared to handle that, so it will crash the kernel upon the next
phylink_pcs call?
The pcs-rzn1-miic.c driver puts a device_link to the MAC to at least
tear down the whole thing when the PCS is unbound, which is saner than
crashing the kernel. I don't see the equivalent protection mechanism here?
Can't the xpcs continue to live without a bound driver? Having a
compatible string in the OF description is perfectly fine though,
and should absolutely not preclude that.
Hi Serge,
On Tue, 5 Dec 2023 13:35:30 +0300
Serge Semin <[email protected]> wrote:
> Synopsys DesignWare XPCS IP-core can be synthesized with the device CSRs
> being accessible over MCI or APB3 interface instead of the MDIO bus (see
> the CSR_INTERFACE HDL parameter). Thus all the PCS registers can be just
> memory mapped and be a subject of standard MMIO operations of course
> taking into account the way the Clause C45 CSRs mapping is defined. This
> commit is about adding a device driver for the DW XPCS Management
> Interface platform device and registering it in the framework of the
> kernel MDIO subsystem.
>
> DW XPCS platform device is supposed to be described by the respective
> compatible string "snps,dw-xpcs-mi", CSRs memory space and optional
> peripheral bus clock source. Note depending on the INDIRECT_ACCESS DW XPCS
> IP-core synthesize parameter the memory-mapped reg-space can be
> represented as either directly or indirectly mapped Clause 45 space. In
> the former case the particular address is determined based on the MMD
> device and the registers offset (5 + 16 bits all together) within the
> device reg-space. In the later case there is only 256 lower address bits
> are utilized for the registers mapping. The upper bits are supposed to be
> written into the respective viewport CSR in order to reach the entire C45
> space.
Too bad the mdio-regmap driver can't be re-used here, it would deal
with reg width for you, for example. I guess the main reason would be
the direct vs indirect accesses ?
I do have a comment tough :
[...]
> +static inline ptrdiff_t dw_xpcs_mmio_addr_format(int dev, int reg)
> +{
> + return FIELD_PREP(0x1f0000, dev) | FIELD_PREP(0xffff, reg);
> +}
> +
> +static inline u16 dw_xpcs_mmio_addr_page(ptrdiff_t csr)
> +{
> + return FIELD_GET(0x1fff00, csr);
> +}
> +
> +static inline ptrdiff_t dw_xpcs_mmio_addr_offset(ptrdiff_t csr)
> +{
> + return FIELD_GET(0xff, csr);
> +}
You shouldn't use inline in C files, only in headers.
Maxime
On Tue, Dec 05, 2023 at 01:35:23PM +0300, Serge Semin wrote:
> There is nothing CM workqueue-related in the driver. So the respective
> include directive can be dropped.
>
> While at it add an empty line delimiter between the generic and local path
> include directives.
>
> Signed-off-by: Serge Semin <[email protected]>
Tested-by: Andrew Lunn <[email protected]>
Andrew
On Tue, Dec 05, 2023 at 01:35:24PM +0300, Serge Semin wrote:
> In particular the xpcs_soft_reset() and xpcs_do_config() functions
> currently return -1 if invalid auto-negotiation mode is specified. That
> value can be then passed to the generic kernel subsystems which require a
> standard kernel errno value. Even though the error conditions are very
> specific (memory corruption or buggy implementation) using a hard-coded -1
> literal doesn't seem correct anyway.
>
> Signed-off-by: Serge Semin <[email protected]>
Tested-by: Andrew Lunn <[email protected]>
Andrew
Hi Serge,
kernel test robot noticed the following build errors:
[auto build test ERROR on net-next/main]
url: https://github.com/intel-lab-lkp/linux/commits/Serge-Semin/net-pcs-xpcs-Drop-sentinel-entry-from-2500basex-ifaces-list/20231205-183808
base: net-next/main
patch link: https://lore.kernel.org/r/20231205103559.9605-12-fancer.lancer%40gmail.com
patch subject: [PATCH net-next 11/16] net: pcs: xpcs: Change xpcs_create_mdiodev() suffix to "byaddr"
config: arc-randconfig-001-20231206 (https://download.01.org/0day-ci/archive/20231206/[email protected]/config)
compiler: arc-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231206/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All error/warnings (new ones prefixed by >>):
drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c: In function 'txgbe_mdio_pcs_init':
>> drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c:150:16: error: implicit declaration of function 'xpcs_create_mdiodev'; did you mean 'xpcs_create_byaddr'? [-Werror=implicit-function-declaration]
150 | xpcs = xpcs_create_mdiodev(mii_bus, 0, PHY_INTERFACE_MODE_10GBASER);
| ^~~~~~~~~~~~~~~~~~~
| xpcs_create_byaddr
>> drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c:150:14: warning: assignment to 'struct dw_xpcs *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
150 | xpcs = xpcs_create_mdiodev(mii_bus, 0, PHY_INTERFACE_MODE_10GBASER);
| ^
cc1: some warnings being treated as errors
vim +150 drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
854cace61387b6 Jiawen Wu 2023-06-06 121
854cace61387b6 Jiawen Wu 2023-06-06 122 static int txgbe_mdio_pcs_init(struct txgbe *txgbe)
854cace61387b6 Jiawen Wu 2023-06-06 123 {
854cace61387b6 Jiawen Wu 2023-06-06 124 struct mii_bus *mii_bus;
854cace61387b6 Jiawen Wu 2023-06-06 125 struct dw_xpcs *xpcs;
854cace61387b6 Jiawen Wu 2023-06-06 126 struct pci_dev *pdev;
854cace61387b6 Jiawen Wu 2023-06-06 127 struct wx *wx;
854cace61387b6 Jiawen Wu 2023-06-06 128 int ret = 0;
854cace61387b6 Jiawen Wu 2023-06-06 129
854cace61387b6 Jiawen Wu 2023-06-06 130 wx = txgbe->wx;
854cace61387b6 Jiawen Wu 2023-06-06 131 pdev = wx->pdev;
854cace61387b6 Jiawen Wu 2023-06-06 132
854cace61387b6 Jiawen Wu 2023-06-06 133 mii_bus = devm_mdiobus_alloc(&pdev->dev);
854cace61387b6 Jiawen Wu 2023-06-06 134 if (!mii_bus)
854cace61387b6 Jiawen Wu 2023-06-06 135 return -ENOMEM;
854cace61387b6 Jiawen Wu 2023-06-06 136
854cace61387b6 Jiawen Wu 2023-06-06 137 mii_bus->name = "txgbe_pcs_mdio_bus";
854cace61387b6 Jiawen Wu 2023-06-06 138 mii_bus->read_c45 = &txgbe_pcs_read;
854cace61387b6 Jiawen Wu 2023-06-06 139 mii_bus->write_c45 = &txgbe_pcs_write;
854cace61387b6 Jiawen Wu 2023-06-06 140 mii_bus->parent = &pdev->dev;
854cace61387b6 Jiawen Wu 2023-06-06 141 mii_bus->phy_mask = ~0;
854cace61387b6 Jiawen Wu 2023-06-06 142 mii_bus->priv = wx;
854cace61387b6 Jiawen Wu 2023-06-06 143 snprintf(mii_bus->id, MII_BUS_ID_SIZE, "txgbe_pcs-%x",
d8c21ef7b2b147 Xiongfeng Wang 2023-08-08 144 pci_dev_id(pdev));
854cace61387b6 Jiawen Wu 2023-06-06 145
854cace61387b6 Jiawen Wu 2023-06-06 146 ret = devm_mdiobus_register(&pdev->dev, mii_bus);
854cace61387b6 Jiawen Wu 2023-06-06 147 if (ret)
854cace61387b6 Jiawen Wu 2023-06-06 148 return ret;
854cace61387b6 Jiawen Wu 2023-06-06 149
854cace61387b6 Jiawen Wu 2023-06-06 @150 xpcs = xpcs_create_mdiodev(mii_bus, 0, PHY_INTERFACE_MODE_10GBASER);
854cace61387b6 Jiawen Wu 2023-06-06 151 if (IS_ERR(xpcs))
854cace61387b6 Jiawen Wu 2023-06-06 152 return PTR_ERR(xpcs);
854cace61387b6 Jiawen Wu 2023-06-06 153
854cace61387b6 Jiawen Wu 2023-06-06 154 txgbe->xpcs = xpcs;
854cace61387b6 Jiawen Wu 2023-06-06 155
854cace61387b6 Jiawen Wu 2023-06-06 156 return 0;
854cace61387b6 Jiawen Wu 2023-06-06 157 }
854cace61387b6 Jiawen Wu 2023-06-06 158
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Serge,
kernel test robot noticed the following build errors:
[auto build test ERROR on net-next/main]
url: https://github.com/intel-lab-lkp/linux/commits/Serge-Semin/net-pcs-xpcs-Drop-sentinel-entry-from-2500basex-ifaces-list/20231205-183808
base: net-next/main
patch link: https://lore.kernel.org/r/20231205103559.9605-14-fancer.lancer%40gmail.com
patch subject: [PATCH net-next 13/16] net: stmmac: intel: Register generic MDIO device
config: x86_64-kexec (https://download.01.org/0day-ci/archive/20231206/[email protected]/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231206/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c: In function 'intel_mgbe_common_data':
>> drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c:646:31: error: 'clk' undeclared (first use in this function)
646 | clk_disable_unprepare(clk);
| ^~~
drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c:646:31: note: each undeclared identifier is reported only once for each function it appears in
vim +/clk +646 drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
446
447 static int intel_mgbe_common_data(struct pci_dev *pdev,
448 struct plat_stmmacenet_data *plat)
449 {
450 struct fwnode_handle *fwnode;
451 char clk_name[20];
452 int ret;
453 int i;
454
455 plat->pdev = pdev;
456 plat->phy_addr = -1;
457 plat->clk_csr = 5;
458 plat->has_gmac = 0;
459 plat->has_gmac4 = 1;
460 plat->force_sf_dma_mode = 0;
461 plat->flags |= (STMMAC_FLAG_TSO_EN | STMMAC_FLAG_SPH_DISABLE);
462
463 /* Multiplying factor to the clk_eee_i clock time
464 * period to make it closer to 100 ns. This value
465 * should be programmed such that the clk_eee_time_period *
466 * (MULT_FACT_100NS + 1) should be within 80 ns to 120 ns
467 * clk_eee frequency is 19.2Mhz
468 * clk_eee_time_period is 52ns
469 * 52ns * (1 + 1) = 104ns
470 * MULT_FACT_100NS = 1
471 */
472 plat->mult_fact_100ns = 1;
473
474 plat->rx_sched_algorithm = MTL_RX_ALGORITHM_SP;
475
476 for (i = 0; i < plat->rx_queues_to_use; i++) {
477 plat->rx_queues_cfg[i].mode_to_use = MTL_QUEUE_DCB;
478 plat->rx_queues_cfg[i].chan = i;
479
480 /* Disable Priority config by default */
481 plat->rx_queues_cfg[i].use_prio = false;
482
483 /* Disable RX queues routing by default */
484 plat->rx_queues_cfg[i].pkt_route = 0x0;
485 }
486
487 for (i = 0; i < plat->tx_queues_to_use; i++) {
488 plat->tx_queues_cfg[i].mode_to_use = MTL_QUEUE_DCB;
489
490 /* Disable Priority config by default */
491 plat->tx_queues_cfg[i].use_prio = false;
492 /* Default TX Q0 to use TSO and rest TXQ for TBS */
493 if (i > 0)
494 plat->tx_queues_cfg[i].tbs_en = 1;
495 }
496
497 /* FIFO size is 4096 bytes for 1 tx/rx queue */
498 plat->tx_fifo_size = plat->tx_queues_to_use * 4096;
499 plat->rx_fifo_size = plat->rx_queues_to_use * 4096;
500
501 plat->tx_sched_algorithm = MTL_TX_ALGORITHM_WRR;
502 plat->tx_queues_cfg[0].weight = 0x09;
503 plat->tx_queues_cfg[1].weight = 0x0A;
504 plat->tx_queues_cfg[2].weight = 0x0B;
505 plat->tx_queues_cfg[3].weight = 0x0C;
506 plat->tx_queues_cfg[4].weight = 0x0D;
507 plat->tx_queues_cfg[5].weight = 0x0E;
508 plat->tx_queues_cfg[6].weight = 0x0F;
509 plat->tx_queues_cfg[7].weight = 0x10;
510
511 plat->dma_cfg->pbl = 32;
512 plat->dma_cfg->pblx8 = true;
513 plat->dma_cfg->fixed_burst = 0;
514 plat->dma_cfg->mixed_burst = 0;
515 plat->dma_cfg->aal = 0;
516 plat->dma_cfg->dche = true;
517
518 plat->axi = devm_kzalloc(&pdev->dev, sizeof(*plat->axi),
519 GFP_KERNEL);
520 if (!plat->axi)
521 return -ENOMEM;
522
523 plat->axi->axi_lpi_en = 0;
524 plat->axi->axi_xit_frm = 0;
525 plat->axi->axi_wr_osr_lmt = 1;
526 plat->axi->axi_rd_osr_lmt = 1;
527 plat->axi->axi_blen[0] = 4;
528 plat->axi->axi_blen[1] = 8;
529 plat->axi->axi_blen[2] = 16;
530
531 plat->ptp_max_adj = plat->clk_ptp_rate;
532 plat->eee_usecs_rate = plat->clk_ptp_rate;
533
534 /* Set system clock */
535 sprintf(clk_name, "%s-%s", "stmmac", pci_name(pdev));
536
537 plat->stmmac_clk = clk_register_fixed_rate(&pdev->dev,
538 clk_name, NULL, 0,
539 plat->clk_ptp_rate);
540
541 if (IS_ERR(plat->stmmac_clk)) {
542 dev_warn(&pdev->dev, "Fail to register stmmac-clk\n");
543 plat->stmmac_clk = NULL;
544 }
545
546 ret = clk_prepare_enable(plat->stmmac_clk);
547 if (ret) {
548 clk_unregister_fixed_rate(plat->stmmac_clk);
549 return ret;
550 }
551
552 plat->ptp_clk_freq_config = intel_mgbe_ptp_clk_freq_config;
553
554 /* Set default value for multicast hash bins */
555 plat->multicast_filter_bins = HASH_TABLE_SIZE;
556
557 /* Set default value for unicast filter entries */
558 plat->unicast_filter_entries = 1;
559
560 /* Set the maxmtu to a default of JUMBO_LEN */
561 plat->maxmtu = JUMBO_LEN;
562
563 plat->flags |= STMMAC_FLAG_VLAN_FAIL_Q_EN;
564
565 /* Use the last Rx queue */
566 plat->vlan_fail_q = plat->rx_queues_to_use - 1;
567
568 /* For fixed-link setup, we allow phy-mode setting */
569 fwnode = dev_fwnode(&pdev->dev);
570 if (fwnode) {
571 int phy_mode;
572
573 /* "phy-mode" setting is optional. If it is set,
574 * we allow either sgmii or 1000base-x for now.
575 */
576 phy_mode = fwnode_get_phy_mode(fwnode);
577 if (phy_mode >= 0) {
578 if (phy_mode == PHY_INTERFACE_MODE_SGMII ||
579 phy_mode == PHY_INTERFACE_MODE_1000BASEX)
580 plat->phy_interface = phy_mode;
581 else
582 dev_warn(&pdev->dev, "Invalid phy-mode\n");
583 }
584 }
585
586 /* Intel mgbe SGMII interface uses pcs-xcps */
587 if (plat->phy_interface == PHY_INTERFACE_MODE_SGMII ||
588 plat->phy_interface == PHY_INTERFACE_MODE_1000BASEX) {
589 struct mdio_board_info *xpcs_info;
590
591 xpcs_info = devm_kzalloc(&pdev->dev,
592 sizeof(*xpcs_info) + MII_BUS_ID_SIZE,
593 GFP_KERNEL);
594 if (!xpcs_info) {
595 ret = -ENOMEM;
596 goto err_alloc_info;
597 }
598
599 xpcs_info->bus_id = (void *)xpcs_info + sizeof(*xpcs_info);
600 snprintf((char *)xpcs_info->bus_id, MII_BUS_ID_SIZE,
601 "stmmac-%x", plat->bus_id);
602
603 snprintf(xpcs_info->modalias, MDIO_NAME_SIZE, "dwxpcs");
604
605 xpcs_info->mdio_addr = INTEL_MGBE_XPCS_ADDR;
606
607 ret = mdiobus_register_board_info(xpcs_info, 1);
608 if (ret)
609 goto err_alloc_info;
610
611 plat->mdio_bus_data->has_xpcs = true;
612 plat->mdio_bus_data->xpcs_an_inband = true;
613 }
614
615 /* For fixed-link setup, we clear xpcs_an_inband */
616 if (fwnode) {
617 struct fwnode_handle *fixed_node;
618
619 fixed_node = fwnode_get_named_child_node(fwnode, "fixed-link");
620 if (fixed_node)
621 plat->mdio_bus_data->xpcs_an_inband = false;
622
623 fwnode_handle_put(fixed_node);
624 }
625
626 /* Ensure mdio bus PHY-scan skips intel serdes and pcs-xpcs */
627 plat->mdio_bus_data->phy_mask = 1 << INTEL_MGBE_ADHOC_ADDR;
628 plat->mdio_bus_data->phy_mask |= 1 << INTEL_MGBE_XPCS_ADDR;
629
630 plat->int_snapshot_num = AUX_SNAPSHOT1;
631
632 plat->crosststamp = intel_crosststamp;
633 plat->flags &= ~STMMAC_FLAG_INT_SNAPSHOT_EN;
634
635 /* Setup MSI vector offset specific to Intel mGbE controller */
636 plat->msi_mac_vec = 29;
637 plat->msi_lpi_vec = 28;
638 plat->msi_sfty_ce_vec = 27;
639 plat->msi_sfty_ue_vec = 26;
640 plat->msi_rx_base_vec = 0;
641 plat->msi_tx_base_vec = 1;
642
643 return 0;
644
645 err_alloc_info:
> 646 clk_disable_unprepare(clk);
647 clk_unregister_fixed_rate(clk);
648
649 return ret;
650 }
651
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Serge,
kernel test robot noticed the following build warnings:
[auto build test WARNING on net-next/main]
url: https://github.com/intel-lab-lkp/linux/commits/Serge-Semin/net-pcs-xpcs-Drop-sentinel-entry-from-2500basex-ifaces-list/20231205-183808
base: net-next/main
patch link: https://lore.kernel.org/r/20231205103559.9605-12-fancer.lancer%40gmail.com
patch subject: [PATCH net-next 11/16] net: pcs: xpcs: Change xpcs_create_mdiodev() suffix to "byaddr"
config: x86_64-randconfig-006-20231206 (https://download.01.org/0day-ci/archive/20231206/[email protected]/config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231206/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c: In function 'txgbe_mdio_pcs_init':
drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c:150:9: error: implicit declaration of function 'xpcs_create_mdiodev'; did you mean 'xpcs_create_byaddr'? [-Werror=implicit-function-declaration]
xpcs = xpcs_create_mdiodev(mii_bus, 0, PHY_INTERFACE_MODE_10GBASER);
^~~~~~~~~~~~~~~~~~~
xpcs_create_byaddr
>> drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c:150:7: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
xpcs = xpcs_create_mdiodev(mii_bus, 0, PHY_INTERFACE_MODE_10GBASER);
^
cc1: some warnings being treated as errors
vim +150 drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
854cace61387b6 Jiawen Wu 2023-06-06 121
854cace61387b6 Jiawen Wu 2023-06-06 122 static int txgbe_mdio_pcs_init(struct txgbe *txgbe)
854cace61387b6 Jiawen Wu 2023-06-06 123 {
854cace61387b6 Jiawen Wu 2023-06-06 124 struct mii_bus *mii_bus;
854cace61387b6 Jiawen Wu 2023-06-06 125 struct dw_xpcs *xpcs;
854cace61387b6 Jiawen Wu 2023-06-06 126 struct pci_dev *pdev;
854cace61387b6 Jiawen Wu 2023-06-06 127 struct wx *wx;
854cace61387b6 Jiawen Wu 2023-06-06 128 int ret = 0;
854cace61387b6 Jiawen Wu 2023-06-06 129
854cace61387b6 Jiawen Wu 2023-06-06 130 wx = txgbe->wx;
854cace61387b6 Jiawen Wu 2023-06-06 131 pdev = wx->pdev;
854cace61387b6 Jiawen Wu 2023-06-06 132
854cace61387b6 Jiawen Wu 2023-06-06 133 mii_bus = devm_mdiobus_alloc(&pdev->dev);
854cace61387b6 Jiawen Wu 2023-06-06 134 if (!mii_bus)
854cace61387b6 Jiawen Wu 2023-06-06 135 return -ENOMEM;
854cace61387b6 Jiawen Wu 2023-06-06 136
854cace61387b6 Jiawen Wu 2023-06-06 137 mii_bus->name = "txgbe_pcs_mdio_bus";
854cace61387b6 Jiawen Wu 2023-06-06 138 mii_bus->read_c45 = &txgbe_pcs_read;
854cace61387b6 Jiawen Wu 2023-06-06 139 mii_bus->write_c45 = &txgbe_pcs_write;
854cace61387b6 Jiawen Wu 2023-06-06 140 mii_bus->parent = &pdev->dev;
854cace61387b6 Jiawen Wu 2023-06-06 141 mii_bus->phy_mask = ~0;
854cace61387b6 Jiawen Wu 2023-06-06 142 mii_bus->priv = wx;
854cace61387b6 Jiawen Wu 2023-06-06 143 snprintf(mii_bus->id, MII_BUS_ID_SIZE, "txgbe_pcs-%x",
d8c21ef7b2b147 Xiongfeng Wang 2023-08-08 144 pci_dev_id(pdev));
854cace61387b6 Jiawen Wu 2023-06-06 145
854cace61387b6 Jiawen Wu 2023-06-06 146 ret = devm_mdiobus_register(&pdev->dev, mii_bus);
854cace61387b6 Jiawen Wu 2023-06-06 147 if (ret)
854cace61387b6 Jiawen Wu 2023-06-06 148 return ret;
854cace61387b6 Jiawen Wu 2023-06-06 149
854cace61387b6 Jiawen Wu 2023-06-06 @150 xpcs = xpcs_create_mdiodev(mii_bus, 0, PHY_INTERFACE_MODE_10GBASER);
854cace61387b6 Jiawen Wu 2023-06-06 151 if (IS_ERR(xpcs))
854cace61387b6 Jiawen Wu 2023-06-06 152 return PTR_ERR(xpcs);
854cace61387b6 Jiawen Wu 2023-06-06 153
854cace61387b6 Jiawen Wu 2023-06-06 154 txgbe->xpcs = xpcs;
854cace61387b6 Jiawen Wu 2023-06-06 155
854cace61387b6 Jiawen Wu 2023-06-06 156 return 0;
854cace61387b6 Jiawen Wu 2023-06-06 157 }
854cace61387b6 Jiawen Wu 2023-06-06 158
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Maxime,
On Tue, Dec 05, 2023 at 01:32:05PM +0100, Maxime Chevallier wrote:
> Hi Serge,
>
> On Tue, 5 Dec 2023 13:35:30 +0300
> Serge Semin <[email protected]> wrote:
>
> > Synopsys DesignWare XPCS IP-core can be synthesized with the device CSRs
> > being accessible over MCI or APB3 interface instead of the MDIO bus (see
> > the CSR_INTERFACE HDL parameter). Thus all the PCS registers can be just
> > memory mapped and be a subject of standard MMIO operations of course
> > taking into account the way the Clause C45 CSRs mapping is defined. This
> > commit is about adding a device driver for the DW XPCS Management
> > Interface platform device and registering it in the framework of the
> > kernel MDIO subsystem.
> >
> > DW XPCS platform device is supposed to be described by the respective
> > compatible string "snps,dw-xpcs-mi", CSRs memory space and optional
> > peripheral bus clock source. Note depending on the INDIRECT_ACCESS DW XPCS
> > IP-core synthesize parameter the memory-mapped reg-space can be
> > represented as either directly or indirectly mapped Clause 45 space. In
> > the former case the particular address is determined based on the MMD
> > device and the registers offset (5 + 16 bits all together) within the
> > device reg-space. In the later case there is only 256 lower address bits
> > are utilized for the registers mapping. The upper bits are supposed to be
> > written into the respective viewport CSR in order to reach the entire C45
> > space.
>
> Too bad the mdio-regmap driver can't be re-used here, it would deal
> with reg width for you, for example. I guess the main reason would be
> the direct vs indirect accesses ?
Right, it's one of the reasons. I could have used the mdio-regmap
here, but that would have meant to implement an additional abstraction
layer: regspace<->regmap<->mdio-regmap<->mii_bus, instead of just
regspace<->mii_bus. This would have also required to patch the
mdio-remap module somehow in order to have the c45 ops supported. It
would have been much easier to do before the commit 99d5fe9c7f3d ("net:
mdio: Remove support for building C45 muxed addresses"). But since
then MDIO reg-address has no longer had muxed dev-address. Of course I
could have got it back to the mdio-regmap driver only, mix the C22/C45
address in the regmap 'addr' argument, then extract the MMD (for C45)
and reg addresses from the regmap IO ops argument and perform the
respective MMIO access. But as you can see it is much hardware and
would cause additional steps for the address translations, than
just directly implement the C22/C45 IO ops and register the MDIO bus
for them. I couldn't find much benefits to follow that road, sorry.
>
> I do have a comment tough :
>
> [...]
>
> > +static inline ptrdiff_t dw_xpcs_mmio_addr_format(int dev, int reg)
> > +{
> > + return FIELD_PREP(0x1f0000, dev) | FIELD_PREP(0xffff, reg);
> > +}
> > +
> > +static inline u16 dw_xpcs_mmio_addr_page(ptrdiff_t csr)
> > +{
> > + return FIELD_GET(0x1fff00, csr);
> > +}
> > +
> > +static inline ptrdiff_t dw_xpcs_mmio_addr_offset(ptrdiff_t csr)
> > +{
> > + return FIELD_GET(0xff, csr);
> > +}
>
> You shouldn't use inline in C files, only in headers.
Could you please clarify why? I failed to find such requirement in the
coding style doc. Moreover there are multiple examples of using the
static-inline-ers in the C files in the kernel including the net/mdio
subsystem.
-Serge(y)
>
> Maxime
> > You shouldn't use inline in C files, only in headers.
>
> Could you please clarify why? I failed to find such requirement in the
> coding style doc. Moreover there are multiple examples of using the
> static-inline-ers in the C files in the kernel including the net/mdio
> subsystem.
The compiler does a better job at deciding what to inline than we
humans do. If you can show the compiler is doing it wrong, then we
might accept them. But in general, netdev does not like inline in .C
file. Also, nothing in MDIO is hot path, it spends a lot of time
waiting for a slow bus. So inline is likely to just bloat the code for
no gain.
Andrew
Hi Andrew
On Wed, Dec 06, 2023 at 06:01:30PM +0100, Andrew Lunn wrote:
> > > You shouldn't use inline in C files, only in headers.
> >
> > Could you please clarify why? I failed to find such requirement in the
> > coding style doc. Moreover there are multiple examples of using the
> > static-inline-ers in the C files in the kernel including the net/mdio
> > subsystem.
>
> The compiler does a better job at deciding what to inline than we
> humans do. If you can show the compiler is doing it wrong, then we
> might accept them.
In general I would have agreed with you especially if the methods were
heavier than what they are:
static inline ptrdiff_t dw_xpcs_mmio_addr_format(int dev, int reg)
{
return FIELD_PREP(0x1f0000, dev) | FIELD_PREP(0xffff, reg);
}
static inline u16 dw_xpcs_mmio_addr_page(ptrdiff_t csr)
{
return FIELD_GET(0x1fff00, csr);
}
static inline ptrdiff_t dw_xpcs_mmio_addr_offset(ptrdiff_t csr)
{
return FIELD_GET(0xff, csr);
}
> But in general, netdev does not like inline in .C
> file.
I see. I'll do as you say if you don't change your mind after my
reasoning below.
> Also, nothing in MDIO is hot path, it spends a lot of time
> waiting for a slow bus. So inline is likely to just bloat the code for
> no gain.
I would have been absolutely with you in this matter, if we were talking
about a normal MDIO bus. In this case the devices are accessed over
the system IO-memory. So the bus isn't that slow.
Regarding the compiler knowing better when to inline the code. Here is
what it does with the methods above. If the inline keyword is
specified the compiler will inline all three methods. If the keyword isn't
specified then dw_xpcs_mmio_addr_format() won't be inlined while the rest
two functions will be. So the only part at consideration is the
dw_xpcs_mmio_addr_format() method since the rest of the functions are
inlined anyway.
The dw_xpcs_mmio_addr_format() function body is of the 5 asm
instructions length (on MIPS). Since the function call in this case
requires two jump instructions (to function and back), one instruction
to save the previous return address on stack and two instructions for
the function arguments, the trade-off of having non-inlined function
are those five additional instructions on each call. There are four
dw_xpcs_mmio_addr_format() calls. So here is what we get in both
cases:
inlined: 5 func ins * 4 calls = 20 ins
non-inlined: (5 func + 1 jump) ins + (1 jump + 1 ra + 2 arg) ins * 4 calls = 22 ins
but seeing the return address needs to be saved anyway in the callers
here is what we finally get:
non-inlined: (5 func + 1 jump) ins + (1 jump + 2 arg) ins * 4 calls = 18 ins
So unless I am mistaken in some of the aspects if we have the function
non-inlined then we'll save 2 instructions in the object file, but
each call would require additional _4_ instructions to execute (2
jumps and 2 arg creations), which makes the function execution almost
two times longer than it would have been should it was inlined. IMO in
this case saving 2 instructions of the object file isn't worth than
getting rid from four instructions on each call seeing the DW XPCS
MCI/APB3 buses are the memory IO interfaces which don't require any
long-time waits to perform the ops. Thus I'd suggest to keep the
inline keywords specified here.
What is your conclusion?
-Serge(y)
>
> Andrew
On Thu, Dec 07, 2023 at 04:35:47PM +0300, Serge Semin wrote:
> Hi Andrew
>
> On Wed, Dec 06, 2023 at 06:01:30PM +0100, Andrew Lunn wrote:
> > The compiler does a better job at deciding what to inline than we
> > humans do. If you can show the compiler is doing it wrong, then we
> > might accept them.
>
> In general I would have agreed with you especially if the methods were
> heavier than what they are:
> static inline ptrdiff_t dw_xpcs_mmio_addr_format(int dev, int reg)
> {
> return FIELD_PREP(0x1f0000, dev) | FIELD_PREP(0xffff, reg);
> }
>
> static inline u16 dw_xpcs_mmio_addr_page(ptrdiff_t csr)
> {
> return FIELD_GET(0x1fff00, csr);
> }
>
> static inline ptrdiff_t dw_xpcs_mmio_addr_offset(ptrdiff_t csr)
> {
> return FIELD_GET(0xff, csr);
> }
>
> > But in general, netdev does not like inline in .C
> > file.
>
> I see. I'll do as you say if you don't change your mind after my
> reasoning below.
This isn't Andrew saying it - you seem to have missed the detail that
"netdev". If Andrew doesn't say it, then DaveM, Jakub or Paolo will.
Have you read the "Inline functions" section in
Documentation/process/4.Coding.rst ?
> > Also, nothing in MDIO is hot path, it spends a lot of time
> > waiting for a slow bus. So inline is likely to just bloat the code for
> > no gain.
>
> I would have been absolutely with you in this matter, if we were talking
> about a normal MDIO bus. In this case the devices are accessed over
> the system IO-memory. So the bus isn't that slow.
>
> Regarding the compiler knowing better when to inline the code. Here is
> what it does with the methods above. If the inline keyword is
> specified the compiler will inline all three methods. If the keyword isn't
> specified then dw_xpcs_mmio_addr_format() won't be inlined while the rest
> two functions will be. So the only part at consideration is the
> dw_xpcs_mmio_addr_format() method since the rest of the functions are
> inlined anyway.
>
> The dw_xpcs_mmio_addr_format() function body is of the 5 asm
> instructions length (on MIPS). Since the function call in this case
> requires two jump instructions (to function and back), one instruction
> to save the previous return address on stack and two instructions for
> the function arguments, the trade-off of having non-inlined function
> are those five additional instructions on each call. There are four
> dw_xpcs_mmio_addr_format() calls. So here is what we get in both
> cases:
> inlined: 5 func ins * 4 calls = 20 ins
> non-inlined: (5 func + 1 jump) ins + (1 jump + 1 ra + 2 arg) ins * 4 calls = 22 ins
> but seeing the return address needs to be saved anyway in the callers
> here is what we finally get:
> non-inlined: (5 func + 1 jump) ins + (1 jump + 2 arg) ins * 4 calls = 18 ins
>
> So unless I am mistaken in some of the aspects if we have the function
> non-inlined then we'll save 2 instructions in the object file, but
> each call would require additional _4_ instructions to execute (2
> jumps and 2 arg creations), which makes the function execution almost
> two times longer than it would have been should it was inlined.
Rather than just focusing on instruction count, you also need to
consider things like branch prediction, prefetching and I-cache
usage. Modern CPUs don't execute instruction-by-instruction anymore.
It is entirely possible that the compiler is making better choices
even if it results in more jumps in the code.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Wed, Dec 06, 2023 at 07:03:46AM +0800, kernel test robot wrote:
> Hi Serge,
>
> kernel test robot noticed the following build errors:
>
> [auto build test ERROR on net-next/main]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Serge-Semin/net-pcs-xpcs-Drop-sentinel-entry-from-2500basex-ifaces-list/20231205-183808
> base: net-next/main
> patch link: https://lore.kernel.org/r/20231205103559.9605-12-fancer.lancer%40gmail.com
> patch subject: [PATCH net-next 11/16] net: pcs: xpcs: Change xpcs_create_mdiodev() suffix to "byaddr"
> config: arc-randconfig-001-20231206 (https://download.01.org/0day-ci/archive/20231206/[email protected]/config)
> compiler: arc-elf-gcc (GCC) 13.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231206/[email protected]/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <[email protected]>
> | Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
>
> All error/warnings (new ones prefixed by >>):
>
> drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c: In function 'txgbe_mdio_pcs_init':
> >> drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c:150:16: error: implicit declaration of function 'xpcs_create_mdiodev'; did you mean 'xpcs_create_byaddr'? [-Werror=implicit-function-declaration]
> 150 | xpcs = xpcs_create_mdiodev(mii_bus, 0, PHY_INTERFACE_MODE_10GBASER);
> | ^~~~~~~~~~~~~~~~~~~
> | xpcs_create_byaddr
> >> drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c:150:14: warning: assignment to 'struct dw_xpcs *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
> 150 | xpcs = xpcs_create_mdiodev(mii_bus, 0, PHY_INTERFACE_MODE_10GBASER);
> | ^
> cc1: some warnings being treated as errors
>
>
> vim +150 drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
Ah, right. I had been creating the series some times earlier than this
code was introduced and just missed it on the last rebase. I'll fix
this on v2.
-Serge(y)
>
> 854cace61387b6 Jiawen Wu 2023-06-06 121
> 854cace61387b6 Jiawen Wu 2023-06-06 122 static int txgbe_mdio_pcs_init(struct txgbe *txgbe)
> 854cace61387b6 Jiawen Wu 2023-06-06 123 {
> 854cace61387b6 Jiawen Wu 2023-06-06 124 struct mii_bus *mii_bus;
> 854cace61387b6 Jiawen Wu 2023-06-06 125 struct dw_xpcs *xpcs;
> 854cace61387b6 Jiawen Wu 2023-06-06 126 struct pci_dev *pdev;
> 854cace61387b6 Jiawen Wu 2023-06-06 127 struct wx *wx;
> 854cace61387b6 Jiawen Wu 2023-06-06 128 int ret = 0;
> 854cace61387b6 Jiawen Wu 2023-06-06 129
> 854cace61387b6 Jiawen Wu 2023-06-06 130 wx = txgbe->wx;
> 854cace61387b6 Jiawen Wu 2023-06-06 131 pdev = wx->pdev;
> 854cace61387b6 Jiawen Wu 2023-06-06 132
> 854cace61387b6 Jiawen Wu 2023-06-06 133 mii_bus = devm_mdiobus_alloc(&pdev->dev);
> 854cace61387b6 Jiawen Wu 2023-06-06 134 if (!mii_bus)
> 854cace61387b6 Jiawen Wu 2023-06-06 135 return -ENOMEM;
> 854cace61387b6 Jiawen Wu 2023-06-06 136
> 854cace61387b6 Jiawen Wu 2023-06-06 137 mii_bus->name = "txgbe_pcs_mdio_bus";
> 854cace61387b6 Jiawen Wu 2023-06-06 138 mii_bus->read_c45 = &txgbe_pcs_read;
> 854cace61387b6 Jiawen Wu 2023-06-06 139 mii_bus->write_c45 = &txgbe_pcs_write;
> 854cace61387b6 Jiawen Wu 2023-06-06 140 mii_bus->parent = &pdev->dev;
> 854cace61387b6 Jiawen Wu 2023-06-06 141 mii_bus->phy_mask = ~0;
> 854cace61387b6 Jiawen Wu 2023-06-06 142 mii_bus->priv = wx;
> 854cace61387b6 Jiawen Wu 2023-06-06 143 snprintf(mii_bus->id, MII_BUS_ID_SIZE, "txgbe_pcs-%x",
> d8c21ef7b2b147 Xiongfeng Wang 2023-08-08 144 pci_dev_id(pdev));
> 854cace61387b6 Jiawen Wu 2023-06-06 145
> 854cace61387b6 Jiawen Wu 2023-06-06 146 ret = devm_mdiobus_register(&pdev->dev, mii_bus);
> 854cace61387b6 Jiawen Wu 2023-06-06 147 if (ret)
> 854cace61387b6 Jiawen Wu 2023-06-06 148 return ret;
> 854cace61387b6 Jiawen Wu 2023-06-06 149
> 854cace61387b6 Jiawen Wu 2023-06-06 @150 xpcs = xpcs_create_mdiodev(mii_bus, 0, PHY_INTERFACE_MODE_10GBASER);
> 854cace61387b6 Jiawen Wu 2023-06-06 151 if (IS_ERR(xpcs))
> 854cace61387b6 Jiawen Wu 2023-06-06 152 return PTR_ERR(xpcs);
> 854cace61387b6 Jiawen Wu 2023-06-06 153
> 854cace61387b6 Jiawen Wu 2023-06-06 154 txgbe->xpcs = xpcs;
> 854cace61387b6 Jiawen Wu 2023-06-06 155
> 854cace61387b6 Jiawen Wu 2023-06-06 156 return 0;
> 854cace61387b6 Jiawen Wu 2023-06-06 157 }
> 854cace61387b6 Jiawen Wu 2023-06-06 158
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
On Wed, Dec 06, 2023 at 08:19:07AM +0800, kernel test robot wrote:
> Hi Serge,
>
> kernel test robot noticed the following build errors:
>
> [auto build test ERROR on net-next/main]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Serge-Semin/net-pcs-xpcs-Drop-sentinel-entry-from-2500basex-ifaces-list/20231205-183808
> base: net-next/main
> patch link: https://lore.kernel.org/r/20231205103559.9605-14-fancer.lancer%40gmail.com
> patch subject: [PATCH net-next 13/16] net: stmmac: intel: Register generic MDIO device
> config: x86_64-kexec (https://download.01.org/0day-ci/archive/20231206/[email protected]/config)
> compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231206/[email protected]/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <[email protected]>
> | Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
>
> All errors (new ones prefixed by >>):
>
> drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c: In function 'intel_mgbe_common_data':
> >> drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c:646:31: error: 'clk' undeclared (first use in this function)
> 646 | clk_disable_unprepare(clk);
> | ^~~
> drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c:646:31: note: each undeclared identifier is reported only once for each function it appears in
>
>
> vim +/clk +646 drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
Right, my series had been originally based on a branch which had some
of this driver parts fixed. I should have more thoroughly performed
the re-base process. I'll make sure it's fixed on v2.
-Serge(y)
>
> 446
> 447 static int intel_mgbe_common_data(struct pci_dev *pdev,
> 448 struct plat_stmmacenet_data *plat)
> 449 {
> 450 struct fwnode_handle *fwnode;
> 451 char clk_name[20];
> 452 int ret;
> 453 int i;
> 454
> 455 plat->pdev = pdev;
> 456 plat->phy_addr = -1;
> 457 plat->clk_csr = 5;
> 458 plat->has_gmac = 0;
> 459 plat->has_gmac4 = 1;
> 460 plat->force_sf_dma_mode = 0;
> 461 plat->flags |= (STMMAC_FLAG_TSO_EN | STMMAC_FLAG_SPH_DISABLE);
> 462
> 463 /* Multiplying factor to the clk_eee_i clock time
> 464 * period to make it closer to 100 ns. This value
> 465 * should be programmed such that the clk_eee_time_period *
> 466 * (MULT_FACT_100NS + 1) should be within 80 ns to 120 ns
> 467 * clk_eee frequency is 19.2Mhz
> 468 * clk_eee_time_period is 52ns
> 469 * 52ns * (1 + 1) = 104ns
> 470 * MULT_FACT_100NS = 1
> 471 */
> 472 plat->mult_fact_100ns = 1;
> 473
> 474 plat->rx_sched_algorithm = MTL_RX_ALGORITHM_SP;
> 475
> 476 for (i = 0; i < plat->rx_queues_to_use; i++) {
> 477 plat->rx_queues_cfg[i].mode_to_use = MTL_QUEUE_DCB;
> 478 plat->rx_queues_cfg[i].chan = i;
> 479
> 480 /* Disable Priority config by default */
> 481 plat->rx_queues_cfg[i].use_prio = false;
> 482
> 483 /* Disable RX queues routing by default */
> 484 plat->rx_queues_cfg[i].pkt_route = 0x0;
> 485 }
> 486
> 487 for (i = 0; i < plat->tx_queues_to_use; i++) {
> 488 plat->tx_queues_cfg[i].mode_to_use = MTL_QUEUE_DCB;
> 489
> 490 /* Disable Priority config by default */
> 491 plat->tx_queues_cfg[i].use_prio = false;
> 492 /* Default TX Q0 to use TSO and rest TXQ for TBS */
> 493 if (i > 0)
> 494 plat->tx_queues_cfg[i].tbs_en = 1;
> 495 }
> 496
> 497 /* FIFO size is 4096 bytes for 1 tx/rx queue */
> 498 plat->tx_fifo_size = plat->tx_queues_to_use * 4096;
> 499 plat->rx_fifo_size = plat->rx_queues_to_use * 4096;
> 500
> 501 plat->tx_sched_algorithm = MTL_TX_ALGORITHM_WRR;
> 502 plat->tx_queues_cfg[0].weight = 0x09;
> 503 plat->tx_queues_cfg[1].weight = 0x0A;
> 504 plat->tx_queues_cfg[2].weight = 0x0B;
> 505 plat->tx_queues_cfg[3].weight = 0x0C;
> 506 plat->tx_queues_cfg[4].weight = 0x0D;
> 507 plat->tx_queues_cfg[5].weight = 0x0E;
> 508 plat->tx_queues_cfg[6].weight = 0x0F;
> 509 plat->tx_queues_cfg[7].weight = 0x10;
> 510
> 511 plat->dma_cfg->pbl = 32;
> 512 plat->dma_cfg->pblx8 = true;
> 513 plat->dma_cfg->fixed_burst = 0;
> 514 plat->dma_cfg->mixed_burst = 0;
> 515 plat->dma_cfg->aal = 0;
> 516 plat->dma_cfg->dche = true;
> 517
> 518 plat->axi = devm_kzalloc(&pdev->dev, sizeof(*plat->axi),
> 519 GFP_KERNEL);
> 520 if (!plat->axi)
> 521 return -ENOMEM;
> 522
> 523 plat->axi->axi_lpi_en = 0;
> 524 plat->axi->axi_xit_frm = 0;
> 525 plat->axi->axi_wr_osr_lmt = 1;
> 526 plat->axi->axi_rd_osr_lmt = 1;
> 527 plat->axi->axi_blen[0] = 4;
> 528 plat->axi->axi_blen[1] = 8;
> 529 plat->axi->axi_blen[2] = 16;
> 530
> 531 plat->ptp_max_adj = plat->clk_ptp_rate;
> 532 plat->eee_usecs_rate = plat->clk_ptp_rate;
> 533
> 534 /* Set system clock */
> 535 sprintf(clk_name, "%s-%s", "stmmac", pci_name(pdev));
> 536
> 537 plat->stmmac_clk = clk_register_fixed_rate(&pdev->dev,
> 538 clk_name, NULL, 0,
> 539 plat->clk_ptp_rate);
> 540
> 541 if (IS_ERR(plat->stmmac_clk)) {
> 542 dev_warn(&pdev->dev, "Fail to register stmmac-clk\n");
> 543 plat->stmmac_clk = NULL;
> 544 }
> 545
> 546 ret = clk_prepare_enable(plat->stmmac_clk);
> 547 if (ret) {
> 548 clk_unregister_fixed_rate(plat->stmmac_clk);
> 549 return ret;
> 550 }
> 551
> 552 plat->ptp_clk_freq_config = intel_mgbe_ptp_clk_freq_config;
> 553
> 554 /* Set default value for multicast hash bins */
> 555 plat->multicast_filter_bins = HASH_TABLE_SIZE;
> 556
> 557 /* Set default value for unicast filter entries */
> 558 plat->unicast_filter_entries = 1;
> 559
> 560 /* Set the maxmtu to a default of JUMBO_LEN */
> 561 plat->maxmtu = JUMBO_LEN;
> 562
> 563 plat->flags |= STMMAC_FLAG_VLAN_FAIL_Q_EN;
> 564
> 565 /* Use the last Rx queue */
> 566 plat->vlan_fail_q = plat->rx_queues_to_use - 1;
> 567
> 568 /* For fixed-link setup, we allow phy-mode setting */
> 569 fwnode = dev_fwnode(&pdev->dev);
> 570 if (fwnode) {
> 571 int phy_mode;
> 572
> 573 /* "phy-mode" setting is optional. If it is set,
> 574 * we allow either sgmii or 1000base-x for now.
> 575 */
> 576 phy_mode = fwnode_get_phy_mode(fwnode);
> 577 if (phy_mode >= 0) {
> 578 if (phy_mode == PHY_INTERFACE_MODE_SGMII ||
> 579 phy_mode == PHY_INTERFACE_MODE_1000BASEX)
> 580 plat->phy_interface = phy_mode;
> 581 else
> 582 dev_warn(&pdev->dev, "Invalid phy-mode\n");
> 583 }
> 584 }
> 585
> 586 /* Intel mgbe SGMII interface uses pcs-xcps */
> 587 if (plat->phy_interface == PHY_INTERFACE_MODE_SGMII ||
> 588 plat->phy_interface == PHY_INTERFACE_MODE_1000BASEX) {
> 589 struct mdio_board_info *xpcs_info;
> 590
> 591 xpcs_info = devm_kzalloc(&pdev->dev,
> 592 sizeof(*xpcs_info) + MII_BUS_ID_SIZE,
> 593 GFP_KERNEL);
> 594 if (!xpcs_info) {
> 595 ret = -ENOMEM;
> 596 goto err_alloc_info;
> 597 }
> 598
> 599 xpcs_info->bus_id = (void *)xpcs_info + sizeof(*xpcs_info);
> 600 snprintf((char *)xpcs_info->bus_id, MII_BUS_ID_SIZE,
> 601 "stmmac-%x", plat->bus_id);
> 602
> 603 snprintf(xpcs_info->modalias, MDIO_NAME_SIZE, "dwxpcs");
> 604
> 605 xpcs_info->mdio_addr = INTEL_MGBE_XPCS_ADDR;
> 606
> 607 ret = mdiobus_register_board_info(xpcs_info, 1);
> 608 if (ret)
> 609 goto err_alloc_info;
> 610
> 611 plat->mdio_bus_data->has_xpcs = true;
> 612 plat->mdio_bus_data->xpcs_an_inband = true;
> 613 }
> 614
> 615 /* For fixed-link setup, we clear xpcs_an_inband */
> 616 if (fwnode) {
> 617 struct fwnode_handle *fixed_node;
> 618
> 619 fixed_node = fwnode_get_named_child_node(fwnode, "fixed-link");
> 620 if (fixed_node)
> 621 plat->mdio_bus_data->xpcs_an_inband = false;
> 622
> 623 fwnode_handle_put(fixed_node);
> 624 }
> 625
> 626 /* Ensure mdio bus PHY-scan skips intel serdes and pcs-xpcs */
> 627 plat->mdio_bus_data->phy_mask = 1 << INTEL_MGBE_ADHOC_ADDR;
> 628 plat->mdio_bus_data->phy_mask |= 1 << INTEL_MGBE_XPCS_ADDR;
> 629
> 630 plat->int_snapshot_num = AUX_SNAPSHOT1;
> 631
> 632 plat->crosststamp = intel_crosststamp;
> 633 plat->flags &= ~STMMAC_FLAG_INT_SNAPSHOT_EN;
> 634
> 635 /* Setup MSI vector offset specific to Intel mGbE controller */
> 636 plat->msi_mac_vec = 29;
> 637 plat->msi_lpi_vec = 28;
> 638 plat->msi_sfty_ce_vec = 27;
> 639 plat->msi_sfty_ue_vec = 26;
> 640 plat->msi_rx_base_vec = 0;
> 641 plat->msi_tx_base_vec = 1;
> 642
> 643 return 0;
> 644
> 645 err_alloc_info:
> > 646 clk_disable_unprepare(clk);
> 647 clk_unregister_fixed_rate(clk);
> 648
> 649 return ret;
> 650 }
> 651
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
On Thu, Dec 07, 2023 at 04:35:47PM +0300, Serge Semin wrote:
> Hi Andrew
>
> On Wed, Dec 06, 2023 at 06:01:30PM +0100, Andrew Lunn wrote:
> > > > You shouldn't use inline in C files, only in headers.
> > >
> > > Could you please clarify why? I failed to find such requirement in the
> > > coding style doc. Moreover there are multiple examples of using the
> > > static-inline-ers in the C files in the kernel including the net/mdio
> > > subsystem.
> >
>
> > The compiler does a better job at deciding what to inline than we
> > humans do. If you can show the compiler is doing it wrong, then we
> > might accept them.
>
> In general I would have agreed with you especially if the methods were
> heavier than what they are:
> static inline ptrdiff_t dw_xpcs_mmio_addr_format(int dev, int reg)
> {
> return FIELD_PREP(0x1f0000, dev) | FIELD_PREP(0xffff, reg);
> }
>
> static inline u16 dw_xpcs_mmio_addr_page(ptrdiff_t csr)
> {
> return FIELD_GET(0x1fff00, csr);
> }
>
> static inline ptrdiff_t dw_xpcs_mmio_addr_offset(ptrdiff_t csr)
> {
> return FIELD_GET(0xff, csr);
> }
>
> > But in general, netdev does not like inline in .C
> > file.
>
> I see. I'll do as you say if you don't change your mind after my
> reasoning below.
>
> > Also, nothing in MDIO is hot path, it spends a lot of time
> > waiting for a slow bus. So inline is likely to just bloat the code for
> > no gain.
>
> I would have been absolutely with you in this matter, if we were talking
> about a normal MDIO bus. In this case the devices are accessed over
> the system IO-memory. So the bus isn't that slow.
O.K, so its not slow. But how often is it used? PHYs tend to get
polled once a second if interrupts are not used. But is the PCS also
polled at the same time? Does this optimisation make a noticeable
difference at once per second? Do you have a requirement that the
system boots very very fast, and this optimisation makes a difference
when there is heavier activity on the PCS at boot? Is the saving
noticeable, when auto-neg takes a little over 1 second.
The best way to make your case is show real world requirements and
benchmark results.
Andrew
Hi Vladimir
On Tue, Dec 05, 2023 at 02:23:16PM +0200, Vladimir Oltean wrote:
> On Tue, Dec 05, 2023 at 02:35:46PM +0300, Serge Semin wrote:
> > Omg, thank you very much for testing the series straight away and
> > sorry for the immediate trouble it caused. I'll need some more time
> > for investigation. I'll get back to this topic a bit later on this
> > week.
>
> Don't worry, I got suspicious when I was CCed to review only a one-line
> change in patch 11/16. It's never about that one line, is it?)
Right. I should have added you to the list of recipients of the entire
series since the patchset changes more than that. The bug you caught
brightly highlights my mistake. I'll make sure you are in the list.
I'll add Jiawen and Mengyuan there too since the driver under their
maintenance is also affected. Hopefully they'll get to test the series
too.
>
> Anyway, the NULL dev->p is a symptom of device_add() not having been
> called, most likely from mdio_device_register().
Absolutely right. I thought that mdio_device_create() having the
device_initialize() method called was enough for the device_attach()
function being happy. It turns out it wasn't and I missed that the
device_private instance is allocated only on the device registration.
So I'll need to call mdio_device_register() after all, if I get to
preserve the current design of the solution.
>
> I'll be honest and say that I still don't quite understand what you're
> trying to achieve. You're trying to bind the hardcoded mdio_devices
> created by xpcs_create() to a driver?
My idea was to reuse the mdio_device which has already been created
either by means of the MDIO-bus OF-subnode or by means of the MDIO-bus
board_info infrastructure (can be utilized in the SJA1105 or Wangxun
Tx GBE). The xpcs_create() method then either probes the device on the MDIO
bus and gets ID from there, or just uses the custom IDs based on the
OF compatible match table or on the platform_data. If no MDIO-device
was created my patchset is supposed to preserve the previous
semantics: create MDIO-device, probe the device on the MDIO-bus, get
device IDs from there. See the next patch for more details:
https://lore.kernel.org/netdev/[email protected]/
> That was attempted a while ago by
> Sean Anderson with the Lynx PCS. Are you aware of the fact that even in
> the good case in which binding the driver actually works, the user can
> then come along and unbind it from the PCS device, and phylink isn't
> prepared to handle that, so it will crash the kernel upon the next
> phylink_pcs call?
To be honest I didn't consider the driver bind/unbind option. But my
case a bit different. DW XPCS MDIO-device is supposed to be created
automatically by means of the DW XPCS MI driver from the DT-nodes
hierarchy like this:
mdio@1f05d000 {
compatible = "snps,dw-xpcs-mi";
reg = <0 0x1f05d000 0 0x1000>;
xgmac_pcs: ethernet-pcs@0 {
compatible = "snps,dw-xpcs";
reg = <0>;
};
};
The platform-device is created for the mdio@1f05d000 node for which
the DW XPCS MI driver is loaded, which calls the
devm_of_mdiobus_register() in the probe() method which registers the
MDIO-bus and then creates the MDIO-device from the ethernet-pcs@0
node. The DW XPCS MDIO-device driver is attached to that MDIO-device
then. In such model the PCS can be supplied to the DW *MAC via the
"pcs-handle = &xgmac_pcs" property.
Regarding the current semantics it's preserved in the framework of the
xpcs_create_byaddr() method (former xpcs_create_mdiodev()) by means of
the next code snippet:
if (mdiobus_is_registered_device(bus, addr)) {
mdiodev = bus->mdio_map[addr];
mdio_device_get(mdiodev);
} else {
mdiodev = mdio_device_create(bus, addr);
if (IS_ERR(mdiodev))
return ERR_CAST(mdiodev);
}
Device can be automatically created if before registering the MDIO-bus
the xpcs_create_byaddr() caller registered the MDIO-device board info
by means of the mdiobus_register_board_info() method. In addition to
that it's now possible to supply some custom data (custom device IDs
in my implementation) to the XPCS driver by means of the
mdio_board_info.platform_data field. See the next patch for
reference:
https://lore.kernel.org/netdev/[email protected]
So what the difference with the Lynx PCS is that in my case the
MDIO-device is created automatically as a result of the DW XPCS MI
MDIO-bus registration. Additionally I implemented the MDIO-device
creation based on the MDIO-board-info, thus there won't be need in the
calling mdio_device_create() on each xpcs_create_mdiodev() invocation.
The later part isn't that important in the framework of this
conversation, but just so you be aware.
Regarding the driver bind/unbind. As I said I didn't actually consider
that option. On the other hand my DW XPCS MDIO-device driver doesn't
do actual probe() or remove(). The only implemented thing is the
of_device_id table, which is used to assign PCS and PMA IDs if
required based on the DT compatible property. So I can easily drop any
MDIO device-driver part and parse the of_device_id table right in the
xpcs_create_bynode(). From that perspective my implementation won't
differ much from the Lynx PCS design. The only difference will be is
the way the MDIO-bus is created and registered. In case of Lynx PCS
the bus is created by the MAC-driver itself. In my case DW XPCS MI is
currently created in the framework of the separate platform driver. Do
you think it would be better to follow the Lynx design pattern in
order to get rid from the possibility of the DW XPCS MI driver being
unbound behind the STMMAC+XPCS couple back?
In this case the Dw MAC DT-node hierarchy would look like this:
xgmac: ethernet@1f054000 {
compatible = "snps,dwxgmac";
reg = <0 0x1f054000 0 0x4000>;
reg-names = "stmmaceth";
ranges;
...
pcs-handle = &xgmac_pcs;
// DW XPCS MI to access the DW XPCS attached to the device
mdio@1f05d000 {
compatible = "snps,dwmac-mi";
reg = <0 0x1f05d000 0 0x1000>;
xgmac_pcs: ethernet-pcs@0 {
compatible = "snps,dw-xpcs";
reg = <0>;
};
};
// Normal MDIO-bus to access external PHYs (it's also called
// as SMA - Station Management Agent - by Synopsys)
mdio {
compatible = "snps,dwmac-mdio";
#address-cells = <1>;
#size-cells = <0>;
};
};
I actually thought to use that hardware description pattern instead,
but after some meditation around that I decided that having the DW
XPCS device defined separately from the DW MAC node seemed better at
least from the code separation point of view. Now I think that it
wasn't the best decision. DW XPCS is always attached to the DW XGMAC
controller. So it would be more correct having it defined as a
sub-node. It would also helped to avoid the platform device driver
bind/unbind problem.
What do you think? Should I re-design my patchset to be supporting the
design above? (After having conversion with you I am more inclined to
do that now than to stick with the currently implemented solution.)
>
> The pcs-rzn1-miic.c driver puts a device_link to the MAC to at least
> tear down the whole thing when the PCS is unbound, which is saner than
> crashing the kernel. I don't see the equivalent protection mechanism here?
You are right. I don't have any equivalent protection here. Thanks for
suggesting a solution.
>
> Can't the xpcs continue to live without a bound driver? Having a
> compatible string in the OF description is perfectly fine though,
> and should absolutely not preclude that.
As I explained above Dw XPCS device can live without a bound driver
because the DW XPCS MDIO-driver doesn't do much but merely gets to be
bound based on the of_device_id table. In my case the problem is in
the DW XPCS MI driver which indeed can be detached. Please see my
long-read text above.
-Serge(y)
On Thu, Dec 07, 2023 at 03:54:08PM +0100, Andrew Lunn wrote:
> On Thu, Dec 07, 2023 at 04:35:47PM +0300, Serge Semin wrote:
> > Hi Andrew
> >
> > On Wed, Dec 06, 2023 at 06:01:30PM +0100, Andrew Lunn wrote:
> > > > > You shouldn't use inline in C files, only in headers.
> > > >
> > > > Could you please clarify why? I failed to find such requirement in the
> > > > coding style doc. Moreover there are multiple examples of using the
> > > > static-inline-ers in the C files in the kernel including the net/mdio
> > > > subsystem.
> > >
> >
> > > The compiler does a better job at deciding what to inline than we
> > > humans do. If you can show the compiler is doing it wrong, then we
> > > might accept them.
> >
> > In general I would have agreed with you especially if the methods were
> > heavier than what they are:
> > static inline ptrdiff_t dw_xpcs_mmio_addr_format(int dev, int reg)
> > {
> > return FIELD_PREP(0x1f0000, dev) | FIELD_PREP(0xffff, reg);
> > }
> >
> > static inline u16 dw_xpcs_mmio_addr_page(ptrdiff_t csr)
> > {
> > return FIELD_GET(0x1fff00, csr);
> > }
> >
> > static inline ptrdiff_t dw_xpcs_mmio_addr_offset(ptrdiff_t csr)
> > {
> > return FIELD_GET(0xff, csr);
> > }
> >
> > > But in general, netdev does not like inline in .C
> > > file.
> >
> > I see. I'll do as you say if you don't change your mind after my
> > reasoning below.
> >
> > > Also, nothing in MDIO is hot path, it spends a lot of time
> > > waiting for a slow bus. So inline is likely to just bloat the code for
> > > no gain.
> >
> > I would have been absolutely with you in this matter, if we were talking
> > about a normal MDIO bus. In this case the devices are accessed over
> > the system IO-memory. So the bus isn't that slow.
>
> O.K, so its not slow. But how often is it used? PHYs tend to get
> polled once a second if interrupts are not used. But is the PCS also
> polled at the same time? Does this optimisation make a noticeable
> difference at once per second? Do you have a requirement that the
> system boots very very fast, and this optimisation makes a difference
> when there is heavier activity on the PCS at boot? Is the saving
> noticeable, when auto-neg takes a little over 1 second.
>
> The best way to make your case is show real world requirements and
> benchmark results.
You do know how to well define your point.) Polling is what currently
implemented in the XPCS driver indeed. So in this case such small
optimization won't be even noticeable. Although theoretically the
IO-access could be performed on the fast paths, in the IRQ context,
but it could be relevant only if the DW XPCS device had the IRQ
feature activated on the particular platform and the DW XPCS driver
supported it. But seeing the driver currently doesn't support it and
the DW XPCS could be also found on the DW MAC SMA MDIO bus
(non-memory-mapped case), always handling IRQ in the hardware IRQ
context would be wrong. Splitting up the handlers would be
over-complication for indeed doubtful reason, since inlining those
methods won't gain significant performance even in that case. And of
course I don't have such strict requirements as you say. So I'll drop
the inline keywords then. Thank you very much for having kept
discussing the topic in order to fully clarify it for me, even though
the issue could have seemed unimportant to spend time for.
-Serge(y)
>
> Andrew
On Fri, Dec 08, 2023 at 05:11:20PM +0300, Serge Semin wrote:
> My idea was to reuse the mdio_device which has already been created
> either by means of the MDIO-bus OF-subnode or by means of the MDIO-bus
> board_info infrastructure (can be utilized in the SJA1105 or Wangxun
> Tx GBE). The xpcs_create() method then either probes the device on the MDIO
> bus and gets ID from there, or just uses the custom IDs based on the
> OF compatible match table or on the platform_data. If no MDIO-device
> was created my patchset is supposed to preserve the previous
> semantics: create MDIO-device, probe the device on the MDIO-bus, get
> device IDs from there. See the next patch for more details:
> https://lore.kernel.org/netdev/[email protected]/
>
> > That was attempted a while ago by
> > Sean Anderson with the Lynx PCS. Are you aware of the fact that even in
> > the good case in which binding the driver actually works, the user can
> > then come along and unbind it from the PCS device, and phylink isn't
> > prepared to handle that, so it will crash the kernel upon the next
> > phylink_pcs call?
>
> To be honest I didn't consider the driver bind/unbind option. But my
> case a bit different. DW XPCS MDIO-device is supposed to be created
> automatically by means of the DW XPCS MI driver from the DT-nodes
> hierarchy like this:
> mdio@1f05d000 {
> compatible = "snps,dw-xpcs-mi";
> reg = <0 0x1f05d000 0 0x1000>;
>
> xgmac_pcs: ethernet-pcs@0 {
> compatible = "snps,dw-xpcs";
> reg = <0>;
> };
> };
> The platform-device is created for the mdio@1f05d000 node for which
> the DW XPCS MI driver is loaded, which calls the
> devm_of_mdiobus_register() in the probe() method which registers the
> MDIO-bus and then creates the MDIO-device from the ethernet-pcs@0
> node. The DW XPCS MDIO-device driver is attached to that MDIO-device
> then. In such model the PCS can be supplied to the DW *MAC via the
> "pcs-handle = &xgmac_pcs" property.
>
> Regarding the current semantics it's preserved in the framework of the
> xpcs_create_byaddr() method (former xpcs_create_mdiodev()) by means of
> the next code snippet:
> if (mdiobus_is_registered_device(bus, addr)) {
> mdiodev = bus->mdio_map[addr];
> mdio_device_get(mdiodev);
> } else {
> mdiodev = mdio_device_create(bus, addr);
> if (IS_ERR(mdiodev))
> return ERR_CAST(mdiodev);
> }
> Device can be automatically created if before registering the MDIO-bus
> the xpcs_create_byaddr() caller registered the MDIO-device board info
> by means of the mdiobus_register_board_info() method. In addition to
> that it's now possible to supply some custom data (custom device IDs
> in my implementation) to the XPCS driver by means of the
> mdio_board_info.platform_data field. See the next patch for
> reference:
> https://lore.kernel.org/netdev/[email protected]
>
> So what the difference with the Lynx PCS is that in my case the
> MDIO-device is created automatically as a result of the DW XPCS MI
> MDIO-bus registration. Additionally I implemented the MDIO-device
> creation based on the MDIO-board-info, thus there won't be need in the
> calling mdio_device_create() on each xpcs_create_mdiodev() invocation.
> The later part isn't that important in the framework of this
> conversation, but just so you be aware.
It's not really different, though. You can connect to the Lynx PCS both
ways, see dpaa2_pcs_create() which also searches for a pcs-handle before
calling lynx_pcs_create_fwnode(). What's subtly different is that we
don't (yet) have "fsl,lynx-pcs" compatible strings in the device tree.
So the MDIO controller will register the PCS devices as struct phy_device
(which still have an underlying struct mdio_device). The PCS layer
connects to the underlying struct mdio_device, and the phy_device on top
remains unconnected to any phylib/phylink MAC driver. That is confusing,
I should really get to adding those compatible strings to suppress the
phy_device creation.
> Regarding the driver bind/unbind. As I said I didn't actually consider
> that option. On the other hand my DW XPCS MDIO-device driver doesn't
> do actual probe() or remove(). The only implemented thing is the
> of_device_id table, which is used to assign PCS and PMA IDs if
> required based on the DT compatible property. So I can easily drop any
> MDIO device-driver part and parse the of_device_id table right in the
> xpcs_create_bynode(). From that perspective my implementation won't
> differ much from the Lynx PCS design. The only difference will be is
> the way the MDIO-bus is created and registered. In case of Lynx PCS
> the bus is created by the MAC-driver itself.
Nope, not true. Follow the pcs-handle in arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi.
> In my case DW XPCS MI is currently created in the framework of the
> separate platform driver. Do you think it would be better to follow
> the Lynx design pattern in order to get rid from the possibility of
> the DW XPCS MI driver being unbound behind the STMMAC+XPCS couple
> back?
I think you actually pointed out a flaw in the Lynx PCS design too.
Actually, it is a larger flaw in the kernel. You can also unbind the
MDIO bus which holds the phy_device, and phylib (and therefore also
phylink) won't expect that either, so it will crash.
> In this case the Dw MAC DT-node hierarchy would look like this:
>
> xgmac: ethernet@1f054000 {
> compatible = "snps,dwxgmac";
> reg = <0 0x1f054000 0 0x4000>;
> reg-names = "stmmaceth";
> ranges;
>
> ...
>
> pcs-handle = &xgmac_pcs;
>
> // DW XPCS MI to access the DW XPCS attached to the device
> mdio@1f05d000 {
> compatible = "snps,dwmac-mi";
> reg = <0 0x1f05d000 0 0x1000>;
>
> xgmac_pcs: ethernet-pcs@0 {
> compatible = "snps,dw-xpcs";
> reg = <0>;
> };
> };
>
> // Normal MDIO-bus to access external PHYs (it's also called
> // as SMA - Station Management Agent - by Synopsys)
> mdio {
> compatible = "snps,dwmac-mdio";
> #address-cells = <1>;
> #size-cells = <0>;
> };
> };
>
> I actually thought to use that hardware description pattern instead,
> but after some meditation around that I decided that having the DW
> XPCS device defined separately from the DW MAC node seemed better at
> least from the code separation point of view. Now I think that it
> wasn't the best decision. DW XPCS is always attached to the DW XGMAC
> controller. So it would be more correct having it defined as a
> sub-node. It would also helped to avoid the platform device driver
> bind/unbind problem.
>
> What do you think? Should I re-design my patchset to be supporting the
> design above? (After having conversion with you I am more inclined to
> do that now than to stick with the currently implemented solution.)
I think that the placement of the "mdio" node as lateral vs subordinate
to the "ethernet" node would have fixed the issue by mistake. We should
be looking at it as a structural problem of the kernel instead. Don't
let it influence what you believe should be the correct design.
> > The pcs-rzn1-miic.c driver puts a device_link to the MAC to at least
> > tear down the whole thing when the PCS is unbound, which is saner than
> > crashing the kernel. I don't see the equivalent protection mechanism here?
>
> You are right. I don't have any equivalent protection here. Thanks for
> suggesting a solution.
I think that a device link between the "ethernet" device and the "mdio"
device (controller, parent of the PHY or PCS), if the Ethernet is not a
parent of the MDIO controller, could also solve that. But it would also
require ACK from PHY maintainers, who may have grander plans to address
this snag.
> > Can't the xpcs continue to live without a bound driver? Having a
> > compatible string in the OF description is perfectly fine though,
> > and should absolutely not preclude that.
>
> As I explained above Dw XPCS device can live without a bound driver
> because the DW XPCS MDIO-driver doesn't do much but merely gets to be
> bound based on the of_device_id table. In my case the problem is in
> the DW XPCS MI driver which indeed can be detached. Please see my
> long-read text above.
Yeah, common design, common problem.
Hi Vladimir,
Sorry for the delayed response. Too much work at this time of the year
(ah, Decembers)..(
On Fri, Dec 08, 2023 at 06:33:43PM +0200, Vladimir Oltean wrote:
> On Fri, Dec 08, 2023 at 05:11:20PM +0300, Serge Semin wrote:
> > My idea was to reuse the mdio_device which has already been created
> > either by means of the MDIO-bus OF-subnode or by means of the MDIO-bus
> > board_info infrastructure (can be utilized in the SJA1105 or Wangxun
> > Tx GBE). The xpcs_create() method then either probes the device on the MDIO
> > bus and gets ID from there, or just uses the custom IDs based on the
> > OF compatible match table or on the platform_data. If no MDIO-device
> > was created my patchset is supposed to preserve the previous
> > semantics: create MDIO-device, probe the device on the MDIO-bus, get
> > device IDs from there. See the next patch for more details:
> > https://lore.kernel.org/netdev/[email protected]/
> >
> > > That was attempted a while ago by
> > > Sean Anderson with the Lynx PCS. Are you aware of the fact that even in
> > > the good case in which binding the driver actually works, the user can
> > > then come along and unbind it from the PCS device, and phylink isn't
> > > prepared to handle that, so it will crash the kernel upon the next
> > > phylink_pcs call?
> >
> > To be honest I didn't consider the driver bind/unbind option. But my
> > case a bit different. DW XPCS MDIO-device is supposed to be created
> > automatically by means of the DW XPCS MI driver from the DT-nodes
> > hierarchy like this:
> > mdio@1f05d000 {
> > compatible = "snps,dw-xpcs-mi";
> > reg = <0 0x1f05d000 0 0x1000>;
> >
> > xgmac_pcs: ethernet-pcs@0 {
> > compatible = "snps,dw-xpcs";
> > reg = <0>;
> > };
> > };
> > The platform-device is created for the mdio@1f05d000 node for which
> > the DW XPCS MI driver is loaded, which calls the
> > devm_of_mdiobus_register() in the probe() method which registers the
> > MDIO-bus and then creates the MDIO-device from the ethernet-pcs@0
> > node. The DW XPCS MDIO-device driver is attached to that MDIO-device
> > then. In such model the PCS can be supplied to the DW *MAC via the
> > "pcs-handle = &xgmac_pcs" property.
> >
> > Regarding the current semantics it's preserved in the framework of the
> > xpcs_create_byaddr() method (former xpcs_create_mdiodev()) by means of
> > the next code snippet:
> > if (mdiobus_is_registered_device(bus, addr)) {
> > mdiodev = bus->mdio_map[addr];
> > mdio_device_get(mdiodev);
> > } else {
> > mdiodev = mdio_device_create(bus, addr);
> > if (IS_ERR(mdiodev))
> > return ERR_CAST(mdiodev);
> > }
> > Device can be automatically created if before registering the MDIO-bus
> > the xpcs_create_byaddr() caller registered the MDIO-device board info
> > by means of the mdiobus_register_board_info() method. In addition to
> > that it's now possible to supply some custom data (custom device IDs
> > in my implementation) to the XPCS driver by means of the
> > mdio_board_info.platform_data field. See the next patch for
> > reference:
> > https://lore.kernel.org/netdev/[email protected]
> >
> > So what the difference with the Lynx PCS is that in my case the
> > MDIO-device is created automatically as a result of the DW XPCS MI
> > MDIO-bus registration. Additionally I implemented the MDIO-device
> > creation based on the MDIO-board-info, thus there won't be need in the
> > calling mdio_device_create() on each xpcs_create_mdiodev() invocation.
> > The later part isn't that important in the framework of this
> > conversation, but just so you be aware.
>
> It's not really different, though. You can connect to the Lynx PCS both
> ways, see dpaa2_pcs_create() which also searches for a pcs-handle before
> calling lynx_pcs_create_fwnode().
Ah, right. Lynx PCS also implements the fwnode-based PCS descriptor
creation.
> What's subtly different is that we
> don't (yet) have "fsl,lynx-pcs" compatible strings in the device tree.
> So the MDIO controller will register the PCS devices as struct phy_device
> (which still have an underlying struct mdio_device). The PCS layer
> connects to the underlying struct mdio_device, and the phy_device on top
> remains unconnected to any phylib/phylink MAC driver. That is confusing,
> I should really get to adding those compatible strings to suppress the
> phy_device creation.
It hasn't been confirmed yet here
https://lore.kernel.org/netdev/na6krkoco7pmsl62dfuj2xlrvpsnod74ptpfyy6gv7dzwmowga@mzsiknjian2i/
but AFAICS it is wrong to have a PCS device registered as PHY by any
means: unmasking address in mii_bus->phy_mask or having the
of_mdiobus_child_is_phy() returned true for a DT-node. So right, a
specific compatible should be added to the PCS DT-nodes.
>
> > Regarding the driver bind/unbind. As I said I didn't actually consider
> > that option. On the other hand my DW XPCS MDIO-device driver doesn't
> > do actual probe() or remove(). The only implemented thing is the
> > of_device_id table, which is used to assign PCS and PMA IDs if
> > required based on the DT compatible property. So I can easily drop any
> > MDIO device-driver part and parse the of_device_id table right in the
> > xpcs_create_bynode(). From that perspective my implementation won't
> > differ much from the Lynx PCS design. The only difference will be is
> > the way the MDIO-bus is created and registered. In case of Lynx PCS
> > the bus is created by the MAC-driver itself.
>
> Nope, not true. Follow the pcs-handle in arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi.
Ah, right, I missed that case. I was referring to the cases when the
Lynx PCS MDIO node is defined as a sub-node of the MAC node.
>
> > In my case DW XPCS MI is currently created in the framework of the
> > separate platform driver. Do you think it would be better to follow
> > the Lynx design pattern in order to get rid from the possibility of
> > the DW XPCS MI driver being unbound behind the STMMAC+XPCS couple
> > back?
>
> I think you actually pointed out a flaw in the Lynx PCS design too.
> Actually, it is a larger flaw in the kernel. You can also unbind the
> MDIO bus which holds the phy_device, and phylib (and therefore also
> phylink) won't expect that either, so it will crash.
>
> > In this case the Dw MAC DT-node hierarchy would look like this:
> >
> > xgmac: ethernet@1f054000 {
> > compatible = "snps,dwxgmac";
> > reg = <0 0x1f054000 0 0x4000>;
> > reg-names = "stmmaceth";
> > ranges;
> >
> > ...
> >
> > pcs-handle = &xgmac_pcs;
> >
> > // DW XPCS MI to access the DW XPCS attached to the device
> > mdio@1f05d000 {
> > compatible = "snps,dwmac-mi";
> > reg = <0 0x1f05d000 0 0x1000>;
> >
> > xgmac_pcs: ethernet-pcs@0 {
> > compatible = "snps,dw-xpcs";
> > reg = <0>;
> > };
> > };
> >
> > // Normal MDIO-bus to access external PHYs (it's also called
> > // as SMA - Station Management Agent - by Synopsys)
> > mdio {
> > compatible = "snps,dwmac-mdio";
> > #address-cells = <1>;
> > #size-cells = <0>;
> > };
> > };
> >
> > I actually thought to use that hardware description pattern instead,
> > but after some meditation around that I decided that having the DW
> > XPCS device defined separately from the DW MAC node seemed better at
> > least from the code separation point of view. Now I think that it
> > wasn't the best decision. DW XPCS is always attached to the DW XGMAC
> > controller. So it would be more correct having it defined as a
> > sub-node. It would also helped to avoid the platform device driver
> > bind/unbind problem.
> >
> > What do you think? Should I re-design my patchset to be supporting the
> > design above? (After having conversion with you I am more inclined to
> > do that now than to stick with the currently implemented solution.)
>
> I think that the placement of the "mdio" node as lateral vs subordinate
> to the "ethernet" node would have fixed the issue by mistake. We should
> be looking at it as a structural problem of the kernel instead. Don't
> let it influence what you believe should be the correct design.
Ok. Thanks for clarification. I won't move the PCS device DT-node to
being subordinate of the MAC DT-node then. Although after some more
digging into the problem I figured out that the solution still needs
to be re-designed a bit. Currently I have the DW XPCS device represented as
the nodes hierarchy:
mdio@1f05d000 {
compatible = "snps,dwmac-mi";
reg = <0 0x1f05d000 0 0x1000>;
xgmac_pcs: ethernet-pcs@0 {
compatible = "snps,dw-xpcs";
reg = <0>;
};
};
When I introduced such representation I assumed that it was possible
to have more than one DW XPCS devices accessible over the same MCI/APB
interface with for instance some static offset. But it turned out I
was wrong again. DW XPCS HW databook explicitly states that
port_id_i[4:0] input signal is specific to the MDIO interface only.
That signal is the MDIO-bus address of the device and it doesn't exist
for the DW XPCS devices mapped to the system IO-memory via the MCI/APB
buses. So there can't be more than one XPCS device accessible over the
same MCI/APB port and the correct way to represent the DW XPCS device
is just:
xgmac_pcs: ethernet-pcs@0 {
compatible = "snps,dw-xpcs";
reg = <0 0x1f05d000 0 0x1000>;
};
with no superior MI node. I'll re-design my patchset to support the
device representation above then: just create a hidden MDIO-bus in the
DW XPCS driver probe method and directly register the XPCS-device on
it. The patch for the MDIO-bus subsystem will be gone after that.
>
> > > The pcs-rzn1-miic.c driver puts a device_link to the MAC to at least
> > > tear down the whole thing when the PCS is unbound, which is saner than
> > > crashing the kernel. I don't see the equivalent protection mechanism here?
> >
> > You are right. I don't have any equivalent protection here. Thanks for
> > suggesting a solution.
>
> I think that a device link between the "ethernet" device and the "mdio"
> device (controller, parent of the PHY or PCS), if the Ethernet is not a
> parent of the MDIO controller, could also solve that. But it would also
> require ACK from PHY maintainers, who may have grander plans to address
> this snag.
Ok. I'll add it in v2. Let's see what the maintainers think about
that.
Thanks for all your comments in my patchset regard. They are really
helpful.
-Serge(y)
>
> > > Can't the xpcs continue to live without a bound driver? Having a
> > > compatible string in the OF description is perfectly fine though,
> > > and should absolutely not preclude that.
> >
> > As I explained above Dw XPCS device can live without a bound driver
> > because the DW XPCS MDIO-driver doesn't do much but merely gets to be
> > bound based on the of_device_id table. In my case the problem is in
> > the DW XPCS MI driver which indeed can be detached. Please see my
> > long-read text above.
>
> Yeah, common design, common problem.
On Thu, Dec 14, 2023 at 02:54:00PM +0300, Serge Semin wrote:
> > > > The pcs-rzn1-miic.c driver puts a device_link to the MAC to at least
> > > > tear down the whole thing when the PCS is unbound, which is saner than
> > > > crashing the kernel. I don't see the equivalent protection mechanism here?
> > >
> > > You are right. I don't have any equivalent protection here. Thanks for
> > > suggesting a solution.
> >
> > I think that a device link between the "ethernet" device and the "mdio"
> > device (controller, parent of the PHY or PCS), if the Ethernet is not a
> > parent of the MDIO controller, could also solve that. But it would also
> > require ACK from PHY maintainers, who may have grander plans to address
> > this snag.
>
> Ok. I'll add it in v2. Let's see what the maintainers think about
> that.
Are you not following the parallel discussion on the topic of PCS
devices having bound drivers?
https://lore.kernel.org/netdev/ZXnV%2FPk1PYxAm%[email protected]/
Sadly I don't have much spare time to join that discussion, but it looks
like you could.
On Thu, Dec 14, 2023 at 02:00:16PM +0200, Vladimir Oltean wrote:
> On Thu, Dec 14, 2023 at 02:54:00PM +0300, Serge Semin wrote:
> > > > > The pcs-rzn1-miic.c driver puts a device_link to the MAC to at least
> > > > > tear down the whole thing when the PCS is unbound, which is saner than
> > > > > crashing the kernel. I don't see the equivalent protection mechanism here?
> > > >
> > > > You are right. I don't have any equivalent protection here. Thanks for
> > > > suggesting a solution.
> > >
> > > I think that a device link between the "ethernet" device and the "mdio"
> > > device (controller, parent of the PHY or PCS), if the Ethernet is not a
> > > parent of the MDIO controller, could also solve that. But it would also
> > > require ACK from PHY maintainers, who may have grander plans to address
> > > this snag.
> >
> > Ok. I'll add it in v2. Let's see what the maintainers think about
> > that.
>
> Are you not following the parallel discussion on the topic of PCS
> devices having bound drivers?
> https://lore.kernel.org/netdev/ZXnV%2FPk1PYxAm%[email protected]/
>
> Sadly I don't have much spare time to join that discussion, but it looks
> like you could.
Ok. Thanks for sharing the link. At least I'll follow up the
discussion in order to pick up/wait for a solution they'll come up
with.
-Serge(y)
On Tue, Dec 05, 2023 at 01:35:29PM +0300, Serge Semin wrote:
> Synopsys DesignWare XPCS IP-core is a Physical Coding Sublayer (PCS) layer
> providing an interface between the Media Access Control (MAC) and Physical
> Medium Attachment Sublayer (PMA) through a Media independent interface.
> >From software point of view it exposes IEEE std. Clause 45 CSR space and
> can be accessible either by MDIO or MCI/APB3 bus interfaces. The later
> case is described by means of a dedicated DT-bindings which imply having
> the DW XPCS Management Interface defined as a DT-supernode which child the
> PCSs nodes would be (in the same way as the standard MDIO buses and
> devices are normally defined).
>
> Besides of that DW XPCS DT-nodes can have the interrupts and clock source
> properties specified. The former one indicates the Clause 73/37
> auto-negotiation events like: negotiation page received, AN is completed
> or incompatible link partner. The clock DT-properties can describe up to
> two clock sources: internal one and the one connected to the chip pad.
> Either of them is supposed to be used as the device reference clocks.
>
> Finally the DW XPCS IP-core can be optionally synthesized with a
> vendor-specific interface connected to Synopsys PMA (also called
> DesignWare Consumer/Enterprise PHY). Alas that isn't auto-detectable
> anyhow so if the DW XPCS device has the respective PMA attached then it
> should be reflected in the DT-node compatible string so the driver would
> be aware of the PMA-specific device capabilities (mainly connected with
> CSRs available for the fine-tunings).
>
> Signed-off-by: Serge Semin <[email protected]>
> ---
> .../bindings/net/pcs/snps,dw-xpcs.yaml | 88 +++++++++++++++++++
> .../bindings/net/snps,dw-xpcs-mi.yaml | 88 +++++++++++++++++++
> 2 files changed, 176 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/pcs/snps,dw-xpcs.yaml
> create mode 100644 Documentation/devicetree/bindings/net/snps,dw-xpcs-mi.yaml
>
> diff --git a/Documentation/devicetree/bindings/net/pcs/snps,dw-xpcs.yaml b/Documentation/devicetree/bindings/net/pcs/snps,dw-xpcs.yaml
> new file mode 100644
> index 000000000000..9694ef51abad
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/pcs/snps,dw-xpcs.yaml
> @@ -0,0 +1,88 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/pcs/snps,dw-xpcs.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Synopsys DesignWare Ethernet PCS
> +
> +maintainers:
> + - Jose Abreu <[email protected]>
> +
> +description:
> + Synopsys DesignWare Ethernet Physical Coding Sublayer provides an interface
> + between Media Access Control and Physical Medium Attachment Sublayer through
> + the Media Independent Interface (XGMII, USXGMII, XLGMII, GMII, etc)
> + controlled by means of the IEEE std. Clause 45 registers set. The PCS can be
> + optionally synthesized with a vendor-specific interface connected to
> + Synopsys PMA (also called DesignWare Consumer/Enterprise PHY) although in
> + general it can be used to communicate with any compatible PHY.
> +
> +properties:
> + compatible:
> + oneOf:
> + - description: Synopsys DesignWare XPCS with none or unknown PMA
> + const: snps,dw-xpcs
> + - description: Synopsys DesignWare XPCS with Consumer Gen1 3G PMA
> + const: snps,dw-xpcs-gen1-3g
> + - description: Synopsys DesignWare XPCS with Consumer Gen2 3G PMA
> + const: snps,dw-xpcs-gen2-3g
> + - description: Synopsys DesignWare XPCS with Consumer Gen2 6G PMA
> + const: snps,dw-xpcs-gen2-6g
> + - description: Synopsys DesignWare XPCS with Consumer Gen4 3G PMA
> + const: snps,dw-xpcs-gen4-3g
> + - description: Synopsys DesignWare XPCS with Consumer Gen4 6G PMA
> + const: snps,dw-xpcs-gen4-6g
> + - description: Synopsys DesignWare XPCS with Consumer Gen5 10G PMA
> + const: snps,dw-xpcs-gen5-10g
> + - description: Synopsys DesignWare XPCS with Consumer Gen5 12G PMA
> + const: snps,dw-xpcs-gen5-12g
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + description:
> + System interface interrupt output (sbd_intr_o) indicating Clause 73/37
> + auto-negotiation events like':' Page received, AN is completed or
like':' ?
> + incompatible link partner.
> + maxItems: 1
> +
> + clocks:
> + description:
> + PCS/PMA interface be can clocked either by internal reference clock
> + source or by an externally connected (via a pad) clock generator.
> + minItems: 1
> + maxItems: 2
> +
> + clock-names:
> + minItems: 1
> + maxItems: 2
> + items:
> + enum: [ core, pad ]
> +
> +required:
> + - compatible
> + - reg
Don't you always need a clock?
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> +
> + mdio-bus {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + xgmac_pcs: ethernet-pcs@0 {
> + compatible = "snps,dw-xpcs";
> + reg = <0>;
> +
> + interrupts = <79 IRQ_TYPE_LEVEL_HIGH>;
> +
> + clocks = <&ccu_core>, <&ccu_pad>;
> + clock-names = "core", "pad";
> + };
> + };
> +...
> diff --git a/Documentation/devicetree/bindings/net/snps,dw-xpcs-mi.yaml b/Documentation/devicetree/bindings/net/snps,dw-xpcs-mi.yaml
> new file mode 100644
> index 000000000000..67ddba9d61fd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/snps,dw-xpcs-mi.yaml
> @@ -0,0 +1,88 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/snps,dw-xpcs-mi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Synopsys DesignWare Ethernet PCS Management Interface
> +
> +maintainers:
> + - Serge Semin <[email protected]>
> +
> +description:
> + Synopsys DesignWare Ethernet PCS provides an interface between MAC and PMA
> + through the Media Independent Interface. The PCS CSRs can be accessible over
> + the Ethernet MDIO bus or directly by means of the APB3/MCI interfaces. In the
> + later case the XPCS can be mapped right to the system IO memory space.
> +
> +allOf:
> + - $ref: mdio.yaml#
> +
> +properties:
> + compatible:
> + const: snps,dw-xpcs-mi
> +
> + reg:
> + items:
> + - description:
> + DW XPCS CSRs space can be either 'directly' or 'indirectly'
> + accessible. In the former case all Clause 45 registers are
> + contiguously mapped within the address space MMD '[20:16]',
> + Reg '[15:0]'. In the later case the space is divided to the
> + multiple 256 register sets. There is a special viewport CSR
> + which is responsible for the set selection. The upper part of
> + the CSR address is supposed to be written in there thus the
> + corresponding subset would be mapped over the lowest 255 CSRs.
> +
> + reg-names:
> + items:
> + - enum: [ direct, indirect ]
> +
> + reg-io-width:
> + description:
> + The way the CSRs are mapped to the memory is platform depended. Since
dependent
> + each Clause 45 CSR is of 16-bits wide the access instructions must be
> + two bytes aligned at least.
> + default: 2
> + enum: [ 2, 4 ]
> +
> + clocks:
> + items:
> + - description: Peripheral MCI/APB3 bus clock source
> +
> + clock-names:
> + items:
> + - const: pclk
> +
> +patternProperties:
> + 'ethernet-pcs@[0-9a-f]+$':
> + type: object
> +
> + $ref: pcs/snps,dw-xpcs.yaml#
This causes dw-xpcs to be validated twice. Does this MDIO bus support
other devices on it or it is fixed config?
> +
> +required:
> + - compatible
> + - reg
> + - reg-names
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + mdio@1f05d000 {
> + compatible = "snps,dw-xpcs-mi";
> + reg = <0x1f05d000 0x1000>;
> + reg-names = "indirect";
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + clocks = <&ccu_pclk>;
> + clock-names = "pclk";
> +
> + reg-io-width = <4>;
> +
> + ethernet-pcs@0 {
> + compatible = "snps,dw-xpcs";
> + reg = <0>;
> + };
> + };
> --
> 2.42.1
>
Hi Rob
On Thu, Dec 14, 2023 at 11:40:58AM -0600, Rob Herring wrote:
> On Tue, Dec 05, 2023 at 01:35:29PM +0300, Serge Semin wrote:
> > Synopsys DesignWare XPCS IP-core is a Physical Coding Sublayer (PCS) layer
> > providing an interface between the Media Access Control (MAC) and Physical
> > Medium Attachment Sublayer (PMA) through a Media independent interface.
> > >From software point of view it exposes IEEE std. Clause 45 CSR space and
> > can be accessible either by MDIO or MCI/APB3 bus interfaces. The later
> > case is described by means of a dedicated DT-bindings which imply having
> > the DW XPCS Management Interface defined as a DT-supernode which child the
> > PCSs nodes would be (in the same way as the standard MDIO buses and
> > devices are normally defined).
> >
> > Besides of that DW XPCS DT-nodes can have the interrupts and clock source
> > properties specified. The former one indicates the Clause 73/37
> > auto-negotiation events like: negotiation page received, AN is completed
> > or incompatible link partner. The clock DT-properties can describe up to
> > two clock sources: internal one and the one connected to the chip pad.
> > Either of them is supposed to be used as the device reference clocks.
> >
> > Finally the DW XPCS IP-core can be optionally synthesized with a
> > vendor-specific interface connected to Synopsys PMA (also called
> > DesignWare Consumer/Enterprise PHY). Alas that isn't auto-detectable
> > anyhow so if the DW XPCS device has the respective PMA attached then it
> > should be reflected in the DT-node compatible string so the driver would
> > be aware of the PMA-specific device capabilities (mainly connected with
> > CSRs available for the fine-tunings).
> >
> > Signed-off-by: Serge Semin <[email protected]>
> > ---
> > .../bindings/net/pcs/snps,dw-xpcs.yaml | 88 +++++++++++++++++++
> > .../bindings/net/snps,dw-xpcs-mi.yaml | 88 +++++++++++++++++++
> > 2 files changed, 176 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/net/pcs/snps,dw-xpcs.yaml
> > create mode 100644 Documentation/devicetree/bindings/net/snps,dw-xpcs-mi.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/net/pcs/snps,dw-xpcs.yaml b/Documentation/devicetree/bindings/net/pcs/snps,dw-xpcs.yaml
> > new file mode 100644
> > index 000000000000..9694ef51abad
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/pcs/snps,dw-xpcs.yaml
> > @@ -0,0 +1,88 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/pcs/snps,dw-xpcs.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Synopsys DesignWare Ethernet PCS
> > +
> > +maintainers:
> > + - Jose Abreu <[email protected]>
> > +
> > +description:
> > + Synopsys DesignWare Ethernet Physical Coding Sublayer provides an interface
> > + between Media Access Control and Physical Medium Attachment Sublayer through
> > + the Media Independent Interface (XGMII, USXGMII, XLGMII, GMII, etc)
> > + controlled by means of the IEEE std. Clause 45 registers set. The PCS can be
> > + optionally synthesized with a vendor-specific interface connected to
> > + Synopsys PMA (also called DesignWare Consumer/Enterprise PHY) although in
> > + general it can be used to communicate with any compatible PHY.
> > +
> > +properties:
> > + compatible:
> > + oneOf:
> > + - description: Synopsys DesignWare XPCS with none or unknown PMA
> > + const: snps,dw-xpcs
> > + - description: Synopsys DesignWare XPCS with Consumer Gen1 3G PMA
> > + const: snps,dw-xpcs-gen1-3g
> > + - description: Synopsys DesignWare XPCS with Consumer Gen2 3G PMA
> > + const: snps,dw-xpcs-gen2-3g
> > + - description: Synopsys DesignWare XPCS with Consumer Gen2 6G PMA
> > + const: snps,dw-xpcs-gen2-6g
> > + - description: Synopsys DesignWare XPCS with Consumer Gen4 3G PMA
> > + const: snps,dw-xpcs-gen4-3g
> > + - description: Synopsys DesignWare XPCS with Consumer Gen4 6G PMA
> > + const: snps,dw-xpcs-gen4-6g
> > + - description: Synopsys DesignWare XPCS with Consumer Gen5 10G PMA
> > + const: snps,dw-xpcs-gen5-10g
> > + - description: Synopsys DesignWare XPCS with Consumer Gen5 12G PMA
> > + const: snps,dw-xpcs-gen5-12g
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + interrupts:
> > + description:
> > + System interface interrupt output (sbd_intr_o) indicating Clause 73/37
> > + auto-negotiation events like':' Page received, AN is completed or
>
> like':' ?
Right. I'll drop it.
>
> > + incompatible link partner.
> > + maxItems: 1
> > +
> > + clocks:
> > + description:
> > + PCS/PMA interface be can clocked either by internal reference clock
s/be can/can be
> > + source or by an externally connected (via a pad) clock generator.
> > + minItems: 1
> > + maxItems: 2
> > +
> > + clock-names:
> > + minItems: 1
> > + maxItems: 2
> > + items:
> > + enum: [ core, pad ]
> > +
> > +required:
> > + - compatible
> > + - reg
>
> Don't you always need a clock?
It depends on the PMA nature. Synopsys PHY requires either of the two
clocks: alt/core clock or pad clock. Both of them might be supplied on
a platform though, but only one can be selected at a time. It's done
by means of the Synopsys PHY-specific CSR exposed in the MMD 1
(PMA/PMD). If there is non-Synopsys PHY (PMA) attached then I guess it
can be also clocked somehow, but it will be platform-depended.
>
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/interrupt-controller/irq.h>
> > +
> > + mdio-bus {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + xgmac_pcs: ethernet-pcs@0 {
> > + compatible = "snps,dw-xpcs";
> > + reg = <0>;
> > +
> > + interrupts = <79 IRQ_TYPE_LEVEL_HIGH>;
> > +
> > + clocks = <&ccu_core>, <&ccu_pad>;
> > + clock-names = "core", "pad";
> > + };
> > + };
> > +...
> > diff --git a/Documentation/devicetree/bindings/net/snps,dw-xpcs-mi.yaml b/Documentation/devicetree/bindings/net/snps,dw-xpcs-mi.yaml
> > new file mode 100644
> > index 000000000000..67ddba9d61fd
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/snps,dw-xpcs-mi.yaml
> > @@ -0,0 +1,88 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/snps,dw-xpcs-mi.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Synopsys DesignWare Ethernet PCS Management Interface
> > +
> > +maintainers:
> > + - Serge Semin <[email protected]>
> > +
> > +description:
> > + Synopsys DesignWare Ethernet PCS provides an interface between MAC and PMA
> > + through the Media Independent Interface. The PCS CSRs can be accessible over
> > + the Ethernet MDIO bus or directly by means of the APB3/MCI interfaces. In the
> > + later case the XPCS can be mapped right to the system IO memory space.
> > +
> > +allOf:
> > + - $ref: mdio.yaml#
> > +
> > +properties:
> > + compatible:
> > + const: snps,dw-xpcs-mi
> > +
> > + reg:
> > + items:
> > + - description:
> > + DW XPCS CSRs space can be either 'directly' or 'indirectly'
> > + accessible. In the former case all Clause 45 registers are
> > + contiguously mapped within the address space MMD '[20:16]',
> > + Reg '[15:0]'. In the later case the space is divided to the
> > + multiple 256 register sets. There is a special viewport CSR
> > + which is responsible for the set selection. The upper part of
> > + the CSR address is supposed to be written in there thus the
> > + corresponding subset would be mapped over the lowest 255 CSRs.
> > +
> > + reg-names:
> > + items:
> > + - enum: [ direct, indirect ]
> > +
> > + reg-io-width:
> > + description:
> > + The way the CSRs are mapped to the memory is platform depended. Since
>
> dependent
Ok.
>
> > + each Clause 45 CSR is of 16-bits wide the access instructions must be
> > + two bytes aligned at least.
> > + default: 2
> > + enum: [ 2, 4 ]
> > +
> > + clocks:
> > + items:
> > + - description: Peripheral MCI/APB3 bus clock source
> > +
> > + clock-names:
> > + items:
> > + - const: pclk
> > +
> > +patternProperties:
> > + 'ethernet-pcs@[0-9a-f]+$':
> > + type: object
> > +
> > + $ref: pcs/snps,dw-xpcs.yaml#
>
> This causes dw-xpcs to be validated twice. Does this MDIO bus support
> other devices on it or it is fixed config?
It turned out I was wrong to define the DW XPCS interface as the MDIO
bus. DW XPCS can be synthesized with one of the next management
interfaces:
1. MDIO - normal serial MDIO-bus interface.
2-3. MCI/APB3 - parallel interfaces so the PCS device can be easier
embedded into the system memory bus.
Initially I thought that more than one device can be accessible over
the same MCI/APB3 port with the MS bits being used to reach particular
device. But just recently I discovered that it wasn't correct. The
port_id_i[4:0] input signal isn't present for the MCI or APB3
interface. Thus the DW XPCS device is just a normal memory-mapped
platform device with no such thing like DW XPCS MI above. I'll change
the currently implemented hierarchical device representation from:
mdio@1f05d000 {
compatible = "snps,dwmac-mi";
reg = <0 0x1f05d000 0 0x1000>;
xgmac_pcs: ethernet-pcs@0 {
compatible = "snps,dw-xpcs";
reg = <0>;
};
};
to just a single node:
xgmac_pcs: ethernet-pcs@0 {
compatible = "snps,dw-xpcs";
reg = <0 0x1f05d000 0 0x1000>;
...
};
I pointed that out earlier today in a comment to another patch of this
series:
https://lore.kernel.org/netdev/xhj7jchcv63y2bmnedxqffnmh3fvdxirccdugnnljruemuiurz@ceafs7mivbqp/
-Serge(y)
>
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - reg-names
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > + - |
> > + mdio@1f05d000 {
> > + compatible = "snps,dw-xpcs-mi";
> > + reg = <0x1f05d000 0x1000>;
> > + reg-names = "indirect";
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + clocks = <&ccu_pclk>;
> > + clock-names = "pclk";
> > +
> > + reg-io-width = <4>;
> > +
> > + ethernet-pcs@0 {
> > + compatible = "snps,dw-xpcs";
> > + reg = <0>;
> > + };
> > + };
> > --
> > 2.42.1
> >