On 05.04.2022 18:15, Philipp Zabel wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Hi Claudio,
>
> On Di, 2022-04-05 at 14:47 +0000, Claudiu.Beznea@xxxxxxxxxxxxx wrote:
>> Hi, Philipp,
>>
>> On 05.04.2022 14:47, Philipp Zabel wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>> know the content is safe
>>>
>>> Hi Claudiu,
>>>
>>> On Di, 2022-04-05 at 14:27 +0300, Claudiu Beznea wrote:
>>>> SAMA7G5 reset controller has 5 extra lines that goes to different
>>>> devices
>>>> (3 lines to USB PHYs, 1 line to DDR controller, one line DDR PHY
>>>> controller). These reset lines could be requested by different
>>>> controller
>>>> drivers (e.g. USB PHY driver) and these controllers' drivers
>>>> could
>>>> assert/deassert these lines when necessary. Thus add support for
>>>> reset_controller_dev which brings this functionality.
>>>>
>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea@xxxxxxxxxxxxx>
>>>> ---
>>>> drivers/power/reset/at91-reset.c | 92
>>>> ++++++++++++++++++++++++++++++--
>>>> 1 file changed, 88 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/power/reset/at91-reset.c
>>>> b/drivers/power/reset/at91-reset.c
>>>> index 0d721e27f545..b04df54c15d2 100644
>>>> --- a/drivers/power/reset/at91-reset.c
>>>> +++ b/drivers/power/reset/at91-reset.c
>>>> @@ -17,6 +17,7 @@
>>>> #include <linux/of_address.h>
>>>> #include <linux/platform_device.h>
>>>> #include <linux/reboot.h>
>>>> +#include <linux/reset-controller.h>
>>>>
>>>> #include <soc/at91/at91sam9_ddrsdr.h>
>>>> #include <soc/at91/at91sam9_sdramc.h>
>>>> @@ -53,12 +54,16 @@ enum reset_type {
>>>> struct at91_reset {
>>>> void __iomem *rstc_base;
>>>> void __iomem *ramc_base[2];
>>>> + void __iomem *dev_base;
>>>> + struct reset_controller_dev rcdev;
>>>> struct clk *sclk;
>>>> struct notifier_block nb;
>>>> u32 args;
>>>> u32 ramc_lpr;
>>>> };
>>>>
>>>> +#define to_at91_reset(r) container_of(r, struct
>>>> at91_reset, rcdev)
>>>> +
>>>> struct at91_reset_data {
>>>> u32 reset_args;
>>>> u32 n_device_reset;
>>>> @@ -191,6 +196,79 @@ static const struct of_device_id
>>>> at91_reset_of_match[] = {
>>>> };
>>>> MODULE_DEVICE_TABLE(of, at91_reset_of_match);
>>>>
>>>> +static int at91_reset_update(struct reset_controller_dev *rcdev,
>>>> + unsigned long id, bool assert)
>>>> +{
>>>> + struct at91_reset *reset = to_at91_reset(rcdev);
>>>> + u32 val;
>>>> +
>>>> + val = readl_relaxed(reset->dev_base);
>>>> + if (assert)
>>>> + val |= BIT(id);
>>>> + else
>>>> + val &= ~BIT(id);
>>>> + writel_relaxed(val, reset->dev_base);
>>>
>>> This read-modify-update should be protected by a spinlock.
>>>
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int at91_reset_assert(struct reset_controller_dev *rcdev,
>>>> + unsigned long id)
>>>> +{
>>>> + return at91_reset_update(rcdev, id, true);
>>>> +}
>>>> +
>>>> +static int at91_reset_deassert(struct reset_controller_dev
>>>> *rcdev,
>>>> + unsigned long id)
>>>> +{
>>>> + return at91_reset_update(rcdev, id, false);
>>>> +}
>>>> +
>>>> +static int at91_reset_dev_status(struct reset_controller_dev
>>>> *rcdev,
>>>> + unsigned long id)
>>>> +{
>>>> + struct at91_reset *reset = to_at91_reset(rcdev);
>>>> + u32 val;
>>>> +
>>>> + val = readl_relaxed(reset->dev_base);
>>>> +
>>>> + return !!(val & BIT(id));
>>>> +}
>>>> +
>>>> +static const struct reset_control_ops at91_reset_ops = {
>>>> + .assert = at91_reset_assert,
>>>> + .deassert = at91_reset_deassert,
>>>> + .status = at91_reset_dev_status,
>>>> +};
>>>> +
>>>> +static int at91_reset_of_xlate(struct reset_controller_dev
>>>> *rcdev,
>>>> + const struct of_phandle_args
>>>> *reset_spec)
>>>> +{
>>>> + return reset_spec->args[0];
>>>> +}
>>>
>>> For 1:1 mappings there is no need for a custom of_xlate handler.
>>> Just
>>> leave of_xlate and of_reset_n_cells empty.
>>
>> I've double checked that. This would work if reset ids are continuous
>> from
>> zero to rcdev.nr_resets. This the of_reset_simple_xlate:
>>
>> static int of_reset_simple_xlate(struct reset_controller_dev *rcdev,
>> const struct of_phandle_args
>> *reset_spec)
>> {
>> if (reset_spec->args[0] >= rcdev->nr_resets)
>> return -EINVAL;
>> return reset_spec->args[0];
>> }
>>
>> But in this driver's case we have 3 ids: 4, 5, 6. That is the reason
>> I had this simple xlate function.
>
> I see. In that case I'd say keep the custom of_xlate but let it return
> -EINVAL if the args[0] value is not 4, 5, or 6.
I will go for this approach (I though of it initially but let aside after)
to also protect the other 2 resets (DDR controller and DDR PHY controller)
which are at bits 0 and 2 in register at rstc->dev_base.
Thank you,
Claudiu Beznea
>
> Or you could set nr_resets to 7, but unless there are more resets at
> the lower bits, that wouldn't necessarily be better.
>
> regards
> Philipp
