| Message ID | 20220401201804.2867154-29-clabbe@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: rockchip: permit to pass self-tests | expand |
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 <clabbe@baylibre.com> > --- > .../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 > + 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? > + > +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. Best regards, Krzysztof
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 <clabbe@baylibre.com> >> --- >> .../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 <clabbe@baylibre.com> > > --- > > .../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 <clabbe@baylibre.com> > >> --- > >> .../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
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
The latest addition to the rockchip crypto driver need to update the driver bindings. Signed-off-by: Corentin Labbe <clabbe@baylibre.com> --- .../crypto/rockchip,rk3288-crypto.yaml | 68 +++++++++++++++++-- 1 file changed, 63 insertions(+), 5 deletions(-)