Re: [PATCH v4] reset: add support for non-DT systems

[David Lechner]

On 02/20/2018 04:39 AM, Philipp Zabel wrote:
> Hi Bartosz, David,
>
> On Mon, 2018-02-19 at 18:21 -0600, David Lechner wrote:
> > On 02/19/2018 10:58 AM, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
> > >
> > > The reset framework only supports device-tree. There are some platforms
> > > however, which need to use it even in legacy, board-file based mode.
> > >
> > > An example of such architecture is the DaVinci family of SoCs which
> > > supports both device tree and legacy boot modes and we don't want to
> > > introduce any regressions.
> > >
> > > We're currently working on converting the platform from its hand-crafted
> > > clock API to using the common clock framework. Part of the overhaul will
> > > be representing the chip's power sleep controller's reset lines using
> > > the reset framework.
> > >
> > > This changeset extends the core reset code with new reset lookup
> > > structures. Each lookup table contains a set of lookup entries which
> > > allow the reset core to associate reset lines with devices (by
> > > comparing the dev_id and con_id strings).
> > >
> > > Machine code can register a set of reset lines using this lookup table
> > > and concerned devices can access them using the regular reset_control
> > > API.
> > >
> > > The new lookup function is only called as a fallback in case the
> > > of_node field is NULL and doesn't change anything for current users.
> > >
> > > Tested with a dummy reset driver with several lookup entries.
> > >
> > > An example lookup table can be found below:
> > >
> > > static struct platform_device foobar_reset_dev = {
> > >           .name = "foobar-reset",
> > > };
> > >
> > > static struct reset_lookup_entry foobar_reset_lookup_entries[] = {
> > >           { .con_id = "foo",    id = 15 },
> > >           { .con_id = "bar",    id = 5  },
> > > };
> > >
> > > static struct reset_lookup_table foobar_reset_lookup_table = {
> > >           .dev_id = "foobar-device",
> > >           .entries = foobar_reset_lookup_entries,
> > >           .num_entries = ARRAY_SIZE(foobar_reset_lookup_entries),
> > >           .dev = &foobar_reset_dev.dev,
> > > };
> >
> > This seems like a lot of boilerplate to register a lookup.
>
> This could be shortened a bit by following the gpiod lookup model,
> adding a RESET_LOOKUP macro and making the array NULL terminated:
>
> #define RESET_LOOKUP(reset_dev_id, idx, con_id) /*...*/
>
> static struct reset_lookup_table foobar_reset_lookup_table = {
> 	.dev_id = "foobar-device",
> 	.entries = {
> 		RESET_LOOKUP("foobar-reset.0", 15, "foo"),
> 		RESET_LOOKUP("foobar-reset.0", 5, "bar"),
> 		{ },
> 	},
> };
>
> 	/*...*/
> 	reset_add_lookup_table(&foobar_reset_lookup_table);
>
> >   Can we have
> > something like phy_create_lookup() instead where there is just a single
> > function call to register a single lookup? This will be much easier to
> > use in the davinci PSC driver.
> >
> > void reset_add_lookup(struct reset_controller_dev *rdev, int index,
> > 		      const char *dev_id, const char *con_id);
>
> In your case the platform code that adds the lookup may be identical to
> the code that registers the struct reset_controller_dev, but that
> doesn't have to be the case. I'm not sure how that is supposed to work
> for the phy framework (I see no platform code adding phy lookups, only
> drivers).
>
> My point was that if the reset controller is registered by a separate
> driver, the platform code may not have access to the struct
> reset_controller_dev, or even the struct platform_device. I like that
> the gpiod lookups can match by dev_id of the gpio chip.
>
> regards
> Philipp

In our use case, we would be adding the lookup in the driver rather than
in the platform code, which is why I am suggesting doing it like the phy
framework.