From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chen-Yu Tsai Subject: Re: Re: [PATCH 4/6] pwm: sun4i: Add support for H6 PWM Date: Tue, 30 Jul 2019 16:32:53 +0800 Message-ID: References: <20190726184045.14669-1-jernej.skrabec@siol.net> <173825848.1FZsmuHfpq@jernej-laptop> <20190729185108.tpilwoooxvi2z72e@pengutronix.de> <2452836.v7ux4bnEjb@jernej-laptop> <20190730080900.hhxrqun7vk4nsj2h@pengutronix.de> Reply-To: wens-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Return-path: Sender: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org In-Reply-To: <20190730080900.hhxrqun7vk4nsj2h-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> List-Post: , List-Help: , List-Archive: , List-Unsubscribe: , To: =?UTF-8?Q?Uwe_Kleine=2DK=C3=B6nig?= , Rob Herring , Frank Rowand Cc: linux-sunxi , =?UTF-8?Q?Jernej_=C5=A0krabec?= , Mark Rutland , linux-pwm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree , linux-kernel , Maxime Ripard , Thierry Reding , Sascha Hauer , linux-arm-kernel List-Id: devicetree@vger.kernel.org On Tue, Jul 30, 2019 at 4:09 PM Uwe Kleine-K=C3=B6nig wrote: > > Hello Rob and Frank, > > Maxime and Jernej on one side and me on the other cannot agree about a > detail in the change to the bindings here. I'm trying to objectively > summarize the situation for you to help deciding what is the right thing > to do here. > > TLDR: The sun4i pwm driver is extended to support a new variant of that > device on the H6 SoC. Compared to the earlier supported variants > allwinner,sun50i-h6-pwm on H6 needs to handle a reset controller and an > additional clock. > > The two positions are: > > - We need a new compatible because only then the driver and/or the dt > schema checker can check that each "allwinner,sun50i-h6-pwm" device > has a reset property and a "bus" clock; and the earlier variants > don't. > > - The driver can be simpler and the device specific knowledge is only > in a single place (the dt) if the device tree is considered valid and > a reset property and "bus" clock is used iff it's provided in the > device tree without additional comparison for the compatible. > > Now our arguments seem to go in circles and Jernej was interested in > your position. That's something I agree with ;-) Can you please share > your view? > > Find below some context about the arguments. A bit more context on the failure modes: If the reset control is missing, anything done to hardware will be silently ignored, since any writes to the registers are ignored. On the other hand, if the bus clock is missing and otherwise not enabled, accessing the device's registers could actually stall the whole system. ChenYu > Best regards > Uwe > > On Tue, Jul 30, 2019 at 12:04:47AM +0200, Jernej =C5=A0krabec wrote: > > Dne ponedeljek, 29. julij 2019 ob 20:51:08 CEST je Uwe Kleine-K=C3=B6ni= g > > napisal(a): > > > On Mon, Jul 29, 2019 at 08:46:25PM +0200, Jernej =C5=A0krabec wrote: > > > > Dne ponedeljek, 29. julij 2019 ob 20:40:41 CEST je Uwe Kleine-K=C3= =B6nig > > > > napisal(a): > > > > > On Mon, Jul 29, 2019 at 06:40:15PM +0200, Jernej =C5=A0krabec wro= te: > > > > > > Dne ponedeljek, 29. julij 2019 ob 18:24:28 CEST je Uwe Kleine-K= =C3=B6nig > > > > > > napisal(a): > > > > > > > On Tue, Jul 30, 2019 at 12:09:40AM +0800, Chen-Yu Tsai wrote: > > > > > > > > On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-K=C3=B6nig > > > > > > > > wrote: > > > > > > > > > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej =C5=A0kr= abec wrote: > > > > > > > > > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe = Kleine-K=C3=B6nig > > > > > > > > > > napisal(a): > > > > > > > > > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skra= bec wrote: > > > > > > > > > > > > --- a/drivers/pwm/pwm-sun4i.c > > > > > > > > > > > > +++ b/drivers/pwm/pwm-sun4i.c > > > > > > > > > > > > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_= data > > > > > > > > > > > > sun4i_pwm_single_bypass =3D {> > > > > > > > > > > > > > > > > > > > > > > > > .npwm =3D 1, > > > > > > > > > > > > > > > > > > > > > > > > }; > > > > > > > > > > > > > > > > > > > > > > > > +static const struct sun4i_pwm_data > > > > > > > > > > > > sun50i_pwm_dual_bypass_clk_rst > > > > > > > > > > > > =3D { > > > > > > > > > > > > + .has_bus_clock =3D true, > > > > > > > > > > > > + .has_prescaler_bypass =3D true, > > > > > > > > > > > > + .has_reset =3D true, > > > > > > > > > > > > + .npwm =3D 2, > > > > > > > > > > > > +}; > > > > > > > > > > > > + > > > > > > > > > > > > > > > > > > > > > > > > static const struct of_device_id sun4i_pwm_dt_ids[= ] =3D { > > > > > > > > > > > > > > > > > > > > > > > > { > > > > > > > > > > > > > > > > > > > > > > > > .compatible =3D "allwinner,sun4i-a10-pwm"= , > > > > > > > > > > > > > > > > > > > > > > > > @@ -347,6 +354,9 @@ static const struct of_device_i= d > > > > > > > > > > > > sun4i_pwm_dt_ids[] =3D > > > > > > > > > > > > { > > > > > > > > > > > > > > > > > > > > > > > > }, { > > > > > > > > > > > > > > > > > > > > > > > > .compatible =3D "allwinner,sun8i-h3-pwm", > > > > > > > > > > > > .data =3D &sun4i_pwm_single_bypass, > > > > > > > > > > > > > > > > > > > > > > > > + }, { > > > > > > > > > > > > + .compatible =3D "allwinner,sun50i-h6-pwm"= , > > > > > > > > > > > > + .data =3D &sun50i_pwm_dual_bypass_clk_rst= , > > > > > > > > > > > > > > > > > > > > > > If you follow my suggestion for the two previous patc= hes, > > (i.e. use devm_clk_get_optional instead of using devm_clk_get iff the > compatible is allwinner,sun50i-h6-pwm; analogous for the reset > controller.) > > > > > > > > > > > > you can just use: > > > > > > > > > > > > > > > > > > > > > > compatible =3D "allwinner,sun50i-h6-pwm", "allwin= ner,sun5i-a10s-pwm"; > > > > > > > > > > > > > > > > > > > > > > and drop this patch. > > > > > > > > > > > > > > > > > > > > Maxime found out that it's not compatible with A10s due= to difference > > > > > > > > > > in bypass bit, but yes, I know what you mean. > > > > > > > > > > > > > > > > > > > > Since H6 requires reset line and bus clock to be specif= ied, it's not > > > > > > > > > > compatible from DT binding side. New yaml based binding= must somehow > > > > > > > > > > know that in order to be able to validate DT node, so i= t needs > > > > > > > > > > standalone compatible. However, depending on conclusion= s of other > > > > > > > > > > discussions, this new compatible can be associated with= already > > > > > > > > > > available quirks structure or have it's own. > > > > > > > > > > > > > > > > > > I cannot follow. You should be able to specify in the bin= ding that the > > > > > > > > > reset line and bus clock is optional. Then allwinner,sun5= 0i-h6-pwm > > > > > > > > > without a reset line and bus clock also verifies, but thi= s doesn't > > > > > > > > > really hurt (and who knows, maybe the next allwinner chip= needs exactly this). > > > > > > > > > > > > > > > > It is not optional. It will not work if either the clocks o= r reset controls > > > > > > > > are missing. How would these be optional anyway? Either it'= s connected and > > > > > > > > thus required, or it's not and therefore should be omitted = from the description. > > > > > > > > > > > > > > [Just arguing about the clock here, the argumentation is anal= ogous for > > > > > > > the reset control.] > > > > > > > > > > > > > > From the driver's perspective it's optional: There are device= s with and > > > > > > > without a bus clock. This doesn't mean that you can just igno= re this > > > > > > > clock if it's specified. It's optional in the sense "If dt do= esn't > > > > > > > specify it, then assume this is a device that doesn't have it= and so you > > > > > > > don't need to handle it." but not in the sense "it doesn't ma= tter if > > > > > > > you handle it or not.". > > > > > > > > > > > > > > Other than that I'm on your side. So for example I think it's= not > > > > > > > optimal that gpiod_get_optional returns NULL if GPIOLIB=3Dn o= r that > > > > > > > devm_reset_control_get_optional returns NULL if RESET_CONTROL= LER=3Dn > > > > > > > because this hides exactly the kind of problem you point out = here. > > > > > > > > > > > > I think there's misunderstanding. I only argued that we can't u= se > > > > > > > > > > > > compatible =3D "allwinner,sun50i-h6-pwm", "allwinner,sun5i-a10s= -pwm"; > > > > > > > > > > > > as you suggested and only > > > > > > > > > > > > compatible =3D "allwinner,sun50i-h6-pwm"; > > > > > > > > > > > > will work. Not because of driver itself (it can still use _opti= onal() > > > > > > variants), but because of DT binding, which should be able to v= alidate H6 > > > > > > PWM node - reset and bus clock references are required in this = case. > > > > > > > > > > I think I understood. In my eyes there is no need to let validati= on of > > > > > the DT bindings catch a missing "optional" property that is neede= d on > > > > > H6. > > > > > > > > > > You have to draw the line somewhere which information the driver = has > > > > > hard-coded and what is only provided by the device tree and just = assumed > > > > > to be correct by the driver. You argue the driver should know tha= t > > > > > > > > No, in this thread I argue that DT validation tool, executed by > > > > > > > > make ARCH=3Darm64 dtbs_check > > > > > > > > should catch that. This is not a driver, but DT binding described i= n YAML. > > > > > > The argumentation is the same. dtbs_check doesn't notice if the base > > > address of your "allwinner,sun50i-h6-pwm" device is wrong. So why sho= uld > > > it catch a missing reset controller phandle? > > > > Of course checking actual values of node properties doesn't make sense = in > > dtbs_check, otherwise we would have million DT bindings. If you have 10= copies > > of the same IP core, of course you would use same compatible, but actua= l > > register ranges, interrupts, etc. would be different in DT nodes. > > > > At this point I would make same argument as were made before, but there= is no > > point going in circles. I'm interested what have DT maintainers to say. > > -- > Pengutronix e.K. | Uwe Kleine-K=C3=B6nig = | > Industrial Linux Solutions | http://www.pengutronix.de/ = | > > -- > You received this message because you are subscribed to the Google Groups= "linux-sunxi" group. > To unsubscribe from this group and stop receiving emails from it, send an= email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org > To view this discussion on the web, visit https://groups.google.com/d/msg= id/linux-sunxi/20190730080900.hhxrqun7vk4nsj2h%40pengutronix.de. --=20 You received this message because you are subscribed to the Google Groups "= linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an e= mail to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org To view this discussion on the web, visit https://groups.google.com/d/msgid= /linux-sunxi/CAGb2v65jFdFZGLti4_B%3D2QPbtrj1b8wh63R5G3NpY_ndpJoV5g%40mail.g= mail.com.