[PATCH v2] PCI: designware: Add support 4 ATUs assignment
Lian Minghuan-B31939
B31939 at freescale.com
Wed Nov 12 02:09:43 PST 2014
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 at freescale.com> 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 at freescale.com> 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 at freescale.com>
>>>> ---
>>>> 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.
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.
And them are also ok for 2 ATUs.
For 2 ATUs, mem/io will be initialized every time read/write PCI device configuration.
- 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 at vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
More information about the linux-arm-kernel
mailing list