Re: [PATCH 2/8] soc: ti: add initial PRM driver with reset control support

[Philipp Zabel <p.zabel@pengutronix.de>]

On Tue, 2019-08-20 at 11:47 -0500, Suman Anna wrote:
> On 8/20/19 2:37 AM, Tero Kristo wrote:
> > On 20.8.2019 2.01, Suman Anna wrote:
> > > Hi Tero,
> > > 
> > > On 8/19/19 4:32 AM, Tero Kristo wrote:
[...]
> > > > > > +{
> > > > > > +    struct omap_reset_data *reset;
> > > > > > +
> > > > > > +    /*
> > > > > > +     * Check if we have resets. If either rstctl or rstst is
> > > > > > +     * non-zero, we have reset registers in place. Additionally
> > > > > > +     * the flag OMAP_PRM_NO_RSTST implies that we have resets.
> > > > > > +     */
> > > > > > +    if (!prm->data->rstctl && !prm->data->rstst &&
> > > > > > +        !(prm->data->flags & OMAP_PRM_NO_RSTST))
> > > > > > +        return 0;
> > > > > > +
> > > > > > +    reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL);
> > > > > > +    if (!reset)
> > > > > > +        return -ENOMEM;
> > > > > > +
> > > > > > +    reset->rcdev.owner = THIS_MODULE;
> > > > > > +    reset->rcdev.ops = &omap_reset_ops;
> > > > > > +    reset->rcdev.of_node = pdev->dev.of_node;
> > > > > > +    reset->rcdev.nr_resets = OMAP_MAX_RESETS;
> > > 
> > > Suggest adding a number of resets to prm->data, and using it so that we
> > > don't even entertain any resets beyond the actual number of resets.
> > 
> > Hmm why bother? Accessing a stale reset bit will just cause access to a
> > reserved bit in the reset register, doing basically nothing. Also, this
> > would not work for am3/am4 wkup, as there is a single reset bit at an
> > arbitrary position.
> 
> The generic convention seems to be defining a reset id value defined
> from include/dt-bindings/reset/ that can be used to match between the
> dt-nodes and the reset-controller driver.
> 
> Philipp,
> Any comments?

Are there only reset bits and reserved bits in the range accessible by
[0..OMAP_MAX_RESETS] or are ther bits with another function as well?
If the latter is the case, I would prefer enumerating the resets in a
dt-bindings header, with the driver containing an enum -> reg/bit
position lookup table.

In general, assuming the device tree contains no errors, this should not
matter much, but I think it is nice if the reset driver, even with a
misconfigured device tree, can't write into arbitrary bit fields.

regards
Philipp

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel