[RFC PATCH] pci: pci-mvebu: setup BAR0 to internal-regs

[RFC PATCH] pci: pci-mvebu: setup BAR0 to internal-regs

[Shmuel Hazan]

From: Shmuel H <sh@tkos.co.il>

Set the port's BAR0 address to the SOC's internal registers address. By default, this register will point to 0xd0000000, which is not correct.

Signed-off-by: Shmuel Hazan <sh@tkos.co.il>
---
Sending again since I forgot to include a number of email addresses. 

Without this patch the wil6210 driver fails on interface up as follows:

# ip link set wlan0 up
[   46.142664] wil6210 0000:01:00.0 wlan0: wil_reset: Use firmware
<wil6210.fw> + board <wil6210.brd>
[   48.244216] wil6210 0000:01:00.0 wlan0: wil_wait_for_fw_ready:
Firmware not ready
ip: SIOCSIFFLAGS: Device timeout

With this patch, interface up succeeds:

# ip link set wlan0 up
[   53.632667] wil6210 0000:01:00.0 wlan0: wil_reset: Use firmware
<wil6210.fw> + board <wil6210.brd>
[   53.666560] wil6210 0000:01:00.0 wlan0: wmi_evt_ready: FW ver.
5.2.0.18(SW 18); MAC 40:0e:85:c0:77:5c; 0 MID's
[   53.676636] wil6210 0000:01:00.0 wlan0: wil_wait_for_fw_ready: FW
ready after 20 ms. HW version 0x00000002
[   53.686478] wil6210 0000:01:00.0 wlan0:
wil_configure_interrupt_moderation: set ITR_TX_CNT_TRSH = 500 usec
[   53.696191] wil6210 0000:01:00.0 wlan0:
wil_configure_interrupt_moderation: set ITR_TX_IDL_CNT_TRSH = 13 usec
[   53.706156] wil6210 0000:01:00.0 wlan0:
wil_configure_interrupt_moderation: set ITR_RX_CNT_TRSH = 500 usec
[   53.715855] wil6210 0000:01:00.0 wlan0:
wil_configure_interrupt_moderation: set ITR_RX_IDL_CNT_TRSH = 13 usec
[   53.725819] wil6210 0000:01:00.0 wlan0: wil_refresh_fw_capabilities:
keep_radio_on_during_sleep (0)

Tested on Armada 38x based system.

Another related bit of information is this U-Boot commit:

  https://gitlab.denx.de/u-boot/u-boot/commit/193a1e9f196b7fb7e913a70936c8a49060a1859c

It looks like some other devices are also affected the BAR0
initialization.
However, by default, u-boot won't initialize any PCI bus. Which
will cause the BAR0 register to stay on its default value. 

Any idea what is the right way to initialize BAR0?
---
 drivers/pci/controller/pci-mvebu.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
index 153a64676bc9..4a00e1b81b4f 100644
--- a/drivers/pci/controller/pci-mvebu.c
+++ b/drivers/pci/controller/pci-mvebu.c
@@ -203,6 +203,11 @@ static void mvebu_pcie_setup_wins(struct mvebu_pcie_port *port)
 	mvebu_writel(port, 0, PCIE_BAR_HI_OFF(1));
 	mvebu_writel(port, ((size - 1) & 0xffff0000) | 1,
 		     PCIE_BAR_CTRL_OFF(1));
+	
+	/* Point BAR0 to the device's internal registers (internal-regs on 
+	 * a38x, orion and more) */
+	mvebu_writel(port, 0xf1000000, PCIE_BAR_LO_OFF(0));
+	mvebu_writel(port, 0, PCIE_BAR_HI_OFF(0));
 }
 
 static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port)
-- 
2.27.0

Re: [RFC PATCH] pci: pci-mvebu: setup BAR0 to internal-regs

[Thomas Petazzoni]

On Mon,  8 Jun 2020 17:40:25 +0300
Shmuel Hazan <sh@tkos.co.il> wrote:

> From: Shmuel H <sh@tkos.co.il>
> 
> Set the port's BAR0 address to the SOC's internal registers address. By default, this register will point to 0xd0000000, which is not correct.
> 
> Signed-off-by: Shmuel Hazan <sh@tkos.co.il>
> ---
> Sending again since I forgot to include a number of email addresses. 
> 
> Without this patch the wil6210 driver fails on interface up as follows:
> 
> # ip link set wlan0 up
> [   46.142664] wil6210 0000:01:00.0 wlan0: wil_reset: Use firmware
> <wil6210.fw> + board <wil6210.brd>
> [   48.244216] wil6210 0000:01:00.0 wlan0: wil_wait_for_fw_ready:
> Firmware not ready
> ip: SIOCSIFFLAGS: Device timeout

Do you have any idea why this particular would not work, while many
other PCIe devices do ?

> 
> With this patch, interface up succeeds:
> 
> # ip link set wlan0 up
> [   53.632667] wil6210 0000:01:00.0 wlan0: wil_reset: Use firmware
> <wil6210.fw> + board <wil6210.brd>
> [   53.666560] wil6210 0000:01:00.0 wlan0: wmi_evt_ready: FW ver.
> 5.2.0.18(SW 18); MAC 40:0e:85:c0:77:5c; 0 MID's
> [   53.676636] wil6210 0000:01:00.0 wlan0: wil_wait_for_fw_ready: FW
> ready after 20 ms. HW version 0x00000002
> [   53.686478] wil6210 0000:01:00.0 wlan0:
> wil_configure_interrupt_moderation: set ITR_TX_CNT_TRSH = 500 usec
> [   53.696191] wil6210 0000:01:00.0 wlan0:
> wil_configure_interrupt_moderation: set ITR_TX_IDL_CNT_TRSH = 13 usec
> [   53.706156] wil6210 0000:01:00.0 wlan0:
> wil_configure_interrupt_moderation: set ITR_RX_CNT_TRSH = 500 usec
> [   53.715855] wil6210 0000:01:00.0 wlan0:
> wil_configure_interrupt_moderation: set ITR_RX_IDL_CNT_TRSH = 13 usec
> [   53.725819] wil6210 0000:01:00.0 wlan0: wil_refresh_fw_capabilities:
> keep_radio_on_during_sleep (0)
> 
> Tested on Armada 38x based system.
> 
> Another related bit of information is this U-Boot commit:
> 
>   https://gitlab.denx.de/u-boot/u-boot/commit/193a1e9f196b7fb7e913a70936c8a49060a1859c
> 
> It looks like some other devices are also affected the BAR0
> initialization.
> However, by default, u-boot won't initialize any PCI bus. Which
> will cause the BAR0 register to stay on its default value. 

Perhaps you want to include more of these details in the commit log.

> diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
> index 153a64676bc9..4a00e1b81b4f 100644
> --- a/drivers/pci/controller/pci-mvebu.c
> +++ b/drivers/pci/controller/pci-mvebu.c
> @@ -203,6 +203,11 @@ static void mvebu_pcie_setup_wins(struct mvebu_pcie_port *port)
>  	mvebu_writel(port, 0, PCIE_BAR_HI_OFF(1));
>  	mvebu_writel(port, ((size - 1) & 0xffff0000) | 1,
>  		     PCIE_BAR_CTRL_OFF(1));
> +	
> +	/* Point BAR0 to the device's internal registers (internal-regs on 
> +	 * a38x, orion and more) */
> +	mvebu_writel(port, 0xf1000000, PCIE_BAR_LO_OFF(0));

Some Armada 370/XP platforms really do use 0xd0000000 as the base
address of the internal registers. This information is available in the
DT. I think you could simply take the base address of the PCIe
controller, round down to 1 MB (which is the size of the internal
registers window) and that would give you the right address.

However, it would be good to understand this a little bit better.

Is this something you're seeing with mainline U-Boot only ? Or also
with the vendor U-Boot ? Only with this specific PCIe device ?

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [RFC PATCH] pci: pci-mvebu: setup BAR0 to internal-regs

[Shmuel H.]

Hi Thomas,

On 6/8/20 10:43 PM, Thomas Petazzoni wrote:
> On Mon,  8 Jun 2020 17:40:25 +0300
> Shmuel Hazan <sh@tkos.co.il> wrote:
>
>> From: Shmuel H <sh@tkos.co.il>
>>
>> Set the port's BAR0 address to the SOC's internal registers address. By default, this register will point to 0xd0000000, which is not correct.
>>
>> Signed-off-by: Shmuel Hazan <sh@tkos.co.il>
>> ---
>> Sending again since I forgot to include a number of email addresses. 
>>
>> Without this patch the wil6210 driver fails on interface up as follows:
>>
>> # ip link set wlan0 up
>> [   46.142664] wil6210 0000:01:00.0 wlan0: wil_reset: Use firmware
>> <wil6210.fw> + board <wil6210.brd>
>> [   48.244216] wil6210 0000:01:00.0 wlan0: wil_wait_for_fw_ready:
>> Firmware not ready
>> ip: SIOCSIFFLAGS: Device timeout
> Do you have any idea why this particular would not work, while many
> other PCIe devices do ?

Unfortunately, there is almost no documentation about the purpose of
this register apart from this cryptic sentence:

     "BAR0 is dedicated to internal register access" (Marvell a38x
functional docs, section 19.8).

I can only assume that only specific devices trigger the need for the
PCIe controller to access the SoC's internal registers and therefore
will fail to operate properly.

>
>> With this patch, interface up succeeds:
>>
>> # ip link set wlan0 up
>> [   53.632667] wil6210 0000:01:00.0 wlan0: wil_reset: Use firmware
>> <wil6210.fw> + board <wil6210.brd>
>> [   53.666560] wil6210 0000:01:00.0 wlan0: wmi_evt_ready: FW ver.
>> 5.2.0.18(SW 18); MAC 40:0e:85:c0:77:5c; 0 MID's
>> [   53.676636] wil6210 0000:01:00.0 wlan0: wil_wait_for_fw_ready: FW
>> ready after 20 ms. HW version 0x00000002
>> [   53.686478] wil6210 0000:01:00.0 wlan0:
>> wil_configure_interrupt_moderation: set ITR_TX_CNT_TRSH = 500 usec
>> [   53.696191] wil6210 0000:01:00.0 wlan0:
>> wil_configure_interrupt_moderation: set ITR_TX_IDL_CNT_TRSH = 13 usec
>> [   53.706156] wil6210 0000:01:00.0 wlan0:
>> wil_configure_interrupt_moderation: set ITR_RX_CNT_TRSH = 500 usec
>> [   53.715855] wil6210 0000:01:00.0 wlan0:
>> wil_configure_interrupt_moderation: set ITR_RX_IDL_CNT_TRSH = 13 usec
>> [   53.725819] wil6210 0000:01:00.0 wlan0: wil_refresh_fw_capabilities:
>> keep_radio_on_during_sleep (0)
>>
>> Tested on Armada 38x based system.
>>
>> Another related bit of information is this U-Boot commit:
>>
>>   https://gitlab.denx.de/u-boot/u-boot/commit/193a1e9f196b7fb7e913a70936c8a49060a1859c
>>
>> It looks like some other devices are also affected the BAR0
>> initialization.
>> However, by default, u-boot won't initialize any PCI bus. Which
>> will cause the BAR0 register to stay on its default value. 
> Perhaps you want to include more of these details in the commit log.

Thanks, will do.

>
>> diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
>> index 153a64676bc9..4a00e1b81b4f 100644
>> --- a/drivers/pci/controller/pci-mvebu.c
>> +++ b/drivers/pci/controller/pci-mvebu.c
>> @@ -203,6 +203,11 @@ static void mvebu_pcie_setup_wins(struct mvebu_pcie_port *port)
>>  	mvebu_writel(port, 0, PCIE_BAR_HI_OFF(1));
>>  	mvebu_writel(port, ((size - 1) & 0xffff0000) | 1,
>>  		     PCIE_BAR_CTRL_OFF(1));
>> +	
>> +	/* Point BAR0 to the device's internal registers (internal-regs on 
>> +	 * a38x, orion and more) */
>> +	mvebu_writel(port, 0xf1000000, PCIE_BAR_LO_OFF(0));
> Some Armada 370/XP platforms really do use 0xd0000000 as the base
> address of the internal registers. This information is available in the
> DT. I think you could simply take the base address of the PCIe
> controller, round down to 1 MB (which is the size of the internal
> registers window) and that would give you the right address.

Thanks, that a great idea.

>
> However, it would be good to understand this a little bit better.
>
> Is this something you're seeing with mainline U-Boot only ? Or also
> with the vendor U-Boot ? Only with this specific PCIe device ?

The current mainline u-boot and the current vendor u-boot (based on
u-boot 2018.4) behave the same. However, the old vendor u-boot (based on
2013) does solve this problem and explicitly enables and initialize this
BAR for each PCI bus.

See for a reference the mvPexWinInit function on
u-boot-marvell/board/mv_ebu/common/mv_hal/pex/mvPexAddrDec.c on the old
2013 vendor u-boot:
   
   
https://github.com/MarvellEmbeddedProcessors/u-boot-marvell/blob/u-boot-2013.01/board/mv_ebu/common/mv_hal/pex/mvPexAddrDec.c#L109

As for PCIe devices, I only tested two devices so far:
    - Wilocity Wil6200 rev 2 (wil6210)
    - Qualcomm Atheros QCA6174 (ath10k_pci)

Both didn't work without this patch but seems to operate properly after
this patch.

For example, without this patch, the QCA6174 fails as follow:

# echo "0000:01:00.0" > /sys/bus/pci/drivers/ath10k_pci/bind
[  554.148214] ath10k_pci 0000:01:00.0: Direct firmware load for
ath10k/cal-pci-0000:01:00.0.bin failed with error -2
[  554.160871] ath10k_pci 0000:01:00.0: qca6174 hw3.2 target 0x05030000
chip_id 0x00340aff sub 14cd:9008
[  554.170181] ath10k_pci 0000:01:00.0: kconfig debug 1 debugfs 1
tracing 1 dfs 0 testmode 0
[  554.180286] ath10k_pci 0000:01:00.0: firmware ver
WLAN.RM.4.4.1-00140-QCARMSWPZ-1 api 6 features wowlan,ignore-otp,mfp
crc32 29eb8ca1
[  554.257017] ath10k_pci 0000:01:00.0: board_file api 2 bmi_id N/A
crc32 4ed3569e
[  555.845051] ath10k_pci 0000:01:00.0: failed to receive control
response completion, polling..
[  556.885048] ath10k_pci 0000:01:00.0: Service connect timeout
[  556.890746] ath10k_pci 0000:01:00.0: failed to connect htt (-110)
[  557.005731] ath10k_pci 0000:01:00.0: could not init core (-110)
[  557.011785] ath10k_pci 0000:01:00.0: could not probe fw (-110)

After this patch, the driver loads successfully (I also verified that
wifi scanning operates properly):

# echo "0000:01:00.0" > /sys/bus/pci/drivers/ath10k_pci/bind
[  203.945785] ath10k_pci 0000:01:00.0: pci irq msi oper_irq_mode 2
irq_mode 0 reset_mode 0
[  204.337879] ath10k_pci 0000:01:00.0: Direct firmware load for
ath10k/pre-cal-pci-0000:01:00.0.bin failed with error -2
[  204.349101] ath10k_pci 0000:01:00.0: Direct firmware load for
ath10k/cal-pci-0000:01:00.0.bin failed with error -2
[  204.371089] ath10k_pci 0000:01:00.0: qca6174 hw3.2 target 0x05030000
chip_id 0x00340aff sub 14cd:9008
[  204.380382] ath10k_pci 0000:01:00.0: kconfig debug 1 debugfs 1
tracing 1 dfs 0 testmode 0
[  204.390499] ath10k_pci 0000:01:00.0: firmware ver
WLAN.RM.4.4.1-00140-QCARMSWPZ-1 api 6 features wowlan,ignore-otp,mfp
crc32 29eb8ca1
[  204.475522] ath10k_pci 0000:01:00.0: board_file api 2 bmi_id N/A
crc32 4ed3569e
[  205.023716] ath10k_pci 0000:01:00.0: Unknown eventid: 3
[  205.044022] ath10k_pci 0000:01:00.0: Unknown eventid: 118809
[  205.049769] ath10k_pci 0000:01:00.0: Unknown eventid: 90118
[  205.055597] ath10k_pci 0000:01:00.0: htt-ver 3.60 wmi-op 4 htt-op 3
cal otp max-sta 32 raw 0 hwcrypto 1

>
> Thomas

-- 
- Shmuel Hazan

mailto:sh@tkos.co.il | tel:+972-523-746-435 | http://tkos.co.il

Re: [RFC PATCH] pci: pci-mvebu: setup BAR0 to internal-regs

[Thomas Petazzoni]

Hello,

On Tue, 9 Jun 2020 14:21:07 +0300
"Shmuel H." <sh@tkos.co.il> wrote:

> Unfortunately, there is almost no documentation about the purpose of
> this register apart from this cryptic sentence:
> 
>      "BAR0 is dedicated to internal register access" (Marvell a38x
> functional docs, section 19.8).
> 
> I can only assume that only specific devices trigger the need for the
> PCIe controller to access the SoC's internal registers and therefore
> will fail to operate properly.

In fact, section 10.2.6 of the Armada XP datasheet, about MSI/MSI-X
support gives a hint: in order for the device to do a write to the MSI
doorbell address, it needs to write to a register in the "internal
registers" space". So it makes a lot of sense that this BAR0 has to be
configured.

Could you try to boot your system without your patch, and with the
pci=nomsi argument on the kernel command line ? This will prevent the
driver from using MSI, so it should fallback to legacy IRQs. If that
works, then we have the confirmation the issue is MSI related. This
will be useful just to have a good commit message that explains the
problem, because otherwise I am fine with your patch.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [RFC PATCH] pci: pci-mvebu: setup BAR0 to internal-regs

[Shmuel H.]

Hi Thomas,

On 6/10/20 1:27 PM, Thomas Petazzoni wrote:
> Hello,
>
> On Tue, 9 Jun 2020 14:21:07 +0300
> "Shmuel H." <sh@tkos.co.il> wrote:
>
>> Unfortunately, there is almost no documentation about the purpose of
>> this register apart from this cryptic sentence:
>>
>>      "BAR0 is dedicated to internal register access" (Marvell a38x
>> functional docs, section 19.8).
>>
>> I can only assume that only specific devices trigger the need for the
>> PCIe controller to access the SoC's internal registers and therefore
>> will fail to operate properly.
> In fact, section 10.2.6 of the Armada XP datasheet, about MSI/MSI-X
> support gives a hint: in order for the device to do a write to the MSI
> doorbell address, it needs to write to a register in the "internal
> registers" space". So it makes a lot of sense that this BAR0 has to be
> configured.
>
> Could you try to boot your system without your patch, and with the
> pci=nomsi argument on the kernel command line ? This will prevent the
> driver from using MSI, so it should fallback to legacy IRQs. If that
> works, then we have the confirmation the issue is MSI related. This
> will be useful just to have a good commit message that explains the
> problem, because otherwise I am fine with your patch.

I can confirm that pci=nomsi seems to solve the issue. The wil6210
operates and loads properly (without my patch), nice catch.

I will send an updated patch shortly.

Thanks!

-- 
- Shmuel Hazan

mailto:sh@tkos.co.il | tel:+972-523-746-435 | http://tkos.co.il

Re: [RFC PATCH] pci: pci-mvebu: setup BAR0 to internal-regs

[Shmuel H.]

Hi Thomas,

On 6/8/20 10:43 PM, Thomas Petazzoni wrote:
> Some Armada 370/XP platforms really do use 0xd0000000 as the base
> address of the internal registers. This information is available in the
> DT. I think you could simply take the base address of the PCIe
> controller, round down to 1 MB (which is the size of the internal
> registers window) and that would give you the right address.

Apparently, the PCIe controller is outside of the internal registers space.

I could try to use a similar code as in
arch/arm/mach-mvebu/pm.c:mvebu_internal_reg_base or get the first child
of "internal-regs" and call of_translate_address on it with one zero cell.

Do you have a better solution?

Thanks,

-- 
- Shmuel Hazan

mailto:sh@tkos.co.il | tel:+972-523-746-435 | http://tkos.co.il

Re: [RFC PATCH] pci: pci-mvebu: setup BAR0 to internal-regs

[Thomas Petazzoni]

On Wed, 10 Jun 2020 17:17:15 +0300
"Shmuel H." <sh@tkos.co.il> wrote:

> Apparently, the PCIe controller is outside of the internal registers space.

No, it is not.

It is outside of the internal-regs node in the DT because it needs more
"ranges" properties, but the PCIe controller registers *are* within the
internal registers window:

                               <0x82000000 0 0x80000 MBUS_ID(0xf0, 0x01) 0x80000 0 0x00002000
                                0x82000000 0 0x40000 MBUS_ID(0xf0, 0x01) 0x40000 0 0x00002000
                                0x82000000 0 0x44000 MBUS_ID(0xf0, 0x01) 0x44000 0 0x00002000
                                0x82000000 0 0x48000 MBUS_ID(0xf0, 0x01) 0x48000 0 0x00002000


> I could try to use a similar code as in
> arch/arm/mach-mvebu/pm.c:mvebu_internal_reg_base or get the first child
> of "internal-regs" and call of_translate_address on it with one zero cell.
> 
> Do you have a better solution?

In mvebu_pcie_map_registers(), we retrieve the address of the PCIe
registers for each port. You can take regs.start, round it down to 1
MB, and you'll get your base address.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com