Messages in this thread |  | | Date | Wed, 6 Feb 2013 12:13:40 -0800 | From | Terje Bergström <> | Subject | Re: [PATCHv5,RESEND 1/8] gpu: host1x: Add host1x driver |
| |
On 04.02.2013 23:43, Thierry Reding wrote: > My point was that you could include the call to host1x_syncpt_reset() > within host1x_syncpt_init(). That will keep unneeded code out of the > host1x_probe() function. Also you don't want to use the syncpoints > uninitialized, right?
Of course, sorry, I misunderstood. That makes a lot of sense.
>>>> + */ >>>> +static u32 syncpt_load_min(struct host1x_syncpt *sp) >>>> +{ >>>> + struct host1x *dev = sp->dev; >>>> + u32 old, live; >>>> + >>>> + do { >>>> + old = host1x_syncpt_read_min(sp); >>>> + live = host1x_sync_readl(dev, >>>> + HOST1X_SYNC_SYNCPT_0 + sp->id * 4); >>>> + } while ((u32)atomic_cmpxchg(&sp->min_val, old, live) != old); >>> >>> I think this warrants a comment. >> >> Sure. It just loops in case there's a race writing to min_val. > > Oh, I see. That'd make a good comment. Is the cast to (u32) really > necessary?
I'll add a comment. atomic_cmpxchg returns a signed value, so I think the cast is needed.
> Save/restore has the disadvantage of the direction not being implicit. > Save could mean save to hardware or save to software. The same is true > for restore. However if the direction is clearly defined, save and > restore work for me. > > Maybe the comment could be changed to be more explicit. Something like: > > /* > * Write cached syncpoint and waitbase values to hardware. > */ > > And for host1x_syncpt_save(): > > /* > * For client-managed registers, update the cached syncpoint and > * waitbase values by reading from the registers. > */
I was using save in the same way as f.ex. i915 (i915_suspend.c): save state of hardware to RAM, restore state from RAM. I'll add proper comments, but save and restore are for all syncpts, not only client managed.
> >>>> +/* >>>> + * Updates the last value read from hardware. >>>> + */ >>>> +u32 host1x_syncpt_load_min(struct host1x_syncpt *sp) >>>> +{ >>>> + u32 val; >>>> + val = sp->dev->syncpt_op.load_min(sp); >>>> + trace_host1x_syncpt_load_min(sp->id, val); >>>> + >>>> + return val; >>>> +} > Maybe the function should be called host1x_syncpt_load() if there is no > equivalent way to load the maximum value (since there is no register to > read from).
Sounds good. Maximum is just a software concept.
> That's certainly true for interrupts. However, if you look at the DMA > subsystem for example, you can also request an unnamed resource. > > The difference is sufficiently subtle that host1x_syncpt_allocate() > would work for me too, though. I just have a slight preference for > host1x_syncpt_request().
I don't really have a strong preference, so I'll follow your suggestion.
Terje
|  |