Re: [PATCH v5 01/14] PCI: rockchip-ep: Fix address translation unit programming

From: Damien Le Moal <dlemoal@kernel.org>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: "Manivannan Sadhasivam" <manivannan.sadhasivam@linaro.org>,
	"Lorenzo Pieralisi" <lpieralisi@kernel.org>,
	"Kishon Vijay Abraham I" <kishon@kernel.org>,
	"Shawn Lin" <shawn.lin@rock-chips.com>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	linux-pci@vger.kernel.org, "Rob Herring" <robh@kernel.org>,
	"Krzysztof Kozlowski" <krzk+dt@kernel.org>,
	"Conor Dooley" <conor+dt@kernel.org>,
	"Heiko Stuebner" <heiko@sntech.de>,
	devicetree@vger.kernel.org, linux-rockchip@lists.infradead.org,
	"Rick Wertenbroek" <rick.wertenbroek@gmail.com>,
	"Niklas Cassel" <cassel@kernel.org>
Subject: Re: [PATCH v5 01/14] PCI: rockchip-ep: Fix address translation unit programming
Date: Sun, 17 Nov 2024 17:04:10 +0900	[thread overview]
Message-ID: <8054f771-d4d6-4d8d-85b6-dd22050897c9@kernel.org> (raw)
In-Reply-To: <20241115224145.GA2064331@bhelgaas>

On 11/16/24 07:41, Bjorn Helgaas wrote:
>> diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
>> index 136274533656..27a7febb74e0 100644
>> --- a/drivers/pci/controller/pcie-rockchip-ep.c
>> +++ b/drivers/pci/controller/pcie-rockchip-ep.c
>> @@ -63,16 +63,23 @@ static void rockchip_pcie_clear_ep_ob_atu(struct rockchip_pcie *rockchip,
>>  			    ROCKCHIP_PCIE_AT_OB_REGION_DESC1(region));
>>  }
>>  
>> +static int rockchip_pcie_ep_ob_atu_num_bits(struct rockchip_pcie *rockchip,
>> +					    u64 pci_addr, size_t size)
>> +{
>> +	int num_pass_bits = fls64(pci_addr ^ (pci_addr + size - 1));
>> +
>> +	return clamp(num_pass_bits, ROCKCHIP_PCIE_AT_MIN_NUM_BITS,
>> +		     ROCKCHIP_PCIE_AT_MAX_NUM_BITS);
>> +}
>> +
>>  static void rockchip_pcie_prog_ep_ob_atu(struct rockchip_pcie *rockchip, u8 fn,
>>  					 u32 r, u64 cpu_addr, u64 pci_addr,
>>  					 size_t size)
>>  {
>> -	int num_pass_bits = fls64(size - 1);
>> +	int num_pass_bits =
>> +		rockchip_pcie_ep_ob_atu_num_bits(rockchip, pci_addr, size);
>>  	u32 addr0, addr1, desc0;
>>  
>> -	if (num_pass_bits < 8)
>> -		num_pass_bits = 8;
>> -
>>  	addr0 = ((num_pass_bits - 1) & PCIE_CORE_OB_REGION_ADDR0_NUM_BITS) |
>>  		(lower_32_bits(pci_addr) & PCIE_CORE_OB_REGION_ADDR0_LO_ADDR);
> 
> PCIE_CORE_OB_REGION_ADDR0_NUM_BITS is 0x3f and
> rockchip_pcie_ep_ob_atu_num_bits() returns something between 8 and
> 0x14, inclusive?  So masking with PCIE_CORE_OB_REGION_ADDR0_NUM_BITS
> doesn't do anything, does it?

Indeed, we could remove that mask.

> Also, "..._NUM_BITS" is kind of a weird name for a mask.

Well, I did not change that. It was like this. Can clean that up too. Do you
want me to send a patch ?

> rockchip_pcie_prog_ob_atu() in pcie-rockchip-host.c is similar but
> different; it looks like all callers supply num_pass_bits=19.  I
> assume it doesn't need a similar change?

I did not check the TRM for host mode. But for my tests, I used 2 rockpro64, one
as RC and the other as EP, and the RC side was working just fine without any
change. So I assume it is OK as-is.

-- 
Damien Le Moal
Western Digital Research

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip