| Message ID | 20230720-ti-mdio-pinmux-v1-0-0bd3bd1cf759@kernel.org |
|---|---|
| Headers | show |
| Series | net: ti: am65-cpsw-nuss: Fix DT binding handling of pinctrl | expand |
Hi Sorry, I didn't receive Roger mail so I'll reply here On Thu, Jul 20, 2023 at 09:00:19AM -0500, Nishanth Menon wrote: > On 16:56-20230720, Roger Quadros wrote: > > Hi, > > > > On 20/07/2023 16:33, Ravi Gunasekaran wrote: > > > > > > > > > On 7/20/2023 3:25 PM, Maxime Ripard wrote: > > >> Hi, > > >> > > >> This series is based on: > > >> https://lore.kernel.org/all/20230713072019.3153871-1-nm@ti.com/ > > >> > > >> It fixes the issue of Linux booting from the DT embedded by U-boot. The > > >> main issue there is that U-Boot doesn't handle the MDIO child node that > > >> might have resources attached to it. > > >> > > >> Thus, any pinctrl configuration that could be attached to the MDIO > > >> child node is effectively ignored. Unfortunately, starting with 6.5-rc1, > > >> Linux does just that. > > > > I didn't get this part. Linux does not ignore pinctrl configuration attached > > to MDIO child node. What changed in 6.5-rc1? Linux doesn't ignore it, but Linux started putting pinctrl configuration on the MDIO node, which is ignored by U-Boot. > > >> This was solved by duplicating the pinctrl configuration onto the MAC > > > > If I remember right, there is no driver model driver for MDIO in u-boot and > > adding the mdio pinctrl into CPSW DT node was a hack in u-boot. Yeah, but we now have in the U-Boot DT two nodes referencing the same pinctrl configuration: the CPSW and the MDIO node. Linux then tries to assign that configuration to both devices and it fails. > > >> device node. Unfortunately, this doesn't work for Linux since now it has > > >> two devices competing for the same pins. > > > > What has really changed here is that you are passing u-boot's device > > tree to Linux. I didn't change anything. This is the default boot process if you don't provide a DT in the ESP partition. > > This has nothing to do with 6.5-rc1 right? The issue started to occur with Nishanth patch to sync with Linux 6.5-rc1 device trees, so there's definitely a connection. > > I suppose your solution is still a hack but of lesser evil than > > duplicating MDIO pinctrl in CPSW node. > > > > The proper solution would be to implement driver model for the > > davinci MDIO driver. Siddharth has been working on this. If that is > > close to ready we should just use that instead. > > But this allows for a cleaner device tree while the driver can be fixed > up independently, correct? Unfortunately, Siddharth's driver model work, > from what I understand, is around 2024 timeframe.. which is probably not > something that helps us in the community at this point. Yeah, at the moment I have to choose between getting the MMC to work in Linux or the Ethernet ports. The former is working thanks to your patch, the latter is broken because of it. Ideally I'd like both :) Maxime
On 20/07/2023 17:12, Maxime Ripard wrote: > Hi > > Sorry, I didn't receive Roger mail so I'll reply here > > On Thu, Jul 20, 2023 at 09:00:19AM -0500, Nishanth Menon wrote: >> On 16:56-20230720, Roger Quadros wrote: >>> Hi, >>> >>> On 20/07/2023 16:33, Ravi Gunasekaran wrote: >>>> >>>> >>>> On 7/20/2023 3:25 PM, Maxime Ripard wrote: >>>>> Hi, >>>>> >>>>> This series is based on: >>>>> https://lore.kernel.org/all/20230713072019.3153871-1-nm@ti.com/ >>>>> >>>>> It fixes the issue of Linux booting from the DT embedded by U-boot. The >>>>> main issue there is that U-Boot doesn't handle the MDIO child node that >>>>> might have resources attached to it. >>>>> >>>>> Thus, any pinctrl configuration that could be attached to the MDIO >>>>> child node is effectively ignored. Unfortunately, starting with 6.5-rc1, >>>>> Linux does just that. >>> >>> I didn't get this part. Linux does not ignore pinctrl configuration attached >>> to MDIO child node. What changed in 6.5-rc1? > > Linux doesn't ignore it, but Linux started putting pinctrl configuration > on the MDIO node, which is ignored by U-Boot. Linux always had MDIO pinctrl configuration in the MDIO node, way before 6.5-rc1. Let's not mention 6.5-rc1 here as it gives an indication that something in 6.5-rc1 caused this issue. ;) Earlier, u-boot didn't enable the MDIO node. With Nishanth's sync it does which is fine, but duplicating the MDIO pinctrl node in the CPSW node causes problems on Linux. I see you reverted that change in patch 3, but probably Nishanth can avoid it if we got this route. > >>>>> This was solved by duplicating the pinctrl configuration onto the MAC >>> >>> If I remember right, there is no driver model driver for MDIO in u-boot and >>> adding the mdio pinctrl into CPSW DT node was a hack in u-boot. > > Yeah, but we now have in the U-Boot DT two nodes referencing the same > pinctrl configuration: the CPSW and the MDIO node. Linux then tries to > assign that configuration to both devices and it fails. Understood. > >>>>> device node. Unfortunately, this doesn't work for Linux since now it has >>>>> two devices competing for the same pins. >>> >>> What has really changed here is that you are passing u-boot's device >>> tree to Linux. > > I didn't change anything. This is the default boot process if you don't > provide a DT in the ESP partition. > OK. >>> This has nothing to do with 6.5-rc1 right? > > The issue started to occur with Nishanth patch to sync with Linux > 6.5-rc1 device trees, so there's definitely a connection. > >>> I suppose your solution is still a hack but of lesser evil than >>> duplicating MDIO pinctrl in CPSW node. >>> >>> The proper solution would be to implement driver model for the >>> davinci MDIO driver. Siddharth has been working on this. If that is >>> close to ready we should just use that instead. >> >> But this allows for a cleaner device tree while the driver can be fixed >> up independently, correct? Unfortunately, Siddharth's driver model work, Yes. MDIO pinctrl no longer needs to be duplicated in CPSW node at u-boot. >> from what I understand, is around 2024 timeframe.. which is probably not >> something that helps us in the community at this point. OK, then we need to resort to some temporary solution like this then. > > Yeah, at the moment I have to choose between getting the MMC to work in > Linux or the Ethernet ports. The former is working thanks to your patch, > the latter is broken because of it. Ideally I'd like both :) >
On 17:41-20230720, Roger Quadros wrote: [...] > >> from what I understand, is around 2024 timeframe.. which is probably not > >> something that helps us in the community at this point. > > OK, then we need to resort to some temporary solution like this then. Thanks. I will squash up the fixup patches, lets try and get the rest of the patches reviewed? I can put out a new RFC based on this series (or updates).
Maxime, On 7/20/23 7:42 PM, Maxime Ripard wrote: > Hi > > Sorry, I didn't receive Roger mail so I'll reply here > > On Thu, Jul 20, 2023 at 09:00:19AM -0500, Nishanth Menon wrote: >> On 16:56-20230720, Roger Quadros wrote: >>> Hi, >>> >>> On 20/07/2023 16:33, Ravi Gunasekaran wrote: >>>> >>>> >>>> On 7/20/2023 3:25 PM, Maxime Ripard wrote: >>>>> Hi, >>>>> >>>>> This series is based on: >>>>> https://lore.kernel.org/all/20230713072019.3153871-1-nm@ti.com/ >>>>> >>>>> It fixes the issue of Linux booting from the DT embedded by U-boot. The >>>>> main issue there is that U-Boot doesn't handle the MDIO child node that >>>>> might have resources attached to it. >>>>> >>>>> Thus, any pinctrl configuration that could be attached to the MDIO >>>>> child node is effectively ignored. Unfortunately, starting with 6.5-rc1, >>>>> Linux does just that. >>> >>> I didn't get this part. Linux does not ignore pinctrl configuration attached >>> to MDIO child node. What changed in 6.5-rc1? > > Linux doesn't ignore it, but Linux started putting pinctrl configuration > on the MDIO node, which is ignored by U-Boot. > >>>>> This was solved by duplicating the pinctrl configuration onto the MAC >>> >>> If I remember right, there is no driver model driver for MDIO in u-boot and >>> adding the mdio pinctrl into CPSW DT node was a hack in u-boot. > > Yeah, but we now have in the U-Boot DT two nodes referencing the same > pinctrl configuration: the CPSW and the MDIO node. Linux then tries to > assign that configuration to both devices and it fails. This response might be late, as I know things have moved ahead after this discussion. But I just wanted to understand the root cause little bit more. Is the issue mainly because the same mdio pinctrl node(phandle) is referenced in two other nodes? Or is it because of same pins being updated in two different places in the kernel? If it is the former, then would a duplicate mdio node help keep the changes to minimum? > >>>>> device node. Unfortunately, this doesn't work for Linux since now it has >>>>> two devices competing for the same pins. >>> >>> What has really changed here is that you are passing u-boot's device >>> tree to Linux. > > I didn't change anything. This is the default boot process if you don't > provide a DT in the ESP partition. > >>> This has nothing to do with 6.5-rc1 right? > > The issue started to occur with Nishanth patch to sync with Linux > 6.5-rc1 device trees, so there's definitely a connection. > >>> I suppose your solution is still a hack but of lesser evil than >>> duplicating MDIO pinctrl in CPSW node. >>> >>> The proper solution would be to implement driver model for the >>> davinci MDIO driver. Siddharth has been working on this. If that is >>> close to ready we should just use that instead. >> >> But this allows for a cleaner device tree while the driver can be fixed >> up independently, correct? Unfortunately, Siddharth's driver model work, >> from what I understand, is around 2024 timeframe.. which is probably not >> something that helps us in the community at this point. > > Yeah, at the moment I have to choose between getting the MMC to work in > Linux or the Ethernet ports. The former is working thanks to your patch, > the latter is broken because of it. Ideally I'd like both :) > > Maxime
Hi Ravi, On Wed, Jul 26, 2023 at 02:44:00PM +0530, Ravi Gunasekaran wrote: > On 7/20/23 7:42 PM, Maxime Ripard wrote: > > Hi > > > > Sorry, I didn't receive Roger mail so I'll reply here > > > > On Thu, Jul 20, 2023 at 09:00:19AM -0500, Nishanth Menon wrote: > >> On 16:56-20230720, Roger Quadros wrote: > >>> Hi, > >>> > >>> On 20/07/2023 16:33, Ravi Gunasekaran wrote: > >>>> > >>>> > >>>> On 7/20/2023 3:25 PM, Maxime Ripard wrote: > >>>>> Hi, > >>>>> > >>>>> This series is based on: > >>>>> https://lore.kernel.org/all/20230713072019.3153871-1-nm@ti.com/ > >>>>> > >>>>> It fixes the issue of Linux booting from the DT embedded by U-boot. The > >>>>> main issue there is that U-Boot doesn't handle the MDIO child node that > >>>>> might have resources attached to it. > >>>>> > >>>>> Thus, any pinctrl configuration that could be attached to the MDIO > >>>>> child node is effectively ignored. Unfortunately, starting with 6.5-rc1, > >>>>> Linux does just that. > >>> > >>> I didn't get this part. Linux does not ignore pinctrl configuration attached > >>> to MDIO child node. What changed in 6.5-rc1? > > > > Linux doesn't ignore it, but Linux started putting pinctrl configuration > > on the MDIO node, which is ignored by U-Boot. > > > >>>>> This was solved by duplicating the pinctrl configuration onto the MAC > >>> > >>> If I remember right, there is no driver model driver for MDIO in u-boot and > >>> adding the mdio pinctrl into CPSW DT node was a hack in u-boot. > > > > Yeah, but we now have in the U-Boot DT two nodes referencing the same > > pinctrl configuration: the CPSW and the MDIO node. Linux then tries to > > assign that configuration to both devices and it fails. > > This response might be late, as I know things have moved ahead after this > discussion. But I just wanted to understand the root cause little bit more. > Is the issue mainly because the same mdio pinctrl node(phandle) is referenced in > two other nodes? Or is it because of same pins being updated in two different > places in the kernel? > > If it is the former, then would a duplicate mdio node help keep the changes > to minimum? Neither, actually :/ The issue is that, with the changes made by Nishanth to bring the 6.5-rc1 DTS in, the same pinctrl node is referenced in two separate nodes, and Linux sees that as conflicting users of the same pins. One quick workaround would be to remove the MDIO pinctrl property, and add it to the MAC pinctrl property. That's fairly dangerous if either get extended though, so it might not be a proper solution long term. I hope it's clearer Maxime
On 14:44-20230726, Ravi Gunasekaran wrote: [...] > If it is the former, then would a duplicate mdio node help keep the changes > to minimum? That is worse hack. How does it make sense to have two mdio nodes to represent the same hardware block? we are trying to get closer to kernel dts support not farther away from it.
Maxime, On 7/26/23 6:19 PM, Maxime Ripard wrote: > Hi Ravi, > > On Wed, Jul 26, 2023 at 02:44:00PM +0530, Ravi Gunasekaran wrote: >> On 7/20/23 7:42 PM, Maxime Ripard wrote: >>> Hi >>> >>> Sorry, I didn't receive Roger mail so I'll reply here >>> >>> On Thu, Jul 20, 2023 at 09:00:19AM -0500, Nishanth Menon wrote: >>>> On 16:56-20230720, Roger Quadros wrote: >>>>> Hi, >>>>> >>>>> On 20/07/2023 16:33, Ravi Gunasekaran wrote: >>>>>> >>>>>> >>>>>> On 7/20/2023 3:25 PM, Maxime Ripard wrote: >>>>>>> Hi, >>>>>>> >>>>>>> This series is based on: >>>>>>> https://lore.kernel.org/all/20230713072019.3153871-1-nm@ti.com/ >>>>>>> >>>>>>> It fixes the issue of Linux booting from the DT embedded by U-boot. The >>>>>>> main issue there is that U-Boot doesn't handle the MDIO child node that >>>>>>> might have resources attached to it. >>>>>>> >>>>>>> Thus, any pinctrl configuration that could be attached to the MDIO >>>>>>> child node is effectively ignored. Unfortunately, starting with 6.5-rc1, >>>>>>> Linux does just that. >>>>> >>>>> I didn't get this part. Linux does not ignore pinctrl configuration attached >>>>> to MDIO child node. What changed in 6.5-rc1? >>> >>> Linux doesn't ignore it, but Linux started putting pinctrl configuration >>> on the MDIO node, which is ignored by U-Boot. >>> >>>>>>> This was solved by duplicating the pinctrl configuration onto the MAC >>>>> >>>>> If I remember right, there is no driver model driver for MDIO in u-boot and >>>>> adding the mdio pinctrl into CPSW DT node was a hack in u-boot. >>> >>> Yeah, but we now have in the U-Boot DT two nodes referencing the same >>> pinctrl configuration: the CPSW and the MDIO node. Linux then tries to >>> assign that configuration to both devices and it fails. >> >> This response might be late, as I know things have moved ahead after this >> discussion. But I just wanted to understand the root cause little bit more. >> Is the issue mainly because the same mdio pinctrl node(phandle) is referenced in >> two other nodes? Or is it because of same pins being updated in two different >> places in the kernel? >> >> If it is the former, then would a duplicate mdio node help keep the changes >> to minimum? > > Neither, actually :/ The issue is that, with the changes made by > Nishanth to bring the 6.5-rc1 DTS in, the same pinctrl node is > referenced in two separate nodes, and Linux sees that as conflicting > users of the same pins. So you mean to say, finally it boils down to the users of the same pins. Even if there are two nodes with the same pinctrl configuration, we would still hit the issue. > > One quick workaround would be to remove the MDIO pinctrl property, and > add it to the MAC pinctrl property. > > That's fairly dangerous if either get extended though, so it might not > be a proper solution long term. A proper solution would be to update the MDIO driver. I'm sorry that it doesn't follow the standard driver model. We have plans to fix it soon. > > I hope it's clearer > > Maxime
On 7/26/23 6:22 PM, Nishanth Menon wrote: > On 14:44-20230726, Ravi Gunasekaran wrote: > [...] >> If it is the former, then would a duplicate mdio node help keep the changes >> to minimum? > > That is worse hack. How does it make sense to have two mdio nodes to > represent the same hardware block? we are trying to get closer to kernel > dts support not farther away from it. > I know it's not a clean hack. In k3-am625-sk-u-boot.dtsi, as a hack, the mdio pinctrl gets added to cpsw node. So was wondering if a duplicate mdio pinctrl node could be added in the same file. Just wanted to know if we could skip the changes in CPSW driver posted by Maxime. But Maxime's approach is much cleaner. We can just sync up kernel dts and not keeping adding the hack to every K3 SoC's DT.
Ravi, On 27/07/2023 08:36, Ravi Gunasekaran wrote: > > > On 7/26/23 6:22 PM, Nishanth Menon wrote: >> On 14:44-20230726, Ravi Gunasekaran wrote: >> [...] >>> If it is the former, then would a duplicate mdio node help keep the changes >>> to minimum? >> >> That is worse hack. How does it make sense to have two mdio nodes to >> represent the same hardware block? we are trying to get closer to kernel >> dts support not farther away from it. >> > > I know it's not a clean hack. In k3-am625-sk-u-boot.dtsi, as a hack, > the mdio pinctrl gets added to cpsw node. So was wondering if a duplicate > mdio pinctrl node could be added in the same file. Just wanted to know if we > could skip the changes in CPSW driver posted by Maxime. > > But Maxime's approach is much cleaner. We can just sync up kernel dts and not > keeping adding the hack to every K3 SoC's DT. > > Everything is sorted now as far as DT sync is concerned. Please see [1] and its dependencies. Only thing missing is to have proper MDIO DM support for TI platforms in u-boot. Then we can get rid of the MDIO hack in am65-cpsw driver. [1] - https://lore.kernel.org/all/20230726151027.2517151-1-nm@ti.com/
Hi, This series is based on: https://lore.kernel.org/all/20230713072019.3153871-1-nm@ti.com/ It fixes the issue of Linux booting from the DT embedded by U-boot. The main issue there is that U-Boot doesn't handle the MDIO child node that might have resources attached to it. Thus, any pinctrl configuration that could be attached to the MDIO child node is effectively ignored. Unfortunately, starting with 6.5-rc1, Linux does just that. This was solved by duplicating the pinctrl configuration onto the MAC device node. Unfortunately, this doesn't work for Linux since now it has two devices competing for the same pins. Let me know what you think, Maxime Signed-off-by: Maxime Ripard <mripard@kernel.org> --- Maxime Ripard (4): pinctrl: Create a select_state variant with the ofnode net: ti: am65-cpsw-nuss: Enforce pinctrl state on the MDIO child node fixup! arm: dts: k3-am62: Bump dtsi from linux v6.5-rc1 fixup! arm: dts: k3-am62: Bump dtsi from linux v6.5-rc1 arch/arm/dts/k3-am625-sk-u-boot.dtsi | 7 ++++-- drivers/net/ti/am65-cpsw-nuss.c | 49 ++++++++++++++++++++++++++++++++++++ drivers/pinctrl/pinctrl-uclass.c | 15 ++++++----- include/dm/pinctrl.h | 26 ++++++++++++++----- 4 files changed, 83 insertions(+), 14 deletions(-) --- base-commit: acff6e7c553d5a839e885730a4018465a34ba5a7 change-id: 20230720-ti-mdio-pinmux-a12525dba973 Best regards,