[PATCH v4 0/3] add AES support for Exynos5433

[PATCH v4 0/3] add AES support for Exynos5433

[Kamil Konieczny]

Add slimSSS node to DT and crypto AES support for Exynos5433. Tested on
Exynos5433 board with crypto run-time self tests and with tcrypt with
command insmod tcrypt.ko mode=500 sec=1

Changes since v3:
- moved bindings documentation out from samsung-sss.txt into separate file
  samsung-slimsss.txt

Changes since v2:
- address Corentine Labbe note: add "const" to char* in declaration of
  struct samsung_aes_variant

Changes since v1:
- address Krzysztof Kozlowski review: add missing comma in struct
  definition, add goto for error code path, correct indentation,
  in documentation add that Exynos5433 has both slimSSS and SSS IPs.

Kamil Konieczny (3):
  arm64: dts: exynos: add SlimSSS for Exynos5433
  dt-bindings: crypto: document Exynos5433 SlimSSS
  crypto: s5p: add AES support for Exynos5433

 .../bindings/crypto/samsung-slimsss.txt       | 19 +++++++
 arch/arm64/boot/dts/exynos/exynos5433.dtsi    |  9 ++++
 drivers/crypto/s5p-sss.c                      | 50 +++++++++++++++++--
 3 files changed, 74 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/crypto/samsung-slimsss.txt

-- 
2.20.1

[PATCH v4 1/3] arm64: dts: exynos: add SlimSSS for Exynos5433

[Kamil Konieczny]

Add DT node for SlimSSS (aka Slim SecuritySubSystem) in Exynos5433 SoC.
The users can use compatibility "samsung,exynos5433-slim-sss".

Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
---
 arch/arm64/boot/dts/exynos/exynos5433.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/exynos/exynos5433.dtsi b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
index e7cd3b67d818..9e5fcb6c7776 100644
--- a/arch/arm64/boot/dts/exynos/exynos5433.dtsi
+++ b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
@@ -559,6 +559,15 @@
 				<&cmu_top CLK_DIV_ACLK_IMEM_200>;
 		};
 
+		slim_sss: slim-sss@11140000 {
+			compatible = "samsung,exynos5433-slim-sss";
+			reg = <0x11140000 0x1000>;
+			interrupts = <GIC_SPI 312 IRQ_TYPE_LEVEL_HIGH>;
+			clock-names = "aclk", "pclk";
+			clocks = <&cmu_imem CLK_ACLK_SLIMSSS>,
+				 <&cmu_imem CLK_PCLK_SLIMSSS>;
+		};
+
 		pd_gscl: power-domain@105c4000 {
 			compatible = "samsung,exynos5433-pd";
 			reg = <0x105c4000 0x20>;
-- 
2.20.1

Re: [PATCH v4 1/3] arm64: dts: exynos: add SlimSSS for Exynos5433

[Krzysztof Kozlowski]

On Fri, Feb 22, 2019 at 01:21:42PM +0100, Kamil Konieczny wrote:
> Add DT node for SlimSSS (aka Slim SecuritySubSystem) in Exynos5433 SoC.
> The users can use compatibility "samsung,exynos5433-slim-sss".
> 
> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
> ---
>  arch/arm64/boot/dts/exynos/exynos5433.dtsi | 9 +++++++++

Thanks, applied.

Best regards,
Krzysztof

[PATCH v4 2/3] dt-bindings: crypto: document Exynos5433 SlimSSS

[Kamil Konieczny]

Document DT bindings for crypto Samsung Exynos5433 SlimSSS (Slim Security
SubSystem) IP.

Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
---
 .../bindings/crypto/samsung-slimsss.txt       | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/crypto/samsung-slimsss.txt

diff --git a/Documentation/devicetree/bindings/crypto/samsung-slimsss.txt b/Documentation/devicetree/bindings/crypto/samsung-slimsss.txt
new file mode 100644
index 000000000000..7ec9a5a7727a
--- /dev/null
+++ b/Documentation/devicetree/bindings/crypto/samsung-slimsss.txt
@@ -0,0 +1,19 @@
+Samsung SoC SlimSSS (Slim Security SubSystem) module
+
+The SlimSSS module in Exynos5433 SoC supports the following:
+-- Feeder (FeedCtrl)
+-- Advanced Encryption Standard (AES) with ECB,CBC,CTR,XTS and (CBC/XTS)/CTS
+-- SHA-1/SHA-256 and (SHA-1/SHA-256)/HMAC
+
+Required properties:
+
+- compatible : Should contain entry for slimSSS version:
+  - "samsung,exynos5433-slim-sss" for Exynos5433 SoC.
+- reg : Offset and length of the register set for the module
+- interrupts : interrupt specifiers of SlimSSS module interrupts (one feed
+		control interrupt).
+
+- clocks : list of clock phandle and specifier pairs for all clocks listed in
+		clock-names property.
+- clock-names : list of device clock input names; should contain "pclk" and
+		"aclk" for slim-sss in Exynos5433.
-- 
2.20.1

[PATCH v4 3/3] crypto: s5p: add AES support for Exynos5433

[Kamil Konieczny]

Add AES crypto HW acceleration for Exynos5433, with the help of SlimSSS IP.

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
---
 drivers/crypto/s5p-sss.c | 50 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 46 insertions(+), 4 deletions(-)

diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
index 0064be0e3941..3f45cc5cb94a 100644
--- a/drivers/crypto/s5p-sss.c
+++ b/drivers/crypto/s5p-sss.c
@@ -232,6 +232,7 @@
  * struct samsung_aes_variant - platform specific SSS driver data
  * @aes_offset: AES register offset from SSS module's base.
  * @hash_offset: HASH register offset from SSS module's base.
+ * @clk_names: names of clocks needed to run SSS IP
  *
  * Specifies platform specific configuration of SSS module.
  * Note: A structure for driver specific platform data is used for future
@@ -240,6 +241,7 @@
 struct samsung_aes_variant {
 	unsigned int			aes_offset;
 	unsigned int			hash_offset;
+	const char			*clk_names[];
 };
 
 struct s5p_aes_reqctx {
@@ -296,6 +298,7 @@ struct s5p_aes_ctx {
 struct s5p_aes_dev {
 	struct device			*dev;
 	struct clk			*clk;
+	struct clk			*pclk;
 	void __iomem			*ioaddr;
 	void __iomem			*aes_ioaddr;
 	int				irq_fc;
@@ -384,11 +387,19 @@ struct s5p_hash_ctx {
 static const struct samsung_aes_variant s5p_aes_data = {
 	.aes_offset	= 0x4000,
 	.hash_offset	= 0x6000,
+	.clk_names	= { "secss", },
 };
 
 static const struct samsung_aes_variant exynos_aes_data = {
 	.aes_offset	= 0x200,
 	.hash_offset	= 0x400,
+	.clk_names	= { "secss", },
+};
+
+static const struct samsung_aes_variant exynos5433_slim_aes_data = {
+	.aes_offset	= 0x400,
+	.hash_offset	= 0x800,
+	.clk_names	= { "pclk", "aclk", },
 };
 
 static const struct of_device_id s5p_sss_dt_match[] = {
@@ -400,6 +411,10 @@ static const struct of_device_id s5p_sss_dt_match[] = {
 		.compatible = "samsung,exynos4210-secss",
 		.data = &exynos_aes_data,
 	},
+	{
+		.compatible = "samsung,exynos5433-slim-sss",
+		.data = &exynos5433_slim_aes_data,
+	},
 	{ },
 };
 MODULE_DEVICE_TABLE(of, s5p_sss_dt_match);
@@ -2208,18 +2223,39 @@ static int s5p_aes_probe(struct platform_device *pdev)
 			return PTR_ERR(pdata->ioaddr);
 	}
 
-	pdata->clk = devm_clk_get(dev, "secss");
+	pdata->clk = devm_clk_get(dev, variant->clk_names[0]);
 	if (IS_ERR(pdata->clk)) {
-		dev_err(dev, "failed to find secss clock source\n");
+		dev_err(dev, "failed to find secss clock %s\n",
+			variant->clk_names[0]);
 		return -ENOENT;
 	}
 
 	err = clk_prepare_enable(pdata->clk);
 	if (err < 0) {
-		dev_err(dev, "Enabling SSS clk failed, err %d\n", err);
+		dev_err(dev, "Enabling clock %s failed, err %d\n",
+			variant->clk_names[0], err);
 		return err;
 	}
 
+	if (variant->clk_names[1]) {
+		pdata->pclk = devm_clk_get(dev, variant->clk_names[1]);
+		if (IS_ERR(pdata->pclk)) {
+			dev_err(dev, "failed to find clock %s\n",
+				variant->clk_names[1]);
+			err = -ENOENT;
+			goto err_clk;
+		}
+
+		err = clk_prepare_enable(pdata->pclk);
+		if (err < 0) {
+			dev_err(dev, "Enabling clock %s failed, err %d\n",
+				variant->clk_names[0], err);
+			goto err_clk;
+		}
+	} else {
+		pdata->pclk = NULL;
+	}
+
 	spin_lock_init(&pdata->lock);
 	spin_lock_init(&pdata->hash_lock);
 
@@ -2295,8 +2331,11 @@ static int s5p_aes_probe(struct platform_device *pdev)
 	tasklet_kill(&pdata->tasklet);
 
 err_irq:
-	clk_disable_unprepare(pdata->clk);
+	if (pdata->pclk)
+		clk_disable_unprepare(pdata->pclk);
 
+err_clk:
+	clk_disable_unprepare(pdata->clk);
 	s5p_dev = NULL;
 
 	return err;
@@ -2323,6 +2362,9 @@ static int s5p_aes_remove(struct platform_device *pdev)
 		pdata->use_hash = false;
 	}
 
+	if (pdata->pclk)
+		clk_disable_unprepare(pdata->pclk);
+
 	clk_disable_unprepare(pdata->clk);
 	s5p_dev = NULL;
 
-- 
2.20.1

Re: [PATCH v4 3/3] crypto: s5p: add AES support for Exynos5433

[Krzysztof Kozlowski]

On Fri, 22 Feb 2019 at 13:22, Kamil Konieczny
<k.konieczny@partner.samsung.com> wrote:
>
> Add AES crypto HW acceleration for Exynos5433, with the help of SlimSSS IP.
>
> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
> ---
>  drivers/crypto/s5p-sss.c | 50 ++++++++++++++++++++++++++++++++++++----
>  1 file changed, 46 insertions(+), 4 deletions(-)
> diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
> index 0064be0e3941..3f45cc5cb94a 100644
> --- a/drivers/crypto/s5p-sss.c
> +++ b/drivers/crypto/s5p-sss.c
> @@ -232,6 +232,7 @@
>   * struct samsung_aes_variant - platform specific SSS driver data
>   * @aes_offset: AES register offset from SSS module's base.
>   * @hash_offset: HASH register offset from SSS module's base.
> + * @clk_names: names of clocks needed to run SSS IP
>   *
>   * Specifies platform specific configuration of SSS module.
>   * Note: A structure for driver specific platform data is used for future
> @@ -240,6 +241,7 @@
>  struct samsung_aes_variant {
>         unsigned int                    aes_offset;
>         unsigned int                    hash_offset;
> +       const char                      *clk_names[];
>  };
>
>  struct s5p_aes_reqctx {
> @@ -296,6 +298,7 @@ struct s5p_aes_ctx {
>  struct s5p_aes_dev {
>         struct device                   *dev;
>         struct clk                      *clk;
> +       struct clk                      *pclk;
>         void __iomem                    *ioaddr;
>         void __iomem                    *aes_ioaddr;
>         int                             irq_fc;
> @@ -384,11 +387,19 @@ struct s5p_hash_ctx {
>  static const struct samsung_aes_variant s5p_aes_data = {
>         .aes_offset     = 0x4000,
>         .hash_offset    = 0x6000,
> +       .clk_names      = { "secss", },
>  };
>
>  static const struct samsung_aes_variant exynos_aes_data = {
>         .aes_offset     = 0x200,
>         .hash_offset    = 0x400,
> +       .clk_names      = { "secss", },
> +};
> +
> +static const struct samsung_aes_variant exynos5433_slim_aes_data = {
> +       .aes_offset     = 0x400,
> +       .hash_offset    = 0x800,
> +       .clk_names      = { "pclk", "aclk", },
>  };
>
>  static const struct of_device_id s5p_sss_dt_match[] = {
> @@ -400,6 +411,10 @@ static const struct of_device_id s5p_sss_dt_match[] = {
>                 .compatible = "samsung,exynos4210-secss",
>                 .data = &exynos_aes_data,
>         },
> +       {
> +               .compatible = "samsung,exynos5433-slim-sss",
> +               .data = &exynos5433_slim_aes_data,
> +       },
>         { },
>  };
>  MODULE_DEVICE_TABLE(of, s5p_sss_dt_match);
> @@ -2208,18 +2223,39 @@ static int s5p_aes_probe(struct platform_device *pdev)
>                         return PTR_ERR(pdata->ioaddr);
>         }
>
> -       pdata->clk = devm_clk_get(dev, "secss");
> +       pdata->clk = devm_clk_get(dev, variant->clk_names[0]);
>         if (IS_ERR(pdata->clk)) {
> -               dev_err(dev, "failed to find secss clock source\n");
> +               dev_err(dev, "failed to find secss clock %s\n",
> +                       variant->clk_names[0]);
>                 return -ENOENT;
>         }
>
>         err = clk_prepare_enable(pdata->clk);
>         if (err < 0) {
> -               dev_err(dev, "Enabling SSS clk failed, err %d\n", err);
> +               dev_err(dev, "Enabling clock %s failed, err %d\n",
> +                       variant->clk_names[0], err);
>                 return err;
>         }
>
> +       if (variant->clk_names[1]) {
> +               pdata->pclk = devm_clk_get(dev, variant->clk_names[1]);

I see on today's linux next:
[ 6.391256] Unable to handle kernel paging request at virtual address 00004000
on all my Exynos boards (U3, XU, XU3, HC1).
I did not do a bisect yet but it might be connected with this commit.

The call trace is:
(strcmp) from [<c0774014>] (of_property_match_string+0x58/0xd0)
(of_property_match_string) from [<c04d3d2c>] (__of_clk_get_by_name+0x5c/0x130)
(__of_clk_get_by_name) from [<c04d3fa0>] (clk_get+0x34/0x70)
(clk_get) from [<c04d3680>] (devm_clk_get+0x38/0x74)
(devm_clk_get) from [<c075c248>] (s5p_aes_probe+0xd4/0x4a0)
(s5p_aes_probe) from [<c059dbc4>] (platform_drv_probe+0x6c/0xa4)
(platform_drv_probe) from [<c059b4c8>] (really_probe+0x280/0x414)
(really_probe) from [<c059b810>] (driver_probe_device+0x78/0x1c0)
(driver_probe_device) from [<c059bbb8>] (device_driver_attach+0x58/0x60)
(device_driver_attach) from [<c059bcbc>] (__driver_attach+0xfc/0x160)
(__driver_attach) from [<c059928c>] (bus_for_each_dev+0x68/0xb4)
(bus_for_each_dev) from [<c059a5c4>] (bus_add_driver+0x158/0x214)
(bus_add_driver) from [<c059cb94>] (driver_register+0x78/0x110)
(driver_register) from [<c01031f8>] (do_one_initcall+0x8c/0x404)

Full log:
https://krzk.eu/#/builders/21/builds/1153/steps/12/logs/serial0

The s5p_aes_probe address points here.

Best regards,
Krzysztof

>
> +               if (IS_ERR(pdata->pclk)) {
> +                       dev_err(dev, "failed to find clock %s\n",
> +                               variant->clk_names[1]);
> +                       err = -ENOENT;
> +                       goto err_clk;
> +               }
> +
> +               err = clk_prepare_enable(pdata->pclk);
> +               if (err < 0) {
> +                       dev_err(dev, "Enabling clock %s failed, err %d\n",
> +                               variant->clk_names[0], err);
> +                       goto err_clk;
> +               }
> +       } else {
> +               pdata->pclk = NULL;
> +       }
> +
>         spin_lock_init(&pdata->lock);
>         spin_lock_init(&pdata->hash_lock);
>
> @@ -2295,8 +2331,11 @@ static int s5p_aes_probe(struct platform_device *pdev)
>         tasklet_kill(&pdata->tasklet);
>
>  err_irq:
> -       clk_disable_unprepare(pdata->clk);
> +       if (pdata->pclk)
> +               clk_disable_unprepare(pdata->pclk);
>
> +err_clk:
> +       clk_disable_unprepare(pdata->clk);
>         s5p_dev = NULL;
>
>         return err;
> @@ -2323,6 +2362,9 @@ static int s5p_aes_remove(struct platform_device *pdev)
>                 pdata->use_hash = false;
>         }
>
> +       if (pdata->pclk)
> +               clk_disable_unprepare(pdata->pclk);
> +
>         clk_disable_unprepare(pdata->clk);
>         s5p_dev = NULL;
>
> --
> 2.20.1
>

Re: [PATCH v4 3/3] crypto: s5p: add AES support for Exynos5433

[Ard Biesheuvel]

On Fri, 1 Mar 2019 at 10:56, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Fri, 22 Feb 2019 at 13:22, Kamil Konieczny
> <k.konieczny@partner.samsung.com> wrote:
> >
> > Add AES crypto HW acceleration for Exynos5433, with the help of SlimSSS IP.
> >
> > Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
> > Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
> > ---
> >  drivers/crypto/s5p-sss.c | 50 ++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 46 insertions(+), 4 deletions(-)
> > diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
> > index 0064be0e3941..3f45cc5cb94a 100644
> > --- a/drivers/crypto/s5p-sss.c
> > +++ b/drivers/crypto/s5p-sss.c
> > @@ -232,6 +232,7 @@
> >   * struct samsung_aes_variant - platform specific SSS driver data
> >   * @aes_offset: AES register offset from SSS module's base.
> >   * @hash_offset: HASH register offset from SSS module's base.
> > + * @clk_names: names of clocks needed to run SSS IP
> >   *
> >   * Specifies platform specific configuration of SSS module.
> >   * Note: A structure for driver specific platform data is used for future
> > @@ -240,6 +241,7 @@
> >  struct samsung_aes_variant {
> >         unsigned int                    aes_offset;
> >         unsigned int                    hash_offset;
> > +       const char                      *clk_names[];
> >  };
> >
> >  struct s5p_aes_reqctx {
> > @@ -296,6 +298,7 @@ struct s5p_aes_ctx {
> >  struct s5p_aes_dev {
> >         struct device                   *dev;
> >         struct clk                      *clk;
> > +       struct clk                      *pclk;
> >         void __iomem                    *ioaddr;
> >         void __iomem                    *aes_ioaddr;
> >         int                             irq_fc;
> > @@ -384,11 +387,19 @@ struct s5p_hash_ctx {
> >  static const struct samsung_aes_variant s5p_aes_data = {
> >         .aes_offset     = 0x4000,
> >         .hash_offset    = 0x6000,
> > +       .clk_names      = { "secss", },
> >  };
> >
> >  static const struct samsung_aes_variant exynos_aes_data = {
> >         .aes_offset     = 0x200,
> >         .hash_offset    = 0x400,
> > +       .clk_names      = { "secss", },
> > +};
> > +
> > +static const struct samsung_aes_variant exynos5433_slim_aes_data = {
> > +       .aes_offset     = 0x400,
> > +       .hash_offset    = 0x800,
> > +       .clk_names      = { "pclk", "aclk", },
> >  };
> >
> >  static const struct of_device_id s5p_sss_dt_match[] = {
> > @@ -400,6 +411,10 @@ static const struct of_device_id s5p_sss_dt_match[] = {
> >                 .compatible = "samsung,exynos4210-secss",
> >                 .data = &exynos_aes_data,
> >         },
> > +       {
> > +               .compatible = "samsung,exynos5433-slim-sss",
> > +               .data = &exynos5433_slim_aes_data,
> > +       },
> >         { },
> >  };
> >  MODULE_DEVICE_TABLE(of, s5p_sss_dt_match);
> > @@ -2208,18 +2223,39 @@ static int s5p_aes_probe(struct platform_device *pdev)
> >                         return PTR_ERR(pdata->ioaddr);
> >         }
> >
> > -       pdata->clk = devm_clk_get(dev, "secss");
> > +       pdata->clk = devm_clk_get(dev, variant->clk_names[0]);
> >         if (IS_ERR(pdata->clk)) {
> > -               dev_err(dev, "failed to find secss clock source\n");
> > +               dev_err(dev, "failed to find secss clock %s\n",
> > +                       variant->clk_names[0]);
> >                 return -ENOENT;
> >         }
> >
> >         err = clk_prepare_enable(pdata->clk);
> >         if (err < 0) {
> > -               dev_err(dev, "Enabling SSS clk failed, err %d\n", err);
> > +               dev_err(dev, "Enabling clock %s failed, err %d\n",
> > +                       variant->clk_names[0], err);
> >                 return err;
> >         }
> >
> > +       if (variant->clk_names[1]) {
> > +               pdata->pclk = devm_clk_get(dev, variant->clk_names[1]);
>
> I see on today's linux next:
> [ 6.391256] Unable to handle kernel paging request at virtual address 00004000
> on all my Exynos boards (U3, XU, XU3, HC1).
> I did not do a bisect yet but it might be connected with this commit.
>
> The call trace is:
> (strcmp) from [<c0774014>] (of_property_match_string+0x58/0xd0)
> (of_property_match_string) from [<c04d3d2c>] (__of_clk_get_by_name+0x5c/0x130)
> (__of_clk_get_by_name) from [<c04d3fa0>] (clk_get+0x34/0x70)
> (clk_get) from [<c04d3680>] (devm_clk_get+0x38/0x74)
> (devm_clk_get) from [<c075c248>] (s5p_aes_probe+0xd4/0x4a0)
> (s5p_aes_probe) from [<c059dbc4>] (platform_drv_probe+0x6c/0xa4)
> (platform_drv_probe) from [<c059b4c8>] (really_probe+0x280/0x414)
> (really_probe) from [<c059b810>] (driver_probe_device+0x78/0x1c0)
> (driver_probe_device) from [<c059bbb8>] (device_driver_attach+0x58/0x60)
> (device_driver_attach) from [<c059bcbc>] (__driver_attach+0xfc/0x160)
> (__driver_attach) from [<c059928c>] (bus_for_each_dev+0x68/0xb4)
> (bus_for_each_dev) from [<c059a5c4>] (bus_add_driver+0x158/0x214)
> (bus_add_driver) from [<c059cb94>] (driver_register+0x78/0x110)
> (driver_register) from [<c01031f8>] (do_one_initcall+0x8c/0x404)
>
> Full log:
> https://krzk.eu/#/builders/21/builds/1153/steps/12/logs/serial0
>
> The s5p_aes_probe address points here.
>

It seems that dereferencing clk_names[1] of struct samsung_aes_variant
exynos_aes_data

> > +       .clk_names      = { "secss", },

returns the first member of

> >  static const struct samsung_aes_variant s5p_aes_data = {
> >         .aes_offset     = 0x4000,

since the array has only one entry in this case, and so the second,
non-existing entry overlaps with the next data item in the object
file.

So the fix would be to use

.clk_names      = { "secss", NULL },

instead (or define the array to have 2 members)


>
> >
> > +               if (IS_ERR(pdata->pclk)) {
> > +                       dev_err(dev, "failed to find clock %s\n",
> > +                               variant->clk_names[1]);
> > +                       err = -ENOENT;
> > +                       goto err_clk;
> > +               }
> > +
> > +               err = clk_prepare_enable(pdata->pclk);
> > +               if (err < 0) {
> > +                       dev_err(dev, "Enabling clock %s failed, err %d\n",
> > +                               variant->clk_names[0], err);
> > +                       goto err_clk;
> > +               }
> > +       } else {
> > +               pdata->pclk = NULL;
> > +       }
> > +
> >         spin_lock_init(&pdata->lock);
> >         spin_lock_init(&pdata->hash_lock);
> >
> > @@ -2295,8 +2331,11 @@ static int s5p_aes_probe(struct platform_device *pdev)
> >         tasklet_kill(&pdata->tasklet);
> >
> >  err_irq:
> > -       clk_disable_unprepare(pdata->clk);
> > +       if (pdata->pclk)
> > +               clk_disable_unprepare(pdata->pclk);
> >
> > +err_clk:
> > +       clk_disable_unprepare(pdata->clk);
> >         s5p_dev = NULL;
> >
> >         return err;
> > @@ -2323,6 +2362,9 @@ static int s5p_aes_remove(struct platform_device *pdev)
> >                 pdata->use_hash = false;
> >         }
> >
> > +       if (pdata->pclk)
> > +               clk_disable_unprepare(pdata->pclk);
> > +
> >         clk_disable_unprepare(pdata->clk);
> >         s5p_dev = NULL;
> >
> > --
> > 2.20.1
> >

Re: [PATCH v4 3/3] crypto: s5p: add AES support for Exynos5433

[Krzysztof Kozlowski]

On Fri, 1 Mar 2019 at 11:07, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Fri, 1 Mar 2019 at 10:56, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >
> > On Fri, 22 Feb 2019 at 13:22, Kamil Konieczny
> > <k.konieczny@partner.samsung.com> wrote:
> > >
> > > Add AES crypto HW acceleration for Exynos5433, with the help of SlimSSS IP.
> > >
> > > Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
> > > Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
> > > ---
> > >  drivers/crypto/s5p-sss.c | 50 ++++++++++++++++++++++++++++++++++++----
> > >  1 file changed, 46 insertions(+), 4 deletions(-)
> > > diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
> > > index 0064be0e3941..3f45cc5cb94a 100644
> > > --- a/drivers/crypto/s5p-sss.c
> > > +++ b/drivers/crypto/s5p-sss.c
> > > @@ -232,6 +232,7 @@
> > >   * struct samsung_aes_variant - platform specific SSS driver data
> > >   * @aes_offset: AES register offset from SSS module's base.
> > >   * @hash_offset: HASH register offset from SSS module's base.
> > > + * @clk_names: names of clocks needed to run SSS IP
> > >   *
> > >   * Specifies platform specific configuration of SSS module.
> > >   * Note: A structure for driver specific platform data is used for future
> > > @@ -240,6 +241,7 @@
> > >  struct samsung_aes_variant {
> > >         unsigned int                    aes_offset;
> > >         unsigned int                    hash_offset;
> > > +       const char                      *clk_names[];
> > >  };
> > >
> > >  struct s5p_aes_reqctx {
> > > @@ -296,6 +298,7 @@ struct s5p_aes_ctx {
> > >  struct s5p_aes_dev {
> > >         struct device                   *dev;
> > >         struct clk                      *clk;
> > > +       struct clk                      *pclk;
> > >         void __iomem                    *ioaddr;
> > >         void __iomem                    *aes_ioaddr;
> > >         int                             irq_fc;
> > > @@ -384,11 +387,19 @@ struct s5p_hash_ctx {
> > >  static const struct samsung_aes_variant s5p_aes_data = {
> > >         .aes_offset     = 0x4000,
> > >         .hash_offset    = 0x6000,
> > > +       .clk_names      = { "secss", },
> > >  };
> > >
> > >  static const struct samsung_aes_variant exynos_aes_data = {
> > >         .aes_offset     = 0x200,
> > >         .hash_offset    = 0x400,
> > > +       .clk_names      = { "secss", },
> > > +};
> > > +
> > > +static const struct samsung_aes_variant exynos5433_slim_aes_data = {
> > > +       .aes_offset     = 0x400,
> > > +       .hash_offset    = 0x800,
> > > +       .clk_names      = { "pclk", "aclk", },
> > >  };
> > >
> > >  static const struct of_device_id s5p_sss_dt_match[] = {
> > > @@ -400,6 +411,10 @@ static const struct of_device_id s5p_sss_dt_match[] = {
> > >                 .compatible = "samsung,exynos4210-secss",
> > >                 .data = &exynos_aes_data,
> > >         },
> > > +       {
> > > +               .compatible = "samsung,exynos5433-slim-sss",
> > > +               .data = &exynos5433_slim_aes_data,
> > > +       },
> > >         { },
> > >  };
> > >  MODULE_DEVICE_TABLE(of, s5p_sss_dt_match);
> > > @@ -2208,18 +2223,39 @@ static int s5p_aes_probe(struct platform_device *pdev)
> > >                         return PTR_ERR(pdata->ioaddr);
> > >         }
> > >
> > > -       pdata->clk = devm_clk_get(dev, "secss");
> > > +       pdata->clk = devm_clk_get(dev, variant->clk_names[0]);
> > >         if (IS_ERR(pdata->clk)) {
> > > -               dev_err(dev, "failed to find secss clock source\n");
> > > +               dev_err(dev, "failed to find secss clock %s\n",
> > > +                       variant->clk_names[0]);
> > >                 return -ENOENT;
> > >         }
> > >
> > >         err = clk_prepare_enable(pdata->clk);
> > >         if (err < 0) {
> > > -               dev_err(dev, "Enabling SSS clk failed, err %d\n", err);
> > > +               dev_err(dev, "Enabling clock %s failed, err %d\n",
> > > +                       variant->clk_names[0], err);
> > >                 return err;
> > >         }
> > >
> > > +       if (variant->clk_names[1]) {
> > > +               pdata->pclk = devm_clk_get(dev, variant->clk_names[1]);
> >
> > I see on today's linux next:
> > [ 6.391256] Unable to handle kernel paging request at virtual address 00004000
> > on all my Exynos boards (U3, XU, XU3, HC1).
> > I did not do a bisect yet but it might be connected with this commit.
> >
> > The call trace is:
> > (strcmp) from [<c0774014>] (of_property_match_string+0x58/0xd0)
> > (of_property_match_string) from [<c04d3d2c>] (__of_clk_get_by_name+0x5c/0x130)
> > (__of_clk_get_by_name) from [<c04d3fa0>] (clk_get+0x34/0x70)
> > (clk_get) from [<c04d3680>] (devm_clk_get+0x38/0x74)
> > (devm_clk_get) from [<c075c248>] (s5p_aes_probe+0xd4/0x4a0)
> > (s5p_aes_probe) from [<c059dbc4>] (platform_drv_probe+0x6c/0xa4)
> > (platform_drv_probe) from [<c059b4c8>] (really_probe+0x280/0x414)
> > (really_probe) from [<c059b810>] (driver_probe_device+0x78/0x1c0)
> > (driver_probe_device) from [<c059bbb8>] (device_driver_attach+0x58/0x60)
> > (device_driver_attach) from [<c059bcbc>] (__driver_attach+0xfc/0x160)
> > (__driver_attach) from [<c059928c>] (bus_for_each_dev+0x68/0xb4)
> > (bus_for_each_dev) from [<c059a5c4>] (bus_add_driver+0x158/0x214)
> > (bus_add_driver) from [<c059cb94>] (driver_register+0x78/0x110)
> > (driver_register) from [<c01031f8>] (do_one_initcall+0x8c/0x404)
> >
> > Full log:
> > https://krzk.eu/#/builders/21/builds/1153/steps/12/logs/serial0
> >
> > The s5p_aes_probe address points here.
> >
>
> It seems that dereferencing clk_names[1] of struct samsung_aes_variant
> exynos_aes_data
>
> > > +       .clk_names      = { "secss", },
>
> returns the first member of
>
> > >  static const struct samsung_aes_variant s5p_aes_data = {
> > >         .aes_offset     = 0x4000,
>
> since the array has only one entry in this case, and so the second,
> non-existing entry overlaps with the next data item in the object
> file.
>
> So the fix would be to use
>
> .clk_names      = { "secss", NULL },
>
> instead (or define the array to have 2 members)

Ah yes, the size of the array is not fixed during definition. Probably
having explicit size would be less error-prone in the future (if
someone adds third clock).

Best regards,
Krzysztof

Re: [PATCH v4 3/3] crypto: s5p: add AES support for Exynos5433

[Guenter Roeck]

On Fri, Feb 22, 2019 at 01:21:44PM +0100, Kamil Konieczny wrote:
> Add AES crypto HW acceleration for Exynos5433, with the help of SlimSSS IP.
> 
> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
> ---
>  drivers/crypto/s5p-sss.c | 50 ++++++++++++++++++++++++++++++++++++----
>  1 file changed, 46 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
> index 0064be0e3941..3f45cc5cb94a 100644
> --- a/drivers/crypto/s5p-sss.c
> +++ b/drivers/crypto/s5p-sss.c
> @@ -232,6 +232,7 @@
>   * struct samsung_aes_variant - platform specific SSS driver data
>   * @aes_offset: AES register offset from SSS module's base.
>   * @hash_offset: HASH register offset from SSS module's base.
> + * @clk_names: names of clocks needed to run SSS IP
>   *
>   * Specifies platform specific configuration of SSS module.
>   * Note: A structure for driver specific platform data is used for future
> @@ -240,6 +241,7 @@
>  struct samsung_aes_variant {
>  	unsigned int			aes_offset;
>  	unsigned int			hash_offset;
> +	const char			*clk_names[];

This array does not have a fixed size.

>  };
>  
>  struct s5p_aes_reqctx {
> @@ -296,6 +298,7 @@ struct s5p_aes_ctx {
>  struct s5p_aes_dev {
>  	struct device			*dev;
>  	struct clk			*clk;
> +	struct clk			*pclk;
>  	void __iomem			*ioaddr;
>  	void __iomem			*aes_ioaddr;
>  	int				irq_fc;
> @@ -384,11 +387,19 @@ struct s5p_hash_ctx {
>  static const struct samsung_aes_variant s5p_aes_data = {
>  	.aes_offset	= 0x4000,
>  	.hash_offset	= 0x6000,
> +	.clk_names	= { "secss", },

In this instantiation, there is no [1] element.

>  };
>  
>  static const struct samsung_aes_variant exynos_aes_data = {
>  	.aes_offset	= 0x200,
>  	.hash_offset	= 0x400,
> +	.clk_names	= { "secss", },

and neither is here.

> +};
> +
> +static const struct samsung_aes_variant exynos5433_slim_aes_data = {
> +	.aes_offset	= 0x400,
> +	.hash_offset	= 0x800,
> +	.clk_names	= { "pclk", "aclk", },
>  };
>  
>  static const struct of_device_id s5p_sss_dt_match[] = {
> @@ -400,6 +411,10 @@ static const struct of_device_id s5p_sss_dt_match[] = {
>  		.compatible = "samsung,exynos4210-secss",
>  		.data = &exynos_aes_data,
>  	},
> +	{
> +		.compatible = "samsung,exynos5433-slim-sss",
> +		.data = &exynos5433_slim_aes_data,
> +	},
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, s5p_sss_dt_match);
> @@ -2208,18 +2223,39 @@ static int s5p_aes_probe(struct platform_device *pdev)
>  			return PTR_ERR(pdata->ioaddr);
>  	}
>  
> -	pdata->clk = devm_clk_get(dev, "secss");
> +	pdata->clk = devm_clk_get(dev, variant->clk_names[0]);
>  	if (IS_ERR(pdata->clk)) {
> -		dev_err(dev, "failed to find secss clock source\n");
> +		dev_err(dev, "failed to find secss clock %s\n",
> +			variant->clk_names[0]);
>  		return -ENOENT;
>  	}
>  
>  	err = clk_prepare_enable(pdata->clk);
>  	if (err < 0) {
> -		dev_err(dev, "Enabling SSS clk failed, err %d\n", err);
> +		dev_err(dev, "Enabling clock %s failed, err %d\n",
> +			variant->clk_names[0], err);
>  		return err;
>  	}
>  
> +	if (variant->clk_names[1]) {

This results in

[   31.721963] Unhandled fault: page domain fault (0x01b) at 0x00004000
[   31.722417] pgd = (ptrval)
[   31.722611] [00004000] *pgd=00000000
[   31.723519] Internal error: : 1b [#1] PREEMPT SMP ARM
...
[   31.753264] [<c0be9978>] (strcmp) from [<c08cad50>] (of_property_match_string+0x58/0xd0)
[   31.754012] [<c08cad50>] (of_property_match_string) from [<c061eaac>] (of_clk_get_hw+0x5c/0x114)
[   31.754731] [<c061eaac>] (of_clk_get_hw) from [<c061656c>] (clk_get+0x34/0x74)
[   31.755397] [<c061656c>] (clk_get) from [<c0615dd8>] (devm_clk_get+0x3c/0x6c)
[   31.756018] [<c0615dd8>] (devm_clk_get) from [<c08b3298>] (s5p_aes_probe+0xcc/0x508)
[   31.756892] [<c08b3298>] (s5p_aes_probe) from [<c06e6740>] (platform_drv_probe+0x48/0x98)
[   31.757571] [<c06e6740>] (platform_drv_probe) from [<c06e40a4>] (really_probe+0x2bc/0x408)
[   31.758245] [<c06e40a4>] (really_probe) from [<c06e43a4>] (driver_probe_device+0x78/0x1cc)
[   31.758913] [<c06e43a4>] (driver_probe_device) from [<c06e4764>] (device_driver_attach+0x58/0x60)
[   31.759578] [<c06e4764>] (device_driver_attach) from [<c06e483c>] (__driver_attach+0xd0/0x168)
[   31.760010] [<c06e483c>] (__driver_attach) from [<c06e1e38>] (bus_for_each_dev+0x74/0xb4)
[   31.760354] [<c06e1e38>] (bus_for_each_dev) from [<c06e3174>] (bus_add_driver+0x174/0x210)
[   31.760697] [<c06e3174>] (bus_add_driver) from [<c06e572c>] (driver_register+0x74/0x108)
[   31.761039] [<c06e572c>] (driver_register) from [<c010325c>] (do_one_initcall+0x90/0x428)
[   31.761390] [<c010325c>] (do_one_initcall) from [<c1001318>] (kernel_init_freeable+0x42c/0x4cc)
[   31.761752] [<c1001318>] (kernel_init_freeable) from [<c0bf3ae8>] (kernel_init+0x8/0x118)
[   31.762093] [<c0bf3ae8>] (kernel_init) from [<c01010b4>] (ret_from_fork+0x14/0x20)

when booting one of the "old" platforms where variant->clk_names[1]
points beyond the end of the array.

Guenter

> +		pdata->pclk = devm_clk_get(dev, variant->clk_names[1]);
> +		if (IS_ERR(pdata->pclk)) {
> +			dev_err(dev, "failed to find clock %s\n",
> +				variant->clk_names[1]);
> +			err = -ENOENT;
> +			goto err_clk;
> +		}
> +
> +		err = clk_prepare_enable(pdata->pclk);
> +		if (err < 0) {
> +			dev_err(dev, "Enabling clock %s failed, err %d\n",
> +				variant->clk_names[0], err);
> +			goto err_clk;
> +		}
> +	} else {
> +		pdata->pclk = NULL;
> +	}
> +
>  	spin_lock_init(&pdata->lock);
>  	spin_lock_init(&pdata->hash_lock);
>  
> @@ -2295,8 +2331,11 @@ static int s5p_aes_probe(struct platform_device *pdev)
>  	tasklet_kill(&pdata->tasklet);
>  
>  err_irq:
> -	clk_disable_unprepare(pdata->clk);
> +	if (pdata->pclk)
> +		clk_disable_unprepare(pdata->pclk);
>  
> +err_clk:
> +	clk_disable_unprepare(pdata->clk);
>  	s5p_dev = NULL;
>  
>  	return err;
> @@ -2323,6 +2362,9 @@ static int s5p_aes_remove(struct platform_device *pdev)
>  		pdata->use_hash = false;
>  	}
>  
> +	if (pdata->pclk)
> +		clk_disable_unprepare(pdata->pclk);
> +
>  	clk_disable_unprepare(pdata->clk);
>  	s5p_dev = NULL;
>  

Re: [PATCH v4 3/3] crypto: s5p: add AES support for Exynos5433

[Krzysztof Kozlowski]

On Tue, 5 Mar 2019 at 18:13, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Fri, Feb 22, 2019 at 01:21:44PM +0100, Kamil Konieczny wrote:
> > Add AES crypto HW acceleration for Exynos5433, with the help of SlimSSS IP.
> >
> > Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
> > Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
> > ---
> >  drivers/crypto/s5p-sss.c | 50 ++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 46 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
> > index 0064be0e3941..3f45cc5cb94a 100644
> > --- a/drivers/crypto/s5p-sss.c
> > +++ b/drivers/crypto/s5p-sss.c
> > @@ -232,6 +232,7 @@
> >   * struct samsung_aes_variant - platform specific SSS driver data
> >   * @aes_offset: AES register offset from SSS module's base.
> >   * @hash_offset: HASH register offset from SSS module's base.
> > + * @clk_names: names of clocks needed to run SSS IP
> >   *
> >   * Specifies platform specific configuration of SSS module.
> >   * Note: A structure for driver specific platform data is used for future
> > @@ -240,6 +241,7 @@
> >  struct samsung_aes_variant {
> >       unsigned int                    aes_offset;
> >       unsigned int                    hash_offset;
> > +     const char                      *clk_names[];
>
> This array does not have a fixed size.

Yes, correct. Kamil already sent a fix:
https://patchwork.kernel.org/patch/10835389/

Best regards,
Krzysztof

Re: [PATCH v4 3/3] crypto: s5p: add AES support for Exynos5433

[Guenter Roeck]

On 3/5/19 11:51 AM, Krzysztof Kozlowski wrote:
> On Tue, 5 Mar 2019 at 18:13, Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On Fri, Feb 22, 2019 at 01:21:44PM +0100, Kamil Konieczny wrote:
>>> Add AES crypto HW acceleration for Exynos5433, with the help of SlimSSS IP.
>>>
>>> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
>>> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
>>> ---
>>>   drivers/crypto/s5p-sss.c | 50 ++++++++++++++++++++++++++++++++++++----
>>>   1 file changed, 46 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
>>> index 0064be0e3941..3f45cc5cb94a 100644
>>> --- a/drivers/crypto/s5p-sss.c
>>> +++ b/drivers/crypto/s5p-sss.c
>>> @@ -232,6 +232,7 @@
>>>    * struct samsung_aes_variant - platform specific SSS driver data
>>>    * @aes_offset: AES register offset from SSS module's base.
>>>    * @hash_offset: HASH register offset from SSS module's base.
>>> + * @clk_names: names of clocks needed to run SSS IP
>>>    *
>>>    * Specifies platform specific configuration of SSS module.
>>>    * Note: A structure for driver specific platform data is used for future
>>> @@ -240,6 +241,7 @@
>>>   struct samsung_aes_variant {
>>>        unsigned int                    aes_offset;
>>>        unsigned int                    hash_offset;
>>> +     const char                      *clk_names[];
>>
>> This array does not have a fixed size.
> 
> Yes, correct. Kamil already sent a fix:
> https://patchwork.kernel.org/patch/10835389/
> 

Excellent. Note that the offending patch is now in mainline.

Guenter

Re: [PATCH v4 0/3] add AES support for Exynos5433

[Herbert Xu]

On Fri, Feb 22, 2019 at 01:21:41PM +0100, Kamil Konieczny wrote:
> Add slimSSS node to DT and crypto AES support for Exynos5433. Tested on
> Exynos5433 board with crypto run-time self tests and with tcrypt with
> command insmod tcrypt.ko mode=500 sec=1
> 
> Changes since v3:
> - moved bindings documentation out from samsung-sss.txt into separate file
>   samsung-slimsss.txt
> 
> Changes since v2:
> - address Corentine Labbe note: add "const" to char* in declaration of
>   struct samsung_aes_variant
> 
> Changes since v1:
> - address Krzysztof Kozlowski review: add missing comma in struct
>   definition, add goto for error code path, correct indentation,
>   in documentation add that Exynos5433 has both slimSSS and SSS IPs.
> 
> Kamil Konieczny (3):
>   arm64: dts: exynos: add SlimSSS for Exynos5433
>   dt-bindings: crypto: document Exynos5433 SlimSSS
>   crypto: s5p: add AES support for Exynos5433
> 
>  .../bindings/crypto/samsung-slimsss.txt       | 19 +++++++
>  arch/arm64/boot/dts/exynos/exynos5433.dtsi    |  9 ++++
>  drivers/crypto/s5p-sss.c                      | 50 +++++++++++++++++--
>  3 files changed, 74 insertions(+), 4 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/crypto/samsung-slimsss.txt

Patches 2-3 applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt