diff mbox

[v3,4/6] bus: ti-sysc: Add support for software reset

Message ID 20180606060826.14671-5-faiz_abbas@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Faiz Abbas June 6, 2018, 6:08 a.m. UTC
Add support for the software reset of a target interconnect
module using its sysconfig and sysstatus registers.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 drivers/bus/ti-sysc.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

Comments

Tony Lindgren June 7, 2018, 7:35 a.m. UTC | #1
* Faiz Abbas <faiz_abbas@ti.com> [180606 06:14]:
> +static int sysc_reset(struct sysc *ddata)
> +{
> +	int offset = ddata->offsets[SYSC_SYSCONFIG];
> +	int val = sysc_read(ddata, offset);
> +
> +	val |= (0x1 << ddata->cap->regbits->srst_shift);
> +	sysc_write(ddata, offset, val);
> +
> +	/* Poll on reset status */
> +	if (ddata->cfg.quirks & SYSC_QUIRK_RESET_STATUS) {
> +		offset = ddata->offsets[SYSC_SYSSTATUS];
> +
> +		return readl_poll_timeout(ddata->module_va + offset, val,
> +				(val & ddata->cfg.syss_mask) == 0x0,
> +				100, MAX_MODULE_SOFTRESET_WAIT);
> +	}
> +
> +	return 0;
> +}

I wonder if we should also add SYSS_QUIRK_RESET_STATUS in
addition to SYSC_QUIRK_RESET status to make it easy to
read the right register?

Then we can add support for SYSC_QUIRK_RESET_STATUS later on
when tested and return error for now.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Faiz Abbas June 7, 2018, 10:24 a.m. UTC | #2
Hi,

On Thursday 07 June 2018 01:05 PM, Tony Lindgren wrote:
> * Faiz Abbas <faiz_abbas@ti.com> [180606 06:14]:
>> +static int sysc_reset(struct sysc *ddata)
>> +{
>> +	int offset = ddata->offsets[SYSC_SYSCONFIG];
>> +	int val = sysc_read(ddata, offset);
>> +
>> +	val |= (0x1 << ddata->cap->regbits->srst_shift);
>> +	sysc_write(ddata, offset, val);
>> +
>> +	/* Poll on reset status */
>> +	if (ddata->cfg.quirks & SYSC_QUIRK_RESET_STATUS) {
>> +		offset = ddata->offsets[SYSC_SYSSTATUS];
>> +
>> +		return readl_poll_timeout(ddata->module_va + offset, val,
>> +				(val & ddata->cfg.syss_mask) == 0x0,
>> +				100, MAX_MODULE_SOFTRESET_WAIT);
>> +	}
>> +
>> +	return 0;
>> +}
> 
> I wonder if we should also add SYSS_QUIRK_RESET_STATUS in
> addition to SYSC_QUIRK_RESET status to make it easy to
> read the right register?

I assumed SYSC_QUIRK is the prefix to indicate the ti-sysc driver not
the register. Are there layouts in which the reset status bit is in the
sysconfig register rather than the sysstatus register?

Thanks,
Faiz
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tony Lindgren June 8, 2018, 6:21 a.m. UTC | #3
* Faiz Abbas <faiz_abbas@ti.com> [180607 10:24]:
> Hi,
> 
> On Thursday 07 June 2018 01:05 PM, Tony Lindgren wrote:
> > * Faiz Abbas <faiz_abbas@ti.com> [180606 06:14]:
> >> +static int sysc_reset(struct sysc *ddata)
> >> +{
> >> +	int offset = ddata->offsets[SYSC_SYSCONFIG];
> >> +	int val = sysc_read(ddata, offset);
> >> +
> >> +	val |= (0x1 << ddata->cap->regbits->srst_shift);
> >> +	sysc_write(ddata, offset, val);
> >> +
> >> +	/* Poll on reset status */
> >> +	if (ddata->cfg.quirks & SYSC_QUIRK_RESET_STATUS) {
> >> +		offset = ddata->offsets[SYSC_SYSSTATUS];
> >> +
> >> +		return readl_poll_timeout(ddata->module_va + offset, val,
> >> +				(val & ddata->cfg.syss_mask) == 0x0,
> >> +				100, MAX_MODULE_SOFTRESET_WAIT);
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> > 
> > I wonder if we should also add SYSS_QUIRK_RESET_STATUS in
> > addition to SYSC_QUIRK_RESET status to make it easy to
> > read the right register?
> 
> I assumed SYSC_QUIRK is the prefix to indicate the ti-sysc driver not
> the register. Are there layouts in which the reset status bit is in the
> sysconfig register rather than the sysstatus register?

Yes we can have reset status bit in either syss or syssconfig register.
We detect that in sysc_init_syss_mask() but we should also set something
at that point to make it clear which reset to use. But as we don't need
the quirk flag, it's probably set a function pointer after the detection.
So how about let's have two functions sysc_reset() and sysc_syss_reset()
and then we can implement sysc_syss_reset() in a separate patch after
testing it when we have a non-platform data using example for
sysc_syss_reset().

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Faiz Abbas June 11, 2018, 6:08 a.m. UTC | #4
Hi Tony,

On Friday 08 June 2018 11:51 AM, Tony Lindgren wrote:
> * Faiz Abbas <faiz_abbas@ti.com> [180607 10:24]:
>> Hi,
>>
>> On Thursday 07 June 2018 01:05 PM, Tony Lindgren wrote:
>>> * Faiz Abbas <faiz_abbas@ti.com> [180606 06:14]:
>>>> +static int sysc_reset(struct sysc *ddata)
>>>> +{
>>>> +	int offset = ddata->offsets[SYSC_SYSCONFIG];
>>>> +	int val = sysc_read(ddata, offset);
>>>> +
>>>> +	val |= (0x1 << ddata->cap->regbits->srst_shift);
>>>> +	sysc_write(ddata, offset, val);
>>>> +
>>>> +	/* Poll on reset status */
>>>> +	if (ddata->cfg.quirks & SYSC_QUIRK_RESET_STATUS) {
>>>> +		offset = ddata->offsets[SYSC_SYSSTATUS];
>>>> +
>>>> +		return readl_poll_timeout(ddata->module_va + offset, val,
>>>> +				(val & ddata->cfg.syss_mask) == 0x0,
>>>> +				100, MAX_MODULE_SOFTRESET_WAIT);
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>
>>> I wonder if we should also add SYSS_QUIRK_RESET_STATUS in
>>> addition to SYSC_QUIRK_RESET status to make it easy to
>>> read the right register?
>>
>> I assumed SYSC_QUIRK is the prefix to indicate the ti-sysc driver not
>> the register. Are there layouts in which the reset status bit is in the
>> sysconfig register rather than the sysstatus register?
> 
> Yes we can have reset status bit in either syss or syssconfig register.

You mean sysstatus and sysconfig right?

> We detect that in sysc_init_syss_mask() but we should also set something
> at that point to make it clear which reset to use. But as we don't need
> the quirk flag, it's probably set a function pointer after the detection.
> So how about let's have two functions sysc_reset() and sysc_syss_reset()
> and then we can implement sysc_syss_reset() in a separate patch after
> testing it when we have a non-platform data using example for
> sysc_syss_reset().
> 

Shouldn't the function I add be called sysc_syss_reset()? The reset
status bit is in the sysstatus.

Thanks,
Faiz

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tony Lindgren June 11, 2018, 6:09 a.m. UTC | #5
* Faiz Abbas <faiz_abbas@ti.com> [180611 06:09]:
> Hi Tony,
> 
> On Friday 08 June 2018 11:51 AM, Tony Lindgren wrote:
> > * Faiz Abbas <faiz_abbas@ti.com> [180607 10:24]:
> >> Hi,
> >>
> >> On Thursday 07 June 2018 01:05 PM, Tony Lindgren wrote:
> >>> * Faiz Abbas <faiz_abbas@ti.com> [180606 06:14]:
> >>>> +static int sysc_reset(struct sysc *ddata)
> >>>> +{
> >>>> +	int offset = ddata->offsets[SYSC_SYSCONFIG];
> >>>> +	int val = sysc_read(ddata, offset);
> >>>> +
> >>>> +	val |= (0x1 << ddata->cap->regbits->srst_shift);
> >>>> +	sysc_write(ddata, offset, val);
> >>>> +
> >>>> +	/* Poll on reset status */
> >>>> +	if (ddata->cfg.quirks & SYSC_QUIRK_RESET_STATUS) {
> >>>> +		offset = ddata->offsets[SYSC_SYSSTATUS];
> >>>> +
> >>>> +		return readl_poll_timeout(ddata->module_va + offset, val,
> >>>> +				(val & ddata->cfg.syss_mask) == 0x0,
> >>>> +				100, MAX_MODULE_SOFTRESET_WAIT);
> >>>> +	}
> >>>> +
> >>>> +	return 0;
> >>>> +}
> >>>
> >>> I wonder if we should also add SYSS_QUIRK_RESET_STATUS in
> >>> addition to SYSC_QUIRK_RESET status to make it easy to
> >>> read the right register?
> >>
> >> I assumed SYSC_QUIRK is the prefix to indicate the ti-sysc driver not
> >> the register. Are there layouts in which the reset status bit is in the
> >> sysconfig register rather than the sysstatus register?
> > 
> > Yes we can have reset status bit in either syss or syssconfig register.
> 
> You mean sysstatus and sysconfig right?

Yup.

> > We detect that in sysc_init_syss_mask() but we should also set something
> > at that point to make it clear which reset to use. But as we don't need
> > the quirk flag, it's probably set a function pointer after the detection.
> > So how about let's have two functions sysc_reset() and sysc_syss_reset()
> > and then we can implement sysc_syss_reset() in a separate patch after
> > testing it when we have a non-platform data using example for
> > sysc_syss_reset().
> > 
> 
> Shouldn't the function I add be called sysc_syss_reset()? The reset
> status bit is in the sysstatus.

Yes

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Faiz Abbas June 11, 2018, 6:27 a.m. UTC | #6
On Monday 11 June 2018 11:39 AM, Tony Lindgren wrote:
> * Faiz Abbas <faiz_abbas@ti.com> [180611 06:09]:
>> Hi Tony,
>>
>> On Friday 08 June 2018 11:51 AM, Tony Lindgren wrote:
>>> * Faiz Abbas <faiz_abbas@ti.com> [180607 10:24]:
>>>> Hi,
>>>>
>>>> On Thursday 07 June 2018 01:05 PM, Tony Lindgren wrote:
>>>>> * Faiz Abbas <faiz_abbas@ti.com> [180606 06:14]:
>>>>>> +static int sysc_reset(struct sysc *ddata)
>>>>>> +{
>>>>>> +	int offset = ddata->offsets[SYSC_SYSCONFIG];
>>>>>> +	int val = sysc_read(ddata, offset);
>>>>>> +
>>>>>> +	val |= (0x1 << ddata->cap->regbits->srst_shift);
>>>>>> +	sysc_write(ddata, offset, val);
>>>>>> +
>>>>>> +	/* Poll on reset status */
>>>>>> +	if (ddata->cfg.quirks & SYSC_QUIRK_RESET_STATUS) {
>>>>>> +		offset = ddata->offsets[SYSC_SYSSTATUS];
>>>>>> +
>>>>>> +		return readl_poll_timeout(ddata->module_va + offset, val,
>>>>>> +				(val & ddata->cfg.syss_mask) == 0x0,
>>>>>> +				100, MAX_MODULE_SOFTRESET_WAIT);
>>>>>> +	}
>>>>>> +
>>>>>> +	return 0;
>>>>>> +}
>>>>>
>>>>> I wonder if we should also add SYSS_QUIRK_RESET_STATUS in
>>>>> addition to SYSC_QUIRK_RESET status to make it easy to
>>>>> read the right register?
>>>>
>>>> I assumed SYSC_QUIRK is the prefix to indicate the ti-sysc driver not
>>>> the register. Are there layouts in which the reset status bit is in the
>>>> sysconfig register rather than the sysstatus register?
>>>
>>> Yes we can have reset status bit in either syss or syssconfig register.
>>
>> You mean sysstatus and sysconfig right?
> 
> Yup.
> 
>>> We detect that in sysc_init_syss_mask() but we should also set something
>>> at that point to make it clear which reset to use. But as we don't need
>>> the quirk flag, it's probably set a function pointer after the detection.
>>> So how about let's have two functions sysc_reset() and sysc_syss_reset()
>>> and then we can implement sysc_syss_reset() in a separate patch after
>>> testing it when we have a non-platform data using example for
>>> sysc_syss_reset().
>>>
>>
>> Shouldn't the function I add be called sysc_syss_reset()? The reset
>> status bit is in the sysstatus.
> 
> Yes
> 

Great. I thought I completely misunderstood you. But I don't see what
adding another function will accomplish. A QUIRK flag used in the same
function would work well enough.

Regards,
Faiz
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tony Lindgren June 11, 2018, 6:29 a.m. UTC | #7
* Faiz Abbas <faiz_abbas@ti.com> [180611 06:28]:
> Great. I thought I completely misunderstood you. But I don't see what
> adding another function will accomplish. A QUIRK flag used in the same
> function would work well enough.

Fine with me as long as the function stays simple for both
syss and sysc reset.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Faiz Abbas June 11, 2018, 6:47 a.m. UTC | #8
Hi,

On Monday 11 June 2018 11:59 AM, Tony Lindgren wrote:
> * Faiz Abbas <faiz_abbas@ti.com> [180611 06:28]:
>> Great. I thought I completely misunderstood you. But I don't see what
>> adding another function will accomplish. A QUIRK flag used in the same
>> function would work well enough>
> Fine with me as long as the function stays simple for both
> syss and sysc reset.
>


In general a reset status bit being in sysstatus is the norm and it
being in sysconfig should be the "quirk" for which a flag needs to be
added. What do you think?

As an aside, naming bitshifts by the name of the platform they were
originally added in seems weird. There should be some generic mask
saying "soft reset is the 0th bit". Currently I am using
SYSC_OMAP4_SOFTRESET for a dra76x module. I guess it depends on how many
different sysconfig types we have.

Thanks,
Faiz


--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tony Lindgren June 11, 2018, 7:03 a.m. UTC | #9
* Faiz Abbas <faiz_abbas@ti.com> [180611 06:48]:
> Hi,
> 
> On Monday 11 June 2018 11:59 AM, Tony Lindgren wrote:
> > * Faiz Abbas <faiz_abbas@ti.com> [180611 06:28]:
> >> Great. I thought I completely misunderstood you. But I don't see what
> >> adding another function will accomplish. A QUIRK flag used in the same
> >> function would work well enough>
> > Fine with me as long as the function stays simple for both
> > syss and sysc reset.
> >
> 
> 
> In general a reset status bit being in sysstatus is the norm and it
> being in sysconfig should be the "quirk" for which a flag needs to be
> added. What do you think?

Sure makes sense to me.

> As an aside, naming bitshifts by the name of the platform they were
> originally added in seems weird. There should be some generic mask
> saying "soft reset is the 0th bit". Currently I am using
> SYSC_OMAP4_SOFTRESET for a dra76x module. I guess it depends on how many
> different sysconfig types we have.

Sure we could have a macro for that.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tony Lindgren July 3, 2018, 7:07 a.m. UTC | #10
* Tony Lindgren <tony@atomide.com> [180611 07:06]:
> * Faiz Abbas <faiz_abbas@ti.com> [180611 06:48]:
> > Hi,
> > 
> > On Monday 11 June 2018 11:59 AM, Tony Lindgren wrote:
> > > * Faiz Abbas <faiz_abbas@ti.com> [180611 06:28]:
> > >> Great. I thought I completely misunderstood you. But I don't see what
> > >> adding another function will accomplish. A QUIRK flag used in the same
> > >> function would work well enough>
> > > Fine with me as long as the function stays simple for both
> > > syss and sysc reset.
> > >
> > 
> > 
> > In general a reset status bit being in sysstatus is the norm and it
> > being in sysconfig should be the "quirk" for which a flag needs to be
> > added. What do you think?
> 
> Sure makes sense to me.
> 
> > As an aside, naming bitshifts by the name of the platform they were
> > originally added in seems weird. There should be some generic mask
> > saying "soft reset is the 0th bit". Currently I am using
> > SYSC_OMAP4_SOFTRESET for a dra76x module. I guess it depends on how many
> > different sysconfig types we have.
> 
> Sure we could have a macro for that.

So what's the conclusion on this one? Are you going to do one
more version of the ti-sysc driver patch?

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Faiz Abbas July 3, 2018, 7:29 a.m. UTC | #11
Hi,

On Tuesday 03 July 2018 12:37 PM, Tony Lindgren wrote:
> * Tony Lindgren <tony@atomide.com> [180611 07:06]:
>> * Faiz Abbas <faiz_abbas@ti.com> [180611 06:48]:
>>> Hi,
>>>
>>> On Monday 11 June 2018 11:59 AM, Tony Lindgren wrote:
>>>> * Faiz Abbas <faiz_abbas@ti.com> [180611 06:28]:
>>>>> Great. I thought I completely misunderstood you. But I don't see what
>>>>> adding another function will accomplish. A QUIRK flag used in the same
>>>>> function would work well enough>
>>>> Fine with me as long as the function stays simple for both
>>>> syss and sysc reset.
>>>>
>>>
>>>
>>> In general a reset status bit being in sysstatus is the norm and it
>>> being in sysconfig should be the "quirk" for which a flag needs to be
>>> added. What do you think?
>>
>> Sure makes sense to me.
>>
>>> As an aside, naming bitshifts by the name of the platform they were
>>> originally added in seems weird. There should be some generic mask
>>> saying "soft reset is the 0th bit". Currently I am using
>>> SYSC_OMAP4_SOFTRESET for a dra76x module. I guess it depends on how many
>>> different sysconfig types we have.
>>
>> Sure we could have a macro for that.
> 
> So what's the conclusion on this one? Are you going to do one
> more version of the ti-sysc driver patch?
> 

Yes. I have just now been able to get back to this. Will post a v4 by
tomorrow.

Thanks,
Faiz
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tony Lindgren July 3, 2018, 7:31 a.m. UTC | #12
* Faiz Abbas <faiz_abbas@ti.com> [180703 07:31]:
> Hi,
> 
> On Tuesday 03 July 2018 12:37 PM, Tony Lindgren wrote:
> > * Tony Lindgren <tony@atomide.com> [180611 07:06]:
> >> * Faiz Abbas <faiz_abbas@ti.com> [180611 06:48]:
> >>> Hi,
> >>>
> >>> On Monday 11 June 2018 11:59 AM, Tony Lindgren wrote:
> >>>> * Faiz Abbas <faiz_abbas@ti.com> [180611 06:28]:
> >>>>> Great. I thought I completely misunderstood you. But I don't see what
> >>>>> adding another function will accomplish. A QUIRK flag used in the same
> >>>>> function would work well enough>
> >>>> Fine with me as long as the function stays simple for both
> >>>> syss and sysc reset.
> >>>>
> >>>
> >>>
> >>> In general a reset status bit being in sysstatus is the norm and it
> >>> being in sysconfig should be the "quirk" for which a flag needs to be
> >>> added. What do you think?
> >>
> >> Sure makes sense to me.
> >>
> >>> As an aside, naming bitshifts by the name of the platform they were
> >>> originally added in seems weird. There should be some generic mask
> >>> saying "soft reset is the 0th bit". Currently I am using
> >>> SYSC_OMAP4_SOFTRESET for a dra76x module. I guess it depends on how many
> >>> different sysconfig types we have.
> >>
> >> Sure we could have a macro for that.
> > 
> > So what's the conclusion on this one? Are you going to do one
> > more version of the ti-sysc driver patch?
> > 
> 
> Yes. I have just now been able to get back to this. Will post a v4 by
> tomorrow.

OK thanks!

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Faiz Abbas July 4, 2018, 1:36 p.m. UTC | #13
Hi,

On Tuesday 03 July 2018 01:01 PM, Tony Lindgren wrote:
> * Faiz Abbas <faiz_abbas@ti.com> [180703 07:31]:
>> Hi,
>>
>> On Tuesday 03 July 2018 12:37 PM, Tony Lindgren wrote:
>>> * Tony Lindgren <tony@atomide.com> [180611 07:06]:
>>>> * Faiz Abbas <faiz_abbas@ti.com> [180611 06:48]:
>>>>> Hi,
>>>>>
>>>>> On Monday 11 June 2018 11:59 AM, Tony Lindgren wrote:
>>>>>> * Faiz Abbas <faiz_abbas@ti.com> [180611 06:28]:
>>>>>>> Great. I thought I completely misunderstood you. But I don't see what
>>>>>>> adding another function will accomplish. A QUIRK flag used in the same
>>>>>>> function would work well enough>
>>>>>> Fine with me as long as the function stays simple for both
>>>>>> syss and sysc reset.
>>>>>>
>>>>>
>>>>>
>>>>> In general a reset status bit being in sysstatus is the norm and it
>>>>> being in sysconfig should be the "quirk" for which a flag needs to be
>>>>> added. What do you think?
>>>>
>>>> Sure makes sense to me.
>>>>
>>>>> As an aside, naming bitshifts by the name of the platform they were
>>>>> originally added in seems weird. There should be some generic mask
>>>>> saying "soft reset is the 0th bit". Currently I am using
>>>>> SYSC_OMAP4_SOFTRESET for a dra76x module. I guess it depends on how many
>>>>> different sysconfig types we have.
>>>>
>>>> Sure we could have a macro for that.
>>>
>>> So what's the conclusion on this one? Are you going to do one
>>> more version of the ti-sysc driver patch?
>>>
>>
>> Yes. I have just now been able to get back to this. Will post a v4 by
>> tomorrow.
> 
> OK thanks!
> 

After taking a second look at this thread, I don't see anything big to
be modified.

We both agree that "reset status bit in sysconfig register" is the quirk
case which can be added once such an IP is discovered in ti-sysc.

All I need to change is SYSC_OMAP4_SOFTRESET to SYSC_SOFT_RESET_SHIFT_0
for better readability and rebase it to the latest mainline.

Do reply if you differ on the above.

Thanks,
Faiz
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tony Lindgren July 5, 2018, 5:55 a.m. UTC | #14
* Faiz Abbas <faiz_abbas@ti.com> [180704 13:37]:
> After taking a second look at this thread, I don't see anything big to
> be modified.
> 
> We both agree that "reset status bit in sysconfig register" is the quirk
> case which can be added once such an IP is discovered in ti-sysc.

Yes agreed.

> All I need to change is SYSC_OMAP4_SOFTRESET to SYSC_SOFT_RESET_SHIFT_0
> for better readability and rebase it to the latest mainline.

Let's not change SYSC_OMAP4_SOFTRESET as only the ti-sysc
driver needs to know that and it can be set based on the
compatible.

How about replace ddata->cfg.quirks & SYSC_QUIRK_RESET_STATUS
test with just ddata->cfg.syss_mask test in your sysc_reset()?

We still need to set SYSC_QUIRK_RESET_STATUS too for pdata
callbacks.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Faiz Abbas July 5, 2018, 6:53 a.m. UTC | #15
Hi,

On Thursday 05 July 2018 11:25 AM, Tony Lindgren wrote:
> * Faiz Abbas <faiz_abbas@ti.com> [180704 13:37]:
>> After taking a second look at this thread, I don't see anything big to
>> be modified.
>>
>> We both agree that "reset status bit in sysconfig register" is the quirk
>> case which can be added once such an IP is discovered in ti-sysc.
> 
> Yes agreed.
> 
>> All I need to change is SYSC_OMAP4_SOFTRESET to SYSC_SOFT_RESET_SHIFT_0
>> for better readability and rebase it to the latest mainline.
> 
> Let's not change SYSC_OMAP4_SOFTRESET as only the ti-sysc
> driver needs to know that and it can be set based on the
> compatible.

Ok.
> 
> How about replace ddata->cfg.quirks & SYSC_QUIRK_RESET_STATUS
> test with just ddata->cfg.syss_mask test in your sysc_reset()?
> 
> We still need to set SYSC_QUIRK_RESET_STATUS too for pdata
> callbacks.

Sure. Do reset in ti-sysc only if dt has the correct syss_mask.

Rebasing and posting v4 soon.

Thanks,
Faiz
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/bus/ti-sysc.c b/drivers/bus/ti-sysc.c
index 4a2244419b9b..74d716a7bd6e 100644
--- a/drivers/bus/ti-sysc.c
+++ b/drivers/bus/ti-sysc.c
@@ -22,11 +22,14 @@ 
 #include <linux/of_address.h>
 #include <linux/of_platform.h>
 #include <linux/slab.h>
+#include <linux/iopoll.h>
 
 #include <linux/platform_data/ti-sysc.h>
 
 #include <dt-bindings/bus/ti-sysc.h>
 
+#define MAX_MODULE_SOFTRESET_WAIT		10000
+
 static const char * const reg_names[] = { "rev", "sysc", "syss", };
 
 enum sysc_clocks {
@@ -74,6 +77,11 @@  struct sysc {
 	struct delayed_work idle_work;
 };
 
+void sysc_write(struct sysc *ddata, int offset, u32 value)
+{
+	writel_relaxed(value, ddata->module_va + offset);
+}
+
 static u32 sysc_read(struct sysc *ddata, int offset)
 {
 	if (ddata->cfg.quirks & SYSC_QUIRK_16BIT) {
@@ -700,6 +708,26 @@  static void sysc_init_revision_quirks(struct sysc *ddata)
 	}
 }
 
+static int sysc_reset(struct sysc *ddata)
+{
+	int offset = ddata->offsets[SYSC_SYSCONFIG];
+	int val = sysc_read(ddata, offset);
+
+	val |= (0x1 << ddata->cap->regbits->srst_shift);
+	sysc_write(ddata, offset, val);
+
+	/* Poll on reset status */
+	if (ddata->cfg.quirks & SYSC_QUIRK_RESET_STATUS) {
+		offset = ddata->offsets[SYSC_SYSSTATUS];
+
+		return readl_poll_timeout(ddata->module_va + offset, val,
+				(val & ddata->cfg.syss_mask) == 0x0,
+				100, MAX_MODULE_SOFTRESET_WAIT);
+	}
+
+	return 0;
+}
+
 /* At this point the module is configured enough to read the revision */
 static int sysc_init_module(struct sysc *ddata)
 {
@@ -716,6 +744,18 @@  static int sysc_init_module(struct sysc *ddata)
 
 		return 0;
 	}
+
+	if (!(ddata->cfg.quirks & SYSC_QUIRK_NO_RESET_ON_INIT) &&
+	    !ddata->legacy_mode) {
+		error = sysc_reset(ddata);
+		if (error) {
+			dev_err(ddata->dev, "Reset failed with %d\n", error);
+			pm_runtime_put_sync(ddata->dev);
+
+			return error;
+		}
+	}
+
 	ddata->revision = sysc_read_revision(ddata);
 	pm_runtime_put_sync(ddata->dev);