Re: [PATCHv9 00/12] PCI: Recode Mobiveil driver and add PCIe Gen4 driver for NXP Layerscape SoCs

[Laurentiu Tudor <laurentiu.tudor@nxp.com>]

[-- Attachment #1: Type: text/plain, Size: 3051 bytes --]



On 10.02.2020 20:41, Li Yang wrote:
> On Mon, Feb 10, 2020 at 9:32 AM Olof Johansson <olof@lixom.net> wrote:
>>
>> On Mon, Feb 10, 2020 at 4:23 PM Russell King - ARM Linux admin
>> <linux@armlinux.org.uk> wrote:
>>>
>>> On Mon, Feb 10, 2020 at 04:12:30PM +0100, Olof Johansson wrote:
>>>> On Thu, Feb 6, 2020 at 11:57 AM Z.q. Hou <zhiqiang.hou@nxp.com> wrote:
>>>>>
>>>>> Hi Olof,
>>>>>
>>>>> Thanks a lot for your comments!
>>>>> And sorry for my delay respond!
>>>>
>>>> Actually, they apply with only minor conflicts on top of current -next.
>>>>
>>>> Bjorn, any chance we can get you to pick these up pretty soon? They
>>>> enable full use of a promising ARM developer system, the SolidRun
>>>> HoneyComb, and would be quite valuable for me and others to be able to
>>>> use with mainline or -next without any additional patches applied --
>>>> which this patchset achieves.
>>>>
>>>> I know there are pending revisions based on feedback. I'll leave it up
>>>> to you and others to determine if that can be done with incremental
>>>> patches on top, or if it should be fixed before the initial patchset
>>>> is applied. But all in all, it's holding up adaption by me and surely
>>>> others of a very interesting platform -- I'm looking to replace my
>>>> aging MacchiatoBin with one of these and would need PCIe/NVMe to work
>>>> before I do.
>>>
>>> If you're going to be using NVMe, make sure you use a power-fail safe
>>> version; I've already had one instance where ext4 failed to mount
>>> because of a corrupted journal using an XPG SX8200 after the Honeycomb
>>> Serror'd, and then I powered it down after a few hours before later
>>> booting it back up.
>>>
>>> EXT4-fs (nvme0n1p2): INFO: recovery required on readonly filesystem
>>> EXT4-fs (nvme0n1p2): write access will be enabled during recovery
>>> JBD2: journal transaction 80849 on nvme0n1p2-8 is corrupt.
>>> EXT4-fs (nvme0n1p2): error loading journal
>>
>> Hmm, using btrfs on mine, not sure if the exposure is similar or not.
>>
>> Do you know if the SErr was due to a known issue and/or if it's
>> something that's fixed in production silicon?
>>
>> (I still can't enable SMMU since across a warm reboot it fails
>> *completely*, with nothing coming up and working. NXP folks, you
>> listening? :)
> 
> This is a known issue about DPAA2 MC bus not working well with SMMU
> based IO mapping.  Adding Laurentiu to the chain who has been looking
> into this issue.

Yes, I'm closely following the issue. I actually have a workaround 
(attached) but haven't submitted as it will probably raise a lot of 
eyebrows. In the mean time I'm following some discussions [1][2][3] on 
the iommu list which seem to try to tackle what appears to be a similar 
issue but with framebuffers. My hope is that we will be able to leverage 
whatever turns out.
In the mean time, can you try the workaround Leo suggested?

[1] https://patchwork.kernel.org/patch/11327667/
[2] https://patchwork.kernel.org/patch/10967729/
[3] https://patchwork.kernel.org/cover/11279577/

---
Best Regards, Laurentiu

[-- Attachment #2: 0001-arm64-dts-lx2160a-add-iommus-property-for-mc-node.patch --]
[-- Type: text/x-patch, Size: 1023 bytes --]

From 75dcd4a7bdf51db65dc5553a255b277f9d126e30 Mon Sep 17 00:00:00 2001
From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
Date: Tue, 19 Nov 2019 17:01:39 +0200
Subject: [PATCH 1/2] arm64: dts: lx2160a: add iommus property for mc node
Content-Type: text/plain; charset="us-ascii"

Enable SMMU management for the MC firmware by adding the required
iommus property in the device tree node.

Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
---
 arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi b/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi
index b032f3890c8c..f46f0d0905b5 100644
--- a/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi
@@ -868,6 +868,7 @@
 			msi-parent = <&its>;
 			/* iommu-map property is fixed up by u-boot */
 			iommu-map = <0 &smmu 0 0>;
+			iommus = <&smmu 0x4000>;
 			dma-coherent;
 			#address-cells = <3>;
 			#size-cells = <1>;
-- 
2.17.1


[-- Attachment #3: 0002-bus-fsl-mc-make-mc-work-with-smmu-disable-bypass-on.patch --]
[-- Type: text/x-patch, Size: 4146 bytes --]

From 46ccd2291e259c906b449f789ee62e03598fe4d7 Mon Sep 17 00:00:00 2001
From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
Date: Tue, 1 Oct 2019 16:22:48 +0300
Subject: [PATCH 2/2] bus: fsl-mc: make mc work with smmu disable bypass on
Content-Type: text/plain; charset="us-ascii"

Since this commit [1] booting kernel on MC based chips started to
fail because this firmware starts before the kernel and as soon as
SMMU is probed it starts to trigger contiguous faults.
This is a workaround that allows MC firmware to run with SMMU
in disable bypass mode. It consists of the following steps:
 1. pause the firmware at early boot to get a chance to setup SMMU
 2. request direct mapping for MC device
 3. resume the firmware
The workaround relies on the fact that no state is lost when
pausing / resuming firmware, as per the docs.
With this patch, platforms with MC firmware can now boot without
having to change the default config to set:
  CONFIG_ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n

[1] 954a03be033 ("iommu/arm-smmu: Break insecure users by disabling bypass by default")

Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
---
 drivers/bus/fsl-mc/fsl-mc-bus.c | 51 +++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
index fec394a28891..f9bc9c384ab5 100644
--- a/drivers/bus/fsl-mc/fsl-mc-bus.c
+++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
@@ -18,6 +18,8 @@
 #include <linux/bitops.h>
 #include <linux/msi.h>
 #include <linux/dma-mapping.h>
+#include <linux/io.h>
+#include <linux/iommu.h>
 
 #include "fsl-mc-private.h"
 
@@ -889,6 +891,12 @@ static int get_mc_addr_translation_ranges(struct device *dev,
 	return 0;
 }
 
+#define FSL_MC_GCR1	0x0
+#define GCR1_P1_STOP	BIT(31)
+
+static u32 boot_gcr1;
+static void __iomem *fsl_mc_regs;
+
 /**
  * fsl_mc_bus_probe - callback invoked when the root MC bus is being
  * added
@@ -906,6 +914,19 @@ static int fsl_mc_bus_probe(struct platform_device *pdev)
 	struct mc_version mc_version;
 	struct resource res;
 
+	/*
+	 * The MC firmware requires full access to the whole address space plus
+	 * it has no way of dealing with any kind of address translation, so
+	 * request direct mapping for it.
+	 */
+	error = iommu_request_dm_for_dev(&pdev->dev);
+	if (error)
+		dev_warn(&pdev->dev, "iommu_request_dm_for_dev() failed: %d\n",
+			 error);
+
+	/* Resume the firmware */
+	writel(boot_gcr1 & ~GCR1_P1_STOP, fsl_mc_regs + FSL_MC_GCR1);
+
 	mc = devm_kzalloc(&pdev->dev, sizeof(*mc), GFP_KERNEL);
 	if (!mc)
 		return -ENOMEM;
@@ -990,6 +1011,13 @@ static int fsl_mc_bus_remove(struct platform_device *pdev)
 	if (!fsl_mc_is_root_dprc(&mc->root_mc_bus_dev->dev))
 		return -EINVAL;
 
+	/*
+	 * Pause back the firmware so that it doesn't trigger faults by the
+	 * time SMMU gets brought down.
+	 */
+	writel(boot_gcr1 | GCR1_P1_STOP, fsl_mc_regs + FSL_MC_GCR1);
+	iounmap(fsl_mc_regs);
+
 	fsl_mc_device_remove(mc->root_mc_bus_dev);
 
 	fsl_destroy_mc_io(mc->root_mc_bus_dev->mc_io);
@@ -1018,6 +1046,8 @@ static struct platform_driver fsl_mc_bus_driver = {
 static int __init fsl_mc_bus_driver_init(void)
 {
 	int error;
+	struct device_node *np;
+	struct resource res;
 
 	error = bus_register(&fsl_mc_bus_type);
 	if (error < 0) {
@@ -1039,9 +1069,30 @@ static int __init fsl_mc_bus_driver_init(void)
 	if (error < 0)
 		goto error_cleanup_dprc_driver;
 
+	np = of_find_matching_node(NULL, fsl_mc_bus_match_table);
+	if (!of_device_is_available(np))
+		goto error_cleanup_dprc_driver;
+	error = of_address_to_resource(np, 1, &res);
+	if (error)
+		goto error_cleanup_dprc_driver;
+	fsl_mc_regs = ioremap(res.start, resource_size(&res));
+	if (!fsl_mc_regs) {
+		error = -ENXIO;
+		goto error_cleanup_dprc_driver;
+	}
+
+	boot_gcr1 = readl(fsl_mc_regs + FSL_MC_GCR1);
+	/*
+	 * If found running, pause MC firmware in order to get a chance
+	 * to setup SMMU for it.
+	 */
+	if (!(boot_gcr1 & GCR1_P1_STOP))
+		writel(boot_gcr1 | GCR1_P1_STOP,  fsl_mc_regs + FSL_MC_GCR1);
+
 	return 0;
 
 error_cleanup_dprc_driver:
+	iounmap(fsl_mc_regs);
 	dprc_driver_exit();
 
 error_cleanup_driver:
-- 
2.17.1