Dne ponedeljek, 04. november 2019 ob 21:27:04 CET je Clément Péron napisal(a):
> Hi,
>
> On Mon, 4 Nov 2019 at 21:19, Jernej Škrabec <jernej.skrabec@xxxxxxxx> wrote:
> > Dne ponedeljek, 04. november 2019 ob 21:10:52 CET je Uwe Kleine-König
> >
> > napisal(a):
> > > Hello Clément,
> > >
> > > On Mon, Nov 04, 2019 at 07:07:00PM +0100, Clément Péron wrote:
> > > > On Mon, 4 Nov 2019 at 09:24, Uwe Kleine-König
> > > >
> > > > <u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
> > > > > On Sun, Nov 03, 2019 at 09:33:30PM +0100, Clément Péron wrote:
> > > > > > From: Jernej Skrabec <jernej.skrabec@xxxxxxxx>
> > > > > >
> > > > > > H6 PWM core needs bus clock to be enabled in order to work.
> > > > > >
> > > > > > Add an optional probe for it and a fallback for previous
> > > > > > bindings without name on module clock.
> > > > > >
> > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@xxxxxxxx>
> > > > > > Signed-off-by: Clément Péron <peron.clem@xxxxxxxxx>
> > > > > > ---
> > > > > >
> > > > > > drivers/pwm/pwm-sun4i.c | 36 ++++++++++++++++++++++++++++++++++++
> > > > > > 1 file changed, 36 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > > > > > index d194b8ebdb00..b5e7ac364f59 100644
> > > > > > --- a/drivers/pwm/pwm-sun4i.c
> > > > > > +++ b/drivers/pwm/pwm-sun4i.c
> > > > > > @@ -78,6 +78,7 @@ struct sun4i_pwm_data {
> > > > > >
> > > > > > struct sun4i_pwm_chip {
> > > > > >
> > > > > > struct pwm_chip chip;
> > > > > >
> > > > > > + struct clk *bus_clk;
> > > > > >
> > > > > > struct clk *clk;
> > > > > > struct reset_control *rst;
> > > > > > void __iomem *base;
> > > > > >
> > > > > > @@ -367,6 +368,31 @@ static int sun4i_pwm_probe(struct
> > > > > > platform_device
> > > > > > *pdev)> >
> > > > >
> > > > > Adding more context here:
> > > > > | pwm->clk = devm_clk_get(&pdev->dev, NULL);
> > > > > |
> > > > > > if (IS_ERR(pwm->clk))
> > > > > >
> > > > > > return PTR_ERR(pwm->clk);
> > > > > >
> > > > > > + /* Get all clocks and reset line */
> > > > > > + pwm->clk = devm_clk_get_optional(&pdev->dev, "mod");
> > > > > > + if (IS_ERR(pwm->clk)) {
> > > > > > + dev_err(&pdev->dev, "get clock failed %ld\n",
> > > > > > + PTR_ERR(pwm->clk));
> > > > > > + return PTR_ERR(pwm->clk);
> > > > > > + }
> > > > >
> > > > > I guess you want to drop the first assignment to pwm->clk.
> > > >
> > > > devm_clk_get_optional will return NULL if there is no entry, I don't
> > > > get where I need to drop it assignment.
> > >
> > > With your patch the code looks as follows:
> > > pwm->clk = devm_clk_get(&pdev->dev, NULL);
> > > if (IS_ERR(pwm->clk))
> > >
> > > return PTR_ERR(pwm->clk);
> > >
> > > /* Get all clocks and reset line */
> > > pwm->clk = devm_clk_get_optional(&pdev->dev, "mod");
> >
> > Actually, it's the other way around, e.g. "mod" clock is checked first.
>
> The first devm_clk_get is indeed wrong, I will remove it!
Sorry, I missed that too. Yeah, it should be removed.
Best regards,
Jernej
>
> > > ...
> > >
> > > The assignment to pwm->clk above the comment is the one I suggested to
> > > drop.
> >
> > Neither can be dropped. DT files for other SoCs don't have clock-names
> > property, so search for "mod" clock will fail and then fallback option
> > without name is used.
> >
> > Best regards,
> > Jernej
> >
> > > > > > + /* Fallback for old dtbs with a single clock and no name */
> > > > > > + if (!pwm->clk) {
> > > > > > + pwm->clk = devm_clk_get(&pdev->dev, NULL);
> > > > > > + if (IS_ERR(pwm->clk)) {
> > > > > > + dev_err(&pdev->dev, "get clock failed
> > > > > > %ld\n",
> > > > > > + PTR_ERR(pwm->clk));
> > > > > > + return PTR_ERR(pwm->clk);
> > > > > > + }
> > > > > > + }
> > > > >
> > > > > There is a slight change of behaviour if I'm not mistaken. If you
> > > > > have
> > > > >
> > > > > this:
> > > > > clocks = <&clk1>;
> > > > > clock-names = "mod";
> > > > >
> > > > > pwm {
> > > > >
> > > > > compatible = "allwinner,sun4i-a10-pwm"
> > > > > clocks = <&clk2>;
> > > > >
> > > > > }
> > > > >
> > > > > you now use clk1 instead of clk2 before.
> > > > >
> > > > > Assuming this is only a theoretical problem, at least pointing this
> > > > > out
> > > > > in the commit log would be good I think.
> > > >
> > > > Yes it's correct and as you said the driver don't check for a correct
> > > > device tree, that why it's now optional probe.
> > > > Let's assume that's the device-tree is correct, I will add a comment
> > > > in the commit log.
> > >
> > > If the mod clock was shared by all peripherals on the bus this would be
> > > IMHO quite elegant. Probably it depends on what you mean by saying
> > > "incorrect" if this snippet is incorrect. (It can be part of a valid dtb
> > > that even complies to the binding documentation. However that's not how
> > > any existing allwinner hardware looks like.) But let's stop arguing as
> > > we agree it's a corner case and if you mention it in the commit log
> > > we're both happy.
> > >
> > > > > What is that clock used for? Is it required to access the hardware
> > > > > registers? Or is it only required while the PWM is enabled? If so
> > > > > you
> > > > > could enable the clock more finegrainded.
> > > >
> > > > Regarding the datasheet it's required to access the hardware.
> > > > page 261 :
> > > > https://linux-sunxi.org/File:Allwinner_H6_V200_User_Manual_V1.1.pdf
> > >
> > > So enabling the bus clock is called "open APB1 Bus gating" in that
> > > manual? If I understand that correctly the bus clock then only need to
> > > be on while accessing the registers and could be disabled once the
> > > hardware is programmed and running.
> > >
> > > Can you please describe that in a comment. Something like:
> > > /*
> > >
> > > * We're keeping the bus clock on for the sake of simplicity.
> > > * Actually it only needs to be on for hardware register
> > > * accesses.
> > > */
> > >
> > > should be fine. This way it's at least obvious that the handling could
> > > be improved.
> > >
> > > Best regards
> > > Uwe
