| Message ID | 1358250244-9678-2-git-send-email-tbergstrom@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 "hw/host1x01_hardware.h" The ordering here looks funny. > +#include "hw/syncpt_hw.c" Why include the source file here? Can't you compile it separately instead? > diff --git a/drivers/gpu/host1x/hw/host1x01.h b/drivers/gpu/host1x/hw/host1x01.h [...] > +int host1x01_init(struct host1x *); For completeness you should probably name the parameter, even if this is a prototype. > diff --git a/drivers/gpu/host1x/hw/host1x01_hardware.h b/drivers/gpu/host1x/hw/host1x01_hardware.h [...] > +#include <linux/types.h> > +#include <linux/bitops.h> > +#include "hw_host1x01_sync.h" Again, a blank line might help between the above two. I also assume that this file will be filled with more content later on, so I guess it's not worth the trouble to postpone it's creation until a later point. > diff --git a/drivers/gpu/host1x/hw/hw_host1x01_sync.h b/drivers/gpu/host1x/hw/hw_host1x01_sync.h [...] > +static inline u32 host1x_sync_syncpt_0_r(void) > +{ > + return 0x400; > +} > +#define HOST1X_SYNC_SYNCPT_0 \ > + host1x_sync_syncpt_0_r() > +static inline u32 host1x_sync_syncpt_base_0_r(void) > +{ > + return 0x600; > +} > +#define HOST1X_SYNC_SYNCPT_BASE_0 \ > + host1x_sync_syncpt_base_0_r() > +static inline u32 host1x_sync_syncpt_cpu_incr_r(void) > +{ > + return 0x700; > +} Perhaps it would be useful to modify these to take the syncpt ID as a parameter? That way you don't have to remember to do the multiplication everytime you access the register? > diff --git a/drivers/gpu/host1x/hw/syncpt_hw.c b/drivers/gpu/host1x/hw/syncpt_hw.c [...] > +/* > + * Updates the last value read from hardware. > + * (was host1x_syncpt_load_min) Can the comment in () not be dropped? Given that this is new code nobody would know about the old name. > + */ > +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. > + if (!host1x_syncpt_check_max(sp, live)) > + dev_err(&dev->dev->dev, > + "%s failed: id=%u, min=%d, max=%d\n", > + __func__, > + sp->id, > + host1x_syncpt_read_min(sp), > + host1x_syncpt_read_max(sp)); You could probably make this fit into less lines. > +/* > + * Write a cpu syncpoint increment to the hardware, without touching > + * the cache. Caller is responsible for host being powered. > + */ The last part of this comment applies to every host1x function, right? So maybe it should just be dropped. > +static void syncpt_debug(struct host1x_syncpt *sp) > +{ > + u32 i; > + for (i = 0; i < host1x_syncpt_nb_pts(sp->dev); i++) { > + u32 max = host1x_syncpt_read_max(sp); > + u32 min = host1x_syncpt_load_min(sp); > + if (!max && !min) > + continue; > + dev_info(&sp->dev->dev->dev, > + "id %d (%s) min %d max %d\n", > + i, sp->name, > + min, max); > + > + } There's a gratuitous blank line above. > + > + for (i = 0; i < host1x_syncpt_nb_bases(sp->dev); i++) { > + u32 base_val; > + host1x_syncpt_read_wait_base(sp); > + base_val = sp->base_val; > + if (base_val) > + dev_info(&sp->dev->dev->dev, > + "waitbase id %d val %d\n", > + i, base_val); > + > + } And another one. > diff --git a/drivers/gpu/host1x/syncpt.c b/drivers/gpu/host1x/syncpt.c [...] > +#include <linux/platform_device.h> > +#include <linux/slab.h> > +#include <linux/stat.h> I don't think this is needed. > +#include <linux/module.h> > +#include "syncpt.h" > +#include "dev.h" > +#include <trace/events/host1x.h> Again, some more spacing would be nice here. And the ordering is a bit weird. Maybe put the trace include above syncpt.h and dev.h? > +#define MAX_SYNCPT_LENGTH 5 This doesn't seem to be used anywhere. > +static struct host1x_syncpt *_host1x_syncpt_alloc(struct host1x *host, > + struct platform_device *pdev, > + int client_managed); Can't you move the actual implementation here? Also I'm not sure if passing the platform_device is the best choice here. struct device should work just as well. > +/* > + * 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. If you decide to change the name, make sure to change it in the syncpt ops as well. > +/* > + * Updates sw shadow state for client managed registers > + */ > +void host1x_syncpt_save(struct host1x *dev) > +{ > + struct host1x_syncpt *sp_base = dev->syncpt; > + u32 i; > + > + for (i = 0; i < host1x_syncpt_nb_pts(dev); i++) { > + if (host1x_syncpt_client_managed(sp_base + i)) > + dev->syncpt_op.load_min(sp_base + i); > + else > + WARN_ON(!host1x_syncpt_min_eq_max(sp_base + i)); > + } > + > + for (i = 0; i < host1x_syncpt_nb_bases(dev); i++) > + dev->syncpt_op.read_wait_base(sp_base + i); > +} A similar comment applies here. Though I'm not so sure about a better name. Perhaps host1x_syncpt_sync()? I know that this must sound all pretty straightforward to you, but for somebody who hasn't used these functions at all the names are quite confusing. So instead of people to go read the documentation I tend to think that making the names as descriptive as possible is essential here. > +/* > + * 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. > +int host1x_syncpt_init(struct host1x *host) > +{ > + struct host1x_syncpt *syncpt, *sp; > + int i; > + > + syncpt = sp = devm_kzalloc(&host->dev->dev, > + sizeof(struct host1x_syncpt) * host->info.nb_pts, You can make this a bit shorter by using sizeof(*sp) instead. > + for (i = 0; i < host->info.nb_pts; ++i, ++sp) { > + sp->id = i; > + sp->dev = host; Perhaps: syncpt[i].id = i; syncpt[i].dev = host; To avoid the need to explicitly keep track of sp? > +static struct host1x_syncpt *_host1x_syncpt_alloc(struct host1x *host, > + struct platform_device *pdev, > + int client_managed) > +{ > + int i; > + struct host1x_syncpt *sp = host->syncpt; > + char *name; > + > + for (i = 0; i < host->info.nb_pts && sp->name; i++, sp++) > + ; > + if (sp->pdev) > + return NULL; > + > + name = kasprintf(GFP_KERNEL, "%02d-%s", sp->id, > + pdev ? dev_name(&pdev->dev) : NULL); > + if (!name) > + return NULL; > + > + sp->pdev = pdev; > + sp->name = name; > + sp->client_managed = client_managed; > + > + return sp; > +} > + > +struct host1x_syncpt *host1x_syncpt_alloc(struct platform_device *pdev, > + int client_managed) > +{ > + struct host1x *host = host1x_get_host(pdev); > + return _host1x_syncpt_alloc(host, pdev, client_managed); > +} I think it's enough to keep track of the struct device here instead of the struct platform_device. 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 IRQ API and might be easier to work with. > +struct host1x_syncpt *host1x_syncpt_get(struct host1x *dev, u32 id) > +{ > + return dev->syncpt + id; > +} Should this perhaps do some error checking. What if the specified syncpt hasn't actually been requested before? > diff --git a/drivers/gpu/host1x/syncpt.h b/drivers/gpu/host1x/syncpt.h [...] > +struct host1x_syncpt { > + int id; > + atomic_t min_val; > + atomic_t max_val; > + u32 base_val; > + const char *name; > + int client_managed; Is this field actually ever used? Looking through the patches none of the clients actually set this. > +/* > + * Returns true if syncpoint min == max, which means that there are no > + * outstanding operations. > + */ > +static inline bool host1x_syncpt_min_eq_max(struct host1x_syncpt *sp) > +{ > + int min, max; > + smp_rmb(); > + min = atomic_read(&sp->min_val); > + max = atomic_read(&sp->max_val); > + return (min == max); > +} Maybe call this host1x_syncpt_idle() or something similar instead? > +{ > + return sp->id != NVSYNCPT_INVALID && > + sp->id < host1x_syncpt_nb_pts(sp->dev); > +} Is there really a need for NVSYNCPT_INVALID? Even if you want to keep the special case you can drop the explicit check because -1 will be larger than host1x_syncpt_nb_pts() anyway. Thierry
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 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. Ok. > >> +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. Sounds good. I guess there are other areas in need of a const, too. > >> + struct dentry *debugfs; > > This doesn't seem to be used anywhere. It's a failed split - it's used in the debug patch (4/8). > >> +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? Hmm, true, this should fit into smaller space. > >> 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 ordering here looks funny. I'll make it more alphabetic. > >> +#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. > >> diff --git a/drivers/gpu/host1x/hw/host1x01.h b/drivers/gpu/host1x/hw/host1x01.h > [...] >> +int host1x01_init(struct host1x *); > > For completeness you should probably name the parameter, even if this is > a prototype. Ok. > >> diff --git a/drivers/gpu/host1x/hw/host1x01_hardware.h b/drivers/gpu/host1x/hw/host1x01_hardware.h > [...] >> +#include <linux/types.h> >> +#include <linux/bitops.h> >> +#include "hw_host1x01_sync.h" > > Again, a blank line might help between the above two. I also assume that > this file will be filled with more content later on, so I guess it's not > worth the trouble to postpone it's creation until a later point. Yeah, most of the content gets added by the dreaded patch 3. > >> diff --git a/drivers/gpu/host1x/hw/hw_host1x01_sync.h b/drivers/gpu/host1x/hw/hw_host1x01_sync.h > [...] >> +static inline u32 host1x_sync_syncpt_0_r(void) >> +{ >> + return 0x400; >> +} >> +#define HOST1X_SYNC_SYNCPT_0 \ >> + host1x_sync_syncpt_0_r() >> +static inline u32 host1x_sync_syncpt_base_0_r(void) >> +{ >> + return 0x600; >> +} >> +#define HOST1X_SYNC_SYNCPT_BASE_0 \ >> + host1x_sync_syncpt_base_0_r() >> +static inline u32 host1x_sync_syncpt_cpu_incr_r(void) >> +{ >> + return 0x700; >> +} > > Perhaps it would be useful to modify these to take the syncpt ID as a > parameter? That way you don't have to remember to do the multiplication > everytime you access the register? Yeah, sounds good. > >> diff --git a/drivers/gpu/host1x/hw/syncpt_hw.c b/drivers/gpu/host1x/hw/syncpt_hw.c > [...] >> +/* >> + * Updates the last value read from hardware. >> + * (was host1x_syncpt_load_min) > > Can the comment in () not be dropped? Given that this is new code nobody > would know about the old name. Yes, it should be dropped. > >> + */ >> +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. > >> + if (!host1x_syncpt_check_max(sp, live)) >> + dev_err(&dev->dev->dev, >> + "%s failed: id=%u, min=%d, max=%d\n", >> + __func__, >> + sp->id, >> + host1x_syncpt_read_min(sp), >> + host1x_syncpt_read_max(sp)); > > You could probably make this fit into less lines. Yes, definitely. Will do. > >> +/* >> + * Write a cpu syncpoint increment to the hardware, without touching >> + * the cache. Caller is responsible for host being powered. >> + */ > > The last part of this comment applies to every host1x function, right? > So maybe it should just be dropped. Yeah, we don't really have runtime PM, so host1x is anyway turned on. In downstream, with dynamic power management, some functions require caller to ensure power is on, some functions turn on power themselves. I'll remove these comments, as they do not apply until we have runtime PM. > >> +static void syncpt_debug(struct host1x_syncpt *sp) >> +{ >> + u32 i; >> + for (i = 0; i < host1x_syncpt_nb_pts(sp->dev); i++) { >> + u32 max = host1x_syncpt_read_max(sp); >> + u32 min = host1x_syncpt_load_min(sp); >> + if (!max && !min) >> + continue; >> + dev_info(&sp->dev->dev->dev, >> + "id %d (%s) min %d max %d\n", >> + i, sp->name, >> + min, max); >> + >> + } > > There's a gratuitous blank line above. Will remove. > >> + >> + for (i = 0; i < host1x_syncpt_nb_bases(sp->dev); i++) { >> + u32 base_val; >> + host1x_syncpt_read_wait_base(sp); >> + base_val = sp->base_val; >> + if (base_val) >> + dev_info(&sp->dev->dev->dev, >> + "waitbase id %d val %d\n", >> + i, base_val); >> + >> + } > > And another one. Consider it gone. > >> diff --git a/drivers/gpu/host1x/syncpt.c b/drivers/gpu/host1x/syncpt.c > [...] >> +#include <linux/platform_device.h> >> +#include <linux/slab.h> >> +#include <linux/stat.h> > > I don't think this is needed. Yup, gone. > >> +#include <linux/module.h> >> +#include "syncpt.h" >> +#include "dev.h" >> +#include <trace/events/host1x.h> > > Again, some more spacing would be nice here. And the ordering is a bit > weird. Maybe put the trace include above syncpt.h and dev.h? Will do. > >> +#define MAX_SYNCPT_LENGTH 5 > > This doesn't seem to be used anywhere. Yeah, it was an old restriction for length of syncpt name, but as we moved to dynamic allocation, it doesn't apply. > >> +static struct host1x_syncpt *_host1x_syncpt_alloc(struct host1x *host, >> + struct platform_device *pdev, >> + int client_managed); > > Can't you move the actual implementation here? Also I'm not sure if > passing the platform_device is the best choice here. struct device > should work just as well. True, and sp->pdev needs to become struct device *, too. > >> +/* >> + * 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. > > If you decide to change the name, make sure to change it in the syncpt > ops as well. Sure. > >> +/* >> + * Updates sw shadow state for client managed registers >> + */ >> +void host1x_syncpt_save(struct host1x *dev) >> +{ >> + struct host1x_syncpt *sp_base = dev->syncpt; >> + u32 i; >> + >> + for (i = 0; i < host1x_syncpt_nb_pts(dev); i++) { >> + if (host1x_syncpt_client_managed(sp_base + i)) >> + dev->syncpt_op.load_min(sp_base + i); >> + else >> + WARN_ON(!host1x_syncpt_min_eq_max(sp_base + i)); >> + } >> + >> + for (i = 0; i < host1x_syncpt_nb_bases(dev); i++) >> + dev->syncpt_op.read_wait_base(sp_base + i); >> +} > > A similar comment applies here. Though I'm not so sure about a better > name. Perhaps host1x_syncpt_sync()? > > I know that this must sound all pretty straightforward to you, but for > somebody who hasn't used these functions at all the names are quite > confusing. So instead of people to go read the documentation I tend to > think that making the names as descriptive as possible is essential > here. I definitely agree that naming should be descriptive. This is used when saving host1x state before it loses power, so that's why it's called host1x_syncpt_save(). But I'm open to changing the naming, if something else would feel more descriptive. > >> +/* >> + * 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. > >> +int host1x_syncpt_init(struct host1x *host) >> +{ >> + struct host1x_syncpt *syncpt, *sp; >> + int i; >> + >> + syncpt = sp = devm_kzalloc(&host->dev->dev, >> + sizeof(struct host1x_syncpt) * host->info.nb_pts, > > You can make this a bit shorter by using sizeof(*sp) instead. Will do. > >> + for (i = 0; i < host->info.nb_pts; ++i, ++sp) { >> + sp->id = i; >> + sp->dev = host; > > Perhaps: > > syncpt[i].id = i; > syncpt[i].dev = host; > > To avoid the need to explicitly keep track of sp? Sounds good. I usually prefer indexing, so I'm happy with this. > >> +static struct host1x_syncpt *_host1x_syncpt_alloc(struct host1x *host, >> + struct platform_device *pdev, >> + int client_managed) >> +{ >> + int i; >> + struct host1x_syncpt *sp = host->syncpt; >> + char *name; >> + >> + for (i = 0; i < host->info.nb_pts && sp->name; i++, sp++) >> + ; >> + if (sp->pdev) >> + return NULL; >> + >> + name = kasprintf(GFP_KERNEL, "%02d-%s", sp->id, >> + pdev ? dev_name(&pdev->dev) : NULL); >> + if (!name) >> + return NULL; >> + >> + sp->pdev = pdev; >> + sp->name = name; >> + sp->client_managed = client_managed; >> + >> + return sp; >> +} >> + >> +struct host1x_syncpt *host1x_syncpt_alloc(struct platform_device *pdev, >> + int client_managed) >> +{ >> + struct host1x *host = host1x_get_host(pdev); >> + return _host1x_syncpt_alloc(host, pdev, client_managed); >> +} > > I think it's enough to keep track of the struct device here instead of > the struct platform_device. Yes, I actually managed to comment the same thing earlier. > > 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 IRQ API and might be easier to work > with. I'm not entirely sure about the difference, but isn't the number to be allocated usually passed to a function ending in _request? Allocate would just allocate the next available - as host1x_syncpt_allocate does. > >> +struct host1x_syncpt *host1x_syncpt_get(struct host1x *dev, u32 id) >> +{ >> + return dev->syncpt + id; >> +} > > Should this perhaps do some error checking. What if the specified syncpt > hasn't actually been requested before? I'll need to check the use of host1x_syncpt_get(). It might be called for un-allocated (or requested, if we choose that) syncpoints. An error check would make sense at least to check that id is smaller than nb_pts. > >> diff --git a/drivers/gpu/host1x/syncpt.h b/drivers/gpu/host1x/syncpt.h > [...] >> +struct host1x_syncpt { >> + int id; >> + atomic_t min_val; >> + atomic_t max_val; >> + u32 base_val; >> + const char *name; >> + int client_managed; > > Is this field actually ever used? Looking through the patches none of > the clients actually set this. VBLANK should be set client_managed, so a follow-up patch would add a call from dc.c to here, with client_managed=false. > >> +/* >> + * Returns true if syncpoint min == max, which means that there are no >> + * outstanding operations. >> + */ >> +static inline bool host1x_syncpt_min_eq_max(struct host1x_syncpt *sp) >> +{ >> + int min, max; >> + smp_rmb(); >> + min = atomic_read(&sp->min_val); >> + max = atomic_read(&sp->max_val); >> + return (min == max); >> +} > > Maybe call this host1x_syncpt_idle() or something similar instead? Sounds fine - although the syncpt itself isn't idle, but the corresponding client. > >> +{ >> + return sp->id != NVSYNCPT_INVALID && >> + sp->id < host1x_syncpt_nb_pts(sp->dev); >> +} > > Is there really a need for NVSYNCPT_INVALID? Even if you want to keep > the special case you can drop the explicit check because -1 will be > larger than host1x_syncpt_nb_pts() anyway. No, it's not really needed, so I'll remove it. Terje
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 IRQ API and might be easier to work > > with. > > I'm not entirely sure about the difference, but isn't the number to be > allocated usually passed to a function ending in _request? Allocate > would just allocate the next available - as host1x_syncpt_allocate does. 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(). Thierry
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
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 0000000..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 0000000..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 0000000..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_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 = &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); + if (!regs) { + dev_err(&dev->dev, "missing regs\n"); + return -ENXIO; + } + + syncpt_irq = platform_get_irq(dev, 0); + if (IS_ERR_VALUE(syncpt_irq)) { + dev_err(&dev->dev, "missing irq\n"); + return -ENXIO; + } + + host = devm_kzalloc(&dev->dev, sizeof(*host), GFP_KERNEL); + if (!host) { + dev_err(&dev->dev, "failed to alloc host1x\n"); + return -ENOMEM; + } + + host->dev = dev; + memcpy(&host->info, devid->data, sizeof(struct host1x_device_info)); + + /* 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; + } + + if (host->info.init) { + err = host->info.init(host); + if (err) + return err; + } + + err = host1x_syncpt_init(host); + if (err) + return err; + + host->clk = devm_clk_get(&dev->dev, NULL); + if (IS_ERR(host->clk)) { + dev_err(&dev->dev, "failed to get clock\n"); + err = PTR_ERR(host->clk); + goto fail_deinit_syncpt; + } + + err = clk_prepare_enable(host->clk); + if (err < 0) { + dev_err(&dev->dev, "failed to enable clock\n"); + goto fail_deinit_syncpt; + } + + host1x_syncpt_reset(host); + + dev_info(&dev->dev, "initialized\n"); + + return 0; + +fail_deinit_syncpt: + host1x_syncpt_deinit(host); + return err; +} + +static int __exit host1x_remove(struct platform_device *dev) +{ + struct host1x *host = platform_get_drvdata(dev); + host1x_syncpt_deinit(host); + clk_disable_unprepare(host->clk); + return 0; +} + +static struct platform_driver platform_driver = { + .probe = host1x_probe, + .remove = __exit_p(host1x_remove), + .driver = { + .owner = THIS_MODULE, + .name = DRIVER_NAME, + .of_match_table = host1x_match, + }, +}; + +module_platform_driver(platform_driver); + +MODULE_AUTHOR("Terje Bergstrom <tbergstrom@nvidia.com>"); +MODULE_DESCRIPTION("Host1x driver for Tegra products"); +MODULE_LICENSE("GPL"); diff --git a/drivers/gpu/host1x/dev.h b/drivers/gpu/host1x/dev.h new file mode 100644 index 0000000..d8f5979 --- /dev/null +++ b/drivers/gpu/host1x/dev.h @@ -0,0 +1,73 @@ +/* + * Copyright (c) 2012-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/>. + */ + +#ifndef HOST1X_DEV_H +#define HOST1X_DEV_H + +#include "syncpt.h" + +struct host1x; +struct host1x_syncpt; +struct platform_device; + +struct host1x_syncpt_ops { + void (*reset)(struct host1x_syncpt *); + void (*reset_wait_base)(struct host1x_syncpt *); + void (*read_wait_base)(struct host1x_syncpt *); + u32 (*load_min)(struct host1x_syncpt *); + void (*cpu_incr)(struct host1x_syncpt *); + int (*patch_wait)(struct host1x_syncpt *, void *patch_addr); + void (*debug)(struct host1x_syncpt *); + const char * (*name)(struct host1x_syncpt *); +}; + +struct host1x_device_info { + 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; +}; + +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; + + struct dentry *debugfs; +}; + +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); +} + +void host1x_sync_writel(struct host1x *host1x, u32 r, u32 v); +u32 host1x_sync_readl(struct host1x *host1x, u32 r); + +#endif diff --git a/drivers/gpu/host1x/hw/Makefile b/drivers/gpu/host1x/hw/Makefile new file mode 100644 index 0000000..9b50863 --- /dev/null +++ b/drivers/gpu/host1x/hw/Makefile @@ -0,0 +1,6 @@ +ccflags-y = -Idrivers/gpu/host1x + +host1x-hw-objs = \ + host1x01.o + +obj-$(CONFIG_TEGRA_HOST1X) += host1x-hw.o diff --git a/drivers/gpu/host1x/hw/host1x01.c b/drivers/gpu/host1x/hw/host1x01.c new file mode 100644 index 0000000..ea6e604 --- /dev/null +++ b/drivers/gpu/host1x/hw/host1x01.c @@ -0,0 +1,35 @@ +/* + * Host1x init for T20 and T30 Architecture Chips + * + * Copyright (c) 2011-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/init.h> +#include <linux/clk.h> +#include <linux/of.h> +#include <linux/of_platform.h> + +#include "hw/host1x01.h" +#include "dev.h" +#include "hw/host1x01_hardware.h" + +#include "hw/syncpt_hw.c" + +int host1x01_init(struct host1x *host) +{ + host->syncpt_op = host1x_syncpt_ops; + + return 0; +} diff --git a/drivers/gpu/host1x/hw/host1x01.h b/drivers/gpu/host1x/hw/host1x01.h new file mode 100644 index 0000000..6ec30051 --- /dev/null +++ b/drivers/gpu/host1x/hw/host1x01.h @@ -0,0 +1,25 @@ +/* + * Host1x init for T20 and T30 Architecture Chips + * + * Copyright (c) 2011-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/>. + */ +#ifndef HOST1X_HOST1X01_H +#define HOST1X_HOST1X01_H + +struct host1x; + +int host1x01_init(struct host1x *); + +#endif /* HOST1X_HOST1X01_H_ */ diff --git a/drivers/gpu/host1x/hw/host1x01_hardware.h b/drivers/gpu/host1x/hw/host1x01_hardware.h new file mode 100644 index 0000000..c1d5324 --- /dev/null +++ b/drivers/gpu/host1x/hw/host1x01_hardware.h @@ -0,0 +1,26 @@ +/* + * Tegra host1x Register Offsets for Tegra20 and Tegra30 + * + * 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/>. + */ + +#ifndef __HOST1X_HOST1X01_HARDWARE_H +#define __HOST1X_HOST1X01_HARDWARE_H + +#include <linux/types.h> +#include <linux/bitops.h> +#include "hw_host1x01_sync.h" + +#endif diff --git a/drivers/gpu/host1x/hw/hw_host1x01_sync.h b/drivers/gpu/host1x/hw/hw_host1x01_sync.h new file mode 100644 index 0000000..b12c1a4 --- /dev/null +++ b/drivers/gpu/host1x/hw/hw_host1x01_sync.h @@ -0,0 +1,72 @@ +/* + * Copyright (c) 2012-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/>. + * + */ + + /* + * Function naming determines intended use: + * + * <x>_r(void) : Returns the offset for register <x>. + * + * <x>_w(void) : Returns the word offset for word (4 byte) element <x>. + * + * <x>_<y>_s(void) : Returns size of field <y> of register <x> in bits. + * + * <x>_<y>_f(u32 v) : Returns a value based on 'v' which has been shifted + * and masked to place it at field <y> of register <x>. This value + * can be |'d with others to produce a full register value for + * register <x>. + * + * <x>_<y>_m(void) : Returns a mask for field <y> of register <x>. This + * value can be ~'d and then &'d to clear the value of field <y> for + * register <x>. + * + * <x>_<y>_<z>_f(void) : Returns the constant value <z> after being shifted + * to place it at field <y> of register <x>. This value can be |'d + * with others to produce a full register value for <x>. + * + * <x>_<y>_v(u32 r) : Returns the value of field <y> from a full register + * <x> value 'r' after being shifted to place its LSB at bit 0. + * This value is suitable for direct comparison with other unshifted + * values appropriate for use in field <y> of register <x>. + * + * <x>_<y>_<z>_v(void) : Returns the constant value for <z> defined for + * field <y> of register <x>. This value is suitable for direct + * comparison with unshifted values appropriate for use in field <y> + * of register <x>. + */ + +#ifndef __hw_host1x01_sync_h__ +#define __hw_host1x01_sync_h__ + +static inline u32 host1x_sync_syncpt_0_r(void) +{ + return 0x400; +} +#define HOST1X_SYNC_SYNCPT_0 \ + host1x_sync_syncpt_0_r() +static inline u32 host1x_sync_syncpt_base_0_r(void) +{ + return 0x600; +} +#define HOST1X_SYNC_SYNCPT_BASE_0 \ + host1x_sync_syncpt_base_0_r() +static inline u32 host1x_sync_syncpt_cpu_incr_r(void) +{ + return 0x700; +} +#define HOST1X_SYNC_SYNCPT_CPU_INCR \ + host1x_sync_syncpt_cpu_incr_r() +#endif /* __hw_host1x01_sync_h__ */ diff --git a/drivers/gpu/host1x/hw/syncpt_hw.c b/drivers/gpu/host1x/hw/syncpt_hw.c new file mode 100644 index 0000000..16e3ada --- /dev/null +++ b/drivers/gpu/host1x/hw/syncpt_hw.c @@ -0,0 +1,146 @@ +/* + * Tegra host1x Syncpoints + * + * 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/io.h> +#include "syncpt.h" +#include "dev.h" + +/* + * Write the current syncpoint value back to hw. + */ +static void syncpt_reset(struct host1x_syncpt *sp) +{ + struct host1x *dev = sp->dev; + int min = host1x_syncpt_read_min(sp); + host1x_sync_writel(dev, min, HOST1X_SYNC_SYNCPT_0 + sp->id * 4); +} + +/* + * Write the current waitbase value back to hw. + */ +static void syncpt_reset_wait_base(struct host1x_syncpt *sp) +{ + struct host1x *dev = sp->dev; + host1x_sync_writel(dev, sp->base_val, + HOST1X_SYNC_SYNCPT_BASE_0 + sp->id * 4); +} + +/* + * Read waitbase value from hw. + */ +static void syncpt_read_wait_base(struct host1x_syncpt *sp) +{ + struct host1x *dev = sp->dev; + sp->base_val = host1x_sync_readl(dev, + HOST1X_SYNC_SYNCPT_BASE_0 + sp->id * 4); +} + +/* + * Updates the last value read from hardware. + * (was host1x_syncpt_load_min) + */ +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); + + if (!host1x_syncpt_check_max(sp, live)) + dev_err(&dev->dev->dev, + "%s failed: id=%u, min=%d, max=%d\n", + __func__, + sp->id, + host1x_syncpt_read_min(sp), + host1x_syncpt_read_max(sp)); + + return live; +} + +/* + * Write a cpu syncpoint increment to the hardware, without touching + * the cache. Caller is responsible for host being powered. + */ +static void syncpt_cpu_incr(struct host1x_syncpt *sp) +{ + struct host1x *dev = sp->dev; + u32 reg_offset = sp->id / 32; + + if (!host1x_syncpt_client_managed(sp) + && host1x_syncpt_min_eq_max(sp)) { + dev_err(&dev->dev->dev, + "Trying to increment syncpoint id %d beyond max\n", + sp->id); + return; + } + host1x_sync_writel(dev, BIT_MASK(sp->id), + HOST1X_SYNC_SYNCPT_CPU_INCR + reg_offset * 4); + wmb(); +} + +static const char *syncpt_name(struct host1x_syncpt *sp) +{ + struct host1x_device_info *info = &sp->dev->info; + const char *name = NULL; + + if (sp->id < info->nb_pts) + name = sp->name; + + return name ? name : ""; +} + +static void syncpt_debug(struct host1x_syncpt *sp) +{ + u32 i; + for (i = 0; i < host1x_syncpt_nb_pts(sp->dev); i++) { + u32 max = host1x_syncpt_read_max(sp); + u32 min = host1x_syncpt_load_min(sp); + if (!max && !min) + continue; + dev_info(&sp->dev->dev->dev, + "id %d (%s) min %d max %d\n", + i, sp->name, + min, max); + + } + + for (i = 0; i < host1x_syncpt_nb_bases(sp->dev); i++) { + u32 base_val; + host1x_syncpt_read_wait_base(sp); + base_val = sp->base_val; + if (base_val) + dev_info(&sp->dev->dev->dev, + "waitbase id %d val %d\n", + i, base_val); + + } +} + +static const struct host1x_syncpt_ops host1x_syncpt_ops = { + .reset = syncpt_reset, + .reset_wait_base = syncpt_reset_wait_base, + .read_wait_base = syncpt_read_wait_base, + .load_min = syncpt_load_min, + .cpu_incr = syncpt_cpu_incr, + .debug = syncpt_debug, + .name = syncpt_name, +}; diff --git a/drivers/gpu/host1x/syncpt.c b/drivers/gpu/host1x/syncpt.c new file mode 100644 index 0000000..b45651f --- /dev/null +++ b/drivers/gpu/host1x/syncpt.c @@ -0,0 +1,217 @@ +/* + * Tegra host1x Syncpoints + * + * 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/platform_device.h> +#include <linux/slab.h> +#include <linux/stat.h> +#include <linux/module.h> +#include "syncpt.h" +#include "dev.h" +#include <trace/events/host1x.h> + +#define MAX_SYNCPT_LENGTH 5 + +static struct host1x_syncpt *_host1x_syncpt_alloc(struct host1x *host, + struct platform_device *pdev, + int client_managed); + +u32 host1x_syncpt_id(struct host1x_syncpt *sp) +{ + return sp->id; +} + +/* + * Updates the value sent to hardware. + */ +u32 host1x_syncpt_incr_max(struct host1x_syncpt *sp, u32 incrs) +{ + return (u32)atomic_add_return(incrs, &sp->max_val); +} + +/* + * Resets syncpoint and waitbase values to sw shadows + */ +void host1x_syncpt_reset(struct host1x *dev) +{ + struct host1x_syncpt *sp_base = dev->syncpt; + u32 i; + + for (i = 0; i < host1x_syncpt_nb_pts(dev); i++) + dev->syncpt_op.reset(sp_base + i); + for (i = 0; i < host1x_syncpt_nb_bases(dev); i++) + dev->syncpt_op.reset_wait_base(sp_base + i); + wmb(); +} + +/* + * Updates sw shadow state for client managed registers + */ +void host1x_syncpt_save(struct host1x *dev) +{ + struct host1x_syncpt *sp_base = dev->syncpt; + u32 i; + + for (i = 0; i < host1x_syncpt_nb_pts(dev); i++) { + if (host1x_syncpt_client_managed(sp_base + i)) + dev->syncpt_op.load_min(sp_base + i); + else + WARN_ON(!host1x_syncpt_min_eq_max(sp_base + i)); + } + + for (i = 0; i < host1x_syncpt_nb_bases(dev); i++) + dev->syncpt_op.read_wait_base(sp_base + i); +} + +/* + * 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; +} + +/* + * Get the current syncpoint base + */ +u32 host1x_syncpt_read_wait_base(struct host1x_syncpt *sp) +{ + u32 val; + sp->dev->syncpt_op.read_wait_base(sp); + val = sp->base_val; + return val; +} + +/* + * Write a cpu syncpoint increment to the hardware, without touching + * the cache. Caller is responsible for host being powered. + */ +void host1x_syncpt_cpu_incr(struct host1x_syncpt *sp) +{ + sp->dev->syncpt_op.cpu_incr(sp); +} + +/* + * Increment syncpoint value from cpu, updating cache + */ +void host1x_syncpt_incr(struct host1x_syncpt *sp) +{ + if (host1x_syncpt_client_managed(sp)) + host1x_syncpt_incr_max(sp, 1); + host1x_syncpt_cpu_incr(sp); +} + +void host1x_syncpt_debug(struct host1x_syncpt *sp) +{ + sp->dev->syncpt_op.debug(sp); +} + +int host1x_syncpt_init(struct host1x *host) +{ + struct host1x_syncpt *syncpt, *sp; + int i; + + syncpt = sp = devm_kzalloc(&host->dev->dev, + sizeof(struct host1x_syncpt) * host->info.nb_pts, + GFP_KERNEL); + if (!syncpt) + return -ENOMEM; + + for (i = 0; i < host->info.nb_pts; ++i, ++sp) { + sp->id = i; + sp->dev = host; + } + + host->syncpt = syncpt; + + return 0; +} + +static struct host1x_syncpt *_host1x_syncpt_alloc(struct host1x *host, + struct platform_device *pdev, + int client_managed) +{ + int i; + struct host1x_syncpt *sp = host->syncpt; + char *name; + + for (i = 0; i < host->info.nb_pts && sp->name; i++, sp++) + ; + if (sp->pdev) + return NULL; + + name = kasprintf(GFP_KERNEL, "%02d-%s", sp->id, + pdev ? dev_name(&pdev->dev) : NULL); + if (!name) + return NULL; + + sp->pdev = pdev; + sp->name = name; + sp->client_managed = client_managed; + + return sp; +} + +struct host1x_syncpt *host1x_syncpt_alloc(struct platform_device *pdev, + int client_managed) +{ + struct host1x *host = host1x_get_host(pdev); + return _host1x_syncpt_alloc(host, pdev, client_managed); +} + +void host1x_syncpt_free(struct host1x_syncpt *sp) +{ + if (!sp) + return; + + kfree(sp->name); + sp->pdev = NULL; + sp->name = NULL; + sp->client_managed = 0; +} + +void host1x_syncpt_deinit(struct host1x *host) +{ + int i; + struct host1x_syncpt *sp = host->syncpt; + for (i = 0; i < host->info.nb_pts; i++, sp++) + kfree(sp->name); +} + +int host1x_syncpt_nb_pts(struct host1x *dev) +{ + return dev->info.nb_pts; +} + +int host1x_syncpt_nb_bases(struct host1x *dev) +{ + return dev->info.nb_bases; +} + +int host1x_syncpt_nb_mlocks(struct host1x *dev) +{ + return dev->info.nb_mlocks; +} + +struct host1x_syncpt *host1x_syncpt_get(struct host1x *dev, u32 id) +{ + return dev->syncpt + id; +} diff --git a/drivers/gpu/host1x/syncpt.h b/drivers/gpu/host1x/syncpt.h new file mode 100644 index 0000000..d9b9b0a --- /dev/null +++ b/drivers/gpu/host1x/syncpt.h @@ -0,0 +1,153 @@ +/* + * Tegra host1x Syncpoints + * + * 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/>. + */ + +#ifndef __HOST1X_SYNCPT_H +#define __HOST1X_SYNCPT_H + +#include <linux/kernel.h> +#include <linux/sched.h> +#include <linux/atomic.h> + +struct host1x; + +#define NVSYNCPT_INVALID (-1) + +struct host1x_syncpt { + int id; + atomic_t min_val; + atomic_t max_val; + u32 base_val; + const char *name; + int client_managed; + struct host1x *dev; + struct platform_device *pdev; +}; + +/* Initialize sync point array */ +int host1x_syncpt_init(struct host1x *); + +/* Free sync point array */ +void host1x_syncpt_deinit(struct host1x *); + +/* + * Read max. It indicates how many operations there are in queue, either in + * channel or in a software thread. + * */ +static inline u32 host1x_syncpt_read_max(struct host1x_syncpt *sp) +{ + smp_rmb(); + return (u32)atomic_read(&sp->max_val); +} + +/* + * Read min, which is a shadow of the current sync point value in hardware. + */ +static inline u32 host1x_syncpt_read_min(struct host1x_syncpt *sp) +{ + smp_rmb(); + return (u32)atomic_read(&sp->min_val); +} + +/* Return number of sync point supported. */ +int host1x_syncpt_nb_pts(struct host1x *dev); + +/* Return number of wait bases supported. */ +int host1x_syncpt_nb_bases(struct host1x *dev); + +/* Return number of mlocks supported. */ +int host1x_syncpt_nb_mlocks(struct host1x *dev); + +/* + * Check sync point sanity. If max is larger than min, there have too many + * sync point increments. + * + * Client managed sync point are not tracked. + * */ +static inline bool host1x_syncpt_check_max(struct host1x_syncpt *sp, u32 real) +{ + u32 max; + if (sp->client_managed) + return true; + max = host1x_syncpt_read_max(sp); + return (s32)(max - real) >= 0; +} + +/* Return true if sync point is client managed. */ +static inline int host1x_syncpt_client_managed(struct host1x_syncpt *sp) +{ + return sp->client_managed; +} + +/* + * Returns true if syncpoint min == max, which means that there are no + * outstanding operations. + */ +static inline bool host1x_syncpt_min_eq_max(struct host1x_syncpt *sp) +{ + int min, max; + smp_rmb(); + min = atomic_read(&sp->min_val); + max = atomic_read(&sp->max_val); + return (min == max); +} + +/* Return pointer to struct denoting sync point id. */ +struct host1x_syncpt *host1x_syncpt_get(struct host1x *dev, u32 id); + +/* Request incrementing a sync point. */ +void host1x_syncpt_cpu_incr(struct host1x_syncpt *sp); + +/* Load current value from hardware to the shadow register. */ +u32 host1x_syncpt_load_min(struct host1x_syncpt *sp); + +/* Save host1x sync point state into shadow registers. */ +void host1x_syncpt_save(struct host1x *dev); + +/* Reset host1x sync point state from shadow registers. */ +void host1x_syncpt_reset(struct host1x *dev); + +/* Read current wait base value into shadow register and return it. */ +u32 host1x_syncpt_read_wait_base(struct host1x_syncpt *sp); + +/* Increment sync point and its max. */ +void host1x_syncpt_incr(struct host1x_syncpt *sp); + +/* Indicate future operations by incrementing the sync point max. */ +u32 host1x_syncpt_incr_max(struct host1x_syncpt *sp, u32 incrs); + +/* Do a debug dump of sync point values. */ +void host1x_syncpt_debug(struct host1x_syncpt *sp); + +/* Check if sync point id is valid. */ +static inline int host1x_syncpt_is_valid(struct host1x_syncpt *sp) +{ + return sp->id != NVSYNCPT_INVALID && + sp->id < host1x_syncpt_nb_pts(sp->dev); +} + +/* Return id of the sync point */ +u32 host1x_syncpt_id(struct host1x_syncpt *sp); + +/* Allocate a sync point for a device. */ +struct host1x_syncpt *host1x_syncpt_alloc(struct platform_device *pdev, + int client_managed); + +/* Free a sync point. */ +void host1x_syncpt_free(struct host1x_syncpt *sp); + +#endif diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig index e7068c5..776ddba 100644 --- a/drivers/video/Kconfig +++ b/drivers/video/Kconfig @@ -21,6 +21,8 @@ source "drivers/gpu/vga/Kconfig" source "drivers/gpu/drm/Kconfig" +source "drivers/gpu/host1x/Kconfig" + source "drivers/gpu/stub/Kconfig" config VGASTATE diff --git a/include/trace/events/host1x.h b/include/trace/events/host1x.h new file mode 100644 index 0000000..3c14cac --- /dev/null +++ b/include/trace/events/host1x.h @@ -0,0 +1,61 @@ +/* + * include/trace/events/host1x.h + * + * Nvhost event logging to ftrace. + * + * Copyright (c) 2010-2013, NVIDIA Corporation. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that 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, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ + +#undef TRACE_SYSTEM +#define TRACE_SYSTEM host1x + +#if !defined(_TRACE_HOST1X_H) || defined(TRACE_HEADER_MULTI_READ) +#define _TRACE_HOST1X_H + +#include <linux/ktime.h> +#include <linux/tracepoint.h> + +DECLARE_EVENT_CLASS(host1x, + TP_PROTO(const char *name), + TP_ARGS(name), + TP_STRUCT__entry(__field(const char *, name)), + TP_fast_assign(__entry->name = name;), + TP_printk("name=%s", __entry->name) +); + +TRACE_EVENT(host1x_syncpt_load_min, + TP_PROTO(u32 id, u32 val), + + TP_ARGS(id, val), + + TP_STRUCT__entry( + __field(u32, id) + __field(u32, val) + ), + + TP_fast_assign( + __entry->id = id; + __entry->val = val; + ), + + TP_printk("id=%d, val=%d", __entry->id, __entry->val) +); + +#endif /* _TRACE_HOST1X_H */ + +/* This part must be outside protection */ +#include <trace/define_trace.h>
Add host1x, the driver for host1x and its client unit 2D. Signed-off-by: Terje Bergstrom <tbergstrom@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