Bug in clk_mux_set_parent for CLK_MUX_INDEX_BIT muxes ?

Bug in clk_mux_set_parent for CLK_MUX_INDEX_BIT muxes ?

[Hans de Goede]

Hi,

While looking at what I believe is a bug in the sunxi gmac clk code
(more on that later), I believe I've also found a bug in

clk_mux_set_parent for the CLK_MUX_INDEX_BIT case.

AFAIK if CLK_MUX_INDEX_BIT is set, then each bit turns
on / off a single parent, so theoretically multiple
parents could be enabled at the same time, but in practice
only one bit should ever be 1. So to select parent 0, set
the register (*) to 0x01, to select parent 1 set it 0x02,
parent 2, 0x04, parent 3, 0x08, etc.

Correct ?

Assuming I got that right, then this bit of code in
clk_mux_set_parent is wrong:

                if (mux->flags & CLK_MUX_INDEX_BIT)
                        index = (1 << ffs(index));

For an input index of 0, ffs returns 0, so we set the register
to 0x01, ok.

For an input index of 1, ffs returns 1, so we set the register
to 0x02, ok.

For an input index of 2, ffs returns 2, so we set the register
to 0x04, ok.

For an input index of 3, ffs returns 1, so we set the register
to 0x02, not good!

I believe this code should simply be:

                if (mux->flags & CLK_MUX_INDEX_BIT)
                        index = 1 << index;

I guess we've been getting away by this because the input index
so far has never gotten above 2.

If I'm right about this, I can whip up a patch to fix this. Note
I don't really have hardware to properly exercise this code-path
AFAIK.

Regards,

Hans


*) This assumes a shift of 0

Bug in clk_mux_set_parent for CLK_MUX_INDEX_BIT muxes ?

[Tero Kristo]

On 11/17/2014 08:57 PM, Mike Turquette wrote:
> Quoting Hans de Goede (2014-11-16 04:34:46)
>> Hi,
>>
>> While looking at what I believe is a bug in the sunxi gmac clk code
>> (more on that later), I believe I've also found a bug in
>>
>> clk_mux_set_parent for the CLK_MUX_INDEX_BIT case.
>>
>> AFAIK if CLK_MUX_INDEX_BIT is set, then each bit turns
>> on / off a single parent, so theoretically multiple
>> parents could be enabled at the same time, but in practice
>> only one bit should ever be 1. So to select parent 0, set
>> the register (*) to 0x01, to select parent 1 set it 0x02,
>> parent 2, 0x04, parent 3, 0x08, etc.
>>
>> Correct ?
>>
>> Assuming I got that right, then this bit of code in
>> clk_mux_set_parent is wrong:
>>
>>                  if (mux->flags & CLK_MUX_INDEX_BIT)
>>                          index = (1 << ffs(index));
>>
>> For an input index of 0, ffs returns 0, so we set the register
>> to 0x01, ok.
>>
>> For an input index of 1, ffs returns 1, so we set the register
>> to 0x02, ok.
>>
>> For an input index of 2, ffs returns 2, so we set the register
>> to 0x04, ok.
>>
>> For an input index of 3, ffs returns 1, so we set the register
>> to 0x02, not good!
>>
>> I believe this code should simply be:
>>
>>                  if (mux->flags & CLK_MUX_INDEX_BIT)
>>                          index = 1 << index;
>>
>> I guess we've been getting away by this because the input index
>> so far has never gotten above 2.
>
> Good catch. I've Cc'd Tero Kristo from TI as the TI SoCs are the only
> other user of CLK_MUX_INDEX_BIT.
>
>>
>> If I'm right about this, I can whip up a patch to fix this. Note
>> I don't really have hardware to properly exercise this code-path
>> AFAIK.
>
> Please do. I think your change is reasonable and Tero can scream if
> something breaks for him.

TI SoC:s use a copy pasted version of the mux clock (with some 
additional tweaks), but we never set the CLK_MUX_INDEX_BIT flag, so we 
have been safe from this bug. Feel free to send a patch against the TI 
clock driver also if you want, I can ack that one. If not, I can craft a 
quick patch for this myself.

-Tero

Bug in clk_mux_set_parent for CLK_MUX_INDEX_BIT muxes ?

[Hans de Goede]

Hi,

