[v3,0/2] Add reset controller to mt7988 infracfg

  From: Frank Wunderlich <frank-w@public-files.de>

Infracfg on mt7988 supports reset controller function which is
needed to get lvts thermal working.

Patches are based on clk-next due to recently added mt7988 clock driver:
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git

changes:
 v3:
   - start resets on RST0 offset (LVTS is RST1)
   - rename offset constants with MT7988 prefix (else collision with reset.h)
 v2:
   - change value of constant to 0 from 9
   - add missing SoB and commit-message for binding-patch


Frank Wunderlich (2):
  dt-bindings: reset: mediatek: add MT7988 reset IDs
  clk: mediatek: add infracfg reset controller for mt7988

 drivers/clk/mediatek/clk-mt7988-infracfg.c    | 23 +++++++++++++++++++
 .../reset/mediatek,mt7988-resets.h            |  6 +++++
 2 files changed, 29 insertions(+)
  

On Wed, Jan 17, 2024 at 07:41:10PM +0100, Frank Wunderlich wrote:
> From: Frank Wunderlich <frank-w@public-files.de>
> 
> Add reset constants for using as index in driver and dts.
> 
> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> ---
> v3:
> - add pcie reset id as suggested by angelo
> 
> v2:
>  - add missing commit message and SoB
>  - change value of infrareset to 0
> ---
>  include/dt-bindings/reset/mediatek,mt7988-resets.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/include/dt-bindings/reset/mediatek,mt7988-resets.h b/include/dt-bindings/reset/mediatek,mt7988-resets.h
> index 493301971367..0eb152889a89 100644
> --- a/include/dt-bindings/reset/mediatek,mt7988-resets.h
> +++ b/include/dt-bindings/reset/mediatek,mt7988-resets.h
> @@ -10,4 +10,10 @@
>  /* ETHWARP resets */
>  #define MT7988_ETHWARP_RST_SWITCH		0
>  
> +/* INFRA resets */
> +#define MT7988_INFRA_RST0_PEXTP_MAC_SWRST	0
> +#define MT7988_INFRA_RST1_THERM_CTRL_SWRST	1

These are just "random" numbers, why not continue the numbering from the
ETHWARP?

> +
> +
>  #endif  /* _DT_BINDINGS_RESET_CONTROLLER_MT7988 */
> +
> -- 
> 2.34.1
>
  

On Fri, Jan 19, 2024 at 10:28:30AM +0100, AngeloGioacchino Del Regno wrote:
> Il 18/01/24 23:00, Frank Wunderlich ha scritto:
> > > Gesendet: Donnerstag, 18. Januar 2024 um 17:49 Uhr
> > > Von: "Conor Dooley" <conor@kernel.org>
> > > On Wed, Jan 17, 2024 at 07:41:10PM +0100, Frank Wunderlich wrote:
> > > > From: Frank Wunderlich <frank-w@public-files.de>
> > > > 
> > > > Add reset constants for using as index in driver and dts.
> > > > 
> > > > Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> > > > ---
> > > > v3:
> > > > - add pcie reset id as suggested by angelo
> > > > 
> > > > v2:
> > > >   - add missing commit message and SoB
> > > >   - change value of infrareset to 0
> > > > ---
> > > >   include/dt-bindings/reset/mediatek,mt7988-resets.h | 6 ++++++
> > > >   1 file changed, 6 insertions(+)
> > > > 
> > > > diff --git a/include/dt-bindings/reset/mediatek,mt7988-resets.h b/include/dt-bindings/reset/mediatek,mt7988-resets.h
> > > > index 493301971367..0eb152889a89 100644
> > > > --- a/include/dt-bindings/reset/mediatek,mt7988-resets.h
> > > > +++ b/include/dt-bindings/reset/mediatek,mt7988-resets.h
> > > > @@ -10,4 +10,10 @@
> > > >   /* ETHWARP resets */
> > > >   #define MT7988_ETHWARP_RST_SWITCH		0
> > > > 
> > > > +/* INFRA resets */
> > > > +#define MT7988_INFRA_RST0_PEXTP_MAC_SWRST	0
> > > > +#define MT7988_INFRA_RST1_THERM_CTRL_SWRST	1
> > > 
> > > These are just "random" numbers, why not continue the numbering from the
> > > ETHWARP?
> > 
> > i can do...basicly these consts are used in DTS and driver only as index.
> > 
> > @angelo what do you think? I though also in leaving some space to allow grouping RST0 and RST1
> > when more consts are added, else the numbers are mixed up.
> > 
> > so e.g. let RST0 start at 20 and RST1 at 40 (or even higher, because RST0 and RST1 can have up to 32 resets).
> > That will allow adding more reset constants between my values and having raising numbers.
> 
> The resets are organized on a per-reset-controller basis, so, the ETHWARP
> reset controller's first reset is RST_SWITCH, the second one is RST_something_else,
> etc. while the first reset of the INFRA reset controller is PEXTP_MAC_SWRST.
> 
> That's why ETHWARP has a reset index 0 and INFRA also starts at 0.
> I think that the numbering is good as it is, and having one driver start at index 5
> while the other starts at index 12 would only overcomplicate registering the resets
> in each driver, or waste bytes by making unnecessarily large arrays, for (imo) no
> good reason.
> 
> This is one header, but it should "in theory" be more than one... so we would have
> one for each hardware block - but that'd make the reset directory over-crowded, as
> other MediaTek SoCs have got even more resets in even more hardware blocks than the
> MT7988. That'd be something like ~4 reset headers per SoC (and will increase with
> newer ones)...
> ...and this is why we have one binding header for resets.

That's okay. The commit message leaves me, who clearly isn't a mediatek
guy, with no information as to why these are not one contiguous set.
IMO being for different reset controllers entirely is fine.

> On the topic of leaving space to allow grouping RST0/RST1: -> No. <-
> The indices have to start from zero and have to be sequential, with no holes.

Agreed.
  

Am 19. Januar 2024 18:04:36 MEZ schrieb Conor Dooley <conor@kernel.org>:
>On Fri, Jan 19, 2024 at 10:28:30AM +0100, AngeloGioacchino Del Regno wrote:
>>
>> The resets are organized on a per-reset-controller basis, so, the ETHWARP
>> reset controller's first reset is RST_SWITCH, the second one is RST_something_else,
>> etc. while the first reset of the INFRA reset controller is PEXTP_MAC_SWRST.
>> 
>> That's why ETHWARP has a reset index 0 and INFRA also starts at 0.
>> I think that the numbering is good as it is, and having one driver start at index 5
>> while the other starts at index 12 would only overcomplicate registering the resets
>> in each driver, or waste bytes by making unnecessarily large arrays, for (imo) no
>> good reason.
>> 
>> This is one header, but it should "in theory" be more than one... so we would have
>> one for each hardware block - but that'd make the reset directory over-crowded, as
>> other MediaTek SoCs have got even more resets in even more hardware blocks than the
>> MT7988. That'd be something like ~4 reset headers per SoC (and will increase with
>> newer ones)...
>> ...and this is why we have one binding header for resets.
>
>That's okay. The commit message leaves me, who clearly isn't a mediatek
>guy, with no information as to why these are not one contiguous set.
>IMO being for different reset controllers entirely is fine.
>
>> On the topic of leaving space to allow grouping RST0/RST1: -> No. <-
>> The indices have to start from zero and have to be sequential, with no holes.
>
>Agreed.

Hi,

Just a friendly reminder.

As far as i understood, Patches are fine so far and do not need any rework,right?

But i have not seen them picked up yet in linux-next.
regards Frank
  

On Thu, Feb 01, 2024 at 12:40:17PM +0100, Frank Wunderlich wrote:
> Am 19. Januar 2024 18:04:36 MEZ schrieb Conor Dooley <conor@kernel.org>:
> >On Fri, Jan 19, 2024 at 10:28:30AM +0100, AngeloGioacchino Del Regno wrote:
> >>
> >> The resets are organized on a per-reset-controller basis, so, the ETHWARP
> >> reset controller's first reset is RST_SWITCH, the second one is RST_something_else,
> >> etc. while the first reset of the INFRA reset controller is PEXTP_MAC_SWRST.
> >> 
> >> That's why ETHWARP has a reset index 0 and INFRA also starts at 0.
> >> I think that the numbering is good as it is, and having one driver start at index 5
> >> while the other starts at index 12 would only overcomplicate registering the resets
> >> in each driver, or waste bytes by making unnecessarily large arrays, for (imo) no
> >> good reason.
> >> 
> >> This is one header, but it should "in theory" be more than one... so we would have
> >> one for each hardware block - but that'd make the reset directory over-crowded, as
> >> other MediaTek SoCs have got even more resets in even more hardware blocks than the
> >> MT7988. That'd be something like ~4 reset headers per SoC (and will increase with
> >> newer ones)...
> >> ...and this is why we have one binding header for resets.
> >
> >That's okay. The commit message leaves me, who clearly isn't a mediatek
> >guy, with no information as to why these are not one contiguous set.
> >IMO being for different reset controllers entirely is fine.
> >
> >> On the topic of leaving space to allow grouping RST0/RST1: -> No. <-
> >> The indices have to start from zero and have to be sequential, with no holes.
> >
> >Agreed.
> 
> Hi,
> 
> Just a friendly reminder.
> 
> As far as i understood, Patches are fine so far and do not need any rework,right?

I suspect I was asking for a commit message that explains why these
numbers don't continue in sequence. With that,
Acked-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

> 
> But i have not seen them picked up yet in linux-next.
> regards Frank
  

On Thu, Feb 01, 2024 at 03:23:03PM +0100, Frank Wunderlich wrote:
> > Gesendet: Donnerstag, 01. Februar 2024 um 14:11 Uhr
> > Von: "Conor Dooley" <conor.dooley@microchip.com>
> > I suspect I was asking for a commit message that explains why these
> > numbers don't continue in sequence. With that,
> > Acked-by: Conor Dooley <conor.dooley@microchip.com>
> >
> > Cheers,
> > Conor.
> 
> OK, i would add this to commit message:
> 
> "Value is starting again from 0 because resets are used in another driver
> than existing constants."

s/driver/device/ and sure.