mbox series

[v7,00/12] Introduce Nuvoton ma35d1 SoC

Message ID 20230412053824.106-1-ychuang570808@gmail.com
Headers show
Series Introduce Nuvoton ma35d1 SoC | expand

Message

Jacky Huang April 12, 2023, 5:38 a.m. UTC
From: Jacky Huang <ychuang3@nuvoton.com>

This patchset adds initial support for the Nuvoton ma35d1 SoC, including
initial device tree, clock driver, reset driver, and serial driver.

This patchset cover letter is based from the initial support for Nuvoton
ma35d1 to keep tracking the version history.

This patchset had been applied to Linux kernel 6.3.0-rc6 and tested on the
Nuvoton ma35d1 SOM evaluation board.

(ma35d1 information: https://www.nuvoton.com/products/microprocessors/arm-cortex-a35-mpus/)
MA35D1 porting on linux-5.10.y can be found at: https://github.com/OpenNuvoton/MPU-Family

v7:
  - Fixed dts system-management node and compatible driver
  - move 'nuvoton,npcm-gcr.yaml' from 'binding/arm/nuvoton' to 'binding/soc/nuvoton'
  - In ma35d1.dtsi, create the soc node for ma35d1 SoC
  - Modify the issues found in serial driver
  - Modify the issues found in clock driver
  - Modify the IDs of reset driver to be contiguous numbers and use lookup table
    to find register offset and bit position.

v6:
  - Combine nuvoton,ma35d1-clk.yaml and nuvoton,ma35d1-clk.h into one patch
  - Combine nuvoton,ma35d1-reset.yaml and nuvoton,ma35d1-reset.h into one patch
  - rename Documentation/devicetree/bindings/arm/npcm directory as nuvoton
  - Remove patch for adding include/linux/mfd/ma35d1-sys.h as it's not required
  - Update dtsi & dts files and move board-specific nodes to dts
  - Modify reset driver
  - Modify serial driver, fix coding style issues
  - Modify clock driver, rewrite the PLL calculation functions

v5:
  - Add ARCH_NUVOTON to arm64 Kconfig
  - Add ARCH_NUVOTON to defconfig
  - Add the clock driver
  - Add the reset driver
  - Add the serial driver
  - Add us to the maintainer

v4:
  - patch 4/5 is a resend
  - Fixed dt_binding_check errors of nuvoton,ma35d1-clk.yaml
  - Modify ma35d1.dtsi
    1. Add a node hxt_24m
    2. Fixed the base address of gic node
    3. Add clocks and clock-names to clock node
  - Fixed borad binding mistakes of nuvoton.yaml

v3:
  - added patch 4/5 and 5/5
  - introduce CONFIG_ARCH_NUVOTON option
  - add initial bindings for Nuvoton Platform boards
  - fixed coding style problem of nuvoton,ma35d1-clk.h
  - added CAPLL to clock-controller node
  - modify the chosen node of ma35d1-evb.dts
  - modify clock yaml "clk-pll-mode" to "nuvoton,clk-pll-mode"

v2:
  - fixed dt_binding_check failed of nuvoton,ma35d1-clk.yaml

Jacky Huang (12):
  arm64: Kconfig.platforms: Add config for Nuvoton MA35 platform
  arm64: defconfig: Add support for Nuvoton MA35 family SoCs
  dt-bindings: clock: nuvoton: add binding for ma35d1 clock controller
  dt-bindings: reset: nuvoton: Document ma35d1 reset control
  dt-bindings: mfd: syscon: Add nuvoton,ma35d1-sys compatible
  dt-bindings: arm: Add initial bindings for Nuvoton platform
  dt-bindings: serial: Document ma35d1 uart controller
  arm64: dts: nuvoton: Add initial ma35d1 device tree
  clk: nuvoton: Add clock driver for ma35d1 clock controller
  reset: Add Nuvoton ma35d1 reset driver support
  tty: serial: Add Nuvoton ma35d1 serial driver support
  MAINTAINERS: Add entry for NUVOTON MA35

 .../bindings/arm/nuvoton/nuvoton,ma35d1.yaml  |  30 +
 .../npcm.yaml => nuvoton/nuvoton,npcm.yaml}   |   2 +-
 .../bindings/clock/nuvoton,ma35d1-clk.yaml    |  63 ++
 .../devicetree/bindings/mfd/syscon.yaml       |   1 +
 .../bindings/reset/nuvoton,ma35d1-reset.yaml  |  46 +
 .../serial/nuvoton,ma35d1-serial.yaml         |  48 +
 .../nuvoton/nuvoton,npcm-gcr.yaml}            |   2 +-
 MAINTAINERS                                   |  13 +-
 arch/arm64/Kconfig.platforms                  |   9 +
 arch/arm64/boot/dts/nuvoton/Makefile          |   2 +
 .../boot/dts/nuvoton/ma35d1-iot-512m.dts      |  56 ++
 .../boot/dts/nuvoton/ma35d1-som-256m.dts      |  56 ++
 arch/arm64/boot/dts/nuvoton/ma35d1.dtsi       | 232 +++++
 arch/arm64/configs/defconfig                  |   1 +
 drivers/clk/Makefile                          |   1 +
 drivers/clk/nuvoton/Kconfig                   |  19 +
 drivers/clk/nuvoton/Makefile                  |   4 +
 drivers/clk/nuvoton/clk-ma35d1-divider.c      | 134 +++
 drivers/clk/nuvoton/clk-ma35d1-pll.c          | 315 ++++++
 drivers/clk/nuvoton/clk-ma35d1.c              | 897 ++++++++++++++++++
 drivers/clk/nuvoton/clk-ma35d1.h              | 123 +++
 drivers/reset/Kconfig                         |   6 +
 drivers/reset/Makefile                        |   1 +
 drivers/reset/reset-ma35d1.c                  | 255 +++++
 drivers/tty/serial/Kconfig                    |  18 +
 drivers/tty/serial/Makefile                   |   1 +
 drivers/tty/serial/ma35d1_serial.c            | 773 +++++++++++++++
 .../dt-bindings/clock/nuvoton,ma35d1-clk.h    | 253 +++++
 .../dt-bindings/reset/nuvoton,ma35d1-reset.h  | 108 +++
 29 files changed, 3466 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/nuvoton/nuvoton,ma35d1.yaml
 rename Documentation/devicetree/bindings/arm/{npcm/npcm.yaml => nuvoton/nuvoton,npcm.yaml} (93%)
 create mode 100644 Documentation/devicetree/bindings/clock/nuvoton,ma35d1-clk.yaml
 create mode 100644 Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml
 create mode 100644 Documentation/devicetree/bindings/serial/nuvoton,ma35d1-serial.yaml
 rename Documentation/devicetree/bindings/{arm/npcm/nuvoton,gcr.yaml => soc/nuvoton/nuvoton,npcm-gcr.yaml} (93%)
 create mode 100644 arch/arm64/boot/dts/nuvoton/ma35d1-iot-512m.dts
 create mode 100644 arch/arm64/boot/dts/nuvoton/ma35d1-som-256m.dts
 create mode 100644 arch/arm64/boot/dts/nuvoton/ma35d1.dtsi
 create mode 100644 drivers/clk/nuvoton/Kconfig
 create mode 100644 drivers/clk/nuvoton/Makefile
 create mode 100644 drivers/clk/nuvoton/clk-ma35d1-divider.c
 create mode 100644 drivers/clk/nuvoton/clk-ma35d1-pll.c
 create mode 100644 drivers/clk/nuvoton/clk-ma35d1.c
 create mode 100644 drivers/clk/nuvoton/clk-ma35d1.h
 create mode 100644 drivers/reset/reset-ma35d1.c
 create mode 100644 drivers/tty/serial/ma35d1_serial.c
 create mode 100644 include/dt-bindings/clock/nuvoton,ma35d1-clk.h
 create mode 100644 include/dt-bindings/reset/nuvoton,ma35d1-reset.h

Comments

Krzysztof Kozlowski April 13, 2023, 4:47 p.m. UTC | #1
On 12/04/2023 07:38, Jacky Huang wrote:
> From: Jacky Huang <ychuang3@nuvoton.com>
> 
> Add Nuvoton ma35d1 system registers compatible.
> 
> Signed-off-by: Jacky Huang <ychuang3@nuvoton.com>

What about the tag? Why did you ignore it?

Also, wasn't this applied? Why do you resend (incorrect version)?

Best regards,
Krzysztof
Krzysztof Kozlowski April 13, 2023, 5:01 p.m. UTC | #2
On 12/04/2023 07:38, Jacky Huang wrote:
> From: Jacky Huang <ychuang3@nuvoton.com>
> 
> Move 'nuvoton,npcm-gcr.yaml' from 'arm/npcm' to 'soc/nuvoton'.
> Rename the '/arm/npcm' directory to 'arm/nuvoton'. Additionally, add
> bindings for ARMv8-based Nuvoton SoCs and platform boards, and include
> the initial bindings for ma35d1 series development boards.
> 
> Signed-off-by: Jacky Huang <ychuang3@nuvoton.com>
> ---
>  .../bindings/arm/nuvoton/nuvoton,ma35d1.yaml  | 30 +++++++++++++++++++
>  .../npcm.yaml => nuvoton/nuvoton,npcm.yaml}   |  2 +-
>  .../nuvoton/nuvoton,npcm-gcr.yaml}            |  2 +-
>  3 files changed, 32 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/arm/nuvoton/nuvoton,ma35d1.yaml
>  rename Documentation/devicetree/bindings/arm/{npcm/npcm.yaml => nuvoton/nuvoton,npcm.yaml} (93%)
>  rename Documentation/devicetree/bindings/{arm/npcm/nuvoton,gcr.yaml => soc/nuvoton/nuvoton,npcm-gcr.yaml} (93%)

I don't think you fixed the failure it was reported. Your changelog also
does not mention it.

Fix all comments and reports given. Path(s) in maintainers is/are broken.



Best regards,
Krzysztof
Krzysztof Kozlowski April 13, 2023, 5:01 p.m. UTC | #3
On 12/04/2023 07:38, Jacky Huang wrote:
> From: Jacky Huang <ychuang3@nuvoton.com>
> 
> Add entry for Nuvoton ma35d1 maintainer and files.
> In addition, update board binding paths of NUVOTON NPCM.


