[1/2] remoteproc: microblaze: Document device tree bindings

Add device tree binding documentation for the Microblaze remoteproc
on Xilinx Zynq.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

 .../bindings/remoteproc/mb_remoteproc.txt          | 46 ++++++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/mb_remoteproc.txt

--
1.8.2.3

On Mon, Jan 19, 2015 at 10:30:19AM +0000, Michal Simek wrote:
> Add device tree binding documentation for the Microblaze remoteproc
> on Xilinx Zynq.
> 
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
> 
>  .../bindings/remoteproc/mb_remoteproc.txt          | 46 ++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/mb_remoteproc.txt
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/mb_remoteproc.txt b/Documentation/devicetree/bindings/remoteproc/mb_remoteproc.txt
> new file mode 100644
> index 000000000000..a2490eac0208
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/mb_remoteproc.txt
> @@ -0,0 +1,46 @@
> +Xilinx ARM-Microblaze remoteproc driver
> +
> +This driver requires specific Zynq hardware design where Microblaze is added
> +to the programmable logic.

The binding should describe the _hardware_, not the particular driver.
Please remove any references to the 'driver'.

What hardware does this binding describe? Please focus on that.

> +Microblaze is connected with PS block via axi bus connected to PS HP port
> +to ensure access to PS DDR.
> +Communication channels are done via soft GPIO IP connected to PS block
> +and to Microblaze. There are also 2 gpio control signals reset and debug
> +which are used for resetting Microblaze.
> +
> +Required properties:
> +- compatible : Should be "xlnx,mb_remoteproc"

s/_/-/ in compatible strings please.

> +- reg : Address and length of the ddr address space
> +- bram: Phandle to bram controller which can access Microblaze BRAM
> +- bram-firmware : Microblaze BRAM bootloader name

Huh? What's this aname used for?

This looks like a filesystem name, which shouldn't be in the DT.

> +- firmware : Default firmware name which can be override by
> +	     "firmware" module parameter

Likewise on all points.

> +- reset : Gpio phandle which reset Microblaze remoteproc
> +- debug : Gpio phandle which setup Microblaze to debug state
> +- ipino : Gpio phandle for Microblaze to ARM communication
> +- vring0 : Gpio phandle for ARM to Microblaze communication vring 0
> +- vring1 : Gpio phandle for ARM to Microblaze communication vring 1

s/phandle/ / -- there's a gpio-specifier too, but we may as well just
ignore the specifics of the format and leave that to the GPIO bindings.

These should all have a "-gpio" suffix, too.

> +
> +Microblaze SoC can be also connected to the PS block via a axi bus.
> +That's why there is the option to allocate interrupts for Microblaze use only.
> +The driver will allocate interrupts to itself and Microblaze sw has to ensure
> +that interrupts are properly enabled and handled by Microblaze interrupt
> +controller.

I'm not sure I follow? What is this for?

Thanks,
Mark.

> +
> +Optional properties:
> + - interrupts : Interrupt mapping for remoteproc
> + - interrupt-parent : Phandle for the interrupt controller
> +
> +Example:
> +mb_remoteproc@800000 {
> +	compatible = "xlnx,mb_remoteproc";
> +	reg = < 0x8000000 0x8000000 >;
> +	bram = <&axi_bram_ctrl_0>;
> +	bram-firmware = "mb.bin";
> +	firmware = "image.elf";
> +	reset = <&zynq_gpio_reset 1 0>;
> +	debug = <&zynq_gpio_reset 0 0>;
> +	ipino = <&zynq_gpio_vring 0 0>;
> +	vring0 = <&zynq_gpio_vring 1 0>;
> +	vring1 = <&zynq_gpio_vring 2 0>;
> +};
> --
> 1.8.2.3
> 


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Mark,

On 01/19/2015 12:04 PM, Mark Rutland wrote:
> On Mon, Jan 19, 2015 at 10:30:19AM +0000, Michal Simek wrote:
>> Add device tree binding documentation for the Microblaze remoteproc
>> on Xilinx Zynq.
>>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> ---
>>
>>  .../bindings/remoteproc/mb_remoteproc.txt          | 46 ++++++++++++++++++++++
>>  1 file changed, 46 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/remoteproc/mb_remoteproc.txt
>>
>> diff --git a/Documentation/devicetree/bindings/remoteproc/mb_remoteproc.txt b/Documentation/devicetree/bindings/remoteproc/mb_remoteproc.txt
>> new file mode 100644
>> index 000000000000..a2490eac0208
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/remoteproc/mb_remoteproc.txt
>> @@ -0,0 +1,46 @@
>> +Xilinx ARM-Microblaze remoteproc driver
>> +
>> +This driver requires specific Zynq hardware design where Microblaze is added
>> +to the programmable logic.
> 
> The binding should describe the _hardware_, not the particular driver.
> Please remove any references to the 'driver'.
> 
> What hardware does this binding describe? Please focus on that.

It is Xilinx Zynq with Microblaze in programmable logic which is remote cpu
where Linux on Zynq can load software on Microblaze and communicate with it.

> 
>> +Microblaze is connected with PS block via axi bus connected to PS HP port
>> +to ensure access to PS DDR.
>> +Communication channels are done via soft GPIO IP connected to PS block
>> +and to Microblaze. There are also 2 gpio control signals reset and debug
>> +which are used for resetting Microblaze.
>> +
>> +Required properties:
>> +- compatible : Should be "xlnx,mb_remoteproc"
> 
> s/_/-/ in compatible strings please.

no problem.

> 
>> +- reg : Address and length of the ddr address space
>> +- bram: Phandle to bram controller which can access Microblaze BRAM
>> +- bram-firmware : Microblaze BRAM bootloader name
> 
> Huh? What's this aname used for?
> 
> This looks like a filesystem name, which shouldn't be in the DT.

It is firmware name called via firmware interface. It doesn't need to be on filesystem.
What's the way how to describe default firmware?
Or this should be out of binding at all and just setup default firmware in the driver
and add options to change it via module parameters?

> 
>> +- firmware : Default firmware name which can be override by
>> +	     "firmware" module parameter
> 
> Likewise on all points.
> 
>> +- reset : Gpio phandle which reset Microblaze remoteproc
>> +- debug : Gpio phandle which setup Microblaze to debug state
>> +- ipino : Gpio phandle for Microblaze to ARM communication
>> +- vring0 : Gpio phandle for ARM to Microblaze communication vring 0
>> +- vring1 : Gpio phandle for ARM to Microblaze communication vring 1
> 
> s/phandle/ / -- there's a gpio-specifier too, but we may as well just
> ignore the specifics of the format and leave that to the GPIO bindings.
> 
> These should all have a "-gpio" suffix, too.

ok. Will add.

> 
>> +
>> +Microblaze SoC can be also connected to the PS block via a axi bus.
>> +That's why there is the option to allocate interrupts for Microblaze use only.
>> +The driver will allocate interrupts to itself and Microblaze sw has to ensure
>> +that interrupts are properly enabled and handled by Microblaze interrupt
>> +controller.
> 
> I'm not sure I follow? What is this for?

Microblaze cpu soft core is in programmable logic on Zynq. Zynq is Cortex-a9 dual core.
This binding describes how microblaze is added to PL and how Linux on a9 communicates with
it Microblaze. Interrupts from all IPs on the system can be connected to arm gic and also
to Microblaze SoC interrupt controller that's why this driver provides the option to allocate
all shared interrupts and for example count how many times this interrupt was triggered.
The second use is to monitor if interrupts were properly forwarded to correct CPU and
restrict others drivers not to allocate them.

Thanks,
Michal

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 01/19/2015 04:30 AM, Michal Simek wrote:
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index 5e343bab9458..3955f42e9e9c 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -64,4 +64,15 @@ config DA8XX_REMOTEPROC
>  	  It's safe to say n here if you're not interested in multimedia
>  	  offloading.
> 
> +config MB_REMOTEPROC
> +	tristate "Support Microblaze remoteproc"
> +	depends on ARCH_ZYNQ && !DEBUG_SG
> +	select GPIO_XILINX
> +	select REMOTEPROC
> +	select RPMSG
> +	default m
> +	help
> +	  Say y here to support Xilinx Microblaze remote processors
> +	  on the Xilinx Zynq.
> +
>  endmenu

The dependency on !DEBUG_SG seems unusual, and worth a comment if it's
truly needed.  And the module should be disabled by default, no?

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 01/22/2015 05:53 PM, Nathan Lynch wrote:
> On 01/19/2015 04:30 AM, Michal Simek wrote:
>> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
>> index 5e343bab9458..3955f42e9e9c 100644
>> --- a/drivers/remoteproc/Kconfig
>> +++ b/drivers/remoteproc/Kconfig
>> @@ -64,4 +64,15 @@ config DA8XX_REMOTEPROC
>>  	  It's safe to say n here if you're not interested in multimedia
>>  	  offloading.
>>
>> +config MB_REMOTEPROC
>> +	tristate "Support Microblaze remoteproc"
>> +	depends on ARCH_ZYNQ && !DEBUG_SG
>> +	select GPIO_XILINX
>> +	select REMOTEPROC
>> +	select RPMSG
>> +	default m
>> +	help
>> +	  Say y here to support Xilinx Microblaze remote processors
>> +	  on the Xilinx Zynq.
>> +
>>  endmenu
> 
> The dependency on !DEBUG_SG seems unusual, and worth a comment if it's
> truly needed.  And the module should be disabled by default, no?

Right - I will remove this dependency. It is not needed now.

Using is like a module is good usecase because you can reload SW on remote cpu.

Thanks,
Michal


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html