* [PATCH 0/5] Add RK816 PMIC support
From: Alex Bee @ 2024-03-21 14:39 UTC (permalink / raw)
To: Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Heiko Stuebner, Linus Walleij, Liam Girdwood, Mark Brown
Cc: Chris Zhong, Zhang Qing, devicetree, linux-arm-kernel,
linux-rockchip, linux-kernel, linux-gpio, Alex Bee
This series aims to add support for Rockchip RK816 PMIC series. As per
datasheet it's targeted for RK3126/RK3128 (RK816-1), RK1108 (RK816-2) and
PX3-SE (RK816-3) but might be used for other SoCs as well. The mfd consists
of an integrated RTC, a GPIO controller, two 32k clock outputs, a power key
(output), 4 buck- and 5 ldo regulators, 3 regulator-switches, and charger
with integrated fuel gauge. Charger and fuel gauge are not part of this
series. Two of the switches (otg/boost) are part of the binding, but not of
the driver. They must only ever by enabled if no battery charging is
happening, but it will be enabled automatically if a battery is attached.
Thus they need some incorporation of a yet to be added charger driver.
Integration in the existing rk8xx-infrastructure was pretty straightforward
and only needed very little tweaking. In order to not further bloat the
driver(s) too much with additional definitions I tried to re-use existing
ones wherever possible.
The patches are loosely based on the vendor's implementation, verified
against the datasheet and tested/measured on RK3126 board. As they are
touching several subsystems I'm sending them (very) early for the
6.10-cycle.
Alex Bee (5):
dt-bindings: mfd: Add rk816 binding
mfd: rk8xx: Add RK816 support
pinctrl: rk805: Add rk816 pinctrl support
regulator: rk808: Support apply_bit for
rk808_set_suspend_voltage_range
regulator: rk808: Add RK816 support
.../bindings/mfd/rockchip,rk816.yaml | 259 ++++++++++++++++++
drivers/mfd/Kconfig | 4 +-
drivers/mfd/rk8xx-core.c | 103 +++++++
drivers/mfd/rk8xx-i2c.c | 45 ++-
drivers/pinctrl/pinctrl-rk805.c | 68 +++++
drivers/regulator/rk808-regulator.c | 218 ++++++++++++++-
include/linux/mfd/rk808.h | 141 ++++++++++
7 files changed, 831 insertions(+), 7 deletions(-)
create mode 100644 Documentation/devicetree/bindings/mfd/rockchip,rk816.yaml
--
2.43.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* RE: [PATCH v2] ARM: unwind: improve unwinders for noreturn case
From: David Laight @ 2024-03-21 14:37 UTC (permalink / raw)
To: 'Russell King'
Cc: Ard Biesheuvel, 'Jiangfeng Xiao', arnd@arndb.de,
keescook@chromium.org, haibo.li@mediatek.com,
angelogioacchino.delregno@collabora.com, amergnat@baylibre.com,
akpm@linux-foundation.org, dave.hansen@linux.intel.com,
douzhaolei@huawei.com, gustavoars@kernel.org, jpoimboe@kernel.org,
kepler.chenxin@huawei.com, kirill.shutemov@linux.intel.com,
linux-hardening@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-mm@kvack.org, linux-arm-kernel@lists.infradead.org,
nixiaoming@huawei.com, peterz@infradead.org, wangbing6@huawei.com,
wangfangpeng1@huawei.com, jannh@google.com, willy@infradead.org
In-Reply-To: <ZfwxMuPflqlVyCDd@shell.armlinux.org.uk>
From: Russell King
> Sent: 21 March 2024 13:08
>
> On Thu, Mar 21, 2024 at 12:57:07PM +0000, David Laight wrote:
> > From: Russell King
> > > Sent: 21 March 2024 12:23
> > ...
> > > > That might mean you can get the BL in the middle of a function
> > > > but where the following instruction is for the 'no stack frame'
> > > > side of the branch.
> > > > That is very likely to break any stack offset calculations.
> > >
> > > No it can't. At any one point in the function, the stack has to be in
> > > a well defined state, so that access to local variables can work, and
> > > also the stack can be correctly unwound. If there exists a point in
> > > the function body which can be reached where the stack could be in two
> > > different states, then the stack can't be restored to the parent
> > > context.
> >
> > Actually you can get there with a function that has a lot of args.
> > So you can have:
> > if (...) {
> > push x
> > bl func
> > add %sp, #8
> > }
> > code;
> > which is fine.
>
> No you can't.... and that isn't even Arm code. Arm doesn't use %sp.
> Moreover, that "bl" will stomp over the link register, meaning this
> function can not return.
With 9+ arguments they spill to see https://godbolt.org/z/Yj3ovd8bY
Where the compiler generates:
f9:
cmp w0, 0
ble .L2
sub sp, sp, #32
mov w7, w0
mov w6, w0
mov w5, w0
mov w4, w0
mov w3, w0
stp x29, x30, [sp, 16]
add x29, sp, 16
mov w2, w0
mov w1, w0
str w0, [sp]
bl f
.L2:
ret
A traceback from inside f() definitely needs to use LR-4
for the stack offset.
(arm64 doesn't seem to support -mno-sched-prolog).
I've failed to get different sized stack frames for the true/false
sides of the branch.
The compiler seems to pre-allocate the space for extra args rather
than using 'push' type instructions.
This was certainly better for some x86 cpu (p-pro?) but has now
gone out of fashion.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH] ARM: dts: imx7s: Add snvs-poweroff support
From: Fabio Estevam @ 2024-03-21 14:31 UTC (permalink / raw)
To: shawnguo; +Cc: imx, kernel, linux-arm-kernel, Fabio Estevam
From: Fabio Estevam <festevam@denx.de>
Just like other i.MX devices, i.MX7 also supports using the SNVS
block to poweroff the chip.
Add an entry for it.
Signed-off-by: Fabio Estevam <festevam@denx.de>
---
arch/arm/boot/dts/nxp/imx/imx7s.dtsi | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/arch/arm/boot/dts/nxp/imx/imx7s.dtsi b/arch/arm/boot/dts/nxp/imx/imx7s.dtsi
index 9c81c6baa2d3..22dd72499ef2 100644
--- a/arch/arm/boot/dts/nxp/imx/imx7s.dtsi
+++ b/arch/arm/boot/dts/nxp/imx/imx7s.dtsi
@@ -636,6 +636,15 @@ snvs_rtc: snvs-rtc-lp {
clock-names = "snvs-rtc";
};
+ snvs_poweroff: snvs-poweroff {
+ compatible = "syscon-poweroff";
+ regmap = <&snvs>;
+ offset = <0x38>;
+ value = <0x60>;
+ mask = <0x60>;
+ status = "disabled";
+ };
+
snvs_pwrkey: snvs-powerkey {
compatible = "fsl,sec-v4.0-pwrkey";
regmap = <&snvs>;
--
2.34.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH v4] clk: imx: imx8mp: Add pm_runtime support for power saving
From: Marc Kleine-Budde @ 2024-03-21 14:11 UTC (permalink / raw)
To: Shengjiu Wang
Cc: abelvesa, peng.fan, mturquette, sboyd, shawnguo, s.hauer, kernel,
festevam, linux-imx, shengjiu.wang, linux-clk, linux-arm-kernel,
linux-kernel
In-Reply-To: <1711026842-7268-1-git-send-email-shengjiu.wang@nxp.com>
[-- Attachment #1.1: Type: text/plain, Size: 816 bytes --]
On 21.03.2024 21:14:02, Shengjiu Wang wrote:
> Add pm_runtime support for power saving. In pm runtime suspend
> state the registers will be reseted, so add registers save
> in pm runtime suspend and restore them in pm runtime resume.
>
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> ---
> changes in v4:
> - use struct clk_hw_onecell_data clk_data in priv struct
Well done! Looks good to me!
For the data structures:
Reviewed-by: Marc Kleine-Budde <mkl@pengutronix.de>
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v5 19/27] iommu/arm-smmu-v3: Keep track of arm_smmu_master_domain for SVA
From: Jason Gunthorpe @ 2024-03-21 13:55 UTC (permalink / raw)
To: Michael Shavit
Cc: iommu, Joerg Roedel, linux-arm-kernel, Robin Murphy, Will Deacon,
Eric Auger, Jean-Philippe Brucker, Moritz Fischer, Nicolin Chen,
patches, Shameerali Kolothum Thodi, Daniel Mentz
In-Reply-To: <CAKHBV26pRuG02Leawej00mm8LZceO27yWUqn9rOwYBTKmtA8UA@mail.gmail.com>
On Thu, Mar 21, 2024 at 06:47:41PM +0800, Michael Shavit wrote:
> On Tue, Mar 5, 2024 at 7:44 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
> >
> > Currently the smmu_domain->devices list is unused for SVA domains.
> > Fill it in with the SSID and master of every arm_smmu_set_pasid()
> > using the same logic as the RID attach.
> >
> > Tested-by: Nicolin Chen <nicolinc@nvidia.com>
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > ---
> > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 23 +++++++++++++++++++--
> > 1 file changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > index 2db2b822292a87..6d15fe3a160acf 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -2603,7 +2603,8 @@ to_smmu_domain_devices(struct iommu_domain *domain)
> > /* The domain can be NULL only when processing the first attach */
> > if (!domain)
> > return NULL;
> > - if (domain->type & __IOMMU_DOMAIN_PAGING)
> > + if ((domain->type & __IOMMU_DOMAIN_PAGING) ||
> > + domain->type == IOMMU_DOMAIN_SVA)
> > return to_smmu_domain(domain);
> > return NULL;
> > }
> > @@ -2813,7 +2814,9 @@ int arm_smmu_set_pasid(struct arm_smmu_master *master,
> > struct arm_smmu_domain *smmu_domain, ioasid_t pasid,
> > const struct arm_smmu_cd *cd)
> > {
> > + struct attach_state state = {.ssid = pasid};
> > struct arm_smmu_cd *cdptr;
> > + int ret;
> >
> > if (!arm_smmu_is_s1_domain(iommu_get_domain_for_dev(master->dev)))
> > return -ENODEV;
> > @@ -2821,14 +2824,30 @@ int arm_smmu_set_pasid(struct arm_smmu_master *master,
> > cdptr = arm_smmu_get_cd_ptr(master, pasid);
> > if (!cdptr)
> > return -ENOMEM;
> > +
> > + mutex_lock(&arm_smmu_asid_lock);
> > + ret = arm_smmu_attach_prepare(master, &smmu_domain->domain, &state);
> > + if (ret)
> > + goto out_unlock;
> > +
> > arm_smmu_write_cd_entry(master, pasid, cdptr, cd);
> > - return 0;
> > +
> > + arm_smmu_attach_commit(master, &state);
> > +
> > +out_unlock:
> > + mutex_unlock(&arm_smmu_asid_lock);
> > + return ret;
> > }
>
> arm_smmu_attach_commit tries to remove the master_domain entry from
> the previous domain that this master was attached to. It gets a
> pointer to the previous domain from the iommu framework with
> iommu_get_domain_for_dev().
> But in this path, arm_smmu_attach_prepare is creating a master_domain
> entry for the pasid domain, which may be different from the one
> returned by iommu_get_domain_for_dev() on the next attach.
>
> I think this ended up being safe in the end because afaict the iommu
> framework requires detaching the previous pasid domain before
> attaching a new one. But nonetheless this is pretty fragile and
> doesn't look intentional.
Oh yeah, this is a mistake!
Commit needs to take in the old domain from the caller which knows
what it actually is.
Thanks,
Jason
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH] net: stmmac: Do not enable/disable runtime PM for PCI devices
From: Mikhail Malyshev @ 2024-03-21 13:54 UTC (permalink / raw)
To: Alexandre Torgue, Jose Abreu, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Maxime Coquelin
Cc: netdev, linux-stm32, linux-arm-kernel, linux-kernel,
Mikhail Malyshev
Common function stmmac_dvr_probe is called for both PCI and non-PCI
device. For PCI devices pm_runtime_enable/disable are called by framework
and should not be called by the driver.
For PCI devices plat->pdev != NULL. Use this fact to detect PCI devices
Signed-off-by: Mikhail Malyshev <mike.malyshev@gmail.com>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 24cd80490d19..db45d8dbc1eb 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -7743,7 +7743,9 @@ int stmmac_dvr_probe(struct device *device,
pm_runtime_get_noresume(device);
pm_runtime_set_active(device);
- if (!pm_runtime_enabled(device))
+
+ /* For PCI devices PM is disabled/enabled by the framework */
+ if (!priv->plat->pdev)
pm_runtime_enable(device);
if (priv->hw->pcs != STMMAC_PCS_TBI &&
@@ -7846,7 +7848,10 @@ void stmmac_dvr_remove(struct device *dev)
mutex_destroy(&priv->lock);
bitmap_free(priv->af_xdp_zc_qps);
- pm_runtime_disable(dev);
+ /* For PCI devices PM is disabled/enabled by the framework */
+ if (!priv->plat->pdev)
+ pm_runtime_disable(dev);
+
pm_runtime_put_noidle(dev);
}
EXPORT_SYMBOL_GPL(stmmac_dvr_remove);
---
base-commit: 23956900041d968f9ad0f30db6dede4daccd7aa9
change-id: 20240321-stmmac-fix-f506d52882d2
Best regards,
--
Mikhail Malyshev <mike.malyshev@gmail.com>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH v4] clk: imx: imx8mp: Add pm_runtime support for power saving
From: Shengjiu Wang @ 2024-03-21 13:14 UTC (permalink / raw)
To: abelvesa, peng.fan, mturquette, sboyd, shawnguo, s.hauer, kernel,
festevam, linux-imx, shengjiu.wang
Cc: linux-clk, linux-arm-kernel, linux-kernel
Add pm_runtime support for power saving. In pm runtime suspend
state the registers will be reseted, so add registers save
in pm runtime suspend and restore them in pm runtime resume.
Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
---
changes in v4:
- use struct clk_hw_onecell_data clk_data in priv struct
changes in v3:
- remove REGS_NUM, use the ARRAY_SIZE
- merge clk_imx8mp_audiomix_drvdata and clk_hw_onecell_data together.
changes in v2:
- move pm_runtime_enable before the clk register
drivers/clk/imx/clk-imx8mp-audiomix.c | 157 ++++++++++++++++++++++----
1 file changed, 136 insertions(+), 21 deletions(-)
diff --git a/drivers/clk/imx/clk-imx8mp-audiomix.c b/drivers/clk/imx/clk-imx8mp-audiomix.c
index 55ed211a5e0b..574a032309c1 100644
--- a/drivers/clk/imx/clk-imx8mp-audiomix.c
+++ b/drivers/clk/imx/clk-imx8mp-audiomix.c
@@ -7,10 +7,12 @@
#include <linux/clk-provider.h>
#include <linux/device.h>
+#include <linux/io.h>
#include <linux/mod_devicetable.h>
#include <linux/module.h>
#include <linux/of.h>
#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
#include <dt-bindings/clock/imx8mp-clock.h>
@@ -18,6 +20,7 @@
#define CLKEN0 0x000
#define CLKEN1 0x004
+#define EARC 0x200
#define SAI1_MCLK_SEL 0x300
#define SAI2_MCLK_SEL 0x304
#define SAI3_MCLK_SEL 0x308
@@ -26,6 +29,11 @@
#define SAI7_MCLK_SEL 0x314
#define PDM_SEL 0x318
#define SAI_PLL_GNRL_CTL 0x400
+#define SAI_PLL_FDIVL_CTL0 0x404
+#define SAI_PLL_FDIVL_CTL1 0x408
+#define SAI_PLL_SSCG_CTL 0x40C
+#define SAI_PLL_MNIT_CTL 0x410
+#define IPG_LP_CTRL 0x504
#define SAIn_MCLK1_PARENT(n) \
static const struct clk_parent_data \
@@ -182,26 +190,82 @@ static struct clk_imx8mp_audiomix_sel sels[] = {
CLK_SAIn(7)
};
+static const u16 audiomix_regs[] = {
+ CLKEN0,
+ CLKEN1,
+ EARC,
+ SAI1_MCLK_SEL,
+ SAI2_MCLK_SEL,
+ SAI3_MCLK_SEL,
+ SAI5_MCLK_SEL,
+ SAI6_MCLK_SEL,
+ SAI7_MCLK_SEL,
+ PDM_SEL,
+ SAI_PLL_GNRL_CTL,
+ SAI_PLL_FDIVL_CTL0,
+ SAI_PLL_FDIVL_CTL1,
+ SAI_PLL_SSCG_CTL,
+ SAI_PLL_MNIT_CTL,
+ IPG_LP_CTRL,
+};
+
+struct clk_imx8mp_audiomix_priv {
+ void __iomem *base;
+ u32 regs_save[ARRAY_SIZE(audiomix_regs)];
+
+ /* Must be last */
+ struct clk_hw_onecell_data clk_data;
+};
+
+static void clk_imx8mp_audiomix_save_restore(struct device *dev, bool save)
+{
+ struct clk_imx8mp_audiomix_priv *priv = dev_get_drvdata(dev);
+ void __iomem *base = priv->base;
+ int i;
+
+ if (save) {
+ for (i = 0; i < ARRAY_SIZE(audiomix_regs); i++)
+ priv->regs_save[i] = readl(base + audiomix_regs[i]);
+ } else {
+ for (i = 0; i < ARRAY_SIZE(audiomix_regs); i++)
+ writel(priv->regs_save[i], base + audiomix_regs[i]);
+ }
+}
+
static int clk_imx8mp_audiomix_probe(struct platform_device *pdev)
{
- struct clk_hw_onecell_data *priv;
+ struct clk_imx8mp_audiomix_priv *priv;
+ struct clk_hw_onecell_data *clk_hw_data;
struct device *dev = &pdev->dev;
void __iomem *base;
struct clk_hw *hw;
- int i;
+ int i, ret;
priv = devm_kzalloc(dev,
- struct_size(priv, hws, IMX8MP_CLK_AUDIOMIX_END),
+ struct_size(priv, clk_data.hws, IMX8MP_CLK_AUDIOMIX_END),
GFP_KERNEL);
if (!priv)
return -ENOMEM;
- priv->num = IMX8MP_CLK_AUDIOMIX_END;
+ clk_hw_data = &priv->clk_data;
+ clk_hw_data->num = IMX8MP_CLK_AUDIOMIX_END;
base = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(base))
return PTR_ERR(base);
+ priv->base = base;
+ dev_set_drvdata(dev, priv);
+
+ /*
+ * pm_runtime_enable needs to be called before clk register.
+ * That is to make core->rpm_enabled to be true for clock
+ * usage.
+ */
+ pm_runtime_get_noresume(dev);
+ pm_runtime_set_active(dev);
+ pm_runtime_enable(dev);
+
for (i = 0; i < ARRAY_SIZE(sels); i++) {
if (sels[i].num_parents == 1) {
hw = devm_clk_hw_register_gate_parent_data(dev,
@@ -216,10 +280,12 @@ static int clk_imx8mp_audiomix_probe(struct platform_device *pdev)
0, NULL, NULL);
}
- if (IS_ERR(hw))
- return PTR_ERR(hw);
+ if (IS_ERR(hw)) {
+ ret = PTR_ERR(hw);
+ goto err_clk_register;
+ }
- priv->hws[sels[i].clkid] = hw;
+ clk_hw_data->hws[sels[i].clkid] = hw;
}
/* SAI PLL */
@@ -228,39 +294,86 @@ static int clk_imx8mp_audiomix_probe(struct platform_device *pdev)
ARRAY_SIZE(clk_imx8mp_audiomix_pll_parents),
CLK_SET_RATE_NO_REPARENT, base + SAI_PLL_GNRL_CTL,
0, 2, 0, NULL, NULL);
- priv->hws[IMX8MP_CLK_AUDIOMIX_SAI_PLL_REF_SEL] = hw;
+ clk_hw_data->hws[IMX8MP_CLK_AUDIOMIX_SAI_PLL_REF_SEL] = hw;
hw = imx_dev_clk_hw_pll14xx(dev, "sai_pll", "sai_pll_ref_sel",
base + 0x400, &imx_1443x_pll);
- if (IS_ERR(hw))
- return PTR_ERR(hw);
- priv->hws[IMX8MP_CLK_AUDIOMIX_SAI_PLL] = hw;
+ if (IS_ERR(hw)) {
+ ret = PTR_ERR(hw);
+ goto err_clk_register;
+ }
+ clk_hw_data->hws[IMX8MP_CLK_AUDIOMIX_SAI_PLL] = hw;
hw = devm_clk_hw_register_mux_parent_data_table(dev,
"sai_pll_bypass", clk_imx8mp_audiomix_pll_bypass_sels,
ARRAY_SIZE(clk_imx8mp_audiomix_pll_bypass_sels),
CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT,
base + SAI_PLL_GNRL_CTL, 16, 1, 0, NULL, NULL);
- if (IS_ERR(hw))
- return PTR_ERR(hw);
- priv->hws[IMX8MP_CLK_AUDIOMIX_SAI_PLL_BYPASS] = hw;
+ if (IS_ERR(hw)) {
+ ret = PTR_ERR(hw);
+ goto err_clk_register;
+ }
+
+ clk_hw_data->hws[IMX8MP_CLK_AUDIOMIX_SAI_PLL_BYPASS] = hw;
hw = devm_clk_hw_register_gate(dev, "sai_pll_out", "sai_pll_bypass",
0, base + SAI_PLL_GNRL_CTL, 13,
0, NULL);
- if (IS_ERR(hw))
- return PTR_ERR(hw);
- priv->hws[IMX8MP_CLK_AUDIOMIX_SAI_PLL_OUT] = hw;
+ if (IS_ERR(hw)) {
+ ret = PTR_ERR(hw);
+ goto err_clk_register;
+ }
+ clk_hw_data->hws[IMX8MP_CLK_AUDIOMIX_SAI_PLL_OUT] = hw;
hw = devm_clk_hw_register_fixed_factor(dev, "sai_pll_out_div2",
"sai_pll_out", 0, 1, 2);
- if (IS_ERR(hw))
- return PTR_ERR(hw);
+ if (IS_ERR(hw)) {
+ ret = PTR_ERR(hw);
+ goto err_clk_register;
+ }
+
+ ret = devm_of_clk_add_hw_provider(&pdev->dev, of_clk_hw_onecell_get,
+ clk_hw_data);
+ if (ret)
+ goto err_clk_register;
+
+ pm_runtime_put_sync(dev);
+ return 0;
+
+err_clk_register:
+ pm_runtime_put_sync(dev);
+ pm_runtime_disable(dev);
+ return ret;
+}
+
+static int clk_imx8mp_audiomix_remove(struct platform_device *pdev)
+{
+ pm_runtime_disable(&pdev->dev);
+
+ return 0;
+}
+
+static int clk_imx8mp_audiomix_runtime_suspend(struct device *dev)
+{
+ clk_imx8mp_audiomix_save_restore(dev, true);
- return devm_of_clk_add_hw_provider(&pdev->dev, of_clk_hw_onecell_get,
- priv);
+ return 0;
}
+static int clk_imx8mp_audiomix_runtime_resume(struct device *dev)
+{
+ clk_imx8mp_audiomix_save_restore(dev, false);
+
+ return 0;
+}
+
+static const struct dev_pm_ops clk_imx8mp_audiomix_pm_ops = {
+ SET_RUNTIME_PM_OPS(clk_imx8mp_audiomix_runtime_suspend,
+ clk_imx8mp_audiomix_runtime_resume, NULL)
+ SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+ pm_runtime_force_resume)
+};
+
static const struct of_device_id clk_imx8mp_audiomix_of_match[] = {
{ .compatible = "fsl,imx8mp-audio-blk-ctrl" },
{ /* sentinel */ }
@@ -269,9 +382,11 @@ MODULE_DEVICE_TABLE(of, clk_imx8mp_audiomix_of_match);
static struct platform_driver clk_imx8mp_audiomix_driver = {
.probe = clk_imx8mp_audiomix_probe,
+ .remove = clk_imx8mp_audiomix_remove,
.driver = {
.name = "imx8mp-audio-blk-ctrl",
.of_match_table = clk_imx8mp_audiomix_of_match,
+ .pm = &clk_imx8mp_audiomix_pm_ops,
},
};
--
2.34.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH v5 14/27] iommu/arm-smmu-v3: Make changing domains be hitless for ATS
From: Jason Gunthorpe @ 2024-03-21 13:28 UTC (permalink / raw)
To: Michael Shavit
Cc: iommu, Joerg Roedel, linux-arm-kernel, Robin Murphy, Will Deacon,
Eric Auger, Jean-Philippe Brucker, Moritz Fischer, Nicolin Chen,
patches, Shameerali Kolothum Thodi
In-Reply-To: <CAKHBV25xx51EuDXK91-o9BnFq5TZ66TAPuZ4YEdJ1UPq8sr7ZQ@mail.gmail.com>
On Thu, Mar 21, 2024 at 08:26:43PM +0800, Michael Shavit wrote:
> Overall I think the patch works, but it took me a while to really
> digest the big picture. Just to make sure I fully understand:
>
> We're trying to satisfy the following invariants for correctness:
> 1. Devices cannot get translations from a domain that was detached
> after arm_smmu_attach_dev() returns.
Yes, regardless of ATS this is the API requirement.
> 2. Devices cannot get cached translations from a domain after
> arm_smmu_atc_inv_domain() returns, regardless of whether the domain is
> simultaneously being attached/detached from a device.
There is no simultaneously here since the group lock is held by the
core code.
> Apart from note point 2. above, the behaviour of translations while in
> the middle of an arm_smmu_attach_dev doesn't have well defined
> requirements:
> 1. Before this patch, devices may get translations belonging to the
> old domain, then aborts or identity translations, and then the new
> domain while arm_smmu_attach_dev() is in progress.
Yes
> 2. Ater this patch, devices may get an arbitrary mix of translations
> belonging to the old domain and the new domain while
> arm_smmu_attach_dev() is in progress.
Yes, but no aborts.
> While disabling and re-enabling ATS inside arm_smmu_attach_dev() would
> meet the same requirements as this patch, it's not optimal since the
> device may still have traffic on other pasids than the one being
> manipulated.
Yes, we can't just disable ATS due to the PASIDs. A SVA PASID might be
present and disabling ATS to change the RID domain would completely
wreck it. Today this scenario is prevented by sva_enable, which is
removed in following patches.
I added this note for context to the commit message:
This is part of the bigger picture to allow changing the RID domain while
a PASID is in use. If a SVA PASID is relying on ATS to function then
changing the RID domain cannot just temporarily toggle ATS off without
also wrecking the SVA PASID. The new infrastructure here is organized so
that the PASID attach/detach flows will make use of it as well in
following patches.
Thanks,
Jason
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH net v2 0/2] MT7530 DSA subdriver fix VLAN egress and handling of all link-local frames
From: Konstantin Ryabitsev @ 2024-03-21 13:19 UTC (permalink / raw)
To: Arınç ÜNAL
Cc: Paolo Abeni, Daniel Golle, DENG Qingfang, Sean Wang, Andrew Lunn,
Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet,
Jakub Kicinski, Matthias Brugger, AngeloGioacchino Del Regno,
netdev, linux-kernel, linux-mediatek, linux-arm-kernel,
Bartel Eerdekens, erkin.bozoglu, mithat.guner
In-Reply-To: <7a7e6c46-03a4-48eb-9f46-84a05259cf26@arinc9.com>
On Thu, Mar 21, 2024 at 02:29:19PM +0300, Arınç ÜNAL wrote:
> > For future memory: I think the SoB in the cover letter is not required,
> > and is, at least to me, a bit confusing.
> >
> > No action needed here, I'll keep this unmodified, but I suggest to omit
> > it in future submission.
>
> b4 puts it on the cover letter so it's not my doing.
This is done because many subsystems use the cover letter as the merge commit
message. Those subsystems who don't follow this practice don't generally care
if there's a Signed-Off-By in the cover letter anyway, so I don't see why this
is a concern that it's there.
-K
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2] dt-bindings: display: samsung,exynos5-dp: convert to DT Schema
From: Rob Herring @ 2024-03-21 13:13 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Conor Dooley, Inki Dae, Seung-Woo Kim, Kyungmin Park,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Daniel Vetter, Krzysztof Kozlowski, Conor Dooley, Alim Akhtar,
dri-devel, devicetree, linux-arm-kernel, linux-samsung-soc,
linux-kernel
In-Reply-To: <90f0c8e5-01ec-4cb9-88a0-fe36d8d20443@linaro.org>
On Thu, Mar 21, 2024 at 08:37:15AM +0100, Krzysztof Kozlowski wrote:
> On 20/03/2024 18:04, Conor Dooley wrote:
> > On Wed, Mar 13, 2024 at 07:28:55PM +0100, Krzysztof Kozlowski wrote:
> >
> >> + clock-names:
> >> + items:
> >> + - const: dp
> >
> >> + phy-names:
> >> + items:
> >> + - const: dp
> >
> > The items lists here are redundant when you only have a single item, no?
> > Isnt it just
> > phy-names:
> > const: dp
>
> Somehow the convention for properties was to define the list. Unlike for
> compatible where we use shorter syntax like you propose. Shall we change
> the approach and use shorter syntax in general?
I guess the difference is -names is usually more than 1 whereas
compatible is frequently only 1. Either way is fine I think.
Rob
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH net v3] net: ll_temac: platform_get_resource replaced by wrong function
From: Claus Hansen Ries @ 2024-03-21 13:08 UTC (permalink / raw)
To: netdev@vger.kernel.org
Cc: davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
pabeni@redhat.com, michal.simek@amd.com, wei.fang@nxp.com,
yangyingliang@huawei.com, robh@kernel.org, harini.katakam@amd.com,
dan.carpenter@linaro.org, u.kleine-koenig@pengutronix.de,
wanghai38@huawei.com, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, Simon Horman
In-Reply-To: <f512ff25a2cd484791757c18facb526c@terma.com>
The function platform_get_resource was replaced with
devm_platform_ioremap_resource_byname and is called using 0 as name.
This eventually ends up in platform_get_resource_byname in the call
stack, where it causes a null pointer in strcmp.
if (type == resource_type(r) && !strcmp(r->name, name))
It should have been replaced with devm_platform_ioremap_resource.
Fixes: bd69058f50d5 ("net: ll_temac: Use devm_platform_ioremap_resource_byname()")
Signed-off-by: Claus Hansen Ries <chr@terma.com>
Cc: stable@vger.kernel.org
Reviewed-by: Simon Horman <horms@kernel.org>
---
v3:
- Cleanup of patch message
v2: https://marc.info/?l=linux-netdev&m=171094427802626&w=2
- fix accidently converting tabs to spaces and wording in commit message
v1: https://marc.info/?l=linux-netdev&m=171087828129633&w=2
drivers/net/ethernet/xilinx/ll_temac_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/xilinx/ll_temac_main.c b/drivers/net/ethernet/xilinx/ll_temac_main.c
index 9df39cf8b097..1072e2210aed 100644
--- a/drivers/net/ethernet/xilinx/ll_temac_main.c
+++ b/drivers/net/ethernet/xilinx/ll_temac_main.c
@@ -1443,7 +1443,7 @@ static int temac_probe(struct platform_device *pdev)
}
/* map device registers */
- lp->regs = devm_platform_ioremap_resource_byname(pdev, 0);
+ lp->regs = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(lp->regs)) {
dev_err(&pdev->dev, "could not map TEMAC registers\n");
return -ENOMEM;
base-commit: d95fcdf4961d27a3d17e5c7728367197adc89b8d
--
2.39.3 (Apple Git-146)
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH v2] ARM: unwind: improve unwinders for noreturn case
From: Russell King (Oracle) @ 2024-03-21 13:08 UTC (permalink / raw)
To: David Laight
Cc: Ard Biesheuvel, 'Jiangfeng Xiao', arnd@arndb.de,
keescook@chromium.org, haibo.li@mediatek.com,
angelogioacchino.delregno@collabora.com, amergnat@baylibre.com,
akpm@linux-foundation.org, dave.hansen@linux.intel.com,
douzhaolei@huawei.com, gustavoars@kernel.org, jpoimboe@kernel.org,
kepler.chenxin@huawei.com, kirill.shutemov@linux.intel.com,
linux-hardening@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-mm@kvack.org, linux-arm-kernel@lists.infradead.org,
nixiaoming@huawei.com, peterz@infradead.org, wangbing6@huawei.com,
wangfangpeng1@huawei.com, jannh@google.com, willy@infradead.org
In-Reply-To: <0fd55e156195440bb1d815dd8300894b@AcuMS.aculab.com>
On Thu, Mar 21, 2024 at 12:57:07PM +0000, David Laight wrote:
> From: Russell King
> > Sent: 21 March 2024 12:23
> ...
> > > That might mean you can get the BL in the middle of a function
> > > but where the following instruction is for the 'no stack frame'
> > > side of the branch.
> > > That is very likely to break any stack offset calculations.
> >
> > No it can't. At any one point in the function, the stack has to be in
> > a well defined state, so that access to local variables can work, and
> > also the stack can be correctly unwound. If there exists a point in
> > the function body which can be reached where the stack could be in two
> > different states, then the stack can't be restored to the parent
> > context.
>
> Actually you can get there with a function that has a lot of args.
> So you can have:
> if (...) {
> push x
> bl func
> add %sp, #8
> }
> code;
> which is fine.
No you can't.... and that isn't even Arm code. Arm doesn't use %sp.
Moreover, that "bl" will stomp over the link register, meaning this
function can not return.
> But if 'func' is 'noreturn' then the 'add %sp, #8' can be discarded
> and then the saved LR is that of 'code' - but the stack offset is wrong.
If func is noreturn, then the remainder of that path isn't expected
to be executed, so anything that happens after the "bl" is irrelevant.
> A PC from LR will always be the next instruction.
> It is only the PC from a fault frame that is the current one.
That sentence makes no sense to me, as I don't think it's even proper
English, so I can't parse it.
> The unwinder probably need to be told which one it has.
> (Or add 4 the fault frame PC so that the unwinder can subtract
> 4 from it.)
That's basically what I said.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* RE: [PATCH v2] ARM: unwind: improve unwinders for noreturn case
From: David Laight @ 2024-03-21 12:57 UTC (permalink / raw)
To: 'Russell King'
Cc: Ard Biesheuvel, 'Jiangfeng Xiao', arnd@arndb.de,
keescook@chromium.org, haibo.li@mediatek.com,
angelogioacchino.delregno@collabora.com, amergnat@baylibre.com,
akpm@linux-foundation.org, dave.hansen@linux.intel.com,
douzhaolei@huawei.com, gustavoars@kernel.org, jpoimboe@kernel.org,
kepler.chenxin@huawei.com, kirill.shutemov@linux.intel.com,
linux-hardening@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-mm@kvack.org, linux-arm-kernel@lists.infradead.org,
nixiaoming@huawei.com, peterz@infradead.org, wangbing6@huawei.com,
wangfangpeng1@huawei.com, jannh@google.com, willy@infradead.org
In-Reply-To: <ZfwmomjUwQdCefzh@shell.armlinux.org.uk>
From: Russell King
> Sent: 21 March 2024 12:23
...
> > That might mean you can get the BL in the middle of a function
> > but where the following instruction is for the 'no stack frame'
> > side of the branch.
> > That is very likely to break any stack offset calculations.
>
> No it can't. At any one point in the function, the stack has to be in
> a well defined state, so that access to local variables can work, and
> also the stack can be correctly unwound. If there exists a point in
> the function body which can be reached where the stack could be in two
> different states, then the stack can't be restored to the parent
> context.
Actually you can get there with a function that has a lot of args.
So you can have:
if (...) {
push x
bl func
add %sp, #8
}
code;
which is fine.
But if 'func' is 'noreturn' then the 'add %sp, #8' can be discarded
and then the saved LR is that of 'code' - but the stack offset is wrong.
> > > This is where the problem lies - because the link register value
> > > created by the BL instruction will point to the instruction after the
> > > BL which will _not_ part of the function that invoked the BL. That
> > > will probably cause issues for the ELF unwinder, which means this
> > > issue probably goes beyond _just_ printing the function name.
> >
> > Isn't this already in the unwinder?
> > A BL itself isn't going to fault with PC = next-instruction.
>
> You are missing the fact that the PC can be the saved LR, and thus
> can very well be the next instruction.
A PC from LR will always be the next instruction.
It is only the PC from a fault frame that is the current one.
The unwinder probably need to be told which one it has.
(Or add 4 the fault frame PC so that the unwinder can subtract
4 from it.)
At least (I don't think) there are any functions where the
called code is responsible for removing arguments.
That is a whole different bag of worms.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v4 1/5] dt-bindings: misc: Add mikrobus-connector
From: Andrew Lunn @ 2024-03-21 12:55 UTC (permalink / raw)
To: Vaishnav Achath
Cc: Michael Walle, Russell King (Oracle), Ayush Singh, open list,
jkridner, robertcnelson, lorforlinux, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Nishanth Menon,
Vignesh Raghavendra, Tero Kristo, Derek Kiernan, Dragan Cvetic,
Arnd Bergmann, Greg Kroah-Hartman, Mark Brown, Johan Hovold,
Alex Elder,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
moderated list:ARM/TEXAS INSTRUMENTS K3 ARCHITECTURE,
open list:SPI SUBSYSTEM, moderated list:GREYBUS SUBSYSTEM,
Vaishnav M A
In-Reply-To: <ef6a1c28-70dc-4077-b644-2704ac3cf30f@ti.com>
> > How will the ethernet boards ([1], [2]) work? Where do they get
> > their MAC address from, for example. The DT has some nice properties
> > for that, but I doubt that will be possible with the manifest files.
> > I've looked at the manifest file for the w5500 board [3] and to me
> > it looks like that board will come up with a random MAC address on
> > each start. Thus, even today, you have some boards which require
> > a more complex description.
> >
>
> Agreed, this is a limitation, unless the corresponding drivers/subsystems
> use device_property_read_* helper to fetch properties, it will not work and
> net/core/of_net.c only implements of_get_* helpers even though the
> underlying functions can be implemented with equivalent
> device_property_read_* equivalent as well.
I think this is a good example of the limitations. For an Ethernet
NIC, you often want to describe properties of both the MAC and the
PHY. What phy-mode should be used, what delays on the RGMII bus, what
LEDs are there etc. This is a pretty much solved problem with DT, we
have a well defined sub tree to represent the MAC, the MDIO bus and
the PHY on the bus.
But we do have two classes of properties here. The MAC address is
unique to a board. So that does need to be stored in the EEPROM, and
cannot be in a one time converted manifest to DT file stored in
/lib/firmware. However, to some extent, this is a solved problem. We
have a DT representation of how to look in an EEPROM to find the MAC
address. So the DT just needs to point to some bytes in the manifest
in the EEPROM.
Andrew
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH net v2] net: ll_temac: platform_get_resource replaced by wrong function
From: Paolo Abeni @ 2024-03-21 12:51 UTC (permalink / raw)
To: Claus Hansen Ries, netdev@vger.kernel.org, Simon Horman
Cc: davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
michal.simek@amd.com, wei.fang@nxp.com, yangyingliang@huawei.com,
robh@kernel.org, harini.katakam@amd.com, dan.carpenter@linaro.org,
u.kleine-koenig@pengutronix.de, wanghai38@huawei.com,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
In-Reply-To: <f512ff25a2cd484791757c18facb526c@terma.com>
On Wed, 2024-03-20 at 14:19 +0000, Claus Hansen Ries wrote:
> From: Claus Hansen Ries <chr@terma.com>
>
> Hope I am resubmitting this correctly, I've fixed the issues in
> the original submission.
Sorry for nit picking, but please re-submit dropping the above line,
you probably don't want them in git history, too.
In the next submission you can retain the already collected RB tag.
Additionally it looks like the From matches the sender email address:
it's not needed (but it does not hurt not will produce any differences
in git history).
Cheers,
Paolo
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v4 1/5] dt-bindings: misc: Add mikrobus-connector
From: Michael Walle @ 2024-03-21 12:44 UTC (permalink / raw)
To: Vaishnav Achath, Andrew Lunn
Cc: Russell King (Oracle), Ayush Singh, open list, jkridner,
robertcnelson, lorforlinux, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Nishanth Menon, Vignesh Raghavendra, Tero Kristo,
Derek Kiernan, Dragan Cvetic, Arnd Bergmann, Greg Kroah-Hartman,
Mark Brown, Johan Hovold, Alex Elder,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
moderated list:ARM/TEXAS INSTRUMENTS K3 ARCHITECTURE,
open list:SPI SUBSYSTEM, moderated list:GREYBUS SUBSYSTEM,
Vaishnav M A
In-Reply-To: <ef6a1c28-70dc-4077-b644-2704ac3cf30f@ti.com>
Hi,
> >>> Is that because the current software support is too limited? Are there
> >>> manufactures who want to create more complex designed, but are limited
> >>> by what can be described in the manifest?
> >>>
> >>
> >> most mikroBUS add-on boards in production lies in the category of
> >> sensors, displays, connectivity, mixed signal (ADC/DAC .etc) and if you
> >> look at the existing bindings under bindings/iio/ , most devices need
> >> only simple descriptions and the properties are mainly standard bus
> >> properties (SPI/I2C properties), IRQ, named-gpios, named properties,
> >> regulators, clocks the extension to manifest was made taking this into
> >> account and the named property description interface just maps the
> >> manifest entries to the unified device property interface under
> >> include/linux/property.h
> >
> > How will the ethernet boards ([1], [2]) work? Where do they get
> > their MAC address from, for example. The DT has some nice properties
> > for that, but I doubt that will be possible with the manifest files.
> > I've looked at the manifest file for the w5500 board [3] and to me
> > it looks like that board will come up with a random MAC address on
> > each start. Thus, even today, you have some boards which require
> > a more complex description.
> >
>
> Agreed, this is a limitation, unless the corresponding
> drivers/subsystems use device_property_read_* helper to fetch
> properties, it will not work and net/core/of_net.c only implements
> of_get_* helpers even though the underlying functions can be implemented
> with equivalent device_property_read_* equivalent as well.
And I don't think this is a good idea given that the alternative is
just working.
> > Apart from the discussion whether the manifest is a suitable or
> > sufficient mechanism to describe the hardware, I think the main
> > problem with the proposed binding, is that it doesn't follow the
> > binding Rob was proposing for a socket. If I want to use DT
> > overlays, how would you describe an add-on board?
> >
> > The proposal was that the base board has something like
> >
> > mikrobus: socket {
> > compatible = "mikrobus-socket";
> > i2c-parent = <&i2c0>;
> > spi-parent = <&spi0>;
> >
> > i2c {};
> > spi {};
> > };
> >
> > an add-on board can then have a DT snippet/overlay like the
> > following:
> >
> > &mikrobus {
> > i2c {
> > eeprom@52: {
> > reg = <52>;
> > compatible = <atmel,at24..>;
> > }
> > };
> >
> > spi {
> > sensor@0: {
> > reg = <0>;
> > compatible = <foobar>;
> > };
> > };
> > };
> >
> > That should be possible with a binding for the mikrobus, which
> > in fact it is just a pin header with a standard pinout. Also as
> > Russell pointed out in v3, the EEPROM/manifest is not part of the
> > mikrobus standard. So maybe that deserves an own compatible, like
> >
> > compatible = "mikroe,click", "mikrobus-socket";
> >
> > Or maybe click-eeprom? Although click seems to be the brand name of
> > MikroElektronika.
>
> Agreed, there is nothing preventing us to convert the binding and update
> the driver to follow the above proposed format and will be done in next
> revision. Click is brand name of MikroElektronika and they don't allow
> custom boards to use that branding, however clickid can be used in the
> case where EEPROM is present/enable the socket to be probeable.
Thinking about this again. I'm not sure this additional compatible
really helps the discovery. It's a chicken egg problem. Only the
modules knows if it has an EEPROM, but then, we already have to
know it's in the socket. So while it might help for a static
configuration, it does not for the discovery.
-michael
> > [1] https://www.mikroe.com/eth-3-click
> > [2] https://www.mikroe.com/eth-wiz-click
> > [3] https://github.com/MikroElektronika/click_id/blob/main/manifests/ETH-WIZ-CLICK.mnfs
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] irqchip/gic-v3-its: Fix double free on error
From: Zenghui Yu @ 2024-03-21 12:43 UTC (permalink / raw)
To: Guanrui Huang; +Cc: maz, shannon.zhao, tglx, linux-arm-kernel, linux-kernel
In-Reply-To: <20240321110454.89410-1-guanrui.huang@linux.alibaba.com>
On 2024/3/21 19:04, Guanrui Huang wrote:
> In its_vpe_irq_domain_alloc, when its_vpe_init() returns an error
> with i > 0, its_vpe_irq_domain_free may free bitmap and vprop_page,
> and then there is a double free in its_vpe_irq_domain_alloc.
>
> Fix it by checking if bitmap is empty in its_vpe_irq_domain_alloc.
> If bitmap is empty and i > 0, means that bitmap have been clear and free
> in its_vpe_irq_domain_free.
>
> Signed-off-by: Guanrui Huang <guanrui.huang@linux.alibaba.com>
> ---
> drivers/irqchip/irq-gic-v3-its.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index fca888b36680..98a1be90caef 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -4562,9 +4562,14 @@ static int its_vpe_irq_domain_alloc(struct irq_domain *domain, unsigned int virq
> }
>
> if (err) {
> - if (i > 0)
> + if (i > 0) {
> its_vpe_irq_domain_free(domain, virq, i);
>
> + /* bitmap and vprop_page be freed in its_vpe_irq_domain_free */
> + if (bitmap_empty(bitmap, nr_ids))
It looks like that 'bitmap' can't be non-empty if you managed to get
here. Right?
> + return err;
> + }
> +
> its_lpi_free(bitmap, base, nr_ids);
> its_free_prop_table(vprop_page);
> }
Is it possible that we handle these 2 cases together? I.e., does the
following approach help?
if (err)
its_vpe_irq_domain_free(domain, virq, i);
Thanks,
Zenghui
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v3] clk: imx: imx8mp: Add pm_runtime support for power saving
From: Shengjiu Wang @ 2024-03-21 12:40 UTC (permalink / raw)
To: Marc Kleine-Budde
Cc: Shengjiu Wang, abelvesa, peng.fan, mturquette, sboyd, shawnguo,
s.hauer, kernel, festevam, linux-imx, linux-clk, linux-arm-kernel,
linux-kernel
In-Reply-To: <20240321-apply-swore-f9c17ad3a8d2-mkl@pengutronix.de>
On Thu, Mar 21, 2024 at 5:04 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> On 21.03.2024 16:36:51, Shengjiu Wang wrote:
> > Add pm_runtime support for power saving. In pm runtime suspend
> > state the registers will be reseted, so add registers save
> > in pm runtime suspend and restore them in pm runtime resume.
> >
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > Reviewed-by: Peng Fan <peng.fan@nxp.com>
> > ---
> > changes in v3:
> > - remove REGS_NUM, use the ARRAY_SIZE
> > - merge clk_imx8mp_audiomix_drvdata and clk_hw_onecell_data together.
>
> Look way nicer, but still room for improvement.
>
> > changes in v2:
> > - move pm_runtime_enable before the clk register
> >
> > drivers/clk/imx/clk-imx8mp-audiomix.c | 157 ++++++++++++++++++++++----
> > 1 file changed, 136 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/clk/imx/clk-imx8mp-audiomix.c b/drivers/clk/imx/clk-imx8mp-audiomix.c
> > index 55ed211a5e0b..5ae33bce8ad8 100644
> > --- a/drivers/clk/imx/clk-imx8mp-audiomix.c
> > +++ b/drivers/clk/imx/clk-imx8mp-audiomix.c
> > @@ -7,10 +7,12 @@
> >
> > #include <linux/clk-provider.h>
> > #include <linux/device.h>
> > +#include <linux/io.h>
> > #include <linux/mod_devicetable.h>
> > #include <linux/module.h>
> > #include <linux/of.h>
> > #include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> >
> > #include <dt-bindings/clock/imx8mp-clock.h>
> >
> > @@ -18,6 +20,7 @@
> >
> > #define CLKEN0 0x000
> > #define CLKEN1 0x004
> > +#define EARC 0x200
> > #define SAI1_MCLK_SEL 0x300
> > #define SAI2_MCLK_SEL 0x304
> > #define SAI3_MCLK_SEL 0x308
> > @@ -26,6 +29,11 @@
> > #define SAI7_MCLK_SEL 0x314
> > #define PDM_SEL 0x318
> > #define SAI_PLL_GNRL_CTL 0x400
> > +#define SAI_PLL_FDIVL_CTL0 0x404
> > +#define SAI_PLL_FDIVL_CTL1 0x408
> > +#define SAI_PLL_SSCG_CTL 0x40C
> > +#define SAI_PLL_MNIT_CTL 0x410
> > +#define IPG_LP_CTRL 0x504
> >
> > #define SAIn_MCLK1_PARENT(n) \
> > static const struct clk_parent_data \
> > @@ -182,26 +190,82 @@ static struct clk_imx8mp_audiomix_sel sels[] = {
> > CLK_SAIn(7)
> > };
> >
> > +static const u16 audiomix_regs[] = {
> > + CLKEN0,
> > + CLKEN1,
> > + EARC,
> > + SAI1_MCLK_SEL,
> > + SAI2_MCLK_SEL,
> > + SAI3_MCLK_SEL,
> > + SAI5_MCLK_SEL,
> > + SAI6_MCLK_SEL,
> > + SAI7_MCLK_SEL,
> > + PDM_SEL,
> > + SAI_PLL_GNRL_CTL,
> > + SAI_PLL_FDIVL_CTL0,
> > + SAI_PLL_FDIVL_CTL1,
> > + SAI_PLL_SSCG_CTL,
> > + SAI_PLL_MNIT_CTL,
> > + IPG_LP_CTRL,
> > +};
> > +
> > +struct clk_imx8mp_audiomix_priv {
> > + void __iomem *base;
> > + struct clk_hw_onecell_data *clk_hw_data;
> > + u32 regs_save[ARRAY_SIZE(audiomix_regs)];
>
> Put the "struct clk_hw_onecell_data clk_hw_data" at the end of the
> struct. Then allocating should be easier and you don't need the
> additional pointer.
Ok, will do this change in next version.
Best Regards
Shengjiu Wang
>
> Marc
>
> --
> Pengutronix e.K. | Marc Kleine-Budde |
> Embedded Linux | https://www.pengutronix.de |
> Vertretung Nürnberg | Phone: +49-5121-206917-129 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v4 1/5] dt-bindings: misc: Add mikrobus-connector
From: Andrew Lunn @ 2024-03-21 12:31 UTC (permalink / raw)
To: Vaishnav Achath
Cc: Ayush Singh, Michael Walle, Krzysztof Kozlowski, open list,
jkridner, robertcnelson, lorforlinux, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Nishanth Menon,
Vignesh Raghavendra, Tero Kristo, Derek Kiernan, Dragan Cvetic,
Arnd Bergmann, Greg Kroah-Hartman, Mark Brown, Johan Hovold,
Alex Elder,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
moderated list:ARM/TEXAS INSTRUMENTS K3 ARCHITECTURE,
open list:SPI SUBSYSTEM, moderated list:GREYBUS SUBSYSTEM,
Vaishnav M A
In-Reply-To: <9ea69bd3-977d-442e-aacc-3c819b1a5630@ti.com>
On Thu, Mar 21, 2024 at 01:05:14PM +0530, Vaishnav Achath wrote:
> Hi Andrew,
>
> On 21/03/24 00:14, Andrew Lunn wrote:
> > On Wed, Mar 20, 2024 at 10:09:05PM +0530, Ayush Singh wrote:
> > > On 3/20/24 01:02, Andrew Lunn wrote:
> > >
> > > > > Yes, after discussion with Vaishnav and trying to brainstorm some way to do
> > > > > the same thing with dt overlays, it seems that trying to use dt overlays
> > > > > will mean need to have completely separate implementation of mikroBUS for
> > > > > local ports and mikroBUS over greybus.
> > > > Could you explain why please?
> > > >
> > > > Are greybus I2C bus masters different from physical I2C bus masters?
> > > > Are greybus SPI bus masters different from physical SPI bus masters?
> > >
> > > Well, they are virtual, so they are not declared in the device tree. I have
> > > linked the greybus i2c implementation. It basically allocates an i2c_adpater
> > > and then adds it using `i2c_add_adapter` method. This adapter can then be
> > > passed to say mikroBUS driver where it can be used as a normal i2c_adapter,
> > > and we can register the device to it.
> >
> > Being virtual does not really stop it being added to the DT.
> >
> > I'm making this all up, but i assume it will look something like this:
> >
> > greybus@42 {
> > compatible = "acme,greybus";
> > reg = <0x42 0x100>;
> >
> > This would represent the greybus host controller.
> >
> > module@0 {
> > reg = <0>;
> >
> > This would represent a module discovered on the bus. I assume there is
> > some sort of addressing? The greybus core code dynamically creates the
> > node in DT to describe the modules it has discovered. This is not too
> > different to USB. You can already describe USB devices in DT, but the
> > assumption is you know they exists, e.g. because they are hard wired,
> > not hot-plugable. The USB core will associate the USB device with the
> > node in DT. But actually creating a node in DT is not too big a jump.
> >
> > interface@0 {
> > compatible = "greybus,i2c";
> > reg = <0>;
> > }
> > interface@1 {
> > compatible = "greybus,spi";
> > reg = <1>;
> > }
> > interface@10 {
> > compatible = "greybus,gpio";
> > reg = <10>;
> > }
> >
> > It can then enumerate the interfaces on the module, and create the I2C
> > node, SPI bus node, the gpio controller etc. Again, the greybus core
> > can add nodes to DT to described the discovered hardware, and
> > associate them to the linux devices which are created.
> >
>
> This proposal looks great and would be the ideal solution, but we met with
> few challenges when initially trying to implement something like this and
> had to drop and take the route with minimal development effort to just
> instantiate mikroBUS devices.
>
> From what we understand, you are recommending to change the manifest
> description format used by greybus to device tree and also add of_bus
> support for greybus - now this will not only solve instantiating mikrobus
> devices on greybus but even complex devices on greybus making it a robust
> solution and using standard tools and support DT offers.
I would not discard the manifest. It exists, it appears to be used by
a lot of devices. So use it. But consider it an 'intermediate
representation'. Take it and transform it to DT.
Looking at:
https://libstock.mikroe.com/projects/view/5435/clickid
there appears to be a name, and hardware revision in the
manifest. Convert that to a string. Run a checksum over the rest of
the manifest, and append that to the string. You can then look in
/lib/firmware for a DT representation which matches.
> However we have a few doubts:
> * For USB or PCIe, to add OF device tree support the parent devices are
> physically present, for example USB device is a child node of USB controller
> (physically description available in a SoC DT) and USB interfaces are child
> of USB devices, how would that hierarchy look for greybus devices?
So DT support for USB, serial and PCIe devices already exists. When
the core enumerates a PCIe or USB bus, it looks in the DT blob for the
vendor:product ID, and associates any node it finds with the
device. It is not used very often, but if you search the .dts files,
you can find examples.
> Would it be
> USB/UART/transport controller -> AP Bridge host controller -> Module ->
> interface -> bundle -> CPort ?
That is for you to decide. I don't know the architecture well enough.
It is rather old, but:
https://lwn.net/Articles/715955/
There is a diagram of the sysfs tree, which looks pretty similar to
what you say above. What is however missing from the diagram is the
Linux devices themselves. Where do the I2C bus, the SPI bus, the GPIO
controller etc appear in the tree?
Maybe look at PCI and USB. Does the device tree representation include
all the bridges and hubs etc. Or does it simply the representation?
You need something to represent the controller. You need modules. Do
you need the details of interface & bundle and cport? Could you
represent them as an address tuple, and just have the devices under
module?
> When this mikrobus driver was initially implemented we could not think of
> such an approach as the SVC and Control functionality were implemented in
> userspace with gbridge ( https://github.com/anobli/gbridge ) with a netlink
> interface to kernel greybus, but today there are references to do it
> completely in kernel ( drivers/greybus/gb-beagleplay.c) and your proposal is
> implementable.
Does gb-beagleplay.c work together with the code in staging? It looks
like the GPIO controller, I2C controller, SPI controller, etc are
still in GregKH "Tree of crap". I don't think it is wise to build on
top of something in staging. So you probably need to spend time to
cleanup that code and moving it into the mainline proper. As you do
that, you could add the code needed to dynamically add nodes to device
tree.
Andrew
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v5 14/27] iommu/arm-smmu-v3: Make changing domains be hitless for ATS
From: Michael Shavit @ 2024-03-21 12:26 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: iommu, Joerg Roedel, linux-arm-kernel, Robin Murphy, Will Deacon,
Eric Auger, Jean-Philippe Brucker, Moritz Fischer, Nicolin Chen,
patches, Shameerali Kolothum Thodi
In-Reply-To: <14-v5-9a37e0c884ce+31e3-smmuv3_newapi_p2_jgg@nvidia.com>
On Tue, Mar 5, 2024 at 7:44 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> The core code allows the domain to be changed on the fly without a forced
> stop in BLOCKED/IDENTITY. In this flow the driver should just continually
> maintain the ATS with no change while the STE is updated.
>
> ATS relies on a linked list smmu_domain->devices to keep track of which
> masters have the domain programmed, but this list is also used by
> arm_smmu_share_asid(), unrelated to ats.
>
> Create two new functions to encapsulate this combined logic:
> arm_smmu_attach_prepare()
> <caller generates and sets the STE>
> arm_smmu_attach_commit()
>
> The two functions can sequence both enabling ATS and disabling across
> the STE store. Have every update of the STE use this sequence.
>
> Installing a S1/S2 domain always enables the ATS if the PCIe device
> supports it.
>
> The enable flow is now ordered differently to allow it to be hitless:
>
> 1) Add the master to the new smmu_domain->devices list
> 2) Program the STE
> 3) Enable ATS at PCIe
> 4) Remove the master from the old smmu_domain
>
> This flow ensures that invalidations to either domain will generate an ATC
> invalidation to the device while the STE is being switched. Thus we don't
> need to turn off the ATS anymore for correctness.
>
> The disable flow is the reverse:
> 1) Disable ATS at PCIe
> 2) Program the STE
> 3) Invalidate the ATC
> 4) Remove the master from the old smmu_domain
>
> Move the nr_ats_masters adjustments to be close to the list
> manipulations. It is a count of the number of ATS enabled masters
> currently in the list. This is stricly before and after the STE/CD are
> revised, and done under the list's spin_lock.
>
> Tested-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 198 ++++++++++++++------
> 1 file changed, 140 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 51a1e7198fd1af..45f2190fc31786 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -1527,7 +1527,8 @@ static void arm_smmu_make_bypass_ste(struct arm_smmu_ste *target)
> }
>
> static void arm_smmu_make_cdtable_ste(struct arm_smmu_ste *target,
> - struct arm_smmu_master *master)
> + struct arm_smmu_master *master,
> + bool ats_enabled)
> {
> struct arm_smmu_ctx_desc_cfg *cd_table = &master->cd_table;
> struct arm_smmu_device *smmu = master->smmu;
> @@ -1550,7 +1551,7 @@ static void arm_smmu_make_cdtable_ste(struct arm_smmu_ste *target,
> STRTAB_STE_1_S1STALLD :
> 0) |
> FIELD_PREP(STRTAB_STE_1_EATS,
> - master->ats_enabled ? STRTAB_STE_1_EATS_TRANS : 0));
> + ats_enabled ? STRTAB_STE_1_EATS_TRANS : 0));
>
> if (smmu->features & ARM_SMMU_FEAT_E2H) {
> /*
> @@ -1578,7 +1579,8 @@ static void arm_smmu_make_cdtable_ste(struct arm_smmu_ste *target,
>
> static void arm_smmu_make_s2_domain_ste(struct arm_smmu_ste *target,
> struct arm_smmu_master *master,
> - struct arm_smmu_domain *smmu_domain)
> + struct arm_smmu_domain *smmu_domain,
> + bool ats_enabled)
> {
> struct arm_smmu_s2_cfg *s2_cfg = &smmu_domain->s2_cfg;
> const struct io_pgtable_cfg *pgtbl_cfg =
> @@ -1594,7 +1596,7 @@ static void arm_smmu_make_s2_domain_ste(struct arm_smmu_ste *target,
>
> target->data[1] = cpu_to_le64(
> FIELD_PREP(STRTAB_STE_1_EATS,
> - master->ats_enabled ? STRTAB_STE_1_EATS_TRANS : 0) |
> + ats_enabled ? STRTAB_STE_1_EATS_TRANS : 0) |
> FIELD_PREP(STRTAB_STE_1_SHCFG,
> STRTAB_STE_1_SHCFG_INCOMING));
>
> @@ -2470,22 +2472,16 @@ static bool arm_smmu_ats_supported(struct arm_smmu_master *master)
> return dev_is_pci(dev) && pci_ats_supported(to_pci_dev(dev));
> }
>
> -static void arm_smmu_enable_ats(struct arm_smmu_master *master,
> - struct arm_smmu_domain *smmu_domain)
> +static void arm_smmu_enable_ats(struct arm_smmu_master *master)
> {
> size_t stu;
> struct pci_dev *pdev;
> struct arm_smmu_device *smmu = master->smmu;
>
> - /* Don't enable ATS at the endpoint if it's not enabled in the STE */
> - if (!master->ats_enabled)
> - return;
> -
> /* Smallest Translation Unit: log2 of the smallest supported granule */
> stu = __ffs(smmu->pgsize_bitmap);
> pdev = to_pci_dev(master->dev);
>
> - atomic_inc(&smmu_domain->nr_ats_masters);
> /*
> * ATC invalidation of PASID 0 causes the entire ATC to be flushed.
> */
> @@ -2494,22 +2490,6 @@ static void arm_smmu_enable_ats(struct arm_smmu_master *master,
> dev_err(master->dev, "Failed to enable ATS (STU %zu)\n", stu);
> }
>
> -static void arm_smmu_disable_ats(struct arm_smmu_master *master,
> - struct arm_smmu_domain *smmu_domain)
> -{
> - if (!master->ats_enabled)
> - return;
> -
> - pci_disable_ats(to_pci_dev(master->dev));
> - /*
> - * Ensure ATS is disabled at the endpoint before we issue the
> - * ATC invalidation via the SMMU.
> - */
> - wmb();
> - arm_smmu_atc_inv_master(master);
> - atomic_dec(&smmu_domain->nr_ats_masters);
> -}
> -
> static int arm_smmu_enable_pasid(struct arm_smmu_master *master)
> {
> int ret;
> @@ -2573,39 +2553,145 @@ arm_smmu_find_master_domain(struct arm_smmu_domain *smmu_domain,
> return NULL;
> }
>
> -static void arm_smmu_detach_dev(struct arm_smmu_master *master)
> +/*
> + * If the domain uses the smmu_domain->devices list return the arm_smmu_domain
> + * structure, otherwise NULL. These domains track attached devices so they can
> + * issue invalidations.
> + */
> +static struct arm_smmu_domain *
> +to_smmu_domain_devices(struct iommu_domain *domain)
> {
> - struct iommu_domain *domain = iommu_get_domain_for_dev(master->dev);
> + /* The domain can be NULL only when processing the first attach */
> + if (!domain)
> + return NULL;
> + if (domain->type & __IOMMU_DOMAIN_PAGING)
> + return to_smmu_domain(domain);
> + return NULL;
> +}
> +
> +static void arm_smmu_remove_master_domain(struct arm_smmu_master *master,
> + struct iommu_domain *domain)
> +{
> + struct arm_smmu_domain *smmu_domain = to_smmu_domain_devices(domain);
> struct arm_smmu_master_domain *master_domain;
> - struct arm_smmu_domain *smmu_domain;
> unsigned long flags;
>
> - if (!domain || !(domain->type & __IOMMU_DOMAIN_PAGING))
> + if (!smmu_domain)
> return;
>
> - smmu_domain = to_smmu_domain(domain);
> - arm_smmu_disable_ats(master, smmu_domain);
> -
> spin_lock_irqsave(&smmu_domain->devices_lock, flags);
> master_domain = arm_smmu_find_master_domain(smmu_domain, master);
> if (master_domain) {
> list_del(&master_domain->devices_elm);
> kfree(master_domain);
> + if (master->ats_enabled)
> + atomic_dec(&smmu_domain->nr_ats_masters);
> }
> spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
> +}
>
> - master->ats_enabled = false;
> +struct attach_state {
> + bool want_ats;
> + bool disable_ats;
> +};
> +
> +/*
> + * Prepare to attach a domain to a master. If disable_ats is not set this will
> + * turn on ATS if supported. smmu_domain can be NULL if the domain being
> + * attached does not have a page table and does not require invalidation
> + * tracking.
> + */
> +static int arm_smmu_attach_prepare(struct arm_smmu_master *master,
> + struct iommu_domain *domain,
> + struct attach_state *state)
> +{
> + struct arm_smmu_domain *smmu_domain =
> + to_smmu_domain_devices(domain);
> + struct arm_smmu_master_domain *master_domain;
> + unsigned long flags;
> +
> + /*
> + * arm_smmu_share_asid() must not see two domains pointing to the same
> + * arm_smmu_master_domain contents otherwise it could randomly write one
> + * or the other to the CD.
> + */
> + lockdep_assert_held(&arm_smmu_asid_lock);
> +
> + state->want_ats = !state->disable_ats && arm_smmu_ats_supported(master);
> +
> + if (smmu_domain) {
> + master_domain = kzalloc(sizeof(*master_domain), GFP_KERNEL);
> + if (!master_domain)
> + return -ENOMEM;
> + master_domain->master = master;
> +
> + /*
> + * During prepare we want the current smmu_domain and new
> + * smmu_domain to be in the devices list before we change any
> + * HW. This ensures that both domains will send ATS
> + * invalidations to the master until we are done.
> + *
> + * It is tempting to make this list only track masters that are
> + * using ATS, but arm_smmu_share_asid() also uses this to change
> + * the ASID of a domain, unrelated to ATS.
> + *
> + * Notice if we are re-attaching the same domain then the list
> + * will have two identical entries and commit will remove only
> + * one of them.
> + */
> + spin_lock_irqsave(&smmu_domain->devices_lock, flags);
> + if (state->want_ats)
> + atomic_inc(&smmu_domain->nr_ats_masters);
> + list_add(&master_domain->devices_elm, &smmu_domain->devices);
> + spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
> + }
> +
> + if (!state->want_ats && master->ats_enabled) {
> + pci_disable_ats(to_pci_dev(master->dev));
> + /*
> + * This is probably overkill, but the config write for disabling
> + * ATS should complete before the STE is configured to generate
> + * UR to avoid AER noise.
> + */
> + wmb();
> + }
> + return 0;
> +}
> +
> +/*
> + * Commit is done after the STE/CD are configured with the EATS setting. It
> + * completes synchronizing the PCI device's ATC and finishes manipulating the
> + * smmu_domain->devices list.
> + */
> +static void arm_smmu_attach_commit(struct arm_smmu_master *master,
> + struct attach_state *state)
> +{
> + lockdep_assert_held(&arm_smmu_asid_lock);
> +
> + if (state->want_ats && !master->ats_enabled) {
> + arm_smmu_enable_ats(master);
> + } else if (master->ats_enabled) {
> + /*
> + * The translation has changed, flush the ATC. At this point the
> + * SMMU is translating for the new domain and both the old&new
> + * domain will issue invalidations.
> + */
> + arm_smmu_atc_inv_master(master);
> + }
> + master->ats_enabled = state->want_ats;
> +
> + arm_smmu_remove_master_domain(master,
> + iommu_get_domain_for_dev(master->dev));
> }
>
> static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
> {
> int ret = 0;
> - unsigned long flags;
> struct arm_smmu_ste target;
> struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> struct arm_smmu_device *smmu;
> struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> - struct arm_smmu_master_domain *master_domain;
> + struct attach_state state = {};
> struct arm_smmu_master *master;
> struct arm_smmu_cd *cdptr;
>
> @@ -2642,11 +2728,6 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
> return -ENOMEM;
> }
>
> - master_domain = kzalloc(sizeof(*master_domain), GFP_KERNEL);
> - if (!master_domain)
> - return -ENOMEM;
> - master_domain->master = master;
> -
> /*
> * Prevent arm_smmu_share_asid() from trying to change the ASID
> * of either the old or new domain while we are working on it.
> @@ -2655,13 +2736,11 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
> */
> mutex_lock(&arm_smmu_asid_lock);
>
> - arm_smmu_detach_dev(master);
> -
> - master->ats_enabled = arm_smmu_ats_supported(master);
> -
> - spin_lock_irqsave(&smmu_domain->devices_lock, flags);
> - list_add(&master_domain->devices_elm, &smmu_domain->devices);
> - spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
> + ret = arm_smmu_attach_prepare(master, domain, &state);
> + if (ret) {
> + mutex_unlock(&arm_smmu_asid_lock);
> + return ret;
> + }
>
> switch (smmu_domain->stage) {
> case ARM_SMMU_DOMAIN_S1: {
> @@ -2670,18 +2749,19 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
> arm_smmu_make_s1_cd(&target_cd, master, smmu_domain);
> arm_smmu_write_cd_entry(master, IOMMU_NO_PASID, cdptr,
> &target_cd);
> - arm_smmu_make_cdtable_ste(&target, master);
> + arm_smmu_make_cdtable_ste(&target, master, state.want_ats);
> arm_smmu_install_ste_for_dev(master, &target);
> break;
> }
> case ARM_SMMU_DOMAIN_S2:
> - arm_smmu_make_s2_domain_ste(&target, master, smmu_domain);
> + arm_smmu_make_s2_domain_ste(&target, master, smmu_domain,
> + state.want_ats);
> arm_smmu_install_ste_for_dev(master, &target);
> arm_smmu_clear_cd(master, IOMMU_NO_PASID);
> break;
> }
>
> - arm_smmu_enable_ats(master, smmu_domain);
> + arm_smmu_attach_commit(master, &state);
> mutex_unlock(&arm_smmu_asid_lock);
> return 0;
> }
> @@ -2715,10 +2795,11 @@ void arm_smmu_remove_pasid(struct arm_smmu_master *master,
> arm_smmu_clear_cd(master, pasid);
> }
>
> -static int arm_smmu_attach_dev_ste(struct device *dev,
> - struct arm_smmu_ste *ste)
> +static int arm_smmu_attach_dev_ste(struct iommu_domain *domain,
> + struct device *dev, struct arm_smmu_ste *ste)
> {
> struct arm_smmu_master *master = dev_iommu_priv_get(dev);
> + struct attach_state state = {};
>
> if (arm_smmu_master_sva_enabled(master))
> return -EBUSY;
> @@ -2736,9 +2817,10 @@ static int arm_smmu_attach_dev_ste(struct device *dev,
> * the stream (STE.EATS == 0b00), causing F_BAD_ATS_TREQ and
> * F_TRANSL_FORBIDDEN events (IHI0070Ea 5.2 Stream Table Entry).
> */
> - arm_smmu_detach_dev(master);
> -
> + state.disable_ats = true;
> + arm_smmu_attach_prepare(master, domain, &state);
> arm_smmu_install_ste_for_dev(master, ste);
> + arm_smmu_attach_commit(master, &state);
> mutex_unlock(&arm_smmu_asid_lock);
>
> /*
> @@ -2756,7 +2838,7 @@ static int arm_smmu_attach_dev_identity(struct iommu_domain *domain,
> struct arm_smmu_ste ste;
>
> arm_smmu_make_bypass_ste(&ste);
> - return arm_smmu_attach_dev_ste(dev, &ste);
> + return arm_smmu_attach_dev_ste(domain, dev, &ste);
> }
>
> static const struct iommu_domain_ops arm_smmu_identity_ops = {
> @@ -2774,7 +2856,7 @@ static int arm_smmu_attach_dev_blocked(struct iommu_domain *domain,
> struct arm_smmu_ste ste;
>
> arm_smmu_make_abort_ste(&ste);
> - return arm_smmu_attach_dev_ste(dev, &ste);
> + return arm_smmu_attach_dev_ste(domain, dev, &ste);
> }
>
> static const struct iommu_domain_ops arm_smmu_blocked_ops = {
> --
> 2.43.2
>
Overall I think the patch works, but it took me a while to really
digest the big picture. Just to make sure I fully understand:
We're trying to satisfy the following invariants for correctness:
1. Devices cannot get translations from a domain that was detached
after arm_smmu_attach_dev() returns.
2. Devices cannot get cached translations from a domain after
arm_smmu_atc_inv_domain() returns, regardless of whether the domain is
simultaneously being attached/detached from a device.
Apart from note point 2. above, the behaviour of translations while in
the middle of an arm_smmu_attach_dev doesn't have well defined
requirements:
1. Before this patch, devices may get translations belonging to the
old domain, then aborts or identity translations, and then the new
domain while arm_smmu_attach_dev() is in progress.
2. Ater this patch, devices may get an arbitrary mix of translations
belonging to the old domain and the new domain while
arm_smmu_attach_dev() is in progress.
While disabling and re-enabling ATS inside arm_smmu_attach_dev() would
meet the same requirements as this patch, it's not optimal since the
device may still have traffic on other pasids than the one being
manipulated.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2] ARM: unwind: improve unwinders for noreturn case
From: Russell King (Oracle) @ 2024-03-21 12:22 UTC (permalink / raw)
To: David Laight
Cc: Ard Biesheuvel, 'Jiangfeng Xiao', arnd@arndb.de,
keescook@chromium.org, haibo.li@mediatek.com,
angelogioacchino.delregno@collabora.com, amergnat@baylibre.com,
akpm@linux-foundation.org, dave.hansen@linux.intel.com,
douzhaolei@huawei.com, gustavoars@kernel.org, jpoimboe@kernel.org,
kepler.chenxin@huawei.com, kirill.shutemov@linux.intel.com,
linux-hardening@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-mm@kvack.org, linux-arm-kernel@lists.infradead.org,
nixiaoming@huawei.com, peterz@infradead.org, wangbing6@huawei.com,
wangfangpeng1@huawei.com, jannh@google.com, willy@infradead.org
In-Reply-To: <db930076c837456f999daee5cb76735f@AcuMS.aculab.com>
On Thu, Mar 21, 2024 at 12:07:51PM +0000, David Laight wrote:
> From: Russell King
> > Sent: 21 March 2024 11:24
> >
> > On Thu, Mar 21, 2024 at 10:22:30AM +0000, David Laight wrote:
> > > How aggressively does the compiler optimise 'noreturn' functions?
> >
> > I've seen cases where the compiler emits a BL instruction as the very
> > last thing in the function, and nothing after it.
>
> I've also seen the compiler defer generating a stack frame until
> after an initial conditional.
.. which is why we pass -mno-sched-prolog to GCC.
> That might mean you can get the BL in the middle of a function
> but where the following instruction is for the 'no stack frame'
> side of the branch.
> That is very likely to break any stack offset calculations.
No it can't. At any one point in the function, the stack has to be in
a well defined state, so that access to local variables can work, and
also the stack can be correctly unwound. If there exists a point in
the function body which can be reached where the stack could be in two
different states, then the stack can't be restored to the parent
context.
> > This is where the problem lies - because the link register value
> > created by the BL instruction will point to the instruction after the
> > BL which will _not_ part of the function that invoked the BL. That
> > will probably cause issues for the ELF unwinder, which means this
> > issue probably goes beyond _just_ printing the function name.
>
> Isn't this already in the unwinder?
> A BL itself isn't going to fault with PC = next-instruction.
You are missing the fact that the PC can be the saved LR, and thus
can very well be the next instruction.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* RE: [PATCH v2] ARM: unwind: improve unwinders for noreturn case
From: David Laight @ 2024-03-21 12:07 UTC (permalink / raw)
To: 'Russell King', Ard Biesheuvel
Cc: 'Jiangfeng Xiao', arnd@arndb.de, keescook@chromium.org,
haibo.li@mediatek.com, angelogioacchino.delregno@collabora.com,
amergnat@baylibre.com, akpm@linux-foundation.org,
dave.hansen@linux.intel.com, douzhaolei@huawei.com,
gustavoars@kernel.org, jpoimboe@kernel.org,
kepler.chenxin@huawei.com, kirill.shutemov@linux.intel.com,
linux-hardening@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-mm@kvack.org, linux-arm-kernel@lists.infradead.org,
nixiaoming@huawei.com, peterz@infradead.org, wangbing6@huawei.com,
wangfangpeng1@huawei.com, jannh@google.com, willy@infradead.org
In-Reply-To: <ZfwYx/8k8FVONg6+@shell.armlinux.org.uk>
From: Russell King
> Sent: 21 March 2024 11:24
>
> On Thu, Mar 21, 2024 at 10:22:30AM +0000, David Laight wrote:
> > How aggressively does the compiler optimise 'noreturn' functions?
>
> I've seen cases where the compiler emits a BL instruction as the very
> last thing in the function, and nothing after it.
I've also seen the compiler defer generating a stack frame until
after an initial conditional.
That might mean you can get the BL in the middle of a function
but where the following instruction is for the 'no stack frame'
side of the branch.
That is very likely to break any stack offset calculations.
> This is where the problem lies - because the link register value
> created by the BL instruction will point to the instruction after the
> BL which will _not_ part of the function that invoked the BL. That
> will probably cause issues for the ELF unwinder, which means this
> issue probably goes beyond _just_ printing the function name.
Isn't this already in the unwinder?
A BL itself isn't going to fault with PC = next-instruction.
For pretty much all code isn't *(LR-4) going to be BL?
On arm that is probably testable.
(It is pretty much impossible to detect a ACLL on x86.)
If it is a direct BL then you'd normally expect to the be
a call the function containing the current 'PC'.
The obvious exception is if there was a tail call, and printing
the called address would then be useful.
(It might help with leaf functions that don't generate a stack frame.)
I remember issues with the solaris sparc backtrace that used to
get confused by leaf functions...
> I have vague memories that Ard has been involved in the unwinder,
> maybe he could comment on this problem? Maybe we need the unwinder
> itself to do the correction? I also wonder whether we should only
> do the correction if we detect that we're pointing at the first
> instruction of a function, and the previous instruction in the
> text segment was a BL.
It might be enough to depend on whether the address is that of a
fault (where the instruction could be retried) or from a call/trap
instruction where it will be the following instruction.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] virtio_ring: Fix the stale index in available ring
From: Gavin Shan @ 2024-03-21 12:06 UTC (permalink / raw)
To: Keir Fraser
Cc: Michael S. Tsirkin, Will Deacon, virtualization, linux-kernel,
jasowang, xuanzhuo, yihyu, shan.gavin, linux-arm-kernel,
Catalin Marinas, mochs
In-Reply-To: <ZfsZljA_mmHKkBsA@google.com>
On 3/21/24 03:15, Keir Fraser wrote:
> On Wed, Mar 20, 2024 at 03:24:16PM +1000, Gavin Shan wrote:
>>
>> Before this patch was posted, I had debugging code to record last 16 transactions
>> to the available and used queue from guest and host side. It did reveal the wrong
>> head was fetched from the available queue.
>>
>> [ 11.785745] ================ virtqueue_get_buf_ctx_split ================
>> [ 11.786238] virtio_net virtio0: output.0:id 74 is not a head!
>> [ 11.786655] head to be released: 036 077
>> [ 11.786952]
>> [ 11.786952] avail_idx:
>> [ 11.787234] 000 63985 <--
>> [ 11.787237] 001 63986
>> [ 11.787444] 002 63987
>> [ 11.787632] 003 63988
>> [ 11.787821] 004 63989
>> [ 11.788006] 005 63990
>> [ 11.788194] 006 63991
>> [ 11.788381] 007 63992
>> [ 11.788567] 008 63993
>> [ 11.788772] 009 63994
>> [ 11.788957] 010 63995
>> [ 11.789141] 011 63996
>> [ 11.789327] 012 63997
>> [ 11.789515] 013 63998
>> [ 11.789701] 014 63999
>> [ 11.789886] 015 64000
>
> Does the error always occur at such a round idx value?
>
> Here, 64000 == 0xFA00. Maybe coincidence but it's improbable enough to be interesting.
>
> This debug code seems rather useful!
>
Keir, Nope, it's just coincidence. We don't have such kind of pattern.
Thanks,
Gavin
>
>
>> [ 11.790068]
>> [ 11.790068] avail_head:
>> [ 11.790529] 000 075 <--
>> [ 11.790718] 001 036
>> [ 11.790890] 002 077
>> [ 11.791061] 003 129
>> [ 11.791231] 004 072
>> [ 11.791400] 005 130
>> [ 11.791574] 006 015
>> [ 11.791748] 007 074
>> [ 11.791918] 008 130
>> [ 11.792094] 009 130
>> [ 11.792263] 010 074
>> [ 11.792437] 011 015
>> [ 11.792617] 012 072
>> [ 11.792788] 013 129
>> [ 11.792961] 014 077 // The last two heads from guest to host: 077, 036
>> [ 11.793134] 015 036
>>
>> [root@nvidia-grace-hopper-05 qemu.main]# cat /proc/vhost
>>
>> avail_idx
>> 000 63998
>> 001 64000
>> 002 63954 <---
>> 003 63955
>> 004 63956
>> 005 63974
>> 006 63981
>> 007 63984
>> 008 63986
>> 009 63987
>> 010 63988
>> 011 63989
>> 012 63992
>> 013 63993
>> 014 63995
>> 015 63997
>>
>> avail_head
>> 000 074
>> 001 015
>> 002 072
>> 003 129
>> 004 074 // The last two heads seen by vhost is: 074, 036
>> 005 036
>> 006 075 <---
>> 007 036
>> 008 077
>> 009 129
>> 010 072
>> 011 130
>> 012 015
>> 013 074
>> 014 130
>> 015 130
>>
>> used_idx
>> 000 64000
>> 001 63882 <---
>> 002 63889
>> 003 63891
>> 004 63898
>> 005 63936
>> 006 63942
>> 007 63946
>> 008 63949
>> 009 63953
>> 010 63957
>> 011 63981
>> 012 63990
>> 013 63992
>> 014 63993
>> 015 63999
>>
>> used_head
>> 000 072
>> 001 129
>> 002 074 // The last two heads published to guest is: 074, 036
>> 003 036
>> 004 075 <---
>> 005 036
>> 006 077
>> 007 129
>> 008 072
>> 009 130
>> 010 015
>> 011 074
>> 012 130
>> 013 130
>> 014 074
>> 015 015
>>
>> Thanks,
>> Gavin
>>
>>
>>
>>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v4 1/5] dt-bindings: misc: Add mikrobus-connector
From: Vaishnav Achath @ 2024-03-21 11:55 UTC (permalink / raw)
To: Michael Walle, Andrew Lunn
Cc: Russell King (Oracle), Ayush Singh, open list, jkridner,
robertcnelson, lorforlinux, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Nishanth Menon, Vignesh Raghavendra, Tero Kristo,
Derek Kiernan, Dragan Cvetic, Arnd Bergmann, Greg Kroah-Hartman,
Mark Brown, Johan Hovold, Alex Elder,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
moderated list:ARM/TEXAS INSTRUMENTS K3 ARCHITECTURE,
open list:SPI SUBSYSTEM, moderated list:GREYBUS SUBSYSTEM,
Vaishnav M A
In-Reply-To: <CZZBT3ZMDCVI.40UX5MB6LY4I@kernel.org>
On 21/03/24 15:08, Michael Walle wrote:
> Hi,
>
>>> Is that because the current software support is too limited? Are there
>>> manufactures who want to create more complex designed, but are limited
>>> by what can be described in the manifest?
>>>
>>
>> most mikroBUS add-on boards in production lies in the category of
>> sensors, displays, connectivity, mixed signal (ADC/DAC .etc) and if you
>> look at the existing bindings under bindings/iio/ , most devices need
>> only simple descriptions and the properties are mainly standard bus
>> properties (SPI/I2C properties), IRQ, named-gpios, named properties,
>> regulators, clocks the extension to manifest was made taking this into
>> account and the named property description interface just maps the
>> manifest entries to the unified device property interface under
>> include/linux/property.h
>
> How will the ethernet boards ([1], [2]) work? Where do they get
> their MAC address from, for example. The DT has some nice properties
> for that, but I doubt that will be possible with the manifest files.
> I've looked at the manifest file for the w5500 board [3] and to me
> it looks like that board will come up with a random MAC address on
> each start. Thus, even today, you have some boards which require
> a more complex description.
>
Agreed, this is a limitation, unless the corresponding
drivers/subsystems use device_property_read_* helper to fetch
properties, it will not work and net/core/of_net.c only implements
of_get_* helpers even though the underlying functions can be implemented
with equivalent device_property_read_* equivalent as well.
> Apart from the discussion whether the manifest is a suitable or
> sufficient mechanism to describe the hardware, I think the main
> problem with the proposed binding, is that it doesn't follow the
> binding Rob was proposing for a socket. If I want to use DT
> overlays, how would you describe an add-on board?
>
> The proposal was that the base board has something like
>
> mikrobus: socket {
> compatible = "mikrobus-socket";
> i2c-parent = <&i2c0>;
> spi-parent = <&spi0>;
>
> i2c {};
> spi {};
> };
>
> an add-on board can then have a DT snippet/overlay like the
> following:
>
> &mikrobus {
> i2c {
> eeprom@52: {
> reg = <52>;
> compatible = <atmel,at24..>;
> }
> };
>
> spi {
> sensor@0: {
> reg = <0>;
> compatible = <foobar>;
> };
> };
> };
>
> That should be possible with a binding for the mikrobus, which
> in fact it is just a pin header with a standard pinout. Also as
> Russell pointed out in v3, the EEPROM/manifest is not part of the
> mikrobus standard. So maybe that deserves an own compatible, like
>
> compatible = "mikroe,click", "mikrobus-socket";
>
> Or maybe click-eeprom? Although click seems to be the brand name of
> MikroElektronika.
Agreed, there is nothing preventing us to convert the binding and update
the driver to follow the above proposed format and will be done in next
revision. Click is brand name of MikroElektronika and they don't allow
custom boards to use that branding, however clickid can be used in the
case where EEPROM is present/enable the socket to be probeable.
Thanks and Regards,
Vaishnav
>
> -michael
>
> [1] https://www.mikroe.com/eth-3-click
> [2] https://www.mikroe.com/eth-wiz-click
> [3] https://github.com/MikroElektronika/click_id/blob/main/manifests/ETH-WIZ-CLICK.mnfs
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH] arm64: Fix early handling of FEAT_E2H0 not being implemented
From: Marc Zyngier @ 2024-03-21 11:54 UTC (permalink / raw)
To: linux-arm-kernel, kvmarm
Cc: Catalin Marinas, Will Deacon, James Morse, Suzuki K Poulose,
Oliver Upton, Zenghui Yu
Commit 3944382fa6f2 introduced checks for the FEAT_E2H0 not being
implemented. However, the check is absolutely wrong and makes a
point it testing a bit that is guaranteed to be zero.
On top of that, the detection happens way too late, after the
init_el2_state has done its job.
This went undetected because the HW this was tested on has E2H being
RAO/WI, and not RES1. However, the bug shows up when run as a nested
guest, where HCR_EL2.E2H is not necessarily set to 1. As a result,
booting the kernel in hVHE mode fails with timer accesses being
cought in a trap loop (which was fun to debug).
Fix the check for ID_AA64MMFR4_EL1.E2H0, and set the HCR_EL2.E2H bit
early so that it can be checked by the rest of the init sequence.
With this, hVHE works again in a NV environment that doesn't have
FEAT_E2H0.
Fixes: 3944382fa6f2 ("arm64: Treat HCR_EL2.E2H as RES1 when ID_AA64MMFR4_EL1.E2H0 is negative")
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
arch/arm64/kernel/head.S | 29 ++++++++++++++++-------------
1 file changed, 16 insertions(+), 13 deletions(-)
diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index ce08b744aaab..06234c3a15f3 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -291,6 +291,21 @@ SYM_INNER_LABEL(init_el2, SYM_L_LOCAL)
blr x2
0:
mov_q x0, HCR_HOST_NVHE_FLAGS
+
+ /*
+ * Compliant CPUs advertise their VHE-onlyness with
+ * ID_AA64MMFR4_EL1.E2H0 < 0. HCR_EL2.E2H can be
+ * RES1 in that case. Publish the E2H bit early so that
+ * it can be picked up by the init_el2_state macro.
+ *
+ * Fruity CPUs seem to have HCR_EL2.E2H set to RAO/WI, but
+ * don't advertise it (they predate this relaxation).
+ */
+ mrs_s x1, SYS_ID_AA64MMFR4_EL1
+ tbz x1, #(ID_AA64MMFR4_EL1_E2H0_SHIFT + ID_AA64MMFR4_EL1_E2H0_WIDTH - 1), 1f
+
+ orr x0, x0, #HCR_E2H
+1:
msr hcr_el2, x0
isb
@@ -303,22 +318,10 @@ SYM_INNER_LABEL(init_el2, SYM_L_LOCAL)
mov_q x1, INIT_SCTLR_EL1_MMU_OFF
- /*
- * Compliant CPUs advertise their VHE-onlyness with
- * ID_AA64MMFR4_EL1.E2H0 < 0. HCR_EL2.E2H can be
- * RES1 in that case.
- *
- * Fruity CPUs seem to have HCR_EL2.E2H set to RES1, but
- * don't advertise it (they predate this relaxation).
- */
- mrs_s x0, SYS_ID_AA64MMFR4_EL1
- ubfx x0, x0, #ID_AA64MMFR4_EL1_E2H0_SHIFT, #ID_AA64MMFR4_EL1_E2H0_WIDTH
- tbnz x0, #(ID_AA64MMFR4_EL1_E2H0_SHIFT + ID_AA64MMFR4_EL1_E2H0_WIDTH - 1), 1f
-
mrs x0, hcr_el2
and x0, x0, #HCR_E2H
cbz x0, 2f
-1:
+
/* Set a sane SCTLR_EL1, the VHE way */
pre_disable_mmu_workaround
msr_s SYS_SCTLR_EL12, x1
--
2.39.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox