[RFC,v2,1/8] video: tegra: Add nvhost driver
On 29.11.2012 11:10, Mark Zhang wrote:
>> +
>> +/**
>> + * Write a cpu syncpoint increment to the hardware, without touching
>> + * the cache. Caller is responsible for host being powered.
>> + */
>> +static void host1x_syncpt_cpu_incr(struct nvhost_syncpt *sp, u32 id)
>> +{
>> + struct nvhost_master *dev = syncpt_to_dev(sp);
>> + u32 reg_offset = id / 32;
>> +
>> + if (!nvhost_module_powered(dev->dev)) {
>> + dev_err(&syncpt_to_dev(sp)->dev->dev,
>> + "Trying to access host1x when it's off");
>> + return;
>> + }
>> +
>> + if (!nvhost_syncpt_client_managed(sp, id)
>> + && nvhost_syncpt_min_eq_max(sp, id)) {
>> + dev_err(&syncpt_to_dev(sp)->dev->dev,
>> + "Trying to increment syncpoint id %d beyond max\n",
>> + id);
>> + return;
>> + }
>> + writel(BIT_MASK(id), dev->sync_aperture +
>> + host1x_sync_syncpt_cpu_incr_r() + reg_offset * 4);
>
> I have a stupid question: According to the name and the context of this
> function, seems it increases the syncpt value which specified by param
> "id". So how does this "writel" increase the value? I don't know much
> about host1x/syncpt reg operations, so could you explain a little bit or
> I just completely have a wrong understanding?
I believe I've implemented most of the requests in this mail, but I seem
to have missed answering this question.
writel() to that register invokes a method in host1x, which increments
the sync point indicated by the value of the register by one.
Best regards,
Terje
Re: [RFC,v2,1/8] video: tegra: Add nvhost driver
On 29.11.2012 11:10, Mark Zhang wrote:
>> +
>> +/**
>> + * Write a cpu syncpoint increment to the hardware, without touching
>> + * the cache. Caller is responsible for host being powered.
>> + */
>> +static void host1x_syncpt_cpu_incr(struct nvhost_syncpt *sp, u32 id)
>> +{
>> + struct nvhost_master *dev = syncpt_to_dev(sp);
>> + u32 reg_offset = id / 32;
>> +
>> + if (!nvhost_module_powered(dev->dev)) {
>> + dev_err(&syncpt_to_dev(sp)->dev->dev,
>> + "Trying to access host1x when it's off");
>> + return;
>> + }
>> +
>> + if (!nvhost_syncpt_client_managed(sp, id)
>> + && nvhost_syncpt_min_eq_max(sp, id)) {
>> + dev_err(&syncpt_to_dev(sp)->dev->dev,
>> + "Trying to increment syncpoint id %d beyond max\n",
>> + id);
>> + return;
>> + }
>> + writel(BIT_MASK(id), dev->sync_aperture +
>> + host1x_sync_syncpt_cpu_incr_r() + reg_offset * 4);
>
> I have a stupid question: According to the name and the context of this
> function, seems it increases the syncpt value which specified by param
> "id". So how does this "writel" increase the value? I don't know much
> about host1x/syncpt reg operations, so could you explain a little bit or
> I just completely have a wrong understanding?
I believe I've implemented most of the requests in this mail, but I seem
to have missed answering this question.
writel() to that register invokes a method in host1x, which increments
the sync point indicated by the value of the register by one.
Best regards,
Terje
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC,v2,1/8] video: tegra: Add nvhost driver
On 11/26/2012 09:19 PM, Terje Bergstr?m wrote:
> Add nvhost, the driver for host1x. This patch adds support for reading and
> incrementing sync points and dynamic power management.
>
> Signed-off-by: Terje Bergstrom
>
> ---
> drivers/video/Kconfig |2 +
> drivers/video/Makefile |2 +
> drivers/video/tegra/host/Kconfig |5 +
> drivers/video/tegra/host/Makefile | 10 +
> drivers/video/tegra/host/chip_support.c| 48 ++
> drivers/video/tegra/host/chip_support.h| 52 +++
> drivers/video/tegra/host/dev.c | 96
> drivers/video/tegra/host/host1x/Makefile |7 +
> drivers/video/tegra/host/host1x/host1x.c | 204 +
> drivers/video/tegra/host/host1x/host1x.h | 78
> drivers/video/tegra/host/host1x/host1x01.c | 37 ++
> drivers/video/tegra/host/host1x/host1x01.h | 29 ++
> .../video/tegra/host/host1x/host1x01_hardware.h| 36 ++
> drivers/video/tegra/host/host1x/host1x_syncpt.c| 156 +++
> drivers/video/tegra/host/host1x/hw_host1x01_sync.h | 398
> drivers/video/tegra/host/nvhost_acm.c | 481
>
> drivers/video/tegra/host/nvhost_acm.h | 45 ++
> drivers/video/tegra/host/nvhost_syncpt.c | 333 ++
> drivers/video/tegra/host/nvhost_syncpt.h | 136 ++
> include/linux/nvhost.h | 143 ++
> 20 files changed, 2298 insertions(+)
[...]
> diff --git a/drivers/video/tegra/host/chip_support.c
> b/drivers/video/tegra/host/chip_support.c
> +#include "chip_support.h"
> +#include "host1x/host1x01.h"
> +
> +struct nvhost_chip_support *nvhost_chip_ops;
> +
> +struct nvhost_chip_support *nvhost_get_chip_ops(void)
> +{
> + return nvhost_chip_ops;
> +}
If you wanna hide "nvhost_chip_ops" from other source files, declare it
as "static". So this is not a static member which means other files is
able to touch it by "extern" but we also define a function to get it,
and this looks redundant.
[...]
> diff --git a/drivers/video/tegra/host/host1x/Makefile
> b/drivers/video/tegra/host/host1x/Makefile
> new file mode 100644
> index 000..330d507
> --- /dev/null
> +++ b/drivers/video/tegra/host/host1x/Makefile
> @@ -0,0 +1,7 @@
> +ccflags-y = -Idrivers/video/tegra/host
> +
> +nvhost-host1x-objs = \
> + host1x.o \
> + host1x01.o
Can we rename this "host1x01.c"? I just really don't like this kind of
variables/files, I mean, I can't imagine the purpose of the file
according to it's name...
[...]
> +
> +static int __devinit nvhost_alloc_resources(struct nvhost_master *host)
> +{
> + int err;
> +
> + err = nvhost_init_chip_support(host);
> + if (err)
> + return err;
> +
> + return 0;
> +}
Just "return nvhost_init_chip_support(host)" is enough. If so, do we
still need this function?
[...]
> +
> +static int __devinit nvhost_probe(struct platform_device *dev)
> +
[...]
> + dev_info(&dev->dev, "initialized\n");
> +
> + return 0;
> +
> +fail:
Add more "free" codes here. Actually, "nvhost_free_resources" frees the
host->intr.syncpt which is not needed to free manually.
Seems at least we need to add "nvhost_syncpt_deinit" here.
[...]
> +
> +static struct of_device_id host1x_match[] __devinitdata = {
> + { .compatible = "nvidia,tegra20-host1x", },
> + { .compatible = "nvidia,tegra30-host1x", },
Again, place tegra30-host1x before tegra20-host1x.
[...]
> +
> +/**
> + * Write a cpu syncpoint increment to the hardware, without touching
> + * the cache. Caller is responsible for host being powered.
> + */
> +static void host1x_syncpt_cpu_incr(struct nvhost_syncpt *sp, u32 id)
> +{
> + struct nvhost_master *dev = syncpt_to_dev(sp);
> + u32 reg_offset = id / 32;
> +
> + if (!nvhost_module_powered(dev->dev)) {
> + dev_err(&syncpt_to_dev(sp)->dev->dev,
> + "Trying to access host1x when it's off");
> + return;
> + }
> +
> + if (!nvhost_syncpt_client_managed(sp, id)
> + && nvhost_syncpt_min_eq_max(sp, id)) {
> + dev_err(&syncpt_to_dev(sp)->dev->dev,
> + "Trying to increment syncpoint id %d beyond max\n",
> + id);
> + return;
> + }
> + writel(BIT_MASK(id), dev->sync_aperture +
> + host1x_sync_syncpt_cpu_incr_r() + reg_offset * 4);
I have a stupid question: According to the name and the context of this
function, seems it increases the syncpt value which specified by param
"id". So how does this "writel" increase the value? I don't know much
about host1x/syncpt reg operations, so could you explain a little bit or
I just completely have a wrong understanding?
[...]
> +
> +static ssize_t powergate_delay_store(struct kobject *kobj,
> + struct kobj_attr
Re: [RFC,v2,1/8] video: tegra: Add nvhost driver
On 11/26/2012 09:19 PM, Terje Bergström wrote:
> Add nvhost, the driver for host1x. This patch adds support for reading and
> incrementing sync points and dynamic power management.
>
> Signed-off-by: Terje Bergstrom
>
> ---
> drivers/video/Kconfig |2 +
> drivers/video/Makefile |2 +
> drivers/video/tegra/host/Kconfig |5 +
> drivers/video/tegra/host/Makefile | 10 +
> drivers/video/tegra/host/chip_support.c| 48 ++
> drivers/video/tegra/host/chip_support.h| 52 +++
> drivers/video/tegra/host/dev.c | 96
> drivers/video/tegra/host/host1x/Makefile |7 +
> drivers/video/tegra/host/host1x/host1x.c | 204 +
> drivers/video/tegra/host/host1x/host1x.h | 78
> drivers/video/tegra/host/host1x/host1x01.c | 37 ++
> drivers/video/tegra/host/host1x/host1x01.h | 29 ++
> .../video/tegra/host/host1x/host1x01_hardware.h| 36 ++
> drivers/video/tegra/host/host1x/host1x_syncpt.c| 156 +++
> drivers/video/tegra/host/host1x/hw_host1x01_sync.h | 398
> drivers/video/tegra/host/nvhost_acm.c | 481
>
> drivers/video/tegra/host/nvhost_acm.h | 45 ++
> drivers/video/tegra/host/nvhost_syncpt.c | 333 ++
> drivers/video/tegra/host/nvhost_syncpt.h | 136 ++
> include/linux/nvhost.h | 143 ++
> 20 files changed, 2298 insertions(+)
[...]
> diff --git a/drivers/video/tegra/host/chip_support.c
> b/drivers/video/tegra/host/chip_support.c
> +#include "chip_support.h"
> +#include "host1x/host1x01.h"
> +
> +struct nvhost_chip_support *nvhost_chip_ops;
> +
> +struct nvhost_chip_support *nvhost_get_chip_ops(void)
> +{
> + return nvhost_chip_ops;
> +}
If you wanna hide "nvhost_chip_ops" from other source files, declare it
as "static". So this is not a static member which means other files is
able to touch it by "extern" but we also define a function to get it,
and this looks redundant.
[...]
> diff --git a/drivers/video/tegra/host/host1x/Makefile
> b/drivers/video/tegra/host/host1x/Makefile
> new file mode 100644
> index 000..330d507
> --- /dev/null
> +++ b/drivers/video/tegra/host/host1x/Makefile
> @@ -0,0 +1,7 @@
> +ccflags-y = -Idrivers/video/tegra/host
> +
> +nvhost-host1x-objs = \
> + host1x.o \
> + host1x01.o
Can we rename this "host1x01.c"? I just really don't like this kind of
variables/files, I mean, I can't imagine the purpose of the file
according to it's name...
[...]
> +
> +static int __devinit nvhost_alloc_resources(struct nvhost_master *host)
> +{
> + int err;
> +
> + err = nvhost_init_chip_support(host);
> + if (err)
> + return err;
> +
> + return 0;
> +}
Just "return nvhost_init_chip_support(host)" is enough. If so, do we
still need this function?
[...]
> +
> +static int __devinit nvhost_probe(struct platform_device *dev)
> +
[...]
> + dev_info(&dev->dev, "initialized\n");
> +
> + return 0;
> +
> +fail:
Add more "free" codes here. Actually, "nvhost_free_resources" frees the
host->intr.syncpt which is not needed to free manually.
Seems at least we need to add "nvhost_syncpt_deinit" here.
[...]
> +
> +static struct of_device_id host1x_match[] __devinitdata = {
> + { .compatible = "nvidia,tegra20-host1x", },
> + { .compatible = "nvidia,tegra30-host1x", },
Again, place tegra30-host1x before tegra20-host1x.
[...]
> +
> +/**
> + * Write a cpu syncpoint increment to the hardware, without touching
> + * the cache. Caller is responsible for host being powered.
> + */
> +static void host1x_syncpt_cpu_incr(struct nvhost_syncpt *sp, u32 id)
> +{
> + struct nvhost_master *dev = syncpt_to_dev(sp);
> + u32 reg_offset = id / 32;
> +
> + if (!nvhost_module_powered(dev->dev)) {
> + dev_err(&syncpt_to_dev(sp)->dev->dev,
> + "Trying to access host1x when it's off");
> + return;
> + }
> +
> + if (!nvhost_syncpt_client_managed(sp, id)
> + && nvhost_syncpt_min_eq_max(sp, id)) {
> + dev_err(&syncpt_to_dev(sp)->dev->dev,
> + "Trying to increment syncpoint id %d beyond max\n",
> + id);
> + return;
> + }
> + writel(BIT_MASK(id), dev->sync_aperture +
> + host1x_sync_syncpt_cpu_incr_r() + reg_offset * 4);
I have a stupid question: According to the name and the context of this
function, seems it increases the syncpt value which specified by param
"id". So how does this "writel" increase the value? I don't know much
about host1x/syncpt reg operations, so could you explain a little bit or
I just completely have a wrong understanding?
[...]
> +
> +static ssize_t powergate_delay_store(struct kobject *kobj,
> + struct kobj_attr
