[PATCH RTEMS] bsps/beagle: Refactored i2c driver
Christian MAUDERER
christian.mauderer at embedded-brains.de
Thu Apr 8 06:24:50 UTC 2021
Hello Vijay and Niteesh,
Am 08.04.21 um 04:17 schrieb Vijay Kumar Banerjee:
> On Wed, Apr 7, 2021 at 7:56 PM Niteesh G. S. <niteesh.gs at gmail.com> wrote:
>>
>> Hello Vijay,
>>
>> On Sun, Mar 28, 2021 at 1:42 AM Vijay Kumar Banerjee <vijay at rtems.org> wrote:
>>>
>>> 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.
>>
>>
>> The registration of the devices will happen based on the device tree, if a user wants to add any device it is
>> expected from him to provide an overlay or enable the device node in the FDT. This removes the need for
>> registration functions.
>> https://devel.rtems.org/ticket/3784
>> https://lists.rtems.org/pipermail/devel/2019-August/027288.html
>>
> Ok, thanks for the links. Please update the documentation accordingly.
>
>>>> {
>>>> - 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?
>>
>>
>> Currently, one cannot, the bus path is determined based on the node index. This can be fixed by allowing the user to pass
>> the path in the device tree as you said.
>> something like
>> i2c0 {
>>
>> rtems-bus-path = "/dev/i2c-eeprom"
>>
>> }
>> If this is OK, I'll add this feature to the patch?
> Node index works and it would also be nice if you add an
> rtems-bus-path field for FDT. Please document that and make sure that
> it works based on node index even when the rtems-bus-path is absent.
>
Please follow the usual FDT conventions for the name. System or vendor
specific names are normally separated with a ','. For Example:
linux,keymap
ibm,opal-rtc
So in this case it should be a
rtems,bus-path
or maybe
rtems,device-path
or just
rtems,path
I used the last one in the imxrt BSP.
>>
>>>
>>>
>>>> + 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?
>>
>>
>> A similar approach to the bus path can be done here. Or if possible we can let the user reinitialize the IRQ at runtime
>> by calling something like beagle_i2c_set_irq()
>>
> My concern was removing the am335x_* call, I think it's Ok since that
> would be removed. It'll be a more general approach.
>
>>>
>>>> + 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.
>>
>> OK. I'll refactor and document this.
>
> Thanks.
>
>>>
>>>
>>>> +{
>>>> + 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?
>>
>> If we have these registration functions, we will have to remove the automatic initialization of the i2c driver during boot time right?
>> I am not sure of how can we have both user-based registration and Device Tree based registration at the same time.
>> https://devel.rtems.org/ticket/3784
>> As per this ticket, I think the goal is to remove these kinds of registration functions so that the user doesn't have to
>> worry about the number of devices or type of the board.
>
> The discussions and the tickets make it clear that these need to go.
> So this is Ok. Thanks for explaining.
Please add a
Update #3784
line in the commit description to make it clear that the change belongs
to that ticket.
>>>
>>>
>>>
>>>
>>> Once again, thank you for working on the patch. The bbb-i2c does need refactoring to work with the update ofw changes.
>>
>> Really sorry for the delay, I have been working with QEMU to participate in GSoC this year. And it is really hard to manage
>> between this and other academic activities.
>>
> Thanks for the good work and good luck for your GSoC application.
>
+1
Best regards
Christian
>
>
>
>>>
>>> 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?
>>
>> Used it during testing. Will be removed.
>
> :) Thanks
>
>
>
> Best regards,
> Vijay
>>>
>>>
>>>> +
>>>> 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
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
>
--
--------------------------------------------
embedded brains GmbH
Herr Christian MAUDERER
Dornierstr. 4
82178 Puchheim
Germany
email: christian.mauderer at embedded-brains.de
phone: +49-89-18 94 741 - 18
fax: +49-89-18 94 741 - 08
Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/
More information about the devel
mailing list