RE: [PATCH v1 2/3] usb: phy: Add driver for the Realtek SoC USB 2.0/3.0 PHY

[Stanley Chang[昌育德]]

Hi Arnd,

> > +#ifdef CONFIG_DEBUG_FS
> > +     struct dentry *debug_dir;
> > +#endif
> > +};
> 
> I'd avoid the #ifdefs here and just leave the debugfs code in unconditionally in
> favor of readability. When debugfs is disabled, everything except for the one
> pointer value should get optimized out.

I will remove this #ifdef.

> > +#define phy_read(addr) __raw_readl(addr) #define phy_write(addr, val)
> > +do { \
> > +     /* Do smp_wmb */ \
> > +     smp_wmb(); __raw_writel(val, addr); \ } while (0)
> 
> Using __raw_readl()/__raw_writel() in a driver is almost never the right
> interface, it does not guarantee atomicity of the access, has the wrong
> byte-order on big-endian kernels and misses the barriers to serialize against
> DMA accesses. smp_wmb() should have no effect here since this only serializes
> access to memory against another CPU if it's paired with an smp_rmb(), but
> not on MMIO registers.

I will try using readl/writel directly.

> > +#define PHY_IO_TIMEOUT_MSEC          (50)
> > +
> > +static inline int utmi_wait_register(void __iomem *reg, u32 mask, u32
> > result)
> > +{
> > +     unsigned long timeout = jiffies +
> > msecs_to_jiffies(PHY_IO_TIMEOUT_MSEC);
> > +
> > +     while (time_before(jiffies, timeout)) {
> > +             /* Do smp_rmb */
> > +             smp_rmb();
> > +             if ((phy_read(reg) & mask) == result)
> > +                     return 0;
> > +             udelay(100);
> > +     }
> > +     pr_err("\033[0;32;31m can't program USB phy \033[m\n");
> > +
> > +     return -ETIMEDOUT;
> > +}
> 
> This should just use read_poll_timeout() or possibly
> read_poll_timeout_atomic(), but not an open-coded version.
> 
I've tried using read_poll_timeout() instead and it works.

> I don't think I've seen escape sequences in a printk in any other driver, so
> please don't start that now.

Okay. I will remove it.

> > +#define DEFAULT_CHIP_REVISION 0xA00
> > +#define MAX_CHIP_REVISION 0xC00
> > +
> > +static inline int __get_chip_revision(void) {
> > +     int chip_revision = 0xFFF;
> > +     char revision[] = "FFF";
> > +     struct soc_device_attribute soc_att[] = {{.revision = revision},
> > +{}};
> 
> You should probably check that you are actually on the right SoC type here as
> well, not just the right revision of an arbitrary chip.
> 
> Ideally I think the revision check should be based off a DT proporty if that's
> possible, so you can have this code in the boot loader.

In this case I just care in the chip version number.
Because the revision number is not recorded in the DTB.

> > +#define RTK_USB2PHY_NAME "rtk-usb2phy"
> 
> Better avoid hiding the driver name like this, it makes it harder to grep the
> source tree for particular driver names.

I will remove this coding style.

> > +     /* rmb for reg read */
> > +     smp_rmb();
> > +     regVal = phy_read(reg_gusb2phyacc0);
> 
> I would expect that you don't need barriers like this, especially if you change
> the phy_read() helper to use the proper readl().
> 
> If you do need to serialize against other CPUs, still, there should be a longer
> explanation about that, since it's so unexpected.

I will use readl to instead the phy_read and remove smp_rmb.

> > +
> > +static void do_rtk_usb2_phy_toggle(struct rtk_usb_phy *rtk_phy,
> > +         int index, bool isConnect);
> 
> It's best to sort your function definitions in a way that avoids forward
> declarations. This makes it easier to read and shows that there are no obvious
> recursions in the source. If you do have an intentional recursion, make sure that
> there is a way to prevent unbounded stack usage, and explain that in a
> comment.

Ok, I'll move this function to just before the first use.


> > +     regAddr = &((struct reg_addr *)rtk_phy->reg_addr)[index];
> > +     phy_data = &((struct phy_data *)rtk_phy->phy_data)[index];

> Why do you need the casts here? It looks like regAddr should be an __iomem
> pointer. Please build your driver with 'make C=1'
> to see if there are any incorrect address space annotations.

I define member reg_addr as
void *reg_addr
So I have to convert it to "struct reg_addr" and use it as array.
And I ran "make C=1" with no error or warning.

> > +static int __get_phy_parameter_by_efuse(struct rtk_usb_phy *rtk_phy,
> > +         struct phy_data *phy_data, int index) {
> > +     u8 value = 0;
> > +     struct nvmem_cell *cell;
> > +     struct soc_device_attribute rtk_soc_groot[] = {
> > +                     { .family = "Realtek Groot",},
> > +                     { /* empty */ }
> > +             };
> > +     struct soc_device_attribute rtk_soc_hank[] = {
> > +                     { .family = "Realtek Hank",},
> > +                     { /* empty */ }
> > +             };
> > +     struct soc_device_attribute rtk_soc_efuse_v1[] = {
> > +                     { .family = "Realtek Phoenix",},
> > +                     { .family = "Realtek Kylin",},
> > +                     { .family = "Realtek Hercules",},
> > +                     { .family = "Realtek Thor",},
> > +                     { .family = "Realtek Hank",},
> > +                     { .family = "Realtek Groot",},
> > +                     { .family = "Realtek Stark",},
> > +                     { .family = "Realtek Parker",},
> > +                     { /* empty */ }
> > +             };
> > +     struct soc_device_attribute rtk_soc_dis_level_at_page0[] = {
> > +                     { .family = "Realtek Phoenix",},
> > +                     { .family = "Realtek Kylin",},
> > +                     { .family = "Realtek Hercules",},
> > +                     { .family = "Realtek Thor",},
> > +                     { .family = "Realtek Hank",},
> > +                     { .family = "Realtek Groot",},
> > +                     { /* empty */ }
> > +             };
> > +
> > +     if (soc_device_match(rtk_soc_efuse_v1)) {
> > +             dev_dbg(rtk_phy->dev, "Use efuse v1 to updated phy
> parameter\n");
> > +             phy_data->check_efuse_version = CHECK_EFUSE_V1;
> 
> I'm not entirely sure what you are trying to do here, but it looks the purpose is
> to tell the difference between implementations of the phy device by looking at
> which SoC it's in. You should only need that very rarely when this information
> cannot be passed through the DT, but you literally already have the per-SoC
> compatible strings below, so just use those, or add other DT properties in the
> binding for specific quirks or capabilities.

My purpose is to judge different SoCs and set different parameters.
The DTS property might be a good way to go, I'll check to see if it's a good fit.

> Remove that of_match_ptr() and ifdef CONFIG_OF check here, new drivers
> should no longer use static platform device definitions and just assume that
> CONFIG_OF is used.
> 
Ok, I will remove it.

Thanks,
Stanley