Re: [PATCH] PCI: exynos: refactor exynos pcie driver

[Jaehoon Chung <jh80.chung@samsung.com>]

Hi Pankaj,

On 12/28/2016 12:10 PM, Pankaj Dubey wrote:
> Hi Jaehoon,
> 
> On 27 December 2016 at 15:48, Jaehoon Chung <jh80.chung@samsung.com> 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
> 
> 
>