Re: [PATCH v2 4/8] phy: rockchip-usb: expose the phy-internal PLLs

[Heiko Stuebner]

Hi Doug,

Am Montag, 9. November 2015, 12:59:58 schrieb Doug Anderson:
> On Sun, Nov 8, 2015 at 8:04 AM, Heiko Stuebner <heiko@xxxxxxxxx> wrote:
> > +static const struct rockchip_usb_phy_pdata rk3066a_pdata = {
> > +       .phys = (struct rockchip_usb_phys[]){
> > +               { .reg = 0x17c, .pll_name = "sclk_otgphy0_480m" },
> > +               { .reg = 0x188, .pll_name = "sclk_otgphy1_480m" },
> > +               { /* sentinel */ }
> > +       },
> > +};
> > +
> > +static const struct rockchip_usb_phy_pdata rk3188_pdata = {
> > +       .phys = (struct rockchip_usb_phys[]){
> > +               { .reg = 0x10c, .pll_name = "sclk_otgphy0_480m" },
> > +               { .reg = 0x11c, .pll_name = "sclk_otgphy1_480m" },
> > +               { /* sentinel */ }
> > +       },
> > +};
> > +
> > +static const struct rockchip_usb_phy_pdata rk3288_pdata = {
> > +       .phys = (struct rockchip_usb_phys[]){
> > +               { .reg = 0x320, .pll_name = "sclk_otgphy0_480m" },
> > +               { .reg = 0x334, .pll_name = "sclk_otgphy1_480m" },
> > +               { .reg = 0x348, .pll_name = "sclk_otgphy2_480m" },
> > +               { /* sentinel */ }
> > +       },
> 
> Slighly annoying to have to add a table for each SoC.  I'd imagine
> this growing quite large.
> 
> Would it be possible to query our parent clock name and then append a
> "_480m" suffix?  Then you can magically get the name you want, right?
> That doesn't preclude you from later overriding this or adding a "dts"
> property for it, but it means no tables for now and (hopefully) for
> the forseeable future.  ...and you get to kill a bunch of code in this
> patch...
> 
> You could fall back to some generic name based on the PHY device tree
> node name if you wanted, or use the ID allocator...
> 
> 
> Otherwise, this all looks good to me.  If you have a good reason why
> using the parent name isn't a good idea, let me know and I can add my
> Reviewed-by on this patch.

Adding a _480m suffix was my initial solution. But the phy-supply clock
is actually optional in the binding, so it may not be specified at all.
Still the code needs this pll to work at all after the change, and
inventing some completely random clock name seemed very strange
(like pll_usbphy_REG or something) - especially as this would never
have worked with the downstream clocks. As it is now, the downstream
clock-tree will work indepent of the supply clock being specified.

Also Rockchip seems to have moved to a different phy-ip, with one
supplying clock, one pll and everything more tightly coupled,
so we're probably getting a new driver for it, instead of additional
entries for the picophy.


Heiko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/