Hi Tero,
On Fri, 2019-08-30 at 10:28 +0300, Tero Kristo wrote:
> On 29/08/2019 16:12, Philipp Zabel wrote:
[...]
> > I wonder if splitting rstctrl/rstst/rstmap out into a separate structure
> > would make sense. That could be linked from omap_reset_data directly.
> > That only makes sense if there'd be enough cases where it can be reused
> > for multiple PRMs instances.
>
> Hmm, splitting these out would make it possible to share the bits for
> ipu:s across devices, same for dsp:s and eve:s.
>
> However, adding too many levels of abstraction makes it kind of
> difficult to follow what is happening with the code, and it would only
> save maybe ~100 bytes of memory at the moment.
Good point, that is likely not worth the additional complexity.
[...]
> > What does the value read from the rstst register indicate? Is it the
> > current state of the reset line? Is it 0 until deassertion is completed,
> > and then it turns to 1?
>
> Value of 1 indicates that the corresponding IP has been reset
> successfully. Writing back 1 to the same bit clears it out, so the
> status can be polled later on.
Then this should not be returned directly by reset_control_status:
/**
* reset_control_status - returns a negative errno if not supported, a
* positive value if the reset line is asserted, or zero if the reset
* line is not asserted or if the desc is NULL (optional reset).
*/
This should return 0 only if the control bit is deasserted and the
status bit is already set.
If either the control bit is asserted, or if the status bit is still
cleared, this should return 1.
[...]
> > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > + if (!res)
> > > + return -ENODEV;
> >
> > This can be merged with devm_ioremap_resource below.
>
> Well, I actually use the "res" later on to map the DT node to the
> corresponding prm_data based on address.
Ok. I glanced over the data->base loop below after questioning whether
it is needed at all.
> >
> > > + match = of_match_device(omap_prm_id_table, &pdev->dev);
> > > + if (!match)
> > > + return -ENOTSUPP;
> > > +
> > > + prm = devm_kzalloc(&pdev->dev, sizeof(*prm), GFP_KERNEL);
> > > + if (!prm)
> > > + return -ENOMEM;
> > > +
> > > + data = match->data;
> > > +
> > > + while (data->base != res->start) {
> > > + if (!data->base)
> > > + return -EINVAL;
> > > + data++;
> > > + }
> >
> > Is this not something that you want to have encoded in the compatible
> > string? They all have a different register layout.
>
> With the addition of all the prm instances later on, this changes. Most
> of the prm instances will have same register layout then. See v1 data
> that was posted earlier [1], but which I dropped for now to keep this
> series isolated for reset handling only. In this patch, you see that for
> DRA7, all the power domain handling related PRM instances have identical
> register layout, they just differ based on base address.
>
> [1] https://www.spinics.net/lists/linux-omap/msg149872.html
>
> It would be possible to encode all of this based on different
> compatibles, but then the amount of different compatible strings would
> explode... DRA7 is just one SoC.
I see only 3 different layouts in that patch. All instances with
identical layout could share the same compatible.
Either way, that is DT bindings territory, and not for me to decide. I
just found it unusual to encode the device type by its base address in
the driver.
> >
> > > +
> > > + prm->data = data;
> > > +
> > > + prm->base = devm_ioremap_resource(&pdev->dev, res);
> >
> > prm->base = devm_platform_ioremap_resource(pdev, 0);
>
> I still need the "res" pointer as indicated above.
Got it. If the lookup by base address is needed, this has to stay split.
regards
Philipp
