Support HMS Profinet Card over Anybus
cover Support HMS Profinet Card over Anybus
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 an mfd, which instantiates two anybus slots.
v1:
first shot
Sven Van Asbroeck (4):
mfd: support the Arcx anybus bridge.
dt-bindings: anybus-bridge: document devicetree binding.
bus: support HMS Anybus-S bus.
misc: support HMS Profinet IRT industrial controller.
.../bindings/mfd/arcx,anybus-bridge.txt | 37 + .../devicetree/bindings/vendor-prefixes.txt | 1 + drivers/bus/Kconfig | 11 + drivers/bus/Makefile | 1 + drivers/bus/anybuss-host.c | 1301 +++++++++++++++++ drivers/mfd/Kconfig | 11 + drivers/mfd/Makefile | 1 + drivers/mfd/anybus-bridge.c | 441 ++++++ drivers/misc/Kconfig | 11 + drivers/misc/Makefile | 1 + drivers/misc/hms-profinet.c | 747 ++++++++++ include/linux/anybuss-client.h | 100 ++ include/linux/anybuss-host.h | 28 + include/uapi/linux/hms-common.h | 14 + include/uapi/linux/hms-profinet.h | 101 ++ 15 files changed, 2806 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/arcx,anybus-bridge.txt create mode 100644 drivers/bus/anybuss-host.c create mode 100644 drivers/mfd/anybus-bridge.c create mode 100644 drivers/misc/hms-profinet.c create mode 100644 include/linux/anybuss-client.h create mode 100644 include/linux/anybuss-host.h create mode 100644 include/uapi/linux/hms-common.h create mode 100644 include/uapi/linux/hms-profinet.h
Signature · 1 line
2.17.1
PATCH 1/4 mfd: support the Arcx anybus bridge.
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 the i.MX parallel WEIM bus. There is also a CAN power readout, unrelated to the anybus.
--- drivers/mfd/Kconfig | 11 + drivers/mfd/Makefile | 1 + drivers/mfd/anybus-bridge.c | 441 +++++++++++++++++++++++++++++++++++ include/linux/anybuss-host.h | 28 +++ 4 files changed, 481 insertions(+) create mode 100644 drivers/mfd/anybus-bridge.c create mode 100644 include/linux/anybuss-host.h
drivers/mfd/Kconfig +11 -0
@@ -125,6 +125,17 @@ config MFD_ATMEL_SMC bool select MFD_SYSCON +config MFD_ANYBUS_BRIDGE + tristate "Arcx Anybus-S Bridge" + select MFD_CORE + select REGMAP + depends on OF + help + Select this to get support for the Arcx Anybus bridge. + It is accessible via the i.MX parallel WEIM bus, and + embeds up to two Anybus-S application connectors (slots). + There is also a CAN power readout, unrelated to the anybus. + config MFD_BCM590XX tristate "Broadcom BCM590xx PMUs" select MFD_CORE
drivers/mfd/Makefile +1 -0
@@ -223,6 +223,7 @@ obj-$(CONFIG_MFD_HI655X_PMIC) += hi655x-pmic.o obj-$(CONFIG_MFD_DLN2) += dln2.o obj-$(CONFIG_MFD_RT5033) += rt5033.o obj-$(CONFIG_MFD_SKY81452) += sky81452.o +obj-$(CONFIG_MFD_ANYBUS_BRIDGE) += anybus-bridge.o intel-soc-pmic-objs := intel_soc_pmic_core.o intel_soc_pmic_crc.o obj-$(CONFIG_INTEL_SOC_PMIC) += intel-soc-pmic.o
drivers/mfd/anybus-bridge.c new file +441 -0
441 lines not on this page · raw message
include/linux/anybuss-host.h new file +28 -0
@@ -0,0 +1,28 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* +* Anybus-S host adapter definitions +* +* Copyright 2018 Arcx Inc +*/ + +#ifndef __LINUX_ANYBUSS_HOST_H__ +#define __LINUX_ANYBUSS_HOST_H__ + +#include <linux/regmap.h> + +typedef void (*anybuss_reset_t )(struct device *dev,bool reset); + +/** +* Platform data of the Anybus-S host controller. +* +* @regmap: provides access to the card dpram. +* MUST NOT use caching +* MUST NOT sleep +* @reset: controls the card reset line. +*/ +struct anybuss_host_pdata { +struct regmap *regmap; +anybuss_reset_t reset; +}; + +#endif /* __LINUX_ANYBUS_S_HOST_H__ */
Signature · 1 line
2.17.1
PATCH 1/4 mfd: support the Arcx anybus bridge.
Hi-
Quoted 24 lines · Sven Van Asbroeck
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 the i.MX parallel WEIM bus. There is also a CAN power readout, unrelated to the anybus.
Signed-off-by Sven Van Asbroeck <[email protected]>--- drivers/mfd/Kconfig | 11 + drivers/mfd/Makefile | 1 + drivers/mfd/anybus-bridge.c | 441 +++++++++++++++++++++++++++++++++++ include/linux/anybuss-host.h | 28 +++ 4 files changed, 481 insertions(+) create mode 100644 drivers/mfd/anybus-bridge.c create mode 100644 include/linux/anybuss-host.hdrivers/mfd/Kconfig +0
-0@@ -125,6 +125,17 @@ config MFD_ATMEL_SMC bool select MFD_SYSCON
drivers/mfd/Kconfig config MFD_ATMEL_SMC · line 137
Quoted 10 lines
drivers/mfd/Kconfig +10
-0+config MFD_ANYBUS_BRIDGE + tristate "Arcx Anybus-S Bridge" + select MFD_CORE + select REGMAP + depends on OF + help + Select this to get support for the Arcx Anybus bridge. + It is accessible via the i.MX parallel WEIM bus, and + embeds up to two Anybus-S application connectors (slots). + There is also a CAN power readout, unrelated to the anybus.
Anybus.
Quoted 36 lines
drivers/mfd/Kconfig +1
-0+ config MFD_BCM590XX tristate "Broadcom BCM590xx PMUs" select MFD_COREdrivers/mfd/anybus-bridge.c new file +25
-0@@ -0,0 +1,441 @@ +// 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.h> +#include <linux/of_gpio.h> +#include <linux/of_address.h> +#include <linux/pwm.h> +#include <linux/mfd/core.h> +#include <linux/of_irq.h> +#include <linux/delay.h> +#include <linux/regmap.h> +#include <linux/idr.h> +#include <linux/spinlock.h> + +#include <linux/anybuss-host.h> +
[snip]
drivers/mfd/anybus-bridge.c line 74
Quoted 8 lines
drivers/mfd/anybus-bridge.c new file +8
-0+ +static struct regmap *create_weim_regmap (struct device *dev, +struct resource *res,int slot) +{ +struct regmap_config regmap_cfg = { + .reg_bits =11 , + .val_bits =8 , +/* bus accesses are simple weim byte accesses.
WEIM
drivers/mfd/anybus-bridge.c line 78
Quoted 4 lines
drivers/mfd/anybus-bridge.c new file +4
-0+* they don't require any synchronization. +* also, the bus driver requirement is that regmap accesses +* must never sleep. +*/
Kernel multi-line comment style (except in networking code) is like:
/* * bus accesses are ... * must never sleep. */
Quoted 33 lines
drivers/mfd/anybus-bridge.c new file +33
-0+ .disable_locking = true, + .reg_read = read_reg_weim, + .reg_write = write_reg_weim, + }; +void __iomem *base; +char name[32 ]; + +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; +/* give the regmap a name, so it shows up in debugfs */ +snprintf (name,sizeof (name),"slot%d" , slot); + regmap_cfg.name =devm_kmemdup (dev, name,sizeof (name),GFP_KERNEL ); +if (regmap_cfg.name ==NULL ) +return ERR_PTR (-ENOMEM ); +return devm_regmap_init (dev,NULL , base, ®map_cfg); +} + +static int add_anybus_slot (struct device *dev,anybuss_reset_t reset, +int slot) +{ +int err, irq; +struct resource mem_res, *irq_res; +struct mfd_cell *cell; +struct anybuss_host_pdata *pdata; +struct gpio_desc *gpio; +struct regmap *regmap; + +/* get irq from devicetree */ + gpio =devm_gpiod_get_index (dev,"irq" , slot,GPIOD_IN ); +if (IS_ERR (gpio))
drivers/mfd/anybus-bridge.c line 121
Quoted 10 lines
drivers/mfd/anybus-bridge.c new file +10
-0+return PTR_ERR (gpio); + irq =gpiod_to_irq (gpio); +if (irq <0 ) { +dev_err (dev,"Anybus-S slot %d: no irq?" , slot); +return -EINVAL ; + } +/* get anybus mem resource from devicetree +* note that the cpld registers sit at dt offset 0 +* anybus slot memory starts at offset 1 +*/
comment style.
Quoted 50 lines
drivers/mfd/anybus-bridge.c new file +50
-0+ err =of_address_to_resource (dev->of_node, slot+1 , &mem_res); +if (err) { +dev_err (dev,"Anybus-S slot %d: no weim memory?" , slot); +return err; + } + regmap =create_weim_regmap (dev, &mem_res, slot); +if (IS_ERR (regmap)) +return PTR_ERR (regmap); +/* add slot as a mfd device */ + irq_res =devm_kzalloc (dev,sizeof (*irq_res),GFP_KERNEL ); +if (!irq_res) +return -ENOMEM ; + irq_res->start = irq_res->end = irq; + irq_res->flags =IORESOURCE_IRQ ; + irq_res->name ="anybus-irq" ; + cell =devm_kzalloc (dev,sizeof (*cell),GFP_KERNEL ); +if (!cell) +return -ENOMEM ; + pdata =devm_kzalloc (dev,sizeof (*pdata),GFP_KERNEL ); +if (!pdata) +return -ENOMEM ; + pdata->reset = reset; + pdata->regmap = regmap; + cell->name ="anybuss-host" ; + cell->num_resources =1 ; + cell->resources = irq_res; + cell->platform_data = pdata; + cell->pdata_size =sizeof (*pdata); +dev_info (dev,"Anybus-S slot %d: [weim 0x%016x-0x%016x] [irq %d]" , + slot, mem_res.start, mem_res.end, irq); + err =devm_mfd_add_devices (dev,PLATFORM_DEVID_AUTO , cell,1 , +NULL ,0 ,NULL ); +if (err) +dev_err (dev,"failed to add Anybus-S slot %d" , slot); +return err; +} + +static int add_anybus_slots (struct device *dev,struct device_node *np, +anybuss_reset_t *resets) +{ +int i, err; + +/* bridge has two Anybus-S slots */ +for (i =0 ; i <2 ; i++) { + err =add_anybus_slot (dev, resets[i], i); +if (err) +return err; + } +return 0 ; +}
drivers/mfd/anybus-bridge.c line 181
Quoted 10 lines
drivers/mfd/anybus-bridge.c new file +10
-0+ +static void do_reset (struct device *dev,u8 rst_bit,bool reset) +{ +unsigned long flags; +struct bridge_priv *cd =dev_get_drvdata (dev); + +spin_lock_irqsave (&cd->regs_lock, flags); +/* CPLD_CONTROL is write-only, so cache its value in +* cd->control_reg +*/
comment style.
drivers/mfd/anybus-bridge.c line 194
Quoted 13 lines
drivers/mfd/anybus-bridge.c new file +13
-0+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: +* EIM bus 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. +*/
comment style.
Quoted 18 lines
drivers/mfd/anybus-bridge.c new file +5
-0+if (reset) +udelay (100 ); +spin_unlock_irqrestore (&cd->regs_lock, flags); +} +include/linux/anybuss-host.h new file +6
-0@@ -0,0 +1,28 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* +* Anybus-S host adapter definitions +* +* Copyright 2018 Arcx Inc +*/
include/linux/anybuss-host.h line 16
Quoted 10 lines
include/linux/anybuss-host.h new file +10
-0+ +#ifndef __LINUX_ANYBUSS_HOST_H__ +#define __LINUX_ANYBUSS_HOST_H__ + +#include <linux/regmap.h> + +typedef void (*anybuss_reset_t )(struct device *dev,bool reset); + +/** +* Platform data of the Anybus-S host controller.
* struct anybuss_host_pdata - Platform data of the Anybus-S host controller.
to convert this comment block to kernel-doc notation.
Quoted 12 lines
include/linux/anybuss-host.h new file +11
-0+* +* @regmap: provides access to the card dpram. +* MUST NOT use caching +* MUST NOT sleep +* @reset: controls the card reset line. + */> +struct anybuss_host_pdata { +struct regmap *regmap; +anybuss_reset_t reset; +}; + +#endif /* __LINUX_ANYBUS_S_HOST_H__ */
Signature · 1 line
~Randy
Re: [PATCH anybus v1 1/4] mfd: support the Arcx anybus bridge.
In reply to Sven Van AsbroeckQuoted 7 lines · Sven Van Asbroeck
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 the i.MX parallel WEIM bus. There is also a CAN power readout, unrelated to the anybus.
Signed-off-by Sven Van Asbroeck <[email protected]>
Quoted 8 lines
--- drivers/mfd/Kconfig | 11 + drivers/mfd/Makefile | 1 + drivers/mfd/anybus-bridge.c | 441 +++++++++++++++++++++++++++++++++++ include/linux/anybuss-host.h | 28 +++ 4 files changed, 481 insertions(+) create mode 100644 drivers/mfd/anybus-bridge.c create mode 100644 include/linux/anybuss-host.h
Wow, this driver is going to need a lot of work.
But before we get going, how many sub-devices does this have?
Signature · 4 lines
Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH anybus v1 1/4] mfd: support the Arcx anybus bridge.
Quoted 1 line
Wow, this driver is going to need a lot of work.
I'm open to reworking this 5 times if need be :)
Quoted 2 lines
But before we get going, how many sub-devices does this have?
This is a custom piece of h/w which exposes two anybus-S slots, plus a power readout in sysfs, I wasn't quite sure if mfd is the right abstraction for this.
PATCH 2/4 dt-bindings: anybus-bridge: document devicetree binding.
In reply to Sven Van AsbroeckThis patch adds devicetree binding documentation for the Arcx anybus bridge.
--- .../bindings/mfd/arcx,anybus-bridge.txt | 37 +++++++++++++++++++ .../devicetree/bindings/vendor-prefixes.txt | 1 + 2 files changed, 38 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/arcx,anybus-bridge.txt
Documentation/devicetree/bindings/mfd/arcx,anybus-bridge.txt new file +37 -0
@@ -0,0 +1,37 @@ +* Arcx anybus bridge + +This chip communicates with the SoC over the WEIM bus. It is +expected that its Device Tree node is specified as the child of a node +corresponding to the WEIM bus used for communication. + +Required properties: + + - compatible : The following chip-specific string: + "arcx,anybus-bridge" + + - reg : + weim memory area where the cpld registers are located, followed by: + weim memory area of the first anybus-s slot, followed by: + weim memory area of the second anybus-s slot. + + - enable-gpios : the gpio connected to the bridge's 'enable gpio'. + + - pwms : the pwm connected to the bridge's 'pwm input'. + + - irq-gpios : the gpios connected to the bridge's interrupt lines. + note that there is no need to provide the 'interrupts' property here. + +Example of usage: + +&weim { + bridge@0,0 { + compatible = "arcx,anybus-bridge"; + reg = <0 0 0x100>, <0 0x400000 0x800>, <1 0x400000 0x800>; + fsl,weim-cs-timing = <0x024400b1 0x00001010 0x20081100 + 0x00000000 0xa0000240 0x00000000>; + enable-gpios = <&gpio5 2 GPIO_ACTIVE_HIGH>; + pwms = <&pwm3 0 571>; + irq-gpios = <&gpio1 1 GPIO_ACTIVE_HIGH>, + <&gpio1 5 GPIO_ACTIVE_HIGH>; + }; +};
Documentation/devicetree/bindings/vendor-prefixes.txt +1 -0
@@ -35,6 +35,7 @@ aptina Aptina Imaging arasan Arasan Chip Systems archermind ArcherMind Technology (Nanjing) Co., Ltd. arctic Arctic Sand +arcx Arcx/Archronix Inc. aries Aries Embedded GmbH arm ARM Ltd. armadeus ARMadeus Systems SARL
Signature · 1 line
2.17.1
Rob Herring
maintainerRe: [PATCH anybus v1 2/4] dt-bindings: anybus-bridge: document devicetree binding.
Quoted 16 lines · Sven Van Asbroeck
This patch adds devicetree binding documentation for the Arcx anybus bridge.
Signed-off-by Sven Van Asbroeck <[email protected]>--- .../bindings/mfd/arcx,anybus-bridge.txt | 37 +++++++++++++++++++ .../devicetree/bindings/vendor-prefixes.txt | 1 + 2 files changed, 38 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/arcx,anybus-bridge.txtDocumentation/devicetree/bindings/mfd/arcx,anybus-bridge.txt new file +0
-0
Documentation/devicetree/bindings/mfd/arcx,anybus-bridge.txt line 5
Quoted 5 lines
Documentation/devicetree/bindings/mfd/arcx,anybus-bridge.txt new file +5
-0@@ -0,0 +1,37 @@ +* Arcx anybus bridge + +This chip communicates with the SoC over the WEIM bus. It is +expected that its Device Tree node is specified as the child of a node +corresponding to the WEIM bus used for communication.
By WEIM you are referring to i.MX external parallel bus? Presumably this can work on any parallel bus, not just i.MX.
Documentation/devicetree/bindings/mfd/arcx,anybus-bridge.txt line 10
Quoted 5 lines
Documentation/devicetree/bindings/mfd/arcx,anybus-bridge.txt new file +5
-0+ +Required properties: + + - compatible : The following chip-specific string: + "arcx,anybus-bridge"
No version or part number?
Documentation/devicetree/bindings/mfd/arcx,anybus-bridge.txt line 22
Quoted 12 lines
Documentation/devicetree/bindings/mfd/arcx,anybus-bridge.txt new file +12
-0+ + - reg : + weim memory area where the cpld registers are located, followed by: + weim memory area of the first anybus-s slot, followed by: + weim memory area of the second anybus-s slot. + + - enable-gpios : the gpio connected to the bridge's 'enable gpio'. + + - pwms : the pwm connected to the bridge's 'pwm input'. + + - irq-gpios : the gpios connected to the bridge's interrupt lines. + note that there is no need to provide the 'interrupts' property here.
Why not? This should not be base on what the OS currently happens to support.
What are the functions of each line? The order must be defined.
Quoted 30 lines
Documentation/devicetree/bindings/mfd/arcx,anybus-bridge.txt new file +15
-0+ +Example of usage: + +&weim { + bridge@0,0 { + compatible = "arcx,anybus-bridge"; + reg = <0 0 0x100>, <0 0x400000 0x800>, <1 0x400000 0x800>; + fsl,weim-cs-timing = <0x024400b1 0x00001010 0x20081100 + 0x00000000 0xa0000240 0x00000000>; + enable-gpios = <&gpio5 2 GPIO_ACTIVE_HIGH>; + pwms = <&pwm3 0 571>; + irq-gpios = <&gpio1 1 GPIO_ACTIVE_HIGH>, + <&gpio1 5 GPIO_ACTIVE_HIGH>; + }; +};Documentation/devicetree/bindings/vendor-prefixes.txt +1
-0@@ -35,6 +35,7 @@ aptina Aptina Imaging arasan Arasan Chip Systems archermind ArcherMind Technology (Nanjing) Co., Ltd. arctic Arctic Sand +arcx Arcx/Archronix Inc. aries Aries Embedded GmbH arm ARM Ltd. armadeus ARMadeus Systems SARLSignature · 1 line
2.17.1
Re: [PATCH anybus v1 2/4] dt-bindings: anybus-bridge: document devicetree binding.
In reply to Sven Van AsbroeckQuoted 33 lines · Sven Van Asbroeck
This patch adds devicetree binding documentation for the Arcx anybus bridge.
Signed-off-by Sven Van Asbroeck <[email protected]>--- .../bindings/mfd/arcx,anybus-bridge.txt | 37 +++++++++++++++++++ .../devicetree/bindings/vendor-prefixes.txt | 1 + 2 files changed, 38 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/arcx,anybus-bridge.txtDocumentation/devicetree/bindings/mfd/arcx,anybus-bridge.txt new file +17
-0@@ -0,0 +1,37 @@ +* Arcx anybus bridge + +This chip communicates with the SoC over the WEIM bus. It is +expected that its Device Tree node is specified as the child of a node +corresponding to the WEIM bus used for communication. + +Required properties: + + - compatible : The following chip-specific string: + "arcx,anybus-bridge" + + - reg : + weim memory area where the cpld registers are located, followed by: + weim memory area of the first anybus-s slot, followed by: + weim memory area of the second anybus-s slot. + + - enable-gpios : the gpio connected to the bridge's 'enable gpio'.
Documentation/devicetree/bindings/mfd/arcx,anybus-bridge.txt line 27
Quoted 10 lines
Documentation/devicetree/bindings/mfd/arcx,anybus-bridge.txt new file +10
-0+ + - pwms : the pwm connected to the bridge's 'pwm input'. + + - irq-gpios : the gpios connected to the bridge's interrupt lines. + note that there is no need to provide the 'interrupts' property here. + +Example of usage: + +&weim { + bridge@0,0 {
I haven't seen this syntax before.
It doesn't mean it's wrong, but will needs Rob et. al to cast an eye.
Documentation/devicetree/bindings/mfd/arcx,anybus-bridge.txt line 31
Quoted 4 lines
Documentation/devicetree/bindings/mfd/arcx,anybus-bridge.txt new file +4
-0+ compatible = "arcx,anybus-bridge"; + reg = <0 0 0x100>, <0 0x400000 0x800>, <1 0x400000 0x800>; + fsl,weim-cs-timing = <0x024400b1 0x00001010 0x20081100 + 0x00000000 0xa0000240 0x00000000>;
This needs to be documented.
Documentation/devicetree/bindings/mfd/arcx,anybus-bridge.txt line 35
Quoted 4 lines
Documentation/devicetree/bindings/mfd/arcx,anybus-bridge.txt new file +4
-0+ enable-gpios = <&gpio5 2 GPIO_ACTIVE_HIGH>; + pwms = <&pwm3 0 571>; + irq-gpios = <&gpio1 1 GPIO_ACTIVE_HIGH>, + <&gpio1 5 GPIO_ACTIVE_HIGH>;
Tabbing.
Quoted 14 lines
Documentation/devicetree/bindings/mfd/arcx,anybus-bridge.txt new file +2
-0+ }; +};Documentation/devicetree/bindings/vendor-prefixes.txt +1
-0@@ -35,6 +35,7 @@ aptina Aptina Imaging arasan Arasan Chip Systems archermind ArcherMind Technology (Nanjing) Co., Ltd. arctic Arctic Sand +arcx Arcx/Archronix Inc. aries Aries Embedded GmbH arm ARM Ltd. armadeus ARMadeus Systems SARL
Signature · 4 lines
Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH anybus v1 2/4] dt-bindings: anybus-bridge: document devicetree binding.
In reply to Sven Van AsbroeckHi Sven,
thanks for your patch!
Quoted 1 line · Sven Van Asbroeck
+ - pwms : the pwm connected to the bridge's 'pwm input'.
That is really unintuitive and needs a detailed explanation. What is a bridge doing with a PWM? Is it 100% certain this is a PWM, it's not just a .... clock? A pwm is a pule WIDTH modulator and I can't for my life understand why a bus bridge needs a signal with variable pulse width, but surprise me! :D
Documentation/devicetree/bindings/mfd/arcx,anybus-bridge.txt line 31
Quoted 2 lines
Documentation/devicetree/bindings/mfd/arcx,anybus-bridge.txt new file +2
-0+ fsl,weim-cs-timing = <0x024400b1 0x00001010 0x20081100 + 0x00000000 0xa0000240 0x00000000>;
Is it just a copy/paste from Documentation/devicetree/bindings/bus/imx-weim.txt leftover?
It was mentioned as undocumented but it is also pretty terse with these opaque hex numbers. I would never let the Freescale binding pass if I was reviewing it.
Look at my bindings for Qualcomm EBI in Documentation/devicetree/bindings/bus/qcom,ebi2.txt for example.
But maybe it's not even supposed to be there.
Yours,
Linus Walleij
PATCH 3/4 bus: support HMS Anybus-S bus.
In reply to Sven Van AsbroeckThe 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
This driver implementation is designed to be almost completely independent from the way the Anybus-S hardware is accessed by the SoC. All it needs is:
- a regmap which accesses the underlying Anybus-S dpram by whatever means;
- an interrupt line, ultimately connected to the Anybus-S card;
- a reset function.
--- drivers/bus/Kconfig | 11 + drivers/bus/Makefile | 1 + drivers/bus/anybuss-host.c | 1301 ++++++++++++++++++++++++++++++++ include/linux/anybuss-client.h | 100 +++ 4 files changed, 1413 insertions(+) create mode 100644 drivers/bus/anybuss-host.c create mode 100644 include/linux/anybuss-client.h
drivers/bus/Kconfig +11 -0
@@ -45,6 +45,17 @@ 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 + default n + 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
drivers/bus/Makefile +1 -0
@@ -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
drivers/bus/anybuss-host.c new file +1301 -0
1,301 lines not on this page · raw message
include/linux/anybuss-client.h new file +100 -0
100 lines not on this page · raw message
Signature · 1 line
2.17.1
PATCH 3/4 bus: support HMS Anybus-S bus.
drivers/bus/Kconfig config IMX_WEIM · line 52
Quoted 13 lines · Sven Van Asbroeck
drivers/bus/Kconfig +5
-0@@ -45,6 +45,17 @@ 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 + default n
Please drop the "default n". That is already the default in Kconfig files.
drivers/bus/Kconfig config IMX_WEIM · line 61
Quoted 9 lines
drivers/bus/Kconfig +6
-0+ 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
Also please check the multi-line comments for kernel comment style.
thanks,
Signature · 1 line
~Randy
Re: [PATCH anybus v1 3/4] bus: support HMS Anybus-S bus.
In reply to Sven Van AsbroeckHi Sven,
thanks for your patch!
Quoted 6 lines · Sven Van Asbroeck
This driver implementation is designed to be almost completely independent from the way the Anybus-S hardware is accessed by the SoC. All it needs is:
- a regmap which accesses the underlying Anybus-S dpram by whatever means;
- an interrupt line, ultimately connected to the Anybus-S card;
- a reset function.
Overall this commit message is a good start! You explain what this thing is and what it does.
What you need to add is a bit of how the driver is architected. That can also be added as comment in the header of the driver file, maybe that is even better, i.e. here:
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * HMS Anybus-S Host Driver
<architecture description goes here>
This is really needed because the driver is starting threads and running completions and tasks and whatnot to the left and right, and when random people come in to maintain this code they will be puzzled. You need an overarching description of how the driver is constructed here.
Please add proper kerneldoc to the struct anybus_host also "buss" is a germanism isn't it? It should be just "anybus"?
drivers/bus/anybuss-host.c line 207
Quoted 4 lines
drivers/bus/anybuss-host.c new file +4
-0+struct anybuss_host { +struct device *dev; +struct device *parent; +struct anybuss_client *client;
There as well?
Just search/replace "s/buss/bus/g" everywhere I suspect.
Quoted 4 lines
drivers/bus/anybuss-host.c new file +4
-0+static irqreturn_t irq_handler (int irq,void *data) +{ +struct anybuss_host *cd = data; +int ind_ab;
drivers/bus/anybuss-host.c line 294
Quoted 10 lines
drivers/bus/anybuss-host.c new file +10
-0+ +/* reading the anybus indicator register acks the 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 ; +}
It looks a bit like you reinvent threaded interrupts but enlighten me on the architecture and I might be able to get it.
Quoted 23 lines
drivers/bus/anybuss-host.c new file +23
-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); +ab_reset (cd, true); +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 ;
drivers/bus/anybuss-host.c line 344
Quoted 10 lines
drivers/bus/anybuss-host.c new file +10
-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); +ab_reset (cd, false); + t->task_fn = task_fn_power_on_2; +return -EINPROGRESS ; +}
This looks complex. Why can't you just sleep() and then retry this instead of shuffleing around different "tasks"? Are you actually modeling a state machine? In that case I can kind of understand it, then you just need one thread/work and assign it an enum of states or something, maybe name that "state" rather than task so we see what is going on.
drivers/bus/anybuss-host.c line 472
Quoted 12 lines
drivers/bus/anybuss-host.c new file +12
-0+static int task_fn_area_3(struct anybuss_host *cd, struct ab_task *t) +static int task_fn_area_2(struct anybuss_host *cd, struct ab_task *t) +static int task_fn_area(struct anybuss_host *cd, struct ab_task *t) +static struct ab_task * +create_area_reader(struct kmem_cache *qcache, u16 flags, u16 addr, + size_t count) +static struct ab_task * +create_area_writer(struct kmem_cache *qcache, u16 flags, u16 addr, + const void *buf, size_t count) +static struct ab_task * +create_area_user_writer (struct kmem_cache *qcache,u16 flags,u16 addr, +const void __user *buf,size_t count)
So there are many different tasks going on.
Are they just created to get something going in process context?
drivers/bus/anybuss-host.c line 754
Quoted 2 lines
drivers/bus/anybuss-host.c new file +2
-0+static void softint_ack(struct anybuss_host *cd) +static void process_softint(struct anybuss_host *cd)
This looks like MSI (message signalled interrupt) and makes me think that you should probably involve the irqchip maintainers. Interrupts shall be represented in the irqchip abstraction.
drivers/bus/anybuss-host.c line 1048
Quoted 5 lines
drivers/bus/anybuss-host.c new file +5
-0+int anybuss_client_driver_register (struct anybuss_client_driver *drv) +{ + drv->driver.bus = &anybus_bus; +return driver_register (&drv->driver); +}
There is nice use of the bus abstractions here.
Quoted 1 line
+
cd->reset=pdata->reset;
This callback thing to handle reset doesn't seem right.
The kernel has a reset for assert/deassert och just assert() reset handling that you can find in drivers/reset/*
Maybe that is what you should be using instead of
rolling your own reset handling?
Quoted 1 line
+
cd->reset(parent, true);
So this would just be something like
#include <linux/reset.h>
r = devm_reset_control_get_exclusive(dev, id);
ret = reset_control_assert(r);
drivers/bus/anybuss-host.c line 1190
Quoted 3 lines
drivers/bus/anybuss-host.c new file +3
-0+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 ]);
This looks like device-unique data so please do this:
#include <linux/random.h>
add_device_randomness(val, 4);
I guess I can provide better review once you add some information on this task state machine business and how it is engineered, looking forward to the next iteration!
Yours,
Linus Walleij
PATCH 4/4 misc: support HMS Profinet IRT industrial controller.
In reply to Sven Van AsbroeckThe 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.
--- drivers/misc/Kconfig | 11 + drivers/misc/Makefile | 1 + drivers/misc/hms-profinet.c | 747 ++++++++++++++++++++++++++++++ include/uapi/linux/hms-common.h | 14 + include/uapi/linux/hms-profinet.h | 101 ++++ 5 files changed, 874 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
drivers/misc/Kconfig +11 -0
@@ -406,6 +406,17 @@ 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 + default n + 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
drivers/misc/Makefile +1 -0
@@ -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
drivers/misc/hms-profinet.c new file +747 -0
747 lines not on this page · raw message
include/uapi/linux/hms-common.h new file +14 -0
@@ -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_ */
include/uapi/linux/hms-profinet.h new file +101 -0
101 lines not on this page · raw message
Signature · 1 line
2.17.1
Re: [PATCH anybus v1 4/4] misc: support HMS Profinet IRT industrial controller.
Hi,
Quoted 38 lines · Sven Van Asbroeck
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.pdfThis 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 <[email protected]>--- drivers/misc/Kconfig | 11 + drivers/misc/Makefile | 1 + drivers/misc/hms-profinet.c | 747 ++++++++++++++++++++++++++++++ include/uapi/linux/hms-common.h | 14 + include/uapi/linux/hms-profinet.h | 101 ++++ 5 files changed, 874 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.hdrivers/misc/Kconfig +0
-0
drivers/misc/Kconfig config SPEAR13XX_PCIE_GADGET · line 412
Quoted 7 lines
drivers/misc/Kconfig +4
-0@@ -406,6 +406,17 @@ 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 + default n
Please drop the "default n".
drivers/misc/hms-profinet.c
Quoted 16 lines
drivers/misc/Kconfig +7
-0+ 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_GUESTdrivers/misc/hms-profinet.c new file +0
-0
Please check multi-line comment style in this source file.
Quoted 95 lines
include/uapi/linux/hms-profinet.h new file +89
-0@@ -0,0 +1,101 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* +* Copyright 2018 Archronix Corp. All Rights Reserved. +* +*/ + +#ifndef _UAPILINUX_PROFINET_H_ +#define _UAPILINUX_PROFINET_H_ + +#include <asm/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;
include/uapi/linux/hms-profinet.h line 99
Quoted 10 lines
include/uapi/linux/hms-profinet.h new file +10
-0+}; + +#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, 1,\ + struct ProfinetConfig)
ioctl magic numbers should be added toDocumentation/ioctl/ioctl-number.txt, please.
Quoted 3 lines
include/uapi/linux/hms-profinet.h new file +2
-0+ +#endif /* _UAPILINUX_PROFINET_H_ */
Signature · 1 line
~Randy
Quoted 13 lines · Sven Van Asbroeck
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.
Quoted 6 lines
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 an mfd, which instantiates two anybus slots.
I highly recommend to look at my recent (for few month?) reviews -- they are almost repeating each other.
Also check what we have under lib/ (hint: read linux/kernel.h for inline and exported function helpers).
I might look at the patches later on if I would have some spare time (I actually don't know why I'm in Cc list here).
Quoted 38 lines
v1:
first shotSven Van Asbroeck (4):
mfd: support the Arcx anybus bridge.
dt-bindings: anybus-bridge: document devicetree binding.
bus: support HMS Anybus-S bus.
misc: support HMS Profinet IRT industrial controller..../bindings/mfd/arcx,anybus-bridge.txt | 37 + .../devicetree/bindings/vendor-prefixes.txt | 1 + drivers/bus/Kconfig | 11 + drivers/bus/Makefile | 1 + drivers/bus/anybuss-host.c | 1301 +++++++++++++++++ drivers/mfd/Kconfig | 11 + drivers/mfd/Makefile | 1 + drivers/mfd/anybus-bridge.c | 441 ++++++ drivers/misc/Kconfig | 11 + drivers/misc/Makefile | 1 + drivers/misc/hms-profinet.c | 747 ++++++++++ include/linux/anybuss-client.h | 100 ++ include/linux/anybuss-host.h | 28 + include/uapi/linux/hms-common.h | 14 + include/uapi/linux/hms-profinet.h | 101 ++ 15 files changed, 2806 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/arcx,anybus-bridge.txt create mode 100644 drivers/bus/anybuss-host.c create mode 100644 drivers/mfd/anybus-bridge.c create mode 100644 drivers/misc/hms-profinet.c create mode 100644 include/linux/anybuss-client.h create mode 100644 include/linux/anybuss-host.h create mode 100644 include/uapi/linux/hms-common.h create mode 100644 include/uapi/linux/hms-profinet.hSignature · 1 line
2.17.1
Signature · 2 lines
With Best Regards, Andy Shevchenko
Thanks for your feedback, I appreciate it !
Quoted 6 lines · Andy Shevchenko
I highly recommend to look at my recent (for few month?) reviews -- they are almost repeating each other.
Also check what we have under
lib/(hint: readlinux/kernel.hfor inline and exported function helpers).
Would you be able, in a few words, to describe which of your reviews I should concentrate on? Or which /lib files are of interest? I've looked through your reviews and in the /lib directory, but other than refcount_t, nothing jumped out at me.