Re: [PATCH v5 6/6] qcom/soc/drivers: Add DTPM description for sdm845

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Fri 07 Jan 14:07 PST 2022, Daniel Lezcano wrote:

> 
> Hi Bjorn,
> 
> On 07/01/2022 20:27, Bjorn Andersson wrote:
> 
> [ ... ]
> 
> >> +#include <linux/dtpm.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of.h>
> >> +#include <linux/platform_device.h>
> >> +
> >> +static struct dtpm_node __initdata sdm845_hierarchy[] = {
> >> +	[0]{ .name = "sdm845" },
> > 
> > Why is the index signifiant here?
> > Doesn't this imply risk that we forget one element, which will be
> > thereby implicitly be left initialized as {} and hence denote
> > termination of the list?
> 
> Yes, that is possible. The other annotation is also possible. The index
> helps to refer from the .parent field.
> 
> That said nothing forces to use the index, so it is a matter of taste.
> 
> >> +	[1]{ .name = "package",
> >> +	     .parent = &sdm845_hierarchy[0] },
> >> +	[2]{ .name = "/cpus/cpu@0",
> >> +	     .type = DTPM_NODE_DT,
> >> +	     .parent = &sdm845_hierarchy[1] },
> >> +	[3]{ .name = "/cpus/cpu@100",
> >> +	     .type = DTPM_NODE_DT,
> >> +	     .parent = &sdm845_hierarchy[1] },
> >> +	[4]{ .name = "/cpus/cpu@200",
> >> +	     .type = DTPM_NODE_DT,
> >> +	     .parent = &sdm845_hierarchy[1] },
> >> +	[5]{ .name = "/cpus/cpu@300",
> >> +	     .type = DTPM_NODE_DT,
> >> +	     .parent = &sdm845_hierarchy[1] },
> >> +	[6]{ .name = "/cpus/cpu@400",
> >> +	     .type = DTPM_NODE_DT,
> >> +	     .parent = &sdm845_hierarchy[1] },
> >> +	[7]{ .name = "/cpus/cpu@500",
> >> +	     .type = DTPM_NODE_DT,
> >> +	     .parent = &sdm845_hierarchy[1] },
> >> +	[8]{ .name = "/cpus/cpu@600",
> >> +	     .type = DTPM_NODE_DT,
> >> +	     .parent = &sdm845_hierarchy[1] },
> >> +	[9]{ .name = "/cpus/cpu@700",
> >> +	     .type = DTPM_NODE_DT,
> >> +	     .parent = &sdm845_hierarchy[1] },
> >> +	[10]{ .name = "/soc@0/gpu@5000000",
> > 
> > It worries me that we encode the textual structure of the dts in the
> > kernel. E.g. for quite a while this was "/soc/gpu@5000000", so if this
> > landed a year ago this driver would have prevented us from correcting
> > the dts.
> 
> Why ? The change should be reflected in the driver also, no ?
> 

There was no update needed to change /soc to /soc@0, but with this
driver in place we would need to do that.

The problem is that the life cycle of the DTB is different from Linux
and we promise our users that the kernel will be backwards compatible
with existing DTBs (at least for a reasonable amount of time).

So if we made a change in the kernel to turn the incorrect
"/soc/gpu@5000000" into "/soc@0/gpu@5000000" we would no longer find a
match if you try to boot with yesterday's DTB.

> > Another concern is that not all busses in the system are capable of
> > 36-bit wide addresses, so it's plausible that we might one day have to
> > create a more accurate representation of the address space. Maybe not on
> > SDM845, but this would force us to be inconsistent.
> 
> Sorry, I'm missing the point :/
> 
> If a change is done in the DT, the code using the description must be
> changed accordingly, no?
> 

No, the kernel should continue to function with the old DTB.

Consider when your Linux distro gives you a new kernel version on your
computer, that shouldn't require an upgrade of "BIOS" in order to boot
the new kernel.

Regards,
Bjorn

> 
> > Regards,
> > Bjorn
> > 
> >> +	     .type = DTPM_NODE_DT,
> >> +	     .parent = &sdm845_hierarchy[1] },
> >> +	[11]{ },
> >> +};
> >> +
> >> +static struct of_device_id __initdata sdm845_dtpm_match_table[] = {
> >> +        { .compatible = "qcom,sdm845", .data = sdm845_hierarchy },
> >> +        {},
> >> +};
> >> +
> >> +static int __init sdm845_dtpm_init(void)
> >> +{
> >> +	return dtpm_create_hierarchy(sdm845_dtpm_match_table);
> >> +}
> >> +late_initcall(sdm845_dtpm_init);
> >> +
> >> +MODULE_DESCRIPTION("Qualcomm DTPM driver");
> >> +MODULE_LICENSE("GPL");
> >> +MODULE_ALIAS("platform:dtpm");
> >> +MODULE_AUTHOR("Daniel Lezcano <daniel.lezcano@kernel.org");
> >> +
> >> -- 
> >> 2.25.1
> >>
> 
> 
> -- 
> <http://www.linaro.org/> Linaro.org ??? Open source software for ARM SoCs
> 
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog