[PATCH 0/4] ASpeed mailbox and LPC control drivers

[PATCH 0/4] ASpeed mailbox and LPC control drivers

[Cyril Bur]

Hello,

I have written some drivers for the ASpeed AST2400/2500 chips. These
chips are designed to be used as BMCs and the core reason for these
drivers has been to introduce a communication channel between two
processors.

The mailbox registers are the channel through which the two processors
can communicate, it is worth noting here that the mailbox driver is
designed to be flexible enough do allow userspace to arbitrarily write
to one or all or a subset of the mailbox registers. This is important
as the ASpeed chip allows for enabling hardware interrupts based on
writes at a per data register level. It is possible that the other
processor will enable interrupts on one of the data registers, as
such, the ASpeed end of the protocol will need to be able to treat
that register specially.

The first intended use of a communication protocol between the ASpeed
and the other processor is so they can arbitrate on board flash chip
access. The goal is to have the ASpeed chip perform the reads and
writes to the flash and present to the other processor in an area of
its RAM across a shared bus. Currently the other processor has the
flash mapped directly on the shared bus. The LPC bus controlling
driver provides a way for the ASpeed userspace to control the mapping
across the LPC bus between the ASpeed and the other processor. The RAM
region that the LPC control driver will use should be specified in the
device tree.

Cyril Bur (4):
  Documentation: dt: mailbox: Add Aspeed ast2400/2500 bindings
  Documentation: dt: misc: Add Aspeed ast2400/2500 LPC Control bindings
  drivers/misc: Add ASpeed LPC control driver
  drivers/mailbox: Add ASpeed mailbox driver

 .../devicetree/bindings/mailbox/aspeed-mbox.txt    |  44 +++
 .../devicetree/bindings/misc/aspeed-lpc-ctrl.txt   |  78 +++++
 drivers/mailbox/Kconfig                            |   9 +
 drivers/mailbox/Makefile                           |   2 +
 drivers/mailbox/aspeed-mbox.c                      | 334 +++++++++++++++++++++
 drivers/misc/Kconfig                               |   9 +
 drivers/misc/Makefile                              |   1 +
 drivers/misc/aspeed-lpc-ctrl.c                     | 269 +++++++++++++++++
 include/uapi/linux/aspeed-lpc-ctrl.h               |  25 ++
 9 files changed, 771 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/aspeed-mbox.txt
 create mode 100644 Documentation/devicetree/bindings/misc/aspeed-lpc-ctrl.txt
 create mode 100644 drivers/mailbox/aspeed-mbox.c
 create mode 100644 drivers/misc/aspeed-lpc-ctrl.c
 create mode 100644 include/uapi/linux/aspeed-lpc-ctrl.h

-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/4] ASpeed mailbox and LPC control drivers

[Cyril Bur]

Hello,

I have written some drivers for the ASpeed AST2400/2500 chips. These
chips are designed to be used as BMCs and the core reason for these
drivers has been to introduce a communication channel between two
processors.

The mailbox registers are the channel through which the two processors
can communicate, it is worth noting here that the mailbox driver is
designed to be flexible enough do allow userspace to arbitrarily write
to one or all or a subset of the mailbox registers. This is important
as the ASpeed chip allows for enabling hardware interrupts based on
writes at a per data register level. It is possible that the other
processor will enable interrupts on one of the data registers, as
such, the ASpeed end of the protocol will need to be able to treat
that register specially.

The first intended use of a communication protocol between the ASpeed
and the other processor is so they can arbitrate on board flash chip
access. The goal is to have the ASpeed chip perform the reads and
writes to the flash and present to the other processor in an area of
its RAM across a shared bus. Currently the other processor has the
flash mapped directly on the shared bus. The LPC bus controlling
driver provides a way for the ASpeed userspace to control the mapping
across the LPC bus between the ASpeed and the other processor. The RAM
region that the LPC control driver will use should be specified in the
device tree.

Cyril Bur (4):
  Documentation: dt: mailbox: Add Aspeed ast2400/2500 bindings
  Documentation: dt: misc: Add Aspeed ast2400/2500 LPC Control bindings
  drivers/misc: Add ASpeed LPC control driver
  drivers/mailbox: Add ASpeed mailbox driver

 .../devicetree/bindings/mailbox/aspeed-mbox.txt    |  44 +++
 .../devicetree/bindings/misc/aspeed-lpc-ctrl.txt   |  78 +++++
 drivers/mailbox/Kconfig                            |   9 +
 drivers/mailbox/Makefile                           |   2 +
 drivers/mailbox/aspeed-mbox.c                      | 334 +++++++++++++++++++++
 drivers/misc/Kconfig                               |   9 +
 drivers/misc/Makefile                              |   1 +
 drivers/misc/aspeed-lpc-ctrl.c                     | 269 +++++++++++++++++
 include/uapi/linux/aspeed-lpc-ctrl.h               |  25 ++
 9 files changed, 771 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/aspeed-mbox.txt
 create mode 100644 Documentation/devicetree/bindings/misc/aspeed-lpc-ctrl.txt
 create mode 100644 drivers/mailbox/aspeed-mbox.c
 create mode 100644 drivers/misc/aspeed-lpc-ctrl.c
 create mode 100644 include/uapi/linux/aspeed-lpc-ctrl.h

-- 
2.11.0

[PATCH 1/4] Documentation: dt: mailbox: Add Aspeed ast2400/2500 bindings

[Cyril Bur]

Signed-off-by: Cyril Bur <cyrilbur-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 .../devicetree/bindings/mailbox/aspeed-mbox.txt    | 44 ++++++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/aspeed-mbox.txt

diff --git a/Documentation/devicetree/bindings/mailbox/aspeed-mbox.txt b/Documentation/devicetree/bindings/mailbox/aspeed-mbox.txt
new file mode 100644
index 000000000000..633cd534d91c
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/aspeed-mbox.txt
@@ -0,0 +1,44 @@
+ASpeed Mailbox Driver
+=====================
+
+The ASpeed mailbox allows for communication between different
+processors. The mailbox on the ASpeed ast2400 and ast2500 is a set of
+16 single byte data registers along with interrupt and configuration
+registers directly on the SoC. These are memory mapped on the aspeed
+and can be accessed via the SuperIO registers on the other processor.
+
+Device Node:
+============
+This represents the mailbox on the Soc.
+
+As the mailbox registers sit on the LPC bus, it makes most sense for
+the device to be within the LPC host node. See
+Documentation/devicetree/bindings/mfd/aspeed-lpc.txt for more
+information. This does not have to be the case, provided the reg
+property can give the full address of the mbox registers.
+
+Required Properties:
+--------------------
+- compatible:	Should be one of the following,
+				"aspeed,ast2400-mbox" for ast2400 SoCs
+				"aspeed,ast2500-mbox" for ast2500 SoCs
+
+- reg:			Contains the mailbox address register range (base
+				address and length). Keeping in mind that if the node
+				exists within the LPC host node and that base is
+				relative to that.
+
+- interrupts:	Contains interrupt information for the mailbox device.
+
+- #mbox-cells:	Common property, should be 1.
+
+Example:
+--------
+
+mbox: mbox@180 {
+	compatible = "aspeed,ast2400-mbox";
+	reg = <0x180 0x5c>;
+	interrupts = <46>;
+	#mbox-cells = <1>;
+};
+
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/4] Documentation: dt: mailbox: Add Aspeed ast2400/2500 bindings

[Cyril Bur]

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
 .../devicetree/bindings/mailbox/aspeed-mbox.txt    | 44 ++++++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/aspeed-mbox.txt

diff --git a/Documentation/devicetree/bindings/mailbox/aspeed-mbox.txt b/Documentation/devicetree/bindings/mailbox/aspeed-mbox.txt
new file mode 100644
index 000000000000..633cd534d91c
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/aspeed-mbox.txt
@@ -0,0 +1,44 @@
+ASpeed Mailbox Driver
+=====================
+
+The ASpeed mailbox allows for communication between different
+processors. The mailbox on the ASpeed ast2400 and ast2500 is a set of
+16 single byte data registers along with interrupt and configuration
+registers directly on the SoC. These are memory mapped on the aspeed
+and can be accessed via the SuperIO registers on the other processor.
+
+Device Node:
+============
+This represents the mailbox on the Soc.
+
+As the mailbox registers sit on the LPC bus, it makes most sense for
+the device to be within the LPC host node. See
+Documentation/devicetree/bindings/mfd/aspeed-lpc.txt for more
+information. This does not have to be the case, provided the reg
+property can give the full address of the mbox registers.
+
+Required Properties:
+--------------------
+- compatible:	Should be one of the following,
+				"aspeed,ast2400-mbox" for ast2400 SoCs
+				"aspeed,ast2500-mbox" for ast2500 SoCs
+
+- reg:			Contains the mailbox address register range (base
+				address and length). Keeping in mind that if the node
+				exists within the LPC host node and that base is
+				relative to that.
+
+- interrupts:	Contains interrupt information for the mailbox device.
+
+- #mbox-cells:	Common property, should be 1.
+
+Example:
+--------
+
+mbox: mbox@180 {
+	compatible = "aspeed,ast2400-mbox";
+	reg = <0x180 0x5c>;
+	interrupts = <46>;
+	#mbox-cells = <1>;
+};
+
-- 
2.11.0

Re: [PATCH 1/4] Documentation: dt: mailbox: Add Aspeed ast2400/2500 bindings

[Rob Herring]

On Thu, Jan 12, 2017 at 11:29:07AM +1100, Cyril Bur wrote:
> Signed-off-by: Cyril Bur <cyrilbur-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  .../devicetree/bindings/mailbox/aspeed-mbox.txt    | 44 ++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/aspeed-mbox.txt
> 
> diff --git a/Documentation/devicetree/bindings/mailbox/aspeed-mbox.txt b/Documentation/devicetree/bindings/mailbox/aspeed-mbox.txt
> new file mode 100644
> index 000000000000..633cd534d91c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/aspeed-mbox.txt
> @@ -0,0 +1,44 @@
> +ASpeed Mailbox Driver
> +=====================
> +
> +The ASpeed mailbox allows for communication between different
> +processors. The mailbox on the ASpeed ast2400 and ast2500 is a set of
> +16 single byte data registers along with interrupt and configuration
> +registers directly on the SoC. These are memory mapped on the aspeed
> +and can be accessed via the SuperIO registers on the other processor.
> +
> +Device Node:
> +============
> +This represents the mailbox on the Soc.
> +
> +As the mailbox registers sit on the LPC bus, it makes most sense for
> +the device to be within the LPC host node. See
> +Documentation/devicetree/bindings/mfd/aspeed-lpc.txt for more
> +information. This does not have to be the case, provided the reg
> +property can give the full address of the mbox registers.

This does have to be the case. I'd expect all devices on the LPC bus to 
be under a LPC bus node.

Drop the last sentence, and:

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] Documentation: dt: mailbox: Add Aspeed ast2400/2500 bindings

[Rob Herring]

On Thu, Jan 12, 2017 at 11:29:07AM +1100, Cyril Bur wrote:
> Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> ---
>  .../devicetree/bindings/mailbox/aspeed-mbox.txt    | 44 ++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/aspeed-mbox.txt
> 
> diff --git a/Documentation/devicetree/bindings/mailbox/aspeed-mbox.txt b/Documentation/devicetree/bindings/mailbox/aspeed-mbox.txt
> new file mode 100644
> index 000000000000..633cd534d91c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/aspeed-mbox.txt
> @@ -0,0 +1,44 @@
> +ASpeed Mailbox Driver
> +=====================
> +
> +The ASpeed mailbox allows for communication between different
> +processors. The mailbox on the ASpeed ast2400 and ast2500 is a set of
> +16 single byte data registers along with interrupt and configuration
> +registers directly on the SoC. These are memory mapped on the aspeed
> +and can be accessed via the SuperIO registers on the other processor.
> +
> +Device Node:
> +============
> +This represents the mailbox on the Soc.
> +
> +As the mailbox registers sit on the LPC bus, it makes most sense for
> +the device to be within the LPC host node. See
> +Documentation/devicetree/bindings/mfd/aspeed-lpc.txt for more
> +information. This does not have to be the case, provided the reg
> +property can give the full address of the mbox registers.

This does have to be the case. I'd expect all devices on the LPC bus to 
be under a LPC bus node.

Drop the last sentence, and:

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

Re: [PATCH 1/4] Documentation: dt: mailbox: Add Aspeed ast2400/2500 bindings

[Cyril Bur]

On Wed, 2017-01-18 at 14:38 -0600, Rob Herring wrote:
> On Thu, Jan 12, 2017 at 11:29:07AM +1100, Cyril Bur wrote:
> > Signed-off-by: Cyril Bur <cyrilbur-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> >  .../devicetree/bindings/mailbox/aspeed-mbox.txt    | 44 ++++++++++++++++++++++
> >  1 file changed, 44 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mailbox/aspeed-mbox.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/mailbox/aspeed-mbox.txt b/Documentation/devicetree/bindings/mailbox/aspeed-mbox.txt
> > new file mode 100644
> > index 000000000000..633cd534d91c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mailbox/aspeed-mbox.txt
> > @@ -0,0 +1,44 @@
> > +ASpeed Mailbox Driver
> > +=====================
> > +
> > +The ASpeed mailbox allows for communication between different
> > +processors. The mailbox on the ASpeed ast2400 and ast2500 is a set of
> > +16 single byte data registers along with interrupt and configuration
> > +registers directly on the SoC. These are memory mapped on the aspeed
> > +and can be accessed via the SuperIO registers on the other processor.
> > +
> > +Device Node:
> > +============
> > +This represents the mailbox on the Soc.
> > +
> > +As the mailbox registers sit on the LPC bus, it makes most sense for
> > +the device to be within the LPC host node. See
> > +Documentation/devicetree/bindings/mfd/aspeed-lpc.txt for more
> > +information. This does not have to be the case, provided the reg
> > +property can give the full address of the mbox registers.
> 
> This does have to be the case. I'd expect all devices on the LPC bus to 
> be under a LPC bus node.
> 
> Drop the last sentence, and:
> 
> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Will do, thanks for the review.

Cyril
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] Documentation: dt: mailbox: Add Aspeed ast2400/2500 bindings

[Cyril Bur]

On Wed, 2017-01-18 at 14:38 -0600, Rob Herring wrote:
> On Thu, Jan 12, 2017 at 11:29:07AM +1100, Cyril Bur wrote:
> > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> > ---
> >  .../devicetree/bindings/mailbox/aspeed-mbox.txt    | 44 ++++++++++++++++++++++
> >  1 file changed, 44 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mailbox/aspeed-mbox.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/mailbox/aspeed-mbox.txt b/Documentation/devicetree/bindings/mailbox/aspeed-mbox.txt
> > new file mode 100644
> > index 000000000000..633cd534d91c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mailbox/aspeed-mbox.txt
> > @@ -0,0 +1,44 @@
> > +ASpeed Mailbox Driver
> > +=====================
> > +
> > +The ASpeed mailbox allows for communication between different
> > +processors. The mailbox on the ASpeed ast2400 and ast2500 is a set of
> > +16 single byte data registers along with interrupt and configuration
> > +registers directly on the SoC. These are memory mapped on the aspeed
> > +and can be accessed via the SuperIO registers on the other processor.
> > +
> > +Device Node:
> > +============
> > +This represents the mailbox on the Soc.
> > +
> > +As the mailbox registers sit on the LPC bus, it makes most sense for
> > +the device to be within the LPC host node. See
> > +Documentation/devicetree/bindings/mfd/aspeed-lpc.txt for more
> > +information. This does not have to be the case, provided the reg
> > +property can give the full address of the mbox registers.
> 
> This does have to be the case. I'd expect all devices on the LPC bus to 
> be under a LPC bus node.
> 
> Drop the last sentence, and:
> 
> Acked-by: Rob Herring <robh@kernel.org>

Will do, thanks for the review.

Cyril

Re: [PATCH 1/4] Documentation: dt: mailbox: Add Aspeed ast2400/2500 bindings

[Benjamin Herrenschmidt]

On Thu, 2017-01-19 at 11:05 +1100, Cyril Bur wrote:
> > > +============
> > > +This represents the mailbox on the Soc.
> > > +
> > > +As the mailbox registers sit on the LPC bus, it makes most sense for
> > > +the device to be within the LPC host node. See
> > > +Documentation/devicetree/bindings/mfd/aspeed-lpc.txt for more
> > > +information. This does not have to be the case, provided the reg
> > > +property can give the full address of the mbox registers.
> > 
> > This does have to be the case. I'd expect all devices on the LPC bus to 
> > be under a LPC bus node.
> > 
> > Drop the last sentence, and:
> > 
> > Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> 
> Will do, thanks for the review.

Well well ... on the BMC side it's not actually on the LPC bus ;-)

However it's within the LPC host controller register set, and so as such
can be represented as a child of it.

Cheers,
Ben.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] Documentation: dt: mailbox: Add Aspeed ast2400/2500 bindings

[Benjamin Herrenschmidt]

On Thu, 2017-01-19 at 11:05 +1100, Cyril Bur wrote:
> > > +============
> > > +This represents the mailbox on the Soc.
> > > +
> > > +As the mailbox registers sit on the LPC bus, it makes most sense for
> > > +the device to be within the LPC host node. See
> > > +Documentation/devicetree/bindings/mfd/aspeed-lpc.txt for more
> > > +information. This does not have to be the case, provided the reg
> > > +property can give the full address of the mbox registers.
> > 
> > This does have to be the case. I'd expect all devices on the LPC bus to 
> > be under a LPC bus node.
> > 
> > Drop the last sentence, and:
> > 
> > Acked-by: Rob Herring <robh@kernel.org>
> 
> Will do, thanks for the review.

Well well ... on the BMC side it's not actually on the LPC bus ;-)

However it's within the LPC host controller register set, and so as such
can be represented as a child of it.

Cheers,
Ben.

[PATCH 2/4] Documentation: dt: misc: Add Aspeed ast2400/2500 LPC Control bindings

[Cyril Bur]

Signed-off-by: Cyril Bur <cyrilbur-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 .../devicetree/bindings/misc/aspeed-lpc-ctrl.txt   | 78 ++++++++++++++++++++++
 1 file changed, 78 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/aspeed-lpc-ctrl.txt

diff --git a/Documentation/devicetree/bindings/misc/aspeed-lpc-ctrl.txt b/Documentation/devicetree/bindings/misc/aspeed-lpc-ctrl.txt
new file mode 100644
index 000000000000..f84ac83211ec
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/aspeed-lpc-ctrl.txt
@@ -0,0 +1,78 @@
+ASpeed LPC Control
+==================
+This binding defines the LPC control for ASpeed SoCs. Partitions of
+the LPC bus can be access by other processors on the system, address
+ranges on the bus can map accesses from another processor to regions
+of the ASpeed SoC memory space.
+
+Reserved Memory:
+================
+The driver provides functionality to map the LPC bus to a region of
+ASpeed ram. A phandle to a reserved memory node must be provided so
+that the driver can safely use this region.
+
+Flash:
+======
+The driver provides functionality to unmap the LPC bus from ASpeed
+RAM, historically the default mapping has been to the SPI flash
+controller on the ASpeed SoC, a phandle to this node should be
+supplied.
+
+Device Node:
+============
+
+As LPC bus configuration registers are at the start of the LPC bus
+memory space, it makes most sense for the device to be within the LPC
+host node. See Documentation/devicetree/bindings/mfd/aspeed-lpc.txt
+for more information. This does not have to be the case, provided the
+reg property can give the full address of the LPC bus.
+
+Required properties:
+--------------------
+
+- compatible:		"aspeed,ast2400-lpc-ctrl" for ASpeed ast2400 SoCs
+					"aspeed,ast2500-lpc-ctrl" for ASpeed ast2500 SoCs
+
+- reg:				Location and size of the configuration registers
+					for the LPC bus. Note that if the device node is
+					within the LPC host node then base is relative to
+					that.
+
+- memory-region:	phandle of the reserved memory region
+- flash:			phandle of the SPI flash controller
+
+Example:
+--------
+
+reserved-memory {
+	#address-cells = <1>;
+	#size-cells = <1>;
+	ranges;
+
+	...
+
+	flash_memory: region@54000000 {
+		compatible = "aspeed,ast2400-lpc-ctrl";
+		no-map;
+		reg = <0x54000000 0x04000000>; /* 64M */
+	};
+};
+
+host_pnor: spi@1e630000 {
+	reg = < 0x1e630000 0x18
+			0x30000000 0x02000000 >;
+	#address-cells = <1>;
+	#size-cells = <0>;
+	compatible = "aspeed,ast2400-smc";
+
+	...
+
+};
+
+lpc-ctrl@0 {
+	compatible = "aspeed,ast2400-lpc-ctrl";
+	memory-region = <&flash_memory>;
+	flash = <&host_pnor>;
+	reg = <0x0 0x80>;
+};
+
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/4] Documentation: dt: misc: Add Aspeed ast2400/2500 LPC Control bindings

[Cyril Bur]

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
 .../devicetree/bindings/misc/aspeed-lpc-ctrl.txt   | 78 ++++++++++++++++++++++
 1 file changed, 78 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/aspeed-lpc-ctrl.txt

diff --git a/Documentation/devicetree/bindings/misc/aspeed-lpc-ctrl.txt b/Documentation/devicetree/bindings/misc/aspeed-lpc-ctrl.txt
new file mode 100644
index 000000000000..f84ac83211ec
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/aspeed-lpc-ctrl.txt
@@ -0,0 +1,78 @@
+ASpeed LPC Control
+==================
+This binding defines the LPC control for ASpeed SoCs. Partitions of
+the LPC bus can be access by other processors on the system, address
+ranges on the bus can map accesses from another processor to regions
+of the ASpeed SoC memory space.
+
+Reserved Memory:
+================
+The driver provides functionality to map the LPC bus to a region of
+ASpeed ram. A phandle to a reserved memory node must be provided so
+that the driver can safely use this region.
+
+Flash:
+======
+The driver provides functionality to unmap the LPC bus from ASpeed
+RAM, historically the default mapping has been to the SPI flash
+controller on the ASpeed SoC, a phandle to this node should be
+supplied.
+
+Device Node:
+============
+
+As LPC bus configuration registers are at the start of the LPC bus
+memory space, it makes most sense for the device to be within the LPC
+host node. See Documentation/devicetree/bindings/mfd/aspeed-lpc.txt
+for more information. This does not have to be the case, provided the
+reg property can give the full address of the LPC bus.
+
+Required properties:
+--------------------
+
+- compatible:		"aspeed,ast2400-lpc-ctrl" for ASpeed ast2400 SoCs
+					"aspeed,ast2500-lpc-ctrl" for ASpeed ast2500 SoCs
+
+- reg:				Location and size of the configuration registers
+					for the LPC bus. Note that if the device node is
+					within the LPC host node then base is relative to
+					that.
+
+- memory-region:	phandle of the reserved memory region
+- flash:			phandle of the SPI flash controller
+
+Example:
+--------
+
+reserved-memory {
+	#address-cells = <1>;
+	#size-cells = <1>;
+	ranges;
+
+	...
+
+	flash_memory: region@54000000 {
+		compatible = "aspeed,ast2400-lpc-ctrl";
+		no-map;
+		reg = <0x54000000 0x04000000>; /* 64M */
+	};
+};
+
+host_pnor: spi@1e630000 {
+	reg = < 0x1e630000 0x18
+			0x30000000 0x02000000 >;
+	#address-cells = <1>;
+	#size-cells = <0>;
+	compatible = "aspeed,ast2400-smc";
+
+	...
+
+};
+
+lpc-ctrl@0 {
+	compatible = "aspeed,ast2400-lpc-ctrl";
+	memory-region = <&flash_memory>;
+	flash = <&host_pnor>;
+	reg = <0x0 0x80>;
+};
+
-- 
2.11.0

Re: [PATCH 2/4] Documentation: dt: misc: Add Aspeed ast2400/2500 LPC Control bindings

[Rob Herring]

On Thu, Jan 12, 2017 at 11:29:08AM +1100, Cyril Bur wrote:
> Signed-off-by: Cyril Bur <cyrilbur-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  .../devicetree/bindings/misc/aspeed-lpc-ctrl.txt   | 78 ++++++++++++++++++++++
>  1 file changed, 78 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/misc/aspeed-lpc-ctrl.txt
> 
> diff --git a/Documentation/devicetree/bindings/misc/aspeed-lpc-ctrl.txt b/Documentation/devicetree/bindings/misc/aspeed-lpc-ctrl.txt
> new file mode 100644
> index 000000000000..f84ac83211ec
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/aspeed-lpc-ctrl.txt
> @@ -0,0 +1,78 @@
> +ASpeed LPC Control
> +==================
> +This binding defines the LPC control for ASpeed SoCs. Partitions of
> +the LPC bus can be access by other processors on the system, address
> +ranges on the bus can map accesses from another processor to regions
> +of the ASpeed SoC memory space.
> +
> +Reserved Memory:
> +================
> +The driver provides functionality to map the LPC bus to a region of
> +ASpeed ram. A phandle to a reserved memory node must be provided so
> +that the driver can safely use this region.
> +
> +Flash:
> +======
> +The driver provides functionality to unmap the LPC bus from ASpeed
> +RAM, historically the default mapping has been to the SPI flash
> +controller on the ASpeed SoC, a phandle to this node should be
> +supplied.
> +
> +Device Node:
> +============
> +
> +As LPC bus configuration registers are at the start of the LPC bus
> +memory space, it makes most sense for the device to be within the LPC
> +host node. See Documentation/devicetree/bindings/mfd/aspeed-lpc.txt
> +for more information. This does not have to be the case, provided the
> +reg property can give the full address of the LPC bus.

Same comment here.

> +
> +Required properties:
> +--------------------
> +
> +- compatible:		"aspeed,ast2400-lpc-ctrl" for ASpeed ast2400 SoCs
> +					"aspeed,ast2500-lpc-ctrl" for ASpeed ast2500 SoCs
> +
> +- reg:				Location and size of the configuration registers
> +					for the LPC bus. Note that if the device node is
> +					within the LPC host node then base is relative to
> +					that.
> +
> +- memory-region:	phandle of the reserved memory region
> +- flash:			phandle of the SPI flash controller
> +
> +Example:
> +--------
> +
> +reserved-memory {
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +	ranges;
> +
> +	...
> +
> +	flash_memory: region@54000000 {
> +		compatible = "aspeed,ast2400-lpc-ctrl";

This doesn't look right?

> +		no-map;
> +		reg = <0x54000000 0x04000000>; /* 64M */

Is this system RAM? reserved-memory is generally for carveouts in system 
RAM (e.g. the memory node).

> +	};
> +};
> +
> +host_pnor: spi@1e630000 {
> +	reg = < 0x1e630000 0x18
> +			0x30000000 0x02000000 >;
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	compatible = "aspeed,ast2400-smc";
> +
> +	...
> +
> +};
> +
> +lpc-ctrl@0 {
> +	compatible = "aspeed,ast2400-lpc-ctrl";
> +	memory-region = <&flash_memory>;
> +	flash = <&host_pnor>;
> +	reg = <0x0 0x80>;
> +};
> +
> -- 
> 2.11.0
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] Documentation: dt: misc: Add Aspeed ast2400/2500 LPC Control bindings

[Rob Herring]

On Thu, Jan 12, 2017 at 11:29:08AM +1100, Cyril Bur wrote:
> Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> ---
>  .../devicetree/bindings/misc/aspeed-lpc-ctrl.txt   | 78 ++++++++++++++++++++++
>  1 file changed, 78 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/misc/aspeed-lpc-ctrl.txt
> 
> diff --git a/Documentation/devicetree/bindings/misc/aspeed-lpc-ctrl.txt b/Documentation/devicetree/bindings/misc/aspeed-lpc-ctrl.txt
> new file mode 100644
> index 000000000000..f84ac83211ec
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/aspeed-lpc-ctrl.txt
> @@ -0,0 +1,78 @@
> +ASpeed LPC Control
> +==================
> +This binding defines the LPC control for ASpeed SoCs. Partitions of
> +the LPC bus can be access by other processors on the system, address
> +ranges on the bus can map accesses from another processor to regions
> +of the ASpeed SoC memory space.
> +
> +Reserved Memory:
> +================
> +The driver provides functionality to map the LPC bus to a region of
> +ASpeed ram. A phandle to a reserved memory node must be provided so
> +that the driver can safely use this region.
> +
> +Flash:
> +======
> +The driver provides functionality to unmap the LPC bus from ASpeed
> +RAM, historically the default mapping has been to the SPI flash
> +controller on the ASpeed SoC, a phandle to this node should be
> +supplied.
> +
> +Device Node:
> +============
> +
> +As LPC bus configuration registers are at the start of the LPC bus
> +memory space, it makes most sense for the device to be within the LPC
> +host node. See Documentation/devicetree/bindings/mfd/aspeed-lpc.txt
> +for more information. This does not have to be the case, provided the
> +reg property can give the full address of the LPC bus.

Same comment here.

> +
> +Required properties:
> +--------------------
> +
> +- compatible:		"aspeed,ast2400-lpc-ctrl" for ASpeed ast2400 SoCs
> +					"aspeed,ast2500-lpc-ctrl" for ASpeed ast2500 SoCs
> +
> +- reg:				Location and size of the configuration registers
> +					for the LPC bus. Note that if the device node is
> +					within the LPC host node then base is relative to
> +					that.
> +
> +- memory-region:	phandle of the reserved memory region
> +- flash:			phandle of the SPI flash controller
> +
> +Example:
> +--------
> +
> +reserved-memory {
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +	ranges;
> +
> +	...
> +
> +	flash_memory: region@54000000 {
> +		compatible = "aspeed,ast2400-lpc-ctrl";

This doesn't look right?

> +		no-map;
> +		reg = <0x54000000 0x04000000>; /* 64M */

Is this system RAM? reserved-memory is generally for carveouts in system 
RAM (e.g. the memory node).

> +	};
> +};
> +
> +host_pnor: spi@1e630000 {
> +	reg = < 0x1e630000 0x18
> +			0x30000000 0x02000000 >;
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	compatible = "aspeed,ast2400-smc";
> +
> +	...
> +
> +};
> +
> +lpc-ctrl@0 {
> +	compatible = "aspeed,ast2400-lpc-ctrl";
> +	memory-region = <&flash_memory>;
> +	flash = <&host_pnor>;
> +	reg = <0x0 0x80>;
> +};
> +
> -- 
> 2.11.0
> 

Re: [PATCH 2/4] Documentation: dt: misc: Add Aspeed ast2400/2500 LPC Control bindings

[Cyril Bur]

On Wed, 2017-01-18 at 15:16 -0600, Rob Herring wrote:
> On Thu, Jan 12, 2017 at 11:29:08AM +1100, Cyril Bur wrote:
> > Signed-off-by: Cyril Bur <cyrilbur-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> >  .../devicetree/bindings/misc/aspeed-lpc-ctrl.txt   | 78 ++++++++++++++++++++++
> >  1 file changed, 78 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/misc/aspeed-lpc-ctrl.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/misc/aspeed-lpc-ctrl.txt b/Documentation/devicetree/bindings/misc/aspeed-lpc-ctrl.txt
> > new file mode 100644
> > index 000000000000..f84ac83211ec
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/misc/aspeed-lpc-ctrl.txt
> > @@ -0,0 +1,78 @@
> > +ASpeed LPC Control
> > +==================
> > +This binding defines the LPC control for ASpeed SoCs. Partitions of
> > +the LPC bus can be access by other processors on the system, address
> > +ranges on the bus can map accesses from another processor to regions
> > +of the ASpeed SoC memory space.
> > +
> > +Reserved Memory:
> > +================
> > +The driver provides functionality to map the LPC bus to a region of
> > +ASpeed ram. A phandle to a reserved memory node must be provided so
> > +that the driver can safely use this region.
> > +
> > +Flash:
> > +======
> > +The driver provides functionality to unmap the LPC bus from ASpeed
> > +RAM, historically the default mapping has been to the SPI flash
> > +controller on the ASpeed SoC, a phandle to this node should be
> > +supplied.
> > +
> > +Device Node:
> > +============
> > +
> > +As LPC bus configuration registers are at the start of the LPC bus
> > +memory space, it makes most sense for the device to be within the LPC
> > +host node. See Documentation/devicetree/bindings/mfd/aspeed-lpc.txt
> > +for more information. This does not have to be the case, provided the
> > +reg property can give the full address of the LPC bus.
> 
> Same comment here.
> 

Hi Rob,

Yes, thanks.

> > +
> > +Required properties:
> > +--------------------
> > +
> > +- compatible:		"aspeed,ast2400-lpc-ctrl" for ASpeed ast2400 SoCs
> > +					"aspeed,ast2500-lpc-ctrl" for ASpeed ast2500 SoCs
> > +
> > +- reg:				Location and size of the configuration registers
> > +					for the LPC bus. Note that if the device node is
> > +					within the LPC host node then base is relative to
> > +					that.
> > +
> > +- memory-region:	phandle of the reserved memory region
> > +- flash:			phandle of the SPI flash controller
> > +
> > +Example:
> > +--------
> > +
> > +reserved-memory {
> > +	#address-cells = <1>;
> > +	#size-cells = <1>;
> > +	ranges;
> > +
> > +	...
> > +
> > +	flash_memory: region@54000000 {
> > +		compatible = "aspeed,ast2400-lpc-ctrl";
> 
> This doesn't look right?
> 

Correct, my mistake, I'll remove.

> > +		no-map;
> > +		reg = <0x54000000 0x04000000>; /* 64M */
> 
> Is this system RAM? reserved-memory is generally for carveouts in system 
> RAM (e.g. the memory node).
> 

Yes it will be a chunk of system RAM. Our intended use case is to use
system ram to buffer host accesses to system flash (on the bmc). This
provides control over concurrent access to the flash and place to add
security measures to prevent the host from backdooring through the
flash. With the use of a protocol through the platform mailbox.

Having said that I don't want to limit myself to just that - there has
been other ideas for a host<->bmc ram buffer which may or may not see
the light of day.

I hope that makes sense,

Thanks for the review,

Cyril

> > +	};
> > +};
> > +
> > +host_pnor: spi@1e630000 {
> > +	reg = < 0x1e630000 0x18
> > +			0x30000000 0x02000000 >;
> > +	#address-cells = <1>;
> > +	#size-cells = <0>;
> > +	compatible = "aspeed,ast2400-smc";
> > +
> > +	...
> > +
> > +};
> > +
> > +lpc-ctrl@0 {
> > +	compatible = "aspeed,ast2400-lpc-ctrl";
> > +	memory-region = <&flash_memory>;
> > +	flash = <&host_pnor>;
> > +	reg = <0x0 0x80>;
> > +};
> > +
> > -- 
> > 2.11.0
> > 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] Documentation: dt: misc: Add Aspeed ast2400/2500 LPC Control bindings

[Cyril Bur]

On Wed, 2017-01-18 at 15:16 -0600, Rob Herring wrote:
> On Thu, Jan 12, 2017 at 11:29:08AM +1100, Cyril Bur wrote:
> > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> > ---
> >  .../devicetree/bindings/misc/aspeed-lpc-ctrl.txt   | 78 ++++++++++++++++++++++
> >  1 file changed, 78 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/misc/aspeed-lpc-ctrl.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/misc/aspeed-lpc-ctrl.txt b/Documentation/devicetree/bindings/misc/aspeed-lpc-ctrl.txt
> > new file mode 100644
> > index 000000000000..f84ac83211ec
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/misc/aspeed-lpc-ctrl.txt
> > @@ -0,0 +1,78 @@
> > +ASpeed LPC Control
> > +==================
> > +This binding defines the LPC control for ASpeed SoCs. Partitions of
> > +the LPC bus can be access by other processors on the system, address
> > +ranges on the bus can map accesses from another processor to regions
> > +of the ASpeed SoC memory space.
> > +
> > +Reserved Memory:
> > +================
> > +The driver provides functionality to map the LPC bus to a region of
> > +ASpeed ram. A phandle to a reserved memory node must be provided so
> > +that the driver can safely use this region.
> > +
> > +Flash:
> > +======
> > +The driver provides functionality to unmap the LPC bus from ASpeed
> > +RAM, historically the default mapping has been to the SPI flash
> > +controller on the ASpeed SoC, a phandle to this node should be
> > +supplied.
> > +
> > +Device Node:
> > +============
> > +
> > +As LPC bus configuration registers are at the start of the LPC bus
> > +memory space, it makes most sense for the device to be within the LPC
> > +host node. See Documentation/devicetree/bindings/mfd/aspeed-lpc.txt
> > +for more information. This does not have to be the case, provided the
> > +reg property can give the full address of the LPC bus.
> 
> Same comment here.
> 

Hi Rob,

Yes, thanks.

> > +
> > +Required properties:
> > +--------------------
> > +
> > +- compatible:		"aspeed,ast2400-lpc-ctrl" for ASpeed ast2400 SoCs
> > +					"aspeed,ast2500-lpc-ctrl" for ASpeed ast2500 SoCs
> > +
> > +- reg:				Location and size of the configuration registers
> > +					for the LPC bus. Note that if the device node is
> > +					within the LPC host node then base is relative to
> > +					that.
> > +
> > +- memory-region:	phandle of the reserved memory region
> > +- flash:			phandle of the SPI flash controller
> > +
> > +Example:
> > +--------
> > +
> > +reserved-memory {
> > +	#address-cells = <1>;
> > +	#size-cells = <1>;
> > +	ranges;
> > +
> > +	...
> > +
> > +	flash_memory: region@54000000 {
> > +		compatible = "aspeed,ast2400-lpc-ctrl";
> 
> This doesn't look right?
> 

Correct, my mistake, I'll remove.

> > +		no-map;
> > +		reg = <0x54000000 0x04000000>; /* 64M */
> 
> Is this system RAM? reserved-memory is generally for carveouts in system 
> RAM (e.g. the memory node).
> 

Yes it will be a chunk of system RAM. Our intended use case is to use
system ram to buffer host accesses to system flash (on the bmc). This
provides control over concurrent access to the flash and place to add
security measures to prevent the host from backdooring through the
flash. With the use of a protocol through the platform mailbox.

Having said that I don't want to limit myself to just that - there has
been other ideas for a host<->bmc ram buffer which may or may not see
the light of day.

I hope that makes sense,

Thanks for the review,

Cyril

> > +	};
> > +};
> > +
> > +host_pnor: spi@1e630000 {
> > +	reg = < 0x1e630000 0x18
> > +			0x30000000 0x02000000 >;
> > +	#address-cells = <1>;
> > +	#size-cells = <0>;
> > +	compatible = "aspeed,ast2400-smc";
> > +
> > +	...
> > +
> > +};
> > +
> > +lpc-ctrl@0 {
> > +	compatible = "aspeed,ast2400-lpc-ctrl";
> > +	memory-region = <&flash_memory>;
> > +	flash = <&host_pnor>;
> > +	reg = <0x0 0x80>;
> > +};
> > +
> > -- 
> > 2.11.0
> > 

[PATCH 3/4] drivers/misc: Add ASpeed LPC control driver

[Cyril Bur]

This driver exposes a reserved chunk of BMC ram to userspace as well as
an ioctl interface to control the BMC<->HOST mapping of the LPC bus.
This allows for a communication channel between the BMC and the host

Signed-off-by: Cyril Bur <cyrilbur-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/misc/Kconfig                 |   9 ++
 drivers/misc/Makefile                |   1 +
 drivers/misc/aspeed-lpc-ctrl.c       | 269 +++++++++++++++++++++++++++++++++++
 include/uapi/linux/aspeed-lpc-ctrl.h |  25 ++++
 4 files changed, 304 insertions(+)
 create mode 100644 drivers/misc/aspeed-lpc-ctrl.c
 create mode 100644 include/uapi/linux/aspeed-lpc-ctrl.h

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 64971baf11fa..8696627ce9d2 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -766,6 +766,15 @@ config PANEL_BOOT_MESSAGE
 	  An empty message will only clear the display at driver init time. Any other
 	  printf()-formatted message is valid with newline and escape codes.
 
+config ASPEED_LPC_CTRL
+	depends on (ARCH_ASPEED || COMPILE_TEST) && REGMAP && MFD_SYSCON
+	bool "Build a driver to control the BMC to HOST LPC bus"
+	default "y"
+	---help---
+	  Provides a driver to control BMC to HOST LPC mappings through
+	  ioctl()s, the driver aso provides a read/write interface to a BMC ram
+	  region where host LPC can be buffered.
+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 31983366090a..de1925a9c80b 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -53,6 +53,7 @@ obj-$(CONFIG_ECHO)		+= echo/
 obj-$(CONFIG_VEXPRESS_SYSCFG)	+= vexpress-syscfg.o
 obj-$(CONFIG_CXL_BASE)		+= cxl/
 obj-$(CONFIG_PANEL)             += panel.o
+obj-$(CONFIG_ASPEED_LPC_CTRL)	+= aspeed-lpc-ctrl.o
 
 lkdtm-$(CONFIG_LKDTM)		+= lkdtm_core.o
 lkdtm-$(CONFIG_LKDTM)		+= lkdtm_bugs.o
diff --git a/drivers/misc/aspeed-lpc-ctrl.c b/drivers/misc/aspeed-lpc-ctrl.c
new file mode 100644
index 000000000000..36ab718681a5
--- /dev/null
+++ b/drivers/misc/aspeed-lpc-ctrl.c
@@ -0,0 +1,269 @@
+/*
+ * Copyright 2016 IBM Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/mfd/syscon.h>
+#include <linux/miscdevice.h>
+#include <linux/mm.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/poll.h>
+#include <linux/regmap.h>
+
+#include <linux/aspeed-lpc-ctrl.h>
+
+#define DEVICE_NAME	"aspeed-lpc-ctrl"
+
+#define HICR7 0x8
+#define HICR8 0xc
+
+struct lpc_ctrl {
+	struct miscdevice	miscdev;
+	struct regmap		*regmap;
+	phys_addr_t		base;
+	resource_size_t		size;
+	uint32_t		pnor_size;
+	uint32_t		pnor_base;
+};
+
+static atomic_t lpc_ctrl_open_count = ATOMIC_INIT(0);
+
+static struct lpc_ctrl *file_lpc_ctrl(struct file *file)
+{
+	return container_of(file->private_data, struct lpc_ctrl, miscdev);
+}
+
+static int lpc_ctrl_mmap(struct file *file, struct vm_area_struct *vma)
+{
+	struct lpc_ctrl *lpc_ctrl = file_lpc_ctrl(file);
+	unsigned long vsize = vma->vm_end - vma->vm_start;
+
+	if (vma->vm_pgoff + vsize > lpc_ctrl->base + lpc_ctrl->size)
+		return -EINVAL;
+
+	/* Other checks? */
+
+	if (remap_pfn_range(vma, vma->vm_start,
+		(lpc_ctrl->base >> PAGE_SHIFT) + vma->vm_pgoff,
+		vsize, vma->vm_page_prot))
+		return -EAGAIN;
+
+	return 0;
+}
+
+static int lpc_ctrl_open(struct inode *inode, struct file *file)
+{
+	if (atomic_inc_return(&lpc_ctrl_open_count) == 1)
+		return 0;
+
+	atomic_dec(&lpc_ctrl_open_count);
+	return -EBUSY;
+}
+
+static ssize_t lpc_ctrl_read(struct file *file, char __user *buf,
+				size_t count, loff_t *ppos)
+{
+	if (!access_ok(VERIFY_WRITE, buf, count))
+		return -EFAULT;
+
+	return -EPERM;
+}
+
+static ssize_t lpc_ctrl_write(struct file *file, const char __user *buf,
+				size_t count, loff_t *ppos)
+{
+	if (!access_ok(VERIFY_READ, buf, count))
+		return -EFAULT;
+
+	return -EPERM;
+}
+
+static long lpc_ctrl_ioctl(struct file *file, unsigned int cmd,
+		unsigned long param)
+{
+	long rc;
+	struct lpc_mapping map;
+	struct lpc_ctrl *lpc_ctrl = file_lpc_ctrl(file);
+	void __user *p = (void __user *)param;
+
+	switch (cmd) {
+	case LPC_CTRL_IOCTL_SIZE:
+		return copy_to_user(p, &lpc_ctrl->size,
+			sizeof(lpc_ctrl->size)) ? -EFAULT : 0;
+	case LPC_CTRL_IOCTL_MAP:
+		if (copy_from_user(&map, p, sizeof(map)))
+			return -EFAULT;
+
+
+		/*
+		 * The top half of HICR7 is the MSB of the BMC address of the
+		 * mapping.
+		 * The bottom half of HICR7 is the MSB of the HOST LPC
+		 * firmware space address of the mapping.
+		 *
+		 * The 1 bits in the top of half of HICR8 represent the bits
+		 * (in the requested address) that should be ignored and
+		 * replaced with those from the top half of HICR7.
+		 * The 1 bits in the bottom half of HICR8 represent the bits
+		 * (in the requested address) that should be kept and pass
+		 * into the BMC address space.
+		 */
+
+		rc = regmap_write(lpc_ctrl->regmap, HICR7,
+				(lpc_ctrl->base | (map.hostaddr >> 16)));
+		if (rc)
+			return rc;
+
+		rc = regmap_write(lpc_ctrl->regmap, HICR8,
+			(~(map.size - 1)) | ((map.size >> 16) - 1));
+		return rc;
+	case LPC_CTRL_IOCTL_UNMAP:
+		/*
+		 * The top nibble in host lpc addresses references which
+		 * firmware space, use space zero hence the & 0x0fff
+		 */
+
+		rc = regmap_write(lpc_ctrl->regmap, HICR7,
+			lpc_ctrl->pnor_base | (((-lpc_ctrl->pnor_size) >> 16) & 0x0fff));
+		if (rc)
+			return rc;
+
+		rc = regmap_write(lpc_ctrl->regmap, HICR8,
+			(~(lpc_ctrl->pnor_size - 1)) | ((lpc_ctrl->pnor_size >> 16) - 1));
+		return rc;
+	}
+
+	return -EINVAL;
+}
+
+static int lpc_ctrl_release(struct inode *inode, struct file *file)
+{
+	atomic_dec(&lpc_ctrl_open_count);
+	return 0;
+}
+
+static const struct file_operations lpc_ctrl_fops = {
+	.owner		= THIS_MODULE,
+	.mmap		= lpc_ctrl_mmap,
+	.open		= lpc_ctrl_open,
+	.read		= lpc_ctrl_read,
+	.write		= lpc_ctrl_write,
+	.release	= lpc_ctrl_release,
+	.unlocked_ioctl	= lpc_ctrl_ioctl,
+};
+
+static int lpc_ctrl_probe(struct platform_device *pdev)
+{
+	struct lpc_ctrl *lpc_ctrl;
+	struct device *dev;
+	struct device_node *node;
+	struct resource resm;
+	int rc;
+
+	if (!pdev || !pdev->dev.of_node)
+		return -ENODEV;
+
+	dev = &pdev->dev;
+
+	lpc_ctrl = devm_kzalloc(dev, sizeof(*lpc_ctrl), GFP_KERNEL);
+	if (!lpc_ctrl)
+		return -ENOMEM;
+
+	node = of_parse_phandle(dev->of_node, "flash", 0);
+	if (!node) {
+		dev_err(dev, "Didn't find host pnor flash node\n");
+		rc = -ENODEV;
+		goto out;
+	}
+
+	rc = of_property_read_u32_index(node, "reg", 3,
+			&lpc_ctrl->pnor_size);
+	if (rc)
+		return rc;
+	rc = of_property_read_u32_index(node, "reg", 2,
+			&lpc_ctrl->pnor_base);
+	if (rc)
+		return rc;
+
+	dev_info(dev, "Host PNOR base: 0x%08x size: 0x%08x\n",
+		lpc_ctrl->pnor_base, lpc_ctrl->pnor_size);
+
+	dev_set_drvdata(&pdev->dev, lpc_ctrl);
+
+	node = of_parse_phandle(dev->of_node, "memory-region", 0);
+	if (!node) {
+		dev_err(dev, "Didn't find reserved memory\n");
+		rc = -EINVAL;
+		goto out;
+	}
+
+	rc = of_address_to_resource(node, 0, &resm);
+	of_node_put(node);
+	if (rc) {
+		dev_err(dev, "Could address to resource\n");
+		rc = -ENOMEM;
+		goto out;
+	}
+
+	lpc_ctrl->size = resource_size(&resm);
+	lpc_ctrl->base = resm.start;
+
+	lpc_ctrl->regmap = syscon_node_to_regmap(
+			pdev->dev.parent->of_node);
+	if (IS_ERR(lpc_ctrl->regmap)) {
+		dev_err(dev, "Couldn't get regmap\n");
+		rc = -ENODEV;
+		goto out;
+	}
+
+	lpc_ctrl->miscdev.minor = MISC_DYNAMIC_MINOR;
+	lpc_ctrl->miscdev.name = DEVICE_NAME;
+	lpc_ctrl->miscdev.fops = &lpc_ctrl_fops;
+	lpc_ctrl->miscdev.parent = dev;
+	rc = misc_register(&lpc_ctrl->miscdev);
+	if (rc)
+		dev_err(dev, "Unable to register device\n");
+	else
+		dev_info(dev, "Loaded at 0x%08x (0x%08x)\n",
+			lpc_ctrl->base, lpc_ctrl->size);
+
+out:
+	return rc;
+}
+
+static int lpc_ctrl_remove(struct platform_device *pdev)
+{
+	struct lpc_ctrl *lpc_ctrl = dev_get_drvdata(&pdev->dev);
+
+	misc_deregister(&lpc_ctrl->miscdev);
+	lpc_ctrl = NULL;
+
+	return 0;
+}
+
+static const struct of_device_id lpc_ctrl_match[] = {
+	{ .compatible = "aspeed,ast2400-lpc-ctrl" },
+	{ },
+};
+
+static struct platform_driver lpc_ctrl_driver = {
+	.driver = {
+		.name		= DEVICE_NAME,
+		.of_match_table = lpc_ctrl_match,
+	},
+	.probe = lpc_ctrl_probe,
+	.remove = lpc_ctrl_remove,
+};
+
+module_platform_driver(lpc_ctrl_driver);
+
+MODULE_DEVICE_TABLE(of, lpc_ctrl_match);
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Cyril Bur <cyrilbur-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>");
+MODULE_DESCRIPTION("Linux device interface to control LPC bus");
diff --git a/include/uapi/linux/aspeed-lpc-ctrl.h b/include/uapi/linux/aspeed-lpc-ctrl.h
new file mode 100644
index 000000000000..c5f1caf827ac
--- /dev/null
+++ b/include/uapi/linux/aspeed-lpc-ctrl.h
@@ -0,0 +1,25 @@
+/*
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#ifndef _UAPI_LINUX_LPC_CTRL_H
+#define _UAPI_LINUX_LPC_CTRL_H
+
+#include <linux/ioctl.h>
+
+struct lpc_mapping {
+	uint32_t hostaddr;
+	uint32_t size;
+};
+
+#define __LPC_CTRL_IOCTL_MAGIC	0xb2
+#define LPC_CTRL_IOCTL_SIZE	_IOR(__LPC_CTRL_IOCTL_MAGIC, 0x00, uint32_t)
+#define LPC_CTRL_IOCTL_MAP	_IOW(__LPC_CTRL_IOCTL_MAGIC, 0x01, struct lpc_mapping)
+#define LPC_CTRL_IOCTL_UNMAP	_IO(__LPC_CTRL_IOCTL_MAGIC, 0x02)
+
+#endif /* _UAPI_LINUX_LPC_CTRL_H */
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/4] drivers/misc: Add ASpeed LPC control driver

[Cyril Bur]

This driver exposes a reserved chunk of BMC ram to userspace as well as
an ioctl interface to control the BMC<->HOST mapping of the LPC bus.
This allows for a communication channel between the BMC and the host

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
 drivers/misc/Kconfig                 |   9 ++
 drivers/misc/Makefile                |   1 +
 drivers/misc/aspeed-lpc-ctrl.c       | 269 +++++++++++++++++++++++++++++++++++
 include/uapi/linux/aspeed-lpc-ctrl.h |  25 ++++
 4 files changed, 304 insertions(+)
 create mode 100644 drivers/misc/aspeed-lpc-ctrl.c
 create mode 100644 include/uapi/linux/aspeed-lpc-ctrl.h

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 64971baf11fa..8696627ce9d2 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -766,6 +766,15 @@ config PANEL_BOOT_MESSAGE
 	  An empty message will only clear the display at driver init time. Any other
 	  printf()-formatted message is valid with newline and escape codes.
 
+config ASPEED_LPC_CTRL
+	depends on (ARCH_ASPEED || COMPILE_TEST) && REGMAP && MFD_SYSCON
+	bool "Build a driver to control the BMC to HOST LPC bus"
+	default "y"
+	---help---
+	  Provides a driver to control BMC to HOST LPC mappings through
+	  ioctl()s, the driver aso provides a read/write interface to a BMC ram
+	  region where host LPC can be buffered.
+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 31983366090a..de1925a9c80b 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -53,6 +53,7 @@ obj-$(CONFIG_ECHO)		+= echo/
 obj-$(CONFIG_VEXPRESS_SYSCFG)	+= vexpress-syscfg.o
 obj-$(CONFIG_CXL_BASE)		+= cxl/
 obj-$(CONFIG_PANEL)             += panel.o
+obj-$(CONFIG_ASPEED_LPC_CTRL)	+= aspeed-lpc-ctrl.o
 
 lkdtm-$(CONFIG_LKDTM)		+= lkdtm_core.o
 lkdtm-$(CONFIG_LKDTM)		+= lkdtm_bugs.o
diff --git a/drivers/misc/aspeed-lpc-ctrl.c b/drivers/misc/aspeed-lpc-ctrl.c
new file mode 100644
index 000000000000..36ab718681a5
--- /dev/null
+++ b/drivers/misc/aspeed-lpc-ctrl.c
@@ -0,0 +1,269 @@
+/*
+ * Copyright 2016 IBM Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/mfd/syscon.h>
+#include <linux/miscdevice.h>
+#include <linux/mm.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/poll.h>
+#include <linux/regmap.h>
+
+#include <linux/aspeed-lpc-ctrl.h>
+
+#define DEVICE_NAME	"aspeed-lpc-ctrl"
+
+#define HICR7 0x8
+#define HICR8 0xc
+
+struct lpc_ctrl {
+	struct miscdevice	miscdev;
+	struct regmap		*regmap;
+	phys_addr_t		base;
+	resource_size_t		size;
+	uint32_t		pnor_size;
+	uint32_t		pnor_base;
+};
+
+static atomic_t lpc_ctrl_open_count = ATOMIC_INIT(0);
+
+static struct lpc_ctrl *file_lpc_ctrl(struct file *file)
+{
+	return container_of(file->private_data, struct lpc_ctrl, miscdev);
+}
+
+static int lpc_ctrl_mmap(struct file *file, struct vm_area_struct *vma)
+{
+	struct lpc_ctrl *lpc_ctrl = file_lpc_ctrl(file);
+	unsigned long vsize = vma->vm_end - vma->vm_start;
+
+	if (vma->vm_pgoff + vsize > lpc_ctrl->base + lpc_ctrl->size)
+		return -EINVAL;
+
+	/* Other checks? */
+
+	if (remap_pfn_range(vma, vma->vm_start,
+		(lpc_ctrl->base >> PAGE_SHIFT) + vma->vm_pgoff,
+		vsize, vma->vm_page_prot))
+		return -EAGAIN;
+
+	return 0;
+}
+
+static int lpc_ctrl_open(struct inode *inode, struct file *file)
+{
+	if (atomic_inc_return(&lpc_ctrl_open_count) == 1)
+		return 0;
+
+	atomic_dec(&lpc_ctrl_open_count);
+	return -EBUSY;
+}
+
+static ssize_t lpc_ctrl_read(struct file *file, char __user *buf,
+				size_t count, loff_t *ppos)
+{
+	if (!access_ok(VERIFY_WRITE, buf, count))
+		return -EFAULT;
+
+	return -EPERM;
+}
+
+static ssize_t lpc_ctrl_write(struct file *file, const char __user *buf,
+				size_t count, loff_t *ppos)
+{
+	if (!access_ok(VERIFY_READ, buf, count))
+		return -EFAULT;
+
+	return -EPERM;
+}
+
+static long lpc_ctrl_ioctl(struct file *file, unsigned int cmd,
+		unsigned long param)
+{
+	long rc;
+	struct lpc_mapping map;
+	struct lpc_ctrl *lpc_ctrl = file_lpc_ctrl(file);
+	void __user *p = (void __user *)param;
+
+	switch (cmd) {
+	case LPC_CTRL_IOCTL_SIZE:
+		return copy_to_user(p, &lpc_ctrl->size,
+			sizeof(lpc_ctrl->size)) ? -EFAULT : 0;
+	case LPC_CTRL_IOCTL_MAP:
+		if (copy_from_user(&map, p, sizeof(map)))
+			return -EFAULT;
+
+
+		/*
+		 * The top half of HICR7 is the MSB of the BMC address of the
+		 * mapping.
+		 * The bottom half of HICR7 is the MSB of the HOST LPC
+		 * firmware space address of the mapping.
+		 *
+		 * The 1 bits in the top of half of HICR8 represent the bits
+		 * (in the requested address) that should be ignored and
+		 * replaced with those from the top half of HICR7.
+		 * The 1 bits in the bottom half of HICR8 represent the bits
+		 * (in the requested address) that should be kept and pass
+		 * into the BMC address space.
+		 */
+
+		rc = regmap_write(lpc_ctrl->regmap, HICR7,
+				(lpc_ctrl->base | (map.hostaddr >> 16)));
+		if (rc)
+			return rc;
+
+		rc = regmap_write(lpc_ctrl->regmap, HICR8,
+			(~(map.size - 1)) | ((map.size >> 16) - 1));
+		return rc;
+	case LPC_CTRL_IOCTL_UNMAP:
+		/*
+		 * The top nibble in host lpc addresses references which
+		 * firmware space, use space zero hence the & 0x0fff
+		 */
+
+		rc = regmap_write(lpc_ctrl->regmap, HICR7,
+			lpc_ctrl->pnor_base | (((-lpc_ctrl->pnor_size) >> 16) & 0x0fff));
+		if (rc)
+			return rc;
+
+		rc = regmap_write(lpc_ctrl->regmap, HICR8,
+			(~(lpc_ctrl->pnor_size - 1)) | ((lpc_ctrl->pnor_size >> 16) - 1));
+		return rc;
+	}
+
+	return -EINVAL;
+}
+
+static int lpc_ctrl_release(struct inode *inode, struct file *file)
+{
+	atomic_dec(&lpc_ctrl_open_count);
+	return 0;
+}
+
+static const struct file_operations lpc_ctrl_fops = {
+	.owner		= THIS_MODULE,
+	.mmap		= lpc_ctrl_mmap,
+	.open		= lpc_ctrl_open,
+	.read		= lpc_ctrl_read,
+	.write		= lpc_ctrl_write,
+	.release	= lpc_ctrl_release,
+	.unlocked_ioctl	= lpc_ctrl_ioctl,
+};
+
+static int lpc_ctrl_probe(struct platform_device *pdev)
+{
+	struct lpc_ctrl *lpc_ctrl;
+	struct device *dev;
+	struct device_node *node;
+	struct resource resm;
+	int rc;
+
+	if (!pdev || !pdev->dev.of_node)
+		return -ENODEV;
+
+	dev = &pdev->dev;
+
+	lpc_ctrl = devm_kzalloc(dev, sizeof(*lpc_ctrl), GFP_KERNEL);
+	if (!lpc_ctrl)
+		return -ENOMEM;
+
+	node = of_parse_phandle(dev->of_node, "flash", 0);
+	if (!node) {
+		dev_err(dev, "Didn't find host pnor flash node\n");
+		rc = -ENODEV;
+		goto out;
+	}
+
+	rc = of_property_read_u32_index(node, "reg", 3,
+			&lpc_ctrl->pnor_size);
+	if (rc)
+		return rc;
+	rc = of_property_read_u32_index(node, "reg", 2,
+			&lpc_ctrl->pnor_base);
+	if (rc)
+		return rc;
+
+	dev_info(dev, "Host PNOR base: 0x%08x size: 0x%08x\n",
+		lpc_ctrl->pnor_base, lpc_ctrl->pnor_size);
+
+	dev_set_drvdata(&pdev->dev, lpc_ctrl);
+
+	node = of_parse_phandle(dev->of_node, "memory-region", 0);
+	if (!node) {
+		dev_err(dev, "Didn't find reserved memory\n");
+		rc = -EINVAL;
+		goto out;
+	}
+
+	rc = of_address_to_resource(node, 0, &resm);
+	of_node_put(node);
+	if (rc) {
+		dev_err(dev, "Could address to resource\n");
+		rc = -ENOMEM;
+		goto out;
+	}
+
+	lpc_ctrl->size = resource_size(&resm);
+	lpc_ctrl->base = resm.start;
+
+	lpc_ctrl->regmap = syscon_node_to_regmap(
+			pdev->dev.parent->of_node);
+	if (IS_ERR(lpc_ctrl->regmap)) {
+		dev_err(dev, "Couldn't get regmap\n");
+		rc = -ENODEV;
+		goto out;
+	}
+
+	lpc_ctrl->miscdev.minor = MISC_DYNAMIC_MINOR;
+	lpc_ctrl->miscdev.name = DEVICE_NAME;
+	lpc_ctrl->miscdev.fops = &lpc_ctrl_fops;
+	lpc_ctrl->miscdev.parent = dev;
+	rc = misc_register(&lpc_ctrl->miscdev);
+	if (rc)
+		dev_err(dev, "Unable to register device\n");
+	else
+		dev_info(dev, "Loaded at 0x%08x (0x%08x)\n",
+			lpc_ctrl->base, lpc_ctrl->size);
+
+out:
+	return rc;
+}
+
+static int lpc_ctrl_remove(struct platform_device *pdev)
+{
+	struct lpc_ctrl *lpc_ctrl = dev_get_drvdata(&pdev->dev);
+
+	misc_deregister(&lpc_ctrl->miscdev);
+	lpc_ctrl = NULL;
+
+	return 0;
+}
+
+static const struct of_device_id lpc_ctrl_match[] = {
+	{ .compatible = "aspeed,ast2400-lpc-ctrl" },
+	{ },
+};
+
+static struct platform_driver lpc_ctrl_driver = {
+	.driver = {
+		.name		= DEVICE_NAME,
+		.of_match_table = lpc_ctrl_match,
+	},
+	.probe = lpc_ctrl_probe,
+	.remove = lpc_ctrl_remove,
+};
+
+module_platform_driver(lpc_ctrl_driver);
+
+MODULE_DEVICE_TABLE(of, lpc_ctrl_match);
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Cyril Bur <cyrilbur@gmail.com>");
+MODULE_DESCRIPTION("Linux device interface to control LPC bus");
diff --git a/include/uapi/linux/aspeed-lpc-ctrl.h b/include/uapi/linux/aspeed-lpc-ctrl.h
new file mode 100644
index 000000000000..c5f1caf827ac
--- /dev/null
+++ b/include/uapi/linux/aspeed-lpc-ctrl.h
@@ -0,0 +1,25 @@
+/*
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#ifndef _UAPI_LINUX_LPC_CTRL_H
+#define _UAPI_LINUX_LPC_CTRL_H
+
+#include <linux/ioctl.h>
+
+struct lpc_mapping {
+	uint32_t hostaddr;
+	uint32_t size;
+};
+
+#define __LPC_CTRL_IOCTL_MAGIC	0xb2
+#define LPC_CTRL_IOCTL_SIZE	_IOR(__LPC_CTRL_IOCTL_MAGIC, 0x00, uint32_t)
+#define LPC_CTRL_IOCTL_MAP	_IOW(__LPC_CTRL_IOCTL_MAGIC, 0x01, struct lpc_mapping)
+#define LPC_CTRL_IOCTL_UNMAP	_IO(__LPC_CTRL_IOCTL_MAGIC, 0x02)
+
+#endif /* _UAPI_LINUX_LPC_CTRL_H */
-- 
2.11.0

Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver

[Greg KH]

On Thu, Jan 12, 2017 at 11:29:09AM +1100, Cyril Bur wrote:
> This driver exposes a reserved chunk of BMC ram to userspace as well as
> an ioctl interface to control the BMC<->HOST mapping of the LPC bus.
> This allows for a communication channel between the BMC and the host

Why not make this a UIO driver?  Why does it have to be a character
device?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver

[Greg KH]

On Thu, Jan 12, 2017 at 11:29:09AM +1100, Cyril Bur wrote:
> This driver exposes a reserved chunk of BMC ram to userspace as well as
> an ioctl interface to control the BMC<->HOST mapping of the LPC bus.
> This allows for a communication channel between the BMC and the host

Why not make this a UIO driver?  Why does it have to be a character
device?

thanks,

greg k-h

Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver

[Benjamin Herrenschmidt]

On Thu, 2017-01-12 at 08:43 +0100, Greg KH wrote:
> On Thu, Jan 12, 2017 at 11:29:09AM +1100, Cyril Bur wrote:
> > This driver exposes a reserved chunk of BMC ram to userspace as
> > well as
> > an ioctl interface to control the BMC<->HOST mapping of the LPC
> > bus.
> > This allows for a communication channel between the BMC and the
> > host
> 
> Why not make this a UIO driver?  Why does it have to be a character
> device?

See my other email (looks like I'm getting things in reverse order
today ;-), basically it should just be some ioctl's, but what they do
should really be done by the kernel as it controls external access to
part of the chip physical address space.

Cheers,
Ben.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver

[Benjamin Herrenschmidt]

On Thu, 2017-01-12 at 08:43 +0100, Greg KH wrote:
> On Thu, Jan 12, 2017 at 11:29:09AM +1100, Cyril Bur wrote:
> > This driver exposes a reserved chunk of BMC ram to userspace as
> > well as
> > an ioctl interface to control the BMC<->HOST mapping of the LPC
> > bus.
> > This allows for a communication channel between the BMC and the
> > host
> 
> Why not make this a UIO driver?  Why does it have to be a character
> device?

See my other email (looks like I'm getting things in reverse order
today ;-), basically it should just be some ioctl's, but what they do
should really be done by the kernel as it controls external access to
part of the chip physical address space.

Cheers,
Ben.

Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver

[Greg KH]

On Thu, Jan 12, 2017 at 11:29:09AM +1100, Cyril Bur wrote:
> +static ssize_t lpc_ctrl_read(struct file *file, char __user *buf,
> +				size_t count, loff_t *ppos)
> +{
> +	if (!access_ok(VERIFY_WRITE, buf, count))
> +		return -EFAULT;
> +
> +	return -EPERM;
> +}
> +
> +static ssize_t lpc_ctrl_write(struct file *file, const char __user *buf,
> +				size_t count, loff_t *ppos)
> +{
> +	if (!access_ok(VERIFY_READ, buf, count))
> +		return -EFAULT;
> +
> +	return -EPERM;
> +}

Those functions don't actually do anything, so why even include them?

And don't call access_ok(), it's racy and no driver should ever do that.

> +static long lpc_ctrl_ioctl(struct file *file, unsigned int cmd,
> +		unsigned long param)
> +{
> +	long rc;
> +	struct lpc_mapping map;
> +	struct lpc_ctrl *lpc_ctrl = file_lpc_ctrl(file);
> +	void __user *p = (void __user *)param;
> +
> +	switch (cmd) {
> +	case LPC_CTRL_IOCTL_SIZE:
> +		return copy_to_user(p, &lpc_ctrl->size,
> +			sizeof(lpc_ctrl->size)) ? -EFAULT : 0;
> +	case LPC_CTRL_IOCTL_MAP:
> +		if (copy_from_user(&map, p, sizeof(map)))
> +			return -EFAULT;
> +
> +
> +		/*
> +		 * The top half of HICR7 is the MSB of the BMC address of the
> +		 * mapping.
> +		 * The bottom half of HICR7 is the MSB of the HOST LPC
> +		 * firmware space address of the mapping.
> +		 *
> +		 * The 1 bits in the top of half of HICR8 represent the bits
> +		 * (in the requested address) that should be ignored and
> +		 * replaced with those from the top half of HICR7.
> +		 * The 1 bits in the bottom half of HICR8 represent the bits
> +		 * (in the requested address) that should be kept and pass
> +		 * into the BMC address space.
> +		 */
> +
> +		rc = regmap_write(lpc_ctrl->regmap, HICR7,
> +				(lpc_ctrl->base | (map.hostaddr >> 16)));
> +		if (rc)
> +			return rc;
> +
> +		rc = regmap_write(lpc_ctrl->regmap, HICR8,
> +			(~(map.size - 1)) | ((map.size >> 16) - 1));

Look Ma, a kernel exploit!

{sigh}

Your assignment is to go find a whiteboard/blackboard/whatever and write
on it 100 times:
	All input is evil.

I want to see the picture of that before you send any more kernel patches.

> +static int lpc_ctrl_release(struct inode *inode, struct file *file)
> +{
> +	atomic_dec(&lpc_ctrl_open_count);

Totally unneeded and unnecessary, why do you care?

Again, use UIO, it will save you from yourself...

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver

[Greg KH]

On Thu, Jan 12, 2017 at 11:29:09AM +1100, Cyril Bur wrote:
> +static ssize_t lpc_ctrl_read(struct file *file, char __user *buf,
> +				size_t count, loff_t *ppos)
> +{
> +	if (!access_ok(VERIFY_WRITE, buf, count))
> +		return -EFAULT;
> +
> +	return -EPERM;
> +}
> +
> +static ssize_t lpc_ctrl_write(struct file *file, const char __user *buf,
> +				size_t count, loff_t *ppos)
> +{
> +	if (!access_ok(VERIFY_READ, buf, count))
> +		return -EFAULT;
> +
> +	return -EPERM;
> +}

Those functions don't actually do anything, so why even include them?

And don't call access_ok(), it's racy and no driver should ever do that.

> +static long lpc_ctrl_ioctl(struct file *file, unsigned int cmd,
> +		unsigned long param)
> +{
> +	long rc;
> +	struct lpc_mapping map;
> +	struct lpc_ctrl *lpc_ctrl = file_lpc_ctrl(file);
> +	void __user *p = (void __user *)param;
> +
> +	switch (cmd) {
> +	case LPC_CTRL_IOCTL_SIZE:
> +		return copy_to_user(p, &lpc_ctrl->size,
> +			sizeof(lpc_ctrl->size)) ? -EFAULT : 0;
> +	case LPC_CTRL_IOCTL_MAP:
> +		if (copy_from_user(&map, p, sizeof(map)))
> +			return -EFAULT;
> +
> +
> +		/*
> +		 * The top half of HICR7 is the MSB of the BMC address of the
> +		 * mapping.
> +		 * The bottom half of HICR7 is the MSB of the HOST LPC
> +		 * firmware space address of the mapping.
> +		 *
> +		 * The 1 bits in the top of half of HICR8 represent the bits
> +		 * (in the requested address) that should be ignored and
> +		 * replaced with those from the top half of HICR7.
> +		 * The 1 bits in the bottom half of HICR8 represent the bits
> +		 * (in the requested address) that should be kept and pass
> +		 * into the BMC address space.
> +		 */
> +
> +		rc = regmap_write(lpc_ctrl->regmap, HICR7,
> +				(lpc_ctrl->base | (map.hostaddr >> 16)));
> +		if (rc)
> +			return rc;
> +
> +		rc = regmap_write(lpc_ctrl->regmap, HICR8,
> +			(~(map.size - 1)) | ((map.size >> 16) - 1));

Look Ma, a kernel exploit!

{sigh}

Your assignment is to go find a whiteboard/blackboard/whatever and write
on it 100 times:
	All input is evil.

I want to see the picture of that before you send any more kernel patches.

> +static int lpc_ctrl_release(struct inode *inode, struct file *file)
> +{
> +	atomic_dec(&lpc_ctrl_open_count);

Totally unneeded and unnecessary, why do you care?

Again, use UIO, it will save you from yourself...

thanks,

greg k-h

Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver

[Cyril Bur]

On Thu, 2017-01-12 at 08:47 +0100, Greg KH wrote:
> On Thu, Jan 12, 2017 at 11:29:09AM +1100, Cyril Bur wrote:
> > +static ssize_t lpc_ctrl_read(struct file *file, char __user *buf,
> > +				size_t count, loff_t *ppos)
> > +{
> > +	if (!access_ok(VERIFY_WRITE, buf, count))
> > +		return -EFAULT;
> > +
> > +	return -EPERM;
> > +}
> > +
> > +static ssize_t lpc_ctrl_write(struct file *file, const char __user *buf,
> > +				size_t count, loff_t *ppos)
> > +{
> > +	if (!access_ok(VERIFY_READ, buf, count))
> > +		return -EFAULT;
> > +
> > +	return -EPERM;
> > +}
> 

Hello Greg,

> Those functions don't actually do anything, so why even include them?
> 

Apologies, I should be more careful with what I send.

> And don't call access_ok(), it's racy and no driver should ever do that.
> 

Oh, duly noted. I'll be sure to check out how and why. Perhaps it
would be wise that no driver actually do that, I'm quite sure I used
other drivers as examples of best practice.

> > +static long lpc_ctrl_ioctl(struct file *file, unsigned int cmd,
> > +		unsigned long param)
> > +{
> > +	long rc;
> > +	struct lpc_mapping map;
> > +	struct lpc_ctrl *lpc_ctrl = file_lpc_ctrl(file);
> > +	void __user *p = (void __user *)param;
> > +
> > +	switch (cmd) {
> > +	case LPC_CTRL_IOCTL_SIZE:
> > +		return copy_to_user(p, &lpc_ctrl->size,
> > +			sizeof(lpc_ctrl->size)) ? -EFAULT : 0;
> > +	case LPC_CTRL_IOCTL_MAP:
> > +		if (copy_from_user(&map, p, sizeof(map)))
> > +			return -EFAULT;
> > +
> > +
> > +		/*
> > +		 * The top half of HICR7 is the MSB of the BMC address of the
> > +		 * mapping.
> > +		 * The bottom half of HICR7 is the MSB of the HOST LPC
> > +		 * firmware space address of the mapping.
> > +		 *
> > +		 * The 1 bits in the top of half of HICR8 represent the bits
> > +		 * (in the requested address) that should be ignored and
> > +		 * replaced with those from the top half of HICR7.
> > +		 * The 1 bits in the bottom half of HICR8 represent the bits
> > +		 * (in the requested address) that should be kept and pass
> > +		 * into the BMC address space.
> > +		 */
> > +
> > +		rc = regmap_write(lpc_ctrl->regmap, HICR7,
> > +				(lpc_ctrl->base | (map.hostaddr >> 16)));
> > +		if (rc)
> > +			return rc;
> > +
> > +		rc = regmap_write(lpc_ctrl->regmap, HICR8,
> > +			(~(map.size - 1)) | ((map.size >> 16) - 1));
> 
> Look Ma, a kernel exploit!
> 

So 'evil' input here could allow the host to control the bmc,
personally I file that under 'stupid' input. Also, stupid but not
accidental, I don't believe one could accidentally come up with such
input, although you never know what silly things human beings sometimes
do. For what its worth, I'm not even sure that can happen but I'll
grant you the benifit of the doubt.

> {sigh}
> 
> Your assignment is to go find a whiteboard/blackboard/whatever and write
> on it 100 times:
> 	All input is evil.
> 
> I want to see the picture of that before you send any more kernel patches.
> 
> > +static int lpc_ctrl_release(struct inode *inode, struct file *file)
> > +{
> > +	atomic_dec(&lpc_ctrl_open_count);
> 
> Totally unneeded and unnecessary, why do you care?
> 

My aim here was to only have one process playing with the LPC mapping
registers at a time.

> Again, use UIO, it will save you from yourself...
> 

Thank-you! This is the first I've heard of UIO and I'll investigate
furthur!



Sincerely,

Cyril

> thanks,
> 
> greg k-h
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver

[Cyril Bur]

On Thu, 2017-01-12 at 08:47 +0100, Greg KH wrote:
> On Thu, Jan 12, 2017 at 11:29:09AM +1100, Cyril Bur wrote:
> > +static ssize_t lpc_ctrl_read(struct file *file, char __user *buf,
> > +				size_t count, loff_t *ppos)
> > +{
> > +	if (!access_ok(VERIFY_WRITE, buf, count))
> > +		return -EFAULT;
> > +
> > +	return -EPERM;
> > +}
> > +
> > +static ssize_t lpc_ctrl_write(struct file *file, const char __user *buf,
> > +				size_t count, loff_t *ppos)
> > +{
> > +	if (!access_ok(VERIFY_READ, buf, count))
> > +		return -EFAULT;
> > +
> > +	return -EPERM;
> > +}
> 

Hello Greg,

> Those functions don't actually do anything, so why even include them?
> 

Apologies, I should be more careful with what I send.

> And don't call access_ok(), it's racy and no driver should ever do that.
> 

Oh, duly noted. I'll be sure to check out how and why. Perhaps it
would be wise that no driver actually do that, I'm quite sure I used
other drivers as examples of best practice.

> > +static long lpc_ctrl_ioctl(struct file *file, unsigned int cmd,
> > +		unsigned long param)
> > +{
> > +	long rc;
> > +	struct lpc_mapping map;
> > +	struct lpc_ctrl *lpc_ctrl = file_lpc_ctrl(file);
> > +	void __user *p = (void __user *)param;
> > +
> > +	switch (cmd) {
> > +	case LPC_CTRL_IOCTL_SIZE:
> > +		return copy_to_user(p, &lpc_ctrl->size,
> > +			sizeof(lpc_ctrl->size)) ? -EFAULT : 0;
> > +	case LPC_CTRL_IOCTL_MAP:
> > +		if (copy_from_user(&map, p, sizeof(map)))
> > +			return -EFAULT;
> > +
> > +
> > +		/*
> > +		 * The top half of HICR7 is the MSB of the BMC address of the
> > +		 * mapping.
> > +		 * The bottom half of HICR7 is the MSB of the HOST LPC
> > +		 * firmware space address of the mapping.
> > +		 *
> > +		 * The 1 bits in the top of half of HICR8 represent the bits
> > +		 * (in the requested address) that should be ignored and
> > +		 * replaced with those from the top half of HICR7.
> > +		 * The 1 bits in the bottom half of HICR8 represent the bits
> > +		 * (in the requested address) that should be kept and pass
> > +		 * into the BMC address space.
> > +		 */
> > +
> > +		rc = regmap_write(lpc_ctrl->regmap, HICR7,
> > +				(lpc_ctrl->base | (map.hostaddr >> 16)));
> > +		if (rc)
> > +			return rc;
> > +
> > +		rc = regmap_write(lpc_ctrl->regmap, HICR8,
> > +			(~(map.size - 1)) | ((map.size >> 16) - 1));
> 
> Look Ma, a kernel exploit!
> 

So 'evil' input here could allow the host to control the bmc,
personally I file that under 'stupid' input. Also, stupid but not
accidental, I don't believe one could accidentally come up with such
input, although you never know what silly things human beings sometimes
do. For what its worth, I'm not even sure that can happen but I'll
grant you the benifit of the doubt.

> {sigh}
> 
> Your assignment is to go find a whiteboard/blackboard/whatever and write
> on it 100 times:
> 	All input is evil.
> 
> I want to see the picture of that before you send any more kernel patches.
> 
> > +static int lpc_ctrl_release(struct inode *inode, struct file *file)
> > +{
> > +	atomic_dec(&lpc_ctrl_open_count);
> 
> Totally unneeded and unnecessary, why do you care?
> 

My aim here was to only have one process playing with the LPC mapping
registers at a time.

> Again, use UIO, it will save you from yourself...
> 

Thank-you! This is the first I've heard of UIO and I'll investigate
furthur!



Sincerely,

Cyril

> thanks,
> 
> greg k-h

Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver

[Greg KH]

On Thu, Jan 12, 2017 at 09:16:03PM +1100, Cyril Bur wrote:
> On Thu, 2017-01-12 at 08:47 +0100, Greg KH wrote:
> > On Thu, Jan 12, 2017 at 11:29:09AM +1100, Cyril Bur wrote:
> > > +static ssize_t lpc_ctrl_read(struct file *file, char __user *buf,
> > > +				size_t count, loff_t *ppos)
> > > +{
> > > +	if (!access_ok(VERIFY_WRITE, buf, count))
> > > +		return -EFAULT;
> > > +
> > > +	return -EPERM;
> > > +}
> > > +
> > > +static ssize_t lpc_ctrl_write(struct file *file, const char __user *buf,
> > > +				size_t count, loff_t *ppos)
> > > +{
> > > +	if (!access_ok(VERIFY_READ, buf, count))
> > > +		return -EFAULT;
> > > +
> > > +	return -EPERM;
> > > +}
> > 
> 
> Hello Greg,
> 
> > Those functions don't actually do anything, so why even include them?
> > 
> 
> Apologies, I should be more careful with what I send.

Hm, that implies you never tested what you sent, nor intended for us to
merge it, an odd thing for you to do :)

> > And don't call access_ok(), it's racy and no driver should ever do that.
> > 
> 
> Oh, duly noted. I'll be sure to check out how and why. Perhaps it
> would be wise that no driver actually do that, I'm quite sure I used
> other drivers as examples of best practice.

You did?  Please point me at that code so we can fix them up properly.
Cargo-cult coding is not a good thing, but it happens, so if we can at
least provide clean code to fixate on, it's good overall for everyone.

> > > +static long lpc_ctrl_ioctl(struct file *file, unsigned int cmd,
> > > +		unsigned long param)
> > > +{
> > > +	long rc;
> > > +	struct lpc_mapping map;
> > > +	struct lpc_ctrl *lpc_ctrl = file_lpc_ctrl(file);
> > > +	void __user *p = (void __user *)param;
> > > +
> > > +	switch (cmd) {
> > > +	case LPC_CTRL_IOCTL_SIZE:
> > > +		return copy_to_user(p, &lpc_ctrl->size,
> > > +			sizeof(lpc_ctrl->size)) ? -EFAULT : 0;
> > > +	case LPC_CTRL_IOCTL_MAP:
> > > +		if (copy_from_user(&map, p, sizeof(map)))
> > > +			return -EFAULT;
> > > +
> > > +
> > > +		/*
> > > +		 * The top half of HICR7 is the MSB of the BMC address of the
> > > +		 * mapping.
> > > +		 * The bottom half of HICR7 is the MSB of the HOST LPC
> > > +		 * firmware space address of the mapping.
> > > +		 *
> > > +		 * The 1 bits in the top of half of HICR8 represent the bits
> > > +		 * (in the requested address) that should be ignored and
> > > +		 * replaced with those from the top half of HICR7.
> > > +		 * The 1 bits in the bottom half of HICR8 represent the bits
> > > +		 * (in the requested address) that should be kept and pass
> > > +		 * into the BMC address space.
> > > +		 */
> > > +
> > > +		rc = regmap_write(lpc_ctrl->regmap, HICR7,
> > > +				(lpc_ctrl->base | (map.hostaddr >> 16)));
> > > +		if (rc)
> > > +			return rc;
> > > +
> > > +		rc = regmap_write(lpc_ctrl->regmap, HICR8,
> > > +			(~(map.size - 1)) | ((map.size >> 16) - 1));
> > 
> > Look Ma, a kernel exploit!
> > 
> 
> So 'evil' input here could allow the host to control the bmc,
> personally I file that under 'stupid' input. Also, stupid but not
> accidental, I don't believe one could accidentally come up with such
> input, although you never know what silly things human beings sometimes
> do. For what its worth, I'm not even sure that can happen but I'll
> grant you the benifit of the doubt.

I think you didn't get the main point here, again:

> > {sigh}
> > 
> > Your assignment is to go find a whiteboard/blackboard/whatever and write
> > on it 100 times:
> > 	All input is evil.

You can NEVER trust any input values sent to the kernel, you have to
ALWAYS verify they are within certain safe ranges.


> > I want to see the picture of that before you send any more kernel patches.
> > 
> > > +static int lpc_ctrl_release(struct inode *inode, struct file *file)
> > > +{
> > > +	atomic_dec(&lpc_ctrl_open_count);
> > 
> > Totally unneeded and unnecessary, why do you care?
> > 
> 
> My aim here was to only have one process playing with the LPC mapping
> registers at a time.

Why?  Who cares?  You don't have internal state being kept by the
driver, so it shouldn't matter.

And again, don't treat an atomic variable as a lock, use a real lock for
the task, it works better, and is the correct thing to do.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver

[Greg KH]

On Thu, Jan 12, 2017 at 09:16:03PM +1100, Cyril Bur wrote:
> On Thu, 2017-01-12 at 08:47 +0100, Greg KH wrote:
> > On Thu, Jan 12, 2017 at 11:29:09AM +1100, Cyril Bur wrote:
> > > +static ssize_t lpc_ctrl_read(struct file *file, char __user *buf,
> > > +				size_t count, loff_t *ppos)
> > > +{
> > > +	if (!access_ok(VERIFY_WRITE, buf, count))
> > > +		return -EFAULT;
> > > +
> > > +	return -EPERM;
> > > +}
> > > +
> > > +static ssize_t lpc_ctrl_write(struct file *file, const char __user *buf,
> > > +				size_t count, loff_t *ppos)
> > > +{
> > > +	if (!access_ok(VERIFY_READ, buf, count))
> > > +		return -EFAULT;
> > > +
> > > +	return -EPERM;
> > > +}
> > 
> 
> Hello Greg,
> 
> > Those functions don't actually do anything, so why even include them?
> > 
> 
> Apologies, I should be more careful with what I send.

Hm, that implies you never tested what you sent, nor intended for us to
merge it, an odd thing for you to do :)

> > And don't call access_ok(), it's racy and no driver should ever do that.
> > 
> 
> Oh, duly noted. I'll be sure to check out how and why. Perhaps it
> would be wise that no driver actually do that, I'm quite sure I used
> other drivers as examples of best practice.

You did?  Please point me at that code so we can fix them up properly.
Cargo-cult coding is not a good thing, but it happens, so if we can at
least provide clean code to fixate on, it's good overall for everyone.

> > > +static long lpc_ctrl_ioctl(struct file *file, unsigned int cmd,
> > > +		unsigned long param)
> > > +{
> > > +	long rc;
> > > +	struct lpc_mapping map;
> > > +	struct lpc_ctrl *lpc_ctrl = file_lpc_ctrl(file);
> > > +	void __user *p = (void __user *)param;
> > > +
> > > +	switch (cmd) {
> > > +	case LPC_CTRL_IOCTL_SIZE:
> > > +		return copy_to_user(p, &lpc_ctrl->size,
> > > +			sizeof(lpc_ctrl->size)) ? -EFAULT : 0;
> > > +	case LPC_CTRL_IOCTL_MAP:
> > > +		if (copy_from_user(&map, p, sizeof(map)))
> > > +			return -EFAULT;
> > > +
> > > +
> > > +		/*
> > > +		 * The top half of HICR7 is the MSB of the BMC address of the
> > > +		 * mapping.
> > > +		 * The bottom half of HICR7 is the MSB of the HOST LPC
> > > +		 * firmware space address of the mapping.
> > > +		 *
> > > +		 * The 1 bits in the top of half of HICR8 represent the bits
> > > +		 * (in the requested address) that should be ignored and
> > > +		 * replaced with those from the top half of HICR7.
> > > +		 * The 1 bits in the bottom half of HICR8 represent the bits
> > > +		 * (in the requested address) that should be kept and pass
> > > +		 * into the BMC address space.
> > > +		 */
> > > +
> > > +		rc = regmap_write(lpc_ctrl->regmap, HICR7,
> > > +				(lpc_ctrl->base | (map.hostaddr >> 16)));
> > > +		if (rc)
> > > +			return rc;
> > > +
> > > +		rc = regmap_write(lpc_ctrl->regmap, HICR8,
> > > +			(~(map.size - 1)) | ((map.size >> 16) - 1));
> > 
> > Look Ma, a kernel exploit!
> > 
> 
> So 'evil' input here could allow the host to control the bmc,
> personally I file that under 'stupid' input. Also, stupid but not
> accidental, I don't believe one could accidentally come up with such
> input, although you never know what silly things human beings sometimes
> do. For what its worth, I'm not even sure that can happen but I'll
> grant you the benifit of the doubt.

I think you didn't get the main point here, again:

> > {sigh}
> > 
> > Your assignment is to go find a whiteboard/blackboard/whatever and write
> > on it 100 times:
> > 	All input is evil.

You can NEVER trust any input values sent to the kernel, you have to
ALWAYS verify they are within certain safe ranges.


> > I want to see the picture of that before you send any more kernel patches.
> > 
> > > +static int lpc_ctrl_release(struct inode *inode, struct file *file)
> > > +{
> > > +	atomic_dec(&lpc_ctrl_open_count);
> > 
> > Totally unneeded and unnecessary, why do you care?
> > 
> 
> My aim here was to only have one process playing with the LPC mapping
> registers at a time.

Why?  Who cares?  You don't have internal state being kept by the
driver, so it shouldn't matter.

And again, don't treat an atomic variable as a lock, use a real lock for
the task, it works better, and is the correct thing to do.

thanks,

greg k-h

Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver

[Benjamin Herrenschmidt]

On Thu, 2017-01-12 at 11:30 +0100, Greg KH wrote:
> > > And don't call access_ok(), it's racy and no driver should ever do that.
> > > 
> > 
> > Oh, duly noted. I'll be sure to check out how and why. Perhaps it
> > would be wise that no driver actually do that, I'm quite sure I used
> > other drivers as examples of best practice.
> 
> You did?  Please point me at that code so we can fix them up properly.
> Cargo-cult coding is not a good thing, but it happens, so if we can at
> least provide clean code to fixate on, it's good overall for everyone.

How so ? I mean, access_ok followed by __get/__put_user is still a
classic, what's wrong with it ?

The test performed by access_ok tests whether the address hits the
kernel/userspace limit, that limit doesn't change afaik, so there is no
race there, and avoids repeatedly testing it for series of subsequent
__get_user or __put_user.

What's wrong with them ?

Cheers,
Ben.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver

[Benjamin Herrenschmidt]

On Thu, 2017-01-12 at 11:30 +0100, Greg KH wrote:
> > > And don't call access_ok(), it's racy and no driver should ever do that.
> > > 
> > 
> > Oh, duly noted. I'll be sure to check out how and why. Perhaps it
> > would be wise that no driver actually do that, I'm quite sure I used
> > other drivers as examples of best practice.
> 
> You did?  Please point me at that code so we can fix them up properly.
> Cargo-cult coding is not a good thing, but it happens, so if we can at
> least provide clean code to fixate on, it's good overall for everyone.

How so ? I mean, access_ok followed by __get/__put_user is still a
classic, what's wrong with it ?

The test performed by access_ok tests whether the address hits the
kernel/userspace limit, that limit doesn't change afaik, so there is no
race there, and avoids repeatedly testing it for series of subsequent
__get_user or __put_user.

What's wrong with them ?

Cheers,
Ben.

Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver

[Greg KH]

On Thu, Jan 12, 2017 at 09:27:47AM -0600, Benjamin Herrenschmidt wrote:
> On Thu, 2017-01-12 at 11:30 +0100, Greg KH wrote:
> > > > And don't call access_ok(), it's racy and no driver should ever do that.
> > > > 
> > > 
> > > Oh, duly noted. I'll be sure to check out how and why. Perhaps it
> > > would be wise that no driver actually do that, I'm quite sure I used
> > > other drivers as examples of best practice.
> > 
> > You did?  Please point me at that code so we can fix them up properly.
> > Cargo-cult coding is not a good thing, but it happens, so if we can at
> > least provide clean code to fixate on, it's good overall for everyone.
> 
> How so ? I mean, access_ok followed by __get/__put_user is still a
> classic, what's wrong with it ?

No "normal" driver should do that, just call copy_to/from_user and be
done with it.  That way all of the proper locking and validation checks
like this are done correctly for you.  Why would a driver ever call the
"raw" __get/__put_user functions?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver

[Greg KH]

On Thu, Jan 12, 2017 at 09:27:47AM -0600, Benjamin Herrenschmidt wrote:
> On Thu, 2017-01-12 at 11:30 +0100, Greg KH wrote:
> > > > And don't call access_ok(), it's racy and no driver should ever do that.
> > > > 
> > > 
> > > Oh, duly noted. I'll be sure to check out how and why. Perhaps it
> > > would be wise that no driver actually do that, I'm quite sure I used
> > > other drivers as examples of best practice.
> > 
> > You did?  Please point me at that code so we can fix them up properly.
> > Cargo-cult coding is not a good thing, but it happens, so if we can at
> > least provide clean code to fixate on, it's good overall for everyone.
> 
> How so ? I mean, access_ok followed by __get/__put_user is still a
> classic, what's wrong with it ?

No "normal" driver should do that, just call copy_to/from_user and be
done with it.  That way all of the proper locking and validation checks
like this are done correctly for you.  Why would a driver ever call the
"raw" __get/__put_user functions?

thanks,

greg k-h

Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver

[Benjamin Herrenschmidt]

On Thu, 2017-01-12 at 17:00 +0100, Greg KH wrote:
> > How so ? I mean, access_ok followed by __get/__put_user is still a
> > classic, what's wrong with it ?
> 
> No "normal" driver should do that, just call copy_to/from_user and be
> done with it.  That way all of the proper locking and validation checks
> like this are done correctly for you.  Why would a driver ever call the
> "raw" __get/__put_user functions?

I supposed historically it was considered faster for some things :-)

Not a huge deal, and yes it's probably cleaner, I was just wondering
what was "racy" about access_ok() that I might have missed...

Cheers,
Ben.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver

[Benjamin Herrenschmidt]

On Thu, 2017-01-12 at 17:00 +0100, Greg KH wrote:
> > How so ? I mean, access_ok followed by __get/__put_user is still a
> > classic, what's wrong with it ?
> 
> No "normal" driver should do that, just call copy_to/from_user and be
> done with it.  That way all of the proper locking and validation checks
> like this are done correctly for you.  Why would a driver ever call the
> "raw" __get/__put_user functions?

I supposed historically it was considered faster for some things :-)

Not a huge deal, and yes it's probably cleaner, I was just wondering
what was "racy" about access_ok() that I might have missed...

Cheers,
Ben.

Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver

[Greg KH]

On Thu, Jan 12, 2017 at 10:07:33AM -0600, Benjamin Herrenschmidt wrote:
> On Thu, 2017-01-12 at 17:00 +0100, Greg KH wrote:
> > > How so ? I mean, access_ok followed by __get/__put_user is still a
> > > classic, what's wrong with it ?
> > 
> > No "normal" driver should do that, just call copy_to/from_user and be
> > done with it.  That way all of the proper locking and validation checks
> > like this are done correctly for you.  Why would a driver ever call the
> > "raw" __get/__put_user functions?
> 
> I supposed historically it was considered faster for some things :-)
> 
> Not a huge deal, and yes it's probably cleaner, I was just wondering
> what was "racy" about access_ok() that I might have missed...

I think, you can change things after access_ok() happens, there used to
be bugs in that area a few years ago.  I think we fixed them by moving
the offending drivers to use copy_*() instead.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver

[Greg KH]

On Thu, Jan 12, 2017 at 10:07:33AM -0600, Benjamin Herrenschmidt wrote:
> On Thu, 2017-01-12 at 17:00 +0100, Greg KH wrote:
> > > How so ? I mean, access_ok followed by __get/__put_user is still a
> > > classic, what's wrong with it ?
> > 
> > No "normal" driver should do that, just call copy_to/from_user and be
> > done with it.  That way all of the proper locking and validation checks
> > like this are done correctly for you.  Why would a driver ever call the
> > "raw" __get/__put_user functions?
> 
> I supposed historically it was considered faster for some things :-)
> 
> Not a huge deal, and yes it's probably cleaner, I was just wondering
> what was "racy" about access_ok() that I might have missed...

I think, you can change things after access_ok() happens, there used to
be bugs in that area a few years ago.  I think we fixed them by moving
the offending drivers to use copy_*() instead.

thanks,

greg k-h

Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver

[Benjamin Herrenschmidt]

On Thu, 2017-01-12 at 17:26 +0100, Greg KH wrote:
> I think, you can change things after access_ok() happens, there used to
> be bugs in that area a few years ago.  I think we fixed them by moving
> the offending drivers to use copy_*() instead.

Ok, I'm surprised though, we still have a metric ton of code,
especially in filesystems, who do access_ok. Generally the idea here is
that the enforcement is done by the MMU normally via the permission in
the page tables. access_ok() is simply needed to make sure we access
the portion of the page tables representing user space, not kernel
space, and that is a pretty fixed dichotomy.

Anyway, this is academic, I agree that copy_to/from_... is nicer.

Cheers,
Ben.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver

[Benjamin Herrenschmidt]

On Thu, 2017-01-12 at 17:26 +0100, Greg KH wrote:
> I think, you can change things after access_ok() happens, there used to
> be bugs in that area a few years ago.  I think we fixed them by moving
> the offending drivers to use copy_*() instead.

Ok, I'm surprised though, we still have a metric ton of code,
especially in filesystems, who do access_ok. Generally the idea here is
that the enforcement is done by the MMU normally via the permission in
the page tables. access_ok() is simply needed to make sure we access
the portion of the page tables representing user space, not kernel
space, and that is a pretty fixed dichotomy.

Anyway, this is academic, I agree that copy_to/from_... is nicer.

Cheers,
Ben.

Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver

[Benjamin Herrenschmidt]

On Thu, 2017-01-12 at 21:16 +1100, Cyril Bur wrote:
> My aim here was to only have one process playing with the LPC mapping
> registers at a time.
> 
> > Again, use UIO, it will save you from yourself...
> > 
> 
> Thank-you! This is the first I've heard of UIO and I'll investigate
> furthur!

Greg, I don't think UIO is the answer here either. Note, this isn't an
exploit so much as root shooting itself in the foot as this driver
should never be accessed by anybody but root, but see below.

This is a BMC, ie, the system controller of a x86 or POWER based
system.

The LPC controller controls the LPC bus which is mastered by the "host"
(ie. the x86 or PPC) and acts as a slave on the BMC side.

It has a bunch of registers that need to be configured in more/less
system specific ways by the BMC, but more so, it has a pair of
registers that allow "mapping" of a region of the BMC physical address 
space into the host address space.

This is by definition dangerous to configure since it gives you a
window to any part of the BMC, kernel space, any IOs, etc... however it
needs to be configured by a userspace daemon which communicates with
the host via a mailbox in order to map either different portions of the
system flash controller address space or reserved memory.

So in fact it should be done by the kernel, not userspace.

What Cyril needs to do to make it more secure is:

  - For random register accesses, white list what registers
specifically are allowed (and if necessary filter values). These
registers aren't dangerous from the BMC perspective and need to be set
appropriately for the host to operate correctly.

  - For the mapping of the LPC FW space <-> BMC space, use ioctl's to
explicit establish the mapping  to a portion of the flash (and nowhere
else) or one of the known reserved memory areas. IE, dont have
userspace just pass raw physical addresses through, but tell the kernel
driver what portion (offset/size) of what area (flash space or reserved
memory region) to configure the HW window for.

Cheers,
Ben.


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver

[Benjamin Herrenschmidt]

On Thu, 2017-01-12 at 21:16 +1100, Cyril Bur wrote:
> My aim here was to only have one process playing with the LPC mapping
> registers at a time.
> 
> > Again, use UIO, it will save you from yourself...
> > 
> 
> Thank-you! This is the first I've heard of UIO and I'll investigate
> furthur!

Greg, I don't think UIO is the answer here either. Note, this isn't an
exploit so much as root shooting itself in the foot as this driver
should never be accessed by anybody but root, but see below.

This is a BMC, ie, the system controller of a x86 or POWER based
system.

The LPC controller controls the LPC bus which is mastered by the "host"
(ie. the x86 or PPC) and acts as a slave on the BMC side.

It has a bunch of registers that need to be configured in more/less
system specific ways by the BMC, but more so, it has a pair of
registers that allow "mapping" of a region of the BMC physical address 
space into the host address space.

This is by definition dangerous to configure since it gives you a
window to any part of the BMC, kernel space, any IOs, etc... however it
needs to be configured by a userspace daemon which communicates with
the host via a mailbox in order to map either different portions of the
system flash controller address space or reserved memory.

So in fact it should be done by the kernel, not userspace.

What Cyril needs to do to make it more secure is:

  - For random register accesses, white list what registers
specifically are allowed (and if necessary filter values). These
registers aren't dangerous from the BMC perspective and need to be set
appropriately for the host to operate correctly.

  - For the mapping of the LPC FW space <-> BMC space, use ioctl's to
explicit establish the mapping  to a portion of the flash (and nowhere
else) or one of the known reserved memory areas. IE, dont have
userspace just pass raw physical addresses through, but tell the kernel
driver what portion (offset/size) of what area (flash space or reserved
memory region) to configure the HW window for.

Cheers,
Ben.

Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver

[Greg KH]

On Thu, Jan 12, 2017 at 09:35:15AM -0600, Benjamin Herrenschmidt wrote:
> On Thu, 2017-01-12 at 21:16 +1100, Cyril Bur wrote:
> > My aim here was to only have one process playing with the LPC mapping
> > registers at a time.
> > 
> > > Again, use UIO, it will save you from yourself...
> > > 
> > 
> > Thank-you! This is the first I've heard of UIO and I'll investigate
> > furthur!
> 
> Greg, I don't think UIO is the answer here either. Note, this isn't an
> exploit so much as root shooting itself in the foot as this driver
> should never be accessed by anybody but root, but see below.
> 
> This is a BMC, ie, the system controller of a x86 or POWER based
> system.
> 
> The LPC controller controls the LPC bus which is mastered by the "host"
> (ie. the x86 or PPC) and acts as a slave on the BMC side.
> 
> It has a bunch of registers that need to be configured in more/less
> system specific ways by the BMC, but more so, it has a pair of
> registers that allow "mapping" of a region of the BMC physical address 
> space into the host address space.
> 
> This is by definition dangerous to configure since it gives you a
> window to any part of the BMC, kernel space, any IOs, etc... however it
> needs to be configured by a userspace daemon which communicates with
> the host via a mailbox in order to map either different portions of the
> system flash controller address space or reserved memory.
> 
> So in fact it should be done by the kernel, not userspace.
> 
> What Cyril needs to do to make it more secure is:
> 
>   - For random register accesses, white list what registers
> specifically are allowed (and if necessary filter values). These
> registers aren't dangerous from the BMC perspective and need to be set
> appropriately for the host to operate correctly.
> 
>   - For the mapping of the LPC FW space <-> BMC space, use ioctl's to
> explicit establish the mapping  to a portion of the flash (and nowhere
> else) or one of the known reserved memory areas. IE, dont have
> userspace just pass raw physical addresses through, but tell the kernel
> driver what portion (offset/size) of what area (flash space or reserved
> memory region) to configure the HW window for.

Yes, something more needs to be documented here, as what was proposed
isn't acceptable at all.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver

[Greg KH]

On Thu, Jan 12, 2017 at 09:35:15AM -0600, Benjamin Herrenschmidt wrote:
> On Thu, 2017-01-12 at 21:16 +1100, Cyril Bur wrote:
> > My aim here was to only have one process playing with the LPC mapping
> > registers at a time.
> > 
> > > Again, use UIO, it will save you from yourself...
> > > 
> > 
> > Thank-you! This is the first I've heard of UIO and I'll investigate
> > furthur!
> 
> Greg, I don't think UIO is the answer here either. Note, this isn't an
> exploit so much as root shooting itself in the foot as this driver
> should never be accessed by anybody but root, but see below.
> 
> This is a BMC, ie, the system controller of a x86 or POWER based
> system.
> 
> The LPC controller controls the LPC bus which is mastered by the "host"
> (ie. the x86 or PPC) and acts as a slave on the BMC side.
> 
> It has a bunch of registers that need to be configured in more/less
> system specific ways by the BMC, but more so, it has a pair of
> registers that allow "mapping" of a region of the BMC physical address 
> space into the host address space.
> 
> This is by definition dangerous to configure since it gives you a
> window to any part of the BMC, kernel space, any IOs, etc... however it
> needs to be configured by a userspace daemon which communicates with
> the host via a mailbox in order to map either different portions of the
> system flash controller address space or reserved memory.
> 
> So in fact it should be done by the kernel, not userspace.
> 
> What Cyril needs to do to make it more secure is:
> 
>   - For random register accesses, white list what registers
> specifically are allowed (and if necessary filter values). These
> registers aren't dangerous from the BMC perspective and need to be set
> appropriately for the host to operate correctly.
> 
>   - For the mapping of the LPC FW space <-> BMC space, use ioctl's to
> explicit establish the mapping  to a portion of the flash (and nowhere
> else) or one of the known reserved memory areas. IE, dont have
> userspace just pass raw physical addresses through, but tell the kernel
> driver what portion (offset/size) of what area (flash space or reserved
> memory region) to configure the HW window for.

Yes, something more needs to be documented here, as what was proposed
isn't acceptable at all.

thanks,

greg k-h

Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver

[Benjamin Herrenschmidt]

On Thu, 2017-01-12 at 09:35 -0600, Benjamin Herrenschmidt wrote:
> Greg, I don't think UIO is the answer here either. Note, this isn't an
> exploit so much as root shooting itself in the foot as this driver
> should never be accessed by anybody but root, but see below.

Reading back my previous email I realize that the lack of coffee
made my prose a lot less clear than I intended it to be :-)

I think some background is in order here, it will help whoever
reviews this and Cyril, skip to the bottom to how I think you should
articulate the driver.

So on a bunch of server systems, you have a system controller typically
known as a BMC controller all sort of things such as power to various
elements, sometimes fans, often the system flash, etc...

The Aspeed BMC family which is what is used on OpenPOWER machines and a
number of x86 as well is typically connected to the host via an LPC
bus. (among others).

This is an ISA bus on steroids, it has IO and MEM/FW cycles (different
address spaces, the subtle differences between MEM and FW can be
ignored for the sake of this discussion). It's generally used by the
BMC chip to provide the host with access to the system flash that
contains the BIOS or other host firmware (via MEM/FW space) along with
a number of SuperIO-style IOs (via IO space) such as UARTs, IPMI
controllers, etc....

On the BMC chip side, this is all configured via a bunch of registers
whose content is related to a given policy of what devices are exposed
how and where on a given system, which is system/vendor specific, so we
don't want to bolt that into the BMC kernel. So this started with a
need to provide something nicer than /dev/mem for user space to
configure these things. At that point, something like UIO could have
still made sense. However...

One important aspect of the configuration is how the MEM/FW space is
exposed to the host (ie, the x86 or POWER). Some registers in that
bridge can define a window remapping all or portion of the LPC MEM/FW
space to a portion of the BMC internal bus, with no specific limits
imposed in HW.

As you can see, this can be pretty nasty. So for this, I think it makes
sense to ensure that this window is configured by a kernel driver that
can apply some serious sanity checks on what it is configured to map.

In practice, user space wants to control this by flipping the mapping
between essentially two types of portions of the BMC address space:

   - The flash space. This is a region of the BMC MMIO space that
more/less directly maps the system flash (at least for reads, writes
are somewhat more complicated).

   - One (or more) reserved area(s) of the BMC physical memory.

The latter is needed for a number of things, such as avoiding letting
the host manipulate the innards of the BMC flash controller via some
evil backdoor, we want to do flash updates by routing the window to a
portion of memory (under control of a mailbox protocol via some
separate set of registers) which the host can use to write new data in
bulk and then request the BMC to flash it. There are other uses, such
as allowing the host to boot from an in-memory flash image rather than
the one in flash (very handy for continuous integration and test, the
BMC can just download new images), etc...

So I think the best approach here is:

	- A pair of ioctls to read and write random registers in the
LPC bridge for all the "generally configuration gunk". These have a
filter to ensure that the registers controlling the above mapping
cannot be accessed that way.

	- An ioctl to control the above mapping window. It takes as
arguments the location in LPC space, the window type (flash vs.
memory), for memory, maybe an ID (several windows to chose from), and
the offset& size in the latter. The driver can enforce that the windows
are one of the specially reserved areas of memory etc...

	- An mmap function to map those reserved windows into userspace
so the daemon can communicate appropriately (only needed for the memory
windows, the flash space is accessed via the normal /dev/mtd drivers)

Greg, does that make sense ?

Cheers,
Ben.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver

[Benjamin Herrenschmidt]

On Thu, 2017-01-12 at 09:35 -0600, Benjamin Herrenschmidt wrote:
> Greg, I don't think UIO is the answer here either. Note, this isn't an
> exploit so much as root shooting itself in the foot as this driver
> should never be accessed by anybody but root, but see below.

Reading back my previous email I realize that the lack of coffee
made my prose a lot less clear than I intended it to be :-)

I think some background is in order here, it will help whoever
reviews this and Cyril, skip to the bottom to how I think you should
articulate the driver.

So on a bunch of server systems, you have a system controller typically
known as a BMC controller all sort of things such as power to various
elements, sometimes fans, often the system flash, etc...

The Aspeed BMC family which is what is used on OpenPOWER machines and a
number of x86 as well is typically connected to the host via an LPC
bus. (among others).

This is an ISA bus on steroids, it has IO and MEM/FW cycles (different
address spaces, the subtle differences between MEM and FW can be
ignored for the sake of this discussion). It's generally used by the
BMC chip to provide the host with access to the system flash that
contains the BIOS or other host firmware (via MEM/FW space) along with
a number of SuperIO-style IOs (via IO space) such as UARTs, IPMI
controllers, etc....

On the BMC chip side, this is all configured via a bunch of registers
whose content is related to a given policy of what devices are exposed
how and where on a given system, which is system/vendor specific, so we
don't want to bolt that into the BMC kernel. So this started with a
need to provide something nicer than /dev/mem for user space to
configure these things. At that point, something like UIO could have
still made sense. However...

One important aspect of the configuration is how the MEM/FW space is
exposed to the host (ie, the x86 or POWER). Some registers in that
bridge can define a window remapping all or portion of the LPC MEM/FW
space to a portion of the BMC internal bus, with no specific limits
imposed in HW.

As you can see, this can be pretty nasty. So for this, I think it makes
sense to ensure that this window is configured by a kernel driver that
can apply some serious sanity checks on what it is configured to map.

In practice, user space wants to control this by flipping the mapping
between essentially two types of portions of the BMC address space:

   - The flash space. This is a region of the BMC MMIO space that
more/less directly maps the system flash (at least for reads, writes
are somewhat more complicated).

   - One (or more) reserved area(s) of the BMC physical memory.

The latter is needed for a number of things, such as avoiding letting
the host manipulate the innards of the BMC flash controller via some
evil backdoor, we want to do flash updates by routing the window to a
portion of memory (under control of a mailbox protocol via some
separate set of registers) which the host can use to write new data in
bulk and then request the BMC to flash it. There are other uses, such
as allowing the host to boot from an in-memory flash image rather than
the one in flash (very handy for continuous integration and test, the
BMC can just download new images), etc...

So I think the best approach here is:

	- A pair of ioctls to read and write random registers in the
LPC bridge for all the "generally configuration gunk". These have a
filter to ensure that the registers controlling the above mapping
cannot be accessed that way.

	- An ioctl to control the above mapping window. It takes as
arguments the location in LPC space, the window type (flash vs.
memory), for memory, maybe an ID (several windows to chose from), and
the offset& size in the latter. The driver can enforce that the windows
are one of the specially reserved areas of memory etc...

	- An mmap function to map those reserved windows into userspace
so the daemon can communicate appropriately (only needed for the memory
windows, the flash space is accessed via the normal /dev/mtd drivers)

Greg, does that make sense ?

Cheers,
Ben.

Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver

[Greg KH]

On Thu, Jan 12, 2017 at 10:29:37AM -0600, Benjamin Herrenschmidt wrote:
> So I think the best approach here is:
> 
> 	- A pair of ioctls to read and write random registers in the
> LPC bridge for all the "generally configuration gunk". These have a
> filter to ensure that the registers controlling the above mapping
> cannot be accessed that way.
> 
> 	- An ioctl to control the above mapping window. It takes as
> arguments the location in LPC space, the window type (flash vs.
> memory), for memory, maybe an ID (several windows to chose from), and
> the offset& size in the latter. The driver can enforce that the windows
> are one of the specially reserved areas of memory etc...
> 
> 	- An mmap function to map those reserved windows into userspace
> so the daemon can communicate appropriately (only needed for the memory
> windows, the flash space is accessed via the normal /dev/mtd drivers)
> 
> Greg, does that make sense ?

Yes, that makes a lot more sense to me.  Thanks for writing it up,
hopefully it survives into the next driver submission, otherwise I'll
ask the same questions again due to not having a short-term memory at
all :)

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver

[Greg KH]

On Thu, Jan 12, 2017 at 10:29:37AM -0600, Benjamin Herrenschmidt wrote:
> So I think the best approach here is:
> 
> 	- A pair of ioctls to read and write random registers in the
> LPC bridge for all the "generally configuration gunk". These have a
> filter to ensure that the registers controlling the above mapping
> cannot be accessed that way.
> 
> 	- An ioctl to control the above mapping window. It takes as
> arguments the location in LPC space, the window type (flash vs.
> memory), for memory, maybe an ID (several windows to chose from), and
> the offset& size in the latter. The driver can enforce that the windows
> are one of the specially reserved areas of memory etc...
> 
> 	- An mmap function to map those reserved windows into userspace
> so the daemon can communicate appropriately (only needed for the memory
> windows, the flash space is accessed via the normal /dev/mtd drivers)
> 
> Greg, does that make sense ?

Yes, that makes a lot more sense to me.  Thanks for writing it up,
hopefully it survives into the next driver submission, otherwise I'll
ask the same questions again due to not having a short-term memory at
all :)

thanks,

greg k-h

[PATCH 4/4] drivers/mailbox: Add ASpeed mailbox driver

[Cyril Bur]

This provides access to the mbox registers on the ast2400 and ast2500
boards.

This driver allows arbitrary reads and writes to the 16 data registers as
the other end may have configured the mbox hardware to provide an
interrupt when a specific register gets written to.

Signed-off-by: Cyril Bur <cyrilbur-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/mailbox/Kconfig       |   9 ++
 drivers/mailbox/Makefile      |   2 +
 drivers/mailbox/aspeed-mbox.c | 334 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 345 insertions(+)
 create mode 100644 drivers/mailbox/aspeed-mbox.c

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index ceff415f201c..10a7f3f2765c 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -152,4 +152,13 @@ config BCM_PDC_MBOX
 	  Mailbox implementation for the Broadcom PDC ring manager,
 	  which provides access to various offload engines on Broadcom
 	  SoCs. Say Y here if you want to use the Broadcom PDC.
+
+config ASPEED_MBOX
+	depends on (ARCH_ASPEED || COMPILE_TEST) && REGMAP && MFD_SYSCON
+	bool "Aspeed ast2400/2500 Mailbox Controller"
+	default "y"
+	---help---
+	  Provides a driver for the MBOX registers found on Aspeed SOCs
+	  (AST2400 and AST2500). This driver provides a device for aspeed
+	  mbox registers
 endif
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 7dde4f609ae8..db5b4f3f29e0 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -31,3 +31,5 @@ obj-$(CONFIG_HI6220_MBOX)	+= hi6220-mailbox.o
 obj-$(CONFIG_BCM_PDC_MBOX)	+= bcm-pdc-mailbox.o
 
 obj-$(CONFIG_TEGRA_HSP_MBOX)	+= tegra-hsp.o
+
+obj-$(CONFIG_ASPEED_MBOX)	+= aspeed-mbox.o
diff --git a/drivers/mailbox/aspeed-mbox.c b/drivers/mailbox/aspeed-mbox.c
new file mode 100644
index 000000000000..c4ee6ba228ea
--- /dev/null
+++ b/drivers/mailbox/aspeed-mbox.c
@@ -0,0 +1,334 @@
+/*
+ * Copyright 2016 IBM Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/mfd/syscon.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/poll.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#define DEVICE_NAME	"aspeed-mbox"
+
+#define ASPEED_MBOX_NUM_REGS 16
+
+#define ASPEED_MBOX_DATA_0 0x00
+#define ASPEED_MBOX_STATUS_0 0x40
+#define ASPEED_MBOX_STATUS_1 0x44
+#define ASPEED_MBOX_BMC_CTRL 0x48
+#define   ASPEED_MBOX_CTRL_RECV BIT(7)
+#define   ASPEED_MBOX_CTRL_MASK BIT(1)
+#define   ASPEED_MBOX_CTRL_SEND BIT(0)
+#define ASPEED_MBOX_HOST_CTRL 0x4c
+#define ASPEED_MBOX_INTERRUPT_0 0x50
+#define ASPEED_MBOX_INTERRUPT_1 0x54
+
+struct aspeed_mbox {
+	struct miscdevice	miscdev;
+	struct regmap		*regmap;
+	unsigned int		base;
+	wait_queue_head_t	queue;
+	struct mutex		mutex;
+};
+
+static atomic_t aspeed_mbox_open_count = ATOMIC_INIT(0);
+
+static u8 aspeed_mbox_inb(struct aspeed_mbox *mbox, int reg)
+{
+	/*
+	 * The mbox registers are actually only one byte but are addressed
+	 * four bytes apart. The other three bytes are marked 'reserved',
+	 * they *should* be zero but lets not rely on it.
+	 * I am going to rely on the fact we can casually read/write to them...
+	 */
+	unsigned int val = 0xff; /* If regmap throws an error return 0xff */
+	int rc = regmap_read(mbox->regmap, mbox->base + reg, &val);
+
+	if (rc)
+		dev_err(mbox->miscdev.parent, "regmap_read() failed with "
+				"%d (reg: 0x%08x)\n", rc, reg);
+
+	return val & 0xff;
+}
+
+static void aspeed_mbox_outb(struct aspeed_mbox *mbox, u8 data, int reg)
+{
+	int rc = regmap_write(mbox->regmap, mbox->base + reg, data);
+
+	if (rc)
+		dev_err(mbox->miscdev.parent, "regmap_write() failed with "
+				"%d (data: %u reg: 0x%08x)\n", rc, data, reg);
+}
+
+static struct aspeed_mbox *file_mbox(struct file *file)
+{
+	return container_of(file->private_data, struct aspeed_mbox, miscdev);
+}
+
+static int aspeed_mbox_open(struct inode *inode, struct file *file)
+{
+	struct aspeed_mbox *mbox = file_mbox(file);
+
+	if (atomic_inc_return(&aspeed_mbox_open_count) == 1) {
+		/*
+		 * Clear the interrupt status bit if it was left on and unmask
+		 * interrupts.
+		 * ASPEED_MBOX_CTRL_RECV bit is W1C, this also unmasks in 1 step
+		 */
+		aspeed_mbox_outb(mbox, ASPEED_MBOX_CTRL_RECV, ASPEED_MBOX_BMC_CTRL);
+		return 0;
+	}
+
+	atomic_dec(&aspeed_mbox_open_count);
+	return -EBUSY;
+}
+
+static ssize_t aspeed_mbox_read(struct file *file, char __user *buf,
+				size_t count, loff_t *ppos)
+{
+	struct aspeed_mbox *mbox = file_mbox(file);
+	char __user *p = buf;
+	ssize_t ret;
+	int i;
+
+	if (!access_ok(VERIFY_WRITE, buf, count))
+		return -EFAULT;
+
+	if (count + *ppos > ASPEED_MBOX_NUM_REGS)
+		return -EINVAL;
+
+	if (file->f_flags & O_NONBLOCK) {
+		if (!(aspeed_mbox_inb(mbox, ASPEED_MBOX_BMC_CTRL) &
+				ASPEED_MBOX_CTRL_RECV))
+			return -EAGAIN;
+	} else if (wait_event_interruptible(mbox->queue,
+				aspeed_mbox_inb(mbox, ASPEED_MBOX_BMC_CTRL) &
+				ASPEED_MBOX_CTRL_RECV)) {
+		return -ERESTARTSYS;
+	}
+
+	mutex_lock(&mbox->mutex);
+
+	for (i = *ppos; count > 0 && i < ASPEED_MBOX_NUM_REGS; i++) {
+		uint8_t reg = aspeed_mbox_inb(mbox, ASPEED_MBOX_DATA_0 + (i * 4));
+
+		ret = __put_user(reg, p);
+		if (ret)
+			goto out_unlock;
+
+		p++;
+		count--;
+	}
+
+	/* ASPEED_MBOX_CTRL_RECV bit is W1C, this also unmasks in 1 step */
+	aspeed_mbox_outb(mbox, ASPEED_MBOX_CTRL_RECV, ASPEED_MBOX_BMC_CTRL);
+	ret = p - buf;
+
+out_unlock:
+	mutex_unlock(&mbox->mutex);
+	return ret;
+}
+
+static ssize_t aspeed_mbox_write(struct file *file, const char __user *buf,
+				size_t count, loff_t *ppos)
+{
+	struct aspeed_mbox *mbox = file_mbox(file);
+	const char __user *p = buf;
+	ssize_t ret;
+	char c;
+	int i;
+
+	if (!access_ok(VERIFY_READ, buf, count))
+		return -EFAULT;
+
+	if (count + *ppos > ASPEED_MBOX_NUM_REGS)
+		return -EINVAL;
+
+	mutex_lock(&mbox->mutex);
+
+	for (i = *ppos; count > 0 && i < ASPEED_MBOX_NUM_REGS; i++) {
+		ret = __get_user(c, p);
+		if (ret)
+			goto out_unlock;
+
+		aspeed_mbox_outb(mbox, c, ASPEED_MBOX_DATA_0 + (i * 4));
+		p++;
+		count--;
+	}
+
+	aspeed_mbox_outb(mbox, ASPEED_MBOX_CTRL_SEND, ASPEED_MBOX_BMC_CTRL);
+	ret = p - buf;
+
+out_unlock:
+	mutex_unlock(&mbox->mutex);
+	return ret;
+}
+
+static unsigned int aspeed_mbox_poll(struct file *file, poll_table *wait)
+{
+	struct aspeed_mbox *mbox = file_mbox(file);
+	unsigned int mask = 0;
+
+	poll_wait(file, &mbox->queue, wait);
+
+	if (aspeed_mbox_inb(mbox, ASPEED_MBOX_BMC_CTRL) & ASPEED_MBOX_CTRL_RECV)
+		mask |= POLLIN;
+
+	return mask;
+}
+
+static int aspeed_mbox_release(struct inode *inode, struct file *file)
+{
+	atomic_dec(&aspeed_mbox_open_count);
+	return 0;
+}
+
+static const struct file_operations aspeed_mbox_fops = {
+	.owner		= THIS_MODULE,
+	.llseek		= no_seek_end_llseek,
+	.read		= aspeed_mbox_read,
+	.write		= aspeed_mbox_write,
+	.open		= aspeed_mbox_open,
+	.release	= aspeed_mbox_release,
+	.poll		= aspeed_mbox_poll,
+};
+
+static irqreturn_t aspeed_mbox_irq(int irq, void *arg)
+{
+	struct aspeed_mbox *mbox = arg;
+
+	if (!(aspeed_mbox_inb(mbox, ASPEED_MBOX_BMC_CTRL) & ASPEED_MBOX_CTRL_RECV))
+		return IRQ_NONE;
+
+	/*
+	 * Leave the status bit set so that we know the data is for us,
+	 * clear it once it has been read.
+	 */
+
+	/* Mask it off, we'll clear it when we the data gets read */
+	aspeed_mbox_outb(mbox, ASPEED_MBOX_CTRL_MASK, ASPEED_MBOX_BMC_CTRL);
+
+	wake_up(&mbox->queue);
+	return IRQ_HANDLED;
+}
+
+static int aspeed_mbox_config_irq(struct aspeed_mbox *mbox,
+		struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	int rc, irq;
+
+	irq = irq_of_parse_and_map(dev->of_node, 0);
+	if (!irq)
+		return -ENODEV;
+
+	rc = devm_request_irq(dev, irq, aspeed_mbox_irq,
+			IRQF_SHARED, DEVICE_NAME, mbox);
+	if (rc < 0) {
+		dev_err(dev, "Unable to request IRQ %d\n", irq);
+		return rc;
+	}
+
+	/*
+	 * Disable all register based interrupts.
+	 */
+	aspeed_mbox_outb(mbox, 0x00, ASPEED_MBOX_INTERRUPT_0); /* regs 0 - 7 */
+	aspeed_mbox_outb(mbox, 0x00, ASPEED_MBOX_INTERRUPT_1); /* regs 8 - 15 */
+
+	/* These registers are write one to clear. Clear them. */
+	aspeed_mbox_outb(mbox, 0xff, ASPEED_MBOX_STATUS_0);
+	aspeed_mbox_outb(mbox, 0xff, ASPEED_MBOX_STATUS_1);
+
+	aspeed_mbox_outb(mbox, ASPEED_MBOX_CTRL_RECV, ASPEED_MBOX_BMC_CTRL);
+	return 0;
+}
+
+static int aspeed_mbox_probe(struct platform_device *pdev)
+{
+	struct aspeed_mbox *mbox;
+	struct device *dev;
+	int rc;
+
+	dev = &pdev->dev;
+
+	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
+	if (!mbox)
+		return -ENOMEM;
+
+	dev_set_drvdata(&pdev->dev, mbox);
+
+	rc = of_property_read_u32(dev->of_node, "reg", &mbox->base);
+	if (rc) {
+		dev_err(dev, "Couldn't read reg device-tree property\n");
+		return rc;
+	}
+
+	mbox->regmap = syscon_node_to_regmap(
+			pdev->dev.parent->of_node);
+	if (IS_ERR(mbox->regmap)) {
+		dev_err(dev, "Couldn't get regmap\n");
+		return -ENODEV;
+	}
+
+	mutex_init(&mbox->mutex);
+	init_waitqueue_head(&mbox->queue);
+
+	mbox->miscdev.minor = MISC_DYNAMIC_MINOR;
+	mbox->miscdev.name = DEVICE_NAME;
+	mbox->miscdev.fops = &aspeed_mbox_fops;
+	mbox->miscdev.parent = dev;
+	rc = misc_register(&mbox->miscdev);
+	if (rc) {
+		dev_err(dev, "Unable to register device\n");
+		return rc;
+	}
+
+	rc = aspeed_mbox_config_irq(mbox, pdev);
+	if (rc) {
+		dev_err(dev, "Failed to configure IRQ\n");
+		misc_deregister(&mbox->miscdev);
+		return rc;
+	}
+
+	return 0;
+}
+
+static int aspeed_mbox_remove(struct platform_device *pdev)
+{
+	struct aspeed_mbox *mbox = dev_get_drvdata(&pdev->dev);
+
+	misc_deregister(&mbox->miscdev);
+
+	return 0;
+}
+
+static const struct of_device_id aspeed_mbox_match[] = {
+	{ .compatible = "aspeed,ast2400-mbox" },
+	{ .compatible = "aspeed,ast2500-mbox" },
+	{ },
+};
+
+static struct platform_driver aspeed_mbox_driver = {
+	.driver = {
+		.name		= DEVICE_NAME,
+		.of_match_table = aspeed_mbox_match,
+	},
+	.probe = aspeed_mbox_probe,
+	.remove = aspeed_mbox_remove,
+};
+
+module_platform_driver(aspeed_mbox_driver);
+
+MODULE_DEVICE_TABLE(of, aspeed_mbox_match);
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Cyril Bur <cyrilbur-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>");
+MODULE_DESCRIPTION("ASpeed mailbox device driver");
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/4] drivers/mailbox: Add ASpeed mailbox driver

[Cyril Bur]

This provides access to the mbox registers on the ast2400 and ast2500
boards.

This driver allows arbitrary reads and writes to the 16 data registers as
the other end may have configured the mbox hardware to provide an
interrupt when a specific register gets written to.

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
 drivers/mailbox/Kconfig       |   9 ++
 drivers/mailbox/Makefile      |   2 +
 drivers/mailbox/aspeed-mbox.c | 334 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 345 insertions(+)
 create mode 100644 drivers/mailbox/aspeed-mbox.c

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index ceff415f201c..10a7f3f2765c 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -152,4 +152,13 @@ config BCM_PDC_MBOX
 	  Mailbox implementation for the Broadcom PDC ring manager,
 	  which provides access to various offload engines on Broadcom
 	  SoCs. Say Y here if you want to use the Broadcom PDC.
+
+config ASPEED_MBOX
+	depends on (ARCH_ASPEED || COMPILE_TEST) && REGMAP && MFD_SYSCON
+	bool "Aspeed ast2400/2500 Mailbox Controller"
+	default "y"
+	---help---
+	  Provides a driver for the MBOX registers found on Aspeed SOCs
+	  (AST2400 and AST2500). This driver provides a device for aspeed
+	  mbox registers
 endif
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 7dde4f609ae8..db5b4f3f29e0 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -31,3 +31,5 @@ obj-$(CONFIG_HI6220_MBOX)	+= hi6220-mailbox.o
 obj-$(CONFIG_BCM_PDC_MBOX)	+= bcm-pdc-mailbox.o
 
 obj-$(CONFIG_TEGRA_HSP_MBOX)	+= tegra-hsp.o
+
+obj-$(CONFIG_ASPEED_MBOX)	+= aspeed-mbox.o
diff --git a/drivers/mailbox/aspeed-mbox.c b/drivers/mailbox/aspeed-mbox.c
new file mode 100644
index 000000000000..c4ee6ba228ea
--- /dev/null
+++ b/drivers/mailbox/aspeed-mbox.c
@@ -0,0 +1,334 @@
+/*
+ * Copyright 2016 IBM Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/mfd/syscon.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/poll.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#define DEVICE_NAME	"aspeed-mbox"
+
+#define ASPEED_MBOX_NUM_REGS 16
+
+#define ASPEED_MBOX_DATA_0 0x00
+#define ASPEED_MBOX_STATUS_0 0x40
+#define ASPEED_MBOX_STATUS_1 0x44
+#define ASPEED_MBOX_BMC_CTRL 0x48
+#define   ASPEED_MBOX_CTRL_RECV BIT(7)
+#define   ASPEED_MBOX_CTRL_MASK BIT(1)
+#define   ASPEED_MBOX_CTRL_SEND BIT(0)
+#define ASPEED_MBOX_HOST_CTRL 0x4c
+#define ASPEED_MBOX_INTERRUPT_0 0x50
+#define ASPEED_MBOX_INTERRUPT_1 0x54
+
+struct aspeed_mbox {
+	struct miscdevice	miscdev;
+	struct regmap		*regmap;
+	unsigned int		base;
+	wait_queue_head_t	queue;
+	struct mutex		mutex;
+};
+
+static atomic_t aspeed_mbox_open_count = ATOMIC_INIT(0);
+
+static u8 aspeed_mbox_inb(struct aspeed_mbox *mbox, int reg)
+{
+	/*
+	 * The mbox registers are actually only one byte but are addressed
+	 * four bytes apart. The other three bytes are marked 'reserved',
+	 * they *should* be zero but lets not rely on it.
+	 * I am going to rely on the fact we can casually read/write to them...
+	 */
+	unsigned int val = 0xff; /* If regmap throws an error return 0xff */
+	int rc = regmap_read(mbox->regmap, mbox->base + reg, &val);
+
+	if (rc)
+		dev_err(mbox->miscdev.parent, "regmap_read() failed with "
+				"%d (reg: 0x%08x)\n", rc, reg);
+
+	return val & 0xff;
+}
+
+static void aspeed_mbox_outb(struct aspeed_mbox *mbox, u8 data, int reg)
+{
+	int rc = regmap_write(mbox->regmap, mbox->base + reg, data);
+
+	if (rc)
+		dev_err(mbox->miscdev.parent, "regmap_write() failed with "
+				"%d (data: %u reg: 0x%08x)\n", rc, data, reg);
+}
+
+static struct aspeed_mbox *file_mbox(struct file *file)
+{
+	return container_of(file->private_data, struct aspeed_mbox, miscdev);
+}
+
+static int aspeed_mbox_open(struct inode *inode, struct file *file)
+{
+	struct aspeed_mbox *mbox = file_mbox(file);
+
+	if (atomic_inc_return(&aspeed_mbox_open_count) == 1) {
+		/*
+		 * Clear the interrupt status bit if it was left on and unmask
+		 * interrupts.
+		 * ASPEED_MBOX_CTRL_RECV bit is W1C, this also unmasks in 1 step
+		 */
+		aspeed_mbox_outb(mbox, ASPEED_MBOX_CTRL_RECV, ASPEED_MBOX_BMC_CTRL);
+		return 0;
+	}
+
+	atomic_dec(&aspeed_mbox_open_count);
+	return -EBUSY;
+}
+
+static ssize_t aspeed_mbox_read(struct file *file, char __user *buf,
+				size_t count, loff_t *ppos)
+{
+	struct aspeed_mbox *mbox = file_mbox(file);
+	char __user *p = buf;
+	ssize_t ret;
+	int i;
+
+	if (!access_ok(VERIFY_WRITE, buf, count))
+		return -EFAULT;
+
+	if (count + *ppos > ASPEED_MBOX_NUM_REGS)
+		return -EINVAL;
+
+	if (file->f_flags & O_NONBLOCK) {
+		if (!(aspeed_mbox_inb(mbox, ASPEED_MBOX_BMC_CTRL) &
+				ASPEED_MBOX_CTRL_RECV))
+			return -EAGAIN;
+	} else if (wait_event_interruptible(mbox->queue,
+				aspeed_mbox_inb(mbox, ASPEED_MBOX_BMC_CTRL) &
+				ASPEED_MBOX_CTRL_RECV)) {
+		return -ERESTARTSYS;
+	}
+
+	mutex_lock(&mbox->mutex);
+
+	for (i = *ppos; count > 0 && i < ASPEED_MBOX_NUM_REGS; i++) {
+		uint8_t reg = aspeed_mbox_inb(mbox, ASPEED_MBOX_DATA_0 + (i * 4));
+
+		ret = __put_user(reg, p);
+		if (ret)
+			goto out_unlock;
+
+		p++;
+		count--;
+	}
+
+	/* ASPEED_MBOX_CTRL_RECV bit is W1C, this also unmasks in 1 step */
+	aspeed_mbox_outb(mbox, ASPEED_MBOX_CTRL_RECV, ASPEED_MBOX_BMC_CTRL);
+	ret = p - buf;
+
+out_unlock:
+	mutex_unlock(&mbox->mutex);
+	return ret;
+}
+
+static ssize_t aspeed_mbox_write(struct file *file, const char __user *buf,
+				size_t count, loff_t *ppos)
+{
+	struct aspeed_mbox *mbox = file_mbox(file);
+	const char __user *p = buf;
+	ssize_t ret;
+	char c;
+	int i;
+
+	if (!access_ok(VERIFY_READ, buf, count))
+		return -EFAULT;
+
+	if (count + *ppos > ASPEED_MBOX_NUM_REGS)
+		return -EINVAL;
+
+	mutex_lock(&mbox->mutex);
+
+	for (i = *ppos; count > 0 && i < ASPEED_MBOX_NUM_REGS; i++) {
+		ret = __get_user(c, p);
+		if (ret)
+			goto out_unlock;
+
+		aspeed_mbox_outb(mbox, c, ASPEED_MBOX_DATA_0 + (i * 4));
+		p++;
+		count--;
+	}
+
+	aspeed_mbox_outb(mbox, ASPEED_MBOX_CTRL_SEND, ASPEED_MBOX_BMC_CTRL);
+	ret = p - buf;
+
+out_unlock:
+	mutex_unlock(&mbox->mutex);
+	return ret;
+}
+
+static unsigned int aspeed_mbox_poll(struct file *file, poll_table *wait)
+{
+	struct aspeed_mbox *mbox = file_mbox(file);
+	unsigned int mask = 0;
+
+	poll_wait(file, &mbox->queue, wait);
+
+	if (aspeed_mbox_inb(mbox, ASPEED_MBOX_BMC_CTRL) & ASPEED_MBOX_CTRL_RECV)
+		mask |= POLLIN;
+
+	return mask;
+}
+
+static int aspeed_mbox_release(struct inode *inode, struct file *file)
+{
+	atomic_dec(&aspeed_mbox_open_count);
+	return 0;
+}
+
+static const struct file_operations aspeed_mbox_fops = {
+	.owner		= THIS_MODULE,
+	.llseek		= no_seek_end_llseek,
+	.read		= aspeed_mbox_read,
+	.write		= aspeed_mbox_write,
+	.open		= aspeed_mbox_open,
+	.release	= aspeed_mbox_release,
+	.poll		= aspeed_mbox_poll,
+};
+
+static irqreturn_t aspeed_mbox_irq(int irq, void *arg)
+{
+	struct aspeed_mbox *mbox = arg;
+
+	if (!(aspeed_mbox_inb(mbox, ASPEED_MBOX_BMC_CTRL) & ASPEED_MBOX_CTRL_RECV))
+		return IRQ_NONE;
+
+	/*
+	 * Leave the status bit set so that we know the data is for us,
+	 * clear it once it has been read.
+	 */
+
+	/* Mask it off, we'll clear it when we the data gets read */
+	aspeed_mbox_outb(mbox, ASPEED_MBOX_CTRL_MASK, ASPEED_MBOX_BMC_CTRL);
+
+	wake_up(&mbox->queue);
+	return IRQ_HANDLED;
+}
+
+static int aspeed_mbox_config_irq(struct aspeed_mbox *mbox,
+		struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	int rc, irq;
+
+	irq = irq_of_parse_and_map(dev->of_node, 0);
+	if (!irq)
+		return -ENODEV;
+
+	rc = devm_request_irq(dev, irq, aspeed_mbox_irq,
+			IRQF_SHARED, DEVICE_NAME, mbox);
+	if (rc < 0) {
+		dev_err(dev, "Unable to request IRQ %d\n", irq);
+		return rc;
+	}
+
+	/*
+	 * Disable all register based interrupts.
+	 */
+	aspeed_mbox_outb(mbox, 0x00, ASPEED_MBOX_INTERRUPT_0); /* regs 0 - 7 */
+	aspeed_mbox_outb(mbox, 0x00, ASPEED_MBOX_INTERRUPT_1); /* regs 8 - 15 */
+
+	/* These registers are write one to clear. Clear them. */
+	aspeed_mbox_outb(mbox, 0xff, ASPEED_MBOX_STATUS_0);
+	aspeed_mbox_outb(mbox, 0xff, ASPEED_MBOX_STATUS_1);
+
+	aspeed_mbox_outb(mbox, ASPEED_MBOX_CTRL_RECV, ASPEED_MBOX_BMC_CTRL);
+	return 0;
+}
+
+static int aspeed_mbox_probe(struct platform_device *pdev)
+{
+	struct aspeed_mbox *mbox;
+	struct device *dev;
+	int rc;
+
+	dev = &pdev->dev;
+
+	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
+	if (!mbox)
+		return -ENOMEM;
+
+	dev_set_drvdata(&pdev->dev, mbox);
+
+	rc = of_property_read_u32(dev->of_node, "reg", &mbox->base);
+	if (rc) {
+		dev_err(dev, "Couldn't read reg device-tree property\n");
+		return rc;
+	}
+
+	mbox->regmap = syscon_node_to_regmap(
+			pdev->dev.parent->of_node);
+	if (IS_ERR(mbox->regmap)) {
+		dev_err(dev, "Couldn't get regmap\n");
+		return -ENODEV;
+	}
+
+	mutex_init(&mbox->mutex);
+	init_waitqueue_head(&mbox->queue);
+
+	mbox->miscdev.minor = MISC_DYNAMIC_MINOR;
+	mbox->miscdev.name = DEVICE_NAME;
+	mbox->miscdev.fops = &aspeed_mbox_fops;
+	mbox->miscdev.parent = dev;
+	rc = misc_register(&mbox->miscdev);
+	if (rc) {
+		dev_err(dev, "Unable to register device\n");
+		return rc;
+	}
+
+	rc = aspeed_mbox_config_irq(mbox, pdev);
+	if (rc) {
+		dev_err(dev, "Failed to configure IRQ\n");
+		misc_deregister(&mbox->miscdev);
+		return rc;
+	}
+
+	return 0;
+}
+
+static int aspeed_mbox_remove(struct platform_device *pdev)
+{
+	struct aspeed_mbox *mbox = dev_get_drvdata(&pdev->dev);
+
+	misc_deregister(&mbox->miscdev);
+
+	return 0;
+}
+
+static const struct of_device_id aspeed_mbox_match[] = {
+	{ .compatible = "aspeed,ast2400-mbox" },
+	{ .compatible = "aspeed,ast2500-mbox" },
+	{ },
+};
+
+static struct platform_driver aspeed_mbox_driver = {
+	.driver = {
+		.name		= DEVICE_NAME,
+		.of_match_table = aspeed_mbox_match,
+	},
+	.probe = aspeed_mbox_probe,
+	.remove = aspeed_mbox_remove,
+};
+
+module_platform_driver(aspeed_mbox_driver);
+
+MODULE_DEVICE_TABLE(of, aspeed_mbox_match);
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Cyril Bur <cyrilbur@gmail.com>");
+MODULE_DESCRIPTION("ASpeed mailbox device driver");
-- 
2.11.0

Re: [PATCH 4/4] drivers/mailbox: Add ASpeed mailbox driver

[Joel Stanley]

On Thu, Jan 12, 2017 at 10:59 AM, Cyril Bur <cyrilbur-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> This provides access to the mbox registers on the ast2400 and ast2500
> boards.

s/boards/SoCs/

>
> This driver allows arbitrary reads and writes to the 16 data registers as
> the other end may have configured the mbox hardware to provide an
> interrupt when a specific register gets written to.
>
> Signed-off-by: Cyril Bur <cyrilbur-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Send this to lkml as well next time you submit.

> ---
>  drivers/mailbox/Kconfig       |   9 ++
>  drivers/mailbox/Makefile      |   2 +
>  drivers/mailbox/aspeed-mbox.c | 334 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 345 insertions(+)
>  create mode 100644 drivers/mailbox/aspeed-mbox.c
>
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index ceff415f201c..10a7f3f2765c 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -152,4 +152,13 @@ config BCM_PDC_MBOX
>           Mailbox implementation for the Broadcom PDC ring manager,
>           which provides access to various offload engines on Broadcom
>           SoCs. Say Y here if you want to use the Broadcom PDC.
> +
> +config ASPEED_MBOX

Call this ASPEED_LPC_MAILBOX, as it's a in the LPC IP block.

> +       depends on (ARCH_ASPEED || COMPILE_TEST) && REGMAP && MFD_SYSCON
> +       bool "Aspeed ast2400/2500 Mailbox Controller"

Call this Aspeed LPC Mailbox Controller, as the layout is shared by
SoCs other than the 2500 and 2400.

> +       default "y"
> +       ---help---
> +         Provides a driver for the MBOX registers found on Aspeed SOCs
> +         (AST2400 and AST2500). This driver provides a device for aspeed
> +         mbox registers
>  endif
> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> index 7dde4f609ae8..db5b4f3f29e0 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -31,3 +31,5 @@ obj-$(CONFIG_HI6220_MBOX)     += hi6220-mailbox.o
>  obj-$(CONFIG_BCM_PDC_MBOX)     += bcm-pdc-mailbox.o
>
>  obj-$(CONFIG_TEGRA_HSP_MBOX)   += tegra-hsp.o
> +
> +obj-$(CONFIG_ASPEED_MBOX)      += aspeed-mbox.o
> diff --git a/drivers/mailbox/aspeed-mbox.c b/drivers/mailbox/aspeed-mbox.c
> new file mode 100644
> index 000000000000..c4ee6ba228ea
> --- /dev/null
> +++ b/drivers/mailbox/aspeed-mbox.c

> +
> +static ssize_t aspeed_mbox_read(struct file *file, char __user *buf,
> +                               size_t count, loff_t *ppos)
> +{
> +       struct aspeed_mbox *mbox = file_mbox(file);
> +       char __user *p = buf;
> +       ssize_t ret;
> +       int i;
> +
> +       if (!access_ok(VERIFY_WRITE, buf, count))
> +               return -EFAULT;

As discussed in the LPC driver review:

 "And don't call access_ok(), it's racy and no driver should ever do that."

Make sure all of the things you fixed in that driver are fixed in this one.

> +
> +       if (count + *ppos > ASPEED_MBOX_NUM_REGS)
> +               return -EINVAL;
> +
> +       if (file->f_flags & O_NONBLOCK) {
> +               if (!(aspeed_mbox_inb(mbox, ASPEED_MBOX_BMC_CTRL) &
> +                               ASPEED_MBOX_CTRL_RECV))
> +                       return -EAGAIN;
> +       } else if (wait_event_interruptible(mbox->queue,
> +                               aspeed_mbox_inb(mbox, ASPEED_MBOX_BMC_CTRL) &
> +                               ASPEED_MBOX_CTRL_RECV)) {
> +               return -ERESTARTSYS;
> +       }
> +
> +       mutex_lock(&mbox->mutex);
> +
> +       for (i = *ppos; count > 0 && i < ASPEED_MBOX_NUM_REGS; i++) {
> +               uint8_t reg = aspeed_mbox_inb(mbox, ASPEED_MBOX_DATA_0 + (i * 4));
> +
> +               ret = __put_user(reg, p);
> +               if (ret)
> +                       goto out_unlock;
> +
> +               p++;
> +               count--;
> +       }
> +
> +       /* ASPEED_MBOX_CTRL_RECV bit is W1C, this also unmasks in 1 step */

W1C? Write to clear?

> +       aspeed_mbox_outb(mbox, ASPEED_MBOX_CTRL_RECV, ASPEED_MBOX_BMC_CTRL);
> +       ret = p - buf;
> +
> +out_unlock:
> +       mutex_unlock(&mbox->mutex);
> +       return ret;
> +}
> +

> +module_platform_driver(aspeed_mbox_driver);
> +
> +MODULE_DEVICE_TABLE(of, aspeed_mbox_match);
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Cyril Bur <cyrilbur-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>");
> +MODULE_DESCRIPTION("ASpeed mailbox device driver");

ASPEED or Aspeed.

> --
> 2.11.0
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] drivers/mailbox: Add ASpeed mailbox driver

[Joel Stanley]

On Thu, Jan 12, 2017 at 10:59 AM, Cyril Bur <cyrilbur@gmail.com> wrote:
> This provides access to the mbox registers on the ast2400 and ast2500
> boards.

s/boards/SoCs/

>
> This driver allows arbitrary reads and writes to the 16 data registers as
> the other end may have configured the mbox hardware to provide an
> interrupt when a specific register gets written to.
>
> Signed-off-by: Cyril Bur <cyrilbur@gmail.com>

Send this to lkml as well next time you submit.

> ---
>  drivers/mailbox/Kconfig       |   9 ++
>  drivers/mailbox/Makefile      |   2 +
>  drivers/mailbox/aspeed-mbox.c | 334 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 345 insertions(+)
>  create mode 100644 drivers/mailbox/aspeed-mbox.c
>
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index ceff415f201c..10a7f3f2765c 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -152,4 +152,13 @@ config BCM_PDC_MBOX
>           Mailbox implementation for the Broadcom PDC ring manager,
>           which provides access to various offload engines on Broadcom
>           SoCs. Say Y here if you want to use the Broadcom PDC.
> +
> +config ASPEED_MBOX

Call this ASPEED_LPC_MAILBOX, as it's a in the LPC IP block.

> +       depends on (ARCH_ASPEED || COMPILE_TEST) && REGMAP && MFD_SYSCON
> +       bool "Aspeed ast2400/2500 Mailbox Controller"

Call this Aspeed LPC Mailbox Controller, as the layout is shared by
SoCs other than the 2500 and 2400.

> +       default "y"
> +       ---help---
> +         Provides a driver for the MBOX registers found on Aspeed SOCs
> +         (AST2400 and AST2500). This driver provides a device for aspeed
> +         mbox registers
>  endif
> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> index 7dde4f609ae8..db5b4f3f29e0 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -31,3 +31,5 @@ obj-$(CONFIG_HI6220_MBOX)     += hi6220-mailbox.o
>  obj-$(CONFIG_BCM_PDC_MBOX)     += bcm-pdc-mailbox.o
>
>  obj-$(CONFIG_TEGRA_HSP_MBOX)   += tegra-hsp.o
> +
> +obj-$(CONFIG_ASPEED_MBOX)      += aspeed-mbox.o
> diff --git a/drivers/mailbox/aspeed-mbox.c b/drivers/mailbox/aspeed-mbox.c
> new file mode 100644
> index 000000000000..c4ee6ba228ea
> --- /dev/null
> +++ b/drivers/mailbox/aspeed-mbox.c

> +
> +static ssize_t aspeed_mbox_read(struct file *file, char __user *buf,
> +                               size_t count, loff_t *ppos)
> +{
> +       struct aspeed_mbox *mbox = file_mbox(file);
> +       char __user *p = buf;
> +       ssize_t ret;
> +       int i;
> +
> +       if (!access_ok(VERIFY_WRITE, buf, count))
> +               return -EFAULT;

As discussed in the LPC driver review:

 "And don't call access_ok(), it's racy and no driver should ever do that."

Make sure all of the things you fixed in that driver are fixed in this one.

> +
> +       if (count + *ppos > ASPEED_MBOX_NUM_REGS)
> +               return -EINVAL;
> +
> +       if (file->f_flags & O_NONBLOCK) {
> +               if (!(aspeed_mbox_inb(mbox, ASPEED_MBOX_BMC_CTRL) &
> +                               ASPEED_MBOX_CTRL_RECV))
> +                       return -EAGAIN;
> +       } else if (wait_event_interruptible(mbox->queue,
> +                               aspeed_mbox_inb(mbox, ASPEED_MBOX_BMC_CTRL) &
> +                               ASPEED_MBOX_CTRL_RECV)) {
> +               return -ERESTARTSYS;
> +       }
> +
> +       mutex_lock(&mbox->mutex);
> +
> +       for (i = *ppos; count > 0 && i < ASPEED_MBOX_NUM_REGS; i++) {
> +               uint8_t reg = aspeed_mbox_inb(mbox, ASPEED_MBOX_DATA_0 + (i * 4));
> +
> +               ret = __put_user(reg, p);
> +               if (ret)
> +                       goto out_unlock;
> +
> +               p++;
> +               count--;
> +       }
> +
> +       /* ASPEED_MBOX_CTRL_RECV bit is W1C, this also unmasks in 1 step */

W1C? Write to clear?

> +       aspeed_mbox_outb(mbox, ASPEED_MBOX_CTRL_RECV, ASPEED_MBOX_BMC_CTRL);
> +       ret = p - buf;
> +
> +out_unlock:
> +       mutex_unlock(&mbox->mutex);
> +       return ret;
> +}
> +

> +module_platform_driver(aspeed_mbox_driver);
> +
> +MODULE_DEVICE_TABLE(of, aspeed_mbox_match);
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Cyril Bur <cyrilbur@gmail.com>");
> +MODULE_DESCRIPTION("ASpeed mailbox device driver");

ASPEED or Aspeed.

> --
> 2.11.0
>

Re: [PATCH 4/4] drivers/mailbox: Add ASpeed mailbox driver

[Benjamin Herrenschmidt]

On Tue, 2017-02-07 at 16:10 +1030, Joel Stanley wrote:

> > +static ssize_t aspeed_mbox_read(struct file *file, char __user *buf,
> > +                               size_t count, loff_t *ppos)
> > +{
> > +       struct aspeed_mbox *mbox = file_mbox(file);
> > +       char __user *p = buf;
> > +       ssize_t ret;
> > +       int i;
> > +
> > +       if (!access_ok(VERIFY_WRITE, buf, count))
> > +               return -EFAULT;
> 
> As discussed in the LPC driver review:
> 
>  "And don't call access_ok(), it's racy and no driver should ever do that."
> 
> Make sure all of the things you fixed in that driver are fixed in this one.

Well... I don't entirely agree with Greg on that one :-) In this specific
case, since he's using __put_user() below, he must use access_ok() first.

The "alternate" option is to bounce everything via a local memory array,
copy_to/from_user (to/from that array) and then do the loop of inb/outb,
but in the grand scheme of things, what is written here is correct *and*
more efficient than the alternate approach. On such a slow SoC, those
cycles do count.

> > +
> > +       if (count + *ppos > ASPEED_MBOX_NUM_REGS)
> > +               return -EINVAL;
> > +
> > +       if (file->f_flags & O_NONBLOCK) {
> > +               if (!(aspeed_mbox_inb(mbox, ASPEED_MBOX_BMC_CTRL) &
> > +                               ASPEED_MBOX_CTRL_RECV))
> > +                       return -EAGAIN;
> > +       } else if (wait_event_interruptible(mbox->queue,
> > +                               aspeed_mbox_inb(mbox, ASPEED_MBOX_BMC_CTRL) &
> > +                               ASPEED_MBOX_CTRL_RECV)) {
> > +               return -ERESTARTSYS;
> > +       }
> > +
> > +       mutex_lock(&mbox->mutex);
> > +
> > +       for (i = *ppos; count > 0 && i < ASPEED_MBOX_NUM_REGS; i++) {
> > +               uint8_t reg = aspeed_mbox_inb(mbox, ASPEED_MBOX_DATA_0 + (i * 4));
> > +
> > +               ret = __put_user(reg, p);
> > +               if (ret)
> > +                       goto out_unlock;
> > +
> > +               p++;
> > +               count--;
> > +       }
> > +
> > +       /* ASPEED_MBOX_CTRL_RECV bit is W1C, this also unmasks in 1 step */
> 
> W1C? Write to clear?
> 
> > +       aspeed_mbox_outb(mbox, ASPEED_MBOX_CTRL_RECV, ASPEED_MBOX_BMC_CTRL);
> > +       ret = p - buf;
> > +
> > +out_unlock:
> > +       mutex_unlock(&mbox->mutex);
> > +       return ret;
> > +}
> > +
> > +module_platform_driver(aspeed_mbox_driver);
> > +
> > +MODULE_DEVICE_TABLE(of, aspeed_mbox_match);
> > +MODULE_LICENSE("GPL");
> > > > +MODULE_AUTHOR("Cyril Bur <cyrilbur-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>");
> > +MODULE_DESCRIPTION("ASpeed mailbox device driver");
> 
> ASPEED or Aspeed.
> 
> > --
> > 2.11.0
> > 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] drivers/mailbox: Add ASpeed mailbox driver

[Benjamin Herrenschmidt]

On Tue, 2017-02-07 at 16:10 +1030, Joel Stanley wrote:

> > +static ssize_t aspeed_mbox_read(struct file *file, char __user *buf,
> > +                               size_t count, loff_t *ppos)
> > +{
> > +       struct aspeed_mbox *mbox = file_mbox(file);
> > +       char __user *p = buf;
> > +       ssize_t ret;
> > +       int i;
> > +
> > +       if (!access_ok(VERIFY_WRITE, buf, count))
> > +               return -EFAULT;
> 
> As discussed in the LPC driver review:
> 
>  "And don't call access_ok(), it's racy and no driver should ever do that."
> 
> Make sure all of the things you fixed in that driver are fixed in this one.

Well... I don't entirely agree with Greg on that one :-) In this specific
case, since he's using __put_user() below, he must use access_ok() first.

The "alternate" option is to bounce everything via a local memory array,
copy_to/from_user (to/from that array) and then do the loop of inb/outb,
but in the grand scheme of things, what is written here is correct *and*
more efficient than the alternate approach. On such a slow SoC, those
cycles do count.

> > +
> > +       if (count + *ppos > ASPEED_MBOX_NUM_REGS)
> > +               return -EINVAL;
> > +
> > +       if (file->f_flags & O_NONBLOCK) {
> > +               if (!(aspeed_mbox_inb(mbox, ASPEED_MBOX_BMC_CTRL) &
> > +                               ASPEED_MBOX_CTRL_RECV))
> > +                       return -EAGAIN;
> > +       } else if (wait_event_interruptible(mbox->queue,
> > +                               aspeed_mbox_inb(mbox, ASPEED_MBOX_BMC_CTRL) &
> > +                               ASPEED_MBOX_CTRL_RECV)) {
> > +               return -ERESTARTSYS;
> > +       }
> > +
> > +       mutex_lock(&mbox->mutex);
> > +
> > +       for (i = *ppos; count > 0 && i < ASPEED_MBOX_NUM_REGS; i++) {
> > +               uint8_t reg = aspeed_mbox_inb(mbox, ASPEED_MBOX_DATA_0 + (i * 4));
> > +
> > +               ret = __put_user(reg, p);
> > +               if (ret)
> > +                       goto out_unlock;
> > +
> > +               p++;
> > +               count--;
> > +       }
> > +
> > +       /* ASPEED_MBOX_CTRL_RECV bit is W1C, this also unmasks in 1 step */
> 
> W1C? Write to clear?
> 
> > +       aspeed_mbox_outb(mbox, ASPEED_MBOX_CTRL_RECV, ASPEED_MBOX_BMC_CTRL);
> > +       ret = p - buf;
> > +
> > +out_unlock:
> > +       mutex_unlock(&mbox->mutex);
> > +       return ret;
> > +}
> > +
> > +module_platform_driver(aspeed_mbox_driver);
> > +
> > +MODULE_DEVICE_TABLE(of, aspeed_mbox_match);
> > +MODULE_LICENSE("GPL");
> > > > +MODULE_AUTHOR("Cyril Bur <cyrilbur@gmail.com>");
> > +MODULE_DESCRIPTION("ASpeed mailbox device driver");
> 
> ASPEED or Aspeed.
> 
> > --
> > 2.11.0
> > 

Re: [PATCH 4/4] drivers/mailbox: Add ASpeed mailbox driver

[Cyril Bur]

On Tue, 2017-02-07 at 16:10 +1030, Joel Stanley wrote:
> On Thu, Jan 12, 2017 at 10:59 AM, Cyril Bur <cyrilbur-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > This provides access to the mbox registers on the ast2400 and ast2500
> > boards.
> 

Hey Joel,

Thanks for review, just one thing, I'll fixup the rest.

Cyril

> s/boards/SoCs/
> 
> > 
> > This driver allows arbitrary reads and writes to the 16 data registers as
> > the other end may have configured the mbox hardware to provide an
> > interrupt when a specific register gets written to.
> > 
> > Signed-off-by: Cyril Bur <cyrilbur-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> Send this to lkml as well next time you submit.
> 
> > ---
> >  drivers/mailbox/Kconfig       |   9 ++
> >  drivers/mailbox/Makefile      |   2 +
> >  drivers/mailbox/aspeed-mbox.c | 334 ++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 345 insertions(+)
> >  create mode 100644 drivers/mailbox/aspeed-mbox.c
> > 
> > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> > index ceff415f201c..10a7f3f2765c 100644
> > --- a/drivers/mailbox/Kconfig
> > +++ b/drivers/mailbox/Kconfig
> > @@ -152,4 +152,13 @@ config BCM_PDC_MBOX
> >           Mailbox implementation for the Broadcom PDC ring manager,
> >           which provides access to various offload engines on Broadcom
> >           SoCs. Say Y here if you want to use the Broadcom PDC.
> > +
> > +config ASPEED_MBOX
> 
> Call this ASPEED_LPC_MAILBOX, as it's a in the LPC IP block.
> 

I think I'll stick with ASPEED_LPC_MBOX, the rest of the file uses MBOX
more widely when talking about controllers.

> > +       depends on (ARCH_ASPEED || COMPILE_TEST) && REGMAP && MFD_SYSCON
> > +       bool "Aspeed ast2400/2500 Mailbox Controller"
> 
> Call this Aspeed LPC Mailbox Controller, as the layout is shared by
> SoCs other than the 2500 and 2400.
> 
> > +       default "y"
> > +       ---help---
> > +         Provides a driver for the MBOX registers found on Aspeed SOCs
> > +         (AST2400 and AST2500). This driver provides a device for aspeed
> > +         mbox registers
> >  endif
> > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> > index 7dde4f609ae8..db5b4f3f29e0 100644
> > --- a/drivers/mailbox/Makefile
> > +++ b/drivers/mailbox/Makefile
> > @@ -31,3 +31,5 @@ obj-$(CONFIG_HI6220_MBOX)     += hi6220-mailbox.o
> >  obj-$(CONFIG_BCM_PDC_MBOX)     += bcm-pdc-mailbox.o
> > 
> >  obj-$(CONFIG_TEGRA_HSP_MBOX)   += tegra-hsp.o
> > +
> > +obj-$(CONFIG_ASPEED_MBOX)      += aspeed-mbox.o
> > diff --git a/drivers/mailbox/aspeed-mbox.c b/drivers/mailbox/aspeed-mbox.c
> > new file mode 100644
> > index 000000000000..c4ee6ba228ea
> > --- /dev/null
> > +++ b/drivers/mailbox/aspeed-mbox.c
> > +
> > +static ssize_t aspeed_mbox_read(struct file *file, char __user *buf,
> > +                               size_t count, loff_t *ppos)
> > +{
> > +       struct aspeed_mbox *mbox = file_mbox(file);
> > +       char __user *p = buf;
> > +       ssize_t ret;
> > +       int i;
> > +
> > +       if (!access_ok(VERIFY_WRITE, buf, count))
> > +               return -EFAULT;
> 
> As discussed in the LPC driver review:
> 
>  "And don't call access_ok(), it's racy and no driver should ever do that."
> 
> Make sure all of the things you fixed in that driver are fixed in this one.
> 
> > +
> > +       if (count + *ppos > ASPEED_MBOX_NUM_REGS)
> > +               return -EINVAL;
> > +
> > +       if (file->f_flags & O_NONBLOCK) {
> > +               if (!(aspeed_mbox_inb(mbox, ASPEED_MBOX_BMC_CTRL) &
> > +                               ASPEED_MBOX_CTRL_RECV))
> > +                       return -EAGAIN;
> > +       } else if (wait_event_interruptible(mbox->queue,
> > +                               aspeed_mbox_inb(mbox, ASPEED_MBOX_BMC_CTRL) &
> > +                               ASPEED_MBOX_CTRL_RECV)) {
> > +               return -ERESTARTSYS;
> > +       }
> > +
> > +       mutex_lock(&mbox->mutex);
> > +
> > +       for (i = *ppos; count > 0 && i < ASPEED_MBOX_NUM_REGS; i++) {
> > +               uint8_t reg = aspeed_mbox_inb(mbox, ASPEED_MBOX_DATA_0 + (i * 4));
> > +
> > +               ret = __put_user(reg, p);
> > +               if (ret)
> > +                       goto out_unlock;
> > +
> > +               p++;
> > +               count--;
> > +       }
> > +
> > +       /* ASPEED_MBOX_CTRL_RECV bit is W1C, this also unmasks in 1 step */
> 
> W1C? Write to clear?
> 
> > +       aspeed_mbox_outb(mbox, ASPEED_MBOX_CTRL_RECV, ASPEED_MBOX_BMC_CTRL);
> > +       ret = p - buf;
> > +
> > +out_unlock:
> > +       mutex_unlock(&mbox->mutex);
> > +       return ret;
> > +}
> > +
> > +module_platform_driver(aspeed_mbox_driver);
> > +
> > +MODULE_DEVICE_TABLE(of, aspeed_mbox_match);
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Cyril Bur <cyrilbur-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>");
> > +MODULE_DESCRIPTION("ASpeed mailbox device driver");
> 
> ASPEED or Aspeed.
> 
> > --
> > 2.11.0
> > 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] drivers/mailbox: Add ASpeed mailbox driver

[Cyril Bur]

On Tue, 2017-02-07 at 16:10 +1030, Joel Stanley wrote:
> On Thu, Jan 12, 2017 at 10:59 AM, Cyril Bur <cyrilbur@gmail.com> wrote:
> > This provides access to the mbox registers on the ast2400 and ast2500
> > boards.
> 

Hey Joel,

Thanks for review, just one thing, I'll fixup the rest.

Cyril

> s/boards/SoCs/
> 
> > 
> > This driver allows arbitrary reads and writes to the 16 data registers as
> > the other end may have configured the mbox hardware to provide an
> > interrupt when a specific register gets written to.
> > 
> > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> 
> Send this to lkml as well next time you submit.
> 
> > ---
> >  drivers/mailbox/Kconfig       |   9 ++
> >  drivers/mailbox/Makefile      |   2 +
> >  drivers/mailbox/aspeed-mbox.c | 334 ++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 345 insertions(+)
> >  create mode 100644 drivers/mailbox/aspeed-mbox.c
> > 
> > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> > index ceff415f201c..10a7f3f2765c 100644
> > --- a/drivers/mailbox/Kconfig
> > +++ b/drivers/mailbox/Kconfig
> > @@ -152,4 +152,13 @@ config BCM_PDC_MBOX
> >           Mailbox implementation for the Broadcom PDC ring manager,
> >           which provides access to various offload engines on Broadcom
> >           SoCs. Say Y here if you want to use the Broadcom PDC.
> > +
> > +config ASPEED_MBOX
> 
> Call this ASPEED_LPC_MAILBOX, as it's a in the LPC IP block.
> 

I think I'll stick with ASPEED_LPC_MBOX, the rest of the file uses MBOX
more widely when talking about controllers.

> > +       depends on (ARCH_ASPEED || COMPILE_TEST) && REGMAP && MFD_SYSCON
> > +       bool "Aspeed ast2400/2500 Mailbox Controller"
> 
> Call this Aspeed LPC Mailbox Controller, as the layout is shared by
> SoCs other than the 2500 and 2400.
> 
> > +       default "y"
> > +       ---help---
> > +         Provides a driver for the MBOX registers found on Aspeed SOCs
> > +         (AST2400 and AST2500). This driver provides a device for aspeed
> > +         mbox registers
> >  endif
> > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> > index 7dde4f609ae8..db5b4f3f29e0 100644
> > --- a/drivers/mailbox/Makefile
> > +++ b/drivers/mailbox/Makefile
> > @@ -31,3 +31,5 @@ obj-$(CONFIG_HI6220_MBOX)     += hi6220-mailbox.o
> >  obj-$(CONFIG_BCM_PDC_MBOX)     += bcm-pdc-mailbox.o
> > 
> >  obj-$(CONFIG_TEGRA_HSP_MBOX)   += tegra-hsp.o
> > +
> > +obj-$(CONFIG_ASPEED_MBOX)      += aspeed-mbox.o
> > diff --git a/drivers/mailbox/aspeed-mbox.c b/drivers/mailbox/aspeed-mbox.c
> > new file mode 100644
> > index 000000000000..c4ee6ba228ea
> > --- /dev/null
> > +++ b/drivers/mailbox/aspeed-mbox.c
> > +
> > +static ssize_t aspeed_mbox_read(struct file *file, char __user *buf,
> > +                               size_t count, loff_t *ppos)
> > +{
> > +       struct aspeed_mbox *mbox = file_mbox(file);
> > +       char __user *p = buf;
> > +       ssize_t ret;
> > +       int i;
> > +
> > +       if (!access_ok(VERIFY_WRITE, buf, count))
> > +               return -EFAULT;
> 
> As discussed in the LPC driver review:
> 
>  "And don't call access_ok(), it's racy and no driver should ever do that."
> 
> Make sure all of the things you fixed in that driver are fixed in this one.
> 
> > +
> > +       if (count + *ppos > ASPEED_MBOX_NUM_REGS)
> > +               return -EINVAL;
> > +
> > +       if (file->f_flags & O_NONBLOCK) {
> > +               if (!(aspeed_mbox_inb(mbox, ASPEED_MBOX_BMC_CTRL) &
> > +                               ASPEED_MBOX_CTRL_RECV))
> > +                       return -EAGAIN;
> > +       } else if (wait_event_interruptible(mbox->queue,
> > +                               aspeed_mbox_inb(mbox, ASPEED_MBOX_BMC_CTRL) &
> > +                               ASPEED_MBOX_CTRL_RECV)) {
> > +               return -ERESTARTSYS;
> > +       }
> > +
> > +       mutex_lock(&mbox->mutex);
> > +
> > +       for (i = *ppos; count > 0 && i < ASPEED_MBOX_NUM_REGS; i++) {
> > +               uint8_t reg = aspeed_mbox_inb(mbox, ASPEED_MBOX_DATA_0 + (i * 4));
> > +
> > +               ret = __put_user(reg, p);
> > +               if (ret)
> > +                       goto out_unlock;
> > +
> > +               p++;
> > +               count--;
> > +       }
> > +
> > +       /* ASPEED_MBOX_CTRL_RECV bit is W1C, this also unmasks in 1 step */
> 
> W1C? Write to clear?
> 
> > +       aspeed_mbox_outb(mbox, ASPEED_MBOX_CTRL_RECV, ASPEED_MBOX_BMC_CTRL);
> > +       ret = p - buf;
> > +
> > +out_unlock:
> > +       mutex_unlock(&mbox->mutex);
> > +       return ret;
> > +}
> > +
> > +module_platform_driver(aspeed_mbox_driver);
> > +
> > +MODULE_DEVICE_TABLE(of, aspeed_mbox_match);
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Cyril Bur <cyrilbur@gmail.com>");
> > +MODULE_DESCRIPTION("ASpeed mailbox device driver");
> 
> ASPEED or Aspeed.
> 
> > --
> > 2.11.0
> > 

Re: [PATCH 4/4] drivers/mailbox: Add ASpeed mailbox driver

[Joel Stanley]

On Wed, Feb 8, 2017 at 9:27 AM, Cyril Bur <cyrilbur-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
>> > index ceff415f201c..10a7f3f2765c 100644
>> > --- a/drivers/mailbox/Kconfig
>> > +++ b/drivers/mailbox/Kconfig
>> > @@ -152,4 +152,13 @@ config BCM_PDC_MBOX
>> >           Mailbox implementation for the Broadcom PDC ring manager,
>> >           which provides access to various offload engines on Broadcom
>> >           SoCs. Say Y here if you want to use the Broadcom PDC.
>> > +
>> > +config ASPEED_MBOX
>>
>> Call this ASPEED_LPC_MAILBOX, as it's a in the LPC IP block.
>>
>
> I think I'll stick with ASPEED_LPC_MBOX, the rest of the file uses MBOX
> more widely when talking about controllers.

No worries. I just wanted the _LPC bit added.

Cheers,

Joel
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] drivers/mailbox: Add ASpeed mailbox driver

[Joel Stanley]

On Wed, Feb 8, 2017 at 9:27 AM, Cyril Bur <cyrilbur@gmail.com> wrote:
>> > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
>> > index ceff415f201c..10a7f3f2765c 100644
>> > --- a/drivers/mailbox/Kconfig
>> > +++ b/drivers/mailbox/Kconfig
>> > @@ -152,4 +152,13 @@ config BCM_PDC_MBOX
>> >           Mailbox implementation for the Broadcom PDC ring manager,
>> >           which provides access to various offload engines on Broadcom
>> >           SoCs. Say Y here if you want to use the Broadcom PDC.
>> > +
>> > +config ASPEED_MBOX
>>
>> Call this ASPEED_LPC_MAILBOX, as it's a in the LPC IP block.
>>
>
> I think I'll stick with ASPEED_LPC_MBOX, the rest of the file uses MBOX
> more widely when talking about controllers.

No worries. I just wanted the _LPC bit added.

Cheers,

Joel