Re: [PATCH net v3] net: pcs: rzn1-miic: Reset PRCMD state before unprotect sequence

Re: [PATCH net v3] net: pcs: rzn1-miic: Reset PRCMD state before unprotect sequence

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] SoC-specific PRCMD semantics are applied unconditionally in a
  shared helper: the new `writel(0x0000, miic->base + MIIC_PRCMD)` is…
- [Medium] Fixes-tag mis-attribution and a changelog that names no
  failure mode.

--

net: pcs: rzn1-miic: Reset PRCMD state before unprotect sequence

This adds a writel(0x0000, MIIC_PRCMD) ahead of the existing
0x00A5/0x0001/0xFFFE/0x0001 unprotect key sequence in miic_unlock_regs(),
so that the sequence starts from the IDLE state of the PRCMD command state
machine as documented for RZ/T2H.

> diff --git a/drivers/net/pcs/pcs-rzn1-miic.c b/drivers/net/pcs/pcs-rzn1-miic.c
> index cb74861e823c..4a2a50827338 100644
> --- a/drivers/net/pcs/pcs-rzn1-miic.c
> +++ b/drivers/net/pcs/pcs-rzn1-miic.c
> @@ -270,6 +270,9 @@ static struct miic_port *phylink_pcs_to_miic_port(struct phylink_pcs *pcs)
>  
>  static void miic_unlock_regs(struct miic *miic)
>  {
> +	/* Reset PRCMD state before unprotect sequence */
> +	writel(0x0000, miic->base + MIIC_PRCMD);
> +

[Severity: Medium]
Should this write be gated on the SoC it is documented for?

miic_unlock_regs() has two consumers, and they belong to different SoCs:

  miic_reg_writel_locked()   /* only rzt2h_miic_of_data.miic_write */
      miic_unlock_regs()
      writel(value, ...)
      miic_lock_regs()

  miic_init_hw()             /* only rzn1_miic_of_data sets the flag */
      if (miic->of_data->init_unlock_lock_regs)
              miic_unlock_regs();

rzn1_miic_of_data carries .init_unlock_lock_regs = true and
.miic_write = miic_reg_writel_unlocked, while rzt2h_miic_of_data carries
.miic_write = miic_reg_writel_locked and does not set the flag. So the new
un-keyed PRCMD write is now also emitted on RZ/N1 (r9a06g032), where the
driver has so far only ever written the documented key sequence to this
write-protected register.

The changelog justifies the write with RZ/T2H documentation only:

  "On RZ/T2H MIIC, writing 0x0000 to PRCMD resets the command state
  machine to the IDLE state."

If the RZ/N1 protect block instead treats a bare 0x0000 as an
out-of-sequence command of the A5/data/~data/data protocol, does the
following unprotect sequence still release protection?

In that case every write that miic_init_hw() issues afterwards, MODCTRL,
CONVRST via miic_converter_enable(), SWCTRL and SWDUPC, would be dropped by
the hardware. There is no read-back of a protected register and
miic_init_hw() ends with:

	return 0;

so probe would still succeed and the only visible symptom would be a
mis-muxed or dead Ethernet path on RZ/N1.

Would it make sense to scope the reset write to RZ/T2H, either via
of_data->type == MIIC_TYPE_RZT2H, a new of_data flag, or by doing it inside
miic_reg_writel_locked(), or alternatively to state in the changelog that
the extra PRCMD write was verified harmless on RZ/N1?

[Severity: Medium]
Does the Fixes tag point at the right commit?

  Fixes: 419747319e3a ("net: pcs: rzn1-miic: Add per-SoC control for MIIC
  register unlock/lock")

git blame shows 419747319e3a only moved the pre-existing RZ/N1 four-write
sequence into miic_unlock_regs(); at that point the helper was reachable
only from the RZ/N1 miic_init_hw() path. The RZ/T2H side the changelog
relies on came from the later commit 08f89e42121d4, which added
miic_lock_regs(), miic_reg_writel_locked() and rzt2h_miic_of_data.

A backport driven by this tag would therefore land the write on kernels
that have no RZ/T2H support at all, where its only effect is an extra
PRCMD write on RZ/N1. Should the tag reference 08f89e42121d4 instead?

Also, which condition actually leaves PRCMD in a non-IDLE state?

On RZ/T2H every register write goes through
unlock, write, miic_lock_regs(), and miic_lock_regs() already does:

	writel(0x0000, miic->base + MIIC_PRCMD);

so PRCMD is 0x0000 after any driver-initiated access. On RZ/N1
miic_unlock_regs() runs once during probe on a freshly reset block. The
remaining source of unknown state would be pre-kernel firmware or the other
master mentioned in miic_init_hw():

	/* Unlock write access to accessory registers (cf datasheet). If this
	 * is going to be used in conjunction with the Cortex-M3, this sequence
	 * will have to be moved in register write
	 */

but the changelog does not mention it. And against a concurrent external
agent, does one extra leading write really give the stated property,
"always starts from a known state, regardless of any previous PRCMD
activity", given the five writes are not atomic as a group?

Could the changelog name the observed symptom or the condition that
produces the stale PRCMD state?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/TYCPR01MB7481CE8351FDB2CDCA229955A4872%40TYCPR01MB7481.jpnprd01.prod.outlook.com

[PATCH net v3] net: pcs: rzn1-miic: Reset PRCMD state before unprotect sequence

[Yoshihisa Yamamoto]

On RZ/T2H MIIC, writing 0x0000 to PRCMD resets the command state
machine to the IDLE state.

Reset PRCMD before issuing the unprotect sequence so that it always
starts from a known state, regardless of any previous PRCMD activity.

Fixes: 419747319e3a ("net: pcs: rzn1-miic: Add per-SoC control for MIIC register unlock/lock")
Signed-off-by: Yoshihisa Yamamoto <yoshihisa.yamamoto.xn@renesas.com>
---
v3:
- Add Fixes tag

v2: https://lore.kernel.org/all/TYCPR01MB748157ECB6B92D8E3FFD433BA4B82@TYCPR01MB7481.jpnprd01.prod.outlook.com
- Fix indentation

v1: https://lore.kernel.org/all/TYCPR01MB748139F059DF2459B9813416A4B92@TYCPR01MB7481.jpnprd01.prod.outlook.com

 drivers/net/pcs/pcs-rzn1-miic.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/pcs/pcs-rzn1-miic.c b/drivers/net/pcs/pcs-rzn1-miic.c
index 2b72fa98d..818756bb8 100644
--- a/drivers/net/pcs/pcs-rzn1-miic.c
+++ b/drivers/net/pcs/pcs-rzn1-miic.c
@@ -270,6 +270,9 @@ static struct miic_port *phylink_pcs_to_miic_port(struct phylink_pcs *pcs)
 
 static void miic_unlock_regs(struct miic *miic)
 {
+	/* Reset PRCMD state before unprotect sequence */
+	writel(0x0000, miic->base + MIIC_PRCMD);
+
 	/* Unprotect register writes */
 	writel(0x00A5, miic->base + MIIC_PRCMD);
 	writel(0x0001, miic->base + MIIC_PRCMD);
-- 
2.34.1