Re: [PATCHv5,RESEND 1/8] gpu: host1x: Add host1x driver

2013-02-06 Thread Terje Bergström 
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(>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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv5,RESEND 1/8] gpu: host1x: Add host1x driver

2013-02-06 Thread Terje Bergström 
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv5,RESEND 1/8] gpu: host1x: Add host1x driver

2013-02-04 Thread Thierry Reding 
On Mon, Feb 04, 2013 at 07:30:08PM -0800, Terje Bergström wrote:
> On 04.02.2013 01:09, Thierry Reding wrote:
> > On Tue, Jan 15, 2013 at 01:43:57PM +0200, Terje Bergstrom wrote:
> >> Add host1x, the driver for host1x and its client unit 2D.
> > 
> > Maybe this could be a bit more verbose. Perhaps describe what host1x is.
> 
> Sure. I could just steal the paragraph from Stephen:
> 
> The Tegra host1x module is the DMA engine for register access to Tegra's
> graphics- and multimedia-related modules. The modules served by host1x
> are referred to as clients. host1x includes some other  functionality,
> such as synchronization.

Yes, that sound good.

> >> + err = host1x_syncpt_init(host);
> >> + if (err)
> >> + return err;
> > [...]
> >> + host1x_syncpt_reset(host);
> > 
> > Why separate host1x_syncpt_reset() from host1x_syncpt_init()? I see why
> > it might be useful to have host1x_syncpt_reset() as a separate function
> > but couldn't it be called as part of host1x_syncpt_init()?
> 
> host1x_syncpt_init() is used for initializing the syncpt structures, and
> is called in probe. host1x_syncpt_reset() should be called whenever we
> think hardware state is lost, for example if VDD_CORE was rail gated due
> to system suspend.

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?

> >> +#include "hw/syncpt_hw.c"
> > 
> > Why include the source file here? Can't you compile it separately
> > instead?
> 
> It's because we need to compile with the hardware headers of that host1x
> version, because we haven't been good at keeping compatibility. So
> host1x01.c #includes version 01 headers, and syncpt_hw.c in this
> compilation unit gets compiled with that. 02 would include 02 headers,
> and syncpt_hw.c would get compiled with its register definitions etc.

Okay, fair enough.

> >> + */
> >> +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(>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?

> >> +/*
> >> + * Resets syncpoint and waitbase values to sw shadows
> >> + */
> >> +void host1x_syncpt_reset(struct host1x *dev)
> > 
> > Maybe host1x_syncpt_flush() would be a better name given the above
> > description? Reset does have this hardware reset connotation so my first
> > intuition had been that this would reset the syncpt value to 0.
> 
> Right, it actually reloads values stored in shadow registers back to
> host1x. Flush doesn't feel like it's conveying the meaning. Would
> host1x_syncpt_restore() work? That'd match with host1x_syncpt_save(),
> which just updates all shadow registers from hardware and is used just
> before host1x loses power.

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.
 */

> >> +/*
> >> + * 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;
> >> +}
> > 
> > I don't know I understand what this means exactly. Does it read the
> > value that hardware last incremented? Perhaps this will become clearer
> > when you add a comment to the syncpt_load_min() implementation.
> 
> It just loads the current syncpt value to shadow register. The shadow
> register is called min, because host1x tracks the range of sync point
> increments that hardware is still going to do, so min is the lower
> boundary of the range.
> 
> max tells what the sync point is expected to reach for hardware to be
> considered idle.
> 
> host1x will f.ex. nop out waits for sync point values outside the range,
> because hardware isn't good at handling syncpt value wrapping.

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

Re: [PATCHv5,RESEND 1/8] gpu: host1x: Add host1x driver

2013-02-04 Thread Terje Bergström 
On 04.02.2013 01:09, Thierry Reding wrote:
> On Tue, Jan 15, 2013 at 01:43:57PM +0200, Terje Bergstrom wrote:
>> Add host1x, the driver for host1x and its client unit 2D.
> 
> Maybe this could be a bit more verbose. Perhaps describe what host1x is.

Sure. I could just steal the paragraph from Stephen:

The Tegra host1x module is the DMA engine for register access to Tegra's
graphics- and multimedia-related modules. The modules served by host1x
are referred to as clients. host1x includes some other  functionality,
such as synchronization.

>> diff --git a/drivers/gpu/host1x/Kconfig b/drivers/gpu/host1x/Kconfig
> [...]
>> @@ -0,0 +1,6 @@
>> +config TEGRA_HOST1X
>> + tristate "Tegra host1x driver"
>> + help
>> +   Driver for the Tegra host1x hardware.
> 
> Maybe s/Tegra/NVIDIA Tegra/?

Sounds good.

>> +
>> +   Required for enabling tegradrm.
> 
> This should probably be dropped. Either encode such knowledge as
> explicit dependencies or in this case just remove it altogether since we
> will probably merge both drivers anyway.

I think this was left from previous versions. Now it just doesn't make
sense. I'll just drop it.

> 
>> diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
> [...]
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include "dev.h"
> 
> Maybe add a blank line between the previous two lines to visually
> separate standard Linux includes from driver-specific ones.

Ok. You commented in quite few places in a similar way. I'll fix all of
them to first include system includes, then driver's own includes, and
add a blank line in between.

> 
>> +#include "hw/host1x01.h"
>> +
>> +#define CREATE_TRACE_POINTS
>> +#include 
>> +
>> +#define DRIVER_NAME  "tegra-host1x"
> 
> You only ever use this once, so maybe it can just be dropped?

Yes.

> 
>> +static struct host1x_device_info host1x_info = {
> 
> Perhaps this should be host1x01_info in order to match the hardware
> revision? That'll avoid it having to be renamed later on when other
> revisions start to appear.

Ok, will do. I thought it'd be awkward being alone until the second
version appears, but I'll add it.

> 
>> +static int host1x_probe(struct platform_device *dev)
>> +{
> [...]
>> + syncpt_irq = platform_get_irq(dev, 0);
>> + if (IS_ERR_VALUE(syncpt_irq)) {
> 
> This is somewhat unusual. It should be fine to just do:
> 
> if (syncpt_irq < 0)
> 
> but IS_ERR_VALUE() should work fine too.

I'll use the simpler version.

> 
>> + memcpy(>info, devid->data, sizeof(struct host1x_device_info));
> 
> Why not make the .info field a pointer to struct host1x_device_info
> instead? That way you don't have to keep separate copies of the same
> information.

This had something to do with __init data and non-init data. But, we're
not really putting this data into __init, so we should be able to use
just a pointer.

> 
>> + /* set common host1x device data */
>> + platform_set_drvdata(dev, host);
>> +
>> + host->regs = devm_request_and_ioremap(>dev, regs);
>> + if (!host->regs) {
>> + dev_err(>dev, "failed to remap host registers\n");
>> + return -ENXIO;
>> + }
> 
> This should probably be rewritten as:
> 
> host->regs = devm_ioremap_resource(>dev, regs);
> if (IS_ERR(host->regs))
> return PTR_ERR(host->regs);
> 
> Though that function will only be available in 3.9-rc1.

Ok, 3.9-rc1 is fine as a basis.

>> + err = host1x_syncpt_init(host);
>> + if (err)
>> + return err;
> [...]
>> + host1x_syncpt_reset(host);
> 
> Why separate host1x_syncpt_reset() from host1x_syncpt_init()? I see why
> it might be useful to have host1x_syncpt_reset() as a separate function
> but couldn't it be called as part of host1x_syncpt_init()?

host1x_syncpt_init() is used for initializing the syncpt structures, and
is called in probe. host1x_syncpt_reset() should be called whenever we
think hardware state is lost, for example if VDD_CORE was rail gated due
to system suspend.

> 
>> + dev_info(>dev, "initialized\n");
> 
> I don't think this is very useful. We should make sure to tell people
> when things fail. When everything goes as planned we don't need to brag
> about it =)

True. I wish other kernel drivers followed that same philosophy. :-)
I'll remove that. It's mainly useful as debug help, but it's as easy to
check from sysfs the state.

> 
>> diff --git a/drivers/gpu/host1x/dev.h b/drivers/gpu/host1x/dev.h
> [...]
>> +struct host1x_syncpt_ops {
> [...]
>> + const char * (*name)(struct host1x_syncpt *);
> 
> Why do we need this? Could we not refer to the syncpt name directly
> instead of going through this wrapper? I'd expect the name to be static.

This must be a relic. I'll remove the wrapper.

> 
>> +struct host1x_device_info {
> 
> Maybe this should be called simply host1x_info? _device seems redundant.

Sure.

> 
>> + int nb_channels;

Re: [PATCHv5,RESEND 1/8] gpu: host1x: Add host1x driver

2013-02-04 Thread Thierry Reding 
On Tue, Jan 15, 2013 at 01:43:57PM +0200, Terje Bergstrom wrote:
> Add host1x, the driver for host1x and its client unit 2D.

Maybe this could be a bit more verbose. Perhaps describe what host1x is.

> diff --git a/drivers/gpu/host1x/Kconfig b/drivers/gpu/host1x/Kconfig
[...]
> @@ -0,0 +1,6 @@
> +config TEGRA_HOST1X
> + tristate "Tegra host1x driver"
> + help
> +   Driver for the Tegra host1x hardware.

Maybe s/Tegra/NVIDIA Tegra/?

> +
> +   Required for enabling tegradrm.

This should probably be dropped. Either encode such knowledge as
explicit dependencies or in this case just remove it altogether since we
will probably merge both drivers anyway.

> diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
[...]
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include "dev.h"

Maybe add a blank line between the previous two lines to visually
separate standard Linux includes from driver-specific ones.

> +#include "hw/host1x01.h"
> +
> +#define CREATE_TRACE_POINTS
> +#include 
> +
> +#define DRIVER_NAME  "tegra-host1x"

You only ever use this once, so maybe it can just be dropped?

> +static struct host1x_device_info host1x_info = {

Perhaps this should be host1x01_info in order to match the hardware
revision? That'll avoid it having to be renamed later on when other
revisions start to appear.

> +static int host1x_probe(struct platform_device *dev)
> +{
[...]
> + syncpt_irq = platform_get_irq(dev, 0);
> + if (IS_ERR_VALUE(syncpt_irq)) {

This is somewhat unusual. It should be fine to just do:

if (syncpt_irq < 0)

but IS_ERR_VALUE() should work fine too.

> + memcpy(>info, devid->data, sizeof(struct host1x_device_info));

Why not make the .info field a pointer to struct host1x_device_info
instead? That way you don't have to keep separate copies of the same
information.

> + /* set common host1x device data */
> + platform_set_drvdata(dev, host);
> +
> + host->regs = devm_request_and_ioremap(>dev, regs);
> + if (!host->regs) {
> + dev_err(>dev, "failed to remap host registers\n");
> + return -ENXIO;
> + }

This should probably be rewritten as:

host->regs = devm_ioremap_resource(>dev, regs);
if (IS_ERR(host->regs))
return PTR_ERR(host->regs);

Though that function will only be available in 3.9-rc1.

> + err = host1x_syncpt_init(host);
> + if (err)
> + return err;
[...]
> + host1x_syncpt_reset(host);

Why separate host1x_syncpt_reset() from host1x_syncpt_init()? I see why
it might be useful to have host1x_syncpt_reset() as a separate function
but couldn't it be called as part of host1x_syncpt_init()?

> + dev_info(>dev, "initialized\n");

I don't think this is very useful. We should make sure to tell people
when things fail. When everything goes as planned we don't need to brag
about it =)

> diff --git a/drivers/gpu/host1x/dev.h b/drivers/gpu/host1x/dev.h
[...]
> +struct host1x_syncpt_ops {
[...]
> + const char * (*name)(struct host1x_syncpt *);

Why do we need this? Could we not refer to the syncpt name directly
instead of going through this wrapper? I'd expect the name to be static.

> +struct host1x_device_info {

Maybe this should be called simply host1x_info? _device seems redundant.

> + int nb_channels;/* host1x: num channels supported */
> + int nb_pts; /* host1x: num syncpoints supported */
> + int nb_bases;   /* host1x: num syncpoints supported */
> + int nb_mlocks;  /* host1x: number of mlocks */
> + int (*init)(struct host1x *); /* initialize per SoC ops */
> + int sync_offset;
> +};

While this isn't public API, maybe it would still be useful to turn the
comments into proper kerneldoc? That's what people are used to.

> +struct host1x {
> + void __iomem *regs;
> + struct host1x_syncpt *syncpt;
> + struct platform_device *dev;
> + struct host1x_device_info info;
> + struct clk *clk;
> +
> + struct host1x_syncpt_ops syncpt_op;

Maybe make this a struct host1x_syncpt_ops * instead so you don't have
separate copies? While at it, maybe this should be const as well.

> + struct dentry *debugfs;

This doesn't seem to be used anywhere.

> +static inline
> +struct host1x *host1x_get_host(struct platform_device *_dev)
> +{
> + struct platform_device *pdev;
> +
> + if (_dev->dev.parent) {
> + pdev = to_platform_device(_dev->dev.parent);
> + return platform_get_drvdata(pdev);
> + } else
> + return platform_get_drvdata(_dev);
> +}

There is a lot of needless casting in here. Why not pass in a struct
device * and use dev_get_drvdata() instead?

> diff --git a/drivers/gpu/host1x/hw/host1x01.c 
> b/drivers/gpu/host1x/hw/host1x01.c
[...]
> +#include "hw/host1x01.h"
> +#include "dev.h"
> +#include "hw/host1x01_hardware.h"

The

Re: [PATCHv5,RESEND 1/8] gpu: host1x: Add host1x driver

2013-02-04 Thread Thierry Reding 
On Tue, Jan 15, 2013 at 01:43:57PM +0200, Terje Bergstrom wrote:
 Add host1x, the driver for host1x and its client unit 2D.

Maybe this could be a bit more verbose. Perhaps describe what host1x is.

 diff --git a/drivers/gpu/host1x/Kconfig b/drivers/gpu/host1x/Kconfig
[...]
 @@ -0,0 +1,6 @@
 +config TEGRA_HOST1X
 + tristate Tegra host1x driver
 + help
 +   Driver for the Tegra host1x hardware.

Maybe s/Tegra/NVIDIA Tegra/?

 +
 +   Required for enabling tegradrm.

This should probably be dropped. Either encode such knowledge as
explicit dependencies or in this case just remove it altogether since we
will probably merge both drivers anyway.

 diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
[...]
 +#include linux/module.h
 +#include linux/list.h
 +#include linux/slab.h
 +#include linux/of.h
 +#include linux/of_device.h
 +#include linux/clk.h
 +#include linux/io.h
 +#include dev.h

Maybe add a blank line between the previous two lines to visually
separate standard Linux includes from driver-specific ones.

 +#include hw/host1x01.h
 +
 +#define CREATE_TRACE_POINTS
 +#include trace/events/host1x.h
 +
 +#define DRIVER_NAME  tegra-host1x

You only ever use this once, so maybe it can just be dropped?

 +static struct host1x_device_info host1x_info = {

Perhaps this should be host1x01_info in order to match the hardware
revision? That'll avoid it having to be renamed later on when other
revisions start to appear.

 +static int host1x_probe(struct platform_device *dev)
 +{
[...]
 + syncpt_irq = platform_get_irq(dev, 0);
 + if (IS_ERR_VALUE(syncpt_irq)) {

This is somewhat unusual. It should be fine to just do:

if (syncpt_irq  0)

but IS_ERR_VALUE() should work fine too.

 + memcpy(host-info, devid-data, sizeof(struct host1x_device_info));

Why not make the .info field a pointer to struct host1x_device_info
instead? That way you don't have to keep separate copies of the same
information.

 + /* set common host1x device data */
 + platform_set_drvdata(dev, host);
 +
 + host-regs = devm_request_and_ioremap(dev-dev, regs);
 + if (!host-regs) {
 + dev_err(dev-dev, failed to remap host registers\n);
 + return -ENXIO;
 + }

This should probably be rewritten as:

host-regs = devm_ioremap_resource(dev-dev, regs);
if (IS_ERR(host-regs))
return PTR_ERR(host-regs);

Though that function will only be available in 3.9-rc1.

 + err = host1x_syncpt_init(host);
 + if (err)
 + return err;
[...]
 + host1x_syncpt_reset(host);

Why separate host1x_syncpt_reset() from host1x_syncpt_init()? I see why
it might be useful to have host1x_syncpt_reset() as a separate function
but couldn't it be called as part of host1x_syncpt_init()?

 + dev_info(dev-dev, initialized\n);

I don't think this is very useful. We should make sure to tell people
when things fail. When everything goes as planned we don't need to brag
about it =)

 diff --git a/drivers/gpu/host1x/dev.h b/drivers/gpu/host1x/dev.h
[...]
 +struct host1x_syncpt_ops {
[...]
 + const char * (*name)(struct host1x_syncpt *);

Why do we need this? Could we not refer to the syncpt name directly
instead of going through this wrapper? I'd expect the name to be static.

 +struct host1x_device_info {

Maybe this should be called simply host1x_info? _device seems redundant.

 + int nb_channels;/* host1x: num channels supported */
 + int nb_pts; /* host1x: num syncpoints supported */
 + int nb_bases;   /* host1x: num syncpoints supported */
 + int nb_mlocks;  /* host1x: number of mlocks */
 + int (*init)(struct host1x *); /* initialize per SoC ops */
 + int sync_offset;
 +};

While this isn't public API, maybe it would still be useful to turn the
comments into proper kerneldoc? That's what people are used to.

 +struct host1x {
 + void __iomem *regs;
 + struct host1x_syncpt *syncpt;
 + struct platform_device *dev;
 + struct host1x_device_info info;
 + struct clk *clk;
 +
 + struct host1x_syncpt_ops syncpt_op;

Maybe make this a struct host1x_syncpt_ops * instead so you don't have
separate copies? While at it, maybe this should be const as well.

 + struct dentry *debugfs;

This doesn't seem to be used anywhere.

 +static inline
 +struct host1x *host1x_get_host(struct platform_device *_dev)
 +{
 + struct platform_device *pdev;
 +
 + if (_dev-dev.parent) {
 + pdev = to_platform_device(_dev-dev.parent);
 + return platform_get_drvdata(pdev);
 + } else
 + return platform_get_drvdata(_dev);
 +}

There is a lot of needless casting in here. Why not pass in a struct
device * and use dev_get_drvdata() instead?

 diff --git a/drivers/gpu/host1x/hw/host1x01.c 
 b/drivers/gpu/host1x/hw/host1x01.c
[...]
 +#include hw/host1x01.h
 +#include dev.h
 +#include

Re: [PATCHv5,RESEND 1/8] gpu: host1x: Add host1x driver

2013-02-04 Thread Terje Bergström 
On 04.02.2013 01:09, Thierry Reding wrote:
 On Tue, Jan 15, 2013 at 01:43:57PM +0200, Terje Bergstrom wrote:
 Add host1x, the driver for host1x and its client unit 2D.
 
 Maybe this could be a bit more verbose. Perhaps describe what host1x is.

Sure. I could just steal the paragraph from Stephen:

The Tegra host1x module is the DMA engine for register access to Tegra's
graphics- and multimedia-related modules. The modules served by host1x
are referred to as clients. host1x includes some other  functionality,
such as synchronization.

 diff --git a/drivers/gpu/host1x/Kconfig b/drivers/gpu/host1x/Kconfig
 [...]
 @@ -0,0 +1,6 @@
 +config TEGRA_HOST1X
 + tristate Tegra host1x driver
 + help
 +   Driver for the Tegra host1x hardware.
 
 Maybe s/Tegra/NVIDIA Tegra/?

Sounds good.

 +
 +   Required for enabling tegradrm.
 
 This should probably be dropped. Either encode such knowledge as
 explicit dependencies or in this case just remove it altogether since we
 will probably merge both drivers anyway.

I think this was left from previous versions. Now it just doesn't make
sense. I'll just drop it.

 
 diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
 [...]
 +#include linux/module.h
 +#include linux/list.h
 +#include linux/slab.h
 +#include linux/of.h
 +#include linux/of_device.h
 +#include linux/clk.h
 +#include linux/io.h
 +#include dev.h
 
 Maybe add a blank line between the previous two lines to visually
 separate standard Linux includes from driver-specific ones.

Ok. You commented in quite few places in a similar way. I'll fix all of
them to first include system includes, then driver's own includes, and
add a blank line in between.

 
 +#include hw/host1x01.h
 +
 +#define CREATE_TRACE_POINTS
 +#include trace/events/host1x.h
 +
 +#define DRIVER_NAME  tegra-host1x
 
 You only ever use this once, so maybe it can just be dropped?

Yes.

 
 +static struct host1x_device_info host1x_info = {
 
 Perhaps this should be host1x01_info in order to match the hardware
 revision? That'll avoid it having to be renamed later on when other
 revisions start to appear.

Ok, will do. I thought it'd be awkward being alone until the second
version appears, but I'll add it.

 
 +static int host1x_probe(struct platform_device *dev)
 +{
 [...]
 + syncpt_irq = platform_get_irq(dev, 0);
 + if (IS_ERR_VALUE(syncpt_irq)) {
 
 This is somewhat unusual. It should be fine to just do:
 
 if (syncpt_irq  0)
 
 but IS_ERR_VALUE() should work fine too.

I'll use the simpler version.

 
 + memcpy(host-info, devid-data, sizeof(struct host1x_device_info));
 
 Why not make the .info field a pointer to struct host1x_device_info
 instead? That way you don't have to keep separate copies of the same
 information.

This had something to do with __init data and non-init data. But, we're
not really putting this data into __init, so we should be able to use
just a pointer.

 
 + /* set common host1x device data */
 + platform_set_drvdata(dev, host);
 +
 + host-regs = devm_request_and_ioremap(dev-dev, regs);
 + if (!host-regs) {
 + dev_err(dev-dev, failed to remap host registers\n);
 + return -ENXIO;
 + }
 
 This should probably be rewritten as:
 
 host-regs = devm_ioremap_resource(dev-dev, regs);
 if (IS_ERR(host-regs))
 return PTR_ERR(host-regs);
 
 Though that function will only be available in 3.9-rc1.

Ok, 3.9-rc1 is fine as a basis.

 + err = host1x_syncpt_init(host);
 + if (err)
 + return err;
 [...]
 + host1x_syncpt_reset(host);
 
 Why separate host1x_syncpt_reset() from host1x_syncpt_init()? I see why
 it might be useful to have host1x_syncpt_reset() as a separate function
 but couldn't it be called as part of host1x_syncpt_init()?

host1x_syncpt_init() is used for initializing the syncpt structures, and
is called in probe. host1x_syncpt_reset() should be called whenever we
think hardware state is lost, for example if VDD_CORE was rail gated due
to system suspend.

 
 + dev_info(dev-dev, initialized\n);
 
 I don't think this is very useful. We should make sure to tell people
 when things fail. When everything goes as planned we don't need to brag
 about it =)

True. I wish other kernel drivers followed that same philosophy. :-)
I'll remove that. It's mainly useful as debug help, but it's as easy to
check from sysfs the state.

 
 diff --git a/drivers/gpu/host1x/dev.h b/drivers/gpu/host1x/dev.h
 [...]
 +struct host1x_syncpt_ops {
 [...]
 + const char * (*name)(struct host1x_syncpt *);
 
 Why do we need this? Could we not refer to the syncpt name directly
 instead of going through this wrapper? I'd expect the name to be static.

This must be a relic. I'll remove the wrapper.

 
 +struct host1x_device_info {
 
 Maybe this should be called simply host1x_info? _device seems redundant.

Sure.

 
 + int nb_channels;/* host1x: num channels supported */
 + int

Re: [PATCHv5,RESEND 1/8] gpu: host1x: Add host1x driver

2013-02-04 Thread Thierry Reding 
On Mon, Feb 04, 2013 at 07:30:08PM -0800, Terje Bergström wrote:
 On 04.02.2013 01:09, Thierry Reding wrote:
  On Tue, Jan 15, 2013 at 01:43:57PM +0200, Terje Bergstrom wrote:
  Add host1x, the driver for host1x and its client unit 2D.
  
  Maybe this could be a bit more verbose. Perhaps describe what host1x is.
 
 Sure. I could just steal the paragraph from Stephen:
 
 The Tegra host1x module is the DMA engine for register access to Tegra's
 graphics- and multimedia-related modules. The modules served by host1x
 are referred to as clients. host1x includes some other  functionality,
 such as synchronization.

Yes, that sound good.

  + err = host1x_syncpt_init(host);
  + if (err)
  + return err;
  [...]
  + host1x_syncpt_reset(host);
  
  Why separate host1x_syncpt_reset() from host1x_syncpt_init()? I see why
  it might be useful to have host1x_syncpt_reset() as a separate function
  but couldn't it be called as part of host1x_syncpt_init()?
 
 host1x_syncpt_init() is used for initializing the syncpt structures, and
 is called in probe. host1x_syncpt_reset() should be called whenever we
 think hardware state is lost, for example if VDD_CORE was rail gated due
 to system suspend.

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?

  +#include hw/syncpt_hw.c
  
  Why include the source file here? Can't you compile it separately
  instead?
 
 It's because we need to compile with the hardware headers of that host1x
 version, because we haven't been good at keeping compatibility. So
 host1x01.c #includes version 01 headers, and syncpt_hw.c in this
 compilation unit gets compiled with that. 02 would include 02 headers,
 and syncpt_hw.c would get compiled with its register definitions etc.

Okay, fair enough.

  + */
  +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?

  +/*
  + * Resets syncpoint and waitbase values to sw shadows
  + */
  +void host1x_syncpt_reset(struct host1x *dev)
  
  Maybe host1x_syncpt_flush() would be a better name given the above
  description? Reset does have this hardware reset connotation so my first
  intuition had been that this would reset the syncpt value to 0.
 
 Right, it actually reloads values stored in shadow registers back to
 host1x. Flush doesn't feel like it's conveying the meaning. Would
 host1x_syncpt_restore() work? That'd match with host1x_syncpt_save(),
 which just updates all shadow registers from hardware and is used just
 before host1x loses power.

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.
 */

  +/*
  + * 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;
  +}
  
  I don't know I understand what this means exactly. Does it read the
  value that hardware last incremented? Perhaps this will become clearer
  when you add a comment to the syncpt_load_min() implementation.
 
 It just loads the current syncpt value to shadow register. The shadow
 register is called min, because host1x tracks the range of sync point
 increments that hardware is still going to do, so min is the lower
 boundary of the range.
 
 max tells what the sync point is expected to reach for hardware to be
 considered idle.
 
 host1x will f.ex. nop out waits for sync point values outside the range,
 because hardware isn't good at handling syncpt value wrapping.

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).

  Also the syncpoint is not actually allocated here, so maybe
  host1x_syncpt_request() would be a better name. As a nice side-effect it
  makes the naming more similar to the

[PATCHv5,RESEND 1/8] gpu: host1x: Add host1x driver

2013-01-15 Thread Terje Bergstrom 
Add host1x, the driver for host1x and its client unit 2D.

Signed-off-by: Terje Bergstrom 
---
 drivers/gpu/Makefile  |1 +
 drivers/gpu/host1x/Kconfig|6 +
 drivers/gpu/host1x/Makefile   |8 ++
 drivers/gpu/host1x/dev.c  |  161 +
 drivers/gpu/host1x/dev.h  |   73 ++
 drivers/gpu/host1x/hw/Makefile|6 +
 drivers/gpu/host1x/hw/host1x01.c  |   35 +
 drivers/gpu/host1x/hw/host1x01.h  |   25 
 drivers/gpu/host1x/hw/host1x01_hardware.h |   26 
 drivers/gpu/host1x/hw/hw_host1x01_sync.h  |   72 ++
 drivers/gpu/host1x/hw/syncpt_hw.c |  146 +++
 drivers/gpu/host1x/syncpt.c   |  217 +
 drivers/gpu/host1x/syncpt.h   |  153 
 drivers/video/Kconfig |2 +
 include/trace/events/host1x.h |   61 
 15 files changed, 992 insertions(+)
 create mode 100644 drivers/gpu/host1x/Kconfig
 create mode 100644 drivers/gpu/host1x/Makefile
 create mode 100644 drivers/gpu/host1x/dev.c
 create mode 100644 drivers/gpu/host1x/dev.h
 create mode 100644 drivers/gpu/host1x/hw/Makefile
 create mode 100644 drivers/gpu/host1x/hw/host1x01.c
 create mode 100644 drivers/gpu/host1x/hw/host1x01.h
 create mode 100644 drivers/gpu/host1x/hw/host1x01_hardware.h
 create mode 100644 drivers/gpu/host1x/hw/hw_host1x01_sync.h
 create mode 100644 drivers/gpu/host1x/hw/syncpt_hw.c
 create mode 100644 drivers/gpu/host1x/syncpt.c
 create mode 100644 drivers/gpu/host1x/syncpt.h
 create mode 100644 include/trace/events/host1x.h

diff --git a/drivers/gpu/Makefile b/drivers/gpu/Makefile
index cc92778..7e227097 100644
--- a/drivers/gpu/Makefile
+++ b/drivers/gpu/Makefile
@@ -1 +1,2 @@
 obj-y  += drm/ vga/ stub/
+obj-$(CONFIG_TEGRA_HOST1X) += host1x/
diff --git a/drivers/gpu/host1x/Kconfig b/drivers/gpu/host1x/Kconfig
new file mode 100644
index 000..e89fb2b
--- /dev/null
+++ b/drivers/gpu/host1x/Kconfig
@@ -0,0 +1,6 @@
+config TEGRA_HOST1X
+   tristate "Tegra host1x driver"
+   help
+ Driver for the Tegra host1x hardware.
+
+ Required for enabling tegradrm.
diff --git a/drivers/gpu/host1x/Makefile b/drivers/gpu/host1x/Makefile
new file mode 100644
index 000..363e6ab
--- /dev/null
+++ b/drivers/gpu/host1x/Makefile
@@ -0,0 +1,8 @@
+ccflags-y = -Idrivers/gpu/host1x
+
+host1x-y = \
+   syncpt.o \
+   dev.o \
+   hw/host1x01.o
+
+obj-$(CONFIG_TEGRA_HOST1X) += host1x.o
diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
new file mode 100644
index 000..cd2b1ef
--- /dev/null
+++ b/drivers/gpu/host1x/dev.c
@@ -0,0 +1,161 @@
+/*
+ * Tegra host1x driver
+ *
+ * Copyright (c) 2010-2013, NVIDIA Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see .
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "dev.h"
+#include "hw/host1x01.h"
+
+#define CREATE_TRACE_POINTS
+#include 
+
+#define DRIVER_NAME"tegra-host1x"
+
+void host1x_sync_writel(struct host1x *host1x, u32 v, u32 r)
+{
+   void __iomem *sync_regs = host1x->regs + host1x->info.sync_offset;
+
+   writel(v, sync_regs + r);
+}
+
+u32 host1x_sync_readl(struct host1x *host1x, u32 r)
+{
+   void __iomem *sync_regs = host1x->regs + host1x->info.sync_offset;
+
+   return readl(sync_regs + r);
+}
+
+static struct host1x_device_info host1x_info = {
+   .nb_channels= 8,
+   .nb_pts = 32,
+   .nb_mlocks  = 16,
+   .nb_bases   = 8,
+   .init   = host1x01_init,
+   .sync_offset= 0x3000,
+};
+
+static struct of_device_id host1x_match[] = {
+   { .compatible = "nvidia,tegra30-host1x", .data = _info, },
+   { .compatible = "nvidia,tegra20-host1x", .data = _info, },
+   { },
+};
+
+static int host1x_probe(struct platform_device *dev)
+{
+   struct host1x *host;
+   struct resource *regs;
+   int syncpt_irq;
+   int err;
+   const struct of_device_id *devid =
+   of_match_device(host1x_match, >dev);
+
+   if (!devid)
+   return -EINVAL;
+
+   regs = platform_get_resource(dev, IORESOURCE_MEM, 0);
+   if (!regs) {
+   dev_err(>dev, "missing regs\n");
+   return -ENXIO;
+   }
+
+

[PATCHv5,RESEND 1/8] gpu: host1x: Add host1x driver

2013-01-15 Thread Terje Bergstrom 
Add host1x, the driver for host1x and its client unit 2D.

Signed-off-by: Terje Bergstrom tbergst...@nvidia.com
---
 drivers/gpu/Makefile  |1 +
 drivers/gpu/host1x/Kconfig|6 +
 drivers/gpu/host1x/Makefile   |8 ++
 drivers/gpu/host1x/dev.c  |  161 +
 drivers/gpu/host1x/dev.h  |   73 ++
 drivers/gpu/host1x/hw/Makefile|6 +
 drivers/gpu/host1x/hw/host1x01.c  |   35 +
 drivers/gpu/host1x/hw/host1x01.h  |   25 
 drivers/gpu/host1x/hw/host1x01_hardware.h |   26 
 drivers/gpu/host1x/hw/hw_host1x01_sync.h  |   72 ++
 drivers/gpu/host1x/hw/syncpt_hw.c |  146 +++
 drivers/gpu/host1x/syncpt.c   |  217 +
 drivers/gpu/host1x/syncpt.h   |  153 
 drivers/video/Kconfig |2 +
 include/trace/events/host1x.h |   61 
 15 files changed, 992 insertions(+)
 create mode 100644 drivers/gpu/host1x/Kconfig
 create mode 100644 drivers/gpu/host1x/Makefile
 create mode 100644 drivers/gpu/host1x/dev.c
 create mode 100644 drivers/gpu/host1x/dev.h
 create mode 100644 drivers/gpu/host1x/hw/Makefile
 create mode 100644 drivers/gpu/host1x/hw/host1x01.c
 create mode 100644 drivers/gpu/host1x/hw/host1x01.h
 create mode 100644 drivers/gpu/host1x/hw/host1x01_hardware.h
 create mode 100644 drivers/gpu/host1x/hw/hw_host1x01_sync.h
 create mode 100644 drivers/gpu/host1x/hw/syncpt_hw.c
 create mode 100644 drivers/gpu/host1x/syncpt.c
 create mode 100644 drivers/gpu/host1x/syncpt.h
 create mode 100644 include/trace/events/host1x.h

diff --git a/drivers/gpu/Makefile b/drivers/gpu/Makefile
index cc92778..7e227097 100644
--- a/drivers/gpu/Makefile
+++ b/drivers/gpu/Makefile
@@ -1 +1,2 @@
 obj-y  += drm/ vga/ stub/
+obj-$(CONFIG_TEGRA_HOST1X) += host1x/
diff --git a/drivers/gpu/host1x/Kconfig b/drivers/gpu/host1x/Kconfig
new file mode 100644
index 000..e89fb2b
--- /dev/null
+++ b/drivers/gpu/host1x/Kconfig
@@ -0,0 +1,6 @@
+config TEGRA_HOST1X
+   tristate Tegra host1x driver
+   help
+ Driver for the Tegra host1x hardware.
+
+ Required for enabling tegradrm.
diff --git a/drivers/gpu/host1x/Makefile b/drivers/gpu/host1x/Makefile
new file mode 100644
index 000..363e6ab
--- /dev/null
+++ b/drivers/gpu/host1x/Makefile
@@ -0,0 +1,8 @@
+ccflags-y = -Idrivers/gpu/host1x
+
+host1x-y = \
+   syncpt.o \
+   dev.o \
+   hw/host1x01.o
+
+obj-$(CONFIG_TEGRA_HOST1X) += host1x.o
diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
new file mode 100644
index 000..cd2b1ef
--- /dev/null
+++ b/drivers/gpu/host1x/dev.c
@@ -0,0 +1,161 @@
+/*
+ * Tegra host1x driver
+ *
+ * Copyright (c) 2010-2013, NVIDIA Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see http://www.gnu.org/licenses/.
+ */
+
+#include linux/module.h
+#include linux/list.h
+#include linux/slab.h
+#include linux/of.h
+#include linux/of_device.h
+#include linux/clk.h
+#include linux/io.h
+#include dev.h
+#include hw/host1x01.h
+
+#define CREATE_TRACE_POINTS
+#include trace/events/host1x.h
+
+#define DRIVER_NAMEtegra-host1x
+
+void host1x_sync_writel(struct host1x *host1x, u32 v, u32 r)
+{
+   void __iomem *sync_regs = host1x-regs + host1x-info.sync_offset;
+
+   writel(v, sync_regs + r);
+}
+
+u32 host1x_sync_readl(struct host1x *host1x, u32 r)
+{
+   void __iomem *sync_regs = host1x-regs + host1x-info.sync_offset;
+
+   return readl(sync_regs + r);
+}
+
+static struct host1x_device_info host1x_info = {
+   .nb_channels= 8,
+   .nb_pts = 32,
+   .nb_mlocks  = 16,
+   .nb_bases   = 8,
+   .init   = host1x01_init,
+   .sync_offset= 0x3000,
+};
+
+static struct of_device_id host1x_match[] = {
+   { .compatible = nvidia,tegra30-host1x, .data = host1x_info, },
+   { .compatible = nvidia,tegra20-host1x, .data = host1x_info, },
+   { },
+};
+
+static int host1x_probe(struct platform_device *dev)
+{
+   struct host1x *host;
+   struct resource *regs;
+   int syncpt_irq;
+   int err;
+   const struct of_device_id *devid =
+   of_match_device(host1x_match, dev-dev);
+
+   if (!devid)
+   return -EINVAL;
+
+   regs = platform_get_resource(dev, IORESOURCE_MEM, 0);
+