| Message ID | 20220509125510.152114-1-carsten.haitzler@foss.arm.com (mailing list archive) |
|---|---|
| State | Queued |
| Headers | show |
| Series | arm64: defconfig: Enable modules for arm displays | expand |
+ Liviu, Robin On Mon, May 09, 2022 at 01:55:10PM +0100, carsten.haitzler@foss.arm.com wrote: > Key devices that support displays on SoCs like the Komeda DRM driver, the > older HDLCD were not enabled by default and should be so displays can work > out of the box on defconfig. Also Candence I2C support should be enabled so > the PHY and thus displays can work too. > While at it, can we get these too enabled which are needed on JUNO IIUC. CONFIG_DRM_I2C_NXP_TDA998X CONFIG_DRM_I2C_NXP_TDA9950 Liviu/Robin who use HDLCD more than me on Juno can confirm if these are needed or not. I seem to have these for some HDLCD clock testing using SCMI.
On Tue, May 10, 2022 at 12:15:45PM +0100, Sudeep Holla wrote: > While at it, can we get these too enabled which are needed on JUNO IIUC. > CONFIG_DRM_I2C_NXP_TDA998X > CONFIG_DRM_I2C_NXP_TDA9950 > Liviu/Robin who use HDLCD more than me on Juno can confirm if these are > needed or not. I seem to have these for some HDLCD clock testing using SCMI. Those two, especially TDA998x, are very widely used so should probably just be enabled anyway regardless of what's on Juno.
On 2022-05-10 12:27, Mark Brown wrote: > On Tue, May 10, 2022 at 12:15:45PM +0100, Sudeep Holla wrote: > >> While at it, can we get these too enabled which are needed on JUNO IIUC. >> CONFIG_DRM_I2C_NXP_TDA998X >> CONFIG_DRM_I2C_NXP_TDA9950 > >> Liviu/Robin who use HDLCD more than me on Juno can confirm if these are >> needed or not. I seem to have these for some HDLCD clock testing using SCMI. > > Those two, especially TDA998x, are very widely used so should probably > just be enabled anyway regardless of what's on Juno. Yeah, DRM_I2C_NXP_TDA998X and I2C_DESIGNWARE_PLATFORM are what's needed for Juno, and they've been enabled already for a long time now, so we're good. Thanks, Robin.
On Tue, May 10, 2022 at 12:56:15PM +0100, Robin Murphy wrote: > On 2022-05-10 12:27, Mark Brown wrote: > > On Tue, May 10, 2022 at 12:15:45PM +0100, Sudeep Holla wrote: > > > > > While at it, can we get these too enabled which are needed on JUNO IIUC. > > > CONFIG_DRM_I2C_NXP_TDA998X > > > CONFIG_DRM_I2C_NXP_TDA9950 > > > > > Liviu/Robin who use HDLCD more than me on Juno can confirm if these are > > > needed or not. I seem to have these for some HDLCD clock testing using SCMI. > > > > Those two, especially TDA998x, are very widely used so should probably > > just be enabled anyway regardless of what's on Juno. > > Yeah, DRM_I2C_NXP_TDA998X and I2C_DESIGNWARE_PLATFORM are what's needed for > Juno, and they've been enabled already for a long time now, so we're good. > Ah my bad, so I haven't refreshed my listed then. I just append to defconfig, which must have complained for duplicates and I am ignoring
On Tue, May 10, 2022 at 12:27:50PM +0100, Mark Brown wrote: > On Tue, May 10, 2022 at 12:15:45PM +0100, Sudeep Holla wrote: > > > While at it, can we get these too enabled which are needed on JUNO IIUC. > > CONFIG_DRM_I2C_NXP_TDA998X > > CONFIG_DRM_I2C_NXP_TDA9950 > > > Liviu/Robin who use HDLCD more than me on Juno can confirm if these are > > needed or not. I seem to have these for some HDLCD clock testing using SCMI. > > Those two, especially TDA998x, are very widely used so should probably > just be enabled anyway regardless of what's on Juno. Indeed, Robin mentioned the same. I am just adding it blindly every time I need to test HDLCD without noticing any warnings.
On 5/10/22 13:07, Sudeep Holla wrote: > On Tue, May 10, 2022 at 12:27:50PM +0100, Mark Brown wrote: >> On Tue, May 10, 2022 at 12:15:45PM +0100, Sudeep Holla wrote: >> >>> While at it, can we get these too enabled which are needed on JUNO IIUC. >>> CONFIG_DRM_I2C_NXP_TDA998X >>> CONFIG_DRM_I2C_NXP_TDA9950 >> >>> Liviu/Robin who use HDLCD more than me on Juno can confirm if these are >>> needed or not. I seem to have these for some HDLCD clock testing using SCMI. >> >> Those two, especially TDA998x, are very widely used so should probably >> just be enabled anyway regardless of what's on Juno. > > Indeed, Robin mentioned the same. I am just adding it blindly every time > I need to test HDLCD without noticing any warnings. > Yup - those were already there. I was just adding these for our new Morello board - all it needed was I2C Cadence enabled as a module and presto... it "just works". As it's just a module it's not really a bad "cost" so defconfig additions seemed the right thing to do. Just chasing up this patch as I sent it earlier this year and it was not merged or denied... :)
On Tue, May 10, 2022 at 12:15:45PM +0100, Sudeep Holla wrote: > + Liviu, Robin > > On Mon, May 09, 2022 at 01:55:10PM +0100, carsten.haitzler@foss.arm.com wrote: > > Key devices that support displays on SoCs like the Komeda DRM driver, the > > older HDLCD were not enabled by default and should be so displays can work > > out of the box on defconfig. Also Candence I2C support should be enabled so > > the PHY and thus displays can work too. > > > > While at it, can we get these too enabled which are needed on JUNO IIUC. > CONFIG_DRM_I2C_NXP_TDA998X > CONFIG_DRM_I2C_NXP_TDA9950 I would not enable by default TDA9950 as that one is for CEC, which Juno doesn't implement so you get warnings everytime time tda998x probes. Best regards, Liviu > > Liviu/Robin who use HDLCD more than me on Juno can confirm if these are > needed or not. I seem to have these for some HDLCD clock testing using SCMI. > > -- > Regards, > Sudeep
On Tue, May 10, 2022 at 01:55:13PM +0100, Liviu Dudau wrote: > On Tue, May 10, 2022 at 12:15:45PM +0100, Sudeep Holla wrote: > > While at it, can we get these too enabled which are needed on JUNO IIUC. > > CONFIG_DRM_I2C_NXP_TDA998X > > CONFIG_DRM_I2C_NXP_TDA9950 > I would not enable by default TDA9950 as that one is for CEC, which Juno doesn't > implement so you get warnings everytime time tda998x probes. If it's generating a warning due to the config option being enabled that doesn't sound good - kernels are supposed to be multiplatform.
On 5/10/22 14:05, Mark Brown wrote: > On Tue, May 10, 2022 at 01:55:13PM +0100, Liviu Dudau wrote: >> On Tue, May 10, 2022 at 12:15:45PM +0100, Sudeep Holla wrote: > >>> While at it, can we get these too enabled which are needed on JUNO IIUC. >>> CONFIG_DRM_I2C_NXP_TDA998X >>> CONFIG_DRM_I2C_NXP_TDA9950 > >> I would not enable by default TDA9950 as that one is for CEC, which Juno doesn't >> implement so you get warnings everytime time tda998x probes. > > If it's generating a warning due to the config option being enabled > that doesn't sound good - kernels are supposed to be multiplatform. is it the: [ 7.471809] tda9950 0-0034: driver requires an interrupt that you're talking about Liviu?
On Tue, May 10, 2022 at 01:34:00PM +0100, Carsten Haitzler wrote: > > > On 5/10/22 13:07, Sudeep Holla wrote: > > On Tue, May 10, 2022 at 12:27:50PM +0100, Mark Brown wrote: > > > On Tue, May 10, 2022 at 12:15:45PM +0100, Sudeep Holla wrote: > > > > > > > While at it, can we get these too enabled which are needed on JUNO IIUC. > > > > CONFIG_DRM_I2C_NXP_TDA998X > > > > CONFIG_DRM_I2C_NXP_TDA9950 > > > > > > > Liviu/Robin who use HDLCD more than me on Juno can confirm if these are > > > > needed or not. I seem to have these for some HDLCD clock testing using SCMI. > > > > > > Those two, especially TDA998x, are very widely used so should probably > > > just be enabled anyway regardless of what's on Juno. > > > > Indeed, Robin mentioned the same. I am just adding it blindly every time > > I need to test HDLCD without noticing any warnings. > > > > Yup - those were already there. I was just adding these for our new Morello > board - all it needed was I2C Cadence enabled as a module and presto... it > "just works". As it's just a module it's not really a bad "cost" so > defconfig additions seemed the right thing to do. Just chasing up this patch > as I sent it earlier this year and it was not merged or denied... :) Was it sent to soc@kernel.org earlier as well ? If so, let me know. I am not sure if the SoC team expects this to come from some platform maintainer or atleast an ack from them so that they can pick up. We can just say it is need for Arm Ltd platforms and provide an ack(me or Liviu).
On 5/10/22 15:51, Sudeep Holla wrote: > On Tue, May 10, 2022 at 01:34:00PM +0100, Carsten Haitzler wrote: >> >> On 5/10/22 13:07, Sudeep Holla wrote: >>> On Tue, May 10, 2022 at 12:27:50PM +0100, Mark Brown wrote: >>>> On Tue, May 10, 2022 at 12:15:45PM +0100, Sudeep Holla wrote: >>>> >>>>> While at it, can we get these too enabled which are needed on JUNO IIUC. >>>>> CONFIG_DRM_I2C_NXP_TDA998X >>>>> CONFIG_DRM_I2C_NXP_TDA9950 >>>>> Liviu/Robin who use HDLCD more than me on Juno can confirm if these are >>>>> needed or not. I seem to have these for some HDLCD clock testing using SCMI. >>>> Those two, especially TDA998x, are very widely used so should probably >>>> just be enabled anyway regardless of what's on Juno. >>> Indeed, Robin mentioned the same. I am just adding it blindly every time >>> I need to test HDLCD without noticing any warnings. >>> >> Yup - those were already there. I was just adding these for our new Morello >> board - all it needed was I2C Cadence enabled as a module and presto... it >> "just works". As it's just a module it's not really a bad "cost" so >> defconfig additions seemed the right thing to do. Just chasing up this patch >> as I sent it earlier this year and it was not merged or denied... :) > Was it sent to soc@kernel.org earlier as well ? If so, let me know. I am > not sure if the SoC team expects this to come from some platform maintainer > or atleast an ack from them so that they can pick up. We can just say it > is need for Arm Ltd platforms and provide an ack(me or Liviu). > Yup- I sent there earlier. Arnd has picked it up. I missed his reply earlier and was hunting git repos for it. So don't worry about this patch - it's on it's way in.
On Tue, May 10, 2022 at 03:05:21PM +0100, Carsten Haitzler wrote: > > > On 5/10/22 14:05, Mark Brown wrote: > > On Tue, May 10, 2022 at 01:55:13PM +0100, Liviu Dudau wrote: > > > On Tue, May 10, 2022 at 12:15:45PM +0100, Sudeep Holla wrote: > > > > > > While at it, can we get these too enabled which are needed on JUNO IIUC. > > > > CONFIG_DRM_I2C_NXP_TDA998X > > > > CONFIG_DRM_I2C_NXP_TDA9950 > > > > > I would not enable by default TDA9950 as that one is for CEC, which Juno doesn't > > > implement so you get warnings everytime time tda998x probes. > > > > If it's generating a warning due to the config option being enabled > > that doesn't sound good - kernels are supposed to be multiplatform. > > is it the: > > [ 7.471809] tda9950 0-0034: driver requires an interrupt > > that you're talking about Liviu? Yes, that is the warning. Best regards, Liviu
On Tue, May 10, 2022 at 05:19:04PM +0100, Liviu Dudau wrote: > On Tue, May 10, 2022 at 03:05:21PM +0100, Carsten Haitzler wrote: > > is it the: > > [ 7.471809] tda9950 0-0034: driver requires an interrupt > > that you're talking about Liviu? > Yes, that is the warning. That doesn't look too worrying TBH, if it's causing confusion we should just either fix the tda998x driver to not instantiate the tda9950 unless it has an interrupt or lower the severity of the log message in the tda9950 code. It's certanly not something that should block use in a defconfig. Ideally we'd not be using the wildcard compatible in the tda998x and would know if the CEC functionality was there but I think that ship has disappeared over the horizon at this point, and in any case someone might just not want to support CEC for some reason.
On Tue, May 10, 2022 at 05:31:11PM +0100, Mark Brown wrote: > On Tue, May 10, 2022 at 05:19:04PM +0100, Liviu Dudau wrote: > > On Tue, May 10, 2022 at 03:05:21PM +0100, Carsten Haitzler wrote: > > > > is it the: > > > > [ 7.471809] tda9950 0-0034: driver requires an interrupt > > > > that you're talking about Liviu? > > > Yes, that is the warning. > > That doesn't look too worrying TBH, if it's causing confusion we should > just either fix the tda998x driver to not instantiate the tda9950 unless > it has an interrupt or lower the severity of the log message in the > tda9950 code. It's certanly not something that should block use in a > defconfig. > > Ideally we'd not be using the wildcard compatible in the tda998x and > would know if the CEC functionality was there but I think that ship has > disappeared over the horizon at this point, and in any case someone > might just not want to support CEC for some reason. The quickest way is to harmonise the treatment of IRQs between tda998x and tda9950. The former treats the IRQ as optional, the later as mandatory. Given that (according to comments in the code) TDA998X is actually a TDA9989+TDA9950, it makes sense to treat the lack of IRQ the same way (make it optional, IMHO). I agree that that should not block adding the two config options to the defconfig. It's just that using Juno as an argument for doing so when tda9950 returns ENXIO on that platform can confuse people. Best regards, Liviu
On 5/10/22 17:31, Mark Brown wrote: > On Tue, May 10, 2022 at 05:19:04PM +0100, Liviu Dudau wrote: >> On Tue, May 10, 2022 at 03:05:21PM +0100, Carsten Haitzler wrote: > >>> is it the: > >>> [ 7.471809] tda9950 0-0034: driver requires an interrupt > >>> that you're talking about Liviu? > >> Yes, that is the warning. > > That doesn't look too worrying TBH, if it's causing confusion we should Looks un-worrying to me. It (to my eyes) looks like an overzealous driver that should silently fail to load if no interrupt can be found as obviously the hardware can't exist, or the DT is misconfigured badly enough so it can never be driven until it's fixed. At best this is some info or so level printk to turn on in debugging your DT or something. > just either fix the tda998x driver to not instantiate the tda9950 unless > it has an interrupt or lower the severity of the log message in the > tda9950 code. It's certanly not something that should block use in a > defconfig. I agree. > Ideally we'd not be using the wildcard compatible in the tda998x and > would know if the CEC functionality was there but I think that ship has > disappeared over the horizon at this point, and in any case someone > might just not want to support CEC for some reason. Right now the best "bang for buck" change would be to lower than complaint to an info level so s/dev_err/dev_info/ or maybe dev_notice there in tda9950.c for that specific complaint. Any preferences?
On Tue, May 10, 2022 at 09:06:52PM +0100, Liviu Dudau wrote: > The quickest way is to harmonise the treatment of IRQs between tda998x and tda9950. > The former treats the IRQ as optional, the later as mandatory. Given that (according > to comments in the code) TDA998X is actually a TDA9989+TDA9950, it makes sense to > treat the lack of IRQ the same way (make it optional, IMHO). The check in tda9950 is sensible enough, the device relies on interrupts to report the status of CEC operations so we'd need to poll constantly which doesn't seem great. It wouldn't be *so* bad for things initiated by the device but given that CEC is a network and we might get messages from other devices in the system at any time we'd need to poll constantly even when otherwise idle which doesn't seem good. I sent a patch to lower the level of the log message, hopefully that's OK for people.
On 5/11/22 14:13, Mark Brown wrote: > On Tue, May 10, 2022 at 09:06:52PM +0100, Liviu Dudau wrote: > >> The quickest way is to harmonise the treatment of IRQs between tda998x and tda9950. >> The former treats the IRQ as optional, the later as mandatory. Given that (according >> to comments in the code) TDA998X is actually a TDA9989+TDA9950, it makes sense to >> treat the lack of IRQ the same way (make it optional, IMHO). > > The check in tda9950 is sensible enough, the device relies on interrupts > to report the status of CEC operations so we'd need to poll constantly > which doesn't seem great. It wouldn't be *so* bad for things initiated > by the device but given that CEC is a network and we might get messages > from other devices in the system at any time we'd need to poll > constantly even when otherwise idle which doesn't seem good. I sent a > patch to lower the level of the log message, hopefully that's OK for > people. Ah - well either way- job done. :) No need to spin up a patch then :)
diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig index 30516dc0b70e..87edd79dac99 100644 --- a/arch/arm64/configs/defconfig +++ b/arch/arm64/configs/defconfig @@ -469,6 +469,7 @@ CONFIG_I2C_TEGRA=y CONFIG_I2C_UNIPHIER_F=y CONFIG_I2C_RCAR=y CONFIG_I2C_CROS_EC_TUNNEL=y +CONFIG_I2C_CADENCE=m CONFIG_SPI=y CONFIG_SPI_ARMADA_3700=y CONFIG_SPI_BCM2835=m @@ -678,7 +679,9 @@ CONFIG_VIDEO_OV5645=m CONFIG_VIDEO_QCOM_CAMSS=m CONFIG_DRM=m CONFIG_DRM_I2C_NXP_TDA998X=m +CONFIG_DRM_HDLCD=m CONFIG_DRM_MALI_DISPLAY=m +CONFIG_DRM_KOMEDA=m CONFIG_DRM_NOUVEAU=m CONFIG_DRM_EXYNOS=m CONFIG_DRM_EXYNOS5433_DECON=y
Key devices that support displays on SoCs like the Komeda DRM driver, the older HDLCD were not enabled by default and should be so displays can work out of the box on defconfig. Also Candence I2C support should be enabled so the PHY and thus displays can work too. Signed-off-by: Carsten Haitzler <carsten.haitzler@arm.com> --- arch/arm64/configs/defconfig | 3 +++ 1 file changed, 3 insertions(+)