[PATCH 0/2] w1: Add 1-wire master driver for AMD programmable logic IP Core

[PATCH 0/2] w1: Add 1-wire master driver for AMD programmable logic IP Core

[Kris Chaplin]

Add a master driver to support the AMD 1-Wire programmable logic IP block.
This block guarantees protocol timing for driving off-board devices such as thermal sensors, proms, etc.

Kris Chaplin (2):
  dt-bindings: w1: Add YAML DT Schema for AMD w1 master and MAINTAINERS
    entry
  w1: Add 1-wire master driver for AMD programmable logic IP Core

 .../bindings/w1/amd,axi-1wire-master.yaml     |  44 ++
 MAINTAINERS                                   |   8 +
 drivers/w1/masters/Kconfig                    |  11 +
 drivers/w1/masters/Makefile                   |   1 +
 drivers/w1/masters/amd_w1.c                   | 422 ++++++++++++++++++
 5 files changed, 486 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml
 create mode 100644 drivers/w1/masters/amd_w1.c

-- 
2.42.GIT

[PATCH 1/2] dt-bindings: w1: Add YAML DT Schema for AMD w1 master and MAINTAINERS entry

[Kris Chaplin]

Add YAML DT Schema for the AMD w1 master IP.

This hardware guarantees protocol timing for driving off-board devices such
as thermal sensors, proms, etc using the 1wire protocol.

Add MAINTAINERS entry for DT Schema.

Co-developed-by: Thomas Delev <thomas.delev@amd.com>
Signed-off-by: Thomas Delev <thomas.delev@amd.com>
Signed-off-by: Kris Chaplin <kris.chaplin@amd.com>
---
 .../bindings/w1/amd,axi-1wire-master.yaml     | 44 +++++++++++++++++++
 MAINTAINERS                                   |  7 +++
 2 files changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml

diff --git a/Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml b/Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml
new file mode 100644
index 000000000000..41f7294a84a3
--- /dev/null
+++ b/Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml
@@ -0,0 +1,44 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/w1/amd,axi-1wire-master.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AMD AXI 1-wire bus master for Programmable Logic
+
+maintainers:
+  - Kris Chaplin <kris.chaplin@amd.com>
+
+properties:
+  compatible:
+    const: amd,axi-1wire-master
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    onewire@a0000000 {
+        compatible = "amd,axi-1wire-master";
+        reg = <0xa0000000 0x10000>;
+        clocks = <&zynqmp_clk 0x47>;
+        interrupts = <GIC_SPI 0x59 IRQ_TYPE_LEVEL_HIGH>;
+    };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 4b2c378b4fd9..6ec3922b256e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1066,6 +1066,13 @@ M:	Sanjay R Mehta <sanju.mehta@amd.com>
 S:	Maintained
 F:	drivers/spi/spi-amd.c
 
+AMD W1 DRIVER
+M:	Kris Chaplin <kris.chaplin@amd.com>
+R:	Thomas Delev <thomas.delev@amd.com>
+R:	Michal Simek <michal.simek@amd.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml
+
 AMD XGBE DRIVER
 M:	"Shyam Sundar S K" <Shyam-sundar.S-k@amd.com>
 L:	netdev@vger.kernel.org
-- 
2.42.GIT

[PATCH 2/2] w1: Add 1-wire master driver for AMD programmable logic IP Core

[Kris Chaplin]

Add a master driver to support the AMD 1-Wire programmable logic IP block.
This block guarantees protocol timing for driving off-board devices such
as thermal sensors, proms, etc.

Add file to MAINTAINERS

Co-developed-by: Thomas Delev <thomas.delev@amd.com>
Signed-off-by: Thomas Delev <thomas.delev@amd.com>
Signed-off-by: Kris Chaplin <kris.chaplin@amd.com>
---
 MAINTAINERS                 |   1 +
 drivers/w1/masters/Kconfig  |  11 +
 drivers/w1/masters/Makefile |   1 +
 drivers/w1/masters/amd_w1.c | 422 ++++++++++++++++++++++++++++++++++++
 4 files changed, 435 insertions(+)
 create mode 100644 drivers/w1/masters/amd_w1.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 6ec3922b256e..7f26dab5261b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1072,6 +1072,7 @@ R:	Thomas Delev <thomas.delev@amd.com>
 R:	Michal Simek <michal.simek@amd.com>
 S:	Maintained
 F:	Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml
+F:	drivers/w1/masters/amd_w1.c
 
 AMD XGBE DRIVER
 M:	"Shyam Sundar S K" <Shyam-sundar.S-k@amd.com>
diff --git a/drivers/w1/masters/Kconfig b/drivers/w1/masters/Kconfig
index ad316573288a..9232fd1f4e5b 100644
--- a/drivers/w1/masters/Kconfig
+++ b/drivers/w1/masters/Kconfig
@@ -67,5 +67,16 @@ config W1_MASTER_SGI
 	  This support is also available as a module.  If so, the module
 	  will be called sgi_w1.
 
+config W1_MASTER_AMD
+	tristate "AMD AXI 1-wire bus master"
+	help
+	  Say Y here is you want to support the AMD AXI 1-wire IP core.
+	  This driver makes use of the programmable logic IP to perform
+	  correctly timed 1 wire transactions without relying on GPIO timing
+	  through the kernel.
+
+	  This driver can also be built as a module.  If so, the module will be
+	  called amd_w1.
+
 endmenu
 
diff --git a/drivers/w1/masters/Makefile b/drivers/w1/masters/Makefile
index c5d85a827e52..cd3da1daaf97 100644
--- a/drivers/w1/masters/Makefile
+++ b/drivers/w1/masters/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_W1_MASTER_MXC)		+= mxc_w1.o
 obj-$(CONFIG_W1_MASTER_GPIO)		+= w1-gpio.o
 obj-$(CONFIG_HDQ_MASTER_OMAP)		+= omap_hdq.o
 obj-$(CONFIG_W1_MASTER_SGI)		+= sgi_w1.o
+obj-$(CONFIG_W1_MASTER_AMD)		+= amd_w1.o
diff --git a/drivers/w1/masters/amd_w1.c b/drivers/w1/masters/amd_w1.c
new file mode 100644
index 000000000000..04bf08c1b6d7
--- /dev/null
+++ b/drivers/w1/masters/amd_w1.c
@@ -0,0 +1,422 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * amd_w1 - AMD 1Wire bus master driver
+ *
+ * Copyright (C) 2022-2023 Advanced Micro Devices, Inc. All Rights Reserved.
+ */
+
+#include <linux/atomic.h>
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/jiffies.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/types.h>
+#include <linux/wait.h>
+
+#include <linux/w1.h>
+
+/* 1-wire AMD IP definition */
+#define AXIW1_IPID	0x10ee4453
+/* Registers offset */
+#define AXIW1_INST_REG	0x0
+#define AXIW1_CTRL_REG	0x4
+#define AXIW1_IRQE_REG	0x8
+#define AXIW1_STAT_REG	0xC
+#define AXIW1_DATA_REG	0x10
+#define AXIW1_IPVER_REG	0x18
+#define AXIW1_IPID_REG	0x1C
+/* Instructions */
+#define AXIW1_INITPRES	0x0800
+#define AXIW1_READBIT	0x0C00
+#define AXIW1_WRITEBIT	0x0E00
+#define AXIW1_READBYTE	0x0D00
+#define AXIW1_WRITEBYTE	0x0F00
+/* Status flag masks */
+#define AXIW1_DONE	BIT(0)
+#define AXIW1_READY	BIT(4)
+#define AXIW1_PRESENCE	BIT(31)
+#define AXIW1_MAJORVER_MASK	GENMASK(23, 8)
+#define AXIW1_MINORVER_MASK	GENMASK(7, 0)
+/* Control flag */
+#define AXIW1_GO	BIT(0)
+#define AXI_CLEAR	0
+#define AXI_RESET	BIT(31)
+#define AXIW1_READDATA	BIT(0)
+/* Interrupt Enable */
+#define AXIW1_READY_IRQ_EN	BIT(4)
+#define AXIW1_DONE_IRQ_EN	BIT(0)
+
+#define AXIW1_TIMEOUT	msecs_to_jiffies(100)
+
+#define DRIVER_NAME	"amd_w1"
+
+struct amd_w1_local {
+	struct device *dev;
+	void __iomem *base_addr;
+	int irq;
+	atomic_t flag;
+	struct clk *clk;
+	wait_queue_head_t wait_queue;
+};
+
+/**
+ * amd_w1_write_register() - Helper to write a hardware register.
+ *
+ * @amd_w1_local:	Pointer to device structure
+ * @reg_offset:		Register offset in bytes to write
+ * @val:		Value to write
+ */
+static inline void amd_w1_write_register(struct amd_w1_local *amd_w1_local,
+					 u8 reg_offset, u32 val)
+{
+	iowrite32(val, amd_w1_local->base_addr + reg_offset);
+};
+
+/**
+ * amd_w1_read_register() - Helper to write a hardware register.
+ *
+ * @amd_w1_local:	Pointer to device structure
+ * @reg_offset:		Register offset in bytes to write
+ *
+ * Return:		Value of register read
+ */
+static inline u32 amd_w1_read_register(struct amd_w1_local *amd_w1_local, u8 reg_offset)
+{
+	return ioread32(amd_w1_local->base_addr + reg_offset);
+};
+
+/**
+ * amd_w1_wait_irq_interruptible_timeout() - Wait for IRQ with timeout.
+ *
+ * @amd_w1_local:	Pointer to device structure
+ * @IRQ:		IRQ channel to wait on
+ *
+ * Return:		%0 - OK, %-EINTR - Interrupted, %-EBUSY - Timed out
+ */
+static inline int amd_w1_wait_irq_interruptible_timeout(struct amd_w1_local *amd_w1_local, u32 IRQ)
+{
+	int ret;
+
+	/* Enable the IRQ requested and wait for flag to indicate it's been triggered */
+	amd_w1_write_register(amd_w1_local, AXIW1_IRQE_REG, IRQ);
+	ret = wait_event_interruptible_timeout(amd_w1_local->wait_queue,
+					       atomic_read(&amd_w1_local->flag) != 0,
+					       AXIW1_TIMEOUT);
+	if (ret < 0) {
+		dev_err(amd_w1_local->dev, "Wait IRQ Interrupted\n");
+		return -EINTR;
+	}
+
+	if (!ret) {
+		dev_err(amd_w1_local->dev, "Wait IRQ Timeout\n");
+		return -EBUSY;
+	}
+
+	/* Clear flag */
+	atomic_set(&amd_w1_local->flag, 0);
+	return 0;
+}
+
+/**
+ * amd_w1_touch_bit() - Performs the touch-bit function, which writes a 0 or 1 and reads the level.
+ *
+ * @data:	Pointer to device structure
+ * @bit:	The level to write
+ *
+ * Return:	The level read
+ */
+static u8 amd_w1_touch_bit(void *data, u8 bit)
+{
+	struct amd_w1_local *amd_w1_local = data;
+	u8 val = 0;
+	int rc;
+
+	/* Wait for READY signal to be 1 to ensure 1-wire IP is ready */
+	while ((amd_w1_read_register(amd_w1_local, AXIW1_STAT_REG) & AXIW1_READY) == 0) {
+		rc = amd_w1_wait_irq_interruptible_timeout(amd_w1_local, AXIW1_READY_IRQ_EN);
+		if (rc < 0)
+			return 1; /* Callee doesn't test for error. Return inactive bus state */
+	}
+
+	if (bit)
+		/* Read. Write read Bit command in register 0 */
+		amd_w1_write_register(amd_w1_local, AXIW1_INST_REG, AXIW1_READBIT);
+	else
+		/* Write. Write tx Bit command in instruction register with bit to transmit */
+		amd_w1_write_register(amd_w1_local, AXIW1_INST_REG,
+				      (AXIW1_WRITEBIT + (bit & 0x01)));
+
+	/* Write Go signal and clear control reset signal in control register */
+	amd_w1_write_register(amd_w1_local, AXIW1_CTRL_REG, AXIW1_GO);
+
+	/* Wait for done signal to be 1 */
+	while ((amd_w1_read_register(amd_w1_local, AXIW1_STAT_REG) & AXIW1_DONE) != 1) {
+		rc = amd_w1_wait_irq_interruptible_timeout(amd_w1_local, AXIW1_DONE_IRQ_EN);
+		if (rc < 0)
+			return 1; /* Callee doesn't test for error. Return inactive bus state */
+	}
+
+	/* If read, Retrieve data from register */
+	if (bit)
+		val = (u8)(amd_w1_read_register(amd_w1_local, AXIW1_DATA_REG) & AXIW1_READDATA);
+
+	/* Clear Go signal in register 1 */
+	amd_w1_write_register(amd_w1_local, AXIW1_CTRL_REG, AXI_CLEAR);
+
+	return val;
+}
+
+/**
+ * amd_w1_read_byte - Performs the read byte function.
+ *
+ * @data:	Pointer to device structure
+ * Return:	The value read
+ */
+static u8 amd_w1_read_byte(void *data)
+{
+	struct amd_w1_local *amd_w1_local = data;
+	u8 val = 0;
+	int rc;
+
+	/* Wait for READY signal to be 1 to ensure 1-wire IP is ready */
+	while ((amd_w1_read_register(amd_w1_local, AXIW1_STAT_REG) & AXIW1_READY) == 0) {
+		rc = amd_w1_wait_irq_interruptible_timeout(amd_w1_local, AXIW1_READY_IRQ_EN);
+		if (rc < 0)
+			return 0xFF; /* Return inactive bus state */
+	}
+
+	/* Write read Byte command in instruction register*/
+	amd_w1_write_register(amd_w1_local, AXIW1_INST_REG, AXIW1_READBYTE);
+	/* Write Go signal and clear control reset signal in control register */
+	amd_w1_write_register(amd_w1_local, AXIW1_CTRL_REG, AXIW1_GO);
+
+	/* Wait for done signal to be 1 */
+	while ((amd_w1_read_register(amd_w1_local, AXIW1_STAT_REG) & AXIW1_DONE) != 1) {
+		rc = amd_w1_wait_irq_interruptible_timeout(amd_w1_local, AXIW1_DONE_IRQ_EN);
+		if (rc < 0)
+			return 0xFF; /* Return inactive bus state */
+	}
+
+	/* Retrieve LSB bit in data register to get RX byte */
+	val = (u8)(amd_w1_read_register(amd_w1_local, AXIW1_DATA_REG) & 0x000000FF);
+
+	/* Clear Go signal in control register */
+	amd_w1_write_register(amd_w1_local, AXIW1_CTRL_REG, AXI_CLEAR);
+
+	return val;
+}
+
+/**
+ * amd_w1_write_byte - Performs the write byte function.
+ *
+ * @data:	The ds2482 channel pointer
+ * @val:	The value to write
+ */
+static void amd_w1_write_byte(void *data, u8 val)
+{
+	struct amd_w1_local *amd_w1_local = data;
+	int rc;
+
+	/* Wait for READY signal to be 1 to ensure 1-wire IP is ready */
+	while ((amd_w1_read_register(amd_w1_local, AXIW1_STAT_REG) & AXIW1_READY) == 0) {
+		rc = amd_w1_wait_irq_interruptible_timeout(amd_w1_local, AXIW1_READY_IRQ_EN);
+		if (rc < 0)
+			return;
+	}
+
+	/* Write tx Byte command in instruction register with bit to transmit */
+	amd_w1_write_register(amd_w1_local, AXIW1_INST_REG, (AXIW1_WRITEBYTE + val));
+	/* Write Go signal and clear control reset signal in register 1 */
+	amd_w1_write_register(amd_w1_local, AXIW1_CTRL_REG, AXIW1_GO);
+
+	/* Wait for done signal to be 1 */
+	while ((amd_w1_read_register(amd_w1_local, AXIW1_STAT_REG) & AXIW1_DONE) != 1) {
+		rc = amd_w1_wait_irq_interruptible_timeout(amd_w1_local, AXIW1_DONE_IRQ_EN);
+		if (rc < 0)
+			return;
+	}
+
+	/* Clear Go signal in control register */
+	amd_w1_write_register(amd_w1_local, AXIW1_CTRL_REG, AXI_CLEAR);
+}
+
+/**
+ * amd_w1_reset_bus() - Issues a reset bus sequence.
+ *
+ * @data:	the bus master data struct
+ * Return:	0=Device present, 1=No device present or error
+ */
+static u8 amd_w1_reset_bus(void *data)
+{
+	struct amd_w1_local *amd_w1_local = data;
+	u8 val = 0;
+	int rc;
+
+	/* Reset 1-wire Axi IP */
+	amd_w1_write_register(amd_w1_local, AXIW1_CTRL_REG, AXI_RESET);
+
+	/* Wait for READY signal to be 1 to ensure 1-wire IP is ready */
+	while ((amd_w1_read_register(amd_w1_local, AXIW1_STAT_REG) & AXIW1_READY) == 0) {
+		rc = amd_w1_wait_irq_interruptible_timeout(amd_w1_local, AXIW1_READY_IRQ_EN);
+		if (rc < 0)
+			return 1; /* Something went wrong with the hardware */
+	}
+	/* Write Initialization command in instruction register */
+	amd_w1_write_register(amd_w1_local, AXIW1_INST_REG, AXIW1_INITPRES);
+	/* Write Go signal and clear control reset signal in register 1 */
+	amd_w1_write_register(amd_w1_local, AXIW1_CTRL_REG, AXIW1_GO);
+
+	/* Wait for done signal to be 1 */
+	while ((amd_w1_read_register(amd_w1_local, AXIW1_STAT_REG) & AXIW1_DONE) != 1) {
+		rc = amd_w1_wait_irq_interruptible_timeout(amd_w1_local, AXIW1_DONE_IRQ_EN);
+		if (rc < 0)
+			return 1; /* Something went wrong with the hardware */
+	}
+	/* Retrieve MSB bit in status register to get failure bit */
+	if ((amd_w1_read_register(amd_w1_local, AXIW1_STAT_REG) & AXIW1_PRESENCE) != 0)
+		val = 1;
+
+	/* Clear Go signal in control register */
+	amd_w1_write_register(amd_w1_local, AXIW1_CTRL_REG, AXI_CLEAR);
+
+	return val;
+}
+
+/* 1-wire master structure */
+static struct w1_bus_master amd_w1_master = {
+	.touch_bit	= amd_w1_touch_bit,
+	.read_byte	= amd_w1_read_byte,
+	.write_byte	= amd_w1_write_byte,
+	.reset_bus	= amd_w1_reset_bus,
+};
+
+/* Reset the 1-wire AXI IP. Put the IP in reset state and clear registers */
+static void amd_w1_reset(struct amd_w1_local *amd_w1_local)
+{
+	amd_w1_write_register(amd_w1_local, AXIW1_CTRL_REG, AXI_RESET);
+	amd_w1_write_register(amd_w1_local, AXIW1_INST_REG, AXI_CLEAR);
+	amd_w1_write_register(amd_w1_local, AXIW1_IRQE_REG, AXI_CLEAR);
+	amd_w1_write_register(amd_w1_local, AXIW1_STAT_REG, AXI_CLEAR);
+	amd_w1_write_register(amd_w1_local, AXIW1_DATA_REG, AXI_CLEAR);
+}
+
+static irqreturn_t amd_w1_irq(int irq, void *lp)
+{
+	struct amd_w1_local *amd_w1_local = lp;
+
+	/* Clear enables in IRQ enable register */
+	amd_w1_write_register(amd_w1_local, AXIW1_IRQE_REG, AXI_CLEAR);
+
+	/* Wake up the waiting queue */
+	atomic_set(&amd_w1_local->flag, 1);
+	wake_up_interruptible(&amd_w1_local->wait_queue);
+
+	return IRQ_HANDLED;
+}
+
+static int amd_w1_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct amd_w1_local *lp;
+	u32 ver_major, ver_minor;
+	int val, rc = 0;
+
+	/* Get iospace for the device */
+	lp = devm_kzalloc(dev, sizeof(*lp), GFP_KERNEL);
+	if (!lp)
+		return -ENOMEM;
+
+	lp->dev = dev;
+	lp->base_addr = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(lp->base_addr))
+		return PTR_ERR(lp->base_addr);
+
+	/* Get IRQ for the device */
+	lp->irq = platform_get_irq(pdev, 0);
+	if (lp->irq <= 0)
+		return lp->irq;
+
+	rc = devm_request_irq(dev, lp->irq, &amd_w1_irq, IRQF_TRIGGER_HIGH, DRIVER_NAME, lp);
+	if (rc) {
+		dev_err(dev, "Could not allocate interrupt %d.\n", lp->irq);
+		return rc;
+	}
+
+	/* Initialize wait queue and flag */
+	init_waitqueue_head(&lp->wait_queue);
+
+	lp->clk = devm_clk_get_enabled(dev, NULL);
+	if (IS_ERR(lp->clk))
+		return PTR_ERR(lp->clk);
+
+	/* Verify IP presence in HW */
+	if (amd_w1_read_register(lp, AXIW1_IPID_REG) != AXIW1_IPID) {
+		dev_err(dev, "AMD 1-wire IP not detected in hardware\n");
+		return -ENODEV;
+	}
+
+	/*
+	 * Allow for future driver expansion supporting new hardware features
+	 * This driver currently only supports hardware 1.x, but include logic
+	 * to detect if a potentially incompatible future version is used
+	 * by reading major version ID. It is highly undesirable for new IP versions
+	 * to break the API, but this code will at least allow for graceful failure
+	 * should that happen. Future new features can be enabled by hardware
+	 * incrementing the minor version and augmenting the driver to detect capability
+	 * using the minor version number
+	 */
+	val = amd_w1_read_register(lp, AXIW1_IPVER_REG);
+	ver_major = FIELD_GET(AXIW1_MAJORVER_MASK, val);
+	ver_minor = FIELD_GET(AXIW1_MINORVER_MASK, val);
+
+	if (ver_major != 1) {
+		dev_err(dev, "AMD AXI W1 Master version %u.%u is not supported by this driver",
+			ver_major, ver_minor);
+		return -ENODEV;
+	}
+
+	amd_w1_master.data = (void *)lp;
+	amd_w1_reset(lp);
+
+	platform_set_drvdata(pdev, lp);
+	rc = w1_add_master_device(&amd_w1_master);
+	if (rc) {
+		dev_err(dev, "Could not add master device\n");
+		amd_w1_reset(lp);
+		return rc;
+	}
+
+	return 0;
+}
+
+static void amd_w1_remove(struct platform_device *pdev)
+{
+	struct amd_w1_local *lp = platform_get_drvdata(pdev);
+
+	w1_remove_master_device(&amd_w1_master);
+	clk_disable_unprepare(lp->clk);
+}
+
+static const struct of_device_id amd_w1_of_match[] = {
+	{ .compatible = "amd,axi-1wire-master" },
+	{ /* end of list */ },
+};
+MODULE_DEVICE_TABLE(of, amd_w1_of_match);
+
+static struct platform_driver amd_w1_driver = {
+	.probe = amd_w1_probe,
+	.remove_new = amd_w1_remove,
+	.driver = {
+		.name = DRIVER_NAME,
+		.of_match_table = amd_w1_of_match,
+	},
+};
+module_platform_driver(amd_w1_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Kris Chaplin <kris.chaplin@amd.com>");
+MODULE_DESCRIPTION("Driver for AMD AXI 1 Wire IP core");
-- 
2.42.GIT

Re: [PATCH 1/2] dt-bindings: w1: Add YAML DT Schema for AMD w1 master and MAINTAINERS entry

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2833 bytes --]

On Fri, Oct 13, 2023 at 02:30:12AM -0700, Kris Chaplin wrote:
> Add YAML DT Schema for the AMD w1 master IP.
> 
> This hardware guarantees protocol timing for driving off-board devices such
> as thermal sensors, proms, etc using the 1wire protocol.
> 
> Add MAINTAINERS entry for DT Schema.
> 
> Co-developed-by: Thomas Delev <thomas.delev@amd.com>
> Signed-off-by: Thomas Delev <thomas.delev@amd.com>
> Signed-off-by: Kris Chaplin <kris.chaplin@amd.com>
> ---
>  .../bindings/w1/amd,axi-1wire-master.yaml     | 44 +++++++++++++++++++
>  MAINTAINERS                                   |  7 +++
>  2 files changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml
> 
> diff --git a/Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml b/Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml
> new file mode 100644
> index 000000000000..41f7294a84a3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml
> @@ -0,0 +1,44 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/w1/amd,axi-1wire-master.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: AMD AXI 1-wire bus master for Programmable Logic

Inconsistent case for the title here bothers my OCD, but the binding is
fine as far as I can see.
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

> +
> +maintainers:
> +  - Kris Chaplin <kris.chaplin@amd.com>
> +
> +properties:
> +  compatible:
> +    const: amd,axi-1wire-master
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    onewire@a0000000 {
> +        compatible = "amd,axi-1wire-master";
> +        reg = <0xa0000000 0x10000>;
> +        clocks = <&zynqmp_clk 0x47>;
> +        interrupts = <GIC_SPI 0x59 IRQ_TYPE_LEVEL_HIGH>;
> +    };
> +
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4b2c378b4fd9..6ec3922b256e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1066,6 +1066,13 @@ M:	Sanjay R Mehta <sanju.mehta@amd.com>
>  S:	Maintained
>  F:	drivers/spi/spi-amd.c
>  
> +AMD W1 DRIVER
> +M:	Kris Chaplin <kris.chaplin@amd.com>
> +R:	Thomas Delev <thomas.delev@amd.com>
> +R:	Michal Simek <michal.simek@amd.com>
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml
> +
>  AMD XGBE DRIVER
>  M:	"Shyam Sundar S K" <Shyam-sundar.S-k@amd.com>
>  L:	netdev@vger.kernel.org
> -- 
> 2.42.GIT
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/2] dt-bindings: w1: Add YAML DT Schema for AMD w1 master and MAINTAINERS entry

[Krzysztof Kozlowski]

On 13/10/2023 11:30, Kris Chaplin wrote:
> Add YAML DT Schema for the AMD w1 master IP.
> 
> This hardware guarantees protocol timing for driving off-board devices such
> as thermal sensors, proms, etc using the 1wire protocol.
> 
> Add MAINTAINERS entry for DT Schema.
> 
> Co-developed-by: Thomas Delev <thomas.delev@amd.com>
> Signed-off-by: Thomas Delev <thomas.delev@amd.com>
> Signed-off-by: Kris Chaplin <kris.chaplin@amd.com>
> ---
>  .../bindings/w1/amd,axi-1wire-master.yaml     | 44 +++++++++++++++++++
>  MAINTAINERS                                   |  7 +++
>  2 files changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml
> 
> diff --git a/Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml b/Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml
> new file mode 100644
> index 000000000000..41f7294a84a3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml
> @@ -0,0 +1,44 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/w1/amd,axi-1wire-master.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: AMD AXI 1-wire bus master for Programmable Logic
> +
> +maintainers:
> +  - Kris Chaplin <kris.chaplin@amd.com>
> +
> +properties:
> +  compatible:
> +    const: amd,axi-1wire-master

That's a quite generic compatible. axi is ARM term, 1-wire is the name
of the bus and master is the role. Concatenating three common words does
not create unique device name. Compatibles are supposed to be specific
and this is really relaxed. Anything can be over AXI, everything in
1wire is 1wire and every master device is a master.



Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: w1: Add YAML DT Schema for AMD w1 master and MAINTAINERS entry

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2163 bytes --]

On Fri, Oct 13, 2023 at 05:04:32PM +0200, Krzysztof Kozlowski wrote:
> On 13/10/2023 11:30, Kris Chaplin wrote:
> > Add YAML DT Schema for the AMD w1 master IP.
> > 
> > This hardware guarantees protocol timing for driving off-board devices such
> > as thermal sensors, proms, etc using the 1wire protocol.
> > 
> > Add MAINTAINERS entry for DT Schema.
> > 
> > Co-developed-by: Thomas Delev <thomas.delev@amd.com>
> > Signed-off-by: Thomas Delev <thomas.delev@amd.com>
> > Signed-off-by: Kris Chaplin <kris.chaplin@amd.com>
> > ---
> >  .../bindings/w1/amd,axi-1wire-master.yaml     | 44 +++++++++++++++++++
> >  MAINTAINERS                                   |  7 +++
> >  2 files changed, 51 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml b/Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml
> > new file mode 100644
> > index 000000000000..41f7294a84a3
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml
> > @@ -0,0 +1,44 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/w1/amd,axi-1wire-master.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: AMD AXI 1-wire bus master for Programmable Logic
> > +
> > +maintainers:
> > +  - Kris Chaplin <kris.chaplin@amd.com>
> > +
> > +properties:
> > +  compatible:
> > +    const: amd,axi-1wire-master
> 
> That's a quite generic compatible. axi is ARM term, 1-wire is the name
> of the bus and master is the role. Concatenating three common words does
> not create unique device name. Compatibles are supposed to be specific
> and this is really relaxed. Anything can be over AXI, everything in
> 1wire is 1wire and every master device is a master.

Given the vendor (and the title of the binding) this is almost certainly
an FPGA IP core, so the generic name is understandable. Using the exact
name of the IP in the AMD/Xilinx catalog probably is the best choice?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/2] w1: Add 1-wire master driver for AMD programmable logic IP Core

[Krzysztof Kozlowski]

On 13/10/2023 11:30, Kris Chaplin wrote:
> Add a master driver to support the AMD 1-Wire programmable logic IP block.
> This block guarantees protocol timing for driving off-board devices such
> as thermal sensors, proms, etc.
> 
> Add file to MAINTAINERS
> 
> Co-developed-by: Thomas Delev <thomas.delev@amd.com>
> Signed-off-by: Thomas Delev <thomas.delev@amd.com>
> Signed-off-by: Kris Chaplin <kris.chaplin@amd.com>
> ---
>  MAINTAINERS                 |   1 +
>  drivers/w1/masters/Kconfig  |  11 +
>  drivers/w1/masters/Makefile |   1 +
>  drivers/w1/masters/amd_w1.c | 422 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 435 insertions(+)
>  create mode 100644 drivers/w1/masters/amd_w1.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6ec3922b256e..7f26dab5261b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1072,6 +1072,7 @@ R:	Thomas Delev <thomas.delev@amd.com>
>  R:	Michal Simek <michal.simek@amd.com>
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml
> +F:	drivers/w1/masters/amd_w1.c
>  
>  AMD XGBE DRIVER
>  M:	"Shyam Sundar S K" <Shyam-sundar.S-k@amd.com>
> diff --git a/drivers/w1/masters/Kconfig b/drivers/w1/masters/Kconfig
> index ad316573288a..9232fd1f4e5b 100644
> --- a/drivers/w1/masters/Kconfig
> +++ b/drivers/w1/masters/Kconfig
> @@ -67,5 +67,16 @@ config W1_MASTER_SGI
>  	  This support is also available as a module.  If so, the module
>  	  will be called sgi_w1.
>  
> +config W1_MASTER_AMD

This looks oddly places. Isn't entry above 'S', so A should go before?
The rule is for entire Linux kernel: do not stuff things to the end of
the lists.

> +	tristate "AMD AXI 1-wire bus master"
> +	help
> +	  Say Y here is you want to support the AMD AXI 1-wire IP core.
> +	  This driver makes use of the programmable logic IP to perform
> +	  correctly timed 1 wire transactions without relying on GPIO timing
> +	  through the kernel.
> +
> +	  This driver can also be built as a module.  If so, the module will be
> +	  called amd_w1.
> +
>  endmenu
>  
> diff --git a/drivers/w1/masters/Makefile b/drivers/w1/masters/Makefile
> index c5d85a827e52..cd3da1daaf97 100644
> --- a/drivers/w1/masters/Makefile
> +++ b/drivers/w1/masters/Makefile
> @@ -11,3 +11,4 @@ obj-$(CONFIG_W1_MASTER_MXC)		+= mxc_w1.o
>  obj-$(CONFIG_W1_MASTER_GPIO)		+= w1-gpio.o
>  obj-$(CONFIG_HDQ_MASTER_OMAP)		+= omap_hdq.o
>  obj-$(CONFIG_W1_MASTER_SGI)		+= sgi_w1.o
> +obj-$(CONFIG_W1_MASTER_AMD)		+= amd_w1.o
> diff --git a/drivers/w1/masters/amd_w1.c b/drivers/w1/masters/amd_w1.c
> new file mode 100644
> index 000000000000..04bf08c1b6d7
> --- /dev/null
> +++ b/drivers/w1/masters/amd_w1.c
> @@ -0,0 +1,422 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * amd_w1 - AMD 1Wire bus master driver
> + *
> + * Copyright (C) 2022-2023 Advanced Micro Devices, Inc. All Rights Reserved.
> + */
> +
> +#include <linux/atomic.h>
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/jiffies.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/types.h>
> +#include <linux/wait.h>
> +
> +#include <linux/w1.h>
> +
> +/* 1-wire AMD IP definition */
> +#define AXIW1_IPID	0x10ee4453
> +/* Registers offset */
> +#define AXIW1_INST_REG	0x0
> +#define AXIW1_CTRL_REG	0x4
> +#define AXIW1_IRQE_REG	0x8
> +#define AXIW1_STAT_REG	0xC
> +#define AXIW1_DATA_REG	0x10
> +#define AXIW1_IPVER_REG	0x18
> +#define AXIW1_IPID_REG	0x1C
> +/* Instructions */
> +#define AXIW1_INITPRES	0x0800
> +#define AXIW1_READBIT	0x0C00
> +#define AXIW1_WRITEBIT	0x0E00
> +#define AXIW1_READBYTE	0x0D00
> +#define AXIW1_WRITEBYTE	0x0F00
> +/* Status flag masks */
> +#define AXIW1_DONE	BIT(0)
> +#define AXIW1_READY	BIT(4)
> +#define AXIW1_PRESENCE	BIT(31)
> +#define AXIW1_MAJORVER_MASK	GENMASK(23, 8)
> +#define AXIW1_MINORVER_MASK	GENMASK(7, 0)
> +/* Control flag */
> +#define AXIW1_GO	BIT(0)
> +#define AXI_CLEAR	0
> +#define AXI_RESET	BIT(31)
> +#define AXIW1_READDATA	BIT(0)
> +/* Interrupt Enable */
> +#define AXIW1_READY_IRQ_EN	BIT(4)
> +#define AXIW1_DONE_IRQ_EN	BIT(0)
> +
> +#define AXIW1_TIMEOUT	msecs_to_jiffies(100)
> +
> +#define DRIVER_NAME	"amd_w1"
> +
> +struct amd_w1_local {
> +	struct device *dev;
> +	void __iomem *base_addr;
> +	int irq;
> +	atomic_t flag;

Document what this flag does and what its purpose.

> +	struct clk *clk;

Why do you need this? It's never changed (after fixing the bug).

> +	wait_queue_head_t wait_queue;
> +};
> +
> +/**
> + * amd_w1_write_register() - Helper to write a hardware register.
> + *
> + * @amd_w1_local:	Pointer to device structure
> + * @reg_offset:		Register offset in bytes to write
> + * @val:		Value to write
> + */
> +static inline void amd_w1_write_register(struct amd_w1_local *amd_w1_local,
> +					 u8 reg_offset, u32 val)
> +{
> +	iowrite32(val, amd_w1_local->base_addr + reg_offset);

Unwrapped code:
  iowrite32(IRQ, amd_w1_local->base_addr + AXIW1_IRQE_REG);

Your wrapper:
  amd_w1_write_register(amd_w1_local, AXIW1_IRQE_REG, IRQ);

Does not look simpler. If you want to use wrappers, they should actually
help.

> +};
> +
> +/**
> + * amd_w1_read_register() - Helper to write a hardware register.
> + *
> + * @amd_w1_local:	Pointer to device structure
> + * @reg_offset:		Register offset in bytes to write
> + *
> + * Return:		Value of register read
> + */
> +static inline u32 amd_w1_read_register(struct amd_w1_local *amd_w1_local, u8 reg_offset)
> +{
> +	return ioread32(amd_w1_local->base_addr + reg_offset);
> +};
> +
> +/**
> + * amd_w1_wait_irq_interruptible_timeout() - Wait for IRQ with timeout.
> + *
> + * @amd_w1_local:	Pointer to device structure
> + * @IRQ:		IRQ channel to wait on
> + *
> + * Return:		%0 - OK, %-EINTR - Interrupted, %-EBUSY - Timed out
> + */
> +static inline int amd_w1_wait_irq_interruptible_timeout(struct amd_w1_local *amd_w1_local, u32 IRQ)

Drop inline.
This does not look like wrapped according to Linux coding convention.
Please abide by the convention, so wrap at 80.

> +{
> +	int ret;
> +
> +	/* Enable the IRQ requested and wait for flag to indicate it's been triggered */
> +	amd_w1_write_register(amd_w1_local, AXIW1_IRQE_REG, IRQ);
> +	ret = wait_event_interruptible_timeout(amd_w1_local->wait_queue,
> +					       atomic_read(&amd_w1_local->flag) != 0,
> +					       AXIW1_TIMEOUT);
> +	if (ret < 0) {
> +		dev_err(amd_w1_local->dev, "Wait IRQ Interrupted\n");
> +		return -EINTR;
> +	}
> +
> +	if (!ret) {
> +		dev_err(amd_w1_local->dev, "Wait IRQ Timeout\n");
> +		return -EBUSY;
> +	}
> +
> +	/* Clear flag */

Drop.

> +	atomic_set(&amd_w1_local->flag, 0);
> +	return 0;
> +}
> +
> +/**
> + * amd_w1_touch_bit() - Performs the touch-bit function, which writes a 0 or 1 and reads the level.
> + *
> + * @data:	Pointer to device structure
> + * @bit:	The level to write
> + *
> + * Return:	The level read
> + */
> +static u8 amd_w1_touch_bit(void *data, u8 bit)
> +{
> +	struct amd_w1_local *amd_w1_local = data;
> +	u8 val = 0;
> +	int rc;
> +
> +	/* Wait for READY signal to be 1 to ensure 1-wire IP is ready */
> +	while ((amd_w1_read_register(amd_w1_local, AXIW1_STAT_REG) & AXIW1_READY) == 0) {
> +		rc = amd_w1_wait_irq_interruptible_timeout(amd_w1_local, AXIW1_READY_IRQ_EN);
> +		if (rc < 0)
> +			return 1; /* Callee doesn't test for error. Return inactive bus state */
> +	}
> +
> +	if (bit)
> +		/* Read. Write read Bit command in register 0 */
> +		amd_w1_write_register(amd_w1_local, AXIW1_INST_REG, AXIW1_READBIT);
> +	else
> +		/* Write. Write tx Bit command in instruction register with bit to transmit */
> +		amd_w1_write_register(amd_w1_local, AXIW1_INST_REG,
> +				      (AXIW1_WRITEBIT + (bit & 0x01)));
> +
> +	/* Write Go signal and clear control reset signal in control register */
> +	amd_w1_write_register(amd_w1_local, AXIW1_CTRL_REG, AXIW1_GO);
> +
> +	/* Wait for done signal to be 1 */
> +	while ((amd_w1_read_register(amd_w1_local, AXIW1_STAT_REG) & AXIW1_DONE) != 1) {
> +		rc = amd_w1_wait_irq_interruptible_timeout(amd_w1_local, AXIW1_DONE_IRQ_EN);
> +		if (rc < 0)
> +			return 1; /* Callee doesn't test for error. Return inactive bus state */
> +	}
> +
> +	/* If read, Retrieve data from register */
> +	if (bit)
> +		val = (u8)(amd_w1_read_register(amd_w1_local, AXIW1_DATA_REG) & AXIW1_READDATA);
> +
> +	/* Clear Go signal in register 1 */
> +	amd_w1_write_register(amd_w1_local, AXIW1_CTRL_REG, AXI_CLEAR);
> +
> +	return val;
> +}
> +
> +/**
> + * amd_w1_read_byte - Performs the read byte function.
> + *
> + * @data:	Pointer to device structure
> + * Return:	The value read
> + */
> +static u8 amd_w1_read_byte(void *data)
> +{
> +	struct amd_w1_local *amd_w1_local = data;
> +	u8 val = 0;
> +	int rc;
> +
> +	/* Wait for READY signal to be 1 to ensure 1-wire IP is ready */
> +	while ((amd_w1_read_register(amd_w1_local, AXIW1_STAT_REG) & AXIW1_READY) == 0) {
> +		rc = amd_w1_wait_irq_interruptible_timeout(amd_w1_local, AXIW1_READY_IRQ_EN);
> +		if (rc < 0)
> +			return 0xFF; /* Return inactive bus state */
> +	}
> +
> +	/* Write read Byte command in instruction register*/
> +	amd_w1_write_register(amd_w1_local, AXIW1_INST_REG, AXIW1_READBYTE);
> +	/* Write Go signal and clear control reset signal in control register */
> +	amd_w1_write_register(amd_w1_local, AXIW1_CTRL_REG, AXIW1_GO);
> +
> +	/* Wait for done signal to be 1 */
> +	while ((amd_w1_read_register(amd_w1_local, AXIW1_STAT_REG) & AXIW1_DONE) != 1) {
> +		rc = amd_w1_wait_irq_interruptible_timeout(amd_w1_local, AXIW1_DONE_IRQ_EN);
> +		if (rc < 0)
> +			return 0xFF; /* Return inactive bus state */
> +	}
> +
> +	/* Retrieve LSB bit in data register to get RX byte */
> +	val = (u8)(amd_w1_read_register(amd_w1_local, AXIW1_DATA_REG) & 0x000000FF);
> +
> +	/* Clear Go signal in control register */
> +	amd_w1_write_register(amd_w1_local, AXIW1_CTRL_REG, AXI_CLEAR);
> +
> +	return val;
> +}
> +
> +/**
> + * amd_w1_write_byte - Performs the write byte function.
> + *
> + * @data:	The ds2482 channel pointer
> + * @val:	The value to write
> + */
> +static void amd_w1_write_byte(void *data, u8 val)
> +{
> +	struct amd_w1_local *amd_w1_local = data;
> +	int rc;
> +
> +	/* Wait for READY signal to be 1 to ensure 1-wire IP is ready */
> +	while ((amd_w1_read_register(amd_w1_local, AXIW1_STAT_REG) & AXIW1_READY) == 0) {
> +		rc = amd_w1_wait_irq_interruptible_timeout(amd_w1_local, AXIW1_READY_IRQ_EN);
> +		if (rc < 0)
> +			return;
> +	}
> +
> +	/* Write tx Byte command in instruction register with bit to transmit */
> +	amd_w1_write_register(amd_w1_local, AXIW1_INST_REG, (AXIW1_WRITEBYTE + val));
> +	/* Write Go signal and clear control reset signal in register 1 */
> +	amd_w1_write_register(amd_w1_local, AXIW1_CTRL_REG, AXIW1_GO);
> +
> +	/* Wait for done signal to be 1 */
> +	while ((amd_w1_read_register(amd_w1_local, AXIW1_STAT_REG) & AXIW1_DONE) != 1) {
> +		rc = amd_w1_wait_irq_interruptible_timeout(amd_w1_local, AXIW1_DONE_IRQ_EN);
> +		if (rc < 0)
> +			return;
> +	}
> +
> +	/* Clear Go signal in control register */
> +	amd_w1_write_register(amd_w1_local, AXIW1_CTRL_REG, AXI_CLEAR);
> +}
> +
> +/**
> + * amd_w1_reset_bus() - Issues a reset bus sequence.
> + *
> + * @data:	the bus master data struct
> + * Return:	0=Device present, 1=No device present or error
> + */
> +static u8 amd_w1_reset_bus(void *data)
> +{
> +	struct amd_w1_local *amd_w1_local = data;
> +	u8 val = 0;
> +	int rc;
> +
> +	/* Reset 1-wire Axi IP */
> +	amd_w1_write_register(amd_w1_local, AXIW1_CTRL_REG, AXI_RESET);
> +
> +	/* Wait for READY signal to be 1 to ensure 1-wire IP is ready */
> +	while ((amd_w1_read_register(amd_w1_local, AXIW1_STAT_REG) & AXIW1_READY) == 0) {
> +		rc = amd_w1_wait_irq_interruptible_timeout(amd_w1_local, AXIW1_READY_IRQ_EN);
> +		if (rc < 0)
> +			return 1; /* Something went wrong with the hardware */
> +	}
> +	/* Write Initialization command in instruction register */
> +	amd_w1_write_register(amd_w1_local, AXIW1_INST_REG, AXIW1_INITPRES);
> +	/* Write Go signal and clear control reset signal in register 1 */
> +	amd_w1_write_register(amd_w1_local, AXIW1_CTRL_REG, AXIW1_GO);
> +
> +	/* Wait for done signal to be 1 */
> +	while ((amd_w1_read_register(amd_w1_local, AXIW1_STAT_REG) & AXIW1_DONE) != 1) {
> +		rc = amd_w1_wait_irq_interruptible_timeout(amd_w1_local, AXIW1_DONE_IRQ_EN);
> +		if (rc < 0)
> +			return 1; /* Something went wrong with the hardware */
> +	}
> +	/* Retrieve MSB bit in status register to get failure bit */
> +	if ((amd_w1_read_register(amd_w1_local, AXIW1_STAT_REG) & AXIW1_PRESENCE) != 0)
> +		val = 1;
> +
> +	/* Clear Go signal in control register */
> +	amd_w1_write_register(amd_w1_local, AXIW1_CTRL_REG, AXI_CLEAR);
> +
> +	return val;
> +}
> +
> +/* 1-wire master structure */
> +static struct w1_bus_master amd_w1_master = {

And how does it work with two devices?

> +	.touch_bit	= amd_w1_touch_bit,
> +	.read_byte	= amd_w1_read_byte,
> +	.write_byte	= amd_w1_write_byte,
> +	.reset_bus	= amd_w1_reset_bus,
> +};
> +
> +/* Reset the 1-wire AXI IP. Put the IP in reset state and clear registers */
> +static void amd_w1_reset(struct amd_w1_local *amd_w1_local)
> +{
> +	amd_w1_write_register(amd_w1_local, AXIW1_CTRL_REG, AXI_RESET);
> +	amd_w1_write_register(amd_w1_local, AXIW1_INST_REG, AXI_CLEAR);
> +	amd_w1_write_register(amd_w1_local, AXIW1_IRQE_REG, AXI_CLEAR);
> +	amd_w1_write_register(amd_w1_local, AXIW1_STAT_REG, AXI_CLEAR);
> +	amd_w1_write_register(amd_w1_local, AXIW1_DATA_REG, AXI_CLEAR);
> +}
> +
> +static irqreturn_t amd_w1_irq(int irq, void *lp)
> +{
> +	struct amd_w1_local *amd_w1_local = lp;
> +
> +	/* Clear enables in IRQ enable register */

I don't understand this comment.

> +	amd_w1_write_register(amd_w1_local, AXIW1_IRQE_REG, AXI_CLEAR);
> +
> +	/* Wake up the waiting queue */


Drop obvious comments.

> +	atomic_set(&amd_w1_local->flag, 1);
> +	wake_up_interruptible(&amd_w1_local->wait_queue);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int amd_w1_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct amd_w1_local *lp;
> +	u32 ver_major, ver_minor;
> +	int val, rc = 0;
> +
> +	/* Get iospace for the device */

This is memory allocation, not IO space.

> +	lp = devm_kzalloc(dev, sizeof(*lp), GFP_KERNEL);
> +	if (!lp)
> +		return -ENOMEM;
> +
> +	lp->dev = dev;
> +	lp->base_addr = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(lp->base_addr))
> +		return PTR_ERR(lp->base_addr);
> +
> +	/* Get IRQ for the device */

Drop obvious comments.

> +	lp->irq = platform_get_irq(pdev, 0);
> +	if (lp->irq <= 0)

Open platform_get_irq() function and read the help. Error handling
couldn't be more explicit there...

> +		return lp->irq;
> +
> +	rc = devm_request_irq(dev, lp->irq, &amd_w1_irq, IRQF_TRIGGER_HIGH, DRIVER_NAME, lp);
> +	if (rc) {
> +		dev_err(dev, "Could not allocate interrupt %d.\n", lp->irq);

return dev_err_probe(). But this leads us to second question: why would
you notify about allocation errors? Core does it. Do you mean request?

> +		return rc;
> +	}
> +
> +	/* Initialize wait queue and flag */
> +	init_waitqueue_head(&lp->wait_queue);
> +
> +	lp->clk = devm_clk_get_enabled(dev, NULL);
> +	if (IS_ERR(lp->clk))
> +		return PTR_ERR(lp->clk);
> +
> +	/* Verify IP presence in HW */
> +	if (amd_w1_read_register(lp, AXIW1_IPID_REG) != AXIW1_IPID) {
> +		dev_err(dev, "AMD 1-wire IP not detected in hardware\n");
> +		return -ENODEV;
> +	}
> +
> +	/*
> +	 * Allow for future driver expansion supporting new hardware features
> +	 * This driver currently only supports hardware 1.x, but include logic
> +	 * to detect if a potentially incompatible future version is used
> +	 * by reading major version ID. It is highly undesirable for new IP versions
> +	 * to break the API, but this code will at least allow for graceful failure
> +	 * should that happen. Future new features can be enabled by hardware
> +	 * incrementing the minor version and augmenting the driver to detect capability
> +	 * using the minor version number
> +	 */
> +	val = amd_w1_read_register(lp, AXIW1_IPVER_REG);
> +	ver_major = FIELD_GET(AXIW1_MAJORVER_MASK, val);
> +	ver_minor = FIELD_GET(AXIW1_MINORVER_MASK, val);
> +
> +	if (ver_major != 1) {
> +		dev_err(dev, "AMD AXI W1 Master version %u.%u is not supported by this driver",
> +			ver_major, ver_minor);
> +		return -ENODEV;
> +	}
> +
> +	amd_w1_master.data = (void *)lp;
> +	amd_w1_reset(lp);
> +
> +	platform_set_drvdata(pdev, lp);
> +	rc = w1_add_master_device(&amd_w1_master);
> +	if (rc) {
> +		dev_err(dev, "Could not add master device\n");
> +		amd_w1_reset(lp);

Why? Does this mean that w1_add_master_device() changes the state of
hardware?

> +		return rc;
> +	}
> +
> +	return 0;
> +}
> +
> +static void amd_w1_remove(struct platform_device *pdev)
> +{
> +	struct amd_w1_local *lp = platform_get_drvdata(pdev);
> +
> +	w1_remove_master_device(&amd_w1_master);
> +	clk_disable_unprepare(lp->clk);

This wasn't tested. Duplicated disable.

> +}
> +
> +static const struct of_device_id amd_w1_of_match[] = {
> +	{ .compatible = "amd,axi-1wire-master" },
> +	{ /* end of list */ },
> +};
> +MODULE_DEVICE_TABLE(of, amd_w1_of_match);
> +
> +static struct platform_driver amd_w1_driver = {
> +	.probe = amd_w1_probe,
> +	.remove_new = amd_w1_remove,
> +	.driver = {
> +		.name = DRIVER_NAME,
> +		.of_match_table = amd_w1_of_match,
> +	},
> +};
> +module_platform_driver(amd_w1_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Kris Chaplin <kris.chaplin@amd.com>");
> +MODULE_DESCRIPTION("Driver for AMD AXI 1 Wire IP core");

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: w1: Add YAML DT Schema for AMD w1 master and MAINTAINERS entry

[Krzysztof Kozlowski]

On 13/10/2023 17:07, Conor Dooley wrote:
>>> +maintainers:
>>> +  - Kris Chaplin <kris.chaplin@amd.com>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: amd,axi-1wire-master
>>
>> That's a quite generic compatible. axi is ARM term, 1-wire is the name
>> of the bus and master is the role. Concatenating three common words does
>> not create unique device name. Compatibles are supposed to be specific
>> and this is really relaxed. Anything can be over AXI, everything in
>> 1wire is 1wire and every master device is a master.
> 
> Given the vendor (and the title of the binding) this is almost certainly
> an FPGA IP core, so the generic name is understandable. Using the exact
> name of the IP in the AMD/Xilinx catalog probably is the best choice?

Other option is that it is a part of some Zynq SoC.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: w1: Add YAML DT Schema for AMD w1 master and MAINTAINERS entry

[Kris Chaplin]

On 13/10/2023 16:07, Conor Dooley wrote:
> On Fri, Oct 13, 2023 at 05:04:32PM +0200, Krzysztof Kozlowski wrote:
>>
>> That's a quite generic compatible. axi is ARM term, 1-wire is the name
>> of the bus and master is the role. Concatenating three common words does
>> not create unique device name. Compatibles are supposed to be specific
>> and this is really relaxed. Anything can be over AXI, everything in
>> 1wire is 1wire and every master device is a master.
> Given the vendor (and the title of the binding) this is almost certainly
> an FPGA IP core, so the generic name is understandable. Using the exact
> name of the IP in the AMD/Xilinx catalog probably is the best choice?

Indeed this is an Programmable Logic IP core - the official name of the 
core in our catalog is axi_1wire_master.  It is a soft HDL core.

regards,

Kris

Re: [PATCH 1/2] dt-bindings: w1: Add YAML DT Schema for AMD w1 master and MAINTAINERS entry

[Krzysztof Kozlowski]

On 13/10/2023 17:23, Kris Chaplin wrote:
> 
> On 13/10/2023 16:07, Conor Dooley wrote:
>> On Fri, Oct 13, 2023 at 05:04:32PM +0200, Krzysztof Kozlowski wrote:
>>>
>>> That's a quite generic compatible. axi is ARM term, 1-wire is the name
>>> of the bus and master is the role. Concatenating three common words does
>>> not create unique device name. Compatibles are supposed to be specific
>>> and this is really relaxed. Anything can be over AXI, everything in
>>> 1wire is 1wire and every master device is a master.
>> Given the vendor (and the title of the binding) this is almost certainly
>> an FPGA IP core, so the generic name is understandable. Using the exact
>> name of the IP in the AMD/Xilinx catalog probably is the best choice?
> 
> Indeed this is an Programmable Logic IP core - the official name of the 
> core in our catalog is axi_1wire_master.  It is a soft HDL core.

AMD product managers are highly skilled in naming things. Sigh.

Go ahead with AXI 1-wire master. Any future - from now to next 100 years
- product from AMD which will be different but sold under the same name,
thus creating conflict in compatible naming, should be rejected because
of that conflict or renamed to something else. If that happen I will
propose a name like "banana-wire".

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: w1: Add YAML DT Schema for AMD w1 master and MAINTAINERS entry

[Kris Chaplin]

On 13/10/2023 16:29, Krzysztof Kozlowski wrote:
>
> AMD product managers are highly skilled in naming things. Sigh.
>
> Go ahead with AXI 1-wire master. Any future - from now to next 100 years
> - product from AMD which will be different but sold under the same name,
> thus creating conflict in compatible naming, should be rejected because
> of that conflict or renamed to something else. If that happen I will
> propose a name like "banana-wire".
>
> Best regards,
> Krzysztof

To be fair on the product managers the IP naming from a hardware 
perspective is in the context of the ex-Xilinx development tools, and as 
such there is no risk of namespace overlap with the rest of the 
business.  I'm not against changing the binding name for clarity in the 
kernel. Would adding pl within the binding (Programmable Logic) assist? 
Ie: amd,pl-axi-1wire-master

Regards
Kris

Re: [PATCH 1/2] dt-bindings: w1: Add YAML DT Schema for AMD w1 master and MAINTAINERS entry

[Rob Herring]

On Fri, Oct 13, 2023 at 04:23:15PM +0100, Kris Chaplin wrote:
> 
> On 13/10/2023 16:07, Conor Dooley wrote:
> > On Fri, Oct 13, 2023 at 05:04:32PM +0200, Krzysztof Kozlowski wrote:
> > > 
> > > That's a quite generic compatible. axi is ARM term, 1-wire is the name
> > > of the bus and master is the role. Concatenating three common words does
> > > not create unique device name. Compatibles are supposed to be specific
> > > and this is really relaxed. Anything can be over AXI, everything in
> > > 1wire is 1wire and every master device is a master.
> > Given the vendor (and the title of the binding) this is almost certainly
> > an FPGA IP core, so the generic name is understandable. Using the exact
> > name of the IP in the AMD/Xilinx catalog probably is the best choice?
> 
> Indeed this is an Programmable Logic IP core - the official name of the core
> in our catalog is axi_1wire_master.  It is a soft HDL core.

Only 1 version of it (ever)? Like other PL IP, it needs a version number 
(and not v1, v2, etc. made up by you). Really, your versioning scheme 
should be documented (like 
bindings/sifive/sifive-blocks-ip-versioning.txt), but Xilinx started 
versioning stuff some time back.

Also, 'master' is not considered great terminology nowadays. Perhaps the 
catalog name should be updated. 

Rob

Re: [PATCH 1/2] dt-bindings: w1: Add YAML DT Schema for AMD w1 master and MAINTAINERS entry

[Kris Chaplin]

On 13/10/2023 18:18, Rob Herring wrote:
> i_1wire_master. It is a soft HDL core.
> Only 1 version of it (ever)? Like other PL IP, it needs a version number
> (and not v1, v2, etc. made up by you). Really, your versioning scheme
> should be documented (like
> bindings/sifive/sifive-blocks-ip-versioning.txt), but Xilinx started
> versioning stuff some time back.

I've specified the PL IP to have both an ID and version number register 
in it, which is queried by the driver on probe.  As such we can version 
autodiscover.  Should there be incompatibilty in the driver due to new 
features in future PL IP, the major version number in the register will 
increment and the same driver can be extended to support the modified 
behaviour.  The default code in this first version will check to ensure 
major is at 1. I'm working with our IP group to encourage that all new 
IP have this mechanism moving forwards, as registers are a lot cheaper 
in logic than they used to be.

+  if (ver_major != 1) {
+     dev_err(dev, "AMD AXI W1 Master version %u.%u is not supported by 
this driver",
+        ver_major, ver_minor);
+     return -ENODEV;
+  }

> Also, 'master' is not considered great terminology nowadays. Perhaps the
> catalog name should be updated.

Agreed -  I've used the term to fit with others in the subsystem.  Would 
this be something that is changed and aligned across W1?  If so I'm 
happy to get the HDL IP renamed and binding / driver documentation to 
match if an alternative designator to replace master has already been 
chosen and standardised across 1Wire.

Regards
Kris

Re: [PATCH 2/2] w1: Add 1-wire master driver for AMD programmable logic IP Core

[Kris Chaplin]

Thank you Krzysztof,

I shall post v2 tomorrow:

On 13/10/2023 16:20, Krzysztof Kozlowski wrote:
> On 13/10/2023 11:30, Kris Chaplin wrote:
>> Add a master driver to support the AMD 1-Wire programmable logic IP block.
>> This block guarantees protocol timing for driving off-board devices such
>> as thermal sensors, proms, etc.
>>
>> Add file to MAINTAINERS
>>
>> Co-developed-by: Thomas Delev <thomas.delev@amd.com>
>> Signed-off-by: Thomas Delev <thomas.delev@amd.com>
>> Signed-off-by: Kris Chaplin <kris.chaplin@amd.com>
>> ---
>>   MAINTAINERS                 |   1 +
>>   drivers/w1/masters/Kconfig  |  11 +
>>   drivers/w1/masters/Makefile |   1 +
>>   drivers/w1/masters/amd_w1.c | 422 ++++++++++++++++++++++++++++++++++++
>>   4 files changed, 435 insertions(+)
>>   create mode 100644 drivers/w1/masters/amd_w1.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 6ec3922b256e..7f26dab5261b 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1072,6 +1072,7 @@ R:      Thomas Delev <thomas.delev@amd.com>
>>   R:   Michal Simek <michal.simek@amd.com>
>>   S:   Maintained
>>   F:   Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml
>> +F:   drivers/w1/masters/amd_w1.c
>>
>>   AMD XGBE DRIVER
>>   M:   "Shyam Sundar S K" <Shyam-sundar.S-k@amd.com>
>> diff --git a/drivers/w1/masters/Kconfig b/drivers/w1/masters/Kconfig
>> index ad316573288a..9232fd1f4e5b 100644
>> --- a/drivers/w1/masters/Kconfig
>> +++ b/drivers/w1/masters/Kconfig
>> @@ -67,5 +67,16 @@ config W1_MASTER_SGI
>>          This support is also available as a module.  If so, the module
>>          will be called sgi_w1.
>>
>> +config W1_MASTER_AMD
> This looks oddly places. Isn't entry above 'S', so A should go before?
> The rule is for entire Linux kernel: do not stuff things to the end of
> the lists.
There doesnt appear to be a specific alphabetical ordering taking place 
in this Kconfig:

config W1_MASTER_MATROX
config W1_MASTER_DS2490
config W1_MASTER_DS2482
config W1_MASTER_MXC
config W1_MASTER_GPIO
config HDQ_MASTER_OMAP
config W1_MASTER_SGI
config W1_MASTER_AMD

I've moved AMD to the top of Kconfig for v2.  If appropriate, please 
advise and I'll send in a pair of patches for Makefile and Kconfig to 
reorder alpahbetically separate to this patch set.

>> +     tristate "AMD AXI 1-wire bus master"
>> +     help
>> +       Say Y here is you want to support the AMD AXI 1-wire IP core.
>> +       This driver makes use of the programmable logic IP to perform
>> +       correctly timed 1 wire transactions without relying on GPIO timing
>> +       through the kernel.
>> +
>> +       This driver can also be built as a module.  If so, the module will be
>> +       called amd_w1.
>> +
>>   endmenu
>>
>> diff --git a/drivers/w1/masters/Makefile b/drivers/w1/masters/Makefile
>> index c5d85a827e52..cd3da1daaf97 100644
>> --- a/drivers/w1/masters/Makefile
>> +++ b/drivers/w1/masters/Makefile
>> @@ -11,3 +11,4 @@ obj-$(CONFIG_W1_MASTER_MXC)         += mxc_w1.o
>>   obj-$(CONFIG_W1_MASTER_GPIO)         += w1-gpio.o
>>   obj-$(CONFIG_HDQ_MASTER_OMAP)                += omap_hdq.o
>>   obj-$(CONFIG_W1_MASTER_SGI)          += sgi_w1.o
>> +obj-$(CONFIG_W1_MASTER_AMD)          += amd_w1.o
>> diff --git a/drivers/w1/masters/amd_w1.c b/drivers/w1/masters/amd_w1.c
>> new file mode 100644
>> index 000000000000..04bf08c1b6d7
>> --- /dev/null
>> +++ b/drivers/w1/masters/amd_w1.c
>> @@ -0,0 +1,422 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * amd_w1 - AMD 1Wire bus master driver
>> + *
>> + * Copyright (C) 2022-2023 Advanced Micro Devices, Inc. All Rights Reserved.
>> + */
>> +
>> +#include <linux/atomic.h>
>> +#include <linux/bitfield.h>
>> +#include <linux/clk.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/jiffies.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/types.h>
>> +#include <linux/wait.h>
>> +
>> +#include <linux/w1.h>
>> +
>> +/* 1-wire AMD IP definition */
>> +#define AXIW1_IPID   0x10ee4453
>> +/* Registers offset */
>> +#define AXIW1_INST_REG       0x0
>> +#define AXIW1_CTRL_REG       0x4
>> +#define AXIW1_IRQE_REG       0x8
>> +#define AXIW1_STAT_REG       0xC
>> +#define AXIW1_DATA_REG       0x10
>> +#define AXIW1_IPVER_REG      0x18
>> +#define AXIW1_IPID_REG       0x1C
>> +/* Instructions */
>> +#define AXIW1_INITPRES       0x0800
>> +#define AXIW1_READBIT        0x0C00
>> +#define AXIW1_WRITEBIT       0x0E00
>> +#define AXIW1_READBYTE       0x0D00
>> +#define AXIW1_WRITEBYTE      0x0F00
>> +/* Status flag masks */
>> +#define AXIW1_DONE   BIT(0)
>> +#define AXIW1_READY  BIT(4)
>> +#define AXIW1_PRESENCE       BIT(31)
>> +#define AXIW1_MAJORVER_MASK  GENMASK(23, 8)
>> +#define AXIW1_MINORVER_MASK  GENMASK(7, 0)
>> +/* Control flag */
>> +#define AXIW1_GO     BIT(0)
>> +#define AXI_CLEAR    0
>> +#define AXI_RESET    BIT(31)
>> +#define AXIW1_READDATA       BIT(0)
>> +/* Interrupt Enable */
>> +#define AXIW1_READY_IRQ_EN   BIT(4)
>> +#define AXIW1_DONE_IRQ_EN    BIT(0)
>> +
>> +#define AXIW1_TIMEOUT        msecs_to_jiffies(100)
>> +
>> +#define DRIVER_NAME  "amd_w1"
>> +
>> +struct amd_w1_local {
>> +     struct device *dev;
>> +     void __iomem *base_addr;
>> +     int irq;
>> +     atomic_t flag;
> Document what this flag does and what its purpose.
Done for v2
>> +     struct clk *clk;
> Why do you need this? It's never changed (after fixing the bug).
Removed in v2
>
>> +     wait_queue_head_t wait_queue;
>> +};
>> +
>> +/**
>> + * amd_w1_write_register() - Helper to write a hardware register.
>> + *
>> + * @amd_w1_local:    Pointer to device structure
>> + * @reg_offset:              Register offset in bytes to write
>> + * @val:             Value to write
>> + */
>> +static inline void amd_w1_write_register(struct amd_w1_local *amd_w1_local,
>> +                                      u8 reg_offset, u32 val)
>> +{
>> +     iowrite32(val, amd_w1_local->base_addr + reg_offset);
> Unwrapped code:
>    iowrite32(IRQ, amd_w1_local->base_addr + AXIW1_IRQE_REG);
>
> Your wrapper:
>    amd_w1_write_register(amd_w1_local, AXIW1_IRQE_REG, IRQ);
>
> Does not look simpler. If you want to use wrappers, they should actually
> help.
Unwrapped in v2
>
>> +};
>> +
>> +/**
>> + * amd_w1_read_register() - Helper to write a hardware register.
>> + *
>> + * @amd_w1_local:    Pointer to device structure
>> + * @reg_offset:              Register offset in bytes to write
>> + *
>> + * Return:           Value of register read
>> + */
>> +static inline u32 amd_w1_read_register(struct amd_w1_local *amd_w1_local, u8 reg_offset)
>> +{
>> +     return ioread32(amd_w1_local->base_addr + reg_offset);
>> +};
>> +
>> +/**
>> + * amd_w1_wait_irq_interruptible_timeout() - Wait for IRQ with timeout.
>> + *
>> + * @amd_w1_local:    Pointer to device structure
>> + * @IRQ:             IRQ channel to wait on
>> + *
>> + * Return:           %0 - OK, %-EINTR - Interrupted, %-EBUSY - Timed out
>> + */
>> +static inline int amd_w1_wait_irq_interruptible_timeout(struct amd_w1_local *amd_w1_local, u32 IRQ)
> Drop inline.
> This does not look like wrapped according to Linux coding convention.
> Please abide by the convention, so wrap at 80.
Inline dropped.  I've wrapped it in v2, albeit at column 84 as that is 
the first break point (comma).
>
>> +{
>> +     int ret;
>> +
>> +     /* Enable the IRQ requested and wait for flag to indicate it's been triggered */
>> +     amd_w1_write_register(amd_w1_local, AXIW1_IRQE_REG, IRQ);
>> +     ret = wait_event_interruptible_timeout(amd_w1_local->wait_queue,
>> +                                            atomic_read(&amd_w1_local->flag) != 0,
>> +                                            AXIW1_TIMEOUT);
>> +     if (ret < 0) {
>> +             dev_err(amd_w1_local->dev, "Wait IRQ Interrupted\n");
>> +             return -EINTR;
>> +     }
>> +
>> +     if (!ret) {
>> +             dev_err(amd_w1_local->dev, "Wait IRQ Timeout\n");
>> +             return -EBUSY;
>> +     }
>> +
>> +     /* Clear flag */
> Drop.
Dropped in v2
>
>> +     atomic_set(&amd_w1_local->flag, 0);
>> +     return 0;
>> +}
>> +
>> +/**
>> + * amd_w1_touch_bit() - Performs the touch-bit function, which writes a 0 or 1 and reads the level.
>> + *
>> + * @data:    Pointer to device structure
>> + * @bit:     The level to write
>> + *
>> + * Return:   The level read
>> + */
>> +static u8 amd_w1_touch_bit(void *data, u8 bit)
>> +{
>> +     struct amd_w1_local *amd_w1_local = data;
>> +     u8 val = 0;
>> +     int rc;
>> +
>> +     /* Wait for READY signal to be 1 to ensure 1-wire IP is ready */
>> +     while ((amd_w1_read_register(amd_w1_local, AXIW1_STAT_REG) & AXIW1_READY) == 0) {
>> +             rc = amd_w1_wait_irq_interruptible_timeout(amd_w1_local, AXIW1_READY_IRQ_EN);
>> +             if (rc < 0)
>> +                     return 1; /* Callee doesn't test for error. Return inactive bus state */
>> +     }
>> +
>> +     if (bit)
>> +             /* Read. Write read Bit command in register 0 */
>> +             amd_w1_write_register(amd_w1_local, AXIW1_INST_REG, AXIW1_READBIT);
>> +     else
>> +             /* Write. Write tx Bit command in instruction register with bit to transmit */
>> +             amd_w1_write_register(amd_w1_local, AXIW1_INST_REG,
>> +                                   (AXIW1_WRITEBIT + (bit & 0x01)));
>> +
>> +     /* Write Go signal and clear control reset signal in control register */
>> +     amd_w1_write_register(amd_w1_local, AXIW1_CTRL_REG, AXIW1_GO);
>> +
>> +     /* Wait for done signal to be 1 */
>> +     while ((amd_w1_read_register(amd_w1_local, AXIW1_STAT_REG) & AXIW1_DONE) != 1) {
>> +             rc = amd_w1_wait_irq_interruptible_timeout(amd_w1_local, AXIW1_DONE_IRQ_EN);
>> +             if (rc < 0)
>> +                     return 1; /* Callee doesn't test for error. Return inactive bus state */
>> +     }
>> +
>> +     /* If read, Retrieve data from register */
>> +     if (bit)
>> +             val = (u8)(amd_w1_read_register(amd_w1_local, AXIW1_DATA_REG) & AXIW1_READDATA);
>> +
>> +     /* Clear Go signal in register 1 */
>> +     amd_w1_write_register(amd_w1_local, AXIW1_CTRL_REG, AXI_CLEAR);
>> +
>> +     return val;
>> +}
>> +
>> +/**
>> + * amd_w1_read_byte - Performs the read byte function.
>> + *
>> + * @data:    Pointer to device structure
>> + * Return:   The value read
>> + */
>> +static u8 amd_w1_read_byte(void *data)
>> +{
>> +     struct amd_w1_local *amd_w1_local = data;
>> +     u8 val = 0;
>> +     int rc;
>> +
>> +     /* Wait for READY signal to be 1 to ensure 1-wire IP is ready */
>> +     while ((amd_w1_read_register(amd_w1_local, AXIW1_STAT_REG) & AXIW1_READY) == 0) {
>> +             rc = amd_w1_wait_irq_interruptible_timeout(amd_w1_local, AXIW1_READY_IRQ_EN);
>> +             if (rc < 0)
>> +                     return 0xFF; /* Return inactive bus state */
>> +     }
>> +
>> +     /* Write read Byte command in instruction register*/
>> +     amd_w1_write_register(amd_w1_local, AXIW1_INST_REG, AXIW1_READBYTE);
>> +     /* Write Go signal and clear control reset signal in control register */
>> +     amd_w1_write_register(amd_w1_local, AXIW1_CTRL_REG, AXIW1_GO);
>> +
>> +     /* Wait for done signal to be 1 */
>> +     while ((amd_w1_read_register(amd_w1_local, AXIW1_STAT_REG) & AXIW1_DONE) != 1) {
>> +             rc = amd_w1_wait_irq_interruptible_timeout(amd_w1_local, AXIW1_DONE_IRQ_EN);
>> +             if (rc < 0)
>> +                     return 0xFF; /* Return inactive bus state */
>> +     }
>> +
>> +     /* Retrieve LSB bit in data register to get RX byte */
>> +     val = (u8)(amd_w1_read_register(amd_w1_local, AXIW1_DATA_REG) & 0x000000FF);
>> +
>> +     /* Clear Go signal in control register */
>> +     amd_w1_write_register(amd_w1_local, AXIW1_CTRL_REG, AXI_CLEAR);
>> +
>> +     return val;
>> +}
>> +
>> +/**
>> + * amd_w1_write_byte - Performs the write byte function.
>> + *
>> + * @data:    The ds2482 channel pointer
>> + * @val:     The value to write
>> + */
>> +static void amd_w1_write_byte(void *data, u8 val)
>> +{
>> +     struct amd_w1_local *amd_w1_local = data;
>> +     int rc;
>> +
>> +     /* Wait for READY signal to be 1 to ensure 1-wire IP is ready */
>> +     while ((amd_w1_read_register(amd_w1_local, AXIW1_STAT_REG) & AXIW1_READY) == 0) {
>> +             rc = amd_w1_wait_irq_interruptible_timeout(amd_w1_local, AXIW1_READY_IRQ_EN);
>> +             if (rc < 0)
>> +                     return;
>> +     }
>> +
>> +     /* Write tx Byte command in instruction register with bit to transmit */
>> +     amd_w1_write_register(amd_w1_local, AXIW1_INST_REG, (AXIW1_WRITEBYTE + val));
>> +     /* Write Go signal and clear control reset signal in register 1 */
>> +     amd_w1_write_register(amd_w1_local, AXIW1_CTRL_REG, AXIW1_GO);
>> +
>> +     /* Wait for done signal to be 1 */
>> +     while ((amd_w1_read_register(amd_w1_local, AXIW1_STAT_REG) & AXIW1_DONE) != 1) {
>> +             rc = amd_w1_wait_irq_interruptible_timeout(amd_w1_local, AXIW1_DONE_IRQ_EN);
>> +             if (rc < 0)
>> +                     return;
>> +     }
>> +
>> +     /* Clear Go signal in control register */
>> +     amd_w1_write_register(amd_w1_local, AXIW1_CTRL_REG, AXI_CLEAR);
>> +}
>> +
>> +/**
>> + * amd_w1_reset_bus() - Issues a reset bus sequence.
>> + *
>> + * @data:    the bus master data struct
>> + * Return:   0=Device present, 1=No device present or error
>> + */
>> +static u8 amd_w1_reset_bus(void *data)
>> +{
>> +     struct amd_w1_local *amd_w1_local = data;
>> +     u8 val = 0;
>> +     int rc;
>> +
>> +     /* Reset 1-wire Axi IP */
>> +     amd_w1_write_register(amd_w1_local, AXIW1_CTRL_REG, AXI_RESET);
>> +
>> +     /* Wait for READY signal to be 1 to ensure 1-wire IP is ready */
>> +     while ((amd_w1_read_register(amd_w1_local, AXIW1_STAT_REG) & AXIW1_READY) == 0) {
>> +             rc = amd_w1_wait_irq_interruptible_timeout(amd_w1_local, AXIW1_READY_IRQ_EN);
>> +             if (rc < 0)
>> +                     return 1; /* Something went wrong with the hardware */
>> +     }
>> +     /* Write Initialization command in instruction register */
>> +     amd_w1_write_register(amd_w1_local, AXIW1_INST_REG, AXIW1_INITPRES);
>> +     /* Write Go signal and clear control reset signal in register 1 */
>> +     amd_w1_write_register(amd_w1_local, AXIW1_CTRL_REG, AXIW1_GO);
>> +
>> +     /* Wait for done signal to be 1 */
>> +     while ((amd_w1_read_register(amd_w1_local, AXIW1_STAT_REG) & AXIW1_DONE) != 1) {
>> +             rc = amd_w1_wait_irq_interruptible_timeout(amd_w1_local, AXIW1_DONE_IRQ_EN);
>> +             if (rc < 0)
>> +                     return 1; /* Something went wrong with the hardware */
>> +     }
>> +     /* Retrieve MSB bit in status register to get failure bit */
>> +     if ((amd_w1_read_register(amd_w1_local, AXIW1_STAT_REG) & AXIW1_PRESENCE) != 0)
>> +             val = 1;
>> +
>> +     /* Clear Go signal in control register */
>> +     amd_w1_write_register(amd_w1_local, AXIW1_CTRL_REG, AXI_CLEAR);
>> +
>> +     return val;
>> +}
>> +
>> +/* 1-wire master structure */
>> +static struct w1_bus_master amd_w1_master = {
> And how does it work with two devices?
Thanks - we've got a 2x instance design running and I've reproduced an 
issue with rmmod on cleanup when both are active.  Fixed and will submit 
in v2.
>
>> +     .touch_bit      = amd_w1_touch_bit,
>> +     .read_byte      = amd_w1_read_byte,
>> +     .write_byte     = amd_w1_write_byte,
>> +     .reset_bus      = amd_w1_reset_bus,
>> +};
>> +
>> +/* Reset the 1-wire AXI IP. Put the IP in reset state and clear registers */
>> +static void amd_w1_reset(struct amd_w1_local *amd_w1_local)
>> +{
>> +     amd_w1_write_register(amd_w1_local, AXIW1_CTRL_REG, AXI_RESET);
>> +     amd_w1_write_register(amd_w1_local, AXIW1_INST_REG, AXI_CLEAR);
>> +     amd_w1_write_register(amd_w1_local, AXIW1_IRQE_REG, AXI_CLEAR);
>> +     amd_w1_write_register(amd_w1_local, AXIW1_STAT_REG, AXI_CLEAR);
>> +     amd_w1_write_register(amd_w1_local, AXIW1_DATA_REG, AXI_CLEAR);
>> +}
>> +
>> +static irqreturn_t amd_w1_irq(int irq, void *lp)
>> +{
>> +     struct amd_w1_local *amd_w1_local = lp;
>> +
>> +     /* Clear enables in IRQ enable register */
> I don't understand this comment.
Reworded in v2 to "Reset interrupt trigger"
>
>> +     amd_w1_write_register(amd_w1_local, AXIW1_IRQE_REG, AXI_CLEAR);
>> +
>> +     /* Wake up the waiting queue */
>
> Drop obvious comments.
Done in v2
>
>> +     atomic_set(&amd_w1_local->flag, 1);
>> +     wake_up_interruptible(&amd_w1_local->wait_queue);
>> +
>> +     return IRQ_HANDLED;
>> +}
>> +
>> +static int amd_w1_probe(struct platform_device *pdev)
>> +{
>> +     struct device *dev = &pdev->dev;
>> +     struct amd_w1_local *lp;
>> +     u32 ver_major, ver_minor;
>> +     int val, rc = 0;
>> +
>> +     /* Get iospace for the device */
> This is memory allocation, not IO space.
Agree - Dropped in v2 with other obvious comments
>> +     lp = devm_kzalloc(dev, sizeof(*lp), GFP_KERNEL);
>> +     if (!lp)
>> +             return -ENOMEM;
>> +
>> +     lp->dev = dev;
>> +     lp->base_addr = devm_platform_ioremap_resource(pdev, 0);
>> +     if (IS_ERR(lp->base_addr))
>> +             return PTR_ERR(lp->base_addr);
>> +
>> +     /* Get IRQ for the device */
> Drop obvious comments.
Dropped in v2
>
>> +     lp->irq = platform_get_irq(pdev, 0);
>> +     if (lp->irq <= 0)
> Open platform_get_irq() function and read the help. Error handling
> couldn't be more explicit there...
Changed in v2
>
>> +             return lp->irq;
>> +
>> +     rc = devm_request_irq(dev, lp->irq, &amd_w1_irq, IRQF_TRIGGER_HIGH, DRIVER_NAME, lp);
>> +     if (rc) {
>> +             dev_err(dev, "Could not allocate interrupt %d.\n", lp->irq);
> return dev_err_probe(). But this leads us to second question: why would
> you notify about allocation errors? Core does it. Do you mean request?
I did mean request - removed dev_err for return in v2.
>> +             return rc;
>> +     }
>> +
>> +     /* Initialize wait queue and flag */
>> +     init_waitqueue_head(&lp->wait_queue);
>> +
>> +     lp->clk = devm_clk_get_enabled(dev, NULL);
>> +     if (IS_ERR(lp->clk))
>> +             return PTR_ERR(lp->clk);
>> +
>> +     /* Verify IP presence in HW */
>> +     if (amd_w1_read_register(lp, AXIW1_IPID_REG) != AXIW1_IPID) {
>> +             dev_err(dev, "AMD 1-wire IP not detected in hardware\n");
>> +             return -ENODEV;
>> +     }
>> +
>> +     /*
>> +      * Allow for future driver expansion supporting new hardware features
>> +      * This driver currently only supports hardware 1.x, but include logic
>> +      * to detect if a potentially incompatible future version is used
>> +      * by reading major version ID. It is highly undesirable for new IP versions
>> +      * to break the API, but this code will at least allow for graceful failure
>> +      * should that happen. Future new features can be enabled by hardware
>> +      * incrementing the minor version and augmenting the driver to detect capability
>> +      * using the minor version number
>> +      */
>> +     val = amd_w1_read_register(lp, AXIW1_IPVER_REG);
>> +     ver_major = FIELD_GET(AXIW1_MAJORVER_MASK, val);
>> +     ver_minor = FIELD_GET(AXIW1_MINORVER_MASK, val);
>> +
>> +     if (ver_major != 1) {
>> +             dev_err(dev, "AMD AXI W1 Master version %u.%u is not supported by this driver",
>> +                     ver_major, ver_minor);
>> +             return -ENODEV;
>> +     }
>> +
>> +     amd_w1_master.data = (void *)lp;
>> +     amd_w1_reset(lp);
>> +
>> +     platform_set_drvdata(pdev, lp);
>> +     rc = w1_add_master_device(&amd_w1_master);
>> +     if (rc) {
>> +             dev_err(dev, "Could not add master device\n");
>> +             amd_w1_reset(lp);
> Why? Does this mean that w1_add_master_device() changes the state of
> hardware?
No it doesnt - removed un-needed reset in v2.
>
>> +             return rc;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static void amd_w1_remove(struct platform_device *pdev)
>> +{
>> +     struct amd_w1_local *lp = platform_get_drvdata(pdev);
>> +
>> +     w1_remove_master_device(&amd_w1_master);
>> +     clk_disable_unprepare(lp->clk);
> This wasn't tested. Duplicated disable.
Thank you.  Bug reproduced with rmmod on module version of driver and 
resolved in v2.
>
>> +}
>> +
>> +static const struct of_device_id amd_w1_of_match[] = {
>> +     { .compatible = "amd,axi-1wire-master" },
>> +     { /* end of list */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, amd_w1_of_match);
>> +
>> +static struct platform_driver amd_w1_driver = {
>> +     .probe = amd_w1_probe,
>> +     .remove_new = amd_w1_remove,
>> +     .driver = {
>> +             .name = DRIVER_NAME,
>> +             .of_match_table = amd_w1_of_match,
>> +     },
>> +};
>> +module_platform_driver(amd_w1_driver);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("Kris Chaplin <kris.chaplin@amd.com>");
>> +MODULE_DESCRIPTION("Driver for AMD AXI 1 Wire IP core");
> Best regards,
> Krzysztof

Regards,
Kris

Re: [PATCH 2/2] w1: Add 1-wire master driver for AMD programmable logic IP Core

[Krzysztof Kozlowski]

On 18/10/2023 17:54, Kris Chaplin wrote:
> Thank you Krzysztof,
> 
> I shall post v2 tomorrow:
> 
> On 13/10/2023 16:20, Krzysztof Kozlowski wrote:
>> On 13/10/2023 11:30, Kris Chaplin wrote:
>>> Add a master driver to support the AMD 1-Wire programmable logic IP block.
>>> This block guarantees protocol timing for driving off-board devices such
>>> as thermal sensors, proms, etc.
>>>
>>> Add file to MAINTAINERS
>>>
>>> Co-developed-by: Thomas Delev <thomas.delev@amd.com>
>>> Signed-off-by: Thomas Delev <thomas.delev@amd.com>
>>> Signed-off-by: Kris Chaplin <kris.chaplin@amd.com>
>>> ---
>>>   MAINTAINERS                 |   1 +
>>>   drivers/w1/masters/Kconfig  |  11 +
>>>   drivers/w1/masters/Makefile |   1 +
>>>   drivers/w1/masters/amd_w1.c | 422 ++++++++++++++++++++++++++++++++++++
>>>   4 files changed, 435 insertions(+)
>>>   create mode 100644 drivers/w1/masters/amd_w1.c
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 6ec3922b256e..7f26dab5261b 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -1072,6 +1072,7 @@ R:      Thomas Delev <thomas.delev@amd.com>
>>>   R:   Michal Simek <michal.simek@amd.com>
>>>   S:   Maintained
>>>   F:   Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml
>>> +F:   drivers/w1/masters/amd_w1.c
>>>
>>>   AMD XGBE DRIVER
>>>   M:   "Shyam Sundar S K" <Shyam-sundar.S-k@amd.com>
>>> diff --git a/drivers/w1/masters/Kconfig b/drivers/w1/masters/Kconfig
>>> index ad316573288a..9232fd1f4e5b 100644
>>> --- a/drivers/w1/masters/Kconfig
>>> +++ b/drivers/w1/masters/Kconfig
>>> @@ -67,5 +67,16 @@ config W1_MASTER_SGI
>>>          This support is also available as a module.  If so, the module
>>>          will be called sgi_w1.
>>>
>>> +config W1_MASTER_AMD
>> This looks oddly places. Isn't entry above 'S', so A should go before?
>> The rule is for entire Linux kernel: do not stuff things to the end of
>> the lists.
> There doesnt appear to be a specific alphabetical ordering taking place 
> in this Kconfig:
> 
> config W1_MASTER_MATROX
> config W1_MASTER_DS2490
> config W1_MASTER_DS2482
> config W1_MASTER_MXC
> config W1_MASTER_GPIO
> config HDQ_MASTER_OMAP
> config W1_MASTER_SGI
> config W1_MASTER_AMD

I understand, happens. Still stuff should not be added to the end of lists.

Best regards,
Krzysztof

[PATCH v2 0/2] w1: Add AXI 1-wire host driver for AMD programmable logic IP core

[Kris Chaplin]

Changes since v1:
 Updated IP name and binding to axi-1wire-host and filenames to match
 Comment pruning where operation obvious, additional comments where not
 Unwrapped helper functions for register read/writes
 Removed un-necessary device reset on fail to add device
 Fixed duplicate clock disable in remove function
 Move bus master structure to per instance
 Improved hardware testing with multiple w1 instances

Add a host driver to support the AMD 1-Wire programmable logic IP block.
This block guarantees protocol timing for driving off-board devices such as thermal sensors, proms, etc.

Kris Chaplin (2):
  dt-bindings: w1: Add YAML DT schema for AMD AXI w1 host and
    MAINTAINERS entry
  w1: Add AXI 1-wire host driver for AMD programmable logic IP core

 .../bindings/w1/amd,axi-1wire-host.yaml       |  44 ++
 MAINTAINERS                                   |   8 +
 drivers/w1/masters/Kconfig                    |  11 +
 drivers/w1/masters/Makefile                   |   1 +
 drivers/w1/masters/amd_axi_w1.c               | 395 ++++++++++++++++++
 5 files changed, 459 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/w1/amd,axi-1wire-host.yaml
 create mode 100644 drivers/w1/masters/amd_axi_w1.c

-- 
2.42.GIT

[PATCH v2 1/2] dt-bindings: w1: Add YAML DT schema for AMD AXI w1 host and MAINTAINERS entry

[Kris Chaplin]

Add YAML DT schema for the AMD AXI w1 host IP.

This hardware guarantees protocol timing for driving off-board devices such
as thermal sensors, proms, etc using the 1wire protocol.

Add MAINTAINERS entry for DT schema.

Co-developed-by: Thomas Delev <thomas.delev@amd.com>
Signed-off-by: Thomas Delev <thomas.delev@amd.com>
Signed-off-by: Kris Chaplin <kris.chaplin@amd.com>
---
 .../bindings/w1/amd,axi-1wire-host.yaml       | 44 +++++++++++++++++++
 MAINTAINERS                                   |  7 +++
 2 files changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/w1/amd,axi-1wire-host.yaml

diff --git a/Documentation/devicetree/bindings/w1/amd,axi-1wire-host.yaml b/Documentation/devicetree/bindings/w1/amd,axi-1wire-host.yaml
new file mode 100644
index 000000000000..ef70fa2c0c5d
--- /dev/null
+++ b/Documentation/devicetree/bindings/w1/amd,axi-1wire-host.yaml
@@ -0,0 +1,44 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/w1/amd,axi-1wire-host.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AMD AXI 1-wire bus host for programmable logic
+
+maintainers:
+  - Kris Chaplin <kris.chaplin@amd.com>
+
+properties:
+  compatible:
+    const: amd,axi-1wire-host
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    onewire@a0000000 {
+        compatible = "amd,axi-1wire-host";
+        reg = <0xa0000000 0x10000>;
+        clocks = <&zynqmp_clk 0x47>;
+        interrupts = <GIC_SPI 0x59 IRQ_TYPE_LEVEL_HIGH>;
+    };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 7a7bd8bd80e9..3dacb7ed6e3b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -890,6 +890,13 @@ Q:	https://patchwork.kernel.org/project/linux-rdma/list/
 F:	drivers/infiniband/hw/efa/
 F:	include/uapi/rdma/efa-abi.h
 
+AMD AXI W1 DRIVER
+M:	Kris Chaplin <kris.chaplin@amd.com>
+R:	Thomas Delev <thomas.delev@amd.com>
+R:	Michal Simek <michal.simek@amd.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/w1/amd,axi-1wire-host.yaml
+
 AMD CDX BUS DRIVER
 M:	Nipun Gupta <nipun.gupta@amd.com>
 M:	Nikhil Agarwal <nikhil.agarwal@amd.com>
-- 
2.42.GIT

[PATCH v2 2/2] w1: Add AXI 1-wire host driver for AMD programmable logic IP core

[Kris Chaplin]

Add a host driver to support the AMD 1-Wire programmable logic IP block.
This block guarantees protocol timing for driving off-board devices such
as thermal sensors, proms, etc.

Add file to MAINTAINERS

Co-developed-by: Thomas Delev <thomas.delev@amd.com>
Signed-off-by: Thomas Delev <thomas.delev@amd.com>
Signed-off-by: Kris Chaplin <kris.chaplin@amd.com>
---
 MAINTAINERS                     |   1 +
 drivers/w1/masters/Kconfig      |  11 +
 drivers/w1/masters/Makefile     |   1 +
 drivers/w1/masters/amd_axi_w1.c | 395 ++++++++++++++++++++++++++++++++
 4 files changed, 408 insertions(+)
 create mode 100644 drivers/w1/masters/amd_axi_w1.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 3dacb7ed6e3b..c31b17df3be9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -896,6 +896,7 @@ R:	Thomas Delev <thomas.delev@amd.com>
 R:	Michal Simek <michal.simek@amd.com>
 S:	Maintained
 F:	Documentation/devicetree/bindings/w1/amd,axi-1wire-host.yaml
+F:	drivers/w1/masters/amd_axi_w1.c
 
 AMD CDX BUS DRIVER
 M:	Nipun Gupta <nipun.gupta@amd.com>
diff --git a/drivers/w1/masters/Kconfig b/drivers/w1/masters/Kconfig
index ad316573288a..513c0b114337 100644
--- a/drivers/w1/masters/Kconfig
+++ b/drivers/w1/masters/Kconfig
@@ -5,6 +5,17 @@
 
 menu "1-wire Bus Masters"
 
+config W1_MASTER_AMD_AXI
+	tristate "AMD AXI 1-wire bus host"
+	help
+	  Say Y here is you want to support the AMD AXI 1-wire IP core.
+	  This driver makes use of the programmable logic IP to perform
+	  correctly timed 1 wire transactions without relying on GPIO timing
+	  through the kernel.
+
+	  This driver can also be built as a module.  If so, the module will be
+	  called amd_w1_axi.
+
 config W1_MASTER_MATROX
 	tristate "Matrox G400 transport layer for 1-wire"
 	depends on PCI
diff --git a/drivers/w1/masters/Makefile b/drivers/w1/masters/Makefile
index c5d85a827e52..6c5a21f9b88c 100644
--- a/drivers/w1/masters/Makefile
+++ b/drivers/w1/masters/Makefile
@@ -3,6 +3,7 @@
 # Makefile for 1-wire bus master drivers.
 #
 
+obj-$(CONFIG_W1_MASTER_AMD_AXI)		+= amd_axi_w1.o
 obj-$(CONFIG_W1_MASTER_MATROX)		+= matrox_w1.o
 obj-$(CONFIG_W1_MASTER_DS2490)		+= ds2490.o
 obj-$(CONFIG_W1_MASTER_DS2482)		+= ds2482.o
diff --git a/drivers/w1/masters/amd_axi_w1.c b/drivers/w1/masters/amd_axi_w1.c
new file mode 100644
index 000000000000..24a05c2de5f1
--- /dev/null
+++ b/drivers/w1/masters/amd_axi_w1.c
@@ -0,0 +1,395 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * amd_axi_w1 - AMD 1Wire programmable logic bus host driver
+ *
+ * Copyright (C) 2022-2023 Advanced Micro Devices, Inc. All Rights Reserved.
+ */
+
+#include <linux/atomic.h>
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/jiffies.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/types.h>
+#include <linux/wait.h>
+
+#include <linux/w1.h>
+
+/* 1-wire AMD IP definition */
+#define AXIW1_IPID	0x10ee4453
+/* Registers offset */
+#define AXIW1_INST_REG	0x0
+#define AXIW1_CTRL_REG	0x4
+#define AXIW1_IRQE_REG	0x8
+#define AXIW1_STAT_REG	0xC
+#define AXIW1_DATA_REG	0x10
+#define AXIW1_IPVER_REG	0x18
+#define AXIW1_IPID_REG	0x1C
+/* Instructions */
+#define AXIW1_INITPRES	0x0800
+#define AXIW1_READBIT	0x0C00
+#define AXIW1_WRITEBIT	0x0E00
+#define AXIW1_READBYTE	0x0D00
+#define AXIW1_WRITEBYTE	0x0F00
+/* Status flag masks */
+#define AXIW1_DONE	BIT(0)
+#define AXIW1_READY	BIT(4)
+#define AXIW1_PRESENCE	BIT(31)
+#define AXIW1_MAJORVER_MASK	GENMASK(23, 8)
+#define AXIW1_MINORVER_MASK	GENMASK(7, 0)
+/* Control flag */
+#define AXIW1_GO	BIT(0)
+#define AXI_CLEAR	0
+#define AXI_RESET	BIT(31)
+#define AXIW1_READDATA	BIT(0)
+/* Interrupt Enable */
+#define AXIW1_READY_IRQ_EN	BIT(4)
+#define AXIW1_DONE_IRQ_EN	BIT(0)
+
+#define AXIW1_TIMEOUT	msecs_to_jiffies(100)
+
+#define DRIVER_NAME	"amd_axi_w1"
+
+struct amd_axi_w1_local {
+	struct device *dev;
+	void __iomem *base_addr;
+	int irq;
+	atomic_t flag;			/* Set on IRQ, cleared once serviced */
+	wait_queue_head_t wait_queue;
+	struct w1_bus_master bus_host;
+};
+
+/**
+ * amd_axi_w1_wait_irq_interruptible_timeout() - Wait for IRQ with timeout.
+ *
+ * @amd_axi_w1_local:	Pointer to device structure
+ * @IRQ:		IRQ channel to wait on
+ *
+ * Return:		%0 - OK, %-EINTR - Interrupted, %-EBUSY - Timed out
+ */
+static int amd_axi_w1_wait_irq_interruptible_timeout(struct amd_axi_w1_local *amd_axi_w1_local,
+						     u32 IRQ)
+{
+	int ret;
+
+	/* Enable the IRQ requested and wait for flag to indicate it's been triggered */
+	iowrite32(IRQ, amd_axi_w1_local->base_addr + AXIW1_IRQE_REG);
+	ret = wait_event_interruptible_timeout(amd_axi_w1_local->wait_queue,
+					       atomic_read(&amd_axi_w1_local->flag) != 0,
+					       AXIW1_TIMEOUT);
+	if (ret < 0) {
+		dev_err(amd_axi_w1_local->dev, "Wait IRQ Interrupted\n");
+		return -EINTR;
+	}
+
+	if (!ret) {
+		dev_err(amd_axi_w1_local->dev, "Wait IRQ Timeout\n");
+		return -EBUSY;
+	}
+
+	atomic_set(&amd_axi_w1_local->flag, 0);
+	return 0;
+}
+
+/**
+ * amd_axi_w1_touch_bit() - Performs the touch-bit function - write a 0 or 1 and reads the level.
+ *
+ * @data:	Pointer to device structure
+ * @bit:	The level to write
+ *
+ * Return:	The level read
+ */
+static u8 amd_axi_w1_touch_bit(void *data, u8 bit)
+{
+	struct amd_axi_w1_local *amd_axi_w1_local = data;
+	u8 val = 0;
+	int rc;
+
+	/* Wait for READY signal to be 1 to ensure 1-wire IP is ready */
+	while ((ioread32(amd_axi_w1_local->base_addr + AXIW1_STAT_REG) & AXIW1_READY) == 0) {
+		rc = amd_axi_w1_wait_irq_interruptible_timeout(amd_axi_w1_local,
+							       AXIW1_READY_IRQ_EN);
+		if (rc < 0)
+			return 1; /* Callee doesn't test for error. Return inactive bus state */
+	}
+
+	if (bit)
+		/* Read. Write read Bit command in register 0 */
+		iowrite32(AXIW1_READBIT, amd_axi_w1_local->base_addr + AXIW1_INST_REG);
+	else
+		/* Write. Write tx Bit command in instruction register with bit to transmit */
+		iowrite32(AXIW1_WRITEBIT + (bit & 0x01),
+			  amd_axi_w1_local->base_addr + AXIW1_INST_REG);
+
+	/* Write Go signal and clear control reset signal in control register */
+	iowrite32(AXIW1_GO, amd_axi_w1_local->base_addr + AXIW1_CTRL_REG);
+
+	/* Wait for done signal to be 1 */
+	while ((ioread32(amd_axi_w1_local->base_addr + AXIW1_STAT_REG) & AXIW1_DONE) != 1) {
+		rc = amd_axi_w1_wait_irq_interruptible_timeout(amd_axi_w1_local, AXIW1_DONE_IRQ_EN);
+		if (rc < 0)
+			return 1; /* Callee doesn't test for error. Return inactive bus state */
+	}
+
+	/* If read, Retrieve data from register */
+	if (bit)
+		val = (u8)(ioread32(amd_axi_w1_local->base_addr + AXIW1_DATA_REG) & AXIW1_READDATA);
+
+	/* Clear Go signal in register 1 */
+	iowrite32(AXI_CLEAR, amd_axi_w1_local->base_addr + AXIW1_CTRL_REG);
+
+	return val;
+}
+
+/**
+ * amd_axi_w1_read_byte - Performs the read byte function.
+ *
+ * @data:	Pointer to device structure
+ * Return:	The value read
+ */
+static u8 amd_axi_w1_read_byte(void *data)
+{
+	struct amd_axi_w1_local *amd_axi_w1_local = data;
+	u8 val = 0;
+	int rc;
+
+	/* Wait for READY signal to be 1 to ensure 1-wire IP is ready */
+	while ((ioread32(amd_axi_w1_local->base_addr + AXIW1_STAT_REG) & AXIW1_READY) == 0) {
+		rc = amd_axi_w1_wait_irq_interruptible_timeout(amd_axi_w1_local,
+							       AXIW1_READY_IRQ_EN);
+		if (rc < 0)
+			return 0xFF; /* Return inactive bus state */
+	}
+
+	/* Write read Byte command in instruction register*/
+	iowrite32(AXIW1_READBYTE, amd_axi_w1_local->base_addr + AXIW1_INST_REG);
+
+	/* Write Go signal and clear control reset signal in control register */
+	iowrite32(AXIW1_GO, amd_axi_w1_local->base_addr + AXIW1_CTRL_REG);
+
+	/* Wait for done signal to be 1 */
+	while ((ioread32(amd_axi_w1_local->base_addr + AXIW1_STAT_REG) & AXIW1_DONE) != 1) {
+		rc = amd_axi_w1_wait_irq_interruptible_timeout(amd_axi_w1_local, AXIW1_DONE_IRQ_EN);
+		if (rc < 0)
+			return 0xFF; /* Return inactive bus state */
+	}
+
+	/* Retrieve LSB bit in data register to get RX byte */
+	val = (u8)(ioread32(amd_axi_w1_local->base_addr + AXIW1_DATA_REG) & 0x000000FF);
+
+	/* Clear Go signal in control register */
+	iowrite32(AXI_CLEAR, amd_axi_w1_local->base_addr + AXIW1_CTRL_REG);
+
+	return val;
+}
+
+/**
+ * amd_axi_w1_write_byte - Performs the write byte function.
+ *
+ * @data:	The ds2482 channel pointer
+ * @val:	The value to write
+ */
+static void amd_axi_w1_write_byte(void *data, u8 val)
+{
+	struct amd_axi_w1_local *amd_axi_w1_local = data;
+	int rc;
+
+	/* Wait for READY signal to be 1 to ensure 1-wire IP is ready */
+	while ((ioread32(amd_axi_w1_local->base_addr + AXIW1_STAT_REG) & AXIW1_READY) == 0) {
+		rc = amd_axi_w1_wait_irq_interruptible_timeout(amd_axi_w1_local,
+							       AXIW1_READY_IRQ_EN);
+		if (rc < 0)
+			return;
+	}
+
+	/* Write tx Byte command in instruction register with bit to transmit */
+	iowrite32(AXIW1_WRITEBYTE + val, amd_axi_w1_local->base_addr + AXIW1_INST_REG);
+
+	/* Write Go signal and clear control reset signal in register 1 */
+	iowrite32(AXIW1_GO, amd_axi_w1_local->base_addr + AXIW1_CTRL_REG);
+
+	/* Wait for done signal to be 1 */
+	while ((ioread32(amd_axi_w1_local->base_addr + AXIW1_STAT_REG) & AXIW1_DONE) != 1) {
+		rc = amd_axi_w1_wait_irq_interruptible_timeout(amd_axi_w1_local,
+							       AXIW1_DONE_IRQ_EN);
+		if (rc < 0)
+			return;
+	}
+
+	/* Clear Go signal in control register */
+	iowrite32(AXI_CLEAR, amd_axi_w1_local->base_addr + AXIW1_CTRL_REG);
+}
+
+/**
+ * amd_axi_w1_reset_bus() - Issues a reset bus sequence.
+ *
+ * @data:	the bus host data struct
+ * Return:	0=Device present, 1=No device present or error
+ */
+static u8 amd_axi_w1_reset_bus(void *data)
+{
+	struct amd_axi_w1_local *amd_axi_w1_local = data;
+	u8 val = 0;
+	int rc;
+
+	/* Reset 1-wire Axi IP */
+	iowrite32(AXI_RESET, amd_axi_w1_local->base_addr + AXIW1_CTRL_REG);
+
+	/* Wait for READY signal to be 1 to ensure 1-wire IP is ready */
+	while ((ioread32(amd_axi_w1_local->base_addr + AXIW1_STAT_REG) & AXIW1_READY) == 0) {
+		rc = amd_axi_w1_wait_irq_interruptible_timeout(amd_axi_w1_local,
+							       AXIW1_READY_IRQ_EN);
+		if (rc < 0)
+			return 1; /* Something went wrong with the hardware */
+	}
+	/* Write Initialization command in instruction register */
+	iowrite32(AXIW1_INITPRES, amd_axi_w1_local->base_addr + AXIW1_INST_REG);
+
+	/* Write Go signal and clear control reset signal in register 1 */
+	iowrite32(AXIW1_GO, amd_axi_w1_local->base_addr + AXIW1_CTRL_REG);
+
+	/* Wait for done signal to be 1 */
+	while ((ioread32(amd_axi_w1_local->base_addr + AXIW1_STAT_REG) & AXIW1_DONE) != 1) {
+		rc = amd_axi_w1_wait_irq_interruptible_timeout(amd_axi_w1_local, AXIW1_DONE_IRQ_EN);
+		if (rc < 0)
+			return 1; /* Something went wrong with the hardware */
+	}
+	/* Retrieve MSB bit in status register to get failure bit */
+	if ((ioread32(amd_axi_w1_local->base_addr + AXIW1_STAT_REG) & AXIW1_PRESENCE) != 0)
+		val = 1;
+
+	/* Clear Go signal in control register */
+	iowrite32(AXI_CLEAR, amd_axi_w1_local->base_addr + AXIW1_CTRL_REG);
+
+	return val;
+}
+
+/* Reset the 1-wire AXI IP. Put the IP in reset state and clear registers */
+static void amd_axi_w1_reset(struct amd_axi_w1_local *amd_axi_w1_local)
+{
+	iowrite32(AXI_RESET, amd_axi_w1_local->base_addr + AXIW1_CTRL_REG);
+	iowrite32(AXI_CLEAR, amd_axi_w1_local->base_addr + AXIW1_INST_REG);
+	iowrite32(AXI_CLEAR, amd_axi_w1_local->base_addr + AXIW1_IRQE_REG);
+	iowrite32(AXI_CLEAR, amd_axi_w1_local->base_addr + AXIW1_STAT_REG);
+	iowrite32(AXI_CLEAR, amd_axi_w1_local->base_addr + AXIW1_DATA_REG);
+}
+
+static irqreturn_t amd_axi_w1_irq(int irq, void *lp)
+{
+	struct amd_axi_w1_local *amd_axi_w1_local = lp;
+
+	/* Reset interrupt trigger */
+	iowrite32(AXI_CLEAR, amd_axi_w1_local->base_addr + AXIW1_IRQE_REG);
+
+	atomic_set(&amd_axi_w1_local->flag, 1);
+	wake_up_interruptible(&amd_axi_w1_local->wait_queue);
+
+	return IRQ_HANDLED;
+}
+
+static int amd_axi_w1_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct amd_axi_w1_local *lp;
+	struct clk *clk;
+	u32 ver_major, ver_minor;
+	int val, rc = 0;
+
+	lp = devm_kzalloc(dev, sizeof(*lp), GFP_KERNEL);
+	if (!lp)
+		return -ENOMEM;
+
+	lp->dev = dev;
+	lp->base_addr = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(lp->base_addr))
+		return PTR_ERR(lp->base_addr);
+
+	lp->irq = platform_get_irq(pdev, 0);
+	if (lp->irq < 0)
+		return lp->irq;
+
+	rc = devm_request_irq(dev, lp->irq, &amd_axi_w1_irq, IRQF_TRIGGER_HIGH, DRIVER_NAME, lp);
+	if (rc)
+		return rc;
+
+	/* Initialize wait queue and flag */
+	init_waitqueue_head(&lp->wait_queue);
+
+	clk = devm_clk_get_enabled(dev, NULL);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	/* Verify IP presence in HW */
+	if (ioread32(lp->base_addr + AXIW1_IPID_REG) != AXIW1_IPID) {
+		dev_err(dev, "AMD 1-wire IP not detected in hardware\n");
+		return -ENODEV;
+	}
+
+	/*
+	 * Allow for future driver expansion supporting new hardware features
+	 * This driver currently only supports hardware 1.x, but include logic
+	 * to detect if a potentially incompatible future version is used
+	 * by reading major version ID. It is highly undesirable for new IP versions
+	 * to break the API, but this code will at least allow for graceful failure
+	 * should that happen. Future new features can be enabled by hardware
+	 * incrementing the minor version and augmenting the driver to detect capability
+	 * using the minor version number
+	 */
+	val = ioread32(lp->base_addr + AXIW1_IPVER_REG);
+	ver_major = FIELD_GET(AXIW1_MAJORVER_MASK, val);
+	ver_minor = FIELD_GET(AXIW1_MINORVER_MASK, val);
+
+	if (ver_major != 1) {
+		dev_err(dev, "AMD AXI W1 host version %u.%u is not supported by this driver",
+			ver_major, ver_minor);
+		return -ENODEV;
+	}
+
+	lp->bus_host.data = lp;
+	lp->bus_host.touch_bit = amd_axi_w1_touch_bit;
+	lp->bus_host.read_byte = amd_axi_w1_read_byte;
+	lp->bus_host.write_byte = amd_axi_w1_write_byte;
+	lp->bus_host.reset_bus = amd_axi_w1_reset_bus;
+
+	amd_axi_w1_reset(lp);
+
+	platform_set_drvdata(pdev, lp);
+	rc = w1_add_master_device(&lp->bus_host);
+	if (rc) {
+		dev_err(dev, "Could not add host device\n");
+		return rc;
+	}
+
+	return 0;
+}
+
+static void amd_axi_w1_remove(struct platform_device *pdev)
+{
+	struct amd_axi_w1_local *lp = platform_get_drvdata(pdev);
+
+	w1_remove_master_device(&lp->bus_host);
+}
+
+static const struct of_device_id amd_axi_w1_of_match[] = {
+	{ .compatible = "amd,axi-1wire-host" },
+	{ /* end of list */ },
+};
+MODULE_DEVICE_TABLE(of, amd_axi_w1_of_match);
+
+static struct platform_driver amd_axi_w1_driver = {
+	.probe = amd_axi_w1_probe,
+	.remove_new = amd_axi_w1_remove,
+	.driver = {
+		.name = DRIVER_NAME,
+		.of_match_table = amd_axi_w1_of_match,
+	},
+};
+module_platform_driver(amd_axi_w1_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Kris Chaplin <kris.chaplin@amd.com>");
+MODULE_DESCRIPTION("Driver for AMD AXI 1 Wire IP core");
-- 
2.42.GIT

Re: [PATCH v2 0/2] w1: Add AXI 1-wire host driver for AMD programmable logic IP core

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1571 bytes --]

On Thu, Oct 19, 2023 at 07:24:16AM -0700, Kris Chaplin wrote:
> Changes since v1:
>  Updated IP name and binding to axi-1wire-host and filenames to match
>  Comment pruning where operation obvious, additional comments where not
>  Unwrapped helper functions for register read/writes
>  Removed un-necessary device reset on fail to add device
>  Fixed duplicate clock disable in remove function
>  Move bus master structure to per instance
>  Improved hardware testing with multiple w1 instances
> 
> Add a host driver to support the AMD 1-Wire programmable logic IP block.
> This block guarantees protocol timing for driving off-board devices such as thermal sensors, proms, etc.

btw, please do not send a vN as a response to the v(N-1) patchset. It
ends up hiding things in the depths of people's mailboxes that sort by
threads.

Cheers,
Conor.

> 
> Kris Chaplin (2):
>   dt-bindings: w1: Add YAML DT schema for AMD AXI w1 host and
>     MAINTAINERS entry
>   w1: Add AXI 1-wire host driver for AMD programmable logic IP core
> 
>  .../bindings/w1/amd,axi-1wire-host.yaml       |  44 ++
>  MAINTAINERS                                   |   8 +
>  drivers/w1/masters/Kconfig                    |  11 +
>  drivers/w1/masters/Makefile                   |   1 +
>  drivers/w1/masters/amd_axi_w1.c               | 395 ++++++++++++++++++
>  5 files changed, 459 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/w1/amd,axi-1wire-host.yaml
>  create mode 100644 drivers/w1/masters/amd_axi_w1.c
> 
> -- 
> 2.42.GIT
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/2] dt-bindings: w1: Add YAML DT schema for AMD AXI w1 host and MAINTAINERS entry

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2927 bytes --]

On Thu, Oct 19, 2023 at 07:24:17AM -0700, Kris Chaplin wrote:
> Add YAML DT schema for the AMD AXI w1 host IP.
> 
> This hardware guarantees protocol timing for driving off-board devices such
> as thermal sensors, proms, etc using the 1wire protocol.
> 
> Add MAINTAINERS entry for DT schema.
> 
> Co-developed-by: Thomas Delev <thomas.delev@amd.com>
> Signed-off-by: Thomas Delev <thomas.delev@amd.com>
> Signed-off-by: Kris Chaplin <kris.chaplin@amd.com>
> ---
>  .../bindings/w1/amd,axi-1wire-host.yaml       | 44 +++++++++++++++++++
>  MAINTAINERS                                   |  7 +++
>  2 files changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/w1/amd,axi-1wire-host.yaml
> 
> diff --git a/Documentation/devicetree/bindings/w1/amd,axi-1wire-host.yaml b/Documentation/devicetree/bindings/w1/amd,axi-1wire-host.yaml
> new file mode 100644
> index 000000000000..ef70fa2c0c5d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/w1/amd,axi-1wire-host.yaml
> @@ -0,0 +1,44 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/w1/amd,axi-1wire-host.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: AMD AXI 1-wire bus host for programmable logic
> +
> +maintainers:
> +  - Kris Chaplin <kris.chaplin@amd.com>
> +
> +properties:
> +  compatible:
> +    const: amd,axi-1wire-host

I was happy with it before, and still am I guess, given that there is an
IP version register that can be used to determine which version of the
IP is in use.
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    onewire@a0000000 {
> +        compatible = "amd,axi-1wire-host";
> +        reg = <0xa0000000 0x10000>;
> +        clocks = <&zynqmp_clk 0x47>;
> +        interrupts = <GIC_SPI 0x59 IRQ_TYPE_LEVEL_HIGH>;
> +    };
> +
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7a7bd8bd80e9..3dacb7ed6e3b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -890,6 +890,13 @@ Q:	https://patchwork.kernel.org/project/linux-rdma/list/
>  F:	drivers/infiniband/hw/efa/
>  F:	include/uapi/rdma/efa-abi.h
>  
> +AMD AXI W1 DRIVER
> +M:	Kris Chaplin <kris.chaplin@amd.com>
> +R:	Thomas Delev <thomas.delev@amd.com>
> +R:	Michal Simek <michal.simek@amd.com>
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/w1/amd,axi-1wire-host.yaml
> +
>  AMD CDX BUS DRIVER
>  M:	Nipun Gupta <nipun.gupta@amd.com>
>  M:	Nikhil Agarwal <nikhil.agarwal@amd.com>
> -- 
> 2.42.GIT
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/2] dt-bindings: w1: Add YAML DT schema for AMD AXI w1 host and MAINTAINERS entry

[Michal Simek]

On 10/19/23 16:30, Conor Dooley wrote:
> On Thu, Oct 19, 2023 at 07:24:17AM -0700, Kris Chaplin wrote:
>> Add YAML DT schema for the AMD AXI w1 host IP.
>>
>> This hardware guarantees protocol timing for driving off-board devices such
>> as thermal sensors, proms, etc using the 1wire protocol.
>>
>> Add MAINTAINERS entry for DT schema.
>>
>> Co-developed-by: Thomas Delev <thomas.delev@amd.com>
>> Signed-off-by: Thomas Delev <thomas.delev@amd.com>
>> Signed-off-by: Kris Chaplin <kris.chaplin@amd.com>
>> ---
>>   .../bindings/w1/amd,axi-1wire-host.yaml       | 44 +++++++++++++++++++
>>   MAINTAINERS                                   |  7 +++
>>   2 files changed, 51 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/w1/amd,axi-1wire-host.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/w1/amd,axi-1wire-host.yaml b/Documentation/devicetree/bindings/w1/amd,axi-1wire-host.yaml
>> new file mode 100644
>> index 000000000000..ef70fa2c0c5d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/w1/amd,axi-1wire-host.yaml
>> @@ -0,0 +1,44 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/w1/amd,axi-1wire-host.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: AMD AXI 1-wire bus host for programmable logic
>> +
>> +maintainers:
>> +  - Kris Chaplin <kris.chaplin@amd.com>
>> +
>> +properties:
>> +  compatible:
>> +    const: amd,axi-1wire-host
> 
> I was happy with it before, and still am I guess, given that there is an
> IP version register that can be used to determine which version of the
> IP is in use.
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

I think we discussed that already that identification register should be 
described as the part of commit message to make this clear.

Thanks,
Michal

Re: [PATCH v2 1/2] dt-bindings: w1: Add YAML DT schema for AMD AXI w1 host and MAINTAINERS entry

[Kris Chaplin]

Thank you Connor and Michal,
>> I was happy with it before, and still am I guess, given that there is an
>> IP version register that can be used to determine which version of the
>> IP is in use.
>> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
>
> I think we discussed that already that identification register should 
> be described as the part of commit message to make this clear.
>
Yes we did - thanks for the reminder, and my bad for the omission.