Re: [PATCH] ARM: stm32: add support for PHYTEC phyCORE stm32mp1

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hi,

Cc += Steffen

On 23.02.22 13:06, Sascha Hauer wrote:
> On Wed, Feb 23, 2022 at 12:10:36PM +0100, Ahmad Fatoum wrote:
>> From: Steffen Trumtrar <s.trumtrar@pengutronix.de>
>> +/ {
>> +	model = "PHYTEC phyCORE-STM32MP1-3 SOM";
>> +	compatible = "phytec,phycore-stm32mp1-3", "st,stm32mp157";
>> +
>> +	chosen {
>> +		environment-sd {
>> +			compatible = "barebox,environment";
>> +			device-path = &sdmmc1, "partname:barebox-environment";
>> +			status = "disabled";
>> +		};
>> +
>> +		environment-emmc {
>> +			compatible = "barebox,environment";
>> +			device-path = &sdmmc2, "partname:barebox-environment";
> 
> I don't see any partition description for sdmmc1/2. Does the environment
> work?

The other STM32MP1 boards do likewise, as barebox is booted from a GPT
partition (named SSBL), the barebox-environment is also in a GPT-partition
that is referenced here by name.

>> +&i2c4 {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&i2c4_pins_a>;
>> +	i2c-scl-rising-time-ns = <185>;
>> +	i2c-scl-falling-time-ns = <20>;
>> +	status = "okay";
>> +	/delete-property/dmas;
>> +	/delete-property/dma-names;
>> +
>> +	pmic: stpmic@33 {
>> +		compatible = "st,stpmic1";
>> +		reg = <0x33>;
>> +		interrupts-extended = <&gpioa 0 IRQ_TYPE_EDGE_FALLING>;
>> +		interrupt-controller;
>> +		#interrupt-cells = <2>;
>> +		status = "okay";
> 
> Could be dropped.
> 
>> +&qspi {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&qspi_clk_pins_a &qspi_bk1_pins_a &qspi_bk2_pins_a>;
>> +	reg = <0x58003000 0x1000>, <0x70000000 0x4000000>;
> 
> Why is the reg property overwritten?
> 
>> +	#address-cells = <1>;
>> +	#size-cells = <0>;
> 
> This is already in the SoC dtsi.
> 
>> +	status = "disabled";
> 
> Is this needed? Isn't the qpsi controller disabled in the SoC dtsi
> anyway? There are several more nodes in this files that are explicitly
> disabled.

I will just drop the nodes we don't use. We don't yet have a QSPI
driver.

> 
>> +
>> +	flash0: n25q128@0 {
>> +		compatible = "micron,n25q128a13", "jedec,spi-nor";
>> +		reg = <0>;
>> +		spi-rx-bus-width = <4>;
>> +		spi-max-frequency = <50000000>;
>> +		m25p,fast-read;
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +	};
>> +};
>> +
>> +&sdmmc1 {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&sdmmc1_b4_pins_a>;
>> +	disable-wp;
>> +	st,neg-edge;
>> +	bus-width = <4>;
>> +	max-frequency = <10000000>;
> 
> This is very low. Is this intended?

Perhaps Enrico or Steffen know more?

> 
>> +	vmmc-supply = <&v3v3>;
>> +	status = "disabled";
>> +};
>> +
>> +&sdmmc2 {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&sdmmc2_b4_pins_a &sdmmc2_d47_pins_a>;
>> +	non-removable;
>> +	no-sd;
>> +	no-sdio;
>> +	st,neg-edge;
>> +	bus-width = <8>;
>> +	vmmc-supply = <&v3v3>;
>> +	vqmmc-supply = <&v3v3>;
>> +	mmc-ddr-3_3v;
>> +	status = "disabled";
>> +};
>> +
>> +&sram {
>> +	dma_pool: dma_pool@0 {
>> +		reg = <0x50000 0x10000>;
> 
> Should probably be dma_pool@50000

Will just remove it.

Thanks for the review,
Ahmad

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox