| Message ID | 20231128005849.19044-5-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for H616 Thermal system | expand |
On 28/11/2023 01:58, Andre Przywara wrote: > > +static struct regmap *sun8i_ths_get_syscon_regmap(struct device_node *node) > +{ > + struct device_node *syscon_node; > + struct platform_device *syscon_pdev; > + struct regmap *regmap = NULL; > + > + syscon_node = of_parse_phandle(node, "syscon", 0); Nope. For the 100th time, this cannot be generic. > + if (!syscon_node) > + return ERR_PTR(-ENODEV); > + > + syscon_pdev = of_find_device_by_node(syscon_node); > + if (!syscon_pdev) { > + /* platform device might not be probed yet */ > + regmap = ERR_PTR(-EPROBE_DEFER); > + goto out_put_node; > + } > + > + /* If no regmap is found then the other device driver is at fault */ > + regmap = dev_get_regmap(&syscon_pdev->dev, NULL); > + if (!regmap) > + regmap = ERR_PTR(-EINVAL); Aren't you open-coding existing API to get regmap from syscon? Best regards, Krzysztof
On Tue, Nov 28, 2023 at 3:43 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 28/11/2023 01:58, Andre Przywara wrote: > > > > +static struct regmap *sun8i_ths_get_syscon_regmap(struct device_node *node) > > +{ > > + struct device_node *syscon_node; > > + struct platform_device *syscon_pdev; > > + struct regmap *regmap = NULL; > > + > > + syscon_node = of_parse_phandle(node, "syscon", 0); > > Nope. For the 100th time, this cannot be generic. > > > + if (!syscon_node) > > + return ERR_PTR(-ENODEV); > > + > > + syscon_pdev = of_find_device_by_node(syscon_node); > > + if (!syscon_pdev) { > > + /* platform device might not be probed yet */ > > + regmap = ERR_PTR(-EPROBE_DEFER); > > + goto out_put_node; > > + } > > + > > + /* If no regmap is found then the other device driver is at fault */ > > + regmap = dev_get_regmap(&syscon_pdev->dev, NULL); > > + if (!regmap) > > + regmap = ERR_PTR(-EINVAL); > > Aren't you open-coding existing API to get regmap from syscon? Not really. This is to get a regmap exported by the device. Syscon's regmap is not tied to the device at all. With this scheme a device to could export just enough registers for the consumer to use, instead of the whole address range. We do this in the R40 clock controller as well, which has some bits that tweak the ethernet controllers RGMII delay... ChenYu
On 28/11/2023 08:50, Chen-Yu Tsai wrote: > On Tue, Nov 28, 2023 at 3:43 PM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 28/11/2023 01:58, Andre Przywara wrote: >>> >>> +static struct regmap *sun8i_ths_get_syscon_regmap(struct device_node *node) >>> +{ >>> + struct device_node *syscon_node; >>> + struct platform_device *syscon_pdev; >>> + struct regmap *regmap = NULL; >>> + >>> + syscon_node = of_parse_phandle(node, "syscon", 0); >> >> Nope. For the 100th time, this cannot be generic. >> >>> + if (!syscon_node) >>> + return ERR_PTR(-ENODEV); >>> + >>> + syscon_pdev = of_find_device_by_node(syscon_node); >>> + if (!syscon_pdev) { >>> + /* platform device might not be probed yet */ >>> + regmap = ERR_PTR(-EPROBE_DEFER); >>> + goto out_put_node; >>> + } >>> + >>> + /* If no regmap is found then the other device driver is at fault */ >>> + regmap = dev_get_regmap(&syscon_pdev->dev, NULL); >>> + if (!regmap) >>> + regmap = ERR_PTR(-EINVAL); >> >> Aren't you open-coding existing API to get regmap from syscon? > > Not really. This is to get a regmap exported by the device. Syscon's regmap > is not tied to the device at all. I am talking about open-coding existing API. Look at syscon.h. > > With this scheme a device to could export just enough registers for the > consumer to use, instead of the whole address range. > > We do this in the R40 clock controller as well, which has some bits that > tweak the ethernet controllers RGMII delay... Not related. > > > ChenYu Best regards, Krzysztof
On Tue, Nov 28, 2023 at 4:30 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 28/11/2023 08:50, Chen-Yu Tsai wrote: > > On Tue, Nov 28, 2023 at 3:43 PM Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > >> > >> On 28/11/2023 01:58, Andre Przywara wrote: > >>> > >>> +static struct regmap *sun8i_ths_get_syscon_regmap(struct device_node *node) > >>> +{ > >>> + struct device_node *syscon_node; > >>> + struct platform_device *syscon_pdev; > >>> + struct regmap *regmap = NULL; > >>> + > >>> + syscon_node = of_parse_phandle(node, "syscon", 0); > >> > >> Nope. For the 100th time, this cannot be generic. > >> > >>> + if (!syscon_node) > >>> + return ERR_PTR(-ENODEV); > >>> + > >>> + syscon_pdev = of_find_device_by_node(syscon_node); > >>> + if (!syscon_pdev) { > >>> + /* platform device might not be probed yet */ > >>> + regmap = ERR_PTR(-EPROBE_DEFER); > >>> + goto out_put_node; > >>> + } > >>> + > >>> + /* If no regmap is found then the other device driver is at fault */ > >>> + regmap = dev_get_regmap(&syscon_pdev->dev, NULL); > >>> + if (!regmap) > >>> + regmap = ERR_PTR(-EINVAL); > >> > >> Aren't you open-coding existing API to get regmap from syscon? > > > > Not really. This is to get a regmap exported by the device. Syscon's regmap > > is not tied to the device at all. > > I am talking about open-coding existing API. Look at syscon.h. The underlying implementation is different. syscon maintains its own mapping of device nodes to regmaps, and creates regmaps when none exist. The regmap is not tied to any struct device. syscon basically exists outside of the driver model and one has no control over what is exposed because it is meant for blocks that are a collection of random stuff. Here the provider device registers the (limited) regmap it wants to provide, tying the new regmap to itself. The consumer gets it via the dev_get_regmap() call. The provider has a main function and isn't exposing that part of its register map to the outside; only the random bits that were stuffed in are. > > With this scheme a device to could export just enough registers for the > > consumer to use, instead of the whole address range. > > > > We do this in the R40 clock controller as well, which has some bits that > > tweak the ethernet controllers RGMII delay... > > Not related. Related as in that is possibly what this code was based on, commit 49a06cae6e7c ("net: stmmac: dwmac-sun8i: Allow getting syscon regmap from external device"). ChenYu
On Tue, Nov 28, 2023 at 4:59 PM Chen-Yu Tsai <wens@csie.org> wrote: > > On Tue, Nov 28, 2023 at 4:30 PM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: > > > > On 28/11/2023 08:50, Chen-Yu Tsai wrote: > > > On Tue, Nov 28, 2023 at 3:43 PM Krzysztof Kozlowski > > > <krzysztof.kozlowski@linaro.org> wrote: > > >> > > >> On 28/11/2023 01:58, Andre Przywara wrote: > > >>> > > >>> +static struct regmap *sun8i_ths_get_syscon_regmap(struct device_node *node) > > >>> +{ > > >>> + struct device_node *syscon_node; > > >>> + struct platform_device *syscon_pdev; > > >>> + struct regmap *regmap = NULL; > > >>> + > > >>> + syscon_node = of_parse_phandle(node, "syscon", 0); > > >> > > >> Nope. For the 100th time, this cannot be generic. > > >> > > >>> + if (!syscon_node) > > >>> + return ERR_PTR(-ENODEV); > > >>> + > > >>> + syscon_pdev = of_find_device_by_node(syscon_node); > > >>> + if (!syscon_pdev) { > > >>> + /* platform device might not be probed yet */ > > >>> + regmap = ERR_PTR(-EPROBE_DEFER); > > >>> + goto out_put_node; > > >>> + } > > >>> + > > >>> + /* If no regmap is found then the other device driver is at fault */ > > >>> + regmap = dev_get_regmap(&syscon_pdev->dev, NULL); > > >>> + if (!regmap) > > >>> + regmap = ERR_PTR(-EINVAL); > > >> > > >> Aren't you open-coding existing API to get regmap from syscon? > > > > > > Not really. This is to get a regmap exported by the device. Syscon's regmap > > > is not tied to the device at all. > > > > I am talking about open-coding existing API. Look at syscon.h. > > The underlying implementation is different. > > syscon maintains its own mapping of device nodes to regmaps, and creates > regmaps when none exist. The regmap is not tied to any struct device. > syscon basically exists outside of the driver model and one has no control > over what is exposed because it is meant for blocks that are a collection > of random stuff. My bad. I failed to realize there is a platform device driver for syscon, in addition to the existing "no struct device" implementation. ChenYu > Here the provider device registers the (limited) regmap it wants to provide, > tying the new regmap to itself. The consumer gets it via the dev_get_regmap() > call. The provider has a main function and isn't exposing that part of its > register map to the outside; only the random bits that were stuffed in are. > > > > With this scheme a device to could export just enough registers for the > > > consumer to use, instead of the whole address range. > > > > > > We do this in the R40 clock controller as well, which has some bits that > > > tweak the ethernet controllers RGMII delay... > > > > Not related. > > Related as in that is possibly what this code was based on, commit > 49a06cae6e7c ("net: stmmac: dwmac-sun8i: Allow getting syscon regmap > from external device"). > > > ChenYu
On Tue, Nov 28, 2023 at 5:02 PM Chen-Yu Tsai <wens@csie.org> wrote: > > On Tue, Nov 28, 2023 at 4:59 PM Chen-Yu Tsai <wens@csie.org> wrote: > > > > On Tue, Nov 28, 2023 at 4:30 PM Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > > > > > > On 28/11/2023 08:50, Chen-Yu Tsai wrote: > > > > On Tue, Nov 28, 2023 at 3:43 PM Krzysztof Kozlowski > > > > <krzysztof.kozlowski@linaro.org> wrote: > > > >> > > > >> On 28/11/2023 01:58, Andre Przywara wrote: > > > >>> > > > >>> +static struct regmap *sun8i_ths_get_syscon_regmap(struct device_node *node) > > > >>> +{ > > > >>> + struct device_node *syscon_node; > > > >>> + struct platform_device *syscon_pdev; > > > >>> + struct regmap *regmap = NULL; > > > >>> + > > > >>> + syscon_node = of_parse_phandle(node, "syscon", 0); > > > >> > > > >> Nope. For the 100th time, this cannot be generic. > > > >> > > > >>> + if (!syscon_node) > > > >>> + return ERR_PTR(-ENODEV); > > > >>> + > > > >>> + syscon_pdev = of_find_device_by_node(syscon_node); > > > >>> + if (!syscon_pdev) { > > > >>> + /* platform device might not be probed yet */ > > > >>> + regmap = ERR_PTR(-EPROBE_DEFER); > > > >>> + goto out_put_node; > > > >>> + } > > > >>> + > > > >>> + /* If no regmap is found then the other device driver is at fault */ > > > >>> + regmap = dev_get_regmap(&syscon_pdev->dev, NULL); > > > >>> + if (!regmap) > > > >>> + regmap = ERR_PTR(-EINVAL); > > > >> > > > >> Aren't you open-coding existing API to get regmap from syscon? > > > > > > > > Not really. This is to get a regmap exported by the device. Syscon's regmap > > > > is not tied to the device at all. > > > > > > I am talking about open-coding existing API. Look at syscon.h. > > > > The underlying implementation is different. > > > > syscon maintains its own mapping of device nodes to regmaps, and creates > > regmaps when none exist. The regmap is not tied to any struct device. > > syscon basically exists outside of the driver model and one has no control > > over what is exposed because it is meant for blocks that are a collection > > of random stuff. > > My bad. I failed to realize there is a platform device driver for syscon, > in addition to the existing "no struct device" implementation. Actually that doesn't do anything on DT platforms as of commit bdb0066df96e ("mfd: syscon: Decouple syscon interface from platform devices"). All the regmaps are, as I previously stated, not tied to any struct device. > > Here the provider device registers the (limited) regmap it wants to provide, > > tying the new regmap to itself. The consumer gets it via the dev_get_regmap() > > call. The provider has a main function and isn't exposing that part of its > > register map to the outside; only the random bits that were stuffed in are. > > > > > > With this scheme a device to could export just enough registers for the > > > > consumer to use, instead of the whole address range. > > > > > > > > We do this in the R40 clock controller as well, which has some bits that > > > > tweak the ethernet controllers RGMII delay... > > > > > > Not related. > > > > Related as in that is possibly what this code was based on, commit > > 49a06cae6e7c ("net: stmmac: dwmac-sun8i: Allow getting syscon regmap > > from external device"). > > > > > > ChenYu
On 28/11/2023 10:09, Chen-Yu Tsai wrote: > On Tue, Nov 28, 2023 at 5:02 PM Chen-Yu Tsai <wens@csie.org> wrote: >> >> On Tue, Nov 28, 2023 at 4:59 PM Chen-Yu Tsai <wens@csie.org> wrote: >>> >>> On Tue, Nov 28, 2023 at 4:30 PM Krzysztof Kozlowski >>> <krzysztof.kozlowski@linaro.org> wrote: >>>> >>>> On 28/11/2023 08:50, Chen-Yu Tsai wrote: >>>>> On Tue, Nov 28, 2023 at 3:43 PM Krzysztof Kozlowski >>>>> <krzysztof.kozlowski@linaro.org> wrote: >>>>>> >>>>>> On 28/11/2023 01:58, Andre Przywara wrote: >>>>>>> >>>>>>> +static struct regmap *sun8i_ths_get_syscon_regmap(struct device_node *node) >>>>>>> +{ >>>>>>> + struct device_node *syscon_node; >>>>>>> + struct platform_device *syscon_pdev; >>>>>>> + struct regmap *regmap = NULL; >>>>>>> + >>>>>>> + syscon_node = of_parse_phandle(node, "syscon", 0); >>>>>> >>>>>> Nope. For the 100th time, this cannot be generic. >>>>>> >>>>>>> + if (!syscon_node) >>>>>>> + return ERR_PTR(-ENODEV); >>>>>>> + >>>>>>> + syscon_pdev = of_find_device_by_node(syscon_node); >>>>>>> + if (!syscon_pdev) { >>>>>>> + /* platform device might not be probed yet */ >>>>>>> + regmap = ERR_PTR(-EPROBE_DEFER); >>>>>>> + goto out_put_node; >>>>>>> + } >>>>>>> + >>>>>>> + /* If no regmap is found then the other device driver is at fault */ >>>>>>> + regmap = dev_get_regmap(&syscon_pdev->dev, NULL); >>>>>>> + if (!regmap) >>>>>>> + regmap = ERR_PTR(-EINVAL); >>>>>> >>>>>> Aren't you open-coding existing API to get regmap from syscon? >>>>> >>>>> Not really. This is to get a regmap exported by the device. Syscon's regmap >>>>> is not tied to the device at all. >>>> >>>> I am talking about open-coding existing API. Look at syscon.h. >>> >>> The underlying implementation is different. >>> >>> syscon maintains its own mapping of device nodes to regmaps, and creates >>> regmaps when none exist. The regmap is not tied to any struct device. >>> syscon basically exists outside of the driver model and one has no control >>> over what is exposed because it is meant for blocks that are a collection >>> of random stuff. >> >> My bad. I failed to realize there is a platform device driver for syscon, >> in addition to the existing "no struct device" implementation. > > Actually that doesn't do anything on DT platforms as of commit bdb0066df96e > ("mfd: syscon: Decouple syscon interface from platform devices"). All the > regmaps are, as I previously stated, not tied to any struct device. Sorry, it's your third reply, so I don't know what exactly you want to discuss. This code open-codes existing API. Fix it. > >>> Here the provider device registers the (limited) regmap it wants to provide, >>> tying the new regmap to itself. The consumer gets it via the dev_get_regmap() >>> call. The provider has a main function and isn't exposing that part of its >>> register map to the outside; only the random bits that were stuffed in are. >>> >>>>> With this scheme a device to could export just enough registers for the >>>>> consumer to use, instead of the whole address range. >>>>> >>>>> We do this in the R40 clock controller as well, which has some bits that >>>>> tweak the ethernet controllers RGMII delay... >>>> >>>> Not related. >>> >>> Related as in that is possibly what this code was based on, commit >>> 49a06cae6e7c ("net: stmmac: dwmac-sun8i: Allow getting syscon regmap >>> from external device"). How duplicating a code is related to R40 controller? Duplicating code is generic problem, not specific and not related to your hardware. Best regards, Krzysztof
On 28/11/2023 10:13, Krzysztof Kozlowski wrote: > On 28/11/2023 10:09, Chen-Yu Tsai wrote: >> On Tue, Nov 28, 2023 at 5:02 PM Chen-Yu Tsai <wens@csie.org> wrote: >>> >>> On Tue, Nov 28, 2023 at 4:59 PM Chen-Yu Tsai <wens@csie.org> wrote: >>>> >>>> On Tue, Nov 28, 2023 at 4:30 PM Krzysztof Kozlowski >>>> <krzysztof.kozlowski@linaro.org> wrote: >>>>> >>>>> On 28/11/2023 08:50, Chen-Yu Tsai wrote: >>>>>> On Tue, Nov 28, 2023 at 3:43 PM Krzysztof Kozlowski >>>>>> <krzysztof.kozlowski@linaro.org> wrote: >>>>>>> >>>>>>> On 28/11/2023 01:58, Andre Przywara wrote: >>>>>>>> >>>>>>>> +static struct regmap *sun8i_ths_get_syscon_regmap(struct device_node *node) >>>>>>>> +{ >>>>>>>> + struct device_node *syscon_node; >>>>>>>> + struct platform_device *syscon_pdev; >>>>>>>> + struct regmap *regmap = NULL; >>>>>>>> + >>>>>>>> + syscon_node = of_parse_phandle(node, "syscon", 0); >>>>>>> >>>>>>> Nope. For the 100th time, this cannot be generic. >>>>>>> >>>>>>>> + if (!syscon_node) >>>>>>>> + return ERR_PTR(-ENODEV); >>>>>>>> + >>>>>>>> + syscon_pdev = of_find_device_by_node(syscon_node); >>>>>>>> + if (!syscon_pdev) { >>>>>>>> + /* platform device might not be probed yet */ >>>>>>>> + regmap = ERR_PTR(-EPROBE_DEFER); >>>>>>>> + goto out_put_node; >>>>>>>> + } >>>>>>>> + >>>>>>>> + /* If no regmap is found then the other device driver is at fault */ >>>>>>>> + regmap = dev_get_regmap(&syscon_pdev->dev, NULL); >>>>>>>> + if (!regmap) >>>>>>>> + regmap = ERR_PTR(-EINVAL); >>>>>>> >>>>>>> Aren't you open-coding existing API to get regmap from syscon? >>>>>> >>>>>> Not really. This is to get a regmap exported by the device. Syscon's regmap >>>>>> is not tied to the device at all. >>>>> >>>>> I am talking about open-coding existing API. Look at syscon.h. >>>> >>>> The underlying implementation is different. >>>> >>>> syscon maintains its own mapping of device nodes to regmaps, and creates >>>> regmaps when none exist. The regmap is not tied to any struct device. >>>> syscon basically exists outside of the driver model and one has no control >>>> over what is exposed because it is meant for blocks that are a collection >>>> of random stuff. >>> >>> My bad. I failed to realize there is a platform device driver for syscon, >>> in addition to the existing "no struct device" implementation. >> >> Actually that doesn't do anything on DT platforms as of commit bdb0066df96e >> ("mfd: syscon: Decouple syscon interface from platform devices"). All the >> regmaps are, as I previously stated, not tied to any struct device. > > > Sorry, it's your third reply, so I don't know what exactly you want to > discuss. > > This code open-codes existing API. Fix it. > >> >>>> Here the provider device registers the (limited) regmap it wants to provide, >>>> tying the new regmap to itself. The consumer gets it via the dev_get_regmap() >>>> call. The provider has a main function and isn't exposing that part of its >>>> register map to the outside; only the random bits that were stuffed in are. >>>> >>>>>> With this scheme a device to could export just enough registers for the >>>>>> consumer to use, instead of the whole address range. >>>>>> >>>>>> We do this in the R40 clock controller as well, which has some bits that >>>>>> tweak the ethernet controllers RGMII delay... >>>>> >>>>> Not related. >>>> >>>> Related as in that is possibly what this code was based on, commit >>>> 49a06cae6e7c ("net: stmmac: dwmac-sun8i: Allow getting syscon regmap >>>> from external device"). > > > How duplicating a code is related to R40 controller? Duplicating code is > generic problem, not specific and not related to your hardware. I think I understand now what you wanted to say - the "syscon" property is pointing not to a syscon. That's the mistake in the bindings - you claim it is a syscon, but it is not and has nothing to do with syscon. Neither in the bindings nor in the Linux drivers. This should be fixed. Best regards, Krzysztof
On Tue, 28 Nov 2023 08:43:32 +0100 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: Hi, > On 28/11/2023 01:58, Andre Przywara wrote: > > > > +static struct regmap *sun8i_ths_get_syscon_regmap(struct device_node *node) > > +{ > > + struct device_node *syscon_node; > > + struct platform_device *syscon_pdev; > > + struct regmap *regmap = NULL; > > + > > + syscon_node = of_parse_phandle(node, "syscon", 0); > > Nope. For the 100th time, this cannot be generic. OK. Shall this name refer to the required functionality (temperature offset fix) or to the target syscon node (like allwinner,misc-syscon). The problem is that this is really a syscon, as in: "random collection of bits that we didn't know where else to put in", so "syscon" alone actually says it all. And btw: it would have been about the same effort (and more helpful!) to type: "This cannot be generic, please check writing-bindings.rst." ;-) > > > + if (!syscon_node) > > + return ERR_PTR(-ENODEV); > > + > > + syscon_pdev = of_find_device_by_node(syscon_node); > > + if (!syscon_pdev) { > > + /* platform device might not be probed yet */ > > + regmap = ERR_PTR(-EPROBE_DEFER); > > + goto out_put_node; > > + } > > + > > + /* If no regmap is found then the other device driver is at fault */ > > + regmap = dev_get_regmap(&syscon_pdev->dev, NULL); > > + if (!regmap) > > + regmap = ERR_PTR(-EINVAL); > > Aren't you open-coding existing API to get regmap from syscon? That's a good point, I lifted that code from sun8i-emac.c, where we have the exact same problem. Unfortunately syscon_regmap_lookup_by_phandle() requires the syscon DT node to have "syscon" in its compatible string list, which we don't have. We actually explicitly dropped this for the A64 (with 1f1f5183981d70bf0950), and never added this for later SoCs in the first place. I guess we could add it back, and it would work for this case here (tested that), but then cannot replace the sun8i-emac.c code, because that would break older DTs. So is there any chance we can drop the requirement for "syscon" in the compatible string list, in the implementation of syscon_regmap_lookup_by_phandle()? Maybe optionally, using a different prototype? Or is there another existing API that does this already? Cheers, Andre
On 28/11/2023 15:33, Andre Przywara wrote: > On Tue, 28 Nov 2023 08:43:32 +0100 > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > Hi, > >> On 28/11/2023 01:58, Andre Przywara wrote: >>> >>> +static struct regmap *sun8i_ths_get_syscon_regmap(struct device_node *node) >>> +{ >>> + struct device_node *syscon_node; >>> + struct platform_device *syscon_pdev; >>> + struct regmap *regmap = NULL; >>> + >>> + syscon_node = of_parse_phandle(node, "syscon", 0); >> >> Nope. For the 100th time, this cannot be generic. > > OK. Shall this name refer to the required functionality (temperature > offset fix) or to the target syscon node (like allwinner,misc-syscon). > The problem is that this is really a syscon, as in: "random collection of > bits that we didn't know where else to put in", so "syscon" alone actually > says it all. Every syscon is a "random collection of bits...", but not every "random collection of bits..." is a syscon. Your target device does not implement syscon nodes. Your Linux implementation does not use it as syscon. Therefore if something does not look like syscon and does not behave like syscon, it is not a syscon. I looked at the bit and this is SRAM, not syscon. I am sorry, but it is something entirely different and we have a binding for it: "sram", I think. > > > And btw: it would have been about the same effort (and more helpful!) to > type: > > "This cannot be generic, please check writing-bindings.rst." ;-) > >> >>> + if (!syscon_node) >>> + return ERR_PTR(-ENODEV); >>> + >>> + syscon_pdev = of_find_device_by_node(syscon_node); >>> + if (!syscon_pdev) { >>> + /* platform device might not be probed yet */ >>> + regmap = ERR_PTR(-EPROBE_DEFER); >>> + goto out_put_node; >>> + } >>> + >>> + /* If no regmap is found then the other device driver is at fault */ >>> + regmap = dev_get_regmap(&syscon_pdev->dev, NULL); >>> + if (!regmap) >>> + regmap = ERR_PTR(-EINVAL); >> >> Aren't you open-coding existing API to get regmap from syscon? > > That's a good point, I lifted that code from sun8i-emac.c, where we have > the exact same problem. > Unfortunately syscon_regmap_lookup_by_phandle() requires the syscon DT > node to have "syscon" in its compatible string list, which we > don't have. We actually explicitly dropped this for the A64 (with > 1f1f5183981d70bf0950), and never added this for later SoCs in the first place. > I guess we could add it back, and it would work for this case here (tested > that), but then cannot replace the sun8i-emac.c code, because that would > break older DTs. > So is there any chance we can drop the requirement for "syscon" in the > compatible string list, in the implementation of > syscon_regmap_lookup_by_phandle()? Maybe optionally, using a different > prototype? Or is there another existing API that does this already? I must correct myself: I was wrong. You are not open-coding, because as pointed out, this is not a phandle to syscon (even if you call it like "syscon"). The code is fine, maybe except missing links (needs double checking, because maybe regmap creates links?). The DT binding and DTS needs fixing, because it is not a syscon. Best regards, Krzysztof
On Tue, 28 Nov 2023 15:48:18 +0100 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: Hi, (adding Maxime for the syscon question below) > On 28/11/2023 15:33, Andre Przywara wrote: > > On Tue, 28 Nov 2023 08:43:32 +0100 > > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > > > Hi, > > > >> On 28/11/2023 01:58, Andre Przywara wrote: > >>> > >>> +static struct regmap *sun8i_ths_get_syscon_regmap(struct device_node *node) > >>> +{ > >>> + struct device_node *syscon_node; > >>> + struct platform_device *syscon_pdev; > >>> + struct regmap *regmap = NULL; > >>> + > >>> + syscon_node = of_parse_phandle(node, "syscon", 0); > >> > >> Nope. For the 100th time, this cannot be generic. > > > > OK. Shall this name refer to the required functionality (temperature > > offset fix) or to the target syscon node (like allwinner,misc-syscon). > > The problem is that this is really a syscon, as in: "random collection of > > bits that we didn't know where else to put in", so "syscon" alone actually > > says it all. > > Every syscon is a "random collection of bits...", but not every "random > collection of bits..." is a syscon. > > Your target device does not implement syscon nodes. Your Linux > implementation does not use it as syscon. Therefore if something does > not look like syscon and does not behave like syscon, it is not a syscon. > > I looked at the bit and this is SRAM, not syscon. I am sorry, but it is > something entirely different and we have a binding for it: "sram", I think. Well, it's somehow both: On the face of it it's a SRAM controller, indeed: it can switch the control of certain SRAM regions between CPU access and peripheral access (for the video and the display engine). But then it's also a syscon, because on top of that, it also controls those random bits, for instance the EMAC clock register, and this ominous THS bit. I guess in hindsight we should have never dropped that "syscon" string then, but I am not sure if adding it back has side effects? And as I mentioned in the cover letter: modelling this as some SRAM region, as you suggest, might be an alternative, but it doesn't sound right either, as I don't think it really is one: I just tried in U-Boot, and I can write and read the whole SRAM C region just fine, with and without the bit set. And SRAM content is preserved, even with the thermal sensor running and the bit cleared (or set). So adding the "syscon" to the compatible would fix most things, but then we need to keep the open coded lookup code in dwmac-sun8i.c (because older DTs would break otherwise). What do people think about this? Samuel, does this affect the D1 LDO driver as well? Cheers, Andre > > > > > > > And btw: it would have been about the same effort (and more helpful!) to > > type: > > > > "This cannot be generic, please check writing-bindings.rst." ;-) > > > >> > >>> + if (!syscon_node) > >>> + return ERR_PTR(-ENODEV); > >>> + > >>> + syscon_pdev = of_find_device_by_node(syscon_node); > >>> + if (!syscon_pdev) { > >>> + /* platform device might not be probed yet */ > >>> + regmap = ERR_PTR(-EPROBE_DEFER); > >>> + goto out_put_node; > >>> + } > >>> + > >>> + /* If no regmap is found then the other device driver is at fault */ > >>> + regmap = dev_get_regmap(&syscon_pdev->dev, NULL); > >>> + if (!regmap) > >>> + regmap = ERR_PTR(-EINVAL); > >> > >> Aren't you open-coding existing API to get regmap from syscon? > > > > That's a good point, I lifted that code from sun8i-emac.c, where we have > > the exact same problem. > > Unfortunately syscon_regmap_lookup_by_phandle() requires the syscon DT > > node to have "syscon" in its compatible string list, which we > > don't have. We actually explicitly dropped this for the A64 (with > > 1f1f5183981d70bf0950), and never added this for later SoCs in the first place. > > I guess we could add it back, and it would work for this case here (tested > > that), but then cannot replace the sun8i-emac.c code, because that would > > break older DTs. > > So is there any chance we can drop the requirement for "syscon" in the > > compatible string list, in the implementation of > > syscon_regmap_lookup_by_phandle()? Maybe optionally, using a different > > prototype? Or is there another existing API that does this already? > > I must correct myself: I was wrong. You are not open-coding, because as > pointed out, this is not a phandle to syscon (even if you call it like > "syscon"). > > The code is fine, maybe except missing links (needs double checking, > because maybe regmap creates links?). The DT binding and DTS needs > fixing, because it is not a syscon. > > Best regards, > Krzysztof >
On Wed, Nov 29, 2023 at 12:10 AM Andre Przywara <andre.przywara@arm.com> wrote: > > On Tue, 28 Nov 2023 15:48:18 +0100 > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > Hi, > > (adding Maxime for the syscon question below) > > > On 28/11/2023 15:33, Andre Przywara wrote: > > > On Tue, 28 Nov 2023 08:43:32 +0100 > > > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > > > > > Hi, > > > > > >> On 28/11/2023 01:58, Andre Przywara wrote: > > >>> > > >>> +static struct regmap *sun8i_ths_get_syscon_regmap(struct device_node *node) > > >>> +{ > > >>> + struct device_node *syscon_node; > > >>> + struct platform_device *syscon_pdev; > > >>> + struct regmap *regmap = NULL; > > >>> + > > >>> + syscon_node = of_parse_phandle(node, "syscon", 0); > > >> > > >> Nope. For the 100th time, this cannot be generic. > > > > > > OK. Shall this name refer to the required functionality (temperature > > > offset fix) or to the target syscon node (like allwinner,misc-syscon). > > > The problem is that this is really a syscon, as in: "random collection of > > > bits that we didn't know where else to put in", so "syscon" alone actually > > > says it all. > > > > Every syscon is a "random collection of bits...", but not every "random > > collection of bits..." is a syscon. > > > > Your target device does not implement syscon nodes. Your Linux > > implementation does not use it as syscon. Therefore if something does > > not look like syscon and does not behave like syscon, it is not a syscon. > > > > I looked at the bit and this is SRAM, not syscon. I am sorry, but it is > > something entirely different and we have a binding for it: "sram", I think. > > Well, it's somehow both: On the face of it it's a SRAM controller, indeed: > it can switch the control of certain SRAM regions between CPU access and > peripheral access (for the video and the display engine). But then it's > also a syscon, because on top of that, it also controls those random bits, > for instance the EMAC clock register, and this ominous THS bit. > I guess in hindsight we should have never dropped that "syscon" string > then, but I am not sure if adding it back has side effects? Either way you would need to add locking around the register accesses, or you could, however unlikely, end up with two simultaneous read-update-write accesses by both consumers (THS and claiming C1). If you add the syscon string back, then you'd have to convert the SRAM controller driver to use syscon as well, as there is no way to provide a custom spinlock for the syscon regmap. Another reason why a driver would want to create its own regmap. > And as I mentioned in the cover letter: modelling this as some SRAM > region, as you suggest, might be an alternative, but it doesn't sound right > either, as I don't think it really is one: I just tried in U-Boot, and I > can write and read the whole SRAM C region just fine, with and without the > bit set. And SRAM content is preserved, even with the thermal sensor > running and the bit cleared (or set). > > So adding the "syscon" to the compatible would fix most things, but then > we need to keep the open coded lookup code in dwmac-sun8i.c (because older > DTs would break otherwise). dwmac-sun8i already falls back to syscon_regmap_lookup_by_phandle() because of even older DTs. I'm the one that added the open coded stuff, mostly because the R40 had the bits embedded in the clock controller, not the system control, and it seemed error prone and hard to debug for some other device to have full access. So you'd just be reverting the driver to the old ways. ChenYu > What do people think about this? > Samuel, does this affect the D1 LDO driver as well? > > Cheers, > Andre > > > > > > > > > > > > And btw: it would have been about the same effort (and more helpful!) to > > > type: > > > > > > "This cannot be generic, please check writing-bindings.rst." ;-) > > > > > >> > > >>> + if (!syscon_node) > > >>> + return ERR_PTR(-ENODEV); > > >>> + > > >>> + syscon_pdev = of_find_device_by_node(syscon_node); > > >>> + if (!syscon_pdev) { > > >>> + /* platform device might not be probed yet */ > > >>> + regmap = ERR_PTR(-EPROBE_DEFER); > > >>> + goto out_put_node; > > >>> + } > > >>> + > > >>> + /* If no regmap is found then the other device driver is at fault */ > > >>> + regmap = dev_get_regmap(&syscon_pdev->dev, NULL); > > >>> + if (!regmap) > > >>> + regmap = ERR_PTR(-EINVAL); > > >> > > >> Aren't you open-coding existing API to get regmap from syscon? > > > > > > That's a good point, I lifted that code from sun8i-emac.c, where we have > > > the exact same problem. > > > Unfortunately syscon_regmap_lookup_by_phandle() requires the syscon DT > > > node to have "syscon" in its compatible string list, which we > > > don't have. We actually explicitly dropped this for the A64 (with > > > 1f1f5183981d70bf0950), and never added this for later SoCs in the first place. > > > I guess we could add it back, and it would work for this case here (tested > > > that), but then cannot replace the sun8i-emac.c code, because that would > > > break older DTs. > > > So is there any chance we can drop the requirement for "syscon" in the > > > compatible string list, in the implementation of > > > syscon_regmap_lookup_by_phandle()? Maybe optionally, using a different > > > prototype? Or is there another existing API that does this already? > > > > I must correct myself: I was wrong. You are not open-coding, because as > > pointed out, this is not a phandle to syscon (even if you call it like > > "syscon"). > > > > The code is fine, maybe except missing links (needs double checking, > > because maybe regmap creates links?). The DT binding and DTS needs > > fixing, because it is not a syscon. > > > > Best regards, > > Krzysztof > > >
On Tue, Nov 28, 2023 at 10:10 AM Andre Przywara <andre.przywara@arm.com> wrote: > > On Tue, 28 Nov 2023 15:48:18 +0100 > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > Hi, > > (adding Maxime for the syscon question below) > > > On 28/11/2023 15:33, Andre Przywara wrote: > > > On Tue, 28 Nov 2023 08:43:32 +0100 > > > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > > > > > Hi, > > > > > >> On 28/11/2023 01:58, Andre Przywara wrote: > > >>> > > >>> +static struct regmap *sun8i_ths_get_syscon_regmap(struct device_node *node) > > >>> +{ > > >>> + struct device_node *syscon_node; > > >>> + struct platform_device *syscon_pdev; > > >>> + struct regmap *regmap = NULL; > > >>> + > > >>> + syscon_node = of_parse_phandle(node, "syscon", 0); > > >> > > >> Nope. For the 100th time, this cannot be generic. Unless it is the 100th time for the submitter, please just point to the documentation. Can we simply ban "syscon" as a property name? It looks like we have 65 cases in upstream dts files. Maybe that's doable. This is where we need levels of warnings with okay for existing vs. don't use in new designs. > > > OK. Shall this name refer to the required functionality (temperature > > > offset fix) or to the target syscon node (like allwinner,misc-syscon). > > > The problem is that this is really a syscon, as in: "random collection of > > > bits that we didn't know where else to put in", so "syscon" alone actually > > > says it all. > > > > Every syscon is a "random collection of bits...", but not every "random > > collection of bits..." is a syscon. > > > > Your target device does not implement syscon nodes. Your Linux > > implementation does not use it as syscon. Therefore if something does > > not look like syscon and does not behave like syscon, it is not a syscon. > > > > I looked at the bit and this is SRAM, not syscon. I am sorry, but it is > > something entirely different and we have a binding for it: "sram", I think. > > Well, it's somehow both: On the face of it it's a SRAM controller, indeed: > it can switch the control of certain SRAM regions between CPU access and > peripheral access (for the video and the display engine). But then it's > also a syscon, because on top of that, it also controls those random bits, > for instance the EMAC clock register, and this ominous THS bit. > I guess in hindsight we should have never dropped that "syscon" string > then, but I am not sure if adding it back has side effects? > > And as I mentioned in the cover letter: modelling this as some SRAM > region, as you suggest, might be an alternative, but it doesn't sound right > either, as I don't think it really is one: I just tried in U-Boot, and I > can write and read the whole SRAM C region just fine, with and without the > bit set. And SRAM content is preserved, even with the thermal sensor > running and the bit cleared (or set). > > So adding the "syscon" to the compatible would fix most things, but then > we need to keep the open coded lookup code in dwmac-sun8i.c (because older > DTs would break otherwise). Really, I'd like to get rid of the "syscon" compatible. It is nothing more than a flag for Linux to create a regmap. Not a fully baked idea, but perhaps what is needed is drivers that request a regmap for a node simply get one regardless. That kind of throws out the Linux driver model though. Alternatively with no "syscon" compatible, we'd have to have table(s) of 100s of compatibles in the kernel. Rob
Hi, On 28/11/2023 16:50, Rob Herring wrote: > On Tue, Nov 28, 2023 at 10:10 AM Andre Przywara <andre.przywara@arm.com> wrote: >> >> On Tue, 28 Nov 2023 15:48:18 +0100 >> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: >> >> Hi, >> >> (adding Maxime for the syscon question below) >> >>> On 28/11/2023 15:33, Andre Przywara wrote: >>>> On Tue, 28 Nov 2023 08:43:32 +0100 >>>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: >>>> >>>> Hi, >>>> >>>>> On 28/11/2023 01:58, Andre Przywara wrote: >>>>>> >>>>>> +static struct regmap *sun8i_ths_get_syscon_regmap(struct device_node *node) >>>>>> +{ >>>>>> + struct device_node *syscon_node; >>>>>> + struct platform_device *syscon_pdev; >>>>>> + struct regmap *regmap = NULL; >>>>>> + >>>>>> + syscon_node = of_parse_phandle(node, "syscon", 0); >>>>> >>>>> Nope. For the 100th time, this cannot be generic. > > Unless it is the 100th time for the submitter, please just point to > the documentation. > > Can we simply ban "syscon" as a property name? It looks like we have > 65 cases in upstream dts files. Maybe that's doable. This is where we > need levels of warnings with okay for existing vs. don't use in new > designs. > >>>> OK. Shall this name refer to the required functionality (temperature >>>> offset fix) or to the target syscon node (like allwinner,misc-syscon). >>>> The problem is that this is really a syscon, as in: "random collection of >>>> bits that we didn't know where else to put in", so "syscon" alone actually >>>> says it all. >>> >>> Every syscon is a "random collection of bits...", but not every "random >>> collection of bits..." is a syscon. >>> >>> Your target device does not implement syscon nodes. Your Linux >>> implementation does not use it as syscon. Therefore if something does >>> not look like syscon and does not behave like syscon, it is not a syscon. >>> >>> I looked at the bit and this is SRAM, not syscon. I am sorry, but it is >>> something entirely different and we have a binding for it: "sram", I think. >> >> Well, it's somehow both: On the face of it it's a SRAM controller, indeed: >> it can switch the control of certain SRAM regions between CPU access and >> peripheral access (for the video and the display engine). But then it's >> also a syscon, because on top of that, it also controls those random bits, >> for instance the EMAC clock register, and this ominous THS bit. >> I guess in hindsight we should have never dropped that "syscon" string >> then, but I am not sure if adding it back has side effects? >> >> And as I mentioned in the cover letter: modelling this as some SRAM >> region, as you suggest, might be an alternative, but it doesn't sound right >> either, as I don't think it really is one: I just tried in U-Boot, and I >> can write and read the whole SRAM C region just fine, with and without the >> bit set. And SRAM content is preserved, even with the thermal sensor >> running and the bit cleared (or set). >> >> So adding the "syscon" to the compatible would fix most things, but then >> we need to keep the open coded lookup code in dwmac-sun8i.c (because older >> DTs would break otherwise). > > Really, I'd like to get rid of the "syscon" compatible. It is nothing > more than a flag for Linux to create a regmap. Yeah, so thinking about it indeed feels a bit like we are changing the DT here to cater for some Linux implementation detail. After all we already access the regmap successfully in dwmac-sun8i.c, is that approach frowned upon (because: driver model) and just tolerated because it's already in the code base? > Not a fully baked idea, but perhaps what is needed is drivers that > request a regmap for a node simply get one regardless. That kind of > throws out the Linux driver model though. Alternatively with no > "syscon" compatible, we'd have to have table(s) of 100s of compatibles > in the kernel. So do you mean to either just remove the explicit syscon compatible check in syscon_node_to_regmap(), or replace it with a check against a list of allowed devices? Wouldn't it be sufficient to leave that check to the (syscon-like) devices, by them exporting a regmap in the first place or not? And we can do filtering of accesses there, like we do in sunxi_sram.c? Cheers, Andre > > Rob
On Thu, Nov 30, 2023 at 1:03 AM Andre Przywara <andre.przywara@arm.com> wrote: > > Hi, > > On 28/11/2023 16:50, Rob Herring wrote: > > On Tue, Nov 28, 2023 at 10:10 AM Andre Przywara <andre.przywara@arm.com> wrote: > >> > >> On Tue, 28 Nov 2023 15:48:18 +0100 > >> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > >> > >> Hi, > >> > >> (adding Maxime for the syscon question below) > >> > >>> On 28/11/2023 15:33, Andre Przywara wrote: > >>>> On Tue, 28 Nov 2023 08:43:32 +0100 > >>>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > >>>> > >>>> Hi, > >>>> > >>>>> On 28/11/2023 01:58, Andre Przywara wrote: > >>>>>> > >>>>>> +static struct regmap *sun8i_ths_get_syscon_regmap(struct device_node *node) > >>>>>> +{ > >>>>>> + struct device_node *syscon_node; > >>>>>> + struct platform_device *syscon_pdev; > >>>>>> + struct regmap *regmap = NULL; > >>>>>> + > >>>>>> + syscon_node = of_parse_phandle(node, "syscon", 0); > >>>>> > >>>>> Nope. For the 100th time, this cannot be generic. > > > > Unless it is the 100th time for the submitter, please just point to > > the documentation. > > > > Can we simply ban "syscon" as a property name? It looks like we have > > 65 cases in upstream dts files. Maybe that's doable. This is where we > > need levels of warnings with okay for existing vs. don't use in new > > designs. > > > >>>> OK. Shall this name refer to the required functionality (temperature > >>>> offset fix) or to the target syscon node (like allwinner,misc-syscon). > >>>> The problem is that this is really a syscon, as in: "random collection of > >>>> bits that we didn't know where else to put in", so "syscon" alone actually > >>>> says it all. > >>> > >>> Every syscon is a "random collection of bits...", but not every "random > >>> collection of bits..." is a syscon. > >>> > >>> Your target device does not implement syscon nodes. Your Linux > >>> implementation does not use it as syscon. Therefore if something does > >>> not look like syscon and does not behave like syscon, it is not a syscon. > >>> > >>> I looked at the bit and this is SRAM, not syscon. I am sorry, but it is > >>> something entirely different and we have a binding for it: "sram", I think. > >> > >> Well, it's somehow both: On the face of it it's a SRAM controller, indeed: > >> it can switch the control of certain SRAM regions between CPU access and > >> peripheral access (for the video and the display engine). But then it's > >> also a syscon, because on top of that, it also controls those random bits, > >> for instance the EMAC clock register, and this ominous THS bit. > >> I guess in hindsight we should have never dropped that "syscon" string > >> then, but I am not sure if adding it back has side effects? > >> > >> And as I mentioned in the cover letter: modelling this as some SRAM > >> region, as you suggest, might be an alternative, but it doesn't sound right > >> either, as I don't think it really is one: I just tried in U-Boot, and I > >> can write and read the whole SRAM C region just fine, with and without the > >> bit set. And SRAM content is preserved, even with the thermal sensor > >> running and the bit cleared (or set). > >> > >> So adding the "syscon" to the compatible would fix most things, but then > >> we need to keep the open coded lookup code in dwmac-sun8i.c (because older > >> DTs would break otherwise). > > > > Really, I'd like to get rid of the "syscon" compatible. It is nothing > > more than a flag for Linux to create a regmap. > > Yeah, so thinking about it indeed feels a bit like we are changing the > DT here to cater for some Linux implementation detail. After all we > already access the regmap successfully in dwmac-sun8i.c, is that > approach frowned upon (because: driver model) and just tolerated because > it's already in the code base? > > > Not a fully baked idea, but perhaps what is needed is drivers that > > request a regmap for a node simply get one regardless. That kind of > throws out the Linux driver model though. Alternatively with no > > "syscon" compatible, we'd have to have table(s) of 100s of compatibles > > in the kernel. > > So do you mean to either just remove the explicit syscon compatible > check in syscon_node_to_regmap(), or replace it with a check against a > list of allowed devices? There is already device_node_to_regmap() which skips the check. It still bypasses the driver model though. > Wouldn't it be sufficient to leave that check to the (syscon-like) > devices, by them exporting a regmap in the first place or not? And we > can do filtering of accesses there, like we do in sunxi_sram.c? > > Cheers, > Andre > > > > > > Rob
diff --git a/drivers/thermal/sun8i_thermal.c b/drivers/thermal/sun8i_thermal.c index 44554c3efc96c..920e419ce7343 100644 --- a/drivers/thermal/sun8i_thermal.c +++ b/drivers/thermal/sun8i_thermal.c @@ -15,6 +15,7 @@ #include <linux/module.h> #include <linux/nvmem-consumer.h> #include <linux/of.h> +#include <linux/of_platform.h> #include <linux/platform_device.h> #include <linux/regmap.h> #include <linux/reset.h> @@ -66,6 +67,7 @@ struct tsensor { struct ths_thermal_chip { bool has_mod_clk; bool has_bus_clk_reset; + bool needs_syscon; int sensor_num; int offset; int scale; @@ -83,6 +85,7 @@ struct ths_device { const struct ths_thermal_chip *chip; struct device *dev; struct regmap *regmap; + struct regmap_field *syscon_regmap_field; struct reset_control *reset; struct clk *bus_clk; struct clk *mod_clk; @@ -325,6 +328,34 @@ static void sun8i_ths_reset_control_assert(void *data) reset_control_assert(data); } +static struct regmap *sun8i_ths_get_syscon_regmap(struct device_node *node) +{ + struct device_node *syscon_node; + struct platform_device *syscon_pdev; + struct regmap *regmap = NULL; + + syscon_node = of_parse_phandle(node, "syscon", 0); + if (!syscon_node) + return ERR_PTR(-ENODEV); + + syscon_pdev = of_find_device_by_node(syscon_node); + if (!syscon_pdev) { + /* platform device might not be probed yet */ + regmap = ERR_PTR(-EPROBE_DEFER); + goto out_put_node; + } + + /* If no regmap is found then the other device driver is at fault */ + regmap = dev_get_regmap(&syscon_pdev->dev, NULL); + if (!regmap) + regmap = ERR_PTR(-EINVAL); + + platform_device_put(syscon_pdev); +out_put_node: + of_node_put(syscon_node); + return regmap; +} + static int sun8i_ths_resource_init(struct ths_device *tmdev) { struct device *dev = tmdev->dev; @@ -369,6 +400,21 @@ static int sun8i_ths_resource_init(struct ths_device *tmdev) if (ret) return ret; + if (tmdev->chip->needs_syscon) { + const struct reg_field sun8i_syscon_reg_field = + REG_FIELD(0x0, 16, 16); + struct regmap *regmap; + + regmap = sun8i_ths_get_syscon_regmap(dev->of_node); + if (IS_ERR(regmap)) + return PTR_ERR(regmap); + tmdev->syscon_regmap_field = devm_regmap_field_alloc(dev, + regmap, + sun8i_syscon_reg_field); + if (IS_ERR(tmdev->syscon_regmap_field)) + return PTR_ERR(tmdev->syscon_regmap_field); + } + ret = sun8i_ths_calibrate(tmdev); if (ret) return ret; @@ -415,6 +461,10 @@ static int sun50i_h6_thermal_init(struct ths_device *tmdev) { int val; + /* The H616 needs to have a bit in the SRAM control register cleared. */ + if (tmdev->syscon_regmap_field) + regmap_field_write(tmdev->syscon_regmap_field, 0); + /* * The manual recommends an overall sample frequency of 50 KHz (20us, * 480 cycles at 24 MHz), which provides plenty of time for both the
The Allwinner H616 SoC needs to clear a bit in one register in the SRAM controller (exported as a syscon), to report reasonable temperature values. On reset, bit 16 in register 0x3000000 is set, which leads to the driver reporting temperatures around 200C. Clearing this bit brings the values down to the expected range. The BSP code does a one-time write in U-Boot, with a comment just mentioning the effect on the THS, but offering no further explanation. To not rely on firmware to set things up for us, add code that queries the syscon device via a DT phandle link, then clear just this single bit. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- drivers/thermal/sun8i_thermal.c | 50 +++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+)