| Message ID | 20240117184111.62371-2-linux@fw-web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add reset controller to mt7988 infracfg | expand |
Il 17/01/24 19:41, Frank Wunderlich ha scritto: > 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> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
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 >
> 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. regards Frank
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. 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. Cheers, Angelo
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
> 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." and adding infracfg in commit title, if this is enough? 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.
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 + + #endif /* _DT_BINDINGS_RESET_CONTROLLER_MT7988 */ +