Heads up on a device tree change

Heads up on a device tree change

[Grant Likely]

Hi Stephen,

I've just pushed out a change which cleans up platform device
registration to use the same path whether or not the device tree is
used. It should be safe, but there is a risk of breakage on powerpc
platforms.

The patch has two effects of note:
- DT generated platform devices move from /sys/devices to under
/sys/devices/platform. Userspace *should* be okay with this, but if
there are any problems then I can post a workaround patch that keeps
DT generiated platform_devices in the current location.
- Resources on platform_devices get registered so they appear in
/proc/iomem and /proc/ioports and so that device drivers get the added
protection of request_region. This will cause breakage on device trees
nodes with partially overlapping memory regions. (ie. 0x100..0x1ff and
0x180..0x27f). I also have a workaround for this, but I doubt that it
will be necessary.

I'm not worried about it, but it is a sufficiently visible change that
I want you to be aware that it is there.

g.

--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: Heads up on a device tree change

[James Hogan]

On 06/02/13 13:11, Grant Likely wrote:
> Hi Stephen,
> 
> I've just pushed out a change which cleans up platform device
> registration to use the same path whether or not the device tree is
> used. It should be safe, but there is a risk of breakage on powerpc
> platforms.
> 
> The patch has two effects of note:
> - DT generated platform devices move from /sys/devices to under
> /sys/devices/platform. Userspace *should* be okay with this, but if
> there are any problems then I can post a workaround patch that keeps
> DT generiated platform_devices in the current location.
> - Resources on platform_devices get registered so they appear in
> /proc/iomem and /proc/ioports and so that device drivers get the added
> protection of request_region. This will cause breakage on device trees
> nodes with partially overlapping memory regions. (ie. 0x100..0x1ff and
> 0x180..0x27f). I also have a workaround for this, but I doubt that it
> will be necessary.

Hi Grant,

If I understand you correctly, the non-overlapping memory regions thing
could be a problem for me. We have a Meta based SoC that has various SoC
registers grouped together for doing GPIOs and Pin control things. I'm
still in the process of converting it to device tree, but the way I've
been handling it is to provide overlapping registers to both the gpio
and pinctl DT nodes. Each GPIO bank's registers are also interleaved
with the others, so I've been providing overlapping register ranges
(offset by 4 for each bank) to the DT node for each gpio bank too, so
each bank can function independently and the driver doesn't have to
worry about multiple banks. Does that sound like a reasonable use case?

I guess I could cheat with the length, or specify each register in it's
own memory resource, but it seems like overkill.

Cheers
James

Re: Heads up on a device tree change

[Grant Likely]

On Wed, Feb 6, 2013 at 1:32 PM, James Hogan <james.hogan@imgtec.com> wrote:
> On 06/02/13 13:11, Grant Likely wrote:
>> Hi Stephen,
>>
>> I've just pushed out a change which cleans up platform device
>> registration to use the same path whether or not the device tree is
>> used. It should be safe, but there is a risk of breakage on powerpc
>> platforms.
>>
>> The patch has two effects of note:
>> - DT generated platform devices move from /sys/devices to under
>> /sys/devices/platform. Userspace *should* be okay with this, but if
>> there are any problems then I can post a workaround patch that keeps
>> DT generiated platform_devices in the current location.
>> - Resources on platform_devices get registered so they appear in
>> /proc/iomem and /proc/ioports and so that device drivers get the added
>> protection of request_region. This will cause breakage on device trees
>> nodes with partially overlapping memory regions. (ie. 0x100..0x1ff and
>> 0x180..0x27f). I also have a workaround for this, but I doubt that it
>> will be necessary.
>
> Hi Grant,
>
> If I understand you correctly, the non-overlapping memory regions thing
> could be a problem for me. We have a Meta based SoC that has various SoC
> registers grouped together for doing GPIOs and Pin control things. I'm
> still in the process of converting it to device tree, but the way I've
> been handling it is to provide overlapping registers to both the gpio
> and pinctl DT nodes. Each GPIO bank's registers are also interleaved
> with the others, so I've been providing overlapping register ranges
> (offset by 4 for each bank) to the DT node for each gpio bank too, so
> each bank can function independently and the driver doesn't have to
> worry about multiple banks. Does that sound like a reasonable use case?
>
> I guess I could cheat with the length, or specify each register in it's
> own memory resource, but it seems like overkill.

Note that overlapping regions are fine /provided/ that they are the
same size or one fits nicely inside another. It's partial overlap that
is a problem

I've been thinking about your exact problem though and I think the
best way to handle it is for the gpio driver to understand multiple
banks. Most memory mapped GPIO controllers should be drivable by the
generic gpio driver anyway, so it is a matter of crafting the binding
so a single node can describe multiple banks. I still need to review
the patch that adds a DT binding for the generic gpio driver.

g.

--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: Heads up on a device tree change

[James Hogan]

On 06/02/13 14:28, Grant Likely wrote:
> On Wed, Feb 6, 2013 at 1:32 PM, James Hogan <james.hogan@imgtec.com> wrote:
>> On 06/02/13 13:11, Grant Likely wrote:
>>> - Resources on platform_devices get registered so they appear in
>>> /proc/iomem and /proc/ioports and so that device drivers get the added
>>> protection of request_region. This will cause breakage on device trees
>>> nodes with partially overlapping memory regions. (ie. 0x100..0x1ff and
>>> 0x180..0x27f). I also have a workaround for this, but I doubt that it
>>> will be necessary.
>>
>> Hi Grant,
>>
>> If I understand you correctly, the non-overlapping memory regions thing
>> could be a problem for me. We have a Meta based SoC that has various SoC
>> registers grouped together for doing GPIOs and Pin control things. I'm
>> still in the process of converting it to device tree, but the way I've
>> been handling it is to provide overlapping registers to both the gpio
>> and pinctl DT nodes. Each GPIO bank's registers are also interleaved
>> with the others, so I've been providing overlapping register ranges
>> (offset by 4 for each bank) to the DT node for each gpio bank too, so
>> each bank can function independently and the driver doesn't have to
>> worry about multiple banks. Does that sound like a reasonable use case?
>>
>> I guess I could cheat with the length, or specify each register in it's
>> own memory resource, but it seems like overkill.
> 
> Note that overlapping regions are fine /provided/ that they are the
> same size or one fits nicely inside another. It's partial overlap that
> is a problem

It still feels a bit artificial to impose that limitation on something
that is supposed to be implementation independent. Having said that it
doesn't particularly bother me having to work around it.

> 
> I've been thinking about your exact problem though and I think the
> best way to handle it is for the gpio driver to understand multiple
> banks.

Something like this works quite nicely for me and keeps the driver code
nice and simple (iterates over children a bit like I2C, no need for
gpio-cells=3). I'd welcome comments:

		gpios: gpios@02005800 {
			#address-cells = <1>;
			#size-cells = <0>;
			compatible = "img,tz1090-gpio";
			reg = <0x02005800 0x90>;

			gpios0: bank@0 {
				#gpio-cells = <2>;
				#interrupt-cells = <2>;
				reg = <0>;
				interrupts = <13 4 /* level */>;
				gpio-controller;
				gpio-ranges = <&pinctrl 0 30>;
				interrupt-controller;
			};
			gpios1: bank@1 {
				#gpio-cells = <2>;
				#interrupt-cells = <2>;
				reg = <1>;
				interrupts = <14 4 /* level */>;
				gpio-controller;
				gpio-ranges = <&pinctrl 30 30>;
				interrupt-controller;
			};
			gpios2: bank@2 {
				#gpio-cells = <2>;
				#interrupt-cells = <2>;
				reg = <2>;
				interrupts = <15 4 /* level */>;
				gpio-controller;
				gpio-ranges = <&pinctrl 60 30>;
				interrupt-controller;
			};
		};

Cheers
James

Re: Heads up on a device tree change

[Grant Likely]

On Thu, 7 Feb 2013 10:32:13 +0000, James Hogan <james.hogan@imgtec.com> wrote:
> On 06/02/13 14:28, Grant Likely wrote:
> > On Wed, Feb 6, 2013 at 1:32 PM, James Hogan <james.hogan@imgtec.com> wrote:
> >> On 06/02/13 13:11, Grant Likely wrote:
> >>> - Resources on platform_devices get registered so they appear in
> >>> /proc/iomem and /proc/ioports and so that device drivers get the added
> >>> protection of request_region. This will cause breakage on device trees
> >>> nodes with partially overlapping memory regions. (ie. 0x100..0x1ff and
> >>> 0x180..0x27f). I also have a workaround for this, but I doubt that it
> >>> will be necessary.
> >>
> >> Hi Grant,
> >>
> >> If I understand you correctly, the non-overlapping memory regions thing
> >> could be a problem for me. We have a Meta based SoC that has various SoC
> >> registers grouped together for doing GPIOs and Pin control things. I'm
> >> still in the process of converting it to device tree, but the way I've
> >> been handling it is to provide overlapping registers to both the gpio
> >> and pinctl DT nodes. Each GPIO bank's registers are also interleaved
> >> with the others, so I've been providing overlapping register ranges
> >> (offset by 4 for each bank) to the DT node for each gpio bank too, so
> >> each bank can function independently and the driver doesn't have to
> >> worry about multiple banks. Does that sound like a reasonable use case?
> >>
> >> I guess I could cheat with the length, or specify each register in it's
> >> own memory resource, but it seems like overkill.
> > 
> > Note that overlapping regions are fine /provided/ that they are the
> > same size or one fits nicely inside another. It's partial overlap that
> > is a problem
> 
> It still feels a bit artificial to impose that limitation on something
> that is supposed to be implementation independent. Having said that it
> doesn't particularly bother me having to work around it.

I've backed out on this. It broke too much.

g.