On 11/1/22 16:07, Stephen Boyd wrote: > Quoting Sean Anderson (2022-10-28 09:13:57) >> On 10/27/22 19:03, Stephen Boyd wrote: >> > Quoting Sean Anderson (2022-10-27 12:11:08) >> >> diff --git a/drivers/phy/freescale/Kconfig b/drivers/phy/freescale/Kconfig >> >> index 853958fb2c06..a6ccccf9e39b 100644 >> >> --- a/drivers/phy/freescale/Kconfig >> >> +++ b/drivers/phy/freescale/Kconfig >> >> @@ -47,3 +47,25 @@ config PHY_FSL_LYNX_28G >> >> found on NXP's Layerscape platforms such as LX2160A. >> >> Used to change the protocol running on SerDes lanes at runtime. >> >> Only useful for a restricted set of Ethernet protocols. >> >> + >> >> +config PHY_FSL_LYNX_10G >> >> + tristate "Freescale QorIQ Lynx 10G SerDes support" >> >> + depends on COMMON_CLK >> > >> > Does something not compile if COMMON_CLK is disabled? >> >> ld: drivers/phy/freescale/phy-fsl-lynx-10g-clk.o: in function `lynx_pll_round_rate': >> phy-fsl-lynx-10g-clk.c:(.text+0x444): undefined reference to `clk_hw_round_rate' >> ld: drivers/phy/freescale/phy-fsl-lynx-10g-clk.o: in function `lynx_clks_init': >> phy-fsl-lynx-10g-clk.c:(.text+0x5eb): undefined reference to `devm_clk_hw_register' >> ld: phy-fsl-lynx-10g-clk.c:(.text+0x625): undefined reference to `devm_clk_hw_register' > > Cool thanks! > >> >> >> diff --git a/drivers/phy/freescale/lynx-10g.h b/drivers/phy/freescale/lynx-10g.h >> >> new file mode 100644 >> >> index 000000000000..75d9353a867b >> >> --- /dev/null >> >> +++ b/drivers/phy/freescale/lynx-10g.h >> >> @@ -0,0 +1,16 @@ >> >> +/* SPDX-License-Identifier: GPL-2.0 */ >> >> +/* >> >> + * Copyright (C) 2022 Sean Anderson <sean.anderson@xxxxxxxx> >> >> + */ >> >> + >> >> +#ifndef LYNX_10G >> >> +#define LYNX_10G >> >> + >> >> +struct clk; >> >> +struct device; >> >> +struct regmap; >> >> + >> >> +int lynx_clks_init(struct device *dev, struct regmap *regmap, >> > >> > Can you use auxiliary bus to register this clk controller instead and >> > then move the clk file to drivers/clk/? >> >> I don't want to have to deal with my clock driver getting unbound (aka >> the user has come and decided to make my life harder). Dynamic binding >> will only add complexity in this situation. > > We have 'suppress_bind_attrs' for that. What dynamic binding are you > thinking about? Huh, neat. That may save me a lot of trouble in another subsystem. This was actually my main objection. I'll have a look at this bus... >> >> I don't know how much context you've picked up, but this driver >> >> - Has one consumer, and is is the serdes. >> - Is not accessible from outside the serdes. >> - Does not share any code with other drivers. >> - Has bits in its registers which can control the reset process of lanes >> using the PLLs. >> >> These drivers are tightly coupled to each other. It is very likely IMO >> that changes to one (bugs, features, etc) will affect the other. For >> this reason, I think it makes sense to keep them in the same source >> directory. I actually would have preferred to keep them in the same >> file. >> > > Using the auxiliary bus is about getting better code review on the > subsystem specific parts of a device. I'm not going to be paying a lot > of attention to the clk parts of this driver if it is outside > drivers/clk. Making this change helps with better code review. > > The Kconfig symbol could be the same for the clk part and the phy part, > and this is already split to a different file. It seems that your > argument for keeping the clk file in the phy directory is because > they're part of the same phy device. Do you expect to get clk driver > review on the clk parts with the clk implementation in a different > directory? ...yes? Actually, I was mainly hoping for phy driver review (since I consider these PLLs part of the serdes), but you seem to be much more responsive, so I'll take what I can get. >> >> + for (i = 0; i < NUM_PLLS; i++) { >> >> + ret = lynx_clk_init(hw_data, dev, regmap, i); >> >> + if (ret) >> >> + return ret; >> >> + >> >> + plls[i] = hw_data->hws[LYNX10G_PLLa(i)]->clk; >> >> + ex_dlys[i] = hw_data->hws[LYNX10G_PLLa_EX_DLY(i)]->clk; >> > >> > Use clk_hw_get_clk() please. >> >> I don't want to do that, because then I'd have to generate the clock ID >> again. And why do we even need a new clock consumer in the first place? >> This is only for internal use by the driver; the consumer is the same as >> the producer. >> > > We want to get rid of the clk pointer stashed in clk_hw. Using > clk_hw_get_clk() lets us easily change that and not need to change this > driver. I don't quite understand why you need to generate a clock ID. > Are you concerned about passing in the 'con_id' argument? That string > can be anything. Yeah, but it has to be unique (there are usually multiple serdes on the same system), and it should be something useful (so e.g. clock_summary is useful). So it needs to be generated at runtime, and I'm already doing it when I create the clock. IMO this would be much easier with numeric IDs; generating strings is a pain. --Sean