Re: [PATCH 1/2] Documentation: dt: reset: Add syscon reset binding

[Andrew F. Davis]

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