[v6,5/6] PCI: uniphier: Add iATU register support
diff mbox series

Message ID 1596795922-705-6-git-send-email-hayashi.kunihiko@socionext.com
State Changes Requested
Delegated to: Lorenzo Pieralisi
Headers show
Series
  • PCI: uniphier: Add features for UniPhier PCIe host controller
Related show

Commit Message

Kunihiko Hayashi Aug. 7, 2020, 10:25 a.m. UTC
This gets iATU register area from reg property. In Synopsys DWC version
4.80 or later, since iATU register area is separated from core register
area, this area is necessary to get from DT independently.

Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
---
 drivers/pci/controller/dwc/pcie-uniphier.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Rob Herring Aug. 17, 2020, 4:48 p.m. UTC | #1
On Fri, Aug 7, 2020 at 4:25 AM Kunihiko Hayashi
<hayashi.kunihiko@socionext.com> wrote:
>
> This gets iATU register area from reg property. In Synopsys DWC version
> 4.80 or later, since iATU register area is separated from core register
> area, this area is necessary to get from DT independently.
>
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> ---
>  drivers/pci/controller/dwc/pcie-uniphier.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c
> index 55a7166..93ef608 100644
> --- a/drivers/pci/controller/dwc/pcie-uniphier.c
> +++ b/drivers/pci/controller/dwc/pcie-uniphier.c
> @@ -471,6 +471,11 @@ static int uniphier_pcie_probe(struct platform_device *pdev)
>         if (IS_ERR(priv->pci.dbi_base))
>                 return PTR_ERR(priv->pci.dbi_base);
>
> +       priv->pci.atu_base =
> +               devm_platform_ioremap_resource_byname(pdev, "atu");
> +       if (IS_ERR(priv->pci.atu_base))
> +               priv->pci.atu_base = NULL;

Keystone has the same 'atu' resource setup. Please move its code to
the DW core and use that.

Rob
Kunihiko Hayashi Aug. 21, 2020, 7:05 a.m. UTC | #2
On 2020/08/18 1:48, Rob Herring wrote:
> On Fri, Aug 7, 2020 at 4:25 AM Kunihiko Hayashi
> <hayashi.kunihiko@socionext.com> wrote:
>>
>> This gets iATU register area from reg property. In Synopsys DWC version
>> 4.80 or later, since iATU register area is separated from core register
>> area, this area is necessary to get from DT independently.
>>
>> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
>> ---
>>   drivers/pci/controller/dwc/pcie-uniphier.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c
>> index 55a7166..93ef608 100644
>> --- a/drivers/pci/controller/dwc/pcie-uniphier.c
>> +++ b/drivers/pci/controller/dwc/pcie-uniphier.c
>> @@ -471,6 +471,11 @@ static int uniphier_pcie_probe(struct platform_device *pdev)
>>          if (IS_ERR(priv->pci.dbi_base))
>>                  return PTR_ERR(priv->pci.dbi_base);
>>
>> +       priv->pci.atu_base =
>> +               devm_platform_ioremap_resource_byname(pdev, "atu");
>> +       if (IS_ERR(priv->pci.atu_base))
>> +               priv->pci.atu_base = NULL;
> 
> Keystone has the same 'atu' resource setup. Please move its code to
> the DW core and use that.

There are some platforms that pci.atu_base is set by other way.
The 'atu' code shouldn't be conflicted with the following existing code.

   drivers/pci/controller/dwc/pci-keystone.c:              atu_base = devm_platform_ioremap_resource_byname(pdev, "atu");
   drivers/pci/controller/dwc/pci-keystone.c:              pci->atu_base = atu_base;
   drivers/pci/controller/dwc/pcie-designware.c:                   pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
   drivers/pci/controller/dwc/pcie-intel-gw.c:     pci->atu_base = pci->dbi_base + data->pcie_atu_offset;
   drivers/pci/controller/dwc/pcie-tegra194.c:     pci->atu_base = devm_ioremap_resource(dev, atu_dma_res);

So I'm not sure where to move the code in the DW core.
Is there any idea?

