Re: [PATCH 1/3] media: rcar-fcp: Add rcar_fcp_soft_reset()

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Jacopo,

Thank you for the patch.

On Mon, Jun 09, 2025 at 09:01:42PM +0200, Jacopo Mondi wrote:
> Add a function to perform soft reset of the FCP.
> 
> It is intended to support the correct stop procedure of the VSPX-FCPVX
> and VSPD-FCPD pairs according to section "62.3.7.3 Reset Operation" of
> the R-Car Hardware Manual at revision 1.20.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/renesas/rcar-fcp.c | 41 +++++++++++++++++++++++++++++++
>  include/media/rcar-fcp.h                  |  5 ++++
>  2 files changed, 46 insertions(+)
> 
> diff --git a/drivers/media/platform/renesas/rcar-fcp.c b/drivers/media/platform/renesas/rcar-fcp.c
> index cee9bbce4e3affb2467dbc28142e1ab2304bf5b0..fefeaf77f0122a2d88ac03a5c42a892dc284a163 100644
> --- a/drivers/media/platform/renesas/rcar-fcp.c
> +++ b/drivers/media/platform/renesas/rcar-fcp.c
> @@ -10,6 +10,8 @@
>  #include <linux/device.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/list.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>

i goes before l.

>  #include <linux/module.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/mutex.h>
> @@ -19,14 +21,30 @@
>  
>  #include <media/rcar-fcp.h>
>  
> +#define RCAR_FCP_REG_RST		0x0010
> +#define RCAR_FCP_REG_RST_SOFTRST	BIT(0)
> +#define RCAR_FCP_REG_STA		0x0018
> +#define RCAR_FCP_REG_STA_ACT		BIT(0)
> +
>  struct rcar_fcp_device {
>  	struct list_head list;
>  	struct device *dev;
> +	void __iomem *base;
>  };
>  
>  static LIST_HEAD(fcp_devices);
>  static DEFINE_MUTEX(fcp_lock);
>  
> +static inline u32 rcar_fcp_read(struct rcar_fcp_device *fcp, u32 reg)
> +{
> +	return ioread32(fcp->base + reg);
> +}
> +
> +static inline void rcar_fcp_write(struct rcar_fcp_device *fcp, u32 reg, u32 val)
> +{
> +	iowrite32(val, fcp->base + reg);
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * Public API
>   */
> @@ -117,6 +135,25 @@ void rcar_fcp_disable(struct rcar_fcp_device *fcp)
>  }
>  EXPORT_SYMBOL_GPL(rcar_fcp_disable);
>  
> +int rcar_fcp_soft_reset(struct rcar_fcp_device *fcp)
> +{
> +	u32 value;
> +	int ret;
> +
> +	if (!fcp)
> +		return 0;
> +
> +	rcar_fcp_write(fcp, RCAR_FCP_REG_RST, RCAR_FCP_REG_RST_SOFTRST);
> +	ret = readl_poll_timeout(fcp->base + RCAR_FCP_REG_STA,
> +				 value, !(value & RCAR_FCP_REG_STA_ACT),
> +				 0, 100);

That will be a busy loop for up to 100µs. Shouldn't the sleep_us
argument be at least 1 ?

You could also use

	ret = read_poll_timeout(rcar_fcp_read, value,
				!(value & FCAR_FCP_REG_STA_ACT), 1, 100, false,
				fcp, RCAR_FCP_REG_STA);

Up to you.

> +	if (ret)
> +		dev_err(fcp->dev, "Failed to soft-reset: %d\n", ret);

ret can only be 0 or -ETIMEDOUT, so I wouldn't print its value in the
message.

With these issues addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(rcar_fcp_soft_reset);
> +
>  /* -----------------------------------------------------------------------------
>   * Platform Driver
>   */
> @@ -131,6 +168,10 @@ static int rcar_fcp_probe(struct platform_device *pdev)
>  
>  	fcp->dev = &pdev->dev;
>  
> +	fcp->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(fcp->base))
> +		return PTR_ERR(fcp->base);
> +
>  	dma_set_max_seg_size(fcp->dev, UINT_MAX);
>  
>  	pm_runtime_enable(&pdev->dev);
> diff --git a/include/media/rcar-fcp.h b/include/media/rcar-fcp.h
> index 179240fb163bd2e7cc347e559f99bae943bf0e34..6ac9be9f675e667d6482a5a2483963fa52a0c622 100644
> --- a/include/media/rcar-fcp.h
> +++ b/include/media/rcar-fcp.h
> @@ -18,6 +18,7 @@ void rcar_fcp_put(struct rcar_fcp_device *fcp);
>  struct device *rcar_fcp_get_device(struct rcar_fcp_device *fcp);
>  int rcar_fcp_enable(struct rcar_fcp_device *fcp);
>  void rcar_fcp_disable(struct rcar_fcp_device *fcp);
> +int rcar_fcp_soft_reset(struct rcar_fcp_device *fcp);
>  #else
>  static inline struct rcar_fcp_device *rcar_fcp_get(const struct device_node *np)
>  {
> @@ -33,6 +34,10 @@ static inline int rcar_fcp_enable(struct rcar_fcp_device *fcp)
>  	return 0;
>  }
>  static inline void rcar_fcp_disable(struct rcar_fcp_device *fcp) { }
> +static inline int rcar_fcp_soft_reset(struct rcar_fcp_device *fcp)
> +{
> +	return 0;
> +}
>  #endif
>  
>  #endif /* __MEDIA_RCAR_FCP_H__ */

-- 
Regards,

Laurent Pinchart