Re: [PATCH 4/8] mhi: pci_generic: Add support for reset

[Bhaumik Bhatt <bbhatt@codeaurora.org>]

On 2020-11-13 09:42, Loic Poulain wrote:
> Hi Bhaumik,
> 
> On Fri, 13 Nov 2020 at 18:12, Bhaumik Bhatt <bbhatt@codeaurora.org> 
> wrote:
>> 
>> On 2020-11-13 06:59, 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 | 117
>> > +++++++++++++++++++++++++++++++++++++-----
>> >  1 file changed, 104 insertions(+), 13 deletions(-)
>> >
>> > diff --git a/drivers/bus/mhi/pci_generic.c
>> > b/drivers/bus/mhi/pci_generic.c
>> > index 0c07cf5..b48c382 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,18 @@ 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)
>> Not recommended.
>> > +     mhi_pdev = devm_kzalloc(&pdev->dev, sizeof(*mhi_pdev), GFP_KERNEL);
>> > +     if (!mhi_pdev)
>> >               return -ENOMEM;
>> Why move away from the mhi_alloc_controller/mhi_free_controller pair 
>> we
>> recommended for use?
> 
> Because now I don't allocate mhi controller separately, it's part of
> the allocated mhi_pci_device that inherit from it. If necessary we
> would need something like mhi_init_controller() to initialize the
> field, but AFAIU everything needs to be zeroed.
Let's make sure we use the mhi_alloc/free_controller APIs because it 
will
help us have better control in case we need track of some allocations.
> 
> Regards,
> Loic

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
a Linux Foundation Collaborative Project