mbox series

[00/14] ARM: suniv: dts: update Allwinner F1C100

Message ID 20220307143421.1106209-1-andre.przywara@arm.com
Headers show
Series ARM: suniv: dts: update Allwinner F1C100 | expand

Message

Andre Przywara March 7, 2022, 2:34 p.m. UTC
The Allwinner F1C100 SoC didn't see much love since its initial merge in
2018: the originally submitted .dts files were very basic, and didn't
cover such simple peripherals as MMC and SPI.
On top of that the watchdog compatible string was wrong, leading to a
non-functional watchdog and reset functionality.

This series aims to fix that, after the series MMC and SPI work, and
make dtbs_check comes back clean.
This was tested with mounting a filesystem on /dev/mmcblk0 on a
LicheePi Nano, also with accessing the SPI flash through /dev/mtdblock
and mtd_debug. Reboot and watchdog now also work.

Mainline U-Boot recently gained F1C100 support, and those DT updates are
needed there as well to get full MMC and SPI access.

The series is structured as follows:
- Patches 01/14 and 02/14 fix the watchdog, which allows to properly
  reboot the system.
- Patches 03-06 fix some shortcomings of the existing DT files, to make
  them DT binding compliant.
- Patches 07-09 are Jesse's recent MMC patches, with the comments from
  the last version addressed [1].
- Patches 10-12 add SPI support, to enable access to the SPI flash on
  the LicheePi Nano board.
- The final two patches (13/14 and 14/14) add the F1C100 platform to
  the multi_v5_defconfig, since it was not covered by any other
  defconfig before, and an ARMv5 compliant kernel is not commonly
  offered by distributions.

I saw George's series from two years ago to add USB support[2], that
looks good on the first glance, I will comment on that once I did some
testing on that.

Cheers,
Andre

Changelog for the MMC patches [1]:
- bindings doc: extend commit message
- .dtsi: extend commit message, re-order mmc0_pins node, add
  drive-strength
- .dts: extend commit message, add alias, regulator and disable-wp

[1] https://lore.kernel.org/linux-arm-kernel/20220130220325.1983918-1-Mr.Bossman075@gmail.com/
[2] https://lore.kernel.org/linux-usb/20200331170219.267732-1-thirtythreeforty@gmail.com/

Andre Przywara (10):
  dt-bindings: watchdog: sunxi: fix F1C100s compatible
  ARM: dts: suniv: F1C100: fix watchdog compatible
  dt-bindings: arm: sunxi: document LicheePi Nano name
  ARM: dts: suniv: F1C100: fix CPU node
  ARM: dts: suniv: F1C100: fix timer node
  dt-bindings: spi: sunxi: document F1C100 controllers
  ARM: dts: suniv: F1C100: add SPI support
  ARM: dts: suniv: licheepi-nano: add SPI flash
  ARM: configs: sync multi_v5_defconfig from savedefconfig
  ARM: configs: multi_v5: Enable Allwinner F1C100

Jesse Taube (4):
  ARM: dts: suniv: F1C100: add clock and reset macros
  dt-bindings: mmc: sunxi: add Allwinner F1c100s compatible
  ARM: dts: suniv: F1C100: add MMC controllers
  ARM: dts: suniv: licheepi-nano: add microSD card

 .../devicetree/bindings/arm/sunxi.yaml        |   5 +
 .../bindings/mmc/allwinner,sun4i-a10-mmc.yaml |   3 +
 .../bindings/spi/allwinner,sun6i-a31-spi.yaml |   1 +
 .../watchdog/allwinner,sun4i-a10-wdt.yaml     |   2 +-
 .../boot/dts/suniv-f1c100s-licheepi-nano.dts  |  31 ++++++
 arch/arm/boot/dts/suniv-f1c100s.dtsi          | 102 ++++++++++++++++--
 arch/arm/configs/multi_v5_defconfig           |  25 ++---
 7 files changed, 140 insertions(+), 29 deletions(-)

Comments

Icenowy Zheng March 8, 2022, 4:23 a.m. UTC | #1
在 2022-03-07星期一的 21:44 -0500,Jesse Taube写道:
> 
> 
> On 3/7/22 09:34, Andre Przywara wrote:
> > The /cpu node in the f1c100s.dtsi is not spec compliant, it's
> > missing
> > the reg property, and the corresponding address and size cells
> > properties.
> > 
> > Add them to make the bindings check pass.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >   arch/arm/boot/dts/suniv-f1c100s.dtsi | 4 ++++
> >   1 file changed, 4 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/suniv-f1c100s.dtsi
> > b/arch/arm/boot/dts/suniv-f1c100s.dtsi
> > index 922efd5e9457..43d342eaf661 100644
> > --- a/arch/arm/boot/dts/suniv-f1c100s.dtsi
> > +++ b/arch/arm/boot/dts/suniv-f1c100s.dtsi
> > @@ -29,9 +29,13 @@ osc32k: clk-32k {
> >         };
> >   
> >         cpus {
> > +               #address-cells = <1>;
> > +               #size-cells = <0>;
> > +
> >                 cpu {
> cpu@0
> 
> also is memory node required?

It should be filled by U-Boot.

> 
> Thanks,
> jesse
> >                         compatible = "arm,arm926ej-s";
> >                         device_type = "cpu";
> > +                       reg = <0x0>;
> >                 };
> >         };
> >
Andre Przywara March 8, 2022, 12:07 p.m. UTC | #2
On Tue, 8 Mar 2022 10:38:37 +0100
Arnd Bergmann <arnd@arndb.de> wrote:

Hi Arnd,

thanks for having a look. I was a bit unsure about the policy of those
changes, so glad to have the discussion.

> On Mon, Mar 7, 2022 at 3:34 PM Andre Przywara <andre.przywara@arm.com> wrote:
> >
> > Some Kconfig options have changed, some other platforms have been
> > removed.  
> 
> Please split this up into logical chunks: list the platforms that were removed
> and remove only the lines corresponding to those platforms in one patch,
> do functional changes in separate patches each with a reason for doing them,
> and cleanups (moving lines to match the savedefconfig output, removing lines
> that are now the default) in one final patch.

Actually this patch is meant to be only about the last past: to sync
multi_v5_defconfig with the output of "make savedefconfig". .config stays
the same. I initially tried to chase down the reason for each line change,
but gave up quickly, because it becomes tedious to learn about this,
especially about symbols that got *removed*. Also Kconfig is somewhat
sensitive, a single "select" or "default" change in one random Kconfig can
affect the result of savedefconfig.

As I noted in the commit message, the .config does *not* change as a result
of this patch, the whole purpose is just to make the next patch clearer.

So I can try find the reason for each removal, if you like, but I am not
sure that's worthwhile? It is my understanding that Kconfig changes tend
to accumulate cruft in the various defconfigs over time. In U-Boot we gave
up on reasoning, and just regularly sync the output of savedefconfig over
to the *_defconfigs, to keep them minimal and meaningful.

And I found Olof's commit 30b10c77837c ("ARM: defconfig: re-run
savedefconfig on multi_v* configs") as a precedence for this kind of cleanup.

> >  CONFIG_AEABI=y
> >  CONFIG_HIGHMEM=y
> > -CONFIG_ZBOOT_ROM_TEXT=0x0
> > -CONFIG_ZBOOT_ROM_BSS=0x0
> >  CONFIG_ARM_APPENDED_DTB=y
> >  CONFIG_ARM_ATAG_DTB_COMPAT=y
> >  CONFIG_CPU_FREQ=y  
> 
> These were not removed, what happened here is that 'savedefconfig'
> no longer produces the lines because they now match the defaults.

Yes, I understand. Is there some policy here, for instance to keep
those in, for clarity?

> > @@ -159,7 +151,6 @@ CONFIG_I2C_ASPEED=m
> >  CONFIG_I2C_AT91=y
> >  CONFIG_I2C_IMX=y
> >  CONFIG_I2C_MV64XXX=y
> > -CONFIG_I2C_NOMADIK=y
> >  CONFIG_SPI=y
> >  CONFIG_SPI_ATMEL=y
> >  CONFIG_SPI_IMX=y  
> 
> This one is still there. Not sure why it's no longer enabled.

It's not in the current .config. From what I can see, it depends on
ARCH_AMBA, which is selected by ARCH_NOMADIK, but that one is not enabled
by multi_v5_defconfig. Not sure if that is an oversight, or a change, a
the dependency is bogus, or something else.

If you find that useful, I can try to find those dependency chains for the
other options, but I definitely lack the knowledge about the history of
those old platforms, so I can't reason about them. But I could present you
the findings and you can then say what to do?

> >  CONFIG_REGULATOR_FIXED_VOLTAGE=y
> >  CONFIG_MEDIA_SUPPORT=y
> >  CONFIG_MEDIA_CAMERA_SUPPORT=y
> > -CONFIG_V4L_PLATFORM_DRIVERS=y
> > -CONFIG_VIDEO_ASPEED=m
> > -CONFIG_VIDEO_ATMEL_ISI=m
> >  CONFIG_DRM=y
> >  CONFIG_DRM_ATMEL_HLCDC=m
> > -CONFIG_DRM_PANEL_SIMPLE=y
> > -CONFIG_DRM_PANEL_EDP=y
> >  CONFIG_DRM_ASPEED_GFX=m
> > -CONFIG_FB_IMX=y
> > -CONFIG_FB_ATMEL=y
> > -CONFIG_BACKLIGHT_ATMEL_LCDC=y  
> 
> This doesn't look right at all. If you want to disable graphics support,
> please do that in a separate patch and explain why we can't have those
> any more. Are you running into problems with the vmlinux size?

As I mentioned, the .config didn't change at all, so those options are
already not included in mainline anymore.

AFAICS, those last options depend on CONFIG_FB, which isn't enabled. Is
that a regression due to the recent fbdev changes?

Cheers,
Andre

> 
> >  CONFIG_LIBCRC32C=y
> >  CONFIG_DEBUG_INFO=y
> > -CONFIG_DEBUG_FS=y
> >  CONFIG_MAGIC_SYSRQ=y
> > +CONFIG_DEBUG_FS=y
> >  CONFIG_DEBUG_KERNEL=y
> >  # CONFIG_SCHED_DEBUG is not set
> >  # CONFIG_DEBUG_PREEMPT is not set  
> 
> This should probably go along with the ZBOOT_ROM change, it's
> only cosmetic.
> 
>         Arnd
Arnd Bergmann March 8, 2022, 1:33 p.m. UTC | #3
On Tue, Mar 8, 2022 at 1:07 PM Andre Przywara <andre.przywara@arm.com> wrote:
> On Tue, 8 Mar 2022 10:38:37 +0100 Arnd Bergmann <arnd@arndb.de> wrote:
> > On Mon, Mar 7, 2022 at 3:34 PM Andre Przywara <andre.przywara@arm.com> wrote:
> > >
> > > Some Kconfig options have changed, some other platforms have been
> > > removed.
> >
> > Please split this up into logical chunks: list the platforms that were removed
> > and remove only the lines corresponding to those platforms in one patch,
> > do functional changes in separate patches each with a reason for doing them,
> > and cleanups (moving lines to match the savedefconfig output, removing lines
> > that are now the default) in one final patch.
>
> Actually this patch is meant to be only about the last past: to sync
> multi_v5_defconfig with the output of "make savedefconfig". .config stays
> the same. I initially tried to chase down the reason for each line change,
> but gave up quickly, because it becomes tedious to learn about this,
> especially about symbols that got *removed*. Also Kconfig is somewhat
> sensitive, a single "select" or "default" change in one random Kconfig can
> affect the result of savedefconfig.
>
> As I noted in the commit message, the .config does *not* change as a result
> of this patch, the whole purpose is just to make the next patch clearer.

Ok, then just change the symbols that move around, not the ones that
may have gone missing unintentionally.

> So I can try find the reason for each removal, if you like, but I am not
> sure that's worthwhile? It is my understanding that Kconfig changes tend
> to accumulate cruft in the various defconfigs over time. In U-Boot we gave
> up on reasoning, and just regularly sync the output of savedefconfig over
> to the *_defconfigs, to keep them minimal and meaningful.
>
> And I found Olof's commit 30b10c77837c ("ARM: defconfig: re-run
> savedefconfig on multi_v* configs") as a precedence for this kind of cleanup.

I know, but I never liked doing this because it hides regressions.

> > >  CONFIG_AEABI=y
> > >  CONFIG_HIGHMEM=y
> > > -CONFIG_ZBOOT_ROM_TEXT=0x0
> > > -CONFIG_ZBOOT_ROM_BSS=0x0
> > >  CONFIG_ARM_APPENDED_DTB=y
> > >  CONFIG_ARM_ATAG_DTB_COMPAT=y
> > >  CONFIG_CPU_FREQ=y
> >
> > These were not removed, what happened here is that 'savedefconfig'
> > no longer produces the lines because they now match the defaults.
>
> Yes, I understand. Is there some policy here, for instance to keep
> those in, for clarity?

In this case, as with the moving lines around, there is no functional
change at all, and doing the same thing on older kernels will still
result in the same behavior. I'm not worried about those at all, so
just put them all into one patch.

> > > @@ -159,7 +151,6 @@ CONFIG_I2C_ASPEED=m
> > >  CONFIG_I2C_AT91=y
> > >  CONFIG_I2C_IMX=y
> > >  CONFIG_I2C_MV64XXX=y
> > > -CONFIG_I2C_NOMADIK=y
> > >  CONFIG_SPI=y
> > >  CONFIG_SPI_ATMEL=y
> > >  CONFIG_SPI_IMX=y
> >
> > This one is still there. Not sure why it's no longer enabled.
>
> It's not in the current .config. From what I can see, it depends on
> ARCH_AMBA, which is selected by ARCH_NOMADIK, but that one is not enabled
> by multi_v5_defconfig. Not sure if that is an oversight, or a change, a
> the dependency is bogus, or something else.
>
> If you find that useful, I can try to find those dependency chains for the
> other options, but I definitely lack the knowledge about the history of
> those old platforms, so I can't reason about them. But I could present you
> the findings and you can then say what to do?

I see this was the result of 66e0c12f9e17 ("ARM: nomadik: switch to use the
Nomadik I2C driver"). It's ok to remove this line and others like it,
just explain
what happened for these, as the driver is still enabled.

> > >  CONFIG_REGULATOR_FIXED_VOLTAGE=y
> > >  CONFIG_MEDIA_SUPPORT=y
> > >  CONFIG_MEDIA_CAMERA_SUPPORT=y
> > > -CONFIG_V4L_PLATFORM_DRIVERS=y
> > > -CONFIG_VIDEO_ASPEED=m
> > > -CONFIG_VIDEO_ATMEL_ISI=m
> > >  CONFIG_DRM=y
> > >  CONFIG_DRM_ATMEL_HLCDC=m
> > > -CONFIG_DRM_PANEL_SIMPLE=y
> > > -CONFIG_DRM_PANEL_EDP=y
> > >  CONFIG_DRM_ASPEED_GFX=m
> > > -CONFIG_FB_IMX=y
> > > -CONFIG_FB_ATMEL=y
> > > -CONFIG_BACKLIGHT_ATMEL_LCDC=y
> >
> > This doesn't look right at all. If you want to disable graphics support,
> > please do that in a separate patch and explain why we can't have those
> > any more. Are you running into problems with the vmlinux size?
>
> As I mentioned, the .config didn't change at all, so those options are
> already not included in mainline anymore.
>
> AFAICS, those last options depend on CONFIG_FB, which isn't enabled. Is
> that a regression due to the recent fbdev changes?

Correct, this part was clearly unintentional, and I don't ever want to see
a patch to remove lines like these with a changelog text that fails to
explain why we want them to be removed. We clearly have platforms
that are enabled in multi_v5_config that use some of those drivers, and
we had users that wanted them to be enabled.

I think there are three separate issues here: FB_IMX and FB_ATMEL
broke because of f611b1e7624c ("drm: Avoid circular dependencies for
CONFIG_FB"), VIDEO_ASPEED and VIDEO_ATMEL_ISI  broke
during b0cd4fb27665 ("media: Kconfig: on !EMBEDDED && !EXPERT,
enable driver filtering"), and PANEL_EDP was introduced in 310720875efa
("ARM: configs: Everyone who had PANEL_SIMPLE now gets
PANEL_EDP") without the option ever making it in [1]. These are all
bugs that need to be addressed individually, and not just in a single
defconfig file but across all the affected platforms.

         Arnd

[1] https://lore.kernel.org/all/CAD=FV=VbYcdSqxLHdSaDPh=X0hbW6VWV0mM-iFy3k0J1q+6MWg@mail.gmail.com/
Arnd Bergmann March 8, 2022, 3:17 p.m. UTC | #4
On Tue, Mar 8, 2022 at 3:30 PM Nicolas Ferre
<nicolas.ferre@microchip.com> wrote:
> On 08/03/2022 at 13:07, Andre Przywara wrote:
>
> As far as I'm concerned and only talking about the drivers, I would
> prefer that we keep the following config options activated in the .config:
>
> CONFIG_VIDEO_ATMEL_ISI=m
> CONFIG_DRM_PANEL_SIMPLE=y
> CONFIG_FB_ATMEL=y
> CONFIG_BACKLIGHT_ATMEL_LCDC=y
>
> The question to know if they are activated by default or not and if the
> associated stack is selected or not, well... I lost track of this, sorry.

These are actually missing, I think the two options we need to
enable are:

CONFIG_MEDIA_PLATFORM_SUPPORT=y
CONFIG_FB=y

All the other options that get removed are either now the default,
or are no longer part of the kernel.

         Arnd
Rob Herring March 8, 2022, 4:08 p.m. UTC | #5
On Mon, 07 Mar 2022 14:34:08 +0000, Andre Przywara wrote:
> The F1C100 series actually features a newer generation watchdog IP, so
> the compatible string was wrong.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  .../devicetree/bindings/watchdog/allwinner,sun4i-a10-wdt.yaml   | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Acked-by: Rob Herring <robh@kernel.org>
Rob Herring March 8, 2022, 4:10 p.m. UTC | #6
On Mon, 07 Mar 2022 14:34:14 +0000, Andre Przywara wrote:
> From: Jesse Taube <mr.bossman075@gmail.com>
> 
> The Allwinner F1C100 series contains two MMC controller blocks. From
> comparing the data sheets, they seem to be compatible with the one used
> in the Allwinner A20: the register layout is the same, and they use the
> same separate sample and output clocks design.
> The only difference is the missing reset line in the A20 version, but
> both the binding and the Linux driver make this optional, so it's still
> a fit.
> 
> Add the new SoC specific name and require it to be paired with the A20
> fallback name, as this is all the driver needs to care about.
> 
> Signed-off-by: Jesse Taube <Mr.Bossman075@gmail.com>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  .../devicetree/bindings/mmc/allwinner,sun4i-a10-mmc.yaml       | 3 +++
>  1 file changed, 3 insertions(+)
> 

Acked-by: Rob Herring <robh@kernel.org>
Guenter Roeck March 9, 2022, 11:02 p.m. UTC | #7
On Mon, Mar 07, 2022 at 02:34:08PM +0000, Andre Przywara wrote:
> The F1C100 series actually features a newer generation watchdog IP, so
> the compatible string was wrong.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> Acked-by: Rob Herring <robh@kernel.org>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  .../devicetree/bindings/watchdog/allwinner,sun4i-a10-wdt.yaml   | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/allwinner,sun4i-a10-wdt.yaml b/Documentation/devicetree/bindings/watchdog/allwinner,sun4i-a10-wdt.yaml
> index 43afa24513b9..d90655418d0e 100644
> --- a/Documentation/devicetree/bindings/watchdog/allwinner,sun4i-a10-wdt.yaml
> +++ b/Documentation/devicetree/bindings/watchdog/allwinner,sun4i-a10-wdt.yaml
> @@ -29,7 +29,7 @@ properties:
>            - const: allwinner,sun6i-a31-wdt
>        - items:
>            - const: allwinner,suniv-f1c100s-wdt
> -          - const: allwinner,sun4i-a10-wdt
> +          - const: allwinner,sun6i-a31-wdt
>        - const: allwinner,sun20i-d1-wdt
>        - items:
>            - const: allwinner,sun20i-d1-wdt-reset
Samuel Holland March 11, 2022, 1:30 a.m. UTC | #8
On 3/7/22 8:34 AM, Andre Przywara wrote:
> From: Jesse Taube <mr.bossman075@gmail.com>
> 
> Include clock and reset macros and replace magic numbers.
> 
> Signed-off-by: Jesse Taube <Mr.Bossman075@gmail.com>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Acked-by: Samuel Holland <samuel@sholland.org>
Samuel Holland March 11, 2022, 2:19 a.m. UTC | #9
On 3/7/22 8:34 AM, Andre Przywara wrote:
> From: Jesse Taube <mr.bossman075@gmail.com>
> 
> The Allwinner F1C100 series contains two MMC controller blocks. From
> comparing the data sheets, they seem to be compatible with the one used
> in the Allwinner A20: the register layout is the same, and they use the
> same separate sample and output clocks design.
> The only difference is the missing reset line in the A20 version, but
> both the binding and the Linux driver make this optional, so it's still
> a fit.
> 
> Add the new SoC specific name and require it to be paired with the A20
> fallback name, as this is all the driver needs to care about.
> 
> Signed-off-by: Jesse Taube <Mr.Bossman075@gmail.com>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Acked-by: Samuel Holland <samuel@sholland.org>
Samuel Holland March 11, 2022, 2:19 a.m. UTC | #10
On 3/7/22 8:34 AM, Andre Przywara wrote:
> From: Jesse Taube <mr.bossman075@gmail.com>
> 
> Enable MMC0 and supply the board setting to enable the microSD card slot
> on the LicheePi Nano board.
> Apart from the always missing write protect switch on microSD slots,
> the card-detect pin is not connected to anything, so we use the
> broken-cd property.
> 
> Signed-off-by: Jesse Taube <Mr.Bossman075@gmail.com>
> [Andre: add alias and vmmc supply]
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Acked-by: Samuel Holland <samuel@sholland.org>
Tested-by: Samuel Holland <samuel@sholland.org>
Samuel Holland March 11, 2022, 2:19 a.m. UTC | #11
On 3/7/22 8:34 AM, Andre Przywara wrote:
> The F1C100 series contains two SPI controllers, and many boards use SPI0
> for a SPI flash, as the BROM is able to boot from that.
> 
> Describe the two controllers in the SoC .dtsi, and also add the PortC
> pins for SPI0, since this is where BROM looks at when trying to boot
> from the commonly used SPI flash.
> 
> The SPI controller seems to be the same as in the H3 chips, but it lacks
> a separate mod clock. The manual says it's connected to AHB directly.
> We don't export that AHB clock directly, but can use the AHB *gate* clock
> as a clock source, since the MMC driver is not supposed to change the AHB

Do you mean the SPI driver here?

> frequency anyway.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arch/arm/boot/dts/suniv-f1c100s.dtsi | 33 ++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/suniv-f1c100s.dtsi b/arch/arm/boot/dts/suniv-f1c100s.dtsi
> index 6f2f97458fe0..f8ec1c7a2ca9 100644
> --- a/arch/arm/boot/dts/suniv-f1c100s.dtsi
> +++ b/arch/arm/boot/dts/suniv-f1c100s.dtsi
> @@ -105,6 +105,34 @@ mmc1: mmc@1c10000 {
>  			#size-cells = <0>;
>  		};
>  
> +		spi0: spi@1c05000 {
> +			compatible = "allwinner,suniv-f1c100s-spi",
> +				     "allwinner,sun8i-h3-spi";
> +			reg = <0x01c05000 0x1000>;
> +			interrupts = <10>;
> +			clocks = <&ccu CLK_BUS_SPI0>, <&ccu CLK_BUS_SPI0>;
> +			clock-names = "ahb", "mod";
> +			resets = <&ccu RST_BUS_SPI0>;
> +			status = "disabled";
> +			num-cs = <1>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +		};
> +
> +		spi1: spi@1c06000 {
> +			compatible = "allwinner,suniv-f1c100s-spi",
> +				     "allwinner,sun8i-h3-spi";
> +			reg = <0x01c06000 0x1000>;
> +			interrupts = <11>;
> +			clocks = <&ccu CLK_BUS_SPI1>, <&ccu CLK_BUS_SPI1>;
> +			clock-names = "ahb", "mod";
> +			resets = <&ccu RST_BUS_SPI1>;
> +			status = "disabled";
> +			num-cs = <1>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +		};
> +

Please keep the nodes sorted by address. These should come before the MMC
controllers.

>  		ccu: clock@1c20000 {
>  			compatible = "allwinner,suniv-f1c100s-ccu";
>  			reg = <0x01c20000 0x400>;
> @@ -138,6 +166,11 @@ mmc0_pins: mmc0-pins {
>  				drive-strength = <30>;
>  			};
>  
> +			spi0_pc_pins: spi0-pc-pins {
> +				pins = "PC0", "PC1", "PC2", "PC3";
> +				function = "spi0";
> +			};
> +
>  			uart0_pe_pins: uart0-pe-pins {
>  				pins = "PE0", "PE1";
>  				function = "uart0";
>
Andre Przywara March 11, 2022, 1:33 p.m. UTC | #12
On Thu, 10 Mar 2022 20:19:57 -0600
Samuel Holland <samuel@sholland.org> wrote:

Hi Samuel,

many thanks for having a look!

> On 3/7/22 8:34 AM, Andre Przywara wrote:
> > The F1C100 series contains two SPI controllers, and many boards use SPI0
> > for a SPI flash, as the BROM is able to boot from that.
> > 
> > Describe the two controllers in the SoC .dtsi, and also add the PortC
> > pins for SPI0, since this is where BROM looks at when trying to boot
> > from the commonly used SPI flash.
> > 
> > The SPI controller seems to be the same as in the H3 chips, but it lacks
> > a separate mod clock. The manual says it's connected to AHB directly.
> > We don't export that AHB clock directly, but can use the AHB *gate* clock
> > as a clock source, since the MMC driver is not supposed to change the AHB  
> 
> Do you mean the SPI driver here?

Yes, indeed.

> 
> > frequency anyway.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  arch/arm/boot/dts/suniv-f1c100s.dtsi | 33 ++++++++++++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/suniv-f1c100s.dtsi b/arch/arm/boot/dts/suniv-f1c100s.dtsi
> > index 6f2f97458fe0..f8ec1c7a2ca9 100644
> > --- a/arch/arm/boot/dts/suniv-f1c100s.dtsi
> > +++ b/arch/arm/boot/dts/suniv-f1c100s.dtsi
> > @@ -105,6 +105,34 @@ mmc1: mmc@1c10000 {
> >  			#size-cells = <0>;
> >  		};
> >  
> > +		spi0: spi@1c05000 {
> > +			compatible = "allwinner,suniv-f1c100s-spi",
> > +				     "allwinner,sun8i-h3-spi";
> > +			reg = <0x01c05000 0x1000>;
> > +			interrupts = <10>;
> > +			clocks = <&ccu CLK_BUS_SPI0>, <&ccu CLK_BUS_SPI0>;
> > +			clock-names = "ahb", "mod";
> > +			resets = <&ccu RST_BUS_SPI0>;
> > +			status = "disabled";
> > +			num-cs = <1>;
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +		};
> > +
> > +		spi1: spi@1c06000 {
> > +			compatible = "allwinner,suniv-f1c100s-spi",
> > +				     "allwinner,sun8i-h3-spi";
> > +			reg = <0x01c06000 0x1000>;
> > +			interrupts = <11>;
> > +			clocks = <&ccu CLK_BUS_SPI1>, <&ccu CLK_BUS_SPI1>;
> > +			clock-names = "ahb", "mod";
> > +			resets = <&ccu RST_BUS_SPI1>;
> > +			status = "disabled";
> > +			num-cs = <1>;
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +		};
> > +  
> 
> Please keep the nodes sorted by address. These should come before the MMC
> controllers.

Argh, sorry, I thought I fixed that up, but it must have messed that
up after a rebase.

Will send a fixed version.

Cheers,
Andre

> 
> >  		ccu: clock@1c20000 {
> >  			compatible = "allwinner,suniv-f1c100s-ccu";
> >  			reg = <0x01c20000 0x400>;
> > @@ -138,6 +166,11 @@ mmc0_pins: mmc0-pins {
> >  				drive-strength = <30>;
> >  			};
> >  
> > +			spi0_pc_pins: spi0-pc-pins {
> > +				pins = "PC0", "PC1", "PC2", "PC3";
> > +				function = "spi0";
> > +			};
> > +
> >  			uart0_pe_pins: uart0-pe-pins {
> >  				pins = "PE0", "PE1";
> >  				function = "uart0";
> >   
>
Ulf Hansson March 11, 2022, 3:41 p.m. UTC | #13
On Mon, 7 Mar 2022 at 15:34, Andre Przywara <andre.przywara@arm.com> wrote:
>
> From: Jesse Taube <mr.bossman075@gmail.com>
>
> The Allwinner F1C100 series contains two MMC controller blocks. From
> comparing the data sheets, they seem to be compatible with the one used
> in the Allwinner A20: the register layout is the same, and they use the
> same separate sample and output clocks design.
> The only difference is the missing reset line in the A20 version, but
> both the binding and the Linux driver make this optional, so it's still
> a fit.
>
> Add the new SoC specific name and require it to be paired with the A20
> fallback name, as this is all the driver needs to care about.
>
> Signed-off-by: Jesse Taube <Mr.Bossman075@gmail.com>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Applied for next, thanks!

Kind regards
Uffe


> ---
>  .../devicetree/bindings/mmc/allwinner,sun4i-a10-mmc.yaml       | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mmc/allwinner,sun4i-a10-mmc.yaml b/Documentation/devicetree/bindings/mmc/allwinner,sun4i-a10-mmc.yaml
> index 4f62ad6ce50c..76137132500d 100644
> --- a/Documentation/devicetree/bindings/mmc/allwinner,sun4i-a10-mmc.yaml
> +++ b/Documentation/devicetree/bindings/mmc/allwinner,sun4i-a10-mmc.yaml
> @@ -55,6 +55,9 @@ properties:
>        - items:
>            - const: allwinner,sun50i-h616-mmc
>            - const: allwinner,sun50i-a100-mmc
> +      - items:
> +          - const: allwinner,suniv-f1c100s-mmc
> +          - const: allwinner,sun7i-a20-mmc
>
>    reg:
>      maxItems: 1
> --
> 2.25.1
>