mbox series

[v6,0/3] hwrng: add hwrng support for Rockchip RK3568

Message ID cover.1720830725.git.daniel@makrotopia.org (mailing list archive)
Headers show
Series hwrng: add hwrng support for Rockchip RK3568 | expand

Message

Daniel Golle July 13, 2024, 12:37 a.m. UTC 
Rockchip SoCs used to have a random number generator as part of their
crypto device.

However newer Rockchip SoCs like the RK3568 have an independent True
Random Number Generator device. This patchset adds a driver for it and
enables it in the device tree.

v5 -> v6:
 * Patch 1: unchanged

 * Patch 2: get rid of #ifdef
   - use if (IS_ENABLED(...)) { ... }instead of #ifdef inside functions
   - use __maybe_unused for functions previously enclosed by #ifdef'ery

 * Patch 3: unchanged

v4 -> v5:
 * Patch 1: always use RK3568 name
   - use full RK3568 name in patch description
   - add RK3568 to title in binding

 * Patch 2: full name and cosmetics
   - also always mention RK3568 as there may be other RNG in other
     (future) Rockchip SoCs
   - remove debug output on successful probe
   - use MODULE_AUTHOR several times instead of single comma-separated

 * Patch 3: unchanged

v3 -> v4:
 * Patch 1: minor corrections
   - fix Rokchip -> Rockchip typo
   - change commit title as requested

 * Patch 2: improved error handling and resource management
   - Always use writel() instead of writel_relaxed()
   - Use pm_runtime_resume_and_get
   - Correctly return error code in rk_rng_read()
   - Make use of devm_reset_control_array_get_exclusive
   - Use devm_pm_runtime_enable and there by get rid of rk_rng_remove()

 * Patch 3:
   - Move node to conform with ordering by address

v2 -> v3: patch adopted by Daniel Golle
 * Patch 1: address comments of Krzysztof Kozlowski, add MAINTAINERS
   - improved description
   - meaningful clock-names
   - add entry in MAINTAINERS files

 * Patch 2: numerous code-style improvements
   - drop misleading rk_rng_write_ctl(), simplify I/O writes
   - drop unused TRNG_RNG_DOUT_[1-7] macros
   - handle error handling for pm_runtime_get_sync()
   - use memcpy_fromio() instead of open coding for-loop
   - some minor white-spaces fixes

 * Patch 3:
   - use clock-names as defined in dt-bindings

v1 -> v2:
 * Patch 1: fix issues reported by Rob Herring and Krzysztof Kozlowski:
   - Rename rockchip-rng.yaml into rockchip,rk3568-rng.yaml
   - Fix binding title and description
   - Fix compatible property
   - Rename clocks and add the corresponding descriptions
   - Drop reset-names
   - Add a bus definition with #address-cells and #size-cells to the
     example.

 * Patch 2: fix issue reported by kernel test robot <lkp@intel.com>
   - Do not read the random registers as big endian, looking at the
     RK3568 TRM this is actually not needed. This fixes a sparse
     warning.

 * Patch 3: unchanged

Aurelien Jarno (3):
  dt-bindings: rng: Add Rockchip RK3568 TRNG
  hwrng: add hwrng driver for Rockchip RK3568 SoC
  arm64: dts: rockchip: add DT entry for RNG to RK356x

 .../bindings/rng/rockchip,rk3568-rng.yaml     |  61 +++++
 MAINTAINERS                                   |   7 +
 arch/arm64/boot/dts/rockchip/rk356x.dtsi      |   9 +
 drivers/char/hw_random/Kconfig                |  14 ++
 drivers/char/hw_random/Makefile               |   1 +
 drivers/char/hw_random/rockchip-rng.c         | 220 ++++++++++++++++++
 6 files changed, 312 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rng/rockchip,rk3568-rng.yaml
 create mode 100644 drivers/char/hw_random/rockchip-rng.c

Comments

Chen-Yu Tsai July 13, 2024, 6:48 a.m. UTC | #1 
Hi,

On Sat, Jul 13, 2024 at 8:38 AM Daniel Golle <daniel@makrotopia.org> wrote:
>
> Rockchip SoCs used to have a random number generator as part of their
> crypto device.
>
> However newer Rockchip SoCs like the RK3568 have an independent True
> Random Number Generator device. This patchset adds a driver for it and
> enables it in the device tree.
>

Have you tested any of the later iterations? For me it stopped working
at v3. After v3 (including v3), all it spits out is zeros.

> v5 -> v6:
>  * Patch 1: unchanged
>
>  * Patch 2: get rid of #ifdef
>    - use if (IS_ENABLED(...)) { ... }instead of #ifdef inside functions
>    - use __maybe_unused for functions previously enclosed by #ifdef'ery
>
>  * Patch 3: unchanged
>
> v4 -> v5:
>  * Patch 1: always use RK3568 name
>    - use full RK3568 name in patch description
>    - add RK3568 to title in binding
>
>  * Patch 2: full name and cosmetics
>    - also always mention RK3568 as there may be other RNG in other
>      (future) Rockchip SoCs
>    - remove debug output on successful probe
>    - use MODULE_AUTHOR several times instead of single comma-separated
>
>  * Patch 3: unchanged
>
> v3 -> v4:
>  * Patch 1: minor corrections
>    - fix Rokchip -> Rockchip typo
>    - change commit title as requested
>
>  * Patch 2: improved error handling and resource management
>    - Always use writel() instead of writel_relaxed()
>    - Use pm_runtime_resume_and_get
>    - Correctly return error code in rk_rng_read()
>    - Make use of devm_reset_control_array_get_exclusive
>    - Use devm_pm_runtime_enable and there by get rid of rk_rng_remove()
>
>  * Patch 3:
>    - Move node to conform with ordering by address
>
> v2 -> v3: patch adopted by Daniel Golle
>  * Patch 1: address comments of Krzysztof Kozlowski, add MAINTAINERS
>    - improved description
>    - meaningful clock-names
>    - add entry in MAINTAINERS files
>
>  * Patch 2: numerous code-style improvements
>    - drop misleading rk_rng_write_ctl(), simplify I/O writes

This is probably the culprit. The RNG and RST control registers have
enable bits in their top 16 bits. Without those set together with the
actual bit values, the writes to the registers have no effect.

Please check all your writel calls against the TRM and add appropriate
bitmasks for the upper 16 bits.


ChenYu

>    - drop unused TRNG_RNG_DOUT_[1-7] macros
>    - handle error handling for pm_runtime_get_sync()
>    - use memcpy_fromio() instead of open coding for-loop
>    - some minor white-spaces fixes
>
>  * Patch 3:
>    - use clock-names as defined in dt-bindings
>
> v1 -> v2:
>  * Patch 1: fix issues reported by Rob Herring and Krzysztof Kozlowski:
>    - Rename rockchip-rng.yaml into rockchip,rk3568-rng.yaml
>    - Fix binding title and description
>    - Fix compatible property
>    - Rename clocks and add the corresponding descriptions
>    - Drop reset-names
>    - Add a bus definition with #address-cells and #size-cells to the
>      example.
>
>  * Patch 2: fix issue reported by kernel test robot <lkp@intel.com>
>    - Do not read the random registers as big endian, looking at the
>      RK3568 TRM this is actually not needed. This fixes a sparse
>      warning.
>
>  * Patch 3: unchanged
>
> Aurelien Jarno (3):
>   dt-bindings: rng: Add Rockchip RK3568 TRNG
>   hwrng: add hwrng driver for Rockchip RK3568 SoC
>   arm64: dts: rockchip: add DT entry for RNG to RK356x
>
>  .../bindings/rng/rockchip,rk3568-rng.yaml     |  61 +++++
>  MAINTAINERS                                   |   7 +
>  arch/arm64/boot/dts/rockchip/rk356x.dtsi      |   9 +
>  drivers/char/hw_random/Kconfig                |  14 ++
>  drivers/char/hw_random/Makefile               |   1 +
>  drivers/char/hw_random/rockchip-rng.c         | 220 ++++++++++++++++++
>  6 files changed, 312 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rng/rockchip,rk3568-rng.yaml
>  create mode 100644 drivers/char/hw_random/rockchip-rng.c
>
> --
> 2.45.2
>
Daniel Golle July 13, 2024, 7:51 a.m. UTC | #2 
Hi Chen-Yu,

thank you for reviewing and testing.

On Sat, Jul 13, 2024 at 02:48:39PM +0800, Chen-Yu Tsai wrote:
> Hi,
> 
> On Sat, Jul 13, 2024 at 8:38 AM Daniel Golle <daniel@makrotopia.org> wrote:
> >
> > Rockchip SoCs used to have a random number generator as part of their
> > crypto device.
> >
> > However newer Rockchip SoCs like the RK3568 have an independent True
> > Random Number Generator device. This patchset adds a driver for it and
> > enables it in the device tree.
> >
> 
> Have you tested any of the later iterations? For me it stopped working
> at v3. After v3 (including v3), all it spits out is zeros.

I've not examined the actual values it returns, I will do so in future
iterations.

Alsom, I misread the original rk_rng_write_ctl() function, I will bring
it back and also improve the comment describing it.

> 
> > v5 -> v6:
> >  * Patch 1: unchanged
> >
> >  * Patch 2: get rid of #ifdef
> >    - use if (IS_ENABLED(...)) { ... }instead of #ifdef inside functions
> >    - use __maybe_unused for functions previously enclosed by #ifdef'ery
> >
> >  * Patch 3: unchanged
> >
> > v4 -> v5:
> >  * Patch 1: always use RK3568 name
> >    - use full RK3568 name in patch description
> >    - add RK3568 to title in binding
> >
> >  * Patch 2: full name and cosmetics
> >    - also always mention RK3568 as there may be other RNG in other
> >      (future) Rockchip SoCs
> >    - remove debug output on successful probe
> >    - use MODULE_AUTHOR several times instead of single comma-separated
> >
> >  * Patch 3: unchanged
> >
> > v3 -> v4:
> >  * Patch 1: minor corrections
> >    - fix Rokchip -> Rockchip typo
> >    - change commit title as requested
> >
> >  * Patch 2: improved error handling and resource management
> >    - Always use writel() instead of writel_relaxed()
> >    - Use pm_runtime_resume_and_get
> >    - Correctly return error code in rk_rng_read()
> >    - Make use of devm_reset_control_array_get_exclusive
> >    - Use devm_pm_runtime_enable and there by get rid of rk_rng_remove()
> >
> >  * Patch 3:
> >    - Move node to conform with ordering by address
> >
> > v2 -> v3: patch adopted by Daniel Golle
> >  * Patch 1: address comments of Krzysztof Kozlowski, add MAINTAINERS
> >    - improved description
> >    - meaningful clock-names
> >    - add entry in MAINTAINERS files
> >
> >  * Patch 2: numerous code-style improvements
> >    - drop misleading rk_rng_write_ctl(), simplify I/O writes
> 
> This is probably the culprit. The RNG and RST control registers have
> enable bits in their top 16 bits. Without those set together with the
> actual bit values, the writes to the registers have no effect.
> 
> Please check all your writel calls against the TRM and add appropriate
> bitmasks for the upper 16 bits.

The upper 16 bits are apparently used as hardware mask when writing the
lower 16 bits...

I will send v7 after testing.


> 
> 
> ChenYu
> 
> >    - drop unused TRNG_RNG_DOUT_[1-7] macros
> >    - handle error handling for pm_runtime_get_sync()
> >    - use memcpy_fromio() instead of open coding for-loop
> >    - some minor white-spaces fixes
> >
> >  * Patch 3:
> >    - use clock-names as defined in dt-bindings
> >
> > v1 -> v2:
> >  * Patch 1: fix issues reported by Rob Herring and Krzysztof Kozlowski:
> >    - Rename rockchip-rng.yaml into rockchip,rk3568-rng.yaml
> >    - Fix binding title and description
> >    - Fix compatible property
> >    - Rename clocks and add the corresponding descriptions
> >    - Drop reset-names
> >    - Add a bus definition with #address-cells and #size-cells to the
> >      example.
> >
> >  * Patch 2: fix issue reported by kernel test robot <lkp@intel.com>
> >    - Do not read the random registers as big endian, looking at the
> >      RK3568 TRM this is actually not needed. This fixes a sparse
> >      warning.
> >
> >  * Patch 3: unchanged
> >
> > Aurelien Jarno (3):
> >   dt-bindings: rng: Add Rockchip RK3568 TRNG
> >   hwrng: add hwrng driver for Rockchip RK3568 SoC
> >   arm64: dts: rockchip: add DT entry for RNG to RK356x
> >
> >  .../bindings/rng/rockchip,rk3568-rng.yaml     |  61 +++++
> >  MAINTAINERS                                   |   7 +
> >  arch/arm64/boot/dts/rockchip/rk356x.dtsi      |   9 +
> >  drivers/char/hw_random/Kconfig                |  14 ++
> >  drivers/char/hw_random/Makefile               |   1 +
> >  drivers/char/hw_random/rockchip-rng.c         | 220 ++++++++++++++++++
> >  6 files changed, 312 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/rng/rockchip,rk3568-rng.yaml
> >  create mode 100644 drivers/char/hw_random/rockchip-rng.c
> >
> > --
> > 2.45.2
> >