Re: [PATCH 2/4] clk: qcom: Add global clock controller driver for MSM8917

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Feb 22, 2023 at 09:51:21AM +0100, Krzysztof Kozlowski wrote:
> On 21/02/2023 18:49, Otto Pflüger wrote:
> > This driver provides clocks, resets and power domains needed for various
> > components of the MSM8917 SoC and the very similar QM215 SoC.
> > 
> > According to [1] in the downstream kernel, the GPU clock has a different
> > source mapping on QM215 (gcc_gfx3d_map vs gcc_gfx3d_map_qm215).
> > 
> > [1]: https://git.codelinaro.org/clo/la/kernel/msm-4.9/-/blob/LF.UM.8.6.2-28000-89xx.0/include/dt-bindings/clock/msm-clocks-hwio-8952.h#L298
> > 
> > Signed-off-by: Otto Pflüger <otto.pflueger@xxxxxxxxx>
> > ---
> >  drivers/clk/qcom/Kconfig       |    8 +
> >  drivers/clk/qcom/Makefile      |    1 +
> >  drivers/clk/qcom/gcc-msm8917.c | 3283 ++++++++++++++++++++++++++++++++
> >  3 files changed, 3292 insertions(+)
> >  create mode 100644 drivers/clk/qcom/gcc-msm8917.c
> > 
> > diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> > index 70d43f0a8919..3ef58b09385a 100644
> > --- a/drivers/clk/qcom/Kconfig
> > +++ b/drivers/clk/qcom/Kconfig
> > @@ -196,6 +196,14 @@ config MSM_GCC_8916
> >  	  Say Y if you want to use devices such as UART, SPI i2c, USB,
> >  	  SD/eMMC, display, graphics, camera etc.
> >  
> > +config MSM_GCC_8917
> > +	tristate "MSM8917 Global Clock Controller"
> 
> I propose to add here:
> 
> depends on ARM64 || COMPILE_TEST

Why should this driver only be enabled on ARM64 systems? In my opinion,
this should also be supported on 32-bit ARM, just like the other clock
drivers for the 64-bit SoCs. Users should be able to boot a 32-bit
kernel if they have to, e.g. if their device has broken firmware that
prohibits booting 64-bit kernels (there have been such cases with
MSM8916) or if they think that 64-bit systems use too much memory (my
QM215 device with 512MB RAM shipped with a 32-bit kernel for this very
reason).

> 
> > +	select QCOM_GDSC
> > +	help
> > +	  Support for the global clock controller on msm8917 devices.
> > +	  Say Y if you want to use devices such as UART, SPI i2c, USB,
> > +	  SD/eMMC, display, graphics, camera etc.
> > +
> 
> (...)
> > +
> > +static int gcc_msm8917_probe(struct platform_device *pdev)
> > +{
> > +	struct regmap *regmap;
> > +
> > +	regmap  = qcom_cc_map(pdev, &gcc_msm8917_desc);
> > +	if (IS_ERR(regmap))
> > +		return PTR_ERR(regmap);
> > +
> > +	if (of_device_is_compatible(pdev->dev.of_node, "qcom,gcc-qm215"))
> 
> Use data in of_device_id instead. This scales poorly if any new variant
> is added here. Unless no new variants will be added? Ever?

Thanks. There are a lot of other related SoCs (MSM8920, MSM8937, MSM8940,
MSM8952, SDM429 and SDM439) which could get added here as variants, so
I'll implement this using data in of_device_id.

Regards,
Otto Pflüger



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux