[U-Boot] [PATCH 0/4] Add a pinctrl driver for Merrifield to pinmux I2C#6
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
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
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@lists.denx.de https://lists.denx.de/listinfo/u-boot
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
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.
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), +};
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),+};
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
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
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@0 { + pad-offset = <111>; + mode-func = <1>; + protected; + }; + i2c6_sda@0 { + pad-offset = <112>; + mode-func = <1>; + protected; + }; + }; };
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@0 {pad-offset = <111>;mode-func = <1>;protected;};i2c6_sda@0 {pad-offset = <112>;mode-func = <1>;protected;};};};
2.7.4
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
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
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)
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@lists.denx.de https://lists.denx.de/listinfo/u-boot
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
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@lists.denx.de https://lists.denx.de/listinfo/u-boot
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
participants (3)
-
Andy Shevchenko -
Bin Meng -
Georgii Staroselskii