Re: [PATCH v1 1/2] efuse: IMG Pistachio eFuse Controller

Re: [PATCH v1 1/2] efuse: IMG Pistachio eFuse Controller

[James Hogan]

Hi,

Some comments below.

Should this driver live in drivers/misc/fuse?

Generally, do we think there should be an efuse subsystem for presenting
efuse data in a standard way to other drivers and userland?

On 17/11/14 15:19, arul.ramasamy-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org wrote:
> From: Arul Ramasamy <Arul.Ramasamy-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> 
> The Pistachio SOC from Imagination Technologies includes a eFuse Controller
> which exposes the status of 128 EFUSE bits to the external world.
> 
> Signed-off-by: Arul Ramasamy <Arul.Ramasamy-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Jude Abraham <Jude.Abraham-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Naidu Tellapati <Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/soc/Kconfig                    |   1 +
>  drivers/soc/Makefile                   |   1 +
>  drivers/soc/pistachio/Makefile         |   1 +
>  drivers/soc/pistachio/fuse/Kconfig     |   9 ++
>  drivers/soc/pistachio/fuse/Makefile    |   1 +
>  drivers/soc/pistachio/fuse/img-efuse.c | 184 +++++++++++++++++++++++++++++++++
>  include/soc/pistachio/img-efuse.h      |  17 +++
>  7 files changed, 214 insertions(+)
>  create mode 100644 drivers/soc/pistachio/Makefile
>  create mode 100644 drivers/soc/pistachio/fuse/Kconfig
>  create mode 100644 drivers/soc/pistachio/fuse/Makefile
>  create mode 100644 drivers/soc/pistachio/fuse/img-efuse.c
>  create mode 100644 include/soc/pistachio/img-efuse.h
> 
> diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> index 76d6bd4..f549573 100644
> --- a/drivers/soc/Kconfig
> +++ b/drivers/soc/Kconfig
> @@ -1,5 +1,6 @@
>  menu "SOC (System On Chip) specific Drivers"
>  
> +source "drivers/soc/pistachio/fuse/Kconfig"
>  source "drivers/soc/qcom/Kconfig"
>  source "drivers/soc/ti/Kconfig"
>  source "drivers/soc/versatile/Kconfig"
> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> index 063113d..d14af91 100644
> --- a/drivers/soc/Makefile
> +++ b/drivers/soc/Makefile
> @@ -2,6 +2,7 @@
>  # Makefile for the Linux Kernel SOC specific device drivers.
>  #
>  
> +obj-$(CONFIG_SOC_IMG)           += pistachio/

inconsistent whitespace. The others use tabs.

What is CONFIG_SOC_IMG? It sounds very generic.

>  obj-$(CONFIG_ARCH_QCOM)		+= qcom/
>  obj-$(CONFIG_ARCH_TEGRA)	+= tegra/
>  obj-$(CONFIG_SOC_TI)		+= ti/
> diff --git a/drivers/soc/pistachio/Makefile b/drivers/soc/pistachio/Makefile
> new file mode 100644
> index 0000000..4d50c94
> --- /dev/null
> +++ b/drivers/soc/pistachio/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_SOC_IMG)	+=	fuse/
> diff --git a/drivers/soc/pistachio/fuse/Kconfig b/drivers/soc/pistachio/fuse/Kconfig
> new file mode 100644
> index 0000000..1a303f0
> --- /dev/null
> +++ b/drivers/soc/pistachio/fuse/Kconfig
> @@ -0,0 +1,9 @@
> +config IMG_EFUSE
> +        tristate "Imagination Technologies eFuse driver"

you have spaces here instead of tabs

> +	depends on HAS_IOMEM
> +        help

this should be indented with a single tab

> +          eFuse driver for Imagination Technologies Pistachio SoC which exposes
> +	  128 bits.
> +
> +	  To compile this driver as a module, choose M here: the module will
> +	  be called img-efuse.

and help text with 1 tab and 2 spaces.

> diff --git a/drivers/soc/pistachio/fuse/Makefile b/drivers/soc/pistachio/fuse/Makefile
> new file mode 100644
> index 0000000..0a78b1a
> --- /dev/null
> +++ b/drivers/soc/pistachio/fuse/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_IMG_EFUSE)	+=	img-efuse.o
> diff --git a/drivers/soc/pistachio/fuse/img-efuse.c b/drivers/soc/pistachio/fuse/img-efuse.c
> new file mode 100644
> index 0000000..25ed2e0
> --- /dev/null
> +++ b/drivers/soc/pistachio/fuse/img-efuse.c
> @@ -0,0 +1,184 @@
> +/*
> + * Imagination Technologies eFuse Driver.
> + *
> + * Copyright (c) 2014 Imagination Technologies Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * Based on drivers/misc/eeprom/sunxi_sid.c Copyright (c) 2013 Oliver Schinagl
> + */
> +#include <linux/clk.h>
> +#include <linux/compiler.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/export.h>
> +#include <linux/fs.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/kobject.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/stat.h>
> +#include <linux/sysfs.h>
> +#include <linux/types.h>
> +
> +#include <soc/pistachio/img-efuse.h>
> +
> +#define	DRV_NAME		"img-efuse"
> +#define	MAX_EFUSE_BYTE_OFFSET	12
> +
> +struct img_efuse *efuse_dev;
> +
> +static u8 img_efuse_read_byte(const unsigned int offset)
> +{
> +	u32 data;
> +
> +	if (offset >= efuse_dev->size)
> +		return 0;
> +
> +	data = readl(efuse_dev->base + round_down(offset, 4));
> +	data >>= (offset % 4) * 8;
> +
> +	return data; /* Only return the last byte */

The last byte? You mean the least significant byte?

> +}
> +
> +int img_efuse_readl(unsigned int offset, u32 *value)
> +{
> +	if ((offset > MAX_EFUSE_BYTE_OFFSET) || ((offset % 4) != 0))

why not range check against efuse_dev->size as above, since you're
already depending on efuse_dev and efuse_dev->base being set.

> +		return -EINVAL;
> +
> +	*value = readl(efuse_dev->base + offset);

If efuse_dev hasn't been set yet (maybe probe failed for some reason, or
you messed up in the DT file by accident), you'll oops here. Maybe worth
just returning -ENODEV in that case?

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(img_efuse_readl);
> +
> +static ssize_t img_efuse_read(struct file *fd, struct kobject *kobj,
> +			      struct bin_attribute *attr, char *buf,
> +			      loff_t pos, size_t size)
> +{
> +	int i;
> +	struct platform_device *pdev;
> +	struct img_efuse *efuse;
> +
> +	pdev = to_platform_device(kobj_to_dev(kobj));
> +	efuse = platform_get_drvdata(pdev);
> +
> +	if (pos < 0 || pos >= efuse->size)
> +		return 0;
> +
> +	if (size > efuse->size - pos)
> +		size = efuse->size - pos;
> +
> +	for (i = 0; i < size; i++)
> +		buf[i] = img_efuse_read_byte(pos + i);
> +
> +	return i;
> +}
> +
> +static struct bin_attribute img_efuse_bin_attr = {
> +	.attr = { .name = "efuse", .mode = S_IRUGO, },
> +	.read = img_efuse_read,
> +};
> +
> +static int img_efuse_remove(struct platform_device *pdev)
> +{
> +	struct img_efuse *efuse;
> +
> +	efuse = platform_get_drvdata(pdev);
> +
> +	clk_disable_unprepare(efuse->osc_clk);
> +	clk_disable_unprepare(efuse->sys_clk);
> +
> +	device_remove_bin_file(&pdev->dev, &img_efuse_bin_attr);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id img_efuse_of_match[] = {
> +	{ .compatible = "img,pistachio-efuse", .data = (void *)16},
> +	{/* sentinel */},
> +};
> +MODULE_DEVICE_TABLE(of, img_efuse_of_match);
> +
> +static int img_efuse_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct resource *res;
> +	const struct of_device_id *of_dev_id;
> +
> +	efuse_dev = devm_kzalloc(&pdev->dev, sizeof(struct img_efuse *),
> +				 GFP_KERNEL);
> +	if (!efuse_dev)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	efuse_dev->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(efuse_dev->base))
> +		return PTR_ERR(efuse_dev->base);
> +
> +	of_dev_id = of_match_device(img_efuse_of_match, &pdev->dev);
> +	if (!of_dev_id)
> +		return -ENODEV;
> +	efuse_dev->size = (int)of_dev_id->data;

Isn't the size of the IO memory region already provided in res, i.e.
resource_size(res). Would that be sufficient for your purposes.

> +
> +	efuse_dev->sys_clk = devm_clk_get(&pdev->dev, "sys");
> +	if (IS_ERR(efuse_dev->sys_clk)) {
> +		dev_err(&pdev->dev, "failed to get system clock");
> +		return PTR_ERR(efuse_dev->sys_clk);
> +	}
> +
> +	efuse_dev->osc_clk = devm_clk_get(&pdev->dev, "efuse");
> +	if (IS_ERR(efuse_dev->osc_clk)) {
> +		dev_err(&pdev->dev, "failed to get oscillator clock");
> +		return PTR_ERR(efuse_dev->osc_clk);
> +	}
> +
> +	ret = clk_prepare_enable(efuse_dev->sys_clk);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "could not enable system clock.\n");
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(efuse_dev->osc_clk);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "could not enable oscillator clock.\n");
> +		goto disable_sysclk;
> +	}
> +
> +	platform_set_drvdata(pdev, efuse_dev);
> +
> +	img_efuse_bin_attr.size = efuse_dev->size;
> +	if (device_create_bin_file(&pdev->dev, &img_efuse_bin_attr)) {
> +		ret = -ENODEV;
> +		goto disable_oscclk;
> +	}
> +
> +	return 0;
> +
> +disable_oscclk:
> +	clk_disable_unprepare(efuse_dev->osc_clk);
> +disable_sysclk:
> +	clk_disable_unprepare(efuse_dev->sys_clk);
> +	return ret;
> +}
> +
> +static struct platform_driver img_efuse_driver = {
> +	.probe = img_efuse_probe,
> +	.remove = img_efuse_remove,
> +	.driver = {
> +		.name = DRV_NAME,
> +		.owner = THIS_MODULE,

No need to set THIS_MODULE when you use module_platform_driver.

> +		.of_match_table = img_efuse_of_match,
> +	},
> +};
> +module_platform_driver(img_efuse_driver);
> +
> +MODULE_AUTHOR("Arul Ramasamy<Arul.Ramasamy-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>");
> +MODULE_AUTHOR("Jude Abraham<Jude.Abraham-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>");
> +MODULE_DESCRIPTION("Imagination Technologies eFuse Driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/soc/pistachio/img-efuse.h b/include/soc/pistachio/img-efuse.h
> new file mode 100644
> index 0000000..e2e738b
> --- /dev/null
> +++ b/include/soc/pistachio/img-efuse.h
> @@ -0,0 +1,17 @@
> +/*
> + * Imagination Technologies Pistachio eFuse driver
> + *
> + * Copyright (C) 2014 Imagination Technologies Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */

No include guards?

> +struct img_efuse {
> +	void __iomem *base;
> +	unsigned int size;
> +	struct clk *osc_clk;
> +	struct clk *sys_clk;
> +};

This looks driver internal, especially since you have a DT binding.
Maybe just have it in the driver source file.

> +
> +int img_efuse_readl(unsigned int offset, u32 *value);

Cheers
James
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 1/2] efuse: IMG Pistachio eFuse Controller

[Ezequiel Garcia]

Hi James,

On 11/17/2014 12:47 PM, James Hogan wrote:
> Hi,
> 
> Some comments below.
> 
> Should this driver live in drivers/misc/fuse?
> 

You might want to take a look at the discussion around Tegra's eFuse
driver, which was originally proposed for drivers/misc and then got
merged in drivers/soc.

I'm curious about who should merge this driver. The Tegra one got merged
through ARM SoC. Maybe this time gregkh can take it directly, given
there's no maintainer for this.

-- 
Ezequiel
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 1/2] efuse: IMG Pistachio eFuse Controller

[James Hogan]

On Mon, Nov 17, 2014 at 05:02:13PM -0300, Ezequiel Garcia wrote:
> Hi James,
> 
> On 11/17/2014 12:47 PM, James Hogan wrote:
> > Hi,
> > 
> > Some comments below.
> > 
> > Should this driver live in drivers/misc/fuse?
> > 
> 
> You might want to take a look at the discussion around Tegra's eFuse
> driver, which was originally proposed for drivers/misc and then got
> merged in drivers/soc.

Thanks, I hadn't spotted that it got merged elsewhere (must've been
looking at an older versions of the patchset).

Curiously the tegra commit 783c8f4c84451bc444e314a71b447239c6ef6fd9 did
add an unused Makefile in drivers/misc/fuse/ ...

Cheers
James
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 1/2] efuse: IMG Pistachio eFuse Controller

[Ezequiel Garcia]

On 11/17/2014 06:29 PM, James Hogan wrote:
> On Mon, Nov 17, 2014 at 05:02:13PM -0300, Ezequiel Garcia wrote:
>> Hi James,
>>
>> On 11/17/2014 12:47 PM, James Hogan wrote:
>>> Hi,
>>>
>>> Some comments below.
>>>
>>> Should this driver live in drivers/misc/fuse?
>>>
>>
>> You might want to take a look at the discussion around Tegra's eFuse
>> driver, which was originally proposed for drivers/misc and then got
>> merged in drivers/soc.
> 
> Thanks, I hadn't spotted that it got merged elsewhere (must've been
> looking at an older versions of the patchset).
> 
> Curiously the tegra commit 783c8f4c84451bc444e314a71b447239c6ef6fd9 did
> add an unused Makefile in drivers/misc/fuse/ ...
> 

And the fix for that is waiting: https://lkml.org/lkml/2014/10/28/848
-- 
Ezequiel
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v1 1/2] efuse: IMG Pistachio eFuse Controller

[Naidu Tellapati]

Hi Andrew,

(We will respond to James Hogan's remaining review comments in a separate email)

>>
>> +obj-$(CONFIG_SOC_IMG)           += pistachio/

> What is CONFIG_SOC_IMG? It sounds very generic.

May I have your suggestions on the above.

(Assuming we create drivers/soc/img/ instead of drivers/soc/pistachio/)


>>  obj-$(CONFIG_ARCH_QCOM)              += qcom/
>>  obj-$(CONFIG_ARCH_TEGRA)     += tegra/
>>  obj-$(CONFIG_SOC_TI)         += ti/
>> diff --git a/drivers/soc/pistachio/Makefile b/drivers/soc/pistachio/Makefile
>> new file mode 100644
>> index 0000000..4d50c94

> Cheers
> James


Regards,
Naidu.--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 1/2] efuse: IMG Pistachio eFuse Controller

[Andrew Bresticker]

Hi Naidu,

On Mon, Nov 17, 2014 at 4:12 PM, Naidu Tellapati
<Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:
> Hi Andrew,
>
> (We will respond to James Hogan's remaining review comments in a separate email)
>
>>>
>>> +obj-$(CONFIG_SOC_IMG)           += pistachio/
>
>> What is CONFIG_SOC_IMG? It sounds very generic.
>
> May I have your suggestions on the above.
>
> (Assuming we create drivers/soc/img/ instead of drivers/soc/pistachio/)

I don't have much visibility into ImgTec SoCs other than Pistachio,
but I think introducing a SOC_IMG Kconfig symbol is a good idea as it
gives drivers for various pieces of IP present on ImgTec SoCs
(watchdog, I2C, DMA, etc.) a more specific dependency than say MIPS or
METAG.  Maybe James has a suggestion for how to deal with this?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 1/2] efuse: IMG Pistachio eFuse Controller

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 1669 bytes --]

On 18/11/14 00:26, Andrew Bresticker wrote:
> Hi Naidu,
> 
> On Mon, Nov 17, 2014 at 4:12 PM, Naidu Tellapati
> <Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:
>> Hi Andrew,
>>
>> (We will respond to James Hogan's remaining review comments in a separate email)
>>
>>>>
>>>> +obj-$(CONFIG_SOC_IMG)           += pistachio/
>>
>>> What is CONFIG_SOC_IMG? It sounds very generic.
>>
>> May I have your suggestions on the above.
>>
>> (Assuming we create drivers/soc/img/ instead of drivers/soc/pistachio/)

What would "belong" in there?
Basically anything that doesn't belong in some particular subsystem?

> I don't have much visibility into ImgTec SoCs other than Pistachio,
> but I think introducing a SOC_IMG Kconfig symbol is a good idea as it
> gives drivers for various pieces of IP present on ImgTec SoCs
> (watchdog, I2C, DMA, etc.) a more specific dependency than say MIPS or
> METAG.  Maybe James has a suggestion for how to deal with this?

I have very limited experience with IMG based SoCs other than Chorus2
and TZ1090, but I'm not convinced an SOC_IMG adds any value tbh if it is
simply a way to group drivers. Both SOC_IMG and METAG || MIPS would end
up being fuzzy/inexact convenience dependencies to reduce clutter on
other platforms, so perhaps it would be best kept simple, flexible and
easily understandable. I don't feel strongly about it either way though.

I suspect certain IP blocks may potentially get licensed separately and
require exceptions (PDP springs to mind?), so it probably is technically
a per-IP block thing. James Hartley might have a better idea about that.

Cheers
James


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v1 1/2] efuse: IMG Pistachio eFuse Controller

[Arnd Bergmann]

On Tuesday 18 November 2014 09:45:17 James Hogan wrote:
> On 18/11/14 00:26, Andrew Bresticker wrote:
> > Hi Naidu,
> > 
> > On Mon, Nov 17, 2014 at 4:12 PM, Naidu Tellapati
> > <Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:
> >> Hi Andrew,
> >>
> >> (We will respond to James Hogan's remaining review comments in a separate email)
> >>
> >>>>
> >>>> +obj-$(CONFIG_SOC_IMG)           += pistachio/
> >>
> >>> What is CONFIG_SOC_IMG? It sounds very generic.
> >>
> >> May I have your suggestions on the above.
> >>
> >> (Assuming we create drivers/soc/img/ instead of drivers/soc/pistachio/)
> 
> What would "belong" in there?
> Basically anything that doesn't belong in some particular subsystem?
> 
> 

I think we have reached the threshold of needing a proper subsystem for
efuse/otp devices, and we should move the existing drivers into one
common directory and find a maintainer who can generalize the interfaces.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 1/2] efuse: IMG Pistachio eFuse Controller

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 1275 bytes --]

On 18/11/14 10:03, Arnd Bergmann wrote:
> On Tuesday 18 November 2014 09:45:17 James Hogan wrote:
>> On 18/11/14 00:26, Andrew Bresticker wrote:
>>> Hi Naidu,
>>>
>>> On Mon, Nov 17, 2014 at 4:12 PM, Naidu Tellapati
>>> <Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:
>>>> Hi Andrew,
>>>>
>>>> (We will respond to James Hogan's remaining review comments in a separate email)
>>>>
>>>>>>
>>>>>> +obj-$(CONFIG_SOC_IMG)           += pistachio/
>>>>
>>>>> What is CONFIG_SOC_IMG? It sounds very generic.
>>>>
>>>> May I have your suggestions on the above.
>>>>
>>>> (Assuming we create drivers/soc/img/ instead of drivers/soc/pistachio/)
>>
>> What would "belong" in there?
>> Basically anything that doesn't belong in some particular subsystem?
>>
>>
> 
> I think we have reached the threshold of needing a proper subsystem for
> efuse/otp devices, and we should move the existing drivers into one
> common directory and find a maintainer who can generalize the interfaces.

In case anybody is interested, Zubair summarised sunxi and JZ4780 efuse
contents here:
https://groups.google.com/d/msg/mips-creator-ci20-dev/-PeqbbifWAQ/Y-tQQfnHqEwJ

It'd be interesting to hear what the IMG one is intended to hold.

Cheers
James


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v1 1/2] efuse: IMG Pistachio eFuse Controller

[James Hartley]

On 11/18/14 09:45, James Hogan wrote:
> On 18/11/14 00:26, Andrew Bresticker wrote:
>> Hi Naidu,
>>
>> On Mon, Nov 17, 2014 at 4:12 PM, Naidu Tellapati
>> <Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:
>>> Hi Andrew,
>>>
>>> (We will respond to James Hogan's remaining review comments in a separate email)
>>>
>>>>> +obj-$(CONFIG_SOC_IMG)           += pistachio/
>>>> What is CONFIG_SOC_IMG? It sounds very generic.
>>> May I have your suggestions on the above.
>>>
>>> (Assuming we create drivers/soc/img/ instead of drivers/soc/pistachio/)
> What would "belong" in there?
> Basically anything that doesn't belong in some particular subsystem?
>
>> I don't have much visibility into ImgTec SoCs other than Pistachio,
>> but I think introducing a SOC_IMG Kconfig symbol is a good idea as it
>> gives drivers for various pieces of IP present on ImgTec SoCs
>> (watchdog, I2C, DMA, etc.) a more specific dependency than say MIPS or
>> METAG.  Maybe James has a suggestion for how to deal with this?
> I have very limited experience with IMG based SoCs other than Chorus2
> and TZ1090, but I'm not convinced an SOC_IMG adds any value tbh if it is
> simply a way to group drivers. Both SOC_IMG and METAG || MIPS would end
> up being fuzzy/inexact convenience dependencies to reduce clutter on
> other platforms, so perhaps it would be best kept simple, flexible and
> easily understandable. I don't feel strongly about it either way though.
>
> I suspect certain IP blocks may potentially get licensed separately and
> require exceptions (PDP springs to mind?), so it probably is technically
> a per-IP block thing. James Hartley might have a better idea about that.
>
> Cheers
> James
>
I think there is value in keeping the clutter down in the kernel config, 
so if there is already precedent for using a symbol such as IMG_SOC then 
I don't see a strong reason not to do it.  IP blocks present in 
pistachio may also be used in other (non MIPS or META) SoCs, so provided 
it is acceptable to expect other SoC providers to add their own symbols 
when required I think we should go ahead with it.

James.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 1/2] efuse: IMG Pistachio eFuse Controller

[Andrew Bresticker]

On Mon, Nov 17, 2014 at 7:19 AM,  <arul.ramasamy-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:
> From: Arul Ramasamy <Arul.Ramasamy-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
>
> The Pistachio SOC from Imagination Technologies includes a eFuse Controller
> which exposes the status of 128 EFUSE bits to the external world.
>
> Signed-off-by: Arul Ramasamy <Arul.Ramasamy-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Jude Abraham <Jude.Abraham-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Naidu Tellapati <Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>

>  drivers/soc/Kconfig                    |   1 +
>  drivers/soc/Makefile                   |   1 +
>  drivers/soc/pistachio/Makefile         |   1 +
>  drivers/soc/pistachio/fuse/Kconfig     |   9 ++
>  drivers/soc/pistachio/fuse/Makefile    |   1 +
>  drivers/soc/pistachio/fuse/img-efuse.c | 184 +++++++++++++++++++++++++++++++++
>  include/soc/pistachio/img-efuse.h      |  17 +++
>  7 files changed, 214 insertions(+)
>  create mode 100644 drivers/soc/pistachio/Makefile
>  create mode 100644 drivers/soc/pistachio/fuse/Kconfig
>  create mode 100644 drivers/soc/pistachio/fuse/Makefile
>  create mode 100644 drivers/soc/pistachio/fuse/img-efuse.c
>  create mode 100644 include/soc/pistachio/img-efuse.h

Is this IP present on other ImgTec SoCs?  Perhaps it makes more sense
to create drivers/soc/img/ instead of drivers/soc/pistachio/?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v1 1/2] efuse: IMG Pistachio eFuse Controller

[Naidu Tellapati]

Hi Andrew,

Many thanks for the review.


> On Mon, Nov 17, 2014 at 7:19 AM,  <arul.ramasamy-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:
>> From: Arul Ramasamy <Arul.Ramasamy-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
>>
>> The Pistachio SOC from Imagination Technologies includes a eFuse Controller
>> which exposes the status of 128 EFUSE bits to the external world.
>>
>> Signed-off-by: Arul Ramasamy <Arul.Ramasamy-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
>> Signed-off-by: Jude Abraham <Jude.Abraham-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
>> Signed-off-by: Naidu Tellapati <Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>

>>  drivers/soc/Kconfig                    |   1 +
>>  drivers/soc/Makefile                   |   1 +
>>  drivers/soc/pistachio/Makefile         |   1 +
>>  drivers/soc/pistachio/fuse/Kconfig     |   9 ++
>>  drivers/soc/pistachio/fuse/Makefile    |   1 +
>>  drivers/soc/pistachio/fuse/img-efuse.c | 184 +++++++++++++++++++++++++++++++++
>>  include/soc/pistachio/img-efuse.h      |  17 +++
>>  7 files changed, 214 insertions(+)
>>  create mode 100644 drivers/soc/pistachio/Makefile
>>  create mode 100644 drivers/soc/pistachio/fuse/Kconfig
>>  create mode 100644 drivers/soc/pistachio/fuse/Makefile
>>  create mode 100644 drivers/soc/pistachio/fuse/img-efuse.c
>>  create mode 100644 include/soc/pistachio/img-efuse.h

> Is this IP present on other ImgTec SoCs?  Perhaps it makes more sense
> to create drivers/soc/img/ instead of drivers/soc/pistachio/?

Ok. Will do it.


Regards,
Naidu.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 2/2] DT: eFuse: Add binding document for IMG Pistachio eFuse Controller

[Andrew Bresticker]

On Mon, Nov 17, 2014 at 7:19 AM,  <arul.ramasamy-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:
> From: Arul Ramasamy <Arul.Ramasamy-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
>
> Add binding document for IMG eFuse Controller present on Pistachio SOC.
>
> Signed-off-by: Arul Ramasamy <Arul.Ramasamy-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Jude Abraham <Jude.Abraham-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Naidu Tellapati <Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> ---
>  .../devicetree/bindings/soc/pistachio/img-efuse.txt    | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/pistachio/img-efuse.txt
>
> diff --git a/Documentation/devicetree/bindings/soc/pistachio/img-efuse.txt b/Documentation/devicetree/bindings/soc/pistachio/img-efuse.txt
> new file mode 100644
> index 0000000..8643a21
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/pistachio/img-efuse.txt
> @@ -0,0 +1,18 @@
> +* IMG Pistachio eFuse controller
> +
> +Required properties:
> +- compatible: Must be "img,pistachio-efuse".
> +- reg: Must contain the base address and length of the eFuse registers.
> +- clocks: Must contain an entry for each entry in clock-names.
> +  See ../clock/clock-bindings.txt for details.
> +- clock-names: Must include the following entries:
> +  - efuse: External oscillator clock

How is the external oscillator related to efuse?  Also, perhaps it
should be called "osc" since it's not an efuse-specific clock.

> +  - sys: eFuse system interface clock

I don't see a system interface gate clock for efuse in the TRM...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v1 2/2] DT: eFuse: Add binding document for IMG Pistachio eFuse Controller

[Naidu Tellapati]

Hi Andrew,

Many thanks for the review.

>> +++ b/Documentation/devicetree/bindings/soc/pistachio/img-efuse.txt
>> @@ -0,0 +1,18 @@
>> +* IMG Pistachio eFuse controller
>> +
>> +Required properties:
>> +- compatible: Must be "img,pistachio-efuse".
>> +- reg: Must contain the base address and length of the eFuse registers.
>> +- clocks: Must contain an entry for each entry in clock-names.
>> +  See ../clock/clock-bindings.txt for details.
>> +- clock-names: Must include the following entries:
>> +  - efuse: External oscillator clock

> How is the external oscillator related to efuse?  Also, perhaps it
> should be called "osc" since it's not an efuse-specific clock.

This is what I read from the eFuse Controller TRM
(Generic eFuse Controller.Technical Reference Manual.pdf) with respect to this clock.

"Free-running oscillator clock – used to clock the fuse-unload state machine. < 50Mhz"

Please comment.

>> +  - sys: eFuse system interface clock

> I don't see a system interface gate clock for efuse in the TRM ...

This is what I read from the above document about the sys_clk.

"System bus clock, synchronous to the IMGBus1 input. < 400 MHz."

I think this clock enables access to shadow RAM where the eFuses status is stored.

Please comment.

regards,
Naidu.





--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 2/2] DT: eFuse: Add binding document for IMG Pistachio eFuse Controller

[Andrew Bresticker]

Hi Naidu,

On Mon, Nov 17, 2014 at 3:34 PM, Naidu Tellapati
<Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:
> Hi Andrew,
>
> Many thanks for the review.
>
>>> +++ b/Documentation/devicetree/bindings/soc/pistachio/img-efuse.txt
>>> @@ -0,0 +1,18 @@
>>> +* IMG Pistachio eFuse controller
>>> +
>>> +Required properties:
>>> +- compatible: Must be "img,pistachio-efuse".
>>> +- reg: Must contain the base address and length of the eFuse registers.
>>> +- clocks: Must contain an entry for each entry in clock-names.
>>> +  See ../clock/clock-bindings.txt for details.
>>> +- clock-names: Must include the following entries:
>>> +  - efuse: External oscillator clock
>
>> How is the external oscillator related to efuse?  Also, perhaps it
>> should be called "osc" since it's not an efuse-specific clock.
>
> This is what I read from the eFuse Controller TRM
> (Generic eFuse Controller.Technical Reference Manual.pdf) with respect to this clock.
>
> "Free-running oscillator clock – used to clock the fuse-unload state machine. < 50Mhz"
>
> Please comment.

Hmm.. is this the 52Mhz external oscillator on Pistachio?  Or something else?

>>> +  - sys: eFuse system interface clock
>
>> I don't see a system interface gate clock for efuse in the TRM ...
>
> This is what I read from the above document about the sys_clk.
>
> "System bus clock, synchronous to the IMGBus1 input. < 400 MHz."
>
> I think this clock enables access to shadow RAM where the eFuses status is stored.

I don't see a bit in CR_PERIP_CLKEN that corresponds to this... Is
this clock derived directly from SYSCLKOUT or PERIPHSYSCLKOUT (i.e. no
gate specifically for efuse)?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v1 1/2] efuse: IMG Pistachio eFuse Controller

[Arul Ramasamy]

Hi James,

[...]

>>> +    if (offset >= efuse_dev->size)
>>> +            return 0;
>>> +
>>> +    data = readl(efuse_dev->base + round_down(offset, 4));
>> +    data >>= (offset % 4) * 8;
>>> +
>>> +    return data; /* Only return the last byte */

>>  The last byte? You mean the least significant byte?

> Yes, it returns the least significant byte.

Since the return type is u8 the function returns last byte in the resultant data after the right shift operation.
If the user space asks for nth byte (offset = n, where  n = 0 to 15) we will return nth byte from the eFuse shadow RAM.

There are total 128 Bits (16 bytes) of Shadow RAM reserved for eFuse status.

[...]

Regards,
R.Arul Raj

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html