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 devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html