[U-Boot] [PATCH] arm: socfpga: set the fpga global bit to disable HPS to FPGA signals

[U-Boot] [PATCH] arm: socfpga: set the fpga global bit to disable HPS to FPGA signals

[dinguyen at opensource.altera.com]

From: Dinh Nguyen <dinh.linux@gmail.com>

We should be setting the FPGA Interface Group global bit that will correctly
disable all interfaces between the FPGA and HPS.

Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
---
 drivers/fpga/socfpga.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c
index 4448250..431e159 100644
--- a/drivers/fpga/socfpga.c
+++ b/drivers/fpga/socfpga.c
@@ -269,7 +269,7 @@ int socfpga_load(Altera_desc *desc, const void *rbf_data, size_t rbf_size)
 	/* Prior programming the FPGA, all bridges need to be shut off */
 
 	/* Disable all signals from hps peripheral controller to fpga */
-	writel(0, &sysmgr_regs->fpgaintfgrp_module);
+	writel(0, &sysmgr_regs->fpgaintfgrp_gbl);
 
 	/* Disable all signals from FPGA to HPS SDRAM */
 #define SDR_CTRLGRP_FPGAPORTRST_ADDRESS	0x5080
-- 
2.6.2

[U-Boot] [PATCH] arm: socfpga: set the fpga global bit to disable HPS to FPGA signals

[Marek Vasut]

On Wednesday, January 06, 2016 at 08:48:43 PM, dinguyen at opensource.altera.com 
wrote:
> From: Dinh Nguyen <dinh.linux@gmail.com>
> 
> We should be setting the FPGA Interface Group global bit that will
> correctly disable all interfaces between the FPGA and HPS.
> 
> Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
> ---
>  drivers/fpga/socfpga.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c
> index 4448250..431e159 100644
> --- a/drivers/fpga/socfpga.c
> +++ b/drivers/fpga/socfpga.c
> @@ -269,7 +269,7 @@ int socfpga_load(Altera_desc *desc, const void
> *rbf_data, size_t rbf_size) /* Prior programming the FPGA, all bridges
> need to be shut off */
> 
>  	/* Disable all signals from hps peripheral controller to fpga */
> -	writel(0, &sysmgr_regs->fpgaintfgrp_module);
> +	writel(0, &sysmgr_regs->fpgaintfgrp_gbl);
> 
>  	/* Disable all signals from FPGA to HPS SDRAM */
>  #define SDR_CTRLGRP_FPGAPORTRST_ADDRESS	0x5080

Looks fine,

Acked-by: Marek Vasut <marex@denx.de>

btw Shouldn't you also fix the same thing in drivers/fpga/socfpga.c ? Even 
better, can't we have a single function to enable/disable the interfaces?

Best regards,
Marek Vasut

[U-Boot] [PATCH] arm: socfpga: set the fpga global bit to disable HPS to FPGA signals

[Dinh Nguyen]

On 01/06/2016 08:21 PM, Marek Vasut wrote:
> On Wednesday, January 06, 2016 at 08:48:43 PM, dinguyen at opensource.altera.com 
> wrote:
>> From: Dinh Nguyen <dinh.linux@gmail.com>
>>
>> We should be setting the FPGA Interface Group global bit that will
>> correctly disable all interfaces between the FPGA and HPS.
>>
>> Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
>> ---
>>  drivers/fpga/socfpga.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c
>> index 4448250..431e159 100644
>> --- a/drivers/fpga/socfpga.c
>> +++ b/drivers/fpga/socfpga.c
>> @@ -269,7 +269,7 @@ int socfpga_load(Altera_desc *desc, const void
>> *rbf_data, size_t rbf_size) /* Prior programming the FPGA, all bridges
>> need to be shut off */
>>
>>  	/* Disable all signals from hps peripheral controller to fpga */
>> -	writel(0, &sysmgr_regs->fpgaintfgrp_module);
>> +	writel(0, &sysmgr_regs->fpgaintfgrp_gbl);
>>
>>  	/* Disable all signals from FPGA to HPS SDRAM */
>>  #define SDR_CTRLGRP_FPGAPORTRST_ADDRESS	0x5080
> 
> Looks fine,
> 
> Acked-by: Marek Vasut <marex@denx.de>
> 
> btw Shouldn't you also fix the same thing in drivers/fpga/socfpga.c ? Even 

I'm confused by what you mean here. The patch is for drivers/fpga/socfpga.c.

> better, can't we have a single function to enable/disable the interfaces?
> 

We probably could, but I don't see a need for other functions to
enable/disable the interfaces.

Dinh

[U-Boot] [PATCH] arm: socfpga: set the fpga global bit to disable HPS to FPGA signals

[Marek Vasut]

On Thursday, January 07, 2016 at 03:50:00 AM, Dinh Nguyen wrote:
> On 01/06/2016 08:21 PM, Marek Vasut wrote:
> > On Wednesday, January 06, 2016 at 08:48:43 PM,
> > dinguyen at opensource.altera.com
> > 
> > wrote:
> >> From: Dinh Nguyen <dinh.linux@gmail.com>
> >> 
> >> We should be setting the FPGA Interface Group global bit that will
> >> correctly disable all interfaces between the FPGA and HPS.
> >> 
> >> Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
> >> ---
> >> 
> >>  drivers/fpga/socfpga.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c
> >> index 4448250..431e159 100644
> >> --- a/drivers/fpga/socfpga.c
> >> +++ b/drivers/fpga/socfpga.c
> >> @@ -269,7 +269,7 @@ int socfpga_load(Altera_desc *desc, const void
> >> *rbf_data, size_t rbf_size) /* Prior programming the FPGA, all bridges
> >> need to be shut off */
> >> 
> >>  	/* Disable all signals from hps peripheral controller to fpga */
> >> 
> >> -	writel(0, &sysmgr_regs->fpgaintfgrp_module);
> >> +	writel(0, &sysmgr_regs->fpgaintfgrp_gbl);
> >> 
> >>  	/* Disable all signals from FPGA to HPS SDRAM */
> >>  
> >>  #define SDR_CTRLGRP_FPGAPORTRST_ADDRESS	0x5080
> > 
> > Looks fine,
> > 
> > Acked-by: Marek Vasut <marex@denx.de>
> > 
> > btw Shouldn't you also fix the same thing in drivers/fpga/socfpga.c ?
> > Even
> 
> I'm confused by what you mean here. The patch is for
> drivers/fpga/socfpga.c.

Oops, sorry, there is another one in arch/arm/mach-socfpga/misc.c . I mispasted
the wrong filename.

> > better, can't we have a single function to enable/disable the interfaces?
> 
> We probably could, but I don't see a need for other functions to
> enable/disable the interfaces.
> 
> Dinh

Best regards,
Marek Vasut

[U-Boot] [PATCH] arm: socfpga: set the fpga global bit to disable HPS to FPGA signals

[Dinh Nguyen]

On 01/06/2016 09:20 PM, Marek Vasut wrote:
> On Thursday, January 07, 2016 at 03:50:00 AM, Dinh Nguyen wrote:
>> On 01/06/2016 08:21 PM, Marek Vasut wrote:
>>> On Wednesday, January 06, 2016 at 08:48:43 PM,
>>> dinguyen at opensource.altera.com
>>>
>>> wrote:
>>>> From: Dinh Nguyen <dinh.linux@gmail.com>
>>>>
>>>> We should be setting the FPGA Interface Group global bit that will
>>>> correctly disable all interfaces between the FPGA and HPS.
>>>>
>>>> Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
>>>> ---
>>>>
>>>>  drivers/fpga/socfpga.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c
>>>> index 4448250..431e159 100644
>>>> --- a/drivers/fpga/socfpga.c
>>>> +++ b/drivers/fpga/socfpga.c
>>>> @@ -269,7 +269,7 @@ int socfpga_load(Altera_desc *desc, const void
>>>> *rbf_data, size_t rbf_size) /* Prior programming the FPGA, all bridges
>>>> need to be shut off */
>>>>
>>>>  	/* Disable all signals from hps peripheral controller to fpga */
>>>>
>>>> -	writel(0, &sysmgr_regs->fpgaintfgrp_module);
>>>> +	writel(0, &sysmgr_regs->fpgaintfgrp_gbl);
>>>>
>>>>  	/* Disable all signals from FPGA to HPS SDRAM */
>>>>  
>>>>  #define SDR_CTRLGRP_FPGAPORTRST_ADDRESS	0x5080
>>>
>>> Looks fine,
>>>
>>> Acked-by: Marek Vasut <marex@denx.de>
>>>
>>> btw Shouldn't you also fix the same thing in drivers/fpga/socfpga.c ?
>>> Even
>>
>> I'm confused by what you mean here. The patch is for
>> drivers/fpga/socfpga.c.
> 
> Oops, sorry, there is another one in arch/arm/mach-socfpga/misc.c . I mispasted
> the wrong filename.
> 

