Re: [PATCH v2 2/6] phy: realtek: usb2: introduce read and write functions to driver data

[Rustam Adilov]

On 2026-03-30 21:32, Vladimir Oltean wrote:
> On Tue, Mar 31, 2026 at 12:19:18AM +0300, Vladimir Oltean wrote:
>> On Fri, Mar 27, 2026 at 09:06:34PM +0500, Rustam Adilov wrote:
>> > +static inline u32 phy_read(void __iomem *reg)
>> > +{
>> > +	return readl(reg);
>> > +}
>> > +
>> > +static inline u32 phy_read_le(void __iomem *reg)
>> > +{
>> > +	return le32_to_cpu(readl(reg));
>> > +}
>> > +
>> > +static inline void phy_write(u32 val, void __iomem *reg)
>> > +{
>> > +	writel(val, reg);
>> > +}
>> > +
>> > +static inline void phy_write_le(u32 val, void __iomem *reg)
>> > +{
>> > +	writel(cpu_to_le32(val), reg);
>> > +}
>> 
>> Please don't name driver-level functions phy_read() and phy_write().
>> That will collide with networking API functions of the same name and
>> will make grep-based code searching more difficult.
>> 
>> Also, have you looked at regmap? It has native support for endianness;
>> it supports regmap_field_read()/regmap_field_write() for abstracting
>> registers which may be found at different places for different HW;
>> it offers regmap_read_poll_timeout() so you don't have to pass the
>> function pointer to utmi_wait_register(). It seems the result would be a
>> bit more elegant.
> 
> Even if you decide not to use regmap. I thought I should let you know
> that LLM review says:
> 
>   Are these double byte-swaps intentional?
> 
>   Since readl() and writel() inherently perform little-endian memory accesses
>   and handle byte-swapping on big-endian architectures automatically, won't
>   wrapping them in le32_to_cpu() and cpu_to_le32() apply a second, redundant
>   byte-swap?

>From my experience (and also understanding), readl returns value in native endian
and doesn't do byte swapping. The same goes writel.
Thus wrapping le32_to_cpu() and cpu_to_le32() around them correctly swaps the
bytes from big endian to little endian, without the double byte-swaps.

The comment even mentions it "{read,write}{b,w,l,q}() access little endian memory
and return result in native endianness." which was provided for reference later on.

>   On big-endian systems, wouldn't these double swaps cancel each other out
>   and result in a native big-endian access instead of the intended
>   little-endian access? If the SoC bus bridge implicitly swaps and requires
>   a native access, should __raw_readl() and __raw_writel() (or ioread32be /
>   iowrite32be) be used instead to avoid obfuscating it with double-swaps?

The __raw_readl() and __raw_writel() are indeed in native endian, and for
RTL9607C SoC it is in big endian.

The way Realtek did it is by using the volatile and wrapping them around
le32_to_cpu() and cpu_to_le32() respectively, which is certainly hacky.

 static inline unsigned int ehci_readl(const struct ehci_hcd *ehci,
 		__u32 __iomem *regs)
 {
+#if defined(CONFIG_RTK_MIPS_SOC)
+	return (le32_to_cpu((*(volatile unsigned long *)(regs))));
+#else
[...]

 static inline void ehci_writel(const struct ehci_hcd *ehci,
 		const unsigned int val, __u32 __iomem *regs)
 {
+#if defined(CONFIG_RTK_MIPS_SOC)
+	((*(volatile unsigned long *)(regs))=cpu_to_le32(val));
+#else

We did a bit of debugging with usb some time ago and and printed the value
of the readl result from the base usb address and got this
[    1.327473] ehci_setup:694 ehci caps: 0xb8021000, value: 0x10000001
[    1.334478] ehci_setup:695 ehci regs: 0xb8021001
[    1.339706] ehci_halt:187 ehci regs: 0xb8021001

"0x10000001" is supposed to be "0x01000010". Otherwise, it would take 0x01
for the cap length and result in immediate halt from timeout.

Even though it is for linux-usb and not the phy subsystem, it was worth
mentioning as it is very much related to this issue.

What is surprising, the ioread32be and iowrite32be functions actually do
swap bytes, thus resulting in little endian (ironic). But using it here
would be just incorrect as it is intended for accessing big endian mmio, 
not for big endian CPU and its little endian USB phy/host.

So, that is a conundrum we have here. Let me know if what you think of it
and maybe even how to better solve it.

Will hold on posting v3 until this hopefully is more or less solved..

>   Also, does passing the __le32 restricted type returned by cpu_to_le32()
>   into writel() (which expects a native u32) trigger Sparse static analysis
>   warnings for an incorrect type in argument?
> 
> For reference:
> https://elixir.bootlin.com/linux/v6.19.10/source/include/asm-generic/io.h#L184
> /*
>  * {read,write}{b,w,l,q}() access little endian memory and return result in
>  * native endianness.
>  */
> 
> and yes, your patch does trigger sparse warnings:
> ../drivers/phy/realtek/phy-rtk-usb2.c:153:16: warning: cast to restricted __le32
> ../drivers/phy/realtek/phy-rtk-usb2.c:163:16: warning: incorrect type in argument 1 (different base types)
> ../drivers/phy/realtek/phy-rtk-usb2.c:163:16:    expected unsigned int val
> ../drivers/phy/realtek/phy-rtk-usb2.c:163:16:    got restricted __le32 [usertype]

I can alleviate it by creating a temp u32 variable to hold the cpu_to_le32() and then
using that temp variable for writel.

> Furthermore, please drop the 'inline' keyword from C files and let the
> compiler decide. Your use of this keyword has no value - you declare
> phy_read(), phy_read_le() etc as inline but then assign function
> pointers to them. How can the compiler inline the indirect calls?

Will drop these.