[U-Boot] [PATCH 0/4] Add a pinctrl driver for Merrifield to pinmux I2C#6

[U-Boot] [PATCH 0/4] Add a pinctrl driver for Merrifield to pinmux I2C#6

[Georgii Staroselskii]

We have lacked the support for any pinmuxing in U-Boot for Merrifield.
A need for pinmuxing some pins has arisen from the fact the I2C#6 is shared
with I2C#8 on Merrifield. The latter is not easily accessible because it's
a part of a separate MCU we don't have easy access to.

I2C#6 can be and should be made use of in Linux but couldn't because it
was blocked by the SCU.

The proposed change is to implement a minimalistic pinctrl driver that
reads the configuration off a device tree blob and configures the pins 
accordingly.

The driver needs some changes to SCU API and thus the addition of
scu_ipc_raw_command().

Andy Shevchenko has been helping me by making a prior review and made
a lot of suggestions about the general approach that should be taken.

He should also be given credit for the initial kernel code that I have
taken as a reference.

The code has been tested on 5 different Edisons on Intel Edison Breakout
board and a couple of custom Emlid PCBs by booting a kernel and issuing
a i2cdetect -r -y 6 and then loading a driver that made use of the bus.

I also created a Gist on Github with a lot of relevant information like

- output of `acpidump -o tables.dat`
- dmesg
- output of `grep -H 15 /sys/bus/acpi/devices/*/status`
- cat /sys/kernel/debug/gpio
- cat /sys/kernel/debug/pinctrl/INTC1002\:00/pins
- output of `i2cdetect -y -r 6` w/ and w/o external device on the bus

Here it is:
https://gist.github.com/staroselskii/097808e05fd609dbafd4fe5ebd618708

Georgii Staroselskii (4):
  x86: cpu: introduce scu_ipc_raw_command()
  x86: tangier: pinmux: add API to configure protected pins
  x86: dts: edison: configure I2C#6 pins
  x86: tangier: acpi: add I2C6 node

 arch/x86/cpu/tangier/Makefile                      |   2 +-
 arch/x86/cpu/tangier/pinmux.c                      | 188 +++++++++++++++++++++
 arch/x86/dts/edison.dts                            |  22 +++
 .../include/asm/arch-tangier/acpi/southcluster.asl |  10 ++
 arch/x86/include/asm/scu.h                         |   4 +
 arch/x86/lib/scu.c                                 |  35 ++++
 6 files changed, 260 insertions(+), 1 deletion(-)
 create mode 100644 arch/x86/cpu/tangier/pinmux.c

-- 
2.7.4

[U-Boot] [PATCH 1/4] x86: cpu: introduce scu_ipc_raw_command()

[Georgii Staroselskii]

This interface will be used to configure properly some pins on
Merrifield that are shared with SCU.

scu_ipc_raw_command() writes SPTR and DPTR registers before sending
a command to SCU.

This code has been ported from Linux work done by Andy Shevchenko.

Signed-off-by: Georgii Staroselskii <georgii.staroselskii@emlid.com>
---
 arch/x86/include/asm/scu.h |  4 ++++
 arch/x86/lib/scu.c         | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/arch/x86/include/asm/scu.h b/arch/x86/include/asm/scu.h
index 7ce5824..f5ec5a1 100644
--- a/arch/x86/include/asm/scu.h
+++ b/arch/x86/include/asm/scu.h
@@ -6,6 +6,8 @@
 #define _X86_ASM_SCU_IPC_H_
 
 /* IPC defines the following message types */
+#define IPCMSG_INDIRECT_READ	0x02
+#define IPCMSG_INDIRECT_WRITE	0x05
 #define IPCMSG_WARM_RESET	0xf0
 #define IPCMSG_COLD_RESET	0xf1
 #define IPCMSG_SOFT_RESET	0xf2
@@ -23,5 +25,7 @@ struct ipc_ifwi_version {
 /* Issue commands to the SCU with or without data */
 int scu_ipc_simple_command(u32 cmd, u32 sub);
 int scu_ipc_command(u32 cmd, u32 sub, u32 *in, int inlen, u32 *out, int outlen);
+int scu_ipc_raw_command(u32 cmd, u32 sub, u32 *in, int inlen, u32 *out,
+			int outlen, u32 dptr, u32 sptr);
 
 #endif	/* _X86_ASM_SCU_IPC_H_ */
diff --git a/arch/x86/lib/scu.c b/arch/x86/lib/scu.c
index caa04c6..847bb77 100644
--- a/arch/x86/lib/scu.c
+++ b/arch/x86/lib/scu.c
@@ -101,6 +101,41 @@ static int scu_ipc_cmd(struct ipc_regs *regs, u32 cmd, u32 sub,
 	return err;
 }
 
+int scu_ipc_raw_command(u32 cmd, u32 sub, u32 *in, int inlen, u32 *out,
+			int outlen, u32 dptr, u32 sptr)
+{
+	int inbuflen = DIV_ROUND_UP(inlen, 4);
+	struct udevice *dev;
+	struct scu *scu;
+	int ret;
+
+	ret = syscon_get_by_driver_data(X86_SYSCON_SCU, &dev);
+	if (ret)
+		return ret;
+
+	scu = dev_get_priv(dev);
+
+	/* Up to 16 bytes */
+	if (inbuflen > 4)
+		return -EINVAL;
+
+	writel(dptr, &scu->regs->dptr);
+	writel(sptr, &scu->regs->sptr);
+
+	/*
+	 * SRAM controller doesn't support 8-bit writes, it only
+	 * supports 32-bit writes, so we have to copy input data into
+	 * the temporary buffer, and SCU FW will use the inlen to
+	 * determine the actual input data length in the temporary
+	 * buffer.
+	 */
+
+	u32 inbuf[4] = {0};
+
+	memcpy(inbuf, in, inlen);
+
+	return scu_ipc_cmd(scu->regs, cmd, sub, inbuf, inlen, out, outlen);
+}
 /**
  * scu_ipc_simple_command() - send a simple command
  * @cmd: command
-- 
2.7.4

[U-Boot] [PATCH 2/4] x86: tangier: pinmux: add API to configure protected pins

[Georgii Staroselskii]

This API is going to be used to configure some pins that are protected
for simple modification.

It's not a comprehensive pinctrl driver but can be turned into one
when we need this in the future. Now it is planned to be used only
in one place. So that's why I decided not to polute the codebase with a
full-blown pinctrl-merrifield nobody will use.

This driver reads corresponding fields in DT and configures pins
accordingly.

The "protected" flag is used to distinguish configuration of SCU-owned
pins from the ordinary ones.

The code has been adapted from Linux work done by Andy Shevchenko
in pinctrl-merrfifield.c

Signed-off-by: Georgii Staroselskii <georgii.staroselskii@emlid.com>
---
 arch/x86/cpu/tangier/Makefile |   2 +-
 arch/x86/cpu/tangier/pinmux.c | 188 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 189 insertions(+), 1 deletion(-)
 create mode 100644 arch/x86/cpu/tangier/pinmux.c

diff --git a/arch/x86/cpu/tangier/Makefile b/arch/x86/cpu/tangier/Makefile
index 8274482..68f4a32 100644
--- a/arch/x86/cpu/tangier/Makefile
+++ b/arch/x86/cpu/tangier/Makefile
@@ -2,5 +2,5 @@
 #
 # Copyright (c) 2017 Intel Corporation
 
-obj-y += car.o tangier.o sdram.o sysreset.o
+obj-y += car.o tangier.o sdram.o sysreset.o pinmux.o
 obj-$(CONFIG_GENERATE_ACPI_TABLE) += acpi.o
diff --git a/arch/x86/cpu/tangier/pinmux.c b/arch/x86/cpu/tangier/pinmux.c
new file mode 100644
index 0000000..c59b63c
--- /dev/null
+++ b/arch/x86/cpu/tangier/pinmux.c
@@ -0,0 +1,188 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2018 Emlid Limited
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <dm/pinctrl.h>
+#include <fdtdec.h>
+#include <regmap.h>
+#include <syscon.h>
+#include <asm/cpu.h>
+#include <asm/scu.h>
+#include <linux/io.h>
+
+#define BUFCFG_OFFSET	0x100
+
+#define MRFLD_FAMILY_LEN	0x400
+
+/* These are taken from Linux kernel */
+#define MRFLD_PINMODE_MASK	0x07
+
+#define pin_to_bufno(f, p)	  ((p) - (f)->pin_base)
+
+struct mrfld_family {
+	unsigned int family_number;
+	unsigned int pin_base;
+	size_t npins;
+	void __iomem *regs;
+};
+
+#define MRFLD_FAMILY(b, s, e)				\
+	{				\
+		.family_number = (b),			\
+		.pin_base = (s),			\
+		.npins = (e) - (s) + 1,			\
+	}
+
+static struct mrfld_family mrfld_families[] = {
+	MRFLD_FAMILY(7, 101, 114),
+};
+
+struct mrfld_pinctrl {
+	const struct mrfld_family *families;
+	size_t nfamilies;
+};
+
+static const struct mrfld_family *
+mrfld_get_family(struct mrfld_pinctrl *mp, unsigned int pin)
+{
+	const struct mrfld_family *family;
+	unsigned int i;
+
+	for (i = 0; i < mp->nfamilies; i++) {
+		family = &mp->families[i];
+		if (pin >= family->pin_base &&
+			pin < family->pin_base + family->npins)
+			return family;
+	}
+
+	printf("failed to find family for pin %u\n", pin);
+	return NULL;
+}
+
+static void __iomem *
+mrfld_get_bufcfg(struct mrfld_pinctrl *pinctrl, unsigned int pin)
+{
+	const struct mrfld_family *family;
+	unsigned int bufno;
+
+	family =  mrfld_get_family(pinctrl, pin);
+	if (!family)
+		return NULL;
+
+	bufno = pin_to_bufno(family, pin);
+
+	return family->regs + BUFCFG_OFFSET + bufno * 4;
+}
+
+static void
+mrfld_setup_families(void *base_addr, struct mrfld_family *families, unsigned int nfam)
+{
+	for (int i = 0; i < nfam; i++) {
+		struct mrfld_family *family = &families[i];
+
+		family->regs = base_addr + family->family_number * MRFLD_FAMILY_LEN;
+	}
+}
+
+static int mrfld_pinconfig_protected(unsigned int pin, u32 mask, u32 bits)
+{
+	struct mrfld_pinctrl *pinctrl;
+	struct udevice *dev;
+	void __iomem *bufcfg;
+	u32 v, value;
+	int ret;
+
+	ret = syscon_get_by_driver_data(X86_SYSCON_PINCONF, &dev);
+	if (ret)
+		return ret;
+
+	pinctrl = dev_get_priv(dev);
+
+	bufcfg = mrfld_get_bufcfg(pinctrl, pin);
+	if (!bufcfg)
+		return -EINVAL;
+
+	value = readl(bufcfg);
+
+	v = (value & ~mask) | (bits & mask);
+
+	debug("scu: v: 0x%x p: 0x%x bits: %d, mask: %d bufcfg: 0x%p\n",
+		    v, (u32)bufcfg, bits, mask, bufcfg);
+
+	ret = scu_ipc_raw_command(IPCMSG_INDIRECT_WRITE, 0, &v, 4,
+				  NULL, 0, (u32)bufcfg, 0);
+	if (ret)
+		pr_err("Failed to set mode via SCU for pin %u (%d)\n", pin, ret);
+
+	return ret;
+}
+static int mrfld_pinctrl_cfg_pin(int pin_node)
+{
+	bool is_protected;
+	int pad_offset;
+	int mode;
+	int ret;
+
+	is_protected = fdtdec_get_bool(gd->fdt_blob, pin_node, "protected");
+	if (!is_protected)
+		return -ENOTSUPP;
+
+	pad_offset = fdtdec_get_int(gd->fdt_blob, pin_node, "pad-offset", -1);
+	if (pad_offset == -1)
+		return -EINVAL;
+
+	mode = fdtdec_get_int(gd->fdt_blob, pin_node, "mode-func", -1);
+	if (mode == -1)
+		return -EINVAL;
+
+	if (mode != 1)
+		return -ENOTSUPP;
+
+	u32 mask = MRFLD_PINMODE_MASK;
+
+	ret = mrfld_pinconfig_protected(pad_offset, mask, mode);
+
+	return ret;
+}
+
+static int tangier_pinctrl_probe(struct udevice *dev)
+{
+	void *base_addr = syscon_get_first_range(X86_SYSCON_PINCONF);
+	struct mrfld_pinctrl *pinctrl = dev_get_priv(dev);
+	int pin_node;
+	int ret;
+
+	mrfld_setup_families(base_addr, mrfld_families,
+			     ARRAY_SIZE(mrfld_families));
+
+	pinctrl->families = mrfld_families;
+	pinctrl->nfamilies = ARRAY_SIZE(mrfld_families);
+
+	for (pin_node = fdt_first_subnode(gd->fdt_blob, dev_of_offset(dev));
+	     pin_node > 0;
+	     pin_node = fdt_next_subnode(gd->fdt_blob, pin_node)) {
+		ret = mrfld_pinctrl_cfg_pin(pin_node);
+		if (ret != 0) {
+			pr_err("%s: invalid configuration for the pin %d\n",
+			      __func__, pin_node);
+		}
+	}
+
+	return 0;
+}
+
+static const struct udevice_id tangier_pinctrl_match[] = {
+	{ .compatible = "intel,pinctrl-tangier", .data = X86_SYSCON_PINCONF },
+	{ /* sentinel */ }
+};
+
+U_BOOT_DRIVER(tangier_pinctrl) = {
+	.name = "tangier_pinctrl",
+	.id = UCLASS_SYSCON,
+	.of_match = tangier_pinctrl_match,
+	.probe = tangier_pinctrl_probe,
+	.priv_auto_alloc_size = sizeof(struct mrfld_pinctrl),
+};
-- 
2.7.4

[U-Boot] [PATCH 3/4] x86: dts: edison: configure I2C#6 pins

[Georgii Staroselskii]

Now that we have the pinctrl driver for Merrifield in place we can make
use of it and set I2C#6 pins appropriately.

Initial configuration came from the firmware.  Which quite likely has
been used in the phones, where that is not part of Atom peripheral, is
in use. Thus we need to override the leftover.

Signed-off-by: Georgii Staroselskii <georgii.staroselskii@emlid.com>
---
 arch/x86/dts/edison.dts | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/arch/x86/dts/edison.dts b/arch/x86/dts/edison.dts
index 1da7f54..0dd7240 100644
--- a/arch/x86/dts/edison.dts
+++ b/arch/x86/dts/edison.dts
@@ -85,4 +85,26 @@
 		compatible = "intel,reset-tangier";
 		u-boot,dm-pre-reloc;
 	};
+
+	pinctrl {
+		compatible = "intel,pinctrl-tangier";
+		reg = <0xff0c0000 0x8000>;
+
+		/*
+		 * Initial configuration came from the firmware.
+		 * Which quite likely has been used in the phones, where I2C #8,
+		 * that is not part of Atom peripheral, is in use.
+		 * Thus we need to override the leftover.
+		 */
+		i2c6_scl at 0 {
+			pad-offset = <111>;
+			mode-func = <1>;
+			protected;
+		};
+		i2c6_sda at 0 {
+			pad-offset = <112>;
+			mode-func = <1>;
+			protected;
+		};
+	};
 };
-- 
2.7.4

[U-Boot] [PATCH 4/4] x86: tangier: acpi: add I2C6 node

[Georgii Staroselskii]

Now that we have I2C#6 working, it's time to add a corresponsing
ACPI binding.

Signed-off-by: Georgii Staroselskii <georgii.staroselskii@emlid.com>
---
 arch/x86/include/asm/arch-tangier/acpi/southcluster.asl | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/x86/include/asm/arch-tangier/acpi/southcluster.asl b/arch/x86/include/asm/arch-tangier/acpi/southcluster.asl
index b200e9f..7cdc4b2 100644
--- a/arch/x86/include/asm/arch-tangier/acpi/southcluster.asl
+++ b/arch/x86/include/asm/arch-tangier/acpi/southcluster.asl
@@ -231,6 +231,16 @@ Device (PCI0)
         }
     }
 
+    Device (I2C6)
+    {
+        Name (_ADR, 0x00090001)
+
+        Method (_STA, 0, NotSerialized)
+        {
+            Return (STA_VISIBLE)
+        }
+    }
+
     Device (GPIO)
     {
         Name (_ADR, 0x000c0000)
-- 
2.7.4

[U-Boot] [PATCH 1/4] x86: cpu: introduce scu_ipc_raw_command()

[Andy Shevchenko]

On Mon, Sep 3, 2018 at 7:40 PM Georgii Staroselskii
<georgii.staroselskii@emlid.com> wrote:
>
> This interface will be used to configure properly some pins on
> Merrifield that are shared with SCU.
>
> scu_ipc_raw_command() writes SPTR and DPTR registers before sending
> a command to SCU.
>
> This code has been ported from Linux work done by Andy Shevchenko.
>

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Georgii Staroselskii <georgii.staroselskii@emlid.com>
> ---
>  arch/x86/include/asm/scu.h |  4 ++++
>  arch/x86/lib/scu.c         | 35 +++++++++++++++++++++++++++++++++++
>  2 files changed, 39 insertions(+)
>
> diff --git a/arch/x86/include/asm/scu.h b/arch/x86/include/asm/scu.h
> index 7ce5824..f5ec5a1 100644
> --- a/arch/x86/include/asm/scu.h
> +++ b/arch/x86/include/asm/scu.h
> @@ -6,6 +6,8 @@
>  #define _X86_ASM_SCU_IPC_H_
>
>  /* IPC defines the following message types */
> +#define IPCMSG_INDIRECT_READ   0x02
> +#define IPCMSG_INDIRECT_WRITE  0x05
>  #define IPCMSG_WARM_RESET      0xf0
>  #define IPCMSG_COLD_RESET      0xf1
>  #define IPCMSG_SOFT_RESET      0xf2
> @@ -23,5 +25,7 @@ struct ipc_ifwi_version {
>  /* Issue commands to the SCU with or without data */
>  int scu_ipc_simple_command(u32 cmd, u32 sub);
>  int scu_ipc_command(u32 cmd, u32 sub, u32 *in, int inlen, u32 *out, int outlen);
> +int scu_ipc_raw_command(u32 cmd, u32 sub, u32 *in, int inlen, u32 *out,
> +                       int outlen, u32 dptr, u32 sptr);
>
>  #endif /* _X86_ASM_SCU_IPC_H_ */
> diff --git a/arch/x86/lib/scu.c b/arch/x86/lib/scu.c
> index caa04c6..847bb77 100644
> --- a/arch/x86/lib/scu.c
> +++ b/arch/x86/lib/scu.c
> @@ -101,6 +101,41 @@ static int scu_ipc_cmd(struct ipc_regs *regs, u32 cmd, u32 sub,
>         return err;
>  }
>
> +int scu_ipc_raw_command(u32 cmd, u32 sub, u32 *in, int inlen, u32 *out,
> +                       int outlen, u32 dptr, u32 sptr)
> +{
> +       int inbuflen = DIV_ROUND_UP(inlen, 4);
> +       struct udevice *dev;
> +       struct scu *scu;
> +       int ret;
> +
> +       ret = syscon_get_by_driver_data(X86_SYSCON_SCU, &dev);
> +       if (ret)
> +               return ret;
> +
> +       scu = dev_get_priv(dev);
> +
> +       /* Up to 16 bytes */
> +       if (inbuflen > 4)
> +               return -EINVAL;
> +
> +       writel(dptr, &scu->regs->dptr);
> +       writel(sptr, &scu->regs->sptr);
> +
> +       /*
> +        * SRAM controller doesn't support 8-bit writes, it only
> +        * supports 32-bit writes, so we have to copy input data into
> +        * the temporary buffer, and SCU FW will use the inlen to
> +        * determine the actual input data length in the temporary
> +        * buffer.
> +        */
> +
> +       u32 inbuf[4] = {0};
> +
> +       memcpy(inbuf, in, inlen);
> +
> +       return scu_ipc_cmd(scu->regs, cmd, sub, inbuf, inlen, out, outlen);
> +}
>  /**
>   * scu_ipc_simple_command() - send a simple command
>   * @cmd: command
> --
> 2.7.4
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot



-- 
With Best Regards,
Andy Shevchenko

[U-Boot] [PATCH 3/4] x86: dts: edison: configure I2C#6 pins

[Andy Shevchenko]

On Mon, Sep 3, 2018 at 7:41 PM Georgii Staroselskii
<georgii.staroselskii@emlid.com> wrote:
>
> Now that we have the pinctrl driver for Merrifield in place we can make
> use of it and set I2C#6 pins appropriately.
>
> Initial configuration came from the firmware.  Which quite likely has
> been used in the phones, where that is not part of Atom peripheral, is
> in use. Thus we need to override the leftover.
>

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>


> Signed-off-by: Georgii Staroselskii <georgii.staroselskii@emlid.com>
> ---
>  arch/x86/dts/edison.dts | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/arch/x86/dts/edison.dts b/arch/x86/dts/edison.dts
> index 1da7f54..0dd7240 100644
> --- a/arch/x86/dts/edison.dts
> +++ b/arch/x86/dts/edison.dts
> @@ -85,4 +85,26 @@
>                 compatible = "intel,reset-tangier";
>                 u-boot,dm-pre-reloc;
>         };
> +
> +       pinctrl {
> +               compatible = "intel,pinctrl-tangier";
> +               reg = <0xff0c0000 0x8000>;
> +
> +               /*
> +                * Initial configuration came from the firmware.
> +                * Which quite likely has been used in the phones, where I2C #8,
> +                * that is not part of Atom peripheral, is in use.
> +                * Thus we need to override the leftover.
> +                */
> +               i2c6_scl at 0 {
> +                       pad-offset = <111>;
> +                       mode-func = <1>;
> +                       protected;
> +               };
> +               i2c6_sda at 0 {
> +                       pad-offset = <112>;
> +                       mode-func = <1>;
> +                       protected;
> +               };
> +       };
>  };
> --
> 2.7.4
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot



-- 
With Best Regards,
Andy Shevchenko

[U-Boot] [PATCH 4/4] x86: tangier: acpi: add I2C6 node

[Andy Shevchenko]

On Mon, Sep 3, 2018 at 7:38 PM Georgii Staroselskii
<georgii.staroselskii@emlid.com> wrote:
>
> Now that we have I2C#6 working, it's time to add a corresponsing
> ACPI binding.
>

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>


> Signed-off-by: Georgii Staroselskii <georgii.staroselskii@emlid.com>
> ---
>  arch/x86/include/asm/arch-tangier/acpi/southcluster.asl | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/arch/x86/include/asm/arch-tangier/acpi/southcluster.asl b/arch/x86/include/asm/arch-tangier/acpi/southcluster.asl
> index b200e9f..7cdc4b2 100644
> --- a/arch/x86/include/asm/arch-tangier/acpi/southcluster.asl
> +++ b/arch/x86/include/asm/arch-tangier/acpi/southcluster.asl
> @@ -231,6 +231,16 @@ Device (PCI0)
>          }
>      }
>
> +    Device (I2C6)
> +    {
> +        Name (_ADR, 0x00090001)
> +
> +        Method (_STA, 0, NotSerialized)
> +        {
> +            Return (STA_VISIBLE)
> +        }
> +    }
> +
>      Device (GPIO)
>      {
>          Name (_ADR, 0x000c0000)
> --
> 2.7.4
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot



-- 
With Best Regards,
Andy Shevchenko

[U-Boot] [PATCH 0/4] Add a pinctrl driver for Merrifield to pinmux I2C#6

[Andy Shevchenko]

On Mon, Sep 3, 2018 at 7:41 PM Georgii Staroselskii
<georgii.staroselskii@emlid.com> wrote:
>
> We have lacked the support for any pinmuxing in U-Boot for Merrifield.
> A need for pinmuxing some pins has arisen from the fact the I2C#6 is shared
> with I2C#8 on Merrifield. The latter is not easily accessible because it's
> a part of a separate MCU we don't have easy access to.
>
> I2C#6 can be and should be made use of in Linux but couldn't because it
> was blocked by the SCU.
>
> The proposed change is to implement a minimalistic pinctrl driver that
> reads the configuration off a device tree blob and configures the pins
> accordingly.
>
> The driver needs some changes to SCU API and thus the addition of
> scu_ipc_raw_command().
>
> Andy Shevchenko has been helping me by making a prior review and made
> a lot of suggestions about the general approach that should be taken.
>
> He should also be given credit for the initial kernel code that I have
> taken as a reference.
>
> The code has been tested on 5 different Edisons on Intel Edison Breakout
> board and a couple of custom Emlid PCBs by booting a kernel and issuing
> a i2cdetect -r -y 6 and then loading a driver that made use of the bus.
>
> I also created a Gist on Github with a lot of relevant information like
>
> - output of `acpidump -o tables.dat`
> - dmesg
> - output of `grep -H 15 /sys/bus/acpi/devices/*/status`
> - cat /sys/kernel/debug/gpio
> - cat /sys/kernel/debug/pinctrl/INTC1002\:00/pins
> - output of `i2cdetect -y -r 6` w/ and w/o external device on the bus
>
> Here it is:
> https://gist.github.com/staroselskii/097808e05fd609dbafd4fe5ebd618708
>

Thanks for doing this!

I'll review patches individually.
By some reason Gmail doesn't show me Simon's and Bin's emails in the
message. Are they delivered?

> Georgii Staroselskii (4):
>   x86: cpu: introduce scu_ipc_raw_command()
>   x86: tangier: pinmux: add API to configure protected pins
>   x86: dts: edison: configure I2C#6 pins
>   x86: tangier: acpi: add I2C6 node
>
>  arch/x86/cpu/tangier/Makefile                      |   2 +-
>  arch/x86/cpu/tangier/pinmux.c                      | 188 +++++++++++++++++++++
>  arch/x86/dts/edison.dts                            |  22 +++
>  .../include/asm/arch-tangier/acpi/southcluster.asl |  10 ++
>  arch/x86/include/asm/scu.h                         |   4 +
>  arch/x86/lib/scu.c                                 |  35 ++++
>  6 files changed, 260 insertions(+), 1 deletion(-)
>  create mode 100644 arch/x86/cpu/tangier/pinmux.c
>
> --
> 2.7.4
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot



-- 
With Best Regards,
Andy Shevchenko

[U-Boot] [PATCH 2/4] x86: tangier: pinmux: add API to configure protected pins

[Andy Shevchenko]

On Mon, Sep 3, 2018 at 7:39 PM Georgii Staroselskii
<georgii.staroselskii@emlid.com> wrote:
>
> This API is going to be used to configure some pins that are protected
> for simple modification.
>
> It's not a comprehensive pinctrl driver but can be turned into one
> when we need this in the future. Now it is planned to be used only
> in one place. So that's why I decided not to polute the codebase with a

polute -> pollute

> full-blown pinctrl-merrifield nobody will use.
>
> This driver reads corresponding fields in DT and configures pins
> accordingly.
>
> The "protected" flag is used to distinguish configuration of SCU-owned
> pins from the ordinary ones.
>
> The code has been adapted from Linux work done by Andy Shevchenko
> in pinctrl-merrfifield.c


> +static struct mrfld_family mrfld_families[] = {
> +       MRFLD_FAMILY(7, 101, 114),
> +};

This perhaps needs a comment that for now we are only serve I2C Family of pins.

> +static const struct mrfld_family *
> +mrfld_get_family(struct mrfld_pinctrl *mp, unsigned int pin)
> +{
> +       const struct mrfld_family *family;
> +       unsigned int i;
> +
> +       for (i = 0; i < mp->nfamilies; i++) {
> +               family = &mp->families[i];

> +               if (pin >= family->pin_base &&
> +                       pin < family->pin_base + family->npins)

Can it be one line?

> +                       return family;
> +       }


> +       printf("failed to find family for pin %u\n", pin);

I think it should be an error message (I don't remember which function
is good for that, pr_err() maybe?).

> +       return NULL;
> +}


> +       return ret;
> +}

Missed blank line here.

> +static int mrfld_pinctrl_cfg_pin(int pin_node)
> +{

> +       is_protected = fdtdec_get_bool(gd->fdt_blob, pin_node, "protected");
> +       if (!is_protected)
> +               return -ENOTSUPP;

Similar comment perhaps needed, that it's for now we support just
protected Family of pins, i.e. I2C one.

> +       if (mode != 1)
> +               return -ENOTSUPP;

I don't think we need to limit to mode 1. Rather to check for mask (if
mode is in a range 0..7).

> +       u32 mask = MRFLD_PINMODE_MASK;

I'm not sure mixing definitions with code is a good idea.

> +       ret = mrfld_pinconfig_protected(pad_offset, mask, mode);
> +
> +       return ret;
> +}
> +
> +static int tangier_pinctrl_probe(struct udevice *dev)
> +{
> +       void *base_addr = syscon_get_first_range(X86_SYSCON_PINCONF);
> +       struct mrfld_pinctrl *pinctrl = dev_get_priv(dev);
> +       int pin_node;
> +       int ret;
> +
> +       mrfld_setup_families(base_addr, mrfld_families,
> +                            ARRAY_SIZE(mrfld_families));
> +
> +       pinctrl->families = mrfld_families;
> +       pinctrl->nfamilies = ARRAY_SIZE(mrfld_families);
> +
> +       for (pin_node = fdt_first_subnode(gd->fdt_blob, dev_of_offset(dev));
> +            pin_node > 0;
> +            pin_node = fdt_next_subnode(gd->fdt_blob, pin_node)) {
> +               ret = mrfld_pinctrl_cfg_pin(pin_node);

> +               if (ret != 0) {

Simple if (ret) should work.

> +                       pr_err("%s: invalid configuration for the pin %d\n",
> +                             __func__, pin_node);
> +               }
> +       }
> +
> +       return 0;
> +}
> +

> +static const struct udevice_id tangier_pinctrl_match[] = {
> +       { .compatible = "intel,pinctrl-tangier", .data = X86_SYSCON_PINCONF },
> +       { /* sentinel */ }
> +};

Side note: I don't know for sure naming standards for compatible
strings, I guess this one is correct.

> +U_BOOT_DRIVER(tangier_pinctrl) = {
> +       .name = "tangier_pinctrl",
> +       .id = UCLASS_SYSCON,
> +       .of_match = tangier_pinctrl_match,
> +       .probe = tangier_pinctrl_probe,
> +       .priv_auto_alloc_size = sizeof(struct mrfld_pinctrl),
> +};

-- 
With Best Regards,
Andy Shevchenko

[U-Boot] [PATCH 0/4] Add a pinctrl driver for Merrifield to pinmux I2C#6

[Andy Shevchenko]

On Mon, Sep 3, 2018 at 9:26 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:

> Thanks for doing this!
>
> I'll review patches individually.
> By some reason Gmail doesn't show me Simon's and Bin's emails in the
> message. Are they delivered?

I reviewed, looks pretty much good, except few small comments in patch 2.

So, your next steps would be:
- wait for Bin and / or Simon reacting on this to see if we have
everything good with mail transport (might be just some Gmail
craziness)
- address my comments
- wait a bit (one day or so) to give time for others to look at
- address (meanwhile) comments from me and others if any
- send a new (v2) version of the series (don't forget to incorporate
tags I gave into your commit message)


--
With Best Regards,
Andy Shevchenko

[U-Boot] [PATCH 1/4] x86: cpu: introduce scu_ipc_raw_command()

[Bin Meng]

On Tue, Sep 4, 2018 at 2:23 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Mon, Sep 3, 2018 at 7:40 PM Georgii Staroselskii
> <georgii.staroselskii@emlid.com> wrote:
> >
> > This interface will be used to configure properly some pins on
> > Merrifield that are shared with SCU.
> >
> > scu_ipc_raw_command() writes SPTR and DPTR registers before sending
> > a command to SCU.
> >
> > This code has been ported from Linux work done by Andy Shevchenko.
> >
>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Somehow I did not receive the original patch email ...

Please see below

>
> > Signed-off-by: Georgii Staroselskii <georgii.staroselskii@emlid.com>
> > ---
> >  arch/x86/include/asm/scu.h |  4 ++++
> >  arch/x86/lib/scu.c         | 35 +++++++++++++++++++++++++++++++++++
> >  2 files changed, 39 insertions(+)
> >
> > diff --git a/arch/x86/include/asm/scu.h b/arch/x86/include/asm/scu.h
> > index 7ce5824..f5ec5a1 100644
> > --- a/arch/x86/include/asm/scu.h
> > +++ b/arch/x86/include/asm/scu.h
> > @@ -6,6 +6,8 @@
> >  #define _X86_ASM_SCU_IPC_H_
> >
> >  /* IPC defines the following message types */
> > +#define IPCMSG_INDIRECT_READ   0x02
> > +#define IPCMSG_INDIRECT_WRITE  0x05
> >  #define IPCMSG_WARM_RESET      0xf0
> >  #define IPCMSG_COLD_RESET      0xf1
> >  #define IPCMSG_SOFT_RESET      0xf2
> > @@ -23,5 +25,7 @@ struct ipc_ifwi_version {
> >  /* Issue commands to the SCU with or without data */
> >  int scu_ipc_simple_command(u32 cmd, u32 sub);
> >  int scu_ipc_command(u32 cmd, u32 sub, u32 *in, int inlen, u32 *out, int outlen);
> > +int scu_ipc_raw_command(u32 cmd, u32 sub, u32 *in, int inlen, u32 *out,
> > +                       int outlen, u32 dptr, u32 sptr);
> >

Can we also add the complete function header with comments that
describe the parameters and return value? I see
scu_ipc_simple_command() has one in the .c file, but scu_ipc_command()
does not. For consistency, either we document the API in the .c, or
move the comment block to the .h?

> >  #endif /* _X86_ASM_SCU_IPC_H_ */
> > diff --git a/arch/x86/lib/scu.c b/arch/x86/lib/scu.c
> > index caa04c6..847bb77 100644
> > --- a/arch/x86/lib/scu.c
> > +++ b/arch/x86/lib/scu.c
> > @@ -101,6 +101,41 @@ static int scu_ipc_cmd(struct ipc_regs *regs, u32 cmd, u32 sub,
> >         return err;
> >  }
> >
> > +int scu_ipc_raw_command(u32 cmd, u32 sub, u32 *in, int inlen, u32 *out,
> > +                       int outlen, u32 dptr, u32 sptr)
> > +{
> > +       int inbuflen = DIV_ROUND_UP(inlen, 4);
> > +       struct udevice *dev;
> > +       struct scu *scu;
> > +       int ret;
> > +
> > +       ret = syscon_get_by_driver_data(X86_SYSCON_SCU, &dev);
> > +       if (ret)
> > +               return ret;
> > +
> > +       scu = dev_get_priv(dev);
> > +
> > +       /* Up to 16 bytes */
> > +       if (inbuflen > 4)
> > +               return -EINVAL;
> > +
> > +       writel(dptr, &scu->regs->dptr);
> > +       writel(sptr, &scu->regs->sptr);
> > +

It looks like that this new API shares some common codes with existing
API scu_ipc_command(). Is it possible to do some refactoring?

> > +       /*
> > +        * SRAM controller doesn't support 8-bit writes, it only
> > +        * supports 32-bit writes, so we have to copy input data into
> > +        * the temporary buffer, and SCU FW will use the inlen to
> > +        * determine the actual input data length in the temporary
> > +        * buffer.
> > +        */
> > +
> > +       u32 inbuf[4] = {0};
> > +
> > +       memcpy(inbuf, in, inlen);
> > +
> > +       return scu_ipc_cmd(scu->regs, cmd, sub, inbuf, inlen, out, outlen);
> > +}
> >  /**
> >   * scu_ipc_simple_command() - send a simple command
> >   * @cmd: command

Regards,
Bin

[U-Boot] [PATCH 2/4] x86: tangier: pinmux: add API to configure protected pins

[Bin Meng]

Hi Georgii,

On Mon, Sep 3, 2018 at 11:07 PM Georgii Staroselskii
<georgii.staroselskii@emlid.com> wrote:
>
> This API is going to be used to configure some pins that are protected
> for simple modification.
>
> It's not a comprehensive pinctrl driver but can be turned into one
> when we need this in the future. Now it is planned to be used only
> in one place. So that's why I decided not to polute the codebase with a
> full-blown pinctrl-merrifield nobody will use.
>
> This driver reads corresponding fields in DT and configures pins
> accordingly.
>
> The "protected" flag is used to distinguish configuration of SCU-owned
> pins from the ordinary ones.
>
> The code has been adapted from Linux work done by Andy Shevchenko
> in pinctrl-merrfifield.c
>
> Signed-off-by: Georgii Staroselskii <georgii.staroselskii@emlid.com>
> ---
>  arch/x86/cpu/tangier/Makefile |   2 +-
>  arch/x86/cpu/tangier/pinmux.c | 188 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 189 insertions(+), 1 deletion(-)
>  create mode 100644 arch/x86/cpu/tangier/pinmux.c
>

Basically this looks good to me, except several styling issues below:

> diff --git a/arch/x86/cpu/tangier/Makefile b/arch/x86/cpu/tangier/Makefile
> index 8274482..68f4a32 100644
> --- a/arch/x86/cpu/tangier/Makefile
> +++ b/arch/x86/cpu/tangier/Makefile
> @@ -2,5 +2,5 @@
>  #
>  # Copyright (c) 2017 Intel Corporation
>
> -obj-y += car.o tangier.o sdram.o sysreset.o
> +obj-y += car.o tangier.o sdram.o sysreset.o pinmux.o
>  obj-$(CONFIG_GENERATE_ACPI_TABLE) += acpi.o
> diff --git a/arch/x86/cpu/tangier/pinmux.c b/arch/x86/cpu/tangier/pinmux.c
> new file mode 100644
> index 0000000..c59b63c
> --- /dev/null
> +++ b/arch/x86/cpu/tangier/pinmux.c
> @@ -0,0 +1,188 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2018 Emlid Limited
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <dm/pinctrl.h>
> +#include <fdtdec.h>
> +#include <regmap.h>
> +#include <syscon.h>
> +#include <asm/cpu.h>
> +#include <asm/scu.h>
> +#include <linux/io.h>
> +
> +#define BUFCFG_OFFSET  0x100
> +
> +#define MRFLD_FAMILY_LEN       0x400
> +
> +/* These are taken from Linux kernel */
> +#define MRFLD_PINMODE_MASK     0x07
> +
> +#define pin_to_bufno(f, p)       ((p) - (f)->pin_base)
> +
> +struct mrfld_family {
> +       unsigned int family_number;
> +       unsigned int pin_base;
> +       size_t npins;
> +       void __iomem *regs;
> +};
> +
> +#define MRFLD_FAMILY(b, s, e)                          \
> +       {                               \
> +               .family_number = (b),                   \
> +               .pin_base = (s),                        \
> +               .npins = (e) - (s) + 1,                 \
> +       }
> +
> +static struct mrfld_family mrfld_families[] = {
> +       MRFLD_FAMILY(7, 101, 114),
> +};
> +
> +struct mrfld_pinctrl {
> +       const struct mrfld_family *families;
> +       size_t nfamilies;
> +};
> +
> +static const struct mrfld_family *
> +mrfld_get_family(struct mrfld_pinctrl *mp, unsigned int pin)
> +{
> +       const struct mrfld_family *family;
> +       unsigned int i;
> +
> +       for (i = 0; i < mp->nfamilies; i++) {
> +               family = &mp->families[i];
> +               if (pin >= family->pin_base &&
> +                       pin < family->pin_base + family->npins)

CHECK: Alignment should match open parenthesis

> +                       return family;
> +       }
> +
> +       printf("failed to find family for pin %u\n", pin);
> +       return NULL;
> +}
> +
> +static void __iomem *
> +mrfld_get_bufcfg(struct mrfld_pinctrl *pinctrl, unsigned int pin)
> +{
> +       const struct mrfld_family *family;
> +       unsigned int bufno;
> +
> +       family =  mrfld_get_family(pinctrl, pin);
> +       if (!family)
> +               return NULL;
> +
> +       bufno = pin_to_bufno(family, pin);
> +
> +       return family->regs + BUFCFG_OFFSET + bufno * 4;
> +}
> +
> +static void
> +mrfld_setup_families(void *base_addr, struct mrfld_family *families, unsigned int nfam)

WARNING: line over 80 characters

> +{
> +       for (int i = 0; i < nfam; i++) {
> +               struct mrfld_family *family = &families[i];
> +
> +               family->regs = base_addr + family->family_number * MRFLD_FAMILY_LEN;

WARNING: line over 80 characters

> +       }
> +}
> +
> +static int mrfld_pinconfig_protected(unsigned int pin, u32 mask, u32 bits)
> +{
> +       struct mrfld_pinctrl *pinctrl;
> +       struct udevice *dev;
> +       void __iomem *bufcfg;
> +       u32 v, value;
> +       int ret;
> +
> +       ret = syscon_get_by_driver_data(X86_SYSCON_PINCONF, &dev);
> +       if (ret)
> +               return ret;
> +
> +       pinctrl = dev_get_priv(dev);
> +
> +       bufcfg = mrfld_get_bufcfg(pinctrl, pin);
> +       if (!bufcfg)
> +               return -EINVAL;
> +
> +       value = readl(bufcfg);
> +
> +       v = (value & ~mask) | (bits & mask);
> +
> +       debug("scu: v: 0x%x p: 0x%x bits: %d, mask: %d bufcfg: 0x%p\n",
> +                   v, (u32)bufcfg, bits, mask, bufcfg);

CHECK: Alignment should match open parenthesis

> +
> +       ret = scu_ipc_raw_command(IPCMSG_INDIRECT_WRITE, 0, &v, 4,
> +                                 NULL, 0, (u32)bufcfg, 0);
> +       if (ret)
> +               pr_err("Failed to set mode via SCU for pin %u (%d)\n", pin, ret);

WARNING: line over 80 characters

> +
> +       return ret;
> +}

nits: needs a blank line here

> +static int mrfld_pinctrl_cfg_pin(int pin_node)
> +{
> +       bool is_protected;
> +       int pad_offset;
> +       int mode;
> +       int ret;
> +
> +       is_protected = fdtdec_get_bool(gd->fdt_blob, pin_node, "protected");
> +       if (!is_protected)
> +               return -ENOTSUPP;
> +
> +       pad_offset = fdtdec_get_int(gd->fdt_blob, pin_node, "pad-offset", -1);
> +       if (pad_offset == -1)
> +               return -EINVAL;
> +
> +       mode = fdtdec_get_int(gd->fdt_blob, pin_node, "mode-func", -1);
> +       if (mode == -1)
> +               return -EINVAL;
> +
> +       if (mode != 1)
> +               return -ENOTSUPP;
> +
> +       u32 mask = MRFLD_PINMODE_MASK;
> +
> +       ret = mrfld_pinconfig_protected(pad_offset, mask, mode);
> +
> +       return ret;
> +}
> +
> +static int tangier_pinctrl_probe(struct udevice *dev)
> +{
> +       void *base_addr = syscon_get_first_range(X86_SYSCON_PINCONF);
> +       struct mrfld_pinctrl *pinctrl = dev_get_priv(dev);
> +       int pin_node;
> +       int ret;
> +
> +       mrfld_setup_families(base_addr, mrfld_families,
> +                            ARRAY_SIZE(mrfld_families));
> +
> +       pinctrl->families = mrfld_families;
> +       pinctrl->nfamilies = ARRAY_SIZE(mrfld_families);
> +
> +       for (pin_node = fdt_first_subnode(gd->fdt_blob, dev_of_offset(dev));
> +            pin_node > 0;
> +            pin_node = fdt_next_subnode(gd->fdt_blob, pin_node)) {
> +               ret = mrfld_pinctrl_cfg_pin(pin_node);
> +               if (ret != 0) {
> +                       pr_err("%s: invalid configuration for the pin %d\n",
> +                             __func__, pin_node);

CHECK: Alignment should match open parenthesis

> +               }
> +       }
> +
> +       return 0;
> +}
> +
> +static const struct udevice_id tangier_pinctrl_match[] = {
> +       { .compatible = "intel,pinctrl-tangier", .data = X86_SYSCON_PINCONF },
> +       { /* sentinel */ }
> +};
> +
> +U_BOOT_DRIVER(tangier_pinctrl) = {
> +       .name = "tangier_pinctrl",
> +       .id = UCLASS_SYSCON,
> +       .of_match = tangier_pinctrl_match,
> +       .probe = tangier_pinctrl_probe,
> +       .priv_auto_alloc_size = sizeof(struct mrfld_pinctrl),
> +};
> --

Regards,
Bin

[U-Boot] [PATCH 3/4] x86: dts: edison: configure I2C#6 pins

[Bin Meng]

On Tue, Sep 4, 2018 at 2:23 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Mon, Sep 3, 2018 at 7:41 PM Georgii Staroselskii
> <georgii.staroselskii@emlid.com> wrote:
> >
> > Now that we have the pinctrl driver for Merrifield in place we can make
> > use of it and set I2C#6 pins appropriately.
> >
> > Initial configuration came from the firmware.  Which quite likely has
> > been used in the phones, where that is not part of Atom peripheral, is
> > in use. Thus we need to override the leftover.
> >
>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>
>
> > Signed-off-by: Georgii Staroselskii <georgii.staroselskii@emlid.com>
> > ---
> >  arch/x86/dts/edison.dts | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> >

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[U-Boot] [PATCH 4/4] x86: tangier: acpi: add I2C6 node

[Bin Meng]

On Mon, Sep 3, 2018 at 11:07 PM Georgii Staroselskii
<georgii.staroselskii@emlid.com> wrote:
>
> Now that we have I2C#6 working, it's time to add a corresponsing
> ACPI binding.
>
> Signed-off-by: Georgii Staroselskii <georgii.staroselskii@emlid.com>
> ---
>  arch/x86/include/asm/arch-tangier/acpi/southcluster.asl | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[U-Boot] [PATCH 2/4] x86: tangier: pinmux: add API to configure protected pins

[Bin Meng]

On Mon, Sep 3, 2018 at 11:07 PM Georgii Staroselskii
<georgii.staroselskii@emlid.com> wrote:
>
> This API is going to be used to configure some pins that are protected
> for simple modification.
>
> It's not a comprehensive pinctrl driver but can be turned into one
> when we need this in the future. Now it is planned to be used only
> in one place. So that's why I decided not to polute the codebase with a
> full-blown pinctrl-merrifield nobody will use.
>
> This driver reads corresponding fields in DT and configures pins
> accordingly.
>
> The "protected" flag is used to distinguish configuration of SCU-owned
> pins from the ordinary ones.
>
> The code has been adapted from Linux work done by Andy Shevchenko
> in pinctrl-merrfifield.c
>
> Signed-off-by: Georgii Staroselskii <georgii.staroselskii@emlid.com>
> ---
>  arch/x86/cpu/tangier/Makefile |   2 +-
>  arch/x86/cpu/tangier/pinmux.c | 188 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 189 insertions(+), 1 deletion(-)
>  create mode 100644 arch/x86/cpu/tangier/pinmux.c
>

BTW: some more styling issues.

> diff --git a/arch/x86/cpu/tangier/Makefile b/arch/x86/cpu/tangier/Makefile
> index 8274482..68f4a32 100644
> --- a/arch/x86/cpu/tangier/Makefile
> +++ b/arch/x86/cpu/tangier/Makefile
> @@ -2,5 +2,5 @@
>  #
>  # Copyright (c) 2017 Intel Corporation
>
> -obj-y += car.o tangier.o sdram.o sysreset.o
> +obj-y += car.o tangier.o sdram.o sysreset.o pinmux.o
>  obj-$(CONFIG_GENERATE_ACPI_TABLE) += acpi.o
> diff --git a/arch/x86/cpu/tangier/pinmux.c b/arch/x86/cpu/tangier/pinmux.c
> new file mode 100644
> index 0000000..c59b63c
> --- /dev/null
> +++ b/arch/x86/cpu/tangier/pinmux.c
> @@ -0,0 +1,188 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2018 Emlid Limited
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <dm/pinctrl.h>
> +#include <fdtdec.h>
> +#include <regmap.h>
> +#include <syscon.h>
> +#include <asm/cpu.h>
> +#include <asm/scu.h>
> +#include <linux/io.h>
> +
> +#define BUFCFG_OFFSET  0x100
> +
> +#define MRFLD_FAMILY_LEN       0x400
> +
> +/* These are taken from Linux kernel */
> +#define MRFLD_PINMODE_MASK     0x07
> +
> +#define pin_to_bufno(f, p)       ((p) - (f)->pin_base)
> +

It would be good if you can make the above 4 defines the same
indention so that they all look aligned.

> +struct mrfld_family {
> +       unsigned int family_number;
> +       unsigned int pin_base;
> +       size_t npins;
> +       void __iomem *regs;
> +};
> +
> +#define MRFLD_FAMILY(b, s, e)                          \
> +       {                               \

nits: and here the ending \ is not aligned with other lines

> +               .family_number = (b),                   \
> +               .pin_base = (s),                        \
> +               .npins = (e) - (s) + 1,                 \
> +       }
> +

[snip]

Regards,
Bin

[U-Boot] [PATCH 1/4] x86: cpu: introduce scu_ipc_raw_command()

[Georgii Staroselskii]

On Tue, Sep 04, 2018 at 12:37:49PM +0800, Bin Meng wrote:
> On Tue, Sep 4, 2018 at 2:23 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Mon, Sep 3, 2018 at 7:40 PM Georgii Staroselskii
> > <georgii.staroselskii@emlid.com> wrote:
> > >
> > > This interface will be used to configure properly some pins on
> > > Merrifield that are shared with SCU.
> > >
> > > scu_ipc_raw_command() writes SPTR and DPTR registers before sending
> > > a command to SCU.
> > >
> > > This code has been ported from Linux work done by Andy Shevchenko.
> > >
> >
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
> Somehow I did not receive the original patch email ...

Sorry for that. Not very experienced in using mailing lists. I hope
you'll get this one.
> 
> Please see below
> 
> >
> > > Signed-off-by: Georgii Staroselskii <georgii.staroselskii@emlid.com>
> > > ---
> > >  arch/x86/include/asm/scu.h |  4 ++++
> > >  arch/x86/lib/scu.c         | 35 +++++++++++++++++++++++++++++++++++
> > >  2 files changed, 39 insertions(+)
> > >
> > > diff --git a/arch/x86/include/asm/scu.h b/arch/x86/include/asm/scu.h
> > > index 7ce5824..f5ec5a1 100644
> > > --- a/arch/x86/include/asm/scu.h
> > > +++ b/arch/x86/include/asm/scu.h
> > > @@ -6,6 +6,8 @@
> > >  #define _X86_ASM_SCU_IPC_H_
> > >
> > >  /* IPC defines the following message types */
> > > +#define IPCMSG_INDIRECT_READ   0x02
> > > +#define IPCMSG_INDIRECT_WRITE  0x05
> > >  #define IPCMSG_WARM_RESET      0xf0
> > >  #define IPCMSG_COLD_RESET      0xf1
> > >  #define IPCMSG_SOFT_RESET      0xf2
> > > @@ -23,5 +25,7 @@ struct ipc_ifwi_version {
> > >  /* Issue commands to the SCU with or without data */
> > >  int scu_ipc_simple_command(u32 cmd, u32 sub);
> > >  int scu_ipc_command(u32 cmd, u32 sub, u32 *in, int inlen, u32 *out, int outlen);
> > > +int scu_ipc_raw_command(u32 cmd, u32 sub, u32 *in, int inlen, u32 *out,
> > > +                       int outlen, u32 dptr, u32 sptr);
> > >
> 
> Can we also add the complete function header with comments that
> describe the parameters and return value? I see
> scu_ipc_simple_command() has one in the .c file, but scu_ipc_command()
> does not. For consistency, either we document the API in the .c, or
> move the comment block to the .h?

Sure. Will do it in the .c, if you don't mind.
> 
> > >  #endif /* _X86_ASM_SCU_IPC_H_ */
> > > diff --git a/arch/x86/lib/scu.c b/arch/x86/lib/scu.c
> > > index caa04c6..847bb77 100644
> > > --- a/arch/x86/lib/scu.c
> > > +++ b/arch/x86/lib/scu.c
> > > @@ -101,6 +101,41 @@ static int scu_ipc_cmd(struct ipc_regs *regs, u32 cmd, u32 sub,
> > >         return err;
> > >  }
> > >
> > > +int scu_ipc_raw_command(u32 cmd, u32 sub, u32 *in, int inlen, u32 *out,
> > > +                       int outlen, u32 dptr, u32 sptr)
> > > +{
> > > +       int inbuflen = DIV_ROUND_UP(inlen, 4);
> > > +       struct udevice *dev;
> > > +       struct scu *scu;
> > > +       int ret;
> > > +
> > > +       ret = syscon_get_by_driver_data(X86_SYSCON_SCU, &dev);
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       scu = dev_get_priv(dev);
> > > +
> > > +       /* Up to 16 bytes */
> > > +       if (inbuflen > 4)
> > > +               return -EINVAL;
> > > +
> > > +       writel(dptr, &scu->regs->dptr);
> > > +       writel(sptr, &scu->regs->sptr);
> > > +
> 
> It looks like that this new API shares some common codes with existing
> API scu_ipc_command(). Is it possible to do some refactoring?

These two functions seem to be so simple that a refactoring to make use
of the common code might actually make them more complex than they need
to be. So I propose to leave them be. But there's a chance I don't just
see a clear decomposition.
> 
> > > +       /*
> > > +        * SRAM controller doesn't support 8-bit writes, it only
> > > +        * supports 32-bit writes, so we have to copy input data into
> > > +        * the temporary buffer, and SCU FW will use the inlen to
> > > +        * determine the actual input data length in the temporary
> > > +        * buffer.
> > > +        */
> > > +
> > > +       u32 inbuf[4] = {0};
> > > +
> > > +       memcpy(inbuf, in, inlen);
> > > +
> > > +       return scu_ipc_cmd(scu->regs, cmd, sub, inbuf, inlen, out, outlen);
> > > +}
> > >  /**
> > >   * scu_ipc_simple_command() - send a simple command
> > >   * @cmd: command
> 
> Regards,
> Bin

Thanks for your comments. I'll send a v2 once I address everything.