On 30/08/2019 12:02, Philipp Zabel wrote:
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.
Hmm ok, let me fix that a bit also.
[...]
+ 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.
I'll try to poke Rob for comment on that on the bindings patch.
+
+ 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.
Ok thanks for the reviews, much appreciated. I'll see if I can get reply
from Rob on the binding item, otherwise I might just send v2 with the
existing bindings out and send a v3 with updated bindings if necessary.
-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
