On Fri, 11 Dec 2020 at 21:15, Hemant Kumar <hemantk@xxxxxxxxxxxxxx> wrote:
>
>
> Hi Loic,
> On 12/10/20 2:02 AM, 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@xxxxxxxxxx>
> > ---
> > drivers/bus/mhi/pci_generic.c | 120 +++++++++++++++++++++++++++++++++++++-----
> > 1 file changed, 107 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c
> > index 077595c..d70d3ea 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@xxxxxxxxxx>
> > */
> >
> > +#include <linux/delay.h>
> > #include <linux/device.h>
> > #include <linux/mhi.h>
> > #include <linux/module.h>
> > @@ -177,6 +178,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)
> > {
> > @@ -196,6 +207,20 @@ static void mhi_pci_status_cb(struct mhi_controller *mhi_cntrl,
> > /* Nothing to do for now */
> > }
> >
> > +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)
> > {
> > @@ -291,16 +316,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_BIT_MASK(info->dma_data_width);
> > @@ -315,17 +344,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);
> > @@ -340,33 +373,94 @@ 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_free_controller(mhi_cntrl);
> > }
> >
> > +void mhi_pci_reset_prepare(struct pci_dev *pdev)
> any reason for not making it static ?
> > +{
> > + 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_reg_soc_reset(mhi_cntrl);
> > +
> > + /* Be sure device reset has been executed */
> > + msleep(500);
> one suggestion, since you are adding this support for any generic MHI
> controller, would it be good to make this configurable in case some
> devcices need mode time to complete reset?
I can add a define, but 500ms is more than enough to cause device
reset, it's overkill to have a per controller value, in worst case we
can increase the value.
Regards,
Loic
