Date:   Thu, 29 Dec 2016 11:53:36 +0000
From:   Joao Pinto <Joao.Pinto@...opsys.com>
To:     Kishon Vijay Abraham I <kishon@...com>,
        Joao Pinto <Joao.Pinto@...opsys.com>,
        Bjorn Helgaas <helgaas@...nel.org>
CC:     Christoph Hellwig <hch@...radead.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        <linux-kernel@...r.kernel.org>, <linux-pci@...r.kernel.org>,
        <nsekhar@...com>, Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH] pci: rename *host* directory to *controller*

Às 11:48 AM de 12/29/2016, Kishon Vijay Abraham I escreveu:
> 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.

I understand you idea. We can simplify it this way:

 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 -> Common files (RC and EP), specific vendor drivers for EP
and EP

BTW dwc states for DesigWare Controller.

What do you think?

Thanks

> 
> Thanks
> Kishon
> 

Powered by blists - more mailing lists