Re: [PATCH 1/2] reset: imx7: Add support for i.MX8MM SoC

[Philipp Zabel]

Hi Anson,

On Mon, 2019-07-01 at 17:39 +0800, Anson.Huang@xxxxxxx wrote:
> From: Anson Huang <Anson.Huang@xxxxxxx>
> 
> i.MX8MM SoC has a subset of i.MX8MQ IP block variant, it can reuse
> the i.MX8MQ reset controller driver and just skip those non-existing
> IP blocks, add support for i.MX8MM SoC reset control.
> 
> Signed-off-by: Anson Huang <Anson.Huang@xxxxxxx>
> ---
>  drivers/reset/reset-imx7.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
> index 3ecd770..941131f 100644
> --- a/drivers/reset/reset-imx7.c
> +++ b/drivers/reset/reset-imx7.c
> @@ -207,6 +207,26 @@ static int imx8mq_reset_set(struct reset_controller_dev *rcdev,
>  	const unsigned int bit = imx7src->signals[id].bit;
>  	unsigned int value = assert ? bit : 0;
>  
> +	if (of_machine_is_compatible("fsl,imx8mm")) {

This should be checked once during probe, not in every reset_set, if
this check has to be made at all. On i.MX8MM these unused reset controls
are not going to be hooked up via phandle, so this function should never
be called with the values that are filtered out here anyway.
And in case somebody makes an error in the device tree, does writing 1
to the reserved bits on i.MX8MM have any negative side effects at all?
Or are these bits just not hooked up? If this is no problem, I assume
this patch is not needed at all.

The correct way to protect against DT writers hooking up the non-
existing reset lines would be to replace rcdev.of_xlate with a version
that returns -EINVAL for them on i.MX8MM. Also in that case I'd check
the reset-controller compatible, not the machine compatible.

> +		switch (id) {
> +		case IMX8MQ_RESET_HDMI_PHY_APB_RESET:
> +		case IMX8MQ_RESET_PCIEPHY2: /* fallthrough */
> +		case IMX8MQ_RESET_PCIEPHY2_PERST: /* fallthrough */
> +		case IMX8MQ_RESET_PCIE2_CTRL_APPS_EN: /* fallthrough */
> +		case IMX8MQ_RESET_PCIE2_CTRL_APPS_TURNOFF: /* fallthrough */
> +		case IMX8MQ_RESET_MIPI_CSI1_CORE_RESET: /* fallthrough */
> +		case IMX8MQ_RESET_MIPI_CSI1_PHY_REF_RESET: /* fallthrough */
> +		case IMX8MQ_RESET_MIPI_CSI1_ESC_RESET: /* fallthrough */
> +		case IMX8MQ_RESET_MIPI_CSI2_CORE_RESET: /* fallthrough */
> +		case IMX8MQ_RESET_MIPI_CSI2_PHY_REF_RESET: /* fallthrough */
> +		case IMX8MQ_RESET_MIPI_CSI2_ESC_RESET: /* fallthrough */
> +		case IMX8MQ_RESET_DDRC2_PHY_RESET: /* fallthrough */
> +		case IMX8MQ_RESET_DDRC2_CORE_RESET: /* fallthrough */
> +		case IMX8MQ_RESET_DDRC2_PRST: /* fallthrough */

I think it would make sense to add IMX8MM_RESET_* defines for all but
these, to avoid confusion when reading the imx8mm.dtsi

regards
Philipp