+JimQ,
On 12/6/2020 12:16 PM, Krzysztof Wilczyński wrote:
> Hello Nicolas, Florian and Florian,
>
> [...]
>> -/* Configuration space read/write support */
>> -static inline int brcm_pcie_cfg_index(int busnr, int devfn, int reg)
>> -{
>> - return ((PCI_SLOT(devfn) & 0x1f) << PCIE_EXT_SLOT_SHIFT)
>> - | ((PCI_FUNC(devfn) & 0x07) << PCIE_EXT_FUNC_SHIFT)
>> - | (busnr << PCIE_EXT_BUSNUM_SHIFT)
>> - | (reg & ~3);
>> -}
>> -
>> static void __iomem *brcm_pcie_map_conf(struct pci_bus *bus, unsigned int devfn,
>> int where)
>> {
>> @@ -716,7 +704,7 @@ static void __iomem *brcm_pcie_map_conf(struct pci_bus *bus, unsigned int devfn,
>> return PCI_SLOT(devfn) ? NULL : base + where;
>>
>> /* For devices, write to the config space index register */
>> - idx = brcm_pcie_cfg_index(bus->number, devfn, 0);
>> + idx = PCIE_ECAM_OFFSET(bus->number, devfn, 0);
>> writel(idx, pcie->base + PCIE_EXT_CFG_INDEX);
>> return base + PCIE_EXT_CFG_DATA + where;
>> }
> [...]
>
> Passing the hard-coded 0 as the "reg" argument here never actually did
> anything, thus the 32 bit alignment was never correctly enforced.
>
> My question would be: should this be 32 bit aligned? It seems like the
> intention was to perhaps make the alignment? I am sadly not intimately
> familiar with his hardware, so I am not sure if there is something to
> fix here or not.
>
> Also, I wonder whether it would be safe to pass the offset (the "where"
> variable) rather than hard-coded 0?
>
> Thank you for help in advance!
>
> Bjorn also asked the same question:
> https://lore.kernel.org/linux-pci/20201120203428.GA272511@bjorn-Precision-5520/
>
> Krzysztof
>
--
Florian
