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

[Vidya Sagar <vidyas@nvidia.com>]

On 12/5/2019 4:52 PM, Kishon Vijay Abraham I wrote:
> 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.
True. But, since this is more of preparatory work, I thought we should just have it done here itself.
Main reason being, once PERST is perceived, endpoint can't take too much initializing its core. So, I want to
keep that part as minimalistic as possible.

- Vidya Sagar

> 
> Thanks
> Kishon