[RFC PATCH 0/4] Add support for QMC HDLC and PHY

[RFC PATCH 0/4] Add support for QMC HDLC and PHY

[Herve Codina]

Hi,

I have a system where I need to handle an HDLC interface.

The HDLC data are transferred using a TDM bus on which a PEF2256 is
present. The PEF2256 transfers data from/to the TDM bus to/from E1 line.
This PEF2256 is also connected to a PowerQUICC SoC for the control path
and the TDM is connected to the SoC (QMC component) for the data path.

From the HDLC driver, I need to handle data using the QMC and carrier
detection using the PEF2256 (E1 line carrier).

The HDLC driver consider the PEF2256 as a generic PHY.
So, the design is the following:

+----------+          +-------------+              +---------+
| HDLC drv | <-data-> | QMC channel | <-- TDM -->  | PEF2256 |
+----------+          +-------------+              |         | <--> E1
   ^   +---------+     +---------+                 |         |
   +-> | Gen PHY | <-> | PEF2256 | <- local bus -> |         |
       +---------+     | PHY drv |                 +---------+
                       +---------+

In order to implement this, I had to:
 1 - Extend the generic PHY API to support get_status() and notification
     on status change.
 2 - Introduce a new kind of generic PHY named "basic phy". This PHY
     familly can provide a link status in the get_status() data.
 3 - Support the PEF2256 PHY as a "basic phy"

The purpose of this RFC series is to discuss this design.

The QMC driver code is available on linux-next. In this series:
- patch 1: driver HDLC using the QMC channel
- patch 2: Extend the generic PHY API
- patch 3: Use the "basic phy" in the HDLC driver
- patch 4: Implement the PEF2256 PHY driver

I did 2 patches for the HDLC driver in order to point the new PHY family
usage in the HDLC driver. In the end, these two patches will be squashed
and the bindings will be added.

Hope to have some feedback on this proposal.

Best regards,
Hervé

Herve Codina (4):
  net: wan: Add support for QMC HDLC
  phy: Extend API to support 'status' get and notification
  net: wan: fsl_qmc_hdlc: Add PHY support
  phy: lantiq: Add PEF2256 PHY support

 drivers/net/wan/Kconfig                 |  12 +
 drivers/net/wan/Makefile                |   1 +
 drivers/net/wan/fsl_qmc_hdlc.c          | 558 ++++++++++++++++++++++++
 drivers/phy/lantiq/Kconfig              |  15 +
 drivers/phy/lantiq/Makefile             |   1 +
 drivers/phy/lantiq/phy-lantiq-pef2256.c | 131 ++++++
 drivers/phy/phy-core.c                  |  88 ++++
 include/linux/phy/phy-basic.h           |  27 ++
 include/linux/phy/phy.h                 |  89 +++-
 9 files changed, 921 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/wan/fsl_qmc_hdlc.c
 create mode 100644 drivers/phy/lantiq/phy-lantiq-pef2256.c
 create mode 100644 include/linux/phy/phy-basic.h

-- 
2.39.2


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[RFC PATCH 1/4] net: wan: Add support for QMC HDLC

[Herve Codina]

The QMC HDLC driver provides support for HDLC using the QMC (QUICC
Multichannel Controller) to transfer the HDLC data.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/net/wan/Kconfig        |  12 +
 drivers/net/wan/Makefile       |   1 +
 drivers/net/wan/fsl_qmc_hdlc.c | 408 +++++++++++++++++++++++++++++++++
 3 files changed, 421 insertions(+)
 create mode 100644 drivers/net/wan/fsl_qmc_hdlc.c

diff --git a/drivers/net/wan/Kconfig b/drivers/net/wan/Kconfig
index dcb069dde66b..8de99f4b647b 100644
--- a/drivers/net/wan/Kconfig
+++ b/drivers/net/wan/Kconfig
@@ -195,6 +195,18 @@ config FARSYNC
 	  To compile this driver as a module, choose M here: the
 	  module will be called farsync.
 
+config FSL_QMC_HDLC
+	tristate "Freescale QMC HDLC support"
+	depends on HDLC
+	depends on CPM_QMC
+	help
+	  HDLC support using the Freescale QUICC Multichannel Controller (QMC).
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called fsl_qmc_hdlc.
+
+	  If unsure, say N.
+
 config FSL_UCC_HDLC
 	tristate "Freescale QUICC Engine HDLC support"
 	depends on HDLC
diff --git a/drivers/net/wan/Makefile b/drivers/net/wan/Makefile
index 5bec8fae47f8..f338f4830626 100644
--- a/drivers/net/wan/Makefile
+++ b/drivers/net/wan/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_WANXL)		+= wanxl.o
 obj-$(CONFIG_PCI200SYN)		+= pci200syn.o
 obj-$(CONFIG_PC300TOO)		+= pc300too.o
 obj-$(CONFIG_IXP4XX_HSS)	+= ixp4xx_hss.o
+obj-$(CONFIG_FSL_QMC_HDLC)	+= fsl_qmc_hdlc.o
 obj-$(CONFIG_FSL_UCC_HDLC)	+= fsl_ucc_hdlc.o
 obj-$(CONFIG_SLIC_DS26522)	+= slic_ds26522.o
 
diff --git a/drivers/net/wan/fsl_qmc_hdlc.c b/drivers/net/wan/fsl_qmc_hdlc.c
new file mode 100644
index 000000000000..f12d00c78497
--- /dev/null
+++ b/drivers/net/wan/fsl_qmc_hdlc.c
@@ -0,0 +1,408 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Freescale QMC HDLC Device Driver
+ *
+ * Copyright 2023 CS GROUP France
+ *
+ * Author: Herve Codina <herve.codina@bootlin.com>
+ */
+
+#include <linux/dma-mapping.h>
+#include <linux/hdlc.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <soc/fsl/qe/qmc.h>
+
+struct qmc_hdlc_desc {
+	struct net_device *netdev;
+	struct sk_buff *skb; /* NULL if the descriptor is not in use */
+	dma_addr_t dma_addr;
+	size_t dma_size;
+};
+
+struct qmc_hdlc {
+	struct device *dev;
+	struct qmc_chan *qmc_chan;
+	struct net_device *netdev;
+	bool is_crc32;
+	spinlock_t tx_lock; /* Protect tx descriptors */
+	struct qmc_hdlc_desc tx_descs[8];
+	unsigned int tx_out;
+	struct qmc_hdlc_desc rx_descs[4];
+};
+
+static inline struct qmc_hdlc *netdev_to_qmc_hdlc(struct net_device *netdev)
+{
+	return (struct qmc_hdlc *)dev_to_hdlc(netdev)->priv;
+}
+
+static int qmc_hdlc_recv_queue(struct qmc_hdlc *qmc_hdlc, struct qmc_hdlc_desc *desc, size_t size);
+
+static void qmc_hcld_recv_complete(void *context, size_t length)
+{
+	struct qmc_hdlc_desc *desc = context;
+	struct net_device *netdev = desc->netdev;
+	struct qmc_hdlc *qmc_hdlc = netdev_to_qmc_hdlc(desc->netdev);
+	int ret;
+
+	dma_unmap_single(qmc_hdlc->dev, desc->dma_addr, desc->dma_size, DMA_FROM_DEVICE);
+
+	netdev->stats.rx_packets++;
+	netdev->stats.rx_bytes += length;
+
+	skb_put(desc->skb, length);
+	desc->skb->protocol = hdlc_type_trans(desc->skb, netdev);
+	netif_rx(desc->skb);
+
+	/* Re-queue a transfer using the same descriptor */
+	ret = qmc_hdlc_recv_queue(qmc_hdlc, desc, desc->dma_size);
+	if (ret) {
+		dev_err(qmc_hdlc->dev, "queue recv desc failed (%d)\n", ret);
+		netdev->stats.rx_errors++;
+	}
+}
+
+static int qmc_hdlc_recv_queue(struct qmc_hdlc *qmc_hdlc, struct qmc_hdlc_desc *desc, size_t size)
+{
+	int ret;
+
+	desc->skb = dev_alloc_skb(size);
+	if (!desc->skb)
+		return -ENOMEM;
+
+	desc->dma_size = size;
+	desc->dma_addr = dma_map_single(qmc_hdlc->dev, desc->skb->data,
+					desc->dma_size, DMA_FROM_DEVICE);
+	ret = dma_mapping_error(qmc_hdlc->dev, desc->dma_addr);
+	if (ret)
+		goto free_skb;
+
+	ret = qmc_chan_read_submit(qmc_hdlc->qmc_chan, desc->dma_addr, desc->dma_size,
+				   qmc_hcld_recv_complete, desc);
+	if (ret)
+		goto dma_unmap;
+
+	return 0;
+
+dma_unmap:
+	dma_unmap_single(qmc_hdlc->dev, desc->dma_addr, desc->dma_size, DMA_FROM_DEVICE);
+free_skb:
+	kfree_skb(desc->skb);
+	desc->skb = NULL;
+	return ret;
+}
+
+static void qmc_hdlc_xmit_complete(void *context)
+{
+	struct qmc_hdlc_desc *desc = context;
+	struct net_device *netdev = desc->netdev;
+	struct qmc_hdlc *qmc_hdlc = netdev_to_qmc_hdlc(netdev);
+	struct sk_buff *skb;
+	unsigned long flags;
+
+	spin_lock_irqsave(&qmc_hdlc->tx_lock, flags);
+	dma_unmap_single(qmc_hdlc->dev, desc->dma_addr, desc->dma_size, DMA_TO_DEVICE);
+	skb = desc->skb;
+	desc->skb = NULL; /* Release the descriptor */
+	if (netif_queue_stopped(netdev))
+		netif_wake_queue(netdev);
+	spin_unlock_irqrestore(&qmc_hdlc->tx_lock, flags);
+
+	netdev->stats.tx_packets++;
+	netdev->stats.tx_bytes += skb->len;
+
+	dev_consume_skb_any(skb);
+}
+
+static int qmc_hdlc_xmit_queue(struct qmc_hdlc *qmc_hdlc, struct qmc_hdlc_desc *desc)
+{
+	int ret;
+
+	desc->dma_addr = dma_map_single(qmc_hdlc->dev, desc->skb->data,
+					desc->dma_size, DMA_TO_DEVICE);
+	ret = dma_mapping_error(qmc_hdlc->dev, desc->dma_addr);
+	if (ret) {
+		dev_err(qmc_hdlc->dev, "failed to map skb\n");
+		return ret;
+	}
+
+	ret = qmc_chan_write_submit(qmc_hdlc->qmc_chan, desc->dma_addr, desc->dma_size,
+				    qmc_hdlc_xmit_complete, desc);
+	if (ret) {
+		dev_err(qmc_hdlc->dev, "qmc chan write returns %d\n", ret);
+		dma_unmap_single(qmc_hdlc->dev, desc->dma_addr, desc->dma_size, DMA_TO_DEVICE);
+		return ret;
+	}
+
+	return 0;
+}
+
+static netdev_tx_t qmc_hdlc_xmit(struct sk_buff *skb, struct net_device *netdev)
+{
+	struct qmc_hdlc *qmc_hdlc = netdev_to_qmc_hdlc(netdev);
+	struct qmc_hdlc_desc *desc;
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&qmc_hdlc->tx_lock, flags);
+	desc = &qmc_hdlc->tx_descs[qmc_hdlc->tx_out];
+	if (desc->skb) {
+		/* Should never happen.
+		 * Previous xmit should have already stopped the queue.
+		 */
+		netif_stop_queue(netdev);
+		spin_unlock_irqrestore(&qmc_hdlc->tx_lock, flags);
+		return NETDEV_TX_BUSY;
+	}
+	spin_unlock_irqrestore(&qmc_hdlc->tx_lock, flags);
+
+	desc->netdev = netdev;
+	desc->dma_size = skb->len;
+	desc->skb = skb;
+	ret = qmc_hdlc_xmit_queue(qmc_hdlc, desc);
+	if (ret) {
+		desc->skb = NULL; /* Release the descriptor */
+		if (ret == -EBUSY) {
+			netif_stop_queue(netdev);
+			return NETDEV_TX_BUSY;
+		}
+		dev_kfree_skb(skb);
+		netdev->stats.tx_dropped++;
+		return NETDEV_TX_OK;
+	}
+
+	qmc_hdlc->tx_out = (qmc_hdlc->tx_out + 1) % ARRAY_SIZE(qmc_hdlc->tx_descs);
+
+	spin_lock_irqsave(&qmc_hdlc->tx_lock, flags);
+	if (qmc_hdlc->tx_descs[qmc_hdlc->tx_out].skb)
+		netif_stop_queue(netdev);
+	spin_unlock_irqrestore(&qmc_hdlc->tx_lock, flags);
+
+	return NETDEV_TX_OK;
+}
+
+static int qmc_hdlc_open(struct net_device *netdev)
+{
+	struct qmc_hdlc *qmc_hdlc = netdev_to_qmc_hdlc(netdev);
+	struct qmc_chan_param chan_param;
+	struct qmc_hdlc_desc *desc;
+	int ret;
+	int i;
+
+	ret = hdlc_open(netdev);
+	if (ret)
+		return ret;
+
+	chan_param.mode = QMC_HDLC;
+	/* HDLC_MAX_MRU + 4 for the CRC
+	 * HDLC_MAX_MRU + 4 + 8 for the CRC and some extraspace needed by the QMC
+	 */
+	chan_param.hdlc.max_rx_buf_size = HDLC_MAX_MRU + 4 + 8;
+	chan_param.hdlc.max_rx_frame_size = HDLC_MAX_MRU + 4;
+	chan_param.hdlc.is_crc32 = qmc_hdlc->is_crc32;
+	ret = qmc_chan_set_param(qmc_hdlc->qmc_chan, &chan_param);
+	if (ret) {
+		dev_err(qmc_hdlc->dev, "failed to set param (%d)\n", ret);
+		goto hdlc_close;
+	}
+
+	/* Queue as many recv descriptors as possible */
+	for (i = 0; i < ARRAY_SIZE(qmc_hdlc->rx_descs); i++) {
+		desc = &qmc_hdlc->rx_descs[i];
+
+		desc->netdev = netdev;
+		ret = qmc_hdlc_recv_queue(qmc_hdlc, desc, chan_param.hdlc.max_rx_buf_size);
+		if (ret) {
+			if (ret == -EBUSY && i != 0)
+				break; /* We use all the QMC chan capability */
+			goto free_desc;
+		}
+	}
+
+	ret = qmc_chan_start(qmc_hdlc->qmc_chan, QMC_CHAN_ALL);
+	if (ret) {
+		dev_err(qmc_hdlc->dev, "qmc chan start failed (%d)\n", ret);
+		goto free_desc;
+	}
+
+	netif_start_queue(netdev);
+
+	return 0;
+
+free_desc:
+	qmc_chan_reset(qmc_hdlc->qmc_chan, QMC_CHAN_ALL);
+	for (i = 0; i < ARRAY_SIZE(qmc_hdlc->rx_descs); i++) {
+		desc = &qmc_hdlc->rx_descs[i];
+		if (!desc->skb)
+			continue;
+		dma_unmap_single(qmc_hdlc->dev, desc->dma_addr, desc->dma_size,
+				 DMA_FROM_DEVICE);
+		kfree_skb(desc->skb);
+		desc->skb = NULL;
+	}
+hdlc_close:
+	hdlc_close(netdev);
+	return ret;
+}
+
+static int qmc_hdlc_close(struct net_device *netdev)
+{
+	struct qmc_hdlc *qmc_hdlc = netdev_to_qmc_hdlc(netdev);
+	struct qmc_hdlc_desc *desc;
+	int i;
+
+	netif_stop_queue(netdev);
+
+	qmc_chan_stop(qmc_hdlc->qmc_chan, QMC_CHAN_ALL);
+	qmc_chan_reset(qmc_hdlc->qmc_chan, QMC_CHAN_ALL);
+
+	for (i = 0; i < ARRAY_SIZE(qmc_hdlc->tx_descs); i++) {
+		desc = &qmc_hdlc->tx_descs[i];
+		if (!desc->skb)
+			continue;
+		dma_unmap_single(qmc_hdlc->dev, desc->dma_addr, desc->dma_size,
+				 DMA_TO_DEVICE);
+		kfree_skb(desc->skb);
+		desc->skb = NULL;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(qmc_hdlc->rx_descs); i++) {
+		desc = &qmc_hdlc->rx_descs[i];
+		if (!desc->skb)
+			continue;
+		dma_unmap_single(qmc_hdlc->dev, desc->dma_addr, desc->dma_size,
+				 DMA_FROM_DEVICE);
+		kfree_skb(desc->skb);
+		desc->skb = NULL;
+	}
+
+	hdlc_close(netdev);
+	return 0;
+}
+
+static int qmc_hdlc_attach(struct net_device *netdev, unsigned short encoding,
+			   unsigned short parity)
+{
+	struct qmc_hdlc *qmc_hdlc = netdev_to_qmc_hdlc(netdev);
+
+	if (encoding != ENCODING_NRZ)
+		return -EINVAL;
+
+	switch (parity) {
+	case PARITY_CRC16_PR1_CCITT:
+		qmc_hdlc->is_crc32 = false;
+		break;
+	case PARITY_CRC32_PR1_CCITT:
+		qmc_hdlc->is_crc32 = true;
+		break;
+	default:
+		dev_err(qmc_hdlc->dev, "unsupported parity %u\n", parity);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static const struct net_device_ops qmc_hdlc_netdev_ops = {
+	.ndo_open       = qmc_hdlc_open,
+	.ndo_stop       = qmc_hdlc_close,
+	.ndo_start_xmit = hdlc_start_xmit,
+	.ndo_siocwandev	= hdlc_ioctl,
+};
+
+static int qmc_hdlc_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct qmc_hdlc *qmc_hdlc;
+	struct qmc_chan_info info;
+	hdlc_device *hdlc;
+	int ret;
+
+	qmc_hdlc = devm_kzalloc(&pdev->dev, sizeof(*qmc_hdlc), GFP_KERNEL);
+	if (!qmc_hdlc)
+		return -ENOMEM;
+
+	qmc_hdlc->dev = &pdev->dev;
+	spin_lock_init(&qmc_hdlc->tx_lock);
+
+	qmc_hdlc->qmc_chan = devm_qmc_chan_get_byphandle(qmc_hdlc->dev, np, "fsl,qmc-chan");
+	if (IS_ERR(qmc_hdlc->qmc_chan)) {
+		ret = PTR_ERR(qmc_hdlc->qmc_chan);
+		return dev_err_probe(qmc_hdlc->dev, ret, "get QMC channel failed\n");
+	}
+
+	ret = qmc_chan_get_info(qmc_hdlc->qmc_chan, &info);
+	if (ret) {
+		dev_err(qmc_hdlc->dev, "get QMC channel info failed %d\n", ret);
+		return ret;
+	}
+	dev_info(qmc_hdlc->dev, "QMC channel mode %d, nb_tx_ts %u, nb_rx_ts %u\n",
+		 info.mode, info.nb_tx_ts, info.nb_rx_ts);
+
+	if (info.mode != QMC_HDLC) {
+		dev_err(qmc_hdlc->dev, "QMC chan mode %d is not QMC_HDLC\n",
+			info.mode);
+		return -EINVAL;
+	}
+
+	qmc_hdlc->netdev = alloc_hdlcdev(qmc_hdlc);
+	if (!qmc_hdlc->netdev) {
+		dev_err(qmc_hdlc->dev, "failed to alloc hdlc dev\n");
+		return -ENOMEM;
+	}
+
+	hdlc = dev_to_hdlc(qmc_hdlc->netdev);
+	hdlc->attach = qmc_hdlc_attach;
+	hdlc->xmit = qmc_hdlc_xmit;
+	SET_NETDEV_DEV(qmc_hdlc->netdev, qmc_hdlc->dev);
+	qmc_hdlc->netdev->tx_queue_len = ARRAY_SIZE(qmc_hdlc->tx_descs);
+	qmc_hdlc->netdev->netdev_ops = &qmc_hdlc_netdev_ops;
+	ret = register_hdlc_device(qmc_hdlc->netdev);
+	if (ret) {
+		dev_err(qmc_hdlc->dev, "failed to register hdlc device (%d)\n", ret);
+		goto free_netdev;
+	}
+
+	platform_set_drvdata(pdev, qmc_hdlc);
+
+	dev_info(qmc_hdlc->dev, "probed\n");
+
+	return 0;
+
+free_netdev:
+	free_netdev(qmc_hdlc->netdev);
+	return ret;
+}
+
+static int qmc_hdlc_remove(struct platform_device *pdev)
+{
+	struct qmc_hdlc *qmc_hdlc = platform_get_drvdata(pdev);
+
+	unregister_hdlc_device(qmc_hdlc->netdev);
+	free_netdev(qmc_hdlc->netdev);
+
+	return 0;
+}
+
+static const struct of_device_id qmc_hdlc_id_table[] = {
+	{ .compatible = "fsl,qmc-hdlc" },
+	{} /* sentinel */
+};
+MODULE_DEVICE_TABLE(of, qmc_hdlc_driver);
+
+static struct platform_driver qmc_hdlc_driver = {
+	.driver = {
+		.name = "fsl-qmc-hdlc",
+		.of_match_table = qmc_hdlc_id_table,
+	},
+	.probe = qmc_hdlc_probe,
+	.remove = qmc_hdlc_remove,
+};
+module_platform_driver(qmc_hdlc_driver);
+
+MODULE_AUTHOR("Herve Codina <herve.codina@bootlin.com>");
+MODULE_DESCRIPTION("QMC HDLC driver");
+MODULE_LICENSE("GPL");
-- 
2.39.2


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[RFC PATCH 2/4] phy: Extend API to support 'status' get and notification

[Herve Codina]

The PHY API provides functions to control and pass information from the
PHY consumer to the PHY provider.
There is no way for the consumer to get direct information from the PHY
or be notified by the PHY.

To fill this hole, two API function are provided:

- phy_get_status()
  This function can be used to get a "status" from the PHY. It is built
  as the same ways as the configure() function. The status information
  present in the status retrieved depends on the PHY's phy_mode.
  This allows to get a "status" depending on the kind of PHY.

- phy_atomic_notifier_(un)register()
  These functions can be used to register/unregister an atomic notifier
  block. The event available at this time is the PHY_EVENT_STATUS status
  event which purpose is to signal some changes in the status available
  using phy_get_status().

An new kind of PHY is added: PHY_MODE_BASIC.
This new kind of PHY represents a basic PHY offering a basic status This
status contains a link state indication.
With the new API, a link state indication can be retrieve using
phy_get_status() and link state changes can be notified.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/phy/phy-core.c        | 88 ++++++++++++++++++++++++++++++++++
 include/linux/phy/phy-basic.h | 27 +++++++++++
 include/linux/phy/phy.h       | 89 ++++++++++++++++++++++++++++++++++-
 3 files changed, 203 insertions(+), 1 deletion(-)
 create mode 100644 include/linux/phy/phy-basic.h

diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index 9951efc03eaa..c7b568b99dce 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -551,6 +551,94 @@ int phy_validate(struct phy *phy, enum phy_mode mode, int submode,
 }
 EXPORT_SYMBOL_GPL(phy_validate);
 
+/**
+ * phy_get_status() - Gets the phy status
+ * @phy: the phy returned by phy_get()
+ * @status: the status to retrieve
+ *
+ * Used to get the PHY status. phy_init() must have been called
+ * on the phy. The status will be retrieved from the current phy mode,
+ * that can be changed using phy_set_mode().
+ *
+ * Return: %0 if successful, a negative error code otherwise
+ */
+int phy_get_status(struct phy *phy, union phy_status *status)
+{
+	int ret;
+
+	if (!phy)
+		return -EINVAL;
+
+	if (!phy->ops->get_status)
+		return -EOPNOTSUPP;
+
+	mutex_lock(&phy->mutex);
+	ret = phy->ops->get_status(phy, status);
+	mutex_unlock(&phy->mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(phy_get_status);
+
+/**
+ * phy_atomic_notifier_register() - Registers an atomic notifier
+ * @phy: the phy returned by phy_get()
+ * @nb: the notifier block to register
+ *
+ * Used to register a notifier block on PHY events. phy_init() must have
+ * been called on the phy.
+ * The notifier function given in the notifier_block must not sleep.
+ * The available PHY events are present in enum phy_events
+ *
+ * Return: %0 if successful, a negative error code otherwise
+ */
+int phy_atomic_notifier_register(struct phy *phy, struct notifier_block *nb)
+{
+	int ret;
+
+	if (!phy)
+		return -EINVAL;
+
+	if (!phy->ops->atomic_notifier_register ||
+	    !phy->ops->atomic_notifier_unregister)
+		return -EOPNOTSUPP;
+
+	mutex_lock(&phy->mutex);
+	ret = phy->ops->atomic_notifier_register(phy, nb);
+	mutex_unlock(&phy->mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(phy_atomic_notifier_register);
+
+/**
+ * phy_atomic_notifier_unregister() - Unregisters an atomic notifier
+ * @phy: the phy returned by phy_get()
+ * @nb: the notifier block to unregister
+ *
+ * Used to unregister a notifier block. phy_init() must have
+ * been called on the phy.
+ *
+ * Return: %0 if successful, a negative error code otherwise
+ */
+int phy_atomic_notifier_unregister(struct phy *phy, struct notifier_block *nb)
+{
+	int ret;
+
+	if (!phy)
+		return -EINVAL;
+
+	if (!phy->ops->atomic_notifier_unregister)
+		return -EOPNOTSUPP;
+
+	mutex_lock(&phy->mutex);
+	ret = phy->ops->atomic_notifier_unregister(phy, nb);
+	mutex_unlock(&phy->mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(phy_atomic_notifier_unregister);
+
 /**
  * _of_phy_get() - lookup and obtain a reference to a phy by phandle
  * @np: device_node for which to get the phy
diff --git a/include/linux/phy/phy-basic.h b/include/linux/phy/phy-basic.h
new file mode 100644
index 000000000000..95668c610c78
--- /dev/null
+++ b/include/linux/phy/phy-basic.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright 2023 CS GROUP France
+ *
+ * Author: Herve Codina <herve.codina@bootlin.com>
+ */
+
+#ifndef __PHY_BASIC_H_
+#define __PHY_BASIC_H_
+
+#include <linux/types.h>
+
+/**
+ * struct phy_status_basic - Basic PHY status
+ *
+ * This structure is used to represent the status of a Basic phy.
+ */
+struct phy_status_basic {
+	/**
+	 * @link_state:
+	 *
+	 * Link state. true, the link is on, false, the link is off.
+	 */
+	bool link_is_on;
+};
+
+#endif /* __PHY_DP_H_ */
diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index 3a570bc59fc7..40370d41012b 100644
--- a/include/linux/phy/phy.h
+++ b/include/linux/phy/phy.h
@@ -16,6 +16,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
 
+#include <linux/phy/phy-basic.h>
 #include <linux/phy/phy-dp.h>
 #include <linux/phy/phy-lvds.h>
 #include <linux/phy/phy-mipi-dphy.h>
@@ -42,7 +43,8 @@ enum phy_mode {
 	PHY_MODE_MIPI_DPHY,
 	PHY_MODE_SATA,
 	PHY_MODE_LVDS,
-	PHY_MODE_DP
+	PHY_MODE_DP,
+	PHY_MODE_BASIC,
 };
 
 enum phy_media {
@@ -67,6 +69,22 @@ union phy_configure_opts {
 	struct phy_configure_opts_lvds		lvds;
 };
 
+/**
+ * union phy_status - Opaque generic phy status
+ *
+ * @basic:	Status availbale phys supporting the Basic phy mode.
+ */
+union phy_status {
+	struct phy_status_basic		basic;
+};
+
+/**
+ * phy_event - event available for notification
+ */
+enum phy_event {
+	PHY_EVENT_STATUS,	/* Event notified on phy_status changes */
+};
+
 /**
  * struct phy_ops - set of function pointers for performing phy operations
  * @init: operation to be performed for initializing phy
@@ -120,6 +138,45 @@ struct phy_ops {
 	 */
 	int	(*validate)(struct phy *phy, enum phy_mode mode, int submode,
 			    union phy_configure_opts *opts);
+
+	/**
+	 * @get_status:
+	 *
+	 * Optional.
+	 *
+	 * Used to get the PHY status. phy_init() must have
+	 * been called on the phy.
+	 *
+	 * Returns: 0 if successful, an negative error code otherwise
+	 */
+	int	(*get_status)(struct phy *phy, union phy_status *status);
+
+	/**
+	 * @atomic_notifier_register:
+	 *
+	 * Optional.
+	 *
+	 * Used to register a notifier block on PHY events. phy_init() must have
+	 * been called on the phy.
+	 * The notifier function given in the notifier_block must not sleep.
+	 * The available PHY events are present in enum phy_events
+	 *
+	 * Returns: 0 if successful, an negative error code otherwise
+	 */
+	int	(*atomic_notifier_register)(struct phy *phy, struct notifier_block *nb);
+
+	/**
+	 * @atomic_notifier_unregister:
+	 *
+	 * Mandatoty if @atomic_notifier_register is set.
+	 *
+	 * Used to unregister a notifier block on PHY events. phy_init() must have
+	 * been called on the phy.
+	 *
+	 * Returns: 0 if successful, an negative error code otherwise
+	 */
+	int	(*atomic_notifier_unregister)(struct phy *phy, struct notifier_block *nb);
+
 	int	(*reset)(struct phy *phy);
 	int	(*calibrate)(struct phy *phy);
 	void	(*release)(struct phy *phy);
@@ -234,6 +291,10 @@ int phy_set_speed(struct phy *phy, int speed);
 int phy_configure(struct phy *phy, union phy_configure_opts *opts);
 int phy_validate(struct phy *phy, enum phy_mode mode, int submode,
 		 union phy_configure_opts *opts);
+int phy_get_status(struct phy *phy, union phy_status *status);
+int phy_atomic_notifier_register(struct phy *phy, struct notifier_block *nb);
+int phy_atomic_notifier_unregister(struct phy *phy, struct notifier_block *nb);
+
 
 static inline enum phy_mode phy_get_mode(struct phy *phy)
 {
@@ -412,6 +473,32 @@ static inline int phy_validate(struct phy *phy, enum phy_mode mode, int submode,
 	return -ENOSYS;
 }
 
+static inline int phy_get_status(struct phy *phy, union phy_status *status)
+{
+	if (!phy)
+		return 0;
+
+	return -ENOSYS;
+}
+
+static inline int phy_atomic_notifier_register(struct phy *phy,
+					       struct notifier_block *nb)
+{
+	if (!phy)
+		return 0;
+
+	return -ENOSYS;
+}
+
+static inline int phy_atomic_notifier_unregister(struct phy *phy,
+						 struct notifier_block *nb)
+{
+	if (!phy)
+		return 0;
+
+	return -ENOSYS;
+}
+
 static inline int phy_get_bus_width(struct phy *phy)
 {
 	return -ENOSYS;
-- 
2.39.2


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[RFC PATCH 3/4] net: wan: fsl_qmc_hdlc: Add PHY support

[Herve Codina]

Add PHY support in the fsl_qmc_hdlc driver in order to be able to
signal carrier changes to the network stack based on the PHY status.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/net/wan/fsl_qmc_hdlc.c | 152 ++++++++++++++++++++++++++++++++-
 1 file changed, 151 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wan/fsl_qmc_hdlc.c b/drivers/net/wan/fsl_qmc_hdlc.c
index f12d00c78497..edea0f678ffe 100644
--- a/drivers/net/wan/fsl_qmc_hdlc.c
+++ b/drivers/net/wan/fsl_qmc_hdlc.c
@@ -12,6 +12,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_platform.h>
+#include <linux/phy/phy.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <soc/fsl/qe/qmc.h>
@@ -27,6 +28,11 @@ struct qmc_hdlc {
 	struct device *dev;
 	struct qmc_chan *qmc_chan;
 	struct net_device *netdev;
+	struct phy *phy;
+	spinlock_t carrier_lock; /* Protect carrier detection */
+	struct notifier_block nb;
+	bool is_phy_notifier;
+	struct delayed_work phy_poll_task;
 	bool is_crc32;
 	spinlock_t tx_lock; /* Protect tx descriptors */
 	struct qmc_hdlc_desc tx_descs[8];
@@ -39,6 +45,126 @@ static inline struct qmc_hdlc *netdev_to_qmc_hdlc(struct net_device *netdev)
 	return (struct qmc_hdlc *)dev_to_hdlc(netdev)->priv;
 }
 
+static int qmc_hdlc_phy_set_carrier(struct qmc_hdlc *qmc_hdlc)
+{
+	union phy_status phy_status;
+	unsigned long flags;
+	int ret;
+
+	if (!qmc_hdlc->phy)
+		return 0;
+
+	spin_lock_irqsave(&qmc_hdlc->carrier_lock, flags);
+
+	ret = phy_get_status(qmc_hdlc->phy, &phy_status);
+	if (ret) {
+		dev_err(qmc_hdlc->dev, "get PHY status failed (%d)\n", ret);
+		goto end;
+	}
+	if (phy_status.basic.link_is_on)
+		netif_carrier_on(qmc_hdlc->netdev);
+	else
+		netif_carrier_off(qmc_hdlc->netdev);
+
+end:
+	spin_unlock_irqrestore(&qmc_hdlc->carrier_lock, flags);
+	return ret;
+}
+
+static int qmc_hdlc_phy_notifier(struct notifier_block *nb, unsigned long action,
+				 void *data)
+{
+	struct qmc_hdlc *qmc_hdlc = container_of(nb, struct qmc_hdlc, nb);
+	int ret;
+
+	if (action != PHY_EVENT_STATUS)
+		return NOTIFY_DONE;
+
+	ret = qmc_hdlc_phy_set_carrier(qmc_hdlc);
+	return ret ? NOTIFY_DONE : NOTIFY_OK;
+}
+
+static void qmc_hdlc_phy_poll_task(struct work_struct *work)
+{
+	struct qmc_hdlc *qmc_hdlc = container_of(work, struct qmc_hdlc, phy_poll_task.work);
+	int ret;
+
+	ret = qmc_hdlc_phy_set_carrier(qmc_hdlc);
+	if (ret) {
+		/* Should not happened but ...
+		 * On error, force carrier on and stop scheduling this task
+		 */
+		dev_err(qmc_hdlc->dev, "set carrier failed (%d) -> force carrier on\n",
+			ret);
+		netif_carrier_on(qmc_hdlc->netdev);
+		return;
+	}
+
+	/* Re-schedule task in 1 sec */
+	queue_delayed_work(system_power_efficient_wq, &qmc_hdlc->phy_poll_task, 1 * HZ);
+}
+
+static int qmc_hdlc_phy_init(struct qmc_hdlc *qmc_hdlc)
+{
+	union phy_status phy_status;
+	int ret;
+
+	if (!qmc_hdlc->phy)
+		return 0;
+
+	ret = phy_init(qmc_hdlc->phy);
+	if (ret) {
+		dev_err(qmc_hdlc->dev, "PHY init failed (%d)\n", ret);
+		return ret;
+	}
+
+	ret = phy_power_on(qmc_hdlc->phy);
+	if (ret) {
+		dev_err(qmc_hdlc->dev, "PHY power-on failed (%d)\n", ret);
+		goto phy_exit;
+	}
+
+	/* Be sure that get_status is supported */
+	ret = phy_get_status(qmc_hdlc->phy, &phy_status);
+	if (ret) {
+		dev_err(qmc_hdlc->dev, "get PHY status failed (%d)\n", ret);
+		goto phy_power_off;
+	}
+
+	qmc_hdlc->nb.notifier_call = qmc_hdlc_phy_notifier;
+	ret = phy_atomic_notifier_register(qmc_hdlc->phy, &qmc_hdlc->nb);
+	if (ret) {
+		qmc_hdlc->is_phy_notifier = false;
+
+		/* Cannot register a PHY notifier -> Use polling */
+		INIT_DELAYED_WORK(&qmc_hdlc->phy_poll_task, qmc_hdlc_phy_poll_task);
+		queue_delayed_work(system_power_efficient_wq, &qmc_hdlc->phy_poll_task, 1 * HZ);
+	} else {
+		qmc_hdlc->is_phy_notifier = true;
+	}
+
+	return 0;
+
+phy_power_off:
+	phy_power_off(qmc_hdlc->phy);
+phy_exit:
+	phy_exit(qmc_hdlc->phy);
+	return ret;
+}
+
+static void qmc_hdlc_phy_exit(struct qmc_hdlc *qmc_hdlc)
+{
+	if (!qmc_hdlc->phy)
+		return;
+
+	if (qmc_hdlc->is_phy_notifier)
+		phy_atomic_notifier_unregister(qmc_hdlc->phy, &qmc_hdlc->nb);
+	else
+		cancel_delayed_work_sync(&qmc_hdlc->phy_poll_task);
+	phy_power_off(qmc_hdlc->phy);
+	phy_exit(qmc_hdlc->phy);
+}
+
 static int qmc_hdlc_recv_queue(struct qmc_hdlc *qmc_hdlc, struct qmc_hdlc_desc *desc, size_t size);
 
 static void qmc_hcld_recv_complete(void *context, size_t length)
@@ -192,10 +318,17 @@ static int qmc_hdlc_open(struct net_device *netdev)
 	int ret;
 	int i;
 
-	ret = hdlc_open(netdev);
+	ret = qmc_hdlc_phy_init(qmc_hdlc);
 	if (ret)
 		return ret;
 
+	ret = hdlc_open(netdev);
+	if (ret)
+		goto phy_exit;
+
+	/* Update carrier */
+	qmc_hdlc_phy_set_carrier(qmc_hdlc);
+
 	chan_param.mode = QMC_HDLC;
 	/* HDLC_MAX_MRU + 4 for the CRC
 	 * HDLC_MAX_MRU + 4 + 8 for the CRC and some extraspace needed by the QMC
@@ -245,6 +378,8 @@ static int qmc_hdlc_open(struct net_device *netdev)
 	}
 hdlc_close:
 	hdlc_close(netdev);
+phy_exit:
+	qmc_hdlc_phy_exit(qmc_hdlc);
 	return ret;
 }
 
@@ -280,6 +415,7 @@ static int qmc_hdlc_close(struct net_device *netdev)
 	}
 
 	hdlc_close(netdev);
+	qmc_hdlc_phy_exit(qmc_hdlc);
 	return 0;
 }
 
@@ -318,6 +454,7 @@ static int qmc_hdlc_probe(struct platform_device *pdev)
 	struct device_node *np = pdev->dev.of_node;
 	struct qmc_hdlc *qmc_hdlc;
 	struct qmc_chan_info info;
+	enum phy_mode phy_mode;
 	hdlc_device *hdlc;
 	int ret;
 
@@ -327,6 +464,7 @@ static int qmc_hdlc_probe(struct platform_device *pdev)
 
 	qmc_hdlc->dev = &pdev->dev;
 	spin_lock_init(&qmc_hdlc->tx_lock);
+	spin_lock_init(&qmc_hdlc->carrier_lock);
 
 	qmc_hdlc->qmc_chan = devm_qmc_chan_get_byphandle(qmc_hdlc->dev, np, "fsl,qmc-chan");
 	if (IS_ERR(qmc_hdlc->qmc_chan)) {
@@ -348,6 +486,18 @@ static int qmc_hdlc_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	qmc_hdlc->phy = devm_of_phy_optional_get(qmc_hdlc->dev, np, NULL);
+	if (IS_ERR(qmc_hdlc->phy))
+		return PTR_ERR(qmc_hdlc->phy);
+	if (qmc_hdlc->phy) {
+		phy_mode = phy_get_mode(qmc_hdlc->phy);
+		if (phy_mode != PHY_MODE_BASIC) {
+			dev_err(qmc_hdlc->dev, "Unsupported PHY mode (%d)\n",
+				phy_mode);
+			return -EINVAL;
+		}
+	}
+
 	qmc_hdlc->netdev = alloc_hdlcdev(qmc_hdlc);
 	if (!qmc_hdlc->netdev) {
 		dev_err(qmc_hdlc->dev, "failed to alloc hdlc dev\n");
-- 
2.39.2


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[RFC PATCH 4/4] phy: lantiq: Add PEF2256 PHY support

[Herve Codina]

The Lantiq PEF2256, is a framer and line interface component designed to
fulfill all required interfacing between an analog E1/T1/J1 line and the
digital PCM system highway/H.100 bus.

The PHY support allows to provide the PEF2556 as a generic PHY and to
use the PHY API to retrieve the E1 line carrier status from the PHY
consumer.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/phy/lantiq/Kconfig              |  15 +++
 drivers/phy/lantiq/Makefile             |   1 +
 drivers/phy/lantiq/phy-lantiq-pef2256.c | 131 ++++++++++++++++++++++++
 3 files changed, 147 insertions(+)
 create mode 100644 drivers/phy/lantiq/phy-lantiq-pef2256.c

diff --git a/drivers/phy/lantiq/Kconfig b/drivers/phy/lantiq/Kconfig
index c4df9709d53f..c87881255458 100644
--- a/drivers/phy/lantiq/Kconfig
+++ b/drivers/phy/lantiq/Kconfig
@@ -2,6 +2,21 @@
 #
 # Phy drivers for Lantiq / Intel platforms
 #
+config PHY_LANTIQ_PEF2256
+	tristate "Lantiq PEF2256 PHY"
+	depends on MFD_PEF2256
+	select GENERIC_PHY
+	help
+	  Enable support for the Lantiq PEF2256 (FALC56) PHY.
+	  The PEF2256 is a framer and line interface between analog E1/T1/J1
+	  line and a digital PCM bus.
+	  This PHY support allows to consider the PEF2256 as a PHY.
+
+	  If unsure, say N.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called phy-lantiq-pef2256.
+
 config PHY_LANTIQ_VRX200_PCIE
 	tristate "Lantiq VRX200/ARX300 PCIe PHY"
 	depends on SOC_TYPE_XWAY || COMPILE_TEST
diff --git a/drivers/phy/lantiq/Makefile b/drivers/phy/lantiq/Makefile
index 7c14eb24ab73..6e501d865620 100644
--- a/drivers/phy/lantiq/Makefile
+++ b/drivers/phy/lantiq/Makefile
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0-only
+obj-$(CONFIG_PHY_LANTIQ_PEF2256)	+= phy-lantiq-pef2256.o
 obj-$(CONFIG_PHY_LANTIQ_RCU_USB2)	+= phy-lantiq-rcu-usb2.o
 obj-$(CONFIG_PHY_LANTIQ_VRX200_PCIE)	+= phy-lantiq-vrx200-pcie.o
diff --git a/drivers/phy/lantiq/phy-lantiq-pef2256.c b/drivers/phy/lantiq/phy-lantiq-pef2256.c
new file mode 100644
index 000000000000..1a1a4f66c102
--- /dev/null
+++ b/drivers/phy/lantiq/phy-lantiq-pef2256.c
@@ -0,0 +1,131 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PEF2256 phy support
+ *
+ * Copyright 2023 CS GROUP France
+ *
+ * Author: Herve Codina <herve.codina@bootlin.com>
+ */
+
+#include <linux/phy/phy.h>
+#include <linux/mfd/pef2256.h>
+#include <linux/module.h>
+#include <linux/notifier.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+struct pef2256_phy {
+	struct phy *phy;
+	struct pef2256 *pef2256;
+	struct device *dev;
+	struct atomic_notifier_head event_notifier_list;
+	struct notifier_block nb;
+};
+
+static int pef2256_carrier_notifier(struct notifier_block *nb, unsigned long action,
+				    void *data)
+{
+	struct pef2256_phy *pef2256 = container_of(nb, struct pef2256_phy, nb);
+
+	switch (action) {
+	case PEF2256_EVENT_CARRIER:
+		return atomic_notifier_call_chain(&pef2256->event_notifier_list,
+						  PHY_EVENT_STATUS,
+						  NULL);
+	default:
+		break;
+	}
+
+	return NOTIFY_DONE;
+}
+
+static int pef2256_phy_atomic_notifier_register(struct phy *phy, struct notifier_block *nb)
+{
+	struct pef2256_phy *pef2256 = phy_get_drvdata(phy);
+
+	return atomic_notifier_chain_register(&pef2256->event_notifier_list, nb);
+}
+
+static int pef2256_phy_atomic_notifier_unregister(struct phy *phy, struct notifier_block *nb)
+{
+	struct pef2256_phy *pef2256 = phy_get_drvdata(phy);
+
+	return atomic_notifier_chain_unregister(&pef2256->event_notifier_list, nb);
+}
+
+static int pef2256_phy_init(struct phy *phy)
+{
+	struct pef2256_phy *pef2256 = phy_get_drvdata(phy);
+
+	ATOMIC_INIT_NOTIFIER_HEAD(&pef2256->event_notifier_list);
+
+	pef2256->nb.notifier_call = pef2256_carrier_notifier;
+	return pef2256_register_event_notifier(pef2256->pef2256, &pef2256->nb);
+}
+
+static int pef2256_phy_exit(struct phy *phy)
+{
+	struct pef2256_phy *pef2256 = phy_get_drvdata(phy);
+
+	return pef2256_unregister_event_notifier(pef2256->pef2256, &pef2256->nb);
+}
+
+static int pef2256_phy_get_status(struct phy *phy, union phy_status *status)
+{
+	struct pef2256_phy *pef2256 = phy_get_drvdata(phy);
+
+	status->basic.link_is_on = pef2256_get_carrier(pef2256->pef2256);
+	return 0;
+}
+
+static const struct phy_ops pef2256_phy_ops = {
+	.owner = THIS_MODULE,
+	.init = pef2256_phy_init,
+	.exit = pef2256_phy_exit,
+	.get_status = pef2256_phy_get_status,
+	.atomic_notifier_register = pef2256_phy_atomic_notifier_register,
+	.atomic_notifier_unregister = pef2256_phy_atomic_notifier_unregister,
+};
+
+static int pef2256_phy_probe(struct platform_device *pdev)
+{
+	struct phy_provider *provider;
+	struct pef2256_phy *pef2256;
+
+	pef2256 = devm_kzalloc(&pdev->dev, sizeof(*pef2256), GFP_KERNEL);
+	if (!pef2256)
+		return -ENOMEM;
+
+	pef2256->dev = &pdev->dev;
+	pef2256->pef2256 = dev_get_drvdata(pef2256->dev->parent);
+
+	pef2256->phy = devm_phy_create(pef2256->dev, NULL, &pef2256_phy_ops);
+	if (IS_ERR(pef2256->phy))
+		return PTR_ERR(pef2256->phy);
+
+	phy_set_drvdata(pef2256->phy, pef2256);
+	pef2256->phy->attrs.mode = PHY_MODE_BASIC;
+
+	provider = devm_of_phy_provider_register(pef2256->dev, of_phy_simple_xlate);
+
+	return PTR_ERR_OR_ZERO(provider);
+}
+
+static const struct of_device_id pef2256_phy_of_match[] = {
+	{ .compatible = "lantiq,pef2256-phy" },
+	{} /* sentinel */
+};
+MODULE_DEVICE_TABLE(of, pef2256_phy_of_match);
+
+static struct platform_driver pef2256_phy_driver = {
+	.driver = {
+		.name = "lantiq-pef2256-phy",
+		.of_match_table = pef2256_phy_of_match,
+	},
+	.probe = pef2256_phy_probe,
+};
+module_platform_driver(pef2256_phy_driver);
+
+MODULE_AUTHOR("Herve Codina <herve.codina@bootlin.com>");
+MODULE_DESCRIPTION("PEF2256 PHY driver");
+MODULE_LICENSE("GPL");
-- 
2.39.2


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RFC PATCH 0/4] Add support for QMC HDLC and PHY

[Herve Codina]

Hi all,

I haven't received any feedback on this RFC.
Have you had a chance to review it ?

Best regards,
Hervé

On Thu, 23 Mar 2023 11:31:50 +0100
Herve Codina <herve.codina@bootlin.com> wrote:

> Hi,
> 
> I have a system where I need to handle an HDLC interface.
> 
> The HDLC data are transferred using a TDM bus on which a PEF2256 is
> present. The PEF2256 transfers data from/to the TDM bus to/from E1 line.
> This PEF2256 is also connected to a PowerQUICC SoC for the control path
> and the TDM is connected to the SoC (QMC component) for the data path.
> 
> From the HDLC driver, I need to handle data using the QMC and carrier
> detection using the PEF2256 (E1 line carrier).
> 
> The HDLC driver consider the PEF2256 as a generic PHY.
> So, the design is the following:
> 
> +----------+          +-------------+              +---------+
> | HDLC drv | <-data-> | QMC channel | <-- TDM -->  | PEF2256 |
> +----------+          +-------------+              |         | <--> E1
>    ^   +---------+     +---------+                 |         |
>    +-> | Gen PHY | <-> | PEF2256 | <- local bus -> |         |
>        +---------+     | PHY drv |                 +---------+
>                        +---------+
> 
> In order to implement this, I had to:
>  1 - Extend the generic PHY API to support get_status() and notification
>      on status change.
>  2 - Introduce a new kind of generic PHY named "basic phy". This PHY
>      familly can provide a link status in the get_status() data.
>  3 - Support the PEF2256 PHY as a "basic phy"
> 
> The purpose of this RFC series is to discuss this design.
> 
> The QMC driver code is available on linux-next. In this series:
> - patch 1: driver HDLC using the QMC channel
> - patch 2: Extend the generic PHY API
> - patch 3: Use the "basic phy" in the HDLC driver
> - patch 4: Implement the PEF2256 PHY driver
> 
> I did 2 patches for the HDLC driver in order to point the new PHY family
> usage in the HDLC driver. In the end, these two patches will be squashed
> and the bindings will be added.
> 
> Hope to have some feedback on this proposal.
> 
> Best regards,
> Hervé
> 
> Herve Codina (4):
>   net: wan: Add support for QMC HDLC
>   phy: Extend API to support 'status' get and notification
>   net: wan: fsl_qmc_hdlc: Add PHY support
>   phy: lantiq: Add PEF2256 PHY support
> 
>  drivers/net/wan/Kconfig                 |  12 +
>  drivers/net/wan/Makefile                |   1 +
>  drivers/net/wan/fsl_qmc_hdlc.c          | 558 ++++++++++++++++++++++++
>  drivers/phy/lantiq/Kconfig              |  15 +
>  drivers/phy/lantiq/Makefile             |   1 +
>  drivers/phy/lantiq/phy-lantiq-pef2256.c | 131 ++++++
>  drivers/phy/phy-core.c                  |  88 ++++
>  include/linux/phy/phy-basic.h           |  27 ++
>  include/linux/phy/phy.h                 |  89 +++-
>  9 files changed, 921 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/net/wan/fsl_qmc_hdlc.c
>  create mode 100644 drivers/phy/lantiq/phy-lantiq-pef2256.c
>  create mode 100644 include/linux/phy/phy-basic.h
> 


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RFC PATCH 2/4] phy: Extend API to support 'status' get and notification

[Vinod Koul]

On 23-03-23, 11:31, Herve Codina wrote:
> The PHY API provides functions to control and pass information from the
> PHY consumer to the PHY provider.
> There is no way for the consumer to get direct information from the PHY
> or be notified by the PHY.
> 
> To fill this hole, two API function are provided:
> 
> - phy_get_status()
>   This function can be used to get a "status" from the PHY. It is built
>   as the same ways as the configure() function. The status information
>   present in the status retrieved depends on the PHY's phy_mode.
>   This allows to get a "status" depending on the kind of PHY.

what does 'status' mean and communicate to used? How does having this
help?

> 
> - phy_atomic_notifier_(un)register()
>   These functions can be used to register/unregister an atomic notifier
>   block. The event available at this time is the PHY_EVENT_STATUS status
>   event which purpose is to signal some changes in the status available
>   using phy_get_status().
> 
> An new kind of PHY is added: PHY_MODE_BASIC.
> This new kind of PHY represents a basic PHY offering a basic status This
> status contains a link state indication.
> With the new API, a link state indication can be retrieve using
> phy_get_status() and link state changes can be notified.
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  drivers/phy/phy-core.c        | 88 ++++++++++++++++++++++++++++++++++
>  include/linux/phy/phy-basic.h | 27 +++++++++++
>  include/linux/phy/phy.h       | 89 ++++++++++++++++++++++++++++++++++-
>  3 files changed, 203 insertions(+), 1 deletion(-)
>  create mode 100644 include/linux/phy/phy-basic.h
> 
> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> index 9951efc03eaa..c7b568b99dce 100644
> --- a/drivers/phy/phy-core.c
> +++ b/drivers/phy/phy-core.c
> @@ -551,6 +551,94 @@ int phy_validate(struct phy *phy, enum phy_mode mode, int submode,
>  }
>  EXPORT_SYMBOL_GPL(phy_validate);
>  
> +/**
> + * phy_get_status() - Gets the phy status
> + * @phy: the phy returned by phy_get()
> + * @status: the status to retrieve
> + *
> + * Used to get the PHY status. phy_init() must have been called
> + * on the phy. The status will be retrieved from the current phy mode,
> + * that can be changed using phy_set_mode().
> + *
> + * Return: %0 if successful, a negative error code otherwise
> + */
> +int phy_get_status(struct phy *phy, union phy_status *status)
> +{
> +	int ret;
> +
> +	if (!phy)
> +		return -EINVAL;
> +
> +	if (!phy->ops->get_status)
> +		return -EOPNOTSUPP;
> +
> +	mutex_lock(&phy->mutex);
> +	ret = phy->ops->get_status(phy, status);
> +	mutex_unlock(&phy->mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(phy_get_status);
> +
> +/**
> + * phy_atomic_notifier_register() - Registers an atomic notifier
> + * @phy: the phy returned by phy_get()
> + * @nb: the notifier block to register
> + *
> + * Used to register a notifier block on PHY events. phy_init() must have
> + * been called on the phy.
> + * The notifier function given in the notifier_block must not sleep.
> + * The available PHY events are present in enum phy_events
> + *
> + * Return: %0 if successful, a negative error code otherwise
> + */
> +int phy_atomic_notifier_register(struct phy *phy, struct notifier_block *nb)
> +{
> +	int ret;
> +
> +	if (!phy)
> +		return -EINVAL;
> +
> +	if (!phy->ops->atomic_notifier_register ||
> +	    !phy->ops->atomic_notifier_unregister)
> +		return -EOPNOTSUPP;
> +
> +	mutex_lock(&phy->mutex);
> +	ret = phy->ops->atomic_notifier_register(phy, nb);
> +	mutex_unlock(&phy->mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(phy_atomic_notifier_register);
> +
> +/**
> + * phy_atomic_notifier_unregister() - Unregisters an atomic notifier
> + * @phy: the phy returned by phy_get()
> + * @nb: the notifier block to unregister
> + *
> + * Used to unregister a notifier block. phy_init() must have
> + * been called on the phy.
> + *
> + * Return: %0 if successful, a negative error code otherwise
> + */
> +int phy_atomic_notifier_unregister(struct phy *phy, struct notifier_block *nb)
> +{
> +	int ret;
> +
> +	if (!phy)
> +		return -EINVAL;
> +
> +	if (!phy->ops->atomic_notifier_unregister)
> +		return -EOPNOTSUPP;
> +
> +	mutex_lock(&phy->mutex);
> +	ret = phy->ops->atomic_notifier_unregister(phy, nb);
> +	mutex_unlock(&phy->mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(phy_atomic_notifier_unregister);
> +
>  /**
>   * _of_phy_get() - lookup and obtain a reference to a phy by phandle
>   * @np: device_node for which to get the phy
> diff --git a/include/linux/phy/phy-basic.h b/include/linux/phy/phy-basic.h
> new file mode 100644
> index 000000000000..95668c610c78
> --- /dev/null
> +++ b/include/linux/phy/phy-basic.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright 2023 CS GROUP France
> + *
> + * Author: Herve Codina <herve.codina@bootlin.com>
> + */
> +
> +#ifndef __PHY_BASIC_H_
> +#define __PHY_BASIC_H_
> +
> +#include <linux/types.h>
> +
> +/**
> + * struct phy_status_basic - Basic PHY status
> + *
> + * This structure is used to represent the status of a Basic phy.
> + */
> +struct phy_status_basic {
> +	/**
> +	 * @link_state:
> +	 *
> +	 * Link state. true, the link is on, false, the link is off.
> +	 */
> +	bool link_is_on;
> +};
> +
> +#endif /* __PHY_DP_H_ */
> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> index 3a570bc59fc7..40370d41012b 100644
> --- a/include/linux/phy/phy.h
> +++ b/include/linux/phy/phy.h
> @@ -16,6 +16,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/regulator/consumer.h>
>  
> +#include <linux/phy/phy-basic.h>
>  #include <linux/phy/phy-dp.h>
>  #include <linux/phy/phy-lvds.h>
>  #include <linux/phy/phy-mipi-dphy.h>
> @@ -42,7 +43,8 @@ enum phy_mode {
>  	PHY_MODE_MIPI_DPHY,
>  	PHY_MODE_SATA,
>  	PHY_MODE_LVDS,
> -	PHY_MODE_DP
> +	PHY_MODE_DP,
> +	PHY_MODE_BASIC,
>  };
>  
>  enum phy_media {
> @@ -67,6 +69,22 @@ union phy_configure_opts {
>  	struct phy_configure_opts_lvds		lvds;
>  };
>  
> +/**
> + * union phy_status - Opaque generic phy status
> + *
> + * @basic:	Status availbale phys supporting the Basic phy mode.
> + */
> +union phy_status {
> +	struct phy_status_basic		basic;
> +};
> +
> +/**
> + * phy_event - event available for notification
> + */
> +enum phy_event {
> +	PHY_EVENT_STATUS,	/* Event notified on phy_status changes */
> +};
> +
>  /**
>   * struct phy_ops - set of function pointers for performing phy operations
>   * @init: operation to be performed for initializing phy
> @@ -120,6 +138,45 @@ struct phy_ops {
>  	 */
>  	int	(*validate)(struct phy *phy, enum phy_mode mode, int submode,
>  			    union phy_configure_opts *opts);
> +
> +	/**
> +	 * @get_status:
> +	 *
> +	 * Optional.
> +	 *
> +	 * Used to get the PHY status. phy_init() must have
> +	 * been called on the phy.
> +	 *
> +	 * Returns: 0 if successful, an negative error code otherwise
> +	 */
> +	int	(*get_status)(struct phy *phy, union phy_status *status);
> +
> +	/**
> +	 * @atomic_notifier_register:
> +	 *
> +	 * Optional.
> +	 *
> +	 * Used to register a notifier block on PHY events. phy_init() must have
> +	 * been called on the phy.
> +	 * The notifier function given in the notifier_block must not sleep.
> +	 * The available PHY events are present in enum phy_events
> +	 *
> +	 * Returns: 0 if successful, an negative error code otherwise
> +	 */
> +	int	(*atomic_notifier_register)(struct phy *phy, struct notifier_block *nb);
> +
> +	/**
> +	 * @atomic_notifier_unregister:
> +	 *
> +	 * Mandatoty if @atomic_notifier_register is set.
> +	 *
> +	 * Used to unregister a notifier block on PHY events. phy_init() must have
> +	 * been called on the phy.
> +	 *
> +	 * Returns: 0 if successful, an negative error code otherwise
> +	 */
> +	int	(*atomic_notifier_unregister)(struct phy *phy, struct notifier_block *nb);
> +
>  	int	(*reset)(struct phy *phy);
>  	int	(*calibrate)(struct phy *phy);
>  	void	(*release)(struct phy *phy);
> @@ -234,6 +291,10 @@ int phy_set_speed(struct phy *phy, int speed);
>  int phy_configure(struct phy *phy, union phy_configure_opts *opts);
>  int phy_validate(struct phy *phy, enum phy_mode mode, int submode,
>  		 union phy_configure_opts *opts);
> +int phy_get_status(struct phy *phy, union phy_status *status);
> +int phy_atomic_notifier_register(struct phy *phy, struct notifier_block *nb);
> +int phy_atomic_notifier_unregister(struct phy *phy, struct notifier_block *nb);
> +
>  
>  static inline enum phy_mode phy_get_mode(struct phy *phy)
>  {
> @@ -412,6 +473,32 @@ static inline int phy_validate(struct phy *phy, enum phy_mode mode, int submode,
>  	return -ENOSYS;
>  }
>  
> +static inline int phy_get_status(struct phy *phy, union phy_status *status)
> +{
> +	if (!phy)
> +		return 0;
> +
> +	return -ENOSYS;
> +}
> +
> +static inline int phy_atomic_notifier_register(struct phy *phy,
> +					       struct notifier_block *nb)
> +{
> +	if (!phy)
> +		return 0;
> +
> +	return -ENOSYS;
> +}
> +
> +static inline int phy_atomic_notifier_unregister(struct phy *phy,
> +						 struct notifier_block *nb)
> +{
> +	if (!phy)
> +		return 0;
> +
> +	return -ENOSYS;
> +}
> +
>  static inline int phy_get_bus_width(struct phy *phy)
>  {
>  	return -ENOSYS;
> -- 
> 2.39.2
> 
> 
> -- 
> linux-phy mailing list
> linux-phy@lists.infradead.org
> https://lists.infradead.org/mailman/listinfo/linux-phy

-- 
~Vinod

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RFC PATCH 2/4] phy: Extend API to support 'status' get and notification

[Herve Codina]

Hi Vinod,

On Wed, 12 Apr 2023 22:18:44 +0530
Vinod Koul <vkoul@kernel.org> wrote:

> On 23-03-23, 11:31, Herve Codina wrote:
> > The PHY API provides functions to control and pass information from the
> > PHY consumer to the PHY provider.
> > There is no way for the consumer to get direct information from the PHY
> > or be notified by the PHY.
> > 
> > To fill this hole, two API function are provided:
> > 
> > - phy_get_status()
> >   This function can be used to get a "status" from the PHY. It is built
> >   as the same ways as the configure() function. The status information
> >   present in the status retrieved depends on the PHY's phy_mode.
> >   This allows to get a "status" depending on the kind of PHY.  
> 
> what does 'status' mean and communicate to used? How does having this
> help?

'status' can be some information that the PHY can provide to the consumer.
The existing API does not provide a generic way to get some information from
the PHY and 'status' with phy_get_status() provides this generic way.
Its exact meaning depends on the kind of PHY. For the PHY_MODE_BASIC,
introduced in this series, 'status' contains information related to the link
state.
And so, the consumer using a PHY_MODE_BASIC PHY can retreive the link state
getting the 'status' from the PHY.

The patch 3 in this RFC details a consumer usage. An HDLC driver uses a
PHY_MODE_BASIC PHY status to know the PHY link state and calls 
netif_carrier_{on,off}() accordingly.

Best regards,
Hervé

> 
> > 
> > - phy_atomic_notifier_(un)register()
> >   These functions can be used to register/unregister an atomic notifier
> >   block. The event available at this time is the PHY_EVENT_STATUS status
> >   event which purpose is to signal some changes in the status available
> >   using phy_get_status().
> > 
> > An new kind of PHY is added: PHY_MODE_BASIC.
> > This new kind of PHY represents a basic PHY offering a basic status This
> > status contains a link state indication.
> > With the new API, a link state indication can be retrieve using
> > phy_get_status() and link state changes can be notified.
> > 
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > ---
> >  drivers/phy/phy-core.c        | 88 ++++++++++++++++++++++++++++++++++
> >  include/linux/phy/phy-basic.h | 27 +++++++++++
> >  include/linux/phy/phy.h       | 89 ++++++++++++++++++++++++++++++++++-
> >  3 files changed, 203 insertions(+), 1 deletion(-)
> >  create mode 100644 include/linux/phy/phy-basic.h
> > 
> > diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> > index 9951efc03eaa..c7b568b99dce 100644
> > --- a/drivers/phy/phy-core.c
> > +++ b/drivers/phy/phy-core.c
> > @@ -551,6 +551,94 @@ int phy_validate(struct phy *phy, enum phy_mode mode, int submode,
> >  }
> >  EXPORT_SYMBOL_GPL(phy_validate);
> >  
> > +/**
> > + * phy_get_status() - Gets the phy status
> > + * @phy: the phy returned by phy_get()
> > + * @status: the status to retrieve
> > + *
> > + * Used to get the PHY status. phy_init() must have been called
> > + * on the phy. The status will be retrieved from the current phy mode,
> > + * that can be changed using phy_set_mode().
> > + *
> > + * Return: %0 if successful, a negative error code otherwise
> > + */
> > +int phy_get_status(struct phy *phy, union phy_status *status)
> > +{
> > +	int ret;
> > +
> > +	if (!phy)
> > +		return -EINVAL;
> > +
> > +	if (!phy->ops->get_status)
> > +		return -EOPNOTSUPP;
> > +
> > +	mutex_lock(&phy->mutex);
> > +	ret = phy->ops->get_status(phy, status);
> > +	mutex_unlock(&phy->mutex);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(phy_get_status);
> > +
> > +/**
> > + * phy_atomic_notifier_register() - Registers an atomic notifier
> > + * @phy: the phy returned by phy_get()
> > + * @nb: the notifier block to register
> > + *
> > + * Used to register a notifier block on PHY events. phy_init() must have
> > + * been called on the phy.
> > + * The notifier function given in the notifier_block must not sleep.
> > + * The available PHY events are present in enum phy_events
> > + *
> > + * Return: %0 if successful, a negative error code otherwise
> > + */
> > +int phy_atomic_notifier_register(struct phy *phy, struct notifier_block *nb)
> > +{
> > +	int ret;
> > +
> > +	if (!phy)
> > +		return -EINVAL;
> > +
> > +	if (!phy->ops->atomic_notifier_register ||
> > +	    !phy->ops->atomic_notifier_unregister)
> > +		return -EOPNOTSUPP;
> > +
> > +	mutex_lock(&phy->mutex);
> > +	ret = phy->ops->atomic_notifier_register(phy, nb);
> > +	mutex_unlock(&phy->mutex);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(phy_atomic_notifier_register);
> > +
> > +/**
> > + * phy_atomic_notifier_unregister() - Unregisters an atomic notifier
> > + * @phy: the phy returned by phy_get()
> > + * @nb: the notifier block to unregister
> > + *
> > + * Used to unregister a notifier block. phy_init() must have
> > + * been called on the phy.
> > + *
> > + * Return: %0 if successful, a negative error code otherwise
> > + */
> > +int phy_atomic_notifier_unregister(struct phy *phy, struct notifier_block *nb)
> > +{
> > +	int ret;
> > +
> > +	if (!phy)
> > +		return -EINVAL;
> > +
> > +	if (!phy->ops->atomic_notifier_unregister)
> > +		return -EOPNOTSUPP;
> > +
> > +	mutex_lock(&phy->mutex);
> > +	ret = phy->ops->atomic_notifier_unregister(phy, nb);
> > +	mutex_unlock(&phy->mutex);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(phy_atomic_notifier_unregister);
> > +
> >  /**
> >   * _of_phy_get() - lookup and obtain a reference to a phy by phandle
> >   * @np: device_node for which to get the phy
> > diff --git a/include/linux/phy/phy-basic.h b/include/linux/phy/phy-basic.h
> > new file mode 100644
> > index 000000000000..95668c610c78
> > --- /dev/null
> > +++ b/include/linux/phy/phy-basic.h
> > @@ -0,0 +1,27 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright 2023 CS GROUP France
> > + *
> > + * Author: Herve Codina <herve.codina@bootlin.com>
> > + */
> > +
> > +#ifndef __PHY_BASIC_H_
> > +#define __PHY_BASIC_H_
> > +
> > +#include <linux/types.h>
> > +
> > +/**
> > + * struct phy_status_basic - Basic PHY status
> > + *
> > + * This structure is used to represent the status of a Basic phy.
> > + */
> > +struct phy_status_basic {
> > +	/**
> > +	 * @link_state:
> > +	 *
> > +	 * Link state. true, the link is on, false, the link is off.
> > +	 */
> > +	bool link_is_on;
> > +};
> > +
> > +#endif /* __PHY_DP_H_ */
> > diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> > index 3a570bc59fc7..40370d41012b 100644
> > --- a/include/linux/phy/phy.h
> > +++ b/include/linux/phy/phy.h
> > @@ -16,6 +16,7 @@
> >  #include <linux/pm_runtime.h>
> >  #include <linux/regulator/consumer.h>
> >  
> > +#include <linux/phy/phy-basic.h>
> >  #include <linux/phy/phy-dp.h>
> >  #include <linux/phy/phy-lvds.h>
> >  #include <linux/phy/phy-mipi-dphy.h>
> > @@ -42,7 +43,8 @@ enum phy_mode {
> >  	PHY_MODE_MIPI_DPHY,
> >  	PHY_MODE_SATA,
> >  	PHY_MODE_LVDS,
> > -	PHY_MODE_DP
> > +	PHY_MODE_DP,
> > +	PHY_MODE_BASIC,
> >  };
> >  
> >  enum phy_media {
> > @@ -67,6 +69,22 @@ union phy_configure_opts {
> >  	struct phy_configure_opts_lvds		lvds;
> >  };
> >  
> > +/**
> > + * union phy_status - Opaque generic phy status
> > + *
> > + * @basic:	Status availbale phys supporting the Basic phy mode.
> > + */
> > +union phy_status {
> > +	struct phy_status_basic		basic;
> > +};
> > +
> > +/**
> > + * phy_event - event available for notification
> > + */
> > +enum phy_event {
> > +	PHY_EVENT_STATUS,	/* Event notified on phy_status changes */
> > +};
> > +
> >  /**
> >   * struct phy_ops - set of function pointers for performing phy operations
> >   * @init: operation to be performed for initializing phy
> > @@ -120,6 +138,45 @@ struct phy_ops {
> >  	 */
> >  	int	(*validate)(struct phy *phy, enum phy_mode mode, int submode,
> >  			    union phy_configure_opts *opts);
> > +
> > +	/**
> > +	 * @get_status:
> > +	 *
> > +	 * Optional.
> > +	 *
> > +	 * Used to get the PHY status. phy_init() must have
> > +	 * been called on the phy.
> > +	 *
> > +	 * Returns: 0 if successful, an negative error code otherwise
> > +	 */
> > +	int	(*get_status)(struct phy *phy, union phy_status *status);
> > +
> > +	/**
> > +	 * @atomic_notifier_register:
> > +	 *
> > +	 * Optional.
> > +	 *
> > +	 * Used to register a notifier block on PHY events. phy_init() must have
> > +	 * been called on the phy.
> > +	 * The notifier function given in the notifier_block must not sleep.
> > +	 * The available PHY events are present in enum phy_events
> > +	 *
> > +	 * Returns: 0 if successful, an negative error code otherwise
> > +	 */
> > +	int	(*atomic_notifier_register)(struct phy *phy, struct notifier_block *nb);
> > +
> > +	/**
> > +	 * @atomic_notifier_unregister:
> > +	 *
> > +	 * Mandatoty if @atomic_notifier_register is set.
> > +	 *
> > +	 * Used to unregister a notifier block on PHY events. phy_init() must have
> > +	 * been called on the phy.
> > +	 *
> > +	 * Returns: 0 if successful, an negative error code otherwise
> > +	 */
> > +	int	(*atomic_notifier_unregister)(struct phy *phy, struct notifier_block *nb);
> > +
> >  	int	(*reset)(struct phy *phy);
> >  	int	(*calibrate)(struct phy *phy);
> >  	void	(*release)(struct phy *phy);
> > @@ -234,6 +291,10 @@ int phy_set_speed(struct phy *phy, int speed);
> >  int phy_configure(struct phy *phy, union phy_configure_opts *opts);
> >  int phy_validate(struct phy *phy, enum phy_mode mode, int submode,
> >  		 union phy_configure_opts *opts);
> > +int phy_get_status(struct phy *phy, union phy_status *status);
> > +int phy_atomic_notifier_register(struct phy *phy, struct notifier_block *nb);
> > +int phy_atomic_notifier_unregister(struct phy *phy, struct notifier_block *nb);
> > +
> >  
> >  static inline enum phy_mode phy_get_mode(struct phy *phy)
> >  {
> > @@ -412,6 +473,32 @@ static inline int phy_validate(struct phy *phy, enum phy_mode mode, int submode,
> >  	return -ENOSYS;
> >  }
> >  
> > +static inline int phy_get_status(struct phy *phy, union phy_status *status)
> > +{
> > +	if (!phy)
> > +		return 0;
> > +
> > +	return -ENOSYS;
> > +}
> > +
> > +static inline int phy_atomic_notifier_register(struct phy *phy,
> > +					       struct notifier_block *nb)
> > +{
> > +	if (!phy)
> > +		return 0;
> > +
> > +	return -ENOSYS;
> > +}
> > +
> > +static inline int phy_atomic_notifier_unregister(struct phy *phy,
> > +						 struct notifier_block *nb)
> > +{
> > +	if (!phy)
> > +		return 0;
> > +
> > +	return -ENOSYS;
> > +}
> > +
> >  static inline int phy_get_bus_width(struct phy *phy)
> >  {
> >  	return -ENOSYS;
> > -- 
> > 2.39.2
> > 
> > 
> > -- 
> > linux-phy mailing list
> > linux-phy@lists.infradead.org
> > https://lists.infradead.org/mailman/listinfo/linux-phy  
> 

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RFC PATCH 0/4] Add support for QMC HDLC and PHY

[Andrew Lunn]

On Thu, Mar 23, 2023 at 11:31:50AM +0100, Herve Codina wrote:
> Hi,
> 
> I have a system where I need to handle an HDLC interface.
> 
> The HDLC data are transferred using a TDM bus on which a PEF2256 is
> present. The PEF2256 transfers data from/to the TDM bus to/from E1 line.
> This PEF2256 is also connected to a PowerQUICC SoC for the control path
> and the TDM is connected to the SoC (QMC component) for the data path.
> 
> From the HDLC driver, I need to handle data using the QMC and carrier
> detection using the PEF2256 (E1 line carrier).
> 
> The HDLC driver consider the PEF2256 as a generic PHY.
> So, the design is the following:
> 
> +----------+          +-------------+              +---------+
> | HDLC drv | <-data-> | QMC channel | <-- TDM -->  | PEF2256 |
> +----------+          +-------------+              |         | <--> E1
>    ^   +---------+     +---------+                 |         |
>    +-> | Gen PHY | <-> | PEF2256 | <- local bus -> |         |
>        +---------+     | PHY drv |                 +---------+
>                        +---------+

Hi Herver

Sorry, i'm late to the conversation. I'm looking at this from two
different perspectives. I help maintain Ethernet PHYs. And i have
hacked on the IDT 82P2288 E1/T1/J1 framer.

I think there is a block missing from this diagram. There appears to
be an MFD driver for the PEF2256? At least, i see an include for
linux/mfd/pef2256.h.

When i look at the 'phy' driver, i don't see anything a typical PHY
driver used for networking would have. A networking PHY driver often
has the ability to change between modes, like SGMII, QSGMII, 10GBASER.
The equivalent here would be changing between E1, T1 and J1. It has
the ability to change the speed, 1G, 2.5G, 10G etc. This could be
implied via the mode, E1 is 2.048Mbps, T1 1.544Mbps, and i forget what
J1 is. The PEF2256 also seems to support E1/T1/J1. How is its modes
configured?

In fact, this PHY driver does not seem to do any configuration of any
sort on the framer. All it seems to be doing is take notification from
one chain and send them out another chain!

I also wounder if this get_status() call is sufficient. Don't you also
want Red, Yellow and Blue alarms? It is not just the carrier is down,
but why it is down.

Overall, i don't see why you want a PHY. What value does it add?

	 Andrew

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RFC PATCH 0/4] Add support for QMC HDLC and PHY

[Herve Codina]

Hi Andrew,

On Thu, 13 Apr 2023 14:48:30 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Thu, Mar 23, 2023 at 11:31:50AM +0100, Herve Codina wrote:
> > Hi,
> > 
> > I have a system where I need to handle an HDLC interface.
> > 
> > The HDLC data are transferred using a TDM bus on which a PEF2256 is
> > present. The PEF2256 transfers data from/to the TDM bus to/from E1 line.
> > This PEF2256 is also connected to a PowerQUICC SoC for the control path
> > and the TDM is connected to the SoC (QMC component) for the data path.
> > 
> > From the HDLC driver, I need to handle data using the QMC and carrier
> > detection using the PEF2256 (E1 line carrier).
> > 
> > The HDLC driver consider the PEF2256 as a generic PHY.
> > So, the design is the following:
> > 
> > +----------+          +-------------+              +---------+
> > | HDLC drv | <-data-> | QMC channel | <-- TDM -->  | PEF2256 |
> > +----------+          +-------------+              |         | <--> E1
> >    ^   +---------+     +---------+                 |         |
> >    +-> | Gen PHY | <-> | PEF2256 | <- local bus -> |         |
> >        +---------+     | PHY drv |                 +---------+
> >                        +---------+  
> 
> Hi Herver
> 
> Sorry, i'm late to the conversation. I'm looking at this from two
> different perspectives. I help maintain Ethernet PHYs. And i have
> hacked on the IDT 82P2288 E1/T1/J1 framer.
> 
> I think there is a block missing from this diagram. There appears to
> be an MFD driver for the PEF2256? At least, i see an include for
> linux/mfd/pef2256.h.

Indeed, there is the MFD driver and this MFD driver does the PEF2256
setup (line configuration, speed, ...).

> 
> When i look at the 'phy' driver, i don't see anything a typical PHY
> driver used for networking would have. A networking PHY driver often
> has the ability to change between modes, like SGMII, QSGMII, 10GBASER.
> The equivalent here would be changing between E1, T1 and J1. It has
> the ability to change the speed, 1G, 2.5G, 10G etc. This could be
> implied via the mode, E1 is 2.048Mbps, T1 1.544Mbps, and i forget what
> J1 is. The PEF2256 also seems to support E1/T1/J1. How is its modes
> configured?

All of these are set by the MFD driver during its probe().
The expected setting come from several properties present in the pef2256
DT node. The binding can be found here:
  https://lore.kernel.org/all/20230328092645.634375-2-herve.codina@bootlin.com/

Further more, the QMC HDLC is not the only PEF2256 consumer.
The PEF2256 is also used for audio path (ie audio over E1) and so the
configuration is shared between network and audio. The setting cannot be
handle by the network part as the PEF2256 must be available and correctly
configured even if the network part is not present.

> 
> In fact, this PHY driver does not seem to do any configuration of any
> sort on the framer. All it seems to be doing is take notification from
> one chain and send them out another chain!

Configuration is done by the parent MFD driver.
The PHY driver has nothing more to do.

> 
> I also wounder if this get_status() call is sufficient. Don't you also
> want Red, Yellow and Blue alarms? It is not just the carrier is down,
> but why it is down.

I don't need them in my use case but if needed can't they be added later?
Also, from the HDLC device point of view what can be done with these alarms?

> 
> Overall, i don't see why you want a PHY. What value does it add?

I need to detect carrier on/off according to the E1 link state.
The HDLC driver is a driver for a QMC device.
The QMC device present in some PowerPC SOC offers the possibility to send
data over a TDM bus.
From the QMC HDLC driver I don't want to refer the PEF2256 as the driver has
nothing to do with the PEF2256 directly.
The QMC HDLC driver send data to a TDM bus using the QMC device.

The PEF2256 is an interface in the data path between the QMC output (TDM bus) and
the E1 line.

We send HDLC over E1 because there is this kind of interface but we would
have sent HDLC over anything else if this interface was different.

Using a PHY to represent this interface was coherent for me.
Using the generic PHY subsystem allows to abstract the specific provider (PEF2256)
from the consumer (QMC HDLC).

Best regards,
Hervé

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RFC PATCH 0/4] Add support for QMC HDLC and PHY

[Andrew Lunn]

> > When i look at the 'phy' driver, i don't see anything a typical PHY
> > driver used for networking would have. A networking PHY driver often
> > has the ability to change between modes, like SGMII, QSGMII, 10GBASER.
> > The equivalent here would be changing between E1, T1 and J1. It has
> > the ability to change the speed, 1G, 2.5G, 10G etc. This could be
> > implied via the mode, E1 is 2.048Mbps, T1 1.544Mbps, and i forget what
> > J1 is. The PEF2256 also seems to support E1/T1/J1. How is its modes
> > configured?
> 
> All of these are set by the MFD driver during its probe().
> The expected setting come from several properties present in the pef2256
> DT node. The binding can be found here:
>   https://lore.kernel.org/all/20230328092645.634375-2-herve.codina@bootlin.com/

I'm surprised to see so much in the binding. I assume you are familiar
with DAHDI. It allows nearly everything to be configured at
runtime. The systems i've used allow you to select the clock
configuration, line build out, user side vs networks side signalling
CRC4 enables or not, etc.

> Further more, the QMC HDLC is not the only PEF2256 consumer.
> The PEF2256 is also used for audio path (ie audio over E1) and so the
> configuration is shared between network and audio. The setting cannot be
> handle by the network part as the PEF2256 must be available and correctly
> configured even if the network part is not present.

But there is no reason why the MFD could not provide a generic PHY to
actually configure the 'PHY'. The HDLC driver can then also use the
generic PHY. It would make your generic PHY less 'pointless'. I'm not
saying it has to be this way, but it is an option.
 
> > In fact, this PHY driver does not seem to do any configuration of any
> > sort on the framer. All it seems to be doing is take notification from
> > one chain and send them out another chain!
> 
> Configuration is done by the parent MFD driver.
> The PHY driver has nothing more to do.
> 
> > 
> > I also wounder if this get_status() call is sufficient. Don't you also
> > want Red, Yellow and Blue alarms? It is not just the carrier is down,
> > but why it is down.
> 
> I don't need them in my use case but if needed can't they be added later?
> Also, from the HDLC device point of view what can be done with these alarms?

https://elixir.bootlin.com/linux/latest/source/Documentation/networking/ethtool-netlink.rst#L472

> Requests link state information. Link up/down flag (as provided by
> ``ETHTOOL_GLINK`` ioctl command) is provided. Optionally, extended
> state might be provided as well. In general, extended state
> describes reasons for why a port is down, or why it operates in some
> non-obvious mode.

The colour of the Alarm gives you an idea which end of the system has
the problem.

> > Overall, i don't see why you want a PHY. What value does it add?
> 
> I need to detect carrier on/off according to the E1 link state.

Why not just use the MFD notifier? What is the value of a PHY driver
translating one notifier into another?

And why is the notifier specific to the PEF2256? What would happen if
i used a analog devices DS2155, DS21Q55, and DS2156, or the IDT
82P2281? Would each have its own notifier? And hence each would need
its own PHY which translates one notifier into another?

There are enough E1/T1/J1 framers we should have a generic API between
the framer and the HDLC device.

    Andrew

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RFC PATCH 0/4] Add support for QMC HDLC and PHY

[Herve Codina]

Hi Andrew

On Fri, 14 Apr 2023 18:15:30 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > > When i look at the 'phy' driver, i don't see anything a typical PHY
> > > driver used for networking would have. A networking PHY driver often
> > > has the ability to change between modes, like SGMII, QSGMII, 10GBASER.
> > > The equivalent here would be changing between E1, T1 and J1. It has
> > > the ability to change the speed, 1G, 2.5G, 10G etc. This could be
> > > implied via the mode, E1 is 2.048Mbps, T1 1.544Mbps, and i forget what
> > > J1 is. The PEF2256 also seems to support E1/T1/J1. How is its modes
> > > configured?  
> > 
> > All of these are set by the MFD driver during its probe().
> > The expected setting come from several properties present in the pef2256
> > DT node. The binding can be found here:
> >   https://lore.kernel.org/all/20230328092645.634375-2-herve.codina@bootlin.com/  
> 
> I'm surprised to see so much in the binding. I assume you are familiar
> with DAHDI. It allows nearly everything to be configured at
> runtime. The systems i've used allow you to select the clock
> configuration, line build out, user side vs networks side signalling
> CRC4 enables or not, etc.

Well, I am not familiar with DAHDI at all.
I didn't even know about the DAHDI project.
The project seems to use specific kernel driver and I would like to avoid
these external drivers.

> 
> > Further more, the QMC HDLC is not the only PEF2256 consumer.
> > The PEF2256 is also used for audio path (ie audio over E1) and so the
> > configuration is shared between network and audio. The setting cannot be
> > handle by the network part as the PEF2256 must be available and correctly
> > configured even if the network part is not present.  
> 
> But there is no reason why the MFD could not provide a generic PHY to
> actually configure the 'PHY'. The HDLC driver can then also use the
> generic PHY. It would make your generic PHY less 'pointless'. I'm not
> saying it has to be this way, but it is an option.

If the pef2256 PHY provides a configure function, who is going to call this
configure(). I mean the one calling the configure will be the configuration
owner. None of the MFD child can own the configuration as this configuration
will impact other children. So the MFD (top level node) owns the configuration.

>  
> > > In fact, this PHY driver does not seem to do any configuration of any
> > > sort on the framer. All it seems to be doing is take notification from
> > > one chain and send them out another chain!  
> > 
> > Configuration is done by the parent MFD driver.
> > The PHY driver has nothing more to do.
> >   
> > > 
> > > I also wounder if this get_status() call is sufficient. Don't you also
> > > want Red, Yellow and Blue alarms? It is not just the carrier is down,
> > > but why it is down.  
> > 
> > I don't need them in my use case but if needed can't they be added later?
> > Also, from the HDLC device point of view what can be done with these alarms?  
> 
> https://elixir.bootlin.com/linux/latest/source/Documentation/networking/ethtool-netlink.rst#L472

Thanks for pointing this interface.
It is specific to ethtool but I can see the idea.
The 'get_status' I proposed could be extended later to provide more
information related to the colour of the alarm if needed.
ethtool and related interfaces are very well fitted with Ethernet and
related PHYs. I am not sure that ethtool will be usable for the pef2256.

> 
> > Requests link state information. Link up/down flag (as provided by
> > ``ETHTOOL_GLINK`` ioctl command) is provided. Optionally, extended
> > state might be provided as well. In general, extended state
> > describes reasons for why a port is down, or why it operates in some
> > non-obvious mode.  
> 
> The colour of the Alarm gives you an idea which end of the system has
> the problem.
> 
> > > Overall, i don't see why you want a PHY. What value does it add?  
> > 
> > I need to detect carrier on/off according to the E1 link state.  
> 
> Why not just use the MFD notifier? What is the value of a PHY driver
> translating one notifier into another?

I translated to another notifier to keep the core (MFD) pef2256 API
independent.

But indeed this could be changed.
If changed, the MFD pef2256 will have to handle the full struct
phy_status_basic as the translation will not be there anymore.
Right now, this structure is pretty simple and contains only the link state
flag. But in the future, this PHY structure can move to something more
complex and I am not sure that filling this struct is the MFD pef2256
responsibility. The PHY pef2256 is responsible for the correct structure
contents not sure that this should be moved to the MFD part.

> 
> And why is the notifier specific to the PEF2256? What would happen if
> i used a analog devices DS2155, DS21Q55, and DS2156, or the IDT
> 82P2281? Would each have its own notifier? And hence each would need
> its own PHY which translates one notifier into another?

Each of them should have their own notifier if they can notify.
At least they will need their own notifier at they PHY driver level.
Having or not a translation from something else would depend on each device
PHY driver implementation.
Also, maybe some PHY will not be able to provide notifications but only
the get_status(). In this case, the notifier is not needed.
The proposed QMC HDLC driver handles this case switching to polling mode
if the PHY is not able to provide notification.

> 
> There are enough E1/T1/J1 framers we should have a generic API between
> the framer and the HDLC device.
> 

I agree.
I would like this first implementation without too much API restriction
in order to see how it goes.
The actual proposal imposes nothing on the PHY internal implementation.
the pef2256 implementation chooses to have two notifiers (one at MFD
level and one at PHY level) but it was not imposed by the API.

Best regards,
Hervé


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RFC PATCH 0/4] Add support for QMC HDLC and PHY

[Andrew Lunn]

> > I'm surprised to see so much in the binding. I assume you are familiar
> > with DAHDI. It allows nearly everything to be configured at
> > runtime. The systems i've used allow you to select the clock
> > configuration, line build out, user side vs networks side signalling
> > CRC4 enables or not, etc.
> 
> Well, I am not familiar with DAHDI at all.
> I didn't even know about the DAHDI project.
> The project seems to use specific kernel driver and I would like to avoid
> these external drivers.

DAHDI is kind of the reference. Pretty much any Linux system being
used as a open source PBX runs Asterisk, and has the out of tree DAHDI
code to provide access to E1/T1 hardware and analogue POTS. I doubt it
will ever be merged into mainline, but i think it gives a good idea
what is needed to fully make use of such hardware.

I don't know what you application is. Are you using libpri for
signalling? How are you exposing the B channel to user space so that
libpri can use it?

> > > Further more, the QMC HDLC is not the only PEF2256 consumer.
> > > The PEF2256 is also used for audio path (ie audio over E1) and so the
> > > configuration is shared between network and audio. The setting cannot be
> > > handle by the network part as the PEF2256 must be available and correctly
> > > configured even if the network part is not present.  
> > 
> > But there is no reason why the MFD could not provide a generic PHY to
> > actually configure the 'PHY'. The HDLC driver can then also use the
> > generic PHY. It would make your generic PHY less 'pointless'. I'm not
> > saying it has to be this way, but it is an option.
> 
> If the pef2256 PHY provides a configure function, who is going to call this
> configure(). I mean the one calling the configure will be the configuration
> owner. None of the MFD child can own the configuration as this configuration
> will impact other children. So the MFD (top level node) owns the configuration.

Fine. Nothing unusual there. The netdev owns the configuration for an
Ethernet device. The MAC driver passes a subset down to any generic
PHY being used to implement a SERDES.

You could have the same architecture here. The MFD implements a
standardised netlink API for configuring E1/T1/J1 devices. Part of it
gets passed to the framer, which could be part of a generic PHY. I
assume you also need to configure the HDLC hardware. It needs to know
if it is using the whole E1 channel in unframed mode, or it should do
a fractional link, using a subset of slots, or is just using one slot
for 64Kbps, which would be an ISDN B channel.

> > > > In fact, this PHY driver does not seem to do any configuration of any
> > > > sort on the framer. All it seems to be doing is take notification from
> > > > one chain and send them out another chain!  
> > > 
> > > Configuration is done by the parent MFD driver.
> > > The PHY driver has nothing more to do.
> > >   
> > > > 
> > > > I also wounder if this get_status() call is sufficient. Don't you also
> > > > want Red, Yellow and Blue alarms? It is not just the carrier is down,
> > > > but why it is down.  
> > > 
> > > I don't need them in my use case but if needed can't they be added later?
> > > Also, from the HDLC device point of view what can be done with these alarms?  
> > 
> > https://elixir.bootlin.com/linux/latest/source/Documentation/networking/ethtool-netlink.rst#L472
> 
> Thanks for pointing this interface.
> It is specific to ethtool but I can see the idea.

Don't equate ethtool with Ethernet. Any netdev can implement it, and a
HDLC device is a netdev. So it could well return link down reason.

> But indeed this could be changed.
> If changed, the MFD pef2256 will have to handle the full struct
> phy_status_basic as the translation will not be there anymore.
> Right now, this structure is pretty simple and contains only the link state
> flag. But in the future, this PHY structure can move to something more
> complex and I am not sure that filling this struct is the MFD pef2256
> responsibility. The PHY pef2256 is responsible for the correct structure
> contents not sure that this should be moved to the MFD part.

Framers, like Ethernet PHYs, are reasonably well defined, because
there are clear standards to follow. You could put the datasheets for
the various frames side by side and quickly get an idea of the common
properties. So you could define a structure now. In order to make it
extendable, just avoid 0 having any meaning other than UNKNOWN. If you
look at ethernet, SPEED_UNKNOWN is 0, DUPLEX_UNKNOWN is 0. So if a new
field is added, we know if a driver has not filled it in.

> > And why is the notifier specific to the PEF2256? What would happen if
> > i used a analog devices DS2155, DS21Q55, and DS2156, or the IDT
> > 82P2281? Would each have its own notifier? And hence each would need
> > its own PHY which translates one notifier into another?
> 
> Each of them should have their own notifier if they can notify.

I doubt you will find a framer which cannot report lost of framing. It
is too much a part of the standards. There are signalling actions you
need to do when a link goes does. So all framer drivers will have a
notifier.

> At least they will need their own notifier at they PHY driver level.
> Having or not a translation from something else would depend on each device
> PHY driver implementation.

Have you ever look at Ethernet PHYs? They all export the same API. You
can plug any Linux MAC driver into any Linux Ethernet PHY driver. It
should be the same here. You should be able to plug any HDLC driver
into any Framer. I should be able to take the same SoC you have
implementing the TDM interface, and plug it into the IDT framer i know
of. We want a standardised API between the HDLC device and the framer.

> I would like this first implementation without too much API restriction
> in order to see how it goes.
> The actual proposal imposes nothing on the PHY internal implementation.
> the pef2256 implementation chooses to have two notifiers (one at MFD
> level and one at PHY level) but it was not imposed by the API.

What i would like to see is some indication the API you are proposing
is generic, and could be implemented by multiple frames and HDLC
devices. The interface between the HDLC driver and the framer should
be generic. The HDLC driver has an abstract reference to a framer. The
framer has a reference to a netdev for the HDLC device.

You can keep this API very simple, just have link up/down
notification, since that is all you want at the moment. But the
implementation should give hints how it can be extended.

	Andrew

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RFC PATCH 0/4] Add support for QMC HDLC and PHY

[Herve Codina]

On Mon, 17 Apr 2023 15:12:14 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > > I'm surprised to see so much in the binding. I assume you are familiar
> > > with DAHDI. It allows nearly everything to be configured at
> > > runtime. The systems i've used allow you to select the clock
> > > configuration, line build out, user side vs networks side signalling
> > > CRC4 enables or not, etc.  
> > 
> > Well, I am not familiar with DAHDI at all.
> > I didn't even know about the DAHDI project.
> > The project seems to use specific kernel driver and I would like to avoid
> > these external drivers.  
> 
> DAHDI is kind of the reference. Pretty much any Linux system being
> used as a open source PBX runs Asterisk, and has the out of tree DAHDI
> code to provide access to E1/T1 hardware and analogue POTS. I doubt it
> will ever be merged into mainline, but i think it gives a good idea
> what is needed to fully make use of such hardware.
> 
> I don't know what you application is. Are you using libpri for
> signalling? How are you exposing the B channel to user space so that
> libpri can use it?

My application (probably a subset of what we can do with E1 lines) does
not use signaling.

I don't expose any channel to the user space but just:
- One hdlc interface using one timeslot.
- One or more dai links (audio channels) that can be used using ALSA library.

> 
> > > > Further more, the QMC HDLC is not the only PEF2256 consumer.
> > > > The PEF2256 is also used for audio path (ie audio over E1) and so the
> > > > configuration is shared between network and audio. The setting cannot be
> > > > handle by the network part as the PEF2256 must be available and correctly
> > > > configured even if the network part is not present.    
> > > 
> > > But there is no reason why the MFD could not provide a generic PHY to
> > > actually configure the 'PHY'. The HDLC driver can then also use the
> > > generic PHY. It would make your generic PHY less 'pointless'. I'm not
> > > saying it has to be this way, but it is an option.  
> > 
> > If the pef2256 PHY provides a configure function, who is going to call this
> > configure(). I mean the one calling the configure will be the configuration
> > owner. None of the MFD child can own the configuration as this configuration
> > will impact other children. So the MFD (top level node) owns the configuration.  
> 
> Fine. Nothing unusual there. The netdev owns the configuration for an
> Ethernet device. The MAC driver passes a subset down to any generic
> PHY being used to implement a SERDES.
> 
> You could have the same architecture here. The MFD implements a
> standardised netlink API for configuring E1/T1/J1 devices. Part of it
> gets passed to the framer, which could be part of a generic PHY. I
> assume you also need to configure the HDLC hardware. It needs to know
> if it is using the whole E1 channel in unframed mode, or it should do
> a fractional link, using a subset of slots, or is just using one slot
> for 64Kbps, which would be an ISDN B channel.

The HDLC driver uses a QMC channel and the DT binding related to this
QMC channel defines the timeslots used by this channel.

From the QMC HDLC nothing is configured. This is done at the QMC level.
  https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/soc/fsl/cpm_qe/fsl,cpm1-scc-qmc.yaml#n91

> 
> > > > > In fact, this PHY driver does not seem to do any configuration of any
> > > > > sort on the framer. All it seems to be doing is take notification from
> > > > > one chain and send them out another chain!    
> > > > 
> > > > Configuration is done by the parent MFD driver.
> > > > The PHY driver has nothing more to do.
> > > >     
> > > > > 
> > > > > I also wounder if this get_status() call is sufficient. Don't you also
> > > > > want Red, Yellow and Blue alarms? It is not just the carrier is down,
> > > > > but why it is down.    
> > > > 
> > > > I don't need them in my use case but if needed can't they be added later?
> > > > Also, from the HDLC device point of view what can be done with these alarms?    
> > > 
> > > https://elixir.bootlin.com/linux/latest/source/Documentation/networking/ethtool-netlink.rst#L472  
> > 
> > Thanks for pointing this interface.
> > It is specific to ethtool but I can see the idea.  
> 
> Don't equate ethtool with Ethernet. Any netdev can implement it, and a
> HDLC device is a netdev. So it could well return link down reason.

Ok.
So the link down reason should be returned by the newly introduced QMC HDLC.

Adding this in the generic PHY infrastructure I used (drivers/phy) is adding
some specific netdev stuff in this subsystem.
Don't forget that the pef2256 can be used without any network part by the
audio subsystem in order to have the user audio data sent to E1 using pure
ALSA path.


> 
> > But indeed this could be changed.
> > If changed, the MFD pef2256 will have to handle the full struct
> > phy_status_basic as the translation will not be there anymore.
> > Right now, this structure is pretty simple and contains only the link state
> > flag. But in the future, this PHY structure can move to something more
> > complex and I am not sure that filling this struct is the MFD pef2256
> > responsibility. The PHY pef2256 is responsible for the correct structure
> > contents not sure that this should be moved to the MFD part.  
> 
> Framers, like Ethernet PHYs, are reasonably well defined, because
> there are clear standards to follow. You could put the datasheets for
> the various frames side by side and quickly get an idea of the common
> properties. So you could define a structure now. In order to make it
> extendable, just avoid 0 having any meaning other than UNKNOWN. If you
> look at ethernet, SPEED_UNKNOWN is 0, DUPLEX_UNKNOWN is 0. So if a new
> field is added, we know if a driver has not filled it in.
> 
> > > And why is the notifier specific to the PEF2256? What would happen if
> > > i used a analog devices DS2155, DS21Q55, and DS2156, or the IDT
> > > 82P2281? Would each have its own notifier? And hence each would need
> > > its own PHY which translates one notifier into another?  
> > 
> > Each of them should have their own notifier if they can notify.  
> 
> I doubt you will find a framer which cannot report lost of framing. It
> is too much a part of the standards. There are signalling actions you
> need to do when a link goes does. So all framer drivers will have a
> notifier.
> 
> > At least they will need their own notifier at they PHY driver level.
> > Having or not a translation from something else would depend on each device
> > PHY driver implementation.  
> 
> Have you ever look at Ethernet PHYs? They all export the same API. You
> can plug any Linux MAC driver into any Linux Ethernet PHY driver. It
> should be the same here. You should be able to plug any HDLC driver
> into any Framer. I should be able to take the same SoC you have
> implementing the TDM interface, and plug it into the IDT framer i know
> of. We want a standardised API between the HDLC device and the framer.

Can you tell me more.
I thought I was providing a "standardised" API between the HDLC device
and the framer. Maybe it was not as complete as you could expect but I had
the feeling that it was standardised.
Right now it is just a link state notification provided by a simple 'basic
PHY'. Any HDLC device that uses a 'basic PHY' can be notified of any changes
of this link state provided by the PHY without knowing what is the exact
PHY behind this 'basic phy'.

> 
> > I would like this first implementation without too much API restriction
> > in order to see how it goes.
> > The actual proposal imposes nothing on the PHY internal implementation.
> > the pef2256 implementation chooses to have two notifiers (one at MFD
> > level and one at PHY level) but it was not imposed by the API.  
> 
> What i would like to see is some indication the API you are proposing
> is generic, and could be implemented by multiple frames and HDLC
> devices. The interface between the HDLC driver and the framer should
> be generic. The HDLC driver has an abstract reference to a framer. The
> framer has a reference to a netdev for the HDLC device.

Here, I do not fully understand.
Why does the framer need a reference to the netdev ?

> 
> You can keep this API very simple, just have link up/down
> notification, since that is all you want at the moment. But the
> implementation should give hints how it can be extended.
> 

Best regards,
Hervé

-- 
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RFC PATCH 0/4] Add support for QMC HDLC and PHY

[Andrew Lunn]

> My application (probably a subset of what we can do with E1 lines) does
> not use signaling.
> 
> I don't expose any channel to the user space but just:
> - One hdlc interface using one timeslot.
> - One or more dai links (audio channels) that can be used using ALSA library.

Well, that obviously does what you need for you application, but it
does not sound very generic. I don't think you could build a PBX on
top of that. I'd think you can build an E1 trunk to VoIP gateway.  So
i also have to wounder how generic vs one specific use case your
building blocks are.

Looking at the binding, i assume you could in fact instantiate 32 hdlc
instances? And that 'transparent' actually means DAI? So you can
instantiate as many DAI as you want. If you wanted to implement
signalling, a third type could be added?

> I thought I was providing a "standardised" API between the HDLC device
> and the framer. Maybe it was not as complete as you could expect but I had
> the feeling that it was standardised.

You are abusing the Generic PHY interface, for something which is not
a PHY. Your PHY driver does nothing a PHY driver normally does,
because you keep arguing the MFD does it all. So to me this is the
wrong abstraction.

You need an abstraction of a framer. You do however want a similar API
to generic PHY. devm_of_framer_optional_get() for the consumer of a
framer, which looks in DT for a phandle to a device.
devm_framer_create() which registers a framer provider, your MFD, with
the framer core. The hdlc driver can then get an instance of the
framer, and either put a notifier call block on its chain, or register
a function to be called when there is change in status.

What i also don't like is you have a very static configuration. You
are putting configuration into DT, which i'm surprised Rob Herring and
Krzysztof Kozlowski accepted. How you use E1 slots is not a hardware
property, it is purely software configuration, so should not be in DT.
So there should be a mechanism to configure how slots are used. You
can then dynamically instantiate HDLC interfaces and DAI links. This
is something which should be in the framer core. But since you have
managed to get this binding accepted, you can skip this. But
implementing the basic framer abstraction will give a place holder for
somebody to implement a much more generic solution in the future.

	 Andrew

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RFC PATCH 0/4] Add support for QMC HDLC and PHY

[Herve Codina]

Hi Andrew,

On Mon, 17 Apr 2023 19:22:26 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > My application (probably a subset of what we can do with E1 lines) does
> > not use signaling.
> > 
> > I don't expose any channel to the user space but just:
> > - One hdlc interface using one timeslot.
> > - One or more dai links (audio channels) that can be used using ALSA library.  
> 
> Well, that obviously does what you need for you application, but it
> does not sound very generic. I don't think you could build a PBX on
> top of that. I'd think you can build an E1 trunk to VoIP gateway.  So
> i also have to wounder how generic vs one specific use case your
> building blocks are.
> 
> Looking at the binding, i assume you could in fact instantiate 32 hdlc
> instances? And that 'transparent' actually means DAI? So you can
> instantiate as many DAI as you want. If you wanted to implement
> signalling, a third type could be added?
> 
> > I thought I was providing a "standardised" API between the HDLC device
> > and the framer. Maybe it was not as complete as you could expect but I had
> > the feeling that it was standardised.  
> 
> You are abusing the Generic PHY interface, for something which is not
> a PHY. Your PHY driver does nothing a PHY driver normally does,
> because you keep arguing the MFD does it all. So to me this is the
> wrong abstraction.
> 
> You need an abstraction of a framer. You do however want a similar API
> to generic PHY. devm_of_framer_optional_get() for the consumer of a
> framer, which looks in DT for a phandle to a device.
> devm_framer_create() which registers a framer provider, your MFD, with
> the framer core. The hdlc driver can then get an instance of the
> framer, and either put a notifier call block on its chain, or register
> a function to be called when there is change in status.
> 
> What i also don't like is you have a very static configuration. You
> are putting configuration into DT, which i'm surprised Rob Herring and
> Krzysztof Kozlowski accepted. How you use E1 slots is not a hardware
> property, it is purely software configuration, so should not be in DT.
> So there should be a mechanism to configure how slots are used. You
> can then dynamically instantiate HDLC interfaces and DAI links. This
> is something which should be in the framer core. But since you have
> managed to get this binding accepted, you can skip this. But
> implementing the basic framer abstraction will give a place holder for
> somebody to implement a much more generic solution in the future.
> 
> 	 Andrew

I can move to a basic framer abstraction as you suggested. At least:
- devm_of_framer_optional_get()
- devm_framer_create()
- framer_notifier_register() or something similar.

Where do you expect to see this framer abstraction and the pef2256
framer part ?
driver/net/wan/framer/, other place ?
I think driver/net/wan/framer/ can be a good place to start as only HDLC
will use this abstraction.

I can use the framer abstraction from the QMC HDLC driver itself or try
to move it to the HDLC core. Do you think it will be interesting to have
it move to the HDLC core ?

Best regards,
Hervé

-- 
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RFC PATCH 0/4] Add support for QMC HDLC and PHY

[Andrew Lunn]

> I can move to a basic framer abstraction as you suggested. At least:
> - devm_of_framer_optional_get()
> - devm_framer_create()
> - framer_notifier_register() or something similar.
> 
> Where do you expect to see this framer abstraction and the pef2256
> framer part ?
> driver/net/wan/framer/, other place ?

That seems like a good location.

> I think driver/net/wan/framer/ can be a good place to start as only HDLC
> will use this abstraction.

> I can use the framer abstraction from the QMC HDLC driver itself or try
> to move it to the HDLC core. Do you think it will be interesting to have
> it move to the HDLC core ?

Having it in the core would be nice. But i don't know that code, so i
cannot say how easy/hard it will be do to. hdlc.c already seems to
have some code for carrier. So you need to be careful not to break
that.

	Andrew

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[RFC PATCH 3/4] net: wan: fsl_qmc_hdlc: Add PHY support

[Herve Codina]

Add PHY support in the fsl_qmc_hdlc driver in order to be able to
signal carrier changes to the network stack based on the PHY status.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/net/wan/fsl_qmc_hdlc.c | 152 ++++++++++++++++++++++++++++++++-
 1 file changed, 151 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wan/fsl_qmc_hdlc.c b/drivers/net/wan/fsl_qmc_hdlc.c
index f12d00c78497..edea0f678ffe 100644
--- a/drivers/net/wan/fsl_qmc_hdlc.c
+++ b/drivers/net/wan/fsl_qmc_hdlc.c
@@ -12,6 +12,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_platform.h>
+#include <linux/phy/phy.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <soc/fsl/qe/qmc.h>
@@ -27,6 +28,11 @@ struct qmc_hdlc {
 	struct device *dev;
 	struct qmc_chan *qmc_chan;
 	struct net_device *netdev;
+	struct phy *phy;
+	spinlock_t carrier_lock; /* Protect carrier detection */
+	struct notifier_block nb;
+	bool is_phy_notifier;
+	struct delayed_work phy_poll_task;
 	bool is_crc32;
 	spinlock_t tx_lock; /* Protect tx descriptors */
 	struct qmc_hdlc_desc tx_descs[8];
@@ -39,6 +45,126 @@ static inline struct qmc_hdlc *netdev_to_qmc_hdlc(struct net_device *netdev)
 	return (struct qmc_hdlc *)dev_to_hdlc(netdev)->priv;
 }
 
+static int qmc_hdlc_phy_set_carrier(struct qmc_hdlc *qmc_hdlc)
+{
+	union phy_status phy_status;
+	unsigned long flags;
+	int ret;
+
+	if (!qmc_hdlc->phy)
+		return 0;
+
+	spin_lock_irqsave(&qmc_hdlc->carrier_lock, flags);
+
+	ret = phy_get_status(qmc_hdlc->phy, &phy_status);
+	if (ret) {
+		dev_err(qmc_hdlc->dev, "get PHY status failed (%d)\n", ret);
+		goto end;
+	}
+	if (phy_status.basic.link_is_on)
+		netif_carrier_on(qmc_hdlc->netdev);
+	else
+		netif_carrier_off(qmc_hdlc->netdev);
+
+end:
+	spin_unlock_irqrestore(&qmc_hdlc->carrier_lock, flags);
+	return ret;
+}
+
+static int qmc_hdlc_phy_notifier(struct notifier_block *nb, unsigned long action,
+				 void *data)
+{
+	struct qmc_hdlc *qmc_hdlc = container_of(nb, struct qmc_hdlc, nb);
+	int ret;
+
+	if (action != PHY_EVENT_STATUS)
+		return NOTIFY_DONE;
+
+	ret = qmc_hdlc_phy_set_carrier(qmc_hdlc);
+	return ret ? NOTIFY_DONE : NOTIFY_OK;
+}
+
+static void qmc_hdlc_phy_poll_task(struct work_struct *work)
+{
+	struct qmc_hdlc *qmc_hdlc = container_of(work, struct qmc_hdlc, phy_poll_task.work);
+	int ret;
+
+	ret = qmc_hdlc_phy_set_carrier(qmc_hdlc);
+	if (ret) {
+		/* Should not happened but ...
+		 * On error, force carrier on and stop scheduling this task
+		 */
+		dev_err(qmc_hdlc->dev, "set carrier failed (%d) -> force carrier on\n",
+			ret);
+		netif_carrier_on(qmc_hdlc->netdev);
+		return;
+	}
+
+	/* Re-schedule task in 1 sec */
+	queue_delayed_work(system_power_efficient_wq, &qmc_hdlc->phy_poll_task, 1 * HZ);
+}
+
+static int qmc_hdlc_phy_init(struct qmc_hdlc *qmc_hdlc)
+{
+	union phy_status phy_status;
+	int ret;
+
+	if (!qmc_hdlc->phy)
+		return 0;
+
+	ret = phy_init(qmc_hdlc->phy);
+	if (ret) {
+		dev_err(qmc_hdlc->dev, "PHY init failed (%d)\n", ret);
+		return ret;
+	}
+
+	ret = phy_power_on(qmc_hdlc->phy);
+	if (ret) {
+		dev_err(qmc_hdlc->dev, "PHY power-on failed (%d)\n", ret);
+		goto phy_exit;
+	}
+
+	/* Be sure that get_status is supported */
+	ret = phy_get_status(qmc_hdlc->phy, &phy_status);
+	if (ret) {
+		dev_err(qmc_hdlc->dev, "get PHY status failed (%d)\n", ret);
+		goto phy_power_off;
+	}
+
+	qmc_hdlc->nb.notifier_call = qmc_hdlc_phy_notifier;
+	ret = phy_atomic_notifier_register(qmc_hdlc->phy, &qmc_hdlc->nb);
+	if (ret) {
+		qmc_hdlc->is_phy_notifier = false;
+
+		/* Cannot register a PHY notifier -> Use polling */
+		INIT_DELAYED_WORK(&qmc_hdlc->phy_poll_task, qmc_hdlc_phy_poll_task);
+		queue_delayed_work(system_power_efficient_wq, &qmc_hdlc->phy_poll_task, 1 * HZ);
+	} else {
+		qmc_hdlc->is_phy_notifier = true;
+	}
+
+	return 0;
+
+phy_power_off:
+	phy_power_off(qmc_hdlc->phy);
+phy_exit:
+	phy_exit(qmc_hdlc->phy);
+	return ret;
+}
+
+static void qmc_hdlc_phy_exit(struct qmc_hdlc *qmc_hdlc)
+{
+	if (!qmc_hdlc->phy)
+		return;
+
+	if (qmc_hdlc->is_phy_notifier)
+		phy_atomic_notifier_unregister(qmc_hdlc->phy, &qmc_hdlc->nb);
+	else
+		cancel_delayed_work_sync(&qmc_hdlc->phy_poll_task);
+	phy_power_off(qmc_hdlc->phy);
+	phy_exit(qmc_hdlc->phy);
+}
+
 static int qmc_hdlc_recv_queue(struct qmc_hdlc *qmc_hdlc, struct qmc_hdlc_desc *desc, size_t size);
 
 static void qmc_hcld_recv_complete(void *context, size_t length)
@@ -192,10 +318,17 @@ static int qmc_hdlc_open(struct net_device *netdev)
 	int ret;
 	int i;
 
-	ret = hdlc_open(netdev);
+	ret = qmc_hdlc_phy_init(qmc_hdlc);
 	if (ret)
 		return ret;
 
+	ret = hdlc_open(netdev);
+	if (ret)
+		goto phy_exit;
+
+	/* Update carrier */
+	qmc_hdlc_phy_set_carrier(qmc_hdlc);
+
 	chan_param.mode = QMC_HDLC;
 	/* HDLC_MAX_MRU + 4 for the CRC
 	 * HDLC_MAX_MRU + 4 + 8 for the CRC and some extraspace needed by the QMC
@@ -245,6 +378,8 @@ static int qmc_hdlc_open(struct net_device *netdev)
 	}
 hdlc_close:
 	hdlc_close(netdev);
+phy_exit:
+	qmc_hdlc_phy_exit(qmc_hdlc);
 	return ret;
 }
 
@@ -280,6 +415,7 @@ static int qmc_hdlc_close(struct net_device *netdev)
 	}
 
 	hdlc_close(netdev);
+	qmc_hdlc_phy_exit(qmc_hdlc);
 	return 0;
 }
 
@@ -318,6 +454,7 @@ static int qmc_hdlc_probe(struct platform_device *pdev)
 	struct device_node *np = pdev->dev.of_node;
 	struct qmc_hdlc *qmc_hdlc;
 	struct qmc_chan_info info;
+	enum phy_mode phy_mode;
 	hdlc_device *hdlc;
 	int ret;
 
@@ -327,6 +464,7 @@ static int qmc_hdlc_probe(struct platform_device *pdev)
 
 	qmc_hdlc->dev = &pdev->dev;
 	spin_lock_init(&qmc_hdlc->tx_lock);
+	spin_lock_init(&qmc_hdlc->carrier_lock);
 
 	qmc_hdlc->qmc_chan = devm_qmc_chan_get_byphandle(qmc_hdlc->dev, np, "fsl,qmc-chan");
 	if (IS_ERR(qmc_hdlc->qmc_chan)) {
@@ -348,6 +486,18 @@ static int qmc_hdlc_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	qmc_hdlc->phy = devm_of_phy_optional_get(qmc_hdlc->dev, np, NULL);
+	if (IS_ERR(qmc_hdlc->phy))
+		return PTR_ERR(qmc_hdlc->phy);
+	if (qmc_hdlc->phy) {
+		phy_mode = phy_get_mode(qmc_hdlc->phy);
+		if (phy_mode != PHY_MODE_BASIC) {
+			dev_err(qmc_hdlc->dev, "Unsupported PHY mode (%d)\n",
+				phy_mode);
+			return -EINVAL;
+		}
+	}
+
 	qmc_hdlc->netdev = alloc_hdlcdev(qmc_hdlc);
 	if (!qmc_hdlc->netdev) {
 		dev_err(qmc_hdlc->dev, "failed to alloc hdlc dev\n");
-- 
2.39.2


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy