Message-id: <54AE6501.8020905@samsung.com>
Date:	Thu, 08 Jan 2015 16:37:45 +0530
From:	Pankaj Dubey <pankaj.dubey@...sung.com>
To:	Philipp Zabel <p.zabel@...gutronix.de>
Cc:	Arnd Bergmann <arnd@...db.de>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	linux-kernel@...r.kernel.org, Tomasz Figa <tomasz.figa@...il.com>,
	Vivek Gautam <gautam.vivek@...sung.com>, kernel@...gutronix.de,
	Lee Jones <lee.jones@...aro.org>,
	Javier Martinez Canillas <javier.martinez@...labora.co.uk>,
	Heiko Stuebner <heiko@...ech.de>
Subject: Re: [PATCH] mfd: syscon: fix syscon probing from dt

Hi Philipp,

On Wednesday 07 January 2015 05:25 PM, Philipp Zabel wrote:
> Hi Pankaj,
>
> Am Mittwoch, den 07.01.2015, 16:47 +0530 schrieb Pankaj Dubey:
>> Hi Philipp,
>>
>> On Wednesday 07 January 2015 04:27 PM, Philipp Zabel wrote:
>>> Am Dienstag, den 06.01.2015, 20:36 +0100 schrieb Arnd Bergmann:
>>>> On Tuesday 06 January 2015 16:30:36 Philipp Zabel wrote:
>>>>> Patch bdb0066df96e ("mfd: syscon: Decouple syscon interface from platform devices")
>>>>> breaks probing pure syscon devices from device tree, such as anatop and
>>>>> iomuxc-gpr on i.MX. This patch adds back the dt id table to match against
>>>>> "syscon" compatible device tree nodes.
>>>>>
>>>>> Signed-off-by: Philipp Zabel <p.zabel@...gutronix.de>
>>>>>
>>>>
>>>> I don't understand it. Why is this required?
>>>
>>> The debugfs entries vanished and I'd like to have a device to register
>>> platform device children to.
>>
>> Yes debugfs entries will not be present for syscon, this is as per
>> discussion happened here [1]. At that time one suggestion came from Arnd
>> that we can have a different representation for syscon regmaps, instead
>> of based on devices just based on of_node pointer.
>>
>> 1: https://lkml.org/lkml/2014/9/26/73
>
> Thank you for the pointer. That would work for me just as well.
>

Well, But so far I do not see any attempt has been done for making this 
kind of debugfs entry using of_node pointer.

>> I was just thinking, missing debugfs entry is your concern, or you want
>> to make child devices of syscon devices? Basically I am still trying to
>> understand your requirement.
>
> Both. I'd like to somehow get the debug entry back, whether by
> optionally associating syscon with a device or by adding special syscon
> support to regmap.
> And I'd like to eventually register platform devices under the syscon
> node in the device tree. Currently in the i.MX device tree we have three
> overlapping nodes next to each other:
>
> 	gpr: iomuxc-gpr@...00000 {
> 		compatible = "fsl,imx6q-iomuxc-gpr", "syscon";
> 		reg = <0x020e0000 0x38>;
> 	};
>
> 	iomuxc: iomuxc@...e0000 {
> 		compatible = "fsl,imx6q-iomuxc";
> 		reg = <0x020e0000 0x4000>;
> 	};
>
> 	ldb: ldb@...e0008 {
> 		compatible = "fsl,imx6q-ldb";
> 		gpr = <&gpr>;
> 	};
>
> The ldb device is controlled purely through the gpr regmap, so in my
> opinion it should be a child to the iomuxc-gpr node:
>
> 	gpr: iomuxc-gpr@...00000 {
> 		compatible = "fsl,imx6q-iomuxc-gpr", "syscon";
> 		reg = <0x020e0000 0x38>;
>
> 		ldb: ldb@...e0008 {
> 			compatible = "fsl,imx6q-ldb";
> 			gpr = <&gpr>;
> 		};
> 	};
>
> 	iomuxc: iomuxc@...e0000 {
> 		compatible = "fsl,imx6q-iomuxc";
> 		reg = <0x020e0000 0x4000>;
> 	};
>
> In that case, if there is no device associated with iomuxc-gpr, which
> device should be the parent to ldb?If the syscon is created from the
> iomuxc device, the ldb device could be made a child to iomuxc.

If you need ldb as child device of gpr, then there is no need of syscon 
or SYSCON APIs here in ldb. In my opinion this is an extra feature which 
are trying to feed into syscon. As in case of sibling devices it makes 
sense to access registers across them using syscon APIs, but in case of 
parent-child devices we already have MFD drivers, and MFD client devices 
which can serve this purpose.

> In fact, since the gpr registers are part of the iomuxc register space,
> one could even think about merging the iomuxg-gpr syscon into the iomuxc
> node:
>
> 	gpr: iomuxc: iomuxc@...e0000 {
> 		compatible = "fsl,imx6q-iomuxc", "fsl,imx6q-iomuxc-gpr";
> 		reg = <0x020e0000 0x4000>;
>
> 		ldb: ldb@...e0008 {
> 			compatible = "fsl,imx6q-ldb";
> 		};
> 	};
>
>> I guess for i.MX iomuxc it could be argued
>>> that the iomuxc-gpr syscon should be merged into the iomuxc pinctrl
>>> device instead of probing iomuxc-gpr as a platform device by itself.

Yes, also in the probe of this driver you can register itself as MFD 
driver and all the child devices and MFD client devices so that child 
devices can be probed. For example a similar approach has been attempted 
for exynos-pmu here [1].

1: http://www.spinics.net/lists/linux-samsung-soc/msg38446.html

While doing this you can still keep "syscon" compatible to the gpr node 
if it's some of it's register will be accessed from some other devices.
But definitely ldb does not need to have a phandle to this syscon 
device, as after being a MFD client device it can access MFD parent 
device address space.

>>> How about allowing to register a syscon for a given device:
>>>
>>> ----8<----
>>> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
>>> index d2280d6..2633b27 100644
>>> --- a/drivers/mfd/syscon.c
>>> +++ b/drivers/mfd/syscon.c
>>> @@ -42,13 +42,14 @@ static struct regmap_config syscon_regmap_config = {
>>>    	.reg_stride = 4,
>>>    };
>>>
>>> -static struct syscon *of_syscon_register(struct device_node *np)
>>> +struct syscon *syscon_register(struct device *dev, struct device_node *np)
>>>    {
>>>    	struct syscon *syscon;
>>>    	struct regmap *regmap;
>>>    	void __iomem *base;
>>>    	int ret;
>>>    	struct regmap_config syscon_config = syscon_regmap_config;
>>> +	struct resource res;
>>>
>>>    	if (!of_device_is_compatible(np, "syscon"))
>>>    		return ERR_PTR(-EINVAL);
>>> @@ -57,7 +58,12 @@ static struct syscon *of_syscon_register(struct device_node *np)
>>>    	if (!syscon)
>>>    		return ERR_PTR(-ENOMEM);
>>>
>>> -	base = of_iomap(np, 0);
>>> +	if (of_address_to_resource(np, 0, &res)) {
>>> +		ret = -ENOMEM;
>>> +		goto err_map;
>>> +	}
>>> +
>>> +	base = ioremap(res.start, resource_size(&res));
>>>    	if (!base) {
>>>    		ret = -ENOMEM;
>>>    		goto err_map;
>>> @@ -69,7 +75,8 @@ static struct syscon *of_syscon_register(struct device_node *np)
>>>    	 else if (of_property_read_bool(np, "little-endian"))
>>>    		syscon_config.val_format_endian = REGMAP_ENDIAN_LITTLE;
>>>
>>> -	regmap = regmap_init_mmio(NULL, base, &syscon_config);
>>> +	syscon_regmap_config.max_register = res.end - res.start - 3;
>>> +	regmap = regmap_init_mmio(dev, base, &syscon_config);
>>>    	if (IS_ERR(regmap)) {
>>>    		pr_err("regmap init failed\n");
>>>    		ret = PTR_ERR(regmap);
>>> @@ -91,6 +98,12 @@ err_map:
>>>    	kfree(syscon);
>>>    	return ERR_PTR(ret);
>>>    }
>>> +EXPORT_SYMBOL_GPL(syscon_register);
>>> +
>>
>> In past a similar approach [2] for letting device driver to register
>> themselves as syscon provided, has been rejected giving reason that
>> syscon should not need it's own platform device.
>
> In that mail Arnd mentions registering a syscon with both an optional
> device and an explicit device_node pointer, which is what this patch
> would do. There is no need for a platform device.
>

I don't think so, as per I recall he was against of any such extra API 
to register syscon [2].

2: https://lkml.org/lkml/2014/9/1/227

> regards
> Philipp
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists