From: Claudiu Beznea <[email protected]>
Hi,
Series adds Ethernet support for Renesas RZ/G3S Ethernet.
Along with it preparatory cleanups and fixes were included.
Patches 1-4 are clock specific.
Patches 5-8 are pinctrl specific.
Patches 9-13 are device tree specific.
Patch 14 updates multi_v7_defconfig with RAVB flag.
Thank you,
Claudiu Beznea
Claudiu Beznea (14):
clk: renesas: rzg2l-cpg: Reuse code in rzg2l_cpg_reset()
clk: renesas: rzg2l-cpg: Check reset monitor registers
clk: renesas: rzg2l-cpg: Add support for MSTOP
clk: renesas: r9a08g045-cpg: Add clock and reset support for ETH0 and
ETH1
pinctrl: renesas: rzg2l: Move arg in the main function block
pinctrl: renesas: rzg2l: Add pin configuration support for pinmux
groups
pinctrl: renesas: rzg2l: Add support to select power source for
Ethernet pins
pinctrl: renesas: rzg2l: add output enable support
dt-bindings: net: renesas,etheravb: Document RZ/G3S support
arm64: renesas: r9a08g045: Add Ethernet nodes
arm64: renesas: rzg3s-smarc-som: Invert the logic for SW_SD2_EN macro
arm64: dts: renesas: Improve documentation for SW_SD0_DEV_SEL
arm64: dts: renesas: rzg3s-smarc-som: Enable Ethernet interfaces
arm: multi_v7_defconfig: Enable CONFIG_RAVB
.../bindings/net/renesas,etheravb.yaml | 1 +
arch/arm/configs/multi_v7_defconfig | 1 +
arch/arm64/boot/dts/renesas/r9a08g045.dtsi | 32 ++++
.../boot/dts/renesas/rzg3s-smarc-som.dtsi | 153 +++++++++++++++-
drivers/clk/renesas/r9a07g043-cpg.c | 116 ++++++------
drivers/clk/renesas/r9a07g044-cpg.c | 158 ++++++++---------
drivers/clk/renesas/r9a08g045-cpg.c | 64 +++++--
drivers/clk/renesas/r9a09g011-cpg.c | 116 ++++++------
drivers/clk/renesas/rzg2l-cpg.c | 166 +++++++++++++++---
drivers/clk/renesas/rzg2l-cpg.h | 21 ++-
drivers/pinctrl/renesas/pinctrl-rzg2l.c | 166 ++++++++++++++++--
11 files changed, 736 insertions(+), 258 deletions(-)
--
2.39.2
From: Claudiu Beznea <[email protected]>
On RZ/G3S different Ethernet pins needs to be configured with different
settings (e.g. power-source need to be set, RGMII TXC, TX_CTL pins need
output-enable). Commit adjust driver to allow specifying pin configuration
for pinmux groups. With this DT settings like the following are taken
into account by driver:
eth0_pins: eth0 {
tx_ctl {
pinmux = <RZG2L_PORT_PINMUX(1, 1, 1)>; /* ET0_TX_CTL */
power-source = <1800>;
output-enable;
drive-strength-microamp = <5200>;
};
};
Signed-off-by: Claudiu Beznea <[email protected]>
---
drivers/pinctrl/renesas/pinctrl-rzg2l.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
index 21ee628363fa..819698dacef0 100644
--- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
+++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
@@ -376,8 +376,11 @@ static int rzg2l_dt_subnode_to_map(struct pinctrl_dev *pctldev,
goto done;
}
- if (num_pinmux)
+ if (num_pinmux) {
nmaps += 1;
+ if (num_configs)
+ nmaps += 1;
+ }
if (num_pins)
nmaps += num_pins;
@@ -462,6 +465,16 @@ static int rzg2l_dt_subnode_to_map(struct pinctrl_dev *pctldev,
maps[idx].data.mux.function = name;
idx++;
+ if (num_configs) {
+ ret = rzg2l_map_add_config(&maps[idx], name,
+ PIN_MAP_TYPE_CONFIGS_GROUP,
+ configs, num_configs);
+ if (ret < 0)
+ goto remove_group;
+
+ idx++;
+ };
+
dev_dbg(pctrl->dev, "Parsed %pOF with %d pins\n", np, num_pinmux);
ret = 0;
goto done;
--
2.39.2
From: Claudiu Beznea <[email protected]>
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 <[email protected]>
---
arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi b/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
index 01a4a9da7afc..275b14acd2ee 100644
--- 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
*/
#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
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";
--
2.39.2
On Mon, Nov 20, 2023 at 8:00 AM Claudiu <[email protected]> wrote:
> Patches 5-8 are pinctrl specific.
I expect Geert to pick these once he's happy with them and merge them
into his tree for pull request to my pinctrl tree.
If you want some other merging approach then inform us!
Yours,
Linus Walleij
From: Claudiu Beznea <[email protected]>
Move arg in the main block of the function as this is used by 3 out of 4
case blocks of switch-case. In this way some lines of code are removed.
Signed-off-by: Claudiu Beznea <[email protected]>
---
drivers/pinctrl/renesas/pinctrl-rzg2l.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
index 9de350ad7e7d..21ee628363fa 100644
--- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
+++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
@@ -842,7 +842,7 @@ static int rzg2l_pinctrl_pinconf_set(struct pinctrl_dev *pctldev,
struct rzg2l_pinctrl_pin_settings settings = pctrl->settings[_pin];
unsigned int *pin_data = pin->drv_data;
enum pin_config_param param;
- unsigned int i;
+ unsigned int i, arg;
u32 cfg, off;
int ret;
u8 bit;
@@ -865,8 +865,7 @@ static int rzg2l_pinctrl_pinconf_set(struct pinctrl_dev *pctldev,
param = pinconf_to_config_param(_configs[i]);
switch (param) {
case PIN_CONFIG_INPUT_ENABLE: {
- unsigned int arg =
- pinconf_to_config_argument(_configs[i]);
+ arg = pinconf_to_config_argument(_configs[i]);
if (!(cfg & PIN_CFG_IEN))
return -EINVAL;
@@ -880,9 +879,10 @@ static int rzg2l_pinctrl_pinconf_set(struct pinctrl_dev *pctldev,
break;
case PIN_CONFIG_DRIVE_STRENGTH: {
- unsigned int arg = pinconf_to_config_argument(_configs[i]);
unsigned int index;
+ arg = pinconf_to_config_argument(_configs[i]);
+
if (!(cfg & PIN_CFG_IOLH_A) || hwcfg->drive_strength_ua)
return -EINVAL;
@@ -907,9 +907,10 @@ static int rzg2l_pinctrl_pinconf_set(struct pinctrl_dev *pctldev,
break;
case PIN_CONFIG_OUTPUT_IMPEDANCE_OHMS: {
- unsigned int arg = pinconf_to_config_argument(_configs[i]);
unsigned int index;
+ arg = pinconf_to_config_argument(_configs[i]);
+
if (!(cfg & PIN_CFG_IOLH_B) || !hwcfg->iolh_groupb_oi[0])
return -EINVAL;
--
2.39.2
From: Claudiu Beznea <[email protected]>
Add switch OFF/OFF description to values of SW_SD0_DEV_SEL for
better understanding.
Signed-off-by: Claudiu Beznea <[email protected]>
---
arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi b/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
index 275b14acd2ee..e090a4837468 100644
--- a/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
+++ b/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
@@ -11,8 +11,8 @@
/*
* Signals of SW_CONFIG switches:
* @SW_SD0_DEV_SEL:
- * 0 - SD0 is connected to eMMC
- * 1 - SD0 is connected to uSD0 card
+ * 0 - (switch OFF) SD0 is connected to eMMC
+ * 1 - (switch ON) SD0 is connected to uSD0 card
* @SW_SD2_EN:
* 0 - (switch OFF) SD2 is connected to SoC
* 1 - (switch ON) SCIF1, SSI0, IRQ0, IRQ1 connected to SoC
--
2.39.2
From: Claudiu Beznea <[email protected]>
ravb driver is used by RZ/G1H. Enable it in multi_v7_defconfig.
Signed-off-by: Claudiu Beznea <[email protected]>
---
arch/arm/configs/multi_v7_defconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
index 10fd74bf85f9..9a04564566a7 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -272,6 +272,7 @@ CONFIG_KS8851=y
CONFIG_LAN966X_SWITCH=m
CONFIG_R8169=y
CONFIG_SH_ETH=y
+CONFIG_RAVB=y
CONFIG_SMSC911X=y
CONFIG_SNI_AVE=y
CONFIG_STMMAC_ETH=y
--
2.39.2
Hi Claudiu,
On Mon, Nov 20, 2023 at 8:01 AM Claudiu <[email protected]> wrote:
> From: Claudiu Beznea <[email protected]>
>
> Move arg in the main block of the function as this is used by 3 out of 4
> case blocks of switch-case. In this way some lines of code are removed.
>
> Signed-off-by: Claudiu Beznea <[email protected]>
Thanks for your patch!
> drivers/pinctrl/renesas/pinctrl-rzg2l.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
Unfortunately your claim is not really backed by the diffstat.
What about moving index, too?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
Hi Claudiu,
On Mon, Nov 20, 2023 at 8:01 AM Claudiu <[email protected]> wrote:
> From: Claudiu Beznea <[email protected]>
>
> On RZ/G3S different Ethernet pins needs to be configured with different
> settings (e.g. power-source need to be set, RGMII TXC, TX_CTL pins need
> output-enable). Commit adjust driver to allow specifying pin configuration
> for pinmux groups. With this DT settings like the following are taken
> into account by driver:
>
> eth0_pins: eth0 {
> tx_ctl {
> pinmux = <RZG2L_PORT_PINMUX(1, 1, 1)>; /* ET0_TX_CTL */
> power-source = <1800>;
> output-enable;
> drive-strength-microamp = <5200>;
> };
> };
>
> Signed-off-by: Claudiu Beznea <[email protected]>
Thanks for your patch!
> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -376,8 +376,11 @@ static int rzg2l_dt_subnode_to_map(struct pinctrl_dev *pctldev,
> goto done;
> }
>
> - if (num_pinmux)
> + if (num_pinmux) {
> nmaps += 1;
> + if (num_configs)
> + nmaps += 1;
I think this would be more readable, and better follow the style of
the surrounding statements, if this new check would not be nested
under the num_pinmux check.
> + }
>
> if (num_pins)
> nmaps += num_pins;
Reviewed-by: Geert Uytterhoeven <[email protected]>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
On 01.12.2023 18:15, Geert Uytterhoeven wrote:
> Hi Claudiu,
>
> On Mon, Nov 20, 2023 at 8:01 AM Claudiu <[email protected]> wrote:
>> From: Claudiu Beznea <[email protected]>
>>
>> Move arg in the main block of the function as this is used by 3 out of 4
>> case blocks of switch-case. In this way some lines of code are removed.
>>
>> Signed-off-by: Claudiu Beznea <[email protected]>
>
> Thanks for your patch!
>
>> drivers/pinctrl/renesas/pinctrl-rzg2l.c | 11 ++++++-----
>> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> Unfortunately your claim is not really backed by the diffstat.
> What about moving index, too?
Sure, I can move it, too.
>
> Gr{oetje,eeting}s,
>
> Geert
>
Hi Claudiu,
On Mon, Nov 20, 2023 at 8:03 AM Claudiu <[email protected]> wrote:
> From: Claudiu Beznea <[email protected]>
>
> 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 <[email protected]>
Thanks for your patch!
> --- 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.
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...
> */
> #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.
> 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 -- [email protected]
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
On Wed, Dec 6, 2023 at 11:33 AM Geert Uytterhoeven <[email protected]> wrote:
> On Mon, Nov 20, 2023 at 8:03 AM Claudiu <[email protected]> wrote:
> > From: Claudiu Beznea <[email protected]>
> >
> > 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 <[email protected]>
>
> Thanks for your patch!
>
> > --- 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.
So whatever we do, something will look odd :-(
> 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...
>
> > */
> > #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?
>
> > 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 -- [email protected]
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
Hi Claudiu,
On Mon, Nov 20, 2023 at 8:03 AM Claudiu <[email protected]> wrote:
> From: Claudiu Beznea <[email protected]>
>
> Add switch OFF/OFF description to values of SW_SD0_DEV_SEL for
> better understanding.
>
> Signed-off-by: Claudiu Beznea <[email protected]>
Thanks for your patch!
> --- a/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
> +++ b/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
> @@ -11,8 +11,8 @@
> /*
> * Signals of SW_CONFIG switches:
> * @SW_SD0_DEV_SEL:
> - * 0 - SD0 is connected to eMMC
> - * 1 - SD0 is connected to uSD0 card
> + * 0 - (switch OFF) SD0 is connected to eMMC
> + * 1 - (switch ON) SD0 is connected to uSD0 card
> * @SW_SD2_EN:
> * 0 - (switch OFF) SD2 is connected to SoC
> * 1 - (switch ON) SCIF1, SSI0, IRQ0, IRQ1 connected to SoC
I guess this makes sense
Reviewed-by: Geert Uytterhoeven <[email protected]>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
Hi, Geert,
On 06.12.2023 12:56, Geert Uytterhoeven wrote:
> On Wed, Dec 6, 2023 at 11:33 AM Geert Uytterhoeven <[email protected]> wrote:
>> On Mon, Nov 20, 2023 at 8:03 AM Claudiu <[email protected]> wrote:
>>> From: Claudiu Beznea <[email protected]>
>>>
>>> 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 <[email protected]>
>>
>> Thanks for your patch!
>>
>>> --- 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 :-(
>
>> 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...
>>
>>> */
>>> #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.
>
>
>>
>>> 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 -- [email protected]
>
> 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
On 06.12.2023 13:27, Geert Uytterhoeven wrote:
> Hi Claudiu,
>
> On Wed, Dec 6, 2023 at 12:12 PM claudiu beznea <[email protected]> wrote:
>> On 06.12.2023 12:56, Geert Uytterhoeven wrote:
>>> On Wed, Dec 6, 2023 at 11:33 AM Geert Uytterhoeven <[email protected]> wrote:
>>>> On Mon, Nov 20, 2023 at 8:03 AM Claudiu <[email protected]> wrote:
>>>>> From: Claudiu Beznea <[email protected]>
>>>>>
>>>>> 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 <[email protected]>
>>>>
>>>> Thanks for your patch!
>>>>
>>>>> --- 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} ?
>
> Perhaps. A disadvantage would be that SW_CONFIG%u doesn't
> give any indication about its purpose...
That's the reason I chose initially to have the signal names instead of
SWCONFIGX.
Now seeing that signal names could be confusing I tend to go with SWCONFIGx
instead.
>
>>> So whatever we do, something will look odd :-(
>>>
>>>> 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...
>>>>
>>>>> */
>>>>> #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.
>
> Most switches on board pull signals LOW when the switch is ON...
>
> Gr{oetje,eeting}s,
>
> Geert
>
Hi Claudiu,
On Wed, Dec 6, 2023 at 12:12 PM claudiu beznea <[email protected]> wrote:
> On 06.12.2023 12:56, Geert Uytterhoeven wrote:
> > On Wed, Dec 6, 2023 at 11:33 AM Geert Uytterhoeven <[email protected]> wrote:
> >> On Mon, Nov 20, 2023 at 8:03 AM Claudiu <[email protected]> wrote:
> >>> From: Claudiu Beznea <[email protected]>
> >>>
> >>> 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 <[email protected]>
> >>
> >> Thanks for your patch!
> >>
> >>> --- 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} ?
Perhaps. A disadvantage would be that SW_CONFIG%u doesn't
give any indication about its purpose...
> > So whatever we do, something will look odd :-(
> >
> >> 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...
> >>
> >>> */
> >>> #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.
Most switches on board pull signals LOW when the switch is ON...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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