On 9/9/2013 5:03 PM, Seungwon Jeon wrote: > 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? Yes, we can. The reason I would like to keep it different because it can be used during power management. For example, - init -> clk_get(); setup_clocks -> clk_prepare_enable(); ufshcd_runtime_suspend() { ... hba->vops->setup_clocks(false); } ufshcd_runtime_resume() { hba->vops->setup_clocks(true); ... } > >> + 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. Just to keep consistent with init() procedures. > >> +} >> + >> /** >> * 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? I am not sure about other M-PHY implementations but in our h/w there are certain M-PHY specific configurations that needs to be done before enabling the host controller and also some state machine check after resetting the controller. > >> + 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? Agree. > >> +}; >> + >> /** >> * 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. Agree. > > 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 > -- Regards, Sujit -- 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