Message-ID: <577144E1.1010100@ti.com>
Date:	Mon, 27 Jun 2016 10:23:13 -0500
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 v4 2/2] reset: add TI SYSCON based reset driver

On 06/23/2016 11:28 AM, Philipp Zabel wrote:
> Am Donnerstag, den 23.06.2016, 09:28 -0500 schrieb Andrew F. Davis:
>> On 06/23/2016 04:05 AM, Philipp Zabel wrote:
>>> Am Mittwoch, den 22.06.2016, 14:46 -0500 schrieb Andrew F. Davis:
>>> [...]
>>>>>> +	depends on HAS_IOMEM
>>>>>> +	select MFD_SYSCON
>>>>>> +	help
>>>>>> +	  This enables the reset driver support for TI devices with
>>>>>> +	  memory-mapped reset registers as part of a syscon device node. If
>>>>>> +	  you wish to use the reset framework for such memory-mapped devices,
>>>>>> +	  say Y here. Otherwise, say N.
>>>>>
>>>>> Actually, do you need the user configurable option at all?
>>>>
>>>> I'm not sure, right now it is selected by other things, but that is true
>>>> for much of Kconfig, it is not platform dependent so it doesn't need to
>>>> only be enabled by arch, it probably isn't hurting anything to leave it?
>>>
>>> No, that's okay. So the intention is to make it possible to enable it
>>> for COMPILE_TESTs on architectures other than TI?
>>
>> I was thinking more that it should be usable on non-TI architectures and
>> so can be user enabled if needed.
> 
> Those architectures could also just select it, then. Having the option
> might improve discoverability though.
> 
> [...]
>>> So far, I have seen the following variants. Depending on the hardware, a
>>> reset could be:
>>> - asserted by setting a bit
>>> - asserted by clearing a bit
>>> - deasserted by clearing/setting the same bit
>>> - deasserted by setting/clearing the same bit in another register
>>> - triggered to be asserted/deasserted automatically with some specific
>>>   timing that the hardware knows about (in that case the manual
>>>   assert/deassert is not available)
>>> The status of the reset line could be read via
>>> - the same bit that is used to assert/deassert
>>> - the same bit in another register
>>> - not at all
>>>
>>> What I've not yet seen but surely exists somewhere is the case where
>>> assert/deassert/status bits are at different bit positions either in the
>>> same register or in different registers.
>>
>> Exactly why I was thinking having a node for the resets would be more
>> future proof, we could add more properties later:
>>
>> pscrst-dsp: dsp {
>> 	reset-control = <0xa3c 8 RESET_ASSERT_CLEAR>;
>> 	reset-status = <0x83c 8 RESET_ASSERT_CLEAR>;
>> +	reset-deassert = <0x840 3 RESET_ASSERT_SET>;
>> +	reset-toggle-time-ms = <20>;
>> };
>>
>> While a fixed length array does make for a more condensed binding we
>> don't get much flexibility.
>>
>> What we could also do is have a longer array then use macros to trim it
>> down for simple cases:
>>
>> reset-bits = <
>> 	SINGLE_BIT_RESET(0xa3c, 8)
>> 	SINGLE_BIT_RESET(0xa40, 7)
>> 	SINGLE_BIT_RESET(0xa44, 6)
>>> ;
> 
> I think there has been opposition in the past against hiding things more
> complex than a single value behind macros.
> 
>> Each real entry could have 9 fields that the macro would expand into:
>> 	(assert reg) (assert bit) (assert flag)
>> 	(deassert reg) (deassert bit) (deassert flag)
>> 	(status reg) (status bit) (status flag)
>>
>> This would be able to handle all of the above cases except the timing
>> one, that case can always be handled by a dedicated driver for that system.
> 
> How about seven:
> 	(assert reg) (assert bit)
> 	(deassert reg) (deassert bit)
>  	(status reg) (status bit)
>         (flags)
> 
> reset-bits = <
> 	0xa3c 8 0xa3c 8 0x83c 8 (ASSERT_CLEAR|DEASSERT_SET|STATUS_SET)
>> ;
> 

I like this, we could also add a none flag:

STATUS_SET
STATUS_CLEAR
STATUS_NONE

to note that this reset doesn't support this and that the numbers given
are not valid.

>> My goal here, I would like to believe, aligns with the goals of DT in
>> general, I would like to see one kernel work on many platforms without
>> having to compile-in a bunch of SoC specific info. Some SoCs will need
>> their own reset driver for when they do something unique (like this
>> reset driver I will post next cycle which sends a message to a power
>> management chip for its resets:
>> http://git.ti.com/gitweb/?p=ti-linux-kernel/ti-linux-kernel.git;a=blob;f=drivers/reset/reset-ti-sci.c;h=3636dc176c8b5c4449eefc1fe13df7b912cec73e;hb=refs/heads/ti-lsk-linux-4.4.y
>> )
>> but I see no reason a bunch of different drivers for simple register bit
>> resets with the only difference being a #define offset. I hope that this
>> effort will get ahead of these drivers and reduce the maintenance burden
>> for you.
> 
> The problem of reducing the amount of simple drivers is completely
> orthogonal to agreeing on a fitting DT binding. A common driver could
> just as well have static syscon_reset_control arrays per compatible
> compiled in.
> 
>> So how much is handled by this driver is up to you, (hopefully without
>> trying to handle every possible case :)).
> 
> It's also up to the DT maintainers to approve the bindings.
> 

Looks like it got an ACK, almost makes me not want to touch it now :/

> I have no delusions that we have to find a compromise between
> driver/binding complexity and the number of supported common cases.
> The reason I find it difficult to make a decision is I don't feel like I
> have enough data.
> Should we support separate status reg? Obviously, you need it. Separate
> deassert reg? Questionable. Those devices usually have set/clear
> registers dedicated to resets and as such are not a good fit for this
> driver anyway. assert/deassert/status bits at different positions? Maybe
> not even needed. Support for triggered, self-clearing resets? Maybe
> better handled by a different driver, too.
> 

Well we can look at existing drivers, a quick looks seems to indicate
the above scheme can handle the reset types currently handled by:

hi6220_reset.c
reset-ath79.c
reset-meson.c
reset-pistachio.c
reset-socfpga.c
reset-sunxi.c
reset-zynq.c

this is most of the current single bit reset drivers, the only ones that
cannot be handled are ones with timing constraints and ones that need
reset hardware setup. So I would say this is a good base set for
covering many simple reset controllers.

I'll push v5 with fixes from these comments and the DT changes here shortly.

Thanks,
Andrew

Powered by blists - more mailing lists