Re: [PATCH V4] PCI: rcar: Add L1 link state fix into data abort hook

[Marek Vasut <marek.vasut@gmail.com>]

On 12/8/20 5:40 PM, Bjorn Helgaas wrote:

[...]

>> The R-Car PCIe controller is capable of handling L0s/L1 link states.
> 
> Minor wording nit: L0s seems irrelevant to this patch.

Of course.

> All PCIe functions are required to support the Power Management
> Capability (PCIe r5.0, sec 7.5.2), and that in turn requires D0,
> D3hot, and D3cold support, and D3hot requires L1 (sec 5.2).
> 
> So saying this device "is capable of handling L1" really doesn't tell
> us anything, and it glosses over the fact that it doesn't do it
> *correctly* and requires help from the driver to work around this
> hardware defect.

I see.

> Does this problem occur in both these cases?
> 
>    1) When ASPM enters L1, and
> 
>    2) When software writes PCI_PM_CTRL to put the device in D3hot?
> 
> IIUC both cases require the link to go to L1.  I guess the same
> software workaround applies to both cases?

Yes

[...]

>> +#ifdef CONFIG_ARM
>> +static int rcar_pcie_aarch32_abort_handler(unsigned long addr,
>> +		unsigned int fsr, struct pt_regs *regs)
>> +{
>> +	u32 pmsr;
>> +
>> +	if (!pcie_base || !__clk_is_enabled(pcie_bus_clk))
>> +		return 1;
>> +
>> +	pmsr = readl(pcie_base + PMSR);
>> +
>> +	/*
>> +	 * Test if the PCIe controller received PM_ENTER_L1 DLLP and
>> +	 * the PCIe controller is not in L1 link state. If true, apply
>> +	 * fix, which will put the controller into L1 link state, from
>> +	 * which it can return to L0s/L0 on its own.
>> +	 */
>> +	if ((pmsr & PMEL1RX) && ((pmsr & PMSTATE) != PMSTATE_L1)) {
>> +		writel(L1IATN, pcie_base + PMCTLR);
>> +		while (!(readl(pcie_base + PMSR) & L1FAEG))
>> +			;
>> +		writel(L1FAEG | PMEL1RX, pcie_base + PMSR);
>> +		return 0;
>> +	}
>> +
>> +	return 1;
> 
> I have no insight into how these abort handlers work.  Looks awfully
> kludgy to me, but if it's the only way and the ARM folks are on board
> with it, I can't object.
> 
> I guess the other alternative would be to have a quirk to stop
> advertising ASPM L1 support and D1/D2/D3hot support.  Obviously that
> may give up some power savings.
> 
> If people aren't comfortable with the reliability or maintainability
> of this approach in the upstream kernel, there's always the option of
> the users who need it carrying this as an out-of-tree patch.

I would highly prefer to be able to use mainline Linux as-is, without 
carrying any extra patches, so BSP is not an option.

>> +}
>> +
>> +static const struct of_device_id rcar_pcie_abort_handler_of_match[] __initconst = {
>> +	{ .compatible = "renesas,pcie-r8a7779" },
>> +	{ .compatible = "renesas,pcie-r8a7790" },
>> +	{ .compatible = "renesas,pcie-r8a7791" },
>> +	{ .compatible = "renesas,pcie-rcar-gen2" },
>> +	{},
>> +};
> 
> Why do we need another copy of these, as opposed to doing something
> with of_device_get_match_data(), e.g., like brcm_pcie_probe() does?

This is not a copy, but as subset of SoCs which are affected by this 
problem.

>> +static int __init rcar_pcie_init(void)
>> +{
>> +	if (of_find_matching_node(NULL, rcar_pcie_abort_handler_of_match)) {
>> +#ifdef CONFIG_ARM_LPAE
>> +		hook_fault_code(17, rcar_pcie_aarch32_abort_handler, SIGBUS, 0,
>> +				"asynchronous external abort");
>> +#else
>> +		hook_fault_code(22, rcar_pcie_aarch32_abort_handler, SIGBUS, 0,
>> +				"imprecise external abort");
>> +#endif
>> +	}
>> +
>> +	return platform_driver_register(&rcar_pcie_driver);
>> +}
>> +device_initcall(rcar_pcie_init);
>> +#else
>>   builtin_platform_driver(rcar_pcie_driver);
>> +#endif
> 
> Is the device_initcall() vs builtin_platform_driver() something
> related to the hook_fault_code()?  What would break if this were
> always builtin_platform_driver()?

rcar_pcie_init() would not be called before probe.