diff mbox series

clk: mediatek: mt8365: Fix inverted topclk operations

Message ID 20230511133226.913600-1-msp@baylibre.com
State New
Headers
Series clk: mediatek: mt8365: Fix inverted topclk operations |

Commit Message

Markus Schneider-Pargmann May 11, 2023, 1:32 p.m. UTC 
  The given operations are inverted for the wrong registers which makes
multiple of the mt8365 hardware units unusable. In my setup at least usb
did not work.

Fixed by swapping the operations with the inverted ones.

Reported-by: Alexandre Mergnat <amergnat@baylibre.com>
Fixes: 905b7430d3cc ("clk: mediatek: mt8365: Convert simple_gate to mtk_gate clocks")
Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
 drivers/clk/mediatek/clk-mt8365.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Alexandre Mergnat May 11, 2023, 1:37 p.m. UTC | #1 
On 11/05/2023 15:32, Markus Schneider-Pargmann wrote:
> The given operations are inverted for the wrong registers which makes
> multiple of the mt8365 hardware units unusable. In my setup at least usb
> did not work.
> 
> Fixed by swapping the operations with the inverted ones.

Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>
Tested-by: Alexandre Mergnat <amergnat@baylibre.com>
Matthias Brugger May 12, 2023, 11:51 a.m. UTC | #2 
On 11/05/2023 15:32, Markus Schneider-Pargmann wrote:
> The given operations are inverted for the wrong registers which makes
> multiple of the mt8365 hardware units unusable. In my setup at least usb
> did not work.
> 
> Fixed by swapping the operations with the inverted ones.
> 
> Reported-by: Alexandre Mergnat <amergnat@baylibre.com>
> Fixes: 905b7430d3cc ("clk: mediatek: mt8365: Convert simple_gate to mtk_gate clocks")
> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>

Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>

> ---
>   drivers/clk/mediatek/clk-mt8365.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/mediatek/clk-mt8365.c b/drivers/clk/mediatek/clk-mt8365.c
> index 6b4e193f648d..6d785ec5754d 100644
> --- a/drivers/clk/mediatek/clk-mt8365.c
> +++ b/drivers/clk/mediatek/clk-mt8365.c
> @@ -583,15 +583,15 @@ static const struct mtk_gate_regs top2_cg_regs = {
>   
>   #define GATE_TOP0(_id, _name, _parent, _shift)			\
>   	GATE_MTK(_id, _name, _parent, &top0_cg_regs,		\
> -		 _shift, &mtk_clk_gate_ops_no_setclr_inv)
> +		 _shift, &mtk_clk_gate_ops_no_setclr)
>   
>   #define GATE_TOP1(_id, _name, _parent, _shift)			\
>   	GATE_MTK(_id, _name, _parent, &top1_cg_regs,		\
> -		 _shift, &mtk_clk_gate_ops_no_setclr)
> +		 _shift, &mtk_clk_gate_ops_no_setclr_inv)
>   
>   #define GATE_TOP2(_id, _name, _parent, _shift)			\
>   	GATE_MTK(_id, _name, _parent, &top2_cg_regs,		\
> -		 _shift, &mtk_clk_gate_ops_no_setclr)
> +		 _shift, &mtk_clk_gate_ops_no_setclr_inv)
>   
>   static const struct mtk_gate top_clk_gates[] = {
>   	GATE_TOP0(CLK_TOP_CONN_32K, "conn_32k", "clk32k", 10),
AngeloGioacchino Del Regno May 15, 2023, 11:49 a.m. UTC | #3 
Il 12/05/23 13:51, Matthias Brugger ha scritto:
> 
> 
> On 11/05/2023 15:32, Markus Schneider-Pargmann wrote:
>> The given operations are inverted for the wrong registers which makes
>> multiple of the mt8365 hardware units unusable. In my setup at least usb
>> did not work.
>>
>> Fixed by swapping the operations with the inverted ones.

...with the not inverted ones, you mean!

Anyway,

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

>>
>> Reported-by: Alexandre Mergnat <amergnat@baylibre.com>
>> Fixes: 905b7430d3cc ("clk: mediatek: mt8365: Convert simple_gate to mtk_gate 
>> clocks")
>> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> 
> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
> 
>> ---
>>   drivers/clk/mediatek/clk-mt8365.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/clk/mediatek/clk-mt8365.c b/drivers/clk/mediatek/clk-mt8365.c
>> index 6b4e193f648d..6d785ec5754d 100644
>> --- a/drivers/clk/mediatek/clk-mt8365.c
>> +++ b/drivers/clk/mediatek/clk-mt8365.c
>> @@ -583,15 +583,15 @@ static const struct mtk_gate_regs top2_cg_regs = {
>>   #define GATE_TOP0(_id, _name, _parent, _shift)            \
>>       GATE_MTK(_id, _name, _parent, &top0_cg_regs,        \
>> -         _shift, &mtk_clk_gate_ops_no_setclr_inv)
>> +         _shift, &mtk_clk_gate_ops_no_setclr)
>>   #define GATE_TOP1(_id, _name, _parent, _shift)            \
>>       GATE_MTK(_id, _name, _parent, &top1_cg_regs,        \
>> -         _shift, &mtk_clk_gate_ops_no_setclr)
>> +         _shift, &mtk_clk_gate_ops_no_setclr_inv)
>>   #define GATE_TOP2(_id, _name, _parent, _shift)            \
>>       GATE_MTK(_id, _name, _parent, &top2_cg_regs,        \
>> -         _shift, &mtk_clk_gate_ops_no_setclr)
>> +         _shift, &mtk_clk_gate_ops_no_setclr_inv)
>>   static const struct mtk_gate top_clk_gates[] = {
>>       GATE_TOP0(CLK_TOP_CONN_32K, "conn_32k", "clk32k", 10),
Markus Schneider-Pargmann May 15, 2023, 2:01 p.m. UTC | #4 
On Mon, May 15, 2023 at 01:49:54PM +0200, AngeloGioacchino Del Regno wrote:
> Il 12/05/23 13:51, Matthias Brugger ha scritto:
> > 
> > 
> > On 11/05/2023 15:32, Markus Schneider-Pargmann wrote:
> > > The given operations are inverted for the wrong registers which makes
> > > multiple of the mt8365 hardware units unusable. In my setup at least usb
> > > did not work.
> > > 
> > > Fixed by swapping the operations with the inverted ones.
> 
> ...with the not inverted ones, you mean!

Actually now I am not sure how to express it correctly, maybe I should
have just left that sentence out of the commit message.

I meant replacing the no_setclr_inv with no_setclr for TOP0 and no_setclr with
no_setclr_inv for TOP1/2.

Best,
Markus

> 
> Anyway,
> 
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> 
> > > 
> > > Reported-by: Alexandre Mergnat <amergnat@baylibre.com>
> > > Fixes: 905b7430d3cc ("clk: mediatek: mt8365: Convert simple_gate to
> > > mtk_gate clocks")
> > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> > 
> > Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
> > 
> > > ---
> > >   drivers/clk/mediatek/clk-mt8365.c | 6 +++---
> > >   1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/clk/mediatek/clk-mt8365.c b/drivers/clk/mediatek/clk-mt8365.c
> > > index 6b4e193f648d..6d785ec5754d 100644
> > > --- a/drivers/clk/mediatek/clk-mt8365.c
> > > +++ b/drivers/clk/mediatek/clk-mt8365.c
> > > @@ -583,15 +583,15 @@ static const struct mtk_gate_regs top2_cg_regs = {
> > >   #define GATE_TOP0(_id, _name, _parent, _shift)            \
> > >       GATE_MTK(_id, _name, _parent, &top0_cg_regs,        \
> > > -         _shift, &mtk_clk_gate_ops_no_setclr_inv)
> > > +         _shift, &mtk_clk_gate_ops_no_setclr)
> > >   #define GATE_TOP1(_id, _name, _parent, _shift)            \
> > >       GATE_MTK(_id, _name, _parent, &top1_cg_regs,        \
> > > -         _shift, &mtk_clk_gate_ops_no_setclr)
> > > +         _shift, &mtk_clk_gate_ops_no_setclr_inv)
> > >   #define GATE_TOP2(_id, _name, _parent, _shift)            \
> > >       GATE_MTK(_id, _name, _parent, &top2_cg_regs,        \
> > > -         _shift, &mtk_clk_gate_ops_no_setclr)
> > > +         _shift, &mtk_clk_gate_ops_no_setclr_inv)
> > >   static const struct mtk_gate top_clk_gates[] = {
> > >       GATE_TOP0(CLK_TOP_CONN_32K, "conn_32k", "clk32k", 10),
> 
>
AngeloGioacchino Del Regno May 15, 2023, 2:17 p.m. UTC | #5 
Il 15/05/23 16:01, Markus Schneider-Pargmann ha scritto:
> On Mon, May 15, 2023 at 01:49:54PM +0200, AngeloGioacchino Del Regno wrote:
>> Il 12/05/23 13:51, Matthias Brugger ha scritto:
>>>
>>>
>>> On 11/05/2023 15:32, Markus Schneider-Pargmann wrote:
>>>> The given operations are inverted for the wrong registers which makes
>>>> multiple of the mt8365 hardware units unusable. In my setup at least usb
>>>> did not work.
>>>>
>>>> Fixed by swapping the operations with the inverted ones.
>>
>> ...with the not inverted ones, you mean!
> 
> Actually now I am not sure how to express it correctly, maybe I should
> have just left that sentence out of the commit message.
> 
> I meant replacing the no_setclr_inv with no_setclr for TOP0 and no_setclr with
> no_setclr_inv for TOP1/2.
> 

Yeah, the sentence may be a bit confusing, but you're actually inverting the
inverted ones and the non-inverted ones.

It's fine, anyway, this commit can get picked as-is imo :-D

> Best,
> Markus
> 
>>
>> Anyway,
>>
>> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>>
>>>>
>>>> Reported-by: Alexandre Mergnat <amergnat@baylibre.com>
>>>> Fixes: 905b7430d3cc ("clk: mediatek: mt8365: Convert simple_gate to
>>>> mtk_gate clocks")
>>>> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
>>>
>>> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
>>>
>>>> ---
>>>>    drivers/clk/mediatek/clk-mt8365.c | 6 +++---
>>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/clk/mediatek/clk-mt8365.c b/drivers/clk/mediatek/clk-mt8365.c
>>>> index 6b4e193f648d..6d785ec5754d 100644
>>>> --- a/drivers/clk/mediatek/clk-mt8365.c
>>>> +++ b/drivers/clk/mediatek/clk-mt8365.c
>>>> @@ -583,15 +583,15 @@ static const struct mtk_gate_regs top2_cg_regs = {
>>>>    #define GATE_TOP0(_id, _name, _parent, _shift)            \
>>>>        GATE_MTK(_id, _name, _parent, &top0_cg_regs,        \
>>>> -         _shift, &mtk_clk_gate_ops_no_setclr_inv)
>>>> +         _shift, &mtk_clk_gate_ops_no_setclr)
>>>>    #define GATE_TOP1(_id, _name, _parent, _shift)            \
>>>>        GATE_MTK(_id, _name, _parent, &top1_cg_regs,        \
>>>> -         _shift, &mtk_clk_gate_ops_no_setclr)
>>>> +         _shift, &mtk_clk_gate_ops_no_setclr_inv)
>>>>    #define GATE_TOP2(_id, _name, _parent, _shift)            \
>>>>        GATE_MTK(_id, _name, _parent, &top2_cg_regs,        \
>>>> -         _shift, &mtk_clk_gate_ops_no_setclr)
>>>> +         _shift, &mtk_clk_gate_ops_no_setclr_inv)
>>>>    static const struct mtk_gate top_clk_gates[] = {
>>>>        GATE_TOP0(CLK_TOP_CONN_32K, "conn_32k", "clk32k", 10),
>>
>>
Markus Schneider-Pargmann May 23, 2023, 11:46 a.m. UTC | #6 
Hi,

just wanted to ask if I need to do something specific for it to go into
a rc? Sorry if I missed doing something for that, I haven't had to fix
something in an rc that often before.

Best,
Markus

On Thu, May 11, 2023 at 03:32:26PM +0200, Markus Schneider-Pargmann wrote:
> The given operations are inverted for the wrong registers which makes
> multiple of the mt8365 hardware units unusable. In my setup at least usb
> did not work.
> 
> Fixed by swapping the operations with the inverted ones.
> 
> Reported-by: Alexandre Mergnat <amergnat@baylibre.com>
> Fixes: 905b7430d3cc ("clk: mediatek: mt8365: Convert simple_gate to mtk_gate clocks")
> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> ---
>  drivers/clk/mediatek/clk-mt8365.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/mediatek/clk-mt8365.c b/drivers/clk/mediatek/clk-mt8365.c
> index 6b4e193f648d..6d785ec5754d 100644
> --- a/drivers/clk/mediatek/clk-mt8365.c
> +++ b/drivers/clk/mediatek/clk-mt8365.c
> @@ -583,15 +583,15 @@ static const struct mtk_gate_regs top2_cg_regs = {
>  
>  #define GATE_TOP0(_id, _name, _parent, _shift)			\
>  	GATE_MTK(_id, _name, _parent, &top0_cg_regs,		\
> -		 _shift, &mtk_clk_gate_ops_no_setclr_inv)
> +		 _shift, &mtk_clk_gate_ops_no_setclr)
>  
>  #define GATE_TOP1(_id, _name, _parent, _shift)			\
>  	GATE_MTK(_id, _name, _parent, &top1_cg_regs,		\
> -		 _shift, &mtk_clk_gate_ops_no_setclr)
> +		 _shift, &mtk_clk_gate_ops_no_setclr_inv)
>  
>  #define GATE_TOP2(_id, _name, _parent, _shift)			\
>  	GATE_MTK(_id, _name, _parent, &top2_cg_regs,		\
> -		 _shift, &mtk_clk_gate_ops_no_setclr)
> +		 _shift, &mtk_clk_gate_ops_no_setclr_inv)
>  
>  static const struct mtk_gate top_clk_gates[] = {
>  	GATE_TOP0(CLK_TOP_CONN_32K, "conn_32k", "clk32k", 10),
> -- 
> 2.40.1
>
Kevin Hilman May 26, 2023, 4:45 p.m. UTC | #7 
Markus Schneider-Pargmann <msp@baylibre.com> writes:

> just wanted to ask if I need to do something specific for it to go into
> a rc? Sorry if I missed doing something for that, I haven't had to fix
> something in an rc that often before.

I think the  Fixes tag is enough to indicate that.

Steven, is there still time for this fix be queued up for v6.4-rc?

Thanks,

Kevin

> On Thu, May 11, 2023 at 03:32:26PM +0200, Markus Schneider-Pargmann wrote:
>> The given operations are inverted for the wrong registers which makes
>> multiple of the mt8365 hardware units unusable. In my setup at least usb
>> did not work.
>> 
>> Fixed by swapping the operations with the inverted ones.
>> 
>> Reported-by: Alexandre Mergnat <amergnat@baylibre.com>
>> Fixes: 905b7430d3cc ("clk: mediatek: mt8365: Convert simple_gate to mtk_gate clocks")
>> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
>> ---
>>  drivers/clk/mediatek/clk-mt8365.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/clk/mediatek/clk-mt8365.c b/drivers/clk/mediatek/clk-mt8365.c
>> index 6b4e193f648d..6d785ec5754d 100644
>> --- a/drivers/clk/mediatek/clk-mt8365.c
>> +++ b/drivers/clk/mediatek/clk-mt8365.c
>> @@ -583,15 +583,15 @@ static const struct mtk_gate_regs top2_cg_regs = {
>>  
>>  #define GATE_TOP0(_id, _name, _parent, _shift)			\
>>  	GATE_MTK(_id, _name, _parent, &top0_cg_regs,		\
>> -		 _shift, &mtk_clk_gate_ops_no_setclr_inv)
>> +		 _shift, &mtk_clk_gate_ops_no_setclr)
>>  
>>  #define GATE_TOP1(_id, _name, _parent, _shift)			\
>>  	GATE_MTK(_id, _name, _parent, &top1_cg_regs,		\
>> -		 _shift, &mtk_clk_gate_ops_no_setclr)
>> +		 _shift, &mtk_clk_gate_ops_no_setclr_inv)
>>  
>>  #define GATE_TOP2(_id, _name, _parent, _shift)			\
>>  	GATE_MTK(_id, _name, _parent, &top2_cg_regs,		\
>> -		 _shift, &mtk_clk_gate_ops_no_setclr)
>> +		 _shift, &mtk_clk_gate_ops_no_setclr_inv)
>>  
>>  static const struct mtk_gate top_clk_gates[] = {
>>  	GATE_TOP0(CLK_TOP_CONN_32K, "conn_32k", "clk32k", 10),
>> -- 
>> 2.40.1
>>
Kevin Hilman June 9, 2023, 6:21 p.m. UTC | #8 
Kevin Hilman <khilman@kernel.org> writes:

> Markus Schneider-Pargmann <msp@baylibre.com> writes:
>
>> just wanted to ask if I need to do something specific for it to go into
>> a rc? Sorry if I missed doing something for that, I haven't had to fix
>> something in an rc that often before.
>
> I think the  Fixes tag is enough to indicate that.
>
> Steven, is there still time for this fix be queued up for v6.4-rc?

Steven: gentle reminder ping.
Stephen Boyd June 13, 2023, 1:11 a.m. UTC | #9 
Quoting Kevin Hilman (2023-06-09 11:21:30)
> Kevin Hilman <khilman@kernel.org> writes:
> 
> > Markus Schneider-Pargmann <msp@baylibre.com> writes:
> >
> >> just wanted to ask if I need to do something specific for it to go into
> >> a rc? Sorry if I missed doing something for that, I haven't had to fix
> >> something in an rc that often before.
> >
> > I think the  Fixes tag is enough to indicate that.
> >
> > Steven, is there still time for this fix be queued up for v6.4-rc?
> 
> Steven: gentle reminder ping.

Who's Steven :)
Stephen Boyd June 13, 2023, 1:13 a.m. UTC | #10 
Quoting Markus Schneider-Pargmann (2023-05-11 06:32:26)
> The given operations are inverted for the wrong registers which makes
> multiple of the mt8365 hardware units unusable. In my setup at least usb
> did not work.
> 
> Fixed by swapping the operations with the inverted ones.
> 
> Reported-by: Alexandre Mergnat <amergnat@baylibre.com>

Checkpatch complains now that there isn't a Closes: tag here.

> Fixes: 905b7430d3cc ("clk: mediatek: mt8365: Convert simple_gate to mtk_gate clocks")
> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> ---

Applied to clk-fixes
Markus Schneider-Pargmann June 13, 2023, 8:21 a.m. UTC | #11 
Hi Stephen,

On Mon, Jun 12, 2023 at 06:13:10PM -0700, Stephen Boyd wrote:
> Quoting Markus Schneider-Pargmann (2023-05-11 06:32:26)
> > The given operations are inverted for the wrong registers which makes
> > multiple of the mt8365 hardware units unusable. In my setup at least usb
> > did not work.
> > 
> > Fixed by swapping the operations with the inverted ones.
> > 
> > Reported-by: Alexandre Mergnat <amergnat@baylibre.com>
> 
> Checkpatch complains now that there isn't a Closes: tag here.

Alexandre reported this internally, so there is nothing I could point to
for a Closes tag.

> 
> > Fixes: 905b7430d3cc ("clk: mediatek: mt8365: Convert simple_gate to mtk_gate clocks")
> > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> > ---
> 
> Applied to clk-fixes

Thank you.

Best,
Markus
Kevin Hilman June 13, 2023, 3:46 p.m. UTC | #12 
Stephen Boyd <sboyd@kernel.org> writes:

> Quoting Kevin Hilman (2023-06-09 11:21:30)
>> Kevin Hilman <khilman@kernel.org> writes:
>> 
>> > Markus Schneider-Pargmann <msp@baylibre.com> writes:
>> >
>> >> just wanted to ask if I need to do something specific for it to go into
>> >> a rc? Sorry if I missed doing something for that, I haven't had to fix
>> >> something in an rc that often before.
>> >
>> > I think the  Fixes tag is enough to indicate that.
>> >
>> > Steven, is there still time for this fix be queued up for v6.4-rc?
>> 
>> Steven: gentle reminder ping.
>
> Who's Steven :)

Sorry, I confused you with Steven Spielberg.  I bet that happens to you
a lot on the mailing list. ;)   (either that, or I can't spell.)

Thanks for queuing this up Stephen. 

Kevin
diff mbox series

Patch

diff --git a/drivers/clk/mediatek/clk-mt8365.c b/drivers/clk/mediatek/clk-mt8365.c
index 6b4e193f648d..6d785ec5754d 100644
--- a/drivers/clk/mediatek/clk-mt8365.c
+++ b/drivers/clk/mediatek/clk-mt8365.c
@@ -583,15 +583,15 @@  static const struct mtk_gate_regs top2_cg_regs = {
 
 #define GATE_TOP0(_id, _name, _parent, _shift)			\
 	GATE_MTK(_id, _name, _parent, &top0_cg_regs,		\
-		 _shift, &mtk_clk_gate_ops_no_setclr_inv)
+		 _shift, &mtk_clk_gate_ops_no_setclr)
 
 #define GATE_TOP1(_id, _name, _parent, _shift)			\
 	GATE_MTK(_id, _name, _parent, &top1_cg_regs,		\
-		 _shift, &mtk_clk_gate_ops_no_setclr)
+		 _shift, &mtk_clk_gate_ops_no_setclr_inv)
 
 #define GATE_TOP2(_id, _name, _parent, _shift)			\
 	GATE_MTK(_id, _name, _parent, &top2_cg_regs,		\
-		 _shift, &mtk_clk_gate_ops_no_setclr)
+		 _shift, &mtk_clk_gate_ops_no_setclr_inv)
 
 static const struct mtk_gate top_clk_gates[] = {
 	GATE_TOP0(CLK_TOP_CONN_32K, "conn_32k", "clk32k", 10),