Linux-PHY Archive on lore.kernel.org

* Re: [PATCH v2 2/6] phy: realtek: usb2: introduce read and write functions to driver data
From: Michael Zavertkin @ 2026-03-31 16:51 UTC (permalink / raw)
  To: linux-phy
In-Reply-To: <20260330213227.zujvcvsxdfknzltz@skbuf>

On Tue, Mar 31, 2026 at 12:32:27AM +0300, 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?
> 
>   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?
> 
>   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.
>  */
There is readl() (and family) function. For most hosts it returns native
endian value (because most systems LE nowadays).
I don't know all context, and don't know exactly why... but for MIPS readl()
returns value in native endianess.
Reference - https://elixir.bootlin.com/linux/v6.19.10/source/arch/mips/include/asm/io.h

So we are in interesting situation, when readl() returns native-endian values (BE in our case),
readl_be() returns BE too, because cpu_to_be32() does nothing on BE system, ioread32() returns BE,
but ioread32be() returns LE because of unconditional swab32(). Using *be
to get LE value... kinda weird. That's why we ended up with these two
functions.

Not a perfect solution, so it would be good to hear others opinions and
come to a more proper solution.
UPD. regmap looks like a pretty good solution for endianess stuff, but
it seems to interfere with current register handling (using struct
offsets as register addresses). So it has to be a big work.

For example, Realtek solves this issue that way -
https://github.com/jameywine/GPL-for-GP3000/blob/5090fbcb6ba743dd7b1314811ef557bad0460147/linux-5.10.x/drivers/usb/host/ehci.h#L742
> 
> 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]
> 
> 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?

Thanks for all other review comments, going to be fixed in next patch
series version.

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

^ permalink raw reply