Ah yes, then we should probably add a function to enable/disable the
interfaces.

Dinh

[U-Boot] [PATCH] arm: socfpga: set the fpga global bit to disable HPS to FPGA signals

[Marek Vasut]

On Thursday, January 07, 2016 at 04:03:02 PM, Dinh Nguyen wrote:
> On 01/06/2016 09:20 PM, Marek Vasut wrote:
> > On Thursday, January 07, 2016 at 03:50:00 AM, Dinh Nguyen wrote:
> >> On 01/06/2016 08:21 PM, Marek Vasut wrote:
> >>> On Wednesday, January 06, 2016 at 08:48:43 PM,
> >>> dinguyen at opensource.altera.com
> >>> 
> >>> wrote:
> >>>> From: Dinh Nguyen <dinh.linux@gmail.com>
> >>>> 
> >>>> We should be setting the FPGA Interface Group global bit that will
> >>>> correctly disable all interfaces between the FPGA and HPS.
> >>>> 
> >>>> Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
> >>>> ---
> >>>> 
> >>>>  drivers/fpga/socfpga.c | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>> 
> >>>> diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c
> >>>> index 4448250..431e159 100644
> >>>> --- a/drivers/fpga/socfpga.c
> >>>> +++ b/drivers/fpga/socfpga.c
> >>>> @@ -269,7 +269,7 @@ int socfpga_load(Altera_desc *desc, const void
> >>>> *rbf_data, size_t rbf_size) /* Prior programming the FPGA, all bridges
> >>>> need to be shut off */
> >>>> 
> >>>>  	/* Disable all signals from hps peripheral controller to fpga */
> >>>> 
> >>>> -	writel(0, &sysmgr_regs->fpgaintfgrp_module);
> >>>> +	writel(0, &sysmgr_regs->fpgaintfgrp_gbl);
> >>>> 
> >>>>  	/* Disable all signals from FPGA to HPS SDRAM */
> >>>>  
> >>>>  #define SDR_CTRLGRP_FPGAPORTRST_ADDRESS	0x5080
> >>> 
> >>> Looks fine,
> >>> 
> >>> Acked-by: Marek Vasut <marex@denx.de>
> >>> 
> >>> btw Shouldn't you also fix the same thing in drivers/fpga/socfpga.c ?
> >>> Even
> >> 
> >> I'm confused by what you mean here. The patch is for
> >> drivers/fpga/socfpga.c.
> > 
> > Oops, sorry, there is another one in arch/arm/mach-socfpga/misc.c . I
> > mispasted the wrong filename.
> 
> Ah yes, then we should probably add a function to enable/disable the
> interfaces.

Yes, it might make sense to unify the code a bit.

Best regards,
Marek Vasut

[U-Boot] [PATCH] arm: socfpga: set the fpga global bit to disable HPS to FPGA signals

[Dinh Nguyen]

On 01/07/2016 09:03 AM, Dinh Nguyen wrote:
> 
> 
> On 01/06/2016 09:20 PM, Marek Vasut wrote:
>> On Thursday, January 07, 2016 at 03:50:00 AM, Dinh Nguyen wrote:
>>> On 01/06/2016 08:21 PM, Marek Vasut wrote:
>>>> On Wednesday, January 06, 2016 at 08:48:43 PM,
>>>> dinguyen at opensource.altera.com
>>>>
>>>> wrote:
>>>>> From: Dinh Nguyen <dinh.linux@gmail.com>
>>>>>
>>>>> We should be setting the FPGA Interface Group global bit that will
>>>>> correctly disable all interfaces between the FPGA and HPS.
>>>>>
>>>>> Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
>>>>> ---
>>>>>
>>>>>  drivers/fpga/socfpga.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c
>>>>> index 4448250..431e159 100644
>>>>> --- a/drivers/fpga/socfpga.c
>>>>> +++ b/drivers/fpga/socfpga.c
>>>>> @@ -269,7 +269,7 @@ int socfpga_load(Altera_desc *desc, const void
>>>>> *rbf_data, size_t rbf_size) /* Prior programming the FPGA, all bridges
>>>>> need to be shut off */
>>>>>
>>>>>  	/* Disable all signals from hps peripheral controller to fpga */
>>>>>
>>>>> -	writel(0, &sysmgr_regs->fpgaintfgrp_module);
>>>>> +	writel(0, &sysmgr_regs->fpgaintfgrp_gbl);
>>>>>
>>>>>  	/* Disable all signals from FPGA to HPS SDRAM */
>>>>>  
>>>>>  #define SDR_CTRLGRP_FPGAPORTRST_ADDRESS	0x5080
>>>>
>>>> Looks fine,
>>>>
>>>> Acked-by: Marek Vasut <marex@denx.de>
>>>>
>>>> btw Shouldn't you also fix the same thing in drivers/fpga/socfpga.c ?
>>>> Even
>>>
>>> I'm confused by what you mean here. The patch is for
>>> drivers/fpga/socfpga.c.
>>
>> Oops, sorry, there is another one in arch/arm/mach-socfpga/misc.c . I mispasted
>> the wrong filename.
>>
> 
> Ah yes, then we should probably add a function to enable/disable the
> interfaces.
> 

But I wonder if I wanted to enable the individual interfaces, i.e. the
separate EMACs that is in fpgaintfgrp_module, would I need to enable the
fpgaintfgrp_gbl bit?

Dinh

[U-Boot] [PATCH] arm: socfpga: set the fpga global bit to disable HPS to FPGA signals

[Marek Vasut]

On Thursday, January 07, 2016 at 04:23:09 PM, Dinh Nguyen wrote:
> On 01/07/2016 09:03 AM, Dinh Nguyen wrote:
> > On 01/06/2016 09:20 PM, Marek Vasut wrote:
> >> On Thursday, January 07, 2016 at 03:50:00 AM, Dinh Nguyen wrote:
> >>> On 01/06/2016 08:21 PM, Marek Vasut wrote:
> >>>> On Wednesday, January 06, 2016 at 08:48:43 PM,
> >>>> dinguyen at opensource.altera.com
> >>>> 
> >>>> wrote:
> >>>>> From: Dinh Nguyen <dinh.linux@gmail.com>
> >>>>> 
> >>>>> We should be setting the FPGA Interface Group global bit that will
> >>>>> correctly disable all interfaces between the FPGA and HPS.
> >>>>> 
> >>>>> Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
> >>>>> ---
> >>>>> 
> >>>>>  drivers/fpga/socfpga.c | 2 +-
> >>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>> 
> >>>>> diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c
> >>>>> index 4448250..431e159 100644
> >>>>> --- a/drivers/fpga/socfpga.c
> >>>>> +++ b/drivers/fpga/socfpga.c
> >>>>> @@ -269,7 +269,7 @@ int socfpga_load(Altera_desc *desc, const void
> >>>>> *rbf_data, size_t rbf_size) /* Prior programming the FPGA, all
> >>>>> bridges need to be shut off */
> >>>>> 
> >>>>>  	/* Disable all signals from hps peripheral controller to fpga */
> >>>>> 
> >>>>> -	writel(0, &sysmgr_regs->fpgaintfgrp_module);
> >>>>> +	writel(0, &sysmgr_regs->fpgaintfgrp_gbl);
> >>>>> 
> >>>>>  	/* Disable all signals from FPGA to HPS SDRAM */
> >>>>>  
> >>>>>  #define SDR_CTRLGRP_FPGAPORTRST_ADDRESS	0x5080
> >>>> 
> >>>> Looks fine,
> >>>> 
> >>>> Acked-by: Marek Vasut <marex@denx.de>
> >>>> 
> >>>> btw Shouldn't you also fix the same thing in drivers/fpga/socfpga.c ?
> >>>> Even
> >>> 
> >>> I'm confused by what you mean here. The patch is for
> >>> drivers/fpga/socfpga.c.
> >> 
> >> Oops, sorry, there is another one in arch/arm/mach-socfpga/misc.c . I
> >> mispasted the wrong filename.
> > 
> > Ah yes, then we should probably add a function to enable/disable the
> > interfaces.
> 
> But I wonder if I wanted to enable the individual interfaces, i.e. the
> separate EMACs that is in fpgaintfgrp_module, would I need to enable the
> fpgaintfgrp_gbl bit?

Aren't you the one working for Altera? You're the one who should know the best 
:-) But I think this gbl bit is like the top-level killswitch, so you need to
enable this one and only then you can fine-tune config of the subblocks (like
the EMACs etc).

Best regards,
Marek Vasut

[U-Boot] [PATCH] arm: socfpga: set the fpga global bit to disable HPS to FPGA signals

[Chin Liang See]

On Thu, 2016-01-07 at 16:33 +0100, Marek Vasut wrote:
> On Thursday, January 07, 2016 at 04:23:09 PM, Dinh Nguyen wrote:
> > On 01/07/2016 09:03 AM, Dinh Nguyen wrote:
> > > On 01/06/2016 09:20 PM, Marek Vasut wrote:
> > > > On Thursday, January 07, 2016 at 03:50:00 AM, Dinh Nguyen
> > > > wrote:
> > > > > On 01/06/2016 08:21 PM, Marek Vasut wrote:
> > > > > > On Wednesday, January 06, 2016 at 08:48:43 PM,
> > > > > > dinguyen at opensource.altera.com
> > > > > > 
> > > > > > wrote:
> > > > > > > From: Dinh Nguyen <dinh.linux@gmail.com>
> > > > > > > 
> > > > > > > We should be setting the FPGA Interface Group global bit
> > > > > > > that will
> > > > > > > correctly disable all interfaces between the FPGA and
> > > > > > > HPS.
> > > > > > > 
> > > > > > > Signed-off-by: Dinh Nguyen <
> > > > > > > dinguyen at opensource.altera.com>
> > > > > > > ---
> > > > > > > 
> > > > > > >  drivers/fpga/socfpga.c | 2 +-
> > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/fpga/socfpga.c
> > > > > > > b/drivers/fpga/socfpga.c
> > > > > > > index 4448250..431e159 100644
> > > > > > > --- a/drivers/fpga/socfpga.c
> > > > > > > +++ b/drivers/fpga/socfpga.c
> > > > > > > @@ -269,7 +269,7 @@ int socfpga_load(Altera_desc *desc,
> > > > > > > const void
> > > > > > > *rbf_data, size_t rbf_size) /* Prior programming the
> > > > > > > FPGA, all
> > > > > > > bridges need to be shut off */
> > > > > > > 
> > > > > > >  	/* Disable all signals from hps peripheral
> > > > > > > controller to fpga */
> > > > > > > 
> > > > > > > -	writel(0, &sysmgr_regs->fpgaintfgrp_module);
> > > > > > > +	writel(0, &sysmgr_regs->fpgaintfgrp_gbl);
> > > > > > > 
> > > > > > >  	/* Disable all signals from FPGA to HPS SDRAM */
> > > > > > >  
> > > > > > >  #define SDR_CTRLGRP_FPGAPORTRST_ADDRESS	0x5080
> > > > > > 
> > > > > > Looks fine,
> > > > > > 
> > > > > > Acked-by: Marek Vasut <marex@denx.de>
> > > > > > 
> > > > > > btw Shouldn't you also fix the same thing in
> > > > > > drivers/fpga/socfpga.c ?
> > > > > > Even
> > > > > 
> > > > > I'm confused by what you mean here. The patch is for
> > > > > drivers/fpga/socfpga.c.
> > > > 
> > > > Oops, sorry, there is another one in arch/arm/mach
> > > > -socfpga/misc.c . I
> > > > mispasted the wrong filename.
> > > 
> > > Ah yes, then we should probably add a function to enable/disable
> > > the
> > > interfaces.
> > 
> > But I wonder if I wanted to enable the individual interfaces, i.e.
> > the
> > separate EMACs that is in fpgaintfgrp_module, would I need to
> > enable the
> > fpgaintfgrp_gbl bit?
> 
> Aren't you the one working for Altera? You're the one who should know
> the best 
> :-) But I think this gbl bit is like the top-level killswitch, so you
> need to
> enable this one and only then you can fine-tune config of the
> subblocks (like
> the EMACs etc).

We would need to enable the global bit to have the individual interface
works. Each interface enable signal is a result of AND between
indvidual enable register and global enable register.

Thanks
Chin Liang

> 
> Best regards,
> Marek Vasut