Re: [PATCH 5/5] PCI: cadence: add EndPoint Controller driver for Cadence PCIe controller

Cyrille Pitchen
2017-12-15T11:50:09Z

Hi Kishon,

Le 15/12/2017 à 06:49, Kishon Vijay Abraham I a écrit :
> Hi Cyrille,
>
> On Thursday 14 December 2017 10:33 PM, Cyrille Pitchen wrote:
>> Le 13/12/2017 à 17:50, Cyrille Pitchen a écrit :
>>> Hi Kishon,
>>>
>>> Le 05/12/2017 à 10:19, Kishon Vijay Abraham I a écrit :
>>>> Hi,
>>>>
>>>> On Friday 01 December 2017 05:50 PM, Lorenzo Pieralisi wrote:
>>>>> On Thu, Nov 23, 2017 at 04:01:50PM +0100, Cyrille Pitchen wrote:
>>>>>> This patch adds support to the Cadence PCIe controller in endpoint mode.
>>>>>
>>>>> Please add a brief description to the log to describe the most salient
>>>>> features.
>>>>>
>>>>>> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@free-electrons.com>
>>>>>> ---
>>>>>> drivers/pci/cadence/Kconfig | 9 +
>>>>>> drivers/pci/cadence/Makefile | 1 +
>>>>>> drivers/pci/cadence/pcie-cadence-ep.c | 553 ++++++++++++++++++++++++++++++++++
>>>>>> 3 files changed, 563 insertions(+)
>>>>>> create mode 100644 drivers/pci/cadence/pcie-cadence-ep.c
>> [...]
>>>>>> +static int cdns_pcie_ep_write_header(struct pci_epc *epc,
>>>>>> + struct pci_epf_header *hdr)
>>>>>> +{
>>>>>> + struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
>>>>>> + struct cdns_pcie *pcie = &ep->pcie;
>>>>>> + u8 fn = 0;
>>>>>> +
>>>>>> + if (fn == 0) {
>>>>>
>>>>> I think there is some code to retrieve fn missing here.
>>>>
>>>> hmm.. the endpoint core has to send the function number which right now it's
>>>> not doing though it has the function number info in pci_epf.
>>>
>>> Would it be OK if I add a new patch in the next series adding a
>>> 'struct pcie_epf *epf' as a 2nd argument to all handlers in the
>>> 'struct pcie_epc_ops'? This way I could have access to epf->func_no as needed.
>
> I prefer we just pass the func_no as the second argument. Do you see a problem
> with that?
>>>
>>
>> Except for pci_epc_start() and pci_epc_stop(), both only called from
>> pci_epc_start_store(), I don't have trouble getting the epf value to be passed
>> as a 2nd argument to all other handlers in 'struct pcie_epc_ops'.
>
> pci_epc_start()/pci_epc_stop() is used to start/stop the end point controller
> as a whole and shouldn't need epf.
>>
>> Now my next question is: is it better to keep the 'struct pci_epc *epc' as
>> the 1st argument of all those handlers or do you prefer me to remove it as
>> the value can always be retrieved from epf->epc, since now we provide epf as
>> a new argument ?
>
> Do we really need to pass epf when func_no is all we need?
>

No, func_no alone is currently all I need so it's perfectly fine for me.
I've just wanted to ask before to be sure I implement the expected solution
before submitting :)

Thanks!

Cyrille

> Thanks
> Kishon
>

--
Cyrille Pitchen, Free Electrons
Embedded Linux and Kernel engineering