>  ARM/NUVOTON NPCM ARCHITECTURE
>  M:	Avi Fishman <avifishman70@gmail.com>
>  M:	Tomer Maimon <tmaimon77@gmail.com>
> @@ -2512,7 +2522,8 @@ L:	openbmc@lists.ozlabs.org (moderated for non-subscribers)
>  S:	Supported
>  F:	Documentation/devicetree/bindings/*/*/*npcm*
>  F:	Documentation/devicetree/bindings/*/*npcm*
> -F:	Documentation/devicetree/bindings/arm/npcm/*

It is not a bisectable change.

Best regards,
Krzysztof
Jacky Huang April 14, 2023, 1:22 a.m. UTC | #4
Dear Krzysztof,


On 2023/4/14 上午 01:01, Krzysztof Kozlowski wrote:
> On 12/04/2023 07:38, Jacky Huang wrote:
>> From: Jacky Huang <ychuang3@nuvoton.com>
>>
>> Add entry for Nuvoton ma35d1 maintainer and files.
>> In addition, update board binding paths of NUVOTON NPCM.
>
>>   ARM/NUVOTON NPCM ARCHITECTURE
>>   M:	Avi Fishman <avifishman70@gmail.com>
>>   M:	Tomer Maimon <tmaimon77@gmail.com>
>> @@ -2512,7 +2522,8 @@ L:	openbmc@lists.ozlabs.org (moderated for non-subscribers)
>>   S:	Supported
>>   F:	Documentation/devicetree/bindings/*/*/*npcm*
>>   F:	Documentation/devicetree/bindings/*/*npcm*
>> -F:	Documentation/devicetree/bindings/arm/npcm/*
> It is not a bisectable change.
>
> Best regards,
> Krzysztof
>

F:    Documentation/devicetree/bindings/*/*/*npcm*
F:    Documentation/devicetree/bindings/*/*npcm*
F:    Documentation/devicetree/bindings/arm/nuvoton/*npcm*    <-- remove 
this
F:    Documentation/devicetree/bindings/soc/nuvoton/*npcm* <-- remove this

Yes, the two statements at the bottom seem redundant. I will remove them.

Best regards,
Jacky Huang
Krzysztof Kozlowski April 14, 2023, 7:46 a.m. UTC | #5
On 14/04/2023 03:22, Jacky Huang wrote:
> Dear Krzysztof,
> 
> 
> On 2023/4/14 上午 01:01, Krzysztof Kozlowski wrote:
>> On 12/04/2023 07:38, Jacky Huang wrote:
>>> From: Jacky Huang <ychuang3@nuvoton.com>
>>>
>>> Add entry for Nuvoton ma35d1 maintainer and files.
>>> In addition, update board binding paths of NUVOTON NPCM.
>>
>>>   ARM/NUVOTON NPCM ARCHITECTURE
>>>   M:	Avi Fishman <avifishman70@gmail.com>
>>>   M:	Tomer Maimon <tmaimon77@gmail.com>
>>> @@ -2512,7 +2522,8 @@ L:	openbmc@lists.ozlabs.org (moderated for non-subscribers)
>>>   S:	Supported
>>>   F:	Documentation/devicetree/bindings/*/*/*npcm*
>>>   F:	Documentation/devicetree/bindings/*/*npcm*
>>> -F:	Documentation/devicetree/bindings/arm/npcm/*
>> It is not a bisectable change.
>>
>> Best regards,
>> Krzysztof
>>
> 
> F:    Documentation/devicetree/bindings/*/*/*npcm*
> F:    Documentation/devicetree/bindings/*/*npcm*
> F:    Documentation/devicetree/bindings/arm/nuvoton/*npcm*    <-- remove 
> this
> F:    Documentation/devicetree/bindings/soc/nuvoton/*npcm* <-- remove this
> 
> Yes, the two statements at the bottom seem redundant. I will remove them.

I did not comment about "redundant". I used the word "bisectable". git
help bisect.

Best regards,
Krzysztof
Jacky Huang April 14, 2023, 8:34 a.m. UTC | #6
On 2023/4/14 下午 03:03, Lee Jones wrote:
> On Fri, 14 Apr 2023, Jacky Huang wrote:
>
>> Dear Krzysztof,
>>
>>
>> On 2023/4/14 上午 12:47, Krzysztof Kozlowski wrote:
>>> On 12/04/2023 07:38, Jacky Huang wrote:
>>>> From: Jacky Huang <ychuang3@nuvoton.com>
>>>>
>>>> Add Nuvoton ma35d1 system registers compatible.
>>>>
>>>> Signed-off-by: Jacky Huang <ychuang3@nuvoton.com>
>>> What about the tag? Why did you ignore it?
>>>
>>> Also, wasn't this applied? Why do you resend (incorrect version)?
>>>
>>> Best regards,
>>> Krzysztof
>>>
>> When I was making this patchset, this patch was still not merged.
>> So I'm not sure if I should remove it.
>> This is just a resend with no updates. And I will remove this patch
>> in the next version as it was applied.
>> If possible, please add the following tags for this patch.
>>
>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> I added this.
>
>> Reviewed-by: Lee Jones <lee@kernel.org>
> When did I provide this?
>
> --
> Lee Jones [李琼斯]

Dear Lee,

Thank you for your help. And, I'm sorry, I thought 'applied' meant 
'reviewed', and this patch
didn't actually get your review tag. If this offends you, please remove 
it. Of course, if you are
willing to provide a review tag, I would greatly appreciate it.


Best regards,
Jacky Huang
Jacky Huang April 14, 2023, 9:04 a.m. UTC | #7
On 2023/4/14 下午 03:46, Krzysztof Kozlowski wrote:
> On 14/04/2023 03:22, Jacky Huang wrote:
>> Dear Krzysztof,
>>
>>
>> On 2023/4/14 上午 01:01, Krzysztof Kozlowski wrote:
>>> On 12/04/2023 07:38, Jacky Huang wrote:
>>>> From: Jacky Huang <ychuang3@nuvoton.com>
>>>>
>>>> Add entry for Nuvoton ma35d1 maintainer and files.
>>>> In addition, update board binding paths of NUVOTON NPCM.
>>>>    ARM/NUVOTON NPCM ARCHITECTURE
>>>>    M:	Avi Fishman <avifishman70@gmail.com>
>>>>    M:	Tomer Maimon <tmaimon77@gmail.com>
>>>> @@ -2512,7 +2522,8 @@ L:	openbmc@lists.ozlabs.org (moderated for non-subscribers)
>>>>    S:	Supported
>>>>    F:	Documentation/devicetree/bindings/*/*/*npcm*
>>>>    F:	Documentation/devicetree/bindings/*/*npcm*
>>>> -F:	Documentation/devicetree/bindings/arm/npcm/*
>>> It is not a bisectable change.
>>>
>>> Best regards,
>>> Krzysztof
>>>
>> F:    Documentation/devicetree/bindings/*/*/*npcm*
>> F:    Documentation/devicetree/bindings/*/*npcm*
>> F:    Documentation/devicetree/bindings/arm/nuvoton/*npcm*    <-- remove
>> this
>> F:    Documentation/devicetree/bindings/soc/nuvoton/*npcm* <-- remove this
>>
>> Yes, the two statements at the bottom seem redundant. I will remove them.
> I did not comment about "redundant". I used the word "bisectable". git
> help bisect.
>
> Best regards,
> Krzysztof
>
Dear Krzysztof,

I do a test on it, but cannot find issues.
Could you please advise on how to duplicate the issue? Thank you.

I do the following 'git bisect' test.

$ git log --oneline
5e186f4f1f94 (HEAD, master) MAINTAINERS: Add entry for NUVOTON MA35
. . .
1a8a804a4f5d Merge tag 'trace-v6.3-rc5-2' of 
git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace


$ git bisect start 5e186f4f1f94 1a8a804a4f5d
  HEAD is now at 5e186f4f1f94 MAINTAINERS: Add entry for NUVOTON MA35
  Bisecting: 68 revisions left to test after this (roughly 6 steps)


Best regards,
Jacky Huang
Lee Jones April 19, 2023, 3:37 p.m. UTC | #8
On Fri, 14 Apr 2023, Jacky Huang wrote:

> 
> 
> On 2023/4/14 下午 03:03, Lee Jones wrote:
> > On Fri, 14 Apr 2023, Jacky Huang wrote:
> > 
> > > Dear Krzysztof,
> > > 
> > > 
> > > On 2023/4/14 上午 12:47, Krzysztof Kozlowski wrote:
> > > > On 12/04/2023 07:38, Jacky Huang wrote:
> > > > > From: Jacky Huang <ychuang3@nuvoton.com>
> > > > > 
> > > > > Add Nuvoton ma35d1 system registers compatible.
> > > > > 
> > > > > Signed-off-by: Jacky Huang <ychuang3@nuvoton.com>
> > > > What about the tag? Why did you ignore it?
> > > > 
> > > > Also, wasn't this applied? Why do you resend (incorrect version)?
> > > > 
> > > > Best regards,
> > > > Krzysztof
> > > > 
> > > When I was making this patchset, this patch was still not merged.
> > > So I'm not sure if I should remove it.
> > > This is just a resend with no updates. And I will remove this patch
> > > in the next version as it was applied.
> > > If possible, please add the following tags for this patch.
> > > 
> > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > I added this.
> > 
> > > Reviewed-by: Lee Jones <lee@kernel.org>
> > When did I provide this?
> > 
> > --
> > Lee Jones [李琼斯]
> 
> Dear Lee,
> 
> Thank you for your help. And, I'm sorry, I thought 'applied' meant
> 'reviewed', and this patch
> didn't actually get your review tag. If this offends you, please remove it.
> Of course, if you are
> willing to provide a review tag, I would greatly appreciate it.

If I replied with "applied" then it's applied and you do not have to
submit it again.
Philipp Zabel April 24, 2023, 7:21 p.m. UTC | #9
Hi Jacky,

On Wed, Apr 12, 2023 at 05:38:22AM +0000, Jacky Huang wrote:
> From: Jacky Huang <ychuang3@nuvoton.com>
> 
> This driver supports individual IP reset for ma35d1. The reset
> control registers is a subset of system control registers.
> 
> Signed-off-by: Jacky Huang <ychuang3@nuvoton.com>
> ---
>  drivers/reset/Kconfig        |   6 +
>  drivers/reset/Makefile       |   1 +
>  drivers/reset/reset-ma35d1.c | 255 +++++++++++++++++++++++++++++++++++
>  3 files changed, 262 insertions(+)
>  create mode 100644 drivers/reset/reset-ma35d1.c
> 
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 2a52c990d4fe..58477c6ca9b8 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -143,6 +143,12 @@ config RESET_NPCM
>  	  This enables the reset controller driver for Nuvoton NPCM
>  	  BMC SoCs.
>  
> +config RESET_NUVOTON_MA35D1
> +	bool "Nuvton MA35D1 Reset Driver"
> +	default ARCH_NUVOTON || COMPILE_TEST
> +	help
> +	  This enables the reset controller driver for Nuvoton MA35D1 SoC.
> +
>  config RESET_OXNAS
>  	bool
>  
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 3e7e5fd633a8..fd52dcf66a99 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -20,6 +20,7 @@ obj-$(CONFIG_RESET_MCHP_SPARX5) += reset-microchip-sparx5.o
>  obj-$(CONFIG_RESET_MESON) += reset-meson.o
>  obj-$(CONFIG_RESET_MESON_AUDIO_ARB) += reset-meson-audio-arb.o
>  obj-$(CONFIG_RESET_NPCM) += reset-npcm.o
> +obj-$(CONFIG_RESET_NUVOTON_MA35D1) += reset-ma35d1.o
>  obj-$(CONFIG_RESET_OXNAS) += reset-oxnas.o
>  obj-$(CONFIG_RESET_PISTACHIO) += reset-pistachio.o
>  obj-$(CONFIG_RESET_POLARFIRE_SOC) += reset-mpfs.o
> diff --git a/drivers/reset/reset-ma35d1.c b/drivers/reset/reset-ma35d1.c
> new file mode 100644
> index 000000000000..57ed710c10f4
> --- /dev/null
> +++ b/drivers/reset/reset-ma35d1.c
> @@ -0,0 +1,255 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2023 Nuvoton Technology Corp.
> + * Author: Chi-Fang Li <cfli0@nuvoton.com>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/reboot.h>
> +#include <linux/reset-controller.h>
> +#include <dt-bindings/reset/nuvoton,ma35d1-reset.h>
> +
> +struct ma35d1_reset_data {
> +	struct reset_controller_dev rcdev;
> +	void __iomem *base;
> +};
> +
> +struct ma35d1_reboot_data {
> +	struct notifier_block restart_handler;
> +	void __iomem *base;
> +};

These two structs can be combined into one, no need to duplicate the
iomem base.

> +
> +static const struct {
> +	unsigned long id;

Why store the id? ids should be contiguous and should start at 0,
so the id could just be an index into the array.

> +	u32 reg_ofs;
> +	u32 bit;
> +} ma35d1_reset_map[] = {
> +	{ MA35D1_RESET_CHIP,    0x20, 0  },
> +	{ MA35D1_RESET_CA35CR0,	0x20, 1  },
> +	{ MA35D1_RESET_CA35CR1, 0x20, 2  },
> +	{ MA35D1_RESET_CM4,     0x20, 3  },
> +	{ MA35D1_RESET_PDMA0,   0x20, 4  },
> +	{ MA35D1_RESET_PDMA1,   0x20, 5  },
> +	{ MA35D1_RESET_PDMA2,   0x20, 6  },
> +	{ MA35D1_RESET_PDMA3,   0x20, 7  },
> +	{ MA35D1_RESET_DISP,    0x20, 9  },
> +	{ MA35D1_RESET_VCAP0,   0x20, 10 },
> +	{ MA35D1_RESET_VCAP1,   0x20, 11 },
> +	{ MA35D1_RESET_GFX,     0x20, 12 },
> +	{ MA35D1_RESET_VDEC,    0x20, 13 },
> +	{ MA35D1_RESET_WHC0,    0x20, 14 },
> +	{ MA35D1_RESET_WHC1,    0x20, 15 },
> +	{ MA35D1_RESET_GMAC0,   0x20, 16 },
> +	{ MA35D1_RESET_GMAC1,   0x20, 17 },
> +	{ MA35D1_RESET_HWSEM,   0x20, 18 },
> +	{ MA35D1_RESET_EBI,     0x20, 19 },
> +	{ MA35D1_RESET_HSUSBH0, 0x20, 20 },
> +	{ MA35D1_RESET_HSUSBH1, 0x20, 21 },
> +	{ MA35D1_RESET_HSUSBD,  0x20, 22 },
> +	{ MA35D1_RESET_USBHL,   0x20, 23 },
> +	{ MA35D1_RESET_SDH0,    0x20, 24 },
> +	{ MA35D1_RESET_SDH1,    0x20, 25 },
> +	{ MA35D1_RESET_NAND,    0x20, 26 },
> +	{ MA35D1_RESET_GPIO,    0x20, 27 },
> +	{ MA35D1_RESET_MCTLP,   0x20, 28 },
> +	{ MA35D1_RESET_MCTLC,   0x20, 29 },
> +	{ MA35D1_RESET_DDRPUB,  0x20, 30 },
> +	{ MA35D1_RESET_TMR0,    0x24, 2  },
> +	{ MA35D1_RESET_TMR1,    0x24, 3  },
> +	{ MA35D1_RESET_TMR2,    0x24, 4  },
> +	{ MA35D1_RESET_TMR3,    0x24, 5  },
> +	{ MA35D1_RESET_I2C0,    0x24, 8  },
> +	{ MA35D1_RESET_I2C1,    0x24, 9  },
> +	{ MA35D1_RESET_I2C2,    0x24, 10 },
> +	{ MA35D1_RESET_I2C3,    0x24, 11 },
> +	{ MA35D1_RESET_QSPI0,   0x24, 12 },
> +	{ MA35D1_RESET_SPI0,    0x24, 13 },
> +	{ MA35D1_RESET_SPI1,    0x24, 14 },
> +	{ MA35D1_RESET_SPI2,    0x24, 15 },
> +	{ MA35D1_RESET_UART0,   0x24, 16 },
> +	{ MA35D1_RESET_UART1,   0x24, 17 },
> +	{ MA35D1_RESET_UART2,   0x24, 18 },
> +	{ MA35D1_RESET_UAER3,   0x24, 19 },
> +	{ MA35D1_RESET_UART4,   0x24, 20 },
> +	{ MA35D1_RESET_UART5,   0x24, 21 },
> +	{ MA35D1_RESET_UART6,   0x24, 22 },
> +	{ MA35D1_RESET_UART7,   0x24, 23 },
> +	{ MA35D1_RESET_CANFD0,  0x24, 24 },
> +	{ MA35D1_RESET_CANFD1,  0x24, 25 },
> +	{ MA35D1_RESET_EADC0,   0x24, 28 },
> +	{ MA35D1_RESET_I2S0,    0x24, 29 },
> +	{ MA35D1_RESET_SC0,     0x28, 0  },
> +	{ MA35D1_RESET_SC1,     0x28, 1  },
> +	{ MA35D1_RESET_QSPI1,   0x28, 4  },
> +	{ MA35D1_RESET_SPI3,    0x28, 6  },
> +	{ MA35D1_RESET_EPWM0,   0x28, 16 },
> +	{ MA35D1_RESET_EPWM1,   0x28, 17 },
> +	{ MA35D1_RESET_QEI0,    0x28, 22 },
> +	{ MA35D1_RESET_QEI1,    0x28, 23 },
> +	{ MA35D1_RESET_ECAP0,   0x28, 26 },
> +	{ MA35D1_RESET_ECAP1,   0x28, 27 },
> +	{ MA35D1_RESET_CANFD2,  0x28, 28 },
> +	{ MA35D1_RESET_ADC0,    0x28, 31 },
> +	{ MA35D1_RESET_TMR4,    0x2C, 0  },
> +	{ MA35D1_RESET_TMR5,    0x2C, 1  },
> +	{ MA35D1_RESET_TMR6,    0x2C, 2  },
> +	{ MA35D1_RESET_TMR7,    0x2C, 3  },
> +	{ MA35D1_RESET_TMR8,    0x2C, 4  },
> +	{ MA35D1_RESET_TMR9,    0x2C, 5  },
> +	{ MA35D1_RESET_TMR10,   0x2C, 6  },
> +	{ MA35D1_RESET_TMR11,   0x2C, 7  },
> +	{ MA35D1_RESET_UART8,   0x2C, 8  },
> +	{ MA35D1_RESET_UART9,   0x2C, 9  },
> +	{ MA35D1_RESET_UART10,  0x2C, 10 },
> +	{ MA35D1_RESET_UART11,  0x2C, 11 },
> +	{ MA35D1_RESET_UART12,  0x2C, 12 },
> +	{ MA35D1_RESET_UART13,  0x2C, 13 },
> +	{ MA35D1_RESET_UART14,  0x2C, 14 },
> +	{ MA35D1_RESET_UART15,  0x2C, 15 },
> +	{ MA35D1_RESET_UART16,  0x2C, 16 },
> +	{ MA35D1_RESET_I2S1,    0x2C, 17 },
> +	{ MA35D1_RESET_I2C4,    0x2C, 18 },
> +	{ MA35D1_RESET_I2C5,    0x2C, 19 },
> +	{ MA35D1_RESET_EPWM2,   0x2C, 20 },
> +	{ MA35D1_RESET_ECAP2,   0x2C, 21 },
> +	{ MA35D1_RESET_QEI2,    0x2C, 22 },
> +	{ MA35D1_RESET_CANFD3,  0x2C, 23 },
> +	{ MA35D1_RESET_KPI,     0x2C, 24 },
> +	{ MA35D1_RESET_GIC,     0x2C, 28 },
> +	{ MA35D1_RESET_SSMCC,   0x2C, 30 },
> +	{ MA35D1_RESET_SSPCC,   0x2C, 31 }
> +};
> +
> +static u32 ma35d1_reset_map_lookup(unsigned long id)
> +{
> +	u32 i;
> +
> +	for (i = 0; i < ARRAY_SIZE(ma35d1_reset_map); i++) {
> +		if (ma35d1_reset_map[i].id == id)
> +			break;
> +	}
> +	return i;
> +}

If you use the id as a look up into the mapping array, this lookup
function wouldn't be necessary.

> +static int ma35d1_restart_handler(struct notifier_block *this,
> +				  unsigned long mode, void *cmd)
> +{
> +	u32 i;
> +	struct ma35d1_reboot_data *data = container_of(this,
> +						       struct ma35d1_reboot_data,
> +						       restart_handler);
> +
> +	i = ma35d1_reset_map_lookup(MA35D1_RESET_CHIP);
> +	writel_relaxed(BIT(ma35d1_reset_map[i].bit),
> +		       data->base + ma35d1_reset_map[i].reg_ofs);
> +	return 0;
> +}
> +
> +static int ma35d1_reset_update(struct reset_controller_dev *rcdev,
> +			       unsigned long id, bool assert)
> +{
> +	u32 i, reg;
> +	struct ma35d1_reset_data *data = container_of(rcdev,
> +						      struct ma35d1_reset_data,
> +						      rcdev);
> +
> +	i = ma35d1_reset_map_lookup(id);
> +	reg = readl_relaxed(data->base + ma35d1_reset_map[i].reg_ofs);
> +	if (assert)
> +		reg |= BIT(ma35d1_reset_map[i].bit);
> +	else
> +		reg &= ~(BIT(ma35d1_reset_map[i].bit));
> +	writel_relaxed(reg, data->base + ma35d1_reset_map[i].reg_ofs);

This requires a spinlock to protect the register from simultaneous
read-modify-writes.

[...]
> +static int ma35d1_reset_probe(struct platform_device *pdev)
> +{
> +	int err;
> +	struct device *dev = &pdev->dev;
> +	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	struct ma35d1_reset_data *reset_data;
> +	struct ma35d1_reboot_data *reboot_data;
> +
> +	if (!pdev->dev.of_node) {
> +		dev_err(&pdev->dev, "Device tree node not found\n");

The driver is only probed via OF, so this isn't reachable and can be
dropped.

regards
Philipp
Jacky Huang April 25, 2023, 1:30 a.m. UTC | #10
On 2023/4/25 上午 03:21, Philipp Zabel wrote:
> Hi Jacky,
>
> On Wed, Apr 12, 2023 at 05:38:22AM +0000, Jacky Huang wrote:
>> From: Jacky Huang <ychuang3@nuvoton.com>
>>
>> This driver supports individual IP reset for ma35d1. The reset
>> control registers is a subset of system control registers.
>>
>> Signed-off-by: Jacky Huang <ychuang3@nuvoton.com>
>> ---
>>   drivers/reset/Kconfig        |   6 +
>>   drivers/reset/Makefile       |   1 +
>>   drivers/reset/reset-ma35d1.c | 255 +++++++++++++++++++++++++++++++++++
>>   3 files changed, 262 insertions(+)
>>   create mode 100644 drivers/reset/reset-ma35d1.c
>>
>> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
>> index 2a52c990d4fe..58477c6ca9b8 100644
>> --- a/drivers/reset/Kconfig
>> +++ b/drivers/reset/Kconfig
>> @@ -143,6 +143,12 @@ config RESET_NPCM
>>   	  This enables the reset controller driver for Nuvoton NPCM
>>   	  BMC SoCs.
>>   
>> +config RESET_NUVOTON_MA35D1
>> +	bool "Nuvton MA35D1 Reset Driver"
>> +	default ARCH_NUVOTON || COMPILE_TEST
>> +	help
>> +	  This enables the reset controller driver for Nuvoton MA35D1 SoC.
>> +
>>   config RESET_OXNAS
>>   	bool
>>   
>> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
>> index 3e7e5fd633a8..fd52dcf66a99 100644
>> --- a/drivers/reset/Makefile
>> +++ b/drivers/reset/Makefile
>> @@ -20,6 +20,7 @@ obj-$(CONFIG_RESET_MCHP_SPARX5) += reset-microchip-sparx5.o
>>   obj-$(CONFIG_RESET_MESON) += reset-meson.o
>>   obj-$(CONFIG_RESET_MESON_AUDIO_ARB) += reset-meson-audio-arb.o
>>   obj-$(CONFIG_RESET_NPCM) += reset-npcm.o
>> +obj-$(CONFIG_RESET_NUVOTON_MA35D1) += reset-ma35d1.o
>>   obj-$(CONFIG_RESET_OXNAS) += reset-oxnas.o
>>   obj-$(CONFIG_RESET_PISTACHIO) += reset-pistachio.o
>>   obj-$(CONFIG_RESET_POLARFIRE_SOC) += reset-mpfs.o
>> diff --git a/drivers/reset/reset-ma35d1.c b/drivers/reset/reset-ma35d1.c
>> new file mode 100644
>> index 000000000000..57ed710c10f4
>> --- /dev/null
>> +++ b/drivers/reset/reset-ma35d1.c
>> @@ -0,0 +1,255 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (C) 2023 Nuvoton Technology Corp.
>> + * Author: Chi-Fang Li <cfli0@nuvoton.com>
>> + */
>> +
>> +#include <linux/device.h>
>> +#include <linux/io.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/reboot.h>
>> +#include <linux/reset-controller.h>
>> +#include <dt-bindings/reset/nuvoton,ma35d1-reset.h>
>> +
>> +struct ma35d1_reset_data {
>> +	struct reset_controller_dev rcdev;
>> +	void __iomem *base;
>> +};
>> +
>> +struct ma35d1_reboot_data {
>> +	struct notifier_block restart_handler;
>> +	void __iomem *base;
>> +};
> These two structs can be combined into one, no need to duplicate the
> iomem base.

Sure, we will combine these two into one structure.

>> +
>> +static const struct {
>> +	unsigned long id;
> Why store the id? ids should be contiguous and should start at 0,
> so the id could just be an index into the array.

Thank you, I didn't notice that the IDs were already consecutive.
The id field is indeed unnecessary, and I will remove it.

>> +	u32 reg_ofs;
>> +	u32 bit;
>> +} ma35d1_reset_map[] = {
>> +	{ MA35D1_RESET_CHIP,    0x20, 0  },
>> +	{ MA35D1_RESET_CA35CR0,	0x20, 1  },
>> +	{ MA35D1_RESET_CA35CR1, 0x20, 2  },
>> +	{ MA35D1_RESET_CM4,     0x20, 3  },
>> +	{ MA35D1_RESET_PDMA0,   0x20, 4  },
>> +	{ MA35D1_RESET_PDMA1,   0x20, 5  },
>> +	{ MA35D1_RESET_PDMA2,   0x20, 6  },
>> +	{ MA35D1_RESET_PDMA3,   0x20, 7  },
>> +	{ MA35D1_RESET_DISP,    0x20, 9  },
>> +	{ MA35D1_RESET_VCAP0,   0x20, 10 },
>> +	{ MA35D1_RESET_VCAP1,   0x20, 11 },
>> +	{ MA35D1_RESET_GFX,     0x20, 12 },
>> +	{ MA35D1_RESET_VDEC,    0x20, 13 },
>> +	{ MA35D1_RESET_WHC0,    0x20, 14 },
>> +	{ MA35D1_RESET_WHC1,    0x20, 15 },
>> +	{ MA35D1_RESET_GMAC0,   0x20, 16 },
>> +	{ MA35D1_RESET_GMAC1,   0x20, 17 },
>> +	{ MA35D1_RESET_HWSEM,   0x20, 18 },
>> +	{ MA35D1_RESET_EBI,     0x20, 19 },
>> +	{ MA35D1_RESET_HSUSBH0, 0x20, 20 },
>> +	{ MA35D1_RESET_HSUSBH1, 0x20, 21 },
>> +	{ MA35D1_RESET_HSUSBD,  0x20, 22 },
>> +	{ MA35D1_RESET_USBHL,   0x20, 23 },
>> +	{ MA35D1_RESET_SDH0,    0x20, 24 },
>> +	{ MA35D1_RESET_SDH1,    0x20, 25 },
>> +	{ MA35D1_RESET_NAND,    0x20, 26 },
>> +	{ MA35D1_RESET_GPIO,    0x20, 27 },
>> +	{ MA35D1_RESET_MCTLP,   0x20, 28 },
>> +	{ MA35D1_RESET_MCTLC,   0x20, 29 },
>> +	{ MA35D1_RESET_DDRPUB,  0x20, 30 },
>> +	{ MA35D1_RESET_TMR0,    0x24, 2  },
>> +	{ MA35D1_RESET_TMR1,    0x24, 3  },
>> +	{ MA35D1_RESET_TMR2,    0x24, 4  },
>> +	{ MA35D1_RESET_TMR3,    0x24, 5  },
>> +	{ MA35D1_RESET_I2C0,    0x24, 8  },
>> +	{ MA35D1_RESET_I2C1,    0x24, 9  },
>> +	{ MA35D1_RESET_I2C2,    0x24, 10 },
>> +	{ MA35D1_RESET_I2C3,    0x24, 11 },
>> +	{ MA35D1_RESET_QSPI0,   0x24, 12 },
>> +	{ MA35D1_RESET_SPI0,    0x24, 13 },
>> +	{ MA35D1_RESET_SPI1,    0x24, 14 },
>> +	{ MA35D1_RESET_SPI2,    0x24, 15 },
>> +	{ MA35D1_RESET_UART0,   0x24, 16 },
>> +	{ MA35D1_RESET_UART1,   0x24, 17 },
>> +	{ MA35D1_RESET_UART2,   0x24, 18 },
>> +	{ MA35D1_RESET_UAER3,   0x24, 19 },
>> +	{ MA35D1_RESET_UART4,   0x24, 20 },
>> +	{ MA35D1_RESET_UART5,   0x24, 21 },
>> +	{ MA35D1_RESET_UART6,   0x24, 22 },
>> +	{ MA35D1_RESET_UART7,   0x24, 23 },
>> +	{ MA35D1_RESET_CANFD0,  0x24, 24 },
>> +	{ MA35D1_RESET_CANFD1,  0x24, 25 },
>> +	{ MA35D1_RESET_EADC0,   0x24, 28 },
>> +	{ MA35D1_RESET_I2S0,    0x24, 29 },
>> +	{ MA35D1_RESET_SC0,     0x28, 0  },
>> +	{ MA35D1_RESET_SC1,     0x28, 1  },
>> +	{ MA35D1_RESET_QSPI1,   0x28, 4  },
>> +	{ MA35D1_RESET_SPI3,    0x28, 6  },
>> +	{ MA35D1_RESET_EPWM0,   0x28, 16 },
>> +	{ MA35D1_RESET_EPWM1,   0x28, 17 },
>> +	{ MA35D1_RESET_QEI0,    0x28, 22 },
>> +	{ MA35D1_RESET_QEI1,    0x28, 23 },
>> +	{ MA35D1_RESET_ECAP0,   0x28, 26 },
>> +	{ MA35D1_RESET_ECAP1,   0x28, 27 },
>> +	{ MA35D1_RESET_CANFD2,  0x28, 28 },
>> +	{ MA35D1_RESET_ADC0,    0x28, 31 },
>> +	{ MA35D1_RESET_TMR4,    0x2C, 0  },
>> +	{ MA35D1_RESET_TMR5,    0x2C, 1  },
>> +	{ MA35D1_RESET_TMR6,    0x2C, 2  },
>> +	{ MA35D1_RESET_TMR7,    0x2C, 3  },
>> +	{ MA35D1_RESET_TMR8,    0x2C, 4  },
>> +	{ MA35D1_RESET_TMR9,    0x2C, 5  },
>> +	{ MA35D1_RESET_TMR10,   0x2C, 6  },
>> +	{ MA35D1_RESET_TMR11,   0x2C, 7  },
>> +	{ MA35D1_RESET_UART8,   0x2C, 8  },
>> +	{ MA35D1_RESET_UART9,   0x2C, 9  },
>> +	{ MA35D1_RESET_UART10,  0x2C, 10 },
>> +	{ MA35D1_RESET_UART11,  0x2C, 11 },
>> +	{ MA35D1_RESET_UART12,  0x2C, 12 },
>> +	{ MA35D1_RESET_UART13,  0x2C, 13 },
>> +	{ MA35D1_RESET_UART14,  0x2C, 14 },
>> +	{ MA35D1_RESET_UART15,  0x2C, 15 },
>> +	{ MA35D1_RESET_UART16,  0x2C, 16 },
>> +	{ MA35D1_RESET_I2S1,    0x2C, 17 },
>> +	{ MA35D1_RESET_I2C4,    0x2C, 18 },
>> +	{ MA35D1_RESET_I2C5,    0x2C, 19 },
>> +	{ MA35D1_RESET_EPWM2,   0x2C, 20 },
>> +	{ MA35D1_RESET_ECAP2,   0x2C, 21 },
>> +	{ MA35D1_RESET_QEI2,    0x2C, 22 },
>> +	{ MA35D1_RESET_CANFD3,  0x2C, 23 },
>> +	{ MA35D1_RESET_KPI,     0x2C, 24 },
>> +	{ MA35D1_RESET_GIC,     0x2C, 28 },
>> +	{ MA35D1_RESET_SSMCC,   0x2C, 30 },
>> +	{ MA35D1_RESET_SSPCC,   0x2C, 31 }
>> +};
>> +
>> +static u32 ma35d1_reset_map_lookup(unsigned long id)
>> +{
>> +	u32 i;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(ma35d1_reset_map); i++) {
>> +		if (ma35d1_reset_map[i].id == id)
>> +			break;
>> +	}
>> +	return i;
>> +}
> If you use the id as a look up into the mapping array, this lookup
> function wouldn't be necessary.

Yes, it's not necessary. I will modify it.

>> +static int ma35d1_restart_handler(struct notifier_block *this,
>> +				  unsigned long mode, void *cmd)
>> +{
>> +	u32 i;
>> +	struct ma35d1_reboot_data *data = container_of(this,
>> +						       struct ma35d1_reboot_data,
>> +						       restart_handler);
>> +
>> +	i = ma35d1_reset_map_lookup(MA35D1_RESET_CHIP);
>> +	writel_relaxed(BIT(ma35d1_reset_map[i].bit),
>> +		       data->base + ma35d1_reset_map[i].reg_ofs);
>> +	return 0;
>> +}
>> +
>> +static int ma35d1_reset_update(struct reset_controller_dev *rcdev,
>> +			       unsigned long id, bool assert)
>> +{
>> +	u32 i, reg;
>> +	struct ma35d1_reset_data *data = container_of(rcdev,
>> +						      struct ma35d1_reset_data,
>> +						      rcdev);
>> +
>> +	i = ma35d1_reset_map_lookup(id);
>> +	reg = readl_relaxed(data->base + ma35d1_reset_map[i].reg_ofs);
>> +	if (assert)
>> +		reg |= BIT(ma35d1_reset_map[i].bit);
>> +	else
>> +		reg &= ~(BIT(ma35d1_reset_map[i].bit));
>> +	writel_relaxed(reg, data->base + ma35d1_reset_map[i].reg_ofs);
> This requires a spinlock to protect the register from simultaneous
> read-modify-writes.

OK, I will add spinlock protect for it.

> [...]
>> +static int ma35d1_reset_probe(struct platform_device *pdev)
>> +{
>> +	int err;
>> +	struct device *dev = &pdev->dev;
>> +	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	struct ma35d1_reset_data *reset_data;
>> +	struct ma35d1_reboot_data *reboot_data;
>> +
>> +	if (!pdev->dev.of_node) {
>> +		dev_err(&pdev->dev, "Device tree node not found\n");
> The driver is only probed via OF, so this isn't reachable and can be
> dropped.
Okay, I will remove this check.
> regards
> Philipp

Thank you for the comments.


Best regards,
Jacky Huang
Ilpo Järvinen April 25, 2023, 7:40 a.m. UTC | #11
On Tue, 25 Apr 2023, Jacky Huang wrote:

> 
> 
> On 2023/4/25 上午 03:21, Philipp Zabel wrote:
> > Hi Jacky,
> > 
> > On Wed, Apr 12, 2023 at 05:38:22AM +0000, Jacky Huang wrote:
> > > From: Jacky Huang <ychuang3@nuvoton.com>
> > > 
> > > This driver supports individual IP reset for ma35d1. The reset
> > > control registers is a subset of system control registers.
> > > 
> > > Signed-off-by: Jacky Huang <ychuang3@nuvoton.com>
> > > ---
> > > 
> > > +static const struct {
> > > +	unsigned long id;
> > Why store the id? ids should be contiguous and should start at 0,
> > so the id could just be an index into the array.
> 
> Thank you, I didn't notice that the IDs were already consecutive.
> The id field is indeed unnecessary, and I will remove it.

I recommend you still keep the IDs in the array initializer though, like 
this:

...
} ma35d1_reset_map[] = {
	[MA35D1_RESET_CHIP] = {0x20, 0},
	[MA35D1_RESET_CA35CR0] = {0x20, 1},
	...

> > > +	u32 reg_ofs;
> > > +	u32 bit;
> > > +} ma35d1_reset_map[] = {
> > > +	{ MA35D1_RESET_CHIP,    0x20, 0  },
> > > +	{ MA35D1_RESET_CA35CR0,	0x20, 1  },
> > > +	{ MA35D1_RESET_CA35CR1, 0x20, 2  },
> > > +	{ MA35D1_RESET_CM4,     0x20, 3  },
> > > +	{ MA35D1_RESET_PDMA0,   0x20, 4  },
> > > +	{ MA35D1_RESET_PDMA1,   0x20, 5  },
> > > +	{ MA35D1_RESET_PDMA2,   0x20, 6  },
> > > +	{ MA35D1_RESET_PDMA3,   0x20, 7  },
> > > +	{ MA35D1_RESET_DISP,    0x20, 9  },
> > > +	{ MA35D1_RESET_VCAP0,   0x20, 10 },
> > > +	{ MA35D1_RESET_VCAP1,   0x20, 11 },
> > > +	{ MA35D1_RESET_GFX,     0x20, 12 },
> > > +	{ MA35D1_RESET_VDEC,    0x20, 13 },
> > > +	{ MA35D1_RESET_WHC0,    0x20, 14 },
> > > +	{ MA35D1_RESET_WHC1,    0x20, 15 },
> > > +	{ MA35D1_RESET_GMAC0,   0x20, 16 },
> > > +	{ MA35D1_RESET_GMAC1,   0x20, 17 },
> > > +	{ MA35D1_RESET_HWSEM,   0x20, 18 },
> > > +	{ MA35D1_RESET_EBI,     0x20, 19 },
> > > +	{ MA35D1_RESET_HSUSBH0, 0x20, 20 },
> > > +	{ MA35D1_RESET_HSUSBH1, 0x20, 21 },
> > > +	{ MA35D1_RESET_HSUSBD,  0x20, 22 },
> > > +	{ MA35D1_RESET_USBHL,   0x20, 23 },
> > > +	{ MA35D1_RESET_SDH0,    0x20, 24 },
> > > +	{ MA35D1_RESET_SDH1,    0x20, 25 },
> > > +	{ MA35D1_RESET_NAND,    0x20, 26 },
> > > +	{ MA35D1_RESET_GPIO,    0x20, 27 },
> > > +	{ MA35D1_RESET_MCTLP,   0x20, 28 },
> > > +	{ MA35D1_RESET_MCTLC,   0x20, 29 },
> > > +	{ MA35D1_RESET_DDRPUB,  0x20, 30 },
> > > +	{ MA35D1_RESET_TMR0,    0x24, 2  },
> > > +	{ MA35D1_RESET_TMR1,    0x24, 3  },
> > > +	{ MA35D1_RESET_TMR2,    0x24, 4  },
> > > +	{ MA35D1_RESET_TMR3,    0x24, 5  },
> > > +	{ MA35D1_RESET_I2C0,    0x24, 8  },
> > > +	{ MA35D1_RESET_I2C1,    0x24, 9  },
> > > +	{ MA35D1_RESET_I2C2,    0x24, 10 },
> > > +	{ MA35D1_RESET_I2C3,    0x24, 11 },
> > > +	{ MA35D1_RESET_QSPI0,   0x24, 12 },
> > > +	{ MA35D1_RESET_SPI0,    0x24, 13 },
> > > +	{ MA35D1_RESET_SPI1,    0x24, 14 },
> > > +	{ MA35D1_RESET_SPI2,    0x24, 15 },
> > > +	{ MA35D1_RESET_UART0,   0x24, 16 },
> > > +	{ MA35D1_RESET_UART1,   0x24, 17 },
> > > +	{ MA35D1_RESET_UART2,   0x24, 18 },
> > > +	{ MA35D1_RESET_UAER3,   0x24, 19 },
> > > +	{ MA35D1_RESET_UART4,   0x24, 20 },
> > > +	{ MA35D1_RESET_UART5,   0x24, 21 },
> > > +	{ MA35D1_RESET_UART6,   0x24, 22 },
> > > +	{ MA35D1_RESET_UART7,   0x24, 23 },
> > > +	{ MA35D1_RESET_CANFD0,  0x24, 24 },
> > > +	{ MA35D1_RESET_CANFD1,  0x24, 25 },
> > > +	{ MA35D1_RESET_EADC0,   0x24, 28 },
> > > +	{ MA35D1_RESET_I2S0,    0x24, 29 },
> > > +	{ MA35D1_RESET_SC0,     0x28, 0  },
> > > +	{ MA35D1_RESET_SC1,     0x28, 1  },
> > > +	{ MA35D1_RESET_QSPI1,   0x28, 4  },
> > > +	{ MA35D1_RESET_SPI3,    0x28, 6  },
> > > +	{ MA35D1_RESET_EPWM0,   0x28, 16 },
> > > +	{ MA35D1_RESET_EPWM1,   0x28, 17 },
> > > +	{ MA35D1_RESET_QEI0,    0x28, 22 },
> > > +	{ MA35D1_RESET_QEI1,    0x28, 23 },
> > > +	{ MA35D1_RESET_ECAP0,   0x28, 26 },
> > > +	{ MA35D1_RESET_ECAP1,   0x28, 27 },
> > > +	{ MA35D1_RESET_CANFD2,  0x28, 28 },
> > > +	{ MA35D1_RESET_ADC0,    0x28, 31 },
> > > +	{ MA35D1_RESET_TMR4,    0x2C, 0  },
> > > +	{ MA35D1_RESET_TMR5,    0x2C, 1  },
> > > +	{ MA35D1_RESET_TMR6,    0x2C, 2  },
> > > +	{ MA35D1_RESET_TMR7,    0x2C, 3  },
> > > +	{ MA35D1_RESET_TMR8,    0x2C, 4  },
> > > +	{ MA35D1_RESET_TMR9,    0x2C, 5  },
> > > +	{ MA35D1_RESET_TMR10,   0x2C, 6  },
> > > +	{ MA35D1_RESET_TMR11,   0x2C, 7  },
> > > +	{ MA35D1_RESET_UART8,   0x2C, 8  },
> > > +	{ MA35D1_RESET_UART9,   0x2C, 9  },
> > > +	{ MA35D1_RESET_UART10,  0x2C, 10 },
> > > +	{ MA35D1_RESET_UART11,  0x2C, 11 },
> > > +	{ MA35D1_RESET_UART12,  0x2C, 12 },
> > > +	{ MA35D1_RESET_UART13,  0x2C, 13 },
> > > +	{ MA35D1_RESET_UART14,  0x2C, 14 },
> > > +	{ MA35D1_RESET_UART15,  0x2C, 15 },
> > > +	{ MA35D1_RESET_UART16,  0x2C, 16 },
> > > +	{ MA35D1_RESET_I2S1,    0x2C, 17 },
> > > +	{ MA35D1_RESET_I2C4,    0x2C, 18 },
> > > +	{ MA35D1_RESET_I2C5,    0x2C, 19 },
> > > +	{ MA35D1_RESET_EPWM2,   0x2C, 20 },
> > > +	{ MA35D1_RESET_ECAP2,   0x2C, 21 },
> > > +	{ MA35D1_RESET_QEI2,    0x2C, 22 },
> > > +	{ MA35D1_RESET_CANFD3,  0x2C, 23 },
> > > +	{ MA35D1_RESET_KPI,     0x2C, 24 },
> > > +	{ MA35D1_RESET_GIC,     0x2C, 28 },
> > > +	{ MA35D1_RESET_SSMCC,   0x2C, 30 },
> > > +	{ MA35D1_RESET_SSPCC,   0x2C, 31 }
> > > +};
Jacky Huang April 25, 2023, 8:07 a.m. UTC | #12
Dear Ilpo,



On 2023/4/25 下午 03:40, Ilpo Järvinen wrote:
> On Tue, 25 Apr 2023, Jacky Huang wrote:
>
>>
>> On 2023/4/25 上午 03:21, Philipp Zabel wrote:
>>> Hi Jacky,
>>>
>>> On Wed, Apr 12, 2023 at 05:38:22AM +0000, Jacky Huang wrote:
>>>> From: Jacky Huang <ychuang3@nuvoton.com>
>>>>
>>>> This driver supports individual IP reset for ma35d1. The reset
>>>> control registers is a subset of system control registers.
>>>>
>>>> Signed-off-by: Jacky Huang <ychuang3@nuvoton.com>
>>>> ---
>>>>
>>>> +static const struct {
>>>> +	unsigned long id;
>>> Why store the id? ids should be contiguous and should start at 0,
>>> so the id could just be an index into the array.
>> Thank you, I didn't notice that the IDs were already consecutive.
>> The id field is indeed unnecessary, and I will remove it.
> I recommend you still keep the IDs in the array initializer though, like
> this:
>
> ...
> } ma35d1_reset_map[] = {
> 	[MA35D1_RESET_CHIP] = {0x20, 0},
> 	[MA35D1_RESET_CA35CR0] = {0x20, 1},
> 	...

Okay, I will modify the code like this.

>>>> +	u32 reg_ofs;
>>>> +	u32 bit;
>>>> +} ma35d1_reset_map[] = {
>>>> +	{ MA35D1_RESET_CHIP,    0x20, 0  },
>>>> +	{ MA35D1_RESET_CA35CR0,	0x20, 1  },
>>>> +	{ MA35D1_RESET_CA35CR1, 0x20, 2  },
>>>> +	{ MA35D1_RESET_CM4,     0x20, 3  },
>>>> +	{ MA35D1_RESET_PDMA0,   0x20, 4  },
>>>> +	{ MA35D1_RESET_PDMA1,   0x20, 5  },
>>>> +	{ MA35D1_RESET_PDMA2,   0x20, 6  },
>>>> +	{ MA35D1_RESET_PDMA3,   0x20, 7  },
>>>> +	{ MA35D1_RESET_DISP,    0x20, 9  },
>>>> +	{ MA35D1_RESET_VCAP0,   0x20, 10 },
>>>> +	{ MA35D1_RESET_VCAP1,   0x20, 11 },
>>>> +	{ MA35D1_RESET_GFX,     0x20, 12 },
>>>> +	{ MA35D1_RESET_VDEC,    0x20, 13 },
>>>> +	{ MA35D1_RESET_WHC0,    0x20, 14 },
>>>> +	{ MA35D1_RESET_WHC1,    0x20, 15 },
>>>> +	{ MA35D1_RESET_GMAC0,   0x20, 16 },
>>>> +	{ MA35D1_RESET_GMAC1,   0x20, 17 },
>>>> +	{ MA35D1_RESET_HWSEM,   0x20, 18 },
>>>> +	{ MA35D1_RESET_EBI,     0x20, 19 },
>>>> +	{ MA35D1_RESET_HSUSBH0, 0x20, 20 },
>>>> +	{ MA35D1_RESET_HSUSBH1, 0x20, 21 },
>>>> +	{ MA35D1_RESET_HSUSBD,  0x20, 22 },
>>>> +	{ MA35D1_RESET_USBHL,   0x20, 23 },
>>>> +	{ MA35D1_RESET_SDH0,    0x20, 24 },
>>>> +	{ MA35D1_RESET_SDH1,    0x20, 25 },
>>>> +	{ MA35D1_RESET_NAND,    0x20, 26 },
>>>> +	{ MA35D1_RESET_GPIO,    0x20, 27 },
>>>> +	{ MA35D1_RESET_MCTLP,   0x20, 28 },
>>>> +	{ MA35D1_RESET_MCTLC,   0x20, 29 },
>>>> +	{ MA35D1_RESET_DDRPUB,  0x20, 30 },
>>>> +	{ MA35D1_RESET_TMR0,    0x24, 2  },
>>>> +	{ MA35D1_RESET_TMR1,    0x24, 3  },
>>>> +	{ MA35D1_RESET_TMR2,    0x24, 4  },
>>>> +	{ MA35D1_RESET_TMR3,    0x24, 5  },
>>>> +	{ MA35D1_RESET_I2C0,    0x24, 8  },
>>>> +	{ MA35D1_RESET_I2C1,    0x24, 9  },
>>>> +	{ MA35D1_RESET_I2C2,    0x24, 10 },
>>>> +	{ MA35D1_RESET_I2C3,    0x24, 11 },
>>>> +	{ MA35D1_RESET_QSPI0,   0x24, 12 },
>>>> +	{ MA35D1_RESET_SPI0,    0x24, 13 },
>>>> +	{ MA35D1_RESET_SPI1,    0x24, 14 },
>>>> +	{ MA35D1_RESET_SPI2,    0x24, 15 },
>>>> +	{ MA35D1_RESET_UART0,   0x24, 16 },
>>>> +	{ MA35D1_RESET_UART1,   0x24, 17 },
>>>> +	{ MA35D1_RESET_UART2,   0x24, 18 },
>>>> +	{ MA35D1_RESET_UAER3,   0x24, 19 },
>>>> +	{ MA35D1_RESET_UART4,   0x24, 20 },
>>>> +	{ MA35D1_RESET_UART5,   0x24, 21 },
>>>> +	{ MA35D1_RESET_UART6,   0x24, 22 },
>>>> +	{ MA35D1_RESET_UART7,   0x24, 23 },
>>>> +	{ MA35D1_RESET_CANFD0,  0x24, 24 },
>>>> +	{ MA35D1_RESET_CANFD1,  0x24, 25 },
>>>> +	{ MA35D1_RESET_EADC0,   0x24, 28 },
>>>> +	{ MA35D1_RESET_I2S0,    0x24, 29 },
>>>> +	{ MA35D1_RESET_SC0,     0x28, 0  },
>>>> +	{ MA35D1_RESET_SC1,     0x28, 1  },
>>>> +	{ MA35D1_RESET_QSPI1,   0x28, 4  },
>>>> +	{ MA35D1_RESET_SPI3,    0x28, 6  },
>>>> +	{ MA35D1_RESET_EPWM0,   0x28, 16 },
>>>> +	{ MA35D1_RESET_EPWM1,   0x28, 17 },
>>>> +	{ MA35D1_RESET_QEI0,    0x28, 22 },
>>>> +	{ MA35D1_RESET_QEI1,    0x28, 23 },
>>>> +	{ MA35D1_RESET_ECAP0,   0x28, 26 },
>>>> +	{ MA35D1_RESET_ECAP1,   0x28, 27 },
>>>> +	{ MA35D1_RESET_CANFD2,  0x28, 28 },
>>>> +	{ MA35D1_RESET_ADC0,    0x28, 31 },
>>>> +	{ MA35D1_RESET_TMR4,    0x2C, 0  },
>>>> +	{ MA35D1_RESET_TMR5,    0x2C, 1  },
>>>> +	{ MA35D1_RESET_TMR6,    0x2C, 2  },
>>>> +	{ MA35D1_RESET_TMR7,    0x2C, 3  },
>>>> +	{ MA35D1_RESET_TMR8,    0x2C, 4  },
>>>> +	{ MA35D1_RESET_TMR9,    0x2C, 5  },
>>>> +	{ MA35D1_RESET_TMR10,   0x2C, 6  },
>>>> +	{ MA35D1_RESET_TMR11,   0x2C, 7  },
>>>> +	{ MA35D1_RESET_UART8,   0x2C, 8  },
>>>> +	{ MA35D1_RESET_UART9,   0x2C, 9  },
>>>> +	{ MA35D1_RESET_UART10,  0x2C, 10 },
>>>> +	{ MA35D1_RESET_UART11,  0x2C, 11 },
>>>> +	{ MA35D1_RESET_UART12,  0x2C, 12 },
>>>> +	{ MA35D1_RESET_UART13,  0x2C, 13 },
>>>> +	{ MA35D1_RESET_UART14,  0x2C, 14 },
>>>> +	{ MA35D1_RESET_UART15,  0x2C, 15 },
>>>> +	{ MA35D1_RESET_UART16,  0x2C, 16 },
>>>> +	{ MA35D1_RESET_I2S1,    0x2C, 17 },
>>>> +	{ MA35D1_RESET_I2C4,    0x2C, 18 },
>>>> +	{ MA35D1_RESET_I2C5,    0x2C, 19 },
>>>> +	{ MA35D1_RESET_EPWM2,   0x2C, 20 },
>>>> +	{ MA35D1_RESET_ECAP2,   0x2C, 21 },
>>>> +	{ MA35D1_RESET_QEI2,    0x2C, 22 },
>>>> +	{ MA35D1_RESET_CANFD3,  0x2C, 23 },
>>>> +	{ MA35D1_RESET_KPI,     0x2C, 24 },
>>>> +	{ MA35D1_RESET_GIC,     0x2C, 28 },
>>>> +	{ MA35D1_RESET_SSMCC,   0x2C, 30 },
>>>> +	{ MA35D1_RESET_SSPCC,   0x2C, 31 }
>>>> +};
Best Regards,
Jacky Huang