Re: [PATCH v6 2/8] arm64: dts: agilex5: add SD/eMMC host controller

["Kathpalia, Tanmay" <tanmay.kathpalia@altera.com>]

Hi Michael,

Thanks for your feedback.

>>   			};
>>   		};
>>   
>> +		/*
>> +		 * Shared SD/eMMC controller node. On the SOCDK OOBE daughter-card
>> +		 * this is used for SD card operation; on the SOCDK eMMC daughter-card
>> +		 * it is configured for eMMC.
>> +		 */
> The documentation about the daughter-card configuration belongs into the
> device trees of the daughter-cards and not in the SoC device tree.

Agreed. The comment is dropped from the SoC DTSI in v7.

>> +		emmc: mmc@10808000 {
> The HPS register address map uses the name "sdmmc" for this group. Maybe
> that's a better label than emmc, if the controller may be used for eMMC
> or SD card.

Agreed. v7 renames the label to sdmmc.

>> +			compatible = "altr,agilex5-sd6hc", "cdns,sd6hc";
>> +			reg = <0x10808000 0x1000>;
>> +			/*
>> +			 * "ciu" (SDMCLK) is listed first so it is selected as the
>> +			 * primary clock by the SDHCI platform layer; the SD6HC PHY
>> +			 * timing calculations are derived from this clock rate.
>> +			 */
> This is surprising to me and I can't find where the first clock is
> selected as primary clock by the SDHCI platform layer.
>
> Could you point me to the code that relies on the order of clocks in the
> device tree?

The comment was wrong: it is not the SDHCI platform layer. v6 did
   clk = devm_clk_get_enabled(dev, NULL);
in sdhci_cdns_probe().
SD6HC now requests the clocks by
name:
   clk = devm_clk_get_enabled(dev, "ciu");
   biu_clk = devm_clk_get_enabled(dev, "biu");
SD4HC still uses the unnamed lookup because its binding has no
clock-names. The DTS clock-order comment is also removed.

>
>
> -&gpio0 {
> +&emmc {
> +	bus-width = <4>;
> +	cap-sd-highspeed;
> +	disable-wp;
> +	max-frequency = <200000000>;
> +	no-mmc;
> +	sd-uhs-sdr104;
> +	vmmc-supply = <&vmmc_reg>;
> +	vqmmc-supply = <&vqmmc_io_reg>;
> +
>   	status = "okay";
> I'd have split the addition of the controller to the SoC device tree and
> the enabling for the board in two separate patches, but that's probably
> just personal taste.
>

Done in v7: SoC node in one patch, SOCDK SD enablement in the next.

Thanks again for the review, it improved the series.

Tanmay