Give SigmaStar and Ingenic the layout their bootloader has

app/controllers/cameras/socs_controller.rb

@@ -107,26 +107,24 @@ def update
       # Against FLASH_CHIP rather than for blankness, because a chip this site
       # does not know is not a choice either. Camera#flash_size_hex and friends
       # fall through to their 8MB branch for anything unrecognised, while
-      # @flash_type_command below would go on to render `run setnor64m` and a
-      # printenv hint naming three variables no bootloader defines. Nothing
+      # Camera#bootloader_macro_suffix would go on to render `run setnor64m` and
+      # a printenv hint naming three variables no bootloader defines. Nothing
       # calls valid? on a Camera, so this is the only thing standing between the
       # query string and the commands.
       @camera.flash_type = @camera.soc.default_flash_chip unless @camera.flash_type.in?(Camera::FLASH_CHIP)
 
-      # The bootloader macros are named after the layout, not the chip. This
-      # used to be the flash type with `nor32m` rewritten to `nor16m`, which is
-      # the same answer for every combination the menu could then produce --
-      # there is no mtdpartsnor32m anywhere upstream, so a 32MB part has always
-      # worn the 16MB layout. Camera#partition_layout says it directly now, and
-      # says it for the 8MB-layout-on-a-larger-chip case too.
-      #
       # After the flash type has settled, not before: the layout defaults to
       # the chip's own, so reading it first left the page telling a 16MB camera
       # to `run urnor16m` and then erasing from the 8MB overlay offset, 733,184
       # bytes into what it had just written. That is the failure #60 described,
       # by another route.
+      #
+      # What the macros are called is Camera's answer now rather than a separate
+      # @flash_type_command read here. It was the layout name, which is right
+      # for the bootloaders that name their macros after the layout and wrong
+      # for the two that do not name them after anything -- and the view read it
+      # four times, so the offsets and the commands could disagree.
       warn_if_layout_changed permitted_params[:partition_layout]
-      @flash_type_command = @camera.partition_layout
 
       if @vendor.name.eql?("SigmaStar") && @camera.flash_type.eql?("nand")
         render 'cameras/socs/sigmastar_nand_is_weird'

app/helpers/installation_helper.rb

@@ -147,7 +147,12 @@ def flashing_uboot(c)
     list_of_commands text
   end
 
-  def flashing_linux(c, c2)
+  # The suffix comes off the camera rather than being passed in beside it: the
+  # macros are named for the bootloader's own environment, and on SigmaStar and
+  # Ingenic that is `uknor`/`urnor` with nothing after it whatever layout is
+  # being installed.
+  def flashing_linux(c)
+    c2 = c.bootloader_macro_suffix
     text = []
     text << do_not_copy_paste
     unless c.network_interface.eql?('wifi')
@@ -179,22 +184,26 @@ def flashing_linux(c, c2)
     list_of_commands text
   end
 
-  # The three bootloader variables the instructions above actually named, for
-  # the hint that tells the reader to go and look them up. preparing_environment
-  # emits `run set…` and flashing_linux emits `run uk…; run ur…`, all from the
-  # same flash_type_command, so building the hint from it too keeps the three
-  # in step -- including the nor32m -> nor16m rewrite the controller does.
+  # The bootloader variables the instructions above actually named, for the hint
+  # that tells the reader to go and look them up. Camera builds the list from
+  # the same suffix the commands are built from, so the two stay in step --
+  # including the nor32m -> nor16m rewrite and the vendors whose macros carry no
+  # suffix and have no `set…` to name.
   #
   # It used to be a fixed `uknor*, urnor*, setnor*`, which named nothing a NAND
   # reader had been given and nothing they could find in their own printenv.
-  def bootloader_variables_html(flash_type_command)
-    safe_join(%w[uk ur set].map { |prefix| tag.code("#{prefix}#{flash_type_command}") }, ', ')
+  def bootloader_variables_html(camera)
+    safe_join(camera.bootloader_variables.map { |name| tag.code(name) }, ', ')
   end
 
-  def preparing_environment(c2)
+  # Put the bootloader on the layout that was just flashed. One macro where
+  # there is one, and the `setenv` that macro would have done where there is
+  # not -- see Camera#layout_commands. Nothing at all when the layout is already
+  # the bootloader's default, which is why every caller checks first.
+  def preparing_environment(camera)
     text = []
     text << do_not_copy_paste
-    text << "run set#{c2}"
+    text.concat(camera.layout_commands)
     list_of_commands text
   end
 

app/models/camera.rb

@@ -241,6 +241,63 @@ def partition_layout_name
     I18n.t("flash_layout.#{partition_layout}")
   end
 
+  # Whether this camera's bootloader carries one NOR mtdparts string and
+  # unsuffixed macros -- see FlashLayout. NAND is a separate environment with
+  # its own uknand/urnand/setnand and is not affected either way.
+  def fixed_mtdparts?
+    !nand? && FlashLayout.fixed_mtdparts?(soc&.vendor&.name)
+  end
+
+  # The suffix this camera's bootloader macros actually carry. `uknor8m` and
+  # friends on HiSilicon and Goke, `uknand` on NAND, and plain `uknor`/`urnor`
+  # on SigmaStar and Ingenic, whose environment has no suffixed macro at all --
+  # the page has been telling those cameras to `run uknor16m` since it first
+  # had an expert section, and U-Boot has been answering `## Error: "uknor16m"
+  # not defined` and flashing nothing.
+  def bootloader_macro_suffix
+    return 'nand' if nand?
+    return 'nor' if fixed_mtdparts?
+
+    partition_layout
+  end
+
+  # Whether the bootloader already boots this layout without being told. Every
+  # one of them defaults to the 8MB partitions, and a full-image flash leaves
+  # the env erased, so that default is what a freshly flashed camera comes up
+  # with.
+  #
+  # NAND is not one of them: mtdpartsubi is not a default anything falls back
+  # to, and a NAND camera has always been told to `run setnand` after a full
+  # image like it is now.
+  def default_bootloader_layout?
+    !nand? && layout_size <= 8
+  end
+
+  # What to run to put the bootloader on this layout, if anything.
+  #
+  # HiSilicon and Goke have a macro for it. SigmaStar and Ingenic do not: their
+  # mtdparts is one string with ${rootmtd} in it, saved unexpanded and expanded
+  # at boot by `cmdnor`, so the layout is changed by setting that variable and
+  # the erase length that goes with it. Empty when there is nothing to change,
+  # which is what rootmtd=5120k already is.
+  def layout_commands
+    return ["run set#{bootloader_macro_suffix}"] unless fixed_mtdparts?
+    return [] if default_bootloader_layout?
+
+    ["setenv rootmtd #{rootfs_max_size.to_i(16) / 1024}k; setenv rootsize #{rootfs_max_size}",
+     'saveenv', 'reset']
+  end
+
+  # The bootloader variables the instructions above actually named, for the hint
+  # that tells the reader to go and look them up. Built from the same suffix the
+  # commands are, so the two cannot drift -- and without a `set…` entry where no
+  # such variable exists, since the reader would not find it in their printenv.
+  def bootloader_variables
+    names = %w[uk ur].map { |prefix| "#{prefix}#{bootloader_macro_suffix}" }
+    names << "set#{bootloader_macro_suffix}" unless fixed_mtdparts?
+    names
+  end
+
   # The NOR numbers come from FlashLayout, which reads them off the bootloader
   # environment. They used to be spelled out here keyed on firmware_version,
   # which agreed with the bootloader only for 8MB+Lite and 16MB+Ultimate; see
@@ -249,8 +306,12 @@ def partition_layout_name
   # Keyed on the layout, not on the chip. The two agree for every combination
   # the menu offered before it grew a second field, and the whole point of the
   # second field is the ones where they do not.
+  #
+  # The vendor goes with it because two of them have a bootloader whose rootfs
+  # offset does not move between layouts. Without it a 16MB SigmaStar or Ingenic
+  # camera is handed 0x350000, which its bootloader never reads.
   def nor_layout
-    FlashLayout.nor(layout_size)
+    FlashLayout.nor(layout_size, soc&.vendor&.name)
   end
 
   def kernel_max_size

app/models/firmware.rb

@@ -448,9 +448,12 @@ def rootfs_member
   # Same table the installation page renders from, so the image and the
   # instructions cannot describe different partition layouts. Keyed on the
   # layout rather than the size, which are the same thing for every image built
-  # before the wizard could tell them apart.
+  # before the wizard could tell them apart, and on the vendor, because
+  # SigmaStar and Ingenic have one mtdparts string whose rootfs offset does not
+  # move -- an image that put it where the other table says is one their
+  # bootloader cannot boot.
   def nor_layout
-    @nor_layout ||= FlashLayout.nor(@layout)
+    @nor_layout ||= FlashLayout.nor(@layout, @soc.vendor.name)
   end
 
   def kernel_offset

app/models/flash_layout.rb

@@ -33,30 +33,69 @@ class FlashLayout
             overlay_offset: 0xD50000 }.freeze
   }.freeze
 
-  # SigmaStar and Ingenic used to be pinned to the 8MB offsets whatever chip was
-  # chosen, on the grounds that their bootloaders defined no uknor/urnor macros
-  # to check against. They do. The repositories checked were u-boot-sigmastar
-  # and u-boot-ingenic, which are not what those SoCs ship; the real ones are
-  # per-SoC, and u-boot-t20, u-boot-t40 and u-boot-msc313e all carry the same
-  # pair as the Hisilicon and Goke bootloaders above, down to the byte:
+  # SigmaStar and Ingenic do not have that pair, and the bootloaders that do are
+  # not the ones those SoCs ship.
   #
-  #   mtdpartsnor16m = 256k(boot),64k(env),3072k(kernel),10240k(rootfs),-(rootfs_data)
-  #   uknor16m : sf erase 0x50000 0x300000    urnor16m : sf erase 0x350000 0xa00000
+  # OpenIPC/firmware's .github/workflows/uboot.yml is what builds the binaries
+  # this site links. It clones openipc/u-boot-sigmastar and openipc/u-boot-
+  # ingenic and runs their build.sh; the SigmaStar one's spinor loop is
+  # `ssc377 ssc377d ssc377de ssc377qe ssc378de ssc378qe` against
+  # include/configs/infinity6c.h, which includes configs/sstar-common.h. That
+  # header is the entire environment every SSC3xx NOR camera boots with:
   #
-  # Only the mtd device name differs -- jz_sfc, NOR_FLASH, sfc.
+  #   kernaddr=0x50000   kernsize=0x200000
+  #   rootaddr=0x250000  rootsize=0x500000   rootmtd=5120k
+  #   uknor / urnor / ubnor            <- no size suffix, and no setnor* at all
+  #   CONFIG_BOOTARGS "... mtdparts=NOR_FLASH:256k(boot),64k(env),2048k(kernel),
+  #                        ${rootmtd}(rootfs),-(rootfs_data) ..."
   #
-  # The pin also could not survive Ultimate on 16MB. Ultimate's NOR rootfs is
-  # 7820KB on ssc338q and 6752KB on t31, and the 8MB layout gives rootfs 5120KB.
-  # There is no arrangement in which those two vendors offer Ultimate on a 16MB
-  # chip and keep the 8MB geometry.
+  # A repo-wide grep for uknor8m|uknor16m|urnor8m|urnor16m|setnor8m|setnor16m|
+  # mtdpartsnor returns nothing in either repo; u-boot-ingenic's
+  # include/configs/isvp_common.h carries the same unsuffixed uknor/urnor.
+  # u-boot-msc313e, u-boot-t20 and u-boot-t40 do define the suffixed pair, which
+  # is what the note this replaces was reading -- but no released binary is
+  # built from them, so no camera runs them.
   #
-  # This is the one place the chip size decides, for every vendor. What has to
-  # travel with it is the instruction to run `setnor16m`: every one of these
-  # bootloaders defaults mtdparts to the 8MB layout, and flashing a full image
-  # leaves the env erased, so a 16MB camera that is never told to switch boots
-  # with 8MB partitions. update.html.erb used to suppress that instruction for
-  # these same two vendors and no longer does.
-  def self.nor(flash_size_mb)
-    flash_size_mb.to_i <= 8 ? NOR[8] : NOR[16]
+  # So these two have one mtdparts string, the kernel partition is 2048k inside
+  # it, and the only thing that varies is ${rootmtd}. The rootfs starts at
+  # 0x250000 at every chip size; what a larger chip buys is a longer rootfs, not
+  # one further up.
+  #
+  # Handing them NOR[16] put the rootfs at 0x350000, which is where the images
+  # openipc.org serves today for ssc377qe have it: layout=16 carries "hsqs" at
+  # 0x350000 and 0xff at 0x250000, and its env region at 0x40000 is blank, so
+  # the camera comes up on the compiled-in bootargs, looks for the rootfs at
+  # 0x250000 and panics on root mount. layout=8 has it at 0x250000 and boots.
+  FIXED_MTDPARTS_VENDORS = %w[SigmaStar Ingenic].freeze
+
+  # The same two questions the table above answers, for a bootloader whose
+  # rootfs cannot move. 16 is not a different partition map, it is `rootmtd`
+  # and the erase length that goes with it set to 10240k -- which is also the
+  # only way an Ultimate rootfs fits: 7832KB on ssc338q, 7252KB on ssc30kq and
+  # 6772KB on t31, against the 5120KB the default leaves.
+  FIXED_MTDPARTS_NOR = {
+    8 => { kernel_offset: 0x50000, kernel_max_size: 0x200000,
+           rootfs_offset: 0x250000, rootfs_max_size: 0x500000,
+           overlay_offset: 0x750000 }.freeze,
+    16 => { kernel_offset: 0x50000, kernel_max_size: 0x200000,
+            rootfs_offset: 0x250000, rootfs_max_size: 0xA00000,
+            overlay_offset: 0xC50000 }.freeze
+  }.freeze
+
+  # Whether this vendor's NOR bootloader has one mtdparts string rather than a
+  # mtdpartsnor8m/mtdpartsnor16m pair to switch between.
+  def self.fixed_mtdparts?(vendor_name)
+    FIXED_MTDPARTS_VENDORS.include?(vendor_name.to_s)
+  end
+
+  # The chip size decides which entry, and the vendor decides which table. What
+  # has to travel with the 16MB entry either way is the instruction to put the
+  # bootloader on it: all of these default to the 8MB partitions -- mtdparts on
+  # HiSilicon and Goke, rootmtd=5120k on SigmaStar and Ingenic -- and flashing a
+  # full image leaves the env erased, so that default is what boots. See
+  # Camera#layout_commands for what each family is told to run.
+  def self.nor(flash_size_mb, vendor_name = nil)
+    table = fixed_mtdparts?(vendor_name) ? FIXED_MTDPARTS_NOR : NOR
+    table[flash_size_mb.to_i <= 8 ? 8 : 16]
   end
 end

app/views/cameras/socs/update.html.erb

@@ -76,17 +76,15 @@
           <% end %>
           <%= flashing_everything(@camera) %>
           <p><%= t('firmware.installation.flashing_full.continue') %></p>
-          <%# SigmaStar and Ingenic were exempted here in 64d37db and 00332d1 and
-              are not any more. Their bootloaders default mtdparts to the 8MB
-              layout like every other one, a full-image flash leaves the env
-              erased so that default is what the camera boots with, and the
-              expert section below has always told them to run setnor16m
-              regardless -- so the exemption made this page contradict itself.
-              It also cannot coexist with Ultimate on 16MB: that rootfs is
-              7820KB on ssc338q against the 5120KB the 8MB layout allows. %>
-          <% unless @flash_type_command.eql?('nor8m') %>
+          <%# Every one of these bootloaders defaults mtdparts to the 8MB layout
+              and a full-image flash leaves the env erased, so that default is
+              what the camera boots with and anything else has to be set here.
+              Asked of the camera rather than of the layout name: SigmaStar and
+              Ingenic have no set* macro to run, and what they are given instead
+              is the setenv that macro would have done. %>
+          <% unless @camera.default_bootloader_layout? %>
             <p><%= t('firmware.installation.flashing_full.continue2') %></p>
-            <%= preparing_environment(@flash_type_command) %>
+            <%= preparing_environment(@camera) %>
           <% end %>
         </div>
       </div>
@@ -126,15 +124,21 @@
         </div>
       </div>
 
-      <h3 class="mb-4 fw-bold"><%= t('firmware.installation.flashing_footfs.title') %></h3>
-      <div class="row">
-        <div class="col col-lg-4"></div>
-        <div class="col col-lg-8">
-          <p><%= t('firmware.installation.flashing_footfs.info') %></p>
-          <%= preparing_environment(@flash_type_command) %>
-          <p><%= t('firmware.installation.flashing_footfs.continue') %></p>
+      <%# Skipped where there is nothing to run. This step used to render
+          unconditionally, so a SigmaStar or Ingenic reader was given `run
+          setnor8m` -- a variable their bootloader does not define -- under a
+          heading telling them it was required. %>
+      <% if @camera.layout_commands.any? %>
+        <h3 class="mb-4 fw-bold"><%= t('firmware.installation.flashing_footfs.title') %></h3>
+        <div class="row">
+          <div class="col col-lg-4"></div>
+          <div class="col col-lg-8">
+            <p><%= t('firmware.installation.flashing_footfs.info') %></p>
+            <%= preparing_environment(@camera) %>
+            <p><%= t('firmware.installation.flashing_footfs.continue') %></p>
+          </div>
         </div>
-      </div>
+      <% end %>
 
       <h3 class="mb-4 fw-bold"><%= t('firmware.installation.flashing_footfs2.title') %></h3>
       <div class="row">
@@ -154,7 +158,7 @@
               <p class="mb-0"><%= t('.sdcard_required_3') %></p>
             </div>
           <% end %>
-          <%= flashing_linux(@camera, @flash_type_command) %>
+          <%= flashing_linux(@camera) %>
         </div>
       </div>
     </div>
@@ -174,7 +178,7 @@
     <% end %>
   </div>
 
-  <p><%= t('firmware.info_html', commands: bootloader_variables_html(@flash_type_command)) %></p>
+  <p><%= t('firmware.info_html', commands: bootloader_variables_html(@camera)) %></p>
 
   <a data-bs-toggle="collapse" href="#collapseExperts" role="button" aria-expanded="false" aria-controls="collapseExperts"><%= t('.advanced_instruction_link') %></a>
 </article>