[PATCH v2 1/3] reset: Add shared reset_control_[de]assert variants
Hans de Goede
hdegoede at redhat.com
Fri Dec 11 10:21:24 PST 2015
Hi,
On 11-12-15 18:10, Philipp Zabel wrote:
> Hi Hans,
>
> thanks for moving this forward.
Thanks for the quick review, I've a couple of (simple) questions
about your review remarks once those are cleared up I'll post a new version
(of just this patch).
> Am Freitag, den 11.12.2015, 16:41 +0100 schrieb Hans de Goede:
>> Add reset_control_deassert_shared / reset_control_assert_shared
>> functions which are intended for use by drivers for hw blocks which
>> (may) share a reset line with another driver / hw block.
>>
>> Unlike the regular reset_control_[de]assert functions these functions
>> keep track of how often deassert_shared / assert_shared have been called
>> and keep the line deasserted as long as deassert has been called more
>> times than assert.
>>
>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>> ---
>> Changes in v2:
>> -This is a new patch in v2 of this patch-set
>> ---
>> drivers/reset/core.c | 121 ++++++++++++++++++++++++++++++++++++---
>> include/linux/reset-controller.h | 2 +
>> include/linux/reset.h | 2 +
>> 3 files changed, 116 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
>> index 9ab9290..8c3436c 100644
>> --- a/drivers/reset/core.c
>> +++ b/drivers/reset/core.c
>> @@ -22,16 +22,29 @@ static DEFINE_MUTEX(reset_controller_list_mutex);
>> static LIST_HEAD(reset_controller_list);
>>
>> /**
>> + * struct reset_line - a reset line
>> + * @list: list entry for the reset controllers reset line list
>> + * @id: ID of the reset line in the reset controller device
>> + * @refcnt: Number of reset_control structs referencing this device
>> + * @deassert_cnt: Number of times this reset line has been deasserted
>> + */
>> +struct reset_line {
>> + struct list_head list;
>> + unsigned int id;
>> + unsigned int refcnt;
>> + unsigned int deassert_cnt;
>> +};
>
> I'd move rcdev into reset_line, too. That way the description is
> complete, and we don't duplicate rcdev when there are multiple
> reset_controls pointing here.
Ack.
>> +/**
>> * struct reset_control - a reset control
>> * @rcdev: a pointer to the reset controller device
>> * this reset control belongs to
>> - * @id: ID of the reset controller in the reset
>> - * controller device
>> + * @line: reset line for this reset control
>> */
>> struct reset_control {
>> struct reset_controller_dev *rcdev;
>> + struct reset_line *line;
>> struct device *dev;
>> - unsigned int id;
>> };
>>
>> /**
> [...]
>> @@ -119,13 +134,55 @@ EXPORT_SYMBOL_GPL(reset_control_assert);
>> int reset_control_deassert(struct reset_control *rstc)
>> {
>
> Maybe WARN_ON(rstc->line->refcnt > 1) ?
I assume you mean deasser_cnt ? Seems reasonable with that change.
>> if (rstc->rcdev->ops->deassert)
>> - return rstc->rcdev->ops->deassert(rstc->rcdev, rstc->id);
>> + return rstc->rcdev->ops->deassert(rstc->rcdev, rstc->line->id);
>>
>> return -ENOTSUPP;
>> }
>> EXPORT_SYMBOL_GPL(reset_control_deassert);
>>
>> /**
>> + * reset_control_assert_shared - asserts a shared reset line
>> + * @rstc: reset controller
>> + *
>> + * Assert a shared reset line, this functions decreases the deassert count
>> + * of the line by one and asserts it if, and only if, the deassert count
>> + * reaches 0.
>
> "After calling this function the shared reset line may be asserted, or
> it may still be deasserted, as long as other users keep it so."
I take it this is to replace the text about "deassert count" ?
>> + */
>> +int reset_control_assert_shared(struct reset_control *rstc)
>> +{
>> + if (!rstc->rcdev->ops->assert)
>> + return -ENOTSUPP;
>
> WARN_ON(rstc->line->deassert_cnt == 0)
>
> Actually, what to do in this case? Assume ops->assert was called, or do
> it again to be sure? Certainly we don't want to wrap deassert_cnt, or
> the next deassert_shared will do nothing.
>
>> + rstc->line->deassert_cnt--;
>> + if (rstc->line->deassert_cnt)
>
> deassert_cnt isn't protected by any lock.
Right, good catch. I believe the best way to fix this is to change deassert_cnt
into an atomic_t and use atomic_dec_return / atomic_int_return,
Downside of using an atomic_t is that doing the WARN_ON you are asking for above
will not be race free, then, but since it is a should never happen scenario I
guess we do not care about the check not being 100% race free. Or we can even
just leave out the check ?
>
>> + return 0;
>> +
>> + return rstc->rcdev->ops->assert(rstc->rcdev, rstc->line->id);
>> +}
>> +EXPORT_SYMBOL_GPL(reset_control_assert_shared);
>> +
>> +/**
>> + * reset_control_deassert_shared - deasserts a shared reset line
>> + * @rstc: reset controller
>> + *
>> + * Assert a shared reset line, this functions increases the deassert count
>
> Deassert
Ack.
>> + * of the line by one and deasserts the reset line (if it was not already
>> + * deasserted).
>
> "After calling this function, the shared reset line is guaranteed to be
> deasserted."
Same question as above.
>> + */
>> +int reset_control_deassert_shared(struct reset_control *rstc)
>> +{
>> + if (!rstc->rcdev->ops->deassert)
>> + return -ENOTSUPP;
>> +
>> + rstc->line->deassert_cnt++;
>> + if (rstc->line->deassert_cnt != 1)
>> + return 0;
>> +
>> + return rstc->rcdev->ops->deassert(rstc->rcdev, rstc->line->id);
>> +}
>> +EXPORT_SYMBOL_GPL(reset_control_deassert_shared);
>> +
>> +/**
>> * 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.
>> @@ -134,12 +191,47 @@ EXPORT_SYMBOL_GPL(reset_control_deassert);
>> int reset_control_status(struct reset_control *rstc)
>> {
>> if (rstc->rcdev->ops->status)
>> - return rstc->rcdev->ops->status(rstc->rcdev, rstc->id);
>> + return rstc->rcdev->ops->status(rstc->rcdev, rstc->line->id);
>>
>> return -ENOTSUPP;
>> }
>> EXPORT_SYMBOL_GPL(reset_control_status);
>>
>> +static struct reset_line *reset_line_get(struct reset_controller_dev *rcdev,
>> + unsigned int index)
>> +{
>> + struct reset_line *line;
>
> lockdep_assert_held here and in reset_line_put would document that these
> functions are called under reset_(controller_)list_mutex.
Ok, I will add these.
>> + list_for_each_entry(line, &rcdev->reset_line_head, list) {
>> + if (line->id == index) {
>> + line->refcnt++;
>> + return line;
>> + }
>> + }
>> +
>> + line = kzalloc(sizeof(*line), GFP_KERNEL);
>> + if (!line)
>> + return NULL;
>> +
>> + list_add(&line->list, &rcdev->reset_line_head);
>> + line->id = index;
>> + line->refcnt = 1;
>> +
>> + return line;
>> +}
>> +
>> +static void reset_line_put(struct reset_line *line)
>> +{
>> + if (!line)
>> + return;
>> +
>> + if (--line->refcnt)
>> + return;
>> +
>> + list_del(&line->list);
>> + kfree(line);
>> +}
>> +
>> /**
>> * of_reset_control_get_by_index - Lookup and obtain a reference to a reset
>> * controller by index.
>> @@ -155,6 +247,7 @@ struct reset_control *of_reset_control_get_by_index(struct device_node *node,
>> {
>> struct reset_control *rstc = ERR_PTR(-EPROBE_DEFER);
>> struct reset_controller_dev *r, *rcdev;
>> + struct reset_line *line;
>> struct of_phandle_args args;
>> int rstc_id;
>> int ret;
>> @@ -186,16 +279,22 @@ struct reset_control *of_reset_control_get_by_index(struct device_node *node,
>> }
>>
>> try_module_get(rcdev->owner);
>> +
>> + /* reset_controller_list_mutex also protects the reset_line list */
>
> Let's rename it to reset_list_mutex, then.
Ack.
>> + line = reset_line_get(rcdev, rstc_id);
>> +
>> mutex_unlock(&reset_controller_list_mutex);
>>
>> rstc = kzalloc(sizeof(*rstc), GFP_KERNEL);
>> - if (!rstc) {
>> + if (!line || !rstc) {
>> + kfree(rstc);
>> + reset_line_put(line);
>> module_put(rcdev->owner);
>> return ERR_PTR(-ENOMEM);
>> }
>>
>> rstc->rcdev = rcdev;
>> - rstc->id = rstc_id;
>> + rstc->line = line;
>>
>> return rstc;
>> }
>> @@ -259,6 +358,10 @@ void reset_control_put(struct reset_control *rstc)
>> if (IS_ERR(rstc))
>> return;
>>
>> + mutex_lock(&reset_controller_list_mutex);
>> + reset_line_put(rstc->line);
>> + mutex_unlock(&reset_controller_list_mutex);
>> +
>> module_put(rstc->rcdev->owner);
>> kfree(rstc);
>> }
>> diff --git a/include/linux/reset-controller.h b/include/linux/reset-controller.h
>> index ce6b962..7f2cbd1 100644
>> --- a/include/linux/reset-controller.h
>> +++ b/include/linux/reset-controller.h
>> @@ -31,6 +31,7 @@ struct of_phandle_args;
>> * @ops: a pointer to device specific struct reset_control_ops
>> * @owner: kernel module of the reset controller driver
>> * @list: internal list of reset controller devices
>> + * @reset_line_head: head of internal list of reset lines
>
> "list of requested reset lines" or "list of reset lines in use" to make
> clear the list only contains entries for the requested controls?
Ok, will fix.
>> * @of_node: corresponding device tree node as phandle target
>> * @of_reset_n_cells: number of cells in reset line specifiers
>> * @of_xlate: translation function to translate from specifier as found in the
> [...]
>
> best regards
> Philipp
>
Regards,
Hans
More information about the linux-arm-kernel
mailing list