Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM

[Paul Walmsley]

On Wed, 16 Jan 2019, Uwe Kleine-König wrote:

> On Wed, Jan 16, 2019 at 09:18:45AM -0800, Paul Walmsley wrote:
> > On Wed, 16 Jan 2019, Uwe Kleine-König wrote:
> > 
> > > On Wed, Jan 16, 2019 at 04:40:42PM +0530, Yash Shah wrote:
> > > > On Wed, Jan 16, 2019 at 3:30 AM Uwe Kleine-König
> > > > <u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
> > > > > On Fri, Jan 11, 2019 at 01:52:44PM +0530, Yash Shah wrote:
> > > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > > > > index a8f47df..3bcaf6a 100644
> > > > > > --- a/drivers/pwm/Kconfig
> > > > > > +++ b/drivers/pwm/Kconfig
> > > > > > @@ -380,6 +380,16 @@ config PWM_SAMSUNG
> > > > > >         To compile this driver as a module, choose M here: the module
> > > > > >         will be called pwm-samsung.
> > > > > >
> > > > > > +config PWM_SIFIVE
> > > > > > +     tristate "SiFive PWM support"
> > > > > > +     depends on OF
> > > > > > +     depends on COMMON_CLK
> > > > >
> > > > > I'd say add:
> > > > >
> > > > >         depends on MACH_SIFIVE || COMPILE_TEST
> > > > >
> > > > > (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.)
> > > > 
> > > > As of now, MACH_SIFIVE/ARCH_SIFIVE isn't available.
> > > > @Paul, Do you have any comments on this?
> > > 
> > > If this is not going to be available at least protect it by
> > > 
> > > 	depends RISCV || COMPILE_TEST
> > 
> > There's nothing RISC-V or SiFive SoC-specific about this driver or IP 
> > block.  The HDL for this IP block is open-source and posted on Github.  
> > The IP block and driver would work unchanged on an ARM or MIPS SoC, and in 
> > fact, SiFive does design ARM-based SoCs as well.  Likewise, any other SoC 
> > vendor could take the HDL for this IP block from the git tree and 
> > implement it on their own SoC.
> > 
> > More generally: it's a basic principle of Linux device drivers that they 
> > should be buildable for any architecture.  The idea here is to prevent 
> > developers from burying architecture or SoC-specific hacks into the 
> > driver.  So there shouldn't be any architecture or SoC-specific code in 
> > any device driver, unless it's abstracted in some way - ideally through a 
> > common framework.
> > 
> > So from this point of view, neither "depends MACH_SIFIVE" nor "depends 
> > RISCV" would be appropriate.  Similarly, the equivalents for other 
> > architectures (e.g. "ARCH_ARM") or SoC manufacturers (e.g., 
> > "MACH_SAMSUNG") wouldn't be appropriate for any generic IP block device 
> > driver like this one.
> 
> The dependency serves two purposes:
> 
>  a) ensure that the build requirements are fulfilled.
>  b) restrict configuration to actually existing machines
> 
> When considering b) it doesn't make sense to enable the driver for (say)
> a machine that is powered by an ARM SoC by TI. If you still want to
> compile test the sifive driver for ARM, enable COMPILE_TEST. That's what
> this symbol is there for.

COMPILE_TEST made slightly more sense before the broad availability of 
open-source soft cores, SoC integration, and IP; and before powerful, 
inexpensive FPGAs and SoCs with FPGA fabrics were common.

Even back then, COMPILE_TEST was starting to look questionable.  IP blocks 
from CPU- and SoC vendor-independent libraries, like DesignWare and the 
Cadence IP libraries, were integrated on SoCs across varying CPU 
architectures.  (Fortunately, looking at the tree, subsystem maintainers 
with DesignWare drivers seem to have largely avoided adding architecture 
or SoC-specific Kconfig restrictions there - and thus have also avoided 
the need for COMPILE_TEST.)  If an unnecessary architecture Kconfig 
dependency was added for a leaf driver, that Kconfig line would either 
need to be patched out by hand, or if present, COMPILE_TEST would need to 
be enabled.

This was less of a problem then.  There were very few FPGA Linux users, 
and they were mostly working on internal proprietary projects. FPGAs that 
could run Linux at a reasonable speed, including MMUs and FPUs, were 
expensive or were missing good tool support.  So most FPGA Linux 
development was restricted to ASIC vendors - the Samsungs, Qualcomms, 
NVIDIAs of the world - for prototyping, and those FPGA platforms never 
made it outside those companies.

The situation is different now.  The major FPGA vendors have inexpensive 
FPGA SoCs with full-featured CPU hard macros.  The development boards can 
be quite affordable (< USD 300 for the Xilinx Ultra96).  There are also 
now open-source SoC HDL implementations - including MMUs, FPUs, and 
peripherals like PWM and UART - for the conventional FPGA market.  These 
can run at mid-1990's clock rates - slow by modern standards but still 
quite usable.

So or vendor-specific IP blocks that are unlikely to ever be reused by 
anyone else, like the NVIDIA or TI PWM blocks, maybe there's still some 
justification for COMPILE_TEST.  But for drivers for open-source, 
SoC-independent peripheral IP blocks - or even for proprietary IP blocks 
from library vendors like Synopsys or Cadence - like this PWM driver, we 
shouldn't add unnecessary architecture, SoC vendor, or COMPILE_TEST 
Kconfig dependencies.  They will just force users to patch the kernel or 
enable COMPILE_TEST for kernels that are actually meant to run on real 
hardware.


- Paul