RE: [PATCH V2 1/3] scsi: ufs: Allow vendor specific initialization

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Sujit,

On Tue, August 27, 2013, Sujit Reddy Thumma wrote:
> Some vendor specific controller versions might need to configure
> vendor specific - registers, clocks, voltage regulators etc. to
> initialize the host controller UTP layer and Uni-Pro stack.
> Provide some common initialization operations that can be used
> to configure vendor specifics. The methods can be extended in
> future, for example, for power mode transitions.
> 
> The operations are vendor/board specific and hence determined with
> the help of compatible property in device tree.
> 
> Signed-off-by: Sujit Reddy Thumma <sthumma@xxxxxxxxxxxxxx>
> ---
>  drivers/scsi/ufs/ufshcd-pci.c    |   8 +-
>  drivers/scsi/ufs/ufshcd-pltfrm.c |  24 +++++-
>  drivers/scsi/ufs/ufshcd.c        | 157 +++++++++++++++++++++++++++++++--------
>  drivers/scsi/ufs/ufshcd.h        |  34 ++++++++-
>  4 files changed, 187 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd-pci.c b/drivers/scsi/ufs/ufshcd-pci.c
> index a823cf4..6b0d299 100644
> --- a/drivers/scsi/ufs/ufshcd-pci.c
> +++ b/drivers/scsi/ufs/ufshcd-pci.c
> @@ -191,7 +191,13 @@ ufshcd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  		return err;
>  	}
> 
> -	err = ufshcd_init(&pdev->dev, &hba, mmio_base, pdev->irq);
> +	err = ufshcd_alloc_host(&pdev->dev, &hba);
> +	if (err) {
> +		dev_err(&pdev->dev, "Allocation failed\n");
> +		return err;
> +	}
> +
> +	err = ufshcd_init(hba, mmio_base, pdev->irq);
>  	if (err) {
>  		dev_err(&pdev->dev, "Initialization failed\n");
>  		return err;
> diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
> index 5e46232..9c94052 100644
> --- a/drivers/scsi/ufs/ufshcd-pltfrm.c
> +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
> @@ -35,9 +35,23 @@
> 
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/of.h>
> 
>  #include "ufshcd.h"
> 
> +static const struct of_device_id ufs_of_match[];
> +static struct ufs_hba_variant_ops *get_variant_ops(struct device *dev)
> +{
> +	if (dev->of_node) {
> +		const struct of_device_id *match;
> +		match = of_match_node(ufs_of_match, dev->of_node);
> +		if (match)
> +			return (struct ufs_hba_variant_ops *)match->data;
> +	}
> +
> +	return NULL;
> +}
> +
>  #ifdef CONFIG_PM
>  /**
>   * ufshcd_pltfrm_suspend - suspend power management function
> @@ -150,10 +164,18 @@ static int ufshcd_pltfrm_probe(struct platform_device *pdev)
>  		goto out;
>  	}
> 
> +	err = ufshcd_alloc_host(dev, &hba);
> +	if (err) {
> +		dev_err(&pdev->dev, "Allocation failed\n");
> +		goto out;
> +	}
> +
> +	hba->vops = get_variant_ops(&pdev->dev);
> +
>  	pm_runtime_set_active(&pdev->dev);
>  	pm_runtime_enable(&pdev->dev);
> 
> -	err = ufshcd_init(dev, &hba, mmio_base, irq);
> +	err = ufshcd_init(hba, mmio_base, irq);
>  	if (err) {
>  		dev_err(dev, "Intialization failed\n");
>  		goto out_disable_rpm;
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index a0f5ac2..743696a 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -174,13 +174,14 @@ static inline u32 ufshcd_get_ufs_version(struct ufs_hba *hba)
>  /**
>   * ufshcd_is_device_present - Check if any device connected to
>   *			      the host controller
> - * @reg_hcs - host controller status register value
> + * @hba: pointer to adapter instance
>   *
>   * Returns 1 if device present, 0 if no device detected
>   */
> -static inline int ufshcd_is_device_present(u32 reg_hcs)
> +static inline int ufshcd_is_device_present(struct ufs_hba *hba)
>  {
> -	return (DEVICE_PRESENT & reg_hcs) ? 1 : 0;
> +	return (ufshcd_readl(hba, REG_CONTROLLER_STATUS) &
> +						DEVICE_PRESENT) ? 1 : 0;
>  }
> 
>  /**
> @@ -1483,11 +1484,10 @@ out:
>   * @hba: per adapter instance
>   *
>   * To bring UFS host controller to operational state,
> - * 1. Check if device is present
> - * 2. Enable required interrupts
> - * 3. Configure interrupt aggregation
> - * 4. Program UTRL and UTMRL base addres
> - * 5. Configure run-stop-registers
> + * 1. Enable required interrupts
> + * 2. Configure interrupt aggregation
> + * 3. Program UTRL and UTMRL base addres
> + * 4. Configure run-stop-registers
>   *
>   * Returns 0 on success, non-zero value on failure
>   */
> @@ -1496,14 +1496,6 @@ static int ufshcd_make_hba_operational(struct ufs_hba *hba)
>  	int err = 0;
>  	u32 reg;
> 
> -	/* check if device present */
> -	reg = ufshcd_readl(hba, REG_CONTROLLER_STATUS);
> -	if (!ufshcd_is_device_present(reg)) {
> -		dev_err(hba->dev, "cc: Device not present\n");
> -		err = -ENXIO;
> -		goto out;
> -	}
> -
>  	/* Enable required interrupts */
>  	ufshcd_enable_intr(hba, UFSHCD_ENABLE_INTRS);
> 
> @@ -1524,6 +1516,7 @@ static int ufshcd_make_hba_operational(struct ufs_hba *hba)
>  	 * UCRDY, UTMRLDY and UTRLRDY bits must be 1
>  	 * DEI, HEI bits must be 0
>  	 */
> +	reg = ufshcd_readl(hba, REG_CONTROLLER_STATUS);
>  	if (!(ufshcd_get_lists_status(reg))) {
>  		ufshcd_enable_run_stop_reg(hba);
>  	} else {
> @@ -1570,6 +1563,9 @@ static int ufshcd_hba_enable(struct ufs_hba *hba)
>  		msleep(5);
>  	}
> 
> +	if (hba->vops && hba->vops->hce_enable_notify)
> +		hba->vops->hce_enable_notify(hba, PRE_CHANGE);
> +
>  	/* start controller initialization sequence */
>  	ufshcd_hba_start(hba);
> 
> @@ -1597,6 +1593,10 @@ static int ufshcd_hba_enable(struct ufs_hba *hba)
>  		}
>  		msleep(5);
>  	}
> +
> +	if (hba->vops && hba->vops->hce_enable_notify)
> +		hba->vops->hce_enable_notify(hba, POST_CHANGE);
> +
>  	return 0;
>  }
> 
> @@ -1613,12 +1613,28 @@ static int ufshcd_link_startup(struct ufs_hba *hba)
>  	/* enable UIC related interrupts */
>  	ufshcd_enable_intr(hba, UIC_COMMAND_COMPL);
> 
> +	if (hba->vops && hba->vops->link_startup_notify)
> +		hba->vops->link_startup_notify(hba, PRE_CHANGE);
> +
>  	ret = ufshcd_dme_link_startup(hba);
>  	if (ret)
>  		goto out;
> 
> -	ret = ufshcd_make_hba_operational(hba);
> +	/* check if device is detected by inter-connect layer */
> +	if (!ufshcd_is_device_present(hba)) {
> +		dev_err(hba->dev, "%s: Device not present\n", __func__);
> +		ret = -ENXIO;
> +		goto out;
> +	}
> 
> +	/* Include any host controller configuration via UIC commands */
> +	if (hba->vops && hba->vops->link_startup_notify) {
> +		ret = hba->vops->link_startup_notify(hba, POST_CHANGE);
> +		if (ret)
> +			goto out;
> +	}
> +
> +	ret = ufshcd_make_hba_operational(hba);
>  out:
>  	if (ret)
>  		dev_err(hba->dev, "link startup failed %d\n", ret);
> @@ -2810,6 +2826,61 @@ static struct scsi_host_template ufshcd_driver_template = {
>  	.can_queue		= UFSHCD_CAN_QUEUE,
>  };
> 
> +static int ufshcd_variant_hba_init(struct ufs_hba *hba)
> +{
> +	int err = 0;
> +
> +	if (!hba->vops)
> +		goto out;
> +
> +	if (hba->vops->init) {
> +		err = hba->vops->init(hba);
If init() is allowed to access host specific registers,
setup_clocks() and setup_regulators() may precede init() generally.
As seeing your description, it seems certain.
<Quot>
@init: called when the driver is initialized
@setup_clocks: called before touching any of the controller registers
@setup_regulators: called before accessing the host controller
</Quot>

Further, possibly init() implementation might substitute all?

> +		if (err)
> +			goto out;
> +	}
> +
> +	if (hba->vops->setup_clocks) {
> +		err = hba->vops->setup_clocks(hba, true);
> +		if (err)
> +			goto out_exit;
> +	}
> +
> +	if (hba->vops->setup_regulators) {
> +		err = hba->vops->setup_regulators(hba, true);
> +		if (err)
> +			goto out_clks;
> +	}
> +
> +	goto out;
> +
> +out_clks:
> +	if (hba->vops->setup_clocks)
> +		hba->vops->setup_clocks(hba, false);
> +out_exit:
> +	if (hba->vops->exit)
> +		hba->vops->exit(hba);
> +out:
> +	if (err)
> +		dev_err(hba->dev, "%s: variant %s init failed err %d\n",
> +			__func__, hba->vops ? hba->vops->name : "", err);
> +	return err;
> +}
> +
> +static void ufshcd_variant_hba_exit(struct ufs_hba *hba)
> +{
> +	if (!hba->vops)
> +		return;
> +
> +	if (hba->vops->setup_clocks)
> +		hba->vops->setup_clocks(hba, false);
> +
> +	if (hba->vops->setup_regulators)
> +		hba->vops->setup_regulators(hba, false);
> +
> +	if (hba->vops->exit)
> +		hba->vops->exit(hba);
It's similar with init() case above.

> +}
> +
>  /**
>   * ufshcd_suspend - suspend power management function
>   * @hba: per adapter instance
> @@ -2894,23 +2965,22 @@ void ufshcd_remove(struct ufs_hba *hba)
>  	ufshcd_hba_stop(hba);
> 
>  	scsi_host_put(hba->host);
> +
> +	ufshcd_variant_hba_exit(hba);
>  }
>  EXPORT_SYMBOL_GPL(ufshcd_remove);
> 
>  /**
> - * ufshcd_init - Driver initialization routine
> + * ufshcd_alloc_host - allocate Host Bus Adapter (HBA)
>   * @dev: pointer to device handle
>   * @hba_handle: driver private handle
> - * @mmio_base: base register address
> - * @irq: Interrupt line of device
>   * Returns 0 on success, non-zero value on failure
>   */
> -int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle,
> -		 void __iomem *mmio_base, unsigned int irq)
> +int ufshcd_alloc_host(struct device *dev, struct ufs_hba **hba_handle)
>  {
>  	struct Scsi_Host *host;
>  	struct ufs_hba *hba;
> -	int err;
> +	int err = 0;
> 
>  	if (!dev) {
>  		dev_err(dev,
> @@ -2919,13 +2989,6 @@ int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle,
>  		goto out_error;
>  	}
> 
> -	if (!mmio_base) {
> -		dev_err(dev,
> -		"Invalid memory reference for mmio_base is NULL\n");
> -		err = -ENODEV;
> -		goto out_error;
> -	}
> -
>  	host = scsi_host_alloc(&ufshcd_driver_template,
>  				sizeof(struct ufs_hba));
>  	if (!host) {
> @@ -2936,9 +2999,40 @@ int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle,
>  	hba = shost_priv(host);
>  	hba->host = host;
>  	hba->dev = dev;
> +	*hba_handle = hba;
> +
> +out_error:
> +	return err;
> +}
> +EXPORT_SYMBOL(ufshcd_alloc_host);
> +
> +/**
> + * ufshcd_init - Driver initialization routine
> + * @hba: per-adapter instance
> + * @mmio_base: base register address
> + * @irq: Interrupt line of device
> + * Returns 0 on success, non-zero value on failure
> + */
> +int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
> +{
> +	int err;
> +	struct Scsi_Host *host = hba->host;
> +	struct device *dev = hba->dev;
> +
> +	if (!mmio_base) {
> +		dev_err(hba->dev,
> +		"Invalid memory reference for mmio_base is NULL\n");
> +		err = -ENODEV;
> +		goto out_error;
> +	}
> +
>  	hba->mmio_base = mmio_base;
>  	hba->irq = irq;
> 
> +	err = ufshcd_variant_hba_init(hba);
> +	if (err)
> +		goto out_error;
> +
>  	/* Read capabilities registers */
>  	ufshcd_hba_capabilities(hba);
> 
> @@ -3010,8 +3104,6 @@ int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle,
>  		goto out_remove_scsi_host;
>  	}
> 
> -	*hba_handle = hba;
> -
>  	/* Hold auto suspend until async scan completes */
>  	pm_runtime_get_sync(dev);
> 
> @@ -3023,6 +3115,7 @@ out_remove_scsi_host:
>  	scsi_remove_host(hba->host);
>  out_disable:
>  	scsi_host_put(host);
> +	ufshcd_variant_hba_exit(hba);
>  out_error:
>  	return err;
>  }
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 8f5624e..72acbc7 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -68,6 +68,8 @@
>  #define UFSHCD "ufshcd"
>  #define UFSHCD_DRIVER_VERSION "0.2"
> 
> +struct ufs_hba;
> +
>  enum dev_cmd_type {
>  	DEV_CMD_TYPE_NOP		= 0x0,
>  	DEV_CMD_TYPE_QUERY		= 0x1,
> @@ -152,6 +154,30 @@ struct ufs_dev_cmd {
>  	struct ufs_query query;
>  };
> 
> +#define PRE_CHANGE      0
> +#define POST_CHANGE     1
> +/**
> + * struct ufs_hba_variant_ops - variant specific callbacks
> + * @name: variant name
> + * @init: called when the driver is initialized
> + * @exit: called to cleanup everything done in init
> + * @setup_clocks: called before touching any of the controller registers
> + * @setup_regulators: called before accessing the host controller
> + * @hce_enable_notify: called before and after HCE enable bit is set to allow
> + *                     variant specific Uni-Pro initialization.
> + * @link_startup_notify: called before and after Link startup is carried out
> + *                       to allow variant specific Uni-Pro initialization.
> + */
> +struct ufs_hba_variant_ops {
> +	const char *name;
> +	int	(*init)(struct ufs_hba *);
> +	void    (*exit)(struct ufs_hba *);
> +	int     (*setup_clocks)(struct ufs_hba *, bool);
> +	int     (*setup_regulators)(struct ufs_hba *, bool);
> +	int     (*hce_enable_notify)(struct ufs_hba *, bool);
I agree on specific link_startup as HCI specification mentions.
Actually we also have implemented similar thing in our host driver.
But I'm not sure of a necessity of hce_enable_notify. Could you explain for that?

> +	int     (*link_startup_notify)(struct ufs_hba *, bool);
If a host specific implementation doesn't have two fully,
one of them will be called with flag unnecessarily.
How about separating into two functions[pre_, post_] for link_startup?

> +};
> +
>  /**
>   * struct ufs_hba - per adapter private structure
>   * @mmio_base: UFSHCI base register address
> @@ -171,6 +197,8 @@ struct ufs_dev_cmd {
>   * @nutrs: Transfer Request Queue depth supported by controller
>   * @nutmrs: Task Management Queue depth supported by controller
>   * @ufs_version: UFS Version to which controller complies
> + * @vops: pointer to variant specific operations
> + * @priv: pointer to variant specific private data
>   * @irq: Irq number of the controller
>   * @active_uic_cmd: handle of active UIC command
>   * @uic_cmd_mutex: mutex for uic command
> @@ -217,6 +245,8 @@ struct ufs_hba {
>  	int nutrs;
>  	int nutmrs;
>  	u32 ufs_version;
> +	struct ufs_hba_variant_ops *vops;
'const' declaration is expected.

Thanks,
Seungwon Jeon

> +	void *priv;
>  	unsigned int irq;
> 
>  	struct uic_command *active_uic_cmd;
> @@ -253,8 +283,8 @@ struct ufs_hba {
>  #define ufshcd_readl(hba, reg)	\
>  	readl((hba)->mmio_base + (reg))
> 
> -int ufshcd_init(struct device *, struct ufs_hba ** , void __iomem * ,
> -			unsigned int);
> +int ufshcd_alloc_host(struct device *, struct ufs_hba **);
> +int ufshcd_init(struct ufs_hba * , void __iomem * , unsigned int);
>  void ufshcd_remove(struct ufs_hba *);
> 
>  /**
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]
  Powered by Linux