Message-ID: <56B7735C.6080406@ti.com>
Date:	Sun, 7 Feb 2016 10:39:56 -0600
From:	"Andrew F. Davis" <afd@...com>
To:	Philipp Zabel <p.zabel@...gutronix.de>
CC:	Rob Herring <robh+dt@...nel.org>, Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>, Suman Anna <s-anna@...com>,
	<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] Documentation: dt: reset: Add syscon reset binding

On 02/04/2016 09:49 AM, Philipp Zabel wrote:
> Hi Andrew,
>
> Am Dienstag, den 02.02.2016, 13:25 -0600 schrieb Andrew F. Davis:
> [...]
>>> sti also chose a single address cell and a logical number to enumerate
>>> the resets and to store the actual reset control and status bit position
>>> in a table in the driver. Is there a reason not to follow the same
>>> approach for ti?
>>
>> The number of reset-able modules is rather large, and we would have to
>> have an entry for them, and then a table of them for each chip, I
>> would like to avoid this as it may become unmaintainable with the number
>> of devices we would like to support.
>
> Maybe I underestimate the amount of work necessary to translate the
> register bit positions from the reference manuals into tables in a
> driver (after all the imx-src driver that started this framework only
> has 5 reset controls registered with it), but to me this doesn't sound
> like a very strong argument.
>

You're right it's not a pain for a single device, in fact I think
we only need 2 or 3 resets currently for any given device, I was more
concerned about the number of different devices leading to hard to
maintain sets of tables. But I concede this isn't the best argument,
I'd just like as little chip layout specific information in the driver
as possible.

>> We currently only need to reset one module this way currently anyway, we
>> will be moving away from toggling bits from the host side to perform resets,
>> but rather ask a power management controller to perform the reset for us
>> (I also have a reset driver that communicates with this controller that I
>> will post when the rest of the needed framework is upstreamed). So, for TI,
>> this syscon based driver will probably mostly be used for compatibility with
>> older SoCs that do not support the management controller, allowing new device
>> drivers to use the reset framework and still function with older SoCs.
>
> So this will only be used for a few legacy devices? I'd probably be less
> hesitant if you proposed this as some ti chip specific binding, as right
> now I just don't expect that many other devices to use this supposedly
> generic binding.
>

I'll probably do that as a last resort if we cant get something more
useful from this driver.

>>> This approach of specifying bits in the device tree at
>>> the consumer side doesn't allow any error handling in the driver to
>>> determine if the bits are in fact valid.
>>
>> Yes, that is lost by not having a table of all valid resets, but this would
>> be like many DT driver/node that allow registers/addresses to be specified
>> and then write/read from those.
>
> True. Still, I'd argue that having a list of registers and bit-shifts in
> a single place (whether that is in the driver or in the reset controller
> node), that can be easily checked against a manual. On the other hand
> sprinkling these values around the device tree at the consumer sites
> makes this much harder to review.
>

Hmmm, I wouldn't be opposed to that, it's suggested a couple times below,
so I might see if that works well. I've worked out a little example,
so before I implement these changes, would something like this be more
acceptable?:

(child node of syscon node)
reset: reset {
	compatible = "syscon-reset";
	#reset-cells = <1>;
	#address-cells = <1>;
	#size-cells = <0>;
		
	dsp@0 {
		reg = <0>;
		control = <0xa44 8 RESET_ASSERT_SET>;
	};

	imu@1 {
		reg = <1>;
		control = <0xa46 7 RESET_ASSERT_SET>;
		status = <0x844 7 RESET_ASSERT_CLEAR>;
		toggle-only;
	};
};

then consumers could just

resets = <&reset 0>, <&reset 1>;
resest-names = "dsp", "imu";

like normal.

> [...]
>>>> +Required properties:
>>>> +--------------------
>>>> + - compatible	: Should be "syscon-reset"
>>>
>>> What if later an erratum turns up and something special needs to be done
>>> (delays, special care about other bits in the same register, etc.)?
>>> This should always contain a soc specific compatible.
>>
>> In this case a specific reset driver would be needed for that device, this
>> driver only intends to cover the more traditional base cases.
>
> And for that to work in a backwards compatible way, you need to have the
> SoC specific compatible value already in the device tree.
>

Then it can be added, "syscon-reset" would just be the generic fall-back when
no other more specific driver matches. This is already the case though, or are
you saying you would like the example DT to contain one?

> [...]
>>> This is a binding for single reset bits that are spread throughout the
>>> register space. For any syscon that has a few registers of contiguous
>>> resets this is rather suboptimal.
>>
>> Yes, this is only intended for when a few resets need controlling out of
>> a large reset space without having to directly encode the reset information
>> into the device driver for that hardware module.
>
> How about if we came up with a way to encode the bit fields in the reset
> controller node?
>

Above.

> [...]
>>> The polarity should have some RESET_ACTIVE_HIGH/LOW #define. Using
>>> numbers in the gpio phandle bindings in the beginning has caused a lot
>>> of problems over time.
>>
>> That works, I'll add these. Something like RESET_ASSERT_{SET,CLEAR} work?
>
> Yes, this is better.
>
>>> Do you really have varying polarity across the resets?
>>
>> Not on the part I'm using, but this should keep things generic.
>
> We already established that this binding is not generic enough for most
> of the current cases, so I'm not convinced it is valuable to complicate
> it for some theoretical case.
>
>> The alternative would be to set the polarity per reset controller node, then
>> if a system has both it could have two controllers and the consumers would
>> have a phandle to the correct one, then all consumers would only need 4
>> instead of 6 args. Actually now that I think about it, this is probably the
>> way to go as most systems I would imagine only have one polarity and it still
>> can work for systems that do have both. I think I'll make this change.
>
> How about going one more step and also moving the register and bit-shift
> description into a property of the reset controller node?
>

Above.

>>>> +		dsp0: dsp0 {
>>>> +			...
>>>> +			resets = <&pscrst 0xa3c 8 0 0x83c 8 0>;
>>>
>>> This sounds a bit error prone and rather verbose for all controllers
>>> that don't have control and status bits peppered randomly around the
>>> register space.
>>
>> I was also thinking about adding the ability to have only one set of args
>> for control, then we just return ENOTSUPP when asked for status when only
>> the control register is provided.
>
> Yes, that should work.
>
>> With the above polarity change, we end up allowing:
>>
>> resets = <&pscrst 0xa3c 8>;
>
> Would this be a reset controller with no control bit, but just a trigger
> that is triggered when the bit is set? (or cleared?).
>

Should be addressed by above binding change.

>> when appropriate. This would cover many common use-cases and keep the
>> framework clean for unique case drivers when needed. It would eliminate
>> the need for many reset-berlin like drivers that only differentiate
>> themselves in trivial ways, like offsets/polarity, etc..
>
> That's not a good example. reset-berlin needs a bit to be set to trigger
> the reset pulse, and doesn't have support for manual assert/deassert.
> Also according to the driver it doesn't signal reset status, so there is
> a fixed delay needed.
> Honestly, I expect most drivers in the near future to fall into two
> categories: either they need special attention, or they are not a good
> fit for this binding because all the resets are (at least mostly)
> contiguous.
>

I'm thinking the same, but at least for the devices I'm working with,
this could still be a useful thing to have.

Thanks,
Andrew

> regards
> Philipp
>

Powered by blists - more mailing lists