The driver always registers pin configurations in device tree. This can cause some inconvenience to users, as pin configurations in the base device tree cannot be disabled in the device tree overlay, even when the relevant devices are not used.
Ignore disabled pin configuration nodes in device tree.
Fixes: 447976ab62c5 ("pinctrl: starfive: Add StarFive JH7110 sys controller driver") Cc: stable@vger.kernel.org Signed-off-by: Nam Cao namcao@linutronix.de --- drivers/pinctrl/starfive/pinctrl-starfive-jh7110.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/pinctrl/starfive/pinctrl-starfive-jh7110.c b/drivers/pinctrl/starfive/pinctrl-starfive-jh7110.c index 640f827a9b2c..b4f799572689 100644 --- a/drivers/pinctrl/starfive/pinctrl-starfive-jh7110.c +++ b/drivers/pinctrl/starfive/pinctrl-starfive-jh7110.c @@ -135,7 +135,7 @@ static int jh7110_dt_node_to_map(struct pinctrl_dev *pctldev, int ret;
ngroups = 0; - for_each_child_of_node(np, child) + for_each_available_child_of_node(np, child) ngroups += 1; nmaps = 2 * ngroups;
@@ -150,7 +150,7 @@ static int jh7110_dt_node_to_map(struct pinctrl_dev *pctldev, nmaps = 0; ngroups = 0; mutex_lock(&sfp->mutex); - for_each_child_of_node(np, child) { + for_each_available_child_of_node(np, child) { int npins = of_property_count_u32_elems(child, "pinmux"); int *pins; u32 *pinmux;
The driver always registers pin configurations in device tree. This can cause some inconvenience to users, as pin configurations in the base device tree cannot be disabled in the device tree overlay, even when the relevant devices are not used.
Ignore disabled pin configuration nodes in device tree.
Fixes: ec648f6b7686 ("pinctrl: starfive: Add pinctrl driver for StarFive SoCs") Cc: stable@vger.kernel.org Signed-off-by: Nam Cao namcao@linutronix.de --- drivers/pinctrl/starfive/pinctrl-starfive-jh7100.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/pinctrl/starfive/pinctrl-starfive-jh7100.c b/drivers/pinctrl/starfive/pinctrl-starfive-jh7100.c index 530fe340a9a1..561fd0c6b9b0 100644 --- a/drivers/pinctrl/starfive/pinctrl-starfive-jh7100.c +++ b/drivers/pinctrl/starfive/pinctrl-starfive-jh7100.c @@ -492,7 +492,7 @@ static int starfive_dt_node_to_map(struct pinctrl_dev *pctldev,
nmaps = 0; ngroups = 0; - for_each_child_of_node(np, child) { + for_each_available_child_of_node(np, child) { int npinmux = of_property_count_u32_elems(child, "pinmux"); int npins = of_property_count_u32_elems(child, "pins");
@@ -527,7 +527,7 @@ static int starfive_dt_node_to_map(struct pinctrl_dev *pctldev, nmaps = 0; ngroups = 0; mutex_lock(&sfp->mutex); - for_each_child_of_node(np, child) { + for_each_available_child_of_node(np, child) { int npins; int i;
Nam Cao wrote:
The driver always registers pin configurations in device tree. This can cause some inconvenience to users, as pin configurations in the base device tree cannot be disabled in the device tree overlay, even when the relevant devices are not used.
Ignore disabled pin configuration nodes in device tree.
Fixes: ec648f6b7686 ("pinctrl: starfive: Add pinctrl driver for StarFive SoCs") Cc: stable@vger.kernel.org Signed-off-by: Nam Cao namcao@linutronix.de
drivers/pinctrl/starfive/pinctrl-starfive-jh7100.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/pinctrl/starfive/pinctrl-starfive-jh7100.c b/drivers/pinctrl/starfive/pinctrl-starfive-jh7100.c index 530fe340a9a1..561fd0c6b9b0 100644 --- a/drivers/pinctrl/starfive/pinctrl-starfive-jh7100.c +++ b/drivers/pinctrl/starfive/pinctrl-starfive-jh7100.c @@ -492,7 +492,7 @@ static int starfive_dt_node_to_map(struct pinctrl_dev *pctldev,
nmaps = 0; ngroups = 0;
- for_each_child_of_node(np, child) {
- for_each_available_child_of_node(np, child) {
Hi Nam,
Is this safe to do? I mean will the children considered "available" not change as drivers are loaded during boot so this is racy?
Also arguably this is not a bugfix, but a new feature.
Same comments apply to the JH7110 patch.
/Emil
int npinmux = of_property_count_u32_elems(child, "pinmux"); int npins = of_property_count_u32_elems(child, "pins");@@ -527,7 +527,7 @@ static int starfive_dt_node_to_map(struct pinctrl_dev *pctldev, nmaps = 0; ngroups = 0; mutex_lock(&sfp->mutex);
- for_each_child_of_node(np, child) {
- for_each_available_child_of_node(np, child) { int npins; int i;
-- 2.39.2
Emil Renner Berthing wrote:
Nam Cao wrote:
The driver always registers pin configurations in device tree. This can cause some inconvenience to users, as pin configurations in the base device tree cannot be disabled in the device tree overlay, even when the relevant devices are not used.
Ignore disabled pin configuration nodes in device tree.
Fixes: ec648f6b7686 ("pinctrl: starfive: Add pinctrl driver for StarFive SoCs") Cc: stable@vger.kernel.org Signed-off-by: Nam Cao namcao@linutronix.de
drivers/pinctrl/starfive/pinctrl-starfive-jh7100.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/pinctrl/starfive/pinctrl-starfive-jh7100.c b/drivers/pinctrl/starfive/pinctrl-starfive-jh7100.c index 530fe340a9a1..561fd0c6b9b0 100644 --- a/drivers/pinctrl/starfive/pinctrl-starfive-jh7100.c +++ b/drivers/pinctrl/starfive/pinctrl-starfive-jh7100.c @@ -492,7 +492,7 @@ static int starfive_dt_node_to_map(struct pinctrl_dev *pctldev,
nmaps = 0; ngroups = 0;
- for_each_child_of_node(np, child) {
- for_each_available_child_of_node(np, child) {
Hi Nam,
Is this safe to do? I mean will the children considered "available" not change as drivers are loaded during boot so this is racy?
I just noticed the Allwinner D1 device trees use /omit-if-no-ref/ in front of the pin group nodes. I think all current pin group nodes (for the JH7100 at least) are used by some peripheral, so if you're removing peripherals from the device tree you should be removing the reference too and this scheme should work for you.
/Emil
On Fri, Dec 01, 2023 at 03:43:04PM +0100, Emil Renner Berthing wrote:
I just noticed the Allwinner D1 device trees use /omit-if-no-ref/ in front of the pin group nodes. I think all current pin group nodes (for the JH7100 at least) are used by some peripheral, so if you're removing peripherals from the device tree you should be removing the reference too and this scheme should work for you.
If I am not mistaken, /omit-if-no-ref/ (and similar stuffs like /delete-node/ and /delete-property/) is only for compile-time node removal. It doesn't do anything with device tree overlay.
Best regards, Nam
Hi Emil,
On Fri, Dec 01, 2023 at 03:28:27PM +0100, Emil Renner Berthing wrote:
Nam Cao wrote:
diff --git a/drivers/pinctrl/starfive/pinctrl-starfive-jh7100.c b/drivers/pinctrl/starfive/pinctrl-starfive-jh7100.c index 530fe340a9a1..561fd0c6b9b0 100644 --- a/drivers/pinctrl/starfive/pinctrl-starfive-jh7100.c +++ b/drivers/pinctrl/starfive/pinctrl-starfive-jh7100.c @@ -492,7 +492,7 @@ static int starfive_dt_node_to_map(struct pinctrl_dev *pctldev,
nmaps = 0; ngroups = 0;
- for_each_child_of_node(np, child) {
- for_each_available_child_of_node(np, child) {
Is this safe to do? I mean will the children considered "available" not change as drivers are loaded during boot so this is racy?
I think if node removal like this causes race condition, we would already have race condition with node addition too: "what if the nodes are added while the drivers are being loaded?"
At least with U-Boot, the device tree overlay is "merged" into the base device tree, before the kernel even runs, so no race there. I don't know if there are any cases where the device tree overlay is not guaranteed to be applied before driver loading, but those cases do not sound sane to me: they would cause race condition, regardless of whether nodes are added or removed.
Also arguably this is not a bugfix, but a new feature.
I'm not sure myself, I haven't seen official documentation/rules about this. But many people do consider this to be a bug:
"Though you can add/override 'status' with 'status = "disabled";' which should be treated very similar to a node not being present. I say similar because it's a source of bugs for the OS to fail to pay attention to status property." - Rob Herring [1].
"Linux has widespread use of the "status" property to indicate that a node does not exist. (...). Expect efforts to fix the kernel code to respect the "status" property." - elinux.org [2].
And I do agree with them. When someone write a device tree with some nodes with "status = disabled" for whatever reasons, clearly they intend to exclude these nodes.
Though I must admit that I am still quite new, so please correct me if my reasoning/understanding is flawed.
Best regards, Nam
[1] https://lore.kernel.org/lkml/CAL_JsqLV5d5cL3o3Dx=--zGD37c5O09rL9AXyRFmceTfBH... [2] https://elinux.org/Device_Tree_Linux#status_property
On Fri, Dec 1, 2023 at 10:23 AM Nam Cao namcao@linutronix.de wrote:
The driver always registers pin configurations in device tree. This can cause some inconvenience to users, as pin configurations in the base device tree cannot be disabled in the device tree overlay, even when the relevant devices are not used.
Ignore disabled pin configuration nodes in device tree.
Fixes: ec648f6b7686 ("pinctrl: starfive: Add pinctrl driver for StarFive SoCs") Cc: stable@vger.kernel.org Signed-off-by: Nam Cao namcao@linutronix.de
Patch applied for fixes.
If there is some doubts about the saneness of this patch, seek input from the devicetree maintainers.
Yours, Linus Walleij
On Fri, Dec 1, 2023 at 10:23 AM Nam Cao namcao@linutronix.de wrote:
The driver always registers pin configurations in device tree. This can cause some inconvenience to users, as pin configurations in the base device tree cannot be disabled in the device tree overlay, even when the relevant devices are not used.
Ignore disabled pin configuration nodes in device tree.
Fixes: 447976ab62c5 ("pinctrl: starfive: Add StarFive JH7110 sys controller driver") Cc: stable@vger.kernel.org Signed-off-by: Nam Cao namcao@linutronix.de
This patch (1/2) applied for fixes.
Yours, Linus Walleij
linux-stable-mirror@lists.linaro.org
-
Emil Renner Berthing -
Linus Walleij -
Nam Cao