Hi, Geert,

On 06.12.2023 12:56, Geert Uytterhoeven wrote:

On Wed, Dec 6, 2023 at 11:33 AM Geert Uytterhoeven [off-list ref] wrote:

quoted

On Mon, Nov 20, 2023 at 8:03 AM Claudiu [off-list ref] wrote:

quoted

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The intention of SW_SD2_EN macro was to reflect the state of SW_CONFIG3
switch available on RZ/G3S Smarc Module. According to documentation SD2
is enabled when switch is in OFF state. For this, changed the logic of
marco to map value 0 to switch's OFF state and value 1 to switch's ON
state. Along with this update the description for each state for better
understanding.

The value of SW_SD2_EN macro was not changed in file because, according to
documentation, the default state for this switch is ON.

Fixes: adb4f0c5699c ("arm64: dts: renesas: Add initial support for RZ/G3S SMARC SoM")
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Thanks for your patch!

quoted

--- a/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
+++ b/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi

@@ -14,8 +14,8 @@
  *     0 - SD0 is connected to eMMC
  *     1 - SD0 is connected to uSD0 card
  * @SW_SD2_EN:
- *     0 - SCIF1, SSI0, IRQ0, IRQ1 connected to SoC
- *     1 - SD2 is connected to SoC
+ *     0 - (switch OFF) SD2 is connected to SoC
+ *     1 - (switch ON)  SCIF1, SSI0, IRQ0, IRQ1 connected to SoC

I think this is still confusing: SW_SD2_EN refers to an active-low signal
(SW_SD2_EN#) in the schematics.

OMG, while the signal is called "SW_SD2_EN#" in the schematics, it is
_not_ active-low!
SW_D2_EN# drives a STG3692 quad SPDT switch, and SD2 is enabled
if SW_D2_EN# is high...

The RZ/G3S SMARC Module User Manual says:

Signal SW_SD2_EN ON: SD2 is disabled.
Signal SW_SD2_EN OFF: SD2 is enabled.

I followed the description in this manual, chapter 2.1.1 SW_CONFIG. The
idea was that these macros to correspond to individual switches, to match
that table (describing switches position) with this code as the user in the
end sets those switches described in table at 2.1.1 w/o necessary going
deep into schematic (at least in the beginning when trying different
functionalities).

Do you think it would be better if we will have these macros named
SWCONFIGX, X in {1, 2, 3, 4, 5, 6} ?

So whatever we do, something will look odd :-(

quoted

Before, SW_SD2_EN used assertion-logic (1 is enabled), and didn't
match the physical signal level.
After your patch, SW_SD2_EN matches the active-low physical level, but
this is not reflected in the name...

quoted

  */
 #define SW_SD0_DEV_SEL 1
 #define SW_SD2_EN      1

@@ -25,7 +25,7 @@ / {

        aliases {
                mmc0 = &sdhi0;
-#if SW_SD2_EN
+#if !SW_SD2_EN

... so this condition looks really weird.

Still, I think the original looks nicer here.

So I suggest to keep the original logic, but clarify the position of
the switch.
Does that make sense?

It will still be odd, AFAICT, as this way as we will map 0 to ON and 1 to
OFF... A bit counterintuitive.

quoted

quoted

                mmc2 = &sdhi2;
 #endif
        };

@@ -116,7 +116,7 @@ &sdhi0 {
 };
 #endif

-#if SW_SD2_EN
+#if !SW_SD2_EN
 &sdhi2 {
        pinctrl-0 = <&sdhi2_pins>;
        pinctrl-names = "default";

So I think SW_SD2_EN should be renamed to SW_SD2_EN_N.

Cfr. SW_ET0_EN_N on RZ/G2UL:

arch/arm64/boot/dts/renesas/r9a07g043u11-smarc.dts- * DIP-Switch SW1 setting
arch/arm64/boot/dts/renesas/r9a07g043u11-smarc.dts- * 1 : High; 0: Low
arch/arm64/boot/dts/renesas/r9a07g043u11-smarc.dts- * SW1-2 :
SW_SD0_DEV_SEL    (0: uSD; 1: eMMC)
arch/arm64/boot/dts/renesas/r9a07g043u11-smarc.dts- * SW1-3 :
SW_ET0_EN_N               (0: ETHER0; 1: CAN0, CAN1, SSI1, RSPI1)
arch/arm64/boot/dts/renesas/r9a07g043u11-smarc.dts- * Please change
below macros according to SW1 setting on the SoM

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds