Re: [PATCH v3 4/6] riscv: dts: microchip: drop duplicated MMC/SDHC node

[Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>]

On 21/09/2021 12:40, Conor.Dooley@microchip.com wrote:
> On 20/09/2021 16:08, Krzysztof Kozlowski wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> Devicetree source is a description of hardware and hardware has only one
>> block @20008000 which can be configured either as eMMC or SDHC.  Having
>> two node for different modes is an obscure, unusual and confusing way to
>> configure it.  Instead the board file is supposed to customize the block
>> to its needs, e.g. to SDHC mode.
>>
>> This fixes dtbs_check warning:
>>    arch/riscv/boot/dts/microchip/microchip-mpfs-icicle-kit.dt.yaml: sdhc@20008000: $nodename:0: 'sdhc@20008000' does not match '^mmc(@.*)?$'
>>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
>>
>> ---
>>
>> Changes since v1:
>> 1. Move also bus-width, suggested by Geert.
>> ---
>>   .../microchip/microchip-mpfs-icicle-kit.dts   | 11 +++++++-
>>   .../boot/dts/microchip/microchip-mpfs.dtsi    | 28 +------------------
>>   2 files changed, 11 insertions(+), 28 deletions(-)
>>
>> diff --git a/arch/riscv/boot/dts/microchip/microchip-mpfs-icicle-kit.dts b/arch/riscv/boot/dts/microchip/microchip-mpfs-icicle-kit.dts
>> index 07f1f3cab686..fc1e5869df1b 100644
>> --- a/arch/riscv/boot/dts/microchip/microchip-mpfs-icicle-kit.dts
>> +++ b/arch/riscv/boot/dts/microchip/microchip-mpfs-icicle-kit.dts
>> @@ -51,8 +51,17 @@ &serial3 {
>>          status = "okay";
>>   };
>>
>> -&sdcard {
>> +&mmc {
>>          status = "okay";
>> +
>> +       bus-width = <4>;
>> +       disable-wp;
>> +       cap-sd-highspeed;
>> +       card-detect-delay = <200>;
>> +       sd-uhs-sdr12;
>> +       sd-uhs-sdr25;
>> +       sd-uhs-sdr50;
>> +       sd-uhs-sdr104;
>>   };
>>
>>   &emac0 {
>> diff --git a/arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi b/arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi
>> index 5084b93188f0..83bc14860960 100644
>> --- a/arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi
>> +++ b/arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi
>> @@ -262,39 +262,13 @@ serial3: serial@20104000 {
>>                          status = "disabled";
>>                  };
>>
>> -               emmc: mmc@20008000 {
>> -                       compatible = "cdns,sd4hc";
>> -                       reg = <0x0 0x20008000 0x0 0x1000>;
>> -                       interrupt-parent = <&plic>;
>> -                       interrupts = <88 89>;
>> -                       pinctrl-names = "default";
>> -                       clocks = <&clkcfg 6>;
>> -                       bus-width = <4>;
>> -                       cap-mmc-highspeed;
>> -                       mmc-ddr-3_3v;
>> -                       max-frequency = <200000000>;
>> -                       non-removable;
>> -                       no-sd;
>> -                       no-sdio;
>> -                       voltage-ranges = <3300 3300>;
>> -                       status = "disabled";
>> -               };
>> -
>> -               sdcard: sdhc@20008000 {
>> +               mmc: mmc@20008000 {
>>                          compatible = "cdns,sd4hc";
>>                          reg = <0x0 0x20008000 0x0 0x1000>;
>>                          interrupt-parent = <&plic>;
>>                          interrupts = <88>;
>>                          pinctrl-names = "default";
>>                          clocks = <&clkcfg 6>;
>> -                       bus-width = <4>;
>> -                       disable-wp;
>> -                       cap-sd-highspeed;
>> -                       card-detect-delay = <200>;
>> -                       sd-uhs-sdr12;
>> -                       sd-uhs-sdr25;
>> -                       sd-uhs-sdr50;
>> -                       sd-uhs-sdr104;
>>                          max-frequency = <200000000>;
>>                          status = "disabled";
>>                  };
>> --
>> 2.30.2
>>
> Hi Krzysztof,
> Seems I missed most of this series other than the new vendor name in the V1.

Unfortunately your name does not appear as maintainer for these files
and get_maintainers.pl brings it only sometimes as a --git fallback.
Also few addresses from that --git fallback are non working, so I am not
always Cc-ing them. Sorry for that, I'll try to Cc you on next Microchip
RISC-V submissions, however you should probably add a proper platform
maintainer entry (similarly to ARM/ARM64 subarchitectures).


> 
> We have been redoing the device tree for the mpfs/icicle kit partly dye 
> to some changes we made to the design. Previously SD and eMMC were 
> different FPGA designs but now both are in the same design and managed 
> by the bootloader, depending on where it finds the image to boot from.
> Since then we've just been using the following single entry in the .dtsi:
> 
>          mmc: mmc@20008000 { /* Common node entry for emmc/sd */
>              compatible = "cdns,sd4hc";
>              reg = <0x0 0x20008000 0x0 0x1000>;
>              clocks = <&clkcfg CLK_MMC>;
>              interrupt-parent = <&plic>;
>              interrupts = <PLIC_INT_MMC_MAIN PLIC_INT_MMC_WAKEUP>;

I'll switch to 2 interrupts.

>              bus-width = <4>;
>              cap-mmc-highspeed;
>              cap-sd-highspeed;
>              no-1-8-v;
>              disable-wp;
>              max-frequency = <200000000>;
>              status = "disabled";

I understand you prefer then the mmc node to be active? Before the DT
had SDHC one enable, so my patch did not introduce changes (except
mentioned interrupt).

I can change it to mmc above with your explanation.


Best regards,
Krzysztof