[PATCH v6 06/16] OMAP2+: UART: Remove certain feilds from omap_uart_state struct

Govindraj govindraj.ti at gmail.com
Wed Oct 12 06:25:18 EDT 2011


On Tue, Oct 11, 2011 at 5:01 AM, Kevin Hilman <khilman at ti.com> wrote:
> "Govindraj.R" <govindraj.raja at ti.com> writes:
>
>> Removing some of the uart info that is maintained in omap_uart_state struct
>> used for UART PM in serial.c.
>>
>> Remove omap_uart_state struct dependency from omap_serial_init,
>> omap_serial_init_port, omap_serial_early_init and omap_uart_idle_init
>> functions.  And populate the same info in omap_uart_port_info struct
>> used as pdata struct.
>
> IMO, this change doesn't belong in this patch and leads to clutter.  The
> rest of the series slowly removes/replaces all the fields from this
> struct, so the right place to remove it's usage all together is at the
> end of the series when (if) all the fields are no longer needed (or have
> been moved.)
>

Okay will move it to end.

> Stated differently, IMO, this patch should leave the uart->num and
> uart->oh and the list_head (uart->node) alone (probably uart->pdev too)
> and just cleanup the fields that are no longer used.  Removing num, oh,
> node here causes churn because you're force to change things here that
> are then removed in later patches.
>

okay will retain the list part.

>> Added omap_uart_hwmod_lookup function to look up oh by name used in
>> serial_port_init and omap_serial_early_init functions.
>
> Because of the above change, you now are doing a hwmod lookup 2 times
> for every UART.  Leaving the uart_list and uart->num in place will avoid
> the need for that change.
>

yes since uart_list was removed, will retain the uart_list
to avoid the look up twice.


>> A list of omap_uart_state was maintained one for each uart, the same
>> is removed.  Number of uarts available is maintained in num_uarts
>> field, re-use the same in omap_serial_init func to register each uart.
>>
>> Remove omap_info which used details from omap_uart_state and use a
>> pdata pointer to pass platform specific info to driver.
>
> There is no omap_info.  Did you mean omap_up_info?

yes sorry typo.

>
>> The mapbase (start_address), membase(io_remap cookie) maintained as
>> part of uart_state struct and pdata struct are removed as this is
>> handled within driver.
>
> This part makes sense.
>

okay will retain this part only.

>> Errata field is also moved to pdata.
>
> Why in this patch instead of the subsequent "Move errata handling from
> serial.c to omap-serial" patch?
>

will move to the errata patch.


>> These changes are done to cleanup serial.c file and prepare for
>> runtime changes.
>
> There are a lot of changes in this patch with very little description as
> to why, and many appear to be unrelated.  They should probably be
> separate patches, or have a better description as to how all the changes
> are related so they belong in the same patch.
>

okay fine will split them into smaller changes.


>> Signed-off-by: Govindraj.R <govindraj.raja at ti.com>
>> ---
>>  arch/arm/mach-omap2/serial.c                  |  132 +++++++++---------------
>>  arch/arm/plat-omap/include/plat/omap-serial.h |    4 +-
>>  drivers/tty/serial/omap-serial.c              |   12 ++-
>>  3 files changed, 61 insertions(+), 87 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
>> index c98b9b4..8c43d1c 100644
>> --- a/arch/arm/mach-omap2/serial.c
>> +++ b/arch/arm/mach-omap2/serial.c
>> @@ -68,14 +68,6 @@ struct omap_uart_state {
>>       int clocked;
>>
>>       int regshift;
>> -     void __iomem *membase;
>> -     resource_size_t mapbase;
>> -
>> -     struct list_head node;
>> -     struct omap_hwmod *oh;
>> -     struct platform_device *pdev;
>> -
>> -     u32 errata;
>>  #if defined(CONFIG_ARCH_OMAP3) && defined(CONFIG_PM)
>>       int context_valid;
>>
>> @@ -90,7 +82,6 @@ struct omap_uart_state {
>>  #endif
>>  };
>>
>> -static LIST_HEAD(uart_list);
>>  static u8 num_uarts;
>>
>>  static int uart_idle_hwmod(struct omap_device *od)
>> @@ -143,7 +134,19 @@ static inline void serial_write_reg(struct omap_uart_state *uart, int offset,
>>       __raw_writeb(value, uart->membase + offset);
>>  }
>>
>> -#if defined(CONFIG_PM) && defined(CONFIG_ARCH_OMAP3)
>> +struct omap_hwmod *omap_uart_hwmod_lookup(int num)
>> +{
>> +     struct omap_hwmod *oh;
>> +     char oh_name[MAX_UART_HWMOD_NAME_LEN];
>> +
>> +     snprintf(oh_name, MAX_UART_HWMOD_NAME_LEN, "uart%d", num + 1);
>> +     oh = omap_hwmod_lookup(oh_name);
>> +     WARN(IS_ERR(oh), "Could not lookup hmwod info for %s\n",
>> +                     oh_name);
>> +     return oh;
>> +}
>> +
>> +#if defined(CONFIG_PM)
>
> The CONFIG_ARCH_OMAP3 part of this #if was dropped with this change with
> no mention as to why.  (I understand why it was done, but it's not
> releveant to $SUBJECT patch so should be a separate patch.)
>
>>  /*
>>   * Work Around for Errata i202 (3430 - 1.12, 3630 - 1.6)
>> @@ -357,22 +360,17 @@ int omap_uart_can_sleep(void)
>>       return can_sleep;
>>  }
>>
>> -static void omap_uart_idle_init(struct omap_uart_state *uart)
>> +static void omap_uart_idle_init(struct omap_uart_port_info *uart,
>> +                             unsigned short num)
>>  {
>> -     int ret;
>> -
>> -     uart->can_sleep = 0;
>> -     omap_uart_smart_idle_enable(uart, 0);
>> -
>>       if (cpu_is_omap34xx() && !cpu_is_ti816x()) {
>> -             u32 mod = (uart->num > 1) ? OMAP3430_PER_MOD : CORE_MOD;
>> +             u32 mod = (num > 1) ? OMAP3430_PER_MOD : CORE_MOD;
>>               u32 wk_mask = 0;
>>               u32 padconf = 0;
>>
>> -             /* XXX These PRM accesses do not belong here */
>
> why?
>
>>               uart->wk_en = OMAP34XX_PRM_REGADDR(mod, PM_WKEN1);
>>               uart->wk_st = OMAP34XX_PRM_REGADDR(mod, PM_WKST1);
>> -             switch (uart->num) {
>> +             switch (num) {
>>               case 0:
>>                       wk_mask = OMAP3430_ST_UART1_MASK;
>>                       padconf = 0x182;
>> @@ -391,12 +389,11 @@ static void omap_uart_idle_init(struct omap_uart_state *uart)
>>                       break;
>>               }
>>               uart->wk_mask = wk_mask;
>> -             uart->padconf = padconf;
>
> The assignment is removed here, making all the rest of the padconf stuff
> that remains useless.
>
> However, a subsequent patch removes the mux stuff entirely, so I suggest
> you just drop this change from here.
>

okay will incorporate this as part of mux changes patch.


>>       } else if (cpu_is_omap24xx()) {
>>               u32 wk_mask = 0;
>>               u32 wk_en = PM_WKEN1, wk_st = PM_WKST1;
>>
>> -             switch (uart->num) {
>> +             switch (num) {
>>               case 0:
>>                       wk_mask = OMAP24XX_ST_UART1_MASK;
>>                       break;
>> @@ -421,7 +418,6 @@ static void omap_uart_idle_init(struct omap_uart_state *uart)
>>               uart->wk_en = NULL;
>>               uart->wk_st = NULL;
>>               uart->wk_mask = 0;
>> -             uart->padconf = 0;
>>       }
>>  }
>>
>> @@ -436,26 +432,13 @@ static void omap_uart_block_sleep(struct omap_uart_state *uart)
>>
>>  static int __init omap_serial_early_init(void)
>>  {
>> -     int i = 0;
>> -
>>       do {
>> -             char oh_name[MAX_UART_HWMOD_NAME_LEN];
>>               struct omap_hwmod *oh;
>> -             struct omap_uart_state *uart;
>>
>> -             snprintf(oh_name, MAX_UART_HWMOD_NAME_LEN,
>> -                      "uart%d", i + 1);
>> -             oh = omap_hwmod_lookup(oh_name);
>> +             oh = omap_uart_hwmod_lookup(num_uarts);
>>               if (!oh)
>>                       break;
>>
>> -             uart = kzalloc(sizeof(struct omap_uart_state), GFP_KERNEL);
>> -             if (WARN_ON(!uart))
>> -                     return -ENODEV;
>> -
>> -             uart->oh = oh;
>> -             uart->num = i++;
>> -             list_add_tail(&uart->node, &uart_list);
>>               num_uarts++;
>>
>>               /*
>> @@ -468,7 +451,7 @@ static int __init omap_serial_early_init(void)
>>                * to determine SoC specific init before omap_device
>>                * is ready.  Therefore, don't allow idle here
>>                */
>> -             uart->oh->flags |= HWMOD_INIT_NO_IDLE | HWMOD_INIT_NO_RESET;
>> +             oh->flags |= HWMOD_INIT_NO_IDLE | HWMOD_INIT_NO_RESET;
>>       } while (1);
>>
>>       return 0;
>> @@ -488,57 +471,47 @@ core_initcall(omap_serial_early_init);
>>   */
>>  void __init omap_serial_init_port(struct omap_board_data *bdata)
>>  {
>> -     struct omap_uart_state *uart;
>>       struct omap_hwmod *oh;
>>       struct platform_device *pdev;
>> -     void *pdata = NULL;
>> +     char *name = DRIVER_NAME;
>> +     struct omap_uart_port_info *pdata;
>>       u32 pdata_size = 0;
>> -     char *name;
>> -     struct omap_uart_port_info omap_up;
>>
>>       if (WARN_ON(!bdata))
>>               return;
>>       if (WARN_ON(bdata->id < 0))
>>               return;
>> -     if (WARN_ON(bdata->id >= num_uarts))
>> +     if (WARN_ON(bdata->id >= OMAP_MAX_HSUART_PORTS))
>
> why?  because of early_init, num_uarts is already the max number
> of UARTs available (based on hwmod probe.)
>

yes will correct this and use num_uarts

>>               return;
>>
>> -     list_for_each_entry(uart, &uart_list, node)
>> -             if (bdata->id == uart->num)
>> -                     break;
>> -
>> -     oh = uart->oh;
>> -     uart->dma_enabled = 0;
>> -     name = DRIVER_NAME;
>> +     oh = omap_uart_hwmod_lookup(bdata->id);
>> +     if (!oh)
>> +             return;
>>
>> -     omap_up.dma_enabled = uart->dma_enabled;
>> -     omap_up.uartclk = OMAP24XX_BASE_BAUD * 16;
>> -     omap_up.mapbase = oh->slaves[0]->addr->pa_start;
>> -     omap_up.membase = omap_hwmod_get_mpu_rt_va(oh);
>> -     omap_up.flags = UPF_BOOT_AUTOCONF | UPF_SHARE_IRQ;
>> +     pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
>> +     if (!pdata) {
>> +             pr_err("Memory allocation for UART pdata failed\n");
>> +             return;
>> +     }
>>
>> -     pdata = &omap_up;
>>       pdata_size = sizeof(struct omap_uart_port_info);
>> +     omap_uart_idle_init(pdata, bdata->id);
>
> Why was this moved here?
>
> ISTR that the order of this call relative to the hwmod/omap_device
> enable/disable calls below was important, especially in the DEBUG_LL
> case.
>
>> -     if (WARN_ON(!oh))
>> -             return;
>> +     pdata->uartclk = OMAP24XX_BASE_BAUD * 16;
>> +     pdata->flags = UPF_BOOT_AUTOCONF;
>> +
>> +     /* Enable the MDR1 errata for OMAP3 */
>> +     if (cpu_is_omap34xx() && !cpu_is_ti816x())
>> +             pdata->errata |= UART_ERRATA_i202_MDR1_ACCESS;
>>
>> -     pdev = omap_device_build(name, uart->num, oh, pdata, pdata_size,
>> -                            omap_uart_latency,
>> -                            ARRAY_SIZE(omap_uart_latency), false);
>> +     pdev = omap_device_build(name, bdata->id, oh, pdata,
>> +                             pdata_size, omap_uart_latency,
>> +                             ARRAY_SIZE(omap_uart_latency), false);
>
> Note the unecesary whitespace changes in this change.
>
>>       WARN(IS_ERR(pdev), "Could not build omap_device for %s: %s.\n",
>>            name, oh->name);
>>
>> -     omap_device_disable_idle_on_suspend(pdev);
>
> This should also be a separate patch at the end of the series, and is
> not related to the changes described in the changelog.
>

okay.

>>       oh->mux = omap_hwmod_mux_init(bdata->pads, bdata->pads_cnt);
>>
>> -     uart->regshift = 2;
>> -     uart->mapbase = oh->slaves[0]->addr->pa_start;
>> -     uart->membase = omap_hwmod_get_mpu_rt_va(oh);
>> -     uart->pdev = pdev;
>> -
>> -     oh->dev_attr = uart;
>> -
>>       console_lock(); /* in case the earlycon is on the UART */
>>
>>       /*
>> @@ -546,23 +519,18 @@ void __init omap_serial_init_port(struct omap_board_data *bdata)
>>        * on init.  Now that omap_device is ready, ensure full idle
>>        * before doing omap_device_enable().
>>        */
>> -     omap_hwmod_idle(uart->oh);
>> +     omap_hwmod_idle(oh);
>>
>> -     omap_device_enable(uart->pdev);
>> -     omap_uart_idle_init(uart);
>> -     omap_hwmod_enable_wakeup(uart->oh);
>> -     omap_device_idle(uart->pdev);
>> +     omap_device_enable(pdev);
>> +     omap_hwmod_enable_wakeup(oh);
>>
>> -     omap_uart_block_sleep(uart);
>>       console_unlock();
>>
>> -     if ((cpu_is_omap34xx() && uart->padconf) ||
>> -         (uart->wk_en && uart->wk_mask))
>> +     if ((cpu_is_omap34xx() && bdata->pads) ||
>> +             (pdata->wk_en && pdata->wk_mask))
>
> This change seems to belong as part of the mux patch.
>

okay moving to mux change patch.

>>               device_init_wakeup(&pdev->dev, true);
>>
>> -     /* Enable the MDR1 errata for OMAP3 */
>> -     if (cpu_is_omap34xx() && !cpu_is_ti816x())
>> -             uart->errata |= UART_ERRATA_i202_MDR1_ACCESS;
>> +     kfree(pdata);
>>  }
>>
>>  /**
>> @@ -574,11 +542,11 @@ void __init omap_serial_init_port(struct omap_board_data *bdata)
>>   */
>>  void __init omap_serial_init(void)
>>  {
>> -     struct omap_uart_state *uart;
>>       struct omap_board_data bdata;
>> +     u8 i;
>>
>> -     list_for_each_entry(uart, &uart_list, node) {
>> -             bdata.id = uart->num;
>> +     for (i = 0; i < num_uarts; i++) {
>> +             bdata.id = i;
>>               bdata.flags = 0;
>>               bdata.pads = NULL;
>>               bdata.pads_cnt = 0;
>> diff --git a/arch/arm/plat-omap/include/plat/omap-serial.h b/arch/arm/plat-omap/include/plat/omap-serial.h
>> index 307cd6f..0f061b4 100644
>> --- a/arch/arm/plat-omap/include/plat/omap-serial.h
>> +++ b/arch/arm/plat-omap/include/plat/omap-serial.h
>> @@ -59,9 +59,9 @@
>>  struct omap_uart_port_info {
>>       bool                    dma_enabled;    /* To specify DMA Mode */
>>       unsigned int            uartclk;        /* UART clock rate */
>> -     void __iomem            *membase;       /* ioremap cookie or NULL */
>> -     resource_size_t         mapbase;        /* resource base */
>>       upf_t                   flags;          /* UPF_* flags */
>> +
>> +     u32                     errata;
>>  };
>>
>>  struct uart_omap_dma {
>> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
>> index 5e713d3..6c2ea54 100644
>> --- a/drivers/tty/serial/omap-serial.c
>> +++ b/drivers/tty/serial/omap-serial.c
>> @@ -1275,10 +1275,16 @@ static int serial_omap_probe(struct platform_device *pdev)
>>       up->port.ops = &serial_omap_pops;
>>       up->port.line = pdev->id;
>>
>> -     up->port.membase = omap_up_info->membase;
>> -     up->port.mapbase = omap_up_info->mapbase;
>> +     up->port.mapbase = mem->start;
>> +     up->port.membase = ioremap(mem->start, resource_size(mem));
>> +
>> +     if (!up->port.membase) {
>> +             dev_err(&pdev->dev, "can't ioremap UART\n");
>> +             ret = -ENOMEM;
>> +             goto err;
>> +     }
>> +
>>       up->port.flags = omap_up_info->flags;
>> -     up->port.irqflags = omap_up_info->irqflags;
>>       up->port.uartclk = omap_up_info->uartclk;
>>       up->uart_dma.uart_base = mem->start;
>
> Kevin
> --
> To unsubscribe from this list: send the line "unsubscribe linux-serial" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



More information about the linux-arm-kernel mailing list