[RFC PATCH linux dev-4.13 0/3] Miscellaneous BMC interfaces

[RFC PATCH linux dev-4.13 0/3] Miscellaneous BMC interfaces

[Andrew Jeffery]

Hello,

The goal of this series is to expose BMC SoC-specific knobs and switches to
userspace. This potentially includes scratch registers - I've exposed the VGA
and SIO scratch registers for the AST2500 as an example.

This series is an RFC because I can think of a few things that could be
objectionable:

1. sysfs vs chardev (at least, for the scratch register access)
2. the name and compatible of the driver given that it's fairly generic
3. for the same reason, the location in the source tree
4. the devicetree bindings (which are as-yet undocumented)

Regarding 1, scratch register access might better be done via a chardev. As a
note for future reference, UIO is not appropriate in this circumstance as we
want to access addresses in the MMIO space that aren't necessarily sized or
aligned in terms of pages.

The implementation is based on the rough outline in benh's recent email to
lkml:

https://lkml.org/lkml/2018/4/10/222

Looking for comment on any of the above points.

Cheers,

Andrew

Andrew Jeffery (3):
  soc: aspeed: Miscellaneous control interfaces
  dts: aspeed-g5: Expose VGA scratch registers
  dts: aspeed-g5: Expose SuperIO scratch registers

 arch/arm/boot/dts/aspeed-g5.dtsi     | 136 +++++++++++++++++++++++++
 drivers/soc/Kconfig                  |   1 +
 drivers/soc/Makefile                 |   1 +
 drivers/soc/aspeed/Kconfig           |  11 +++
 drivers/soc/aspeed/Makefile          |   1 +
 drivers/soc/aspeed/aspeed-bmc-misc.c | 187 +++++++++++++++++++++++++++++++++++
 6 files changed, 337 insertions(+)
 create mode 100644 drivers/soc/aspeed/Kconfig
 create mode 100644 drivers/soc/aspeed/Makefile
 create mode 100644 drivers/soc/aspeed/aspeed-bmc-misc.c

-- 
2.14.1

[RFC PATCH linux dev-4.13 1/3] soc: aspeed: Miscellaneous control interfaces

[Andrew Jeffery]

The ASPEED BMC SoCs have many knobs and switches that are sometimes
design-specific and often defy any approach to unify them under an
existing subsystem.

Add a driver to translate a devicetree table into sysfs entries to
expose bits and fields for manipulation from userspace. This encompasses
concepts from scratch registers to boolean conditions to enable or
disable host interface features.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/soc/Kconfig                  |   1 +
 drivers/soc/Makefile                 |   1 +
 drivers/soc/aspeed/Kconfig           |  11 +++
 drivers/soc/aspeed/Makefile          |   1 +
 drivers/soc/aspeed/aspeed-bmc-misc.c | 187 +++++++++++++++++++++++++++++++++++
 5 files changed, 201 insertions(+)
 create mode 100644 drivers/soc/aspeed/Kconfig
 create mode 100644 drivers/soc/aspeed/Makefile
 create mode 100644 drivers/soc/aspeed/aspeed-bmc-misc.c

diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
index 07fc0ac51c52..776fd5978f70 100644
--- a/drivers/soc/Kconfig
+++ b/drivers/soc/Kconfig
@@ -1,6 +1,7 @@
 menu "SOC (System On Chip) specific Drivers"
 
 source "drivers/soc/actions/Kconfig"
+source "drivers/soc/aspeed/Kconfig"
 source "drivers/soc/atmel/Kconfig"
 source "drivers/soc/bcm/Kconfig"
 source "drivers/soc/fsl/Kconfig"
diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
index 9241125416ba..94a5b97c4466 100644
--- a/drivers/soc/Makefile
+++ b/drivers/soc/Makefile
@@ -3,6 +3,7 @@
 #
 
 obj-$(CONFIG_ARCH_ACTIONS)	+= actions/
+obj-$(CONFIG_ARCH_ASPEED)       += aspeed/
 obj-$(CONFIG_ARCH_AT91)		+= atmel/
 obj-y				+= bcm/
 obj-$(CONFIG_ARCH_DOVE)		+= dove/
diff --git a/drivers/soc/aspeed/Kconfig b/drivers/soc/aspeed/Kconfig
new file mode 100644
index 000000000000..704a4efe180f
--- /dev/null
+++ b/drivers/soc/aspeed/Kconfig
@@ -0,0 +1,11 @@
+menu "ASPEED SoC drivers"
+
+config ASPEED_BMC_MISC
+	bool "Miscellaneous ASPEED BMC interfaces"
+	depends on ARCH_ASPEED || COMPILE_TEST
+	default ARCH_ASPEED
+	help
+	  Say yes to expose VGA and LPC scratch registers, and other
+	  miscellaneous control interfaces specific to the ASPEED BMC SoCs
+
+endmenu
diff --git a/drivers/soc/aspeed/Makefile b/drivers/soc/aspeed/Makefile
new file mode 100644
index 000000000000..d1a80f9a584f
--- /dev/null
+++ b/drivers/soc/aspeed/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_ASPEED_BMC_MISC) += aspeed-bmc-misc.o
diff --git a/drivers/soc/aspeed/aspeed-bmc-misc.c b/drivers/soc/aspeed/aspeed-bmc-misc.c
new file mode 100644
index 000000000000..48522a736a05
--- /dev/null
+++ b/drivers/soc/aspeed/aspeed-bmc-misc.c
@@ -0,0 +1,187 @@
+#include <linux/kobject.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
+
+#define DEVICE_NAME "aspeed-bmc-misc"
+
+struct aspeed_bmc_ctrl {
+	const char *name;
+	u32 offset;
+	u32 mask;
+	u32 shift;
+	struct regmap *map;
+	struct kobj_attribute attr;
+};
+
+struct aspeed_bmc_misc {
+	struct device *dev;
+	struct regmap *map;
+	struct aspeed_bmc_ctrl *ctrls;
+	int nr_ctrls;
+};
+
+static int aspeed_bmc_misc_parse_dt_child(struct device_node *child,
+					  struct aspeed_bmc_ctrl *ctrl)
+{
+	int rc;
+
+	/* Example child:
+	 *
+	 * ilpc2ahb {
+	 *     offset = <0x80>;
+	 *     bit-mask = <0x1>;
+	 *     bit-shift = <6>;
+	 *     label = "foo";
+	 * }
+	 */
+	if (of_property_read_string(child, "label", &ctrl->name))
+		ctrl->name = child->name;
+
+	rc = of_property_read_u32(child, "offset", &ctrl->offset);
+	if (rc < 0)
+		return rc;
+
+	rc = of_property_read_u32(child, "bit-mask", &ctrl->mask);
+	if (rc < 0)
+		return rc;
+
+	rc = of_property_read_u32(child, "bit-shift", &ctrl->shift);
+	if (rc < 0)
+		return rc;
+
+	ctrl->mask <<= ctrl->shift;
+
+	return 0;
+}
+
+static int aspeed_bmc_misc_parse_dt(struct aspeed_bmc_misc *bmc,
+				    struct device_node *parent)
+{
+	struct aspeed_bmc_ctrl *ctrl;
+	struct device_node *child;
+	int rc;
+
+	bmc->nr_ctrls = of_get_child_count(parent);
+	bmc->ctrls = devm_kcalloc(bmc->dev, bmc->nr_ctrls, sizeof(*bmc->ctrls),
+				  GFP_KERNEL);
+	if (!bmc->ctrls)
+		return -ENOMEM;
+
+	ctrl = bmc->ctrls;
+	for_each_child_of_node(parent, child) {
+		rc = aspeed_bmc_misc_parse_dt_child(child, ctrl++);
+		if (rc < 0) {
+			of_node_put(child);
+			return rc;
+		}
+	}
+
+	return 0;
+}
+
+static ssize_t aspeed_bmc_misc_show(struct kobject *kobj,
+				    struct kobj_attribute *attr, char *buf)
+{
+	struct aspeed_bmc_ctrl *ctrl;
+	unsigned int val;
+	int rc;
+
+	ctrl = container_of(attr, struct aspeed_bmc_ctrl, attr);
+	rc = regmap_read(ctrl->map, ctrl->offset, &val);
+	if (rc)
+		return rc;
+
+	val &= ctrl->mask;
+	val >>= ctrl->shift;
+
+	return sprintf(buf, "%u\n", val);
+}
+
+static ssize_t aspeed_bmc_misc_store(struct kobject *kobj,
+				     struct kobj_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct aspeed_bmc_ctrl *ctrl;
+	long val;
+	int rc;
+
+	rc = kstrtol(buf, 0, &val);
+	if (rc)
+		return rc;
+
+	ctrl = container_of(attr, struct aspeed_bmc_ctrl, attr);
+	val <<= ctrl->shift;
+	rc = regmap_update_bits(ctrl->map, ctrl->offset, ctrl->mask, val);
+
+	return rc < 0 ? rc : count;
+}
+
+static int aspeed_bmc_misc_add_sysfs_attr(struct aspeed_bmc_misc *bmc,
+					  struct aspeed_bmc_ctrl *ctrl)
+{
+	ctrl->map = bmc->map;
+
+	sysfs_attr_init(&ctrl->attr.attr);
+	ctrl->attr.attr.name = ctrl->name;
+	ctrl->attr.attr.mode = 0664;
+	ctrl->attr.show = aspeed_bmc_misc_show;
+	ctrl->attr.store = aspeed_bmc_misc_store;
+
+	return sysfs_create_file(&bmc->dev->kobj, &ctrl->attr.attr);
+}
+
+static int aspeed_bmc_misc_populate_sysfs(struct aspeed_bmc_misc *bmc)
+{
+	int rc;
+	int i;
+
+	for (i = 0; i < bmc->nr_ctrls; i++) {
+		rc = aspeed_bmc_misc_add_sysfs_attr(bmc, &bmc->ctrls[i]);
+		if (rc < 0)
+			return rc;
+	}
+
+	return 0;
+}
+
+static int aspeed_bmc_misc_probe(struct platform_device *pdev)
+{
+	struct aspeed_bmc_misc *bmc;
+	int rc;
+
+	bmc = devm_kzalloc(&pdev->dev, sizeof(*bmc), GFP_KERNEL);
+	if (!bmc)
+		return -ENOMEM;
+
+	bmc->dev = &pdev->dev;
+	bmc->map = syscon_node_to_regmap(pdev->dev.parent->of_node);
+	if (IS_ERR(bmc->map))
+		return PTR_ERR(bmc->map);
+
+	rc = aspeed_bmc_misc_parse_dt(bmc, pdev->dev.of_node);
+	if (rc < 0)
+		return rc;
+
+	return aspeed_bmc_misc_populate_sysfs(bmc);
+}
+
+static const struct of_device_id aspeed_bmc_misc_match[] = {
+	{ .compatible = "aspeed,bmc-misc" },
+	{ },
+};
+
+static struct platform_driver aspeed_bmc_misc = {
+	.driver = {
+		.name		= DEVICE_NAME,
+		.of_match_table = aspeed_bmc_misc_match,
+	},
+	.probe = aspeed_bmc_misc_probe,
+};
+
+module_platform_driver(aspeed_bmc_misc);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Andrew Jeffery <andrew@aj.id.au>");
-- 
2.14.1

[RFC PATCH linux dev-4.13 2/3] dts: aspeed-g5: Expose VGA scratch registers

[Andrew Jeffery]

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 arch/arm/boot/dts/aspeed-g5.dtsi | 48 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
index 1468b4ad22dc..8321df50c593 100644
--- a/arch/arm/boot/dts/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed-g5.dtsi
@@ -158,6 +158,11 @@
 
 				};
 
+				vga_scratch: scratch@50 {
+					compatible = "aspeed,bmc-misc";
+					reg = <0x50 0x20>;
+				};
+
 				hwrng@78 {
 					compatible = "timeriomem_rng";
 					reg = <0x78 0x4>;
@@ -1425,3 +1430,46 @@
 		groups = "WDTRST2";
 	};
 };
+
+&vga_scratch {
+	vga0 {
+		offset = <0x50>;
+		bit-mask = <0xffffffff>;
+		bit-shift = <0>;
+	};
+	vga1 {
+		offset = <0x54>;
+		bit-mask = <0xffffffff>;
+		bit-shift = <0>;
+	};
+	vga2 {
+		offset = <0x58>;
+		bit-mask = <0xffffffff>;
+		bit-shift = <0>;
+	};
+	vga3 {
+		offset = <0x5c>;
+		bit-mask = <0xffffffff>;
+		bit-shift = <0>;
+	};
+	vga4 {
+		offset = <0x60>;
+		bit-mask = <0xffffffff>;
+		bit-shift = <0>;
+	};
+	vga5 {
+		offset = <0x64>;
+		bit-mask = <0xffffffff>;
+		bit-shift = <0>;
+	};
+	vga6 {
+		offset = <0x68>;
+		bit-mask = <0xffffffff>;
+		bit-shift = <0>;
+	};
+	vga7 {
+		offset = <0x6c>;
+		bit-mask = <0xffffffff>;
+		bit-shift = <0>;
+	};
+};
-- 
2.14.1

[RFC PATCH linux dev-4.13 3/3] dts: aspeed-g5: Expose SuperIO scratch registers

[Andrew Jeffery]

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 arch/arm/boot/dts/aspeed-g5.dtsi | 88 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 88 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
index 8321df50c593..25a88dd44d91 100644
--- a/arch/arm/boot/dts/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed-g5.dtsi
@@ -325,6 +325,11 @@
 						status = "disabled";
 					};
 
+					sio_scratch: scratch@f0 {
+						compatible = "aspeed,bmc-misc";
+						reg = <0xf0 0x10>;
+					};
+
 					mbox: mbox@180 {
 						compatible = "aspeed,ast2500-mbox";
 						reg = <0x180 0x5c>;
@@ -1473,3 +1478,86 @@
 		bit-shift = <0>;
 	};
 };
+
+&sio_scratch {
+	siorx_2b {
+		offset = <0xf0>;
+		bit-mask = <0xff>;
+		bit-shift = <24>;
+	};
+	siorx_2a {
+		offset = <0xf0>;
+		bit-mask = <0xff>;
+		bit-shift = <16>;
+	};
+	siorx_29 {
+		offset = <0xf0>;
+		bit-mask = <0xff>;
+		bit-shift = <8>;
+	};
+	siorx_28 {
+		offset = <0xf0>;
+		bit-mask = <0xff>;
+		bit-shift = <0>;
+	};
+	siorx_2f {
+		offset = <0xf4>;
+		bit-mask = <0xff>;
+		bit-shift = <24>;
+	};
+	siorx_2e {
+		offset = <0xf4>;
+		bit-mask = <0xff>;
+		bit-shift = <16>;
+	};
+	siorx_2d {
+		offset = <0xf4>;
+		bit-mask = <0xff>;
+		bit-shift = <8>;
+	};
+	siorx_2c {
+		offset = <0xf4>;
+		bit-mask = <0xff>;
+		bit-shift = <0>;
+	};
+	siorx_23 {
+		offset = <0xf8>;
+		bit-mask = <0xff>;
+		bit-shift = <24>;
+	};
+	siorx_22 {
+		offset = <0xf8>;
+		bit-mask = <0xff>;
+		bit-shift = <16>;
+	};
+	siorx_21 {
+		offset = <0xf8>;
+		bit-mask = <0xff>;
+		bit-shift = <8>;
+	};
+	siorx_20 {
+		offset = <0xf8>;
+		bit-mask = <0xff>;
+		bit-shift = <0>;
+	};
+	siorx_27 {
+		offset = <0xfc>;
+		bit-mask = <0xff>;
+		bit-shift = <24>;
+	};
+	siorx_26 {
+		offset = <0xfc>;
+		bit-mask = <0xff>;
+		bit-shift = <16>;
+	};
+	siorx_25 {
+		offset = <0xfc>;
+		bit-mask = <0xff>;
+		bit-shift = <8>;
+	};
+	siorx_24 {
+		offset = <0xfc>;
+		bit-mask = <0xff>;
+		bit-shift = <0>;
+	};
+};
-- 
2.14.1

Re: [RFC PATCH linux dev-4.13 1/3] soc: aspeed: Miscellaneous control interfaces

[Joel Stanley]

On 12 April 2018 at 13:21, Andrew Jeffery <andrew@aj.id.au> wrote:
> The ASPEED BMC SoCs have many knobs and switches that are sometimes
> design-specific and often defy any approach to unify them under an
> existing subsystem.
>
> Add a driver to translate a devicetree table into sysfs entries to
> expose bits and fields for manipulation from userspace. This encompasses
> concepts from scratch registers to boolean conditions to enable or
> disable host interface features.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  drivers/soc/Kconfig                  |   1 +
>  drivers/soc/Makefile                 |   1 +
>  drivers/soc/aspeed/Kconfig           |  11 +++
>  drivers/soc/aspeed/Makefile          |   1 +
>  drivers/soc/aspeed/aspeed-bmc-misc.c | 187 +++++++++++++++++++++++++++++++++++
>  5 files changed, 201 insertions(+)
>  create mode 100644 drivers/soc/aspeed/Kconfig
>  create mode 100644 drivers/soc/aspeed/Makefile
>  create mode 100644 drivers/soc/aspeed/aspeed-bmc-misc.c
>
> diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> index 07fc0ac51c52..776fd5978f70 100644
> --- a/drivers/soc/Kconfig
> +++ b/drivers/soc/Kconfig
> @@ -1,6 +1,7 @@
>  menu "SOC (System On Chip) specific Drivers"
>
>  source "drivers/soc/actions/Kconfig"
> +source "drivers/soc/aspeed/Kconfig"
>  source "drivers/soc/atmel/Kconfig"
>  source "drivers/soc/bcm/Kconfig"
>  source "drivers/soc/fsl/Kconfig"
> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> index 9241125416ba..94a5b97c4466 100644
> --- a/drivers/soc/Makefile
> +++ b/drivers/soc/Makefile
> @@ -3,6 +3,7 @@
>  #
>
>  obj-$(CONFIG_ARCH_ACTIONS)     += actions/
> +obj-$(CONFIG_ARCH_ASPEED)       += aspeed/
>  obj-$(CONFIG_ARCH_AT91)                += atmel/
>  obj-y                          += bcm/
>  obj-$(CONFIG_ARCH_DOVE)                += dove/
> diff --git a/drivers/soc/aspeed/Kconfig b/drivers/soc/aspeed/Kconfig
> new file mode 100644
> index 000000000000..704a4efe180f
> --- /dev/null
> +++ b/drivers/soc/aspeed/Kconfig
> @@ -0,0 +1,11 @@
> +menu "ASPEED SoC drivers"
> +
> +config ASPEED_BMC_MISC
> +       bool "Miscellaneous ASPEED BMC interfaces"

Do you mean bool or tristate? if you mean bool, drop the module-y bits below.

> +       depends on ARCH_ASPEED || COMPILE_TEST
> +       default ARCH_ASPEED
> +       help
> +         Say yes to expose VGA and LPC scratch registers, and other
> +         miscellaneous control interfaces specific to the ASPEED BMC SoCs
> +
> +endmenu
> diff --git a/drivers/soc/aspeed/Makefile b/drivers/soc/aspeed/Makefile
> new file mode 100644
> index 000000000000..d1a80f9a584f
> --- /dev/null
> +++ b/drivers/soc/aspeed/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_ASPEED_BMC_MISC) += aspeed-bmc-misc.o
> diff --git a/drivers/soc/aspeed/aspeed-bmc-misc.c b/drivers/soc/aspeed/aspeed-bmc-misc.c
> new file mode 100644
> index 000000000000..48522a736a05
> --- /dev/null
> +++ b/drivers/soc/aspeed/aspeed-bmc-misc.c

copyright headahs

> @@ -0,0 +1,187 @@
> +#include <linux/kobject.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
> +
> +#define DEVICE_NAME "aspeed-bmc-misc"

This is the name of our chardev? No, it appears to just be the driver
name. You can probably just use it directly.

An observation: It's the only thing inside this file that is aspeed
specific. If not for the name, it could be generic code.

lgtm. I suggest writing up some bindings and putting on the asbestos pants.

I'll merge this into openbmc with the IBM copyright added to the top.
With this adding userspace ABI, anyone who writes code against it will
need to make themselves available for rework as it goes through
upstream review.

Cheers,

Joel

Re: [RFC PATCH linux dev-4.13 2/3] dts: aspeed-g5: Expose VGA scratch registers

[Joel Stanley]

On 12 April 2018 at 13:21, Andrew Jeffery <andrew@aj.id.au> wrote:
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  arch/arm/boot/dts/aspeed-g5.dtsi | 48 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
>
> diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
> index 1468b4ad22dc..8321df50c593 100644
> --- a/arch/arm/boot/dts/aspeed-g5.dtsi
> +++ b/arch/arm/boot/dts/aspeed-g5.dtsi
> @@ -158,6 +158,11 @@
>
>                                 };
>
> +                               vga_scratch: scratch@50 {
> +                                       compatible = "aspeed,bmc-misc";

Convention is to use aspeed,<socname>-<hardware>, so this would be
aspeed,ast2500-bmc-misc.


> +                                       reg = <0x50 0x20>;
> +                               };
> +
>                                 hwrng@78 {
>                                         compatible = "timeriomem_rng";
>                                         reg = <0x78 0x4>;
> @@ -1425,3 +1430,46 @@
>                 groups = "WDTRST2";
>         };
>  };
> +
> +&vga_scratch {
> +       vga0 {

Do these names come out in the userspace API?

> +               offset = <0x50>;

Would reg <> be more devicetree-y than offset = <>?


> +               bit-mask = <0xffffffff>;
> +               bit-shift = <0>;

Should we assume this is the default? Or something else? Having a
default for the common case would be nice.

> +       };
> +       vga1 {
> +               offset = <0x54>;
> +               bit-mask = <0xffffffff>;
> +               bit-shift = <0>;

Re: [RFC PATCH linux dev-4.13 1/3] soc: aspeed: Miscellaneous control interfaces

[Andrew Jeffery]

On Thu, 19 Apr 2018, at 12:37, Joel Stanley wrote:
> On 12 April 2018 at 13:21, Andrew Jeffery <andrew@aj.id.au> wrote:
> > The ASPEED BMC SoCs have many knobs and switches that are sometimes
> > design-specific and often defy any approach to unify them under an
> > existing subsystem.
> >
> > Add a driver to translate a devicetree table into sysfs entries to
> > expose bits and fields for manipulation from userspace. This encompasses
> > concepts from scratch registers to boolean conditions to enable or
> > disable host interface features.
> >
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> >  drivers/soc/Kconfig                  |   1 +
> >  drivers/soc/Makefile                 |   1 +
> >  drivers/soc/aspeed/Kconfig           |  11 +++
> >  drivers/soc/aspeed/Makefile          |   1 +
> >  drivers/soc/aspeed/aspeed-bmc-misc.c | 187 +++++++++++++++++++++++++++++++++++
> >  5 files changed, 201 insertions(+)
> >  create mode 100644 drivers/soc/aspeed/Kconfig
> >  create mode 100644 drivers/soc/aspeed/Makefile
> >  create mode 100644 drivers/soc/aspeed/aspeed-bmc-misc.c
> >
> > diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> > index 07fc0ac51c52..776fd5978f70 100644
> > --- a/drivers/soc/Kconfig
> > +++ b/drivers/soc/Kconfig
> > @@ -1,6 +1,7 @@
> >  menu "SOC (System On Chip) specific Drivers"
> >
> >  source "drivers/soc/actions/Kconfig"
> > +source "drivers/soc/aspeed/Kconfig"
> >  source "drivers/soc/atmel/Kconfig"
> >  source "drivers/soc/bcm/Kconfig"
> >  source "drivers/soc/fsl/Kconfig"
> > diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> > index 9241125416ba..94a5b97c4466 100644
> > --- a/drivers/soc/Makefile
> > +++ b/drivers/soc/Makefile
> > @@ -3,6 +3,7 @@
> >  #
> >
> >  obj-$(CONFIG_ARCH_ACTIONS)     += actions/
> > +obj-$(CONFIG_ARCH_ASPEED)       += aspeed/
> >  obj-$(CONFIG_ARCH_AT91)                += atmel/
> >  obj-y                          += bcm/
> >  obj-$(CONFIG_ARCH_DOVE)                += dove/
> > diff --git a/drivers/soc/aspeed/Kconfig b/drivers/soc/aspeed/Kconfig
> > new file mode 100644
> > index 000000000000..704a4efe180f
> > --- /dev/null
> > +++ b/drivers/soc/aspeed/Kconfig
> > @@ -0,0 +1,11 @@
> > +menu "ASPEED SoC drivers"
> > +
> > +config ASPEED_BMC_MISC
> > +       bool "Miscellaneous ASPEED BMC interfaces"
> 
> Do you mean bool or tristate? if you mean bool, drop the module-y bits below.

Should be tristate, was just slapping bits together :)

> 
> > +       depends on ARCH_ASPEED || COMPILE_TEST
> > +       default ARCH_ASPEED
> > +       help
> > +         Say yes to expose VGA and LPC scratch registers, and other
> > +         miscellaneous control interfaces specific to the ASPEED BMC SoCs
> > +
> > +endmenu
> > diff --git a/drivers/soc/aspeed/Makefile b/drivers/soc/aspeed/Makefile
> > new file mode 100644
> > index 000000000000..d1a80f9a584f
> > --- /dev/null
> > +++ b/drivers/soc/aspeed/Makefile
> > @@ -0,0 +1 @@
> > +obj-$(CONFIG_ASPEED_BMC_MISC) += aspeed-bmc-misc.o
> > diff --git a/drivers/soc/aspeed/aspeed-bmc-misc.c b/drivers/soc/aspeed/aspeed-bmc-misc.c
> > new file mode 100644
> > index 000000000000..48522a736a05
> > --- /dev/null
> > +++ b/drivers/soc/aspeed/aspeed-bmc-misc.c
> 
> copyright headahs

Good catch.

> 
> > @@ -0,0 +1,187 @@
> > +#include <linux/kobject.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/mfd/syscon.h>
> > +
> > +#define DEVICE_NAME "aspeed-bmc-misc"
> 
> This is the name of our chardev? No, it appears to just be the driver
> name. You can probably just use it directly.

Yep

> 
> An observation: It's the only thing inside this file that is aspeed
> specific. If not for the name, it could be generic code.

I agree, hence some of the bullet points in the cover letter. But it's name and location was also partly driven by Arnd's suggestion that we stick the junk in drivers/soc, and it felt a bit weird not to use aspeed in the name given the convention there.

> 
> lgtm. I suggest writing up some bindings and putting on the asbestos pants.

Wheee...

> 
> I'll merge this into openbmc with the IBM copyright added to the top.

Sounds good.

> With this adding userspace ABI, anyone who writes code against it will
> need to make themselves available for rework as it goes through
> upstream review.

Yeah. Will try to make sure people are aware of this.

Thanks for the feedback.

Andrew

Re: [RFC PATCH linux dev-4.13 2/3] dts: aspeed-g5: Expose VGA scratch registers

[Andrew Jeffery]

On Thu, 19 Apr 2018, at 12:57, Joel Stanley wrote:
> On 12 April 2018 at 13:21, Andrew Jeffery <andrew@aj.id.au> wrote:
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> >  arch/arm/boot/dts/aspeed-g5.dtsi | 48 ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 48 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
> > index 1468b4ad22dc..8321df50c593 100644
> > --- a/arch/arm/boot/dts/aspeed-g5.dtsi
> > +++ b/arch/arm/boot/dts/aspeed-g5.dtsi
> > @@ -158,6 +158,11 @@
> >
> >                                 };
> >
> > +                               vga_scratch: scratch@50 {
> > +                                       compatible = "aspeed,bmc-misc";
> 
> Convention is to use aspeed,<socname>-<hardware>, so this would be
> aspeed,ast2500-bmc-misc.

Yeah, this partly comes back to the naming problem you mentioned in the other patch. The functionality is generic, and so probably shouldn't mention aspeed at all, let alone the SoC revision.

> 
> 
> > +                                       reg = <0x50 0x20>;
> > +                               };
> > +
> >                                 hwrng@78 {
> >                                         compatible = "timeriomem_rng";
> >                                         reg = <0x78 0x4>;
> > @@ -1425,3 +1430,46 @@
> >                 groups = "WDTRST2";
> >         };
> >  };
> > +
> > +&vga_scratch {
> > +       vga0 {
> 
> Do these names come out in the userspace API?

Yes, though you can override the use of the node name by adding a label property inside the node. That way if someone complains about the node naming we don't have to break userspace.

> 
> > +               offset = <0x50>;
> 
> Would reg <> be more devicetree-y than offset = <>?

There's a mix. I was looking through some other syscon-type stuff and some of those drivers used offset. This approach might avoid militant opinions around the relationship between the unit address and the reg property, as you "can't" have multiple nodes with the same unit address. There's some discussion on this in the PECI threads.

> 
> 
> > +               bit-mask = <0xffffffff>;
> > +               bit-shift = <0>;
> 
> Should we assume this is the default? Or something else? Having a
> default for the common case would be nice.

Whilst the nodes I've added in these examples expose an entire register, I think this would actually be a bit of an unusual case. More likely you'll want to expose a single bit from some register, in which case a default for mask (though maybe 0x1?) or shift isn't going to make any sense.

Cheers,

Andrew

Re: [RFC PATCH linux dev-4.13 2/3] dts: aspeed-g5: Expose VGA scratch registers

[Joel Stanley]

On 19 April 2018 at 13:30, Andrew Jeffery <andrew@aj.id.au> wrote:
> On Thu, 19 Apr 2018, at 12:57, Joel Stanley wrote:
>> On 12 April 2018 at 13:21, Andrew Jeffery <andrew@aj.id.au> wrote:
>> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>> > ---
>> >  arch/arm/boot/dts/aspeed-g5.dtsi | 48 ++++++++++++++++++++++++++++++++++++++++
>> >  1 file changed, 48 insertions(+)
>> >
>> > diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
>> > index 1468b4ad22dc..8321df50c593 100644
>> > --- a/arch/arm/boot/dts/aspeed-g5.dtsi
>> > +++ b/arch/arm/boot/dts/aspeed-g5.dtsi
>> > @@ -158,6 +158,11 @@
>> >
>> >                                 };
>> >
>> > +                               vga_scratch: scratch@50 {
>> > +                                       compatible = "aspeed,bmc-misc";
>>
>> Convention is to use aspeed,<socname>-<hardware>, so this would be
>> aspeed,ast2500-bmc-misc.
>
> Yeah, this partly comes back to the naming problem you mentioned in the other patch. The functionality is generic, and so probably shouldn't mention aspeed at all, let alone the SoC revision.
>

I disagree. The bindings need to be hardware specific, but the code
can be generic.

We have a preceidnce for this in the reset controller. There's a
controller called reset-simple that contains generic code (with some
quirks for some systems it supports), and a list of compatible
strings.

On the other side of the equation, the bindings themselves are
specific to each SoC.

If we get push back for being generic we can cite them as our inspiration :)

>>
>>
>> > +                                       reg = <0x50 0x20>;
>> > +                               };
>> > +
>> >                                 hwrng@78 {
>> >                                         compatible = "timeriomem_rng";
>> >                                         reg = <0x78 0x4>;
>> > @@ -1425,3 +1430,46 @@
>> >                 groups = "WDTRST2";
>> >         };
>> >  };
>> > +
>> > +&vga_scratch {
>> > +       vga0 {
>>
>> Do these names come out in the userspace API?
>
> Yes, though you can override the use of the node name by adding a label property inside the node. That way if someone complains about the node naming we don't have to break userspace.

Similar to the leds bindings, sgtm.

>
>>
>> > +               offset = <0x50>;
>>
>> Would reg <> be more devicetree-y than offset = <>?
>
> There's a mix. I was looking through some other syscon-type stuff and some of those drivers used offset. This approach might avoid militant opinions around the relationship between the unit address and the reg property, as you "can't" have multiple nodes with the same unit address. There's some discussion on this in the PECI threads.
>
>>
>>
>> > +               bit-mask = <0xffffffff>;
>> > +               bit-shift = <0>;
>>
>> Should we assume this is the default? Or something else? Having a
>> default for the common case would be nice.
>
> Whilst the nodes I've added in these examples expose an entire register, I think this would actually be a bit of an unusual case. More likely you'll want to expose a single bit from some register, in which case a default for mask (though maybe 0x1?) or shift isn't going to make any sense.

Ok. If you can think of something that makes the bindings less verbose
I'm all for that.

Re: [RFC PATCH linux dev-4.13 2/3] dts: aspeed-g5: Expose VGA scratch registers

[Andrew Jeffery]

> >
> >>
> >>
> >> > +               bit-mask = <0xffffffff>;
> >> > +               bit-shift = <0>;
> >>
> >> Should we assume this is the default? Or something else? Having a
> >> default for the common case would be nice.
> >
> > Whilst the nodes I've added in these examples expose an entire register, I think this would actually be a bit of an unusual case. More likely you'll want to expose a single bit from some register, in which case a default for mask (though maybe 0x1?) or shift isn't going to make any sense.
> 
> Ok. If you can think of something that makes the bindings less verbose
> I'm all for that.

Part of the issue (IMO) is we're stretching the intent of the driver here to expose the scratch registers. Hence the bullet point on sysfs vs chardev mentioned in the cover letter. Using the driver for the scratch registers was driven by some conversations with BenH; we could not do this and go the chardev route instead. I'd like Ben to chime in, as I know JK is more for the chardev approach.

Andrew

RE: [RFC PATCH linux dev-4.13 1/3] soc: aspeed: Miscellaneous control interfaces

[Eugene.Cho]

>> An observation: It's the only thing inside this file that is aspeed 
>> specific. If not for the name, it could be generic code.

>I agree, hence some of the bullet points in the cover letter. But it's name and location was also partly driven by Arnd's suggestion that we stick the junk in drivers/soc, and it felt a bit weird not to use aspeed in the name >given the convention there.

Just FYI - We've got the same exact problem on Nuvoton (just odds and ends in misc registers). Does this imply we will need a soc/npcm750 which also does the exact same thing?

I'm guessing making it too generic - will make it harder to get upstream?

Re: [RFC PATCH linux dev-4.13 1/3] soc: aspeed: Miscellaneous control interfaces

[Andrew Jeffery]

Hi Eugene,

On Wed, 2 May 2018, at 02:51, Eugene.Cho@dell.com wrote:
> 
> >> An observation: It's the only thing inside this file that is aspeed 
> >> specific. If not for the name, it could be generic code.
> 
> >I agree, hence some of the bullet points in the cover letter. But it's name and location was also partly driven by Arnd's suggestion that we stick the junk in drivers/soc, and it felt a bit weird not to use aspeed in the name >given the convention there.
> 
> Just FYI - We've got the same exact problem on Nuvoton (just odds and 
> ends in misc registers). Does this imply we will need a soc/npcm750 
> which also does the exact same thing?

I don't think so, if anything it's an argument for moving it out of soc/aspeed

> 
> I'm guessing making it too generic - will make it harder to get upstream?
> 

Probably, but the more evidence we have for requiring it the harder it is to argue against, and so the fact that this could help the NPCM as well might even make it easier to upstream.

Have you played with the implementation at all? Would be good to get some consensus on it being a decent approach here before we go upstream with it. I also need to add devicetree bindings and ABI documentation...

Cheers,

Andrew

RE: [RFC PATCH linux dev-4.13 1/3] soc: aspeed: Miscellaneous control interfaces

[Eugene.Cho]

Hey Andrew - 

> Have you played with the implementation at all? Would be good to get some consensus on it being a decent approach here before we go upstream with it. I also need to add devicetree bindings and ABI documentation...

Yea we have a "Dell" driver which almost matches exactly aspeed-bmc-misc.c (sysfs node per register field...  label,reg-offset, and bit-mask configured via dt). 
I think what you got is good, only thing I would say is maybe remove the references to aspeed?

Re: [RFC PATCH linux dev-4.13 1/3] soc: aspeed: Miscellaneous control interfaces

[Andrew Jeffery]

On Fri, 11 May 2018, at 04:09, Eugene.Cho@dell.com wrote:
> Hey Andrew - 
> 
> > Have you played with the implementation at all? Would be good to get some consensus on it being a decent approach here before we go upstream with it. I also need to add devicetree bindings and ABI documentation...
> 
> Yea we have a "Dell" driver which almost matches exactly aspeed-bmc-
> misc.c (sysfs node per register field...  label,reg-offset, and bit-mask 
> configured via dt). 
> I think what you got is good, only thing I would say is maybe remove the 
> references to aspeed?
> 

Yeah, I'll rework the patches to file-off the ASPEED parts and find a more appropriate home for the driver.

Cheers for the feedback.

Andrew