Hi,
Note: Resent this v3 series with missing maintainers added in CC.
This series introduces the QMC HDLC support.
Patches were previously sent as part of a full feature series and were
previously reviewed in that context:
"Add support for QMC HDLC, framer infrastructure and PEF2256 framer" [1]
In order to ease the merge, the full feature series has been split and
needed parts were merged in v6.8-rc1:
- "Prepare the PowerQUICC QMC and TSA for the HDLC QMC driver" [2]
- "Add support for framer infrastructure and PEF2256 framer" [3]
This series contains patches related to the QMC HDLC part (QMC HDLC
driver):
- Introduce the QMC HDLC driver (patches 1 and 2)
- Add timeslots change support in QMC HDLC (patch 3)
- Add framer support as a framer consumer in QMC HDLC (patch 4)
Compare to the original full feature series, a modification was done on
patch 3 in order to use a coherent prefix in the commit title.
I kept the patches unsquashed as they were previously sent and reviewed.
Of course, I can squash them if needed.
Compared to the previous iteration:
https://lore.kernel.org/linux-kernel/[email protected]/
this v3 series:
- Remove 'inline' function specifier from .c file.
- Fixed a bug introduced in the previous iteration.
- Remove one lock/unlock sequence in the QMC HDCL xmit path.
- Use bitmap_from_u64().
Best regards,
Hervé
[1]: https://lore.kernel.org/linux-kernel/[email protected]/
[2]: https://lore.kernel.org/linux-kernel/[email protected]/
[3]: https://lore.kernel.org/linux-kernel/[email protected]/
Changes v2 -> v3
- Patch 1
Remove 'inline' function specifier from .c file.
Fix a bug introduced when added WARN_ONCE(). The warn condition must
be desc->skb (descriptor used) instead of !desc->skb.
Remove a lock/unlock section locking the entire qmc_hdlc_xmit()
function.
- Patch 5
Use bitmap_from_u64() everywhere instead of bitmap_from_arr32() and
bitmap_from_arr64().
Changes v1 -> v2
- Patch 1
Use the same qmc_hdlc initialisation in qmc_hcld_recv_complete()
than the one present in qmc_hcld_xmit_complete().
Use WARN_ONCE()
- Patch 3 (new patch in v2)
Make bitmap_onto() available to users
- Patch 4 (new patch in v2)
Introduce bitmap_off()
- Patch 5 (patch 3 in v1)
Use bitmap_*() functions
- Patch 6 (patch 4 in v1)
No changes
Changes compare to the full feature series:
- Patch 3
Use 'net: wan: fsl_qmc_hdlc:' as commit title prefix
Patches extracted:
- Patch 1 : full feature series patch 7
- Patch 2 : full feature series patch 8
- Patch 3 : full feature series patch 20
- Patch 4 : full feature series patch 27
Herve Codina (6):
net: wan: Add support for QMC HDLC
MAINTAINERS: Add the Freescale QMC HDLC driver entry
bitmap: Make bitmap_onto() available to users
bitmap: Introduce bitmap_off()
net: wan: fsl_qmc_hdlc: Add runtime timeslots changes support
net: wan: fsl_qmc_hdlc: Add framer support
MAINTAINERS | 7 +
drivers/net/wan/Kconfig | 12 +
drivers/net/wan/Makefile | 1 +
drivers/net/wan/fsl_qmc_hdlc.c | 807 +++++++++++++++++++++++++++++++++
include/linux/bitmap.h | 3 +
lib/bitmap.c | 45 +-
6 files changed, 874 insertions(+), 1 deletion(-)
create mode 100644 drivers/net/wan/fsl_qmc_hdlc.c
--
2.43.0
After contributing the driver, add myself as the maintainer for the
Freescale QMC HDLC driver.
Signed-off-by: Herve Codina <[email protected]>
---
MAINTAINERS | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 8d1052fa6a69..15cd3a8e5866 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8584,6 +8584,13 @@ F: Documentation/devicetree/bindings/soc/fsl/cpm_qe/fsl,cpm1-scc-qmc.yaml
F: drivers/soc/fsl/qe/qmc.c
F: include/soc/fsl/qe/qmc.h
+FREESCALE QUICC ENGINE QMC HDLC DRIVER
+M: Herve Codina <[email protected]>
+L: [email protected]
+L: [email protected]
+S: Maintained
+F: drivers/net/wan/fsl_qmc_hdlc.c
+
FREESCALE QUICC ENGINE TSA DRIVER
M: Herve Codina <[email protected]>
L: [email protected]
--
2.43.0
After contributing the driver, add myself as the maintainer for the
Freescale QMC HDLC driver.
Signed-off-by: Herve Codina <[email protected]>
---
MAINTAINERS | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 8d1052fa6a69..15cd3a8e5866 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8584,6 +8584,13 @@ F: Documentation/devicetree/bindings/soc/fsl/cpm_qe/fsl,cpm1-scc-qmc.yaml
F: drivers/soc/fsl/qe/qmc.c
F: include/soc/fsl/qe/qmc.h
+FREESCALE QUICC ENGINE QMC HDLC DRIVER
+M: Herve Codina <[email protected]>
+L: [email protected]
+L: [email protected]
+S: Maintained
+F: drivers/net/wan/fsl_qmc_hdlc.c
+
FREESCALE QUICC ENGINE TSA DRIVER
M: Herve Codina <[email protected]>
L: [email protected]
--
2.43.0
The QMC HDLC driver provides support for HDLC using the QMC (QUICC
Multichannel Controller) to transfer the HDLC data.
Signed-off-by: Herve Codina <[email protected]>
Reviewed-by: Christophe Leroy <[email protected]>
Acked-by: Jakub Kicinski <[email protected]>
---
drivers/net/wan/Kconfig | 12 +
drivers/net/wan/Makefile | 1 +
drivers/net/wan/fsl_qmc_hdlc.c | 426 +++++++++++++++++++++++++++++++++
3 files changed, 439 insertions(+)
create mode 100644 drivers/net/wan/fsl_qmc_hdlc.c
diff --git a/drivers/net/wan/Kconfig b/drivers/net/wan/Kconfig
index 7dda87756d3f..31ab2136cdf1 100644
--- a/drivers/net/wan/Kconfig
+++ b/drivers/net/wan/Kconfig
@@ -197,6 +197,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 8119b49d1da9..00e9b7ee1e01 100644
--- a/drivers/net/wan/Makefile
+++ b/drivers/net/wan/Makefile
@@ -25,6 +25,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..835500910d1b
--- /dev/null
+++ b/drivers/net/wan/fsl_qmc_hdlc.c
@@ -0,0 +1,426 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Freescale QMC HDLC Device Driver
+ *
+ * Copyright 2023 CS GROUP France
+ *
+ * Author: Herve Codina <[email protected]>
+ */
+
+#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 struct qmc_hdlc *netdev_to_qmc_hdlc(struct net_device *netdev)
+{
+ return dev_to_hdlc(netdev)->priv;
+}
+
+static int qmc_hdlc_recv_queue(struct qmc_hdlc *qmc_hdlc, struct qmc_hdlc_desc *desc, size_t size);
+
+#define QMC_HDLC_RX_ERROR_FLAGS (QMC_RX_FLAG_HDLC_OVF | \
+ QMC_RX_FLAG_HDLC_UNA | \
+ QMC_RX_FLAG_HDLC_ABORT | \
+ QMC_RX_FLAG_HDLC_CRC)
+
+static void qmc_hcld_recv_complete(void *context, size_t length, unsigned int flags)
+{
+ struct qmc_hdlc_desc *desc = context;
+ struct net_device *netdev = desc->netdev;
+ struct qmc_hdlc *qmc_hdlc = netdev_to_qmc_hdlc(netdev);
+ int ret;
+
+ dma_unmap_single(qmc_hdlc->dev, desc->dma_addr, desc->dma_size, DMA_FROM_DEVICE);
+
+ if (flags & QMC_HDLC_RX_ERROR_FLAGS) {
+ netdev->stats.rx_errors++;
+ if (flags & QMC_RX_FLAG_HDLC_OVF) /* Data overflow */
+ netdev->stats.rx_over_errors++;
+ if (flags & QMC_RX_FLAG_HDLC_UNA) /* bits received not multiple of 8 */
+ netdev->stats.rx_frame_errors++;
+ if (flags & QMC_RX_FLAG_HDLC_ABORT) /* Received an abort sequence */
+ netdev->stats.rx_frame_errors++;
+ if (flags & QMC_RX_FLAG_HDLC_CRC) /* CRC error */
+ netdev->stats.rx_crc_errors++;
+ kfree_skb(desc->skb);
+ } else {
+ 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 err;
+ int ret;
+
+ spin_lock_irqsave(&qmc_hdlc->tx_lock, flags);
+
+ desc = &qmc_hdlc->tx_descs[qmc_hdlc->tx_out];
+ if (WARN_ONCE(desc->skb, "No tx descriptors available\n")) {
+ /* Should never happen.
+ * Previous xmit should have already stopped the queue.
+ */
+ netif_stop_queue(netdev);
+ ret = NETDEV_TX_BUSY;
+ goto end;
+ }
+
+ desc->netdev = netdev;
+ desc->dma_size = skb->len;
+ desc->skb = skb;
+ err = qmc_hdlc_xmit_queue(qmc_hdlc, desc);
+ if (err) {
+ desc->skb = NULL; /* Release the descriptor */
+ if (err == -EBUSY) {
+ netif_stop_queue(netdev);
+ ret = NETDEV_TX_BUSY;
+ goto end;
+ }
+ dev_kfree_skb(skb);
+ netdev->stats.tx_dropped++;
+ ret = NETDEV_TX_OK;
+ goto end;
+ }
+
+ qmc_hdlc->tx_out = (qmc_hdlc->tx_out + 1) % ARRAY_SIZE(qmc_hdlc->tx_descs);
+
+ if (qmc_hdlc->tx_descs[qmc_hdlc->tx_out].skb)
+ netif_stop_queue(netdev);
+
+ ret = NETDEV_TX_OK;
+end:
+ spin_unlock_irqrestore(&qmc_hdlc->tx_lock, flags);
+ return ret;
+}
+
+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;
+
+ qmc_chan_stop(qmc_hdlc->qmc_chan, QMC_CHAN_ALL);
+ qmc_chan_reset(qmc_hdlc->qmc_chan, QMC_CHAN_ALL);
+
+ netif_stop_queue(netdev);
+
+ 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_bychild(qmc_hdlc->dev, np);
+ 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;
+ }
+
+ 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);
+
+ 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 <[email protected]>");
+MODULE_DESCRIPTION("QMC HDLC driver");
+MODULE_LICENSE("GPL");
--
2.43.0
The QMC HDLC driver provides support for HDLC using the QMC (QUICC
Multichannel Controller) to transfer the HDLC data.
Signed-off-by: Herve Codina <[email protected]>
Reviewed-by: Christophe Leroy <[email protected]>
Acked-by: Jakub Kicinski <[email protected]>
---
drivers/net/wan/Kconfig | 12 +
drivers/net/wan/Makefile | 1 +
drivers/net/wan/fsl_qmc_hdlc.c | 426 +++++++++++++++++++++++++++++++++
3 files changed, 439 insertions(+)
create mode 100644 drivers/net/wan/fsl_qmc_hdlc.c
diff --git a/drivers/net/wan/Kconfig b/drivers/net/wan/Kconfig
index 7dda87756d3f..31ab2136cdf1 100644
--- a/drivers/net/wan/Kconfig
+++ b/drivers/net/wan/Kconfig
@@ -197,6 +197,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 8119b49d1da9..00e9b7ee1e01 100644
--- a/drivers/net/wan/Makefile
+++ b/drivers/net/wan/Makefile
@@ -25,6 +25,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..835500910d1b
--- /dev/null
+++ b/drivers/net/wan/fsl_qmc_hdlc.c
@@ -0,0 +1,426 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Freescale QMC HDLC Device Driver
+ *
+ * Copyright 2023 CS GROUP France
+ *
+ * Author: Herve Codina <[email protected]>
+ */
+
+#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 struct qmc_hdlc *netdev_to_qmc_hdlc(struct net_device *netdev)
+{
+ return dev_to_hdlc(netdev)->priv;
+}
+
+static int qmc_hdlc_recv_queue(struct qmc_hdlc *qmc_hdlc, struct qmc_hdlc_desc *desc, size_t size);
+
+#define QMC_HDLC_RX_ERROR_FLAGS (QMC_RX_FLAG_HDLC_OVF | \
+ QMC_RX_FLAG_HDLC_UNA | \
+ QMC_RX_FLAG_HDLC_ABORT | \
+ QMC_RX_FLAG_HDLC_CRC)
+
+static void qmc_hcld_recv_complete(void *context, size_t length, unsigned int flags)
+{
+ struct qmc_hdlc_desc *desc = context;
+ struct net_device *netdev = desc->netdev;
+ struct qmc_hdlc *qmc_hdlc = netdev_to_qmc_hdlc(netdev);
+ int ret;
+
+ dma_unmap_single(qmc_hdlc->dev, desc->dma_addr, desc->dma_size, DMA_FROM_DEVICE);
+
+ if (flags & QMC_HDLC_RX_ERROR_FLAGS) {
+ netdev->stats.rx_errors++;
+ if (flags & QMC_RX_FLAG_HDLC_OVF) /* Data overflow */
+ netdev->stats.rx_over_errors++;
+ if (flags & QMC_RX_FLAG_HDLC_UNA) /* bits received not multiple of 8 */
+ netdev->stats.rx_frame_errors++;
+ if (flags & QMC_RX_FLAG_HDLC_ABORT) /* Received an abort sequence */
+ netdev->stats.rx_frame_errors++;
+ if (flags & QMC_RX_FLAG_HDLC_CRC) /* CRC error */
+ netdev->stats.rx_crc_errors++;
+ kfree_skb(desc->skb);
+ } else {
+ 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 err;
+ int ret;
+
+ spin_lock_irqsave(&qmc_hdlc->tx_lock, flags);
+
+ desc = &qmc_hdlc->tx_descs[qmc_hdlc->tx_out];
+ if (WARN_ONCE(desc->skb, "No tx descriptors available\n")) {
+ /* Should never happen.
+ * Previous xmit should have already stopped the queue.
+ */
+ netif_stop_queue(netdev);
+ ret = NETDEV_TX_BUSY;
+ goto end;
+ }
+
+ desc->netdev = netdev;
+ desc->dma_size = skb->len;
+ desc->skb = skb;
+ err = qmc_hdlc_xmit_queue(qmc_hdlc, desc);
+ if (err) {
+ desc->skb = NULL; /* Release the descriptor */
+ if (err == -EBUSY) {
+ netif_stop_queue(netdev);
+ ret = NETDEV_TX_BUSY;
+ goto end;
+ }
+ dev_kfree_skb(skb);
+ netdev->stats.tx_dropped++;
+ ret = NETDEV_TX_OK;
+ goto end;
+ }
+
+ qmc_hdlc->tx_out = (qmc_hdlc->tx_out + 1) % ARRAY_SIZE(qmc_hdlc->tx_descs);
+
+ if (qmc_hdlc->tx_descs[qmc_hdlc->tx_out].skb)
+ netif_stop_queue(netdev);
+
+ ret = NETDEV_TX_OK;
+end:
+ spin_unlock_irqrestore(&qmc_hdlc->tx_lock, flags);
+ return ret;
+}
+
+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;
+
+ qmc_chan_stop(qmc_hdlc->qmc_chan, QMC_CHAN_ALL);
+ qmc_chan_reset(qmc_hdlc->qmc_chan, QMC_CHAN_ALL);
+
+ netif_stop_queue(netdev);
+
+ 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_bychild(qmc_hdlc->dev, np);
+ 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;
+ }
+
+ 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);
+
+ 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 <[email protected]>");
+MODULE_DESCRIPTION("QMC HDLC driver");
+MODULE_LICENSE("GPL");
--
2.43.0
Currently the bitmap_onto() is available only for CONFIG_NUMA=y case,
while some users may benefit out of it and being independent to NUMA
code.
Make it available to users by moving out of ifdeffery and exporting for
modules.
Signed-off-by: Herve Codina <[email protected]>
---
lib/bitmap.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/bitmap.c b/lib/bitmap.c
index 09522af227f1..2feccb5047dc 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -547,7 +547,6 @@ int bitmap_bitremap(int oldbit, const unsigned long *old,
}
EXPORT_SYMBOL(bitmap_bitremap);
-#ifdef CONFIG_NUMA
/**
* bitmap_onto - translate one bitmap relative to another
* @dst: resulting translated bitmap
@@ -681,7 +680,9 @@ void bitmap_onto(unsigned long *dst, const unsigned long *orig,
m++;
}
}
+EXPORT_SYMBOL(bitmap_onto);
+#ifdef CONFIG_NUMA
/**
* bitmap_fold - fold larger bitmap into smaller, modulo specified size
* @dst: resulting smaller bitmap
--
2.43.0
Hi,
Note: Resent this v3 series with missing maintainers added in CC.
This series introduces the QMC HDLC support.
Patches were previously sent as part of a full feature series and were
previously reviewed in that context:
"Add support for QMC HDLC, framer infrastructure and PEF2256 framer" [1]
In order to ease the merge, the full feature series has been split and
needed parts were merged in v6.8-rc1:
- "Prepare the PowerQUICC QMC and TSA for the HDLC QMC driver" [2]
- "Add support for framer infrastructure and PEF2256 framer" [3]
This series contains patches related to the QMC HDLC part (QMC HDLC
driver):
- Introduce the QMC HDLC driver (patches 1 and 2)
- Add timeslots change support in QMC HDLC (patch 3)
- Add framer support as a framer consumer in QMC HDLC (patch 4)
Compare to the original full feature series, a modification was done on
patch 3 in order to use a coherent prefix in the commit title.
I kept the patches unsquashed as they were previously sent and reviewed.
Of course, I can squash them if needed.
Compared to the previous iteration:
https://lore.kernel.org/linux-kernel/[email protected]/
this v3 series:
- Remove 'inline' function specifier from .c file.
- Fixed a bug introduced in the previous iteration.
- Remove one lock/unlock sequence in the QMC HDCL xmit path.
- Use bitmap_from_u64().
Best regards,
Hervé
[1]: https://lore.kernel.org/linux-kernel/[email protected]/
[2]: https://lore.kernel.org/linux-kernel/[email protected]/
[3]: https://lore.kernel.org/linux-kernel/[email protected]/
Changes v2 -> v3
- Patch 1
Remove 'inline' function specifier from .c file.
Fix a bug introduced when added WARN_ONCE(). The warn condition must
be desc->skb (descriptor used) instead of !desc->skb.
Remove a lock/unlock section locking the entire qmc_hdlc_xmit()
function.
- Patch 5
Use bitmap_from_u64() everywhere instead of bitmap_from_arr32() and
bitmap_from_arr64().
Changes v1 -> v2
- Patch 1
Use the same qmc_hdlc initialisation in qmc_hcld_recv_complete()
than the one present in qmc_hcld_xmit_complete().
Use WARN_ONCE()
- Patch 3 (new patch in v2)
Make bitmap_onto() available to users
- Patch 4 (new patch in v2)
Introduce bitmap_off()
- Patch 5 (patch 3 in v1)
Use bitmap_*() functions
- Patch 6 (patch 4 in v1)
No changes
Changes compare to the full feature series:
- Patch 3
Use 'net: wan: fsl_qmc_hdlc:' as commit title prefix
Patches extracted:
- Patch 1 : full feature series patch 7
- Patch 2 : full feature series patch 8
- Patch 3 : full feature series patch 20
- Patch 4 : full feature series patch 27
Herve Codina (6):
net: wan: Add support for QMC HDLC
MAINTAINERS: Add the Freescale QMC HDLC driver entry
bitmap: Make bitmap_onto() available to users
bitmap: Introduce bitmap_off()
net: wan: fsl_qmc_hdlc: Add runtime timeslots changes support
net: wan: fsl_qmc_hdlc: Add framer support
MAINTAINERS | 7 +
drivers/net/wan/Kconfig | 12 +
drivers/net/wan/Makefile | 1 +
drivers/net/wan/fsl_qmc_hdlc.c | 807 +++++++++++++++++++++++++++++++++
include/linux/bitmap.h | 3 +
lib/bitmap.c | 45 +-
6 files changed, 874 insertions(+), 1 deletion(-)
create mode 100644 drivers/net/wan/fsl_qmc_hdlc.c
--
2.43.0
The bitmap_onto() function translates one bitmap relative to another but
no function are present to perform the reverse translation.
Introduce bitmap_off() to fill this hole.
Signed-off-by: Herve Codina <[email protected]>
---
include/linux/bitmap.h | 3 +++
lib/bitmap.c | 42 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 45 insertions(+)
diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
index 99451431e4d6..5ecfcbbc91f4 100644
--- a/include/linux/bitmap.h
+++ b/include/linux/bitmap.h
@@ -65,6 +65,7 @@ struct device;
* bitmap_remap(dst, src, old, new, nbits) *dst = map(old, new)(src)
* bitmap_bitremap(oldbit, old, new, nbits) newbit = map(old, new)(oldbit)
* bitmap_onto(dst, orig, relmap, nbits) *dst = orig relative to relmap
+ * bitmap_off(dst, orig, relmap, nbits) *dst = bitmap_onto() reverse operation
* bitmap_fold(dst, orig, sz, nbits) dst bits = orig bits mod sz
* bitmap_parse(buf, buflen, dst, nbits) Parse bitmap dst from kernel buf
* bitmap_parse_user(ubuf, ulen, dst, nbits) Parse bitmap dst from user buf
@@ -208,6 +209,8 @@ int bitmap_bitremap(int oldbit,
const unsigned long *old, const unsigned long *new, int bits);
void bitmap_onto(unsigned long *dst, const unsigned long *orig,
const unsigned long *relmap, unsigned int bits);
+void bitmap_off(unsigned long *dst, const unsigned long *orig,
+ const unsigned long *relmap, unsigned int bits);
void bitmap_fold(unsigned long *dst, const unsigned long *orig,
unsigned int sz, unsigned int nbits);
diff --git a/lib/bitmap.c b/lib/bitmap.c
index 2feccb5047dc..71343967335e 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -682,6 +682,48 @@ void bitmap_onto(unsigned long *dst, const unsigned long *orig,
}
EXPORT_SYMBOL(bitmap_onto);
+/**
+ * bitmap_off - revert operation done by bitmap_onto()
+ * @dst: resulting translated bitmap
+ * @orig: original untranslated bitmap
+ * @relmap: bitmap relative to which translated
+ * @bits: number of bits in each of these bitmaps
+ *
+ * Suppose onto computed using bitmap_onto(onto, src, relmap, n)
+ * The operation bitmap_off(result, onto, relmap, n) leads to a
+ * result equal or equivalent to src.
+ *
+ * The result can be 'equivalent' because bitmap_onto() and
+ * bitmap_off() are not bijective.
+ * The result and src values are equivalent in that sense that a
+ * call to bitmap_onto(onto, src, relmap, n) and a call to
+ * bitmap_onto(onto, result, relmap, n) will lead to the same onto
+ * value.
+ *
+ * If either of @orig or @relmap is empty (no set bits), then @dst
+ * will be returned empty.
+ *
+ * All bits in @dst not set by the above rule are cleared.
+ */
+void bitmap_off(unsigned long *dst, const unsigned long *orig,
+ const unsigned long *relmap, unsigned int bits)
+{
+ unsigned int n, m; /* same meaning as in above comment */
+
+ if (dst == orig) /* following doesn't handle inplace mappings */
+ return;
+ bitmap_zero(dst, bits);
+
+ m = 0;
+ for_each_set_bit(n, relmap, bits) {
+ /* m == bitmap_pos_to_ord(relmap, n, bits) */
+ if (test_bit(n, orig))
+ set_bit(m, dst);
+ m++;
+ }
+}
+EXPORT_SYMBOL(bitmap_off);
+
#ifdef CONFIG_NUMA
/**
* bitmap_fold - fold larger bitmap into smaller, modulo specified size
--
2.43.0
Currently the bitmap_onto() is available only for CONFIG_NUMA=y case,
while some users may benefit out of it and being independent to NUMA
code.
Make it available to users by moving out of ifdeffery and exporting for
modules.
Signed-off-by: Herve Codina <[email protected]>
---
lib/bitmap.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/bitmap.c b/lib/bitmap.c
index 09522af227f1..2feccb5047dc 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -547,7 +547,6 @@ int bitmap_bitremap(int oldbit, const unsigned long *old,
}
EXPORT_SYMBOL(bitmap_bitremap);
-#ifdef CONFIG_NUMA
/**
* bitmap_onto - translate one bitmap relative to another
* @dst: resulting translated bitmap
@@ -681,7 +680,9 @@ void bitmap_onto(unsigned long *dst, const unsigned long *orig,
m++;
}
}
+EXPORT_SYMBOL(bitmap_onto);
+#ifdef CONFIG_NUMA
/**
* bitmap_fold - fold larger bitmap into smaller, modulo specified size
* @dst: resulting smaller bitmap
--
2.43.0
The bitmap_onto() function translates one bitmap relative to another but
no function are present to perform the reverse translation.
Introduce bitmap_off() to fill this hole.
Signed-off-by: Herve Codina <[email protected]>
---
include/linux/bitmap.h | 3 +++
lib/bitmap.c | 42 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 45 insertions(+)
diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
index 99451431e4d6..5ecfcbbc91f4 100644
--- a/include/linux/bitmap.h
+++ b/include/linux/bitmap.h
@@ -65,6 +65,7 @@ struct device;
* bitmap_remap(dst, src, old, new, nbits) *dst = map(old, new)(src)
* bitmap_bitremap(oldbit, old, new, nbits) newbit = map(old, new)(oldbit)
* bitmap_onto(dst, orig, relmap, nbits) *dst = orig relative to relmap
+ * bitmap_off(dst, orig, relmap, nbits) *dst = bitmap_onto() reverse operation
* bitmap_fold(dst, orig, sz, nbits) dst bits = orig bits mod sz
* bitmap_parse(buf, buflen, dst, nbits) Parse bitmap dst from kernel buf
* bitmap_parse_user(ubuf, ulen, dst, nbits) Parse bitmap dst from user buf
@@ -208,6 +209,8 @@ int bitmap_bitremap(int oldbit,
const unsigned long *old, const unsigned long *new, int bits);
void bitmap_onto(unsigned long *dst, const unsigned long *orig,
const unsigned long *relmap, unsigned int bits);
+void bitmap_off(unsigned long *dst, const unsigned long *orig,
+ const unsigned long *relmap, unsigned int bits);
void bitmap_fold(unsigned long *dst, const unsigned long *orig,
unsigned int sz, unsigned int nbits);
diff --git a/lib/bitmap.c b/lib/bitmap.c
index 2feccb5047dc..71343967335e 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -682,6 +682,48 @@ void bitmap_onto(unsigned long *dst, const unsigned long *orig,
}
EXPORT_SYMBOL(bitmap_onto);
+/**
+ * bitmap_off - revert operation done by bitmap_onto()
+ * @dst: resulting translated bitmap
+ * @orig: original untranslated bitmap
+ * @relmap: bitmap relative to which translated
+ * @bits: number of bits in each of these bitmaps
+ *
+ * Suppose onto computed using bitmap_onto(onto, src, relmap, n)
+ * The operation bitmap_off(result, onto, relmap, n) leads to a
+ * result equal or equivalent to src.
+ *
+ * The result can be 'equivalent' because bitmap_onto() and
+ * bitmap_off() are not bijective.
+ * The result and src values are equivalent in that sense that a
+ * call to bitmap_onto(onto, src, relmap, n) and a call to
+ * bitmap_onto(onto, result, relmap, n) will lead to the same onto
+ * value.
+ *
+ * If either of @orig or @relmap is empty (no set bits), then @dst
+ * will be returned empty.
+ *
+ * All bits in @dst not set by the above rule are cleared.
+ */
+void bitmap_off(unsigned long *dst, const unsigned long *orig,
+ const unsigned long *relmap, unsigned int bits)
+{
+ unsigned int n, m; /* same meaning as in above comment */
+
+ if (dst == orig) /* following doesn't handle inplace mappings */
+ return;
+ bitmap_zero(dst, bits);
+
+ m = 0;
+ for_each_set_bit(n, relmap, bits) {
+ /* m == bitmap_pos_to_ord(relmap, n, bits) */
+ if (test_bit(n, orig))
+ set_bit(m, dst);
+ m++;
+ }
+}
+EXPORT_SYMBOL(bitmap_off);
+
#ifdef CONFIG_NUMA
/**
* bitmap_fold - fold larger bitmap into smaller, modulo specified size
--
2.43.0
QMC channels support runtime timeslots changes but nothing is done at
the QMC HDLC driver to handle these changes.
Use existing IFACE ioctl in order to configure the timeslots to use.
Signed-off-by: Herve Codina <[email protected]>
Reviewed-by: Christophe Leroy <[email protected]>
Acked-by: Jakub Kicinski <[email protected]>
---
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 835500910d1b..b25d918d5e4e 100644
--- a/drivers/net/wan/fsl_qmc_hdlc.c
+++ b/drivers/net/wan/fsl_qmc_hdlc.c
@@ -7,6 +7,7 @@
* Author: Herve Codina <[email protected]>
*/
+#include <linux/bitmap.h>
#include <linux/dma-mapping.h>
#include <linux/hdlc.h>
#include <linux/module.h>
@@ -32,6 +33,7 @@ struct qmc_hdlc {
struct qmc_hdlc_desc tx_descs[8];
unsigned int tx_out;
struct qmc_hdlc_desc rx_descs[4];
+ u32 slot_map;
};
static struct qmc_hdlc *netdev_to_qmc_hdlc(struct net_device *netdev)
@@ -206,6 +208,144 @@ static netdev_tx_t qmc_hdlc_xmit(struct sk_buff *skb, struct net_device *netdev)
return ret;
}
+static int qmc_hdlc_xlate_slot_map(struct qmc_hdlc *qmc_hdlc,
+ u32 slot_map, struct qmc_chan_ts_info *ts_info)
+{
+ DECLARE_BITMAP(ts_mask_avail, 64);
+ DECLARE_BITMAP(ts_mask, 64);
+ DECLARE_BITMAP(map, 64);
+
+ /* Tx and Rx available masks must be identical */
+ if (ts_info->rx_ts_mask_avail != ts_info->tx_ts_mask_avail) {
+ dev_err(qmc_hdlc->dev, "tx and rx available timeslots mismatch (0x%llx, 0x%llx)\n",
+ ts_info->rx_ts_mask_avail, ts_info->tx_ts_mask_avail);
+ return -EINVAL;
+ }
+
+ bitmap_from_u64(ts_mask_avail, ts_info->rx_ts_mask_avail);
+ bitmap_from_u64(map, slot_map);
+ bitmap_onto(ts_mask, map, ts_mask_avail, 64);
+
+ if (bitmap_weight(ts_mask, 64) != bitmap_weight(map, 64)) {
+ dev_err(qmc_hdlc->dev, "Cannot translate timeslots %*pb -> (%*pb, %*pb)\n",
+ 64, map, 64, ts_mask_avail, 64, ts_mask);
+ return -EINVAL;
+ }
+
+ bitmap_to_arr64(&ts_info->tx_ts_mask, ts_mask, 64);
+ ts_info->rx_ts_mask = ts_info->tx_ts_mask;
+ return 0;
+}
+
+static int qmc_hdlc_xlate_ts_info(struct qmc_hdlc *qmc_hdlc,
+ const struct qmc_chan_ts_info *ts_info, u32 *slot_map)
+{
+ DECLARE_BITMAP(ts_mask_avail, 64);
+ DECLARE_BITMAP(ts_mask, 64);
+ DECLARE_BITMAP(map, 64);
+ u32 array32[2];
+
+ /* Tx and Rx masks and available masks must be identical */
+ if (ts_info->rx_ts_mask_avail != ts_info->tx_ts_mask_avail) {
+ dev_err(qmc_hdlc->dev, "tx and rx available timeslots mismatch (0x%llx, 0x%llx)\n",
+ ts_info->rx_ts_mask_avail, ts_info->tx_ts_mask_avail);
+ return -EINVAL;
+ }
+ if (ts_info->rx_ts_mask != ts_info->tx_ts_mask) {
+ dev_err(qmc_hdlc->dev, "tx and rx timeslots mismatch (0x%llx, 0x%llx)\n",
+ ts_info->rx_ts_mask, ts_info->tx_ts_mask);
+ return -EINVAL;
+ }
+
+ bitmap_from_u64(ts_mask_avail, ts_info->rx_ts_mask_avail);
+ bitmap_from_u64(ts_mask, ts_info->rx_ts_mask);
+ bitmap_off(map, ts_mask, ts_mask_avail, 64);
+
+ if (bitmap_weight(ts_mask, 64) != bitmap_weight(map, 64)) {
+ dev_err(qmc_hdlc->dev, "Cannot translate timeslots (%*pb, %*pb) -> %*pb\n",
+ 64, ts_mask_avail, 64, ts_mask, 64, map);
+ return -EINVAL;
+ }
+
+ bitmap_to_arr32(array32, map, 64);
+ if (array32[1]) {
+ dev_err(qmc_hdlc->dev, "Slot map out of 32bit (%*pb, %*pb) -> %*pb\n",
+ 64, ts_mask_avail, 64, ts_mask, 64, map);
+ return -EINVAL;
+ }
+
+ *slot_map = array32[0];
+ return 0;
+}
+
+static int qmc_hdlc_set_iface(struct qmc_hdlc *qmc_hdlc, int if_iface, const te1_settings *te1)
+{
+ struct qmc_chan_ts_info ts_info;
+ int ret;
+
+ ret = qmc_chan_get_ts_info(qmc_hdlc->qmc_chan, &ts_info);
+ if (ret) {
+ dev_err(qmc_hdlc->dev, "get QMC channel ts info failed %d\n", ret);
+ return ret;
+ }
+ ret = qmc_hdlc_xlate_slot_map(qmc_hdlc, te1->slot_map, &ts_info);
+ if (ret)
+ return ret;
+
+ ret = qmc_chan_set_ts_info(qmc_hdlc->qmc_chan, &ts_info);
+ if (ret) {
+ dev_err(qmc_hdlc->dev, "set QMC channel ts info failed %d\n", ret);
+ return ret;
+ }
+
+ qmc_hdlc->slot_map = te1->slot_map;
+
+ return 0;
+}
+
+static int qmc_hdlc_ioctl(struct net_device *netdev, struct if_settings *ifs)
+{
+ struct qmc_hdlc *qmc_hdlc = netdev_to_qmc_hdlc(netdev);
+ te1_settings te1;
+
+ switch (ifs->type) {
+ case IF_GET_IFACE:
+ ifs->type = IF_IFACE_E1;
+ if (ifs->size < sizeof(te1)) {
+ if (!ifs->size)
+ return 0; /* only type requested */
+
+ ifs->size = sizeof(te1); /* data size wanted */
+ return -ENOBUFS;
+ }
+
+ memset(&te1, 0, sizeof(te1));
+
+ /* Update slot_map */
+ te1.slot_map = qmc_hdlc->slot_map;
+
+ if (copy_to_user(ifs->ifs_ifsu.te1, &te1, sizeof(te1)))
+ return -EFAULT;
+ return 0;
+
+ case IF_IFACE_E1:
+ case IF_IFACE_T1:
+ if (!capable(CAP_NET_ADMIN))
+ return -EPERM;
+
+ if (netdev->flags & IFF_UP)
+ return -EBUSY;
+
+ if (copy_from_user(&te1, ifs->ifs_ifsu.te1, sizeof(te1)))
+ return -EFAULT;
+
+ return qmc_hdlc_set_iface(qmc_hdlc, ifs->type, &te1);
+
+ default:
+ return hdlc_ioctl(netdev, ifs);
+ }
+}
+
static int qmc_hdlc_open(struct net_device *netdev)
{
struct qmc_hdlc *qmc_hdlc = netdev_to_qmc_hdlc(netdev);
@@ -332,13 +472,14 @@ 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,
+ .ndo_siocwandev = qmc_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_ts_info ts_info;
struct qmc_chan_info info;
hdlc_device *hdlc;
int ret;
@@ -368,6 +509,15 @@ static int qmc_hdlc_probe(struct platform_device *pdev)
return -EINVAL;
}
+ ret = qmc_chan_get_ts_info(qmc_hdlc->qmc_chan, &ts_info);
+ if (ret) {
+ dev_err(qmc_hdlc->dev, "get QMC channel ts info failed %d\n", ret);
+ return ret;
+ }
+ ret = qmc_hdlc_xlate_ts_info(qmc_hdlc, &ts_info, &qmc_hdlc->slot_map);
+ if (ret)
+ return ret;
+
qmc_hdlc->netdev = alloc_hdlcdev(qmc_hdlc);
if (!qmc_hdlc->netdev) {
dev_err(qmc_hdlc->dev, "failed to alloc hdlc dev\n");
--
2.43.0
Add framer support in the fsl_qmc_hdlc driver in order to be able to
signal carrier changes to the network stack based on the framer status
Also use this framer to provide information related to the E1/T1 line
interface on IF_GET_IFACE and configure the line interface according to
IF_IFACE_{E1,T1} information.
Signed-off-by: Herve Codina <[email protected]>
Reviewed-by: Christophe Leroy <[email protected]>
---
drivers/net/wan/fsl_qmc_hdlc.c | 239 ++++++++++++++++++++++++++++++++-
1 file changed, 235 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wan/fsl_qmc_hdlc.c b/drivers/net/wan/fsl_qmc_hdlc.c
index b25d918d5e4e..432b5111b106 100644
--- a/drivers/net/wan/fsl_qmc_hdlc.c
+++ b/drivers/net/wan/fsl_qmc_hdlc.c
@@ -9,6 +9,7 @@
#include <linux/bitmap.h>
#include <linux/dma-mapping.h>
+#include <linux/framer/framer.h>
#include <linux/hdlc.h>
#include <linux/module.h>
#include <linux/of.h>
@@ -28,6 +29,9 @@ struct qmc_hdlc {
struct device *dev;
struct qmc_chan *qmc_chan;
struct net_device *netdev;
+ struct framer *framer;
+ spinlock_t carrier_lock; /* Protect carrier detection */
+ struct notifier_block nb;
bool is_crc32;
spinlock_t tx_lock; /* Protect tx descriptors */
struct qmc_hdlc_desc tx_descs[8];
@@ -41,6 +45,195 @@ static struct qmc_hdlc *netdev_to_qmc_hdlc(struct net_device *netdev)
return dev_to_hdlc(netdev)->priv;
}
+static int qmc_hdlc_framer_set_carrier(struct qmc_hdlc *qmc_hdlc)
+{
+ struct framer_status framer_status;
+ unsigned long flags;
+ int ret;
+
+ if (!qmc_hdlc->framer)
+ return 0;
+
+ spin_lock_irqsave(&qmc_hdlc->carrier_lock, flags);
+
+ ret = framer_get_status(qmc_hdlc->framer, &framer_status);
+ if (ret) {
+ dev_err(qmc_hdlc->dev, "get framer status failed (%d)\n", ret);
+ goto end;
+ }
+ if (framer_status.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_framer_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 != FRAMER_EVENT_STATUS)
+ return NOTIFY_DONE;
+
+ ret = qmc_hdlc_framer_set_carrier(qmc_hdlc);
+ return ret ? NOTIFY_DONE : NOTIFY_OK;
+}
+
+static int qmc_hdlc_framer_start(struct qmc_hdlc *qmc_hdlc)
+{
+ struct framer_status framer_status;
+ int ret;
+
+ if (!qmc_hdlc->framer)
+ return 0;
+
+ ret = framer_power_on(qmc_hdlc->framer);
+ if (ret) {
+ dev_err(qmc_hdlc->dev, "framer power-on failed (%d)\n", ret);
+ return ret;
+ }
+
+ /* Be sure that get_status is supported */
+ ret = framer_get_status(qmc_hdlc->framer, &framer_status);
+ if (ret) {
+ dev_err(qmc_hdlc->dev, "get framer status failed (%d)\n", ret);
+ goto framer_power_off;
+ }
+
+ qmc_hdlc->nb.notifier_call = qmc_hdlc_framer_notifier;
+ ret = framer_notifier_register(qmc_hdlc->framer, &qmc_hdlc->nb);
+ if (ret) {
+ dev_err(qmc_hdlc->dev, "framer notifier register failed (%d)\n", ret);
+ goto framer_power_off;
+ }
+
+ return 0;
+
+framer_power_off:
+ framer_power_off(qmc_hdlc->framer);
+ return ret;
+}
+
+static void qmc_hdlc_framer_stop(struct qmc_hdlc *qmc_hdlc)
+{
+ if (!qmc_hdlc->framer)
+ return;
+
+ framer_notifier_unregister(qmc_hdlc->framer, &qmc_hdlc->nb);
+ framer_power_off(qmc_hdlc->framer);
+}
+
+static int qmc_hdlc_framer_set_iface(struct qmc_hdlc *qmc_hdlc, int if_iface,
+ const te1_settings *te1)
+{
+ struct framer_config config;
+ int ret;
+
+ if (!qmc_hdlc->framer)
+ return 0;
+
+ ret = framer_get_config(qmc_hdlc->framer, &config);
+ if (ret)
+ return ret;
+
+ switch (if_iface) {
+ case IF_IFACE_E1:
+ config.iface = FRAMER_IFACE_E1;
+ break;
+ case IF_IFACE_T1:
+ config.iface = FRAMER_IFACE_T1;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ switch (te1->clock_type) {
+ case CLOCK_DEFAULT:
+ /* Keep current value */
+ break;
+ case CLOCK_EXT:
+ config.clock_type = FRAMER_CLOCK_EXT;
+ break;
+ case CLOCK_INT:
+ config.clock_type = FRAMER_CLOCK_INT;
+ break;
+ default:
+ return -EINVAL;
+ }
+ config.line_clock_rate = te1->clock_rate;
+
+ return framer_set_config(qmc_hdlc->framer, &config);
+}
+
+static int qmc_hdlc_framer_get_iface(struct qmc_hdlc *qmc_hdlc, int *if_iface, te1_settings *te1)
+{
+ struct framer_config config;
+ int ret;
+
+ if (!qmc_hdlc->framer) {
+ *if_iface = IF_IFACE_E1;
+ return 0;
+ }
+
+ ret = framer_get_config(qmc_hdlc->framer, &config);
+ if (ret)
+ return ret;
+
+ switch (config.iface) {
+ case FRAMER_IFACE_E1:
+ *if_iface = IF_IFACE_E1;
+ break;
+ case FRAMER_IFACE_T1:
+ *if_iface = IF_IFACE_T1;
+ break;
+ }
+
+ if (!te1)
+ return 0; /* Only iface type requested */
+
+ switch (config.clock_type) {
+ case FRAMER_CLOCK_EXT:
+ te1->clock_type = CLOCK_EXT;
+ break;
+ case FRAMER_CLOCK_INT:
+ te1->clock_type = CLOCK_INT;
+ break;
+ default:
+ return -EINVAL;
+ }
+ te1->clock_rate = config.line_clock_rate;
+ return 0;
+}
+
+static int qmc_hdlc_framer_init(struct qmc_hdlc *qmc_hdlc)
+{
+ int ret;
+
+ if (!qmc_hdlc->framer)
+ return 0;
+
+ ret = framer_init(qmc_hdlc->framer);
+ if (ret) {
+ dev_err(qmc_hdlc->dev, "framer init failed (%d)\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static void qmc_hdlc_framer_exit(struct qmc_hdlc *qmc_hdlc)
+{
+ if (!qmc_hdlc->framer)
+ return;
+
+ framer_exit(qmc_hdlc->framer);
+}
+
static int qmc_hdlc_recv_queue(struct qmc_hdlc *qmc_hdlc, struct qmc_hdlc_desc *desc, size_t size);
#define QMC_HDLC_RX_ERROR_FLAGS (QMC_RX_FLAG_HDLC_OVF | \
@@ -300,6 +493,12 @@ static int qmc_hdlc_set_iface(struct qmc_hdlc *qmc_hdlc, int if_iface, const te1
qmc_hdlc->slot_map = te1->slot_map;
+ ret = qmc_hdlc_framer_set_iface(qmc_hdlc, if_iface, te1);
+ if (ret) {
+ dev_err(qmc_hdlc->dev, "framer set iface failed %d\n", ret);
+ return ret;
+ }
+
return 0;
}
@@ -307,11 +506,16 @@ static int qmc_hdlc_ioctl(struct net_device *netdev, struct if_settings *ifs)
{
struct qmc_hdlc *qmc_hdlc = netdev_to_qmc_hdlc(netdev);
te1_settings te1;
+ int ret;
switch (ifs->type) {
case IF_GET_IFACE:
- ifs->type = IF_IFACE_E1;
if (ifs->size < sizeof(te1)) {
+ /* Retrieve type only */
+ ret = qmc_hdlc_framer_get_iface(qmc_hdlc, &ifs->type, NULL);
+ if (ret)
+ return ret;
+
if (!ifs->size)
return 0; /* only type requested */
@@ -321,6 +525,11 @@ static int qmc_hdlc_ioctl(struct net_device *netdev, struct if_settings *ifs)
memset(&te1, 0, sizeof(te1));
+ /* Retrieve info from framer */
+ ret = qmc_hdlc_framer_get_iface(qmc_hdlc, &ifs->type, &te1);
+ if (ret)
+ return ret;
+
/* Update slot_map */
te1.slot_map = qmc_hdlc->slot_map;
@@ -354,10 +563,17 @@ static int qmc_hdlc_open(struct net_device *netdev)
int ret;
int i;
- ret = hdlc_open(netdev);
+ ret = qmc_hdlc_framer_start(qmc_hdlc);
if (ret)
return ret;
+ ret = hdlc_open(netdev);
+ if (ret)
+ goto framer_stop;
+
+ /* Update carrier */
+ qmc_hdlc_framer_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
@@ -407,6 +623,8 @@ static int qmc_hdlc_open(struct net_device *netdev)
}
hdlc_close:
hdlc_close(netdev);
+framer_stop:
+ qmc_hdlc_framer_stop(qmc_hdlc);
return ret;
}
@@ -442,6 +660,7 @@ static int qmc_hdlc_close(struct net_device *netdev)
}
hdlc_close(netdev);
+ qmc_hdlc_framer_stop(qmc_hdlc);
return 0;
}
@@ -490,6 +709,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_bychild(qmc_hdlc->dev, np);
if (IS_ERR(qmc_hdlc->qmc_chan)) {
@@ -518,10 +738,19 @@ static int qmc_hdlc_probe(struct platform_device *pdev)
if (ret)
return ret;
+ qmc_hdlc->framer = devm_framer_optional_get(qmc_hdlc->dev, "fsl,framer");
+ if (IS_ERR(qmc_hdlc->framer))
+ return PTR_ERR(qmc_hdlc->framer);
+
+ ret = qmc_hdlc_framer_init(qmc_hdlc);
+ if (ret)
+ return ret;
+
qmc_hdlc->netdev = alloc_hdlcdev(qmc_hdlc);
if (!qmc_hdlc->netdev) {
dev_err(qmc_hdlc->dev, "failed to alloc hdlc dev\n");
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto framer_exit;
}
hdlc = dev_to_hdlc(qmc_hdlc->netdev);
@@ -537,11 +766,12 @@ static int qmc_hdlc_probe(struct platform_device *pdev)
}
platform_set_drvdata(pdev, qmc_hdlc);
-
return 0;
free_netdev:
free_netdev(qmc_hdlc->netdev);
+framer_exit:
+ qmc_hdlc_framer_exit(qmc_hdlc);
return ret;
}
@@ -551,6 +781,7 @@ static int qmc_hdlc_remove(struct platform_device *pdev)
unregister_hdlc_device(qmc_hdlc->netdev);
free_netdev(qmc_hdlc->netdev);
+ qmc_hdlc_framer_exit(qmc_hdlc);
return 0;
}
--
2.43.0
Hi all,
I duplicated patches in this series :(
My bad, I was mistaken in 'git format-patch'.
Can you consider only the "[PATCH v3 RESEND n/6]: xxxxxx" in this review.
The other patches ("RESEND PATCH v3") are the duplicated ones.
If ok I will send a clean v4 series.
Of course, if some modification are needed, I will also send a clean v4.
Let me know if a clean v4 is needed right now.
Sorry for this mistake.
Regards,
Hervé
On Mon, 12 Feb 2024 08:56:28 +0100
Herve Codina <[email protected]> wrote:
> Hi,
>
> Note: Resent this v3 series with missing maintainers added in CC.
>
> This series introduces the QMC HDLC support.
>
> Patches were previously sent as part of a full feature series and were
> previously reviewed in that context:
> "Add support for QMC HDLC, framer infrastructure and PEF2256 framer" [1]
>
> In order to ease the merge, the full feature series has been split and
> needed parts were merged in v6.8-rc1:
> - "Prepare the PowerQUICC QMC and TSA for the HDLC QMC driver" [2]
> - "Add support for framer infrastructure and PEF2256 framer" [3]
>
> This series contains patches related to the QMC HDLC part (QMC HDLC
> driver):
> - Introduce the QMC HDLC driver (patches 1 and 2)
> - Add timeslots change support in QMC HDLC (patch 3)
> - Add framer support as a framer consumer in QMC HDLC (patch 4)
>
> Compare to the original full feature series, a modification was done on
> patch 3 in order to use a coherent prefix in the commit title.
>
> I kept the patches unsquashed as they were previously sent and reviewed.
> Of course, I can squash them if needed.
>
> Compared to the previous iteration:
> https://lore.kernel.org/linux-kernel/[email protected]/
> this v3 series:
> - Remove 'inline' function specifier from .c file.
> - Fixed a bug introduced in the previous iteration.
> - Remove one lock/unlock sequence in the QMC HDCL xmit path.
> - Use bitmap_from_u64().
>
> Best regards,
> Hervé
>
> [1]: https://lore.kernel.org/linux-kernel/[email protected]/
> [2]: https://lore.kernel.org/linux-kernel/[email protected]/
> [3]: https://lore.kernel.org/linux-kernel/[email protected]/
>
> Changes v2 -> v3
> - Patch 1
> Remove 'inline' function specifier from .c file.
> Fix a bug introduced when added WARN_ONCE(). The warn condition must
> be desc->skb (descriptor used) instead of !desc->skb.
> Remove a lock/unlock section locking the entire qmc_hdlc_xmit()
> function.
>
> - Patch 5
> Use bitmap_from_u64() everywhere instead of bitmap_from_arr32() and
> bitmap_from_arr64().
>
> Changes v1 -> v2
> - Patch 1
> Use the same qmc_hdlc initialisation in qmc_hcld_recv_complete()
> than the one present in qmc_hcld_xmit_complete().
> Use WARN_ONCE()
>
> - Patch 3 (new patch in v2)
> Make bitmap_onto() available to users
>
> - Patch 4 (new patch in v2)
> Introduce bitmap_off()
>
> - Patch 5 (patch 3 in v1)
> Use bitmap_*() functions
>
> - Patch 6 (patch 4 in v1)
> No changes
>
> Changes compare to the full feature series:
> - Patch 3
> Use 'net: wan: fsl_qmc_hdlc:' as commit title prefix
>
> Patches extracted:
> - Patch 1 : full feature series patch 7
> - Patch 2 : full feature series patch 8
> - Patch 3 : full feature series patch 20
> - Patch 4 : full feature series patch 27
>
> Herve Codina (6):
> net: wan: Add support for QMC HDLC
> MAINTAINERS: Add the Freescale QMC HDLC driver entry
> bitmap: Make bitmap_onto() available to users
> bitmap: Introduce bitmap_off()
> net: wan: fsl_qmc_hdlc: Add runtime timeslots changes support
> net: wan: fsl_qmc_hdlc: Add framer support
>
> MAINTAINERS | 7 +
> drivers/net/wan/Kconfig | 12 +
> drivers/net/wan/Makefile | 1 +
> drivers/net/wan/fsl_qmc_hdlc.c | 807 +++++++++++++++++++++++++++++++++
> include/linux/bitmap.h | 3 +
> lib/bitmap.c | 45 +-
> 6 files changed, 874 insertions(+), 1 deletion(-)
> create mode 100644 drivers/net/wan/fsl_qmc_hdlc.c
>
--
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
QMC channels support runtime timeslots changes but nothing is done at
the QMC HDLC driver to handle these changes.
Use existing IFACE ioctl in order to configure the timeslots to use.
Signed-off-by: Herve Codina <[email protected]>
Reviewed-by: Christophe Leroy <[email protected]>
Acked-by: Jakub Kicinski <[email protected]>
---
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 835500910d1b..b25d918d5e4e 100644
--- a/drivers/net/wan/fsl_qmc_hdlc.c
+++ b/drivers/net/wan/fsl_qmc_hdlc.c
@@ -7,6 +7,7 @@
* Author: Herve Codina <[email protected]>
*/
+#include <linux/bitmap.h>
#include <linux/dma-mapping.h>
#include <linux/hdlc.h>
#include <linux/module.h>
@@ -32,6 +33,7 @@ struct qmc_hdlc {
struct qmc_hdlc_desc tx_descs[8];
unsigned int tx_out;
struct qmc_hdlc_desc rx_descs[4];
+ u32 slot_map;
};
static struct qmc_hdlc *netdev_to_qmc_hdlc(struct net_device *netdev)
@@ -206,6 +208,144 @@ static netdev_tx_t qmc_hdlc_xmit(struct sk_buff *skb, struct net_device *netdev)
return ret;
}
+static int qmc_hdlc_xlate_slot_map(struct qmc_hdlc *qmc_hdlc,
+ u32 slot_map, struct qmc_chan_ts_info *ts_info)
+{
+ DECLARE_BITMAP(ts_mask_avail, 64);
+ DECLARE_BITMAP(ts_mask, 64);
+ DECLARE_BITMAP(map, 64);
+
+ /* Tx and Rx available masks must be identical */
+ if (ts_info->rx_ts_mask_avail != ts_info->tx_ts_mask_avail) {
+ dev_err(qmc_hdlc->dev, "tx and rx available timeslots mismatch (0x%llx, 0x%llx)\n",
+ ts_info->rx_ts_mask_avail, ts_info->tx_ts_mask_avail);
+ return -EINVAL;
+ }
+
+ bitmap_from_u64(ts_mask_avail, ts_info->rx_ts_mask_avail);
+ bitmap_from_u64(map, slot_map);
+ bitmap_onto(ts_mask, map, ts_mask_avail, 64);
+
+ if (bitmap_weight(ts_mask, 64) != bitmap_weight(map, 64)) {
+ dev_err(qmc_hdlc->dev, "Cannot translate timeslots %*pb -> (%*pb, %*pb)\n",
+ 64, map, 64, ts_mask_avail, 64, ts_mask);
+ return -EINVAL;
+ }
+
+ bitmap_to_arr64(&ts_info->tx_ts_mask, ts_mask, 64);
+ ts_info->rx_ts_mask = ts_info->tx_ts_mask;
+ return 0;
+}
+
+static int qmc_hdlc_xlate_ts_info(struct qmc_hdlc *qmc_hdlc,
+ const struct qmc_chan_ts_info *ts_info, u32 *slot_map)
+{
+ DECLARE_BITMAP(ts_mask_avail, 64);
+ DECLARE_BITMAP(ts_mask, 64);
+ DECLARE_BITMAP(map, 64);
+ u32 array32[2];
+
+ /* Tx and Rx masks and available masks must be identical */
+ if (ts_info->rx_ts_mask_avail != ts_info->tx_ts_mask_avail) {
+ dev_err(qmc_hdlc->dev, "tx and rx available timeslots mismatch (0x%llx, 0x%llx)\n",
+ ts_info->rx_ts_mask_avail, ts_info->tx_ts_mask_avail);
+ return -EINVAL;
+ }
+ if (ts_info->rx_ts_mask != ts_info->tx_ts_mask) {
+ dev_err(qmc_hdlc->dev, "tx and rx timeslots mismatch (0x%llx, 0x%llx)\n",
+ ts_info->rx_ts_mask, ts_info->tx_ts_mask);
+ return -EINVAL;
+ }
+
+ bitmap_from_u64(ts_mask_avail, ts_info->rx_ts_mask_avail);
+ bitmap_from_u64(ts_mask, ts_info->rx_ts_mask);
+ bitmap_off(map, ts_mask, ts_mask_avail, 64);
+
+ if (bitmap_weight(ts_mask, 64) != bitmap_weight(map, 64)) {
+ dev_err(qmc_hdlc->dev, "Cannot translate timeslots (%*pb, %*pb) -> %*pb\n",
+ 64, ts_mask_avail, 64, ts_mask, 64, map);
+ return -EINVAL;
+ }
+
+ bitmap_to_arr32(array32, map, 64);
+ if (array32[1]) {
+ dev_err(qmc_hdlc->dev, "Slot map out of 32bit (%*pb, %*pb) -> %*pb\n",
+ 64, ts_mask_avail, 64, ts_mask, 64, map);
+ return -EINVAL;
+ }
+
+ *slot_map = array32[0];
+ return 0;
+}
+
+static int qmc_hdlc_set_iface(struct qmc_hdlc *qmc_hdlc, int if_iface, const te1_settings *te1)
+{
+ struct qmc_chan_ts_info ts_info;
+ int ret;
+
+ ret = qmc_chan_get_ts_info(qmc_hdlc->qmc_chan, &ts_info);
+ if (ret) {
+ dev_err(qmc_hdlc->dev, "get QMC channel ts info failed %d\n", ret);
+ return ret;
+ }
+ ret = qmc_hdlc_xlate_slot_map(qmc_hdlc, te1->slot_map, &ts_info);
+ if (ret)
+ return ret;
+
+ ret = qmc_chan_set_ts_info(qmc_hdlc->qmc_chan, &ts_info);
+ if (ret) {
+ dev_err(qmc_hdlc->dev, "set QMC channel ts info failed %d\n", ret);
+ return ret;
+ }
+
+ qmc_hdlc->slot_map = te1->slot_map;
+
+ return 0;
+}
+
+static int qmc_hdlc_ioctl(struct net_device *netdev, struct if_settings *ifs)
+{
+ struct qmc_hdlc *qmc_hdlc = netdev_to_qmc_hdlc(netdev);
+ te1_settings te1;
+
+ switch (ifs->type) {
+ case IF_GET_IFACE:
+ ifs->type = IF_IFACE_E1;
+ if (ifs->size < sizeof(te1)) {
+ if (!ifs->size)
+ return 0; /* only type requested */
+
+ ifs->size = sizeof(te1); /* data size wanted */
+ return -ENOBUFS;
+ }
+
+ memset(&te1, 0, sizeof(te1));
+
+ /* Update slot_map */
+ te1.slot_map = qmc_hdlc->slot_map;
+
+ if (copy_to_user(ifs->ifs_ifsu.te1, &te1, sizeof(te1)))
+ return -EFAULT;
+ return 0;
+
+ case IF_IFACE_E1:
+ case IF_IFACE_T1:
+ if (!capable(CAP_NET_ADMIN))
+ return -EPERM;
+
+ if (netdev->flags & IFF_UP)
+ return -EBUSY;
+
+ if (copy_from_user(&te1, ifs->ifs_ifsu.te1, sizeof(te1)))
+ return -EFAULT;
+
+ return qmc_hdlc_set_iface(qmc_hdlc, ifs->type, &te1);
+
+ default:
+ return hdlc_ioctl(netdev, ifs);
+ }
+}
+
static int qmc_hdlc_open(struct net_device *netdev)
{
struct qmc_hdlc *qmc_hdlc = netdev_to_qmc_hdlc(netdev);
@@ -332,13 +472,14 @@ 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,
+ .ndo_siocwandev = qmc_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_ts_info ts_info;
struct qmc_chan_info info;
hdlc_device *hdlc;
int ret;
@@ -368,6 +509,15 @@ static int qmc_hdlc_probe(struct platform_device *pdev)
return -EINVAL;
}
+ ret = qmc_chan_get_ts_info(qmc_hdlc->qmc_chan, &ts_info);
+ if (ret) {
+ dev_err(qmc_hdlc->dev, "get QMC channel ts info failed %d\n", ret);
+ return ret;
+ }
+ ret = qmc_hdlc_xlate_ts_info(qmc_hdlc, &ts_info, &qmc_hdlc->slot_map);
+ if (ret)
+ return ret;
+
qmc_hdlc->netdev = alloc_hdlcdev(qmc_hdlc);
if (!qmc_hdlc->netdev) {
dev_err(qmc_hdlc->dev, "failed to alloc hdlc dev\n");
--
2.43.0
Add framer support in the fsl_qmc_hdlc driver in order to be able to
signal carrier changes to the network stack based on the framer status
Also use this framer to provide information related to the E1/T1 line
interface on IF_GET_IFACE and configure the line interface according to
IF_IFACE_{E1,T1} information.
Signed-off-by: Herve Codina <[email protected]>
Reviewed-by: Christophe Leroy <[email protected]>
---
drivers/net/wan/fsl_qmc_hdlc.c | 239 ++++++++++++++++++++++++++++++++-
1 file changed, 235 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wan/fsl_qmc_hdlc.c b/drivers/net/wan/fsl_qmc_hdlc.c
index b25d918d5e4e..432b5111b106 100644
--- a/drivers/net/wan/fsl_qmc_hdlc.c
+++ b/drivers/net/wan/fsl_qmc_hdlc.c
@@ -9,6 +9,7 @@
#include <linux/bitmap.h>
#include <linux/dma-mapping.h>
+#include <linux/framer/framer.h>
#include <linux/hdlc.h>
#include <linux/module.h>
#include <linux/of.h>
@@ -28,6 +29,9 @@ struct qmc_hdlc {
struct device *dev;
struct qmc_chan *qmc_chan;
struct net_device *netdev;
+ struct framer *framer;
+ spinlock_t carrier_lock; /* Protect carrier detection */
+ struct notifier_block nb;
bool is_crc32;
spinlock_t tx_lock; /* Protect tx descriptors */
struct qmc_hdlc_desc tx_descs[8];
@@ -41,6 +45,195 @@ static struct qmc_hdlc *netdev_to_qmc_hdlc(struct net_device *netdev)
return dev_to_hdlc(netdev)->priv;
}
+static int qmc_hdlc_framer_set_carrier(struct qmc_hdlc *qmc_hdlc)
+{
+ struct framer_status framer_status;
+ unsigned long flags;
+ int ret;
+
+ if (!qmc_hdlc->framer)
+ return 0;
+
+ spin_lock_irqsave(&qmc_hdlc->carrier_lock, flags);
+
+ ret = framer_get_status(qmc_hdlc->framer, &framer_status);
+ if (ret) {
+ dev_err(qmc_hdlc->dev, "get framer status failed (%d)\n", ret);
+ goto end;
+ }
+ if (framer_status.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_framer_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 != FRAMER_EVENT_STATUS)
+ return NOTIFY_DONE;
+
+ ret = qmc_hdlc_framer_set_carrier(qmc_hdlc);
+ return ret ? NOTIFY_DONE : NOTIFY_OK;
+}
+
+static int qmc_hdlc_framer_start(struct qmc_hdlc *qmc_hdlc)
+{
+ struct framer_status framer_status;
+ int ret;
+
+ if (!qmc_hdlc->framer)
+ return 0;
+
+ ret = framer_power_on(qmc_hdlc->framer);
+ if (ret) {
+ dev_err(qmc_hdlc->dev, "framer power-on failed (%d)\n", ret);
+ return ret;
+ }
+
+ /* Be sure that get_status is supported */
+ ret = framer_get_status(qmc_hdlc->framer, &framer_status);
+ if (ret) {
+ dev_err(qmc_hdlc->dev, "get framer status failed (%d)\n", ret);
+ goto framer_power_off;
+ }
+
+ qmc_hdlc->nb.notifier_call = qmc_hdlc_framer_notifier;
+ ret = framer_notifier_register(qmc_hdlc->framer, &qmc_hdlc->nb);
+ if (ret) {
+ dev_err(qmc_hdlc->dev, "framer notifier register failed (%d)\n", ret);
+ goto framer_power_off;
+ }
+
+ return 0;
+
+framer_power_off:
+ framer_power_off(qmc_hdlc->framer);
+ return ret;
+}
+
+static void qmc_hdlc_framer_stop(struct qmc_hdlc *qmc_hdlc)
+{
+ if (!qmc_hdlc->framer)
+ return;
+
+ framer_notifier_unregister(qmc_hdlc->framer, &qmc_hdlc->nb);
+ framer_power_off(qmc_hdlc->framer);
+}
+
+static int qmc_hdlc_framer_set_iface(struct qmc_hdlc *qmc_hdlc, int if_iface,
+ const te1_settings *te1)
+{
+ struct framer_config config;
+ int ret;
+
+ if (!qmc_hdlc->framer)
+ return 0;
+
+ ret = framer_get_config(qmc_hdlc->framer, &config);
+ if (ret)
+ return ret;
+
+ switch (if_iface) {
+ case IF_IFACE_E1:
+ config.iface = FRAMER_IFACE_E1;
+ break;
+ case IF_IFACE_T1:
+ config.iface = FRAMER_IFACE_T1;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ switch (te1->clock_type) {
+ case CLOCK_DEFAULT:
+ /* Keep current value */
+ break;
+ case CLOCK_EXT:
+ config.clock_type = FRAMER_CLOCK_EXT;
+ break;
+ case CLOCK_INT:
+ config.clock_type = FRAMER_CLOCK_INT;
+ break;
+ default:
+ return -EINVAL;
+ }
+ config.line_clock_rate = te1->clock_rate;
+
+ return framer_set_config(qmc_hdlc->framer, &config);
+}
+
+static int qmc_hdlc_framer_get_iface(struct qmc_hdlc *qmc_hdlc, int *if_iface, te1_settings *te1)
+{
+ struct framer_config config;
+ int ret;
+
+ if (!qmc_hdlc->framer) {
+ *if_iface = IF_IFACE_E1;
+ return 0;
+ }
+
+ ret = framer_get_config(qmc_hdlc->framer, &config);
+ if (ret)
+ return ret;
+
+ switch (config.iface) {
+ case FRAMER_IFACE_E1:
+ *if_iface = IF_IFACE_E1;
+ break;
+ case FRAMER_IFACE_T1:
+ *if_iface = IF_IFACE_T1;
+ break;
+ }
+
+ if (!te1)
+ return 0; /* Only iface type requested */
+
+ switch (config.clock_type) {
+ case FRAMER_CLOCK_EXT:
+ te1->clock_type = CLOCK_EXT;
+ break;
+ case FRAMER_CLOCK_INT:
+ te1->clock_type = CLOCK_INT;
+ break;
+ default:
+ return -EINVAL;
+ }
+ te1->clock_rate = config.line_clock_rate;
+ return 0;
+}
+
+static int qmc_hdlc_framer_init(struct qmc_hdlc *qmc_hdlc)
+{
+ int ret;
+
+ if (!qmc_hdlc->framer)
+ return 0;
+
+ ret = framer_init(qmc_hdlc->framer);
+ if (ret) {
+ dev_err(qmc_hdlc->dev, "framer init failed (%d)\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static void qmc_hdlc_framer_exit(struct qmc_hdlc *qmc_hdlc)
+{
+ if (!qmc_hdlc->framer)
+ return;
+
+ framer_exit(qmc_hdlc->framer);
+}
+
static int qmc_hdlc_recv_queue(struct qmc_hdlc *qmc_hdlc, struct qmc_hdlc_desc *desc, size_t size);
#define QMC_HDLC_RX_ERROR_FLAGS (QMC_RX_FLAG_HDLC_OVF | \
@@ -300,6 +493,12 @@ static int qmc_hdlc_set_iface(struct qmc_hdlc *qmc_hdlc, int if_iface, const te1
qmc_hdlc->slot_map = te1->slot_map;
+ ret = qmc_hdlc_framer_set_iface(qmc_hdlc, if_iface, te1);
+ if (ret) {
+ dev_err(qmc_hdlc->dev, "framer set iface failed %d\n", ret);
+ return ret;
+ }
+
return 0;
}
@@ -307,11 +506,16 @@ static int qmc_hdlc_ioctl(struct net_device *netdev, struct if_settings *ifs)
{
struct qmc_hdlc *qmc_hdlc = netdev_to_qmc_hdlc(netdev);
te1_settings te1;
+ int ret;
switch (ifs->type) {
case IF_GET_IFACE:
- ifs->type = IF_IFACE_E1;
if (ifs->size < sizeof(te1)) {
+ /* Retrieve type only */
+ ret = qmc_hdlc_framer_get_iface(qmc_hdlc, &ifs->type, NULL);
+ if (ret)
+ return ret;
+
if (!ifs->size)
return 0; /* only type requested */
@@ -321,6 +525,11 @@ static int qmc_hdlc_ioctl(struct net_device *netdev, struct if_settings *ifs)
memset(&te1, 0, sizeof(te1));
+ /* Retrieve info from framer */
+ ret = qmc_hdlc_framer_get_iface(qmc_hdlc, &ifs->type, &te1);
+ if (ret)
+ return ret;
+
/* Update slot_map */
te1.slot_map = qmc_hdlc->slot_map;
@@ -354,10 +563,17 @@ static int qmc_hdlc_open(struct net_device *netdev)
int ret;
int i;
- ret = hdlc_open(netdev);
+ ret = qmc_hdlc_framer_start(qmc_hdlc);
if (ret)
return ret;
+ ret = hdlc_open(netdev);
+ if (ret)
+ goto framer_stop;
+
+ /* Update carrier */
+ qmc_hdlc_framer_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
@@ -407,6 +623,8 @@ static int qmc_hdlc_open(struct net_device *netdev)
}
hdlc_close:
hdlc_close(netdev);
+framer_stop:
+ qmc_hdlc_framer_stop(qmc_hdlc);
return ret;
}
@@ -442,6 +660,7 @@ static int qmc_hdlc_close(struct net_device *netdev)
}
hdlc_close(netdev);
+ qmc_hdlc_framer_stop(qmc_hdlc);
return 0;
}
@@ -490,6 +709,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_bychild(qmc_hdlc->dev, np);
if (IS_ERR(qmc_hdlc->qmc_chan)) {
@@ -518,10 +738,19 @@ static int qmc_hdlc_probe(struct platform_device *pdev)
if (ret)
return ret;
+ qmc_hdlc->framer = devm_framer_optional_get(qmc_hdlc->dev, "fsl,framer");
+ if (IS_ERR(qmc_hdlc->framer))
+ return PTR_ERR(qmc_hdlc->framer);
+
+ ret = qmc_hdlc_framer_init(qmc_hdlc);
+ if (ret)
+ return ret;
+
qmc_hdlc->netdev = alloc_hdlcdev(qmc_hdlc);
if (!qmc_hdlc->netdev) {
dev_err(qmc_hdlc->dev, "failed to alloc hdlc dev\n");
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto framer_exit;
}
hdlc = dev_to_hdlc(qmc_hdlc->netdev);
@@ -537,11 +766,12 @@ static int qmc_hdlc_probe(struct platform_device *pdev)
}
platform_set_drvdata(pdev, qmc_hdlc);
-
return 0;
free_netdev:
free_netdev(qmc_hdlc->netdev);
+framer_exit:
+ qmc_hdlc_framer_exit(qmc_hdlc);
return ret;
}
@@ -551,6 +781,7 @@ static int qmc_hdlc_remove(struct platform_device *pdev)
unregister_hdlc_device(qmc_hdlc->netdev);
free_netdev(qmc_hdlc->netdev);
+ qmc_hdlc_framer_exit(qmc_hdlc);
return 0;
}
--
2.43.0
On 12/02/2024 08.56, Herve Codina wrote:
> The bitmap_onto() function translates one bitmap relative to another but
> no function are present to perform the reverse translation.
>
> Introduce bitmap_off() to fill this hole.
>
> Signed-off-by: Herve Codina <[email protected]>
> ---
> include/linux/bitmap.h | 3 +++
> lib/bitmap.c | 42 ++++++++++++++++++++++++++++++++++++++++++
This patch, or the next in the series, should include a diffstat
mentioning lib/test_bitmap.c. And please make sure that the tests
exercise both expected use as well as corner cases, so that the actual
expected behavior is documented in code and not just in prose (which may
be ambiguous), and so that behavior-changing refactorings will not go
unnoticed.
Rasmus
On Mon, Feb 12, 2024 at 08:56:29AM +0100, Herve Codina wrote:
> The QMC HDLC driver provides support for HDLC using the QMC (QUICC
> Multichannel Controller) to transfer the HDLC data.
..
> +#include <linux/dma-mapping.h>
> +#include <linux/hdlc.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
I do not see how these are being used, am I right?
What's is missing OTOH is the mod_devicetable.h.
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
+ Blank line?
> +#include <soc/fsl/qe/qmc.h>
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On Mon, 12 Feb 2024 14:27:16 +0200
Andy Shevchenko <[email protected]> wrote:
> On Mon, Feb 12, 2024 at 08:56:31AM +0100, Herve Codina wrote:
> > Currently the bitmap_onto() is available only for CONFIG_NUMA=y case,
> > while some users may benefit out of it and being independent to NUMA
> > code.
> >
> > Make it available to users by moving out of ifdeffery and exporting for
> > modules.
>
> Wondering if you are trying to have something like
> https://lore.kernel.org/lkml/[email protected]/
>
Yes, it looks like.
Can you confirm that your bitmap_scatter() do the same operations as the
existing bitmap_onto() ?
If so, your bitmap_gather() will match my bitmap_off() (patch 4 in this series).
Thanks,
Hervé
--
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Mon, Feb 12, 2024 at 02:37:53PM +0100, Herve Codina wrote:
> On Mon, 12 Feb 2024 14:27:16 +0200
> Andy Shevchenko <[email protected]> wrote:
> > On Mon, Feb 12, 2024 at 08:56:31AM +0100, Herve Codina wrote:
> > > Currently the bitmap_onto() is available only for CONFIG_NUMA=y case,
> > > while some users may benefit out of it and being independent to NUMA
> > > code.
> > >
> > > Make it available to users by moving out of ifdeffery and exporting for
> > > modules.
> >
> > Wondering if you are trying to have something like
> > https://lore.kernel.org/lkml/[email protected]/
>
> Yes, it looks like.
> Can you confirm that your bitmap_scatter() do the same operations as the
> existing bitmap_onto() ?
I have test cases to be 100% sure, but on the first glance, yes it does with
the adjustment to the atomicity of the operations (which I do not understand
why be atomic in the original bitmap_onto() implementation).
This actually gives a question if we should use your approach or mine.
At least the help of bitmap_onto() is kinda hard to understand.
> If so, your bitmap_gather() will match my bitmap_off() (patch 4 in this
> series).
Yes.
--
With Best Regards,
Andy Shevchenko
On Mon, 12 Feb 2024 16:01:38 +0200
Andy Shevchenko <[email protected]> wrote:
> On Mon, Feb 12, 2024 at 02:37:53PM +0100, Herve Codina wrote:
> > On Mon, 12 Feb 2024 14:27:16 +0200
> > Andy Shevchenko <[email protected]> wrote:
> > > On Mon, Feb 12, 2024 at 08:56:31AM +0100, Herve Codina wrote:
> > > > Currently the bitmap_onto() is available only for CONFIG_NUMA=y case,
> > > > while some users may benefit out of it and being independent to NUMA
> > > > code.
> > > >
> > > > Make it available to users by moving out of ifdeffery and exporting for
> > > > modules.
> > >
> > > Wondering if you are trying to have something like
> > > https://lore.kernel.org/lkml/[email protected]/
> >
> > Yes, it looks like.
> > Can you confirm that your bitmap_scatter() do the same operations as the
> > existing bitmap_onto() ?
>
> I have test cases to be 100% sure, but on the first glance, yes it does with
> the adjustment to the atomicity of the operations (which I do not understand
> why be atomic in the original bitmap_onto() implementation).
>
> This actually gives a question if we should use your approach or mine.
> At least the help of bitmap_onto() is kinda hard to understand.
Agree, the bitmap_onto() code is simpler to understand than its help.
I introduced bitmap_off() to be the "reverse" bitmap_onto() operations
and I preferred to avoid duplicating function that do the same things.
On my side, I initially didn't use the bitmap_*() functions and did the the
bits manipulation by hand.
During the review, it was suggested to use the bitmap_*() family and I followed
this suggestion. I did tests to be sure that bitmap_onto() and bitmap_off() did
exactly the same things as my previous code did.
>
> > If so, your bitmap_gather() will match my bitmap_off() (patch 4 in this
> > series).
>
> Yes.
>
Regards,
Hervé
On Mon, Feb 12, 2024 at 03:20:22PM +0100, Herve Codina wrote:
> On Mon, 12 Feb 2024 16:01:38 +0200
> Andy Shevchenko <[email protected]> wrote:
..
> Agree, the bitmap_onto() code is simpler to understand than its help.
>
> I introduced bitmap_off() to be the "reverse" bitmap_onto() operations
> and I preferred to avoid duplicating function that do the same things.
>
> On my side, I initially didn't use the bitmap_*() functions and did the the
> bits manipulation by hand.
> During the review, it was suggested to use the bitmap_*() family and I followed
> this suggestion.
I also would go this way, the problems I see with the current implementation are:
- being related to NUMA (and as Rasmus once pointed out better to be there);
- unclear naming, esp. proposed bitmap_off();
- the quite hard to understand help text
- atomicity when it's not needed (AFAICT).
> I did tests to be sure that bitmap_onto() and bitmap_off() did
> exactly the same things as my previous code did.
Yuri, what do you think about all this?
--
With Best Regards,
Andy Shevchenko
On Mon, Feb 12, 2024 at 08:56:32AM +0100, Herve Codina wrote:
> The bitmap_onto() function translates one bitmap relative to another but
> no function are present to perform the reverse translation.
>
> Introduce bitmap_off() to fill this hole.
>
> Signed-off-by: Herve Codina <[email protected]>
> ---
> include/linux/bitmap.h | 3 +++
> lib/bitmap.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 45 insertions(+)
>
> diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> index 99451431e4d6..5ecfcbbc91f4 100644
> --- a/include/linux/bitmap.h
> +++ b/include/linux/bitmap.h
> @@ -65,6 +65,7 @@ struct device;
> * bitmap_remap(dst, src, old, new, nbits) *dst = map(old, new)(src)
> * bitmap_bitremap(oldbit, old, new, nbits) newbit = map(old, new)(oldbit)
> * bitmap_onto(dst, orig, relmap, nbits) *dst = orig relative to relmap
> + * bitmap_off(dst, orig, relmap, nbits) *dst = bitmap_onto() reverse operation
> * bitmap_fold(dst, orig, sz, nbits) dst bits = orig bits mod sz
> * bitmap_parse(buf, buflen, dst, nbits) Parse bitmap dst from kernel buf
> * bitmap_parse_user(ubuf, ulen, dst, nbits) Parse bitmap dst from user buf
> @@ -208,6 +209,8 @@ int bitmap_bitremap(int oldbit,
> const unsigned long *old, const unsigned long *new, int bits);
> void bitmap_onto(unsigned long *dst, const unsigned long *orig,
> const unsigned long *relmap, unsigned int bits);
> +void bitmap_off(unsigned long *dst, const unsigned long *orig,
> + const unsigned long *relmap, unsigned int bits);
> void bitmap_fold(unsigned long *dst, const unsigned long *orig,
> unsigned int sz, unsigned int nbits);
>
> diff --git a/lib/bitmap.c b/lib/bitmap.c
> index 2feccb5047dc..71343967335e 100644
> --- a/lib/bitmap.c
> +++ b/lib/bitmap.c
> @@ -682,6 +682,48 @@ void bitmap_onto(unsigned long *dst, const unsigned long *orig,
> }
> EXPORT_SYMBOL(bitmap_onto);
>
> +/**
> + * bitmap_off - revert operation done by bitmap_onto()
This is definitely a bad name. I've no a better idea, but even
bitmap_onto_revert() would be better.
> + * @dst: resulting translated bitmap
> + * @orig: original untranslated bitmap
> + * @relmap: bitmap relative to which translated
> + * @bits: number of bits in each of these bitmaps
> + *
> + * Suppose onto computed using bitmap_onto(onto, src, relmap, n)
> + * The operation bitmap_off(result, onto, relmap, n) leads to a
> + * result equal or equivalent to src.
Agree with Rasmus. This should be well tested.
> + * The result can be 'equivalent' because bitmap_onto() and
> + * bitmap_off() are not bijective.
> + * The result and src values are equivalent in that sense that a
> + * call to bitmap_onto(onto, src, relmap, n) and a call to
> + * bitmap_onto(onto, result, relmap, n) will lead to the same onto
> + * value.
Did you mean "a call to bitmap_onto(onto, src, relmap, n) and a
call to bitmap_off(onto, result, relmap, n)"?
I think the whole paragraph adds more confusion than explanations.
If a new function is supposed to revert the result of some other
function, I'd better focus on testing that it actually reverts as
advertised, and keep description as brief as possible.
> + * If either of @orig or @relmap is empty (no set bits), then @dst
> + * will be returned empty.
Is this an exception from the 'revert' policy? Doesn't look like that.
So, what for mentioning this specific case?
> + * All bits in @dst not set by the above rule are cleared.
The above rule is about empty @orig and @relmap, not about setting
bits. What did you mean here?
> + */
> +void bitmap_off(unsigned long *dst, const unsigned long *orig,
> + const unsigned long *relmap, unsigned int bits)
> +{
> + unsigned int n, m; /* same meaning as in above comment */
In the above comment, n means the size of bitmaps, and m is not
mentioned at all.
> + if (dst == orig) /* following doesn't handle inplace mappings */
> + return;
> + bitmap_zero(dst, bits);
Can you add an empty line after 'return'.
> + m = 0;
> + for_each_set_bit(n, relmap, bits) {
> + /* m == bitmap_pos_to_ord(relmap, n, bits) */
Don't think we need this comment here. If you want to underline that
m tracks bit order, can you just give it a more explanatory name. For
example, 'bit_order'.
> + if (test_bit(n, orig))
> + set_bit(m, dst);
> + m++;
> + }
> +}
> +EXPORT_SYMBOL(bitmap_off);
> +
> #ifdef CONFIG_NUMA
> /**
> * bitmap_fold - fold larger bitmap into smaller, modulo specified size
> --
> 2.43.0
On Mon, Feb 12, 2024 at 10:37:18AM -0800, Yury Norov wrote:
> On Mon, Feb 12, 2024 at 08:56:32AM +0100, Herve Codina wrote:
> > The bitmap_onto() function translates one bitmap relative to another but
> > no function are present to perform the reverse translation.
> >
> > Introduce bitmap_off() to fill this hole.
> >
> > Signed-off-by: Herve Codina <[email protected]>
> > ---
> > include/linux/bitmap.h | 3 +++
> > lib/bitmap.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 45 insertions(+)
> >
> > diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> > index 99451431e4d6..5ecfcbbc91f4 100644
> > --- a/include/linux/bitmap.h
> > +++ b/include/linux/bitmap.h
> > @@ -65,6 +65,7 @@ struct device;
> > * bitmap_remap(dst, src, old, new, nbits) *dst = map(old, new)(src)
> > * bitmap_bitremap(oldbit, old, new, nbits) newbit = map(old, new)(oldbit)
> > * bitmap_onto(dst, orig, relmap, nbits) *dst = orig relative to relmap
> > + * bitmap_off(dst, orig, relmap, nbits) *dst = bitmap_onto() reverse operation
> > * bitmap_fold(dst, orig, sz, nbits) dst bits = orig bits mod sz
> > * bitmap_parse(buf, buflen, dst, nbits) Parse bitmap dst from kernel buf
> > * bitmap_parse_user(ubuf, ulen, dst, nbits) Parse bitmap dst from user buf
> > @@ -208,6 +209,8 @@ int bitmap_bitremap(int oldbit,
> > const unsigned long *old, const unsigned long *new, int bits);
> > void bitmap_onto(unsigned long *dst, const unsigned long *orig,
> > const unsigned long *relmap, unsigned int bits);
> > +void bitmap_off(unsigned long *dst, const unsigned long *orig,
> > + const unsigned long *relmap, unsigned int bits);
> > void bitmap_fold(unsigned long *dst, const unsigned long *orig,
> > unsigned int sz, unsigned int nbits);
> >
> > diff --git a/lib/bitmap.c b/lib/bitmap.c
> > index 2feccb5047dc..71343967335e 100644
> > --- a/lib/bitmap.c
> > +++ b/lib/bitmap.c
> > @@ -682,6 +682,48 @@ void bitmap_onto(unsigned long *dst, const unsigned long *orig,
> > }
> > EXPORT_SYMBOL(bitmap_onto);
> >
> > +/**
> > + * bitmap_off - revert operation done by bitmap_onto()
>
> This is definitely a bad name. I've no a better idea, but even
> bitmap_onto_revert() would be better.
>
> > + * @dst: resulting translated bitmap
> > + * @orig: original untranslated bitmap
> > + * @relmap: bitmap relative to which translated
> > + * @bits: number of bits in each of these bitmaps
> > + *
> > + * Suppose onto computed using bitmap_onto(onto, src, relmap, n)
> > + * The operation bitmap_off(result, onto, relmap, n) leads to a
> > + * result equal or equivalent to src.
>
> Agree with Rasmus. This should be well tested.
>
> > + * The result can be 'equivalent' because bitmap_onto() and
> > + * bitmap_off() are not bijective.
> > + * The result and src values are equivalent in that sense that a
> > + * call to bitmap_onto(onto, src, relmap, n) and a call to
> > + * bitmap_onto(onto, result, relmap, n) will lead to the same onto
> > + * value.
>
> Did you mean "a call to bitmap_onto(onto, src, relmap, n) and a
> call to bitmap_off(onto, result, relmap, n)"?
>
> I think the whole paragraph adds more confusion than explanations.
> If a new function is supposed to revert the result of some other
> function, I'd better focus on testing that it actually reverts as
> advertised, and keep description as brief as possible.
>
> > + * If either of @orig or @relmap is empty (no set bits), then @dst
> > + * will be returned empty.
>
> Is this an exception from the 'revert' policy? Doesn't look like that.
> So, what for mentioning this specific case?
>
> > + * All bits in @dst not set by the above rule are cleared.
>
> The above rule is about empty @orig and @relmap, not about setting
> bits. What did you mean here?
>
> > + */
> > +void bitmap_off(unsigned long *dst, const unsigned long *orig,
> > + const unsigned long *relmap, unsigned int bits)
> > +{
> > + unsigned int n, m; /* same meaning as in above comment */
>
> In the above comment, n means the size of bitmaps, and m is not
> mentioned at all.
>
> > + if (dst == orig) /* following doesn't handle inplace mappings */
> > + return;
> > + bitmap_zero(dst, bits);
>
> Can you add an empty line after 'return'.
>
> > + m = 0;
> > + for_each_set_bit(n, relmap, bits) {
> > + /* m == bitmap_pos_to_ord(relmap, n, bits) */
>
> Don't think we need this comment here. If you want to underline that
> m tracks bit order, can you just give it a more explanatory name. For
> example, 'bit_order'.
>
> > + if (test_bit(n, orig))
> > + set_bit(m, dst);
> > + m++;
Forgot to mention - we need a __set_bit() and __test_bit(), because the
whole function is not atomic. This applies to the bitmap_onto() as
well. Can you please send a patch fixing it for bitmap_onto() in the
next iteration?
> > + }
> > +}
> > +EXPORT_SYMBOL(bitmap_off);
> > +
> > #ifdef CONFIG_NUMA
> > /**
> > * bitmap_fold - fold larger bitmap into smaller, modulo specified size
> > --
> > 2.43.0
On Mon, Feb 12, 2024 at 08:56:31AM +0100, Herve Codina wrote:
> Currently the bitmap_onto() is available only for CONFIG_NUMA=y case,
> while some users may benefit out of it and being independent to NUMA
> code.
>
> Make it available to users by moving out of ifdeffery and exporting for
> modules.
Wondering if you are trying to have something like
https://lore.kernel.org/lkml/[email protected]/
--
With Best Regards,
Andy Shevchenko
On Mon, Feb 12, 2024 at 04:36:36PM +0200, Andy Shevchenko wrote:
> On Mon, Feb 12, 2024 at 03:20:22PM +0100, Herve Codina wrote:
> > On Mon, 12 Feb 2024 16:01:38 +0200
> > Andy Shevchenko <[email protected]> wrote:
>
> ...
>
> > Agree, the bitmap_onto() code is simpler to understand than its help.
> >
> > I introduced bitmap_off() to be the "reverse" bitmap_onto() operations
> > and I preferred to avoid duplicating function that do the same things.
> >
> > On my side, I initially didn't use the bitmap_*() functions and did the the
> > bits manipulation by hand.
> > During the review, it was suggested to use the bitmap_*() family and I followed
> > this suggestion.
>
> I also would go this way, the problems I see with the current implementation are:
Sure, opencoding and duplicating the functionality is always a bad
idea.
> - being related to NUMA (and as Rasmus once pointed out better to be there);
It's 'related to NUMA' for the only reason - it's used by NUMA only.
Nothing NUMA-specific in the function itself.
Now that we've got a non-NUMA user, the bitmap_onto() is not related
to NUMA anymore.
> - unclear naming, esp. proposed bitmap_off();
That's I agree. Scatter/gather from your last approach sound better.
Do you plan to send a v2?
> - the quite hard to understand help text
Yes, we need a picture that would illustrate what actually happens
> - atomicity when it's not needed (AFAICT).
Agree. A series of atomic ops is not atomic. For example
if (test_bit(n, map))
set_bit(m, map);
is not atomic as a whole. And this is what we do in bitmap_onto/off()
in a loop. This must be fixed by using underscoded version.
> > I did tests to be sure that bitmap_onto() and bitmap_off() did
> > exactly the same things as my previous code did.
>
> Yuri, what do you think about all this?
I think your scatter/gather is better then this onto/off by naming and
implementation. If you'll send a v2, and it would work for Herve, I'd
prefer scatter/gather. But we can live with onto/off as well.
Thanks,
Yury
On Thu, Feb 15, 2024 at 06:46:12PM +0100, Herve Codina wrote:
> On Mon, 12 Feb 2024 11:13:13 -0800
> Yury Norov <[email protected]> wrote:
..
> > That's I agree. Scatter/gather from your last approach sound better.
> > Do you plan to send a v2?
See below.
..
> > I think your scatter/gather is better then this onto/off by naming and
> > implementation. If you'll send a v2, and it would work for Herve, I'd
> > prefer scatter/gather. But we can live with onto/off as well.
>
> Andy, I tested your bitmap_{scatter,gather}() in my code.
> I simply replaced my bitmap_{onto,off}() calls by calls to your helpers and
> it works perfectly for my use case.
>
> I didn't use your whole patch
> "[PATCH v1 2/5] lib/bitmap: Introduce bitmap_scatter() and bitmap_gather() helpers"
> because it didn't apply on a v6.8-rc1 based branch.
> I just manually extracted the needed functions for my tests and I didn't look
> at the lib/test_bitmap.c part.
>
> Now what's the plan ?
> Andy, do you want to send a v2 of this patch or may I get the patch, modify it
> according to reviews already present in v1 and integrate it in my current
> series ?
I would like to do that, but under pile of different things.
I would try my best but if you have enough time and motivation feel free
to take over, address the comments and integrate in your series.
I dunno what to do with bitmap_onto(), perhaps in a separate patch we can
replace it with bitmap_scatter() (IIUC) with explanation that the former
1) uses atomic ops while being non-atomic as a whole, and b) having quite
hard to get documentation. At least that's how I see it, I mean that I would
like to leave bitmap_onto() alone and address it separately.
> Yury, any preferences ?
--
With Best Regards,
Andy Shevchenko
Hi Andy, Yury,
On Thu, 15 Feb 2024 21:17:23 +0200
Andy Shevchenko <[email protected]> wrote:
[...]
> > Now what's the plan ?
> > Andy, do you want to send a v2 of this patch or may I get the patch, modify it
> > according to reviews already present in v1 and integrate it in my current
> > series ?
>
> I would like to do that, but under pile of different things.
> I would try my best but if you have enough time and motivation feel free
> to take over, address the comments and integrate in your series.
>
> I dunno what to do with bitmap_onto(), perhaps in a separate patch we can
> replace it with bitmap_scatter() (IIUC) with explanation that the former
> 1) uses atomic ops while being non-atomic as a whole, and b) having quite
> hard to get documentation. At least that's how I see it, I mean that I would
> like to leave bitmap_onto() alone and address it separately.
>
I will take the Andy's bitmap_{scatter,gather}() patch in my next iteration.
And use bitmap_{scatter,gather}() in my code.
For bitmap_onto() replacement, nothing will be done in my next iteration as
it is out of this series scope.
Hervé
On Wed, Feb 21, 2024 at 02:44:31PM +0100, Herve Codina wrote:
> On Thu, 15 Feb 2024 21:17:23 +0200
> Andy Shevchenko <[email protected]> wrote:
[...]
> > > Now what's the plan ?
> > > Andy, do you want to send a v2 of this patch or may I get the patch, modify it
> > > according to reviews already present in v1 and integrate it in my current
> > > series ?
> >
> > I would like to do that, but under pile of different things.
> > I would try my best but if you have enough time and motivation feel free
> > to take over, address the comments and integrate in your series.
> >
> > I dunno what to do with bitmap_onto(), perhaps in a separate patch we can
> > replace it with bitmap_scatter() (IIUC) with explanation that the former
> > 1) uses atomic ops while being non-atomic as a whole, and b) having quite
> > hard to get documentation. At least that's how I see it, I mean that I would
> > like to leave bitmap_onto() alone and address it separately.
>
> I will take the Andy's bitmap_{scatter,gather}() patch in my next iteration.
> And use bitmap_{scatter,gather}() in my code.
Thank you and sorry that I have no time to finish that. I will be happy to help
reviewing if you Cc me.
> For bitmap_onto() replacement, nothing will be done in my next iteration as
> it is out of this series scope.
I agree on this. This will be a separate logical change related to NUMA with
explanation and replacement of all callers at once.
--
With Best Regards,
Andy Shevchenko
On Thu, Feb 22, 2024 at 01:05:16PM +0100, Herve Codina wrote:
> On Mon, 12 Feb 2024 14:22:56 +0200
> Andy Shevchenko <[email protected]> wrote:
..
> > > +#include <linux/dma-mapping.h>
> > > +#include <linux/hdlc.h>
> > > +#include <linux/module.h>
> >
> > > +#include <linux/of.h>
> > > +#include <linux/of_platform.h>
> >
> > I do not see how these are being used, am I right?
> > What's is missing OTOH is the mod_devicetable.h.
>
> Agree for removing of.h and of_platform.h.
>
> Why do I need mod_devicetable.h ?
> Isn't including module.h enough ?
In that header the definitions of many of ID table data structures are located.
You are using that in the code.
--
With Best Regards,
Andy Shevchenko
On Thu, 22 Feb 2024 15:19:11 +0200
Andy Shevchenko <[email protected]> wrote:
> On Thu, Feb 22, 2024 at 01:05:16PM +0100, Herve Codina wrote:
> > On Mon, 12 Feb 2024 14:22:56 +0200
> > Andy Shevchenko <[email protected]> wrote:
>
> ...
>
> > > > +#include <linux/dma-mapping.h>
> > > > +#include <linux/hdlc.h>
> > > > +#include <linux/module.h>
> > >
> > > > +#include <linux/of.h>
> > > > +#include <linux/of_platform.h>
> > >
> > > I do not see how these are being used, am I right?
> > > What's is missing OTOH is the mod_devicetable.h.
> >
> > Agree for removing of.h and of_platform.h.
> >
> > Why do I need mod_devicetable.h ?
> > Isn't including module.h enough ?
>
> In that header the definitions of many of ID table data structures are located.
> You are using that in the code.
>
Ok, thanks.
Hervé