From: Kishon Vijay Abraham I <kishon@ti.com>
To: Joao Pinto <Joao.Pinto@synopsys.com>, Bjorn Helgaas <helgaas@kernel.org>
Cc: Christoph Hellwig <hch@infradead.org>,
Bjorn Helgaas <bhelgaas@google.com>,
<linux-kernel@vger.kernel.org>, <linux-pci@vger.kernel.org>,
<nsekhar@ti.com>, Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [PATCH] pci: rename *host* directory to *controller*
Date: Thu, 29 Dec 2016 17:28:24 +0530 [thread overview]
Message-ID: <5864FA60.2070603@ti.com> (raw)
In-Reply-To: <1d5f9c4b-b619-36fc-082c-962c34cc3194@synopsys.com>
Hi,
On Thursday 29 December 2016 05:23 PM, Joao Pinto wrote:
> À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?
I'd like to avoid using different directory structures for different IPs. Lets
try to make it uniform.
Thanks
Kishon
next prev parent reply other threads:[~2016-12-29 11:59 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-12-28 8:09 [PATCH] pci: rename *host* directory to *controller* Kishon Vijay Abraham I
2016-12-28 9:22 ` Christoph Hellwig
2016-12-28 10:40 ` Kishon Vijay Abraham I
2016-12-28 13:57 ` Joao Pinto
2016-12-28 16:41 ` Bjorn Helgaas
2016-12-28 17:17 ` Joao Pinto
2016-12-28 17:20 ` Joao Pinto
2016-12-29 5:46 ` Kishon Vijay Abraham I
2016-12-29 10:38 ` Joao Pinto
2016-12-29 11:48 ` Kishon Vijay Abraham I
2016-12-29 11:53 ` Joao Pinto
2016-12-29 11:58 ` Kishon Vijay Abraham I [this message]
2016-12-29 12:08 ` Joao Pinto
2016-12-29 12:20 ` Kishon Vijay Abraham I
2016-12-29 12:23 ` Joao Pinto
2016-12-29 13:19 ` Joao Pinto
2016-12-30 6:23 ` Kishon Vijay Abraham I
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5864FA60.2070603@ti.com \
--to=kishon@ti.com \
--cc=Joao.Pinto@synopsys.com \
--cc=bhelgaas@google.com \
--cc=hch@infradead.org \
--cc=helgaas@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=nsekhar@ti.com \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).