[PATCH/RFC] Booting Xilinx ML510 board using SystemACE

I am using locally the attached (hackish) patch; could someone propose a
way to make it acceptable to mainline (or should I just keep this
quiet)?

 		 (version >> 12) & 0xf, (version >> 8) & 0x0f, version &
0xff);
**********************************************************************************************
IMPORTANT: The contents of this email and any attachments are confidential. They are intended for the 
named recipient(s) only.
If you have received this email in error, please notify the system manager or the sender immediately and do 
not disclose the contents to anyone or make copies thereof.

Alon,

There are at least two other ways that you might be able to reset a
board:
1) Internally through the ICAP device.
2) Through a GPIO connected externally to the reset logic.

Part of this is board specific, part of is it design specific.
Probably it would be best to have a mechanism in the device tree which
references the
reset mechanism?  In any event, you probably don't want a driver to
eplicitly reference the plaform code.  If you want to do it explicitly
like this, it would better to have the plaform code reference the driver
mechanism.  This would, to some extent, generalize to the device tree
case.

Steve
 
> -----Original Message-----
> From: linuxppc-dev-bounces+stephen=neuendorffer.name@lists.ozlabs.org
[mailto:linuxppc-dev-
> bounces+stephen=neuendorffer.name@lists.ozlabs.org] On Behalf Of Alon
Ziv
> Sent: Sunday, November 15, 2009 1:34 AM
> To: linuxppc-dev
> Subject: [PATCH/RFC] Booting Xilinx ML510 board using SystemACE
> 
> I am using locally the attached (hackish) patch; could someone propose
a
> way to make it acceptable to mainline (or should I just keep this
> quiet)?
> 
> diff --git a/arch/powerpc/platforms/44x/virtex.c
> b/arch/powerpc/platforms/44x/virtex.c
> index cf96cca..749a330 100644
> --- a/arch/powerpc/platforms/44x/virtex.c
> +++ b/arch/powerpc/platforms/44x/virtex.c
> @@ -51,6 +51,16 @@ static int __init virtex_probe(void)
>  	return 1;
>  }
> 
> +void (*board_reset_system)(char *);
> +EXPORT_SYMBOL(board_reset_system);
> +
> +static void virtex_reset_system(char *cmd)
> +{
> +	if (board_reset_system)
> +		board_reset_system(cmd);
> +	ppc4xx_reset_system(cmd);
> +}
> +
>  define_machine(virtex) {
>  	.name			= "Xilinx Virtex440",
>  	.probe			= virtex_probe,
> @@ -58,5 +68,5 @@ define_machine(virtex) {
>  	.init_IRQ		= xilinx_intc_init_tree,
>  	.get_irq		= xilinx_intc_get_irq,
>  	.calibrate_decr		= generic_calibrate_decr,
> -	.restart		= ppc4xx_reset_system,
> +	.restart		= virtex_reset_system,
>  };
> diff --git a/drivers/block/xsysace.c b/drivers/block/xsysace.c
> index b20abe1..f3b4ab9 100644
> --- a/drivers/block/xsysace.c
> +++ b/drivers/block/xsysace.c
> @@ -950,6 +950,19 @@ static struct block_device_operations ace_fops =
{
>  	.getgeo = ace_getgeo,
>  };
> 
> +extern void (*board_reset_system)(char *);
> +static struct ace_device *the_ace_device;
> +
> +/*
--------------------------------------------------------------------
> + * Board reset using ACE (forced FPGA reconfigure)
> + */
> +static void ace_reset_system(char *cmd)
> +{
> +	printk(KERN_EMERG "ace_reset_system: attempting suicide\n");
> +	ace_out(the_ace_device, ACE_CTRL, ACE_CTRL_FORCECFGMODE |
> +		ACE_CTRL_CFGMODE | ACE_CTRL_CFGSTART | ACE_CTRL_CFGSEL);
> +}
> +
>  /*
--------------------------------------------------------------------
>   * SystemACE device setup/teardown code
>   */
> @@ -1040,6 +1053,9 @@ static int __devinit ace_setup(struct ace_device
> *ace)
>  	val |= ACE_CTRL_DATABUFRDYIRQ | ACE_CTRL_ERRORIRQ;
>  	ace_out(ace, ACE_CTRL, val);
> 
> +	board_reset_system = ace_reset_system;
> +	the_ace_device = ace;
> +
>  	/* Print the identification */
>  	dev_info(ace->dev, "Xilinx SystemACE revision %i.%i.%i\n",
>  		 (version >> 12) & 0xf, (version >> 8) & 0x0f, version &
> 0xff);
>
************************************************************************
**********************
> IMPORTANT: The contents of this email and any attachments are
confidential. They are intended for the
> named recipient(s) only.
> If you have received this email in error, please notify the system
manager or the sender immediately
> and do
> not disclose the contents to anyone or make copies thereof.
> 
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev


This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

Hi,

On Monday, November 16, 2009, Stephen wrote:
> There are at least two other ways that you might be able to reset a
> board:
> 1) Internally through the ICAP device.
> 2) Through a GPIO connected externally to the reset logic.
> 

