Hi, Bjorn,
On 12.05.2025 23:38, Bjorn Helgaas wrote:
> On Fri, May 09, 2025 at 01:29:40PM +0300, Claudiu Beznea wrote:
>> On 05.05.2025 14:26, Claudiu Beznea wrote:
>>> On 01.05.2025 23:12, Bjorn Helgaas wrote:
>>>> On Wed, Apr 30, 2025 at 01:32:33PM +0300, Claudiu wrote:
>>>>> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
>>>>>
>>>>> 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.
>>>>>
>>>>> Other Renesas RZ SoCs (e.g., RZ/G3E, RZ/V2H) share the same AXI registers
>>>>> but have both Root Complex and Endpoint capabilities. As a result, the PCIe
>>>>> host driver can be reused for these variants with minimal adjustments.
>> ...
>
>>>>> +static void rzg3s_pcie_update_bits(void __iomem *base, u32 offset, u32 mask, u32 val)
>>>>> +{
>>>>> + u32 tmp;
>>>>> +
>>>>> + tmp = readl(base + offset);
>>>>> + tmp &= ~mask;
>>>>> + tmp |= val & mask;
>>>>> + writel(tmp, base + offset);
>>>>> +}
>>>>
>>>> Nothing rzg3s-specific here.
>>>>
>>>> I think u32p_replace_bits() (include/linux/bitfield.h) is basically this.
>>>
>>> I wasn't aware of it. I'll use it in the next version. Thank for pointing it.
>>
>> I look into changing to u32p_replace_bits() but this one needs a mask that
>> can be verified at build time. It cannot be used directly in this function.
>> Would you prefer me to replace all the calls to rzg3s_pcie_update_bits() with:
>>
>> tmp = readl();
>> u32p_replace_bits(&tmp, ...)
>> writel(tmp);
>
> It seems like this is the prevailing way it's used.
>
> You have ~20 calls, so it seems like it might be excessive to replace
> each with readl/u32p_replace_bits/writel.
>
> But maybe you could use u32p_replace_bits() inside
> rzg3s_pcie_update_bits().
I tried it like:
#define rzg3s_pcie_update_bits(base, offset, mask, val) \
do { \
u32 tmp = readl((base) + (offset)); \
u32p_replace_bits(&tmp, (val), (mask)); \
writel(tmp, (base) + (offset)); \
} while (0)
But the mask may still depend on runtime variable. E.g. there is this call
in the driver (and other similar):
static void rzg3s_pcie_msi_irq_mask(struct irq_data *d)
{
struct rzg3s_pcie_msi *msi = irq_data_get_irq_chip_data(d);
struct rzg3s_pcie_host *host = rzg3s_msi_to_host(msi);
u8 reg_bit = d->hwirq % RZG3S_PCI_MSI_INT_PER_REG;
u8 reg_id = d->hwirq / RZG3S_PCI_MSI_INT_PER_REG;
guard(raw_spinlock_irqsave)(&host->hw_lock);
rzg3s_pcie_update_bits(host->axi, RZG3S_PCI_MSIRM(reg_id),
BIT(reg_bit), BIT(reg_bit));
}
reg_id is a runtime variable and cannot be checked at compile time thus the
compilation of u32p_replace_bits() fails with:
../include/linux/bitfield.h:166:3: error: call to ‘__bad_mask’ declared
with attribute error: bad bitfield mask
166 | __bad_mask();
| ^~~~~~~~~~~~
Please let me know if you have other suggestions.
Thank you,
Claudiu
