Date:	Tue, 14 Jun 2016 21:00:00 +0000
From:	Trent Piepho <tpiepho@...etacorp.com>
To:	atull <atull@...nsource.altera.com>
CC:	Rob Herring <robh+dt@...nel.org>,
	"pantelis.antoniou@...sulko.com" <pantelis.antoniou@...sulko.com>,
	Moritz Fischer <moritz.fischer@...us.com>,
	Josh Cartwright <joshc@...com>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"monstr@...str.eu" <monstr@...str.eu>,
	"michal.simek@...inx.com" <michal.simek@...inx.com>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	"Jonathan Corbet" <corbet@....net>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"delicious.quinoa@...il.com" <delicious.quinoa@...il.com>,
	"dinguyen@...nsource.altera.com" <dinguyen@...nsource.altera.com>,
	Matthew Gerlach <mgerlach@...era.com>
Subject: Re: [PATCH v16 6/6] ARM: socfpga: fpga bridge driver support

On Mon, 2016-06-13 at 14:35 -0500, atull wrote:
> > > +
> > > +	/* Allow bridge to be visible to L3 masters or not */
> > > +	if (priv->remap_mask) {
> > > +		priv->l3_remap_value |= ALT_L3_REMAP_MPUZERO_MSK;
> > 
> > Doesn't seem like this belongs here.  I realize the write-only register
> > is a problem.  Maybe the syscon driver should be initializing this
> > value?
> > 
> > > +
> > > +		if (enable)
> > > +			priv->l3_remap_value |= priv->remap_mask;
> > > +		else
> > > +			priv->l3_remap_value &= ~priv->remap_mask;
> > > +
> > > +		ret = regmap_write(priv->l3reg, ALT_L3_REMAP_OFST,
> > > +				   priv->l3_remap_value);
> > 
> > This isn't going work if more than one bridge is used.  Each bridge has
> > its own priv and thus priv->l3_remap_value.  Each bridge's priv will
> > have just the bit for it's own remap set.  The 2nd bridge to be enabled
> > will turn off the 1st bridge when it re-write the l3 register.
> > 
> > If all the bridges shared a static global to cache the reg, then this
> > problem would be a replaced by a race, since nothing would be managing
> > concurrent access to that global from the independent bridge devices.
> > 
> > How about using the already existing regmap cache ability take care of
> > this?  Use regmap_update_bits() to update just the desired bit and let
> > remap take care of keeping track caching the register and protecting
> > access from multiple users.  It should support that and it should
> > support write-only registers, with the creator of the regmap (the syscon
> > driver in this case) supplying the initial value of the write-only reg.
> > Which is where ALT_L3_REMAP_MPUZERO_MSK could go in.
> 
> Please correct me if I'm wrong, but I think that regmap supports
> the features you are talking about, but not syscon.

From my testing, it will work ok if the syscon driver were to set
syscon_config.cache_type one of the caches.  Since the l3 regs read back
as 0, rather than not being readable at all, making them write-only and
giving a default isn't strictly necessary.

It wouldn't be hard to add a write-only property to syscon.

> One simple solution would be to take l3_remap_value out of the priv
> and let it be shared by all h2f bridges.  That involves the least
> amount of change.

You'll need a spin-lock to protect against concurrent access.


> > > +
> > > +static int alt_hps2fpga_enable_set(struct fpga_bridge *bridge, bool enable)
> > > +{
> > > +	return _alt_hps2fpga_enable_set(bridge->priv, enable);
> > > +}
> > > +
> > > +static const struct fpga_bridge_ops altera_hps2fpga_br_ops = {
> > > +	.enable_set = alt_hps2fpga_enable_set,
> > > +	.enable_show = alt_hps2fpga_enable_show,
> > > +};
> > > +
> > > +static struct altera_hps2fpga_data hps2fpga_data  = {
> > > +	.name = HPS2FPGA_BRIDGE_NAME,
> > > +	.remap_mask = ALT_L3_REMAP_H2F_MSK,
> > > +};
> > 
> > Each of these data structs also includes space for all the private data
> > field of the drivers' state.  Seems a bit inefficient if only two of
> > them are configuration data.  It also means only one device of each type
> > can exists.  If one creates two bridges of the same type they'll
> > (silently) share a priv data struct and randomly break.  And the config
> > data structs can't be const.
> 
> Our hardware doesn't contain two devices of any of these three types.

Not yet, but doesn't Arria10 have three FPGA2SDRAM bridges?  But really,
it's just that I think having each device allocated state data for just
that device is more in line with the Linux device model than shared
static state pre-allocated for all devices of the same type to share.
 
> > What if these structs were a different altera_hps_config struct, which
> > the private data struct could then copy or point to?
> > 
> > struct altera_hpsbridge_config {
> > 	const char *name;
> > 	uint32_t remap_mask;
> > };
> > 
> > struct altera_hpsbridge_data {
> > 	const struct altera_hpsbridge_config *config;
> > 	...;
> > 	struct clk *clk;
> > };
> 
> Yes, that sounds good and sensible.  I'll do that in v18.

Another thought, if the "altr,l3regs" binding included not just the
phandle to the l3regs device, but also the bit associated with the
bridge, then there wouldn't need to be bridge specific configuration for
the driver.  The same way the reset and clock properties point not just
to the reset and clock controller, but to the bit in the controller.

> > > +
> > > +static struct altera_hps2fpga_data lwhps2fpga_data  = {
> > > +	.name = LWHPS2FPGA_BRIDGE_NAME,
> > > +	.remap_mask = ALT_L3_REMAP_LWH2F_MSK,
> > > +};
> > > +
> > > +static struct altera_hps2fpga_data fpga2hps_data  = {
> > > +	.name = FPGA2HPS_BRIDGE_NAME,
> > > +};
> > > +
> > > +static const struct of_device_id altera_fpga_of_match[] = {
> > > +	{ .compatible = "altr,socfpga-hps2fpga-bridge",
> > > +	  .data = &hps2fpga_data },
> > > +	{ .compatible = "altr,socfpga-lwhps2fpga-bridge",
> > > +	  .data = &lwhps2fpga_data },
> > > +	{ .compatible = "altr,socfpga-fpga2hps-bridge",
> > > +	  .data = &fpga2hps_data },
> > > +	{},
> > > +};
> > > +
> > > +static int alt_fpga_bridge_probe(struct platform_device *pdev)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	struct altera_hps2fpga_data *priv;
> > > +	const struct of_device_id *of_id;
> > > +	u32 enable;
> > > +	int ret;
> > > +
> > > +	of_id = of_match_device(altera_fpga_of_match, dev);
> > > +	priv = (struct altera_hps2fpga_data *)of_id->data;
> > > +
> > > +	priv->bridge_reset = devm_reset_control_get(dev, priv->name);
> > > +	if (IS_ERR(priv->bridge_reset)) {
> > > +		dev_err(dev, "Could not get %s reset control\n", priv->name);
> > > +		return PTR_ERR(priv->bridge_reset);
> > > +	}
> > > +
> > 
> > 
> > > +	priv->l3reg = syscon_regmap_lookup_by_compatible("altr,l3regs");
> > > +	if (IS_ERR(priv->l3reg)) {
> > > +		dev_err(dev, "regmap for altr,l3regs lookup failed\n");
> > > +		return PTR_ERR(priv->l3reg);
> > > +	}
> > 
> > Perhaps this could be wrapped in if(priv->remap_mask) { }.  The fpga2hps
> > bridge has no bits in the l3 remap register, so why should it need a
> > phandle to the l3 syscon?  This also prevents this driver from working
> > on Arria10, since it has no l3remap register at all, for any of the
> > bridges, so there's nothing for the phandle to point to.
> 
> Agreed.
> 
> > 
> > > +
> > > +	priv->clk = of_clk_get(dev->of_node, 0);
> > > +	if (IS_ERR(priv->clk)) {
> > > +		dev_err(dev, "no clock specified\n");
> > > +		return PTR_ERR(priv->clk);
> > > +	}
> > 
> > devm_clk_get(dev, NULL); should get the 1st clock in the OF node, but
> > use the dev resource manager, so it doesn't need to be put.
> 
> Yes
> 
> > 
> > > +
> > > +	ret = clk_prepare_enable(priv->clk);
> > > +	if (ret) {
> > > +		dev_err(dev, "could not enable clock\n");
> > > +		return -EBUSY;
> > 
> > clk_put() on clk missing here and also the other error returns.
> 
> I'll use devm_clk_get() so that won't be needed.
> 
> > 
> > > +	}
> > > +
> > > +	ret = fpga_bridge_register(dev, priv->name, &altera_hps2fpga_br_ops,
> > > +				   priv);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	if (!of_property_read_u32(dev->of_node, "bridge-enable", &enable)) {
> > > +		if (enable > 1) {
> > > +			dev_warn(dev, "invalid bridge-enable %u > 1\n", enable);
> > > +		} else {
> > > +			dev_info(dev, "%s bridge\n",
> > > +				 (enable ? "enabling" : "disabling"));
> > > +
> > > +			ret = _alt_hps2fpga_enable_set(priv, enable);
> > 
> > Should this go through the bridge api, e.g. fpga_bridge_enable()?  Since
> > the bridge has already been registered.  Or is the bridge framework
> > supposed to be able to support bridges that might be enabled or disabled
> > behind its back?  If so, then isn't there a race here with
> > _alt_hps2fpga_enable_set() possible being called at the same time as
> > other operations on this bridge triggered by the code in fpga-bridge.c?
> > 
> > Alternatively, could the bridge be enabled or disabled before being
> > registered?
> 
> I'll do the enabling/disabling before registering the driver.
> 
> Thanks for your code review!
> 
> Alan

Powered by blists - more mailing lists