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
