Message ID | 20200211142614.13567-1-paul@crapouillou.net |
---|---|
State | Not Applicable, archived |
Headers | show |
Series | [v5,1/5] dt-bindings: Document JZ47xx VPU auxiliary processor | expand |
Context | Check | Description |
---|---|---|
robh/checkpatch | success | |
robh/dt-meta-schema | success |
On Tue, Feb 11, 2020 at 11:26:09AM -0300, Paul Cercueil wrote: > Inside the Video Processing Unit (VPU) of the recent JZ47xx SoCs from > Ingenic is a second Xburst MIPS CPU very similar to the main core. > This document describes the devicetree bindings for this auxiliary > processor. > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- > > Notes: > v2: Update TCSM0 address in example > v3: Change node name to 'video-decoder' > v4: Convert to YAML. I didn't add Rob's Ack on v3 because of that (sorry Rob) > v5: - Fix 'reg' not in <addr, len> pairs > - Add missing include to devicetree example > > .../bindings/remoteproc/ingenic,vpu.yaml | 77 +++++++++++++++++++ > 1 file changed, 77 insertions(+) > create mode 100644 Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml Reviewed-by: Rob Herring <robh@kernel.org>
On Tue, 11 Feb 2020 11:26:09 -0300, Paul Cercueil wrote: > Inside the Video Processing Unit (VPU) of the recent JZ47xx SoCs from > Ingenic is a second Xburst MIPS CPU very similar to the main core. > This document describes the devicetree bindings for this auxiliary > processor. > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- > > Notes: > v2: Update TCSM0 address in example > v3: Change node name to 'video-decoder' > v4: Convert to YAML. I didn't add Rob's Ack on v3 because of that (sorry Rob) > v5: - Fix 'reg' not in <addr, len> pairs > - Add missing include to devicetree example > > .../bindings/remoteproc/ingenic,vpu.yaml | 77 +++++++++++++++++++ > 1 file changed, 77 insertions(+) > create mode 100644 Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml > Please add Acked-by/Reviewed-by tags when posting new versions. However, there's no need to repost patches *only* to add the tags. The upstream maintainer will do that for acks received on the version they apply. If a tag was not added on purpose, please state why and what changed.
On Tue, Feb 11, 2020 at 8:07 PM Rob Herring <robh@kernel.org> wrote: > > On Tue, 11 Feb 2020 11:26:09 -0300, Paul Cercueil wrote: > > Inside the Video Processing Unit (VPU) of the recent JZ47xx SoCs from > > Ingenic is a second Xburst MIPS CPU very similar to the main core. > > This document describes the devicetree bindings for this auxiliary > > processor. > > > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > > --- > > > > Notes: > > v2: Update TCSM0 address in example > > v3: Change node name to 'video-decoder' > > v4: Convert to YAML. I didn't add Rob's Ack on v3 because of that (sorry Rob) > > v5: - Fix 'reg' not in <addr, len> pairs > > - Add missing include to devicetree example > > > > .../bindings/remoteproc/ingenic,vpu.yaml | 77 +++++++++++++++++++ > > 1 file changed, 77 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml > > > > Please add Acked-by/Reviewed-by tags when posting new versions. However, > there's no need to repost patches *only* to add the tags. The upstream > maintainer will do that for acks received on the version they apply. > > If a tag was not added on purpose, please state why and what changed. Disregard this, you did say why.
Hi Paul, I still wonder about the use of pm_runtime mechanism as a more generic alternative... Else just a minor remark inline. On 2/11/20 3:26 PM, Paul Cercueil wrote: > The .prepare() callback is called before the firmware is loaded to > memory. This is useful for instance in the case where some setup is > required for the memory to be accessible. > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- > > Notes: > v2-v4: No change > v5: Move calls to prepare/unprepare to rproc_fw_boot/rproc_shutdown > > drivers/remoteproc/remoteproc_core.c | 16 +++++++++++++++- > include/linux/remoteproc.h | 4 ++++ > 2 files changed, 19 insertions(+), 1 deletion(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index fe5c7a2f9767..022b927e176b 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -1373,6 +1373,14 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw) > > dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size); > > + if (rproc->ops->prepare) { > + ret = rproc->ops->prepare(rproc); > + if (ret) { > + dev_err(dev, "Failed to prepare rproc: %d\n", ret); > + return ret; > + } > + } > + > /* > * if enabling an IOMMU isn't relevant for this rproc, this is > * just a nop > @@ -1380,7 +1388,7 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw) > ret = rproc_enable_iommu(rproc); > if (ret) { > dev_err(dev, "can't enable iommu: %d\n", ret); > - return ret; > + goto unprepare_rproc; > } > > rproc->bootaddr = rproc_get_boot_addr(rproc, fw); > @@ -1424,6 +1432,9 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw) > rproc->table_ptr = NULL; > disable_iommu: > rproc_disable_iommu(rproc); > +unprepare_rproc: > + if (rproc->ops->unprepare) > + rproc->ops->unprepare(rproc); > return ret; > } > > @@ -1823,6 +1834,9 @@ void rproc_shutdown(struct rproc *rproc) > > rproc_disable_iommu(rproc); > > + if (rproc->ops->unprepare) > + rproc->ops->unprepare(rproc); > + > /* Free the copy of the resource table */ > kfree(rproc->cached_table); > rproc->cached_table = NULL; > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h > index 5f201f0c86c3..a6272d1ba384 100644 > --- a/include/linux/remoteproc.h > +++ b/include/linux/remoteproc.h > @@ -355,6 +355,8 @@ enum rsc_handling_status { > > /** > * struct rproc_ops - platform-specific device handlers > + * @prepare: prepare the device for power up (before the firmware is loaded) > + * @unprepare: unprepare the device after it is stopped Would be nice here to precise that these functions are optional you can look at rproc_ops struct for example. Regards, Arnaud > * @start: power on the device and boot it > * @stop: power off the device > * @kick: kick a virtqueue (virtqueue id given as a parameter) > @@ -371,6 +373,8 @@ enum rsc_handling_status { > * @get_boot_addr: get boot address to entry point specified in firmware > */ > struct rproc_ops { > + int (*prepare)(struct rproc *rproc); > + void (*unprepare)(struct rproc *rproc); > int (*start)(struct rproc *rproc); > int (*stop)(struct rproc *rproc); > void (*kick)(struct rproc *rproc, int vqid); >
Hi Arnaud, Le mar., févr. 18, 2020 at 17:31, Arnaud POULIQUEN <arnaud.pouliquen@st.com> a écrit : > Hi Paul, > > I still wonder about the use of pm_runtime mechanism as a more > generic alternative... The use of pm_runtime is perfect if CONFIG_PM is enabled, but otherwise it's a bit cumbersome, as the clocks must be enabled in the probe. -Paul > Else just a minor remark inline. > > On 2/11/20 3:26 PM, Paul Cercueil wrote: >> The .prepare() callback is called before the firmware is loaded to >> memory. This is useful for instance in the case where some setup is >> required for the memory to be accessible. >> >> Signed-off-by: Paul Cercueil <paul@crapouillou.net> >> --- >> >> Notes: >> v2-v4: No change >> v5: Move calls to prepare/unprepare to >> rproc_fw_boot/rproc_shutdown >> >> drivers/remoteproc/remoteproc_core.c | 16 +++++++++++++++- >> include/linux/remoteproc.h | 4 ++++ >> 2 files changed, 19 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/remoteproc/remoteproc_core.c >> b/drivers/remoteproc/remoteproc_core.c >> index fe5c7a2f9767..022b927e176b 100644 >> --- a/drivers/remoteproc/remoteproc_core.c >> +++ b/drivers/remoteproc/remoteproc_core.c >> @@ -1373,6 +1373,14 @@ static int rproc_fw_boot(struct rproc >> *rproc, const struct firmware *fw) >> >> dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size); >> >> + if (rproc->ops->prepare) { >> + ret = rproc->ops->prepare(rproc); >> + if (ret) { >> + dev_err(dev, "Failed to prepare rproc: %d\n", ret); >> + return ret; >> + } >> + } >> + >> /* >> * if enabling an IOMMU isn't relevant for this rproc, this is >> * just a nop >> @@ -1380,7 +1388,7 @@ static int rproc_fw_boot(struct rproc *rproc, >> const struct firmware *fw) >> ret = rproc_enable_iommu(rproc); >> if (ret) { >> dev_err(dev, "can't enable iommu: %d\n", ret); >> - return ret; >> + goto unprepare_rproc; >> } >> >> rproc->bootaddr = rproc_get_boot_addr(rproc, fw); >> @@ -1424,6 +1432,9 @@ static int rproc_fw_boot(struct rproc *rproc, >> const struct firmware *fw) >> rproc->table_ptr = NULL; >> disable_iommu: >> rproc_disable_iommu(rproc); >> +unprepare_rproc: >> + if (rproc->ops->unprepare) >> + rproc->ops->unprepare(rproc); >> return ret; >> } >> >> @@ -1823,6 +1834,9 @@ void rproc_shutdown(struct rproc *rproc) >> >> rproc_disable_iommu(rproc); >> >> + if (rproc->ops->unprepare) >> + rproc->ops->unprepare(rproc); >> + >> /* Free the copy of the resource table */ >> kfree(rproc->cached_table); >> rproc->cached_table = NULL; >> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h >> index 5f201f0c86c3..a6272d1ba384 100644 >> --- a/include/linux/remoteproc.h >> +++ b/include/linux/remoteproc.h >> @@ -355,6 +355,8 @@ enum rsc_handling_status { >> >> /** >> * struct rproc_ops - platform-specific device handlers >> + * @prepare: prepare the device for power up (before the firmware >> is loaded) >> + * @unprepare: unprepare the device after it is stopped > > Would be nice here to precise that these functions are optional > you can look at rproc_ops struct for example. > > Regards, > Arnaud > >> * @start: power on the device and boot it >> * @stop: power off the device >> * @kick: kick a virtqueue (virtqueue id given as a parameter) >> @@ -371,6 +373,8 @@ enum rsc_handling_status { >> * @get_boot_addr: get boot address to entry point specified in >> firmware >> */ >> struct rproc_ops { >> + int (*prepare)(struct rproc *rproc); >> + void (*unprepare)(struct rproc *rproc); >> int (*start)(struct rproc *rproc); >> int (*stop)(struct rproc *rproc); >> void (*kick)(struct rproc *rproc, int vqid); >>
diff --git a/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml b/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml new file mode 100644 index 000000000000..c019f9fbe916 --- /dev/null +++ b/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml @@ -0,0 +1,77 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: "http://devicetree.org/schemas/remoteproc/ingenic,vpu.yaml#" +$schema: "http://devicetree.org/meta-schemas/core.yaml#" + +title: Ingenic Video Processing Unit bindings + +description: + Inside the Video Processing Unit (VPU) of the recent JZ47xx SoCs from + Ingenic is a second Xburst MIPS CPU very similar to the main core. + This document describes the devicetree bindings for this auxiliary + processor. + +maintainers: + - Paul Cercueil <paul@crapouillou.net> + +properties: + compatible: + const: ingenic,jz4770-vpu-rproc + + reg: + items: + - description: aux registers + - description: tcsm0 registers + - description: tcsm1 registers + - description: sram registers + + reg-names: + items: + - const: aux + - const: tcsm0 + - const: tcsm1 + - const: sram + + clocks: + items: + - description: aux clock + - description: vpu clock + + clock-names: + items: + - const: aux + - const: vpu + + interrupts: + description: VPU hardware interrupt + +required: + - compatible + - reg + - reg-names + - clocks + - clock-names + - interrupts + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/jz4770-cgu.h> + + vpu: video-decoder@132a0000 { + compatible = "ingenic,jz4770-vpu-rproc"; + + reg = <0x132a0000 0x20>, /* AUX */ + <0x132b0000 0x4000>, /* TCSM0 */ + <0x132c0000 0xc000>, /* TCSM1 */ + <0x132f0000 0x7000>; /* SRAM */ + reg-names = "aux", "tcsm0", "tcsm1", "sram"; + + clocks = <&cgu JZ4770_CLK_AUX>, <&cgu JZ4770_CLK_VPU>; + clock-names = "aux", "vpu"; + + interrupt-parent = <&cpuintc>; + interrupts = <3>; + };
Inside the Video Processing Unit (VPU) of the recent JZ47xx SoCs from Ingenic is a second Xburst MIPS CPU very similar to the main core. This document describes the devicetree bindings for this auxiliary processor. Signed-off-by: Paul Cercueil <paul@crapouillou.net> --- Notes: v2: Update TCSM0 address in example v3: Change node name to 'video-decoder' v4: Convert to YAML. I didn't add Rob's Ack on v3 because of that (sorry Rob) v5: - Fix 'reg' not in <addr, len> pairs - Add missing include to devicetree example .../bindings/remoteproc/ingenic,vpu.yaml | 77 +++++++++++++++++++ 1 file changed, 77 insertions(+) create mode 100644 Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml