[U-Boot] [PATCH] dm: i2c: mxc_i2c: implement i2c_idle_bus
Heiko Schocher
hs at denx.de
Mon Mar 21 07:50:32 CET 2016
Hello Peng Fan,
Sorry for the late reply ...
Am 11.03.2016 um 09:47 schrieb Peng Fan:
> Implement i2c_idle_bus in driver, then setup_i2c can
> be dropped for boards which enable DM_I2C/DM_GPIO/PINCTRL.
> The i2c_idle_bus force bus idle flow follows setup_i2c in
> arch/arm/imx-common/i2c-mxv7.c
>
> This patch is an implementation following linux kernel patch:
> "
> commit 1c4b6c3bcf30d0804db0d0647d8ebeb862c6f7e5
> Author: Gao Pan <b54642 at freescale.com>
> Date: Fri Oct 23 20:28:54 2015 +0800
>
> i2c: imx: implement bus recovery
>
> Implement bus recovery methods for i2c-imx so we can recover from
> situations where SCL/SDA are stuck low.
>
> Once i2c bus SCL/SDA are stuck low during transfer, config the i2c
> pinctrl to gpio mode by calling pinctrl sleep set function, and then
> use GPIO to emulate the i2c protocol to send nine dummy clock to recover
> i2c device. After recovery, set i2c pinctrl to default group setting.
> "
>
> See Documentation/devicetree/bindings/i2c/i2c-imx.txt for detailed
> description.
> 1. Introuduce scl_gpio/sda_gpio/bus in mxc_i2c_bus.
> 2. Discard the __weak attribute for i2c_idle_bus and implement it,
> since we have pinctrl driver/driver model gpio driver. We can
> use device tree, but not let board code to do this.
> 3. gpio state for mxc_i2c is not a must, but it is recommended. If
> there is no gpio state, driver will give tips, but not fail.
> 4. The i2c controller was first probed, default pinctrl state will
> be used, so when need to use gpio function, need to do
> "pinctrl_select_state(dev, "gpio")" and after force bus idle,
> need to switch back "pinctrl_select_state(dev, "default")".
>
> This is example about how to use the gpio force bus
> idle function:
> "
> &i2c1 {
> clock-frequency = <100000>;
> pinctrl-names = "default", "gpio";
> pinctrl-0 = <&pinctrl_i2c1>;
> pinctrl-1 = <&pinctrl_i2c1_gpio>;
> scl-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>;
> sda-gpios = <&gpio1 29 GPIO_ACTIVE_HIGH>;
> status = "okay";
> [....]
> };
>
> [.....]
>
> pinctrl_i2c1_gpio: i2c1grp_gpio {
> fsl,pins = <
> MX6UL_PAD_UART4_TX_DATA__GPIO1_IO28 0x1b8b0
> MX6UL_PAD_UART4_RX_DATA__GPIO1_IO29 0x1b8b0
> >;
> };
> "
>
> Signed-off-by: Peng Fan <van.freenix at gmail.com>
> Cc: Albert Aribaud <albert.u.boot at aribaud.net>
> Cc: Stefano Babic <sbabic at denx.de>
> Cc: Heiko Schocher <hs at denx.de>
> Cc: Simon Glass <sjg at chromium.org>
> Cc: York Sun <york.sun at nxp.com>
> ---
> arch/arm/include/asm/imx-common/mxc_i2c.h | 10 +++
> drivers/i2c/mxc_i2c.c | 101 +++++++++++++++++++++++++++---
> 2 files changed, 102 insertions(+), 9 deletions(-)
Thanks for this patch. In principle it looks very good ... I
have only a "nitpick" ... Couldn;t we split this patch into a
common piece, which does the deblocking of the bus, and a
"board/driver" specific part, where we do the pinmux changes?
There is nothing "imx" special in the deblocking of the i2c
bus, beside switching the pinmux ... and as you use DM and DT
there is not even in your patch a imx special part ...
So I tend to say, we can move this piece of code into a more
common place (drivers/i2c/i2c_core.c or into a new file i2c_deblock.c)
instead having it in the imx i2c driver ...
Can you rework this?
Thanks a lot!
bye,
Heiko
>
> diff --git a/arch/arm/include/asm/imx-common/mxc_i2c.h b/arch/arm/include/asm/imx-common/mxc_i2c.h
> index 355b25e..b0b6d61 100644
> --- a/arch/arm/include/asm/imx-common/mxc_i2c.h
> +++ b/arch/arm/include/asm/imx-common/mxc_i2c.h
> @@ -5,6 +5,7 @@
> */
> #ifndef __ASM_ARCH_MXC_MXC_I2C_H__
> #define __ASM_ARCH_MXC_MXC_I2C_H__
> +#include <asm-generic/gpio.h>
> #include <asm/imx-common/iomux-v3.h>
>
> struct i2c_pin_ctrl {
> @@ -30,6 +31,10 @@ struct i2c_pads_info {
> * The following two is only to be compatible with non-DM part.
> * @idle_bus_fn: function to force bus idle
> * @idle_bus_data: parameter for idle_bus_fun
> + * For DM:
> + * bus: The device structure for i2c bus controller
> + * scl-gpio: specify the gpio related to SCL pin
> + * sda-gpio: specify the gpio related to SDA pin
> */
> struct mxc_i2c_bus {
> /*
> @@ -46,6 +51,11 @@ struct mxc_i2c_bus {
> #ifndef CONFIG_DM_I2C
> int (*idle_bus_fn)(void *p);
> void *idle_bus_data;
> +#else
> + struct udevice *bus;
> + /* Use gpio to force bus idle when bus state is abnormal */
> + struct gpio_desc scl_gpio;
> + struct gpio_desc sda_gpio;
> #endif
> };
>
> diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c
> index b2d15c9..445fa21 100644
> --- a/drivers/i2c/mxc_i2c.c
> +++ b/drivers/i2c/mxc_i2c.c
> @@ -23,6 +23,7 @@
> #include <i2c.h>
> #include <watchdog.h>
> #include <dm.h>
> +#include <dm/pinctrl.h>
> #include <fdtdec.h>
>
> DECLARE_GLOBAL_DATA_PTR;
> @@ -334,17 +335,74 @@ int i2c_idle_bus(struct mxc_i2c_bus *i2c_bus)
> }
> #else
> /*
> - * Since pinmux is not supported, implement a weak function here.
> - * You can implement your i2c_bus_idle in board file. When pinctrl
> - * is supported, this can be removed.
> + * See Linux Documentation/devicetree/bindings/i2c/i2c-imx.txt
> + * "
> + * scl-gpios: specify the gpio related to SCL pin
> + * sda-gpios: specify the gpio related to SDA pin
> + * add pinctrl to configure i2c pins to gpio function for i2c
> + * bus recovery, call it "gpio" state
> + * "
> + *
> + * The i2c_idle_bus is an implementation following Linux Kernel.
> */
> -int __i2c_idle_bus(struct mxc_i2c_bus *i2c_bus)
> +int i2c_idle_bus(struct mxc_i2c_bus *i2c_bus)
> {
> - return 0;
> -}
> + struct udevice *bus = i2c_bus->bus;
> + struct gpio_desc *scl_gpio = &i2c_bus->scl_gpio;
> + struct gpio_desc *sda_gpio = &i2c_bus->sda_gpio;
> + int sda, scl;
> + int i, ret = 0;
> + ulong elapsed, start_time;
>
> -int i2c_idle_bus(struct mxc_i2c_bus *i2c_bus)
> - __attribute__((weak, alias("__i2c_idle_bus")));
> + if (pinctrl_select_state(bus, "gpio")) {
> + dev_dbg(bus, "Can not to switch to use gpio pinmux\n");
> + /*
> + * GPIO pinctrl for i2c force idle is not a must,
> + * but it is strongly recommended to be used.
> + * Because it can help you to recover from bad
> + * i2c bus state. Do not return failure, because
> + * it is not a must.
> + */
> + return 0;
> + }
> +
> + dm_gpio_set_dir_flags(scl_gpio, GPIOD_IS_IN);
> + dm_gpio_set_dir_flags(sda_gpio, GPIOD_IS_IN);
> + scl = dm_gpio_get_value(scl_gpio);
> + sda = dm_gpio_get_value(sda_gpio);
> +
> + if ((sda & scl) == 1)
> + goto exit; /* Bus is idle already */
> +
> + /* Send high and low on the SCL line */
> + for (i = 0; i < 9; i++) {
> + dm_gpio_set_dir_flags(scl_gpio, GPIOD_IS_OUT);
> + dm_gpio_set_value(scl_gpio, 0);
> + udelay(50);
> + dm_gpio_set_dir_flags(scl_gpio, GPIOD_IS_IN);
> + udelay(50);
> + }
> + start_time = get_timer(0);
> + for (;;) {
> + dm_gpio_set_dir_flags(scl_gpio, GPIOD_IS_IN);
> + dm_gpio_set_dir_flags(sda_gpio, GPIOD_IS_IN);
> + scl = dm_gpio_get_value(scl_gpio);
> + sda = dm_gpio_get_value(sda_gpio);
> + if ((sda & scl) == 1)
> + break;
> + WATCHDOG_RESET();
> + elapsed = get_timer(start_time);
> + if (elapsed > (CONFIG_SYS_HZ / 5)) { /* .2 seconds */
> + ret = -EBUSY;
> + printf("%s: failed to clear bus, sda=%d scl=%d\n", __func__, sda, scl);
> + break;
> + }
> + }
> +
> +exit:
> + pinctrl_select_state(bus, "default");
> + return ret;
> +}
> #endif
>
> static int i2c_init_transfer(struct mxc_i2c_bus *i2c_bus, u8 chip,
> @@ -664,8 +722,10 @@ static int mxc_i2c_set_bus_speed(struct udevice *bus, unsigned int speed)
> static int mxc_i2c_probe(struct udevice *bus)
> {
> struct mxc_i2c_bus *i2c_bus = dev_get_priv(bus);
> + const void *fdt = gd->fdt_blob;
> + int node = bus->of_offset;
> fdt_addr_t addr;
> - int ret;
> + int ret, ret2;
>
> i2c_bus->driver_data = dev_get_driver_data(bus);
>
> @@ -675,12 +735,35 @@ static int mxc_i2c_probe(struct udevice *bus)
>
> i2c_bus->base = addr;
> i2c_bus->index = bus->seq;
> + i2c_bus->bus = bus;
>
> /* Enable clk */
> ret = enable_i2c_clk(1, bus->seq);
> if (ret < 0)
> return ret;
>
> + /*
> + * See Documentation/devicetree/bindings/i2c/i2c-imx.txt
> + * Use gpio to force bus idle when necessary.
> + */
> + ret = fdt_find_string(fdt, node, "pinctrl-names", "gpio");
> + if (ret < 0) {
> + dev_info(dev, "i2c bus %d at %lu, no gpio pinctrl state.\n", bus->seq, i2c_bus->base);
> + } else {
> + ret = gpio_request_by_name_nodev(fdt, node, "scl-gpios",
> + 0, &i2c_bus->scl_gpio,
> + GPIOD_IS_OUT);
> + ret2 = gpio_request_by_name_nodev(fdt, node, "sda-gpios",
> + 0, &i2c_bus->sda_gpio,
> + GPIOD_IS_OUT);
> + if (!dm_gpio_is_valid(&i2c_bus->sda_gpio) |
> + !dm_gpio_is_valid(&i2c_bus->scl_gpio) |
> + ret | ret2) {
> + dev_err(dev, "i2c bus %d at %lu, fail to request scl/sda gpio\n", bus->seq, i2c_bus->base);
> + return -ENODEV;
> + }
> + }
> +
> ret = i2c_idle_bus(i2c_bus);
> if (ret < 0) {
> /* Disable clk */
>
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
More information about the U-Boot
mailing list