[PATCH] cadence_qspi: Refactor the flash reset functionality

[Michal Simek]

On 11/7/24 05:57, Abbarapu, Venkatesh wrote:
> Hi Michal,
> 
>> -----Original Message-----
>> From: Simek, Michal <michal.simek at amd.com>
>> Sent: Wednesday, November 6, 2024 5:17 PM
>> To: Abbarapu, Venkatesh <venkatesh.abbarapu at amd.com>; u-boot at lists.denx.de
>> Cc: jagan at amarulasolutions.com; vigneshr at ti.com; u-kumar1 at ti.com;
>> trini at konsulko.com; seanga2 at gmail.com; caleb.connolly at linaro.org;
>> sjg at chromium.org; william.zhang at broadcom.com; stefan_b at posteo.net; git (AMD-
>> Xilinx) <git at amd.com>
>> Subject: Re: [PATCH] cadence_qspi: Refactor the flash reset functionality
>>
>>
>>
>> On 10/28/24 10:24, Venkatesh Yadav Abbarapu wrote:
>>> As the flash reset is handled in spi nor core, removing the flash
>>> reset functionality. As the configuration like tristate and hysterisis
>>> need to be enabled by the cdo. Handle the flash reset only for mini
>>> u-boot case.
>>>
>>> Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu at amd.com>
>>> ---
>>>    drivers/spi/cadence_ospi_versal.c | 43 -------------------------------
>>>    drivers/spi/cadence_qspi.c        |  7 +++--
>>>    2 files changed, 5 insertions(+), 45 deletions(-)
>>>
>>> diff --git a/drivers/spi/cadence_ospi_versal.c
>>> b/drivers/spi/cadence_ospi_versal.c
>>> index 222f828f54..de51e4c37f 100644
>>> --- a/drivers/spi/cadence_ospi_versal.c
>>> +++ b/drivers/spi/cadence_ospi_versal.c
>>> @@ -125,48 +125,6 @@ int cadence_qspi_apb_wait_for_dma_cmplt(struct
>> cadence_spi_priv *priv)
>>>    	return 0;
>>>    }
>>>
>>> -#if defined(CONFIG_DM_GPIO)
>>> -int cadence_qspi_versal_flash_reset(struct udevice *dev) -{
>>> -	struct gpio_desc gpio;
>>> -	u32 reset_gpio;
>>> -	int ret;
>>> -
>>> -	/* request gpio and set direction as output set to 1 */
>>> -	ret = gpio_request_by_name(dev, "reset-gpios", 0, &gpio,
>>> -				   GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE);
>>> -	if (ret) {
>>> -		printf("%s: unable to reset ospi flash device", __func__);
>>> -		return ret;
>>> -	}
>>> -
>>> -	reset_gpio = PMIO_NODE_ID_BASE + gpio.offset;
>>> -
>>> -	/* Request for pin */
>>> -	xilinx_pm_request(PM_PINCTRL_REQUEST, reset_gpio, 0, 0, 0, NULL);
>>> -
>>> -	/* Enable hysteresis in cmos receiver */
>>> -	xilinx_pm_request(PM_PINCTRL_CONFIG_PARAM_SET, reset_gpio,
>>> -			  PM_PINCTRL_CONFIG_SCHMITT_CMOS,
>>> -			  PM_PINCTRL_INPUT_TYPE_SCHMITT, 0, NULL);
>>> -
>>> -	/* Disable Tri-state */
>>> -	xilinx_pm_request(PM_PINCTRL_CONFIG_PARAM_SET, reset_gpio,
>>> -			  PM_PINCTRL_CONFIG_TRI_STATE,
>>> -			  PM_PINCTRL_TRI_STATE_DISABLE, 0, NULL);
>>> -	udelay(1);
>>> -
>>> -	/* Set value 0 to pin */
>>> -	dm_gpio_set_value(&gpio, 0);
>>> -	udelay(1);
>>> -
>>> -	/* Set value 1 to pin */
>>> -	dm_gpio_set_value(&gpio, 1);
>>> -	udelay(1);
>>> -
>>> -	return 0;
>>> -}
>>> -#else
>>>    int cadence_qspi_versal_flash_reset(struct udevice *dev)
>>>    {
>>>    	/* CRP WPROT */
>>> @@ -209,7 +167,6 @@ int cadence_qspi_versal_flash_reset(struct udevice
>>> *dev)
>>>
>>>    	return 0;
>>>    }
>>> -#endif
>>>
>>>    void cadence_qspi_apb_enable_linear_mode(bool enable)
>>>    {
>>> diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
>>> index 9c466f8695..f8c3fe8428 100644
>>> --- a/drivers/spi/cadence_qspi.c
>>> +++ b/drivers/spi/cadence_qspi.c
>>> @@ -258,8 +258,11 @@ static int cadence_spi_probe(struct udevice *bus)
>>>    		if (priv->read_delay >= 0)
>>>    			priv->read_delay = -1;
>>>
>>> -	/* Reset ospi flash device */
>>> -	return cadence_qspi_versal_flash_reset(bus);
>>> +	if (!CONFIG_IS_ENABLED(DM_GPIO))
>>
>> I think this is not the right thing to do here.
>> Because there could be boards which requires to do some other GPIO handling out
>> of spi core. I think this patch is introducing limitation for nothing.
>>
>> I would keep origin code and just move this condition to
>> drivers/spi/cadence_ospi_versal.c
> 
> This function cadence_qspi_versal_flash_reset() is specific to AMD only.

yes I know and we shouldn't really bother generic code about it.
I think that function name should be more generic that other platforms can hook 
it up. But logic when versal requires this only when DM_GPIO is not enabled 
should be in vendor specific file.

M