On 11/7/24 05:57, Abbarapu, Venkatesh wrote:

Hi Michal,

...

-----Original Message----- From: Simek, Michal michal.simek@amd.com Sent: Wednesday, November 6, 2024 5:17 PM To: Abbarapu, Venkatesh venkatesh.abbarapu@amd.com; u-boot@lists.denx.de Cc: jagan@amarulasolutions.com; vigneshr@ti.com; u-kumar1@ti.com; trini@konsulko.com; seanga2@gmail.com; caleb.connolly@linaro.org; sjg@chromium.org; william.zhang@broadcom.com; stefan_b@posteo.net; git (AMD- Xilinx) git@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@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