Re: [PATCH] fsl-ifc: set extended addressing for systems whose bootloader doesn't

[Scott Wood]

On Mon, Oct 10, 2016 at 01:56:31PM -0700, David Singleton wrote:
> For 32bit systems whose bootloader doesn't set the extended 36-bit
> addressing register for flash devices above the 4GB boundary
> we can set up in the driver.
> 
> This patch checks the number of address-cells in the dts file
> for the fsl-ifc flash controller.  If #address-cells is 2 then
> it's a 36-bit address mapping,

Not necessarily.

> so set the extended address register
> in the ccsr for the upper 0xf address, as specified in the dts file.

The changelog should not mention 0xf as that gives the impression that the
patch assumes the upper bits are 0xf.

> The code only sets the extended addressing register if the
> dts defines 36-bit addressing for the flash devices AND
> the register was not set by the boot loader.
> 
> If the bootloader has set the extended addressing register
> the code does not update the register.

This is not accurate if the bootloader has set the extended addressing
register to zero.

> 
> Cc: xe-kernel@xxxxxxxxxxxxxxxxxx
> Cc: scottwood@xxxxxxxxxxxxx
> Signed-off-by: David Singleton <davsingl@xxxxxxxxx>
> ---
>  drivers/memory/fsl_ifc.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)

Who are you asking to apply this?  If it's me, please CC
linuxppc-dev@xxxxxxxxxxxxxxxx (and CC me at oss@xxxxxxxxxxxx, not
scottwood@xxxxxxxxxxxxx).  If you want it to go via the mtd tree, CC
the relevant maintainer and list (and me).

> diff --git a/drivers/memory/fsl_ifc.c b/drivers/memory/fsl_ifc.c
> index 1b182b1..3d7a52e 100644
> --- a/drivers/memory/fsl_ifc.c
> +++ b/drivers/memory/fsl_ifc.c
> @@ -78,6 +78,41 @@ EXPORT_SYMBOL(fsl_ifc_find);
>  static int fsl_ifc_ctrl_init(struct fsl_ifc_ctrl *ctrl)
>  {
>  	struct fsl_ifc_global __iomem *ifc = ctrl->gregs;
> +	struct device_node *np;
> +
> +	/*
> +	 * enable extended 36-bit addressing
> +	 * 24.3.2 Extended Chip Select Property registers (IFC_CSPRn_EXT)
> +	 * The extended chip select property register (CSPRn_EXT) contains
> +	 * the extended base address, that is, the most significant
> +	 * bits (msb) of the base address.
> +	 * Set it here for systems where the bootloader doesn't.
> +	 */

Quoting manual section numbers is meaningless without referencing a specific
document.  This comment does not need to be anything more than:

/*
 * Some bootloaders neglect to set the extended base address,
 * so set it based on the device tree.
 */

> +	np = of_find_compatible_node(NULL, NULL, "fsl,ifc");

The function that calls this has the node already.  Pass it in instead
of searching from scratch.

> +	if (np) {
> +		const u32 *prop;
> +
> +		prop = of_get_property(np, "#address-cells", NULL);
> +		if (prop) {
> +			u32 cells;
> +			/*
> +			 * #address-cells 2 means 36-bit addresses are used
> +			 * and the if cspr_ext register is zero, the
> +			 * bootloader didn't set it, we'll set it manually
> +			 */
> +			cells = of_n_addr_cells(np);
> +			if ((cells == 2) && !(ifc_in32(&ifc->cspr_cs[0].cspr_ext))) {

Unnecessary parens.

Why are you only fixing CS0?

> +				prop = of_get_property(np, "reg", NULL);

This is the reg property of the controller, not the node that corresponds to
CS0.  You cannot assume that CCSR and CS0 have the same upper address bits.

> +				if (prop) {
> +					u32 extaddr;
> +
> +					extaddr = *prop; /* get the top nibble for 36-bit */

cspr_ext is 8 bits on some chips (40-bit addressing).  Just remove the
comment.

> +					pr_info("fsl-ifc extended 36-bit addressing\n");

If you must print something here it should indicate that a workaround is
being applied.

-Scott