On Mon, Nov 22, 2010 at 17:38, DebBarma, Tarun Kanti <tarun.kanti@xxxxxx> wrote:
>
>> -----Original Message-----
>> From: Varadarajan, Charulatha
>> Sent: Monday, November 22, 2010 5:30 PM
>> To: DebBarma, Tarun Kanti
>> Cc: linux-omap@xxxxxxxxxxxxxxx
>> Subject: Re: [PATCHv4 12/14] OMAP: dmtimer: switch-over to platform device
>> driver
>>
>> On Mon, Nov 22, 2010 at 14:44, DebBarma, Tarun Kanti <tarun.kanti@xxxxxx>
>> wrote:
>> >> -----Original Message-----
>> >> From: Varadarajan, Charulatha
>> >> Sent: Monday, November 22, 2010 12:15 PM
>> >> To: DebBarma, Tarun Kanti
>> >> Cc: linux-omap@xxxxxxxxxxxxxxx
>> >> Subject: Re: [PATCHv4 12/14] OMAP: dmtimer: switch-over to platform
>> device
>> >> driver
>> >>
>> >> <<snip>>
>> >>
>> >> > -/*
>> >> > - * Reads timer registers in posted and non-posted mode. The posted
>> mode
>> >> bit
>> >> > - * is encoded in reg. Note that in posted mode write pending bit
>> must
>> >> be
>> >> > - * checked. Otherwise a read of a non completed write will produce
>> an
>> >> error.
>> >> > +/**
>> >> > + * omap_dm_timer_read_reg - read timer registers in posted and non-
>> >> posted mode
>> >> > + * @timer: timer pointer over which read operation to perform
>> >> > + * @reg: lowest byte holds the register offset
>> >> > + *
>> >> > + * The posted mode bit is encoded in reg. Note that in posted mode
>> >> write
>> >> > + * pending bit must be checked. Otherwise a read of a non completed
>> >> write
>> >> > + * will produce an error.
>> >> > */
>> >> > static inline u32 omap_dm_timer_read_reg(struct omap_dm_timer
>> *timer,
>> >> u32 reg)
>> >> > {
>> >> > + struct dmtimer_platform_data *pdata = timer->pdev-
>> >> >dev.platform_data;
>> >> > +
>> >> > + if (reg >= OMAP_TIMER_WAKEUP_EN_REG)
>> >> > + reg += pdata->func_offst;
>> >> > +
>> >> > if (timer->posted)
>> >> > - while (readl(timer->io_base +
>> (OMAP_TIMER_WRITE_PEND_REG
>> >> & 0xff))
>> >> > - & (reg >> WPSHIFT))
>> >> > + while (readl(timer->io_base +
>> >> > + ((OMAP_TIMER_WRITE_PEND_REG + pdata-
>> >func_offst)
>> >> > + & 0xff)) & (reg >> WPSHIFT))
>> >>
>> >> You may add a timeout in this.
>> > There are few reasons why I have not done:
>> > (1) This was in this way from the beginning and I thought some analysis
>> > Was done already for not adding timeout and so did not wish to alter
>> that.
>> > (2) wanted to keep low level functions as simple as possible by avoiding
>> > As many checks as possible unless really needed.
>> > (3) The client driver can take care of timeout. Even if timeout is
>> > Introduced in this api, the client driver anyways have to check for the
>> > Timeout.
>>
>> I don't think that client driver should take care of this. The client
>> driver should
>> check only for the return value. If timeout, return error.
> So what I am saying is it can add the timeout instead of checking for the
> Return value. This would keep the low level function simple.
> Please try to understand that this is a common function called by all the
> Client drivers associated with all the timers. If you can reduce a check
> The benefit is tremendous.
I do not agree. If low level driver functions are using these common read/write
functions, there is a possibility of getting struck up in the while loop since
there is no timeout. If you think that there is no possibility of timeout when
called from low level driver functions, remove the while loop unless if it is
really required.
If these functions uses while loops, please have a timeout error. Otherwise,
analyze and remove the while loop from these functions.
>
> --
> Tarun
>
>>
>> In your case, one of the calls to omap_dm_timer_write_reg() is as given
>> below:
>> omap2_gp_timer_init()
>> -> omap2_gp_clockevent_init()
>> -> omap_dm_timer_request_specific()
>> -> omap_dm_timer_prepare()
>> -> omap_dm_timer_write_reg()
>>
>> Here timeout is not taken care.
>>
>> >
>> > --
>> > Tarun
>> >>
>> >> > cpu_relax();
>> >> > return readl(timer->io_base + (reg & 0xff));
>> >> > }
>> >> >
>> >> > -/*
>> >> > - * Writes timer registers in posted and non-posted mode. The posted
>> >> mode bit
>> >> > - * is encoded in reg. Note that in posted mode the write pending bit
>> >> must be
>> >> > - * checked. Otherwise a write on a register which has a pending
>> write
>> >> will be
>> >> > - * lost.
>> >> > +/**
>> >> > + * omap_dm_timer_write_reg - write timer registers in posted and
>> non-
>> >> posted mode
>> >> > + * @timer: timer pointer over which write operation is to
>> perform
>> >> > + * @reg: lowest byte holds the register offset
>> >> > + * @value: data to write into the register
>> >> > + *
>> >> > + * The posted mode bit is encoded in reg. Note that in posted mode
>> the
>> >> write
>> >> > + * pending bit must be checked. Otherwise a write on a register
>> which
>> >> has a
>> >> > + * pending write will be lost.
>> >> > */
>> >> > static void omap_dm_timer_write_reg(struct omap_dm_timer *timer, u32
>> >> reg,
>> >> > u32 value)
>> >> > {
>> >> > + struct dmtimer_platform_data *pdata = timer->pdev-
>> >> >dev.platform_data;
>> >> > +
>> >> > + if (reg >= OMAP_TIMER_WAKEUP_EN_REG)
>> >> > + reg += pdata->func_offst;
>> >> > +
>> >> > if (timer->posted)
>> >> > - while (readl(timer->io_base +
>> (OMAP_TIMER_WRITE_PEND_REG
>> >> & 0xff))
>> >> > - & (reg >> WPSHIFT))
>> >> > + while (readl(timer->io_base +
>> >> > + ((OMAP_TIMER_WRITE_PEND_REG + pdata-
>> >func_offst)
>> >> > + & 0xff)) & (reg >> WPSHIFT))
>> >>
>> >> Ditto.
>> >>
>> >> > cpu_relax();
>> >> > writel(value, timer->io_base + (reg & 0xff));
>> >> > }
>> >>
>> >>
>> >> <<snip>>
>> >
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
