Hi Jaehoon, On 27 December 2016 at 15:48, Jaehoon Chung <jh80.chung@xxxxxxxxxxx> 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. > 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. 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@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
 |