diff mbox

[2/3] PCI: designware: use untranslated address while programming ATU

Message ID 1403719007-9352-3-git-send-email-kishon@ti.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Kishon Vijay Abraham I June 25, 2014, 5:56 p.m. UTC
In DRA7, the cpu sees 32bit address, but the pcie controller can see only 28bit
address. So whenever the cpu issues a read/write request, the 4 most
significant bits are used by L3 to determine the target controller.
For example, the cpu reserves 0x2000_0000 - 0x2FFF_FFFF for PCIe controller but
the PCIe controller will see only (0x000_0000 - 0xFFF_FFF). So for programming
the outbound translation window the *base* should be programmed as 0x000_0000.
Whenever we try to write to say 0x2000_0000, it will be translated to whatever
we have programmed in the translation window with base as 0x000_0000.

This is needed when the dt node is modelled something like below
axi {
	compatible = "simple-bus";
	#size-cells = <1>;
	#address-cells = <1>;
	ranges = <0x0        0x20000000 0x10000000 // 28-bit bus
		  0x51000000 0x51000000 0x3000>;
	pcie@51000000 {
		reg = <0x1000 0x2000>, <0x51002000 0x14c>, <0x51000000 0x2000>;
		reg-names = "config", "ti_conf", "rc_dbics";
		#address-cells = <3>;
		#size-cells = <2>;
		ranges = <0x81000000 0 0          0x03000 0 0x00010000
			  0x82000000 0 0x20013000 0x13000 0 0xffed000>;
	};
};

Here the CPU address for configuration space is 0x20013000 and the controller
address for configuration space is 0x13000. The controller address should be
used while programming the ATU (in order for translation to happen properly in
DRA7xx).

Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Mohit Kumar <mohit.kumar@st.com>
Cc: Jingoo Han <jg1.han@samsung.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/host/pcie-designware.c |   49 ++++++++++++++++++++++++++++--------
 drivers/pci/host/pcie-designware.h |    4 +++
 2 files changed, 42 insertions(+), 11 deletions(-)

Comments

Pratyush Anand June 26, 2014, 5:37 a.m. UTC | #1
Hi Kishon,

 Few things, if you can help me to understand:

On Wed, Jun 25, 2014 at 11:26 PM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
> In DRA7, the cpu sees 32bit address, but the pcie controller can see only 28bit
> address. So whenever the cpu issues a read/write request, the 4 most
> significant bits are used by L3 to determine the target controller.
> For example, the cpu reserves 0x2000_0000 - 0x2FFF_FFFF for PCIe controller but
> the PCIe controller will see only (0x000_0000 - 0xFFF_FFF). So for programming
> the outbound translation window the *base* should be programmed as 0x000_0000.
> Whenever we try to write to say 0x2000_0000, it will be translated to whatever
> we have programmed in the translation window with base as 0x000_0000.
>
> This is needed when the dt node is modelled something like below
> axi {
>         compatible = "simple-bus";
>         #size-cells = <1>;
>         #address-cells = <1>;
>         ranges = <0x0        0x20000000 0x10000000 // 28-bit bus
>                   0x51000000 0x51000000 0x3000>;
>         pcie@51000000 {
>                 reg = <0x1000 0x2000>, <0x51002000 0x14c>, <0x51000000 0x2000>;
>                 reg-names = "config", "ti_conf", "rc_dbics";

So for DRA7, config base which will be coming from reg property should
be 0x1000 and size 0x2000, no?

>                 #address-cells = <3>;
>                 #size-cells = <2>;
>                 ranges = <0x81000000 0 0          0x03000 0 0x00010000

range type 0x81000000 tells that it is IO

>                           0x82000000 0 0x20013000 0x13000 0 0xffed000>;

range type 0x81000000 tells that it is mem


>         };
> };
>
> Here the CPU address for configuration space is 0x20013000 and the controller
> address for configuration space is 0x13000. The controller address should
be

If above understanding is correct then:

Aren't these addresses(0x20013000  and 0x13000) from mem space
instead of configuration space.

If yes, then how can you get two addresses (CPU and Controller address)
from reg property for configuration space?

~Pratyush
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kishon Vijay Abraham I June 26, 2014, 6:10 a.m. UTC | #2
Hi Pratyush,

On Thursday 26 June 2014 11:07 AM, Pratyush Anand wrote:
> Hi Kishon,
> 
>  Few things, if you can help me to understand:
> 
> On Wed, Jun 25, 2014 at 11:26 PM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>> In DRA7, the cpu sees 32bit address, but the pcie controller can see only 28bit
>> address. So whenever the cpu issues a read/write request, the 4 most
>> significant bits are used by L3 to determine the target controller.
>> For example, the cpu reserves 0x2000_0000 - 0x2FFF_FFFF for PCIe controller but
>> the PCIe controller will see only (0x000_0000 - 0xFFF_FFF). So for programming
>> the outbound translation window the *base* should be programmed as 0x000_0000.
>> Whenever we try to write to say 0x2000_0000, it will be translated to whatever
>> we have programmed in the translation window with base as 0x000_0000.
>>
>> This is needed when the dt node is modelled something like below
>> axi {
>>         compatible = "simple-bus";
>>         #size-cells = <1>;
>>         #address-cells = <1>;
>>         ranges = <0x0        0x20000000 0x10000000 // 28-bit bus
>>                   0x51000000 0x51000000 0x3000>;
>>         pcie@51000000 {
>>                 reg = <0x1000 0x2000>, <0x51002000 0x14c>, <0x51000000 0x2000>;
>>                 reg-names = "config", "ti_conf", "rc_dbics";
> 
> So for DRA7, config base which will be coming from reg property should
> be 0x1000 and size 0x2000, no?

right. The first element in 'reg' and 'reg-names' specify exactly that.
> 
>>                 #address-cells = <3>;
>>                 #size-cells = <2>;
>>                 ranges = <0x81000000 0 0          0x03000 0 0x00010000
> 
> range type 0x81000000 tells that it is IO
> 
>>                           0x82000000 0 0x20013000 0x13000 0 0xffed000>;
> 
> range type 0x81000000 tells that it is mem
> 
> 
>>         };
>> };
>>
>> Here the CPU address for configuration space is 0x20013000 and the controller
>> address for configuration space is 0x13000. The controller address should
> be
> 
> If above understanding is correct then:
> 
> Aren't these addresses(0x20013000  and 0x13000) from mem space
> instead of configuration space.

Sorry. I didn't get you. Configuration space is different from mem space and IO
space. We specify only the configuration space in "reg", the IO space and
memory space should be specified in ranges.

In my case
configuration space range: 0x20001000 - 0x20002fff
IO space range:            0x20003000 - 0x20012fff
Mem space range:           0x20013000 - 0x2fffffff

Here only the configuration space is obtained from 'reg' and 'IO' and 'MEM'
space is obtained from ranges.
> 
> If yes, then how can you get two addresses (CPU and Controller address)
> from reg property for configuration space?

I used platform_get_resource_byname() to get CPU address and of_get_address()
to get the untranslated controller address.

Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pratyush ANAND June 26, 2014, 6:33 a.m. UTC | #3
Hi Kishon,

On Thu, Jun 26, 2014 at 02:10:02PM +0800, Kishon Vijay Abraham I wrote:
> Hi Pratyush,
> 
> On Thursday 26 June 2014 11:07 AM, Pratyush Anand wrote:
> > Hi Kishon,
> > 
> >  Few things, if you can help me to understand:
> > 
> > On Wed, Jun 25, 2014 at 11:26 PM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
> >> In DRA7, the cpu sees 32bit address, but the pcie controller can see only 28bit
> >> address. So whenever the cpu issues a read/write request, the 4 most
> >> significant bits are used by L3 to determine the target controller.
> >> For example, the cpu reserves 0x2000_0000 - 0x2FFF_FFFF for PCIe controller but
> >> the PCIe controller will see only (0x000_0000 - 0xFFF_FFF). So for programming
> >> the outbound translation window the *base* should be programmed as 0x000_0000.
> >> Whenever we try to write to say 0x2000_0000, it will be translated to whatever
> >> we have programmed in the translation window with base as 0x000_0000.
> >>
> >> This is needed when the dt node is modelled something like below
> >> axi {
> >>         compatible = "simple-bus";
> >>         #size-cells = <1>;
> >>         #address-cells = <1>;
> >>         ranges = <0x0        0x20000000 0x10000000 // 28-bit bus
> >>                   0x51000000 0x51000000 0x3000>;
> >>         pcie@51000000 {
> >>                 reg = <0x1000 0x2000>, <0x51002000 0x14c>, <0x51000000 0x2000>;
> >>                 reg-names = "config", "ti_conf", "rc_dbics";
> > 
> > So for DRA7, config base which will be coming from reg property should
> > be 0x1000 and size 0x2000, no?
> 
> right. The first element in 'reg' and 'reg-names' specify exactly that.
> > 
> >>                 #address-cells = <3>;
> >>                 #size-cells = <2>;
> >>                 ranges = <0x81000000 0 0          0x03000 0 0x00010000
> > 
> > range type 0x81000000 tells that it is IO
> > 
> >>                           0x82000000 0 0x20013000 0x13000 0 0xffed000>;
> > 
> > range type 0x81000000 tells that it is mem
> > 
> > 
> >>         };
> >> };
> >>
> >> Here the CPU address for configuration space is 0x20013000 and the controller
> >> address for configuration space is 0x13000. The controller address should
> > be
> > 
> > If above understanding is correct then:
> > 
> > Aren't these addresses(0x20013000  and 0x13000) from mem space
> > instead of configuration space.
> 
> Sorry. I didn't get you. Configuration space is different from mem space and IO
> space. We specify only the configuration space in "reg", the IO space and
> memory space should be specified in ranges.
> 
> In my case
> configuration space range: 0x20001000 - 0x20002fff
> IO space range:            0x20003000 - 0x20012fff
> Mem space range:           0x20013000 - 0x2fffffff
> 
> Here only the configuration space is obtained from 'reg' and 'IO' and 'MEM'
> space is obtained from ranges.
> > 
> > If yes, then how can you get two addresses (CPU and Controller address)
> > from reg property for configuration space?
> 
> I used platform_get_resource_byname() to get CPU address and of_get_address()
> to get the untranslated controller address.

This is what I am not able to understand that how does
platform_get_resource_byname gives correct CPU address from reg =
<0x1000 0x2000>?

cfg_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config");

how cfg_res->start is now 0x20001000? Shouldn't cfg_res->start be
0x1000?

What am I missing?

Thanks for explaining.

~Pratyush



> Thanks
> Kishon
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kishon Vijay Abraham I June 26, 2014, 8:24 a.m. UTC | #4
Hi Pratyush,

On Thursday 26 June 2014 12:03 PM, Pratyush Anand wrote:
> Hi Kishon,
> 
> On Thu, Jun 26, 2014 at 02:10:02PM +0800, Kishon Vijay Abraham I wrote:
>> Hi Pratyush,
>>
>> On Thursday 26 June 2014 11:07 AM, Pratyush Anand wrote:
>>> Hi Kishon,
>>>
>>>  Few things, if you can help me to understand:
>>>
>>> On Wed, Jun 25, 2014 at 11:26 PM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>>> In DRA7, the cpu sees 32bit address, but the pcie controller can see only 28bit
>>>> address. So whenever the cpu issues a read/write request, the 4 most
>>>> significant bits are used by L3 to determine the target controller.
>>>> For example, the cpu reserves 0x2000_0000 - 0x2FFF_FFFF for PCIe controller but
>>>> the PCIe controller will see only (0x000_0000 - 0xFFF_FFF). So for programming
>>>> the outbound translation window the *base* should be programmed as 0x000_0000.
>>>> Whenever we try to write to say 0x2000_0000, it will be translated to whatever
>>>> we have programmed in the translation window with base as 0x000_0000.
>>>>
>>>> This is needed when the dt node is modelled something like below
>>>> axi {
>>>>         compatible = "simple-bus";
>>>>         #size-cells = <1>;
>>>>         #address-cells = <1>;
>>>>         ranges = <0x0        0x20000000 0x10000000 // 28-bit bus
>>>>                   0x51000000 0x51000000 0x3000>;
>>>>         pcie@51000000 {
>>>>                 reg = <0x1000 0x2000>, <0x51002000 0x14c>, <0x51000000 0x2000>;
>>>>                 reg-names = "config", "ti_conf", "rc_dbics";
>>>
>>> So for DRA7, config base which will be coming from reg property should
>>> be 0x1000 and size 0x2000, no?
>>
>> right. The first element in 'reg' and 'reg-names' specify exactly that.
>>>
>>>>                 #address-cells = <3>;
>>>>                 #size-cells = <2>;
>>>>                 ranges = <0x81000000 0 0          0x03000 0 0x00010000
>>>
>>> range type 0x81000000 tells that it is IO
>>>
>>>>                           0x82000000 0 0x20013000 0x13000 0 0xffed000>;
>>>
>>> range type 0x81000000 tells that it is mem
>>>
>>>
>>>>         };
>>>> };
>>>>
>>>> Here the CPU address for configuration space is 0x20013000 and the controller
>>>> address for configuration space is 0x13000. The controller address should
>>> be
>>>
>>> If above understanding is correct then:
>>>
>>> Aren't these addresses(0x20013000  and 0x13000) from mem space
>>> instead of configuration space.
>>
>> Sorry. I didn't get you. Configuration space is different from mem space and IO
>> space. We specify only the configuration space in "reg", the IO space and
>> memory space should be specified in ranges.
>>
>> In my case
>> configuration space range: 0x20001000 - 0x20002fff
>> IO space range:            0x20003000 - 0x20012fff
>> Mem space range:           0x20013000 - 0x2fffffff
>>
>> Here only the configuration space is obtained from 'reg' and 'IO' and 'MEM'
>> space is obtained from ranges.
>>>
>>> If yes, then how can you get two addresses (CPU and Controller address)
>>> from reg property for configuration space?
>>
>> I used platform_get_resource_byname() to get CPU address and of_get_address()
>> to get the untranslated controller address.
> 
> This is what I am not able to understand that how does
> platform_get_resource_byname gives correct CPU address from reg =
> <0x1000 0x2000>?
> 
> cfg_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config");
> 
> how cfg_res->start is now 0x20001000? Shouldn't cfg_res->start be
> 0x1000?
> 
> What am I missing?

IIUC, converting to the cpu address happens long back when the platform device
is created from dt node.

See if the following makes sense to you.

starting from drivers/of/platform.c
of_device_alloc() -> of_address_to_resource() -> __of_address_to_resource() ->
of_translate_address() -> __of_translate_address() -> of_translate_one() ->
of_bus_default_translate()

So the 'resource' in 'struct platform_device' already holds the translated
address. platform_get_resource_byname() just returns the resource from this
platform device.

Cheers
Kishon
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kishon Vijay Abraham I July 8, 2014, 12:31 p.m. UTC | #5
Hi Mohit, Jingoo,

On Wednesday 25 June 2014 11:26 PM, Kishon Vijay Abraham I wrote:
> In DRA7, the cpu sees 32bit address, but the pcie controller can see only 28bit
> address. So whenever the cpu issues a read/write request, the 4 most
> significant bits are used by L3 to determine the target controller.
> For example, the cpu reserves 0x2000_0000 - 0x2FFF_FFFF for PCIe controller but
> the PCIe controller will see only (0x000_0000 - 0xFFF_FFF). So for programming
> the outbound translation window the *base* should be programmed as 0x000_0000.
> Whenever we try to write to say 0x2000_0000, it will be translated to whatever
> we have programmed in the translation window with base as 0x000_0000.
> 
> This is needed when the dt node is modelled something like below
> axi {
> 	compatible = "simple-bus";
> 	#size-cells = <1>;
> 	#address-cells = <1>;
> 	ranges = <0x0        0x20000000 0x10000000 // 28-bit bus
> 		  0x51000000 0x51000000 0x3000>;
> 	pcie@51000000 {
> 		reg = <0x1000 0x2000>, <0x51002000 0x14c>, <0x51000000 0x2000>;
> 		reg-names = "config", "ti_conf", "rc_dbics";
> 		#address-cells = <3>;
> 		#size-cells = <2>;
> 		ranges = <0x81000000 0 0          0x03000 0 0x00010000
> 			  0x82000000 0 0x20013000 0x13000 0 0xffed000>;
> 	};
> };
> 
> Here the CPU address for configuration space is 0x20013000 and the controller
> address for configuration space is 0x13000. The controller address should be
> used while programming the ATU (in order for translation to happen properly in
> DRA7xx).

Are you okay with this patch? Can you give your Reviewed-by/Acked-by?

Cheers
Kishon
> 
> Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Mohit Kumar <mohit.kumar@st.com>
> Cc: Jingoo Han <jg1.han@samsung.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/host/pcie-designware.c |   49 ++++++++++++++++++++++++++++--------
>  drivers/pci/host/pcie-designware.h |    4 +++
>  2 files changed, 42 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index 0b7b455..11aba6d 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -401,8 +401,15 @@ 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;
> -	int i;
> +	u32 val, na, ns;
> +	const __be32 *addrp;
> +	int i, index;
> +
> +	/* Find the address cell size and the number of cells in order to get
> +	 * the untranslated address.
> +	 */
> +	of_property_read_u32(np, "#address-cells", &na);
> +	ns = of_n_size_cells(np);
>  
>  	cfg_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config");
>  	if (cfg_res) {
> @@ -410,6 +417,12 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
>  		pp->config.cfg1_size = resource_size(cfg_res)/2;
>  		pp->cfg0_base = cfg_res->start;
>  		pp->cfg1_base = cfg_res->start + pp->config.cfg0_size;
> +
> +		/* Find the untranslated configuration space address */
> +		index = of_property_match_string(np, "reg-names", "config");
> +		addrp = of_get_address(np, index, false, false);
> +		pp->cfg0_mod_addr = of_read_number(addrp, ns);
> +		pp->cfg1_mod_addr = pp->cfg0_mod_addr + pp->config.cfg0_size;
>  	} else {
>  		dev_err(pp->dev, "missing *config* reg space\n");
>  	}
> @@ -435,12 +448,20 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
>  			pp->config.io_size = resource_size(&pp->io);
>  			pp->config.io_bus_addr = range.pci_addr;
>  			pp->io_base = range.cpu_addr;
> +
> +			/* Find the untranslated IO space address */
> +			pp->io_mod_addr = of_read_number(parser.range -
> +							 parser.np + na, ns);
>  		}
>  		if (restype == IORESOURCE_MEM) {
>  			of_pci_range_to_resource(&range, np, &pp->mem);
>  			pp->mem.name = "MEM";
>  			pp->config.mem_size = resource_size(&pp->mem);
>  			pp->config.mem_bus_addr = range.pci_addr;
> +
> +			/* Find the untranslated MEM space address */
> +			pp->mem_mod_addr = of_read_number(parser.range -
> +							  parser.np + na, ns);
>  		}
>  		if (restype == 0) {
>  			of_pci_range_to_resource(&range, np, &pp->cfg);
> @@ -448,6 +469,12 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
>  			pp->config.cfg1_size = resource_size(&pp->cfg)/2;
>  			pp->cfg0_base = pp->cfg.start;
>  			pp->cfg1_base = pp->cfg.start + pp->config.cfg0_size;
> +
> +			/* Find the untranslated configuration space address */
> +			pp->cfg0_mod_addr = of_read_number(parser.range -
> +							   parser.np + na, ns);
> +			pp->cfg1_mod_addr = pp->cfg0_mod_addr +
> +					    pp->config.cfg0_size;
>  		}
>  	}
>  
> @@ -522,9 +549,9 @@ 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,
>  			  PCIE_ATU_VIEWPORT);
> -	dw_pcie_writel_rc(pp, pp->cfg0_base, PCIE_ATU_LOWER_BASE);
> -	dw_pcie_writel_rc(pp, (pp->cfg0_base >> 32), PCIE_ATU_UPPER_BASE);
> -	dw_pcie_writel_rc(pp, pp->cfg0_base + pp->config.cfg0_size - 1,
> +	dw_pcie_writel_rc(pp, pp->cfg0_mod_addr, PCIE_ATU_LOWER_BASE);
> +	dw_pcie_writel_rc(pp, (pp->cfg0_mod_addr >> 32), PCIE_ATU_UPPER_BASE);
> +	dw_pcie_writel_rc(pp, pp->cfg0_mod_addr + pp->config.cfg0_size - 1,
>  			  PCIE_ATU_LIMIT);
>  	dw_pcie_writel_rc(pp, busdev, PCIE_ATU_LOWER_TARGET);
>  	dw_pcie_writel_rc(pp, 0, PCIE_ATU_UPPER_TARGET);
> @@ -538,9 +565,9 @@ static void dw_pcie_prog_viewport_cfg1(struct pcie_port *pp, u32 busdev)
>  	dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX1,
>  			  PCIE_ATU_VIEWPORT);
>  	dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_CFG1, PCIE_ATU_CR1);
> -	dw_pcie_writel_rc(pp, pp->cfg1_base, PCIE_ATU_LOWER_BASE);
> -	dw_pcie_writel_rc(pp, (pp->cfg1_base >> 32), PCIE_ATU_UPPER_BASE);
> -	dw_pcie_writel_rc(pp, pp->cfg1_base + pp->config.cfg1_size - 1,
> +	dw_pcie_writel_rc(pp, pp->cfg1_mod_addr, PCIE_ATU_LOWER_BASE);
> +	dw_pcie_writel_rc(pp, (pp->cfg1_mod_addr >> 32), PCIE_ATU_UPPER_BASE);
> +	dw_pcie_writel_rc(pp, pp->cfg1_mod_addr + pp->config.cfg1_size - 1,
>  			  PCIE_ATU_LIMIT);
>  	dw_pcie_writel_rc(pp, busdev, PCIE_ATU_LOWER_TARGET);
>  	dw_pcie_writel_rc(pp, 0, PCIE_ATU_UPPER_TARGET);
> @@ -553,9 +580,9 @@ static void dw_pcie_prog_viewport_mem_outbound(struct pcie_port *pp)
>  	dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX0,
>  			  PCIE_ATU_VIEWPORT);
>  	dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_MEM, PCIE_ATU_CR1);
> -	dw_pcie_writel_rc(pp, pp->mem_base, PCIE_ATU_LOWER_BASE);
> -	dw_pcie_writel_rc(pp, (pp->mem_base >> 32), PCIE_ATU_UPPER_BASE);
> -	dw_pcie_writel_rc(pp, pp->mem_base + pp->config.mem_size - 1,
> +	dw_pcie_writel_rc(pp, pp->mem_mod_addr, PCIE_ATU_LOWER_BASE);
> +	dw_pcie_writel_rc(pp, (pp->mem_mod_addr >> 32), PCIE_ATU_UPPER_BASE);
> +	dw_pcie_writel_rc(pp, pp->mem_mod_addr + pp->config.mem_size - 1,
>  			  PCIE_ATU_LIMIT);
>  	dw_pcie_writel_rc(pp, pp->config.mem_bus_addr, PCIE_ATU_LOWER_TARGET);
>  	dw_pcie_writel_rc(pp, upper_32_bits(pp->config.mem_bus_addr),
> diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
> index 77f592f..13828f8 100644
> --- a/drivers/pci/host/pcie-designware.h
> +++ b/drivers/pci/host/pcie-designware.h
> @@ -36,11 +36,15 @@ struct pcie_port {
>  	u8			root_bus_nr;
>  	void __iomem		*dbi_base;
>  	u64			cfg0_base;
> +	u64			cfg0_mod_addr;
>  	void __iomem		*va_cfg0_base;
>  	u64			cfg1_base;
> +	u64			cfg1_mod_addr;
>  	void __iomem		*va_cfg1_base;
>  	u64			io_base;
> +	u64			io_mod_addr;
>  	u64			mem_base;
> +	u64			mem_mod_addr;
>  	struct resource		cfg;
>  	struct resource		io;
>  	struct resource		mem;
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jingoo Han July 8, 2014, 1:17 p.m. UTC | #6
On Tuesday, July 08, 2014 9:31 PM, Kishon Vijay Abraham I wrote:
> 
> Hi Mohit, Jingoo,
> 
> On Wednesday 25 June 2014 11:26 PM, Kishon Vijay Abraham I wrote:
> > In DRA7, the cpu sees 32bit address, but the pcie controller can see only 28bit
> > address. So whenever the cpu issues a read/write request, the 4 most
> > significant bits are used by L3 to determine the target controller.
> > For example, the cpu reserves 0x2000_0000 - 0x2FFF_FFFF for PCIe controller but
> > the PCIe controller will see only (0x000_0000 - 0xFFF_FFF). So for programming
> > the outbound translation window the *base* should be programmed as 0x000_0000.
> > Whenever we try to write to say 0x2000_0000, it will be translated to whatever
> > we have programmed in the translation window with base as 0x000_0000.
> >
> > This is needed when the dt node is modelled something like below
> > axi {
> > 	compatible = "simple-bus";
> > 	#size-cells = <1>;
> > 	#address-cells = <1>;
> > 	ranges = <0x0        0x20000000 0x10000000 // 28-bit bus
> > 		  0x51000000 0x51000000 0x3000>;
> > 	pcie@51000000 {
> > 		reg = <0x1000 0x2000>, <0x51002000 0x14c>, <0x51000000 0x2000>;
> > 		reg-names = "config", "ti_conf", "rc_dbics";
> > 		#address-cells = <3>;
> > 		#size-cells = <2>;
> > 		ranges = <0x81000000 0 0          0x03000 0 0x00010000
> > 			  0x82000000 0 0x20013000 0x13000 0 0xffed000>;
> > 	};
> > };
> >
> > Here the CPU address for configuration space is 0x20013000 and the controller
> > address for configuration space is 0x13000. The controller address should be
> > used while programming the ATU (in order for translation to happen properly in
> > DRA7xx).
> 
> Are you okay with this patch? Can you give your Reviewed-by/Acked-by?

Sorry, I cannot understand fully what this patch does. :-(
However, I have some questions as below.

1. io_mod_addr
The 'io_mod_addr' variable is not used. Then, how about removing
'io_mod_addr' from this patch?

2. ranges
In the case of Exynos PCIe, there is no limitation such as 28bit for
access. Then, when there is no 'ranges' property in DT file, is there
no side effect? I mean the compatibility with other SoC's PCIe drivers.

3. _addr post fix

How about renaming 'cfg0_mod_addr', 'mem_mod_addr' as below?

 	u64			cfg0_base;
+	u64			cfg0_mod_base;
 	void __iomem		*va_cfg0_base;
 	u64			cfg1_base;
+	u64			cfg1_mod_base;
 	void __iomem		*va_cfg1_base;
 	u64			io_base;
+	u64			io_mod_base;
 	u64			mem_base;
+	u64			mem_mod_base;


Best regards,
Jingoo Han

> 
> Cheers
> Kishon
> >
> > Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Mohit Kumar <mohit.kumar@st.com>
> > Cc: Jingoo Han <jg1.han@samsung.com>
> > Cc: Marek Vasut <marex@denx.de>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> > ---
> >  drivers/pci/host/pcie-designware.c |   49 ++++++++++++++++++++++++++++--------
> >  drivers/pci/host/pcie-designware.h |    4 +++
> >  2 files changed, 42 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> > index 0b7b455..11aba6d 100644
> > --- a/drivers/pci/host/pcie-designware.c
> > +++ b/drivers/pci/host/pcie-designware.c
> > @@ -401,8 +401,15 @@ 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;
> > -	int i;
> > +	u32 val, na, ns;
> > +	const __be32 *addrp;
> > +	int i, index;
> > +
> > +	/* Find the address cell size and the number of cells in order to get
> > +	 * the untranslated address.
> > +	 */
> > +	of_property_read_u32(np, "#address-cells", &na);
> > +	ns = of_n_size_cells(np);
> >
> >  	cfg_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config");
> >  	if (cfg_res) {
> > @@ -410,6 +417,12 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
> >  		pp->config.cfg1_size = resource_size(cfg_res)/2;
> >  		pp->cfg0_base = cfg_res->start;
> >  		pp->cfg1_base = cfg_res->start + pp->config.cfg0_size;
> > +
> > +		/* Find the untranslated configuration space address */
> > +		index = of_property_match_string(np, "reg-names", "config");
> > +		addrp = of_get_address(np, index, false, false);
> > +		pp->cfg0_mod_addr = of_read_number(addrp, ns);
> > +		pp->cfg1_mod_addr = pp->cfg0_mod_addr + pp->config.cfg0_size;
> >  	} else {
> >  		dev_err(pp->dev, "missing *config* reg space\n");
> >  	}
> > @@ -435,12 +448,20 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
> >  			pp->config.io_size = resource_size(&pp->io);
> >  			pp->config.io_bus_addr = range.pci_addr;
> >  			pp->io_base = range.cpu_addr;
> > +
> > +			/* Find the untranslated IO space address */
> > +			pp->io_mod_addr = of_read_number(parser.range -
> > +							 parser.np + na, ns);
> >  		}
> >  		if (restype == IORESOURCE_MEM) {
> >  			of_pci_range_to_resource(&range, np, &pp->mem);
> >  			pp->mem.name = "MEM";
> >  			pp->config.mem_size = resource_size(&pp->mem);
> >  			pp->config.mem_bus_addr = range.pci_addr;
> > +
> > +			/* Find the untranslated MEM space address */
> > +			pp->mem_mod_addr = of_read_number(parser.range -
> > +							  parser.np + na, ns);
> >  		}
> >  		if (restype == 0) {
> >  			of_pci_range_to_resource(&range, np, &pp->cfg);
> > @@ -448,6 +469,12 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
> >  			pp->config.cfg1_size = resource_size(&pp->cfg)/2;
> >  			pp->cfg0_base = pp->cfg.start;
> >  			pp->cfg1_base = pp->cfg.start + pp->config.cfg0_size;
> > +
> > +			/* Find the untranslated configuration space address */
> > +			pp->cfg0_mod_addr = of_read_number(parser.range -
> > +							   parser.np + na, ns);
> > +			pp->cfg1_mod_addr = pp->cfg0_mod_addr +
> > +					    pp->config.cfg0_size;
> >  		}
> >  	}
> >
> > @@ -522,9 +549,9 @@ 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,
> >  			  PCIE_ATU_VIEWPORT);
> > -	dw_pcie_writel_rc(pp, pp->cfg0_base, PCIE_ATU_LOWER_BASE);
> > -	dw_pcie_writel_rc(pp, (pp->cfg0_base >> 32), PCIE_ATU_UPPER_BASE);
> > -	dw_pcie_writel_rc(pp, pp->cfg0_base + pp->config.cfg0_size - 1,
> > +	dw_pcie_writel_rc(pp, pp->cfg0_mod_addr, PCIE_ATU_LOWER_BASE);
> > +	dw_pcie_writel_rc(pp, (pp->cfg0_mod_addr >> 32), PCIE_ATU_UPPER_BASE);
> > +	dw_pcie_writel_rc(pp, pp->cfg0_mod_addr + pp->config.cfg0_size - 1,
> >  			  PCIE_ATU_LIMIT);
> >  	dw_pcie_writel_rc(pp, busdev, PCIE_ATU_LOWER_TARGET);
> >  	dw_pcie_writel_rc(pp, 0, PCIE_ATU_UPPER_TARGET);
> > @@ -538,9 +565,9 @@ static void dw_pcie_prog_viewport_cfg1(struct pcie_port *pp, u32 busdev)
> >  	dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX1,
> >  			  PCIE_ATU_VIEWPORT);
> >  	dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_CFG1, PCIE_ATU_CR1);
> > -	dw_pcie_writel_rc(pp, pp->cfg1_base, PCIE_ATU_LOWER_BASE);
> > -	dw_pcie_writel_rc(pp, (pp->cfg1_base >> 32), PCIE_ATU_UPPER_BASE);
> > -	dw_pcie_writel_rc(pp, pp->cfg1_base + pp->config.cfg1_size - 1,
> > +	dw_pcie_writel_rc(pp, pp->cfg1_mod_addr, PCIE_ATU_LOWER_BASE);
> > +	dw_pcie_writel_rc(pp, (pp->cfg1_mod_addr >> 32), PCIE_ATU_UPPER_BASE);
> > +	dw_pcie_writel_rc(pp, pp->cfg1_mod_addr + pp->config.cfg1_size - 1,
> >  			  PCIE_ATU_LIMIT);
> >  	dw_pcie_writel_rc(pp, busdev, PCIE_ATU_LOWER_TARGET);
> >  	dw_pcie_writel_rc(pp, 0, PCIE_ATU_UPPER_TARGET);
> > @@ -553,9 +580,9 @@ static void dw_pcie_prog_viewport_mem_outbound(struct pcie_port *pp)
> >  	dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX0,
> >  			  PCIE_ATU_VIEWPORT);
> >  	dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_MEM, PCIE_ATU_CR1);
> > -	dw_pcie_writel_rc(pp, pp->mem_base, PCIE_ATU_LOWER_BASE);
> > -	dw_pcie_writel_rc(pp, (pp->mem_base >> 32), PCIE_ATU_UPPER_BASE);
> > -	dw_pcie_writel_rc(pp, pp->mem_base + pp->config.mem_size - 1,
> > +	dw_pcie_writel_rc(pp, pp->mem_mod_addr, PCIE_ATU_LOWER_BASE);
> > +	dw_pcie_writel_rc(pp, (pp->mem_mod_addr >> 32), PCIE_ATU_UPPER_BASE);
> > +	dw_pcie_writel_rc(pp, pp->mem_mod_addr + pp->config.mem_size - 1,
> >  			  PCIE_ATU_LIMIT);
> >  	dw_pcie_writel_rc(pp, pp->config.mem_bus_addr, PCIE_ATU_LOWER_TARGET);
> >  	dw_pcie_writel_rc(pp, upper_32_bits(pp->config.mem_bus_addr),
> > diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
> > index 77f592f..13828f8 100644
> > --- a/drivers/pci/host/pcie-designware.h
> > +++ b/drivers/pci/host/pcie-designware.h
> > @@ -36,11 +36,15 @@ struct pcie_port {
> >  	u8			root_bus_nr;
> >  	void __iomem		*dbi_base;
> >  	u64			cfg0_base;
> > +	u64			cfg0_mod_addr;
> >  	void __iomem		*va_cfg0_base;
> >  	u64			cfg1_base;
> > +	u64			cfg1_mod_addr;
> >  	void __iomem		*va_cfg1_base;
> >  	u64			io_base;
> > +	u64			io_mod_addr;
> >  	u64			mem_base;
> > +	u64			mem_mod_addr;
> >  	struct resource		cfg;
> >  	struct resource		io;
> >  	struct resource		mem;
> >

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mohit KUMAR DCG July 9, 2014, 3:40 a.m. UTC | #7
Hello Kishon,

> -----Original Message-----
> From: Kishon Vijay Abraham I [mailto:kishon@ti.com]
> Sent: Tuesday, July 08, 2014 6:01 PM
> To: devicetree@vger.kernel.org; linux-doc@vger.kernel.org; linux-
> pci@vger.kernel.org; jg1.han@samsung.com; bhelgaas@google.com; Mohit
> KUMAR DCG; linux-kernel@vger.kernel.org
> Cc: grant.likely@linaro.org; Jason Gunthorpe; Marek Vasut; Arnd Bergmann
> Subject: Re: [PATCH 2/3] PCI: designware: use untranslated address while
> programming ATU
> 
> Hi Mohit, Jingoo,
> 
> On Wednesday 25 June 2014 11:26 PM, Kishon Vijay Abraham I wrote:
> > In DRA7, the cpu sees 32bit address, but the pcie controller can see
> > only 28bit address. So whenever the cpu issues a read/write request,
> > the 4 most significant bits are used by L3 to determine the target controller.
> > For example, the cpu reserves 0x2000_0000 - 0x2FFF_FFFF for PCIe
> > controller but the PCIe controller will see only (0x000_0000 -
> > 0xFFF_FFF). So for programming the outbound translation window the
> *base* should be programmed as 0x000_0000.
> > Whenever we try to write to say 0x2000_0000, it will be translated to
> > whatever we have programmed in the translation window with base as
> 0x000_0000.
> >
> > This is needed when the dt node is modelled something like below axi {
> > 	compatible = "simple-bus";
> > 	#size-cells = <1>;
> > 	#address-cells = <1>;
> > 	ranges = <0x0        0x20000000 0x10000000 // 28-bit bus
> > 		  0x51000000 0x51000000 0x3000>;
> > 	pcie@51000000 {
> > 		reg = <0x1000 0x2000>, <0x51002000 0x14c>, <0x51000000
> 0x2000>;
> > 		reg-names = "config", "ti_conf", "rc_dbics";
> > 		#address-cells = <3>;
> > 		#size-cells = <2>;
> > 		ranges = <0x81000000 0 0          0x03000 0 0x00010000
> > 			  0x82000000 0 0x20013000 0x13000 0 0xffed000>;
> > 	};
> > };
> >
> > Here the CPU address for configuration space is 0x20013000 and the
> > controller address for configuration space is 0x13000. The controller
> > address should be used while programming the ATU (in order for
> > translation to happen properly in DRA7xx).
> 
> Are you okay with this patch? Can you give your Reviewed-by/Acked-by?
> 

- looks fine to me.
Reviewed-by: Mohit Kumar<mohit.kumar@st.com>

Thanks
Mohit
> Cheers
> Kishon
> >
> > Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Mohit Kumar <mohit.kumar@st.com>
> > Cc: Jingoo Han <jg1.han@samsung.com>
> > Cc: Marek Vasut <marex@denx.de>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> > ---
> >  drivers/pci/host/pcie-designware.c |   49
> ++++++++++++++++++++++++++++--------
> >  drivers/pci/host/pcie-designware.h |    4 +++
> >  2 files changed, 42 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/pci/host/pcie-designware.c
> > b/drivers/pci/host/pcie-designware.c
> > index 0b7b455..11aba6d 100644
> > --- a/drivers/pci/host/pcie-designware.c
> > +++ b/drivers/pci/host/pcie-designware.c
> > @@ -401,8 +401,15 @@ 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;
> > -	int i;
> > +	u32 val, na, ns;
> > +	const __be32 *addrp;
> > +	int i, index;
> > +
> > +	/* Find the address cell size and the number of cells in order to get
> > +	 * the untranslated address.
> > +	 */
> > +	of_property_read_u32(np, "#address-cells", &na);
> > +	ns = of_n_size_cells(np);
> >
> >  	cfg_res = platform_get_resource_byname(pdev,
> IORESOURCE_MEM, "config");
> >  	if (cfg_res) {
> > @@ -410,6 +417,12 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
> >  		pp->config.cfg1_size = resource_size(cfg_res)/2;
> >  		pp->cfg0_base = cfg_res->start;
> >  		pp->cfg1_base = cfg_res->start + pp->config.cfg0_size;
> > +
> > +		/* Find the untranslated configuration space address */
> > +		index = of_property_match_string(np, "reg-names",
> "config");
> > +		addrp = of_get_address(np, index, false, false);
> > +		pp->cfg0_mod_addr = of_read_number(addrp, ns);
> > +		pp->cfg1_mod_addr = pp->cfg0_mod_addr + pp-
> >config.cfg0_size;
> >  	} else {
> >  		dev_err(pp->dev, "missing *config* reg space\n");
> >  	}
> > @@ -435,12 +448,20 @@ int __init dw_pcie_host_init(struct pcie_port
> *pp)
> >  			pp->config.io_size = resource_size(&pp->io);
> >  			pp->config.io_bus_addr = range.pci_addr;
> >  			pp->io_base = range.cpu_addr;
> > +
> > +			/* Find the untranslated IO space address */
> > +			pp->io_mod_addr = of_read_number(parser.range -
> > +							 parser.np + na, ns);
> >  		}
> >  		if (restype == IORESOURCE_MEM) {
> >  			of_pci_range_to_resource(&range, np, &pp->mem);
> >  			pp->mem.name = "MEM";
> >  			pp->config.mem_size = resource_size(&pp->mem);
> >  			pp->config.mem_bus_addr = range.pci_addr;
> > +
> > +			/* Find the untranslated MEM space address */
> > +			pp->mem_mod_addr =
> of_read_number(parser.range -
> > +							  parser.np + na, ns);
> >  		}
> >  		if (restype == 0) {
> >  			of_pci_range_to_resource(&range, np, &pp->cfg);
> @@ -448,6 +469,12
> > @@ int __init dw_pcie_host_init(struct pcie_port *pp)
> >  			pp->config.cfg1_size = resource_size(&pp->cfg)/2;
> >  			pp->cfg0_base = pp->cfg.start;
> >  			pp->cfg1_base = pp->cfg.start + pp-
> >config.cfg0_size;
> > +
> > +			/* Find the untranslated configuration space address
> */
> > +			pp->cfg0_mod_addr =
> of_read_number(parser.range -
> > +							   parser.np + na, ns);
> > +			pp->cfg1_mod_addr = pp->cfg0_mod_addr +
> > +					    pp->config.cfg0_size;
> >  		}
> >  	}
> >
> > @@ -522,9 +549,9 @@ 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,
> >  			  PCIE_ATU_VIEWPORT);
> > -	dw_pcie_writel_rc(pp, pp->cfg0_base, PCIE_ATU_LOWER_BASE);
> > -	dw_pcie_writel_rc(pp, (pp->cfg0_base >> 32),
> PCIE_ATU_UPPER_BASE);
> > -	dw_pcie_writel_rc(pp, pp->cfg0_base + pp->config.cfg0_size - 1,
> > +	dw_pcie_writel_rc(pp, pp->cfg0_mod_addr,
> PCIE_ATU_LOWER_BASE);
> > +	dw_pcie_writel_rc(pp, (pp->cfg0_mod_addr >> 32),
> PCIE_ATU_UPPER_BASE);
> > +	dw_pcie_writel_rc(pp, pp->cfg0_mod_addr + pp->config.cfg0_size -
> 1,
> >  			  PCIE_ATU_LIMIT);
> >  	dw_pcie_writel_rc(pp, busdev, PCIE_ATU_LOWER_TARGET);
> >  	dw_pcie_writel_rc(pp, 0, PCIE_ATU_UPPER_TARGET); @@ -538,9
> +565,9 @@
> > static void dw_pcie_prog_viewport_cfg1(struct pcie_port *pp, u32 busdev)
> >  	dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND |
> PCIE_ATU_REGION_INDEX1,
> >  			  PCIE_ATU_VIEWPORT);
> >  	dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_CFG1, PCIE_ATU_CR1);
> > -	dw_pcie_writel_rc(pp, pp->cfg1_base, PCIE_ATU_LOWER_BASE);
> > -	dw_pcie_writel_rc(pp, (pp->cfg1_base >> 32),
> PCIE_ATU_UPPER_BASE);
> > -	dw_pcie_writel_rc(pp, pp->cfg1_base + pp->config.cfg1_size - 1,
> > +	dw_pcie_writel_rc(pp, pp->cfg1_mod_addr,
> PCIE_ATU_LOWER_BASE);
> > +	dw_pcie_writel_rc(pp, (pp->cfg1_mod_addr >> 32),
> PCIE_ATU_UPPER_BASE);
> > +	dw_pcie_writel_rc(pp, pp->cfg1_mod_addr + pp->config.cfg1_size -
> 1,
> >  			  PCIE_ATU_LIMIT);
> >  	dw_pcie_writel_rc(pp, busdev, PCIE_ATU_LOWER_TARGET);
> >  	dw_pcie_writel_rc(pp, 0, PCIE_ATU_UPPER_TARGET); @@ -553,9
> +580,9 @@
> > static void dw_pcie_prog_viewport_mem_outbound(struct pcie_port *pp)
> >  	dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND |
> PCIE_ATU_REGION_INDEX0,
> >  			  PCIE_ATU_VIEWPORT);
> >  	dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_MEM, PCIE_ATU_CR1);
> > -	dw_pcie_writel_rc(pp, pp->mem_base, PCIE_ATU_LOWER_BASE);
> > -	dw_pcie_writel_rc(pp, (pp->mem_base >> 32),
> PCIE_ATU_UPPER_BASE);
> > -	dw_pcie_writel_rc(pp, pp->mem_base + pp->config.mem_size - 1,
> > +	dw_pcie_writel_rc(pp, pp->mem_mod_addr,
> PCIE_ATU_LOWER_BASE);
> > +	dw_pcie_writel_rc(pp, (pp->mem_mod_addr >> 32),
> PCIE_ATU_UPPER_BASE);
> > +	dw_pcie_writel_rc(pp, pp->mem_mod_addr + pp->config.mem_size
> - 1,
> >  			  PCIE_ATU_LIMIT);
> >  	dw_pcie_writel_rc(pp, pp->config.mem_bus_addr,
> PCIE_ATU_LOWER_TARGET);
> >  	dw_pcie_writel_rc(pp, upper_32_bits(pp->config.mem_bus_addr),
> > diff --git a/drivers/pci/host/pcie-designware.h
> > b/drivers/pci/host/pcie-designware.h
> > index 77f592f..13828f8 100644
> > --- a/drivers/pci/host/pcie-designware.h
> > +++ b/drivers/pci/host/pcie-designware.h
> > @@ -36,11 +36,15 @@ struct pcie_port {
> >  	u8			root_bus_nr;
> >  	void __iomem		*dbi_base;
> >  	u64			cfg0_base;
> > +	u64			cfg0_mod_addr;
> >  	void __iomem		*va_cfg0_base;
> >  	u64			cfg1_base;
> > +	u64			cfg1_mod_addr;
> >  	void __iomem		*va_cfg1_base;
> >  	u64			io_base;
> > +	u64			io_mod_addr;
> >  	u64			mem_base;
> > +	u64			mem_mod_addr;
> >  	struct resource		cfg;
> >  	struct resource		io;
> >  	struct resource		mem;
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kishon Vijay Abraham I July 9, 2014, 9:38 a.m. UTC | #8
Hi Jingoo,

On Tuesday 08 July 2014 06:47 PM, Jingoo Han wrote:
> On Tuesday, July 08, 2014 9:31 PM, Kishon Vijay Abraham I wrote:
>>
>> Hi Mohit, Jingoo,
>>
>> On Wednesday 25 June 2014 11:26 PM, Kishon Vijay Abraham I wrote:
>>> In DRA7, the cpu sees 32bit address, but the pcie controller can see only 28bit
>>> address. So whenever the cpu issues a read/write request, the 4 most
>>> significant bits are used by L3 to determine the target controller.
>>> For example, the cpu reserves 0x2000_0000 - 0x2FFF_FFFF for PCIe controller but
>>> the PCIe controller will see only (0x000_0000 - 0xFFF_FFF). So for programming
>>> the outbound translation window the *base* should be programmed as 0x000_0000.
>>> Whenever we try to write to say 0x2000_0000, it will be translated to whatever
>>> we have programmed in the translation window with base as 0x000_0000.
>>>
>>> This is needed when the dt node is modelled something like below
>>> axi {
>>> 	compatible = "simple-bus";
>>> 	#size-cells = <1>;
>>> 	#address-cells = <1>;
>>> 	ranges = <0x0        0x20000000 0x10000000 // 28-bit bus
>>> 		  0x51000000 0x51000000 0x3000>;
>>> 	pcie@51000000 {
>>> 		reg = <0x1000 0x2000>, <0x51002000 0x14c>, <0x51000000 0x2000>;
>>> 		reg-names = "config", "ti_conf", "rc_dbics";
>>> 		#address-cells = <3>;
>>> 		#size-cells = <2>;
>>> 		ranges = <0x81000000 0 0          0x03000 0 0x00010000
>>> 			  0x82000000 0 0x20013000 0x13000 0 0xffed000>;
>>> 	};
>>> };
>>>
>>> Here the CPU address for configuration space is 0x20013000 and the controller
>>> address for configuration space is 0x13000. The controller address should be
>>> used while programming the ATU (in order for translation to happen properly in
>>> DRA7xx).
>>
>> Are you okay with this patch? Can you give your Reviewed-by/Acked-by?
> 
> Sorry, I cannot understand fully what this patch does. :-(
> However, I have some questions as below.
> 
> 1. io_mod_addr
> The 'io_mod_addr' variable is not used. Then, how about removing
> 'io_mod_addr' from this patch?

Actually I missed using it in io_outbound(). Will fix it.
> 
> 2. ranges
> In the case of Exynos PCIe, there is no limitation such as 28bit for
> access. Then, when there is no 'ranges' property in DT file, is there
> no side effect? I mean the compatibility with other SoC's PCIe drivers.

It shouldn't break anything. The ranges is used to resolve cpu address while
adding resources to the device. So in other SoCs it'll use use the parent
'ranges' which is usually ocp/simple-bus to resolve cpu address. In my case it
has to use the axi ranges. So other SoCs shouldn't be impacted.
> 
> 3. _addr post fix
> 
> How about renaming 'cfg0_mod_addr', 'mem_mod_addr' as below?

sure.

Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index 0b7b455..11aba6d 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -401,8 +401,15 @@  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;
-	int i;
+	u32 val, na, ns;
+	const __be32 *addrp;
+	int i, index;
+
+	/* Find the address cell size and the number of cells in order to get
+	 * the untranslated address.
+	 */
+	of_property_read_u32(np, "#address-cells", &na);
+	ns = of_n_size_cells(np);
 
 	cfg_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config");
 	if (cfg_res) {
@@ -410,6 +417,12 @@  int __init dw_pcie_host_init(struct pcie_port *pp)
 		pp->config.cfg1_size = resource_size(cfg_res)/2;
 		pp->cfg0_base = cfg_res->start;
 		pp->cfg1_base = cfg_res->start + pp->config.cfg0_size;
+
+		/* Find the untranslated configuration space address */
+		index = of_property_match_string(np, "reg-names", "config");
+		addrp = of_get_address(np, index, false, false);
+		pp->cfg0_mod_addr = of_read_number(addrp, ns);
+		pp->cfg1_mod_addr = pp->cfg0_mod_addr + pp->config.cfg0_size;
 	} else {
 		dev_err(pp->dev, "missing *config* reg space\n");
 	}
@@ -435,12 +448,20 @@  int __init dw_pcie_host_init(struct pcie_port *pp)
 			pp->config.io_size = resource_size(&pp->io);
 			pp->config.io_bus_addr = range.pci_addr;
 			pp->io_base = range.cpu_addr;
+
+			/* Find the untranslated IO space address */
+			pp->io_mod_addr = of_read_number(parser.range -
+							 parser.np + na, ns);
 		}
 		if (restype == IORESOURCE_MEM) {
 			of_pci_range_to_resource(&range, np, &pp->mem);
 			pp->mem.name = "MEM";
 			pp->config.mem_size = resource_size(&pp->mem);
 			pp->config.mem_bus_addr = range.pci_addr;
+
+			/* Find the untranslated MEM space address */
+			pp->mem_mod_addr = of_read_number(parser.range -
+							  parser.np + na, ns);
 		}
 		if (restype == 0) {
 			of_pci_range_to_resource(&range, np, &pp->cfg);
@@ -448,6 +469,12 @@  int __init dw_pcie_host_init(struct pcie_port *pp)
 			pp->config.cfg1_size = resource_size(&pp->cfg)/2;
 			pp->cfg0_base = pp->cfg.start;
 			pp->cfg1_base = pp->cfg.start + pp->config.cfg0_size;
+
+			/* Find the untranslated configuration space address */
+			pp->cfg0_mod_addr = of_read_number(parser.range -
+							   parser.np + na, ns);
+			pp->cfg1_mod_addr = pp->cfg0_mod_addr +
+					    pp->config.cfg0_size;
 		}
 	}
 
@@ -522,9 +549,9 @@  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,
 			  PCIE_ATU_VIEWPORT);
-	dw_pcie_writel_rc(pp, pp->cfg0_base, PCIE_ATU_LOWER_BASE);
-	dw_pcie_writel_rc(pp, (pp->cfg0_base >> 32), PCIE_ATU_UPPER_BASE);
-	dw_pcie_writel_rc(pp, pp->cfg0_base + pp->config.cfg0_size - 1,
+	dw_pcie_writel_rc(pp, pp->cfg0_mod_addr, PCIE_ATU_LOWER_BASE);
+	dw_pcie_writel_rc(pp, (pp->cfg0_mod_addr >> 32), PCIE_ATU_UPPER_BASE);
+	dw_pcie_writel_rc(pp, pp->cfg0_mod_addr + pp->config.cfg0_size - 1,
 			  PCIE_ATU_LIMIT);
 	dw_pcie_writel_rc(pp, busdev, PCIE_ATU_LOWER_TARGET);
 	dw_pcie_writel_rc(pp, 0, PCIE_ATU_UPPER_TARGET);
@@ -538,9 +565,9 @@  static void dw_pcie_prog_viewport_cfg1(struct pcie_port *pp, u32 busdev)
 	dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX1,
 			  PCIE_ATU_VIEWPORT);
 	dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_CFG1, PCIE_ATU_CR1);
-	dw_pcie_writel_rc(pp, pp->cfg1_base, PCIE_ATU_LOWER_BASE);
-	dw_pcie_writel_rc(pp, (pp->cfg1_base >> 32), PCIE_ATU_UPPER_BASE);
-	dw_pcie_writel_rc(pp, pp->cfg1_base + pp->config.cfg1_size - 1,
+	dw_pcie_writel_rc(pp, pp->cfg1_mod_addr, PCIE_ATU_LOWER_BASE);
+	dw_pcie_writel_rc(pp, (pp->cfg1_mod_addr >> 32), PCIE_ATU_UPPER_BASE);
+	dw_pcie_writel_rc(pp, pp->cfg1_mod_addr + pp->config.cfg1_size - 1,
 			  PCIE_ATU_LIMIT);
 	dw_pcie_writel_rc(pp, busdev, PCIE_ATU_LOWER_TARGET);
 	dw_pcie_writel_rc(pp, 0, PCIE_ATU_UPPER_TARGET);
@@ -553,9 +580,9 @@  static void dw_pcie_prog_viewport_mem_outbound(struct pcie_port *pp)
 	dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX0,
 			  PCIE_ATU_VIEWPORT);
 	dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_MEM, PCIE_ATU_CR1);
-	dw_pcie_writel_rc(pp, pp->mem_base, PCIE_ATU_LOWER_BASE);
-	dw_pcie_writel_rc(pp, (pp->mem_base >> 32), PCIE_ATU_UPPER_BASE);
-	dw_pcie_writel_rc(pp, pp->mem_base + pp->config.mem_size - 1,
+	dw_pcie_writel_rc(pp, pp->mem_mod_addr, PCIE_ATU_LOWER_BASE);
+	dw_pcie_writel_rc(pp, (pp->mem_mod_addr >> 32), PCIE_ATU_UPPER_BASE);
+	dw_pcie_writel_rc(pp, pp->mem_mod_addr + pp->config.mem_size - 1,
 			  PCIE_ATU_LIMIT);
 	dw_pcie_writel_rc(pp, pp->config.mem_bus_addr, PCIE_ATU_LOWER_TARGET);
 	dw_pcie_writel_rc(pp, upper_32_bits(pp->config.mem_bus_addr),
diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
index 77f592f..13828f8 100644
--- a/drivers/pci/host/pcie-designware.h
+++ b/drivers/pci/host/pcie-designware.h
@@ -36,11 +36,15 @@  struct pcie_port {
 	u8			root_bus_nr;
 	void __iomem		*dbi_base;
 	u64			cfg0_base;
+	u64			cfg0_mod_addr;
 	void __iomem		*va_cfg0_base;
 	u64			cfg1_base;
+	u64			cfg1_mod_addr;
 	void __iomem		*va_cfg1_base;
 	u64			io_base;
+	u64			io_mod_addr;
 	u64			mem_base;
+	u64			mem_mod_addr;
 	struct resource		cfg;
 	struct resource		io;
 	struct resource		mem;