| Message ID | 20220809000300.6384-1-guptarud@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] ARM: msm8960: Rename cxo_board to cxo-board and add alias | expand |
On 09/08/2022 03:02, Rudraksha Gupta wrote: > This patch renames cxo_board to be up to date with the current naming > style. It also adds an alias. Same comment as v1. Additionally you do not explain why you are doing it. > > Signed-off-by: Rudraksha Gupta <guptarud@gmail.com> > --- > v2: > - Group the correct changes together Don't link new patchsets to some other threads. > > arch/arm/boot/dts/qcom-msm8960.dtsi | 2 +- > drivers/clk/qcom/gcc-msm8960.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/boot/dts/qcom-msm8960.dtsi b/arch/arm/boot/dts/qcom-msm8960.dtsi > index 0e099aa7c889..2ed969785b78 100644 > --- a/arch/arm/boot/dts/qcom-msm8960.dtsi > +++ b/arch/arm/boot/dts/qcom-msm8960.dtsi > @@ -58,7 +58,7 @@ cpu-pmu { > }; > > clocks { > - cxo_board { > + cxo_board: cxo-board { > compatible = "fixed-clock"; > #clock-cells = <0>; > clock-frequency = <19200000>; > diff --git a/drivers/clk/qcom/gcc-msm8960.c b/drivers/clk/qcom/gcc-msm8960.c > index 051745ef99c8..56ce05a846dd 100644 > --- a/drivers/clk/qcom/gcc-msm8960.c > +++ b/drivers/clk/qcom/gcc-msm8960.c > @@ -3624,7 +3624,7 @@ static int gcc_msm8960_probe(struct platform_device *pdev) > if (!match) > return -EINVAL; > > - ret = qcom_cc_register_board_clk(dev, "cxo_board", "cxo", 19200000); > + ret = qcom_cc_register_board_clk(dev, "cxo-board", "cxo", 19200000); My comment from v1 still applies - this does not match subsystem. Additionally DTS change and driver change *cannot* go together. Basically your commit is non-bisectable which is indication it is not correct approach. Best regards, Krzysztof
On 09/08/2022 03:03, Rudraksha Gupta wrote: > This adds a very basic device tree file for the Samsung Galaxy Express > SGH-I437. Currently, the following things work: UART, eMMC, SD Card, and > USB. Use subject prefix matching the subsystem. > > Signed-off-by: Rudraksha Gupta <guptarud@gmail.com> > --- > v2: > - Group the correct changes together > > arch/arm/boot/dts/Makefile | 1 + > .../dts/qcom-msm8960-samsung-expressatt.dts | 337 ++++++++++++++++++ > 2 files changed, 338 insertions(+) > create mode 100644 arch/arm/boot/dts/qcom-msm8960-samsung-expressatt.dts > > diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile > index 05d8aef6e5d2..d55f196ad733 100644 > --- a/arch/arm/boot/dts/Makefile > +++ b/arch/arm/boot/dts/Makefile > @@ -1049,6 +1049,7 @@ dtb-$(CONFIG_ARCH_QCOM) += \ > qcom-msm8660-surf.dtb \ > qcom-msm8916-samsung-serranove.dtb \ > qcom-msm8960-cdp.dtb \ > + qcom-msm8960-samsung-expressatt.dtb \ > qcom-msm8974-lge-nexus5-hammerhead.dtb \ > qcom-msm8974-sony-xperia-rhine-amami.dtb \ > qcom-msm8974-sony-xperia-rhine-honami.dtb \ > diff --git a/arch/arm/boot/dts/qcom-msm8960-samsung-expressatt.dts b/arch/arm/boot/dts/qcom-msm8960-samsung-expressatt.dts > new file mode 100644 > index 000000000000..cf557f0c9a59 > --- /dev/null > +++ b/arch/arm/boot/dts/qcom-msm8960-samsung-expressatt.dts > @@ -0,0 +1,337 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +#include <dt-bindings/input/input.h> > +#include "qcom-msm8960.dtsi" > + > +/ { > + model = "Samsung Galaxy S3 SGH-I437"; > + compatible = "samsung,expressatt", "qcom,msm8960"; Undocumented compatible. Run checkpatch. > + > + aliases { > + serial0 = &gsbi5_serial; > + }; > + > + chosen { > + stdout-path = "serial0:115200n8"; > + > + #address-cells = <1>; > + #size-cells = <1>; > + ranges; These three properties are confusing. Why adding them? > + }; > + > + soc { > + gsbi@16400000 { Override by alias. > + status = "ok"; okay, not ok. Status goes at the end of properties. > + qcom,mode = <GSBI_PROT_I2C_UART>; > + serial@16440000 { > + status = "ok"; Same comments. > + }; > + }; > + > + amba { > + /* eMMC */ > + sdcc1: mmc@12400000 { OK, I'll abandon the review. This file is really not matching anything in the upstream. Please start your work from a proper upstreamed, recent board. Best regards, Krzysztof
> OK, I'll abandon the review. This file is really not matching anything > in the upstream. Please start your work from a proper upstreamed, recent > board. I based it off of qcom-msm8960-cdp.dts. If there is a dts that you would like me to model off of, please link it to me. Otherwise, I will find another recent dts and model off of that
On Mon 08 Aug 17:02 PDT 2022, Rudraksha Gupta wrote: > This patch renames cxo_board to be up to date with the current naming > style. It also adds an alias. > > Signed-off-by: Rudraksha Gupta <guptarud@gmail.com> > --- > v2: > - Group the correct changes together > > arch/arm/boot/dts/qcom-msm8960.dtsi | 2 +- > drivers/clk/qcom/gcc-msm8960.c | 2 +- Clock and dts patches goes through two different paths towards mainline, so they should be separated. > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/boot/dts/qcom-msm8960.dtsi b/arch/arm/boot/dts/qcom-msm8960.dtsi > index 0e099aa7c889..2ed969785b78 100644 > --- a/arch/arm/boot/dts/qcom-msm8960.dtsi > +++ b/arch/arm/boot/dts/qcom-msm8960.dtsi > @@ -58,7 +58,7 @@ cpu-pmu { > }; > > clocks { > - cxo_board { > + cxo_board: cxo-board { > compatible = "fixed-clock"; > #clock-cells = <0>; > clock-frequency = <19200000>; > diff --git a/drivers/clk/qcom/gcc-msm8960.c b/drivers/clk/qcom/gcc-msm8960.c > index 051745ef99c8..56ce05a846dd 100644 > --- a/drivers/clk/qcom/gcc-msm8960.c > +++ b/drivers/clk/qcom/gcc-msm8960.c > @@ -3624,7 +3624,7 @@ static int gcc_msm8960_probe(struct platform_device *pdev) > if (!match) > return -EINVAL; > > - ret = qcom_cc_register_board_clk(dev, "cxo_board", "cxo", 19200000); > + ret = qcom_cc_register_board_clk(dev, "cxo-board", "cxo", 19200000); This breaks compatibility with existing DTB files. What you probably want is to make sure that any clocks with parent name of "cxo", should have a .fw_name = "cxo", then you can make a phandle-based reference in DT and these global names doesn't matter (and in the end we can remove this board_clk from the driver). Regards, Bjorn > if (ret) > return ret; > > -- > 2.25.1 >
> Clock and dts patches goes through two different paths towards mainline, > so they should be separated. Gotcha, thanks. I will do that. > This breaks compatibility with existing DTB files. > What you probably want is to make sure that any clocks with parent name > of "cxo", should have a .fw_name = "cxo", then you can make a > phandle-based reference in DT and these global names doesn't matter (and > in the end we can remove this board_clk from the driver). Ah, I see. If I understand correctly, it should be something like this, right? https://github.com/torvalds/linux/blob/master/drivers/clk/qcom/gcc-msm8996.c#L169-L172
On Wed, Aug 10, 2022 at 10:51:51PM -0400, Rudraksha Gupta wrote: > > Clock and dts patches goes through two different paths towards mainline, > > > so they should be separated. > > Gotcha, thanks. I will do that. > > > > This breaks compatibility with existing DTB files. > > > What you probably want is to make sure that any clocks with parent name > > > of "cxo", should have a .fw_name = "cxo", then you can make a > > > phandle-based reference in DT and these global names doesn't matter (and > > > in the end we can remove this board_clk from the driver). > > Ah, I see. If I understand correctly, it should be something like this, > right? > https://github.com/torvalds/linux/blob/master/drivers/clk/qcom/gcc-msm8996.c#L169-L172 Correct, this will try to find the clock by clock-names of "cxo" and if not found fall back to do a lookup globally just on the name "xo_board". So it seems making .name "cxo_board" should handle both cases nicely. Regards, Bjorn
Hello, So I'm trying to add a cxo-board node to my dts, however the current implementation seems like it wants cxo_board. It was recommended a while ago that I refactor gcc-msm8960.c to be more like https://github.com/torvalds/linux/blob/master/drivers/clk/qcom/gcc-msm8996.c#L36 . However, I have a couple of questions: - The xo struct that I listed above is listed in another struct https://github.com/torvalds/linux/blob/master/drivers/clk/qcom/gcc-msm8996.c#L3408 which is listed in the SoC desc struct https://github.com/torvalds/linux/blob/master/drivers/clk/qcom/gcc-msm8996.c#L3818 . My question is that even though gcc-msm8960.c has an msm8960/apq8064 desc struct, it doesn't have anything like gcc_msm8996_hws. How would I know what goes in a hypothetical gcc_(msm8960/apq8064)_hws struct? I'm assuming that all I need in the hw struct is the pxo and cxo listed here https://github.com/torvalds/linux/blob/master/drivers/clk/qcom/gcc-msm8960.c#L3727 however I'm not 100% sure how to verify this. Would anything else go into a hypothetical gcc_(msm8960/apq8064)_hws struct? - Is there documentation on how the gcc-<soc> files work? I'm still quite new to contributing to the Linux kernel and would like to learn more about the modern way to format these files and to learn more about how they work in general Thanks, Rudraksha
diff --git a/arch/arm/boot/dts/qcom-msm8960.dtsi b/arch/arm/boot/dts/qcom-msm8960.dtsi index 0e099aa7c889..2ed969785b78 100644 --- a/arch/arm/boot/dts/qcom-msm8960.dtsi +++ b/arch/arm/boot/dts/qcom-msm8960.dtsi @@ -58,7 +58,7 @@ cpu-pmu { }; clocks { - cxo_board { + cxo_board: cxo-board { compatible = "fixed-clock"; #clock-cells = <0>; clock-frequency = <19200000>; diff --git a/drivers/clk/qcom/gcc-msm8960.c b/drivers/clk/qcom/gcc-msm8960.c index 051745ef99c8..56ce05a846dd 100644 --- a/drivers/clk/qcom/gcc-msm8960.c +++ b/drivers/clk/qcom/gcc-msm8960.c @@ -3624,7 +3624,7 @@ static int gcc_msm8960_probe(struct platform_device *pdev) if (!match) return -EINVAL; - ret = qcom_cc_register_board_clk(dev, "cxo_board", "cxo", 19200000); + ret = qcom_cc_register_board_clk(dev, "cxo-board", "cxo", 19200000); if (ret) return ret;
This patch renames cxo_board to be up to date with the current naming style. It also adds an alias. Signed-off-by: Rudraksha Gupta <guptarud@gmail.com> --- v2: - Group the correct changes together arch/arm/boot/dts/qcom-msm8960.dtsi | 2 +- drivers/clk/qcom/gcc-msm8960.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)