| Message ID | 20231120212037.911774-1-peter.griffin@linaro.org |
|---|---|
| Headers | show |
| Series | Add minimal Tensor/GS101 SoC support and Oriole/Pixel6 board | expand |
Hi Guenter, Thanks for the review. On Mon, 20 Nov 2023 at 22:00, Guenter Roeck <linux@roeck-us.net> wrote: > > On 11/20/23 13:20, Peter Griffin wrote: > > The WDT uses the CPU core signal DBGACK to determine whether the SoC > > is running in debug mode or not. If the DBGACK signal is asserted and > > DBGACK_MASK is enabled, then WDT output and interrupt is masked. > > > > Presence of the DBGACK_MASK bit is determined by adding a new > > QUIRK_HAS_DBGACK_BIT quirk. Currently only gs101 SoC is known to have > > the DBGACK_MASK bit so add the quirk to drv_data_gs101_cl1 and > > drv_data_gs101_cl1 quirks. > > > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > > --- > > drivers/watchdog/s3c2410_wdt.c | 32 +++++++++++++++++++++++++++----- > > 1 file changed, 27 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c > > index 08b8c57dd812..ed561deeeed9 100644 > > --- a/drivers/watchdog/s3c2410_wdt.c > > +++ b/drivers/watchdog/s3c2410_wdt.c > > @@ -34,9 +34,10 @@ > > > > #define S3C2410_WTCNT_MAXCNT 0xffff > > > > -#define S3C2410_WTCON_RSTEN (1 << 0) > > -#define S3C2410_WTCON_INTEN (1 << 2) > > -#define S3C2410_WTCON_ENABLE (1 << 5) > > +#define S3C2410_WTCON_RSTEN (1 << 0) > > +#define S3C2410_WTCON_INTEN (1 << 2) > > +#define S3C2410_WTCON_ENABLE (1 << 5) > > +#define S3C2410_WTCON_DBGACK_MASK (1 << 16) > > > > #define S3C2410_WTCON_DIV16 (0 << 3) > > #define S3C2410_WTCON_DIV32 (1 << 3) > > @@ -107,12 +108,16 @@ > > * %QUIRK_HAS_PMU_CNT_EN: PMU block has some register (e.g. CLUSTERx_NONCPU_OUT) > > * with "watchdog counter enable" bit. That bit should be set to make watchdog > > * counter running. > > + * > > + * %QUIRK_HAS_DBGACK_BIT: WTCON register has DBGACK_MASK bit. Enables masking > > + * WDT interrupt and reset request according to CPU core DBGACK signal. > > This is a bit difficult to understand. I _think_ it means that the DBGACK_MASK bit > has to be set to be able to trigger interrupt and reset requests. Not quite, it is a bit that controls masking the watchdog outputs when the SoC is in debug mode. > "masking" normally refers to disabling something (at least in interrupt context). > "Enables masking WDT interrupt" sounds like the bit has to be set in order to > be able to disable interupts, and the code below suggests that the bit has to be > set for the driver to work. Is that the case ? It might make sense to explain this > a bit further. Maybe I explained it more clearly in the commit message than the comment "The WDT uses the CPU core signal DBGACK to determine whether the SoC is running in debug mode or not. If the DBGACK signal is asserted and DBGACK_MASK is enabled, then WDT output and interrupt is masked." Is that any clearer? Or maybe simpler again "Enabling DBGACK_MASK bit masks the watchdog outputs when the SoC is in debug mode. Debug mode is determined by the DBGACK CPU signal." Let me know what you think is the clearest and most succinct and I can update the comment. > > > */ > > #define QUIRK_HAS_WTCLRINT_REG (1 << 0) > > #define QUIRK_HAS_PMU_MASK_RESET (1 << 1) > > #define QUIRK_HAS_PMU_RST_STAT (1 << 2) > > #define QUIRK_HAS_PMU_AUTO_DISABLE (1 << 3) > > #define QUIRK_HAS_PMU_CNT_EN (1 << 4) > > +#define QUIRK_HAS_DBGACK_BIT (1 << 5) > > > > /* These quirks require that we have a PMU register map */ > > #define QUIRKS_HAVE_PMUREG \ > > @@ -279,7 +284,7 @@ static const struct s3c2410_wdt_variant drv_data_gs101_cl0 = { > > .cnt_en_reg = GS_CLUSTER0_NONCPU_OUT, > > .cnt_en_bit = 8, > > .quirks = QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_MASK_RESET | QUIRK_HAS_PMU_CNT_EN | > > - QUIRK_HAS_WTCLRINT_REG, > > + QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_DBGACK_BIT, > > }; > > > > static const struct s3c2410_wdt_variant drv_data_gs101_cl1 = { > > @@ -291,7 +296,7 @@ static const struct s3c2410_wdt_variant drv_data_gs101_cl1 = { > > .cnt_en_reg = GS_CLUSTER1_NONCPU_OUT, > > .cnt_en_bit = 7, > > .quirks = QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_MASK_RESET | QUIRK_HAS_PMU_CNT_EN | > > - QUIRK_HAS_WTCLRINT_REG, > > + QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_DBGACK_BIT, > > }; > > > > static const struct of_device_id s3c2410_wdt_match[] = { > > @@ -408,6 +413,21 @@ static int s3c2410wdt_enable(struct s3c2410_wdt *wdt, bool en) > > return 0; > > } > > > > +static void s3c2410wdt_mask_dbgack(struct s3c2410_wdt *wdt, bool mask) > > I think I must be missing something. This is only ever called with mask==true, > meaning the bit, if present, is always set. > > Why not call the function s3c2410wdt_set_dbgack() and drop the unnecessary > parameter ? I can update like you suggest, it would simplify the logic a little bit. regards, Peter.
On 11/20/23 14:45, Peter Griffin wrote: > Hi Guenter, > > Thanks for the review. > > On Mon, 20 Nov 2023 at 22:00, Guenter Roeck <linux@roeck-us.net> wrote: >> >> On 11/20/23 13:20, Peter Griffin wrote: >>> The WDT uses the CPU core signal DBGACK to determine whether the SoC >>> is running in debug mode or not. If the DBGACK signal is asserted and >>> DBGACK_MASK is enabled, then WDT output and interrupt is masked. >>> >>> Presence of the DBGACK_MASK bit is determined by adding a new >>> QUIRK_HAS_DBGACK_BIT quirk. Currently only gs101 SoC is known to have >>> the DBGACK_MASK bit so add the quirk to drv_data_gs101_cl1 and >>> drv_data_gs101_cl1 quirks. >>> >>> Signed-off-by: Peter Griffin <peter.griffin@linaro.org> >>> --- >>> drivers/watchdog/s3c2410_wdt.c | 32 +++++++++++++++++++++++++++----- >>> 1 file changed, 27 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c >>> index 08b8c57dd812..ed561deeeed9 100644 >>> --- a/drivers/watchdog/s3c2410_wdt.c >>> +++ b/drivers/watchdog/s3c2410_wdt.c >>> @@ -34,9 +34,10 @@ >>> >>> #define S3C2410_WTCNT_MAXCNT 0xffff >>> >>> -#define S3C2410_WTCON_RSTEN (1 << 0) >>> -#define S3C2410_WTCON_INTEN (1 << 2) >>> -#define S3C2410_WTCON_ENABLE (1 << 5) >>> +#define S3C2410_WTCON_RSTEN (1 << 0) >>> +#define S3C2410_WTCON_INTEN (1 << 2) >>> +#define S3C2410_WTCON_ENABLE (1 << 5) >>> +#define S3C2410_WTCON_DBGACK_MASK (1 << 16) >>> >>> #define S3C2410_WTCON_DIV16 (0 << 3) >>> #define S3C2410_WTCON_DIV32 (1 << 3) >>> @@ -107,12 +108,16 @@ >>> * %QUIRK_HAS_PMU_CNT_EN: PMU block has some register (e.g. CLUSTERx_NONCPU_OUT) >>> * with "watchdog counter enable" bit. That bit should be set to make watchdog >>> * counter running. >>> + * >>> + * %QUIRK_HAS_DBGACK_BIT: WTCON register has DBGACK_MASK bit. Enables masking >>> + * WDT interrupt and reset request according to CPU core DBGACK signal. >> >> This is a bit difficult to understand. I _think_ it means that the DBGACK_MASK bit >> has to be set to be able to trigger interrupt and reset requests. > > Not quite, it is a bit that controls masking the watchdog outputs when the SoC > is in debug mode. > >> "masking" normally refers to disabling something (at least in interrupt context). >> "Enables masking WDT interrupt" sounds like the bit has to be set in order to >> be able to disable interupts, and the code below suggests that the bit has to be >> set for the driver to work. Is that the case ? It might make sense to explain this >> a bit further. > > Maybe I explained it more clearly in the commit message than the comment > > "The WDT uses the CPU core signal DBGACK to determine whether the SoC > is running in debug mode or not. If the DBGACK signal is asserted and > DBGACK_MASK is enabled, then WDT output and interrupt is masked." > > Is that any clearer? Or maybe simpler again > > "Enabling DBGACK_MASK bit masks the watchdog outputs when the SoC is > in debug mode. Debug mode is determined by the DBGACK CPU signal." > > Let me know what you think is the clearest and most succinct and I can > update the comment. > You are still using the term "masked" which I think just hides what the code is really doing. Why not just say "disable" ? "Setting the DBGACK_MASK bit disables the watchdog outputs when the SoC is in debug mode. Debug mode is determined by the DBGACK CPU signal." That seems to be much clearer to me, though I think there should still be a comment along the line of "disable watchdog output if CPU is in debug mode" in the code. That doesn't really explain _why_ the watchdog is disabled in this mode, but at least it makes it obvious what is happening. >> >>> */ >>> #define QUIRK_HAS_WTCLRINT_REG (1 << 0) >>> #define QUIRK_HAS_PMU_MASK_RESET (1 << 1) >>> #define QUIRK_HAS_PMU_RST_STAT (1 << 2) >>> #define QUIRK_HAS_PMU_AUTO_DISABLE (1 << 3) >>> #define QUIRK_HAS_PMU_CNT_EN (1 << 4) >>> +#define QUIRK_HAS_DBGACK_BIT (1 << 5) >>> >>> /* These quirks require that we have a PMU register map */ >>> #define QUIRKS_HAVE_PMUREG \ >>> @@ -279,7 +284,7 @@ static const struct s3c2410_wdt_variant drv_data_gs101_cl0 = { >>> .cnt_en_reg = GS_CLUSTER0_NONCPU_OUT, >>> .cnt_en_bit = 8, >>> .quirks = QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_MASK_RESET | QUIRK_HAS_PMU_CNT_EN | >>> - QUIRK_HAS_WTCLRINT_REG, >>> + QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_DBGACK_BIT, >>> }; >>> >>> static const struct s3c2410_wdt_variant drv_data_gs101_cl1 = { >>> @@ -291,7 +296,7 @@ static const struct s3c2410_wdt_variant drv_data_gs101_cl1 = { >>> .cnt_en_reg = GS_CLUSTER1_NONCPU_OUT, >>> .cnt_en_bit = 7, >>> .quirks = QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_MASK_RESET | QUIRK_HAS_PMU_CNT_EN | >>> - QUIRK_HAS_WTCLRINT_REG, >>> + QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_DBGACK_BIT, >>> }; >>> >>> static const struct of_device_id s3c2410_wdt_match[] = { >>> @@ -408,6 +413,21 @@ static int s3c2410wdt_enable(struct s3c2410_wdt *wdt, bool en) >>> return 0; >>> } >>> >>> +static void s3c2410wdt_mask_dbgack(struct s3c2410_wdt *wdt, bool mask) >> >> I think I must be missing something. This is only ever called with mask==true, >> meaning the bit, if present, is always set. >> >> Why not call the function s3c2410wdt_set_dbgack() and drop the unnecessary >> parameter ? > > I can update like you suggest, it would simplify the logic a little bit. > Please do. Thanks, Guenter
Hi Guenter, On Mon, 20 Nov 2023 at 23:03, Guenter Roeck <linux@roeck-us.net> wrote: > > On 11/20/23 14:45, Peter Griffin wrote: > > Hi Guenter, > > > > Thanks for the review. > > > > On Mon, 20 Nov 2023 at 22:00, Guenter Roeck <linux@roeck-us.net> wrote: > >> > >> On 11/20/23 13:20, Peter Griffin wrote: > >>> The WDT uses the CPU core signal DBGACK to determine whether the SoC > >>> is running in debug mode or not. If the DBGACK signal is asserted and > >>> DBGACK_MASK is enabled, then WDT output and interrupt is masked. > >>> > >>> Presence of the DBGACK_MASK bit is determined by adding a new > >>> QUIRK_HAS_DBGACK_BIT quirk. Currently only gs101 SoC is known to have > >>> the DBGACK_MASK bit so add the quirk to drv_data_gs101_cl1 and > >>> drv_data_gs101_cl1 quirks. > >>> > >>> Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > >>> --- > >>> drivers/watchdog/s3c2410_wdt.c | 32 +++++++++++++++++++++++++++----- > >>> 1 file changed, 27 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c > >>> index 08b8c57dd812..ed561deeeed9 100644 > >>> --- a/drivers/watchdog/s3c2410_wdt.c > >>> +++ b/drivers/watchdog/s3c2410_wdt.c > >>> @@ -34,9 +34,10 @@ > >>> > >>> #define S3C2410_WTCNT_MAXCNT 0xffff > >>> > >>> -#define S3C2410_WTCON_RSTEN (1 << 0) > >>> -#define S3C2410_WTCON_INTEN (1 << 2) > >>> -#define S3C2410_WTCON_ENABLE (1 << 5) > >>> +#define S3C2410_WTCON_RSTEN (1 << 0) > >>> +#define S3C2410_WTCON_INTEN (1 << 2) > >>> +#define S3C2410_WTCON_ENABLE (1 << 5) > >>> +#define S3C2410_WTCON_DBGACK_MASK (1 << 16) > >>> > >>> #define S3C2410_WTCON_DIV16 (0 << 3) > >>> #define S3C2410_WTCON_DIV32 (1 << 3) > >>> @@ -107,12 +108,16 @@ > >>> * %QUIRK_HAS_PMU_CNT_EN: PMU block has some register (e.g. CLUSTERx_NONCPU_OUT) > >>> * with "watchdog counter enable" bit. That bit should be set to make watchdog > >>> * counter running. > >>> + * > >>> + * %QUIRK_HAS_DBGACK_BIT: WTCON register has DBGACK_MASK bit. Enables masking > >>> + * WDT interrupt and reset request according to CPU core DBGACK signal. > >> > >> This is a bit difficult to understand. I _think_ it means that the DBGACK_MASK bit > >> has to be set to be able to trigger interrupt and reset requests. > > > > Not quite, it is a bit that controls masking the watchdog outputs when the SoC > > is in debug mode. > > > >> "masking" normally refers to disabling something (at least in interrupt context). > >> "Enables masking WDT interrupt" sounds like the bit has to be set in order to > >> be able to disable interupts, and the code below suggests that the bit has to be > >> set for the driver to work. Is that the case ? It might make sense to explain this > >> a bit further. > > > > Maybe I explained it more clearly in the commit message than the comment > > > > "The WDT uses the CPU core signal DBGACK to determine whether the SoC > > is running in debug mode or not. If the DBGACK signal is asserted and > > DBGACK_MASK is enabled, then WDT output and interrupt is masked." > > > > Is that any clearer? Or maybe simpler again > > > > "Enabling DBGACK_MASK bit masks the watchdog outputs when the SoC is > > in debug mode. Debug mode is determined by the DBGACK CPU signal." > > > > Let me know what you think is the clearest and most succinct and I can > > update the comment. > > > > You are still using the term "masked" which I think just hides what > the code is really doing. Why not just say "disable" ? The reason for using the "masked" terminology was that is what the Watchdog IP TRM uses throughout to describe the feature. But I agree just saying disable is clearer. > > "Setting the DBGACK_MASK bit disables the watchdog outputs when the SoC is > in debug mode. Debug mode is determined by the DBGACK CPU signal." > > That seems to be much clearer to me, though I think there should still > be a comment along the line of "disable watchdog output if CPU is in > debug mode" in the code. I will update the quirk comment with the less ambiguous wording and also add an extra comment like you suggest. > > That doesn't really explain _why_ the watchdog is disabled in this mode, > but at least it makes it obvious what is happening. > > >> > >>> */ > >>> #define QUIRK_HAS_WTCLRINT_REG (1 << 0) > >>> #define QUIRK_HAS_PMU_MASK_RESET (1 << 1) > >>> #define QUIRK_HAS_PMU_RST_STAT (1 << 2) > >>> #define QUIRK_HAS_PMU_AUTO_DISABLE (1 << 3) > >>> #define QUIRK_HAS_PMU_CNT_EN (1 << 4) > >>> +#define QUIRK_HAS_DBGACK_BIT (1 << 5) > >>> > >>> /* These quirks require that we have a PMU register map */ > >>> #define QUIRKS_HAVE_PMUREG \ > >>> @@ -279,7 +284,7 @@ static const struct s3c2410_wdt_variant drv_data_gs101_cl0 = { > >>> .cnt_en_reg = GS_CLUSTER0_NONCPU_OUT, > >>> .cnt_en_bit = 8, > >>> .quirks = QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_MASK_RESET | QUIRK_HAS_PMU_CNT_EN | > >>> - QUIRK_HAS_WTCLRINT_REG, > >>> + QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_DBGACK_BIT, > >>> }; > >>> > >>> static const struct s3c2410_wdt_variant drv_data_gs101_cl1 = { > >>> @@ -291,7 +296,7 @@ static const struct s3c2410_wdt_variant drv_data_gs101_cl1 = { > >>> .cnt_en_reg = GS_CLUSTER1_NONCPU_OUT, > >>> .cnt_en_bit = 7, > >>> .quirks = QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_MASK_RESET | QUIRK_HAS_PMU_CNT_EN | > >>> - QUIRK_HAS_WTCLRINT_REG, > >>> + QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_DBGACK_BIT, > >>> }; > >>> > >>> static const struct of_device_id s3c2410_wdt_match[] = { > >>> @@ -408,6 +413,21 @@ static int s3c2410wdt_enable(struct s3c2410_wdt *wdt, bool en) > >>> return 0; > >>> } > >>> > >>> +static void s3c2410wdt_mask_dbgack(struct s3c2410_wdt *wdt, bool mask) > >> > >> I think I must be missing something. This is only ever called with mask==true, > >> meaning the bit, if present, is always set. > >> > >> Why not call the function s3c2410wdt_set_dbgack() and drop the unnecessary > >> parameter ? > > > > I can update like you suggest, it would simplify the logic a little bit. > > > > Please do. Will do Thanks, Peter
On 11/20/23 15:20, Peter Griffin wrote: > Hi Guenter, > > On Mon, 20 Nov 2023 at 23:03, Guenter Roeck <linux@roeck-us.net> wrote: >> >> On 11/20/23 14:45, Peter Griffin wrote: >>> Hi Guenter, >>> >>> Thanks for the review. >>> >>> On Mon, 20 Nov 2023 at 22:00, Guenter Roeck <linux@roeck-us.net> wrote: >>>> >>>> On 11/20/23 13:20, Peter Griffin wrote: >>>>> The WDT uses the CPU core signal DBGACK to determine whether the SoC >>>>> is running in debug mode or not. If the DBGACK signal is asserted and >>>>> DBGACK_MASK is enabled, then WDT output and interrupt is masked. >>>>> >>>>> Presence of the DBGACK_MASK bit is determined by adding a new >>>>> QUIRK_HAS_DBGACK_BIT quirk. Currently only gs101 SoC is known to have >>>>> the DBGACK_MASK bit so add the quirk to drv_data_gs101_cl1 and >>>>> drv_data_gs101_cl1 quirks. >>>>> >>>>> Signed-off-by: Peter Griffin <peter.griffin@linaro.org> >>>>> --- >>>>> drivers/watchdog/s3c2410_wdt.c | 32 +++++++++++++++++++++++++++----- >>>>> 1 file changed, 27 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c >>>>> index 08b8c57dd812..ed561deeeed9 100644 >>>>> --- a/drivers/watchdog/s3c2410_wdt.c >>>>> +++ b/drivers/watchdog/s3c2410_wdt.c >>>>> @@ -34,9 +34,10 @@ >>>>> >>>>> #define S3C2410_WTCNT_MAXCNT 0xffff >>>>> >>>>> -#define S3C2410_WTCON_RSTEN (1 << 0) >>>>> -#define S3C2410_WTCON_INTEN (1 << 2) >>>>> -#define S3C2410_WTCON_ENABLE (1 << 5) >>>>> +#define S3C2410_WTCON_RSTEN (1 << 0) >>>>> +#define S3C2410_WTCON_INTEN (1 << 2) >>>>> +#define S3C2410_WTCON_ENABLE (1 << 5) >>>>> +#define S3C2410_WTCON_DBGACK_MASK (1 << 16) >>>>> >>>>> #define S3C2410_WTCON_DIV16 (0 << 3) >>>>> #define S3C2410_WTCON_DIV32 (1 << 3) >>>>> @@ -107,12 +108,16 @@ >>>>> * %QUIRK_HAS_PMU_CNT_EN: PMU block has some register (e.g. CLUSTERx_NONCPU_OUT) >>>>> * with "watchdog counter enable" bit. That bit should be set to make watchdog >>>>> * counter running. >>>>> + * >>>>> + * %QUIRK_HAS_DBGACK_BIT: WTCON register has DBGACK_MASK bit. Enables masking >>>>> + * WDT interrupt and reset request according to CPU core DBGACK signal. >>>> >>>> This is a bit difficult to understand. I _think_ it means that the DBGACK_MASK bit >>>> has to be set to be able to trigger interrupt and reset requests. >>> >>> Not quite, it is a bit that controls masking the watchdog outputs when the SoC >>> is in debug mode. >>> >>>> "masking" normally refers to disabling something (at least in interrupt context). >>>> "Enables masking WDT interrupt" sounds like the bit has to be set in order to >>>> be able to disable interupts, and the code below suggests that the bit has to be >>>> set for the driver to work. Is that the case ? It might make sense to explain this >>>> a bit further. >>> >>> Maybe I explained it more clearly in the commit message than the comment >>> >>> "The WDT uses the CPU core signal DBGACK to determine whether the SoC >>> is running in debug mode or not. If the DBGACK signal is asserted and >>> DBGACK_MASK is enabled, then WDT output and interrupt is masked." >>> >>> Is that any clearer? Or maybe simpler again >>> >>> "Enabling DBGACK_MASK bit masks the watchdog outputs when the SoC is >>> in debug mode. Debug mode is determined by the DBGACK CPU signal." >>> >>> Let me know what you think is the clearest and most succinct and I can >>> update the comment. >>> >> >> You are still using the term "masked" which I think just hides what >> the code is really doing. Why not just say "disable" ? > > The reason for using the "masked" terminology was that is what the > Watchdog IP TRM uses throughout to describe the feature. But I agree > just saying disable is clearer. > At least please say something like "masked (disabled)" if you want to use the term. Thanks, Guenter
Hi Rob, Thanks for your review. On Tue, 21 Nov 2023 at 15:16, Rob Herring <robh@kernel.org> wrote: > > On Mon, Nov 20, 2023 at 09:20:27PM +0000, Peter Griffin wrote: > > Specifying samsung,uart-fifosize in both DT and driver static data is error > > prone and relies on driver probe order and dt aliases to be correct. > > > > Additionally on many Exynos platforms these are (USI) universal serial > > interfaces which can be uart, spi or i2c, so it can change per board. > > > > For google,gs101-uart and exynosautov9-uart make samsung,uart-fifosize a > > required property. For these platforms fifosize now *only* comes from DT. > > > > It is hoped other Exynos platforms will also switch over time. > > Then allow the property on them. Not sure I fully understand your comment. Can you elaborate? Do you mean leave the 'samsung,uart-fifosize' as an optional property like it is currently even for the platforms that now require it to be present to function correctly? I deliberately restricted the yaml change to only require this property for the SoCs that already set the 'samsung,uart-fifosize' dt property. As setting the property and having the driver use what is specified in DT also requires a corresponding driver update (otherwise fifosize gets overwritten by the driver static data, and then becomes dependent on probe order, dt aliases etc). The rationale was drivers 'opt in' and add themselves to the compatibles in this patch as they migrate away from obtaining fifo size from driver static data to obtaining it from DT. > > > > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > > --- > > .../bindings/serial/samsung_uart.yaml | 17 +++++++++++++++++ > > 1 file changed, 17 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/serial/samsung_uart.yaml b/Documentation/devicetree/bindings/serial/samsung_uart.yaml > > index ccc3626779d9..22a1edadc4fe 100644 > > --- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml > > +++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml > > @@ -133,6 +133,23 @@ allOf: > > - const: uart > > - const: clk_uart_baud0 > > > > + - if: > > + properties: > > + compatible: > > + contains: > > + enum: > > + - google,gs101-uart > > + - samsung,exynosautov9-uart > > + then: > > + properties: > > + samsung,uart-fifosize: > > + description: The fifo size supported by the UART channel. > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + enum: [16, 64, 256] > > We already have 'fifo-size' in several drivers. Use that. Please move > its type/description definitions to serial.yaml and make drivers just do > 'fifo-size: true' if they use it. What do you suggest we do for the samsung,uart-fifosize property that is being used upstream? > > > + > > + required: > > + - samsung,uart-fifosize > > A new required property is an ABI break. Please explain why that is okay > in the commit message. > I can update the commit message to make clear there is an ABI break. As mentioned above the platforms where this is now required are either already setting the property or are new in this series. Is that sufficient justification? regards, Peter
On Mon, Nov 20, 2023 at 3:21 PM Peter Griffin <peter.griffin@linaro.org> wrote: > > The WDT uses the CPU core signal DBGACK to determine whether the SoC > is running in debug mode or not. If the DBGACK signal is asserted and > DBGACK_MASK is enabled, then WDT output and interrupt is masked. > > Presence of the DBGACK_MASK bit is determined by adding a new > QUIRK_HAS_DBGACK_BIT quirk. Currently only gs101 SoC is known to have > the DBGACK_MASK bit so add the quirk to drv_data_gs101_cl1 and > drv_data_gs101_cl1 quirks. > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > --- > drivers/watchdog/s3c2410_wdt.c | 32 +++++++++++++++++++++++++++----- > 1 file changed, 27 insertions(+), 5 deletions(-) > > diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c > index 08b8c57dd812..ed561deeeed9 100644 > --- a/drivers/watchdog/s3c2410_wdt.c > +++ b/drivers/watchdog/s3c2410_wdt.c > @@ -34,9 +34,10 @@ > > #define S3C2410_WTCNT_MAXCNT 0xffff > > -#define S3C2410_WTCON_RSTEN (1 << 0) > -#define S3C2410_WTCON_INTEN (1 << 2) > -#define S3C2410_WTCON_ENABLE (1 << 5) > +#define S3C2410_WTCON_RSTEN (1 << 0) > +#define S3C2410_WTCON_INTEN (1 << 2) > +#define S3C2410_WTCON_ENABLE (1 << 5) > +#define S3C2410_WTCON_DBGACK_MASK (1 << 16) > > #define S3C2410_WTCON_DIV16 (0 << 3) > #define S3C2410_WTCON_DIV32 (1 << 3) > @@ -107,12 +108,16 @@ > * %QUIRK_HAS_PMU_CNT_EN: PMU block has some register (e.g. CLUSTERx_NONCPU_OUT) > * with "watchdog counter enable" bit. That bit should be set to make watchdog > * counter running. > + * > + * %QUIRK_HAS_DBGACK_BIT: WTCON register has DBGACK_MASK bit. Enables masking > + * WDT interrupt and reset request according to CPU core DBGACK signal. > */ > #define QUIRK_HAS_WTCLRINT_REG (1 << 0) > #define QUIRK_HAS_PMU_MASK_RESET (1 << 1) > #define QUIRK_HAS_PMU_RST_STAT (1 << 2) > #define QUIRK_HAS_PMU_AUTO_DISABLE (1 << 3) > #define QUIRK_HAS_PMU_CNT_EN (1 << 4) > +#define QUIRK_HAS_DBGACK_BIT (1 << 5) > > /* These quirks require that we have a PMU register map */ > #define QUIRKS_HAVE_PMUREG \ > @@ -279,7 +284,7 @@ static const struct s3c2410_wdt_variant drv_data_gs101_cl0 = { > .cnt_en_reg = GS_CLUSTER0_NONCPU_OUT, > .cnt_en_bit = 8, > .quirks = QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_MASK_RESET | QUIRK_HAS_PMU_CNT_EN | > - QUIRK_HAS_WTCLRINT_REG, > + QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_DBGACK_BIT, > }; > > static const struct s3c2410_wdt_variant drv_data_gs101_cl1 = { > @@ -291,7 +296,7 @@ static const struct s3c2410_wdt_variant drv_data_gs101_cl1 = { > .cnt_en_reg = GS_CLUSTER1_NONCPU_OUT, > .cnt_en_bit = 7, > .quirks = QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_MASK_RESET | QUIRK_HAS_PMU_CNT_EN | > - QUIRK_HAS_WTCLRINT_REG, > + QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_DBGACK_BIT, > }; > This patch states it's adding the feature, but in fact it's also enabling this feature for gs101. Suggest moving this patch before the one enabling gs101 wdt. This way, one patch will only add the feature, and another patch will enable gs101 entirely (with this feature used). At least it seems like more atomic approach to me. > static const struct of_device_id s3c2410_wdt_match[] = { > @@ -408,6 +413,21 @@ static int s3c2410wdt_enable(struct s3c2410_wdt *wdt, bool en) > return 0; > } > > +static void s3c2410wdt_mask_dbgack(struct s3c2410_wdt *wdt, bool mask) > +{ > + unsigned long wtcon; > + > + if (!(wdt->drv_data->quirks & QUIRK_HAS_DBGACK_BIT)) > + return; > + > + wtcon = readl(wdt->reg_base + S3C2410_WTCON); > + if (mask) > + wtcon |= S3C2410_WTCON_DBGACK_MASK; > + else > + wtcon &= ~S3C2410_WTCON_DBGACK_MASK; > + writel(wtcon, wdt->reg_base + S3C2410_WTCON); > +} > + > static int s3c2410wdt_keepalive(struct watchdog_device *wdd) > { > struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd); > @@ -737,6 +757,8 @@ static int s3c2410wdt_probe(struct platform_device *pdev) > wdt->wdt_device.bootstatus = s3c2410wdt_get_bootstatus(wdt); > wdt->wdt_device.parent = dev; > > + s3c2410wdt_mask_dbgack(wdt, true); > + > /* > * If "tmr_atboot" param is non-zero, start the watchdog right now. Also > * set WDOG_HW_RUNNING bit, so that watchdog core can kick the watchdog. > -- > 2.43.0.rc1.413.gea7ed67945-goog >
On 11/21/23 09:52, Sam Protsenko wrote: > On Mon, Nov 20, 2023 at 3:21 PM Peter Griffin <peter.griffin@linaro.org> wrote: >> >> The WDT uses the CPU core signal DBGACK to determine whether the SoC >> is running in debug mode or not. If the DBGACK signal is asserted and >> DBGACK_MASK is enabled, then WDT output and interrupt is masked. >> >> Presence of the DBGACK_MASK bit is determined by adding a new >> QUIRK_HAS_DBGACK_BIT quirk. Currently only gs101 SoC is known to have >> the DBGACK_MASK bit so add the quirk to drv_data_gs101_cl1 and >> drv_data_gs101_cl1 quirks. >> >> Signed-off-by: Peter Griffin <peter.griffin@linaro.org> >> --- >> drivers/watchdog/s3c2410_wdt.c | 32 +++++++++++++++++++++++++++----- >> 1 file changed, 27 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c >> index 08b8c57dd812..ed561deeeed9 100644 >> --- a/drivers/watchdog/s3c2410_wdt.c >> +++ b/drivers/watchdog/s3c2410_wdt.c >> @@ -34,9 +34,10 @@ >> >> #define S3C2410_WTCNT_MAXCNT 0xffff >> >> -#define S3C2410_WTCON_RSTEN (1 << 0) >> -#define S3C2410_WTCON_INTEN (1 << 2) >> -#define S3C2410_WTCON_ENABLE (1 << 5) >> +#define S3C2410_WTCON_RSTEN (1 << 0) >> +#define S3C2410_WTCON_INTEN (1 << 2) >> +#define S3C2410_WTCON_ENABLE (1 << 5) >> +#define S3C2410_WTCON_DBGACK_MASK (1 << 16) >> >> #define S3C2410_WTCON_DIV16 (0 << 3) >> #define S3C2410_WTCON_DIV32 (1 << 3) >> @@ -107,12 +108,16 @@ >> * %QUIRK_HAS_PMU_CNT_EN: PMU block has some register (e.g. CLUSTERx_NONCPU_OUT) >> * with "watchdog counter enable" bit. That bit should be set to make watchdog >> * counter running. >> + * >> + * %QUIRK_HAS_DBGACK_BIT: WTCON register has DBGACK_MASK bit. Enables masking >> + * WDT interrupt and reset request according to CPU core DBGACK signal. >> */ >> #define QUIRK_HAS_WTCLRINT_REG (1 << 0) >> #define QUIRK_HAS_PMU_MASK_RESET (1 << 1) >> #define QUIRK_HAS_PMU_RST_STAT (1 << 2) >> #define QUIRK_HAS_PMU_AUTO_DISABLE (1 << 3) >> #define QUIRK_HAS_PMU_CNT_EN (1 << 4) >> +#define QUIRK_HAS_DBGACK_BIT (1 << 5) >> >> /* These quirks require that we have a PMU register map */ >> #define QUIRKS_HAVE_PMUREG \ >> @@ -279,7 +284,7 @@ static const struct s3c2410_wdt_variant drv_data_gs101_cl0 = { >> .cnt_en_reg = GS_CLUSTER0_NONCPU_OUT, >> .cnt_en_bit = 8, >> .quirks = QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_MASK_RESET | QUIRK_HAS_PMU_CNT_EN | >> - QUIRK_HAS_WTCLRINT_REG, >> + QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_DBGACK_BIT, >> }; >> >> static const struct s3c2410_wdt_variant drv_data_gs101_cl1 = { >> @@ -291,7 +296,7 @@ static const struct s3c2410_wdt_variant drv_data_gs101_cl1 = { >> .cnt_en_reg = GS_CLUSTER1_NONCPU_OUT, >> .cnt_en_bit = 7, >> .quirks = QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_MASK_RESET | QUIRK_HAS_PMU_CNT_EN | >> - QUIRK_HAS_WTCLRINT_REG, >> + QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_DBGACK_BIT, >> }; >> > > This patch states it's adding the feature, but in fact it's also > enabling this feature for gs101. Suggest moving this patch before the > one enabling gs101 wdt. This way, one patch will only add the feature, > and another patch will enable gs101 entirely (with this feature used). > At least it seems like more atomic approach to me. > Both approaches have their merits and their downsides. I for my part am not even sure if DBGACK_MASK should be enabled unconditionally if supported. With your approach, it would be impossible (or at least more difficult) to revert if it turns out to be broken and/or have unexpected side effects. That seems less desirable to me than the current approach. Guenter
On 21/11/2023 18:15, Peter Griffin wrote: > Hi Rob, > > Thanks for your review. > > On Tue, 21 Nov 2023 at 15:16, Rob Herring <robh@kernel.org> wrote: >> >> On Mon, Nov 20, 2023 at 09:20:27PM +0000, Peter Griffin wrote: >>> Specifying samsung,uart-fifosize in both DT and driver static data is error >>> prone and relies on driver probe order and dt aliases to be correct. >>> >>> Additionally on many Exynos platforms these are (USI) universal serial >>> interfaces which can be uart, spi or i2c, so it can change per board. >>> >>> For google,gs101-uart and exynosautov9-uart make samsung,uart-fifosize a >>> required property. For these platforms fifosize now *only* comes from DT. >>> >>> It is hoped other Exynos platforms will also switch over time. >> >> Then allow the property on them. > > Not sure I fully understand your comment. Can you elaborate? Do you > mean leave the 'samsung,uart-fifosize' as an optional property like it > is currently even for the platforms that now require it to be present > to function correctly? > > I deliberately restricted the yaml change to only require this > property for the SoCs that already set the 'samsung,uart-fifosize' dt > property. As setting the property and having the driver use what is > specified in DT also requires a corresponding driver update (otherwise > fifosize gets overwritten by the driver static data, and then becomes > dependent on probe order, dt aliases etc). The rationale was drivers > 'opt in' and add themselves to the compatibles in this patch as they > migrate away from obtaining fifo size from driver static data to > obtaining it from DT. Your code diff looks like you are adding the property only to these models. > >> >>> >>> Signed-off-by: Peter Griffin <peter.griffin@linaro.org> >>> --- >>> .../bindings/serial/samsung_uart.yaml | 17 +++++++++++++++++ >>> 1 file changed, 17 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/serial/samsung_uart.yaml b/Documentation/devicetree/bindings/serial/samsung_uart.yaml >>> index ccc3626779d9..22a1edadc4fe 100644 >>> --- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml >>> +++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml >>> @@ -133,6 +133,23 @@ allOf: >>> - const: uart >>> - const: clk_uart_baud0 >>> >>> + - if: >>> + properties: >>> + compatible: >>> + contains: >>> + enum: >>> + - google,gs101-uart >>> + - samsung,exynosautov9-uart >>> + then: >>> + properties: >>> + samsung,uart-fifosize: >>> + description: The fifo size supported by the UART channel. >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + enum: [16, 64, 256] >> >> We already have 'fifo-size' in several drivers. Use that. Please move >> its type/description definitions to serial.yaml and make drivers just do >> 'fifo-size: true' if they use it. > > What do you suggest we do for the samsung,uart-fifosize property that > is being used upstream? Nothing, your diff is just wrong. Or at least nothing needed. Just drop all this properties: here and only make it required for Google GS101. > >> >>> + >>> + required: >>> + - samsung,uart-fifosize >> >> A new required property is an ABI break. Please explain why that is okay >> in the commit message. >> > > I can update the commit message to make clear there is an ABI break. > As mentioned above the platforms where this is now required are either > already setting the property or are new in this series. Is that > sufficient justification? Yes, but only first case. You need to order your patches correctly - first is ABI break expecting ExynopsAutov9 to provide FIFO size in DTS with its explanation. Second commit is adding GS101 where there is no ABI break. Best regards, Krzysztof
On 21/11/2023 19:10, Guenter Roeck wrote: >>> static const struct s3c2410_wdt_variant drv_data_gs101_cl1 = { >>> @@ -291,7 +296,7 @@ static const struct s3c2410_wdt_variant drv_data_gs101_cl1 = { >>> .cnt_en_reg = GS_CLUSTER1_NONCPU_OUT, >>> .cnt_en_bit = 7, >>> .quirks = QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_MASK_RESET | QUIRK_HAS_PMU_CNT_EN | >>> - QUIRK_HAS_WTCLRINT_REG, >>> + QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_DBGACK_BIT, >>> }; >>> >> >> This patch states it's adding the feature, but in fact it's also >> enabling this feature for gs101. Suggest moving this patch before the >> one enabling gs101 wdt. This way, one patch will only add the feature, >> and another patch will enable gs101 entirely (with this feature used). >> At least it seems like more atomic approach to me. >> > > Both approaches have their merits and their downsides. I for my part am not > even sure if DBGACK_MASK should be enabled unconditionally if supported. > With your approach, it would be impossible (or at least more difficult) to > revert if it turns out to be broken and/or have unexpected side effects. > That seems less desirable to me than the current approach. Reversing the patches does not change this. It is enabled unconditionally in current order as well. Sam's idea is correct here - first you add support for new quirk, then you add new SoC which will use this quirk. Doing the other way - first SoC and then new quirk - looks like SoC was added incomplete. Best regards, Krzysztof
Hi Krzysztof / Guenter / Sam, On Wed, 22 Nov 2023 at 07:53, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 21/11/2023 19:10, Guenter Roeck wrote: > > >>> static const struct s3c2410_wdt_variant drv_data_gs101_cl1 = { > >>> @@ -291,7 +296,7 @@ static const struct s3c2410_wdt_variant drv_data_gs101_cl1 = { > >>> .cnt_en_reg = GS_CLUSTER1_NONCPU_OUT, > >>> .cnt_en_bit = 7, > >>> .quirks = QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_MASK_RESET | QUIRK_HAS_PMU_CNT_EN | > >>> - QUIRK_HAS_WTCLRINT_REG, > >>> + QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_DBGACK_BIT, > >>> }; > >>> > >> > >> This patch states it's adding the feature, but in fact it's also > >> enabling this feature for gs101. Suggest moving this patch before the > >> one enabling gs101 wdt. This way, one patch will only add the feature, > >> and another patch will enable gs101 entirely (with this feature used). > >> At least it seems like more atomic approach to me. > >> > > > > Both approaches have their merits and their downsides. I for my part am not > > even sure if DBGACK_MASK should be enabled unconditionally if supported. > > With your approach, it would be impossible (or at least more difficult) to > > revert if it turns out to be broken and/or have unexpected side effects. > > That seems less desirable to me than the current approach. > > Reversing the patches does not change this. It is enabled > unconditionally in current order as well. > > Sam's idea is correct here - first you add support for new quirk, then > you add new SoC which will use this quirk. Doing the other way - first > SoC and then new quirk - looks like SoC was added incomplete. Sure I can swap the order around if that's what you prefer. I ordered it this way so it was clear who the user of the new debug feature was. Peter
Hi Krzysztof, On Wed, 22 Nov 2023 at 07:49, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 21/11/2023 18:15, Peter Griffin wrote: > > Hi Rob, > > > > Thanks for your review. > > > > On Tue, 21 Nov 2023 at 15:16, Rob Herring <robh@kernel.org> wrote: > >> > >> On Mon, Nov 20, 2023 at 09:20:27PM +0000, Peter Griffin wrote: > >>> Specifying samsung,uart-fifosize in both DT and driver static data is error > >>> prone and relies on driver probe order and dt aliases to be correct. > >>> > >>> Additionally on many Exynos platforms these are (USI) universal serial > >>> interfaces which can be uart, spi or i2c, so it can change per board. > >>> > >>> For google,gs101-uart and exynosautov9-uart make samsung,uart-fifosize a > >>> required property. For these platforms fifosize now *only* comes from DT. > >>> > >>> It is hoped other Exynos platforms will also switch over time. > >> > >> Then allow the property on them. > > > > Not sure I fully understand your comment. Can you elaborate? Do you > > mean leave the 'samsung,uart-fifosize' as an optional property like it > > is currently even for the platforms that now require it to be present > > to function correctly? > > > > I deliberately restricted the yaml change to only require this > > property for the SoCs that already set the 'samsung,uart-fifosize' dt > > property. As setting the property and having the driver use what is > > specified in DT also requires a corresponding driver update (otherwise > > fifosize gets overwritten by the driver static data, and then becomes > > dependent on probe order, dt aliases etc). The rationale was drivers > > 'opt in' and add themselves to the compatibles in this patch as they > > migrate away from obtaining fifo size from driver static data to > > obtaining it from DT. > > Your code diff looks like you are adding the property only to these models. OK, I intended to only make it a required DT property for these models. Presumably there is a better way to achieve that. > > > > >> > >>> > >>> Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > >>> --- > >>> .../bindings/serial/samsung_uart.yaml | 17 +++++++++++++++++ > >>> 1 file changed, 17 insertions(+) > >>> > >>> diff --git a/Documentation/devicetree/bindings/serial/samsung_uart.yaml b/Documentation/devicetree/bindings/serial/samsung_uart.yaml > >>> index ccc3626779d9..22a1edadc4fe 100644 > >>> --- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml > >>> +++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml > >>> @@ -133,6 +133,23 @@ allOf: > >>> - const: uart > >>> - const: clk_uart_baud0 > >>> > >>> + - if: > >>> + properties: > >>> + compatible: > >>> + contains: > >>> + enum: > >>> + - google,gs101-uart > >>> + - samsung,exynosautov9-uart > >>> + then: > >>> + properties: > >>> + samsung,uart-fifosize: > >>> + description: The fifo size supported by the UART channel. > >>> + $ref: /schemas/types.yaml#/definitions/uint32 > >>> + enum: [16, 64, 256] > >> > >> We already have 'fifo-size' in several drivers. Use that. Please move > >> its type/description definitions to serial.yaml and make drivers just do > >> 'fifo-size: true' if they use it. > > > > What do you suggest we do for the samsung,uart-fifosize property that > > is being used upstream? > > Nothing, your diff is just wrong. Or at least nothing needed. Just drop > all this properties: here and only make it required for Google GS101. OK, so your happy with the approach just not the implementation/patch and you don't want it updated to use fifo-size instead of samsung,uart-fifosize > > > > > >> > >>> + > >>> + required: > >>> + - samsung,uart-fifosize > >> > >> A new required property is an ABI break. Please explain why that is okay > >> in the commit message. > >> > > > > I can update the commit message to make clear there is an ABI break. > > As mentioned above the platforms where this is now required are either > > already setting the property or are new in this series. Is that > > sufficient justification? > Yes, but only first case. You need to order your patches correctly - > first is ABI break expecting ExynopsAutov9 to provide FIFO size in DTS > with its explanation. Second commit is adding GS101 where there is no > ABI break. OK, I'll drop the ExynopsAutov9 part then. I don't want to complicate this series by introducing an ABI breakage on some other unrelated Exynos platform. Peter
Hi folks, Firstly, thanks to everyone who reviewed the previous series. V4 incorporates all the review feedback received so far, and is rebased onto linux-next. As this series spans multiple subsytems the expectation is that Krzysztof will apply the whole series through the Samsung SoC tree. If the relevant subsystem maintainers can give a acked-by or reviewed-by on the relevant patches that would be most appreciated! This series adds initial SoC support for the GS101 SoC and also initial board support for Pixel 6 phone (Oriole). The gs101 / Tensor SoC is also used in Pixel6a (bluejay) and Pixel 6 Pro (raven) phones. Currently DT is added for the gs101 SoC and Oriole. As you can see from the patches the SoC is based on a Samsung Exynos SoC, and therefore lots of the low level Exynos drivers and bindings can be re-used. The support added in this series consists of: * cpus * pinctrl * some CCF implementation * watchdog * uart * gpio This is enough to boot through to a busybox initramfs and shell using an upstream kernel though :) More platform support will be added over the following weeks and months. For further information on how to build and flash the upstream kernel on your Pixel 6, with a prebuilt busybox initramfs please refer to the script and README.md here: https://git.codelinaro.org/linaro/googlelt/pixelscripts Note 1: I've removed the dtbo overlay from v2 and later submissions and will re-submit once I have appropriate documentation for it. Note 2: I've left the bootargs in dts with earlycon for now, for two reasons. 1) The bootloader hangs if bootargs isn't present in the dtb as it tries to re-write this with additional bootargs. 2) There is a issue whereby the full serial console doesn't come up properly if `exynos4210,mmio32,0x10A00000` isn't specified in bootargs. This has been diagnosed by Tudor as some prior bootstage adding earlycon to the bootargs but not specifying the 'mmio32' parameter which then causes earlycon to use 8bit accesses. On gs101 blk_perioc0 is restricted to 32b data width accesses causing a SError interrupt before any earlycon UART output. kind regards, Peter. Changes since v3: - Add reviewed-by and tested-by tags - google,gs101-clock.yaml: move Required to before Allof, enum for cmu*top/misc (Krzysztof) - samsung-wdt.yaml: stick to 80chars (Sam) - google.yaml - remove new line - samsung,pinctrl-wakeup-interrupt: sort alphabetically (RobH) - gs101-oriole.dts: update gpio-keys pinctrl-0 phandle for keys (Stephen) - samsung,exynos-sysreg.yaml - Alphabetical order (RobH) - pinctrl-exynos: update/move comments, slight cosmetic changes (Sam) - samsung_tty.c: update to generic drv_data name/macro (Arnd) - samsung_uart.yaml: make samsung,uart-fifosize required for gs101-uart (Arnd) - pinctrl-exynos: Remove eint irqs from alive pin controller node (Peter/Rob) - Fixup kernel test robot unused const variable warnings - clk-gs101: Update to mout_cmu_eh_bus to CLK_CON_MUX_MUX_CLKCMU_EH_BUS (Chanwoo) - clk-gs101: Update g3aa gout/dout/mout names to g3aa_g3aa for consistency (Chanwoo) - Remove .eint_gpio_init() cb on alive, alive_far, gsacore & gsactrl banks (Sam) - s3c2410_wdt: Drop windowed watchdog mode for now (Peter) - s3c2410_wdt: Separate gs101 SoC support from dbgack feature (Sam) - Move dts to arch/arm64/boot/dts/exynos/google directory (Krzysztof) Changes since v2: - Fixup pinctrl@174d0000: interrupts: [..] is too long DTC warning (Tudor) - Add missing windowed watchdog code (Guenter) - Fixup UART YAML bindings error (Krzysztof) - gs101.dtsi add missing serial_0 alias (me) - samsung_tty.c: fixup gs101_serial_drv_data so fifosize is obtained from DT Changes since v1: - Remove irq/gs101.h and replace macros with irq numbers globally - exynos-pmu - keep alphabetical order - add cmu_apm to clock bindings documentation - sysreg bindings - remove superfluous `google,gs101-sysreg` - watchdog bindings - Alphanumerical order, update gs201 comment - samsung,pinctrl.yaml - add new "if:then:else:" to narrow for google SoC - samsung,pinctrl-wakeup-interrupt.yaml - Alphanumerical order - samsung,pinctrl- add google,gs101-wakeup-eint compatible - clk-pll: fixup typos - clk-gs101: fix kernel test robot warnings (add 2 new clocks,dividers,gate) - clk-gs101: fix alphabetical order - clk-gs101: cmu_apm: fixup typo and missing empty entry - clk-gs101: cmu_misc: remove clocks that were being registerred twice - pinctrl: filter sel: rename/reorder variables, add comment for FLTCON bitfield - pinctrl: filter sel: avoid setting reserved bits by loop over FLTCON1 pins as well - pinctrl: gs101: rename bank_type_6/7 structs to be more specific, split from filter - watchdog: s3c2410_wdt: remove dev_info prints - gs101.dtsi/oriole.dts: order by unit node, remove underscores from node name, blank lines add SoC node, split dts and dtsi into separate patches, remove 'DVT' suffix - gs101-oriole.dtso: Remove overlay until board_id is documented properly - Add GS101_PIN_* macros to gs101-pinctrl.h instead of using Exynos ones - gpio-keys: update linux,code to use input-event-code macros - add dedicated gs101-uart compatible Peter Griffin (19): dt-bindings: soc: samsung: exynos-pmu: Add gs101 compatible dt-bindings: clock: Add Google gs101 clock management unit bindings dt-bindings: soc: google: exynos-sysreg: add dedicated SYSREG compatibles to GS101 dt-bindings: watchdog: Document Google gs101 watchdog bindings dt-bindings: arm: google: Add bindings for Google ARM platforms dt-bindings: pinctrl: samsung: add google,gs101-pinctrl compatible dt-bindings: pinctrl: samsung: add gs101-wakeup-eint compatible dt-bindings: serial: samsung: Add google-gs101-uart compatible dt-bindings: serial: samsung: Make samsung,uart-fifosize required property clk: samsung: clk-pll: Add support for pll_{0516,0517,518} clk: samsung: clk-gs101: Add cmu_top, cmu_misc and cmu_apm support pinctrl: samsung: Add filter selection support for alive banks pinctrl: samsung: Add gs101 SoC pinctrl configuration watchdog: s3c2410_wdt: Add support for Google gs101 SoC watchdog: s3c2410_wdt: Add support for WTCON register DBGACK_MASK bit tty: serial: samsung: Add gs101 compatible and common fifoszdt_serial_drv_data arm64: dts: exynos: google: Add initial Google gs101 SoC support arm64: dts: exynos: google: Add initial Oriole/pixel 6 board support MAINTAINERS: add entry for Google Tensor SoC .../devicetree/bindings/arm/google.yaml | 45 + .../bindings/clock/google,gs101-clock.yaml | 110 + .../samsung,pinctrl-wakeup-interrupt.yaml | 2 + .../bindings/pinctrl/samsung,pinctrl.yaml | 1 + .../bindings/serial/samsung_uart.yaml | 18 + .../bindings/soc/samsung/exynos-pmu.yaml | 2 + .../soc/samsung/samsung,exynos-sysreg.yaml | 6 + .../bindings/watchdog/samsung-wdt.yaml | 8 +- MAINTAINERS | 10 + arch/arm64/boot/dts/exynos/Makefile | 2 + arch/arm64/boot/dts/exynos/google/Makefile | 4 + .../boot/dts/exynos/google/gs101-oriole.dts | 79 + .../boot/dts/exynos/google/gs101-pinctrl.dtsi | 1275 +++++++++++ .../boot/dts/exynos/google/gs101-pinctrl.h | 32 + arch/arm64/boot/dts/exynos/google/gs101.dtsi | 437 ++++ drivers/clk/samsung/Makefile | 1 + drivers/clk/samsung/clk-gs101.c | 2036 +++++++++++++++++ drivers/clk/samsung/clk-pll.c | 6 + drivers/clk/samsung/clk-pll.h | 3 + .../pinctrl/samsung/pinctrl-exynos-arm64.c | 159 ++ drivers/pinctrl/samsung/pinctrl-exynos.c | 91 +- drivers/pinctrl/samsung/pinctrl-exynos.h | 41 + drivers/pinctrl/samsung/pinctrl-samsung.c | 4 + drivers/pinctrl/samsung/pinctrl-samsung.h | 23 + drivers/tty/serial/samsung_tty.c | 16 + drivers/watchdog/s3c2410_wdt.c | 75 +- include/dt-bindings/clock/google,gs101.h | 232 ++ 27 files changed, 4707 insertions(+), 11 deletions(-) create mode 100644 Documentation/devicetree/bindings/arm/google.yaml create mode 100644 Documentation/devicetree/bindings/clock/google,gs101-clock.yaml create mode 100644 arch/arm64/boot/dts/exynos/google/Makefile create mode 100644 arch/arm64/boot/dts/exynos/google/gs101-oriole.dts create mode 100644 arch/arm64/boot/dts/exynos/google/gs101-pinctrl.dtsi create mode 100644 arch/arm64/boot/dts/exynos/google/gs101-pinctrl.h create mode 100644 arch/arm64/boot/dts/exynos/google/gs101.dtsi create mode 100644 drivers/clk/samsung/clk-gs101.c create mode 100644 include/dt-bindings/clock/google,gs101.h