[PATCH RTEMS] bsps/beagle: Refactored i2c driver
Vijay Kumar Banerjee
vijay at rtems.org
Sat Mar 27 20:12:35 UTC 2021
Hi Niteesh,
Thanks for the patch. I have some questions below.
On Mon, Mar 22, 2021 at 11:48 AM G S Niteesh Babu <niteesh.gs at gmail.com>
wrote:
>
> Refactored the i2c driver to parse register values from the device
> tree rather than hardcoding them. But still the clocks have to
> initialized manually.
> ---
> bsps/arm/beagle/i2c/bbb-i2c.c | 100 ++++++++++++++++--------------
> bsps/arm/beagle/include/bsp.h | 4 ++
> bsps/arm/beagle/include/bsp/i2c.h | 32 +---------
> bsps/arm/beagle/start/bspstart.c | 53 +++++++++++-----
> 4 files changed, 96 insertions(+), 93 deletions(-)
>
> diff --git a/bsps/arm/beagle/i2c/bbb-i2c.c b/bsps/arm/beagle/i2c/bbb-i2c.c
> index b2a7cf814d..c315b6fc3b 100644
> --- a/bsps/arm/beagle/i2c/bbb-i2c.c
> +++ b/bsps/arm/beagle/i2c/bbb-i2c.c
> @@ -25,6 +25,7 @@
> #include <bsp/bbb-gpio.h>
> #include <rtems/score/assert.h>
> #include <dev/i2c/i2c.h>
> +#include <ofw/ofw.h>
>
> typedef struct bbb_i2c_bus {
> i2c_bus base;
> @@ -34,12 +35,6 @@ typedef struct bbb_i2c_bus {
> volatile uint32_t *i2c_clkctrl;
> volatile uint32_t *clkstctrl;
> } clkregs;
> - struct {
> - volatile uint32_t *conf_sda;
> - uint32_t mmode_sda;
> - volatile uint32_t *conf_scl;
> - uint32_t mmode_scl;
> - } pinregs;
> rtems_id task_id;
> rtems_vector_number irq;
> i2c_msg *buffer;
> @@ -56,19 +51,29 @@ typedef struct bbb_i2c_bus {
> #else
> #define debug_print(fmt, args...)
> #endif
> +/*
> + * Here we assume the number of i2c nodes
> + * will be less than 100.
> + */
> +#define PATH_LEN strlen("/dev/i2c-xx")
>
> static int am335x_i2c_fill_registers(
> bbb_i2c_bus *bus,
> - uintptr_t register_base
> + phandle_t node
> )
> {
> - /* FIXME: The pin handling should be replaced by a proper pin handling
during
> - * initialization. This one is heavily board specific. */
> -#if ! IS_AM335X
> - printk ("The I2C driver currently only works on Beagle Bone. Please
add your pin configs.");
> - return EINVAL;
> -#endif
> - bus->regs = (volatile bbb_i2c_regs *) register_base;
> + ssize_t rv;
> + rtems_ofw_memory_area reg;
> +
> + rv = rtems_ofw_get_reg(node, ®, sizeof(reg));
> + if (rv <= 0)
> + return EINVAL;
> +
> + bus->regs = (volatile bbb_i2c_regs *)reg.start;
> +
> + /*
> + * FIXME: Implement a clock driver to parse and setup clocks
> + */
> switch ((intptr_t) bus->regs) {
> case AM335X_I2C0_BASE:
> bus->clkregs.ctrl_clkctrl = ®(AM335X_SOC_CM_WKUP_REGS +
> @@ -77,10 +82,6 @@ static int am335x_i2c_fill_registers(
> AM335X_CM_WKUP_I2C0_CLKCTRL);
> bus->clkregs.clkstctrl = ®(AM335X_SOC_CM_WKUP_REGS +
> AM335X_CM_WKUP_CLKSTCTRL);
> - bus->pinregs.conf_sda = ®(AM335X_PADCONF_BASE +
AM335X_CONF_I2C0_SDA);
> - bus->pinregs.mmode_sda = 0;
> - bus->pinregs.conf_scl = ®(AM335X_PADCONF_BASE +
AM335X_CONF_I2C0_SCL);
> - bus->pinregs.mmode_scl = 0;
> break;
> case AM335X_I2C1_BASE:
> bus->clkregs.ctrl_clkctrl = ®(AM335X_SOC_CM_WKUP_REGS +
> @@ -88,10 +89,6 @@ static int am335x_i2c_fill_registers(
> bus->clkregs.i2c_clkctrl = ®(AM335X_CM_PER_ADDR +
> AM335X_CM_PER_I2C1_CLKCTRL);
> bus->clkregs.clkstctrl = NULL;
> - bus->pinregs.conf_sda = ®(AM335X_PADCONF_BASE +
AM335X_CONF_SPI0_D1);
> - bus->pinregs.mmode_sda = 2;
> - bus->pinregs.conf_scl = ®(AM335X_PADCONF_BASE +
AM335X_CONF_SPI0_CS0);
> - bus->pinregs.mmode_scl = 2;
> break;
> case AM335X_I2C2_BASE:
> bus->clkregs.ctrl_clkctrl = ®(AM335X_SOC_CM_WKUP_REGS +
> @@ -99,24 +96,12 @@ static int am335x_i2c_fill_registers(
> bus->clkregs.i2c_clkctrl = ®(AM335X_CM_PER_ADDR +
> AM335X_CM_PER_I2C2_CLKCTRL);
> bus->clkregs.clkstctrl = NULL;
> - bus->pinregs.conf_sda = ®(AM335X_PADCONF_BASE +
AM335X_CONF_UART1_CTSN);
> - bus->pinregs.mmode_sda = 3;
> - bus->pinregs.conf_scl = ®(AM335X_PADCONF_BASE +
AM335X_CONF_UART1_RTSN);
> - bus->pinregs.mmode_scl = 3;
> break;
> default:
> return EINVAL;
> }
> - return 0;
> -}
>
> -static void am335x_i2c_pinmux( bbb_i2c_bus *bus )
> -{
> - *bus->pinregs.conf_sda =
> - ( BBB_RXACTIVE | BBB_SLEWCTRL | bus->pinregs.mmode_sda);
> -
> - *bus->pinregs.conf_scl =
> - ( BBB_RXACTIVE | BBB_SLEWCTRL | bus->pinregs.mmode_scl);
> + return 0;
> }
>
> static void am335x_i2c_module_clk_enable( bbb_i2c_bus *bus )
> @@ -453,18 +438,16 @@ static void am335x_i2c_destroy( i2c_bus *base )
> i2c_bus_destroy_and_free( &bus->base );
> }
>
> -int am335x_i2c_bus_register(
> - const char *bus_path,
> - uintptr_t register_base,
> - uint32_t input_clock,
> - rtems_vector_number irq
> +static int am335x_i2c_bus_register(
> + phandle_t node
> )
This is method can be used by the application to register i2c device, why
is it being removed? It is also documented here:
https://docs.rtems.org/branches/master/user/bsps/bsps-arm.html#id2
If we really want to make it static, then it at least needs to be removed
from the documentation. But I'm not sure if we want to make it static.
> {
> - bbb_i2c_bus *bus;
> - rtems_status_code sc;
> - int err;
> -
> - (void) input_clock; /* FIXME: Unused. Left for compatibility. */
> + bbb_i2c_bus *bus;
> + rtems_status_code sc;
> + rtems_vector_number irq;
> + int err;
> + int unit;
> + char bus_path[PATH_LEN];
>
> bus = (bbb_i2c_bus *) i2c_bus_alloc_and_init( sizeof( *bus ) );
>
> @@ -472,15 +455,24 @@ int am335x_i2c_bus_register(
> return -1;
> }
>
> + unit = beagle_get_node_unit(node);
> +
How would a user register a custom path? Does it mean that the only way to
register a custom path is through device tree?
> + snprintf(bus_path, PATH_LEN, "/dev/i2c-%d", unit);
If suppose the unit is 99, I think it'll still return "/dev/i2c-9.
Should this be PATH_LEN + 1 here (or +1 can be added in the macro
definition itself) to accommodate the 2 digit unit?
> +
> + err = rtems_ofw_get_interrupts(node, &irq, sizeof(irq));
Can this be done from the application and then pass irq as an argument to
am335x_i2c_bus_register, in order to keep the function signature same?
> + if (err < 1) {
> + ( *bus->base.destroy )( &bus->base );
> + rtems_set_errno_and_return_minus_one( err );
> + }
> bus->irq = irq;
>
> - err = am335x_i2c_fill_registers(bus, register_base);
> + err = am335x_i2c_fill_registers(bus, node);
> if (err != 0) {
> ( *bus->base.destroy )( &bus->base );
> rtems_set_errno_and_return_minus_one( err );
> }
> - am335x_i2c_module_clk_enable(bus);
> - am335x_i2c_pinmux( bus );
> +
> + am335x_i2c_module_clk_enable( bus );
> err = am335x_i2c_reset( bus );
> if (err != 0) {
> ( *bus->base.destroy )( &bus->base );
> @@ -506,3 +498,15 @@ int am335x_i2c_bus_register(
>
> return i2c_bus_register( &bus->base, bus_path );
> }
> +
> +void beagle_i2c_init(phandle_t node)
This is a nice wrapper and maybe you could add all the needed ofw calls
here to get the values and then call am335x_i2c_bus_register. Please also
document this function so that it also acts as a reference for someone on
how to use the am335x_i2c_bus_register.
> +{
> + int rv;
> +
> + if (!rtems_ofw_is_node_compatible(node, "ti,omap4-i2c"))
> + return ;
> +
> + rv = am335x_i2c_bus_register(node);
> + if (rv != 0)
> + printk("i2c: Could not register device (%d)\n", rv);
> +}
> diff --git a/bsps/arm/beagle/include/bsp.h b/bsps/arm/beagle/include/bsp.h
> index cb415fda89..80a9cc291d 100644
> --- a/bsps/arm/beagle/include/bsp.h
> +++ b/bsps/arm/beagle/include/bsp.h
> @@ -49,6 +49,8 @@
> #include <libcpu/omap3.h>
> #include <libcpu/am335x.h>
>
> +#include <ofw/ofw.h>
> +
> #define BSP_FEATURE_IRQ_EXTENSION
>
> /* UART base clock frequency */
> @@ -68,6 +70,8 @@
>
> #define udelay(u) rtems_task_wake_after(1 +
((u)/rtems_configuration_get_microseconds_per_tick()))
>
> +int beagle_get_node_unit(phandle_t node);
> +
> /* Write a uint32_t value to a memory address. */
> static inline void
> write32(uint32_t address, uint32_t value)
> diff --git a/bsps/arm/beagle/include/bsp/i2c.h
b/bsps/arm/beagle/include/bsp/i2c.h
> index 60f71194bf..d80b376fa0 100644
> --- a/bsps/arm/beagle/include/bsp/i2c.h
> +++ b/bsps/arm/beagle/include/bsp/i2c.h
> @@ -26,6 +26,7 @@
> #include <rtems.h>
> #include <bsp.h>
> #include <dev/i2c/i2c.h>
> +#include <ofw/ofw.h>
>
> #ifdef __cplusplus
> extern "C" {
> @@ -38,10 +39,6 @@ extern "C" {
> #define BBB_I2C_1_BUS_PATH "/dev/i2c-1"
> #define BBB_I2C_2_BUS_PATH "/dev/i2c-2"
>
> -#define BBB_I2C0_IRQ 70
> -#define BBB_I2C1_IRQ 71
> -#define BBB_I2C2_IRQ 30
> -
> typedef enum {
> I2C0,
> I2C1,
> @@ -87,32 +84,7 @@ typedef struct i2c_regs {
> uint32_t BBB_I2C_SBLOCK;
> } bbb_i2c_regs;
>
> -int am335x_i2c_bus_register(
> - const char *bus_path,
> - uintptr_t register_base,
> - uint32_t input_clock, /* FIXME: Unused. Left for
compatibility. */
> - rtems_vector_number irq
> -);
> -
> -static inline int bbb_register_i2c_1( void )
Instead of removing these wrappers, could you use these to register the
i2c-1 or i2c-2 paths, just for compatibility reasons?
Once again, thank you for working on the patch. The bbb-i2c does need
refactoring to work with the update ofw changes.
Best regards,
Vijay
> -{
> - return am335x_i2c_bus_register(
> - BBB_I2C_1_BUS_PATH,
> - AM335X_I2C1_BASE,
> - I2C_BUS_CLOCK_DEFAULT,
> - BBB_I2C1_IRQ
> - );
> -}
> -
> -static inline int bbb_register_i2c_2( void )
> -{
> - return am335x_i2c_bus_register(
> - BBB_I2C_2_BUS_PATH,
> - AM335X_I2C2_BASE,
> - I2C_BUS_CLOCK_DEFAULT,
> - BBB_I2C2_IRQ
> - );
> -}
> +void beagle_i2c_init( phandle_t node );
>
> #ifdef __cplusplus
> }
> diff --git a/bsps/arm/beagle/start/bspstart.c
b/bsps/arm/beagle/start/bspstart.c
> index a0736294c9..d45eec9e92 100644
> --- a/bsps/arm/beagle/start/bspstart.c
> +++ b/bsps/arm/beagle/start/bspstart.c
> @@ -22,9 +22,15 @@
> #include "bsp-soc-detect.h"
> #include <arm/ti/ti_pinmux.h>
> #include <ofw/ofw.h>
> +#include <bsp/i2c.h>
> +#include <string.h>
> +#include <stdlib.h>
> +#include <ctype.h>
>
> #include "bspdebug.h"
>
> +#define MIN(l,r) ((l) < (r) ? (l) : (r))
Where is it used?
> +
> void bsp_start(void)
> {
> const char *type;
> @@ -70,6 +76,7 @@ static void traverse_fdt_nodes( phandle_t node )
> * Put all driver initialization functions here
> */
> beagle_pinmux_init(node);
> + beagle_i2c_init(node);
> }
> }
>
> @@ -80,24 +87,40 @@ static void bbb_drivers_initialize(void)
> traverse_fdt_nodes(node);
> }
>
> -static void bbb_i2c_0_initialize(void)
> +int beagle_get_node_unit(phandle_t node)
> {
> - int err;
> -
> - err = am335x_i2c_bus_register(BBB_I2C_0_BUS_PATH,
> - AM335X_I2C0_BASE,
> - I2C_BUS_CLOCK_DEFAULT,
> - BBB_I2C0_IRQ);
> - if (err != 0) {
> - printk("rtems i2c-0: Device could not be registered (%d)", err);
> + char name[30];
> + char prop[30];
> + char prop_val[30];
> + static int unit;
> + phandle_t aliases;
> + int rv;
> + int i;
> +
> + rv = rtems_ofw_get_prop(node, "name", name, sizeof(name));
> + if (rv <= 0)
> + return unit++;
> +
> + aliases = rtems_ofw_find_device("/aliases");
> +
> + rv = rtems_ofw_next_prop(aliases, NULL, &prop[0], sizeof(prop));
> +
> + while (rv > 0) {
> + rv = rtems_ofw_get_prop(aliases, prop, prop_val, sizeof(prop_val));
> +
> + if (strstr(prop_val, name) != NULL) {
> + for (i = strlen(prop) - 1; i >= 0; i--) {
> + if (!isdigit(prop[i]))
> + break;
> + }
> +
> + return strtol(&prop[i + 1], NULL, 10);
> + }
> + rv = rtems_ofw_next_prop(aliases, prop, prop, sizeof(prop));
> }
> -}
>
> -RTEMS_SYSINIT_ITEM(
> - bbb_i2c_0_initialize,
> - RTEMS_SYSINIT_LAST,
> - RTEMS_SYSINIT_ORDER_LAST_BUT_5
> -);
> + return unit++;
> +}
>
> RTEMS_SYSINIT_ITEM(
> bbb_drivers_initialize,
> --
> 2.17.1
>
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20210327/71798797/attachment-0001.html>
More information about the devel
mailing list