Re: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function
On 4/6/20 1:28 PM, Tom Rini wrote: > On Mon, Mar 09, 2020 at 01:21:59AM -0700, chee.hong@intel.com wrote: > >> From: Chee Hong Ang >> >> This commit (82de42fa14682d408da935adfb0f935354c5008f) calls child's >> ofdata_to_platdata() method before the parent is probed in dm core. >> This has caused the driver no longer able to get the correct parent >> clock's register base in the ofdata_to_platdata() method because the >> parent clocks will only be probed after the child's ofdata_to_platdata(). >> To resolve this, the clock parent's register base will only be retrieved >> by the child in probe() method instead of ofdata_to_platdata(). >> >> Signed-off-by: Chee Hong Ang >> Reviewed-by: Ley Foon Tan > > Applied to u-boot/master, thanks! You probably want to replace this one with [PATCH] dm: core: Read parent ofdata before children
Re: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function
On Mon, Mar 09, 2020 at 01:21:59AM -0700, chee.hong@intel.com wrote: > From: Chee Hong Ang > > This commit (82de42fa14682d408da935adfb0f935354c5008f) calls child's > ofdata_to_platdata() method before the parent is probed in dm core. > This has caused the driver no longer able to get the correct parent > clock's register base in the ofdata_to_platdata() method because the > parent clocks will only be probed after the child's ofdata_to_platdata(). > To resolve this, the clock parent's register base will only be retrieved > by the child in probe() method instead of ofdata_to_platdata(). > > Signed-off-by: Chee Hong Ang > Reviewed-by: Ley Foon Tan Applied to u-boot/master, thanks! -- Tom signature.asc Description: PGP signature
RE: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function
> -Original Message- > From: Tom Rini > Sent: Friday, April 3, 2020 8:21 PM > To: Tan, Ley Foon > Cc: Marek Vasut ; Simon Glass ; Ang, > Chee Hong ; U-Boot Mailing List b...@lists.denx.de>; Simon Goldschmidt > ; See, Chin Liang > ; Westergreen, Dalon > > Subject: Re: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register > base in probe function > > On Fri, Apr 03, 2020 at 03:52:54AM +, Tan, Ley Foon wrote: > > > > > > > -Original Message- > > > From: Marek Vasut > > > Sent: Friday, April 3, 2020 6:47 AM > > > To: Tom Rini > > > Cc: Simon Glass ; Ang, Chee Hong > > > ; U-Boot Mailing List > > > ; Simon Goldschmidt > > > ; See, Chin Liang > > > ; Tan, Ley Foon ; > > > Westergreen, Dalon > > > Subject: Re: [PATCH v1 1/2] clk: socfpga: Read the clock parent's > > > register base in probe function > > > > > > On 4/3/20 12:10 AM, Tom Rini wrote: > > > > On Thu, Apr 02, 2020 at 11:07:31PM +0200, Marek Vasut wrote: > > > >> On 4/2/20 10:54 PM, Tom Rini wrote: > > > >> [...] > > > >> > > > >>>>>>>> I'm not sure it definitely should be changed. But I'll do a > > > >>>>>>>> patch and see how it looks. > > > >>>>>>> > > > >>>>>>> Do I understand it correctly that the patch > > > >>>>>>> 82de42fa14682d408da935adfb0f935354c5008f actually > completely > > > >>>>>>> breaks SoCFPGA ? Then I would say this is a release blocker ? > > > >>>>>> Yes. A10 SPL won't boot at all. It crashes during the clock > > > >>>>>> manager > > > setup. > > > >>>>> > > > >>>>> This came in right at the beginning of the cycle. I thought > > > >>>>> the purpose of the 3-month cycle was to allow time to test? > > > >>>> > > > >>>> It was ... altera ? > > > >>> > > > >>> Sorry, I'm missing how that's an answer to the question. This > > > >>> came in basically right at the start of the merge window. > > > >> > > > >> I don't have an A10 available right now, so what can I do ? > > > > > > > > Ah, so the answer is "I can't test this platform myself". That's > > > > what then, thanks. > > > > > > Clearly Altera can , since they reported this issue. > > Yes, we have Arria10 devkit here and we see the issue when testing latest > uboot. > > It looks like other platform [1] also encounter issue with patch > 82de42fa14682d40. Like Marek said, we don't know any other driver is > broken after this patch. > > > > [1] https://patchwork.ozlabs.org/patch/1265188/ > > Yes, my concern here is that the platform in question wasn't tested after -rc1 > or -rc2 or -rc3 only right now just before release. So we're in a bit of a > scramble to fix the driver. Thanks! > We will improve our regression test going forward. Sorry about that. Thanks. Regards Ley Foon
Re: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function
On Fri, Apr 03, 2020 at 03:52:54AM +, Tan, Ley Foon wrote: > > > > -Original Message- > > From: Marek Vasut > > Sent: Friday, April 3, 2020 6:47 AM > > To: Tom Rini > > Cc: Simon Glass ; Ang, Chee Hong > > ; U-Boot Mailing List ; > > Simon Goldschmidt ; See, Chin Liang > > ; Tan, Ley Foon ; > > Westergreen, Dalon > > Subject: Re: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register > > base in probe function > > > > On 4/3/20 12:10 AM, Tom Rini wrote: > > > On Thu, Apr 02, 2020 at 11:07:31PM +0200, Marek Vasut wrote: > > >> On 4/2/20 10:54 PM, Tom Rini wrote: > > >> [...] > > >> > > >>>>>>>> I'm not sure it definitely should be changed. But I'll do a > > >>>>>>>> patch and see how it looks. > > >>>>>>> > > >>>>>>> Do I understand it correctly that the patch > > >>>>>>> 82de42fa14682d408da935adfb0f935354c5008f actually completely > > >>>>>>> breaks SoCFPGA ? Then I would say this is a release blocker ? > > >>>>>> Yes. A10 SPL won't boot at all. It crashes during the clock manager > > setup. > > >>>>> > > >>>>> This came in right at the beginning of the cycle. I thought the > > >>>>> purpose of the 3-month cycle was to allow time to test? > > >>>> > > >>>> It was ... altera ? > > >>> > > >>> Sorry, I'm missing how that's an answer to the question. This came > > >>> in basically right at the start of the merge window. > > >> > > >> I don't have an A10 available right now, so what can I do ? > > > > > > Ah, so the answer is "I can't test this platform myself". That's what > > > then, thanks. > > > > Clearly Altera can , since they reported this issue. > Yes, we have Arria10 devkit here and we see the issue when testing latest > uboot. > It looks like other platform [1] also encounter issue with patch > 82de42fa14682d40. Like Marek said, we don't know any other driver is broken > after this patch. > > [1] https://patchwork.ozlabs.org/patch/1265188/ Yes, my concern here is that the platform in question wasn't tested after -rc1 or -rc2 or -rc3 only right now just before release. So we're in a bit of a scramble to fix the driver. Thanks! -- Tom signature.asc Description: PGP signature
RE: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function
> -Original Message- > From: Marek Vasut > Sent: Friday, April 3, 2020 6:47 AM > To: Tom Rini > Cc: Simon Glass ; Ang, Chee Hong > ; U-Boot Mailing List ; > Simon Goldschmidt ; See, Chin Liang > ; Tan, Ley Foon ; > Westergreen, Dalon > Subject: Re: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register > base in probe function > > On 4/3/20 12:10 AM, Tom Rini wrote: > > On Thu, Apr 02, 2020 at 11:07:31PM +0200, Marek Vasut wrote: > >> On 4/2/20 10:54 PM, Tom Rini wrote: > >> [...] > >> > >>>>>>>> I'm not sure it definitely should be changed. But I'll do a > >>>>>>>> patch and see how it looks. > >>>>>>> > >>>>>>> Do I understand it correctly that the patch > >>>>>>> 82de42fa14682d408da935adfb0f935354c5008f actually completely > >>>>>>> breaks SoCFPGA ? Then I would say this is a release blocker ? > >>>>>> Yes. A10 SPL won't boot at all. It crashes during the clock manager > setup. > >>>>> > >>>>> This came in right at the beginning of the cycle. I thought the > >>>>> purpose of the 3-month cycle was to allow time to test? > >>>> > >>>> It was ... altera ? > >>> > >>> Sorry, I'm missing how that's an answer to the question. This came > >>> in basically right at the start of the merge window. > >> > >> I don't have an A10 available right now, so what can I do ? > > > > Ah, so the answer is "I can't test this platform myself". That's what > > then, thanks. > > Clearly Altera can , since they reported this issue. Yes, we have Arria10 devkit here and we see the issue when testing latest uboot. It looks like other platform [1] also encounter issue with patch 82de42fa14682d40. Like Marek said, we don't know any other driver is broken after this patch. [1] https://patchwork.ozlabs.org/patch/1265188/ Regards Ley Foon
Re: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function
On 4/3/20 12:10 AM, Tom Rini wrote: > On Thu, Apr 02, 2020 at 11:07:31PM +0200, Marek Vasut wrote: >> On 4/2/20 10:54 PM, Tom Rini wrote: >> [...] >> I'm not sure it definitely should be changed. But I'll do a patch and see how it looks. >>> >>> Do I understand it correctly that the patch >>> 82de42fa14682d408da935adfb0f935354c5008f actually completely breaks >>> SoCFPGA ? Then I would say this is a release blocker ? >> Yes. A10 SPL won't boot at all. It crashes during the clock manager >> setup. > > This came in right at the beginning of the cycle. I thought the > purpose of the 3-month cycle was to allow time to test? It was ... altera ? >>> >>> Sorry, I'm missing how that's an answer to the question. This came in >>> basically right at the start of the merge window. >> >> I don't have an A10 available right now, so what can I do ? > > Ah, so the answer is "I can't test this platform myself". That's what > then, thanks. Clearly Altera can , since they reported this issue.
Re: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function
On Thu, Apr 02, 2020 at 11:07:31PM +0200, Marek Vasut wrote: > On 4/2/20 10:54 PM, Tom Rini wrote: > [...] > > >> I'm not sure it definitely should be changed. But I'll do a patch and > >> see how it looks. > > > > Do I understand it correctly that the patch > > 82de42fa14682d408da935adfb0f935354c5008f actually completely breaks > > SoCFPGA ? Then I would say this is a release blocker ? > Yes. A10 SPL won't boot at all. It crashes during the clock manager > setup. > >>> > >>> This came in right at the beginning of the cycle. I thought the > >>> purpose of the 3-month cycle was to allow time to test? > >> > >> It was ... altera ? > > > > Sorry, I'm missing how that's an answer to the question. This came in > > basically right at the start of the merge window. > > I don't have an A10 available right now, so what can I do ? Ah, so the answer is "I can't test this platform myself". That's what then, thanks. -- Tom signature.asc Description: PGP signature
Re: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function
Hi Marek, On Thu, 2 Apr 2020 at 15:07, Marek Vasut wrote: > > On 4/2/20 10:54 PM, Tom Rini wrote: > [...] > > >> I'm not sure it definitely should be changed. But I'll do a patch and > >> see how it looks. > > > > Do I understand it correctly that the patch > > 82de42fa14682d408da935adfb0f935354c5008f actually completely breaks > > SoCFPGA ? Then I would say this is a release blocker ? > Yes. A10 SPL won't boot at all. It crashes during the clock manager > setup. > >>> > >>> This came in right at the beginning of the cycle. I thought the > >>> purpose of the 3-month cycle was to allow time to test? > >> > >> It was ... altera ? > > > > Sorry, I'm missing how that's an answer to the question. This came in > > basically right at the start of the merge window. > > I don't have an A10 available right now, so what can I do ? > > >>> I do plan to try out changing the behaviour to read a parent's ofdata > >>> before the child, but I am not comfortable adding such a major change > >>> just before a release. It could have any number of ill effects. > >>> > >>> Can you update the clock driver? E.g. you could move some of the code > >>> from socfpga_a10_ofdata_to_platdata() to a probe() method? > >> > >> Can we revert the patch which broke arria10 instead ? It did work > >> before, so who knows how many other ill side effects there are ... > > > > I'm not in favor of reverting 82de42fa1468 unless the answer is "that > > commit was wrong" and not "that commit caused other problems to surface, > > that we can fix". I am quite willing to say "we need to delay the > > release to test changes for problems that've surfaced and verify more > > hardware is fine". > > If I understand this correctly, the clock code did scan the DT and bound > all the clocks, so that drivers can use them. That's how bind function > should work, it creates instances of devices, but without probing them. > This doesn't work anymore if I understand it correctly, but that means > the basic premise of DT is broken ? And the solution is to do more magic > in probe function , which I don't think even works here ? No...please see here from the cover letter: Cover-letter: dm: core: Fully separate ofdata_to_platdata() from probe() At present ofdata_to_platdata() is done as part of device_probe(). Drivers are supposed to read their platdata in the ofdata_to_platdata method() but this requirement is not always followed. Having these methods separate helps deal with of-platdata, where the probe() method is common to both DT/of-platdata operation, but the ofdata_to_platdata() method is implemented differently. Another case has come up where this separate is useful. Generation of ACPI tables uses the of-platdata but does not want to probe the device. Probing would cause U-Boot to violate one of its design principles, viz that it should only probe devices that are used. For ACPI we want to generate a table for each device, even if U-Boot does not use it. In fact it may not even be possible to probe the device - e.g. an SD card which is not present will cause an error on probe, yet we still must tell Linux about the SD card connector in case it is used while Linux is running. This series splits out the ofdata_to_platdata() part of device_probe() so that it can be used separately from device_probe(). Thus devices now go through two distinct states when probing: reading platform data and actually touching the hardware to bring the device up. This should not break existing boards since the operations still happen in mostly the same order. The main change is that parents and uclasses are probed after ofdata_to_platdata() is called. HOWEVER it is quite possible that some boards break the rules and due to a series of unfortunate events, something will break. Two boards were found in this category already. SO this series may require some tidying up from board maintainers, if problems arise. Note that there are cases where devices must be probed in the ofdata_to_platdata() method. An example is where a GPIO is selected - this obviously requires that the GPIO device is probed. One board was found to burst its size limit with this series, despite the very small size increase. The patches to remove use of BUG_ON() are to ensure that this series passes tests. END Regards, Simon
Re: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function
On 4/2/20 10:54 PM, Tom Rini wrote: [...] >> I'm not sure it definitely should be changed. But I'll do a patch and >> see how it looks. > > Do I understand it correctly that the patch > 82de42fa14682d408da935adfb0f935354c5008f actually completely breaks > SoCFPGA ? Then I would say this is a release blocker ? Yes. A10 SPL won't boot at all. It crashes during the clock manager setup. >>> >>> This came in right at the beginning of the cycle. I thought the >>> purpose of the 3-month cycle was to allow time to test? >> >> It was ... altera ? > > Sorry, I'm missing how that's an answer to the question. This came in > basically right at the start of the merge window. I don't have an A10 available right now, so what can I do ? >>> I do plan to try out changing the behaviour to read a parent's ofdata >>> before the child, but I am not comfortable adding such a major change >>> just before a release. It could have any number of ill effects. >>> >>> Can you update the clock driver? E.g. you could move some of the code >>> from socfpga_a10_ofdata_to_platdata() to a probe() method? >> >> Can we revert the patch which broke arria10 instead ? It did work >> before, so who knows how many other ill side effects there are ... > > I'm not in favor of reverting 82de42fa1468 unless the answer is "that > commit was wrong" and not "that commit caused other problems to surface, > that we can fix". I am quite willing to say "we need to delay the > release to test changes for problems that've surfaced and verify more > hardware is fine". If I understand this correctly, the clock code did scan the DT and bound all the clocks, so that drivers can use them. That's how bind function should work, it creates instances of devices, but without probing them. This doesn't work anymore if I understand it correctly, but that means the basic premise of DT is broken ? And the solution is to do more magic in probe function , which I don't think even works here ?
Re: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function
On Thu, Apr 02, 2020 at 09:45:25PM +0200, Marek Vasut wrote: > On 4/2/20 8:50 PM, Simon Glass wrote: > > Hi. > > Hi, > > [...] > > I suspect we could change this, so that > device_ofdata_to_platdata() first calls itself on its parent. > > I can think of various reasons why this change might be desirable. > >>> > >>> I think this is how it worked before already. > >> > >> Well effectively, yes, because ofdata and probe were joined together. > > > Simon, do you have plan to fix this DM core issue ? > > I'm not sure it definitely should be changed. But I'll do a patch and > see how it looks. > >>> > >>> Do I understand it correctly that the patch > >>> 82de42fa14682d408da935adfb0f935354c5008f actually completely breaks > >>> SoCFPGA ? Then I would say this is a release blocker ? > >> Yes. A10 SPL won't boot at all. It crashes during the clock manager setup. > > > > This came in right at the beginning of the cycle. I thought the > > purpose of the 3-month cycle was to allow time to test? > > It was ... altera ? Sorry, I'm missing how that's an answer to the question. This came in basically right at the start of the merge window. > > I do plan to try out changing the behaviour to read a parent's ofdata > > before the child, but I am not comfortable adding such a major change > > just before a release. It could have any number of ill effects. > > > > Can you update the clock driver? E.g. you could move some of the code > > from socfpga_a10_ofdata_to_platdata() to a probe() method? > > Can we revert the patch which broke arria10 instead ? It did work > before, so who knows how many other ill side effects there are ... I'm not in favor of reverting 82de42fa1468 unless the answer is "that commit was wrong" and not "that commit caused other problems to surface, that we can fix". I am quite willing to say "we need to delay the release to test changes for problems that've surfaced and verify more hardware is fine". -- Tom signature.asc Description: PGP signature
Re: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function
On 02.04.2020 21:54, Marek Vasut wrote: On 4/2/20 9:53 PM, Simon Goldschmidt wrote: On 02.04.2020 21:49, Simon Glass wrote: Hi Marek, On Thu, 2 Apr 2020 at 13:45, Marek Vasut wrote: On 4/2/20 8:50 PM, Simon Glass wrote: Hi. Hi, [...] I suspect we could change this, so that device_ofdata_to_platdata() first calls itself on its parent. I can think of various reasons why this change might be desirable. I think this is how it worked before already. Well effectively, yes, because ofdata and probe were joined together. Simon, do you have plan to fix this DM core issue ? I'm not sure it definitely should be changed. But I'll do a patch and see how it looks. Do I understand it correctly that the patch 82de42fa14682d408da935adfb0f935354c5008f actually completely breaks SoCFPGA ? Then I would say this is a release blocker ? Yes. A10 SPL won't boot at all. It crashes during the clock manager setup. This came in right at the beginning of the cycle. I thought the purpose of the 3-month cycle was to allow time to test? It was ... altera ? I do plan to try out changing the behaviour to read a parent's ofdata before the child, but I am not comfortable adding such a major change just before a release. It could have any number of ill effects. Can you update the clock driver? E.g. you could move some of the code from socfpga_a10_ofdata_to_platdata() to a probe() method? Can we revert the patch which broke arria10 instead ? It did work before, so who knows how many other ill side effects there are ... No, sorry, we need to fix Altera. Other boards have fixed driver bugs exposed by the patch. BTW what is a good Altera board to get that doesn't cost too much? A problem here might be that you'd need to have a gen5, an A10 and a stratix board to find all problems... And agilex too ... Hmm, I thought we would try and keep stratix and agilex more close than gen5/A10... I even don't have any non-gen5 boards myself here, so I cannot even test those, either. But in the end, yes, it would be a good thing to have all those boards execute basic tests at least after rc1 has been tagged. I wonder if OSADL could help out here, given they already have a broad range of boards testing linux-rt already. Or do we have a separate hardware area for this? Regards, Simon
Re: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function
Any links to one? - Simon On Thu, 2 Apr 2020 at 13:54, Marek Vasut wrote: > > On 4/2/20 9:53 PM, Simon Goldschmidt wrote: > > > > > > On 02.04.2020 21:49, Simon Glass wrote: > >> Hi Marek, > >> > >> On Thu, 2 Apr 2020 at 13:45, Marek Vasut wrote: > >>> > >>> On 4/2/20 8:50 PM, Simon Glass wrote: > Hi. > >>> > >>> Hi, > >>> > >>> [...] > >>> > >>> I suspect we could change this, so that > >>> device_ofdata_to_platdata() first calls itself on its parent. > >>> > >>> I can think of various reasons why this change might be > >>> desirable. > >> > >> I think this is how it worked before already. > > > > Well effectively, yes, because ofdata and probe were joined > > together. > >>> > Simon, do you have plan to fix this DM core issue ? > >>> > >>> I'm not sure it definitely should be changed. But I'll do a patch > >>> and > >>> see how it looks. > >> > >> Do I understand it correctly that the patch > >> 82de42fa14682d408da935adfb0f935354c5008f actually completely breaks > >> SoCFPGA ? Then I would say this is a release blocker ? > > Yes. A10 SPL won't boot at all. It crashes during the clock manager > > setup. > > This came in right at the beginning of the cycle. I thought the > purpose of the 3-month cycle was to allow time to test? > >>> > >>> It was ... altera ? > >>> > I do plan to try out changing the behaviour to read a parent's ofdata > before the child, but I am not comfortable adding such a major change > just before a release. It could have any number of ill effects. > > Can you update the clock driver? E.g. you could move some of the code > from socfpga_a10_ofdata_to_platdata() to a probe() method? > >>> > >>> Can we revert the patch which broke arria10 instead ? It did work > >>> before, so who knows how many other ill side effects there are ... > >> > >> No, sorry, we need to fix Altera. Other boards have fixed driver bugs > >> exposed by the patch. > >> > >> BTW what is a good Altera board to get that doesn't cost too much? > > > > A problem here might be that you'd need to have a gen5, an A10 and a > > stratix board to find all problems... > > And agilex too ...
Re: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function
On 4/2/20 9:53 PM, Simon Goldschmidt wrote: > > > On 02.04.2020 21:49, Simon Glass wrote: >> Hi Marek, >> >> On Thu, 2 Apr 2020 at 13:45, Marek Vasut wrote: >>> >>> On 4/2/20 8:50 PM, Simon Glass wrote: Hi. >>> >>> Hi, >>> >>> [...] >>> >>> I suspect we could change this, so that >>> device_ofdata_to_platdata() first calls itself on its parent. >>> >>> I can think of various reasons why this change might be >>> desirable. >> >> I think this is how it worked before already. > > Well effectively, yes, because ofdata and probe were joined > together. >>> Simon, do you have plan to fix this DM core issue ? >>> >>> I'm not sure it definitely should be changed. But I'll do a patch >>> and >>> see how it looks. >> >> Do I understand it correctly that the patch >> 82de42fa14682d408da935adfb0f935354c5008f actually completely breaks >> SoCFPGA ? Then I would say this is a release blocker ? > Yes. A10 SPL won't boot at all. It crashes during the clock manager > setup. This came in right at the beginning of the cycle. I thought the purpose of the 3-month cycle was to allow time to test? >>> >>> It was ... altera ? >>> I do plan to try out changing the behaviour to read a parent's ofdata before the child, but I am not comfortable adding such a major change just before a release. It could have any number of ill effects. Can you update the clock driver? E.g. you could move some of the code from socfpga_a10_ofdata_to_platdata() to a probe() method? >>> >>> Can we revert the patch which broke arria10 instead ? It did work >>> before, so who knows how many other ill side effects there are ... >> >> No, sorry, we need to fix Altera. Other boards have fixed driver bugs >> exposed by the patch. >> >> BTW what is a good Altera board to get that doesn't cost too much? > > A problem here might be that you'd need to have a gen5, an A10 and a > stratix board to find all problems... And agilex too ...
Re: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function
On 02.04.2020 21:49, Simon Glass wrote: Hi Marek, On Thu, 2 Apr 2020 at 13:45, Marek Vasut wrote: On 4/2/20 8:50 PM, Simon Glass wrote: Hi. Hi, [...] I suspect we could change this, so that device_ofdata_to_platdata() first calls itself on its parent. I can think of various reasons why this change might be desirable. I think this is how it worked before already. Well effectively, yes, because ofdata and probe were joined together. Simon, do you have plan to fix this DM core issue ? I'm not sure it definitely should be changed. But I'll do a patch and see how it looks. Do I understand it correctly that the patch 82de42fa14682d408da935adfb0f935354c5008f actually completely breaks SoCFPGA ? Then I would say this is a release blocker ? Yes. A10 SPL won't boot at all. It crashes during the clock manager setup. This came in right at the beginning of the cycle. I thought the purpose of the 3-month cycle was to allow time to test? It was ... altera ? I do plan to try out changing the behaviour to read a parent's ofdata before the child, but I am not comfortable adding such a major change just before a release. It could have any number of ill effects. Can you update the clock driver? E.g. you could move some of the code from socfpga_a10_ofdata_to_platdata() to a probe() method? Can we revert the patch which broke arria10 instead ? It did work before, so who knows how many other ill side effects there are ... No, sorry, we need to fix Altera. Other boards have fixed driver bugs exposed by the patch. BTW what is a good Altera board to get that doesn't cost too much? A problem here might be that you'd need to have a gen5, an A10 and a stratix board to find all problems... Regards, Simon Regards, Simon
Re: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function
On 4/2/20 9:49 PM, Simon Glass wrote: > Hi Marek, > > On Thu, 2 Apr 2020 at 13:45, Marek Vasut wrote: >> >> On 4/2/20 8:50 PM, Simon Glass wrote: >>> Hi. >> >> Hi, >> >> [...] >> >> I suspect we could change this, so that >> device_ofdata_to_platdata() first calls itself on its parent. >> >> I can think of various reasons why this change might be desirable. > > I think this is how it worked before already. Well effectively, yes, because ofdata and probe were joined together. >> >>> Simon, do you have plan to fix this DM core issue ? >> >> I'm not sure it definitely should be changed. But I'll do a patch and >> see how it looks. > > Do I understand it correctly that the patch > 82de42fa14682d408da935adfb0f935354c5008f actually completely breaks > SoCFPGA ? Then I would say this is a release blocker ? Yes. A10 SPL won't boot at all. It crashes during the clock manager setup. >>> >>> This came in right at the beginning of the cycle. I thought the >>> purpose of the 3-month cycle was to allow time to test? >> >> It was ... altera ? >> >>> I do plan to try out changing the behaviour to read a parent's ofdata >>> before the child, but I am not comfortable adding such a major change >>> just before a release. It could have any number of ill effects. >>> >>> Can you update the clock driver? E.g. you could move some of the code >>> from socfpga_a10_ofdata_to_platdata() to a probe() method? >> >> Can we revert the patch which broke arria10 instead ? It did work >> before, so who knows how many other ill side effects there are ... > > No, sorry, we need to fix Altera. Other boards have fixed driver bugs > exposed by the patch. How is altera broken again ? It used to work fine. > BTW what is a good Altera board to get that doesn't cost too much? With arria10 ? I think there's only one, the a10 socdk ...
Re: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function
Hi Marek, On Thu, 2 Apr 2020 at 13:45, Marek Vasut wrote: > > On 4/2/20 8:50 PM, Simon Glass wrote: > > Hi. > > Hi, > > [...] > > I suspect we could change this, so that > device_ofdata_to_platdata() first calls itself on its parent. > > I can think of various reasons why this change might be desirable. > >>> > >>> I think this is how it worked before already. > >> > >> Well effectively, yes, because ofdata and probe were joined together. > > > Simon, do you have plan to fix this DM core issue ? > > I'm not sure it definitely should be changed. But I'll do a patch and > see how it looks. > >>> > >>> Do I understand it correctly that the patch > >>> 82de42fa14682d408da935adfb0f935354c5008f actually completely breaks > >>> SoCFPGA ? Then I would say this is a release blocker ? > >> Yes. A10 SPL won't boot at all. It crashes during the clock manager setup. > > > > This came in right at the beginning of the cycle. I thought the > > purpose of the 3-month cycle was to allow time to test? > > It was ... altera ? > > > I do plan to try out changing the behaviour to read a parent's ofdata > > before the child, but I am not comfortable adding such a major change > > just before a release. It could have any number of ill effects. > > > > Can you update the clock driver? E.g. you could move some of the code > > from socfpga_a10_ofdata_to_platdata() to a probe() method? > > Can we revert the patch which broke arria10 instead ? It did work > before, so who knows how many other ill side effects there are ... No, sorry, we need to fix Altera. Other boards have fixed driver bugs exposed by the patch. BTW what is a good Altera board to get that doesn't cost too much? Regards, Simon
Re: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function
On 4/2/20 8:50 PM, Simon Glass wrote: > Hi. Hi, [...] I suspect we could change this, so that device_ofdata_to_platdata() first calls itself on its parent. I can think of various reasons why this change might be desirable. >>> >>> I think this is how it worked before already. >> >> Well effectively, yes, because ofdata and probe were joined together. > Simon, do you have plan to fix this DM core issue ? I'm not sure it definitely should be changed. But I'll do a patch and see how it looks. >>> >>> Do I understand it correctly that the patch >>> 82de42fa14682d408da935adfb0f935354c5008f actually completely breaks >>> SoCFPGA ? Then I would say this is a release blocker ? >> Yes. A10 SPL won't boot at all. It crashes during the clock manager setup. > > This came in right at the beginning of the cycle. I thought the > purpose of the 3-month cycle was to allow time to test? It was ... altera ? > I do plan to try out changing the behaviour to read a parent's ofdata > before the child, but I am not comfortable adding such a major change > just before a release. It could have any number of ill effects. > > Can you update the clock driver? E.g. you could move some of the code > from socfpga_a10_ofdata_to_platdata() to a probe() method? Can we revert the patch which broke arria10 instead ? It did work before, so who knows how many other ill side effects there are ... -- Best regards, Marek Vasut
Re: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function
Hi. On Wed, 1 Apr 2020 at 20:44, Ang, Chee Hong wrote: > > > On 4/2/20 4:34 AM, Simon Glass wrote: > > > Hi, > > > > > > On Tue, 31 Mar 2020 at 20:33, Ang, Chee Hong > > wrote: > > >> > > >>> Hi Marek, > > >>> > > >>> On Wed, 11 Mar 2020 at 05:55, Marek Vasut wrote: > > > > On 3/11/20 12:50 PM, Simon Glass wrote: > > > Hi, > > > > Hi, > > > > > On Mon, 9 Mar 2020 at 02:22, wrote: > > >> > > >> From: Chee Hong Ang > > >> > > >> This commit (82de42fa14682d408da935adfb0f935354c5008f) calls > > >> child's > > >> ofdata_to_platdata() method before the parent is probed in dm core. > > >> This has caused the driver no longer able to get the correct > > >> parent clock's register base in the ofdata_to_platdata() method > > >> because the parent clocks will only be probed after the child's > > ofdata_to_platdata(). > > >> To resolve this, the clock parent's register base will only be > > >> retrieved by the child in probe() method instead of > > >> ofdata_to_platdata(). > > > > > > I think one thing that is going on here is that DM allows ofdata > > > to be read for a device before its parent devices have been read, > > > but it requires that parent devices be probed before their children. > > > > This seems wrong. The clock driver should be able to instantiate > > devices and read their ofdata without probing them. That is one of > > the core design principles of the DM. > > >>> > > >>> That's a different question. Yes you can read ofdata without probing a > > device. > > >>> That's why we have two methods. > > >>> > > >>> The point I am making is that at present there is no requirement > > >>> that a parent's ofdata be read before a child's ofdata is read. But > > >>> there is a requirement that a parent be probed before a child is probed. > > >>> > > > > > The idea is that it should be possible to read the ofdata for a > > > node without needing to have done so for parents. But perhaps this > > > assumption is too brave? > > > > Why is it brave ? That's how it always was, the DT is already > > there, so why wouldn't you be able to read it. > > >>> > > >>> That was my thinking too. But we are finding in a few situations > > >>> that the child's ofdata depends on the parent's. For example, the > > >>> parent may have a base address, or a range mapping, or something > > >>> else that is needed for the child to correctly get its base address, > > >>> etc. > > >>> > > > > > I suspect we could change this, so that > > > device_ofdata_to_platdata() first calls itself on its parent. > > > > > > I can think of various reasons why this change might be desirable. > > > > I think this is how it worked before already. > > >>> > > >>> Well effectively, yes, because ofdata and probe were joined together. > > > > > >> Simon, do you have plan to fix this DM core issue ? > > > > > > I'm not sure it definitely should be changed. But I'll do a patch and > > > see how it looks. > > > > Do I understand it correctly that the patch > > 82de42fa14682d408da935adfb0f935354c5008f actually completely breaks > > SoCFPGA ? Then I would say this is a release blocker ? > Yes. A10 SPL won't boot at all. It crashes during the clock manager setup. This came in right at the beginning of the cycle. I thought the purpose of the 3-month cycle was to allow time to test? I do plan to try out changing the behaviour to read a parent's ofdata before the child, but I am not comfortable adding such a major change just before a release. It could have any number of ill effects. Can you update the clock driver? E.g. you could move some of the code from socfpga_a10_ofdata_to_platdata() to a probe() method? Regards, Simon
RE: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function
> On 4/2/20 4:34 AM, Simon Glass wrote: > > Hi, > > > > On Tue, 31 Mar 2020 at 20:33, Ang, Chee Hong > wrote: > >> > >>> Hi Marek, > >>> > >>> On Wed, 11 Mar 2020 at 05:55, Marek Vasut wrote: > > On 3/11/20 12:50 PM, Simon Glass wrote: > > Hi, > > Hi, > > > On Mon, 9 Mar 2020 at 02:22, wrote: > >> > >> From: Chee Hong Ang > >> > >> This commit (82de42fa14682d408da935adfb0f935354c5008f) calls > >> child's > >> ofdata_to_platdata() method before the parent is probed in dm core. > >> This has caused the driver no longer able to get the correct > >> parent clock's register base in the ofdata_to_platdata() method > >> because the parent clocks will only be probed after the child's > ofdata_to_platdata(). > >> To resolve this, the clock parent's register base will only be > >> retrieved by the child in probe() method instead of > >> ofdata_to_platdata(). > > > > I think one thing that is going on here is that DM allows ofdata > > to be read for a device before its parent devices have been read, > > but it requires that parent devices be probed before their children. > > This seems wrong. The clock driver should be able to instantiate > devices and read their ofdata without probing them. That is one of > the core design principles of the DM. > >>> > >>> That's a different question. Yes you can read ofdata without probing a > device. > >>> That's why we have two methods. > >>> > >>> The point I am making is that at present there is no requirement > >>> that a parent's ofdata be read before a child's ofdata is read. But > >>> there is a requirement that a parent be probed before a child is probed. > >>> > > > The idea is that it should be possible to read the ofdata for a > > node without needing to have done so for parents. But perhaps this > > assumption is too brave? > > Why is it brave ? That's how it always was, the DT is already > there, so why wouldn't you be able to read it. > >>> > >>> That was my thinking too. But we are finding in a few situations > >>> that the child's ofdata depends on the parent's. For example, the > >>> parent may have a base address, or a range mapping, or something > >>> else that is needed for the child to correctly get its base address, etc. > >>> > > > I suspect we could change this, so that > > device_ofdata_to_platdata() first calls itself on its parent. > > > > I can think of various reasons why this change might be desirable. > > I think this is how it worked before already. > >>> > >>> Well effectively, yes, because ofdata and probe were joined together. > > > >> Simon, do you have plan to fix this DM core issue ? > > > > I'm not sure it definitely should be changed. But I'll do a patch and > > see how it looks. > > Do I understand it correctly that the patch > 82de42fa14682d408da935adfb0f935354c5008f actually completely breaks > SoCFPGA ? Then I would say this is a release blocker ? Yes. A10 SPL won't boot at all. It crashes during the clock manager setup.
Re: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function
On 4/2/20 4:34 AM, Simon Glass wrote: > Hi, > > On Tue, 31 Mar 2020 at 20:33, Ang, Chee Hong wrote: >> >>> Hi Marek, >>> >>> On Wed, 11 Mar 2020 at 05:55, Marek Vasut wrote: On 3/11/20 12:50 PM, Simon Glass wrote: > Hi, Hi, > On Mon, 9 Mar 2020 at 02:22, wrote: >> >> From: Chee Hong Ang >> >> This commit (82de42fa14682d408da935adfb0f935354c5008f) calls >> child's >> ofdata_to_platdata() method before the parent is probed in dm core. >> This has caused the driver no longer able to get the correct parent >> clock's register base in the ofdata_to_platdata() method because >> the parent clocks will only be probed after the child's >> ofdata_to_platdata(). >> To resolve this, the clock parent's register base will only be >> retrieved by the child in probe() method instead of ofdata_to_platdata(). > > I think one thing that is going on here is that DM allows ofdata to > be read for a device before its parent devices have been read, but > it requires that parent devices be probed before their children. This seems wrong. The clock driver should be able to instantiate devices and read their ofdata without probing them. That is one of the core design principles of the DM. >>> >>> That's a different question. Yes you can read ofdata without probing a >>> device. >>> That's why we have two methods. >>> >>> The point I am making is that at present there is no requirement that a >>> parent's >>> ofdata be read before a child's ofdata is read. But there is a requirement >>> that a >>> parent be probed before a child is probed. >>> > The idea is that it should be possible to read the ofdata for a node > without needing to have done so for parents. But perhaps this > assumption is too brave? Why is it brave ? That's how it always was, the DT is already there, so why wouldn't you be able to read it. >>> >>> That was my thinking too. But we are finding in a few situations that the >>> child's >>> ofdata depends on the parent's. For example, the parent may have a base >>> address, or a range mapping, or something else that is needed for the child >>> to >>> correctly get its base address, etc. >>> > I suspect we could change this, so that device_ofdata_to_platdata() > first calls itself on its parent. > > I can think of various reasons why this change might be desirable. I think this is how it worked before already. >>> >>> Well effectively, yes, because ofdata and probe were joined together. > >> Simon, do you have plan to fix this DM core issue ? > > I'm not sure it definitely should be changed. But I'll do a patch and > see how it looks. Do I understand it correctly that the patch 82de42fa14682d408da935adfb0f935354c5008f actually completely breaks SoCFPGA ? Then I would say this is a release blocker ?
Re: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function
Hi, On Tue, 31 Mar 2020 at 20:33, Ang, Chee Hong wrote: > > > Hi Marek, > > > > On Wed, 11 Mar 2020 at 05:55, Marek Vasut wrote: > > > > > > On 3/11/20 12:50 PM, Simon Glass wrote: > > > > Hi, > > > > > > Hi, > > > > > > > On Mon, 9 Mar 2020 at 02:22, wrote: > > > >> > > > >> From: Chee Hong Ang > > > >> > > > >> This commit (82de42fa14682d408da935adfb0f935354c5008f) calls > > > >> child's > > > >> ofdata_to_platdata() method before the parent is probed in dm core. > > > >> This has caused the driver no longer able to get the correct parent > > > >> clock's register base in the ofdata_to_platdata() method because > > > >> the parent clocks will only be probed after the child's > > > >> ofdata_to_platdata(). > > > >> To resolve this, the clock parent's register base will only be > > > >> retrieved by the child in probe() method instead of > > > >> ofdata_to_platdata(). > > > > > > > > I think one thing that is going on here is that DM allows ofdata to > > > > be read for a device before its parent devices have been read, but > > > > it requires that parent devices be probed before their children. > > > > > > This seems wrong. The clock driver should be able to instantiate > > > devices and read their ofdata without probing them. That is one of the > > > core design principles of the DM. > > > > That's a different question. Yes you can read ofdata without probing a > > device. > > That's why we have two methods. > > > > The point I am making is that at present there is no requirement that a > > parent's > > ofdata be read before a child's ofdata is read. But there is a requirement > > that a > > parent be probed before a child is probed. > > > > > > > > > The idea is that it should be possible to read the ofdata for a node > > > > without needing to have done so for parents. But perhaps this > > > > assumption is too brave? > > > > > > Why is it brave ? That's how it always was, the DT is already there, > > > so why wouldn't you be able to read it. > > > > That was my thinking too. But we are finding in a few situations that the > > child's > > ofdata depends on the parent's. For example, the parent may have a base > > address, or a range mapping, or something else that is needed for the child > > to > > correctly get its base address, etc. > > > > > > > > > I suspect we could change this, so that device_ofdata_to_platdata() > > > > first calls itself on its parent. > > > > > > > > I can think of various reasons why this change might be desirable. > > > > > > I think this is how it worked before already. > > > > Well effectively, yes, because ofdata and probe were joined together. > Simon, do you have plan to fix this DM core issue ? I'm not sure it definitely should be changed. But I'll do a patch and see how it looks. Regards, Simon
RE: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function
> Hi Marek, > > On Wed, 11 Mar 2020 at 05:55, Marek Vasut wrote: > > > > On 3/11/20 12:50 PM, Simon Glass wrote: > > > Hi, > > > > Hi, > > > > > On Mon, 9 Mar 2020 at 02:22, wrote: > > >> > > >> From: Chee Hong Ang > > >> > > >> This commit (82de42fa14682d408da935adfb0f935354c5008f) calls > > >> child's > > >> ofdata_to_platdata() method before the parent is probed in dm core. > > >> This has caused the driver no longer able to get the correct parent > > >> clock's register base in the ofdata_to_platdata() method because > > >> the parent clocks will only be probed after the child's > > >> ofdata_to_platdata(). > > >> To resolve this, the clock parent's register base will only be > > >> retrieved by the child in probe() method instead of ofdata_to_platdata(). > > > > > > I think one thing that is going on here is that DM allows ofdata to > > > be read for a device before its parent devices have been read, but > > > it requires that parent devices be probed before their children. > > > > This seems wrong. The clock driver should be able to instantiate > > devices and read their ofdata without probing them. That is one of the > > core design principles of the DM. > > That's a different question. Yes you can read ofdata without probing a device. > That's why we have two methods. > > The point I am making is that at present there is no requirement that a > parent's > ofdata be read before a child's ofdata is read. But there is a requirement > that a > parent be probed before a child is probed. > > > > > > The idea is that it should be possible to read the ofdata for a node > > > without needing to have done so for parents. But perhaps this > > > assumption is too brave? > > > > Why is it brave ? That's how it always was, the DT is already there, > > so why wouldn't you be able to read it. > > That was my thinking too. But we are finding in a few situations that the > child's > ofdata depends on the parent's. For example, the parent may have a base > address, or a range mapping, or something else that is needed for the child to > correctly get its base address, etc. > > > > > > I suspect we could change this, so that device_ofdata_to_platdata() > > > first calls itself on its parent. > > > > > > I can think of various reasons why this change might be desirable. > > > > I think this is how it worked before already. > > Well effectively, yes, because ofdata and probe were joined together. > > Regards, > Simon Simon, do you have plan to fix this DM core issue ?
Re: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function
Hi Marek, On Wed, 11 Mar 2020 at 05:55, Marek Vasut wrote: > > On 3/11/20 12:50 PM, Simon Glass wrote: > > Hi, > > Hi, > > > On Mon, 9 Mar 2020 at 02:22, wrote: > >> > >> From: Chee Hong Ang > >> > >> This commit (82de42fa14682d408da935adfb0f935354c5008f) calls child's > >> ofdata_to_platdata() method before the parent is probed in dm core. > >> This has caused the driver no longer able to get the correct parent > >> clock's register base in the ofdata_to_platdata() method because the > >> parent clocks will only be probed after the child's ofdata_to_platdata(). > >> To resolve this, the clock parent's register base will only be retrieved > >> by the child in probe() method instead of ofdata_to_platdata(). > > > > I think one thing that is going on here is that DM allows ofdata to be > > read for a device before its parent devices have been read, but it > > requires that parent devices be probed before their children. > > This seems wrong. The clock driver should be able to instantiate devices > and read their ofdata without probing them. That is one of the core > design principles of the DM. That's a different question. Yes you can read ofdata without probing a device. That's why we have two methods. The point I am making is that at present there is no requirement that a parent's ofdata be read before a child's ofdata is read. But there is a requirement that a parent be probed before a child is probed. > > > The idea is that it should be possible to read the ofdata for a node > > without needing to have done so for parents. But perhaps this > > assumption is too brave? > > Why is it brave ? That's how it always was, the DT is already there, so > why wouldn't you be able to read it. That was my thinking too. But we are finding in a few situations that the child's ofdata depends on the parent's. For example, the parent may have a base address, or a range mapping, or something else that is needed for the child to correctly get its base address, etc. > > > I suspect we could change this, so that device_ofdata_to_platdata() > > first calls itself on its parent. > > > > I can think of various reasons why this change might be desirable. > > I think this is how it worked before already. Well effectively, yes, because ofdata and probe were joined together. Regards, Simon
Re: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function
On 3/11/20 12:50 PM, Simon Glass wrote: > Hi, Hi, > On Mon, 9 Mar 2020 at 02:22, wrote: >> >> From: Chee Hong Ang >> >> This commit (82de42fa14682d408da935adfb0f935354c5008f) calls child's >> ofdata_to_platdata() method before the parent is probed in dm core. >> This has caused the driver no longer able to get the correct parent >> clock's register base in the ofdata_to_platdata() method because the >> parent clocks will only be probed after the child's ofdata_to_platdata(). >> To resolve this, the clock parent's register base will only be retrieved >> by the child in probe() method instead of ofdata_to_platdata(). > > I think one thing that is going on here is that DM allows ofdata to be > read for a device before its parent devices have been read, but it > requires that parent devices be probed before their children. This seems wrong. The clock driver should be able to instantiate devices and read their ofdata without probing them. That is one of the core design principles of the DM. > The idea is that it should be possible to read the ofdata for a node > without needing to have done so for parents. But perhaps this > assumption is too brave? Why is it brave ? That's how it always was, the DT is already there, so why wouldn't you be able to read it. > I suspect we could change this, so that device_ofdata_to_platdata() > first calls itself on its parent. > > I can think of various reasons why this change might be desirable. I think this is how it worked before already. -- Best regards, Marek Vasut
Re: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function
Hi,
On Mon, 9 Mar 2020 at 02:22, wrote:
>
> From: Chee Hong Ang
>
> This commit (82de42fa14682d408da935adfb0f935354c5008f) calls child's
> ofdata_to_platdata() method before the parent is probed in dm core.
> This has caused the driver no longer able to get the correct parent
> clock's register base in the ofdata_to_platdata() method because the
> parent clocks will only be probed after the child's ofdata_to_platdata().
> To resolve this, the clock parent's register base will only be retrieved
> by the child in probe() method instead of ofdata_to_platdata().
I think one thing that is going on here is that DM allows ofdata to be
read for a device before its parent devices have been read, but it
requires that parent devices be probed before their children.
The idea is that it should be possible to read the ofdata for a node
without needing to have done so for parents. But perhaps this
assumption is too brave?
I suspect we could change this, so that device_ofdata_to_platdata()
first calls itself on its parent.
I can think of various reasons why this change might be desirable.
>
> Signed-off-by: Chee Hong Ang
> ---
> drivers/clk/altera/clk-arria10.c | 40
> ++--
> 1 file changed, 18 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/clk/altera/clk-arria10.c
> b/drivers/clk/altera/clk-arria10.c
> index affeb31..b7eed94 100644
> --- a/drivers/clk/altera/clk-arria10.c
> +++ b/drivers/clk/altera/clk-arria10.c
> @@ -274,6 +274,8 @@ static int socfpga_a10_clk_bind(struct udevice *dev)
> static int socfpga_a10_clk_probe(struct udevice *dev)
> {
> struct socfpga_a10_clk_platdata *plat = dev_get_platdata(dev);
> + struct socfpga_a10_clk_platdata *pplat;
> + struct udevice *pdev;
> const void *fdt = gd->fdt_blob;
> int offset = dev_of_offset(dev);
>
> @@ -281,6 +283,21 @@ static int socfpga_a10_clk_probe(struct udevice *dev)
>
> socfpga_a10_handoff_workaround(dev);
>
> + if (!fdt_node_check_compatible(fdt, offset, "altr,clk-mgr")) {
This is strange to me. I think the idea here is to detect whether this
is the root clk node or one of the ones created in the bind() function
above. Is that right?
If so, it should be enough to say:
if (dev_ofvalid(dev))
If you actually need to distinguish between different compatible
strings, you can use the .data member in your udevice_id table, with
dev_get_driver_data().
> + plat->regs = devfdt_get_addr(dev);
> + } else {
> + pdev = dev_get_parent(dev);
> + if (!pdev)
> + return -ENODEV;
> +
> + pplat = dev_get_platdata(pdev);
> + if (!pplat)
> + return -EINVAL;
> +
> + plat->ctl_reg = dev_read_u32_default(dev, "reg", 0x0);
> + plat->regs = pplat->regs;
> + }
> +
> if (!fdt_node_check_compatible(fdt, offset,
>"altr,socfpga-a10-pll-clock")) {
> /* Main PLL has 3 upstream clock */
> @@ -304,29 +321,8 @@ static int socfpga_a10_clk_probe(struct udevice *dev)
> static int socfpga_a10_ofdata_to_platdata(struct udevice *dev)
> {
> struct socfpga_a10_clk_platdata *plat = dev_get_platdata(dev);
> - struct socfpga_a10_clk_platdata *pplat;
> - struct udevice *pdev;
> - const void *fdt = gd->fdt_blob;
> unsigned int divreg[3], gatereg[2];
> - int ret, offset = dev_of_offset(dev);
> - u32 regs;
> -
> - regs = dev_read_u32_default(dev, "reg", 0x0);
> -
> - if (!fdt_node_check_compatible(fdt, offset, "altr,clk-mgr")) {
> - plat->regs = devfdt_get_addr(dev);
> - } else {
> - pdev = dev_get_parent(dev);
> - if (!pdev)
> - return -ENODEV;
> -
> - pplat = dev_get_platdata(pdev);
> - if (!pplat)
> - return -EINVAL;
> -
> - plat->ctl_reg = regs;
> - plat->regs = pplat->regs;
> - }
> + int ret;
>
> plat->type = SOCFPGA_A10_CLK_UNKNOWN_CLK;
>
> --
> 2.7.4
>
Regards,
Simon
Re: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function
On 3/9/20 1:52 PM, Ang, Chee Hong wrote: >> On 3/9/20 9:21 AM, chee.hong@intel.com wrote: >>> From: Chee Hong Ang >>> >>> This commit (82de42fa14682d408da935adfb0f935354c5008f) calls child's >>> ofdata_to_platdata() method before the parent is probed in dm core. >>> This has caused the driver no longer able to get the correct parent >>> clock's register base in the ofdata_to_platdata() method because the >>> parent clocks will only be probed after the child's ofdata_to_platdata(). >>> To resolve this, the clock parent's register base will only be >>> retrieved by the child in probe() method instead of ofdata_to_platdata(). >> >> You should be able to bind the drivers and resolve their register offsets >> without >> probing them, so this look more like a bug in the driver core ? > The problem is the children clock driver need to resolve/derive their > register base > from their clock parents. With this new change, clock parents are still not > yet > being initialized when the children clock drivers need to resolve their > register base > from their parent. > A10 is not booting in mainline due to this issue. > I can't think of a better way to fix this. Should we fix the clock driver > itself or the > DM core ? It seems more like a bug in the later, since the register offsets are in the DT and reading out the DT information should be possible before .probe() is called for any of the clock.
RE: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function
> On 3/9/20 9:21 AM, chee.hong@intel.com wrote:
> > From: Chee Hong Ang
> >
> > This commit (82de42fa14682d408da935adfb0f935354c5008f) calls child's
> > ofdata_to_platdata() method before the parent is probed in dm core.
> > This has caused the driver no longer able to get the correct parent
> > clock's register base in the ofdata_to_platdata() method because the
> > parent clocks will only be probed after the child's ofdata_to_platdata().
> > To resolve this, the clock parent's register base will only be
> > retrieved by the child in probe() method instead of ofdata_to_platdata().
>
> You should be able to bind the drivers and resolve their register offsets
> without
> probing them, so this look more like a bug in the driver core ?
The problem is the children clock driver need to resolve/derive their register
base
from their clock parents. With this new change, clock parents are still not yet
being initialized when the children clock drivers need to resolve their
register base
from their parent.
A10 is not booting in mainline due to this issue.
I can't think of a better way to fix this. Should we fix the clock driver
itself or the
DM core ?
>
> > Signed-off-by: Chee Hong Ang
> > ---
> > drivers/clk/altera/clk-arria10.c | 40
> > ++--
> > 1 file changed, 18 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/clk/altera/clk-arria10.c
> > b/drivers/clk/altera/clk-arria10.c
> > index affeb31..b7eed94 100644
> > --- a/drivers/clk/altera/clk-arria10.c
> > +++ b/drivers/clk/altera/clk-arria10.c
> > @@ -274,6 +274,8 @@ static int socfpga_a10_clk_bind(struct udevice
> > *dev) static int socfpga_a10_clk_probe(struct udevice *dev) {
> > struct socfpga_a10_clk_platdata *plat = dev_get_platdata(dev);
> > + struct socfpga_a10_clk_platdata *pplat;
> > + struct udevice *pdev;
> > const void *fdt = gd->fdt_blob;
> > int offset = dev_of_offset(dev);
> >
> > @@ -281,6 +283,21 @@ static int socfpga_a10_clk_probe(struct udevice
> > *dev)
> >
> > socfpga_a10_handoff_workaround(dev);
> >
> > + if (!fdt_node_check_compatible(fdt, offset, "altr,clk-mgr")) {
> > + plat->regs = devfdt_get_addr(dev);
> > + } else {
> > + pdev = dev_get_parent(dev);
> > + if (!pdev)
> > + return -ENODEV;
> > +
> > + pplat = dev_get_platdata(pdev);
> > + if (!pplat)
> > + return -EINVAL;
> > +
> > + plat->ctl_reg = dev_read_u32_default(dev, "reg", 0x0);
> > + plat->regs = pplat->regs;
> > + }
> > +
> > if (!fdt_node_check_compatible(fdt, offset,
> >"altr,socfpga-a10-pll-clock")) {
> > /* Main PLL has 3 upstream clock */ @@ -304,29 +321,8 @@
> static int
> > socfpga_a10_clk_probe(struct udevice *dev) static int
> > socfpga_a10_ofdata_to_platdata(struct udevice *dev) {
> > struct socfpga_a10_clk_platdata *plat = dev_get_platdata(dev);
> > - struct socfpga_a10_clk_platdata *pplat;
> > - struct udevice *pdev;
> > - const void *fdt = gd->fdt_blob;
> > unsigned int divreg[3], gatereg[2];
> > - int ret, offset = dev_of_offset(dev);
> > - u32 regs;
> > -
> > - regs = dev_read_u32_default(dev, "reg", 0x0);
> > -
> > - if (!fdt_node_check_compatible(fdt, offset, "altr,clk-mgr")) {
> > - plat->regs = devfdt_get_addr(dev);
> > - } else {
> > - pdev = dev_get_parent(dev);
> > - if (!pdev)
> > - return -ENODEV;
> > -
> > - pplat = dev_get_platdata(pdev);
> > - if (!pplat)
> > - return -EINVAL;
> > -
> > - plat->ctl_reg = regs;
> > - plat->regs = pplat->regs;
> > - }
> > + int ret;
> >
> > plat->type = SOCFPGA_A10_CLK_UNKNOWN_CLK;
> >
> >
>
>
> --
> Best regards,
> Marek Vasut
Re: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function
On 3/9/20 9:21 AM, chee.hong@intel.com wrote:
> From: Chee Hong Ang
>
> This commit (82de42fa14682d408da935adfb0f935354c5008f) calls child's
> ofdata_to_platdata() method before the parent is probed in dm core.
> This has caused the driver no longer able to get the correct parent
> clock's register base in the ofdata_to_platdata() method because the
> parent clocks will only be probed after the child's ofdata_to_platdata().
> To resolve this, the clock parent's register base will only be retrieved
> by the child in probe() method instead of ofdata_to_platdata().
You should be able to bind the drivers and resolve their register
offsets without probing them, so this look more like a bug in the driver
core ?
> Signed-off-by: Chee Hong Ang
> ---
> drivers/clk/altera/clk-arria10.c | 40
> ++--
> 1 file changed, 18 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/clk/altera/clk-arria10.c
> b/drivers/clk/altera/clk-arria10.c
> index affeb31..b7eed94 100644
> --- a/drivers/clk/altera/clk-arria10.c
> +++ b/drivers/clk/altera/clk-arria10.c
> @@ -274,6 +274,8 @@ static int socfpga_a10_clk_bind(struct udevice *dev)
> static int socfpga_a10_clk_probe(struct udevice *dev)
> {
> struct socfpga_a10_clk_platdata *plat = dev_get_platdata(dev);
> + struct socfpga_a10_clk_platdata *pplat;
> + struct udevice *pdev;
> const void *fdt = gd->fdt_blob;
> int offset = dev_of_offset(dev);
>
> @@ -281,6 +283,21 @@ static int socfpga_a10_clk_probe(struct udevice *dev)
>
> socfpga_a10_handoff_workaround(dev);
>
> + if (!fdt_node_check_compatible(fdt, offset, "altr,clk-mgr")) {
> + plat->regs = devfdt_get_addr(dev);
> + } else {
> + pdev = dev_get_parent(dev);
> + if (!pdev)
> + return -ENODEV;
> +
> + pplat = dev_get_platdata(pdev);
> + if (!pplat)
> + return -EINVAL;
> +
> + plat->ctl_reg = dev_read_u32_default(dev, "reg", 0x0);
> + plat->regs = pplat->regs;
> + }
> +
> if (!fdt_node_check_compatible(fdt, offset,
> "altr,socfpga-a10-pll-clock")) {
> /* Main PLL has 3 upstream clock */
> @@ -304,29 +321,8 @@ static int socfpga_a10_clk_probe(struct udevice *dev)
> static int socfpga_a10_ofdata_to_platdata(struct udevice *dev)
> {
> struct socfpga_a10_clk_platdata *plat = dev_get_platdata(dev);
> - struct socfpga_a10_clk_platdata *pplat;
> - struct udevice *pdev;
> - const void *fdt = gd->fdt_blob;
> unsigned int divreg[3], gatereg[2];
> - int ret, offset = dev_of_offset(dev);
> - u32 regs;
> -
> - regs = dev_read_u32_default(dev, "reg", 0x0);
> -
> - if (!fdt_node_check_compatible(fdt, offset, "altr,clk-mgr")) {
> - plat->regs = devfdt_get_addr(dev);
> - } else {
> - pdev = dev_get_parent(dev);
> - if (!pdev)
> - return -ENODEV;
> -
> - pplat = dev_get_platdata(pdev);
> - if (!pplat)
> - return -EINVAL;
> -
> - plat->ctl_reg = regs;
> - plat->regs = pplat->regs;
> - }
> + int ret;
>
> plat->type = SOCFPGA_A10_CLK_UNKNOWN_CLK;
>
>
--
Best regards,
Marek Vasut
RE: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function
> -Original Message- > From: Ang, Chee Hong > Sent: Monday, March 9, 2020 4:22 PM > To: u-boot@lists.denx.de > Cc: Marek Vasut ; Simon Goldschmidt > ; See, Chin Liang > ; Tan, Ley Foon ; > Westergreen, Dalon ; Ang, Chee Hong > > Subject: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in > probe function > > From: Chee Hong Ang > > This commit (82de42fa14682d408da935adfb0f935354c5008f) calls child's > ofdata_to_platdata() method before the parent is probed in dm core. > This has caused the driver no longer able to get the correct parent clock's > register base in the ofdata_to_platdata() method because the parent clocks > will only be probed after the child's ofdata_to_platdata(). > To resolve this, the clock parent's register base will only be retrieved by > the > child in probe() method instead of ofdata_to_platdata(). > > Signed-off-by: Chee Hong Ang Reviewed-by: Ley Foon Tan
