* Re: [PATCH v3 4/4] dt-bindings: drm: bridge: adi,adv7511.txt: convert to yaml
From: Laurent Pinchart @ 2020-06-03 0:03 UTC (permalink / raw)
To: Ricardo Cañuelo
Cc: kernel, devicetree, linux-arm-kernel, robh+dt, xuwei5,
michal.simek, mcoquelin.stm32, marex
In-Reply-To: <20200601063308.13045-5-ricardo.canuelo@collabora.com>
Hi Ricardo,
Thank you for the patch.
On Mon, Jun 01, 2020 at 08:33:08AM +0200, Ricardo Cañuelo wrote:
> Convert the ADV7511/11w/13/33/35 DT bindings to json-schema. The
> original binding has been split into two files: adi,adv7511.yaml for
> ADV7511/11W/13 and adi,adv7533.yaml for ADV7533/35.
>
> Signed-off-by: Ricardo Cañuelo <ricardo.canuelo@collabora.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Great work :-)
> ---
> .../bindings/display/bridge/adi,adv7511.txt | 143 -----------
> .../bindings/display/bridge/adi,adv7511.yaml | 231 ++++++++++++++++++
> .../bindings/display/bridge/adi,adv7533.yaml | 175 +++++++++++++
> 3 files changed, 406 insertions(+), 143 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> create mode 100644 Documentation/devicetree/bindings/display/bridge/adi,adv7511.yaml
> create mode 100644 Documentation/devicetree/bindings/display/bridge/adi,adv7533.yaml
>
> diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> deleted file mode 100644
> index 659523f538bf..000000000000
> --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> +++ /dev/null
> @@ -1,143 +0,0 @@
> -Analog Devices ADV7511(W)/13/33/35 HDMI Encoders
> -------------------------------------------------
> -
> -The ADV7511, ADV7511W, ADV7513, ADV7533 and ADV7535 are HDMI audio and video
> -transmitters compatible with HDMI 1.4 and DVI 1.0. They support color space
> -conversion, S/PDIF, CEC and HDCP. ADV7533/5 supports the DSI interface for input
> -pixels, while the others support RGB interface.
> -
> -Required properties:
> -
> -- compatible: Should be one of:
> - "adi,adv7511"
> - "adi,adv7511w"
> - "adi,adv7513"
> - "adi,adv7533"
> - "adi,adv7535"
> -
> -- reg: I2C slave addresses
> - The ADV7511 internal registers are split into four pages exposed through
> - different I2C addresses, creating four register maps. Each map has it own
> - I2C address and acts as a standard slave device on the I2C bus. The main
> - address is mandatory, others are optional and revert to defaults if not
> - specified.
> -
> -
> -The ADV7511 supports a large number of input data formats that differ by their
> -color depth, color format, clock mode, bit justification and random
> -arrangement of components on the data bus. The combination of the following
> -properties describe the input and map directly to the video input tables of the
> -ADV7511 datasheet that document all the supported combinations.
> -
> -- adi,input-depth: Number of bits per color component at the input (8, 10 or
> - 12).
> -- adi,input-colorspace: The input color space, one of "rgb", "yuv422" or
> - "yuv444".
> -- adi,input-clock: The input clock type, one of "1x" (one clock cycle per
> - pixel), "2x" (two clock cycles per pixel), "ddr" (one clock cycle per pixel,
> - data driven on both edges).
> -
> -The following input format properties are required except in "rgb 1x" and
> -"yuv444 1x" modes, in which case they must not be specified.
> -
> -- adi,input-style: The input components arrangement variant (1, 2 or 3), as
> - listed in the input format tables in the datasheet.
> -- adi,input-justification: The input bit justification ("left", "evenly",
> - "right").
> -
> -- avdd-supply: A 1.8V supply that powers up the AVDD pin on the chip.
> -- dvdd-supply: A 1.8V supply that powers up the DVDD pin on the chip.
> -- pvdd-supply: A 1.8V supply that powers up the PVDD pin on the chip.
> -- dvdd-3v-supply: A 3.3V supply that powers up the pin called DVDD_3V
> - on the chip.
> -- bgvdd-supply: A 1.8V supply that powers up the BGVDD pin. This is
> - needed only for ADV7511.
> -
> -The following properties are required for ADV7533 and ADV7535:
> -
> -- adi,dsi-lanes: Number of DSI data lanes connected to the DSI host. It should
> - be one of 1, 2, 3 or 4.
> -- a2vdd-supply: 1.8V supply that powers up the A2VDD pin on the chip.
> -- v3p3-supply: A 3.3V supply that powers up the V3P3 pin on the chip.
> -- v1p2-supply: A supply that powers up the V1P2 pin on the chip. It can be
> - either 1.2V or 1.8V for ADV7533 but only 1.8V for ADV7535.
> -
> -Optional properties:
> -
> -- interrupts: Specifier for the ADV7511 interrupt
> -- pd-gpios: Specifier for the GPIO connected to the power down signal
> -
> -- adi,clock-delay: Video data clock delay relative to the pixel clock, in ps
> - (-1200 ps .. 1600 ps). Defaults to no delay.
> -- adi,embedded-sync: The input uses synchronization signals embedded in the
> - data stream (similar to BT.656). Defaults to separate H/V synchronization
> - signals.
> -- adi,disable-timing-generator: Only for ADV7533 and ADV7535. Disables the
> - internal timing generator. The chip will rely on the sync signals in the
> - DSI data lanes, rather than generate its own timings for HDMI output.
> -- clocks: from common clock binding: reference to the CEC clock.
> -- clock-names: from common clock binding: must be "cec".
> -- reg-names : Names of maps with programmable addresses.
> - It can contain any map needing a non-default address.
> - Possible maps names are : "main", "edid", "cec", "packet"
> -
> -Required nodes:
> -
> -The ADV7511 has two video ports. Their connections are modelled using the OF
> -graph bindings specified in Documentation/devicetree/bindings/graph.txt.
> -
> -- Video port 0 for the RGB, YUV or DSI input. In the case of ADV7533/5, the
> - remote endpoint phandle should be a reference to a valid mipi_dsi_host device
> - node.
> -- Video port 1 for the HDMI output
> -- Audio port 2 for the HDMI audio input
> -
> -
> -Example
> --------
> -
> - adv7511w: hdmi@39 {
> - compatible = "adi,adv7511w";
> - /*
> - * The EDID page will be accessible on address 0x66 on the I2C
> - * bus. All other maps continue to use their default addresses.
> - */
> - reg = <0x39>, <0x66>;
> - reg-names = "main", "edid";
> - interrupt-parent = <&gpio3>;
> - interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
> - clocks = <&cec_clock>;
> - clock-names = "cec";
> -
> - adi,input-depth = <8>;
> - adi,input-colorspace = "rgb";
> - adi,input-clock = "1x";
> - adi,input-style = <1>;
> - adi,input-justification = "evenly";
> -
> - ports {
> - #address-cells = <1>;
> - #size-cells = <0>;
> -
> - port@0 {
> - reg = <0>;
> - adv7511w_in: endpoint {
> - remote-endpoint = <&dpi_out>;
> - };
> - };
> -
> - port@1 {
> - reg = <1>;
> - adv7511_out: endpoint {
> - remote-endpoint = <&hdmi_connector_in>;
> - };
> - };
> -
> - port@2 {
> - reg = <2>;
> - codec_endpoint: endpoint {
> - remote-endpoint = <&i2s0_cpu_endpoint>;
> - };
> - };
> - };
> - };
> diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.yaml b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.yaml
> new file mode 100644
> index 000000000000..71b344e812dd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.yaml
> @@ -0,0 +1,231 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/bridge/adi,adv7511.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices ADV7511/11W/13 HDMI Encoders
> +
> +maintainers:
> + - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> +
> +description: |
> + The ADV7511, ADV7511W and ADV7513 are HDMI audio and video
> + transmitters compatible with HDMI 1.4 and DVI 1.0. They support color
> + space conversion, S/PDIF, CEC and HDCP. The transmitter input is
> + parallel RGB or YUV data.
> +
> +properties:
> + compatible:
> + enum:
> + - adi,adv7511
> + - adi,adv7511w
> + - adi,adv7513
> +
> + reg:
> + description: |
> + I2C slave addresses.
> +
> + The ADV7511/11W/13 internal registers are split into four pages
> + exposed through different I2C addresses, creating four register
> + maps. Each map has it own I2C address and acts as a standard slave
> + device on the I2C bus. The main address is mandatory, others are
> + optional and revert to defaults if not specified.
> + minItems: 1
> + maxItems: 4
> +
> + reg-names:
> + description:
> + Names of maps with programmable addresses. It can contain any map
> + needing a non-default address.
> + minItems: 1
> + items:
> + - const: main
> + - const: edid
> + - const: cec
> + - const: packet
> +
> + clocks:
> + description: Reference to the CEC clock.
> + maxItems: 1
> +
> + clock-names:
> + const: cec
> +
> + interrupts:
> + maxItems: 1
> +
> + pd-gpios:
> + description: GPIO connected to the power down signal.
> + maxItems: 1
> +
> + avdd-supply:
> + description: A 1.8V supply that powers up the AVDD pin.
> +
> + dvdd-supply:
> + description: A 1.8V supply that powers up the DVDD pin.
> +
> + pvdd-supply:
> + description: A 1.8V supply that powers up the PVDD pin.
> +
> + dvdd-3v-supply:
> + description: A 3.3V supply that powers up the DVDD_3V pin.
> +
> + bgvdd-supply:
> + description: A 1.8V supply that powers up the BGVDD pin.
> +
> + adi,input-depth:
> + description: Number of bits per color component at the input.
> + allOf:
> + - $ref: /schemas/types.yaml#/definitions/uint32
> + - enum: [ 8, 10, 12 ]
> +
> + adi,input-colorspace:
> + description: Input color space.
> + enum: [ rgb, yuv422, yuv444 ]
> +
> + adi,input-clock:
> + description: |
> + Input clock type.
> + "1x": one clock cycle per pixel
> + "2x": two clock cycles per pixel
> + "dd": one clock cycle per pixel, data driven on both edges
> + enum: [ 1x, 2x, dd ]
> +
> + adi,clock-delay:
> + description:
> + Video data clock delay relative to the pixel clock, in ps
> + (-1200ps .. 1600 ps).
> + $ref: /schemas/types.yaml#/definitions/uint32
> + default: 0
> +
> + adi,embedded-sync:
> + description:
> + If defined, the input uses synchronization signals embedded in the
> + data stream (similar to BT.656).
> + type: boolean
> +
> + adi,input-style:
> + description:
> + Input components arrangement variant as listed in the input
> + format tables in the datasheet.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [ 1, 2, 3 ]
> +
> + adi,input-justification:
> + description: Input bit justification.
> + enum: [ left, evenly, right ]
> +
> + ports:
> + description:
> + The ADV7511(W)/13 has two video ports and one audio port. This node
> + models their connections as documented in
> + Documentation/devicetree/bindings/media/video-interfaces.txt
> + Documentation/devicetree/bindings/graph.txt
> + type: object
> + properties:
> + port@0:
> + description: Video port for the RGB or YUV input.
> + type: object
> +
> + port@1:
> + description: Video port for the HDMI output.
> + type: object
> +
> + port@2:
> + description: Audio port for the HDMI output.
> + type: object
> +
> +# adi,input-colorspace and adi,input-clock are required except in
> +# "rgb 1x" and "yuv444 1x" modes, in which case they must not be
> +# specified.
> +if:
> + not:
> + properties:
> + adi,input-colorspace:
> + contains:
> + enum: [ rgb, yuv444 ]
> + adi,input-clock:
> + contains:
> + const: 1x
> +
> +then:
> + required:
> + - adi,input-style
> + - adi,input-justification
> +
> +else:
> + properties:
> + adi,input-style: false
> + adi,input-justification: false
> +
> +
> +required:
> + - compatible
> + - reg
> + - ports
> + - adi,input-depth
> + - adi,input-colorspace
> + - adi,input-clock
> + - avdd-supply
> + - dvdd-supply
> + - pvdd-supply
> + - dvdd-3v-supply
> + - bgvdd-supply
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> +
> + adv7511w: hdmi@39 {
> + compatible = "adi,adv7511w";
> + /*
> + * The EDID page will be accessible on address 0x66 on the I2C
> + * bus. All other maps continue to use their default addresses.
> + */
> + reg = <0x39>, <0x66>;
> + reg-names = "main", "edid";
> + interrupt-parent = <&gpio3>;
> + interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
> + clocks = <&cec_clock>;
> + clock-names = "cec";
> + avdd-supply = <&v1v8>;
> + dvdd-supply = <&v1v8>;
> + pvdd-supply = <&v1v8>;
> + dvdd-3v-supply = <&v3v3>;
> + bgvdd-supply = <&v1v8>;
> +
> + adi,input-depth = <8>;
> + adi,input-colorspace = "yuv422";
> + adi,input-clock = "1x";
> +
> + adi,input-style = <3>;
> + adi,input-justification = "right";
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> + adv7511w_in: endpoint {
> + remote-endpoint = <&dpi_out>;
> + };
> + };
> +
> + port@1 {
> + reg = <1>;
> + adv7511_out: endpoint {
> + remote-endpoint = <&hdmi_connector_in>;
> + };
> + };
> +
> + port@2 {
> + reg = <2>;
> + codec_endpoint: endpoint {
> + remote-endpoint = <&i2s0_cpu_endpoint>;
> + };
> + };
> + };
> + };
> +
> +...
> diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7533.yaml b/Documentation/devicetree/bindings/display/bridge/adi,adv7533.yaml
> new file mode 100644
> index 000000000000..18761f49e5fe
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7533.yaml
> @@ -0,0 +1,175 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/bridge/adi,adv7533.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices ADV7533/35 HDMI Encoders
> +
> +maintainers:
> + - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> +
> +description: |
> + The ADV7533 and ADV7535 are HDMI audio and video transmitters
> + compatible with HDMI 1.4 and DVI 1.0. They support color space
> + conversion, S/PDIF, CEC and HDCP. The transmitter input is MIPI DSI.
> +
> +properties:
> + compatible:
> + enum:
> + - adi,adv7533
> + - adi,adv7535
> +
> + reg:
> + description: |
> + I2C slave addresses.
> +
> + The ADV7533/35 internal registers are split into four pages
> + exposed through different I2C addresses, creating four register
> + maps. Each map has it own I2C address and acts as a standard slave
> + device on the I2C bus. The main address is mandatory, others are
> + optional and revert to defaults if not specified.
> + minItems: 1
> + maxItems: 4
> +
> + reg-names:
> + description:
> + Names of maps with programmable addresses. It can contain any map
> + needing a non-default address.
> + minItems: 1
> + items:
> + - const: main
> + - const: edid
> + - const: cec
> + - const: packet
> +
> + clocks:
> + description: Reference to the CEC clock.
> + maxItems: 1
> +
> + clock-names:
> + const: cec
> +
> + interrupts:
> + maxItems: 1
> +
> + pd-gpios:
> + description: GPIO connected to the power down signal.
> + maxItems: 1
> +
> + avdd-supply:
> + description: A 1.8V supply that powers up the AVDD pin.
> +
> + dvdd-supply:
> + description: A 1.8V supply that powers up the DVDD pin.
> +
> + pvdd-supply:
> + description: A 1.8V supply that powers up the PVDD pin.
> +
> + a2vdd-supply:
> + description: A 1.8V supply that powers up the A2VDD pin.
> +
> + v3p3-supply:
> + description: A 3.3V supply that powers up the V3P3 pin.
> +
> + v1p2-supply:
> + description:
> + A supply that powers up the V1P2 pin. It can be either 1.2V
> + or 1.8V for ADV7533 but only 1.8V for ADV7535.
> +
> + adi,disable-timing-generator:
> + description:
> + Disables the internal timing generator. The chip will rely on the
> + sync signals in the DSI data lanes, rather than generating its own
> + timings for HDMI output.
> + type: boolean
> +
> + adi,dsi-lanes:
> + description: Number of DSI data lanes connected to the DSI host.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [ 1, 2, 3, 4 ]
> +
> + ports:
> + description:
> + The ADV7533/35 has two video ports and one audio port. This node
> + models their connections as documented in
> + Documentation/devicetree/bindings/media/video-interfaces.txt
> + Documentation/devicetree/bindings/graph.txt
> + type: object
> + properties:
> + port@0:
> + description:
> + Video port for the DSI input. The remote endpoint phandle
> + should be a reference to a valid mipi_dsi_host_device.
> + type: object
> +
> + port@1:
> + description: Video port for the HDMI output.
> + type: object
> +
> + port@2:
> + description: Audio port for the HDMI output.
> + type: object
> +
> +required:
> + - compatible
> + - reg
> + - ports
> + - adi,dsi-lanes
> + - avdd-supply
> + - dvdd-supply
> + - pvdd-supply
> + - a2vdd-supply
> + - v3p3-supply
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> +
> + adv7533: hdmi@39 {
> + compatible = "adi,adv7533";
> + /*
> + * The EDID page will be accessible on address 0x66 on the I2C
> + * bus. All other maps continue to use their default addresses.
> + */
> + reg = <0x39>, <0x66>;
> + reg-names = "main", "edid";
> + interrupt-parent = <&gpio3>;
> + interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
> + clocks = <&cec_clock>;
> + clock-names = "cec";
> + adi,dsi-lanes = <4>;
> + avdd-supply = <&v1v8>;
> + dvdd-supply = <&v1v8>;
> + pvdd-supply = <&v1v8>;
> + a2vdd-supply = <&v1v8>;
> + v3p3-supply = <&v3v3>;
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> + adv7533_in: endpoint {
> + remote-endpoint = <&dsi_out>;
> + };
> + };
> +
> + port@1 {
> + reg = <1>;
> + adv7533_out: endpoint {
> + remote-endpoint = <&hdmi_connector_in>;
> + };
> + };
> +
> + port@2 {
> + reg = <2>;
> + codec_endpoint: endpoint {
> + remote-endpoint = <&i2s0_cpu_endpoint>;
> + };
> + };
> + };
> + };
> +
> +...
> --
> 2.18.0
>
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH v2 1/2] media: dt-bindings: media: xilinx: Add Xilinx UHD-SDI Receiver Subsystem
From: Laurent Pinchart @ 2020-06-03 1:13 UTC (permalink / raw)
To: Vishal Sagar
Cc: Hyun Kwon, mchehab@kernel.org, robh+dt@kernel.org,
mark.rutland@arm.com, Michal Simek, linux-media@vger.kernel.org,
devicetree@vger.kernel.org, hans.verkuil@cisco.com,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, Dinesh Kumar, Sandip Kothari,
Joe Perches
In-Reply-To: <DM6PR02MB6876116CECBA49741FF57E79A78A0@DM6PR02MB6876.namprd02.prod.outlook.com>
Hi Vishal,
On Mon, Jun 01, 2020 at 03:14:52PM +0000, Vishal Sagar wrote:
> On Wednesday, May 6, 2020 6:32 PM, Laurent Pinchart wrote:
> > On Wed, Apr 29, 2020 at 07:47:03PM +0530, Vishal Sagar wrote:
> > > Add bindings documentation for Xilinx UHD-SDI Receiver Subsystem.
> > >
> > > The Xilinx UHD-SDI Receiver Subsystem consists of SMPTE UHD-SDI (RX) IP
> > > core, an SDI RX to Video Bridge IP core to convert SDI video to native
> > > video and a Video In to AXI4-Stream IP core to convert native video to
> > > AXI4-Stream.
> > >
> > > Signed-off-by: Vishal Sagar <vishal.sagar@xilinx.com>
> > > ---
> > > v2
> > > - Removed references to xlnx,video*
> > > - Fixed as per Sakari Ailus and Rob Herring's comments
> > > - Converted to yaml format
> > >
> > > .../bindings/media/xilinx/xlnx,sdirxss.yaml | 132 ++++++++++++++++++
> > > 1 file changed, 132 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/media/xilinx/xlnx,sdirxss.yaml
> > >
> > > diff --git
> > a/Documentation/devicetree/bindings/media/xilinx/xlnx,sdirxss.yaml
> > b/Documentation/devicetree/bindings/media/xilinx/xlnx,sdirxss.yaml
> > > new file mode 100644
> > > index 000000000000..9133ad19df55
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/xilinx/xlnx,sdirxss.yaml
> > > @@ -0,0 +1,132 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/media/xilinx/xlnx,sdirxss.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +
> > > +title: Xilinx SMPTE UHD-SDI Receiver Subsystem
> > > +
> > > +maintainers:
> > > + - Vishal Sagar <vishal.sagar@xilinx.com>
> > > +
> > > +description: |
> > > + The SMPTE UHD-SDI Receiver (RX) Subsystem allows you to quickly create systems
> > > + based on SMPTE SDI protocols. It receives unaligned native SDI streams from
> > > + the SDI GT PHY and outputs an AXI4-Stream video stream, native video, or
> > > + native SDI using Xilinx transceivers as the physical layer.
> > > +
> > > + The subsystem consists of
> > > + 1 - SMPTE UHD-SDI Rx
> > > + 2 - SDI Rx to Native Video Bridge
> > > + 3 - Video In to AXI4-Stream Bridge
> > > +
> > > + The subsystem can capture SDI streams in upto 12G mode 8 data streams and output
> >
> > s/upto/up to/
>
> I will fix this in next version.
>
> > > + a dual pixel per clock RGB/YUV444,422/420 10/12 bits per component AXI4-Stream.
> > > +
> > > +properties:
> > > + compatible:
> > > + items:
> > > + - enum:
> > > + - xlnx,v-smpte-uhdsdi-rx-ss-2.0
> > > +
> > > + reg:
> > > + maxItems: 1
> > > +
> > > + interrupts:
> > > + maxItems: 1
> > > +
> > > + clocks:
> > > + description: List of clock specifiers
> > > + items:
> > > + - description: AXI4-Lite clock
> > > + - description: SMPTE UHD-SDI Rx core clock
> > > + - description: Video clock
> > > +
> > > + clock-names:
> > > + items:
> > > + - const: s_axi_aclk
> > > + - const: sdi_rx_clk
> > > + - const: video_out_clk
> > > +
> > > + xlnx,bpp:
> > > + description: Bits per pixel supported. Can be 10 or 12 bits per pixel only.
> > > + allOf:
> > > + - $ref: "/schemas/types.yaml#/definitions/uint32"
> > > + - enum: [10, 12]
> >
> > I don't see this as a design parameter in the documentation (pg290,
> > v2.0). What does it correspond to ? All the BPC mentions in the
> > documentation always state that 10-bit is the only supported value.
>
> The new version of IP being released will have 10 and 12 bit support. It is already in the Xilinx linux-xlnx repo.
> I will rename this to "xlnx,bpc" instead of "xlnx,bpp" to refer to bits per component.
Is the documentation for the new IP core version available ? Should this
property only be allowed for the new version, given that in v2.0 the BPC
is fixed to 10 ?
> > > +
> > > + xlnx,line-rate:
> > > + description: |
> > > + The maximum mode supported by the design. Possible values are as below
> > > + 12G_SDI_8DS - 12G mode with 8 data streams
> > > + 6G_SDI - 6G mode
> > > + 3G_SDI - 3G mode
> > > + enum:
> > > + - 12G_SDI_8DS
> > > + - 6G_SDI
> > > + - 3G_SDI
> >
> > How about making this an integer property, with #define in
> > include/dt-bindings/media/xilinx-sdi.h ? As far as I understand, the SDI
> > TX subsystem has the same parameter, so the #define could be shared
> > between the two.
>
> Yes that is ok with me. I will add this in the next version.
>
> > > +
> > > + xlnx,include-edh:
> > > + type: boolean
> > > + description: |
> > > + This is present when the Error Detection and Handling processor is
> > > + enabled in design.
> > > +
> > > + ports:
> > > + type: object
> > > + description: |
> > > + Generally the SDI port is connected to a device like SDI Broadcast camera
> > > + which is independently controlled. Hence port@0 is a source port which can be
> > > + connected to downstream IP which can work with AXI4 Stream data.
> >
> > We should still have an input port. It can be connected to a DT node for
> > a physical SDI connector, or any other component in the platform (I
> > expect the former to be the common case). There are DT bindings for
> > connectors in Documentation/devicetree/bindings/display/connector/, we
> > should add one for SDI.
>
> Yes the input port is a physical SDI connector connected to an equipment like broadcast camera.
> But the camera/equipment can't be controlled by the V4L2 pipeline and SDI protocol is unidirectional.
>
> If we add another dt node, then I think another dummy v4l subdev driver will need to implemented and loaded
> to complete the pipe as Xilinx Video driver will need it.
We don't necessarily need a driver for the connector (although it may be
a good idea to do so, but that's a separate question). The sdi-rx driver
could handle the SDI connector DT node by parsing its properties
manually (assuming it would contain properties that need to be parsed).
> Could you please share the reason to have this input port in the SDI Rx driver?
We try to make sure the whole pipeline is modelled in DT. This applies
to both V4L2 and DRM/KMS. While the connector doesn't need to be
controlled by software (it's just a connector), it may still have
properties that matter from a software point of view. For instance the
label property can be used to specify how the connector is labeled on
the board or on the device's case, allowing applications to display the
correct labels to the users. Another use case related to the 4-pin
mini-DIN connectors typically used for S-Video. On some devices, they
are also used for composite video, with multiple video sources connected
to the same mini-DIN connector with a special cable. Kernel drivers need
to know how signals are routed, and DT nodes help there.
> > > + properties:
> > > + port@0:
> > > + type: object
> > > + description: Source port
> > > + properties:
> > > + reg:
> > > + const: 0
> > > + endpoint:
> > > + type: object
> > > + properties:
> > > + remote-endpoint: true
> > > + required:
> > > + - remote-endpoint
> > > + additionalProperties: false
> > > + additionalProperties: false
> > > +
> > > +required:
> > > + - compatible
> > > + - reg
> > > + - interrupts
> > > + - clocks
> > > + - clock-names
> > > + - xlnx,line-rate
> > > + - xlnx,bpp
> > > + - ports
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > + - |
> > > + uhdsdirxss: v-smpte-uhdsdi-rxss@80000000 {
I forgot to mention, you can drop the label as it's not used.
> > > + compatible = "xlnx,v-smpte-uhdsdi-rx-ss-2.0";
> > > + interrupt-parent = <&gic>;
> > > + interrupts = <0 89 4>;
> > > + reg = <0x0 0x80000000 0x0 0x10000>;
> > > + xlnx,include-edh;
> > > + xlnx,line-rate = "12G_SDI_8DS";
> > > + clocks = <&clk_1>, <&si570_1>, <&clk_2>;
> > > + clock-names = "s_axi_aclk", "sdi_rx_clk", "video_out_clk";
> > > + xlnx,bpp = <10>;
And I would group the xlnx,* properties after the standard properties.
> > > +
> > > + ports {
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > + port@0 {
> > > + reg = <0>;
> > > + sdirx_out: endpoint {
> > > + remote-endpoint = <&vcap_sdirx_in>;
> > > + };
> > > + };
> > > + };
> > > + };
--
Regards,
Laurent Pinchart
^ permalink raw reply
* [PATCH 0/3] Convert i.MX/MXS I2C/LPI2C binding doc to json-schema
From: Anson Huang @ 2020-06-03 1:58 UTC (permalink / raw)
To: aisheng.dong, robh+dt, shawnguo, s.hauer, kernel, festevam, linux,
pandy.gao, wolfram, linux-i2c, devicetree, linux-arm-kernel,
linux-kernel
Cc: Linux-imx
Coverts i.MX/MXS I2C.LPI2C binding doc to json-schema, some examples are
too old, update them based on latest DT file, also add more compatible
based on supported SoCs.
Anson Huang (3):
dt-bindings: i2c: Convert imx lpi2c to json-schema
dt-bindings: i2c: Convert mxs i2c to json-schema
dt-bindings: i2c: Convert imx i2c to json-schema
.../devicetree/bindings/i2c/i2c-imx-lpi2c.txt | 20 ----
.../devicetree/bindings/i2c/i2c-imx-lpi2c.yaml | 45 ++++++++
Documentation/devicetree/bindings/i2c/i2c-imx.txt | 49 ---------
Documentation/devicetree/bindings/i2c/i2c-imx.yaml | 118 +++++++++++++++++++++
Documentation/devicetree/bindings/i2c/i2c-mxs.txt | 25 -----
Documentation/devicetree/bindings/i2c/i2c-mxs.yaml | 55 ++++++++++
6 files changed, 218 insertions(+), 94 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.txt
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.yaml
delete mode 100644 Documentation/devicetree/bindings/i2c/i2c-imx.txt
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-imx.yaml
delete mode 100644 Documentation/devicetree/bindings/i2c/i2c-mxs.txt
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mxs.yaml
--
2.7.4
^ permalink raw reply
* [PATCH 3/3] dt-bindings: i2c: Convert imx i2c to json-schema
From: Anson Huang @ 2020-06-03 1:58 UTC (permalink / raw)
To: aisheng.dong, robh+dt, shawnguo, s.hauer, kernel, festevam, linux,
pandy.gao, wolfram, linux-i2c, devicetree, linux-arm-kernel,
linux-kernel
Cc: Linux-imx
In-Reply-To: <1591149535-342-1-git-send-email-Anson.Huang@nxp.com>
Convert the i.MX I2C binding to DT schema format using json-schema,
some improvements applied, such as update example based on latest DT
file, add more compatible for existing SoCs, and remove unnecessary
common property "pinctrl".
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
Documentation/devicetree/bindings/i2c/i2c-imx.txt | 49 ---------
Documentation/devicetree/bindings/i2c/i2c-imx.yaml | 118 +++++++++++++++++++++
2 files changed, 118 insertions(+), 49 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/i2c/i2c-imx.txt
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-imx.yaml
diff --git a/Documentation/devicetree/bindings/i2c/i2c-imx.txt b/Documentation/devicetree/bindings/i2c/i2c-imx.txt
deleted file mode 100644
index b967544..0000000
--- a/Documentation/devicetree/bindings/i2c/i2c-imx.txt
+++ /dev/null
@@ -1,49 +0,0 @@
-* Freescale Inter IC (I2C) and High Speed Inter IC (HS-I2C) for i.MX
-
-Required properties:
-- compatible :
- - "fsl,imx1-i2c" for I2C compatible with the one integrated on i.MX1 SoC
- - "fsl,imx21-i2c" for I2C compatible with the one integrated on i.MX21 SoC
- - "fsl,vf610-i2c" for I2C compatible with the one integrated on Vybrid vf610 SoC
-- reg : Should contain I2C/HS-I2C registers location and length
-- interrupts : Should contain I2C/HS-I2C interrupt
-- clocks : Should contain the I2C/HS-I2C clock specifier
-
-Optional properties:
-- clock-frequency : Constains desired I2C/HS-I2C bus clock frequency in Hz.
- The absence of the property indicates the default frequency 100 kHz.
-- dmas: A list of two dma specifiers, one for each entry in dma-names.
-- dma-names: should contain "tx" and "rx".
-- scl-gpios: specify the gpio related to SCL pin
-- sda-gpios: specify the gpio related to SDA pin
-- pinctrl: add extra pinctrl to configure i2c pins to gpio function for i2c
- bus recovery, call it "gpio" state
-
-Examples:
-
-i2c@83fc4000 { /* I2C2 on i.MX51 */
- compatible = "fsl,imx51-i2c", "fsl,imx21-i2c";
- reg = <0x83fc4000 0x4000>;
- interrupts = <63>;
-};
-
-i2c@70038000 { /* HS-I2C on i.MX51 */
- compatible = "fsl,imx51-i2c", "fsl,imx21-i2c";
- reg = <0x70038000 0x4000>;
- interrupts = <64>;
- clock-frequency = <400000>;
-};
-
-i2c0: i2c@40066000 { /* i2c0 on vf610 */
- compatible = "fsl,vf610-i2c";
- reg = <0x40066000 0x1000>;
- interrupts =<0 71 0x04>;
- dmas = <&edma0 0 50>,
- <&edma0 0 51>;
- dma-names = "rx","tx";
- pinctrl-names = "default", "gpio";
- pinctrl-0 = <&pinctrl_i2c1>;
- pinctrl-1 = <&pinctrl_i2c1_gpio>;
- scl-gpios = <&gpio5 26 GPIO_ACTIVE_HIGH>;
- sda-gpios = <&gpio5 27 GPIO_ACTIVE_HIGH>;
-};
diff --git a/Documentation/devicetree/bindings/i2c/i2c-imx.yaml b/Documentation/devicetree/bindings/i2c/i2c-imx.yaml
new file mode 100644
index 0000000..0d31d1c
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-imx.yaml
@@ -0,0 +1,118 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i2c/i2c-imx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Freescale Inter IC (I2C) and High Speed Inter IC (HS-I2C) for i.MX
+
+maintainers:
+ - Wolfram Sang <wolfram@the-dreams.de>
+
+properties:
+ compatible:
+ oneOf:
+ - const: fsl,imx1-i2c
+ - const: fsl,imx21-i2c
+ - const: fsl,vf610-i2c
+ - items:
+ - const: fsl,imx35-i2c
+ - const: fsl,imx1-i2c
+ - items:
+ - enum:
+ - fsl,imx25-i2c
+ - fsl,imx27-i2c
+ - fsl,imx31-i2c
+ - fsl,imx50-i2c
+ - fsl,imx51-i2c
+ - fsl,imx53-i2c
+ - fsl,imx6q-i2c
+ - fsl,imx6sl-i2c
+ - fsl,imx6sx-i2c
+ - fsl,imx6sll-i2c
+ - fsl,imx6ul-i2c
+ - fsl,imx7s-i2c
+ - fsl,imx8mq-i2c
+ - fsl,imx8mm-i2c
+ - fsl,imx8mn-i2c
+ - fsl,imx8mp-i2c
+ - const: fsl,imx21-i2c
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ maxItems: 1
+
+ clock-frequency:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: |
+ Constains desired I2C/HS-I2C bus clock frequency in Hz.
+ The absence of the property indicates the default frequency 100 kHz.
+ default: 100000
+
+ dmas:
+ items:
+ - description: DMA controller phandle and request line for RX
+ - description: DMA controller phandle and request line for TX
+
+ dma-names:
+ items:
+ - const: rx
+ - const: tx
+
+ sda-gpios:
+ $ref: '/schemas/types.yaml#/definitions/phandle'
+ description: |
+ gpio used for the sda signal, this should be flagged as
+ active high using open drain with (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)
+ from <dt-bindings/gpio/gpio.h> since the signal is by definition
+ open drain.
+ maxItems: 1
+
+ scl-gpios:
+ $ref: '/schemas/types.yaml#/definitions/phandle'
+ description: |
+ gpio used for the scl signal, this should be flagged as
+ active high using open drain with (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)
+ from <dt-bindings/gpio/gpio.h> since the signal is by definition
+ open drain.
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - clocks
+
+examples:
+ - |
+ #include <dt-bindings/clock/imx5-clock.h>
+ #include <dt-bindings/clock/vf610-clock.h>
+ #include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+ i2c@83fc4000 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "fsl,imx51-i2c", "fsl,imx21-i2c";
+ reg = <0x83fc4000 0x4000>;
+ interrupts = <63>;
+ clocks = <&clks IMX5_CLK_I2C2_GATE>;
+ };
+
+ i2c@40066000 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "fsl,vf610-i2c";
+ reg = <0x40066000 0x1000>;
+ interrupts = <71 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clks VF610_CLK_I2C0>;
+ clock-names = "ipg";
+ dmas = <&edma0 0 50>,
+ <&edma0 0 51>;
+ dma-names = "rx","tx";
+ };
--
2.7.4
^ permalink raw reply related
* [PATCH 2/3] dt-bindings: i2c: Convert mxs i2c to json-schema
From: Anson Huang @ 2020-06-03 1:58 UTC (permalink / raw)
To: aisheng.dong, robh+dt, shawnguo, s.hauer, kernel, festevam, linux,
pandy.gao, wolfram, linux-i2c, devicetree, linux-arm-kernel,
linux-kernel
Cc: Linux-imx
In-Reply-To: <1591149535-342-1-git-send-email-Anson.Huang@nxp.com>
Convert the MXS I2C binding to DT schema format using json-schema
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
Documentation/devicetree/bindings/i2c/i2c-mxs.txt | 25 ----------
Documentation/devicetree/bindings/i2c/i2c-mxs.yaml | 55 ++++++++++++++++++++++
2 files changed, 55 insertions(+), 25 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/i2c/i2c-mxs.txt
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mxs.yaml
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mxs.txt b/Documentation/devicetree/bindings/i2c/i2c-mxs.txt
deleted file mode 100644
index 4e1c8ac..0000000
--- a/Documentation/devicetree/bindings/i2c/i2c-mxs.txt
+++ /dev/null
@@ -1,25 +0,0 @@
-* Freescale MXS Inter IC (I2C) Controller
-
-Required properties:
-- compatible: Should be "fsl,<chip>-i2c"
-- reg: Should contain registers location and length
-- interrupts: Should contain ERROR interrupt number
-- clock-frequency: Desired I2C bus clock frequency in Hz.
- Only 100000Hz and 400000Hz modes are supported.
-- dmas: DMA specifier, consisting of a phandle to DMA controller node
- and I2C DMA channel ID.
- Refer to dma.txt and fsl-mxs-dma.txt for details.
-- dma-names: Must be "rx-tx".
-
-Examples:
-
-i2c0: i2c@80058000 {
- #address-cells = <1>;
- #size-cells = <0>;
- compatible = "fsl,imx28-i2c";
- reg = <0x80058000 2000>;
- interrupts = <111>;
- clock-frequency = <100000>;
- dmas = <&dma_apbx 6>;
- dma-names = "rx-tx";
-};
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mxs.yaml b/Documentation/devicetree/bindings/i2c/i2c-mxs.yaml
new file mode 100644
index 0000000..7adcba3
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-mxs.yaml
@@ -0,0 +1,55 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i2c/i2c-mxs.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Freescale MXS Inter IC (I2C) Controller
+
+maintainers:
+ - Shawn Guo <shawn.guo@linaro.org>
+
+properties:
+ compatible:
+ enum:
+ - fsl,imx23-i2c
+ - fsl,imx28-i2c
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ clock-frequency:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: |
+ Desired I2C bus clock frequency in Hz, only 100000Hz and 400000Hz
+ modes are supported.
+ default: 100000
+
+ dmas:
+ maxItems: 1
+
+ dma-names:
+ const: rx-tx
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - dmas
+ - dma-names
+
+examples:
+ - |
+ i2c@80058000 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "fsl,imx28-i2c";
+ reg = <0x80058000 2000>;
+ interrupts = <111>;
+ clock-frequency = <100000>;
+ dmas = <&dma_apbx 6>;
+ dma-names = "rx-tx";
+ };
--
2.7.4
^ permalink raw reply related
* [PATCH 1/3] dt-bindings: i2c: Convert imx lpi2c to json-schema
From: Anson Huang @ 2020-06-03 1:58 UTC (permalink / raw)
To: aisheng.dong, robh+dt, shawnguo, s.hauer, kernel, festevam, linux,
pandy.gao, wolfram, linux-i2c, devicetree, linux-arm-kernel,
linux-kernel
Cc: Linux-imx
In-Reply-To: <1591149535-342-1-git-send-email-Anson.Huang@nxp.com>
Convert the i.MX LPI2C binding to DT schema format using json-schema
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
.../devicetree/bindings/i2c/i2c-imx-lpi2c.txt | 20 ----------
.../devicetree/bindings/i2c/i2c-imx-lpi2c.yaml | 45 ++++++++++++++++++++++
2 files changed, 45 insertions(+), 20 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.txt
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.yaml
diff --git a/Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.txt b/Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.txt
deleted file mode 100644
index f0c072f..0000000
--- a/Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.txt
+++ /dev/null
@@ -1,20 +0,0 @@
-* Freescale Low Power Inter IC (LPI2C) for i.MX
-
-Required properties:
-- compatible :
- - "fsl,imx7ulp-lpi2c" for LPI2C compatible with the one integrated on i.MX7ULP soc
- - "fsl,imx8qxp-lpi2c" for LPI2C compatible with the one integrated on i.MX8QXP soc
- - "fsl,imx8qm-lpi2c" for LPI2C compatible with the one integrated on i.MX8QM soc
-- reg : address and length of the lpi2c master registers
-- interrupts : lpi2c interrupt
-- clocks : lpi2c clock specifier
-
-Examples:
-
-lpi2c7: lpi2c7@40a50000 {
- compatible = "fsl,imx7ulp-lpi2c";
- reg = <0x40A50000 0x10000>;
- interrupt-parent = <&intc>;
- interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
- clocks = <&clks IMX7ULP_CLK_LPI2C7>;
-};
diff --git a/Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.yaml b/Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.yaml
new file mode 100644
index 0000000..3c0be0c
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.yaml
@@ -0,0 +1,45 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i2c/i2c-imx-lpi2c.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Freescale Low Power Inter IC (LPI2C) for i.MX
+
+maintainers:
+ - Anson Huang <Anson.Huang@nxp.com>
+
+properties:
+ compatible:
+ enum:
+ - fsl,imx7ulp-lpi2c
+ - fsl,imx8qxp-lpi2c
+ - fsl,imx8qm-lpi2c
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - clocks
+
+examples:
+ - |
+ #include <dt-bindings/clock/imx7ulp-clock.h>
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+ lpi2c7@40a50000 {
+ compatible = "fsl,imx7ulp-lpi2c";
+ reg = <0x40A50000 0x10000>;
+ interrupt-parent = <&intc>;
+ interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clks IMX7ULP_CLK_LPI2C7>;
+ };
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v10 00/10] exynos-ufs: Add support for UFS HCI
From: Martin K. Petersen @ 2020-06-03 2:31 UTC (permalink / raw)
To: robh, Alim Akhtar
Cc: Martin K . Petersen, krzk, linux-samsung-soc, avri.altman,
stanley.chu, linux-scsi, linux-arm-kernel, cang, devicetree,
kwmad.kim, linux-kernel
In-Reply-To: <20200528011658.71590-1-alim.akhtar@samsung.com>
On Thu, 28 May 2020 06:46:48 +0530, Alim Akhtar wrote:
> This patch-set introduces UFS (Universal Flash Storage) host
> controller support for Samsung family SoC. Mostly, it consists of
> UFS PHY and host specific driver.
> [...]
Applied [1,2,3,4,5,9] to 5.9/scsi-queue. The series won't show up in
my public tree until shortly after -rc1 is released.
Thanks!
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply
* RE: [PATCH V3 1/3] dt-bindings: mailbox: imx-mu: support i.MX8M
From: Peng Fan @ 2020-06-03 3:10 UTC (permalink / raw)
To: Peng Fan, shawnguo@kernel.org, Fabio Estevam,
kernel@pengutronix.de, Aisheng Dong, robh+dt@kernel.org,
sboyd@kernel.org, linux@rempel-privat.de,
jaswinder.singh@linaro.org
Cc: linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, dl-linux-imx, Leonard Crestez,
Daniel Baluta, l.stach@pengutronix.de, devicetree@vger.kernel.org,
linux-clk@vger.kernel.org
In-Reply-To: <1590999602-29482-2-git-send-email-peng.fan@nxp.com>
> Subject: [PATCH V3 1/3] dt-bindings: mailbox: imx-mu: support i.MX8M
I'll drop this patch for yaml update, since https://lkml.org/lkml/2020/6/1/370
includes imx8mq/mm/n/p.
Thanks,
Peng.
>
> From: Peng Fan <peng.fan@nxp.com>
>
> Add i.MX8MQ/M/N/P compatible string to support i.MX8M SoCs
>
> Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> Documentation/devicetree/bindings/mailbox/fsl,mu.txt | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> index 26b7a88c2fea..906377acf2cd 100644
> --- a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> +++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> @@ -18,7 +18,8 @@ Messaging Unit Device Node:
> Required properties:
> -------------------
> - compatible : should be "fsl,<chip>-mu", the supported chips include
> - imx6sx, imx7s, imx8qxp, imx8qm.
> + imx6sx, imx7s, imx8qxp, imx8qm, imx8mq, imx8mm, imx8mn,
> + imx8mp.
> The "fsl,imx6sx-mu" compatible is seen as generic and should
> be included together with SoC specific compatible.
> There is a version 1.0 MU on imx7ulp, use "fsl,imx7ulp-mu"
> --
> 2.16.4
^ permalink raw reply
* [PATCH V4 0/2] imx8m: add mu support
From: peng.fan @ 2020-06-03 3:35 UTC (permalink / raw)
To: shawnguo, fabio.estevam, kernel, aisheng.dong, robh+dt, sboyd,
linux
Cc: linux-arm-kernel, linux-kernel, linux-imx, leonard.crestez,
daniel.baluta, l.stach, devicetree, linux-clk, Peng Fan
From: Peng Fan <peng.fan@nxp.com>
V4:
Drop patch 1/3, since https://lkml.org/lkml/2020/6/1/370 already
has the yaml changes
V3:
Add R-b tag
Remove undocumented property
V2:
Add dt-bindings
Merge dts changes into one patch, since all is to add mu node
Add mu dt bindings
Add mu node
Add i.MX8MP mu root clk
Peng Fan (2):
arm64: dts: imx8m: add mu node
clk: imx8mp: add mu root clk
arch/arm64/boot/dts/freescale/imx8mm.dtsi | 8 ++++++++
arch/arm64/boot/dts/freescale/imx8mn.dtsi | 8 ++++++++
arch/arm64/boot/dts/freescale/imx8mp.dtsi | 8 ++++++++
arch/arm64/boot/dts/freescale/imx8mq.dtsi | 8 ++++++++
drivers/clk/imx/clk-imx8mp.c | 1 +
5 files changed, 33 insertions(+)
--
2.16.4
^ permalink raw reply
* [PATCH V4 1/2] arm64: dts: imx8m: add mu node
From: peng.fan @ 2020-06-03 3:35 UTC (permalink / raw)
To: shawnguo, fabio.estevam, kernel, aisheng.dong, robh+dt, sboyd,
linux
Cc: linux-arm-kernel, linux-kernel, linux-imx, leonard.crestez,
daniel.baluta, l.stach, devicetree, linux-clk, Peng Fan
In-Reply-To: <1591155360-26173-1-git-send-email-peng.fan@nxp.com>
From: Peng Fan <peng.fan@nxp.com>
Add mu node to let A53 could communicate with M Core.
Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
arch/arm64/boot/dts/freescale/imx8mm.dtsi | 8 ++++++++
arch/arm64/boot/dts/freescale/imx8mn.dtsi | 8 ++++++++
arch/arm64/boot/dts/freescale/imx8mp.dtsi | 8 ++++++++
arch/arm64/boot/dts/freescale/imx8mq.dtsi | 8 ++++++++
4 files changed, 32 insertions(+)
diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
index aaf6e71101a1..d9e787ea246f 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
@@ -775,6 +775,14 @@
status = "disabled";
};
+ mu: mailbox@30aa0000 {
+ compatible = "fsl,imx8mm-mu", "fsl,imx6sx-mu";
+ reg = <0x30aa0000 0x10000>;
+ interrupts = <GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clk IMX8MM_CLK_MU_ROOT>;
+ #mbox-cells = <2>;
+ };
+
usdhc1: mmc@30b40000 {
compatible = "fsl,imx8mm-usdhc", "fsl,imx7d-usdhc";
reg = <0x30b40000 0x10000>;
diff --git a/arch/arm64/boot/dts/freescale/imx8mn.dtsi b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
index 9a4b65a267d4..3dca1fb34ea3 100644
--- a/arch/arm64/boot/dts/freescale/imx8mn.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
@@ -675,6 +675,14 @@
status = "disabled";
};
+ mu: mailbox@30aa0000 {
+ compatible = "fsl,imx8mn-mu", "fsl,imx6sx-mu";
+ reg = <0x30aa0000 0x10000>;
+ interrupts = <GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clk IMX8MN_CLK_MU_ROOT>;
+ #mbox-cells = <2>;
+ };
+
usdhc1: mmc@30b40000 {
compatible = "fsl,imx8mn-usdhc", "fsl,imx7d-usdhc";
reg = <0x30b40000 0x10000>;
diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
index 45e2c0a4e889..1bc14bb44d90 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
@@ -621,6 +621,14 @@
status = "disabled";
};
+ mu: mailbox@30aa0000 {
+ compatible = "fsl,imx8mp-mu", "fsl,imx6sx-mu";
+ reg = <0x30aa0000 0x10000>;
+ interrupts = <GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clk IMX8MP_CLK_MU_ROOT>;
+ #mbox-cells = <2>;
+ };
+
i2c5: i2c@30ad0000 {
compatible = "fsl,imx8mp-i2c", "fsl,imx21-i2c";
#address-cells = <1>;
diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
index 978f8122c0d2..3e762919d61f 100644
--- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
@@ -959,6 +959,14 @@
status = "disabled";
};
+ mu: mailbox@30aa0000 {
+ compatible = "fsl,imx8mq-mu", "fsl,imx6sx-mu";
+ reg = <0x30aa0000 0x10000>;
+ interrupts = <GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clk IMX8MQ_CLK_MU_ROOT>;
+ #mbox-cells = <2>;
+ };
+
usdhc1: mmc@30b40000 {
compatible = "fsl,imx8mq-usdhc",
"fsl,imx7d-usdhc";
--
2.16.4
^ permalink raw reply related
* [PATCH V4 2/2] clk: imx8mp: add mu root clk
From: peng.fan @ 2020-06-03 3:36 UTC (permalink / raw)
To: shawnguo, fabio.estevam, kernel, aisheng.dong, robh+dt, sboyd,
linux
Cc: linux-arm-kernel, linux-kernel, linux-imx, leonard.crestez,
daniel.baluta, l.stach, devicetree, linux-clk, Peng Fan
In-Reply-To: <1591155360-26173-1-git-send-email-peng.fan@nxp.com>
From: Peng Fan <peng.fan@nxp.com>
Add mu root clk for mu mailbox usage.
Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
drivers/clk/imx/clk-imx8mp.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
index b4d9db9d5bf1..ca747712400f 100644
--- a/drivers/clk/imx/clk-imx8mp.c
+++ b/drivers/clk/imx/clk-imx8mp.c
@@ -680,6 +680,7 @@ static int imx8mp_clocks_probe(struct platform_device *pdev)
hws[IMX8MP_CLK_I2C2_ROOT] = imx_clk_hw_gate4("i2c2_root_clk", "i2c2", ccm_base + 0x4180, 0);
hws[IMX8MP_CLK_I2C3_ROOT] = imx_clk_hw_gate4("i2c3_root_clk", "i2c3", ccm_base + 0x4190, 0);
hws[IMX8MP_CLK_I2C4_ROOT] = imx_clk_hw_gate4("i2c4_root_clk", "i2c4", ccm_base + 0x41a0, 0);
+ hws[IMX8MP_CLK_MU_ROOT] = imx_clk_hw_gate4("mu_root_clk", "ipg_root", ccm_base + 0x4210, 0);
hws[IMX8MP_CLK_OCOTP_ROOT] = imx_clk_hw_gate4("ocotp_root_clk", "ipg_root", ccm_base + 0x4220, 0);
hws[IMX8MP_CLK_PCIE_ROOT] = imx_clk_hw_gate4("pcie_root_clk", "pcie_aux", ccm_base + 0x4250, 0);
hws[IMX8MP_CLK_PWM1_ROOT] = imx_clk_hw_gate4("pwm1_root_clk", "pwm1", ccm_base + 0x4280, 0);
--
2.16.4
^ permalink raw reply related
* Re: [PATCH 06/12] PM / devfreq: Add cpu based scaling support to passive_governor
From: Chanwoo Choi @ 2020-06-03 4:07 UTC (permalink / raw)
To: andrew-sh.cheng
Cc: MyungJoo Ham, Kyungmin Park, Rob Herring, Mark Rutland,
Matthias Brugger, Rafael J . Wysocki, Viresh Kumar,
Nishanth Menon, Stephen Boyd, Liam Girdwood, Mark Brown,
devicetree, srv_heupstream, linux-pm, linux-kernel,
linux-mediatek, Sibi Sankar, linux-arm-kernel
In-Reply-To: <1591098190.30729.15.camel@mtksdaap41>
Hi Andrew-sh.Cheng,
Do you know that why cannot show the patches sent from you on mailing list?
Even if you sent them to linux-pm mailing list, I cannot find
your patches on linux-pm's patchwork[1] and others.
[1] https://patchwork.kernel.org/project/linux-pm/list/
Could you find you patch on mailing list?
Do you use git send-email when you send these patches?
I used the thunderbird tool and gmail for reading the patches.
When I tried to read the original source of this patch,
it looks like that the body of patch is encoded.
I cannot read the plain text of patch body.
- When gmail, use 'Show original'
- When thunderbird, use 'More -> View Source'
If I'm missing something to check this patch,
please let me know. I'll fix my environment.
It is strange situation on my case.
On 6/2/20 8:43 PM, andrew-sh.cheng wrote:
> On Thu, 2020-05-28 at 15:14 +0900, Chanwoo Choi wrote:
>> Hi Andrew-sh.Cheng,
>>
>> Thanks for your posting. I like this approach absolutely.
>> I think that it is necessary. When I developed the embedded product,
>> I needed this feature always.
>>
>> I add the comments on below.
>>
>>
>> And the following email is not valid. So, I dropped this email
>> from Cc list.
>> Saravana Kannan <skannan@codeaurora.org>
>>
>>
>> On 5/20/20 12:43 PM, Andrew-sh.Cheng wrote:
>>> From: Saravana Kannan <skannan@codeaurora.org>
>>>
>>> Many CPU architectures have caches that can scale independent of the
>>> CPUs. Frequency scaling of the caches is necessary to make sure that the
>>> cache is not a performance bottleneck that leads to poor performance and
>>> power. The same idea applies for RAM/DDR.
>>>
>>> To achieve this, this patch adds support for cpu based scaling to the
>>> passive governor. This is accomplished by taking the current frequency
>>> of each CPU frequency domain and then adjust the frequency of the cache
>>> (or any devfreq device) based on the frequency of the CPUs. It listens
>>> to CPU frequency transition notifiers to keep itself up to date on the
>>> current CPU frequency.
>>>
>>> To decide the frequency of the device, the governor does one of the
>>> following:
>>> * Derives the optimal devfreq device opp from required-opps property of
>>> the parent cpu opp_table.
>>>
>>> * Scales the device frequency in proportion to the CPU frequency. So, if
>>> the CPUs are running at their max frequency, the device runs at its
>>> max frequency. If the CPUs are running at their min frequency, the
>>> device runs at its min frequency. It is interpolated for frequencies
>>> in between.
>>>
>>> Andrew-sh.Cheng change
>>> dev_pm_opp_xlate_opp to dev_pm_opp_xlate_required_opp devfreq->max_freq
>>> to devfreq->user_min_freq_req.data.freq.qos->min_freq.target_value
>>> for kernel-5.7
>>>
>>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>>> [Sibi: Integrated cpu-freqmap governor into passive_governor]
>>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>>> Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
>>> ---
>>> drivers/devfreq/Kconfig | 2 +
>>> drivers/devfreq/governor_passive.c | 278 ++++++++++++++++++++++++++++++++++---
>>> include/linux/devfreq.h | 40 +++++-
>>> 3 files changed, 299 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>>> index 0b1df12e0f21..d9067950af6a 100644
>>> --- a/drivers/devfreq/Kconfig
>>> +++ b/drivers/devfreq/Kconfig
>>> @@ -73,6 +73,8 @@ config DEVFREQ_GOV_PASSIVE
>>> device. This governor does not change the frequency by itself
>>> through sysfs entries. The passive governor recommends that
>>> devfreq device uses the OPP table to get the frequency/voltage.
>>> + Alternatively the governor can also be chosen to scale based on
>>> + the online CPUs current frequency.
>>>
>>> comment "DEVFREQ Drivers"
>>>
>>> diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
>>> index 2d67d6c12dce..7dcda02a5bb7 100644
>>> --- a/drivers/devfreq/governor_passive.c
>>> +++ b/drivers/devfreq/governor_passive.c
>>> @@ -8,11 +8,89 @@
>>> */
>>>
>>> #include <linux/module.h>
>>> +#include <linux/cpu.h>
>>> +#include <linux/cpufreq.h>
>>> +#include <linux/cpumask.h>
>>> #include <linux/device.h>
>>> #include <linux/devfreq.h>
>>> +#include <linux/slab.h>
>>> #include "governor.h"
>>>
>>> -static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
>>> +static unsigned int xlate_cpufreq_to_devfreq(struct devfreq_passive_data *data,
>>
>> Need to change 'unsigned int' to 'unsigned long'
> Get it.
If you add the blank line before/after of your reply,
it is better to catch your reply. Please add the blank line for me.
>> .
>>
>>> + unsigned int cpu)
>>> +{
>>> + unsigned int cpu_min, cpu_max, dev_min, dev_max, cpu_percent, max_state;
>>
>> Better to define them separately as following and then need to rename
>> the variable. Usually, use the 'min_freq' and 'max_freq' word for
>> the minimum/maximum frequency.
>>
>> unsigned int cpu_min_freq, cpu_max_freq, cpu_curr_freq, cpu_percent;
>> unsigned long dev_min_freq, dev_max_freq, dev_max_state,
>>
>> The devfreq used 'unsigned long'. The cpufreq used 'unsigned long'
>> and 'unsigned int'. You need to handle them properly.
> Get it.
> For cpu_freq, I separate it into "unsigned long cpu_curr_freq" and
> "unsigned int cpu_curr_freq_khz"
>>
>>
>>> + struct devfreq_cpu_state *cpu_state = data->cpu_state[cpu];
>>> + struct devfreq *devfreq = (struct devfreq *)data->this;
>>> + unsigned long *freq_table = devfreq->profile->freq_table;
>>
>> In this function, use 'cpu' work for cpufreq and use 'dev' for devfreq.
>> So, I think 'dev_freq_table' is proper name instead of 'freq_table'
>> for the readability.
>>
>> freq_table -> dev_freq_table
>>
>>> + struct dev_pm_opp *opp = NULL, *cpu_opp = NULL;
>>
>> In the get_target_freq_with_devfreq(), use 'p_opp' indicating
>> the OPP of parent device. For the consistency, I think that
>> use 'p_opp' instead of 'cpu_opp'.
>>
>>> + unsigned long cpu_freq, freq;
>>
>> Define the 'cpu_freq' on above with cpu_min_freq/cpu_max_freq definition.
>> cpu_freq -> cpu_curr_freq.
> Get it.
> Will modify them for readability.
>>
>>> +
>>> + if (!cpu_state || cpu_state->first_cpu != cpu ||
>>> + !cpu_state->opp_table || !devfreq->opp_table)
>>> + return 0;
>>> +
>>> + cpu_freq = cpu_state->freq * 1000;
>>> + cpu_opp = devfreq_recommended_opp(cpu_state->dev, &cpu_freq, 0);
>>> + if (IS_ERR(cpu_opp))
>>> + return 0;
>>> +
>>> + opp = dev_pm_opp_xlate_required_opp(cpu_state->opp_table,
>>> + devfreq->opp_table, cpu_opp);
>>> + dev_pm_opp_put(cpu_opp);
>>> +
>>> + if (!IS_ERR(opp)) {
>>> + freq = dev_pm_opp_get_freq(opp);
>>> + dev_pm_opp_put(opp);
>>
>> Better to add the 'out' goto statement.
>> If you use 'goto out', you can reduce the one indentation
>> without 'else' statement.
> Get it.
>>
>>
>>> + } else {
>>
>> As I commented, when dev_pm_opp_xlate_required_opp() return successfully
>> , use 'goto out'. We can remove 'else' and then reduce the unneeded indentation.
>>
>>
>>> + /* Use Interpolation if required opps is not available */
>>> + cpu_min = cpu_state->min_freq;
>>> + cpu_max = cpu_state->max_freq;
>>> + cpu_freq = cpu_state->freq;
>>> +
>>> + if (freq_table) {
>>> + /* Get minimum frequency according to sorting order */
>>> + max_state = freq_table[devfreq->profile->max_state - 1];
>>> + if (freq_table[0] < max_state) {
>>> + dev_min = freq_table[0];
>>> + dev_max = max_state;
>>> + } else {
>>> + dev_min = max_state;
>>> + dev_max = freq_table[0];
>>> + }
>>> + } else {
>>> + if (devfreq->user_max_freq_req.data.freq.qos->max_freq.target_value
>>> + <= devfreq->user_min_freq_req.data.freq.qos->min_freq.target_value)
>>> + return 0;
>>> + dev_min =
>>> + devfreq->user_min_freq_req.data.freq.qos->min_freq.target_value;
>>> + dev_max =
>>> + devfreq->user_max_freq_req.data.freq.qos->max_freq.target_value;
>>
>> I think it is not proper to access the variable of pm_qos structure directly.
>> Instead of direct access, you have to use the exported PM QoS function such as
>> - pm_qos_read_value(devfreq->dev.parent, DEV_PM_QOS_MIN_FREQUENCY);
>> - pm_qos_read_value(devfreq->dev.parent, DEV_PM_QOS_MAX_FREQUENCY);
> Get it.
>>
>>> + }
>>> + cpu_percent = ((cpu_freq - cpu_min) * 100) / cpu_max - cpu_min;
>>> + freq = dev_min + mult_frac(dev_max - dev_min, cpu_percent, 100);
>>> + }
>>
>>
>> I think that you better to add 'out' jump label as following:
>>
>> out:
>>
>>> +
>>> + return freq;
>>> +}
>>> +
>>> +static int get_target_freq_with_cpufreq(struct devfreq *devfreq,
>>> + unsigned long *freq)
>>> +{
>>> + struct devfreq_passive_data *p_data =
>>> + (struct devfreq_passive_data *)devfreq->data;
>>> + unsigned int cpu, target_freq = 0;
>>
>> Need to define 'target_freq' with 'unsigned long' type.
> Get it.
>>
>>> +
>>> + for_each_online_cpu(cpu)
>>> + target_freq = max(target_freq,
>>> + xlate_cpufreq_to_devfreq(p_data, cpu));
>>> +
>>> + *freq = target_freq;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int get_target_freq_with_devfreq(struct devfreq *devfreq,
>>> unsigned long *freq)
>>> {
>>> struct devfreq_passive_data *p_data
>>> @@ -23,16 +101,6 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
>>> int i, count, ret = 0;
>>>
>>> /*
>>> - * If the devfreq device with passive governor has the specific method
>>> - * to determine the next frequency, should use the get_target_freq()
>>> - * of struct devfreq_passive_data.
>>> - */
>>> - if (p_data->get_target_freq) {
>>> - ret = p_data->get_target_freq(devfreq, freq);
>>> - goto out;
>>> - }
>>> -
>>> - /*
>>> * If the parent and passive devfreq device uses the OPP table,
>>> * get the next frequency by using the OPP table.
>>> */
>>> @@ -102,6 +170,37 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
>>> return ret;
>>> }
>>>
>>> +static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
>>> + unsigned long *freq)
>>> +{
>>> + struct devfreq_passive_data *p_data =
>>> + (struct devfreq_passive_data *)devfreq->data;
>>> + int ret;
>>> +
>>> + /*
>>> + * If the devfreq device with passive governor has the specific method
>>> + * to determine the next frequency, should use the get_target_freq()
>>> + * of struct devfreq_passive_data.
>>> + */
>>> + if (p_data->get_target_freq)
>>> + return p_data->get_target_freq(devfreq, freq);
>>> +
>>> + switch (p_data->parent_type) {
>>> + case DEVFREQ_PARENT_DEV:
>>> + ret = get_target_freq_with_devfreq(devfreq, freq);
>>> + break;
>>> + case CPUFREQ_PARENT_DEV:
>>> + ret = get_target_freq_with_cpufreq(devfreq, freq);
>>> + break;
>>> + default:
>>> + ret = -EINVAL;
>>> + dev_err(&devfreq->dev, "Invalid parent type\n");
>>> + break;
>>> + }
>>> +
>>> + return ret;
>>> +}
>>> +
>>> static int update_devfreq_passive(struct devfreq *devfreq, unsigned long freq)
>>> {
>>> int ret;
>>> @@ -156,6 +255,140 @@ static int devfreq_passive_notifier_call(struct notifier_block *nb,
>>> return NOTIFY_DONE;
>>> }
>>>
>>> +static int cpufreq_passive_notifier_call(struct notifier_block *nb,
>>> + unsigned long event, void *ptr)
>>> +{
>>> + struct devfreq_passive_data *data =
>>> + container_of(nb, struct devfreq_passive_data, nb);
>>> + struct devfreq *devfreq = (struct devfreq *)data->this;
>>> + struct devfreq_cpu_state *cpu_state;
>>> + struct cpufreq_freqs *freq = ptr;
>>
>> How about changing 'freq' to 'cpu_freqs'?
>>
>> In the drivers/cpufreq/cpufreq.c, use 'freqs' name indicating
>> the instance of 'struct cpufreq_freqs'. And in order to
>> identfy, how about adding 'cpu_' prefix for variable name?
>>
>>> + unsigned int current_freq;
>>
>> Need to define curr_freq with 'unsigned long' type
>> and better to use 'curr_freq' variable name.
> It is good to change current_freq to curr_freq, but why should it us
> 'unsigned long'?
> I think it is 'unsigned int'.
I think that 'curr_freq' is proper. Yes, it is 'unsigned int'.
When you changing the cpu frequency to device frequency,
recommend to handle them between unsigned int and unsigned long.
>>
>>> + int ret;
>>> +
>>> + if (event != CPUFREQ_POSTCHANGE || !freq ||
>>> + !data->cpu_state[freq->policy->cpu])
>>> + return 0;
>>> +
>>> + cpu_state = data->cpu_state[freq->policy->cpu];
>>> + if (cpu_state->freq == freq->new)
>>> + return 0;
>>> +
>>> + /* Backup current freq and pre-update cpu state freq*/
>>> + current_freq = cpu_state->freq;
>>> + cpu_state->freq = freq->new;
>>> +
>>> + mutex_lock(&devfreq->lock);
>>> + ret = update_devfreq(devfreq);
>>> + mutex_unlock(&devfreq->lock);
>>> + if (ret) {
>>> + cpu_state->freq = current_freq;
>>> + dev_err(&devfreq->dev, "Couldn't update the frequency.\n");
>>> + return ret;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int cpufreq_passive_register(struct devfreq_passive_data **p_data)
>>> +{
>>> + struct devfreq_passive_data *data = *p_data;
>>> + struct devfreq *devfreq = (struct devfreq *)data->this;
>>> + struct device *dev = devfreq->dev.parent;
>>> + struct opp_table *opp_table = NULL;
>>> + struct devfreq_cpu_state *state;
>>
>> For the readability, I thinkt 'cpu_state' is proper instead of 'state'.
> Get it.
>>
>>> + struct cpufreq_policy *policy;
>>> + struct device *cpu_dev;
>>> + unsigned int cpu;
>>> + int ret;
>>> +
>>> + get_online_cpus();
>>
>> Add blank line.
> Get it.
>>
>>> + data->nb.notifier_call = cpufreq_passive_notifier_call;
>>> + ret = cpufreq_register_notifier(&data->nb,
>>> + CPUFREQ_TRANSITION_NOTIFIER);
>>> + if (ret) {
>>> + dev_err(dev, "Couldn't register cpufreq notifier.\n");
>>> + data->nb.notifier_call = NULL;
>>> + goto out;
>>> + }
>>> +
>>> + /* Populate devfreq_cpu_state */
>>> + for_each_online_cpu(cpu) {
>>> + if (data->cpu_state[cpu])
>>> + continue;
>>> +
>>> + policy = cpufreq_cpu_get(cpu);
>>
>> cpufreq_cpu_get() might return 'NULL'. I think you need to handle
>> return value as following:
>>
>> if (!policy) {
>> ret = -EINVAL;
>> goto out;
>> } else if (PTR_ERR(policy) == -EPROBE_DEFER) {
>> goto out;
>> } else if (IS_ERR(policy) {
>> ret = PTR_ERR(policy);
>> dev_err(dev, "Couldn't get the cpufreq_poliy.\n");
>> goto out;
>> }
>>
>> If cpufreq_cpu_get() return successfully, to do next.
>> It reduces the one indentaion.
>>
>>
> Get it.
>>
>>> + if (policy) {
>>> + state = kzalloc(sizeof(*state), GFP_KERNEL);
>>> + if (!state) {
>>> + ret = -ENOMEM;
>>> + goto out;
>>> + }
>>> +
>>> + cpu_dev = get_cpu_device(cpu);
>>> + if (!cpu_dev) {
>>> + dev_err(dev, "Couldn't get cpu device.\n");
>>> + ret = -ENODEV;
>>> + goto out;
>>> + }
>>> +
>>> + opp_table = dev_pm_opp_get_opp_table(cpu_dev);
>>> + if (IS_ERR(devfreq->opp_table)) {
>>> + ret = PTR_ERR(opp_table);
>>> + goto out;
>>> + }
>>> +
>>> + state->dev = cpu_dev;
>>> + state->opp_table = opp_table;
>>> + state->first_cpu = cpumask_first(policy->related_cpus);
>>> + state->freq = policy->cur;
>>> + state->min_freq = policy->cpuinfo.min_freq;
>>> + state->max_freq = policy->cpuinfo.max_freq;
>>> + data->cpu_state[cpu] = state;
>>
>> Add blank line.
>>
>>> + cpufreq_cpu_put(policy);
>>> + } else {
>>> + ret = -EPROBE_DEFER;
>>> + goto out;
>>> + }
>>> + }
>>
>> Add blank line.
> Get it.
>>> +out:
>>> + put_online_cpus();
>>> + if (ret)
>>> + return ret;
>>> +
>>> + /* Update devfreq */
>>> + mutex_lock(&devfreq->lock);
>>> + ret = update_devfreq(devfreq);
>>> + mutex_unlock(&devfreq->lock);
>>> + if (ret)
>>> + dev_err(dev, "Couldn't update the frequency.\n");
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int cpufreq_passive_unregister(struct devfreq_passive_data **p_data)
>>> +{
>>> + struct devfreq_passive_data *data = *p_data;
>>> + struct devfreq_cpu_state *cpu_state;
>>> + int cpu;
>>> +
>>> + if (data->nb.notifier_call)
>>> + cpufreq_unregister_notifier(&data->nb,
>>> + CPUFREQ_TRANSITION_NOTIFIER);
>>> +
>>> + for_each_possible_cpu(cpu) {
>>> + cpu_state = data->cpu_state[cpu];
>>> + if (cpu_state) {
>>> + if (cpu_state->opp_table)
>>> + dev_pm_opp_put_opp_table(cpu_state->opp_table);
>>> + kfree(cpu_state);
>>> + cpu_state = NULL;
>>> + }
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> static int devfreq_passive_event_handler(struct devfreq *devfreq,
>>> unsigned int event, void *data)
>>> {
>>> @@ -165,7 +398,7 @@ static int devfreq_passive_event_handler(struct devfreq *devfreq,
>>> struct notifier_block *nb = &p_data->nb;
>>> int ret = 0;
>>>
>>> - if (!parent)
>>> + if (p_data->parent_type == DEVFREQ_PARENT_DEV && !parent)
>>> return -EPROBE_DEFER;
>>
>> If you modify the devfreq_passive_event_handler() as following,
>> you can move this condition for DEVFREQ_PARENT_DEV into
>> (register|unregister)_parent_dev_notifier.
>>
>> switch (event) {
>> case DEVFREQ_GOV_START:
>> ret = register_parent_dev_notifier(p_data);
>> break;
>> case DEVFREQ_GOV_STOP:
>> ret = unregister_parent_dev_notifier(p_data);
>> break;
>> default:
>> ret = -EINVAL;
>> break;
>> }
>>
>> return ret;
>>
> Get it.
>>>
>>> switch (event) {
>>> @@ -173,13 +406,24 @@ static int devfreq_passive_event_handler(struct devfreq *devfreq,
>>> if (!p_data->this)
>>> p_data->this = devfreq;
>>>
>>> - nb->notifier_call = devfreq_passive_notifier_call;
>>> - ret = devfreq_register_notifier(parent, nb,
>>> - DEVFREQ_TRANSITION_NOTIFIER);
>>> + if (p_data->parent_type == DEVFREQ_PARENT_DEV) {
>>> + nb->notifier_call = devfreq_passive_notifier_call;
>>> + ret = devfreq_register_notifier(parent, nb,
>>> + DEVFREQ_TRANSITION_NOTIFIER);
>>> + } else if (p_data->parent_type == CPUFREQ_PARENT_DEV) {
>>> + ret = cpufreq_passive_register(&p_data);
>>
>> I think that we better to collect the code related to notifier registration
>> into one function like devfreq_pass_register_notifier() instead of
>> cpufreq_passive_register() as following: I think it is more simple and readable.
>>
>> If you have more proper function name of register_parent_dev_notifier,
>> please give your opinion.
>>
>> int register_parent_dev_notifier(struct devfreq_passive_data **p_data)
>> switch (p_data->parent_type) {
>> case DEVFREQ_PARENT_DEV:
>> nb->notifier_call = devfreq_passive_notifier_call;
>> ret = devfreq_register_notifier(parent, nb,
>> break;
>> case CPUFREQ_PARENT_DEV:
>> cpufreq_register_notifier(...)
>> ...
>> break;
>> }
> Not fully understanding.
> Do you mean expanding cpufreq_passive_register()?
Yes and rename it for both cpufreq and devfreq.
> I think leave it in function will be with clean for this code segment.
I want that one function handle the notifier register
for both cpufreq and devfreq so that we make it more simply as following:
On the step hanling the governor event, don't need to consider
the type of parent device of devfreq deivce with this style.
case DEVFREQ_GOV_START:
ret = register_notifier(...);
break;
case DEVFREQ_GOV_STOP:
ret = unregister_notifier(...);
break;
>
>>
>>
>>> + } else {
>>> + ret = -EINVAL;
>>> + }
>>> break;
>>> case DEVFREQ_GOV_STOP:
>>> - WARN_ON(devfreq_unregister_notifier(parent, nb,
>>> - DEVFREQ_TRANSITION_NOTIFIER));
>>> + if (p_data->parent_type == DEVFREQ_PARENT_DEV)
>>> + WARN_ON(devfreq_unregister_notifier(parent, nb,
>>> + DEVFREQ_TRANSITION_NOTIFIER));
>>> + else if (p_data->parent_type == CPUFREQ_PARENT_DEV)
>>> + cpufreq_passive_unregister(&p_data);
>>> + else
>>> + ret = -EINVAL;
>>
>> ditto. unregister_parent_dev_notifier(struct devfreq_passive_data **p_data)
> Get it.
ditto. As I aboved commented.
>>
>>> break;
>>> default:
>>> break;
>>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
>>> index a4b19d593151..04ce576fd6f1 100644
>>> --- a/include/linux/devfreq.h
>>> +++ b/include/linux/devfreq.h
>>> @@ -278,6 +278,32 @@ struct devfreq_simple_ondemand_data {
>>>
>>> #if IS_ENABLED(CONFIG_DEVFREQ_GOV_PASSIVE)
>>> /**
>>> + * struct devfreq_cpu_state - holds the per-cpu state
>>> + * @freq: the current frequency of the cpu.
>>> + * @min_freq: the min frequency of the cpu.
>>> + * @max_freq: the max frequency of the cpu.
>>> + * @first_cpu: the cpumask of the first cpu of a policy.
>>> + * @dev: reference to cpu device.
>>> + * @opp_table: reference to cpu opp table.
>>> + *
>>> + * This structure stores the required cpu_state of a cpu.
>>> + * This is auto-populated by the governor.
>>> + */
>>> +struct devfreq_cpu_state {> + unsigned int freq;
>>
>> It is better to change from 'freq' to 'curr_freq'
>> for more correct expression.
> Get it.
>>
>>> + unsigned int min_freq;
>>> + unsigned int max_freq;
>>> + unsigned int first_cpu;
>>> + struct device *dev;
>>
>> How about changing the name 'dev' to 'cpu_dev'?
> Okay.
>>
>>
>>> + struct opp_table *opp_table;
>>> +};
>>
>> devfreq_cpu_state is only handled by within driver/devfreq/governor_passive.c.
>>
>> So, you can move it into drivers/devfreq/governor_passive.c
>> and just add the definition into include/linux/devfreq.h as following:
>> It is able to prevent the access of variable of 'struct devfreq_cpu_state'
>> outside.
>>
>> struct devfreq_cpu_state;
> Get it.
>>
>>> +
>>> +enum devfreq_parent_dev_type {
>>> + DEVFREQ_PARENT_DEV,
>>> + CPUFREQ_PARENT_DEV,
>>> +};
>>> +
>>> +/**
>>> * struct devfreq_passive_data - ``void *data`` fed to struct devfreq
>>> * and devfreq_add_device
>>> * @parent: the devfreq instance of parent device.
>>> @@ -288,13 +314,15 @@ struct devfreq_simple_ondemand_data {
>>> * using governors except for passive governor.
>>> * If the devfreq device has the specific method to decide
>>> * the next frequency, should use this callback.
>>> - * @this: the devfreq instance of own device.
>>> - * @nb: the notifier block for DEVFREQ_TRANSITION_NOTIFIER list
>>> + * @parent_type parent type of the device
>>
>> Need to add ':' at the end of word. -> "parent_type:".
>>
>>> + * @this: the devfreq instance of own device.
>>> + * @nb: the notifier block for DEVFREQ_TRANSITION_NOTIFIER list
>>
>> I knew that you make them with same indentation.
>> But, actually, it is not related to this patch like clean-up code.
>> Even if it is not pretty, you better to don't touch 'this' and 'nb' indentaion.
> Get it.
>>
>>> + * @cpu_state: the state min/max/current frequency of all online cpu's
>>> *
>>> * The devfreq_passive_data have to set the devfreq instance of parent
>>> * device with governors except for the passive governor. But, don't need to
>>> - * initialize the 'this' and 'nb' field because the devfreq core will handle
>>> - * them.
>>> + * initialize the 'this', 'nb' and 'cpu_state' field because the devfreq core
>>> + * will handle them.
>>> */
>>> struct devfreq_passive_data {
>>> /* Should set the devfreq instance of parent device */
>>> @@ -303,9 +331,13 @@ struct devfreq_passive_data {
>>> /* Optional callback to decide the next frequency of passvice device */
>>> int (*get_target_freq)(struct devfreq *this, unsigned long *freq);
>>>
>>> + /* Should set the type of parent device */
>>> + enum devfreq_parent_dev_type parent_type;
>>> +
>>> /* For passive governor's internal use. Don't need to set them */
>>> struct devfreq *this;
>>> struct notifier_block nb;
>>> + struct devfreq_cpu_state *cpu_state[NR_CPUS];
>>> };
>>> #endif
>>>
>>>
>>
>>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* Re: [PATCH 06/12] PM / devfreq: Add cpu based scaling support to passive_governor
From: Chanwoo Choi @ 2020-06-03 4:12 UTC (permalink / raw)
To: andrew-sh.cheng
Cc: MyungJoo Ham, Kyungmin Park, Rob Herring, Mark Rutland,
Matthias Brugger, Rafael J . Wysocki, Viresh Kumar,
Nishanth Menon, Stephen Boyd, Liam Girdwood, Mark Brown,
devicetree, srv_heupstream, linux-pm, linux-kernel,
linux-mediatek, Sibi Sankar, linux-arm-kernel
In-Reply-To: <1591100614.1804.1.camel@mtksdaap41>
Hi Andrew-sh.Cheng,
On 6/2/20 9:23 PM, andrew-sh.cheng wrote:
> On Thu, 2020-05-28 at 16:17 +0900, Chanwoo Choi wrote:
>> Hi Andrew-sh.Cheng,
>>
>> The exynos-bus.c used the passive governor.
>> Even if don't make the problem because DEVFREQ_PARENT_DEV is zero,
>> you need to initialize the parent_type with DEVFREQ_PARENT_DEV as following:
>>
>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>> index 8fa8eb541373..1c71c47bc2ac 100644
>> --- a/drivers/devfreq/exynos-bus.c
>> +++ b/drivers/devfreq/exynos-bus.c
>> @@ -369,6 +369,7 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
>> return -ENOMEM;
>>
>> passive_data->parent = parent_devfreq;
>> + passive_data->parent_type = DEVFREQ_PARENT_DEV;
>>
>> /* Add devfreq device for exynos bus with passive governor */
>> bus->devfreq = devm_devfreq_add_device(dev, profile, DEVFREQ_GOV_PASSIVE,
> Hi Chanwoo Choi,
> Do you just remind me to initialize it to DEVFREQ_PARENT_DEV whn use
> this governor?
Yes. This change was not included in this patchset.
> I will do it and thank you for reminding.
Thanks.
(snip)
And, this patchset doesn't include the dt-binding example
and any real example in devicetree. If possible, I recommend
you better to update dt-binding document with example.
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* Re: [PATCH V6 4/5] clk: qcom: Add ipq6018 apss clock controller
From: Stephen Boyd @ 2020-06-03 5:31 UTC (permalink / raw)
To: Sivaprakash Murugesan, agross, bjorn.andersson, devicetree,
linux-arm-msm, linux-clk, linux-kernel, mturquette, robh+dt
In-Reply-To: <4266555a-5e4f-4340-1b3c-487a70805751@codeaurora.org>
Quoting Sivaprakash Murugesan (2020-06-02 03:47:20)
>
> On 6/2/2020 1:06 AM, Stephen Boyd wrote:
> > Quoting Sivaprakash Murugesan (2020-06-01 05:41:15)
> >> On 5/28/2020 7:29 AM, Stephen Boyd wrote:
> >>> Quoting Sivaprakash Murugesan (2020-05-27 05:24:51)
> >>>> diff --git a/drivers/clk/qcom/apss-ipq6018.c b/drivers/clk/qcom/apss-ipq6018.c
> >>>> new file mode 100644
> >>>> index 0000000..004f7e1
> >>>> --- /dev/null
> >>>> +++ b/drivers/clk/qcom/apss-ipq6018.c
> >>>> @@ -0,0 +1,106 @@
> >>>> + P_XO,
> >>>> + P_APSS_PLL_EARLY,
> >>>> +};
> >>>> +
> >>>> +static const struct clk_parent_data parents_apcs_alias0_clk_src[] = {
> >>>> + { .fw_name = "xo" },
> >>>> + { .fw_name = "pll" },
> >>> This pll clk is not described in the binding. Please add it there.
> >> Sorry I did not get this, this PLL is not directly defined in this
> >> driver and it comes
> >>
> >> from dts. do you still want to describe it in binding?
> >>
> > Yes, there should be a clock-names property for "pll" and a clocks
> > property in the binding document. I didn't see that.
>
> These are defined in
>
> https://lkml.org/lkml/2020/5/27/658and
>
> https://lkml.org/lkml/2020/5/27/659
>
> it has been defined as part of mailbox binding, since this driver does
>
> not have a dts node and it is child of apcs mailbox driver.
>
Ah alright. Sounds good.
^ permalink raw reply
* Re: [PATCH] h8300: dts: Fix /chosen:stdout-path
From: Masahiro Yamada @ 2020-06-03 5:46 UTC (permalink / raw)
To: Geert Uytterhoeven, Rob Herring
Cc: Yoshinori Sato, DTML, moderated list:H8/300 ARCHITECTURE,
Linux Kernel Mailing List
In-Reply-To: <20200325095326.10875-1-geert+renesas@glider.be>
On Wed, Mar 25, 2020 at 6:53 PM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
>
> arch/h8300/boot/dts/h8s_sim.dts:11.3-25: Warning (chosen_node_stdout_path): /chosen:stdout-path: property is not a string
> arch/h8300/boot/dts/h8300h_sim.dts:11.3-25: Warning (chosen_node_stdout_path): /chosen:stdout-path: property is not a string
>
> Drop the angle brackets to fix this.
>
> A similar fix was already applied to arch/h8300/boot/dts/edosk2674.dts
> in commit 780ffcd51cb28717 ("h8300: register address fix").
>
> Fixes: 38d6bded13084d50 ("h8300: devicetree source")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Masahiro Yamada <masahiroy@kernel.org>
Unfortunately, h8300 maintainer is not responding...
How to get this in?
Perhaps, Rob can pick this up?
Thanks.
> ---
> arch/h8300/boot/dts/h8300h_sim.dts | 2 +-
> arch/h8300/boot/dts/h8s_sim.dts | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/h8300/boot/dts/h8300h_sim.dts b/arch/h8300/boot/dts/h8300h_sim.dts
> index 595398b9d0180a80..e1d4d9b7f6b40c04 100644
> --- a/arch/h8300/boot/dts/h8300h_sim.dts
> +++ b/arch/h8300/boot/dts/h8300h_sim.dts
> @@ -8,7 +8,7 @@
>
> chosen {
> bootargs = "earlyprintk=h8300-sim";
> - stdout-path = <&sci0>;
> + stdout-path = &sci0;
> };
> aliases {
> serial0 = &sci0;
> diff --git a/arch/h8300/boot/dts/h8s_sim.dts b/arch/h8300/boot/dts/h8s_sim.dts
> index 932cc3c5a81bcdd2..4848e40e607ecc1d 100644
> --- a/arch/h8300/boot/dts/h8s_sim.dts
> +++ b/arch/h8300/boot/dts/h8s_sim.dts
> @@ -8,7 +8,7 @@
>
> chosen {
> bootargs = "earlyprintk=h8300-sim";
> - stdout-path = <&sci0>;
> + stdout-path = &sci0;
> };
> aliases {
> serial0 = &sci0;
> --
> 2.17.1
>
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* [PATCH 3/3] dt-bindings: spi: Convert imx lpspi to json-schema
From: Anson Huang @ 2020-06-03 6:13 UTC (permalink / raw)
To: broonie, robh+dt, shawnguo, s.hauer, kernel, festevam, marex,
linux-spi, devicetree, linux-arm-kernel, linux-kernel
Cc: Linux-imx
In-Reply-To: <1591164809-13964-1-git-send-email-Anson.Huang@nxp.com>
Convert the i.MX LPSPI binding to DT schema format using json-schema
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
.../devicetree/bindings/spi/spi-fsl-lpspi.txt | 29 -----------
.../devicetree/bindings/spi/spi-fsl-lpspi.yaml | 60 ++++++++++++++++++++++
2 files changed, 60 insertions(+), 29 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/spi/spi-fsl-lpspi.txt
create mode 100644 Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
diff --git a/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.txt b/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.txt
deleted file mode 100644
index e71b81a..0000000
--- a/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.txt
+++ /dev/null
@@ -1,29 +0,0 @@
-* Freescale Low Power SPI (LPSPI) for i.MX
-
-Required properties:
-- compatible :
- - "fsl,imx7ulp-spi" for LPSPI compatible with the one integrated on i.MX7ULP soc
- - "fsl,imx8qxp-spi" for LPSPI compatible with the one integrated on i.MX8QXP soc
-- reg : address and length of the lpspi master registers
-- interrupt-parent : core interrupt controller
-- interrupts : lpspi interrupt
-- clocks : lpspi clock specifier. Its number and order need to correspond to the
- value in clock-names.
-- clock-names : Corresponding to per clock and ipg clock in "clocks"
- respectively. In i.MX7ULP, it only has per clk, so use CLK_DUMMY
- to fill the "ipg" blank.
-- spi-slave : spi slave mode support. In slave mode, add this attribute without
- value. In master mode, remove it.
-
-Examples:
-
-lpspi2: lpspi@40290000 {
- compatible = "fsl,imx7ulp-spi";
- reg = <0x40290000 0x10000>;
- interrupt-parent = <&intc>;
- interrupts = <GIC_SPI 28 IRQ_TYPE_LEVEL_HIGH>;
- clocks = <&clks IMX7ULP_CLK_LPSPI2>,
- <&clks IMX7ULP_CLK_DUMMY>;
- clock-names = "per", "ipg";
- spi-slave;
-};
diff --git a/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml b/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
new file mode 100644
index 0000000..d18e2b0f
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
@@ -0,0 +1,60 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/spi/spi-fsl-lpspi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Freescale Low Power SPI (LPSPI) for i.MX
+
+maintainers:
+ - Anson Huang <Anson.Huang@nxp.com>
+
+allOf:
+ - $ref: "/schemas/spi/spi-controller.yaml#"
+
+properties:
+ compatible:
+ enum:
+ - fsl,imx7ulp-spi
+ - fsl,imx8qxp-spi
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ items:
+ - description: SoC SPI per clock
+ - description: SoC SPI ipg clock
+ maxItems: 2
+
+ clock-names:
+ items:
+ - const: per
+ - const: ipg
+ maxItems: 2
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - clocks
+ - clock-names
+
+examples:
+ - |
+ #include <dt-bindings/clock/imx7ulp-clock.h>
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+ spi@40290000 {
+ compatible = "fsl,imx7ulp-spi";
+ reg = <0x40290000 0x10000>;
+ interrupt-parent = <&intc>;
+ interrupts = <GIC_SPI 28 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clks IMX7ULP_CLK_LPSPI2>,
+ <&clks IMX7ULP_CLK_DUMMY>;
+ clock-names = "per", "ipg";
+ spi-slave;
+ };
--
2.7.4
^ permalink raw reply related
* [PATCH 2/3] dt-bindings: spi: Convert imx cspi to json-schema
From: Anson Huang @ 2020-06-03 6:13 UTC (permalink / raw)
To: broonie, robh+dt, shawnguo, s.hauer, kernel, festevam, marex,
linux-spi, devicetree, linux-arm-kernel, linux-kernel
Cc: Linux-imx
In-Reply-To: <1591164809-13964-1-git-send-email-Anson.Huang@nxp.com>
Convert the i.MX CSPI binding to DT schema format using json-schema,
update compatible, remove obsolete properties "fsl,spi-num-chipselects"
and update the example based on latest DT file.
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
.../devicetree/bindings/spi/fsl-imx-cspi.txt | 56 -------------
.../devicetree/bindings/spi/fsl-imx-cspi.yaml | 97 ++++++++++++++++++++++
2 files changed, 97 insertions(+), 56 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt
create mode 100644 Documentation/devicetree/bindings/spi/fsl-imx-cspi.yaml
diff --git a/Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt b/Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt
deleted file mode 100644
index 33bc58f..0000000
--- a/Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt
+++ /dev/null
@@ -1,56 +0,0 @@
-* Freescale (Enhanced) Configurable Serial Peripheral Interface
- (CSPI/eCSPI) for i.MX
-
-Required properties:
-- compatible :
- - "fsl,imx1-cspi" for SPI compatible with the one integrated on i.MX1
- - "fsl,imx21-cspi" for SPI compatible with the one integrated on i.MX21
- - "fsl,imx27-cspi" for SPI compatible with the one integrated on i.MX27
- - "fsl,imx31-cspi" for SPI compatible with the one integrated on i.MX31
- - "fsl,imx35-cspi" for SPI compatible with the one integrated on i.MX35
- - "fsl,imx51-ecspi" for SPI compatible with the one integrated on i.MX51
- - "fsl,imx53-ecspi" for SPI compatible with the one integrated on i.MX53 and later Soc
- - "fsl,imx8mq-ecspi" for SPI compatible with the one integrated on i.MX8MQ
- - "fsl,imx8mm-ecspi" for SPI compatible with the one integrated on i.MX8MM
- - "fsl,imx8mn-ecspi" for SPI compatible with the one integrated on i.MX8MN
- - "fsl,imx8mp-ecspi" for SPI compatible with the one integrated on i.MX8MP
-- reg : Offset and length of the register set for the device
-- interrupts : Should contain CSPI/eCSPI interrupt
-- clocks : Clock specifiers for both ipg and per clocks.
-- clock-names : Clock names should include both "ipg" and "per"
-See the clock consumer binding,
- Documentation/devicetree/bindings/clock/clock-bindings.txt
-
-Recommended properties:
-- cs-gpios : GPIOs to use as chip selects, see spi-bus.txt. While the native chip
-select lines can be used, they appear to always generate a pulse between each
-word of a transfer. Most use cases will require GPIO based chip selects to
-generate a valid transaction.
-
-Optional properties:
-- num-cs : Number of total chip selects, see spi-bus.txt.
-- dmas: DMA specifiers for tx and rx dma. See the DMA client binding,
-Documentation/devicetree/bindings/dma/dma.txt.
-- dma-names: DMA request names, if present, should include "tx" and "rx".
-- fsl,spi-rdy-drctl: Integer, representing the value of DRCTL, the register
-controlling the SPI_READY handling. Note that to enable the DRCTL consideration,
-the SPI_READY mode-flag needs to be set too.
-Valid values are: 0 (disabled), 1 (edge-triggered burst) and 2 (level-triggered burst).
-
-Obsolete properties:
-- fsl,spi-num-chipselects : Contains the number of the chipselect
-
-Example:
-
-ecspi@70010000 {
- #address-cells = <1>;
- #size-cells = <0>;
- compatible = "fsl,imx51-ecspi";
- reg = <0x70010000 0x4000>;
- interrupts = <36>;
- cs-gpios = <&gpio3 24 0>, /* GPIO3_24 */
- <&gpio3 25 0>; /* GPIO3_25 */
- dmas = <&sdma 3 7 1>, <&sdma 4 7 2>;
- dma-names = "rx", "tx";
- fsl,spi-rdy-drctl = <1>;
-};
diff --git a/Documentation/devicetree/bindings/spi/fsl-imx-cspi.yaml b/Documentation/devicetree/bindings/spi/fsl-imx-cspi.yaml
new file mode 100644
index 0000000..cac023d
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/fsl-imx-cspi.yaml
@@ -0,0 +1,97 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/spi/fsl-imx-cspi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Freescale (Enhanced) Configurable Serial Peripheral Interface (CSPI/eCSPI) for i.MX
+
+maintainers:
+ - Shawn Guo <shawn.guo@linaro.org>
+
+allOf:
+ - $ref: "/schemas/spi/spi-controller.yaml#"
+
+properties:
+ compatible:
+ oneOf:
+ - const: fsl,imx1-cspi
+ - const: fsl,imx21-cspi
+ - const: fsl,imx27-cspi
+ - const: fsl,imx31-cspi
+ - const: fsl,imx35-cspi
+ - const: fsl,imx51-ecspi
+ - const: fsl,imx53-ecspi
+ - items:
+ - enum:
+ - fsl,imx50-ecspi
+ - fsl,imx6q-ecspi
+ - fsl,imx6sx-ecspi
+ - fsl,imx6sl-ecspi
+ - fsl,imx6sll-ecspi
+ - fsl,imx6ul-ecspi
+ - fsl,imx7d-ecspi
+ - fsl,imx8mq-ecspi
+ - fsl,imx8mm-ecspi
+ - fsl,imx8mn-ecspi
+ - fsl,imx8mp-ecspi
+ - const: fsl,imx51-ecspi
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ items:
+ - description: SoC SPI ipg clock
+ - description: SoC SPI per clock
+ maxItems: 2
+
+ clock-names:
+ items:
+ - const: ipg
+ - const: per
+ maxItems: 2
+
+ dmas:
+ items:
+ - description: DMA controller phandle and request line for RX
+ - description: DMA controller phandle and request line for TX
+
+ dma-names:
+ items:
+ - const: rx
+ - const: tx
+
+ fsl,spi-rdy-drctl:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: |
+ Integer, representing the value of DRCTL, the register controlling
+ the SPI_READY handling. Note that to enable the DRCTL consideration,
+ the SPI_READY mode-flag needs to be set too.
+ Valid values are: 0 (disabled), 1 (edge-triggered burst) and 2 (level-triggered burst).
+ enum: [0, 1, 2]
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - clocks
+ - clock-names
+
+examples:
+ - |
+ #include <dt-bindings/clock/imx5-clock.h>
+
+ spi@70010000 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "fsl,imx51-ecspi";
+ reg = <0x70010000 0x4000>;
+ interrupts = <36>;
+ clocks = <&clks IMX5_CLK_ECSPI1_IPG_GATE>,
+ <&clks IMX5_CLK_ECSPI1_PER_GATE>;
+ clock-names = "ipg", "per";
+ };
--
2.7.4
^ permalink raw reply related
* [PATCH 1/3] dt-bindings: spi: Convert mxs spi to json-schema
From: Anson Huang @ 2020-06-03 6:13 UTC (permalink / raw)
To: broonie, robh+dt, shawnguo, s.hauer, kernel, festevam, marex,
linux-spi, devicetree, linux-arm-kernel, linux-kernel
Cc: Linux-imx
In-Reply-To: <1591164809-13964-1-git-send-email-Anson.Huang@nxp.com>
Convert the MXS SPI binding to DT schema format using json-schema
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
Documentation/devicetree/bindings/spi/mxs-spi.txt | 26 ----------
Documentation/devicetree/bindings/spi/mxs-spi.yaml | 55 ++++++++++++++++++++++
2 files changed, 55 insertions(+), 26 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/spi/mxs-spi.txt
create mode 100644 Documentation/devicetree/bindings/spi/mxs-spi.yaml
diff --git a/Documentation/devicetree/bindings/spi/mxs-spi.txt b/Documentation/devicetree/bindings/spi/mxs-spi.txt
deleted file mode 100644
index 3499b73..0000000
--- a/Documentation/devicetree/bindings/spi/mxs-spi.txt
+++ /dev/null
@@ -1,26 +0,0 @@
-* Freescale MX233/MX28 SSP/SPI
-
-Required properties:
-- compatible: Should be "fsl,<soc>-spi", where soc is "imx23" or "imx28"
-- reg: Offset and length of the register set for the device
-- interrupts: Should contain SSP ERROR interrupt
-- dmas: DMA specifier, consisting of a phandle to DMA controller node
- and SSP DMA channel ID.
- Refer to dma.txt and fsl-mxs-dma.txt for details.
-- dma-names: Must be "rx-tx".
-
-Optional properties:
-- clock-frequency : Input clock frequency to the SPI block in Hz.
- Default is 160000000 Hz.
-
-Example:
-
-ssp0: ssp@80010000 {
- #address-cells = <1>;
- #size-cells = <0>;
- compatible = "fsl,imx28-spi";
- reg = <0x80010000 0x2000>;
- interrupts = <96>;
- dmas = <&dma_apbh 0>;
- dma-names = "rx-tx";
-};
diff --git a/Documentation/devicetree/bindings/spi/mxs-spi.yaml b/Documentation/devicetree/bindings/spi/mxs-spi.yaml
new file mode 100644
index 0000000..ef99d12
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/mxs-spi.yaml
@@ -0,0 +1,55 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/spi/mxs-spi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Freescale MX233/MX28 SSP/SPI
+
+maintainers:
+ - Marek Vasut <marex@denx.de>
+
+allOf:
+ - $ref: "/schemas/spi/spi-controller.yaml#"
+
+properties:
+ compatible:
+ enum:
+ - fsl,imx23-spi
+ - fsl,imx28-spi
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ dmas:
+ maxItems: 1
+
+ dma-names:
+ const: rx-tx
+
+ clock-frequency:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: input clock frequency to the SPI block in Hz.
+ default: 160000000
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - dmas
+ - dma-names
+
+examples:
+ - |
+ spi@80010000 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "fsl,imx28-spi";
+ reg = <0x80010000 0x2000>;
+ interrupts = <96>;
+ dmas = <&dma_apbh 0>;
+ dma-names = "rx-tx";
+ };
--
2.7.4
^ permalink raw reply related
* [PATCH 0/3] Convert mxs/imx spi/cspi/lpspi binding to json-schema
From: Anson Huang @ 2020-06-03 6:13 UTC (permalink / raw)
To: broonie, robh+dt, shawnguo, s.hauer, kernel, festevam, marex,
linux-spi, devicetree, linux-arm-kernel, linux-kernel
Cc: Linux-imx
This patch series converts mxs/imx spi/cspi/lpspi binding to json-schema.
In fsl-imx-cspi.yaml, also update compatible, remove obsolete properties
"fsl,spi-num-chipselects" and update the example based on latest DT file;
In spi-fsl-lpspi.yaml, the original maintainer's email address
pandy.gao@nxp.com is no longer valid, so I use mine.
Anson Huang (3):
dt-bindings: spi: Convert mxs spi to json-schema
dt-bindings: spi: Convert imx cspi to json-schema
dt-bindings: spi: Convert imx lpspi to json-schema
.../devicetree/bindings/spi/fsl-imx-cspi.txt | 56 -------------
.../devicetree/bindings/spi/fsl-imx-cspi.yaml | 97 ++++++++++++++++++++++
Documentation/devicetree/bindings/spi/mxs-spi.txt | 26 ------
Documentation/devicetree/bindings/spi/mxs-spi.yaml | 55 ++++++++++++
.../devicetree/bindings/spi/spi-fsl-lpspi.txt | 29 -------
.../devicetree/bindings/spi/spi-fsl-lpspi.yaml | 60 +++++++++++++
6 files changed, 212 insertions(+), 111 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt
create mode 100644 Documentation/devicetree/bindings/spi/fsl-imx-cspi.yaml
delete mode 100644 Documentation/devicetree/bindings/spi/mxs-spi.txt
create mode 100644 Documentation/devicetree/bindings/spi/mxs-spi.yaml
delete mode 100644 Documentation/devicetree/bindings/spi/spi-fsl-lpspi.txt
create mode 100644 Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
--
2.7.4
^ permalink raw reply
* Re: Security Random Number Generator support
From: Neal Liu @ 2020-06-03 7:29 UTC (permalink / raw)
To: Marc Zyngier, Julius Werner, Ard Biesheuvel
Cc: Neal Liu,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Herbert Xu, Arnd Bergmann, Greg Kroah-Hartman, Sean Wang, lkml,
wsd_upstream, Crystal Guo (郭晶), Rob Herring,
Linux Crypto Mailing List, Matt Mackall, Matthias Brugger,
linux-mediatek@lists.infradead.org, Linux ARM
In-Reply-To: <85dfc0142d3879d50c0ba18bcc71e199@misterjones.org>
On Tue, 2020-06-02 at 21:02 +0800, Marc Zyngier wrote:
> On 2020-06-02 13:14, Ard Biesheuvel wrote:
> > On Tue, 2 Jun 2020 at 10:15, Neal Liu <neal.liu@mediatek.com> wrote:
> >>
> >> These patch series introduce a security random number generator
> >> which provides a generic interface to get hardware rnd from Secure
> >> state. The Secure state can be Arm Trusted Firmware(ATF), Trusted
> >> Execution Environment(TEE), or even EL2 hypervisor.
> >>
> >> Patch #1..2 adds sec-rng kernel driver for Trustzone based SoCs.
> >> For security awareness SoCs on ARMv8 with TrustZone enabled,
> >> peripherals like entropy sources is not accessible from normal world
> >> (linux) and rather accessible from secure world (HYP/ATF/TEE) only.
> >> This driver aims to provide a generic interface to Arm Trusted
> >> Firmware or Hypervisor rng service.
> >>
> >>
> >> changes since v1:
> >> - rename mt67xx-rng to mtk-sec-rng since all MediaTek ARMv8 SoCs can
> >> reuse
> >> this driver.
> >> - refine coding style and unnecessary check.
> >>
> >> changes since v2:
> >> - remove unused comments.
> >> - remove redundant variable.
> >>
> >> changes since v3:
> >> - add dt-bindings for MediaTek rng with TrustZone enabled.
> >> - revise HWRNG SMC call fid.
> >>
> >> changes since v4:
> >> - move bindings to the arm/firmware directory.
> >> - revise driver init flow to check more property.
> >>
> >> changes since v5:
> >> - refactor to more generic security rng driver which
> >> is not platform specific.
> >>
> >> *** BLURB HERE ***
> >>
> >> Neal Liu (2):
> >> dt-bindings: rng: add bindings for sec-rng
> >> hwrng: add sec-rng driver
> >>
> >
> > There is no reason to model a SMC call as a driver, and represent it
> > via a DT node like this.
>
> +1.
>
> > It would be much better if this SMC interface is made truly generic,
> > and wired into the arch_get_random() interface, which can be used much
> > earlier.
>
> Wasn't there a plan to standardize a SMC call to rule them all?
>
> M.
Could you give us a hint how to make this SMC interface more generic in
addition to my approach?
There is no (easy) way to get platform-independent SMC function ID,
which is why we encode it into device tree, and provide a generic
driver. In this way, different devices can be mapped and then get
different function ID internally.
^ permalink raw reply
* Re: [PATCH v7 21/24] iommu/arm-smmu-v3: Add stall support for platform devices
From: Jean-Philippe Brucker @ 2020-06-03 7:38 UTC (permalink / raw)
To: Shameerali Kolothum Thodi
Cc: devicetree@vger.kernel.org, kevin.tian@intel.com,
fenghua.yu@intel.com, linux-pci@vger.kernel.org,
felix.kuehling@amd.com, robin.murphy@arm.com,
christian.koenig@amd.com, hch@infradead.org, jgg@ziepe.ca,
iommu@lists.linux-foundation.org, catalin.marinas@arm.com,
zhangfei.gao@linaro.org, will@kernel.org, linux-mm@kvack.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <c165fe41230f49baba991f1a416a4739@huawei.com>
On Tue, Jun 02, 2020 at 12:12:30PM +0000, Shameerali Kolothum Thodi wrote:
> > > > > > + if (ssid_valid)
> > > > > > + flt->prm.flags |=
> > > > IOMMU_FAULT_PAGE_REQUEST_PASID_VALID;
> > > > >
> > > > > Do we need to set this for STALL mode only support? I had an issue
> > > > > with this being set on a vSVA POC based on our D06 zip
> > > > > device(which is a "fake " pci dev that supports STALL mode but no
> > > > > PRI). The issue is, CMDQ_OP_RESUME doesn't have any ssid or SSV
> > > > > params and works on sid
> > > > and stag only.
> > > >
> > > > I don't understand the problem, arm_smmu_page_response() doesn't set
> > > > SSID or SSV when sending a CMDQ_OP_RESUME. Could you detail the flow
> > > > of a stall event and RESUME command in your prototype? Are you
> > > > getting issues with the host driver or the guest driver?
> > >
> > > The issue is on the host side iommu_page_response(). The flow is
> > > something like below.
> > >
> > > Stall: Host:-
> > >
> > > arm_smmu_handle_evt()
> > > iommu_report_device_fault()
> > > vfio_pci_iommu_dev_fault_handler()
> > >
> > > Stall: Qemu:-
> > >
> > > vfio_dma_fault_notifier_handler()
> > > inject_faults()
> > > smmuv3_inject_faults()
> > >
> > > Stall: Guest:-
> > >
> > > arm_smmu_handle_evt()
> > > iommu_report_device_fault()
> > > iommu_queue_iopf
> > > ...
> > > iopf_handle_group()
> > > iopf_handle_single()
> > > handle_mm_fault()
> > > iopf_complete()
> > > iommu_page_response()
> > > arm_smmu_page_response()
> > > arm_smmu_cmdq_issue_cmd(CMDQ_OP_RESUME)
> > >
> > > Resume: Qemu:-
> > >
> > > smmuv3_cmdq_consume(SMMU_CMD_RESUME)
> > > smmuv3_notify_page_resp()
> > > vfio:ioctl(page_response) --> struct iommu_page_response is filled
> > > with only version, grpid and code.
> > >
> > > Resume: Host:-
> > > ioctl(page_response)
> > > iommu_page_response() --> fails as the pending req has PASID_VALID
> > flag
> > > set and it checks for a match.
> >
> > I believe the fix needs to be here. It's also wrong for PRI since not all PCIe
> > endpoint require a PASID in the page response. Could you try the attached
> > patch?
>
> Going through the patch, yes, that will definitely fix the issue. But isn't it better if
> the request itself indicate whether it expects a response msg with a valid pasid or
> not? The response msg can come from userspace as well(vSVA) and if for some reason
> doesn't set it for a req that expects pasid then it should be an error, right? In the temp
> fix I had, I introduced another flag to indicate the endpoint has PRI support or not and
> used that to verify the pasid requirement. But for the PRI case you mentioned
> above, not sure it is easy to get that information or not. May be I am complicating things
> here :)
No you're right, we shouldn't send back malformed responses to the SMMU. I
suppose we can store a flag "PASID required" in the fault and check that
against the response. If we have to discard the guest's response, then we
can either fake a response (abort the stall) right away, or wait for the
response timeout to kick, which will do the same.
Thanks,
Jean
^ permalink raw reply
* Re: Security Random Number Generator support
From: Marc Zyngier @ 2020-06-03 7:40 UTC (permalink / raw)
To: Neal Liu
Cc: Julius Werner, Ard Biesheuvel,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Herbert Xu, Arnd Bergmann, Greg Kroah-Hartman, Sean Wang,
linux-mediatek, lkml, wsd_upstream, Rob Herring,
Linux Crypto Mailing List, Matt Mackall, Matthias Brugger,
Crystal Guo (郭晶), Linux ARM
In-Reply-To: <1591169342.4878.9.camel@mtkswgap22>
On 2020-06-03 08:29, Neal Liu wrote:
> On Tue, 2020-06-02 at 21:02 +0800, Marc Zyngier wrote:
>> On 2020-06-02 13:14, Ard Biesheuvel wrote:
>> > On Tue, 2 Jun 2020 at 10:15, Neal Liu <neal.liu@mediatek.com> wrote:
>> >>
>> >> These patch series introduce a security random number generator
>> >> which provides a generic interface to get hardware rnd from Secure
>> >> state. The Secure state can be Arm Trusted Firmware(ATF), Trusted
>> >> Execution Environment(TEE), or even EL2 hypervisor.
>> >>
>> >> Patch #1..2 adds sec-rng kernel driver for Trustzone based SoCs.
>> >> For security awareness SoCs on ARMv8 with TrustZone enabled,
>> >> peripherals like entropy sources is not accessible from normal world
>> >> (linux) and rather accessible from secure world (HYP/ATF/TEE) only.
>> >> This driver aims to provide a generic interface to Arm Trusted
>> >> Firmware or Hypervisor rng service.
>> >>
>> >>
>> >> changes since v1:
>> >> - rename mt67xx-rng to mtk-sec-rng since all MediaTek ARMv8 SoCs can
>> >> reuse
>> >> this driver.
>> >> - refine coding style and unnecessary check.
>> >>
>> >> changes since v2:
>> >> - remove unused comments.
>> >> - remove redundant variable.
>> >>
>> >> changes since v3:
>> >> - add dt-bindings for MediaTek rng with TrustZone enabled.
>> >> - revise HWRNG SMC call fid.
>> >>
>> >> changes since v4:
>> >> - move bindings to the arm/firmware directory.
>> >> - revise driver init flow to check more property.
>> >>
>> >> changes since v5:
>> >> - refactor to more generic security rng driver which
>> >> is not platform specific.
>> >>
>> >> *** BLURB HERE ***
>> >>
>> >> Neal Liu (2):
>> >> dt-bindings: rng: add bindings for sec-rng
>> >> hwrng: add sec-rng driver
>> >>
>> >
>> > There is no reason to model a SMC call as a driver, and represent it
>> > via a DT node like this.
>>
>> +1.
>>
>> > It would be much better if this SMC interface is made truly generic,
>> > and wired into the arch_get_random() interface, which can be used much
>> > earlier.
>>
>> Wasn't there a plan to standardize a SMC call to rule them all?
>>
>> M.
>
> Could you give us a hint how to make this SMC interface more generic in
> addition to my approach?
> There is no (easy) way to get platform-independent SMC function ID,
> which is why we encode it into device tree, and provide a generic
> driver. In this way, different devices can be mapped and then get
> different function ID internally.
The idea is simply to have *one* single ID that caters for all
implementations, just like we did for PSCI at the time. This
requires ARM to edict a standard, which is what I was referring
to above.
There is zero benefit in having a platform-dependent ID. It just
pointlessly increases complexity, and means we cannot use the RNG
before the firmware tables are available (yes, we need it that
early).
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* Re: Security Random Number Generator support
From: Neal Liu @ 2020-06-03 7:54 UTC (permalink / raw)
To: Marc Zyngier
Cc: Neal Liu, Julius Werner, Ard Biesheuvel,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Herbert Xu, Arnd Bergmann, Greg Kroah-Hartman, Sean Wang,
linux-mediatek, lkml, wsd_upstream, Rob Herring,
Linux Crypto Mailing List, Matt Mackall, Matthias Brugger,
Crystal Guo (郭晶), Linux ARM
In-Reply-To: <fcbe37f6f9cbcde24f9c28bc504f1f0e@kernel.org>
On Wed, 2020-06-03 at 08:40 +0100, Marc Zyngier wrote:
> On 2020-06-03 08:29, Neal Liu wrote:
> > On Tue, 2020-06-02 at 21:02 +0800, Marc Zyngier wrote:
> >> On 2020-06-02 13:14, Ard Biesheuvel wrote:
> >> > On Tue, 2 Jun 2020 at 10:15, Neal Liu <neal.liu@mediatek.com> wrote:
> >> >>
> >> >> These patch series introduce a security random number generator
> >> >> which provides a generic interface to get hardware rnd from Secure
> >> >> state. The Secure state can be Arm Trusted Firmware(ATF), Trusted
> >> >> Execution Environment(TEE), or even EL2 hypervisor.
> >> >>
> >> >> Patch #1..2 adds sec-rng kernel driver for Trustzone based SoCs.
> >> >> For security awareness SoCs on ARMv8 with TrustZone enabled,
> >> >> peripherals like entropy sources is not accessible from normal world
> >> >> (linux) and rather accessible from secure world (HYP/ATF/TEE) only.
> >> >> This driver aims to provide a generic interface to Arm Trusted
> >> >> Firmware or Hypervisor rng service.
> >> >>
> >> >>
> >> >> changes since v1:
> >> >> - rename mt67xx-rng to mtk-sec-rng since all MediaTek ARMv8 SoCs can
> >> >> reuse
> >> >> this driver.
> >> >> - refine coding style and unnecessary check.
> >> >>
> >> >> changes since v2:
> >> >> - remove unused comments.
> >> >> - remove redundant variable.
> >> >>
> >> >> changes since v3:
> >> >> - add dt-bindings for MediaTek rng with TrustZone enabled.
> >> >> - revise HWRNG SMC call fid.
> >> >>
> >> >> changes since v4:
> >> >> - move bindings to the arm/firmware directory.
> >> >> - revise driver init flow to check more property.
> >> >>
> >> >> changes since v5:
> >> >> - refactor to more generic security rng driver which
> >> >> is not platform specific.
> >> >>
> >> >> *** BLURB HERE ***
> >> >>
> >> >> Neal Liu (2):
> >> >> dt-bindings: rng: add bindings for sec-rng
> >> >> hwrng: add sec-rng driver
> >> >>
> >> >
> >> > There is no reason to model a SMC call as a driver, and represent it
> >> > via a DT node like this.
> >>
> >> +1.
> >>
> >> > It would be much better if this SMC interface is made truly generic,
> >> > and wired into the arch_get_random() interface, which can be used much
> >> > earlier.
> >>
> >> Wasn't there a plan to standardize a SMC call to rule them all?
> >>
> >> M.
> >
> > Could you give us a hint how to make this SMC interface more generic in
> > addition to my approach?
> > There is no (easy) way to get platform-independent SMC function ID,
> > which is why we encode it into device tree, and provide a generic
> > driver. In this way, different devices can be mapped and then get
> > different function ID internally.
>
> The idea is simply to have *one* single ID that caters for all
> implementations, just like we did for PSCI at the time. This
> requires ARM to edict a standard, which is what I was referring
> to above.
>
> There is zero benefit in having a platform-dependent ID. It just
> pointlessly increases complexity, and means we cannot use the RNG
> before the firmware tables are available (yes, we need it that
> early).
>
> M.
Do you know which ARM expert could edict this standard?
Or is there any chance that we can make one? And be reviewed by
maintainers?
^ permalink raw reply
* Re: [PATCH v3 06/10] arm64: dts: actions: Add DMA Controller for S700
From: Manivannan Sadhasivam @ 2020-06-03 8:30 UTC (permalink / raw)
To: Amit Singh Tomar, andre.przywara, afaerber, vkoul, robh+dt
Cc: dan.j.williams, cristian.ciocaltea, linux-kernel,
linux-arm-kernel, linux-actions, devicetree
In-Reply-To: <1591119192-18538-7-git-send-email-amittomer25@gmail.com>
On 2 June 2020 11:03:08 PM IST, Amit Singh Tomar <amittomer25@gmail.com> wrote:
>This commit adds DAM controller present on Actions S700, it differs
DMA
>from
>S900 in terms of number of dma channels and requests.
>
>Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com>
>---
>Changes since v2:
> * added power-domain property as sps
> is enabled now and DMA needs it.
>Changes since v1:
> * No Change.
>Changes since RFC:
> * No Change.
>---
> arch/arm64/boot/dts/actions/s700.dtsi | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
>diff --git a/arch/arm64/boot/dts/actions/s700.dtsi
>b/arch/arm64/boot/dts/actions/s700.dtsi
>index f8eb72bb4125..605594dd7a0e 100644
>--- a/arch/arm64/boot/dts/actions/s700.dtsi
>+++ b/arch/arm64/boot/dts/actions/s700.dtsi
>@@ -6,6 +6,7 @@
> #include <dt-bindings/clock/actions,s700-cmu.h>
> #include <dt-bindings/interrupt-controller/arm-gic.h>
> #include <dt-bindings/reset/actions,s700-reset.h>
>+#include <dt-bindings/power/owl-s700-powergate.h>
Sort this alphabetically.
Thanks,
Mani
>
> / {
> compatible = "actions,s700";
>@@ -244,5 +245,19 @@
> <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
> };
>+
>+ dma: dma-controller@e0230000 {
>+ compatible = "actions,s700-dma";
>+ reg = <0x0 0xe0230000 0x0 0x1000>;
>+ interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>,
>+ <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>,
>+ <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>,
>+ <GIC_SPI 60 IRQ_TYPE_LEVEL_HIGH>;
>+ #dma-cells = <1>;
>+ dma-channels = <10>;
>+ dma-requests = <44>;
>+ clocks = <&cmu CLK_DMAC>;
>+ power-domains = <&sps S700_PD_DMA>;
>+ };
> };
> };
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* Re: (EXT) [PATCH v8 00/13] add ecspi ERR009165 for i.mx6/7 soc family
From: Matthias Schiffer @ 2020-06-03 8:31 UTC (permalink / raw)
To: Robin Gong
Cc: devicetree, linux-kernel, linux-spi, linux-imx, kernel,
linux-arm-kernel, mark.rutland, broonie, robh+dt, catalin.marinas,
vkoul, will.deacon, shawnguo, festevam, s.hauer, martin.fuzzey,
u.kleine-koenig, dan.j.williams, Markus Niebel
In-Reply-To: <1590006865-20900-1-git-send-email-yibin.gong@nxp.com>
On Thu, 2020-05-21 at 04:34 +0800, Robin Gong wrote:
> There is ecspi ERR009165 on i.mx6/7 soc family, which cause FIFO
> transfer to be send twice in DMA mode. Please get more information
> from:
> https://www.nxp.com/docs/en/errata/IMX6DQCE.pdf. The workaround is
> adding
> new sdma ram script which works in XCH mode as PIO inside sdma
> instead
> of SMC mode, meanwhile, 'TX_THRESHOLD' should be 0. The issue should
> be
> exist on all legacy i.mx6/7 soc family before i.mx6ul.
> NXP fix this design issue from i.mx6ul, so newer chips including
> i.mx6ul/
> 6ull/6sll do not need this workaroud anymore. All other i.mx6/7/8
> chips
> still need this workaroud. This patch set add new 'fsl,imx6ul-ecspi'
> for ecspi driver and 'ecspi_fixed' in sdma driver to choose if need
> errata
> or not.
> The first two reverted patches should be the same issue, though, it
> seems 'fixed' by changing to other shp script. Hope Sean or Sascha
> could
> have the chance to test this patch set if could fix their issues.
> Besides, enable sdma support for i.mx8mm/8mq and fix ecspi1 not work
> on i.mx8mm because the event id is zero.
>
> PS:
> Please get sdma firmware from below linux-firmware and copy it to
> your
> local rootfs /lib/firmware/imx/sdma.
Hello Robin,
we have tried out this series, and there seems to be an issue with the
PIO fallback. We are testing on an i.MX6Q board, and our kernel is a
mostly-unmodified 5.4, on which we backported all SDMA patches from
next-20200602 (imx-sdma.c is identical to next-20200602 version), and
then applied this whole series.
We build the SDMA driver as a kernel module, which is loaded by udev,
so the root filesystem is ready and the SDMA firmware can be loaded.
The behaviour we're seeing is the following:
1. As long as the SDMA driver is not loaded, initializing spi_imx will
be deferred
2. imx_sdma is loaded. The SDMA firmware is not yet loaded at this
point
3. spi_imx is initialized and an SPI-NOR flash is probed. To load the
BFPT, the driver will attempt to use DMA; this will fail with EINVAL as
long as the SDMA firmware is not ready, so the fallback to PIO happens
(4. SDMA firmware is ready, subsequent SPI transfers use DMA)
The problem happens in step 3: Whenever the driver falls back to PIO,
the received data is corrupt. The behaviour is specific to the
fallback: When I disable DMA completely via spi_imx.use_dma, or when
the timing is lucky and the SDMA firmware gets loaded before the flash
is probed, no corruption can be observed.
Kind regards,
Matthias
>
https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/imx/sdma
>
> v2:
> 1.Add commit log for reverted patches.
> 2.Add comment for 'ecspi_fixed' in sdma driver.
> 3.Add 'fsl,imx6sll-ecspi' compatible instead of 'fsl,imx6ul-ecspi'
> rather than remove.
> v3:
> 1.Confirm with design team make sure ERR009165 fixed on
> i.mx6ul/i.mx6ull
> /i.mx6sll, not fixed on i.mx8m/8mm and other i.mx6/7 legacy
> chips.
> Correct dts related dts patch in v2.
> 2.Clean eratta information in binding doc and new 'tx_glitch_fixed'
> flag
> in spi-imx driver to state ERR009165 fixed or not.
> 3.Enlarge burst size to fifo size for tx since tx_wml set to 0 in
> the
> errata workaroud, thus improve performance as possible.
> v4:
> 1.Add Ack tag from Mark and Vinod
> 2.Remove checking 'event_id1' zero as 'event_id0'.
> v5:
> 1.Add the last patch for compatible with the current uart driver
> which
> using rom script, so both uart ram script and rom script
> supported
> in latest firmware, by default uart rom script used. UART driver
> will be broken without this patch.
> v6:
> 1.Resend after rebase the latest next branch.
> 2.Remove below No.13~No.15 patches of v5 because they were
> mergered.
> ARM: dts: imx6ul: add dma support on ecspi
> ARM: dts: imx6sll: correct sdma compatible
> arm64: defconfig: Enable SDMA on i.mx8mq/8mm
> 3.Revert "dmaengine: imx-sdma: fix context cache" since
> 'context_loaded' removed.
> v7:
> 1.Put the last patch 13/13 'Revert "dmaengine: imx-sdma: fix
> context
> cache"' to the ahead of 03/13 'Revert "dmaengine: imx-sdma:
> refine
> to load context only once" so that no building waring during
> comes out
> during bisect.
> 2.Address Sascha's comments, including eliminating any i.mx6sx in
> this
> series, adding new 'is_imx6ul_ecspi()' instead imx in imx51 and
> taking
> care SMC bit for PIO.
> 3.Add back missing 'Reviewed-by' tag on 08/15(v5):09/13(v7)
> 'spi: imx: add new i.mx6ul compatible name in binding doc'
> v8:
> 1.remove 0003-Revert-dmaengine-imx-sdma-fix-context-cache.patch and
> merge
> it into 04/13 of v7
> 2.add 0005-spi-imx-fallback-to-PIO-if-dma-setup-failure.patch for
> no any
> ecspi function broken even if sdma firmware not updated.
> 3.merge 'tx.dst_maxburst' changes in the two continous patches into
> one
> patch to avoid confusion.
> 4.fix typo 'duplicated'.
>
> Robin Gong (13):
> Revert "ARM: dts: imx6q: Use correct SDMA script for SPI5 core"
> Revert "ARM: dts: imx6: Use correct SDMA script for SPI cores"
> Revert "dmaengine: imx-sdma: refine to load context only once"
> dmaengine: imx-sdma: remove duplicated sdma_load_context
> spi: imx: fallback to PIO if dma setup failure
> dmaengine: imx-sdma: add mcu_2_ecspi script
> spi: imx: fix ERR009165
> spi: imx: remove ERR009165 workaround on i.mx6ul
> spi: imx: add new i.mx6ul compatible name in binding doc
> dmaengine: imx-sdma: remove ERR009165 on i.mx6ul
> dma: imx-sdma: add i.mx6ul compatible name
> dmaengine: imx-sdma: fix ecspi1 rx dma not work on i.mx8mm
> dmaengine: imx-sdma: add uart rom script
>
> .../devicetree/bindings/dma/fsl-imx-sdma.txt | 1 +
> .../devicetree/bindings/spi/fsl-imx-cspi.txt | 1 +
> arch/arm/boot/dts/imx6q.dtsi | 2 +-
> arch/arm/boot/dts/imx6qdl.dtsi | 8 +-
> drivers/dma/imx-sdma.c | 67 ++++++++++
> ------
> drivers/spi/spi-imx.c | 92
> +++++++++++++++++++---
> include/linux/platform_data/dma-imx-sdma.h | 8 +-
> 7 files changed, 135 insertions(+), 44 deletions(-)
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox