[PATCH anybus v3 0/6] Support HMS Profinet Card over Anybus

[PATCH anybus v3 0/6] Support HMS Profinet Card over Anybus

[thesven73]

From: Sven Van Asbroeck <svendev@arcx.com>

This patch set adds support for the HMS Industrial Networks AB Profinet card.

Profinet is an industry technical standard for data communication over
Industrial Ethernet, designed for collecting data from, and controlling,
equipment in industrial systems, with a particular strength in delivering data
under tight time constraints (on the order of 1ms or less).

The profinet card itself is connected to the system via an industrial bus
called 'anybus'.

I have followed the bus driver/client driver pattern, and created an anybus
bus driver, plus a client driver for the profinet card.

In case this patch set gets (eventually) accepted, drivers for other anybus
client cards may follow: flnet, cc-link, ...

The anybus slot on the host is located on an 'anybus bridge', which is
custom h/w designed by arcx. Its driver is modeled as a misc device, which
exposes a dual reset controller, plus a power readout unrelated to the anybus.

v3:
	devicetree-bindings
		adding the vendor prefix is now a separate commit
	anybus-bridge:
		moved misc driver to drivers/bus/
		converted of_gpio_* to gpiod_* abstractions
		can power readout is now a fixed-voltage regulator
	anybuss-host:
		converted refcounts from atomic_t to refcount_t
		fixed potential use-after-free
	hms-profinet:
		applied minor kernel build robot suggestion

v2:
	added architecture overview comments to host driver
	completely reworked anybus-bridge driver, it becomes a reset controller
	anybuss-host driver now needs devicetree entry, link to reset controller
	I will hold off on kernel-doc until the overall architecture gets
		more validation / approval
	fixed Kconfig, comment-style, document ioctl magic numbers
	removed redundant pwm dependency
	renamed enable-gpios to reset-gpios
	stop driving reset-gpio after unloading driver
	use interrupt-parent / interrupts method to describe interrupts
		in the devicetree
	convert references 'i.MX WEIM parallel bus' to 'parallel bus'
	replace devicetree functions with more generic platform_get_resource()
							platform_get_irq()
	added device unique data to add_device_randomness()

v1:
	first shot

Sven Van Asbroeck (6):
  misc: support the Arcx anybus bridge
  dt-bindings: Add vendor prefix for arcx / Archronix
  dt-bindings: anybus-bridge: document devicetree binding
  bus: support HMS Anybus-S bus
  dt-bindings: anybuss-host: document devicetree binding
  misc: support HMS Profinet IRT industrial controller

 .../bindings/bus/arcx,anybuss-host.txt        |   36 +
 .../bindings/misc/arcx,anybus-bridge.txt      |   34 +
 .../devicetree/bindings/vendor-prefixes.txt   |    1 +
 Documentation/ioctl/ioctl-number.txt          |    1 +
 drivers/bus/Kconfig                           |   20 +
 drivers/bus/Makefile                          |    2 +
 drivers/bus/anybus-bridge.c                   |  321 ++++
 drivers/bus/anybuss-host.c                    | 1503 +++++++++++++++++
 drivers/misc/Kconfig                          |   10 +
 drivers/misc/Makefile                         |    1 +
 drivers/misc/hms-profinet.c                   |  753 +++++++++
 include/linux/anybuss-client.h                |  100 ++
 include/uapi/linux/hms-common.h               |   14 +
 include/uapi/linux/hms-profinet.h             |  102 ++
 14 files changed, 2898 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/bus/arcx,anybuss-host.txt
 create mode 100644 Documentation/devicetree/bindings/misc/arcx,anybus-bridge.txt
 create mode 100644 drivers/bus/anybus-bridge.c
 create mode 100644 drivers/bus/anybuss-host.c
 create mode 100644 drivers/misc/hms-profinet.c
 create mode 100644 include/linux/anybuss-client.h
 create mode 100644 include/uapi/linux/hms-common.h
 create mode 100644 include/uapi/linux/hms-profinet.h

-- 
2.17.1

[PATCH anybus v3 1/6] misc: support the Arcx anybus bridge

[thesven73]

From: Sven Van Asbroeck <svendev@arcx.com>

Add a driver for the Arcx anybus bridge.

This chip embeds up to two Anybus-S application connectors
(slots), and connects to the SoC via a parallel memory bus.
There is also a CAN power readout, unrelated to the Anybus,
modelled as a regulator.

Signed-off-by: Sven Van Asbroeck <svendev@arcx.com>
---
 drivers/bus/Kconfig         |  10 ++
 drivers/bus/Makefile        |   1 +
 drivers/bus/anybus-bridge.c | 321 ++++++++++++++++++++++++++++++++++++
 3 files changed, 332 insertions(+)
 create mode 100644 drivers/bus/anybus-bridge.c

diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
index 1851112ccc29..f29131529ddb 100644
--- a/drivers/bus/Kconfig
+++ b/drivers/bus/Kconfig
@@ -173,6 +173,16 @@ config VEXPRESS_CONFIG
 	  Platform configuration infrastructure for the ARM Ltd.
 	  Versatile Express.
 
+config ARCX_ANYBUS_BRIDGE
+	tristate "Arcx Anybus-S Bridge"
+	depends on OF && GPIOLIB
+	help
+	  Select this to get support for the Arcx Anybus bridge.
+	  It connects to the SoC via a parallel memory bus, and
+	  embeds up to two Anybus-S application connectors (slots).
+	  There is also a CAN power readout, unrelated to the Anybus,
+	  modelled as a regulator.
+
 config DA8XX_MSTPRI
 	bool "TI da8xx master peripheral priority driver"
 	depends on ARCH_DAVINCI_DA8XX
diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
index ca300b1914ce..24fa2cde0692 100644
--- a/drivers/bus/Makefile
+++ b/drivers/bus/Makefile
@@ -30,5 +30,6 @@ obj-$(CONFIG_TI_SYSC)		+= ti-sysc.o
 obj-$(CONFIG_TS_NBUS)		+= ts-nbus.o
 obj-$(CONFIG_UNIPHIER_SYSTEM_BUS)	+= uniphier-system-bus.o
 obj-$(CONFIG_VEXPRESS_CONFIG)	+= vexpress-config.o
+obj-$(CONFIG_ARCX_ANYBUS_BRIDGE)	+= anybus-bridge.o
 
 obj-$(CONFIG_DA8XX_MSTPRI)	+= da8xx-mstpri.o
diff --git a/drivers/bus/anybus-bridge.c b/drivers/bus/anybus-bridge.c
new file mode 100644
index 000000000000..9eee39efc3aa
--- /dev/null
+++ b/drivers/bus/anybus-bridge.c
@@ -0,0 +1,321 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Arcx Anybus Bridge driver
+ *
+ * Copyright (C) 2018 Arcx Inc
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/delay.h>
+#include <linux/idr.h>
+#include <linux/spinlock.h>
+#include <linux/reset-controller.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+
+#define CPLD_STATUS1		0x80
+#define CPLD_CONTROL		0x80
+#define CPLD_CONTROL_CRST	0x40
+#define CPLD_CONTROL_RST1	0x04
+#define CPLD_CONTROL_RST2	0x80
+#define CPLD_STATUS1_AB		0x02
+#define CPLD_STATUS1_CAN_POWER	0x01
+#define CPLD_DESIGN_LO		0x81
+#define CPLD_DESIGN_HI		0x82
+#define CPLD_CAP		0x83
+#define CPLD_CAP_COMPAT		0x01
+#define CPLD_CAP_SEP_RESETS	0x02
+
+struct bridge_priv {
+	struct device *class_dev;
+	struct reset_controller_dev rcdev;
+	bool common_reset;
+	struct gpio_desc *reset_gpiod;
+	void __iomem *cpld_base;
+	spinlock_t regs_lock;
+	u8 control_reg;
+	char version[3];
+	u16 design_no;
+};
+
+static void do_reset(struct bridge_priv *cd, u8 rst_bit, bool reset)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&cd->regs_lock, flags);
+	/*
+	 * CPLD_CONTROL is write-only, so cache its value in
+	 * cd->control_reg
+	 */
+	if (reset)
+		cd->control_reg &= ~rst_bit;
+	else
+		cd->control_reg |= rst_bit;
+	writeb(cd->control_reg, cd->cpld_base + CPLD_CONTROL);
+	/*
+	 * h/w work-around:
+	 * the hardware is 'too fast', so a reset followed by an immediate
+	 * not-reset will _not_ change the anybus reset line in any way,
+	 * losing the reset. to prevent this from happening, introduce
+	 * a minimum reset duration.
+	 * Verified minimum safe duration required using a scope
+	 * on 14-June-2018: 100 us.
+	 */
+	if (reset)
+		udelay(100);
+	spin_unlock_irqrestore(&cd->regs_lock, flags);
+}
+
+static int anybuss_reset(struct bridge_priv *cd,
+			     unsigned long id, bool reset)
+{
+	if (id >= 2)
+		return -EINVAL;
+	if (cd->common_reset)
+		do_reset(cd, CPLD_CONTROL_CRST, reset);
+	else
+		do_reset(cd, id ? CPLD_CONTROL_RST2 : CPLD_CONTROL_RST1, reset);
+	return 0;
+}
+
+static int anybuss_reset_assert(struct reset_controller_dev *rcdev,
+			     unsigned long id)
+{
+	struct bridge_priv *cd = container_of(rcdev, struct bridge_priv, rcdev);
+
+	return anybuss_reset(cd, id, true);
+}
+
+static int anybuss_reset_deassert(struct reset_controller_dev *rcdev,
+			     unsigned long id)
+{
+	struct bridge_priv *cd = container_of(rcdev, struct bridge_priv, rcdev);
+
+	return anybuss_reset(cd, id, false);
+}
+
+static const struct reset_control_ops anybuss_reset_ops = {
+	.assert		= anybuss_reset_assert,
+	.deassert	= anybuss_reset_deassert,
+};
+
+static ssize_t version_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct bridge_priv *cd = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%s\n", cd->version);
+}
+static DEVICE_ATTR_RO(version);
+
+static ssize_t design_number_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct bridge_priv *cd = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%d\n", cd->design_no);
+}
+static DEVICE_ATTR_RO(design_number);
+
+static struct attribute *bridge_attributes[] = {
+	&dev_attr_version.attr,
+	&dev_attr_design_number.attr,
+	NULL,
+};
+
+static struct attribute_group bridge_attribute_group = {
+	.attrs = bridge_attributes,
+};
+
+static const struct attribute_group *bridge_attribute_groups[] = {
+	&bridge_attribute_group,
+	NULL,
+};
+
+static void bridge_device_release(struct device *dev)
+{
+	kfree(dev);
+}
+
+static int can_power_is_enabled(struct regulator_dev *rdev)
+{
+	struct bridge_priv *cd = rdev_get_drvdata(rdev);
+
+	return !(readb(cd->cpld_base + CPLD_STATUS1) & CPLD_STATUS1_CAN_POWER);
+}
+
+static struct regulator_ops can_power_ops = {
+	.is_enabled = can_power_is_enabled,
+};
+
+static const struct regulator_desc can_power_desc = {
+	.name = "regulator-can-power",
+	.id = -1,
+	.type = REGULATOR_VOLTAGE,
+	.owner = THIS_MODULE,
+	.ops = &can_power_ops,
+};
+
+static struct class *bridge_class;
+static DEFINE_IDA(bridge_index_ida);
+
+static int bridge_probe(struct platform_device *pdev)
+{
+	struct bridge_priv *cd;
+	struct device *dev = &pdev->dev;
+	struct regulator_config config = { };
+	struct regulator_dev *regulator;
+	int err, id;
+	struct resource *res;
+	u8 status1, cap;
+
+	cd = devm_kzalloc(dev, sizeof(*cd), GFP_KERNEL);
+	if (!cd)
+		return -ENOMEM;
+	dev_set_drvdata(dev, cd);
+	spin_lock_init(&cd->regs_lock);
+	cd->reset_gpiod = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(cd->reset_gpiod))
+		return PTR_ERR(cd->reset_gpiod);
+
+	/* CPLD control memory, sits at index 0 */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	cd->cpld_base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(cd->cpld_base)) {
+		dev_err(dev,
+			"failed to map cpld base address\n");
+		err = PTR_ERR(cd->cpld_base);
+		goto out_reset;
+	}
+
+	/* identify cpld */
+	status1 = readb(cd->cpld_base + CPLD_STATUS1);
+	cd->design_no = (readb(cd->cpld_base + CPLD_DESIGN_HI) << 8) |
+				readb(cd->cpld_base + CPLD_DESIGN_LO);
+	snprintf(cd->version, sizeof(cd->version), "%c%d",
+			'A' + ((status1>>5) & 0x7),
+			(status1>>2) & 0x7);
+	dev_info(dev, "Bridge is design number %d, revision %s\n",
+		cd->design_no,
+		cd->version);
+	cap = readb(cd->cpld_base + CPLD_CAP);
+	if (!(cap & CPLD_CAP_COMPAT)) {
+		dev_err(dev, "unsupported bridge [cap=0x%02X]", cap);
+		err = -ENODEV;
+		goto out_reset;
+	}
+
+	if (status1 & CPLD_STATUS1_AB) {
+		dev_info(dev, "Bridge has anybus-S slot(s)");
+		cd->common_reset = !(cap & CPLD_CAP_SEP_RESETS);
+		dev_info(dev, "Bridge supports %s", cd->common_reset ?
+			"a common reset" : "separate resets");
+		cd->rcdev.owner	= THIS_MODULE;
+		cd->rcdev.nr_resets = 2;
+		cd->rcdev.ops = &anybuss_reset_ops;
+		cd->rcdev.of_node = dev->of_node;
+		err = devm_reset_controller_register(dev, &cd->rcdev);
+		if (err)
+			goto out_reset;
+	}
+
+	id = ida_simple_get(&bridge_index_ida, 0, 0, GFP_KERNEL);
+	if (id < 0) {
+		err = id;
+		goto out_reset;
+	}
+	/* export can power readout as a regulator */
+	config.dev = dev;
+	config.driver_data = cd;
+	regulator = devm_regulator_register(dev, &can_power_desc, &config);
+	if (IS_ERR(regulator)) {
+		err = PTR_ERR(regulator);
+		goto out_reset;
+	}
+	/* make bridge info visible to userspace */
+	cd->class_dev = kzalloc(sizeof(*cd->class_dev), GFP_KERNEL);
+	if (!cd->class_dev) {
+		err = -ENOMEM;
+		goto out_ida;
+	}
+	cd->class_dev->class = bridge_class;
+	cd->class_dev->groups = bridge_attribute_groups;
+	cd->class_dev->parent = dev;
+	cd->class_dev->id = id;
+	cd->class_dev->release = bridge_device_release;
+	dev_set_name(cd->class_dev, "bridge%d", cd->class_dev->id);
+	dev_set_drvdata(cd->class_dev, cd);
+	err = device_register(cd->class_dev);
+	if (err)
+		goto out_dev;
+	return 0;
+out_dev:
+	put_device(cd->class_dev);
+out_ida:
+	ida_simple_remove(&bridge_index_ida, id);
+out_reset:
+	gpiod_set_value_cansleep(cd->reset_gpiod, 1);
+	return err;
+}
+
+static int bridge_remove(struct platform_device *pdev)
+{
+	struct bridge_priv *cd = platform_get_drvdata(pdev);
+	int id = cd->class_dev->id;
+
+	device_unregister(cd->class_dev);
+	ida_simple_remove(&bridge_index_ida, id);
+	gpiod_set_value_cansleep(cd->reset_gpiod, 1);
+	return 0;
+}
+
+static const struct of_device_id bridge_of_match[] = {
+	{ .compatible = "arcx,anybus-bridge" },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(of, bridge_of_match);
+
+static struct platform_driver bridge_driver = {
+	.probe = bridge_probe,
+	.remove = bridge_remove,
+	.driver		= {
+		.name   = "arcx-anybus-bridge",
+		.owner	= THIS_MODULE,
+		.of_match_table	= of_match_ptr(bridge_of_match),
+	},
+};
+
+static int __init bridge_init(void)
+{
+	int err;
+
+	bridge_class = class_create(THIS_MODULE, "arcx_anybus_bridge");
+	if (!IS_ERR(bridge_class)) {
+		err = platform_driver_register(&bridge_driver);
+		if (err)
+			class_destroy(bridge_class);
+	} else
+		err = PTR_ERR(bridge_class);
+	return err;
+}
+
+static void __exit bridge_exit(void)
+{
+	platform_driver_unregister(&bridge_driver);
+	class_destroy(bridge_class);
+}
+
+module_init(bridge_init);
+module_exit(bridge_exit);
+
+MODULE_DESCRIPTION("Arcx Anybus Bridge driver");
+MODULE_AUTHOR("Sven Van Asbroeck <svendev@arcx.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

Re: [PATCH anybus v3 1/6] misc: support the Arcx anybus bridge

[Rob Herring]

On Sun, Nov 04, 2018 at 10:54:56AM -0500, thesven73@gmail.com wrote:
> From: Sven Van Asbroeck <svendev@arcx.com>
> 
> Add a driver for the Arcx anybus bridge.
> 
> This chip embeds up to two Anybus-S application connectors
> (slots), and connects to the SoC via a parallel memory bus.
> There is also a CAN power readout, unrelated to the Anybus,
> modelled as a regulator.

bridge vs. host are confusing me as those are often the same thing. But 
here bridge is just some auxilliary controls for the bus and the host is 
the actual bus with devices? I'm not sure why you split this into 2 DT 
nodes? How many devices are connected to the host processor (i.MX) bus?

Rob

Re: [PATCH anybus v3 1/6] misc: support the Arcx anybus bridge

[Sven Van Asbroeck]

On Mon, Nov 5, 2018 at 4:20 PM Rob Herring <robh@kernel.org> wrote:
>
> bridge vs. host are confusing me as those are often the same thing. But
> here bridge is just some auxilliary controls for the bus and the host is
> the actual bus with devices? I'm not sure why you split this into 2 DT
> nodes? How many devices are connected to the host processor (i.MX) bus?
>

The anybus can only have a single client card (device) on the bus. I
know, we can criticise the name, but that's how HMS Industrial
Networks designed it.

The anybus-bridge is how we (at arcx / Archronix) physically connect
anybus slots to the iMX. It is a CPLD chip connected to the i.MX WEIM
bus on the SoC side. On the other side of the CPLD, there are two
anybus slots. Each slot may hold an anybus card (profinet, flnet,
cc-link, ...)

The anybuss-host implements the anybus specification in a
platform-independent way. Some other company could attach anybus slots
to their CPU through a completely different bus, or spi, and they
could still attach the anybuss-host driver onto it. All the driver
needs is a regmap providing access to the anybus slot memory, an
interrupt, and a reset controller.

Sven

Re: [PATCH anybus v3 1/6] misc: support the Arcx anybus bridge

[Rob Herring]

On Mon, Nov 5, 2018 at 3:50 PM Sven Van Asbroeck <thesven73@gmail.com> wrote:
>
> On Mon, Nov 5, 2018 at 4:20 PM Rob Herring <robh@kernel.org> wrote:
> >
> > bridge vs. host are confusing me as those are often the same thing. But
> > here bridge is just some auxilliary controls for the bus and the host is
> > the actual bus with devices? I'm not sure why you split this into 2 DT
> > nodes? How many devices are connected to the host processor (i.MX) bus?
> >
>
> The anybus can only have a single client card (device) on the bus. I
> know, we can criticise the name, but that's how HMS Industrial
> Networks designed it.
>
> The anybus-bridge is how we (at arcx / Archronix) physically connect
> anybus slots to the iMX. It is a CPLD chip connected to the i.MX WEIM
> bus on the SoC side. On the other side of the CPLD, there are two
> anybus slots. Each slot may hold an anybus card (profinet, flnet,
> cc-link, ...)
>
> The anybuss-host implements the anybus specification in a
> platform-independent way. Some other company could attach anybus slots
> to their CPU through a completely different bus, or spi, and they
> could still attach the anybuss-host driver onto it. All the driver
> needs is a regmap providing access to the anybus slot memory, an
> interrupt, and a reset controller.

It doesn't really sound like the host should be in DT. The bridge
should register itself as an anybus provider and that should in turn
enable the anybus host protocol.

Rob

Re: [PATCH anybus v3 1/6] misc: support the Arcx anybus bridge

[Sven Van Asbroeck]

On Tue, Nov 6, 2018 at 8:58 AM Rob Herring <robh@kernel.org> wrote:

> It doesn't really sound like the host should be in DT. The bridge
> should register itself as an anybus provider and that should in turn
> enable the anybus host protocol.

Very good point. Just to make sure we're on the same page, could you point
me to a relevant example where something registers as a provider?

v1 of this patch did not have the host in DT. The host just required platform
data with a regmap and a reset (the interrupt was passed via resources):

struct anybuss_host_pdata {
    struct regmap *regmap;
    void (*reset)(struct device *dev, bool reset);
};

But there were problems with this approach.

The review feedback told me that my self-rolled reset callback should really
be a reset controller. I looked for ways to pass a handle to a reset controller
via platform data. This has recently been introduced via:

reset_controller_add_lookup()

This binds a client device to a reset controller, without using the devicetree,
so the device can grab its controller via (devm_)reset_control_get*. Great!
But... to make the binding, you have to specify the full device names of the
controllers and client devices. See this example from psc-da850.c:

static struct reset_control_lookup da850_psc0_reset_lookup_table[] = {
    RESET_LOOKUP("da850-psc0", 15, "davinci-rproc.0", NULL),
};

I very quickly found myself in ida_simple_get() hell, trying to second-guess
what the devices I was creating, would be called !

So instead I put the host in DT, then I could easily connect the reset
controller. This also greatly simplified the bridge driver, a lot of boilerplate
would simply disappear.

Suggestions are very welcome :)

Sven

Re: [PATCH anybus v3 1/6] misc: support the Arcx anybus bridge

[Rob Herring]

On Tue, Nov 6, 2018 at 8:46 AM Sven Van Asbroeck <thesven73@gmail.com> wrote:
>
> On Tue, Nov 6, 2018 at 8:58 AM Rob Herring <robh@kernel.org> wrote:
>
> > It doesn't really sound like the host should be in DT. The bridge
> > should register itself as an anybus provider and that should in turn
> > enable the anybus host protocol.
>
> Very good point. Just to make sure we're on the same page, could you point
> me to a relevant example where something registers as a provider?

Not sure exactly. Perhaps I2C SMBus functions that implement the
register access protocol on top of I2C bus.

> v1 of this patch did not have the host in DT. The host just required platform
> data with a regmap and a reset (the interrupt was passed via resources):
>
> struct anybuss_host_pdata {
>     struct regmap *regmap;
>     void (*reset)(struct device *dev, bool reset);
> };
>
> But there were problems with this approach.

A block diagram would help. Something that shows the host SoC, your
CPLD, reset, irq, etc.

> The review feedback told me that my self-rolled reset callback should really
> be a reset controller. I looked for ways to pass a handle to a reset controller
> via platform data. This has recently been introduced via:

Maybe an overkill for 1 reset.

>
> reset_controller_add_lookup()
>
> This binds a client device to a reset controller, without using the devicetree,
> so the device can grab its controller via (devm_)reset_control_get*. Great!
> But... to make the binding, you have to specify the full device names of the
> controllers and client devices. See this example from psc-da850.c:
>
> static struct reset_control_lookup da850_psc0_reset_lookup_table[] = {
>     RESET_LOOKUP("da850-psc0", 15, "davinci-rproc.0", NULL),
> };
>
> I very quickly found myself in ida_simple_get() hell, trying to second-guess
> what the devices I was creating, would be called !
>
> So instead I put the host in DT, then I could easily connect the reset
> controller. This also greatly simplified the bridge driver, a lot of boilerplate
> would simply disappear.

If the host is not a h/w component, but just a s/w protocol then it
doesn't belong in DT. Perhaps it could be a library which the bridge
driver can call into.

What are the resets connected to? The slots? Maybe you should model
the slots in DT.

Rob

Re: [PATCH anybus v3 1/6] misc: support the Arcx anybus bridge

[Sven Van Asbroeck]

On Tue, Nov 6, 2018 at 1:31 PM Rob Herring <robh@kernel.org> wrote:

> If the host is not a h/w component, but just a s/w protocol then it
> doesn't belong in DT. Perhaps it could be a library which the bridge
> driver can call into.

Anybus cards have an id register, which identifies what they are, so
that the appropriate client driver may be instantiated.
In that sense anybus is very suited to the bus/client abstraction of
pci/usb/etc.

> What are the resets connected to? The slots? Maybe you should model
> the slots in DT.
>

Yes, the resets are ultimately connected to the slots.
I'm happy to model the slots in DT. It makes sense, they are physical,
hardware components.

>
> A block diagram would help. Something that shows the host SoC, your
> CPLD, reset, irq, etc.
>

  +------------------------------------------------------------------------+
  |                                  SOC (i.MX6)                           |
  |------------------------------------------------------------------------|
  |     i.MX WEIM bus                                   | i.MX GPIO        |
  +------------------------------------------------------------------------+
                ^                                              ^ ^
                | parallel bus                                 | | irq x2
                v                                              | |
  +------------------------------------------------------------------------+
  |                                     CPLD                               |
  |------------------------------------------------------------------------|
  |             anybus slot 1             ||            anybus slot 2      |
  |------------------------------------------------------------------------|
  | memory bus               | irq| reset || memory bus       | irq| reset |
  +------------------------------------------------------------------------+
                ^                                               ^
                |                                               |
                v                                               v
  +--------------------------------------+ +-------------------------------+
  |          anybus card                 | |           anybus card         |
  +--------------------------------------+ +-------------------------------+

Re: [PATCH anybus v3 1/6] misc: support the Arcx anybus bridge

[Arnd Bergmann]

On Tue, Nov 6, 2018 at 9:05 PM Sven Van Asbroeck <thesven73@gmail.com> wrote:
> On Tue, Nov 6, 2018 at 1:31 PM Rob Herring <robh@kernel.org> wrote:
>
> > If the host is not a h/w component, but just a s/w protocol then it
> > doesn't belong in DT. Perhaps it could be a library which the bridge
> > driver can call into.
>
> Anybus cards have an id register, which identifies what they are, so
> that the appropriate client driver may be instantiated.
> In that sense anybus is very suited to the bus/client abstraction of
> pci/usb/etc.
>
> > What are the resets connected to? The slots? Maybe you should model
> > the slots in DT.
> >
>
> Yes, the resets are ultimately connected to the slots.
> I'm happy to model the slots in DT. It makes sense, they are physical,
> hardware components.

It may also be necessary to have a way to add more DT properties to
a device in the slot that can not be probed by looking at the ID
register. Ideally it should not be needed, but we have added this
to most buses anyway: USB, PCI, MMC and others can all be
probed automatically for many devices, but there are some devices
that need a place where extra information can be stored, e.g.
an ethernet MAC address (in the absence of a PROM), an
external clock or reset line that is not part of the normal protocol,
or a more specific identification when two devices accidentally
have the same ID.

       Arnd

[PATCH anybus v3 2/6] dt-bindings: Add vendor prefix for arcx / Archronix

[thesven73]

From: Sven Van Asbroeck <svendev@arcx.com>

arcx Inc. is an engineering company which provides advanced
embedded systems and consulting services.

Archronix is a technology design and product engineering firm
specializing in hardware control systems and enabling software.
Clients include OEM's in the transportation, aerospace,
medical and commercial sectors.

Websites:
	http://www.arcx.com/
	http://www.archronix.com/

Signed-off-by: Sven Van Asbroeck <svendev@arcx.com>
---
 Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 2c3fc512e746..2ad1128c4470 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -35,6 +35,7 @@ aptina	Aptina Imaging
 arasan	Arasan Chip Systems
 archermind ArcherMind Technology (Nanjing) Co., Ltd.
 arctic	Arctic Sand
+arcx	arcx Inc. / Archronix Inc.
 aries	Aries Embedded GmbH
 arm	ARM Ltd.
 armadeus	ARMadeus Systems SARL
-- 
2.17.1

Re: [PATCH anybus v3 2/6] dt-bindings: Add vendor prefix for arcx / Archronix

[Andreas Färber]

Am 04.11.18 um 16:54 schrieb thesven73@gmail.com:
> From: Sven Van Asbroeck <svendev@arcx.com>
> 
> arcx Inc. is an engineering company which provides advanced
> embedded systems and consulting services.
> 
> Archronix is a technology design and product engineering firm
> specializing in hardware control systems and enabling software.
> Clients include OEM's in the transportation, aerospace,
> medical and commercial sectors.
> 
> Websites:
> 	http://www.arcx.com/
> 	http://www.archronix.com/
> 
> Signed-off-by: Sven Van Asbroeck <svendev@arcx.com>

Reviewed-by: Andreas Färber <afaerber@suse.de>

Thanks,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH anybus v3 2/6] dt-bindings: Add vendor prefix for arcx / Archronix

[Rob Herring]

On Sun,  4 Nov 2018 10:54:57 -0500, thesven73@gmail.com wrote:
> From: Sven Van Asbroeck <svendev@arcx.com>
> 
> arcx Inc. is an engineering company which provides advanced
> embedded systems and consulting services.
> 
> Archronix is a technology design and product engineering firm
> specializing in hardware control systems and enabling software.
> Clients include OEM's in the transportation, aerospace,
> medical and commercial sectors.
> 
> Websites:
> 	http://www.arcx.com/
> 	http://www.archronix.com/
> 
> Signed-off-by: Sven Van Asbroeck <svendev@arcx.com>
> ---
>  Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
>  1 file changed, 1 insertion(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH anybus v3 3/6] dt-bindings: anybus-bridge: document devicetree binding

[thesven73]

From: Sven Van Asbroeck <svendev@arcx.com>

This patch adds devicetree binding documentation for the
Arcx anybus bridge.

Signed-off-by: Sven Van Asbroeck <svendev@arcx.com>
---
 .../bindings/misc/arcx,anybus-bridge.txt      | 34 +++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/arcx,anybus-bridge.txt

diff --git a/Documentation/devicetree/bindings/misc/arcx,anybus-bridge.txt b/Documentation/devicetree/bindings/misc/arcx,anybus-bridge.txt
new file mode 100644
index 000000000000..cb801b7568b0
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/arcx,anybus-bridge.txt
@@ -0,0 +1,34 @@
+* Arcx anybus bridge
+
+This chip communicates with the SoC over a parallel bus. It is
+expected that its Device Tree node is specified as the child of a node
+corresponding to the parallel bus used for communication.
+
+Required properties:
+
+  - compatible : The following chip-specific string:
+        "arcx,anybus-bridge"
+
+  - reg : bus memory area where the cpld registers are located.
+
+  - reset-gpios : the GPIO pin connected to the reset line of the bridge.
+
+  - #reset-cells : Must be 1.
+	this bridge is a reset provider to its two embedded Anybus-S slots.
+
+Example of usage:
+
+This example places the bridge on top of the i.MX WEIM parallel bus, see:
+Documentation/devicetree/bindings/bus/imx-weim.txt
+
+&weim {
+	anybus_bridge: bridge@0,0 {
+		compatible = "arcx,anybus-bridge";
+		reg = <0 0 0x100>;
+		reset-gpios = <&gpio5 2 GPIO_ACTIVE_HIGH>;
+		#reset-cells = <1>;
+		/* fsl,weim-cs-timing is a i.MX WEIM bus specific property */
+		fsl,weim-cs-timing = <0x024400b1 0x00001010 0x20081100
+				0x00000000 0xa0000240 0x00000000>;
+	};
+};
-- 
2.17.1

Re: [PATCH anybus v3 3/6] dt-bindings: anybus-bridge: document devicetree binding

[Rob Herring]

On Sun,  4 Nov 2018 10:54:58 -0500, thesven73@gmail.com wrote:
> From: Sven Van Asbroeck <svendev@arcx.com>
> 
> This patch adds devicetree binding documentation for the
> Arcx anybus bridge.
> 
> Signed-off-by: Sven Van Asbroeck <svendev@arcx.com>
> ---
>  .../bindings/misc/arcx,anybus-bridge.txt      | 34 +++++++++++++++++++
>  1 file changed, 34 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/misc/arcx,anybus-bridge.txt
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH anybus v3 4/6] bus: support HMS Anybus-S bus

[thesven73]

From: Sven Van Asbroeck <svendev@arcx.com>

The Anybus-S/Anybus-M is a series of interchangeable fieldbus communication
modules featuring on board memory and processing power. All software and
hardware functionality required to communicate on the fieldbus is
incorporated in the module itself, allowing the application to focus on
other tasks.

Typical applications are frequency inverters, HMI and visualization
devices, instruments, scales, robotics, PLC’s and intelligent measuring
devices.

Official documentation:
https://www.anybus.com/docs/librariesprovider7/default-document-library/
manuals-design-guides/hms-hmsi-27-275.pdf

Signed-off-by: Sven Van Asbroeck <svendev@arcx.com>
---
 drivers/bus/Kconfig            |   10 +
 drivers/bus/Makefile           |    1 +
 drivers/bus/anybuss-host.c     | 1503 ++++++++++++++++++++++++++++++++
 include/linux/anybuss-client.h |  100 +++
 4 files changed, 1614 insertions(+)
 create mode 100644 drivers/bus/anybuss-host.c
 create mode 100644 include/linux/anybuss-client.h

diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
index f29131529ddb..a11980889711 100644
--- a/drivers/bus/Kconfig
+++ b/drivers/bus/Kconfig
@@ -45,6 +45,16 @@ config IMX_WEIM
 	  The WEIM(Wireless External Interface Module) works like a bus.
 	  You can attach many different devices on it, such as NOR, onenand.
 
+config HMS_ANYBUSS_HOST
+	tristate "HMS Anybus-S Host/Bus Driver"
+	select REGMAP
+	depends on OF
+	help
+	  Driver for the HMS Industrial Networks Anybus-S bus.
+	  You can attach Anybus-S compatible cards to it, which
+	  typically provide fieldbus and industrial ethernet
+	  functionality.
+
 config MIPS_CDMM
 	bool "MIPS Common Device Memory Map (CDMM) Driver"
 	depends on CPU_MIPSR2
diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
index 24fa2cde0692..d2ab4716b509 100644
--- a/drivers/bus/Makefile
+++ b/drivers/bus/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_BRCMSTB_GISB_ARB)	+= brcmstb_gisb.o
 obj-$(CONFIG_FSL_MC_BUS)	+= fsl-mc/
 
 obj-$(CONFIG_IMX_WEIM)		+= imx-weim.o
+obj-$(CONFIG_HMS_ANYBUSS_HOST)	+= anybuss-host.o
 obj-$(CONFIG_MIPS_CDMM)		+= mips_cdmm.o
 obj-$(CONFIG_MVEBU_MBUS) 	+= mvebu-mbus.o
 
diff --git a/drivers/bus/anybuss-host.c b/drivers/bus/anybuss-host.c
new file mode 100644
index 000000000000..05b3bb0281d2
--- /dev/null
+++ b/drivers/bus/anybuss-host.c
@@ -0,0 +1,1503 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * HMS Anybus-S Host Driver
+ *
+ * Copyright (C) 2018 Arcx Inc
+ */
+
+/*
+ * Architecture Overview
+ * =====================
+ * This driver (running on the CPU/SoC) and the Anybus-S communicate
+ * by reading and writing data to/from the Anybus-S Dual-Port RAM (dpram).
+ * This is memory connected to both the SoC and Anybus-S host, which both sides
+ * can access freely and concurrently.
+ *
+ * Synchronization happens by means of two registers located in the dpram:
+ * IND_AB: written exclusively by the Anybus host; and
+ * IND_AP: written exclusively by the driver.
+ *
+ * Communication happens using one of the following mechanisms:
+ * 1. reserve, read/write, release dpram memory areas:
+ *	using an IND_AB/IND_AP protocol, the driver is able to reserve certain
+ *	memory areas. no dpram memory can be read or written except if reserved.
+ *	(with a few limited exceptions)
+ * 2. send and receive data structures via a shared mailbox:
+ *	using an IND_AB/IND_AP protocol, the driver and Anybus host are able to
+ *	exchange commands and responses using a shared mailbox.
+ * 3. receive software interrupts:
+ *	using an IND_AB/IND_AP protocol, the Anybus is able to notify the driver
+ *	of certain events such as: bus online/offline, data available.
+ *	note that software interrupt event bits are located in a memory area
+ *	which must be reserved before it can be accessed.
+ *
+ * The manual[1] is silent on whether these mechanisms can happen concurrently,
+ * or how they should be synchronized. However, section 13 (Driver Example)
+ * provides the following suggestion for developing a driver:
+ * a) an interrupt handler which updates global variables;
+ * b) a continuously-running task handling area requests (1 above)
+ * c) a continuously-running task handling mailbox requests (2 above)
+ * The example conspicuously leaves out software interrupts (3 above), which
+ * is the thorniest issue to get right (see below).
+ *
+ * The naive, straightforward way to implement this would be:
+ * - create an isr which updates shared variables;
+ * - create a work_struct which handles software interrupts on a queue;
+ * - create a function which does reserve/update/unlock in a loop;
+ * - create a function which does mailbox send/receive in a loop;
+ * - call the above functions from the driver's read/write/ioctl;
+ * - synchronize using mutexes/spinlocks:
+ *	+ only one area request at a time
+ *	+ only one mailbox request at a time
+ *	+ protect AB_IND, AB_IND against data hazards (e.g. read-after-write)
+ *
+ * Unfortunately, the presence of the software interrupt causes subtle yet
+ * considerable synchronization issues; especially problematic is the
+ * requirement to reserve/release the area which contains the status bits.
+ *
+ * The driver architecture presented here sidesteps these synchronization issues
+ * by accessing the dpram from a single kernel thread only. User-space throws
+ * "tasks" (i.e. 1, 2 above) into a task queue, waits for their completion,
+ * and the kernel thread runs them to completion.
+ *
+ * Each task has a task_function, which is called/run by the queue thread.
+ * That function communicates with the Anybus hardware, and returns either
+ * 0 (OK), a negative error code (error), or -EINPROGRESS (waiting).
+ * On OK or error, the queue thread completes and dequeues the task,
+ * which also releases the user space thread which may still be waiting for it.
+ * On -EINPROGRESS (waiting), the queue thread will leave the task on the queue,
+ * and revisit (call again) whenever an interrupt event comes in.
+ *
+ * Each task has a state machine, which is run by calling its task_function.
+ * It ensures that the task will go through its various stages over time,
+ * returning -EINPROGRESS if it wants to wait for an event to happen.
+ *
+ * Note that according to the manual's driver example, the following operations
+ * may run independent of each other:
+ * - area reserve/read/write/release	(point 1 above)
+ * - mailbox operations			(point 2 above)
+ * - switching power on/off
+ *
+ * To allow them to run independently, each operation class gets its own queue.
+ *
+ * Userspace processes A, B, C, D post tasks to the appropriate queue,
+ * and wait for task completion:
+ *
+ *	process A	B	C	D
+ *		|	|	|	|
+ *		v	v	v	v
+ *	|<-----	========================================
+ *	|		|	   |		|
+ *	|		v	   v		v-------<-------+
+ *	|	+--------------------------------------+	|
+ *	|	| power q     | mbox q    | area q     |	|
+ *	|	|------------|------------|------------|	|
+ *	|	| task       | task       | task       |	|
+ *	|	| task       | task       | task       |	|
+ *	|	| task wait  | task wait  | task wait  |	|
+ *	|	+--------------------------------------+	|
+ *	|		^	   ^		^		|
+ *	|		|	   |		|		^
+ *	|	+--------------------------------------+	|
+ *	|	|	     queue thread	       |	|
+ *	|	|--------------------------------------|	|
+ *	|	| single-threaded:		       |	|
+ *	|	| loop:				       |	|
+ *	v	|   for each queue:		       |	|
+ *	|	|     run task state machine	       |	|
+ *	|	|     if task waiting:		       |	|
+ *	|	|       leave on queue		       |	|
+ *	|	|     if task done:		       |	|
+ *	|	|       complete task, remove from q   |	|
+ *	|	|   if software irq event bits set:    |	|
+ *	|	|     notify userspace		       |	|
+ *	|	|     post clear event bits task------>|>-------+
+ *	|	|   wait for IND_AB changed event OR   |
+ *	|	|            task added event	  OR   |
+ *	|	|	     timeout		       |
+ *	|	| end loop			       |
+ *	|	+--------------------------------------+
+ *	|	+		wake up		       +
+ *	|	+--------------------------------------+
+ *	|		^			^
+ *	|		|			|
+ *	+-------->-------			|
+ *						|
+ *		+--------------------------------------+
+ *		|	interrupt service routine      |
+ *		|--------------------------------------|
+ *		| wake up queue thread on IND_AB change|
+ *		+--------------------------------------+
+ *
+ * Note that the Anybus interrupt is dual-purpose:
+ * - after a reset, triggered when the card becomes ready;
+ * - during normal operation, triggered when AB_IND changes.
+ * This is why the interrupt service routine doesn't just wake up the
+ * queue thread, but also completes the card_boot completion.
+ *
+ * [1] https://www.anybus.com/docs/librariesprovider7/default-document-library/
+ *	manuals-design-guides/hms-hmsi-27-275.pdf
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/interrupt.h>
+#include <linux/atomic.h>
+#include <linux/kthread.h>
+#include <linux/kfifo.h>
+#include <linux/spinlock.h>
+#include <linux/uaccess.h>
+#include <linux/reset.h>
+#include <linux/regmap.h>
+#include <linux/of.h>
+#include <linux/random.h>
+#include <linux/refcount.h>
+
+#include <linux/anybuss-client.h>
+
+#define DPRAM_SIZE		0x800
+#define MAX_MBOX_MSG_SZ		0x0FF
+#define TIMEOUT			(HZ*2)
+#define MAX_DATA_AREA_SZ	0x200
+#define MAX_FBCTRL_AREA_SZ	0x1BE
+
+#define REG_BOOTLOADER_V	0x7C0
+#define REG_API_V		0x7C2
+#define REG_FIELDBUS_V		0x7C4
+#define REG_SERIAL_NO		0x7C6
+#define REG_FIELDBUS_TYPE	0x7CC
+#define REG_MODULE_SW_V		0x7CE
+#define REG_IND_AB		0x7FF
+#define REG_IND_AP		0x7FE
+#define REG_EVENT_CAUSE		0x7ED
+#define MBOX_IN_AREA		0x400
+#define MBOX_OUT_AREA		0x520
+#define DATA_IN_AREA		0x000
+#define DATA_OUT_AREA		0x200
+#define FBCTRL_AREA		0x640
+
+#define EVENT_CAUSE_DC          0x01
+#define EVENT_CAUSE_FBOF        0x02
+#define EVENT_CAUSE_FBON        0x04
+
+#define IND_AB_UPDATED		0x08
+#define IND_AX_MIN		0x80
+#define IND_AX_MOUT		0x40
+#define IND_AX_IN		0x04
+#define IND_AX_OUT		0x02
+#define IND_AX_FBCTRL		0x01
+#define IND_AP_LOCK		0x08
+#define IND_AP_ACTION		0x10
+#define IND_AX_EVNT		0x20
+#define IND_AP_ABITS		(IND_AX_IN | IND_AX_OUT | \
+					IND_AX_FBCTRL | \
+					IND_AP_ACTION | IND_AP_LOCK)
+
+#define INFO_TYPE_FB		0x0002
+#define INFO_TYPE_APP		0x0001
+#define INFO_COMMAND		0x4000
+
+#define OP_MODE_FBFC		0x0002
+#define OP_MODE_FBS		0x0004
+#define OP_MODE_CD		0x0200
+
+#define CMD_START_INIT		0x0001
+#define CMD_ANYBUS_INIT		0x0002
+#define CMD_END_INIT		0x0003
+
+/* ------------- ref counted tasks ------------- */
+
+struct ab_task;
+typedef int (*ab_task_fn_t)(struct anybuss_host *cd,
+					struct ab_task *t);
+typedef void (*ab_done_fn_t)(struct anybuss_host *cd);
+
+struct area_priv {
+	bool is_write;
+	u16 flags;
+	u16 addr;
+	size_t count;
+	u8 buf[MAX_DATA_AREA_SZ];
+};
+
+struct anybus_mbox_hdr {
+	u16 id;
+	u16 info;
+	u16 cmd_num;
+	u16 data_size;
+	u16 frame_count;
+	u16 frame_num;
+	u16 offset_high;
+	u16 offset_low;
+	u16 extended[8];
+} __packed;
+
+struct mbox_priv {
+	struct anybus_mbox_hdr hdr;
+	size_t msg_out_sz;
+	size_t msg_in_sz;
+	u8 msg[MAX_MBOX_MSG_SZ];
+};
+
+struct ab_task {
+	struct kmem_cache	*cache;
+	refcount_t		refcnt;
+	ab_task_fn_t		task_fn;
+	ab_done_fn_t		done_fn;
+	int			result;
+	struct completion	done;
+	unsigned long		start_jiffies;
+	union {
+		struct area_priv area_pd;
+		struct mbox_priv mbox_pd;
+	};
+};
+
+static struct ab_task *ab_task_create_get(struct kmem_cache *cache,
+			ab_task_fn_t task_fn)
+{
+	struct ab_task *t;
+
+	t = kmem_cache_alloc(cache, GFP_KERNEL);
+	if (!t)
+		return NULL;
+	t->cache = cache;
+	refcount_set(&t->refcnt, 1);
+	t->task_fn = task_fn;
+	t->done_fn = NULL;
+	t->result = 0;
+	init_completion(&t->done);
+	return t;
+}
+
+static void ab_task_put(struct ab_task *t)
+{
+	struct kmem_cache *cache = t->cache;
+
+	if (refcount_dec_and_test(&t->refcnt))
+		kmem_cache_free(cache, t);
+}
+
+static struct ab_task *__ab_task_get(struct ab_task *t)
+{
+	refcount_inc(&t->refcnt);
+	return t;
+}
+
+static void __ab_task_finish(struct ab_task *t, struct anybuss_host *cd)
+{
+	if (t->done_fn)
+		t->done_fn(cd);
+	complete(&t->done);
+}
+
+static void
+ab_task_dequeue_finish_put(struct kfifo *q, struct anybuss_host *cd)
+{
+	int ret;
+	struct ab_task *t;
+
+	ret = kfifo_out(q, &t, sizeof(t));
+	WARN_ON(!ret);
+	__ab_task_finish(t, cd);
+	ab_task_put(t);
+}
+
+static int
+ab_task_enqueue(struct ab_task *t, struct kfifo *q, spinlock_t *slock,
+		wait_queue_head_t *wq)
+{
+	int ret;
+
+	t->start_jiffies = jiffies;
+	__ab_task_get(t);
+	ret = kfifo_in_spinlocked(q, &t, sizeof(t), slock);
+	if (!ret) {
+		ab_task_put(t);
+		return -ENOMEM;
+	}
+	wake_up(wq);
+	return 0;
+}
+
+static int
+ab_task_enqueue_wait(struct ab_task *t, struct kfifo *q, spinlock_t *slock,
+		     wait_queue_head_t *wq)
+{
+	int ret;
+
+	ret = ab_task_enqueue(t, q, slock, wq);
+	if (ret)
+		return ret;
+	ret = wait_for_completion_interruptible(&t->done);
+	if (ret)
+		return ret;
+	return t->result;
+}
+
+/* ------------------------ anybus hardware ------------------------ */
+
+struct anybuss_host {
+	struct device *dev;
+	struct anybuss_client *client;
+	struct reset_control *reset;
+	struct regmap *regmap;
+	int irq;
+	struct task_struct *qthread;
+	wait_queue_head_t wq;
+	struct completion card_boot;
+	atomic_t ind_ab;
+	spinlock_t qlock;
+	struct kmem_cache *qcache;
+	struct kfifo qs[3];
+	struct kfifo *powerq;
+	struct kfifo *mboxq;
+	struct kfifo *areaq;
+	bool power_on;
+	bool softint_pending;
+	atomic_t dc_event;
+	wait_queue_head_t dc_wq;
+	atomic_t fieldbus_online;
+	struct kernfs_node *fieldbus_online_sd;
+};
+
+static int test_dpram(struct regmap *regmap)
+{
+	int i;
+	unsigned int val;
+
+	for (i = 0; i < DPRAM_SIZE; i++)
+		regmap_write(regmap, i, (u8)i);
+	for (i = 0; i < DPRAM_SIZE; i++) {
+		regmap_read(regmap, i, &val);
+		if ((u8)val != (u8)i)
+			return -EIO;
+	}
+	return 0;
+}
+
+static int read_ind_ab(struct regmap *regmap)
+{
+	unsigned long timeout = jiffies + HZ/2;
+	unsigned int a, b;
+
+	while (time_before_eq(jiffies, timeout)) {
+		regmap_read(regmap, REG_IND_AB, &a);
+		regmap_read(regmap, REG_IND_AB, &b);
+		if (likely(a == b))
+			return (int)a;
+		cpu_relax();
+	}
+	WARN(1, "IND_AB register not stable");
+	return -ETIMEDOUT;
+}
+
+static int write_ind_ap(struct regmap *regmap, unsigned int ind_ap)
+{
+	unsigned long timeout = jiffies + HZ/2;
+	unsigned int v;
+
+	while (time_before_eq(jiffies, timeout)) {
+		regmap_write(regmap, REG_IND_AP, ind_ap);
+		regmap_read(regmap, REG_IND_AP, &v);
+		if (likely(ind_ap == v))
+			return 0;
+		cpu_relax();
+	}
+	WARN(1, "IND_AP register not stable");
+	return -ETIMEDOUT;
+}
+
+static irqreturn_t irq_handler(int irq, void *data)
+{
+	struct anybuss_host *cd = data;
+	int ind_ab;
+
+	/*
+	 * irq handler needs exclusive access to the IND_AB register,
+	 * because the act of reading the register acks the interrupt.
+	 *
+	 * store the register value in cd->ind_ab (an atomic_t), so that the
+	 * queue thread is able to read it without causing an interrupt ack
+	 * side-effect (and without spuriously acking an interrupt).
+	 */
+	ind_ab = read_ind_ab(cd->regmap);
+	if (ind_ab < 0)
+		return IRQ_NONE;
+	atomic_set(&cd->ind_ab, ind_ab);
+	complete(&cd->card_boot);
+	wake_up(&cd->wq);
+	return IRQ_HANDLED;
+}
+
+/* ------------------------ power on/off tasks --------------------- */
+
+static int task_fn_power_off(struct anybuss_host *cd,
+				struct ab_task *t)
+{
+	if (!cd->power_on)
+		return 0;
+	disable_irq(cd->irq);
+	reset_control_assert(cd->reset);
+	atomic_set(&cd->ind_ab, IND_AB_UPDATED);
+	atomic_set(&cd->fieldbus_online, 0);
+	sysfs_notify_dirent(cd->fieldbus_online_sd);
+	cd->power_on = false;
+	return 0;
+}
+
+static int task_fn_power_on_2(struct anybuss_host *cd,
+				struct ab_task *t)
+{
+	if (completion_done(&cd->card_boot)) {
+		cd->power_on = true;
+		return 0;
+	}
+	if (time_after(jiffies, t->start_jiffies + TIMEOUT)) {
+		disable_irq(cd->irq);
+		reset_control_assert(cd->reset);
+		dev_err(cd->dev, "power on timed out");
+		return -ETIMEDOUT;
+	}
+	return -EINPROGRESS;
+}
+
+static int task_fn_power_on(struct anybuss_host *cd,
+				struct ab_task *t)
+{
+	unsigned int dummy;
+
+	if (cd->power_on)
+		return 0;
+	/*
+	 * anybus docs: prevent false 'init done' interrupt by
+	 * doing a dummy read of IND_AB register while in reset.
+	 */
+	regmap_read(cd->regmap, REG_IND_AB, &dummy);
+	reinit_completion(&cd->card_boot);
+	enable_irq(cd->irq);
+	reset_control_deassert(cd->reset);
+	t->task_fn = task_fn_power_on_2;
+	return -EINPROGRESS;
+}
+
+int anybuss_set_power(struct anybuss_client *client, bool power_on)
+{
+	struct anybuss_host *cd = client->host;
+	struct ab_task *t;
+	int err;
+
+	t = ab_task_create_get(cd->qcache, power_on ?
+				task_fn_power_on : task_fn_power_off);
+	if (!t)
+		return -ENOMEM;
+	err = ab_task_enqueue_wait(t, cd->powerq, &cd->qlock, &cd->wq);
+	ab_task_put(t);
+	return err;
+}
+EXPORT_SYMBOL_GPL(anybuss_set_power);
+
+/* ---------------------------- area tasks ------------------------ */
+
+static int task_fn_area_3(struct anybuss_host *cd, struct ab_task *t)
+{
+	struct area_priv *pd = &t->area_pd;
+
+	if (!cd->power_on)
+		return -EIO;
+	if (atomic_read(&cd->ind_ab) & pd->flags) {
+		/* area not released yet */
+		if (time_after(jiffies, t->start_jiffies + TIMEOUT))
+			return -ETIMEDOUT;
+		return -EINPROGRESS;
+	}
+	return 0;
+}
+
+static int task_fn_area_2(struct anybuss_host *cd, struct ab_task *t)
+{
+	struct area_priv *pd = &t->area_pd;
+	unsigned int ind_ap;
+	int ret;
+
+	if (!cd->power_on)
+		return -EIO;
+	regmap_read(cd->regmap, REG_IND_AP, &ind_ap);
+	if (!(atomic_read(&cd->ind_ab) & pd->flags)) {
+		/* we don't own the area yet */
+		if (time_after(jiffies, t->start_jiffies + TIMEOUT)) {
+			dev_warn(cd->dev, "timeout waiting for area");
+			dump_stack();
+			return -ETIMEDOUT;
+		}
+		return -EINPROGRESS;
+	}
+	/* we own the area, do what we're here to do */
+	if (pd->is_write)
+		regmap_bulk_write(cd->regmap, pd->addr, pd->buf,
+							pd->count);
+	else
+		regmap_bulk_read(cd->regmap, pd->addr, pd->buf,
+							pd->count);
+	/* ask to release the area, must use unlocked release */
+	ind_ap &= ~IND_AP_ABITS;
+	ind_ap |= pd->flags;
+	ret = write_ind_ap(cd->regmap, ind_ap);
+	if (ret)
+		return ret;
+	t->task_fn = task_fn_area_3;
+	return -EINPROGRESS;
+}
+
+static int task_fn_area(struct anybuss_host *cd, struct ab_task *t)
+{
+	struct area_priv *pd = &t->area_pd;
+	unsigned int ind_ap;
+	int ret;
+
+	if (!cd->power_on)
+		return -EIO;
+	regmap_read(cd->regmap, REG_IND_AP, &ind_ap);
+	/* ask to take the area */
+	ind_ap &= ~IND_AP_ABITS;
+	ind_ap |= pd->flags | IND_AP_ACTION | IND_AP_LOCK;
+	ret = write_ind_ap(cd->regmap, ind_ap);
+	if (ret)
+		return ret;
+	t->task_fn = task_fn_area_2;
+	return -EINPROGRESS;
+}
+
+static struct ab_task *
+create_area_reader(struct kmem_cache *qcache, u16 flags, u16 addr,
+				size_t count)
+{
+	struct ab_task *t;
+	struct area_priv *ap;
+
+	t = ab_task_create_get(qcache, task_fn_area);
+	if (!t)
+		return NULL;
+	ap = &t->area_pd;
+	ap->flags = flags;
+	ap->addr = addr;
+	ap->is_write = false;
+	ap->count = count;
+	return t;
+}
+
+static struct ab_task *
+create_area_writer(struct kmem_cache *qcache, u16 flags, u16 addr,
+				const void *buf, size_t count)
+{
+	struct ab_task *t;
+	struct area_priv *ap;
+
+	t = ab_task_create_get(qcache, task_fn_area);
+	if (!t)
+		return NULL;
+	ap = &t->area_pd;
+	ap->flags = flags;
+	ap->addr = addr;
+	ap->is_write = true;
+	ap->count = count;
+	memcpy(ap->buf, buf, count);
+	return t;
+}
+
+static struct ab_task *
+create_area_user_writer(struct kmem_cache *qcache, u16 flags, u16 addr,
+				const void __user *buf, size_t count)
+{
+	struct ab_task *t;
+	struct area_priv *ap;
+
+	t = ab_task_create_get(qcache, task_fn_area);
+	if (!t)
+		return ERR_PTR(-ENOMEM);
+	ap = &t->area_pd;
+	ap->flags = flags;
+	ap->addr = addr;
+	ap->is_write = true;
+	ap->count = count;
+	if (copy_from_user(ap->buf, buf, count)) {
+		ab_task_put(t);
+		return ERR_PTR(-EFAULT);
+	}
+	return t;
+}
+
+static bool area_range_ok(u16 addr, size_t count, u16 area_start,
+							size_t area_sz)
+{
+	u16 area_end_ex = area_start + area_sz;
+	u16 addr_end_ex;
+
+	if (addr < area_start)
+		return false;
+	if (addr >= area_end_ex)
+		return false;
+	addr_end_ex = addr + count;
+	if (addr_end_ex > area_end_ex)
+		return false;
+	return true;
+}
+
+/* -------------------------- mailbox tasks ----------------------- */
+
+struct msgAnybusInit {
+	u16 input_io_len;
+	u16 input_dpram_len;
+	u16 input_total_len;
+	u16 output_io_len;
+	u16 output_dpram_len;
+	u16 output_total_len;
+	u16 op_mode;
+	u16 notif_config;
+	u16 wd_val;
+} __packed;
+
+static int task_fn_mbox_2(struct anybuss_host *cd, struct ab_task *t)
+{
+	struct mbox_priv *pd = &t->mbox_pd;
+	unsigned int ind_ap;
+
+	if (!cd->power_on)
+		return -EIO;
+	regmap_read(cd->regmap, REG_IND_AP, &ind_ap);
+	if (((atomic_read(&cd->ind_ab) ^ ind_ap) & IND_AX_MOUT) == 0) {
+		/* output message not here */
+		if (time_after(jiffies, t->start_jiffies + TIMEOUT))
+			return -ETIMEDOUT;
+		return -EINPROGRESS;
+	}
+	/* grab the returned header and msg */
+	regmap_bulk_read(cd->regmap, MBOX_OUT_AREA, &pd->hdr,
+		sizeof(pd->hdr));
+	regmap_bulk_read(cd->regmap, MBOX_OUT_AREA + sizeof(pd->hdr),
+		pd->msg, pd->msg_in_sz);
+	/* tell anybus we've consumed the message */
+	ind_ap ^= IND_AX_MOUT;
+	return write_ind_ap(cd->regmap, ind_ap);
+}
+
+static int task_fn_mbox(struct anybuss_host *cd, struct ab_task *t)
+{
+	struct mbox_priv *pd = &t->mbox_pd;
+	unsigned int ind_ap;
+	int ret;
+
+	if (!cd->power_on)
+		return -EIO;
+	regmap_read(cd->regmap, REG_IND_AP, &ind_ap);
+	if ((atomic_read(&cd->ind_ab) ^ ind_ap) & IND_AX_MIN) {
+		/* mbox input area busy */
+		if (time_after(jiffies, t->start_jiffies + TIMEOUT))
+			return -ETIMEDOUT;
+		return -EINPROGRESS;
+	}
+	/* write the header and msg to input area */
+	regmap_bulk_write(cd->regmap, MBOX_IN_AREA, &pd->hdr,
+					sizeof(pd->hdr));
+	regmap_bulk_write(cd->regmap, MBOX_IN_AREA + sizeof(pd->hdr),
+					pd->msg, pd->msg_out_sz);
+	/* tell anybus we gave it a message */
+	ind_ap ^= IND_AX_MIN;
+	ret = write_ind_ap(cd->regmap, ind_ap);
+	if (ret)
+		return ret;
+	t->start_jiffies = jiffies;
+	t->task_fn = task_fn_mbox_2;
+	return -EINPROGRESS;
+}
+
+static void log_invalid_other(struct device *dev,
+				struct anybus_mbox_hdr *hdr)
+{
+	size_t ext_offs = ARRAY_SIZE(hdr->extended) - 1;
+	u16 code = be16_to_cpu(hdr->extended[ext_offs]);
+
+	dev_err(dev, "   Invalid other: [0x%02X]", code);
+}
+
+static const char * const EMSGS[] = {
+	"Invalid Message ID",
+	"Invalid Message Type",
+	"Invalid Command",
+	"Invalid Data Size",
+	"Message Header Malformed (offset 008h)",
+	"Message Header Malformed (offset 00Ah)",
+	"Message Header Malformed (offset 00Ch - 00Dh)",
+	"Invalid Address",
+	"Invalid Response",
+	"Flash Config Error",
+};
+
+static int mbox_cmd_err(struct device *dev, struct mbox_priv *mpriv)
+{
+	int i;
+	u8 ecode;
+	struct anybus_mbox_hdr *hdr = &mpriv->hdr;
+	u16 info = be16_to_cpu(hdr->info);
+	u8 *phdr = (u8 *)hdr;
+	u8 *pmsg = mpriv->msg;
+
+	if (!(info & 0x8000))
+		return 0;
+	ecode = (info >> 8) & 0x0F;
+	dev_err(dev, "mailbox command failed:");
+	if (ecode == 0x0F)
+		log_invalid_other(dev, hdr);
+	else if (ecode < ARRAY_SIZE(EMSGS))
+		dev_err(dev, "   Error code: %s (0x%02X)",
+			EMSGS[ecode], ecode);
+	else
+		dev_err(dev, "   Error code: 0x%02X\n", ecode);
+	dev_err(dev, "Failed command:");
+	dev_err(dev, "Message Header:");
+	for (i = 0; i < sizeof(mpriv->hdr); i += 2)
+		dev_err(dev, "%02X%02X", phdr[i], phdr[i+1]);
+	dev_err(dev, "Message Data:");
+	for (i = 0; i < mpriv->msg_in_sz; i += 2)
+		dev_err(dev, "%02X%02X", pmsg[i], pmsg[i+1]);
+	dev_err(dev, "Stack dump:");
+	dump_stack();
+	return -EIO;
+}
+
+static int _anybus_mbox_cmd(struct anybuss_host *cd,
+				u16 cmd_num, bool is_fb_cmd,
+				const void *msg_out, size_t msg_out_sz,
+				void *msg_in, size_t msg_in_sz,
+				const void *ext, size_t ext_sz)
+{
+	struct ab_task *t;
+	struct mbox_priv *pd;
+	struct anybus_mbox_hdr *h;
+	size_t msg_sz = max(msg_in_sz, msg_out_sz);
+	u16 info;
+	int err;
+
+	if (msg_sz > MAX_MBOX_MSG_SZ)
+		return -EINVAL;
+	if (ext && (ext_sz > sizeof(h->extended)))
+		return -EINVAL;
+	t = ab_task_create_get(cd->qcache, task_fn_mbox);
+	if (!t)
+		return -ENOMEM;
+	pd = &t->mbox_pd;
+	h = &pd->hdr;
+	info = is_fb_cmd ? INFO_TYPE_FB : INFO_TYPE_APP;
+	/*
+	 * prevent uninitialized memory in the header from being sent
+	 * across the anybus
+	 */
+	memset(h, 0, sizeof(*h));
+	h->info = cpu_to_be16(info | INFO_COMMAND);
+	h->cmd_num = cpu_to_be16(cmd_num);
+	h->data_size = cpu_to_be16(msg_out_sz);
+	h->frame_count = cpu_to_be16(1);
+	h->frame_num = cpu_to_be16(1);
+	h->offset_high = cpu_to_be16(0);
+	h->offset_low = cpu_to_be16(0);
+	if (ext)
+		memcpy(h->extended, ext, ext_sz);
+	memcpy(pd->msg, msg_out, msg_out_sz);
+	pd->msg_out_sz = msg_out_sz;
+	pd->msg_in_sz = msg_in_sz;
+	err = ab_task_enqueue_wait(t, cd->powerq, &cd->qlock, &cd->wq);
+	if (err)
+		goto out;
+	/*
+	 * mailbox mechanism worked ok, but maybe the mbox response
+	 * contains an error ?
+	 */
+	err = mbox_cmd_err(cd->dev, pd);
+	if (err)
+		goto out;
+	memcpy(msg_in, pd->msg, msg_in_sz);
+out:
+	ab_task_put(t);
+	return err;
+}
+
+/* ------------------------ anybus queues ------------------------ */
+
+static void process_q(struct anybuss_host *cd, struct kfifo *q)
+{
+	struct ab_task *t;
+	int ret;
+
+	ret = kfifo_out_peek(q, &t, sizeof(t));
+	if (!ret)
+		return;
+	t->result = t->task_fn(cd, t);
+	if (t->result != -EINPROGRESS)
+		ab_task_dequeue_finish_put(q, cd);
+}
+
+static bool qs_have_work(struct kfifo *qs, size_t num)
+{
+	size_t i;
+	struct ab_task *t;
+	int ret;
+
+	for (i = 0; i < num; i++, qs++) {
+		ret = kfifo_out_peek(qs, &t, sizeof(t));
+		if (ret && (t->result != -EINPROGRESS))
+			return true;
+	}
+	return false;
+}
+
+static void process_qs(struct anybuss_host *cd)
+{
+	size_t i;
+	struct kfifo *qs = cd->qs;
+	size_t nqs = ARRAY_SIZE(cd->qs);
+
+	for (i = 0; i < nqs; i++, qs++)
+		process_q(cd, qs);
+}
+
+static void softint_ack(struct anybuss_host *cd)
+{
+	unsigned int ind_ap;
+
+	cd->softint_pending = false;
+	if (!cd->power_on)
+		return;
+	regmap_read(cd->regmap, REG_IND_AP, &ind_ap);
+	ind_ap &= ~IND_AX_EVNT;
+	ind_ap |= atomic_read(&cd->ind_ab) & IND_AX_EVNT;
+	write_ind_ap(cd->regmap, ind_ap);
+}
+
+static void process_softint(struct anybuss_host *cd)
+{
+	static const u8 zero;
+	int ret;
+	unsigned int ind_ap, ev;
+	struct ab_task *t;
+
+	if (!cd->power_on)
+		return;
+	if (cd->softint_pending)
+		return;
+	regmap_read(cd->regmap, REG_IND_AP, &ind_ap);
+	if (!((atomic_read(&cd->ind_ab) ^ ind_ap) & IND_AX_EVNT))
+		return;
+	/* process software interrupt */
+	regmap_read(cd->regmap, REG_EVENT_CAUSE, &ev);
+	if (ev & EVENT_CAUSE_FBON) {
+		atomic_set(&cd->fieldbus_online, 1);
+		sysfs_notify_dirent(cd->fieldbus_online_sd);
+		dev_dbg(cd->dev, "Fieldbus ON");
+	}
+	if (ev & EVENT_CAUSE_FBOF) {
+		atomic_set(&cd->fieldbus_online, 0);
+		sysfs_notify_dirent(cd->fieldbus_online_sd);
+		dev_dbg(cd->dev, "Fieldbus OFF");
+	}
+	if (ev & EVENT_CAUSE_DC) {
+		atomic_inc(&cd->dc_event);
+		wake_up_all(&cd->dc_wq);
+		dev_dbg(cd->dev, "Fieldbus data changed");
+	}
+	/*
+	 * reset the event cause bits.
+	 * this must be done while owning the fbctrl area, so we'll
+	 * enqueue a task to do that.
+	 */
+	t = create_area_writer(cd->qcache, IND_AX_FBCTRL,
+		REG_EVENT_CAUSE, &zero, sizeof(zero));
+	if (!t) {
+		ret = -ENOMEM;
+		goto out;
+	}
+	t->done_fn = softint_ack;
+	ret = ab_task_enqueue(t, cd->powerq, &cd->qlock, &cd->wq);
+	ab_task_put(t);
+	cd->softint_pending = true;
+out:
+	WARN_ON(ret);
+	if (ret)
+		softint_ack(cd);
+}
+
+static int qthread_fn(void *data)
+{
+	struct anybuss_host *cd = data;
+	struct kfifo *qs = cd->qs;
+	size_t nqs = ARRAY_SIZE(cd->qs);
+	unsigned int ind_ab;
+
+	/*
+	 * this kernel thread has exclusive access to the anybus's memory.
+	 * only exception: the IND_AB register, which is accessed exclusively
+	 * by the interrupt service routine (ISR). This thread must not touch
+	 * the IND_AB register, but it does require access to its value.
+	 *
+	 * the interrupt service routine stores the register's value in
+	 * cd->ind_ab (an atomic_t), where we may safely access it, with the
+	 * understanding that it can be modified by the ISR at any time.
+	 */
+
+	while (!kthread_should_stop()) {
+		/*
+		 * make a local copy of IND_AB, so we can go around the loop
+		 * again in case it changed while processing queues and softint.
+		 */
+		ind_ab = atomic_read(&cd->ind_ab);
+		process_qs(cd);
+		process_softint(cd);
+		wait_event_timeout(cd->wq,
+			(atomic_read(&cd->ind_ab) != ind_ab) ||
+				qs_have_work(qs, nqs) ||
+				kthread_should_stop(),
+			HZ);
+		/*
+		 * time out so even 'stuck' tasks will run eventually,
+		 * and can time out.
+		 */
+	}
+
+	return 0;
+}
+
+/* ------------------------ anybus exports ------------------------ */
+
+int anybuss_start_init(struct anybuss_client *client,
+			const struct anybuss_memcfg *cfg)
+{
+	int ret;
+	u16 op_mode;
+	struct anybuss_host *cd = client->host;
+	struct msgAnybusInit msg = {
+		.input_io_len = cpu_to_be16(cfg->input_io),
+		.input_dpram_len = cpu_to_be16(cfg->input_dpram),
+		.input_total_len = cpu_to_be16(cfg->input_total),
+		.output_io_len = cpu_to_be16(cfg->output_io),
+		.output_dpram_len = cpu_to_be16(cfg->output_dpram),
+		.output_total_len = cpu_to_be16(cfg->output_total),
+		.notif_config = cpu_to_be16(0x000F),
+		.wd_val = cpu_to_be16(0),
+	};
+
+	switch (cfg->offl_mode) {
+	case AB_OFFL_MODE_CLEAR:
+		op_mode = 0;
+		break;
+	case AB_OFFL_MODE_FREEZE:
+		op_mode = OP_MODE_FBFC;
+		break;
+	case AB_OFFL_MODE_SET:
+		op_mode = OP_MODE_FBS;
+		break;
+	default:
+		return -EINVAL;
+	}
+	msg.op_mode = cpu_to_be16(op_mode | OP_MODE_CD);
+	ret = _anybus_mbox_cmd(cd, CMD_START_INIT, false, NULL, 0,
+						NULL, 0, NULL, 0);
+	if (ret)
+		return ret;
+	return _anybus_mbox_cmd(cd, CMD_ANYBUS_INIT, false,
+			&msg, sizeof(msg), NULL, 0, NULL, 0);
+}
+EXPORT_SYMBOL_GPL(anybuss_start_init);
+
+int anybuss_finish_init(struct anybuss_client *client)
+{
+	struct anybuss_host *cd = client->host;
+
+	return _anybus_mbox_cmd(cd, CMD_END_INIT, false, NULL, 0,
+					NULL, 0, NULL, 0);
+}
+EXPORT_SYMBOL_GPL(anybuss_finish_init);
+
+int anybuss_read_fbctrl(struct anybuss_client *client, u16 addr,
+				void *buf, size_t count)
+{
+	struct anybuss_host *cd = client->host;
+	struct ab_task *t;
+	int ret;
+
+	if (count == 0)
+		return 0;
+	if (!area_range_ok(addr, count, FBCTRL_AREA,
+					MAX_FBCTRL_AREA_SZ))
+		return -EFAULT;
+	t = create_area_reader(cd->qcache, IND_AX_FBCTRL, addr, count);
+	if (!t)
+		return -ENOMEM;
+	ret = ab_task_enqueue_wait(t, cd->powerq, &cd->qlock, &cd->wq);
+	if (ret)
+		goto out;
+	memcpy(buf, t->area_pd.buf, count);
+out:
+	ab_task_put(t);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(anybuss_read_fbctrl);
+
+int anybuss_write_input(struct anybuss_client *client,
+				const char __user *buf, size_t size,
+				loff_t *offset)
+{
+	ssize_t len = min_t(loff_t, MAX_DATA_AREA_SZ - *offset, size);
+	struct anybuss_host *cd = client->host;
+	struct ab_task *t;
+	int ret;
+
+	if (len <= 0)
+		return 0;
+	t = create_area_user_writer(cd->qcache, IND_AX_IN,
+		DATA_IN_AREA + *offset, buf, len);
+	if (IS_ERR(t))
+		return PTR_ERR(t);
+	ret = ab_task_enqueue_wait(t, cd->powerq, &cd->qlock, &cd->wq);
+	ab_task_put(t);
+	if (ret)
+		return ret;
+	/* success */
+	*offset += len;
+	return len;
+}
+EXPORT_SYMBOL_GPL(anybuss_write_input);
+
+int anybuss_read_output(struct anybuss_client *client, int *dc_event,
+				char __user *buf, size_t size,
+				loff_t *offset)
+{
+	ssize_t len = min_t(loff_t, MAX_DATA_AREA_SZ - *offset, size);
+	struct anybuss_host *cd = client->host;
+	struct ab_task *t;
+	int ret;
+
+	if (len <= 0)
+		return 0;
+	t = create_area_reader(cd->qcache, IND_AX_OUT,
+		DATA_OUT_AREA + *offset, len);
+	if (!t)
+		return -ENOMEM;
+	*dc_event = atomic_read(&cd->dc_event);
+	ret = ab_task_enqueue_wait(t, cd->powerq, &cd->qlock, &cd->wq);
+	if (ret)
+		goto out;
+	if (copy_to_user(buf, t->area_pd.buf, len))
+		ret = -EFAULT;
+out:
+	ab_task_put(t);
+	if (ret)
+		return ret;
+	/* success */
+	*offset += len;
+	return len;
+}
+EXPORT_SYMBOL_GPL(anybuss_read_output);
+
+unsigned int anybuss_poll(struct anybuss_client *client, int dc_event,
+				struct file *filp, poll_table *wait)
+{
+	struct anybuss_host *cd = client->host;
+	unsigned int mask = POLLIN | POLLRDNORM | POLLOUT | POLLWRNORM;
+
+	poll_wait(filp, &cd->dc_wq, wait);
+	/* data changed ? */
+	if (atomic_read(&cd->dc_event) != dc_event)
+		mask |= POLLPRI | POLLERR;
+	return mask;
+}
+EXPORT_SYMBOL_GPL(anybuss_poll);
+
+int anybuss_send_msg(struct anybuss_client *client, u16 cmd_num,
+				const void *buf, size_t count)
+{
+	struct anybuss_host *cd = client->host;
+
+	return _anybus_mbox_cmd(cd, cmd_num, true, buf, count, NULL, 0,
+					NULL, 0);
+}
+EXPORT_SYMBOL_GPL(anybuss_send_msg);
+
+int anybuss_send_ext(struct anybuss_client *client, u16 cmd_num,
+	const void *buf, size_t count)
+{
+	struct anybuss_host *cd = client->host;
+
+	return _anybus_mbox_cmd(cd, cmd_num, true, NULL, 0, NULL, 0,
+					buf, count);
+}
+EXPORT_SYMBOL_GPL(anybuss_send_ext);
+
+int anybuss_recv_msg(struct anybuss_client *client, u16 cmd_num,
+	void *buf, size_t count)
+{
+	struct anybuss_host *cd = client->host;
+
+	return _anybus_mbox_cmd(cd, cmd_num, true, NULL, 0, buf, count,
+					NULL, 0);
+}
+EXPORT_SYMBOL_GPL(anybuss_recv_msg);
+
+/* ------------------------ bus functions ------------------------ */
+
+static int anybus_bus_match(struct device *dev,
+				struct device_driver *drv)
+{
+	struct anybuss_client_driver *adrv =
+		to_anybuss_client_driver(drv);
+	struct anybuss_client *adev =
+		to_anybuss_client(dev);
+
+	return adrv->fieldbus_type == adev->fieldbus_type;
+}
+
+static int anybus_bus_probe(struct device *dev)
+{
+	struct anybuss_client_driver *adrv =
+		to_anybuss_client_driver(dev->driver);
+	struct anybuss_client *adev =
+		to_anybuss_client(dev);
+
+	if (!adrv->probe)
+		return -ENODEV;
+	return adrv->probe(adev);
+}
+
+static int anybus_bus_remove(struct device *dev)
+{
+	struct anybuss_client_driver *adrv =
+		to_anybuss_client_driver(dev->driver);
+
+	if (adrv->remove)
+		return adrv->remove(to_anybuss_client(dev));
+	return 0;
+}
+
+static struct bus_type anybus_bus = {
+	.name		= "anybuss",
+	.match		= anybus_bus_match,
+	.probe		= anybus_bus_probe,
+	.remove		= anybus_bus_remove,
+};
+
+int anybuss_client_driver_register(struct anybuss_client_driver *drv)
+{
+	drv->driver.bus = &anybus_bus;
+	return driver_register(&drv->driver);
+}
+EXPORT_SYMBOL_GPL(anybuss_client_driver_register);
+
+void anybuss_client_driver_unregister(struct anybuss_client_driver *drv)
+{
+	return driver_unregister(&drv->driver);
+}
+EXPORT_SYMBOL_GPL(anybuss_client_driver_unregister);
+
+/* ------------------------ attributes ------------------------ */
+
+static ssize_t state_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	struct anybuss_host *cd = dev_get_drvdata(dev);
+
+	return snprintf(buf, PAGE_SIZE, "%s\n",
+		atomic_read(&cd->fieldbus_online) ?
+			"online" : "offline");
+}
+
+static DEVICE_ATTR_RO(state);
+
+static struct attribute *ctrl_attrs[] = {
+	&dev_attr_state.attr,
+	NULL
+};
+
+static struct attribute_group ctrl_group = { .attrs = ctrl_attrs };
+
+static void client_device_release(struct device *dev)
+{
+	kfree(to_anybuss_client(dev));
+}
+
+static int taskq_alloc(struct device *dev, struct kfifo *q)
+{
+	void *buf;
+	size_t size = 64 * sizeof(struct ab_task *);
+
+	buf = devm_kzalloc(dev, size, GFP_KERNEL);
+	if (!buf)
+		return -EIO;
+	return kfifo_init(q, buf, size);
+}
+
+/*
+ * parallel bus limitation:
+ *
+ * the anybus is 8-bit wide. we can't assume that the hardware will translate
+ * word accesses on the parallel bus to multiple byte-accesses on the anybus.
+ *
+ * therefore to be safe, we will limit parallel bus accesses to a single byte
+ * at a time.
+ *
+ * we can revisit if/when hardware becomes available that provides bus width
+ * translations.
+ */
+
+static int read_reg_bus(void *context, unsigned int reg,
+				unsigned int *val)
+{
+	void __iomem *base = context;
+
+	*val = readb(base + reg);
+	return 0;
+}
+
+static int write_reg_bus(void *context, unsigned int reg,
+				unsigned int val)
+{
+	void __iomem *base = context;
+
+	writeb(val, base + reg);
+	return 0;
+}
+
+static struct regmap *create_parallel_regmap(struct platform_device *pdev)
+{
+	struct regmap_config regmap_cfg = {
+		.reg_bits = 11,
+		.val_bits = 8,
+		/*
+		 * single-byte parallel bus accesses are atomic, so don't
+		 * require any synchronization.
+		 */
+		.disable_locking = true,
+		.reg_read = read_reg_bus,
+		.reg_write = write_reg_bus,
+	};
+	struct resource *res;
+	void __iomem *base;
+	struct device *dev = &pdev->dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (resource_size(res) < (1<<regmap_cfg.reg_bits))
+		return ERR_PTR(-EINVAL);
+	base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(base))
+		return (struct regmap *)base;
+	return devm_regmap_init(dev, NULL, base, &regmap_cfg);
+}
+
+static int anybus_host_probe(struct platform_device *pdev)
+{
+	int ret, i;
+	u8 val[4];
+	u16 fieldbus_type;
+	struct anybuss_host *cd;
+	struct device *dev = &pdev->dev;
+
+	cd = devm_kzalloc(dev, sizeof(*cd), GFP_KERNEL);
+	if (!cd)
+		return -ENOMEM;
+	cd->dev = dev;
+	init_completion(&cd->card_boot);
+	init_waitqueue_head(&cd->wq);
+	init_waitqueue_head(&cd->dc_wq);
+	for (i = 0; i < ARRAY_SIZE(cd->qs); i++) {
+		ret = taskq_alloc(dev, &cd->qs[i]);
+		if (ret)
+			return ret;
+	}
+	if (WARN_ON(ARRAY_SIZE(cd->qs) < 3))
+		return -EINVAL;
+	cd->powerq = &cd->qs[0];
+	cd->mboxq = &cd->qs[1];
+	cd->areaq = &cd->qs[2];
+	cd->reset = devm_reset_control_get_exclusive(dev, NULL);
+	if (IS_ERR(cd->reset))
+		return PTR_ERR(cd->reset);
+	cd->regmap = create_parallel_regmap(pdev);
+	if (IS_ERR(cd->regmap))
+		return PTR_ERR(cd->regmap);
+	spin_lock_init(&cd->qlock);
+	atomic_set(&cd->dc_event, 0);
+	atomic_set(&cd->fieldbus_online, 0);
+	cd->qcache = kmem_cache_create(dev_name(dev),
+		sizeof(struct ab_task), 0, 0, NULL);
+	if (!cd->qcache)
+		return -ENOMEM;
+	cd->irq = platform_get_irq(pdev, 0);
+	if (cd->irq <= 0) {
+		ret = cd->irq;
+		goto err_qcache;
+	}
+	/*
+	 * use a dpram test to check if a card is present, this is only
+	 * possible while in reset.
+	 */
+	reset_control_assert(cd->reset);
+	if (test_dpram(cd->regmap)) {
+		dev_err(dev, "no Anybus-S card in slot");
+		ret = -ENODEV;
+		goto err_qcache;
+	}
+	ret = devm_request_irq(dev, cd->irq, irq_handler, 0, dev_name(dev), cd);
+	if (ret) {
+		dev_err(dev, "could not request irq");
+		goto err_qcache;
+	}
+	/*
+	 * startup sequence:
+	 *   perform dummy IND_AB read to prevent false 'init done' irq
+	 *     (already done by test_dpram() above)
+	 *   release reset
+	 *   wait for first interrupt
+	 *   interrupt came in: ready to go !
+	 */
+	reset_control_deassert(cd->reset);
+	ret = wait_for_completion_timeout(&cd->card_boot, TIMEOUT);
+	if (ret == 0)
+		ret = -ETIMEDOUT;
+	if (ret < 0)
+		goto err_reset;
+	/*
+	 * according to the anybus docs, we're allowed to read these
+	 * without handshaking / reserving the area
+	 */
+	dev_info(dev, "Anybus-S card detected");
+	regmap_bulk_read(cd->regmap, REG_BOOTLOADER_V, val, 2);
+	dev_info(dev, "Bootloader version: %02X%02X",
+			val[0], val[1]);
+	regmap_bulk_read(cd->regmap, REG_API_V, val, 2);
+	dev_info(dev, "API version: %02X%02X", val[0], val[1]);
+	regmap_bulk_read(cd->regmap, REG_FIELDBUS_V, val, 2);
+	dev_info(dev, "Fieldbus version: %02X%02X", val[0], val[1]);
+	regmap_bulk_read(cd->regmap, REG_SERIAL_NO, val, 4);
+	dev_info(dev, "Serial number: %02X%02X%02X%02X",
+		val[0], val[1], val[2], val[3]);
+	add_device_randomness(&val, 4);
+	regmap_bulk_read(cd->regmap, REG_FIELDBUS_TYPE, &fieldbus_type,
+				sizeof(fieldbus_type));
+	fieldbus_type = be16_to_cpu(fieldbus_type);
+	dev_info(dev, "Fieldbus type: %04X", fieldbus_type);
+	regmap_bulk_read(cd->regmap, REG_MODULE_SW_V, val, 2);
+	dev_info(dev, "Module SW version: %02X%02X",
+		val[0], val[1]);
+	/* put card back reset until a client driver releases it */
+	disable_irq(cd->irq);
+	reset_control_assert(cd->reset);
+	atomic_set(&cd->ind_ab, IND_AB_UPDATED);
+	/* attributes */
+	ret = sysfs_create_group(&dev->kobj, &ctrl_group);
+	if (ret < 0)
+		goto err_reset;
+	cd->fieldbus_online_sd =
+		sysfs_get_dirent(dev->kobj.sd, "state");
+	if (!cd->fieldbus_online_sd) {
+		ret = -ENODEV;
+		goto err_sysfs;
+	}
+	/* fire up the queue thread */
+	cd->qthread = kthread_run(qthread_fn, cd, dev_name(dev));
+	if (IS_ERR(cd->qthread)) {
+		dev_err(dev, "could not create kthread");
+		ret = PTR_ERR(cd->qthread);
+		goto err_dirent;
+	}
+	/*
+	 * now advertise that we've detected a client device (card).
+	 * the bus infrastructure will match it to a client driver.
+	 */
+	cd->client = kzalloc(sizeof(*cd->client), GFP_KERNEL);
+	if (!cd->client) {
+		ret = -ENOMEM;
+		goto err_kthread;
+	}
+	cd->client->fieldbus_type = fieldbus_type;
+	cd->client->host = cd;
+	cd->client->dev.bus = &anybus_bus;
+	cd->client->dev.parent = dev;
+	cd->client->dev.id = pdev->id;
+	cd->client->dev.release = client_device_release;
+	dev_set_name(&cd->client->dev, "%s.card0",
+				dev_name(&pdev->dev));
+	ret = device_register(&cd->client->dev);
+	if (ret)
+		goto err_client;
+	platform_set_drvdata(pdev, cd);
+	dev_set_drvdata(dev, cd);
+	return 0;
+err_client:
+	put_device(&cd->client->dev);
+err_kthread:
+	kthread_stop(cd->qthread);
+err_dirent:
+	sysfs_put(cd->fieldbus_online_sd);
+err_sysfs:
+	sysfs_remove_group(&dev->kobj, &ctrl_group);
+err_reset:
+	reset_control_assert(cd->reset);
+err_qcache:
+	kmem_cache_destroy(cd->qcache);
+	return ret;
+}
+
+static int anybus_host_remove(struct platform_device *pdev)
+{
+	struct anybuss_host *cd = platform_get_drvdata(pdev);
+
+	device_unregister(&cd->client->dev);
+	kthread_stop(cd->qthread);
+	sysfs_put(cd->fieldbus_online_sd);
+	sysfs_remove_group(&cd->dev->kobj, &ctrl_group);
+	reset_control_assert(cd->reset);
+	kmem_cache_destroy(cd->qcache);
+	return 0;
+}
+
+static const struct of_device_id host_of_match[] = {
+	{ .compatible = "arcx,anybuss-host" },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(of, host_of_match);
+
+static struct platform_driver anybus_host_driver = {
+	.probe = anybus_host_probe,
+	.remove = anybus_host_remove,
+	.driver	= {
+		.name = "anybuss-host",
+		.owner	= THIS_MODULE,
+		.of_match_table	= of_match_ptr(host_of_match),
+	},
+};
+
+static int __init anybus_init(void)
+{
+	int ret;
+
+	ret = bus_register(&anybus_bus);
+	if (ret) {
+		pr_err("could not register Anybus-S bus: %d\n", ret);
+		return ret;
+	}
+	return platform_driver_register(&anybus_host_driver);
+}
+module_init(anybus_init);
+
+static void __exit anybus_exit(void)
+{
+	platform_driver_unregister(&anybus_host_driver);
+	bus_unregister(&anybus_bus);
+}
+module_exit(anybus_exit);
+
+MODULE_DESCRIPTION("HMS Anybus-S Host Driver");
+MODULE_AUTHOR("Sven Van Asbroeck <svendev@arcx.com>");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/anybuss-client.h b/include/linux/anybuss-client.h
new file mode 100644
index 000000000000..9d439d1a496f
--- /dev/null
+++ b/include/linux/anybuss-client.h
@@ -0,0 +1,100 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Anybus-S client adapter definitions
+ *
+ * Copyright 2018 Arcx Inc
+ */
+
+#ifndef __LINUX_ANYBUSS_CLIENT_H__
+#define __LINUX_ANYBUSS_CLIENT_H__
+
+#include <linux/device.h>
+#include <linux/types.h>
+#include <linux/poll.h>
+
+struct anybuss_host;
+
+struct anybuss_client {
+	struct device dev;
+	struct anybuss_host *host;
+	u16 fieldbus_type;
+};
+
+struct anybuss_client_driver {
+	struct device_driver driver;
+	int (*probe)(struct anybuss_client *adev);
+	int (*remove)(struct anybuss_client *adev);
+	u16 fieldbus_type;
+};
+
+int anybuss_client_driver_register(struct anybuss_client_driver *drv);
+void anybuss_client_driver_unregister(
+				struct anybuss_client_driver *drv);
+
+static inline struct anybuss_client *to_anybuss_client(
+					struct device *dev)
+{
+	return container_of(dev, struct anybuss_client, dev);
+}
+
+static inline struct anybuss_client_driver *to_anybuss_client_driver(
+					struct device_driver *drv)
+{
+	return container_of(drv, struct anybuss_client_driver, driver);
+}
+
+static inline void *
+anybuss_get_drvdata(const struct anybuss_client *client)
+{
+	return dev_get_drvdata(&client->dev);
+}
+
+static inline void
+anybuss_set_drvdata(struct anybuss_client *client, void *data)
+{
+	dev_set_drvdata(&client->dev, data);
+}
+
+int anybuss_set_power(struct anybuss_client *client, bool power_on);
+
+enum anybuss_offl_mode {
+	AB_OFFL_MODE_CLEAR = 0,
+	AB_OFFL_MODE_FREEZE,
+	AB_OFFL_MODE_SET
+};
+
+struct anybuss_memcfg {
+	u16 input_io;
+	u16 input_dpram;
+	u16 input_total;
+
+	u16 output_io;
+	u16 output_dpram;
+	u16 output_total;
+
+	enum anybuss_offl_mode offl_mode;
+};
+
+int anybuss_start_init(struct anybuss_client *client,
+			const struct anybuss_memcfg *cfg);
+int anybuss_finish_init(struct anybuss_client *client);
+int anybuss_read_fbctrl(struct anybuss_client *client, u16 addr,
+				void *buf, size_t count);
+int anybuss_send_msg(struct anybuss_client *client, u16 cmd_num,
+	const void *buf, size_t count);
+int anybuss_send_ext(struct anybuss_client *client, u16 cmd_num,
+	const void *buf, size_t count);
+int anybuss_recv_msg(struct anybuss_client *client, u16 cmd_num,
+	void *buf, size_t count);
+
+/* these help clients make a struct file_operations */
+int anybuss_write_input(struct anybuss_client *client,
+				const char __user *buf, size_t size,
+				loff_t *offset);
+int anybuss_read_output(struct anybuss_client *client, int *dc_event,
+				char __user *buf, size_t size,
+				loff_t *offset);
+unsigned int anybuss_poll(struct anybuss_client *client,
+		int dc_event, struct file *filp, poll_table *wait);
+
+#endif /* __LINUX_ANYBUSS_CLIENT_H__ */
-- 
2.17.1

Re: [PATCH anybus v3 4/6] bus: support HMS Anybus-S bus

[Arnd Bergmann]

On Sun, Nov 4, 2018 at 4:55 PM <thesven73@gmail.com> wrote:
> From: Sven Van Asbroeck <svendev@arcx.com>

> +struct anybus_mbox_hdr {
> +       u16 id;
> +       u16 info;
> +       u16 cmd_num;
> +       u16 data_size;
> +       u16 frame_count;
> +       u16 frame_num;
> +       u16 offset_high;
> +       u16 offset_low;
> +       u16 extended[8];
> +} __packed;

I don't think you want this to be __packed, it won't change the layout
but instead force byte accesses on architectures that don't have
fast unaligned load/store instructions.

Instead of the __packed, it's almost always better to ensure that
the structure itself is aligned to a 16-bit address.

> +static struct ab_task *ab_task_create_get(struct kmem_cache *cache,
> +                       ab_task_fn_t task_fn)
> +{
> +       struct ab_task *t;
> +
> +       t = kmem_cache_alloc(cache, GFP_KERNEL);
> +       if (!t)
> +               return NULL;
> +       t->cache = cache;
> +       refcount_set(&t->refcnt, 1);
> +       t->task_fn = task_fn;
> +       t->done_fn = NULL;
> +       t->result = 0;
> +       init_completion(&t->done);
> +       return t;
> +}

It looks like you build your own object management here. Maybe
use kobject, or at least kref instead of the refcount_t based
low-level implementation?

> +struct anybuss_host {
> +       struct device *dev;
> +       struct anybuss_client *client;
> +       struct reset_control *reset;
> +       struct regmap *regmap;
> +       int irq;
> +       struct task_struct *qthread;
> +       wait_queue_head_t wq;
> +       struct completion card_boot;
> +       atomic_t ind_ab;
> +       spinlock_t qlock;
> +       struct kmem_cache *qcache;
> +       struct kfifo qs[3];
> +       struct kfifo *powerq;
> +       struct kfifo *mboxq;
> +       struct kfifo *areaq;
> +       bool power_on;
> +       bool softint_pending;
> +       atomic_t dc_event;
> +       wait_queue_head_t dc_wq;
> +       atomic_t fieldbus_online;
> +       struct kernfs_node *fieldbus_online_sd;
> +};

Similarly, should that anybuss_host sturcture maybe be based on
a 'struct device' itself? What is the hierarchy of devices in sysfs?

> +
> +static int read_ind_ab(struct regmap *regmap)
> +{
> +       unsigned long timeout = jiffies + HZ/2;
> +       unsigned int a, b;
> +
> +       while (time_before_eq(jiffies, timeout)) {
> +               regmap_read(regmap, REG_IND_AB, &a);
> +               regmap_read(regmap, REG_IND_AB, &b);
> +               if (likely(a == b))
> +                       return (int)a;
> +               cpu_relax();
> +       }
> +       WARN(1, "IND_AB register not stable");
> +       return -ETIMEDOUT;
> +}

500ms is an awfully long time for a busy loop. Can you change the
cpu_relax() into a msleep() or usleep_range() to let other processes
get some time?

I see this is called from the interrupt handler at the moment, which
means you cannot call sleeping functions, but it also means that
the timeout may never happen because the timer tick IRQ cannot
get through. That means you may have to change the irq handler
logic, e.g. to try this a few times but then defer to a bottom half
if it fails for a long time.

> +/* -------------------------- mailbox tasks ----------------------- */
> +
> +struct msgAnybusInit {
> +       u16 input_io_len;
> +       u16 input_dpram_len;
> +       u16 input_total_len;
> +       u16 output_io_len;
> +       u16 output_dpram_len;
> +       u16 output_total_len;
> +       u16 op_mode;
> +       u16 notif_config;
> +       u16 wd_val;
> +} __packed;

Again, probably not __packed here.

> +static int task_fn_mbox_2(struct anybuss_host *cd, struct ab_task *t)
> +{
> +       struct mbox_priv *pd = &t->mbox_pd;
> +       unsigned int ind_ap;
> +
> +       if (!cd->power_on)
> +               return -EIO;
> +       regmap_read(cd->regmap, REG_IND_AP, &ind_ap);
> +       if (((atomic_read(&cd->ind_ab) ^ ind_ap) & IND_AX_MOUT) == 0) {
> +               /* output message not here */
> +               if (time_after(jiffies, t->start_jiffies + TIMEOUT))
> +                       return -ETIMEDOUT;
> +               return -EINPROGRESS;
> +       }

Again, avoid busy loops with long timeouts like this.

       Arnd

Re: [PATCH anybus v3 4/6] bus: support HMS Anybus-S bus

[Sven Van Asbroeck]

On Thu, Nov 8, 2018 at 9:07 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> It looks like you build your own object management here. Maybe
> use kobject, or at least kref instead of the refcount_t based
> low-level implementation?

Excellent point. I'll replace with krefs.

>
>
> I see this is called from the interrupt handler at the moment, which
> means you cannot call sleeping functions, but it also means that
> the timeout may never happen because the timer tick IRQ cannot
> get through. That means you may have to change the irq handler
> logic, e.g. to try this a few times but then defer to a bottom half
> if it fails for a long time.

Touche ! Yes, this is very likely a big problem.

What if I converted the interrupt handler into a threaded interrupt handler?
That would allow the timer tick to get through, correct?

Point taken about cpu_relax(), I will fix.

Re: [PATCH anybus v3 4/6] bus: support HMS Anybus-S bus

[Arnd Bergmann]

On Thu, Nov 8, 2018 at 4:47 PM Sven Van Asbroeck <thesven73@gmail.com> wrote:
> On Thu, Nov 8, 2018 at 9:07 AM Arnd Bergmann <arnd@arndb.de> wrote:
> > I see this is called from the interrupt handler at the moment, which
> > means you cannot call sleeping functions, but it also means that
> > the timeout may never happen because the timer tick IRQ cannot
> > get through. That means you may have to change the irq handler
> > logic, e.g. to try this a few times but then defer to a bottom half
> > if it fails for a long time.
>
> Touche ! Yes, this is very likely a big problem.
>
> What if I converted the interrupt handler into a threaded interrupt handler?
> That would allow the timer tick to get through, correct?

Yes, with a threaded IRQ handler, the tick comes through, and you can
use a sleeping function to back off between the retries.

        Arnd

Re: [PATCH anybus v3 4/6] bus: support HMS Anybus-S bus

[Sven Van Asbroeck]

On Thu, Nov 8, 2018 at 9:07 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> > +struct anybus_mbox_hdr {
> > +       u16 id;
> > +       u16 info;
> > +       u16 cmd_num;
> > +       u16 data_size;
> > +       u16 frame_count;
> > +       u16 frame_num;
> > +       u16 offset_high;
> > +       u16 offset_low;
> > +       u16 extended[8];
> > +} __packed;
>
> I don't think you want this to be __packed, it won't change the layout
> but instead force byte accesses on architectures that don't have
> fast unaligned load/store instructions.
>
> Instead of the __packed, it's almost always better to ensure that
> the structure itself is aligned to a 16-bit address.
>

A general question about __packed.

My current understanding is this:
(please tell me if it's incorrect or incomplete)

+ without __packed, the compiler is free to pad the struct in whatever
way it feels is best.
+ with __packed, the fields have to be laid out EXACTLY as specified.

Is it possible that someone will compile this on an architecture that 'likes'
16-bit ints to be 32-bit aligned? If such an architecture does not currently
exist, could it appear in the future?

If the compiler changes the padding in the struct, the driver will
break, as the struct layout is tightly defined by the anybus spec.

Would it be better to stay safe, and keep __packed in case of such
tightly defined structures?

Re: [PATCH anybus v3 4/6] bus: support HMS Anybus-S bus

[Arnd Bergmann]

On Fri, Nov 9, 2018 at 5:25 PM Sven Van Asbroeck <thesven73@gmail.com> wrote:
>
> On Thu, Nov 8, 2018 at 9:07 AM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > > +struct anybus_mbox_hdr {
> > > +       u16 id;
> > > +       u16 info;
> > > +       u16 cmd_num;
> > > +       u16 data_size;
> > > +       u16 frame_count;
> > > +       u16 frame_num;
> > > +       u16 offset_high;
> > > +       u16 offset_low;
> > > +       u16 extended[8];
> > > +} __packed;
> >
> > I don't think you want this to be __packed, it won't change the layout
> > but instead force byte accesses on architectures that don't have
> > fast unaligned load/store instructions.
> >
> > Instead of the __packed, it's almost always better to ensure that
> > the structure itself is aligned to a 16-bit address.
> >
>
> A general question about __packed.
>
> My current understanding is this:
> (please tell me if it's incorrect or incomplete)
>
> + without __packed, the compiler is free to pad the struct in whatever
> way it feels is best.
> + with __packed, the fields have to be laid out EXACTLY as specified.

It's not up to the compiler but the ELF ABI. The rules are largely consisten
among the architectures we support, but there are a couple of notable
exceptions:

- ARM OABI requires 32-bit alignment for structures
- x86-32 aligns 64-bit members to 32-bit rather than 64-bit
- m68k has some oddities, I think they can pack certain
  members (don't remember the details)

> Is it possible that someone will compile this on an architecture that 'likes'
> 16-bit ints to be 32-bit aligned? If such an architecture does not currently
> exist, could it appear in the future?

I'm fairly sure Linux would not be portable to an architecture like this
without a major development effort.

> If the compiler changes the padding in the struct, the driver will
> break, as the struct layout is tightly defined by the anybus spec.
>
> Would it be better to stay safe, and keep __packed in case of such
> tightly defined structures?

Generally I think it does more harm than good. If you require
__packed attributes for correct structure layout,
then only apply it to those members that are not naturally
aligned, and mark the structure itself as __aligned(n) with
the alignment you expect if that makes a difference.

        Arnd

Re: [PATCH anybus v3 4/6] bus: support HMS Anybus-S bus

[Geert Uytterhoeven]

On Fri, Nov 9, 2018 at 10:03 PM Arnd Bergmann <arnd@arndb.de> wrote:
> On Fri, Nov 9, 2018 at 5:25 PM Sven Van Asbroeck <thesven73@gmail.com> wrote:
> > On Thu, Nov 8, 2018 at 9:07 AM Arnd Bergmann <arnd@arndb.de> wrote:
> > > > +struct anybus_mbox_hdr {
> > > > +       u16 id;
> > > > +       u16 info;
> > > > +       u16 cmd_num;
> > > > +       u16 data_size;
> > > > +       u16 frame_count;
> > > > +       u16 frame_num;
> > > > +       u16 offset_high;
> > > > +       u16 offset_low;
> > > > +       u16 extended[8];
> > > > +} __packed;
> > >
> > > I don't think you want this to be __packed, it won't change the layout
> > > but instead force byte accesses on architectures that don't have
> > > fast unaligned load/store instructions.
> > >
> > > Instead of the __packed, it's almost always better to ensure that
> > > the structure itself is aligned to a 16-bit address.
> > >
> >
> > A general question about __packed.
> >
> > My current understanding is this:
> > (please tell me if it's incorrect or incomplete)
> >
> > + without __packed, the compiler is free to pad the struct in whatever
> > way it feels is best.
> > + with __packed, the fields have to be laid out EXACTLY as specified.
>
> It's not up to the compiler but the ELF ABI. The rules are largely consisten
> among the architectures we support, but there are a couple of notable
> exceptions:
>
> - ARM OABI requires 32-bit alignment for structures
> - x86-32 aligns 64-bit members to 32-bit rather than 64-bit
> - m68k has some oddities, I think they can pack certain
>   members (don't remember the details)

M68k aligns 16-bit and larger members to 16-bit.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH anybus v3 4/6] bus: support HMS Anybus-S bus

[Sven Van Asbroeck]

On Thu, Nov 8, 2018 at 9:07 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
>
> > +struct anybuss_host {
> > +       struct device *dev;
> > +       struct anybuss_client *client;
> > +       struct reset_control *reset;
> > +       struct regmap *regmap;
> > +       int irq;
> > +       struct task_struct *qthread;
> > +       wait_queue_head_t wq;
> > +       struct completion card_boot;
> > +       atomic_t ind_ab;
> > +       spinlock_t qlock;
> > +       struct kmem_cache *qcache;
> > +       struct kfifo qs[3];
> > +       struct kfifo *powerq;
> > +       struct kfifo *mboxq;
> > +       struct kfifo *areaq;
> > +       bool power_on;
> > +       bool softint_pending;
> > +       atomic_t dc_event;
> > +       wait_queue_head_t dc_wq;
> > +       atomic_t fieldbus_online;
> > +       struct kernfs_node *fieldbus_online_sd;
> > +};
>
> Similarly, should that anybuss_host sturcture maybe be based on
> a 'struct device' itself? What is the hierarchy of devices in sysfs?
>

The anybus host 'is-a' platform device. The dev member in struct anybuss_host is
actually &pdev->dev.

The host's platform device shows up in sysfs under
/sys/bus/platform/devices/anybuss-host.0.auto
where .0 is the first anybus slot on the bridge, .1 the second slot.
If there is more than one bridge, we'll see .2, .3 etc.

Here are the virtual files in that dir:

drwxr-xr-x    3 root     root             0 Nov 12 11:02
anybuss-host.0.auto.card0
lrwxrwxrwx    1 root     root             0 Nov 12 11:11 driver ->
../../../../../../../bus/platform/drivers/anybuss-host
-rw-r--r--    1 root     root          4096 Nov 12 11:11 driver_override
drwxr-xr-x    3 root     root             0 Nov 12 11:02 misc
-r--r--r--    1 root     root          4096 Nov 12 11:11 modalias
drwxr-xr-x    2 root     root             0 Nov 12 11:11 power
-r--r--r--    1 root     root          4096 Nov 12 11:11 state
lrwxrwxrwx    1 root     root             0 Nov 12 11:11 subsystem ->
../../../../../../../bus/platform
-rw-r--r--    1 root     root          4096 Nov 12 11:02 uevent

Where
anybuss-host.0.auto.card0 just points to the detected anybus card, if any.
Nothing to see there, it's just an artefact of the bus/device abstraction.
misc points to the detected anybus card driver (which is implemented as a
misc class device).

This sysfs hierarchy makes sense to me. Is there something I should be
doing (or not doing) ?

[PATCH anybus v3 5/6] dt-bindings: anybuss-host: document devicetree binding

[thesven73]

From: Sven Van Asbroeck <svendev@arcx.com>

This patch adds devicetree binding documentation for the
Arcx Anybus-S host.

Signed-off-by: Sven Van Asbroeck <svendev@arcx.com>
---
 .../bindings/bus/arcx,anybuss-host.txt        | 36 +++++++++++++++++++
 1 file changed, 36 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/bus/arcx,anybuss-host.txt

diff --git a/Documentation/devicetree/bindings/bus/arcx,anybuss-host.txt b/Documentation/devicetree/bindings/bus/arcx,anybuss-host.txt
new file mode 100644
index 000000000000..5c28e4e09bb2
--- /dev/null
+++ b/Documentation/devicetree/bindings/bus/arcx,anybuss-host.txt
@@ -0,0 +1,36 @@
+* Arcx Anybus-S host
+
+This host communicates with the SoC over a parallel bus. It is
+expected that its Device Tree node is specified as the child of a node
+corresponding to the parallel bus used for communication.
+
+Required properties:
+
+  - compatible : The following string:
+        "arcx,anybuss-host"
+
+  - reg : bus memory area where the Anybus-S host dpram is located.
+
+  - interrupts : interrupt connected to the Anybus-S host interrupt line.
+	Generic interrupt client node bindings are described in
+		interrupt-controller/interrupts.txt
+
+  - resets : the reset line associated with the Anybus-S host.
+
+Example of usage:
+
+This example places the Anybus-S host on top of the i.MX WEIM parallel bus, see:
+Documentation/devicetree/bindings/bus/imx-weim.txt
+
+&weim {
+	anybus-host@0 {
+		compatible = "arcx,anybuss-host";
+		reg = <0 0x400000 0x800>;
+		interrupt-parent = <&gpio1>;
+		interrupts = <1 IRQ_TYPE_LEVEL_LOW>;
+		resets = <&anybus_bridge 0>;
+		/* fsl,weim-cs-timing is a i.MX WEIM bus specific property */
+		fsl,weim-cs-timing = <0x024400b1 0x00001010 0x20081100
+				0x00000000 0xa0000240 0x00000000>;
+	};
+};
-- 
2.17.1

Re: [PATCH anybus v3 5/6] dt-bindings: anybuss-host: document devicetree binding

[Arnd Bergmann]

On Sun, Nov 4, 2018 at 4:55 PM <thesven73@gmail.com> wrote:


> ---
>  .../bindings/bus/arcx,anybuss-host.txt        | 36 +++++++++++++++++++
>  1 file changed, 36 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/bus/arcx,anybuss-host.txt
>
> diff --git a/Documentation/devicetree/bindings/bus/arcx,anybuss-host.txt b/Documentation/devicetree/bindings/bus/arcx,anybuss-host.txt
> new file mode 100644
> index 000000000000..5c28e4e09bb2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/bus/arcx,anybuss-host.txt
> @@ -0,0 +1,36 @@
> +* Arcx Anybus-S host
> +
> +This host communicates with the SoC over a parallel bus. It is
> +expected that its Device Tree node is specified as the child of a node
> +corresponding to the parallel bus used for communication.
> +
> +Required properties:
> +
> +  - compatible : The following string:
> +        "arcx,anybuss-host"
> +
> +  - reg : bus memory area where the Anybus-S host dpram is located.
> +
> +  - interrupts : interrupt connected to the Anybus-S host interrupt line.
> +       Generic interrupt client node bindings are described in
> +               interrupt-controller/interrupts.txt
> +
> +  - resets : the reset line associated with the Anybus-S host.
> +
> +Example of usage:
> +
> +This example places the Anybus-S host on top of the i.MX WEIM parallel bus, see:
> +Documentation/devicetree/bindings/bus/imx-weim.txt

To allow describing connected devices, I think we need a #address-cells
and #size-cells property here, with fixed values. If a device is attached by
a single 32-bit integer, then

 #address-cells = <1>;
 #size-cells = <0>;

should be enough, otherwise you might need additional address cells.
Actually listing the device(s) can be optional, but we do need the
adress in order
to let the bus code associate a device_node pointer with the child
device structure to let a driver read the properties.

       Arnd

Re: [PATCH anybus v3 5/6] dt-bindings: anybuss-host: document devicetree binding

[Sven Van Asbroeck]

Hi Arnd, thank you for the review and the feedback !

>
> To allow describing connected devices, I think we need a #address-cells
> and #size-cells property here, with fixed values.

I'm not sure I understand. Connected devices aren't described in the
devicetree. The anybus specification defines an id register, which is
then used to load the client driver automatically, in the manner of
pci/usb.

In case I have misinterpreted your feedback, could you clarify a bit?
Thanks,
Sven

E.g. here's the definition of an anybus client driver, a profinet card:

static struct anybuss_client_driver profinet_driver = {
    .probe = profinet_probe,
    .remove = profinet_remove,
    .driver = {
        .name   = "hms-profinet",
        .owner = THIS_MODULE,
    },
    .fieldbus_type = 0x0089,
};

static int __init profinet_init(void)
{
    return anybuss_client_driver_register(&profinet_driver);
}
module_init(profinet_init);

Re: [PATCH anybus v3 5/6] dt-bindings: anybuss-host: document devicetree binding

[Arnd Bergmann]

On Thu, Nov 8, 2018 at 3:21 PM Sven Van Asbroeck <thesven73@gmail.com> wrote:
>
> Hi Arnd, thank you for the review and the feedback !
>
> >
> > To allow describing connected devices, I think we need a #address-cells
> > and #size-cells property here, with fixed values.
>
> I'm not sure I understand. Connected devices aren't described in the
> devicetree. The anybus specification defines an id register, which is
> then used to load the client driver automatically, in the manner of
> pci/usb.
>
> In case I have misinterpreted your feedback, could you clarify a bit?

This is related to my reply on another patch of this series. I think it's
likely that you need to be able to describe anybus devices in DT
for additional properties in some cases, even if the common case
doesn't need it.

We do the same thing on PCI and USB, where normally everything
is probed through hardware access, but a device driver can look
at dev->of_node to see if that contains any further properties.

       Arnd

Re: [PATCH anybus v3 5/6] dt-bindings: anybuss-host: document devicetree binding

[Sven Van Asbroeck]

On Thu, Nov 8, 2018 at 9:27 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> We do the same thing on PCI and USB, where normally everything
> is probed through hardware access, but a device driver can look
> at dev->of_node to see if that contains any further properties.
>

This is very interesting. Spent some time trying to find an example of this
for pci/usb, but drew a blank. Could you point me to an example?

One of our products has pcie ethernet, but we have to patch it so it can
grab its mac address from the bootloader. You are saying that there is
a standard way to accomplish this?

[PATCH anybus v3 6/6] misc: support HMS Profinet IRT industrial controller

[thesven73]

From: Sven Van Asbroeck <svendev@arcx.com>

The Anybus-S PROFINET IRT communication module provides instant integration
to any Ethernet based LAN via SMTP, FTP, HTTP as well as PROFINET and
Modbus-TCP. Additional protocols can be implemented on top of TCP/IP
or UDP using the transparent socket interface.

Official documentation:
https://www.anybus.com/docs/librariesprovider7/default-document-library
/manuals-design-guides/hms-hmsi-168-52.pdf

This implementation is an Anybus-S client driver, designed to be
instantiated by the Anybus-S bus driver when it discovers the Profinet
card.

If loaded successfully, the driver creates a /dev/profinet%d devnode,
and a /sys/class/misc/profinet%d sysfs subdir:
- the card can be configured with a single, atomic ioctl on the devnode;
- the card's internal dpram is accessed by calling read/write/seek
    on the devnode.
- the card's "fieldbus specific area" properties can be accessed via
    the sysfs dir.

Signed-off-by: Sven Van Asbroeck <svendev@arcx.com>
---
 Documentation/ioctl/ioctl-number.txt |   1 +
 drivers/misc/Kconfig                 |  10 +
 drivers/misc/Makefile                |   1 +
 drivers/misc/hms-profinet.c          | 753 +++++++++++++++++++++++++++
 include/uapi/linux/hms-common.h      |  14 +
 include/uapi/linux/hms-profinet.h    | 102 ++++
 6 files changed, 881 insertions(+)
 create mode 100644 drivers/misc/hms-profinet.c
 create mode 100644 include/uapi/linux/hms-common.h
 create mode 100644 include/uapi/linux/hms-profinet.h

diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
index 13a7c999c04a..a389a4ec1429 100644
--- a/Documentation/ioctl/ioctl-number.txt
+++ b/Documentation/ioctl/ioctl-number.txt
@@ -241,6 +241,7 @@ Code  Seq#(hex)	Include File		Comments
 					<http://web.archive.org/web/*/http://mikonos.dia.unisa.it/tcfs>
 'l'	40-7F	linux/udf_fs_i.h	in development:
 					<http://sourceforge.net/projects/linux-udf/>
+'l'	80-9F	linux/hms-profinet.h	Anybus-S
 'm'	00-09	linux/mmtimer.h		conflict!
 'm'	all	linux/mtio.h		conflict!
 'm'	all	linux/soundcard.h	conflict!
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 3726eacdf65d..cf1ac5784048 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -406,6 +406,16 @@ config SPEAR13XX_PCIE_GADGET
 	 entry will be created for that controller. User can use these
 	 sysfs node to configure PCIe EP as per his requirements.
 
+config HMS_PROFINET
+	tristate "HMS Profinet IRT Controller (Anybus-S)"
+	select HMS_ANYBUSS_HOST
+	help
+	 If you say yes here you get support for the HMS Industrial
+	 Networks Profinet IRT Controller.
+	 This driver can also be built as a module. If so, the module
+	 will be called hms-profinet.
+	 If unsure, say N.
+
 config VMWARE_BALLOON
 	tristate "VMware Balloon Driver"
 	depends on VMWARE_VMCI && X86 && HYPERVISOR_GUEST
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index af22bbc3d00c..dcf0468187b6 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -34,6 +34,7 @@ obj-$(CONFIG_SENSORS_TSL2550)	+= tsl2550.o
 obj-$(CONFIG_DS1682)		+= ds1682.o
 obj-$(CONFIG_C2PORT)		+= c2port/
 obj-$(CONFIG_HMC6352)		+= hmc6352.o
+obj-$(CONFIG_HMS_PROFINET)	+= hms-profinet.o
 obj-y				+= eeprom/
 obj-y				+= cb710/
 obj-$(CONFIG_SPEAR13XX_PCIE_GADGET)	+= spear13xx_pcie_gadget.o
diff --git a/drivers/misc/hms-profinet.c b/drivers/misc/hms-profinet.c
new file mode 100644
index 000000000000..dd6c0f715bf1
--- /dev/null
+++ b/drivers/misc/hms-profinet.c
@@ -0,0 +1,753 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * HMS Profinet Client Driver
+ *
+ * Copyright (C) 2018 Arcx Inc
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/idr.h>
+#include <linux/miscdevice.h>
+
+#include <linux/anybuss-client.h>
+#include <uapi/linux/hms-profinet.h>
+
+#define PROFI_DPRAM_SIZE	512
+
+/*
+ *  --------------------------------------------------------------
+ * Anybus Profinet mailbox messages - definitions
+ * --------------------------------------------------------------
+ */
+
+/*
+ * note that we're depending on the layout of these structures being
+ * exactly as advertised - which means they need to be packed.
+ */
+
+struct msgEthConfig {
+	u32 ip_addr, subnet_msk, gateway_addr;
+} __packed;
+
+struct msgMacAddr {
+	u8 addr[6];
+} __packed;
+
+struct msgStr {
+	char	s[128];
+} __packed;
+
+struct msgShortStr {
+	char	s[64];
+} __packed;
+
+struct msgHicp {
+	char	enable;
+} __packed;
+
+/*
+ * --------------------------------------------------------------
+ * Fieldbus Specific Area - memory locations
+ * --------------------------------------------------------------
+ */
+#define FSA_NETWORK_STATUS	0x700
+#define FSA_LAYER_STATUS	0x7B2
+#define FSA_IO_CTRL_STATUS	0x7B0
+#define FSA_LAYER_FAULT_CODE	0x7B4
+
+struct profi_priv {
+	struct anybuss_client *client;
+	int id;
+	atomic_t refcount;
+	char node_name[16];
+	struct miscdevice misc;
+	struct device *dev;	/* just a link to the misc device */
+	struct mutex enable_lock;
+};
+
+static int profinet_configure(struct anybuss_client *ab,
+				struct ProfinetConfig *cfg)
+{
+	int ret;
+
+	if (cfg->eth.is_valid) {
+		struct msgEthConfig msg = {
+			.ip_addr = cfg->eth.ip_addr,
+			.subnet_msk = cfg->eth.subnet_msk,
+			.gateway_addr = cfg->eth.gateway_addr,
+		};
+		ret = anybuss_send_msg(ab, 0x0001, &msg, sizeof(msg));
+		if (ret)
+			return ret;
+	}
+	if (cfg->dev_id.is_valid) {
+		u16 ext[2] = {
+			cpu_to_be16(cfg->dev_id.vendorid),
+			cpu_to_be16(cfg->dev_id.deviceid)
+		};
+		ret = anybuss_send_ext(ab, 0x0102, ext, sizeof(ext));
+		if (ret)
+			return ret;
+	}
+	if (cfg->station_name.is_valid) {
+		struct msgStr msg = { 0 };
+
+		strncpy(msg.s, cfg->station_name.name, sizeof(msg.s));
+		ret = anybuss_send_msg(ab, 0x0103, &msg, sizeof(msg));
+		if (ret)
+			return ret;
+	}
+	if (cfg->station_type.is_valid) {
+		struct msgShortStr msg = { 0 };
+
+		strncpy(msg.s, cfg->station_type.name, sizeof(msg.s));
+		ret = anybuss_send_msg(ab, 0x0104, &msg, sizeof(msg));
+		if (ret)
+			return ret;
+	}
+	if (cfg->mac_addr.is_valid) {
+		struct msgMacAddr msg = { 0 };
+
+		memcpy(msg.addr, cfg->mac_addr.addr, sizeof(msg.addr));
+		ret = anybuss_send_msg(ab, 0x0019, &msg, sizeof(msg));
+		if (ret)
+			return ret;
+	}
+	if (cfg->host_domain.is_valid) {
+		size_t len;
+		struct msgStr msg = { 0 };
+		/*
+		 * check if host and domain names fit in msg structure
+		 */
+		len =	strnlen(cfg->host_domain.hostname,
+				sizeof(cfg->host_domain.hostname))
+					+ 1 +
+			strnlen(cfg->host_domain.domainname,
+				sizeof(cfg->host_domain.domainname))
+					+ 1;
+		if (len > sizeof(msg.s))
+			return -ENAMETOOLONG;
+		strncpy(msg.s, cfg->host_domain.hostname,
+			sizeof(msg.s));
+		len = strnlen(msg.s, sizeof(msg.s)) + 1; /* NULL term */
+		strncpy(msg.s + len, cfg->host_domain.domainname,
+				sizeof(msg.s) - len);
+		ret = anybuss_send_msg(ab, 0x0032, &msg, sizeof(msg));
+		if (ret)
+			return ret;
+	}
+	if (cfg->hicp.is_valid) {
+		struct msgHicp msg = {
+			.enable = cfg->hicp.enable ? 1 : 0,
+		};
+		ret = anybuss_send_msg(ab, 0x0013, &msg, sizeof(msg));
+		if (ret)
+			return ret;
+	}
+	if (cfg->web_server.is_valid) {
+		ret = anybuss_send_msg(ab,
+			cfg->web_server.enable ? 0x0005 : 0x0004,
+			NULL, 0);
+		if (ret)
+			return ret;
+	}
+	if (cfg->ftp_server.disable) {
+		ret = anybuss_send_msg(ab, 0x0006, NULL, 0);
+		if (ret)
+			return ret;
+	}
+	if (cfg->global_admin_mode.enable) {
+		ret = anybuss_send_msg(ab, 0x000B, NULL, 0);
+		if (ret)
+			return ret;
+	}
+	if (cfg->vfs.disable) {
+		ret = anybuss_send_msg(ab, 0x0011, NULL, 0);
+		if (ret)
+			return ret;
+	}
+	if (cfg->stop_mode.is_valid) {
+		u16 action;
+
+		switch (cfg->stop_mode.action) {
+		case HMS_SMA_CLEAR:
+			action = 0;
+			break;
+		case HMS_SMA_FREEZE:
+			action = 1;
+			break;
+		case HMS_SMA_SET:
+			action = 2;
+			break;
+		default:
+			return -EINVAL;
+		}
+		action = cpu_to_be16(action);
+		ret = anybuss_send_ext(ab, 0x0101, &action,
+						sizeof(action));
+		if (ret)
+			return ret;
+	}
+	if (cfg->snmp_system_descr.is_valid) {
+		struct msgStr msg = { 0 };
+
+		strncpy(msg.s, cfg->snmp_system_descr.description,
+					sizeof(msg.s));
+		ret = anybuss_send_msg(ab, 0x0120, &msg, sizeof(msg));
+		if (ret)
+			return ret;
+	}
+	if (cfg->snmp_iface_descr.is_valid) {
+		struct msgStr msg = { 0 };
+
+		strncpy(msg.s, cfg->snmp_iface_descr.description,
+					sizeof(msg.s));
+		ret = anybuss_send_msg(ab, 0x0121, &msg, sizeof(msg));
+		if (ret)
+			return ret;
+	}
+	if (cfg->mib2_system_descr.is_valid) {
+		struct msgStr msg = { 0 };
+
+		strncpy(msg.s, cfg->mib2_system_descr.description,
+					sizeof(msg.s));
+		ret = anybuss_send_msg(ab, 0x0124, &msg, sizeof(msg));
+		if (ret)
+			return ret;
+	}
+	if (cfg->mib2_system_contact.is_valid) {
+		struct msgStr msg = { 0 };
+
+		strncpy(msg.s, cfg->mib2_system_contact.contact,
+					sizeof(msg.s));
+		ret = anybuss_send_msg(ab, 0x0125, &msg, sizeof(msg));
+		if (ret)
+			return ret;
+	}
+	if (cfg->mib2_system_location.is_valid) {
+		struct msgStr msg = { 0 };
+
+		strncpy(msg.s, cfg->mib2_system_location.location,
+					sizeof(msg.s));
+		ret = anybuss_send_msg(ab, 0x0126, &msg, sizeof(msg));
+		if (ret)
+			return ret;
+	}
+	return 0;
+}
+
+static int profinet_enable(struct profi_priv *priv,
+				struct ProfinetConfig *cfg)
+{
+	int ret;
+	struct anybuss_client *client = priv->client;
+
+	/* Initialization Sequence, Generic Anybus Mode */
+	const struct anybuss_memcfg mem_cfg = {
+		.input_io = 220,
+		.input_dpram = PROFI_DPRAM_SIZE,
+		.input_total = PROFI_DPRAM_SIZE,
+		.output_io = 220,
+		.output_dpram = PROFI_DPRAM_SIZE,
+		.output_total = PROFI_DPRAM_SIZE,
+		.offl_mode = AB_OFFL_MODE_CLEAR,
+	};
+	if (mutex_lock_interruptible(&priv->enable_lock))
+		return -ERESTARTSYS;
+	/*
+	 * switch anybus off then on, this ensures we can do a complete
+	 * configuration cycle in case anybus was already on.
+	 */
+	anybuss_set_power(client, false);
+	ret = anybuss_set_power(client, true);
+	if (ret)
+		goto err_init;
+	ret = anybuss_start_init(client, &mem_cfg);
+	if (ret)
+		goto err_init;
+	if (cfg)
+		ret = profinet_configure(client, cfg);
+	if (ret)
+		goto err_init;
+	ret = anybuss_finish_init(client);
+	if (ret)
+		goto err_init;
+	mutex_unlock(&priv->enable_lock);
+	return 0;
+err_init:
+	anybuss_set_power(client, false);
+	mutex_unlock(&priv->enable_lock);
+	return ret;
+}
+
+static int profinet_disable(struct profi_priv *priv)
+{
+	int ret;
+
+	if (mutex_lock_interruptible(&priv->enable_lock))
+		return -ERESTARTSYS;
+	ret = anybuss_set_power(priv->client, false);
+	mutex_unlock(&priv->enable_lock);
+	return ret;
+}
+
+static int fbctrl_readw(struct anybuss_client *client, u16 addr)
+{
+	int ret;
+	u16 val;
+
+	ret = anybuss_read_fbctrl(client, addr, &val, sizeof(val));
+	if (ret < 0)
+		return ret;
+	return (int)be16_to_cpu(val);
+}
+
+static ssize_t mac_addr_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct profi_priv *priv = dev_get_drvdata(dev);
+	struct msgMacAddr response;
+	int ret;
+
+	ret = anybuss_recv_msg(priv->client, 0x0010, &response,
+						sizeof(response));
+	if (ret)
+		return ret;
+	return snprintf(buf, PAGE_SIZE, "%02X:%02X:%02X:%02X:%02X:%02X\n",
+		response.addr[0], response.addr[1],
+		response.addr[2], response.addr[3],
+		response.addr[4], response.addr[5]);
+}
+
+static DEVICE_ATTR_RO(mac_addr);
+
+static ssize_t start_defaults_store(struct device *dev,
+	struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct profi_priv *priv = dev_get_drvdata(dev);
+	unsigned long num;
+
+	if (kstrtoul(buf, 0, &num))
+		return -EINVAL;
+	if (num)
+		profinet_enable(priv, NULL);
+	return count;
+}
+
+static DEVICE_ATTR_WO(start_defaults);
+
+static ssize_t ip_addr_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct profi_priv *priv = dev_get_drvdata(dev);
+	struct msgEthConfig response;
+	int ret;
+
+	ret = anybuss_recv_msg(priv->client, 0x0002, &response,
+						sizeof(response));
+	if (ret)
+		return ret;
+	return snprintf(buf, PAGE_SIZE, "%d.%d.%d.%d\n",
+		response.ip_addr & 0xFF,
+		(response.ip_addr >>  8) & 0xFF,
+		(response.ip_addr >> 16) & 0xFF,
+		(response.ip_addr >> 24) & 0xFF);
+}
+
+static DEVICE_ATTR_RO(ip_addr);
+
+static ssize_t subnet_mask_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct profi_priv *priv = dev_get_drvdata(dev);
+	struct msgEthConfig response;
+	int ret;
+
+	ret = anybuss_recv_msg(priv->client, 0x0002, &response,
+						sizeof(response));
+	if (ret)
+		return ret;
+	return snprintf(buf, PAGE_SIZE, "%d.%d.%d.%d\n",
+		response.subnet_msk & 0xFF,
+		(response.subnet_msk >>  8) & 0xFF,
+		(response.subnet_msk >> 16) & 0xFF,
+		(response.subnet_msk >> 24) & 0xFF);
+}
+
+static DEVICE_ATTR_RO(subnet_mask);
+
+static ssize_t gateway_addr_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct profi_priv *priv = dev_get_drvdata(dev);
+	struct msgEthConfig response;
+	int ret;
+
+	ret = anybuss_recv_msg(priv->client, 0x0002, &response,
+						sizeof(response));
+	if (ret)
+		return ret;
+	return snprintf(buf, PAGE_SIZE, "%d.%d.%d.%d\n",
+		response.gateway_addr & 0xFF,
+		(response.gateway_addr >>  8) & 0xFF,
+		(response.gateway_addr >> 16) & 0xFF,
+		(response.gateway_addr >> 24) & 0xFF);
+}
+
+static DEVICE_ATTR_RO(gateway_addr);
+
+static ssize_t hostname_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct profi_priv *priv = dev_get_drvdata(dev);
+	struct msgStr response;
+	int ret;
+
+	ret = anybuss_recv_msg(priv->client, 0x0034, &response,
+						sizeof(response));
+	if (ret)
+		return ret;
+	return snprintf(buf, PAGE_SIZE, "%s\n", response.s);
+}
+
+static DEVICE_ATTR_RO(hostname);
+
+static ssize_t domainname_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct profi_priv *priv = dev_get_drvdata(dev);
+	struct msgStr response;
+	int ret, pos;
+
+	ret = anybuss_recv_msg(priv->client, 0x0034, &response,
+						sizeof(response));
+	if (ret)
+		return ret;
+	/*
+	 * domain name string located right behind null-terminated
+	 * host name string.
+	 */
+	pos = strnlen(response.s, sizeof(response.s)) + 1;
+	if (pos >= sizeof(response.s))
+		return -ENAMETOOLONG;
+	return snprintf(buf, PAGE_SIZE, "%s\n", response.s + pos);
+}
+
+static DEVICE_ATTR_RO(domainname);
+
+static ssize_t network_link_on_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct profi_priv *priv = dev_get_drvdata(dev);
+	int ns;
+
+	ns = fbctrl_readw(priv->client, FSA_NETWORK_STATUS);
+	if (ns < 0)
+		return ns;
+	return snprintf(buf, PAGE_SIZE, "%d\n", ns & 1);
+}
+
+static DEVICE_ATTR_RO(network_link_on);
+
+static ssize_t network_ip_in_use_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct profi_priv *priv = dev_get_drvdata(dev);
+	int ns;
+
+	ns = fbctrl_readw(priv->client, FSA_NETWORK_STATUS);
+	if (ns < 0)
+		return ns;
+	return snprintf(buf, PAGE_SIZE, "%d\n", (ns>>1) & 1);
+}
+
+static DEVICE_ATTR_RO(network_ip_in_use);
+
+static ssize_t layer_status_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct profi_priv *priv = dev_get_drvdata(dev);
+	const char *s;
+	int ls;
+
+	ls = fbctrl_readw(priv->client, FSA_LAYER_STATUS);
+	if (ls < 0)
+		return ls;
+	switch (ls) {
+	case 0x0000:
+		s = "not yet initialized";
+		break;
+	case 0x0001:
+		s = "successfully initialized";
+		break;
+	case 0x0002:
+		s = "failed to initialize";
+		break;
+	default:
+		return -EINVAL;
+	}
+	return snprintf(buf, PAGE_SIZE, "%s\n", s);
+}
+
+static DEVICE_ATTR_RO(layer_status);
+
+static ssize_t io_controller_status_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	struct profi_priv *priv = dev_get_drvdata(dev);
+	const char *s;
+	int w;
+
+	w = fbctrl_readw(priv->client, FSA_IO_CTRL_STATUS);
+	if (w < 0)
+		return w;
+	switch (w) {
+	case 0x0000:
+		s = "No connection made";
+		break;
+	case 0x0001:
+		s = "STOP";
+		break;
+	case 0x0002:
+		s = "RUN";
+		break;
+	case 0x0004:
+		s = "STATION OK";
+		break;
+	case 0x0008:
+		s = "STATION PROBLEM";
+		break;
+	case 0x0010:
+		s = "PRIMARY";
+		break;
+	case 0x0020:
+		s = "BACKUP";
+		break;
+	default:
+		return -EINVAL;
+	}
+	return snprintf(buf, PAGE_SIZE, "%s\n", s);
+}
+
+static DEVICE_ATTR_RO(io_controller_status);
+
+static ssize_t layer_fault_code_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	int fc;
+	struct profi_priv *priv = dev_get_drvdata(dev);
+
+	fc = fbctrl_readw(priv->client, FSA_LAYER_FAULT_CODE);
+	if (fc < 0)
+		return fc;
+	return snprintf(buf, PAGE_SIZE, "%d\n", fc);
+}
+
+static DEVICE_ATTR_RO(layer_fault_code);
+
+static struct attribute *ctrl_attrs[] = {
+	&dev_attr_mac_addr.attr,
+	&dev_attr_start_defaults.attr,
+	&dev_attr_ip_addr.attr,
+	&dev_attr_subnet_mask.attr,
+	&dev_attr_gateway_addr.attr,
+	&dev_attr_hostname.attr,
+	&dev_attr_domainname.attr,
+	&dev_attr_network_link_on.attr,
+	&dev_attr_network_ip_in_use.attr,
+	&dev_attr_io_controller_status.attr,
+	&dev_attr_layer_status.attr,
+	&dev_attr_layer_fault_code.attr,
+	NULL
+};
+
+static struct attribute_group ctrl_group = { .attrs = ctrl_attrs };
+
+struct profi_open_file {
+	struct profi_priv *priv;
+	int event;
+};
+
+static int profi_open(struct inode *node, struct file *filp)
+{
+	struct profi_open_file *of;
+	struct profi_priv *priv = container_of(filp->private_data,
+					struct profi_priv, misc);
+
+	of = kzalloc(sizeof(*of), GFP_KERNEL);
+	if (!of)
+		return -ENOMEM;
+	of->priv = priv;
+	filp->private_data = of;
+	atomic_inc(&priv->refcount);
+	return 0;
+}
+
+static int profi_release(struct inode *node, struct file *filp)
+{
+	struct profi_open_file *of = filp->private_data;
+	struct profi_priv *priv = of->priv;
+
+	kfree(of);
+	if (!atomic_dec_and_test(&priv->refcount))
+		return 0;
+	return profinet_disable(priv);
+}
+
+static long profi_ioctl(struct file *filp, unsigned int cmd,
+						unsigned long arg)
+{
+	struct profi_open_file *of = filp->private_data;
+	struct profi_priv *priv = of->priv;
+	void __user *argp = (void __user *)arg;
+	struct ProfinetConfig config;
+
+	if (_IOC_TYPE(cmd) != PROFINET_IOC_MAGIC)
+		return -EINVAL;
+	if (!(_IOC_DIR(cmd) & _IOC_WRITE))
+		return -EINVAL;
+	switch (cmd) {
+	case PROFINET_IOCSETCONFIG:
+		if (copy_from_user(&config, argp, sizeof(config)))
+			return -EFAULT;
+		return profinet_enable(priv, &config);
+	default:
+		break;
+	}
+	return -ENOTTY;
+}
+
+static ssize_t
+profi_read(struct file *filp, char __user *buf, size_t size,
+							loff_t *offset)
+{
+	struct profi_open_file *of = filp->private_data;
+	struct profi_priv *priv = of->priv;
+
+	return anybuss_read_output(priv->client, &of->event, buf, size,
+							offset);
+}
+
+static ssize_t
+profi_write(struct file *filp, const char __user *buf, size_t size,
+							loff_t *offset)
+{
+	struct profi_open_file *of = filp->private_data;
+	struct profi_priv *priv = of->priv;
+
+	return anybuss_write_input(priv->client, buf, size, offset);
+}
+
+static unsigned int profi_poll(struct file *filp, poll_table *wait)
+{
+	struct profi_open_file *of = filp->private_data;
+	struct profi_priv *priv = of->priv;
+
+	return anybuss_poll(priv->client, of->event, filp, wait);
+}
+
+static const struct file_operations fops = {
+	.open = profi_open,
+	.release = profi_release,
+	.read = profi_read,
+	.write = profi_write,
+	.unlocked_ioctl = profi_ioctl,
+	.poll = profi_poll,
+	.llseek = generic_file_llseek,
+	.owner = THIS_MODULE,
+};
+
+static DEFINE_IDA(profi_index_ida);
+
+static int profinet_probe(struct anybuss_client *client)
+{
+	struct profi_priv *priv;
+	struct device *dev = &client->dev;
+	int err;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+	atomic_set(&priv->refcount, 0);
+	mutex_init(&priv->enable_lock);
+	priv->client = client;
+	priv->misc.minor = MISC_DYNAMIC_MINOR;
+	priv->id = ida_simple_get(&profi_index_ida, 0, 0, GFP_KERNEL);
+	if (priv->id < 0)
+		return priv->id;
+	snprintf(priv->node_name, sizeof(priv->node_name), "profinet%d",
+				priv->id);
+	priv->misc.name = priv->node_name;
+	priv->misc.fops = &fops;
+	priv->misc.parent = client->dev.parent;
+	err = misc_register(&priv->misc);
+	if (err < 0) {
+		dev_err(dev, "could not register device (%d)", err);
+		goto err_ida;
+	}
+	priv->dev = priv->misc.this_device;
+	dev_set_drvdata(priv->dev, priv);
+	err = sysfs_create_group(&priv->dev->kobj, &ctrl_group);
+	if (err < 0) {
+		dev_err(dev, "could not create sysfs group (%d)", err);
+		goto err_register;
+	}
+	dev_info(priv->dev, "detected on %s", dev_name(&client->dev));
+	anybuss_set_drvdata(client, priv);
+	return 0;
+err_register:
+	misc_deregister(&priv->misc);
+err_ida:
+	ida_simple_remove(&profi_index_ida, priv->id);
+	return err;
+}
+
+static int profinet_remove(struct anybuss_client *client)
+{
+	struct profi_priv *priv = anybuss_get_drvdata(client);
+
+	sysfs_remove_group(&priv->dev->kobj, &ctrl_group);
+	misc_deregister(&priv->misc);
+	ida_simple_remove(&profi_index_ida, priv->id);
+	return 0;
+}
+
+static struct anybuss_client_driver profinet_driver = {
+	.probe = profinet_probe,
+	.remove = profinet_remove,
+	.driver		= {
+		.name   = "hms-profinet",
+		.owner	= THIS_MODULE,
+	},
+	.fieldbus_type = 0x0089,
+};
+
+static int __init profinet_init(void)
+{
+	return anybuss_client_driver_register(&profinet_driver);
+}
+module_init(profinet_init);
+
+static void __exit profinet_exit(void)
+{
+	return anybuss_client_driver_unregister(&profinet_driver);
+}
+module_exit(profinet_exit);
+
+MODULE_AUTHOR("Sven Van Asbroeck <svendev@arcx.com>");
+MODULE_DESCRIPTION("HMS Profinet IRT Driver (Anybus-S)");
+MODULE_LICENSE("GPL v2");
diff --git a/include/uapi/linux/hms-common.h b/include/uapi/linux/hms-common.h
new file mode 100644
index 000000000000..4b69963a3863
--- /dev/null
+++ b/include/uapi/linux/hms-common.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright 2018 Archronix Corp. All Rights Reserved.
+ *
+ */
+
+#ifndef _UAPILINUX_HMSCOMMON_H_
+#define _UAPILINUX_HMSCOMMON_H_
+
+#define HMS_SMA_CLEAR		0
+#define HMS_SMA_FREEZE		1
+#define HMS_SMA_SET		2
+
+#endif /* _UAPILINUX_HMSCOMMON_H_ */
diff --git a/include/uapi/linux/hms-profinet.h b/include/uapi/linux/hms-profinet.h
new file mode 100644
index 000000000000..e53aaeb3ce22
--- /dev/null
+++ b/include/uapi/linux/hms-profinet.h
@@ -0,0 +1,102 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright 2018 Archronix Corp. All Rights Reserved.
+ *
+ */
+
+#ifndef _UAPILINUX_PROFINET_H_
+#define _UAPILINUX_PROFINET_H_
+
+#include <linux/types.h>
+#include <linux/hms-common.h>
+
+#define PROFI_CFG_STRLEN	64
+
+struct ProfinetConfig {
+	struct {
+		/* addresses IN NETWORK ORDER! */
+		__u32 ip_addr;
+		__u32 subnet_msk;
+		__u32 gateway_addr;
+		__u8  is_valid:1;
+	} eth;
+	struct {
+		__u16 vendorid, deviceid;
+		__u8  is_valid:1;
+	} dev_id;
+	struct {
+		char name[PROFI_CFG_STRLEN];
+		__u8 is_valid:1;
+	} station_name;
+	struct {
+		char name[PROFI_CFG_STRLEN];
+		__u8 is_valid:1;
+	} station_type;
+	struct {
+		__u8 addr[6];
+		__u8 is_valid:1;
+	} mac_addr;
+	struct {
+		char hostname[PROFI_CFG_STRLEN];
+		char domainname[PROFI_CFG_STRLEN];
+		__u8 is_valid:1;
+	} host_domain;
+	struct {
+		__u8 enable:1;
+		__u8 is_valid:1;
+	} hicp;
+	struct {
+		__u8 enable:1;
+		__u8 is_valid:1;
+	} web_server;
+	struct {
+		__u8 disable:1;
+	} ftp_server;
+	struct {
+		__u8 enable:1;
+	} global_admin_mode;
+	struct {
+		__u8 disable:1;
+	} vfs;
+	struct {
+		/* one of HMS_SMA_CLEAR/FREEZE/SET */
+		int action;
+		__u8 is_valid:1;
+	} stop_mode;
+	struct {
+		char description[PROFI_CFG_STRLEN];
+		__u8 is_valid:1;
+	} snmp_system_descr;
+	struct {
+		char description[PROFI_CFG_STRLEN];
+		__u8 is_valid:1;
+	} snmp_iface_descr;
+	struct {
+		char description[PROFI_CFG_STRLEN];
+		__u8 is_valid:1;
+	} mib2_system_descr;
+	struct {
+		char contact[PROFI_CFG_STRLEN];
+		__u8 is_valid:1;
+	} mib2_system_contact;
+	struct {
+		char location[PROFI_CFG_STRLEN];
+		__u8 is_valid:1;
+	} mib2_system_location;
+	/*
+	 * use non-volatile defaults for any properties not specified.
+	 * when in doubt, keep this OFF.
+	 */
+	__u8 use_nv_defaults:1;
+};
+
+#define PROFINET_IOC_MAGIC 'l'
+
+/*
+ * Configures profinet according to the ProfinetConfig structure, and
+ * switches the card on if it was previously off.
+ */
+#define PROFINET_IOCSETCONFIG   _IOW(PROFINET_IOC_MAGIC, 0x80,\
+						struct ProfinetConfig)
+
+#endif /* _UAPILINUX_PROFINET_H_ */
-- 
2.17.1

Re: [PATCH anybus v3 6/6] misc: support HMS Profinet IRT industrial controller

[Arnd Bergmann]

On Sun, Nov 4, 2018 at 4:55 PM <thesven73@gmail.com> wrote:

> +
> +struct msgEthConfig {
> +       u32 ip_addr, subnet_msk, gateway_addr;
> +} __packed;

The __packed attribute makes it slower to access but doesn't
change the layout, so better drop it.

> +struct msgMacAddr {
> +       u8 addr[6];
> +} __packed;
> +
> +struct msgStr {
> +       char    s[128];
> +} __packed;
> +
> +struct msgShortStr {
> +       char    s[64];
> +} __packed;
> +
> +struct msgHicp {
> +       char    enable;
> +} __packed;

The __packed on these ones has no effect, and can just be dropped
for readability.

> +static ssize_t mac_addr_show(struct device *dev,
> +                               struct device_attribute *attr,
> +                               char *buf)
> +{
> +       struct profi_priv *priv = dev_get_drvdata(dev);
> +       struct msgMacAddr response;
> +       int ret;
> +
> +       ret = anybuss_recv_msg(priv->client, 0x0010, &response,
> +                                               sizeof(response));
> +       if (ret)
> +               return ret;
> +       return snprintf(buf, PAGE_SIZE, "%02X:%02X:%02X:%02X:%02X:%02X\n",
> +               response.addr[0], response.addr[1],
> +               response.addr[2], response.addr[3],
> +               response.addr[4], response.addr[5]);
> +}
> +
> +static DEVICE_ATTR_RO(mac_addr);

> +static ssize_t ip_addr_show(struct device *dev,
> +                               struct device_attribute *attr,
> +                               char *buf)
> +{

I don't understand much about field bus, but I have a general feeling
that if you configure a mac address and IP address, this should
be connect ed to the network layer in some form, possibly being
exposed as a network device and manipulated using netlink
instead of sysfs or ioctl.

Can you explain how this fits together and why you got the the
sysfs interface?

> +struct ProfinetConfig {

The CamelCase naming seems odd here.

> +       struct {
> +               /* addresses IN NETWORK ORDER! */
> +               __u32 ip_addr;
> +               __u32 subnet_msk;
> +               __u32 gateway_addr;
> +               __u8  is_valid:1;
> +       } eth;

This is where packing might make sense, since you have
padding fields in a user space structure ;-) . It would be better
to just avoid the implicit padding though and name all fields.

Overall, this structure feels too complex for an ioctl interface,
with the nested structures. If we end up keeping that
ioctl, maybe at least make it a flat structure without padding,
or alternatively make it a set of separate ioctls.

      Arnd

Re: [PATCH anybus v3 6/6] misc: support HMS Profinet IRT industrial controller

[Sven Van Asbroeck]

Hi Arnd,

These are great questions, I had been wondering when they would be brought up :)

On Thu, Nov 8, 2018 at 9:20 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> I don't understand much about field bus, but I have a general feeling
> that if you configure a mac address and IP address, this should
> be connect ed to the network layer in some form, possibly being
> exposed as a network device and manipulated using netlink
> instead of sysfs or ioctl.
>
> Can you explain how this fits together and why you got the the
> sysfs interface?

I started typing here how a fieldbus differs from a networking
device, and how it does not fit in the kernel abstraction for
networking devices.

Then I started explaining the "roll my own" userspace API I created
for fieldbus devices... and it hit me. There is already a fieldbus API !

include/linux/uio_driver.h

Let me go and check how well it fits with what my device needs.


> > +       struct {
> > +               /* addresses IN NETWORK ORDER! */
> > +               __u32 ip_addr;
> > +               __u32 subnet_msk;
> > +               __u32 gateway_addr;
> > +               __u8  is_valid:1;
> > +       } eth;
>
> Overall, this structure feels too complex for an ioctl interface,
> with the nested structures. If we end up keeping that
> ioctl, maybe at least make it a flat structure without padding,
> or alternatively make it a set of separate ioctls.
>

I agree that it feels complex, but it's the best I could come up with.
There are a few hidden constraints:

1. The profinet card configuration settings must be applied atomically.
And they can only be applied right after device reset.

So if we use smaller, separate ioctls, we will end up resetting the device
each time we send an ioctl. And to assemble a real configuration, we need
5 or 6 ioctls, so the card gets reset 5 or 6 times, which takes ages.
It cannot work this way.

2. Configuration settings are optional. That's why why each little struct
has an is_valid member. If we use a flatter structure, we need a bool for
every config setting in the struct, to indicate if it should be applied.
Which feels clunky.

One way to overcome this is by letting the ioctls change data in the driver,
but not on the card. Then a separate "apply" ioctl could apply the whole
configuration atomically.

Example:
ioctl(clear all settings?);
ioctl(set ip address);
ioctl(set stop mode action);
ioctl(enable internal webserver);
ioctl(apply);

But of course, what happens if two processes try to configure the
driver at the same time?
The ioctl calls would be interleaved, and the result would be very messy...

There must be a better way?

Re: [PATCH anybus v3 6/6] misc: support HMS Profinet IRT industrial controller

[Arnd Bergmann]

On Thu, Nov 8, 2018 at 4:35 PM Sven Van Asbroeck <thesven73@gmail.com> wrote:
> On Thu, Nov 8, 2018 at 9:20 AM Arnd Bergmann <arnd@arndb.de> wrote:
 > > +       struct {
> > > +               /* addresses IN NETWORK ORDER! */
> > > +               __u32 ip_addr;
> > > +               __u32 subnet_msk;
> > > +               __u32 gateway_addr;
> > > +               __u8  is_valid:1;
> > > +       } eth;
> >
> > Overall, this structure feels too complex for an ioctl interface,
> > with the nested structures. If we end up keeping that
> > ioctl, maybe at least make it a flat structure without padding,
> > or alternatively make it a set of separate ioctls.
> >
>
> I agree that it feels complex, but it's the best I could come up with.
> There are a few hidden constraints:
>
> 1. The profinet card configuration settings must be applied atomically.
> And they can only be applied right after device reset.
>
> So if we use smaller, separate ioctls, we will end up resetting the device
> each time we send an ioctl. And to assemble a real configuration, we need
> 5 or 6 ioctls, so the card gets reset 5 or 6 times, which takes ages.
> It cannot work this way.
>
> 2. Configuration settings are optional. That's why why each little struct
> has an is_valid member. If we use a flatter structure, we need a bool for
> every config setting in the struct, to indicate if it should be applied.
> Which feels clunky.

I think a more common way to do this would be to use a __u64
member containing a bitmask of which fields are valid.

> One way to overcome this is by letting the ioctls change data in the driver,
> but not on the card. Then a separate "apply" ioctl could apply the whole
> configuration atomically.
>
> Example:
> ioctl(clear all settings?);
> ioctl(set ip address);
> ioctl(set stop mode action);
> ioctl(enable internal webserver);
> ioctl(apply);
>
> But of course, what happens if two processes try to configure the
> driver at the same time?
> The ioctl calls would be interleaved, and the result would be very messy...
>
> There must be a better way?

In particular the bits about optional fields would fit much better
into netlink, which is a kind of TLV interface, and you can send
a number of configuration steps in one 'sendmsg' syscall,
but leave out the ones that you are not interested in.

This would also allow you to specify standard parameters that
could apply to multiple vendors or fieldbus protocols, and have
a common configuration tool for all of them.

       Arnd

Re: [PATCH anybus v3 0/6] Support HMS Profinet Card over Anybus

[Sven Van Asbroeck]

Arnd, Rob, Linus,

Many thanks for your constructive feedback so far !

Is there anything in general about this set that would prevent it from being
mainlined? Perhaps I am trying to do too much at once, dropping a patchset
that is too complex to be properly reviewed?

I've been thinking about reworking the host to its simplest, yet feature-
complete form. Just serialize all card accesses with a single lock.
Then the kernel thread, kqueue, kcache, kref etc would all disappear.
The price we pay is a reduction in performance/parallelism.
We could then increase parallelism at a later stage.
Would that be of any help?

Comments on v3 will go into v4 shortly.
(Rob, I'm not sure how to address your feedback, other than to s/host/slot/g ?)

Yours,
Sven

Re: [PATCH anybus v3 0/6] Support HMS Profinet Card over Anybus

[Arnd Bergmann]

On Fri, Nov 9, 2018 at 5:02 PM Sven Van Asbroeck <thesven73@gmail.com> wrote:
>
> Arnd, Rob, Linus,
>
> Many thanks for your constructive feedback so far !
>
> Is there anything in general about this set that would prevent it from being
> mainlined? Perhaps I am trying to do too much at once, dropping a patchset
> that is too complex to be properly reviewed?

As usual, it comes down to the user space interfaces I think. Designing
a user interface is hard, most importantly because you cannot change it
once anyone starts relying on it, as opposed to implementation details
that you are free to change at any point.

It's also hard to find reviewers for it, the best you can hope for
is that you find people that have both knowledge about the type of
hardware you work with and Linux user interface design, and
get them to review the code.

> I've been thinking about reworking the host to its simplest, yet feature-
> complete form. Just serialize all card accesses with a single lock.
> Then the kernel thread, kqueue, kcache, kref etc would all disappear.
> The price we pay is a reduction in performance/parallelism.
> We could then increase parallelism at a later stage.
> Would that be of any help?

I don't think that's a major issue. If you are happy with the
implementation there, and you use the interfaces correctly, that
won't stop the driver from getting merged, even if you could
rework it to use something else later.

      Arnd

Re: [PATCH anybus v3 0/6] Support HMS Profinet Card over Anybus

[Sven Van Asbroeck]

On Fri, Nov 9, 2018 at 4:22 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
>
> As usual, it comes down to the user space interfaces I think. Designing
> a user interface is hard, most importantly because you cannot change it
> once anyone starts relying on it, as opposed to implementation details
> that you are free to change at any point.

Thanks for the feedback !
I will rework v4 so it uses a userspace interface shared with other
fieldbus drivers - uio.
Let's see how it goes.

Re: [PATCH anybus v3 0/6] Support HMS Profinet Card over Anybus

[Arnd Bergmann]

On Fri, Nov 9, 2018 at 10:47 PM Sven Van Asbroeck <thesven73@gmail.com> wrote:
>
> On Fri, Nov 9, 2018 at 4:22 PM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> >
> > As usual, it comes down to the user space interfaces I think. Designing
> > a user interface is hard, most importantly because you cannot change it
> > once anyone starts relying on it, as opposed to implementation details
> > that you are free to change at any point.
>
> Thanks for the feedback !
> I will rework v4 so it uses a userspace interface shared with other
> fieldbus drivers - uio.

You mentioned this before, but I don't see how this would actually
work. Isn't uio just a slim wrapper for allowing user space access
to hardware registers while avoiding a kernel driver?

      Arnd