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

[David Lechner <david@lechnology.com>]

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@baylibre.com>
>>>
>>> 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.