Unfortunately none of these is relevant for the specific board in
question (Xilinx ML510 reference system)...

> Probably it would be best to have a mechanism in the device tree which
> references the reset mechanism?

I am sorely lacking in expertise for this :(, and wouldn't even know
where to begin...  Is it possible at all to add custom information into
the device tree?  And even if yes--how will platform code bind to the
reset mechanism?

> [...] In any event, you probably don't want a driver to
> eplicitly reference the plaform code.  If you want to do it explicitly
> like this, it would better to have the plaform code reference the
driver
> mechanism.

I don't see how this can be done: if the driver is to publish some
"driver_reset_system" function to the platform code, it needs _some_
mechanism for telling this fact to the system...  And such a mechanism
won't look all that different from my callback, IMO (except it may be
slightly prettied up).
Of course, one obvious thing that must be done is move this code out of
arch/powerpc/platforms/44x/virtex.c and into (e.g.)
arch/powerpc/kernel/setup-common.c, and add some
"set_machine_restart_function" wrapper to access it more cleanly (also
defining this function as a null function when inapplicable).  If this
satisfies your standards, I can easily post an updated patch :)

-az
**********************************************************************************************
IMPORTANT: The contents of this email and any attachments are confidential. They are intended for the 
named recipient(s) only.
If you have received this email in error, please notify the system manager or the sender immediately and do 
not disclose the contents to anyone or make copies thereof.

> -----Original Message-----
> From: linuxppc-dev-bounces+stephen=neuendorffer.name@lists.ozlabs.org
[mailto:linuxppc-dev-
> bounces+stephen=neuendorffer.name@lists.ozlabs.org] On Behalf Of Alon
Ziv
> Sent: Thursday, November 19, 2009 4:57 AM
> To: Stephen Neuendorffer; linuxppc-dev
> Cc: John Linn
> Subject: RE: [PATCH/RFC] Booting Xilinx ML510 board using SystemACE
> 
> Hi,
> 
> On Monday, November 16, 2009, Stephen wrote:
> > There are at least two other ways that you might be able to reset a
> > board:
> > 1) Internally through the ICAP device.
> > 2) Through a GPIO connected externally to the reset logic.
> >
> 
> Unfortunately none of these is relevant for the specific board in
> question (Xilinx ML510 reference system)...

Well, board != system. :)  ML510 could easily include an ICAP device.

> > Probably it would be best to have a mechanism in the device tree
which
> > references the reset mechanism?
> 
> I am sorely lacking in expertise for this :(, and wouldn't even know
> where to begin...  Is it possible at all to add custom information
into
> the device tree?  And even if yes--how will platform code bind to the
> reset mechanism?
> 
> > [...] In any event, you probably don't want a driver to
> > eplicitly reference the plaform code.  If you want to do it
explicitly
> > like this, it would better to have the plaform code reference the
> driver
> > mechanism.
> 
> I don't see how this can be done: if the driver is to publish some
> "driver_reset_system" function to the platform code, it needs _some_
> mechanism for telling this fact to the system...

Think of it this way: The driver is usable on many more platforms than
just
the one you've modified.  Your addition of the hook into the platform
code
requires that that hook always be there.  It would be much better to
provide a configuration-based way of allowing the platform code to
make use of the sysace reset, if it desires.

> And such a mechanism
> won't look all that different from my callback, IMO (except it may be
> slightly prettied up).

The callback isn't the problem, it's how the callback gets registered
with
the platform code/device tree.

> Of course, one obvious thing that must be done is move this code out
of
> arch/powerpc/platforms/44x/virtex.c and into (e.g.)
> arch/powerpc/kernel/setup-common.c, and add some
> "set_machine_restart_function" wrapper to access it more cleanly (also
> defining this function as a null function when inapplicable).  If this
> satisfies your standards, I can easily post an updated patch :)

The driver isn't even powerpc specific, it could also be used on the
microblaze,
and I think you'll find alot of resistance to adding that kind of hook
to an architecture
that has just spent a bunch of time getting rid of alot of direct
binding between
platform code and drivers.  Grant, do you have a comment here?

Steve

This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

On Thu, Nov 19, 2009 at 10:32 AM, Stephen Neuendorffer
<stephen.neuendorffer@xilinx.com> wrote:
>> Of course, one obvious thing that must be done is move this code out
> of
>> arch/powerpc/platforms/44x/virtex.c and into (e.g.)
>> arch/powerpc/kernel/setup-common.c, and add some
>> "set_machine_restart_function" wrapper to access it more cleanly (also
>> defining this function as a null function when inapplicable).  If this
>> satisfies your standards, I can easily post an updated patch :)
>
> The driver isn't even powerpc specific, it could also be used on the
> microblaze,
> and I think you'll find alot of resistance to adding that kind of hook
> to an architecture
> that has just spent a bunch of time getting rid of alot of direct
> binding between
> platform code and drivers.  Grant, do you have a comment here?

Sorry for the delay, I was at a conference and got behind on email.
Yes, I've got comments.

First, the practical matter is that this is only applicable as a reset
mechanism for Xilinx PowerPC (440 or 405) and Microblaze platforms,
but the sysace device also appears on other boards, and is used to
reconfigure FPGA's that don't reset the whole system.  One of the AMCC
boards for example.

So, you want the ability to have the driver reset the system, but you
cannot break other platforms.  You also cannot call out to the driver
from platform code because the xsysace driver may get loaded as a
module.

The cleanest solution would be to call out to the xsysace driver from
platform code; but as already stated that isn't possible because the
xsysace driver may be loaded as a module.  The only way I can think of
to do that cleanly would be to register a bus notifier from platform
code to wait for an xsysace device to get probed.  But still not
exactly pretty.

For the time being, I don't think there is a good way to avoid
architecture specific hooks into the driver itself.  I recommend
adding a new pair of functions to the driver (protected by #if
defined(CONFIG_PPC)) that looks for a 'xlnx,can-reset-system'
property.  It the property is present, then simply replace the
ppc_md.restart hook in the machine definition.  Make sure to also
restore it again when the driver is removed.  Similar code would be
needed for Microblaze too, but someone else can write that.

It is important to trigger the behaviour on whether or not a property
like 'xlnx,can-restart-system' is present so that it can be chosen at
runtime whether or not the sysace should actually be used for restart
so that it doesn't break things for multiplatform.

Also, you need to add documentation about the new property to
Documentation/powerpc/dts-bindings/ before I will merge the change.
The documentation change can appear in the same patch.

It may not be the prettiest thing in the world, but in the absence of
a cross-architecture machine restart infrastructure I think it is the
best option.  At the very least it contains the changes to the driver
itself.

Cheers,
g.