[PATCH v1 0/4] Convert m68k MVME147 WD33C93 SCSI driver to DMA API

[PATCH v1 0/4] Convert m68k MVME147 WD33C93 SCSI driver to DMA API

[Michael Schmitz]

This series was precipitated by Arnd removing CONFIG_VIRT_TO_BUS. The
m68k WD33C93 still used virt_to_bus to convert virtual addresses to
physical addresses suitable for the DMA engines (note m68k does not
have an IOMMU and uses a direct mapping for DMA addresses). 

Arnd suggested to use dma_map_single() to set up dma mappings instead
of open-coding much the same in every driver dma_setup() function. An
earlier patch series has converted the three Amiga WD33C93 drivers,
this series now takes care of the sole remaining m68k WD33C93 driver.

The m68k VME mvme147 driver has no DMA addressing or alignment
restrictions and can be converted in the same way as the Amiga a3000
one in my previous series, but requires conversion to a platform
device driver first.

Only compile tested so far, and hardware testing might be hard to do.
I'd appreciate someone giving this a thorough review.

The first two patches shoukd be taken through Geert's tree before the other
two can be applied by the SCSI maintainers. Please advise should you prefer
this to be split into two separate series.

Cheers,

   Michael

[PATCH v1 1/4] m68k - add MVME147 SCSI base address to mvme147hw.h

[Michael Schmitz]

The base address for the WD33C93 SCSI host adapter on mvme147
boards is missing from mvme147hw.h. This information will be
needed for platform device conversion of the mvme147_scsi
driver, so add it here.

CC: linux-scsi@vger.kernel.org
Link: https://lore.kernel.org/r/6d1d88ee-1cf6-c735-1e6d-bafd2096e322@gmail.com
Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
---
 arch/m68k/include/asm/mvme147hw.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/m68k/include/asm/mvme147hw.h b/arch/m68k/include/asm/mvme147hw.h
index e28eb1c0e0bf..fd8c1e4fc7be 100644
--- a/arch/m68k/include/asm/mvme147hw.h
+++ b/arch/m68k/include/asm/mvme147hw.h
@@ -93,6 +93,7 @@ struct pcc_regs {
 #define M147_SCC_B_ADDR		0xfffe3000
 #define M147_SCC_PCLK		5000000
 
+#define MVME147_SCSI_BASE	0xfffe4000
 #define MVME147_IRQ_SCSI_PORT	(IRQ_USER+0x45)
 #define MVME147_IRQ_SCSI_DMA	(IRQ_USER+0x46)
 
-- 
2.17.1

[PATCH v1 2/4] m68k - set up platform device for mvme147_scsi

[Michael Schmitz]

Set up a platform device for the mvme147_scsi driver. The
platform device is required for conversion of the driver to
the DMA API.

CC: linux-scsi@vger.kernel.org
Link: https://lore.kernel.org/r/6d1d88ee-1cf6-c735-1e6d-bafd2096e322@gmail.com
Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
---
 arch/m68k/mvme147/config.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/arch/m68k/mvme147/config.c b/arch/m68k/mvme147/config.c
index 4e6218115f43..c6e7dfe3eb54 100644
--- a/arch/m68k/mvme147/config.c
+++ b/arch/m68k/mvme147/config.c
@@ -21,6 +21,7 @@
 #include <linux/console.h>
 #include <linux/linkage.h>
 #include <linux/init.h>
+#include <linux/platform_device.h>
 #include <linux/major.h>
 #include <linux/rtc.h>
 #include <linux/interrupt.h>
@@ -188,3 +189,23 @@ int mvme147_hwclk(int op, struct rtc_time *t)
 	}
 	return 0;
 }
+
+static const struct resource mvme147_scsi_rsrc[] __initconst = {
+	DEFINE_RES_MEM(MVME147_SCSI_BASE, 0xff),
+	DEFINE_RES_IRQ(MVME147_IRQ_SCSI_PORT),
+};
+
+int __init mvme147_platform_init(void)
+{
+	struct platform_device *pdev;
+	int rv = 0;
+
+	pdev = platform_device_register_simple("mvme147-scsi", -1,
+		mvme147_scsi_rsrc, ARRAY_SIZE(mvme147_scsi_rsrc));
+	if (IS_ERR(pdev))
+		rv = PTR_ERR(pdev);
+
+	return rv;
+}
+
+arch_initcall(mvme147_platform_init);
-- 
2.17.1

[PATCH v1 3/4] scsi - convert mvme146_scsi.c to platform device

[Michael Schmitz]

Convert the mvme147_scsi driver to a platform device driver.
This is required for conversion of the driver to the DMA API.

CC: linux-scsi@vger.kernel.org
Link: https://lore.kernel.org/r/6d1d88ee-1cf6-c735-1e6d-bafd2096e322@gmail.com
Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
---
 drivers/scsi/mvme147.c | 86 ++++++++++++++++++++++++++++--------------
 1 file changed, 58 insertions(+), 28 deletions(-)

diff --git a/drivers/scsi/mvme147.c b/drivers/scsi/mvme147.c
index 472fa043094f..aa6476c3e70c 100644
--- a/drivers/scsi/mvme147.c
+++ b/drivers/scsi/mvme147.c
@@ -3,6 +3,7 @@
 #include <linux/mm.h>
 #include <linux/blkdev.h>
 #include <linux/interrupt.h>
+#include <linux/platform_device.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -85,40 +86,50 @@ static struct scsi_host_template mvme147_host_template = {
 	.cmd_size		= sizeof(struct scsi_pointer),
 };
 
-static struct Scsi_Host *mvme147_shost;
-
-static int __init mvme147_init(void)
+static int __init mvme147_scsi_probe(struct platform_device *pdev)
 {
+	struct resource *mres, *ires;
+	struct Scsi_Host *mvme147_inst;
 	wd33c93_regs regs;
 	struct WD33C93_hostdata *hdata;
 	int error = -ENOMEM;
 
-	if (!MACH_IS_MVME147)
-		return 0;
+	mres = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!mres)
+		return -ENODEV;
+
+	ires = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (!ires)
+		return -ENODEV;
+
+	if (!request_mem_region(mres->start, resource_size(mres), "wd33c93"))
+		return -EBUSY;
 
-	mvme147_shost = scsi_host_alloc(&mvme147_host_template,
+	mvme147_inst = scsi_host_alloc(&mvme147_host_template,
 			sizeof(struct WD33C93_hostdata));
-	if (!mvme147_shost)
+	if (!mvme147_inst)
 		goto err_out;
-	mvme147_shost->base = 0xfffe4000;
-	mvme147_shost->irq = MVME147_IRQ_SCSI_PORT;
 
-	regs.SASR = (volatile unsigned char *)0xfffe4000;
-	regs.SCMD = (volatile unsigned char *)0xfffe4001;
 
-	hdata = shost_priv(mvme147_shost);
+	mvme147_inst->base = mres->start;
+	mvme147_inst->irq = ires->start;
+
+	regs.SASR = (volatile unsigned char *)mres->start;
+	regs.SCMD = (volatile unsigned char *)(mres->start)+0x1;
+
+	hdata = shost_priv(mvme147_inst);
 	hdata->no_sync = 0xff;
 	hdata->fast = 0;
 	hdata->dma_mode = CTRL_DMA;
 
-	wd33c93_init(mvme147_shost, regs, dma_setup, dma_stop, WD33C93_FS_8_10);
+	wd33c93_init(mvme147_inst, regs, dma_setup, dma_stop, WD33C93_FS_8_10);
 
-	error = request_irq(MVME147_IRQ_SCSI_PORT, mvme147_intr, 0,
-			"MVME147 SCSI PORT", mvme147_shost);
+	error = request_irq(ires->start, mvme147_intr, 0,
+			"MVME147 SCSI PORT", mvme147_inst);
 	if (error)
 		goto err_unregister;
-	error = request_irq(MVME147_IRQ_SCSI_DMA, mvme147_intr, 0,
-			"MVME147 SCSI DMA", mvme147_shost);
+	error = request_irq(ires->start+1, mvme147_intr, 0,
+			"MVME147 SCSI DMA", mvme147_inst);
 	if (error)
 		goto err_free_irq;
 #if 0	/* Disabled; causes problems booting */
@@ -133,30 +144,49 @@ static int __init mvme147_init(void)
 	m147_pcc->dma_cntrl = 0x00;	/* ensure DMA is stopped */
 	m147_pcc->dma_intr = 0x89;	/* Ack and enable ints */
 
-	error = scsi_add_host(mvme147_shost, NULL);
+	error = scsi_add_host(mvme147_inst, &pdev->dev);
 	if (error)
 		goto err_free_irq;
-	scsi_scan_host(mvme147_shost);
+
+	platform_set_drvdata(pdev, mvme147_inst);
+
+	scsi_scan_host(mvme147_inst);
 	return 0;
 
 err_free_irq:
-	free_irq(MVME147_IRQ_SCSI_PORT, mvme147_shost);
+	free_irq(ires->start, mvme147_inst);
 err_unregister:
-	scsi_host_put(mvme147_shost);
+	scsi_host_put(mvme147_inst);
 err_out:
 	return error;
 }
 
-static void __exit mvme147_exit(void)
+static int __exit mvme147_scsi_remove(struct platform_device *pdev)
 {
-	scsi_remove_host(mvme147_shost);
+	struct Scsi_Host *mvme147_inst = platform_get_drvdata(pdev);
+	struct resource *mres = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	struct resource *ires = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+
+	scsi_remove_host(mvme147_inst);
 
 	/* XXX Make sure DMA is stopped! */
-	free_irq(MVME147_IRQ_SCSI_PORT, mvme147_shost);
-	free_irq(MVME147_IRQ_SCSI_DMA, mvme147_shost);
+	free_irq(ires->start, mvme147_inst);
+	free_irq(ires->start+1, mvme147_inst);
 
-	scsi_host_put(mvme147_shost);
+	scsi_host_put(mvme147_inst);
+	release_mem_region(mres->start, resource_size(mres));
+	return 0;
 }
 
-module_init(mvme147_init);
-module_exit(mvme147_exit);
+static struct platform_driver mvme147_scsi_driver = {
+	.remove = __exit_p(mvme147_scsi_remove),
+	.driver   = {
+		.name	= "mvme147-scsi",
+	},
+};
+
+module_platform_driver_probe(mvme147_scsi_driver, mvme147_scsi_probe);
+
+MODULE_DESCRIPTION("MVME147 built-in SCSI");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:mvme147-scsi");
-- 
2.17.1

[PATCH v1 4/4] scsi - convert mvme147_scsi driver to DMA API

[Michael Schmitz]

Convert mvme147_scsi to DMA API to eliminate one of the last
usages of virt_to_bus().

CC: linux-scsi@vger.kernel.org
Link: https://lore.kernel.org/r/6d1d88ee-1cf6-c735-1e6d-bafd2096e322@gmail.com
Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
---
 drivers/scsi/mvme147.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/mvme147.c b/drivers/scsi/mvme147.c
index aa6476c3e70c..6ffd15f08629 100644
--- a/drivers/scsi/mvme147.c
+++ b/drivers/scsi/mvme147.c
@@ -21,6 +21,8 @@
 #include "wd33c93.h"
 #include "mvme147.h"
 
+#define DMA_DIR(d)   ((d == DATA_OUT_DIR) ? DMA_TO_DEVICE : DMA_FROM_DEVICE)
+
 static irqreturn_t mvme147_intr(int irq, void *data)
 {
 	struct Scsi_Host *instance = data;
@@ -35,10 +37,20 @@ static irqreturn_t mvme147_intr(int irq, void *data)
 static int dma_setup(struct scsi_cmnd *cmd, int dir_in)
 {
 	struct scsi_pointer *scsi_pointer = WD33C93_scsi_pointer(cmd);
+	unsigned long len = scsi_pointer->this_residual;
 	struct Scsi_Host *instance = cmd->device->host;
 	struct WD33C93_hostdata *hdata = shost_priv(instance);
 	unsigned char flags = 0x01;
-	unsigned long addr = virt_to_bus(scsi_pointer->ptr);
+	dma_addr_t addr;
+
+	addr = dma_map_single(instance->dma_dev, scsi_pointer->ptr,
+			      len, DMA_DIR(dir_in));
+	if (dma_mapping_error(instance->dma_dev, addr)) {
+		dev_warn(instance->dma_dev, "cannot map SCSI data block %p\n",
+			 scsi_pointer->ptr);
+		return 1;
+	}
+	scsi_pointer->dma_handle = addr;
 
 	/* setup dma direction */
 	if (!dir_in)
@@ -47,14 +59,6 @@ static int dma_setup(struct scsi_cmnd *cmd, int dir_in)
 	/* remember direction */
 	hdata->dma_dir = dir_in;
 
-	if (dir_in) {
-		/* invalidate any cache */
-		cache_clear(addr, scsi_pointer->this_residual);
-	} else {
-		/* push any dirty cache */
-		cache_push(addr, scsi_pointer->this_residual);
-	}
-
 	/* start DMA */
 	m147_pcc->dma_bcr = scsi_pointer->this_residual | (1 << 24);
 	m147_pcc->dma_dadr = addr;
@@ -67,7 +71,13 @@ static int dma_setup(struct scsi_cmnd *cmd, int dir_in)
 static void dma_stop(struct Scsi_Host *instance, struct scsi_cmnd *SCpnt,
 		     int status)
 {
+	struct scsi_pointer *scsi_pointer = WD33C93_scsi_pointer(SCpnt);
+	struct WD33C93_hostdata *hdata = shost_priv(instance);
+
 	m147_pcc->dma_cntrl = 0;
+	dma_unmap_single(instance->dma_dev, scsi_pointer->dma_handle,
+			 scsi_pointer->this_residual,
+			 DMA_DIR(hdata->dma_dir));
 }
 
 static struct scsi_host_template mvme147_host_template = {
@@ -94,6 +104,11 @@ static int __init mvme147_scsi_probe(struct platform_device *pdev)
 	struct WD33C93_hostdata *hdata;
 	int error = -ENOMEM;
 
+	if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32))) {
+		dev_warn(&pdev->dev, "cannot use 32 bit DMA\n");
+		return -ENODEV;
+	}
+
 	mres = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!mres)
 		return -ENODEV;
-- 
2.17.1

Re: [PATCH v1 2/4] m68k - set up platform device for mvme147_scsi

[kernel test robot]

Hi Michael,

I love your patch! Perhaps something to improve:

[auto build test WARNING on geert-m68k/for-next]
[also build test WARNING on mkp-scsi/for-next jejb-scsi/for-next linus/master v5.19-rc5 next-20220708]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Michael-Schmitz/Convert-m68k-MVME147-WD33C93-SCSI-driver-to-DMA-API/20220709-081556
base:   https://git.kernel.org/pub/scm/linux/kernel/git/geert/linux-m68k.git for-next
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20220709/202207091359.WPLQuHhB-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/3a5e770e42ebe8984096d0cd1d93a95a1d7b67e7
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Michael-Schmitz/Convert-m68k-MVME147-WD33C93-SCSI-driver-to-DMA-API/20220709-081556
        git checkout 3a5e770e42ebe8984096d0cd1d93a95a1d7b67e7
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash arch/m68k/mvme147/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   arch/m68k/mvme147/config.c:77:13: warning: no previous prototype for 'mvme147_init_IRQ' [-Wmissing-prototypes]
      77 | void __init mvme147_init_IRQ(void)
         |             ^~~~~~~~~~~~~~~~
>> arch/m68k/mvme147/config.c:198:12: warning: no previous prototype for 'mvme147_platform_init' [-Wmissing-prototypes]
     198 | int __init mvme147_platform_init(void)
         |            ^~~~~~~~~~~~~~~~~~~~~


vim +/mvme147_platform_init +198 arch/m68k/mvme147/config.c

   197	
 > 198	int __init mvme147_platform_init(void)
   199	{
   200		struct platform_device *pdev;
   201		int rv = 0;
   202	
   203		pdev = platform_device_register_simple("mvme147-scsi", -1,
   204			mvme147_scsi_rsrc, ARRAY_SIZE(mvme147_scsi_rsrc));
   205		if (IS_ERR(pdev))
   206			rv = PTR_ERR(pdev);
   207	
   208		return rv;
   209	}
   210	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v1 2/4] m68k - set up platform device for mvme147_scsi

[Arnd Bergmann]

On Sat, Jul 9, 2022 at 2:10 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
> +
> +static const struct resource mvme147_scsi_rsrc[] __initconst = {
> +       DEFINE_RES_MEM(MVME147_SCSI_BASE, 0xff),
> +       DEFINE_RES_IRQ(MVME147_IRQ_SCSI_PORT),
> +};

The size should almost certainly be 0x100, not 0xff here.

Otherwise this looks good to me.

       Arnd

Re: [PATCH v1 1/4] m68k - add MVME147 SCSI base address to mvme147hw.h

[Arnd Bergmann]

On Sat, Jul 9, 2022 at 2:10 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
> diff --git a/arch/m68k/include/asm/mvme147hw.h b/arch/m68k/include/asm/mvme147hw.h
> index e28eb1c0e0bf..fd8c1e4fc7be 100644
> --- a/arch/m68k/include/asm/mvme147hw.h
> +++ b/arch/m68k/include/asm/mvme147hw.h
> @@ -93,6 +93,7 @@ struct pcc_regs {
>  #define M147_SCC_B_ADDR                0xfffe3000
>  #define M147_SCC_PCLK          5000000
>
> +#define MVME147_SCSI_BASE      0xfffe4000

I think this should be an 'void *__iomem' token, not a plain integer.
Apparently the driver internally uses a 'volatile void *', but some of
the other front-ends are already converted to use __iomem.

        Arnd

Re: [PATCH v1 3/4] scsi - convert mvme146_scsi.c to platform device

[Arnd Bergmann]

On Sat, Jul 9, 2022 at 2:10 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
>
> Convert the mvme147_scsi driver to a platform device driver.
> This is required for conversion of the driver to the DMA API.
>
> CC: linux-scsi@vger.kernel.org
> Link: https://lore.kernel.org/r/6d1d88ee-1cf6-c735-1e6d-bafd2096e322@gmail.com
> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>

The patch looks correct to me, but the type cast for the address doesn't
seem right:

> -       regs.SASR = (volatile unsigned char *)0xfffe4000;
> -       regs.SCMD = (volatile unsigned char *)0xfffe4001;
>
> -       hdata = shost_priv(mvme147_shost);
> +       mvme147_inst->base = mres->start;
> +       mvme147_inst->irq = ires->start;
> +
> +       regs.SASR = (volatile unsigned char *)mres->start;
> +       regs.SCMD = (volatile unsigned char *)(mres->start)+0x1;

A resource would pass a phys_addr_t token, but the driver expects a
virtual address that should be an __iomem pointer. The MMIO area
already gets mapped into virtual addresses in arch/m68k/kernel/head.S,
so it makes sense to skip the extra ioremap() and just use the address,
but then you can't pass it as an IORESOUCRE_MEM token and should
use platform_data with the pointer instead.

The alternative is to do it the normal way and pass the physical address
as a resource, that you can pass into devm_platform_ioremap_resource()
or a similar helper.

       Arnd

Re: [PATCH v1 2/4] m68k - set up platform device for mvme147_scsi

[Michael Schmitz]

Hi Arnd,

thanks for your review!

Am 11.07.2022 um 03:56 schrieb Arnd Bergmann:
> On Sat, Jul 9, 2022 at 2:10 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
>> +
>> +static const struct resource mvme147_scsi_rsrc[] __initconst = {
>> +       DEFINE_RES_MEM(MVME147_SCSI_BASE, 0xff),
>> +       DEFINE_RES_IRQ(MVME147_IRQ_SCSI_PORT),
>> +};
>
> The size should almost certainly be 0x100, not 0xff here.

OK, will fix that. The resource size is taken from one of the Amiga 
drivers and might be excessive for mvme147, but 0xff is certainly odd.

Cheers,

	Michael


> Otherwise this looks good to me.
>
>        Arnd
>

Re: [PATCH v1 1/4] m68k - add MVME147 SCSI base address to mvme147hw.h

[Michael Schmitz]

Hi Arns,

Am 11.07.2022 um 04:06 schrieb Arnd Bergmann:
> On Sat, Jul 9, 2022 at 2:10 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
>> diff --git a/arch/m68k/include/asm/mvme147hw.h b/arch/m68k/include/asm/mvme147hw.h
>> index e28eb1c0e0bf..fd8c1e4fc7be 100644
>> --- a/arch/m68k/include/asm/mvme147hw.h
>> +++ b/arch/m68k/include/asm/mvme147hw.h
>> @@ -93,6 +93,7 @@ struct pcc_regs {
>>  #define M147_SCC_B_ADDR                0xfffe3000
>>  #define M147_SCC_PCLK          5000000
>>
>> +#define MVME147_SCSI_BASE      0xfffe4000
>
> I think this should be an 'void *__iomem' token, not a plain integer.
> Apparently the driver internally uses a 'volatile void *', but some of
> the other front-ends are already converted to use __iomem.

I'll pass the base address through a platform data struct in the next 
version to address your other concerns. Haven't seen __iomem types used 
in the other drivers - two are Zorro devices, and two platform devices 
(a3000 and sgiwd93). Found no other wd33c93 drivers...

Cheers,

	Michael


>
>         Arnd
>

Re: [PATCH v1 3/4] scsi - convert mvme146_scsi.c to platform device

[Geert Uytterhoeven]

On Sun, Jul 10, 2022 at 6:12 PM Arnd Bergmann <arnd@kernel.org> wrote:
> On Sat, Jul 9, 2022 at 2:10 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
> > Convert the mvme147_scsi driver to a platform device driver.
> > This is required for conversion of the driver to the DMA API.
> >
> > CC: linux-scsi@vger.kernel.org
> > Link: https://lore.kernel.org/r/6d1d88ee-1cf6-c735-1e6d-bafd2096e322@gmail.com
> > Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
>
> The patch looks correct to me, but the type cast for the address doesn't
> seem right:
>
> > -       regs.SASR = (volatile unsigned char *)0xfffe4000;
> > -       regs.SCMD = (volatile unsigned char *)0xfffe4001;
> >
> > -       hdata = shost_priv(mvme147_shost);
> > +       mvme147_inst->base = mres->start;
> > +       mvme147_inst->irq = ires->start;
> > +
> > +       regs.SASR = (volatile unsigned char *)mres->start;
> > +       regs.SCMD = (volatile unsigned char *)(mres->start)+0x1;
>
> A resource would pass a phys_addr_t token, but the driver expects a
> virtual address that should be an __iomem pointer. The MMIO area
> already gets mapped into virtual addresses in arch/m68k/kernel/head.S,
> so it makes sense to skip the extra ioremap() and just use the address,
> but then you can't pass it as an IORESOUCRE_MEM token and should
> use platform_data with the pointer instead.
>
> The alternative is to do it the normal way and pass the physical address
> as a resource, that you can pass into devm_platform_ioremap_resource()
> or a similar helper.

I would prefer the latter.  While head.S sets up the mapping,
__ioremap() does not have support for this on the mvme platform,
so this has to be added first. Look at the amiga and virt platforms
for examples.

If you do want to pass the virtual address as platform data (for now
;-), please provide the physical memory resource too, so we don't
have to go through another synchronization step with the m68k and
scsi trees later.

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v1 3/4] scsi - convert mvme146_scsi.c to platform device

[Michael Schmitz]

Hi Geert,

Am 11.07.2022 um 19:16 schrieb Geert Uytterhoeven:
> On Sun, Jul 10, 2022 at 6:12 PM Arnd Bergmann <arnd@kernel.org> wrote:
>> On Sat, Jul 9, 2022 at 2:10 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
>>> Convert the mvme147_scsi driver to a platform device driver.
>>> This is required for conversion of the driver to the DMA API.
>>>
>>> CC: linux-scsi@vger.kernel.org
>>> Link: https://lore.kernel.org/r/6d1d88ee-1cf6-c735-1e6d-bafd2096e322@gmail.com
>>> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
>>
>> The patch looks correct to me, but the type cast for the address doesn't
>> seem right:
>>
>>> -       regs.SASR = (volatile unsigned char *)0xfffe4000;
>>> -       regs.SCMD = (volatile unsigned char *)0xfffe4001;
>>>
>>> -       hdata = shost_priv(mvme147_shost);
>>> +       mvme147_inst->base = mres->start;
>>> +       mvme147_inst->irq = ires->start;
>>> +
>>> +       regs.SASR = (volatile unsigned char *)mres->start;
>>> +       regs.SCMD = (volatile unsigned char *)(mres->start)+0x1;
>>
>> A resource would pass a phys_addr_t token, but the driver expects a
>> virtual address that should be an __iomem pointer. The MMIO area
>> already gets mapped into virtual addresses in arch/m68k/kernel/head.S,
>> so it makes sense to skip the extra ioremap() and just use the address,
>> but then you can't pass it as an IORESOUCRE_MEM token and should
>> use platform_data with the pointer instead.

OK, got it now (I had missed the physical/virtual mismatch entirely).

>>
>> The alternative is to do it the normal way and pass the physical address
>> as a resource, that you can pass into devm_platform_ioremap_resource()
>> or a similar helper.
>
> I would prefer the latter.  While head.S sets up the mapping,
> __ioremap() does not have support for this on the mvme platform,
> so this has to be added first. Look at the amiga and virt platforms
> for examples.

I see - doesn't look too hard to do, and should not affect any other 
existing code.
Is it worth adding the same support for Atari as well?

> If you do want to pass the virtual address as platform data (for now
> ;-), please provide the physical memory resource too, so we don't
> have to go through another synchronization step with the m68k and
> scsi trees later.

That's what I had intended to do, but I'd rather get this sorted once 
and for all.

Cheers,

	Michael


>
> Thanks!
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
>

Re: [PATCH v1 3/4] scsi - convert mvme146_scsi.c to platform device

[Geert Uytterhoeven]

Hi Michael,

On Mon, Jul 11, 2022 at 9:57 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
> Am 11.07.2022 um 19:16 schrieb Geert Uytterhoeven:
> > On Sun, Jul 10, 2022 at 6:12 PM Arnd Bergmann <arnd@kernel.org> wrote:
> >> A resource would pass a phys_addr_t token, but the driver expects a
> >> virtual address that should be an __iomem pointer. The MMIO area
> >> already gets mapped into virtual addresses in arch/m68k/kernel/head.S,
> >> so it makes sense to skip the extra ioremap() and just use the address,
> >> but then you can't pass it as an IORESOUCRE_MEM token and should
> >> use platform_data with the pointer instead.
>
> OK, got it now (I had missed the physical/virtual mismatch entirely).

And the __iomem is there so we can catch mistakes using sparse
(make C=2 path/to/file/.o).

> >> The alternative is to do it the normal way and pass the physical address
> >> as a resource, that you can pass into devm_platform_ioremap_resource()
> >> or a similar helper.
> >
> > I would prefer the latter.  While head.S sets up the mapping,
> > __ioremap() does not have support for this on the mvme platform,
> > so this has to be added first. Look at the amiga and virt platforms
> > for examples.
>
> I see - doesn't look too hard to do, and should not affect any other
> existing code.
> Is it worth adding the same support for Atari as well?

From a quick glance at arch/m68k/kernel/head.S, it seems that
on Atari there is no identity mapping (the high I/O area is mapped
to the virtual low area).  That means __ioremap() and iounmap()
wouldn't be symmetrical, but it can be done.

Note that on Amiga we only use the identity shortcut for Zorro III
memory (and only for the first half?), i.e. ioremap() on Zorro II I/O
does add new mappings.  Hence most Zorro II drivers use ZTWO_VADDR()
instead of ioremap().

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v1 1/4] m68k - add MVME147 SCSI base address to mvme147hw.h

[Arnd Bergmann]

On Mon, Jul 11, 2022 at 6:16 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
> Am 11.07.2022 um 04:06 schrieb Arnd Bergmann:
> > On Sat, Jul 9, 2022 at 2:10 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
> > I think this should be an 'void *__iomem' token, not a plain integer.
> > Apparently the driver internally uses a 'volatile void *', but some of
> > the other front-ends are already converted to use __iomem.
>
> I'll pass the base address through a platform data struct in the next
> version to address your other concerns. Haven't seen __iomem types used
> in the other drivers - two are Zorro devices, and two platform devices
> (a3000 and sgiwd93). Found no other wd33c93 drivers...

Right, I noticed this as well. The ideal way to do this would be to change
all of these files to use __iomem tokens consistently, and then use
ioread32be()/iowrite32be() in place of the volatile pointer dereference.

Maybe leave that for a follow-up series, you'll probably uncover
more of these issues once you take that step.

       Arnd

Re: [PATCH v1 3/4] scsi - convert mvme146_scsi.c to platform device

[Michael Schmitz]

Hi Geert,

Am 11.07.2022 um 20:27 schrieb Geert Uytterhoeven:
> Hi Michael,
>
> On Mon, Jul 11, 2022 at 9:57 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
>> Am 11.07.2022 um 19:16 schrieb Geert Uytterhoeven:
>>> On Sun, Jul 10, 2022 at 6:12 PM Arnd Bergmann <arnd@kernel.org> wrote:
>>>> A resource would pass a phys_addr_t token, but the driver expects a
>>>> virtual address that should be an __iomem pointer. The MMIO area
>>>> already gets mapped into virtual addresses in arch/m68k/kernel/head.S,
>>>> so it makes sense to skip the extra ioremap() and just use the address,
>>>> but then you can't pass it as an IORESOUCRE_MEM token and should
>>>> use platform_data with the pointer instead.
>>
>> OK, got it now (I had missed the physical/virtual mismatch entirely).
>
> And the __iomem is there so we can catch mistakes using sparse
> (make C=2 path/to/file/.o).

I'll need to make a habit of that ...

>>>> The alternative is to do it the normal way and pass the physical address
>>>> as a resource, that you can pass into devm_platform_ioremap_resource()
>>>> or a similar helper.
>>>
>>> I would prefer the latter.  While head.S sets up the mapping,
>>> __ioremap() does not have support for this on the mvme platform,
>>> so this has to be added first. Look at the amiga and virt platforms
>>> for examples.
>>
>> I see - doesn't look too hard to do, and should not affect any other
>> existing code.
>> Is it worth adding the same support for Atari as well?
>
> From a quick glance at arch/m68k/kernel/head.S, it seems that
> on Atari there is no identity mapping (the high I/O area is mapped
> to the virtual low area).  That means __ioremap() and iounmap()
> wouldn't be symmetrical, but it can be done.

As I read it, it's the other way: virtual 0xffxxxxxx is mapped to phys. 
0x00ffxxxx, and all hardware addresses are given in the upper window 
(Medusa/Hades use that window only, and have identity mappings there).

So returning identity mapping in the high window would work AFAICS. I'll 
give that a try sometime.

> Note that on Amiga we only use the identity shortcut for Zorro III
> memory (and only for the first half?), i.e. ioremap() on Zorro II I/O
> does add new mappings.  Hence most Zorro II drivers use ZTWO_VADDR()
> instead of ioremap().

I see ZTWO_VADDR() already returns void __iomem* ... Fixing this patch 
would sort out the m68k drivers, leaving SGI. I'll have to see whether I 
can dig up a SGI workstation with this driver; might be easier than 
getting hold of any Amgias here Dowh Under.

Cheers,

	Michael


>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
>

Re: [PATCH v1 1/4] m68k - add MVME147 SCSI base address to mvme147hw.h

[Michael Schmitz]

Am 11.07.2022 um 20:45 schrieb Arnd Bergmann:
> On Mon, Jul 11, 2022 at 6:16 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
>> Am 11.07.2022 um 04:06 schrieb Arnd Bergmann:
>>> On Sat, Jul 9, 2022 at 2:10 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
>>> I think this should be an 'void *__iomem' token, not a plain integer.
>>> Apparently the driver internally uses a 'volatile void *', but some of
>>> the other front-ends are already converted to use __iomem.
>>
>> I'll pass the base address through a platform data struct in the next
>> version to address your other concerns. Haven't seen __iomem types used
>> in the other drivers - two are Zorro devices, and two platform devices
>> (a3000 and sgiwd93). Found no other wd33c93 drivers...
>
> Right, I noticed this as well. The ideal way to do this would be to change
> all of these files to use __iomem tokens consistently, and then use
> ioread32be()/iowrite32be() in place of the volatile pointer dereference.

On a second glance, the Amiga drivers _do_ use iomem types (ZTWO_VADDR() 
does the Right Thing, leaving this driver and SGI.

Changes in wd33c93.c proper (which I read your mail to suggest) would 
require actual hardware regression testing IMO. Not sure I can do that.

Cheers,

	Michael

>
> Maybe leave that for a follow-up series, you'll probably uncover
> more of these issues once you take that step.
>
>        Arnd
>

Re: [PATCH v1 1/4] m68k - add MVME147 SCSI base address to mvme147hw.h

[Brad Boyer]

On Mon, Jul 11, 2022 at 04:16:17PM +1200, Michael Schmitz wrote:
> Found no other wd33c93 drivers...

If it matters, I have a wd33c93 based NuBus card. I've just never
found the time to write a driver for it. To be honest, I haven't
even put it in a system to see if it works.

	Brad Boyer
	flar@allandria.com

Re: [PATCH v1 3/4] scsi - convert mvme146_scsi.c to platform device

[Michael Schmitz]

Hi Geert,

I just realized the commit message subject for this patch needs 
correcting. Will that mess up your workflow (assuming I retain the 
subject line for the announcement mail)?

Cheers,

	Michael


Am 12.07.2022 um 08:02 schrieb Michael Schmitz:
> Hi Geert,
>
> Am 11.07.2022 um 20:27 schrieb Geert Uytterhoeven:
>> Hi Michael,
>>
>> On Mon, Jul 11, 2022 at 9:57 AM Michael Schmitz <schmitzmic@gmail.com>
>> wrote:
>>> Am 11.07.2022 um 19:16 schrieb Geert Uytterhoeven:
>>>> On Sun, Jul 10, 2022 at 6:12 PM Arnd Bergmann <arnd@kernel.org> wrote:
>>>>> A resource would pass a phys_addr_t token, but the driver expects a
>>>>> virtual address that should be an __iomem pointer. The MMIO area
>>>>> already gets mapped into virtual addresses in arch/m68k/kernel/head.S,
>>>>> so it makes sense to skip the extra ioremap() and just use the
>>>>> address,
>>>>> but then you can't pass it as an IORESOUCRE_MEM token and should
>>>>> use platform_data with the pointer instead.
>>>
>>> OK, got it now (I had missed the physical/virtual mismatch entirely).
>>
>> And the __iomem is there so we can catch mistakes using sparse
>> (make C=2 path/to/file/.o).
>
> I'll need to make a habit of that ...
>
>>>>> The alternative is to do it the normal way and pass the physical
>>>>> address
>>>>> as a resource, that you can pass into devm_platform_ioremap_resource()
>>>>> or a similar helper.
>>>>
>>>> I would prefer the latter.  While head.S sets up the mapping,
>>>> __ioremap() does not have support for this on the mvme platform,
>>>> so this has to be added first. Look at the amiga and virt platforms
>>>> for examples.
>>>
>>> I see - doesn't look too hard to do, and should not affect any other
>>> existing code.
>>> Is it worth adding the same support for Atari as well?
>>
>> From a quick glance at arch/m68k/kernel/head.S, it seems that
>> on Atari there is no identity mapping (the high I/O area is mapped
>> to the virtual low area).  That means __ioremap() and iounmap()
>> wouldn't be symmetrical, but it can be done.
>
> As I read it, it's the other way: virtual 0xffxxxxxx is mapped to phys.
> 0x00ffxxxx, and all hardware addresses are given in the upper window
> (Medusa/Hades use that window only, and have identity mappings there).
>
> So returning identity mapping in the high window would work AFAICS. I'll
> give that a try sometime.
>
>> Note that on Amiga we only use the identity shortcut for Zorro III
>> memory (and only for the first half?), i.e. ioremap() on Zorro II I/O
>> does add new mappings.  Hence most Zorro II drivers use ZTWO_VADDR()
>> instead of ioremap().
>
> I see ZTWO_VADDR() already returns void __iomem* ... Fixing this patch
> would sort out the m68k drivers, leaving SGI. I'll have to see whether I
> can dig up a SGI workstation with this driver; might be easier than
> getting hold of any Amgias here Dowh Under.
>
> Cheers,
>
>     Michael
>
>
>>
>> Gr{oetje,eeting}s,
>>
>>                         Geert
>>
>> --
>> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
>> geert@linux-m68k.org
>>
>> In personal conversations with technical people, I call myself a
>> hacker. But
>> when I'm talking to journalists I just say "programmer" or something
>> like that.
>>                                 -- Linus Torvalds
>>

Re: [PATCH v1 3/4] scsi - convert mvme146_scsi.c to platform device

[Geert Uytterhoeven]

Hi Michael,

On Tue, Jul 12, 2022 at 5:27 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
> I just realized the commit message subject for this patch needs
> correcting. Will that mess up your workflow (assuming I retain the
> subject line for the announcement mail)?

Np, I'm used to editing subject lines all the time ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v1 3/4] scsi - convert mvme146_scsi.c to platform device

[Michael Schmitz]

Hi Geert,

thanks - v2 sent with these edits just now.

Cheers,

	Michael


Am 12.07.2022 um 18:57 schrieb Geert Uytterhoeven:
> Hi Michael,
>
> On Tue, Jul 12, 2022 at 5:27 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
>> I just realized the commit message subject for this patch needs
>> correcting. Will that mess up your workflow (assuming I retain the
>> subject line for the announcement mail)?
>
> Np, I'm used to editing subject lines all the time ;-)
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
>