[PATCH 1/2] rtc: imxdi: (trivial) fix a typo

[PATCH 1/2] rtc: imxdi: (trivial) fix a typo

[Martin Kaiser]

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/rtc/rtc-imxdi.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-imxdi.c b/drivers/rtc/rtc-imxdi.c
index 8d8049bd..67b56b8 100644
--- a/drivers/rtc/rtc-imxdi.c
+++ b/drivers/rtc/rtc-imxdi.c
@@ -67,7 +67,7 @@
 #define DSR_ETAD  (1 << 21)      /* External tamper A detected */
 #define DSR_EBD   (1 << 20)      /* External boot detected */
 #define DSR_SAD   (1 << 19)      /* SCC alarm detected */
-#define DSR_TTD   (1 << 18)      /* Temperatur tamper detected */
+#define DSR_TTD   (1 << 18)      /* Temperature tamper detected */
 #define DSR_CTD   (1 << 17)      /* Clock tamper detected */
 #define DSR_VTD   (1 << 16)      /* Voltage tamper detected */
 #define DSR_WBF   (1 << 10)      /* Write Busy Flag (synchronous) */
-- 
1.7.10.4

[PATCH 2/2] rtc: imxdi: use the security violation interrupt

[Martin Kaiser]

The DryIce chipset has a dedicated security violation interrupt that is
triggered for security violations (if configured to do so). According to
the publicly available imx258 reference manual, irq 56 is used for this
interrupt.

Install a handler for the security violation interrupt. Move the code
for handling security violations from the "normal" interrupt handler
into the security violation interrupt handler.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 .../devicetree/bindings/rtc/imxdi-rtc.txt          |    4 +-
 arch/arm/boot/dts/imx25.dtsi                       |    2 +-
 drivers/rtc/rtc-imxdi.c                            |   64 ++++++++++++++------
 3 files changed, 49 insertions(+), 21 deletions(-)

diff --git a/Documentation/devicetree/bindings/rtc/imxdi-rtc.txt b/Documentation/devicetree/bindings/rtc/imxdi-rtc.txt
index c9d80d7..c53c7d8 100644
--- a/Documentation/devicetree/bindings/rtc/imxdi-rtc.txt
+++ b/Documentation/devicetree/bindings/rtc/imxdi-rtc.txt
@@ -6,12 +6,12 @@ Required properties:
 - compatible: should be: "fsl,imx25-rtc"
 - reg: physical base address of the controller and length of memory mapped
   region.
-- interrupts: rtc alarm interrupt
+- interrupts: rtc alarm interrupt, dryice security violation interrupt
 
 Example:
 
 rtc@80056000 {
 	compatible = "fsl,imx53-rtc", "fsl,imx25-rtc";
 	reg = <0x80056000 2000>;
-	interrupts = <29>;
+	interrupts = <29>, <56>;
 };
diff --git a/arch/arm/boot/dts/imx25.dtsi b/arch/arm/boot/dts/imx25.dtsi
index 831d09a..3b754e3 100644
--- a/arch/arm/boot/dts/imx25.dtsi
+++ b/arch/arm/boot/dts/imx25.dtsi
@@ -560,7 +560,7 @@
 				reg = <0x53ffc000 0x4000>;
 				clocks = <&clks 81>;
 				clock-names = "ipg";
-				interrupts = <25>;
+				interrupts = <25>, <56>;
 			};
 		};
 
diff --git a/drivers/rtc/rtc-imxdi.c b/drivers/rtc/rtc-imxdi.c
index 67b56b8..9e1cf49 100644
--- a/drivers/rtc/rtc-imxdi.c
+++ b/drivers/rtc/rtc-imxdi.c
@@ -109,6 +109,7 @@
  * @rtc: pointer to rtc struct
  * @ioaddr: IO registers pointer
  * @irq: dryice normal interrupt
+ * @sec_irq: dryice security violation interrupt
  * @clk: input reference clock
  * @dsr: copy of the DSR register
  * @irq_lock: interrupt enable register (DIER) lock
@@ -121,6 +122,7 @@ struct imxdi_dev {
 	struct rtc_device *rtc;
 	void __iomem *ioaddr;
 	int irq;
+	int sec_irq;
 	struct clk *clk;
 	u32 dsr;
 	spinlock_t irq_lock;
@@ -688,24 +690,6 @@ static irqreturn_t dryice_norm_irq(int irq, void *dev_id)
 	dier = readl(imxdi->ioaddr + DIER);
 	dsr = readl(imxdi->ioaddr + DSR);
 
-	/* handle the security violation event */
-	if (dier & DIER_SVIE) {
-		if (dsr & DSR_SVF) {
-			/*
-			 * Disable the interrupt when this kind of event has
-			 * happened.
-			 * There cannot be more than one event of this type,
-			 * because it needs a complex state change
-			 * including a main power cycle to get again out of
-			 * this state.
-			 */
-			di_int_disable(imxdi, DIER_SVIE);
-			/* report the violation */
-			di_report_tamper_info(imxdi, dsr);
-			rc = IRQ_HANDLED;
-		}
-	}
-
 	/* handle write complete and write error cases */
 	if (dier & DIER_WCIE) {
 		/*If the write wait queue is empty then there is no pending
@@ -743,6 +727,40 @@ static irqreturn_t dryice_norm_irq(int irq, void *dev_id)
 }
 
 /*
+ * dryice security violation interrupt handler
+ */
+static irqreturn_t dryice_sec_irq(int irq, void *dev_id)
+{
+	struct imxdi_dev *imxdi = dev_id;
+	u32 dsr, dier;
+	irqreturn_t rc = IRQ_NONE;
+
+	dier = readl(imxdi->ioaddr + DIER);
+	dsr = readl(imxdi->ioaddr + DSR);
+
+	/* handle the security violation event */
+	if (dier & DIER_SVIE) {
+		if (dsr & DSR_SVF) {
+			/*
+			 * Disable the interrupt when this kind of event has
+			 * happened.
+			 * There cannot be more than one event of this type,
+			 * because it needs a complex state change
+			 * including a main power cycle to get again out of
+			 * this state.
+			 */
+			di_int_disable(imxdi, DIER_SVIE);
+			/* report the violation */
+			di_report_tamper_info(imxdi, dsr);
+			rc = IRQ_HANDLED;
+		}
+	}
+
+	return rc;
+}
+
+
+/*
  * post the alarm event from user context so it can sleep
  * on the write completion.
  */
@@ -783,6 +801,9 @@ static int __init dryice_rtc_probe(struct platform_device *pdev)
 	imxdi->irq = platform_get_irq(pdev, 0);
 	if (imxdi->irq < 0)
 		return imxdi->irq;
+	imxdi->sec_irq = platform_get_irq(pdev, 1);
+	if (imxdi->sec_irq < 0)
+		return imxdi->sec_irq;
 
 	init_waitqueue_head(&imxdi->write_wait);
 
@@ -815,6 +836,13 @@ static int __init dryice_rtc_probe(struct platform_device *pdev)
 		goto err;
 	}
 
+	rc = devm_request_irq(&pdev->dev, imxdi->sec_irq, dryice_sec_irq,
+			IRQF_SHARED, pdev->name, imxdi);
+	if (rc) {
+		dev_warn(&pdev->dev, "security violation interrupt not available.\n");
+		goto err;
+	}
+
 	platform_set_drvdata(pdev, imxdi);
 	imxdi->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
 				  &dryice_rtc_ops, THIS_MODULE);
-- 
1.7.10.4

Re: [PATCH 2/2] rtc: imxdi: use the security violation interrupt

[Alexandre Belloni]

Hi,

On 19/11/2016 at 14:03:34 +0100, Martin Kaiser wrote :
> The DryIce chipset has a dedicated security violation interrupt that is
> triggered for security violations (if configured to do so). According to
> the publicly available imx258 reference manual, irq 56 is used for this
> interrupt.
> 
> Install a handler for the security violation interrupt. Move the code
> for handling security violations from the "normal" interrupt handler
> into the security violation interrupt handler.
> 
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
>  .../devicetree/bindings/rtc/imxdi-rtc.txt          |    4 +-
>  arch/arm/boot/dts/imx25.dtsi                       |    2 +-

This change should be in a separate patch.

> @@ -783,6 +801,9 @@ static int __init dryice_rtc_probe(struct platform_device *pdev)
>  	imxdi->irq = platform_get_irq(pdev, 0);
>  	if (imxdi->irq < 0)
>  		return imxdi->irq;
> +	imxdi->sec_irq = platform_get_irq(pdev, 1);
> +	if (imxdi->sec_irq < 0)
> +		return imxdi->sec_irq;
>  
>  	init_waitqueue_head(&imxdi->write_wait);
>  
> @@ -815,6 +836,13 @@ static int __init dryice_rtc_probe(struct platform_device *pdev)
>  		goto err;
>  	}
>  
> +	rc = devm_request_irq(&pdev->dev, imxdi->sec_irq, dryice_sec_irq,
> +			IRQF_SHARED, pdev->name, imxdi);
> +	if (rc) {
> +		dev_warn(&pdev->dev, "security violation interrupt not available.\n");
> +		goto err;

If you make that error fatal, it means that the DT ABI changed. I would
simply issue the warning and continue anyway.

> +	}
> +
>  	platform_set_drvdata(pdev, imxdi);
>  	imxdi->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
>  				  &dryice_rtc_ops, THIS_MODULE);
> -- 
> 1.7.10.4
> 

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 1/2] rtc: imxdi: (trivial) fix a typo

[Alexandre Belloni]

On 19/11/2016 at 14:03:33 +0100, Martin Kaiser wrote :
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
>  drivers/rtc/rtc-imxdi.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
Applied, thanks.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH v2] rtc: imxdi: use the security violation interrupt

[Martin Kaiser]

The DryIce chipset has a dedicated security violation interrupt that is
triggered for security violations (if configured to do so).  According
to the publicly available imx258 reference manual, irq 56 is used for
this interrupt.

Install a handler for the security violation interrupt if an irq for
this is provided by platform data / device tree. Move the code for
handling security violations from the "normal" interrupt handler into
the security violation interrupt handler.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
v2:
  - make sec_irq optional to avoid breaking the Device Tree ABI
  - removed the Device Tree bindings, I'll prepare a separate patch for them

 drivers/rtc/rtc-imxdi.c |   68 ++++++++++++++++++++++++++++++++++-------------
 1 file changed, 50 insertions(+), 18 deletions(-)

diff --git a/drivers/rtc/rtc-imxdi.c b/drivers/rtc/rtc-imxdi.c
index 67b56b8..ec6077a0 100644
--- a/drivers/rtc/rtc-imxdi.c
+++ b/drivers/rtc/rtc-imxdi.c
@@ -109,6 +109,7 @@
  * @rtc: pointer to rtc struct
  * @ioaddr: IO registers pointer
  * @irq: dryice normal interrupt
+ * @sec_irq: dryice security violation interrupt
  * @clk: input reference clock
  * @dsr: copy of the DSR register
  * @irq_lock: interrupt enable register (DIER) lock
@@ -121,6 +122,7 @@ struct imxdi_dev {
 	struct rtc_device *rtc;
 	void __iomem *ioaddr;
 	int irq;
+	int sec_irq;
 	struct clk *clk;
 	u32 dsr;
 	spinlock_t irq_lock;
@@ -688,24 +690,6 @@ static irqreturn_t dryice_norm_irq(int irq, void *dev_id)
 	dier = readl(imxdi->ioaddr + DIER);
 	dsr = readl(imxdi->ioaddr + DSR);
 
-	/* handle the security violation event */
-	if (dier & DIER_SVIE) {
-		if (dsr & DSR_SVF) {
-			/*
-			 * Disable the interrupt when this kind of event has
-			 * happened.
-			 * There cannot be more than one event of this type,
-			 * because it needs a complex state change
-			 * including a main power cycle to get again out of
-			 * this state.
-			 */
-			di_int_disable(imxdi, DIER_SVIE);
-			/* report the violation */
-			di_report_tamper_info(imxdi, dsr);
-			rc = IRQ_HANDLED;
-		}
-	}
-
 	/* handle write complete and write error cases */
 	if (dier & DIER_WCIE) {
 		/*If the write wait queue is empty then there is no pending
@@ -743,6 +727,40 @@ static irqreturn_t dryice_norm_irq(int irq, void *dev_id)
 }
 
 /*
+ * dryice security violation interrupt handler
+ */
+static irqreturn_t dryice_sec_irq(int irq, void *dev_id)
+{
+	struct imxdi_dev *imxdi = dev_id;
+	u32 dsr, dier;
+	irqreturn_t rc = IRQ_NONE;
+
+	dier = readl(imxdi->ioaddr + DIER);
+	dsr = readl(imxdi->ioaddr + DSR);
+
+	/* handle the security violation event */
+	if (dier & DIER_SVIE) {
+		if (dsr & DSR_SVF) {
+			/*
+			 * Disable the interrupt when this kind of event has
+			 * happened.
+			 * There cannot be more than one event of this type,
+			 * because it needs a complex state change
+			 * including a main power cycle to get again out of
+			 * this state.
+			 */
+			di_int_disable(imxdi, DIER_SVIE);
+			/* report the violation */
+			di_report_tamper_info(imxdi, dsr);
+			rc = IRQ_HANDLED;
+		}
+	}
+
+	return rc;
+}
+
+
+/*
  * post the alarm event from user context so it can sleep
  * on the write completion.
  */
@@ -783,6 +801,13 @@ static int __init dryice_rtc_probe(struct platform_device *pdev)
 	imxdi->irq = platform_get_irq(pdev, 0);
 	if (imxdi->irq < 0)
 		return imxdi->irq;
+	/* the 2nd irq is the security violation irq
+	   make this optional, don't break the device tree ABI */
+	rc = platform_get_irq(pdev, 1);
+	if (rc > 0)
+		imxdi->sec_irq = rc;
+	else
+		imxdi->sec_irq = IRQ_NOTCONNECTED;
 
 	init_waitqueue_head(&imxdi->write_wait);
 
@@ -815,6 +840,13 @@ static int __init dryice_rtc_probe(struct platform_device *pdev)
 		goto err;
 	}
 
+	rc = devm_request_irq(&pdev->dev, imxdi->sec_irq, dryice_sec_irq,
+			IRQF_SHARED, pdev->name, imxdi);
+	if (rc) {
+		dev_warn(&pdev->dev, "security violation interrupt not available.\n");
+		/* this is not an error, see above */
+	}
+
 	platform_set_drvdata(pdev, imxdi);
 	imxdi->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
 				  &dryice_rtc_ops, THIS_MODULE);
-- 
1.7.10.4

Re: [PATCH v2] rtc: imxdi: use the security violation interrupt

[Lucas Stach]

Am Montag, den 19.12.2016, 23:41 +0100 schrieb Martin Kaiser:
> The DryIce chipset has a dedicated security violation interrupt that is
> triggered for security violations (if configured to do so).  According
> to the publicly available imx258 reference manual, irq 56 is used for
> this interrupt.
> 
> Install a handler for the security violation interrupt if an irq for
> this is provided by platform data / device tree. Move the code for
> handling security violations from the "normal" interrupt handler into
> the security violation interrupt handler.
> 
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
> v2:
>   - make sec_irq optional to avoid breaking the Device Tree ABI
>   - removed the Device Tree bindings, I'll prepare a separate patch for them

Please submit the binding as a separate patch in the same series.
Otherwise you are building de-facto DT bindings by changing the driver.

> 
>  drivers/rtc/rtc-imxdi.c |   68 ++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 50 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-imxdi.c b/drivers/rtc/rtc-imxdi.c
> index 67b56b8..ec6077a0 100644
> --- a/drivers/rtc/rtc-imxdi.c
> +++ b/drivers/rtc/rtc-imxdi.c
> @@ -109,6 +109,7 @@
>   * @rtc: pointer to rtc struct
>   * @ioaddr: IO registers pointer
>   * @irq: dryice normal interrupt
> + * @sec_irq: dryice security violation interrupt

This isn't used outside the probe routine, so doesn't need to be saved
in the driver data.

>   * @clk: input reference clock
>   * @dsr: copy of the DSR register
>   * @irq_lock: interrupt enable register (DIER) lock
> @@ -121,6 +122,7 @@ struct imxdi_dev {
>  	struct rtc_device *rtc;
>  	void __iomem *ioaddr;
>  	int irq;
> +	int sec_irq;
>  	struct clk *clk;
>  	u32 dsr;
>  	spinlock_t irq_lock;
> @@ -688,24 +690,6 @@ static irqreturn_t dryice_norm_irq(int irq, void *dev_id)
>  	dier = readl(imxdi->ioaddr + DIER);
>  	dsr = readl(imxdi->ioaddr + DSR);
>  
> -	/* handle the security violation event */
> -	if (dier & DIER_SVIE) {
> -		if (dsr & DSR_SVF) {
> -			/*
> -			 * Disable the interrupt when this kind of event has
> -			 * happened.
> -			 * There cannot be more than one event of this type,
> -			 * because it needs a complex state change
> -			 * including a main power cycle to get again out of
> -			 * this state.
> -			 */
> -			di_int_disable(imxdi, DIER_SVIE);
> -			/* report the violation */
> -			di_report_tamper_info(imxdi, dsr);
> -			rc = IRQ_HANDLED;
> -		}
> -	}
> -
>  	/* handle write complete and write error cases */
>  	if (dier & DIER_WCIE) {
>  		/*If the write wait queue is empty then there is no pending
> @@ -743,6 +727,40 @@ static irqreturn_t dryice_norm_irq(int irq, void *dev_id)
>  }
>  
>  /*
> + * dryice security violation interrupt handler
> + */
> +static irqreturn_t dryice_sec_irq(int irq, void *dev_id)
> +{
> +	struct imxdi_dev *imxdi = dev_id;
> +	u32 dsr, dier;
> +	irqreturn_t rc = IRQ_NONE;
> +
> +	dier = readl(imxdi->ioaddr + DIER);
> +	dsr = readl(imxdi->ioaddr + DSR);
> +
> +	/* handle the security violation event */
> +	if (dier & DIER_SVIE) {
> +		if (dsr & DSR_SVF) {
> +			/*
> +			 * Disable the interrupt when this kind of event has
> +			 * happened.
> +			 * There cannot be more than one event of this type,
> +			 * because it needs a complex state change
> +			 * including a main power cycle to get again out of
> +			 * this state.
> +			 */
> +			di_int_disable(imxdi, DIER_SVIE);
> +			/* report the violation */
> +			di_report_tamper_info(imxdi, dsr);
> +			rc = IRQ_HANDLED;
> +		}
> +	}
> +
> +	return rc;
> +}

This separate handler isn't needed. It is reading the same IRQ status
register, so you can just leave the code as is and request the sec-irq
with the same "dryice_norm_irq" handler.

> +
> +
> +/*
>   * post the alarm event from user context so it can sleep
>   * on the write completion.
>   */
> @@ -783,6 +801,13 @@ static int __init dryice_rtc_probe(struct platform_device *pdev)
>  	imxdi->irq = platform_get_irq(pdev, 0);
>  	if (imxdi->irq < 0)
>  		return imxdi->irq;

Insert blank line for better legibility.

> +	/* the 2nd irq is the security violation irq
> +	   make this optional, don't break the device tree ABI */
> +	rc = platform_get_irq(pdev, 1);
> +	if (rc > 0)
> +		imxdi->sec_irq = rc;
> +	else
> +		imxdi->sec_irq = IRQ_NOTCONNECTED;
>  
>  	init_waitqueue_head(&imxdi->write_wait);
>  
> @@ -815,6 +840,13 @@ static int __init dryice_rtc_probe(struct platform_device *pdev)
>  		goto err;
>  	}
>  
> +	rc = devm_request_irq(&pdev->dev, imxdi->sec_irq, dryice_sec_irq,
> +			IRQF_SHARED, pdev->name, imxdi);
> +	if (rc) {
> +		dev_warn(&pdev->dev, "security violation interrupt not available.\n");
> +		/* this is not an error, see above */
> +	}
> +
Please just fold this into the "if (rc > 0)" path above.

>  	platform_set_drvdata(pdev, imxdi);
>  	imxdi->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
>  				  &dryice_rtc_ops, THIS_MODULE);

Regards,
Lucas

[PATCH 1/2 v3] rtc: imxdi: use the security violation interrupt

[Martin Kaiser]

The DryIce chipset has a dedicated security violation interrupt that is
triggered for security violations (if configured to do so).  According
to the publicly available imx258 reference manual, irq 56 is used for
this interrupt.

If an irq number is provided for the security violation interrupt,
install the same handler that we're already using for the "normal"
interrupt.

imxdi->irq is used only in the probe function, make it a local variable.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
v3:
  - add the device tree changes to this patch series
  - install the same interrupt handler for normal and sec irq, rename 
    the handler function accordingly
  - make imxdi->irq and imxdi->sec_irq local variables in the probe 
    function

v2:
  - make sec_irq optional to avoid breaking the Device Tree ABI
  - removed the Device Tree bindings, I'll prepare a separate patch for them

 drivers/rtc/rtc-imxdi.c |   28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/rtc/rtc-imxdi.c b/drivers/rtc/rtc-imxdi.c
index 67b56b8..73d6908 100644
--- a/drivers/rtc/rtc-imxdi.c
+++ b/drivers/rtc/rtc-imxdi.c
@@ -108,7 +108,6 @@
  * @pdev: pionter to platform dev
  * @rtc: pointer to rtc struct
  * @ioaddr: IO registers pointer
- * @irq: dryice normal interrupt
  * @clk: input reference clock
  * @dsr: copy of the DSR register
  * @irq_lock: interrupt enable register (DIER) lock
@@ -120,7 +119,6 @@ struct imxdi_dev {
 	struct platform_device *pdev;
 	struct rtc_device *rtc;
 	void __iomem *ioaddr;
-	int irq;
 	struct clk *clk;
 	u32 dsr;
 	spinlock_t irq_lock;
@@ -677,9 +675,9 @@ static int dryice_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 };
 
 /*
- * dryice "normal" interrupt handler
+ * interrupt handler for dryice "normal" and security violation interrupt
  */
-static irqreturn_t dryice_norm_irq(int irq, void *dev_id)
+static irqreturn_t dryice_irq(int irq, void *dev_id)
 {
 	struct imxdi_dev *imxdi = dev_id;
 	u32 dsr, dier;
@@ -765,6 +763,7 @@ static int __init dryice_rtc_probe(struct platform_device *pdev)
 {
 	struct resource *res;
 	struct imxdi_dev *imxdi;
+	int norm_irq, sec_irq;
 	int rc;
 
 	imxdi = devm_kzalloc(&pdev->dev, sizeof(*imxdi), GFP_KERNEL);
@@ -780,9 +779,15 @@ static int __init dryice_rtc_probe(struct platform_device *pdev)
 
 	spin_lock_init(&imxdi->irq_lock);
 
-	imxdi->irq = platform_get_irq(pdev, 0);
-	if (imxdi->irq < 0)
-		return imxdi->irq;
+	norm_irq = platform_get_irq(pdev, 0);
+	if (norm_irq < 0)
+		return norm_irq;
+
+	/* the 2nd irq is the security violation irq
+	   make this optional, don't break the device tree ABI */
+	sec_irq = platform_get_irq(pdev, 1);
+	if (sec_irq <= 0)
+		sec_irq = IRQ_NOTCONNECTED;
 
 	init_waitqueue_head(&imxdi->write_wait);
 
@@ -808,13 +813,20 @@ static int __init dryice_rtc_probe(struct platform_device *pdev)
 	if (rc != 0)
 		goto err;
 
-	rc = devm_request_irq(&pdev->dev, imxdi->irq, dryice_norm_irq,
+	rc = devm_request_irq(&pdev->dev, norm_irq, dryice_irq,
 			IRQF_SHARED, pdev->name, imxdi);
 	if (rc) {
 		dev_warn(&pdev->dev, "interrupt not available.\n");
 		goto err;
 	}
 
+	rc = devm_request_irq(&pdev->dev, sec_irq, dryice_irq,
+			IRQF_SHARED, pdev->name, imxdi);
+	if (rc) {
+		dev_warn(&pdev->dev, "security violation interrupt not available.\n");
+		/* this is not an error, see above */
+	}
+
 	platform_set_drvdata(pdev, imxdi);
 	imxdi->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
 				  &dryice_rtc_ops, THIS_MODULE);
-- 
1.7.10.4

[PATCH 2/2 v2] ARM: i.MX25: add the optional security violation irq

[Martin Kaiser]

Add the security violation irq as an optional entry to rtc-imxdi's
device tree bindings and to i.MX25's dtsi file.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
v2:
  - clarify that the security violation interrupt is optional
  - use the same syntax as everwhere else for lists of interrupt numbers

 Documentation/devicetree/bindings/rtc/imxdi-rtc.txt |    5 ++++-
 arch/arm/boot/dts/imx25.dtsi                        |    2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/rtc/imxdi-rtc.txt b/Documentation/devicetree/bindings/rtc/imxdi-rtc.txt
index c9d80d7..323cf26 100644
--- a/Documentation/devicetree/bindings/rtc/imxdi-rtc.txt
+++ b/Documentation/devicetree/bindings/rtc/imxdi-rtc.txt
@@ -8,10 +8,13 @@ Required properties:
   region.
 - interrupts: rtc alarm interrupt
 
+Optional properties:
+- interrupts: dryice security violation interrupt
+
 Example:
 
 rtc@80056000 {
 	compatible = "fsl,imx53-rtc", "fsl,imx25-rtc";
 	reg = <0x80056000 2000>;
-	interrupts = <29>;
+	interrupts = <29 56>;
 };
diff --git a/arch/arm/boot/dts/imx25.dtsi b/arch/arm/boot/dts/imx25.dtsi
index 831d09a..331d1e1 100644
--- a/arch/arm/boot/dts/imx25.dtsi
+++ b/arch/arm/boot/dts/imx25.dtsi
@@ -560,7 +560,7 @@
 				reg = <0x53ffc000 0x4000>;
 				clocks = <&clks 81>;
 				clock-names = "ipg";
-				interrupts = <25>;
+				interrupts = <25 56>;
 			};
 		};
 
-- 
1.7.10.4

Re: [PATCH v2] rtc: imxdi: use the security violation interrupt

[Martin Kaiser]

Hello Lucas,

thanks for taking the time to review my patch.

Thus wrote Lucas Stach (l.stach@pengutronix.de):

> > diff --git a/drivers/rtc/rtc-imxdi.c b/drivers/rtc/rtc-imxdi.c
> > index 67b56b8..ec6077a0 100644
> > --- a/drivers/rtc/rtc-imxdi.c
> > +++ b/drivers/rtc/rtc-imxdi.c
> > @@ -109,6 +109,7 @@
> >   * @rtc: pointer to rtc struct
> >   * @ioaddr: IO registers pointer
> >   * @irq: dryice normal interrupt
> > + * @sec_irq: dryice security violation interrupt

> This isn't used outside the probe routine, so doesn't need to be saved
> in the driver data.

The same goes for imxdi->irq, I made this a local variable as well.

> > +	rc = devm_request_irq(&pdev->dev, imxdi->sec_irq, dryice_sec_irq,
> > +			IRQF_SHARED, pdev->name, imxdi);
> > +	if (rc) {
> > +		dev_warn(&pdev->dev, "security violation interrupt not available.\n");
> > +		/* this is not an error, see above */
> > +	}
> > +
> Please just fold this into the "if (rc > 0)" path above.

Are you sure that this would be correct?

My understanding is that we should mask the interrupts by writing 0 into
DIER and set the DryIce to a sane state in di_handle_state() before we
install interrupt handlers.

Best regards,
Martin

Re: [PATCH 2/2 v2] ARM: i.MX25: add the optional security violation irq

[Shawn Guo]

On Thu, Dec 22, 2016 at 12:01:16AM +0100, Martin Kaiser wrote:
> Add the security violation irq as an optional entry to rtc-imxdi's
> device tree bindings and to i.MX25's dtsi file.
> 
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
> v2:
>   - clarify that the security violation interrupt is optional
>   - use the same syntax as everwhere else for lists of interrupt numbers
> 
>  Documentation/devicetree/bindings/rtc/imxdi-rtc.txt |    5 ++++-
>  arch/arm/boot/dts/imx25.dtsi                        |    2 +-

I take DTS changes only.  Please make the bindings doc change a separate
patch, and have it go together with driver patch.  And I apply DTS patch
only after driver and bindings changes get accepted by RTC maintainer.

Shawn

>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/rtc/imxdi-rtc.txt b/Documentation/devicetree/bindings/rtc/imxdi-rtc.txt
> index c9d80d7..323cf26 100644
> --- a/Documentation/devicetree/bindings/rtc/imxdi-rtc.txt
> +++ b/Documentation/devicetree/bindings/rtc/imxdi-rtc.txt
> @@ -8,10 +8,13 @@ Required properties:
>    region.
>  - interrupts: rtc alarm interrupt
>  
> +Optional properties:
> +- interrupts: dryice security violation interrupt
> +
>  Example:
>  
>  rtc@80056000 {
>  	compatible = "fsl,imx53-rtc", "fsl,imx25-rtc";
>  	reg = <0x80056000 2000>;
> -	interrupts = <29>;
> +	interrupts = <29 56>;
>  };
> diff --git a/arch/arm/boot/dts/imx25.dtsi b/arch/arm/boot/dts/imx25.dtsi
> index 831d09a..331d1e1 100644
> --- a/arch/arm/boot/dts/imx25.dtsi
> +++ b/arch/arm/boot/dts/imx25.dtsi
> @@ -560,7 +560,7 @@
>  				reg = <0x53ffc000 0x4000>;
>  				clocks = <&clks 81>;
>  				clock-names = "ipg";
> -				interrupts = <25>;
> +				interrupts = <25 56>;
>  			};
>  		};
>  
> -- 
> 1.7.10.4
> 

[PATCH 1/3 v4] rtc: imxdi: use the security violation interrupt

[Martin Kaiser]

The DryIce chipset has a dedicated security violation interrupt that is
triggered for security violations (if configured to do so).  According
to the publicly available imx258 reference manual, irq 56 is used for
this interrupt.

If an irq number is provided for the security violation interrupt,
install the same handler that we're already using for the "normal"
interrupt.

imxdi->irq is used only in the probe function, make it a local variable.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
v4:
  - separate patches for imx25.dtsi and documentation of the new dt binding

v3:
  - add the device tree changes to this patch series
  - install the same interrupt handler for normal and sec irq, rename
    the handler function accordingly
  - make imxdi->irq and imxdi->sec_irq local variables in the probe
    function

v2:
  - make sec_irq optional to avoid breaking the Device Tree ABI
  - removed the Device Tree bindings, I'll prepare a separate patch for them


 drivers/rtc/rtc-imxdi.c |   28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/rtc/rtc-imxdi.c b/drivers/rtc/rtc-imxdi.c
index 67b56b8..73d6908 100644
--- a/drivers/rtc/rtc-imxdi.c
+++ b/drivers/rtc/rtc-imxdi.c
@@ -108,7 +108,6 @@
  * @pdev: pionter to platform dev
  * @rtc: pointer to rtc struct
  * @ioaddr: IO registers pointer
- * @irq: dryice normal interrupt
  * @clk: input reference clock
  * @dsr: copy of the DSR register
  * @irq_lock: interrupt enable register (DIER) lock
@@ -120,7 +119,6 @@ struct imxdi_dev {
 	struct platform_device *pdev;
 	struct rtc_device *rtc;
 	void __iomem *ioaddr;
-	int irq;
 	struct clk *clk;
 	u32 dsr;
 	spinlock_t irq_lock;
@@ -677,9 +675,9 @@ static int dryice_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 };
 
 /*
- * dryice "normal" interrupt handler
+ * interrupt handler for dryice "normal" and security violation interrupt
  */
-static irqreturn_t dryice_norm_irq(int irq, void *dev_id)
+static irqreturn_t dryice_irq(int irq, void *dev_id)
 {
 	struct imxdi_dev *imxdi = dev_id;
 	u32 dsr, dier;
@@ -765,6 +763,7 @@ static int __init dryice_rtc_probe(struct platform_device *pdev)
 {
 	struct resource *res;
 	struct imxdi_dev *imxdi;
+	int norm_irq, sec_irq;
 	int rc;
 
 	imxdi = devm_kzalloc(&pdev->dev, sizeof(*imxdi), GFP_KERNEL);
@@ -780,9 +779,15 @@ static int __init dryice_rtc_probe(struct platform_device *pdev)
 
 	spin_lock_init(&imxdi->irq_lock);
 
-	imxdi->irq = platform_get_irq(pdev, 0);
-	if (imxdi->irq < 0)
-		return imxdi->irq;
+	norm_irq = platform_get_irq(pdev, 0);
+	if (norm_irq < 0)
+		return norm_irq;
+
+	/* the 2nd irq is the security violation irq
+	   make this optional, don't break the device tree ABI */
+	sec_irq = platform_get_irq(pdev, 1);
+	if (sec_irq <= 0)
+		sec_irq = IRQ_NOTCONNECTED;
 
 	init_waitqueue_head(&imxdi->write_wait);
 
@@ -808,13 +813,20 @@ static int __init dryice_rtc_probe(struct platform_device *pdev)
 	if (rc != 0)
 		goto err;
 
-	rc = devm_request_irq(&pdev->dev, imxdi->irq, dryice_norm_irq,
+	rc = devm_request_irq(&pdev->dev, norm_irq, dryice_irq,
 			IRQF_SHARED, pdev->name, imxdi);
 	if (rc) {
 		dev_warn(&pdev->dev, "interrupt not available.\n");
 		goto err;
 	}
 
+	rc = devm_request_irq(&pdev->dev, sec_irq, dryice_irq,
+			IRQF_SHARED, pdev->name, imxdi);
+	if (rc) {
+		dev_warn(&pdev->dev, "security violation interrupt not available.\n");
+		/* this is not an error, see above */
+	}
+
 	platform_set_drvdata(pdev, imxdi);
 	imxdi->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
 				  &dryice_rtc_ops, THIS_MODULE);
-- 
1.7.10.4

[PATCH 2/3] ARM: dts: imx25.dtsi: DryIce security violation interrupt

[Martin Kaiser]

The DryIce block on i.MX25 chipset uses two interrupts: A normal and a
security violation interrupt. Add the security violation interrupt to
the list, it is optional.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 arch/arm/boot/dts/imx25.dtsi |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/imx25.dtsi b/arch/arm/boot/dts/imx25.dtsi
index 831d09a..331d1e1 100644
--- a/arch/arm/boot/dts/imx25.dtsi
+++ b/arch/arm/boot/dts/imx25.dtsi
@@ -560,7 +560,7 @@
 				reg = <0x53ffc000 0x4000>;
 				clocks = <&clks 81>;
 				clock-names = "ipg";
-				interrupts = <25>;
+				interrupts = <25 56>;
 			};
 		};
 
-- 
1.7.10.4

[PATCH 3/3] Documentation: dt: rtc-imxdi: security violation interrupt

[Martin Kaiser]

Document the DryIce security violation interrupt.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 Documentation/devicetree/bindings/rtc/imxdi-rtc.txt |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/rtc/imxdi-rtc.txt b/Documentation/devicetree/bindings/rtc/imxdi-rtc.txt
index c9d80d7..323cf26 100644
--- a/Documentation/devicetree/bindings/rtc/imxdi-rtc.txt
+++ b/Documentation/devicetree/bindings/rtc/imxdi-rtc.txt
@@ -8,10 +8,13 @@ Required properties:
   region.
 - interrupts: rtc alarm interrupt
 
+Optional properties:
+- interrupts: dryice security violation interrupt
+
 Example:
 
 rtc@80056000 {
 	compatible = "fsl,imx53-rtc", "fsl,imx25-rtc";
 	reg = <0x80056000 2000>;
-	interrupts = <29>;
+	interrupts = <29 56>;
 };
-- 
1.7.10.4

Re: [PATCH 2/3] ARM: dts: imx25.dtsi: DryIce security violation interrupt

[Shawn Guo]

On Tue, Jan 03, 2017 at 07:50:57PM +0100, Martin Kaiser wrote:
> The DryIce block on i.MX25 chipset uses two interrupts: A normal and a
> security violation interrupt. Add the security violation interrupt to
> the list, it is optional.
> 
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>

It looks like an independent patch, which improves the hardware
description.  So I just applied it.

Shawn

> ---
>  arch/arm/boot/dts/imx25.dtsi |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/imx25.dtsi b/arch/arm/boot/dts/imx25.dtsi
> index 831d09a..331d1e1 100644
> --- a/arch/arm/boot/dts/imx25.dtsi
> +++ b/arch/arm/boot/dts/imx25.dtsi
> @@ -560,7 +560,7 @@
>  				reg = <0x53ffc000 0x4000>;
>  				clocks = <&clks 81>;
>  				clock-names = "ipg";
> -				interrupts = <25>;
> +				interrupts = <25 56>;
>  			};
>  		};
>  
> -- 
> 1.7.10.4
> 

Re: [PATCH 1/3 v4] rtc: imxdi: use the security violation interrupt

[Alexandre Belloni]

On 03/01/2017 at 19:49:50 +0100, Martin Kaiser wrote :
> The DryIce chipset has a dedicated security violation interrupt that is
> triggered for security violations (if configured to do so).  According
> to the publicly available imx258 reference manual, irq 56 is used for
> this interrupt.
> 
> If an irq number is provided for the security violation interrupt,
> install the same handler that we're already using for the "normal"
> interrupt.
> 
> imxdi->irq is used only in the probe function, make it a local variable.
> 
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
> v4:
>   - separate patches for imx25.dtsi and documentation of the new dt binding
> 
> v3:
>   - add the device tree changes to this patch series
>   - install the same interrupt handler for normal and sec irq, rename
>     the handler function accordingly
>   - make imxdi->irq and imxdi->sec_irq local variables in the probe
>     function
> 
> v2:
>   - make sec_irq optional to avoid breaking the Device Tree ABI
>   - removed the Device Tree bindings, I'll prepare a separate patch for them
> 
> 
>  drivers/rtc/rtc-imxdi.c |   28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)
> 
Applied after fixing the multiline comment, thanks.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 3/3] Documentation: dt: rtc-imxdi: security violation interrupt

[Alexandre Belloni]

On 03/01/2017 at 19:51:25 +0100, Martin Kaiser wrote :
> Document the DryIce security violation interrupt.
> 
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
>  Documentation/devicetree/bindings/rtc/imxdi-rtc.txt |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
Applied, thanks.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com