| Message ID | 20250130-rk3588-trng-submission-v1-2-97ff76568e49@collabora.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | RK3588 Hardware Random Number Generator Driver | expand |
On Thu, Jan 30, 2025 at 05:31:16PM +0100, Nicolas Frattaroli wrote: > The Rockchip RK3588 SoC has two hardware RNGs accessible to the > non-secure world: an RNG in the Crypto IP, and a standalone RNG that is > new to this SoC. > > Add a binding for this new standalone RNG. > > The RNG is capable of firing an interrupt when entropy is ready, but > all known driver implementations choose to poll instead for performance > reasons. Hence, make the interrupt optional, as it may disappear in > future hardware revisions entirely and certainly isn't needed for the > hardware to function. > > The reset is optional as well, as the RNG functions without an explicit > reset. Rockchip's downstream driver does not use the reset at all, > indicating that their engineers have deemed it unnecessary. > > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com> > --- > .../bindings/rng/rockchip,rk3588-rng.yaml | 61 ++++++++++++++++++++++ > MAINTAINERS | 2 + > 2 files changed, 63 insertions(+) > > diff --git a/Documentation/devicetree/bindings/rng/rockchip,rk3588-rng.yaml b/Documentation/devicetree/bindings/rng/rockchip,rk3588-rng.yaml > new file mode 100644 > index 0000000000000000000000000000000000000000..dff843fa4bf9d5704bbcd106398328588d80b02d > --- /dev/null > +++ b/Documentation/devicetree/bindings/rng/rockchip,rk3588-rng.yaml > @@ -0,0 +1,61 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/rng/rockchip,rk3588-rng.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Rockchip RK3588 TRNG > + > +description: True Random Number Generator on Rockchip RK3588 SoC > + > +maintainers: > + - Nicolas Frattaroli <nicolas.frattaroli@collabora.com> > + > +properties: > + compatible: > + enum: > + - rockchip,rk3588-rng > + > + reg: > + maxItems: 1 > + > + clocks: > + items: > + - description: TRNG AHB clock > + > + # Optional, not used by some driver implementations > + interrupts: > + maxItems: 1 > + > + # Optional, hardware works without explicit reset These sorts of comments are not needed, "required" conveys this information. > + resets: > + maxItems: 1 > + > +required: > + - compatible > + - reg > + - clocks > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/clock/rockchip,rk3588-cru.h> > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/interrupt-controller/irq.h> > + #include <dt-bindings/reset/rockchip,rk3588-cru.h> > + bus { > + #address-cells = <2>; > + #size-cells = <2>; > + > + rng@fe378000 { > + compatible = "rockchip,rk3588-rng"; > + reg = <0x0 0xfe378000 0x0 0x200>; > + interrupts = <GIC_SPI 400 IRQ_TYPE_LEVEL_HIGH 0>; > + clocks = <&scmi_clk SCMI_HCLK_SECURE_NS>; > + resets = <&scmi_reset SCMI_SRST_H_TRNG_NS>; > + status = "disabled"; > + }; > + }; > + > +... > diff --git a/MAINTAINERS b/MAINTAINERS > index bc8ce7af3303f747e0ef028e5a7b29b0bbba99f4..7daf9bfeb0cb4e9e594b809012c7aa243b0558ae 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -20420,8 +20420,10 @@ F: include/uapi/linux/rkisp1-config.h > ROCKCHIP RK3568 RANDOM NUMBER GENERATOR SUPPORT > M: Daniel Golle <daniel@makrotopia.org> > M: Aurelien Jarno <aurelien@aurel32.net> > +M: Nicolas Frattaroli <nicolas.frattaroli@collabora.com> ^^ tbh, not really sure this part of the change should be in this patch > S: Maintained > F: Documentation/devicetree/bindings/rng/rockchip,rk3568-rng.yaml > +F: Documentation/devicetree/bindings/rng/rockchip,rk3588-rng.yaml > F: drivers/char/hw_random/rockchip-rng.c > > ROCKCHIP RASTER 2D GRAPHIC ACCELERATION UNIT DRIVER > > -- > 2.48.1 >
On 30/01/2025 17:31, Nicolas Frattaroli wrote: > +title: Rockchip RK3588 TRNG > + > +description: True Random Number Generator on Rockchip RK3588 SoC > + > +maintainers: > + - Nicolas Frattaroli <nicolas.frattaroli@collabora.com> > + > +properties: > + compatible: > + enum: > + - rockchip,rk3588-rng > + > + reg: > + maxItems: 1 > + > + clocks: > + items: > + - description: TRNG AHB clock > + > + # Optional, not used by some driver implementations What driver implementations? Downstream? They do not matter, because they are full of all sort of crap. Can this block have interrupt really disconnected? This is the question you should answer. > + interrupts: > + maxItems: 1 > + > + # Optional, hardware works without explicit reset Just because bootloader did something? With that reasoning nothing is ever required because firmware can abstract it. Either you have there a reset or not. In this particular case your driver is irrelevant. > + resets: > + maxItems: 1 > + > +required: > + - compatible > + - reg > + - clocks > + BTW, there is a binding for Rockchip TRNG, with a bit different clocks so I have feeling yours is incomplete here. > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/clock/rockchip,rk3588-cru.h> > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/interrupt-controller/irq.h> > + #include <dt-bindings/reset/rockchip,rk3588-cru.h> > + bus { > + #address-cells = <2>; > + #size-cells = <2>; > + > + rng@fe378000 { > + compatible = "rockchip,rk3588-rng"; > + reg = <0x0 0xfe378000 0x0 0x200>; > + interrupts = <GIC_SPI 400 IRQ_TYPE_LEVEL_HIGH 0>; > + clocks = <&scmi_clk SCMI_HCLK_SECURE_NS>; > + resets = <&scmi_reset SCMI_SRST_H_TRNG_NS>; > + status = "disabled"; Examples cannot be disabled. > + }; > + }; > + > +... > diff --git a/MAINTAINERS b/MAINTAINERS > index bc8ce7af3303f747e0ef028e5a7b29b0bbba99f4..7daf9bfeb0cb4e9e594b809012c7aa243b0558ae 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -20420,8 +20420,10 @@ F: include/uapi/linux/rkisp1-config.h > ROCKCHIP RK3568 RANDOM NUMBER GENERATOR SUPPORT > M: Daniel Golle <daniel@makrotopia.org> > M: Aurelien Jarno <aurelien@aurel32.net> > +M: Nicolas Frattaroli <nicolas.frattaroli@collabora.com> Like Conor said, this is not really relevant and should be a separate patch. Best regards, Krzysztof
On Friday 31 January 2025 09:30:51 Central European Standard Time Krzysztof Kozlowski wrote: > On 30/01/2025 17:31, Nicolas Frattaroli wrote: > > +title: Rockchip RK3588 TRNG > > + > > +description: True Random Number Generator on Rockchip RK3588 SoC > > + > > +maintainers: > > + - Nicolas Frattaroli <nicolas.frattaroli@collabora.com> > > + > > +properties: > > + compatible: > > + enum: > > + - rockchip,rk3588-rng > > + > > + reg: > > + maxItems: 1 > > + > > + clocks: > > + items: > > + - description: TRNG AHB clock > > + > > + # Optional, not used by some driver implementations > > What driver implementations? Downstream? They do not matter, because > they are full of all sort of crap. Downstream and this very driver in the series. The patch includes a lengthy explanation of why all known implementations have foregone using the interrupt. > Can this block have interrupt really disconnected? This is the question > you should answer. I do not have the gift of prescience. If you want me to make the interrupt required even though it is pointless just because all hardware that implements this that I'm currently aware of does have the interrupt then I will add it as a required property. > > + interrupts: > > + maxItems: 1 > > + > > + # Optional, hardware works without explicit reset > > Just because bootloader did something? With that reasoning nothing is > ever required because firmware can abstract it. Either you have there a > reset or not. In this particular case your driver is irrelevant. No, it's not the bootloader that does it, it's the hardware itself. It's power-on reset. Hardware can reset itself without software, I don't know where the assumption of it being done by the bootloader comes from. > > + resets: > > + maxItems: 1 > > + > > +required: > > + - compatible > > + - reg > > + - clocks > > + > > BTW, there is a binding for Rockchip TRNG, with a bit different clocks > so I have feeling yours is incomplete here. I know and they're not. As the cover letter states, this is different hardware from the other Rockchip TRNG. This hardware only has one clock. It is a separate binding with different clocks because it is different hardware. The patch's message also states this: > [...] and a standalone RNG that is new to this SoC. > > Add a binding for this new standalone RNG. It's a standalone RNG that is new to this SoC. It has different registers, and different clock requirements. > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/clock/rockchip,rk3588-cru.h> > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > + #include <dt-bindings/interrupt-controller/irq.h> > > + #include <dt-bindings/reset/rockchip,rk3588-cru.h> > > + bus { > > + #address-cells = <2>; > > + #size-cells = <2>; > > + > > + rng@fe378000 { > > + compatible = "rockchip,rk3588-rng"; > > + reg = <0x0 0xfe378000 0x0 0x200>; > > + interrupts = <GIC_SPI 400 IRQ_TYPE_LEVEL_HIGH 0>; > > + clocks = <&scmi_clk SCMI_HCLK_SECURE_NS>; > > + resets = <&scmi_reset SCMI_SRST_H_TRNG_NS>; > > + status = "disabled"; > > Examples cannot be disabled. Okay, I will fix this. > > + }; > > + }; > > + > > +... > > diff --git a/MAINTAINERS b/MAINTAINERS > > index > > bc8ce7af3303f747e0ef028e5a7b29b0bbba99f4..7daf9bfeb0cb4e9e594b809012c7aa2 > > 43b0558ae 100644 --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -20420,8 +20420,10 @@ F: include/uapi/linux/rkisp1-config.h > > > > ROCKCHIP RK3568 RANDOM NUMBER GENERATOR SUPPORT > > M: Daniel Golle <daniel@makrotopia.org> > > M: Aurelien Jarno <aurelien@aurel32.net> > > > > +M: Nicolas Frattaroli <nicolas.frattaroli@collabora.com> > > Like Conor said, this is not really relevant and should be a separate patch. Should just the added M be a separate patch or also adding the file to the MAINTAINERS file? Because if the latter is separate then I think checkpatch will yell at me, and if the former is separate then the binding's statement that it is maintained by me is disconnected from what's in the MAINTAINERS file. > > > > Best regards, > Krzysztof
diff --git a/Documentation/devicetree/bindings/rng/rockchip,rk3588-rng.yaml b/Documentation/devicetree/bindings/rng/rockchip,rk3588-rng.yaml new file mode 100644 index 0000000000000000000000000000000000000000..dff843fa4bf9d5704bbcd106398328588d80b02d --- /dev/null +++ b/Documentation/devicetree/bindings/rng/rockchip,rk3588-rng.yaml @@ -0,0 +1,61 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/rng/rockchip,rk3588-rng.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Rockchip RK3588 TRNG + +description: True Random Number Generator on Rockchip RK3588 SoC + +maintainers: + - Nicolas Frattaroli <nicolas.frattaroli@collabora.com> + +properties: + compatible: + enum: + - rockchip,rk3588-rng + + reg: + maxItems: 1 + + clocks: + items: + - description: TRNG AHB clock + + # Optional, not used by some driver implementations + interrupts: + maxItems: 1 + + # Optional, hardware works without explicit reset + resets: + maxItems: 1 + +required: + - compatible + - reg + - clocks + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/rockchip,rk3588-cru.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/reset/rockchip,rk3588-cru.h> + bus { + #address-cells = <2>; + #size-cells = <2>; + + rng@fe378000 { + compatible = "rockchip,rk3588-rng"; + reg = <0x0 0xfe378000 0x0 0x200>; + interrupts = <GIC_SPI 400 IRQ_TYPE_LEVEL_HIGH 0>; + clocks = <&scmi_clk SCMI_HCLK_SECURE_NS>; + resets = <&scmi_reset SCMI_SRST_H_TRNG_NS>; + status = "disabled"; + }; + }; + +... diff --git a/MAINTAINERS b/MAINTAINERS index bc8ce7af3303f747e0ef028e5a7b29b0bbba99f4..7daf9bfeb0cb4e9e594b809012c7aa243b0558ae 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -20420,8 +20420,10 @@ F: include/uapi/linux/rkisp1-config.h ROCKCHIP RK3568 RANDOM NUMBER GENERATOR SUPPORT M: Daniel Golle <daniel@makrotopia.org> M: Aurelien Jarno <aurelien@aurel32.net> +M: Nicolas Frattaroli <nicolas.frattaroli@collabora.com> S: Maintained F: Documentation/devicetree/bindings/rng/rockchip,rk3568-rng.yaml +F: Documentation/devicetree/bindings/rng/rockchip,rk3588-rng.yaml F: drivers/char/hw_random/rockchip-rng.c ROCKCHIP RASTER 2D GRAPHIC ACCELERATION UNIT DRIVER
The Rockchip RK3588 SoC has two hardware RNGs accessible to the non-secure world: an RNG in the Crypto IP, and a standalone RNG that is new to this SoC. Add a binding for this new standalone RNG. The RNG is capable of firing an interrupt when entropy is ready, but all known driver implementations choose to poll instead for performance reasons. Hence, make the interrupt optional, as it may disappear in future hardware revisions entirely and certainly isn't needed for the hardware to function. The reset is optional as well, as the RNG functions without an explicit reset. Rockchip's downstream driver does not use the reset at all, indicating that their engineers have deemed it unnecessary. Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com> --- .../bindings/rng/rockchip,rk3588-rng.yaml | 61 ++++++++++++++++++++++ MAINTAINERS | 2 + 2 files changed, 63 insertions(+)