From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
To: Loic Poulain <loic.poulain@linaro.org>
Cc: hemantk@codeaurora.org, linux-arm-msm@vger.kernel.org,
bbhatt@codeaurora.org
Subject: Re: [PATCH v3 5/9] mhi: pci_generic: Add support for reset
Date: Sat, 28 Nov 2020 11:19:41 +0530 [thread overview]
Message-ID: <20201128054941.GD3077@thinkpad> (raw)
In-Reply-To: <1606404547-10737-6-git-send-email-loic.poulain@linaro.org>
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
>
next prev parent reply other threads:[~2020-11-28 22:13 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-26 15:28 [PATCH v3 0/9] mhi: pci_generic: Misc improvements Loic Poulain
2020-11-26 15:28 ` [PATCH v3 1/9] mhi: Add mhi_controller_initialize helper Loic Poulain
2020-11-28 5:42 ` Manivannan Sadhasivam
2020-12-02 1:41 ` Bhaumik Bhatt
2020-11-26 15:29 ` [PATCH v3 2/9] mhi: pci-generic: Increase number of hardware events Loic Poulain
2020-11-28 5:44 ` Manivannan Sadhasivam
2020-11-26 15:29 ` [PATCH v3 3/9] mhi: pci-generic: Perform hard reset on remove Loic Poulain
2020-11-27 17:34 ` Jeffrey Hugo
2020-11-27 17:40 ` Jeffrey Hugo
2020-11-26 15:29 ` [PATCH v3 4/9] mhi: pci_generic: Enable burst mode for hardware channels Loic Poulain
2020-11-28 5:45 ` Manivannan Sadhasivam
2020-11-26 15:29 ` [PATCH v3 5/9] mhi: pci_generic: Add support for reset Loic Poulain
2020-11-28 5:49 ` Manivannan Sadhasivam [this message]
2020-11-30 9:08 ` Loic Poulain
2020-12-02 1:24 ` Bhaumik Bhatt
2020-12-07 14:05 ` Loic Poulain
2020-11-26 15:29 ` [PATCH v3 6/9] mhi: pci_generic: Add suspend/resume/recovery procedure Loic Poulain
2020-11-26 15:29 ` [PATCH v3 7/9] mhi: pci_generic: Add PCI error handlers Loic Poulain
2020-11-26 15:29 ` [PATCH v3 8/9] mhi: pci_generic: Add health-check Loic Poulain
2020-11-28 5:59 ` Manivannan Sadhasivam
2020-12-01 0:59 ` Hemant Kumar
2020-12-01 1:02 ` Hemant Kumar
2020-11-26 15:29 ` [PATCH v3 9/9] mhi: pci_generic: Increase controller timeout value Loic Poulain
2020-11-28 5:51 ` Manivannan Sadhasivam
2020-11-28 6:00 ` [PATCH v3 0/9] mhi: pci_generic: Misc improvements Manivannan Sadhasivam
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201128054941.GD3077@thinkpad \
--to=manivannan.sadhasivam@linaro.org \
--cc=bbhatt@codeaurora.org \
--cc=hemantk@codeaurora.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=loic.poulain@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.