The latest addition to the rockchip crypto driver need to update the
driver bindings.
Signed-off-by: Corentin Labbe <[email protected]>
---
.../crypto/rockchip,rk3288-crypto.yaml | 68 +++++++++++++++++--
1 file changed, 63 insertions(+), 5 deletions(-)
diff --git a/Documentation/devicetree/bindings/crypto/rockchip,rk3288-crypto.yaml b/Documentation/devicetree/bindings/crypto/rockchip,rk3288-crypto.yaml
index 66db671118c3..e6c00bc8bebf 100644
--- a/Documentation/devicetree/bindings/crypto/rockchip,rk3288-crypto.yaml
+++ b/Documentation/devicetree/bindings/crypto/rockchip,rk3288-crypto.yaml
@@ -11,8 +11,18 @@ maintainers:
properties:
compatible:
- enum:
- - rockchip,rk3288-crypto
+ oneOf:
+ - description: crypto IP present on RK3288 SoCs
+ items:
+ - const: rockchip,rk3288-crypto
+ - description: crypto IP present on RK3328 SoCs
+ items:
+ - const: rockchip,rk3328-crypto
+ - description: crypto IPs present on RK3399. crypto0 is the first IP with
+ RSA support, crypto1 is the second IP without RSA.
+ enum:
+ - rockchip,rk3399-crypto0
+ - rockchip,rk3399-crypto1
reg:
maxItems: 1
@@ -21,16 +31,65 @@ properties:
maxItems: 1
clocks:
+ minItems: 3
maxItems: 4
clock-names:
+ minItems: 3
maxItems: 4
resets:
- maxItems: 1
+ minItems: 1
+ maxItems: 3
reset-names:
- maxItems: 1
+ deprecated: true
+
+allOf:
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: rockchip,rk3288-crypto
+ then:
+ properties:
+ clock-names:
+ items:
+ - const: "aclk"
+ - const: "hclk"
+ - const: "sclk"
+ - const: "apb_pclk"
+ minItems: 4
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: rockchip,rk3328-crypto
+ then:
+ properties:
+ clock-names:
+ items:
+ - const: "hclk_master"
+ - const: "hclk_slave"
+ - const: "sclk"
+ maxItems: 3
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - rockchip,rk3399-crypto0
+ - rockchip,rk3399-crypto1
+ then:
+ properties:
+ clock-names:
+ items:
+ - const: "hclk_master"
+ - const: "hclk_slave"
+ - const: "sclk"
+ maxItems: 3
+ resets:
+ minItems: 3
required:
- compatible
@@ -39,7 +98,6 @@ required:
- clocks
- clock-names
- resets
- - reset-names
additionalProperties: false
--
2.35.1
On 02/04/2022 13:53, Krzysztof Kozlowski wrote:
> On 01/04/2022 22:17, Corentin Labbe wrote:
>> The latest addition to the rockchip crypto driver need to update the
>> driver bindings.
>>
>> Signed-off-by: Corentin Labbe <[email protected]>
>> ---
>> .../crypto/rockchip,rk3288-crypto.yaml | 68 +++++++++++++++++--
>> 1 file changed, 63 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/crypto/rockchip,rk3288-crypto.yaml b/Documentation/devicetree/bindings/crypto/rockchip,rk3288-crypto.yaml
>> index 66db671118c3..e6c00bc8bebf 100644
>> --- a/Documentation/devicetree/bindings/crypto/rockchip,rk3288-crypto.yaml
>> +++ b/Documentation/devicetree/bindings/crypto/rockchip,rk3288-crypto.yaml
>> @@ -11,8 +11,18 @@ maintainers:
>>
>> properties:
>> compatible:
>> - enum:
>> - - rockchip,rk3288-crypto
>> + oneOf:
>> + - description: crypto IP present on RK3288 SoCs
>> + items:
>> + - const: rockchip,rk3288-crypto
>> + - description: crypto IP present on RK3328 SoCs
>
> These two comments are not helping, so this should be just enum.
>
>> + items:
>> + - const: rockchip,rk3328-crypto
>> + - description: crypto IPs present on RK3399. crypto0 is the first IP with
>> + RSA support, crypto1 is the second IP without RSA.
>
> The second part of this comment is helpful, first not. You have chosen
> enum in your first patch, so just extend it with comments. Additionally
> indexing does not scale. What if next generation reverses it and crypto0
> does not have RSA and crypto1 has?
Actually let me re-think this. Is programming model (registers?) same
between crypto0 and crypto1? If yes, this should be same compatible and
add a dedicated property "rockchip,rsa"?
I looked at your driver and you modeled it as main and sub devices. I
wonder why - are there some dependencies? It would be helpful to have
such information here in commit msg as well. Your commit #26 says that
only difference is the RSA.
Best regards,
Krzysztof
Le Sat, Apr 02, 2022 at 01:53:58PM +0200, Krzysztof Kozlowski a ?crit :
> On 01/04/2022 22:17, Corentin Labbe wrote:
> > The latest addition to the rockchip crypto driver need to update the
> > driver bindings.
> >
> > Signed-off-by: Corentin Labbe <[email protected]>
> > ---
> > .../crypto/rockchip,rk3288-crypto.yaml | 68 +++++++++++++++++--
> > 1 file changed, 63 insertions(+), 5 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/crypto/rockchip,rk3288-crypto.yaml b/Documentation/devicetree/bindings/crypto/rockchip,rk3288-crypto.yaml
> > index 66db671118c3..e6c00bc8bebf 100644
> > --- a/Documentation/devicetree/bindings/crypto/rockchip,rk3288-crypto.yaml
> > +++ b/Documentation/devicetree/bindings/crypto/rockchip,rk3288-crypto.yaml
> > @@ -11,8 +11,18 @@ maintainers:
> >
> > properties:
> > compatible:
> > - enum:
> > - - rockchip,rk3288-crypto
> > + oneOf:
> > + - description: crypto IP present on RK3288 SoCs
> > + items:
> > + - const: rockchip,rk3288-crypto
> > + - description: crypto IP present on RK3328 SoCs
>
> These two comments are not helping, so this should be just enum.
>
> > + items:
> > + - const: rockchip,rk3328-crypto
> > + - description: crypto IPs present on RK3399. crypto0 is the first IP with
> > + RSA support, crypto1 is the second IP without RSA.
>
> The second part of this comment is helpful, first not. You have chosen
> enum in your first patch, so just extend it with comments. Additionally
> indexing does not scale. What if next generation reverses it and crypto0
> does not have RSA and crypto1 has?
>
> Something like:
>
> properties:
> compatible:
> enum:
> - rockchip,rk3288-crypto
> - rockchip,rk3328-crypto
> # With RSA
> - rockchip,rk3399-crypto-rsa
> # Without RSA
> - rockchip,rk3399-crypto-norsa
>
Hello
There will never be new SoCs with this crypto, rockchip seems to have dropped this IP for a different crypto v2 on their new SoCs.
I will answer more on that on your second mail.
> > + enum:
> > + - rockchip,rk3399-crypto0
> > + - rockchip,rk3399-crypto1
> >
> > reg:
> > maxItems: 1
> > @@ -21,16 +31,65 @@ properties:
> > maxItems: 1
> >
> > clocks:
> > + minItems: 3
> > maxItems: 4
> >
> > clock-names:
> > + minItems: 3
> > maxItems: 4
> >
> > resets:
> > - maxItems: 1
> > + minItems: 1
> > + maxItems: 3
> >
> > reset-names:
> > - maxItems: 1
> > + deprecated: true
>
> Why reset-names are being deprecated? Did we talk about this?
>
Since I use the devm_reset_control_array_get_exclusive, there is no need to have reset-names.
> > +
> > +allOf:
> > + - if:
> > + properties:
> > + compatible:
> > + contains:
> > + const: rockchip,rk3288-crypto
> > + then:
> > + properties:
> > + clock-names:
> > + items:
> > + - const: "aclk"
> > + - const: "hclk"
> > + - const: "sclk"
> > + - const: "apb_pclk"
> > + minItems: 4
>
> minItems for clocks
> max for resets and reset-names
>
> > + - if:
> > + properties:
> > + compatible:
> > + contains:
> > + const: rockchip,rk3328-crypto
> > + then:
> > + properties:
> > + clock-names:
> > + items:
> > + - const: "hclk_master"
> > + - const: "hclk_slave"
> > + - const: "sclk"
> > + maxItems: 3
>
> min/max for clocks
> max for resets and reset-names
>
> > + - if:
> > + properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - rockchip,rk3399-crypto0
> > + - rockchip,rk3399-crypto1
> > + then:
> > + properties:
> > + clock-names:
> > + items:
> > + - const: "hclk_master"
> > + - const: "hclk_slave"
> > + - const: "sclk"
> > + maxItems: 3
> > + resets:
> > + minItems: 3
>
> Similarly.
>
I will fix that in v5
Thanks.
On 02/04/2022 22:10, LABBE Corentin wrote:
> Le Sat, Apr 02, 2022 at 01:53:58PM +0200, Krzysztof Kozlowski a écrit :
>> On 01/04/2022 22:17, Corentin Labbe wrote:
>>> The latest addition to the rockchip crypto driver need to update the
>>> driver bindings.
>>>
>>>
>>> reset-names:
>>> - maxItems: 1
>>> + deprecated: true
>>
>> Why reset-names are being deprecated? Did we talk about this?
>>
>
> Since I use the devm_reset_control_array_get_exclusive, there is no need to have reset-names.
The reset-names are not only for Linux driver. In any case, Linux driver
could get always reset/clock/gpio by index, not by name.
Additionally, there can be different implementation in different
system/user of bindings.
Therefore the driver implementation does not matter (or matters little)
for the bindings, so for multi entries the reset-names are needed.
Best regards,
Krzysztof
Le Sat, Apr 02, 2022 at 02:07:26PM +0200, Krzysztof Kozlowski a ?crit :
> On 02/04/2022 13:53, Krzysztof Kozlowski wrote:
> > On 01/04/2022 22:17, Corentin Labbe wrote:
> >> The latest addition to the rockchip crypto driver need to update the
> >> driver bindings.
> >>
> >> Signed-off-by: Corentin Labbe <[email protected]>
> >> ---
> >> .../crypto/rockchip,rk3288-crypto.yaml | 68 +++++++++++++++++--
> >> 1 file changed, 63 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/crypto/rockchip,rk3288-crypto.yaml b/Documentation/devicetree/bindings/crypto/rockchip,rk3288-crypto.yaml
> >> index 66db671118c3..e6c00bc8bebf 100644
> >> --- a/Documentation/devicetree/bindings/crypto/rockchip,rk3288-crypto.yaml
> >> +++ b/Documentation/devicetree/bindings/crypto/rockchip,rk3288-crypto.yaml
> >> @@ -11,8 +11,18 @@ maintainers:
> >>
> >> properties:
> >> compatible:
> >> - enum:
> >> - - rockchip,rk3288-crypto
> >> + oneOf:
> >> + - description: crypto IP present on RK3288 SoCs
> >> + items:
> >> + - const: rockchip,rk3288-crypto
> >> + - description: crypto IP present on RK3328 SoCs
> >
> > These two comments are not helping, so this should be just enum.
> >
> >> + items:
> >> + - const: rockchip,rk3328-crypto
> >> + - description: crypto IPs present on RK3399. crypto0 is the first IP with
> >> + RSA support, crypto1 is the second IP without RSA.
> >
> > The second part of this comment is helpful, first not. You have chosen
> > enum in your first patch, so just extend it with comments. Additionally
> > indexing does not scale. What if next generation reverses it and crypto0
> > does not have RSA and crypto1 has?
>
> Actually let me re-think this. Is programming model (registers?) same
> between crypto0 and crypto1? If yes, this should be same compatible and
> add a dedicated property "rockchip,rsa"?
>
> I looked at your driver and you modeled it as main and sub devices. I
> wonder why - are there some dependencies? It would be helpful to have
> such information here in commit msg as well. Your commit #26 says that
> only difference is the RSA.
>
Hello
There is no dependency, my only problem is that only one of 2 instance need to register crypto algos.
The only perfect way is to have a list_head of devices, but I found this a bit complex/overkill.
I understand my current way is not ideal, I will probably try this other way. In that case, yes problably the 2 node need to have the same compatible (and only a future rockchip,rsa will permit to distinct where RSA is).
Regards