On 11/18/2014 01:30 PM, Tero Kristo wrote:
> On 11/17/2014 08:57 PM, Mike Turquette wrote:
>> Quoting Hans de Goede (2014-11-16 04:34:46)
>>> Hi,
>>>
>>> While looking at what I believe is a bug in the sunxi gmac clk code
>>> (more on that later), I believe I've also found a bug in
>>>
>>> clk_mux_set_parent for the CLK_MUX_INDEX_BIT case.
>>>
>>> AFAIK if CLK_MUX_INDEX_BIT is set, then each bit turns
>>> on / off a single parent, so theoretically multiple
>>> parents could be enabled at the same time, but in practice
>>> only one bit should ever be 1. So to select parent 0, set
>>> the register (*) to 0x01, to select parent 1 set it 0x02,
>>> parent 2, 0x04, parent 3, 0x08, etc.
>>>
>>> Correct ?
>>>
>>> Assuming I got that right, then this bit of code in
>>> clk_mux_set_parent is wrong:
>>>
>>>                  if (mux->flags & CLK_MUX_INDEX_BIT)
>>>                          index = (1 << ffs(index));
>>>
>>> For an input index of 0, ffs returns 0, so we set the register
>>> to 0x01, ok.
>>>
>>> For an input index of 1, ffs returns 1, so we set the register
>>> to 0x02, ok.
>>>
>>> For an input index of 2, ffs returns 2, so we set the register
>>> to 0x04, ok.
>>>
>>> For an input index of 3, ffs returns 1, so we set the register
>>> to 0x02, not good!
>>>
>>> I believe this code should simply be:
>>>
>>>                  if (mux->flags & CLK_MUX_INDEX_BIT)
>>>                          index = 1 << index;
>>>
>>> I guess we've been getting away by this because the input index
>>> so far has never gotten above 2.
>>
>> Good catch. I've Cc'd Tero Kristo from TI as the TI SoCs are the only
>> other user of CLK_MUX_INDEX_BIT.
>>
>>>
>>> If I'm right about this, I can whip up a patch to fix this. Note
>>> I don't really have hardware to properly exercise this code-path
>>> AFAIK.
>>
>> Please do. I think your change is reasonable and Tero can scream if
>> something breaks for him.
> 
> TI SoC:s use a copy pasted version of the mux clock (with some additional tweaks), but we never set the CLK_MUX_INDEX_BIT flag, so we have been safe from this bug. Feel free to send a patch against the TI clock driver also if you want, I can ack that one. If not, I can craft a quick patch for this myself.

I've just send a fix for the generic version of this code, I'm sorry
but I do not have the time to also fix the TI code.

Regards,

Hans

Bug in clk_mux_set_parent for CLK_MUX_INDEX_BIT muxes ?

[Tero Kristo]

On 11/19/2014 03:49 PM, Hans de Goede wrote:
> Hi,
>
> On 11/18/2014 01:30 PM, Tero Kristo wrote:
>> On 11/17/2014 08:57 PM, Mike Turquette wrote:
>>> Quoting Hans de Goede (2014-11-16 04:34:46)
>>>> Hi,
>>>>
>>>> While looking at what I believe is a bug in the sunxi gmac clk code
>>>> (more on that later), I believe I've also found a bug in
>>>>
>>>> clk_mux_set_parent for the CLK_MUX_INDEX_BIT case.
>>>>
>>>> AFAIK if CLK_MUX_INDEX_BIT is set, then each bit turns
>>>> on / off a single parent, so theoretically multiple
>>>> parents could be enabled at the same time, but in practice
>>>> only one bit should ever be 1. So to select parent 0, set
>>>> the register (*) to 0x01, to select parent 1 set it 0x02,
>>>> parent 2, 0x04, parent 3, 0x08, etc.
>>>>
>>>> Correct ?
>>>>
>>>> Assuming I got that right, then this bit of code in
>>>> clk_mux_set_parent is wrong:
>>>>
>>>>                   if (mux->flags & CLK_MUX_INDEX_BIT)
>>>>                           index = (1 << ffs(index));
>>>>
>>>> For an input index of 0, ffs returns 0, so we set the register
>>>> to 0x01, ok.
>>>>
>>>> For an input index of 1, ffs returns 1, so we set the register
>>>> to 0x02, ok.
>>>>
>>>> For an input index of 2, ffs returns 2, so we set the register
>>>> to 0x04, ok.
>>>>
>>>> For an input index of 3, ffs returns 1, so we set the register
>>>> to 0x02, not good!
>>>>
>>>> I believe this code should simply be:
>>>>
>>>>                   if (mux->flags & CLK_MUX_INDEX_BIT)
>>>>                           index = 1 << index;
>>>>
>>>> I guess we've been getting away by this because the input index
>>>> so far has never gotten above 2.
>>>
>>> Good catch. I've Cc'd Tero Kristo from TI as the TI SoCs are the only
>>> other user of CLK_MUX_INDEX_BIT.
>>>
>>>>
>>>> If I'm right about this, I can whip up a patch to fix this. Note
>>>> I don't really have hardware to properly exercise this code-path
>>>> AFAIK.
>>>
>>> Please do. I think your change is reasonable and Tero can scream if
>>> something breaks for him.
>>
>> TI SoC:s use a copy pasted version of the mux clock (with some additional tweaks), but we never set the CLK_MUX_INDEX_BIT flag, so we have been safe from this bug. Feel free to send a patch against the TI clock driver also if you want, I can ack that one. If not, I can craft a quick patch for this myself.
>
> I've just send a fix for the generic version of this code, I'm sorry
> but I do not have the time to also fix the TI code.

Yep thats fine, I was mainly thinking if you want easy credit for that, 
I'll post a patch against that separately.

Thanks, Tero.