Re: [PATCH phy-fixes] phy: marvell: mvebu-a3700-utmi: fix incorrect USB2_PHY_CTRL register access

From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Gabor Juhos <j4g8y7@gmail.com>
Cc: Vinod Koul <vkoul@kernel.org>,
	 Neil Armstrong <neil.armstrong@linaro.org>,
	 Igal Liberman <igall@marvell.com>,
	 Kishon Vijay Abraham I <kishon@kernel.org>,
	 linux-phy@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH phy-fixes] phy: marvell: mvebu-a3700-utmi: fix incorrect USB2_PHY_CTRL register access
Date: Wed, 01 Apr 2026 10:10:48 +0200	[thread overview]
Message-ID: <87a4vn85iv.fsf@bootlin.com> (raw)
In-Reply-To: <141b3528-f83a-4fc3-954f-630ce45e54df@gmail.com> (Gabor Juhos's message of "Wed, 1 Apr 2026 09:13:57 +0200")

On 01/04/2026 at 09:13:57 +02, Gabor Juhos <j4g8y7@gmail.com> wrote:

> Hi Miquel,
>
>> Hi Gabor,
>> 
>> On 21/03/2026 at 15:42:32 +01, Gabor Juhos <j4g8y7@gmail.com> wrote:
>> 
>>> The mvebu_a3700_utmi_phy_power_off() function tries to modify the
>>> USB2_PHY_CTRL register by using the IO address of the PHY IP block along
>>> with the readl/writel IO accessors. However, the register exist in the
>>> USB miscellaneous register space, and as such it must be accessed via
>>> regmap like it is done in the mvebu_a3700_utmi_phy_power_on() function.
>>>
>>> Change the code to use regmap_update_bits() for modífying the register
>> 
>> Spurious accent here :-)                             ^
>> 
>> Do you imply that the register access was not working? Or that it was
>> not using the correct API? ...
>
> It was using the wrong API with wrong base address.
>
> The USB2_PHY_CTRL(x) macro gives back an offset relative to the base address of
> the USB miscellaneous registers. However the current code uses that offset with
> the base address of the UTMI PHY registers.
>
> So, instead of modifying the 'USB2 Host PHY Control' register at 0xd005f804 it
> changes the 'USB2 UTMI PHY PLL Control 1' register at 0xd005f004.

Ok, that's what I was asking.

> Since the miscellaneous registers can be accessed only via the regmap obtained
> from a syscon node we have to use the regmap API instead of the generic IO
> accessors.

Yeah yeah, I was asking to clarify the problem, whether it was a
register access issue or an API mismatch in the first place.

>> ... (hence potential issues with locking might arise)
>
> The regmap API uses built-in locking so there should be no locking
> issues.

Yes, that's what I was implying, I was talking about the bare readl()
call here (aka the "incorrect" API).

Thanks,
Miquèl

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy