| Message ID | 20230111205125.1860858-1-konrad.dybcio@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] dt-bindings: cpufreq: Make reg-names a required property | expand |
On Thu, Jan 12, 2023 at 04:41:50PM +0100, Konrad Dybcio wrote: > > > On 12.01.2023 16:37, Bjorn Andersson wrote: > > On Wed, Jan 11, 2023 at 09:51:25PM +0100, Konrad Dybcio wrote: > >> In preparation for CPRh-aware OSM programming, change the probe > >> function so that we determine the number of frequency domains by > >> counting the number of reg-names entries that begin with > >> "freq-domain", as the aforementioned changes require introduction > >> of non-freq-domain register spaces. > >> > > > > Requiring reg-names would break backwards compatibility with at least > > sc7280 and sm6115. > Ouch, you're correct.. > > Does checking for reg-names and applying the code flow proposed in this > patch if found and the existing one if not sound good? Why support 2 ways? > Konrad > > > > Regards, > > Bjorn > > > >> Fixes: 1a6a8b0080b0 ("cpufreq: qcom-hw: Fix reading "reg" with address/size-cells != 2") > >> Fixes: 054a3ef683a1 ("cpufreq: qcom-hw: Allocate qcom_cpufreq_data during probe") > >> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > >> --- > >> drivers/cpufreq/qcom-cpufreq-hw.c | 34 ++++++++++++++++++++++--------- > >> 1 file changed, 24 insertions(+), 10 deletions(-) > >> > >> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c > >> index 9505a812d6a1..89d5ed267399 100644 > >> --- a/drivers/cpufreq/qcom-cpufreq-hw.c > >> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c > >> @@ -651,8 +651,9 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev) > >> struct device *dev = &pdev->dev; > >> struct device_node *soc_node; > >> struct device *cpu_dev; > >> + const char *reg_name; > >> struct clk *clk; > >> - int ret, i, num_domains, reg_sz; > >> + int ret, i, num_reg_names, num_domains = 0; > >> > >> clk = clk_get(dev, "xo"); > >> if (IS_ERR(clk)) > >> @@ -684,19 +685,32 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev) > >> if (!soc_node) > >> return -EINVAL; > >> > >> - ret = of_property_read_u32(soc_node, "#address-cells", ®_sz); > >> - if (ret) > >> + num_reg_names = of_property_count_strings(dev->of_node, "reg-names"); > >> + if (num_reg_names <= 0) { > >> + ret = num_reg_names ? num_reg_names : -ENODATA; > >> goto of_exit; > >> + } > >> > >> - ret = of_property_read_u32(soc_node, "#size-cells", &i); > >> - if (ret) > >> - goto of_exit; > >> + for (i = 0; i < num_reg_names; i++) { > >> + ret = of_property_read_string_index(dev->of_node, "reg-names", i, ®_name); > >> + if (ret < 0) > >> + goto of_exit; > >> > >> - reg_sz += i; > >> + /* > >> + * Check if the i-th reg is a freq-domain base, no need to add 1 > >> + * more byte for idx, as sizeof counts \0 whereas strlen does not. > >> + */ > >> + if (strlen(reg_name) == sizeof("freq-domain")) { > >> + /* Check if this reg-name begins with "freq-domain" */ > >> + if (!strncmp(reg_name, "freq-domain", sizeof("freq-domain") - 1)) > >> + num_domains++; > >> + } > >> + } > >> > >> - num_domains = of_property_count_elems_of_size(dev->of_node, "reg", sizeof(u32) * reg_sz); This code was not great to begin with. Any code parsing 'reg' on it's own is suspect IMO. It's a standard property and all parsing of it should be in drivers/of/address.c. (Yes, I know there are other cases.) The reg entries are already available as platform_device resources? Why don't you use that? There's also of_address_count(), but I prefer if there's a platform device equivalent like we have for interrupts. Rob
On 13.01.2023 20:41, Rob Herring wrote: > On Thu, Jan 12, 2023 at 04:41:50PM +0100, Konrad Dybcio wrote: >> >> >> On 12.01.2023 16:37, Bjorn Andersson wrote: >>> On Wed, Jan 11, 2023 at 09:51:25PM +0100, Konrad Dybcio wrote: >>>> In preparation for CPRh-aware OSM programming, change the probe >>>> function so that we determine the number of frequency domains by >>>> counting the number of reg-names entries that begin with >>>> "freq-domain", as the aforementioned changes require introduction >>>> of non-freq-domain register spaces. >>>> >>> >>> Requiring reg-names would break backwards compatibility with at least >>> sc7280 and sm6115. >> Ouch, you're correct.. >> >> Does checking for reg-names and applying the code flow proposed in this >> patch if found and the existing one if not sound good? > > Why support 2 ways? Targets that are supported by the current revision of this driver (which only specify frequency-domain-N MMIO spaces as reg entries) assume that ARRAY_SIZE(reg) == the number of frequency domains. These usually range from 1 to 3. We can either hardcode the number of frequency domains on targets that require more, different register spaces (for manual hardware programming, which also happens on currently supported hardware, just that the secure firmware does it for us.. see [1]) or check with reg-names. Requiring reg-names would break backwards compatibility with older DTs for at least two SoCs. > > >> Konrad >>> >>> Regards, >>> Bjorn >>> >>>> Fixes: 1a6a8b0080b0 ("cpufreq: qcom-hw: Fix reading "reg" with address/size-cells != 2") >>>> Fixes: 054a3ef683a1 ("cpufreq: qcom-hw: Allocate qcom_cpufreq_data during probe") >>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >>>> --- >>>> drivers/cpufreq/qcom-cpufreq-hw.c | 34 ++++++++++++++++++++++--------- >>>> 1 file changed, 24 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c >>>> index 9505a812d6a1..89d5ed267399 100644 >>>> --- a/drivers/cpufreq/qcom-cpufreq-hw.c >>>> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c >>>> @@ -651,8 +651,9 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev) >>>> struct device *dev = &pdev->dev; >>>> struct device_node *soc_node; >>>> struct device *cpu_dev; >>>> + const char *reg_name; >>>> struct clk *clk; >>>> - int ret, i, num_domains, reg_sz; >>>> + int ret, i, num_reg_names, num_domains = 0; >>>> >>>> clk = clk_get(dev, "xo"); >>>> if (IS_ERR(clk)) >>>> @@ -684,19 +685,32 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev) >>>> if (!soc_node) >>>> return -EINVAL; >>>> >>>> - ret = of_property_read_u32(soc_node, "#address-cells", ®_sz); >>>> - if (ret) >>>> + num_reg_names = of_property_count_strings(dev->of_node, "reg-names"); >>>> + if (num_reg_names <= 0) { >>>> + ret = num_reg_names ? num_reg_names : -ENODATA; >>>> goto of_exit; >>>> + } >>>> >>>> - ret = of_property_read_u32(soc_node, "#size-cells", &i); >>>> - if (ret) >>>> - goto of_exit; >>>> + for (i = 0; i < num_reg_names; i++) { >>>> + ret = of_property_read_string_index(dev->of_node, "reg-names", i, ®_name); >>>> + if (ret < 0) >>>> + goto of_exit; >>>> >>>> - reg_sz += i; >>>> + /* >>>> + * Check if the i-th reg is a freq-domain base, no need to add 1 >>>> + * more byte for idx, as sizeof counts \0 whereas strlen does not. >>>> + */ >>>> + if (strlen(reg_name) == sizeof("freq-domain")) { >>>> + /* Check if this reg-name begins with "freq-domain" */ >>>> + if (!strncmp(reg_name, "freq-domain", sizeof("freq-domain") - 1)) >>>> + num_domains++; >>>> + } >>>> + } >>>> >>>> - num_domains = of_property_count_elems_of_size(dev->of_node, "reg", sizeof(u32) * reg_sz); > > This code was not great to begin with. Any code parsing 'reg' on it's > own is suspect IMO. It's a standard property and all parsing of it > should be in drivers/of/address.c. (Yes, I know there are other cases.) > > The reg entries are already available as platform_device resources? Why > don't you use that? There's also of_address_count(), but I prefer if > there's a platform device equivalent like we have for interrupts. Hm.. I knew this was suspiciously bare-dt-operation, but never quite connected the dots.. perhaps that's a good idea to pursue.. Konrad [1] https://patchwork.kernel.org/project/linux-pm/patch/20210701105730.322718-7-angelogioacchino.delregno@somainline.org/ > > Rob
On Wed, Jan 11, 2023 at 09:51:25PM +0100, Konrad Dybcio wrote: > In preparation for CPRh-aware OSM programming, change the probe > function so that we determine the number of frequency domains by > counting the number of reg-names entries that begin with > "freq-domain", as the aforementioned changes require introduction > of non-freq-domain register spaces. > > Fixes: 1a6a8b0080b0 ("cpufreq: qcom-hw: Fix reading "reg" with address/size-cells != 2") > Fixes: 054a3ef683a1 ("cpufreq: qcom-hw: Allocate qcom_cpufreq_data during probe") Why do you need these fixes tags? Thanks, Mani > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > drivers/cpufreq/qcom-cpufreq-hw.c | 34 ++++++++++++++++++++++--------- > 1 file changed, 24 insertions(+), 10 deletions(-) > > diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c > index 9505a812d6a1..89d5ed267399 100644 > --- a/drivers/cpufreq/qcom-cpufreq-hw.c > +++ b/drivers/cpufreq/qcom-cpufreq-hw.c > @@ -651,8 +651,9 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev) > struct device *dev = &pdev->dev; > struct device_node *soc_node; > struct device *cpu_dev; > + const char *reg_name; > struct clk *clk; > - int ret, i, num_domains, reg_sz; > + int ret, i, num_reg_names, num_domains = 0; > > clk = clk_get(dev, "xo"); > if (IS_ERR(clk)) > @@ -684,19 +685,32 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev) > if (!soc_node) > return -EINVAL; > > - ret = of_property_read_u32(soc_node, "#address-cells", ®_sz); > - if (ret) > + num_reg_names = of_property_count_strings(dev->of_node, "reg-names"); > + if (num_reg_names <= 0) { > + ret = num_reg_names ? num_reg_names : -ENODATA; > goto of_exit; > + } > > - ret = of_property_read_u32(soc_node, "#size-cells", &i); > - if (ret) > - goto of_exit; > + for (i = 0; i < num_reg_names; i++) { > + ret = of_property_read_string_index(dev->of_node, "reg-names", i, ®_name); > + if (ret < 0) > + goto of_exit; > > - reg_sz += i; > + /* > + * Check if the i-th reg is a freq-domain base, no need to add 1 > + * more byte for idx, as sizeof counts \0 whereas strlen does not. > + */ > + if (strlen(reg_name) == sizeof("freq-domain")) { > + /* Check if this reg-name begins with "freq-domain" */ > + if (!strncmp(reg_name, "freq-domain", sizeof("freq-domain") - 1)) > + num_domains++; > + } > + } > > - num_domains = of_property_count_elems_of_size(dev->of_node, "reg", sizeof(u32) * reg_sz); > - if (num_domains <= 0) > - return num_domains; > + if (num_domains <= 0) { > + ret = -EINVAL; > + goto of_exit; > + } > > qcom_cpufreq.data = devm_kzalloc(dev, sizeof(struct qcom_cpufreq_data) * num_domains, > GFP_KERNEL); > -- > 2.39.0 >
On Wed, Jan 11, 2023 at 09:51:24PM +0100, Konrad Dybcio wrote: > In preparation for determining the number of frequency domains based > on reg-names entries, rather than performing calculations based on > reg entries and parent #{address,size}-cells, make reg-names a > required property. > As Bjorn mentioned in the driver patch, this would break backwards compatibility. Thanks, Mani > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.yaml | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.yaml b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.yaml > index 99e159bc5fb1..460a7ab7b87c 100644 > --- a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.yaml > +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.yaml > @@ -74,6 +74,7 @@ properties: > required: > - compatible > - reg > + - reg-names > - clocks > - clock-names > - '#freq-domain-cells' > -- > 2.39.0 >
diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.yaml b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.yaml index 99e159bc5fb1..460a7ab7b87c 100644 --- a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.yaml +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.yaml @@ -74,6 +74,7 @@ properties: required: - compatible - reg + - reg-names - clocks - clock-names - '#freq-domain-cells'
In preparation for determining the number of frequency domains based on reg-names entries, rather than performing calculations based on reg entries and parent #{address,size}-cells, make reg-names a required property. Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> --- Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.yaml | 1 + 1 file changed, 1 insertion(+)