Re: [PATCH] pci: rename *host* directory to *controller*

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

Hi,

On Thursday 29 December 2016 04:08 PM, Joao Pinto wrote:
> 
> Hi,
> 
> Às 5:46 AM de 12/29/2016, Kishon Vijay Abraham I escreveu:
>> Hi,
>>
>> On Wednesday 28 December 2016 10:50 PM, Joao Pinto wrote:
>>> Às 5:17 PM de 12/28/2016, Joao Pinto escreveu:
>>>> Às 4:41 PM de 12/28/2016, Bjorn Helgaas escreveu:
>>>>> On Wed, Dec 28, 2016 at 01:57:13PM +0000, Joao Pinto wrote:
>>>>>> Às 9:22 AM de 12/28/2016, Christoph Hellwig escreveu:
>>>>>>> On Wed, Dec 28, 2016 at 01:39:37PM +0530, Kishon Vijay Abraham I wrote:
>>>>>>>> As discussed during our LPC discussions, I'm posting the rename patch
>>>>>>>> here. I'll post the rest of EP series before the next merge window.
>>>>>>>>
>>>>>>>> There might be hiccups because of this renaming but feel this is
>>>>>>>> necessary for long-term maintenance.
>>>>>>>
>>>>>>> if we do this rename it would be great to get it to Linus NOW after
>>>>>>> -rc1 as that minimizes the impact on the 4.11 merge window.
>>>>>>
>>>>>> Rename it to controller is a bit vague I thing since we have the PCI Endpoint IP
>>>>>> also. Wouldn't be better to name it rc_controller?
>>>>>
>>>>> I think Kishon's whole goal is to add PCI Endpoint IP, so he wants a
>>>>> neutral name that can cover both RC and Endpoint.
>>
>> right.
>>>>>
>>>>> I'm not a huge fan of "controller" because it feels a little bit long
>>>>> and while I suppose it technically does include the concept of the PCI
>>>>> interface of an endpoint, it still suggests more of the host side to
>>>>> me.
>>>>>
>>>>> Doesn't USB have a similar situation?  I see there's a
>>>>> drivers/usb/host/ (probably where we copied from in the first place).
>>>>> Is a USB gadget the USB analog of what you're doing?  How do they
>>>>> share code between the master/slave sides?
>>>>>
>>>>
>>>> The usb/host contains the implemnentations by the spec of the several
>>>> *hci (USB Host) and then you can have for example the USB 3.0 Designware
>>>> Host specific ops in dwc3 and for USB 2.0 in dwc2/.
>>
>> right, each IP have a separate directory in USB. I thought of doing something
>> similar for PCI but decided against it since that would involve identifying all
>> the PCI IPs used and eventually result in more directories.
>>>> For device purposes it uses the core/ and then some of the device functions
>>>> are extended from the gadget/ package in which you can use mass_storage and
>>>> other types of functions.
>>
>> That would be similar for PCI endpoint. All endpoint specific core
>> functionality will be added in drivers/pci/endpoint (see RFC [1]).
>>>>
>>>> In our case in PCI we have the core functions inside /drivers/pci and the host
>>>> mangled inside host. I suggest:
>>>>
>>>> drivers/pci
>>>>  drivers/pci/core/
>>>>  drivers/pci/core/hotplug
>>>>  drivers/pci/core/pcie
>>>>  drivers/pci/core/<all other files inside pci/ today>
>>>>  drivers/pci/host
>>>>  drivers/pci/dwc -> here would be pcie-designware and the specific vendor drivers
>>>
>>> Correction:
>>> drivers/pci/host/dwc -> here would be pcie-designware and the specific vendor
>>> drivers
>>>
>>>>  drivers/pci/<vendorN> -> here would be the drivers for vendorN controller
>>>
>>> Correction:
>>> drivers/pci/host/<vendorN> -> here would be the drivers for vendorN controller
>>>
>>>>  drivers/pci/endpoint -> common code
>>>>  drivers/pci/endpoint/dwc -> implementation of Synopsys specific endpoint ops
>>>>  drivers/pci/<vendorN> -> implementation of other vendors specific endpoint ops
>>
>> There are some parts of the dwc driver that is common to both root complex and
>> endpoint. Where should that be? I'm sure no one wants to duplicate the common
>> piece in both root complex and endpoint.
> 
> You are right, the config space is almost the same and some ops also common.
> I would suggest:
> 
> drivers/pci
>   drivers/pci/core/
>     drivers/pci/core/hotplug
>     drivers/pci/core/pcie
>     drivers/pci/core/<all other files inside pci/ today>
>   drivers/pci/dwc
>     drivers/pci/dwc/common.c -> common ops and registers between RC and endpoint
>     drivers/pci/dwc/host/
>     drivers/pci/dwc/endpoint/
> 
> What do you think?

I don't think we should have sub-directories within dwc (USB too doesn't have
sub-directories). Where should the platform specific driver be kept? For
example pci-dra7xx.c (which use dwc) has both rc and ep specific parts but the
changes are so minimal that splitting the file won't make much sense.

And such a change would also mean we create a separate directory for every
other driver present right now in pci/host.

Thanks
Kishon