| Message ID | 1606404547-10737-6-git-send-email-loic.poulain@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mhi: pci_generic: Misc improvements | expand |
On Thu, Nov 26, 2020 at 04:29:03PM +0100, Loic Poulain wrote: > Add support for resetting the device, reset can be triggered in case > of error or manually via sysfs (/sys/bus/pci/devices/*/reset). > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > --- > drivers/bus/mhi/pci_generic.c | 119 +++++++++++++++++++++++++++++++++++++----- > 1 file changed, 106 insertions(+), 13 deletions(-) > > diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c > index 6b6e5bf..67fcbcf 100644 > --- a/drivers/bus/mhi/pci_generic.c > +++ b/drivers/bus/mhi/pci_generic.c > @@ -8,6 +8,7 @@ > * Copyright (C) 2020 Linaro Ltd <loic.poulain@linaro.org> > */ > > +#include <linux/delay.h> > #include <linux/device.h> > #include <linux/mhi.h> > #include <linux/module.h> > @@ -179,6 +180,16 @@ static const struct pci_device_id mhi_pci_id_table[] = { > }; > MODULE_DEVICE_TABLE(pci, mhi_pci_id_table); > > +enum mhi_pci_device_status { > + MHI_PCI_DEV_STARTED, > +}; > + > +struct mhi_pci_device { > + struct mhi_controller mhi_cntrl; > + struct pci_saved_state *pci_state; > + unsigned long status; > +}; > + > static int mhi_pci_read_reg(struct mhi_controller *mhi_cntrl, > void __iomem *addr, u32 *out) > { > @@ -203,6 +214,20 @@ static inline void mhi_pci_reset(struct mhi_controller *mhi_cntrl) > writel(1, mhi_cntrl->regs + DEV_RESET_REG); > } > > +static bool mhi_pci_is_alive(struct mhi_controller *mhi_cntrl) > +{ > + struct pci_dev *pdev = to_pci_dev(mhi_cntrl->cntrl_dev); > + u16 vendor = 0; > + > + if (pci_read_config_word(pdev, PCI_VENDOR_ID, &vendor)) > + return false; > + > + if (vendor == (u16) ~0 || vendor == 0) > + return false; > + > + return true; > +} > + > static int mhi_pci_claim(struct mhi_controller *mhi_cntrl, > unsigned int bar_num, u64 dma_mask) > { > @@ -298,16 +323,20 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > { > const struct mhi_pci_dev_info *info = (struct mhi_pci_dev_info *) id->driver_data; > const struct mhi_controller_config *mhi_cntrl_config; > + struct mhi_pci_device *mhi_pdev; > struct mhi_controller *mhi_cntrl; > int err; > > dev_dbg(&pdev->dev, "MHI PCI device found: %s\n", info->name); > > - mhi_cntrl = mhi_alloc_controller(); > - if (!mhi_cntrl) > + mhi_pdev = devm_kzalloc(&pdev->dev, sizeof(*mhi_pdev), GFP_KERNEL); > + if (!mhi_pdev) > return -ENOMEM; Still not agreeing to use the alloc API? I know that it does only one job but the reason for pushing this API is that the MHI stack will misbehave terribly if a non-initialized structure is passed to it. And the only way to ensure is to provide an API and recommend the users to use it. > > mhi_cntrl_config = info->config; > + mhi_cntrl = &mhi_pdev->mhi_cntrl; > + > + mhi_initialize_controller(mhi_cntrl); No, please just stick to alloc API. Thanks, Mani > mhi_cntrl->cntrl_dev = &pdev->dev; > mhi_cntrl->iova_start = 0; > mhi_cntrl->iova_stop = (dma_addr_t)DMA_BIT_MASK(info->dma_data_width); > @@ -322,17 +351,21 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > err = mhi_pci_claim(mhi_cntrl, info->bar_num, DMA_BIT_MASK(info->dma_data_width)); > if (err) > - goto err_release; > + return err; > > err = mhi_pci_get_irqs(mhi_cntrl, mhi_cntrl_config); > if (err) > - goto err_release; > + return err; > + > + pci_set_drvdata(pdev, mhi_pdev); > > - pci_set_drvdata(pdev, mhi_cntrl); > + /* Have stored pci confspace at hand for restore in sudden PCI error */ > + pci_save_state(pdev); > + mhi_pdev->pci_state = pci_store_saved_state(pdev); > > err = mhi_register_controller(mhi_cntrl, mhi_cntrl_config); > if (err) > - goto err_release; > + return err; > > /* MHI bus does not power up the controller by default */ > err = mhi_prepare_for_power_up(mhi_cntrl); > @@ -347,37 +380,97 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > goto err_unprepare; > } > > + set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status); > + > return 0; > > err_unprepare: > mhi_unprepare_after_power_down(mhi_cntrl); > err_unregister: > mhi_unregister_controller(mhi_cntrl); > -err_release: > - mhi_free_controller(mhi_cntrl); > > return err; > } > > static void mhi_pci_remove(struct pci_dev *pdev) > { > - struct mhi_controller *mhi_cntrl = pci_get_drvdata(pdev); > + struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev); > + struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl; > + > + if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) { > + mhi_power_down(mhi_cntrl, true); > + mhi_unprepare_after_power_down(mhi_cntrl); > + } > > - mhi_power_down(mhi_cntrl, true); > - mhi_unprepare_after_power_down(mhi_cntrl); > mhi_unregister_controller(mhi_cntrl); > > /* MHI-layer reset could not be enough, always hard-reset the device */ > mhi_pci_reset(mhi_cntrl); > +} > + > +void mhi_pci_reset_prepare(struct pci_dev *pdev) > +{ > + struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev); > + struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl; > + > + dev_info(&pdev->dev, "reset\n"); > + > + /* Clean up MHI state */ > + if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) { > + mhi_power_down(mhi_cntrl, false); > + mhi_unprepare_after_power_down(mhi_cntrl); > + } > + > + /* cause internal device reset */ > + mhi_pci_reset(mhi_cntrl); > + > + /* Be sure device reset has been executed */ > + msleep(500); > +} > + > +void mhi_pci_reset_done(struct pci_dev *pdev) > +{ > + struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev); > + struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl; > + int err; > + > + /* Restore initial known working PCI state */ > + pci_load_saved_state(pdev, mhi_pdev->pci_state); > + pci_restore_state(pdev); > + > + /* Is device status available ? */ > + if (!mhi_pci_is_alive(mhi_cntrl)) { > + dev_err(&pdev->dev, "reset failed\n"); > + return; > + } > > - mhi_free_controller(mhi_cntrl); > + err = mhi_prepare_for_power_up(mhi_cntrl); > + if (err) { > + dev_err(&pdev->dev, "failed to prepare MHI controller\n"); > + return; > + } > + > + err = mhi_sync_power_up(mhi_cntrl); > + if (err) { > + dev_err(&pdev->dev, "failed to power up MHI controller\n"); > + mhi_unprepare_after_power_down(mhi_cntrl); > + return; > + } > + > + set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status); > } > > +static const struct pci_error_handlers mhi_pci_err_handler = { > + .reset_prepare = mhi_pci_reset_prepare, > + .reset_done = mhi_pci_reset_done, > +}; > + > static struct pci_driver mhi_pci_driver = { > .name = "mhi-pci-generic", > .id_table = mhi_pci_id_table, > .probe = mhi_pci_probe, > - .remove = mhi_pci_remove > + .remove = mhi_pci_remove, > + .err_handler = &mhi_pci_err_handler, > }; > module_pci_driver(mhi_pci_driver); > > -- > 2.7.4 >
Hi Mani, Bhaumik, On Sat, 28 Nov 2020 at 06:49, Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote: > > > { > > @@ -298,16 +323,20 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > { > > const struct mhi_pci_dev_info *info = (struct mhi_pci_dev_info *) id->driver_data; > > const struct mhi_controller_config *mhi_cntrl_config; > > + struct mhi_pci_device *mhi_pdev; > > struct mhi_controller *mhi_cntrl; > > int err; > > > > dev_dbg(&pdev->dev, "MHI PCI device found: %s\n", info->name); > > > > - mhi_cntrl = mhi_alloc_controller(); > > - if (!mhi_cntrl) > > + mhi_pdev = devm_kzalloc(&pdev->dev, sizeof(*mhi_pdev), GFP_KERNEL); > > + if (!mhi_pdev) > > return -ENOMEM; > > Still not agreeing to use the alloc API? I know that it does only one > job but the reason for pushing this API is that the MHI stack will > misbehave terribly if a non-initialized structure is passed to it. And > the only way to ensure is to provide an API and recommend the users to > use it. I think there is a misunderstanding of my will here :-), actually, I'm not against using a specific API, but as you see here I'm not allocating a mhi_controller structure but a mhi_pci_device structure, which in turn includes (or inherit from) mhi_controller struct. If mhi_alloc_controller() is the only API allowing to create+initialize a mhi_controller object, that implies: a. Statically allocated mhi_controller is not possible b. non-standalone mhi_controller structure is not possible (my case) If you mandate this and do not allow a. and b. , then yes, I'll use mhi_controller_alloc, but that would mean having two nested dynamic allocations, one for mhi_pdev and one for mhi_pdev->mhi_cntrl, cross-referencing (for finding mhi_pdev from mhi_cntrl since no more container_of), and double freeing... that complicates a bit, is suboptimal and does not really make sense, since conceptually, mhi_pdev and its mhi_cntl member are the same 'object' (exactly like mhi_dev and mhi_dev->dev are the same object in MHI core). I understand we may have to perform some extra initialization and cannot just do zeroed allocation for the mhi_controller, but what I say is that this initialization should be do-able, regardless you want to (also) dynamically allocate the device or not. That why I proposed to introduce mhi_initialize_controller() as a solution to keep things simple, in the same way as other subsystems: device_initialize, snd_device_initialize, nand_controller_init... Regards, Loic > > > > > mhi_cntrl_config = info->config; > > + mhi_cntrl = &mhi_pdev->mhi_cntrl; > > + > > + mhi_initialize_controller(mhi_cntrl); > > No, please just stick to alloc API. > > Thanks, > Mani > > > mhi_cntrl->cntrl_dev = &pdev->dev; > > mhi_cntrl->iova_start = 0; > > mhi_cntrl->iova_stop = (dma_addr_t)DMA_BIT_MASK(info->dma_data_width); > > @@ -322,17 +351,21 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > > > err = mhi_pci_claim(mhi_cntrl, info->bar_num, DMA_BIT_MASK(info->dma_data_width)); > > if (err) > > - goto err_release; > > + return err; > > > > err = mhi_pci_get_irqs(mhi_cntrl, mhi_cntrl_config); > > if (err) > > - goto err_release; > > + return err; > > + > > + pci_set_drvdata(pdev, mhi_pdev); > > > > - pci_set_drvdata(pdev, mhi_cntrl); > > + /* Have stored pci confspace at hand for restore in sudden PCI error */ > > + pci_save_state(pdev); > > + mhi_pdev->pci_state = pci_store_saved_state(pdev); > > > > err = mhi_register_controller(mhi_cntrl, mhi_cntrl_config); > > if (err) > > - goto err_release; > > + return err; > > > > /* MHI bus does not power up the controller by default */ > > err = mhi_prepare_for_power_up(mhi_cntrl); > > @@ -347,37 +380,97 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > goto err_unprepare; > > } > > > > + set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status); > > + > > return 0; > > > > err_unprepare: > > mhi_unprepare_after_power_down(mhi_cntrl); > > err_unregister: > > mhi_unregister_controller(mhi_cntrl); > > -err_release: > > - mhi_free_controller(mhi_cntrl); > > > > return err; > > } > > > > static void mhi_pci_remove(struct pci_dev *pdev) > > { > > - struct mhi_controller *mhi_cntrl = pci_get_drvdata(pdev); > > + struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev); > > + struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl; > > + > > + if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) { > > + mhi_power_down(mhi_cntrl, true); > > + mhi_unprepare_after_power_down(mhi_cntrl); > > + } > > > > - mhi_power_down(mhi_cntrl, true); > > - mhi_unprepare_after_power_down(mhi_cntrl); > > mhi_unregister_controller(mhi_cntrl); > > > > /* MHI-layer reset could not be enough, always hard-reset the device */ > > mhi_pci_reset(mhi_cntrl); > > +} > > + > > +void mhi_pci_reset_prepare(struct pci_dev *pdev) > > +{ > > + struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev); > > + struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl; > > + > > + dev_info(&pdev->dev, "reset\n"); > > + > > + /* Clean up MHI state */ > > + if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) { > > + mhi_power_down(mhi_cntrl, false); > > + mhi_unprepare_after_power_down(mhi_cntrl); > > + } > > + > > + /* cause internal device reset */ > > + mhi_pci_reset(mhi_cntrl); > > + > > + /* Be sure device reset has been executed */ > > + msleep(500); > > +} > > + > > +void mhi_pci_reset_done(struct pci_dev *pdev) > > +{ > > + struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev); > > + struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl; > > + int err; > > + > > + /* Restore initial known working PCI state */ > > + pci_load_saved_state(pdev, mhi_pdev->pci_state); > > + pci_restore_state(pdev); > > + > > + /* Is device status available ? */ > > + if (!mhi_pci_is_alive(mhi_cntrl)) { > > + dev_err(&pdev->dev, "reset failed\n"); > > + return; > > + } > > > > - mhi_free_controller(mhi_cntrl); > > + err = mhi_prepare_for_power_up(mhi_cntrl); > > + if (err) { > > + dev_err(&pdev->dev, "failed to prepare MHI controller\n"); > > + return; > > + } > > + > > + err = mhi_sync_power_up(mhi_cntrl); > > + if (err) { > > + dev_err(&pdev->dev, "failed to power up MHI controller\n"); > > + mhi_unprepare_after_power_down(mhi_cntrl); > > + return; > > + } > > + > > + set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status); > > } > > > > +static const struct pci_error_handlers mhi_pci_err_handler = { > > + .reset_prepare = mhi_pci_reset_prepare, > > + .reset_done = mhi_pci_reset_done, > > +}; > > + > > static struct pci_driver mhi_pci_driver = { > > .name = "mhi-pci-generic", > > .id_table = mhi_pci_id_table, > > .probe = mhi_pci_probe, > > - .remove = mhi_pci_remove > > + .remove = mhi_pci_remove, > > + .err_handler = &mhi_pci_err_handler, > > }; > > module_pci_driver(mhi_pci_driver); > > > > -- > > 2.7.4 > >
Hi Loic/Mani, On 2020-11-30 01:08 AM, Loic Poulain wrote: > Hi Mani, Bhaumik, > > On Sat, 28 Nov 2020 at 06:49, Manivannan Sadhasivam > <manivannan.sadhasivam@linaro.org> wrote: >> >> > { >> > @@ -298,16 +323,20 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) >> > { >> > const struct mhi_pci_dev_info *info = (struct mhi_pci_dev_info *) id->driver_data; >> > const struct mhi_controller_config *mhi_cntrl_config; >> > + struct mhi_pci_device *mhi_pdev; >> > struct mhi_controller *mhi_cntrl; >> > int err; >> > >> > dev_dbg(&pdev->dev, "MHI PCI device found: %s\n", info->name); >> > >> > - mhi_cntrl = mhi_alloc_controller(); >> > - if (!mhi_cntrl) >> > + mhi_pdev = devm_kzalloc(&pdev->dev, sizeof(*mhi_pdev), GFP_KERNEL); >> > + if (!mhi_pdev) >> > return -ENOMEM; >> >> Still not agreeing to use the alloc API? I know that it does only one >> job but the reason for pushing this API is that the MHI stack will >> misbehave terribly if a non-initialized structure is passed to it. And >> the only way to ensure is to provide an API and recommend the users to >> use it. > > I think there is a misunderstanding of my will here :-), actually, I'm > not against using a specific API, but as you see here I'm not > allocating a mhi_controller structure but a mhi_pci_device structure, > which in turn includes (or inherit from) mhi_controller struct. > > If mhi_alloc_controller() is the only API allowing to > create+initialize a mhi_controller object, that implies: > a. Statically allocated mhi_controller is not possible > b. non-standalone mhi_controller structure is not possible (my case) > > If you mandate this and do not allow a. and b. , then yes, I'll use > mhi_controller_alloc, but that would mean having two nested dynamic > allocations, one for mhi_pdev and one for mhi_pdev->mhi_cntrl, > cross-referencing (for finding mhi_pdev from mhi_cntrl since no more > container_of), and double freeing... that complicates a bit, is > suboptimal and does not really make sense, since conceptually, > mhi_pdev and its mhi_cntl member are the same 'object' (exactly like > mhi_dev and mhi_dev->dev are the same object in MHI core). > > I understand we may have to perform some extra initialization and > cannot just do zeroed allocation for the mhi_controller, but what I > say is that this initialization should be do-able, regardless you want > to (also) dynamically allocate the device or not. That why I proposed > to introduce mhi_initialize_controller() as a solution to keep things > simple, in the same way as other subsystems: device_initialize, > snd_device_initialize, nand_controller_init... > > Regards, > Loic > > I don't see using the new API as a problem but it is more work for the controller and having one API is better than having two and having to explain why we have them. If Mani agrees with this approach I am OK with it. >> >> > >> > mhi_cntrl_config = info->config; >> > + mhi_cntrl = &mhi_pdev->mhi_cntrl; >> > + >> > + mhi_initialize_controller(mhi_cntrl); >> >> No, please just stick to alloc API. >> >> Thanks, >> Mani >> >> > mhi_cntrl->cntrl_dev = &pdev->dev; >> > mhi_cntrl->iova_start = 0; >> > mhi_cntrl->iova_stop = (dma_addr_t)DMA_BIT_MASK(info->dma_data_width); >> > @@ -322,17 +351,21 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) >> > >> > err = mhi_pci_claim(mhi_cntrl, info->bar_num, DMA_BIT_MASK(info->dma_data_width)); >> > if (err) >> > - goto err_release; >> > + return err; >> > >> > err = mhi_pci_get_irqs(mhi_cntrl, mhi_cntrl_config); >> > if (err) >> > - goto err_release; >> > + return err; >> > + >> > + pci_set_drvdata(pdev, mhi_pdev); >> > >> > - pci_set_drvdata(pdev, mhi_cntrl); >> > + /* Have stored pci confspace at hand for restore in sudden PCI error */ >> > + pci_save_state(pdev); >> > + mhi_pdev->pci_state = pci_store_saved_state(pdev); >> > >> > err = mhi_register_controller(mhi_cntrl, mhi_cntrl_config); >> > if (err) >> > - goto err_release; >> > + return err; >> > >> > /* MHI bus does not power up the controller by default */ >> > err = mhi_prepare_for_power_up(mhi_cntrl); >> > @@ -347,37 +380,97 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) >> > goto err_unprepare; >> > } >> > >> > + set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status); >> > + >> > return 0; >> > >> > err_unprepare: >> > mhi_unprepare_after_power_down(mhi_cntrl); >> > err_unregister: >> > mhi_unregister_controller(mhi_cntrl); >> > -err_release: >> > - mhi_free_controller(mhi_cntrl); >> > >> > return err; >> > } >> > >> > static void mhi_pci_remove(struct pci_dev *pdev) >> > { >> > - struct mhi_controller *mhi_cntrl = pci_get_drvdata(pdev); >> > + struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev); >> > + struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl; >> > + >> > + if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) { >> > + mhi_power_down(mhi_cntrl, true); >> > + mhi_unprepare_after_power_down(mhi_cntrl); >> > + } >> > >> > - mhi_power_down(mhi_cntrl, true); >> > - mhi_unprepare_after_power_down(mhi_cntrl); >> > mhi_unregister_controller(mhi_cntrl); >> > >> > /* MHI-layer reset could not be enough, always hard-reset the device */ >> > mhi_pci_reset(mhi_cntrl); >> > +} >> > + >> > +void mhi_pci_reset_prepare(struct pci_dev *pdev) >> > +{ >> > + struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev); >> > + struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl; >> > + >> > + dev_info(&pdev->dev, "reset\n"); >> > + >> > + /* Clean up MHI state */ >> > + if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) { >> > + mhi_power_down(mhi_cntrl, false); >> > + mhi_unprepare_after_power_down(mhi_cntrl); >> > + } >> > + >> > + /* cause internal device reset */ >> > + mhi_pci_reset(mhi_cntrl); >> > + >> > + /* Be sure device reset has been executed */ >> > + msleep(500); >> > +} >> > + >> > +void mhi_pci_reset_done(struct pci_dev *pdev) >> > +{ >> > + struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev); >> > + struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl; >> > + int err; >> > + >> > + /* Restore initial known working PCI state */ >> > + pci_load_saved_state(pdev, mhi_pdev->pci_state); >> > + pci_restore_state(pdev); >> > + >> > + /* Is device status available ? */ >> > + if (!mhi_pci_is_alive(mhi_cntrl)) { >> > + dev_err(&pdev->dev, "reset failed\n"); >> > + return; >> > + } >> > >> > - mhi_free_controller(mhi_cntrl); >> > + err = mhi_prepare_for_power_up(mhi_cntrl); >> > + if (err) { >> > + dev_err(&pdev->dev, "failed to prepare MHI controller\n"); >> > + return; >> > + } >> > + >> > + err = mhi_sync_power_up(mhi_cntrl); >> > + if (err) { >> > + dev_err(&pdev->dev, "failed to power up MHI controller\n"); >> > + mhi_unprepare_after_power_down(mhi_cntrl); >> > + return; >> > + } >> > + >> > + set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status); >> > } >> > >> > +static const struct pci_error_handlers mhi_pci_err_handler = { >> > + .reset_prepare = mhi_pci_reset_prepare, >> > + .reset_done = mhi_pci_reset_done, >> > +}; >> > + >> > static struct pci_driver mhi_pci_driver = { >> > .name = "mhi-pci-generic", >> > .id_table = mhi_pci_id_table, >> > .probe = mhi_pci_probe, >> > - .remove = mhi_pci_remove >> > + .remove = mhi_pci_remove, >> > + .err_handler = &mhi_pci_err_handler, >> > }; >> > module_pci_driver(mhi_pci_driver); >> > >> > -- >> > 2.7.4 >> > Thanks, Bhaumik --- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Hi Mani, On Wed, 2 Dec 2020 at 02:24, Bhaumik Bhatt <bbhatt@codeaurora.org> wrote: > > On Sat, 28 Nov 2020 at 06:49, Manivannan Sadhasivam > > <manivannan.sadhasivam@linaro.org> wrote: > >> > >> > { > >> > @@ -298,16 +323,20 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > >> > { > >> > const struct mhi_pci_dev_info *info = (struct mhi_pci_dev_info *) id->driver_data; > >> > const struct mhi_controller_config *mhi_cntrl_config; > >> > + struct mhi_pci_device *mhi_pdev; > >> > struct mhi_controller *mhi_cntrl; > >> > int err; > >> > > >> > dev_dbg(&pdev->dev, "MHI PCI device found: %s\n", info->name); > >> > > >> > - mhi_cntrl = mhi_alloc_controller(); > >> > - if (!mhi_cntrl) > >> > + mhi_pdev = devm_kzalloc(&pdev->dev, sizeof(*mhi_pdev), GFP_KERNEL); > >> > + if (!mhi_pdev) > >> > return -ENOMEM; > >> > >> Still not agreeing to use the alloc API? I know that it does only one > >> job but the reason for pushing this API is that the MHI stack will > >> misbehave terribly if a non-initialized structure is passed to it. And > >> the only way to ensure is to provide an API and recommend the users to > >> use it. > > > > I think there is a misunderstanding of my will here :-), actually, I'm > > not against using a specific API, but as you see here I'm not > > allocating a mhi_controller structure but a mhi_pci_device structure, > > which in turn includes (or inherit from) mhi_controller struct. > > > > If mhi_alloc_controller() is the only API allowing to > > create+initialize a mhi_controller object, that implies: > > a. Statically allocated mhi_controller is not possible > > b. non-standalone mhi_controller structure is not possible (my case) > > > > If you mandate this and do not allow a. and b. , then yes, I'll use > > mhi_controller_alloc, but that would mean having two nested dynamic > > allocations, one for mhi_pdev and one for mhi_pdev->mhi_cntrl, > > cross-referencing (for finding mhi_pdev from mhi_cntrl since no more > > container_of), and double freeing... that complicates a bit, is > > suboptimal and does not really make sense, since conceptually, > > mhi_pdev and its mhi_cntl member are the same 'object' (exactly like > > mhi_dev and mhi_dev->dev are the same object in MHI core). > > > > I understand we may have to perform some extra initialization and > > cannot just do zeroed allocation for the mhi_controller, but what I > > say is that this initialization should be do-able, regardless you want > > to (also) dynamically allocate the device or not. That why I proposed > > to introduce mhi_initialize_controller() as a solution to keep things > > simple, in the same way as other subsystems: device_initialize, > > snd_device_initialize, nand_controller_init... > > > > Regards, > > Loic > > > > > I don't see using the new API as a problem but it is more work for the > controller and having one API is better than having two and having to > explain > why we have them. Before going with V4, I would like your feedback here. Thanks, Loic > > If Mani agrees with this approach I am OK with it. > >> > >> > > >> > mhi_cntrl_config = info->config; > >> > + mhi_cntrl = &mhi_pdev->mhi_cntrl; > >> > + > >> > + mhi_initialize_controller(mhi_cntrl); > >> > >> No, please just stick to alloc API. > >> > >> Thanks, > >> Mani > >> > >> > mhi_cntrl->cntrl_dev = &pdev->dev; > >> > mhi_cntrl->iova_start = 0; > >> > mhi_cntrl->iova_stop = (dma_addr_t)DMA_BIT_MASK(info->dma_data_width); > >> > @@ -322,17 +351,21 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > >> > > >> > err = mhi_pci_claim(mhi_cntrl, info->bar_num, DMA_BIT_MASK(info->dma_data_width)); > >> > if (err) > >> > - goto err_release; > >> > + return err; > >> > > >> > err = mhi_pci_get_irqs(mhi_cntrl, mhi_cntrl_config); > >> > if (err) > >> > - goto err_release; > >> > + return err; > >> > + > >> > + pci_set_drvdata(pdev, mhi_pdev); > >> > > >> > - pci_set_drvdata(pdev, mhi_cntrl); > >> > + /* Have stored pci confspace at hand for restore in sudden PCI error */ > >> > + pci_save_state(pdev); > >> > + mhi_pdev->pci_state = pci_store_saved_state(pdev); > >> > > >> > err = mhi_register_controller(mhi_cntrl, mhi_cntrl_config); > >> > if (err) > >> > - goto err_release; > >> > + return err; > >> > > >> > /* MHI bus does not power up the controller by default */ > >> > err = mhi_prepare_for_power_up(mhi_cntrl); > >> > @@ -347,37 +380,97 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > >> > goto err_unprepare; > >> > } > >> > > >> > + set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status); > >> > + > >> > return 0; > >> > > >> > err_unprepare: > >> > mhi_unprepare_after_power_down(mhi_cntrl); > >> > err_unregister: > >> > mhi_unregister_controller(mhi_cntrl); > >> > -err_release: > >> > - mhi_free_controller(mhi_cntrl); > >> > > >> > return err; > >> > } > >> > > >> > static void mhi_pci_remove(struct pci_dev *pdev) > >> > { > >> > - struct mhi_controller *mhi_cntrl = pci_get_drvdata(pdev); > >> > + struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev); > >> > + struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl; > >> > + > >> > + if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) { > >> > + mhi_power_down(mhi_cntrl, true); > >> > + mhi_unprepare_after_power_down(mhi_cntrl); > >> > + } > >> > > >> > - mhi_power_down(mhi_cntrl, true); > >> > - mhi_unprepare_after_power_down(mhi_cntrl); > >> > mhi_unregister_controller(mhi_cntrl); > >> > > >> > /* MHI-layer reset could not be enough, always hard-reset the device */ > >> > mhi_pci_reset(mhi_cntrl); > >> > +} > >> > + > >> > +void mhi_pci_reset_prepare(struct pci_dev *pdev) > >> > +{ > >> > + struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev); > >> > + struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl; > >> > + > >> > + dev_info(&pdev->dev, "reset\n"); > >> > + > >> > + /* Clean up MHI state */ > >> > + if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) { > >> > + mhi_power_down(mhi_cntrl, false); > >> > + mhi_unprepare_after_power_down(mhi_cntrl); > >> > + } > >> > + > >> > + /* cause internal device reset */ > >> > + mhi_pci_reset(mhi_cntrl); > >> > + > >> > + /* Be sure device reset has been executed */ > >> > + msleep(500); > >> > +} > >> > + > >> > +void mhi_pci_reset_done(struct pci_dev *pdev) > >> > +{ > >> > + struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev); > >> > + struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl; > >> > + int err; > >> > + > >> > + /* Restore initial known working PCI state */ > >> > + pci_load_saved_state(pdev, mhi_pdev->pci_state); > >> > + pci_restore_state(pdev); > >> > + > >> > + /* Is device status available ? */ > >> > + if (!mhi_pci_is_alive(mhi_cntrl)) { > >> > + dev_err(&pdev->dev, "reset failed\n"); > >> > + return; > >> > + } > >> > > >> > - mhi_free_controller(mhi_cntrl); > >> > + err = mhi_prepare_for_power_up(mhi_cntrl); > >> > + if (err) { > >> > + dev_err(&pdev->dev, "failed to prepare MHI controller\n"); > >> > + return; > >> > + } > >> > + > >> > + err = mhi_sync_power_up(mhi_cntrl); > >> > + if (err) { > >> > + dev_err(&pdev->dev, "failed to power up MHI controller\n"); > >> > + mhi_unprepare_after_power_down(mhi_cntrl); > >> > + return; > >> > + } > >> > + > >> > + set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status); > >> > } > >> > > >> > +static const struct pci_error_handlers mhi_pci_err_handler = { > >> > + .reset_prepare = mhi_pci_reset_prepare, > >> > + .reset_done = mhi_pci_reset_done, > >> > +}; > >> > + > >> > static struct pci_driver mhi_pci_driver = { > >> > .name = "mhi-pci-generic", > >> > .id_table = mhi_pci_id_table, > >> > .probe = mhi_pci_probe, > >> > - .remove = mhi_pci_remove > >> > + .remove = mhi_pci_remove, > >> > + .err_handler = &mhi_pci_err_handler, > >> > }; > >> > module_pci_driver(mhi_pci_driver); > >> > > >> > -- > >> > 2.7.4 > >> > > > Thanks, > Bhaumik > --- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora > Forum, > a Linux Foundation Collaborative Project
diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c index 6b6e5bf..67fcbcf 100644 --- a/drivers/bus/mhi/pci_generic.c +++ b/drivers/bus/mhi/pci_generic.c @@ -8,6 +8,7 @@ * Copyright (C) 2020 Linaro Ltd <loic.poulain@linaro.org> */ +#include <linux/delay.h> #include <linux/device.h> #include <linux/mhi.h> #include <linux/module.h> @@ -179,6 +180,16 @@ static const struct pci_device_id mhi_pci_id_table[] = { }; MODULE_DEVICE_TABLE(pci, mhi_pci_id_table); +enum mhi_pci_device_status { + MHI_PCI_DEV_STARTED, +}; + +struct mhi_pci_device { + struct mhi_controller mhi_cntrl; + struct pci_saved_state *pci_state; + unsigned long status; +}; + static int mhi_pci_read_reg(struct mhi_controller *mhi_cntrl, void __iomem *addr, u32 *out) { @@ -203,6 +214,20 @@ static inline void mhi_pci_reset(struct mhi_controller *mhi_cntrl) writel(1, mhi_cntrl->regs + DEV_RESET_REG); } +static bool mhi_pci_is_alive(struct mhi_controller *mhi_cntrl) +{ + struct pci_dev *pdev = to_pci_dev(mhi_cntrl->cntrl_dev); + u16 vendor = 0; + + if (pci_read_config_word(pdev, PCI_VENDOR_ID, &vendor)) + return false; + + if (vendor == (u16) ~0 || vendor == 0) + return false; + + return true; +} + static int mhi_pci_claim(struct mhi_controller *mhi_cntrl, unsigned int bar_num, u64 dma_mask) { @@ -298,16 +323,20 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) { const struct mhi_pci_dev_info *info = (struct mhi_pci_dev_info *) id->driver_data; const struct mhi_controller_config *mhi_cntrl_config; + struct mhi_pci_device *mhi_pdev; struct mhi_controller *mhi_cntrl; int err; dev_dbg(&pdev->dev, "MHI PCI device found: %s\n", info->name); - mhi_cntrl = mhi_alloc_controller(); - if (!mhi_cntrl) + mhi_pdev = devm_kzalloc(&pdev->dev, sizeof(*mhi_pdev), GFP_KERNEL); + if (!mhi_pdev) return -ENOMEM; mhi_cntrl_config = info->config; + mhi_cntrl = &mhi_pdev->mhi_cntrl; + + mhi_initialize_controller(mhi_cntrl); mhi_cntrl->cntrl_dev = &pdev->dev; mhi_cntrl->iova_start = 0; mhi_cntrl->iova_stop = (dma_addr_t)DMA_BIT_MASK(info->dma_data_width); @@ -322,17 +351,21 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) err = mhi_pci_claim(mhi_cntrl, info->bar_num, DMA_BIT_MASK(info->dma_data_width)); if (err) - goto err_release; + return err; err = mhi_pci_get_irqs(mhi_cntrl, mhi_cntrl_config); if (err) - goto err_release; + return err; + + pci_set_drvdata(pdev, mhi_pdev); - pci_set_drvdata(pdev, mhi_cntrl); + /* Have stored pci confspace at hand for restore in sudden PCI error */ + pci_save_state(pdev); + mhi_pdev->pci_state = pci_store_saved_state(pdev); err = mhi_register_controller(mhi_cntrl, mhi_cntrl_config); if (err) - goto err_release; + return err; /* MHI bus does not power up the controller by default */ err = mhi_prepare_for_power_up(mhi_cntrl); @@ -347,37 +380,97 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) goto err_unprepare; } + set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status); + return 0; err_unprepare: mhi_unprepare_after_power_down(mhi_cntrl); err_unregister: mhi_unregister_controller(mhi_cntrl); -err_release: - mhi_free_controller(mhi_cntrl); return err; } static void mhi_pci_remove(struct pci_dev *pdev) { - struct mhi_controller *mhi_cntrl = pci_get_drvdata(pdev); + struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev); + struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl; + + if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) { + mhi_power_down(mhi_cntrl, true); + mhi_unprepare_after_power_down(mhi_cntrl); + } - mhi_power_down(mhi_cntrl, true); - mhi_unprepare_after_power_down(mhi_cntrl); mhi_unregister_controller(mhi_cntrl); /* MHI-layer reset could not be enough, always hard-reset the device */ mhi_pci_reset(mhi_cntrl); +} + +void mhi_pci_reset_prepare(struct pci_dev *pdev) +{ + struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev); + struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl; + + dev_info(&pdev->dev, "reset\n"); + + /* Clean up MHI state */ + if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) { + mhi_power_down(mhi_cntrl, false); + mhi_unprepare_after_power_down(mhi_cntrl); + } + + /* cause internal device reset */ + mhi_pci_reset(mhi_cntrl); + + /* Be sure device reset has been executed */ + msleep(500); +} + +void mhi_pci_reset_done(struct pci_dev *pdev) +{ + struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev); + struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl; + int err; + + /* Restore initial known working PCI state */ + pci_load_saved_state(pdev, mhi_pdev->pci_state); + pci_restore_state(pdev); + + /* Is device status available ? */ + if (!mhi_pci_is_alive(mhi_cntrl)) { + dev_err(&pdev->dev, "reset failed\n"); + return; + } - mhi_free_controller(mhi_cntrl); + err = mhi_prepare_for_power_up(mhi_cntrl); + if (err) { + dev_err(&pdev->dev, "failed to prepare MHI controller\n"); + return; + } + + err = mhi_sync_power_up(mhi_cntrl); + if (err) { + dev_err(&pdev->dev, "failed to power up MHI controller\n"); + mhi_unprepare_after_power_down(mhi_cntrl); + return; + } + + set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status); } +static const struct pci_error_handlers mhi_pci_err_handler = { + .reset_prepare = mhi_pci_reset_prepare, + .reset_done = mhi_pci_reset_done, +}; + static struct pci_driver mhi_pci_driver = { .name = "mhi-pci-generic", .id_table = mhi_pci_id_table, .probe = mhi_pci_probe, - .remove = mhi_pci_remove + .remove = mhi_pci_remove, + .err_handler = &mhi_pci_err_handler, }; module_pci_driver(mhi_pci_driver);
Add support for resetting the device, reset can be triggered in case of error or manually via sysfs (/sys/bus/pci/devices/*/reset). Signed-off-by: Loic Poulain <loic.poulain@linaro.org> --- drivers/bus/mhi/pci_generic.c | 119 +++++++++++++++++++++++++++++++++++++----- 1 file changed, 106 insertions(+), 13 deletions(-)