[U-Boot] [PATCH] rockchip: i2c: enable I2C inside GRF for rk3066 and rk3188
In order to make I2C work on rk3066 and rk3188 boards GFR must be updated.
Signed-off-by: Alexander Kochetkov al.kochet@gmail.com --- drivers/i2c/rk_i2c.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 80 insertions(+), 5 deletions(-)
diff --git a/drivers/i2c/rk_i2c.c b/drivers/i2c/rk_i2c.c index 332280c..3ec5474 100644 --- a/drivers/i2c/rk_i2c.c +++ b/drivers/i2c/rk_i2c.c @@ -12,6 +12,7 @@ #include <dm.h> #include <errno.h> #include <i2c.h> +#include <syscon.h> #include <asm/io.h> #include <asm/arch/clock.h> #include <asm/arch/i2c.h> @@ -34,6 +35,13 @@ struct rk_i2c { unsigned int speed; };
+/** + * @grf_offset: offset inside the grf regmap for setting the i2c type + */ +struct rk_i2c_soc_data { + int grf_offset; +}; + static inline void rk_i2c_get_div(int div, int *divh, int *divl) { *divl = div / 2; @@ -381,9 +389,37 @@ static int rockchip_i2c_ofdata_to_platdata(struct udevice *bus) static int rockchip_i2c_probe(struct udevice *bus) { struct rk_i2c *priv = dev_get_priv(bus); + struct rk_i2c_soc_data *soc_data; + uint32_t value; + int bus_nr; + void *grf; + int ret;
priv->regs = dev_read_addr_ptr(bus);
+ soc_data = (struct rk_i2c_soc_data*)dev_get_driver_data(bus); + + if (soc_data->grf_offset >= 0) { + grf = syscon_get_first_range(ROCKCHIP_SYSCON_GRF); + if (IS_ERR(grf)) { + ret = PTR_ERR(grf); + debug("%s: Could not get syscon for %s: %d\n", + __func__, bus->name, ret); + return ret; + } + + ret = dev_read_alias_seq(bus, &bus_nr); + if (ret < 0) { + debug("%s: Could not get alias for %s: %d\n", + __func__, bus->name, ret); + return ret; + } + + /* 27+i: write mask, 11+i: value */ + value = BIT(27 + bus_nr) | BIT(11 + bus_nr); + writel(value, grf + soc_data->grf_offset); + } + return 0; }
@@ -392,12 +428,51 @@ static const struct dm_i2c_ops rockchip_i2c_ops = { .set_bus_speed = rockchip_i2c_set_bus_speed, };
+static const struct rk_i2c_soc_data rk3066_soc_data = { + .grf_offset = 0x154, +}; + +static const struct rk_i2c_soc_data rk3188_soc_data = { + .grf_offset = 0x0a4, +}; + +static const struct rk_i2c_soc_data rk3228_soc_data = { + .grf_offset = -1, +}; + +static const struct rk_i2c_soc_data rk3288_soc_data = { + .grf_offset = -1, +}; + +static const struct rk_i2c_soc_data rk3328_soc_data = { + .grf_offset = -1, +}; + +static const struct rk_i2c_soc_data rk3399_soc_data = { + .grf_offset = -1, +}; + static const struct udevice_id rockchip_i2c_ids[] = { - { .compatible = "rockchip,rk3066-i2c" }, - { .compatible = "rockchip,rk3188-i2c" }, - { .compatible = "rockchip,rk3288-i2c" }, - { .compatible = "rockchip,rk3328-i2c" }, - { .compatible = "rockchip,rk3399-i2c" }, + { + .compatible = "rockchip,rk3066-i2c", + .data = (ulong)&rk3066_soc_data, + }, + { + .compatible = "rockchip,rk3188-i2c", + .data = (ulong)&rk3188_soc_data, + }, + { + .compatible = "rockchip,rk3288-i2c", + .data = (ulong)&rk3288_soc_data, + }, + { + .compatible = "rockchip,rk3328-i2c", + .data = (ulong)&rk3328_soc_data, + }, + { + .compatible = "rockchip,rk3399-i2c", + .data = (ulong)&rk3399_soc_data, + }, { } };
On 20 Feb 2018, at 09:43, Alexander Kochetkov al.kochet@gmail.com wrote:
In order to make I2C work on rk3066 and rk3188 boards GFR must be updated.
The commit message does not really tell me what the change is… …see below for a couple questions.
Signed-off-by: Alexander Kochetkov al.kochet@gmail.com
Acked-by: Philipp Tomsich philipp.tomsich@theobroma-systems.com
drivers/i2c/rk_i2c.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 80 insertions(+), 5 deletions(-)
diff --git a/drivers/i2c/rk_i2c.c b/drivers/i2c/rk_i2c.c index 332280c..3ec5474 100644 --- a/drivers/i2c/rk_i2c.c +++ b/drivers/i2c/rk_i2c.c @@ -12,6 +12,7 @@ #include <dm.h> #include <errno.h> #include <i2c.h> +#include <syscon.h> #include <asm/io.h> #include <asm/arch/clock.h> #include <asm/arch/i2c.h> @@ -34,6 +35,13 @@ struct rk_i2c { unsigned int speed; };
+/**
- @grf_offset: offset inside the grf regmap for setting the i2c type
- */
+struct rk_i2c_soc_data {
- int grf_offset;
+};
static inline void rk_i2c_get_div(int div, int *divh, int *divl) { *divl = div / 2; @@ -381,9 +389,37 @@ static int rockchip_i2c_ofdata_to_platdata(struct udevice *bus) static int rockchip_i2c_probe(struct udevice *bus) { struct rk_i2c *priv = dev_get_priv(bus);
struct rk_i2c_soc_data *soc_data;
uint32_t value;
int bus_nr;
void *grf;
int ret;
priv->regs = dev_read_addr_ptr(bus);
soc_data = (struct rk_i2c_soc_data*)dev_get_driver_data(bus);
if (soc_data->grf_offset >= 0) {
grf = syscon_get_first_range(ROCKCHIP_SYSCON_GRF);if (IS_ERR(grf)) {ret = PTR_ERR(grf);debug("%s: Could not get syscon for %s: %d\n",__func__, bus->name, ret);return ret;}ret = dev_read_alias_seq(bus, &bus_nr);if (ret < 0) {debug("%s: Could not get alias for %s: %d\n",__func__, bus->name, ret);return ret;}/* 27+i: write mask, 11+i: value */value = BIT(27 + bus_nr) | BIT(11 + bus_nr);
What exactly are you trying to configure here?
Do you need to bring these out of reset, is this some IO config or are these clock gates? Note that if it’s any of these, then the respective drivers (i.e. reset, pinctrl, clock) should be modified instead of putting this here.
writel(value, grf + soc_data->grf_offset);
This should be a rk_setreg(…) call.
- }
- return 0;
}
@@ -392,12 +428,51 @@ static const struct dm_i2c_ops rockchip_i2c_ops = { .set_bus_speed = rockchip_i2c_set_bus_speed, };
+static const struct rk_i2c_soc_data rk3066_soc_data = {
- .grf_offset = 0x154,
Please don’t use open-coded constants for something like this.
+};
+static const struct rk_i2c_soc_data rk3188_soc_data = {
- .grf_offset = 0x0a4,
+};
+static const struct rk_i2c_soc_data rk3228_soc_data = {
- .grf_offset = -1,
+};
+static const struct rk_i2c_soc_data rk3288_soc_data = {
- .grf_offset = -1,
+};
+static const struct rk_i2c_soc_data rk3328_soc_data = {
- .grf_offset = -1,
+};
+static const struct rk_i2c_soc_data rk3399_soc_data = {
- .grf_offset = -1,
+};
static const struct udevice_id rockchip_i2c_ids[] = {
- { .compatible = "rockchip,rk3066-i2c" },
- { .compatible = "rockchip,rk3188-i2c" },
- { .compatible = "rockchip,rk3288-i2c" },
- { .compatible = "rockchip,rk3328-i2c" },
- { .compatible = "rockchip,rk3399-i2c" },
- {
.compatible = "rockchip,rk3066-i2c",.data = (ulong)&rk3066_soc_data,- },
- {
.compatible = "rockchip,rk3188-i2c",.data = (ulong)&rk3188_soc_data,- },
- {
.compatible = "rockchip,rk3288-i2c",.data = (ulong)&rk3288_soc_data,- },
- {
.compatible = "rockchip,rk3328-i2c",.data = (ulong)&rk3328_soc_data,- },
- {
.compatible = "rockchip,rk3399-i2c",.data = (ulong)&rk3399_soc_data,- }, { }
};
-- 1.7.9.5
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Hello, Philipp!
What exactly are you trying to configure here?
Do you need to bring these out of reset, is this some IO config or are these clock gates? Note that if it’s any of these, then the respective drivers (i.e. reset, pinctrl, clock) should be modified instead of putting this here.
This is rki2c_sel bit used to switch I2C IP between different implementations. Here comment from TRM: "1: rki2c is used instead of old i2c». U-boot and kernel I2C drivers want work with legacy IP implementation.
The patch is backport from upstream linux and the code was located inside I2C driver code.
+static const struct rk_i2c_soc_data rk3066_soc_data = {
- .grf_offset = 0x154,
Please don’t use open-coded constants for something like this.
This is GRF_SON_CON1 register. A lot of rockchip drivers inside linux kernel use following technic to make drivers universal between different chips. Logically it is not far away from simple constant definition using define syntax. I don’t use that constant inside functions directly.
I tried to replace constant with following code:
#include <asm/arch/grf_rk3036.h> #include <asm/arch/grf_rk3188.h>
...
static const struct rk_i2c_soc_data rk3066_soc_data = { .grf_offset = offsetof(struct rk3036_grf, soc_con1), };
static const struct rk_i2c_soc_data rk3188_soc_data = { .grf_offset = offsetof(struct rk3188_grf, soc_con1), };
But the code wan’t compile, because grf_rk3036.h and grf_rk3188.h both define a lot of constants with same name and that produce a lot of build errors.
What about if I just add comment to grf_offset definintion something like that? That way it could be ease found using text search.
static const struct rk_i2c_soc_data rk3066_soc_data = { .grf_offset = 0x154, // offsetof(struct rk3036_grf, soc_con1), };
static const struct rk_i2c_soc_data rk3188_soc_data = { .grf_offset = 0x0a4, // offsetof(struct rk3188_grf, soc_con1), };
Another solution I can do is following. I declare RK3066_GRF_SOC_CON1 and RK3188_GFR_SOC_CON1 constants at top of i2c driver and use them.
static const struct rk_i2c_soc_data rk3066_soc_data = { .grf_offset = RK3066_GRF_SOC_CON1, };
static const struct rk_i2c_soc_data rk3188_soc_data = { .grf_offset = RK3188_GRF_SOC_CON1, };
What do you think? How to do better?
Regards, Alexander.
Alexander,
On 26 Feb 2018, at 10:25, Alexander Kochetkov al.kochet@gmail.com wrote:
Hello, Philipp!
What exactly are you trying to configure here?
Do you need to bring these out of reset, is this some IO config or are these clock gates? Note that if it’s any of these, then the respective drivers (i.e. reset, pinctrl, clock) should be modified instead of putting this here.
This is rki2c_sel bit used to switch I2C IP between different implementations. Here comment from TRM: "1: rki2c is used instead of old i2c». U-boot and kernel I2C drivers want work with legacy IP implementation.
Given that I would have never guessed: this clearly needs comments and symbolic constants then ;-)
The patch is backport from upstream linux and the code was located inside I2C driver code.
My gut feeling is that this shouldn’t go into the i2c driver (after all, the device is not a ‘rki2c’ until this bit is set). However, if we want it in the i2c driver, we should have an offset-of for the GRF offset and also make the bit-positions configurable via the driver-data.
From your description this sounds like a (secondary) reset-bit for the i2c controller. With a *-u-boot.dtsi, you could even model the GRF-offset and the bit-number via the device tree. The only drawback from this is that resets are (not yet) automatically processed when probing the device in the driver model core implementation.
+static const struct rk_i2c_soc_data rk3066_soc_data = {
- .grf_offset = 0x154,
Please don’t use open-coded constants for something like this.
This is GRF_SON_CON1 register. A lot of rockchip drivers inside linux kernel use following technic to make drivers universal between different chips. Logically it is not far away from simple constant definition using define syntax. I don’t use that constant inside functions directly.
I tried to replace constant with following code:
#include <asm/arch/grf_rk3036.h> #include <asm/arch/grf_rk3188.h>
...
static const struct rk_i2c_soc_data rk3066_soc_data = { .grf_offset = offsetof(struct rk3036_grf, soc_con1), };
static const struct rk_i2c_soc_data rk3188_soc_data = { .grf_offset = offsetof(struct rk3188_grf, soc_con1), };
But the code wan’t compile, because grf_rk3036.h and grf_rk3188.h both define a lot of constants with same name and that produce a lot of build errors.
Yes, that you be preferable.
However, the GRF drivers first need to be cleaned up (i.e. the enums for the IOMUX definitions need to move into the pinctrl drivers), so only the structure definitions remain in the GRF drivers.
Some of this has happened lately for some of the more recent chips, but there hasn’t been a larger effort to do it: e.g. commit 301fff4e574d373a139dd47aceabc5b4259873da for the RK3328
It would be great if you’d pick this task up for the 3036 and 3188.
What about if I just add comment to grf_offset definintion something like that? That way it could be ease found using text search.
static const struct rk_i2c_soc_data rk3066_soc_data = { .grf_offset = 0x154, // offsetof(struct rk3036_grf, soc_con1), };
static const struct rk_i2c_soc_data rk3188_soc_data = { .grf_offset = 0x0a4, // offsetof(struct rk3188_grf, soc_con1), };
Another solution I can do is following. I declare RK3066_GRF_SOC_CON1 and RK3188_GFR_SOC_CON1 constants at top of i2c driver and use them.
static const struct rk_i2c_soc_data rk3066_soc_data = { .grf_offset = RK3066_GRF_SOC_CON1, };
static const struct rk_i2c_soc_data rk3188_soc_data = { .grf_offset = RK3188_GRF_SOC_CON1, };
What do you think? How to do better?
Regards, Alexander.
Simon,
please take a look and advise whether you want this as a reset-driver and have us adding auto-processing of resets to the initial bind/probe/init cycle.
Thanks, Philipp.
On 26 Feb 2018, at 10:43, Dr. Philipp Tomsich philipp.tomsich@theobroma-systems.com wrote:
Alexander,
On 26 Feb 2018, at 10:25, Alexander Kochetkov al.kochet@gmail.com wrote:
Hello, Philipp!
What exactly are you trying to configure here?
Do you need to bring these out of reset, is this some IO config or are these clock gates? Note that if it’s any of these, then the respective drivers (i.e. reset, pinctrl, clock) should be modified instead of putting this here.
This is rki2c_sel bit used to switch I2C IP between different implementations. Here comment from TRM: "1: rki2c is used instead of old i2c». U-boot and kernel I2C drivers want work with legacy IP implementation.
Given that I would have never guessed: this clearly needs comments and symbolic constants then ;-)
The patch is backport from upstream linux and the code was located inside I2C driver code.
My gut feeling is that this shouldn’t go into the i2c driver (after all, the device is not a ‘rki2c’ until this bit is set). However, if we want it in the i2c driver, we should have an offset-of for the GRF offset and also make the bit-positions configurable via the driver-data.
From your description this sounds like a (secondary) reset-bit for the i2c controller. With a *-u-boot.dtsi, you could even model the GRF-offset and the bit-number via the device tree. The only drawback from this is that resets are (not yet) automatically processed when probing the device in the driver model core implementation.
+static const struct rk_i2c_soc_data rk3066_soc_data = {
- .grf_offset = 0x154,
Please don’t use open-coded constants for something like this.
This is GRF_SON_CON1 register. A lot of rockchip drivers inside linux kernel use following technic to make drivers universal between different chips. Logically it is not far away from simple constant definition using define syntax. I don’t use that constant inside functions directly.
I tried to replace constant with following code:
#include <asm/arch/grf_rk3036.h> #include <asm/arch/grf_rk3188.h>
...
static const struct rk_i2c_soc_data rk3066_soc_data = { .grf_offset = offsetof(struct rk3036_grf, soc_con1), };
static const struct rk_i2c_soc_data rk3188_soc_data = { .grf_offset = offsetof(struct rk3188_grf, soc_con1), };
But the code wan’t compile, because grf_rk3036.h and grf_rk3188.h both define a lot of constants with same name and that produce a lot of build errors.
Yes, that you be preferable.
However, the GRF drivers first need to be cleaned up (i.e. the enums for the IOMUX definitions need to move into the pinctrl drivers), so only the structure definitions remain in the GRF drivers.
Some of this has happened lately for some of the more recent chips, but there hasn’t been a larger effort to do it: e.g. commit 301fff4e574d373a139dd47aceabc5b4259873da for the RK3328
It would be great if you’d pick this task up for the 3036 and 3188.
What about if I just add comment to grf_offset definintion something like that? That way it could be ease found using text search.
static const struct rk_i2c_soc_data rk3066_soc_data = { .grf_offset = 0x154, // offsetof(struct rk3036_grf, soc_con1), };
static const struct rk_i2c_soc_data rk3188_soc_data = { .grf_offset = 0x0a4, // offsetof(struct rk3188_grf, soc_con1), };
Another solution I can do is following. I declare RK3066_GRF_SOC_CON1 and RK3188_GFR_SOC_CON1 constants at top of i2c driver and use them.
static const struct rk_i2c_soc_data rk3066_soc_data = { .grf_offset = RK3066_GRF_SOC_CON1, };
static const struct rk_i2c_soc_data rk3188_soc_data = { .grf_offset = RK3188_GRF_SOC_CON1, };
What do you think? How to do better?
Regards, Alexander.
Simon,
please take a look and advise whether you want this as a reset-driver and have us adding auto-processing of resets to the initial bind/probe/init cycle.
Thanks, Philipp.
On 26 Feb 2018, at 10:43, Dr. Philipp Tomsich philipp.tomsich@theobroma-systems.com wrote:
Alexander,
On 26 Feb 2018, at 10:25, Alexander Kochetkov al.kochet@gmail.com wrote:
Hello, Philipp!
What exactly are you trying to configure here?
Do you need to bring these out of reset, is this some IO config or are these clock gates? Note that if it’s any of these, then the respective drivers (i.e. reset, pinctrl, clock) should be modified instead of putting this here.
This is rki2c_sel bit used to switch I2C IP between different implementations. Here comment from TRM: "1: rki2c is used instead of old i2c». U-boot and kernel I2C drivers want work with legacy IP implementation.
Given that I would have never guessed: this clearly needs comments and symbolic constants then ;-)
The patch is backport from upstream linux and the code was located inside I2C driver code.
My gut feeling is that this shouldn’t go into the i2c driver (after all, the device is not a ‘rki2c’ until this bit is set). However, if we want it in the i2c driver, we should have an offset-of for the GRF offset and also make the bit-positions configurable via the driver-data.
From your description this sounds like a (secondary) reset-bit for the i2c controller. With a *-u-boot.dtsi, you could even model the GRF-offset and the bit-number via the device tree. The only drawback from this is that resets are (not yet) automatically processed when probing the device in the driver model core implementation.
+static const struct rk_i2c_soc_data rk3066_soc_data = {
- .grf_offset = 0x154,
Please don’t use open-coded constants for something like this.
This is GRF_SON_CON1 register. A lot of rockchip drivers inside linux kernel use following technic to make drivers universal between different chips. Logically it is not far away from simple constant definition using define syntax. I don’t use that constant inside functions directly.
I tried to replace constant with following code:
#include <asm/arch/grf_rk3036.h> #include <asm/arch/grf_rk3188.h>
...
static const struct rk_i2c_soc_data rk3066_soc_data = { .grf_offset = offsetof(struct rk3036_grf, soc_con1), };
static const struct rk_i2c_soc_data rk3188_soc_data = { .grf_offset = offsetof(struct rk3188_grf, soc_con1), };
But the code wan’t compile, because grf_rk3036.h and grf_rk3188.h both define a lot of constants with same name and that produce a lot of build errors.
Yes, that you be preferable.
However, the GRF drivers first need to be cleaned up (i.e. the enums for the IOMUX definitions need to move into the pinctrl drivers), so only the structure definitions remain in the GRF drivers.
Some of this has happened lately for some of the more recent chips, but there hasn’t been a larger effort to do it: e.g. commit 301fff4e574d373a139dd47aceabc5b4259873da for the RK3328
It would be great if you’d pick this task up for the 3036 and 3188.
What about if I just add comment to grf_offset definintion something like that? That way it could be ease found using text search.
static const struct rk_i2c_soc_data rk3066_soc_data = { .grf_offset = 0x154, // offsetof(struct rk3036_grf, soc_con1), };
static const struct rk_i2c_soc_data rk3188_soc_data = { .grf_offset = 0x0a4, // offsetof(struct rk3188_grf, soc_con1), };
Another solution I can do is following. I declare RK3066_GRF_SOC_CON1 and RK3188_GFR_SOC_CON1 constants at top of i2c driver and use them.
static const struct rk_i2c_soc_data rk3066_soc_data = { .grf_offset = RK3066_GRF_SOC_CON1, };
static const struct rk_i2c_soc_data rk3188_soc_data = { .grf_offset = RK3188_GRF_SOC_CON1, };
What do you think? How to do better?
Regards, Alexander.
26 февр. 2018 г., в 12:43, Dr. Philipp Tomsich philipp.tomsich@theobroma-systems.com написал(а):
However, the GRF drivers first need to be cleaned up (i.e. the enums for the IOMUX definitions need to move into the pinctrl drivers), so only the structure definitions remain in the GRF drivers.
Some of this has happened lately for some of the more recent chips, but there hasn’t been a larger effort to do it: e.g. commit 301fff4e574d373a139dd47aceabc5b4259873da for the RK3328
It would be great if you’d pick this task up for the 3036 and 3188.
I'll do that.
One more note. grf_rk3188.h also contain GRF_SOC_CON bit definitions. They also have to renamed for consistency and to avoid conflicts. For example, RKI2C0_SEL_SHIFT to RK3188_RKI2C0_SEL_SHIFT.
grf_rk3036.h doesn’t contain GRF_SOC_CON bit definition at all. And I don’t have rk3036 based board. I can find TRM for rk3036 and make definitions, but I can’t be shoure they will be valid. But RKI2Cx_SEL_SHIFT is the same for both chips.
From your description this sounds like a (secondary) reset-bit for the i2c controller. With a *-u-boot.dtsi, you could even model the GRF-offset and the bit-number via the device tree. The only drawback from this is that resets are (not yet) automatically processed when probing the device in the driver model core implementation.
Actually it is impossible to reset I2C using that pin. So it is not reset logically. And DT should be consistent between linux and u-boot, so adding reset node to u-boot DT also require to add DT node to linux.
I think, It’s better to hide that thing inside driver and forget about it.
Regards, Alexander.
This one patch not needed. Just found, that the code already implemented in the pinctrl_rk3188_i2c_config(). It’s mystery for me why pinctrl is not called for i2c DT nodes automatically. If I do explicit call "ret = pinctrl_request_noflags(pinctrl, PERIPH_ID_I2C0);» all will work.
Is this configuration feature or something I should fix in code?
Regards, Alexander.
participants (2)
-
Alexander Kochetkov -
Dr. Philipp Tomsich