Greetings,
I have developed an AM3354 based SoM and it uses an external SI5351 clock
generator to drive the clock inputs for an external duart and I2S audio
master clock. With the registration according to the documentation the
reference clock is not being detected and hence the clock generator is
not working as expect.
After trying many different things, I started to look around the mailing
lists to find information related to this issue.
I came acrossed post that has the exact same issue:
https://lkml.org/lkml/2013/2/18/468
Seeing as the patch did not land upstream, I am wondering if there is
a solution that I am not seeing.
I am willing to provide a patch given appropriate guidance.
Regards,
Michael
On 04/15/2015 12:17 AM, Michael Welling wrote:
> Greetings,
>
> I have developed an AM3354 based SoM and it uses an external SI5351 clock
> generator to drive the clock inputs for an external duart and I2S audio
> master clock. With the registration according to the documentation the
> reference clock is not being detected and hence the clock generator is
> not working as expect.
>
> After trying many different things, I started to look around the mailing
> lists to find information related to this issue.
>
> I came acrossed post that has the exact same issue:
> https://lkml.org/lkml/2013/2/18/468
>
> Seeing as the patch did not land upstream, I am wondering if there is
> a solution that I am not seeing.
>
> I am willing to provide a patch given appropriate guidance.
Hi Michael,
The info on the email you referenced is kind of obsolete, TI SoCs are
calling of_clk_init() during boot now, and thus external clock nodes
should be registered fine also. Maybe you can provide the actual DTS
patch you are trying out so we can help better...? Are you seeing any
boot time error / warning prints for your new clock?
-Tero
On Wed, Apr 15, 2015 at 09:34:48AM +0300, Tero Kristo wrote:
> On 04/15/2015 12:17 AM, Michael Welling wrote:
> >Greetings,
> >
> >I have developed an AM3354 based SoM and it uses an external SI5351 clock
> >generator to drive the clock inputs for an external duart and I2S audio
> >master clock. With the registration according to the documentation the
> >reference clock is not being detected and hence the clock generator is
> >not working as expect.
> >
> >After trying many different things, I started to look around the mailing
> >lists to find information related to this issue.
> >
> >I came acrossed post that has the exact same issue:
> >https://lkml.org/lkml/2013/2/18/468
> >
> >Seeing as the patch did not land upstream, I am wondering if there is
> >a solution that I am not seeing.
> >
> >I am willing to provide a patch given appropriate guidance.
>
> Hi Michael,
>
> The info on the email you referenced is kind of obsolete, TI SoCs
> are calling of_clk_init() during boot now, and thus external clock
> nodes should be registered fine also. Maybe you can provide the
> actual DTS patch you are trying out so we can help better...? Are
See attached patch and console output.
> you seeing any boot time error / warning prints for your new clock?
With the debug messages on you see that the reference clock is not being
detected.
Whilest debugging I found that the of_clk_get is returning an error no matter
which clock I pass it:
http://lxr.free-electrons.com/source/drivers/clk/clk-si5351.c#L1131
>
> -Tero
On 04/15/2015 05:09 PM, Michael Welling wrote:
> On Wed, Apr 15, 2015 at 09:34:48AM +0300, Tero Kristo wrote:
>> On 04/15/2015 12:17 AM, Michael Welling wrote:
>>> Greetings,
>>>
>>> I have developed an AM3354 based SoM and it uses an external SI5351 clock
>>> generator to drive the clock inputs for an external duart and I2S audio
>>> master clock. With the registration according to the documentation the
>>> reference clock is not being detected and hence the clock generator is
>>> not working as expect.
>>>
>>> After trying many different things, I started to look around the mailing
>>> lists to find information related to this issue.
>>>
>>> I came acrossed post that has the exact same issue:
>>> https://lkml.org/lkml/2013/2/18/468
>>>
>>> Seeing as the patch did not land upstream, I am wondering if there is
>>> a solution that I am not seeing.
>>>
>>> I am willing to provide a patch given appropriate guidance.
>>
>> Hi Michael,
>>
>> The info on the email you referenced is kind of obsolete, TI SoCs
>> are calling of_clk_init() during boot now, and thus external clock
>> nodes should be registered fine also. Maybe you can provide the
>> actual DTS patch you are trying out so we can help better...? Are
>
> See attached patch and console output.
I see a bug in your dt data.
<snip>
+ clocks {
+ ref27: ref27 {
+ #clock-cells = <0>;
+ compatibale = "fixed-clock";
This should be compatible, right? DT is annoying in that it doesn't
verify property names.
+ clock-frequency = <27000000>;
+ };
+ };
-Tero
>
>> you seeing any boot time error / warning prints for your new clock?
>
> With the debug messages on you see that the reference clock is not being
> detected.
>
> Whilest debugging I found that the of_clk_get is returning an error no matter
> which clock I pass it:
> http://lxr.free-electrons.com/source/drivers/clk/clk-si5351.c#L1131
>
>>
>> -Tero
On Wed, Apr 15, 2015 at 09:43:30PM +0300, Tero Kristo wrote:
> On 04/15/2015 05:09 PM, Michael Welling wrote:
> >On Wed, Apr 15, 2015 at 09:34:48AM +0300, Tero Kristo wrote:
> >>On 04/15/2015 12:17 AM, Michael Welling wrote:
> >>>Greetings,
> >>>
> >>>I have developed an AM3354 based SoM and it uses an external SI5351 clock
> >>>generator to drive the clock inputs for an external duart and I2S audio
> >>>master clock. With the registration according to the documentation the
> >>>reference clock is not being detected and hence the clock generator is
> >>>not working as expect.
> >>>
> >>>After trying many different things, I started to look around the mailing
> >>>lists to find information related to this issue.
> >>>
> >>>I came acrossed post that has the exact same issue:
> >>>https://lkml.org/lkml/2013/2/18/468
> >>>
> >>>Seeing as the patch did not land upstream, I am wondering if there is
> >>>a solution that I am not seeing.
> >>>
> >>>I am willing to provide a patch given appropriate guidance.
> >>
> >>Hi Michael,
> >>
> >>The info on the email you referenced is kind of obsolete, TI SoCs
> >>are calling of_clk_init() during boot now, and thus external clock
> >>nodes should be registered fine also. Maybe you can provide the
> >>actual DTS patch you are trying out so we can help better...? Are
> >
> >See attached patch and console output.
>
> I see a bug in your dt data.
>
> <snip>
>
> + clocks {
> + ref27: ref27 {
> + #clock-cells = <0>;
> + compatibale = "fixed-clock";
>
> This should be compatible, right? DT is annoying in that it doesn't
> verify property names.
>
Ooops.
Now the clock appears in /sys/kernel/debug/clk:
root@som3517-som200:/sys/kernel/debug/clk# cat clk_summary
clock enable_cnt prepare_cnt rate accuracy phase
----------------------------------------------------------------------------------------
ref27 0 0 27000000 0 0
...
There is still an issue with the si5351.
I had to comment out the clk_put here for the frequency to show up:
http://lxr.free-electrons.com/source/drivers/clk/clk-si5351.c#L1133
Ideas?
> + clock-frequency = <27000000>;
> + };
> + };
>
> -Tero
>
> >
> >>you seeing any boot time error / warning prints for your new clock?
> >
> >With the debug messages on you see that the reference clock is not being
> >detected.
> >
> >Whilest debugging I found that the of_clk_get is returning an error no matter
> >which clock I pass it:
> >http://lxr.free-electrons.com/source/drivers/clk/clk-si5351.c#L1131
> >
> >>
> >>-Tero
>
On Wed, Apr 15, 2015 at 12:47 PM, Michael Welling <[email protected]> wrote:
> On Wed, Apr 15, 2015 at 09:43:30PM +0300, Tero Kristo wrote:
>> On 04/15/2015 05:09 PM, Michael Welling wrote:
>> >On Wed, Apr 15, 2015 at 09:34:48AM +0300, Tero Kristo wrote:
>> >>On 04/15/2015 12:17 AM, Michael Welling wrote:
>> >>>Greetings,
>> >>>
>> >>>I have developed an AM3354 based SoM and it uses an external SI5351 clock
>> >>>generator to drive the clock inputs for an external duart and I2S audio
>> >>>master clock. With the registration according to the documentation the
>> >>>reference clock is not being detected and hence the clock generator is
>> >>>not working as expect.
>> >>>
>> >>>After trying many different things, I started to look around the mailing
>> >>>lists to find information related to this issue.
>> >>>
>> >>>I came acrossed post that has the exact same issue:
>> >>>https://lkml.org/lkml/2013/2/18/468
>> >>>
>> >>>Seeing as the patch did not land upstream, I am wondering if there is
>> >>>a solution that I am not seeing.
>> >>>
>> >>>I am willing to provide a patch given appropriate guidance.
>> >>
>> >>Hi Michael,
>> >>
>> >>The info on the email you referenced is kind of obsolete, TI SoCs
>> >>are calling of_clk_init() during boot now, and thus external clock
>> >>nodes should be registered fine also. Maybe you can provide the
>> >>actual DTS patch you are trying out so we can help better...? Are
>> >
>> >See attached patch and console output.
>>
>> I see a bug in your dt data.
>>
>> <snip>
>>
>> + clocks {
>> + ref27: ref27 {
>> + #clock-cells = <0>;
>> + compatibale = "fixed-clock";
>>
>> This should be compatible, right? DT is annoying in that it doesn't
>> verify property names.
>>
>
> Ooops.
>
> Now the clock appears in /sys/kernel/debug/clk:
> root@som3517-som200:/sys/kernel/debug/clk# cat clk_summary
> clock enable_cnt prepare_cnt rate accuracy phase
> ----------------------------------------------------------------------------------------
> ref27 0 0 27000000 0 0
> ...
>
> There is still an issue with the si5351.
>
> I had to comment out the clk_put here for the frequency to show up:
> http://lxr.free-electrons.com/source/drivers/clk/clk-si5351.c#L1133
>
> Ideas?
What is the most recent upstream commit that you are based on?
Regards,
Mike
>
>> + clock-frequency = <27000000>;
>> + };
>> + };
>>
>> -Tero
>>
>> >
>> >>you seeing any boot time error / warning prints for your new clock?
>> >
>> >With the debug messages on you see that the reference clock is not being
>> >detected.
>> >
>> >Whilest debugging I found that the of_clk_get is returning an error no matter
>> >which clock I pass it:
>> >http://lxr.free-electrons.com/source/drivers/clk/clk-si5351.c#L1131
>> >
>> >>
>> >>-Tero
>>
On Wed, Apr 15, 2015 at 01:45:53PM -0700, Mike Turquette wrote:
> On Wed, Apr 15, 2015 at 12:47 PM, Michael Welling <[email protected]> wrote:
> > On Wed, Apr 15, 2015 at 09:43:30PM +0300, Tero Kristo wrote:
> >> On 04/15/2015 05:09 PM, Michael Welling wrote:
> >> >On Wed, Apr 15, 2015 at 09:34:48AM +0300, Tero Kristo wrote:
> >> >>On 04/15/2015 12:17 AM, Michael Welling wrote:
> >> >>>Greetings,
> >> >>>
> >> >>>I have developed an AM3354 based SoM and it uses an external SI5351 clock
> >> >>>generator to drive the clock inputs for an external duart and I2S audio
> >> >>>master clock. With the registration according to the documentation the
> >> >>>reference clock is not being detected and hence the clock generator is
> >> >>>not working as expect.
> >> >>>
> >> >>>After trying many different things, I started to look around the mailing
> >> >>>lists to find information related to this issue.
> >> >>>
> >> >>>I came acrossed post that has the exact same issue:
> >> >>>https://lkml.org/lkml/2013/2/18/468
> >> >>>
> >> >>>Seeing as the patch did not land upstream, I am wondering if there is
> >> >>>a solution that I am not seeing.
> >> >>>
> >> >>>I am willing to provide a patch given appropriate guidance.
> >> >>
> >> >>Hi Michael,
> >> >>
> >> >>The info on the email you referenced is kind of obsolete, TI SoCs
> >> >>are calling of_clk_init() during boot now, and thus external clock
> >> >>nodes should be registered fine also. Maybe you can provide the
> >> >>actual DTS patch you are trying out so we can help better...? Are
> >> >
> >> >See attached patch and console output.
> >>
> >> I see a bug in your dt data.
> >>
> >> <snip>
> >>
> >> + clocks {
> >> + ref27: ref27 {
> >> + #clock-cells = <0>;
> >> + compatibale = "fixed-clock";
> >>
> >> This should be compatible, right? DT is annoying in that it doesn't
> >> verify property names.
> >>
> >
> > Ooops.
> >
> > Now the clock appears in /sys/kernel/debug/clk:
> > root@som3517-som200:/sys/kernel/debug/clk# cat clk_summary
> > clock enable_cnt prepare_cnt rate accuracy phase
> > ----------------------------------------------------------------------------------------
> > ref27 0 0 27000000 0 0
> > ...
> >
> > There is still an issue with the si5351.
> >
> > I had to comment out the clk_put here for the frequency to show up:
> > http://lxr.free-electrons.com/source/drivers/clk/clk-si5351.c#L1133
> >
> > Ideas?
>
> What is the most recent upstream commit that you are based on?
I am working from 4.0.0-rc7.
7b43b47373d40d557cd7e1a84a0bd8ebc4d745ab
>
> Regards,
> Mike
>
> >
> >> + clock-frequency = <27000000>;
> >> + };
> >> + };
> >>
> >> -Tero
> >>
> >> >
> >> >>you seeing any boot time error / warning prints for your new clock?
> >> >
> >> >With the debug messages on you see that the reference clock is not being
> >> >detected.
> >> >
> >> >Whilest debugging I found that the of_clk_get is returning an error no matter
> >> >which clock I pass it:
> >> >http://lxr.free-electrons.com/source/drivers/clk/clk-si5351.c#L1131
> >> >
> >> >>
> >> >>-Tero
> >>
On 04/15/2015 11:51 PM, Michael Welling wrote:
> On Wed, Apr 15, 2015 at 01:45:53PM -0700, Mike Turquette wrote:
>> On Wed, Apr 15, 2015 at 12:47 PM, Michael Welling <[email protected]> wrote:
>>> On Wed, Apr 15, 2015 at 09:43:30PM +0300, Tero Kristo wrote:
>>>> On 04/15/2015 05:09 PM, Michael Welling wrote:
>>>>> On Wed, Apr 15, 2015 at 09:34:48AM +0300, Tero Kristo wrote:
>>>>>> On 04/15/2015 12:17 AM, Michael Welling wrote:
>>>>>>> Greetings,
>>>>>>>
>>>>>>> I have developed an AM3354 based SoM and it uses an external SI5351 clock
>>>>>>> generator to drive the clock inputs for an external duart and I2S audio
>>>>>>> master clock. With the registration according to the documentation the
>>>>>>> reference clock is not being detected and hence the clock generator is
>>>>>>> not working as expect.
>>>>>>>
>>>>>>> After trying many different things, I started to look around the mailing
>>>>>>> lists to find information related to this issue.
>>>>>>>
>>>>>>> I came acrossed post that has the exact same issue:
>>>>>>> https://lkml.org/lkml/2013/2/18/468
>>>>>>>
>>>>>>> Seeing as the patch did not land upstream, I am wondering if there is
>>>>>>> a solution that I am not seeing.
>>>>>>>
>>>>>>> I am willing to provide a patch given appropriate guidance.
>>>>>>
>>>>>> Hi Michael,
>>>>>>
>>>>>> The info on the email you referenced is kind of obsolete, TI SoCs
>>>>>> are calling of_clk_init() during boot now, and thus external clock
>>>>>> nodes should be registered fine also. Maybe you can provide the
>>>>>> actual DTS patch you are trying out so we can help better...? Are
>>>>>
>>>>> See attached patch and console output.
>>>>
>>>> I see a bug in your dt data.
>>>>
>>>> <snip>
>>>>
>>>> + clocks {
>>>> + ref27: ref27 {
>>>> + #clock-cells = <0>;
>>>> + compatibale = "fixed-clock";
>>>>
>>>> This should be compatible, right? DT is annoying in that it doesn't
>>>> verify property names.
>>>>
>>>
>>> Ooops.
>>>
>>> Now the clock appears in /sys/kernel/debug/clk:
>>> root@som3517-som200:/sys/kernel/debug/clk# cat clk_summary
>>> clock enable_cnt prepare_cnt rate accuracy phase
>>> ----------------------------------------------------------------------------------------
>>> ref27 0 0 27000000 0 0
>>> ...
>>>
>>> There is still an issue with the si5351.
>>>
>>> I had to comment out the clk_put here for the frequency to show up:
>>> http://lxr.free-electrons.com/source/drivers/clk/clk-si5351.c#L1133
>>>
>>> Ideas?
>>
>> What is the most recent upstream commit that you are based on?
>
> I am working from 4.0.0-rc7.
>
> 7b43b47373d40d557cd7e1a84a0bd8ebc4d745ab
Hmm, I wonder why si5351 calls clk_put immediately after of_clk_get in
the first place, as far as I understand this destroys the clock handle,
which is still being used later in the code.
-Tero
>
>>
>> Regards,
>> Mike
>>
>>>
>>>> + clock-frequency = <27000000>;
>>>> + };
>>>> + };
>>>>
>>>> -Tero
>>>>
>>>>>
>>>>>> you seeing any boot time error / warning prints for your new clock?
>>>>>
>>>>> With the debug messages on you see that the reference clock is not being
>>>>> detected.
>>>>>
>>>>> Whilest debugging I found that the of_clk_get is returning an error no matter
>>>>> which clock I pass it:
>>>>> http://lxr.free-electrons.com/source/drivers/clk/clk-si5351.c#L1131
>>>>>
>>>>>>
>>>>>> -Tero
>>>>
On Thu, Apr 16, 2015 at 07:32:32AM +0300, Tero Kristo wrote:
> On 04/15/2015 11:51 PM, Michael Welling wrote:
> >On Wed, Apr 15, 2015 at 01:45:53PM -0700, Mike Turquette wrote:
> >>On Wed, Apr 15, 2015 at 12:47 PM, Michael Welling <[email protected]> wrote:
> >>>On Wed, Apr 15, 2015 at 09:43:30PM +0300, Tero Kristo wrote:
> >>>>On 04/15/2015 05:09 PM, Michael Welling wrote:
> >>>>>On Wed, Apr 15, 2015 at 09:34:48AM +0300, Tero Kristo wrote:
> >>>>>>On 04/15/2015 12:17 AM, Michael Welling wrote:
> >>>>>>>Greetings,
> >>>>>>>
> >>>>>>>I have developed an AM3354 based SoM and it uses an external SI5351 clock
> >>>>>>>generator to drive the clock inputs for an external duart and I2S audio
> >>>>>>>master clock. With the registration according to the documentation the
> >>>>>>>reference clock is not being detected and hence the clock generator is
> >>>>>>>not working as expect.
> >>>>>>>
> >>>>>>>After trying many different things, I started to look around the mailing
> >>>>>>>lists to find information related to this issue.
> >>>>>>>
> >>>>>>>I came acrossed post that has the exact same issue:
> >>>>>>>https://lkml.org/lkml/2013/2/18/468
> >>>>>>>
> >>>>>>>Seeing as the patch did not land upstream, I am wondering if there is
> >>>>>>>a solution that I am not seeing.
> >>>>>>>
> >>>>>>>I am willing to provide a patch given appropriate guidance.
> >>>>>>
> >>>>>>Hi Michael,
> >>>>>>
> >>>>>>The info on the email you referenced is kind of obsolete, TI SoCs
> >>>>>>are calling of_clk_init() during boot now, and thus external clock
> >>>>>>nodes should be registered fine also. Maybe you can provide the
> >>>>>>actual DTS patch you are trying out so we can help better...? Are
> >>>>>
> >>>>>See attached patch and console output.
> >>>>
> >>>>I see a bug in your dt data.
> >>>>
> >>>><snip>
> >>>>
> >>>>+ clocks {
> >>>>+ ref27: ref27 {
> >>>>+ #clock-cells = <0>;
> >>>>+ compatibale = "fixed-clock";
> >>>>
> >>>>This should be compatible, right? DT is annoying in that it doesn't
> >>>>verify property names.
> >>>>
> >>>
> >>>Ooops.
> >>>
> >>>Now the clock appears in /sys/kernel/debug/clk:
> >>>root@som3517-som200:/sys/kernel/debug/clk# cat clk_summary
> >>> clock enable_cnt prepare_cnt rate accuracy phase
> >>>----------------------------------------------------------------------------------------
> >>> ref27 0 0 27000000 0 0
> >>>...
> >>>
> >>>There is still an issue with the si5351.
> >>>
> >>>I had to comment out the clk_put here for the frequency to show up:
> >>>http://lxr.free-electrons.com/source/drivers/clk/clk-si5351.c#L1133
> >>>
> >>>Ideas?
> >>
> >>What is the most recent upstream commit that you are based on?
> >
> >I am working from 4.0.0-rc7.
> >
> >7b43b47373d40d557cd7e1a84a0bd8ebc4d745ab
>
> Hmm, I wonder why si5351 calls clk_put immediately after of_clk_get
> in the first place, as far as I understand this destroys the clock
> handle, which is still being used later in the code.
>
> -Tero
Not sure how this ever worked. This has been in the code since the
initial commit.
Sebastian?
>
> >
> >>
> >>Regards,
> >>Mike
> >>
> >>>
> >>>>+ clock-frequency = <27000000>;
> >>>>+ };
> >>>>+ };
> >>>>
> >>>>-Tero
> >>>>
> >>>>>
> >>>>>>you seeing any boot time error / warning prints for your new clock?
> >>>>>
> >>>>>With the debug messages on you see that the reference clock is not being
> >>>>>detected.
> >>>>>
> >>>>>Whilest debugging I found that the of_clk_get is returning an error no matter
> >>>>>which clock I pass it:
> >>>>>http://lxr.free-electrons.com/source/drivers/clk/clk-si5351.c#L1131
> >>>>>
> >>>>>>
> >>>>>>-Tero
> >>>>
>
On 16.04.2015 18:17, Michael Welling wrote:
> On Thu, Apr 16, 2015 at 07:32:32AM +0300, Tero Kristo wrote:
>> On 04/15/2015 11:51 PM, Michael Welling wrote:
>>> On Wed, Apr 15, 2015 at 01:45:53PM -0700, Mike Turquette wrote:
>>>> On Wed, Apr 15, 2015 at 12:47 PM, Michael Welling <[email protected]> wrote:
[...]
>>>>> There is still an issue with the si5351.
>>>>>
>>>>> I had to comment out the clk_put here for the frequency to show up:
>>>>> http://lxr.free-electrons.com/source/drivers/clk/clk-si5351.c#L1133
>>>>>
>>>>> Ideas?
>>>>
>>>> What is the most recent upstream commit that you are based on?
>>>
>>> I am working from 4.0.0-rc7.
>>>
>>> 7b43b47373d40d557cd7e1a84a0bd8ebc4d745ab
>>
>> Hmm, I wonder why si5351 calls clk_put immediately after of_clk_get
>> in the first place, as far as I understand this destroys the clock
>> handle, which is still being used later in the code.
>
> Not sure how this ever worked. This has been in the code since the
> initial commit.
The reason it worked before may be related with recent rework of
clk_put() itself and clk cookies instead of pointers. I lost track on
the recent clk subsystem changes here, sorry.
However, droping the clk immediately surely isn't right.
The thing is, we can remove the clk_put() just because there is no
_remove() for that driver. I remember that back in the days the driver
was mainlined, clk removal wasn't too easy.
FWIW, as soon as _remove() support will be added by someone, we'll have
to rethink passing struct clk* by platform_data or at least
double-check if we ever used [of_]clk_get() to obtain it.
Mind to send a patch removing the clk_put() on !IS_ERR and add a proper
error path instead? While of_clk_get() is the only calls that need
cleanup on error in si5351_dt_parse() we should probably move that
calls to the end of this function. Otherwise we'd also have to cleanup
on every of_parse_foo() failure.
Sebastian
On Thu, Apr 16, 2015 at 10:37:19PM +0200, Sebastian Hesselbarth wrote:
> On 16.04.2015 18:17, Michael Welling wrote:
> >On Thu, Apr 16, 2015 at 07:32:32AM +0300, Tero Kristo wrote:
> >>On 04/15/2015 11:51 PM, Michael Welling wrote:
> >>>On Wed, Apr 15, 2015 at 01:45:53PM -0700, Mike Turquette wrote:
> >>>>On Wed, Apr 15, 2015 at 12:47 PM, Michael Welling <[email protected]> wrote:
> [...]
> >>>>>There is still an issue with the si5351.
> >>>>>
> >>>>>I had to comment out the clk_put here for the frequency to show up:
> >>>>>http://lxr.free-electrons.com/source/drivers/clk/clk-si5351.c#L1133
> >>>>>
> >>>>>Ideas?
> >>>>
> >>>>What is the most recent upstream commit that you are based on?
> >>>
> >>>I am working from 4.0.0-rc7.
> >>>
> >>>7b43b47373d40d557cd7e1a84a0bd8ebc4d745ab
> >>
> >>Hmm, I wonder why si5351 calls clk_put immediately after of_clk_get
> >>in the first place, as far as I understand this destroys the clock
> >>handle, which is still being used later in the code.
> >
> >Not sure how this ever worked. This has been in the code since the
> >initial commit.
>
> The reason it worked before may be related with recent rework of
> clk_put() itself and clk cookies instead of pointers. I lost track on
> the recent clk subsystem changes here, sorry.
>
> However, droping the clk immediately surely isn't right.
> The thing is, we can remove the clk_put() just because there is no
> _remove() for that driver. I remember that back in the days the driver
> was mainlined, clk removal wasn't too easy.
>
> FWIW, as soon as _remove() support will be added by someone, we'll have
> to rethink passing struct clk* by platform_data or at least
> double-check if we ever used [of_]clk_get() to obtain it.
>
> Mind to send a patch removing the clk_put() on !IS_ERR and add a proper
> error path instead? While of_clk_get() is the only calls that need
> cleanup on error in si5351_dt_parse() we should probably move that
> calls to the end of this function. Otherwise we'd also have to cleanup
> on every of_parse_foo() failure.
What would be the proper error path?
What cleanup is required?
It should be noted that there are more deep rooted issues with the driver
that I have noticed. For one the driver behaves differently if the debugging
is on and when it is off.
Here is what the kernel reports with debugging off:
root@som3517-som200:~# cat /sys/kernel/debug/clk/clk_summary
clock enable_cnt prepare_cnt rate accuracy phase
----------------------------------------------------------------------------------------
ref27 0 0 27000000 0 0
xtal 0 0 27000000 0 0
pllb 0 0 599999994 0 0
ms0 0 0 12499999 0 0
clk0 0 0 12499999 0 0
plla 0 0 599999994 0 0
ms2 0 0 8219178 0 0
clk2 0 0 8219178 0 0
ms1 0 0 94117646 0 0
clk1 0 0 94117646 0 0
Here is what the kernel reports with debugging on:
clock enable_cnt prepare_cnt rate accuracy phase
----------------------------------------------------------------------------------------
ref27 0 0 27000000 0 0
xtal 0 0 27000000 0 0
pllb 0 0 884736000 0 0
ms0 0 0 18432000 0 0
clk0 0 0 18432000 0 0
plla 0 0 897023997 0 0
ms2 0 0 12287999 0 0
clk2 0 0 12287999 0 0
ms1 0 0 140709646 0 0
clk1 0 0 140709646 0 0
Note this is with the following devicetree entry:
si5351: clock-generator {
#address-cells = <1>;
#size-cells = <0>;
#clock-cells = <1>;
compatible = "silabs,si5351a-msop";
reg = <0x60>;
status = "okay";
/* connect xtal input to 27MHz reference */
clocks = <&ref27>;
/* connect xtal input as source of pll0 and pll1 */
silabs,pll-source = <0 0>, <1 0>;
clkout0: clkout0 {
reg = <0>;
silabs,drive-strength = <8>;
silabs,multisynth-source = <1>;
silabs,clock-source = <0>;
silabs,pll-master;
clock-frequency = <18432000>;
};
clkout1: clkout1 {
reg = <1>;
silabs,drive-strength = <8>;
silabs,multisynth-source = <0>;
silabs,clock-source = <0>;
clock-frequency = <8000000>;
};
clkout2: clkout2 {
reg = <2>;
silabs,drive-strength = <8>;
silabs,multisynth-source = <0>;
silabs,clock-source = <0>;
silabs,pll-master;
clock-frequency = <12288000>;
};
};
I am losing hope that this driver is stable enough to even use in production.
>
> Sebastian
>
On 17.04.2015 00:09, Michael Welling wrote:
> On Thu, Apr 16, 2015 at 10:37:19PM +0200, Sebastian Hesselbarth wrote:
>> On 16.04.2015 18:17, Michael Welling wrote:
>>> On Thu, Apr 16, 2015 at 07:32:32AM +0300, Tero Kristo wrote:
>>>> On 04/15/2015 11:51 PM, Michael Welling wrote:
>>>>> On Wed, Apr 15, 2015 at 01:45:53PM -0700, Mike Turquette wrote:
>>>>>> On Wed, Apr 15, 2015 at 12:47 PM, Michael Welling <[email protected]> wrote:
>> [...]
>>>>>>> There is still an issue with the si5351.
>>>>>>>
>>>>>>> I had to comment out the clk_put here for the frequency to show up:
>>>>>>> http://lxr.free-electrons.com/source/drivers/clk/clk-si5351.c#L1133
>>>>>>>
>>>>>>> Ideas?
>>>>>>
>>>>>> What is the most recent upstream commit that you are based on?
>>>>>
>>>>> I am working from 4.0.0-rc7.
>>>>>
>>>>> 7b43b47373d40d557cd7e1a84a0bd8ebc4d745ab
>>>>
>>>> Hmm, I wonder why si5351 calls clk_put immediately after of_clk_get
>>>> in the first place, as far as I understand this destroys the clock
>>>> handle, which is still being used later in the code.
>>>
>>> Not sure how this ever worked. This has been in the code since the
>>> initial commit.
>>
>> The reason it worked before may be related with recent rework of
>> clk_put() itself and clk cookies instead of pointers. I lost track on
>> the recent clk subsystem changes here, sorry.
>>
>> However, droping the clk immediately surely isn't right.
>> The thing is, we can remove the clk_put() just because there is no
>> _remove() for that driver. I remember that back in the days the driver
>> was mainlined, clk removal wasn't too easy.
>>
>> FWIW, as soon as _remove() support will be added by someone, we'll have
>> to rethink passing struct clk* by platform_data or at least
>> double-check if we ever used [of_]clk_get() to obtain it.
>>
>> Mind to send a patch removing the clk_put() on !IS_ERR and add a proper
>> error path instead? While of_clk_get() is the only calls that need
>> cleanup on error in si5351_dt_parse() we should probably move that
>> calls to the end of this function. Otherwise we'd also have to cleanup
>> on every of_parse_foo() failure.
>
> What would be the proper error path?
> What cleanup is required?
A proper error path would be to release any claimed resource
on any error. If you look at the code, the only resources that
need to be released are the two clocks in question.
> It should be noted that there are more deep rooted issues with the driver
> that I have noticed. For one the driver behaves differently if the debugging
> is on and when it is off.
I guess you mean #define DEBUG in the driver?
> Here is what the kernel reports with debugging off:
Do you have any measurement equipment to check what is actually set?
> root@som3517-som200:~# cat /sys/kernel/debug/clk/clk_summary
> clock enable_cnt prepare_cnt rate accuracy phase
> ----------------------------------------------------------------------------------------
> ref27 0 0 27000000 0 0
> xtal 0 0 27000000 0 0
> pllb 0 0 599999994 0 0
> ms0 0 0 12499999 0 0
> clk0 0 0 12499999 0 0
> plla 0 0 599999994 0 0
> ms2 0 0 8219178 0 0
> clk2 0 0 8219178 0 0
> ms1 0 0 94117646 0 0
> clk1 0 0 94117646 0 0
>
> Here is what the kernel reports with debugging on:
> clock enable_cnt prepare_cnt rate accuracy phase
> ----------------------------------------------------------------------------------------
> ref27 0 0 27000000 0 0
> xtal 0 0 27000000 0 0
> pllb 0 0 884736000 0 0
> ms0 0 0 18432000 0 0
> clk0 0 0 18432000 0 0
Is this what you expect for clk0?
> plla 0 0 897023997 0 0
> ms2 0 0 12287999 0 0
> clk2 0 0 12287999 0 0
ditto for clk2?
> ms1 0 0 140709646 0 0
> clk1 0 0 140709646 0 0
This is wrong, I agree. Looks like round_rate()/recalc_rate() of msynth
or clkout is broken with respect to non-pll-master clocks.
I had a quick look at drivers/clk.c too, there has been a lot of churn
in clk API since I last booted my device using si5351.
Is there any way to try out a less recent kernel, let's say two or
three releases before 4.0?
We should just confirm that there has been an issue with it before
already.
I have no clue about the debug on/off issue at the moment.
> Note this is with the following devicetree entry:
> si5351: clock-generator {
> #address-cells = <1>;
> #size-cells = <0>;
> #clock-cells = <1>;
> compatible = "silabs,si5351a-msop";
> reg = <0x60>;
> status = "okay";
>
> /* connect xtal input to 27MHz reference */
> clocks = <&ref27>;
>
> /* connect xtal input as source of pll0 and pll1 */
> silabs,pll-source = <0 0>, <1 0>;
>
> clkout0: clkout0 {
> reg = <0>;
> silabs,drive-strength = <8>;
> silabs,multisynth-source = <1>;
> silabs,clock-source = <0>;
> silabs,pll-master;
> clock-frequency = <18432000>;
> };
>
> clkout1: clkout1 {
> reg = <1>;
> silabs,drive-strength = <8>;
> silabs,multisynth-source = <0>;
> silabs,clock-source = <0>;
> clock-frequency = <8000000>;
> };
>
> clkout2: clkout2 {
> reg = <2>;
> silabs,drive-strength = <8>;
> silabs,multisynth-source = <0>;
> silabs,clock-source = <0>;
> silabs,pll-master;
> clock-frequency = <12288000>;
> };
> };
>
> I am losing hope that this driver is stable enough to even use in production.
Who said it is stable for production use? The driver is written from
scratch based on _very_ limited information of the datasheet an appnote.
Also, I only have a single setup with si5351, that is no way enough to
test every combination.
I never heard serious complaints before, so either you help improving
this driver or better ask SiLabs for a table-based driver for your
specific setup.
Sebastian
On Fri, Apr 17, 2015 at 01:23:50AM +0200, Sebastian Hesselbarth wrote:
> On 17.04.2015 00:09, Michael Welling wrote:
> >On Thu, Apr 16, 2015 at 10:37:19PM +0200, Sebastian Hesselbarth wrote:
> >>On 16.04.2015 18:17, Michael Welling wrote:
> >>>On Thu, Apr 16, 2015 at 07:32:32AM +0300, Tero Kristo wrote:
> >>>>On 04/15/2015 11:51 PM, Michael Welling wrote:
> >>>>>On Wed, Apr 15, 2015 at 01:45:53PM -0700, Mike Turquette wrote:
> >>>>>>On Wed, Apr 15, 2015 at 12:47 PM, Michael Welling <[email protected]> wrote:
> >>[...]
> >>>>>>>There is still an issue with the si5351.
> >>>>>>>
> >>>>>>>I had to comment out the clk_put here for the frequency to show up:
> >>>>>>>http://lxr.free-electrons.com/source/drivers/clk/clk-si5351.c#L1133
> >>>>>>>
> >>>>>>>Ideas?
> >>>>>>
> >>>>>>What is the most recent upstream commit that you are based on?
> >>>>>
> >>>>>I am working from 4.0.0-rc7.
> >>>>>
> >>>>>7b43b47373d40d557cd7e1a84a0bd8ebc4d745ab
> >>>>
> >>>>Hmm, I wonder why si5351 calls clk_put immediately after of_clk_get
> >>>>in the first place, as far as I understand this destroys the clock
> >>>>handle, which is still being used later in the code.
> >>>
> >>>Not sure how this ever worked. This has been in the code since the
> >>>initial commit.
> >>
> >>The reason it worked before may be related with recent rework of
> >>clk_put() itself and clk cookies instead of pointers. I lost track on
> >>the recent clk subsystem changes here, sorry.
> >>
> >>However, droping the clk immediately surely isn't right.
> >>The thing is, we can remove the clk_put() just because there is no
> >>_remove() for that driver. I remember that back in the days the driver
> >>was mainlined, clk removal wasn't too easy.
> >>
> >>FWIW, as soon as _remove() support will be added by someone, we'll have
> >>to rethink passing struct clk* by platform_data or at least
> >>double-check if we ever used [of_]clk_get() to obtain it.
> >>
> >>Mind to send a patch removing the clk_put() on !IS_ERR and add a proper
> >>error path instead? While of_clk_get() is the only calls that need
> >>cleanup on error in si5351_dt_parse() we should probably move that
> >>calls to the end of this function. Otherwise we'd also have to cleanup
> >>on every of_parse_foo() failure.
> >
> >What would be the proper error path?
> >What cleanup is required?
>
> A proper error path would be to release any claimed resource
> on any error. If you look at the code, the only resources that
> need to be released are the two clocks in question.
So for every error return in the probe function and in the of si5351_dt_parse
it needs to clk_put first right?
See attached patch to see if we are on the same page.
>
> >It should be noted that there are more deep rooted issues with the driver
> >that I have noticed. For one the driver behaves differently if the debugging
> >is on and when it is off.
>
> I guess you mean #define DEBUG in the driver?
Yes.
>
> >Here is what the kernel reports with debugging off:
>
> Do you have any measurement equipment to check what is actually set?
Yes, I have an oscilloscope here at my desk.
The reported numbers do not always correspond to the actual output in some
cases.
The ms2 output has appeared to stop working all together sometime whilest
testing. I may have to solder a new chip on there.
Could misconfiguration damage the chip?
>
> >root@som3517-som200:~# cat /sys/kernel/debug/clk/clk_summary
> > clock enable_cnt prepare_cnt rate accuracy phase
> >----------------------------------------------------------------------------------------
> > ref27 0 0 27000000 0 0
> > xtal 0 0 27000000 0 0
> > pllb 0 0 599999994 0 0
> > ms0 0 0 12499999 0 0
> > clk0 0 0 12499999 0 0
> > plla 0 0 599999994 0 0
> > ms2 0 0 8219178 0 0
> > clk2 0 0 8219178 0 0
> > ms1 0 0 94117646 0 0
> > clk1 0 0 94117646 0 0
> >
> >Here is what the kernel reports with debugging on:
> > clock enable_cnt prepare_cnt rate accuracy phase
> >----------------------------------------------------------------------------------------
> > ref27 0 0 27000000 0 0
> > xtal 0 0 27000000 0 0
> > pllb 0 0 884736000 0 0
> > ms0 0 0 18432000 0 0
> > clk0 0 0 18432000 0 0
>
> Is this what you expect for clk0?
Yes.
>
> > plla 0 0 897023997 0 0
> > ms2 0 0 12287999 0 0
> > clk2 0 0 12287999 0 0
>
> ditto for clk2?
Yes.
>
> > ms1 0 0 140709646 0 0
> > clk1 0 0 140709646 0 0
>
> This is wrong, I agree. Looks like round_rate()/recalc_rate() of msynth
> or clkout is broken with respect to non-pll-master clocks.
>
> I had a quick look at drivers/clk.c too, there has been a lot of churn
> in clk API since I last booted my device using si5351.
>
> Is there any way to try out a less recent kernel, let's say two or
> three releases before 4.0?
Could you provide a specific version that you think has the best chances of
working?
>
> We should just confirm that there has been an issue with it before
> already.
>
> I have no clue about the debug on/off issue at the moment.
>
> >Note this is with the following devicetree entry:
> > si5351: clock-generator {
> > #address-cells = <1>;
> > #size-cells = <0>;
> > #clock-cells = <1>;
> > compatible = "silabs,si5351a-msop";
> > reg = <0x60>;
> > status = "okay";
> >
> > /* connect xtal input to 27MHz reference */
> > clocks = <&ref27>;
> >
> > /* connect xtal input as source of pll0 and pll1 */
> > silabs,pll-source = <0 0>, <1 0>;
> >
> > clkout0: clkout0 {
> > reg = <0>;
> > silabs,drive-strength = <8>;
> > silabs,multisynth-source = <1>;
> > silabs,clock-source = <0>;
> > silabs,pll-master;
> > clock-frequency = <18432000>;
> > };
> >
> > clkout1: clkout1 {
> > reg = <1>;
> > silabs,drive-strength = <8>;
> > silabs,multisynth-source = <0>;
> > silabs,clock-source = <0>;
> > clock-frequency = <8000000>;
> > };
> >
> > clkout2: clkout2 {
> > reg = <2>;
> > silabs,drive-strength = <8>;
> > silabs,multisynth-source = <0>;
> > silabs,clock-source = <0>;
> > silabs,pll-master;
> > clock-frequency = <12288000>;
> > };
> > };
> >
> >I am losing hope that this driver is stable enough to even use in production.
>
> Who said it is stable for production use? The driver is written from
> scratch based on _very_ limited information of the datasheet an appnote.
> Also, I only have a single setup with si5351, that is no way enough to
> test every combination.
Well it is not in staging and I am sure it took much work to get it working
for you.
>
> I never heard serious complaints before, so either you help improving
> this driver or better ask SiLabs for a table-based driver for your
> specific setup.
I have routines to program the chip from U-Boot and Linux userspace using
the table method. I was hoping that a mainline driver could replace these
hackish utilities.
>
> Sebastian
On 04/17/2015 05:00 AM, Michael Welling wrote:
> On Fri, Apr 17, 2015 at 01:23:50AM +0200, Sebastian Hesselbarth wrote:
>> On 17.04.2015 00:09, Michael Welling wrote:
>>> On Thu, Apr 16, 2015 at 10:37:19PM +0200, Sebastian Hesselbarth wrote:
>>>> On 16.04.2015 18:17, Michael Welling wrote:
>>>>> On Thu, Apr 16, 2015 at 07:32:32AM +0300, Tero Kristo wrote:
>>>>>> On 04/15/2015 11:51 PM, Michael Welling wrote:
>>>>>>> On Wed, Apr 15, 2015 at 01:45:53PM -0700, Mike Turquette wrote:
>>>>>>>> On Wed, Apr 15, 2015 at 12:47 PM, Michael Welling <[email protected]> wrote:
>>>> [...]
>>>>>>>>> There is still an issue with the si5351.
>>>>>>>>>
>>>>>>>>> I had to comment out the clk_put here for the frequency to show up:
>>>>>>>>> http://lxr.free-electrons.com/source/drivers/clk/clk-si5351.c#L1133
>>>>>>>>>
>>>>>>>>> Ideas?
>>>>>>>>
>>>>>>>> What is the most recent upstream commit that you are based on?
>>>>>>>
>>>>>>> I am working from 4.0.0-rc7.
>>>>>>>
>>>>>>> 7b43b47373d40d557cd7e1a84a0bd8ebc4d745ab
>>>>>>
>>>>>> Hmm, I wonder why si5351 calls clk_put immediately after of_clk_get
>>>>>> in the first place, as far as I understand this destroys the clock
>>>>>> handle, which is still being used later in the code.
>>>>>
>>>>> Not sure how this ever worked. This has been in the code since the
>>>>> initial commit.
>>>>
>>>> The reason it worked before may be related with recent rework of
>>>> clk_put() itself and clk cookies instead of pointers. I lost track on
>>>> the recent clk subsystem changes here, sorry.
>>>>
>>>> However, droping the clk immediately surely isn't right.
>>>> The thing is, we can remove the clk_put() just because there is no
>>>> _remove() for that driver. I remember that back in the days the driver
>>>> was mainlined, clk removal wasn't too easy.
>>>>
>>>> FWIW, as soon as _remove() support will be added by someone, we'll have
>>>> to rethink passing struct clk* by platform_data or at least
>>>> double-check if we ever used [of_]clk_get() to obtain it.
>>>>
>>>> Mind to send a patch removing the clk_put() on !IS_ERR and add a proper
>>>> error path instead? While of_clk_get() is the only calls that need
>>>> cleanup on error in si5351_dt_parse() we should probably move that
>>>> calls to the end of this function. Otherwise we'd also have to cleanup
>>>> on every of_parse_foo() failure.
>>>
>>> What would be the proper error path?
>>> What cleanup is required?
>>
>> A proper error path would be to release any claimed resource
>> on any error. If you look at the code, the only resources that
>> need to be released are the two clocks in question.
>
> So for every error return in the probe function and in the of si5351_dt_parse
> it needs to clk_put first right?
>
> See attached patch to see if we are on the same page.
>
>>
>>> It should be noted that there are more deep rooted issues with the driver
>>> that I have noticed. For one the driver behaves differently if the debugging
>>> is on and when it is off.
>>
>> I guess you mean #define DEBUG in the driver?
>
> Yes.
>
>>
>>> Here is what the kernel reports with debugging off:
>>
>> Do you have any measurement equipment to check what is actually set?
>
> Yes, I have an oscilloscope here at my desk.
> The reported numbers do not always correspond to the actual output in some
> cases.
>
> The ms2 output has appeared to stop working all together sometime whilest
> testing. I may have to solder a new chip on there.
>
> Could misconfiguration damage the chip?
>
>>
>>> root@som3517-som200:~# cat /sys/kernel/debug/clk/clk_summary
>>> clock enable_cnt prepare_cnt rate accuracy phase
>>> ----------------------------------------------------------------------------------------
>>> ref27 0 0 27000000 0 0
>>> xtal 0 0 27000000 0 0
>>> pllb 0 0 599999994 0 0
>>> ms0 0 0 12499999 0 0
>>> clk0 0 0 12499999 0 0
>>> plla 0 0 599999994 0 0
>>> ms2 0 0 8219178 0 0
>>> clk2 0 0 8219178 0 0
>>> ms1 0 0 94117646 0 0
>>> clk1 0 0 94117646 0 0
>>>
>>> Here is what the kernel reports with debugging on:
>>> clock enable_cnt prepare_cnt rate accuracy phase
>>> ----------------------------------------------------------------------------------------
>>> ref27 0 0 27000000 0 0
>>> xtal 0 0 27000000 0 0
>>> pllb 0 0 884736000 0 0
>>> ms0 0 0 18432000 0 0
>>> clk0 0 0 18432000 0 0
>>
>> Is this what you expect for clk0?
>
> Yes.
>
>>
>>> plla 0 0 897023997 0 0
>>> ms2 0 0 12287999 0 0
>>> clk2 0 0 12287999 0 0
>>
>> ditto for clk2?
>
> Yes.
>
>>
>>> ms1 0 0 140709646 0 0
>>> clk1 0 0 140709646 0 0
>>
>> This is wrong, I agree. Looks like round_rate()/recalc_rate() of msynth
>> or clkout is broken with respect to non-pll-master clocks.
>>
>> I had a quick look at drivers/clk.c too, there has been a lot of churn
>> in clk API since I last booted my device using si5351.
>>
>> Is there any way to try out a less recent kernel, let's say two or
>> three releases before 4.0?
>
> Could you provide a specific version that you think has the best chances of
> working?
>
>>
>> We should just confirm that there has been an issue with it before
>> already.
>>
>> I have no clue about the debug on/off issue at the moment.
>>
>>> Note this is with the following devicetree entry:
>>> si5351: clock-generator {
>>> #address-cells = <1>;
>>> #size-cells = <0>;
>>> #clock-cells = <1>;
>>> compatible = "silabs,si5351a-msop";
>>> reg = <0x60>;
>>> status = "okay";
>>>
>>> /* connect xtal input to 27MHz reference */
>>> clocks = <&ref27>;
>>>
>>> /* connect xtal input as source of pll0 and pll1 */
>>> silabs,pll-source = <0 0>, <1 0>;
>>>
>>> clkout0: clkout0 {
>>> reg = <0>;
>>> silabs,drive-strength = <8>;
>>> silabs,multisynth-source = <1>;
>>> silabs,clock-source = <0>;
>>> silabs,pll-master;
>>> clock-frequency = <18432000>;
>>> };
>>>
>>> clkout1: clkout1 {
>>> reg = <1>;
>>> silabs,drive-strength = <8>;
>>> silabs,multisynth-source = <0>;
>>> silabs,clock-source = <0>;
>>> clock-frequency = <8000000>;
>>> };
>>>
>>> clkout2: clkout2 {
>>> reg = <2>;
>>> silabs,drive-strength = <8>;
>>> silabs,multisynth-source = <0>;
>>> silabs,clock-source = <0>;
>>> silabs,pll-master;
>>> clock-frequency = <12288000>;
>>> };
>>> };
>>>
>>> I am losing hope that this driver is stable enough to even use in production.
>>
>> Who said it is stable for production use? The driver is written from
>> scratch based on _very_ limited information of the datasheet an appnote.
>> Also, I only have a single setup with si5351, that is no way enough to
>> test every combination.
>
> Well it is not in staging and I am sure it took much work to get it working
> for you.
non-staging doesn't mean code is absolutely bug free. Linux kernel is
still just software, and as we know, every piece of software has bugs
(except maybe the simplest hello-world app.)
>>
>> I never heard serious complaints before, so either you help improving
>> this driver or better ask SiLabs for a table-based driver for your
>> specific setup.
>
> I have routines to program the chip from U-Boot and Linux userspace using
> the table method. I was hoping that a mainline driver could replace these
> hackish utilities.
You can still replace your hack solution.
The beauty of linux comes in that if you find a bug someplace, you can
just fix it, post a patch upstream, and get it fixed for good. :)
-Tero
On 17.04.2015 04:00, Michael Welling wrote:
> On Fri, Apr 17, 2015 at 01:23:50AM +0200, Sebastian Hesselbarth wrote:
>> On 17.04.2015 00:09, Michael Welling wrote:
>>> On Thu, Apr 16, 2015 at 10:37:19PM +0200, Sebastian Hesselbarth wrote:
>>>> On 16.04.2015 18:17, Michael Welling wrote:
[...]
>>> What would be the proper error path?
>>> What cleanup is required?
>>
>> A proper error path would be to release any claimed resource
>> on any error. If you look at the code, the only resources that
>> need to be released are the two clocks in question.
>
> So for every error return in the probe function and in the of si5351_dt_parse
> it needs to clk_put first right?
Not quite. The driver should clk_put() every clock that it called a
[of_]clk_get() for. The thing is that clocks can be passed by
platform_data and we never claim them.
> See attached patch to see if we are on the same page.
Adding a .remove() function needs more care because of the pdata passed
clocks. I admit that back when the driver was introduced clk_put()
wasn't really necessary at all.
[...]
>>> Here is what the kernel reports with debugging off:
>>
>> Do you have any measurement equipment to check what is actually set?
>
> Yes, I have an oscilloscope here at my desk.
> The reported numbers do not always correspond to the actual output in some
> cases.
is "not always correspond" close or way off the requested frequency?
Stability is an issue that I am aware of. Neither the datasheet nor the
appnote were clear about any order the clocks should be set or how long
we should wait between changing pll/ms/clkout parameters.
SiLabs suggests to configure all clocks at once and never change them
later, at least that is what you can read from the public documents.
The drivers you mention below can however reveal some steps that have
to be taken care of before and after changing parameters.
> The ms2 output has appeared to stop working all together sometime whilest
> testing. I may have to solder a new chip on there.
>
> Could misconfiguration damage the chip?
You should know that a lot of things can damage a chip and
misconfiguration is among them, yes. I cannot tell if that
is the cause though.
[...]
>> Is there any way to try out a less recent kernel, let's say two or
>> three releases before 4.0?
>
> Could you provide a specific version that you think has the best chances of
> working?
My guess with 2-3 releases before 4.0 was because somewhere in between
clk API must have switched from passing struct clk pointers to clk
cookies.
[...]
From your patch (that you should inline next time please):
@@ -1129,11 +1130,21 @@ static int si5351_dt_parse(struct i2c_client
*client,
return -ENOMEM;
pdata->clk_xtal = of_clk_get(np, 0);
- if (!IS_ERR(pdata->clk_xtal))
- clk_put(pdata->clk_xtal);
- pdata->clk_clkin = of_clk_get(np, 1);
- if (!IS_ERR(pdata->clk_clkin))
- clk_put(pdata->clk_clkin);
+ if (IS_ERR(pdata->clk_xtal)) {
+ dev_err(&client->dev,
+ "xtal clock not speficied\n");
+ return -ENODEV;
+ }
+
+ if (variant == SI5351_VARIANT_C) {
+ pdata->clk_clkin = of_clk_get(np, 1);
+ if (IS_ERR(pdata->clk_clkin)) {
+ dev_err(&client->dev,
+ "clkin clock not speficied\n");
+ ret = -ENODEV;
+ goto err_put_clk_of;
+ }
+ }
Basically, yes. But as I said, if you move that to the end of
si5351_dt_parse() you won't have to add
@ -1143,14 +1154,16 @@ static int si5351_dt_parse(struct i2c_client *client,
if (num >= 2) {
dev_err(&client->dev,
"invalid pll %d on pll-source prop\n", num);
- return -EINVAL;
+ ret = -EINVAL;
+ goto err_put_clk_of;
}
this to each of the sanity checks. Claiming the clock resources at
the end saves you from tearing down the resources on each error above.
Another thing is that you now require VARIANT_C devices to always
pass both, xtal and clkin. I can live with it until we find somebody
who actually uses C-type devices with only one clock input connected.
Adding a .remove() function to si5351 definitely needs more care with
respect to claimed and pdata passed clocks. I am not sure we should
go for it before we haven't ruled out the other issues.
Sebastian
On Fri, Apr 17, 2015 at 11:12:03AM +0200, Sebastian Hesselbarth wrote:
> On 17.04.2015 04:00, Michael Welling wrote:
> >On Fri, Apr 17, 2015 at 01:23:50AM +0200, Sebastian Hesselbarth wrote:
> >>On 17.04.2015 00:09, Michael Welling wrote:
> >>>On Thu, Apr 16, 2015 at 10:37:19PM +0200, Sebastian Hesselbarth wrote:
> >>>>On 16.04.2015 18:17, Michael Welling wrote:
> [...]
> >>>What would be the proper error path?
> >>>What cleanup is required?
> >>
> >>A proper error path would be to release any claimed resource
> >>on any error. If you look at the code, the only resources that
> >>need to be released are the two clocks in question.
> >
> >So for every error return in the probe function and in the of si5351_dt_parse
> >it needs to clk_put first right?
>
> Not quite. The driver should clk_put() every clock that it called a
> [of_]clk_get() for. The thing is that clocks can be passed by
> platform_data and we never claim them.
I've always said clocks (as in struct clk) should never be passed through
platform data.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
On Fri, Apr 17, 2015 at 11:12:03AM +0200, Sebastian Hesselbarth wrote:
> On 17.04.2015 04:00, Michael Welling wrote:
> >On Fri, Apr 17, 2015 at 01:23:50AM +0200, Sebastian Hesselbarth wrote:
> >>On 17.04.2015 00:09, Michael Welling wrote:
> >>>On Thu, Apr 16, 2015 at 10:37:19PM +0200, Sebastian Hesselbarth wrote:
> >>>>On 16.04.2015 18:17, Michael Welling wrote:
> [...]
> >>>What would be the proper error path?
> >>>What cleanup is required?
> >>
> >>A proper error path would be to release any claimed resource
> >>on any error. If you look at the code, the only resources that
> >>need to be released are the two clocks in question.
> >
> >So for every error return in the probe function and in the of si5351_dt_parse
> >it needs to clk_put first right?
>
> Not quite. The driver should clk_put() every clock that it called a
> [of_]clk_get() for. The thing is that clocks can be passed by
> platform_data and we never claim them.
>
Should the ones from the platform data be claimed?
> >See attached patch to see if we are on the same page.
>
> Adding a .remove() function needs more care because of the pdata passed
> clocks. I admit that back when the driver was introduced clk_put()
> wasn't really necessary at all.
>
Is it necessary now?
> [...]
> >>>Here is what the kernel reports with debugging off:
> >>
> >>Do you have any measurement equipment to check what is actually set?
> >
> >Yes, I have an oscilloscope here at my desk.
> >The reported numbers do not always correspond to the actual output in some
> >cases.
>
> is "not always correspond" close or way off the requested frequency?
>
Both.
> Stability is an issue that I am aware of. Neither the datasheet nor the
> appnote were clear about any order the clocks should be set or how long
> we should wait between changing pll/ms/clkout parameters.
>
> SiLabs suggests to configure all clocks at once and never change them
> later, at least that is what you can read from the public documents.
> The drivers you mention below can however reveal some steps that have
> to be taken care of before and after changing parameters.
>
The drivers were not provided we wrote them and they do nothing but write
the registers with the raw values provided from the SiLab clock generator
software.
> >The ms2 output has appeared to stop working all together sometime whilest
> >testing. I may have to solder a new chip on there.
> >
> >Could misconfiguration damage the chip?
>
> You should know that a lot of things can damage a chip and
> misconfiguration is among them, yes. I cannot tell if that
> is the cause though.
>
Well I did something to the chip.
> [...]
> >>Is there any way to try out a less recent kernel, let's say two or
> >>three releases before 4.0?
> >
> >Could you provide a specific version that you think has the best chances of
> >working?
>
> My guess with 2-3 releases before 4.0 was because somewhere in between
> clk API must have switched from passing struct clk pointers to clk
> cookies.
>
> [...]
>
> From your patch (that you should inline next time please):
>
> @@ -1129,11 +1130,21 @@ static int si5351_dt_parse(struct i2c_client
> *client,
> return -ENOMEM;
>
> pdata->clk_xtal = of_clk_get(np, 0);
> - if (!IS_ERR(pdata->clk_xtal))
> - clk_put(pdata->clk_xtal);
> - pdata->clk_clkin = of_clk_get(np, 1);
> - if (!IS_ERR(pdata->clk_clkin))
> - clk_put(pdata->clk_clkin);
> + if (IS_ERR(pdata->clk_xtal)) {
> + dev_err(&client->dev,
> + "xtal clock not speficied\n");
> + return -ENODEV;
> + }
> +
> + if (variant == SI5351_VARIANT_C) {
> + pdata->clk_clkin = of_clk_get(np, 1);
> + if (IS_ERR(pdata->clk_clkin)) {
> + dev_err(&client->dev,
> + "clkin clock not speficied\n");
> + ret = -ENODEV;
> + goto err_put_clk_of;
> + }
> + }
>
> Basically, yes. But as I said, if you move that to the end of
> si5351_dt_parse() you won't have to add
>
> @ -1143,14 +1154,16 @@ static int si5351_dt_parse(struct i2c_client *client,
> if (num >= 2) {
> dev_err(&client->dev,
> "invalid pll %d on pll-source prop\n", num);
> - return -EINVAL;
> + ret = -EINVAL;
> + goto err_put_clk_of;
> }
>
> this to each of the sanity checks. Claiming the clock resources at
> the end saves you from tearing down the resources on each error above.
>
Sure I can move it down put it seems counterintuitive to parse parameters that
rely on the clocks before they are determined to be specified.
> Another thing is that you now require VARIANT_C devices to always
> pass both, xtal and clkin. I can live with it until we find somebody
> who actually uses C-type devices with only one clock input connected.
>
Okay I did not realize that the C variant could run with one or the other.
> Adding a .remove() function to si5351 definitely needs more care with
> respect to claimed and pdata passed clocks. I am not sure we should
> go for it before we haven't ruled out the other issues.
>
Okay. Either way we should probably open a new thread because this is not
an issue with the processor support. Let me know how you want to proceed.
It looks like this chip is used on the dove-cubox:
http://lxr.free-electrons.com/source/arch/arm/boot/dts/dove-cubox.dts#L77
I wonder if this issue effects that board as well.
Any one out there want with one of these board want to test it?
It looks like the clock is used for a audio master clock so it may sound
pretty funny when playing audio. :)
> Sebastian
On Fri, Apr 17, 2015 at 11:18:33AM +0100, Russell King - ARM Linux wrote:
> On Fri, Apr 17, 2015 at 11:12:03AM +0200, Sebastian Hesselbarth wrote:
> > On 17.04.2015 04:00, Michael Welling wrote:
> > >On Fri, Apr 17, 2015 at 01:23:50AM +0200, Sebastian Hesselbarth wrote:
> > >>On 17.04.2015 00:09, Michael Welling wrote:
> > >>>On Thu, Apr 16, 2015 at 10:37:19PM +0200, Sebastian Hesselbarth wrote:
> > >>>>On 16.04.2015 18:17, Michael Welling wrote:
> > [...]
> > >>>What would be the proper error path?
> > >>>What cleanup is required?
> > >>
> > >>A proper error path would be to release any claimed resource
> > >>on any error. If you look at the code, the only resources that
> > >>need to be released are the two clocks in question.
> > >
> > >So for every error return in the probe function and in the of si5351_dt_parse
> > >it needs to clk_put first right?
> >
> > Not quite. The driver should clk_put() every clock that it called a
> > [of_]clk_get() for. The thing is that clocks can be passed by
> > platform_data and we never claim them.
>
> I've always said clocks (as in struct clk) should never be passed through
> platform data.
>
What is the alternative for systems that still use the old platform files?
Hypothetically speaking of course.
> --
> FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
> according to speedtest.net.
On Fri, Apr 17, 2015 at 02:06:23PM -0500, Michael Welling wrote:
> On Fri, Apr 17, 2015 at 11:18:33AM +0100, Russell King - ARM Linux wrote:
> > On Fri, Apr 17, 2015 at 11:12:03AM +0200, Sebastian Hesselbarth wrote:
> > > On 17.04.2015 04:00, Michael Welling wrote:
> > > >On Fri, Apr 17, 2015 at 01:23:50AM +0200, Sebastian Hesselbarth wrote:
> > > >>On 17.04.2015 00:09, Michael Welling wrote:
> > > >>>On Thu, Apr 16, 2015 at 10:37:19PM +0200, Sebastian Hesselbarth wrote:
> > > >>>>On 16.04.2015 18:17, Michael Welling wrote:
> > > [...]
> > > >>>What would be the proper error path?
> > > >>>What cleanup is required?
> > > >>
> > > >>A proper error path would be to release any claimed resource
> > > >>on any error. If you look at the code, the only resources that
> > > >>need to be released are the two clocks in question.
> > > >
> > > >So for every error return in the probe function and in the of si5351_dt_parse
> > > >it needs to clk_put first right?
> > >
> > > Not quite. The driver should clk_put() every clock that it called a
> > > [of_]clk_get() for. The thing is that clocks can be passed by
> > > platform_data and we never claim them.
> >
> > I've always said clocks (as in struct clk) should never be passed through
> > platform data.
> >
>
> What is the alternative for systems that still use the old platform files?
clkdev, which has pre-existed DT.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
Quoting Russell King - ARM Linux (2015-04-17 03:18:33)
> On Fri, Apr 17, 2015 at 11:12:03AM +0200, Sebastian Hesselbarth wrote:
> > On 17.04.2015 04:00, Michael Welling wrote:
> > >On Fri, Apr 17, 2015 at 01:23:50AM +0200, Sebastian Hesselbarth wrote:
> > >>On 17.04.2015 00:09, Michael Welling wrote:
> > >>>On Thu, Apr 16, 2015 at 10:37:19PM +0200, Sebastian Hesselbarth wrote:
> > >>>>On 16.04.2015 18:17, Michael Welling wrote:
> > [...]
> > >>>What would be the proper error path?
> > >>>What cleanup is required?
> > >>
> > >>A proper error path would be to release any claimed resource
> > >>on any error. If you look at the code, the only resources that
> > >>need to be released are the two clocks in question.
> > >
> > >So for every error return in the probe function and in the of si5351_dt_parse
> > >it needs to clk_put first right?
> >
> > Not quite. The driver should clk_put() every clock that it called a
> > [of_]clk_get() for. The thing is that clocks can be passed by
> > platform_data and we never claim them.
>
> I've always said clocks (as in struct clk) should never be passed through
> platform data.
+1
And for ccf clock drivers Stephen and I plan to change the behavior of
clk_register() at some point so that it returns an error code and not a
struct clk. This will make clk_dev the only way to get at a struct clk
for users of the ccf implementation.
Of course it is still possible to clk_get from some place and pass as
platform_data, but every little bit helps.
Regards,
Mike
>
> --
> FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
> according to speedtest.net.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/