[PATCH v3 2/7] mfd: omap: control: core system control driver

[Valentin, Eduardo]

Hello,

On Thu, Jun 28, 2012 at 1:55 PM, Valentin, Eduardo
<eduardo.valentin at ti.com> wrote:
> Hello again,
>
> On Thu, Jun 28, 2012 at 1:51 PM, Valentin, Eduardo
> <eduardo.valentin at ti.com> wrote:
>> Hello,
>>
>> On Thu, Jun 28, 2012 at 1:26 PM, Konstantin Baydarov
>> <kbaidarov at dev.rtsoft.ru> wrote:
>>> On 06/28/2012 02:12 PM, Konstantin Baydarov wrote:
>>>> The interface(design) of omap-control-core.c has already been discussed many times :(
>>>> Eduardo, in his patch set, suggested following design:
>>>> - omap-control-core.c ioremaps SCM window and provide functions to read/write SCP register for bandgap and usb.
>>>>
>>>> IIRC, this approach didn't satisfy and it was suggested to have private read/write in bandgap and usb.
>>>>
>>>> So, my patch set introduces following design:
>>>> - omap-control-core.c don't provide read/write functions for bandgap and usb.
>>>> - bandgap and usb use their own private read/write functions
>>>> - Each omap-control-core.c, bandgap and usb drivers remap SCM window. It's OK because SCM window is statically mapped to the same virtual address. But the problem is that SMP memory window isn't protected. I'm not sure whether it's possible to protect SCM window using this approach.
>>> I mean:
>>>
>>> - Each omap-control-core.c, bandgap and usb drivers remap SCM window. It's OK because SCM window is statically mapped. So each call of ioremap in omap-control-core.c, bandgap and usb drivers returns the same virtual address. But the problem is that SCM memory window isn't protected. I'm not sure whether it's possible to protect SCM window using this approach(when each driver remaps the same IOMEM).
>>>
>>>>
>>>> Another possible design is:
>>>> - omap-control-core.c ioremaps and reserves SCM IOMEM window
>>>> - omap-control-core.c exports omap_control_get_base(virtual base address is returned) to use in bandgap and usb_phy driver.
>>>> - Bandgap and usb phy uses their own private read/write function.
>>>> IIUC, this way was suggested by Tony.
>>
>> Well I understood slightly different :-)
>>
>> I think the point is not really where to put the access functions, but
>> to have each driver handling a separate part of the the io window. As
>> I said before, so that they don't access each other io area.
>>
>> If you have 1 io window, for the above mentioned constraint, you won't
>> protect anything. So, in that sense, it doesn't make much difference
>> if you have access functions in core, or in the children, as they are
>> all sharing the same io window. Of course, in case we put only 1 io
>> window, for me it is safer if that window is managed in only one
>> place, instead of several places.
>>
>> The question is then, can we split the io area into smaller windows
>> for each children? Considering the children registers are not
>> contiguous :-(. In theory we can put several entries in the 'reg' DT
>> property, but that becomes a bit messy as it will change depending on
>> OMAP version. Anyways, if we split the scm io window into several io
>> smaller areas/chunks, then it makes sense to have access functions in
>> each children.
>>
>>>>
>>>> I guess It's better to settle the design(interface) of omap-control-core.c, bandgap and usb phy and then submit the next version of patch set.
>>>
>>
>> Agreed. Here. We need to decide how to have this design and stick to it.
>
> Once the design is agreed, the series can probably be split into
> parts, so we can have the scm core and its children worked separately

Just to be clear, what I was proposing is to have each driver taking
care of its own io window.

For instance, for BG, assuming we have a DT like this:

		ctrl_module_core: ctrl_module_core at 4a002000 {
			compatible = "ti,omap4-control";
			ti,hwmods = "ctrl_module_core";
			#address-cells = <1>;
			#size-cells = <1>;
			ranges;
			bandgap: bandgap at 4a002000 {
				 reg = <0x4a00232C 0x4 0x4a002378 0x18>;
				 compatible = "ti,omap4460-bandgap";
				 interrupts = <0 126 4>; /* talert */
				 ti,tshut-gpio = <86>; /* tshut */
			 };
			usb {
				compatible = "ti,omap4-usb-phy";
			};
		};

then, while probing, it can request those by simply:

	i = 0;
	do {
		void __iomem *chunk;

		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
		if (!res)
			break;
		chunk = devm_request_and_ioremap(&pdev->dev, res);
		if (i == 0)
			bg_ptr->base = chunk;
		if (!chunk) {
			dev_err(&pdev->dev,
				"failed to request the IO (%d:%pR).\n",
				i, res);
			return ERR_PTR(-EADDRNOTAVAIL);
		}
		i++;
	} while (res);

The driver needs to adapt its reg offsets, of course, it is doable and
with the current design it is just a matter of redefining the register
offsets.

But for the above to work, the core driver must not request the
complete IO area.

>
>>
>>
>> --
>>
>> Eduardo Valentin
>
>
>
> --
>
> Eduardo Valentin



-- 

Eduardo Valentin