| Message ID | 1505371738-128683-3-git-send-email-vadimp@mellanox.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream, archived |
| Headers | show |
On Thu, 14 Sep 2017, Vadim Pasternak wrote: > The mlxreg a multifunction device driver handling LEDs, events, exposing > through sysfs reset signal, and reset causes info. These components share > a common register space. > > Signed-off-by: Vadim Pasternak <vadimp@mellanox.com> > --- > v4->v5: > Comments pointed out by Rob: > - Avoid duplications in reg; > - Remove unnecessary details in description; > - Do not use names like phandels; > Changes added by Vadim: > - Combine hotplug nodes interrupt aggregation, register offset and mask > properties into hotplug-spec properties; > - Combine reset, cause, etc subnodes register offset, mask and effective > bit properties into attr-spec properties; > v3->v4: > Comments pointed out by Rob: > - Make a separate patch /devicetree/bindings/vendor-prefixes.txt; > - Add .txt to Documentation/devicetree/bindings/mfd/mellanox,mlxreg-core > and send it within this series; > - Modify "compatible" property; > - Modify explanation for "deferred" property; > - Describe each subnode by its own section; > - Don't use underscore in attribute names; > --- > .../bindings/mfd/mellanox,mlxreg-core.txt | 165 +++++++++++++++++++++ > 1 file changed, 165 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mfd/mellanox,mlxreg-core.txt Wow! I have never seen a binding like this before. > diff --git a/Documentation/devicetree/bindings/mfd/mellanox,mlxreg-core.txt b/Documentation/devicetree/bindings/mfd/mellanox,mlxreg-core.txt > new file mode 100644 > index 0000000..1e3cce5 > --- /dev/null > +++ b/Documentation/devicetree/bindings/mfd/mellanox,mlxreg-core.txt > @@ -0,0 +1,165 @@ > +Mellanox programmable device control. > +------------------------------------- > +This binding defines the device control interface over for Mellanox BMC based > +switches. > + > +Required properties: > +- compatible = "mellanox,mlxreg-i2c" or > + "mellanox,mlxreg-i2c-16" > + > +- #address-cells : must be 1; > +- #size-cells : must be 0; > +- reg : I2C address; > + > +Optional properties: > +- interrupt-parent : phandle of parent interrupt controller; > +- interrupts : interrupt line; > +- deferred : I2C deferred bus phandle; > + I2C bus activation order enforce for the cases when hot-plug > + devices are attached to I2C bus, which is initialized after the > + I2C bus, where programmable device is attached; Dependencies aren't usually expressed in DT. Why can't this be done pragmatically? Usually we try to interrogate a device that we depend on and if it's not present, we return -EPROBE_DEFER and try again latter. > +- hotplug-spec <aggr mask>: > + - aggr : interrupt top aggregation register offset; > + - mask : interrupt top aggregation register mask; What is this? Are you describing register layout in DT? You should use the 'reg' property to describe registers. Bit masks are usually defined in C. > +Optional nodes and their properties: > + - psu : power supply unit nodes: > + Required properties: > + - hotplug-spec <aggr reg mask>: > + - aggr : effective bit in aggregation interrupt mask; > + - reg : interrupt status register offset for all group members; > + - mask : interrupt register mask; > + - hotplugs - list of relevant dynamic device nodes; What is a dynamic device node? I think I saw the code for this? Are you attempting to 'enable' and 'disable' nodes on-the-fly? That's not really what the status property is for. Usually we disable nodes in DTS(I) files which cover many devices, then enable them in more specific device files if they are located on that device. They are not to be used as a flag you just toggle on and off. That's a hack. > + - fan : fan unit nodes: > + Required properties: > + - hotplug-spec <aggr reg mask>: > + - aggr : effective bit in aggregation interrupt mask; > + - reg : interrupt status register offset for all group members; > + - mask : interrupt register mask; > + - hotplugs - list of relevant dynamic device nodes; > + - pwr : power cable nodes: > + Required properties: > + - hotplug-spec <aggr reg mask>: > + - aggr : effective bit in aggregation interrupt mask; > + - reg : interrupt status register offset for all group members; > + - mask : interrupt register mask; > + - hotplugs : list of relevant dynamic device nodes; > + - host : host side nodes (CPU host side for BMC): > + Required properties: > + - hotplug-spec <aggr reg mask>: > + - aggr : effective bit in aggregation interrupt mask; > + - reg : interrupt status register offset for all group members; > + - mask : interrupt register mask; > + - hotplugs : list of relevant dynamic device nodes; > + - asic : asic nodes: > + Required properties: > + - hotplug-spec <aggr reg mask>: > + - aggr : effective bit in aggregation interrupt mask; > + - reg : interrupt status register offsets array per group member; > + - mask : interrupt register mask; > + - hotplugs : list of relevant device nodes; > + - reset : reset nodes for system reset operations: > + Required properties: > + - reset control subnodes: > + Required properties: > + - attr-spec <reg mask>: This property is not defined, but again, 'reg' is the correct property to use. Masks are usually defined in C code. > + - reg : register offset; > + - mask : attribute mask; > + - cause - reset cause nodes for reading system reset cause: > + Required properties: > + - cause info subnodes: > + Required properties: > + - attr-spec <reg mask>: > + - reg : register offset; > + - mask : attribute mask; > + - mux - mux nodes: > + Required properties: > + - mux control subnodes for hardware select operations: > + Required properties: > + - attr-spec <reg mask bit>: > + - reg : register offset; > + - mask : attribute mask; > + - bit : effective bit; > + Optional property: > + - hotplugs : dynamic device node selected by mux; > + - gprw - general purpose read-write register nodes: > + Required properties: > + - general purpose read-write register subnodes: > + Required properties: > + - attr-spec <reg mask>: > + - reg : register offset; > + - mask : attribute mask; > + - gpro - general purpose read only register nodes: > + Required properties: > + - general purpose read only register subnodes: > + Required properties: > + - attr-spec <reg mask>: > + - reg : register offset. > + - mask : attribute mask. > + - led - led nodes for led operations control: > + Required properties: > + - led control nodes: > + Required properties: > + - attr-spec <reg mask>: > + - reg : register offset; > + - mask : attribute mask; > + > +Example: > + mlxcpld-mng-ctrl@71 { > + #address-cells = <1>; > + #size-cells = <0>; > + interrupt-parent = <&gpio>; > + interrupts = <ASPEED_GPIO(S, 1) 2>; > + compatible = "mellanox,mlxcpld-ctrl-i2c"; > + reg = <0x71>; > + deferred = <&i2c6>; > + hotplug-spec = <0x3a 0x4c>; > + > + pwr: { > + hotplug-spec = <0x08 0x64 0x03>; > + hotplugs = <&psu1_ctrl &psu2_ctrl>; > + }; > + > + mux { > + spi_burn_bios_ci { > + attr-spec = <0x32 0xfe 0x00>; > + hotplugs = <&spi2>; > + }; > + }; > + > + led { > + status-green { > + attr-spec = <0x20 0xf0>; > + }; > + status-red { > + attr-spec = <0x20 0xf0>; > + }; > + }; > + > + reset { > + sys_power_cycle { > + attr-spec = <0x30 0xfb>; > + }; > + }; > + > + cause { > + ac_power_cycle { > + attr-spec = <0x1d 0xfe>; > + }; > + sys_pwr_cycle: { > + attr-spec = <0x1e 0xfb>; > + }; > + }; > + > + gpro { > + cpld_mng_version { > + attr-spec <0x00 0xff 0xff>; > + }; > + }; > + > + gprw { > + sw_reset_cause { > + attr-spec = <0x37 0x0f 0x0f>; > + }; > + }; > + };
On Thu, 14 Sep 2017, Lee Jones wrote: > On Thu, 14 Sep 2017, Vadim Pasternak wrote: > > > The mlxreg a multifunction device driver handling LEDs, events, exposing > > through sysfs reset signal, and reset causes info. These components share > > a common register space. > > > > Signed-off-by: Vadim Pasternak <vadimp@mellanox.com> > > --- > > v4->v5: > > Comments pointed out by Rob: > > - Avoid duplications in reg; > > - Remove unnecessary details in description; > > - Do not use names like phandels; > > Changes added by Vadim: > > - Combine hotplug nodes interrupt aggregation, register offset and mask > > properties into hotplug-spec properties; > > - Combine reset, cause, etc subnodes register offset, mask and effective > > bit properties into attr-spec properties; > > v3->v4: > > Comments pointed out by Rob: > > - Make a separate patch /devicetree/bindings/vendor-prefixes.txt; > > - Add .txt to Documentation/devicetree/bindings/mfd/mellanox,mlxreg-core > > and send it within this series; > > - Modify "compatible" property; > > - Modify explanation for "deferred" property; > > - Describe each subnode by its own section; > > - Don't use underscore in attribute names; > > --- > > .../bindings/mfd/mellanox,mlxreg-core.txt | 165 +++++++++++++++++++++ > > 1 file changed, 165 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/mfd/mellanox,mlxreg-core.txt > > Wow! I have never seen a binding like this before. > > > diff --git a/Documentation/devicetree/bindings/mfd/mellanox,mlxreg-core.txt b/Documentation/devicetree/bindings/mfd/mellanox,mlxreg-core.txt > > new file mode 100644 > > index 0000000..1e3cce5 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/mfd/mellanox,mlxreg-core.txt > > @@ -0,0 +1,165 @@ > > +Mellanox programmable device control. > > +------------------------------------- > > +This binding defines the device control interface over for Mellanox BMC based > > +switches. > > + > > +Required properties: > > +- compatible = "mellanox,mlxreg-i2c" or > > + "mellanox,mlxreg-i2c-16" > > + > > +- #address-cells : must be 1; > > +- #size-cells : must be 0; > > +- reg : I2C address; > > + > > +Optional properties: > > +- interrupt-parent : phandle of parent interrupt controller; > > +- interrupts : interrupt line; > > +- deferred : I2C deferred bus phandle; > > + I2C bus activation order enforce for the cases when hot-plug > > + devices are attached to I2C bus, which is initialized after the > > + I2C bus, where programmable device is attached; > > Dependencies aren't usually expressed in DT. > > Why can't this be done pragmatically? > > Usually we try to interrogate a device that we depend on and if it's > not present, we return -EPROBE_DEFER and try again latter. Actually, thinking about this. Are these hierarchical? If so, they would be better represented as a sub-node.
> > + > > +Optional properties: > > +- interrupt-parent : phandle of parent interrupt controller; > > +- interrupts : interrupt line; > > +- deferred : I2C deferred bus phandle; > > + I2C bus activation order enforce for the cases when hot-plug > > + devices are attached to I2C bus, which is initialized after the > > + I2C bus, where programmable device is attached; > > Dependencies aren't usually expressed in DT. > > Why can't this be done pragmatically? > > Usually we try to interrogate a device that we depend on and if it's not > present, we return -EPROBE_DEFER and try again latter. This particular device table is for BMC system with Aspeed2500 SoC. It has 14 i2c busses. I have the cases, like for example the following: programmable device (in this case this is Lattice CPLD) is located on bus 4 and through this device presence of FANs drawers are detected. For which presented unit EEPROM driver should be connected. And this EEPROMs are connected to i2c switch, which is located at bus 6. So I want to enforce initialization order. And in mlxreg-i2c driver, which I removed from the last patch I do it as: child = of_parse_phandle(np, "deferred", 0); if (child) { adapter = of_find_i2c_adapter_by_node(child); of_node_put(child); if (!adapter) return -EPROBE_DEFER; } > > > +- hotplug-spec <aggr mask>: > > + - aggr : interrupt top aggregation register offset; > > + - mask : interrupt top aggregation register mask; > > What is this? Are you describing register layout in DT? > > You should use the 'reg' property to describe registers. > > Bit masks are usually defined in C. This driver is for programmable devices. I have to support about 12-15 1U systems and in the close future several modular systems. This systems can have different number of replaceable units, different LEDs, resets, selects, etc. Register map is different for the different systems. It could be slight Difference (some bits in several registers), and could be a big enough difference. Programmable devices could be connected to i2c bus, to LPC bus, to SPI bus. My intention was to have a driver which doesn't depend on register map layout and on physical bus. In case I'll have bits defined in C, it would be necessary to modify the In the future the driver code for each new system. So, yes, I define several critical group of registers in DTS. And I thought about more or less the same level of register description trough APCI (I need to support ARM and x86 arch). > > > +Optional nodes and their properties: > > + - psu : power supply unit nodes: > > + Required properties: > > + - hotplug-spec <aggr reg mask>: > > + - aggr : effective bit in aggregation interrupt mask; > > + - reg : interrupt status register offset for all group members; > > + - mask : interrupt register mask; > > + - hotplugs - list of relevant dynamic device nodes; > > What is a dynamic device node? Replaceable units, like PSU, FAN, for which on interrupt I connect/ disconnect f.e. EEPROM. Power cables, for which on cable in/out I connect/disconnect PSU controller driver. Network switch device, CPU, for which on up/down I connect/disconnect hwmon drivers (this is BMC side). I also provide support on some special mux selection to open access to remote flashes (BIOS, shared NIC, etc), for the maintenance access. This is the reason I am doing node enabling and disabling on the fly. > > I think I saw the code for this? Are you attempting to 'enable' and 'disable' > nodes on-the-fly? That's not really what the status property is for. Usually > we disable nodes in DTS(I) files which cover many devices, then enable them > in more specific device files if they are located on that device. > > They are not to be used as a flag you just toggle on and off. That's a hack. > > > + - fan : fan unit nodes: > > + Required properties: > > + - hotplug-spec <aggr reg mask>: > > + - aggr : effective bit in aggregation interrupt mask; > > + - reg : interrupt status register offset for all group members; > > + - mask : interrupt register mask; > > + - hotplugs - list of relevant dynamic device nodes; > > + - pwr : power cable nodes: > > + Required properties: > > + - hotplug-spec <aggr reg mask>: > > + - aggr : effective bit in aggregation interrupt mask; > > + - reg : interrupt status register offset for all group members; > > + - mask : interrupt register mask; > > + - hotplugs : list of relevant dynamic device nodes; > > + - host : host side nodes (CPU host side for BMC): > > + Required properties: > > + - hotplug-spec <aggr reg mask>: > > + - aggr : effective bit in aggregation interrupt mask; > > + - reg : interrupt status register offset for all group members; > > + - mask : interrupt register mask; > > + - hotplugs : list of relevant dynamic device nodes; > > + - asic : asic nodes: > > + Required properties: > > + - hotplug-spec <aggr reg mask>: > > + - aggr : effective bit in aggregation interrupt mask; > > + - reg : interrupt status register offsets array per group > member; > > + - mask : interrupt register mask; > > + - hotplugs : list of relevant device nodes; > > + - reset : reset nodes for system reset operations: > > + Required properties: > > + - reset control subnodes: > > + Required properties: > > + - attr-spec <reg mask>: > > This property is not defined, but again, 'reg' is the correct property to use. > Masks are usually defined in C code. > > > + - reg : register offset; > > + - mask : attribute mask; > > + - cause - reset cause nodes for reading system reset cause: > > + Required properties: > > + - cause info subnodes: > > + Required properties: > > + - attr-spec <reg mask>: > > + - reg : register offset; > > + - mask : attribute mask; > > + - mux - mux nodes: > > + Required properties: > > + - mux control subnodes for hardware select operations: > > + Required properties: > > + - attr-spec <reg mask bit>: > > + - reg : register offset; > > + - mask : attribute mask; > > + - bit : effective bit; > > + Optional property: > > + - hotplugs : dynamic device node selected by mux; > > + - gprw - general purpose read-write register nodes: > > + Required properties: > > + - general purpose read-write register subnodes: > > + Required properties: > > + - attr-spec <reg mask>: > > + - reg : register offset; > > + - mask : attribute mask; > > + - gpro - general purpose read only register nodes: > > + Required properties: > > + - general purpose read only register subnodes: > > + Required properties: > > + - attr-spec <reg mask>: > > + - reg : register offset. > > + - mask : attribute mask. > > + - led - led nodes for led operations control: > > + Required properties: > > + - led control nodes: > > + Required properties: > > + - attr-spec <reg mask>: > > + - reg : register offset; > > + - mask : attribute mask; > > + > > +Example: > > + mlxcpld-mng-ctrl@71 { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + interrupt-parent = <&gpio>; > > + interrupts = <ASPEED_GPIO(S, 1) 2>; > > + compatible = "mellanox,mlxcpld-ctrl-i2c"; > > + reg = <0x71>; > > + deferred = <&i2c6>; > > + hotplug-spec = <0x3a 0x4c>; > > + > > + pwr: { > > + hotplug-spec = <0x08 0x64 0x03>; > > + hotplugs = <&psu1_ctrl &psu2_ctrl>; > > + }; > > + > > + mux { > > + spi_burn_bios_ci { > > + attr-spec = <0x32 0xfe 0x00>; > > + hotplugs = <&spi2>; > > + }; > > + }; > > + > > + led { > > + status-green { > > + attr-spec = <0x20 0xf0>; > > + }; > > + status-red { > > + attr-spec = <0x20 0xf0>; > > + }; > > + }; > > + > > + reset { > > + sys_power_cycle { > > + attr-spec = <0x30 0xfb>; > > + }; > > + }; > > + > > + cause { > > + ac_power_cycle { > > + attr-spec = <0x1d 0xfe>; > > + }; > > + sys_pwr_cycle: { > > + attr-spec = <0x1e 0xfb>; > > + }; > > + }; > > + > > + gpro { > > + cpld_mng_version { > > + attr-spec <0x00 0xff 0xff>; > > + }; > > + }; > > + > > + gprw { > > + sw_reset_cause { > > + attr-spec = <0x37 0x0f 0x0f>; > > + }; > > + }; > > + }; > > -- > Lee Jones > Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source > software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
diff --git a/Documentation/devicetree/bindings/mfd/mellanox,mlxreg-core.txt b/Documentation/devicetree/bindings/mfd/mellanox,mlxreg-core.txt new file mode 100644 index 0000000..1e3cce5 --- /dev/null +++ b/Documentation/devicetree/bindings/mfd/mellanox,mlxreg-core.txt @@ -0,0 +1,165 @@ +Mellanox programmable device control. +------------------------------------- +This binding defines the device control interface over for Mellanox BMC based +switches. + +Required properties: +- compatible = "mellanox,mlxreg-i2c" or + "mellanox,mlxreg-i2c-16" + +- #address-cells : must be 1; +- #size-cells : must be 0; +- reg : I2C address; + +Optional properties: +- interrupt-parent : phandle of parent interrupt controller; +- interrupts : interrupt line; +- deferred : I2C deferred bus phandle; + I2C bus activation order enforce for the cases when hot-plug + devices are attached to I2C bus, which is initialized after the + I2C bus, where programmable device is attached; +- hotplug-spec <aggr mask>: + - aggr : interrupt top aggregation register offset; + - mask : interrupt top aggregation register mask; + +Optional nodes and their properties: + - psu : power supply unit nodes: + Required properties: + - hotplug-spec <aggr reg mask>: + - aggr : effective bit in aggregation interrupt mask; + - reg : interrupt status register offset for all group members; + - mask : interrupt register mask; + - hotplugs - list of relevant dynamic device nodes; + - fan : fan unit nodes: + Required properties: + - hotplug-spec <aggr reg mask>: + - aggr : effective bit in aggregation interrupt mask; + - reg : interrupt status register offset for all group members; + - mask : interrupt register mask; + - hotplugs - list of relevant dynamic device nodes; + - pwr : power cable nodes: + Required properties: + - hotplug-spec <aggr reg mask>: + - aggr : effective bit in aggregation interrupt mask; + - reg : interrupt status register offset for all group members; + - mask : interrupt register mask; + - hotplugs : list of relevant dynamic device nodes; + - host : host side nodes (CPU host side for BMC): + Required properties: + - hotplug-spec <aggr reg mask>: + - aggr : effective bit in aggregation interrupt mask; + - reg : interrupt status register offset for all group members; + - mask : interrupt register mask; + - hotplugs : list of relevant dynamic device nodes; + - asic : asic nodes: + Required properties: + - hotplug-spec <aggr reg mask>: + - aggr : effective bit in aggregation interrupt mask; + - reg : interrupt status register offsets array per group member; + - mask : interrupt register mask; + - hotplugs : list of relevant device nodes; + - reset : reset nodes for system reset operations: + Required properties: + - reset control subnodes: + Required properties: + - attr-spec <reg mask>: + - reg : register offset; + - mask : attribute mask; + - cause - reset cause nodes for reading system reset cause: + Required properties: + - cause info subnodes: + Required properties: + - attr-spec <reg mask>: + - reg : register offset; + - mask : attribute mask; + - mux - mux nodes: + Required properties: + - mux control subnodes for hardware select operations: + Required properties: + - attr-spec <reg mask bit>: + - reg : register offset; + - mask : attribute mask; + - bit : effective bit; + Optional property: + - hotplugs : dynamic device node selected by mux; + - gprw - general purpose read-write register nodes: + Required properties: + - general purpose read-write register subnodes: + Required properties: + - attr-spec <reg mask>: + - reg : register offset; + - mask : attribute mask; + - gpro - general purpose read only register nodes: + Required properties: + - general purpose read only register subnodes: + Required properties: + - attr-spec <reg mask>: + - reg : register offset. + - mask : attribute mask. + - led - led nodes for led operations control: + Required properties: + - led control nodes: + Required properties: + - attr-spec <reg mask>: + - reg : register offset; + - mask : attribute mask; + +Example: + mlxcpld-mng-ctrl@71 { + #address-cells = <1>; + #size-cells = <0>; + interrupt-parent = <&gpio>; + interrupts = <ASPEED_GPIO(S, 1) 2>; + compatible = "mellanox,mlxcpld-ctrl-i2c"; + reg = <0x71>; + deferred = <&i2c6>; + hotplug-spec = <0x3a 0x4c>; + + pwr: { + hotplug-spec = <0x08 0x64 0x03>; + hotplugs = <&psu1_ctrl &psu2_ctrl>; + }; + + mux { + spi_burn_bios_ci { + attr-spec = <0x32 0xfe 0x00>; + hotplugs = <&spi2>; + }; + }; + + led { + status-green { + attr-spec = <0x20 0xf0>; + }; + status-red { + attr-spec = <0x20 0xf0>; + }; + }; + + reset { + sys_power_cycle { + attr-spec = <0x30 0xfb>; + }; + }; + + cause { + ac_power_cycle { + attr-spec = <0x1d 0xfe>; + }; + sys_pwr_cycle: { + attr-spec = <0x1e 0xfb>; + }; + }; + + gpro { + cpld_mng_version { + attr-spec <0x00 0xff 0xff>; + }; + }; + + gprw { + sw_reset_cause { + attr-spec = <0x37 0x0f 0x0f>; + }; + }; + };
The mlxreg a multifunction device driver handling LEDs, events, exposing through sysfs reset signal, and reset causes info. These components share a common register space. Signed-off-by: Vadim Pasternak <vadimp@mellanox.com> --- v4->v5: Comments pointed out by Rob: - Avoid duplications in reg; - Remove unnecessary details in description; - Do not use names like phandels; Changes added by Vadim: - Combine hotplug nodes interrupt aggregation, register offset and mask properties into hotplug-spec properties; - Combine reset, cause, etc subnodes register offset, mask and effective bit properties into attr-spec properties; v3->v4: Comments pointed out by Rob: - Make a separate patch /devicetree/bindings/vendor-prefixes.txt; - Add .txt to Documentation/devicetree/bindings/mfd/mellanox,mlxreg-core and send it within this series; - Modify "compatible" property; - Modify explanation for "deferred" property; - Describe each subnode by its own section; - Don't use underscore in attribute names; --- .../bindings/mfd/mellanox,mlxreg-core.txt | 165 +++++++++++++++++++++ 1 file changed, 165 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/mellanox,mlxreg-core.txt