Thank you,

---
Best Regards
Kunihiko Hayashi
Rob Herring Sept. 3, 2020, 10:12 p.m. UTC | #3
On Fri, Aug 21, 2020 at 1:05 AM Kunihiko Hayashi
<hayashi.kunihiko@socionext.com> wrote:
>
> On 2020/08/18 1:48, Rob Herring wrote:
> > On Fri, Aug 7, 2020 at 4:25 AM Kunihiko Hayashi
> > <hayashi.kunihiko@socionext.com> wrote:
> >>
> >> This gets iATU register area from reg property. In Synopsys DWC version
> >> 4.80 or later, since iATU register area is separated from core register
> >> area, this area is necessary to get from DT independently.
> >>
> >> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> >> ---
> >>   drivers/pci/controller/dwc/pcie-uniphier.c | 5 +++++
> >>   1 file changed, 5 insertions(+)
> >>
> >> diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c
> >> index 55a7166..93ef608 100644
> >> --- a/drivers/pci/controller/dwc/pcie-uniphier.c
> >> +++ b/drivers/pci/controller/dwc/pcie-uniphier.c
> >> @@ -471,6 +471,11 @@ static int uniphier_pcie_probe(struct platform_device *pdev)
> >>          if (IS_ERR(priv->pci.dbi_base))
> >>                  return PTR_ERR(priv->pci.dbi_base);
> >>
> >> +       priv->pci.atu_base =
> >> +               devm_platform_ioremap_resource_byname(pdev, "atu");
> >> +       if (IS_ERR(priv->pci.atu_base))
> >> +               priv->pci.atu_base = NULL;
> >
> > Keystone has the same 'atu' resource setup. Please move its code to
> > the DW core and use that.
>
> There are some platforms that pci.atu_base is set by other way.
> The 'atu' code shouldn't be conflicted with the following existing code.

No, it's not a conflict but needless duplication.

>    drivers/pci/controller/dwc/pci-keystone.c:              atu_base = devm_platform_ioremap_resource_byname(pdev, "atu");
>    drivers/pci/controller/dwc/pci-keystone.c:              pci->atu_base = atu_base;
>    drivers/pci/controller/dwc/pcie-designware.c:                   pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
>    drivers/pci/controller/dwc/pcie-intel-gw.c:     pci->atu_base = pci->dbi_base + data->pcie_atu_offset;

This one should have had an 'atu' region in DT.

>    drivers/pci/controller/dwc/pcie-tegra194.c:     pci->atu_base = devm_ioremap_resource(dev, atu_dma_res);

Unfortunately, a different name was used. That is the mess which is
the DW PCI controller.

>
> So I'm not sure where to move the code in the DW core.
> Is there any idea?

You just need this and then remove the keystone code:

diff --git a/drivers/pci/controller/dwc/pcie-designware.c
b/drivers/pci/controller/dwc/pcie-designware.c
index b723e0cc41fb..680084467447 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -556,6 +556,8 @@ void dw_pcie_setup(struct dw_pcie *pci)
                                       dw_pcie_iatu_unroll_enabled(pci))) {
                pci->iatu_unroll_enabled = true;
                if (!pci->atu_base)
+                       pci->atu_base =
devm_platform_ioremap_resource_byname(pdev, "atu");
+               if (IS_ERR(pci->atu_base))
                        pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
        }
        dev_dbg(pci->dev, "iATU unroll: %s\n", pci->iatu_unroll_enabled ?
Kunihiko Hayashi Sept. 7, 2020, 4:09 p.m. UTC | #4
Hi Rob,

On 2020/09/04 7:12, Rob Herring wrote:
> On Fri, Aug 21, 2020 at 1:05 AM Kunihiko Hayashi
> <hayashi.kunihiko@socionext.com> wrote:
>>
>> On 2020/08/18 1:48, Rob Herring wrote:
>>> On Fri, Aug 7, 2020 at 4:25 AM Kunihiko Hayashi
>>> <hayashi.kunihiko@socionext.com> wrote:
>>>>
>>>> This gets iATU register area from reg property. In Synopsys DWC version
>>>> 4.80 or later, since iATU register area is separated from core register
>>>> area, this area is necessary to get from DT independently.
>>>>
>>>> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
>>>> ---
>>>>    drivers/pci/controller/dwc/pcie-uniphier.c | 5 +++++
>>>>    1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c
>>>> index 55a7166..93ef608 100644
>>>> --- a/drivers/pci/controller/dwc/pcie-uniphier.c
>>>> +++ b/drivers/pci/controller/dwc/pcie-uniphier.c
>>>> @@ -471,6 +471,11 @@ static int uniphier_pcie_probe(struct platform_device *pdev)
>>>>           if (IS_ERR(priv->pci.dbi_base))
>>>>                   return PTR_ERR(priv->pci.dbi_base);
>>>>
>>>> +       priv->pci.atu_base =
>>>> +               devm_platform_ioremap_resource_byname(pdev, "atu");
>>>> +       if (IS_ERR(priv->pci.atu_base))
>>>> +               priv->pci.atu_base = NULL;
>>>
>>> Keystone has the same 'atu' resource setup. Please move its code to
>>> the DW core and use that.
>>
>> There are some platforms that pci.atu_base is set by other way.
>> The 'atu' code shouldn't be conflicted with the following existing code.
> 
> No, it's not a conflict but needless duplication.

I see.

>>     drivers/pci/controller/dwc/pci-keystone.c:              atu_base = devm_platform_ioremap_resource_byname(pdev, "atu");
>>     drivers/pci/controller/dwc/pci-keystone.c:              pci->atu_base = atu_base;
>>     drivers/pci/controller/dwc/pcie-designware.c:                   pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
>>     drivers/pci/controller/dwc/pcie-intel-gw.c:     pci->atu_base = pci->dbi_base + data->pcie_atu_offset;
> 
> This one should have had an 'atu' region in DT.
> 
>>     drivers/pci/controller/dwc/pcie-tegra194.c:     pci->atu_base = devm_ioremap_resource(dev, atu_dma_res);
> 
> Unfortunately, a different name was used. That is the mess which is
> the DW PCI controller.

Okay, this has already set atu_base, so ignore it for this patch.
  
>>
>> So I'm not sure where to move the code in the DW core.
>> Is there any idea?
> 
> You just need this and then remove the keystone code:
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c
> b/drivers/pci/controller/dwc/pcie-designware.c
> index b723e0cc41fb..680084467447 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -556,6 +556,8 @@ void dw_pcie_setup(struct dw_pcie *pci)
>                                         dw_pcie_iatu_unroll_enabled(pci))) {
>                  pci->iatu_unroll_enabled = true;
>                  if (!pci->atu_base)
> +                       pci->atu_base =
> devm_platform_ioremap_resource_byname(pdev, "atu");
> +               if (IS_ERR(pci->atu_base))
>                          pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
>          }
>          dev_dbg(pci->dev, "iATU unroll: %s\n", pci->iatu_unroll_enabled ?
> 

I've got it. I think I need to add a bit of code to this though,
I'll try to apply this and remove the duplicate code.

I'll separate this from other PER/AER patches and send new series.

Thank you,

---
Best Regards
Kunihiko Hayashi

Patch
diff mbox series

diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c
index 55a7166..93ef608 100644
--- a/drivers/pci/controller/dwc/pcie-uniphier.c
+++ b/drivers/pci/controller/dwc/pcie-uniphier.c
@@ -471,6 +471,11 @@  static int uniphier_pcie_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->pci.dbi_base))
 		return PTR_ERR(priv->pci.dbi_base);
 
+	priv->pci.atu_base =
+		devm_platform_ioremap_resource_byname(pdev, "atu");
+	if (IS_ERR(priv->pci.atu_base))
+		priv->pci.atu_base = NULL;
+
 	priv->base = devm_platform_ioremap_resource_byname(pdev, "link");
 	if (IS_ERR(priv->base))
 		return PTR_ERR(priv->base);