Re: [RFC,v2,1/8] video: tegra: Add nvhost driver

From: Mark Zhang
Date: Thu Nov 29 2012 - 04:10:57 EST

On 11/26/2012 09:19 PM, Terje BergstrÃm <tbergstrom@xxxxxxxxxx> 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 <tbergstrom@xxxxxxxxxx>
>
> ---
> 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 0000000..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_attribute *attr, const char *buf, size_t count)
> +{
> + int powergate_delay = 0, ret = 0;
> + struct nvhost_device_power_attr *power_attribute =
> + container_of(attr, struct nvhost_device_power_attr,
> + power_attr[NVHOST_POWER_SYSFS_ATTRIB_POWERGATE_DELAY]);
> + struct platform_device *dev = power_attribute->ndev;
> + struct nvhost_device_data *pdata = platform_get_drvdata(dev);
> +
> + if (!pdata->can_powergate) {
> + dev_info(&dev->dev, "does not support power-gating\n");
> + return count;
> + }
> +
> + mutex_lock(&pdata->lock);
> + ret = sscanf(buf, "%d", &powergate_delay);
> + if (ret == 1 && powergate_delay >= 0)
> + pdata->powergate_delay = powergate_delay;
> + else
> + dev_err(&dev->dev, "Invalid powergate delay\n");
> + mutex_unlock(&pdata->lock);
> +
> + return count;

Why we need to return an unchanged param? Seems param "count" doesn't
make sense here.

[...]
> +
> +int nvhost_module_init(struct platform_device *dev)
> +{
> + int i = 0, err = 0;
> + struct kobj_attribute *attr = NULL;
> + struct nvhost_device_data *pdata = platform_get_drvdata(dev);
> +
> + /* initialize clocks to known state */
> + while (pdata->clocks[i].name && i < NVHOST_MODULE_MAX_CLOCKS) {
> + long rate = pdata->clocks[i].default_rate;
> + struct clk *c;
> +
> + c = devm_clk_get(&dev->dev, pdata->clocks[i].name);
> + if (IS_ERR_OR_NULL(c)) {
> + dev_err(&dev->dev, "Cannot get clock %s\n",
> + pdata->clocks[i].name);
> + return -ENODEV;
> + }
> +
> + rate = clk_round_rate(c, rate);
> + clk_prepare_enable(c);
> + clk_set_rate(c, rate);
> + clk_disable_unprepare(c);
> + pdata->clk[i] = c;
> + i++;
> + }
> + pdata->num_clks = i;
> +
> + mutex_init(&pdata->lock);
> + init_waitqueue_head(&pdata->idle_wq);
> + INIT_DELAYED_WORK(&pdata->powerstate_down, powerstate_down_handler);
> +
> + /* power gate units that we can power gate */
> + if (pdata->can_powergate) {
> + do_powergate_locked(pdata->powergate_ids[0]);
> + do_powergate_locked(pdata->powergate_ids[1]);

Seems we don't set these 2 powergate_ids. Does this mean we have not
enabled power management feature in this version?

[...]
> +
> +int nvhost_module_suspend(struct platform_device *dev)
> +{
> + int ret;
> + struct nvhost_device_data *pdata = platform_get_drvdata(dev);
> +
> + ret = wait_event_timeout(pdata->idle_wq, is_module_idle(dev),
> + ACM_SUSPEND_WAIT_FOR_IDLE_TIMEOUT);
> + if (ret == 0) {
> + dev_info(&dev->dev, "%s prevented suspend\n",
> + dev_name(&dev->dev));
> + return -EBUSY;
> + }
> +

I'm not sure whether there is a race condition here. We wait until this
module is idle(refcount == 0), then try to powergate it next. But the
wait queue function "powerstate_down_handler" might already powergate
it. So we need to either "cancel_delayed_work(&pdate->powerstate_down)"
before waiting the module to idle state or add some protection codes in
"to_state_powergated_locked".

> + mutex_lock(&pdata->lock);
> + cancel_delayed_work(&pdata->powerstate_down);
> + to_state_powergated_locked(dev);
> + mutex_unlock(&pdata->lock);
> +
> + if (pdata->suspend_ndev)
> + pdata->suspend_ndev(dev);
> +
> + return 0;
> +}
> +
[...]
> +
> +int nvhost_syncpt_init(struct platform_device *dev,
> + struct nvhost_syncpt *sp)
> +{
> + int i;
> + struct nvhost_master *host = syncpt_to_dev(sp);
> + int err = 0;
> +
> + /* Allocate structs for min, max and base values */
> + sp->min_val = kzalloc(sizeof(atomic_t) * nvhost_syncpt_nb_pts(sp),
> + GFP_KERNEL);
> + sp->max_val = kzalloc(sizeof(atomic_t) * nvhost_syncpt_nb_pts(sp),
> + GFP_KERNEL);
> + sp->base_val = kzalloc(sizeof(u32) * nvhost_syncpt_nb_bases(sp),
> + GFP_KERNEL);
> + sp->lock_counts =
> + kzalloc(sizeof(atomic_t) * nvhost_syncpt_nb_mlocks(sp),
> + GFP_KERNEL);
> +
> + if (!(sp->min_val && sp->max_val && sp->base_val && sp->lock_counts)) {
> + /* frees happen in the deinit */
> + err = -ENOMEM;
> + goto fail;
> + }
> +
> + sp->kobj = kobject_create_and_add("syncpt", &dev->dev.kobj);
> + if (!sp->kobj) {
> + err = -EIO;
> + goto fail;
> + }
> +
> + /* Allocate two attributes for each sync point: min and max */
> + sp->syncpt_attrs = kzalloc(sizeof(*sp->syncpt_attrs)
> + * nvhost_syncpt_nb_pts(sp) * 2, GFP_KERNEL);
> + if (!sp->syncpt_attrs) {
> + err = -ENOMEM;
> + goto fail;
> + }
> +
> + /* Fill in the attributes */
> + for (i = 0; i < nvhost_syncpt_nb_pts(sp); i++) {
> + char name[MAX_SYNCPT_LENGTH];
> + struct kobject *kobj;
> + struct nvhost_syncpt_attr *min = &sp->syncpt_attrs[i*2];
> + struct nvhost_syncpt_attr *max = &sp->syncpt_attrs[i*2+1];
> +
> + /* Create one directory per sync point */
> + snprintf(name, sizeof(name), "%d", i);
> + kobj = kobject_create_and_add(name, sp->kobj);

Where do we "kobject_put" this kobj?

[...]
> + if (!kobj) {
> + err = -EIO;
> + goto fail;
> + }
> +
> + min->id = i;
> + min->host = host;
> + min->attr.attr.name = min_name;
> + min->attr.attr.mode = S_IRUGO;
> + min->attr.show = syncpt_min_show;
> + if (sysfs_create_file(kobj, &min->attr.attr)) {
> + err = -EIO;
> + goto fail;
> + }
> +
> + max->id = i;
> + max->host = host;
> + max->attr.attr.name = max_name;
> + max->attr.attr.mode = S_IRUGO;
> + max->attr.show = syncpt_max_show;
> + if (sysfs_create_file(kobj, &max->attr.attr)) {
> + err = -EIO;
> + goto fail;
> + }
> + }
> +
> + return err;
> +
> +fail:
> + nvhost_syncpt_deinit(sp);
> + return err;
> +}
> +
[...]
> +/* public host1x sync-point management APIs */
> +u32 host1x_syncpt_incr_max(u32 id, u32 incrs);
> +void host1x_syncpt_incr(u32 id);
> +u32 host1x_syncpt_read(u32 id);
> +
> +#endif
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/