Re: [PATCH 2/2] mfd: madera: Wait for BOOT_DONE before reading device ID

[Charles Keepax <ckeepax@opensource.cirrus.com>]

On Mon, Oct 01, 2018 at 03:33:57PM +0100, Richard Fitzgerald wrote:
> It isn't safe to read the device ID in SOFTWARE_RESET register
> until the silicon boot sequence has completed. This patch
> rearranges the code to wait for BOOT_DONE first.
> 
> If we don't have a GPIO to control hard reset we must issue
> a soft reset to get the silicon into a known-good booted state
> before reading the device ID.
> 
> Signed-off-by: Stuart Henderson <stuarth@opensource.cirrus.com>
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---
>  drivers/mfd/madera-core.c | 33 +++++++++++++++------------------
>  1 file changed, 15 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/mfd/madera-core.c b/drivers/mfd/madera-core.c
> index 3ff9a0615658..b9d42edf96c9 100644
> --- a/drivers/mfd/madera-core.c
> +++ b/drivers/mfd/madera-core.c
> @@ -454,10 +454,23 @@ int madera_dev_init(struct madera *madera)
>  	regcache_cache_only(madera->regmap, false);
>  	regcache_cache_only(madera->regmap_32bit, false);
>  
> +	/* If we don't have a reset GPIO use a soft reset */
> +	if (!madera->pdata.reset) {
> +		ret = madera_soft_reset(madera);
> +		if (ret)
> +			goto err_reset;
> +	}
> +

I think we should leave this soft reset lower down its better
to only write to the chip once we have identified it. It will
mean we need to wait for boot done twice (in the soft reset
case) but its only on probe and that is probably better than
doing some writes to an unknown chip.

>  	/*
> -	 * Now we can power up and verify that this is a chip we know about
> -	 * before we start doing any writes to its registers.
> +	 * Must wait for internal boot sequence to complete before
> +	 * reading the device ID
>  	 */
> +	ret = madera_wait_for_boot(madera);
> +	if (ret) {
> +		dev_err(madera->dev, "Device failed initial boot: %d\n", ret);
> +		goto err_reset;
> +	}
> +
>  	ret = regmap_read(madera->regmap, MADERA_SOFTWARE_RESET, &hwid);
>  	if (ret) {
>  		dev_err(dev, "Failed to read ID register: %d\n", ret);
> @@ -519,22 +532,6 @@ int madera_dev_init(struct madera *madera)
>  		goto err_reset;
>  	}
>  
> -	/*
> -	 * It looks like a device we support. If we don't have a hard reset
> -	 * we can now attempt a soft reset.
> -	 */
> -	if (!madera->pdata.reset) {
> -		ret = madera_soft_reset(madera);
> -		if (ret)
> -			goto err_reset;
> -	}
> -
> -	ret = madera_wait_for_boot(madera);
> -	if (ret) {
> -		dev_err(madera->dev, "Device failed initial boot: %d\n", ret);
> -		goto err_reset;
> -	}

And this guy would then only need done in the case of a soft
reset.

Thanks,
Charles

> -
>  	ret = regmap_read(madera->regmap, MADERA_HARDWARE_REVISION,
>  			  &madera->rev);
>  	if (ret) {
> -- 
> 2.11.0