[U-Boot] [PATCH 1/2] i2c: mvtwsi: disable i2c slave on Armada 38x
Equivalent code that disables the hidden i2c0 slave already exists in the Turris Omnia platform specific code. But this hidden i2c0 slave that interferes the i2c bus is not board specific. Armada 38x SoCs and at least some Kirkwood variants are affected as well. Add code to disable this slave to the i2c bus driver to make it work on all affected hardware.
Use the bind callback because we want this to always run at boot, regardless of whether U-Boot uses the i2c bus.
Cc: Rabeeh Khoury rabeeh@solid-run.com Cc: Chris Packham judge.packham@gmail.com Reviewed-by: Stefan Roese sr@denx.de Reviewed-by: Heiko Schocher hs@denx.de Signed-off-by: Baruch Siach baruch@tkos.co.il --- v2: * Use clrbits_le32 (Stefan Roese)
* Apply to Kirkwood (Chris Packham)
* Add review tags from Stefan and Heiko --- drivers/i2c/mvtwsi.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/mvtwsi.c b/drivers/i2c/mvtwsi.c index f9822e56b894..c0d9a71627db 100644 --- a/drivers/i2c/mvtwsi.c +++ b/drivers/i2c/mvtwsi.c @@ -11,6 +11,7 @@ #include <i2c.h> #include <linux/errno.h> #include <asm/io.h> +#include <linux/bitops.h> #include <linux/compat.h> #ifdef CONFIG_DM_I2C #include <dm.h> @@ -70,8 +71,10 @@ struct mvtwsi_registers { u32 baudrate; /* When writing */ }; u32 xtnd_slave_addr; - u32 reserved[2]; + u32 reserved0[2]; u32 soft_reset; + u32 reserved1[27]; + u32 debug; };
#endif @@ -795,6 +798,23 @@ static int mvtwsi_i2c_ofdata_to_platdata(struct udevice *bus) return 0; }
+static void twsi_disable_i2c_slave(struct mvtwsi_registers *twsi) +{ + clrbits_le32(&twsi->debug, BIT(18)); +} + +static int mvtwsi_i2c_bind(struct udevice *bus) +{ + struct mvtwsi_registers *twsi = devfdt_get_addr_ptr(bus); + + /* Disable the hidden slave in i2c0 of these platforms */ + if ((IS_ENABLED(CONFIG_ARMADA_38X) || IS_ENABLED(CONFIG_KIRKWOOD)) + && bus->req_seq == 0) + twsi_disable_i2c_slave(twsi); + + return 0; +} + static int mvtwsi_i2c_probe(struct udevice *bus) { struct mvtwsi_i2c_dev *dev = dev_get_priv(bus); @@ -850,6 +870,7 @@ U_BOOT_DRIVER(i2c_mvtwsi) = { .name = "i2c_mvtwsi", .id = UCLASS_I2C, .of_match = mvtwsi_i2c_ids, + .bind = mvtwsi_i2c_bind, .probe = mvtwsi_i2c_probe, .ofdata_to_platdata = mvtwsi_i2c_ofdata_to_platdata, .priv_auto_alloc_size = sizeof(struct mvtwsi_i2c_dev),
Code that disables the i2c slave is now in the mvtwsi i2c driver. Platform must enable DM_I2C to use that code. Add a comment in the code as a reminder for the planned DM_I2C migration of Turris Omnia.
Reviewed-by: Heiko Schocher hs@denx.de Signed-off-by: Baruch Siach baruch@tkos.co.il --- v2: Add Reviewed-by from Heiko --- board/CZ.NIC/turris_omnia/turris_omnia.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/board/CZ.NIC/turris_omnia/turris_omnia.c b/board/CZ.NIC/turris_omnia/turris_omnia.c index da663cf1bb0c..160d30cd795a 100644 --- a/board/CZ.NIC/turris_omnia/turris_omnia.c +++ b/board/CZ.NIC/turris_omnia/turris_omnia.c @@ -321,7 +321,11 @@ int board_early_init_f(void) writel(OMNIA_GPP_OUT_ENA_LOW, MVEBU_GPIO0_BASE + 0x04); writel(OMNIA_GPP_OUT_ENA_MID, MVEBU_GPIO1_BASE + 0x04);
- /* Disable I2C debug mode blocking 0x64 I2C address */ + /* + * Disable I2C debug mode blocking 0x64 I2C address. + * Note: that would be redundant once Turris Omnia migrates to DM_I2C, + * because the mvtwsi driver includes equivalent code. + */ i2c_debug_reg = readl(MVEBU_TWSI_BASE + MVTWSI_ARMADA_DEBUG_REG); i2c_debug_reg &= ~(1<<18); writel(i2c_debug_reg, MVEBU_TWSI_BASE + MVTWSI_ARMADA_DEBUG_REG);
On 29.05.2018 06:45, Baruch Siach wrote:
Equivalent code that disables the hidden i2c0 slave already exists in the Turris Omnia platform specific code. But this hidden i2c0 slave that interferes the i2c bus is not board specific. Armada 38x SoCs and at least some Kirkwood variants are affected as well. Add code to disable this slave to the i2c bus driver to make it work on all affected hardware.
Use the bind callback because we want this to always run at boot, regardless of whether U-Boot uses the i2c bus.
Cc: Rabeeh Khoury rabeeh@solid-run.com Cc: Chris Packham judge.packham@gmail.com Reviewed-by: Stefan Roese sr@denx.de Reviewed-by: Heiko Schocher hs@denx.de Signed-off-by: Baruch Siach baruch@tkos.co.il
v2:
Use clrbits_le32 (Stefan Roese)
Apply to Kirkwood (Chris Packham)
Add review tags from Stefan and Heiko
Please add the patch revision also to the patch subject next time:
[PATCH 1/2 v2] i2c: mvtwsi: disable i2c slave on Armada 38x
One more comment below...
drivers/i2c/mvtwsi.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/mvtwsi.c b/drivers/i2c/mvtwsi.c index f9822e56b894..c0d9a71627db 100644 --- a/drivers/i2c/mvtwsi.c +++ b/drivers/i2c/mvtwsi.c @@ -11,6 +11,7 @@ #include <i2c.h> #include <linux/errno.h> #include <asm/io.h> +#include <linux/bitops.h> #include <linux/compat.h> #ifdef CONFIG_DM_I2C #include <dm.h> @@ -70,8 +71,10 @@ struct mvtwsi_registers { u32 baudrate; /* When writing */ }; u32 xtnd_slave_addr;
- u32 reserved[2];
u32 reserved0[2]; u32 soft_reset;
u32 reserved1[27];
u32 debug; };
#endif
@@ -795,6 +798,23 @@ static int mvtwsi_i2c_ofdata_to_platdata(struct udevice *bus) return 0; }
+static void twsi_disable_i2c_slave(struct mvtwsi_registers *twsi) +{
- clrbits_le32(&twsi->debug, BIT(18));
+}
+static int mvtwsi_i2c_bind(struct udevice *bus) +{
- struct mvtwsi_registers *twsi = devfdt_get_addr_ptr(bus);
- /* Disable the hidden slave in i2c0 of these platforms */
- if ((IS_ENABLED(CONFIG_ARMADA_38X) || IS_ENABLED(CONFIG_KIRKWOOD))
We could better use the compatible check here:
if (device_is_compatible(dev, "marvell,mv64xxx-i2c"))
&& bus->req_seq == 0)twsi_disable_i2c_slave(twsi);- return 0;
+}
- static int mvtwsi_i2c_probe(struct udevice *bus) { struct mvtwsi_i2c_dev *dev = dev_get_priv(bus);
@@ -850,6 +870,7 @@ U_BOOT_DRIVER(i2c_mvtwsi) = { .name = "i2c_mvtwsi", .id = UCLASS_I2C, .of_match = mvtwsi_i2c_ids,
- .bind = mvtwsi_i2c_bind, .probe = mvtwsi_i2c_probe, .ofdata_to_platdata = mvtwsi_i2c_ofdata_to_platdata, .priv_auto_alloc_size = sizeof(struct mvtwsi_i2c_dev),
Thanks, Stefan
Hi Stefan,
On Tue, May 29, 2018 at 08:11:55AM +0200, Stefan Roese wrote:
On 29.05.2018 06:45, Baruch Siach wrote:
Equivalent code that disables the hidden i2c0 slave already exists in the Turris Omnia platform specific code. But this hidden i2c0 slave that interferes the i2c bus is not board specific. Armada 38x SoCs and at least some Kirkwood variants are affected as well. Add code to disable this slave to the i2c bus driver to make it work on all affected hardware.
Use the bind callback because we want this to always run at boot, regardless of whether U-Boot uses the i2c bus.
Cc: Rabeeh Khoury rabeeh@solid-run.com Cc: Chris Packham judge.packham@gmail.com Reviewed-by: Stefan Roese sr@denx.de Reviewed-by: Heiko Schocher hs@denx.de Signed-off-by: Baruch Siach baruch@tkos.co.il
[snip]
+static int mvtwsi_i2c_bind(struct udevice *bus) +{
- struct mvtwsi_registers *twsi = devfdt_get_addr_ptr(bus);
- /* Disable the hidden slave in i2c0 of these platforms */
- if ((IS_ENABLED(CONFIG_ARMADA_38X) || IS_ENABLED(CONFIG_KIRKWOOD))
We could better use the compatible check here:
if (device_is_compatible(dev, "marvell,mv64xxx-i2c"))
This is not an equivalent check. marvell,mv64xxx-i2c covers other SoCs that might not be affected.
Furthermore, this makes a build time test into a run time one. This bloats the code for platforms like Allwinner that are unlikely to be affected.
What is the advantage of device_is_compatible()? Is it feasible to build a multi-platform U-Boot image?
&& bus->req_seq == 0)twsi_disable_i2c_slave(twsi);- return 0;
+}
baruch
On Wed, May 30, 2018 at 5:55 AM Baruch Siach baruch@tkos.co.il wrote:
Hi Stefan,
On Tue, May 29, 2018 at 08:11:55AM +0200, Stefan Roese wrote:
On 29.05.2018 06:45, Baruch Siach wrote:
Equivalent code that disables the hidden i2c0 slave already exists in the Turris Omnia platform specific code. But this hidden i2c0 slave
that
interferes the i2c bus is not board specific. Armada 38x SoCs and at least some Kirkwood variants are affected as well. Add code to disable this slave to the i2c bus driver to make it work on all affected hardware.
Use the bind callback because we want this to always run at boot, regardless of whether U-Boot uses the i2c bus.
Cc: Rabeeh Khoury rabeeh@solid-run.com Cc: Chris Packham judge.packham@gmail.com Reviewed-by: Stefan Roese sr@denx.de Reviewed-by: Heiko Schocher hs@denx.de Signed-off-by: Baruch Siach baruch@tkos.co.il
[snip]
+static int mvtwsi_i2c_bind(struct udevice *bus) +{
- struct mvtwsi_registers *twsi = devfdt_get_addr_ptr(bus);
- /* Disable the hidden slave in i2c0 of these platforms */
- if ((IS_ENABLED(CONFIG_ARMADA_38X) || IS_ENABLED(CONFIG_KIRKWOOD))
We could better use the compatible check here:
if (device_is_compatible(dev, "marvell,mv64xxx-i2c"))
This is not an equivalent check. marvell,mv64xxx-i2c covers other SoCs
that
might not be affected.
I agree with Baruch on this. The CONFIG_ARMADA_38X/CONFIG_KIRKWOOD singles out platforms that we know have this feature and work. Using the compatible string Is likely to flush out platforms that this hasn't been tested on and/or where it doesn't work.
Furthermore, this makes a build time test into a run time one. This
bloats the
code for platforms like Allwinner that are unlikely to be affected.
What is the advantage of device_is_compatible()? Is it feasible to build a multi-platform U-Boot image?
&& bus->req_seq == 0)twsi_disable_i2c_slave(twsi);- return 0;
+}
baruch
-- http://baruch.siach.name/blog/ ~. .~ Tk Open
Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -
On 29.05.2018 19:55, Baruch Siach wrote:
Hi Stefan,
On Tue, May 29, 2018 at 08:11:55AM +0200, Stefan Roese wrote:
On 29.05.2018 06:45, Baruch Siach wrote:
Equivalent code that disables the hidden i2c0 slave already exists in the Turris Omnia platform specific code. But this hidden i2c0 slave that interferes the i2c bus is not board specific. Armada 38x SoCs and at least some Kirkwood variants are affected as well. Add code to disable this slave to the i2c bus driver to make it work on all affected hardware.
Use the bind callback because we want this to always run at boot, regardless of whether U-Boot uses the i2c bus.
Cc: Rabeeh Khoury rabeeh@solid-run.com Cc: Chris Packham judge.packham@gmail.com Reviewed-by: Stefan Roese sr@denx.de Reviewed-by: Heiko Schocher hs@denx.de Signed-off-by: Baruch Siach baruch@tkos.co.il
[snip]
+static int mvtwsi_i2c_bind(struct udevice *bus) +{
- struct mvtwsi_registers *twsi = devfdt_get_addr_ptr(bus);
- /* Disable the hidden slave in i2c0 of these platforms */
- if ((IS_ENABLED(CONFIG_ARMADA_38X) || IS_ENABLED(CONFIG_KIRKWOOD))
We could better use the compatible check here:
if (device_is_compatible(dev, "marvell,mv64xxx-i2c"))
This is not an equivalent check. marvell,mv64xxx-i2c covers other SoCs that might not be affected.
I only see Marvell Kirkwood and Armada XP / A38x boards using this compatible property.
Furthermore, this makes a build time test into a run time one. This bloats the code for platforms like Allwinner that are unlikely to be affected.
Yes, this is a disadvantage.
What is the advantage of device_is_compatible()? Is it feasible to build a multi-platform U-Boot image?
The main advantage is, that multiple platforms can be supported in one single image. This is how its done in Linux. I agree, that this is not so common in U-Boot, e.g. to support Orion and Kirkwood in one single U-Boot image. So lets keep it this way and apply this patch version if nobody else objects.
Thanks, Stefan
On Tue, May 29, 2018 at 4:46 PM Baruch Siach baruch@tkos.co.il wrote:
Equivalent code that disables the hidden i2c0 slave already exists in the Turris Omnia platform specific code. But this hidden i2c0 slave that interferes the i2c bus is not board specific. Armada 38x SoCs and at least some Kirkwood variants are affected as well. Add code to disable this slave to the i2c bus driver to make it work on all affected hardware.
Use the bind callback because we want this to always run at boot, regardless of whether U-Boot uses the i2c bus.
Cc: Rabeeh Khoury rabeeh@solid-run.com Cc: Chris Packham judge.packham@gmail.com Reviewed-by: Stefan Roese sr@denx.de Reviewed-by: Heiko Schocher hs@denx.de Signed-off-by: Baruch Siach baruch@tkos.co.il
Tested-by: Chris Packham judge.packham@gmail.com
Hello Baruch,
Am 29.05.2018 um 06:45 schrieb Baruch Siach:
Equivalent code that disables the hidden i2c0 slave already exists in the Turris Omnia platform specific code. But this hidden i2c0 slave that interferes the i2c bus is not board specific. Armada 38x SoCs and at least some Kirkwood variants are affected as well. Add code to disable this slave to the i2c bus driver to make it work on all affected hardware.
Use the bind callback because we want this to always run at boot, regardless of whether U-Boot uses the i2c bus.
Cc: Rabeeh Khoury rabeeh@solid-run.com Cc: Chris Packham judge.packham@gmail.com Reviewed-by: Stefan Roese sr@denx.de Reviewed-by: Heiko Schocher hs@denx.de Signed-off-by: Baruch Siach baruch@tkos.co.il
v2:
Use clrbits_le32 (Stefan Roese)
Apply to Kirkwood (Chris Packham)
Add review tags from Stefan and Heiko
drivers/i2c/mvtwsi.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)
Your patch breaks build for sun8i, see travis build:
https://travis-ci.org/hsdenx/u-boot-i2c/jobs/388655845
Please fix this issue, and send a v3, thanks!
bye, Heiko
participants (4)
-
Baruch Siach -
Chris Packham -
Heiko Schocher -
Stefan Roese