Re: [PATCH v3 10/11] phy: sun4i-usb: Replace types with explicit quirk flags

[Andre Przywara <andre.przywara@arm.com>]

On Thu, 10 Nov 2022 19:40:58 +0800
Icenowy Zheng <uwu@icenowy.me> wrote:

Hi,

> 在 2022-11-10星期四的 13:04 +0530，Vinod Koul写道：
> > On 06-11-22, 23:54, Icenowy Zheng wrote:  
> > > 
> > > 
> > > 于 2022年11月6日 GMT+08:00 下午11:48:25, Andre Przywara
> > > <andre.przywara@arm.com> 写到:  
> > > > So far we were assigning some crude "type" (SoC name, really) to
> > > > each
> > > > Allwinner USB PHY model, then guarding certain quirks based on
> > > > this.
> > > > This does not only look weird, but gets more or more cumbersome
> > > > to
> > > > maintain.
> > > > 
> > > > Remove the bogus type names altogether, instead introduce flags
> > > > for each
> > > > quirk, and explicitly check for them.
> > > > This improves readability, and simplifies future extensions.
> > > > 
> > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > > > ---
> > > > drivers/phy/allwinner/phy-sun4i-usb.c | 50 ++++++++--------------
> > > > -----
> > > > 1 file changed, 15 insertions(+), 35 deletions(-)
> > > > 
> > > > diff --git a/drivers/phy/allwinner/phy-sun4i-usb.c
> > > > b/drivers/phy/allwinner/phy-sun4i-usb.c
> > > > index 51fb24c6dcb3..422129c66282 100644
> > > > --- a/drivers/phy/allwinner/phy-sun4i-usb.c
> > > > +++ b/drivers/phy/allwinner/phy-sun4i-usb.c
> > > > @@ -99,27 +99,17 @@
> > > > #define DEBOUNCE_TIME                   msecs_to_jiffies(50)
> > > > #define POLL_TIME                       msecs_to_jiffies(250)
> > > > 
> > > > -enum sun4i_usb_phy_type {
> > > > -       sun4i_a10_phy,
> > > > -       sun6i_a31_phy,
> > > > -       sun8i_a33_phy,
> > > > -       sun8i_a83t_phy,
> > > > -       sun8i_h3_phy,
> > > > -       sun8i_r40_phy,
> > > > -       sun8i_v3s_phy,
> > > > -       sun50i_a64_phy,
> > > > -       sun50i_h6_phy,
> > > > -};
> > > > -
> > > > struct sun4i_usb_phy_cfg {
> > > >         int num_phys;
> > > >         int hsic_index;
> > > > -       enum sun4i_usb_phy_type type;
> > > >         u32 disc_thresh;
> > > >         u32 hci_phy_ctl_clear;
> > > >         u8 phyctl_offset;
> > > >         bool dedicated_clocks;
> > > >         bool phy0_dual_route;
> > > > +       bool phy2_is_hsic;  
> > > 
> > > Maybe use a `int hsic_phy` instead? But the problem is this
> > > practice is
> > > assuming USB0 could not be HSIC -- although USB0 is usually OTG.  
> > 
> > why should it be int.. dont think hsic_phy is improvement over
> > phy2_is_hsic?  
> 
> Yes because it may express phy1_is_hsic, etc (although this kind of
> thing hadn't happened yet).

Yeah, I tried to not interpret too much into this, instead just named it
as it is used today. I don't have any insight into the A83T PHY or in
Allwinner's plans regarding this. So far this seems like a one-off hack
that is needed for this particular PHY on this particular SoC.
It it code-internal anyway, so we can change it at any time later should
the need arise.

If people like another name better, I am of course happy to use that.

Cheers,
Andre

> 
> >   
> > >   
> > > > +       bool siddq_in_base;
> > > > +       bool poll_vbusen;
> > > >         int missing_phys;
> > > > };
> > > > 
> > > > @@ -251,7 +241,7 @@ static void sun4i_usb_phy_passby(struct
> > > > sun4i_usb_phy *phy, int enable)
> > > >                 SUNXI_AHB_INCRX_ALIGN_EN | SUNXI_ULPI_BYPASS_EN;
> > > > 
> > > >         /* A83T USB2 is HSIC */
> > > > -       if (phy_data->cfg->type == sun8i_a83t_phy && phy->index
> > > > == 2)
> > > > +       if (phy_data->cfg->phy2_is_hsic && phy->index == 2)
> > > >                 bits |= SUNXI_EHCI_HS_FORCE |
> > > > SUNXI_HSIC_CONNECT_INT |
> > > >                         SUNXI_HSIC;
> > > > 
> > > > @@ -295,8 +285,7 @@ static int sun4i_usb_phy_init(struct phy
> > > > *_phy)
> > > >                 writel(val, phy->pmu + REG_HCI_PHY_CTL);
> > > >         }
> > > > 
> > > > -       if (data->cfg->type == sun8i_a83t_phy ||
> > > > -           data->cfg->type == sun50i_h6_phy) {
> > > > +       if (data->cfg->siddq_in_base) {
> > > >                 if (phy->index == 0) {
> > > >                         val = readl(data->base + data->cfg-  
> > > > >phyctl_offset);  
> > > >                         val |= PHY_CTL_VBUSVLDEXT;
> > > > @@ -340,8 +329,7 @@ static int sun4i_usb_phy_exit(struct phy
> > > > *_phy)
> > > >         struct sun4i_usb_phy_data *data =
> > > > to_sun4i_usb_phy_data(phy);
> > > > 
> > > >         if (phy->index == 0) {
> > > > -               if (data->cfg->type == sun8i_a83t_phy ||
> > > > -                   data->cfg->type == sun50i_h6_phy) {
> > > > +               if (data->cfg->siddq_in_base) {
> > > >                         void __iomem *phyctl = data->base +
> > > >                                 data->cfg->phyctl_offset;
> > > > 
> > > > @@ -414,9 +402,8 @@ static bool sun4i_usb_phy0_poll(struct
> > > > sun4i_usb_phy_data *data)
> > > >          * vbus using the N_VBUSEN pin on the pmic, so we must
> > > > poll
> > > >          * when using the pmic for vbus-det _and_ we're driving
> > > > vbus.
> > > >          */
> > > > -       if ((data->cfg->type == sun6i_a31_phy ||
> > > > -            data->cfg->type == sun8i_a33_phy) &&
> > > > -           data->vbus_power_supply && data-  
> > > > >phys[0].regulator_on)  
> > > > +       if (data->cfg->poll_vbusen && data->vbus_power_supply &&
> > > > +           data->phys[0].regulator_on)
> > > >                 return true;
> > > > 
> > > >         return false;
> > > > @@ -861,7 +848,6 @@ static int sun4i_usb_phy_probe(struct
> > > > platform_device *pdev)
> > > > 
> > > > static const struct sun4i_usb_phy_cfg suniv_f1c100s_cfg = {
> > > >         .num_phys = 1,
> > > > -       .type = sun4i_a10_phy,
> > > >         .disc_thresh = 3,
> > > >         .phyctl_offset = REG_PHYCTL_A10,
> > > >         .dedicated_clocks = true,
> > > > @@ -869,7 +855,6 @@ static const struct sun4i_usb_phy_cfg
> > > > suniv_f1c100s_cfg = {
> > > > 
> > > > static const struct sun4i_usb_phy_cfg sun4i_a10_cfg = {
> > > >         .num_phys = 3,
> > > > -       .type = sun4i_a10_phy,
> > > >         .disc_thresh = 3,
> > > >         .phyctl_offset = REG_PHYCTL_A10,
> > > >         .dedicated_clocks = false,
> > > > @@ -877,7 +862,6 @@ static const struct sun4i_usb_phy_cfg
> > > > sun4i_a10_cfg = {
> > > > 
> > > > static const struct sun4i_usb_phy_cfg sun5i_a13_cfg = {
> > > >         .num_phys = 2,
> > > > -       .type = sun4i_a10_phy,
> > > >         .disc_thresh = 2,
> > > >         .phyctl_offset = REG_PHYCTL_A10,
> > > >         .dedicated_clocks = false,
> > > > @@ -885,15 +869,14 @@ static const struct sun4i_usb_phy_cfg
> > > > sun5i_a13_cfg = {
> > > > 
> > > > static const struct sun4i_usb_phy_cfg sun6i_a31_cfg = {
> > > >         .num_phys = 3,
> > > > -       .type = sun6i_a31_phy,
> > > >         .disc_thresh = 3,
> > > >         .phyctl_offset = REG_PHYCTL_A10,
> > > >         .dedicated_clocks = true,
> > > > +       .poll_vbusen = true,
> > > > };
> > > > 
> > > > static const struct sun4i_usb_phy_cfg sun7i_a20_cfg = {
> > > >         .num_phys = 3,
> > > > -       .type = sun4i_a10_phy,
> > > >         .disc_thresh = 2,
> > > >         .phyctl_offset = REG_PHYCTL_A10,
> > > >         .dedicated_clocks = false,
> > > > @@ -901,31 +884,31 @@ static const struct sun4i_usb_phy_cfg
> > > > sun7i_a20_cfg = {
> > > > 
> > > > static const struct sun4i_usb_phy_cfg sun8i_a23_cfg = {
> > > >         .num_phys = 2,
> > > > -       .type = sun6i_a31_phy,
> > > >         .disc_thresh = 3,
> > > >         .phyctl_offset = REG_PHYCTL_A10,
> > > >         .dedicated_clocks = true,
> > > > +       .poll_vbusen = true,
> > > > };
> > > > 
> > > > static const struct sun4i_usb_phy_cfg sun8i_a33_cfg = {
> > > >         .num_phys = 2,
> > > > -       .type = sun8i_a33_phy,
> > > >         .disc_thresh = 3,
> > > >         .phyctl_offset = REG_PHYCTL_A33,
> > > >         .dedicated_clocks = true,
> > > > +       .poll_vbusen = true,
> > > > };
> > > > 
> > > > static const struct sun4i_usb_phy_cfg sun8i_a83t_cfg = {
> > > >         .num_phys = 3,
> > > >         .hsic_index = 2,
> > > > -       .type = sun8i_a83t_phy,
> > > >         .phyctl_offset = REG_PHYCTL_A33,
> > > >         .dedicated_clocks = true,
> > > > +       .siddq_in_base = true,
> > > > +       .phy2_is_hsic = true,
> > > > };
> > > > 
> > > > static const struct sun4i_usb_phy_cfg sun8i_h3_cfg = {
> > > >         .num_phys = 4,
> > > > -       .type = sun8i_h3_phy,
> > > >         .disc_thresh = 3,
> > > >         .phyctl_offset = REG_PHYCTL_A33,
> > > >         .dedicated_clocks = true,
> > > > @@ -935,7 +918,6 @@ static const struct sun4i_usb_phy_cfg
> > > > sun8i_h3_cfg = {
> > > > 
> > > > static const struct sun4i_usb_phy_cfg sun8i_r40_cfg = {
> > > >         .num_phys = 3,
> > > > -       .type = sun8i_r40_phy,
> > > >         .disc_thresh = 3,
> > > >         .phyctl_offset = REG_PHYCTL_A33,
> > > >         .dedicated_clocks = true,
> > > > @@ -945,7 +927,6 @@ static const struct sun4i_usb_phy_cfg
> > > > sun8i_r40_cfg = {
> > > > 
> > > > static const struct sun4i_usb_phy_cfg sun8i_v3s_cfg = {
> > > >         .num_phys = 1,
> > > > -       .type = sun8i_v3s_phy,
> > > >         .disc_thresh = 3,
> > > >         .phyctl_offset = REG_PHYCTL_A33,
> > > >         .dedicated_clocks = true,
> > > > @@ -955,16 +936,15 @@ static const struct sun4i_usb_phy_cfg
> > > > sun8i_v3s_cfg = {
> > > > 
> > > > static const struct sun4i_usb_phy_cfg sun20i_d1_cfg = {
> > > >         .num_phys = 2,
> > > > -       .type = sun50i_h6_phy,
> > > >         .phyctl_offset = REG_PHYCTL_A33,
> > > >         .dedicated_clocks = true,
> > > >         .hci_phy_ctl_clear = PHY_CTL_SIDDQ,
> > > >         .phy0_dual_route = true,
> > > > +       .siddq_in_base = true,
> > > > };
> > > > 
> > > > static const struct sun4i_usb_phy_cfg sun50i_a64_cfg = {
> > > >         .num_phys = 2,
> > > > -       .type = sun50i_a64_phy,
> > > >         .disc_thresh = 3,
> > > >         .phyctl_offset = REG_PHYCTL_A33,
> > > >         .dedicated_clocks = true,
> > > > @@ -974,11 +954,11 @@ static const struct sun4i_usb_phy_cfg
> > > > sun50i_a64_cfg = {
> > > > 
> > > > static const struct sun4i_usb_phy_cfg sun50i_h6_cfg = {
> > > >         .num_phys = 4,
> > > > -       .type = sun50i_h6_phy,
> > > >         .phyctl_offset = REG_PHYCTL_A33,
> > > >         .dedicated_clocks = true,
> > > >         .phy0_dual_route = true,
> > > >         .missing_phys = BIT(1) | BIT(2),
> > > > +       .siddq_in_base = true,
> > > > };
> > > > 
> > > > static const struct of_device_id sun4i_usb_phy_of_match[] = {  
> >   
>