From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750869AbcL1Fgh (ORCPT ); Wed, 28 Dec 2016 00:36:37 -0500 Received: from mailout2.samsung.com ([203.254.224.25]:53024 "EHLO mailout2.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750744AbcL1Fge (ORCPT ); Wed, 28 Dec 2016 00:36:34 -0500 X-AuditID: b6c32a2d-f79a76d0000074b4-2c-58634cc9f198 Subject: Re: [PATCH] PCI: exynos: refactor exynos pcie driver To: Pankaj Dubey Cc: Alim Akhtar , linux-kernel@vger.kernel.org, linux-samsung-soc , "linux-arm-kernel@lists.infradead.org" , linux-pci@vger.kernel.org, Krzysztof Kozlowski , Kukjin Kim , Jingoo Han , Bjorn Helgaas , sanath@samsung.com, Niyas Ahmed S T , CPGS From: Jaehoon Chung Message-id: Date: Wed, 28 Dec 2016 14:25:29 +0900 User-Agent: Mozilla/5.0 (X11; Linux i686; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-version: 1.0 In-reply-to: Content-type: text/plain; charset=utf-8 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA02Sa0hTYRjHe3fOzuboxNu6PSmlHDLScnnWdMdSu2gxUEMqSouy0zw5y104 ZwvrS1FROUu72G1EWbgKC6t5qQQrjFpXlOxG1DQ1SKMLNKSQom3HQN4vv+d5/3+eC4+a0F6m otWlNqcg2vgyhtKQLfcTEpMe55oLktt6pnE9Z1sorm63hRtoT+AuB0+ruOq+LwTX0XFdxfn6 Xiu5rtYzFPf8rJ/iTnXcUXD9Nb1K7kJzUMWdaGggFtGm254PKlOtz2Xy1VdQpsa6naaqpnpk +umbnk+tFdItAl8siHGCzWwvLrWVZDA5K4uyilJSk9kkNo0zMnE23ipkMNm5+UnLSstCXTJx 2/gyVyiVz0sSMzczXbS7nEKcxS45M5h1LKvXsclGnV6v1xnmrZ+vTwlJNgqWtuaHpGNQV/6y 7wixC52Ld6MoNWAD3P01RMk8GToD10KsUWuxF8H+awdJOdivgDe/H6L/jt97Divkj0sIOhua InYtDiB4fHFDmCfgDHjqblOEeSKeDRVdAWXYQOD3BLw/3xsxUHgO3BzyR0Q0zoS3lXeJMJM4 HnZXdUU0k/BqaOweUMma8fDrWIAMcxReCcffDkY0BE6Az8GjpMyx0Hj1KyF3+loF+25p3Egd 4mnguzeSzobOFy+UMk+AQX+TSuYY+ON9h8J9Aq5E8Dd4g5KDgwjeNV9RyKp50N0TIORi4+DQ cL9CLkDDgX1aWWKC1o/fRwoshhPekyM79ZDw+Uqr6jCK9YyaxzNqBs+oGWoRUY8mCw7JWiJI KQ69TuKtkstWojPbrT4UudLEpFsoWJvTjrAaMWPpjf2bCrRKfpu03dqOQE0wE2ljjrlASxfz 23cIor1IdJUJUjtKCe34CBE9yWwP3bzNWcQaUlmDPi30jKyRmULXli8o0OIS3ilsFQSHIP73 KdRR0bsQ933o5dQZJEtLvasc7gF3+vDOq3nMwpmvllbPOHS8znDm2Q1/YU3e4JLp9nWXrHzM 8m5X6if+h+Yi+J/iFjw+K+bRlurhe1PX7o0Zk6W92VD4jZoT781etCLXkZAX8D4pfbWmvOZj FS3SQU3wziw4trhwzOYoMS37x5KqNQ/6TQwpWXg2kRAl/h/x2NG3uwMAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrIIsWRmVeSWpSXmKPExsVy+t9jAd2TPskRBq1fmC0ezNvGZrGkKcPi 5SFNixVfZrJb9D9+zWxx/vwGdotNj6+xWlzeNYfN4uy842wWM87vY7J4MuURq8WirV/YLaat W8fswOuxc9Zddo8Fm0o9Nq3qZPPYvKTeo2/LKkaPz5vkAtii3GwyUhNTUosUUvOS81My89Jt lUJD3HQtlBTyEnNTbZUidH1DgpQUyhJzSoE8IwM04OAc4B6spG+X4Jaxd+sxloJXehVXHk9k bmCcr9rFyMkhIWAi8bN5AhOELSZx4d56NhBbSGApo8TJGewQ9gNGiWM7uUBsYQFbidNde8Hq RQS0JTov32PtYuQCqpnDInGrp5kdxGEWuMUssWrrDWaQKjYBHYnt346DdfAK2Enc6N4PFmcR UJVo6rsMtI2DQ1QgTOJ5oxNEiaDEj8n3WEBsToFgibdzp7ODlDALqEtMmZILEmYWkJfYvOYt 8wRGgVlIOmYhVM1CUrWAkXkVo0RqQXJBcVJ6rlFearlecWJucWleul5yfu4mRnB0PpPewXh4 l/shRgEORiUe3oBrSRFCrIllxZW5hxglOJiVRHjNvZMjhHhTEiurUovy44tKc1KLDzGaAn0x kVlKNDkfmDjySuINTcxNzI0NLMwtLU2MlMR5G2c/CxcSSE8sSc1OTS1ILYLpY+LglGpg7Ale O/XGdVsP3oa963ZFn3ijU1I++9qK/ZujBXnsNri3t9vr/or4e+zGiVMWwTd0BD9t5ZVgXt5x sKLUKyc4+CO7lM/kNVn+6SaX1+w9pnS6uL76rO4cfqWzj1+d4yp8EM/nMSNuFWtZqJ3EEmu2 9S/b3J4kTwgM42oJeD6Ba8XT6ZFfqzalKLEUZyQaajEXFScCADpDspHkAgAA X-MTR: 20000000000000000@CPGS X-CMS-MailID: 20161228052529epcas5p49caf53993ad583e405569b23fbc2a529 X-Msg-Generator: CA X-Sender-IP: 203.254.230.27 X-Local-Sender: =?UTF-8?B?7KCV7J6s7ZuIG1RpemVuIFBsYXRmb3JtIExhYihTL1fshLw=?= =?UTF-8?B?7YSwKRvsgrzshLHsoITsnpAbUzUo7LGF7J6EKS/ssYXsnoQ=?= X-Global-Sender: =?UTF-8?B?SmFlaG9vbiBDaHVuZxtUaXplbiBQbGF0Zm9ybSBMYWIuG1Nh?= =?UTF-8?B?bXN1bmcgRWxlY3Ryb25pY3MbUzUvU2VuaW9yIEVuZ2luZWVy?= X-Sender-Code: =?UTF-8?B?QzEwG1NUQUYbQzEwVjgxMTE=?= CMS-TYPE: 105P DLP-Filter: Pass X-CFilter-Loop: Reflected X-HopCount: 7 X-CMS-RootMailID: 20161223105411epcas1p3727f726e757ec6c6d7bff04a9af40077 X-RootMTR: 20161223105411epcas1p3727f726e757ec6c6d7bff04a9af40077 References: <1482490587-13611-1-git-send-email-pankaj.dubey@samsung.com> <5860E6F7.2090703@samsung.com> <79b89c49-a9de-3daa-9afa-b8f36304a226@samsung.com> <5861CE23.9060202@samsung.com> <6de4bc5b-05fb-793f-d3ab-b1c6ee19cfeb@samsung.com> <58620B5A.1060900@samsung.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Pankaj, On 12/28/2016 12:10 PM, Pankaj Dubey wrote: > Hi Jaehoon, > > On 27 December 2016 at 15:48, Jaehoon Chung wrote: >> Dear Alim, >> >> On 12/27/2016 03:34 PM, Alim Akhtar wrote: >>> Hi Jaehoon, >>> >> [snip] >>>> >>>> Ah. Right..And i'm doing the refactoring to reuse the current pci-exynos.c. >>> There is a nice refactoring patch posted by Pankaj recently @ >>> https://lkml.org/lkml/2016/12/23/73 >>> I would suggest you to rebase your work on this top. >> >> Well, i don't think so. Pankaj's patch might be good way..but i can't agree about a few point. >> If based on Pankaj's patch, it's more complex.. >> >> why put the ops callback for getting clock and mem resource? >> > > I think I replied reason behind this in reply to Jingoo. Well I will > explain with some example here. Current exynos_pcie struct contains > all mem, phy, gpio and clock resources in one place and this driver is > supported only for Exynos5440. Until this it was fine. As Jingoo > mentioned when he up streamed this driver, generic phy framework was > not ready so he used phy_base along with some additional phy related > sfr base in pcie driver itself. > > Moving ahead for adding support for PCIe module on Exynos7, Exynos5433 > or other existing Exynos (non-mainlined) and some of upcoming SoC > seeing the differences in these resources (mem, clk, regmap handles, > gpio etc.) we will endup adding following kind of code in probe to get > these resources from DT > > if (of_machine_is_compatible(exynos7420)) { > /* get certain mem resources */ > /* get certain clock resources */ > /* get certain regmap handle resources if required */ > } else if (of_machine_is_compatble(exynos5433)) { > /* get certain but may be different mem resources */ > /* get certain clock resources */ > /* get certain regmap handle resources if required */ > } else if (of_machine_is_compatible(exynosMMMM)) { > //may be something else > } > > for giving real example, exynos7420 does not uses only two clocks > "bus", "pcie_bus" there are other clocks required to be taken care in > driver. It also need to take care of pmu regmap handle and sysreg > regmap handle. > > So if we keep exynos_pcie as only struct then we will keep adding all > these resources in only one struct making it more complex. Also > resource initialization part will become complex. For the same reason > I decided to separate these rather keeping them in single struct. This > is very obvious design used in many drivers (samsung or other > vendors). Just to give one more example see pcie-qcom.c where they > also have different clocks and it is managed in same fashion. > >> If PHY generic framework is used, it's unnecessary. because it needs to get elbi and dbi resources. > > Surely elbi and dbi are fixed, but as we already have one example of > exynos5440 which used block_base (additional SFR base other than phy > base), I can see some of existing and upcoming SoC will need more SFR > base than these two, it all depends on how h/w engineers design these > modules and distribute SFR access. > >> clock resources("pcie" and "pcie_bus") are general things. >> > > With current Exynos SoCs which have PCIe hardware modules I can see > this will not remain uniform across various SoCs, some of them require > more clocks to be handled by PCIe driver. > >> If Pankaj's patch is applied, also need to make the exynos5433_pcie_* callback functions? >> It doesn't make sense. >> > > Adding exynos5433_pcie_ops struct and hooks specific to the exynos5433 > is a small overhead compared to the flexibility it provides. This will > make current driver flexibility with less > of_machine_is_compatible(...) kind of conditional statement, less > complex exynos_pcie struct. > For the hooks of exynos_pcie_ops also if two different Exynos SoC > shares same mem, clock and other resources the actual hooks will point > to same functions so no overhead of implementing these for each SoC. > Only separate implementation will be required if they differ in these > resources. This approach also very common across various drivers > nothing special here. > > I hope I am clear in explaining the intention and pros and cons of > both approaches. Yep, clear..Well, My opinion is also my preference. (Maybe i think that you also know a little what my meaning is. ) There is no objection that we want to support the other Exynos variants. :) This discussion is for how to support better than now. So I will send the patch based on your patch...then we can discuss more. how about? I think we can maintain more better than now. As you already know, current exynos-pcie has to modify for supporting other. > >> I want to know maintainer's opinion..we can just touch a little for supporting All Exynos SoCs. >> > > I also have same intention but design should be flexible to adopt > future SoC at least some of them which we are seeing will be supported > soon and we can see the differences. Sure, I will wait for your patch. And You can also check my patches.. :) Best Regards, Jaehoon Chung > > Thanks, > Pankaj Dubey > >> Best Regards, >> Jaehoon Chung >> >>> >>>> Maybe..Today or Tomorrow..I will send the patches..At that time, could you also check them? >>>> Any comments might be helpful to me! :) >>>> >>> Will wait for you patches :-) >>> >>>> Best Regards, >>>> Jaehoon Chung >>>> >> [snip] >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > >