<div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-size:small">Hello Vijay,</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, Mar 28, 2021 at 1:42 AM Vijay Kumar Banerjee <<a href="mailto:vijay@rtems.org" target="_blank">vijay@rtems.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Hi Niteesh,<br><br>Thanks for the patch. I have some questions below.<br><br>On Mon, Mar 22, 2021 at 11:48 AM G S Niteesh Babu <<a href="mailto:niteesh.gs@gmail.com" target="_blank">niteesh.gs@gmail.com</a>> wrote:<br>><br>> Refactored the i2c driver to parse register values from the device<br>> tree rather than hardcoding them. But still the clocks have to<br>> initialized manually.<br>> ---<br>>  bsps/arm/beagle/i2c/bbb-i2c.c     | 100 ++++++++++++++++--------------<br>>  bsps/arm/beagle/include/bsp.h     |   4 ++<br>>  bsps/arm/beagle/include/bsp/i2c.h |  32 +---------<br>>  bsps/arm/beagle/start/bspstart.c  |  53 +++++++++++-----<br>>  4 files changed, 96 insertions(+), 93 deletions(-)<br>><br>> diff --git a/bsps/arm/beagle/i2c/bbb-i2c.c b/bsps/arm/beagle/i2c/bbb-i2c.c<br>> index b2a7cf814d..c315b6fc3b 100644<br>> --- a/bsps/arm/beagle/i2c/bbb-i2c.c<br>> +++ b/bsps/arm/beagle/i2c/bbb-i2c.c<br>> @@ -25,6 +25,7 @@<br>>  #include <bsp/bbb-gpio.h><br>>  #include <rtems/score/assert.h><br>>  #include <dev/i2c/i2c.h><br>> +#include <ofw/ofw.h><br>><br>>  typedef struct bbb_i2c_bus {<br>>    i2c_bus base;<br>> @@ -34,12 +35,6 @@ typedef struct bbb_i2c_bus {<br>>      volatile uint32_t *i2c_clkctrl;<br>>      volatile uint32_t *clkstctrl;<br>>    } clkregs;<br>> -  struct {<br>> -    volatile uint32_t *conf_sda;<br>> -    uint32_t mmode_sda;<br>> -    volatile uint32_t *conf_scl;<br>> -    uint32_t mmode_scl;<br>> -  } pinregs;<br>>    rtems_id task_id;<br>>    rtems_vector_number irq;<br>>    i2c_msg *buffer;<br>> @@ -56,19 +51,29 @@ typedef struct bbb_i2c_bus {<br>>  #else<br>>  #define debug_print(fmt, args...)<br>>  #endif<br>> +/*<br>> + * Here we assume the number of i2c nodes<br>> + * will be less than 100.<br>> + */<br>> +#define PATH_LEN strlen("/dev/i2c-xx")<br>><br>>  static int am335x_i2c_fill_registers(<br>>    bbb_i2c_bus *bus,<br>> -  uintptr_t register_base<br>> +  phandle_t    node<br>>  )<br>>  {<br>> -  /* FIXME: The pin handling should be replaced by a proper pin handling during<br>> -   * initialization. This one is heavily board specific. */<br>> -#if ! IS_AM335X<br>> -  printk ("The I2C driver currently only works on Beagle Bone. Please add your pin configs.");<br>> -  return EINVAL;<br>> -#endif<br>> -  bus->regs = (volatile bbb_i2c_regs *) register_base;<br>> +  ssize_t rv;<br>> +  rtems_ofw_memory_area reg;<br>> +<br>> +  rv = rtems_ofw_get_reg(node, &reg, sizeof(reg));<br>> +  if (rv <= 0)<br>> +    return EINVAL;<br>> +<br>> +  bus->regs = (volatile bbb_i2c_regs *)reg.start;<br>> +<br>> +  /*<br>> +   * FIXME: Implement a clock driver to parse and setup clocks<br>> +   */<br>>    switch ((intptr_t) bus->regs) {<br>>    case AM335X_I2C0_BASE:<br>>      bus->clkregs.ctrl_clkctrl = &REG(AM335X_SOC_CM_WKUP_REGS +<br>> @@ -77,10 +82,6 @@ static int am335x_i2c_fill_registers(<br>>                                   AM335X_CM_WKUP_I2C0_CLKCTRL);<br>>      bus->clkregs.clkstctrl = &REG(AM335X_SOC_CM_WKUP_REGS +<br>>                                     AM335X_CM_WKUP_CLKSTCTRL);<br>> -    bus->pinregs.conf_sda = &REG(AM335X_PADCONF_BASE + AM335X_CONF_I2C0_SDA);<br>> -    bus->pinregs.mmode_sda = 0;<br>> -    bus->pinregs.conf_scl = &REG(AM335X_PADCONF_BASE + AM335X_CONF_I2C0_SCL);<br>> -    bus->pinregs.mmode_scl = 0;<br>>      break;<br>>    case AM335X_I2C1_BASE:<br>>      bus->clkregs.ctrl_clkctrl = &REG(AM335X_SOC_CM_WKUP_REGS +<br>> @@ -88,10 +89,6 @@ static int am335x_i2c_fill_registers(<br>>      bus->clkregs.i2c_clkctrl = &REG(AM335X_CM_PER_ADDR +<br>>                                   AM335X_CM_PER_I2C1_CLKCTRL);<br>>      bus->clkregs.clkstctrl = NULL;<br>> -    bus->pinregs.conf_sda = &REG(AM335X_PADCONF_BASE + AM335X_CONF_SPI0_D1);<br>> -    bus->pinregs.mmode_sda = 2;<br>> -    bus->pinregs.conf_scl = &REG(AM335X_PADCONF_BASE + AM335X_CONF_SPI0_CS0);<br>> -    bus->pinregs.mmode_scl = 2;<br>>      break;<br>>    case AM335X_I2C2_BASE:<br>>      bus->clkregs.ctrl_clkctrl = &REG(AM335X_SOC_CM_WKUP_REGS +<br>> @@ -99,24 +96,12 @@ static int am335x_i2c_fill_registers(<br>>      bus->clkregs.i2c_clkctrl = &REG(AM335X_CM_PER_ADDR +<br>>                                   AM335X_CM_PER_I2C2_CLKCTRL);<br>>      bus->clkregs.clkstctrl = NULL;<br>> -    bus->pinregs.conf_sda = &REG(AM335X_PADCONF_BASE + AM335X_CONF_UART1_CTSN);<br>> -    bus->pinregs.mmode_sda = 3;<br>> -    bus->pinregs.conf_scl = &REG(AM335X_PADCONF_BASE + AM335X_CONF_UART1_RTSN);<br>> -    bus->pinregs.mmode_scl = 3;<br>>      break;<br>>    default:<br>>      return EINVAL;<br>>    }<br>> -  return 0;<br>> -}<br>><br>> -static void am335x_i2c_pinmux( bbb_i2c_bus *bus )<br>> -{<br>> -  *bus->pinregs.conf_sda =<br>> -    ( BBB_RXACTIVE | BBB_SLEWCTRL | bus->pinregs.mmode_sda);<br>> -<br>> -  *bus->pinregs.conf_scl =<br>> -    ( BBB_RXACTIVE | BBB_SLEWCTRL | bus->pinregs.mmode_scl);<br>> +  return 0;<br>>  }<br>><br>>  static void am335x_i2c_module_clk_enable( bbb_i2c_bus *bus )<br>> @@ -453,18 +438,16 @@ static void am335x_i2c_destroy( i2c_bus *base )<br>>    i2c_bus_destroy_and_free( &bus->base );<br>>  }<br>><br>> -int am335x_i2c_bus_register(<br>> -  const char         *bus_path,<br>> -  uintptr_t           register_base,<br>> -  uint32_t            input_clock,<br>> -  rtems_vector_number irq<br>> +static int am335x_i2c_bus_register(<br>> +  phandle_t           node<br>>  )<br><div>This is method can be used by the application to register i2c device, why is it being removed? It is also documented here: <a href="https://docs.rtems.org/branches/master/user/bsps/bsps-arm.html#id2" target="_blank">https://docs.rtems.org/branches/master/user/bsps/bsps-arm.html#id2</a></div><div>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.</div></div></blockquote><div> </div><div><span class="gmail_default" style="font-size:small">The registration of the devices will happen based on the device tree, if a user wants to add any device it is</span></div><div><span class="gmail_default" style="font-size:small">expected from him to provide an overlay or enable the device node in the FDT. This removes the need for</span></div><div><span class="gmail_default" style="font-size:small">registration functions.</span></div><div><span class="gmail_default" style="font-size:small"><a href="https://devel.rtems.org/ticket/3784">https://devel.rtems.org/ticket/3784</a><br></span></div><div><span class="gmail_default" style="font-size:small"><a href="https://lists.rtems.org/pipermail/devel/2019-August/027288.html">https://lists.rtems.org/pipermail/devel/2019-August/027288.html</a><br></span></div><div><span class="gmail_default" style="font-size:small"><br></span></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">>  {<br>> -  bbb_i2c_bus      *bus;<br>> -  rtems_status_code sc;<br>> -  int               err;<br>> -<br>> -  (void) input_clock; /* FIXME: Unused. Left for compatibility. */<br>> +  bbb_i2c_bus        *bus;<br>> +  rtems_status_code   sc;<br>> +  rtems_vector_number irq;<br>> +  int                 err;<br>> +  int                 unit;<br>> +  char                bus_path[PATH_LEN];<br>><br>>    bus = (bbb_i2c_bus *) i2c_bus_alloc_and_init( sizeof( *bus ) );<br>><br>> @@ -472,15 +455,24 @@ int am335x_i2c_bus_register(<br>>      return -1;<br>>    }<br>><br>> +  unit = beagle_get_node_unit(node);<br><div>> +</div><div><br></div><div>How would a user register a custom path? Does it mean that the only way to register a custom path is through device tree?<br></div></div></blockquote><div> </div><div><span class="gmail_default" style="font-size:small">Currently, one cannot, the bus path is determined based on the node index. This can be fixed by allowing the user to pass</span></div><div><span class="gmail_default" style="font-size:small">the path in the device tree as you said.</span></div><div><span class="gmail_default" style="font-size:small">something like</span></div><div><span class="gmail_default" style="font-size:small">i2c0 {</span></div></div><blockquote style="margin:0 0 0 40px;border:none;padding:0px"><div class="gmail_quote"><div><span class="gmail_default" style="font-size:small">rtems-bus-path = "/dev/i2c-eeprom"</span></div></div></blockquote><span class="gmail_default" style="font-size:small"></span>}<div><div class="gmail_default" style="font-size:small">If this is OK, I'll add this feature to the patch?</div><div class="gmail_quote"><div></div><div><span class="gmail_default" style="font-size:small"></span> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div></div><div><br></div><div>> +  snprintf(bus_path, PATH_LEN, "/dev/i2c-%d", unit);</div><div>If suppose the unit is 99, I think it'll still return "/dev/i2c-9. </div><div>Should this be PATH_LEN + 1 here (or +1 can be added in the macro definition itself) to accommodate the 2 digit unit?</div><div><br></div>> +<br><div>> +  err = rtems_ofw_get_interrupts(node, &irq, sizeof(irq));</div><div><br></div><div>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?</div></div></blockquote><div> </div><div><div class="gmail_default" style="font-size:small">A similar approach to the bus path can be done here. Or if possible we can let the user reinitialize the IRQ at runtime<br></div><div class="gmail_default" style="font-size:small">by calling something like beagle_i2c_set_irq()</div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div></div>> +  if (err < 1) {<br>> +    ( *bus->base.destroy )( &bus->base );<br>> +    rtems_set_errno_and_return_minus_one( err );<br>> +  }<br>>    bus->irq = irq;<br>><br>> -  err = am335x_i2c_fill_registers(bus, register_base);<br>> +  err = am335x_i2c_fill_registers(bus, node);<br>>    if (err != 0) {<br>>      ( *bus->base.destroy )( &bus->base );<br>>      rtems_set_errno_and_return_minus_one( err );<br>>    }<br>> -  am335x_i2c_module_clk_enable(bus);<br>> -  am335x_i2c_pinmux( bus );<br>> +<br>> +  am335x_i2c_module_clk_enable( bus );<br>>    err = am335x_i2c_reset( bus );<br>>    if (err != 0) {<br>>      ( *bus->base.destroy )( &bus->base );<br>> @@ -506,3 +498,15 @@ int am335x_i2c_bus_register(<br>><br>>    return i2c_bus_register( &bus->base, bus_path );<br>>  }<br>> +<br><div>> +void beagle_i2c_init(phandle_t node)</div><div><br></div><div>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.<br></div></div></blockquote><div><span class="gmail_default" style="font-size:small">OK. I'll refactor and document this.</span></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div></div><div><br></div>> +{<br>> +  int rv;<br>> +<br>> +  if (!rtems_ofw_is_node_compatible(node, "ti,omap4-i2c"))<br>> +    return ;<br>> +<br>> +  rv = am335x_i2c_bus_register(node);<br>> +  if (rv != 0)<br>> +    printk("i2c: Could not register device (%d)\n", rv);<br>> +}<br>> diff --git a/bsps/arm/beagle/include/bsp.h b/bsps/arm/beagle/include/bsp.h<br>> index cb415fda89..80a9cc291d 100644<br>> --- a/bsps/arm/beagle/include/bsp.h<br>> +++ b/bsps/arm/beagle/include/bsp.h<br>> @@ -49,6 +49,8 @@<br>>  #include <libcpu/omap3.h><br>>  #include <libcpu/am335x.h><br>><br>> +#include <ofw/ofw.h><br>> +<br>>  #define BSP_FEATURE_IRQ_EXTENSION<br>><br>>  /* UART base clock frequency */<br>> @@ -68,6 +70,8 @@<br>><br>>  #define udelay(u) rtems_task_wake_after(1 + ((u)/rtems_configuration_get_microseconds_per_tick()))<br>><br>> +int beagle_get_node_unit(phandle_t node);<br>> +<br>>  /* Write a uint32_t value to a memory address. */<br>>  static inline void<br>>  write32(uint32_t address, uint32_t value)<br>> diff --git a/bsps/arm/beagle/include/bsp/i2c.h b/bsps/arm/beagle/include/bsp/i2c.h<br>> index 60f71194bf..d80b376fa0 100644<br>> --- a/bsps/arm/beagle/include/bsp/i2c.h<br>> +++ b/bsps/arm/beagle/include/bsp/i2c.h<br>> @@ -26,6 +26,7 @@<br>>  #include <rtems.h><br>>  #include <bsp.h><br>>  #include <dev/i2c/i2c.h><br>> +#include <ofw/ofw.h><br>><br>>  #ifdef __cplusplus<br>>  extern "C" {<br>> @@ -38,10 +39,6 @@ extern "C" {<br>>  #define BBB_I2C_1_BUS_PATH "/dev/i2c-1"<br>>  #define BBB_I2C_2_BUS_PATH "/dev/i2c-2"<br>><br>> -#define BBB_I2C0_IRQ 70<br>> -#define BBB_I2C1_IRQ 71<br>> -#define BBB_I2C2_IRQ 30<br>> -<br>>  typedef enum {<br>>    I2C0,<br>>    I2C1,<br>> @@ -87,32 +84,7 @@ typedef struct i2c_regs {<br>>    uint32_t BBB_I2C_SBLOCK;<br>>  } bbb_i2c_regs;<br>><br>> -int am335x_i2c_bus_register(<br>> -  const char         *bus_path,<br>> -  uintptr_t           register_base,<br>> -  uint32_t            input_clock, /* FIXME: Unused. Left for compatibility. */<br>> -  rtems_vector_number irq<br>> -);<br>> -<br><div>> -static inline int bbb_register_i2c_1( void )</div><div><br></div><div>Instead of removing these wrappers, could you use these to register the i2c-1 or i2c-2 paths, just for compatibility reasons? </div></div></blockquote><div><span class="gmail_default" style="font-size:small">If we have these registration functions, we will have to remove the automatic initialization of the i2c driver during boot time right?</span></div><div><span class="gmail_default" style="font-size:small">I am not sure of how can we have both user-based registration and Device Tree based registration at the same time.</span></div><div><div class="gmail_default" style="font-size:small"><a href="https://devel.rtems.org/ticket/3784">https://devel.rtems.org/ticket/3784</a><br></div><div class="gmail_default" style="font-size:small">As per this ticket, I think the goal is to remove these kinds of registration functions so that the user doesn't have to</div><div class="gmail_default" style="font-size:small">worry about the number of devices or type of the board.</div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><br></div><div><br></div><div><br></div><div>Once again, thank you for working on the patch. The bbb-i2c does need refactoring to work with the update ofw changes.</div></div></blockquote><div><span class="gmail_default" style="font-size:small"></span></div><div><span class="gmail_default" style="font-size:small">Really sorry for the delay, I have been working with QEMU to participate in GSoC this year. And it is really hard to manage</span></div><div><span class="gmail_default" style="font-size:small">between this and other academic activities.</span></div><div><span class="gmail_default" style="font-size:small"></span> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>Best regards,</div><div>Vijay<br></div><div><br></div>> -{<br>> -  return am335x_i2c_bus_register(<br>> -    BBB_I2C_1_BUS_PATH,<br>> -    AM335X_I2C1_BASE,<br>> -    I2C_BUS_CLOCK_DEFAULT,<br>> -    BBB_I2C1_IRQ<br>> -  );<br>> -}<br>> -<br>> -static inline int bbb_register_i2c_2( void )<br>> -{<br>> -  return am335x_i2c_bus_register(<br>> -    BBB_I2C_2_BUS_PATH,<br>> -    AM335X_I2C2_BASE,<br>> -    I2C_BUS_CLOCK_DEFAULT,<br>> -    BBB_I2C2_IRQ<br>> -  );<br>> -}<br>> +void beagle_i2c_init( phandle_t node );<br>><br>>  #ifdef __cplusplus<br>>  }<br>> diff --git a/bsps/arm/beagle/start/bspstart.c b/bsps/arm/beagle/start/bspstart.c<br>> index a0736294c9..d45eec9e92 100644<br>> --- a/bsps/arm/beagle/start/bspstart.c<br>> +++ b/bsps/arm/beagle/start/bspstart.c<br>> @@ -22,9 +22,15 @@<br>>  #include "bsp-soc-detect.h"<br>>  #include <arm/ti/ti_pinmux.h><br>>  #include <ofw/ofw.h><br>> +#include <bsp/i2c.h><br>> +#include <string.h><br>> +#include <stdlib.h><br>> +#include <ctype.h><br>><br>>  #include "bspdebug.h"<br>><br><div>> +#define MIN(l,r) ((l) < (r) ? (l) : (r))</div><div><br></div><div>Where is it used?</div></div></blockquote><div><span class="gmail_default" style="font-size:small">Used it during testing. Will be removed.</span> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><br></div>> +<br>>  void bsp_start(void)<br>>  {<br>>    const char *type;<br>> @@ -70,6 +76,7 @@ static void traverse_fdt_nodes( phandle_t node )<br>>       * Put all driver initialization functions here<br>>       */<br>>      beagle_pinmux_init(node);<br>> +    beagle_i2c_init(node);<br>>    }<br>>  }<br>><br>> @@ -80,24 +87,40 @@ static void bbb_drivers_initialize(void)<br>>    traverse_fdt_nodes(node);<br>>  }<br>><br>> -static void bbb_i2c_0_initialize(void)<br>> +int beagle_get_node_unit(phandle_t node)<br>>  {<br>> -  int err;<br>> -<br>> -  err = am335x_i2c_bus_register(BBB_I2C_0_BUS_PATH,<br>> -                                AM335X_I2C0_BASE,<br>> -                                I2C_BUS_CLOCK_DEFAULT,<br>> -                                BBB_I2C0_IRQ);<br>> -  if (err != 0) {<br>> -    printk("rtems i2c-0: Device could not be registered (%d)", err);<br>> +  char name[30];<br>> +  char prop[30];<br>> +  char prop_val[30];<br>> +  static int unit;<br>> +  phandle_t aliases;<br>> +  int rv;<br>> +  int i;<br>> +<br>> +  rv = rtems_ofw_get_prop(node, "name", name, sizeof(name));<br>> +  if (rv <= 0)<br>> +    return unit++;<br>> +<br>> +  aliases = rtems_ofw_find_device("/aliases");<br>> +<br>> +  rv = rtems_ofw_next_prop(aliases, NULL, &prop[0], sizeof(prop));<br>> +<br>> +  while (rv > 0) {<br>> +    rv = rtems_ofw_get_prop(aliases, prop, prop_val, sizeof(prop_val));<br>> +<br>> +    if (strstr(prop_val, name) != NULL) {<br>> +      for (i = strlen(prop) - 1; i >= 0; i--) {<br>> +        if (!isdigit(prop[i]))<br>> +          break;<br>> +      }<br>> +<br>> +      return strtol(&prop[i + 1], NULL, 10);<br>> +    }<br>> +    rv = rtems_ofw_next_prop(aliases, prop, prop, sizeof(prop));<br>>    }<br>> -}<br>><br>> -RTEMS_SYSINIT_ITEM(<br>> -  bbb_i2c_0_initialize,<br>> -  RTEMS_SYSINIT_LAST,<br>> -  RTEMS_SYSINIT_ORDER_LAST_BUT_5<br>> -);<br>> +  return unit++;<br>> +}<br>><br>>  RTEMS_SYSINIT_ITEM(<br>>         bbb_drivers_initialize,<br>> --<br>> 2.17.1<br>><br>> _______________________________________________<br>> devel mailing list<br>> <a href="mailto:devel@rtems.org" target="_blank">devel@rtems.org</a><br>> <a href="http://lists.rtems.org/mailman/listinfo/devel" target="_blank">http://lists.rtems.org/mailman/listinfo/devel</a></div>
</blockquote></div></div></div>