Message-ID: <CAGcde9HTd7Gxc89ZaYW8z-pc91ps3kwHx3KsEpNRLcoHO9S-zw@mail.gmail.com>
Date:   Wed, 28 Dec 2016 08:40:30 +0530
From:   Pankaj Dubey <pankaj.dubey@...sung.com>
To:     Jaehoon Chung <jh80.chung@...sung.com>
Cc:     Alim Akhtar <alim.akhtar@...sung.com>,
        linux-kernel@...r.kernel.org,
        linux-samsung-soc <linux-samsung-soc@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>, linux-pci@...r.kernel.org,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Kukjin Kim <kgene@...nel.org>,
        Jingoo Han <jingoohan1@...il.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>, sanath@...sung.com,
        Niyas Ahmed S T <niyas.ahmed@...sung.com>,
        CPGS <cpgs@...sung.com>
Subject: Re: [PATCH] PCI: exynos: refactor exynos pcie driver

Hi Jaehoon,

On 27 December 2016 at 15:48, Jaehoon Chung <jh80.chung@...sung.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.

> 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@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists