Re: [PATCH 4/4] PCI: pci-epf-test: Add support to defer core initialization

[Kishon Vijay Abraham I <kishon@ti.com>]

Hi,

On 01/12/19 7:59 pm, Vidya Sagar wrote:
> On 11/27/2019 2:50 PM, Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On 13/11/19 2:38 PM, Vidya Sagar wrote:
>>> Add support to defer core initialization and to receive a notifier
>>> when core is ready to accommodate platforms where core is not for
>>> initialization untile reference clock from host is available.
>>>
>>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
>>> ---
>>>   drivers/pci/endpoint/functions/pci-epf-test.c | 114 ++++++++++++------
>>>   1 file changed, 77 insertions(+), 37 deletions(-)
>>>
>>> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c 
>>> b/drivers/pci/endpoint/functions/pci-epf-test.c
>>> index bddff15052cc..068024fab544 100644
>>> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
>>> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
>>> @@ -360,18 +360,6 @@ static void pci_epf_test_cmd_handler(struct 
>>> work_struct *work)
>>>                  msecs_to_jiffies(1));
>>>   }
>>> -static int pci_epf_test_notifier(struct notifier_block *nb, unsigned 
>>> long val,
>>> -                 void *data)
>>> -{
>>> -    struct pci_epf *epf = container_of(nb, struct pci_epf, nb);
>>> -    struct pci_epf_test *epf_test = epf_get_drvdata(epf);
>>> -
>>> -    queue_delayed_work(kpcitest_workqueue, &epf_test->cmd_handler,
>>> -               msecs_to_jiffies(1));
>>> -
>>> -    return NOTIFY_OK;
>>> -}
>>> -
>>>   static void pci_epf_test_unbind(struct pci_epf *epf)
>>>   {
>>>       struct pci_epf_test *epf_test = epf_get_drvdata(epf);
>>> @@ -428,6 +416,78 @@ static int pci_epf_test_set_bar(struct pci_epf 
>>> *epf)
>>>       return 0;
>>>   }
>>> +static int pci_epf_test_core_init(struct pci_epf *epf)
>>> +{
>>> +    struct pci_epf_header *header = epf->header;
>>> +    const struct pci_epc_features *epc_features;
>>> +    struct pci_epc *epc = epf->epc;
>>> +    struct device *dev = &epf->dev;
>>> +    bool msix_capable = false;
>>> +    bool msi_capable = true;
>>> +    int ret;
>>> +
>>> +    epc_features = pci_epc_get_features(epc, epf->func_no);
>>> +    if (epc_features) {
>>> +        msix_capable = epc_features->msix_capable;
>>> +        msi_capable = epc_features->msi_capable;
>>> +    }
>>> +
>>> +    ret = pci_epc_write_header(epc, epf->func_no, header);
>>> +    if (ret) {
>>> +        dev_err(dev, "Configuration header write failed\n");
>>> +        return ret;
>>> +    }
>>> +
>>> +    ret = pci_epf_test_set_bar(epf);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    if (msi_capable) {
>>> +        ret = pci_epc_set_msi(epc, epf->func_no, epf->msi_interrupts);
>>> +        if (ret) {
>>> +            dev_err(dev, "MSI configuration failed\n");
>>> +            return ret;
>>> +        }
>>> +    }
>>> +
>>> +    if (msix_capable) {
>>> +        ret = pci_epc_set_msix(epc, epf->func_no, 
>>> epf->msix_interrupts);
>>> +        if (ret) {
>>> +            dev_err(dev, "MSI-X configuration failed\n");
>>> +            return ret;
>>> +        }
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int pci_epf_test_notifier(struct notifier_block *nb, unsigned 
>>> long val,
>>> +                 void *data)
>>> +{
>>> +    struct pci_epf *epf = container_of(nb, struct pci_epf, nb);
>>> +    struct pci_epf_test *epf_test = epf_get_drvdata(epf);
>>> +    int ret;
>>> +
>>> +    switch (val) {
>>> +    case CORE_INIT:
>>> +        ret = pci_epf_test_core_init(epf);
>>> +        if (ret)
>>> +            return NOTIFY_BAD;
>>> +        break;
>>> +
>>> +    case LINK_UP:
>>> +        queue_delayed_work(kpcitest_workqueue, &epf_test->cmd_handler,
>>> +                   msecs_to_jiffies(1));
>>> +        break;
>>> +
>>> +    default:
>>> +        dev_err(&epf->dev, "Invalid EPF test notifier event\n");
>>> +        return NOTIFY_BAD;
>>> +    }
>>> +
>>> +    return NOTIFY_OK;
>>> +}
>>> +
>>>   static int pci_epf_test_alloc_space(struct pci_epf *epf)
>>>   {
>>>       struct pci_epf_test *epf_test = epf_get_drvdata(epf);
>>> @@ -496,12 +556,11 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>>>   {
>>>       int ret;
>>>       struct pci_epf_test *epf_test = epf_get_drvdata(epf);
>>> -    struct pci_epf_header *header = epf->header;
>>>       const struct pci_epc_features *epc_features;
>>>       enum pci_barno test_reg_bar = BAR_0;
>>>       struct pci_epc *epc = epf->epc;
>>> -    struct device *dev = &epf->dev;
>>>       bool linkup_notifier = false;
>>> +    bool skip_core_init = false;
>>>       bool msix_capable = false;
>>>       bool msi_capable = true;
>>> @@ -511,6 +570,7 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>>>       epc_features = pci_epc_get_features(epc, epf->func_no);
>>>       if (epc_features) {
>>>           linkup_notifier = epc_features->linkup_notifier;
>>> +        skip_core_init = epc_features->skip_core_init;
>>>           msix_capable = epc_features->msix_capable;
>>>           msi_capable = epc_features->msi_capable;
>>
>> Are these used anywhere in this function?
> Nope. I'll remove them.
> 
>>>           test_reg_bar = pci_epc_get_first_free_bar(epc_features);
>>> @@ -520,34 +580,14 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>>>       epf_test->test_reg_bar = test_reg_bar;
>>>       epf_test->epc_features = epc_features;
>>> -    ret = pci_epc_write_header(epc, epf->func_no, header);
>>> -    if (ret) {
>>> -        dev_err(dev, "Configuration header write failed\n");
>>> -        return ret;
>>> -    }
>>> -
>>>       ret = pci_epf_test_alloc_space(epf);
>>>       if (ret)
>>>           return ret;
>>> -    ret = pci_epf_test_set_bar(epf);
>>> -    if (ret)
>>> -        return ret;
>>> -
>>> -    if (msi_capable) {
>>> -        ret = pci_epc_set_msi(epc, epf->func_no, epf->msi_interrupts);
>>> -        if (ret) {
>>> -            dev_err(dev, "MSI configuration failed\n");
>>> -            return ret;
>>> -        }
>>> -    }
>>> -
>>> -    if (msix_capable) {
>>> -        ret = pci_epc_set_msix(epc, epf->func_no, 
>>> epf->msix_interrupts);
>>> -        if (ret) {
>>> -            dev_err(dev, "MSI-X configuration failed\n");
>>> +    if (!skip_core_init) {
>>> +        ret = pci_epf_test_core_init(epf);
>>> +        if (ret)
>>>               return ret;
>>> -        }
>>>       }
>>>       if (linkup_notifier) {
>>
>> This could as well be moved to pci_epf_test_core_init().
> Yes, but I would like to keep only the code that touches hardware in 
> pci_epf_test_core_init()
> to minimize the time it takes to execute it. Is there any strong reason 
> to move it? if not,
> I would prefer to leave it here in this function itself.

There is no point in scheduling a work to check for commands from host 
when the EP itself is not initialized.

Thanks
Kishon