[U-Boot] [PATCH] reset: socfpga: release more A10 peripherals out of reset

[U-Boot] [PATCH] reset: socfpga: release more A10 peripherals out of reset

[tien.fong.chee at intel.com]

From: Tien Fong Chee <tien.fong.chee@intel.com>

Current implementation almost release all peripherals out of reset for
gen5, but A10 has more peripherals than gen5, hence this patch is required
to release the rest of peripherals to support old kernels.

Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
---
 drivers/reset/reset-socfpga.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/reset/reset-socfpga.c b/drivers/reset/reset-socfpga.c
index cb8312619f..d8b8b25fc3 100644
--- a/drivers/reset/reset-socfpga.c
+++ b/drivers/reset/reset-socfpga.c
@@ -127,6 +127,9 @@ static int socfpga_reset_remove(struct udevice *dev)
 	if (socfpga_reset_keep_enabled()) {
 		puts("Deasserting all peripheral resets\n");
 		writel(0, data->modrst_base + 4);
+#if defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
+		writel(0, data->modrst_base + 8);
+#endif
 	}
 
 	return 0;
-- 
2.13.0

[U-Boot] [PATCH] reset: socfpga: release more A10 peripherals out of reset

[Marek Vasut]

On 5/24/19 1:40 PM, tien.fong.chee at intel.com wrote:
> From: Tien Fong Chee <tien.fong.chee@intel.com>
> 
> Current implementation almost release all peripherals out of reset for
> gen5, but A10 has more peripherals than gen5, hence this patch is required
> to release the rest of peripherals to support old kernels.
> 
> Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
> ---
>  drivers/reset/reset-socfpga.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/reset/reset-socfpga.c b/drivers/reset/reset-socfpga.c
> index cb8312619f..d8b8b25fc3 100644
> --- a/drivers/reset/reset-socfpga.c
> +++ b/drivers/reset/reset-socfpga.c
> @@ -127,6 +127,9 @@ static int socfpga_reset_remove(struct udevice *dev)
>  	if (socfpga_reset_keep_enabled()) {
>  		puts("Deasserting all peripheral resets\n");
>  		writel(0, data->modrst_base + 4);
> +#if defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
> +		writel(0, data->modrst_base + 8);

This should be using match on compatible string.
Which register is this modrst_base + 8 ?

> +#endif
>  	}
>  
>  	return 0;
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] reset: socfpga: release more A10 peripherals out of reset

[Simon Goldschmidt]

On Fri, May 24, 2019 at 1:44 PM Marek Vasut <marex@denx.de> wrote:
>
> On 5/24/19 1:40 PM, tien.fong.chee at intel.com wrote:
> > From: Tien Fong Chee <tien.fong.chee@intel.com>
> >
> > Current implementation almost release all peripherals out of reset for
> > gen5, but A10 has more peripherals than gen5, hence this patch is required
> > to release the rest of peripherals to support old kernels.
> >
> > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
> > ---
> >  drivers/reset/reset-socfpga.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/reset/reset-socfpga.c b/drivers/reset/reset-socfpga.c
> > index cb8312619f..d8b8b25fc3 100644
> > --- a/drivers/reset/reset-socfpga.c
> > +++ b/drivers/reset/reset-socfpga.c
> > @@ -127,6 +127,9 @@ static int socfpga_reset_remove(struct udevice *dev)
> >       if (socfpga_reset_keep_enabled()) {
> >               puts("Deasserting all peripheral resets\n");
> >               writel(0, data->modrst_base + 4);
> > +#if defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
> > +             writel(0, data->modrst_base + 8);
>
> This should be using match on compatible string.
> Which register is this modrst_base + 8 ?

Well, even modrst_base + 4 is not really ideal. After all, modrst_base is a
dts-given offset into the rstmgr's registers, so there is not really a
comptile time known constant to do this.

Maybe it would be better to use an offset to the device's membase.
However, as this is a backwards-compatibility workaround, there's no use to
add this into the devicetree.

And unfortunately, both gen5 and a10 use the same compatible string, which
probably would have to be changed.

Regards,
Simon

>
> > +#endif
> >       }
> >
> >       return 0;
> >
>
>
> --
> Best regards,
> Marek Vasut

[U-Boot] [PATCH] reset: socfpga: release more A10 peripherals out of reset

[Chee, Tien Fong]

On Fri, 2019-05-24 at 13:53 +0200, Simon Goldschmidt wrote:
> On Fri, May 24, 2019 at 1:44 PM Marek Vasut <marex@denx.de> wrote:
> > 
> > 
> > On 5/24/19 1:40 PM, tien.fong.chee at intel.com wrote:
> > > 
> > > From: Tien Fong Chee <tien.fong.chee@intel.com>
> > > 
> > > Current implementation almost release all peripherals out of
> > > reset for
> > > gen5, but A10 has more peripherals than gen5, hence this patch is
> > > required
> > > to release the rest of peripherals to support old kernels.
> > > 
> > > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
> > > ---
> > >  drivers/reset/reset-socfpga.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/reset/reset-socfpga.c b/drivers/reset/reset-
> > > socfpga.c
> > > index cb8312619f..d8b8b25fc3 100644
> > > --- a/drivers/reset/reset-socfpga.c
> > > +++ b/drivers/reset/reset-socfpga.c
> > > @@ -127,6 +127,9 @@ static int socfpga_reset_remove(struct
> > > udevice *dev)
> > >       if (socfpga_reset_keep_enabled()) {
> > >               puts("Deasserting all peripheral resets\n");
> > >               writel(0, data->modrst_base + 4);
> > > +#if defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
> > > +             writel(0, data->modrst_base + 8);
> > This should be using match on compatible string.
> > Which register is this modrst_base + 8 ?
> Well, even modrst_base + 4 is not really ideal. After all,
> modrst_base is a
> dts-given offset into the rstmgr's registers, so there is not really
> a
> comptile time known constant to do this.
> 
> Maybe it would be better to use an offset to the device's membase.
> However, as this is a backwards-compatibility workaround, there's no
> use to
> add this into the devicetree.
> 
> And unfortunately, both gen5 and a10 use the same compatible string,
> which
> probably would have to be changed.
> 
> Regards,
> Simon

This register is per1modrst, here is the link https://www.intel.com/con
tent/www/us/en/programmable/hps/arria-
10/hps.html#reg_soc_top/sfo1429890576520.html
This whole mechanism should be removed once Dinh has fixed this issue
at Linux v4.14-lts or moving to Linux v5.0 onwards.

> 
> > 
> > 
> > > 
> > > +#endif
> > >       }
> > > 
> > >       return 0;
> > > 
> > 
> > --
> > Best regards,
> > Marek Vasut

[U-Boot] [PATCH] reset: socfpga: release more A10 peripherals out of reset

[Simon Goldschmidt]

On Fri, May 24, 2019 at 1:57 PM Chee, Tien Fong
<tien.fong.chee@intel.com> wrote:
>
> On Fri, 2019-05-24 at 13:53 +0200, Simon Goldschmidt wrote:
> > On Fri, May 24, 2019 at 1:44 PM Marek Vasut <marex@denx.de> wrote:
> > >
> > >
> > > On 5/24/19 1:40 PM, tien.fong.chee at intel.com wrote:
> > > >
> > > > From: Tien Fong Chee <tien.fong.chee@intel.com>
> > > >
> > > > Current implementation almost release all peripherals out of
> > > > reset for
> > > > gen5, but A10 has more peripherals than gen5, hence this patch is
> > > > required
> > > > to release the rest of peripherals to support old kernels.
> > > >
> > > > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
> > > > ---
> > > >  drivers/reset/reset-socfpga.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/reset/reset-socfpga.c b/drivers/reset/reset-
> > > > socfpga.c
> > > > index cb8312619f..d8b8b25fc3 100644
> > > > --- a/drivers/reset/reset-socfpga.c
> > > > +++ b/drivers/reset/reset-socfpga.c
> > > > @@ -127,6 +127,9 @@ static int socfpga_reset_remove(struct
> > > > udevice *dev)
> > > >       if (socfpga_reset_keep_enabled()) {
> > > >               puts("Deasserting all peripheral resets\n");
> > > >               writel(0, data->modrst_base + 4);
> > > > +#if defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
> > > > +             writel(0, data->modrst_base + 8);
> > > This should be using match on compatible string.
> > > Which register is this modrst_base + 8 ?
> > Well, even modrst_base + 4 is not really ideal. After all,
> > modrst_base is a
> > dts-given offset into the rstmgr's registers, so there is not really
> > a
> > comptile time known constant to do this.
> >
> > Maybe it would be better to use an offset to the device's membase.
> > However, as this is a backwards-compatibility workaround, there's no
> > use to
> > add this into the devicetree.
> >
> > And unfortunately, both gen5 and a10 use the same compatible string,
> > which
> > probably would have to be changed.
> >
> > Regards,
> > Simon
>
> This register is per1modrst, here is the link https://www.intel.com/con
> tent/www/us/en/programmable/hps/arria-
> 10/hps.html#reg_soc_top/sfo1429890576520.html
> This whole mechanism should be removed once Dinh has fixed this issue
> at Linux v4.14-lts or moving to Linux v5.0 onwards.

I agree it can be removed in the future, but is it really enough to have this
in 4.14-lts? I'm not so sure that we should prevent people from using
mainline U-Boot with older kernels...

Regards,
Simon

>
> >
> > >
> > >
> > > >
> > > > +#endif
> > > >       }
> > > >
> > > >       return 0;
> > > >
> > >
> > > --
> > > Best regards,
> > > Marek Vasut

[U-Boot] [PATCH] reset: socfpga: release more A10 peripherals out of reset

[Chee, Tien Fong]

On Fri, 2019-05-24 at 14:00 +0200, Simon Goldschmidt wrote:
> On Fri, May 24, 2019 at 1:57 PM Chee, Tien Fong
> <tien.fong.chee@intel.com> wrote:
> > 
> > 
> > On Fri, 2019-05-24 at 13:53 +0200, Simon Goldschmidt wrote:
> > > 
> > > On Fri, May 24, 2019 at 1:44 PM Marek Vasut <marex@denx.de>
> > > wrote:
> > > > 
> > > > 
> > > > 
> > > > On 5/24/19 1:40 PM, tien.fong.chee at intel.com wrote:
> > > > > 
> > > > > 
> > > > > From: Tien Fong Chee <tien.fong.chee@intel.com>
> > > > > 
> > > > > Current implementation almost release all peripherals out of
> > > > > reset for
> > > > > gen5, but A10 has more peripherals than gen5, hence this
> > > > > patch is
> > > > > required
> > > > > to release the rest of peripherals to support old kernels.
> > > > > 
> > > > > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
> > > > > ---
> > > > >  drivers/reset/reset-socfpga.c | 3 +++
> > > > >  1 file changed, 3 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/reset/reset-socfpga.c
> > > > > b/drivers/reset/reset-
> > > > > socfpga.c
> > > > > index cb8312619f..d8b8b25fc3 100644
> > > > > --- a/drivers/reset/reset-socfpga.c
> > > > > +++ b/drivers/reset/reset-socfpga.c
> > > > > @@ -127,6 +127,9 @@ static int socfpga_reset_remove(struct
> > > > > udevice *dev)
> > > > >       if (socfpga_reset_keep_enabled()) {
> > > > >               puts("Deasserting all peripheral resets\n");
> > > > >               writel(0, data->modrst_base + 4);
> > > > > +#if defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
> > > > > +             writel(0, data->modrst_base + 8);
> > > > This should be using match on compatible string.
> > > > Which register is this modrst_base + 8 ?
> > > Well, even modrst_base + 4 is not really ideal. After all,
> > > modrst_base is a
> > > dts-given offset into the rstmgr's registers, so there is not
> > > really
> > > a
> > > comptile time known constant to do this.
> > > 
> > > Maybe it would be better to use an offset to the device's
> > > membase.
> > > However, as this is a backwards-compatibility workaround, there's
> > > no
> > > use to
> > > add this into the devicetree.
> > > 
> > > And unfortunately, both gen5 and a10 use the same compatible
> > > string,
> > > which
> > > probably would have to be changed.
> > > 
> > > Regards,
> > > Simon
> > This register is per1modrst, here is the link https://www.intel.com
> > /con
> > tent/www/us/en/programmable/hps/arria-
> > 10/hps.html#reg_soc_top/sfo1429890576520.html
> > This whole mechanism should be removed once Dinh has fixed this
> > issue
> > at Linux v4.14-lts or moving to Linux v5.0 onwards.
> I agree it can be removed in the future, but is it really enough to
> have this
> in 4.14-lts? I'm not so sure that we should prevent people from using
> mainline U-Boot with older kernels...

I have no strong opinion on this, but how old the kernel U-Boot want to
support, subject to the maintainer :) .

> 
> Regards,
> Simon
> 
> > 
> > 
> > > 
> > > 
> > > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > +#endif
> > > > >       }
> > > > > 
> > > > >       return 0;
> > > > > 
> > > > --
> > > > Best regards,
> > > > Marek Vasut

[U-Boot] [PATCH] reset: socfpga: release more A10 peripherals out of reset

[Simon Goldschmidt]

Am 24.05.2019 um 14:16 schrieb Chee, Tien Fong:
> On Fri, 2019-05-24 at 14:00 +0200, Simon Goldschmidt wrote:
>> On Fri, May 24, 2019 at 1:57 PM Chee, Tien Fong
>> <tien.fong.chee@intel.com> wrote:
>>>
>>>
>>> On Fri, 2019-05-24 at 13:53 +0200, Simon Goldschmidt wrote:
>>>>
>>>> On Fri, May 24, 2019 at 1:44 PM Marek Vasut <marex@denx.de>
>>>> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 5/24/19 1:40 PM, tien.fong.chee at intel.com wrote:
>>>>>>
>>>>>>
>>>>>> From: Tien Fong Chee <tien.fong.chee@intel.com>
>>>>>>
>>>>>> Current implementation almost release all peripherals out of
>>>>>> reset for
>>>>>> gen5, but A10 has more peripherals than gen5, hence this
>>>>>> patch is
>>>>>> required
>>>>>> to release the rest of peripherals to support old kernels.
>>>>>>
>>>>>> Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>

Even if this is old, I'm ok with it, since it's only "temporary" until 
the Linux drivers can handle RST, so:

Reviewed-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>

>>>>>> ---
>>>>>>   drivers/reset/reset-socfpga.c | 3 +++
>>>>>>   1 file changed, 3 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/reset/reset-socfpga.c
>>>>>> b/drivers/reset/reset-
>>>>>> socfpga.c
>>>>>> index cb8312619f..d8b8b25fc3 100644
>>>>>> --- a/drivers/reset/reset-socfpga.c
>>>>>> +++ b/drivers/reset/reset-socfpga.c
>>>>>> @@ -127,6 +127,9 @@ static int socfpga_reset_remove(struct
>>>>>> udevice *dev)
>>>>>>        if (socfpga_reset_keep_enabled()) {
>>>>>>                puts("Deasserting all peripheral resets\n");
>>>>>>                writel(0, data->modrst_base + 4);
>>>>>> +#if defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
>>>>>> +             writel(0, data->modrst_base + 8);
>>>>> This should be using match on compatible string.
>>>>> Which register is this modrst_base + 8 ?
>>>> Well, even modrst_base + 4 is not really ideal. After all,
>>>> modrst_base is a
>>>> dts-given offset into the rstmgr's registers, so there is not
>>>> really
>>>> a
>>>> comptile time known constant to do this.
>>>>
>>>> Maybe it would be better to use an offset to the device's
>>>> membase.
>>>> However, as this is a backwards-compatibility workaround, there's
>>>> no
>>>> use to
>>>> add this into the devicetree.
>>>>
>>>> And unfortunately, both gen5 and a10 use the same compatible
>>>> string,
>>>> which
>>>> probably would have to be changed.
>>>>
>>>> Regards,
>>>> Simon
>>> This register is per1modrst, here is the link https://www.intel.com
>>> /con
>>> tent/www/us/en/programmable/hps/arria-
>>> 10/hps.html#reg_soc_top/sfo1429890576520.html
>>> This whole mechanism should be removed once Dinh has fixed this
>>> issue
>>> at Linux v4.14-lts or moving to Linux v5.0 onwards.
>> I agree it can be removed in the future, but is it really enough to
>> have this
>> in 4.14-lts? I'm not so sure that we should prevent people from using
>> mainline U-Boot with older kernels...
> 
> I have no strong opinion on this, but how old the kernel U-Boot want to
> support, subject to the maintainer :) .
> 
>>
>> Regards,
>> Simon
>>
>>>
>>>
>>>>
>>>>
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>> +#endif
>>>>>>        }
>>>>>>
>>>>>>        return 0;
>>>>>>
>>>>> --
>>>>> Best regards,
>>>>> Marek Vasut