Re: [PATCH] Decouple CFG and IO in Designware PCIe Driver

[Pratyush Anand]

On Mon, Jun 27, 2016 at 6:38 PM, dongbo (E) <dongbo4@xxxxxxxxxx> wrote:
> Hi, all.
>
> How about exchanging the assignments of `MEMORYs' and `CFGs/IOs'?
> In other words, assign MEMEORYs to iatu0, CFGs and IOs to iatu1.
>
> Once the iatu0 is initialized to MEMORY accesses, its BASE_ADDR,
> LIMIT and TYPE is fixed. MEMORYs match with iatu0 at first, so
> they will never being treated as IOs or CFGs by mistake.

This should be acceptable, so you can send it as a formal patch.

However, other corner point for failure is still there. Suppose,
another thread tries to write on IO space when iatu1 was programmed
for CFG.

~Pratyush

>
> The number of viewpoints used remains two, it would be OK for
> platforms only have 2 viewpoints.
>
> Here is the patch:
>
> Signed-off-by: Dong Bo <dongbo4@xxxxxxxxxx>
> ---
>  drivers/pci/host/pcie-designware.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index aafd766..1bd88d9 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -599,11 +599,11 @@ static int dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
>                 va_cfg_base = pp->va_cfg1_base;
>         }
>
> -       dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
> +       dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
>                                   type, cpu_addr,
>                                   busdev, cfg_size);
>         ret = dw_pcie_cfg_read(va_cfg_base + where, size, val);
> -       dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
> +       dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
>                                   PCIE_ATU_TYPE_IO, pp->io_base,
>                                   pp->io_bus_addr, pp->io_size);
>
> @@ -636,11 +636,11 @@ static int dw_pcie_wr_other_conf(struct pcie_port *pp, struct pci_bus *bus,
>                 va_cfg_base = pp->va_cfg1_base;
>         }
>
> -       dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
> +       dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
>                                   type, cpu_addr,
>                                   busdev, cfg_size);
>         ret = dw_pcie_cfg_write(va_cfg_base + where, size, val);
> -       dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
> +       dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
>                                   PCIE_ATU_TYPE_IO, pp->io_base,
>                                   pp->io_bus_addr, pp->io_size);
>
> @@ -779,7 +779,7 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
>          * we should not program the ATU here.
>          */
>         if (!pp->ops->rd_other_conf)
> -               dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
> +               dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
>                                           PCIE_ATU_TYPE_MEM, pp->mem_base,
>                                           pp->mem_bus_addr, pp->mem_size);
>
> --
> 1.9.1
>
> On 2016/6/27 12:37, Pratyush Anand wrote:
>> On Wed, Jun 22, 2016 at 1:54 PM, dongbo (E) <dongbo4@xxxxxxxxxx> wrote:
>>> From: Dong Bo <dongbo4@xxxxxxxxxx>
>>>
>>> In designware PCIe driver, the iatu0 is used for both CFG and IO accesses.
>>> When PCIe sends CFGs to peripherals (e.g. lspci),
>>> iatu0 frequently switches between CFG and IO alternatively.
>>>
>>> If the LIMIT of MEMORY is a value between CFGs BASE_ADDR and IOs LIMIT,
>>> this probably results in a MEMORY beging matched to be an IO by mistake.
>>>
>>> Considering the following configurations:
>>> MEMORY          ->      BASE_ADDR: 0xb4100000, LIMIT: 0xb4100FFF, TYPE=mem
>>> CFG             ->      BASE_ADDR: 0xb4000000, LIMIT: 0xb4000FFF, TYPE=cfg
>>> IO              ->      BASE_ADDR: 0xFFFFFFFF, LIMIT: 0xFFFFFFFE, TYPE=io
>>> Suppose PCIe has just completed a CFG access, to switch back to IO, it set the BASE_ADDR to 0xFFFFFFFF, LIMIT 0xFFFFFFFE and TYPE to io.
>>> When another CFG access come,
>>> PCIe first set BASE_ADDR to 0xb4000000 to switch to CFG.
>>> At this moment, a MEMORY access shows up, due to `0xb4000000 <= MEMORY BASE_ADDR <= MEMORY LIMIE <= 0xFFFFFFF, it matches with iatu0.
>>> And it is treated as an IO access by mistake, then sent to perpheral.
>>>
>>
>> Hummm...This portion of driver has always been buggy.
>>
>>> This patch fixes the problem by decoupling CFG and IO, reassigning iatu2 to IO.
>>
>> But, we can not just assign IOs to iatu2.
>> IIRC then, there are atleast two platforms which have only 2
>> viewports, therefore they can not program iatu2.
>>
>> Jingoo,Bjorn: IMHO, we should modify this portion of code, since more
>> number of platforms has 4+ viewports. Probably, we can take following
>> approach:
>>
>> (1) Pass number of viewports through DT. If we have *atleast*  3
>> viewports then assign separate viewports to memory and IO,  and share
>> one with CFG0 and CFG1.
>> (2) If we can have *atleast*  4 then, we may have separate for CFG0
>> and CFG1 as well.
>>
>> (3) If we have *only* 2 then,  either we let them work as they work
>> today with bug, or may be we restrict them from using IO transactions.
>> So assign one to memory and share other for CFG0 and CFG1.
>>
>> Please let me know your opnion.
>>
>> ~Pratyush
>>
>>>
>>> Signed-off-by: Dong Bo <dongbo4@xxxxxxxxxx>
>>> ---
>>>  drivers/pci/host/pcie-designware.c | 14 +++++++-------
>>>  1 file changed, 7 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
>>> index aafd766..1a40305 100644
>>> --- a/drivers/pci/host/pcie-designware.c
>>> +++ b/drivers/pci/host/pcie-designware.c
>>> @@ -51,6 +51,7 @@
>>>  #define PCIE_ATU_VIEWPORT              0x900
>>>  #define PCIE_ATU_REGION_INBOUND                (0x1 << 31)
>>>  #define PCIE_ATU_REGION_OUTBOUND       (0x0 << 31)
>>> +#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
>>> @@ -603,9 +604,6 @@ static int dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
>>>                                   type, cpu_addr,
>>>                                   busdev, cfg_size);
>>>         ret = dw_pcie_cfg_read(va_cfg_base + where, size, val);
>>> -       dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
>>> -                                 PCIE_ATU_TYPE_IO, pp->io_base,
>>> -                                 pp->io_bus_addr, pp->io_size);
>>>
>>>         return ret;
>>>  }
>>> @@ -640,9 +638,6 @@ static int dw_pcie_wr_other_conf(struct pcie_port *pp, struct pci_bus *bus,
>>>                                   type, cpu_addr,
>>>                                   busdev, cfg_size);
>>>         ret = dw_pcie_cfg_write(va_cfg_base + where, size, val);
>>> -       dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
>>> -                                 PCIE_ATU_TYPE_IO, pp->io_base,
>>> -                                 pp->io_bus_addr, pp->io_size);
>>>
>>>         return ret;
>>>  }
>>> @@ -778,10 +773,15 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
>>>          * uses its own address translation component rather than ATU, so
>>>          * we should not program the ATU here.
>>>          */
>>> -       if (!pp->ops->rd_other_conf)
>>> +       if (!pp->ops->rd_other_conf) {
>>>                 dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
>>>                                           PCIE_ATU_TYPE_MEM, pp->mem_base,
>>>                                           pp->mem_bus_addr, pp->mem_size);
>>> +               dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX2,
>>> +                                       PCIE_ATU_TYPE_IO, pp->io_base,
>>> +                                       pp->io_bus_addr, pp->io_size);
>>> +
>>> +       }
>>>
>>>         dw_pcie_wr_own_conf(pp, PCI_BASE_ADDRESS_0, 4, 0);
>>>
>>> --
>>> 1.9.1
>>>
>>
>> .
>>
>