[PATCH 0/2] can: xilinx_can: Add support for reset

[PATCH 0/2] can: xilinx_can: Add support for reset

[Michal Simek]

Hi,

IP core has option reset line which can be wired that's why add support
for optional reset.

Thanks,
Michal


Michal Simek (1):
  dt-bindings: can: xilinx_can: Add reset description

Srinivas Neeli (1):
  can: xilinx_can: Add support for controller reset

 .../bindings/net/can/xilinx,can.yaml          |  3 +++
 drivers/net/can/xilinx_can.c                  | 25 ++++++++++++++++---
 2 files changed, 24 insertions(+), 4 deletions(-)

-- 
2.36.1

[PATCH 1/2] dt-bindings: can: xilinx_can: Add reset description

[Michal Simek]

IP core has input for reset signal which can be connected that's why
describe optional reset property.

Signed-off-by: Michal Simek <michal.simek@amd.com>
---

 Documentation/devicetree/bindings/net/can/xilinx,can.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/can/xilinx,can.yaml b/Documentation/devicetree/bindings/net/can/xilinx,can.yaml
index 897d2cbda45b..64d57c343e6f 100644
--- a/Documentation/devicetree/bindings/net/can/xilinx,can.yaml
+++ b/Documentation/devicetree/bindings/net/can/xilinx,can.yaml
@@ -46,6 +46,9 @@ properties:
     $ref: /schemas/types.yaml#/definitions/uint32
     description: CAN Tx mailbox buffer count (CAN FD)
 
+  resets:
+    maxItems: 1
+
 required:
   - compatible
   - reg
-- 
2.36.1

[PATCH 2/2] can: xilinx_can: Add support for controller reset

[Michal Simek]

From: Srinivas Neeli <srinivas.neeli@amd.com>

Add support for an optional reset for the CAN controller using the reset
driver. If the CAN node contains the "resets" property, then this logic
will perform CAN controller reset.

Signed-off-by: Srinivas Neeli <srinivas.neeli@amd.com>
Signed-off-by: Michal Simek <michal.simek@amd.com>
---

 drivers/net/can/xilinx_can.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 4d3283db3a13..929e00061898 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -30,6 +30,7 @@
 #include <linux/can/error.h>
 #include <linux/phy/phy.h>
 #include <linux/pm_runtime.h>
+#include <linux/reset.h>
 
 #define DRIVER_NAME	"xilinx_can"
 
@@ -200,6 +201,7 @@ struct xcan_devtype_data {
  * @can_clk:			Pointer to struct clk
  * @devtype:			Device type specific constants
  * @transceiver:		Optional pointer to associated CAN transceiver
+ * @rstc:			Pointer to reset control
  */
 struct xcan_priv {
 	struct can_priv can;
@@ -218,6 +220,7 @@ struct xcan_priv {
 	struct clk *can_clk;
 	struct xcan_devtype_data devtype;
 	struct phy *transceiver;
+	struct reset_control *rstc;
 };
 
 /* CAN Bittiming constants as per Xilinx CAN specs */
@@ -1799,6 +1802,16 @@ static int xcan_probe(struct platform_device *pdev)
 	priv->can.do_get_berr_counter = xcan_get_berr_counter;
 	priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
 					CAN_CTRLMODE_BERR_REPORTING;
+	priv->rstc = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);
+	if (IS_ERR(priv->rstc)) {
+		dev_err(&pdev->dev, "Cannot get CAN reset.\n");
+		ret = PTR_ERR(priv->rstc);
+		goto err_free;
+	}
+
+	ret = reset_control_reset(priv->rstc);
+	if (ret)
+		goto err_free;
 
 	if (devtype->cantype == XAXI_CANFD) {
 		priv->can.data_bittiming_const =
@@ -1827,7 +1840,7 @@ static int xcan_probe(struct platform_device *pdev)
 	/* Get IRQ for the device */
 	ret = platform_get_irq(pdev, 0);
 	if (ret < 0)
-		goto err_free;
+		goto err_reset;
 
 	ndev->irq = ret;
 
@@ -1843,21 +1856,21 @@ static int xcan_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->can_clk)) {
 		ret = dev_err_probe(&pdev->dev, PTR_ERR(priv->can_clk),
 				    "device clock not found\n");
-		goto err_free;
+		goto err_reset;
 	}
 
 	priv->bus_clk = devm_clk_get(&pdev->dev, devtype->bus_clk_name);
 	if (IS_ERR(priv->bus_clk)) {
 		ret = dev_err_probe(&pdev->dev, PTR_ERR(priv->bus_clk),
 				    "bus clock not found\n");
-		goto err_free;
+		goto err_reset;
 	}
 
 	transceiver = devm_phy_optional_get(&pdev->dev, NULL);
 	if (IS_ERR(transceiver)) {
 		ret = PTR_ERR(transceiver);
 		dev_err_probe(&pdev->dev, ret, "failed to get phy\n");
-		goto err_free;
+		goto err_reset;
 	}
 	priv->transceiver = transceiver;
 
@@ -1904,6 +1917,8 @@ static int xcan_probe(struct platform_device *pdev)
 err_disableclks:
 	pm_runtime_put(priv->dev);
 	pm_runtime_disable(&pdev->dev);
+err_reset:
+	reset_control_assert(priv->rstc);
 err_free:
 	free_candev(ndev);
 err:
@@ -1920,10 +1935,12 @@ static int xcan_probe(struct platform_device *pdev)
 static void xcan_remove(struct platform_device *pdev)
 {
 	struct net_device *ndev = platform_get_drvdata(pdev);
+	struct xcan_priv *priv = netdev_priv(ndev);
 
 	unregister_candev(ndev);
 	pm_runtime_disable(&pdev->dev);
 	free_candev(ndev);
+	reset_control_assert(priv->rstc);
 }
 
 static struct platform_driver xcan_driver = {
-- 
2.36.1

Re: [PATCH 1/2] dt-bindings: can: xilinx_can: Add reset description

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 993 bytes --]

On Tue, Jul 11, 2023 at 04:03:54PM +0200, Michal Simek wrote:
> IP core has input for reset signal which can be connected that's why
> describe optional reset property.
> 
> Signed-off-by: Michal Simek <michal.simek@amd.com>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

> ---
> 
>  Documentation/devicetree/bindings/net/can/xilinx,can.yaml | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/can/xilinx,can.yaml b/Documentation/devicetree/bindings/net/can/xilinx,can.yaml
> index 897d2cbda45b..64d57c343e6f 100644
> --- a/Documentation/devicetree/bindings/net/can/xilinx,can.yaml
> +++ b/Documentation/devicetree/bindings/net/can/xilinx,can.yaml
> @@ -46,6 +46,9 @@ properties:
>      $ref: /schemas/types.yaml#/definitions/uint32
>      description: CAN Tx mailbox buffer count (CAN FD)
>  
> +  resets:
> +    maxItems: 1
> +
>  required:
>    - compatible
>    - reg
> -- 
> 2.36.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/2] can: xilinx_can: Add support for controller reset

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 4359 bytes --]

On 11.07.2023 16:03:55, Michal Simek wrote:
> From: Srinivas Neeli <srinivas.neeli@amd.com>
> 
> Add support for an optional reset for the CAN controller using the reset
> driver. If the CAN node contains the "resets" property, then this logic
> will perform CAN controller reset.
> 
> Signed-off-by: Srinivas Neeli <srinivas.neeli@amd.com>
> Signed-off-by: Michal Simek <michal.simek@amd.com>
> ---
> 
>  drivers/net/can/xilinx_can.c | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
> index 4d3283db3a13..929e00061898 100644
> --- a/drivers/net/can/xilinx_can.c
> +++ b/drivers/net/can/xilinx_can.c
> @@ -30,6 +30,7 @@
>  #include <linux/can/error.h>
>  #include <linux/phy/phy.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/reset.h>
>  
>  #define DRIVER_NAME	"xilinx_can"
>  
> @@ -200,6 +201,7 @@ struct xcan_devtype_data {
>   * @can_clk:			Pointer to struct clk
>   * @devtype:			Device type specific constants
>   * @transceiver:		Optional pointer to associated CAN transceiver
> + * @rstc:			Pointer to reset control
>   */
>  struct xcan_priv {
>  	struct can_priv can;
> @@ -218,6 +220,7 @@ struct xcan_priv {
>  	struct clk *can_clk;
>  	struct xcan_devtype_data devtype;
>  	struct phy *transceiver;
> +	struct reset_control *rstc;
>  };
>  
>  /* CAN Bittiming constants as per Xilinx CAN specs */
> @@ -1799,6 +1802,16 @@ static int xcan_probe(struct platform_device *pdev)
>  	priv->can.do_get_berr_counter = xcan_get_berr_counter;
>  	priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
>  					CAN_CTRLMODE_BERR_REPORTING;
> +	priv->rstc = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);
> +	if (IS_ERR(priv->rstc)) {
> +		dev_err(&pdev->dev, "Cannot get CAN reset.\n");
> +		ret = PTR_ERR(priv->rstc);
> +		goto err_free;
> +	}
> +
> +	ret = reset_control_reset(priv->rstc);
> +	if (ret)
> +		goto err_free;
>  
>  	if (devtype->cantype == XAXI_CANFD) {
>  		priv->can.data_bittiming_const =
> @@ -1827,7 +1840,7 @@ static int xcan_probe(struct platform_device *pdev)
>  	/* Get IRQ for the device */
>  	ret = platform_get_irq(pdev, 0);
>  	if (ret < 0)
> -		goto err_free;
> +		goto err_reset;
>  
>  	ndev->irq = ret;
>  
> @@ -1843,21 +1856,21 @@ static int xcan_probe(struct platform_device *pdev)
>  	if (IS_ERR(priv->can_clk)) {
>  		ret = dev_err_probe(&pdev->dev, PTR_ERR(priv->can_clk),
>  				    "device clock not found\n");
> -		goto err_free;
> +		goto err_reset;
>  	}
>  
>  	priv->bus_clk = devm_clk_get(&pdev->dev, devtype->bus_clk_name);
>  	if (IS_ERR(priv->bus_clk)) {
>  		ret = dev_err_probe(&pdev->dev, PTR_ERR(priv->bus_clk),
>  				    "bus clock not found\n");
> -		goto err_free;
> +		goto err_reset;
>  	}
>  
>  	transceiver = devm_phy_optional_get(&pdev->dev, NULL);
>  	if (IS_ERR(transceiver)) {
>  		ret = PTR_ERR(transceiver);
>  		dev_err_probe(&pdev->dev, ret, "failed to get phy\n");
> -		goto err_free;
> +		goto err_reset;
>  	}
>  	priv->transceiver = transceiver;
>  
> @@ -1904,6 +1917,8 @@ static int xcan_probe(struct platform_device *pdev)
>  err_disableclks:
>  	pm_runtime_put(priv->dev);
>  	pm_runtime_disable(&pdev->dev);
> +err_reset:
> +	reset_control_assert(priv->rstc);
>  err_free:
>  	free_candev(ndev);
>  err:
> @@ -1920,10 +1935,12 @@ static int xcan_probe(struct platform_device *pdev)
>  static void xcan_remove(struct platform_device *pdev)
>  {
>  	struct net_device *ndev = platform_get_drvdata(pdev);
> +	struct xcan_priv *priv = netdev_priv(ndev);
>  
>  	unregister_candev(ndev);
>  	pm_runtime_disable(&pdev->dev);
>  	free_candev(ndev);
> +	reset_control_assert(priv->rstc);

Nitpick: Can you make this symmetric with respect to the error handling
in xcan_probe()?

Oh - that's not a cosmetic issue, it's a use-after-free. free_candev()
frees your priv.

>  }
>  
>  static struct platform_driver xcan_driver = {
> -- 
> 2.36.1
> 
>

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]