Re: [PATCH v5 2/6] PCI: rzg3s-host: Add Renesas RZ/G3S SoC host driver

[Claudiu Beznea <claudiu.beznea@tuxon.dev>]

Hi, Geert,

On 10/23/25 11:00, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Tue, 7 Oct 2025 at 15:37, Claudiu <claudiu.beznea@tuxon.dev> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> The Renesas RZ/G3S features a PCIe IP that complies with the PCI Express
>> Base Specification 4.0 and supports speeds of up to 5 GT/s. It functions
>> only as a root complex, with a single-lane (x1) configuration. The
>> controller includes Type 1 configuration registers, as well as IP
>> specific registers (called AXI registers) required for various adjustments.
>>
>> Hardware manual can be downloaded from the address in the "Link" section.
>> The following steps should be followed to access the manual:
>> 1/ Click the "User Manual" button
>> 2/ Click "Confirm"; this will start downloading an archive
>> 3/ Open the downloaded archive
>> 4/ Navigate to r01uh1014ej*-rzg3s-users-manual-hardware -> Deliverables
>> 5/ Open the file r01uh1014ej*-rzg3s.pdf
>>
>> Link: https://www.renesas.com/en/products/rz-g3s?queryID=695cc067c2d89e3f271d43656ede4d12
>> Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Thanks for your patch!
> 
>> --- /dev/null
>> +++ b/drivers/pci/controller/pcie-rzg3s-host.c
> 
>> +static void rzg3s_pcie_irq_compose_msi_msg(struct irq_data *data,
>> +                                          struct msi_msg *msg)
>> +{
>> +       struct rzg3s_pcie_msi *msi = irq_data_get_irq_chip_data(data);
>> +       struct rzg3s_pcie_host *host = rzg3s_msi_to_host(msi);
>> +       u32 drop_mask = RZG3S_PCI_MSIRCVWADRL_ENA |
>> +                       RZG3S_PCI_MSIRCVWADRL_MSG_DATA_ENA;
> 
> This should include bit 2 (which is hardwired to zero (for now)),
> so I think you better add
> 
>     #define RZG3S_PCI_MSIRCVWADRL_ADDR  GENMASK(31, 3)
> 
>> +       u32 lo, hi;
>> +
>> +       /*
>> +        * Enable and msg data enable bits are part of the address lo. Drop
>> +        * them.
>> +        */
>> +       lo = readl_relaxed(host->axi + RZG3S_PCI_MSIRCVWADRL) & ~drop_mask;
> 
> ... and use FIELD_GET() with the new definition here.

Bits 31..3 of RZG3S_PCI_MSIRCVWADRL contains only bits 31..3 of the MSI
receive window address low, AFAIU. Using FIELD_GET() for bits 31..3 on the
value read from RZG3S_PCI_MSIRCVWADRL and passing this value to
msg->address_lo will lead to an NVMe device not working.

The documentation of RZG3S_PCI_MSIRCVWADRL on bits 31..3 specifies: "Set
the MSI receiving window's Start Address [31:3]. However, they must be aligned
 to the size set by the MSI Receive Window Mask"

The RZG3S_PCI_MSIRCVWMSKL have the last 2 bits set to 0x3, always, as of
the current documentation.

The value written to RZG3S_PCI_MSIRCVWADRL in rzg3s_pcie_msi_hw_setup() is
aligned to 128 (RZG3S_PCI_MSI_INT_NR * sizeof(u32)) and thus bits 2..0 will
be zero, and so, these bits are used by HW to allow us, e.g., to enable the
MSI window.

RZ/G3E have 64 MSI interrupts and this will be aligned to 256, thus, the
last 3 LSB bits of the address written to RZG3S_PCI_MSIRCVWADRL will always
be zero (at least with the current known setups) and we can use the
register RZG3S_PCI_MSIRCVWADRL as proposed in this patch.

Due to these I haven't added more alignment constraints on the value set in
RZG3S_PCI_MSIRCVWADRL.

Thank you for your review,
Claudiu