Re: [PATCH 1/2] ARM: dts: am335x: Replace numeric pinmux address with macro defines

[Tony Lindgren <tony@atomide.com>]

Hi,

* Christina Quast <cquast@hanoverdisplays.com> [190313 14:28]:
> The values are extraced from the "AM335x SitaraTM Processors Technical
> Reference Manual", Section 9.3.1 CONTROL_MODULE Registers, based on the
> file autogenerated by TI PinMux.

Thanks for updating this series. Few comments below.

> diff --git a/include/dt-bindings/pinctrl/am335x.h b/include/dt-bindings/pinctrl/am335x.h
> new file mode 100644
> index 000000000000..033a44efdc1e
> --- /dev/null
> +++ b/include/dt-bindings/pinctrl/am335x.h

I think these defines should be just added to the existing
include/dt-bindings/pinctrl/am33xx.h. That is assuming the
padconf registers are the same for all the variants.

> +#define PIN_MODE(mode)		(mode)
> +#define PIN_PULL_UD_EN		(0x1U << 3U)
> +#define	PIN_PULL_TYPE_SEL	(0x1U << 4U)
> +#define	PIN_RX_ACTIVE		(0x1U << 5U)
> +#define PIN_SLEW_SLOW		(0x1U << 6U)

Hmm so in include/dt-bindings/pinctrl/am33xx.h we already have
these defined but with different names?

> +#define AM335X_PIN_OFFSET_MIN			0x0800U

You should leave out the generic control module registers
defines. So starting below..

> +#define AM335X_CONTROL_REVISION			0x0
> +#define AM335X_CONTROL_HWINFO			0x4
> +#define AM335X_CONTROL_SYSCONFIG		0x10
> +#define AM335X_CONTROL_STATUS			0x40
> +#define AM335X_CONTROL_EMIF_SDRAM_CONFIG	0x110
...
> +#define AM335X_BB_SCALE				0x7d0
> +#define AM335X_USB_VID_PID			0x7f4
> +#define AM335X_EFUSE_SMA			0x7fc

.. all the way here. This header should only have the
padconf area registers that should all have PIN in the
name. So only keep the ones from below..

> +#define AM335X_PIN_GPMC_AD0			0x800
> +#define AM335X_PIN_GPMC_AD1			0x804
> +#define AM335X_PIN_GPMC_AD2			0x808
...
> +#define AM335X_PIN_USB0_DRVVBUS			0xa1c
> +#define AM335X_PIN_USB1_DRVVBUS			0xa34

.. to here. Then also drop the defines from here..

> +#define AM335X_CQDETECT_STATUS			0xe00
> +#define AM335X_DDR_IO_CTRL			0xe04
> +#define AM335X_VTP_CTRL				0xe0c
...
> +#define AM335X_DDR_CMD2_IOCTRL			0x140c
> +#define AM335X_DDR_DATA0_IOCTRL			0x1440
> +#define AM335X_DDR_DATA1_IOCTRL			0x1444

.. to here.

> +#define AM335X_PIN_OFFSET_MAX			0x1320U

And then adjust the AM335X_PIN_OFFSET_MAX accordingly
if that is needed.

Note that the padconf range is specified in am33xx-l4.dtsi
for pinmux@800 in the reg range so this header should
contain the same registers. Some SoCs have multiple padconf
ranges but am335x only has one.

Regards,

Tony