Re: [PATCH v2] PCI: designware: Add support 4 ATUs assignment

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Minghuan,

On Wed, Nov 12, 2014 at 3:39 PM, Lian Minghuan-B31939
<B31939@xxxxxxxxxxxxx> wrote:
> Hi Srikanth,
>
> please see my comments inline.
>
> Thanks,
> Minghuan
>
>
> On 2014年11月12日 17:01, Srikanth Thokala wrote:
>>
>> Hi Minghuan,
>>
>> On Wed, Nov 12, 2014 at 12:44 PM, Lian Minghuan-B31939
>> <B31939@xxxxxxxxxxxxx> wrote:
>>>
>>> Hi  Srikanth,
>>>
>>> Thanks for your comments, please see my reply inline.
>>>
>>>
>>> On 2014年11月12日 14:22, Srikanth Thokala wrote:
>>>>
>>>> Hi,
>>>>
>>>> On Tue, Nov 11, 2014 at 10:37 AM, Minghuan Lian
>>>> <Minghuan.Lian@xxxxxxxxxxxxx> wrote:
>>>>>
>>>>> Currently, pcie-designware.c only supports two ATUs, ATU0 is used
>>>>> for CFG0 and MEM, ATU1 is used for CFG1 and IO. There is a conflict
>>>>> when MEM and CFG0 are accessed simultaneously. The patch adds
>>>>> 'num-atus' property to PCIe dts node to describe the number of
>>>>> PCIe controller's ATUs. If num_atus is bigger than or equal to 4,
>>>>> we will change ATUs assignment: ATU0 for CFG0, ATU1 for CFG1,
>>>>> ATU2 for MEM, ATU3 for IO.
>>>>>
>>>>> Signed-off-by: Minghuan Lian <Minghuan.Lian@xxxxxxxxxxxxx>
>>>>> ---
>>>>> change log:
>>>>> v1-v2:
>>>>> 1. add the default value to property num-atus description
>>>>> 2. Use atu_idx[] instead of single values
>>>>> 3. initialize num_atus to 2
>>>>>
>>>>>    .../devicetree/bindings/pci/designware-pcie.txt    |  1 +
>>>>>    drivers/pci/host/pcie-designware.c                 | 53
>>>>> ++++++++++++++++------
>>>>>    drivers/pci/host/pcie-designware.h                 |  9 ++++
>>>>>    3 files changed, 50 insertions(+), 13 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt
>>>>> b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>>>>> index 9f4faa8..64d0533 100644
>>>>> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
>>>>> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>>>>> @@ -26,3 +26,4 @@ Optional properties:
>>>>>    - bus-range: PCI bus numbers covered (it is recommended for new
>>>>> devicetrees to
>>>>>      specify this property, to keep backwards compatibility a range of
>>>>> 0x00-0xff
>>>>>      is assumed if not present)
>>>>> +- num-atus: number of ATUs. The default value is 2 if not present.
>>>>> diff --git a/drivers/pci/host/pcie-designware.c
>>>>> b/drivers/pci/host/pcie-designware.c
>>>>> index dfed00a..46a609d 100644
>>>>> --- a/drivers/pci/host/pcie-designware.c
>>>>> +++ b/drivers/pci/host/pcie-designware.c
>>>>> @@ -48,6 +48,8 @@
>>>>>    #define PCIE_ATU_VIEWPORT              0x900
>>>>>    #define PCIE_ATU_REGION_INBOUND                (0x1 << 31)
>>>>>    #define PCIE_ATU_REGION_OUTBOUND       (0x0 << 31)
>>>>> +#define PCIE_ATU_REGION_INDEX3         (0x3 << 0)
>>>>> +#define PCIE_ATU_REGION_INDEX2         (0x2 << 0)
>>>>>    #define PCIE_ATU_REGION_INDEX1         (0x1 << 0)
>>>>>    #define PCIE_ATU_REGION_INDEX0         (0x0 << 0)
>>>>>    #define PCIE_ATU_CR1                   0x904
>>>>> @@ -346,7 +348,7 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
>>>>>           struct of_pci_range range;
>>>>>           struct of_pci_range_parser parser;
>>>>>           struct resource *cfg_res;
>>>>> -       u32 val, na, ns;
>>>>> +       u32 num_atus = 2, val, na, ns;
>>>>
>>>> I think this can be u8?
>>>
>>> [Minghuan] I define num-atus like this: num-atus = <6> (our controller
>>> supports 6 outbound ATUs)
>>> So, num_atus should be u32 type.
>>> If we use u8 type to define num_atus, the dts node should be changed to
>>> num_atus = /bits/ 8 <8>.
>>> I prefer the previous definition num-atus = <6> which is more simple and
>>> clear.
>>
>> Yes, I agree.  But as it holds only 6 possible distinct values, I
>> prefer to use u8.
>
> [Minghuan] PCIe Designware IP supports more than 6 ATUs. But our
> current PCIe controller only supports 6 outbound ATUs and 6 inbound ATUs.
> The next PCIe controller may supports more ATUs. I think u32 can be better
> compatible with hardware upgrade.
>
> I inquired dts, almost all dts property use u32 type.

I don't think this property will have values > 255, but if you think
so you could
use u16 and then u32.

> for example:
>    #address-cells = <3>;
>    #size-cells = <2>;
>    num-lanes = <1>;
>
>
>>>>>           const __be32 *addrp;
>>>>>           int i, index, ret;
>>>>>
>>>>> @@ -486,6 +488,19 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
>>>>>                   }
>>>>>           }
>>>>>
>>>>> +       of_property_read_u32(np, "num-atus", &num_atus);
>>>>
>>>> and here too?
>>>
>>> [Minghuan] please refer to the above interpretation.
>>>
>>>>> +       if (num_atus >= 4) {
>>>>> +               pp->atu_idx[ATU_TYPE_CFG0] = PCIE_ATU_REGION_INDEX0;
>>>>> +               pp->atu_idx[ATU_TYPE_CFG1] = PCIE_ATU_REGION_INDEX1;
>>>>> +               pp->atu_idx[ATU_TYPE_MEM] = PCIE_ATU_REGION_INDEX2;
>>>>> +               pp->atu_idx[ATU_TYPE_IO] = PCIE_ATU_REGION_INDEX3;
>>>>> +       } else {
>>>>> +               pp->atu_idx[ATU_TYPE_CFG0] = PCIE_ATU_REGION_INDEX0;
>>>>> +               pp->atu_idx[ATU_TYPE_MEM] = PCIE_ATU_REGION_INDEX0;
>>>>> +               pp->atu_idx[ATU_TYPE_CFG1] = PCIE_ATU_REGION_INDEX1;
>>>>> +               pp->atu_idx[ATU_TYPE_IO] = PCIE_ATU_REGION_INDEX1;
>>>>> +       }
>>>>> +
>>>>>           if (pp->ops->host_init)
>>>>>                   pp->ops->host_init(pp);
>>>>>
>>>>> @@ -511,8 +526,9 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
>>>>>
>>>>>    static void dw_pcie_prog_viewport_cfg0(struct pcie_port *pp, u32
>>>>> busdev)
>>>>>    {
>>>>> -       /* Program viewport 0 : OUTBOUND : CFG0 */
>>>>> -       dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND |
>>>>> PCIE_ATU_REGION_INDEX0,
>>>>> +       /* Program viewport : OUTBOUND : CFG0 */
>>>>> +       dw_pcie_writel_rc(pp,
>>>>> +                         PCIE_ATU_REGION_OUTBOUND |
>>>>> pp->atu_idx[ATU_TYPE_CFG0],
>>>>>                             PCIE_ATU_VIEWPORT);
>>>>>           dw_pcie_writel_rc(pp, pp->cfg0_mod_base,
>>>>> PCIE_ATU_LOWER_BASE);
>>>>>           dw_pcie_writel_rc(pp, (pp->cfg0_mod_base >> 32),
>>>>> PCIE_ATU_UPPER_BASE);
>>>>> @@ -526,8 +542,9 @@ static void dw_pcie_prog_viewport_cfg0(struct
>>>>> pcie_port *pp, u32 busdev)
>>>>>
>>>>>    static void dw_pcie_prog_viewport_cfg1(struct pcie_port *pp, u32
>>>>> busdev)
>>>>>    {
>>>>> -       /* Program viewport 1 : OUTBOUND : CFG1 */
>>>>> -       dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND |
>>>>> PCIE_ATU_REGION_INDEX1,
>>>>> +       /* Program viewport : OUTBOUND : CFG1 */
>>>>> +       dw_pcie_writel_rc(pp,
>>>>> +                         PCIE_ATU_REGION_OUTBOUND |
>>>>> pp->atu_idx[ATU_TYPE_CFG1],
>>>>>                             PCIE_ATU_VIEWPORT);
>>>>>           dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_CFG1, PCIE_ATU_CR1);
>>>>>           dw_pcie_writel_rc(pp, pp->cfg1_mod_base,
>>>>> PCIE_ATU_LOWER_BASE);
>>>>> @@ -541,8 +558,9 @@ static void dw_pcie_prog_viewport_cfg1(struct
>>>>> pcie_port *pp, u32 busdev)
>>>>>
>>>>>    static void dw_pcie_prog_viewport_mem_outbound(struct pcie_port *pp)
>>>>>    {
>>>>> -       /* Program viewport 0 : OUTBOUND : MEM */
>>>>> -       dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND |
>>>>> PCIE_ATU_REGION_INDEX0,
>>>>> +       /* Program viewport : OUTBOUND : MEM */
>>>>> +       dw_pcie_writel_rc(pp,
>>>>> +                         PCIE_ATU_REGION_OUTBOUND |
>>>>> pp->atu_idx[ATU_TYPE_MEM],
>>>>>                             PCIE_ATU_VIEWPORT);
>>>>>           dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_MEM, PCIE_ATU_CR1);
>>>>>           dw_pcie_writel_rc(pp, pp->mem_mod_base, PCIE_ATU_LOWER_BASE);
>>>>> @@ -557,8 +575,9 @@ static void
>>>>> dw_pcie_prog_viewport_mem_outbound(struct
>>>>> pcie_port *pp)
>>>>>
>>>>>    static void dw_pcie_prog_viewport_io_outbound(struct pcie_port *pp)
>>>>>    {
>>>>> -       /* Program viewport 1 : OUTBOUND : IO */
>>>>> -       dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND |
>>>>> PCIE_ATU_REGION_INDEX1,
>>>>> +       /* Program viewport : OUTBOUND : IO */
>>>>> +       dw_pcie_writel_rc(pp,
>>>>> +                         PCIE_ATU_REGION_OUTBOUND |
>>>>> pp->atu_idx[ATU_TYPE_IO],
>>>>>                             PCIE_ATU_VIEWPORT);
>>>>>           dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_IO, PCIE_ATU_CR1);
>>>>>           dw_pcie_writel_rc(pp, pp->io_mod_base, PCIE_ATU_LOWER_BASE);
>>>>> @@ -585,12 +604,14 @@ static int dw_pcie_rd_other_conf(struct pcie_port
>>>>> *pp, struct pci_bus *bus,
>>>>>                   dw_pcie_prog_viewport_cfg0(pp, busdev);
>>>>>                   ret = dw_pcie_cfg_read(pp->va_cfg0_base + address,
>>>>> where, size,
>>>>>                                   val);
>>>>> -               dw_pcie_prog_viewport_mem_outbound(pp);
>>>>> +               if (pp->atu_idx[ATU_TYPE_MEM] ==
>>>>> pp->atu_idx[ATU_TYPE_CFG0])
>>>>> +                       dw_pcie_prog_viewport_mem_outbound(pp);
>>>>>           } else {
>>>>>                   dw_pcie_prog_viewport_cfg1(pp, busdev);
>>>>>                   ret = dw_pcie_cfg_read(pp->va_cfg1_base + address,
>>>>> where, size,
>>>>>                                   val);
>>>>> -               dw_pcie_prog_viewport_io_outbound(pp);
>>>>> +               if (pp->atu_idx[ATU_TYPE_IO] ==
>>>>> pp->atu_idx[ATU_TYPE_CFG1])
>>>>> +                       dw_pcie_prog_viewport_io_outbound(pp);
>>>>>           }
>>>>>
>>>>>           return ret;
>>>>> @@ -610,12 +631,14 @@ static int dw_pcie_wr_other_conf(struct pcie_port
>>>>> *pp, struct pci_bus *bus,
>>>>>                   dw_pcie_prog_viewport_cfg0(pp, busdev);
>>>>>                   ret = dw_pcie_cfg_write(pp->va_cfg0_base + address,
>>>>> where, size,
>>>>>                                   val);
>>>>> -               dw_pcie_prog_viewport_mem_outbound(pp);
>>>>> +               if (pp->atu_idx[ATU_TYPE_MEM] ==
>>>>> pp->atu_idx[ATU_TYPE_CFG0])
>>>>> +                       dw_pcie_prog_viewport_mem_outbound(pp);
>>>>>           } else {
>>>>>                   dw_pcie_prog_viewport_cfg1(pp, busdev);
>>>>>                   ret = dw_pcie_cfg_write(pp->va_cfg1_base + address,
>>>>> where, size,
>>>>>                                   val);
>>>>> -               dw_pcie_prog_viewport_io_outbound(pp);
>>>>> +               if (pp->atu_idx[ATU_TYPE_IO] ==
>>>>> pp->atu_idx[ATU_TYPE_CFG1])
>>>>> +                       dw_pcie_prog_viewport_io_outbound(pp);
>>>>>           }
>>>>>
>>>>>           return ret;
>>>>> @@ -770,6 +793,10 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
>>>>>           u32 membase;
>>>>>           u32 memlimit;
>>>>>
>>>>> +       /* set ATUs setting for MEM and IO */
>>>>> +       dw_pcie_prog_viewport_mem_outbound(pp);
>>>>> +       dw_pcie_prog_viewport_io_outbound(pp);
>>>>> +
>>>>
>>>> I see from the code, these settings are required for the calls other
>>>> than
>>>> dw_pcie_(rd/wr)_other_conf, is it correct? If it is so, I feel this
>>>> change
>>>> is
>>>> independent of this patch and should go as a separte patch?
>>>
>>> [Minghuan] dw_pcie(rd/wr)_other_confg only calls the
>>> dw_pcie_prog_viewport_mem/io_outbound when
>>> we only use 2 ATUs.
>>> The patch is to support 4ATUs. If no the calls, ATU2(MEM) and ATU3 will
>>> not be initialized, and PCIe device driver will be broken.
>>
>> When you have only 2 ATUs (CFG0=MEM & CFG1=IO), you are calling
>> mem/io_outbound() twice with the same writes which is not the case in the
>> original driver. So, I mentioned it should go as a separate patch.
>
> [Minghuan] Sorry, I do not understand what you mean.
> How to separate patch?
> A patch is to add the following code based on original code
>
> +       /* set ATUs setting for MEM and IO */
> +       dw_pcie_prog_viewport_mem_outbound(pp);
> +       dw_pcie_prog_viewport_io_outbound(pp);
> +
>
> But why add this patch? 2 ATUs does not need them.
>
> Only 4 ATUs support needs them.

Then you may have to add a condition here, num_atus >= 4?

> And them are also ok for 2 ATUs.

You are just re-writing them anyway, so I don't see a place for them here.

So, this fragment should just work,

+++
if (num_atus >=4 ) {
  dw_pcie_prog_viewport_mem_outbound(pp);
  dw_pcie_prog_viewport_io_outbound(pp);
}
+++

Is that correct? Am I still missing?

> For 2 ATUs, mem/io will be initialized every time read/write PCI device
> configuration.

Just out of curiosity, is it really required to initialize mem/io everytime
there is a config read/write? Would it not work when initialized just once like
for the case of 4 ATUs?

- Srikanth

>
>
>
> - Srikanth
>
>>>> - Srikanth
>>>>
>>>>>           /* set the number of lanes */
>>>>>           dw_pcie_readl_rc(pp, PCIE_PORT_LINK_CONTROL, &val);
>>>>>           val &= ~PORT_LINK_MODE_MASK;
>>>>> diff --git a/drivers/pci/host/pcie-designware.h
>>>>> b/drivers/pci/host/pcie-designware.h
>>>>> index c625675..37604f9 100644
>>>>> --- a/drivers/pci/host/pcie-designware.h
>>>>> +++ b/drivers/pci/host/pcie-designware.h
>>>>> @@ -22,6 +22,14 @@
>>>>>    #define MAX_MSI_IRQS                   32
>>>>>    #define MAX_MSI_CTRLS                  (MAX_MSI_IRQS / 32)
>>>>>
>>>>> +enum ATU_TYPE {
>>>>> +       ATU_TYPE_CFG0,
>>>>> +       ATU_TYPE_CFG1,
>>>>> +       ATU_TYPE_MEM,
>>>>> +       ATU_TYPE_IO,
>>>>> +       ATU_TYPE_MAX
>>>>> +};
>>>>> +
>>>>>    struct pcie_port {
>>>>>           struct device           *dev;
>>>>>           u8                      root_bus_nr;
>>>>> @@ -53,6 +61,7 @@ struct pcie_port {
>>>>>           struct irq_domain       *irq_domain;
>>>>>           unsigned long           msi_data;
>>>>>           DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS);
>>>>> +       u8                      atu_idx[ATU_TYPE_MAX];
>>>>>    };
>>>>>
>>>>>    struct pcie_host_ops {
>>>>> --
>>>>> 1.9.1
>>>>>
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe linux-pci" 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-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux